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/e2e/smoke/smoke_campfire.bats b/e2e/smoke/smoke_campfire.bats index db0a24a5..522d1c86 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 af24e0f3..b6cd655f 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 663772a5..a7bdef17 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 2600974e..b739499e 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 97b9055d..d4140796 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)