Require --force to delete a chat line when nobody can confirm it

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}'

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 </dev/null` on a
+// terminal is still a human reading styled output, and the redirection is over
+// by the time they copy the command.
+func deleteLineCmd(cmd *cobra.Command, lineID, chatID, projectID string) string {
+	c := fmt.Sprintf("basecamp chat delete %s --room %s --in %s", lineID, chatID, projectID)
+	if isNonInteractiveCommand(cmd) {
+		c += " --force"
+	}
+	return c
+}
+
 func newChatLineDeleteCmd(project, chatID *string) *cobra.Command {
 	var force bool
 
@@ -1047,20 +1071,24 @@ func newChatLineDeleteCmd(project, chatID *string) *cobra.Command {
 
 This permanently deletes the message — it is not moved to trash.
 
+Deleting asks for confirmation. Where nothing can answer that — any
+machine-output mode (--json, --agent, --quiet), or a redirected stdin —
+--force is required instead, so a permanent delete always carries an
+explicit statement of intent.
+
 You can pass either a line ID or a Basecamp line URL:
   basecamp chat delete 789 --in my-project
+  basecamp chat delete 789 --in my-project --json --force
   basecamp chat delete https://3.basecamp.com/123/buckets/456/chats/789/lines/111`,
 		Args: cobra.ExactArgs(1),
 		RunE: func(cmd *cobra.Command, args []string) error {
 			app := appctx.FromContext(cmd.Context())
 
-			// Refuse before any account, project or chat lookup. This command
-			// confirms interactively unless told otherwise, and
-			// isNonInteractiveCommand only knows about flags, the env var and
-			// stdout — it never looks at stdin. An agent in a PTY with stdin on
-			// /dev/null and no --json lands here, and a prompt reached there
-			// waits on /dev/tty instead of failing. Failing up front costs it
-			// nothing; reaching the prompt spent two round trips first.
+			// A chat line delete is permanent — the API does not trash it — so
+			// it happens only with --force or a confirmation somebody can
+			// answer. Refuse before any account, project or chat lookup: an
+			// invocation that cannot proceed should not spend two round trips
+			// discovering that.
 			if err := ensureDeleteConfirmable(cmd, force); err != nil {
 				return err
 			}
@@ -1113,10 +1141,10 @@ You can pass either a line ID or a Basecamp line URL:
 				return output.ErrUsage("Invalid line ID")
 			}
 
-			// Confirm destructive action in interactive mode. ensureDeleteConfirmable
-			// above already rejected the case where the prompt cannot be answered,
-			// so a failure here is the user canceling.
-			if !force && !isNonInteractiveCommand(cmd) {
+			// Confirm unless forced. ensureDeleteConfirmable established that
+			// without --force a prompt will be shown and can be answered, so
+			// the only outcomes left here are confirm and cancel.
+			if !force {
 				confirmed, err := tui.ConfirmDangerous("Permanently delete this chat line?")
 				switch {
 				case errors.Is(err, tui.ErrCanceled):

internal/commands/chat_test.go

@@ -10,6 +10,7 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"os"
+	"runtime"
 	"strings"
 	"testing"
 
@@ -1511,21 +1512,28 @@ func TestChatDeleteReturnsDeletedPayload(t *testing.T) {
 	assert.Equal(t, "111", data["id"])
 }
 
-// TestChatDeleteSkipsPromptInAgentMode verifies that --agent mode skips the
-// confirmation prompt and issues the DELETE call.
-func TestChatDeleteSkipsPromptInAgentMode(t *testing.T) {
+// TestChatDeleteRequiresForceInAgentMode is the inverse of what this test used
+// to assert. Agent mode skips the confirmation prompt — that part is unchanged
+// and correct — but it used to then delete anyway, so `basecamp chat delete
+// <id> --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 </dev/null`.
+	t.Run("human whose stdin was redirected still keeps the confirmation", func(t *testing.T) {
+		t.Setenv("BASECAMP_NO_KEYRING", "1")
+		terminalStdioForPrompt(t)         // stdout/stderr terminals: styled, human-facing
+		nonInteractiveStdin(t, "devnull") // ...but this invocation cannot be prompted
+
+		app, buf := newChatDeleteTestApp(&mockChatUpdateTransport{})
+
+		deleteCmd := chatLineDeleteBreadcrumb(t, app, buf)
+		assert.NotContains(t, deleteCmd, "--force",
+			"the show command's stdin says nothing about the terminal the reader will paste into")
+
+		// No guard round-trip here, unlike the rows above: the guard answers for
+		// *this* process, which by construction cannot be prompted. The
+		// breadcrumb is a claim about a different invocation.
+	})
+
+	t.Run("human on a terminal keeps the confirmation", func(t *testing.T) {
+		t.Setenv("BASECAMP_NO_KEYRING", "1")
+		terminalStdioForPrompt(t)
+
+		app, buf := newChatDeleteTestApp(&mockChatUpdateTransport{})
+		// No machine-output flag, and a *bytes.Buffer stdout, so a real
+		// invocation here would reach the confirmation prompt.
+
+		deleteCmd := chatLineDeleteBreadcrumb(t, app, buf)
+		assert.NotContains(t, deleteCmd, "--force",
+			"a human who runs this is owed the confirmation; the hint must not spend it for them")
+
+		// And the bare command is genuinely accepted here — not merely missing a
+		// flag it would have needed.
+		require.NoError(t, ensureDeleteConfirmable(deleteGuard(app), false),
+			"the unforced breadcrumb %q must be runnable in the context that produced it", deleteCmd)
+	})
+}
+
+// deleteGuard builds the command the suggested delete would actually run as.
+// It mirrors executeChatCommand's wiring, including SetOut — isMachineOutput
+// inspects cmd.OutOrStdout(), so a guard left on the real os.Stdout would read
+// go test's pipe as a machine consumer and disagree with the command that
+// produced the breadcrumb.
+func deleteGuard(app *appctx.App) *cobra.Command {
+	guard := NewChatCmd()
+	guard.SetContext(appctx.WithApp(context.Background(), app))
+	guard.SetOut(&bytes.Buffer{})
+	guard.SetErr(&bytes.Buffer{})
+	return guard
+}
+
+// terminalStdioForPrompt points stdin and stderr at a pseudo-terminal, the pair
+// stdinarg.InteractivePrompt asks about, so a confirmation could really be shown.
+func terminalStdioForPrompt(t *testing.T) {
+	t.Helper()
+
+	if runtime.GOOS == "windows" {
+		t.Skip("no /dev/ptmx on Windows")
+	}
+	pty, err := os.OpenFile("/dev/ptmx", os.O_RDWR, 0)
+	if err != nil {
+		t.Skipf("open /dev/ptmx: %v", err)
+	}
+	origIn, origErr := os.Stdin, os.Stderr
+	os.Stdin, os.Stderr = pty, pty
+	t.Cleanup(func() {
+		os.Stdin, os.Stderr = origIn, origErr
+		pty.Close()
+	})
+}