Skip to content
Open
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
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"},
"+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