Add basecamp connect setup: record trust, write connect.json, check readiness

.surface

@@ -668,6 +668,8 @@ CMD basecamp config show
 CMD basecamp config trust
 CMD basecamp config unset
 CMD basecamp config untrust
+CMD basecamp connect
+CMD basecamp connect setup
 CMD basecamp docs
 CMD basecamp docs archive
 CMD basecamp docs doc
@@ -5341,6 +5343,62 @@ FLAG basecamp config untrust --stats type=bool
 FLAG basecamp config untrust --styled type=bool
 FLAG basecamp config untrust --todolist type=string
 FLAG basecamp config untrust --verbose type=count
+FLAG basecamp connect --account type=string
+FLAG basecamp connect --agent type=bool
+FLAG basecamp connect --cache-dir type=string
+FLAG basecamp connect --count type=bool
+FLAG basecamp connect --help type=bool
+FLAG basecamp connect --hints type=bool
+FLAG basecamp connect --ids-only type=bool
+FLAG basecamp connect --in type=string
+FLAG basecamp connect --jq type=string
+FLAG basecamp connect --json type=bool
+FLAG basecamp connect --markdown type=bool
+FLAG basecamp connect --md type=bool
+FLAG basecamp connect --no-hints type=bool
+FLAG basecamp connect --no-stats type=bool
+FLAG basecamp connect --profile type=string
+FLAG basecamp connect --project type=string
+FLAG basecamp connect --quiet type=bool
+FLAG basecamp connect --stats type=bool
+FLAG basecamp connect --styled type=bool
+FLAG basecamp connect --todolist type=string
+FLAG basecamp connect --verbose type=count
+FLAG basecamp connect setup --account type=string
+FLAG basecamp connect setup --agent type=bool
+FLAG basecamp connect setup --allow type=stringArray
+FLAG basecamp connect setup --cache-dir type=string
+FLAG basecamp connect setup --class type=stringArray
+FLAG basecamp connect setup --concurrency type=int
+FLAG basecamp connect setup --count type=bool
+FLAG basecamp connect setup --deadline type=duration
+FLAG basecamp connect setup --driver type=string
+FLAG basecamp connect setup --expect-identity type=string
+FLAG basecamp connect setup --help type=bool
+FLAG basecamp connect setup --hints type=bool
+FLAG basecamp connect setup --ids-only type=bool
+FLAG basecamp connect setup --in type=string
+FLAG basecamp connect setup --jq type=string
+FLAG basecamp connect setup --json type=bool
+FLAG basecamp connect setup --markdown type=bool
+FLAG basecamp connect setup --md type=bool
+FLAG basecamp connect setup --no-hints type=bool
+FLAG basecamp connect setup --no-stats type=bool
+FLAG basecamp connect setup --no-watch-completions type=stringArray
+FLAG basecamp connect setup --operator type=string
+FLAG basecamp connect setup --operator-profile type=string
+FLAG basecamp connect setup --profile type=string
+FLAG basecamp connect setup --project type=string
+FLAG basecamp connect setup --quiet type=bool
+FLAG basecamp connect setup --remove-route type=stringArray
+FLAG basecamp connect setup --route type=stringArray
+FLAG basecamp connect setup --stats type=bool
+FLAG basecamp connect setup --styled type=bool
+FLAG basecamp connect setup --todolist type=string
+FLAG basecamp connect setup --trust type=string
+FLAG basecamp connect setup --verbose type=count
+FLAG basecamp connect setup --watch-completions type=stringArray
+FLAG basecamp connect setup --worktrees type=bool
 FLAG basecamp docs --account type=string
 FLAG basecamp docs --agent type=bool
 FLAG basecamp docs --cache-dir type=string
@@ -18517,6 +18575,8 @@ SUB basecamp config show
 SUB basecamp config trust
 SUB basecamp config unset
 SUB basecamp config untrust
+SUB basecamp connect
+SUB basecamp connect setup
 SUB basecamp docs
 SUB basecamp docs archive
 SUB basecamp docs doc

e2e/smoke/smoke_lifecycle.bats

@@ -16,6 +16,10 @@ load smoke_helper
   mark_out_of_scope "Waits on a person approving the connection in a browser"
 }
 
+@test "connect setup is out of scope" {
+  mark_out_of_scope "Needs a connected agent profile and writes local connector policy — covered by Go tests in internal/commands and internal/connector/setup"
+}
+
 @test "auth refresh is out of scope" {
   mark_out_of_scope "Requires OAuth credentials"
 }

internal/auth/auth.go

@@ -143,11 +143,16 @@ func NewManager(cfg *config.Config, httpClient *http.Client) *Manager {
 	}
 }
 
+// ProfileCredentialKey is the credential key a named profile's credential
+// is stored under, for a caller that must name one profile's credential
+// without a manager bound to it.
+func ProfileCredentialKey(profile string) string { return "profile:" + profile }
+
 // credentialKey returns the storage key for credentials.
 // Profile mode: "profile:<name>", No-profile mode: origin URL.
 func (m *Manager) credentialKey() string {
 	if m.cfg.ActiveProfile != "" {
-		return "profile:" + m.cfg.ActiveProfile
+		return ProfileCredentialKey(m.cfg.ActiveProfile)
 	}
 	return config.NormalizeBaseURL(m.cfg.BaseURL)
 }

internal/auth/lock.go

@@ -7,6 +7,7 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
+	"sync/atomic"
 	"time"
 
 	"github.com/gofrs/flock"
@@ -128,6 +129,73 @@ func (s *Store) withKeyLock(ctx context.Context, key string, fn func() error) er
 	return fn()
 }
 
+// WithCredential runs fn with one key's credential while holding that key's
+// cross-process lock, so no other process's login, refresh, import or
+// logout for the key can land while fn runs. A caller that must act on a
+// credential being what it just read — writing a file that names the
+// identity it authenticates as, say — does its read, its decision and its
+// write inside fn.
+//
+// A host that cannot lock at all is an error here, not a warning: the
+// callers that reach for this are the ones whose guarantee is the lock.
+//
+// fn must not take the same key's lock again — no AccessToken, no
+// StoredAccessToken, no SetUserIdentity, no nested WithCredential. Locks
+// are per open file description, so a second acquire in this process waits
+// on itself until the wait bound expires. fn must not make a network
+// request either: every other process's work on the key queues behind it.
+//
+// A key with nothing stored is reported as ErrNoCredential, never as a nil
+// credential.
+func (s *Store) WithCredential(ctx context.Context, key string, fn func(HeldCredential) error) error {
+	release, err := s.lockFile(lockRequest{name: keyLockName(key), what: "credential", done: ctx.Done(), cause: ctx.Err, require: true})
+	if err != nil {
+		return err
+	}
+	defer release()
+	creds, err := s.load(key, lockRequest{done: ctx.Done(), cause: ctx.Err})
+	if err != nil {
+		return err
+	}
+	held := &heldCredential{creds: creds, key: key}
+	defer held.released.Store(true)
+	return fn(held)
+}
+
+// HeldCredential is a credential read while its key's lock is held, and the
+// proof a write can ask for: a function that must not run without the lock
+// takes one. Only WithCredential makes one, it stops counting when the lock
+// is released, and Held is the only way to read it — the interface carries
+// nothing a wrapper could answer for.
+type HeldCredential interface {
+	// heldUnderLock cannot be implemented outside this package. It can be
+	// promoted by embedding the interface, which is why nothing reads a
+	// proof through methods: Held is the only reader.
+	heldUnderLock()
+}
+
+type heldCredential struct {
+	creds    *Credentials
+	key      string
+	released atomic.Bool
+}
+
+func (h *heldCredential) valid() bool { return !h.released.Load() }
+
+func (h *heldCredential) heldUnderLock() {}
+
+// Held reads a lock proof: the credential and key it was made with, and
+// whether the lock is still held. Anything this package did not make — a
+// nil, a struct embedding the interface — is reported as not held, whatever
+// its own methods say.
+func Held(h HeldCredential) (creds *Credentials, key string, held bool) {
+	own, ok := h.(*heldCredential)
+	if !ok || own == nil || !own.valid() || own.creds == nil || own.key == "" {
+		return nil, "", false
+	}
+	return own.creds, own.key, true
+}
+
 // withStoreLock runs fn while holding the whole-store lock. fn must be one
 // store operation and must not make a network request: every process's
 // Save and Delete queue behind it.
@@ -237,6 +305,11 @@ type lockRequest struct {
 	wait  time.Duration
 	done  <-chan struct{}
 	cause func() error
+	// require refuses the unlocked fall-through: a caller whose guarantee
+	// IS the lock (it decides on a credential and writes a file naming what
+	// that credential authenticates as) gets an error where an ordinary
+	// caller would get a warning and an unsynchronized run.
+	require bool
 }
 
 // lockFile takes the lock named name — exclusive, or shared when the
@@ -271,7 +344,15 @@ func (s *Store) lockFile(req lockRequest) (func(), error) {
 		return s.unlocked(req, err.Error())
 	}
 
-	fl := flock.New(filepath.Join(s.lockDir(), req.name))
+	lockPath := filepath.Join(s.lockDir(), req.name)
+	if req.require {
+		// A required lock is only a lock if every process takes the same
+		// inode: refuse a lock directory or file this user does not own.
+		if err := requirePrivateLockPath(s.lockDir(), lockPath); err != nil {
+			return s.unlocked(req, err.Error())
+		}
+	}
+	fl := flock.New(lockPath)
 	try := fl.TryLock
 	if req.shared {
 		try = fl.TryRLock
@@ -352,6 +433,16 @@ func (s *Store) unlocked(req lockRequest, reason string) (func(), error) {
 	if err := lockCause(req.cause); err != nil {
 		return noop, err
 	}
+	if req.require {
+		// The caller asked for the lock itself, not for best effort: there
+		// is nothing to fall through to.
+		return noop, &output.Error{
+			Code:    output.CodeLockUnavailable,
+			Message: fmt.Sprintf("This host cannot lock the %s: %s", req.what, richtext.SanitizeSingleLine(reason)),
+			Hint: "Nothing was changed. Other commands still work; connector setup is what needs the lock, so point XDG_CONFIG_HOME at a filesystem that supports locking " +
+				"(some network and FUSE mounts do not).",
+		}
+	}
 	s.warnUnlockable(req.what, reason)
 	return noop, nil
 }

internal/auth/lock_test.go

@@ -1097,3 +1097,138 @@ func TestStoreSaveKeepsOtherKeys(t *testing.T) {
 	require.NoError(t, json.Unmarshal(raw, &all))
 	assert.Len(t, all, 8)
 }
+
+// WithCredential holds the key's lock while fn runs: a caller that decides
+// on a credential and writes inside fn cannot have it replaced underneath.
+func TestWithCredentialHoldsTheKeyLock(t *testing.T) {
+	t.Setenv("BASECAMP_NO_KEYRING", "1")
+	dir := t.TempDir()
+	store := NewStore(dir)
+	const key = "profile:agent"
+	require.NoError(t, store.Save(key, &Credentials{AccessToken: "first", OAuthType: "agent"}))
+
+	inside := make(chan struct{})
+	replaced := make(chan error, 1)
+	err := store.WithCredential(context.Background(), key, func(held HeldCredential) error {
+		creds, heldKey, ok := Held(held)
+		require.True(t, ok)
+		assert.Equal(t, "first", creds.AccessToken)
+		assert.Equal(t, key, heldKey)
+		go func() {
+			close(inside)
+			replaced <- NewStore(dir).withKeyLock(context.Background(), key, func() error {
+				return NewStore(dir).Save(key, &Credentials{AccessToken: "second", OAuthType: "agent"})
+			})
+		}()
+		<-inside
+		time.Sleep(200 * time.Millisecond) // let the other writer reach the lock
+		// The other writer is waiting on the key lock, so what was read is
+		// still what is stored.
+		again, err := store.Load(key)
+		require.NoError(t, err)
+		assert.Equal(t, "first", again.AccessToken)
+		return nil
+	})
+	require.NoError(t, err)
+	require.NoError(t, <-replaced)
+
+	after, err := store.Load(key)
+	require.NoError(t, err)
+	assert.Equal(t, "second", after.AccessToken, "the waiting writer lands once the section ends")
+}
+
+// A host that cannot lock at all is an error for WithCredential, never a
+// warning and an unsynchronized run: its callers' guarantee is the lock.
+func TestWithCredentialRefusesWhenTheHostCannotLock(t *testing.T) {
+	t.Setenv("BASECAMP_NO_KEYRING", "1")
+	dir := t.TempDir()
+	store := NewStore(dir)
+	const key = "profile:agent"
+	require.NoError(t, store.Save(key, &Credentials{AccessToken: "first", OAuthType: "agent"}))
+	// No lock file can be created where nothing may be written.
+	locks := store.lockDir()
+	require.NoError(t, os.MkdirAll(locks, 0o700))
+	require.NoError(t, os.Chmod(locks, 0o500))
+	t.Cleanup(func() { _ = os.Chmod(locks, 0o700) })
+
+	ran := false
+	err := store.WithCredential(context.Background(), key, func(HeldCredential) error {
+		ran = true
+		return nil
+	})
+	require.Error(t, err)
+	assert.False(t, ran, "the section never runs unlocked")
+	var apiErr *output.Error
+	require.ErrorAs(t, err, &apiErr)
+	assert.Equal(t, output.CodeLockUnavailable, apiErr.Code)
+}
+
+// The proof stops being valid when the lock is released, so a callback that
+// keeps it cannot write with it afterwards.
+func TestHeldCredentialIsInvalidAfterTheLockIsReleased(t *testing.T) {
+	t.Setenv("BASECAMP_NO_KEYRING", "1")
+	store := NewStore(t.TempDir())
+	const key = "profile:agent"
+	require.NoError(t, store.Save(key, &Credentials{AccessToken: "first", OAuthType: "agent"}))
+
+	var kept HeldCredential
+	require.NoError(t, store.WithCredential(context.Background(), key, func(held HeldCredential) error {
+		kept = held
+		creds, heldKey, ok := Held(held)
+		require.True(t, ok)
+		require.NotNil(t, creds)
+		assert.Equal(t, key, heldKey)
+		return nil
+	}))
+
+	creds, heldKey, ok := Held(kept)
+	assert.False(t, ok, "the lock is gone")
+	assert.Nil(t, creds)
+	assert.Empty(t, heldKey)
+}
+
+// A required lock is only a lock if every process takes the same inode, so
+// a lock directory or lock file this user does not own is refused rather
+// than used.
+func TestWithCredentialRefusesAnUntrustworthyLockPath(t *testing.T) {
+	t.Setenv("BASECAMP_NO_KEYRING", "1")
+	const key = "profile:agent"
+
+	for name, plant := range map[string]func(t *testing.T, dir string){
+		"the lock directory is a symlink": func(t *testing.T, dir string) {
+			locks := filepath.Join(dir, "locks")
+			require.NoError(t, os.RemoveAll(locks))
+			require.NoError(t, os.Symlink(t.TempDir(), locks))
+		},
+		"the lock directory is writable by others": func(t *testing.T, dir string) {
+			locks := filepath.Join(dir, "locks")
+			require.NoError(t, os.MkdirAll(locks, 0o700))
+			require.NoError(t, os.Chmod(locks, 0o777))
+		},
+		"the lock file is a symlink": func(t *testing.T, dir string) {
+			locks := filepath.Join(dir, "locks")
+			require.NoError(t, os.MkdirAll(locks, 0o700))
+			planted := filepath.Join(locks, keyLockName(key))
+			require.NoError(t, os.RemoveAll(planted))
+			require.NoError(t, os.Symlink(filepath.Join(t.TempDir(), "planted"), planted))
+		},
+	} {
+		t.Run(name, func(t *testing.T) {
+			dir := t.TempDir()
+			store := NewStore(dir)
+			require.NoError(t, store.Save(key, &Credentials{AccessToken: "first", OAuthType: "agent"}))
+			plant(t, dir)
+
+			ran := false
+			err := store.WithCredential(context.Background(), key, func(HeldCredential) error {
+				ran = true
+				return nil
+			})
+			require.Error(t, err)
+			assert.False(t, ran, "the section never runs on a lock it cannot trust")
+			var apiErr *output.Error
+			require.ErrorAs(t, err, &apiErr)
+			assert.Equal(t, output.CodeLockUnavailable, apiErr.Code)
+		})
+	}
+}

internal/auth/lockpriv_other.go

@@ -0,0 +1,8 @@
+//go:build !unix
+
+package auth
+
+// Without Unix ownership and modes there is nothing to check here: a
+// required lock rests on the platform's own access control for the user's
+// configuration directory.
+func requirePrivateLockPath(string, string) error { return nil }