fix(docs): resolve wiki links before fetch - #1572
Conversation
Spec source: active@ecd83f429f36744bd8734710aa90b10e35ea1c93f974288da42551032cb7201e
|
|
📝 WalkthroughWalkthroughWiki document references now resolve through the wiki node API before fetches run. Dry-run output and tests were updated to cover the extra resolve step, resolved document IDs, and unescaped JSON output. ChangesWiki document fetch and dry-run JSON output
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
shortcuts/doc/docs_fetch_v2_test.go (1)
177-177: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
LARKSUITE_CLI_CONFIG_DIRsetup withTestFactory.This test builds the CLI via
cmdutil.TestFactory, which supplies an in-memory config closure and does not read from the filesystem, so thet.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())line is unnecessary here.Based on learnings: only set
LARKSUITE_CLI_CONFIG_DIRwhen the test exercises the realNewDefault()factory path; shortcut tests usingcmdutil.TestFactory(t, config)should not set 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/docs_fetch_v2_test.go` at line 177, Remove the redundant LARKSUITE_CLI_CONFIG_DIR setup from this shortcut test, since TestFactory already uses the in-memory config path and does not touch the filesystem. Keep the test focused on the cmdutil.TestFactory-based flow in docs_fetch_v2_test.go and only use LARKSUITE_CLI_CONFIG_DIR in tests that go through the real NewDefault factory.Source: Learnings
shortcuts/doc/docs_fetch_v2.go (1)
80-80: 📐 Maintainability & Code Quality | 🔵 TrivialEncode
documentIDfor defensive path construction.Consistent with
shortcuts/drive/drive_export.go(lines 62, 115), usevalidate.EncodePathSegmentwhen interpolating identifiers into URL paths to prevent injection risks and handle special characters, even if the ID source appears vetted.Code change
apiPath := fmt.Sprintf("/open-apis/docs_ai/v1/documents/%s/fetch", documentID)Should be:
apiPath := fmt.Sprintf("/open-apis/docs_ai/v1/documents/%s/fetch", validate.EncodePathSegment(documentID))🤖 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.go` at line 80, The docs fetch path is building a URL with an unencoded document identifier, which should be hardened. Update the `docsFetchV2` path construction to encode `documentID` with `validate.EncodePathSegment` before interpolating it into the `/open-apis/docs_ai/v1/documents/.../fetch` route, following the same defensive pattern used in `drive_export.go` and preserving the existing `apiPath` logic.
🤖 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.
Nitpick comments:
In `@shortcuts/doc/docs_fetch_v2_test.go`:
- Line 177: Remove the redundant LARKSUITE_CLI_CONFIG_DIR setup from this
shortcut test, since TestFactory already uses the in-memory config path and does
not touch the filesystem. Keep the test focused on the cmdutil.TestFactory-based
flow in docs_fetch_v2_test.go and only use LARKSUITE_CLI_CONFIG_DIR in tests
that go through the real NewDefault factory.
In `@shortcuts/doc/docs_fetch_v2.go`:
- Line 80: The docs fetch path is building a URL with an unencoded document
identifier, which should be hardened. Update the `docsFetchV2` path construction
to encode `documentID` with `validate.EncodePathSegment` before interpolating it
into the `/open-apis/docs_ai/v1/documents/.../fetch` route, following the same
defensive pattern used in `drive_export.go` and preserving the existing
`apiPath` logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9e660fd6-d9d5-400a-9b10-bf782bab8b42
📒 Files selected for processing (4)
shortcuts/doc/doc_media_test.goshortcuts/doc/docs_fetch_v2.goshortcuts/doc/docs_fetch_v2_test.goshortcuts/doc/helpers.go
github-actions
Bot
added
size/L
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/cmdutil/dryrun.go (1)
130-141: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffEncoder logic duplicates
output.PrintJsonNoHTMLEscape.
marshalJSONNoHTMLEscapehere and the encoder block ininternal/output/print.go(PrintJsonNoHTMLEscape) implement the samejson.Encoder+SetEscapeHTML(false)+ trailing-newline handling. Since this package already importsoutput, consider exposing a singleoutput-level byte helper and reusing it to keep the no-escape behavior centralized.Note: the two-layer design is correct here —
DryRunAPI.MarshalJSONreturns unescaped bytes and the outerPrintJsonNoHTMLEscapealso disables escaping, so the compaction step does not re-escape</>. Both must stay in sync, which is an additional reason to share one helper.🤖 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/cmdutil/dryrun.go` around lines 130 - 141, The no-HTML-escape JSON encoding logic is duplicated between marshalJSONNoHTMLEscape and output.PrintJsonNoHTMLEscape, so centralize it in one shared output-level byte helper and reuse it from both places. Update the existing helper in internal/output/print.go (or add a reusable byte-returning helper there) to handle json.Encoder, SetEscapeHTML(false), and trimming the trailing newline, then have DryRunAPI.MarshalJSON call that shared helper instead of maintaining its own copy. Ensure the no-escape behavior stays identical in both paths.
🤖 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.
Nitpick comments:
In `@internal/cmdutil/dryrun.go`:
- Around line 130-141: The no-HTML-escape JSON encoding logic is duplicated
between marshalJSONNoHTMLEscape and output.PrintJsonNoHTMLEscape, so centralize
it in one shared output-level byte helper and reuse it from both places. Update
the existing helper in internal/output/print.go (or add a reusable
byte-returning helper there) to handle json.Encoder, SetEscapeHTML(false), and
trimming the trailing newline, then have DryRunAPI.MarshalJSON call that shared
helper instead of maintaining its own copy. Ensure the no-escape behavior stays
identical in both paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 75de0993-47e7-4694-b2a9-72ab8ee67180
📒 Files selected for processing (6)
internal/cmdutil/dryrun.gointernal/cmdutil/dryrun_test.gointernal/output/print.goshortcuts/common/runner.goshortcuts/doc/docs_fetch_v2.goshortcuts/doc/docs_fetch_v2_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- shortcuts/doc/docs_fetch_v2.go
- shortcuts/doc/docs_fetch_v2_test.go
3 participants
业务背景
Fixes #1034.
When users pass a Wiki URL to
docs +fetch, the CLI should not treat the wiki node token as a document token. It must resolve the Wiki node first, then fetch the underlyingdoc/docxobject token. Prompt-only guidance is not stable enough for this behavior.实现要点
docs +fetchexecution viaGET /open-apis/wiki/v2/spaces/get_node.降级与局限
无。本 PR only changes
docs +fetch; it does not implement cross-shortcut dispatch for Sheets/Base/Slides.验证
go test ./shortcuts/doc -run 'TestDocsFetchWiki|TestDocsFetchDryRunWiki'go test ./shortcuts/docgit diff --check origin/main...HEADmake fmt-checkmake vetGOFLAGS=-buildvcs=false make unit-testNote: plain
make unit-testfailed in this RFC 209 worktree becausego listcould not obtain VCS status ininternal/qualitygate/deptestand suggested-buildvcs=false; the identical target passed withGOFLAGS=-buildvcs=false.AI Review Context
wiki/v2/spaces/get_nodebefore docs fetch and does not use the raw wiki token as document_id.obj_type=docxresolves toobj_tokenand fetches that document.--docparam and actionable hint.shortcuts/docSummary by CodeRabbit