feat(docs): default XML fetch to visible comments - #2247
Open
SunPeiYang996 wants to merge 19 commits into
Open
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:
📝 WalkthroughWalkthrough
ChangesFormat-based document comments
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant DocsFetch
participant DocumentService
participant OutputFormatter
Caller->>DocsFetch: request document fetch with format
DocsFetch->>DocumentService: fetch with format-specific extra parameters
DocumentService-->>DocsFetch: return document and comment data
DocsFetch->>OutputFormatter: render JSON or pretty output
OutputFormatter-->>Caller: return formatted document
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 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
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2247 +/- ##
=======================================
Coverage 76.36% 76.36%
=======================================
Files 1011 1011
Lines 111269 111273 +4
=======================================
+ Hits 84970 84974 +4
Misses 19815 19815
Partials 6484 6484 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
SunPeiYang996
marked this pull request as ready for review
August 8, 2026 14:53
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/doc/docs_fetch_v2_test.go`:
- Around line 593-606: The test
TestValidateFetchV2CommentsRunsConditionalScopeCheck only covers the permitted
path. Add a denied-scope case using a bot runtime that lacks
docsFetchCommentReadScope, invoke validateFetchV2 with comments enabled, and
assert failure through the checkShortcutScopes contract: verify the typed error
category, subtype, parameter, and preserved cause directly.
In `@shortcuts/doc/docs_fetch_v2.go`:
- Around line 186-190: Update the fetch validation flow around
shouldIncludeFetchComments and effectiveFetchReadMode so --comments combined
with --scope outline returns the established typed validation error instead of
being treated as valid with comments omitted. Preserve normal outline behavior
without --comments, and update the outline cases in docs_fetch_v2_test.go to
expect rejection rather than success.
In `@skills/lark-doc/references/lark-doc-fetch.md`:
- Around line 97-103: Update the comment response example’s identity field near
reference_map.comments from "user" to "bot", while leaving the surrounding
reference_map and comments structure unchanged.
- Line 117: Stop exposing raw engine comment IDs and keep public discussion
references opaque cN values. In
skills/lark-doc/references/lark-doc-fetch.md:117-117, remove the statement that
discussion.comment-id exposes a comment ID; at 134-134, remove comment-id from
the XML example. In
tests/cli_e2e/docs/docs_fetch_comments_workflow_test.go:161-161, stop comparing
sidecar data with data.comment_id; at 315-315, stop requiring a numeric public
comment-id attribute.
🪄 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: 71c37467-cdf0-44f9-ba18-a7bf4b390a46
📒 Files selected for processing (7)
shortcuts/doc/docs_fetch.goshortcuts/doc/docs_fetch_v2.goshortcuts/doc/docs_fetch_v2_test.goskills/lark-doc/SKILL.mdskills/lark-doc/references/lark-doc-fetch.mdtests/cli_e2e/docs/docs_fetch_comments_workflow_test.gotests/cli_e2e/docs/docs_fetch_dryrun_test.go
Comment on lines
+593
to
+606
| func TestValidateFetchV2CommentsRunsConditionalScopeCheck(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| config := &core.CliConfig{AppID: "test-app"} | ||
| factory, _, _, _ := cmdutil.TestFactory(t, config) | ||
| base := newFetchBodyTestRuntime(context.Background()) | ||
| runtime := common.TestNewRuntimeContextForAPI(context.Background(), base.Cmd, config, factory, core.AsBot) | ||
| runtime.Format = "json" | ||
| mustSetFetchFlag(t, runtime, "comments", "true") | ||
|
|
||
| if err := validateFetchV2(context.Background(), runtime); err != nil { | ||
| t.Fatalf("validateFetchV2() err=%v", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Add a denied-scope test.
This test only confirms that a permitted setup succeeds. If runtime.EnsureScopes is removed from validateFetchV2, it still passes.
Configure a bot runtime without docsFetchCommentReadScope. Assert that validation fails. Assert the typed error category, subtype, parameter, and preserved cause according to the checkShortcutScopes error contract.
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/doc/docs_fetch_v2_test.go` around lines 593 - 606, The test
TestValidateFetchV2CommentsRunsConditionalScopeCheck only covers the permitted
path. Add a denied-scope case using a bot runtime that lacks
docsFetchCommentReadScope, invoke validateFetchV2 with comments enabled, and
assert failure through the checkShortcutScopes contract: verify the typed error
category, subtype, parameter, and preserved cause directly.
Source: Coding guidelines
Comment on lines
+186
to
+190
| func shouldIncludeFetchComments(runtime *common.RuntimeContext) bool { | ||
| // Outline is a directory-only view and intentionally performs no comment | ||
| // query, even when a caller reuses a flag bundle containing --comments. | ||
| return runtime.Bool("comments") && effectiveFetchReadMode(runtime) != "outline" | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject --comments with --scope outline.
Line 189 returns false for this combination. This skips format, identity, and scope validation. The command then succeeds without requesting comments.
Return a typed validation error for this unsupported combination. Update the outline cases in shortcuts/doc/docs_fetch_v2_test.go that currently accept the ignored flag.
Proposed fix
func validateFetchV2(_ context.Context, runtime *common.RuntimeContext) error {
if err := validateDocsV2Only(runtime, "+fetch", docsFetchLegacyFlags()); err != nil {
return err
}
...
if err := validateReadModeFlags(runtime); err != nil {
return err
}
+ if runtime.Bool("comments") && effectiveFetchReadMode(runtime) == "outline" {
+ return common.ValidationErrorf("--comments is not supported with --scope outline").WithParam("--comments")
+ }
if err := validateFetchCommentDocFormat(runtime); err != nil {As per coding guidelines, “never silently coerce unsupported inputs, ignore unhonored options ... Return a typed validation error when a requested behavior cannot be honored.”
🤖 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` around lines 186 - 190, Update the fetch
validation flow around shouldIncludeFetchComments and effectiveFetchReadMode so
--comments combined with --scope outline returns the established typed
validation error instead of being treated as valid with comments omitted.
Preserve normal outline behavior without --comments, and update the outline
cases in docs_fetch_v2_test.go to expect rejection rather than success.
Source: Coding guidelines
Comment on lines
+97
to
103
| }, | ||
| "comments": { | ||
| "c1": { | ||
| "data": "<discussion timezone=\"Asia/Shanghai\" comment-id=\"739284756192837\">...</discussion>" | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the identity in the comment response example.
The example includes reference_map.comments but shows "identity": "user" on Line 86. The CLI rejects comment fetches for user identity. Set the example identity to "bot".
🤖 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` around lines 97 - 103, Update
the comment response example’s identity field near reference_map.comments from
"user" to "bot", while leaving the surrounding reference_map and comments
structure unchanged.
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@c82053c1904a9e04dc7cdb7d71cd82ef2ac6bf9e🧩 Skill updatenpx skills add larksuite/cli#sun/docx-fetch-comments-v2 -y -g |
SunPeiYang996
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
shortcuts/doc/docs_fetch_v2_test.go (1)
627-629: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert that Markdown omits
include_comments.A missing map key and a
falsevalue both evaluate tofalsehere. The Markdown cases pass if the request sends"include_comments":false, although the contract requires the default comment-free parameter set.Assert key presence for XML. Assert key absence for Markdown formats.
Proposed test fix
- if got["include_comments"] != tt.wantComments { - t.Fatalf("include_comments=%v, want %v in %#v", got["include_comments"], tt.wantComments, got) + if tt.wantComments { + if got["include_comments"] != true { + t.Fatalf("include_comments=%v, want true in %#v", got["include_comments"], got) + } + } else if _, ok := got["include_comments"]; ok { + t.Fatalf("include_comments must be omitted for %s: %#v", tt.docFormat, got) }As per coding guidelines, “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/doc/docs_fetch_v2_test.go` around lines 627 - 629, Update the assertions in the test covering the `include_comments` request parameter: for XML cases, assert the key is present and matches `tt.wantComments`; for Markdown cases, assert the `include_comments` key is absent rather than comparing its value. Use the existing test case format indicators and `got` map so the contract directly distinguishes missing keys from false values.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 `@shortcuts/doc/docs_fetch_v2_test.go`:
- Around line 760-824: Update TestDocsFetchXMLOutputContract to use the opaque
cN reference and the required discussion/message XML shape in its commentData
fixture. Change the assertions for reference_map.comments.c1.data to expect the
transformed contract and explicitly verify that the raw engine ID "1" is not
emitted.
In `@tests/cli_e2e/docs/docs_fetch_dryrun_test.go`:
- Around line 114-118: Update this error-path test to decode the CLI stderr
through errs.ProblemOf and assert the structured category/type, subtype, and
param fields for the unknown --comments flag, along with the preserved cause;
replace the substring-based stderr assertions while retaining the exit-code and
empty-stdout checks.
---
Outside diff comments:
In `@shortcuts/doc/docs_fetch_v2_test.go`:
- Around line 627-629: Update the assertions in the test covering the
`include_comments` request parameter: for XML cases, assert the key is present
and matches `tt.wantComments`; for Markdown cases, assert the `include_comments`
key is absent rather than comparing its value. Use the existing test case format
indicators and `got` map so the contract directly distinguishes missing keys
from false values.
🪄 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: ead2ca13-e305-4477-843f-34180fd7eced
📒 Files selected for processing (8)
shortcuts/doc/docs_fetch_v2.goshortcuts/doc/docs_fetch_v2_test.goskills/lark-doc/SKILL.mdskills/lark-doc/references/lark-doc-fetch.mdtests/cli_e2e/docs/coverage.mdtests/cli_e2e/docs/docs_fetch_comments_workflow_test.gotests/cli_e2e/docs/docs_fetch_dryrun_test.gotests/cli_e2e/docs/docs_update_dryrun_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- skills/lark-doc/references/lark-doc-fetch.md
- skills/lark-doc/SKILL.md
Comment on lines
+760
to
+824
| func TestDocsFetchXMLOutputContract(t *testing.T) { | ||
| const ( | ||
| content = `<p comment-refs="c1">body</p>` | ||
| commentData = `<comment id="1"><msg user="Reviewer">looks good</msg></comment>` | ||
| ) | ||
|
|
||
| for _, outputFormat := range []string{"json", "pretty"} { | ||
| t.Run(outputFormat, func(t *testing.T) { | ||
| t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) | ||
| docToken := "doxcnFetchComments" + outputFormat | ||
| f, stdout, _, reg := cmdutil.TestFactory(t, docsTestConfigWithAppID("docs-fetch-comments-"+outputFormat)) | ||
| stub := registerDocsAIStub(reg, "POST", "/open-apis/docs_ai/v1/documents/"+docToken+"/fetch", map[string]interface{}{ | ||
| "document": map[string]interface{}{ | ||
| "document_id": docToken, | ||
| "revision_id": float64(1), | ||
| "content": content, | ||
| "reference_map": map[string]interface{}{ | ||
| "comments": map[string]interface{}{ | ||
| "c1": map[string]interface{}{"data": commentData}, | ||
| }, | ||
| }, | ||
| }, | ||
| }) | ||
|
|
||
| err := mountAndRunDocs(t, DocsFetch, []string{ | ||
| "+fetch", | ||
| "--doc", docToken, | ||
| "--doc-format", "xml", | ||
| "--format", outputFormat, | ||
| "--as", "bot", | ||
| }, f, stdout) | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
|
|
||
| body := decodeRequestBody(t, stub.CapturedBody) | ||
| var extra map[string]bool | ||
| if err := json.Unmarshal([]byte(body["extra_param"].(string)), &extra); err != nil { | ||
| t.Fatalf("decode extra_param: %v", err) | ||
| } | ||
| if extra["include_comments"] != true { | ||
| t.Fatalf("request extra_param = %#v, want include_comments=true", extra) | ||
| } | ||
|
|
||
| if outputFormat == "pretty" { | ||
| if got := stdout.String(); got != content+"\n" { | ||
| t.Fatalf("pretty stdout = %q, want body only", got) | ||
| } | ||
| return | ||
| } | ||
|
|
||
| var envelope map[string]interface{} | ||
| if err := json.Unmarshal(stdout.Bytes(), &envelope); err != nil { | ||
| t.Fatalf("decode JSON output: %v\nraw=%s", err, stdout.String()) | ||
| } | ||
| data, _ := envelope["data"].(map[string]interface{}) | ||
| document, _ := data["document"].(map[string]interface{}) | ||
| if got := document["content"]; got != content { | ||
| t.Fatalf("document.content = %#v, want %q", got, content) | ||
| } | ||
| referenceMap, _ := document["reference_map"].(map[string]interface{}) | ||
| comments, _ := referenceMap["comments"].(map[string]interface{}) | ||
| comment, _ := comments["c1"].(map[string]interface{}) | ||
| if got := comment["data"]; got != commentData { | ||
| t.Fatalf("comments.c1.data = %#v, want %q", got, commentData) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not encode raw engine comment IDs in the XML output contract.
The fixture returns <comment id="1">, and the test requires that raw value in reference_map.comments.c1.data. This locks in the raw engine ID and legacy <comment>/<msg> shape.
Use the required opaque cN reference and <discussion>/<message> XML contract. Assert that raw engine comment IDs are not emitted.
🤖 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 760 - 824, Update
TestDocsFetchXMLOutputContract to use the opaque cN reference and the required
discussion/message XML shape in its commentData fixture. Change the assertions
for reference_map.comments.c1.data to expect the transformed contract and
explicitly verify that the raw engine ID "1" is not emitted.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/cli_e2e/docs/docs_fetch_comments_workflow_test.go`:
- Around line 189-205: Strengthen the “partial returns only intersecting local
comments” case by recording local reference keys and quotes from the full
response, selecting a distinct keyword tied to the intended comment, and
asserting the keyword response contains exactly the matching local keys while
excluding a known non-matching key. Keep the existing read-only and
whole-document assertions, but directly validate the filtered local comment set
rather than only requiring a positive count.
🪄 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: 7d3e5749-dfc6-43f3-a25b-5a83ed576323
📒 Files selected for processing (1)
tests/cli_e2e/docs/docs_fetch_comments_workflow_test.go
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
--commentsflag and make XML fetch request visible comments by defaultComment-v2 contract
For XML fetch, the CLI sends
extra_param.include_comments=truesilently. The SDK returns the document body plusreference_map.comments, using the compactcomment/quote/msg/img/cite/reactionprotocol. Local comments remain linked through body references; whole-document comments useis_whole=true. The service and SDK enforce visibility and limits; the CLI does not reinterpret comment payloads.Validation
c82053c1904a9e04dc7cdb7d71cd82ef2ac6bf9eboe_sun_ai_testbot full/partial/outline/Markdown/IM/pretty matrix: passed in7.34sboe_sun_ai_testuser equivalent matrix: passed in5.21s12.15s24.723s, exit 0, zero skipe2e-livesucceeded on attempt 3 after unrelated shared live-environment flakes in Base visibility and Slides cleanupThe tested service versions are ai_edit
1.0.0.643, SDKv0.0.64-0.20260809142315-58391682205e, and Enginev1.0.0.9562inboe_sun_ai_testonly.Release boundary
This PR is open and unmerged; no production deployment was performed. The four-repository engineering path is verified, but the broader Active Spec release gate remains blocked pending controlled multi-principal/cross-tenant fixtures, principal/RPC traces, Engine direct-consumer and mixed-version evidence, rollback/observation evidence, and required owner sign-offs. Those missing gates are not claimed by this CLI PR.
AI Review Context
docx-fetch-comments-protocol-v2revised-active-spec-v2.md5c30689361bc6f398f65c051881d870b2d1d7db8bf1fb508e294ed2e93db731dlark-cli docs +fetch comment-v2 request/output contract