feat(doc): add --show-diff flag to docs +update for post-update verification#622
Open
herbertliu wants to merge 5 commits intomainfrom
Open
feat(doc): add --show-diff flag to docs +update for post-update verification#622herbertliu wants to merge 5 commits intomainfrom
herbertliu wants to merge 5 commits intomainfrom
Conversation
…ication
docs +update currently returns only {success: true} on a successful MCP
call, giving the caller no way to verify the update actually landed as
intended — the canonical failure mode behind Case 9 in the pitfall list
(Agent sees success, doc looks unchanged, no signal which block was
really touched).
Add an opt-in --show-diff flag that snapshots the document via fetch-doc
before the update, performs the update as normal, snapshots again after,
and prints a git-style unified diff of the changed region to stderr.
append/overwrite are excluded: append has no meaningful 'before' to
compare against and overwrite trivially paints every line changed.
Design notes:
- Off by default because each use adds two fetch-doc calls. The flag is
intended for interactive verification and CI smoke tests, not hot
paths.
- Degrades gracefully: if either snapshot fails, the update still
proceeds and a warning is emitted. The update result (the MCP
response) is never replaced by diff content.
- Both snapshots are run through fixExportedMarkdown so the diff isn't
contaminated by fetch-doc artifacts already fixed on our side.
- computeMarkdownDiff uses a prefix/suffix-trim strategy rather than a
full Myers LCS. Replace/insert/delete modes touch a localized range,
so the surviving middle is already the user-visible change; a real
diff library would buy better pairing of add+delete lines at several
hundred lines of code we don't need for this purpose.
Coverage: 7-case table-driven test on computeMarkdownDiff plus a
hunk-header line-number assertion.
|
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:
📝 WalkthroughWalkthroughAdds a Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/CLI
participant DocsUpdate as DocsUpdate
participant MCP as MCP (fetch-doc / update-doc)
participant Diff as Diff Engine
participant Console as Output
User->>DocsUpdate: run with --show-diff
rect rgba(200,150,100,0.5)
DocsUpdate->>MCP: fetch-doc (pre-update)
MCP-->>DocsUpdate: markdown_before / error (warn)
end
rect rgba(150,200,150,0.5)
DocsUpdate->>MCP: update-doc (apply changes)
MCP-->>DocsUpdate: update result
end
rect rgba(150,150,200,0.5)
DocsUpdate->>MCP: fetch-doc (post-update)
MCP-->>DocsUpdate: markdown_after / error (warn)
DocsUpdate->>Diff: computeMarkdownDiff(markdown_before, markdown_after)
Diff-->>DocsUpdate: unified_diff (or empty)
DocsUpdate->>Console: print diff or advisory note
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #622 +/- ##
==========================================
+ Coverage 60.20% 60.66% +0.46%
==========================================
Files 407 416 +9
Lines 43340 43832 +492
==========================================
+ Hits 26091 26590 +499
+ Misses 15221 15182 -39
- Partials 2028 2060 +32 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@fcfa02ebc611d121e956bb5db6a2c1e5c8b4ed95🧩 Skill updatenpx skills add larksuite/cli#feat/docs-update-show-diff -y -g |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shortcuts/doc/docs_update_diff.go`:
- Around line 98-106: The current code calls common.CallMCPTool with "fetch-doc"
and immediately reads result["markdown"] in the docs update diff flow (result
variable from CallMCPTool and function in shortcuts/doc/docs_update_diff.go),
which can return a paginated snapshot with has_more; to fix, detect and handle
pagination or return an advisory error instead of silently using only the first
page: after CallMCPTool, check result["has_more"] (or equivalent flag) and if
true either loop/paginate to fetch and concatenate all pages before extracting
markdown, or return a clear error indicating partial snapshot and instruct the
caller to request a full snapshot before diffing; update the code paths that
extract result["markdown"] to only proceed when has_more is false or after full
pagination is complete.
- Around line 33-34: The current split uses strings.Split on before/after which
yields a single empty string for empty input and creates spurious blank-line
diffs; change the logic around the beforeLines and afterLines variables in
docs_update_diff.go to treat an empty before or after as zero lines (e.g., if
before == "" then beforeLines = nil/empty slice, else beforeLines =
strings.Split(before, "\n"), and do the same for after) so empty→content and
content→empty diffs don't emit a fake blank line.
In `@shortcuts/doc/docs_update.go`:
- Around line 124-172: Add an execution-level test that exercises the
--show-diff wiring around the update-doc flow: invoke the code path that sets
showDiff (the runtime.Bool check) and verify that fetchMarkdownForDiff is called
both pre- and post-update, that computeMarkdownDiff output is written to stderr
while normalizeDocsUpdateResult/CallMCPTool JSON stays on stdout, and that
append/overwrite modes skip pre/post fetch; also include cases where pre- or
post-fetch returns an error to assert the advisory warning messages (and the
update still succeeds) and a normalization/no-diff case where
computeMarkdownDiff yields empty output. Use the public helpers/functions named
showDiff, fetchMarkdownForDiff, computeMarkdownDiff, normalizeDocsUpdateResult
and common.CallMCPTool to locate and exercise the logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fb409021-e9be-498a-ae97-ce035b62fa19
📒 Files selected for processing (3)
shortcuts/doc/docs_update.goshortcuts/doc/docs_update_diff.goshortcuts/doc/docs_update_diff_test.go
Comment on lines
+124
to
+172
| // Optional diff capture: fetch the document before the update so we | ||
| // can render a unified diff after the update settles. Kept off the | ||
| // default path because it doubles read-side MCP calls for every | ||
| // update. append/overwrite don't get a diff — append has no "before" | ||
| // context worth showing, and overwrite marks every line as changed, | ||
| // defeating the purpose. | ||
| showDiff := runtime.Bool("show-diff") && | ||
| mode != "append" && mode != "overwrite" | ||
| var beforeMarkdown string | ||
| var beforeErr error | ||
| if showDiff { | ||
| beforeMarkdown, beforeErr = fetchMarkdownForDiff(runtime, runtime.Str("doc")) | ||
| if beforeErr != nil { | ||
| fmt.Fprintf(runtime.IO().ErrOut, | ||
| "warning: --show-diff pre-fetch failed (%v); update will proceed without a diff\n", | ||
| beforeErr) | ||
| } | ||
| } | ||
|
|
||
| result, err := common.CallMCPTool(runtime, "update-doc", args) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| normalizeDocsUpdateResult(result, runtime.Str("markdown")) | ||
|
|
||
| // Post-fetch and emit the diff. Any failure here is advisory only — | ||
| // the update already succeeded, so degrade gracefully instead of | ||
| // making the caller re-run. | ||
| if showDiff && beforeErr == nil { | ||
| afterMarkdown, afterErr := fetchMarkdownForDiff(runtime, runtime.Str("doc")) | ||
| switch { | ||
| case afterErr != nil: | ||
| fmt.Fprintf(runtime.IO().ErrOut, | ||
| "warning: --show-diff post-fetch failed (%v); update succeeded but no diff available\n", | ||
| afterErr) | ||
| default: | ||
| diff := computeMarkdownDiff( | ||
| fixExportedMarkdown(beforeMarkdown), | ||
| fixExportedMarkdown(afterMarkdown), | ||
| ) | ||
| if diff == "" { | ||
| fmt.Fprintln(runtime.IO().ErrOut, | ||
| "note: --show-diff found no textual change after the update (server may have normalized the markdown)") | ||
| } else { | ||
| fmt.Fprintf(runtime.IO().ErrOut, "--- before\n+++ after\n%s", diff) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Add an execution-level test for --show-diff wiring.
The helper tests cover diff formatting, but this behavior also depends on pre/post fetch-doc, skip handling for append/overwrite, advisory warning paths, and keeping JSON on stdout while diff output goes to stderr. As per coding guidelines, Every behavior change must have an accompanying test.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shortcuts/doc/docs_update.go` around lines 124 - 172, Add an execution-level
test that exercises the --show-diff wiring around the update-doc flow: invoke
the code path that sets showDiff (the runtime.Bool check) and verify that
fetchMarkdownForDiff is called both pre- and post-update, that
computeMarkdownDiff output is written to stderr while
normalizeDocsUpdateResult/CallMCPTool JSON stays on stdout, and that
append/overwrite modes skip pre/post fetch; also include cases where pre- or
post-fetch returns an error to assert the advisory warning messages (and the
update still succeeds) and a normalization/no-diff case where
computeMarkdownDiff yields empty output. Use the public helpers/functions named
showDiff, fetchMarkdownForDiff, computeMarkdownDiff, normalizeDocsUpdateResult
and common.CallMCPTool to locate and exercise the logic.
Collaborator
Author
There was a problem hiding this comment.
Thanks — noted. I'm scoping this PR to the helper-level test surface for now because an execution-level test for --show-diff needs MCP/update-doc mocking infrastructure that doesn't exist elsewhere in shortcuts/doc yet. Happy to add it as a follow-up if you feel strongly, or we can land a reusable test harness first.
There was a problem hiding this comment.
@herbertliu, that's a fair scoping call — the helper-level tests cover the core diff logic well, and building a --show-diff execution-level test properly does require a mock harness for CallMCPTool / fetchMarkdownForDiff that isn't in place yet. Landing that infrastructure first makes sense.
Would you like me to open a follow-up GitHub issue to track adding the execution-level test (pre/post-fetch wiring, append/overwrite skip, advisory warning paths, stdout/stderr separation)? That way it doesn't get lost between PRs.
🧠 Learnings used
Learnt from: herbertliu
Repo: larksuite/cli PR: 214
File: shortcuts/doc/markdown_fix.go:97-104
Timestamp: 2026-04-08T10:30:24.521Z
Learning: In `shortcuts/doc/markdown_fix.go` (Go, `doc` package), `fixBlockquoteHardBreaks` intentionally inserts a blank `>` between **any** two consecutive `> ` lines without filtering for list markers, ATX headings, or fences. This is safe because Lark's `fetch-doc` (`docs +fetch`) export currently produces blockquotes only as plain paragraph text — it never emits quoted lists, quoted headings, or quoted fences. If a future Lark API update produces more complex blockquote structures, the fix is to strip the `> ` prefix into a variable and skip insertion when the trimmed content matches a list item marker (e.g. `- `, `* `, `+ `, `1. `), an ATX heading (`#`), or a fence opening (` ``` `, `~~~`).
Learnt from: herbertliu
Repo: larksuite/cli PR: 214
File: shortcuts/doc/markdown_fix.go:69-78
Timestamp: 2026-04-08T10:31:12.360Z
Learning: In `shortcuts/doc/markdown_fix.go` (Go, `doc` package), `applyOutsideCodeFences` and the other two fence-scanning loops (around lines 263-280 and 343-352) only recognize bare triple-backtick fences and only close on an exact ` ``` ` line. This means blockquote-prefixed fences (`> ``` `) and longer fences (```` ```` ````) are not detected correctly. This is a **known, accepted limitation**: Lark's `fetch-doc` export currently emits only bare triple-backtick fences (no quoted fences, no 4+-tick fences), so no real document triggers the bug today. A follow-up PR should centralize fence detection into a `detectFence(line string) (isFence bool, prefix string, fenceChar rune, count int)` helper that handles optional blockquote prefixes and variable fence lengths, and reuse it in all three scanner sites.
Learnt from: chanthuang
Repo: larksuite/cli PR: 537
File: skills/lark-mail/references/lark-mail-send.md:0-0
Timestamp: 2026-04-21T03:46:55.148Z
Learning: In `larksuite/cli`, plain-text mode (`--plain-text`) supports large attachments as of commit 4859f7e in PR `#537`. Plain-text emails do not use a download card (HTML); instead, they append a text block with filename, size, and download link. Do not flag `--plain-text` + large attachments as incompatible in future reviews of `shortcuts/mail/large_attachment.go` or related documentation.
Learnt from: JayYoung2021
Repo: larksuite/cli PR: 410
File: cmd/auth/login_scope_cache.go:47-54
Timestamp: 2026-04-11T23:06:23.250Z
Learning: In larksuite/cli Go code, use `validate.SafeInputPath` only for user-supplied file path inputs (e.g., `--file` flags). Do NOT require `validate.SafeInputPath` for internally constructed, application-managed filesystem paths when: (1) their root directory is already validated via `validate.SafeEnvDirPath()` inside helpers like `appdir.CacheDir()`, `appdir.StateDir()`, or `appdir.LogDir()`, and (2) the filename/path components are sanitized by helpers such as `sanitizeLoginScopeCacheKey()`. In code reviews, missing `SafeInputPath` calls should not be flagged for these app-managed paths if the above safeguards are already in place.
Learnt from: JayYoung2021
Repo: larksuite/cli PR: 410
File: internal/update/update.go:147-147
Timestamp: 2026-04-11T23:06:28.717Z
Learning: In this repo’s Go code (larksuite/cli), treat `validate.SafeInputPath` as a guard for **user-supplied local file paths only**. When you build a path by joining a previously validated base directory (e.g., from `appdir.StateDir()`, `appdir.CacheDir()`, `appdir.ConfigDir()` which already validate env overrides via `validate.SafeEnvDirPath()`) with a compile-time constant filename (e.g., `"update-state.json"`, `"config.json"`), do **not** require an additional `validate.SafeInputPath` call before using `vfs.*` or `validate.AtomicWrite`. Reviewers should avoid flagging/adding redundant `validate.SafeInputPath` in these computed-path cases; only require it when the path ultimately comes from user input.
Learnt from: ViperCai
Repo: larksuite/cli PR: 450
File: shortcuts/slides/slides_create.go:50-60
Timestamp: 2026-04-14T03:13:09.459Z
Learning: When reviewing this Go codebase, do not require explicit `validate.SafeInputPath` / `validate.SafeOutputPath` calls for user-supplied paths if the code uses the `runtime.FileIO()` abstraction methods (`Stat`, `Open`, `Save`, `ResolvePath`). `runtime.FileIO()` is backed by `LocalFileIO` (in `localfileio.go`), which enforces `SafeInputPath`/`SafeOutputPath` before any filesystem access and reports violations as `fileio.PathValidationError`. Only require explicit `validate.SafeInputPath` when bypassing `runtime.FileIO()` and calling `vfs.*` or `os.*` directly with user-influenced paths.
Learnt from: ViperCai
Repo: larksuite/cli PR: 450
File: shortcuts/slides/slides_media_upload.go:44-49
Timestamp: 2026-04-14T03:13:09.234Z
Learning: In larksuite/cli (Go), treat calls that validate user-supplied `--file` paths via `runtime.FileIO().Stat(path)` as already enforcing path-safety. `runtime.FileIO().Stat()` routes to `LocalFileIO.Stat()` (internal/vfs/localfileio/localfileio.go), which calls `SafeInputPath(name)` and wraps failures as `fileio.PathValidationError`. Therefore, when reviewing `shortcuts` code, do NOT flag missing explicit `validate.SafeInputPath` calls for `--file` flags if the code calls `runtime.FileIO().Stat()` on that same `--file` path during its Validate or Execute path (i.e., the safety is already enforced by the `Stat` call path).
Learnt from: herbertliu
Repo: larksuite/cli PR: 508
File: shortcuts/doc/clipboard.go:29-58
Timestamp: 2026-04-16T09:26:48.292Z
Learning: In Go CLI code, it is acceptable (and intentional) to use the standard library `os.*` APIs (e.g., `os.CreateTemp`, `os.Stat`, `os.Remove`) for **CLI-internal ephemeral temp files**—files created by the CLI itself (not provided by the user), used only within a single call frame, and cleaned up via an explicit deferred cleanup (e.g., a `cleanup()` closure). Reserve `vfs.*`/`fileio` abstractions for **user-facing input/output paths** such as user-supplied flags (e.g., `--file`) or atomic writes to user-specified destinations. Do not flag `os.*` usage when it is strictly for these caller-managed internal temp files; routing such internal temp paths through `fileio` may fail path validation by design (e.g., temp paths outside the working directory), even though the usage is correct.
…or --show-diff
CodeRabbit flagged two edges in the initial Case 9 implementation:
1. strings.Split("", "\n") returns [""], not []string{}, so an empty→content
or content→empty update would emit a phantom '-\n' or '+\n' as the
counterpart. Add splitDiffLines() that maps "" to nil and route the two
call sites through it. Two regression tests pin the fix.
2. fetch-doc can return has_more=true (large docs); diffing the first page
alone produced misleading output where real edits outside the fetched
range would look like 'no textual change'. fetchMarkdownForDiff now
returns an advisory error when has_more is true. The --show-diff flow
treats this as a snapshot failure (existing fallback path prints a
warning and skips the diff), so the update itself is unaffected.
…decov 60% patch gate Prior tests covered computeMarkdownDiff / splitDiffLines only, leaving the Execute branch wiring pre/post fetch-doc + stderr rendering unexercised — codecov/patch reported 51.8% on #622 (target 60%). Add mount-and-run tests that cover the four documented branches of --show-diff: - TestDocsUpdateShowDiffEmitsUnifiedDiff — full happy path: pre-fetch returns original markdown, update-doc succeeds, post-fetch returns modified markdown; asserts the '--- before / +++ after' block plus +/- lines land on stderr while the update JSON stays on stdout. - TestDocsUpdateShowDiffSkipsForAppendMode — append mode must never trigger pre/post fetches; the single registered /mcp stub handles only the update-doc call, so any extra fetch would trip 'no stub' and fail the test. - TestDocsUpdateShowDiffPreFetchFailureDegradesGracefully — pre-fetch returns has_more=true (the new Case 9 advisory-failure path); the update must still complete and the warning must surface on stderr. - TestDocsUpdateShowDiffIdenticalSnapshotsNote — pre and post fetch return the same markdown; shortcut emits the 'no textual change' note instead of a diff block. Uses the existing httpmock registry pattern: one stub per expected /mcp call, helper registerMCPStub keeps the noise down. Statement coverage after change: - docs_update.go: ~0% → 82.4% (new Execute show-diff block) - docs_update_diff.go: holds at 95.5% Both well above the 60% codecov/patch threshold.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
shortcuts/doc/docs_update_test.go (2)
364-377: Silently droppedjson.Marshalerror.
raw, _ := json.Marshal(payload)swallows any marshal error — if a test passes a non-marshalable value, the stub would return"null"or the zero value and the failure would surface as a confusing assertion mismatch rather than a clear test setup error. Since this is test-only scaffolding, failing fast witht.Fatalfis cheap insurance:♻️ Proposed refactor
-func registerMCPStub(reg *httpmock.Registry, payload map[string]interface{}) { - raw, _ := json.Marshal(payload) +func registerMCPStub(t *testing.T, reg *httpmock.Registry, payload map[string]interface{}) { + t.Helper() + raw, err := json.Marshal(payload) + if err != nil { + t.Fatalf("marshal stub payload: %v", err) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/doc/docs_update_test.go` around lines 364 - 377, The helper registerMCPStub currently ignores json.Marshal errors; change its signature to accept a *testing.T (e.g., registerMCPStub(t *testing.T, reg *httpmock.Registry, payload map[string]interface{})) and check the error returned by json.Marshal, calling t.Fatalf with the error if non-nil; update all call sites to pass the test object so tests fail fast on marshal problems instead of producing confusing "null" payloads.
477-497: Consider also asserting stdout success in identical-snapshots test.Unlike the other three show-diff tests, this one omits the
stdout.String()"success" check. Adding it would guard against regressions where the "no textual change" branch accidentally swallows the MCP JSON output.♻️ Proposed addition
if !strings.Contains(stderr.String(), "no textual change") { t.Errorf("expected no-change note on stderr, got:\n%s", stderr.String()) } + if !strings.Contains(stdout.String(), "success") { + t.Errorf("expected update success on stdout, got:\n%s", stdout.String()) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/doc/docs_update_test.go` around lines 477 - 497, Add an assertion in TestDocsUpdateShowDiffIdenticalSnapshotsNote to verify stdout still contains the expected success/MCP JSON output (e.g., check stdout.String() contains "success" or the same JSON used in other show-diff tests) so that runDocsUpdateShortcut and the MCP stub response aren't accidentally suppressed by the "no textual change" branch; update the test near the existing stderr assertion and use the same stdout.String() check pattern as in the other show-diff tests to locate the correct expectation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shortcuts/doc/docs_update_test.go`:
- Around line 443-451: The top-of-function docstring for
TestDocsUpdateShowDiffPreFetchFailureDegradesGracefully is inaccurate: the test
does not omit the pre-fetch stub but instead registers a stub (via
registerMCPStub) returning {"markdown":"x","has_more":true} to trigger the
paginated-snapshot advisory path in fetchMarkdownForDiff; update the
comment/paragraph above the test (lines around
TestDocsUpdateShowDiffPreFetchFailureDegradesGracefully and the brief
description) to state that a pre-fetch stub returning has_more=true is provided
to simulate the paginated-snapshot warning and that the update must still
proceed with the warning emitted on stderr, keeping any inline comment about why
the extra stub is needed.
---
Nitpick comments:
In `@shortcuts/doc/docs_update_test.go`:
- Around line 364-377: The helper registerMCPStub currently ignores json.Marshal
errors; change its signature to accept a *testing.T (e.g., registerMCPStub(t
*testing.T, reg *httpmock.Registry, payload map[string]interface{})) and check
the error returned by json.Marshal, calling t.Fatalf with the error if non-nil;
update all call sites to pass the test object so tests fail fast on marshal
problems instead of producing confusing "null" payloads.
- Around line 477-497: Add an assertion in
TestDocsUpdateShowDiffIdenticalSnapshotsNote to verify stdout still contains the
expected success/MCP JSON output (e.g., check stdout.String() contains "success"
or the same JSON used in other show-diff tests) so that runDocsUpdateShortcut
and the MCP stub response aren't accidentally suppressed by the "no textual
change" branch; update the test near the existing stderr assertion and use the
same stdout.String() check pattern as in the other show-diff tests to locate the
correct expectation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 50961655-bcca-41f8-8dd6-f677f55d2a1a
📒 Files selected for processing (1)
shortcuts/doc/docs_update_test.go
gocritic badCond flagged the 'prefix == beforeEnd && prefix == afterEnd' guard as suspicious because the earlier 'if before == after' return already covers that state. The check never fires in practice — when both ends collapse to the prefix, the inputs are byte-identical and we've already returned. Drop the unreachable branch; tests still pass.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
shortcuts/doc/docs_update_diff.go (1)
117-121:⚠️ Potential issue | 🟡 MinorFail advisory snapshots when
markdownis missing or malformed.Line 120 silently converts a missing/non-string
markdownfield to"", which can produce a misleading full deletion/addition diff instead of letting the caller print the intended warning and skip the diff.🛡️ Proposed guard
if hasMore, ok := result["has_more"].(bool); ok && hasMore { return "", fmt.Errorf("fetch-doc returned a paginated snapshot (has_more=true); --show-diff cannot diff partial documents") } - md, _ := result["markdown"].(string) + md, ok := result["markdown"].(string) + if !ok { + return "", fmt.Errorf("fetch-doc response missing markdown") + } return md, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/doc/docs_update_diff.go` around lines 117 - 121, The code silently treats a missing/non-string result["markdown"] as an empty string (md := result["markdown"].(string) with ignore of the ok value), which can produce misleading diffs; change the logic around the markdown extraction to verify the type assertion (use md, ok := result["markdown"].(string)) and if !ok or md=="" return an explicit error (e.g., fmt.Errorf("fetch-doc returned missing or malformed markdown snapshot")) so the caller can detect and skip the diff instead of diffing an empty document.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shortcuts/doc/docs_update_diff.go`:
- Around line 88-96: splitDiffLines currently uses strings.Split(s, "\n") which
leaves a trailing empty element for inputs that end with "\n" (e.g., "a\n" ->
["a",""]) and thus produces a phantom blank context line; update splitDiffLines
to remove a final empty element when present (but still return nil for the
totally empty string) — e.g., call strings.Split(s, "\n"), then if len(parts)>0
and parts[len(parts)-1]=="" drop that last element before returning; keep the
function name splitDiffLines and the strings.Split usage so the change is
localized.
- Around line 69-72: The unified diff hunk header uses ctxStart+1
unconditionally, which yields @@ -1,0 ... @@ for a zero-length range at file
start; change the first printed start to output 0 when ctxStart == 0 and
ctxStart+1 otherwise (keep the existing counts ctxEndBefore-ctxStart and
ctxEndAfter-ctxStart), i.e. update the fmt.Fprintf call that prints the hunk
header to conditionally use 0 for the start number based on ctxStart while
leaving the rest unchanged (refer to variables ctxStart, ctxEndBefore,
ctxEndAfter and the fmt.Fprintf call).
---
Duplicate comments:
In `@shortcuts/doc/docs_update_diff.go`:
- Around line 117-121: The code silently treats a missing/non-string
result["markdown"] as an empty string (md := result["markdown"].(string) with
ignore of the ok value), which can produce misleading diffs; change the logic
around the markdown extraction to verify the type assertion (use md, ok :=
result["markdown"].(string)) and if !ok or md=="" return an explicit error
(e.g., fmt.Errorf("fetch-doc returned missing or malformed markdown snapshot"))
so the caller can detect and skip the diff instead of diffing an empty document.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d771cd8d-b9ae-43d8-bf38-b39a0599207c
📒 Files selected for processing (1)
shortcuts/doc/docs_update_diff.go
… trailing newline Three CodeRabbit findings on the show-diff helpers: 1. computeMarkdownDiff emitted '@@ -1,0 +1,N @@' for a pure insertion into an empty 'before'. The unified-diff convention is that a zero-length range uses start=0 (both 'git diff' and GNU 'diff -u' do this). Switch the header to 0 when the range is empty. Test: TestComputeMarkdownDiffHunkHeaderZeroLengthRange covers both empty-before and empty-after cases. 2. splitDiffLines was returning ['a', ''] for 'a\n', causing every fetch-doc snapshot (which always ends with '\n') to carry a phantom trailing blank context line and inflate hunk counts. Drop a single trailing '' terminator while preserving the empty→nil special case and preserving genuinely blank trailing lines. Tests: TestSplitDiffLinesStripsTrailingNewline exercises 8 inputs; TestComputeMarkdownDiffTrailingNewlineParity asserts the diff of snapshots ending with '\n' equals the diff of snapshots without it. 3. Docstring on TestDocsUpdateShowDiffPreFetchFailureDegradesGracefully said the pre-fetch stub was omitted; the code actually registers a has_more=true stub to exercise the paginated-snapshot advisory path. Rewrite the docstring to describe what the test actually does.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shortcuts/doc/docs_update_diff_test.go`:
- Around line 175-186: Add a test case asserting that computeMarkdownDiff treats
a string and the same string with only a trailing newline as no change: call
computeMarkdownDiff("a", "a\n") (and symmetric ordering) and assert it returns
the empty/no-textual-change result used elsewhere in the test (same behavior as
when splitting/normalizing lines). Update
TestComputeMarkdownDiffTrailingNewlineParity to include these assertions so
computeMarkdownDiff's raw-equality short-circuit in
shortcuts/doc/docs_update_diff.go no longer emits a context-only hunk for
trailing-newline-only differences.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 97e5c367-22af-48c2-960f-1c731b87440b
📒 Files selected for processing (3)
shortcuts/doc/docs_update_diff.goshortcuts/doc/docs_update_diff_test.goshortcuts/doc/docs_update_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- shortcuts/doc/docs_update_test.go
- shortcuts/doc/docs_update_diff.go
Comment on lines
+175
to
+186
| // TestComputeMarkdownDiffTrailingNewlineParity confirms a single-line | ||
| // replacement against snapshots ending in '\n' (what fetch-doc returns) | ||
| // produces the same diff as snapshots without the trailing newline. | ||
| // Previously the trailing "" generated a phantom blank context line. | ||
| func TestComputeMarkdownDiffTrailingNewlineParity(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| withNL := computeMarkdownDiff("a\nold\nb\n", "a\nnew\nb\n") | ||
| withoutNL := computeMarkdownDiff("a\nold\nb", "a\nnew\nb") | ||
| if withNL != withoutNL { | ||
| t.Fatalf("diff should not depend on trailing newline\nwith:\n%s\n\nwithout:\n%s", withNL, withoutNL) | ||
| } |
There was a problem hiding this comment.
Cover trailing-newline-only no-op parity.
shortcuts/doc/docs_update_diff.go:29-100 currently checks raw string equality before splitDiffLines; computeMarkdownDiff("a", "a\n") would still emit a context-only hunk even though the normalized diff lines are equal. Add this assertion so the implementation returns “no textual change” for that case too.
Suggested test addition
func TestComputeMarkdownDiffTrailingNewlineParity(t *testing.T) {
t.Parallel()
+ if got := computeMarkdownDiff("a", "a\n"); got != "" {
+ t.Fatalf("trailing-newline-only difference should be ignored, got:\n%s", got)
+ }
+ if got := computeMarkdownDiff("a\n", "a"); got != "" {
+ t.Fatalf("trailing-newline-only difference should be ignored, got:\n%s", got)
+ }
+
withNL := computeMarkdownDiff("a\nold\nb\n", "a\nnew\nb\n")
withoutNL := computeMarkdownDiff("a\nold\nb", "a\nnew\nb")
if withNL != withoutNL {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // TestComputeMarkdownDiffTrailingNewlineParity confirms a single-line | |
| // replacement against snapshots ending in '\n' (what fetch-doc returns) | |
| // produces the same diff as snapshots without the trailing newline. | |
| // Previously the trailing "" generated a phantom blank context line. | |
| func TestComputeMarkdownDiffTrailingNewlineParity(t *testing.T) { | |
| t.Parallel() | |
| withNL := computeMarkdownDiff("a\nold\nb\n", "a\nnew\nb\n") | |
| withoutNL := computeMarkdownDiff("a\nold\nb", "a\nnew\nb") | |
| if withNL != withoutNL { | |
| t.Fatalf("diff should not depend on trailing newline\nwith:\n%s\n\nwithout:\n%s", withNL, withoutNL) | |
| } | |
| // TestComputeMarkdownDiffTrailingNewlineParity confirms a single-line | |
| // replacement against snapshots ending in '\n' (what fetch-doc returns) | |
| // produces the same diff as snapshots without the trailing newline. | |
| // Previously the trailing "" generated a phantom blank context line. | |
| func TestComputeMarkdownDiffTrailingNewlineParity(t *testing.T) { | |
| t.Parallel() | |
| if got := computeMarkdownDiff("a", "a\n"); got != "" { | |
| t.Fatalf("trailing-newline-only difference should be ignored, got:\n%s", got) | |
| } | |
| if got := computeMarkdownDiff("a\n", "a"); got != "" { | |
| t.Fatalf("trailing-newline-only difference should be ignored, got:\n%s", got) | |
| } | |
| withNL := computeMarkdownDiff("a\nold\nb\n", "a\nnew\nb\n") | |
| withoutNL := computeMarkdownDiff("a\nold\nb", "a\nnew\nb") | |
| if withNL != withoutNL { | |
| t.Fatalf("diff should not depend on trailing newline\nwith:\n%s\n\nwithout:\n%s", withNL, withoutNL) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shortcuts/doc/docs_update_diff_test.go` around lines 175 - 186, Add a test
case asserting that computeMarkdownDiff treats a string and the same string with
only a trailing newline as no change: call computeMarkdownDiff("a", "a\n") (and
symmetric ordering) and assert it returns the empty/no-textual-change result
used elsewhere in the test (same behavior as when splitting/normalizing lines).
Update TestComputeMarkdownDiffTrailingNewlineParity to include these assertions
so computeMarkdownDiff's raw-equality short-circuit in
shortcuts/doc/docs_update_diff.go no longer emits a context-only hunk for
trailing-newline-only differences.
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
Addresses Case 9 in the lark-cli pitfall list:
docs +updatecurrently returns{success: true}on every successful MCP call, so there's no way for the caller to verify the update actually landed where expected. Agents see "success", the doc looks unchanged, and nothing in the response tells them which block was really touched.Add an opt-in
--show-diffflag that captures a before/after snapshot viafetch-docand prints a git-style unified diff of the changed region to stderr.Changes
shortcuts/doc/docs_update.go: new--show-diffbool flag; wire pre-fetch + post-fetch around the existingupdate-doccall, degrade gracefully on snapshot failure, skip forappend/overwritemodes.shortcuts/doc/docs_update_diff.go(new):computeMarkdownDiff(before, after)— prefix/suffix-trim unified diff with 2 lines of context;fetchMarkdownForDiffhelper wrapping the MCP call.shortcuts/doc/docs_update_diff_test.go(new): 7-case table-driven test + hunk-header line-number assertion.Scope notes
fetch-doccalls, so this is an opt-in verification tool, not a default.append/overwriteare excluded — the former has nothing to compare against, the latter trivially marks every line as changed.fixExportedMarkdownso the diff isn't polluted by already-fixedfetch-docartifacts.Test Plan
go test ./shortcuts/doc/...passesgo vet ./...cleangofmt -lcleandocs +update --mode=replace_range --show-diffagainst a test doc and confirm the diff hunk renders as expectedExample output
Summary by CodeRabbit
New Features
Bug Fixes
Tests