Skip to content

fix: surface Codex headless failures instead of awaiting input - #1073

Draft
rpelevin wants to merge 1 commit into
construct-worlds:mainfrom
rpelevin:fix/codex-headless-failure-signal
Draft

fix: surface Codex headless failures instead of awaiting input#1073
rpelevin wants to merge 1 commit into
construct-worlds:mainfrom
rpelevin:fix/codex-headless-failure-signal

Conversation

@rpelevin

@rpelevin rpelevin commented Aug 1, 2026

Copy link
Copy Markdown

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:

  • preserves the Codex child exit status through the shared turn driver;
  • detects the reported read-only/sandbox write-block message;
  • emits an actionable Error and Errored session status instead of silently awaiting input;
  • keeps the change scoped to failure signaling; approval-mode flag translation remains a separate follow-up.

Validation

  • cargo +1.88.0 test -p construct-adapter-codex -p construct-adapter-common
    • 32 Codex tests passed
    • 13 shared-adapter tests passed
  • cargo +1.88.0 build -p construct-adapter-codex -p construct-adapter-common passed
  • Full workspace check/build reached the affected adapters but is currently blocked by the pinned wstunnel dependency failing on an unrelated missing cfg_select! macro.

@edwin-zvs edwin-zvs left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 #311 will auto-close an issue this only partly addresses — SetApprovalMode is still ignored (crates/adapter-codex/src/lib.rs:1175), which is the part that makes writes actually work. Refs #311 would be more accurate.
  • The TurnOutcome signature change isn't required: tokio caches the exit status (FusedChild::Done), so a second child.wait().await after drive_turn returns it again — adapter-hermes already relies on this. I confirmed with a scratch test (drive_turn to completion, then wait()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) duplicates construct_adapter_common::short.
  • First-blocked-line-wins sits right below the session_id logic 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_stdout is generic over AsyncRead, 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

codex: headless sessions run read-only and cannot write files (SetApprovalMode ignored in exec path)

2 participants