fix(codex): propagate failed app-server turns

[AaronZ345]

Summary

• inspect turn/completed status and error data from Codex app-server
• emit EventError instead of a successful empty EventResult when the turn failed
• clear failed-turn state so the later idle notification cannot emit a duplicate completion

Why

Codex app-server can report a failed turn through turn/completed with turn.status = "failed" and an error object. The adapter currently ignores both fields and always completes successfully. Scheduled jobs then render (empty response) and are recorded as completed, hiding the upstream error.

Tests

• go test ./agent/codex
• git diff --check

[chenhg5]

Conclusion: Approve

Overall assessment:
Codex app-server's turn/completed notification carries both a status field ("completed" / "failed") and an optional error object. The previous adapter ignored both and always emitted a successful EventResult — so failed turns surfaced to users as (empty response) and to scheduled-job logs as "completed". This PR inspects both fields and routes a failed turn to EventError via a new failTurn helper that mirrors the existing completeTurn lifecycle. The fix is small (24 production lines), focused, and well-tested. Local go test -race ./agent/codex/... is green.

Review scope:

• Read agent/codex/appserver_session.go (handleNotification for turn/completed, the new failTurn, and the existing completeTurn for comparison), and the new TestAppServerSession_FailedTurnEmitsError test.
• Verified that emitError already emits core.Event{Type: core.EventError, Error: err} and that downstream consumers handle EventError.

✅ What looks good:

• The dual-trigger check (status == "failed" OR turn.error != nil) catches both ways Codex can report a failed turn: explicit failure status and a status of "completed" with an attached error object. Either path now produces EventError instead of a deceptive EventResult.
• failTurn reuses the same stateMu guard as completeTurn: the currentTurn == "" early-return makes it idempotent and race-safe against a delayed thread/status/changed idle notification. The new test asserts exactly this — the second notification must NOT emit a duplicate event.
• Error message fallback to "turn failed (no details)" keeps the surface stable even if Codex sends status="failed" without an error payload.
• Minimal blast radius: only turn/completed is touched; everything else (item lifecycle, rate limits, usage updates) is unchanged.

🟠 Should improve:

• Ordering race (worth a follow-up issue, not blocking): if thread/status/changed (idle) arrives BEFORE turn/completed (failed), the existing completeTurn will emit a successful EventResult and clear currentTurn; the subsequent failTurn then no-ops and the failure is silently lost. The Codex app-server contract says turn/completed precedes idle, but degraded network conditions can reorder them. A defensive mitigation would be to remember the last terminal status for the current turn (e.g. lastTurnStatus) and overwrite an already-emitted EventResult if a later notification disagrees — or at minimum, log a warning when failTurn finds currentTurn == "". Either approach is small and contained.
• Test coverage gaps: the new test only covers status="failed" with an error message. Worth adding cases for (a) status="failed" with error == nil (verifies the "turn failed (no details)" fallback), (b) status="completed" with error != nil (verifies the || branch), and (c) a cancelled-style status to document current behavior (currently treated as success — may be intentional, but should be a conscious choice).
• Multi-line error messages: strings.TrimSpace(notif.Turn.Error.Message) keeps only the first line if Codex sends stack-trace-style messages. If the message can contain newlines (likely for internal_error), consider passing through the full message or splitting on first newline for a "summary + detail" pattern.

🔵 Optional:

• The condition strings.EqualFold(strings.TrimSpace(notif.Turn.Status), "failed") is a tiny bit defensive — if the SDK already lowercases or trims, both calls are free, but it's worth a comment noting why we re-do it. Alternatively, capture status := strings.ToLower(strings.TrimSpace(notif.Turn.Status)) once at the top of the case.
• A slog.Warn inside failTurn (with turn_id and error) would help post-mortem correlation in scheduled-job failure logs.

❓ Questions:

• Are there any downstream consumers of EventResult that should ALSO subscribe to EventError to handle the new failure path correctly? In particular, scheduled-job wrappers that compute "did this cron run succeed?" from the event stream. A quick grep -rn "EventResult\|EventError" --include="*.go" on the cron path would close the loop.

Testing / Risk:

• Verified: go test -race ./agent/codex/... is green. TestAppServerSession_FailedTurnEmitsError passes, including the post-failure idle-doesn't-emit-duplicate assertion.
• Unverified risk: the ordering race described above (idle before failed). Acceptable for a first fix; capture in a follow-up issue.

Next step:

• Land as-is. File a follow-up issue for the ordering-race mitigation and the additional test cases.