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.

jeremy

Reviewed at 8ead732.

Say when the old folder moves, and say where it went.
- AdoptLegacyStateDir hands the whole ledger (every filter set's cursor, the hold, held records) to the first profile that verifies, and the result is discarded.
- The other profile starts at the present and drops mentions since its last run, with nothing pointing at where they went.
- Suggest a WARN line on adoption, and on an empty-ledger start when a sibling <acct>-<agent>-* folder exists, a warning that names it and --since.
Shadow promote moves the old shadow ledger for an unverified profile. promote does no identity check. Promoting a profile whose credential was replaced strands the held ledger, and the real profile starts empty and unheld. That contradicts "only a run that has verified it is the agent". Refuse when the shadow side resolves to the old folder, with the hint "run connect --shadow -P <profile> first".
The refusal for the old folder's lock.
- An older build holding <acct>-<agent> for another profile is reported as "already runs for this profile" (early check).
- A distinct message for that case, for example "an older connector for this agent (pid N) holds ; restart it on this build, then start this profile", plus a brief retry in AdoptLegacyStateDir and connectLockFree, would also cover two profiles starting at the same moment at upgrade. A supervisor that doesn't restart on its own otherwise stays down.
Rollback. Worth adding to the decline: rolling back means stopping the connector and mv <acct>-<agent>-<profile> <acct>-<agent> first, for one profile. Otherwise the hold, the held records and the cursor are left behind, and a held agent dispatches.
The hold is per profile now. One sentence in the --hold, connect and release help would say so. The comments at lock.go:35, connect_run.go:423-424 and ledger_events.go:602-603 still say per agent. The caps' "per agent" means the mentioning agent and is still right.
Two tests:
- a working credential that reads back as another person, which leaves the old ledger in place (the current identity test fails at the mint, before Me);
- connect --shadow adopting a seeded old shadow ledger.

robzolkos · 2026-10-15T14:13:14Z

Thanks, Jeremy. Your review is carried into #907, pinned here to 13109a9413f7c2ac942e6f120a62cc663b24b330. Rob’s profile-state implementation/tests and provenance are retained; this old PR and its branch are unchanged.

Where the old folder went — applied. A verified run adopts the whole normal/shadow directory under its legacy lock; checkpoints retain their keys, held records and the durable hold remain. Adoption WARN names profile/from/to. A fresh sibling with no ledger warns where older state lives and names --since; it does not borrow that ledger. identity then adoption · adoption WARN · sibling WARN · cursor/hold/path test · sibling test. Preserving rows is not historical replay: the existing pre-run handoff age guard remains; held-work operator test separately pins individually held records.
Unverified legacy shadow promotion — applied. Offline promote refuses when shadow resolves to the legacy directory, before creating normal state. Its hint says run connect --shadow -P <profile> first, verify, stop, then promote. refusal · unverified refusal test · verified shadow adoption/promote test.
Legacy-lock wording and brief retry — applied. The legacy refusal names an older connector for this agent, folder and wrapped holder metadata, and asks to restart its existing profile on this build. Both early connectLockFree and adoption use an initial try plus three 50ms retries; error wrapping preserves lock-unavailable classification. retry/refusal · early check · legacy refusal test. No deterministic test forces release during that 150ms retry window; do not count the race suite as that proof.
Rollback — documented, not silently declined. Stop all normal/shadow connectors and supervisors; back up; choose one profile; restore its whole directories to the legacy names offline, including saved padded account spelling; never overwrite an existing ledger or remove other profiles/credentials. Old-after-adoption mixed builds remain unsafe. mixed-version warning · downgrade procedure. This reverse-rename procedure was not executed as a runtime downgrade scenario.
Per-profile hold/help/comments — applied. --hold, connect and release name the profile/own hold. The three named instance-lock/ledger comments now say profile; mentioning-agent caps retain their meaning. --hold · connect help · release help · lock comment · run comment · ledger comment · hold/help test.
Both requested tests — added. A working bearer whose Me returns another agent leaves legacy checkpoint/held work/hold in place and creates no profile directory. A verified shadow run adopts seeded legacy shadow state, resumes, stops, promotes and retains held state. wrong-person test · shadow test.

Old bot/body inventory, rechecked against integrated code:

Applied: promotion always targets the profile normal directory; existing legacy normal ledger/lock/inspection errors fail closed. Decisions use legacy fallback without adoption, recheck after locking and use a noncreating lock-free open. promotion · decision guards · inspection guard · uninspectable-folder test.
Old decline superseded: non-ENOENT profile/legacy lookup errors now propagate; do not repeat the old same-parent permission argument. resolver. Direct source proof, no dedicated injected legacy-only lookup-error test.
Retained declines: no extra stable migration lock for two verified adopters—the post-rename loser can obtain only empty state, while each canonical profile lock still excludes duplicate same-profile runs. The reported promote/stale-writer hazards are fixed independently. No persistent crash marker: the directory moves as a unit; a rolled-back rename is re-adopted. Neither argument is claimed as deterministic interleaving/power-loss test proof. adoption/locks · sequential single-adopter test.
Namespace choice retained for review: basecamp-connect-<agent> keys live in isolated local profile ledgers, not server consumers; renaming would orphan adopted checkpoints. namespace · resume test. Independent mention-cap counts follow independent ledgers; no cross-profile lease/dedup/global-cap redesign. Mixed-build risk is handled by the explicit stop/restore procedure, not an old-binary exclusion guarantee.
Still applicable evidence/process limits: fresh human validation remains pending. The old mutation survivors (three race-only schedules and one second-site inspection error) are not retroactively counted as proven by an ordinary -race run. Historical resolved thread state is not fresh reviewer acceptance.

No missing in-scope implementation fix was found in this comparison. Existing exact-head CI remains unchanged; no tests were rerun and LiveLab was untouched. Fresh Copilot review is quota-blocked, not completed. These proposed dispositions do not resolve old reviews or clear companion-server Ops/activation gates.

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