Conversation
![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
basecamp-cli/internal/commands/campfire.go
Lines 309 to 313 in e51d1b0
Stop forcing project selection for campfire post
The new SDK call no longer uses a bucket/project, but this block still forces ensureProject when no --project/config is set. That means bcq campfire post --campfire <id> will now prompt or fail in non‑interactive contexts even though a campfire ID is sufficient after the flat-route change. This is a functional regression for scripts and makes a previously valid flow (ID-only) unusable unless the user provides an unrelated project.
ℹ️ 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".
There was a problem hiding this comment.
Pull request overview
This pull request migrates the CLI to use flat SDK routes by removing bucketID/projectID parameters from method signatures. The changes are part of a coordinated effort with the SDK to flatten all routes from /{accountId}/buckets/{projectId}/resource/{id} to /{accountId}/resource/{id}.
Changes:
- Updated SDK dependency to version with flat route support
- Removed project/bucket ID parameters from single-resource SDK method calls across ~33 command files
- Removed timesheet entry CRUD commands and clock shortcut (functionality removed from SDK)
- Eliminated unnecessary project resolution logic from commands that no longer require project context
- Updated breadcrumb commands to remove
--inflags where project is no longer needed
Reviewed changes
Copilot reviewed 32 out of 33 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| go.mod/go.sum | Updated basecamp-sdk dependency to flat routes version |
| internal/observability/collector.go | Removed BucketID field from OperationMetrics struct |
| internal/tui/resolve/*.go | Updated SDK calls to remove bucketID parameters |
| internal/names/resolver.go | Updated API paths to use flat routes (removed /buckets/{id} prefix) |
| internal/commands/webhooks.go | Removed project resolution, now operates at account level |
| internal/commands/todos.go | Removed project parameters from single-resource operations (show, complete, reopen, position) |
| internal/commands/messages.go | Removed project resolution from single-resource operations |
| internal/commands/cards.go | Removed project parameters from card/column/step operations that don't need project context |
| internal/commands/timesheet.go | Removed timesheet entry CRUD and clock command, updated remaining commands |
| internal/commands/timeline.go | Updated ProjectTimeline calls to remove bucketID parameter |
| internal/commands/*.go | Similar updates across 25+ other command files |
| internal/commands/commands_test.go | Removed clock command from tests |
| internal/cli/root.go | Removed clock command registration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 37 out of 38 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 37 out of 38 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 39 out of 40 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jeremy
force-pushed
the
main
branch
5 times, most recently
from
b0ec09b to
34084fa
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 39 out of 40 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 50 out of 51 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Re: Codex review (campfire post forcing project selection) Fixed — |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 56 out of 57 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Update all SDK service calls to use flat URL routing where bucketID/ projectID has been removed from method signatures. This is the CLI-side companion to basecamp/basecamp-sdk#74 which flattens all SDK routes from /{accountId}/buckets/{projectId}/resource/{id} to /{accountId}/resource/{id}. Key changes: - Remove bucketID first-arg from ~120 SDK method calls across 25+ files - Remove project resolution from single-resource commands (show, complete, update, etc.) that no longer need project context - Flatten raw API paths in show.go and resolver.go - Remove timesheet entry CRUD commands (Get/Create/Update/Trash removed from flat SDK) and clock shortcut - Remove unused project parameters from card column/step and todolist group subcommands - Update command catalog, breadcrumbs, and tests
…arded Addresses PR review: commands that no longer need project context should use extractID() instead of extractWithProject() with the second return value discarded.
Remove "without project shows error" tests for events, messagetypes, recordings visibility, timesheet recording, and webhooks — these commands no longer require project context with flat SDK routes.
ProjectTimeline and ProjectReport now accept projectID param. Webhooks List/Create now accept bucketID param. Timesheet entry route segment fixed (timesheet_entries).
Timesheet: add --project/--in flags, pass projectID to ProjectReport. Webhooks: add --project/--in flags, pass bucketID to List/Create. Boost delete: add project resolution (matches list/show/create). Timeline: pass projectID to ProjectTimeline in both one-shot and watch.
…ost delete These SDK methods operate by globally-unique ID and don't accept a bucketID param. The resolution chains were prompting users for a project that was never used.
Aligns URL parse output with flat route terminology. Updates unit tests, e2e tests, and skill documentation.
When a campfire ID is passed directly, the flat SDK doesn't need a project. Only resolve project when deriving campfire from project dock.
The Comment resolver requires a projectID to show the recording picker. Ensure project is resolved before calling the interactive path.
SDK flat branch merged to main. Bumps from 96abdc8 (flat) to 6b4c632 (main), picking up the /buckets/ → /projects/ path corrections for GetProjectTimeline and GetProjectTimesheet.
These tests were added aspirationally in #132 for a breadcrumbs feature that hasn't been built yet. Mark them as skip to unblock the release.
- Use GOWORK=off in provenance-check so go list reads go.mod directly instead of resolving through the workspace - Skip unimplemented breadcrumb e2e tests (aspirational from #132)
Summary
bucketID/projectIDhas been removed from method signatures/{accountId}/buckets/{projectId}/resource/{id}to/{accountId}/resource/{id}clockshortcut (methods removed from flat SDK)Test plan
go build ./...— clean compilego vet ./...— no warningsgolangci-lint run ./...— 0 issuesgo test ./...— all pass (except pre-existingTestFindRepoConfigmacOS symlink issue on main)bcq todos show <id>works without--inbcq done <id>works without--inbcq comment --on <id> --content "test"works without--inbcq todos --in <project>still works (listing needs project)bcq todo --content "test" --in <project> --list <list>still works (creation needs project)