Repository navigation
Stop sampling retries after a terminal provider authentication failure - #5516
markstuart-oai merged 109 commits into
Conversation
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. |
![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: 89d4bae0cb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Approved at 89d4bae after coordinator and independent static review of all four files, the pinned client/error context and preceding patches. Final HTTP 401 now stops outer sampling retries while preserving status, provider code and request ID. WebSocket fallback occurs before prompt dispatch, and tests cover bounded credential recovery, prior tool effects and cancellation.
Considered the two automated findings: HTTP 403 retry behavior is unchanged baseline policy, explicitly retained here; sharing the authentication budget across WebSocket and HTTP could reduce requests, but each transport remains bounded and a rejected handshake has sent no prompt. Neither demonstrates a correctness regression in this scoped final-401 fix.
Linux and Windows native snapshots passed; macOS and other hosted checks remain pending. No candidate code or local tests ran.
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Approved after coordinator and independent static re-review. One RequestAuthRecovery now spans WebSocket-to-HTTP negotiation, so fallback cannot spend a second authentication recovery budget. Independent requests retain fresh budgets; terminal 403 joins 401 as non-retryable. The tests cover rotation, preconnect separation, fallback, later requests and prior tool effects. Hosted native builds and Rust analysis remain pending.
Static review only; no candidate code or local tests ran.
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Re-reviewed the native test-fixture correction and patch metadata. Rotation fixtures now enable authentication and verify rotated credentials survive WebSocket-to-HTTP fallback. Production recovery-budget and terminal 401/403 behavior is unchanged. No new findings. Hosted native CI is pending. Coordinator and independent static review; no candidate code or local tests executed.
|
@codex review |
|
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 Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Approved at a6e599a. The cumulative integration preserves bounded authentication recovery, completed effects and both provider-error test families. The authentication-retry patch executable payload is unchanged. All four stack heads contain the exact reviewed W3C security patch from 5451026; the inherited tracing blocker is addressed here. Final ancestry commits do not change the reviewed trees.
Hosted checks: 12 SUCCESS, 8 SKIPPED. Windows, typing and macOS sandbox checks were skipped, not passed. Coordinator and independent static review found no actionable integration regressions. No candidate code or local tests executed; no CI reruns requested.
The landed parent has the exact tree already integrated by the private candidate. Preserve that candidate tree and the original public feature ancestry.
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Re-reviewed incremental update from a6e599a. 43 of 44 changed files exactly match current base 6bc55f7; the remaining series manifest updates the inherited request-preparation checksum and resultant tree. Authentication patch and provider-error/WebSocket tests remain byte-identical to the approved head. Patch checksum verified; prior 403 handling and shared recovery-budget fixes remain intact. Coordinator and independent static reviews found no new actionable issues. Hosted CI still running. No candidate code, tests, builds, or CI reruns executed.
Summary
A final HTTP 401 or 403 must stop sampling retries after the provider client finishes its existing authentication recovery. Preserve the final HTTP status, provider code and request ID, along with any completed tool effect.
One sampling request now shares its recovery state between WebSocket attempts and HTTP fallback. A failed recovered credential cannot start another recovery cycle merely by changing transports. Startup preconnect and each later model request keep their own recovery opportunity. Ordinary credential rotation, cancellation, and eligible non-auth retries retain their existing behavior.
Test plan
Current head
b22b3d7b32includes the landed request-editor parent6bc55f7c. Its complete source tree matches the prepared native producer. Normal source preparation and a new wheel/fixture build verify Core98e03aa3; all 116 native inputs and all 50 installed SDK/native files match their recorded products.Fresh installed checks passed 193 cases: 87 auth cases, 15 inherited provider/compaction controls, 43 trace-context cases, 45 native tracing/transport cases, and 3 archive checks. One saved-login replacement case is explicitly skipped inside the local Linux outer sandbox. Two fresh independent source reviews passed on this head.
Required formatting, lint, type checks, and the full SDK gate passed: 4,458 parallel tests passed with 176 skips, followed by 41 serial passes. The run used eight workers. Source fingerprints and all installed SDK/native files stayed unchanged before and after verification. All current hosted platform CI checks completed successfully, including the Linux, macOS, and Windows checks.
Issue number
This covers terminal provider authentication failures under REQ62 and the protection of completed tool effects under REQ63.
Checks