Repository navigation
Delete Campfire lines through their Campfire, and explain Ping refusals - #970
Merged
Merged
Conversation
bc3#14171 makes the generic recording routes answer 403 to any write on a recording in a Ping. The TUI trashed whatever its Detail view, the :trash action and the Hey view held through the generic trash route, so a Campfire line opened from search went there too: in a Ping that is now a 403, and in a project Campfire it moved the line to the trash past the checks the line endpoint makes on who may delete a line and until when. Those three now go through Hub.RemoveRecording, which decides by the API's own recording type: a Chat::Lines::* recording is deleted through Campfires().DeleteLine, anything else is trashed as before. Detail passes the type and parent it loaded; :trash has only the scope's label, so the hub reads the recording first, and writes nothing when it can't tell. The confirmation and status say delete for a line, since that can't be undone. The CLI commands that send a write to one of the gated routes (recordings trash, archive, restore and visibility, the typed nouns' trash, archive and restore, subscriptions add and remove, spotlight and unspotlight) explain the refusal: bc3's 403 is empty, so on that status the recording is read back once, and when it is in a Ping the error says so and names the command that works, chat delete for a line or subscriptions subscribe/unsubscribe for the Ping's Campfire.
…eader The palette's :trash deleted a Campfire line outright, with none of the confirmation t gives in the line's view, though deleting a line can't be undone. It now reads the recording, and for a line says to press t twice there instead, writing nothing; anything else it trashes as before. The Ping-line refusal named chat delete without --force, which chat delete refuses wherever no confirmation can be shown, so an agent reading JSON could not run it. It now builds the command with deleteLineCmd, which adds --force for machine output and leaves it off for a person who will be asked.
Detail read a recording's type with the rest of its fields, so one field it couldn't parse, such as a timestamp, left the type unknown: t then asked to trash, and RemoveRecording, reading the type itself, deleted the line for good. Detail now reads the type and parent on their own, and RemoveRecording takes only a type the caller has, writing nothing without one, since the caller's confirmation can't have said which would happen. Its read is left for the one thing a caller may lack, a line's Campfire.
github-actions
Bot
added
commands
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: a92a24f921
ℹ️ 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".
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Recovery hints omit the originating profile and account, so copied commands can run under different credentials or account scope.
1 open issue
What changed in this PR
This PR routes Campfire line deletion through the Campfire API in the dev-only TUI and adds actionable CLI errors for writes that Pings refuse.
Changes:
- Distinguishes permanent line deletion from recoverable trash, with appropriate confirmation and palette safeguards.
- Explains Ping write refusals and suggests supported commands.
- Adds routing and error-handling tests, plus Ping restrictions in the skill documentation.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
| File | Description |
|---|---|
| skills/basecamp/SKILL.md | Documents Ping write restrictions. |
| internal/tui/workspace/views/hey.go | Uses type-aware removal and confirmation. |
| internal/tui/workspace/views/hey_test.go | Tests trash and line-deletion routing. |
| internal/tui/workspace/views/detail.go | Tracks API type and parent for confirmed deletion. |
| internal/tui/workspace/views/detail_test.go | Tests line labels, malformed display fields, and ordinary trash. |
| internal/tui/workspace/data/remove_recording_test.go | Tests removal routing and fail-safe behavior. |
| internal/tui/workspace/data/hub.go | Adds type-aware removal and recording metadata lookup. |
| internal/tui/workspace/actions.go | Refuses palette trash for Campfire lines. |
| internal/tui/workspace/actions_test.go | Tests palette safeguards and ordinary trash. |
| internal/commands/subscriptions.go | Explains refused Ping subscription updates. |
| internal/commands/spotlight.go | Explains refused Ping spotlight changes. |
| internal/commands/recordings.go | Adds shared Ping refusal detection and recovery hints. |
| internal/commands/recordings_test.go | Tests refusal hints, output modes, and fallback errors. |
Review effort: Balanced
The TUI read a recording's type from the bucket-less /recordings/<id>.json, which bc3 answers with a 404 for some types, such as Kanban::Card, so :trash on a card stopped trashing it. RecordingKind and RemoveRecording now take the project and read /buckets/<project>/recordings/<id>.json, as Detail does. The commands a Ping refusal names now carry the profile and account the refused command ran under, read before it resolved its account, as the spotlight breadcrumbs do, so pasting one into a fresh shell acts on the same account.
Member
Author
|
Review threads: 2 resolved (2 fixed, 0 declined).
On 805c2d8, CI is green, Copilot has approved, and Codex's code and security passes completed with no findings. The |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What was wrong
bc3#14171 makes Basecamp's generic recording routes answer an empty
403 Forbiddento any write on a recording in a Ping: trash, archive, unarchive, client visibility, title, publication, others' subscriptions, spotlights. Subscribing or unsubscribing yourself, bubbling up, boosts and the Campfire line endpoints still work.The TUI trashed whatever its Detail view, its
:trashpalette action and its Hey view held throughRecordings().Trash. A Campfire line reaches Detail from a search hit (bc3's search returns lines, Ping lines included), from recents, a quick jump, or abasecamp tuideep link to a Ping line. Pressingttwice on one was:DELETE /chats/:chat/lines/:idmakes on who may delete a line and until when. That works today and still will after the deploy.The Hey view named in review can't hold a line on main: its feed is
Recordings().Listof messages, to-dos and documents only. It now decides by type like the others anyway.The CLI commands that write through the gated routes would report bc3's bare 403 as "access denied", with no hint.
What changed
TUI (dev builds only):
Hub.RemoveRecordingdecides by the API's recording type. It deletes aChat::Lines::*recording withCampfires().DeleteLineand trashes everything else.RemoveRecording.t delete.:trashhas only the scope's label for the type, so it reads the recording first, through its bucket (bc3's bucket-less/recordings/<id>.jsonanswers 404 for a card).ttwice in the line's view, where deletion is confirmed.RemoveRecording.CLI:
pingWriteRefusalfollows theclientsForbiddenErrorpattern. On a 403 from a gated route it reads the recording back once. If its bucket is a Circle, the error says Basecamp doesn't change a Ping that way and names the command that works:basecamp chat delete <line> --room <campfire> --in <ping>, built withdeleteLineCmdso it carries--forceonly for machine output;subscriptions subscribeorsubscriptions unsubscribe.Any other 403, or a recording it can't read, converts as before. It's wired into:
recordings trash,archive,restoreandvisibility;trash,archiveandrestore;subscriptions addandremove;spotlightandunspotlight.Skill: one bullet in the Pings section of
skills/basecamp/SKILL.mdsaying which commands a Ping refuses.Sweep
Changed: the paths above.
Left as they are:
Hub.TrashRecording. Their items are never lines, and they live in a project.s) and boost (b) subscribe or unsubscribe you, or boost, which a Ping allows. Lines carry no comments, and the comment routes aren't gated.recordings trash <line in a project Campfire>still trashes the line through the generic route, which bc3 keeps serving outside Pings.chat update,chat delete,pings *,boost,subscriptions subscribeandunsubscribeuse the line, Ping, boost and self-subscription endpoints, which bc3#14171 leaves open.basecamp mcpsends what the agent asks to the API, from the basecamp/mcp catalog, and returns bc3's 403 as it comes.tools show,update,trash,enable,disableandrepositionon a Ping's Campfire now get a 404 or 403 (the same bc3 PR changesDocks::ToolsController). Reaching that means passing a Ping's Campfire ID to a dock-tool command, so the plain error stands.basecamp connect) only boosts lines and reads subscriptions.files moveonly moves items in a project's Docs & Files.Tests
These failed on
mainfirst:Campfire::Line, or no label (quick jump) is confirmed as a delete and sends onlyDELETE /chats/222/lines/111. That holds even when a field Detail displays won't parse.:trash: on the same three labels it sends no write and returns the line refusal. It writes nothing when it can't read the recording.chat deleteconfirmation guard it would hit.--forceoff.--profileand--account, every hint names both.These pin behavior that main already had:
:trashand Hey still trash a non-line.Hub.RemoveRecordinghas unit tests for each branch, including that it writes nothing without a type or without a line's Campfire.bin/cipasses on the head.Release
The TUI is built only with
-tags dev; release builds carry a stubtuicommand, so that half ships in no released binary. The CLI half only changes the text of errors bc3 would return anyway. Nothing needs to be released before bc3#14171 deploys. The next routine release can carry this.