fix(sheets): enforce required-flag contract in batch sub-ops

Batch sub-ops reuse each shortcut's shared *Input builder through mapFlagView,
which seeds flag-defs defaults — so any required check that lives OUTSIDE the
builder (cobra MarkFlagsOneRequired, or a shortcut's own Validate) is silently
bypassed and the default value wins. Two gaps surfaced in PR review:

- +csv-put: with neither --start-cell nor --range set, start-cell's "A1"
  default won and the paste silently anchored at A1. Require an explicit anchor
  (guard on Changed, mirroring the standalone MarkFlagsOneRequired).
- +sheet-move: --index (plus >=0 bounds for index / source-index) was not
  enforced in the batch path; a missing --index silently moved the sheet to the
  front. Mirror SheetMove.Validate.

Also from the same review:
- +batch-update: an explicit --continue-on-error=false now wins over an
  --operations envelope's continue_on_error:true (guard on Changed, not value).
- validateDropdownRanges rejects malformed sheet!range ("!A1", "Sheet1!",
  "Sheet1!bad") at Validate instead of deferring to the server.

Tests added/updated for each path; full sheets suite green.
This commit is contained in:
xiongyuanwen-byted
2026-06-03 12:31:57 +08:00
parent ad7b20935c
commit da198cf06a
7 changed files with 200 additions and 15 deletions

View File

@@ -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

View File

@@ -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",

View File

@@ -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())
}
}

View File

@@ -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) {

View File

@@ -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

View File

@@ -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.

View File

@@ -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")