feat(doc): add docs +batch-update for multi-operation sequential updates#623
feat(doc): add docs +batch-update for multi-operation sequential updates#623herbertliu wants to merge 3 commits intomainfrom
Conversation
github-actions
Bot
added
domain/ccm
📝 WalkthroughWalkthroughAdds a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as "DocsBatchUpdate\n(shortcut)"
participant MCP as "MCP /update-doc"
participant Store as "Document Store"
User->>CLI: run docs +batch-update --doc <id> --operations <JSON> --on-error=<mode>
CLI->>CLI: parse & validate operations array (trim BOM, validate modes/selection)
alt DryRun
CLI->>User: emit planned per-op MCP calls and op count
else Execute
loop for each operation
CLI->>MCP: call /update-doc with built args
MCP->>Store: apply update
Store-->>MCP: response (success/error + payload)
MCP-->>CLI: normalized result
CLI->>User: (optionally) advisory warnings per op
end
CLI-->>User: aggregated envelope {doc,total,applied,results,stopped_early}
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #623 +/- ##
==========================================
+ Coverage 63.20% 64.65% +1.45%
==========================================
Files 491 517 +26
Lines 42237 45871 +3634
==========================================
+ Hits 26694 29659 +2965
- Misses 13211 13621 +410
- Partials 2332 2591 +259 ☔ 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@b39d5783a32f711f2c76fd5829df582c0dfc00da🧩 Skill updatenpx skills add larksuite/cli#feat/docs-batch-update -y -g |
![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_batch_update.go (2)
185-201: Prefix check rejects valid JSON arrays with UTF‑8 BOM / leading whitespace inside the payload.
strings.HasPrefix(trimmed, "[")runs afterTrimSpace, but if the input has a UTF‑8 BOM (\uFEFF) at the start — common when users--operations@file.json`` with an editor-saved file —TrimSpacedoes not strip the BOM and the check reports "must be a JSON array" even though `json.Unmarshal` would accept the body. This is a minor UX rough edge given the `File` input mode on line 61.Either strip a leading BOM before the prefix check, or detect the array-vs-object case by inspecting the first non-space, non-BOM rune. Not a blocker.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/doc/docs_batch_update.go` around lines 185 - 201, The prefix check in parseBatchUpdateOps wrongly rejects valid JSON when the input begins with a UTF‑8 BOM; before testing strings.HasPrefix(trimmed, "["), strip any leading BOM (U+FEFF) or determine the first non-space, non‑BOM rune and use that for the array-vs-object check so valid JSON files pass; adjust the logic that prepares trimmed (and the subsequent json.Unmarshal fallback) to remove a leading BOM character if present so that ops parsing and the existing error paths remain intact.
104-146:Executere-parses--operationsbut skips per-op validation.
ValidaterunsparseBatchUpdateOps+validateBatchUpdateOpfor every op, andExecutere-parses here (line 105) but does not re-runvalidateBatchUpdateOp. For the normal CLI path this is fine becauseValidategatesExecute, but:
- It's a latent footgun for any future caller that invokes
Executewithout going throughValidate(tests, programmatic reuse, or a refactor that rewires the pipeline) — malformed ops would reach MCP unchecked.- You already pay the JSON unmarshal cost three times (
Validate,DryRun,Execute).Consider parsing+validating once and stashing the result on the runtime, or at minimum re-running
validateBatchUpdateOpin theExecuteloop for defense-in-depth.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/doc/docs_batch_update.go` around lines 104 - 146, Execute currently re-parses --operations via parseBatchUpdateOps but skips per-op validation (validateBatchUpdateOp), risking malformed ops if Execute is called without Validate and wasting repeated unmarshalling; fix by parsing once and storing the parsed/validated ops on the runtime context (e.g., attach to runtime with a stable key) in Validate/DryRun and have Execute read that cached slice, or if you prefer a minimal change, call validateBatchUpdateOp for each op inside Execute's loop before using it (referencing Execute, parseBatchUpdateOps, validateBatchUpdateOp, Validate and DryRun to locate the related code).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@shortcuts/doc/docs_batch_update.go`:
- Around line 185-201: The prefix check in parseBatchUpdateOps wrongly rejects
valid JSON when the input begins with a UTF‑8 BOM; before testing
strings.HasPrefix(trimmed, "["), strip any leading BOM (U+FEFF) or determine the
first non-space, non‑BOM rune and use that for the array-vs-object check so
valid JSON files pass; adjust the logic that prepares trimmed (and the
subsequent json.Unmarshal fallback) to remove a leading BOM character if present
so that ops parsing and the existing error paths remain intact.
- Around line 104-146: Execute currently re-parses --operations via
parseBatchUpdateOps but skips per-op validation (validateBatchUpdateOp), risking
malformed ops if Execute is called without Validate and wasting repeated
unmarshalling; fix by parsing once and storing the parsed/validated ops on the
runtime context (e.g., attach to runtime with a stable key) in Validate/DryRun
and have Execute read that cached slice, or if you prefer a minimal change, call
validateBatchUpdateOp for each op inside Execute's loop before using it
(referencing Execute, parseBatchUpdateOps, validateBatchUpdateOp, Validate and
DryRun to locate the related code).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6d7433de-4443-48b2-8ad6-cba6785fc7a3
📒 Files selected for processing (3)
shortcuts/doc/docs_batch_update.goshortcuts/doc/docs_batch_update_test.goshortcuts/doc/shortcuts.go
…isfy codecov 60% patch gate Prior unit tests covered parse / validate / build-args only, leaving DryRun and Execute at 0% which dropped the codecov/patch gate on #623 to 35.5%. Add mount-and-run tests that exercise the remaining paths: - TestDocsBatchUpdateDryRun — 3-op dry-run: asserts op_count, per-op step desc ("[1/3] replace_range" etc.) and overall batch description - TestDocsBatchUpdateValidateRejectsMalformedOp — validates the per-op Validate loop rejects a replace_range op missing its selection before any MCP call - TestDocsBatchUpdateValidateRejectsInvalidOnError — rejects a bogus --on-error enum value up front - TestDocsBatchUpdateExecuteAllSuccess — mocks two /mcp stubs and asserts stdout envelope: total=2, applied=2, stopped_early=false, per-op success=true - TestDocsBatchUpdateStopsOnFirstFailure — only one MCP stub for a 2-op batch, so the second call trips 'no stub' in httpmock; asserts applied=1, stopped_early=true, and the partial-result envelope still gets emitted before the error returns Introduces registerUpdateDocMCPStubs(count, payload) helper because httpmock stubs match exactly once, so batch paths need one stub per expected call. Coverage on docs_batch_update.go rises from ~35% to ~76%, above the codecov/patch 60% threshold.
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_batch_update_test.go`:
- Around line 394-441: Add a new test (e.g.,
TestDocsBatchUpdateContinuesOnError) alongside
TestDocsBatchUpdateStopsOnFirstFailure that uses registerUpdateDocMCPStubs and
runBatchUpdateShortcut to exercise the new --on-error=continue behavior: arrange
stubs so one operation fails and a later one succeeds, invoke
runBatchUpdateShortcut with the "--on-error=continue" flag, assert the command
does not abort remaining ops (check err is nil or expected non-fatal behavior),
unmarshal stdout into the same envelope shape used in
TestDocsBatchUpdateStopsOnFirstFailure and assert Data.Total equals number of
ops, Data.Applied equals number of successful ops, Data.StoppedEarly is false,
and additionally inspect the per-operation result entries in the output to
verify the failed op contains an error and the later op shows success; reuse
helpers registerUpdateDocMCPStubs and runBatchUpdateShortcut to set up stubs and
run the shortcut.
🪄 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: 23d82d57-4524-43f3-bd0b-1c3d90190e78
📒 Files selected for processing (1)
shortcuts/doc/docs_batch_update_test.go
fangshuyu-768
left a comment
fangshuyu-768
left a comment
There was a problem hiding this comment.
Some design questions about partial-failure recovery for sequential batches; nothing blocking, but worth thinking through before this is the recommended path.
1. Two error signals on --on-error=stop. When op N fails, Execute calls runtime.Out() to emit the partial result, then returns callErr. Callers see both a JSON result (with stopped_early=true) and a non-zero exit code. Single-update doesn't have this ambiguity because it fails fast. Worth deciding whether one of the two signals is sufficient.
2. Result envelope lacks a per-op block-id map. The PR says "pair with docs +fetch after a partial run to recover manually", but reconciling a 10-op batch mid-failure without knowing which blocks each op touched is hard. Including affected block ids per op would make recovery tractable.
3. Validation can't catch in-batch staleness. The Validate hook runs upfront, but it can't guard against later ops referencing block-ids that earlier ops invalidated (e.g., op[0] deletes a section that op[1]'s selection-by-title resolves into). For sequential semantics this may be acceptable, but it's worth calling out in the docs alongside the partial-failure note.
fangshuyu-768
left a comment
fangshuyu-768
left a comment
There was a problem hiding this comment.
Three design notes inline; expands on the earlier summary review.
| fmt.Fprintf(runtime.IO().ErrOut, | ||
| "error: --operations[%d] failed; stopping (--on-error=stop); %d/%d applied before the failure\n", | ||
| i, successCount, len(ops)) | ||
| runtime.Out(map[string]interface{}{ |
There was a problem hiding this comment.
Two error signals on --on-error=stop: runtime.Out(...) emits the partial result with stopped_early=true AND the function returns callErr, so callers see both a JSON success-shaped response and a non-zero exit code. Single-update doesn't have this ambiguity because it fails fast. Worth deciding which signal is canonical — scripts that key on exit code will see the JSON as residual stderr/stdout, and agents that key on JSON shape may not check exit code.
There was a problem hiding this comment.
Documented as a deliberate design decision in 3d0302e — added a 'Two design notes' block to the Go doc on DocsBatchUpdate:
On --on-error=stop the shortcut emits TWO failure signals: the stdout JSON envelope (with stopped_early=true and the partial result list), AND a non-zero exit via the returned error. They are complementary: scripts that key on exit code see 'the batch did not complete cleanly'; callers parsing JSON see 'exactly which ops succeeded and how far we got'. Don't treat them as redundant.
Rationale: collapsing to one signal would cost callers in the other camp — exit-code-only loses the per-op detail; JSON-only would force every shell script to JQ-parse to know whether to abort. Keeping both feels right for a CLI surface.
| successCount++ | ||
| } | ||
|
|
||
| runtime.Out(map[string]interface{}{ |
There was a problem hiding this comment.
The result envelope (total / applied / stopped_early) doesn't include a per-op block-id map. The PR description says 'pair with docs +fetch after a partial run to recover manually,' but reconciling a 10-op batch mid-failure without knowing which blocks each op touched is hard. Including affected block ids per op (e.g., new applied: [{op_index, block_ids: [...]}, ...]) would make recovery tractable for agents.
There was a problem hiding this comment.
Acknowledged but deferring to a follow-up. The MCP update-doc v1 response doesn't carry block-id information for affected blocks (just {success: true} typically), so producing a per-op block-id map would require an extra fetch-doc per op (or a diff against a pre-batch snapshot) — that's a non-trivial scope.
The cleaner path is to layer this on top of v2 docs +update, where --block-id is already the input/output contract: a v2 batch wrapper can echo the block IDs each op touched without any extra API calls. Since v1 is hidden/deprecated, investing here would be writing throwaway code.
For now the recovery story is what's stated in the description: pair with docs +fetch after a partial run. Will revisit once v2 batch surface is decided.
| {Name: "operations", Desc: "JSON array of operations (each entry: mode, markdown, selection_with_ellipsis, selection_by_title, new_title)", Required: true, Input: []string{common.File, common.Stdin}}, | ||
| {Name: "on-error", Default: "stop", Desc: "behavior when a single operation fails: stop | continue"}, | ||
| }, | ||
| Validate: func(ctx context.Context, runtime *common.RuntimeContext) error { |
There was a problem hiding this comment.
The upfront Validate hook can't catch in-batch staleness: if op[0] deletes a section that op[1]'s selection-by-title resolves into, op[1] passes Validate but fails at MCP time. For sequential semantics this may be by design, but it's worth calling out alongside the partial-failure note in the docstring/skill md so agents don't assume Validate guards against everything pre-execution.
There was a problem hiding this comment.
Documented as deliberate sequential semantics in 3d0302e — same Go doc block on DocsBatchUpdate:
Validate runs once up front against the static op list, before any MCP write. It cannot foresee in-batch staleness — for example, if op[0] deletes a section that op[1]'s --selection-by-title resolves into, op[1] passes Validate but fails at execute time. This is by design (sequential semantics; each op sees the doc state produced by the previous op). If you need stricter atomicity, split the batch or fetch the doc between groups of related ops.
Adding mid-batch re-validation would couple Validate to MCP state and lose the up-front 'fail before any MCP call' guarantee, which is the primary value of the static check. Documenting the limit explicitly seems like the better trade.
Agents editing a document often need to apply several related changes (rename a heading, update intro paragraph, insert a new section). Doing this today means N independent 'docs +update' invocations, each with its own MCP round-trip, partial-validation scope, and inconsistent reporting. Case 10 in the pitfall list tracks this as the 'no batch semantics' gap. Add a docs +batch-update shortcut that accepts a JSON array of update operations and applies them sequentially against a single document. The shape of each operation mirrors the flags of docs +update (mode, markdown, selection_with_ellipsis, selection_by_title, new_title), so converting an existing workflow is mechanical. Explicit non-goal: atomicity. There is no server-side transaction for update-doc, so a mid-batch failure leaves the document in a partial- apply state. The shortcut is honest about that in its description and supports --on-error=stop (default) to halt at the first failure plus --on-error=continue for best-effort runs. The response always reports per-op success/failure and how many ops landed before any halt, so callers can pair with docs +fetch to reconcile state. Validate runs the single-op rule set across every op before any MCP call, so a malformed entry fails the whole invocation up front rather than after N successful ops. Dry-run prints the update-doc argument map for each op. Coverage: 8-case parse test (object/scalar/empty/malformed rejections plus single and multi-op success), 9-case validate test (mode / markdown / selection combinations), and a buildBatchUpdateArgs test verifying optional fields are omitted when empty.
…isfy codecov 60% patch gate Prior unit tests covered parse / validate / build-args only, leaving DryRun and Execute at 0% which dropped the codecov/patch gate on #623 to 35.5%. Add mount-and-run tests that exercise the remaining paths: - TestDocsBatchUpdateDryRun — 3-op dry-run: asserts op_count, per-op step desc ("[1/3] replace_range" etc.) and overall batch description - TestDocsBatchUpdateValidateRejectsMalformedOp — validates the per-op Validate loop rejects a replace_range op missing its selection before any MCP call - TestDocsBatchUpdateValidateRejectsInvalidOnError — rejects a bogus --on-error enum value up front - TestDocsBatchUpdateExecuteAllSuccess — mocks two /mcp stubs and asserts stdout envelope: total=2, applied=2, stopped_early=false, per-op success=true - TestDocsBatchUpdateStopsOnFirstFailure — only one MCP stub for a 2-op batch, so the second call trips 'no stub' in httpmock; asserts applied=1, stopped_early=true, and the partial-result envelope still gets emitted before the error returns Introduces registerUpdateDocMCPStubs(count, payload) helper because httpmock stubs match exactly once, so batch paths need one stub per expected call. Coverage on docs_batch_update.go rises from ~35% to ~76%, above the codecov/patch 60% threshold.
herbertliu
force-pushed
the
feat/docs-batch-update
branch
from
5d23894 to
3d0302e
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
shortcuts/doc/docs_batch_update.go (1)
130-163: Consider checking context cancellation within the loop.For batches with many operations, the loop doesn't check
ctx.Done(), so a cancellation signal (e.g., user interrupt, timeout) won't be honored until after the current MCP call completes and the next iteration begins. Whilecommon.CallMCPToolmay internally respect context, explicitly checking at loop entry ensures prompt termination.♻️ Proposed fix to respect context cancellation
for i, op := range ops { + select { + case <-ctx.Done(): + runtime.Out(map[string]interface{}{ + "doc": runtime.Str("doc"), + "total": len(ops), + "applied": successCount, + "results": results, + "stopped_early": true, + }, nil) + return ctx.Err() + default: + } + // Re-run the same static warnings the single-op shortcut emits so // batch users get the same advisory signal per-op. for _, w := range docsUpdateWarnings(op.Mode, op.Markdown) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/doc/docs_batch_update.go` around lines 130 - 163, The batch loop over ops doesn't check for context cancellation; add an early check of the context's Done channel at the top of the loop (before emitting docsUpdateWarnings/buildBatchUpdateArgs and calling common.CallMCPTool) and return the context error if cancelled so the operation can terminate promptly; use runtime.Context().Done() (or the appropriate ctx in scope) and return runtime.Context().Err() (or ctx.Err()) when cancelled, ensuring this check is present before calling common.CallMCPTool and before appending success results (affecting the logic around normalizeWhiteboardResult, results = append(... batchUpdateResult ...), successCount, and the stopOnError handling).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@shortcuts/doc/docs_batch_update.go`:
- Around line 130-163: The batch loop over ops doesn't check for context
cancellation; add an early check of the context's Done channel at the top of the
loop (before emitting docsUpdateWarnings/buildBatchUpdateArgs and calling
common.CallMCPTool) and return the context error if cancelled so the operation
can terminate promptly; use runtime.Context().Done() (or the appropriate ctx in
scope) and return runtime.Context().Err() (or ctx.Err()) when cancelled,
ensuring this check is present before calling common.CallMCPTool and before
appending success results (affecting the logic around normalizeWhiteboardResult,
results = append(... batchUpdateResult ...), successCount, and the stopOnError
handling).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b7913659-4685-4cbe-a3e0-780890508c2c
📒 Files selected for processing (3)
shortcuts/doc/docs_batch_update.goshortcuts/doc/docs_batch_update_test.goshortcuts/doc/shortcuts.go
✅ Files skipped from review due to trivial changes (1)
- shortcuts/doc/shortcuts.go
🚧 Files skipped from review as they are similar to previous changes (1)
- shortcuts/doc/docs_batch_update_test.go
…el mid-batch Two CodeRabbit nitpicks from PR #623 review: 1. parseBatchUpdateOps's `[` prefix probe was running on the output of strings.TrimSpace, which doesn't treat U+FEFF (UTF-8 BOM) as whitespace. Payloads written by editors / shells that prepend a BOM (PowerShell redirection, some Windows editors) would fail with a confusing "must be a JSON array" error before the JSON ever got parsed. Trim the BOM explicitly between two TrimSpace passes so bytes around it don't matter. 2. The Execute loop didn't check ctx.Err() between ops, so a parent timeout or user interrupt would have to wait for the current MCP call to finish AND for the next op to also start before being honored — for a 10-op batch with a slow doc, that's a long tail. Check at the top of each iteration and emit the partial envelope (applied, results, stopped_early=true) before returning ctx.Err(), mirroring the --on-error=stop shape so callers don't need a separate parse path for cancellation. Tests: two new BOM cases in TestParseBatchUpdateOps (bare BOM + BOM sandwiched in whitespace), and TestDocsBatchUpdateRespectsContextCancellation which pre-cancels the context and verifies (a) ExecuteContext returns context.Canceled, (b) zero MCP stubs were consumed (no calls escaped the cancelled-ctx guard), (c) the partial envelope shows applied=0, stopped_early=true.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
shortcuts/doc/docs_batch_update_test.go (1)
410-453: ⚡ Quick winAssert the failed op is preserved in
results[]on stop-on-error.This test only checks
total,applied, andstopped_early. A regression that stops before appending the failing op toresults[]would still pass, even though that per-op entry is part of the batch contract. Please also assert the results length and that the last entry is the failed op with a non-empty error.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_batch_update_test.go` around lines 410 - 453, Update TestDocsBatchUpdateStopsOnFirstFailure to also assert that the per-op results array preserves the failing operation: extend the envelope struct to include Data.Results as a slice (e.g., []struct{Op map[string]interface{} `json:"operation"`; Error string `json:"error"`} or match the actual result schema), then after the existing envelope checks assert len(envelope.Data.Results) == 2 and that the last entry (envelope.Data.Results[1]) has a non-empty error string (or non-nil error field) to ensure the failing op is recorded; place these assertions immediately after the stopped_early checks in TestDocsBatchUpdateStopsOnFirstFailure (the test using runBatchUpdateShortcut).
🤖 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_batch_update_test.go`:
- Around line 270-345: The mounted-unit tests (TestDocsBatchUpdateDryRun,
TestDocsBatchUpdateValidateRejectsMalformedOp,
TestDocsBatchUpdateValidateRejectsInvalidOnError) only exercise shortcut logic;
add a real CLI end-to-end dry-run test under tests/cli_e2e/dryrun that invokes
the built CLI with the docs +batch-update command (same ops payload used by
TestDocsBatchUpdateDryRun) to verify the process exit code, that stdout is a
JSON envelope containing "op_count":3 and per-step descriptions like "[1/3]
replace_range", and that a malformed op triggers a non-zero exit code and an
error JSON from the Validate stage (mirror assertions from
TestDocsBatchUpdateValidateRejectsMalformedOp/InvalidOnError). Ensure the new
test uses the same request shapes and checks stdout JSON and process exit status
rather than internal runBatchUpdateShortcut calls.
---
Nitpick comments:
In `@shortcuts/doc/docs_batch_update_test.go`:
- Around line 410-453: Update TestDocsBatchUpdateStopsOnFirstFailure to also
assert that the per-op results array preserves the failing operation: extend the
envelope struct to include Data.Results as a slice (e.g., []struct{Op
map[string]interface{} `json:"operation"`; Error string `json:"error"`} or match
the actual result schema), then after the existing envelope checks assert
len(envelope.Data.Results) == 2 and that the last entry
(envelope.Data.Results[1]) has a non-empty error string (or non-nil error field)
to ensure the failing op is recorded; place these assertions immediately after
the stopped_early checks in TestDocsBatchUpdateStopsOnFirstFailure (the test
using runBatchUpdateShortcut).
🪄 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: 656d8805-fe4e-4f4c-a9e3-9bf112530935
📒 Files selected for processing (2)
shortcuts/doc/docs_batch_update.goshortcuts/doc/docs_batch_update_test.go
| func TestDocsBatchUpdateDryRun(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| f, stdout, _, _ := cmdutil.TestFactory(t, batchUpdateTestConfig()) | ||
|
|
||
| ops := `[ | ||
| {"mode":"replace_range","markdown":"A","selection_with_ellipsis":"old...A"}, | ||
| {"mode":"insert_before","markdown":"B","selection_by_title":"## Intro"}, | ||
| {"mode":"delete_range","selection_with_ellipsis":"stale...end"} | ||
| ]` | ||
| err := runBatchUpdateShortcut(t, f, stdout, []string{ | ||
| "+batch-update", | ||
| "--doc", "DOC123", | ||
| "--operations", ops, | ||
| "--dry-run", | ||
| "--as", "bot", | ||
| }) | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| out := stdout.String() | ||
| if !strings.Contains(out, "3-op sequential batch") { | ||
| t.Errorf("dry-run should describe 3-op batch, got: %s", out) | ||
| } | ||
| if !strings.Contains(out, `"op_count": 3`) && !strings.Contains(out, `"op_count":3`) { | ||
| t.Errorf("dry-run should set op_count=3, got: %s", out) | ||
| } | ||
| // Each op index appears as [i/N] inside a step desc. | ||
| for _, prefix := range []string{"[1/3] replace_range", "[2/3] insert_before", "[3/3] delete_range"} { | ||
| if !strings.Contains(out, prefix) { | ||
| t.Errorf("dry-run missing step %q; got: %s", prefix, out) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // TestDocsBatchUpdateValidateRejectsMalformedOp ensures a bad op inside | ||
| // --operations short-circuits before any MCP call. Covers the per-op | ||
| // Validate loop path that the parse/validate unit tests alone don't | ||
| // exercise from the CLI entry point. | ||
| func TestDocsBatchUpdateValidateRejectsMalformedOp(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| f, _, _, _ := cmdutil.TestFactory(t, batchUpdateTestConfig()) | ||
| err := runBatchUpdateShortcut(t, f, nil, []string{ | ||
| "+batch-update", | ||
| "--doc", "DOC123", | ||
| "--operations", `[{"mode":"replace_range","markdown":"x"}]`, // missing selection | ||
| "--dry-run", | ||
| "--as", "bot", | ||
| }) | ||
| if err == nil { | ||
| t.Fatalf("expected validation error for missing selection, got nil") | ||
| } | ||
| if !strings.Contains(err.Error(), "requires selection_with_ellipsis or selection_by_title") { | ||
| t.Fatalf("unexpected error message: %v", err) | ||
| } | ||
| } | ||
|
|
||
| func TestDocsBatchUpdateValidateRejectsInvalidOnError(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| f, _, _, _ := cmdutil.TestFactory(t, batchUpdateTestConfig()) | ||
| err := runBatchUpdateShortcut(t, f, nil, []string{ | ||
| "+batch-update", | ||
| "--doc", "DOC123", | ||
| "--operations", `[{"mode":"append","markdown":"x"}]`, | ||
| "--on-error", "panic-on-everything", | ||
| "--dry-run", | ||
| "--as", "bot", | ||
| }) | ||
| if err == nil { | ||
| t.Fatalf("expected validation error for bad --on-error, got nil") | ||
| } | ||
| if !strings.Contains(err.Error(), "invalid --on-error") { | ||
| t.Fatalf("unexpected error message: %v", err) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add a real CLI dry-run E2E for docs +batch-update.
These mounted-command tests cover the shortcut logic, but they do not exercise the runner’s actual dry-run surface: exit code, stdout JSON envelope, and the repo-specific Validate-stage reject behavior. Please add a tests/cli_e2e/... case for this shortcut so the dry-run path is covered through the same CLI layer users hit.
Based on learnings: "Dry-run E2E tests are required for every shortcut change; place in tests/cli_e2e/dryrun/ or corresponding domain directory" and "Validate-stage rejects → non-zero exit code."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shortcuts/doc/docs_batch_update_test.go` around lines 270 - 345, The
mounted-unit tests (TestDocsBatchUpdateDryRun,
TestDocsBatchUpdateValidateRejectsMalformedOp,
TestDocsBatchUpdateValidateRejectsInvalidOnError) only exercise shortcut logic;
add a real CLI end-to-end dry-run test under tests/cli_e2e/dryrun that invokes
the built CLI with the docs +batch-update command (same ops payload used by
TestDocsBatchUpdateDryRun) to verify the process exit code, that stdout is a
JSON envelope containing "op_count":3 and per-step descriptions like "[1/3]
replace_range", and that a malformed op triggers a non-zero exit code and an
error JSON from the Validate stage (mirror assertions from
TestDocsBatchUpdateValidateRejectsMalformedOp/InvalidOnError). Ensure the new
test uses the same request shapes and checks stdout JSON and process exit status
rather than internal runBatchUpdateShortcut calls.
|
Stepping back from the line-by-line review to raise a structural concern: should Codebase context:
What pushes me to raise this now (rather than as a v2 follow-up) is that the rationale used in the per-op
That argument is correct, and it's self-defeating. Adding one field to v1 was rejected as not worth it, but landing a whole new shortcut on v1 is several times the surface area — tests, docstring, skill teaching, plus the migration cost when v1 is removed. If "one field of v1 investment is throwaway," "one shortcut of v1 investment" is the same conclusion at higher cost. Either both are worth doing or neither is. The two shapes I think justify shipping:
What I'd push back on is the current shape: v1-only with v2 deferred. That commits the team to maintaining a v1-pinned shortcut on a sunset timeline, asks agents to adopt a surface that will need to be rewritten or removed, and — by the same logic used to defer block-id reporting — burns review/test budget on code that's expected to be thrown away. The one thing that flips this: an internal v1 sunset timeline substantially longer than the rest of the v2 migration. If v1 has guaranteed remaining life that's long enough to amortize this work, v1-only is defensible as a near-term ergonomic win. Without that, the default "deprecated = no new investment" convention should apply. Happy to be talked out of this if I'm misreading the timeline or the migration path. Otherwise I'd rather we land (1) or (2) than ship v1-only and chase a v2 follow-up. |
2 participants
Summary
Addresses Case 10 in the lark-cli pitfall list: Agents applying several related edits to one document currently pay N MCP round-trips and get N separate `{success:true}` responses, with no unified error/recovery story. This PR adds a `docs +batch-update` shortcut that accepts a JSON array of operations and runs them sequentially against a single document.
Changes
Explicit non-goal: atomicity
There is no server-side transaction for `update-doc`. A mid-batch failure leaves the document in a partial-apply state. The shortcut description states this up front. Callers choose how to handle it:
Pair with `docs +fetch` after a partial run to reconcile state manually.
Shape
```bash
lark-cli docs +batch-update --doc --operations '[
{"mode":"replace_range","markdown":"New intro","selection_by_title":"## Intro"},
{"mode":"insert_before","markdown":"> Note: revised","selection_with_ellipsis":"## Intro"},
{"mode":"delete_range","selection_with_ellipsis":"stale section...end"}
]'
```
Response:
```json
{
"doc": "",
"total": 3,
"applied": 3,
"stopped_early": false,
"results": [
{"index":0, "mode":"replace_range", "success":true, "result":{...}},
...
]
}
```
Validation semantics
Single-op validation runs against every op before any MCP call, so a malformed entry fails the whole invocation up front rather than after N successful ops. This matches what batch-users expect from `kubectl apply`-style tooling.
Test Plan
Summary by CodeRabbit
New Features
Bug Fixes / Behavior
Tests