From d469772176cf619f45ffe7413f36af734ed99542 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sat, 22 Aug 2026 02:19:00 -0700 Subject: [PATCH] Require --force to delete a chat line when nobody can confirm it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BREAKING: `basecamp chat delete ` now fails without --force in machine-output modes (--agent, --json, --quiet, config-driven json/quiet, BASECAMP_NONINTERACTIVE) and wherever stdin cannot answer a prompt. Add --force to restore the previous behavior. The flag is unchanged and was already the documented form in SKILL.md. A chat line delete is permanent — the API does not trash it. It used to happen unconfirmed in exactly the modes where nobody could object. Machine-output mode skips the confirmation prompt, which is right, but it then deleted anyway: `basecamp chat delete --json` destroyed a message with no statement of intent anywhere in the invocation. Skipping a confirmation is not the same as answering one. So the invariant is now one line — a permanent delete happens only with --force, or with a confirmation that will be shown and can be answered — and it leaves three outcomes and no fourth: --force proceed, intent stated a prompt that will be shown and can be answered ask, and honor the answer anything else refuse, naming --force "Anything else" merges two cases that used to end differently: machine mode, which skipped the prompt and deleted; and a terminal with redirected stdin, which showed the prompt to a caller with nothing to type — because isNonInteractiveCommand reads flags, the env var and stdout, never stdin — where bubbletea waits on /dev/tty rather than failing. Same missing affirmation either way. The refusal lands before any account, project or chat lookup, so an invocation that cannot proceed does not spend two round trips discovering it. isNonInteractiveCommand is read but not widened: missingArg and noChanges use it to choose between help and a structured error, and moving it would change many commands at once. TestChatDeleteSkipsPromptInAgentMode pinned the old behavior; it is inverted, and a matrix covers every mode plus the forced positives. No .surface-breaking entry: the snapshot is unchanged at 18438 entries — `chat delete` and --force both still exist, and this is a behavioral break, not a surface removal, which is the only thing that file tracks. This PR needs the `breaking` label; it is the sole input to the release notes' Breaking Changes section, and there is no CHANGELOG to edit. --- e2e/chat.bats | 23 ++++ internal/commands/chat.go | 54 ++++++-- internal/commands/chat_test.go | 231 ++++++++++++++++++++++++++++++--- internal/commands/helpers.go | 62 ++++++--- skills/basecamp/SKILL.md | 2 +- 5 files changed, 322 insertions(+), 50 deletions(-) diff --git a/e2e/chat.bats b/e2e/chat.bats index 9d435ca28..686a4ab0e 100644 --- a/e2e/chat.bats +++ b/e2e/chat.bats @@ -95,6 +95,29 @@ load test_helper assert_output_contains "ID required" } +# A chat line delete is permanent — the API does not trash it. In machine-output +# mode there is no confirmation prompt and nobody to answer one, so the intent +# has to be stated with --force. This resolves before any request, so no +# cassette is needed; a delete that reached the wire would be the bug. +@test "chat delete in json mode requires --force" { + create_credentials + create_global_config '{"account_id": 99999, "project_id": 123}' + + run basecamp chat delete 111 --json + assert_failure + assert_json_value '.code' 'usage' + assert_json_value '.hint | contains("--force")' 'true' +} + +@test "chat delete in agent mode requires --force" { + create_credentials + create_global_config '{"account_id": 99999, "project_id": 123}' + + run basecamp chat delete 111 --agent + assert_failure + assert_output_contains "--force" +} + @test "chat update without args shows error" { create_credentials create_global_config '{"account_id": 99999, "project_id": 123}' diff --git a/internal/commands/chat.go b/internal/commands/chat.go index af24e0f3a..f1312239a 100644 --- a/internal/commands/chat.go +++ b/internal/commands/chat.go @@ -757,8 +757,8 @@ You can pass either a line ID or a Basecamp line URL: output.WithBreadcrumbs( output.Breadcrumb{ Action: "delete", - Cmd: fmt.Sprintf("basecamp chat delete %s --room %s --in %s", lineID, effectiveChatID, resolvedProjectID), - Description: "Delete line", + Cmd: deleteLineCmd(cmd, lineID, effectiveChatID, resolvedProjectID), + Description: "Delete line (permanent)", }, output.Breadcrumb{ Action: "messages", @@ -1037,6 +1037,30 @@ edit to rich text.`, return cmd } +// deleteLineCmd builds the delete breadcrumb for whoever will read it. +// Breadcrumbs are not machine-only — they render in styled and Markdown output +// too — so the flag depends on the audience: +// +// - Machine output: no confirmation is shown in that mode, so the bare +// command could only fail. Emit --force. +// - Human-facing output: they will be asked to confirm. Emit the bare command, +// because handing over a pre-forced one quietly spends the affirmation this +// change exists to require. +// +// The audience is isNonInteractiveCommand — flags, env and stdout. Deliberately +// NOT deleteNeedsForce, which also asks whether *this* process could be prompted. +// That is the wrong question: the breadcrumb describes a future invocation, and +// this one's stdin says nothing about it. `chat line 111 --agent` permanently destroyed a message with no statement of intent +// anywhere in the invocation and nobody in a position to object. Skipping the +// prompt is not the same as answering it. +func TestChatDeleteRequiresForceInAgentMode(t *testing.T) { t.Setenv("BASECAMP_NO_KEYRING", "1") - transport := &mockChatDeleteTransport{} + transport := &countingChatTransport{inner: &mockChatDeleteTransport{}} app, _ := newChatDeleteTestApp(transport) app.Flags.Agent = true // machine output — no prompt cmd := NewChatCmd() err := executeChatCommand(cmd, app, "delete", "111") - require.NoError(t, err) + require.Error(t, err) - assert.Equal(t, "DELETE", transport.capturedMethod) - assert.Contains(t, transport.capturedPath, "/lines/") + outErr := output.AsError(err) + require.NotNil(t, outErr) + assert.Equal(t, output.CodeUsage, outErr.Code) + assert.Contains(t, outErr.Hint, "--force") + assert.Zero(t, transport.requests, "nothing should have been requested, let alone deleted") } // TestChatDeleteForceSkipsPrompt verifies that --force bypasses the confirmation @@ -1812,31 +1820,86 @@ func TestChatPostRejectsPositionalWithContentFlag(t *testing.T) { require.Contains(t, err.Error(), "cannot combine") } -// TestChatDeleteRefusesWhenStdinCannotConfirm covers the gap isMachineOutput -// cannot see: it checks flags, the env var and stdout, never stdin. An agent in -// a PTY with stdin on /dev/null and no --json reaches the confirmation prompt, -// which used to block on /dev/tty. It must now fail with a usage error naming -// --force, before it issues a single request. -func TestChatDeleteRefusesWhenStdinCannotConfirm(t *testing.T) { - for _, kind := range []string{"pipe", "devnull"} { - t.Run(kind, func(t *testing.T) { +// TestChatDeleteConfirmationMatrix pins the whole invariant: a permanent delete +// happens only with --force, or with a confirmation that will be shown and can +// be answered. Every other shape refuses, names --force, and issues nothing. +// +// The rows split into two failures that used to end differently. Machine-output +// mode skipped the prompt and deleted regardless. A terminal with redirected +// stdin did show the prompt — isMachineOutput reads flags, the env var and +// stdout, never stdin — to a caller with nothing to type, where bubbletea waits +// on /dev/tty rather than failing. Same missing affirmation, same answer now. +func TestChatDeleteConfirmationMatrix(t *testing.T) { + for _, tc := range []struct { + name string + apply func(t *testing.T, app *appctx.App) + args []string + deletes bool + }{ + { + name: "agent mode", + apply: func(_ *testing.T, app *appctx.App) { app.Flags.Agent = true }, + }, + { + name: "json mode", + apply: func(_ *testing.T, app *appctx.App) { app.Flags.JSON = true }, + }, + { + name: "quiet mode", + apply: func(_ *testing.T, app *appctx.App) { app.Flags.Quiet = true }, + }, + { + name: "config-driven json", + apply: func(_ *testing.T, app *appctx.App) { app.Config.Format = "json" }, + }, + { + name: "noninteractive env", + apply: func(t *testing.T, _ *appctx.App) { + t.Setenv("BASECAMP_NONINTERACTIVE", "1") + }, + }, + { + name: "piped stdin", + apply: func(t *testing.T, _ *appctx.App) { nonInteractiveStdin(t, "pipe") }, + }, + { + name: "stdin on /dev/null", + apply: func(t *testing.T, _ *appctx.App) { nonInteractiveStdin(t, "devnull") }, + }, + { + name: "forced in agent mode", + apply: func(_ *testing.T, app *appctx.App) { app.Flags.Agent = true }, + args: []string{"--force"}, + deletes: true, + }, + { + name: "forced with stdin on /dev/null", + apply: func(t *testing.T, _ *appctx.App) { nonInteractiveStdin(t, "devnull") }, + args: []string{"--force"}, + deletes: true, + }, + } { + t.Run(tc.name, func(t *testing.T) { t.Setenv("BASECAMP_NO_KEYRING", "1") - nonInteractiveStdin(t, kind) transport := &countingChatTransport{inner: &mockChatDeleteTransport{}} app, _ := newChatDeleteTestApp(transport) - // No machine-output flag and a *bytes.Buffer stdout, so - // isNonInteractiveCommand is false and the confirm is reached. + tc.apply(t, app) cmd := NewChatCmd() - err := executeChatCommand(cmd, app, "delete", "111") - require.Error(t, err, "delete must not silently succeed on a confirmation nobody can answer") + err := executeChatCommand(cmd, app, append([]string{"delete", "111"}, tc.args...)...) + if tc.deletes { + require.NoError(t, err) + assert.Equal(t, "DELETE", transport.inner.(*mockChatDeleteTransport).capturedMethod) + return + } + + require.Error(t, err, "a delete nobody can confirm must not succeed") outErr := output.AsError(err) require.NotNil(t, outErr) assert.Equal(t, output.CodeUsage, outErr.Code) assert.Contains(t, outErr.Hint, "--force") - assert.Zero(t, transport.requests, "the refusal belongs before the account and project lookups, not after them") }) @@ -1853,3 +1916,129 @@ func (t *countingChatTransport) RoundTrip(req *http.Request) (*http.Response, er t.requests++ return t.inner.RoundTrip(req) } + +// chatLineDeleteBreadcrumb runs `chat line` and returns its delete breadcrumb. +func chatLineDeleteBreadcrumb(t *testing.T, app *appctx.App, buf *bytes.Buffer) string { + t.Helper() + + app.Flags.Hints = true // breadcrumbs are stripped from the envelope without this + + cmd := NewChatCmd() + require.NoError(t, executeChatCommand(cmd, app, "line", "111")) + + var envelope struct { + Breadcrumbs []struct { + Action string `json:"action"` + Cmd string `json:"cmd"` + } `json:"breadcrumbs"` + } + require.NoError(t, json.Unmarshal(buf.Bytes(), &envelope)) + + for _, b := range envelope.Breadcrumbs { + if b.Action == "delete" { + return b.Cmd + } + } + t.Fatal("expected a delete breadcrumb") + return "" +} + +// TestChatLineDeleteBreadcrumbMatchesItsReader pins both directions of the +// breadcrumb, because breadcrumbs are not machine-only — they render in styled +// and Markdown output too, so the same string is read by people. +// +// - A machine consumer sees no confirmation prompt, so a breadcrumb without +// --force is a command that can only fail. A suggested next step that always +// errors is worse than no suggestion. +// - A human on a terminal will be asked to confirm. Handing them a pre-forced +// command spends that affirmation silently — the exact protection this +// change exists to add, removed by the hint meant to help them. +// +// Neither assertion string-matches for its own sake: each feeds the emitted +// command back through the guard that governs the real delete, so the +// suggestion and the gate cannot drift apart. +func TestChatLineDeleteBreadcrumbMatchesItsReader(t *testing.T) { + t.Run("machine consumer gets a runnable command", func(t *testing.T) { + t.Setenv("BASECAMP_NO_KEYRING", "1") + + app, buf := newChatDeleteTestApp(&mockChatUpdateTransport{}) + app.Flags.JSON = true + + deleteCmd := chatLineDeleteBreadcrumb(t, app, buf) + assert.Contains(t, deleteCmd, "--force") + + require.NoError(t, ensureDeleteConfirmable(deleteGuard(app), strings.Contains(deleteCmd, "--force")), + "the breadcrumb %q is rejected by the very guard it would hit", deleteCmd) + }) + + // The show command's own stdin is irrelevant: the redirection ends with it, + // and the reader pastes into whatever terminal they are sitting at. Judging + // promptability from this process would hand a human a pre-forced command + // purely because they ran `chat line 111 --json` destroyed a message with no +// statement of intent anywhere in the invocation. And a terminal with stdin +// redirected does not skip the prompt: isNonInteractiveCommand reads flags, the +// env var and stdout, never stdin, so the confirmation is shown to an agent +// that has nothing to type with, and bubbletea answers a non-terminal stdin by +// opening /dev/tty and waiting on the real terminal. +// +// Both are the same failure — a destructive act with nobody affirming it — so +// both get the same answer. Requiring the flag precisely where no human can +// confirm is the point; it is not an obstacle to a caller who means it, since +// --force is one word and already the documented form. +// +// It asks InteractivePrompt rather than InteractiveStdio because the +// confirmation is a huh form, which draws to stderr. +// +// isNonInteractiveCommand is read, never widened: its other callers (missingArg, +// noChanges) use it to choose between help and a structured error, and changing +// it would move behavior across many commands. func ensureDeleteConfirmable(cmd *cobra.Command, force bool) error { - if force || isNonInteractiveCommand(cmd) || stdinarg.InteractivePrompt() { + if force || !deleteNeedsForce(cmd) { return nil } // Name the requirement, not one end of it: this also fires when stdin is a // terminal but stderr is redirected, where the confirmation would be drawn // somewhere nobody can see it. return output.ErrUsageHint( - "This deletion needs a confirmation that can't be shown or answered here", - "Confirming needs a terminal on both stdin and stderr. Pass --force to delete without confirming.") + "Permanent deletion needs --force here", + "Nothing can confirm this delete: it is not trashable and cannot be undone. "+ + "Machine-output modes show no prompt at all, and confirming interactively needs a "+ + "terminal on both stdin and stderr. Pass --force to state the intent explicitly.") } // isNonInteractiveCommand returns true when command-level flows should avoid diff --git a/skills/basecamp/SKILL.md b/skills/basecamp/SKILL.md index 59b7b3094..f039ac835 100644 --- a/skills/basecamp/SKILL.md +++ b/skills/basecamp/SKILL.md @@ -1157,7 +1157,7 @@ basecamp chat post "Hello!" --in basecamp chat post "@Jane.Smith, check this" --in # With @mention (auto text/html) basecamp chat line --in # Show line basecamp chat update "edited content" --in # Edit existing message in place -basecamp chat delete --in --force # Delete line (permanent, not trashable) +basecamp chat delete --in --force # Delete line (permanent, not trashable; --force required) ``` ### Pings (Direct Messages)