feat(docs): add opt-in --emulate fallback for block_move_after/block_copy_insert_after - #1427
feat(docs): add opt-in --emulate fallback for block_move_after/block_copy_insert_after#1427leeguooooo wants to merge 5 commits into
Conversation
…copy_insert_after Some server deployments do not read src_block_ids and reject both commands (larksuite#758). --emulate opts into a client-side fallback: fetch the source blocks (full detail), rebuild them after --block-id via block_insert_after, re-fetch to verify the rebuilt blocks exist, then block_delete the originals (move only). Insert always precedes delete and any failure stops with the inserted/deleted state on stderr, so no path loses source content. The emulation is not semantically identical and says so in the flag help, a stderr warning, and a semantic_differences field: rebuilt blocks get new ids, comments and #block_id anchors are not migrated, and images are re-uploaded from their href. Block types carrying server-side tokens or state (whiteboard, sheet, bitable, table, ...) are rejected before any write. Default behavior without --emulate is unchanged.
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a client-side ChangesBlock move/copy emulation feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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
![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 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_update_emulate_test.go`:
- Around line 92-125: Update the two tests to assert typed error metadata
instead of relying on err.Error() text: in
TestBuildEmulatedBlockContentRejectsUnsupportedType, after calling
buildEmulatedBlockContent use errs.ProblemOf(err) to assert category/subtype
(already done) and replace the string substring checks with assertions on the
error’s params/cause by using errors.As to extract *errs.ValidationError (or the
concrete typed error) and check its Param or other fields; likewise in
TestRebuildEmulatedImgTagRequiresHref, after calling rebuildEmulatedImgTag use
errs.ProblemOf to check SubtypeFailedPrecondition and use errors.As to get the
validation error and assert the Param (or relevant field) is "href"/missing,
removing reliance on message text. Ensure you reference
buildEmulatedBlockContent and rebuildEmulatedImgTag when updating the
assertions.
In `@shortcuts/doc/docs_update_emulate.go`:
- Around line 375-398: The verification currently treats any newly appearing
block id as proof of a successful rebuild; change the newIDs collection to be
anchor-scoped so only blocks that sit inside the expected rebuilt subtree are
considered. Specifically, when iterating afterBlocks (the code that calls
indexEmulatedBlocks and builds newIDs), compute the anchor extent (e.g., derive
minStart/maxEnd or anchorID from the original srcList entries) and filter so you
only append b.id when afterBlocks[b.id] lies within that anchor extent or has
the expected parent/ancestor matching the anchor; keep the subsequent sort and
emptiness check but base them on this filtered set, and only call
emulateUpdateDoc("block_delete", ...) when the filtered newIDs prove the rebuilt
subtree exists at the anchor. Ensure you reference afterBlocks,
indexEmulatedBlocks, newIDs and srcList when making the change.
- Around line 344-346: The error returned from buildEmulatedBlockContent can be
a typed validation error missing the CLI param metadata; before returning err
from the caller in docs_update_emulate.go, detect when err is a validation/typed
error and set its param to "--src-block-ids" (or wrap it so the typed error's
Param field is populated) so rebuild failures surface the correct CLI metadata;
update the return path that currently does "piece, err :=
buildEmulatedBlockContent(b); if err != nil { return err }" to attach or wrap
the error with Param="--src-block-ids" using the project's typed error helper or
setter on the validation error type.
🪄 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: d1c51b66-fcfe-4fb2-a19e-1262630a5a22
📒 Files selected for processing (6)
shortcuts/doc/docs_update_emulate.goshortcuts/doc/docs_update_emulate_test.goshortcuts/doc/docs_update_test.goshortcuts/doc/docs_update_v2.goskills/lark-doc/references/lark-doc-update.mdtests/cli_e2e/docs/docs_update_dryrun_test.go
Address review feedback on the --emulate fallback: - Verification was "any new block id appeared after the insert", which in a shared document would treat a concurrent collaborator's edit as proof of our own insert and delete the originals — data loss. Replace it with anchor-scoped verification: confirm the rebuilt blocks landed as a contiguous run immediately after the anchor (document start for a page-id anchor, the final slots for -1) with the expected count and tag sequence. Anything that does not line up fails conservatively and keeps the originals, leaving a recoverable duplicate rather than risking loss. The top-level walker matches Lark's flat DocxXML export (sibling blocks, no wrapping page element). - buildEmulatedBlockContent now tags unsupported-type and missing-href rejections with Param --src-block-ids so the command layer knows which flag carried the offending source block. - Error-path tests assert the typed envelope via the shared assertValidationContract helper (category/subtype/param) instead of err.Error() substrings; add coverage for the concurrent-edit guard and the document-start/-end anchor cases.
|
Thanks @coderabbitai — addressed all three in 43b811e: 1. Anchor-scoped verification (the data-loss one) — fixed. You're right that "any new block id appeared" would, in a shared doc, treat a concurrent collaborator's edit as proof of our own insert and then delete the originals. Verification is now anchor-scoped: it confirms the rebuilt blocks landed as a contiguous run immediately after the anchor (document start for a page-id anchor, the final slots for 2. Tag the offending 3. Typed-error assertions — fixed. The two error-path tests now assert the typed envelope (category/subtype/param) via the package's shared Re-verified after the changes: full |
|
Triggering a fresh review pass to pick up the 43b811e changes. ʕ •ᴥ•ʔ 🎉 ✅ Action performedReview finished.
|
There was a problem hiding this comment.
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_update_emulate_test.go (1)
352-403: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd typed error metadata assertions to validation test cases.
Each case should assert the typed error envelope (category/subtype/param) instead of relying solely on message substring matching. Per context snippet 1,
validateUpdateV2returnserrs.NewValidationErrorwithSubtypeInvalidArgumentand.WithParam(...)for each rejection. Useerrors.As(err, &ve)to extract*errs.ValidationErrorand assertve.Parammatches the expected flag (e.g.,"--emulate","--revision-id","--src-block-ids"), and useerrs.ProblemOf(err)to assertSubtype. As per coding guidelines, error-path tests must assert typed metadata viaerrs.ProblemOf(category/subtype/param), not message substrings alone.♻️ Example refactor for one test case
{ name: "rejects non move/copy command", setFlags: map[string]string{"command": "append", "content": "<p>x</p>", "emulate": "true"}, want: "--emulate only applies to block_move_after and block_copy_insert_after", },In the test loop body:
err := validateUpdateV2(context.Background(), runtime) if err == nil { t.Fatal("expected validation error") } + // Assert typed metadata + problem, ok := errs.ProblemOf(err) + if !ok || problem.Subtype != errs.SubtypeInvalidArgument { + t.Fatalf("expected SubtypeInvalidArgument, got %v", err) + } + var ve *errs.ValidationError + if !errors.As(err, &ve) { + t.Fatalf("expected *errs.ValidationError, got %T", err) + } + // Each case should assert the expected param, e.g.: + // if ve.Param != "--emulate" { ... } + if !strings.Contains(err.Error(), tt.want) { t.Fatalf("error missing %q: %v", tt.want, err) }Repeat for all four test cases, asserting the appropriate
ve.Paramvalue for each.🤖 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_update_emulate_test.go` around lines 352 - 403, The test TestDocsUpdateEmulateValidation currently only checks error message substrings; update each subtest to extract the typed validation error returned by validateUpdateV2 using errors.As into a *errs.ValidationError (ve) and assert ve.Param equals the expected flag (e.g., "--emulate", "--revision-id", "--src-block-ids") and that errs.ProblemOf(err).Subtype (or the full ProblemOf result) matches SubtypeInvalidArgument; remove or keep substring checks as a secondary assertion but ensure the primary assertions use errors.As(., &ve) and errs.ProblemOf to validate category/subtype/param for each case. Use the test names and setFlags to map expected params: first two -> "--emulate", second -> "--revision-id", third -> "--src-block-ids", fourth -> "--src-block-ids"/contains-no-ids as appropriate, and update the loop body accordingly.Source: Coding guidelines
🧹 Nitpick comments (1)
shortcuts/doc/docs_update_emulate.go (1)
226-233: 💤 Low valueConsider caching compiled regexes for attribute lookups.
xmlAttrValuecompiles a new regex on each call. InrebuildEmulatedImgTag, this results in up to 6 compilations per img tag. Since this is the emulation fallback path (not the hot path), the impact is likely minimal, but a cached approach would improve performance for documents with many images.+var xmlAttrValueCache = make(map[string]*regexp.Regexp) +var xmlAttrValueMu sync.Mutex + func xmlAttrValue(attrs, name string) (string, bool) { - re := regexp.MustCompile(`(?:^|\s)` + regexp.QuoteMeta(name) + `="([^"]*)"`) + xmlAttrValueMu.Lock() + re, ok := xmlAttrValueCache[name] + if !ok { + re = regexp.MustCompile(`(?:^|\s)` + regexp.QuoteMeta(name) + `="([^"]*)"`) + xmlAttrValueCache[name] = re + } + xmlAttrValueMu.Unlock() m := re.FindStringSubmatch(attrs)Alternatively, pre-compile regexes for the known attribute names used in
rebuildEmulatedImgTag.🤖 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_update_emulate.go` around lines 226 - 233, xmlAttrValue currently compiles a new regexp on every call (causing multiple compilations per img tag in rebuildEmulatedImgTag); change it to reuse compiled regexes by introducing a package-level cache (e.g., map[string]*regexp.Regexp guarded by sync.RWMutex) or precompile regexes for the known attribute names used by rebuildEmulatedImgTag, and update xmlAttrValue to fetch the regexp from that cache (or the precompiled set) before calling FindStringSubmatch.
🤖 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.
Outside diff comments:
In `@shortcuts/doc/docs_update_emulate_test.go`:
- Around line 352-403: The test TestDocsUpdateEmulateValidation currently only
checks error message substrings; update each subtest to extract the typed
validation error returned by validateUpdateV2 using errors.As into a
*errs.ValidationError (ve) and assert ve.Param equals the expected flag (e.g.,
"--emulate", "--revision-id", "--src-block-ids") and that
errs.ProblemOf(err).Subtype (or the full ProblemOf result) matches
SubtypeInvalidArgument; remove or keep substring checks as a secondary assertion
but ensure the primary assertions use errors.As(., &ve) and errs.ProblemOf to
validate category/subtype/param for each case. Use the test names and setFlags
to map expected params: first two -> "--emulate", second -> "--revision-id",
third -> "--src-block-ids", fourth -> "--src-block-ids"/contains-no-ids as
appropriate, and update the loop body accordingly.
---
Nitpick comments:
In `@shortcuts/doc/docs_update_emulate.go`:
- Around line 226-233: xmlAttrValue currently compiles a new regexp on every
call (causing multiple compilations per img tag in rebuildEmulatedImgTag);
change it to reuse compiled regexes by introducing a package-level cache (e.g.,
map[string]*regexp.Regexp guarded by sync.RWMutex) or precompile regexes for the
known attribute names used by rebuildEmulatedImgTag, and update xmlAttrValue to
fetch the regexp from that cache (or the precompiled set) before calling
FindStringSubmatch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bf83c279-ba50-46ec-ad2a-c939b2196c35
📒 Files selected for processing (2)
shortcuts/doc/docs_update_emulate.goshortcuts/doc/docs_update_emulate_test.go
Address CodeRabbit review on PR larksuite#1427: - TestDocsUpdateEmulateValidation now asserts the typed error envelope (errs.ProblemOf subtype + *errs.ValidationError.Param) per case instead of relying on message substrings alone; substring check kept as secondary. - xmlAttrValue memoizes its per-attribute-name regex via sync.Map so re-inserting image-heavy docs no longer recompiles the same pattern on every call.
Add doc comments to the remaining undocumented functions in the emulate fallback and its tests, raising docstring coverage from 42% to 100% to satisfy the CodeRabbit threshold. Comments only; no behavior change.
… test Replace the inline ProblemOf/errors.As block in TestDocsUpdateEmulateValidation with the package's assertValidationContract helper (already used elsewhere in this file), matching the established Doc error-contract test idiom and also pinning Category/Params. No behavior change.
|
Thanks @coderabbitai — picked up both items from the 43b811e pass. 1. Typed-error assertions in 2. Also raised docstring coverage on both touched files from 42% to 100% (cc4e47c) to clear the coverage gate — comments only, no behavior change. Re-verified: full |
|
Tip For best results, initiate chat on the files or code changes.
The docstring sweep to 100% is a nice finishing touch. All green across ʕ •ᴥ•ʔ ✨ |
1 participant
Summary
On some server deployments (reproduced on a JP-tenant Lark international workspace, see #758),
docs +update --command block_move_after/block_copy_insert_afterfail withcommand 'block_move_after' requires content or src_block_ids but both were emptyeven though the CLI sendssrc_block_idscorrectly — the server side does not read the field. This PR adds an opt-in--emulateflag that reproduces move/copy semantics client-side with the primitives that do work, so agents are not blocked while the server bug persists. Default behavior is completely unchanged; without--emulateboth commands still send the same single request as today.Changes
--emulate(bool, default false) todocs +update, valid only withblock_move_after/block_copy_insert_after. When set, the command runs a client-side orchestration: fetch source blocks (full detail) → rebuild them after--block-idviablock_insert_after→ re-fetch to verify the rebuilt blocks exist →block_deletethe originals (move only). Insert always precedes delete and any failure stops immediately with the inserted/deleted state reported on stderr, so no path loses source content (shortcuts/doc/docs_update_emulate.go).p,h1–h9,ul,ol,li(re-wrapped in its original list type),blockquote,callout(rgb colors dropped, emoji kept),pre,img(rebuilt fromhref, effectively a re-upload). Block types carrying server-side tokens or state (whiteboard,sheet,bitable,table,synced_reference, …) are rejected with a typedfailed_preconditionerror before any write.semantic_differencesfield of the success envelope: rebuilt blocks get new block ids, comments and#block_idanchors on the originals are not migrated, and image file tokens change.--emulaterejects a non-default--revision-id(the orchestration issues multiple writes against the latest revision) and duplicate ids in--src-block-ids.--dry-run --emulateprints the multi-call plan (fetch → insert → verify-fetch → delete for move; three calls for copy).lark-doc-update.mdskill reference so agents discover it next to the move/copy documentation.Test Plan
make unit-test(full-racesuite green)go vet ./...gofmt -l .(no output)go mod tidy(no go.mod/go.sum changes)go run github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.1.6 run --new-from-rev=origin/main(0 issues)LARK_CLI_BIN=$PWD/lark-cli go test ./tests/cli_e2e/docs/ -run TestDocs_DryRunDefaultsToV2OpenAPI--emulate: unchanged behavior — the server still does not perform the move (observed both the docs +update --command block_move_after / block_copy_insert_after with --src-block-ids fails with 'src_block_ids empty' #758 hard error and, on a later run,result: "failed"with degrade warning "Instruction produced no document changes"; either waysrc_block_idsis not honored)--emulatesingle paragraph move, multi-block move (p+li),imgmove to-1(re-uploaded, new src token), copy keeping the originaltablesource block rejected withfailed_preconditionbefore any writeRelated Issues
Summary by CodeRabbit
New Features
--emulateflag forblock_move_afterandblock_copy_insert_afteras a client-side fallback when servers reject requests due to missingsrc_block_ids, including dry-run and execute support.Documentation
docs +updateguidance with behavior notes and limitations (block-id changes, link/comment behavior, image handling, supported block types, stop-on-failure).Tests