One connector per profile: key the state folder and lock by profile by robzolkos · Pull Request #901 · basecamp/basecamp-cli

robzolkos · 2026-10-13T22:09:47Z

Step 5 of the plan on Follow-up: multiple connections per agent ("Plan (final)", step 5, "One connector per profile"). Branched off main; it doesn't depend on any other step.

What changes

The connector's state folder is now $XDG_STATE_HOME/basecamp/connect/<account>-<agent>-<profile> (same under connect-shadow). It used to be <account>-<agent>. The instance lock and the ledger live inside it, so both are per profile now, and so is the feed cursor, because it's stored in that ledger.
Two profiles for the same agent on one computer can each run a connector. Running a second connector on the same profile is still refused (lock_unavailable, "another connector already runs for this profile").
Migration. The old <account>-<agent> folder is renamed to the new one by the first profile that starts and proves it is that agent. This happens after the identity check, so a profile whose credential was rotated out never takes it. The rename happens while holding the old folder's lock:
- If an older build's connector is still running there, the start is refused, as it is today.
- Running it again does nothing. A profile that already has its own folder leaves the old one alone.
- If two profiles for the agent exist at upgrade, the first one to start and verify takes the old folder. The other starts with an empty folder: it enters the feed at the present, so nothing is replayed.
Until a run adopts the old folder, status, doctor, setup's "running" check and the decisions (discard, release, import) all act on it where it is. No operator command moves it. Only a run that has verified it is the agent does.
The copy in skills/basecamp-connect/SKILL.md now says one connector per profile.

Every reader of the old `<account>-<agent>` folder

The plan listed this as unverified. A search for every caller of connectStateDir, connectStatePath, StateDirName, InstanceHolder, AcquireInstanceLock, StateRoot and ResolveStateDir turned up:

Reader	Where	Now
`basecamp connect` (run, `--shadow`, `--hold`)	`connect_run.go` `runConnect`	Early lock check on the resolved folder (it creates nothing). Adopts, then locks, after the identity check.
`connect status` / `status --shadow` (ledger and lock holder)	`connect_operator.go` `runConnectStatus`	Resolved folder
`connect discard`, `release` (`openConnectLedger`)	`connect_operator.go`	Resolved folder, never moved. Without the lock, the ledger is opened without creating it. With the lock, it is looked for again while holding it.
`connect import`	`connect_operator.go`	Same as above (it takes the lock)
`connect shadow promote`	`connect_operator.go`	The shadow side is resolved. The destination is always the profile's own folder. When the old folder exists, promote holds its lock and refuses if that folder holds a ledger or can't be inspected. It used to build the old name by hand.
`connect doctor` ledger checks	`connect_doctor.go` `ledgerChecks`	Resolved folder
Guided `connect setup` "connector running" summary	`connect_setup_guided.go` `connectorRunning`	Resolved folder, per profile
`connector.ResolveStateDir`	`ledger_dispatch.go`	Removed. It had no callers left: its only caller was the worker's MCP server, removed in #815 along with `basecamp mcp --connect-state`. `internal/mcpserver` doesn't read connector state at all. It also accepted only the old name.

Nothing else in this repo, or in the sibling agent repos on the author's machine, reads the folder.

A choice to check: the consumer namespace stays `basecamp-connect-<agent>`

The plan says to key the "cursor namespace" by profile. The cursor is now per profile because the ledger that stores it is per profile: the checkpoint store is that ledger's checkpoints table, and the server never sees the namespace. Renaming the namespace would have orphaned the adopted cursor. Intake would then log "no stored position: entering the feed at the present", which breaks "migration preserves the cursor", unless every checkpoint row were rewritten during adoption. Two profiles' connectors can never share a ledger: the same folder means the same lock, so the second one is refused. TestARunAdoptsTheLegacyStateDirectoryWithItsCursor shows the adopted run resuming from the stored position.

What it doesn't do

Old and new builds side by side for one agent. After adoption, an older build started for that agent makes a new <account>-<agent> folder and runs beside the new one. That's a downgrade case, so no new guard was added.
Mention caps across profiles. The per-24h agent-mention caps are counted in each ledger, so they now apply per profile rather than per agent. This follows from the plan's "two connections serving the same project both answer".

Tests

Run with a temporary XDG_STATE_HOME:

internal/connector/statedir_test.go
- names are one per profile
- the old folder moves with its contents and leaves no stray lock-holder file
- adopting twice changes nothing
- only one profile adopts
- a profile that already has a folder is left alone
- no old folder means nothing is created
- adoption is refused while the old lock is held
- two profiles hold their locks at once, and the same profile can only hold its lock once
internal/commands/connect_profile_state_test.go, running the real command against the fake Basecamp:
- connect -P agent and connect -P agent-b (same agent) run side by side, each with its own ledger and lock; a third connect -P agent is refused
- a run adopts the old folder and logs "resuming the feed from the stored position"; afterwards the ledger holds the old cursor and records
- a start is refused while an older connector holds the old folder, and the old folder is left in place
- status reads the old folder until a run adopts it; once adopted, no other profile sees it
- a profile that can't prove it is the agent leaves the old folder alone
also in connect_profile_state_test.go: decisions act on the old ledger without moving it, including beside an older connector; import is refused beside one; promote moves an old shadow ledger into the profile; promote is refused while the old folder holds a ledger, is locked, or can't be inspected; promote is refused beside an older shadow

Mutations, each applied to a copy of the tree with test names parsed from the output. In the first round, 7 of 7 production changes are caught:

Mutation	Caught by
Folder name ignores the profile (`main`'s behaviour)	11 tests
The run never adopts	the cursor test
Adoption skips the old lock	the connector refusal test
Readers skip the fallback	the status test
Adoption before the identity check	2 tests
The stale lock-holder file is kept	the move test
The early lock check creates the folder	2 tests

All mutations were re-run against the head 8ead7320: 11 of 11 production changes are caught. That is the seven above, plus three for promote (promoting into the old layout, skipping the old-folder check, skipping the old lock) and one for decisions moving the old folder.

Four mutations survive, and they're labelled, not counted as covered:

Race-only guards (3). These can't be reached without injecting an interleaving:
- re-checking the new folder after taking the old lock in AdoptLegacyStateDir
- re-checking the ledger after taking the lock in openConnectLedger
- opening the ledger without creating it in the lock-free decision path
Second site (1). In promote, treating a ledger-inspection error as "no ledger". The guarantee rests on the lock taken just before it, which already refuses a folder that can't be inspected.

Review record

Pi (openai-codex/gpt-6.1-sol:xhigh), local adversarial reads in the worktree. Each pass read a stated commit range:

Pass	Read	Found	Outcome
1	`f78ef234`	(a) stale old-folder path in operator commands	fixed
		(b) an older build started after adoption runs beside the new one	declined, see "What it doesn't do"
		(c) a retry after a crash between rename and directory sync skips the sync	declined: the folder moves as a unit, so a lost rename leaves the whole ledger under the old name, where the next start adopts it again. Nothing is lost or replayed.
		Keeping the namespace	Pi checked it and found no problem
2	`f78ef234..5ad7d4fa`	(a) operator commands adopting without verifying the profile	fixed by removing writer adoption
		(b) a lock-free decision could create a ledger at a stale path	fixed
3	`f78ef234..f84eacba`	two promote findings: an older connector holding the old lock before its ledger exists, and an old folder that can't be inspected	fixed in `8ead7320`
4	`f84eacba..8ead7320`	no findings

Every commit on the branch was read by at least one pass.

Copilot. One finding on f78ef234 about the lock living inside the folder being renamed. The promote-orphaning and stale-writer parts are fixed. The stable-migration-lock part is declined, with the reasons given in the thread.

make test passes in full and make lint reports 0 issues.

The connector's state folder was <account>-<agent>, so its instance lock and ledger belonged to the agent: two profiles for the same agent on one computer could not each run a connector. The folder is now <account>-<agent>-<profile> (and the same under connect-shadow), so the lock and the ledger that holds the feed cursor are per profile. A second connector on the same profile is still refused. The old <account>-<agent> folder carries over by rename to the first profile that starts and proves it is that agent, under the old folder's lock: a connector an older build is still running there keeps it, and the start is refused as before. Adopting is idempotent; a profile that already has its own folder leaves the old one alone. Until a run adopts it, status, doctor, discard, release, import, shadow promote and guided setup's running check read the old folder. ResolveStateDir had no callers left since the worker's MCP server was removed (#815), and it accepted only the old name, so it goes.

Copilot

🟡 Changes recommended

The legacy-directory lock can be recreated during a rename race, bypassing serialization and potentially orphaning promoted state.

1 open finding

Lock stable path to prevent concurrent legacy adoption · New

What changed in this PR

Keys connector state, locks, ledgers, and cursors by profile while migrating legacy agent-level state.

Changes:

Adds profile-specific state directories with legacy adoption and fallback.
Updates all connector state consumers and tests.
Revises connector guidance for per-profile locking.

[!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.

File	Description
`skills/basecamp-connect/SKILL.md`	Documents per-profile connector limits.
`internal/connector/statedir.go`	Adds state naming and migration.
`internal/connector/statedir_test.go`	Tests naming, adoption, and locking.
`internal/connector/operator_migration_test.go`	Uses profile-specific fixture paths.
`internal/connector/lock.go`	Updates lock refusal wording.
`internal/connector/lock_test.go`	Removes obsolete agent-level rationale.
`internal/connector/ledger_dispatch.go`	Removes obsolete state-path resolver.
`internal/connector/ledger_dispatch_test.go`	Removes resolver tests.
`internal/commands/connect_start_wait_test.go`	Updates state helper calls.
`internal/commands/connect_setup_guided.go`	Checks running state per profile.
`internal/commands/connect_run.go`	Resolves, adopts, and locks profile state.
`internal/commands/connect_profile_state_test.go`	Adds profile and migration integration tests.
`internal/commands/connect_operator.go`	Resolves operator state per profile.
`internal/commands/connect_operator_test.go`	Updates operator tests.
`internal/commands/connect_doctor.go`	Checks the profile-specific ledger.

🧠 Review effort: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

…was taken A start can adopt the old <account>-<agent> folder between an operator command resolving it and taking its lock. Taking the lock then recreates an empty folder, and import would apply to a new, empty ledger and report success. The ledger is looked for again under the lock.

Copilot

🟡 Changes recommended

Shadow promotion can race legacy-state adoption and mutate an unlocked replacement directory.

2 open findings

🧠 Review effort: Balanced

Discard, release, import and shadow promote resolved the old <account>-<agent> folder and acted on that path. A start adopting it in between left them holding a path that no longer named the ledger, and taking a lock there recreated an empty folder: promote could move the shadow ledger into it and orphan it. Each one now adopts the old folder for its profile first, under the old folder's lock, and acts on the profile's folder. While an older build's connector holds that lock, a decision acts on its ledger in place as before, and import and promote are refused as before. Reads (status, doctor, setup's running check) still read the old folder without moving it.

Copilot

🟡 Changes recommended

Offline operator commands can adopt legacy state without proving the selected profile still belongs to the agent.

3 open findings

🧠 Review effort: Balanced

Pi's second pass found the writer adoption in the previous commit let an operator command move the agent's history into whichever profile ran it, with no proof that profile is the agent, and that the fallback beside a held lock could still open a stale path and create a fresh ledger there. Back to one owner rule: only a run that has proved it is the agent moves the old folder. Decisions act on the folder status reads, old or new: one that takes no lock opens the ledger without creating it, so a path a start moved away from under it fails instead of making a new ledger, and one that takes the lock looks for the ledger again under it. Shadow promote moves the shadow ledger into the profile's own folder, never into the old one, and refuses while the old folder still holds the connector's ledger, as it refuses any existing ledger.

Copilot

🟡 Changes recommended

Legacy-state inspection errors are currently mistaken for absence, risking misleading results or duplicate promoted state.

4 open findings

1 resolved since last review

Prevent offline commands from adopting unverified legacy ledgers

🧠 Review effort: Balanced

Pi's third pass: promote looked only for a ledger in the old <account>-<agent> folder, so it went ahead beside an older connector that had taken its lock but not yet made its ledger, and read an old folder it could not inspect as one with no ledger. Promote now takes the old folder's lock for the whole promote when that folder exists, refuses while it is held, and refuses on anything but a clean "no ledger".

Copilot

🔵 Needs a closer look

Concurrency-sensitive state migration and cross-version locking warrant final human validation.

2 open findings

2 resolved since last review

🧠 Review effort: Balanced

robzolkos · 2026-10-13T22:58:35Z

🤖 Review summary for head 8ead7320. CI is green: 26 checks pass and 5 are skipped. The branch merges cleanly. There are 0 unresolved threads.

Which commit each reviewer read

Pi (openai-codex/gpt-6.1-sol:xhigh), local adversarial passes in the worktree, not posted to the PR:
- f78ef234: 3 findings
- f78ef234..5ad7d4fa: 2 findings
- f78ef234..f84eacba: 2 findings
- f84eacba..8ead7320: no findings
Every commit on the branch was read by at least one pass.
Copilot: reviewed every push, through 8ead7320. Its review of the head adds no new findings, and the two it still showed as open are answered and resolved below.

Fixed

Operator commands acting on a stale old-folder path (Pi 1, Copilot r4264604638, r4264690764)
Operator commands adopting without the identity check (Pi 2, r4264770462)
A lock-free decision creating a ledger at a stale path (Pi 2)
Promote beside an older connector's lock, and promote reading an uninspectable folder as having no ledger (Pi 3, r4264863635)

Declined, with reasons

A stable migration lock for two racing adopters (r4264604638). The worst case is that the losing adopter starts with an empty folder. That is the same as a second profile starting fresh, and no state is lost or shared.
Propagating non-ENOENT Lstat errors in connectStatePath (r4264863676). Both names sit in the same parent folder and go through the same permission check, so there's no input that reaches that branch.
An older build started after adoption runs beside the new one (Pi 1). This is a mixed-version or downgrade case. The guard would be a marker at the old path, and every reader and adopter would then have to special-case it.
The directory sync isn't retried after a crash between rename and sync (Pi 1). The folder moves as a unit. If a power loss undoes the rename, the whole ledger is back under the old name, and the next verified start adopts it again.

For a person to decide: the plan asked for the cursor namespace to be keyed by profile. This PR keeps basecamp-connect-<agent> instead. The cursor is still per profile, because it lives in the profile's own ledger, and renaming the namespace would orphan the adopted cursor. Pi checked this choice and found no problem; the details are in the description.

Copilot AI balanced review requested due to automatic review settings October 13, 2026 22:09

robzolkos requested a review from a team as a code owner October 13, 2026 22:09

github-actions Bot added commands CLI command implementations tests Tests (unit and e2e) skills Agent skills labels Oct 13, 2026

Copilot started reviewing on behalf of robzolkos October 13, 2026 22:10 View session