Uh oh!
There was an error while loading. Please reload this page.
Require --force to delete a chat line when nobody can confirm it - #654
Conversation
There was a problem hiding this comment.
Pull request overview
Requires explicit confirmation or --force before permanently deleting chat lines in non-interactive contexts.
Changes:
- Rejects unsafe deletes before API lookups.
- Adds unit and end-to-end coverage for output and stdin modes.
- Documents the
--forcerequirement for agents.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
skills/basecamp/SKILL.md | Documents forced permanent deletion. |
internal/commands/helpers.go | Implements the confirmation invariant. |
internal/commands/chat.go | Gates deletion before lookups and prompts safely. |
internal/commands/chat_test.go | Tests confirmation and force scenarios. |
e2e/chat.bats | Verifies machine-mode refusal. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
751f1df to
9d571b9Compare48a8a37 to
7da1097CompareThere was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:7da10977c9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Uh oh!
There was an error while loading. Please reload this page.
| willPrompt := !isNonInteractiveCommand(cmd) | ||
| if force || (willPrompt && stdinarg.InteractivePrompt()) { |
There was a problem hiding this comment.
When the saved format is json or quiet but the user explicitly passes --styled or --md in a real terminal, ApplyFlags changes the effective writer to that human-facing format, yet isNonInteractiveCommand still returns true because App.IsMachineOutput consults the stale configured format. This new gate therefore rejects an otherwise confirmable delete and demands --force, despite explicit flags taking precedence over configuration; base the prompt decision on the effective/overridden format.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed, and deliberately not changing it — flagging for human review rather than resolving.
The premise checks out: ApplyFlags swaps the writer to Styled/Markdown for --styled/--md, but App.IsMachineOutput() reads Config.Format, so a saved format: json plus --styled on a real terminal reports machine output and no prompt is shown.
Two reasons to leave it:
The direction of the change is safe. In that corner the old behavior was delete without confirming. The new behavior is a usage error naming
--force. This PR makes that corner stricter, not more dangerous, and the remedy is one documented flag.The fix cannot be local, and a partial one is worse than none.
ensureDeleteConfirmableand the prompt block inchat.gomust agree; if the guard says confirmable andisNonInteractiveCommandthen skips the prompt, the delete proceeds unconfirmed — precisely the bug this PR exists to close. Agreeing means basing both on the effective format, i.e. changingIsMachineOutput(orisMachineOutput), which also movesmissingArgandnoChangesacross many commands. That is the widening the PR description explicitly declines as a separate decision.
So: real, narrow, and safe-by-default today. Worth its own PR that fixes IsMachineOutput to honor explicit format overrides everywhere at once, with the missingArg/noChanges fallout reviewed on its merits. Happy to open that if a maintainer agrees.
9d571b9 to
dcd04d6Compare7da1097 to
76efcb1Comparedcd04d6 to
dd5b89bCompare76efcb1 to
f21770aComparedd5b89b to
9fca545Comparef21770a to
85d0751CompareThere was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:85d0751338
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Uh oh!
There was an error while loading. Please reload this page.
9fca545 to
c29832fCompare85d0751 to
a9231bcComparec29832f to
2005e9cComparea9231bc to
774f60dCompare2005e9c to
5d9dffaCompare774f60d to
83b04baCompare5d9dffa to
b9b5c80Compare83b04ba to
e24ff1cCompareThere was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:e24ff1c459
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Uh oh!
There was an error while loading. Please reload this page.
e24ff1c to
d469772Compare638d1b7 to
9f96500CompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
9f96500 to
49b90a5CompareUh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:49b90a597a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
49b90a5 to
7aa6a9eCompareUh oh!
There was an error while loading. Please reload this page.
BREAKING: `basecamp chat delete <id>` 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 <id> --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.
7aa6a9e to
7ea0be6Comparejeremy
commented
Aug 24, 2026
One policy decision needs a maintainer before mergeEverything else on this PR is resolved. This is the single open thread, deliberately left unresolved rather than fixed, and it wants an explicit accept or reject. The edge: Why it was not fixed here:
The two options:
Related but already fixed: the same audience distinction was applied within this PR's own scope — Also worth recording, since nothing in CI would have caught it: this change broke |
Uh oh!
There was an error while loading. Please reload this page.
The force-flag work merged in #654 at 7ea0be6, before two comment revisions landed on the feature branch. The historical paragraph now speaks of the old gate in the past tense, and the closing note says what actually happened to isNonInteractiveCommand: the delete path bypasses it — machineReadsThisOutput asks the narrower question — and it is left untouched for its other callers, not "read, never widened".
ApplyFlags lets --styled and --md rebuild the writer over a configured json/quiet format, but IsMachineOutput never read those flags — so `basecamp --styled <cmd>` on a format=json machine was human to the renderer and machine to every gate. That split surfaced as the declined review thread on #654: chat delete demanded --force for an invocation whose confirmation a person could see and answer, and chat.go grew humanFormatOverride to state the rule at one call site because the predicate couldn't. The clause sits between the machine-flag block and the config switch, mirroring ApplyFlags' ordering: --json --styled resolves JSON-first there, so machine flags still win here. Deliberately not delegated to EffectiveFormat(): that would also pull in FormatAuto-on-a-pipe, flipping upgrade's stdout, attachment progress, the skill-update nudge, and quickstart/wizard gating for every `basecamp foo | grep`. TestMachinePredicatesAgree pins the whole flag/config matrix against machineReadsThisOutput, asserting that one divergence explicitly instead of skipping it.
The problem
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 <id> --jsondestroyed a message with no statement of intent anywhere in the invocation and nobody in a position to object. Skipping a confirmation is not the same as answering one.TestChatDeleteSkipsPromptInAgentModepinned that behavior. It is inverted here.The invariant
A permanent delete happens only with
--force, or with a confirmation that will be shown and can be answered. Three outcomes, no fourth:--force--force, before any request"Anything else" merges two cases that used to end differently:
isMachineOutputreads flags, the env var and stdout, never stdin — to a caller with nothing to type, where bubbletea waits on/dev/ttyrather than failing (the hang class from Refuse to prompt when nothing can answer the prompt #652).Same missing affirmation, so the same answer.
Placement and scope
The refusal lands before any account, project or chat lookup, so an invocation that cannot proceed does not spend two round trips discovering it. Asserted by a request counter, not by inspection.
isNonInteractiveCommandis read but not widened:missingArgandnoChangesuse it to choose between showing help and returning a structured error, and changing it would move behavior across many commands at once. This PR changes one command.Tests
TestChatDeleteConfirmationMatrixcovers agent / json / quiet / config-driven json /BASECAMP_NONINTERACTIVE/ piped stdin / stdin on/dev/null, plus the forced positives, each asserting zero requests on refusal. Two e2e cases resolve before any request, so no cassette is needed — a delete that reached the wire would itself be the bug.Release mechanics
.surface-breakingentry belongs here. That file receives removed.surfacelines; the snapshot is unchanged at 18438 entries, becausechat deleteand--forceboth still exist. This is a behavioral break, not a surface removal, so that check will never fire on it — worth stating explicitly rather than leaving a reviewer to wonder.breakinglabel is applied and is the sole input to the release notes' Breaking Changes section (.github/release.yml:8). There is no CHANGELOG to edit.bin/cigreen on this branch.Summary by cubic
Require explicit intent for permanent chat deletes. Previously machine-output or undrivable prompts could delete without confirmation; now deletion only proceeds with --force or when a prompt will be shown and can be answered, and we refuse early with a usage hint naming --force.
--styled,--md) overrides a machine-output config, we carry that flag so the suggestion remains runnable and confirmable.ensureDeleteConfirmablerefuses before any account/project/chat lookup whendeleteNeedsForceis true (set byBASECAMP_NONINTERACTIVE, by the rendered audience viaEffectiveFormatfor machine readers, or when stdin/stderr can’t answer a prompt). We do not widenisNonInteractiveCommand.--json, update help text examples, and clarify inskills/basecamp/SKILL.mdthat--forceis required.Migration
--agent,--json,--quiet, configjson|quiet, whenBASECAMP_NONINTERACTIVEis set, or when stdin/stderr are not terminals.Written for commit 7ea0be6. Summary will update on new commits.