fix(sheets): +batch-update sub-op CLI-side validation + cond-format / filter schema alignment#1018
Merged
zhengzhijiej-tech merged 2 commits intoMay 21, 2026
Conversation
…builders
Sub-ops that omit --sheet-id (or any other required flag) used to slip
past CLI validation — Validate ran only against the standalone shortcut
path, and batchOpDispatch's translators built bodies from whatever
flagView returned, so a structurally broken sub-op surfaced as an opaque
server "sheet undefined not found" after a network round-trip.
Push each batchable shortcut's check trio down into its xxxInput builder:
1. resolveSpreadsheetToken — stays in Validate (batch already does it
once at the top level; sub-ops don't repeat).
2. requireSheetSelector(sheetID, sheetName) — new helper; flagView-
agnostic XOR + control-char check, called at the top of every
xxxInput.
3. shortcut-specific required / range / enum checks (--dimension,
--range, --start <= --end, --type pixel needs --size,
--float-image-id, image-token XOR image-uri, ...) — moved out of
Validate into the builder body.
All ~30 batchable xxxInput builders now return (map, error). Standalone
Validate shrinks to validateViaInput(xxxInput); DryRun / Execute
propagate the error. batch_op_dispatch entries drop the noErrTranslate
wrapper and pass the builder directly — its error bubbles up wrapped
with "operations[N] (+shortcut):" context.
Tests:
- TestBatchOp_ErrorEquivalence (7 cases): XOR / logical-constraint
errors fire identically from standalone and batch sub-op paths.
- TestBatchOp_RejectsBadSubOpInput (8 cases): cobra-required flags that
standalone catches via MarkFlagRequired now also get rejected CLI-side
on the batch path (where cobra is not in the loop).
- TestBatchOp_BodyMatchesStandalone (~40 cases) and
TestBatchOp_DispatchCoversReportedBugs continue to pass — bodies stay
byte-identical.
- BOE smoke (spreadsheet ICFwstkUGheyfptGWS2bB7RgcDf, sheet 51991c):
+batch-update with a sub-op missing --sheet-id now returns
"operations[0] (+dim-insert): specify at least one of --sheet-id or
--sheet-name" before any network call.
sheetMoveBatchInput (xiongyuanwen's batch-only explicit-source-index
requirement) is preserved — it's an orthogonal batch-specific constraint
not affected by this push-down.
…te#4 + larksuite#5) Two latent bugs in the object_crud translator surfaced during BOE smoke testing of +batch-update. Both are schema-alignment fixes against manage_conditional_format_object / manage_filter_object as declared in sheet-skill-spec/canonical-spec/tool-schemas/mcp-tools.json. larksuite#4 +cond-format: rule_type path + enum vocabulary --------------------------------------------------- condFormatEnhance used to write the user's --rule-type value into `properties.rule.type` (nested under a `rule` object). The server schema actually puts it at flat `properties.rule_type` and silently drops the nested form — so every conditional-format create/update secretly built the wrong document. Worse, the CLI enum exposed via flag-defs.json was its own invented vocabulary (cellValue / formula / duplicate / unique / topBottom / aboveBelowAverage / dataBar / colorScale / iconSet / textContains / dateOccurring / blankCell / errorCell) — none of those values were the strings the server accepts. Fix: - condFormatEnhance now writes `properties.rule_type = <value>` directly (no nested `rule` object). - Synced flag-defs.json + lark-sheets-conditional-format.md enum vocabulary from base to match the server: duplicateValues, uniqueValues, cellIs, containsText, timePeriod, containsBlanks, notContainsBlanks, dataBar, colorScale, rank, aboveAverage, expression, iconSet. -⚠️ Breaking: scripts passing the old CLI-invented enum values (e.g. --rule-type cellValue) now get a cobra "invalid value … allowed: …" error listing the new vocabulary. No alias layer. - TestObjectCRUDShortcuts_DryRun's +cond-format-update case updated to assert the flat properties.rule_type shape + new enum. larksuite#5 +filter-{update,delete}: auto-inject filter_id = sheet_id ------------------------------------------------------------- manage_filter_object's contract is "filter_id === sheet_id" for the sheet-scoped filter (per per-tool description in mcp-tools.json), and update / delete operations MUST carry filter_id. Standalone filterUpdateInput / filterDeleteInput never set it, so the server rejected with "filter_id is required for update/delete operation" on every call — both standalone AND inside +batch-update. Fix: - filterUpdateInput / filterDeleteInput now set input["filter_id"] = sheetID. - Because filter_id must equal sheet_id (not sheet_name), update / delete reject when only --sheet-name is given — there's no network lookup available inside the builder. The friendly error points at +workbook-info for resolving sheet-name → sheet-id. - create still omits filter_id (server requires that — id is server-allocated on creation). - New tests: * TestObjectCRUDShortcuts_DryRun gains a +filter-update happy-path case asserting filter_id is auto-injected + --range hoisting. * +filter-delete case updated to assert filter_id presence. * TestBatchOp_RejectsBadSubOpInput gains two cases asserting both +filter-update and +filter-delete reject --sheet-name-only with the friendly error. Docs (larksuite#2 + larksuite#3 + larksuite#8) synced from sheet-skill-spec ------------------------------------------------- Companion doc fixes that landed via npm run generate:cli + sync:cli in sheet-skill-spec; included here because the regenerated flag-defs and references markdown are byte-tracked in this repo: - larksuite#2: lark-sheets-sheet-structure.md — +dim-{hide,unhide,group, ungroup} --start/--end desc changed from "(0-based, inclusive)" to "(0-based)" / "(exclusive)" to match the half-open range semantics the code has always implemented (requireDimRange: end > start; dimRange uses end - 1 for column end letters). - larksuite#3: lark-sheets-workbook.md — +sheet-move section gains a note about the batch-internal requirement to pass --sheet-id AND --source-index explicitly (sheetMoveBatchInput's constraint). - larksuite#8: lark-sheets-pivot-table.md — +pivot-create --properties example drops the stale data_range field (the actual server schema uses --source as a hoisted flag; properties only carries rows / columns / values / filters / show_*_grand_total).
|
|
github-actions
Bot
added
domain/ccm
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
zhengzhijiej-tech
marked this pull request as ready for review
May 21, 2026 09:19
zhengzhijiej-tech
merged commit May 21, 2026
8e8a511
into
larksuite:feat/lark-sheets-refactor
4 checks passed
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
3 participants
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
Two adjacent fixes around
+batch-updateand related write shortcuts, both surfaced by BOE smoke testing of multi-op batches.8d0fefd
fix(sheets): push +batch-update sub-op validation down into xxxInput builders— sub-ops that omit--sheet-id(or any other required flag) used to slip past CLI validation and surface as opaque server errors likesheet undefined not foundafter a network round-trip. Every batchablexxxInputbuilder now returns(map, error)and owns its own sheet-selector + required-field checks via the newrequireSheetSelectorhelper. StandaloneValidateshrinks tovalidateViaInput(xxxInput), so identical bad input now fires identical CLI errors from both standalone and batch paths (wrapped asoperations[N] (+shortcut): ...in the batch case).sheetMoveBatchInput's explicit-source-indexconstraint is preserved.4e44e66
fix(sheets): align +cond-format / +filter with server schema (#4 + #5)+cond-format:condFormatEnhancewas writingproperties.rule.type(nested), but the server schema (manage_conditional_format_object) wants flatproperties.rule_type. The CLI enum (cellValue / formula / duplicate / ...) was also a CLI-invented vocabulary none of whose values the server accepts. Breaking: enum aligned with server (cellIs / duplicateValues / containsText / timePeriod / containsBlanks / notContainsBlanks / dataBar / colorScale / rank / aboveAverage / expression / iconSet); scripts using old names now hit cobrainvalid value ..., allowed: .... No alias layer.+filter-{update,delete}: server contract isfilter_id === sheet_idand both ops MUST carryfilter_id— the CLI translator never set it, so every call failed server-side. Translator now auto-injectsfilter_idfromsheet_id. Becausefilter_idmust equalsheet_id(notsheet_name), update/delete reject--sheet-name-only with a friendly error pointing at+workbook-info.+dim-{hide,unhide,group,ungroup}--start/--enddesc changed from(0-based, inclusive)to(0-based)/(exclusive)to match the half-open range semantics the code has always implemented.+sheet-movereference gains a batch-internal warning about explicit--sheet-id+--source-index.+pivot-create --propertiesexample drops the staledata_rangefield (data source goes through--source).Test plan
go vet ./shortcuts/sheets/...go build ./...go test ./shortcuts/sheets/...— all green, includingTestBatchOp_BodyMatchesStandalone(~40 cases): batch sub-op bodies stay byte-identical to standalone bodies.TestBatchOp_ErrorEquivalence(7 cases): XOR / logical-constraint errors fire identically standalone vs batch.TestBatchOp_RejectsBadSubOpInput(10 cases): cobra-required flags + filter sheet-name-only now rejected CLI-side in batch.TestObjectCRUDShortcuts_DryRun: cond-format body uses flatproperties.rule_type; filter update/delete auto-injectfilter_id = sheet_id.ICFwstkUGheyfptGWS2bB7RgcDf):--sheet-idin a batch sub-op now returnsoperations[0] (+dim-insert): specify at least one of --sheet-id or --sheet-namebefore any network call.+cond-format-create --rule-type cellIswritesproperties.rule_type: "cellIs"flat; old enumcellValuerejected by cobra with the new enum listed.+filter-update --sheet-id Xauto-populatesfilter_id: X;--sheet-nameonly gets the friendly error.Notes / follow-ups
+cond-format-{create,update} --rule-type: any script passing the old vocabulary needs to migrate to server enum (cellValue → cellIs,duplicate → duplicateValues, etc.). Per offline discussion no alias layer is added — earlier alignment is better than carrying two vocabularies.+sparkline-updatestill requires user to hand-fillproperties.sparklines[].sparkline_id; auto list-then-update is a separate change of its own (architecture limit, not in this PR).0ea7c14/1e05e7bflagView abstraction; no overlap with those commits.