From e4309bb5b2dfac1d383bd66a1f01d89787b85bc0 Mon Sep 17 00:00:00 2001 From: xiongyuanwen-byted Date: Wed, 3 Jun 2026 14:10:59 +0800 Subject: [PATCH] fix(sheets): harden batch type-checking and +workbook-create edge cases From the branch code-review doc (3 findings): - +batch-update sub-ops: `operations` is skipped by parse-time schema validation and mapFlagView coerces a type-mismatched scalar to its zero value, so "index":"abc" or "multiple":"true" silently became 0 / false and wrote to the wrong place. translateBatchOp now runs validateRawTypes, which checks each sub-op scalar against its flag-defs type and rejects mismatches. - +workbook-create with empty arrays: buildInitialFillInput returned (nil,nil) for empty rows while the caller wrote fill["excel_id"] unconditionally, so --values '[]' panicked on a nil map and --headers '[]' produced an illegal "A1:1" range. It now also returns nil when no cells survive (maxCols==0 guard) and Execute/DryRun skip the fill when fill==nil. - +workbook-create partial failure: after the spreadsheet was created, a first-sheet lookup or fill failure returned a bare fmt.Errorf, losing the new token. It now returns a structured partial_success error carrying spreadsheet_token in the detail so callers can retry or clean up. Tests added for each path; sheets suite green. --- shortcuts/sheets/batch_op_contract_test.go | 51 ++++++++++++++ shortcuts/sheets/batch_op_dispatch.go | 6 ++ shortcuts/sheets/execute_paths_test.go | 77 ++++++++++++++++++++++ shortcuts/sheets/flag_view.go | 73 ++++++++++++++++++++ shortcuts/sheets/lark_sheet_workbook.go | 51 ++++++++++---- 5 files changed, 247 insertions(+), 11 deletions(-) diff --git a/shortcuts/sheets/batch_op_contract_test.go b/shortcuts/sheets/batch_op_contract_test.go index 3129fc1b0..a2750b2de 100644 --- a/shortcuts/sheets/batch_op_contract_test.go +++ b/shortcuts/sheets/batch_op_contract_test.go @@ -467,6 +467,57 @@ func TestBatchOp_ErrorEquivalence(t *testing.T) { } } +// TestBatchOp_RejectsWrongScalarType locks the type-check that closes the +// silent-coercion gap: `operations` skips parse-time schema validation, and +// mapFlagView coerces a mismatched scalar to its zero value, so a sub-op field +// whose JSON type contradicts its flag-defs type must be rejected up front +// rather than landing as 0 / false in the wrong place. +func TestBatchOp_RejectsWrongScalarType(t *testing.T) { + t.Parallel() + cases := []struct { + name string + subShortcut string + subInput string + wantContains string + }{ + { + name: "int flag given a string", + subShortcut: "+sheet-move", + subInput: `{"sheet-id":"sh1","source-index":2,"index":"abc"}`, + wantContains: "--index must be a number", + }, + { + name: "int flag given a boolean", + subShortcut: "+sheet-move", + subInput: `{"sheet-id":"sh1","source-index":true,"index":0}`, + wantContains: "--source-index must be a number", + }, + { + name: "bool flag given a string", + subShortcut: "+cells-set", + subInput: `{"sheet-id":"sh1","range":"A1","cells":[[{"value":1}]],"allow-overwrite":"true"}`, + wantContains: "--allow-overwrite must be a boolean", + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + var subInput map[string]interface{} + if err := json.Unmarshal([]byte(tc.subInput), &subInput); err != nil { + t.Fatalf("bad subInput JSON: %v", err) + } + rawOp := map[string]interface{}{"shortcut": tc.subShortcut, "input": subInput} + _, err := translateBatchOp(rawOp, testToken, 0) + if err == nil { + t.Fatalf("translateBatchOp accepted wrong-typed field; want error containing %q", tc.wantContains) + } + if !strings.Contains(err.Error(), tc.wantContains) { + t.Errorf("error = %q, want substring %q", err.Error(), tc.wantContains) + } + }) + } +} + // TestBatchOp_GuardsBeyondCobra locks the two batch sub-ops whose standalone // required-flag enforcement lives OUTSIDE the shared *Input builder — so it is // invisible to TestBatchOp_ErrorEquivalence and was missed by the refactor: diff --git a/shortcuts/sheets/batch_op_dispatch.go b/shortcuts/sheets/batch_op_dispatch.go index a348f3d4c..271d4eeca 100644 --- a/shortcuts/sheets/batch_op_dispatch.go +++ b/shortcuts/sheets/batch_op_dispatch.go @@ -306,6 +306,12 @@ func translateBatchOp(raw interface{}, token string, index int) (map[string]inte } } fv := newMapFlagViewForCommand(sc, input) + // operations is skipped by parse-time schema validation, so type-check the + // sub-op's scalar fields here before the translator reads them via + // Int/Bool/Float64 (which would otherwise coerce a wrong type to zero). + if err := fv.validateRawTypes(); err != nil { + return nil, common.FlagErrorf("operations[%d] (%s): %v", index, sc, err) + } sheetIDFlag, sheetNameFlag := sheetSelectorFlagsForSubOp(sc) sheetID := strings.TrimSpace(fv.Str(sheetIDFlag)) sheetName := strings.TrimSpace(fv.Str(sheetNameFlag)) diff --git a/shortcuts/sheets/execute_paths_test.go b/shortcuts/sheets/execute_paths_test.go index 1906faf53..27eb2b2e2 100644 --- a/shortcuts/sheets/execute_paths_test.go +++ b/shortcuts/sheets/execute_paths_test.go @@ -9,6 +9,7 @@ import ( "testing" "github.com/larksuite/cli/internal/httpmock" + "github.com/larksuite/cli/internal/output" ) // TestExecute_WorkbookInfo_Happy stubs the invoke_read endpoint and @@ -362,6 +363,82 @@ func TestExecute_WorkbookCreate(t *testing.T) { } } +// TestExecute_WorkbookCreate_EmptyArraysSkipFill locks the fix for the nil-map +// panic / illegal-range bug: --values '[]' or --headers '[]' must short-circuit +// the initial fill (no structure/fill calls fire) and finish with the +// spreadsheet created but no initial_fill — never panic on a nil fill map. +func TestExecute_WorkbookCreate_EmptyArraysSkipFill(t *testing.T) { + t.Parallel() + for _, tc := range []struct{ name, flag, val string }{ + {"empty values", "--values", "[]"}, + {"empty headers", "--headers", "[]"}, + } { + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + create := &httpmock.Stub{ + Method: "POST", + URL: "/open-apis/sheets/v3/spreadsheets", + Body: map[string]interface{}{ + "code": 0, "msg": "success", + "data": map[string]interface{}{ + "spreadsheet": map[string]interface{}{"spreadsheet_token": "shtNEW", "title": "X"}, + }, + }, + } + // Only the create stub is provided: an empty array must skip the fill + // entirely, so no structure/fill call fires (and no nil-map panic). + out, err := runShortcutWithStubs(t, WorkbookCreate, []string{"--title", "X", tc.flag, tc.val}, create) + if err != nil { + t.Fatalf("execute failed: %v\nout=%s", err, out) + } + data := decodeEnvelopeData(t, out) + if data["initial_fill"] != nil { + t.Errorf("initial_fill should be absent for %s %s; got %#v", tc.flag, tc.val, data["initial_fill"]) + } + if ss, _ := data["spreadsheet"].(map[string]interface{}); ss["spreadsheet_token"] != "shtNEW" { + t.Errorf("spreadsheet_token = %v, want shtNEW", ss["spreadsheet_token"]) + } + }) + } +} + +// TestExecute_WorkbookCreate_FillFailureKeepsToken locks the partial-success +// contract: when the spreadsheet is created but the follow-up fill can't resolve +// its first sheet, the error must be structured and retain spreadsheet_token so +// the caller can recover instead of orphaning the new workbook. +func TestExecute_WorkbookCreate_FillFailureKeepsToken(t *testing.T) { + t.Parallel() + create := &httpmock.Stub{ + Method: "POST", + URL: "/open-apis/sheets/v3/spreadsheets", + Body: map[string]interface{}{ + "code": 0, "msg": "success", + "data": map[string]interface{}{ + "spreadsheet": map[string]interface{}{"spreadsheet_token": "shtNEW", "title": "X"}, + }, + }, + } + // Structure comes back with no sheets, so lookupFirstSheetID fails AFTER the + // spreadsheet already exists — exercising the partial-success path. + structure := toolOutputStub("shtNEW", "read", `{"sheets":[]}`) + out, err := runShortcutWithStubs(t, WorkbookCreate, []string{"--title", "X", "--values", `[["a"]]`}, create, structure) + if err == nil { + t.Fatalf("expected a partial-success error; got nil\nout=%s", out) + } + exitErr, ok := err.(*output.ExitError) + if !ok { + t.Fatalf("error type = %T, want *output.ExitError (structured)", err) + } + if exitErr.Detail == nil { + t.Fatal("ExitError.Detail is nil; want structured detail carrying the token") + } + detail, _ := exitErr.Detail.Detail.(map[string]interface{}) + if detail["spreadsheet_token"] != "shtNEW" { + t.Errorf("detail.spreadsheet_token = %v, want shtNEW (must survive the fill failure)", detail["spreadsheet_token"]) + } +} + // TestExecute_DimMove covers the native v3 move_dimension call. CLI's // --source-range "1:3" (1-based inclusive) is parsed into v3's // source.{start_index=0,end_index=2} (0-based inclusive); --target "11" is diff --git a/shortcuts/sheets/flag_view.go b/shortcuts/sheets/flag_view.go index 3e922c527..1d906baf0 100644 --- a/shortcuts/sheets/flag_view.go +++ b/shortcuts/sheets/flag_view.go @@ -254,3 +254,76 @@ func (m mapFlagView) Changed(name string) bool { _, ok := m.lookupRaw(name) return ok } + +// validateRawTypes rejects sub-op input fields whose JSON type contradicts the +// flag's declared type in flag-defs. +batch-update skips parse-time schema +// validation for `operations`, and Int/Int64/Float64/Bool silently fall back to +// the zero value on a type mismatch — so without this guard a wrong-typed scalar +// (e.g. "index":"abc" or "multiple":"true") would land as 0 / false instead of +// erroring, writing to the wrong place. Only numeric and boolean flags are +// checked; string and composite (array/object) flags stay permissive because +// Str() intentionally coerces them and the translator/schema validates shape. +// +// Returns a bare error; the +batch-update translator wraps it with the +// operations[i] () context. +func (m mapFlagView) validateRawTypes() error { + if len(m.raw) == 0 { + return nil + } + defs, err := loadFlagDefs() + if err != nil { + return nil + } + spec, ok := defs[m.command] + if !ok { + return nil + } + declaredType := make(map[string]string, len(spec.Flags)) + for _, df := range spec.Flags { + declaredType[df.Name] = df.Type + } + for rawKey, val := range m.raw { + name := rawKey + typ, ok := declaredType[name] + if !ok { + // flag-defs use hyphen names; tolerate the underscore form users send. + name = strings.ReplaceAll(rawKey, "_", "-") + typ, ok = declaredType[name] + } + if !ok { + continue // unknown key — leave it for the translator / schema layer + } + switch typ { + case "int", "int64", "float64": + if _, isNum := val.(float64); !isNum { + return fmt.Errorf("--%s must be a number, got %s", name, jsonTypeName(val)) + } + case "bool": + if _, isBool := val.(bool); !isBool { + return fmt.Errorf("--%s must be a boolean, got %s", name, jsonTypeName(val)) + } + } + } + return nil +} + +// jsonTypeName names the JSON kind of a value decoded by encoding/json, for +// type-mismatch error messages. +func jsonTypeName(v interface{}) string { + switch v.(type) { + case nil: + return "null" + case bool: + return "boolean" + case float64: + return "number" + case string: + return "string" + case []interface{}: + return "array" + case map[string]interface{}: + return "object" + default: + return fmt.Sprintf("%T", v) + } +} diff --git a/shortcuts/sheets/lark_sheet_workbook.go b/shortcuts/sheets/lark_sheet_workbook.go index 2d788421c..cf4ab6498 100644 --- a/shortcuts/sheets/lark_sheet_workbook.go +++ b/shortcuts/sheets/lark_sheet_workbook.go @@ -598,8 +598,7 @@ var WorkbookCreate = common.Shortcut{ POST("/open-apis/sheets/v3/spreadsheets"). Desc("create spreadsheet"). Body(body) - if runtime.Str("headers") != "" || runtime.Str("values") != "" { - fill, _ := buildInitialFillInput(runtime) + if fill, _ := buildInitialFillInput(runtime); fill != nil { fill["excel_id"] = "" fill["sheet_id"] = "" // resolved from the workbook at execute time wireBody, _ := buildToolBody("set_cell_range", fill) @@ -629,24 +628,27 @@ var WorkbookCreate = common.Shortcut{ result := map[string]interface{}{"spreadsheet": ss} - if runtime.Str("headers") != "" || runtime.Str("values") != "" { - fill, err := buildInitialFillInput(runtime) - if err != nil { - return err - } + // --headers / --values are optional. buildInitialFillInput returns + // (nil, nil) when both are absent or empty, in which case we skip the + // fill entirely rather than dereferencing a nil map. + fill, err := buildInitialFillInput(runtime) + if err != nil { + return err + } + if fill != nil { fill["excel_id"] = token // set_cell_range needs a concrete sheet selector; the create // response doesn't echo the default sheet's id, so read it back. firstSheetID, err := lookupFirstSheetID(ctx, runtime, token) if err != nil { - return fmt.Errorf("spreadsheet %s created but resolving its first sheet for initial fill failed: %w", token, err) + return workbookCreatedButFillFailed(token, ss, + fmt.Sprintf("resolving its first sheet for initial fill failed: %v", err)) } fill["sheet_id"] = firstSheetID fillOut, err := callTool(ctx, runtime, token, ToolKindWrite, "set_cell_range", fill) if err != nil { - // Spreadsheet exists; surface the fill failure but keep the new - // token in the envelope so the caller can recover or retry. - return fmt.Errorf("spreadsheet %s created but initial fill failed: %w", token, err) + return workbookCreatedButFillFailed(token, ss, + fmt.Sprintf("initial fill failed: %v", err)) } result["initial_fill"] = fillOut } @@ -658,6 +660,26 @@ var WorkbookCreate = common.Shortcut{ }, } +// workbookCreatedButFillFailed builds a structured partial-success error for the +// window where the spreadsheet POST succeeded but the follow-up initial fill did +// not. The new spreadsheet_token is surfaced in the error detail so callers can +// retry the fill (+cells-set / +csv-put) or delete the orphan, instead of only +// finding the token interpolated into a bare error string. +func workbookCreatedButFillFailed(token string, spreadsheet interface{}, reason string) error { + return &output.ExitError{ + Code: output.ExitAPI, + Detail: &output.ErrDetail{ + Type: "partial_success", + Message: fmt.Sprintf("spreadsheet %s created but %s", token, reason), + Hint: "the spreadsheet exists; retry the fill with the returned spreadsheet_token, or delete it", + Detail: map[string]interface{}{ + "spreadsheet_token": token, + "spreadsheet": spreadsheet, + }, + }, + } +} + // buildInitialFillInput zips --headers + --values into a single set_cell_range // payload writing to the first sheet starting at A1. func buildInitialFillInput(runtime *common.RuntimeContext) (map[string]interface{}, error) { @@ -692,6 +714,13 @@ func buildInitialFillInput(runtime *common.RuntimeContext) (map[string]interface maxCols = len(r) } } + if maxCols == 0 { + // --headers '[]' / --values '[]' parse to rows that carry no cells. + // There is nothing to write and a 0-width range ("A1:1") would be + // illegal, so treat it as "no initial fill" — same contract as the + // len(rows)==0 case above — and let the caller skip the write. + return nil, nil + } // Normalize rows to the same length so cells matrix is rectangular. for i := range rows { for len(rows[i]) < maxCols {