diff --git a/e2e/chat.bats b/e2e/chat.bats index 9d435ca2..686a4ab0 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 af24e0f3..f1312239 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 59b7b309..f039ac83 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)