Repository navigation
Keep agent connector profiles safe across server upgrades - #907
Conversation
|
Rob, R2-D2: I'm taking one copy fix on this branch, so please hold pushes to |
|
Pushed cd5c2955, copy only: the |
Reuse Rob Zolkos’s profile migration and operator coverage from CLI #901 at 8ead732 (f78ef23 through 8ead732). Preserve checkpoints, held work and offline operator fallback. Apply Jeremy Daer’s October 14 review: refuse unverified legacy shadow promotion, log adoption and sibling state, distinguish legacy locks, cover working wrong-person credentials and shadow adoption, and document mixed-version/downgrade recovery.
Reuse safe copy and lost-handover tests from Rob Zolkos’s CLI #900 at 7ba7e96 (2765820 through 7ba7e96), without its unconditional adds promises. Approval may add or replace until personal-agent adds are enabled, and shared agents still replace. Apply Jeremy Daer’s review: name the refused profile and both causes, warn overlap at guided project selection, keep live credential files, distinguish the newest lost connection from Disconnect all, and contract-test the skill stop fragment.
Seed explicitly held work in migration tests, verify it stays held through normal/shadow adoption and release, then discard it through the selected profile. Cover canonical-account lock collisions and invalid adoption names. Clarify the existing pre-run handoff age guard: migrated state is preserved, not automatically replayed.
Older builds wrote account ids verbatim into state directory names. Find that original directory during adoption, but canonicalize the new profile path so numeric aliases cannot bypass its lock. Cover a padded legacy account migration and document downgrade spelling.
… kind Basecamp will let team agents add connections too, behind their own switch (bc3 #13840), so the help, README, rollout doc and connect skill no longer say shared agents always replace. The CLI can't see the server's switches, so the copy says what each approval does under either.
cd5c295 to
396da98
Compare
|
Rebased onto main at 2dbca87 (v0.14.0): the head is now 396da98, rebased from cd5c295. What conflicted. All three conflicts were in copy that main had also changed:
One conflict the text merge didn't catch: two test helpers main added to
Not changed here, a call for Rob or the operator: Pings (#914) belong to no project. Every connection of one agent that runs without |
|
Review threads: 1 resolved (0 fixed, 1 declined with the reasoning in each thread). |
|
@codex review |
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. |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Review threads: 1 resolved (0 fixed, 1 declined with the reasoning in each thread). The Ping question no longer waits on a decision: it's in the description as a known issue before adding goes on. A design is in progress, and adding is off, so it can't happen yet. |
Release prep for v0.15.0, which ships #907. Bumps nix/package.nix and both plugin.json versions; the v0.15.0 tag goes on this commit.

Summary
Make connector state per profile, and connection guidance safe to release before the companion server change.
connect --shadow, verify, stop, then promote.auth agent connectandconnecthelp, README, rollout doc and connect skill all say so. No unconditional "each approval adds" promise, including in the skill published on merge.Companion server change: bc3 #13840, which merges after this CLI is released.
Known issue before adding goes on
With two connections of one agent, a Ping can be answered twice, or boosted twice by the acknowledgement (#918), because Pings (#914) belong to no project: serving each project from one connection doesn't keep them apart. A design is in progress. Adding is off, so it can't happen yet. Raised in the rebase note.
CLI-first rollout / rollback
The secret remains opaque; no new server endpoint or feature flag is required by this CLI. Upgrade without reconnecting:
Operator upgrade, mixed-version and downgrade runbook: an older build restarted after adoption can recreate legacy state and miss the migrated hold/cursor. Downgrade offline, choosing one profile and restoring its whole normal/shadow directories to the old names; never overwrite existing ledgers or delete other profiles' state/credentials. Server switch-off disables adding, not existing added connections; server code downgrade after own-secret issuance is unsupported.
The consumer namespace intentionally stays per agent: it is a key in each isolated local profile ledger, never a server consumer. Renaming it would orphan migrated checkpoints.
Head and validation
Head
396da982, rebased onto main at2dbca87a(v0.14.0) fromcd5c2955; what conflicted and how it was resolved is in the rebase note. Six commits:786820a201c246323a3c901c3602f08625918356396da982396da982: 27 successful, 4 skipped, 1 neutral / 32 total, none failing or pending. Skipped: Auto-merge, Benchmarks, Refresh the Nix vendorHash, Skill Evals. Neutral: cubic, which didn't review this head. Skipped and neutral checks are not test proof.make checkandgo test ./...pass at396da982, andgo vetpasses at each of the six commits (rebase note).auth agent connectandconnecthelp's add-or-replace wording, and that "always for shared" is gone.Earlier validation, at
13109a9(before the copy commit and the rebase onto v0.14.0)bin/ciexit 0, fresh temporary HOME/XDG,BASECAMP_NO_KEYRING=1. The script is onlyexec make check: a local CI pass, not a recorded GitHub signoff.Go skips: opt-in callback preview; existing SDK redirect-classification skip pending re-pin; three subprocess entrypoints (credential-lock helper, fake MCP server, crash helper); one additional nested foreign-ownership test requiring root/chown. The helper entrypoints are invoked by their parent tests, but skips themselves are not proof. BATS skips: six PowerShell installer cases because
pwshis absent, and two unimplemented URL breadcrumb cases.Source review at
13109a9: 8/8 state-reader classes rechecked, no actionable CLI finding. This is source review, not executed lifecycle/CI.Isolated lifecycle evidence — limited matrix
The lifecycle report records 28 passed / 34 planned atomic observations, 6 explicitly unexecuted. Synthetic-only real processes, HTTP, browser approvals and WebSockets; isolated HOME/XDG/keyring and loopback network. No customer credentials. These are observations from real lifecycle runs, not fake-server unit tests, and not a claim of 34/34 runtime scenarios. The table below is the CLI-relevant subset, not another aggregate denominator. These runs predate team-agent adding behind its own switch in bc3 #13840, so the rows where shared approvals replaced describe a server without that switch.
meon old server exit 0; real shared-agent approval/mint/me on integrated server. Not a separate old-server new-approval/feed proof13109a913109a9B/C reconnected and emitted 2/2 fresh post-revocation requests. Later B-only disconnect stopped its socket with disconnected-or-replaced copy while other connections survivedThe two older admitted requests were retained after migration but excluded from handoff after release by the existing
before_this_runage guard. Migration does not promise historical replay. Explicitly held records remaining held and accessible to release/discard are separately covered by automated tests.Limits: guided selection warning is source/unit-covered, not real-TTY exercised. Genuine runtime full shadow adoption/promote/import/discard/all-operator-command coverage was not executed; automated CLI tests cover these paths. Runtime status/release/adoption were exercised. The six unexecuted runtime groups are shared Adminland rotation, stale browser forms across replacement races, concurrent HTTP mint/revoke/scope interleavings, actual replica lag/roles, full shadow/operator commands, and ONCE browser/HTTP/schema downgrade. Automated server/CLI coverage is separate, not counted as lifecycle proof.
Attribution / source consolidation
Preserve the original work, not its rollout-unsafe promises:
8ead7320: profile migration/operator implementation and tests, with Jeremy Daer's October 14 review fixes.7ba7e962: safe copy/lost-handover coverage adapted for CLI-first release, flag OFF/ON and shared agents.Rob Zolkos's authorship and source provenance are preserved in the first five commits; the sixth, the copy change, is Jeremy Daer's. Existing source PRs/branches are not closed or rewritten by this consolidation.
Original-review replies
Point-by-point replacement-only replies to the latest review bodies, with pinned source/test links and explicit retained declines/human decisions:
The cross-repository comparison covered 36/36 Jeremy top-level points and their nested subfindings, plus older bot/body findings: 81 pre-reply source records (7 PR bodies, 40 reviews, 6 issue comments, 28 inline comments) and all 14 GraphQL threads (the same 28 inline comments, not extra findings). Empty bodies and a Copilot quota failure were accounted for, not counted as approvals. Rechecking those records found no further in-scope source defect. This is a disposition map, not a claim that every literal proposal was implemented or accepted.
The old switch-on CLI release prerequisite is superseded by the CLI-first plan: refusal, help, README and the skill published on merge say an approval replaces the existing connection wherever Basecamp hasn't enabled adding for the agent's kind. Unsupported mixed builds and the stop/whole-directory downgrade procedure remain explicit.