From 7ea0be6612d880a668b8bfc353bbd35d6bc4a377 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 ++ e2e/smoke/smoke_campfire.bats | 6 +- internal/commands/chat.go | 98 ++++++-- internal/commands/chat_test.go | 401 +++++++++++++++++++++++++++++++-- internal/commands/helpers.go | 91 ++++++-- skills/basecamp/SKILL.md | 2 +- 6 files changed, 570 insertions(+), 51 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/e2e/smoke/smoke_campfire.bats b/e2e/smoke/smoke_campfire.bats index db0a24a58..522d1c863 100644 --- a/e2e/smoke/smoke_campfire.bats +++ b/e2e/smoke/smoke_campfire.bats @@ -72,8 +72,12 @@ setup_file() { local line_id line_id=$(<"$id_file") + # --force is required in machine-output mode: the delete is permanent (the API + # does not trash it) and --json shows no confirmation prompt, so the intent has + # to be stated. This suite means it — without the flag the delete is refused + # and the posted test message is left behind. run_smoke basecamp campfire delete "$line_id" \ - --room "$QA_CAMPFIRE" -p "$QA_PROJECT" --json + --room "$QA_CAMPFIRE" -p "$QA_PROJECT" --json --force assert_success assert_json_value '.ok' 'true' } diff --git a/internal/commands/chat.go b/internal/commands/chat.go index af24e0f3a..b6cd655f6 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", @@ -778,6 +778,71 @@ You can pass either a line ID or a Basecamp line URL: 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 the *rendered format* of this envelope — the only property of +// the current invocation that survives into the one the reader will type. Three +// tempting predicates are all wrong here, each for the same reason: +// +// - stdin: `chat line 111 guide.md` — a document written for a person — is +// classified as machine output. +// +// EffectiveFormat resolves the auto case the same way the renderer does, so a +// plain redirect (auto → JSON) is machine, while an explicitly human format +// stays human wherever it is written. +// +// When the guess is wrong the reader gets a usage error naming --force, which is +// recoverable. The reverse mistake is a permanent delete with no confirmation, +// which is not — so this errs toward omitting the flag. +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 machineReadsThisOutput(cmd) { + return c + " --force" + } + // Human-facing, so no --force. But if this invocation is only human-facing + // because a flag overrode a machine-output config, that flag has to travel + // with the suggestion: pasting it starts a fresh process which re-reads the + // config, renders json/quiet again, and refuses the very command we just + // handed over for lacking --force. Carrying the flag keeps it both runnable + // and confirmable, which adding --force would not. + return c + humanFormatOverride(appctx.FromContext(cmd.Context())) +} + +// humanFormatOverride returns the flag that is holding this invocation in a +// human format against a configured machine one, or "" when the configuration +// is not fighting it. +func humanFormatOverride(app *appctx.App) string { + if app == nil || app.Config == nil { + return "" + } + switch app.Config.Format { + case "json", "quiet": // a fresh process would render machine output + default: + return "" + } + switch { + case app.Flags.Styled: + return " --styled" + case app.Flags.MD: + return " --md" + default: + return "" + } +} + func newChatLineUpdateCmd(project, chatID, contentType *string) *cobra.Command { var content string @@ -1047,20 +1112,26 @@ 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, --force is +required instead, so a permanent delete always carries an explicit statement +of intent. That covers any machine-output mode (--json, --agent, --quiet), +BASECAMP_NONINTERACTIVE, and a confirmation that could not be shown or +answered — which needs a terminal on both stdin and stderr, since the prompt +is drawn to stderr. + 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 +1184,11 @@ 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 — one + // predicate for both, so the guard cannot accept an invocation that + // this block then skips, which would delete unconfirmed. + if !force { confirmed, err := tui.ConfirmDangerous("Permanently delete this chat line?") switch { case errors.Is(err, tui.ErrCanceled): diff --git a/internal/commands/chat_test.go b/internal/commands/chat_test.go index 663772a5c..a7bdef17a 100644 --- a/internal/commands/chat_test.go +++ b/internal/commands/chat_test.go @@ -10,8 +10,10 @@ import ( "net/http" "net/http/httptest" "os" + "runtime" "strings" "testing" + "time" "github.com/spf13/cobra" "github.com/stretchr/testify/assert" @@ -1511,21 +1513,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 +// --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 +1821,115 @@ 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. +// +// Each row starts from an invocation that would genuinely prompt — styled +// output, and stdin/stderr on a pty — and then removes exactly one thing. That +// setup is the whole point of the test: with the fixture's default JSON output, +// or with go test's non-terminal stdio, every row refuses through a condition it +// was not testing, and the table passes with any single guard deleted. +// +// The format field is what production would actually render for that input, so +// the flag and config rows exercise the path from their input to the audience +// decision rather than asserting it in the fixture. +func TestChatDeleteConfirmationMatrix(t *testing.T) { + for _, tc := range []struct { + name string + format output.Format // what this invocation renders; Styled = a person reads it + apply func(t *testing.T, app *appctx.App) + args []string + deletes bool + }{ + { + name: "agent mode renders quiet, which is parsed", + format: output.FormatQuiet, + apply: func(_ *testing.T, app *appctx.App) { app.Flags.Agent = true }, + }, + { + name: "json mode is parsed", + format: output.FormatJSON, + apply: func(_ *testing.T, app *appctx.App) { app.Flags.JSON = true }, + }, + { + name: "quiet mode is parsed", + format: output.FormatQuiet, + apply: func(_ *testing.T, app *appctx.App) { app.Flags.Quiet = true }, + }, + { + name: "config-driven json is parsed", + format: output.FormatJSON, + apply: func(_ *testing.T, app *appctx.App) { app.Config.Format = "json" }, + }, + { + // Styled output: only the env var disqualifies this one, so + // deleting the NonInteractiveEnv clause must fail exactly here. + name: "noninteractive env, despite human output", + format: output.FormatStyled, + apply: func(t *testing.T, _ *appctx.App) { + t.Setenv("BASECAMP_NONINTERACTIVE", "1") + }, + }, + { + // Styled output and no env var: only stdin disqualifies these. + name: "piped stdin, despite human output", + format: output.FormatStyled, + apply: func(t *testing.T, _ *appctx.App) { nonInteractiveStdin(t, "pipe") }, + }, + { + name: "stdin on /dev/null, despite human output", + format: output.FormatStyled, + apply: func(t *testing.T, _ *appctx.App) { nonInteractiveStdin(t, "devnull") }, + }, + { + name: "forced in agent mode", + format: output.FormatQuiet, + apply: func(_ *testing.T, app *appctx.App) { app.Flags.Agent = true }, + args: []string{"--force"}, + deletes: true, + }, + { + name: "forced with stdin on /dev/null", + format: output.FormatStyled, + 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) + t.Setenv("BASECAMP_NONINTERACTIVE", "") // rows opt in; no leakage between them + promptableStdio(t) transport := &countingChatTransport{inner: &mockChatDeleteTransport{}} + buf := &bytes.Buffer{} app, _ := newChatDeleteTestApp(transport) - // No machine-output flag and a *bytes.Buffer stdout, so - // isNonInteractiveCommand is false and the confirm is reached. + // Override the fixture's JSON writer: it would otherwise refuse + // every row through the audience clause, whatever the row is about. + app.Output = output.New(output.Options{Format: tc.format, Writer: buf}) + 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") + // Bounded, because promptableStdio hands the process a real pty: + // if a regression lets one of these rows reach the confirmation, + // huh waits for a keystroke that never comes and the suite hangs + // instead of failing. A hung CI job is a much worse signal than a + // red one — it looks like infrastructure, not like this test. + err := executeChatWithin(t, cmd, app, 10*time.Second, + 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 +1946,269 @@ 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 +// out of the rendered envelope. JSON only — the point is to prove the breadcrumb +// really reaches the wire, and the other formats do not emit parseable output. +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 which invocations get a +// pre-forced delete suggestion. Breadcrumbs are not machine-only — they render +// in styled and Markdown output too — and the string outlives the invocation +// that produced it, so the only sound basis is what this output *is*, not how +// this process happens to be wired. +// +// Getting it wrong in one direction hands a machine a command that can only +// fail. In the other it hands a person a command that permanently deletes +// without the confirmation they were owed. Three predicates were tried and +// discarded before this one; each row below is a shape that defeated an earlier +// attempt, so they are regression cases, not enumeration for its own sake. +func TestChatLineDeleteBreadcrumbMatchesItsReader(t *testing.T) { + for _, tc := range []struct { + name string + format output.Format + configFmt string + flags func(app *appctx.App) + wire func(t *testing.T) + wantForce bool + wantFlag string // an override the suggestion must carry forward + }{ + {name: "json is parsed by a program", format: output.FormatJSON, wantForce: true}, + {name: "quiet is parsed", format: output.FormatQuiet, wantForce: true}, + {name: "ids-only is parsed", format: output.FormatIDs, wantForce: true}, + { + name: "auto off a tty resolves to json", + format: output.FormatAuto, + wantForce: true, + }, + {name: "styled is read by a person", format: output.FormatStyled, wantForce: false}, + { + // The flag is what makes this human; a fresh process re-reads the + // config, renders json again, and would refuse the bare command. + name: "styled overriding a json config carries the flag", + format: output.FormatStyled, + configFmt: "json", + flags: func(app *appctx.App) { app.Flags.Styled = true }, + wantForce: false, + wantFlag: "--styled", + }, + { + name: "md overriding a quiet config carries the flag", + format: output.FormatMarkdown, + configFmt: "quiet", + flags: func(app *appctx.App) { app.Flags.MD = true }, + wantForce: false, + wantFlag: "--md", + }, + { + // No configured machine format to fight, so nothing to carry. + name: "styled with no configured format stays bare", + format: output.FormatStyled, + flags: func(app *appctx.App) { app.Flags.Styled = true }, + wantForce: false, + }, + { + name: "markdown redirected to a file is still read by a person", + format: output.FormatMarkdown, + wantForce: false, + }, + { + // The redirection ends with the show command; the reader pastes + // into whatever terminal they are sitting at. + name: "styled with this invocation's stdin redirected", + format: output.FormatStyled, + wire: func(t *testing.T) { nonInteractiveStdin(t, "devnull") }, + }, + { + // A one-shot env assignment likewise ends with the command, and + // does not change what the output looks like. + name: "styled under BASECAMP_NONINTERACTIVE", + format: output.FormatStyled, + wire: func(t *testing.T) { t.Setenv("BASECAMP_NONINTERACTIVE", "1") }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + t.Setenv("BASECAMP_NO_KEYRING", "1") + if tc.wire != nil { + tc.wire(t) + } + + buf := &bytes.Buffer{} + app, _ := newChatDeleteTestApp(&mockChatUpdateTransport{}) + // A *bytes.Buffer writer is not a TTY — the shape a redirect or a + // pipe produces, which is exactly what misled the earlier predicate. + app.Output = output.New(output.Options{Format: tc.format, Writer: buf}) + app.Config.Format = tc.configFmt + if tc.flags != nil { + tc.flags(app) + } + + cmd := NewChatCmd() + cmd.SetContext(appctx.WithApp(context.Background(), app)) + cmd.SetOut(&bytes.Buffer{}) + cmd.SetErr(&bytes.Buffer{}) + + got := deleteLineCmd(cmd, "111", "789", "123") + + if tc.wantFlag != "" { + assert.Contains(t, got, tc.wantFlag, + "%s: the override must travel with the suggestion, or a fresh process "+ + "re-reads the config and refuses this command", tc.name) + } else { + assert.NotContains(t, got, "--styled") + assert.NotContains(t, got, "--md") + } + + if tc.wantForce { + assert.Contains(t, got, "--force", + "%s: no confirmation is shown, so the bare command could only fail", tc.name) + } else { + assert.NotContains(t, got, "--force", + "%s: the reader will be asked to confirm; the hint must not spend that for them", tc.name) + } + }) + } +} + +// TestChatLineDeleteBreadcrumbReachesTheEnvelope proves the suggestion actually +// ships, and that a machine consumer's copy survives the guard it would hit. +// The table above tests the decision; this tests that the decision is wired in. +func TestChatLineDeleteBreadcrumbReachesTheEnvelope(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) +} + +// executeChatWithin runs a chat command and fails if it has not returned within +// the timeout, converting a blocked prompt into a legible failure. +func executeChatWithin(t *testing.T, cmd *cobra.Command, app *appctx.App, timeout time.Duration, args ...string) error { + t.Helper() + + done := make(chan error, 1) + go func() { done <- executeChatCommand(cmd, app, args...) }() + + select { + case err := <-done: + return err + case <-time.After(timeout): + t.Fatalf("chat %v blocked instead of returning; it reached a prompt nothing will answer", args) + return nil + } +} + +// promptableStdio points stdin and stderr at a pseudo-terminal — the pair +// stdinarg.InteractivePrompt asks about — so a confirmation could really be +// shown and answered. Tests that want to prove some *other* condition refuses +// start here, then remove only that condition. +func promptableStdio(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() + }) +} + +// 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 +} + +// TestDeleteConfirmableFollowsTheAudienceNotTheDevice pins the guard directly, +// without running a delete — these shapes would reach a real prompt on a pty and +// block, which is the point: they are the cases where a confirmation *can* be +// shown, so demanding --force there contradicts the invariant. +// +// The distinction is between "stdout is redirected" and "a program is reading +// this". `chat delete … --md >report.md` with terminal stdin and stderr writes a +// document to a file while asking a person, on their terminal, whether to go +// ahead. That is confirmable, and was being refused. +func TestDeleteConfirmableFollowsTheAudienceNotTheDevice(t *testing.T) { + for _, tc := range []struct { + name string + format output.Format + env string + wantsForce bool + }{ + {name: "markdown to a file is read by a person", format: output.FormatMarkdown}, + {name: "styled to a pager is read by a person", format: output.FormatStyled}, + {name: "json is parsed, so no prompt is shown", format: output.FormatJSON, wantsForce: true}, + {name: "quiet is parsed", format: output.FormatQuiet, wantsForce: true}, + {name: "ids-only is parsed", format: output.FormatIDs, wantsForce: true}, + { + name: "the escape hatch still wins over a human format", + format: output.FormatStyled, + env: "1", + wantsForce: true, + }, + } { + t.Run(tc.name, func(t *testing.T) { + t.Setenv("BASECAMP_NO_KEYRING", "1") + t.Setenv("BASECAMP_NONINTERACTIVE", tc.env) + promptableStdio(t) // a confirmation could genuinely be shown and answered + + buf := &bytes.Buffer{} + app, _ := newChatDeleteTestApp(&mockChatDeleteTransport{}) + // Not a TTY — the shape a redirect or pipe produces, and the thing + // the old predicate mistook for "a machine is reading this". + app.Output = output.New(output.Options{Format: tc.format, Writer: buf}) + + err := ensureDeleteConfirmable(deleteGuard(app), false) + + if tc.wantsForce { + require.Error(t, err, "%s: nothing will ask, so --force must be required", tc.name) + assert.Contains(t, output.AsError(err).Hint, "--force") + } else { + require.NoError(t, err, + "%s: the confirmation can be shown on stderr and answered on stdin", tc.name) + } + }) + } +} diff --git a/internal/commands/helpers.go b/internal/commands/helpers.go index 2600974e7..b739499ed 100644 --- a/internal/commands/helpers.go +++ b/internal/commands/helpers.go @@ -56,29 +56,90 @@ func noChanges(cmd *cobra.Command) error { return cmd.Help() } -// ensureDeleteConfirmable rejects an invocation that will reach a confirmation -// prompt it cannot drive. isNonInteractiveCommand decides whether to prompt at -// all, but it reads flags, the env var and stdout — never stdin. The gap is a -// PTY with stdin redirected, which is how an agent says "I have nothing to -// type": the prompt is not skipped, and bubbletea answers a non-terminal stdin -// by opening /dev/tty and waiting on the real terminal. It asks -// InteractivePrompt rather than InteractiveStdio because the confirmation is a -// huh form, which draws to stderr. +// machineReadsThisOutput reports whether the envelope being written will be +// parsed by a program rather than read by a person. +func machineReadsThisOutput(cmd *cobra.Command) bool { + app := appctx.FromContext(cmd.Context()) + if app == nil || app.Output == nil { + return isMachineOutput(cmd) + } + switch app.Output.EffectiveFormat() { + case output.FormatStyled, output.FormatMarkdown: + // Both are written to be read: styled for a terminal, Markdown for a + // file or a pager like glow. Redirecting either does not change that. + return false + default: + // JSON, quiet, ids, count — parsed, not read. + return true + } +} + +// deleteNeedsForce reports whether a delete in this invocation's context would +// have to carry --force: either no confirmation will be shown (machine output), +// or one would be shown to stdio that cannot answer it. +// +// The three clauses are the three distinct ways a confirmation fails to happen, +// and each needs its own question: +// +// - BASECAMP_NONINTERACTIVE: the operator said never prompt me. +// - machineReadsThisOutput: a program is parsing this, so no prompt is shown. +// Deliberately NOT isNonInteractiveCommand, whose isMachineOutput treats any +// redirected stdout as machine — that would demand --force for +// `chat delete … --md >report.md` even though the confirmation draws to a +// terminal stderr and a person can answer it. +// - InteractivePrompt: a prompt would be shown but nothing could answer it. +// +// This is about the invocation running now. A breadcrumb suggesting a *future* +// delete asks a narrower question — see deleteLineCmd, which uses the audience +// clause alone, because the current process's stdin and env say nothing about +// the terminal the reader will paste into. +func deleteNeedsForce(cmd *cobra.Command) bool { + return config.NonInteractiveEnv() || machineReadsThisOutput(cmd) || !stdinarg.InteractivePrompt() +} + +// ensureDeleteConfirmable gates a permanent, non-trashable delete on one +// invariant: it happens only when the caller said --force, or when a +// confirmation will actually be shown and can actually be answered. +// +// That leaves exactly three outcomes, and no fourth: +// +// - --force: proceed. The caller stated the intent explicitly. +// - a confirmation that will be shown and can be answered: prompt, and let +// the answer decide. +// - anything else: refuse, naming --force, before any lookup or request. +// +// The "anything else" is two distinct cases that used to end differently. +// Machine-output mode skips the prompt entirely, so the delete went through +// unconfirmed — `basecamp chat delete --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. // -// Deliberately narrow: this does not change when a command prompts, only what -// happens when the prompt it already decided to show cannot be answered. -// Widening isNonInteractiveCommand itself would change missingArg and noChanges -// across many commands, which is a separate decision. +// 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 97b9055d4..d41407960 100644 --- a/skills/basecamp/SKILL.md +++ b/skills/basecamp/SKILL.md @@ -1162,7 +1162,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)