feat(docs): default XML fetch to visible comments - #2247
Open
SunPeiYang996 wants to merge 10 commits into
Open
Conversation
📝 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@0082a3453985685b2dae516c5dc039bd3895ba40🧩 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.
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, comment-specific scope preflight, and bot/JSON-only validationextra_param.include_comments=trueby default for XML fetches under both user and bot identities--commentsfailure as a typedvalidation/invalid_argumentenvelope with structured parameter metadataCompatibility and release gate
This PR is a code candidate for the comment-v2 CLI contract. It remains open and unmerged.
boe_sun_ai_test.BOE-E2Ematrix in the active spec must be executed bybe_integration; this CLI repository's gated live workflow is only a subset of that matrix.Implementation status:
code_ready_pending_boe_live_e2e.The CLI live workflow is intentionally scoped to user/bot identity, full/partial comment filtering, JSON sidecar preservation, pretty body-only output, and Markdown/IM Markdown boundaries. Full BOE fixtures and cross-service coverage—including authorization, no-document behavior, cross-tenant denial, mixed-version checks, and resource nodes—belong to the
be_integrationgate and are not claimed as passed here.The CLI workflow subset can be invoked on an authorized credentialed remote environment with:
Required environment key names (values intentionally omitted):
LARK_DOCS_FETCH_COMMENTS_E2ETEST_BOT1_APP_ID+TEST_TENANT_ACCESS_TOKEN, orLARKSUITE_CLI_APP_ID+LARKSUITE_CLI_TENANT_ACCESS_TOKENTEST_BOT1_APP_ID+TEST_USER_ACCESS_TOKEN, orLARKSUITE_CLI_APP_ID+LARKSUITE_CLI_USER_ACCESS_TOKEN; a verified locallark-cli authuser session is also supportedValidation
0082a3453985685b2dae516c5dc039bd3895ba40make build && go test ./shortcuts/doc ./tests/cli_e2e/docs/... -count=1 && go vet ./tests/cli_e2e/docs/...: passedmake unit-testbefore the final test-assertion-only commit: passedgo vet ./...before the final test-assertion-only commit: passedbe_integrationAll Go build and test commands were executed on the configured remote development host.
AI Review Context
workflow artifact be_spec_review/revised-active-spec-v2.mddocx-fetch-comments-protocol-v25c30689361bc6f398f65c051881d870b2d1d7db8bf1fb508e294ed2e93db731dBE-1lark-cli docs +fetch comment-v2 request/output contractN/A (no IDL change)lark-security-knowledge:INDEX.mdRL-06, RULE-DS-08, RULE-DC-01, RULE-IP-01, RULE-IP-02, RULE-IP-06, RULE-IP-07, RULE-AD-01, RULE-AD-03, RULE-CS-02Default-on XML returns only current-principal-authorized L4 comment data; the CLI does not choose the principal, Markdown/IM remain unchanged, and BOE principal/permission/log-leak gates stay mandatory before delivery or release.passSummary by CodeRabbit
--commentsoption has been removed; use the documented fetch behavior instead.