diff --git a/shortcuts/sheets/batch_op_contract_test.go b/shortcuts/sheets/batch_op_contract_test.go index f8ade3f1e..3129fc1b0 100644 --- a/shortcuts/sheets/batch_op_contract_test.go +++ b/shortcuts/sheets/batch_op_contract_test.go @@ -467,6 +467,69 @@ func TestBatchOp_ErrorEquivalence(t *testing.T) { } } +// 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: +// - +csv-put: standalone requires one-of(start-cell, range) via cobra's +// MarkFlagsOneRequired (PostMount); a batch sub-op never runs cobra. +// - +sheet-move: standalone requires --index (>=0) and source-index>=0 in +// SheetMove.Validate; the batch path uses a dedicated builder. +// +// Without an explicit guard, mapFlagView's flag-default fallback silently wins +// (start-cell→"A1", index→0), so the batch sub-op diverges from the standalone +// contract instead of failing. +func TestBatchOp_GuardsBeyondCobra(t *testing.T) { + t.Parallel() + cases := []struct { + name string + subShortcut string + subInput string + wantContains string + }{ + { + name: "+csv-put without start-cell or range", + subShortcut: "+csv-put", + subInput: `{"sheet-id":"sh1","csv":"a,b"}`, + wantContains: "--start-cell or --range is required", + }, + { + name: "+sheet-move without index", + subShortcut: "+sheet-move", + subInput: `{"sheet-id":"sh1","source-index":2}`, + wantContains: "requires index", + }, + { + name: "+sheet-move negative index", + subShortcut: "+sheet-move", + subInput: `{"sheet-id":"sh1","source-index":2,"index":-1}`, + wantContains: "--index must be >= 0", + }, + { + name: "+sheet-move negative source-index", + subShortcut: "+sheet-move", + subInput: `{"sheet-id":"sh1","source-index":-1,"index":0}`, + wantContains: "--source-index must be >= 0", + }, + } + 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 bad input; 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_RejectsBadSubOpInput pins down the secondary guard: for // inputs that cobra's MarkFlagRequired catches on the standalone path, // the +batch-update sub-op (which has no cobra layer) must still reject diff --git a/shortcuts/sheets/batch_op_dispatch.go b/shortcuts/sheets/batch_op_dispatch.go index 82a0cd258..a348f3d4c 100644 --- a/shortcuts/sheets/batch_op_dispatch.go +++ b/shortcuts/sheets/batch_op_dispatch.go @@ -213,6 +213,19 @@ func sheetMoveBatchInput(fv flagView, token, sheetID, sheetName string) (map[str if !fv.Changed("source-index") { return nil, common.FlagErrorf("+sheet-move in +batch-update requires source_index (auto-derive needs a network lookup unavailable mid-batch)") } + if fv.Int("source-index") < 0 { + return nil, common.FlagErrorf("--source-index must be >= 0") + } + // Standalone +sheet-move requires --index (see SheetMove.Validate). A batch + // sub-op skips that path, and mapFlagView falls back to the flag default (0), + // which would silently move the sheet to the front. Require it explicitly so + // the batch contract matches the standalone one. + if !fv.Changed("index") { + return nil, common.FlagErrorf("+sheet-move in +batch-update requires index") + } + if fv.Int("index") < 0 { + return nil, common.FlagErrorf("--index must be >= 0") + } return map[string]interface{}{ "excel_id": token, "operation": "move", diff --git a/shortcuts/sheets/csv_put_range_alias_test.go b/shortcuts/sheets/csv_put_range_alias_test.go index 707eebd1d..4a631d6f6 100644 --- a/shortcuts/sheets/csv_put_range_alias_test.go +++ b/shortcuts/sheets/csv_put_range_alias_test.go @@ -3,7 +3,10 @@ package sheets -import "testing" +import ( + "strings" + "testing" +) // +csv-put locates with --start-cell, while +csv-get / +cells-set locate with // --range. Agents routinely carry --range over to +csv-put and hit a guaranteed @@ -35,16 +38,20 @@ func TestCsvPutInput_RangeAliasForStartCell(t *testing.T) { } } -// With neither --start-cell nor --range set, +csv-put keeps its existing -// behavior: --start-cell defaults to A1, so the paste anchors at A1. -func TestCsvPutInput_DefaultsToA1(t *testing.T) { +// With neither --start-cell nor --range explicitly set, csvPutInput rejects the +// call instead of silently anchoring at the "A1" flag default. Standalone never +// reaches this path — cobra's MarkFlagsOneRequired(start-cell, range) catches it +// first — but a +batch-update sub-op skips cobra, so the guard must live in the +// shared builder too. Otherwise a batch +csv-put with no anchor silently pastes +// at A1, diverging from the standalone contract. +func TestCsvPutInput_RequiresStartCellOrRange(t *testing.T) { fv := newMapFlagViewForCommand("+csv-put", map[string]interface{}{"csv": "a,b"}) - input, err := csvPutInput(fv, "tok", "sid", "") - if err != nil { - t.Fatalf("csvPutInput returned error: %v", err) + _, err := csvPutInput(fv, "tok", "sid", "") + if err == nil { + t.Fatal("csvPutInput accepted missing start-cell/range; want a required-flag error") } - if got, _ := input["start_cell"].(string); got != "A1" { - t.Errorf("start_cell = %q, want %q (default)", got, "A1") + if !strings.Contains(err.Error(), "--start-cell or --range is required") { + t.Errorf("error = %q, want it to mention '--start-cell or --range is required'", err.Error()) } } diff --git a/shortcuts/sheets/execute_paths_test.go b/shortcuts/sheets/execute_paths_test.go index 25932768c..1906faf53 100644 --- a/shortcuts/sheets/execute_paths_test.go +++ b/shortcuts/sheets/execute_paths_test.go @@ -271,6 +271,52 @@ func TestExecute_BatchUpdate_Translated(t *testing.T) { } } +// TestExecute_BatchUpdate_ContinueOnErrorPrecedence locks the flag-vs-envelope +// precedence: an explicit --continue-on-error=false must keep the strict +// transaction even when the --operations envelope carries continue_on_error:true, +// while an envelope value still applies when the flag is absent. Guards against +// the regression where the flag was read by value (runtime.Bool) rather than by +// Changed(). +func TestExecute_BatchUpdate_ContinueOnErrorPrecedence(t *testing.T) { + t.Parallel() + envelope := `{"operations":[{"shortcut":"+cells-set","input":{"sheet-id":"sh1","range":"A1","cells":[[{"value":1}]]}}],"continue_on_error":true}` + + t.Run("explicit false overrides envelope", func(t *testing.T) { + t.Parallel() + stub := toolOutputStub(testToken, "write", `{"results":[{"ok":true}]}`) + _, err := runShortcutWithStubs(t, BatchUpdate, []string{ + "--url", testURL, + "--operations", envelope, + "--continue-on-error=false", + "--yes", + }, stub) + if err != nil { + t.Fatalf("execute failed: %v", err) + } + input := decodeToolInput(t, decodeRawEnvelopeBody(t, stub.CapturedBody), "batch_update") + if input["continue_on_error"] == true { + t.Errorf("explicit --continue-on-error=false must win over envelope; got continue_on_error=%#v", input["continue_on_error"]) + } + }) + + t.Run("envelope applies when flag absent", func(t *testing.T) { + t.Parallel() + stub := toolOutputStub(testToken, "write", `{"results":[{"ok":true}]}`) + _, err := runShortcutWithStubs(t, BatchUpdate, []string{ + "--url", testURL, + "--operations", envelope, + "--yes", + }, stub) + if err != nil { + t.Fatalf("execute failed: %v", err) + } + input := decodeToolInput(t, decodeRawEnvelopeBody(t, stub.CapturedBody), "batch_update") + if input["continue_on_error"] != true { + t.Errorf("envelope continue_on_error:true should apply when --continue-on-error absent; got %#v", input["continue_on_error"]) + } + }) +} + // TestExecute_WorkbookCreate covers the create POST + first-sheet lookup + // set_cell_range follow-up. Stubs all three endpoints. func TestExecute_WorkbookCreate(t *testing.T) { diff --git a/shortcuts/sheets/lark_sheet_batch_update.go b/shortcuts/sheets/lark_sheet_batch_update.go index 3df12ca8f..696c40f23 100644 --- a/shortcuts/sheets/lark_sheet_batch_update.go +++ b/shortcuts/sheets/lark_sheet_batch_update.go @@ -104,11 +104,16 @@ func batchUpdateInput(runtime *common.RuntimeContext, token string) (map[string] "excel_id": token, "operations": translated, } - if runtime.Bool("continue-on-error") { - input["continue_on_error"] = true + if runtime.Changed("continue-on-error") { + // An explicit --continue-on-error always wins over the envelope, so + // --continue-on-error=false keeps the strict-transaction default even + // when the --operations envelope carries continue_on_error:true. + if runtime.Bool("continue-on-error") { + input["continue_on_error"] = true + } } else if envelope, _ := parseJSONFlag(runtime, "operations"); envelope != nil { - // Honor an inline override when --operations is an envelope object - // rather than a bare operations array. + // No explicit flag: honor an inline override when --operations is an + // envelope object rather than a bare operations array. if m, ok := envelope.(map[string]interface{}); ok { if v, ok := m["continue_on_error"].(bool); ok && v { input["continue_on_error"] = true @@ -472,6 +477,16 @@ func validateDropdownRanges(runtime *common.RuntimeContext) ([]string, error) { if !strings.Contains(s, "!") { return nil, common.FlagErrorf("--ranges[%d] (%q) must include a sheet prefix", i, s) } + // Validate the sheet!range shape up front so malformed entries like + // "!A1" (no sheet), "Sheet1!" (no range) or "Sheet1!bad" (bad ref) fail + // here at Validate instead of slipping through to DryRun/Execute. + _, sub, err := splitSheetPrefixedRange(s) + if err != nil { + return nil, common.FlagErrorf("--ranges[%d]: %v", i, err) + } + if _, _, err := rangeDimensions(sub); err != nil { + return nil, common.FlagErrorf("--ranges[%d] (%q): %v", i, s, err) + } out = append(out, s) } return out, nil diff --git a/shortcuts/sheets/lark_sheet_batch_update_test.go b/shortcuts/sheets/lark_sheet_batch_update_test.go index 0d957fca4..e122c1355 100644 --- a/shortcuts/sheets/lark_sheet_batch_update_test.go +++ b/shortcuts/sheets/lark_sheet_batch_update_test.go @@ -302,6 +302,39 @@ func TestBatchUpdate_ValidationGuards(t *testing.T) { } } +// TestValidateDropdownRanges_RejectsMalformedRange locks the up-front sheet!range +// validation: entries that merely contain "!" but are otherwise malformed (empty +// sheet, empty range, or an unparseable A1 ref) must fail at Validate rather than +// slip through to DryRun/Execute. Covers +dropdown-update / +dropdown-delete, +// which fan out over --ranges. +func TestValidateDropdownRanges_RejectsMalformedRange(t *testing.T) { + t.Parallel() + cases := []struct { + name string + ranges string + want string + }{ + {"no sheet prefix at all", `["A1:A5"]`, "must include a sheet prefix"}, + {"empty sheet name", `["!A1:A5"]`, "must use sheet!range form"}, + {"empty range after prefix", `["Sheet1!"]`, "must use sheet!range form"}, + {"unparseable ref", `["Sheet1!bad"]`, "invalid cell ref"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + stdout, stderr, err := runShortcutCapturingErr(t, DropdownUpdate, []string{ + "--url", testURL, + "--ranges", tc.ranges, + "--options", `["a"]`, + "--dry-run", + }) + if err == nil || !strings.Contains(stdout+stderr+err.Error(), tc.want) { + t.Errorf("ranges=%s: expected error containing %q; got=%s|%s|%v", tc.ranges, tc.want, stdout, stderr, err) + } + }) + } +} + // TestBatchUpdate_TranslatorRejects covers per-op shape errors caught by // translateBatchOp: unknown shortcut, missing shortcut, banned (read / // fan-out / legacy v2) shortcuts, hand-filled reserved keys, etc. diff --git a/shortcuts/sheets/lark_sheet_write_cells.go b/shortcuts/sheets/lark_sheet_write_cells.go index 3374d2d91..07768614f 100644 --- a/shortcuts/sheets/lark_sheet_write_cells.go +++ b/shortcuts/sheets/lark_sheet_write_cells.go @@ -310,10 +310,18 @@ func csvPutInput(runtime flagView, token, sheetID, sheetName string) (map[string // defaults to "A1" and is therefore never empty. A range like "A1:H17" // collapses to its top-left cell; +csv-put pastes from the anchor and // auto-expands, so the range's lower-right bound is irrelevant. + // + // Standalone enforces "one of --start-cell / --range" via cobra's + // MarkFlagsOneRequired (see PostMount). A +batch-update sub-op never runs + // cobra, so without an explicit check the default "A1" silently wins and the + // paste lands at A1 instead of failing like the standalone command. Mirror + // the standalone contract: when --start-cell is absent, --range is mandatory. if !runtime.Changed("start-cell") { - if rng := strings.TrimSpace(runtime.Str("range")); rng != "" { - anchor = strings.TrimSpace(strings.SplitN(rng, ":", 2)[0]) + rng := strings.TrimSpace(runtime.Str("range")) + if rng == "" { + return nil, common.FlagErrorf("--start-cell or --range is required") } + anchor = strings.TrimSpace(strings.SplitN(rng, ":", 2)[0]) } if anchor == "" { return nil, common.FlagErrorf("--start-cell is required")