From c8abdee931db7d9fa681ab7f76aa5ea0e1cbae7e Mon Sep 17 00:00:00 2001 From: xiongyuanwen-byted Date: Thu, 9 Jul 2026 16:44:48 +0800 Subject: [PATCH] feat(sheets): make validation errors prescriptive for hot failure modes Driven by the edit-eval-extra-35Q reports: ~70% of lark-cli sheets errors were missing-required / JSON-shape / wrong-value classes whose messages said what broke but not how to fix it, pushing agents into --help / --print-schema probe loops. - composite JSON shape errors inline a compact skeleton auto-generated from the schema (e.g. --cells -> [[{"value": ...}]]) when the type mismatch is shallow container confusion - +batch-update: missing 'shortcut' shows the entry template; a disallowed shortcut inlines the full allow-list; exceeding the 100-op cap says how many batches to split into; sub-op translator failures append the shortcut's complete input-key contract - +table-put: dtypes/formats keys that miss every column call out the A1-letter habit and inline the declared column names; empty cells in a date-typed column name the three ways out - schema enum errors suggest across casing, vocabulary aliases, and edit distance --- shortcuts/sheets/batch_op_dispatch.go | 73 ++++++- shortcuts/sheets/flag_schema_validate.go | 191 ++++++++++++++++-- shortcuts/sheets/flag_schema_validate_test.go | 136 +++++++++++++ .../sheets/lark_sheet_batch_update_test.go | 89 ++++++++ shortcuts/sheets/lark_sheet_table_io.go | 45 ++++- shortcuts/sheets/lark_sheet_table_io_test.go | 61 ++++++ 6 files changed, 569 insertions(+), 26 deletions(-) diff --git a/shortcuts/sheets/batch_op_dispatch.go b/shortcuts/sheets/batch_op_dispatch.go index e3820629a..d43916072 100644 --- a/shortcuts/sheets/batch_op_dispatch.go +++ b/shortcuts/sheets/batch_op_dispatch.go @@ -4,6 +4,7 @@ package sheets import ( + "sort" "strings" ) @@ -206,6 +207,54 @@ var batchOpDispatch = map[string]batchOpMapping{ "+float-image-delete": {"manage_float_image_object", objDeleteTranslate(floatImageDeleteSpec)}, } +// allowedBatchShortcuts lists every shortcut accepted inside +batch-update, +// sorted, for the not-allowed error hint. +func allowedBatchShortcuts() []string { + out := make([]string, 0, len(batchOpDispatch)) + for sc := range batchOpDispatch { + out = append(out, sc) + } + sort.Strings(out) + return out +} + +// subOpInputContract renders one shortcut's complete sub-op key vocabulary +// (wire-style underscore names) for the translator-failure hint: required +// flags are marked, the sheet selector pair collapses to a choose-one, and +// spreadsheet locators are omitted (reserved for the batch top level). +// Returns "" for shortcuts without a flag-defs entry. +func subOpInputContract(sc string) string { + defs, _ := loadFlagDefs() + spec, ok := defs[sc] + if !ok { + return "" + } + idFlag, nameFlag := sheetSelectorFlagsForSubOp(sc) + var keys []string + sheetSelector := "" + for _, df := range spec.Flags { + if df.Kind == "system" || df.Hidden { + continue + } + switch df.Name { + case "url", "spreadsheet-token": + continue // reserved: supplied by +batch-update top level + case idFlag, nameFlag: + sheetSelector = strings.ReplaceAll(idFlag, "-", "_") + "|" + strings.ReplaceAll(nameFlag, "-", "_") + " (choose one)" + continue + } + key := strings.ReplaceAll(df.Name, "-", "_") + if df.Required == "required" { + key += " (required)" + } + keys = append(keys, key) + } + if sheetSelector != "" { + keys = append([]string{sheetSelector}, keys...) + } + return strings.Join(keys, ", ") +} + // rejectLocalImageInBatch blocks the local-file --image source inside // +batch-update: a batch sub-op has no upload phase, so the file could not be // turned into a file_token. Callers must pass --image-token / --image-uri. @@ -271,7 +320,8 @@ func translateBatchOp(raw interface{}, token string, index int) (map[string]inte } scRaw, present := op["shortcut"] if !present { - return nil, sheetsValidationForFlag("operations", "operations[%d]: 'shortcut' field is required", index) + return nil, sheetsValidationForFlag("operations", "operations[%d]: 'shortcut' field is required", index). + WithHint(`each entry must look like {"shortcut":"+cells-set","input":{"sheet_name":"…","range":"A1:B2","cells":[[…]]}} — input uses the shortcut's own flag names`) } sc, ok := scRaw.(string) if !ok || sc == "" { @@ -279,13 +329,15 @@ func translateBatchOp(raw interface{}, token string, index int) (map[string]inte } mapping, ok := batchOpDispatch[sc] if !ok { + // Inline the full allow-list: an agent that guessed a read op or a + // fan-out wrapper can pick the right shortcut immediately instead of + // spending a --print-schema round trip on the operations enum. return nil, sheetsValidationForFlag( "operations", "operations[%d]: shortcut %q not allowed in +batch-update "+ - "(read ops / fan-out wrappers like +batch-update / +cells-batch-set-style / +cells-batch-clear / +dropdown-{update,delete} are excluded; "+ - "run `lark-cli sheets +batch-update --print-schema --flag-name operations` to see the full enum)", + "(read ops / fan-out wrappers like +batch-update / +cells-batch-set-style / +cells-batch-clear / +dropdown-{update,delete} are excluded)", index, sc, - ) + ).WithHint("allowed shortcuts: %s", strings.Join(allowedBatchShortcuts(), ", ")) } inputRaw, hasInput := op["input"] var input map[string]interface{} @@ -333,7 +385,14 @@ func translateBatchOp(raw interface{}, token string, index int) (map[string]inte sheetName := strings.TrimSpace(fv.Str(sheetNameFlag)) body, err := mapping.translate(fv, token, sheetID, sheetName) if err != nil { - return nil, sheetsValidationForFlag("operations", "operations[%d] (%s): %v", index, sc, err) + // The inner error names one problem at a time (first missing flag); + // the hint lists the sub-op's complete key contract so an agent fixes + // every gap in a single retry instead of iterating flag by flag. + verr := sheetsValidationForFlag("operations", "operations[%d] (%s): %v", index, sc, err) + if contract := subOpInputContract(sc); contract != "" { + verr = verr.WithHint("%s input keys: %s", sc, contract) + } + return nil, verr } return map[string]interface{}{ "tool_name": mapping.mcpToolName, @@ -354,7 +413,9 @@ func translateBatchOperations(rawOps []interface{}, token string) ([]interface{} return nil, sheetsValidationForFlag("operations", "--operations must be a non-empty JSON array") } if len(rawOps) > maxBatchOperations { - return nil, sheetsValidationForFlag("operations", "--operations accepts at most %d entries; got %d", maxBatchOperations, len(rawOps)) + batches := (len(rawOps) + maxBatchOperations - 1) / maxBatchOperations + return nil, sheetsValidationForFlag("operations", "--operations accepts at most %d entries; got %d", maxBatchOperations, len(rawOps)). + WithHint("split the operations into %d separate +batch-update calls of at most %d entries each", batches, maxBatchOperations) } out := make([]interface{}, 0, len(rawOps)) for i, raw := range rawOps { diff --git a/shortcuts/sheets/flag_schema_validate.go b/shortcuts/sheets/flag_schema_validate.go index af433b576..e635167e1 100644 --- a/shortcuts/sheets/flag_schema_validate.go +++ b/shortcuts/sheets/flag_schema_validate.go @@ -5,6 +5,7 @@ package sheets import ( "encoding/json" + "errors" "fmt" "sort" "strings" @@ -97,11 +98,22 @@ func validateValueAgainstSchema(fv flagView, name string, value interface{}) err // Composite-JSON shape errors (e.g. +cells-set --cells, chart // --properties) are the highest-frequency usage-layer failure for // sheets, and agents often burn several retries guessing the shape. - // Point them straight at --print-schema, which dumps the exact JSON - // Schema for this (command, flag) pair. The hint is always actionable: - // reaching this branch means entry[name] resolved a schema from the - // embedded index, and --print-schema reads that same index, so the - // suggested command is guaranteed to print it. + // A shallow type mismatch means the caller misremembered the overall + // container shape (the classic {"cells": ...} wrapper around what + // must be a bare 2D array), so inline a skeleton of the expected + // shape — that fixes the retry without a --print-schema round trip. + // Deeper failures keep the --print-schema pointer, which dumps the + // exact JSON Schema for this (command, flag) pair; reaching this + // branch means entry[name] resolved a schema from the embedded + // index, so the suggested command is guaranteed to print it. + var tm *typeMismatchError + if errors.As(vErr, &tm) && pathDepth(tm.path) <= skeletonPathDepthLimit { + if sk := schemaSkeleton(&schema, skeletonMaxDepth); sk != "" { + return sheetsValidationForFlag(name, + "--%s: %s; expected shape: %s (run `lark-cli sheets %s --print-schema --flag-name %s` for the full JSON Schema)", + name, vErr.Error(), sk, command, name).WithCause(vErr) + } + } return sheetsValidationForFlag(name, "--%s: %s; run `lark-cli sheets %s --print-schema --flag-name %s` to see the expected JSON Schema", name, vErr.Error(), command, name).WithCause(vErr) @@ -243,7 +255,7 @@ func validateAgainstSchema(value interface{}, schema *schemaProperty, path strin if schema.Type != "" { if !matchesJSONType(value, schema.Type) { - return fmt.Errorf("%sexpected type %q, got %q", pathPrefix(path), schema.Type, jsType(value)) //nolint:forbidigo // intermediate error; validateFlagAgainstSchema wraps it into a typed flag validation error with a --print-schema hint + return &typeMismatchError{path: path, expected: schema.Type, got: jsType(value)} } } @@ -279,7 +291,7 @@ func validateAgainstSchema(value interface{}, schema *schemaProperty, path strin if !matched { msg := fmt.Sprintf("%svalue %s is not in enum %s", pathPrefix(path), formatJSONValue(value), formatEnum(schema.Enum)) - if hint := suggestEnumMatch(value, schema.Enum); hint != "" { + if hint := suggestEnumForError(value, schema.Enum); hint != "" { msg += fmt.Sprintf(` (did you mean %q?)`, hint) } return fmt.Errorf("%s", msg) //nolint:forbidigo // intermediate error; validateFlagAgainstSchema wraps it into a typed flag validation error with a --print-schema hint @@ -388,6 +400,126 @@ func validateAgainstSchema(value interface{}, schema *schemaProperty, path strin return nil } +// typeMismatchError is the type-check branch of validateAgainstSchema +// as a typed error, so validateValueAgainstSchema can recognize shape +// confusion (vs. deep value errors) and inline a skeleton of the +// expected shape. Error() keeps the exact legacy wording. +type typeMismatchError struct { + path string + expected string + got string +} + +func (e *typeMismatchError) Error() string { + return fmt.Sprintf("%sexpected type %q, got %q", pathPrefix(e.path), e.expected, e.got) +} + +// pathDepth counts how many levels below the flag root a JSON path +// points at: "" → 0, "[0]" → 1, "[0][3]" → 2, "[0][3].value" → 3, +// "legend" → 1, "snapshot.axes" → 2. Every "[" and "." starts a new +// segment; a leading bare key (no bracket) is one segment of its own. +func pathDepth(path string) int { + depth := strings.Count(path, "[") + strings.Count(path, ".") + if path != "" && path[0] != '[' { + depth++ + } + return depth +} + +// Skeleton rendering bounds: a mismatch at depth ≤ 2 is container-shape +// confusion worth a skeleton; deeper mismatches are value-level and the +// full schema pointer serves better. The skeleton itself stops after +// four levels and eight keys per object so it stays one line; a wide +// object (> skeletonWideObject keys) collapses its children to type +// placeholders so every key stays visible instead of the first branch +// eating the whole line. +const ( + skeletonPathDepthLimit = 2 + skeletonMaxDepth = 4 + skeletonMaxKeys = 8 + skeletonWideObject = 2 +) + +// schemaSkeleton renders a compact single-line sketch of the shape a +// schema expects, e.g. [[{"value": …, "formula": "…", …}]] for +// +cells-set --cells. Required keys come first, then alphabetical, +// capped at skeletonMaxKeys with a trailing … marker. Values render as +// their type placeholder; enum strings show the first allowed value. +func schemaSkeleton(s *schemaProperty, depth int) string { + if s == nil { + return "…" + } + if len(s.OneOf) > 0 && s.Type == "" { + return schemaSkeleton(s.OneOf[0], depth) + } + switch s.Type { + case "array": + if depth <= 0 { + return "[…]" + } + return "[" + schemaSkeleton(s.Items, depth-1) + "]" + case "object": + if depth <= 0 || len(s.Properties) == 0 { + return "{…}" + } + keys := skeletonKeys(s) + childDepth := depth - 1 + if len(s.Properties) > skeletonWideObject { + childDepth = 0 + } + parts := make([]string, 0, len(keys)+1) + for _, k := range keys { + parts = append(parts, fmt.Sprintf("%q: %s", k, schemaSkeleton(s.Properties[k], childDepth))) + } + if len(s.Properties) > len(keys) { + parts = append(parts, "…") + } + return "{" + strings.Join(parts, ", ") + "}" + case "string": + if len(s.Enum) > 0 { + return formatJSONValue(s.Enum[0]) + } + return `"…"` + case "number", "integer": + return "0" + case "boolean": + return "false" + } + return "…" +} + +// skeletonKeys picks which object keys a skeleton shows: required keys +// first (schema order), then remaining keys alphabetically, capped at +// skeletonMaxKeys. +func skeletonKeys(s *schemaProperty) []string { + keys := make([]string, 0, skeletonMaxKeys) + seen := make(map[string]struct{}, skeletonMaxKeys) + for _, k := range s.Required { + if _, ok := s.Properties[k]; !ok { + continue + } + if len(keys) == skeletonMaxKeys { + return keys + } + keys = append(keys, k) + seen[k] = struct{}{} + } + rest := make([]string, 0, len(s.Properties)) + for k := range s.Properties { + if _, dup := seen[k]; !dup { + rest = append(rest, k) + } + } + sort.Strings(rest) + for _, k := range rest { + if len(keys) == skeletonMaxKeys { + break + } + keys = append(keys, k) + } + return keys +} + func matchesJSONType(value interface{}, expected string) bool { switch expected { case "object": @@ -473,25 +605,48 @@ func joinFormatted(values []interface{}) string { return strings.Join(parts, ", ") } -// suggestEnumMatch returns a "did you mean" candidate when the user's -// value differs from an allowed enum entry only in casing — the most -// common real-world mistake ("SUM" vs "sum", "True" vs "true"). The -// match is restricted to strings; non-string enums (numbers, etc.) -// don't have a casing notion. Returns "" when no near-miss exists. +// suggestEnumMatch returns the canonical enum entry when the user's +// value unambiguously means one — casing ("SUM" vs "sum", "True" vs +// "true") or a cross-vocabulary alias (CSS "center" for Lark's vertical +// "middle"). Callers auto-apply the result, so it must stay restricted +// to unambiguous matches (edit-distance guesses belong in +// suggestEnumForError only). Non-string values have no vocabulary +// notion. Returns "" when no unambiguous match exists. func suggestEnumMatch(value interface{}, values []interface{}) string { s, ok := value.(string) if !ok { return "" } - lower := strings.ToLower(s) + canon := canonicalEnumValue(s, stringEnumEntries(values)) + if canon == "" || canon == s { // skip exact-equal (already would have matched). + return "" + } + return canon +} + +// stringEnumEntries extracts the string members of a JSON-schema enum +// list (mixed-type enums keep only their string entries). +func stringEnumEntries(values []interface{}) []string { + out := make([]string, 0, len(values)) for _, v := range values { - if vs, ok := v.(string); ok && strings.ToLower(vs) == lower { - if vs != s { // skip exact-equal (already would have matched). - return vs - } + if vs, ok := v.(string); ok { + out = append(out, vs) } } - return "" + return out +} + +// suggestEnumForError picks the "did you mean" candidate for an enum +// error message. Unlike suggestEnumMatch (whose result is auto-applied, +// so it must stay unambiguous), this one may also draw on edit distance +// — the suggestion is only prose, the user still has to re-issue the +// value explicitly. +func suggestEnumForError(value interface{}, values []interface{}) string { + s, ok := value.(string) + if !ok { + return "" + } + return closestEnumValue(s, stringEnumEntries(values)) } func pathPrefix(path string) string { diff --git a/shortcuts/sheets/flag_schema_validate_test.go b/shortcuts/sheets/flag_schema_validate_test.go index 388b949e1..90b37f40f 100644 --- a/shortcuts/sheets/flag_schema_validate_test.go +++ b/shortcuts/sheets/flag_schema_validate_test.go @@ -360,6 +360,142 @@ func TestValidateInputAgainstSchema_RealEnumCaseNormalized(t *testing.T) { } } +// TestValidateAgainstSchema_EnumAliasNormalized pins the cross-vocabulary +// auto-fix: CSS-habit "center" for a vertical alignment unambiguously means +// Lark's "middle", so the payload is normalized in place and the call +// proceeds — same treatment as the "SUM" vs "sum" casing class. +func TestValidateAgainstSchema_EnumAliasNormalized(t *testing.T) { + t.Parallel() + schema := parseSchema(t, `{ + "type":"object", + "properties":{"vertical_alignment":{"type":"string","enum":["top","middle","bottom"]}} + }`) + obj := map[string]interface{}{"vertical_alignment": "center"} + if err := validateAgainstSchema(obj, schema, ""); err != nil { + t.Fatalf("center should normalize to middle and pass, got: %v", err) + } + if got := obj["vertical_alignment"]; got != "middle" { + t.Errorf("vertical_alignment = %q, want normalized to %q", got, "middle") + } +} + +// TestValidateAgainstSchema_EnumTypoSuggestedNotApplied pins the auto-apply +// boundary on the error path: an edit-distance typo stays an error with a +// "did you mean" suggestion, never a silent rewrite. +func TestValidateAgainstSchema_EnumTypoSuggestedNotApplied(t *testing.T) { + t.Parallel() + schema := parseSchema(t, `{ + "type":"object", + "properties":{"order":{"type":"string","enum":["asc","desc"]}} + }`) + obj := map[string]interface{}{"order": "ascc"} + err := validateAgainstSchema(obj, schema, "") + if err == nil { + t.Fatal("typo must be rejected") + } + if !strings.Contains(err.Error(), `did you mean "asc"?`) { + t.Errorf("enum error should suggest asc for the typo, got %q", err.Error()) + } + if got := obj["order"]; got != "ascc" { + t.Errorf("typo must not be rewritten, got %q", got) + } +} + +// TestValidateValueAgainstSchema_ShapeSkeletonOnShallowTypeMismatch +// pins the highest-frequency eval failure: passing an object where +// --cells expects a 2D array must inline a skeleton of the expected +// shape (with the "value" key visible) so an agent fixes the retry +// without a --print-schema round trip. +func TestValidateValueAgainstSchema_ShapeSkeletonOnShallowTypeMismatch(t *testing.T) { + t.Parallel() + fv := mapFlagView{command: "+cells-set"} + err := validateValueAgainstSchema(fv, "cells", + map[string]interface{}{"cells": []interface{}{}}) + if err == nil { + t.Fatal("object where array expected must fail") + } + msg := err.Error() + for _, want := range []string{`expected type "array", got "object"`, "expected shape: [[{", `"value"`} { + if !strings.Contains(msg, want) { + t.Errorf("error should contain %q, got %q", want, msg) + } + } + + // Deep value-level mismatch keeps the plain --print-schema pointer + // (a whole-shape skeleton would not address the actual problem). + deep := []interface{}{[]interface{}{ + map[string]interface{}{"note": 12.5}, + }} + err = validateValueAgainstSchema(fv, "cells", deep) + if err == nil { + t.Fatal("wrong type for note must fail") + } + if strings.Contains(err.Error(), "expected shape:") { + t.Errorf("deep mismatch should not inline a skeleton, got %q", err.Error()) + } + if !strings.Contains(err.Error(), "--print-schema") { + t.Errorf("deep mismatch should keep the --print-schema pointer, got %q", err.Error()) + } +} + +func TestPathDepth(t *testing.T) { + t.Parallel() + cases := []struct { + path string + want int + }{ + {"", 0}, + {"[0]", 1}, + {"[0][3]", 2}, + {"[0][3].value", 3}, + {"legend", 1}, + {"snapshot.axes", 2}, + } + for _, c := range cases { + if got := pathDepth(c.path); got != c.want { + t.Errorf("pathDepth(%q) = %d, want %d", c.path, got, c.want) + } + } +} + +func TestSchemaSkeleton(t *testing.T) { + t.Parallel() + schema := parseSchema(t, `{ + "type":"array", + "items":{ + "type":"array", + "items":{ + "type":"object", + "properties":{ + "value":{}, + "formula":{"type":"string"}, + "align":{"type":"string","enum":["top","middle","bottom"]}, + "styles":{"type":"object","properties":{"bold":{"type":"boolean"}}} + } + } + } + }`) + got := schemaSkeleton(schema, skeletonMaxDepth) + // Wide object (>2 keys) collapses nested containers to placeholders; + // enum strings surface their first allowed value. + want := `[[{"align": "top", "formula": "…", "styles": {…}, "value": …}]]` + if got != want { + t.Errorf("skeleton = %s, want %s", got, want) + } + + narrow := parseSchema(t, `{ + "type":"object", + "required":["sheets"], + "properties":{"sheets":{"type":"array","items":{"type":"object","properties":{"name":{"type":"string"}}}}} + }`) + got = schemaSkeleton(narrow, skeletonMaxDepth) + // Narrow object (≤2 keys) keeps descending so the inner shape shows. + want = `{"sheets": [{"name": "…"}]}` + if got != want { + t.Errorf("narrow skeleton = %s, want %s", got, want) + } +} + // TestValidateAgainstSchema_NilSchemaSafe pins the defensive // `if schema == nil { return nil }` guard. Current production callers // always hand validator a real schema, but the guard means future diff --git a/shortcuts/sheets/lark_sheet_batch_update_test.go b/shortcuts/sheets/lark_sheet_batch_update_test.go index 486f20b95..31e54b7be 100644 --- a/shortcuts/sheets/lark_sheet_batch_update_test.go +++ b/shortcuts/sheets/lark_sheet_batch_update_test.go @@ -5,6 +5,7 @@ package sheets import ( "encoding/json" + "strings" "testing" ) @@ -419,6 +420,94 @@ func TestBatchUpdate_TranslatorRejects(t *testing.T) { } } +// TestBatchUpdate_PrescriptiveHints pins the recovery hints that ride on the +// highest-frequency batch failures, so an agent can repair its payload in a +// single retry without --help / --print-schema round trips. +func TestBatchUpdate_PrescriptiveHints(t *testing.T) { + t.Parallel() + cases := []struct { + name string + opsJSON string + wantMatch string + wantInHint []string + }{ + { + name: "missing shortcut gets entry template", + opsJSON: `[{"input":{"range":"A1"}}]`, + wantMatch: "'shortcut' field is required", + wantInHint: []string{`{"shortcut":"+cells-set"`, `"input"`}, + }, + { + name: "disallowed shortcut lists the allow-list inline", + opsJSON: `[{"shortcut":"+cells-batch-set-style","input":{}}]`, + wantMatch: "not allowed in +batch-update", + wantInHint: []string{"allowed shortcuts:", "+cells-set-style", "+range-copy"}, + }, + { + name: "translator failure lists full key contract", + opsJSON: `[{"shortcut":"+dim-insert","input":{"sheet_name":"s"}}]`, + wantMatch: "--position is required", + wantInHint: []string{"+dim-insert input keys:", "sheet_id|sheet_name (choose one)", "position (required)", "count (required)", "inherit_style"}, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + _, _, err := runShortcutCapturingErr(t, BatchUpdate, []string{ + "--url", testURL, + "--operations", tc.opsJSON, + "--yes", + "--dry-run", + }) + ve := requireValidation(t, err, tc.wantMatch) + for _, want := range tc.wantInHint { + if !strings.Contains(ve.Hint, want) { + t.Errorf("hint should contain %q, got %q", want, ve.Hint) + } + } + }) + } +} + +// TestTranslateBatchOperations_OverLimitSplitHint pins the split +// prescription on the 100-entry cap: the hint must say how many batches +// the caller should re-issue. +func TestTranslateBatchOperations_OverLimitSplitHint(t *testing.T) { + t.Parallel() + ops := make([]interface{}, 185) + for i := range ops { + ops[i] = map[string]interface{}{"shortcut": "+cells-set", "input": map[string]interface{}{}} + } + _, err := translateBatchOperations(ops, "shtcnX") + ve := requireValidation(t, err, "accepts at most 100 entries; got 185") + for _, want := range []string{"2 separate +batch-update calls", "at most 100 entries each"} { + if !strings.Contains(ve.Hint, want) { + t.Errorf("hint should contain %q, got %q", want, ve.Hint) + } + } +} + +// TestSubOpInputContract pins the contract line derivation from flag-defs: +// reserved spreadsheet locators are omitted, the sheet selector collapses +// to a choose-one, and required flags are marked. +func TestSubOpInputContract(t *testing.T) { + t.Parallel() + got := subOpInputContract("+dim-insert") + for _, want := range []string{"sheet_id|sheet_name (choose one)", "position (required)", "count (required)", "inherit_style"} { + if !strings.Contains(got, want) { + t.Errorf("contract should contain %q, got %q", want, got) + } + } + for _, banned := range []string{"url", "spreadsheet_token", "dry_run"} { + if strings.Contains(got, banned) { + t.Errorf("contract must not expose %q, got %q", banned, got) + } + } + if got := subOpInputContract("+no-such-shortcut"); got != "" { + t.Errorf("unknown shortcut should yield empty contract, got %q", got) + } +} + // TestBatchUpdate_DimFreezeInjectsFreeze covers the static-freeze-only // path: +dim-freeze always injects operation=freeze (count==0 unfreeze // path of the single shortcut is intentionally not supported in batch). diff --git a/shortcuts/sheets/lark_sheet_table_io.go b/shortcuts/sheets/lark_sheet_table_io.go index 0d92504ff..26259fa72 100644 --- a/shortcuts/sheets/lark_sheet_table_io.go +++ b/shortcuts/sheets/lark_sheet_table_io.go @@ -317,17 +317,52 @@ func (in *tableSheetIn) normalize(idx int) (tableSheetSpec, error) { // compare against the canonical set. for k := range in.Dtypes { if !seenCol[k] { - return tableSheetSpec{}, common.ValidationErrorf("--sheets[%d] %q: dtypes references unknown column %q", idx, in.Name, 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 { if !seenCol[k] { - return tableSheetSpec{}, common.ValidationErrorf("--sheets[%d] %q: formats references unknown column %q", idx, in.Name, k) + return tableSheetSpec{}, common.ValidationErrorf("--sheets[%d] %q: formats references unknown column %q", idx, in.Name, k). + WithHint("%s", columnKeyHint("formats", k, in.Columns)) } } return spec, nil } +// columnKeyHint explains a dtypes/formats key that matched no column. The +// dominant failure is Excel habit — keying by column letter (A/B/AA) instead +// of the column name — so call that out explicitly; either way, inline the +// declared column names so the retry needs no second look at the payload. +func columnKeyHint(field, key string, columns []string) string { + shown := columns + const maxShown = 12 + suffix := "" + if len(shown) > maxShown { + shown = shown[:maxShown] + suffix = ", …" + } + list := `"` + strings.Join(shown, `", "`) + `"` + suffix + if isColumnLetterKey(key) { + return fmt.Sprintf("%s keys must be column names from `columns`, not A1-style column letters; this sheet's columns: %s", field, list) + } + return fmt.Sprintf("%s keys must exactly match a name in `columns`: %s", field, list) +} + +// isColumnLetterKey reports whether key looks like an A1-style column letter +// (A, B, AA, …) rather than a real column name. +func isColumnLetterKey(key string) bool { + if key == "" || len(key) > 3 { + return false + } + for _, r := range key { + if r < 'A' || r > 'Z' { + return false + } + } + return true +} + func (p *tablePayload) validate() error { if len(p.Sheets) == 0 { return common.ValidationErrorf("--sheets: must contain at least one sheet") @@ -584,6 +619,12 @@ var excelEpoch = time.Date(1899, 12, 30, 0, 0, 0, 0, time.UTC) // parser still rejects it cleanly. func isoDateToSerial(s string) (int, error) { s = strings.TrimSpace(s) + if s == "" { + // Empty cells in a date-typed column are the classic header/total-row + // clash with the column-wide dtype declaration; name the three ways + // out so the caller does not have to guess what "bad format" means. + return 0, fmt.Errorf("date column has an empty cell — drop the empty rows, fill real yyyy-mm-dd dates, or declare the column dtype as object (text)") //nolint:forbidigo // intermediate error; callers wrap it into a typed --sheets/--values validation error with row/column context + } if i := strings.Index(s, "T"); i > 0 { s = s[:i] } diff --git a/shortcuts/sheets/lark_sheet_table_io_test.go b/shortcuts/sheets/lark_sheet_table_io_test.go index 342ffd6c6..f11b330e2 100644 --- a/shortcuts/sheets/lark_sheet_table_io_test.go +++ b/shortcuts/sheets/lark_sheet_table_io_test.go @@ -54,6 +54,67 @@ func TestTablePut_IsoDateToSerial(t *testing.T) { } } +// TestTablePut_EmptyDatePrescription pins the empty-cell branch: the error +// must name the three ways out (drop rows / fill dates / object dtype) +// instead of the generic "must be ISO" parse failure. +func TestTablePut_EmptyDatePrescription(t *testing.T) { + t.Parallel() + for _, in := range []string{"", " "} { + _, err := isoDateToSerial(in) + if err == nil { + t.Fatalf("isoDateToSerial(%q) should fail", in) + } + for _, want := range []string{"empty cell", "object (text)"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("isoDateToSerial(%q) error should contain %q, got %q", in, want, err.Error()) + } + } + } +} + +// TestTablePut_ColumnKeyHint pins the dtypes/formats unknown-column hint: +// A1-style letter keys get the Excel-habit callout, and the declared column +// names ride inline either way. +func TestTablePut_ColumnKeyHint(t *testing.T) { + t.Parallel() + cols := []string{"姓名", "出生日期"} + got := columnKeyHint("dtypes", "A", cols) + for _, want := range []string{"not A1-style column letters", `"姓名", "出生日期"`} { + if !strings.Contains(got, want) { + t.Errorf("letter-key hint should contain %q, got %q", want, got) + } + } + got = columnKeyHint("formats", "出生 日期", cols) + if strings.Contains(got, "A1-style") { + t.Errorf("non-letter key must not get the letter callout, got %q", got) + } + if !strings.Contains(got, `"姓名", "出生日期"`) { + t.Errorf("hint should inline column names, got %q", got) + } + + many := make([]string, 20) + for i := range many { + many[i] = fmt.Sprintf("col%02d", i) + } + got = columnKeyHint("dtypes", "X", many) + if !strings.Contains(got, ", …") || strings.Contains(got, "col19") { + t.Errorf("hint should truncate long column lists, got %q", got) + } +} + +func TestIsColumnLetterKey(t *testing.T) { + t.Parallel() + cases := map[string]bool{ + "A": true, "Z": true, "AA": true, "ABC": true, + "": false, "ABCD": false, "a": false, "A1": false, "姓名": false, + } + for in, want := range cases { + if got := isColumnLetterKey(in); got != want { + t.Errorf("isColumnLetterKey(%q) = %v, want %v", in, got, want) + } + } +} + func TestTablePut_BuildTypedCell(t *testing.T) { t.Parallel()