Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 35 additions & 7 deletions shortcuts/sheets/csv_put_guard_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,33 +22,61 @@ func newCSVGuardRuntime(csvVal string) *common.RuntimeContext {
return &common.RuntimeContext{Cmd: cmd}
}

// TestGuardCSVValueIsNotFilePath covers the existing-file tier: a bare --csv
// value naming a real file is a forgotten "@". The prescription names the fix
// with a <path> placeholder — the untrusted value must not be spliced into
// TestGuardCSVValueIsNotFilePath covers the existing-file tier for a value
// that is NOT path-shaped: an inline value colliding with a real file name.
// Reading it would be a guess, so both routes are prescribed — with a <path>
// placeholder, since the untrusted value must not be spliced into
// command-shaped text an agent would copy verbatim.
func TestGuardCSVValueIsNotFilePath(t *testing.T) {
dir := t.TempDir()
cmdutil.TestChdir(t, dir)
if err := os.WriteFile("data.csv", []byte("a,b\n1,2\n"), 0644); err != nil {
if err := os.WriteFile("README.md", []byte("a,b\n1,2\n"), 0644); err != nil {
t.Fatal(err)
}

err := guardCSVValueIsNotFilePath(newCSVGuardRuntime("data.csv"))
err := guardCSVValueIsNotFilePath(newCSVGuardRuntime("README.md"))
ve := requireValidation(t, err, "existing file")
if !strings.Contains(ve.Message, `"data.csv"`) {
if !strings.Contains(ve.Message, `"README.md"`) {
t.Errorf("message should name the offending value as data, got: %q", ve.Message)
}
if !strings.Contains(ve.Message, "--csv @<path>") {
t.Errorf("message should prescribe the @ form via placeholder, got: %q", ve.Message)
}
if strings.Contains(ve.Message, "@data.csv") {
if strings.Contains(ve.Message, "@README.md") {
t.Errorf("message must not splice the value into a command fragment, got: %q", ve.Message)
}
if ve.Param != "--csv" {
t.Errorf("param = %q, want --csv", ve.Param)
}
}

// TestGuardCSVValueReadsForgottenAtPath covers the tier the 08-29..31 reflow
// added: a path-SHAPED value naming a real file is a forgotten "@" and nothing
// else, so the file is read and the substitution is reported in the envelope
// instead of costing a round trip.
func TestGuardCSVValueReadsForgottenAtPath(t *testing.T) {
dir := t.TempDir()
cmdutil.TestChdir(t, dir)
if err := os.WriteFile("data.csv", []byte("a,b\n1,2\n"), 0644); err != nil {
t.Fatal(err)
}

runtime := newCSVGuardRuntime("./data.csv")
if err := guardCSVValueIsNotFilePath(runtime); err != nil {
t.Fatalf("a path-shaped existing file should be read, got: %v", err)
}
if got := runtime.Str("csv"); got != "a,b\n1,2\n" {
t.Errorf("--csv = %q, want the file contents", got)
}
if !runtime.InputResolvedFromSource("csv") {
t.Error("the value came from a file, so it must be marked resolved")
}
warnings := csvForgottenAtWarnings(runtime)
if len(warnings) != 1 || !strings.Contains(warnings[0], "@./data.csv") {
t.Errorf("warnings = %v, want one naming the explicit @ form", warnings)
}
}

// TestGuardCSVValueIsNotFilePath_MissingButPathShaped covers the second tier.
// A path that doesn't resolve used to pass through and be written into the
// cell verbatim — a wrong value with a success exit code. The common source is
Expand Down
74 changes: 74 additions & 0 deletions shortcuts/sheets/execute_paths_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package sheets
import (
"encoding/json"
"errors"
"net/http"
"strings"
"testing"

Expand Down Expand Up @@ -1185,3 +1186,76 @@ func decodeRawEnvelopeBody(t *testing.T, raw []byte) map[string]interface{} {
}
return body
}

// TestExecute_TransientReadRetry pins the read-only retry: an identical read
// is reissued when the tool answers with its own timeout wording, and the
// write path is never reissued because this API has no idempotency key.
func TestExecute_TransientReadRetry(t *testing.T) {
t.Parallel()

timeoutBody := map[string]interface{}{
"code": 1310299, "msg": "server time out error", "data": map[string]interface{}{},
}

t.Run("a read retries past a tool timeout", func(t *testing.T) {
t.Parallel()
parent, stdout, _, reg := newTestRig(t, WorkbookInfo)
calls := 0
count := func(*http.Request) { calls++ }
readURL := "/open-apis/sheet_ai/v2/spreadsheets/" + testToken + "/tools/invoke_read"
// Stubs are served in registration order, so the first try fails and
// the retry meets the success stub.
reg.Register(&httpmock.Stub{Method: "POST", URL: readURL, Body: timeoutBody, OnMatch: count})
reg.Register(&httpmock.Stub{Method: "POST", URL: readURL, OnMatch: count, Body: map[string]interface{}{
"code": 0, "msg": "success",
"data": map[string]interface{}{"output": `{"sheets":[]}`},
}})
parent.SetArgs([]string{"+workbook-info", "--url", testURL})
if err := parent.Execute(); err != nil {
t.Fatalf("the second try should succeed, got: %v", err)
}
if calls != 2 {
t.Errorf("calls = %d, want 2 (one retry)", calls)
}
if !strings.Contains(stdout.String(), `"ok": true`) {
t.Errorf("stdout should carry the successful read, got %q", stdout.String())
}
})

t.Run("a persistent failure surfaces after the attempts are spent", func(t *testing.T) {
t.Parallel()
parent, _, _, reg := newTestRig(t, WorkbookInfo)
calls := 0
reg.Register(&httpmock.Stub{
Method: "POST", URL: "/open-apis/sheet_ai/v2/spreadsheets/" + testToken + "/tools/invoke_read",
Body: timeoutBody, Reusable: true, OnMatch: func(*http.Request) { calls++ },
})
parent.SetArgs([]string{"+workbook-info", "--url", testURL})
if err := parent.Execute(); err == nil {
t.Fatal("expected the failure to surface")
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
if calls != readRetryAttempts {
t.Errorf("calls = %d, want %d", calls, readRetryAttempts)
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
}
})

t.Run("a write is never reissued", func(t *testing.T) {
t.Parallel()
parent, _, _, reg := newTestRig(t, CellsSet)
calls := 0
reg.Register(&httpmock.Stub{
Method: "POST", URL: "/open-apis/sheet_ai/v2/spreadsheets/" + testToken + "/tools/invoke_write",
Body: timeoutBody, Reusable: true, OnMatch: func(*http.Request) { calls++ },
})
parent.SetArgs([]string{"+cells-set", "--url", testURL, "--sheet-name", "s",
"--range", "A1:A1", "--cells", `[[{"value":"x"}]]`})
if err := parent.Execute(); err == nil {
t.Fatal("expected the failure to surface")
}
// A create that timed out after the backend committed it must not be
// committed twice.
if calls != 1 {
t.Errorf("calls = %d, want 1 (writes are not retried)", calls)
}
})
}
170 changes: 157 additions & 13 deletions shortcuts/sheets/flag_ergonomics.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
chainEnumNormalization(cmd)
chainFlagAliases(cmd)
chainRangeSheetPrefix(cmd)
chainMultiAreaRange(cmd)
chainRequiredFlagHelp(cmd)
}
}
Expand Down Expand Up @@ -107,21 +108,26 @@
// actually registered. Only pairs with identical value semantics belong
// here: the rewrite is invisible, so it must be safe to apply unread.
//
// +csv-put's file → csv is the one entry whose value semantics differ, and it
// carries its own value-side rule to make them match: --file names a path by
// definition, so the value is read as one (resolveCSVPathFromFileAlias) rather
// than being written into the sheet as literal text. Without that, the alias
// itself manufactured a failure — an agent that wrote `--file ./data.csv` got
// "--csv value is an existing file", an error about a flag it never typed.
// +csv-put's file / csv-file → csv are the entries whose value semantics
// differ, and they carry their own value-side rule to make them match: both
// name a path by definition, so the value is read as one
// (resolveCSVPathFromFileAlias) rather than being written into the sheet as
// literal text. Without that, the alias itself manufactured a failure — an
// agent that wrote `--file ./data.csv` got "--csv value is an existing file",
// an error about a flag it never typed.
var commandFlagAliases = map[string]map[string]string{
"+csv-put": {"file": "csv"},
"+sheet-create": {"name": "title"},
// data / content name the payload the way sibling CLIs do and carry
// --csv's own value semantics (inline text, @file or -), so they are pure
// renames. csv-file joins file on the path-valued side. 08-29..31 reflow:
// 8 of +csv-put's 29 rejections were one of these four names.
"+csv-put": {"file": "csv", "csv-file": "csv", "data": "csv", "content": "csv"},
"+sheet-create": {"name": "title", "sheet-name": "title"},
// The new name is the only name-valued input a rename takes, so the
// habitual spellings are unambiguous (unlike +sheet-copy, where a name
// could mean the copy's title or the source selector and gets a
// prescription instead). 07-28 root-cause report #25: 10/10 wrote
// --new-name, 24 occurrences.
"+sheet-rename": {"name": "title", "new-name": "title"},
"+sheet-rename": {"name": "title", "new-name": "title", "new-title": "title"},
// size → width/height: the styles protocol (--styles row_sizes/col_sizes)
// spells the pixel dimension "size", and pre-2026-07 batches accepted it
// here too — the rename is the single largest sub-op error cluster in
Expand All @@ -140,6 +146,18 @@
// into {"value":…}, so a --values matrix ('[["工作内容"]]') is accepted
// verbatim as --cells — the name was the only thing wrong.
"+cells-set": {"values": "cells"},
// 08-29..31 reflow, long-tail table. Each of these names an input the
// command already has under one other spelling, with identical value
// semantics: the import name (16 rejections, all but one on windows),
// the export destination (13 + 2), and the replacement text (7). None is
// within the did-you-mean budget -- "title" shares no prefix with "name",
// and "replace" is 4 edits from "replacement".
"+workbook-import": {"title": "name"},
"+workbook-export": {"file": "output-path", "outdir": "output-path", "output-dir": "output-path", "output": "output-path"},
Comment thread
xiongyuanwen-byted marked this conversation as resolved.
"+cells-replace": {"replace": "replacement"},
"+csv-get": {"output": "output-path"},
// +sheet-create already answers to "name"; new-title joins new-name on
// +sheet-rename for the same reason.
}

// intuitiveFlagHints carries the prescription for habitual names whose fix
Expand Down Expand Up @@ -173,8 +191,17 @@
"underline": "use --font-line underline",
"font-bold": "use --font-weight bold",
"bg-color": "use --background-color",
// Google Sheets API vocabulary (wrapStrategy).
// Google Sheets API vocabulary (wrapStrategy), plus the openpyxl / CSS
// wrap spellings (08-29..31 reflow: 4 of the 22 unknown-flag
// rejections). Not silent renames — the values differ too
// (--wrap-text true vs --word-wrap auto-wrap).
"wrap-strategy": "use --word-wrap (overflow / auto-wrap / word-clip)",
"wrap-text": "use --word-wrap (overflow / auto-wrap / word-clip)",
"text-wrap": "use --word-wrap (overflow / auto-wrap / word-clip)",
"wrap": "use --word-wrap (overflow / auto-wrap / word-clip)",
// There is no composite style flag here: the OpenAPI's {style:{…}}
// envelope is one flat flag per field on this command.
"style": "there is no single --style flag — pass each field on its own: --font-weight, --font-style, --font-color, --background-color, --font-size, --border-styles",
// The border family: the only border flag is --border-styles (composite
// JSON); color and per-side variants ride inside it.
"border-style": `borders take one composite flag: --border-styles '{"all":{"style":"solid","weight":"thin","color":"#000000"}}' (sides: top/bottom/left/right, or "all" for all four)`,
Expand All @@ -191,6 +218,7 @@
"styles": `range-level styling goes through +styles-put (same {"styles":[...]} vocabulary); per-cell styles ride inside the cells objects as cell_styles`,
},
"+table-put": {
"payload": `the sub-sheet payload flag is --sheets ({"sheets":[{"name":"Sheet1","columns":[…],"data":[…]}]}); --values takes an untyped 2D array instead`,
"start-cell": `anchor each sub-sheet via the "start_cell" field inside --sheets (e.g. {"sheets":[{"name":"Sheet1","start_cell":"B2",…}]}); to paste CSV at a cell use +csv-put --start-cell`,
"sheet-name": `+table-put has no sheet selector — each --sheets item carries its own "name" field ({"sheets":[{"name":"Sheet1",…}]})`,
"sheet-id": `+table-put has no sheet selector — each --sheets item carries its own "name" field ({"sheets":[{"name":"Sheet1",…}]})`,
Expand All @@ -202,6 +230,34 @@
"+chart-config-update": {
"show-labels": "use --data-labels value (or any value/category/percentage combination such as value_category_percentage; use series for series names or none to hide labels)",
},
// 08-29..31 reflow, long-tail table. These name a real input, but the fix
// is not a rename: the value moves to a differently-shaped flag, or the
// command does not carry that concept at all.
"+dim-delete": {
// 18 rejections, the largest single long-tail entry. +dim-insert does
// take --position, so the habit carries over to its sibling, where
// rows and columns are named by an A1 span instead.
"position": `+dim-delete names what to remove with --range: "3:5" deletes rows 3 through 5, "C:E" deletes columns C through E`,
"index": `+dim-delete names what to remove with --range: "3:5" deletes rows 3 through 5, "C:E" deletes columns C through E`,
"dimension": `+dim-delete infers rows vs columns from --range: "3:5" is rows, "C:E" is columns`,
},
"+cells-unmerge": {
"ranges": `+cells-unmerge takes one span per call: --range "A1:B2"; unmerge several regions with several calls (or one +batch-update carrying them all)`,
},
"+csv-get": {
"include": "+csv-get returns values only; for formulas / styles / comments use +cells-get --include formula,style",
"include-all": "+csv-get returns values only; for formulas / styles / comments use +cells-get --include formula,style",
},
"+cells-get": {
"value-only": "+cells-get returns values by default; --include adds categories on top, so drop this flag (or narrow the output with --jq)",
},
"+workbook-import": {
"output-path": "+workbook-import uploads a local file and returns the new spreadsheet's token and url; it writes nothing locally. Capture the JSON result instead, or use +workbook-export --output-path to pull a sheet back down",
},
"+styles-put": {
"sheet-name": `+styles-put has no sheet selector -- each --styles item carries its own "name" field ({"styles":[{"name":"Sheet1","cell_styles":[…]}]})`,
"sheet-id": `+styles-put has no sheet selector -- each --styles item carries its own "name" field ({"styles":[{"name":"Sheet1","cell_styles":[…]}]})`,
},
}

// chainFlagAliases installs an invisible flag-name rewrite (via
Expand Down Expand Up @@ -335,13 +391,30 @@
return "lark-cli/sheets-alias-source/" + canonical
}

// pathValuedCSVAliases are the +csv-put spellings whose value is a path rather
// than CSV text (see resolveCSVPathFromFileAlias). The other two aliases
// (data / content) carry --csv's own semantics and need no value-side rule.
var pathValuedCSVAliases = []string{"file", "csv-file"}

// aliasSpellingUsed returns the habitual spelling that supplied canonical's
// value, or fallback when the annotation is missing.
func aliasSpellingUsed(cmd *cobra.Command, canonical, fallback string) string {
if cmd == nil {
return fallback

Check warning on line 403 in shortcuts/sheets/flag_ergonomics.go

View check run for this annotation

Codecov / codecov/patch

shortcuts/sheets/flag_ergonomics.go#L403

Added line #L403 was not covered by tests
}
if used := cmd.Annotations[aliasSourceAnnotation(canonical)]; used != "" {
return used
}
return fallback

Check warning on line 408 in shortcuts/sheets/flag_ergonomics.go

View check run for this annotation

Codecov / codecov/patch

shortcuts/sheets/flag_ergonomics.go#L408

Added line #L408 was not covered by tests
}

// flagValueCameFromAlias reports whether canonical's value was supplied under
// the given habitual spelling on this invocation.
func flagValueCameFromAlias(cmd *cobra.Command, canonical, alias string) bool {
// any of the given habitual spellings on this invocation.
func flagValueCameFromAlias(cmd *cobra.Command, canonical string, aliases ...string) bool {
if cmd == nil {
return false
}
return cmd.Annotations[aliasSourceAnnotation(canonical)] == alias
return slices.Contains(aliases, cmd.Annotations[aliasSourceAnnotation(canonical)])
}

// sheetsFlagErrorFunc overrides the root FlagErrorFunc for sheets commands.
Expand Down Expand Up @@ -459,6 +532,69 @@
return strings.Join(parts, ", ") + suffix
}

// chainMultiAreaRange rejects an Excel multi-area --range on any command that
// takes one. It is chained AFTER chainRangeSheetPrefix so a sheet prefix has
// already moved into --sheet-name: a sheet whose name contains a comma must
// not be read as several areas.
func chainMultiAreaRange(cmd *cobra.Command) {
prev := cmd.PreRunE
cmd.PreRunE = func(c *cobra.Command, args []string) error {
if prev != nil {
if err := prev(c, args); err != nil {
return err
}
}
if want, err := c.Flags().GetBool("print-schema"); err == nil && want {
return nil

Check warning on line 548 in shortcuts/sheets/flag_ergonomics.go

View check run for this annotation

Codecov / codecov/patch

shortcuts/sheets/flag_ergonomics.go#L548

Added line #L548 was not covered by tests
}
rng, err := c.Flags().GetString("range")
if err != nil {
return nil //nolint:nilerr // the command has no plain --range; nothing to check
}
return rejectMultiAreaRange(rng)
}
}

// rejectMultiAreaRange answers the Excel multi-area habit — several
// non-adjacent cells joined by commas ("A3,G3,H3") — before it reaches the
// backend, which rejects it as an opaque "[90015206] invalid range" (or, on a
// write path, as "invalid cell ref"). A single A1 range never contains a
// comma, so the shape is unambiguous; the fix is not (the enclosing
// rectangle, or one call per area), so this prescribes both rather than
// picking one. 08-29..31 reflow: 54 of +cells-get's 69 rejections, 5 more on
// +csv-get and 4 on +cells-set-style — the habit is not specific to reads,
// which is why the check rides the shared --range chain.
func rejectMultiAreaRange(rng string) error {
rng = strings.TrimSpace(rng)
if !strings.Contains(rng, ",") {
return nil
}
areas := strings.Split(rng, ",")
first, last := strings.TrimSpace(areas[0]), strings.TrimSpace(areas[len(areas)-1])
hint := "use the enclosing rectangle in one call"
if enclosing := enclosingRangeHint(first, last); enclosing != "" {
hint = fmt.Sprintf("use the enclosing rectangle in one call (--range %q)", enclosing)
}
return sheetsValidationForFlag("range", "--range %q lists %d separate areas; one call takes ONE continuous A1 range", rng, len(areas)).
WithHint("%s, or issue one call per area — several areas in one request go through +batch-update, which carries a separate op per area", hint)
}

// enclosingRangeHint spells the rectangle covering the caller's first and last
// area, so the prescription carries a range they can paste. Empty when either
// end is not a plain cell reference — a guess is worse than the generic hint.
func enclosingRangeHint(first, last string) string {
if first == "" || last == "" || strings.Contains(first, ":") || strings.Contains(last, ":") {
return ""
}
if _, _, ok := splitCellRef(first); !ok {
return ""

Check warning on line 590 in shortcuts/sheets/flag_ergonomics.go

View check run for this annotation

Codecov / codecov/patch

shortcuts/sheets/flag_ergonomics.go#L590

Added line #L590 was not covered by tests
}
if _, _, ok := splitCellRef(last); !ok {
return ""

Check warning on line 593 in shortcuts/sheets/flag_ergonomics.go

View check run for this annotation

Codecov / codecov/patch

shortcuts/sheets/flag_ergonomics.go#L593

Added line #L593 was not covered by tests
}
return first + ":" + last
}

// ─── enum vocabulary normalization ──────────────────────────────────────

// enumAliases maps habitual values agents import from CSS / Excel / Google
Expand All @@ -484,6 +620,14 @@
// the first two need mapping — overflow is spelled the same in both.
"wrap": "auto-wrap",
"clip": "word-clip",
// CSS text-decoration vocabulary for --font-line, whose Lark spelling is
// the hyphenated line-through. 08-29..31 reflow: every one of the 8
// --font-line rejections was one of these words.
"strikethrough": "line-through",
"strike": "line-through",
"line_through": "line-through",
"linethrough": "line-through",
"underlined": "underline",
// Combined chart data-label vocabulary emitted by models. The tool enum
// spells the same intent as one value.
"percentage,value": "value_percentage",
Expand Down
Loading
Loading