Repository navigation
Say what tools trash really does: delete or disable, never trash - #940
Conversation
DELETE /dock/tools/:id deletes an external link or an empty extra tool of a type, and disables every other tool. Neither goes to the trash, and both answer 204. The help promised permanent removal of the tool and all its content, and the summary said "trashed". Describe both outcomes in the help, word the summary as "removed from the dock (deleted or disabled)", and point a breadcrumb at tools show, which tells them apart. The data key stays "trashed" for existing scripts.
Codex review: a Card Table counts only cards, so one with columns and no cards is empty and gets deleted; and a deleted external link can still be restored in the web app until it is destroyed. Say both.
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. |
github-actions
Bot
added
commands
There was a problem hiding this comment.
🟢 Approved
These narrowly scoped messaging changes preserve behavior and data compatibility, include focused regression coverage, and have no identified blocking issues.
No open issues 🎉
What changed in this PR
Corrects basecamp tools trash messaging to describe Basecamp’s delete-or-disable behavior without changing the operation or breaking existing script output.
Changes:
- Updates help, agent notes, and the success summary to clarify that tools never enter the trash.
- Adds a
tools showbreadcrumb and regression coverage while preserving{"trashed": true}compatibility.
[!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 |
|---|---|
internal/commands/tools.go |
Clarifies removal outcomes and adds a verification breadcrumb. |
internal/commands/tools_test.go |
Pins the corrected summary and project-scoped breadcrumb. |
Review effort: Balanced
…d text The built-in MCP server's delete_tool still described itself as "Delete a tool (trash it)", from the vendored SDK model. basecamp-sdk#1031 corrected the description; pin the SDK at that merge (scripts/bump-sdk.sh) and re-vendor the model from it (scripts/sync-mcp-model.sh). The bump also brings in the SDK's other change since v0.27.0: reading a pixel dimension one ulp off an integer as that integer (basecamp-sdk#1028).
![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: 64d1b99039
ℹ️ 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".
The Nix flake build computed sha256-U8sN… for the go modules at the new pin.
tools trash points to tools show to tell its outcomes apart, but show read every missing position as disabled, so a deleted tool came back "at position disabled": the opposite outcome. Check status first. Codex review on #940.
basecamp tools trashsaid it would "permanently trash a dock tool" and remove "the tool and all its content", and then reported "Tool N trashed". Basecamp does neither.DELETE /dock/tools/:idtakes one of two paths, and neither one uses the trash (bc3 API docs, "Trash a tool", corrected in basecamp/bc3#13973):tools disable, andtools enablebrings it back.Both outcomes return 204. A customer saw this through the MCP server: the project's original To-dos, Docs & Files and Chat were reported as trashed and restorable, but they had only been disabled. basecamp/basecamp-mcp-server#273 fixes the MCP reply.
Changes
tools trashhelp now describes both outcomes and says how to tell them apart: runtools show <id>, and a deleted tool has status"deleted". Thetoolsagent notes say the same in one line.basecamp tools show <id>.{"trashed": true}, so scripts that read it keep working. A comment at the call site explains why.TestToolsTrashSummaryDoesNotClaimTrashpins the summary and the breadcrumb. It fails onmain, where it gets "Tool 557 trashed" and noshowbreadcrumb.Not in this PR
MCP model sync (follow-up commits)
basecamp/basecamp-sdk#1031 is merged as
d69e9669. This PR now pins the SDK at that commit withscripts/bump-sdk.sh, a pseudo-version since no Go release has been cut. It re-vendorsinternal/mcpserver/modelfrom the commit withscripts/sync-mcp-model.sh, so the built-in MCP server'sdelete_toolnow describes both outcomes. The catalog snapshot is regenerated, andnix/package.nix'svendorHashis updated for the new modules. The bump also brings in basecamp/basecamp-sdk#1028, which reads a pixel dimension one ulp off an integer as that integer. That is the SDK's only other Go change since v0.27.0.Review
Our Codex review (gpt-6.1-sol, high) of the sync commit found nothing.
Codex (gpt-6.1-sol, high) made two findings, and both are taken. "Empty" now carries bc3's Card Table rule, so a table with columns and no cards counts as empty and gets deleted. The blanket "can't be restored" is gone, because a deleted external link can be restored in the web app until it's destroyed.