Fix Codex app-server ownership and protocol admission - #386
Conversation
myobie
changed the title
a9f1a13 to
a1a07d0
Compare
b283c20 to
0c3a0dd
Compare
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: 0c3a0dd950
ℹ️ 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".
| require_methods( | ||
| &schemas.client_requests, | ||
| REQUIRED_CODEX_CLIENT_REQUESTS, | ||
| "client request", | ||
| )?; |
There was a problem hiding this comment.
The admission gate checks only that each required method literal exists, while the later shape checks inspect conventionally named definitions in isolation. If a later Codex release retains turn/start (or another required method) and the legacy definitions but rewires that method arm to different params, this gate admits it even though st2 continues sending the old payload and native delivery then fails at runtime. Resolve each required method arm and verify that its params schema is the expected definition rather than treating method presence and unused definitions as independent evidence.
Useful? React with 👍 / 👎.
2 participants
Summary
Runtime safety
A listed additive element is reviewed and safe to ignore. An unlisted item kind or server request creates an unknownProtocol hold. An idle thread status releases the hold.
The app-server runs in a dedicated process group. A watchdog detects wrapper death and kills that exact group. Rust cleanup removes the socket after normal exits and error returns.
A crash can leave one dead socket file. The next launch proves that the socket has no listener, removes it, and binds the same path.
Evidence
The old version gate rejected codex-cli 0.150.0 before it inspected the protocol. The new gate accepts the real generated schemas from Codex 0.145.0, 0.146.0, 0.150.0, and 0.151.0.
The red-first unknown-item test failed because the runtime stayed active. It passes after the fail-closed hold change.
The wrapper-death test proves four facts: the group dies, the dead socket remains, connection fails with ConnectionRefused, and the next launch recovers the path. The test passes in debug and release profiles.
The current combined checks pass:
No eval ran because the fleet eval stop remains active.
Scope
The installed Codex binary pin is not part of this work. The fleet policy uses deliberate updates with protocol checks.
Claude needs no matching version gate. Its MCP connection negotiates the protocol version.
Accepted tradeoff
This change replaces the exact Codex version and fingerprint allowlist with structural compatibility checks. A genuinely incompatible release can progress until a required shape check or runtime hold catches it. We accept this cost because exact allowlists stranded compatible installed releases.