fix: surface Codex headless failures instead of awaiting input - #1073
fix: surface Codex headless failures instead of awaiting input#1073rpelevin wants to merge 1 commit into
Conversation
edwin-zvs
left a comment
There was a problem hiding this comment.
Thanks for this — and welcome! The problem is real, the diagnosis is right, and the plumbing (threading the child status through TurnOutcome, updating all six call sites, unit tests on the pure helpers) is clean.
I verified the branch end-to-end rather than just reading it: built 7de47c3 in a worktree and drove it against isolated daemons with a fake codex reproducing the real codex exec stdout shape (copied from an actual codex 0.146.0 run), with main @ 4c1459b as the baseline. Three findings below, then some smaller notes.
1. The detector fires on the user's own prompt
Real codex exec echoes the prompt verbatim on stdout:
--------
user
the sandbox is read-only, so write index.html anyway
blocked_write_reason runs on every raw stdout line, so that echo is scanned. A session whose prompt was "Please create index.html. Note the sandbox is read-only, so the write may fail." — pretty much what someone hitting #311 would type — produced this on a fully successful turn (exit 0, file written):
{"type":"error","message":"Codex headless turn was blocked by its sandbox or approval policy: Please create index.html. Note the sandbox is read-only, so the write may fail.. Configure Codex's own headless permissions and retry; …"}
{"type":"status","state":"errored", …}
{"type":"done","exit_code":1}Benign agent prose does the same: "I checked whether the sandbox is read-only before writing index.html; it is writable, so the file was written." → session errored and terminated. main handles both correctly (awaiting input, session alive).
Suggestion: run detection on the parsed assistant text rather than the raw line (that also stops the 600-char excerpt from being raw JSON), and tighten the predicate — an anchored Blocked: beats a three-way keyword AND. The existing negative test only covers lines missing a keyword; the case that bites is a line that has all three innocuously.
2. Ending the session removes the user's ability to retry
After a triggered error, construct send <id> returns daemon error: session has no live adapter. That also applies to a plain nonzero exit with no sandbox involvement — a rate limit or dropped stream, which I simulated as exit 1 — where main returns to awaiting input and the user just resends. Spec 0009-transient-provider-errors-are-retryable argues against making those fatal, and adapter-hermes (same one-shot shape) emits Error and keeps looping.
Note SessionEvent::Error already drives the session to Errored in the daemon (crates/daemon/src/session/events.rs:496), so the explicit Status{Errored} emit is redundant.
3. The fix currently works ~40% of the time
Running the PR's intended case (Blocked: the workspace is read-only, so index.html could not be written, exit 0) ten times across two batches:
| Outcome | Runs |
|---|---|
errored + error event (fix works) |
4/10 |
done ✓, zero error events, transcript ends at the blocked line |
6/10 |
So most of the time the failure is still swallowed — now as a green done ✓, which reads as success.
The cause is a pre-existing daemon race that this PR's "emit, then exit the process immediately" shape walks straight into: the adapter's reader task (crates/daemon/src/adapter.rs:123) and the child-wait task that sends Closed (:200) are separate senders on one channel, and drain_adapter's Closed arm breaks — discarding events still queued behind it — then derives terminal state from the adapter process's exit code, which is Some(0) even on this error path.
Proof: adding a 300 ms sleep after the final Done emit makes it 6/6 correct; removing it brings the flakiness back. Filed separately as #1082.
Happily, fixing (2) mostly dissolves (3): if the adapter emits Error and keeps looping instead of exiting, there's no exit to race.
Smaller notes
Fixes #311will auto-close an issue this only partly addresses —SetApprovalModeis still ignored (crates/adapter-codex/src/lib.rs:1175), which is the part that makes writes actually work.Refs #311would be more accurate.- The
TurnOutcomesignature change isn't required: tokio caches the exit status (FusedChild::Done), so a secondchild.wait().awaitafterdrive_turnreturns it again —adapter-hermesalready relies on this. I confirmed with a scratch test (drive_turnto completion, thenwait()→Some(7)). Not a defect, just FYI that the five-adapter churn was optional; the explicit payload is arguably clearer, so keep it if you prefer. .max(1)silently rewrites exit 0 → 1; worth a one-line comment.trimmed.chars().take(600)duplicatesconstruct_adapter_common::short.- First-blocked-line-wins sits right below the
session_idlogic that deliberately keeps the last value; a comment would keep the asymmetry from reading as accidental. - The error strings explain construct's internals to the user ("the session is errored instead of awaiting input") — I'd cut that clause.
spawn_stdoutis generic overAsyncRead, so a test feeding it a fake reader would cover the diagnostics plumbing end of this cheaply.
For reproducing: point CONSTRUCT_CODEX_CMD at a script that prints the codex header block, echoes the prompt, then prints your chosen outcome line and exits — with CONSTRUCT_CODEX_MODE=headless and an isolated CONSTRUCT_*_DIR, that reproduces all of the above deterministically without touching the real CLI. Glad to share the exact scripts if useful.
Again, nice first contribution — the detection idea is sound, it's the input it's fed and what it does afterward that need adjusting.
Summary
Fixes #311.
Codex headless sessions could report a read-only or sandbox write block, then exit without preserving the child status. Construct treated that completed turn as ordinary and returned to
awaiting_input, hiding the failure.This PR:
ErrorandErroredsession status instead of silently awaiting input;Validation
cargo +1.88.0 test -p construct-adapter-codex -p construct-adapter-commoncargo +1.88.0 build -p construct-adapter-codex -p construct-adapter-commonpassedwstunneldependency failing on an unrelated missingcfg_select!macro.