Skip to content

feat(sheets): accept the --range / --cells / border shapes callers actually send - #2317

Closed
chendaxin-tk wants to merge 6 commits into
mainfrom
feat/call-compat
Closed

feat(sheets): accept the --range / --cells / border shapes callers actually send#2317
chendaxin-tk wants to merge 6 commits into
mainfrom
feat/call-compat

Conversation

@chendaxin-tk

@chendaxin-tk chendaxin-tk commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

Summary

Four input shapes that callers habitually send to the sheets shortcuts are now accepted outright — or, where the meaning is genuinely ambiguous, rejected with a prescription that names the exact fix. Each one is an eval-trace cluster measured against real calls, and each rewrite happens on an existing normalizer seam, so --writes items and +batch-update sub-ops get it too — the same item translates the same way whichever of the three entry points it arrives through.

Changes

  • A sheet prefix in --range is read as the selector. --range "Sheet1!A1:D20" no longer dies on specify at least one of --sheet-id or --sheet-name — the prefix fills sheet_name and the bare A1 range reaches the tool. 707 traced calls failed on that error, 53% of them already naming the sheet inside --range. Covers the standalone commands (cobra PreRunE), +batch-update sub-ops and +cells-set --writes items alike. An explicit --sheet-id / --sheet-name stays authoritative: the CLI never rewrites the selector from a prefix that disagrees with it, and never sizes a qualified anchor either — that combination keeps failing locally with the cells-vs-range mismatch instead of shipping a range and a sheet_name naming different sheets. Forwarding a disagreeing prefix inside --range itself is unchanged pre-existing behavior; how the backend resolves it is not something this PR touches.

  • The openpyxl / gspread --cells shapes are accepted. A {"cells": […]} envelope (the flag name mistaken for a JSON key — 11 of 21 traced expected type "array", got "object" rejections), a lone cell object without the 2D wrapper, and bare scalars in cell slots all rewrite onto the wire contract; --values becomes a silent alias for --cells. A bare single-cell --range now behaves as an anchor sized from the payload, the same inference +csv-put already does for --start-cell. null cells stay rejected — {} and {"value":""} are both plausible readings — and the cells-vs-range mismatch (132 rejections across 93 case-runs) now reports both axes at once and hands back the range that fits the payload rather than resizing on the caller's behalf.

  • Border thickness vocabulary gets its observed fallbacks. openpyxl's hair in either the style or the weight slot, a numeric line width, and the Google Sheets width key fold onto the thin / medium / thick enum. Everything that scored zero in the trace tally (dashDot, mediumDashed, xlContinuous, CSS hidden, …) stays rejected with the enum in the message.

  • The sheet part of a range is parsed with the front-end ref lexer's grammar. splitRangeSheetPrefix and parseCellRange now share one scanner, so both agree on what a separator is: the full-width , the backslash-escaped forms of both widths, quoted names with the doubled-quote escape, and a ! living inside a quoted name ('Q1!Sales'!A1). parseCellRange keeps the qualifier exactly as written, since ranges rendered from it are both shipped to the server and printed for the caller to paste back.

Test Plan

  • Unit tests pass — go test ./shortcuts/sheets/
  • Dry-run E2E — go test ./tests/cli_e2e/sheets/ -run DryRun; new files sheets_range_sheet_prefix_dryrun_test.go, sheets_cells_shapes_dryrun_test.go and sheets_border_vocab_dryrun_test.go assert method / URL / tool name / full tool input for every accepted shape, and pin that the deliberately-rejected ones still fail
  • Manual local verification — each accepted and rejected shape driven through lark-cli sheets +cells-get / +cells-set / +cells-set-style … --dry-run and the emitted payload inspected
  • Every commit builds and passes both suites in isolation, so the history stays bisectable
  • Review follow-up (4784a4df) — each reported finding reproduced against a freshly built binary before being fixed or declined, with the outcome recorded under the comment it answers
  • Live E2E — TestSheets_CallCompatWorkflow builds its own workbook, writes through a quoted sheet prefix with no selector flag (scalar cells, bare anchor), reads back through the same prefix and stamps an openpyxl hair border, then tears the workbook down. Skips without tenant credentials; CI's e2e-live job is what runs it

Related Issues

  • None

Summary by CodeRabbit

  • New Features

    • Sheet-prefixed ranges now work across reads, clears, batch updates, and writes, including quoted sheet names.
    • +cells-set accepts --values as an alias for --cells.
    • Single-cell anchors expand automatically to match multi-cell payloads.
    • Cell payloads support scalar, enveloped, mixed, and --writes formats with consistent normalization.
    • Border styles support additional width and weight formats, including numeric values and hair.
  • Bug Fixes

    • Improved validation messages for mismatched, ragged, empty, or invalid cell matrices.
    • Explicit sheet selectors remain authoritative when ranges include sheet prefixes.

Eval traces: 707 calls to +cells-get / +csv-get / +csv-put / +cells-set /
+cells-clear died on "specify at least one of --sheet-id or --sheet-name",
and 53% of them had already named the sheet inside --range
("Sheet1!A1:D20"). The sheet was known, only the flag was missing — so the
prefix now fills the selector and the bare A1 range goes to the tool.
Wired on both paths: a PreRunE stage in the sheets ergonomics layer for
standalone commands, and the sub-op translator for +batch-update.

The grammar follows the front-end ref lexer (byted-sheet TractorLexer):
the full-width ! is an equal separator, an unquoted name can contain
neither width (so splitting on the first one is safe), and a quoted name
keeps its doubled-quote escape and may itself contain a "!". Unquoted
names with spaces are accepted here though the lexer rejects them — a
--range flag has none of a formula's tokenizing ambiguity.
sheetNameFromA1 delegates to the same splitter instead of carrying a
second, looser grammar.

Scope guards: an explicit --sheet-id / --sheet-name stays authoritative
and --range passes through untouched, so a disagreeing prefix cannot
silently retarget a write; only --range carries the rewrite, since
+range-copy / +range-move / +range-fill name their destination sheet with
--target-sheet-id.
07-28 只修了 border_styles.<side>.style 里的 thin/medium/thick,同族的另外两种
写法仍在报错。对 596 条 trace 做频次统计,边框取值的错法就这几种:

  weight 槽 "hair"   476 次 / 19 个用例   ← 本次新增
  style  槽 "thin"  1795 次 / 39 个用例   (07-28 已修)
  style  槽 "hair"    76 次 /  2 个用例   ← 本次新增
  weight 槽 数字        10 次 /  2 个用例   ← 本次新增(07-28 报告 Case 2)
  width  键(GSheets) 35 次 /  3 个用例   ← 本次新增

根因是契约把一个视觉概念拆成 style(线型)× weight(粗细)两个字段,而 openpyxl
把两者塞进一个词 Side(border_style="thin"),于是同几个粗细词在两个槽位都会出现。
borderWeightWord 一个函数同时服务两个槽位,挂在 expandBorderAllShorthand 这个唯一
漏斗上,四条载体路径(--border-styles / --cells 内联 / --styles 载荷 /
+workbook-create)一起生效。

weight 先于 style 归一是有意的:{"style":"thin","weight":"1"} 只有等 "1" 先变成
"thin",style 那步才看得出显式 weight 与词义一致而非冲突。显式冲突
(thin + thick)保持报错,不替用户选。

刻意不收:openpyxl 完整线型表(dashDot / mediumDashed / slantDashDot)、VBA
xlContinuous、CSS hidden、Google Sheets SOLID_THICK、line_style / thickness 等
键别名、style 与 weight 装反、px/pt 后缀 —— trace 里全是 0 次;solid_thin、
border_width、border_color 各只有 1 个用例。它们继续走 enum 报错(报错带允许值
和 did-you-mean,一轮能改对),符合本文件顶部的静默别名准入门槛:真实词汇 **且**
跨批次/≥3 任务复现。新增用例里有一条反向断言把这条线钉住。

TestCellsSetStyle_BorderWeightNumberNamesEnum 的探针从数字换成布尔——数字现在会被
归一化,不再走报错路径,enum-over-skeleton 那条文案规则改用布尔来钉。
…the rest

The --cells shape family is the single largest client-side rejection cluster
for +cells-set in the eval corpus. Traced against 14,024 real calls it splits
into two habits, and each gets the treatment its ambiguity allows.

Accepted outright, both unambiguous, both on the existing jsonFlagNormalizers
seam (so --writes items and +batch-update sub-ops get them too):

  - {"cells": […]} envelope — an agent generating the payload in a script
    writes json.dump({"cells": cells}, f), mistaking the flag name for a JSON
    key. 11 of 21 traced `expected type "array", got "object"` rejections are
    this exact shape. Only a lone "cells" key unwraps; siblings mean the
    object is the whole tool input and dropping them would write elsewhere.
  - bare scalars in cell slots — the openpyxl / gspread habit of passing a
    plain values matrix, which real rows mix with cell objects as soon as a
    formula appears (["1","电动大门",10331.00,{"formula":"=D2*E2"}]).

  null is deliberately left failing: {} (leave the cell alone) and
  {"value":""} (write an empty string) are both plausible readings, and the
  normalizer only rewrites what is beyond doubt.

Renamed silently on the same grounds: --values is what gspread calls the
payload, and what this CLI's own +workbook-create calls its untyped 2D data.
Because bare scalars now lift into {"value":…}, the plain matrix a --values
caller passes ('[["工作内容"]]') is already accepted verbatim under --cells —
the name was the only thing wrong, which puts it in commandFlagAliases rather
than the prescription table. That drops the round trip a prescription costs
(eval F8: 170 hits, 1.9% of failures) and covers the +batch-update sub-op
path, which reads the same alias table and would otherwise get no hint at all
(a prescription only rides on cobra's unknown-flag branch).

Inferred, matching the libraries these callers arrive from: a bare
single-cell --range is now an anchor, sized from the payload — the same
inference +csv-put already does for --start-cell. The range resolves locally
and ships in full, so the server still gets the strict match it enforces. An
explicit extent ("A1:A1", "A1:C10") is never inferred over.

Prescribed, because it cannot be guessed safely: the cells-vs-range mismatch
(132 rejections across 93 case-runs) now reports both axes at once and hands
back the range that fits the payload, plus the inclusive-end note that
explains its biggest sub-bucket — A1:C10 being 10 rows. Growing the range
would overwrite rows the caller never mentioned and shrinking it would drop
data, so the choice stays with the caller. Ragged rows get their own message
instead of being reported as a range mismatch.

Supporting refactor: parseCellRange replaces the prefix-strip / split-on-":"
/ splitCellRef triplication (rangeDimensions becomes a thin wrapper, its
error wording kept byte-for-byte since +styles-put surfaces it verbatim), and
cellsExtent is the one authority on whether a payload is rectangular, so the
anchor expansion and the dimension check cannot disagree. Two bugs fell out
of the new tests: a leading space before the sheet name survived into every
rendered range, and a payload of empty rows would have rendered a malformed
suggestion.
…mmar

parseCellRange cut the sheet off with strings.Index(range, "!"), which
disagrees with the grammar splitRangeSheetPrefix already implements from the
front-end ref lexer (byted-sheet TractorLexer.ts). Two spellings the lexer
treats as ordinary therefore failed to parse at all:

  --range '甘特图!B3'        full-width separator (ExclamationMark accepts it)
  --range "'Q1!Actual'!B3"   quoted name owning a "!" (quotes delimit, so it may)

An unparsable range is deliberately deferred ("the range validator's job"),
so the failure was silent in both directions: the anchor never expanded and
the dimension mismatch never got its prescription. Reachable whenever the
prefix survives to the shortcut — an explicit --sheet-id/--sheet-name keeps
it (the selector rewrite only fires when the pair is empty), as do
--source-range / --target-range, which that rewrite deliberately skips.

The grammar now lives in one place. scanSheetQualifier reports the parsed
sheet name AND the byte offset just past the separator; splitRangeSheetPrefix
is rewritten on top of it (all 20 of its grammar cases unchanged), and
parseCellRange slices the qualifier off at that offset. The offset is the
point: a range rendered from a parse is both shipped to the server and
printed for the caller to paste back, so the qualifier has to survive
verbatim — quotes, full-width separator and all — which a name parsed and
re-quoted could not promise.

Naming, while here: cellRange.prefix said where the field sits, not what it
holds. It is now sheetQualifier (verbatim, separator included) alongside
sheetName (parsed, unquoted) — the sheet a range names is what the type is
about, and the next caller that needs it should not reach for the raw string.
@CLAassistant

CLAassistant commented Aug 12, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions Bot added domain/ccm PR touches the ccm domain size/L Large or sensitive change across domains or core paths labels Aug 12, 2026
@coderabbitai

coderabbitai Bot commented Aug 12, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 52b1c917-709f-46b8-8b75-5f4c7e9ed1c3

📥 Commits

Reviewing files that changed from the base of the PR and between 4784a4d and 8f5c6cc.

📒 Files selected for processing (1)
  • tests/cli_e2e/sheets/sheets_call_compat_workflow_test.go

📝 Walkthrough

Walkthrough

Sheets shortcuts now support sheet-qualified ranges, common cells-set payload shapes, anchor-range expansion, rectangular validation, and broader border vocabulary normalization. Unit, integration, and CLI dry-run tests cover accepted inputs and validation errors.

Changes

Sheets normalization and validation

Layer / File(s) Summary
Sheet-qualified range handling
shortcuts/sheets/range_sheet_prefix.go, shortcuts/sheets/flag_view.go, shortcuts/sheets/flag_ergonomics.go, shortcuts/sheets/batch_op_dispatch.go, shortcuts/sheets/lark_sheet_object_crud.go, shortcuts/sheets/range_sheet_prefix_test.go, tests/cli_e2e/sheets/sheets_range_sheet_prefix_dryrun_test.go
Supported commands extract sheet names from prefixed ranges, preserve explicit selectors, and normalize standalone and batch operations.
Cell payload and range validation
shortcuts/sheets/helpers.go, shortcuts/sheets/lark_sheet_write_cells.go, shortcuts/sheets/cells_set_writes_test.go, shortcuts/sheets/lark_sheet_write_cells_test.go, shortcuts/sheets/batch_key_vocab_test.go, shortcuts/sheets/json_flag_normalize_test.go, shortcuts/sheets/flag_ergonomics_test.go, tests/cli_e2e/sheets/sheets_cells_shapes_dryrun_test.go
cells-set unwraps supported envelopes, converts scalar cells, expands bare anchors, validates rectangular payloads, and reports dimension mismatches. --values aliases --cells.
Border vocabulary normalization
shortcuts/sheets/style_vocab.go, shortcuts/sheets/styles_acceptance_test.go, shortcuts/sheets/styles_prescription_test.go, shortcuts/sheets/json_flag_normalize_test.go, tests/cli_e2e/sheets/sheets_border_vocab_dryrun_test.go
Border aliases, thickness words, numeric widths, and width values normalize to supported styles and weights. Unsupported styles remain validation errors.
Sheets compatibility workflow
tests/cli_e2e/sheets/sheets_call_compat_workflow_test.go
An end-to-end workflow verifies qualified writes, read-back values, anchor expansion, and hair border normalization.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant CLI
  participant RangeNormalizer
  participant CellsSet
  participant SheetsRequest
  CLI->>RangeNormalizer: process prefixed --range
  RangeNormalizer->>CLI: set sheet_name and bare range
  CLI->>CellsSet: normalize cells and validate dimensions
  CellsSet->>SheetsRequest: build normalized set_cell_range request
Loading

Possibly related PRs

  • larksuite/cli#2091: Covers the same Sheets batch-update, cell normalization, range-prefix, and style vocabulary areas.
  • larksuite/cli#2146: Covers related Sheets flag normalization and alias ergonomics in flag_ergonomics.go.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 93.33% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly summarizes the PR's main changes to accept common Sheets range, cell, and border input shapes.
Description check ✅ Passed The description includes the required Summary, Changes, Test Plan, and Related Issues sections with specific scope and verification details.
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/call-compat

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 8

🧹 Nitpick comments (1)
tests/cli_e2e/sheets/sheets_cells_shapes_dryrun_test.go (1)

133-138: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Error assertions rely on message text instead of typed error metadata. Both new rejection tests check a substring of the rendered error. Each error is an *errs.ValidationError that carries Param, Category, and Subtype. A change that drops the param attribution or reclassifies the error would keep both tests green.

  • tests/cli_e2e/sheets/sheets_cells_shapes_dryrun_test.go#L133-L138: parse the stderr envelope with gjson and assert error.type, error.param, and error.message instead of matching two escapings of got "null" across combined output.
  • shortcuts/sheets/lark_sheet_write_cells_test.go#L775-L781: add an errors.As assertion on *errs.ValidationError and check ve.Param alongside the existing message check.

The coding guidelines state: "Error tests must assert typed metadata and cause preservation rather than message text alone."

🤖 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 `@tests/cli_e2e/sheets/sheets_cells_shapes_dryrun_test.go` around lines 133 -
138, Strengthen the rejection assertions in
tests/cli_e2e/sheets/sheets_cells_shapes_dryrun_test.go:133-138 by parsing the
stderr envelope with gjson and asserting error.type, error.param, and
error.message instead of matching rendered text in combined output. In
shortcuts/sheets/lark_sheet_write_cells_test.go:775-781, use errors.As to
extract *errs.ValidationError and assert ve.Param alongside the existing message
check; both sites require direct test updates.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@shortcuts/sheets/flag_ergonomics_test.go`:
- Around line 475-495: Decode each dry-run request and assert normalized fields
directly instead of searching serialized output: in
shortcuts/sheets/flag_ergonomics_test.go:475-495, verify cells[0][0].value is
"工作内容"; in shortcuts/sheets/cells_set_writes_test.go:47-62, verify the batch
operation input contains sheet_name "Sheet1" and range "A1".

In `@shortcuts/sheets/flag_view.go`:
- Around line 363-384: Update normalizeRangeSheetPrefix to write the derived
sheet selector using the key returned by lookupRawWithKey("sheet-name"),
preserving the existing spelling when both empty selector keys are present. Keep
the range value normalization unchanged.

In `@shortcuts/sheets/lark_sheet_write_cells.go`:
- Around line 1009-1060: Replace first-ASCII-! prefix parsing in
stripSheetPrefix and the batch fan-out range splitting logic with
scanSheetQualifier, matching parseCellRange’s separator grammar for quoted names
and full-width separators. Preserve existing prefix and range behavior, and add
regression tests covering both quoted sheet names containing ! and full-width
separators.

In `@shortcuts/sheets/range_sheet_prefix_test.go`:
- Around line 63-189: Add a self-contained live end-to-end test alongside
TestRangeSheetPrefix_StandaloneFillsSelector that creates or selects a
disposable sheet, executes one operation using a sheet-prefixed range, verifies
the live result, and cleans up the sheet/resource with guaranteed teardown.
Exercise the normalized selector/range through the actual Sheets tool rather
than only dry-run request inspection.

In `@shortcuts/sheets/styles_acceptance_test.go`:
- Around line 187-189: Strengthen both rejection tests to assert typed
validation metadata, not only message text: in
shortcuts/sheets/styles_acceptance_test.go lines 187-189, extend the corpus
error expectation with the validation category and subtype and call
requireValidation before checking the message; in
tests/cli_e2e/sheets/sheets_border_vocab_dryrun_test.go lines 72-78, parse the
typed JSON validation envelope from stderr and assert error.type, error.subtype,
error.param, and the expected message.

In `@tests/cli_e2e/sheets/sheets_border_vocab_dryrun_test.go`:
- Around line 17-20: Add a self-contained live E2E test alongside
TestSheets_BorderWeightVocabularyDryRun that invokes the Sheets service with
each newly accepted --border-styles form and verifies the request succeeds with
the normalized payload. Keep the existing dry-run assertions, and use the test’s
established live-service setup and cleanup patterns.

In `@tests/cli_e2e/sheets/sheets_range_sheet_prefix_dryrun_test.go`:
- Around line 192-196: Update the validation-error assertions in the sheets
range dry-run test to parse the structured validation envelope from the command
result instead of checking only combined output text. Assert the typed
error.type, error.subtype, and error.param fields, and retain the message
assertion plus cause-preservation check using the test’s existing error-envelope
helpers.
- Around line 139-172: Add a separate dry-run E2E test for the `sheets
+batch-update` command, rather than extending
`TestSheets_CellsSetWritesSheetPrefixDryRun`. Invoke it with an `--operations`
item using the `Sheet1!A1` range and cells payload, then inspect the emitted
batch operation to assert `sheet_name` is `Sheet1` and `range` is `A1`.

---

Nitpick comments:
In `@tests/cli_e2e/sheets/sheets_cells_shapes_dryrun_test.go`:
- Around line 133-138: Strengthen the rejection assertions in
tests/cli_e2e/sheets/sheets_cells_shapes_dryrun_test.go:133-138 by parsing the
stderr envelope with gjson and asserting error.type, error.param, and
error.message instead of matching rendered text in combined output. In
shortcuts/sheets/lark_sheet_write_cells_test.go:775-781, use errors.As to
extract *errs.ValidationError and assert ve.Param alongside the existing message
check; both sites require direct test updates.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: d3261178-c657-4097-a95d-48bf39057729

📥 Commits

Reviewing files that changed from the base of the PR and between 1e87f67 and 86e2ddc.

📒 Files selected for processing (19)
  • shortcuts/sheets/batch_key_vocab_test.go
  • shortcuts/sheets/batch_op_dispatch.go
  • shortcuts/sheets/cells_set_writes_test.go
  • shortcuts/sheets/flag_ergonomics.go
  • shortcuts/sheets/flag_ergonomics_test.go
  • shortcuts/sheets/flag_view.go
  • shortcuts/sheets/helpers.go
  • shortcuts/sheets/json_flag_normalize_test.go
  • shortcuts/sheets/lark_sheet_object_crud.go
  • shortcuts/sheets/lark_sheet_write_cells.go
  • shortcuts/sheets/lark_sheet_write_cells_test.go
  • shortcuts/sheets/range_sheet_prefix.go
  • shortcuts/sheets/range_sheet_prefix_test.go
  • shortcuts/sheets/style_vocab.go
  • shortcuts/sheets/styles_acceptance_test.go
  • shortcuts/sheets/styles_prescription_test.go
  • tests/cli_e2e/sheets/sheets_border_vocab_dryrun_test.go
  • tests/cli_e2e/sheets/sheets_cells_shapes_dryrun_test.go
  • tests/cli_e2e/sheets/sheets_range_sheet_prefix_dryrun_test.go

Comment thread shortcuts/sheets/flag_ergonomics_test.go
Comment thread shortcuts/sheets/flag_view.go
Comment thread shortcuts/sheets/lark_sheet_write_cells.go
Comment thread shortcuts/sheets/range_sheet_prefix_test.go
Comment thread shortcuts/sheets/styles_acceptance_test.go
Comment thread tests/cli_e2e/sheets/sheets_border_vocab_dryrun_test.go
Comment thread tests/cli_e2e/sheets/sheets_range_sheet_prefix_dryrun_test.go
Comment thread tests/cli_e2e/sheets/sheets_range_sheet_prefix_dryrun_test.go
@github-actions

github-actions Bot commented Aug 12, 2026

Copy link
Copy Markdown

🚀 PR Preview Install Guide

🧰 CLI update

npm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@8f5c6ccc853c3fcf1356f44fac2e7c3b6a50770e

🧩 Skill update

npx skills add larksuite/cli#feat/call-compat -y -g

@codecov

codecov Bot commented Aug 12, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 91.11111% with 24 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.53%. Comparing base (6402080) to head (8f5c6cc).
⚠️ Report is 50 commits behind head on main.

Files with missing lines Patch % Lines
shortcuts/sheets/range_sheet_prefix.go 82.79% 8 Missing and 8 partials ⚠️
shortcuts/sheets/style_vocab.go 90.47% 3 Missing and 3 partials ⚠️
shortcuts/sheets/flag_view.go 88.23% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2317      +/-   ##
==========================================
+ Coverage   76.35%   76.53%   +0.18%     
==========================================
  Files         991     1044      +53     
  Lines      106029   116110   +10081     
==========================================
+ Hits        80954    88868    +7914     
- Misses      18941    20450    +1509     
- Partials     6134     6792     +658     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@xiongyuanwen-byted xiongyuanwen-byted left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed with the branch checked out locally: built the binary, ran the unit suite and the dry-run E2E suite (all green once LARK_CLI_BIN points at a fresh build), and drove the accepted / rejected shapes through --dry-run by hand, comparing against a pre-PR binary. The trace-driven scoping, the accept-vs-prescribe discipline, and the test coverage are all excellent.

Verified locally:

  • All four clusters emit exactly the wire payloads the tests pin; the deliberately-rejected shapes still fail with the enum / prescription in the message.
  • +batch-update sub-ops get every rewrite: sheet prefix, {"cells":…} envelope, scalar lift, values alias, anchor expansion.
  • --writes items get the prefix rewrite and the scalar lift, but not the envelope unwrap or the values alias — see inline comment.
  • The both-axes mismatch message hands back a paste-able range and reads well.

Three findings from hands-on probing, most severe first, inline below. Only the first one is something I'd want resolved (or explicitly accepted) before merge.

Comment thread shortcuts/sheets/lark_sheet_write_cells.go
Comment thread shortcuts/sheets/lark_sheet_write_cells.go
Comment thread shortcuts/sheets/flag_ergonomics.go
Anchor expansion no longer sizes a sheet-qualified range. Such a range only
reaches expandAnchorRange beside an explicit --sheet-id / --sheet-name, since
all three entry points fold the prefix into the selector when none was given —
so the prefix is one that disagrees with the selector, and sizing it shipped
{"range":"Sheet1!A1:B2","sheet_name":"Other"} where the pre-anchor CLI had
failed locally with the cells-vs-range mismatch. Trading a local prescription
for a wire payload whose two halves name different sheets is the wrong
direction; a qualified anchor stays a mismatch.

--writes items now really do get the payload rewrites. cellsSetWritesOps gives
each item the standalone pipeline through a per-item flag view, but that runs
after requireJSONArray has validated the array, so an item spelling its payload
"values" or wrapping it in a {"cells": …} envelope died on the array schema
while the identical +batch-update sub-op was accepted. The rewrites move onto
the jsonFlagNormalizers seam for --writes, one step ahead of the schema, so the
two spellings of the same write agree. values → cells only when "cells" is
absent: two spellings with different payloads stays normalizeSubOpInputKeys'
conflict to report.

The derived selector is left as the only spelling of itself.
normalizeSubOpInputKeys keeps a duplicate key whose two values agree rather
than erroring, and two empty strings agree — so an input carrying both
"sheet-name":"" and "sheet_name":"" kept the hyphen form, which lookupRaw finds
first and which then shadowed the sheet_name just derived from the range
prefix, failing as "specify at least one of --sheet-id or --sheet-name".

Test coverage the review asked for: a +batch-update dry-run case for the prefix
rewrite (the sub-op path had unit coverage but no E2E), and the two tests that
grepped a rendered envelope now decode the dry-run body and assert the fields
that reach the wire.
The dry-run E2E pins what the CLI builds; nothing pinned that the backend
takes it. That gap matters more for rewrites than for ordinary flags: each one
turns a caller spelling into a wire payload the caller never sees, so a payload
the server rejects would be a worse outcome than the client-side error it
replaced.

TestSheets_CallCompatWorkflow writes through a sheet-qualified --range with no
selector flag at all, with bare scalars in the cell slots and a bare A1 acting
as an anchor — three rewrites composed in one call — then reads back through
the same prefix and stamps an openpyxl "hair" border over the result. The sheet
is named with a space in it so the prefix takes its quoted form, the spelling
the ref-lexer grammar exists for and the one a first-ASCII-"!" split would cut
in half.

The read-back compares values collected out of the decoded payload rather than
a fixed path: get_cell_ranges' response nesting is the backend's to change and
is pinned nowhere in this repo, while the values having survived the round trip
is the actual claim. The number is compared numerically for the same reason.

Self-contained: it builds its own workbook, and createSpreadsheet's cleanup
tears it down. Skips without tenant credentials, so local runs are unaffected
and CI's e2e-live job is what exercises it.
@chendaxin-tk

Copy link
Copy Markdown
Collaborator Author

Superseded by #2338 — same commits (8f5c6ccc), branch renamed feat/call-compatfeat/sheets-accept-caller-shapes.

The rename had to go through a new PR: GitHub's branch-rename API deleted the old head ref without retargeting this PR, so the review record lives here and the diff moves there. All four findings from the review above were fixed in 4784a4df (already in 8f5c6ccc), one nit declined on the record, 11/11 conversations resolved.

@chendaxin-tk
chendaxin-tk deleted the feat/call-compat branch August 13, 2026 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

domain/ccm PR touches the ccm domain size/L Large or sensitive change across domains or core paths

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants