Repository navigation
Conversation
chat uploads lists the files posted in a chat (ListCampfireUploads). chat bots lists, shows, creates, updates and deletes chatbots (ListChatbots, GetChatbot, CreateChatbot, UpdateChatbot, DeleteChatbot): managing them is an OAuth call an administrator makes, so the chatbot-key reason API-COVERAGE.md gave for skipping chatbots now covers only posting as one.
|
🤖 @codex review |
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. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 9 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/commands/chat_bots.go">
<violation number="1" location="internal/commands/chat_bots.go:354">
P2: `--command-url` is documented as HTTPS, but `create` and `update` send any string to the API without checking the scheme. Reject values that do not start with `https://` with `output.ErrUsage` before the request, in both commands.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| }, | ||
| } | ||
|
|
||
| cmd.Flags().StringVar(&commandURL, "command-url", "", "HTTPS address Basecamp calls when the bot is addressed") |
There was a problem hiding this comment.
P2: --command-url is documented as HTTPS, but create and update send any string to the API without checking the scheme. Reject values that do not start with https:// with output.ErrUsage before the request, in both commands.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At internal/commands/chat_bots.go, line 354:
<comment>`--command-url` is documented as HTTPS, but `create` and `update` send any string to the API without checking the scheme. Reject values that do not start with `https://` with `output.ErrUsage` before the request, in both commands.</comment>
<file context>
@@ -0,0 +1,504 @@
+ },
+ }
+
+ cmd.Flags().StringVar(&commandURL, "command-url", "", "HTTPS address Basecamp calls when the bot is addressed")
+ return cmd
+}
</file context>
There was a problem hiding this comment.
🤖 Not changed: the server owns which command URLs it accepts, and a client-side scheme check would refuse what the API might accept now or later, or pass what it refuses for another reason. The API's own refusal comes back through the usual error envelope. Leaving this thread open for a person to decide.
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved defects leave uploads incorrectly ordered, hide chatbot names or posting URLs, and generate follow-up commands that can use the wrong credentials or account.
3 open issues
What changed in this PR
Adds chat upload listing and OAuth-based chatbot management to the CLI, exposing six existing SDK operations. Chatbot posting remains out of scope.
Changes:
- Adds
chat uploadswith pagination controls andchat botswith list, show, create, update, and delete commands. - Adds input validation, deletion confirmation, and help explaining account-wide effects and chatbot-key handling.
- Updates command discovery, API coverage, unit tests, and end-to-end smoke scenarios.
[!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/commands.go |
Adds uploads and bots to the command catalog. |
internal/commands/chat.go |
Registers the new chat subcommands. |
internal/commands/chat_bots.go |
Implements upload listing and chatbot management. |
internal/commands/chat_bots_test.go |
Tests requests, validation, updates, and deletion safeguards. |
e2e/smoke/smoke_campfire.bats |
Adds upload-listing and chatbot lifecycle smoke tests. |
e2e/smoke/smoke_aliases.bats |
Records coverage treatment for the bot alias. |
e2e/chat.bats |
Checks usage errors and security-related help text. |
API-COVERAGE.md |
Documents newly covered operations and distinguishes chatbot posting. |
.surface |
Records new commands, aliases, arguments, and flags. |
Review effort: Balanced
…llow-ups chat uploads asks for newest first, so a --limit keeps the latest files. Every chat uploads and chat bots breadcrumb carries --profile and --account where a fresh process wouldn't pick them up. Chatbots get a presenter schema that keeps the name and both URLs. The smoke lifecycle runs serially within its file and marks itself unverifiable only on a forbidden answer; the e2e refusals assert the exact error.
|
🤖 @codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
All reported issues were addressed across 7 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
… run it A smoke token that isn't an account administrator gets forbidden on create, and show, update and delete read the bot create made, as the timesheet and lineup lifecycles already allow. The coverage row says newest first, as the command asks.
There was a problem hiding this comment.
🔵 Needs a closer look
A maintainer must confirm the decision to reverse the CLI’s chatbot-management exclusion because it introduces account-wide administrator mutations and exposes posting-key URLs.
No open issues 🎉
1 resolved since last review
Review effort: Balanced
|
🤖 Converged on
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
basecamp chat uploadslists the files posted in a chat, andbasecamp chat botslists, shows, adds, renames and deletes its chatbots.Why
[CLI] Campfire chatbots and the uploads list: the coverage report (basecamp-sdk#1055) lists six SDK operations the CLI doesn't reach.
API-COVERAGE.mdskipped chatbots because they "require chatbot key, not OAuth", but that's only true of posting as a chatbot. Managing them goes through OAuth, which is how the MCP server already serves all five as*_chatbottools.The decision this PR asks for: whether the CLI should manage chatbots at all. It reverses a recorded skip, so it's a person's call, not this job's. If the answer is no, drop
chat_bots.go'sbotsgroup and the chatbots rows, andchat uploadsstands alone.What changed
ListCampfireUploadsbasecamp chat uploadsListChatbotsbasecamp chat bots listGetChatbotbasecamp chat bots show <id>CreateChatbotbasecamp chat bots create <name> [--command-url URL]UpdateChatbotbasecamp chat bots update <id> [--name NAME] [--command-url URL]DeleteChatbotbasecamp chat bots delete <id> [--force]--roomor the project's one chat, and the project from--in, as the otherchatcommands do. Chatbot routes name the project, so it's resolved even when--roomis given.chat uploadsreturns the chat lines that carried a file, newest first (asked for explicitly, so--limitkeeps the latest): 100 by default, or--limit,--allor--page. Contradictory flags are refused before any request.updatereads the chatbot first and sends back whatever you didn't change, because the API takes the name on every update. Without that, a rename might clear the command URL. The SDK leaves out an empty command URL, so you can change one but not remove it, and the help says so.updatewith nothing to change is refused.deleteis permanent and account-wide. Likechat delete, it asks for confirmation, or needs--forcewhere nothing can answer.lines_urlholds the bot's key, so anyone who has it can post as the bot and it should be treated like a password. The CLI printslines_urlas the API returns it, as the MCP server does: handing it to whoever runs the bot is the point of creating one.lines_urlandcommand_urlin the styled list, the Markdown list and the detail view. Every follow-up command carries--profileand--accountwhere a fresh process wouldn't pick them up.botis an alias ofbots. Catalog entry,.surface, andAPI-COVERAGE.md: chatbot management and chat uploads are now covered rows. Posting as a chatbot stays out of scope for the old reason, and the notes now explain the difference. Summary counts go to 227 of 227.skills/basecamp/SKILL.mdisn't touched: it's 130 bytes under its 16 KB limit, and the help covers these commands.Builds on
Main at
f45d3c8e, whose pinned SDK (v0.34.0) has all six operations. No bump in this PR.Left out
Nothing from the card. Posting as a chatbot (
POSTtolines_urlwith the key) isn't an operation on the card and stays out of scope.Tests
internal/commands/chat_bots_test.go): each command's method, path and body;uploadsasks for newest first, and--pagesends the page; every command's follow-ups carry the profile; conflictinguploadsflags are refused with nothing sent;createwith and without--command-url;updatekeeps the field you leave out, in all three combinations; an empty name, an empty command URL, nothing to change and a non-numeric or zero ID are each refused before any request;deletewithout--forcein machine output sends nothing, and with it sends one DELETE; a 403 oncreatecomes back as an error with nothing printed and no retry.internal/presenter/presenter_test.go): the chatbot schema keeps the name and both URLs in each view.e2e/chat.bats): the usage refusals, asserting the exact error and hint, and the help text naminglines_url's key and the administrator requirement.e2e/smoke/smoke_campfire.bats):uploadsafter the existing upload, and a chatbot lifecycle (create, list, show, update, delete--force). The file now runs its tests in order (BATS_NO_PARALLELIZE_WITHIN_FILE), which the existing line lifecycle also relied on. Create is marked unverifiable only on aforbiddenanswer, since the smoke user may not be an administrator, and.qa-allowlistnames it and the three tests that read its bot, as it does for timesheets and lineup. Written but not run, since smoke needs a live account.chat botis marked as an alias insmoke_aliases.bats.On thelio,
bin/cipasses fmt, vet, golangci-lint (0 issues), actionlint and zizmor. Its unit tests fail only inTestHardenConfigDir(internal/cli, two subtests). That's the same failure #992 recorded, and it reproduces on a clean export off45d3c8eon that machine; this PR doesn't touchinternal/cli. The rest ofmake check(e2e, surface, skill drift, smoke coverage, naming, provenance, tidy) passes. Remote CI is the check forTestHardenConfigDir.Gap card
https://app.basecamp.com/2914079/buckets/48521764/card_tables/cards/10447988117 (the MCP server already serves the chatbot operations)
Summary by cubic
Adds
basecamp chat uploadsto list files posted in a chat, andbasecamp chat botsto list, show, create, rename, and delete chatbots. Chatbot management works through OAuth as an administrator's call, so it reverses the prior reasoning that skipped chatbots; only posting as a chatbot, which needs the chatbot key, stays out of scope. Reviewers get to decide whether chatbot management belongs in the CLI — if not, drop thebotsgroup and keepchat uploadsstandalone.Notable behaviors
chat uploadsrequests files newest first so a--limitkeeps the latest ones.updatereads the chatbot first and sends back what you didn't change, since the API takes the name on every update; a command URL can be changed but not removed.deleteis permanent and account-wide, so it asks for confirmation or needs--forcewhere nothing can answer.lines_urlcarries the bot's key and should be treated like a password.Written for commit eca8d44. Summary will update on new commits.