Require --force to delete a chat line when nobody can confirm it - #654
Require --force to delete a chat line when nobody can confirm it#654jeremy wants to merge 1 commit into
Conversation
github-actions
Bot
added
commands
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
9d571b9
Compare
48a8a37 to
7da1097
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There 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".
| 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
dcd04d6
Compare
7da1097 to
76efcb1
Compare
dcd04d6 to
dd5b89b
Compare
76efcb1 to
f21770a
Compare
dd5b89b to
9fca545
Compare
f21770a to
85d0751
Compare
There 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".
9fca545 to
c29832f
Compare
85d0751 to
a9231bc
Compare
c29832f to
2005e9c
Compare
a9231bc to
774f60d
Compare
2005e9c to
5d9dffa
Compare
774f60d to
83b04ba
Compare
5d9dffa to
b9b5c80
Compare
83b04ba to
e24ff1c
Compare
There 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".
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.
e24ff1c to
d469772
Compare
|
| Source | Result |
|---|---|
bin/ci locally on this exact commit |
pass — fmt, vet, lint, unit, e2e BATS, naming, CLI surface, skill drift, SDK provenance, go mod tidy |
Test (dispatched via workflow_dispatch) |
✅ success |
| Security (dispatched) | ✅ success |
| CodeQL Advanced (dispatched) | ✅ success |
The dispatched runs are pinned to d469772, not to an ancestor.
Before merging: #652 merges first, at which point GitHub retargets this PR to main, firing a fresh pull_request event that runs the full suite for real. Prefer that over the manual dispatch above — it is the one reflecting the merged-with-main state.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d469772176
ℹ️ 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".
| if isNonInteractiveCommand(cmd) { | ||
| c += " --force" |
There was a problem hiding this comment.
When hints are shown by a human-facing invocation such as BASECAMP_NONINTERACTIVE=1 basecamp chat line 111 --styled --hints, this condition appends --force even though that environment assignment ends with the show command and does not change its styled output format. Copying the breadcrumb into the terminal then bypasses the confirmation that would otherwise be available. The fresh evidence beyond the redirected-stdin finding is that the revised audience check still includes the invocation-scoped BASECAMP_NONINTERACTIVE state; use machine output—not the noninteractive environment—to decide whether the breadcrumb needs --force.
Useful? React with 👍 / 👎.
| // 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) { |
There was a problem hiding this comment.
When the pre-release make smoke suite reaches the Campfire lifecycle, e2e/smoke/smoke_campfire.bats:75-78 still runs basecamp campfire delete ... --json without --force and asserts success. This new guard now deterministically returns a usage error before issuing the DELETE, so the smoke suite fails and leaves its posted test message behind; update that invocation to include --force.
Useful? React with 👍 / 👎.
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 non-interactive runs skipped the prompt and deleted; now chat delete refuses unless --force, or it prompts only when a confirmation will be shown and can be answered.
skills/basecamp/SKILL.mdinclude a--json --forceexample.Migration
Written for commit d469772. Summary will update on new commits.