feat(docs): add local authoring and scripting workflows - #1921
Open
SunPeiYang996 wants to merge 1 commit into
Open
feat(docs): add local authoring and scripting workflows#1921SunPeiYang996 wants to merge 1 commit into
SunPeiYang996 wants to merge 1 commit into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds ChangesDocumentation workflows and infrastructure
Estimated code review effort: 5 (Critical) | ~120 minutes Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant CLI
participant DocsScript
participant Docxparse
participant ResourceHandler
participant DocsAPI
CLI->>DocsScript: Run parse or conversion command
DocsScript->>Docxparse: Parse XML or Markdown
Docxparse-->>DocsScript: Return XML and profile
DocsScript->>ResourceHandler: Prepare local or remote resources
ResourceHandler->>DocsAPI: Upload and bind document resources
DocsAPI-->>DocsScript: Return document response
DocsScript-->>CLI: Return profile, XML, warnings, or paths
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
domain/ccm
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@d17c0e9dfb459f48d84ec4f431f27a5c499f5cb3🧩 Skill updatenpx skills add larksuite/cli#sun/tempv2 -y -g |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (8)
internal/vfs/localfileio/localfileio.go-85-92 (1)
85-92: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winDo not discard temporary-directory cleanup failures.
If
RemoveAllfails, this returns no path while leaving an orphaned directory or file. Include the cleanup failure in the returned typed error so the write is not silently discarded.As per coding guidelines, filesystem writes must not be discarded silently.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/vfs/localfileio/localfileio.go` around lines 85 - 92, Update the error paths in the temporary-file creation and close handling around tempFile.Close to capture failures from vfs.RemoveAll instead of discarding them. Return a fileio.WriteError that preserves both the original write/close error and cleanup failure, while retaining the existing cleanup attempt and error context.Source: Coding guidelines
skills/lark-doc/references/lark-doc-script.md-66-84 (1)
66-84: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument the
--outputand--overwritecontract.Add an example and the saved response fields (
saved_path,size_bytes), including existing-file behavior. This section currently documents only the inlinedata.xmlflow.As per coding guidelines, CLI flags and output formats are machine-consumed interfaces and must be documented specifically.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/lark-doc/references/lark-doc-script.md` around lines 66 - 84, 补充“Markdown 转 XML”部分对 --output 和 --overwrite 的契约说明：增加将转换结果保存到文件的命令示例，并记录成功响应中的 saved_path 和 size_bytes 字段；同时明确目标文件已存在时未启用 --overwrite 与启用该选项的行为。保留现有内联 data.xml 响应说明，并确保 CLI 参数及输出格式描述准确。Source: Coding guidelines
shortcuts/doc/docs_script.go-121-122 (1)
121-122: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winReject surrounding whitespace instead of changing the output path.
strings.TrimSpacesilently turns a requested path such as" draft.xml"into"draft.xml". Compare the raw value with its trimmed form and return a typed--outputvalidation error, as already done for--file-name.As per coding guidelines, requested CLI values must not be silently coerced.
Also applies to: 175-188, 220-222, 266-267
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shortcuts/doc/docs_script.go` around lines 121 - 122, Update the output-path validation around outputPath and the related output handling at the referenced locations to preserve the raw --output value, compare it with its trimmed form, and return the existing typed --output validation error when surrounding whitespace is present. Match the established --file-name validation behavior without silently coercing the requested path.Source: Coding guidelines
skills/lark-doc/references/lark-doc-whiteboard.md-32-34 (1)
32-34: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winResolve the contradictory SVG ownership rule.
Lines 32-34 allow simple SVG to be written directly by the main Agent, but Step 2B still says SVG insertion uses a SubAgent and instructs the main Agent to start one. Rewrite Step 2B so simple SVG follows the direct path and only design-heavy or layout-sensitive SVG is delegated.
Proposed clarification
-### 步骤 2B: SubAgent 使用 SVG 插入图表 +### 步骤 2B: 使用 SVG 插入图表 -主 Agent 启动 SubAgent，让它用 `docs +create` / `docs +update` 插入： +简单 SVG 由主 Agent 直接写入本地 XML；需要专门视觉设计、较高信息密度或容易布局失败的 SVG，再由主 Agent 启动 SubAgent 使用 `docs +create` / `docs +update` 插入：🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/lark-doc/references/lark-doc-whiteboard.md` around lines 32 - 34, 更新步骤 2B 的 SVG 插入规则，消除其与前文的冲突：简单 SVG 由主 Agent 直接写入本地 XML，只有需要专门视觉设计、信息密度较高或布局容易出错的复杂 SVG 才启动 SubAgent 生成完整片段，并保留其他图表类型的既有流程。skills/lark-doc/references/lark-doc-update.md-86-103 (1)
86-103: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDocument
okas the envelope flag andresultas the business outcome. Inskills/lark-doc/references/lark-doc-update.md:86-103, spell out whetherpartial_success/failedstill returnok: true, and which field clients should branch on.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/lark-doc/references/lark-doc-update.md` around lines 86 - 103, Clarify the response documentation around the result table and JSON example by adding the envelope-level ok field and explicitly distinguishing it from the business-level result field. State whether partial_success and failed responses still use ok: true, and instruct clients to branch on result for update outcomes while using ok only for envelope/request success.Source: Coding guidelines
skills/lark-doc/references/lark-doc-xml-extended-blocks.md-6-14 (1)
6-14: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClarify that these extended-tag lines are signatures, not copyable XML. The placeholders in
<figure view-type>,<button action=OpenLink|...>, etc. should be quoted and filled with real values, or this section should explicitly say it’s a signature list so readers don’t paste invalid markup.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/lark-doc/references/lark-doc-xml-extended-blocks.md` around lines 6 - 14, Clarify in the extended-blocks section that the listed tags are signatures, not directly copyable XML examples. Mark placeholder attributes and values as illustrative or document the required quoting and replacement with real values, especially in the figure, button, time, sheet, task, chat_card, and okr entries.shortcuts/doc/internal/docxparse/markdown_cjk.go-27-39 (1)
27-39: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCheck fence indentation before trimming it.
Both preprocessors classify four-space-indented backticks as fences, although GFM treats them as indented code. This leaves fence state active and suppresses later transformations. (github.github.com)
shortcuts/doc/internal/docxparse/markdown_cjk.go#L27-L39: enter or close fence state only when indentation after any blockquote prefix is at most three spaces.shortcuts/doc/internal/docxparse/markdown.go#L692-L699: apply the same eligibility check before togglinginFence.As per coding guidelines, transcribe supported input faithfully rather than silently reclassifying it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shortcuts/doc/internal/docxparse/markdown_cjk.go` around lines 27 - 39, Update the fence detection in shortcuts/doc/internal/docxparse/markdown_cjk.go at lines 27-39 to check indentation after any blockquote prefix before entering or closing fenceMarker state; only allow up to three spaces, while preserving four-space-indented backticks as code. Apply the same eligibility check in shortcuts/doc/internal/docxparse/markdown.go at lines 692-699 before toggling inFence, keeping both preprocessors consistent.Source: Coding guidelines
shortcuts/doc/internal/docxparse/markdown.go-200-205 (1)
200-205: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDetect task checkboxes anywhere in the list. If the first item is plain but a later item is a task,
renderListfalls back to normal list mode and drops the checkbox marker.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shortcuts/doc/internal/docxparse/markdown.go` around lines 200 - 205, Update isTaskList to inspect every list item rather than only list.FirstChild(), returning true when findTaskCheckbox identifies a checkbox on any item and false when none do. Preserve safe handling of empty lists and non-list-item children, and ensure renderList uses task-list mode when a later item contains a task checkbox.
🧹 Nitpick comments (1)
internal/vfs/localfileio/localfileio_test.go (1)
309-316: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover every new validation rule directly.
Add table cases for empty patterns/names, patterns with zero or multiple
*, and control/dangerous Unicode characters. The current cases only prove separator rejection.As per coding guidelines, contract tests must assert each new behavior directly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/vfs/localfileio/localfileio_test.go` around lines 309 - 316, Expand the table-driven cases in the local file validation tests to cover empty patterns and file names, patterns containing zero or multiple asterisks, and control or dangerous Unicode characters. Assert each validation rule directly while retaining the existing separator-rejection cases.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 67-82: Update the results job to include windows-compat in its
needs dependency, summary table, and failure-evaluation loop. Reuse the existing
job-result formatting and failure handling used for the other checks so Windows
failures prevent the aggregate results gate from passing.
In `@shortcuts/doc/docs_script.go`:
- Around line 346-379: Replace the map-based request and response handling in
docsScriptFetchBody and fetchDocsScriptContent with typed request/response
structs, including the nested export options and document content fields. Route
the fetch request through runtime.CallAPITyped instead of doDocAPI, preserving
the existing endpoint, method, language behavior, and return the typed API error
unchanged.
- Around line 382-405: Update saveDocsScriptXML and the FileIO SaveOptions
contract to support exclusive creation when overwrite is false. Pass the
no-replace/exclusive-create option for the non-overwrite path so the write
itself atomically fails if outputPath is created concurrently, while retaining
replacement behavior when overwrite is true and mapping the exclusive-create
failure through the existing save-error handling.
In `@shortcuts/doc/internal/docxparse/markdown_cjk.go`:
- Around line 148-154: Update the CJK markup rewrite logic around
shouldRewriteCJKMarkup so content containing nested Markdown delimiters (* , _ ,
or ~) is not flattened into escaped text; skip rewriting such spans or
recursively process the inner fragment. Preserve supported nested
emphasis/strike structure, including cases like nested emphasis inside a CJK
span, and apply the same fix to the corresponding later rewrite block.
In `@shortcuts/doc/internal/docxparse/markdown_extensions.go`:
- Around line 93-108: The scanMathClose function must skip delimiter candidates
that are escaped, including cases with an odd number of preceding backslashes.
Replace the direct bytes.Index acceptance flow with candidate-by-candidate
validation of preceding backslash parity, continuing the search until an
unescaped delimiter is found while preserving the existing latex-tag and
no-close behavior.
- Around line 294-311: The containerBlock parser must reject blocks that reach
EOF without an explicit closing tag instead of allowing Goldmark to auto-close
them. Update containerBlock and its parser lifecycle, including Continue and
Close, to track whether the matching close tag was consumed, then return the
established validation error when the block ends unclosed while preserving
normal parsing for explicitly closed containers.
In `@shortcuts/doc/internal/docxparse/markdown.go`:
- Around line 130-136: Update the fallback handling in renderBlockNode and
renderInlineNode to return a validation error for unsupported Goldmark node
kinds instead of calling extractMarkdownText and flattening them into plain
text. Preserve existing rendering for explicitly supported nodes and ensure the
error identifies the unhandled node kind.
In `@shortcuts/doc/internal/docxparse/profile.go`:
- Around line 104-109: Update detectFormat to recognize valid Markdown autolinks
such as URI and email forms before the generic strings.HasPrefix(trimmed, "<")
XML check. Return FormatMarkdown for those autolinks, while preserving FormatXML
detection for other leading-angle-bracket content and the existing
BOM/whitespace handling.
- Around line 360-364: Update the node-display handling around the current !=
node branch to traverse visible child text before checking metadata attributes
such as text, name, title, or alt. Only write and return the attribute-derived
display when child traversal produces no visible text, ensuring anchor content
like “Click here” is preferred over its title.
In `@shortcuts/doc/internal/docxparse/schema.go`:
- Around line 228-233: Update the rule transformation handling in the schema
parsing flow so a false result from rule.transform does not continue with the
original alias value. Instead, return and propagate the established typed
validation error for the unsupported value, ensuring invalid aliases such as
column width are rejected rather than silently ignored.
- Around line 166-169: Add "i" to the strictPhrasingTags map so italic elements
are enforced as phrasing-only and cannot contain structural elements such as
tables. Add regression coverage for an <i> element containing a table, verifying
structural validation rejects it.
In `@shortcuts/doc/internal/docxparse/wordcount.go`:
- Around line 64-69: Update textCounter.write and the related ASCII compound
handling so each maximal ASCII/alphanumeric run is consumed and classified once,
rather than repeatedly calling matchASCIICompound on every remaining suffix.
Preserve connector-based compound detection and existing token counting behavior
while eliminating the quadratic scan for long runs.
In `@shortcuts/doc/internal/docxparse/xml.go`:
- Around line 21-29: Update validateSource to detect DOCTYPE and ENTITY
declarations through XML token scanning rather than matching raw source text.
Ignore occurrences inside comments and CDATA sections, while still rejecting
active declarations and preserving the existing input-size validation and error
behavior.
- Around line 153-155: Update the newline normalization in the default branch
handling child.text to replace CR/LF characters with whitespace rather than
deleting them, preserving a word boundary such as “Hello world” before the
existing non-empty check.
In `@skills/lark-doc/references/genres/route-personal-brand.md`:
- Line 14: 更新个人品牌路由条件，明确将邮件草稿也归入 route_platform，并要求选择对应的 email
leaf；保留现有对小红书笔记和微信公众号文章的路由规则，以及个人身份、经历和信誉目标作为 leaf contract 硬约束的要求。
In `@skills/lark-doc/references/lark-doc-create-workflow.md`:
- Line 42: Remove the requirement in Step 3 to display the Presentation Decision
in chain-of-thought, and replace it with a concise user-visible decision summary
or structured JSON record containing only the key decisions without private
reasoning.
- Line 48: Update the JSON contract example near presentation_mode so its value
is one of the declared modes—formal, normal, or rich—rather than explanatory
prose. Move the “contract 为 none 时默认 rich” rule into surrounding documentation
while keeping the contract itself machine-valid.
In `@skills/lark-doc/references/lark-doc-create.md`:
- Line 56: Update skills/lark-doc/references/lark-doc-create.md:56 so multiple
document titles are rejected with a typed, actionable validation error rather
than silently retaining the first. Update
skills/lark-doc/references/lark-doc-fetch.md:65 so context is applied to nested
matches, or the incompatible option is explicitly rejected; do not document
silently ignoring requested context.
In `@skills/lark-doc/references/lark-doc-fetch.md`:
- Line 110: Update the image/source fetching flow for <img> and <source> so
direct GET is allowed only for trusted HTTPS URLs. Validate the initial URL
scheme and reject private, loopback, and link-local hosts, then inspect every
redirect destination with the same checks before following it; otherwise use the
existing token-based preview/download flow.
In `@tests/cli_e2e/docs/docs_script_test.go`:
- Around line 310-315: Update docsScriptE2EEnv to accept *testing.T, set
LARKSUITE_CLI_CONFIG_DIR to t.TempDir(), and preserve the existing environment
entries. Update every caller of docsScriptE2EEnv to pass its test handle so
these non-live E2E tests use isolated CLI configuration.
---
Minor comments:
In `@internal/vfs/localfileio/localfileio.go`:
- Around line 85-92: Update the error paths in the temporary-file creation and
close handling around tempFile.Close to capture failures from vfs.RemoveAll
instead of discarding them. Return a fileio.WriteError that preserves both the
original write/close error and cleanup failure, while retaining the existing
cleanup attempt and error context.
In `@shortcuts/doc/docs_script.go`:
- Around line 121-122: Update the output-path validation around outputPath and
the related output handling at the referenced locations to preserve the raw
--output value, compare it with its trimmed form, and return the existing typed
--output validation error when surrounding whitespace is present. Match the
established --file-name validation behavior without silently coercing the
requested path.
In `@shortcuts/doc/internal/docxparse/markdown_cjk.go`:
- Around line 27-39: Update the fence detection in
shortcuts/doc/internal/docxparse/markdown_cjk.go at lines 27-39 to check
indentation after any blockquote prefix before entering or closing fenceMarker
state; only allow up to three spaces, while preserving four-space-indented
backticks as code. Apply the same eligibility check in
shortcuts/doc/internal/docxparse/markdown.go at lines 692-699 before toggling
inFence, keeping both preprocessors consistent.
In `@shortcuts/doc/internal/docxparse/markdown.go`:
- Around line 200-205: Update isTaskList to inspect every list item rather than
only list.FirstChild(), returning true when findTaskCheckbox identifies a
checkbox on any item and false when none do. Preserve safe handling of empty
lists and non-list-item children, and ensure renderList uses task-list mode when
a later item contains a task checkbox.
In `@skills/lark-doc/references/lark-doc-script.md`:
- Around line 66-84: 补充“Markdown 转 XML”部分对 --output 和 --overwrite
的契约说明：增加将转换结果保存到文件的命令示例，并记录成功响应中的 saved_path 和 size_bytes 字段；同时明确目标文件已存在时未启用
--overwrite 与启用该选项的行为。保留现有内联 data.xml 响应说明，并确保 CLI 参数及输出格式描述准确。
In `@skills/lark-doc/references/lark-doc-update.md`:
- Around line 86-103: Clarify the response documentation around the result table
and JSON example by adding the envelope-level ok field and explicitly
distinguishing it from the business-level result field. State whether
partial_success and failed responses still use ok: true, and instruct clients to
branch on result for update outcomes while using ok only for envelope/request
success.
In `@skills/lark-doc/references/lark-doc-whiteboard.md`:
- Around line 32-34: 更新步骤 2B 的 SVG 插入规则，消除其与前文的冲突：简单 SVG 由主 Agent 直接写入本地
XML，只有需要专门视觉设计、信息密度较高或布局容易出错的复杂 SVG 才启动 SubAgent 生成完整片段，并保留其他图表类型的既有流程。
In `@skills/lark-doc/references/lark-doc-xml-extended-blocks.md`:
- Around line 6-14: Clarify in the extended-blocks section that the listed tags
are signatures, not directly copyable XML examples. Mark placeholder attributes
and values as illustrative or document the required quoting and replacement with
real values, especially in the figure, button, time, sheet, task, chat_card, and
okr entries.
---
Nitpick comments:
In `@internal/vfs/localfileio/localfileio_test.go`:
- Around line 309-316: Expand the table-driven cases in the local file
validation tests to cover empty patterns and file names, patterns containing
zero or multiple asterisks, and control or dangerous Unicode characters. Assert
each validation rule directly while retaining the existing separator-rejection
cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 468eb50f-3a72-49c3-a7ee-2443eea468ee
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (64)
.github/workflows/ci.yml.gitignoreextension/fileio/types.gogo.modinternal/vfs/localfileio/localfileio.gointernal/vfs/localfileio/localfileio_test.goshortcuts/doc/docs_script.goshortcuts/doc/docs_script_test.goshortcuts/doc/internal/docxparse/markdown.goshortcuts/doc/internal/docxparse/markdown_cjk.goshortcuts/doc/internal/docxparse/markdown_extensions.goshortcuts/doc/internal/docxparse/model.goshortcuts/doc/internal/docxparse/parse_test.goshortcuts/doc/internal/docxparse/profile.goshortcuts/doc/internal/docxparse/schema.goshortcuts/doc/internal/docxparse/wordcount.goshortcuts/doc/internal/docxparse/xml.goshortcuts/doc/internal/docxparse/xml_compat.goshortcuts/doc/shortcuts.goskills/lark-doc/SKILL.mdskills/lark-doc/references/genres/business-analysis.mdskills/lark-doc/references/genres/data-report.mdskills/lark-doc/references/genres/email.mdskills/lark-doc/references/genres/execution-plan.mdskills/lark-doc/references/genres/formal-doc.mdskills/lark-doc/references/genres/meeting-minutes.mdskills/lark-doc/references/genres/memo-brief.mdskills/lark-doc/references/genres/official-redhead.mdskills/lark-doc/references/genres/prd.mdskills/lark-doc/references/genres/proposal.mdskills/lark-doc/references/genres/research-report.mdskills/lark-doc/references/genres/retrospective.mdskills/lark-doc/references/genres/route-consumer.mdskills/lark-doc/references/genres/route-creative.mdskills/lark-doc/references/genres/route-knowledge.mdskills/lark-doc/references/genres/route-marketing.mdskills/lark-doc/references/genres/route-media.mdskills/lark-doc/references/genres/route-opinion.mdskills/lark-doc/references/genres/route-personal-brand.mdskills/lark-doc/references/genres/route-platform.mdskills/lark-doc/references/genres/route-report.mdskills/lark-doc/references/genres/route-workplace.mdskills/lark-doc/references/genres/sop-tutorial.mdskills/lark-doc/references/genres/technical-doc.mdskills/lark-doc/references/genres/wechat.mdskills/lark-doc/references/genres/weekly-report.mdskills/lark-doc/references/genres/white-paper.mdskills/lark-doc/references/genres/xiaohongshu.mdskills/lark-doc/references/lark-doc-create-workflow.mdskills/lark-doc/references/lark-doc-create.mdskills/lark-doc/references/lark-doc-fetch.mdskills/lark-doc/references/lark-doc-script.mdskills/lark-doc/references/lark-doc-update.mdskills/lark-doc/references/lark-doc-whiteboard.mdskills/lark-doc/references/lark-doc-word-stat.mdskills/lark-doc/references/lark-doc-xml-extended-blocks.mdskills/lark-doc/references/lark-doc-xml.mdskills/lark-doc/references/style/lark-doc-create-workflow.mdskills/lark-doc/references/style/lark-doc-style.mdskills/lark-doc/references/style/lark-doc-update-workflow.mdskills/lark-doc/scripts/doc_word_stat.pytests/cli_e2e/docs/coverage.mdtests/cli_e2e/docs/docs_create_fetch_test.gotests/cli_e2e/docs/docs_script_test.go
💤 Files with no reviewable changes (5)
- skills/lark-doc/references/style/lark-doc-update-workflow.md
- skills/lark-doc/references/style/lark-doc-style.md
- skills/lark-doc/references/style/lark-doc-create-workflow.md
- skills/lark-doc/references/lark-doc-word-stat.md
- skills/lark-doc/scripts/doc_word_stat.py
| - [`lark-doc-media-insert.md`](lark-doc-media-insert.md) — 插入图片/文件到文档 | ||
| |参数|必填|说明| | ||
| |-|-|-| | ||
| |`--title`|否|文档标题，Markdown 导入时使用；XML 创建推荐在 `--content` 开头写 `<title>...</title>`；多个标题仅保留第一个并在 `warnings` / `degrade_details` 提示| |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not silently discard or ignore requested document content.
These documented fallbacks violate the repository contract: unsupported or ambiguous input must be rejected with a typed, actionable validation error, or honored without loss.
skills/lark-doc/references/lark-doc-create.md#L56-L56: reject multiple titles instead of retaining only the first.skills/lark-doc/references/lark-doc-fetch.md#L65-L65: either apply context to nested matches or reject the incompatible option instead of ignoring it.
📍 Affects 2 files
skills/lark-doc/references/lark-doc-create.md#L56-L56(this comment)skills/lark-doc/references/lark-doc-fetch.md#L65-L65
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@skills/lark-doc/references/lark-doc-create.md` at line 56, Update
skills/lark-doc/references/lark-doc-create.md:56 so multiple document titles are
rejected with a typed, actionable validation error rather than silently
retaining the first. Update skills/lark-doc/references/lark-doc-fetch.md:65 so
context is applied to nested matches, or the incompatible option is explicitly
rejected; do not document silently ignoring requested context.
Source: Coding guidelines
| 返回中可能含 `<sheet>`、`<bitable>`、`<cite file-type="sheets|bitable">`。内部数据无法通过 `docs +fetch` 获取，提取 `token` 等属性后切到 [`lark-sheets`](../../lark-sheets/SKILL.md) / [`lark-base`](../../lark-base/SKILL.md) 下钻，详见 [SKILL.md 快速决策](../SKILL.md) 路由表。 | ||
| |返回内容|处理方式| | ||
| |-|-| | ||
| |`<img>`、`<source>`|有 `url` 时直接 HTTP GET；否则提取 `token`，预览用 `docs +media-preview`，下载用 `docs +media-download`| |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="skills/lark-doc/references/lark-doc-fetch.md"
echo "== file metadata =="
wc -l "$file"
echo
echo "== relevant lines =="
sed -n '90,130p' "$file" | cat -n
echo
echo "== search for direct HTTP GET / url handling references =="
rg -n "HTTP GET|url|media-preview|media-download|source|img" skills/lark-doc -SRepository: larksuite/cli
Length of output: 12091
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== lark-doc-xml relevant section =="
sed -n '1,80p' skills/lark-doc/references/lark-doc-xml.md | cat -n
echo
echo "== media-preview / media-download sections =="
sed -n '1,120p' skills/lark-doc/references/lark-doc-media-preview.md | cat -n
echo
sed -n '1,120p' skills/lark-doc/references/lark-doc-media-download.md | cat -n
echo
echo "== resource-cover url constraints =="
sed -n '1,90p' skills/lark-doc/references/lark-doc-resource-cover.md | cat -nRepository: larksuite/cli
Length of output: 9384
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== lark-shared security rules =="
sed -n '1,220p' skills/lark-shared/SKILL.md | cat -n
echo
echo "== fetch doc surrounding the affected line =="
sed -n '100,116p' skills/lark-doc/references/lark-doc-fetch.md | cat -n
echo
echo "== search for SSRF / URL restriction guidance across skills =="
rg -n "SSRF|private|loopback|link-local|multicast|userinfo|https://|HTTP GET|direct HTTP GET|publicly|安全边界|URL" skills -SRepository: larksuite/cli
Length of output: 50372
Restrict direct GET to trusted HTTPS URLs. If url can come from document content, fetching it directly can hit internal endpoints; require https://, reject private/loopback/link-local hosts, and re-check redirects before following the link.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@skills/lark-doc/references/lark-doc-fetch.md` at line 110, Update the
image/source fetching flow for <img> and <source> so direct GET is allowed only
for trusted HTTPS URLs. Validate the initial URL scheme and reject private,
loopback, and link-local hosts, then inspect every redirect destination with the
same checks before following it; otherwise use the existing token-based
preview/download flow.
SunPeiYang996
force-pushed
the
sun/tempv2
branch
from
July 24, 2026 08:33
147ac37 to
f13ff1b
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
cmd/build_bench_test.go (2)
100-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the stats test fail on regressions.
The test only logs counts, so any command-tree change still passes. Add stable expected-count or invariant assertions, or make this a diagnostic helper rather than a test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/build_bench_test.go` around lines 100 - 104, Update the command-tree stats test around the existing totalCmds, runnableCmds, groupCmds, and totalFlags logging so it actively detects regressions by asserting stable expected counts or documented invariants. Preserve the logs for diagnostics, and ensure mismatches fail the test rather than merely being reported.
31-42: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winHoist benchmark options outside the timed loops.
Lines 41, 54, and 68 construct the
BuildOptionon every iteration, so the reported deltas include option construction rather than isolatingBuildcost. Create each option beforeb.ResetTimer()and reuse it.Also applies to: 45-55, 58-69
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/build_bench_test.go` around lines 31 - 42, Move the WithoutServiceCommands() BuildOption construction in BenchmarkBuild_WithoutServiceCommands and the other affected benchmark functions outside their timed loops, before b.ResetTimer(). Store each option in a local variable and reuse it in every Build call so timing measures Build rather than option creation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Line 106: Update the Windows checkout step using actions/checkout to set
persist-credentials to false, matching the integration workflows while leaving
the pinned action reference unchanged.
In `@cmd/build_bench_test.go`:
- Around line 72-76: Align TestBuild_CommandTreeStats with its documented
default-build contract by removing the WithoutPlugins() option from the Build
call. Keep the existing test name and comments, so the test measures the default
command tree including normal plugin behavior.
- Around line 89-93: Update the flag-counting logic in the command tree walk
around cmd.Flags() and totalFlags so inherited and local entries are
deduplicated before logging. Maintain a shared set of unique flag identifiers
across the walk, incrementing totalFlags only when a flag is first encountered,
so persistent flags such as --profile contribute once.
---
Nitpick comments:
In `@cmd/build_bench_test.go`:
- Around line 100-104: Update the command-tree stats test around the existing
totalCmds, runnableCmds, groupCmds, and totalFlags logging so it actively
detects regressions by asserting stable expected counts or documented
invariants. Preserve the logs for diagnostics, and ensure mismatches fail the
test rather than merely being reported.
- Around line 31-42: Move the WithoutServiceCommands() BuildOption construction
in BenchmarkBuild_WithoutServiceCommands and the other affected benchmark
functions outside their timed loops, before b.ResetTimer(). Store each option in
a local variable and reuse it in every Build call so timing measures Build
rather than option creation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 11942ae5-d055-43d9-aef6-1d22e37864c5
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (20)
.github/workflows/ci.yml.gitignorecmd/build_bench_test.goextension/fileio/types.gogo.modinternal/vfs/localfileio/localfileio.gointernal/vfs/localfileio/localfileio_test.goshortcuts/doc/docs_script.goshortcuts/doc/docs_script_test.goshortcuts/doc/internal/docxparse/markdown.goshortcuts/doc/internal/docxparse/markdown_cjk.goshortcuts/doc/internal/docxparse/markdown_extensions.goshortcuts/doc/internal/docxparse/model.goshortcuts/doc/internal/docxparse/parse_test.goshortcuts/doc/internal/docxparse/profile.goshortcuts/doc/internal/docxparse/schema.goshortcuts/doc/internal/docxparse/wordcount.goshortcuts/doc/internal/docxparse/xml.goshortcuts/doc/internal/docxparse/xml_compat.goshortcuts/doc/shortcuts.go
🚧 Files skipped from review as they are similar to previous changes (17)
- go.mod
- .gitignore
- extension/fileio/types.go
- shortcuts/doc/shortcuts.go
- shortcuts/doc/internal/docxparse/model.go
- shortcuts/doc/internal/docxparse/schema.go
- internal/vfs/localfileio/localfileio_test.go
- shortcuts/doc/internal/docxparse/profile.go
- internal/vfs/localfileio/localfileio.go
- shortcuts/doc/docs_script.go
- shortcuts/doc/internal/docxparse/markdown_cjk.go
- shortcuts/doc/internal/docxparse/markdown_extensions.go
- shortcuts/doc/internal/docxparse/wordcount.go
- shortcuts/doc/internal/docxparse/xml.go
- shortcuts/doc/docs_script_test.go
- shortcuts/doc/internal/docxparse/markdown.go
- shortcuts/doc/internal/docxparse/parse_test.go
SunPeiYang996
force-pushed
the
sun/tempv2
branch
from
August 3, 2026 08:45
f13ff1b to
d17c0e9
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1921 +/- ##
==========================================
- Coverage 75.55% 75.52% -0.04%
==========================================
Files 931 944 +13
Lines 99362 104085 +4723
==========================================
+ Hits 75077 78611 +3534
- Misses 18549 19423 +874
- Partials 5736 6051 +315 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (6)
shortcuts/doc/docs_script_test.go (1)
263-281: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the post-creation cleanup path.
This test covers the validation failure, which happens before any file is created. The cleanup calls in
createDocsScriptTempXMLatshortcuts/doc/docs_script.golines 641 and 653 run after the temporary workspace exists, and no test exercises them. A regression there would leave orphaned<name>_<random>_folderdirectories in the user's working directory. Add a test that forces the decision save to fail and asserts the workspace is removed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shortcuts/doc/docs_script_test.go` around lines 263 - 281, Add a test alongside TestDocsScriptInitDraftRequiresPresentationDecision that allows temporary workspace creation, forces the presentation-decision save to fail, and verifies the command returns the expected error. After execution, read the configured working directory and assert no generated <name>_<random>_folder workspace remains, covering the cleanup paths in createDocsScriptTempXML.Source: Coding guidelines
shortcuts/doc/internal/docxparse/markdown.go (1)
414-425: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
elementXMLdiscardsinnerfor void tags.If
tagis a void tag,renderNodesproduces<tag/>and the suffix check at line 421 fails. The function then returnsrenderedand dropsinner. No current caller passes a non-emptyinnerfor a void tag, so this is not an active defect. Make the contract explicit so a future caller cannot lose content silently.♻️ Proposed refactor
func elementXML(tag string, attrs map[string]string, inner string) string { node := newElement(tag, attrs) rendered := renderNodes([]*Node{node}) if inner == "" { return rendered } + // Void tags have no content model; callers must not pass inner content. close := "</" + tag + ">" if strings.HasSuffix(rendered, close) { return strings.TrimSuffix(rendered, close) + inner + close } return rendered }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shortcuts/doc/internal/docxparse/markdown.go` around lines 414 - 425, Update elementXML to explicitly handle void tags when inner is non-empty, preserving the content according to the function’s contract instead of silently returning rendered unchanged. Keep the existing closing-tag insertion behavior for non-void tags and make the void-tag behavior explicit using the available tag-rendering logic.shortcuts/doc/internal/docxparse/parse_test.go (1)
36-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the reason each invalid input is rejected.
The subtests only check that
Parsereturns a non-nil error. A case can pass for the wrong reason. For example,<code><![CDATA[a < b</code>could fail on the unterminated CDATA or on an unrelated token error, and the test would not detect the difference. Add an expected message fragment per case, asTestParseRejectsUnsafeXMLDeclarationsalready does.♻️ Proposed refactor
tests := []struct { name string source string + want string }{ - {name: "missing closing tag", source: `<p>one`}, + {name: "missing closing tag", source: `<p>one`, want: "missing closing tag"}, ... } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if _, err := Parse(tt.source, FormatXML); err == nil { - t.Fatalf("Parse(%q) succeeded, want validation error", tt.source) + _, err := Parse(tt.source, FormatXML) + if err == nil { + t.Fatalf("Parse(%q) succeeded, want validation error", tt.source) + } + if !strings.Contains(err.Error(), tt.want) { + t.Fatalf("Parse(%q) error = %v, want %q", tt.source, err, tt.want) } }) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shortcuts/doc/internal/docxparse/parse_test.go` around lines 36 - 60, Update TestParseXMLRejectsInvalidInput to add an expected error-message fragment for each test case and assert the returned error contains that fragment, following the pattern used by TestParseRejectsUnsafeXMLDeclarations. Keep the existing invalid inputs and ensure each assertion verifies the specific validation reason rather than only non-nil error status.internal/vfs/localfileio/localfileio_test.go (1)
346-355: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an absolute-path case to cover the remaining guard branch.
The three cases cover the
parent == ".",parent == "..", andfilepath.Dir(parent) != "."branches. Thefilepath.IsAbs(clean)branch has no test. If that condition is removed, this test still passes.💚 Proposed change
- for _, path := range []string{"image.png", "../tmp/image.png", "one/two/image.png"} { + for _, path := range []string{"image.png", "../tmp/image.png", "one/two/image.png", filepath.Join(dir, "sub", "image.png")} { if err := fio.RemoveTempDirFile(path); !errors.Is(err, fileio.ErrPathValidation) { t.Fatalf("RemoveTempDirFile(%q) error = %v, want path validation", path, err) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/vfs/localfileio/localfileio_test.go` around lines 346 - 355, Add an absolute path such as filepath.Join(dir, "image.png") to the table in TestLocalFileIO_RemoveTempDirFileRejectsBroadPaths, ensuring RemoveTempDirFile exercises and expects fileio.ErrPathValidation for the filepath.IsAbs(clean) guard branch.shortcuts/doc/docs_create_v2.go (1)
92-121: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider separating the content assignment from the cleanup guard.
The condition
temps != nilcontrols two unrelated actions: registering the cleanup defer and writingbody["content"]. The behavior is correct today, becausematerializeRemoteDocImagesreturns a niltempsonly when it also returns the content unchanged. The coupling is implicit, so a future change to the return contract could drop the materialized content silently.Assigning
body["content"] = contentunconditionally makes the intent explicit and is equivalent today.♻️ Proposed change
content, resources, temps, err := materializeRemoteDocImages(runtime, common.GetString(body, "content"), resources) if err != nil { return err } + body["content"] = content if temps != nil { defer cleanupRemoteDocImageTemps(runtime, temps) - body["content"] = content }Apply the same change in
executeUpdateV2inshortcuts/doc/docs_update_v2.go, which repeats this pattern.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shortcuts/doc/docs_create_v2.go` around lines 92 - 121, In executeCreateV2, assign body["content"] = content unconditionally after materializeRemoteDocImages returns, while keeping the temps != nil guard solely for registering cleanupRemoteDocImageTemps. Apply the same separation in executeUpdateV2, preserving its existing cleanup behavior.internal/vfs/localfileio/localfileio.go (1)
116-121: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign whitespace rejection between the two validators.
validateTempFileNamerejects\t,\r, and\nexplicitly.validateTempDirectoryPatterndoes not.charcheck.RejectControlCharsallows\tand\nby design, andstrings.TrimSpaceonly removes leading and trailing whitespace. A pattern such as"lark-doc\t*"is therefore accepted and produces a directory name that contains a tab.All current call sites pass compile-time constants, so this is not exploitable today. Make the two validators consistent to keep the guard correct if a pattern ever becomes user-influenced.
♻️ Proposed change
func validateTempDirectoryPattern(pattern string) error { - if strings.TrimSpace(pattern) == "" || strings.ContainsAny(pattern, `/\\`) || strings.Count(pattern, "*") != 1 { + if strings.TrimSpace(pattern) == "" || strings.ContainsAny(pattern, "/\\\t\r\n") || strings.Count(pattern, "*") != 1 { return fmt.Errorf("temporary directory pattern must be one non-empty path component containing exactly one '*'") } return charcheck.RejectControlChars(pattern, "temporary directory pattern") }Note: the current raw string
`/\\`works as a character set becauseContainsAnymatches any single rune in it, but the second backslash is redundant.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/vfs/localfileio/localfileio.go` around lines 116 - 121, Update validateTempDirectoryPattern to explicitly reject tab, carriage-return, and newline characters, matching validateTempFileName before the existing path-component, wildcard, and control-character checks. Keep the current validation behavior otherwise unchanged; optionally simplify the redundant backslash in the strings.ContainsAny character set.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@shortcuts/common/drive_media_upload.go`:
- Around line 150-162: Update waitDriveMediaMultipartRequest to check ctx.Err()
before the zero-delay return, and return the existing typed cancellation error
with ctx.Err() preserved as its cause when cancellation wins during the timer
wait. Add tests covering both zero-delay and delayed cancellation, asserting the
typed metadata and underlying cause.
In `@shortcuts/doc/internal/docxparse/profile.go`:
- Around line 260-279: Use the same segment kind for ordered and unordered list
markers in extractNodeSegments: when appending the formatted ordered marker in
the "ol" branch, set its kind to segmentMarker, matching the existing bullet
marker, while preserving the sequence calculation and marker text.
In `@shortcuts/doc/local_doc_resources_test.go`:
- Around line 319-322: The test around downloadRemoteDocImageToTemp must
validate the typed error metadata rather than only matching “20MiB” in the
message. Follow the assertion pattern used by
TestDownloadRemoteDocImageToTempRejectsNonImageBeforeCreatingTemp, using
errs.ProblemOf to assert the error category, errs.SubtypeInvalidArgument
subtype, and “href” parameter, while also verifying the underlying cause is
preserved.
In `@skills/lark-doc/references/lark-doc-create-workflow.md`:
- Around line 96-109: Update the Step 4 handling of data.path and draft_path so
the stored draft_path is absolute or explicitly relative to the caller’s current
working directory, while preserving work_dir for temporary materials. Ensure
Step 6’s parse command can resolve @<draft_path> and read the generated release
candidate without changing the command’s existing validation behavior.
- Line 91: The Step 4 init-draft command must avoid embedding generated JSON
directly inside single quotes. Update the command using --presentation-decision
- with piped or heredoc JSON, or a current-directory temporary file referenced
via @<relative-path>, while preserving the existing init-draft arguments and
JSON format.
In `@skills/lark-doc/references/lark-doc-create.md`:
- Line 46: Update the --doc-format guidance in lark-doc-create.md to prohibit
mixing complete XML and Markdown document formats, while explicitly allowing
documented XML extension tags such as <b>, <u>, and <img> within Markdown when
using markdown format. Keep the existing recommendation to specify the format
explicitly.
In `@skills/lark-doc/references/lark-doc-fetch.md`:
- Around line 102-104: Rename the duplicate `## 参数` heading in the `理解局部读取结果`
section to the unique heading `#### 输出结构`, preserving the surrounding local-read
result content and section hierarchy.
In `@skills/lark-doc/references/lark-doc-script.md`:
- Around line 38-52: Update the `create-temp-xml` command example to pass
`--file-name "川西"` so it matches the documented sample output path, leaving the
existing output and filename explanation unchanged.
In `@skills/lark-doc/references/lark-doc-xml.md`:
- Line 16: Update the source signature in the lark-doc XML reference to show the
path attribute used for local attachments, changing the documented form from
source name to source path while preserving the existing upload guidance.
---
Nitpick comments:
In `@internal/vfs/localfileio/localfileio_test.go`:
- Around line 346-355: Add an absolute path such as filepath.Join(dir,
"image.png") to the table in TestLocalFileIO_RemoveTempDirFileRejectsBroadPaths,
ensuring RemoveTempDirFile exercises and expects fileio.ErrPathValidation for
the filepath.IsAbs(clean) guard branch.
In `@internal/vfs/localfileio/localfileio.go`:
- Around line 116-121: Update validateTempDirectoryPattern to explicitly reject
tab, carriage-return, and newline characters, matching validateTempFileName
before the existing path-component, wildcard, and control-character checks. Keep
the current validation behavior otherwise unchanged; optionally simplify the
redundant backslash in the strings.ContainsAny character set.
In `@shortcuts/doc/docs_create_v2.go`:
- Around line 92-121: In executeCreateV2, assign body["content"] = content
unconditionally after materializeRemoteDocImages returns, while keeping the
temps != nil guard solely for registering cleanupRemoteDocImageTemps. Apply the
same separation in executeUpdateV2, preserving its existing cleanup behavior.
In `@shortcuts/doc/docs_script_test.go`:
- Around line 263-281: Add a test alongside
TestDocsScriptInitDraftRequiresPresentationDecision that allows temporary
workspace creation, forces the presentation-decision save to fail, and verifies
the command returns the expected error. After execution, read the configured
working directory and assert no generated <name>_<random>_folder workspace
remains, covering the cleanup paths in createDocsScriptTempXML.
In `@shortcuts/doc/internal/docxparse/markdown.go`:
- Around line 414-425: Update elementXML to explicitly handle void tags when
inner is non-empty, preserving the content according to the function’s contract
instead of silently returning rendered unchanged. Keep the existing closing-tag
insertion behavior for non-void tags and make the void-tag behavior explicit
using the available tag-rendering logic.
In `@shortcuts/doc/internal/docxparse/parse_test.go`:
- Around line 36-60: Update TestParseXMLRejectsInvalidInput to add an expected
error-message fragment for each test case and assert the returned error contains
that fragment, following the pattern used by
TestParseRejectsUnsafeXMLDeclarations. Keep the existing invalid inputs and
ensure each assertion verifies the specific validation reason rather than only
non-nil error status.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 68c29db6-1476-4084-a13b-ef01e351dd01
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (80)
.github/workflows/ci.yml.gitignorecmd/build_bench_test.gocmd/root_test.goextension/fileio/types.gogo.modinternal/vfs/localfileio/localfileio.gointernal/vfs/localfileio/localfileio_test.goshortcuts/common/drive_media_upload.goshortcuts/common/runner.goshortcuts/common/runner_input_test.goshortcuts/doc/doc_media_insert.goshortcuts/doc/doc_media_upload.goshortcuts/doc/docs_create.goshortcuts/doc/docs_create_test.goshortcuts/doc/docs_create_v2.goshortcuts/doc/docs_script.goshortcuts/doc/docs_script_test.goshortcuts/doc/docs_update.goshortcuts/doc/docs_update_test.goshortcuts/doc/docs_update_v2.goshortcuts/doc/helpers.goshortcuts/doc/html5_block_resources.goshortcuts/doc/internal/docxparse/block_catalog.goshortcuts/doc/internal/docxparse/block_catalog_test.goshortcuts/doc/internal/docxparse/markdown.goshortcuts/doc/internal/docxparse/markdown_cjk.goshortcuts/doc/internal/docxparse/markdown_extensions.goshortcuts/doc/internal/docxparse/model.goshortcuts/doc/internal/docxparse/parse_test.goshortcuts/doc/internal/docxparse/profile.goshortcuts/doc/internal/docxparse/wordcount.goshortcuts/doc/internal/docxparse/xml.goshortcuts/doc/internal/docxparse/xml_compat.goshortcuts/doc/local_doc_resources.goshortcuts/doc/local_doc_resources_test.goshortcuts/doc/shortcuts.goskills/lark-doc/SKILL.mdskills/lark-doc/references/genres/business-analysis.mdskills/lark-doc/references/genres/data-report.mdskills/lark-doc/references/genres/email.mdskills/lark-doc/references/genres/execution-plan.mdskills/lark-doc/references/genres/formal-doc.mdskills/lark-doc/references/genres/meeting-minutes.mdskills/lark-doc/references/genres/memo-brief.mdskills/lark-doc/references/genres/official-redhead.mdskills/lark-doc/references/genres/prd.mdskills/lark-doc/references/genres/proposal.mdskills/lark-doc/references/genres/research-report.mdskills/lark-doc/references/genres/retrospective.mdskills/lark-doc/references/genres/route-consumer.mdskills/lark-doc/references/genres/route-creative.mdskills/lark-doc/references/genres/route-knowledge.mdskills/lark-doc/references/genres/route-marketing.mdskills/lark-doc/references/genres/route-media.mdskills/lark-doc/references/genres/route-opinion.mdskills/lark-doc/references/genres/route-personal-brand.mdskills/lark-doc/references/genres/route-platform.mdskills/lark-doc/references/genres/route-report.mdskills/lark-doc/references/genres/route-workplace.mdskills/lark-doc/references/genres/sop-tutorial.mdskills/lark-doc/references/genres/technical-doc.mdskills/lark-doc/references/genres/wechat.mdskills/lark-doc/references/genres/weekly-report.mdskills/lark-doc/references/genres/white-paper.mdskills/lark-doc/references/genres/xiaohongshu.mdskills/lark-doc/references/lark-doc-create-workflow.mdskills/lark-doc/references/lark-doc-create.mdskills/lark-doc/references/lark-doc-fetch.mdskills/lark-doc/references/lark-doc-md.mdskills/lark-doc/references/lark-doc-script.mdskills/lark-doc/references/lark-doc-update.mdskills/lark-doc/references/lark-doc-whiteboard.mdskills/lark-doc/references/lark-doc-word-stat.mdskills/lark-doc/references/lark-doc-xml-extended-blocks.mdskills/lark-doc/references/lark-doc-xml.mdskills/lark-doc/references/style/lark-doc-create-workflow.mdskills/lark-doc/references/style/lark-doc-style.mdskills/lark-doc/references/style/lark-doc-update-workflow.mdskills/lark-doc/scripts/doc_word_stat.py
💤 Files with no reviewable changes (5)
- skills/lark-doc/references/style/lark-doc-create-workflow.md
- skills/lark-doc/references/style/lark-doc-style.md
- skills/lark-doc/references/style/lark-doc-update-workflow.md
- skills/lark-doc/references/lark-doc-word-stat.md
- skills/lark-doc/scripts/doc_word_stat.py
🚧 Files skipped from review as they are similar to previous changes (19)
- go.mod
- skills/lark-doc/references/genres/route-report.md
- skills/lark-doc/references/genres/route-workplace.md
- skills/lark-doc/references/genres/data-report.md
- .gitignore
- skills/lark-doc/references/genres/route-creative.md
- skills/lark-doc/references/genres/official-redhead.md
- skills/lark-doc/references/genres/wechat.md
- skills/lark-doc/references/genres/white-paper.md
- skills/lark-doc/references/genres/sop-tutorial.md
- skills/lark-doc/references/lark-doc-whiteboard.md
- skills/lark-doc/references/genres/formal-doc.md
- skills/lark-doc/references/genres/xiaohongshu.md
- skills/lark-doc/references/lark-doc-xml-extended-blocks.md
- shortcuts/doc/shortcuts.go
- shortcuts/doc/internal/docxparse/wordcount.go
- skills/lark-doc/SKILL.md
- skills/lark-doc/references/genres/business-analysis.md
- shortcuts/doc/internal/docxparse/markdown_cjk.go
Comment on lines
+96
to
+109
| - `data.path` 记为 `draft_path`，其所在目录记为 `work_dir`；所有临时素材只放在该目录。 | ||
| - CLI 会自动保存 `.presentation-decision.json` 作为固定基线。要求、资料或 contract 实质变化时，提交新决策并重新初始化，不得直接改基线。 | ||
|
|
||
| ### Step 5：生成 release candidate。 | ||
|
|
||
| 读取 [`lark-doc-xml.md`](lark-doc-xml.md)，并结合 Presentation Decision、适用 contract 和 Philosophy 生成完整 XML。使用扩展标签时按需读取 [`拓展标签`](lark-doc-xml-extended-blocks.md)。 | ||
|
|
||
| 1. 公开网络图片使用 `<img href="https://..."/>`；已有本地图片使用 `<img path="@relative/path"/>`；画板使用 `<whiteboard path="@relative/path"/>` 并遵循[`画板工作流`](lark-doc-whiteboard.md)；HTML 使用 `<html5-block path="@relative/path"/>` 并遵循[`拓展标签`](lark-doc-xml-extended-blocks.md)。 | ||
| 2. 只向 Step 4 返回的 `draft_path` 写入 release candidate，不得复用其他任务的路径。 | ||
| 3. 发现 XML 语法问题时只修复最小范围，不无故重写正确内容。 | ||
|
|
||
| ### Step 6：执行 Draft Profile Gate。 | ||
|
|
||
| 1. 执行 `lark-cli docs +script --command parse --content "@<draft_path>" --format json`。命令必须成功且 `data.warning` 为空； parse 未通过时，只修改导致失败或 warning 的最小 XML 片段，禁止重新生成或覆盖整个草稿；仅当草稿为空、截断或无法形成有效文档结构时才全文重建。 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repository files of interest:"
git ls-files | rg '(^|/)(lark-doc-create-workflow|lark-doc-md|lark-doc-xml|lark-doc-xml-extended-blocks|lark-doc-whiteboard)\.md$|references/'
echo
echo "Line 96-109 of reference workflow:"
sed -n '85,115p' skills/lark-doc/references/lark-doc-create-workflow.md || true
echo
echo "Search `@file` and path references in relevant files:"
rg -n --fixed-strings '`@file`|path|draft_path|work_dir|parse --content' skills/lark-doc/references skills -g '*.md' | head -200Repository: larksuite/cli
Length of output: 24270
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Relevant markdown files:"
for f in \
skills/lark-doc/references/lark-doc-create-workflow.md \
skills/lark-doc/references/lark-doc-md.md \
skills/lark-doc/references/lark-doc-create.md \
skills/lark-doc/references/lark-doc-xml.md
do
if [ -f "$f" ]; then
echo "--- $f ($(wc -l < "$f") lines) ---"
rg -n '`@file`|`@file`|@<|content|parse|data\.path|work_dir|draft' "$f" || true
fi
done
echo
echo "Focused excerpts:"
python3 - <<'PY'
from pathlib import Path
files = [
"skills/lark-doc/references/lark-doc-create-workflow.md",
"skills/lark-doc/references/lark-doc-md.md",
"skills/lark-doc/references/lark-doc-create.md",
"skills/lark-doc/references/lark-doc-xml.md",
]
for path in files:
p = Path(path)
if not p.exists():
continue
lines = p.read_text(errors="replace").splitlines()
print(f"--- {path} ---")
for i, line in enumerate(lines, 1):
if any(s in line for s in ["`@file`", "@<", "data.path", "work_dir", "parse --content", "command create", "command init-draft", "content": "@"]):
start=max(1,i-4); end=min(len(lines),i+4)
for n in range(start,end+1):
print(f"{n}: {lines[n-1]}")
print()
PYRepository: larksuite/cli
Length of output: 2606
Make the parsed draft_path valid relative to the current working directory.
lark-doc-md.md requires @file paths to be relative paths under the current working directory. This workflow stores data.path as draft_path, but Step 6 loads it at the caller’s cwd. If data.path is not an absolute path or cwd-relative path, parse --content "@<draft_path>" will fail instead of reading the generated draft.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@skills/lark-doc/references/lark-doc-create-workflow.md` around lines 96 -
109, Update the Step 4 handling of data.path and draft_path so the stored
draft_path is absolute or explicitly relative to the caller’s current working
directory, while preserving work_dir for temporary materials. Ensure Step 6’s
parse command can resolve @<draft_path> and read the generated release candidate
without changing the command’s existing validation behavior.
8 tasks
SunPeiYang996
force-pushed
the
sun/tempv2
branch
from
August 5, 2026 07:39
d17c0e9 to
0f6418e
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (7)
tests/cli_e2e/docs/docs_script_test.go (2)
458-461: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the error type and subtype for this validation failure.
The test checks
error.param,error.message, anderror.hint. It does not checkerror.typeorerror.subtype, so a change from a validation error to another category still passes.TestDocsScriptStrictFlagIsRemovedalready assertserror.subtype; use the same assertions here.💚 Proposed fix
require.Equal(t, "--presentation-decision", gjson.Get(result.Stderr, "error.param").String()) + require.Equal(t, "invalid_argument", gjson.Get(result.Stderr, "error.subtype").String()) require.Contains(t, gjson.Get(result.Stderr, "error.message").String(), "word_count must be omitted")Based on learnings, Validate-stage failures must write the typed JSON validation envelope to stderr, and tests should assert
error.type,error.subtype,error.param, anderror.message.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/cli_e2e/docs/docs_script_test.go` around lines 458 - 461, Update the validation-failure assertions in the affected docs script test to verify error.type and error.subtype in addition to the existing error.param, error.message, and error.hint checks. Match the expected type and subtype assertions used by TestDocsScriptStrictFlagIsRemoved, ensuring the Validate-stage failure remains classified as a typed JSON validation error.Source: Learnings
280-289: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBuild the environment before the goroutines start.
docsScriptE2EEnv(t)runs inside each spawned goroutine. It callst.Helper()andt.TempDir(), andt.TempDir()reports setup failures throught.Fatalf. AFatalfcall from a non-test goroutine does not stop the test as intended. Hoisting the call also avoids creating eight config directories for one shared configuration.♻️ Proposed fix
+ env := docsScriptE2EEnv(t) for i := 0; i < count; i++ { fileName := "川西" if i%2 == 1 { fileName = "川西.xml" } go func(fileName string) { result, err := clie2e.RunCmd(ctx, clie2e.Request{ Args: []string{"docs", "+script", "--command", "create-temp-xml", "--file-name", fileName}, DefaultAs: "bot", WorkDir: workDir, - Env: docsScriptE2EEnv(t), + Env: env, })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/cli_e2e/docs/docs_script_test.go` around lines 280 - 289, Compute the shared result of docsScriptE2EEnv(t) before launching the goroutines in the test, then pass that environment into each clie2e.Request instead of calling docsScriptE2EEnv inside the goroutine. Keep the existing concurrent command execution and shared configuration behavior unchanged.shortcuts/doc/internal/docxparse/wordcount.go (1)
111-250: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract one rune classifier for text and code.
writeCodeRuneandwriteRunerepeat the same Han, letter, digit, Chinese-punctuation, symbol, and fallback branches. The only difference is thekeepsLexemeexpression at Line 157 and Line 227. A future change to one branch will silently diverge from the other and change counts for one mode only. Pass the mode as a parameter and keep a single implementation.♻️ Suggested shape
func (c *textCounter) writeRuneWithMode(r rune, code bool) { // ... shared branches ... if isEnglishPunctuation(r) { keepsLexeme := c.lexeme == lexemeEnglish && (r == '\'' || r == '-') if !code { keepsLexeme = c.lexeme == lexemeEnglish && (r == '\'' || r == '-' || c.lexemeHasDigit && r == '.') || c.lexeme == lexemeNumber && (r == '.' || r == ',' || r == '-') } // ... shared tail ... } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shortcuts/doc/internal/docxparse/wordcount.go` around lines 111 - 250, Extract the duplicated classification logic from writeCodeRune and writeRune into one writeRuneWithMode method that accepts a code-mode parameter, then make both existing methods delegate to it. Preserve all shared Han, letter, digit, punctuation, symbol, fallback, and counting behavior; select the existing code-specific versus text-specific keepsLexeme rules solely from the mode parameter.shortcuts/doc/internal/docxparse/markdown.go (1)
484-495: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDo not drop
innerwhen the tag serializes as self-closing.
elementXMLcurrently returnsrenderedwhenever the empty node does not end with</tag>, including future void/unknown tags. Sinceinnershould be inserted between the start and end tags, return an error or assert the container form before changing the logic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shortcuts/doc/internal/docxparse/markdown.go` around lines 484 - 495, Update elementXML to validate that the rendered node has a container form before inserting inner; when rendered is self-closing or otherwise lacks the expected closing tag, return or assert an error instead of silently dropping inner. Preserve the existing insertion behavior for valid start/end-tag serialization.Source: Coding guidelines
shortcuts/doc/internal/docxparse/errors.go (1)
16-18: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider supporting cause preservation in
newParseError.
newParseErrorusesfmt.Sprintf, so a%wverb produces the literal text%!w(...)and drops the error chain. Parser code that wraps a lower-layer error (for example anencoding/xmlorstrconvfailure) loses that cause before the command boundary converts it to the publicerrs.*contract. Add anUnwrapmethod backed byfmt.Errorfso callers can preserve causes.♻️ Proposed refactor
type parseError struct { message string + cause error } func (e *parseError) Error() string { return e.message } + +func (e *parseError) Unwrap() error { return e.cause } func newParseError(format string, args ...any) error { - return &parseError{message: fmt.Sprintf(format, args...)} + wrapped := fmt.Errorf(format, args...) + return &parseError{message: wrapped.Error(), cause: errors.Unwrap(wrapped)} }Based on coding guidelines: "Preserve typed lower-layer errors unchanged and preserve causes with
.WithCause(err)."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shortcuts/doc/internal/docxparse/errors.go` around lines 16 - 18, Update newParseError to construct its message with fmt.Errorf so %w arguments preserve the underlying error chain instead of being formatted literally. Add an Unwrap method to parseError that returns the stored cause, while keeping existing non-wrapped formatting behavior unchanged.Source: Coding guidelines
internal/vfs/localfileio/localfileio.go (2)
123-128: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReject
.and..as temporary file names.
validateTempFileNameaccepts"."and"..", becausefilepath.Base(".")returns"."and neither value contains a separator.CreateTempDirFilethen fails atvfs.OpenFileand reports aWriteErrorinstead of aPathValidationError. The path stays inside the temporary directory, so there is no escape, only a misclassified error.♻️ Proposed validation
func validateTempFileName(fileName string) error { - if strings.TrimSpace(fileName) == "" || fileName != filepath.Base(fileName) || strings.ContainsAny(fileName, "/\\\t\r\n") { + if strings.TrimSpace(fileName) == "" || fileName == "." || fileName == ".." || + fileName != filepath.Base(fileName) || strings.ContainsAny(fileName, "/\\\t\r\n") { return fmt.Errorf("temporary file name must be one non-empty path component") } return charcheck.RejectControlChars(fileName, "temporary file name") }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/vfs/localfileio/localfileio.go` around lines 123 - 128, Update validateTempFileName to explicitly reject the special path components "." and ".." alongside the existing empty, separator, and control-character checks, so CreateTempDirFile returns a PathValidationError before attempting vfs.OpenFile.
100-114: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider restricting the recursive delete to CLI-created temporary directories.
RemoveTempDirFileaccepts any path with exactly one relative parent component and then callsvfs.RemoveAllon that parent. A future caller that passes, for example,src/main.gowould delete the wholesrcdirectory. All current callers pass aCreateTempDirFileresult, so there is no defect today.Add a cheap ownership check to keep the blast radius small, for example remove the file first and then delete the parent only when it is empty, or require the parent name to carry a known CLI temporary prefix.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/vfs/localfileio/localfileio.go` around lines 100 - 114, Restrict RemoveTempDirFile so it only removes directories owned by the CLI temporary-file flow, rather than recursively deleting any one-level relative parent. Reuse the existing CreateTempDirFile naming or ownership convention if available; otherwise remove the targeted file first and delete its parent only after confirming it is empty, while preserving the current path validation and error wrapping.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/vfs/rename_noreplace_other.go`:
- Around line 17-18: Update the error handling around the hard-link attempt in
renameNoReplace so only errors that confirm the filesystem lacks the atomic
no-replace primitive are wrapped as errors.ErrUnsupported; return
missing-source, invalid-path, permission, and other I/O failures unchanged.
In `@internal/vfs/rename_noreplace_windows_test.go`:
- Around line 20-23: Extend the test table for windowsExtendedPath with a long
relative-path case, using a relative path containing longComponent and expecting
it to remain unchanged without an extended-path prefix. Preserve the existing
absolute, UNC, and already-extended cases.
In `@shortcuts/common/drive_media_upload_test.go`:
- Around line 26-42: Update
TestWaitDriveMediaMultipartRequestReturnsTypedCancellation to start
waitDriveMediaMultipartRequest with a nonzero delay, cancel the context after
the wait begins, and assert it returns promptly with context.Canceled and the
expected network transport Problem fields. Use synchronization or a bounded
timeout to verify cancellation interrupts the pacing wait rather than relying on
pre-cancellation.
In `@shortcuts/doc/docs_script.go`:
- Around line 305-316: Update the dry-run metadata built by NewDryRunAPI so it
reports the remote resource preflight performed by executeDocsScript and
docsScriptResourceWarnings, including cases where
resolveDocsScriptPresentationDecision loads a saved decision. Derive network or
a dedicated preflight key from the same condition used by the execution path,
rather than always setting network to false, while preserving the local-only
value when no remote preflight occurs.
In `@shortcuts/doc/internal/docxparse/markdown.go`:
- Around line 598-613: Update the KindDefinitionTerm branch to process the
rendered fragment through parseMarkdownInlineFragment or the shared
renderInlineChildren path instead of calling parseXML directly, preserving
paragraph-based spacing and stripMarkdownEscapesInNodes behavior before adding
the nodes to the bold element.
In `@tests/cli_e2e/docs/coverage.md`:
- Line 27: Update the affected Markdown table cells in the coverage
documentation to replace pipe-delimited alternatives inside code spans with
prose-separated alternatives, including the entries around the docs fetch row
and the additional reported rows. Preserve the documented CLI options and values
while ensuring each row maintains the expected table column count and passes
markdownlint MD056.
- Line 27: The coverage row’s bot workflow reference uses an outdated subtest
name. In the docs +fetch row, replace “TestDocs_CreateAndFetchWorkflow/fetch as
bot” with the actual test symbol “TestDocs_CreateAndFetchWorkflowAsBot”, leaving
the other coverage references unchanged.
In `@tests/cli_e2e/docs/docs_update_dryrun_test.go`:
- Around line 253-258: Extend the validation-failure assertions after
result.AssertExitCode in the docs update dry-run test to verify that
result.Stdout is empty. Keep the existing typed-envelope checks on result.Stderr
unchanged, enforcing that validation errors use stderr while stdout remains
reserved for program data.
---
Nitpick comments:
In `@internal/vfs/localfileio/localfileio.go`:
- Around line 123-128: Update validateTempFileName to explicitly reject the
special path components "." and ".." alongside the existing empty, separator,
and control-character checks, so CreateTempDirFile returns a PathValidationError
before attempting vfs.OpenFile.
- Around line 100-114: Restrict RemoveTempDirFile so it only removes directories
owned by the CLI temporary-file flow, rather than recursively deleting any
one-level relative parent. Reuse the existing CreateTempDirFile naming or
ownership convention if available; otherwise remove the targeted file first and
delete its parent only after confirming it is empty, while preserving the
current path validation and error wrapping.
In `@shortcuts/doc/internal/docxparse/errors.go`:
- Around line 16-18: Update newParseError to construct its message with
fmt.Errorf so %w arguments preserve the underlying error chain instead of being
formatted literally. Add an Unwrap method to parseError that returns the stored
cause, while keeping existing non-wrapped formatting behavior unchanged.
In `@shortcuts/doc/internal/docxparse/markdown.go`:
- Around line 484-495: Update elementXML to validate that the rendered node has
a container form before inserting inner; when rendered is self-closing or
otherwise lacks the expected closing tag, return or assert an error instead of
silently dropping inner. Preserve the existing insertion behavior for valid
start/end-tag serialization.
In `@shortcuts/doc/internal/docxparse/wordcount.go`:
- Around line 111-250: Extract the duplicated classification logic from
writeCodeRune and writeRune into one writeRuneWithMode method that accepts a
code-mode parameter, then make both existing methods delegate to it. Preserve
all shared Han, letter, digit, punctuation, symbol, fallback, and counting
behavior; select the existing code-specific versus text-specific keepsLexeme
rules solely from the mode parameter.
In `@tests/cli_e2e/docs/docs_script_test.go`:
- Around line 458-461: Update the validation-failure assertions in the affected
docs script test to verify error.type and error.subtype in addition to the
existing error.param, error.message, and error.hint checks. Match the expected
type and subtype assertions used by TestDocsScriptStrictFlagIsRemoved, ensuring
the Validate-stage failure remains classified as a typed JSON validation error.
- Around line 280-289: Compute the shared result of docsScriptE2EEnv(t) before
launching the goroutines in the test, then pass that environment into each
clie2e.Request instead of calling docsScriptE2EEnv inside the goroutine. Keep
the existing concurrent command execution and shared configuration behavior
unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f6851bed-a44a-4f99-b2e6-71378230c6db
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (99)
.github/workflows/ci.yml.gitignorecmd/build_bench_test.gocmd/root_test.goextension/fileio/types.gogo.modinternal/vfs/default.gointernal/vfs/fs.gointernal/vfs/localfileio/atomicwrite.gointernal/vfs/localfileio/localfileio.gointernal/vfs/localfileio/localfileio_test.gointernal/vfs/osfs.gointernal/vfs/rename_noreplace_darwin.gointernal/vfs/rename_noreplace_linux.gointernal/vfs/rename_noreplace_other.gointernal/vfs/rename_noreplace_windows.gointernal/vfs/rename_noreplace_windows_test.goscripts/ci-workflow.test.shshortcuts/common/drive_media_upload.goshortcuts/common/drive_media_upload_test.goshortcuts/common/runner.goshortcuts/common/runner_input_test.goshortcuts/doc/doc_media_insert.goshortcuts/doc/doc_media_upload.goshortcuts/doc/docs_create.goshortcuts/doc/docs_create_test.goshortcuts/doc/docs_create_v2.goshortcuts/doc/docs_result_test.goshortcuts/doc/docs_script.goshortcuts/doc/docs_script_test.goshortcuts/doc/docs_update.goshortcuts/doc/docs_update_test.goshortcuts/doc/docs_update_v2.goshortcuts/doc/helpers.goshortcuts/doc/html5_block_resources.goshortcuts/doc/internal/docxparse/block_catalog.goshortcuts/doc/internal/docxparse/block_catalog_test.goshortcuts/doc/internal/docxparse/errors.goshortcuts/doc/internal/docxparse/markdown.goshortcuts/doc/internal/docxparse/markdown_cjk.goshortcuts/doc/internal/docxparse/markdown_extensions.goshortcuts/doc/internal/docxparse/model.goshortcuts/doc/internal/docxparse/parse_test.goshortcuts/doc/internal/docxparse/profile.goshortcuts/doc/internal/docxparse/wordcount.goshortcuts/doc/internal/docxparse/xml.goshortcuts/doc/internal/docxparse/xml_compat.goshortcuts/doc/local_doc_resources.goshortcuts/doc/local_doc_resources_test.goshortcuts/doc/shortcuts.goskills/lark-doc/SKILL.mdskills/lark-doc/references/genres/business-analysis.mdskills/lark-doc/references/genres/data-report.mdskills/lark-doc/references/genres/email.mdskills/lark-doc/references/genres/execution-plan.mdskills/lark-doc/references/genres/formal-doc.mdskills/lark-doc/references/genres/meeting-minutes.mdskills/lark-doc/references/genres/memo-brief.mdskills/lark-doc/references/genres/official-redhead.mdskills/lark-doc/references/genres/prd.mdskills/lark-doc/references/genres/proposal.mdskills/lark-doc/references/genres/research-report.mdskills/lark-doc/references/genres/retrospective.mdskills/lark-doc/references/genres/route-consumer.mdskills/lark-doc/references/genres/route-creative.mdskills/lark-doc/references/genres/route-knowledge.mdskills/lark-doc/references/genres/route-marketing.mdskills/lark-doc/references/genres/route-media.mdskills/lark-doc/references/genres/route-opinion.mdskills/lark-doc/references/genres/route-personal-brand.mdskills/lark-doc/references/genres/route-platform.mdskills/lark-doc/references/genres/route-report.mdskills/lark-doc/references/genres/route-workplace.mdskills/lark-doc/references/genres/sop-tutorial.mdskills/lark-doc/references/genres/technical-doc.mdskills/lark-doc/references/genres/wechat.mdskills/lark-doc/references/genres/weekly-report.mdskills/lark-doc/references/genres/white-paper.mdskills/lark-doc/references/genres/xiaohongshu.mdskills/lark-doc/references/lark-doc-create-workflow.mdskills/lark-doc/references/lark-doc-create.mdskills/lark-doc/references/lark-doc-fetch.mdskills/lark-doc/references/lark-doc-md.mdskills/lark-doc/references/lark-doc-script.mdskills/lark-doc/references/lark-doc-update.mdskills/lark-doc/references/lark-doc-whiteboard.mdskills/lark-doc/references/lark-doc-word-stat.mdskills/lark-doc/references/lark-doc-xml-extended-blocks.mdskills/lark-doc/references/lark-doc-xml.mdskills/lark-doc/references/style/lark-doc-create-workflow.mdskills/lark-doc/references/style/lark-doc-style.mdskills/lark-doc/references/style/lark-doc-update-workflow.mdskills/lark-doc/scripts/doc_word_stat.pytests/cli_e2e/docs/coverage.mdtests/cli_e2e/docs/docs_create_fetch_test.gotests/cli_e2e/docs/docs_local_resources_dryrun_test.gotests/cli_e2e/docs/docs_local_resources_workflow_test.gotests/cli_e2e/docs/docs_script_test.gotests/cli_e2e/docs/docs_update_dryrun_test.go
💤 Files with no reviewable changes (5)
- skills/lark-doc/references/style/lark-doc-update-workflow.md
- skills/lark-doc/scripts/doc_word_stat.py
- skills/lark-doc/references/lark-doc-word-stat.md
- skills/lark-doc/references/style/lark-doc-create-workflow.md
- skills/lark-doc/references/style/lark-doc-style.md
🚧 Files skipped from review as they are similar to previous changes (39)
- skills/lark-doc/references/lark-doc-whiteboard.md
- shortcuts/doc/internal/docxparse/block_catalog.go
- cmd/root_test.go
- shortcuts/doc/internal/docxparse/block_catalog_test.go
- skills/lark-doc/references/genres/formal-doc.md
- shortcuts/common/runner_input_test.go
- shortcuts/doc/docs_create_test.go
- shortcuts/doc/doc_media_upload.go
- .gitignore
- skills/lark-doc/references/genres/route-report.md
- shortcuts/doc/shortcuts.go
- skills/lark-doc/references/genres/wechat.md
- tests/cli_e2e/docs/docs_create_fetch_test.go
- shortcuts/doc/docs_update_test.go
- shortcuts/common/runner.go
- skills/lark-doc/references/genres/route-creative.md
- shortcuts/doc/helpers.go
- shortcuts/doc/html5_block_resources.go
- skills/lark-doc/references/genres/data-report.md
- skills/lark-doc/references/lark-doc-md.md
- shortcuts/doc/docs_create.go
- shortcuts/common/drive_media_upload.go
- skills/lark-doc/references/genres/xiaohongshu.md
- skills/lark-doc/references/genres/route-workplace.md
- skills/lark-doc/references/genres/business-analysis.md
- skills/lark-doc/SKILL.md
- skills/lark-doc/references/lark-doc-xml-extended-blocks.md
- skills/lark-doc/references/lark-doc-update.md
- shortcuts/doc/internal/docxparse/markdown_extensions.go
- shortcuts/doc/docs_create_v2.go
- shortcuts/doc/docs_update_v2.go
- shortcuts/doc/docs_update.go
- shortcuts/doc/internal/docxparse/xml.go
- skills/lark-doc/references/genres/white-paper.md
- skills/lark-doc/references/genres/sop-tutorial.md
- skills/lark-doc/references/lark-doc-create-workflow.md
- shortcuts/doc/internal/docxparse/model.go
- skills/lark-doc/references/genres/official-redhead.md
- shortcuts/doc/docs_script_test.go
Comment on lines
+17
to
+18
| if err != nil && !errors.Is(err, fs.ErrExist) && !errors.Is(err, fs.ErrPermission) { | ||
| return fmt.Errorf("%w: filesystem has no atomic no-replace primitive", errors.ErrUnsupported) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve non-capability failures.
Lines 17-18 classify almost every link failure as errors.ErrUnsupported. A missing source, invalid path, or I/O failure does not prove that the filesystem lacks hard-link support. Preserve these errors. Map only confirmed unsupported-primitive errors to errors.ErrUnsupported.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/vfs/rename_noreplace_other.go` around lines 17 - 18, Update the
error handling around the hard-link attempt in renameNoReplace so only errors
that confirm the filesystem lacks the atomic no-replace primitive are wrapped as
errors.ErrUnsupported; return missing-source, invalid-path, permission, and
other I/O failures unchanged.
Comment on lines
+20
to
+23
| {name: "short", path: `C:\short\file.xml`, want: `C:\short\file.xml`}, | ||
| {name: "drive", path: `C:\` + longComponent, want: `\\?\C:\` + longComponent}, | ||
| {name: "UNC", path: `\\server\share\` + longComponent, want: `\\?\UNC\server\share\` + longComponent}, | ||
| {name: "already extended", path: `\\?\C:\` + longComponent, want: `\\?\C:\` + longComponent}, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Test long relative paths.
windowsExtendedPath preserves relative paths, including long relative paths. The table does not assert this behavior. Add a long relative-path case to prevent a future prefix regression.
Proposed test case
{name: "UNC", path: `\\server\share\` + longComponent, want: `\\?\UNC\server\share\` + longComponent},
{name: "already extended", path: `\\?\C:\` + longComponent, want: `\\?\C:\` + longComponent},
+ {name: "long relative", path: longComponent, want: longComponent},As per coding guidelines, every behavior change must have an accompanying test, and contract tests must assert the changed behavior directly.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| {name: "short", path: `C:\short\file.xml`, want: `C:\short\file.xml`}, | |
| {name: "drive", path: `C:\` + longComponent, want: `\\?\C:\` + longComponent}, | |
| {name: "UNC", path: `\\server\share\` + longComponent, want: `\\?\UNC\server\share\` + longComponent}, | |
| {name: "already extended", path: `\\?\C:\` + longComponent, want: `\\?\C:\` + longComponent}, | |
| {name: "short", path: `C:\short\file.xml`, want: `C:\short\file.xml`}, | |
| {name: "drive", path: `C:\` + longComponent, want: `\\?\C:\` + longComponent}, | |
| {name: "UNC", path: `\\server\share\` + longComponent, want: `\\?\UNC\server\share\` + longComponent}, | |
| {name: "already extended", path: `\\?\C:\` + longComponent, want: `\\?\C:\` + longComponent}, | |
| {name: "long relative", path: longComponent, want: longComponent}, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/vfs/rename_noreplace_windows_test.go` around lines 20 - 23, Extend
the test table for windowsExtendedPath with a long relative-path case, using a
relative path containing longComponent and expecting it to remain unchanged
without an extended-path prefix. Preserve the existing absolute, UNC, and
already-extended cases.
Source: Coding guidelines
Comment on lines
+26
to
+42
| func TestWaitDriveMediaMultipartRequestReturnsTypedCancellation(t *testing.T) { | ||
| for _, delay := range []time.Duration{0, time.Hour} { | ||
| t.Run(delay.String(), func(t *testing.T) { | ||
| ctx, cancel := context.WithCancel(context.Background()) | ||
| cancel() | ||
|
|
||
| err := waitDriveMediaMultipartRequest(ctx, delay) | ||
| if !errors.Is(err, context.Canceled) { | ||
| t.Fatalf("error = %v, want context.Canceled cause", err) | ||
| } | ||
| problem, ok := errs.ProblemOf(err) | ||
| if !ok || problem.Category != errs.CategoryNetwork || problem.Subtype != errs.SubtypeNetworkTransport { | ||
| t.Fatalf("problem = %#v, ok = %v", problem, ok) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Test cancellation after the pacing wait starts.
Line 30 cancels the context before waitDriveMediaMultipartRequest starts. An implementation that checks ctx.Err() and then uses a non-cancellable sleep would pass this test.
Start a nonzero wait, cancel after the wait begins, and assert prompt return of the typed cancellation error. As per coding guidelines, “Every behavior change must have an accompanying test, and contract tests must assert the changed field or behavior directly.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@shortcuts/common/drive_media_upload_test.go` around lines 26 - 42, Update
TestWaitDriveMediaMultipartRequestReturnsTypedCancellation to start
waitDriveMediaMultipartRequest with a nonzero delay, cancel the context after
the wait begins, and assert it returns promptly with context.Canceled and the
expected network transport Problem fields. Use synchronization or a bounded
timeout to verify cancellation interrupts the pacing wait rather than relying on
pre-cancellation.
Source: Coding guidelines
| | ✓ | docs +create | shortcut | docs/helpers_test.go::createDocWithRetry; docs_create_fetch_test.go::TestDocs_CreateAndFetchWorkflowAsUser/create as user; docs_update_dryrun_test.go::TestDocs_DryRunDefaultsToV2OpenAPI/create; docs_update_dryrun_test.go::TestDocs_CreateTitleDryRunPrependsContent | `--parent-token`; `--doc-format markdown`; `--content`; `--title` | helper asserts returned doc id from `data.document.document_id`; dry-run asserts title is prepended into request body content | | ||
| | ✓ | docs +fetch | shortcut | docs_fetch_dryrun_test.go::TestDocsFetchDryRunIgnoresAPIVersionCompatFlag; docs_create_fetch_test.go::TestDocs_CreateAndFetchWorkflow/fetch as bot; docs_update_test.go::TestDocs_UpdateWorkflow/verify as bot; docs_create_fetch_test.go::TestDocs_CreateAndFetchWorkflowAsUser/fetch as user; docs_update_dryrun_test.go::TestDocs_DryRunDefaultsToV2OpenAPI/fetch | `--doc <docToken>`; `--doc-format markdown`; default `extra_param.enable_user_cite_reference_map=true`; `--api-version v1` compatibility flag still dry-runs the v2 fetch endpoint | | | ||
| | ✓ | docs +create | shortcut | docs/helpers_test.go::createDocWithRetry; docs_create_fetch_test.go::TestDocs_CreateAndFetchWorkflowAsUser/create as user; docs_local_resources_workflow_test.go::TestDocs_LocalResourcesWorkflowAsBot/create image and source; docs_local_resources_workflow_test.go::TestDocs_LocalResourcesWorkflowAsUser/create image and source; docs_local_resources_dryrun_test.go::TestDocs_LocalResourcesDryRun/create; docs_update_dryrun_test.go::TestDocs_DryRunDefaultsToV2OpenAPI/create; docs_update_dryrun_test.go::TestDocs_CreateTitleDryRunPrependsContent; docs_update_dryrun_test.go::TestDocs_CreateEmptyContentFileReportsActionableError | `--parent-token`; `--doc-format markdown`; `--content`; `--title`; XML `<img path="@relative" width="display-px">` + `<source path="@relative">` | local-resource workflows assert returned image/file block IDs and bound tokens; image binding preserves intrinsic dimensions and converts display size to `scale`; empty `@file` content reports the file path and `init-draft` recovery action | | ||
| | ✓ | docs +fetch | shortcut | docs_fetch_dryrun_test.go::TestDocsFetchDryRunIgnoresAPIVersionCompatFlag; docs_create_fetch_test.go::TestDocs_CreateAndFetchWorkflow/fetch as bot; docs_update_test.go::TestDocs_UpdateWorkflow/verify as bot; docs_create_fetch_test.go::TestDocs_CreateAndFetchWorkflowAsUser/fetch as user; docs_local_resources_workflow_test.go::testDocsLocalResourcesWorkflow/fetch verifies persisted resources; docs_update_dryrun_test.go::TestDocs_DryRunDefaultsToV2OpenAPI/fetch | `--doc <docToken>`; `--doc-format markdown|xml`; `--detail full`; default `extra_param.enable_user_cite_reference_map=true`; `--api-version v1` compatibility flag still dry-runs the v2 fetch endpoint | local-resource fetch asserts captions/file names persist and internal markers/paths do not leak | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Remove raw pipe delimiters from table cells.
The | characters inside these code spans are parsed as Markdown table delimiters. They create extra columns and can shift or drop the final table content. markdownlint reports this as an MD056 table-column error.
Use prose-separated alternatives instead.
Proposed fix
- `--doc-format markdown|xml`
+ `--doc-format` with `markdown` or `xml`
- `--presentation-decision <JSON|`@file`|->`
+ `--presentation-decision` with `JSON`, `@file`, or `-`
- `--command overwrite|append|block_replace`
+ `--command` with `overwrite`, `append`, or `block_replace`Also applies to: 31-31, 36-36
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 27-27: Spaces inside code span elements
(MD038, no-space-in-code)
[warning] 27-27: Spaces inside code span elements
(MD038, no-space-in-code)
[warning] 27-27: Spaces inside code span elements
(MD038, no-space-in-code)
[warning] 27-27: Table column count
Expected: 6; Actual: 7; Too many cells, extra data will be missing
(MD056, table-column-count)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/cli_e2e/docs/coverage.md` at line 27, Update the affected Markdown
table cells in the coverage documentation to replace pipe-delimited alternatives
inside code spans with prose-separated alternatives, including the entries
around the docs fetch row and the additional reported rows. Preserve the
documented CLI options and values while ensuring each row maintains the expected
table column count and passes markdownlint MD056.
Source: Linters/SAST tools
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the actual bot test name in the coverage row.
The test is named TestDocs_CreateAndFetchWorkflowAsBot in tests/cli_e2e/docs/docs_create_fetch_test.go. This row uses TestDocs_CreateAndFetchWorkflow/fetch as bot, so the reference does not identify the test correctly.
Proposed fix
- docs_create_fetch_test.go::TestDocs_CreateAndFetchWorkflow/fetch as bot
+ docs_create_fetch_test.go::TestDocs_CreateAndFetchWorkflowAsBot/fetch as bot📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| | ✓ | docs +fetch | shortcut | docs_fetch_dryrun_test.go::TestDocsFetchDryRunIgnoresAPIVersionCompatFlag; docs_create_fetch_test.go::TestDocs_CreateAndFetchWorkflow/fetch as bot; docs_update_test.go::TestDocs_UpdateWorkflow/verify as bot; docs_create_fetch_test.go::TestDocs_CreateAndFetchWorkflowAsUser/fetch as user; docs_local_resources_workflow_test.go::testDocsLocalResourcesWorkflow/fetch verifies persisted resources; docs_update_dryrun_test.go::TestDocs_DryRunDefaultsToV2OpenAPI/fetch | `--doc <docToken>`; `--doc-format markdown|xml`; `--detail full`; default `extra_param.enable_user_cite_reference_map=true`; `--api-version v1` compatibility flag still dry-runs the v2 fetch endpoint | local-resource fetch asserts captions/file names persist and internal markers/paths do not leak | | |
| | ✓ | docs +fetch | shortcut | docs_fetch_dryrun_test.go::TestDocsFetchDryRunIgnoresAPIVersionCompatFlag; docs_create_fetch_test.go::TestDocs_CreateAndFetchWorkflowAsBot/fetch as bot; docs_update_test.go::TestDocs_UpdateWorkflow/verify as bot; docs_create_fetch_test.go::TestDocs_CreateAndFetchWorkflowAsUser/fetch as user; docs_local_resources_workflow_test.go::testDocsLocalResourcesWorkflow/fetch verifies persisted resources; docs_update_dryrun_test.go::TestDocs_DryRunDefaultsToV2OpenAPI/fetch | `--doc <docToken>`; `--doc-format markdown|xml`; `--detail full`; default `extra_param.enable_user_cite_reference_map=true`; `--api-version v1` compatibility flag still dry-runs the v2 fetch endpoint | local-resource fetch asserts captions/file names persist and internal markers/paths do not leak | |
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 27-27: Spaces inside code span elements
(MD038, no-space-in-code)
[warning] 27-27: Spaces inside code span elements
(MD038, no-space-in-code)
[warning] 27-27: Spaces inside code span elements
(MD038, no-space-in-code)
[warning] 27-27: Table column count
Expected: 6; Actual: 7; Too many cells, extra data will be missing
(MD056, table-column-count)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/cli_e2e/docs/coverage.md` at line 27, The coverage row’s bot workflow
reference uses an outdated subtest name. In the docs +fetch row, replace
“TestDocs_CreateAndFetchWorkflow/fetch as bot” with the actual test symbol
“TestDocs_CreateAndFetchWorkflowAsBot”, leaving the other coverage references
unchanged.
Comment on lines
+253
to
+258
| result.AssertExitCode(t, 2) | ||
| require.Equal(t, "validation", gjson.Get(result.Stderr, "error.type").String()) | ||
| require.Equal(t, "invalid_argument", gjson.Get(result.Stderr, "error.subtype").String()) | ||
| require.Equal(t, "--content", gjson.Get(result.Stderr, "error.param").String()) | ||
| require.Equal(t, `--content file "draft.xml" is empty`, gjson.Get(result.Stderr, "error.message").String()) | ||
| require.Contains(t, gjson.Get(result.Stderr, "error.hint").String(), "exact data.path returned by that command") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that stdout stays empty for this validation failure.
The test verifies the typed envelope on stderr but never checks stdout. A regression that writes the error envelope to stdout would still pass. Add an empty-stdout assertion so the stream separation contract is enforced.
💚 Proposed fix
result.AssertExitCode(t, 2)
+ require.Empty(t, result.Stdout, "stdout must stay reserved for program data")
require.Equal(t, "validation", gjson.Get(result.Stderr, "error.type").String())Based on learnings: "Validate-stage failures must exit with code 2, write the typed JSON validation envelope to result.Stderr, and leave result.Stdout empty so stdout remains reserved for program data."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| result.AssertExitCode(t, 2) | |
| require.Equal(t, "validation", gjson.Get(result.Stderr, "error.type").String()) | |
| require.Equal(t, "invalid_argument", gjson.Get(result.Stderr, "error.subtype").String()) | |
| require.Equal(t, "--content", gjson.Get(result.Stderr, "error.param").String()) | |
| require.Equal(t, `--content file "draft.xml" is empty`, gjson.Get(result.Stderr, "error.message").String()) | |
| require.Contains(t, gjson.Get(result.Stderr, "error.hint").String(), "exact data.path returned by that command") | |
| result.AssertExitCode(t, 2) | |
| require.Empty(t, result.Stdout, "stdout must stay reserved for program data") | |
| require.Equal(t, "validation", gjson.Get(result.Stderr, "error.type").String()) | |
| require.Equal(t, "invalid_argument", gjson.Get(result.Stderr, "error.subtype").String()) | |
| require.Equal(t, "--content", gjson.Get(result.Stderr, "error.param").String()) | |
| require.Equal(t, `--content file "draft.xml" is empty`, gjson.Get(result.Stderr, "error.message").String()) | |
| require.Contains(t, gjson.Get(result.Stderr, "error.hint").String(), "exact data.path returned by that command") |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/cli_e2e/docs/docs_update_dryrun_test.go` around lines 253 - 258, Extend
the validation-failure assertions after result.AssertExitCode in the docs update
dry-run test to verify that result.Stdout is empty. Keep the existing
typed-envelope checks on result.Stderr unchanged, enforcing that validation
errors use stderr while stdout remains reserved for program data.
Sources: Coding guidelines, Learnings
SunPeiYang996
force-pushed
the
sun/tempv2
branch
from
August 5, 2026 08:00
0f6418e to
d7b6468
Compare
SunPeiYang996
changed the title
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
shortcuts/doc/internal/docxparse/markdown.go (1)
599-606: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRender definition terms through
renderInlineChildren.This branch calls
parseXML(fragment)directly. Every other inline site callsrenderInlineChildren, which wraps the fragment in<p>throughparseMarkdownInlineFragmentand then appliesstripMarkdownEscapesInNodes. A definition term therefore loses two behaviors: significant spaces between adjacent inline elements, and backslash-escape stripping. A term such as**a** *b*ora\-brenders differently from the same content in a paragraph.🐛 Proposed fix
case extast.KindDefinitionTerm: - fragment, err := renderInlineFragment(child, source, false) - if err != nil { - return nil, err - } - nodes, err := parseXML(fragment) + nodes, err := renderInlineChildren(child, source) if err != nil { return nil, err }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shortcuts/doc/internal/docxparse/markdown.go` around lines 599 - 606, Update the definition-term handling around renderInlineFragment to use renderInlineChildren for the term’s inline children instead of parsing the rendered fragment directly with parseXML. Preserve the existing error propagation while ensuring the shared paragraph wrapping, significant-space handling, and stripMarkdownEscapesInNodes behavior are applied.
🧹 Nitpick comments (2)
shortcuts/doc/docs_result_test.go (1)
44-47: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSelect the shortcut from the table instead of the case name.
The shortcut choice depends on the string
"update". If a case name changes, the test silently runsDocsCreateagainst the update stub. Add ashortcutfield to the table.♻️ Proposed refactor
tests := []struct { name string method string path string args []string + mk func() *common.Shortcut }{Then set the field per case and replace the name check:
- shortcut := DocsCreate - if tt.name == "update" { - shortcut = DocsUpdate - } - err := mountAndRunDocs(t, shortcut, tt.args, f, stdout) + err := mountAndRunDocs(t, tt.mk(), tt.args, f, stdout)Use the concrete shortcut type declared in this package for the field.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shortcuts/doc/docs_result_test.go` around lines 44 - 47, Add a shortcut field to the test-case table using the package’s concrete shortcut type, assign DocsCreate or DocsUpdate in each case, and update the test setup to use tt.shortcut directly instead of branching on tt.name.tests/cli_e2e/docs/docs_local_resources_dryrun_test.go (1)
21-22: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGive each subtest its own timeout budget.
The 30-second deadline starts once in the parent test and covers all four subtests. Each subtest starts a separate CLI process. If the first subtests are slow, the later subtests fail on a context deadline instead of a real assertion. Create the context inside the subtest body.
♻️ Proposed change
- ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) - t.Cleanup(cancel) - workDir := t.TempDir()t.Run(tt.name, func(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + t.Cleanup(cancel) result, err := clie2e.RunCmd(ctx, clie2e.Request{🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/cli_e2e/docs/docs_local_resources_dryrun_test.go` around lines 21 - 22, Move context.WithTimeout and its cleanup from the parent test into each subtest body, giving every CLI process in the four subtests an independent 30-second timeout budget while preserving the existing timeout duration and cancellation behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@shortcuts/doc/internal/docxparse/markdown.go`:
- Around line 599-606: Update the definition-term handling around
renderInlineFragment to use renderInlineChildren for the term’s inline children
instead of parsing the rendered fragment directly with parseXML. Preserve the
existing error propagation while ensuring the shared paragraph wrapping,
significant-space handling, and stripMarkdownEscapesInNodes behavior are
applied.
---
Nitpick comments:
In `@shortcuts/doc/docs_result_test.go`:
- Around line 44-47: Add a shortcut field to the test-case table using the
package’s concrete shortcut type, assign DocsCreate or DocsUpdate in each case,
and update the test setup to use tt.shortcut directly instead of branching on
tt.name.
In `@tests/cli_e2e/docs/docs_local_resources_dryrun_test.go`:
- Around line 21-22: Move context.WithTimeout and its cleanup from the parent
test into each subtest body, giving every CLI process in the four subtests an
independent 30-second timeout budget while preserving the existing timeout
duration and cancellation behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ac9a9fd6-633c-4bd2-ae90-58065c492e68
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (99)
.github/workflows/ci.yml.gitignorecmd/build_bench_test.gocmd/root_test.goextension/fileio/types.gogo.modinternal/vfs/default.gointernal/vfs/fs.gointernal/vfs/localfileio/atomicwrite.gointernal/vfs/localfileio/localfileio.gointernal/vfs/localfileio/localfileio_test.gointernal/vfs/osfs.gointernal/vfs/rename_noreplace_darwin.gointernal/vfs/rename_noreplace_linux.gointernal/vfs/rename_noreplace_other.gointernal/vfs/rename_noreplace_windows.gointernal/vfs/rename_noreplace_windows_test.goscripts/ci-workflow.test.shshortcuts/common/drive_media_upload.goshortcuts/common/drive_media_upload_test.goshortcuts/common/runner.goshortcuts/common/runner_input_test.goshortcuts/doc/doc_media_insert.goshortcuts/doc/doc_media_upload.goshortcuts/doc/docs_create.goshortcuts/doc/docs_create_test.goshortcuts/doc/docs_create_v2.goshortcuts/doc/docs_result_test.goshortcuts/doc/docs_script.goshortcuts/doc/docs_script_test.goshortcuts/doc/docs_update.goshortcuts/doc/docs_update_test.goshortcuts/doc/docs_update_v2.goshortcuts/doc/helpers.goshortcuts/doc/html5_block_resources.goshortcuts/doc/internal/docxparse/block_catalog.goshortcuts/doc/internal/docxparse/block_catalog_test.goshortcuts/doc/internal/docxparse/errors.goshortcuts/doc/internal/docxparse/markdown.goshortcuts/doc/internal/docxparse/markdown_cjk.goshortcuts/doc/internal/docxparse/markdown_extensions.goshortcuts/doc/internal/docxparse/model.goshortcuts/doc/internal/docxparse/parse_test.goshortcuts/doc/internal/docxparse/profile.goshortcuts/doc/internal/docxparse/wordcount.goshortcuts/doc/internal/docxparse/xml.goshortcuts/doc/internal/docxparse/xml_compat.goshortcuts/doc/local_doc_resources.goshortcuts/doc/local_doc_resources_test.goshortcuts/doc/shortcuts.goskills/lark-doc/SKILL.mdskills/lark-doc/references/genres/business-analysis.mdskills/lark-doc/references/genres/data-report.mdskills/lark-doc/references/genres/email.mdskills/lark-doc/references/genres/execution-plan.mdskills/lark-doc/references/genres/formal-doc.mdskills/lark-doc/references/genres/meeting-minutes.mdskills/lark-doc/references/genres/memo-brief.mdskills/lark-doc/references/genres/official-redhead.mdskills/lark-doc/references/genres/prd.mdskills/lark-doc/references/genres/proposal.mdskills/lark-doc/references/genres/research-report.mdskills/lark-doc/references/genres/retrospective.mdskills/lark-doc/references/genres/route-consumer.mdskills/lark-doc/references/genres/route-creative.mdskills/lark-doc/references/genres/route-knowledge.mdskills/lark-doc/references/genres/route-marketing.mdskills/lark-doc/references/genres/route-media.mdskills/lark-doc/references/genres/route-opinion.mdskills/lark-doc/references/genres/route-personal-brand.mdskills/lark-doc/references/genres/route-platform.mdskills/lark-doc/references/genres/route-report.mdskills/lark-doc/references/genres/route-workplace.mdskills/lark-doc/references/genres/sop-tutorial.mdskills/lark-doc/references/genres/technical-doc.mdskills/lark-doc/references/genres/wechat.mdskills/lark-doc/references/genres/weekly-report.mdskills/lark-doc/references/genres/white-paper.mdskills/lark-doc/references/genres/xiaohongshu.mdskills/lark-doc/references/lark-doc-create-workflow.mdskills/lark-doc/references/lark-doc-create.mdskills/lark-doc/references/lark-doc-fetch.mdskills/lark-doc/references/lark-doc-md.mdskills/lark-doc/references/lark-doc-script.mdskills/lark-doc/references/lark-doc-update.mdskills/lark-doc/references/lark-doc-whiteboard.mdskills/lark-doc/references/lark-doc-word-stat.mdskills/lark-doc/references/lark-doc-xml-extended-blocks.mdskills/lark-doc/references/lark-doc-xml.mdskills/lark-doc/references/style/lark-doc-create-workflow.mdskills/lark-doc/references/style/lark-doc-style.mdskills/lark-doc/references/style/lark-doc-update-workflow.mdskills/lark-doc/scripts/doc_word_stat.pytests/cli_e2e/docs/coverage.mdtests/cli_e2e/docs/docs_create_fetch_test.gotests/cli_e2e/docs/docs_local_resources_dryrun_test.gotests/cli_e2e/docs/docs_local_resources_workflow_test.gotests/cli_e2e/docs/docs_script_test.gotests/cli_e2e/docs/docs_update_dryrun_test.go
💤 Files with no reviewable changes (5)
- skills/lark-doc/references/style/lark-doc-style.md
- skills/lark-doc/references/style/lark-doc-create-workflow.md
- skills/lark-doc/references/style/lark-doc-update-workflow.md
- skills/lark-doc/scripts/doc_word_stat.py
- skills/lark-doc/references/lark-doc-word-stat.md
🚧 Files skipped from review as they are similar to previous changes (90)
- internal/vfs/rename_noreplace_other.go
- internal/vfs/rename_noreplace_windows.go
- internal/vfs/rename_noreplace_linux.go
- shortcuts/doc/internal/docxparse/block_catalog_test.go
- cmd/root_test.go
- internal/vfs/osfs.go
- shortcuts/doc/docs_create_test.go
- shortcuts/doc/doc_media_insert.go
- .gitignore
- shortcuts/doc/shortcuts.go
- skills/lark-doc/references/genres/technical-doc.md
- skills/lark-doc/references/genres/route-platform.md
- shortcuts/doc/internal/docxparse/errors.go
- skills/lark-doc/references/genres/route-marketing.md
- skills/lark-doc/references/genres/weekly-report.md
- skills/lark-doc/references/genres/route-creative.md
- skills/lark-doc/references/lark-doc-md.md
- skills/lark-doc/references/genres/route-workplace.md
- tests/cli_e2e/docs/docs_create_fetch_test.go
- internal/vfs/rename_noreplace_windows_test.go
- skills/lark-doc/references/genres/route-media.md
- scripts/ci-workflow.test.sh
- skills/lark-doc/references/genres/route-personal-brand.md
- skills/lark-doc/references/genres/wechat.md
- shortcuts/doc/internal/docxparse/block_catalog.go
- skills/lark-doc/references/genres/business-analysis.md
- internal/vfs/rename_noreplace_darwin.go
- internal/vfs/localfileio/atomicwrite.go
- shortcuts/common/runner.go
- skills/lark-doc/references/lark-doc-create.md
- shortcuts/common/runner_input_test.go
- skills/lark-doc/references/genres/memo-brief.md
- shortcuts/common/drive_media_upload.go
- shortcuts/common/drive_media_upload_test.go
- skills/lark-doc/references/genres/proposal.md
- go.mod
- cmd/build_bench_test.go
- skills/lark-doc/references/genres/route-knowledge.md
- internal/vfs/fs.go
- skills/lark-doc/references/lark-doc-xml-extended-blocks.md
- skills/lark-doc/references/genres/research-report.md
- skills/lark-doc/references/genres/execution-plan.md
- shortcuts/doc/internal/docxparse/model.go
- shortcuts/doc/docs_create_v2.go
- shortcuts/doc/internal/docxparse/markdown_extensions.go
- skills/lark-doc/references/genres/sop-tutorial.md
- skills/lark-doc/references/genres/email.md
- skills/lark-doc/references/genres/prd.md
- skills/lark-doc/references/genres/xiaohongshu.md
- skills/lark-doc/references/lark-doc-whiteboard.md
- skills/lark-doc/references/genres/data-report.md
- .github/workflows/ci.yml
- shortcuts/doc/docs_update_test.go
- skills/lark-doc/references/genres/meeting-minutes.md
- skills/lark-doc/references/genres/white-paper.md
- internal/vfs/default.go
- shortcuts/doc/docs_update.go
- skills/lark-doc/references/lark-doc-create-workflow.md
- skills/lark-doc/references/genres/retrospective.md
- shortcuts/doc/internal/docxparse/xml_compat.go
- shortcuts/doc/html5_block_resources.go
- shortcuts/doc/internal/docxparse/markdown_cjk.go
- skills/lark-doc/references/genres/route-opinion.md
- shortcuts/doc/helpers.go
- skills/lark-doc/references/genres/route-report.md
- skills/lark-doc/references/genres/route-consumer.md
- tests/cli_e2e/docs/docs_script_test.go
- shortcuts/doc/docs_create.go
- extension/fileio/types.go
- shortcuts/doc/docs_update_v2.go
- skills/lark-doc/references/genres/formal-doc.md
- skills/lark-doc/SKILL.md
- shortcuts/doc/docs_script.go
- internal/vfs/localfileio/localfileio.go
- skills/lark-doc/references/lark-doc-xml.md
- skills/lark-doc/references/lark-doc-fetch.md
- shortcuts/doc/doc_media_upload.go
- tests/cli_e2e/docs/coverage.md
- skills/lark-doc/references/lark-doc-update.md
- shortcuts/doc/internal/docxparse/parse_test.go
- tests/cli_e2e/docs/docs_local_resources_workflow_test.go
- shortcuts/doc/internal/docxparse/xml.go
- shortcuts/doc/local_doc_resources.go
- tests/cli_e2e/docs/docs_update_dryrun_test.go
- shortcuts/doc/docs_script_test.go
- shortcuts/doc/internal/docxparse/wordcount.go
- shortcuts/doc/local_doc_resources_test.go
- skills/lark-doc/references/genres/official-redhead.md
- shortcuts/doc/internal/docxparse/profile.go
- internal/vfs/localfileio/localfileio_test.go
8 tasks
Add docs +script workflows for isolated draft initialization, tolerant XML/Markdown profiling, and Markdown-to-XML conversion. Support local and remote document resources across create and update flows with safe upload, binding, verification, and cleanup. Extend FileIO and VFS with atomic no-clobber and temporary workspace capabilities, including cross-platform coverage. Refresh lark-doc guidance and expand dry-run, live E2E, unit, concurrency, performance, and Windows CI coverage.
SunPeiYang996
force-pushed
the
sun/tempv2
branch
from
August 6, 2026 04:51
d7b6468 to
52f9a9d
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add local-first document authoring workflows so agents can initialize isolated drafts, inspect XML or Markdown before publishing, and convert Markdown without an OpenAPI round trip. Extend document create/update flows to handle local images, remote images, and file attachments safely across supported platforms.
Changes
docs +scriptoperations forinit-draft,create-temp-xml,parse, andmarkdown-to-xml, including Presentation Decision baselines and document profiles.docs +createanddocs +update, covering upload, token binding, verification, cleanup, image dimensions, and replay-safe output.FileIOcapabilities and the internal VFS with atomic no-clobber saves and unique temporary workspaces on Linux, macOS, Windows, and fallback platforms.lark-docauthoring guidance and genre references; expand unit, dry-run E2E, live E2E, concurrency, performance, and Windows compatibility coverage.Test Plan
lark-cli docscreate/fetch/update/script workflows pass with temporary resources and cleanupCurrent validation is not green yet: source-contract fixtures, the deterministic public-content guard, and the Windows compatibility assertion fail. Dry-run and live E2E are gated on those jobs and were skipped for the current SHA.
Related Issues