fix(codex): harden app-server turn continuity - #98995
Conversation
andrexibiza
left a comment
There was a problem hiding this comment.
Reviewed exact head 4a51e51320f50ff4e3005255cc6f9fa3c9243cae against merge base 4f22543509d1b91dc45bcb369447126c5eb14fb7 and current main@d63f996a757f6255fc1454239616ab4b4435e0f5. The branch is 2 commits ahead / 2 behind current main; the two newer main commits are Photon-only, so I did not find a current file collision there. I read the production diff across runtime/session/transport/config/gateway persistence, the focused regressions, the overlapping Codex PRs, and the exact-head workflow state.
There is a lot of careful work here that is worth preserving: strict turn/completed correlation matches the current Codex v2 contract; the poisoned-write retirement model is the right direction for an indeterminate stdio write; resume fallback is narrowed instead of treating every JSON-RPC failure as a missing thread; final text is no longer promoted after a failed/timeout turn; and the same-text steer/initial-echo reconciliation is substantially more thoughtful than content dedup. The test surface is also materially better than current main. 🚀
I do have three P1 boundary issues before I would treat this as a safe consolidated landing object.
P1 — the cwd marker turns untrusted message text into repository-routing authority
_resolve_codex_handoff_cwd() reads the routing marker directly out of user_message, and test_two_marked_repositories_get_distinct_mappings_and_strip_marker proves that arbitrary message text such as prefix [HERMES_RUNTIME_CWD=...] suffix can switch one long-lived Hermes session from repo A to repo B. codex_app_server_workspace_roots bounds where the caller may point, but it does not prove who is allowed to select a repository.
That distinction matters on the gateway path: the same user_message is the inbound conversation payload. With more than one allowed workspace, a remote/user-authored message can therefore select any configured repository and cause the Codex subprocess/thread to run there. require_explicit_cwd=true makes the marker mandatory, but does not make it authoritative; it actually makes message content the only presented routing proof.
The routing coordinate should come from trusted turn/handoff metadata (or another authenticated route object) and be checked against workspace roots there. If the textual marker is retained as a compatibility projection, it should only be consumed when its provenance is a trusted internal handoff source; ordinary inbound text containing the same bytes must remain data. Right now the parser has location validation but no provenance boundary.
P1 — this introduces a second turn-budget owner and preserves the exact 600s failure that #98940 is fixing
The new agent.codex_app_server_turn_timeout is independent of Hermes' existing agent.run_budget_seconds, defaults to 600 seconds, and the integration test explicitly pins captured["turn_timeout"] == 600.0 when no Codex-specific override is supplied.
That means a caller that deliberately configures a 10,800s/14,400s run budget can still be terminalized at 600s unless it discovers and duplicates the value into a second config key. Conversely, a shorter Hermes run budget is not the authority driving this inner app-server loop. #98940 is not merely duplicate timeout work: its concrete defect is exactly that a native Codex turn must consume the remaining monotonic Hermes run budget rather than own an independent hardcoded 600s lifecycle.
Please settle one precedence/ownership contract before merge. If a Codex-specific timeout is retained as a safety cap, compose it explicitly with the remaining Hermes run budget (for example, the earlier applicable deadline wins) and test both run_budget < codex cap and run_budget > 600. If the run budget is intended to be the sole user-requested turn authority, absorb #98940's remaining-budget propagation instead. Either way, preserve #98940's contribution rather than flattening it as duplicate work.
P1 — turn/steer still accepts an uncorrelated success response
request_steer() currently does:
accepted_turn_id = response.get("turnId") if isinstance(response, dict) else None
accepted = accepted_turn_id in {None, turn_id}so {} / a missing turnId is promoted to a confirmed reservation and can return True. That is the exact invariant isolated in #82492: a successful turn/steer acknowledgement is correlated only when it names the exact active turnId. A missing/malformed acknowledgement is an unknown-delivery state, not proof that this generation accepted the steer.
The new reservation machinery makes this more important, not less, because a falsely confirmed same-text steer participates in end-of-turn echo reconciliation. Require exact equality with the active turn ID. If you want to preserve the possibility that Codex accepted the steer before returning a malformed response, classify that case as indeterminate/false rather than confirmed/true. #82492 should be credited as the focused source of this correlation rule.
Interlock / merge-order notes
- #98940 is competing/overlapping on deadline, approval lifetime, terminal classification, and session retirement. It contains a distinct existing-run-budget authority contract that this head does not subsume. Reconcile before either lands; do not merge both overlapping implementations independently.
- #82492 is a focused missing invariant inside this head, not superseded work yet. Its exact-turn steer acknowledgement rule should be absorbed or land first and be preserved through reconciliation.
- #41905 is complementary on continuity scope. This PR persists
(Hermes session, canonical cwd) -> Codex threadand explicitly tests that a reset/new Hermes session starts fresh. #41905 instead preserves Codex continuity across idle/daily Hermes session rotation for durable gateway thread/topic lanes while keeping explicit fresh-start boundaries fresh. Those are different lifetime policies. Decide the gateway-lane contract before calling this the continuity superseder, and preserve #41905's author/issue lineage. - #61751 is architectural adjacency: it moves Codex through the shared turn finalizer instead of continuing to grow a parallel early-return finalizer. This PR adds more persistence/finalization behavior to the early-return path, so merge order should be decided rather than mechanically combining both.
- #83129 is complementary transport-loss typing. This head improves indeterminate timeouts, but still uses plain
RuntimeErrorfor some broken/closed stdin paths; do not treat the two as equivalent without reconciling that typed failure boundary. - #93546 is partially superseded by the richer initial-echo/generation reconciliation here, but its salvage lineage (#38254 / #43127 and the contributors credited there) should survive if this implementation becomes the landing object.
Exact-head acceptance
The author-reported focused receipts are useful, but they are not hosted exact-object acceptance. On 4a51e513..., CI run 33348761383, Docker run 33348761088, and Nix run 33348761007 all concluded action_required, and each currently exposes zero jobs. The PR also explicitly has not run the full repository suite. After the three boundaries above are reconciled, rebase/synchronize to current main and require actual CI/Docker/Nix jobs to execute and go green on the final SHA before landing.
The core direction is strong. The main thing I would avoid is letting this become a new consolidated control plane while three older focused invariants are still weaker at the exact boundaries it is trying to centralize: routing authority, deadline authority, and steer settlement. Fix those, reconcile the continuity/finalizer ownership, and this becomes a much more coherent end-to-end Codex runtime rather than another overlapping slice.
What does this PR do?
This PR hardens Hermes' optional Codex app-server runtime so a turn is continuous, repository-scoped, deadline-bounded, and only considered successful after a valid terminal protocol event.
The default
auto/ provider runtime remains unchanged. These changes apply only whenmodel.openai_runtime: codex_app_serveris selected.Problem
The existing adapter could lose server-side Codex continuity when the Hermes agent was rebuilt, reuse the wrong Codex thread after changing repositories, and treat streamed assistant text as a completed answer even when no matching
turn/completedevent arrived. Several lifecycle steps also had independent or unbounded waits, which could outlive the configured turn timeout. A separate hardcoded 90-second post-tool watchdog could incorrectly retire a healthy long-reasoning turn even when its absolute deadline was much longer. Finally, an expired stdin write could leave a late JSON-RPC frame on a connection that Hermes might otherwise reuse.Behavior after this change
[HERMES_RUNTIME_CWD=/absolute/path]handoff marker can scope a turn to an allowed repository root; deployments may require the marker explicitly.agent.codex_app_server_turn_timeoutdrives one absolute deadline shared by startup, initialize, thread start/resume, turn start, stdin writes, approvals, polling, steering, interruption, and completion.agent.codex_app_server_post_tool_quiet_timeout(default 90 seconds) and now refreshes on turn-scoped reasoning, content, item, command-output, token-usage, and server-request activity instead of false-failing a healthy turn after tool use.turn/startresponse without a non-emptyturn.idfails immediately with a structural diagnostic instead of waiting for the absolute deadline.turn/completedfor the active thread and turn with a successful terminal status.turn/steeris deadline-bounded and reconciled by turn generation, preserving legitimate steers even when their text is identical to the initial prompt.assistant→assistanttranscript rows.thread/resumefalls back to a fresh thread only for explicit missing/stale/incompatible thread or rollout errors.Related work / overlap
This is a consolidated end-to-end hardening pass. I searched the open and closed PR queue before publishing. Existing PRs address individual subsets of this failure family, including #41905, #61751, #82492, #83129, #93546, and #98940.
This PR adds the integrated invariants that are not present together in any one of those changes: per-workspace continuity, the explicit cwd contract, a single absolute deadline across the entire lifecycle, poisoned-client retirement after ambiguous writes, strict scoped completion, generation-aware identical-steer handling, and fail-closed final-response persistence. It should be reconciled with those PRs rather than merged alongside overlapping hunks without review.
Type of Change
Changes Made
agent/codex_runtime.pyagent/transports/codex_app_server.pyagent/transports/codex_app_server_session.pyagent/agent_init.py,run_agent.py,gateway/run.py,hermes_cli/config_defaults.pyagent/codex_runtime.pycli-config.yaml.exampleand user/bundled-skill documentationHow to Test
Run the focused CI-parity suite:
Result on this branch:
Additional validation:
Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qsuite; the focused 193-test suite and the 155-test post-tool follow-up suite passedDocumentation & Housekeeping
cli-config.yaml.examplefor the new config keysCONTRIBUTING.md/AGENTS.mdchanges are N/Afcntl) and Windows (msvcrt) implementations (Linux exercised locally; Windows path covered structurally but not live-tested here)Screenshots / Logs
Not applicable; this is a runtime/protocol lifecycle change. The focused test result is included above.