feat(fetch): add multi-entity content reads - #2202
Conversation
|
|
github-actions
Bot
added
domain/base
📝 WalkthroughWalkthroughChangesThe PR adds shared Markdown content fetching, anchored XML rendering, pagination, Wiki resolution, temporary-file delivery, safety-result reuse, and the Unified fetch and delivery
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
f416990 to
addd32f
Compare
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (13)
shortcuts/common/fetch_content_delivery.go (1)
56-57: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueAvoid the redundant string-to-byte conversions.
len([]byte(content))copies the whole body only to measure it.len(content)on a string already returns the byte length without allocating. Line 71 then makes a second full copy. For a body above the 24 KiB threshold this allocates the content twice per call before any file is written.Note that
body := []byte(content)at Line 71 is still required, becauseWriteTempMarkdownandsha256.Sum256need a byte slice.♻️ Proposed change to measure the length without copying
delivery := FetchContentDelivery{Content: content} autoSpill := runtime.Bool("full") && - runtime.JqExpr == "" && len([]byte(content)) > FetchContentSpillThreshold + runtime.JqExpr == "" && len(content) > FetchContentSpillThreshold if !autoSpill { return delivery, scan, nil }🤖 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/fetch_content_delivery.go` around lines 56 - 57, Update the autoSpill condition in the content-fetch flow to use len(content) instead of converting content to []byte for measurement, while preserving the existing runtime.Bool, JqExpr, and threshold checks. Keep the body := []byte(content) conversion used later by WriteTempMarkdown and sha256.Sum256.shortcuts/common/contentread/fetch.go (1)
26-28: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument why
ctxis discarded.
FetchDocInfoacceptsctxand drops it at Line 27.CallAPITypeduses the context stored in theRuntimeContext(shortcuts/common/runner.goLine 291). A caller that passes a derived context with a deadline or cancellation gets no effect, and the call site gives no warning.Add a short comment next to
_ = ctx, or remove the parameter.🤖 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/contentread/fetch.go` around lines 26 - 28, Add a concise comment next to the `_ = ctx` statement in `FetchDocInfo` explaining that `CallAPITyped` uses the context held by `RuntimeContext`, so the method’s supplied context is intentionally unused.shortcuts/common/contentread/anchored_markdown.go (1)
316-336: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAlign
readTextEOF handling with the other collectors.
readTextreturns any decoder error, includingio.EOF.renderChildren(Line 90),collectRows(Line 264), andcollectCells(Line 293) all treatio.EOFas a normal end and return the text collected so far.If the content stream ends inside an element,
readTextpropagatesio.EOFup toFetchAnchoredMarkdown, which converts it toinvalid_responseand discards every rendered block. The sibling paths would instead return the partial content. Make the behavior consistent so a truncated tail does not fail the whole read.♻️ Proposed EOF handling in `readText`
for { tok, err := dec.Token() + if errors.Is(err, io.EOF) { + return b.String(), nil + } if err != nil { return b.String(), 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/common/contentread/anchored_markdown.go` around lines 316 - 336, Update anchoredMarkdownRenderer.readText to treat io.EOF from dec.Token as normal completion, returning the text collected so far with no error, while continuing to propagate other decoder errors. Keep the existing depth-based end-element handling unchanged.shortcuts/common/contentread/tables.go (1)
26-26: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueRequire a pipe in the delimiter line to avoid false table detection.
gfmDelimiterRematches a bare thematic break such as---. If a prose line contains a pipe and the next line is---, Line 48 treats the pair as a table header plus delimiter. The following pipe-containing prose lines are then consumed as table rows and can be dropped with a truncation hint.A GFM delimiter row must contain at least one pipe when the table has more than one column, and a single-column table still uses a leading or trailing pipe in practice. Add that check.
♻️ Proposed narrowing of the delimiter check
- if !inFence && i+1 < len(lines) && strings.Contains(line, "|") && gfmDelimiterRe.MatchString(lines[i+1]) { + if !inFence && i+1 < len(lines) && strings.Contains(line, "|") && + strings.Contains(lines[i+1], "|") && gfmDelimiterRe.MatchString(lines[i+1]) {Also applies to: 48-48
🤖 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/contentread/tables.go` at line 26, Update gfmDelimiterRe and its use in the table-detection logic around the line-48 check so delimiter rows must contain at least one pipe character. Preserve matching of valid GFM delimiter rows, including single-column rows with a leading or trailing pipe, while rejecting bare thematic breaks such as "---" to prevent prose from being consumed as table rows.shortcuts/doc/docs_skill_doc_test.go (1)
24-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFail instead of skipping when the skill doc is missing.
lark-doc-fetch.mdis a tracked file in this repository. If it is renamed or deleted, this test skips and the flag-drift guard silently stops running. Uset.Fatalfso the loss of the guard is visible.♻️ Proposed fix
data, err := os.ReadFile(filepath.Join("..", "..", "skills", "lark-doc", "references", "lark-doc-fetch.md")) if err != nil { - t.Skipf("skill doc not found: %v", err) + t.Fatalf("read skill doc: %v", 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/docs_skill_doc_test.go` around lines 24 - 27, Update the os.ReadFile error handling in the test to call t.Fatalf instead of t.Skipf when lark-doc-fetch.md cannot be read, ensuring the flag-drift guard fails visibly when the tracked skill document is missing.shortcuts/doc/docs_fetch_v2_test.go (1)
1022-1024: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRegister
embed-max-rowsin the test runtime.Production registers
embed-max-rowswith default50inv2FetchFlags. This fixture omits it.runtime.Int("embed-max-rows")swallows the lookup error and returns0, which means "no limit". Any future test that reachesdryRunAnchoredMarkdownFetchthrough this runtime will assert the wrong row cap and still pass.♻️ Proposed fixture fix
cmd.Flags().Bool("full", false, "") cmd.Flags().String("page-token", "", "") cmd.Flags().Int("page-size", 0, "") + cmd.Flags().Int("embed-max-rows", fetchDefaultInt("embed-max-rows"), "")🤖 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_fetch_v2_test.go` around lines 1022 - 1024, Update the test command fixture that registers flags alongside “full”, “page-token”, and “page-size” to also register “embed-max-rows” with the production default of 50, so runtime.Int("embed-max-rows") observes the configured row cap.tests/cli_e2e/docs/docs_fetch_dryrun_test.go (1)
56-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the typed error envelope fields, not a stderr substring.
require.Contains(t, result.Stderr, "--page-size")passes even whenerror.paramnames a different flag, because the flag name also appears in the message text. Parse the JSON envelope from stderr and asserterror.type,error.subtype, anderror.param.♻️ Proposed assertions
require.NoError(t, err) result.AssertExitCode(t, 2) - require.Contains(t, result.Stderr, "--page-size") + require.Equal(t, "validation", gjson.Get(result.Stderr, "error.type").String(), "stderr:\n%s", result.Stderr) + require.NotEmpty(t, gjson.Get(result.Stderr, "error.subtype").String(), "stderr:\n%s", result.Stderr) + require.Equal(t, "--page-size", gjson.Get(result.Stderr, "error.param").String(), "stderr:\n%s", result.Stderr)Based on learnings: "Validate-stage failures must exit with code 2, write the typed JSON validation envelope to result.Stderr... Parse and assert error.type, error.subtype, error.param, and error.message from stderr."
🤖 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_fetch_dryrun_test.go` around lines 56 - 58, Replace the stderr substring assertion in the dry-run fetch test with JSON envelope parsing. Assert the typed validation fields error.type, error.subtype, error.param, and error.message, while preserving the expected exit code 2 and stderr source.Source: Learnings
shortcuts/drive/drive_fetch_envelope.go (1)
83-90: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCorrect the
withPaginationdoc comment.The comment states "No-op when hasMore is false". The body assigns
HasMoreandNextPageTokenunconditionally. Align the comment with the behavior.📝 Proposed fix
// withPagination records a pagination cursor so a --format json -// consumer sees has_more / next_page_token alongside the content. No-op when -// hasMore is false. +// consumer sees has_more / next_page_token alongside the content. Both fields +// use omitempty, so a false hasMore and an empty cursor are omitted from JSON. func (e *fetchEnvelope) withPagination(hasMore bool, nextToken string) *fetchEnvelope {🤖 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/drive/drive_fetch_envelope.go` around lines 83 - 90, Update the doc comment for fetchEnvelope.withPagination to remove the inaccurate “No-op when hasMore is false” statement and describe that it always assigns HasMore and the trimmed NextPageToken.shortcuts/minutes/minutes_fetch.go (2)
106-115: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the local
contextvariable; it shadows the importedcontextpackage.The
contextpackage is imported at Line 7. The local slice at Line 106 hides it inside this function. Rename it to keep the package name available.♻️ Proposed fix
- var context []string + var details []string if hint := strings.TrimSpace(problem.Hint); hint != "" && !strings.Contains(warning, hint) { - context = append(context, "hint: "+hint) + details = append(details, "hint: "+hint) } if logID := strings.TrimSpace(problem.LogID); logID != "" { - context = append(context, "log_id: "+logID) + details = append(details, "log_id: "+logID) } - if len(context) > 0 { - warning += " (" + strings.Join(context, "; ") + ")" + if len(details) > 0 { + warning += " (" + strings.Join(details, "; ") + ")" }🤖 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/minutes/minutes_fetch.go` around lines 106 - 115, Rename the local slice variable context in the warning-enrichment block to a non-conflicting name, and update its append, length, and strings.Join references while preserving the existing hint and logID behavior. Keep the imported context package accessible within the function.
120-129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAttach
--includeto the validation error.
ParseIncludesreturns a typed validation error without a param. Both callers surface it for the--includeflag. Setting the param lets the CLI name the flag and lets error-path tests assert the metadata.♻️ Proposed fix
- return nil, common.ValidationErrorf("invalid --include value %q (allowed: transcript, note-doc)", v) + return nil, common.ValidationErrorf("invalid --include value %q (allowed: transcript, note-doc)", v).WithParam("--include")As per coding guidelines: "Error-path tests must assert typed metadata through
errs.ProblemOf(category,subtype, andparam)".🤖 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/minutes/minutes_fetch.go` around lines 120 - 129, Update ParseIncludes to attach the --include parameter when constructing the validation error for an invalid value, using the existing typed validation-error API so errs.ProblemOf reports the expected category, subtype, and param metadata. Keep the valid-value parsing and returned set unchanged.Source: Coding guidelines
shortcuts/drive/drive_fetch_dispatch.go (1)
219-250: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated
contentread.FetchOptionsconstruction.The same four-field
FetchOptionsliteral appears at Lines 29-34, Lines 67-72, and Lines 230-235. A single helper keeps the flag names in one place and prevents drift when a pagination flag is renamed.♻️ Proposed helper
+// fetchOptionsFor builds the shared content-read options from the fetch flags. +func fetchOptionsFor(runtime *common.RuntimeContext) contentread.FetchOptions { + return contentread.FetchOptions{ + MaxRows: runtime.Int("embed-max-rows"), + Full: runtime.Bool("full"), + PageToken: strings.TrimSpace(runtime.Str("page-token")), + PageSize: runtime.Int("page-size"), + } +}Then each call site becomes:
- maxRows := runtime.Int("embed-max-rows") - opts := contentread.FetchOptions{ - MaxRows: maxRows, - Full: runtime.Bool("full"), - PageToken: strings.TrimSpace(runtime.Str("page-token")), - PageSize: runtime.Int("page-size"), - } + opts := fetchOptionsFor(runtime)🤖 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/drive/drive_fetch_dispatch.go` around lines 219 - 250, Extract the repeated contentread.FetchOptions construction into a shared helper near the fetch functions, using the existing runtime values for embed-max-rows, full, page-token, and page-size. Update fetchWikiDirect and the other FetchMarkdown call sites that currently build the same literal to use this helper, keeping all existing pagination behavior unchanged.shortcuts/minutes/minutes_fetch_test.go (2)
93-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the metadata
optionalMinutesWarningembeds.This test asserts only the
"note documents omitted"prefix. The hint andlog_idembedding inoptionalMinutesWarning(shortcuts/minutes/minutes_fetch.goLines 107-115) is not asserted by any test in this file, so deleting that block would not fail the suite.Extend this test, or add one, so the warning string is asserted to carry the typed hint and the log ID from the upstream failure. The degraded value is a string by design, so assert the embedded metadata rather than calling
errs.ProblemOfon the warning.As per coding guidelines: "Every behavior change must have an accompanying test, and contract tests must assert the changed field or behavior directly so reverting the implementation causes failure."
🤖 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/minutes/minutes_fetch_test.go` around lines 93 - 108, Extend TestFetchMinutesMarkdownNoteFailureDegradesToWarning to assert that the warning includes the typed hint and upstream log ID produced by optionalMinutesWarning, in addition to the existing “note documents omitted” text. Read the metadata from the warning string directly rather than using errs.ProblemOf, and configure the mocked failure with stable hint and log ID values so reverting optionalMinutesWarning would fail the test.Source: Coding guidelines
141-147: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGuard against a missing title before comparing positions.
strings.Indexreturns-1for an absent title. IfrenderChaptersdropped a chapter,-1 < pos("Untimed")still holds and the ordering assertion passes. Assert that every position is non-negative first, so the regression guard cannot pass on missing output. The same applies to Lines 161-165.🤖 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/minutes/minutes_fetch_test.go` around lines 141 - 147, Update the ordering assertions around renderChapters and the corresponding check at lines 161-165 to first verify every strings.Index result is non-negative, then compare the positions. Preserve the existing failure reporting while ensuring missing chapter titles cannot satisfy the ordering condition.
🤖 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/contentread/anchored_markdown_test.go`:
- Around line 263-265: Update both assertions in the anchored Markdown tests
near the expanded synced-content checks to verify that the rendered output does
not contain the actual placeholder marker “内容可能未展开” emitted by renderEmbedTable.
Remove the ineffective “base 技能” checks so reverting the expanded-text behavior
causes these contract tests to fail.
In `@shortcuts/common/contentread/fetch_test.go`:
- Around line 187-189: Update the HTTP 500 assertion in the FetchDocInfo test to
extract the typed problem with errs.ProblemOf, then assert its Category,
populated Subtype, and param metadata while preserving the existing non-nil
error check. Also verify the underlying cause is preserved, without requiring a
specific Subtype constant.
In `@shortcuts/common/contentread/pagination.go`:
- Around line 15-17: Update ApplyPagination to validate pageSize before
converting it to int32, rejecting values above math.MaxInt32 rather than
allowing overflow or truncation; preserve the existing handling for positive
in-range values and non-positive values. Prefer propagating a typed validation
error at the CLI flag boundary if the surrounding callers support validation
errors.
In `@shortcuts/common/fetch_content_delivery_test.go`:
- Around line 279-282: Add an errors.Is(err, scan.BlockErr) assertion to the
PrepareFetchContentDelivery test alongside the existing blocked-result checks,
verifying that the returned error preserves scan.BlockErr as its cause while
retaining the existing ContentSafetyError classification coverage.
In `@shortcuts/doc/docs_fetch_v2_test.go`:
- Around line 1086-1100: Update TestValidatePaginatedReadFlagsRevisionConflict
to assert typed validation metadata instead of only matching err.Error(). Use
errs.ProblemOf to verify category and subtype, then use errors.As to extract
*errs.ValidationError and assert its Param value; preserve the existing conflict
scenario and failure expectations.
In `@shortcuts/doc/docs_fetch_v2_wiki_test.go`:
- Around line 410-426: Add a test covering ResolveFetchURLDetailed and the
wikiNodeURL shortcut branch for a node_type=shortcut response containing
origin_node_token. Assert that the resolved contracted URL targets the origin
node token, using wikiNodeStub or an equivalent stub setup and preserving
existing ParseResourceURL/BuildResourceURL tests.
In `@shortcuts/drive/drive_fetch_test.go`:
- Around line 69-76: Update the validation-error assertions in
shortcuts/drive/drive_fetch_test.go at lines 69-76 to use errs.ProblemOf for the
expected category and subtype, while retaining errors.As to verify
ValidationError.Param and the existing hint check. In
tests/cli_e2e/drive/drive_fetch_dryrun_test.go at lines 142-207, parse the
structured validation envelope from stderr for every rejected flag combination
and assert its error type, subtype, and parameter.
In `@shortcuts/drive/drive_fetch_validate.go`:
- Around line 31-33: Update the minutes validation branch in drive fetch
validation to remove the redundant fetchType interpolation and report the
pagination flag actually supplied. Add a firstPaginationFlag helper near
hasPaginationFlags, checking --full, --page-token, then --page-size, and use its
result for both the error message context and WithParam instead of the fixed
--full.
In `@shortcuts/drive/drive_skill_doc_test.go`:
- Around line 24-27: Replace the os.ReadFile call in the skill document test
with the repository filesystem abstraction from internal/vfs, using it to read
the committed lark-drive-fetch.md document while preserving the existing error
handling and assertion flow.
In `@skills/lark-drive/references/lark-drive-fetch.md`:
- Line 53: Update the `--token` + `--type` documentation row to remove `doc`
from the listed aliases, preserving only `sheets` → `sheet` and `base` →
`bitable`; keep `docx` as the distinct supported type.
In `@skills/lark-slides/SKILL.md`:
- Around line 88-89: Update the “读取指定页、检查版式 / 元素结构，或编辑前回读” guidance to
distinguish `slides +xml-get` as the shortcut from `xml_presentation.slide.get`
as the native API, and specify the correct command form for single-slide reads
while preserving the existing token and ID requirements.
---
Nitpick comments:
In `@shortcuts/common/contentread/anchored_markdown.go`:
- Around line 316-336: Update anchoredMarkdownRenderer.readText to treat io.EOF
from dec.Token as normal completion, returning the text collected so far with no
error, while continuing to propagate other decoder errors. Keep the existing
depth-based end-element handling unchanged.
In `@shortcuts/common/contentread/fetch.go`:
- Around line 26-28: Add a concise comment next to the `_ = ctx` statement in
`FetchDocInfo` explaining that `CallAPITyped` uses the context held by
`RuntimeContext`, so the method’s supplied context is intentionally unused.
In `@shortcuts/common/contentread/tables.go`:
- Line 26: Update gfmDelimiterRe and its use in the table-detection logic around
the line-48 check so delimiter rows must contain at least one pipe character.
Preserve matching of valid GFM delimiter rows, including single-column rows with
a leading or trailing pipe, while rejecting bare thematic breaks such as "---"
to prevent prose from being consumed as table rows.
In `@shortcuts/common/fetch_content_delivery.go`:
- Around line 56-57: Update the autoSpill condition in the content-fetch flow to
use len(content) instead of converting content to []byte for measurement, while
preserving the existing runtime.Bool, JqExpr, and threshold checks. Keep the
body := []byte(content) conversion used later by WriteTempMarkdown and
sha256.Sum256.
In `@shortcuts/doc/docs_fetch_v2_test.go`:
- Around line 1022-1024: Update the test command fixture that registers flags
alongside “full”, “page-token”, and “page-size” to also register
“embed-max-rows” with the production default of 50, so
runtime.Int("embed-max-rows") observes the configured row cap.
In `@shortcuts/doc/docs_skill_doc_test.go`:
- Around line 24-27: Update the os.ReadFile error handling in the test to call
t.Fatalf instead of t.Skipf when lark-doc-fetch.md cannot be read, ensuring the
flag-drift guard fails visibly when the tracked skill document is missing.
In `@shortcuts/drive/drive_fetch_dispatch.go`:
- Around line 219-250: Extract the repeated contentread.FetchOptions
construction into a shared helper near the fetch functions, using the existing
runtime values for embed-max-rows, full, page-token, and page-size. Update
fetchWikiDirect and the other FetchMarkdown call sites that currently build the
same literal to use this helper, keeping all existing pagination behavior
unchanged.
In `@shortcuts/drive/drive_fetch_envelope.go`:
- Around line 83-90: Update the doc comment for fetchEnvelope.withPagination to
remove the inaccurate “No-op when hasMore is false” statement and describe that
it always assigns HasMore and the trimmed NextPageToken.
In `@shortcuts/minutes/minutes_fetch_test.go`:
- Around line 93-108: Extend
TestFetchMinutesMarkdownNoteFailureDegradesToWarning to assert that the warning
includes the typed hint and upstream log ID produced by optionalMinutesWarning,
in addition to the existing “note documents omitted” text. Read the metadata
from the warning string directly rather than using errs.ProblemOf, and configure
the mocked failure with stable hint and log ID values so reverting
optionalMinutesWarning would fail the test.
- Around line 141-147: Update the ordering assertions around renderChapters and
the corresponding check at lines 161-165 to first verify every strings.Index
result is non-negative, then compare the positions. Preserve the existing
failure reporting while ensuring missing chapter titles cannot satisfy the
ordering condition.
In `@shortcuts/minutes/minutes_fetch.go`:
- Around line 106-115: Rename the local slice variable context in the
warning-enrichment block to a non-conflicting name, and update its append,
length, and strings.Join references while preserving the existing hint and logID
behavior. Keep the imported context package accessible within the function.
- Around line 120-129: Update ParseIncludes to attach the --include parameter
when constructing the validation error for an invalid value, using the existing
typed validation-error API so errs.ProblemOf reports the expected category,
subtype, and param metadata. Keep the valid-value parsing and returned set
unchanged.
In `@tests/cli_e2e/docs/docs_fetch_dryrun_test.go`:
- Around line 56-58: Replace the stderr substring assertion in the dry-run fetch
test with JSON envelope parsing. Assert the typed validation fields error.type,
error.subtype, error.param, and error.message, while preserving the expected
exit code 2 and stderr source.
🪄 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: b841c8ce-d93a-4c95-9ced-26d8bd4f73f0
📒 Files selected for processing (55)
extension/fileio/types.gointernal/contentartifact/temp.gointernal/contentartifact/temp_test.gointernal/output/emitter.gointernal/output/emitter_contract_test.gointernal/vfs/localfileio/localfileio.gointernal/vfs/localfileio/localfileio_test.goshortcuts/common/contentread/anchored_markdown.goshortcuts/common/contentread/anchored_markdown_table.goshortcuts/common/contentread/anchored_markdown_test.goshortcuts/common/contentread/fetch.goshortcuts/common/contentread/fetch_test.goshortcuts/common/contentread/images.goshortcuts/common/contentread/markdown.goshortcuts/common/contentread/pagination.goshortcuts/common/contentread/postprocess_test.goshortcuts/common/contentread/tables.goshortcuts/common/contentread/types.goshortcuts/common/fetch_content_delivery.goshortcuts/common/fetch_content_delivery_test.goshortcuts/common/resource_url.goshortcuts/common/runner.goshortcuts/common/wiki_node.goshortcuts/doc/docs_fetch_markdown.goshortcuts/doc/docs_fetch_spill_test.goshortcuts/doc/docs_fetch_v2.goshortcuts/doc/docs_fetch_v2_test.goshortcuts/doc/docs_fetch_v2_wiki_test.goshortcuts/doc/docs_skill_doc_test.goshortcuts/drive/drive_fetch.goshortcuts/drive/drive_fetch_dispatch.goshortcuts/drive/drive_fetch_envelope.goshortcuts/drive/drive_fetch_input.goshortcuts/drive/drive_fetch_spill_test.goshortcuts/drive/drive_fetch_test.goshortcuts/drive/drive_fetch_validate.goshortcuts/drive/drive_skill_doc_test.goshortcuts/drive/shortcuts.goshortcuts/drive/shortcuts_test.goshortcuts/minutes/minutes_fetch.goshortcuts/minutes/minutes_fetch_test.goskills/lark-base/SKILL.mdskills/lark-doc/SKILL.mdskills/lark-doc/references/lark-doc-fetch.mdskills/lark-drive/SKILL.mdskills/lark-drive/references/lark-drive-fetch.mdskills/lark-minutes/SKILL.mdskills/lark-sheets/SKILL.mdskills/lark-slides/SKILL.mdtests/cli_e2e/docs/coverage.mdtests/cli_e2e/docs/docs_create_fetch_test.gotests/cli_e2e/docs/docs_fetch_dryrun_test.gotests/cli_e2e/drive/coverage.mdtests/cli_e2e/drive/drive_fetch_dryrun_test.gotests/cli_e2e/drive/drive_fetch_workflow_test.go
| if strings.Contains(got, "base 技能") { | ||
| t.Errorf("expanded synced must not be a placeholder, got:\n%s", got) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert the real placeholder marker, not base 技能.
renderEmbedTable emits 内容可能未展开 for the placeholder branch (shortcuts/common/contentread/anchored_markdown.go Line 229). The string base 技能 is not produced anywhere in the package. Both assertions therefore always pass, and a revert of the expanded-text branch would not fail these tests.
Assert the absence of 内容可能未展开 instead.
As per coding guidelines: "contract tests must assert the changed field or behavior directly so reverting the implementation causes failure."
💚 Proposed assertion fix for both sites
- if strings.Contains(got, "base 技能") {
+ if strings.Contains(got, "内容可能未展开") {
t.Errorf("expanded synced must not be a placeholder, got:\n%s", got)
}- if strings.Contains(got, "base 技能") {
+ if strings.Contains(got, "内容可能未展开") {
t.Errorf("expanded component must not be a placeholder, got:\n%s", got)
}Also applies to: 282-284
🤖 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/contentread/anchored_markdown_test.go` around lines 263 -
265, Update both assertions in the anchored Markdown tests near the expanded
synced-content checks to verify that the rendered output does not contain the
actual placeholder marker “内容可能未展开” emitted by renderEmbedTable. Remove the
ineffective “base 技能” checks so reverting the expanded-text behavior causes
these contract tests to fail.
Source: Coding guidelines
| if _, err := FetchDocInfo(context.Background(), rt, Request{URL: "x"}); err == nil { | ||
| t.Fatal("expected error on HTTP 500") | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert typed error metadata on the HTTP 500 path.
This test only checks that the error is non-nil. A regression that returns an untyped error still passes. Assert the typed metadata through errs.ProblemOf.
Assert Category and a populated Subtype rather than a specific subtype constant, because the API layer may fall back to SubtypeUnknown.
As per coding guidelines: "Error-path tests must assert typed metadata through errs.ProblemOf (category, subtype, and param) and verify cause preservation rather than relying only on message substrings."
💚 Proposed typed assertions
- if _, err := FetchDocInfo(context.Background(), rt, Request{URL: "x"}); err == nil {
- t.Fatal("expected error on HTTP 500")
- }
+ _, err := FetchDocInfo(context.Background(), rt, Request{URL: "x"})
+ if err == nil {
+ t.Fatal("expected error on HTTP 500")
+ }
+ p, ok := errs.ProblemOf(err)
+ if !ok {
+ t.Fatalf("expected a typed errs.* error, got %T: %v", err, err)
+ }
+ if p.Category == "" || p.Subtype == "" {
+ t.Errorf("problem metadata incomplete: category=%q subtype=%q", p.Category, p.Subtype)
+ }Based on learnings: in larksuite/cli Go test suites for shortcuts domain packages, when exercising error paths, avoid asserting a specific Subtype constant that may not exist for that layer; assert Category and verify Subtype is populated.
🤖 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/contentread/fetch_test.go` around lines 187 - 189, Update
the HTTP 500 assertion in the FetchDocInfo test to extract the typed problem
with errs.ProblemOf, then assert its Category, populated Subtype, and param
metadata while preserving the existing non-nil error check. Also verify the
underlying cause is preserved, without requiring a specific Subtype constant.
Sources: Coding guidelines, Learnings
| if pageSize > 0 { | ||
| req.PageSize = int32(pageSize) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Bound pageSize before narrowing to int32.
pageSize comes from the user-supplied --page-size flag (see shortcuts/drive/drive_fetch_dispatch.go Line 30 and shortcuts/doc/docs_fetch_markdown.go Line 149). On 64-bit platforms a value above math.MaxInt32 wraps and produces a negative or truncated page_size in the request body. The CLI then sends a value the caller did not request.
Reject or clamp out-of-range values instead of coercing them silently.
As per coding guidelines: "never silently coerce unsupported inputs, ignore unhonored options, default missing identities, or discard writes."
🛡️ Proposed clamp inside `ApplyPagination`
req.EnablePagination = true
req.PageToken = strings.TrimSpace(pageToken)
- if pageSize > 0 {
- req.PageSize = int32(pageSize)
- }
+ if pageSize > 0 {
+ if pageSize > math.MaxInt32 {
+ pageSize = math.MaxInt32
+ }
+ req.PageSize = int32(pageSize)
+ }Add "math" to the import block. Prefer a typed validation error at the flag boundary if the callers should reject the value instead.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 15-15: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values.
Context: int32(pageSize)
Note: [CWE-190] Integer Overflow or Wraparound.
(integer-overflow-narrowing-conversion-go)
🤖 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/contentread/pagination.go` around lines 15 - 17, Update
ApplyPagination to validate pageSize before converting it to int32, rejecting
values above math.MaxInt32 rather than allowing overflow or truncation; preserve
the existing handling for positive in-range values and non-positive values.
Prefer propagating a typed validation error at the CLI flag boundary if the
surrounding callers support validation errors.
Sources: Coding guidelines, Linters/SAST tools
| delivery, scan, err := PrepareFetchContentDelivery(rctx, map[string]any{"content": content}, content, testFetchContentJQPath) | ||
| if err == nil || !scan.Blocked { | ||
| t.Fatalf("PrepareFetchContentDelivery() = (%#v, blocked=%t, %v), want content-safety block", delivery, scan.Blocked, err) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Determine the typed category and subtype produced for a content-safety block.
set -euo pipefail
echo "--- ScanResult and BlockErr construction ---"
rg -nP --type=go -C6 '\bBlockErr\b'
echo "--- ScanForSafety implementation ---"
ast-grep run --pattern 'func ScanForSafety($$$) $_ { $$$ }' --lang go internal/outputRepository: larksuite/cli
Length of output: 196
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- repo files matching test ---"
git ls-files | rg 'shortcuts/common/fetch_content_delivery_test\.go|content.*delivery|fetch.*content|errors|errs|problem' || true
echo "--- target file outline ---"
ast-grep outline shortcuts/common/fetch_content_delivery_test.go --view compact || true
echo "--- target test relevant lines ---"
sed -n '240,310p' shortcuts/common/fetch_content_delivery_test.go || true
echo "--- Search for PrepareFetchContentDelivery definitions/usages ---"
rg -n --type=go -C8 'PrepareFetchContentDelivery|ScanResult|ScanForSafety|BlockErr|content-safety|content safe|unsafe' . || trueRepository: larksuite/cli
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- target test relevant lines ---"
sed -n '260,295p' shortcuts/common/fetch_content_delivery_test.go
echo "--- imports in target test ---"
sed -n '1,40p' shortcuts/common/fetch_content_delivery_test.go
echo "--- implementation around safety scan ---"
sed -n '47,55p' shortcuts/common/fetch_content_delivery.go
echo "--- scan structs and test mocks ---"
rg -n --type=go -C12 'ScanForSafety|ScanResult|runtime\.(\*|Mock).*Scan|func \(.*ScanOutputForSafety|ScanOutputForSafety' shortcuts/common/fetch_content_delivery_test.go shortcuts/common/fetch_content_delivery.go internal/output shortcuts common 2>/dev/null || true
echo "--- focused scan result definitions ---"
rg -n --type=go -C8 'type ScanResult|BlockErr|ScanOutputForSafety' internal shortcuts cmd 2>/dev/null | head -n 240Repository: larksuite/cli
Length of output: 50369
Assert cause preservation for the block error.
PrepareFetchContentDelivery returns scan.BlockErr directly, so add an errors.Is(err, scan.BlockErr) check. The existing scan test covers the typed ContentSafetyError classification.
🤖 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/fetch_content_delivery_test.go` around lines 279 - 282, Add
an errors.Is(err, scan.BlockErr) assertion to the PrepareFetchContentDelivery
test alongside the existing blocked-result checks, verifying that the returned
error preserves scan.BlockErr as its cause while retaining the existing
ContentSafetyError classification coverage.
Source: Coding guidelines
| func TestValidatePaginatedReadFlagsRevisionConflict(t *testing.T) { | ||
| t.Parallel() | ||
| rt := newFetchShortcutTestRuntime(t, "", map[string]string{ | ||
| "doc-format": "markdown", | ||
| "revision-id": "42", | ||
| "full": "true", | ||
| }) | ||
| err := validatePaginatedReadFlags(rt) | ||
| if err == nil { | ||
| t.Fatal("expected conflict error for --full + historical --revision-id") | ||
| } | ||
| if !strings.Contains(err.Error(), "revision-id") { | ||
| t.Fatalf("error should blame --revision-id, got: %v", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Assert typed error metadata instead of a message substring.
This error-path test checks only err.Error(). A message reword breaks the test without a behavior change, and a wrong param or subtype passes it. Assert category, subtype, and param. Read Param through errors.As into *errs.ValidationError, because errs.ProblemOf does not expose it.
♻️ Proposed typed assertions
err := validatePaginatedReadFlags(rt)
if err == nil {
t.Fatal("expected conflict error for --full + historical --revision-id")
}
- if !strings.Contains(err.Error(), "revision-id") {
- t.Fatalf("error should blame --revision-id, got: %v", err)
+ problem, ok := errs.ProblemOf(err)
+ if !ok || problem.Category != errs.CategoryValidation || problem.Subtype != errs.SubtypeInvalidArgument {
+ t.Fatalf("problem = %#v, want a validation/invalid-argument problem", problem)
+ }
+ var validationErr *errs.ValidationError
+ if !errors.As(err, &validationErr) || validationErr.Param != "--full" {
+ t.Fatalf("param = %#v, want --full", err)
+ }
+ if !strings.Contains(err.Error(), "revision-id") {
+ t.Fatalf("message should name --revision-id, got: %v", err)
}Based on learnings and coding guidelines: "Error-path tests must assert typed metadata through errs.ProblemOf (category, subtype, and param)", and errs.ProblemOf returns a Problem without a Param field, so Param must be read via errors.As on *errs.ValidationError.
📝 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.
| func TestValidatePaginatedReadFlagsRevisionConflict(t *testing.T) { | |
| t.Parallel() | |
| rt := newFetchShortcutTestRuntime(t, "", map[string]string{ | |
| "doc-format": "markdown", | |
| "revision-id": "42", | |
| "full": "true", | |
| }) | |
| err := validatePaginatedReadFlags(rt) | |
| if err == nil { | |
| t.Fatal("expected conflict error for --full + historical --revision-id") | |
| } | |
| if !strings.Contains(err.Error(), "revision-id") { | |
| t.Fatalf("error should blame --revision-id, got: %v", err) | |
| } | |
| } | |
| func TestValidatePaginatedReadFlagsRevisionConflict(t *testing.T) { | |
| t.Parallel() | |
| rt := newFetchShortcutTestRuntime(t, "", map[string]string{ | |
| "doc-format": "markdown", | |
| "revision-id": "42", | |
| "full": "true", | |
| }) | |
| err := validatePaginatedReadFlags(rt) | |
| if err == nil { | |
| t.Fatal("expected conflict error for --full + historical --revision-id") | |
| } | |
| problem, ok := errs.ProblemOf(err) | |
| if !ok || problem.Category != errs.CategoryValidation || problem.Subtype != errs.SubtypeInvalidArgument { | |
| t.Fatalf("problem = %#v, want a validation/invalid-argument problem", problem) | |
| } | |
| var validationErr *errs.ValidationError | |
| if !errors.As(err, &validationErr) || validationErr.Param != "--full" { | |
| t.Fatalf("param = %#v, want --full", err) | |
| } | |
| if !strings.Contains(err.Error(), "revision-id") { | |
| t.Fatalf("message should name --revision-id, got: %v", 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/docs_fetch_v2_test.go` around lines 1086 - 1100, Update
TestValidatePaginatedReadFlagsRevisionConflict to assert typed validation
metadata instead of only matching err.Error(). Use errs.ProblemOf to verify
category and subtype, then use errors.As to extract *errs.ValidationError and
assert its Param value; preserve the existing conflict scenario and failure
expectations.
Sources: Coding guidelines, Learnings
| err := validateFetchTypeFlags(runtime, "minutes") | ||
| var validationErr *errs.ValidationError | ||
| if !errors.As(err, &validationErr) { | ||
| t.Fatalf("validateFetchTypeFlags() error = %T %v, want validation error", err, err) | ||
| } | ||
| if validationErr.Param != "--as" || !strings.Contains(validationErr.Hint, "--as user") { | ||
| t.Fatalf("validation error = %#v", validationErr) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the typed validation-error contract in both test layers.
The tests can pass after a category or subtype regression because they primarily assert text.
shortcuts/drive/drive_fetch_test.go#L69-L76: Useerrs.ProblemOfto assert the validation category and subtype. Keeperrors.Asto assertValidationError.Param.tests/cli_e2e/drive/drive_fetch_dryrun_test.go#L142-L207: Parse the structured validation envelope from stderr. Assert error type, subtype, and parameter for each rejected flag combination.
As per coding guidelines, “Error-path tests must assert typed metadata through errs.ProblemOf (category, subtype, and param)”. Based on learnings, errs.ProblemOf does not expose Param; use errors.As for that assertion.
📍 Affects 2 files
shortcuts/drive/drive_fetch_test.go#L69-L76(this comment)tests/cli_e2e/drive/drive_fetch_dryrun_test.go#L142-L207
🤖 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/drive/drive_fetch_test.go` around lines 69 - 76, Update the
validation-error assertions in shortcuts/drive/drive_fetch_test.go at lines
69-76 to use errs.ProblemOf for the expected category and subtype, while
retaining errors.As to verify ValidationError.Param and the existing hint check.
In tests/cli_e2e/drive/drive_fetch_dryrun_test.go at lines 142-207, parse the
structured validation envelope from stderr for every rejected flag combination
and assert its error type, subtype, and parameter.
Sources: Coding guidelines, Learnings
| if hasPaginationFlags(runtime) && fetchType == "minutes" { | ||
| return common.ValidationErrorf("--full/--page-token/--page-size do not apply to minutes (got %s)", fetchType).WithParam("--full") | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Report the offending pagination flag, and drop the redundant (got %s).
Inside this branch fetchType is always "minutes", so the message renders as "do not apply to minutes (got minutes)". WithParam("--full") is also fixed, so a user who passed only --page-size is pointed at --full.
🐛 Proposed fix
- if hasPaginationFlags(runtime) && fetchType == "minutes" {
- return common.ValidationErrorf("--full/--page-token/--page-size do not apply to minutes (got %s)", fetchType).WithParam("--full")
- }
+ if fetchType == "minutes" {
+ if param := firstPaginationFlag(runtime); param != "" {
+ return common.ValidationErrorf("--full/--page-token/--page-size do not apply to minutes").WithParam(param)
+ }
+ }Add the helper next to hasPaginationFlags:
// firstPaginationFlag names the pagination flag the caller set, or "" when none is set.
func firstPaginationFlag(runtime *common.RuntimeContext) string {
switch {
case runtime.Bool("full"):
return "--full"
case strings.TrimSpace(runtime.Str("page-token")) != "":
return "--page-token"
case runtime.Int("page-size") > 0:
return "--page-size"
}
return ""
}📝 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.
| if hasPaginationFlags(runtime) && fetchType == "minutes" { | |
| return common.ValidationErrorf("--full/--page-token/--page-size do not apply to minutes (got %s)", fetchType).WithParam("--full") | |
| } | |
| if fetchType == "minutes" { | |
| if param := firstPaginationFlag(runtime); param != "" { | |
| return common.ValidationErrorf("--full/--page-token/--page-size do not apply to minutes").WithParam(param) | |
| } | |
| } |
🤖 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/drive/drive_fetch_validate.go` around lines 31 - 33, Update the
minutes validation branch in drive fetch validation to remove the redundant
fetchType interpolation and report the pagination flag actually supplied. Add a
firstPaginationFlag helper near hasPaginationFlags, checking --full,
--page-token, then --page-size, and use its result for both the error message
context and WithParam instead of the fixed --full.
| data, err := os.ReadFile(filepath.Join("..", "..", "skills", "lark-drive", "references", "lark-drive-fetch.md")) | ||
| if err != nil { | ||
| t.Fatalf("skill doc not found: %v", err) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the repository filesystem abstraction.
os.ReadFile bypasses the required internal/vfs filesystem APIs. Read the committed skill document through the repository filesystem abstraction instead. The temporary-fixture exception does not apply to this repository document.
🤖 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/drive/drive_skill_doc_test.go` around lines 24 - 27, Replace the
os.ReadFile call in the skill document test with the repository filesystem
abstraction from internal/vfs, using it to read the committed
lark-drive-fetch.md document while preserving the existing error handling and
assertion flow.
Source: Coding guidelines
| | 参数 | 必填 | 说明 | | ||
| |---|---|---| | ||
| | `--url` | 二选一 | 文档 URL（推荐） | | ||
| | `--token` + `--type` | 二选一 | 裸 token 需 `--type`（docx / sheet / bitable / slides / file / minutes / wiki；也接受别名 doc / sheets / base） | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
doc is not an alias of docx.
This row lists doc together with sheets and base as aliases. In normalizeFetchType (shortcuts/drive/drive_fetch_input.go Lines 104-110) only sheets → sheet and base → bitable are aliases. doc and docx stay distinct, as the comment at Line 27 of that file states. A reader who treats doc as an alias will pass --type doc for a docx token and get a wrong-type request.
📝 Proposed fix
-| `--token` + `--type` | 二选一 | 裸 token 需 `--type`（docx / sheet / bitable / slides / file / minutes / wiki；也接受别名 doc / sheets / base） |
+| `--token` + `--type` | 二选一 | 裸 token 需 `--type`（doc / docx / sheet / bitable / slides / file / minutes / wiki；`doc` 与 `docx` 是不同类型，不可互换；别名：`sheets` = `sheet`，`base` = `bitable`） |📝 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.
| | `--token` + `--type` | 二选一 | 裸 token 需 `--type`（docx / sheet / bitable / slides / file / minutes / wiki；也接受别名 doc / sheets / base） | | |
| | `--token` + `--type` | 二选一 | 裸 token 需 `--type`（doc / docx / sheet / bitable / slides / file / minutes / wiki；`doc` 与 `docx` 是不同类型，不可互换；别名：`sheets` = `sheet`，`base` = `bitable`） | |
🤖 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-drive/references/lark-drive-fetch.md` at line 53, Update the
`--token` + `--type` documentation row to remove `doc` from the listed aliases,
preserving only `sheets` → `sheet` and `base` → `bitable`; keep `docx` as the
distinct supported type.
| | 速览 / 理解 / 总结整份 PPT（只关注内容） | `drive +fetch --url "<原 URL>"` 直接读取 Markdown | [`lark-drive-fetch.md`](../lark-drive/references/lark-drive-fetch.md) | | ||
| | 读取指定页、检查版式 / 元素结构，或编辑前回读 | 解析 slides/wiki token，用 shortcut 回读全文 XML 或读取单页 XML，保存 `xml_presentation_id`、`slide_id`、`revision_id` | `slides +xml-get`、`xml_presentation.slide.get`、`lark-slides-xml-presentations-get.md` | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Identify xml_presentation.slide.get as a native API.
Line 89 calls both XML read paths a “shortcut”. slides +xml-get is a shortcut, but xml_presentation.slide.get is a native API. Update the wording so agents use the correct command form for single-slide reads.
🤖 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-slides/SKILL.md` around lines 88 - 89, Update the “读取指定页、检查版式 /
元素结构，或编辑前回读” guidance to distinguish `slides +xml-get` as the shortcut from
`xml_presentation.slide.get` as the native API, and specify the correct command
form for single-slide reads while preserving the existing token and ID
requirements.
| return dec.Skip() | ||
| default: | ||
| // Unknown wrapper (including the synthetic contentroot): descend into children. | ||
| return r.renderChildren(dec, name) |
There was a problem hiding this comment.
[P1] Do not silently flatten existing Docx content
Both docs +fetch --doc-format markdown for whole-document reads and drive +fetch now treat this renderer as successful, but it has no native <table> handler and readText discards nested elements and their attributes. For example, a table containing <a href="https://example.com">value</a> currently renders as just value\n, losing both the table structure and the link destination, so the document API fallback never runs. This is a silent fidelity regression from the previous Markdown path. Please keep the existing path until the renderer preserves the supported Docx XML semantics, or make the renderer fidelity-complete before treating this read as successful.
| // --full reads may replace inline content with a local file descriptor. | ||
| func emitPaginatedMarkdown(runtime *common.RuntimeContext, content, title string, updateTime int64, hasMore bool, nextPageToken string) error { | ||
| nextPageToken = strings.TrimSpace(nextPageToken) | ||
| data := map[string]interface{}{ |
There was a problem hiding this comment.
[P1] Preserve the existing docs fetch response contract
Whole-document Markdown reads previously forwarded the Docs API response, so existing consumers could read fields such as data.document.document_id, revision_id, and any returned reference_map, as well as data.tips. This synthetic response only keeps content, title, and update_time, so an existing command such as docs +fetch --doc-format markdown --jq ".data.document.document_id" now returns null. The bundled lark-doc-fetch.md contract still documents these fields. Please retain the existing metadata when adding pagination, or avoid switching the established command to a response shape that silently removes public fields.
3 participants
Summary
Add a unified
drive +fetchshortcut for readable Markdown snapshots across Docs, Sheets, Base, Slides, Drive files, Minutes, and Wiki-backed resources. Reuse the shared content-read path for whole-document Markdown indocs +fetch, and update the embedded Skills to route overview reads while preserving entity-native structured and editing workflows.Changes
drive +fetchwith URL auto-detection or explicit token/type input, Wiki unwrapping and source metadata, selector preservation, Minutes artifacts, and a consistent resource envelope.docs +fetchthrough the paginated anchored-Markdown path, while preserving scoped/XML reads and the document API path for historical revisions or explicit language selection.--fullresponses through private temporary Markdown files after content-safety scanning, with inline fallback when local file delivery is unavailable.Test Plan
go vet ./...gofmt -l .produces no outputgo mod tidyproduces no module changesgo run github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.1.6 run --new-from-rev=upstream/mainreports 0 issuesnode scripts/skill-format-check/index.js skillsmake unit-test: every candidate-touched package passes, but the full run currently hits the upstreamshortcuts/imfailure documented belowUpstream baseline note:
upstream/mainatb546516bfailsTestAllIMShortcutsUseAffordanceExamples/+messages-searchbecause the newly dual-identity shortcut example still pins--as user. The same failure reproduces in a clean worktree atb546516b; this PR does not modify any IM files.Related Issues
Summary by CodeRabbit
New Features
drive +fetchfor unified Markdown retrieval across documents, sheets, bases, slides, files, Minutes, and Wiki content.Bug Fixes
Documentation