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 dbf6583a6b..95a22475a7 100644 --- a/shortcuts/sheets/csv_put_guard_test.go +++ b/shortcuts/sheets/csv_put_guard_test.go @@ -132,3 +132,159 @@ 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") + 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")) + 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) + } + }) + + 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") + 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. + 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") + 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) + } + }) +} + +// 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 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) + } + }) + + 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) + } + 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 1880c67b67..125f5759dc 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,126 @@ func chainFlagAliases(cmd *cobra.Command) { usable[alias] = target } } + trackers := installAliasProvenance(cmd) + 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 + } + // 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 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 { + // 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)} + fl.Value = tracked + out[name] = tracked + } + return out +} + +// aliasSourceAnnotation names the command annotation holding the alias +// 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 +} + +// 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..ad691de977 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" ) @@ -692,3 +694,157 @@ 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) + } + }) +} + +// 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) + } + }) +} + +// 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) + } + + 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") + }) +} 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..a850fb4ca4 100644 --- a/shortcuts/sheets/lark_sheet_write_cells.go +++ b/shortcuts/sheets/lark_sheet_write_cells.go @@ -6,17 +6,21 @@ package sheets import ( "context" "encoding/csv" + "errors" "fmt" "image" _ "image/gif" _ "image/jpeg" _ "image/png" + "io/fs" "path/filepath" "strconv" "strings" "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 +228,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,6 +401,12 @@ var CsvPut = common.Shortcut{ cmd.MarkFlagsMutuallyExclusive("start-cell", "range") }, Validate: func(ctx context.Context, runtime *common.RuntimeContext) error { + // 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 + } if err := guardCSVValueIsNotFilePath(runtime); err != nil { return err } @@ -475,6 +487,80 @@ 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. +// +// 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 + } + if runtime.InputResolvedFromSource("csv") { + return nil + } + raw := strings.TrimSpace(runtime.Str("csv")) + if raw == "" || strings.HasPrefix(raw, "@") { + return nil + } + data, err := cmdutil.ReadInputFile(runtime.FileIO(), raw) + if err != nil { + 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 - < )") + } + } + if err := runtime.Cmd.Flags().Set("csv", common.StripUTF8BOM(string(data))); err != nil { + return sheetsValidationForFlag("file", "--file: %v", err).WithCause(err) + } + // 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 // 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..e56dd65b3b 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"}, @@ -249,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 4f47fefb1f..3c3a7cee66 100644 --- a/shortcuts/sheets/styles_prescription_test.go +++ b/shortcuts/sheets/styles_prescription_test.go @@ -574,13 +574,45 @@ 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) + } + // 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") } }) }