feat(sheets): typed table I/O & error contract, workbook import/export, skill refresh#1355
Merged
Conversation
feat(sheets): add gridline show/hide shortcuts
…tfalls Add targeted guidance to six lark-sheets references to reduce frequent mistakes when editing spreadsheets through the CLI: - write-cells: sanity-check units / dimension conversion / quantity factors before formula writes (formulas can run clean yet be off by a factor); keep derived output off original data columns to avoid clobbering source - core-operations: prefer live formulas for derived values even when "live update" is not explicitly requested; scope rewrite/transform precisely so rows/columns that should stay unchanged are kept 1:1; treat header-stated format rules as checklist items; confirm the artifact file actually exists before finishing; write back bare values from local scripts - visual-standards: apply border/header formatting on explicit request and identify the real header row; keep font size consistent with the source - range-operations: keep total column width within A4 for printing - read-data: dedup/compare long numbers via raw values, not csv formatted display (scientific notation collapses distinct numbers and causes false duplicates) - chart: format date/number axes via source-cell number_format; place charts outside the data area so they do not cover existing data
- Add lark_sheet_table_io.go with +table-put / +table-get and tests - Refactor read-data; extend workbook; register new shortcuts - Sync generated flag defs/schemas (go:embed) from sheet-skill-spec - Sync skill references (write-cells numeric-column guidance, plus read-data / workbook / chart updates)
Quick-ref table (SKILL.md, the first decision point) had no +table-put and gated typed writes on "DataFrame", so a model holding a Counter/list/dict would fall back to +csv-put and silently lose number/date fidelity. - split csv-put row to plain-text values (no numeric/date semantics) - add +table-put row for typed writes into an existing sheet - add +workbook-create --sheets row for create + typed write in one shot - add judgment note: number/amount/date/percent/count -> +table-put (or +workbook-create --sheets when the workbook does not exist yet); plain text -> +csv-put - reframe write-cells scenario row to lead with numeric semantics - point new-table writes at +workbook-create --sheets (one shot) instead of the create-empty-then-table-put two-step Synced from sheet-skill-spec canonical (generate:cli + sync:cli).
Mirror the upstream sheet-skill-spec change removing the "not applicable to local Excel files" tail from the sheets skill and reference descriptions.
Mirror the upstream sheet-skill-spec change removing the "applies to Feishu sheets only" tail from the 14 sheet reference descriptions.
Import a local xlsx/xls/csv as a new spreadsheet by delegating to the shared drive import flow with the target type pinned to sheet. Refactor drive +import to expose ImportParams / ValidateImport / PlanImportDryRun / RunImport (behavior unchanged, existing drive tests still cover it); sheets reuses them. Regenerate flag_defs_gen.go and sync the spec mirror.
Replace +workbook-export's parallel export-task implementation with the shared drive ExportParams/RunExport core (pinned to type=sheet). Drops ~90 lines of duplicated poll/download code; +workbook-export now inherits drive's ctx cancellation, resume-on-timeout, filename sanitize/overwrite, and the full set of export status labels. The output contract aligns with drive's (adds ready/downloaded/doc_type; saved_path preserved). Also normalize an empty drive --output-dir to "." so drive +export behavior is unchanged, and fix the sheets export e2e to call +workbook-export instead of a nonexistent +export.
…ested metric - range-operations: only widen new / overflowing columns; never recompute or shrink the widths of existing columns (any blanket resize, even by 1px, breaks the original visual format) - chart: when the user asks for a share / percentage, the value axis should be a percentage (pie, or stack.percentage on bar/column) rather than raw counts
Replace scoring-framework wording in the examples with plain functional consequences (e.g. "not delivered", "goes stale when the source changes", "breaks the original visual format"), so the references stay agent-facing.
Bring the hand-applied write-cells example in line with the spec-generated reference so the CLI mirror is byte-identical to the canonical source.
docs(sheets): strengthen lark-sheets references for common editing pitfalls
Sync the formula-support wording from sheet-skill-spec (flag-defs, skill references) and update the hand-authored cobra Description and comment for +csv-put. +csv-put evaluates a leading-= cell as a formula via set_range_from_csv; descriptions only, no behavior change.
The chart reference's placement example used non-existent flags --dimension/--start/--end for +dim-insert. The real signature is --position (required) + --count (required); copying the example fails Validate with "--position is required". Replace it with +dim-insert --position V --count 6 (insert 6 columns before V, i.e. after U), aligning with the sheet-structure reference.
Sync three reference-doc corrections from the spec source:
1. chart: label position.row as 0-based (first row = row:0), distinct
from the 1-based row numbers used by A1 ranges and +dim-insert
--position, removing the row-base ambiguity.
2. chart: convert the three runnable examples whose JSON contains a
quoted sheet prefix ('Sheet1'!A1) from inline single-quoted
--properties '{...}' to a stdin heredoc (--properties - <<'JSON').
Inside an inline single-quoted string bash strips the inner quotes
around the sheet name (and splits names with spaces into words),
corrupting the JSON; a quoted heredoc delimiter performs no shell
substitution and preserves it. Adds a short note on the pitfall.
3. filter / filter-view: add the full conditions[].type x compare_type
enum table (text / number / multiValue / color and their respective
compare_type values and values shape), and call out the
equals/notEquals (with s) vs equal/notEqual (no s) gotcha. The docs
previously only showed two values via examples.
The base flag description for +sheet-create's --index omitted the
coordinate base, while its siblings +sheet-move ("Target position
(0-based)") and +sheet-copy already state 0-based. Align the description
so the index base is unambiguous. Synced from the spec source
(flag-defs.json + workbook reference).
docs(sheets): chart / filter / workbook reference corrections
|
|
github-actions
Bot
added
domain/ccm
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds a typed table protocol (TablePut/TableGet), centralizes Drive export/import into exported helpers, integrates those cores into workbook shortcuts (create/export/import), removes CsvGet's rows-json path, updates CLI flag/schema/dispatch metadata, and adds tests and documentation for the new contracts. ChangesSheets Typed Table Protocol & Workbook Integration
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@9534e33e0f1c1e6d13310b1ceb8e98e59243fe2f🧩 Skill updatenpx skills add larksuite/cli#feat/lark-sheets-develop -y -g |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1355 +/- ##
==========================================
+ Coverage 74.03% 74.19% +0.16%
==========================================
Files 787 789 +2
Lines 76328 77614 +1286
==========================================
+ Hits 56509 57587 +1078
- Misses 15572 15666 +94
- Partials 4247 4361 +114 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
shortcuts/sheets/data/flag-schemas.json (1)
17-68:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd gridline shortcuts to
+batch-update.operations.shortcutenum.
+sheet-show-gridlineand+sheet-hide-gridlineare now batchable in dispatch/tests, but they’re missing from the schema enum, so schema-validated+batch-updatepayloads can be rejected incorrectly.Suggested fix
"enum": [ "+cells-set", "+cells-set-style", "+cells-clear", "+cells-merge", "+cells-unmerge", "+cells-replace", "+csv-put", "+dropdown-set", "+dim-insert", "+dim-delete", "+dim-hide", "+dim-unhide", "+dim-freeze", "+dim-group", "+dim-ungroup", "+rows-resize", "+cols-resize", "+range-move", "+range-copy", "+range-fill", "+range-sort", "+sheet-create", "+sheet-delete", "+sheet-rename", "+sheet-move", "+sheet-copy", "+sheet-hide", "+sheet-unhide", "+sheet-set-tab-color", + "+sheet-show-gridline", + "+sheet-hide-gridline", "+chart-create", "+chart-update", "+chart-delete", "+pivot-create", "+pivot-update",🤖 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/data/flag-schemas.json` around lines 17 - 68, The enum for batch-update operation shortcuts is missing the new gridline actions; update the enum array in the flag-schemas.json entry for the +batch-update.operations.shortcut (the same enum shown in the diff) to include "+sheet-show-gridline" and "+sheet-hide-gridline" so schema-validated +batch-update payloads accept those batchable operations.
🤖 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/lark_sheet_table_io_test.go`:
- Around line 176-181: The test currently checks error messages with
strings.Contains which is fragile; update the t.Run cases (those calling
parseTablePutPayload with stubFlagView{"sheets": tt.json}) to assert structured
error metadata using errs.ProblemOf(err) — verify the Problem.Category,
Problem.Subtype and any Problem.Param match the expected values from the test
case — and also assert cause preservation by unwrapping the returned error
(errors.Unwrap or errors.Is) to compare against the expected cause value in the
table; replace the strings.Contains assertions with these typed checks to cover
category/subtype/param and preserved cause for parseTablePutPayload and the
other indicated test blocks.
In `@shortcuts/sheets/lark_sheet_table_io.go`:
- Around line 399-401: The code is re-wrapping lower-layer errors with
fmt.Errorf which strips typed errs.* classifications; instead, pass through the
original error from lastDataRow (return nil, err) or, if you must add context at
the command layer, wrap using the appropriate errs.* constructor (e.g., errs.New
or another specific errs.* type) while preserving the original as the cause;
update the occurrences that call lastDataRow and similar calls (the append path
around lastDataRow and the other ranges noted) so they either return the
underlying err directly or wrap it with an errs.* typed error rather than
fmt.Errorf/output.Err*.
- Around line 836-838: The returned tableGetSheet for the "--sheet-id" branch
only sets id and drops the existing sheet name, causing downstream output to
emit name:"" and breaking +table-get -> +table-put round trips; update the
branch that returns []tableGetSheet{{...}} to include the original name (e.g.,
[]tableGetSheet{{id: id, name: t.name}}) and also ensure the emitter that writes
name (the code around t.name at the later emission) uses the preserved name
instead of blanking it so both the return path and the emission path keep the
non-empty sheet name.
- Around line 371-380: The function tablePutFullRange can produce invalid ranges
when totalRows is 0 (e.g., "A1:C0"); add an early guard at the top of
tablePutFullRange to return an empty/no-op range string (""), e.g. if totalRows
<= 0 { return "" }, and apply the same fix to the analogous function around the
730-733 block (the tablePutRange implementation) so both return "" when there
are no rows to write; update any callers/tests expecting a no-op behavior if
needed.
---
Outside diff comments:
In `@shortcuts/sheets/data/flag-schemas.json`:
- Around line 17-68: The enum for batch-update operation shortcuts is missing
the new gridline actions; update the enum array in the flag-schemas.json entry
for the +batch-update.operations.shortcut (the same enum shown in the diff) to
include "+sheet-show-gridline" and "+sheet-hide-gridline" so schema-validated
+batch-update payloads accept those batchable operations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 059d6655-5406-414b-b220-3b8d15bc432d
📒 Files selected for processing (31)
shortcuts/drive/drive_export.goshortcuts/drive/drive_export_test.goshortcuts/drive/drive_import.goshortcuts/sheets/batch_op_contract_test.goshortcuts/sheets/batch_op_dispatch.goshortcuts/sheets/data/flag-defs.jsonshortcuts/sheets/data/flag-schemas.jsonshortcuts/sheets/flag_defs_gen.goshortcuts/sheets/flag_schemas_gen.goshortcuts/sheets/lark_sheet_read_data.goshortcuts/sheets/lark_sheet_read_data_test.goshortcuts/sheets/lark_sheet_table_io.goshortcuts/sheets/lark_sheet_table_io_test.goshortcuts/sheets/lark_sheet_workbook.goshortcuts/sheets/lark_sheet_workbook_export_test.goshortcuts/sheets/lark_sheet_workbook_import_test.goshortcuts/sheets/lark_sheet_workbook_test.goshortcuts/sheets/lark_sheet_write_cells.goshortcuts/sheets/shortcuts.goskills/lark-sheets/SKILL.mdskills/lark-sheets/references/lark-sheets-chart.mdskills/lark-sheets/references/lark-sheets-core-operations.mdskills/lark-sheets/references/lark-sheets-filter-view.mdskills/lark-sheets/references/lark-sheets-filter.mdskills/lark-sheets/references/lark-sheets-financial-modeling-standards.mdskills/lark-sheets/references/lark-sheets-range-operations.mdskills/lark-sheets/references/lark-sheets-read-data.mdskills/lark-sheets/references/lark-sheets-visual-standards.mdskills/lark-sheets/references/lark-sheets-workbook.mdskills/lark-sheets/references/lark-sheets-write-cells.mdtests/cli_e2e/sheets/sheets_crud_workflow_test.go
💤 Files with no reviewable changes (1)
- shortcuts/sheets/lark_sheet_read_data_test.go
Add `counta` (count non-empty cells, incl. text) to manage_chart_object dim2.series[].aggregateType in the chart flag schema. `count` only counts numeric cells, so counting occurrences of a text/category column renders an empty chart; `counta` enables category frequency counts. Synced from the sheet-skill-spec canonical schema.
…y dropping them A +workbook-create call carrying only cell_merges / row_sizes / col_sizes (no --values / --sheets and no cell_styles) used to create the workbook but silently drop the requested visual ops. Two reasons, both fixed: - workbookCreateStyleDimensions only counted cell_styles when computing the write extent, so cell_merges / row_sizes / col_sizes always contributed 0 → buildValuesPayload returned a nil payload → Execute skipped writeTypedSheets entirely → no visual ops ran. Extend the helper to fold the merge / resize ranges in. - Pure row_sizes / col_sizes payloads can never expand a cell rectangle (they are dimension ranges, not cell ranges), so even with the extent fix Execute would still skip the write path. Add a no-data branch: when payload == nil but a styles item is present, look up the default sheet and apply visual ops directly via applyWorkbookCreateVisualOps. The dry-run plan mirrors this so the preview shows the visual ops. Also picks up the --values trailing-JSON-data EOF check (mirror of the --sheets one in lark_sheet_table_io.go). End-to-end verified against a real spreadsheet: a cell_merges-only +workbook-create now produces a sheet with merged_cells_count: 1.
…lidation errors
common.ValidationErrorf goes through fmt.Sprintf, which does not support
%w — the seven call sites that used `%w` were rendering the cause as
literal `%!w(*fmt.wrapError=&{...})` and dropping the cause from the
typed-error chain (so callers couldn't errors.As back to the underlying
error).
Switch each to `%v` for clean rendering and attach the cause via
.WithCause(err) so the typed contract is preserved. Touched call sites:
- lark_sheet_dataframe.go: --dataframe Arrow decode / stdin read / file
read failures (3 call sites).
- lark_sheet_table_io.go: --sheets invalid JSON, payload-validate
per-cell coercion error, buildSheetMatrix per-cell error,
--dataframe-out arrow encode failure (4 call sites).
End-to-end verified against a real spreadsheet: both invalid-JSON and
typed-cell errors now render readable messages instead of %!w(...).
…hema Mirror of the sheet-skill-spec change adding the two gridline shortcuts to cli-schemas.json batch_update.operations.shortcut enum. Synced from the upstream canonical via generate:cli + sync:cli. Verified end-to-end on a real spreadsheet — +batch-update with a +sheet-hide-gridline op passes schema validation and the backend run returns succeeded: 1.
Mirror of the sheet-skill-spec update that documents +workbook-export's default-no-download behavior and its relationship to drive +export --doc-type sheet. Synced from canonical via generate:cli + sync:cli + go generate. End-to-end verified against a real spreadsheet: - Omit --output-path → ok:true, downloaded:false, file_token returned - Pass --output-path ./crfix_test.xlsx → ok:true, file saved (17892 bytes), saved_path returned The --help output for +workbook-export now states the default behavior and points callers at `drive +export --doc-type sheet` when they need the --output-dir / --file-name / --overwrite split.
…ings
Per the coding guideline "Error-path tests must assert typed metadata via
errs.ProblemOf (category / subtype / param) and cause preservation, not
message substrings alone." — sweep through every error-path assertion in
the sheets domain and replace the
`strings.Contains(stdout+stderr+err.Error(), ...)` pattern with two
small helpers landed in helpers_test.go:
requireProblem(t, err, wantCategory, wantSubtype, msgContains)
-> *errs.Problem
requireValidation(t, err, msgContains)
-> *errs.ValidationError // shorthand for CategoryValidation +
// SubtypeInvalidArgument; lets callers
// also assert .Param / .Params / .Cause
~60 assertion sites across 18 test files now check the typed envelope
shape, with message-substring checks moved onto the returned Problem
(.Message / .Hint / .Param). The substring is preserved as a sanity
check rather than the sole assertion, so a category drift like
validation → internal would now fail loudly instead of slipping past.
Cases intentionally left as substring (each with a one-line reason):
- Errors that come straight from cobra's native flag parser (untyped
*errors.errorString — e.g. "required flag(s) ... not set", mutually-
exclusive groups). Re-typing these needs a custom FlagErrorFunc and
is out of scope here.
- Intermediate errors from decodeArrowToSheet that the caller wraps
into a typed envelope (`//nolint:forbidigo` reason). Those unit
tests assert the unwrapped intermediate directly.
One production tweak:
- shortcuts/sheets/flag_schema.go: printFlagSchemaFor returns typed
*errs.ValidationError (with WithParam("--flag-name") on the
unknown-flag branch) instead of raw fmt.Errorf. The framework
already wraps this when called via --print-schema, so user-facing
behaviour is unchanged; direct callers (and tests) now get the
typed envelope.
Verified: go test ./shortcuts/sheets/... passes; golangci-lint
--new-from-rev=origin/main reports 0 issues.
…ings
Mirror of the sweep just landed in shortcuts/sheets: replace error-path
substring assertions with typed-envelope checks via two small helpers
landed in a new shortcuts/common/typed_error_assertions_test.go:
requireProblem(t, err, wantCategory, wantSubtype, msgContains)
-> *errs.Problem
requireValidation(t, err, msgContains)
-> *errs.ValidationError // shorthand for CategoryValidation +
// SubtypeInvalidArgument; lets callers
// also assert .Param / .Params / .Cause
8 sites moved to typed assertions across runner_jq_test.go,
mcp_client_test.go, drive_media_upload_typed_test.go, and
runner_input_test.go (the input tests already used a typed-param helper;
this just retargets the substring follow-up onto the typed Message).
Sites intentionally left as substring + comment (production returns raw
fmt.Errorf, not a typed envelope):
- runner_botinfo_test.go (6 sites): BotInfo / fetchBotInfo wrap upstream
errors with fmt.Errorf so the SDK-level message ([99991], 403,
invalid character, etc.) shows through.
- runner_args_test.go (4 sites in 2 tests): rejectPositionalArgs returns
raw fmt.Errorf to satisfy cobra's PositionalArgs contract.
- permission_grant_test.go (2 sites): assert on stderr / hint strings,
not error messages — already out of the err.Error() substring class.
No production code changes.
Verified: go test ./shortcuts/common/... passes;
golangci-lint --new-from-rev=origin/main ./shortcuts/common/... reports
0 issues.
Collaborator
Author
Code review round-upAll 10 inline review comments now have point-by-point replies on the diff. Summarizing here for the two CodeRabbit "outside diff range" notes and the overall picture: Outside-diff CodeRabbit notes
Sites intentionally left as substring (each carries a one-line comment in code)
Total: ~70 substring sites moved to typed envelope assertions across 25 test files; the ~16 left are all listed above with a stated reason and a code comment. Commits answering the inline comments
Every behavior fix above was end-to-end verified against a real spreadsheet (PPE) — not just unit tests. The cases live in the per-comment replies. |
…ed in CR Four review-flagged bugs, all in lark_sheet_table_io.go (bundled because they touch the same file and the same +table-put / +table-get domain): 1. +table-get --dry-run dropped the --sheet-id / --sheet-name selector from the get_cell_ranges body, while Execute always passed it. Agents that validate the dry-run shape and then run live would see a request shape mismatch. The dry-run now calls sheetSelectorForToolInput so the body matches Execute. 2. isDateNumberFormat used a simple `strings.ContainsRune(_, 'y')` so number formats like "JPY #,##0" (a currency prefix that happens to contain a lone 'Y') were misread as date formats — round-tripping integer cells out as ISO dates. The detector is now token-aware: it skips quoted "...", `\\x`-escaped, and `[...]` bracket sections, and only fires on an unescaped `yy` (a real Excel year token). 3. sheetCreateDims sized new append-mode sheets by `headerOn(s)` only, but writeSheetData forces a header on empty append sheets when Header == nil. Near 50000 rows / 200 cols this created the sheet one row short and the follow-up set_cell_range bounced off the backend ceiling. Size now matches the forced-header logic exactly. 4. tableGetTargets fallback paths (read-failure / selector mismatch on --sheet-id) returned a target with name="" — already corrected for --sheet-id structure-success path in 086876d, but the structure- failure fallback still left it empty. Use the id as the name there too so the +table-get → +table-put round-trip never breaks on a nameless sheet. End-to-end verified against a real spreadsheet: - table-get --dry-run with --sheet-name / --sheet-id both render the selector field in the get_cell_ranges body - A real round-trip (typed put → get) preserves dtypes + formats
readDataframeBytes used to read the whole Arrow file unbounded — a
stdin / file > 1 GiB would OOM the CLI long before the backend
per-sheet ceilings kicked in. decodeArrowToSheet then materialized
every record into [][]interface{} regardless of size.
Three caps now match the backend's per-sheet hard ceilings:
- byte cap: 256 MiB (covers worst-case 200×50000 cells × ~25 B Arrow
overhead). File path pre-Stat()s before opening; both file and stdin
paths read through io.LimitReader so an oversized input is rejected
without allocating the full payload.
- column cap: 200, checked at schema-decode time before allocating any
per-column slices.
- row cap: 50000, checked during record-batch iteration so a 1M-row
Arrow file is rejected mid-stream instead of fully decoding first.
End-to-end verified against PPE — a 257 MiB file is rejected at file-
Stat with a typed validation error before any read happens.
…orkError The poll loop in RunExport returned ctx.Err() directly in two places — on the inter-attempt sleep cancel and on the pre-attempt deadline check. That let context.Canceled / context.DeadlineExceeded escape as untyped errors at the cobra layer, bypassing the typed-error contract every other failure path already honors. Add wrapExportContextErr that maps both into errs.NewNetworkError with SubtypeNetworkTransport / SubtypeNetworkTimeout respectively and preserves the cause via .WithCause(err), so callers can still errors.Is(err, context.Canceled) downstream. CR-flagged at drive_export.go:229 / :234.
The dependency-license check still has to --ignore Apache Arrow wholesale because go-licenses' classifier parses its LICENSE.txt as a single license and mis-reports the module as LicenseRef-C-Ares / Unknown (Arrow inlines the c-ares 3rdparty notice alongside its own Apache-2.0). Re-classifying on our side isn't possible without changing go-licenses itself. The CR concern was that --ignore is too wide — a future Arrow re-license or new inlined dep would silently sail through. Add a follow-up step that re-checks Arrow's LICENSE.txt independently: it must still open with "Apache License" AND must still inline the c-ares 3rdparty notice (the two facts that make the --ignore safe today). If either invariant breaks, CI fails here and forces a human to re-evaluate the ignore. Verified locally — both assertions pass against the current pinned Arrow v17.
Mirror of the sheet-skill-spec change that fixes three places teaching
an invalid +table-put payload shape — the typed protocol only has
columns / data / dtypes / formats (no formula field) and must always
be wrapped in an outer {"sheets":[...]} envelope. write-cells and the
SKILL.md decision table previously used the wrong field names (type /
format) and pointed users at +table-put for formula writes, which the
shortcut can't actually accept.
Synced from upstream canonical via generate:cli + sync:cli.
…-create AGENTS.md requires a dry-run E2E for every new shortcut and a live E2E for new flows. Three new files cover the four shortcuts this branch adds or materially changes: - sheets_gridline_dryrun_test.go — pins +sheet-show-gridline / +sheet-hide-gridline as a single modify_workbook_structure call with the right operation name (show_gridline / hide_gridline) and sheet_id, so an op-name typo would trip CI before any live run. - sheets_workbook_import_dryrun_test.go — pins +workbook-import as a two-step plan (drive media upload + drive import-task create) with the doc type hard-coded to "sheet" — the wrapper's whole reason for existing on top of generic drive +import. --name reaches file_name on the wire; file_extension is sniffed from the local file. - sheets_table_put_typed_workflow_test.go — two live workflows running against a freshly created spreadsheet. The first runs the full typed +table-put → +table-get round-trip (date / numeric / object columns with custom number_format) and asserts the dtype + format contract holds end-to-end. The second exercises the typed +workbook-create --sheets path: create + write in one shortcut, the payload sheet name adopts the workbook's default sheet (no empty "Sheet1" left behind), and the typed contract still survives the read-back. End-to-end verified locally (user identity): typed put round-trips preserve dtypes (date → datetime64[ns], numeric → float64, object → object) + formats verbatim; workbook-create adopts the named sheet as the first sheet with the same typed shape intact.
…spec Mirror of the sheet-skill-spec change that adds a DataFrame ↔ JSON bridge as a skill-bundled Python script instead of inside the CLI binary. Per PR #1355 review (docx NcmxdRo2yoZ4OXxoMUZcxRZ7nHd, §4.2): keep the CLI a thin JSON/REST client; pandas / Arrow editing lives in the caller's Python process. Synced from canonical via generate:cli + sync:cli. - skills/lark-sheets/scripts/sheets_df.py (new): pandas DataFrame ↔ one sheet, .parquet / .feather / .arrow / .csv / .json. Shells out to `+table-put` / `+table-get` over typed JSON — no CLI changes. - SKILL.md decision tree + write-cells.md +table-put section: explicit pointers so pandas users land on the script instead of hand-rolling the `--sheets` payload. End-to-end verified against PPE: 3-row DataFrame (datetime / float / object) round-trips parquet → script put → real sheet → script get → parquet with dtypes preserved.
…pt from spec" This reverts commit 2964983.
Mirror of the sheet-skill-spec change that ships a 32-line helper-only sheets_df.py (df_to_sheet + sheet_to_df) and removes the corresponding inline `def` blocks from three reference docs. - skills/lark-sheets/scripts/sheets_df.py (new): pandas DataFrame ↔ one +table-put / +table-get sheet, importable as a library. Same helper pair the docs already taught, lifted out of the prose so callers can `from sheets_df import df_to_sheet, sheet_to_df`. - lark-sheets-write-cells.md / lark-sheets-read-data.md / lark-sheets-workbook.md: drop the inline helper definitions; keep the usage examples (single/multi-sheet, round-trip) and switch them to import-from-script. workbook reference's +workbook-create --sheets section now points pandas users at the helper directly (was previously a textual reference back to write-cells). End-to-end verified against PPE (--as user): - +workbook-create with df_to_sheet for three sheets (income / balance / cashflow): create ok, dtypes (datetime64[ns] / float64) + formats (#,##0 / 0.0% / yyyy-mm-dd) survive on read-back through sheet_to_df. - read → pandas mutate → write-back round-trip preserves both data and formats.
The previous commit (5fac9c3) shipped sheets_df.py and inadvertently included its `__pycache__/sheets_df.cpython-312.pyc` — local Python import created the bytecode cache during PPE round-trip verification and `git add skills/lark-sheets/` swept it in. Remove the pyc and add Python bytecode patterns to .gitignore so the skill-bundled helper scripts don't pull cache files into future commits.
Per the design review at NcmxdRo2yoZ4OXxoMUZcxRZ7nHd, the Arrow IPC binary input/output channel adds a heavy columnar runtime to the CLI for no new capability — the typed JSON --sheets path already covers everything, and the column-major / zero-copy advantages collapse the moment the CLI re- encodes into the row-oriented sheets OpenAPI JSON body. Removing it also lets us drop the `--ignore github.com/apache/arrow/go/v17` license-check escape hatch. Deleted: - shortcuts/sheets/lark_sheet_dataframe.go (+ test) - --dataframe branches in +table-put / +workbook-create - --dataframe-out branch in +table-get - StdinConsumed / MarkStdinConsumed exported methods (the binary stdin reader was the only out-of-band consumer); internal stdinConsumed guard against duplicate `-` input flags stays - apache/arrow/go/v17 + transitive deps via `go mod tidy` - CI go-licenses --ignore for arrow and the LICENSE.txt assertion step - --dataframe / --dataframe-out coverage in skill references Pandas users keep the round-trip via the existing skill script skills/lark-sheets/scripts/sheets_df.py over the JSON path. The full pre-removal state is preserved on branch feat/sheets-arrow-stash. Upstream sheet-skill-spec follow-up: the two flag rows in the canonical spec + base table tblV2F6fqIjyCFQW must also be dropped so the next sync does not re-add them.
zhengzhijiej-tech
previously approved these changes
Jun 24, 2026
Mirrors sheet-skill-spec 5562f83. The +table-put / +workbook-create
--sheets flag descriptions (and the --print-schema description on the
sheets array) now point at the existing df_to_sheet helper instead of
the previous misleading one-liner that produced a dict missing the
outer {"sheets":[...]} envelope and the per-sheet `name`. Agents that
copy-paste the description verbatim now build a valid payload.
Auto-synced via spec's generate:cli + sync:consumers; go generate
./shortcuts/sheets/... regenerated flag_defs_gen.go so its embedded
flagDefs stays byte-equal to data/flag-defs.json.
AGENTS.md requires both dry-run and live E2E for every newly registered shortcut, and behavior-changing refactors need at least the matching half. Three gaps remained on feat/lark-sheets-develop: - +sheet-show-gridline / +sheet-hide-gridline (new): only dry-run E2E. Add sheets_gridline_workflow_test.go — create a real spreadsheet, toggle hide then show against a live sub-sheet, assert ok=true on both (gridline state is write-only — there is no read-back field on +sheet-info / +workbook-info — so a successful envelope is the meaningful signal; the dry-run E2E already pins the wire shape). - +workbook-import (new): only dry-run E2E. Add sheets_workbook_import_workflow_test.go — write a local CSV, run the full upload → create-task → poll, assert ready=true with a sheet token, +info confirms the imported workbook is reachable, cleanup deletes the spreadsheet. - +workbook-export refactor (no-download default changed): had live E2E but no dry-run E2E in tests/cli_e2e/. Add sheets_workbook_export_dryrun_test.go — pin the three sheet- specific differences vs drive +export: type=sheet hard-coded, csv mode routes --sheet-id onto sub_id (xlsx mode omits it), and --output-path maps onto the dry-run plan's top-level output_dir. Also pins the csv-without-sheet-id validation error.
…ailure
Three "made it halfway and stopped" exits in the sheets domain previously
disagreed on shape, which made the post-failure recovery flow hard for
agents to predict from one command to another:
- +table-put partial write → exit 1, stdout ok:false envelope
- +table-put zero-sheet write → exit 1, stderr api/server_error
- +workbook-create create-but-fill → exit 2, stderr validation/failed_precondition
OutPartialFailure exists exactly for "the side effect landed but the
follow-up didn't" — it stamps an ok:false result envelope on stdout
(carrying the state the caller needs to recover) and returns the bare
partial-failure exit signal. The workbook-create fill-failure path was
the odd one out: it surfaced as a typed failed_precondition error on
stderr, which agents couldn't tell apart from a plain validation refusal
even though the spreadsheet really did exist and a retry / cleanup was
possible.
Migrate workbookCreatedButFillFailed onto OutPartialFailure so the four
call sites in +workbook-create's Execute (sheet-resolve failure, initial
fill failure, style-only resolve failure, style-only apply failure) emit
the same envelope shape +table-put's partial write does:
{
"ok": false,
"data": {
"spreadsheet_token": "shtNEW",
"reason": "spreadsheet shtNEW created but initial fill failed",
"hint": "the spreadsheet exists; retry the fill … or delete it",
"cause": {"category": "...", "subtype": "...", "message": "..."}
}
}
The inner failure's typed problem (category / subtype / message) is
flattened into the `cause` field so agents stay diagnosable from the JSON
envelope alone, instead of having to errors.Unwrap a Go error.
Updated TestExecute_WorkbookCreate_FillFailureKeepsToken to assert the
new shape (ok:false envelope on stdout, *output.PartialFailureError exit
signal, structured cause carrying the underlying invalid_response
subtype) — preserving the original test intent (token must survive for
recovery; inner cause must stay diagnosable) under the new contract.
- shortcuts/sheets/flag_schema_validate.go:106 — composite-JSON shape validation was wrapping vErr's message into a typed sheets validation error without preserving vErr as the typed cause; add the missing .WithCause(vErr) so errors.Unwrap and ProblemOf still find the underlying validator error (matches every other typed-error chain helper in the file). - shortcuts/sheets/lark_sheet_batch_update.go:92 — comment claimed batchUpdateInput returns "FlagErrorf-typed errors", but FlagErrorf no longer exists (the typed-error migration replaced it with common.ValidationErrorf / errs.ValidationError); update the comment to reflect what is actually returned. - shortcuts/drive/drive_export.go:121 — drop the ValidateExport public alias and rename to validateExport. sheets +workbook-export reuses RunExport / PlanExportDryRun from this package but inlines its own (sheet-specific) Validate, so there is no cross-package call site — ValidateExport was a misleading sibling of the genuinely-shared ValidateImport. Comment added to record the asymmetry so future readers do not export it back.
The earlier --dataframe / --dataframe-out + apache/arrow/go/v17 removal deleted the arrow consumer but left two indirect lines in go.mod pinned to the versions arrow had pulled in: - github.com/kr/text v0.2.0 - golang.org/x/exp v0.0.0-20240222234643-814bf88cf225 With arrow gone, larksuite/cli was the only requirer of those exact versions; every real consumer needs lower ones (kr/pretty wants kr/text v0.1.0; charmbracelet/huh wants x/exp …20231006; xo/terminfo wants x/exp …20220909). Removing the two indirect lines and running `go mod tidy` lets MVS pick the real-consumer versions and drops the explicit indirect entries entirely — go.mod net-diff against main is now zero for this branch. Verified locally: go build ./...; go test ./shortcuts/sheets/... ./shortcuts/drive/... ./shortcuts/common/... ./internal/auth/... ./cmd/auth/... — all green.
fangshuyu-768
approved these changes
Jun 24, 2026
liangshuo-1
approved these changes
Jun 24, 2026
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
6 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
Lands the accumulated
feat/lark-sheets-developwork intomain(337 commits relative to local main, 57 truly branch-only). The thrust is typed, type-faithful batch I/O for Lark Sheets plus a domain-wide migration to the typed error contract, with broadlark-sheetsskill/doc refresh from the upstream spec.Changes
New shortcuts & capabilities (feat)
+table-put/+table-getwith column type & number-format preservation (lark_sheet_table_io.go).--sheetsJSON accepted by+table-put/+workbook-create(pandas DataFrame ↔ payload helper atskills/lark-sheets/scripts/sheets_df.py).+table-put --styles(cell_styles / cell_merges / row_sizes / col_sizes).+workbook-createrework: typed--sheets+--styles(deep-merge cell/border styles, manual border validation); old--headers/--header-styleremoved (subsumed).+workbook-importwraps the drive import core;+workbook-exportrefactored to reuse the drive export core (inherits resume/cancel/sanitize/status labels).+sheet-show-gridline/+sheet-hide-gridline.--url/--spreadsheet-tokenaccept/wiki/<node_token>(resolved at Execute, Validate/DryRun stays network-free).aggregateTypegainscounta(non-empty count incl. text).--tokenalias for--spreadsheet-token; write-op error messages carry actionable hints (--print-schema, stdin fallback); stdin single-input constraint surfaced in flag help.stylesacceptshalign/valign/horizontal_align/vertical_alignas aliases for the canonical alignment fields.Notable removal
--dataframe/--dataframe-outchannel +apache/arrow/go/v17dep: an earlier revision of this branch added an Arrow IPC binary input/output channel to+table-put/+table-get/+workbook-create, plus a CIgo-licenses --ignore github.com/apache/arrow/go/v17exemption. It was removed before merge (commit9cd27855): the JSON--sheetspath already covers everything pandas users need, and the column-major / zero-copy benefits collapse the moment the CLI re-encodes into the row-oriented sheets OpenAPI JSON body. Pandas users keep the round-trip viaskills/lark-sheets/scripts/sheets_df.pyover the typed JSON path. Pre-removal state preserved on branchfeat/sheets-arrow-stash.Robustness & typed error contract (refactor / fix)
common.FlagErrorf→common.ValidationErrorf;+table-putpartial-success ported off legacyoutput.ExitErrortoruntime.OutPartialFailure; file-I/O and tool-call errors classified.+workbook-create: when the spreadsheet POST succeeds but the follow-up initial fill fails, the result now emits the sameOutPartialFailureenvelope shape as+table-put's partial write (ok:false stdout envelope withspreadsheet_token+reason+hint+ structuredcause), instead of the previousfailed_preconditionstderr envelope — so agents see one consistent "side effect landed but follow-up didn't" shape across the domain.+table-getnow defaults to the full used range (probed over the physical grid) instead of the A1 current_region — fixes silent truncation past internal blank rows; append mode's probe gets the same fix.rule_type(cross-field), with a schema-drift guard test — closes a class of dirty payloads that crashed the frontend on snapshot deserialization.+pivot-create:--target-positionand--rangeare mutually exclusive (both map toproperties.range).set_cell_rangemention_typeconstrained to the proto MENTION_FILE_TYPE enum.+cells-get/+csv-get--max-charsdefault raised 200000 → 500000.Skill / docs (synced from sheet-skill-spec)
set+Hquoting & guidance generalized; cheat-sheet adds hallucinated commands;+workbook-importrouting.+csv-putformula-with-comma example & RFC-4180 escaping; structured-write preference surfaced at the decision point.--max-charsexposed as a visible flag with "spill to file" guidance for large reads.read-datacross-checks+sheet-infofor hidden rows/cols and+workbook-infofor physical row_count.lark-sheetsskill version bumped to 3.0.0.Test Plan
go test -race ./cmd/... ./internal/... ./shortcuts/... ./extension/...go build ./...,golangci-lint run --new-from-rev=origin/main(0 issues),gofmtclean+table-put --styles(tests/cli_e2e/sheets/sheets_table_put_dryrun_test.go)+workbook-info,+cells-get,+csv-get,+sheet-create,+sheet-rename,+csv-put)+sheet-{show,hide}-gridline,+workbook-import,+workbook-exporttests/cli_e2e) — not run locally (requires live credentials)Related Issues
Summary by CodeRabbit
New Features
Enhancements
Removals
Documentation