Carry an explicit project into the bare boost list usage hint - #591
Carry an explicit project into the bare boost list usage hint#591jeremy wants to merge 1 commit into
Conversation
github-actions
Bot
added
commands
Review carefully before merging. Consider a major version bump. |
![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: 4e88f2cf90
ℹ️ 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 PR removes the CLI’s account-wide boost list behavior and the --all-projects flag to avoid calling the soon-to-be-withdrawn BC5 endpoint (GET /:account/boosts.json), keeping boost list item-scoped only and updating docs/surface tracking accordingly.
Changes:
- Remove
boost listaccount-wide fallback +--all-projectsflag; keepboost listitem-scoped and return a usage hint on bare invocation. - Update unit tests to assert bare
boost listis a usage error and makes no API requests; remove now-obsolete account-wide route stubs/tests. - Update docs and CLI surface metadata to reflect the removal (SKILL.md, ACCOUNT-WIDE-LISTINGS.md, API-COVERAGE.md,
.surface,.surface-breaking).
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 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
internal/commands/boost.go |
Removes account-wide boost listing + --all-projects; makes boost list item-scoped and returns a usage hint when no ID is provided. |
internal/commands/boost_test.go |
Reworks boost list tests around the new “usage error, no API call” behavior; deletes account-wide listing tests. |
internal/commands/accountwide_test.go |
Drops boost from account-wide listing todolist rejection coverage since it no longer has an account-wide path. |
skills/basecamp/SKILL.md |
Updates documentation to reflect 7 (not 8) account-wide list commands. |
ACCOUNT-WIDE-LISTINGS.md |
Updates the method matrix/invariants to reflect removal of the account-wide boost aggregate. |
API-COVERAGE.md |
Updates EverythingService coverage note/matrix to reflect 16/17 reachable methods due to boost feed withdrawal. |
.surface |
Removes --all-projects from basecamp boost(s) list. |
.surface-breaking |
Records the removed --all-projects flags as acknowledged breaking surface changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
4e88f2c to
1caccdb
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
jeremy
changed the title
PR #590 withdrew the account-wide boost listing (bc3 #12464) while this branch was in flight, absorbing its flag removal, surface acknowledgments, docs, and most of its tests. What survives is the one refinement #590 did not pick up: a bare `boost list` that names a project — via --project, its --in alias, or the root-level flag — gets a usage hint that carries that project forward into the corrected invocation, instead of the generic missing-argument path. An explicit --event rides along in the same hint rather than being silently dropped from the correction. A config-supplied project still takes the generic path: only an explicit flag is a statement of intent worth echoing back. The new test pins the contract across all three flag forms plus the --event combination, and asserts no request reaches the API. It fails against the pre-change code, where a bare interactive invocation answered with help and a nil error rather than a structured usage error.
1caccdb to
5f1f4a2
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Why
This PR set out to withdraw the CLI's account-wide boost listing ahead of BC5 removing
GET /:account/boosts.json(basecamp/bc3#12464; diagnosis basecamp/bc3#12458). While it was in flight, #590 landed the same withdrawal as part of bounding all the account-wide listings — item-ID-requiredboost list,--all-projectsgone,.surface/.surface-breakingacknowledgments, docs, and the guard tests — and #594 adopted SDK v0.11.0, which droppedEverything().Boosts()entirely (basecamp/basecamp-sdk#504). The original body's premise that the SDK method stays is obsolete.Rebased onto post-#590/#594 main, one refinement from this branch survives that #590 did not pick up: the missing-ID error for
boost listis project-aware.What
boost listthat names a project explicitly — via--project, its--inalias, or the root-level flag that lands inapp.Flags.Project— now gets a usage hint that carries the project forward into the corrected invocation (Pass the item's ID or URL: basecamp boost list <id> --project my-project) instead of the generic missing-argument path. An explicit--eventrides along in the same hint rather than being silently dropped from the correction. A config-supplied project still takes the generic path: only an explicit flag is a statement of intent worth echoing back..surface-breakingentries (already present on main at lines 136–137), ACCOUNT-WIDE-LISTINGS.md/API-COVERAGE.md/SKILL.md updates, and the bare/configured-project/--eventguard tests all resolve to main's versions with no residual delta.Tests
TestBoostListExplicitProjectWithoutIDCarriesProjectIntoHintpins the contract across all three flag forms plus the--eventcombination, and asserts no request reaches the API. Shown red against pre-change main, where a bare interactive invocation answered with help and a nil error rather than a structured usage error.make check(fmt, vet, lint, unit, e2e, surface, skill-drift, smoke-coverage, provenance, tidy) andmake check-surface-compatboth pass.