fix(codex): improve OAuth websocket parity for fast/priority responses#2575
fix(codex): improve OAuth websocket parity for fast/priority responses#2575Willhong wants to merge 4 commits intorouter-for-me:devfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the codex executor to handle websocket session metadata and instructions more robustly, including setting specific headers and default values. It also introduces a default "websockets" attribute for the codex provider in the synthesizer and filestore. Feedback highlights a potential session ID mismatch between headers and body metadata, as well as opportunities to refactor duplicated logic in the websocket executor and the authentication attribute handling.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f9cd37cc14
ℹ️ 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".
|
Addressed the review feedback. This follow-up update includes:
I also pushed the updated branch, and the live instance I am testing against is already running this patched build. |
luispater
left a comment
There was a problem hiding this comment.
Summary:
I found one blocking issue in the websocket request normalization path.
Key findings:
- Blocking:
normalizeCodexWebsocketInstructionsnow injectsYou are a helpful assistant.wheneverinstructionsis missing or blank. That silently changes the effective prompt for requests that previously had no system instructions, so the websocket path stops being behaviorally neutral relative to the existing empty-string handling in the Codex executors. For a compatibility-layer fix, that prompt mutation is too invasive to ship.
Test plan:
- Reviewed the websocket request preparation changes and the Codex auth routing updates.
luispater
left a comment
There was a problem hiding this comment.
Summary:
The websocket parity direction makes sense, but this is a protocol-sensitive path and I do not have enough confidence to approve it without regression coverage.
Key findings:
- Non-blocking: please add targeted tests around websocket request-body normalization and header/session shaping so future parity work does not silently drift again.
- Non-blocking: defaulting file-backed Codex auths to websocket-capable changes routing behavior; a focused regression test around that fallback would make this much safer.
Test plan:
- Reviewed the Codex websocket/body/header changes and the auth synthesis updates.
This is an automated Codex review result and still requires manual verification by a human reviewer.
|
@luispater Addressed the requested change and pushed an update. What changed:
Validation:
Could you take another look when you have a moment? |
Summary
Improve Codex OAuth websocket parity so
service_tier=prioritybehaves closer to official Codex/fastbehavior.This patch focuses on Codex OAuth websocket routing and request-shape parity, and is related to the same Codex websocket transport area touched by #2230 / #2256, but addresses a different problem:
What this changes
1) Default file-backed Codex OAuth auths to websocket-capable
Files:
internal/watcher/synthesizer/file.gosdk/auth/filestore.goBehavior:
websocketsmetadata when present.Attributes["websockets"] = "true"for providercodex.Why:
2) Normalize Codex websocket request bodies in both execute paths
File:
internal/runtime/executor/codex_websockets_executor.goBehavior:
ExecuteandExecuteStream:modelstream=truein streaming pathinstructionsWhy:
3) Add session-aligned websocket metadata/header parity
Files:
internal/runtime/executor/codex_websockets_executor.gointernal/runtime/executor/codex_executor.goBehavior:
prompt_cache_keyfrom the execution/session id when absent.client_metadata.x-codex-turn-metadata.Session_idx-client-request-idx-codex-turn-metadataConversation_idon Codex websocket requests.Why:
service_tier=priority.Investigation summary
During investigation, the following concrete issues were confirmed:
/fastsendsservice_tier: "priority"over websocket."default".service_tier: "default"can be rejected upstream on the Codex websocket path.Validation
Official Codex baseline
Using traced official Codex websocket requests:
service_tierservice_tier: "priority"Proxy behavior after this patch
The proxy websocket path:
service_tier: "priority"upstreamLatency benchmark
Short websocket prompt:
Reply with exactly: OK and nothing else.Patched proxy, comparing omitted tier vs priority:
omit(null)
priority
This is not a dramatic improvement, but it is measurable and consistently in the expected direction after the websocket parity fixes.
Notes / scope
response.completed.service_tier = default, so that field alone does not appear reliable for verifying Codex fast behavior on this route./fastbehavior.Related