From d784b0c4ab1c47d70dd424501a75ecfe15c5744a Mon Sep 17 00:00:00 2001 From: hehanlin1996 <227351593+hehanlin1996@users.noreply.github.com> Date: Mon, 22 Jun 2026 17:10:00 +0800 Subject: [PATCH 1/2] fix(sheets): mark +delete-dimension high-risk-write Deleting rows/columns is an irreversible structural change, but the deprecated +delete-dimension alias was annotated Risk: "write" and so never triggered the confirmation gate. Its modern replacement +dim-delete and the sibling +delete-sheet are both high-risk-write. Align +delete-dimension with them: without --yes it now returns a structured confirmation_required error (exit 10); --yes executes; --dry-run still previews without confirmation. - Lock the risk level with a metadata test. - Add gate-behavior tests (without --yes blocked / with --yes executes / --dry-run bypasses), and pass --yes in the existing execute tests. Closes larksuite/cli#1532 Co-Authored-By: Claude Opus 4.8 (1M context) --- .../lark_sheets_row_column_management.go | 8 +- .../lark_sheets_sheet_dimension_test.go | 101 ++++++++++++++++++ 2 files changed, 106 insertions(+), 3 deletions(-) diff --git a/shortcuts/sheets/backward/lark_sheets_row_column_management.go b/shortcuts/sheets/backward/lark_sheets_row_column_management.go index ea1e913a8..f2a822c2e 100644 --- a/shortcuts/sheets/backward/lark_sheets_row_column_management.go +++ b/shortcuts/sheets/backward/lark_sheets_row_column_management.go @@ -309,9 +309,11 @@ var SheetDeleteDimension = common.Shortcut{ Service: "sheets", Command: "+delete-dimension", Description: "Delete rows or columns", - Risk: "write", - Scopes: []string{"sheets:spreadsheet:write_only", "sheets:spreadsheet:read"}, - AuthTypes: []string{"user", "bot"}, + // Deleting rows/columns is an irreversible structural change, so it + // requires the same confirmation gate (--yes) as +delete-sheet. + Risk: "high-risk-write", + Scopes: []string{"sheets:spreadsheet:write_only", "sheets:spreadsheet:read"}, + AuthTypes: []string{"user", "bot"}, Flags: []common.Flag{ {Name: "url", Desc: "spreadsheet URL"}, {Name: "spreadsheet-token", Desc: "spreadsheet token"}, diff --git a/shortcuts/sheets/backward/lark_sheets_sheet_dimension_test.go b/shortcuts/sheets/backward/lark_sheets_sheet_dimension_test.go index 51b13ff5b..15f6f46f4 100644 --- a/shortcuts/sheets/backward/lark_sheets_sheet_dimension_test.go +++ b/shortcuts/sheets/backward/lark_sheets_sheet_dimension_test.go @@ -6,6 +6,7 @@ package backward import ( "context" "encoding/json" + "net/http" "strconv" "strings" "testing" @@ -911,6 +912,7 @@ func TestSheetDeleteDimensionExecuteSuccess(t *testing.T) { "--dimension", "ROWS", "--start-index", "3", "--end-index", "7", + "--yes", "--as", "user", }, f, stdout) if err != nil { @@ -948,6 +950,7 @@ func TestSheetDeleteDimensionExecuteWithURL(t *testing.T) { "--dimension", "COLUMNS", "--start-index", "1", "--end-index", "2", + "--yes", "--as", "user", }, f, stdout) if err != nil { @@ -971,9 +974,107 @@ func TestSheetDeleteDimensionExecuteAPIError(t *testing.T) { "--dimension", "ROWS", "--start-index", "3", "--end-index", "7", + "--yes", "--as", "user", }, f, nil) if err == nil { t.Fatal("expected API error, got nil") } } + +// ── DeleteDimension: high-risk-write confirmation gate (issue #50) ──────────── + +// TestSheetDeleteDimensionRiskIsHighRiskWrite locks the risk level so a +// regression cannot silently drop +delete-dimension back below the confirmation +// gate. Deleting rows/columns is an irreversible structural change and must +// share the same risk tier as +delete-sheet. +func TestSheetDeleteDimensionRiskIsHighRiskWrite(t *testing.T) { + t.Parallel() + if SheetDeleteDimension.Risk != "high-risk-write" { + t.Fatalf("SheetDeleteDimension.Risk = %q, want \"high-risk-write\"", SheetDeleteDimension.Risk) + } +} + +// TestSheetDeleteDimensionWithoutYesRequiresConfirmation verifies that running +// +delete-dimension without --yes is blocked by the high-risk confirmation gate +// and never reaches the server. No HTTP stub is registered on purpose: the gate +// must short-circuit before Execute, so any outbound request would itself be a +// failure. Reaching a "requires confirmation" error proves the gate fired first. +func TestSheetDeleteDimensionWithoutYesRequiresConfirmation(t *testing.T) { + f, _, _, _ := cmdutil.TestFactory(t, sheetsTestConfig()) + + err := mountAndRunSheets(t, SheetDeleteDimension, []string{ + "+delete-dimension", + "--spreadsheet-token", "shtTOKEN", + "--sheet-id", "sheet1", + "--dimension", "ROWS", + "--start-index", "3", + "--end-index", "7", + "--as", "user", + }, f, nil) + if err == nil { + t.Fatal("expected confirmation-required error without --yes, got nil") + } + if !strings.Contains(err.Error(), "requires confirmation") { + t.Fatalf("expected confirmation-required error, got: %v", err) + } +} + +// TestSheetDeleteDimensionWithYesExecutes verifies that passing --yes satisfies +// the confirmation gate and the delete is performed. +func TestSheetDeleteDimensionWithYesExecutes(t *testing.T) { + f, stdout, _, reg := cmdutil.TestFactory(t, sheetsTestConfig()) + called := false + reg.Register(&httpmock.Stub{ + Method: "DELETE", + URL: "/open-apis/sheets/v2/spreadsheets/shtTOKEN/dimension_range", + Body: map[string]interface{}{"code": 0, "msg": "success", "data": map[string]interface{}{"delCount": float64(5), "majorDimension": "ROWS"}}, + OnMatch: func(*http.Request) { called = true }, + }) + + err := mountAndRunSheets(t, SheetDeleteDimension, []string{ + "+delete-dimension", + "--spreadsheet-token", "shtTOKEN", + "--sheet-id", "sheet1", + "--dimension", "ROWS", + "--start-index", "3", + "--end-index", "7", + "--yes", + "--as", "user", + }, f, stdout) + if err != nil { + t.Fatalf("unexpected error with --yes: %v", err) + } + if !called { + t.Fatal("API should be called once confirmation is satisfied with --yes") + } + if !strings.Contains(stdout.String(), `"delCount"`) { + t.Fatalf("stdout missing delCount: %s", stdout.String()) + } +} + +// TestSheetDeleteDimensionDryRunBypassesConfirmation verifies that --dry-run +// previews the delete without --yes and without hitting the server: dry-run is +// the sanctioned safe-preview path for high-risk operations, so the confirmation +// gate must not block it. No HTTP stub is registered: a dry-run that reached the +// network would be a failure. +func TestSheetDeleteDimensionDryRunBypassesConfirmation(t *testing.T) { + f, stdout, _, _ := cmdutil.TestFactory(t, sheetsTestConfig()) + + err := mountAndRunSheets(t, SheetDeleteDimension, []string{ + "+delete-dimension", + "--spreadsheet-token", "shtTOKEN", + "--sheet-id", "sheet1", + "--dimension", "ROWS", + "--start-index", "3", + "--end-index", "7", + "--dry-run", + "--as", "user", + }, f, stdout) + if err != nil { + t.Fatalf("dry-run should not require --yes, got: %v", err) + } + if !strings.Contains(stdout.String(), "DELETE") { + t.Fatalf("dry-run output should preview the DELETE request: %s", stdout.String()) + } +} From 68a4a85b69091076b3827414569a043cb85da63e Mon Sep 17 00:00:00 2001 From: hehanlin1996 <227351593+hehanlin1996@users.noreply.github.com> Date: Mon, 22 Jun 2026 17:16:45 +0800 Subject: [PATCH 2/2] test(sheets): assert typed error metadata in delete-dimension error paths Address CodeRabbit review: error-path tests must assert typed metadata via errs.ProblemOf, not nil/message-substring checks. - confirmation path: assert Category=confirmation / Subtype=confirmation_required - API-error path: assert Category=api / Code=90001 Co-Authored-By: Claude Opus 4.8 (1M context) --- .../lark_sheets_sheet_dimension_test.go | 22 ++++++++++++++----- 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/shortcuts/sheets/backward/lark_sheets_sheet_dimension_test.go b/shortcuts/sheets/backward/lark_sheets_sheet_dimension_test.go index 15f6f46f4..7e289df5b 100644 --- a/shortcuts/sheets/backward/lark_sheets_sheet_dimension_test.go +++ b/shortcuts/sheets/backward/lark_sheets_sheet_dimension_test.go @@ -13,6 +13,7 @@ import ( "github.com/spf13/cobra" + "github.com/larksuite/cli/errs" "github.com/larksuite/cli/internal/cmdutil" "github.com/larksuite/cli/internal/httpmock" "github.com/larksuite/cli/shortcuts/common" @@ -977,8 +978,15 @@ func TestSheetDeleteDimensionExecuteAPIError(t *testing.T) { "--yes", "--as", "user", }, f, nil) - if err == nil { - t.Fatal("expected API error, got nil") + p, ok := errs.ProblemOf(err) + if !ok { + t.Fatalf("error type = %T, want typed problem", err) + } + if p.Category != errs.CategoryAPI { + t.Fatalf("got category=%q, want %q", p.Category, errs.CategoryAPI) + } + if p.Code != 90001 { + t.Fatalf("got code=%d, want 90001", p.Code) } } @@ -1012,11 +1020,13 @@ func TestSheetDeleteDimensionWithoutYesRequiresConfirmation(t *testing.T) { "--end-index", "7", "--as", "user", }, f, nil) - if err == nil { - t.Fatal("expected confirmation-required error without --yes, got nil") + p, ok := errs.ProblemOf(err) + if !ok { + t.Fatalf("error type = %T, want typed problem (gate must produce a typed confirmation error)", err) } - if !strings.Contains(err.Error(), "requires confirmation") { - t.Fatalf("expected confirmation-required error, got: %v", err) + if p.Category != errs.CategoryConfirmation || p.Subtype != errs.SubtypeConfirmationRequired { + t.Fatalf("got category=%q subtype=%q, want %q/%q", + p.Category, p.Subtype, errs.CategoryConfirmation, errs.SubtypeConfirmationRequired) } }