Skip to content

feat(sheets): typed table I/O, workbook import/export, and lark-sheets skill refresh#1355

Open
xiongyuanwen-byted wants to merge 27 commits into
mainfrom
feat/lark-sheets-develop
Open

feat(sheets): typed table I/O, workbook import/export, and lark-sheets skill refresh#1355
xiongyuanwen-byted wants to merge 27 commits into
mainfrom
feat/lark-sheets-develop

Conversation

@xiongyuanwen-byted

@xiongyuanwen-byted xiongyuanwen-byted commented Jun 9, 2026

Copy link
Copy Markdown
Collaborator

Summary

Brings the feat/lark-sheets-develop line up to date with main (merged in, no conflicts) and lands the accumulated Lark Sheets work for review into main: typed table I/O, workbook import/export, gridline shortcuts, and a broad lark-sheets skill/doc refresh.

Changes

  • Typed table I/O: new +table-put / +table-get shortcuts for DataFrame-style typed import/export with column type & number-format preservation (lark_sheet_table_io.go, +1148 LOC with tests).
  • Workbook import/export: new +workbook-import wrapping the drive import core; +workbook-export refactored to reuse the drive export core (drive_import.go / drive_export.go).
  • Gridline shortcuts: +sheet-show-gridline / +sheet-hide-gridline.
  • Skill / docs: skills/lark-sheets refresh — new financial-modeling-standards reference plus chart / filter / read-data / write-cells / workbook references and SKILL.md routing updates.
  • Fixes (surfaced by CI after the merge): regenerated flag_defs_gen.go to match data/flag-defs.json (codegen drift); tightened rows typing in lark_sheet_table_io.go to clear an asasalint finding.

Test Plan

  • Unit tests pass — go test -race ./cmd/... ./internal/... ./shortcuts/... ./extension/...
  • go build ./..., golangci-lint run --new-from-rev=origin/main (0 issues), and gofmt all clean
  • Manual local lark-cli sheets <command> end-to-end verification (not run this round)
  • e2e suite (tests/cli_e2e) — not run locally (requires live credentials)

Related Issues

  • None

Summary by CodeRabbit

  • New Features

    • Typed table read/write: +table-get and +table-put.
    • Sheet gridline toggles: +sheet-show-gridline and +sheet-hide-gridline.
    • Workbook import shortcut and typed --sheets support for workbook creation.
  • Enhancements

    • CSV input treats =-prefixed cells as formulas.
    • Export/import UX: empty output-dir now normalizes to current dir; exports produce resumable dry-run/execute envelopes and clearer download semantics (including markdown fetch).
  • Removals

    • Removed +csv-get --rows-json (use +table-get).
  • Documentation

    • Added financial-modeling standards and expanded sheets references.

zhengzhijiej-tech and others added 21 commits June 4, 2026 17:00
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
@CLAassistant

CLAassistant commented Jun 9, 2026

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 3 committers have signed the CLA.

✅ xiongyuanwen-byted
✅ zhengzhijiej-tech
❌ Chenweifeng-bd
You have signed the CLA already but the status is still pending? Let us recheck it.

@github-actions github-actions Bot added domain/ccm PR touches the ccm domain size/XL Architecture-level or global-impact change labels Jun 9, 2026
@coderabbitai

coderabbitai Bot commented Jun 9, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

This 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.

Changes

Sheets Typed Table Protocol & Workbook Integration

Layer / File(s) Summary
Drive Export/Import Core Extraction
shortcuts/drive/drive_export.go, shortcuts/drive/drive_import.go, shortcuts/drive/drive_export_test.go
ExportParams/ImportParams structs and exported helpers (ValidateExport, PlanExportDryRun, RunExport, etc.) encapsulate validation, planning, and execution logic; --output-dir "" is normalized to ".". Tests cover empty --output-dir download behavior.
Typed Table Protocol (TablePut & TableGet)
shortcuts/sheets/lark_sheet_table_io.go, shortcuts/sheets/lark_sheet_table_io_test.go
Implements typed table read/write: payload parsing, validation, typed cell conversion (ISO date ↔ serial), matrix building, batched set_cell_range writes, append/overwrite modes, partial-failure envelopes, and comprehensive unit tests plus dry-run behavior for writes and reads.
Workbook Operations Refactoring & Typed Sheet Creation
shortcuts/sheets/lark_sheet_workbook.go, shortcuts/sheets/lark_sheet_workbook_export_test.go, shortcuts/sheets/lark_sheet_workbook_import_test.go
WorkbookExport/WorkbookImport now delegate to drive core helpers; WorkbookCreate gains a typed --sheets mode (XOR with --values) that uses the TablePut payload parsing/write helpers. Tests cover export-only behavior, import type pinning, and typed create flows.
Sheet Visibility Shortcuts & Batch Operations
shortcuts/sheets/lark_sheet_workbook.go, shortcuts/sheets/batch_op_dispatch.go, shortcuts/sheets/batch_op_contract_test.go, shortcuts/sheets/data/flag-defs.json, shortcuts/sheets/flag_defs_gen.go
Adds +sheet-show-gridline and +sheet-hide-gridline, integrates them into batch dispatch, and adds dry-run contract tests.
CSV Refinements & Flag Schema Updates
shortcuts/sheets/lark_sheet_read_data.go, shortcuts/sheets/lark_sheet_read_data_test.go, shortcuts/sheets/lark_sheet_write_cells.go, shortcuts/sheets/data/flag-defs.json, shortcuts/sheets/data/flag-schemas.json, shortcuts/sheets/flag_defs_gen.go, shortcuts/sheets/flag_schemas_gen.go, shortcuts/sheets/shortcuts.go, tests/cli_e2e/sheets/sheets_crud_workflow_test.go
Removes --rows-json from CsvGet and adds --include-row-prefix; documents CsvPut formula semantics; updates generated flag metadata and schemas to add +table-get/+table-put/+workbook-import and WorkbookCreate --sheets. Updates e2e test to call sheets +workbook-export.
Object CRUD & Pivot Mutex
shortcuts/sheets/lark_sheet_object_crud.go, shortcuts/sheets/lark_sheet_object_crud_test.go
Adds validateCreateInput hook and maps --target-position/--range into properties.range with a validation mutex for conflicting inputs; adds tests covering the mutex behavior.
Skills & Reference Documentation
skills/lark-sheets/SKILL.md, skills/lark-sheets/references/*.md
Adds a financial-modeling standards doc and updates various sheets references to document typed workflows, new shortcuts, CSV/formula semantics, and guardrails for write operations.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

  • larksuite/cli#1264: Related to adding sheet gridline shortcuts and dispatch/contract wiring for those operations.
  • larksuite/cli#194: Refactors Drive export/import logic touching shortcuts/drive/drive_export.go and drive_import.go.
  • larksuite/cli#360: Overlaps on Drive import grant/permission handling and import execution flow.

Suggested labels

feature

Suggested reviewers

  • caojie0621
  • zhengzhijiej-tech
  • fangshuyu-768

Poem

"🐇 I hopped through rows and typed each cell,
dates turned to serials and back so well.
TablePut wrote, TableGet read true,
workbooks export, imports stitch through.
A carrot-coded cheer for sheets anew!"

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/lark-sheets-develop
⚔️ Resolve merge conflicts
  • Resolve merge conflict in branch feat/lark-sheets-develop

@github-actions

github-actions Bot commented Jun 9, 2026

Copy link
Copy Markdown

🚀 PR Preview Install Guide

🧰 CLI update

npm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@773b93cb102e7781eeb6efbd4299ab60149455bc

🧩 Skill update

npx skills add larksuite/cli#feat/lark-sheets-develop -y -g

@codecov

codecov Bot commented Jun 9, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 82.18218% with 178 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.96%. Comparing base (eed711b) to head (773b93c).

Files with missing lines Patch % Lines
shortcuts/sheets/lark_sheet_table_io.go 81.60% 66 Missing and 51 partials ⚠️
shortcuts/drive/drive_export.go 83.78% 23 Missing and 7 partials ⚠️
shortcuts/sheets/lark_sheet_workbook.go 80.00% 12 Missing and 4 partials ⚠️
shortcuts/drive/drive_import.go 83.72% 7 Missing and 7 partials ⚠️
shortcuts/sheets/lark_sheet_read_data.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1355      +/-   ##
==========================================
+ Coverage   71.79%   71.96%   +0.17%     
==========================================
  Files         690      691       +1     
  Lines       65596    66121     +525     
==========================================
+ Hits        47094    47584     +490     
+ Misses      14845    14835      -10     
- Partials     3657     3702      +45     

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

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 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 win

Add gridline shortcuts to +batch-update.operations.shortcut enum.

+sheet-show-gridline and +sheet-hide-gridline are now batchable in dispatch/tests, but they’re missing from the schema enum, so schema-validated +batch-update payloads 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

📥 Commits

Reviewing files that changed from the base of the PR and between 2b4c634 and 82a9838.

📒 Files selected for processing (31)
  • shortcuts/drive/drive_export.go
  • shortcuts/drive/drive_export_test.go
  • shortcuts/drive/drive_import.go
  • shortcuts/sheets/batch_op_contract_test.go
  • shortcuts/sheets/batch_op_dispatch.go
  • shortcuts/sheets/data/flag-defs.json
  • shortcuts/sheets/data/flag-schemas.json
  • shortcuts/sheets/flag_defs_gen.go
  • shortcuts/sheets/flag_schemas_gen.go
  • shortcuts/sheets/lark_sheet_read_data.go
  • shortcuts/sheets/lark_sheet_read_data_test.go
  • shortcuts/sheets/lark_sheet_table_io.go
  • shortcuts/sheets/lark_sheet_table_io_test.go
  • shortcuts/sheets/lark_sheet_workbook.go
  • shortcuts/sheets/lark_sheet_workbook_export_test.go
  • shortcuts/sheets/lark_sheet_workbook_import_test.go
  • shortcuts/sheets/lark_sheet_workbook_test.go
  • shortcuts/sheets/lark_sheet_write_cells.go
  • shortcuts/sheets/shortcuts.go
  • skills/lark-sheets/SKILL.md
  • skills/lark-sheets/references/lark-sheets-chart.md
  • skills/lark-sheets/references/lark-sheets-core-operations.md
  • skills/lark-sheets/references/lark-sheets-filter-view.md
  • skills/lark-sheets/references/lark-sheets-filter.md
  • skills/lark-sheets/references/lark-sheets-financial-modeling-standards.md
  • skills/lark-sheets/references/lark-sheets-range-operations.md
  • skills/lark-sheets/references/lark-sheets-read-data.md
  • skills/lark-sheets/references/lark-sheets-visual-standards.md
  • skills/lark-sheets/references/lark-sheets-workbook.md
  • skills/lark-sheets/references/lark-sheets-write-cells.md
  • tests/cli_e2e/sheets/sheets_crud_workflow_test.go
💤 Files with no reviewable changes (1)
  • shortcuts/sheets/lark_sheet_read_data_test.go

Comment on lines +176 to +181
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
_, err := parseTablePutPayload(stubFlagView{"sheets": tt.json})
if err == nil || !strings.Contains(err.Error(), tt.want) {
t.Errorf("want error containing %q, got %v", tt.want, err)
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Error-path tests should assert typed problem metadata, not just message substrings.

These assertions currently rely on strings.Contains(...), so category/subtype/param/cause regressions can slip through unnoticed. Please assert errs.ProblemOf(err) metadata (and preserved cause) on error paths.

As per coding guidelines, “Error-path tests must assert typed metadata via errs.ProblemOf (category/subtype/param) and cause preservation, not message substrings alone.”

Also applies to: 291-293, 445-450, 472-479, 502-504, 518-520, 672-674

🤖 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/lark_sheet_table_io_test.go` around lines 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.

Source: Coding guidelines

Comment on lines +371 to +380
func tablePutFullRange(s *tableSheetSpec, totalRows int) string {
_, col0, row0, err := sheetAnchor(s)
if err != nil {
return strings.TrimSpace(s.StartCell)
}
ncols := len(s.Columns)
return fmt.Sprintf("%s%d:%s%d",
columnIndexToLetter(col0), row0+1,
columnIndexToLetter(col0+ncols-1), row0+totalRows)
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Dry-run can render an invalid range when there is nothing to write.

When header=false and rows=[], len(matrix)==0, so tablePutFullRange(..., 0) can produce an invalid end row (e.g., A1:C0) in preview output. Return an empty/no-op range string for this case.

Also applies to: 730-733

🤖 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/lark_sheet_table_io.go` around lines 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.

Comment on lines +399 to +401
lastRow, err := lastDataRow(ctx, runtime, token, sheetID)
if err != nil {
return nil, fmt.Errorf("resolving last data row for append: %w", err)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Use typed errs.* errors and avoid downgrading lower-layer typed errors.

These paths currently re-wrap with fmt.Errorf and return output.Errorf / output.ExitError, which drops the typed error contract expected by command-facing failures.

As per coding guidelines, “Command-facing failures must be typed errs.* errors — never use legacy output.Err* helpers or bare fmt.Errorf,” and “Pass through lower-layer typed errors unchanged — re-wrapping downgrades their classification.”

Also applies to: 529-550, 653-653, 679-706

🤖 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/lark_sheet_table_io.go` around lines 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*.

Source: Coding guidelines

Comment on lines +836 to +838
if id != "" {
return []tableGetSheet{{id: id}}, nil
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Preserve a non-empty sheet name when --sheet-id is used.

Line 837 returns a target with only id, and Line 872 emits name from t.name, so the output can contain name: "". That breaks +table-get -> +table-put round-tripping because +table-put requires non-empty sheet names.

💡 Minimal fix
 if id != "" {
-    return []tableGetSheet{{id: id}}, nil
+    // Prefer resolving real sheet name from workbook structure; fallback keeps output valid.
+    return []tableGetSheet{{id: id, name: id}}, nil
 }

Also applies to: 872-873

🤖 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/lark_sheet_table_io.go` around lines 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.

xiongyuanwen-byted and others added 2 commits June 9, 2026 19:52
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.
zhengzhijiej-tech and others added 4 commits June 11, 2026 16:45
…n +pivot-create

Both flags map to the same wire field (properties.range), so passing
non-default values for both is ambiguous. Mirror the
--target-sheet-id / --target-sheet-name mutex pattern: --target-position
takes priority over --range, and supplying both with non-default values
is rejected up front with a typed FlagErrorf. --target-position=A1 is
the documented default and is treated as "not set".

Add a symmetric validateCreateInput hook on objectCRUDSpec (alongside
the existing validateUpdateInput), wire it into objectCreateInput, and
inject the pivot-specific check on pivotSpec.
feat(sheets): add counta to chart aggregateType enum
- --values builds a type-less typed payload, writing through --sheets' batched set_cell_range path (raw passthrough preserves auto-detect; large tables batch; big ints via json.Number)
- drop --headers (subsumed by --values first row) and --header-style (typed header no longer auto-bold; use --styles instead)
- styles: deep-merge overlapping cell_styles/border_styles fields (was wholesale-replace which dropped fields); add manual border_styles validation (style/weight enums + sides) since --styles is on parseJSONFlagSkip and bypasses the schema validator
- regenerate flag-defs/flag-schemas/skills mirror from sheet-skill-spec (--styles flag + full per-side border schema)

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
shortcuts/sheets/lark_sheet_workbook_test.go (1)

431-450: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Use typed error assertions instead of substring matching for these new negative-path cases.

These new cases assert failures via message substrings, which is brittle and misses typed error contract checks (category/subtype/param + cause). Please assert via typed error metadata (errs.ProblemOf and errors.As for validation Param) for each error-path case.

As per coding guidelines, “**/*_test.go: Error-path tests must assert typed metadata via errs.ProblemOf (category / subtype / param) and cause preservation, not message substrings alone.”

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

In `@shortcuts/sheets/lark_sheet_workbook_test.go` around lines 431 - 450, The
test currently checks failure cases by substring matching on stdout/stderr/err;
update the table-driven tests using WorkbookCreate and runShortcutCapturingErr
to assert typed error metadata instead: for each case ensure err != nil, call
errs.ProblemOf(err) and compare the returned Problem's Category/Subtype/Param to
the expected values for that scenario, and use errors.As to extract and assert
the concrete validation Param/cause where applicable to preserve the underlying
cause; replace the strings.Contains(...) assertion block with these typed checks
(using errs.ProblemOf and errors.As) so tests validate the error contract rather
than message substrings.

Source: Coding guidelines

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

Outside diff comments:
In `@shortcuts/sheets/lark_sheet_workbook_test.go`:
- Around line 431-450: The test currently checks failure cases by substring
matching on stdout/stderr/err; update the table-driven tests using
WorkbookCreate and runShortcutCapturingErr to assert typed error metadata
instead: for each case ensure err != nil, call errs.ProblemOf(err) and compare
the returned Problem's Category/Subtype/Param to the expected values for that
scenario, and use errors.As to extract and assert the concrete validation
Param/cause where applicable to preserve the underlying cause; replace the
strings.Contains(...) assertion block with these typed checks (using
errs.ProblemOf and errors.As) so tests validate the error contract rather than
message substrings.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: ea9b5e28-5ad3-4f14-aa4f-17d32e3d2b56

📥 Commits

Reviewing files that changed from the base of the PR and between cf3c5f1 and a72331d.

📒 Files selected for processing (12)
  • shortcuts/sheets/data/flag-defs.json
  • shortcuts/sheets/data/flag-schemas.json
  • shortcuts/sheets/execute_paths_test.go
  • shortcuts/sheets/flag_defs_gen.go
  • shortcuts/sheets/flag_schema_validate.go
  • shortcuts/sheets/lark_sheet_table_io.go
  • shortcuts/sheets/lark_sheet_table_io_test.go
  • shortcuts/sheets/lark_sheet_workbook.go
  • shortcuts/sheets/lark_sheet_workbook_test.go
  • skills/lark-sheets/SKILL.md
  • skills/lark-sheets/references/lark-sheets-workbook.md
  • skills/lark-sheets/references/lark-sheets-write-cells.md
💤 Files with no reviewable changes (1)
  • skills/lark-sheets/references/lark-sheets-write-cells.md
✅ Files skipped from review due to trivial changes (1)
  • skills/lark-sheets/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (4)
  • shortcuts/sheets/lark_sheet_table_io_test.go
  • shortcuts/sheets/data/flag-schemas.json
  • shortcuts/sheets/lark_sheet_table_io.go
  • shortcuts/sheets/lark_sheet_workbook.go

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

domain/ccm PR touches the ccm domain size/XL Architecture-level or global-impact change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants