Conversation
An agent credential renews by minting with its client secret, and every process mints for itself. Nothing remembered a refusal, so once a secret was rotated server-side, anything that polled through the CLI presented the dead secret on every poll, indefinitely: in production, one agent's rotated secret was sent about 350 times an hour for more than twelve hours, nearly all of it answered 429 by the abuse tracker the first 401s had tripped. The verdict is now kept on the stored credential (MintHold) and answered locally, before any network I/O, for the client credentials it was about: - invalid_client: held until a login stores a different secret. bc3's rotate! and disconnect! replace or clear the digest and a quarantine is one-way, so nothing makes that secret good again. - invalid_grant (bc3's "Agent is no longer active") and a bare 401/403: held an hour, then tried once, so a reactivated account recovers on its own. - 429: held until Retry-After (60s default, capped), answered as a retryable rate limit. - 5xx, network: nothing held, retried as before. A successful mint clears the hold, and the hold names a fingerprint of the client id and secret, so a new secret is never held to the old one's refusal. The write happens under the credential key's lock, against a fresh read, and only while that read still holds the refused secret. auth status reports the remembered refusal and when it lifts.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
github-actions
Bot
added
commands
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The unlocked fallback still permits a concurrent login to be overwritten, and malformed rate-limit holds can block renewal indefinitely.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Open (4)
What changed in this PR
Persists agent token-mint refusals to prevent repeated invalid credential requests and exposes renewal failures through auth status.
Changes:
- Adds credential-scoped mint holds for refusals and rate limits.
- Clears holds after successful renewal or credential replacement.
- Adds auth-status reporting and tests for hold behavior.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
| File | Description |
|---|---|
internal/commands/auth.go |
Reports agent renewal refusals. |
internal/commands/auth_status_agent_test.go |
Tests auth-status refusal reporting. |
internal/auth/keyring.go |
Persists mint holds with credentials. |
internal/auth/auth.go |
Adds a testable clock. |
internal/auth/agent.go |
Records and clears mint holds. |
internal/auth/agent_test.go |
Updates refusal test setup. |
internal/auth/agent_hold.go |
Implements hold creation, validation, and persistence. |
internal/auth/agent_hold_test.go |
Tests refusal and rate-limit hold behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if current.OAuthType != oauthTypeAgent || agentClientFingerprint(current.ClientID, current.ClientSecret) != hold.Client { | ||
| return | ||
| } | ||
| current.MintHold = hold | ||
| if err := m.store.Save(origin, current); err != nil { |
| if hold.Until != 0 { | ||
| until := time.Unix(hold.Until, 0) | ||
| if !now.Before(until) || until.Sub(now) > maxAgentMintHold { | ||
| return nil | ||
| } | ||
| } |
| // - 429 is held until its Retry-After (a minute when it gives none, at | ||
| // most an hour), and answered in the meantime as a rate limit. |
| assert.Equal(t, 1, calls, "the report asked the token endpoint") | ||
| assert.Contains(t, buf.String(), "the refusal is remembered") | ||
| assert.Contains(t, buf.String(), "invalid_client") | ||
|
|
||
| var envelope struct { | ||
| Notice string `json:"notice"` | ||
| } | ||
| require.NoError(t, json.Unmarshal(buf.Bytes(), &envelope), buf.String()) | ||
| assert.Contains(t, envelope.Notice, "--with-client-credentials") | ||
| assert.NotContains(t, buf.String(), "rotated-away") |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dec3e4d2d6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| wait = defaultAgentRateLimitHold | ||
| } | ||
| hold.Kind = mintHoldRateLimited | ||
| hold.Until = now.Add(min(wait, maxAgentMintHold)).Unix() |
There was a problem hiding this comment.
For a delta-seconds header, converting now.Add(wait) directly to Unix seconds truncates the deadline. If a Retry-After: 300 response is processed at 12:00:00.999, the persisted hold expires at 12:05:00.000, so another mint can be sent almost one second before the server's requested delay has elapsed and potentially trigger another 429. Preserve subsecond precision or round the deadline up when storing it.
Useful? React with 👍 / 👎.
| if hold.Until != 0 { | ||
| until := time.Unix(hold.Until, 0) | ||
| if !now.Before(until) || until.Sub(now) > maxAgentMintHold { | ||
| return nil |
There was a problem hiding this comment.
If a stored rate_limited hold is damaged or written without until, hold.Until == 0 skips this expiry check and the later rate-limit branch returns a local refusal based on the Unix epoch on every invocation. Such a credential can therefore never mint again without being replaced, contrary to the stated fail-open handling for damaged holds. Treat a zero deadline as invalid for rate_limited holds before returning an error.
Useful? React with 👍 / 👎.
| if current.OAuthType != oauthTypeAgent || agentClientFingerprint(current.ClientID, current.ClientSecret) != hold.Client { | ||
| return | ||
| } | ||
| current.MintHold = hold | ||
| if err := m.store.Save(origin, current); err != nil { |
There was a problem hiding this comment.
On a host where the per-credential lock cannot be created and execution deliberately falls through unlocked, a concurrent login can save a new secret after this fingerprint comparison but before Save. This function then writes the previously loaded credential back, restoring the refused secret and overwriting the newly minted token—the exact stale-write case the fresh read is intended to prevent. The comparison and update need to occur in one atomic store operation, or the write must be skipped when that guarantee is unavailable.
Useful? React with 👍 / 👎.
What happened in production
An agent's secret was rotated on the server. After that, a process on the owner's machine kept sending
grant_type=client_credentialswith the old secret toPOST /oauth/tokens, about six times a minute for more than twelve hours: roughly 350 requests an hour, all using a secret that could never work again. bc3 answered 401invalid_client. That tripped its abuse tracker, which blocks the address for five minutes, so most of those requests got 429 withRetry-After. That works out to about 340 × 429 and 12 × 401 an hour.Root cause
An agent credential renews by minting a new token with its client secret, and each process mints on its own.
mintAgentCredentialintentionally forgot every failed mint and left the stored credential "exactly as it was, so the next command tries again".agentMintRefusalalready sorted refusals correctly, but nothing saved the result. So every CLI invocation minted again, and so did every poll of a connector that shells out tobasecampor runsbasecamp connect. A 429'sRetry-Afterwas lost the same way.The fix
The token endpoint's verdict is now saved on the stored credential as
Credentials.MintHold(seeinternal/auth/agent_hold.go). The next mint checks it inresolveAgentMintand fails locally without making a request.invalid_clientOauth::AgentClients.rotate!replaces the secret digest,disconnect!clears it, and quarantine (Oauth::Client::Disabling) can't be undone. No later server state makes that secret valid again, and each retry counts against the abuse tracker.invalid_grant, or a 401/403 with no error codehandle_client_credentials_grant, bc3'sinvalid_grantmeans "Agent is no longer active", which reactivating the account undoes. bc3 always names its refusals, so an unnamed 401/403 came from something else (a proxy or WAF) and doesn't prove the secret is dead.Retry-After(60s if absent, at most 30m sinceretryAftercaps what it parses)rate_limitthat says when the hold ends.--with-client-credentialsremedy. It adds that the refusal is remembered, and when it will be retried if it will be.m.muand the credential key's cross-process lock, which logins also take. It is written against a fresh read, and only if that read still has the refused secret. On a host where locking isn't possible, a login that stored a new secret mid-refusal therefore can't be overwritten.LoginClientCredentialsand the connect handover don't record holds: nothing is stored yet, and a login is a single attempt, not a loop.auth status. It already asksRefreshRefusal, which now reports the hold. An expired agent credential shows "expired, and the renewal would be refused: …" with the agent-login hint. The JSON has a newrenewal_refusedfield.basecamp connect(in-process)I checked this but didn't change anything. The connector gets every token through
managerTokens→Manager.AccessToken. The intake feed treats a failed token (the mint'sErrAuth) as unrecoverable, andrunPartthen cancels the whole run. So the connector already stops on its first refused mint. Before this change, each restart (systemdRestart=always,RestartSec=5, within its start limit) sent the dead secret again. Now a restart fails locally without a request.Admission, outbox and membership only log token failures and back off; they now get a cheap local failure too. One gap remains: a feed sitting idle on its websocket may not notice for a while. Canceling the run from a wrapper around
managerTokenswould close that gap, but it isn't needed to stop the retry storm, so it's left out.A related problem I found but didn't fix: the feed also treats a 429 or 5xx from the token endpoint as unrecoverable, so one throttled mint ends the connector run. The hold keeps the restart loop cheap, but the right fix belongs in the feed's error classification, in a separate change.
Tests
internal/auth/agent_hold_test.go, run against a local token endpoint that counts requests, with the Manager's clock as a test seam:invalid_clientmeans the next mints make zero requests: in the same process, 30 days later, and from a new Manager. The remembered error carries the agent remedy and never the secret.Retry-After: 300means no request at +299s and one at +300s. The success clears the hold. Also tested: noRetry-After, a very large value, the HTTP-date form, and0.invalid_grantand bare 401/403 get one retry per hour, a second refusal holds again, and a later success clears the hold.auth statusreports the remembered refusal and its remedy without calling the token endpoint.I checked that the tests catch the bug they target: with the hold check disabled, the refusal, rate-limit and recheck tests fail, and with the fingerprint check removed, the stale-write test fails.
make lintis clean andgo test ./...passes on Linux.Summary by cubic
Stops agent credentials from repeatedly presenting a secret the token endpoint has refused. Previously a rotated secret was sent again on every poll, tripping the abuse tracker; now the refusal is stored with the credential and answered locally.
Remembered refusals
invalid_clientis held until a login stores a different secret.invalid_grantand bare 401/403 are held an hour, then tried once.Retry-After(60s default, capped at an hour).5xxand network errors are not held and are retried as before.Implementation notes
auth statusreports the remembered refusal and its remedy.Written for commit dec3e4d. Summary will update on new commits.