feat(sheets): harden chart update workflows - #2058
Closed
zhengzhijiej-tech wants to merge 28 commits into
Closed
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.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
domain/ccm
…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.
…eat/chart-snapshot-detached-header # Conflicts: # shortcuts/sheets/flag_ergonomics.go # shortcuts/sheets/flag_ergonomics_test.go
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| if has_authoritative_rows and offset < len(row_indices): | ||
| try: | ||
| row_num = int(row_indices[offset]) | ||
| except (TypeError, ValueError): |
| var ranges []string | ||
| start := 0 | ||
| inQuote := false | ||
| for i := 0; i <= len(value); i++ { |
| if err != nil { | ||
| return err | ||
| } | ||
| input, err := chartConfigUpdateInput(runtime, token, sheetID, sheetName) |
| if err != nil { | ||
| return err | ||
| } | ||
| input, err := chartDataUpdateInput(runtime, token, sheetID, sheetName) |
Collaborator
Author
|
Superseded by #2374, which combines the chart workflow improvements with the special chart types on a clean branch from current main. |
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.
Problem
Chart shortcuts lacked a detached-header input and returned insufficient post-write state for efficient follow-up edits. Batch chart operations also needed clearer partial-failure and input-normalization behavior.
Changes
Add --header-range translation for +chart-create-basic and +chart-data-update, sync generated flag/reference metadata, preserve standalone/batch request parity, and harden batch partial-failure and chart flag compatibility paths.
Behavior
The CLI forwards detached data/header ranges to manage_chart_object. Create, config-update, and data-update responses are consumed directly from the server without additional CLI-side snapshot transforms.
Related changes
Validation
go test -count=1 ./shortcuts/sheets/... and go vet ./shortcuts/sheets/... passed. Remote Linux candidate dry-runs produced the expected basic_chart.header_range and data_updates.header_range payloads.