Repository navigation
fix: bound authentication recovery across transport fallback - #2162
markstuart-oai merged 38 commits into
Conversation
🦋 Changeset detectedLatest commit: 8a22c44 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
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. |
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Approved after coordinator and independent static review. The native patch matches the reviewed Python authentication correction: terminal HTTP 401 stops outer sampling retries, client refresh remains bounded, and rejected WebSocket handshakes fall back before prompt dispatch. TS regressions cover preserved status/code/request ID, completed tool effects and cancellation. All eight hosted checks passed on this exact commit. No candidate code or local tests ran.
markstuart-oai
changed the title
|
@codex review |
🛡️ 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. |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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.
Re-reviewed d08cfd8e0c9a53d6f0bc00b3c53501f20e22fb05: approved. Authentication recovery now shares one budget across WebSocket-to-HTTP fallback; terminal 403 joins 401 as non-retryable. Preconnect, warmup and subsequent requests retain independent budgets. Reviewed credential rotation, cancellation, effect preservation and metadata coverage. Hosted CI is still running; this is a code-review approval, not a claim that all checks passed. Static review only; no local tests run.
d08cfd8 to
291d975
Compare
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Package relocation and authentication patch rebasing preserve behavior. Updated recovery test distinguishes startup preconnect from the first turn HTTP recovery budget. Node tests, coverage and Windows paths passed; native packaging pending. Static and independent review; hosted CI follow-up remains open; no candidate code or tests executed.
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Base integration preserves the authentication runtime patch byte-for-byte. Stronger tests distinguish startup prewarm from sampling retries, verify identical retry bodies and prevent repeated tool effects. Static and independent review; hosted CI pending. No candidate code or tests executed.
|
@codex review |
🛡️ 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. |
![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: 231f7a6aa9
ℹ️ 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".
|
Codex Review: Didn't find any major issues. Keep it up! 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 after static follow-up and independent review. The six-file increment integrates the reviewed child-tracing changes from #2179 at 44f6e78. Child creation context, including absent context, remains authoritative for later steps and direct/CodeMode approvals. Existing feature changes remain intact; no new actionable findings. Hosted native and Node CI remain pending. No candidate code or tests were executed locally.
…-owned-child-trace-context
🛡️ 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. |
|
Codex Review: Didn't find any major issues. Hooray! 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". |
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Approved at 15b9bf5. Authentication recovery remains intact; inherited A2A serialized-result and response-encoding changes compose without regression. Shared tracing/child-admission changes match reviewed 1875314, and patch hashes match the manifests.
Current reported hosted checks: 7 SUCCESS, 1 IN_PROGRESS. Coordinator and independent static review found no actionable integration regressions. No candidate code or local tests executed; no CI reruns requested.
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
🛡️ 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 8a22c44. This merge has exactly the same complete Git tree (8e405a35f9b3176328de6c4f5d91203fbeee150a) as previously approved 15b9bf5. No new code or substantive unresolved feedback was introduced. Coordinator and independent static review found no actionable source issues.
Current reported hosted CI: {"IN_PROGRESS": 3, "SUCCESS": 5}. No candidate code, local tests, generators, or CI reruns were executed by this reviewer.
Builds on merged #2172. Current head:
8a22c449, with the exact source tree qualified at15b9bf5d. The merge adds current v1 ancestry and resolves the historical series conflict without changing source.Summary
A final HTTP 401 or 403 must end sampling after the provider client's normal credential recovery. Share that recovery budget across WebSocket-to-HTTP fallback so one model request cannot recover twice. Preserve final HTTP metadata and completed tool effects. Independent startup preconnect and later requests keep their own recovery opportunity.
This authentication change targets current v1, including the extracted
@openai/agents-codexruntime. Earlier heada580fde7included the reviewed child-tracing composition from #2179 on v1512b066a; its auth feature patch remains unchanged.Test plan
At
15b9bf5d, the normal development producer verifies Core77c98d47, all 133 native inputs and six physical products. All 26 source authentication cases and 18 authentication tests from freshly installed normal archives pass with no skips. Installed ESM/CommonJS imports pass.The first cached product failed 11 cases. Invalidating the five changed Cargo source owners and rebuilding the same source cleared all failures; the matching binding changed. Source preparation, cumulative TypeScript checks and integration source reviews pass. Current-head human approval and hosted code/security reviews are complete on
8a22c449. Two independent reviews passed on the complete authentication change. After review, the required full SDK script passed 9,549 tests with 116 skips across 368 passing test files. Source, component, repository and native-product guards remained stable; changeset validation passed.Final verification used private candidate
f57f795c, which integrates actual v1ee46a255with public head8a22c449. Its tree isac61a34f3e54e7724ecf4d9375ae434fe024df95. The authentication diff and its recorded dependencies remain unchanged across the upstream advances. All 78 native cases in the three updated upstream suites also pass on the qualified Core77c98d47product.Earlier qualification:
The authentic Linux release from alpha run 42008988825 passes all 18 installed-package auth controls and all 26 supported source cases, with no skips. The supported command is
pnpm test:native packages/agents-codex/test/native/parity-error-metadata.test.ts --maxWorkers=1. Normal source preparation/audit, test typing, focused lint, five-package JS builds and commit hooks also pass.The earlier CI merge producer
633cb827had the identical complete source tree as that earlier qualified head. All five physical SDK archives, starter archive, native input/product hashes, release receipt, helper modes and installed-app/starter receipts verify. The executor fixture retains its original producer identity; its full 45-crate dependency and source closure matches this candidate. Earlier combined Core is52d2ec0d.The controls cover terminal 401/403, fresh later requests, cancellation, retained metadata and single tool effects. Two cases finish a real tool turn, await the Core-facing carrier's close handshake, then submit another public request. Each observes two rejected turn-kind WebSocket upgrades and exactly one final HTTP attempt, without repeating the effect. The separate unknown-outcome guard remains unchanged. Earlier Core controls cover successful credential rotation and cross-transport recovery; those production functions are unchanged here.
At that earlier qualification, Linux alpha passed while the other platform and final gates remained pending. The separate policy-isolation correction later landed in #2179. Earlier producers retain their own historical evidence; current validation is recorded above.
Issue number
This covers terminal provider authentication failures under REQ62 and the protection of completed tool effects under REQ63. It does not claim complete acceptance of either requirement.
Checks
.agents/skills/code-change-verification/scripts/run.shAGENTS.md