feat(bubble-up): add bubble-up add/remove commands - #677
Conversation
github-actions
Bot
added
commands
There was a problem hiding this comment.
🟡 Changes recommended
The SDK pin remains unreleased, incidental SDK operations lack required CLI coverage, and validation/provenance documentation is incomplete.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds personal bubble-up add/remove commands, backed by new SDK operations.
Changes:
- Implements add/remove commands with URL support and scheduling.
- Registers commands and adds tests/surface snapshots.
- Updates the SDK pin and vendored MCP model.
[!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 summaries
| File | Description |
|---|---|
.surface |
Adds bubble-up command surface. |
go.mod |
Pins the SDK feature revision. |
go.sum |
Updates dependency checksums. |
internal/cli/root.go |
Registers the command group. |
internal/commands/bubble_up.go |
Implements add/remove behavior. |
internal/commands/bubble_up_test.go |
Tests requests and argument handling. |
internal/commands/commands.go |
Adds the catalog entry. |
internal/commands/commands_test.go |
Registers the command in parity tests. |
internal/mcpserver/catalog_test.go |
Updates the served-operation count. |
internal/mcpserver/domains.go |
Maps BubbleUps into the account domain. |
internal/mcpserver/model/behavior-model.json |
Adds operation behavior metadata. |
internal/mcpserver/model/openapi.json |
Syncs the expanded SDK API model. |
internal/mcpserver/model/PROVENANCE.json |
Records model source provenance. |
internal/mcpserver/testdata/catalog_snapshot.txt |
Updates the MCP catalog snapshot. |
Review details
- Files reviewed: 13/14 changed files
- Comments generated: 6
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![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: 10f088d1f2
ℹ️ 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".
| charm.land/bubbletea/v2 v2.0.9 | ||
| charm.land/lipgloss/v2 v2.0.6 | ||
| github.com/basecamp/basecamp-sdk/go v0.15.0 | ||
| github.com/basecamp/basecamp-sdk/go v0.15.1-0.20260902165153-4b2a7cddbd0f |
There was a problem hiding this comment.
This dependency bump also introduces ListRecentProjects, RecordProjectVisit, SpotlightRecording, and UnspotlightRecording in the vendored SDK model, but a repo-wide search finds no corresponding CLI commands, catalog entries, or registrations. Consequently those new SDK capabilities remain inaccessible from the CLI; add their command surfaces or explicitly resolve any missing service-wrapper blocker before taking this bump.
AGENTS.md reference: AGENTS.md:L131-L132
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Flagging to the operator rather than resolving. The SDK pin advances monotonically to the bubble-up head, which also carries ListRecentProjects, RecordProjectVisit, and SpotlightRecording/UnspotlightRecording — all added to the SDK after v0.15.0. Each needs its own CLI command, catalog entry, root registration, tests, and coverage row (with its own flag/output design), which is separate feature work from bubble-up. Whether to bundle those commands here or land bubble-up now and track them as follow-up coverage is a scope/sequencing call, so I'm leaving this open for the operator. (CLI Surface Check is green — the completeness bar is a documented practice, not a CI gate.)
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. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 14 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 257b5da3b9
ℹ️ 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".
| "at": { | ||
| "type": "string", | ||
| "description": "Timing for the bubble-up. `\"now\"` bubbles up immediately; a scheduling\nkeyword (`\"today\"`, `\"tomorrow\"`, `\"weekend\"`, `\"next_week\"`) or an ISO8601\ndate (e.g. `\"2026-09-10\"`) schedules it to resurface later. bc3 requires a\nvalue \u2014 omitting `at` errors server-side (`Date.iso8601(nil)`) \u2014 so send\n`\"now\"` for the immediate case." |
There was a problem hiding this comment.
When an MCP caller invokes the newly advertised create_bubble_up action with only its required recordingId, this schema permits at to be omitted, and the generic dispatcher consequently posts a nil body. The description on this property confirms that current bc3 raises on a missing value; the CLI command avoids that by injecting "now", but MCP bypasses the command entirely. Require at in the served schema or inject "now" on the MCP path so the minimal tool call does not fail server-side.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Flagging to the operator rather than resolving. The MCP surface faithfully serves the SDK's optional-at schema. Injecting now would mean special-casing one operation in the deliberately generic catalog dispatcher (there is no per-operation default hook), and marking at required in the served schema would diverge the vendored model from the SDK source it mirrors and contradict the SDK's deliberate optional-at design. The missing-at server-side raise is the same bc3 gap the operator is resolving via a forthcoming bc3 default, which fixes every path at once (SDK, MCP, and the CLI without its now stopgap). Leaving open for the operator's call on an interim MCP-path default.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9433de4b60
ℹ️ 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.
🟡 Changes recommended
The unreleased SDK pin, incomplete SDK-method CLI coverage, and incorrect --at now summary must be resolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
internal/commands/bubble_up.go:92
- An explicit
--at nowenters this branch and reports that the recording was “Scheduled,” even though the command and SDK definenowas an immediate bubble-up. Treat explicitnowthe same as the omitted default; a regression assertion for the summary would also catch this case.
- Files reviewed: 15/16 changed files
- Comments generated: 2
- Review effort level: Balanced
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb021e8f08
ℹ️ 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.
🟡 Changes recommended
The SDK remains unreleased, additional imported SDK operations lack required CLI parity, and ISO 8601 validation rejects a supported form.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
go.mod:9
- This still pins an unreleased pseudo-version, while the referenced SDK PR #839 is open/blocked and the latest release remains v0.15.0. The PR description explicitly requires replacing this with the released tag before merge; after that, regenerate
go.sum, both provenance files/model snapshots, and the Nix vendor hash.
github.com/basecamp/basecamp-sdk/go v0.15.1-0.20260902171637-e03865568c6f
- Files reviewed: 17/18 changed files
- Comments generated: 2
- Review effort level: Balanced
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
You’re at about 93% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
🟡 Changes recommended
The SDK bump still exposes four operations without the repository-required CLI coverage, and the agent metadata names the wrong full-list command.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 17/18 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 74b1fc9ff5
ℹ️ 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.
🟡 Changes recommended
SDK provenance is inconsistent, and recording URL validation plus the MCP omission behavior need correction.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
internal/commands/bubble_up.go:167
extractIDfalls back to a parsed URL's project or account ID when no recording ID exists (internal/urlarg/urlarg.go:114-124). Consequently, a project URL such as/123/projects/456is accepted here and sent as recording 456, potentially targeting the wrong resource instead of returning a usage error. Resolve throughextractWithProjectand reject an empty recording component.
- Files reviewed: 17/18 changed files
- Comments generated: 2
- Review effort level: Balanced
Bump the basecamp-sdk/go pin to the current feat/recording-bubble-up head via `make bump-sdk`, syncing go.mod and internal/version/sdk-provenance.json (the prior manual pin left provenance at v0.15.0, tripping provenance-check). Bump google.golang.org/grpc v1.82.1 -> v1.83.1 to clear GHSA-vp52-pcj8-j9qc (HIGH: gRPC-Go HTTP/2 DATA-frame OOM), which Trivy flags on the module graph; grpc is compiled in via sigstore-go's verify path. Regenerate the Nix vendorHash for the go.mod/go.sum change (verified building via the pinned nixos/nix image).
TestCatalogModelProvenance pins the vendored model's PROVENANCE ref to the basecamp-sdk version go.mod resolves. Bumping the SDK pin to the bubble-up head left the ref at the prior commit; refresh it to the go.mod pseudo-version for e0386556. The vendored behavior-model.json/openapi.json are byte-identical (the SDK head added only tests and docs, no operations), so only the provenance pointer moves.
Command fixes from review: - Validate --at locally before account resolution — a typo like "tomorow" is now a usage error instead of a server-side Date.iso8601 raise. - Report an explicit `--at now` as immediate (bubbled_up:true, "Bubbled up" summary), matching the omitted default rather than claiming it was scheduled. - Set bubbled_up from the effective timing, so a future --at reports bubbled_up:false (only in the scheduled set). - Reject a non-positive recording id as a usage error. - Point the post-add "view" breadcrumb at the registered `basecamp show`; `basecamp recordings show` does not exist. - Cover remove's non-id rejection and the new --at/id paths in tests; the URL-acceptance test no longer pastes a cross-account URL. Docs: API-COVERAGE.md bubble_ups row now lists the add/remove write ops and the SDK version; the basecamp agent skill gains bubble-up triggers, quick reference, and a Personal-section entry.
…-ups Round-two review fixes: - validateBubbleUpAt now accepts only "now", the keywords, or YYYY-MM-DD. bc3 schedules at day granularity (keywords map to fixed hours; Date.iso8601 reduces any value to a calendar day), so a timestamp with a time component would be silently truncated and echoing it back would misreport the honored time. Rejecting timestamps and ISO basic dates keeps the contract to the one form bc3 honors as written; help text updated to match. Tests cover a bare date reaching the wire and rejection of a timestamp / basic date. - API-COVERAGE.md summary totals move 184 -> 186 implemented and 196 -> 198 tracked for the two new bubble-up write endpoints, keeping the aggregate consistent with the per-row change. - Drop the duplicated bubble-up rows from the agent skill's quick reference (kept the personal-commands set next to bookmarks).
go.mod pins basecamp-sdk f439f493; bring internal/version/sdk-provenance.json, the vendored model PROVENANCE.json, and the Nix vendorHash into lockstep with that commit so TestCatalogModelProvenance, provenance-check, and the flake build all resolve against one SDK version.
…arred
add reports the requested timing rather than asserting a resulting state:
bc3 answers with a bare 204 and add is idempotent, so an add over an already
bubbled-up recording is indistinguishable from a fresh one. Drop the
bubbled_up state claim and summarize as a request. The agent note points to
"basecamp notifications bubbleups", the full current-and-scheduled listing,
instead of the general notifications inbox.
Projects view carries Starred through ProjectInfo and uses it for the star
marker and the /projects/{id}/star.json toggle. A project filed into a stack
is bookmarked but not starred, so keying the toggle off Bookmarked tried to
unstar an already-unstarred project.
644d52f to
ba6ee1a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ba6ee1a9c1
ℹ️ 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".
Quick Jump renders its project category as "Starred" and the interactive project picker prefixes a ★ glyph, but both keyed off Bookmarked, so a project filed into a stack but not starred was mislabeled as starred. Point both at Starred; ranking and the Home "Bookmarks" section keep Bookmarked, which is the aggregate pinned-or-stacked state they mean.
There was a problem hiding this comment.
🟡 Changes recommended
The optimistic star toggle leaves bookmark membership stale, and the skill overstates accepted date formats.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 26/27 changed files
- Comments generated: 2
- Review effort level: Balanced
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b6de9dbb66
ℹ️ 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.
🟡 Changes recommended
User-facing star labels and scheduling documentation remain inconsistent, and the corrected toggle behavior lacks regression coverage.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
skills/basecamp/SKILL.md:1111
- The skill promises any ISO 8601 date, but
validateBubbleUpAtaccepts only the extended calendar formYYYY-MM-DDand explicitly rejects basic ISO dates and timestamps. Describe the narrower format here so agents do not generate values the command rejects.
`next_week`) or an ISO8601 date. Both verbs are idempotent. There is no
internal/tui/workspace/views/projects.go:738
- This action now explicitly toggles a star, but its visible key hint still says
b bookmark(defaultProjectKeysline 119, pinned byTestProjects_ShortHelp_LeftPanel). That is misleading for stacked-but-unstarred projects; update the hint and its test to saystar.
// The toggle drives /projects/{id}/star.json, so it tracks Starred — a
// project filed into a stack is Bookmarked but not necessarily Starred.
internal/tui/workspace/views/projects.go:744
- The core toggle fix is not covered by the added tests: they verify only marker rendering, so a regression to deriving the request from
Bookmarkedwould still pass. Add a stacked project case (Bookmarked: true, Starred: false) that asserts the toggle chooses the star/create path and that an error restoresStarred.
newStarred := !p.Starred
// Optimistic: flip in local data, re-sort
p.Starred = newStarred
v.syncProjectList()
return v.setBookmark(projectID, newStarred)
- Files reviewed: 26/27 changed files
- Comments generated: 1
- Review effort level: Balanced
…consistent The skill note called --at an "ISO8601 date", which also promises the basic form (20260910) that validateBubbleUpAt rejects; name the accepted calendar form (YYYY-MM-DD) to match the command contract. Restore the projects-view toggle's error copy to "bookmark" so the surface's user-facing wording stays consistent with the "Bookmark updated" success text.
There was a problem hiding this comment.
🟡 Changes recommended
The changed star-toggle path lacks regression coverage, and the required SDK release-tag repin remains pending.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 26/27 changed files
- Comments generated: 1
- Review effort level: Balanced
The marker tests covered display; none drove the toggle itself, so a regression from Starred back to Bookmarked could POST/DELETE the wrong endpoint for a stacked-but-unstarred project and still pass. Add a test that toggles such a project and asserts it flips Starred, POSTs /star.json (never DELETE), and rolls the star back on error, backed by a new NewTestSessionWithClient helper that injects a stub-transport SDK.
There was a problem hiding this comment.
🟡 Changes recommended
The served MCP create_bubble_up action still permits omitting at, although the current backend rejects that request.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 27/28 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b8d69c081e
ℹ️ 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".
What
Surfaces the new basecamp-sdk Bubble Up write operations as a CLI command
group, mirroring
basecamp bookmarks:addbubbles a recording up in your readings. It bubbles up now bydefault;
--atschedules it instead — a keyword (today,tomorrow,weekend,next_week) or an ISO8601 date.removepops the bubble-up.Both verbs are idempotent and take a recording id or a pasted Basecamp URL, the
same as
bookmarks. Bubble-up is personal, so there's no--in <project>.The
atdefaultbc3 currently requires
at— an omitted param raises server-side(
Date.iso8601(nil)) — soaddsends"now"when--atis absent rather thanomitting it. That keeps the ergonomic "add == bubble up now" contract working
against bc3 today; the SDK layer stays neutral (it sends exactly what it's
given).
No status read is offered: the per-recording bubble-up
GETis anunrenderable API gap (its jbuilder lives outside
app/views/api/). The fullcurrent+scheduled list is
basecamp notifications(SDKGetBubbleUps).Dependency — blocked on the SDK PR
Depends on basecamp/basecamp-sdk#839 (adds the
BubbleUpsops). The SDK ispinned here to that feature branch as a pseudo-version placeholder
(
v0.15.1-0.20260902165153-4b2a7cddbd0f). Before merge, once the SDK PRlands and a release is cut, this must be re-pinned to the released
go/vX.Y.Ztag and the vendored MCP model re-synced from it(
scripts/sync-mcp-model.sh).Because the pin is a branch ahead of the last release (
go/v0.15.0), the MCPmodel re-sync also pulls the SDK-main operations merged since then
(recent-projects, delegated-events) — that's why the served-operation count
moves 247 → 253 rather than by just the two bubble-up ops. It resolves to a
clean tag-to-tag bump when re-synced from the release tag. The new
BubbleUpstag is claimed by the existing personal/account MCP domain.
Tests
bubble_up_test.go:addsendsat:"now"by default,--atschedulesverbatim, URL arg resolves,
removeDELETEs, non-id is a usage error.TestCatalogMatchesRegisteredCommands,TestSurfaceSnapshot, and the MCPTestCatalog*(snapshot, provenance,served-count) pass.
go build ./...,go vet,gofmtclean. (The pre-existing interactive-TTYtests —
TestBareBasecampNeverReportsASetupError,TestExplicitSetupStillRefuses,TestDeleteConfirmableFollowsTheAudienceNotTheDevice— fail identically ona pristine
origin/mainin a non-TTY shell; unrelated to this change.)Release note: Features (
enhancement).Summary by cubic
Previously, the CLI could only list bubble-ups through
basecamp notifications bubbleups; it now supports personalbasecamp bubble-up add|removecommands for changing them.addbubbles a recording up immediately by default or schedules it with--at, whileremovepops it; both are idempotent, andaddreports the requested timing because the API returns no resulting state.--atlocally.--atacceptsnow,today,tomorrow,weekend,next_week, or aYYYY-MM-DDdate; timestamps and basic-format dates are rejected because Basecamp uses day granularity.basecamp notifications bubbleupsremains the full current and scheduled list; no per-recording status command is added.Starredinstead ofBookmarked, including the project toggle, fixing labels and toggling for bookmarked-but-unstarred projects.Dependencies
github.com/basecamp/basecamp-sdk/goto the merged bubble-up support through a pseudo-version; repin it to thev0.15.1release and rerunscripts/sync-mcp-model.shwhen available.google.golang.org/grpcto v1.83.1 to address GHSA-vp52-pcj8-j9qc and refreshes related checksums and the Nix vendor hash.Written for commit b8d69c0. Summary will update on new commits.