feat(sheets): error prescription overhaul, batch-update hardening, and read offload - #2091
Open
xiongyuanwen-byted wants to merge 15 commits into
Open
feat(sheets): error prescription overhaul, batch-update hardening, and read offload#2091xiongyuanwen-byted wants to merge 15 commits into
xiongyuanwen-byted wants to merge 15 commits into
Conversation
为什么：豆包 Excel Agent 案例中模型 7 次参数错误，报错只说"错了"不说 "怎么改对"，模型反复试错并静默降级交付（5 张饼图丢数据标签）。 怎么改： - strict unexpected-property 报错附该节点合法 key 列表（cap 15 截断） + did-you-mean（复用 internal/suggest） - required-missing 报错附缺失字段的 type/description/enum 一行提示 - 深层 type mismatch 报错附该字段 enum/description 后缀 - +batch-update 顶层 --sheet-id/--sheet-name（含下划线拼写）特判， 直接指明 per-op locator 契约，不给误导性 fuzzy 建议 - batch sub-op input 拒收 cell_styles/styles/cell_merges 包裹结构， 报错教学扁平 flags 写法 - 守护测试锁定 wrappedSubOpInputKeys 与 batchOpDispatch 的互斥假设 约束：不改 legacy 报错措辞前缀、保留 --print-schema 指针、不动宽松 AdditionalProperties 设计（内嵌 schema 当前无 strict 节点，该路径为预置）。 验证：gofmt -l 无输出、go vet 干净、go test -count=1 ./shortcuts/sheets/ ./internal/suggest/ 全部通过。
## Background Round 2 of eval-driven sheets optimization, rebased onto the latest `feat/lark-sheets-develop` (`8897196d`). ## Changes - **feat(sheets): cut agent error rate and --help lookups (eval round 2)** — targets the top failure modes from round 2 evals, reducing agent error rate and the number of `--help` lookups. - **chore(sheets): sync skill docs and flag data from sheet-skill-spec** — syncs skill docs and flag data from sheet-skill-spec. ## Notes - During rebase, the "import mislabeled .xls workbooks by sniffing content" fix already existed on the target branch (identical patch-id), so it was auto-skipped — no duplicate. - The target branch was force-rewritten and advanced in the meantime; the two new commits were cleanly replayed onto the new tip via `--onto` with no conflicts. One hunk touching the `--type` description in `lark-sheets-workbook.md` was auto-dropped because upstream already has the same end state — no content lost.
Map the new `truncation` value in --include to include_truncation_info on the get_cell_ranges tool input, so +cells-get can return per-cell isRowTruncated / isColTruncated. Flag metadata and reference synced from sheet-skill-spec; flag_defs_gen.go regenerated.
…sv/table-get Reads are capped by max_chars (default 500000; the backend tool also truncates at ~50000 when unset). Add --output-path to +cells-get / +csv-get / +table-get: when set, the result is written to a cwd-relative path as JSON and the char cap is lifted to unbounded, so a large sheet lands on disk in full instead of being clipped for stdout. +table-get previously never sent max_chars, so it silently dropped rows past the backend ~50000 default with no signal. It now takes --max-chars (default 500000, sent explicitly) and surfaces truncated / truncation_warning when the read is clipped, steering callers to --output-path for a lossless full read.
为什么：fail-fast 单错报错导致「挤牙膏」修复回路——修一处、重试、 撞下一处。评测实测（turbo 三批 247 次失败）约 32% 的失败轮次是 挤牙膏，其中 local→local 类（39 次）本可一次报出。 怎么改： - validateAgainstSchema 重构为 collectSchemaErrors 收集器版：命中 错误后继续遍历，全部问题一次报出（每条带各自的教学信息） - translateBatchOperations 同步做 op 级聚合：多个坏 op 一条报错 编号列出，不再 fail-fast 在第一个 - 三条护栏：单错误输出逐字不变（向后兼容）；显示 cap 5 条 + 收集 到 6 即全树短路（病态大数组不爆炸）；类型错节点不下钻、oneOf 用一次性探测器（防级联噪音/误报泄漏） 验证：gofmt/vet 干净，go test -count=1 全过（既有测试零改动， 新增 5 个聚合测试：多错编号、cap 截断、oneOf 不泄漏、batch 双 op 聚合、单 op 保持原文）。
Feat/error schema hints
…ceptance (#2028) * feat(sheets): harden +batch-update sub-op contract (P0-1/2/3/5) Eval-driven fixes for the top +batch-update error clusters (137 errors across 7 eval batches, attribution in the optimization plan doc): - Reject unknown sub-op input keys with did-you-mean + full key contract instead of silently ignoring them (silent ignore surfaced as misleading 'missing required flag' errors — the largest cluster, ~35 hits). Habitual spellings are rewritten in place: camelCase -> snake_case, commandFlagAliases (new: size -> width/height on the resize pair, the pre-July vocabulary and the --styles protocol spelling, 15+ hits), single-entry ranges unwraps onto range. - Aggregate per-op validation errors into one pass (each op's first error) instead of fail-fast first-error-only; single-error batches keep the standalone-shaped error (contract tests unchanged). - Precheck cells matrix vs range locally: empty cells (prescribes +cells-clear) and row/column count mismatches no longer reach the server mid-batch. - Correct the atomicity story: execution is fail-fast without rollback (verified against live batches; docs promised a rollback that does not happen). Partial-failure errors now spell out that succeeded operations stay applied and prescribe resending only the failed tail, preventing double-apply on retry. * feat(cli): accept @file payload reads from the system temp dir (P0-6) Agents stage generated payloads (batch operations JSON, CSV) in /tmp as a matter of course; the relative-to-cwd-only policy pushed every --operations @/tmp/ops.json through an extra python/stdin round trip (recurring friction cluster in eval traces). Scope is deliberately narrow: a new SafeTempAbsInputPath accepts an absolute READ path only when it resolves (symlinks included) under the canonical os.TempDir(), and only the @file expansion in cmdutil.ReadInputFile uses it. SafeInputPath stays strict — uploads, drive sync and the CI quality gates treat 'absolute paths rejected' as a load-bearing invariant. Absolute paths outside the temp dir now get a prescriptive error naming all three options (relative path, temp-dir path, stdin). * feat(sheets): add +styles-put, +dim-delete --ranges, freeze in --styles Batch-B of the +batch-update overhaul (attribution: ~73% of real batch calls were pure formatting finishers hand-built as operations arrays). - +styles-put: declarative visual spec for existing spreadsheets. Reuses the workbook-create/table-put --styles parser (identical vocabulary and aggregate-all-issues errors), expands client-side into ONE atomic batch_update per spec: cell_merges -> cell_styles -> row_sizes -> col_sizes -> freeze. Style stamps are safe to re-run. Verified live: 6/6 sub-ops applied, frozen rows / merges / row heights confirmed by read-back. - freeze section added to the shared --styles pipeline ({rows, cols}), so +workbook-create and +table-put gain it too — closes the one gap that still forced a separate +dim-freeze call. - +dim-delete --ranges: scattered row/column ranges in one atomic batch, ordered DESCENDING so earlier deletions never shift later indexes (the recurring failure of hand-built dim-delete batches); same-dimension and non-overlap enforced, nesting inside +batch-update rejected with a prescription. - +cells-batch-set-style enters phase-1 deprecation: kept working, docs point at +styles-put, an in-band note steers new usage. Skill docs regenerated from sheet-skill-spec (new lark-sheets-styles-put reference, three-way dispatch in guideline 6, fail-fast-no-rollback wording). * feat(sheets): accept height/width as one-way aliases for size in --styles row/col_sizes size stays the canonical dimension key: it keeps row_sizes and col_sizes items shape-uniform (the array name already carries the dimension), it is what shipped with workbook-create/table-put and what models demonstrably converge on, and it matches the dimension-neutral precedent of comparable APIs. The Excel-vocabulary words are accepted silently only where unambiguous — height inside row_sizes, width inside col_sizes; the wrong dimension's word gets a targeted error instead of a rewrite, and giving both size and the alias is rejected. Shared parser, so +workbook-create / +table-put / +styles-put all gain it. * chore(sheets): sync skill docs — +cells-batch-set-style fully exits the skill surface Regenerated from sheet-skill-spec: the deprecated command no longer appears anywhere in SKILL.md or the references (multi-range styling routes to +styles-put); its flag-defs entry is untouched, so the command and --help keep working for compatibility callers, with the in-band supersedence note steering them to +styles-put. * fix(sheets): forgive habitual vocabulary on the --styles payload path 07-20 rerun attribution: the batch-update dispatch worked (calls 105->25, errors 21->8; +styles-put adopted by 16/35 tasks) but +styles-put itself hit a 56% stateful error rate — the redesign moved traffic from the flag path onto the payload path, and the round-2 forgiveness layers (key aliases, enum-value canonicalization) only existed on the flag path. Both dominant clusters are fixed in the shared styles pipeline, so +styles-put / +table-put / +workbook-create / typed --cells all gain it: - border family folding (largest cluster, up to 88 issues in one retry): borders/border objects, border_top..right objects, border_style/color/ weight scalars, and flattened border_<side>_<attr> keys all fold into the canonical nested border_styles; border_style:"thin" reads as a thin solid line (weight vocabulary in the style slot). - enum VALUE canonicalization inside cell_styles (~10 server-side round trips: vertical_alignment "center" -> "middle"), sourced from the +cells-set-style flag enums; off-enum values now fail client-side with a did-you-mean instead of failing the whole batch server-side. - wrap family: wrap_text/text_wrap -> word_wrap, boolean -> enum. - bare-string cell_merges entries read as {range, merge_type:all}. - fore_color gets a prescription (openpyxl fgColor is the FILL color; a silent pick could color the wrong thing). Replayed the eval-failing payload shapes live: 2/2 applied. * feat(sheets): resize type optional in --styles; acceptance-surface contract tests - {range, size} in row/col_sizes now means a pixel resize (type stays for standard/auto) — the payload path matches the flag path, where --width never required --type. - Two closure tests turn the --styles acceptance surface into a locked contract instead of open-ended patching: * vocabulary parity — every +cells-set-style flag (iterated from flag-defs) must be accepted verbatim by the payload path, so a future flag can never again ship without payload-path support; * prior corpus — every model spelling observed across the 07-08..07-20 eval batches must either normalize to canonical or produce a targeted prescription; silent ignoring and bare rejection both fail the suite. New eval finding -> add a corpus row -> fix -> locked. Skill docs regenerated: the border shorthand ({style,weight,color} on all four sides) is now the teaching form, border_styles demoted to per-side differences; resize examples drop the type ceremony. * fix(sheets): close the 07-21 rerun residuals — full-form thin, range coalescing, typed-cells style key Valid rerun (skill injection verified at ~31k chars/task): +styles-put stateful error rate fell 56.5% -> 34.3% and batch-update stayed at its post-dispatch low. Three residual clusters, all closed: - weight vocabulary in the FULL nested form's style slot (border_styles.<side>.style:"thin" — 8 tasks, the dominant residual; the earlier rewrite only covered the shorthand scalar path). Now normalized in expandBorderAllShorthand, the single border touchpoint shared by the flag, typed-cells and styles-payload paths. - per-row specs blowing the 100-op cap (184/203/861-op expansions): coalesceStyleStamps fuses identical-style entries into rectangles (vertical fixpoint merge on same column span, horizontal on same row span) before the cap — a declarative spec describes intent, execution shape is the CLI's to optimize. Cap message now also routes alternating-row banding to +cond-format-create. - typed --cells habitual keys (recurring server-side 900015206 in both reruns): cells[][].style object rewrites to cell_styles; cells[][].type gets a prescription instead of a server round trip. The content-in-styles message is also neutral now (was workbook-create-specific). Corpus + coalescing + typed-cells tests added; live replay: full-form thin accepted and 3 same-style rows fused, all applied. * refactor(sheets): give the style-vocabulary acceptance layer its own home Pure mechanical move, zero behavior change (locked by the acceptance contract tests). The acceptance layer had grown as an accretion across helpers.go and lark_sheet_workbook.go — deliberate design (one canonical form + wide acceptance, per the divergent-priors evidence), accidental placement. style_vocab.go now holds the whole subsystem — flag-path style builders, key aliases, enum-value canonicalization, border folding/normalization, typed-cells cell-object rewrites — under a header that states the design contract: rewrites must be unambiguous, ambiguity prescribes, silence and bare rejection are both bugs, closure is enforced by the parity + prior-corpus tests. helpers.go shrinks back to generic plumbing (818 -> 542 lines). The other two acceptance surfaces keep their own homes: cobra flag ergonomics in flag_ergonomics.go, batch sub-op key vocabulary in batch_op_dispatch.go. * fix(sheets): enforce the cells-vs-range match on single-cell ranges too The precheck deliberately skipped bare single-cell ranges when server behavior was unverified; the 07-21 rerun supplied the evidence (12 rows against range "A1" failing server-side with row count 1) — +cells-set has no anchor semantics, the strict match applies everywhere. Corpus updated accordingly. * feat(sheets): make +csv-get --range optional — omitted reads the whole sheet The tool requires a range but clips past-grid references and reports the clip in actual_range, so the CLI sends an over-wide whole-columns range (A:ZZZ) when --range is omitted: one call reads the entire sheet, no workbook-info pre-flight to size it first. Eval evidence: 'required flag(s) range not set' was the most-missed required flag on +csv-get (4 tasks in the 07-21 batch) — the models' intent was always 'read it all'. Blank --range now means the same as omitting it. Skill docs updated (quick-reference row, read-data full-read example, flag desc); round-3 backlog item A5. * feat(sheets): add editing rule #10 — never fabricate missing values 补齐 / 扩展 / 按原表格式续填时，查不到或无法确定的值一律留空 + 备注注明，禁止用推算 / 估算 / 凭空数据充数；原表已示范缺失值写法时照抄。 Synced from sheet-skill-spec (SoT). * fix(sheets): accept side-first border word order and wrap_strategy 07-21 evening batch (first run carrying the previous fixes — thin full-form / style/type keys / csv-get range all at zero): the corpus loop caught the next spelling permutations. bottom_border / bottom_border_style (side-first word order, alongside the border_bottom family already folded) and wrap_strategy (the Google Sheets API word) now normalize; three corpus rows lock them. * feat(sheets): universal did-you-mean on unknown style fields; formalize the silent-alias admission bar The alias table was drifting toward per-permutation entries with fast- falling marginal value (first aliases covered 15+ errors each, the last ones 1-2). The asymmetry that matters: rejection is universal, aliases are per-word. So: - unknown style fields now reject with did-you-mean + the full canonical field list (this error had neither — the actual reason word-order permutations turned into multi-issue retry loops). Any future permutation costs one self-healing retry, zero new code, and corrects the whole session (silent aliases never correct the model in-session). - the acceptance-layer contract now states the admission bar for silent aliases: real external vocabularies only (Excel/openpyxl, CSS, Google Sheets API — a finite set), recurring across batches or ≥3 tasks, zero ambiguity. Permutations go to the universal rejection. Existing permutation aliases are grandfathered. * feat(sheets): +cells-set --writes — scattered multi-region writes in one atomic call The last compressible batch-update scenario: eval traces show 'fix all broken formulas across ranges/sheets' (~6 calls / 3 tasks per batch) still hand-assembled as +batch-update operations arrays. --writes takes [{sheet_name|sheet_id, range, cells}, ...] (up to 100 items, cross-sheet) and fans it into ONE atomic batch_update of set_cell_range ops. Design decisions: - the sheet selector LIVES IN EACH ITEM — no top-level fallback, no precedence table; same convention models already learned from +batch-update sub-ops and +styles-put items. A top-level --sheet-name/--sheet-id with --writes is rejected with the fix. - every item runs the exact standalone pipeline via a per-item flag view: key vocabulary (camelCase, aliases, did-you-mean), the style acceptance layer for inline cell_styles, matrix precheck, schema validation; item errors aggregate so one retry fixes all. - XOR with --range/--cells/--copy-to-range; top-level --allow-overwrite propagates to items that don't override it. - nesting inside +batch-update rejected (expands into its own batch). - predictable prior handled: --styles on +cells-set now hints the layering (range-level styling -> +styles-put; per-cell styles ride in the cells objects) instead of a bare unknown-flag error. Live smoke: value + formula regions in one call, 2/2 applied. Expected effect: batch-update calls drop another ~30% to its heterogeneous-atomic-chain steady state. * chore(sheets): bump lark-sheets skill version to 3.1.0 Version roll-up for the batch-update optimization series: cells-set --writes, universal did-you-mean on style fields, border word-order and wrap_strategy acceptance, editing rule #10, and optional +csv-get --range.
…edundant none The flag name was inverted relative to actual behavior, and `none` was redundant. - Map --inherit-style onto the modify_sheet_structure backend so the name matches behavior (verified on a live sheet): `before` inherits the preceding row/column (side=after, position-1); `after` inherits the following (side=before). Insertion always lands before --position. - Warn only when `before` is used at the first row/column (no preceding dimension to copy from); `after` has no such edge. - Drop the `none` enum value: it was identical to omitting the flag and misleadingly implied "no inheritance" (the backend always copies a neighbour). Omitting the flag inherits the following row/column; `none` is now rejected by enum validation. Clear formats afterwards for a blank one. - Regenerate flag-defs, update tests and the skill reference.
…e flag and style-field fixes
Three fixes from the 07-28 root-cause analysis of failed agent traces, all
aimed at the first-try success rate rather than the recovery loop.
Border acceptance layer was unreachable on two of its three carrier paths.
expandBorderAllShorthand already moves a weight word out of the style slot
({"style":"thin"} -> style solid + weight thin), but on --border-styles and
typed --cells it ran after parseJSONFlag's schema check, so the enum error
fired first and the rewrite never happened. Move it ahead of validation via
the jsonFlagNormalizers seam; --styles already validated post-expansion and
is unchanged. An explicitly conflicting weight still takes the enum error.
Unknown-flag prescriptions for the names agents reach for most: +cells-set
--values, +dim-freeze --frozen-row-count and siblings, +cells-set-style
--font-bold / --bg-color / --wrap-strategy and the whole --border-* family
(no such flags; borders take one composite --border-styles). +sheet-rename
--new-name / --name alias to --title, matching +sheet-create.
Unsupported cell_styles field names now name the right field instead of the
nearest string: bold / font_bold -> font_weight, text_align ->
horizontal_alignment, a nested openpyxl-style font object -> the flat font_*
fields. Where no prescription applies, the edit-distance fallback is capped
at two edits, so a concept-swap neighbour (font_bold -> font_color, three
edits) stays silent rather than sending the retry the wrong way; a curated
prescription also drops the contradicting machine-readable suggestions.
…velope in errors and help
Second batch from the 07-28 root-cause analysis, all aimed at the retry that
follows a rejection.
Enum-bearing type mismatches now answer with the allowed values instead of a
whole-payload skeleton. --border-styles with weight:1 used to reply "expected
type string, got number; expected shape: {"bottom": {…}, "left": {…}, …}",
which never mentions thin/medium/thick; the skeleton is for container-shape
confusion, so a field that declares an enum falls through to the hint that
names it. The --border-styles help now inlines both vocabularies (style is the
line type, weight the thickness and a string, not a pixel number), spells the
{all:{…}} shorthand, and states that no --border-all / --border-top /
--border-color exist. --word-wrap additionally accepts the Google Sheets
wrapStrategy words wrap and clip.
The sheet selector states that one of the pair is required rather than only
that they are mutually exclusive, in help across all shortcuts that take it,
and the rejection hints where the name comes from: a fresh workbook has one
sheet named Sheet1, any other needs a +workbook-info lookup. Eval traces
recover on the very next call, so the gap was which name to pass.
A --sheets payload written as a bare array now says the top level must be
{"sheets":[…]} instead of quoting Go's "cannot unmarshal array into Go value
of type struct { Sheets []sheets.tableSheetIn }", which names the internal
type rather than the fix. The skeleton hint is unchanged.
…spec Mirrors `npm run sync:cli` output from the spec repo, which is the source of truth for skill docs and flag data. Two independent changes ride along: - The border and sheet-selector flag descriptions from spec commit 3dd7f9c, matching the help text already committed here in 0b59556 (data/flag-defs.json is byte-identical, so `go generate` is a no-op). - The upstream read-flow work: SKILL.md 3.1.0 to 3.1.1, a longer read-data reference, and five read-side helper scripts under skills/lark-sheets/scripts. The scripts land as machine resources and are not embedded in the binary (content_embed.go whitelists docs only). make unit-test passes.
…into the sheet A +csv-put --csv value naming a file that doesn't resolve used to be written into the anchor cell as literal text, with a success exit code — a wrong value in the sheet that nothing surfaces, which costs more than a rejection. The common source is an absolute path: @ only reads relative paths, so the caller drops the @ and retries, and the path string lands in A1 (07-28 root-cause audit, finding D1). The existing guard only caught values naming a file that does exist (the forgotten-@ case). The guard now also rejects a value that is unmistakably path-shaped: no separator or whitespace, pure ASCII, and either a .csv/.tsv extension or an explicit ./ ../ / ~/ prefix. All three conditions are required — that is what keeps prose that merely mentions a filename, N/A, README.md and CJK content out of it, the misjudgments that retired the previous name-shape heuristic. This flips one pinned case: a bare "nope.csv" was previously written verbatim and now errs with the fix inlined. To make the shape check safe for correct invocations, resolveInputFlags now records which flags had their value replaced from @file or stdin, exposed as RuntimeContext.InputResolvedFromSource; the guard skips resolved values entirely. By Validate time a piped value is indistinguishable from a typed one, so without the origin bit the guard would re-reject a correct `--csv @file` whose content happens to look like a path — and stdin, which the error text prescribes for verbatim writes, would not actually escape it. The @@ escape stays inline and guarded. This origin bit is the one common-layer addition; it carries no domain logic.
# Conflicts: # internal/validate/path.go # internal/vfs/localfileio/path.go
github-actions
Bot
added
domain/ccm
📝 WalkthroughWalkthroughThis PR adds safer confirmation and path handling, expands Sheets batch, style, read, schema, and flag behavior, introduces ChangesCLI infrastructure
Read-only analysis scripts
Estimated code review effort: 5 (Critical) | ~150 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant CellsSet
participant Validator
participant BatchUpdate
Caller->>CellsSet: Submit --writes payload
CellsSet->>Validator: Normalize and validate write items
Validator-->>CellsSet: Return set_cell_range operations
CellsSet->>BatchUpdate: Submit one batch_update request
BatchUpdate-->>Caller: Return result or validation error
sequenceDiagram
participant Caller
participant StylesPut
participant StyleParser
participant BatchUpdate
Caller->>StylesPut: Submit --styles payload
StylesPut->>StyleParser: Parse and normalize style items
StyleParser-->>StylesPut: Return ordered operations
StylesPut->>BatchUpdate: Submit expanded batch_update request
BatchUpdate-->>Caller: Return execution result
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2091 +/- ##
==========================================
+ Coverage 75.16% 75.22% +0.05%
==========================================
Files 912 916 +4
Lines 96475 97723 +1248
==========================================
+ Hits 72517 73508 +991
- Misses 18381 18563 +182
- Partials 5577 5652 +75 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@a5032bbb55e1e90aef66311eaae20dc975ba9f70🧩 Skill updatenpx skills add larksuite/cli#feat/lark-sheets-develop -y -g |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (8)
skills/lark-sheets/scripts/lark_detect_subtables.py (1)
486-488: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePass
strict=Falseto the offsetzip.Ruff B905 flags the implicit default; the pairing here is intentionally ragged, so make that explicit.
♻️ Proposed tweak
- for previous, current in zip(grid.row_numbers, grid.row_numbers[1:]): + for previous, current in zip(grid.row_numbers, grid.row_numbers[1:], strict=False):🤖 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 `@skills/lark-sheets/scripts/lark_detect_subtables.py` around lines 486 - 488, Update the adjacent-row construction loop to call zip with strict=False, making the intentionally ragged pairing explicit and resolving Ruff B905 while preserving the existing previous/current mapping behavior.Source: Linters/SAST tools
skills/lark-sheets/scripts/lark_profile_table.py (1)
130-138: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the
zippairing explicit.
row_numbersandvaluesare built in lockstep byparse_annotated_csv, sostrict=Trueboth silences Ruff B905 and asserts that invariant.♻️ Proposed tweak
- for row_num, row in zip(grid.row_numbers, grid.values) + for row_num, row in zip(grid.row_numbers, grid.values, strict=True)🤖 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 `@skills/lark-sheets/scripts/lark_profile_table.py` around lines 130 - 138, Update the zip call in the first_non_empty computation to pass strict=True, explicitly asserting the lockstep relationship between grid.row_numbers and grid.values while preserving the existing filtering and return behavior.Source: Linters/SAST tools
skills/lark-sheets/scripts/lark_inspect_workbook.py (1)
81-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winValidate numeric args before the first network call.
--max-sheetsis checked only after+workbook-infohas already run, and--preview-rowsisn't checked at all —--preview-rows 0buildsA1:<col>0, which only fails once+csv-getrejects it remotely. Validating both (plus--max-chars) at the top ofinspect_workbookgives a local, actionable error.♻️ Proposed reordering
def inspect_workbook(args) -> tuple[dict[str, Any], list[str]]: warnings: list[str] = [] + if args.max_sheets < 1: + raise LarkCliError("--max-sheets must be at least 1") + if args.preview_rows < 1: + raise LarkCliError("--preview-rows must be at least 1") workbook = envelope_data( @@ - if args.max_sheets < 1: - raise LarkCliError("--max-sheets must be at least 1") inspect_count = len(target_sheets)🤖 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 `@skills/lark-sheets/scripts/lark_inspect_workbook.py` around lines 81 - 108, Validate --max-sheets, --preview-rows, and --max-chars at the beginning of inspect_workbook, before any workbook-info or other network calls. Reject values below their supported minimums with local, actionable LarkCliError messages, while preserving the existing later inspection behavior for valid arguments.skills/lark-sheets/scripts/lark_sheet_read_cli.py (1)
116-130: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAnnotate
emit_errorasNoReturn.
emit_erroralways terminates, but its-> Nonesignature makes every caller's follow-upemit_success(ACTION, data, warnings)look like it uses a possibly-unbounddata(seelark_detect_subtables.pyLine 534-535,lark_inspect_workbook.pyLine 163-164,lark_profile_table.pyLine 513-514).NoReturnmakes the control flow explicit to readers and type checkers.♻️ Proposed annotation
-def emit_error(action: str, message: str, warnings: list[str] | None = None) -> None: +def emit_error(action: str, message: str, warnings: list[str] | None = None) -> NoReturn:Add
from typing import Any, NoReturnto the imports on Line 8.🤖 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 `@skills/lark-sheets/scripts/lark_sheet_read_cli.py` around lines 116 - 130, Update the `emit_error` function annotation to return `NoReturn` instead of `None`, and import `NoReturn` from `typing` alongside the existing typing imports. Preserve its current error output and `sys.exit(1)` behavior.shortcuts/sheets/flag_schema.go (1)
228-242: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
schemaChildKeyscan under-report keys thatschemaChildwould actually resolve.
schemaChildfalls back tooneOfbranches whenpropertiesexists but lacks the segment;schemaChildKeysreturns as soon aspropertiesis found, so for a node carrying bothpropertiesandoneOfthe "available keys" list omits resolvable branch keys.♻️ Proposed tweak
if props, ok := m["properties"].(map[string]interface{}); ok { for k := range props { seen[k] = struct{}{} } - return } if items, ok := m["items"]; ok { collect(items, depth+1) return } if branches, ok := m["oneOf"].([]interface{}); ok { for _, b := range branches { collect(b, depth+1) } }🤖 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/sheets/flag_schema.go` around lines 228 - 242, Update schemaChildKeys so finding a properties map does not return before collecting keys from oneOf branches; preserve property-key collection and then recurse through oneOf when present, matching schemaChild’s fallback resolution behavior.shortcuts/sheets/flag_schema_validate_test.go (1)
673-780: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert typed metadata and cause preservation on these
validateValueAgainstSchemaerror paths.These four tests exercise the typed wrapper (
sheetsValidationForFlag(...).WithCause(vErr)) but only match message substrings, so a regression that drops the--cellsparam, changes the subtype, or loses the wrapped cause would still pass. The file already hasrequireValidationfor this; add a cause check too (e.g.errors.Ason the unwrapped*typeMismatchErrorfor the deep-mismatch case).As per coding guidelines: "Error-path tests must assert typed metadata through
errs.ProblemOf(category,subtype, andparam) and verify cause preservation rather than relying only on message substrings."🤖 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/sheets/flag_schema_validate_test.go` around lines 673 - 780, Strengthen the four validateValueAgainstSchema tests to assert typed error metadata, not only message text. Use the existing requireValidation helper and errs.ProblemOf to verify category, subtype, and the --cells parameter for each error path. Also verify cause preservation by unwrapping the returned error and using errors.As to locate the underlying validation error, including *typeMismatchError for the deep-mismatch case.Source: Coding guidelines
shortcuts/sheets/flag_ergonomics_test.go (1)
494-606: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAll cases here use hyphenated flag spellings, so this suite doesn't exercise the underscore-variant gap noted in
flag_ergonomics.go(Lines 204-216) — worth a follow-up case (e.g.--new_sheet_name) once that's fixed, but not a defect in this test as written.🤖 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/sheets/flag_ergonomics_test.go` around lines 494 - 606, Extend TestShortcuts_IntuitiveFlagHints with a follow-up case using an underscore-form flag such as --new_sheet_name, once the underscore-variant handling in flag_ergonomics.go is fixed. Assert the same curated hint and absence of contradictory edit-distance suggestions as the corresponding hyphenated case.shortcuts/sheets/helpers.go (1)
462-501: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
colorThemebare-hex arrays are not covered by the normalizer.
style.colorThemeis documented in the schema as an array of#RRGGBBstrings, but the walk only rewrites string values under color-keys; array elements ("colorTheme": ["4472C4", "ED7D31"]) fall through untouched and will hit the same server-side rejection the normalizer exists to prevent. Consider extending the array branch to rewrite bare-hex strings when the enclosing key is a color key.🤖 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/sheets/helpers.go` around lines 462 - 501, Extend normalizeChartHexColors to track whether the current value is under a color key, and in the []interface{} branch prefix bare-hex string elements when that context applies. Preserve recursive traversal and leave non-color arrays and non-bare-hex values unchanged.
🤖 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 `@internal/cmdutil/confirm_test.go`:
- Around line 38-42: Strengthen confirmation contract coverage by extracting the
argument-dependent logic from RequireConfirmation into a testable helper such as
requireConfirmationFor(action string, args []string). Update the tests to assert
the complete “add --yes to confirm; re-run: …” hint for fixed arguments and the
plain hint when stdin arguments are supplied, so removing the retry branch fails
the tests.
In `@internal/cmdutil/confirm.go`:
- Around line 29-36: Update RequireConfirmation and the retry-command rendering
path to sanitize os.Args before embedding them in the WithHint message. Redact
values for sensitive flags such as --app-secret, --app-token, and --token in
both --flag value and --flag=value forms, while preserving non-sensitive
arguments and the existing retry behavior.
In `@internal/cmdutil/resolve.go`:
- Around line 88-95: Replace the os.ReadFile call in the resolved-path branch of
the input-reading function with internal/vfs.ReadFile, preserving the existing
error wrapping via wrapInputFileError and the SafeTempAbsInputPath validation
flow so injected or mocked filesystems can intercept the read.
In `@internal/vfs/localfileio/path.go`:
- Around line 152-170: Update absPathUnderTempDir to reject degenerate canonical
temp directories before resolving or accepting the input path, including when
filepath.EvalSymlinks(os.TempDir()) resolves to the filesystem root or otherwise
lacks a meaningful parent boundary. Preserve the existing canonical symlink
resolution and isUnderDir checks for valid, non-root temporary directories.
In `@shortcuts/sheets/batch_op_dispatch.go`:
- Around line 401-431: Guard the camelCase-to-kebab rewrite in the
key-normalization logic and the ranges-to-range unwrap branch against existing
canonical keys before assigning. Reuse the alias branch’s collision checks for
both the hyphenated and underscore forms, and return the established typed
validation error when a requested rewrite would overwrite an existing value;
preserve current rewrites when no canonical spelling is present.
In `@shortcuts/sheets/chart_examples_test.go`:
- Around line 34-42: Update the “unknown type lists available” test to assert
that the returned ValidationError’s Param equals "--print-example". Keep the
existing message-content assertion unchanged, using the ve value returned by
requireValidation.
In `@shortcuts/sheets/flag_ergonomics.go`:
- Around line 204-216: Normalize the parsed flag name by converting underscores
to hyphens before looking it up in intuitiveFlagHints within the curated hint
branch. Use the normalized name for the lookup while preserving the existing
hint, valid-flag list, and suggestions-clearing behavior, so underscore-spelled
habitual flags resolve to the same prescriptions as their hyphenated forms.
In `@shortcuts/sheets/read_output.go`:
- Around line 34-41: Honor an explicitly supplied max-chars value in
maxCharsInput instead of always returning the unbounded sentinel when
output-path is set; use the unbounded default only when max-chars was not
explicitly supplied, or reject the combination with typed validation. In
shortcuts/sheets/lark_sheet_read_data_test.go lines 37-52, add dry-run
assertions confirming explicit caps survive output-path handling. Update
skills/lark-sheets/references/lark-sheets-read-data.md lines 164-166, 183-185,
and 198-199 to document the chosen precedence or validation error.
In `@skills/lark-sheets/references/lark-sheets-chart.md`:
- Line 30: Update the +chart-create Flags table to include --print-example,
documenting that it accepts column, bar, line, area, pie, scatter, radar, or
combo and only prints the corresponding minimal template without creating a
chart. Keep the existing chart-create flag descriptions unchanged.
In `@skills/lark-sheets/references/lark-sheets-read-data.md`:
- Around line 93-94: Insert a blank line between the final table row and the
following list item in the markdown content, preserving the table and list text
unchanged.
In `@skills/lark-sheets/references/lark-sheets-styles-put.md`:
- Line 19: Update the freeze documentation in
skills/lark-sheets/references/lark-sheets-styles-put.md at lines 19-19 and
skills/lark-sheets/references/lark-sheets-workbook.md at lines 202-202 to state
that at least one of rows or cols must be greater than zero, while preserving
that zero or omission disables the respective dimension. Also update the
matching schema text at line 50 of the styles-put reference if it is regenerated
from the same source.
In `@tests/cli_e2e/sheets/sheets_sheet_shortcuts_dryrun_test.go`:
- Around line 258-290: Add live E2E tests alongside
TestSheets_DimInsertDryRunInheritAfterKeepsBeforePosition covering +dim-insert
and +dim-delete, including --inherit-style variants and +dim-delete --ranges.
Create the required spreadsheet/sheet data, execute the shortcuts with bot
credentials where applicable, assert the resulting sheet structure and styles,
and clean up all created resources; retain the existing dry-run payload test.
---
Nitpick comments:
In `@shortcuts/sheets/flag_ergonomics_test.go`:
- Around line 494-606: Extend TestShortcuts_IntuitiveFlagHints with a follow-up
case using an underscore-form flag such as --new_sheet_name, once the
underscore-variant handling in flag_ergonomics.go is fixed. Assert the same
curated hint and absence of contradictory edit-distance suggestions as the
corresponding hyphenated case.
In `@shortcuts/sheets/flag_schema_validate_test.go`:
- Around line 673-780: Strengthen the four validateValueAgainstSchema tests to
assert typed error metadata, not only message text. Use the existing
requireValidation helper and errs.ProblemOf to verify category, subtype, and the
--cells parameter for each error path. Also verify cause preservation by
unwrapping the returned error and using errors.As to locate the underlying
validation error, including *typeMismatchError for the deep-mismatch case.
In `@shortcuts/sheets/flag_schema.go`:
- Around line 228-242: Update schemaChildKeys so finding a properties map does
not return before collecting keys from oneOf branches; preserve property-key
collection and then recurse through oneOf when present, matching schemaChild’s
fallback resolution behavior.
In `@shortcuts/sheets/helpers.go`:
- Around line 462-501: Extend normalizeChartHexColors to track whether the
current value is under a color key, and in the []interface{} branch prefix
bare-hex string elements when that context applies. Preserve recursive traversal
and leave non-color arrays and non-bare-hex values unchanged.
In `@skills/lark-sheets/scripts/lark_detect_subtables.py`:
- Around line 486-488: Update the adjacent-row construction loop to call zip
with strict=False, making the intentionally ragged pairing explicit and
resolving Ruff B905 while preserving the existing previous/current mapping
behavior.
In `@skills/lark-sheets/scripts/lark_inspect_workbook.py`:
- Around line 81-108: Validate --max-sheets, --preview-rows, and --max-chars at
the beginning of inspect_workbook, before any workbook-info or other network
calls. Reject values below their supported minimums with local, actionable
LarkCliError messages, while preserving the existing later inspection behavior
for valid arguments.
In `@skills/lark-sheets/scripts/lark_profile_table.py`:
- Around line 130-138: Update the zip call in the first_non_empty computation to
pass strict=True, explicitly asserting the lockstep relationship between
grid.row_numbers and grid.values while preserving the existing filtering and
return behavior.
In `@skills/lark-sheets/scripts/lark_sheet_read_cli.py`:
- Around line 116-130: Update the `emit_error` function annotation to return
`NoReturn` instead of `None`, and import `NoReturn` from `typing` alongside the
existing typing imports. Preserve its current error output and `sys.exit(1)`
behavior.
🪄 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 Plus
Run ID: 4219a54f-8599-4e64-84d4-fce10bfbdf25
📒 Files selected for processing (62)
internal/cmdutil/confirm.gointernal/cmdutil/confirm_test.gointernal/cmdutil/resolve.gointernal/validate/path.gointernal/vfs/localfileio/path.gointernal/vfs/localfileio/path_test.goshortcuts/common/runner.goshortcuts/common/runner_input_test.goshortcuts/common/testing.goshortcuts/sheets/batch_key_vocab_test.goshortcuts/sheets/batch_op_contract_test.goshortcuts/sheets/batch_op_dispatch.goshortcuts/sheets/cells_set_writes_test.goshortcuts/sheets/chart_examples.goshortcuts/sheets/chart_examples_test.goshortcuts/sheets/csv_put_guard_test.goshortcuts/sheets/data/flag-defs.jsonshortcuts/sheets/data/flag-schemas.jsonshortcuts/sheets/flag_defs_gen.goshortcuts/sheets/flag_defs_test.goshortcuts/sheets/flag_ergonomics.goshortcuts/sheets/flag_ergonomics_test.goshortcuts/sheets/flag_schema.goshortcuts/sheets/flag_schema_validate.goshortcuts/sheets/flag_schema_validate_test.goshortcuts/sheets/flag_schemas_gen.goshortcuts/sheets/helpers.goshortcuts/sheets/helpers_test.goshortcuts/sheets/json_flag_normalize_test.goshortcuts/sheets/lark_sheet_batch_update.goshortcuts/sheets/lark_sheet_batch_update_test.goshortcuts/sheets/lark_sheet_range_operations.goshortcuts/sheets/lark_sheet_read_data.goshortcuts/sheets/lark_sheet_read_data_test.goshortcuts/sheets/lark_sheet_sheet_structure.goshortcuts/sheets/lark_sheet_sheet_structure_test.goshortcuts/sheets/lark_sheet_styles_put.goshortcuts/sheets/lark_sheet_styles_put_test.goshortcuts/sheets/lark_sheet_table_io.goshortcuts/sheets/lark_sheet_workbook.goshortcuts/sheets/lark_sheet_write_cells.goshortcuts/sheets/read_output.goshortcuts/sheets/sheet_ai_api.goshortcuts/sheets/sheet_ai_api_flatten_test.goshortcuts/sheets/shortcuts.goshortcuts/sheets/style_vocab.goshortcuts/sheets/styles_acceptance_test.goshortcuts/sheets/styles_prescription_test.goskills/lark-sheets/SKILL.mdskills/lark-sheets/references/lark-sheets-batch-update.mdskills/lark-sheets/references/lark-sheets-chart.mdskills/lark-sheets/references/lark-sheets-read-data.mdskills/lark-sheets/references/lark-sheets-sheet-structure.mdskills/lark-sheets/references/lark-sheets-styles-put.mdskills/lark-sheets/references/lark-sheets-workbook.mdskills/lark-sheets/references/lark-sheets-write-cells.mdskills/lark-sheets/scripts/lark_detect_subtables.pyskills/lark-sheets/scripts/lark_inspect_workbook.pyskills/lark-sheets/scripts/lark_profile_table.pyskills/lark-sheets/scripts/lark_sheet_range.pyskills/lark-sheets/scripts/lark_sheet_read_cli.pytests/cli_e2e/sheets/sheets_sheet_shortcuts_dryrun_test.go
Comment on lines
+258
to
+290
|
|
||
| func TestSheets_DimInsertDryRunInheritAfterKeepsBeforePosition(t *testing.T) { | ||
| setSheetsDryRunEnv(t) | ||
|
|
||
| ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) | ||
| t.Cleanup(cancel) | ||
|
|
||
| result, err := clie2e.RunCmd(ctx, clie2e.Request{ | ||
| Args: []string{ | ||
| "sheets", "+dim-insert", | ||
| "--spreadsheet-token", "shtDryRun", | ||
| "--sheet-id", "sheet1", | ||
| "--position", "D", | ||
| "--count", "1", | ||
| "--inherit-style", "after", | ||
| "--dry-run", | ||
| }, | ||
| DefaultAs: "user", | ||
| }) | ||
| require.NoError(t, err) | ||
| result.AssertExitCode(t, 0) | ||
|
|
||
| out := result.Stdout | ||
| require.Equal(t, "/open-apis/sheet_ai/v2/spreadsheets/shtDryRun/tools/invoke_write", clie2e.DryRunGet(out, "api.0.url").String(), "stdout:\n%s", out) | ||
| require.Equal(t, "modify_sheet_structure", clie2e.DryRunGet(out, "tool_name").String(), "stdout:\n%s", out) | ||
| require.Equal(t, "insert", clie2e.DryRunGet(out, "tool_input.operation").String(), "stdout:\n%s", out) | ||
| // inherit-style=after copies the following column's style via a plain | ||
| // before-insert at the same position (the backend anchors on the following | ||
| // column), so position stays D with side=before — the blank lands before D. | ||
| require.Equal(t, "D", clie2e.DryRunGet(out, "tool_input.position").String(), "stdout:\n%s", out) | ||
| require.Equal(t, int64(1), clie2e.DryRunGet(out, "tool_input.count").Int(), "stdout:\n%s", out) | ||
| require.Equal(t, "before", clie2e.DryRunGet(out, "tool_input.side").String(), "inherit-style=after copies the following-side style via side=before; stdout:\n%s", out) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n --type=go -C2 'DimInsert|DimDelete|dim-insert|dim-delete' tests/cli_e2e/sheets | rg -n 'SkipWithoutUserToken|bot|DefaultAs.*bot'Repository: larksuite/cli
Length of output: 151
🏁 Script executed:
#!/bin/bash
set -e
echo "Files:"
fd -a 'sheets.*_test\.go|.*_sheet.*test\.go' tests/cli_e2e 2>/dev/null | sed 's#^\./##' | sort
echo
echo "DimInsert/DimDelete/dim-insert/dim-delete references in tests/cli_e2e (context):"
rg -n --type=go -C3 'DimInsert|DimDelete|dim-insert|dim-delete|DimDelete|insert_dimensions|delete_dimensions|inherit-style|ranges' tests/cli_e2e || true
echo
echo "DryRunGet/usages of defaultAs bot in sheets tests:"
rg -n --type=go -C2 'DryRunGet|DefaultAs|SkipWithoutUserToken|bot' tests/cli_e2e/sheets || true
echo
echo "Target file lines 220-310:"
sed -n '220,310p' tests/cli_e2e/sheets/sheets_sheet_shortcuts_dryrun_test.goRepository: larksuite/cli
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -e
echo "Target file context around all dim shortcuts tests:"
rg -n -C8 'func TestSheets_Dim(Insert|Delete)|+dim-(insert|delete)|inherit-style|--ranges|--position|--side|ToolInput|tool_input|DryRunGet' tests/cli_e2e/sheets || trueRepository: larksuite/cli
Length of output: 452
🏁 Script executed:
#!/bin/bash
set -e
echo "Search only for dim shortcuts in tests/cli_e2e:"
rg -n -C4 'dim-(insert|delete)|DimInsert|DimDelete|--inherit-style|--ranges|--position|--side|modify_sheet_structure|tool_input' tests/cli_e2e || true
echo
echo "Search workflow files under tests/cli_e2e/sheets only for dim shortcuts:"
rg -n -C4 'dim-(insert|delete)|DimInsert|DimDelete|inherit-style|ranges' tests/cli_e2e/sheets || trueRepository: larksuite/cli
Length of output: 11026
Add live E2E coverage for the dim shortcut behavior changes.
TestSheets_DimInsertDryRunInheritAfterKeepsBeforePosition only inspects a dry-run payload. tests/cli_e2e/**/*.go requires live E2E coverage for new flows or behavior changes with create/use/cleanup and bot credentials where applicable, and there is no workflow test exercising +dim-insert/+dim-delete with these flags or +dim-delete --ranges.
🤖 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_sheet_shortcuts_dryrun_test.go` around lines 258
- 290, Add live E2E tests alongside
TestSheets_DimInsertDryRunInheritAfterKeepsBeforePosition covering +dim-insert
and +dim-delete, including --inherit-style variants and +dim-delete --ranges.
Create the required spreadsheet/sheet data, execute the shortcuts with bot
credentials where applicable, assert the resulting sheet structure and styles,
and clean up all created resources; retain the existing dry-run payload test.
Source: Path instructions
…tch and read caps Review fixes from CodeRabbit / code-quality on PR #2091: - cmdutil: route the @/tmp fast-path read through vfs.ReadFile so fakes keep intercepting it; suppress the confirmation retry line when argv carries a credential-bearing flag (secrets stay out of error envelopes); extract requireConfirmationFor and pin the composed hint in tests - localfileio: reject a degenerate TMPDIR (e.g. "/") so the temp-dir read exception cannot widen into accepting arbitrary absolute paths - sheets batch dispatch: duplicate canonical + variant input keys (sheetName/sheet_name, ranges/range) now reject with a prescriptive error instead of silently overwriting the canonical value - sheets flag ergonomics: curated unknown-flag hints match underscore spellings (--frozen_rows hits the --frozen-rows entry) - sheets read offload: an explicit --max-chars survives --output-path instead of being replaced by the unbounded sentinel - chart: declare --print-example in flag-defs.json (single source with the generated reference tables) instead of imperative registration; assert the validation Param in the unknown-type test - e2e: add live +dim-insert / +dim-delete workflow coverage (insert lands before position, --range delete, atomic scattered --ranges) - docs (synced from sheet-skill-spec): +chart-create flags table gains --print-example, freeze docs state the at-least-one-positive rule, read-data table gets its MD058 blank line, detect-subtables explains its fallback except
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/cli_e2e/sheets/sheets_dim_workflow_test.go`:
- Around line 80-100: The dim-insert workflow currently covers only the before
inheritance mapping. Add a corresponding test case using “--inherit-style after”
in the existing dim-insert test flow, and directly assert that column B remains
blank while the original B and C values shift right, matching the expected A1:D1
output.
🪄 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 Plus
Run ID: fa78106f-171b-494c-b70a-44502981829e
📒 Files selected for processing (22)
internal/cmdutil/confirm.gointernal/cmdutil/confirm_test.gointernal/cmdutil/resolve.gointernal/vfs/localfileio/path.goshortcuts/sheets/batch_key_vocab_test.goshortcuts/sheets/batch_op_dispatch.goshortcuts/sheets/chart_examples.goshortcuts/sheets/chart_examples_test.goshortcuts/sheets/data/flag-defs.jsonshortcuts/sheets/data/flag-schemas.jsonshortcuts/sheets/flag_defs_gen.goshortcuts/sheets/flag_ergonomics.goshortcuts/sheets/flag_ergonomics_test.goshortcuts/sheets/lark_sheet_read_data_test.goshortcuts/sheets/read_output.goskills/lark-sheets/references/lark-sheets-chart.mdskills/lark-sheets/references/lark-sheets-read-data.mdskills/lark-sheets/references/lark-sheets-styles-put.mdskills/lark-sheets/references/lark-sheets-workbook.mdskills/lark-sheets/references/lark-sheets-write-cells.mdskills/lark-sheets/scripts/lark_detect_subtables.pytests/cli_e2e/sheets/sheets_dim_workflow_test.go
🚧 Files skipped from review as they are similar to previous changes (19)
- internal/vfs/localfileio/path.go
- shortcuts/sheets/chart_examples_test.go
- skills/lark-sheets/references/lark-sheets-styles-put.md
- internal/cmdutil/confirm.go
- shortcuts/sheets/chart_examples.go
- shortcuts/sheets/batch_key_vocab_test.go
- internal/cmdutil/resolve.go
- skills/lark-sheets/references/lark-sheets-workbook.md
- shortcuts/sheets/flag_ergonomics.go
- shortcuts/sheets/read_output.go
- skills/lark-sheets/references/lark-sheets-read-data.md
- skills/lark-sheets/scripts/lark_detect_subtables.py
- shortcuts/sheets/flag_ergonomics_test.go
- shortcuts/sheets/batch_op_dispatch.go
- skills/lark-sheets/references/lark-sheets-write-cells.md
- shortcuts/sheets/lark_sheet_read_data_test.go
- shortcuts/sheets/data/flag-schemas.json
- shortcuts/sheets/data/flag-defs.json
- shortcuts/sheets/flag_defs_gen.go
Comment on lines
+80
to
+100
| t.Run("dim-insert inherit-style before lands before position", func(t *testing.T) { | ||
| result, err := clie2e.RunCmd(ctx, clie2e.Request{ | ||
| Args: []string{ | ||
| "sheets", "+dim-insert", | ||
| "--spreadsheet-token", spreadsheetToken, | ||
| "--sheet-id", sheetID, | ||
| "--position", "B", | ||
| "--count", "1", | ||
| "--inherit-style", "before", | ||
| }, | ||
| DefaultAs: "bot", | ||
| }) | ||
| require.NoError(t, err) | ||
| result.AssertExitCode(t, 0) | ||
| result.AssertStdoutStatus(t, true) | ||
|
|
||
| // Old columns B/C shift right; the blank column sits at B. | ||
| out := readRow1(t, "A1:D1") | ||
| assert.Contains(t, out, "r1c1,,r1c2,r1c3", | ||
| "inserted blank column must land before position B; stdout:\n%s", out) | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Cover the --inherit-style after mapping.
Line 80 only exercises before, although the test rationale identifies after as the regression direction. Add an after case asserting that insertion still leaves column B blank and shifts the original B/C values right; otherwise an after mapping regression passes this workflow. As per coding guidelines, “Every behavior change must have an accompanying test, and contract tests must assert the changed field or behavior directly.”
🤖 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_dim_workflow_test.go` around lines 80 - 100, The
dim-insert workflow currently covers only the before inheritance mapping. Add a
corresponding test case using “--inherit-style after” in the existing dim-insert
test flow, and directly assert that column B remains blank while the original B
and C values shift right, matching the expected A1:D1 output.
Source: Coding guidelines
| if argReadsStdin(a) || argCarriesSecret(a) { | ||
| return "" | ||
| } | ||
| parts = append(parts, shellQuoteArg(a)) |
Collaborator
There was a problem hiding this comment.
[P1] Avoid echoing free-form payloads into confirmation errors
The guard only detects credential-looking flag names, but high-risk commands also accept sensitive free-form values such as apps +db-execute --sql and base +form-submit --json. A short inline password, SQL literal, or PII payload is therefore copied into the confirmation envelope, stderr, and downstream logs. Please avoid rendering arbitrary argv values here—either whitelist known-safe flags or fall back to the generic add --yes hint for payload-bearing invocations.
| // only the listed failures need resending. | ||
| if strings.Contains(detail.Message, "succeeded") && | ||
| !strings.Contains(detail.Message, " 0 succeeded") { | ||
| if len(detail.Failures) == 1 { |
Collaborator
There was a problem hiding this comment.
[P1] Do not infer fail-fast execution from a single failure
A single failure does not imply fail-fast: +batch-update --continue-on-error can report one failed operation while later operations have already succeeded. Prescribing operations[i:] in that case replays those later successes and can double-apply inserts, deletes, or moves. Please pass the execution mode into this formatter, or emit recovery guidance that distinguishes failed operations from the not-run tail.
|
|
||
| ## 使用场景 | ||
|
|
||
| 写入。对存量表格的多个子表批量应用视觉规格：新表美化、加汇总行后统一版式、按分组合并同类单元格、调列宽行高、冻结表头。整份规格展开为一次原子批量提交，全部生效或全部不生效；纯样式盖章可安全重放（同一份规格重发无副作用）。 |
Collaborator
There was a problem hiding this comment.
[P1] Stop promising transactional batch semantics
+styles-put is implemented with the same batch_update path that this PR documents as fail-fast without rollback. If operations for an earlier sheet succeed and a later sheet is missing or otherwise fails, the earlier changes remain applied, contradicting “all or nothing” and making whole-spec retries unsafe. Please replace the atomic/all-or-nothing wording here and in the matching help for the new batch-backed helpers with explicit no-rollback and recovery semantics.
| continue | ||
| } | ||
| seenName[name] = true | ||
| payload, itemProbs := parseWorkbookCreateStyleItem(item, path) |
Collaborator
There was a problem hiding this comment.
[P2] Reject unknown styles item keys before expansion
parseWorkbookCreateStyleItem ignores unrecognized top-level keys. For example, an item containing valid cell_styles plus freezee: {"rows":1} succeeds, applies the styles, and silently drops the requested freeze. Please reject keys outside name, cell_styles, row_sizes, col_sizes, cell_merges, and freeze, ideally with a did-you-mean hint.
| "input": map[string]interface{}{ | ||
| "excel_id": token, | ||
| "sheet_name": spec.name, | ||
| "range": stripSheetPrefix(cs.Range), |
Collaborator
There was a problem hiding this comment.
[P2] Validate sheet-prefixed ranges before stripping them
This silently discards the range’s sheet prefix without checking it against spec.name. An item with name: "Summary" and range: "Detail!A1:D1" is therefore applied to Summary!A1:D1 and exits successfully. Please reject prefixed ranges or require the prefix to match the item’s target sheet before removing it.
| return err | ||
| } | ||
| b = append(b, '\n') | ||
| if _, err := runtime.FileIO().Save(path, fileio.SaveOptions{}, bytes.NewReader(b)); err != nil { |
Collaborator
There was a problem hiding this comment.
[P2] Preserve typed errors for read-output file failures
Returning the raw FileIO.Save error causes an unsafe path such as --output-path ../../outside.json to fall through as internal/unknown instead of the required validation error, while write failures also lose the file_io subtype. Please return common.WrapSaveErrorTyped(err) here and add assertions for the typed category, subtype, and preserved cause.
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
Aggregates the sheets work on
feat/lark-sheets-developsince #1833, driven by eval-batch error attribution (07-11 / 07-21 / 07-28 batches). Four themes:Error messages as prescriptions
Batch-update contract hardening (#2028)
+batch-update, from a dedicated 137-error attribution round and five re-run validations, including a cross-model batch (Doubao).Read-path improvements
--includetruncation support on+cells-get.--output-pathfor full-read file offload oncells/csv/table-get, avoiding stdout truncation on large reads.Correctness fixes
+dim-insert --inherit-styleside mapping corrected (backend always inherits the anchor column's style;sideonly picks placement):before→side=after+pos-1,after→side=before; redundantnonedropped.--csvvalues instead of silently writing them into the sheet.origin/mainhas been merged back into this branch (conflicts resolved ininternal/validate/path.goandinternal/vfs/localfileio/path.go, keeping bothSafeTempAbsInputPathandLocalInputPath).Test plan
go build ./...clean.go test ./shortcuts/sheets/...,./internal/vfs/localfileio/...,./internal/validate/...all pass post-merge.Summary by CodeRabbit
New Features
+styles-putfor styles, merges, row/column sizing, and freezes.+cells-set --writesand multi-range+dim-delete --ranges.+chart-create --print-exampleand improved--print-schema --flag-namefor dotted paths.Bug Fixes
re-run … --yeswhen safe).Documentation
--output-path, truncation reporting, and whole-sheet defaults like optional--rangewhere supported).