Skip to content

Keep agent connector profiles safe across server upgrades - #907

Merged
robzolkos merged 6 commits into
mainfrom
feature/agent-connections-compatible
Oct 20, 2026
Merged

robzolkos merged 6 commits into
mainfrom
feature/agent-connections-compatible

Conversation

@robzolkos

@robzolkos robzolkos commented Oct 14, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Make connector state per profile, and connection guidance safe to release before the companion server change.

  • Separate each profile's ledger, cursor, durable hold and instance lock. Two profiles for one agent can run together; a second run of the same profile is refused.
  • Adopt the whole legacy state directory only after the credential proves the saved agent identity, under the legacy lock. Preserve checkpoints, held work and operator decisions; log both migration paths and warn fresh siblings where earlier state lives. Canonical account aliases meet one new-profile lock without losing older builds' literal account spelling.
  • Keep status/doctor/offline decisions on legacy state until adoption. Refuse unverified legacy shadow promotion: first run the selected profile with connect --shadow, verify, stop, then promote.
  • Name the profile in refusals and say disconnected in Basecamp, or replaced by a newer connection. An approval may add or replace: an agent of either kind, personal or team, adds a connection once Basecamp enables adding for that kind; until then a new approval replaces the existing connection. The auth agent connect and connect help, README, rollout doc and connect skill all say so. No unconditional "each approval adds" promise, including in the skill published on merge.
  • Warn about project overlap at guided project selection and in help: requests admitted by both connections can be answered twice. Overlap remains allowed; no lease/deduplication redesign.
  • Point lost-handover cleanup at the newest connection, not Disconnect all. Do not read/delete external credential files; on an add-capable server an old file or overwritten profile credential can remain live until its connection is disconnected.

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:

  1. Stop older connectors/supervisors for this account/agent on the host.
  2. Restart its existing profile on this build before adding another profile. Check the adoption WARN, resumed cursor, hold and status.
  3. Add another connection only once Basecamp enables adding for the agent's kind, personal or team. Until then a second approval replaces the first.

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 at 2dbca87a (v0.14.0) from cd5c2955; what conflicted and how it was resolved is in the rebase note. Six commits:

Commit Author
786820a2 Rob Zolkos Isolate connector state by profile and verify legacy adoption
01c24632 Rob Zolkos Make connection guidance safe for CLI-first release
3a3c901c Rob Zolkos Prove held work survives adoption and remains operable
3602f086 Rob Zolkos Preserve legacy lock errors through diagnostics
25918356 Rob Zolkos Keep legacy account spelling while canonicalizing profile locks
396da982 Jeremy Daer Say approvals add or replace by what Basecamp enables for the agent's kind
  • GitHub checks on 396da982: 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 check and go test ./... pass at 396da982, and go vet passes at each of the six commits (rebase note).
  • The copy commit's contract test pins the auth agent connect and connect help'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/ci exit 0, fresh temporary HOME/XDG, BASECAMP_NO_KEYRING=1. The script is only exec make check: a local CI pass, not a recorded GitHub signoff.

Check Result
Go top-level 4,930 passed / 4,935 total, 0 failures, 5 skips
Go including subtests 9,265 passed / 9,271 total, 0 failures, 6 skips
E2E/BATS 490 passed / 498 total, 0 failures, 8 skips
Focused migration/copy race suite 36/36 passed, no skips, exit 0
Deliberate regression probes 5/5 caught, each mutant exit 1; clean suite exit 0
Lint / surface / skill drift / provenance / tidy Passed; lint 0 issues; 19,565 surface entries unchanged

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 pwsh is 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.

Scenario actually exercised Observation
Released v0.13.2 on the old server Real browser approval, mint/me/projects, six-check setup and live feed subscription; 2/2 synthetic assignments admitted under a durable hold
Released v0.13.1 Existing A credential me on old server exit 0; real shared-agent approval/mint/me on integrated server. Not a separate old-server new-approval/feed proof
Earlier integrated CLI before server upgrade No re-pair; cursor, hold and 2/2 admitted records migrated; status/release exit 0; same-profile second connector exit 5; fresh event emitted one request. This baseline observation preceded 13109a9
Integrated server adding OFF A freshly minted without re-pair; a second personal approval replaced the first as intended
Adding ON / shared agents Same-host new profile and separate-HOME connections; 3/3 disjoint project events each emitted one request. Same-name personal reapproval added; shared v0.13.1/new/v0.13.2 approvals still replaced
Own-secret authentication / scope Valid full-scope mint HTTP 200; cross-agent/tampered secrets rejected; refresh/introspection/revocation denied. Read-only write HTTP 403 while A retained full write access
#1 and own-row disconnect A stopped after original #1 disconnect; 13109a9 B/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 survived
Deliberate project overlap Same event emitted once on each connector, 2/2, accepted policy. JSON setup prints no human warning; built help warning verified
Adding-only rollback 4/4 existing own-secret profiles freshly minted; legacy reapproval preserved all four; fresh event admission continued
Disconnect all 5/5 legacy/own connections stopped/refused

The two older admitted requests were retained after migration but excluded from handoff after release by the existing before_this_run age 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:

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.

@robzolkos
robzolkos requested a review from a team as a code owner October 14, 2026 20:27
@robzolkos
robzolkos requested review from jeremy and a balanced review from Copilot October 14, 2026 20:27
@github-actions github-actions Bot added commands CLI command implementations tests Tests (unit and e2e) skills Agent skills auth OAuth authentication docs labels Oct 14, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@jeremy

jeremy commented Oct 17, 2026

Copy link
Copy Markdown
Member

Rob, R2-D2: I'm taking one copy fix on this branch, so please hold pushes to feature/agent-connections-compatible until I hand it back here. Basecamp will let team agents add connections too, behind their own switch (bc3 #13840), so the help, README, rollout doc and skill shouldn't say shared agents always replace.

Copilot AI balanced review requested due to automatic review settings October 17, 2026 05:39
@jeremy

jeremy commented Oct 17, 2026

Copy link
Copy Markdown
Member

Pushed cd5c2955, copy only: the auth agent connect and connect help, README, rollout doc and connect skill now say an approval adds a connection where Basecamp has enabled adding for the agent's kind, personal or team, and otherwise replaces the existing one. The contract test pins the new wording and that "always for shared" is gone. make fmt-check vet check-surface check-skill-drift test-skill-drift test-sync-skills check-naming pass, as does go test ./internal/commands. The branch is yours again.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Durable-state migration, concurrent operator access, and mixed-version rollback warrant final human validation.

No open issues 🎉

Review effort: Balanced

robzolkos and others added 6 commits October 19, 2026 23:16
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.
@jeremy
jeremy force-pushed the feature/agent-connections-compatible branch from cd5c295 to 396da98 Compare October 20, 2026 06:30
Copilot AI balanced review requested due to automatic review settings October 20, 2026 06:30
@jeremy

jeremy commented Oct 20, 2026

Copy link
Copy Markdown
Member

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 connect_operator_test.go (#879) still called connectStatePath(file, shadow). They now pass the fixture's profile. The change is folded into the first commit, where the signature changes, so every commit builds.

make check and go test ./... pass at 396da98, and go vet passes at each of the six commits.

Not changed here, a call for Rob or the operator: Pings (#914) belong to no project. Every connection of one agent that runs without --project hears them, so the advice to serve each project from one connection doesn't stop a Ping being answered twice, or boosted twice by the acknowledgement (#918). This comes from reading the code; I haven't run it. Should the overlap copy say so, or should a connection be able to opt out of Pings? That's a product decision.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Long profile names break connector state access after upgrade, and durable-state migration still warrants human review.

1 open issue

Review effort: Balanced

Comment thread internal/connector/statedir.go
@jeremy

jeremy commented Oct 20, 2026

Copy link
Copy Markdown
Member

Review threads: 1 resolved (0 fixed, 1 declined with the reasoning in each thread).
Declined: #907 (comment) — a profile name longer than about 238 characters makes the state directory's name too long. No real profile reaches that length, and the failure is loud and loses nothing.
Open for a decision: #907 (comment) — with two connections of one agent, a Ping can be answered twice. Rob or the operator decides whether the overlap copy should say so, or whether a connection should be able to opt out of Pings.

@jeremy

jeremy commented Oct 20, 2026

Copy link
Copy Markdown
Member

@codex review

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 20, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-20T06:49:11.202006Z 396da98 Manual request
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Another round soon, please!

Reviewed commit: 396da982eb

ℹ️ 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".

@robzolkos
robzolkos merged commit 1079392 into main Oct 20, 2026
33 checks passed
@robzolkos
robzolkos deleted the feature/agent-connections-compatible branch October 20, 2026 18:42
@jeremy

jeremy commented Oct 20, 2026

Copy link
Copy Markdown
Member

Review threads: 1 resolved (0 fixed, 1 declined with the reasoning in each thread).
Declined: #907 (comment) — a profile name longer than about 238 characters makes the state directory's name too long; no real profile reaches that length, and the failure is loud and loses nothing.
Not reported on the head: cubic, 396da98 — it didn't review this head because its AI review limit was reached. Reviews resume 2026-11-01.

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.

jeremy added a commit that referenced this pull request Oct 20, 2026
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auth OAuth authentication commands CLI command implementations docs skills Agent skills tests Tests (unit and e2e)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants