From f13e431538d9fb74eda34a1c63e8790e3d788cc2 Mon Sep 17 00:00:00 2001 From: xiongyuanwen-byted Date: Sat, 29 Aug 2026 14:40:49 +0800 Subject: [PATCH 1/4] feat(sheets): cut the top command-error clusters from the 08-18..24 eval batch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A trace analysis over 14,818 `lark-cli sheets` calls attributed 2,036 command errors to 57 (subcommand, flag) groups, with the top 6 covering 76%. Four of them are ours to fix; each is addressed at the layer that produced it. --styles vocabulary (291 cases, the only group whose retry also failed): - Prescribe the border family as a whole. foldBorderFamilyAliases already absorbs border / borders / border_ / border_; what still reached the error path was the Lark OpenAPI's border_type (FULL_BORDER, OUTER_BORDER) and CSS's border_width — real vocabularies with no equivalent here, and border_type was the single top field in the group. Neither maps unambiguously onto a per-side style/weight/color triple, so they get one shared answer, not a silent alias. - Prescribe the OpenAPI's nested {range, style:{...}} envelope, plus bg_color / fill_color / text_color. - Match prescriptions on the key's letters alone, so border_type, borderType and border-type are one mistake, not three. - Collapse repeated issues in the --styles / --writes folds. One wrong field name in a payload styling N cells produced N identical issues, each re-listing the full supported vocabulary: the fold meant to save round trips was burying its own answer. The defect is now stated once and the other locations are named. --sheets payload (419 cases): - Accept dtypes / formats as a positional array. The same pandas habit that produces `columns` and `data` produces df.dtypes.tolist(), and the payloads were otherwise correct. Only a 1:1 match with `columns` is accepted; a length mismatch is rejected rather than guessed. +csv-put --file (35 cases): - Read the value as a path. --file is aliased onto --csv because agents reach for it, but the names promise different things — rewriting only the name left the path to be written into the sheet as literal text, which the file-path guard then rejected, with an error naming a flag the caller never typed. The read goes through the same cmdutil.ReadInputFile as @file, so the relative-path policy is unchanged and stdin stays the out-of-tree route. A value naming nothing readable still falls through to the guard, so --file holding literal CSV keeps working. --help (88 cases of "required flag(s) ... not set"): - Mark required flags in the sheets help. MarkFlagRequired only sets a completion annotation cobra never renders, so a required flag read exactly like an optional one. Sourced from flag-defs, since +chart-create and +csv-put deliberately clear that annotation after mounting; a flag cobra has put in a one-required group is left unmarked, because neither member of such a pair is individually required. The remaining big group (absolute paths passed to --file / @file, 636 cases) is deliberately untouched: the cwd-relative policy is a protocol decision, and that thread is being followed up separately. --- shortcuts/sheets/csv_put_guard_test.go | 72 ++++++++++++ shortcuts/sheets/flag_ergonomics.go | 96 +++++++++++++++- shortcuts/sheets/flag_ergonomics_test.go | 60 ++++++++++ shortcuts/sheets/helpers.go | 91 +++++++++++++++ shortcuts/sheets/helpers_test.go | 59 ++++++++++ shortcuts/sheets/lark_sheet_table_io.go | 115 ++++++++++++++++--- shortcuts/sheets/lark_sheet_table_io_test.go | 65 ++++++++++- shortcuts/sheets/lark_sheet_workbook.go | 21 ++-- shortcuts/sheets/lark_sheet_write_cells.go | 77 ++++++++++++- shortcuts/sheets/style_vocab.go | 65 +++++++++++ shortcuts/sheets/styles_acceptance_test.go | 21 ++++ shortcuts/sheets/styles_prescription_test.go | 30 ++++- 12 files changed, 732 insertions(+), 40 deletions(-) diff --git a/shortcuts/sheets/csv_put_guard_test.go b/shortcuts/sheets/csv_put_guard_test.go index dbf6583a6b..1f7a2cd9b0 100644 --- a/shortcuts/sheets/csv_put_guard_test.go +++ b/shortcuts/sheets/csv_put_guard_test.go @@ -132,3 +132,75 @@ func TestGuardCSVValueIsNotFilePath_PassesThrough(t *testing.T) { } } } + +// newCSVFileAliasRuntime is newCSVGuardRuntime plus the record chainFlagAliases +// leaves when the value was typed as --file. +func newCSVFileAliasRuntime(csvVal string) *common.RuntimeContext { + rctx := newCSVGuardRuntime(csvVal) + rctx.Cmd.Annotations = map[string]string{aliasSourceAnnotation("csv"): "file"} + return rctx +} + +// TestResolveCSVPathFromFileAlias covers the value-side half of the file → csv +// alias: --file names a path, so a value naming a readable file is read like +// `--csv @` instead of being written into the sheet as literal text. +func TestResolveCSVPathFromFileAlias(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) + } + + t.Run("reads the named file", func(t *testing.T) { + rctx := newCSVFileAliasRuntime("./data.csv") + resolved, err := resolveCSVPathFromFileAlias(rctx) + if err != nil || !resolved { + t.Fatalf("resolved = %v, err = %v; want the file to be read", resolved, err) + } + if got, _ := rctx.Cmd.Flags().GetString("csv"); got != "a,b\n1,2\n" { + t.Errorf("--csv = %q, want the file contents", got) + } + }) + + t.Run("an out-of-tree path is rejected toward stdin", func(t *testing.T) { + _, err := resolveCSVPathFromFileAlias(newCSVFileAliasRuntime("/tmp/data.csv")) + ve := requireValidation(t, err, "relative path") + if ve.Param != "--file" { + t.Errorf("param = %q, want the flag the caller actually typed", ve.Param) + } + if !strings.Contains(ve.Hint, "--csv - <") { + t.Errorf("hint should offer stdin for a file outside the tree, got: %q", ve.Hint) + } + }) + + t.Run("inline CSV text still passes through", func(t *testing.T) { + // --file holding literal CSV was accepted before this rule existed; + // naming nothing readable, it stays the --csv guard's business. + rctx := newCSVFileAliasRuntime("a,b\n1,2") + resolved, err := resolveCSVPathFromFileAlias(rctx) + if err != nil || resolved { + t.Fatalf("resolved = %v, err = %v; want the value left alone", resolved, err) + } + if got, _ := rctx.Cmd.Flags().GetString("csv"); got != "a,b\n1,2" { + t.Errorf("--csv = %q, want the value untouched", got) + } + }) + + t.Run("a value typed as --csv is not re-read as a path", func(t *testing.T) { + // Without the alias record the rule must not fire: --csv promises text, + // and its own guard (not this one) answers a path passed to it. + resolved, err := resolveCSVPathFromFileAlias(newCSVGuardRuntime("./data.csv")) + if err != nil || resolved { + t.Fatalf("resolved = %v, err = %v; want --csv left to its guard", resolved, err) + } + }) + + t.Run("@file and stdin values are already contents", func(t *testing.T) { + rctx := newCSVFileAliasRuntime("./data.csv") + common.TestMarkInputResolved(rctx, "csv") + resolved, err := resolveCSVPathFromFileAlias(rctx) + if err != nil || resolved { + t.Fatalf("resolved = %v, err = %v; want a resolved value left alone", resolved, err) + } + }) +} diff --git a/shortcuts/sheets/flag_ergonomics.go b/shortcuts/sheets/flag_ergonomics.go index 1880c67b67..6d0e4b76e5 100644 --- a/shortcuts/sheets/flag_ergonomics.go +++ b/shortcuts/sheets/flag_ergonomics.go @@ -40,9 +40,58 @@ func withFlagErgonomics(prev func(cmd *cobra.Command)) func(cmd *cobra.Command) chainEnumNormalization(cmd) chainFlagAliases(cmd) chainRangeSheetPrefix(cmd) + chainRequiredFlagHelp(cmd) } } +// chainRequiredFlagHelp marks a required flag as required in --help. The +// framework calls MarkFlagRequired, which only sets a completion annotation +// cobra's help never renders, so a required flag was listed exactly like an +// optional one and callers learned the difference by omitting it and reading +// the failure (08-18..24 eval: 88 calls hit "required flag(s) \"title\" not +// set" on +workbook-create, after a --help showing nothing but "Spreadsheet +// title"). The marker is a domain decoration here rather than a framework one, +// so no other domain's help shifts. +// +// flag-defs is the source of truth, not the cobra annotation: two commands +// deliberately clear that annotation after mounting (+csv-put relaxes +// --start-cell for its one-required pair, +chart-create relaxes --properties +// for --print-example) while the flag stays required on the real path. The +// exception is a flag cobra has since put in a one-required GROUP, where +// neither member is individually required — those keep the plain description. +func chainRequiredFlagHelp(cmd *cobra.Command) { + defs, err := loadFlagDefs() + if err != nil { + return + } + spec, ok := defs[cmd.Name()] + if !ok { + return + } + for _, df := range spec.Flags { + // "xor" flags (--url / --spreadsheet-token, --sheet-id / --sheet-name) + // are required as a pair, and their descriptions already say so. + if df.Kind == "system" || df.Required != "required" { + continue + } + fl := cmd.Flags().Lookup(df.Name) + if fl == nil || strings.HasPrefix(fl.Usage, requiredFlagHelpPrefix) { + continue + } + // Same literal cobra uses for MarkFlagsOneRequired (it exports no + // constant); runner.go's --print-schema relaxation reads it too. + if _, grouped := fl.Annotations["cobra_annotation_one_required"]; grouped { + continue + } + fl.Usage = requiredFlagHelpPrefix + fl.Usage + } +} + +// requiredFlagHelpPrefix leads the description rather than trailing it: this +// domain's payload flags carry paragraph-long descriptions that a terminal +// wraps or truncates, and the marker has to survive that. +const requiredFlagHelpPrefix = "(required) " + // ─── intuitive flag names: silent aliases & prescriptions ─────────────── // // Eval traces show unknown-flag failures cluster on a handful of habitual @@ -56,9 +105,14 @@ func withFlagErgonomics(prev func(cmd *cobra.Command)) func(cmd *cobra.Command) // commandFlagAliases maps, per command, habitual flag names onto the flag // actually registered. Only pairs with identical value semantics belong -// here: the rewrite is invisible, so it must be safe to apply unread -// (+csv-put --file with a path value still trips the file-path guard, which -// prescribes @file / stdin). +// 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. var commandFlagAliases = map[string]map[string]string{ "+csv-put": {"file": "csv"}, "+sheet-create": {"name": "title"}, @@ -170,17 +224,47 @@ func chainFlagAliases(cmd *cobra.Command) { usable[alias] = target } } + flags := cmd.Flags() flagalias.InstallNormalizer(cmd, func(name string) string { if strings.Contains(name, "_") { name = strings.ReplaceAll(name, "_", "-") } - if target, ok := usable[name]; ok { - name = target + target, ok := usable[name] + if !ok { + return name + } + // Remember the spelling for the one rule that needs it (a value read + // as a path only because the caller wrote --file). InstallNormalizer + // records nothing itself by design, and pflag also normalizes names + // outside parsing (AddFlag, Lookup, Set), so the record is gated on + // the parser actually walking argv — same guard flagalias.Bind uses + // for its own source tracking. + if flags.Parsed() { + if cmd.Annotations == nil { + cmd.Annotations = map[string]string{} + } + cmd.Annotations[aliasSourceAnnotation(target)] = name } - return name + return target }) } +// aliasSourceAnnotation names the command annotation holding the alias +// spelling that supplied a canonical flag's value, or is absent when the +// canonical name was typed. +func aliasSourceAnnotation(canonical string) string { + return "lark-cli/sheets-alias-source/" + canonical +} + +// 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 { + if cmd == nil { + return false + } + return cmd.Annotations[aliasSourceAnnotation(canonical)] == alias +} + // sheetsFlagErrorFunc overrides the root FlagErrorFunc for sheets commands. // It keeps the root behavior (typed error, did-you-mean suggestions, the // offending flag on params) and additionally inlines the full valid-flag diff --git a/shortcuts/sheets/flag_ergonomics_test.go b/shortcuts/sheets/flag_ergonomics_test.go index 9314f6af35..a0802732fa 100644 --- a/shortcuts/sheets/flag_ergonomics_test.go +++ b/shortcuts/sheets/flag_ergonomics_test.go @@ -692,3 +692,63 @@ func TestShortcuts_IntuitiveFlagHints(t *testing.T) { }) } } + +// TestShortcuts_RequiredFlagsMarkedInHelp pins the required marker on the +// mounted commands. It is a sheets-local decoration (chainRequiredFlagHelp), +// so the assertions live here rather than against the framework: cobra renders +// no marker of its own, and the two commands that relax the annotation after +// mounting must still be answered from flag-defs. +func TestShortcuts_RequiredFlagsMarkedInHelp(t *testing.T) { + t.Parallel() + + usageOf := func(t *testing.T, command, flag string) string { + t.Helper() + parent, _, _, _ := newTestRig(t, shortcutFromRegistry(t, command)) + cmd, _, err := parent.Find([]string{command}) + if err != nil { + t.Fatalf("Find(%q) error = %v", command, err) + } + fl := cmd.Flags().Lookup(flag) + if fl == nil { + t.Fatalf("%s has no --%s", command, flag) + } + return fl.Usage + } + + t.Run("a required flag says so", func(t *testing.T) { + t.Parallel() + if got := usageOf(t, "+workbook-create", "title"); !strings.HasPrefix(got, "(required) ") { + t.Errorf("--title usage = %q, want the required marker", got) + } + }) + + t.Run("an optional flag is left alone", func(t *testing.T) { + t.Parallel() + if got := usageOf(t, "+workbook-create", "folder-token"); strings.Contains(got, "(required)") { + t.Errorf("--folder-token usage = %q, want no required marker", got) + } + }) + + t.Run("a relaxed-but-still-required flag says so", func(t *testing.T) { + t.Parallel() + // +chart-create clears the cobra annotation so --print-example can run + // without it; --properties is still required on every other path. + if got := usageOf(t, "+chart-create", "properties"); !strings.HasPrefix(got, "(required) ") { + t.Errorf("--properties usage = %q, want the required marker", got) + } + }) + + t.Run("a one-required pair marks neither member", func(t *testing.T) { + t.Parallel() + // +csv-put takes --start-cell OR its --range alias, so neither is + // individually required — saying otherwise would be a false statement. + for _, flag := range []string{"start-cell", "range"} { + if got := usageOf(t, "+csv-put", flag); strings.Contains(got, "(required)") { + t.Errorf("--%s usage = %q, want no required marker", flag, got) + } + } + if got := usageOf(t, "+csv-put", "csv"); !strings.HasPrefix(got, "(required) ") { + t.Errorf("--csv usage = %q, want the required marker", got) + } + }) +} diff --git a/shortcuts/sheets/helpers.go b/shortcuts/sheets/helpers.go index a72837e52a..ce7c3bbea2 100644 --- a/shortcuts/sheets/helpers.go +++ b/shortcuts/sheets/helpers.go @@ -11,6 +11,7 @@ import ( "context" "encoding/json" "errors" + "fmt" neturl "net/url" "strings" @@ -752,6 +753,96 @@ func aggregatedIssueText(err error) string { return msg + " (" + hint + ")" } +// collapseAggregatedIssues renders the collected sub-errors for a folded +// message, stating each DISTINCT defect once and naming the other locations +// it occurred at. Results keep first-appearance order. +// +// One wrong field name in a payload that styles N cells produces N identical +// issues, each re-listing the full supported-field vocabulary. 08-18..24 +// eval: a six-cell payload with one bad field spent 1.6k characters saying +// the same thing six times, and the prescription the agent needed was buried +// mid-message — the fold meant to save round trips was drowning its own +// answer. Deduplicated, that payload states the fix once and names the six +// ranges, which is what a rewrite actually needs. +// +// Two issues are "the same defect" when their text matches after every +// [] is blanked, so cell_styles[0] and cell_styles[7] collapse while +// two different bad fields never do. +func collapseAggregatedIssues(probs []error) []string { + const maxRepeatPaths = 3 + type group struct { + text string + paths []string + } + order := make([]string, 0, len(probs)) + groups := make(map[string]*group, len(probs)) + for _, e := range probs { + text := aggregatedIssueText(e) + key := blankIssueIndices(text) + g, seen := groups[key] + if !seen { + g = &group{text: text} + groups[key] = g + order = append(order, key) + continue + } + g.paths = append(g.paths, issuePathToken(text)) + } + out := make([]string, 0, len(order)) + for _, key := range order { + g := groups[key] + if len(g.paths) == 0 { + out = append(out, g.text) + continue + } + shown := g.paths + suffix := "" + if len(shown) > maxRepeatPaths { + suffix = fmt.Sprintf(", +%d more", len(shown)-maxRepeatPaths) + shown = shown[:maxRepeatPaths] + } + out = append(out, fmt.Sprintf("%s [same at %d more: %s%s]", + g.text, len(g.paths), strings.Join(shown, ", "), suffix)) + } + return out +} + +// blankIssueIndices replaces every [] with [#], so the grouping key of +// an issue ignores which item it was found on. +func blankIssueIndices(text string) string { + var b strings.Builder + b.Grow(len(text)) + for i := 0; i < len(text); i++ { + if text[i] != '[' { + b.WriteByte(text[i]) + continue + } + j := i + 1 + for j < len(text) && text[j] >= '0' && text[j] <= '9' { + j++ + } + if j > i+1 && j < len(text) && text[j] == ']' { + b.WriteString("[#]") + i = j + continue + } + b.WriteByte(text[i]) + } + return b.String() +} + +// issuePathToken is the leading path of an issue message ("--styles.styles[0] +// .cell_styles[1].border_type"), used to name a repeat's location. Every +// collected sub-error starts with its path, either inline or as a "path: " +// prefix added by prefixValidationIssue. +func issuePathToken(text string) string { + token := text + if i := strings.IndexByte(token, ' '); i >= 0 { + token = token[:i] + } + return strings.TrimSuffix(token, ":") +} + // prefixValidationIssue re-labels a collected sub-error with the path it was // found at ("--writes[2]"), keeping its Hint. Formatting the inner error into // a new message with "%v" would drop that hint on the floor — the collectors diff --git a/shortcuts/sheets/helpers_test.go b/shortcuts/sheets/helpers_test.go index 6f7edfbf13..20907e3a4e 100644 --- a/shortcuts/sheets/helpers_test.go +++ b/shortcuts/sheets/helpers_test.go @@ -7,6 +7,7 @@ import ( "bytes" "encoding/json" "errors" + "fmt" "strings" "testing" @@ -393,3 +394,61 @@ func TestParseSpreadsheetRef(t *testing.T) { }) } } + +// TestCollapseAggregatedIssues covers the grouping rule behind the folded +// --styles / --writes messages: one defect repeated across items is stated +// once and its other locations named, while genuinely different defects each +// keep their own line. +func TestCollapseAggregatedIssues(t *testing.T) { + t.Parallel() + + issue := func(msg string) error { return common.ValidationErrorf("%s", msg) } + + t.Run("identical defects across items collapse", func(t *testing.T) { + t.Parallel() + got := collapseAggregatedIssues([]error{ + issue("--styles.styles[0].cell_styles[0].border_type is bad; supported: a, b"), + issue("--styles.styles[0].cell_styles[1].border_type is bad; supported: a, b"), + }) + if len(got) != 1 { + t.Fatalf("got %d lines, want 1: %q", len(got), got) + } + if !strings.Contains(got[0], "[same at 1 more: --styles.styles[0].cell_styles[1].border_type]") { + t.Errorf("the collapsed line must name the other location, got %q", got[0]) + } + }) + + t.Run("different defects stay separate", func(t *testing.T) { + t.Parallel() + got := collapseAggregatedIssues([]error{ + issue("--styles.styles[0].cell_styles[0].border_type is bad"), + issue("--styles.styles[0].cell_styles[1].bg_color is bad"), + }) + if len(got) != 2 { + t.Fatalf("got %d lines, want 2: %q", len(got), got) + } + }) + + t.Run("a long repeat lists a few locations then counts the rest", func(t *testing.T) { + t.Parallel() + probs := make([]error, 0, 9) + for i := 0; i < 9; i++ { + probs = append(probs, issue(fmt.Sprintf("--styles.styles[0].cell_styles[%d].border_type is bad", i))) + } + got := collapseAggregatedIssues(probs) + if len(got) != 1 { + t.Fatalf("got %d lines, want 1: %q", len(got), got) + } + if !strings.Contains(got[0], "[same at 8 more:") || !strings.Contains(got[0], "+5 more]") { + t.Errorf("want 3 locations named and the rest counted, got %q", got[0]) + } + }) + + t.Run("a lone issue is rendered verbatim", func(t *testing.T) { + t.Parallel() + got := collapseAggregatedIssues([]error{issue("--writes[0]: nope")}) + if len(got) != 1 || got[0] != "--writes[0]: nope" { + t.Errorf("got %q, want the message unchanged", got) + } + }) +} diff --git a/shortcuts/sheets/lark_sheet_table_io.go b/shortcuts/sheets/lark_sheet_table_io.go index e32da10333..cbece759c9 100644 --- a/shortcuts/sheets/lark_sheet_table_io.go +++ b/shortcuts/sheets/lark_sheet_table_io.go @@ -158,15 +158,96 @@ type tableColumnSpec struct { // df.dtypes.astype(str).to_dict()}`) and lets handwritten payloads stay flat // rather than nest a {name, type, format} object per column. type tableSheetIn struct { - Name string `json:"name"` - StartCell string `json:"start_cell"` - Mode string `json:"mode"` - Header *bool `json:"header"` - AllowOverwrite *bool `json:"allow_overwrite"` - Columns []string `json:"columns"` - Data [][]interface{} `json:"data"` - Dtypes map[string]string `json:"dtypes"` - Formats map[string]string `json:"formats"` + Name string `json:"name"` + StartCell string `json:"start_cell"` + Mode string `json:"mode"` + Header *bool `json:"header"` + AllowOverwrite *bool `json:"allow_overwrite"` + Columns []string `json:"columns"` + Data [][]interface{} `json:"data"` + Dtypes columnLabels `json:"dtypes"` + Formats columnLabels `json:"formats"` +} + +// columnLabels holds `dtypes` / `formats` as written on the wire. The +// documented shape is the column-name-keyed map, but the positional array is +// the other half of the same pandas habit that produces `columns` and `data`: +// `df.dtypes.tolist()` / a hand-written `["object","float64",…]` lines up with +// `columns` by index. 08-18..24 eval: `dtypes` as an array was the single +// largest --sheets decode failure (113 cases) — the payload was otherwise +// correct and every retry just rewrote the same information as a map. +// +// Accepting it is unambiguous only when the array lines up 1:1 with `columns`, +// which resolve enforces — a length mismatch means the caller lost track of +// which label belongs to which column, so guessing an alignment would write +// the wrong types silently. +type columnLabels struct { + byName map[string]string + positional []string +} + +// labelsByName builds the map form, for call sites (tests, synthesized +// payloads) that construct a tableSheetIn directly rather than decoding one. +func labelsByName(m map[string]string) columnLabels { return columnLabels{byName: m} } + +// UnmarshalJSON accepts either shape and defers the columns-dependent part of +// validation to resolve. A null decodes to the empty value (field omitted). +func (c *columnLabels) UnmarshalJSON(b []byte) error { + trimmed := strings.TrimSpace(string(b)) + switch { + case trimmed == "" || trimmed == "null": + return nil + case strings.HasPrefix(trimmed, "["): + // []*string, not []string: a positional array written from a frame with + // unlabeled columns carries nulls, and rejecting those would push the + // caller back to the map form for a payload we can read exactly. + var arr []*string + if err := json.Unmarshal(b, &arr); err != nil { + return err + } + c.positional = make([]string, len(arr)) + for i, v := range arr { + if v != nil { + c.positional[i] = *v + } + } + return nil + default: + return json.Unmarshal(b, &c.byName) + } +} + +// resolve returns the column-name-keyed form, zipping a positional array +// against columns. field is "dtypes" / "formats" and the sheet coordinates +// carry the error's context, matching the rest of normalize's messages. +func (c columnLabels) resolve(field string, idx int, sheet string, columns []string) (map[string]string, error) { + if c.positional == nil { + return c.byName, nil + } + if len(c.positional) != len(columns) { + return nil, common.ValidationErrorf( + "--sheets[%d] %q: %s is a positional array of %d entries but the sheet has %d columns", + idx, sheet, field, len(c.positional), len(columns)). + WithHint("a positional %s array must line up 1:1 with `columns`; otherwise key it by column name, e.g. %s:{%q:\"…\"}", + field, field, firstColumnName(columns)) + } + out := make(map[string]string, len(c.positional)) + for i, name := range columns { + if strings.TrimSpace(c.positional[i]) == "" { + continue // unlabeled column: same as omitting it from the map + } + out[name] = c.positional[i] + } + return out, nil +} + +// firstColumnName is the sample column name inlined in the positional-array +// hint, so the suggested map form is spelled with a name the caller recognizes. +func firstColumnName(columns []string) string { + if len(columns) > 0 && strings.TrimSpace(columns[0]) != "" { + return columns[0] + } + return "colA" } // dtypeToTypeFormat maps a pandas-style dtype string to the internal column @@ -331,6 +412,14 @@ func (in *tableSheetIn) normalize(idx int) (tableSheetSpec, error) { AllowOverwrite: in.AllowOverwrite, Rows: in.Data, } + dtypes, err := in.Dtypes.resolve("dtypes", idx, in.Name, in.Columns) + if err != nil { + return tableSheetSpec{}, err + } + formats, err := in.Formats.resolve("formats", idx, in.Name, in.Columns) + if err != nil { + return tableSheetSpec{}, err + } seenCol := make(map[string]bool, len(in.Columns)) spec.Columns = make([]tableColumnSpec, len(in.Columns)) for j, name := range in.Columns { @@ -342,8 +431,8 @@ func (in *tableSheetIn) normalize(idx int) (tableSheetSpec, error) { return tableSheetSpec{}, common.ValidationErrorf("--sheets[%d] %q: duplicate column name %q", idx, in.Name, name) } seenCol[name] = true - typ, format := dtypeToTypeFormat(in.Dtypes[name]) - if f, ok := in.Formats[name]; ok { + typ, format := dtypeToTypeFormat(dtypes[name]) + if f, ok := formats[name]; ok { format = strings.TrimSpace(f) } spec.Columns[j] = tableColumnSpec{Name: name, Type: typ, Format: format} @@ -353,13 +442,13 @@ func (in *tableSheetIn) normalize(idx int) (tableSheetSpec, error) { // silently ignoring them would let the writer succeed with the wrong // formatting. The check runs after the column list is built so we can // compare against the canonical set. - for k := range in.Dtypes { + for k := range dtypes { if !seenCol[k] { return tableSheetSpec{}, common.ValidationErrorf("--sheets[%d] %q: dtypes references unknown column %q", idx, in.Name, k). WithHint("%s", columnKeyHint("dtypes", k, in.Columns)) } } - for k := range in.Formats { + for k := range formats { if !seenCol[k] { return tableSheetSpec{}, common.ValidationErrorf("--sheets[%d] %q: formats references unknown column %q", idx, in.Name, k). WithHint("%s", columnKeyHint("formats", k, in.Columns)) diff --git a/shortcuts/sheets/lark_sheet_table_io_test.go b/shortcuts/sheets/lark_sheet_table_io_test.go index 60474dfd7c..f2f3ab14b1 100644 --- a/shortcuts/sheets/lark_sheet_table_io_test.go +++ b/shortcuts/sheets/lark_sheet_table_io_test.go @@ -289,8 +289,8 @@ func TestNormalize_DefaultsAndFormatOverride(t *testing.T) { in := &tableSheetIn{ Name: "S", Columns: []string{"id", "amt", "d", "raw"}, - Dtypes: map[string]string{"amt": "float64", "d": "datetime64[ns]"}, // id, raw left unspecified - Formats: map[string]string{"amt": "#,##0.00"}, // override float default ("") + Dtypes: labelsByName(map[string]string{"amt": "float64", "d": "datetime64[ns]"}), // id, raw left unspecified + Formats: labelsByName(map[string]string{"amt": "#,##0.00"}), // override float default ("") Data: [][]interface{}{}, } spec, err := in.normalize(0) @@ -1947,3 +1947,64 @@ func TestTableGet_CharBudgetSpansTheWholeWorkbook(t *testing.T) { t.Errorf("second sheet asked for max_chars=%d, not reduced by what the first consumed (%d) — the budget is per-workbook, not per-sheet", caps[1], caps[0]) } } + +// TestPositionalColumnLabels covers the dtypes / formats array form: the same +// pandas habit that produces `columns` and `data` also produces a positional +// `df.dtypes.tolist()`, and rejecting it cost a full retry on payloads that +// were otherwise correct (08-18..24 eval, the largest --sheets decode failure). +// Accepting it is only unambiguous when the array lines up 1:1 with columns. +func TestPositionalColumnLabels(t *testing.T) { + t.Parallel() + + t.Run("positional dtypes and formats zip onto columns", func(t *testing.T) { + t.Parallel() + p, err := parseTablePutPayload(newMapFlagViewForCommand("+table-put", map[string]interface{}{ + "sheets": `{"sheets":[{"name":"S","columns":["id","amt"],"data":[["001",1.5]],` + + `"dtypes":["object","float64"],"formats":[null,"#,##0.00"]}]}`, + })) + if err != nil { + t.Fatalf("positional labels rejected: %v", err) + } + want := []tableColumnSpec{ + {Name: "id", Type: "string", Format: "@"}, // object → text, leading zero survives + {Name: "amt", Type: "number", Format: "#,##0.00"}, // float64 + its positional format + } + for i, w := range want { + if got := p.Sheets[0].Columns[i]; got != w { + t.Errorf("columns[%d] = %+v, want %+v", i, got, w) + } + } + }) + + t.Run("a mismatched length is rejected, not guessed", func(t *testing.T) { + t.Parallel() + _, err := parseTablePutPayload(newMapFlagViewForCommand("+table-put", map[string]interface{}{ + "sheets": `{"sheets":[{"name":"S","columns":["id","amt"],"data":[["001",1.5]],"dtypes":["object"]}]}`, + })) + ve := requireValidation(t, err, "positional array of 1 entries but the sheet has 2 columns") + if !strings.Contains(ve.Hint, "line up 1:1") { + t.Errorf("hint should explain the alignment rule, got %q", ve.Hint) + } + }) + + t.Run("the map form is unchanged", func(t *testing.T) { + t.Parallel() + p, err := parseTablePutPayload(newMapFlagViewForCommand("+table-put", map[string]interface{}{ + "sheets": `{"sheets":[{"name":"S","columns":["id","amt"],"data":[["001",1.5]],"dtypes":{"amt":"float64"}}]}`, + })) + if err != nil { + t.Fatalf("map form rejected: %v", err) + } + if got := p.Sheets[0].Columns[1].Type; got != "number" { + t.Errorf("columns[1].Type = %q, want number", got) + } + }) + + t.Run("an unknown key in the map form still errors", func(t *testing.T) { + t.Parallel() + _, err := parseTablePutPayload(newMapFlagViewForCommand("+table-put", map[string]interface{}{ + "sheets": `{"sheets":[{"name":"S","columns":["id"],"data":[["001"]],"dtypes":{"nope":"float64"}}]}`, + })) + requireValidation(t, err, `dtypes references unknown column "nope"`) + }) +} diff --git a/shortcuts/sheets/lark_sheet_workbook.go b/shortcuts/sheets/lark_sheet_workbook.go index cc1a23ca82..d60656e9cb 100644 --- a/shortcuts/sheets/lark_sheet_workbook.go +++ b/shortcuts/sheets/lark_sheet_workbook.go @@ -1340,17 +1340,16 @@ func joinStyleValidationErrors(probs []error) error { return verr } const maxShown = 8 - shown := probs - if len(shown) > maxShown { - shown = shown[:maxShown] - } - msgs := make([]string, 0, len(shown)) - for _, e := range shown { - msgs = append(msgs, aggregatedIssueText(e)) - } + msgs := collapseAggregatedIssues(probs) + distinct := len(msgs) suffix := "" - if len(probs) > maxShown { - suffix = fmt.Sprintf(" (+%d more)", len(probs)-maxShown) + if len(msgs) > maxShown { + suffix = fmt.Sprintf(" (+%d more)", len(msgs)-maxShown) + msgs = msgs[:maxShown] + } + if distinct < len(probs) { + return sheetsValidationForFlag("styles", "--styles has %d issues (%d distinct): %s%s", len(probs), distinct, strings.Join(msgs, " | "), suffix). + WithCause(probs[0]) } return sheetsValidationForFlag("styles", "--styles has %d issues: %s%s", len(probs), strings.Join(msgs, " | "), suffix). WithCause(probs[0]) @@ -1662,7 +1661,7 @@ func normalizeWorkbookCreateStyleObject(in map[string]interface{}, path string) // misleads worse than silence. msg := fmt.Sprintf("%s.%s is not a supported style field", path, k) lower := strings.ToLower(k) - if rx, ok := styleFieldPrescriptions[lower]; ok { + if rx := styleFieldPrescriptionFor(k); rx != "" { msg += " — " + rx } else if match := suggest.Closest(lower, workbookCreateCellStyleFieldList, 1); len(match) > 0 && suggest.Levenshtein(lower, match[0]) <= 2 { msg += fmt.Sprintf(" — did you mean %q?", match[0]) diff --git a/shortcuts/sheets/lark_sheet_write_cells.go b/shortcuts/sheets/lark_sheet_write_cells.go index e8ea2f38b4..b7d522b9f4 100644 --- a/shortcuts/sheets/lark_sheet_write_cells.go +++ b/shortcuts/sheets/lark_sheet_write_cells.go @@ -6,6 +6,7 @@ package sheets import ( "context" "encoding/csv" + "errors" "fmt" "image" _ "image/gif" @@ -17,6 +18,8 @@ import ( "unicode" "github.com/larksuite/cli/errs" + "github.com/larksuite/cli/extension/fileio" + "github.com/larksuite/cli/internal/cmdutil" "github.com/larksuite/cli/internal/validate" "github.com/larksuite/cli/shortcuts/common" "github.com/spf13/cobra" @@ -224,15 +227,17 @@ func joinWritesValidationErrors(probs []error) error { return verr } const maxShown = 8 - msgs := make([]string, 0, len(probs)) - for _, e := range probs { - msgs = append(msgs, aggregatedIssueText(e)) - } + msgs := collapseAggregatedIssues(probs) + distinct := len(msgs) suffix := "" if len(msgs) > maxShown { suffix = fmt.Sprintf(" (+%d more)", len(msgs)-maxShown) msgs = msgs[:maxShown] } + if distinct < len(probs) { + return sheetsValidationForFlag("writes", "--writes has %d issues (%d distinct): %s%s", len(probs), distinct, strings.Join(msgs, " | "), suffix). + WithCause(probs[0]) + } return sheetsValidationForFlag("writes", "--writes has %d issues: %s%s", len(probs), strings.Join(msgs, " | "), suffix). WithCause(probs[0]) } @@ -395,9 +400,17 @@ var CsvPut = common.Shortcut{ cmd.MarkFlagsMutuallyExclusive("start-cell", "range") }, Validate: func(ctx context.Context, runtime *common.RuntimeContext) error { - if err := guardCSVValueIsNotFilePath(runtime); err != nil { + resolved, err := resolveCSVPathFromFileAlias(runtime) + if err != nil { return err } + // The guard asks "is this inline CSV actually a path?" — already + // answered when the value arrived under --file and was read as one. + if !resolved { + if err := guardCSVValueIsNotFilePath(runtime); err != nil { + return err + } + } return validateViaInput(csvPutInput)(ctx, runtime) }, DryRun: func(ctx context.Context, runtime *common.RuntimeContext) *common.DryRunAPI { @@ -475,6 +488,60 @@ func csvPutWriteRangeFromInput(input map[string]interface{}) (string, bool) { return fmt.Sprintf("%s:%s%d", anchor, endCol, endRow), true } +// resolveCSVPathFromFileAlias reads the CSV file named by a value that arrived +// under the --file alias, replacing the flag value with its contents exactly as +// `--csv @` would. It reports whether it did. +// +// --file is aliased onto --csv because agents habitually reach for it, but the +// two names promise different things: --csv holds CSV text, --file holds a +// path. Rewriting only the name left the path to be written into the sheet as +// literal text, which the file-path guard then had to reject — so the alias +// meant to save a round trip spent one instead, on an error naming a flag the +// caller never typed (08-18..24 eval, 35 cases). A caller who writes --file +// means a path in every vocabulary this alias was added for, so reading one is +// the only reading; --csv keeps its guard, unchanged, for callers who type it. +// +// Values already resolved by the framework (--file @x / --file -) are left +// alone: they are contents, not a path. The read goes through the same +// cmdutil.ReadInputFile as @file, so the relative-path policy is identical — +// an absolute path is rejected here exactly as it would be there, and stdin +// stays the out-of-tree route. +// +// Only a value that names a readable file is claimed. A value that names +// nothing falls through untouched — `--file` holding literal CSV text was +// accepted before this rule existed and still is, judged by the same guard as +// --csv rather than by this one's narrower reading. +func resolveCSVPathFromFileAlias(runtime *common.RuntimeContext) (bool, error) { + if runtime == nil || !flagValueCameFromAlias(runtime.Cmd, "csv", "file") { + return false, nil + } + if runtime.InputResolvedFromSource("csv") { + return false, nil + } + raw := strings.TrimSpace(runtime.Str("csv")) + if raw == "" || strings.HasPrefix(raw, "@") { + return false, nil + } + data, err := cmdutil.ReadInputFile(runtime.FileIO(), raw) + if err != nil { + // A rejected PATH is reported: the caller named a real location the + // policy will not read (absolute, or outside the tree), and the fix is + // stdin. Any other failure — the usual one being "no such file" — + // leaves the value for the --csv guard, which decides between "you + // meant a path" and "this is one-cell CSV text" on the value's shape. + if !errors.Is(err, fileio.ErrPathValidation) { + return false, nil + } + return false, sheetsValidationForFlag("file", "--file %v", err). + WithCause(err). + WithHint("--file reads a path relative to the current directory; pipe a file outside it in via stdin instead (--csv - < )") + } + if err := runtime.Cmd.Flags().Set("csv", common.StripUTF8BOM(string(data))); err != nil { + return false, sheetsValidationForFlag("file", "--file: %v", err).WithCause(err) + } + return true, nil +} + // guardCSVValueIsNotFilePath catches the common slip of passing a CSV file path // to --csv without the "@" that reads it (e.g. `--csv data.csv` instead of // `--csv @data.csv`). Because any string is a valid one-cell CSV, the mistake diff --git a/shortcuts/sheets/style_vocab.go b/shortcuts/sheets/style_vocab.go index 2fde82db14..b3220e134a 100644 --- a/shortcuts/sheets/style_vocab.go +++ b/shortcuts/sheets/style_vocab.go @@ -135,6 +135,71 @@ var styleFieldPrescriptions = map[string]string{ "underline": `underline is font_line:"underline"`, "text_align": "horizontal text alignment is horizontal_alignment (left/center/right)", "font": `cell_styles has no nested font object — use the flat font_* fields (font:{"bold":true,"size":18,"color":"#000"} becomes font_weight:"bold", font_size:18, font_color:"#000")`, + // The OpenAPI's own request shape is {range, style:{…}}, so a cell_styles + // item written from the API docs nests one level too deep. Distance-based + // suggestion is useless here (the fix is structural, not a rename) and the + // bare "style is not a supported style field" reads like the whole payload + // shape is wrong. 08-18..24 eval, --styles group. + "style": `cell_styles has no nested style object — the style fields sit directly on the item, next to range ({"range":"A1:B2","style":{"font_weight":"bold"}} becomes {"range":"A1:B2","font_weight":"bold"})`, + // bg_color / text_color read unambiguously (unlike fore_color, which is + // rejected as ambiguous above) but stay prescriptions rather than silent + // aliases: they are spelling permutations, not words from a real external + // vocabulary, and the silent-alias admission bar excludes those. + "bg_color": "the cell fill is background_color", + "fill_color": "the cell fill is background_color", + "text_color": "the text color is font_color", +} + +// borderFieldPrescription answers any unsupported border-family spelling that +// survived foldBorderFamilyAliases (which already absorbs border / borders / +// border_ / border_ and their word-order twins). What is left is +// vocabulary with no equivalent here at all: the Lark OpenAPI's own +// border_type (FULL_BORDER / OUTER_BORDER / …) and CSS's border_width. Both +// are real external vocabularies, so they recur; neither maps unambiguously +// onto a per-side style/weight/color triple — FULL_BORDER vs OUTER_BORDER +// differ on the interior edges this payload cannot address. 08-18..24 eval: +// border_type was the top single field in the --styles error group, and the +// did-you-mean it drew ("border_styles") sent the retry back with the same +// unusable value. +const borderFieldPrescription = `borders go in border ({"border":{"style":"solid","weight":"thin","color":"#000000"}} — all four sides) or border_styles for per-side control ({"border_styles":{"bottom":{"style":"solid"}}}); style is solid/dashed/dotted/double/none, weight is thin/medium/thick — there is no border_type / border_width field` + +// styleFieldPrescriptionsSquashed keys the curated table by letters alone, so +// every separator spelling of one mistake (border_type / borderType / +// border-type) resolves to the same prescription. Built once at init; the +// parity test asserts no two entries collide after squashing. +var styleFieldPrescriptionsSquashed = func() map[string]string { + out := make(map[string]string, len(styleFieldPrescriptions)) + for k, v := range styleFieldPrescriptions { + out[squashStyleFieldKey(k)] = v + } + return out +}() + +// squashStyleFieldKey reduces a field name to its letters and digits, lowercased. +func squashStyleFieldKey(field string) string { + var b strings.Builder + for _, r := range strings.ToLower(field) { + if r == '_' || r == '-' || r == ' ' { + continue + } + b.WriteRune(r) + } + return b.String() +} + +// styleFieldPrescriptionFor returns the curated fix for an unsupported +// cell_styles field name, or "" when the generic did-you-mean should answer +// instead. The border family gets one shared answer: enumerating its spelling +// permutations is endless, but every one of them has the same two-form fix. +func styleFieldPrescriptionFor(field string) string { + key := squashStyleFieldKey(field) + if rx, ok := styleFieldPrescriptionsSquashed[key]; ok { + return rx + } + if strings.HasPrefix(key, "border") || strings.HasSuffix(key, "border") { + return borderFieldPrescription + } + return "" } // cellStyleEnumFields sources the enum vocabulary for enum-bearing diff --git a/shortcuts/sheets/styles_acceptance_test.go b/shortcuts/sheets/styles_acceptance_test.go index 839ccd4e8f..aab0f7ea0a 100644 --- a/shortcuts/sheets/styles_acceptance_test.go +++ b/shortcuts/sheets/styles_acceptance_test.go @@ -198,6 +198,27 @@ var stylesPriorCorpus = []struct { {name: "wrap_strategy aliases to word_wrap", fields: map[string]interface{}{"wrap_strategy": "auto-wrap"}, check: wantStyle("word_wrap", "auto-wrap")}, + // 08-18..24 batch. The border family's remaining spellings come from the + // Lark OpenAPI (border_type: FULL_BORDER / OUTER_BORDER) and CSS + // (border_width) — real vocabularies, but neither maps onto a per-side + // style/weight/color triple, so they stay prescriptions. The nested + // {range, style:{…}} envelope is the OpenAPI request shape copied one + // level too deep. + {name: "border_type prescribed", fields: map[string]interface{}{"border_type": "solid"}, + wantErr: "there is no border_type / border_width field"}, + {name: "camelCase borderType prescribed", fields: map[string]interface{}{"borderType": "FULL_BORDER"}, + wantErr: "borders go in border"}, + {name: "kebab border-style prescribed", fields: map[string]interface{}{"border-style": "solid"}, + wantErr: "borders go in border"}, + {name: "border_width prescribed", fields: map[string]interface{}{"border_width": float64(1)}, + wantErr: "borders go in border"}, + {name: "nested style envelope prescribed", + fields: map[string]interface{}{"style": map[string]interface{}{"font_weight": "bold"}}, + wantErr: "no nested style object"}, + {name: "bg_color prescribed", fields: map[string]interface{}{"bg_color": "#FFFFFF"}, + wantErr: "the cell fill is background_color"}, + {name: "text_color prescribed", fields: map[string]interface{}{"text_color": "#000000"}, + wantErr: "the text color is font_color"}, // prescriptions (ambiguous / unsupported / typo) {name: "fore_color prescribed", fields: map[string]interface{}{"fore_color": "#F00"}, wantErr: "ambiguous"}, {name: "indent rejected not ignored", fields: map[string]interface{}{"indent": float64(2)}, wantErr: "not a supported style field"}, diff --git a/shortcuts/sheets/styles_prescription_test.go b/shortcuts/sheets/styles_prescription_test.go index 4f47fefb1f..e2e5ff6696 100644 --- a/shortcuts/sheets/styles_prescription_test.go +++ b/shortcuts/sheets/styles_prescription_test.go @@ -574,13 +574,37 @@ func TestAggregatedIssuesKeepPrescriptions(t *testing.T) { t.Run("folded --writes issues inline each hint", func(t *testing.T) { t.Parallel() + // Two DIFFERENT defects: both must be rendered, and the one whose + // prescription lives in Hint must still carry it inline. (Two copies of + // the SAME defect collapse instead — pinned below.) _, _, err := runShortcutCapturingErr(t, CellsSet, []string{ "--url", testURL, - "--writes", `[{"range":"A1","cells":[[{"value":1}]]},{"range":"B1","cells":[[{"value":2}]]}]`, + "--writes", `[{"range":"A1","cells":[[{"value":1}]]},{"sheet_name":"S","range":"A1:B1","cells":[[{"value":2}]]}]`, }) ve := requireValidation(t, err, "--writes has 2 issues") - if strings.Count(ve.Message, "+workbook-info") != 2 { - t.Errorf("each issue should carry its own prescription inline, got %q", ve.Message) + if !strings.Contains(ve.Message, "+workbook-info") { + t.Errorf("the first issue's Hint prescription should be inlined, got %q", ve.Message) + } + if !strings.Contains(ve.Message, `--range "A1:B1" spans`) { + t.Errorf("the second issue should be rendered too, got %q", ve.Message) + } + }) + + t.Run("identical issues collapse to one prescription", func(t *testing.T) { + t.Parallel() + // One defect repeated per item is the shape that used to bury its own + // answer: N copies of the same message, each re-listing the full + // vocabulary. It must be stated once, with the other locations named. + _, _, err := runShortcutCapturingErr(t, CellsSet, []string{ + "--url", testURL, + "--writes", `[{"range":"A1","cells":[[{"value":1}]]},{"range":"B1","cells":[[{"value":2}]]}]`, + }) + ve := requireValidation(t, err, "--writes has 2 issues (1 distinct)") + if strings.Count(ve.Message, "+workbook-info") != 1 { + t.Errorf("the repeated prescription should appear once, got %q", ve.Message) + } + if !strings.Contains(ve.Message, "[same at 1 more: --writes[1]]") { + t.Errorf("the collapsed issue must name where else it occurred, got %q", ve.Message) } }) } From 224a416bc1498eaf0365ed39d08a6d3424ab4544 Mon Sep 17 00:00:00 2001 From: xiongyuanwen-byted Date: Sat, 29 Aug 2026 15:02:57 +0800 Subject: [PATCH 2/4] fix(sheets): correct --file alias provenance and mark its value resolved MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two defects in the +csv-put --file rule from the previous commit, both found in review: - The alias record was written from the flag-name normalizer, which pflag also runs for Lookup and Set — including once with the canonical name right after the rewrite, and again on every later lookup. `--file a.csv --csv ./b.csv` therefore still counted as "supplied by --file", and an explicit --csv path was silently read as a file instead of meeting its guard. The spelling is now staged by the normalizer and committed by the flag's Value, which runs once per real occurrence, so the last occurrence wins in either order. - The rewritten value was not marked as read from a source, so a file whose contents are themselves path-shaped ("report.csv") failed csvPutInput's shape check — a valid CSV rejected as a caller who forgot the @. Marking it makes --file behave exactly like --csv @ all the way down, which also lets the guard skip it on its own rather than through a special case in Validate. RuntimeContext gains an exported MarkInputResolved for the second half: the bit already existed for @file / stdin, and a domain that resolves a source itself needs to set it. No other domain calls it, so nothing else changes. Also assert the typed contract (Param, Cause) rather than message text alone in the style-prescription corpus and the collapsed-issue fold, per the repo's error-test guideline. --- shortcuts/common/runner.go | 12 ++- shortcuts/common/testing.go | 2 +- shortcuts/sheets/csv_put_guard_test.go | 55 ++++++++--- shortcuts/sheets/flag_ergonomics.go | 96 +++++++++++++++++--- shortcuts/sheets/flag_ergonomics_test.go | 50 ++++++++++ shortcuts/sheets/lark_sheet_write_cells.go | 35 +++---- shortcuts/sheets/styles_acceptance_test.go | 11 ++- shortcuts/sheets/styles_prescription_test.go | 8 ++ 8 files changed, 220 insertions(+), 49 deletions(-) diff --git a/shortcuts/common/runner.go b/shortcuts/common/runner.go index 1c6f5ce5b1..5746017da3 100644 --- a/shortcuts/common/runner.go +++ b/shortcuts/common/runner.go @@ -1266,7 +1266,13 @@ func (ctx *RuntimeContext) InputResolvedFromSource(name string) bool { return ctx.inputResolved[name] } -func (ctx *RuntimeContext) markInputResolved(name string) { +// MarkInputResolved records that the named flag now holds content read from an +// external source rather than a value typed inline, setting the bit +// InputResolvedFromSource reports. resolveInputFlags calls it for @file and +// stdin; a domain calls it when it resolves a source itself (sheets reads the +// path a +csv-put caller passed as --file), so the shape guards downstream +// treat the result as content, exactly as they would for --csv @. +func (ctx *RuntimeContext) MarkInputResolved(name string) { if ctx.inputResolved == nil { ctx.inputResolved = map[string]bool{} } @@ -1312,7 +1318,7 @@ func resolveInputFlags(rctx *RuntimeContext, flags []Flag) error { // strip a leading UTF-8 BOM so it can't corrupt the first CSV // cell or break JSON parsing downstream. rctx.Cmd.Flags().Set(fl.Name, StripUTF8BOM(string(data))) - rctx.markInputResolved(fl.Name) + rctx.MarkInputResolved(fl.Name) continue } @@ -1349,7 +1355,7 @@ func resolveInputFlags(rctx *RuntimeContext, flags []Flag) error { // strip a leading UTF-8 BOM so it // can't corrupt the first CSV cell or break JSON parsing downstream. rctx.Cmd.Flags().Set(fl.Name, StripUTF8BOM(string(data))) - rctx.markInputResolved(fl.Name) + rctx.MarkInputResolved(fl.Name) continue } } diff --git a/shortcuts/common/testing.go b/shortcuts/common/testing.go index b094b5630b..cb72b7ade0 100644 --- a/shortcuts/common/testing.go +++ b/shortcuts/common/testing.go @@ -43,7 +43,7 @@ func TestNewRuntimeContextWithBotInfo(cmd *cobra.Command, cfg *core.CliConfig, i // domain tests can exercise guards that branch on InputResolvedFromSource // without wiring the full resolveInputFlags path. func TestMarkInputResolved(rctx *RuntimeContext, name string) { - rctx.markInputResolved(name) + rctx.MarkInputResolved(name) } // TestNewRuntimeContextForAPI creates a RuntimeContext ready for HTTP tests: diff --git a/shortcuts/sheets/csv_put_guard_test.go b/shortcuts/sheets/csv_put_guard_test.go index 1f7a2cd9b0..6a5691dc50 100644 --- a/shortcuts/sheets/csv_put_guard_test.go +++ b/shortcuts/sheets/csv_put_guard_test.go @@ -153,21 +153,43 @@ func TestResolveCSVPathFromFileAlias(t *testing.T) { t.Run("reads the named file", func(t *testing.T) { rctx := newCSVFileAliasRuntime("./data.csv") - resolved, err := resolveCSVPathFromFileAlias(rctx) - if err != nil || !resolved { - t.Fatalf("resolved = %v, err = %v; want the file to be read", resolved, err) + if err := resolveCSVPathFromFileAlias(rctx); err != nil { + t.Fatalf("resolve: %v", err) } if got, _ := rctx.Cmd.Flags().GetString("csv"); got != "a,b\n1,2\n" { t.Errorf("--csv = %q, want the file contents", got) } }) + t.Run("the contents are marked source-resolved", func(t *testing.T) { + // Contents read from a file may legitimately look like anything, + // including a path — the shape guards downstream must see the same bit + // they would for --csv @, or a one-cell CSV holding "report.csv" + // is rejected as a caller who forgot the @. + if err := os.WriteFile("pathshaped.csv", []byte("report.csv\n"), 0644); err != nil { + t.Fatal(err) + } + rctx := newCSVFileAliasRuntime("./pathshaped.csv") + if err := resolveCSVPathFromFileAlias(rctx); err != nil { + t.Fatalf("resolve: %v", err) + } + if !rctx.InputResolvedFromSource("csv") { + t.Error("the rewritten value must be marked as read from a source") + } + if err := guardCSVValueIsNotFilePath(rctx); err != nil { + t.Errorf("path-shaped file contents must survive the guard, got: %v", err) + } + }) + t.Run("an out-of-tree path is rejected toward stdin", func(t *testing.T) { - _, err := resolveCSVPathFromFileAlias(newCSVFileAliasRuntime("/tmp/data.csv")) + err := resolveCSVPathFromFileAlias(newCSVFileAliasRuntime("/tmp/data.csv")) ve := requireValidation(t, err, "relative path") if ve.Param != "--file" { t.Errorf("param = %q, want the flag the caller actually typed", ve.Param) } + if ve.Cause == nil { + t.Error("the underlying path error should be preserved as Cause") + } if !strings.Contains(ve.Hint, "--csv - <") { t.Errorf("hint should offer stdin for a file outside the tree, got: %q", ve.Hint) } @@ -177,30 +199,37 @@ func TestResolveCSVPathFromFileAlias(t *testing.T) { // --file holding literal CSV was accepted before this rule existed; // naming nothing readable, it stays the --csv guard's business. rctx := newCSVFileAliasRuntime("a,b\n1,2") - resolved, err := resolveCSVPathFromFileAlias(rctx) - if err != nil || resolved { - t.Fatalf("resolved = %v, err = %v; want the value left alone", resolved, err) + if err := resolveCSVPathFromFileAlias(rctx); err != nil { + t.Fatalf("resolve: %v", err) } if got, _ := rctx.Cmd.Flags().GetString("csv"); got != "a,b\n1,2" { t.Errorf("--csv = %q, want the value untouched", got) } + if rctx.InputResolvedFromSource("csv") { + t.Error("a value that was not read from a file must not be marked resolved") + } }) t.Run("a value typed as --csv is not re-read as a path", func(t *testing.T) { // Without the alias record the rule must not fire: --csv promises text, // and its own guard (not this one) answers a path passed to it. - resolved, err := resolveCSVPathFromFileAlias(newCSVGuardRuntime("./data.csv")) - if err != nil || resolved { - t.Fatalf("resolved = %v, err = %v; want --csv left to its guard", resolved, err) + rctx := newCSVGuardRuntime("./data.csv") + if err := resolveCSVPathFromFileAlias(rctx); err != nil { + t.Fatalf("resolve: %v", err) + } + if got, _ := rctx.Cmd.Flags().GetString("csv"); got != "./data.csv" { + t.Errorf("--csv = %q, want --csv left to its guard", got) } }) t.Run("@file and stdin values are already contents", func(t *testing.T) { rctx := newCSVFileAliasRuntime("./data.csv") common.TestMarkInputResolved(rctx, "csv") - resolved, err := resolveCSVPathFromFileAlias(rctx) - if err != nil || resolved { - t.Fatalf("resolved = %v, err = %v; want a resolved value left alone", resolved, err) + if err := resolveCSVPathFromFileAlias(rctx); err != nil { + t.Fatalf("resolve: %v", err) + } + if got, _ := rctx.Cmd.Flags().GetString("csv"); got != "./data.csv" { + t.Errorf("--csv = %q, want a resolved value left alone", got) } }) } diff --git a/shortcuts/sheets/flag_ergonomics.go b/shortcuts/sheets/flag_ergonomics.go index 6d0e4b76e5..83eab0b37b 100644 --- a/shortcuts/sheets/flag_ergonomics.go +++ b/shortcuts/sheets/flag_ergonomics.go @@ -224,7 +224,7 @@ func chainFlagAliases(cmd *cobra.Command) { usable[alias] = target } } - flags := cmd.Flags() + trackers := installAliasProvenance(cmd) flagalias.InstallNormalizer(cmd, func(name string) string { if strings.Contains(name, "_") { name = strings.ReplaceAll(name, "_", "-") @@ -233,25 +233,93 @@ func chainFlagAliases(cmd *cobra.Command) { if !ok { return name } - // Remember the spelling for the one rule that needs it (a value read - // as a path only because the caller wrote --file). InstallNormalizer - // records nothing itself by design, and pflag also normalizes names - // outside parsing (AddFlag, Lookup, Set), so the record is gated on - // the parser actually walking argv — same guard flagalias.Bind uses - // for its own source tracking. - if flags.Parsed() { - if cmd.Annotations == nil { - cmd.Annotations = map[string]string{} - } - cmd.Annotations[aliasSourceAnnotation(target)] = name + if tracker := trackers[target]; tracker != nil { + tracker.pending = name } return target }) } +// ─── alias provenance ─────────────────────────────────────────────────── +// +// One rule needs to know WHICH spelling supplied a value, not just that the +// flag was set: +csv-put reads --file's value as a path (see +// resolveCSVPathFromFileAlias) while --csv's is text. InstallNormalizer +// records nothing by design, and the rewrite it performs cannot be observed +// after the fact — so the spelling is captured as it parses. +// +// Recording it from the normalizer alone does not work: pflag normalizes a +// name on Lookup and Set too, including once more with the canonical name +// immediately after the rewrite, and again on every later Lookup (Str, the +// framework's own resolveInputFlags). A record written or cleared there would +// answer for calls that are not flag occurrences at all. So the normalizer +// only stages a pending spelling and the flag's Value commits it: Value.Set +// runs exactly once per real occurrence, so the last occurrence wins and +// `--file a.csv --csv ./b.csv` correctly ends up with no alias attribution. +// +// Same shape as flagalias.Bind's own pendingSource/commit, which this cannot +// reuse: Bind advertises its aliases on the flag and in the exported +// manifest, and these rewrites are deliberately silent. + +// aliasProvenanceFlags names, per command, the canonical flags whose supplying +// spelling must be tracked. Only +csv-put's --csv qualifies: it is the one +// alias in commandFlagAliases whose value semantics differ from its target's. +// Kept explicit rather than derived, so wrapping a flag's Value stays a +// deliberate, reviewed act. +var aliasProvenanceFlags = map[string][]string{ + "+csv-put": {"csv"}, +} + +// aliasTrackingValue wraps a flag's pflag.Value to commit the staged spelling +// on each real Set. Only string flags are tracked (see aliasProvenanceFlags), +// so no richer pflag value interface is at stake. +type aliasTrackingValue struct { + pflag.Value + cmd *cobra.Command + key string + pending string +} + +func (v *aliasTrackingValue) Set(raw string) error { + if v.pending == "" { + delete(v.cmd.Annotations, v.key) + } else { + if v.cmd.Annotations == nil { + v.cmd.Annotations = map[string]string{} + } + v.cmd.Annotations[v.key] = v.pending + v.pending = "" + } + return v.Value.Set(raw) +} + +// installAliasProvenance wraps the tracked flags' values and returns the +// trackers by canonical flag name, for the normalizer to stage into. +func installAliasProvenance(cmd *cobra.Command) map[string]*aliasTrackingValue { + names := aliasProvenanceFlags[cmd.Name()] + if len(names) == 0 { + return nil + } + out := make(map[string]*aliasTrackingValue, len(names)) + for _, name := range names { + fl := cmd.Flags().Lookup(name) + if fl == nil { + continue + } + if tracked, ok := fl.Value.(*aliasTrackingValue); ok { + out[name] = tracked // already installed (PostMount composed twice) + continue + } + tracked := &aliasTrackingValue{Value: fl.Value, cmd: cmd, key: aliasSourceAnnotation(name)} + fl.Value = tracked + out[name] = tracked + } + return out +} + // aliasSourceAnnotation names the command annotation holding the alias -// spelling that supplied a canonical flag's value, or is absent when the -// canonical name was typed. +// spelling that supplied a canonical flag's value, absent when the canonical +// name supplied it. func aliasSourceAnnotation(canonical string) string { return "lark-cli/sheets-alias-source/" + canonical } diff --git a/shortcuts/sheets/flag_ergonomics_test.go b/shortcuts/sheets/flag_ergonomics_test.go index a0802732fa..53b4a6c54d 100644 --- a/shortcuts/sheets/flag_ergonomics_test.go +++ b/shortcuts/sheets/flag_ergonomics_test.go @@ -7,10 +7,12 @@ import ( "encoding/json" "errors" "fmt" + "os" "strings" "testing" "github.com/larksuite/cli/errs" + "github.com/larksuite/cli/internal/cmdutil" "github.com/larksuite/cli/shortcuts/common" "github.com/spf13/cobra" ) @@ -752,3 +754,51 @@ func TestShortcuts_RequiredFlagsMarkedInHelp(t *testing.T) { } }) } + +// TestCsvPut_FileAliasProvenance pins which spelling a value is attributed to +// when both are on one command line. The record is committed by the flag's +// Value on each real occurrence, so the LAST occurrence wins — pflag also +// normalizes names on Lookup and Set (the framework's own input resolution +// looks --csv up before Validate runs), and attributing those would either +// lose a legitimate --file or steal an explicit --csv. +func TestCsvPut_FileAliasProvenance(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) + } + + run := func(t *testing.T, extra ...string) (string, error) { + t.Helper() + args := append([]string{"--url", testURL, "--sheet-name", "s", "--start-cell", "A1"}, extra...) + stdout, _, err := runShortcutCapturingErr(t, shortcutFromRegistry(t, "+csv-put"), append(args, "--dry-run")) + return stdout, err + } + + t.Run("--file alone reads the file", func(t *testing.T) { + stdout, err := run(t, "--file", "./data.csv") + if err != nil { + t.Fatalf("--file should read the path, got: %v", err) + } + if !strings.Contains(stdout, `a,b`) { + t.Errorf("dry-run body should carry the file contents, got %q", stdout) + } + }) + + t.Run("a later --csv occurrence keeps its own semantics", func(t *testing.T) { + // The last occurrence supplied the value and it was typed --csv, so the + // path must hit the --csv guard rather than being read as a file. + _, err := run(t, "--file", "./data.csv", "--csv", "./data.csv") + requireValidation(t, err, "is an existing file, not inline CSV") + }) + + t.Run("a later --file occurrence reads the file", func(t *testing.T) { + stdout, err := run(t, "--csv", "./data.csv", "--file", "./data.csv") + if err != nil { + t.Fatalf("the last occurrence was --file, so it should read the path, got: %v", err) + } + if !strings.Contains(stdout, `a,b`) { + t.Errorf("dry-run body should carry the file contents, got %q", stdout) + } + }) +} diff --git a/shortcuts/sheets/lark_sheet_write_cells.go b/shortcuts/sheets/lark_sheet_write_cells.go index b7d522b9f4..6cf1f27144 100644 --- a/shortcuts/sheets/lark_sheet_write_cells.go +++ b/shortcuts/sheets/lark_sheet_write_cells.go @@ -400,16 +400,14 @@ var CsvPut = common.Shortcut{ cmd.MarkFlagsMutuallyExclusive("start-cell", "range") }, Validate: func(ctx context.Context, runtime *common.RuntimeContext) error { - resolved, err := resolveCSVPathFromFileAlias(runtime) - if err != nil { + // Order matters: --file's value is resolved to contents first, so the + // guard below sees the same thing it would for --csv @ — that is, + // a resolved value it skips. + if err := resolveCSVPathFromFileAlias(runtime); err != nil { return err } - // The guard asks "is this inline CSV actually a path?" — already - // answered when the value arrived under --file and was read as one. - if !resolved { - if err := guardCSVValueIsNotFilePath(runtime); err != nil { - return err - } + if err := guardCSVValueIsNotFilePath(runtime); err != nil { + return err } return validateViaInput(csvPutInput)(ctx, runtime) }, @@ -511,16 +509,16 @@ func csvPutWriteRangeFromInput(input map[string]interface{}) (string, bool) { // nothing falls through untouched — `--file` holding literal CSV text was // accepted before this rule existed and still is, judged by the same guard as // --csv rather than by this one's narrower reading. -func resolveCSVPathFromFileAlias(runtime *common.RuntimeContext) (bool, error) { +func resolveCSVPathFromFileAlias(runtime *common.RuntimeContext) error { if runtime == nil || !flagValueCameFromAlias(runtime.Cmd, "csv", "file") { - return false, nil + return nil } if runtime.InputResolvedFromSource("csv") { - return false, nil + return nil } raw := strings.TrimSpace(runtime.Str("csv")) if raw == "" || strings.HasPrefix(raw, "@") { - return false, nil + return nil } data, err := cmdutil.ReadInputFile(runtime.FileIO(), raw) if err != nil { @@ -530,16 +528,21 @@ func resolveCSVPathFromFileAlias(runtime *common.RuntimeContext) (bool, error) { // leaves the value for the --csv guard, which decides between "you // meant a path" and "this is one-cell CSV text" on the value's shape. if !errors.Is(err, fileio.ErrPathValidation) { - return false, nil + return nil } - return false, sheetsValidationForFlag("file", "--file %v", err). + return sheetsValidationForFlag("file", "--file %v", err). WithCause(err). WithHint("--file reads a path relative to the current directory; pipe a file outside it in via stdin instead (--csv - < )") } if err := runtime.Cmd.Flags().Set("csv", common.StripUTF8BOM(string(data))); err != nil { - return false, sheetsValidationForFlag("file", "--file: %v", err).WithCause(err) + return sheetsValidationForFlag("file", "--file: %v", err).WithCause(err) } - return true, nil + // The value is now file contents, so every downstream shape check has to + // treat it as such — the same bit @file and stdin get. Without it a file + // holding one path-shaped cell ("report.csv") reads back as a caller who + // forgot the @, and csvPutInput rejects a perfectly good CSV. + runtime.MarkInputResolved("csv") + return nil } // guardCSVValueIsNotFilePath catches the common slip of passing a CSV file path diff --git a/shortcuts/sheets/styles_acceptance_test.go b/shortcuts/sheets/styles_acceptance_test.go index aab0f7ea0a..e56dd65b3b 100644 --- a/shortcuts/sheets/styles_acceptance_test.go +++ b/shortcuts/sheets/styles_acceptance_test.go @@ -270,8 +270,15 @@ func TestStylesAcceptance_PriorCorpus(t *testing.T) { t.Parallel() proto, err := acceptStyleItem(t, tc.fields) if tc.wantErr != "" { - if err == nil || !strings.Contains(err.Error(), tc.wantErr) { - t.Fatalf("want prescription containing %q, got err=%v", tc.wantErr, err) + // A prescription is only usable if it is also typed: an agent + // reads Param to know which flag to fix, and the message alone + // would keep passing if that attribution regressed. + ve := requireValidation(t, err, tc.wantErr) + if ve.Param != "--styles" { + t.Errorf("Param = %q, want --styles", ve.Param) + } + if ve.Cause == nil { + t.Error("the prescription should keep the underlying error as Cause") } return } diff --git a/shortcuts/sheets/styles_prescription_test.go b/shortcuts/sheets/styles_prescription_test.go index e2e5ff6696..3c3a7cee66 100644 --- a/shortcuts/sheets/styles_prescription_test.go +++ b/shortcuts/sheets/styles_prescription_test.go @@ -606,5 +606,13 @@ func TestAggregatedIssuesKeepPrescriptions(t *testing.T) { if !strings.Contains(ve.Message, "[same at 1 more: --writes[1]]") { t.Errorf("the collapsed issue must name where else it occurred, got %q", ve.Message) } + // Collapsing must not cost the fold's typed attribution: the flag to + // fix and the underlying error both still ride along. + if ve.Param != "--writes" { + t.Errorf("Param = %q, want --writes", ve.Param) + } + if ve.Cause == nil { + t.Error("the collapsed aggregate should keep the first issue as Cause") + } }) } From 962f2061bc22f6b7eb79914636bc4783f5b8c854 Mon Sep 17 00:00:00 2001 From: xiongyuanwen-byted Date: Sat, 29 Aug 2026 15:27:51 +0800 Subject: [PATCH 3/4] fix(sheets): answer every unreadable --file path under --file, and stage alias provenance only while parsing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both from self-review of the branch. An unreadable path passed as --file fell through to the --csv guard, which answered naming a flag the caller never typed — and for a file that exists but cannot be opened, prescribed "pass the same path with an @ prefix", which routes through this very reader and fails identically. Only one case may fall through now: a value that names nothing AND is not path-shaped, i.e. literal CSV text, which --file accepted before this rule existed. A path-shaped value naming nothing, an unreadable file, and a directory each answer under --file. The alias spelling was staged with no Parsed() guard (the first commit had one; flagalias.Bind still does). chainFlagAliases looks its aliases up while installing and pflag normalizes on Lookup, so composing PostMount twice — which installAliasProvenance explicitly anticipates — replayed "file" through the already-installed normalizer at mount time, and the next real --csv occurrence committed it: an explicit --csv path would then be read from disk instead of meeting its guard. Verified the new regression test fails without the guard. --- shortcuts/sheets/csv_put_guard_test.go | 49 ++++++++++++++++++++++ shortcuts/sheets/flag_ergonomics.go | 7 +++- shortcuts/sheets/flag_ergonomics_test.go | 23 ++++++++++ shortcuts/sheets/lark_sheet_write_cells.go | 42 +++++++++++++------ 4 files changed, 107 insertions(+), 14 deletions(-) diff --git a/shortcuts/sheets/csv_put_guard_test.go b/shortcuts/sheets/csv_put_guard_test.go index 6a5691dc50..10f438f623 100644 --- a/shortcuts/sheets/csv_put_guard_test.go +++ b/shortcuts/sheets/csv_put_guard_test.go @@ -233,3 +233,52 @@ func TestResolveCSVPathFromFileAlias(t *testing.T) { } }) } + +// TestResolveCSVPathFromFileAlias_UnreadablePaths pins that every unreadable +// path is answered under --file, the flag the caller typed. Handing these to +// the --csv guard instead named the wrong flag and, for a file that exists but +// cannot be opened, prescribed "pass it with @" — advice that routes through +// this very reader and fails identically. +func TestResolveCSVPathFromFileAlias_UnreadablePaths(t *testing.T) { + dir := t.TempDir() + cmdutil.TestChdir(t, dir) + + t.Run("a path-shaped value that names nothing", func(t *testing.T) { + err := resolveCSVPathFromFileAlias(newCSVFileAliasRuntime("./typo.csv")) + ve := requireValidation(t, err, "names no file under the current directory") + if ve.Param != "--file" { + t.Errorf("param = %q, want --file", ve.Param) + } + if ve.Cause == nil { + t.Error("the underlying read error should be preserved as Cause") + } + }) + + t.Run("a file that exists but cannot be read", func(t *testing.T) { + if err := os.WriteFile("noread.csv", []byte("a,b\n"), 0o000); err != nil { + t.Fatal(err) + } + if _, err := os.ReadFile("noread.csv"); err == nil { + t.Skip("running with rights that ignore file modes") + } + err := resolveCSVPathFromFileAlias(newCSVFileAliasRuntime("./noread.csv")) + ve := requireValidation(t, err, "cannot read file") + if ve.Param != "--file" { + t.Errorf("param = %q, want --file", ve.Param) + } + if strings.Contains(ve.Hint, "@") { + t.Errorf("@file shares this reader, so it cannot be the fix; hint was %q", ve.Hint) + } + }) + + t.Run("a directory", func(t *testing.T) { + if err := os.Mkdir("adir", 0o755); err != nil { + t.Fatal(err) + } + err := resolveCSVPathFromFileAlias(newCSVFileAliasRuntime("./adir")) + ve := requireValidation(t, err, "cannot read file") + if ve.Param != "--file" { + t.Errorf("param = %q, want --file", ve.Param) + } + }) +} diff --git a/shortcuts/sheets/flag_ergonomics.go b/shortcuts/sheets/flag_ergonomics.go index 83eab0b37b..fa0fb5a6ea 100644 --- a/shortcuts/sheets/flag_ergonomics.go +++ b/shortcuts/sheets/flag_ergonomics.go @@ -225,6 +225,7 @@ func chainFlagAliases(cmd *cobra.Command) { } } trackers := installAliasProvenance(cmd) + flags := cmd.Flags() flagalias.InstallNormalizer(cmd, func(name string) string { if strings.Contains(name, "_") { name = strings.ReplaceAll(name, "_", "-") @@ -233,7 +234,11 @@ func chainFlagAliases(cmd *cobra.Command) { if !ok { return name } - if tracker := trackers[target]; tracker != nil { + // Stage only while the parser is walking argv. pflag normalizes on + // Lookup too, and this function's own alias lookups run again if + // PostMount is composed twice — arming pending at mount time would let + // the next real --csv occurrence commit a spelling nobody typed. + if tracker := trackers[target]; tracker != nil && flags.Parsed() { tracker.pending = name } return target diff --git a/shortcuts/sheets/flag_ergonomics_test.go b/shortcuts/sheets/flag_ergonomics_test.go index 53b4a6c54d..40d4bd5a64 100644 --- a/shortcuts/sheets/flag_ergonomics_test.go +++ b/shortcuts/sheets/flag_ergonomics_test.go @@ -802,3 +802,26 @@ func TestCsvPut_FileAliasProvenance(t *testing.T) { } }) } + +// TestCsvPut_FileAliasProvenance_DoubleMount pins the staging guard. The +// ergonomics chain looks its aliases up while installing, and pflag normalizes +// a name on Lookup — so composing PostMount twice replays "file" through an +// already-installed normalizer at mount time. Without the Parsed() guard that +// arms the pending spelling before parsing starts, and the next real --csv +// occurrence commits it: an explicit --csv path would be read from disk instead +// of meeting its guard. +func TestCsvPut_FileAliasProvenance_DoubleMount(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) + } + + sc := shortcutFromRegistry(t, "+csv-put") + sc.PostMount = withFlagErgonomics(sc.PostMount) // a second, redundant pass + _, _, err := runShortcutCapturingErr(t, sc, []string{ + "--url", testURL, "--sheet-name", "s", "--start-cell", "A1", + "--csv", "./data.csv", "--dry-run", + }) + requireValidation(t, err, "is an existing file, not inline CSV") +} diff --git a/shortcuts/sheets/lark_sheet_write_cells.go b/shortcuts/sheets/lark_sheet_write_cells.go index 6cf1f27144..a850fb4ca4 100644 --- a/shortcuts/sheets/lark_sheet_write_cells.go +++ b/shortcuts/sheets/lark_sheet_write_cells.go @@ -12,6 +12,7 @@ import ( _ "image/gif" _ "image/jpeg" _ "image/png" + "io/fs" "path/filepath" "strconv" "strings" @@ -505,10 +506,13 @@ func csvPutWriteRangeFromInput(input map[string]interface{}) (string, bool) { // an absolute path is rejected here exactly as it would be there, and stdin // stays the out-of-tree route. // -// Only a value that names a readable file is claimed. A value that names -// nothing falls through untouched — `--file` holding literal CSV text was -// accepted before this rule existed and still is, judged by the same guard as -// --csv rather than by this one's narrower reading. +// Exactly one value falls through untouched: one that names nothing AND is not +// path-shaped, i.e. literal CSV text, which `--file` accepted before this rule +// existed and still does. Every other outcome is answered here, naming --file — +// the flag the caller actually typed. Handing an unreadable path to the --csv +// guard instead would answer with the wrong flag and, for a file that exists +// but cannot be read, with advice that cannot work ("pass it with @", which +// uses this very reader). func resolveCSVPathFromFileAlias(runtime *common.RuntimeContext) error { if runtime == nil || !flagValueCameFromAlias(runtime.Cmd, "csv", "file") { return nil @@ -522,17 +526,29 @@ func resolveCSVPathFromFileAlias(runtime *common.RuntimeContext) error { } data, err := cmdutil.ReadInputFile(runtime.FileIO(), raw) if err != nil { - // A rejected PATH is reported: the caller named a real location the - // policy will not read (absolute, or outside the tree), and the fix is - // stdin. Any other failure — the usual one being "no such file" — - // leaves the value for the --csv guard, which decides between "you - // meant a path" and "this is one-cell CSV text" on the value's shape. - if !errors.Is(err, fileio.ErrPathValidation) { + switch { + case errors.Is(err, fileio.ErrPathValidation): + // A real location the policy will not read (absolute, or outside + // the tree). The fix is stdin. + return sheetsValidationForFlag("file", "--file %v", err). + WithCause(err). + WithHint("--file reads a path relative to the current directory; pipe a file outside it in via stdin instead (--csv - < )") + case errors.Is(err, fs.ErrNotExist) && !csvValueLooksLikePath(raw): + // Names nothing and does not look like a path: literal CSV text + // passed under --file. Leave it for the --csv guard, which judges + // inline values on their shape. return nil + case errors.Is(err, fs.ErrNotExist): + return sheetsValidationForFlag("file", "--file %q names no file under the current directory", raw). + WithCause(err). + WithHint("--file takes a path relative to the current directory; for a file outside it, pipe the contents in instead (--csv - < )") + default: + // Exists but cannot be read (permissions, a directory). @file + // shares this reader, so pointing there would be dead advice. + return sheetsValidationForFlag("file", "--file %v", err). + WithCause(err). + WithHint("--file reads the path itself; to pass contents this process cannot open, pipe them in instead (--csv - < )") } - return sheetsValidationForFlag("file", "--file %v", err). - WithCause(err). - WithHint("--file reads a path relative to the current directory; pipe a file outside it in via stdin instead (--csv - < )") } if err := runtime.Cmd.Flags().Set("csv", common.StripUTF8BOM(string(data))); err != nil { return sheetsValidationForFlag("file", "--file: %v", err).WithCause(err) From cbe3a8f4d096c590b4059c0766aebe7b9163883b Mon Sep 17 00:00:00 2001 From: xiongyuanwen-byted Date: Sun, 30 Aug 2026 13:46:09 +0800 Subject: [PATCH 4/4] fix(sheets): reset alias staging on remount, and assert cause on every --file read error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second review pass, both valid. FlagSet.Parsed() stays true once parsing has started, so the guard added in the last commit only covers a remount that happens BEFORE the first parse. A remount afterwards — its own alias lookups running through the normalizer the first pass installed — could still leave a spelling staged for the next occurrence to commit, which would read an explicit --csv path from disk. Re-running the install now resets staging, closing the window from the other side. Verified the new parse/remount/reparse test fails without the reset. The unreadable-file and directory branches preserve the read error as Cause; their tests now assert it, matching the sibling that already did. --- shortcuts/sheets/csv_put_guard_test.go | 6 ++++ shortcuts/sheets/flag_ergonomics.go | 8 +++++- shortcuts/sheets/flag_ergonomics_test.go | 35 ++++++++++++++++++++---- 3 files changed, 42 insertions(+), 7 deletions(-) diff --git a/shortcuts/sheets/csv_put_guard_test.go b/shortcuts/sheets/csv_put_guard_test.go index 10f438f623..95a22475a7 100644 --- a/shortcuts/sheets/csv_put_guard_test.go +++ b/shortcuts/sheets/csv_put_guard_test.go @@ -266,6 +266,9 @@ func TestResolveCSVPathFromFileAlias_UnreadablePaths(t *testing.T) { if ve.Param != "--file" { t.Errorf("param = %q, want --file", ve.Param) } + if ve.Cause == nil { + t.Error("the underlying read error should be preserved as Cause") + } if strings.Contains(ve.Hint, "@") { t.Errorf("@file shares this reader, so it cannot be the fix; hint was %q", ve.Hint) } @@ -280,5 +283,8 @@ func TestResolveCSVPathFromFileAlias_UnreadablePaths(t *testing.T) { if ve.Param != "--file" { t.Errorf("param = %q, want --file", ve.Param) } + if ve.Cause == nil { + t.Error("the underlying read error should be preserved as Cause") + } }) } diff --git a/shortcuts/sheets/flag_ergonomics.go b/shortcuts/sheets/flag_ergonomics.go index fa0fb5a6ea..125f5759dc 100644 --- a/shortcuts/sheets/flag_ergonomics.go +++ b/shortcuts/sheets/flag_ergonomics.go @@ -312,7 +312,13 @@ func installAliasProvenance(cmd *cobra.Command) map[string]*aliasTrackingValue { continue } if tracked, ok := fl.Value.(*aliasTrackingValue); ok { - out[name] = tracked // already installed (PostMount composed twice) + // Already installed (PostMount composed twice). Reset staging: a + // remount's own alias lookups run through the normalizer installed + // by the first pass, and after parsing has started the Parsed() + // guard no longer stops them — a spelling left staged there would + // be committed by whichever occurrence sets the flag next. + tracked.pending = "" + out[name] = tracked continue } tracked := &aliasTrackingValue{Value: fl.Value, cmd: cmd, key: aliasSourceAnnotation(name)} diff --git a/shortcuts/sheets/flag_ergonomics_test.go b/shortcuts/sheets/flag_ergonomics_test.go index 40d4bd5a64..ad691de977 100644 --- a/shortcuts/sheets/flag_ergonomics_test.go +++ b/shortcuts/sheets/flag_ergonomics_test.go @@ -817,11 +817,34 @@ func TestCsvPut_FileAliasProvenance_DoubleMount(t *testing.T) { t.Fatal(err) } - sc := shortcutFromRegistry(t, "+csv-put") - sc.PostMount = withFlagErgonomics(sc.PostMount) // a second, redundant pass - _, _, err := runShortcutCapturingErr(t, sc, []string{ - "--url", testURL, "--sheet-name", "s", "--start-cell", "A1", - "--csv", "./data.csv", "--dry-run", + t.Run("mounted twice before parsing", func(t *testing.T) { + sc := shortcutFromRegistry(t, "+csv-put") + sc.PostMount = withFlagErgonomics(sc.PostMount) // a second, redundant pass + _, _, err := runShortcutCapturingErr(t, sc, []string{ + "--url", testURL, "--sheet-name", "s", "--start-cell", "A1", + "--csv", "./data.csv", "--dry-run", + }) + requireValidation(t, err, "is an existing file, not inline CSV") + }) + + t.Run("remounted after a parse", func(t *testing.T) { + // Parsed() stays true once parsing has started, so a remount at that + // point can stage through the normalizer the first pass installed. + // Re-running the install resets staging, which is what keeps the next + // parse's --csv occurrence from inheriting it. + parent, _, _, _ := newTestRig(t, shortcutFromRegistry(t, "+csv-put")) + parent.SetArgs([]string{"+csv-put", "--url", testURL, "--sheet-name", "s", + "--start-cell", "A1", "--file", "./data.csv", "--dry-run"}) + if err := parent.Execute(); err != nil { + t.Fatalf("first run: %v", err) + } + cmd, _, err := parent.Find([]string{"+csv-put"}) + if err != nil { + t.Fatalf("Find: %v", err) + } + withFlagErgonomics(nil)(cmd) + parent.SetArgs([]string{"+csv-put", "--url", testURL, "--sheet-name", "s", + "--start-cell", "A1", "--csv", "./data.csv", "--dry-run"}) + requireValidation(t, parent.Execute(), "is an existing file, not inline CSV") }) - requireValidation(t, err, "is an existing file, not inline CSV") }