Repository navigation
Keep basecamp connect running, and starting, through a rate-limited or failing token endpoint - #811
Conversation
|
@codex review |
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
![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: 247204b957
ℹ️ 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".
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Valid HTTP-date and oversized Retry-After values are discarded, causing premature retries.
Review effort: Balanced
Findings: 1
Open (3)
What changed in this PR
Keeps active connectors alive when agent token renewal encounters retryable HTTP failures.
Changes:
- Translates retryable CLI token errors into SDK errors.
- Preserves numeric
Retry-Aftervalues. - Adds feed classification tests.
[!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/connect.go |
Adapts retryable renewal errors for SDK classification. |
internal/commands/connect_token_retry_test.go |
Tests transient, throttled, and fatal renewals. |
internal/auth/revoke.go |
Carries parsed retry delays through the error chain. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@codex review |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
All reported issues were addressed
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Converged on 20d3240: CI is green (CodeQL and all four race shards included), Copilot approved this head with no suppressed or previously-missed items in its review body, Codex found no issues on this head, cubic's check passed on it, the branch merges cleanly into main, and there are 0 unresolved threads. All review findings were fixed rather than declined:
Left for a human decision, as the PR body explains: the connector's startup identity read still exits during a rate limit or hold, and a refusal that arrives while the feed is idle on its websocket is not acted on here. This PR is based on main, not stacked on #787. If #787's |
A 503 or a bot user's OAuth refresh could name a Retry-After of years, and the connector would wait it: a hang, not a wait. Every wait a server names, whatever the status or token path, now goes through one cap, auth.MaxServerWait, the bound #787 already puts on a mint hold, so a held rate limit and every other named wait agree. When the server asked for longer, the start and the feed say so once: what it asked for, and what is waited.
796cba0 to
cc2f30e
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cc2f30e4a6
ℹ️ 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".
MaxServerWait was an hour, while #787 writes a rate-limit hold under the thirty-minute bound its Retry-After parser applies: an agent's 429 was capped at thirty minutes and every other named wait at an hour. The one cap is now that thirty minutes, and the hold is written under it by name. A 429 held for the cap keeps what the server asked for, so the start and the feed say both.
|
@codex review |
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a4438209e8
ℹ️ 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".
Two parsers read the header: the agent-connect one, which #787's hold used, read a count too large for an int as no header at all and held for the one-minute default, while the connector's saturated it to the cap. The hold was then shorter than the wait the connector reported, and another process could mint inside it. retryAfter now reads through retryAfterSeconds and applies its own ceiling, so there is one parser and the hold and the reported wait agree.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0921b56798
ℹ️ 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".
A bot user's refresh reaches the start wrapped in this package's SDK error, whose wait is already capped, and the start's log read only that outer error: it said the cap as if the server had asked for it. Every SDK error down the chain is read now, so the log says what was asked and what is waited.
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Review threads: 8 resolved (7 fixed, 1 declined with the reasoning in its thread).
The wait rule, which applies to any further variant: wait exactly what the server named, on any retryable status, never longer than the one cap (auth.MaxServerWait, 30 minutes, the bound #787 writes a rate-limit hold under). Otherwise use the feed's own backoff. Every Retry-After goes through one parser. A wait capped below what the server asked for is logged once, with both durations. Head ecc90a7:
|
The failure
When the agent token endpoint answers a renewal with a 429 or a 5xx, or doesn't answer at all,
basecamp connectstops.managerTokenshanded it the CLI's own error, so the feed read every non-transport failure as unrecoverable and ended the run.Since #814 the connector is started from the person's Claude session by
/basecamp-connect, and nothing restarts it. A connector that exits on a brief rate limit stays down until someone notices, and requests made while it's down are not picked up later.The fix
One rule for the feed and the start.
tokenRetryis the only classifier.feedTokens, which hands it every token failure already classified as the SDK's error type.awaitConnectToken.The status decides first. A response that arrived is classified by its status, even if its body was cut off or too large to read.
invalid_client/invalid_grantis a refusal.A request that got no response is a network failure, and it's retried. That covers a dropped or refused connection, DNS, a timeout, and TLS or proxy failures. Retrying a misconfigured TLS or proxy setup forever is accepted on purpose: the process stays visibly alive and logs why.
Never retried: an egress-policy block (
surfguard.ErrBlocked), or a URL the client can't send.How long it waits: exactly the wait the server named, never longer than one cap. Otherwise it uses the feed's backoff. Nothing else sets it.
auth.MaxServerWait, 30 minutes: the bound Remember the token endpoint's refusal of an agent's secret #787 already writes a rate-limit hold under, so a held rate limit and every other named wait agree.The start waits instead of exiting. It logs each wait once and keeps trying. A SIGTERM, an interrupt or a canceled context ends the wait promptly, even mid-renewal, with the connector's usual exit codes, and a second signal forces the exit. Any other failure at start still reads "Could not read who profile … is". Before waiting, the start checks the instance lock under the agent that connect.json names, taking it and releasing it at once. A second connector for a running agent is therefore refused immediately instead of waiting out a rate limit first. The lock is held for the run only once the credential is proven to be that agent, so a start holding some other agent's credential doesn't block the real connector while it waits.
The skill knows.
/basecamp-connecttreats the "could not get the agent's token yet" line as a connector that is waiting, not one that failed, and leaves it running.What stays fatal
A credential refusal exits as a disconnect, in the running feed and at start alike. That means
invalid_client,invalid_grant, or a bare 401/403, including one whose body was cut off. Other non-retryable answers keep their current behaviour.Dropped since earlier revisions
connect service, so the CLI no longer writes a systemd unit. The start waiting still matters, more so now: nothing restarts the connector at all.Tests
connect_token_retry_test.go: the live feed throughfeedTokens, against a fake token endpoint:invalid_client, a bare 401, or a truncated 401 → unrecoverable, and recognised as a disconnectconnect_start_wait_test.go: the start:agent_hold_test: pins the wait a held mint carries.go test ./internal/auth/... ./internal/commands/... ./internal/connector/... -race -count=2and lint, compared with a cleanorigin/mainon the same machine, show no new failures.Side effects
setup.ErrorTextnow renders such a failure asHTTP 429/HTTP 503.Not addressed
A credential refusal that arrives while the feed sits idle on its websocket. The feed makes no token requests while idle, so nothing notices a refusal until it next needs a token.