From 51fca73594f9c91bcbb0f1be4f92f9de3a15427d Mon Sep 17 00:00:00 2001 From: Rob Zolkos Date: Wed, 14 Oct 2026 15:50:43 -0400 Subject: [PATCH 1/6] Isolate connector state by profile and verify legacy adoption MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reuse Rob Zolkos’s profile migration and operator coverage from CLI #901 at 8ead7320 (f78ef234 through 8ead7320). 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. --- docs/agent-connections-rollout.md | 109 +++++ internal/commands/connect_doctor.go | 2 +- internal/commands/connect_operator.go | 75 +++- internal/commands/connect_operator_test.go | 16 +- .../commands/connect_profile_review_test.go | 74 +++ .../commands/connect_profile_state_test.go | 421 ++++++++++++++++++ internal/commands/connect_run.go | 118 ++++- internal/commands/connect_setup_guided.go | 6 +- internal/commands/connect_start_wait_test.go | 4 +- internal/connector/ledger_dispatch.go | 95 ---- internal/connector/ledger_dispatch_test.go | 41 +- internal/connector/lock.go | 16 +- internal/connector/lock_test.go | 2 - internal/connector/operator_migration_test.go | 4 +- internal/connector/statedir.go | 84 ++++ internal/connector/statedir_test.go | 142 ++++++ skills/basecamp-connect/SKILL.md | 4 +- 17 files changed, 1012 insertions(+), 201 deletions(-) create mode 100644 docs/agent-connections-rollout.md create mode 100644 internal/commands/connect_profile_review_test.go create mode 100644 internal/commands/connect_profile_state_test.go create mode 100644 internal/connector/statedir.go create mode 100644 internal/connector/statedir_test.go diff --git a/docs/agent-connections-rollout.md b/docs/agent-connections-rollout.md new file mode 100644 index 000000000..c90579dd4 --- /dev/null +++ b/docs/agent-connections-rollout.md @@ -0,0 +1,109 @@ +# Agent connections: CLI-first rollout and rollback + +This CLI can merge and release before the server changes. It stores the +connection secret as opaque data; no connection API, self-disconnect endpoint, +or server feature switch is required. A second approval **may replace the +existing connection**: it does on the current server, for shared agents, and +for personal agents while adding connections is disabled. Only personal agents +on a server with adding enabled gain another independent connection. Consult +the approval page; do not reconnect merely to upgrade the CLI. + +## Upgrade one host + +1. Stop **all older connectors for the same account/agent on this host**, normal + and shadow, including supervisors that could restart them. Keep credentials, + profiles and `connect.json` unchanged. Do not add a second profile yet. +2. Start the existing profile with this build (`basecamp connect -P `). + After its credential proves the saved identity, the first verified run + renames `-` to `--` under the old + instance lock. A WARN names both paths. The entire ledger moves: all filter + checkpoints, durable hold, held records, decisions and connection history. + A held connector stays held; migration never calls release. +3. Check `basecamp connect status -P ` and `basecamp connect doctor + -P `. Only if intended, use `basecamp connect release -P `; + release clears this profile's hold, not other profiles' holds. Held records + still require individual decisions. +4. For a legacy shadow, run `basecamp connect --shadow -P ` first, + then stop it. Only then may `basecamp connect shadow promote -P ` + move that profile's shadow ledger into normal state. Offline promotion + refuses the unverified legacy shadow instead of assigning it to a stale + profile. Normal state must not already have a ledger. +5. After server support is confirmed, add other profiles with distinct device + names and project lists. Each profile has its own ledger, cursor and lock. + A second run of the same profile is refused. Deliberate project overlap is + allowed; requests admitted by both can be answered twice. There is no lease + or cross-profile claim. + +State is under `$XDG_STATE_HOME/basecamp/connect` and `connect-shadow` (default +`~/.local/state/basecamp`). Before adoption, status, doctor and operator commands +read the legacy folder without moving it. A working credential that reads back +as a different person cannot adopt. The first verified profile owns the old +state, not necessarily the first profile configured locally. A fresh sibling +starts at the present unless `--since ` is supplied, and warns where +older state remains. `--since` can replay work; inspect status before choosing +it. If an existing profile already has its own folder, migration does not +replace it or merge ledgers; inspect any remaining legacy state separately. + +The consumer namespace intentionally remains `basecamp-connect-` (and +its shadow counterpart). Checkpoints are local rows in the **profile's isolated +ledger**, not server-side consumers. Changing the namespace would orphan the +adopted cursor; independent ledgers isolate identical keys. + +## Mixed builds are not a safe steady state + +After adoption an older build can recreate the old folder and run beside the +new one. Its lock cannot see the new profile folder. Its fresh ledger would +lose the old hold and cursor, and could dispatch duplicate work. Never restart +an older supervisor while new connectors for that agent run on the same state +root. Stop old builds before adoption; upgrade all launch commands. Distinct +hosts/state roots remain independent, as before. + +## CLI downgrade (one chosen profile, offline) + +Stop every connector/supervisor for this account/agent in this state root. Back +up the entire normal and shadow state directories with their permissions (they +contain sensitive request data). Choose **one** profile for the old build; +older builds cannot isolate several profiles for one agent. Ensure neither +legacy destination exists. If one exists, stop: inspect and reconcile manually, +never overwrite or merge SQLite ledgers. + +For each applicable group (`connect`, `connect-shadow`), rename the **whole** +`--` directory back to `-`. +Do not move just `ledger.db`: journals and migration/promotion state belong to +the same directory. Verify ownership/private permissions. Leave all other +profile directories intact and stopped; do not delete them. Start only the +chosen profile on the older binary and check status/doctor before releasing +any hold. Downgrading without restoring the directory silently leaves held +work/cursors behind and is unsafe. Preserve config/credentials unchanged. + +This is a CLI-state rollback, not a server-code rollback. Once own-secret +connections exist, server code predating their authentication is unsupported. +Server switch-off disables **adding** while keeping already-added connections +live; use that switch for server rollback, not a casual code revert. + +## Credentials left outside the CLI + +Never read or delete a user's `~/.basecamp-agent/credentials.json` during setup. +On an add-capable personal agent it can remain a live secret alongside this +profile; reconnecting a profile can likewise leave its previous connection +live. If the owner confirms an old credential is no longer used, have them +disconnect **that connection** in Basecamp and then remove their unused file. +Do not direct them to Disconnect all. Shared connections are managed in +Adminland, personal ones on My > Agent. A lost one-shot handover can be cleaned +up later by disconnecting the newest connection; its secret was not retained. + +## Provenance and proof boundaries + +Profile migration/operator fixes and tests reuse Rob Zolkos's +[CLI #901](https://github.com/basecamp/basecamp-cli/pull/901) at `8ead7320`. +Connection copy and lost-handover coverage reuse his +[CLI #900](https://github.com/basecamp/basecamp-cli/pull/900) at `7ba7e962`, adapted +for CLI-first release and Jeremy Daer's October 14 review. The per-connection +logout/revoke guidance from Jeremy's [#851](https://github.com/basecamp/basecamp-cli/pull/851) +is already in the base. + +Unit tests use synthetic fake servers and isolated local state. They prove +identity-before-adoption, hold/cursor preservation, sibling isolation, lock +refusal, operator fallback and safe copy—not real server lifecycle behavior. +Real old/new CLI and server flag-off/on HTTP/OAuth/feed scenarios must be run +in the separate isolated integration harness before enabling server adds. diff --git a/internal/commands/connect_doctor.go b/internal/commands/connect_doctor.go index 67609d173..29c70c4f2 100644 --- a/internal/commands/connect_doctor.go +++ b/internal/commands/connect_doctor.go @@ -124,7 +124,7 @@ func feedCheck(ctx context.Context, account *basecamp.AccountClient) setup.Check // gaps with their epoch ids, losses, the hold, and lifecycle messages that // wait for a decision. It reads the ledger read-only. func ledgerChecks(ctx context.Context, p connectProfile) []setup.Check { - dir, err := connectStatePath(p.file, false) + dir, err := connectStatePath(p.name, p.file, false) if err != nil { return []setup.Check{{Name: "Ledger", Status: setup.StatusFail, Message: errorMessage(err)}} } diff --git a/internal/commands/connect_operator.go b/internal/commands/connect_operator.go index ad4f71f19..af42db8ed 100644 --- a/internal/commands/connect_operator.go +++ b/internal/commands/connect_operator.go @@ -97,7 +97,7 @@ func parseEventIDArg(raw string) (int64, error) { // metadata beside it is written best-effort and says nothing on its own. func openConnectLedger(ctx context.Context, p connectProfile, requireStopped bool) (*connector.Ledger, func(), error) { done := func() {} - dir, err := connectStatePath(p.file, false) + dir, err := connectStatePath(p.name, p.file, false) if err != nil { return nil, done, err } @@ -132,8 +132,17 @@ func openConnectLedger(ctx context.Context, p connectProfile, requireStopped boo return nil, done, lockErr } done = func() { _ = lock.Release() } + if _, err := os.Lstat(path); err != nil { + done() + return nil, func() {}, output.ErrUsageHint(fmt.Sprintf("Profile %q's connector state moved while this command started", p.name), "Run the command again.") + } + } + var ledger *connector.Ledger + if requireStopped || outOfDate { + ledger, err = connector.OpenLedger(path) //nolint:contextcheck // OpenLedger migrates on its own context + } else { + ledger, err = connector.OpenExistingLedger(ctx, path) } - ledger, err := connector.OpenLedger(path) //nolint:contextcheck // OpenLedger migrates on its own context if err != nil { done() return nil, func() {}, err @@ -205,7 +214,7 @@ func runConnectStatus(cmd *cobra.Command, shadow bool) error { if err != nil { return err } - dir, err := connectStatePath(p.file, shadow) + dir, err := connectStatePath(p.name, p.file, shadow) if err != nil { return err } @@ -457,7 +466,7 @@ func newConnectReleaseCmd() *cobra.Command { return &cobra.Command{ Use: "release", Short: "Clear the hold: requests are handed off again", - Long: `Clear the durable hold that basecamp connect --hold or shadow promote set. + Long: `Clear this profile's durable hold that basecamp connect --hold or shadow promote set. Records that arrived after the hold are handed off; held records stay held until each is discarded.`, Example: ` basecamp connect release -P agent`, @@ -505,7 +514,10 @@ shadow or a held ledger; run promote again to finish. Start the connector afterwards: intake continues from the promoted position, nothing is handed off until basecamp connect release, and held records wait -to be discarded.`, +to be discarded. The hold and records belong to the selected profile. + +An old, agent-wide shadow ledger must first be adopted by a verified run: +basecamp connect --shadow -P . Stop that run before promoting.`, Example: ` basecamp connect shadow promote -P agent`, Args: cobra.NoArgs, RunE: func(cmd *cobra.Command, _ []string) error { @@ -513,25 +525,34 @@ to be discarded.`, if err != nil { return err } - shadowDir, err := connectStatePath(p.file, true) + stateHome, err := connectStateHome() if err != nil { return err } - stateHome, err := connectStateHome() + shadowDir, err := connectStatePath(p.name, p.file, true) if err != nil { return err } + if filepath.Base(shadowDir) == connector.LegacyStateDirName(p.file.AccountID, p.file.Agent.PersonID) { + return output.ErrUsageHint(fmt.Sprintf("Profile %q has not verified ownership of the legacy shadow ledger", p.name), + "Run basecamp connect --shadow -P "+richtext.ShellQuote(p.name)+" first, then stop it before promoting.") + } parent, err := ensurePrivateChain(stateHome, "basecamp", "connect") if err != nil { return output.ErrUsage("The connector's state directory cannot be used: " + err.Error()) } - res, err := connector.PromoteShadow(cmd.Context(), connector.PromoteOptions{ - ShadowDir: shadowDir, - StateDir: filepath.Join(parent, connector.StateDirName(p.file.AccountID, p.file.Agent.PersonID)), - AccountID: p.file.AccountID, - AgentID: p.file.Agent.PersonID, - By: operatorName(), - }) + var res connector.PromoteResult + release, err := lockLegacyStateWithoutLedger(filepath.Join(parent, connector.LegacyStateDirName(p.file.AccountID, p.file.Agent.PersonID)), p.file) + if err == nil { + defer release() + res, err = connector.PromoteShadow(cmd.Context(), connector.PromoteOptions{ + ShadowDir: shadowDir, + StateDir: filepath.Join(parent, connector.StateDirName(p.file.AccountID, p.file.Agent.PersonID, p.name)), + AccountID: p.file.AccountID, + AgentID: p.file.Agent.PersonID, + By: operatorName(), + }) + } switch { case errors.Is(err, connector.ErrAlreadyRunning): return &output.Error{Code: output.CodeLockUnavailable, Message: err.Error(), @@ -551,6 +572,30 @@ to be discarded.`, return cmd } +func lockLegacyStateWithoutLedger(legacy string, file setup.File) (func(), error) { + switch _, err := os.Lstat(legacy); { + case errors.Is(err, os.ErrNotExist): + return func() {}, nil + case err != nil: + return nil, err + } + lock, err := connector.AcquireLegacyInstanceLock(legacy, file.AccountID, file.Agent.PersonID) + if err != nil { + return nil, err + } + ledger := filepath.Join(legacy, connector.LedgerFile) + switch _, err := os.Lstat(ledger); { + case err == nil: + err = fmt.Errorf("connector: %s: %w", ledger, connector.ErrLedgerExists) + _ = lock.Release() + return nil, err + case !errors.Is(err, os.ErrNotExist): + _ = lock.Release() + return nil, err + } + return func() { _ = lock.Release() }, nil +} + // --- import --------------------------------------------------------------- // maxReconciliationBytes bounds a reconciliation file. @@ -583,7 +628,7 @@ The file is JSON: if err != nil { return output.ErrUsage(strings.TrimPrefix(err.Error(), "connector: ")) } - dir, err := connectStatePath(p.file, false) + dir, err := connectStatePath(p.name, p.file, false) if err != nil { return err } diff --git a/internal/commands/connect_operator_test.go b/internal/commands/connect_operator_test.go index 8719715c3..8db8a2eb6 100644 --- a/internal/commands/connect_operator_test.go +++ b/internal/commands/connect_operator_test.go @@ -57,7 +57,7 @@ func newOperatorFixture(t *testing.T) operatorFixture { // record 1 and a blocked record 2, and returns it open. func (f operatorFixture) ledger(t *testing.T, shadow bool) *connector.Ledger { t.Helper() - dir, err := connectStateDir(f.file, shadow) + dir, err := connectStateDir("agent", f.file, shadow) require.NoError(t, err) l, err := connector.OpenLedger(filepath.Join(dir, connector.LedgerFile)) require.NoError(t, err) @@ -155,7 +155,7 @@ func TestConnectDiscardAndRelease(t *testing.T) { require.NoError(t, err, out) assert.Contains(t, out, "Released") - dir, err := connectStatePath(f.file, false) + dir, err := connectStatePath("agent", f.file, false) require.NoError(t, err) l, err = connector.OpenLedgerReadOnly(context.Background(), filepath.Join(dir, connector.LedgerFile)) require.NoError(t, err) @@ -189,7 +189,7 @@ func TestConnectShadowPromoteAndImport(t *testing.T) { require.Error(t, err) assert.Equal(t, output.CodeUsage, usageError(t, err).Code) - dir, err := connectStatePath(f.file, false) + dir, err := connectStatePath("agent", f.file, false) require.NoError(t, err) lock, err := connector.AcquireInstanceLock(dir, f.file.AccountID, f.file.Agent.PersonID, time.Now()) require.NoError(t, err) @@ -301,7 +301,7 @@ func TestFakeMCPServer(t *testing.T) { func TestOperatorCommandsDoNotMigrateUnderARunningConnector(t *testing.T) { f := newOperatorFixture(t) require.NoError(t, f.ledger(t, false).Close()) - dir, err := connectStatePath(f.file, false) + dir, err := connectStatePath("agent", f.file, false) require.NoError(t, err) // A ledger an older binary wrote: its last migration is not recorded. @@ -325,7 +325,7 @@ func TestOperatorCommandsDoNotMigrateUnderARunningConnector(t *testing.T) { func TestTheMigrationGuardIsTheLockNotItsMetadata(t *testing.T) { f := newOperatorFixture(t) require.NoError(t, f.ledger(t, false).Close()) - dir, err := connectStatePath(f.file, false) + dir, err := connectStatePath("agent", f.file, false) require.NoError(t, err) db, err := sql.Open("sqlite", filepath.Join(dir, connector.LedgerFile)) require.NoError(t, err) @@ -348,7 +348,7 @@ func TestTheMigrationGuardIsTheLockNotItsMetadata(t *testing.T) { func TestImportRunsOnALedgerOlderThanTheBuild(t *testing.T) { f := newOperatorFixture(t) require.NoError(t, f.ledger(t, false).Close()) - dir, err := connectStatePath(f.file, false) + dir, err := connectStatePath("agent", f.file, false) require.NoError(t, err) db, err := sql.Open("sqlite", filepath.Join(dir, connector.LedgerFile)) require.NoError(t, err) @@ -365,7 +365,7 @@ func TestImportRunsOnALedgerOlderThanTheBuild(t *testing.T) { // itself over the lock it holds. _, err = f.run(t, output.FormatJSON, "import", file) if err != nil { - assert.NotContains(t, err.Error(), "already holds this account") + assert.NotContains(t, err.Error(), "already runs for this profile") assert.NotContains(t, err.Error(), "Stop the connector") return } @@ -396,7 +396,7 @@ func TestDoctorRefusesAShadowingToken(t *testing.T) { require.Error(t, err, args[0]) assert.Contains(t, err.Error(), "BASECAMP_TOKEN", args[0]) } - dir, err := connectStatePath(f.file, false) + dir, err := connectStatePath("agent", f.file, false) require.NoError(t, err) db, err := sql.Open("sqlite", filepath.Join(dir, connector.LedgerFile)) require.NoError(t, err) diff --git a/internal/commands/connect_profile_review_test.go b/internal/commands/connect_profile_review_test.go new file mode 100644 index 000000000..f7318512c --- /dev/null +++ b/internal/commands/connect_profile_review_test.go @@ -0,0 +1,74 @@ +//go:build unix + +package commands + +import ( + "os" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/basecamp/basecamp-cli/internal/connector" + "github.com/basecamp/basecamp-cli/internal/output" +) + +func TestAVerifiedWrongPersonCannotAdoptLegacyState(t *testing.T) { + f := newOperatorFixture(t) + legacy := seedLegacyLedger(t, f.s, f.file) + f.s.agentID = setupPeerAgent // working bearer, but Me proves a different agent + r := startProfileConnector(t, f.s, "agent") + err := r.awaitExit(t) + require.Error(t, err) + assert.Contains(t, err.Error(), "connect.json") + assertLegacyLedgerKept(t, legacy, f.s, f.file) + _, err = os.Lstat(filepath.Join(connectStateRoot(t), connector.StateDirName(f.file.AccountID, f.file.Agent.PersonID, "agent"))) + assert.ErrorIs(t, err, os.ErrNotExist) +} + +func TestAShadowRunAdoptsAndPromotesItsHeldLegacyState(t *testing.T) { + f := newOperatorFixture(t) + legacy := seedLegacyLedger(t, f.s, f.file, true) + r := startProfileConnector(t, f.s, "agent", "--shadow") + r.awaitRunning(t) + require.Eventually(t, func() bool { return strings.Contains(r.out.String(), "resuming the feed from the stored position") }, 10*time.Second, 20*time.Millisecond) + assert.Contains(t, r.out.String(), "adopted legacy state") + r.cancel() + _ = r.awaitExit(t) + shadow := filepath.Join(filepath.Dir(connectStateRoot(t)), "connect-shadow", connector.StateDirName(f.file.AccountID, f.file.Agent.PersonID, "agent")) + assertLegacyLedgerKept(t, shadow, f.s, f.file, true) + _, err := os.Lstat(legacy) + assert.ErrorIs(t, err, os.ErrNotExist) + out, err := f.run(t, output.FormatJSON, "shadow", "promote") + require.NoError(t, err, out) + dir := filepath.Join(connectStateRoot(t), connector.StateDirName(f.file.AccountID, f.file.Agent.PersonID, "agent")) + assertLegacyLedgerKept(t, dir, f.s, f.file, true) +} + +func TestAFreshSiblingNamesWhereTheEarlierHeldStateLives(t *testing.T) { + f := newOperatorFixture(t) + setUpSecondProfile(t, f.s, "agent-b") + seedLegacyLedger(t, f.s, f.file) + a := startProfileConnector(t, f.s, "agent") + a.awaitRunning(t) + b := startProfileConnector(t, f.s, "agent-b") + b.awaitRunning(t) + assert.Contains(t, b.out.String(), "this profile has no ledger") + assert.Contains(t, b.out.String(), "--since") + assert.Contains(t, b.out.String(), "sibling="+filepath.Join(connectStateRoot(t), connector.StateDirName(f.file.AccountID, f.file.Agent.PersonID, "agent"))) +} + +func TestStatePathsRefuseInvalidProfilesBeforeCreatingAnything(t *testing.T) { + f := newOperatorFixture(t) + for _, profile := range []string{"", "../escape", "a/b", "a\\b", ".", "profile name"} { + _, err := connectStatePath(profile, f.file, false) + require.Error(t, err, profile) + _, err = connectStateDir(profile, f.file, false) + require.Error(t, err, profile) + } + _, err := os.Lstat(connectStateRoot(t)) + assert.ErrorIs(t, err, os.ErrNotExist) +} diff --git a/internal/commands/connect_profile_state_test.go b/internal/commands/connect_profile_state_test.go new file mode 100644 index 000000000..456e40df5 --- /dev/null +++ b/internal/commands/connect_profile_state_test.go @@ -0,0 +1,421 @@ +//go:build unix + +package commands + +import ( + "context" + "fmt" + "os" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/basecamp/basecamp-sdk/go/pkg/basecamp/eventfeed" + + "github.com/basecamp/basecamp-cli/internal/appctx" + "github.com/basecamp/basecamp-cli/internal/connector" + "github.com/basecamp/basecamp-cli/internal/connector/setup" + "github.com/basecamp/basecamp-cli/internal/output" +) + +type runningConnector struct { + out *lockedBuffer + done chan error + cancel context.CancelFunc +} + +func startProfileConnector(t *testing.T, s *connectSetupServer, profile string, args ...string) runningConnector { + t.Helper() + app := newConnectSetupApp(t, s, profile) + ctx, cancel := context.WithCancel(t.Context()) + out := &lockedBuffer{} + app.Output = output.New(output.Options{Format: output.FormatJSON, Writer: out}) + cmd := NewConnectCmd() + cmd.SetArgs(args) + cmd.SetContext(appctx.WithApp(ctx, app)) + cmd.SetOut(out) + cmd.SetErr(out) + cmd.SilenceErrors, cmd.SilenceUsage = true, true + r := runningConnector{out: out, done: make(chan error, 1), cancel: cancel} + go func() { r.done <- cmd.Execute() }() + t.Cleanup(func() { + cancel() + select { + case <-r.done: + case <-time.After(10 * time.Second): + t.Errorf("the connector for %s did not stop when canceled", profile) + } + }) + return r +} + +func (r runningConnector) awaitRunning(t *testing.T) { + t.Helper() + deadline := time.After(15 * time.Second) + for !strings.Contains(r.out.String(), "connector: running") { + select { + case err := <-r.done: + r.done <- err + t.Fatalf("the connector exited before it ran: %v\n%s", err, r.out.String()) + case <-deadline: + t.Fatalf("the connector never ran: %s", r.out.String()) + case <-time.After(20 * time.Millisecond): + } + } +} + +func (r runningConnector) awaitExit(t *testing.T) error { + t.Helper() + select { + case err := <-r.done: + r.done <- err + return err + case <-time.After(15 * time.Second): + t.Fatalf("the connector kept running: %s", r.out.String()) + return nil + } +} + +func setUpSecondProfile(t *testing.T, s *connectSetupServer, profile string) setup.File { + t.Helper() + out, err := runAgentConnect(t, newConnectSetupApp(t, s, profile)) + require.NoError(t, err, out) + out, err = runConnectSetupCmd(t, newConnectSetupApp(t, s, profile), "--operator", fmt.Sprint(setupOperatorPerson), serveArg()) + require.NoError(t, err, out) + file, err := setup.Load(connectSetupPath(t, profile)) + require.NoError(t, err) + return file +} + +func connectStateRoot(t *testing.T) string { + t.Helper() + root, err := connector.StateRoot() + require.NoError(t, err) + return root +} + +func connectRunKey(t *testing.T, s *connectSetupServer, file setup.File, shadow ...bool) eventfeed.CheckpointKey { + t.Helper() + origin, err := eventfeed.CanonicalOrigin(s.srv.URL) + require.NoError(t, err) + filters := eventfeed.Filters{ExcludePerformers: []int64{file.Agent.PersonID}, ActorTypes: []string{"person"}} + return eventfeed.CheckpointKey{Origin: origin, AccountID: file.AccountID, + ConsumerNamespace: connectConsumerNamespace(file.Agent.PersonID, len(shadow) > 0 && shadow[0]), FilterKey: filters.FilterKey()} +} + +const legacyPosition = "legacy-position-41" + +func seedLegacyLedger(t *testing.T, s *connectSetupServer, file setup.File, shadow ...bool) string { + t.Helper() + stateHome, err := connectStateHome() + require.NoError(t, err) + group := "connect" + if len(shadow) > 0 && shadow[0] { + group = "connect-shadow" + } + legacy, err := ensurePrivateChain(stateHome, "basecamp", group, connector.LegacyStateDirName(file.AccountID, file.Agent.PersonID)) + require.NoError(t, err) + l, err := connector.OpenLedger(filepath.Join(legacy, connector.LedgerFile)) + require.NoError(t, err) + ctx := context.Background() + _, err = l.RecordSeen(ctx, eventfeed.Event{ID: 41, Kind: "comment_created", EventType: "comment.created", Action: "created", + CreatedAt: time.Now(), BucketID: setupProject, CreatorID: setupOperatorPerson, RecordingID: 77}, connector.LanePoll) + require.NoError(t, err) + require.NoError(t, l.Save(ctx, connectRunKey(t, s, file, shadow...), legacyPosition)) + _, err = l.SetHold(ctx, "local:tester", connector.HoldByOperator) + require.NoError(t, err) + require.NoError(t, l.Close()) + return legacy +} + +func assertLegacyLedgerKept(t *testing.T, dir string, s *connectSetupServer, file setup.File, shadow ...bool) { + t.Helper() + l, err := connector.OpenLedgerReadOnly(context.Background(), filepath.Join(dir, connector.LedgerFile)) + require.NoError(t, err) + defer func() { _ = l.Close() }() + position, ok, err := l.Load(context.Background(), connectRunKey(t, s, file, shadow...)) + require.NoError(t, err) + require.True(t, ok, "the cursor came with the directory") + assert.Equal(t, legacyPosition, position) + _, found, err := l.Get(context.Background(), 41) + require.NoError(t, err) + assert.True(t, found, "the ledger's records came with it") + _, held, err := l.HoldMarker(context.Background()) + require.NoError(t, err) + assert.True(t, held, "migration must never release the hold") +} + +func TestTwoProfilesForOneAgentEachRunAConnector(t *testing.T) { + f := newOperatorFixture(t) + other := setUpSecondProfile(t, f.s, "agent-b") + require.Equal(t, f.file.Agent.PersonID, other.Agent.PersonID, "both profiles are the same agent") + require.Equal(t, f.file.AccountID, other.AccountID) + + a := startProfileConnector(t, f.s, "agent") + a.awaitRunning(t) + b := startProfileConnector(t, f.s, "agent-b") + b.awaitRunning(t) + + root := connectStateRoot(t) + for _, profile := range []string{"agent", "agent-b"} { + dir := filepath.Join(root, connector.StateDirName(f.file.AccountID, f.file.Agent.PersonID, profile)) + _, err := os.Lstat(filepath.Join(dir, connector.LedgerFile)) + require.NoError(t, err, "%s runs on a ledger of its own", profile) + holder, ok := connector.InstanceHolder(dir, f.file.AccountID, f.file.Agent.PersonID) + require.True(t, ok, "%s holds the lock in its own state directory", profile) + assert.Equal(t, os.Getpid(), holder.PID) + } + + again := startProfileConnector(t, f.s, "agent") + err := again.awaitExit(t) + require.Error(t, err) + assert.Equal(t, output.CodeLockUnavailable, usageError(t, err).Code, again.out.String()) + assert.Contains(t, err.Error(), "already runs for this profile") +} + +func TestARunAdoptsTheLegacyStateDirectoryWithItsCursor(t *testing.T) { + f := newOperatorFixture(t) + root := connectStateRoot(t) + legacy := seedLegacyLedger(t, f.s, f.file) + + r := startProfileConnector(t, f.s, "agent") + r.awaitRunning(t) + require.Eventually(t, func() bool { return strings.Contains(r.out.String(), "resuming the feed from the stored position") }, + 10*time.Second, 20*time.Millisecond, "the run did not resume from the old cursor: %s", r.out.String()) + + dir := filepath.Join(root, connector.StateDirName(f.file.AccountID, f.file.Agent.PersonID, "agent")) + assert.Contains(t, r.out.String(), "state="+dir) + assert.Contains(t, r.out.String(), "adopted legacy state") + assert.Contains(t, r.out.String(), "from="+legacy) + assert.Contains(t, r.out.String(), "to="+dir) + assert.Contains(t, r.out.String(), "the hold stands") + _, err := os.Lstat(legacy) + assert.ErrorIs(t, err, os.ErrNotExist, "the old directory moved rather than being copied") + r.cancel() + _ = r.awaitExit(t) + + assertLegacyLedgerKept(t, dir, f.s, f.file) +} + +func TestAStartRefusesWhileAnOlderConnectorHoldsTheLegacyDirectory(t *testing.T) { + f := newOperatorFixture(t) + root := connectStateRoot(t) + legacy := seedLegacyLedger(t, f.s, f.file) + lock, err := connector.AcquireInstanceLock(legacy, f.file.AccountID, f.file.Agent.PersonID, time.Now()) + require.NoError(t, err) + t.Cleanup(func() { _ = lock.Release() }) + + r := startProfileConnector(t, f.s, "agent") + err = r.awaitExit(t) + require.Error(t, err) + assert.Equal(t, output.CodeLockUnavailable, usageError(t, err).Code, r.out.String()) + assert.Contains(t, err.Error(), "an older connector for this agent") + assert.Contains(t, err.Error(), legacy) + _, err = os.Lstat(filepath.Join(legacy, connector.LedgerFile)) + require.NoError(t, err, "a running older connector keeps its directory") + _, err = os.Lstat(filepath.Join(root, connector.StateDirName(f.file.AccountID, f.file.Agent.PersonID, "agent"))) + assert.ErrorIs(t, err, os.ErrNotExist) +} + +func TestOperatorCommandsReadTheLegacyDirectoryUntilARunAdoptsIt(t *testing.T) { + f := newOperatorFixture(t) + root := connectStateRoot(t) + legacy := seedLegacyLedger(t, f.s, f.file) + + dir, err := connectStatePath("agent", f.file, false) + require.NoError(t, err) + assert.Equal(t, legacy, dir) + out, err := f.run(t, output.FormatJSON, "status") + require.NoError(t, err, out) + assert.Contains(t, out, `"profile": "agent"`) + _, err = os.Lstat(filepath.Join(root, connector.StateDirName(f.file.AccountID, f.file.Agent.PersonID, "agent"))) + assert.ErrorIs(t, err, os.ErrNotExist, "reading the state moves nothing") + + adopted, err := connectStateDir("agent", f.file, false) + require.NoError(t, err) + assert.Equal(t, filepath.Join(root, connector.StateDirName(f.file.AccountID, f.file.Agent.PersonID, "agent")), adopted) + dir, err = connectStatePath("agent", f.file, false) + require.NoError(t, err) + assert.Equal(t, adopted, dir) + + other, err := connectStatePath("agent-b", f.file, false) + require.NoError(t, err) + assert.Equal(t, filepath.Join(root, connector.StateDirName(f.file.AccountID, f.file.Agent.PersonID, "agent-b")), other, + "once adopted, the old directory is no other profile's") +} + +func TestAProfileThatCannotProveItIsTheAgentLeavesTheLegacyDirectory(t *testing.T) { + f := newOperatorFixture(t) + legacy := seedLegacyLedger(t, f.s, f.file) + app := newConnectSetupApp(t, f.s, "agent") + creds, err := app.Auth.GetStore().Load(app.Auth.CredentialKey()) + require.NoError(t, err) + creds.ExpiresAt = time.Now().Add(-time.Minute).Unix() + require.NoError(t, app.Auth.GetStore().Save(app.Auth.CredentialKey(), creds)) + f.s.mu.Lock() + f.s.refuseSecret = true + f.s.mu.Unlock() + + r := startProfileConnector(t, f.s, "agent") + err = r.awaitExit(t) + require.Error(t, err) + assert.Contains(t, err.Error(), "disconnected", r.out.String()) + _, err = os.Lstat(filepath.Join(legacy, connector.LedgerFile)) + require.NoError(t, err, "the old directory waits for the profile that is the agent") + _, err = os.Lstat(filepath.Join(connectStateRoot(t), connector.StateDirName(f.file.AccountID, f.file.Agent.PersonID, "agent"))) + assert.ErrorIs(t, err, os.ErrNotExist) +} + +func openLegacyLedger(t *testing.T, file setup.File, group string) (string, *connector.Ledger) { + t.Helper() + stateHome, err := connectStateHome() + require.NoError(t, err) + legacy, err := ensurePrivateChain(stateHome, "basecamp", group, connector.LegacyStateDirName(file.AccountID, file.Agent.PersonID)) + require.NoError(t, err) + l, err := connector.OpenLedger(filepath.Join(legacy, connector.LedgerFile)) + require.NoError(t, err) + return legacy, l +} + +func heldLegacyLedger(t *testing.T, file setup.File) string { + t.Helper() + legacy, l := openLegacyLedger(t, file, "connect") + _, err := l.SetHold(context.Background(), "local:tester", connector.HoldByOperator) + require.NoError(t, err) + require.NoError(t, l.Close()) + return legacy +} + +func assertNotHeld(t *testing.T, dir string) { + t.Helper() + l, err := connector.OpenLedgerReadOnly(context.Background(), filepath.Join(dir, connector.LedgerFile)) + require.NoError(t, err) + defer func() { _ = l.Close() }() + _, held, err := l.HoldMarker(context.Background()) + require.NoError(t, err) + assert.False(t, held, "the release reached this ledger") +} + +func TestADecisionActsOnTheLegacyLedgerWithoutMovingIt(t *testing.T) { + f := newOperatorFixture(t) + legacy := heldLegacyLedger(t, f.file) + + out, err := f.run(t, output.FormatJSON, "release") + require.NoError(t, err, out) + + assertNotHeld(t, legacy) + _, err = os.Lstat(filepath.Join(connectStateRoot(t), connector.StateDirName(f.file.AccountID, f.file.Agent.PersonID, "agent"))) + assert.ErrorIs(t, err, os.ErrNotExist, "only a run that proves it is the agent moves the old folder") +} + +func TestADecisionBesideAnOlderConnectorActsOnItsLedger(t *testing.T) { + f := newOperatorFixture(t) + legacy := heldLegacyLedger(t, f.file) + lock, err := connector.AcquireInstanceLock(legacy, f.file.AccountID, f.file.Agent.PersonID, time.Now()) + require.NoError(t, err) + t.Cleanup(func() { _ = lock.Release() }) + + out, err := f.run(t, output.FormatJSON, "release") + require.NoError(t, err, out) + + assertNotHeld(t, legacy) + _, err = os.Lstat(filepath.Join(connectStateRoot(t), connector.StateDirName(f.file.AccountID, f.file.Agent.PersonID, "agent"))) + assert.ErrorIs(t, err, os.ErrNotExist) +} + +func TestAnImportBesideAnOlderConnectorIsRefused(t *testing.T) { + f := newOperatorFixture(t) + legacy := heldLegacyLedger(t, f.file) + lock, err := connector.AcquireInstanceLock(legacy, f.file.AccountID, f.file.Agent.PersonID, time.Now()) + require.NoError(t, err) + t.Cleanup(func() { _ = lock.Release() }) + file := filepath.Join(t.TempDir(), "reconciliation.json") + require.NoError(t, os.WriteFile(file, []byte(`{"version":1,"entries":[{"event_id":2,"decision":"done"}]}`), 0o600)) + + _, err = f.run(t, output.FormatJSON, "import", file) + require.Error(t, err) + assert.Equal(t, output.CodeLockUnavailable, usageError(t, err).Code) +} + +func TestPromoteRefusesAnUnverifiedLegacyShadowLedger(t *testing.T) { + f := newOperatorFixture(t) + legacyShadow, l := openLegacyLedger(t, f.file, "connect-shadow") + require.NoError(t, l.Close()) + + out, err := f.run(t, output.FormatJSON, "shadow", "promote") + require.Error(t, err, out) + assert.Equal(t, output.CodeUsage, usageError(t, err).Code) + assert.Contains(t, output.AsError(err).Hint, "basecamp connect --shadow -P agent") + _, err = os.Lstat(filepath.Join(legacyShadow, connector.LedgerFile)) + require.NoError(t, err, "the legacy shadow waits for verified ownership") + _, err = os.Lstat(filepath.Join(connectStateRoot(t), connector.StateDirName(f.file.AccountID, f.file.Agent.PersonID, "agent"))) + assert.ErrorIs(t, err, os.ErrNotExist) +} + +func TestPromoteRefusesWhileTheOldFolderHoldsTheConnectorsLedger(t *testing.T) { + f := newOperatorFixture(t) + legacy := seedLegacyLedger(t, f.s, f.file) + require.NoError(t, f.ledger(t, true).Close()) + + _, err := f.run(t, output.FormatJSON, "shadow", "promote") + require.Error(t, err) + assert.Equal(t, output.CodeUsage, usageError(t, err).Code) + assert.Contains(t, err.Error(), "already has a ledger") + assertLegacyLedgerKept(t, legacy, f.s, f.file) + _, err = os.Lstat(filepath.Join(connectStateRoot(t), connector.StateDirName(f.file.AccountID, f.file.Agent.PersonID, "agent"))) + assert.ErrorIs(t, err, os.ErrNotExist) +} + +func TestPromoteBesideAnOlderShadowIsRefused(t *testing.T) { + f := newOperatorFixture(t) + legacyShadow, l := openLegacyLedger(t, f.file, "connect-shadow") + require.NoError(t, l.Close()) + lock, err := connector.AcquireInstanceLock(legacyShadow, f.file.AccountID, f.file.Agent.PersonID, time.Now()) + require.NoError(t, err) + t.Cleanup(func() { _ = lock.Release() }) + + _, err = f.run(t, output.FormatJSON, "shadow", "promote") + require.Error(t, err) + assert.Equal(t, output.CodeUsage, usageError(t, err).Code) + assert.Contains(t, err.Error(), "has not verified ownership") + _, err = os.Lstat(filepath.Join(legacyShadow, connector.LedgerFile)) + require.NoError(t, err) +} + +func TestPromoteRefusesWhileAnOlderConnectorHoldsTheOldFolder(t *testing.T) { + f := newOperatorFixture(t) + require.NoError(t, f.ledger(t, true).Close()) + stateHome, err := connectStateHome() + require.NoError(t, err) + legacy, err := ensurePrivateChain(stateHome, "basecamp", "connect", connector.LegacyStateDirName(f.file.AccountID, f.file.Agent.PersonID)) + require.NoError(t, err) + lock, err := connector.AcquireInstanceLock(legacy, f.file.AccountID, f.file.Agent.PersonID, time.Now()) + require.NoError(t, err) + t.Cleanup(func() { _ = lock.Release() }) + + _, err = f.run(t, output.FormatJSON, "shadow", "promote") + require.Error(t, err) + assert.Equal(t, output.CodeLockUnavailable, usageError(t, err).Code) + _, err = os.Lstat(filepath.Join(connectStateRoot(t), connector.StateDirName(f.file.AccountID, f.file.Agent.PersonID, "agent"), connector.LedgerFile)) + assert.ErrorIs(t, err, os.ErrNotExist, "nothing was promoted beside a connector that is starting") +} + +func TestPromoteRefusesAnOldFolderItCannotInspect(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("root reads a directory whatever its mode") + } + f := newOperatorFixture(t) + require.NoError(t, f.ledger(t, true).Close()) + legacy := seedLegacyLedger(t, f.s, f.file) + require.NoError(t, os.Chmod(legacy, 0o000)) + t.Cleanup(func() { _ = os.Chmod(legacy, 0o700) }) + + _, err := f.run(t, output.FormatJSON, "shadow", "promote") + require.Error(t, err) + _, err = os.Lstat(filepath.Join(connectStateRoot(t), connector.StateDirName(f.file.AccountID, f.file.Agent.PersonID, "agent"), connector.LedgerFile)) + assert.ErrorIs(t, err, os.ErrNotExist, "an old ledger that cannot be seen is not taken for none") +} diff --git a/internal/commands/connect_run.go b/internal/commands/connect_run.go index 345de3318..f6e8f7aa2 100644 --- a/internal/commands/connect_run.go +++ b/internal/commands/connect_run.go @@ -46,12 +46,9 @@ func addConnectRunFlags(cmd *cobra.Command, f *connectRunFlags) { fl.Var((*repeatedString)(&f.projects), "project", "Only hear events in this project id (repeatable; default every project the agent can see)") fl.BoolVar(&f.shadow, "shadow", false, "Admit and log in an isolated state directory; dispatch and post nothing") fl.Int64Var(&f.since, "since", 0, "Enter the feed just after this event id, whatever the ledger holds") - fl.BoolVar(&f.hold, "hold", false, "Set the durable hold: intake and admission run, nothing is dispatched or posted until the hold is released, and earlier records wait for review") + fl.BoolVar(&f.hold, "hold", false, "Set this profile's durable hold: intake and admission run, nothing is dispatched or posted until release, and earlier records wait for review") } -// connectStateHome is the directory holding the connector's state root, from -// connector.StateRoot so the connector and the worker's MCP server agree on -// one place. func connectStateHome() (string, error) { root, err := connector.StateRoot() if err != nil { @@ -78,37 +75,92 @@ func ensurePrivateChain(root string, parts ...string) (string, error) { return dir, nil } -// connectStateDir is the connector's state directory for a set-up profile, -// created owner-only: $XDG_STATE_HOME/basecamp/connect/-, or -// connect-shadow for a shadow run. Everything that reads the connector's -// state (status) resolves it here. -func connectStateDir(file setup.File, shadow bool) (string, error) { +func connectStateDir(profile string, file setup.File, shadow bool) (string, error) { + dir, _, err := prepareConnectStateDir(profile, file, shadow) + return dir, err +} + +// Only a run that verified the credential may call this: adoption assigns the +// entire legacy ledger (including its hold and every checkpoint) to a profile. +func prepareConnectStateDir(profile string, file setup.File, shadow bool) (string, bool, error) { + if _, err := setup.Path("", profile); err != nil { + return "", false, err + } stateHome, err := connectStateHome() if err != nil { - return "", err + return "", false, err + } + parts := connectStateParts(profile, file, shadow) + parent, err := ensurePrivateChain(stateHome, parts[:len(parts)-1]...) + if err != nil { + return "", false, err + } + adopted, err := connector.AdoptLegacyStateDir(parent, file.AccountID, file.Agent.PersonID, profile) + if err != nil { + return "", false, err } - return ensurePrivateChain(stateHome, connectStateParts(file, shadow)...) + dir := filepath.Join(parent, parts[len(parts)-1]) + if err := setup.EnsurePrivateDir(dir); err != nil { + return "", adopted, err + } + return dir, adopted, nil } // connectStatePath is connectStateDir's path, created nothing: for commands // that only read the connector's state, and must not make a directory to do // it. -func connectStatePath(file setup.File, shadow bool) (string, error) { +func connectStatePath(profile string, file setup.File, shadow bool) (string, error) { + if _, err := setup.Path("", profile); err != nil { + return "", err + } stateHome, err := connectStateHome() if err != nil { return "", err } - return filepath.Join(append([]string{stateHome}, connectStateParts(file, shadow)...)...), nil + parts := connectStateParts(profile, file, shadow) + dir := filepath.Join(append([]string{stateHome}, parts...)...) + legacy := filepath.Join(filepath.Dir(dir), connector.LegacyStateDirName(file.AccountID, file.Agent.PersonID)) + if _, err := os.Lstat(dir); errors.Is(err, os.ErrNotExist) { + if _, err := os.Lstat(legacy); err == nil { + return legacy, nil + } else if !errors.Is(err, os.ErrNotExist) { + return "", err + } + } else if err != nil { + return "", err + } + return dir, nil +} + +// A fresh profile never borrows another profile's ledger. Name where earlier +// work lives so an upgrade does not silently look like it lost that work. +func warnConnectSiblingState(logger *slog.Logger, dir string, file setup.File, profile string) { + entries, err := os.ReadDir(filepath.Dir(dir)) + if err != nil { + logger.Warn("connector: could not inspect sibling state", "error", err) + return + } + prefix := connector.LegacyStateDirName(file.AccountID, file.Agent.PersonID) + for _, entry := range entries { + if entry.Name() == filepath.Base(dir) || (entry.Name() != prefix && !strings.HasPrefix(entry.Name(), prefix+"-")) { + continue + } + sibling := filepath.Join(filepath.Dir(dir), entry.Name()) + if _, err := os.Lstat(filepath.Join(sibling, connector.LedgerFile)); err == nil { + logger.Warn("connector: this profile has no ledger; earlier cursor, hold and records remain in sibling state; entering at the present unless --since selects an earlier event", + "profile", profile, "sibling", sibling) + } + } } -func connectStateParts(file setup.File, shadow bool) []string { +func connectStateParts(profile string, file setup.File, shadow bool) []string { group := "connect" if shadow { // An isolated ledger, lock and checkpoint: a shadow never shares a // position or a record with the connector it watches beside. group = "connect-shadow" } - return []string{"basecamp", group, connector.StateDirName(file.AccountID, file.Agent.PersonID)} + return []string{"basecamp", group, connector.StateDirName(file.AccountID, file.Agent.PersonID, profile)} } func runConnect(cmd *cobra.Command, f *connectRunFlags) error { @@ -175,7 +227,7 @@ func runConnect(cmd *cobra.Command, f *connectRunFlags) error { // until the credential is proven to be that agent, holding its lock // through the wait could keep the agent's real connector from starting. // The lock is taken for the run once the identity is verified. - stateDir, err := connectStateDir(file, f.shadow) + stateDir, err := connectStatePath(name, file, f.shadow) if err != nil { return output.ErrUsage("The connector's state directory cannot be used: " + err.Error()) } @@ -214,6 +266,17 @@ func runConnect(cmd *cobra.Command, f *connectRunFlags) error { return output.ErrAuth(err.Error()) } agentID := me.ID + stateDir, adopted, err := prepareConnectStateDir(name, file, f.shadow) + if adopted { + logger.Warn("connector: adopted legacy state; cursor, hold and held records belong to this profile now", + "profile", name, "from", filepath.Join(filepath.Dir(stateDir), connector.LegacyStateDirName(account, agentID)), "to", stateDir) + } + switch { + case errors.Is(err, connector.ErrAlreadyRunning): + return &output.Error{Code: output.CodeLockUnavailable, Message: err.Error()} + case err != nil: + return output.ErrUsage("The connector's state directory cannot be used: " + err.Error()) + } lock, err := acquireConnectLock(stateDir, account, agentID) if err != nil { return err @@ -226,6 +289,9 @@ func runConnect(cmd *cobra.Command, f *connectRunFlags) error { } policy.Buckets = buckets + if _, err := os.Lstat(filepath.Join(stateDir, connector.LedgerFile)); errors.Is(err, os.ErrNotExist) { + warnConnectSiblingState(logger, stateDir, file, name) + } ledger, err := connector.OpenLedger(filepath.Join(stateDir, connector.LedgerFile)) if err != nil { return err @@ -401,7 +467,7 @@ func connectorStoppedBy(err error, agent, profile string, refused func() bool) ( } // acquireConnectLock takes the connector's instance lock for the run, or -// says another connector for this agent already holds it. +// says another connector for this profile already holds it. func acquireConnectLock(stateDir, account string, agentID int64) (*connector.InstanceLock, error) { lock, err := connector.AcquireInstanceLock(stateDir, account, agentID, time.Now()) if errors.Is(err, connector.ErrAlreadyRunning) { @@ -413,10 +479,24 @@ func acquireConnectLock(stateDir, account string, agentID int64) (*connector.Ins // connectLockFree reports, by taking and at once releasing it, whether the // instance lock is free: the start's look before it waits for a token. func connectLockFree(stateDir, account string, agentID int64) error { - lock, err := acquireConnectLock(stateDir, account, agentID) - if err != nil { + if _, err := os.Lstat(stateDir); errors.Is(err, os.ErrNotExist) { + return nil + } else if err != nil { return err } + var lock *connector.InstanceLock + var err error + if filepath.Base(stateDir) == connector.LegacyStateDirName(account, agentID) { + lock, err = connector.AcquireLegacyInstanceLock(stateDir, account, agentID) + if err != nil { + return &output.Error{Code: output.CodeLockUnavailable, Message: err.Error()} + } + } else { + lock, err = acquireConnectLock(stateDir, account, agentID) + if err != nil { + return err + } + } return lock.Release() } diff --git a/internal/commands/connect_setup_guided.go b/internal/commands/connect_setup_guided.go index d3e68afb6..4fcd5b4b7 100644 --- a/internal/commands/connect_setup_guided.go +++ b/internal/commands/connect_setup_guided.go @@ -163,7 +163,7 @@ func runGuidedConnectSetup(cmd *cobra.Command, app *appctx.App, f *connectSetupF file = &loaded } - renderGuidedSummary(w, r, agent, *file, connectorRunning(*file), name) + renderGuidedSummary(w, r, agent, *file, connectorRunning(name, *file), name) return nil } @@ -543,8 +543,8 @@ func chooseGuidedProjects(w io.Writer, agent guidedAgent) ([]guidedProject, erro // connectorRunning reports whether a connector for this setup is running: // the instance lock's holder, as `connect status` reads it, is alive. -func connectorRunning(file setup.File) bool { - dir, err := connectStatePath(file, false) +func connectorRunning(profile string, file setup.File) bool { + dir, err := connectStatePath(profile, file, false) if err != nil { return false } diff --git a/internal/commands/connect_start_wait_test.go b/internal/commands/connect_start_wait_test.go index 901e4c2df..343594bbe 100644 --- a/internal/commands/connect_start_wait_test.go +++ b/internal/commands/connect_start_wait_test.go @@ -488,7 +488,7 @@ func TestAnOversizedServerFaultIsWaitedOut(t *testing.T) { // before the start waits for a token, not after. func TestASecondConnectorIsRefusedBeforeItWaitsForAToken(t *testing.T) { f := newOperatorFixture(t) - dir, err := connectStateDir(f.file, false) + dir, err := connectStateDir("agent", f.file, false) require.NoError(t, err) lock, err := connector.AcquireInstanceLock(dir, f.file.AccountID, f.file.Agent.PersonID, time.Now()) require.NoError(t, err) @@ -594,7 +594,7 @@ func TestAStartWaitingForItsTokenHoldsNoLock(t *testing.T) { require.Eventually(t, func() bool { return strings.Contains(out.String(), "could not get the agent's token yet") }, 10*time.Second, 20*time.Millisecond, "the start never waited: %s", out.String()) - dir, err := connectStateDir(f.file, false) + dir, err := connectStateDir("agent", f.file, false) require.NoError(t, err) lock, err := connector.AcquireInstanceLock(dir, f.file.AccountID, f.file.Agent.PersonID, time.Now()) require.NoError(t, err, "a waiting start held the agent's lock") diff --git a/internal/connector/ledger_dispatch.go b/internal/connector/ledger_dispatch.go index 18addbf81..50a3cc9c9 100644 --- a/internal/connector/ledger_dispatch.go +++ b/internal/connector/ledger_dispatch.go @@ -15,7 +15,6 @@ import ( "os" "path/filepath" "slices" - "strconv" "strings" "time" @@ -1287,56 +1286,6 @@ func scanAttributes(text string, pos int) (sgid string, end int, ok bool) { return sgid, pos, false } -// StateDirName is the connector's state directory for one account and agent, -// "-", inside StateRoot. -func StateDirName(accountID string, agentID int64) string { - return accountID + "-" + strconv.FormatInt(agentID, 10) -} - -// ErrNotAStateDir is a directory that is not a connector state directory for -// the account asked about. StateDirError carries why. -var ErrNotAStateDir = errors.New("not the connector's state directory for this account") - -// StateDirError says which rule a state directory failed, in fields a caller -// can build its own message from rather than by reading this one. -type StateDirError struct { - // Dir is the directory as given, made absolute. - Dir string - // Root is the connector's state root, the only place a state directory - // lives. - Root string - // Account is the account the directory names, empty when it names none; - // Want is the account it had to name. - Account, Want string - // Why is the rule it failed. - Why StateDirProblem -} - -// StateDirProblem is why a state directory was refused. -type StateDirProblem string - -const ( - // StateDirElsewhere is a directory outside the state root. - StateDirElsewhere StateDirProblem = "outside the connector's state root" - // StateDirMisnamed is a directory not named -. - StateDirMisnamed StateDirProblem = "not named -" - // StateDirOtherAccount is another account's state directory. - StateDirOtherAccount StateDirProblem = "another account's" -) - -func (e *StateDirError) Error() string { - switch e.Why { - case StateDirElsewhere: - return fmt.Sprintf("%s is not inside %s", e.Dir, e.Root) - case StateDirOtherAccount: - return fmt.Sprintf("%s belongs to account %s, not %s", e.Dir, e.Account, e.Want) - default: - return fmt.Sprintf("%s is not named -", e.Dir) - } -} - -func (e *StateDirError) Unwrap() error { return ErrNotAStateDir } - // StateRoot is where every connector state directory lives: // $XDG_STATE_HOME/basecamp/connect, or ~/.local/state/basecamp/connect when // XDG_STATE_HOME is unset or not absolute, as the XDG specification says. @@ -1352,49 +1301,5 @@ func StateRoot() (string, error) { return filepath.Join(filepath.Clean(base), "basecamp", "connect"), nil } -// ResolveStateDir is the one place a state directory is accepted: dir must -// be exactly StateRoot/-, and its account must be -// accountID, compared as numbers. It returns the directory made absolute — -// which is the one every caller should go on to use — and the agent's Person -// id. -// -// The location is part of the check, not only the name. A directory named -// for this account anywhere else — a copy of another account's ledger renamed -// to match — is refused, because the name is what binds a ledger to an -// account and anyone can choose a name. -func ResolveStateDir(dir, accountID string) (string, int64, error) { - root, err := StateRoot() - if err != nil { - return "", 0, err - } - abs, err := filepath.Abs(dir) - if err != nil { - return "", 0, fmt.Errorf("connector: state directory %q: %w", dir, err) - } - refuse := func(why StateDirProblem, account string) (string, int64, error) { - return "", 0, &StateDirError{Dir: abs, Root: root, Account: account, Want: accountID, Why: why} - } - if filepath.Dir(abs) != root { - return refuse(StateDirElsewhere, "") - } - account, agent, ok := strings.Cut(filepath.Base(abs), "-") - agentID, err := strconv.ParseInt(agent, 10, 64) - if !ok || err != nil || agentID <= 0 || agent != strconv.FormatInt(agentID, 10) { - // The agent is spelled one way, so one directory answers to one name: - // "+52007412" and "052007412" are other directories, not this one. - return refuse(StateDirMisnamed, account) - } - given, errGiven := strconv.ParseUint(account, 10, 64) - if errGiven != nil || given == 0 { - // Not an account at all: the name is wrong, not another account's. - return refuse(StateDirMisnamed, account) - } - want, errWant := strconv.ParseUint(accountID, 10, 64) - if errWant != nil || given != want { - return refuse(StateDirOtherAccount, account) - } - return abs, agentID, nil -} - // LedgerFile is the ledger's file name inside the state directory. const LedgerFile = "ledger.db" diff --git a/internal/connector/ledger_dispatch_test.go b/internal/connector/ledger_dispatch_test.go index 7042e2141..811f28591 100644 --- a/internal/connector/ledger_dispatch_test.go +++ b/internal/connector/ledger_dispatch_test.go @@ -840,51 +840,12 @@ func entityEncodedSGID(mention string) string { return mention[:i] + fmt.Sprintf("&#x%x;", mention[i]) + mention[i+1:] } -func TestResolveStateDirAcceptsOnlyTheCanonicalDirectory(t *testing.T) { +func TestStateRootFollowsXDGStateHome(t *testing.T) { home := t.TempDir() t.Setenv("XDG_STATE_HOME", home) root, err := StateRoot() require.NoError(t, err) assert.Equal(t, filepath.Join(home, "basecamp", "connect"), root) - canonical := filepath.Join(root, StateDirName("999", adapterAgentID)) - - resolved, agentID, err := ResolveStateDir(canonical, "999") - require.NoError(t, err) - assert.Equal(t, adapterAgentID, agentID) - assert.Equal(t, canonical, resolved) - resolved, agentID, err = ResolveStateDir(canonical+"/../"+StateDirName("999", adapterAgentID)+"/", "0999") - require.NoError(t, err, "accounts compare as numbers, and a trailing slash or a .. is the same directory") - assert.Equal(t, adapterAgentID, agentID) - assert.Equal(t, canonical, resolved, "the directory every caller goes on to use is the one that was checked") - - for name, dir := range map[string]string{ - "outside the root": filepath.Join(t.TempDir(), StateDirName("999", adapterAgentID)), - "nested in the root": filepath.Join(root, "x", StateDirName("999", adapterAgentID)), - "another account": filepath.Join(root, StateDirName("1000", adapterAgentID)), - "no agent": filepath.Join(root, "999-"), - "no account": filepath.Join(root, "-52007412"), - "not a number": filepath.Join(root, "999-abc"), - "a signed agent": filepath.Join(root, "999-+52007412"), - "a padded agent": filepath.Join(root, "999-052007412"), - "the root itself": root, - "above the root": filepath.Join(root, "..", StateDirName("999", adapterAgentID)), - } { - t.Run(name, func(t *testing.T) { - _, _, err := ResolveStateDir(dir, "999") - assert.ErrorIs(t, err, ErrNotAStateDir) - var refusal *StateDirError - require.ErrorAs(t, err, &refusal, "the refusal says why in fields, not in a message to be parsed") - assert.Equal(t, root, refusal.Root) - assert.Equal(t, "999", refusal.Want) - if name == "another account" { - assert.Equal(t, StateDirOtherAccount, refusal.Why) - assert.Equal(t, "1000", refusal.Account) - } - if name == "outside the root" { - assert.Equal(t, StateDirElsewhere, refusal.Why) - } - }) - } t.Setenv("XDG_STATE_HOME", "relative/state") root, err = StateRoot() diff --git a/internal/connector/lock.go b/internal/connector/lock.go index f153542b8..6def79436 100644 --- a/internal/connector/lock.go +++ b/internal/connector/lock.go @@ -12,17 +12,8 @@ import ( "github.com/basecamp/basecamp-cli/internal/connector/setup" ) -// ErrAlreadyRunning reports a second connector for the same agent. -var ErrAlreadyRunning = errors.New("connector: another connector already holds this account and agent") +var ErrAlreadyRunning = errors.New("connector: another connector already runs for this profile") -// InstanceLock is the refusal of a second connector on one agent identity. -// -// It is keyed on the ACCOUNT and the agent's Person id, not on the profile -// name. Two profiles can hold credentials for the same agent, and the failure -// this prevents is not "the same configuration twice" — it is one agent's -// mentions being dispatched twice, which is a property of the identity, not of -// the file that names it. -// // The kernel drops a flock when the holding descriptor closes, process death // included, so a crashed connector cannot wedge the lock and there is no // stale-lock reaping to get wrong. The metadata written beside it is @@ -41,7 +32,8 @@ type instanceHolder struct { AgentID int64 `json:"agent_person_id"` } -// AcquireInstanceLock takes the lock for one account and agent, or refuses. +// AcquireInstanceLock takes the lock in a profile's state directory, or refuses. +// The account and agent name the file; the containing directory isolates profiles. func AcquireInstanceLock(dir, accountID string, agentPersonID int64, now time.Time) (*InstanceLock, error) { account, err := strconv.ParseUint(accountID, 10, 64) if err != nil || account == 0 || agentPersonID <= 0 { @@ -59,7 +51,7 @@ func AcquireInstanceLock(dir, accountID string, agentPersonID int64, now time.Ti // the lock file itself are vetted here, by the same check the connector's // trust file gets. Someone who can write the directory can point two // connectors at two inodes, and then neither excludes the other: one - // agent's mentions dispatched twice, which is the failure this lock + // profile's mentions dispatched twice, which is the failure this lock // exists to prevent. unlock, err := setup.TryLockPrivate(path) switch { diff --git a/internal/connector/lock_test.go b/internal/connector/lock_test.go index ff3852b93..736ac0f69 100644 --- a/internal/connector/lock_test.go +++ b/internal/connector/lock_test.go @@ -28,8 +28,6 @@ func TestSecondConnectorOnTheSameAgentIsRefused(t *testing.T) { assert.Contains(t, err.Error(), "pid", "the refusal says who is holding it") } -// The key is the identity, not the profile that names it: two profiles can -// hold credentials for one agent, and it is the agent that gets double-served. func TestTheLockIsKeyedOnAccountAndAgent(t *testing.T) { dir := t.TempDir() now := time.Now() diff --git a/internal/connector/operator_migration_test.go b/internal/connector/operator_migration_test.go index 2ccb12b22..850e6ddf1 100644 --- a/internal/connector/operator_migration_test.go +++ b/internal/connector/operator_migration_test.go @@ -34,8 +34,8 @@ const ( func shadowFixture(t *testing.T) (shadowDir, stateDir string) { t.Helper() root := filepath.Join(t.TempDir(), "basecamp") - shadowDir = filepath.Join(root, "connect-shadow", StateDirName(opAccount, opAgent)) - stateDir = filepath.Join(root, "connect", StateDirName(opAccount, opAgent)) + shadowDir = filepath.Join(root, "connect-shadow", StateDirName(opAccount, opAgent, "agent")) + stateDir = filepath.Join(root, "connect", StateDirName(opAccount, opAgent, "agent")) for _, dir := range []string{root, filepath.Dir(shadowDir), filepath.Dir(stateDir)} { require.NoError(t, os.MkdirAll(dir, 0o700)) require.NoError(t, os.Chmod(dir, 0o700)) diff --git a/internal/connector/statedir.go b/internal/connector/statedir.go new file mode 100644 index 000000000..eeb637734 --- /dev/null +++ b/internal/connector/statedir.go @@ -0,0 +1,84 @@ +package connector + +import ( + "errors" + "fmt" + "os" + "path/filepath" + "strconv" + "time" + + "github.com/basecamp/basecamp-cli/internal/connector/setup" +) + +func StateDirName(accountID string, agentID int64, profile string) string { + return LegacyStateDirName(accountID, agentID) + "-" + profile +} + +func LegacyStateDirName(accountID string, agentID int64) string { + if account, err := strconv.ParseUint(accountID, 10, 64); err == nil { + accountID = strconv.FormatUint(account, 10) + } + return accountID + "-" + strconv.FormatInt(agentID, 10) +} + +func AdoptLegacyStateDir(parent, accountID string, agentID int64, profile string) (bool, error) { + if _, err := setup.Path("", profile); err != nil { + return false, err + } + if account, err := strconv.ParseUint(accountID, 10, 64); err != nil || account == 0 || agentID <= 0 { + return false, errors.New("connector: state needs a numeric account id and an agent person id") + } + dir := filepath.Join(parent, StateDirName(accountID, agentID, profile)) + legacy := filepath.Join(parent, LegacyStateDirName(accountID, agentID)) + if present, err := pathPresent(dir); err != nil || present { + return false, err + } + if present, err := pathPresent(legacy); err != nil || !present { + return false, err + } + lock, err := AcquireLegacyInstanceLock(legacy, accountID, agentID) + if err != nil { + return false, err + } + defer func() { _ = lock.Release() }() + if present, err := pathPresent(dir); err != nil || present { + return false, err + } + if err := os.Rename(legacy, dir); err != nil { + return false, fmt.Errorf("connector: move %s to %s: %w", legacy, dir, err) + } + _ = os.Remove(filepath.Join(dir, filepath.Base(lock.Path())+".json")) + return true, syncDirectory(parent) +} + +// AcquireLegacyInstanceLock briefly waits for another verified adopter. A +// persistently held legacy lock belongs to an older connector, not necessarily +// this profile. Older builds must be stopped before migration. +func AcquireLegacyInstanceLock(dir, accountID string, agentID int64) (*InstanceLock, error) { + for attempt := 0; ; attempt++ { + lock, err := AcquireInstanceLock(dir, accountID, agentID, time.Now()) + if !errors.Is(err, ErrAlreadyRunning) { + return lock, err + } + if attempt == 3 { + return nil, LegacyLockError(dir, err) + } + time.Sleep(50 * time.Millisecond) + } +} + +func LegacyLockError(dir string, err error) error { + return fmt.Errorf("connector: an older connector for this agent holds %s (%v); stop it and restart its existing profile on this build before starting another profile: %w", dir, err, ErrAlreadyRunning) +} + +func pathPresent(path string) (bool, error) { + switch _, err := os.Lstat(path); { + case err == nil: + return true, nil + case errors.Is(err, os.ErrNotExist): + return false, nil + default: + return false, fmt.Errorf("connector: inspect %s: %w", path, err) + } +} diff --git a/internal/connector/statedir_test.go b/internal/connector/statedir_test.go new file mode 100644 index 000000000..4c2fa80f8 --- /dev/null +++ b/internal/connector/statedir_test.go @@ -0,0 +1,142 @@ +package connector + +import ( + "os" + "path/filepath" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func stateParent(t *testing.T) string { + t.Helper() + parent := filepath.Join(t.TempDir(), "connect") + require.NoError(t, os.Mkdir(parent, 0o700)) + return parent +} + +func legacyStateDir(t *testing.T, parent string) string { + t.Helper() + legacy := filepath.Join(parent, LegacyStateDirName("2914079", 52007412)) + require.NoError(t, os.Mkdir(legacy, 0o700)) + require.NoError(t, os.WriteFile(filepath.Join(legacy, LedgerFile), []byte("the old ledger"), 0o600)) + return legacy +} + +func TestStateDirNamesAreOnePerProfile(t *testing.T) { + assert.Equal(t, "2914079-52007412", LegacyStateDirName("2914079", 52007412)) + assert.Equal(t, "2914079-52007412-rob-agent", StateDirName("2914079", 52007412, "rob-agent")) + assert.Equal(t, StateDirName("02914079", 52007412, "a"), StateDirName("2914079", 52007412, "a")) + assert.NotEqual(t, StateDirName("2914079", 52007412, "a"), StateDirName("2914079", 52007412, "b")) + assert.NotEqual(t, StateDirName("2914079", 52007412, "1-a"), StateDirName("2914079", 520074121, "a")) +} + +func TestAdoptMovesTheLegacyDirectoryToTheProfile(t *testing.T) { + parent := stateParent(t) + legacy := legacyStateDir(t, parent) + + adopted, err := AdoptLegacyStateDir(parent, "2914079", 52007412, "agent-a") + require.NoError(t, err) + assert.True(t, adopted) + + dir := filepath.Join(parent, StateDirName("2914079", 52007412, "agent-a")) + data, err := os.ReadFile(filepath.Join(dir, LedgerFile)) + require.NoError(t, err) + assert.Equal(t, "the old ledger", string(data)) + _, err = os.Lstat(legacy) + assert.ErrorIs(t, err, os.ErrNotExist) + entries, err := os.ReadDir(dir) + require.NoError(t, err) + for _, e := range entries { + assert.NotEqual(t, ".json", filepath.Ext(e.Name()), "no holder is left behind for a lock nobody holds") + } + + again, err := AdoptLegacyStateDir(parent, "2914079", 52007412, "agent-a") + require.NoError(t, err) + assert.False(t, again, "adopting twice moves nothing") + data, err = os.ReadFile(filepath.Join(dir, LedgerFile)) + require.NoError(t, err) + assert.Equal(t, "the old ledger", string(data)) +} + +func TestOnlyOneProfileAdoptsTheLegacyDirectory(t *testing.T) { + parent := stateParent(t) + legacyStateDir(t, parent) + + first, err := AdoptLegacyStateDir(parent, "2914079", 52007412, "agent-a") + require.NoError(t, err) + require.True(t, first) + second, err := AdoptLegacyStateDir(parent, "2914079", 52007412, "agent-b") + require.NoError(t, err) + assert.False(t, second) + + _, err = os.Lstat(filepath.Join(parent, StateDirName("2914079", 52007412, "agent-b"))) + assert.ErrorIs(t, err, os.ErrNotExist, "the second profile starts a directory of its own") + _, err = os.Lstat(filepath.Join(parent, StateDirName("2914079", 52007412, "agent-a"), LedgerFile)) + require.NoError(t, err) +} + +func TestAdoptLeavesAProfileThatAlreadyHasADirectoryAlone(t *testing.T) { + parent := stateParent(t) + legacy := legacyStateDir(t, parent) + dir := filepath.Join(parent, StateDirName("2914079", 52007412, "agent-a")) + require.NoError(t, os.Mkdir(dir, 0o700)) + require.NoError(t, os.WriteFile(filepath.Join(dir, LedgerFile), []byte("the new ledger"), 0o600)) + + adopted, err := AdoptLegacyStateDir(parent, "2914079", 52007412, "agent-a") + require.NoError(t, err) + assert.False(t, adopted) + + data, err := os.ReadFile(filepath.Join(dir, LedgerFile)) + require.NoError(t, err) + assert.Equal(t, "the new ledger", string(data)) + data, err = os.ReadFile(filepath.Join(legacy, LedgerFile)) + require.NoError(t, err) + assert.Equal(t, "the old ledger", string(data)) +} + +func TestAdoptWithNoLegacyDirectoryCreatesNothing(t *testing.T) { + parent := stateParent(t) + + adopted, err := AdoptLegacyStateDir(parent, "2914079", 52007412, "agent-a") + require.NoError(t, err) + assert.False(t, adopted) + entries, err := os.ReadDir(parent) + require.NoError(t, err) + assert.Empty(t, entries) +} + +func TestAdoptRefusesWhileAConnectorRunsInTheLegacyDirectory(t *testing.T) { + parent := stateParent(t) + legacy := legacyStateDir(t, parent) + lock, err := AcquireInstanceLock(legacy, "2914079", 52007412, time.Now()) + require.NoError(t, err) + t.Cleanup(func() { _ = lock.Release() }) + + adopted, err := AdoptLegacyStateDir(parent, "2914079", 52007412, "agent-a") + require.ErrorIs(t, err, ErrAlreadyRunning) + assert.False(t, adopted) + _, err = os.Lstat(filepath.Join(legacy, LedgerFile)) + require.NoError(t, err) + _, err = os.Lstat(filepath.Join(parent, StateDirName("2914079", 52007412, "agent-a"))) + assert.ErrorIs(t, err, os.ErrNotExist) +} + +func TestTwoProfilesHoldTheirLocksAtOnceAndOneProfileOnlyOnce(t *testing.T) { + parent := stateParent(t) + now := time.Now() + a := filepath.Join(parent, StateDirName("2914079", 52007412, "agent-a")) + b := filepath.Join(parent, StateDirName("2914079", 52007412, "agent-b")) + + first, err := AcquireInstanceLock(a, "2914079", 52007412, now) + require.NoError(t, err) + t.Cleanup(func() { _ = first.Release() }) + other, err := AcquireInstanceLock(b, "2914079", 52007412, now) + require.NoError(t, err, "another profile for the same agent runs beside it") + t.Cleanup(func() { _ = other.Release() }) + + _, err = AcquireInstanceLock(a, "2914079", 52007412, now) + require.ErrorIs(t, err, ErrAlreadyRunning) +} diff --git a/skills/basecamp-connect/SKILL.md b/skills/basecamp-connect/SKILL.md index 6206e9f01..f34cfd3c5 100644 --- a/skills/basecamp-connect/SKILL.md +++ b/skills/basecamp-connect/SKILL.md @@ -58,7 +58,7 @@ Claude Code only. The connector runs on Linux and macOS. ### 1. Start it, and watch its output -Only one connector per agent can run at a time. If it's already running +Only one connector per profile can run at a time. If it's already running elsewhere, it refuses to start and says so: tell the person, and don't start a second. @@ -541,7 +541,7 @@ output and tell the person what it means: - **"was disconnected in Basecamp, or connected on another computer"**: run `basecamp connect setup -P ''` (it reconnects with one approval), then start it again. -- **Already running**: another connector for this agent holds its lock, on +- **Already running**: another connector for this profile holds its lock, on this computer or in another session. Don't start a second. - **`BASECAMP_TOKEN` is set**: the person unsets it, then you start it again. - **Anything else**: show the error, and run `basecamp connect doctor -P From 72e3abb582e570b531b039bfe5e98d820d18469e Mon Sep 17 00:00:00 2001 From: Rob Zolkos Date: Wed, 14 Oct 2026 15:58:46 -0400 Subject: [PATCH 2/6] Make connection guidance safe for CLI-first release MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reuse safe copy and lost-handover tests from Rob Zolkos’s CLI #900 at 7ba7e962 (27658202 through 7ba7e962), 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. --- README.md | 18 +++++++-- internal/auth/agent.go | 4 +- internal/auth/agent_connect.go | 15 ++++---- internal/auth/agent_connect_test.go | 26 ++++++++----- internal/commands/auth_agent.go | 18 +++++++-- internal/commands/connect.go | 16 ++++++-- .../commands/connect_copy_contract_test.go | 38 +++++++++++++++++++ internal/commands/connect_overlap_test.go | 30 +++++++++++++++ internal/commands/connect_run.go | 12 +++--- .../commands/connect_run_disconnect_test.go | 12 +++--- internal/commands/connect_setup_guided.go | 8 ++-- .../commands/connect_setup_guided_test.go | 3 +- internal/commands/connect_setup_test.go | 2 +- internal/connector/ledger_events.go | 3 +- .../basecamp-connect/unconfirmed-identity.yml | 2 +- skills/basecamp-connect/SKILL.md | 30 +++++++++------ 16 files changed, 177 insertions(+), 60 deletions(-) create mode 100644 internal/commands/connect_copy_contract_test.go create mode 100644 internal/commands/connect_overlap_test.go diff --git a/README.md b/README.md index 3ecf2f7be..fb577eb4a 100644 --- a/README.md +++ b/README.md @@ -270,9 +270,21 @@ basecamp connect setup -P my-agent --serve 12345 # who may give it work, basecamp connect -P my-agent # the watcher ``` -Connect each agent once. Every connection rotates the agent's secret, so a -second one, from this CLI or from the HTTP handshake on Basecamp's instructions -page, disconnects the first and asks you to approve again. +An approval may add a connection or replace an existing one: read the approval +page before proceeding. Personal agents can have several connections when +Basecamp enables adding them. Otherwise, and always for shared agents, a new +approval replaces the existing connection. This applies to both the CLI and +Basecamp's HTTP handshake. Do not reconnect just to upgrade the CLI. + +Each profile has its own projects, ledger, cursor, hold and local lock. When +two connections serve the same project, a request both admit can be answered +twice, so serve each project from one. Use a distinct `--device-name` for each +profile on the same computer. Where Basecamp lists several connections, you +can rename them and disconnect one without using Disconnect all. Reconnecting +a profile on an add-capable server leaves its old connection live until you +disconnect it; older credential files can remain live too. Do not delete them +as part of setup. See the [upgrade and downgrade runbook](docs/agent-connections-rollout.md) +for legacy state adoption and mixed-version precautions. In Claude Code, the `/basecamp-connect` skill runs the watcher and hands each request to a subagent. It ships in this CLI's plugin from 0.12.0 on, and an diff --git a/internal/auth/agent.go b/internal/auth/agent.go index ec90cbdd8..5e9217e58 100644 --- a/internal/auth/agent.go +++ b/internal/auth/agent.go @@ -525,8 +525,8 @@ func (m *Manager) agentMintRefusal(resp *http.Response, body []byte, mint *agent } // ErrAgentCredentialRefused is the cause of a mint the token endpoint -// refused for the credentials themselves: the agent was disconnected in -// Basecamp, or connected on another computer, which replaced its secret. +// refused for the credentials themselves: this connection may have been +// disconnected in Basecamp or replaced by a newer approval. var ErrAgentCredentialRefused = errors.New("the agent's credential was refused") // clientRefusalCodes are the RFC 6749 §5.2 codes that say THE CREDENTIALS diff --git a/internal/auth/agent_connect.go b/internal/auth/agent_connect.go index fafca6a99..4181e5720 100644 --- a/internal/auth/agent_connect.go +++ b/internal/auth/agent_connect.go @@ -314,12 +314,11 @@ func (m *Manager) ConnectAgent(ctx context.Context, opts AgentConnectOptions) (* // adoptAgentGrantOrSayWhatWasLost stores the handed-over credential, and // says what a failure costs. // -// The poll that answered burned the one-time code and MINTED THE AGENT'S -// SECRET — a rotation, for an agent something was already connected to — -// so what this holds is the only live copy and the server will not hand it -// over twice. A failure here (a refused mint, a profile that cannot be -// written, a Ctrl-C landing in the gap) therefore leaves the agent -// connected to nothing. Retrying this command starts a new ceremony, which +// The poll that answered burned the one-time code and minted this connection's +// secret, possibly replacing an existing connection. The server will not hand +// it over twice. A failure here (a refused mint, a profile that cannot be +// written, a Ctrl-C landing in the gap) leaves this connection without a kept +// credential; other connections may still work. Retrying this command starts a new ceremony, which // is the right remedy; polling the spent code again is not, and neither is // looking for the secret somewhere. func (m *Manager) adoptAgentGrantOrSayWhatWasLost(ctx context.Context, disc *discovery, conn *AgentConnection, log func(string), opts ClientCredentialsOptions) (*LoginResult, error) { @@ -344,14 +343,14 @@ func reconnectRemedy(err error) error { return err } hinted := *e - hinted.Hint = "Disconnect the agent in Basecamp and run the connect command again; the secret this connection was handed is gone." + hinted.Hint = "Disconnect the newest connection in Basecamp (last in the list, where connections are listed), not Disconnect all, and run the connect command again; its secret is gone, so cleanup can wait." return &hinted } // warnHandoverLost says what a failure after the handover costs, in the // one line an operator has to act on. func warnHandoverLost(log func(string)) { - log("warning: the connection was approved and the agent's client secret was handed over once, but keeping it failed. Disconnect the agent in Basecamp and connect again.") + log("warning: the connection was approved and its client secret was handed over once, but keeping it failed. Disconnect the newest connection in Basecamp (last in the list, where connections are listed), not Disconnect all, and connect again; its secret is gone, so cleanup can wait.") } // spentHandoverError marks an error raised once the poll had already been diff --git a/internal/auth/agent_connect_test.go b/internal/auth/agent_connect_test.go index e4da47f27..63ac007bc 100644 --- a/internal/auth/agent_connect_test.go +++ b/internal/auth/agent_connect_test.go @@ -387,7 +387,7 @@ func TestConnectAgentRefusesATokenWiderThanTheApproval(t *testing.T) { require.Error(t, err) assert.Contains(t, err.Error(), "wider than the scope") assertNoAgentCredential(t, m) - assert.Contains(t, cl.joined(), "Disconnect the agent in Basecamp and connect again") + assertLostHandoverRemedy(t, cl.joined()) assert.NotContains(t, cl.joined(), "minted", "no token from a refused mint is kept") } @@ -535,7 +535,7 @@ func TestConnectAgentRefusesAScopeItCannotStore(t *testing.T) { assertNoAgentCredential(t, m) // The poll was answered, so the code is burned and the secret minted // whatever this process made of the answer. - assert.Contains(t, cl.joined(), "Disconnect the agent in Basecamp and connect again") + assertLostHandoverRemedy(t, cl.joined()) var e *output.Error assert.ErrorAs(t, err, &e, "the refusal keeps its own class through the spent-handover marker") } @@ -697,11 +697,9 @@ func TestConnectAgentStoresNothingWhenTheProfileCannotBeCommitted(t *testing.T) require.Error(t, err) assert.Contains(t, err.Error(), "bound to another account") assertNoAgentCredential(t, m) - // The poll burned the code and rotated the agent's secret, so this - // failure leaves the agent connected to nothing. Saying "nothing was - // stored" alone would send the operator looking for a credential that - // no longer exists anywhere. - assert.Contains(t, cl.joined(), "Disconnect the agent in Basecamp and connect again") + // The poll burned the code and minted this connection's secret. Other + // connections may survive, but this one has no retained credential. + assertLostHandoverRemedy(t, cl.joined()) } // TestConnectAgentRefusesTheLaunchpadFallback: Launchpad has no agent @@ -842,7 +840,7 @@ func TestConnectAgentRefusesAHandoverWiderThanTheRequest(t *testing.T) { assert.Contains(t, err.Error(), "read-only") assert.Empty(t, as.calls(&as.tokenForms), "a handover wider than the request is never minted with") assertNoAgentCredential(t, m) - assert.Contains(t, cl.joined(), "Disconnect the agent in Basecamp and connect again") + assertLostHandoverRemedy(t, cl.joined()) } // TestConnectAgentHonorsASlowRetryAfter: a Retry-After longer than a @@ -880,7 +878,7 @@ func TestConnectAgentHandsBackARemedyTheOperatorCanFollow(t *testing.T) { require.Error(t, err) var e *output.Error require.ErrorAs(t, err, &e) - assert.Contains(t, e.Hint, "Disconnect the agent in Basecamp") + assertLostHandoverRemedy(t, e.Hint) assert.NotContains(t, e.Hint, "--with-client-credentials") } @@ -931,6 +929,14 @@ func TestConnectAgentReportsAnAccountLimitFromThePoll(t *testing.T) { // what it was going to do when it answered 200 — the code is burned and // the secret minted — so a body that never arrives costs exactly what a // refused mint costs and has to say the same thing. +func assertLostHandoverRemedy(t *testing.T, text string) { + t.Helper() + assert.Contains(t, text, "Disconnect the newest connection in Basecamp") + assert.Contains(t, text, "last in the list, where connections are listed") + assert.Contains(t, text, "not Disconnect all") + assert.Contains(t, text, "cleanup can wait") +} + func TestConnectAgentTreatsAnInterruptedHandoverAsSpent(t *testing.T) { as := startConnectAS(t) as.pollHijack = true @@ -939,7 +945,7 @@ func TestConnectAgentTreatsAnInterruptedHandoverAsSpent(t *testing.T) { cl := &collectLogger{} _, err := m.ConnectAgent(context.Background(), connectOptions(cl, newTestClock())) require.Error(t, err) - assert.Contains(t, cl.joined(), "Disconnect the agent in Basecamp and connect again") + assertLostHandoverRemedy(t, cl.joined()) assertNoAgentCredential(t, m) assert.Empty(t, as.calls(&as.tokenForms), "nothing was minted with a credential that never arrived") } diff --git a/internal/commands/auth_agent.go b/internal/commands/auth_agent.go index a66eeac4b..2be1b3e86 100644 --- a/internal/commands/auth_agent.go +++ b/internal/commands/auth_agent.go @@ -52,6 +52,17 @@ the agent's own OAuth client: this CLI mints an access token from it whenever it needs one, so there is no token to paste and none to renew by hand. +An approval may add a connection or replace an existing one; read the +approval page before proceeding. Personal agents can keep several connections +when Basecamp has enabled adding them. Otherwise, and always for shared +agents, connecting replaces the existing connection, wherever it runs. + +The device name defaults to this computer's name. Give each profile its own +--device-name when several on this computer connect the same agent. +Reconnecting a profile stores its new credential in place of the old one. +Where approval adds, the old connection stays live until disconnected in +Basecamp; other credential files are not removed or revoked by this CLI. + The account comes from the agent you approve — pass --account only to insist on one, and the connection is refused if the agent belongs to another. @@ -65,8 +76,9 @@ Over SSH, in CI, or on a host with no display the browser is skipped and the link is yours to open on any device; --local forces a launch anyway, --no-browser skips it. Ctrl-C cancels the wait and stores nothing. -Disconnecting is done in Basecamp: disconnecting the agent there kills the -client secret this holds. Drop the local copy with ` + "`basecamp auth logout -P `" + `.`, +Disconnecting is done in Basecamp: disconnecting this connection there kills +the secret this holds. Where several connections are listed, choose this +connection, not Disconnect all. Drop the local copy with ` + "`basecamp auth logout -P `" + `.`, Annotations: map[string]string{AnnotationProfileMayCreate: "true"}, Args: cobra.NoArgs, RunE: func(cmd *cobra.Command, args []string) error { @@ -94,7 +106,7 @@ client secret this holds. Drop the local copy with ` + "`basecamp auth logout -P }, } - cmd.Flags().StringVar(&deviceName, "device-name", "", "Name this computer on the approval page (default: this host's name)") + cmd.Flags().StringVar(&deviceName, "device-name", "", "Name this device/connection (default: this host's name; use a distinct name for each profile)") cmd.Flags().StringVar(&softwareName, "software-name", "", "Name the software this CLI runs inside, shown as \"Connected to\"") cmd.Flags().StringVar(&scope, "scope", "", "OAuth scope to ask for: 'read' or 'full' (default full; the approval page can downgrade it)") cmd.Flags().BoolVar(&noBrowser, "no-browser", false, "Print the link instead of opening a browser") diff --git a/internal/commands/connect.go b/internal/commands/connect.go index c59318449..2d739b031 100644 --- a/internal/commands/connect.go +++ b/internal/commands/connect.go @@ -57,9 +57,15 @@ line: events seen, admission's verdicts, and a "type":"request" line for each trusted request, which is the one to act on. Logs go to stderr. SIGINT and SIGTERM exit 130 and 143. A request made before the connector started is not picked up. --shadow admits and logs in an isolated state directory and hands -off nothing. --hold sets a durable hold: intake and admission run, nothing is -handed off, and earlier records wait for review, until basecamp connect -release. Linux and macOS only. +off nothing. --hold sets this profile's durable hold: intake and admission run, +nothing is handed off, and earlier records wait for review, until basecamp +connect release for this profile. Linux and macOS only. + +One connector runs per profile in this state directory. Personal agents can +run several connections when Basecamp enables adding them; otherwise, and for +shared agents, a new approval replaces the existing connection. Each profile +serves its own projects and has its own hold. When two connections serve the +same project, a request both admit can be answered twice. basecamp connect status what it heard and holds basecamp connect doctor what it needs to run @@ -358,6 +364,10 @@ does not serve is handed off, and nobody is told. --watch-completions '` (add `--no-browser` if the person is on another device). Show the link and code. They open the link, check the code, choose the agent (or create one) and approve. -- `oauth_type` `agent`: already connected. Don't connect again: that rotates - the agent's secret and disconnects any other computer using it. +- `oauth_type` `agent`: already connected. Don't connect again: approval may + replace an existing connection, or add one and leave this profile's old + connection live until disconnected in Basecamp. - Anything else: a person's login is stored. Ask; never connect an agent over it. @@ -527,7 +531,8 @@ Failed checks worth knowing: - **Stream ticket refused.** The account event feed isn't enabled for this account or agent. That's a Basecamp setting the person has to ask for. - **Scope: not full access.** The agent couldn't reply. Connect again with full - access, with the person's consent, since it rotates the secret. + access only with consent: approval may replace the existing connection, + or add one and leave the read-only connection live. Check the approval page. **Never edit connect.json by hand**, and never read it directly: use `basecamp connect show -P '' --json`, which checks the file is safe @@ -538,11 +543,12 @@ first. Every change goes through setup, which keeps whatever you don't pass. The background task notifies you when the connector exits. Read the end of its output and tell the person what it means: -- **"was disconnected in Basecamp, or connected on another computer"**: run +- **"was disconnected in Basecamp, or replaced by a newer connection"**: run `basecamp connect setup -P ''` (it reconnects with one approval), then start it again. -- **Already running**: another connector for this profile holds its lock, on - this computer or in another session. Don't start a second. +- **Already running**: another connector holds the profile's lock in this + state directory. Don't start a second. An older build holding legacy state + must be stopped and its existing profile restarted on this build first. - **`BASECAMP_TOKEN` is set**: the person unsets it, then you start it again. - **Anything else**: show the error, and run `basecamp connect doctor -P ''`. From 222621a044280f66a4d26ba13f0f2aaca67037b8 Mon Sep 17 00:00:00 2001 From: Rob Zolkos Date: Wed, 14 Oct 2026 16:01:16 -0400 Subject: [PATCH 3/6] Prove held work survives adoption and remains operable 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. --- docs/agent-connections-rollout.md | 7 +++- internal/commands/connect_operator.go | 5 +-- .../commands/connect_profile_review_test.go | 33 +++++++++++++++++++ .../commands/connect_profile_state_test.go | 11 ++++++- internal/connector/statedir_test.go | 30 +++++++++++++++++ 5 files changed, 82 insertions(+), 4 deletions(-) diff --git a/docs/agent-connections-rollout.md b/docs/agent-connections-rollout.md index c90579dd4..e981db33b 100644 --- a/docs/agent-connections-rollout.md +++ b/docs/agent-connections-rollout.md @@ -22,7 +22,12 @@ the approval page; do not reconnect merely to upgrade the CLI. 3. Check `basecamp connect status -P ` and `basecamp connect doctor -P `. Only if intended, use `basecamp connect release -P `; release clears this profile's hold, not other profiles' holds. Held records - still require individual decisions. + still require individual decisions. This does not promise replay: the existing + handoff age guard closes eligible requests from before a newly started run + as `discarded` / `before_this_run`, retaining their ledger rows. Requests + admitted during a hold are not necessarily individually held. Use a fresh + trusted event for handoff proof after a restart; inspect historical records + rather than assuming release will hand them off. 4. For a legacy shadow, run `basecamp connect --shadow -P ` first, then stop it. Only then may `basecamp connect shadow promote -P ` move that profile's shadow ledger into normal state. Offline promotion diff --git a/internal/commands/connect_operator.go b/internal/commands/connect_operator.go index af42db8ed..40848593a 100644 --- a/internal/commands/connect_operator.go +++ b/internal/commands/connect_operator.go @@ -467,8 +467,9 @@ func newConnectReleaseCmd() *cobra.Command { Use: "release", Short: "Clear the hold: requests are handed off again", Long: `Clear this profile's durable hold that basecamp connect --hold or shadow promote set. -Records that arrived after the hold are handed off; held records stay held -until each is discarded.`, +Eligible requests from this run are handed off; held records stay held until +each is discarded. Restarting does not replay older requests: the existing +handoff age guard closes them with reason before_this_run.`, Example: ` basecamp connect release -P agent`, Args: cobra.NoArgs, RunE: func(cmd *cobra.Command, _ []string) error { diff --git a/internal/commands/connect_profile_review_test.go b/internal/commands/connect_profile_review_test.go index f7318512c..6dd8c4cd2 100644 --- a/internal/commands/connect_profile_review_test.go +++ b/internal/commands/connect_profile_review_test.go @@ -3,6 +3,7 @@ package commands import ( + "context" "os" "path/filepath" "strings" @@ -61,6 +62,38 @@ func TestAFreshSiblingNamesWhereTheEarlierHeldStateLives(t *testing.T) { assert.Contains(t, b.out.String(), "sibling="+filepath.Join(connectStateRoot(t), connector.StateDirName(f.file.AccountID, f.file.Agent.PersonID, "agent"))) } +func TestMigratedHeldWorkRemainsAddressableByTheOperator(t *testing.T) { + f := newOperatorFixture(t) + seedLegacyLedger(t, f.s, f.file) + r := startProfileConnector(t, f.s, "agent") + r.awaitRunning(t) + r.cancel() + _ = r.awaitExit(t) + + out, err := f.run(t, output.FormatJSON, "release") + require.NoError(t, err, out) + assert.Contains(t, out, `"still_held": 1`) + dir := filepath.Join(connectStateRoot(t), connector.StateDirName(f.file.AccountID, f.file.Agent.PersonID, "agent")) + assertNotHeld(t, dir) + l, err := connector.OpenLedgerReadOnly(context.Background(), filepath.Join(dir, connector.LedgerFile)) + require.NoError(t, err) + record, found, err := l.Get(context.Background(), 41) + require.NoError(t, err) + require.True(t, found) + assert.Equal(t, connector.StateHeld, record.State) + require.NoError(t, l.Close()) + + out, err = f.run(t, output.FormatJSON, "discard", "41") + require.NoError(t, err, out) + l, err = connector.OpenLedgerReadOnly(context.Background(), filepath.Join(dir, connector.LedgerFile)) + require.NoError(t, err) + defer func() { _ = l.Close() }() + record, found, err = l.Get(context.Background(), 41) + require.NoError(t, err) + require.True(t, found) + assert.Equal(t, connector.StateDiscarded, record.State) +} + func TestStatePathsRefuseInvalidProfilesBeforeCreatingAnything(t *testing.T) { f := newOperatorFixture(t) for _, profile := range []string{"", "../escape", "a/b", "a\\b", ".", "profile name"} { diff --git a/internal/commands/connect_profile_state_test.go b/internal/commands/connect_profile_state_test.go index 456e40df5..f8af6570c 100644 --- a/internal/commands/connect_profile_state_test.go +++ b/internal/commands/connect_profile_state_test.go @@ -18,6 +18,7 @@ import ( "github.com/basecamp/basecamp-cli/internal/appctx" "github.com/basecamp/basecamp-cli/internal/connector" + "github.com/basecamp/basecamp-cli/internal/connector/admission" "github.com/basecamp/basecamp-cli/internal/connector/setup" "github.com/basecamp/basecamp-cli/internal/output" ) @@ -125,6 +126,13 @@ func seedLegacyLedger(t *testing.T, s *connectSetupServer, file setup.File, shad _, err = l.RecordSeen(ctx, eventfeed.Event{ID: 41, Kind: "comment_created", EventType: "comment.created", Action: "created", CreatedAt: time.Now(), BucketID: setupProject, CreatorID: setupOperatorPerson, RecordingID: 77}, connector.LanePoll) require.NoError(t, err) + _, err = l.Admission().Commit(ctx, admission.Verdict{ + EventID: 41, EventType: "comment.created", BucketID: setupProject, RecordingID: 77, RequesterID: setupOperatorPerson, + State: admission.StateAdmitted, Trigger: admission.TriggerMentioned, ConversationKey: "recording:77", Served: true, + Reply: &admission.ReplyDestination{Kind: admission.ReplyComment, RecordingID: 77}, + Snapshot: &admission.Snapshot{Type: "Comment", Content: "synthetic held request", UpdatedAt: time.Now()}, + }) + require.NoError(t, err) require.NoError(t, l.Save(ctx, connectRunKey(t, s, file, shadow...), legacyPosition)) _, err = l.SetHold(ctx, "local:tester", connector.HoldByOperator) require.NoError(t, err) @@ -141,9 +149,10 @@ func assertLegacyLedgerKept(t *testing.T, dir string, s *connectSetupServer, fil require.NoError(t, err) require.True(t, ok, "the cursor came with the directory") assert.Equal(t, legacyPosition, position) - _, found, err := l.Get(context.Background(), 41) + record, found, err := l.Get(context.Background(), 41) require.NoError(t, err) assert.True(t, found, "the ledger's records came with it") + assert.Equal(t, connector.StateHeld, record.State, "held work is not orphaned or dispatched on upgrade") _, held, err := l.HoldMarker(context.Background()) require.NoError(t, err) assert.True(t, held, "migration must never release the hold") diff --git a/internal/connector/statedir_test.go b/internal/connector/statedir_test.go index 4c2fa80f8..03def783b 100644 --- a/internal/connector/statedir_test.go +++ b/internal/connector/statedir_test.go @@ -124,6 +124,36 @@ func TestAdoptRefusesWhileAConnectorRunsInTheLegacyDirectory(t *testing.T) { assert.ErrorIs(t, err, os.ErrNotExist) } +func TestCanonicalAccountSpellingsMeetTheSameProfileLock(t *testing.T) { + parent := stateParent(t) + dir := filepath.Join(parent, StateDirName("2914079", 52007412, "agent-a")) + lock, err := AcquireInstanceLock(dir, "2914079", 52007412, time.Now()) + require.NoError(t, err) + t.Cleanup(func() { _ = lock.Release() }) + alias := filepath.Join(parent, StateDirName("02914079", 52007412, "agent-a")) + _, err = AcquireInstanceLock(alias, "02914079", 52007412, time.Now()) + assert.ErrorIs(t, err, ErrAlreadyRunning) +} + +func TestAdoptionRefusesInvalidStateNamesWithoutTouchingLegacyState(t *testing.T) { + parent := stateParent(t) + legacy := legacyStateDir(t, parent) + for _, profile := range []string{"", "../other", "a/b", "a\\\\b", "a b"} { + adopted, err := AdoptLegacyStateDir(parent, "2914079", 52007412, profile) + require.Error(t, err) + assert.False(t, adopted) + } + for _, account := range []string{"", "../other", "0", "not-an-account"} { + adopted, err := AdoptLegacyStateDir(parent, account, 52007412, "agent-a") + require.Error(t, err) + assert.False(t, adopted) + } + entries, err := os.ReadDir(parent) + require.NoError(t, err) + require.Len(t, entries, 1) + assert.Equal(t, filepath.Base(legacy), entries[0].Name()) +} + func TestTwoProfilesHoldTheirLocksAtOnceAndOneProfileOnlyOnce(t *testing.T) { parent := stateParent(t) now := time.Now() From 78c88a859c75320e1f01b33e2dfdf47e76fc9660 Mon Sep 17 00:00:00 2001 From: Rob Zolkos Date: Wed, 14 Oct 2026 16:03:42 -0400 Subject: [PATCH 4/6] Preserve legacy lock errors through diagnostics --- internal/commands/connect_run.go | 5 ++++- internal/connector/statedir.go | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/internal/commands/connect_run.go b/internal/commands/connect_run.go index e99f54f51..070d81d25 100644 --- a/internal/commands/connect_run.go +++ b/internal/commands/connect_run.go @@ -488,9 +488,12 @@ func connectLockFree(stateDir, account string, agentID int64) error { var err error if filepath.Base(stateDir) == connector.LegacyStateDirName(account, agentID) { lock, err = connector.AcquireLegacyInstanceLock(stateDir, account, agentID) - if err != nil { + if errors.Is(err, connector.ErrAlreadyRunning) { return &output.Error{Code: output.CodeLockUnavailable, Message: err.Error()} } + if err != nil { + return err + } } else { lock, err = acquireConnectLock(stateDir, account, agentID) if err != nil { diff --git a/internal/connector/statedir.go b/internal/connector/statedir.go index eeb637734..b6d40ba13 100644 --- a/internal/connector/statedir.go +++ b/internal/connector/statedir.go @@ -69,7 +69,7 @@ func AcquireLegacyInstanceLock(dir, accountID string, agentID int64) (*InstanceL } func LegacyLockError(dir string, err error) error { - return fmt.Errorf("connector: an older connector for this agent holds %s (%v); stop it and restart its existing profile on this build before starting another profile: %w", dir, err, ErrAlreadyRunning) + return fmt.Errorf("connector: an older connector for this agent holds %s; stop it and restart its existing profile on this build before starting another profile (%w)", dir, err) } func pathPresent(path string) (bool, error) { From 13109a9413f7c2ac942e6f120a62cc663b24b330 Mon Sep 17 00:00:00 2001 From: Rob Zolkos Date: Wed, 14 Oct 2026 16:08:15 -0400 Subject: [PATCH 5/6] Keep legacy account spelling while canonicalizing profile locks 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. --- docs/agent-connections-rollout.md | 6 +++++- internal/commands/connect_run.go | 9 +++++---- internal/connector/statedir.go | 8 +++++--- internal/connector/statedir_test.go | 17 +++++++++++++++++ 4 files changed, 32 insertions(+), 8 deletions(-) diff --git a/docs/agent-connections-rollout.md b/docs/agent-connections-rollout.md index e981db33b..6c89aac05 100644 --- a/docs/agent-connections-rollout.md +++ b/docs/agent-connections-rollout.md @@ -40,7 +40,11 @@ the approval page; do not reconnect merely to upgrade the CLI. or cross-profile claim. State is under `$XDG_STATE_HOME/basecamp/connect` and `connect-shadow` (default -`~/.local/state/basecamp`). Before adoption, status, doctor and operator commands +`~/.local/state/basecamp`). New profile directories canonicalize numeric account +aliases so padded account ids cannot bypass the same-profile lock. Legacy +lookup preserves the saved account spelling used by older builds; the adoption +WARN names both paths. On downgrade, restore that saved spelling if padded. +Before adoption, status, doctor and operator commands read the legacy folder without moving it. A working credential that reads back as a different person cannot adopt. The first verified profile owns the old state, not necessarily the first profile configured locally. A fresh sibling diff --git a/internal/commands/connect_run.go b/internal/commands/connect_run.go index 070d81d25..1ae4c38c7 100644 --- a/internal/commands/connect_run.go +++ b/internal/commands/connect_run.go @@ -140,9 +140,10 @@ func warnConnectSiblingState(logger *slog.Logger, dir string, file setup.File, p logger.Warn("connector: could not inspect sibling state", "error", err) return } - prefix := connector.LegacyStateDirName(file.AccountID, file.Agent.PersonID) + prefix := strings.TrimSuffix(connector.StateDirName(file.AccountID, file.Agent.PersonID, ""), "-") + legacyName := connector.LegacyStateDirName(file.AccountID, file.Agent.PersonID) for _, entry := range entries { - if entry.Name() == filepath.Base(dir) || (entry.Name() != prefix && !strings.HasPrefix(entry.Name(), prefix+"-")) { + if entry.Name() == filepath.Base(dir) || (entry.Name() != prefix && entry.Name() != legacyName && !strings.HasPrefix(entry.Name(), prefix+"-")) { continue } sibling := filepath.Join(filepath.Dir(dir), entry.Name()) @@ -231,7 +232,7 @@ func runConnect(cmd *cobra.Command, f *connectRunFlags) error { if err != nil { return output.ErrUsage("The connector's state directory cannot be used: " + err.Error()) } - if err := connectLockFree(stateDir, account, file.Agent.PersonID); err != nil { + if err := connectLockFree(stateDir, file.AccountID, file.Agent.PersonID); err != nil { return err } @@ -269,7 +270,7 @@ func runConnect(cmd *cobra.Command, f *connectRunFlags) error { stateDir, adopted, err := prepareConnectStateDir(name, file, f.shadow) if adopted { logger.Warn("connector: adopted legacy state; cursor, hold and held records belong to this profile now", - "profile", name, "from", filepath.Join(filepath.Dir(stateDir), connector.LegacyStateDirName(account, agentID)), "to", stateDir) + "profile", name, "from", filepath.Join(filepath.Dir(stateDir), connector.LegacyStateDirName(file.AccountID, agentID)), "to", stateDir) } switch { case errors.Is(err, connector.ErrAlreadyRunning): diff --git a/internal/connector/statedir.go b/internal/connector/statedir.go index b6d40ba13..7cecefe15 100644 --- a/internal/connector/statedir.go +++ b/internal/connector/statedir.go @@ -12,13 +12,15 @@ import ( ) func StateDirName(accountID string, agentID int64, profile string) string { + if account, err := strconv.ParseUint(accountID, 10, 64); err == nil { + accountID = strconv.FormatUint(account, 10) + } return LegacyStateDirName(accountID, agentID) + "-" + profile } +// LegacyStateDirName preserves the saved spelling: older builds used it +// verbatim. Only the new profile directory canonicalizes account aliases. func LegacyStateDirName(accountID string, agentID int64) string { - if account, err := strconv.ParseUint(accountID, 10, 64); err == nil { - accountID = strconv.FormatUint(account, 10) - } return accountID + "-" + strconv.FormatInt(agentID, 10) } diff --git a/internal/connector/statedir_test.go b/internal/connector/statedir_test.go index 03def783b..dc3938649 100644 --- a/internal/connector/statedir_test.go +++ b/internal/connector/statedir_test.go @@ -135,6 +135,23 @@ func TestCanonicalAccountSpellingsMeetTheSameProfileLock(t *testing.T) { assert.ErrorIs(t, err, ErrAlreadyRunning) } +func TestAdoptionPreservesAnOlderBuildsPaddedAccountDirectory(t *testing.T) { + parent := stateParent(t) + legacy := filepath.Join(parent, LegacyStateDirName("02914079", 52007412)) + require.Equal(t, "02914079-52007412", filepath.Base(legacy)) + require.NoError(t, os.Mkdir(legacy, 0o700)) + require.NoError(t, os.WriteFile(filepath.Join(legacy, LedgerFile), []byte("the held legacy ledger"), 0o600)) + adopted, err := AdoptLegacyStateDir(parent, "02914079", 52007412, "agent-a") + require.NoError(t, err) + require.True(t, adopted) + dir := filepath.Join(parent, StateDirName("2914079", 52007412, "agent-a")) + data, err := os.ReadFile(filepath.Join(dir, LedgerFile)) + require.NoError(t, err) + assert.Equal(t, "the held legacy ledger", string(data)) + _, err = os.Lstat(legacy) + assert.ErrorIs(t, err, os.ErrNotExist) +} + func TestAdoptionRefusesInvalidStateNamesWithoutTouchingLegacyState(t *testing.T) { parent := stateParent(t) legacy := legacyStateDir(t, parent) From cd5c29558c571c243928497ca92dbad196dee2bd Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Fri, 16 Oct 2026 21:57:35 -0700 Subject: [PATCH 6/6] Say approvals add or replace by what Basecamp enables for the agent's 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. --- README.md | 4 ++-- docs/agent-connections-rollout.md | 7 ++++--- internal/commands/auth_agent.go | 6 +++--- internal/commands/connect.go | 6 +++--- internal/commands/connect_copy_contract_test.go | 11 ++++++----- skills/basecamp-connect/SKILL.md | 6 +++--- 6 files changed, 21 insertions(+), 19 deletions(-) diff --git a/README.md b/README.md index fb577eb4a..1cbc08fe7 100644 --- a/README.md +++ b/README.md @@ -271,8 +271,8 @@ basecamp connect -P my-agent # the watcher ``` An approval may add a connection or replace an existing one: read the approval -page before proceeding. Personal agents can have several connections when -Basecamp enables adding them. Otherwise, and always for shared agents, a new +page before proceeding. Personal and team agents can have several connections +where Basecamp enables adding them for that kind of agent. Otherwise a new approval replaces the existing connection. This applies to both the CLI and Basecamp's HTTP handshake. Do not reconnect just to upgrade the CLI. diff --git a/docs/agent-connections-rollout.md b/docs/agent-connections-rollout.md index 6c89aac05..d81275f47 100644 --- a/docs/agent-connections-rollout.md +++ b/docs/agent-connections-rollout.md @@ -3,9 +3,10 @@ This CLI can merge and release before the server changes. It stores the connection secret as opaque data; no connection API, self-disconnect endpoint, or server feature switch is required. A second approval **may replace the -existing connection**: it does on the current server, for shared agents, and -for personal agents while adding connections is disabled. Only personal agents -on a server with adding enabled gain another independent connection. Consult +existing connection**: it does on the current server, and for any agent whose +kind (personal or team) doesn't have adding connections enabled. Where +Basecamp has enabled it for the agent's kind, personal first and team later, +the approval gains another independent connection instead. Consult the approval page; do not reconnect merely to upgrade the CLI. ## Upgrade one host diff --git a/internal/commands/auth_agent.go b/internal/commands/auth_agent.go index 2be1b3e86..e9c03f800 100644 --- a/internal/commands/auth_agent.go +++ b/internal/commands/auth_agent.go @@ -53,9 +53,9 @@ whenever it needs one, so there is no token to paste and none to renew by hand. An approval may add a connection or replace an existing one; read the -approval page before proceeding. Personal agents can keep several connections -when Basecamp has enabled adding them. Otherwise, and always for shared -agents, connecting replaces the existing connection, wherever it runs. +approval page before proceeding. An agent, personal or team, keeps several +connections where Basecamp has enabled adding them for its kind. Otherwise +connecting replaces the existing connection, wherever it runs. The device name defaults to this computer's name. Give each profile its own --device-name when several on this computer connect the same agent. diff --git a/internal/commands/connect.go b/internal/commands/connect.go index 2d739b031..12cb00ac9 100644 --- a/internal/commands/connect.go +++ b/internal/commands/connect.go @@ -61,9 +61,9 @@ off nothing. --hold sets this profile's durable hold: intake and admission run, nothing is handed off, and earlier records wait for review, until basecamp connect release for this profile. Linux and macOS only. -One connector runs per profile in this state directory. Personal agents can -run several connections when Basecamp enables adding them; otherwise, and for -shared agents, a new approval replaces the existing connection. Each profile +One connector runs per profile in this state directory. An agent can run +several connections where Basecamp enables adding them for its kind; otherwise +a new approval replaces the existing connection. Each profile serves its own projects and has its own hold. When two connections serve the same project, a request both admit can be answered twice. diff --git a/internal/commands/connect_copy_contract_test.go b/internal/commands/connect_copy_contract_test.go index d01b87eb4..cb64b8309 100644 --- a/internal/commands/connect_copy_contract_test.go +++ b/internal/commands/connect_copy_contract_test.go @@ -23,16 +23,17 @@ func TestTheSkillDocumentsTheDisconnectLineItGets(t *testing.T) { assert.Contains(t, message, "or replaced") } -func TestConnectionHelpIsSafeBeforeServerAddsAndForSharedAgents(t *testing.T) { +func TestConnectionHelpIsSafeBeforeServerAddsForEitherKindOfAgent(t *testing.T) { help := newAuthAgentConnectCmd().Long assert.Contains(t, help, "approval may add a connection or replace") - assert.Contains(t, help, "always for shared") - assert.Contains(t, help, "when Basecamp has enabled adding") + assert.Contains(t, help, "An agent, personal or team, keeps several") + assert.Contains(t, help, "where Basecamp has enabled adding them for its kind") + assert.NotContains(t, help, "always for shared") assert.NotContains(t, help, "Each approval adds") assert.Contains(t, help, "old connection stays live") assert.Contains(t, help, "not removed or revoked") help = NewConnectCmd().Long - assert.Contains(t, help, "otherwise, and for") - assert.Contains(t, help, "shared agents") + assert.Contains(t, help, "where Basecamp enables adding them for its kind") + assert.NotContains(t, help, "shared agents") assert.Contains(t, help, "own hold") } diff --git a/skills/basecamp-connect/SKILL.md b/skills/basecamp-connect/SKILL.md index 2e2283be5..fb7fca4ec 100644 --- a/skills/basecamp-connect/SKILL.md +++ b/skills/basecamp-connect/SKILL.md @@ -429,9 +429,9 @@ Ask only what you can't find out. create` for an agent: that signs in a person. If Basecamp's instructions page connected this agent over HTTP before (a `~/.basecamp-agent/credentials.json`; never read it), ask whether the agent - is personal or shared. Warn before approval: shared agents and personal - agents without adding enabled replace the existing connection; an - add-capable personal agent keeps both live. Read the approval page, never + is personal or shared. Warn before approval: an agent whose kind (personal + or shared) doesn't have adding enabled replaces the existing connection; + one whose kind does keeps both live. Read the approval page, never assume adding is enabled. Don't delete credential files. If the owner says nothing uses the old file, have them disconnect its connection in Basecamp, then remove that unused file themselves. Then connect the agent with