fix(engines): rewrite the Codex adapter against the real app-server protocol (#914)

[frankbria]

Closes #914.

--engine codex could not work at all: the adapter spoke a protocol that does not exist. initialize was sent without the required clientInfo, so the server returned a JSON-RPC error and the '"result" not in response' check killed every run at the handshake. thread/start and turn/start went out id-less with invented params, and the event loop waited on session_started / notification / tool_call — none of which appear in ServerNotification — replying with an invented tool_call/approved.

What the protocol actually is

Verified two ways: against codex app-server generate-json-schema and against a live codex app-server (codex-cli 0.141.0) session.

-> {"id":1,"method":"initialize","params":{"clientInfo":{"name","version"}}}
<- {"id":1,"result":{...}}
-> {"method":"initialized"}
-> {"id":2,"method":"thread/start","params":{"cwd","sandbox","approvalPolicy"}}
<- {"id":2,"result":{"thread":{"id":...}}}
-> {"id":3,"method":"turn/start","params":{"threadId","cwd","input":[{"type":"text","text":…}]}}
<- notifications: item/started, item/completed, thread/tokenUsage/updated, error, …
<- {"id":N,"method":"item/*/requestApproval",…}   (server *requests*)
<- {"method":"turn/completed","params":{"turn":{"status":…}}}

Two details that bite if you assume plain JSON-RPC 2.0: the wire format carries no jsonrpc field (confirmed live — the server accepts messages without it, and JSONRPCRequest requires only id + method), and approvals arrive as requests that must be answered by their own id. There is no turn/failed and no turn/cancelled; the terminal state is turn.status ∈ {completed, failed, interrupted}.

Changes

Handshake and turn flow — real initialize (with clientInfo) → initialized notification → thread/start → turn/start, all id-carrying requests correlated by id. The threadId comes from the thread/start response instead of a locally-invented uuid. JSON-RPC error responses are surfaced verbatim with their code, at every step.

Approvals answered by request id — item/commandExecution/requestApproval and item/fileChange/requestApproval get {"id": <same>, "result": {"decision": "accept"|"decline"}} per the approval policy. Any server request we do not implement gets an explicit -32601 reply, because an unanswered request wedges the turn. The v1 callbacks (applyPatchApproval, execCommandApproval) use a different decision enum and are deliberately not guessed at — we are on the v2 path.

Transport — a single reader thread drains stdout into a queue.Queue, replacing the select()-on-the-fd plus buffered-TextIOWrapper mix. That mix was the stall bug: once a burst was slurped into the Python buffer, select reported "no data" and a run that had already received turn/completed sat until the 5-minute stall timeout. Malformed JSON is now logged and skipped, distinct from EOF, so one stray non-JSON log line no longer aborts a live turn as "Process terminated unexpectedly".

Result mapping — terminal status from turn/completed's turn.status (failed carries turn.error.message), tokens from thread/tokenUsage/updated, and AgentResult.output from the final agentMessage item.

Sandbox default — sandbox_mode now defaults to workspace-write rather than None. The adapter exists to write code into the workspace; inheriting whatever ~/.codex/config.toml happens to say could silently produce a read-only run.

Acceptance criteria

AC	Evidence
Handshake/turn match the schema; approvals by request id; JSON-RPC errors handled	TestCodexHandshake, TestCodexApproval (7 tests)
stdout drained without mixing select() and buffered reads; malformed JSON skipped and distinct from EOF; burst-then-silence asserts no stall	TestCodexTransport — the burst test writes 50 lines + turn/completed into a real os.pipe(), leaves it open, and asserts completion in < 2 s against a 2 s stall timeout
Contract test against a checked-in generate-json-schema fixture	TestCodexSchemaContract validates every outbound message against ClientRequest / ClientNotification / the two approval *Response schemas in tests/core/adapters/fixtures/codex_app_server/
Engine stays registered and advertised	engine_registry untouched; proven working end-to-end below

Demo — real binary, real turn

Driving the actual CodexAdapter against the real codex CLI in a scratch git repo:

[progress] Started userMessage      [progress] Started fileChange
[progress] Started reasoning        [progress] Completed fileChange
[progress] Started agentMessage     [progress] Completed agentMessage
============================================================
status       : completed
error        : None
modified     : ['greeting.py']
tokens       : AdapterTokenUsage(input_tokens=35557, output_tokens=140)
duration_ms  : 15554
------------------------------------------------------------
greeting.py exists: True
def hello():
    return "hi from codex"

The file on disk is the outcome evidence — the adapter completed a turn, the delegated agent wrote real code, and modified_files picked it up.

Review

codex review --base main (cross-family, pre-PR) raised one [P2]: a JSON-RPC error on the turn/start id was treated as a harmless late ack, so a rejected turn would sit until the stall/turn timeout and lose the server's reason. Verified live before fixing — a bad threadId returns {"id":3,"error":{"code":-32600,"message":"invalid thread id: …"}} and no turn/completed ever follows. Fixed in fe93933 with a regression test asserting the failure surfaces in < 2 s against a 30 s stall timeout.

Tests

tests/core/adapters/ — 194 passed (28 for codex, up from 20). ruff check clean.

Known limitations

• cf work start --engine codex --execute still hard-requires OPENAI_API_KEY even though the codex CLI works fine on a ChatGPT login — the live demo above ran with no OPENAI_API_KEY set. That gate lives in cli/app.py → require_openai_api_key(), outside this issue's scope; filed as a follow-up.
• The binary-gated real-CLI contract tier (running this against the installed codex in CI) is [P0.21] Run every shipped engine adapter against its real CLI in a binary-gated smoke/contract tier #915 (P0.21). This PR verifies against the real binary manually and ships the schema fixture that tier will reuse.
• The schema fixture is pinned to codex-cli 0.141.0; refreshing it is a documented one-liner in the fixture README.

[github-actions]

Claude finished @frankbria's task in 6m 4s —— View job

---

GLM Precision Bug Review — Codex app-server adapter (#1009)

Scope: concrete defects only (logic errors, null/unhandled paths, security, data loss/races, contract breaks, resource leaks). Style/architecture/coverage are covered by the general review.

• Gather PR context & full diff
• Read codeframe/core/adapters/codex.py in full + callers/callees
• Check existing comments to avoid duplicates
• Verify outbound messages against checked-in schema fixtures
• Analyze transport, handshake, approval, result-mapping for defects
• Post findings

---

✅ GLM review: no defects found.

No inline findings. What I verified against the checked-in fixtures/codex_app_server/ schema and the surrounding code:

• Outbound contracts — initialize (clientInfo.{name,version}, title optional), initialized (valid ClientNotification), thread/start (cwd/approvalPolicy/sandbox all in ThreadStartParams), and turn/start (threadId+input+cwd in TurnStartParams) all validate. Enum values the code emits are correct: AskForApproval ∈ {never,on-request} ✓, SandboxMode = workspace-write ✓, approval decision ∈ {accept,decline} ✓ (both *RequestApprovalResponse fixtures).
• id correlation — _request and _stream_turn dispatch correctly: id+method → server request answered by its own id; response with no method + error on turn_request_id → surfaced failure (the fe93933 fix); late/duplicate handshake responses (id 1/2) ignored via continue. Server-request ids and client-request ids can't collide because the if method: check separates them.
• turn/start rejection — a JSON-RPC error on the turn/start id returns < 2 s instead of waiting out the stall/turn timeout (test_rejected_turn_start_fails_immediately).
• transport — single reader thread → queue.Queue; stdin is written only from the main thread (no race). _EOF is always enqueued in finally, so a dead process can't wedge recv. Malformed lines are skipped distinct from EOF.
• timeouts — stall/turn/read windows are all > 0-guarded; defaults (read 30 s < stall 300 s) can't starve the stall check. Process is always killed in run()'s finally; no leak path.
• None-safety — every msg["error"]/.get("turn")/.get("item")/tokenUsage access is or {}-guarded; no deref of a possibly-None payload.
• production caller — runtime.py:785 passes adapter_kwargs={} for codex, so defaults (approval_policy="auto" → server never, sandbox="workspace-write") are used; no value-mismatch in the real path.