fix(agent): resume codex app-server thread across turns to preserve session context - #41905
fix(agent): resume codex app-server thread across turns to preserve session context#41905gmkseta wants to merge 2 commits into
Conversation
Live verification against real Codex 0.133Ran the actual
The resumed session reattaches to the exact thread id from turn 1 and the model recalls context the fresh |
325aa3d to
6359c6e
Compare
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
Analysis
Correctness ✅
- The fix solves a real problem: the gateway builds a fresh AIAgent per message, but codex app-server needs the same thread across turns to preserve working context (file reads, tool output, long-running task state).
- The implementation is layered and correct: gateway persists
codex_thread_id→ runtime passesresume_thread_idto session → session callsthread/resumeif set → falls back tothread/starton any failure. - The
_session_codex_threadshasattrguard handles forward compatibility correctly.
Testing ✅
- Excellent test coverage:
thread/resumesuccess, resume failure (method unsupported, timeout, RuntimeError), no resume id, session id alias cross-fill, integration test for thread id round-tripping.
Security ✅
- No security concerns.
Documentation ✅
- Added cross-turn continuity docs to the transport file.
- Updated user guide docs.
Recommendation
Approve — well-engineered fix with thorough tests.
42d8235 to
e8b595e
Compare
|
@teknium1 when you have a chance, could this get another quick look and be merged if it still looks good? I rebased it onto current Relevant validation on the rebased branch is passing: |
e8b595e to
b2c19da
Compare
…ession context
The codex_app_server runtime is documented to keep "one Codex thread per
Hermes session", but the gateway builds a fresh AIAgent per inbound message,
so agent._codex_session was None at the start of every turn and
ensure_started() issued a brand-new thread/start. A Codex thread's working
context (files read, command output, plan state, long-running task state) lives
inside the thread, not in Hermes' transcript (turn/start only sends the latest
user message), so every follow-up message handed the model an empty thread and
silently lost all prior work. Observed live: three distinct codex thread ids
across three Discord messages in one session.
Persist the codex thread id per session and resume it:
- transport: CodexAppServerSession gains resume_thread_id; ensure_started()
issues thread/resume {threadId} (reloads the on-disk rollout) instead of
thread/start, falling back to thread/start on any failure (CodexAppServerError
/ TimeoutError / RuntimeError) so a turn is never lost. Thread-id extraction
is shared by start + resume via _extract_thread_id().
- runtime: run_codex_app_server_turn passes the stored id into the session and
records turn.thread_id after each turn (covers should_retire respawns within
one process); a guard rebuilds a live session only if it is on a different
thread than requested (never in normal operation, so codex is not respawned
on every cache hit).
- gateway: a per-session _session_codex_threads map (mirrors
_session_model_overrides) keyed by the stable session_key so it survives
compaction's session_id rotation; restored onto the fresh per-message agent,
persisted from the turn result (skipped when the run generation is stale so a
discarded turn can't repopulate the map after a reset), and cleared on /new,
auto-reset, compression-exhaustion reset, and session expiry.
Tests: TestThreadResume (resume vs start, graceful fallback incl. RuntimeError,
id flows into turn/start + result), TestCodexThreadResumePersistence (id
round-trips, cache-hit reuse vs mismatch rebuild). Docs: new "Cross-turn thread
persistence" section.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
b2c19da to
7bb6a5c
Compare
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the well-scoped continuity fix. The underlying defect is still present on current main: CodexAppServerSession.ensure_started() unconditionally sends thread/start (agent/transports/codex_app_server_session.py:240-271), and the runtime constructs it with no resume target (agent/codex_runtime.py:392-400).
Problems
- The PR's
gateway/run.py:14220andgateway/run.py:14696use rawself.session_storeinside async gateway code. Current main enforces the awaitedself.async_session_storeboundary to keep blocking SQLite/filesystem work off the event loop (tests/gateway/test_async_session_store.py:56-107). - The persistence layer has moved since this branch:
state.db'sgateway_routingtable is primary andsessions.jsonis only an optional mirror (gateway/session.py:1043-1083, commit94205a113). The runbook should not depend onsessions.jsonbeing present.
Suggested changes
- Salvage the resume adapter/runtime logic, but port gateway persistence to
await self.async_session_storeand cover restart recovery withgateway.write_sessions_json: false.
Automated hermes-sweeper review.
| ) | ||
| if not _codex_resume_thread_id: | ||
| try: | ||
| _codex_resume_thread_id = self.session_store.get_codex_thread_id( |
There was a problem hiding this comment.
Current main requires async gateway code to use the awaited async_session_store facade; raw self.session_store calls are rejected by tests/gateway/test_async_session_store.py. Port this read to await self.async_session_store.get_codex_thread_id(session_key) during salvage.
| if _codex_tid and hasattr(self, "_session_codex_threads"): | ||
| self._session_codex_threads[session_key] = _codex_tid | ||
| try: | ||
| self.session_store.set_codex_thread_id(session_key, _codex_tid) |
There was a problem hiding this comment.
This synchronous routing-index write runs in an async gateway function. Use await self.async_session_store.set_codex_thread_id(...) on current main so the SQLite/filesystem write stays off the event loop.
|
Confirmed the unresolved restart/cache-eviction path on current main (42bd436), including a gateway-hygiene trigger that evicts the live app-server session after native compaction is skipped/no-op. Full sanitized reproduction and source analysis: #41904 (comment) |
|
I independently reproduced and validated the deployment case this PR addresses on Ubuntu 24.04 with Codex CLI 0.146, Discord, and a systemd-managed gateway. Persisting the Codex thread ID in session metadata, closing the original app-server process, then resuming that thread from a fresh process preserved an exact earlier-context recall. Without resume, a gateway restart or agent-cache eviction created a blank Codex thread and lost that context. The maintainer request to use the async |
|
I reviewed the restart/cache-eviction path against current Current
That means this PR does not need a second lease, binding state machine, or I suggest shrinking the refresh to:
@gmkseta, @teknium1: would you prefer that the existing PR branch be refreshed with this scope, or a small successor PR that preserves #41905's authorship and history? I can prepare the focused patch and tests after you choose the route. |
What does this PR do?
The
codex_app_serverruntime is documented to keep "one Codex thread per Hermes session" (agent/transports/codex_app_server_session.py), but the gateway builds a freshAIAgentper inbound message. Before this fix, each follow-up could call a newthread/start, handing Codex an empty working thread even though Hermes still had the message transcript.A Codex thread's working context — files read, command output, plan state, and long-task runtime state — lives inside the Codex thread, not in Hermes' transcript replay. Losing the thread id makes Discord/Telegram follow-ups look like the agent forgot the task.
This PR records the Codex thread id per Hermes session and resumes it with
thread/resume. The mapping is persisted in~/.hermes/sessions/sessions.json, so a gateway restart does not lose the resume target. For durable thread/topic lanes, normal idle/daily Hermes transcript resets also keep the Codex working thread; explicit fresh-start boundaries still drop it.Related Issue
Fixes #41904
Type of Change
Changes Made
agent/transports/codex_app_server_session.py—CodexAppServerSessionacceptsresume_thread_id.ensure_started()issuesthread/resume {threadId}when set, falling back tothread/starton resume failure so the user turn is not dropped.agent/codex_runtime.py—run_codex_app_server_turn()passes the requested resume thread into the session and records the returnedcodex_thread_idon the agent after each turn.gateway/session.py— persistsSessionEntry.codex_thread_idtosessions.jsonwith helpers for reading/writing/clearing it. Idle/daily reset carries the id only for durable thread/topic lanes; DM/non-thread sessions and suspended stuck sessions still start fresh.gateway/run.py— restores the persisted thread id onto fresh per-message agents, persists successful turn ids, skips stale-generation writes after/newor/stop, keeps thread/topic ids through expiry finalization, and updates auto-reset system/user notices so transcript reset does not contradict Codex resume behavior.How to Test
Validated on the rebased PR branch:
Result:
226 passed.New coverage includes:
TestThreadResumeforthread/resumeversusthread/start, resume fallback, and thread id extraction.TestCodexThreadResumePersistencefor runtime round-tripping of the resume id.TestCodexThreadPersistenceforSessionEntryJSON roundtrip, oldsessions.jsoncompatibility,SessionStoresave/reload/clear behavior, thread idle/daily reset carry-over, DM reset fresh-start behavior, and suspended reset fresh-start behavior.Operational Notes
For a gateway lane such as a Discord thread, a successful Codex app-server turn writes
codex_thread_idunder that lane'ssession_keyin~/.hermes/sessions/sessions.json. After a gateway restart, the next turn in the same platform thread loads that persisted id and asks Codex tothread/resume.Idle/daily Hermes transcript resets rotate the Hermes session id but keep the Codex thread id for durable thread/topic lanes, so same-thread follow-ups can continue Codex working context without replaying Discord history into the prompt.
/new,/reset, suspended stuck-session recovery, and explicit lane re-binding remain fresh-start boundaries.Platforms tested
Checklist
origin/main