Stop concurrent basecamp processes from losing a credential, and let an agent authenticate by jorgemanrubia · Pull Request #720 · basecamp/basecamp-cli

jorgemanrubia · 2026-09-15T07:52:40Z

Run several basecamp processes against one profile — a connector beside a
handful of basecamp mcp -P <agent> workers, a shell with xargs -P, a script
running commands in parallel — and credentials go missing. The two shapes it
takes are token refresh failed: and Not authenticated for profile: on a
profile that was logged in a moment ago. Both are read-modify-write races over
the credential store, and nothing kept the processes out of each other's way.

The other half of this: a Basecamp agent has no person behind it, no browser to
sign in with, and is granted no refresh token at all. Neither login flow the CLI
has applies to it, and the refresh path has nothing to work with.

Originally tracked in CLI auth: cross-process refresh lock and client_credentials grant.

Why

The connector will run beside several worker processes sharing one profile, so
the first failure stops being an occasional annoyance and becomes the normal
case. And it authenticates as a Basecamp agent principal, which the CLI cannot
do at all today — the bot-user device-flow profile is a stand-in.

What

The races. The authorization server rotates the refresh token on every
grant, so two processes refreshing at once leave one of them holding a token the
server has already retired — and the credential is gone at the next command.
Separately, the keyring fallback keeps every profile in one credentials.json
and rewrites the whole document on each write, so two processes saving
different profiles at the same time drop one of them outright.

Both are fixed by holding a lock across the whole cycle rather than around the
write, and the two cycles have different extents, so there are two. A per-key
lock spans load → renew → save, network round trip included; a whole-store lock
covers the file backend's document rewrite and nothing else. Every mutation of a
credential now runs under the first: the refresh path, a forced auth refresh,
both interactive logins, an imported token, the identity writeback, logout and
revoke. The renewal path also re-reads and re-checks the expiry under the
lock, so twenty concurrent commands make one token request between them rather
than twenty; a credential comfortably inside its lifetime takes no lock at all.

The lock is an flock(2) on a file in the config directory, not a lock on the
store — which is usually the OS keyring, where a file lock would guard nothing.
What has to be serialized is the CLI's own critical section, and every process
reaches it through the same code whatever backend holds the bytes. The kernel
drops an flock when the holder's descriptor closes, process death included, so
there is no stale lock to reap. Three limits are documented in lock.go, all of
which degrade to the behavior that shipped before rather than to something
worse: filesystems whose flock is local-only or a no-op, a host where the lock
cannot be created at all (warn once and run unsynchronized, because failing
closed would break setups that work today), and processes that disagree about
XDG_CONFIG_HOME while sharing a keyring.

Agents. A new credential kind whose durable credential is the OAuth client
itself: a client id and secret that mint a one-hour self-token, stored together
in the keyring where one is available, because they — not a refresh token — are
what survives an expiry. Renewal dispatches on the kind: an agent mints another
token, everything else refreshes as before. Nothing is written unless the mint
succeeded and nothing is ever deleted, so a server having a bad minute leaves the
client credentials in place for the next command instead of emptying the store
and asking for a login. Logout forgets an agent credential without attempting a
revocation and says why: the same client would mint another, so rotating the
secret in Basecamp is what ends an agent's access.

basecamp auth login --with-client-credentials --client-id <id> -P agent --account 999
is how the credential gets there, with the secret on stdin — never an argument,
so it stays out of shell history and the process table. It is the shape
--with-token already established for machine identities and now shares its
profile handling.

Out of scope

The grant is minted by the connection ceremony bc3 PR 12314 provides, which is
not merged. Nothing here has been exercised against production: it is built
to the documented shape and covered against a local token endpoint. The live
check waits on that PR and on card 04; until then the bot-user device-flow
profile stays the fallback.

Testing

The concurrency proof runs twenty real processes, not goroutines and not
mocks: the test binary re-invokes itself, each child stops at a barrier, and the
parent releases all of them at once only after every one has reported ready —
without that, the scheduler is free to run each to completion before the next
starts and a test of concurrency never becomes concurrent. The mock
authorization server rotates the refresh token and refuses a spent one, so a
lost rotation is counted rather than tolerated: twenty usable tokens, one grant
between them, no stale presentations, and the credential intact afterwards.
A second test does the same for twenty different profiles saving at once, and a
third holds the lock from the parent and shows a real child's save blocked until
it is released. Every one of them was checked by sabotage — with the locks made
no-ops they fail, three runs out of three, with exactly the two errors from the
report.

Around that: the lock's own behavior (held across the network round trip, a dead
holder's lock is free immediately, an unlockable host proceeds, a canceled save
waiting on the lock writes nothing), the agent grant (mints instead of
refreshing, re-mints only on expiry, keeps the credential when a mint fails,
refuses an unsafe stored endpoint, refuses a redirect, classifies a refusal
against a rate limit against a server fault), and the command surface (creates
the profile, JSON envelope, stores nothing when refused, the secret never
echoed, bad stdin, flag exclusivity) — plus e2e coverage for the new flags.

bin/ci is green, and go test -race ./internal/auth/ -count=2 is clean.
Locally that is everything except lint-actions, which needs actionlint and
zizmor; no workflow files are touched.

Review

Reviewed adversarially with Codex over three rounds — lock scope versus the real
read-modify-write window, a process dying or hanging under the lock, NFS and
flock caveats, the keyring path where a file lock guards nothing, expiry races
and clock skew on re-mint, whether a failed mint can wipe a good credential, and
whether the concurrency test proves what it claims. Eight findings applied and
two argued down on the record; the last commit is that work.

…rite Several `basecamp` processes routinely share one profile — a connector beside a handful of `basecamp mcp -P <agent>` workers, a shell running commands in parallel, a script with xargs -P — and nothing kept them out of each other's way. Two failures followed from that, both of them read-modify-write races over the credential store: * Two processes enter the refresh window together, both refresh, both save. The authorization server rotates the refresh token on every grant, so the second save overwrites the first's rotated token. The next command is refused with invalid_grant and the credential is gone: "token refresh failed". * The file store (the keyring fallback) keeps every credential in one credentials.json and rewrites the whole document on every Save. Two processes saving DIFFERENT keys at the same time drop one outright — a profile that was logged in a moment ago reports "Not authenticated for profile:". Both are fixed by holding a lock across the whole cycle rather than around the write, and the two cycles have different extents, so there are two locks. The KEY lock (one file per credential key) is held across load → renew → save, network round trip included; it is per key so a refresh on one profile does not stall a command on another. The STORE lock (one file for the whole store) is held for the duration of a single Save or Delete and nothing else. The order is always key → store, so they cannot deadlock. Every read-modify-write over a credential now runs under the key lock: the refresh path, a forced `auth refresh`, an imported token, the identity writeback, logout and revoke. The renewal path also re-reads and re-checks the expiry UNDER the lock, so twenty concurrent commands make one token request between them rather than twenty. A credential comfortably inside its lifetime takes no lock at all — reading one key is atomic in both backends. The lock is an flock(2) on a file in the config directory, not a lock on the store, which is usually the OS keyring where a file lock would guard nothing: what has to be serialized is the CLI's own critical section, and every process reaches it through this code whatever backend holds the bytes. The kernel drops an flock when the holder's descriptor closes, process death included, so a crashed holder cannot wedge it and there is no stale lock to reap. A host where the lock cannot be created at all warns once and proceeds unsynchronized — that is where every process was before this; a lock that is HELD past the wait is an error, because someone is demonstrably inside the critical section.

A Basecamp "agent" is a principal with no person behind it. It has no browser to send anywhere, so neither the device flow nor the authorization-code flow applies, and the tokens it is issued carry NO REFRESH TOKEN. The grant itself is the durable credential: a client id and a client secret, which mint a fresh one-hour self-token whenever the last one is spent. So a new credential kind, `oauth_type: "agent"`, alongside the existing bc5, launchpad and imported-token ones. Renewal dispatches on it: an agent credential mints another self-token (RFC 6749 §4.4, POSTed to the same token endpoint, on the same BC5 egress lane, with the same endpoint checks and redirect refusal every other credential-carrying POST gets), and every other kind refreshes as before. The client id and secret are stored with the token they mint — in the OS keyring wherever one is available, which is where every other profile's refresh token already lives — because they, not a refresh token, are what survives an expiry. Nothing is written unless a mint succeeded, and nothing is ever deleted: a mint that fails leaves the stored credential exactly as it was, so the next command tries again with the same client rather than finding an empty store and an instruction to log in. A refused mint is an auth-class error (the client is wrong, and a fresh login is the remedy); a 5xx or a network failure keeps its own class and stays retryable. A token response with no expires_in is stored with the grant's documented one-hour lifetime rather than as non-expiring, since zero is how every other path here spells "never renew" and would strand a token the server retires within the hour. Logout forgets an agent credential without attempting a revocation and says why: revoking one self-token accomplishes nothing while the client that minted it can mint another — rotating the client secret in Basecamp is what ends an agent's access. `auth status` reports an agent credential as renewable, since having no refresh token is not the same as having nothing to renew with. The grant is minted by the connection ceremony bc3 PR 12314 adds, which is not deployed yet, so this is built to the documented shape and covered against a local token endpoint. It has not been exercised against production, and the bot-user device-flow profile stays the fallback until it has.

The way an agent's credential gets into the store. `--client-id <id>` names the agent's OAuth client, the matching secret is read from stdin — never an argument, so it stays out of shell history and the process table — and one mint proves the pair before anything is written: a refused mint stores neither a credential nor a profile entry. It is the shape `--with-token` already established for machine identities, and now shares its profile handling: the credential goes under a named profile, which is created alongside it when `--account <id>` says which account it addresses. That resolution — which profile, what the config file needs written, and every refusal reachable without the secret — moves into `resolveHeadlessProfile` and `profileTarget.register`, which both logins call; the wording each login uses is a field rather than a fork. `--expect-identity` is refused rather than ignored: an agent is not a person, so there is no identity to assert.

Eight of them, all in the seams between the lock and what it was meant to cover: * Both interactive logins wrote their credential outside the lock, so a refresh already in flight in another process could save the credential the person had just replaced back over the new one. Verification and persistence become one step that takes the lock, and the last check that the login was not canceled is made under it — the wait for a lock is itself a window a Ctrl-C lands in — with a matching check under the store lock, and the grant handed back when nothing was kept. * The whole-store lock wrapped keyring writes, which it was never for: the keyring writes one entry at a time and a locked macOS keychain leaves `security` waiting on a person inside a child nothing can cancel, with every other profile's save queued behind it. It now applies to the file backend, whose whole-document rewrite is the reason it exists. The migration, which does that file work precisely when the keyring is in use, takes it unconditionally. * A refused mint reported a 429 as "authenticate again", which would put an automated caller into a re-login loop against a server asking it to slow down. Anything that is not a 4xx-about-the-client now goes through the package's own classifier, so a 429 keeps its Retry-After and a 5xx stays retryable. * A token whose whole lifetime is shorter than the five-minute refresh window would be re-minted on every single command. The renewal moment is stored with the token and scales with the lifetime the server actually issued. * The agent login stored the credential before the profile entry it belongs to, so a config file that could be read but not written left an orphaned client secret. The entry is committed between the mint that proves the client and the write that stores it. And the concurrency tests were not proving what they claimed: starting children from goroutines does not make them overlap. They now stop at a barrier and are released together once every one of them is ready, with a test that holds the lock from the parent and shows a real second process's save blocked until it is released. All of them were checked by sabotage — made no-ops, the locks' tests fail.

Copilot

🟡 Changes recommended

Cancellation can still permit credential deletion, and an unpaired --client-id is silently ignored.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds cross-process credential locking and OAuth client-credentials authentication for Basecamp agents.

Changes:

Serializes credential renewal and file-store mutations.
Adds agent login, token minting, renewal, and logout behavior.
Expands documentation and concurrency/authentication tests.

[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.

File summaries

File	Description
`.surface`	Records the new login flags.
`README.md`	Documents agent authentication.
`e2e/auth.bats`	Tests the new CLI surface.
`skills/basecamp/SKILL.md`	Adds agent login guidance.
`internal/auth/agent.go`	Implements agent token minting and renewal.
`internal/auth/agent_test.go`	Tests agent credential behavior.
`internal/auth/auth.go`	Integrates locking and renewal dispatch.
`internal/auth/keyring.go`	Locks credential-store mutations.
`internal/auth/lock.go`	Implements cross-process locks.
`internal/auth/lock_test.go`	Tests lock behavior and process races.
`internal/auth/revoke.go`	Locks revocation and handles agents.
`internal/commands/auth.go`	Adds the agent login command flow.
`internal/commands/auth_login_agent_test.go`	Tests agent login commands.

Review details

Files reviewed: 13/13 changed files
Comments generated: 2
Review effort level: Balanced

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

…ray --client-id is refused The lock was taken before the caller's cancellation was checked, so a command that had already been stopped would acquire it and carry on into the critical section — a logout, above all, which revokes the token server-side and deletes it locally and rechecks nothing. The check is made before the first attempt and again the instant the lock is in hand, which is its own window: a waiter can be canceled exactly as the holder releases. Making it there rather than at each call site is the point — every caller is covered without any of them having to remember. And `--client-id` passed without `--with-client-credentials` was silently ignored while the ordinary interactive login ran, so a command naming which agent to authenticate as would sign a person in instead and say nothing about the flag it dropped.

A host where the lock cannot be created at all carries on unsynchronized, which is right for a live command and wrong for one that has been stopped: the cancellation check sat after those returns, so on such a host a canceled save still wrote and a canceled logout still revoked and deleted. It now comes first, before the fall-throughs and before the lock is attempted at all, and again where a failed acquisition falls through.

Copilot

🟡 Changes recommended

Cancellation, agent recovery guidance, and concurrent profile registration have unresolved correctness issues.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

internal/auth/agent.go:310

m.errAuth attaches basecamp auth login -P <profile> as the remedy. For an agent mint refusal that command starts the interactive person flow, so following the hint can replace the agent credential with a person's login. Agent failures need a client-credentials-specific hint that tells the operator to pipe the rotated secret and reuse the stored client ID/profile.

Files reviewed: 13/13 changed files
Comments generated: 2
Review effort level: Balanced

The check at the top of lockFile is not a gate on its own: the steps between it and the fall-throughs touch a filesystem that can block, so a wait can end while one of them runs. Collecting the fall-throughs into a single helper is what makes that structural — none of them can forget the check, and a new one cannot be added without it.

The default remedy on an auth_required error is `basecamp auth login -P <profile>`, which signs a PERSON in. An operator following it after a refused mint would replace the agent's credential with their own and find out later. Every auth failure about an agent credential now carries the client-credentials login instead, addressed to the same profile, naming the client the credential already holds, and saying where the secret goes.

jorgemanrubia · 2026-09-15T08:11:56Z

🤖 On the suppressed finding from the second review (internal/auth/agent.go:310 — the mint refusal carrying basecamp auth login -P <profile> as its remedy): applied in 2b130d3.

That was a genuinely dangerous hint. Following it after a refused mint starts the interactive flow and stores a person's credential under the agent's profile, and nothing would have said so. Every auth failure about an agent credential now carries basecamp auth login --with-client-credentials --client-id <the client it already holds> -P <the same profile>, prefixed with where the secret goes, and TestAgentFailureNeverPointsAtTheInteractiveLogin pins that for both a refused mint and a credential that cannot mint at all.

Copilot

🟡 Changes recommended

Concurrent profile registration and stale metadata writes remain possible, while agent recovery hints and token-import cancellation are incorrect.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (5)

Previously missed (3) — in code that hasn't changed since the last review.

internal/auth/auth.go:1169

The new key-lock wait is detached from the command context here. If --with-token is waiting behind another process and its Cobra context is canceled, context.Background() keeps waiting and can store the imported token after cancellation. Thread cmd.Context() into the import API and use SaveContext, as the other login paths do.
internal/auth/agent.go:311
A refused client or rotated secret is returned through m.errAuth, whose hint is the ordinary basecamp auth login -P <profile> command. That starts a person login and can replace the agent credential with the wrong credential kind. Return an agent-specific recovery hint that tells the operator to rerun --with-client-credentials with the client ID and current secret on stdin.
internal/auth/auth.go:337
Agent renewal setup failures are passed through m.hintLogin, which directs users to the ordinary interactive login. For an agent missing a stored client ID/secret/endpoint, that remedy changes the credential to a person login instead of repairing it. Use an agent-specific hint for this branch (and preserve more specific non-auth errors).

internal/auth/auth.go:1785

The key lock prevents token fields from being overwritten, but it does not tie this metadata to the credential that produced it. If process A receives identity data, process B replaces the profile credential, and then A enters this block, A loads B's credential and writes A's user identity into it. Pass an expected access token/version and skip the update when the stored credential changed, or include the identity in the original login's locked store operation.

	return m.store.withKeyLock(context.Background(), credKey, func() error {
		creds, err := m.store.Load(credKey)
		if err != nil {
			return err
		}

internal/commands/auth.go:968

BeforeStore does not actually preserve the stated “no orphaned secret” invariant under concurrent agent setup. registerProfile performs an unlocked load/modify/rename of config.json, so two processes creating different profiles can both register from the same snapshot; the last rename drops the other profile, after which both credentials are stored and one client secret is orphaned. Serialize the global-config read/modify/write (ideally in the shared profile writer) before relying on registration as the prerequisite for storing the credential.

		BeforeStore: func(result *auth.LoginResult) error {
			registered, regErr := target.register(app, result.Scope)
			isDefault = registered
			return regErr

Files reviewed: 13/13 changed files
Comments generated: 1
Review effort level: Balanced

Copilot

🟡 Changes recommended

OAuth response handling can expose the client secret and persist unsupported server-provided scopes.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

internal/auth/agent.go:164

The token endpoint's scope is accepted verbatim, although the CLI only supports read or full. On initial login this value is then persisted into the profile by BeforeStore and printed directly in the human success line; a malformed response can therefore create an unsupported profile scope (and can inject terminal controls). Validate any non-empty response scope against the supported values before applying the token, as the interactive login verifier already does in internal/commands/auth.go:1371-1372.

Files reviewed: 13/13 changed files
Comments generated: 1
Review effort level: Balanced

…of what the server says back Two more from the review of the last commit. The agent remedy was attached at the sites that knew about agents, which left every other way an agent credential can fail — a stored token endpoint the CLI refuses to post to, a 401 the SDK classified far from here — carrying the interactive login, the one that stores a person's credential under the agent's profile. It is now decided in one place, from the stored credential, so a failure path added later cannot get it wrong. Where the command cannot be completed from what is stored — no client id, no active profile, which are themselves reasons it is being suggested — it says so with placeholders instead of emitting something that would fail on the error that produced it. And the mint's request carries a client secret, so the response is no longer quoted as it came: the secret is removed verbatim and percent-encoded, and if it survives in some other escaping the excerpt is withheld altogether. Sanitizing a server's text strips control sequences, not credentials, and a diagnostic is not worth a leaked one.

…ved redaction A login's first mint runs before anything is stored, so deriving its remedy from the stored credential made rendering a refusal the thing that reached for the OS keyring — and answered with the interactive login, or a previous person's, for an operation that is explicitly an agent's. The mint path supplies the remedy it already knows instead, from the client id in hand; every other path keeps deriving it from the credential. The redaction had two holes. A control sequence inside the echoed secret ("agent-\x1b[31msecret") matched nothing, and stripping it then put the secret back together on the way to the terminal — so the text is redacted again after sanitizing, not only before. And the percent-encoded check ran through url.QueryUnescape, which refuses a whole string over one bad escape: a stray per-cent sign in the same sentence ("quota 100%") was enough to switch it off. The decode is tolerant now, leaving malformed escapes alone rather than giving up on the string.

Narrowing the credential-refusal codes meant nothing while a 401 or 403 could put a named policy refusal back: a 403 saying access_denied is a policy refusal however it is numbered, and it was still being answered with "pipe the secret in again". The status is consulted only when the server named no reason — which is exactly when nothing but the credentials could have produced it.

Copilot

🟡 Changes recommended

Stored-but-unusable agent credentials can still trigger person login, and some generated recovery commands cannot succeed.

Get a fresh assessment by requesting another Copilot review.

Review details

Files reviewed: 27/27 changed files
Comments generated: 2
Review effort level: Balanced

A profile with no account of its own inherits whatever the global or repo config set, and the recovery command was emitting that as an explicit `--account` — turning an unrelated inherited value into a binding nobody asked for. The login itself refuses exactly that, taking an account only from a flag or the environment, so the suggestion was either a command that fails or one that succeeds wrongly. Only a value this invocation supplied is filled in; anything else keeps the placeholder.

jorgemanrubia · 2026-09-15T10:08:48Z

🤖 The last two suppressed findings, applied in 8665972 and 0e4cf0e (see below), plus one the narrowing had left open:

The recovery command binding an inherited account — right, and it defeated a safeguard that already existed: a profile with no account of its own inherits whatever the global or repo config set, resolveHeadlessProfile refuses to bind that (it takes an account only from a flag or the environment), and the hint was emitting it anyway — so the suggested command either fails or succeeds wrongly. Only a value this invocation supplied is filled in now; anything else keeps the <account-id> placeholder.
The recovery command dropping the scope — applied in 73ce7e9, answered on its thread.
And one more I found while applying yours: narrowing clientRefusalCodes meant nothing while a bare-401/403 fallback could put a named policy refusal back — a 403 {"error":"access_denied"} was still being answered with "pipe the secret in again". The status is consulted only when the server named no reason, which is exactly when nothing but the credentials could have produced it.

Every finding from all ten reviews is now applied or answered on its thread, and Codex has no remaining findings.

Copilot

🔵 Needs a closer look

It changes security-sensitive OAuth and cross-process credential persistence while the agent grant remains untested against production.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

internal/auth/agent.go:607

Returning "agent" here makes the exported LoginResult contract inaccurate: its OAuthType field is still documented as only "bc5" or "launchpad" in internal/auth/auth.go:711-715. Update that field/type documentation to include agent results so callers do not treat this new value as invalid.

Files reviewed: 27/27 changed files
Comments generated: 0 new
Review effort level: Balanced

…ssing A credential that is in the store and holds nothing usable is not "nothing is stored", and a caller reading it that way goes and gets a login — which for an agent profile means signing a person in over it. `basecamp setup` did exactly that. It is a broken credential now, reported with the remedy for what it actually is. And the recovery command stops naming `--account` for a profile entry that exists without one. Whether the login can bind an account there depends on which config layer defines the profile, which is not visible from where the command is built — so for a profile defined in a repo or system config the suggestion was guaranteed to fail with "not the global config's entry". Without it the login refuses by naming what it needs, layer included, which is better guidance than a guess. The create case, where `--account` is required and always accepted, still carries it.

`LoginResult.OAuthType` still documented itself as "bc5" or "launchpad" while `LoginClientCredentials` returns "agent", so a caller reading the contract would treat a valid result as invalid.

jorgemanrubia · 2026-09-15T10:18:10Z

🤖 Three more, applied in 165b754 and 0f1dbd0:

CheckAuthenticated collapsing a stored-but-unusable credential into absence — right, and it is the same hazard in a new place: setup falls through to an interactive login on a false, so an agent credential holding nothing usable would have been replaced by a person's. A credential that is in the store and cannot serve a token is now a broken one, reported with the remedy for what it actually is. TestWizardAuthRefusesRatherThanReplaceABrokenAgent asserts setup refuses and never touches the network; TestAStoredButUnusableCredentialIsNotAbsence covers the manager.
The recovery command's --account for an accountless profile entry — you are right that it cannot be made correct from where it is built. Whether the login can bind an account there depends on which config layer defines the profile, which internal/auth cannot see, so for a profile defined in a repo or system config the suggestion was guaranteed to fail with "not the global config's entry". It is gone for that case: without it the login refuses by naming exactly what it needs, layer included, which is better guidance than a guess. The create case, where --account is required and always accepted, still carries it.
LoginResult.OAuthType's contract — corrected; it documents agent now.

On the headline verdict: yes, and that is the right thing to hold this to. The PR says so in its own words under Out of scope — the client_credentials grant has never been exercised against production, because the ceremony that mints these tokens (bc3 12314) is not merged. It is built to the documented shape, covered against a local token endpoint, and the bot-user device-flow profile stays the fallback until someone has run it for real. The live check is card 04's, not this one's.

Copilot

🟡 Changes recommended

Agent recovery remains broken for accountless profiles, and troubleshooting guidance can misclassify stored agents lacking an access token.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

internal/auth/agent.go:516

For an existing accountless profile this recovery command omits --account, but resolveHeadlessProfile rejects that exact invocation at lines 854–856 before it can reauthenticate. This reintroduces the previously fixed dead-end for globally defined unbound profiles: include an account placeholder whenever the entry has no account so the suggested command can perform the required binding.

	if entry, registered := m.cfg.Profiles[m.cfg.ActiveProfile]; !registered || entry == nil {
		command += " --account " + m.accountToBind()
	}

Files reviewed: 27/27 changed files
Comments generated: 3
Review effort level: Balanced

`auth status` reported a stored credential holding no access token as simply "not logged in", with no `oauth_type` — and every piece of guidance this branch added tells its reader to decide from `oauth_type` how to recover. Absent, it falls through to the interactive login: the way an agent gets replaced by a person, arrived at by following the instructions. The type is reported now even when the token is not, and the three places that give the rule say to follow the CLI's own hint rather than choosing for themselves whenever the type is missing.

Copilot

🔵 Needs a closer look

It changes credential persistence and OAuth behavior across processes while the agent grant remains unverified against production.

Review details

Files reviewed: 27/27 changed files
Comments generated: 0 new
Review effort level: Balanced

"Not logged in" is a statement about the credential, and a store that could not be read makes none. Status was making it anyway — and that is worse here than it sounds, because the rules this branch added tell their reader to decide from `oauth_type` how to recover: absent, they reach for the interactive login, and an agent gets replaced by a person by following the instructions exactly. One load now, with its three answers told apart: nothing usable stored is "not logged in" and a login is the answer; a store that could not be read is reported as the failure it was, claiming nothing; and a credential that is there but holds no usable token is described as what it is, kind included. Asking what a renewal would do moved up with it, which is also what tells the manager what the credential is, so every remedy below names the right login.

jorgemanrubia · 2026-09-15T10:30:23Z

🤖 Last of the thread, applied in a797083 and f168628 — and it is worth naming, because it is the rule this branch added turning back into a code bug.

Every piece of guidance here tells its reader to decide from oauth_type how to recover. But auth status returned early at its not-logged-in branch, which since this branch also covers a stored agent credential holding no access token — so oauth_type was absent, and the instruction fell through to the interactive login. An agent replaced by a person, arrived at by following the instructions exactly.

Fixed at the source rather than in the prose. authStatusReport now does one load and tells its three answers apart:

nothing usable stored → "not logged in", interactive login;
a store that could not be read → reported as the failure it was, claiming nothing (it was saying "not logged in", which is a statement about the credential that an unreadable store does not support);
a credential that is there but holds no usable token → described as what it is, oauth_type included.

The three docs also now say to follow the CLI's own hint rather than choosing for themselves whenever the type is missing.

That closes every finding from all eleven Copilot reviews and thirteen rounds with Codex — each one applied, or argued on its thread. Codex signs off on the code with the documented limitations explicit.

What is deliberately not done here, both written up for their own card: registerProfile's unlocked config.json read-modify-write, and binding the identity writeback to the credential that produced it.

What nobody can check from here, and what I would want before merging: a real Basecamp agent (authenticate, let it re-mint, rotate the secret, follow the reported recovery command, confirm principal/account/scope); the Windows file backend under concurrent processes; a macOS keychain locked after its availability probe in a long-running process; and CI green on the actual merge commit. The first of those is card 04's, and the reason Out of scope says this grant has never run against production.

Copilot

🔵 Needs a closer look

Pre-mint agent login failures can still recommend the interactive person-login flow.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

internal/auth/agent.go:586

An auth-class discovery failure here (for example, an invalid BASECAMP_OAUTH_ISSUER) still carries the default basecamp auth login hint. On a first agent login no agent kind has been remembered yet, so following that hint starts the person login—the replacement path this change is intended to prevent. Return a configuration-specific hint or re-hint the error with the attempted client-credentials command, and cover a pre-mint failure in the agent-login tests.

This issue also appears on line 593 of the same file.

internal/auth/agent.go:594

This pre-mint endpoint-validation error is also an auth error with the generic interactive-login hint. Because the attempted agent credential has not been remembered, a first login against an unsafe discovered endpoint tells the operator to run a person login instead. Replace that generic hint with an agent/configuration-specific remediation before returning it.

	if err := requireSecureOAuthEndpoint("token endpoint", disc.config.TokenEndpoint); err != nil {
		return nil, err

Files reviewed: 28/28 changed files
Comments generated: 0 new
Review effort level: Balanced

Copilot

🔵 Needs a closer look

It changes security-sensitive authentication, persistent secrets, and cross-platform process locking without production validation of the new grant.

Review details

Files reviewed: 28/28 changed files
Comments generated: 0 new
Review effort level: Balanced

jorgemanrubia added 4 commits September 15, 2026 09:32

Copilot AI balanced review requested due to automatic review settings September 15, 2026 07:52

github-actions Bot added commands CLI command implementations tests Tests (unit and e2e) skills Agent skills auth OAuth authentication docs labels Sep 15, 2026

Copilot started reviewing on behalf of jorgemanrubia September 15, 2026 07:53

View session