fix(codex): retain completion evidence for fresh status polls - #1795
fix(codex): retain completion evidence for fresh status polls#1795Asentient wants to merge 2 commits into
Conversation
github-actions
Bot
added
intake:clean
|
👋 Thanks for the contribution — intake looks complete. Your PR body carries everything the maintainer's validation pipeline reads first: the problem, the reasoning, the human intent behind it, and an AI-disclosure. It will be applied, built, and tested against gate marker read: ai= |
📝 WalkthroughWalkthroughThe PR introduces shared persisted hook-state documents with retained turn generations, unified status decoding, corrupt-state recovery, and Codex completion-aware status debouncing across fresh CLI polls and live instance updates. ChangesCodex completion state
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant HookHandler
participant HookStateStore
participant SessionInstance
participant CodexSession
HookHandler->>HookStateStore: persist turn event
HookStateStore-->>SessionInstance: expose retained generations
SessionInstance->>CodexSession: compare completion evidence with bound session
CodexSession-->>SessionInstance: provide current status observation
SessionInstance-->>SessionInstance: settle completion-aware debounce
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/session/instance.go (1)
4951-4974: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCodex subagent-rejection path doesn't restore retained completion evidence, corrupting the new matching fields.
Unlike the Claude branch (which restores
hookStateSessionID/hookGeneration/hookLastTurn*Generationon both its rejection paths at lines 4878-4881 and 4904-4907), the Codex subagent-rejection branch here returns at line 4972 without restoringprevHookStateSessionID/prevHookGeneration/prevHookStartedGeneration/prevHookCompletedGeneration.Since these fields are unconditionally overwritten at the top of the function (lines 4818-4821) from the incoming
statusbefore the subagent check runs, a completing subagent thread'sStateSessionIDand turn generations get written into the instance even though its session-id rebind is correctly rejected. This poisonshasMatchingCodexCompletionLocked()(i.hookStateSessionIDno longer equalsi.CodexSessionID), causing the exact "stuck running/uncertain status" regression issue#1792sets out to fix — but now for any Codex conductor session that uses subagents, since a subagent'sagent-turn-completearriving after the main thread's genuine completion wipes out the valid retained evidence.🐛 Proposed fix: restore retained fields on subagent rejection
if i.shouldRejectCodexSubagentRebind(sessionID) { + i.hookStateSessionID = prevHookStateSessionID + i.hookGeneration = prevHookGeneration + i.hookLastTurnStartedGeneration = prevHookStartedGeneration + i.hookLastTurnCompletedGeneration = prevHookCompletedGeneration _ = WriteSessionIDLifecycleEvent(SessionIDLifecycleEvent{ InstanceID: i.ID, Tool: i.Tool, Action: "reject", Source: hookSource, OldID: i.CodexSessionID, Candidate: sessionID, HookEvent: status.Event, Reason: "candidate_is_subagent_thread", }) sessionLog.Debug("codex_session_rebind_rejected_subagent", slog.String("old_id", i.CodexSessionID), slog.String("candidate", sessionID), slog.String("event", status.Event), ) return }Want me to also add a regression test in
internal/session/codex_completion_evidence_test.gocovering a subagent completion arriving after a matching main-thread completion?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/session/instance.go` around lines 4951 - 4974, Restore the retained Codex hook evidence before returning from the shouldRejectCodexSubagentRebind branch: reinstate the prior hook state session ID, hook generation, started generation, and completed generation values captured before processing status. Keep the rejection lifecycle event and debug logging unchanged, and ensure hasMatchingCodexCompletionLocked continues to see the main thread’s retained completion evidence.
🧹 Nitpick comments (2)
internal/session/hook_state_test.go (1)
50-70: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd test coverage for the
deadevent reset branch.This suite covers the session-ID-mismatch reset (
hook_state.golines 76-81) but not the separateevent.Status == "dead" && event.SessionID == ""reset (hook_state.golines 89-95), which is explicitly called out as a distinct safety-reset case.✅ Suggested test addition
func TestAdvanceHookStateDeadEventClearsRetainedEvidence(t *testing.T) { t.Parallel() state := HookStateDocument{ SchemaVersion: HookStateSchemaV1, Generation: 5, SessionID: "thread-1", StateSessionID: "thread-1", LastTurnStartedGeneration: 4, LastTurnCompletedGeneration: 5, } next, err := AdvanceHookState(state, HookStateEvent{ Kind: HookStatusOnly, Status: "dead", Event: "session.dead", }) if err != nil { t.Fatalf("AdvanceHookState: %v", err) } if next.StateSessionID != "" || next.LastTurnStartedGeneration != 0 || next.LastTurnCompletedGeneration != 0 { t.Fatalf("dead event did not clear retained evidence: %#v", next) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/session/hook_state_test.go` around lines 50 - 70, Add a focused test alongside TestAdvanceHookStateNewSessionCannotReusePriorCompletion for a HookStatusOnly event with Status "dead" and no SessionID. Seed retained session and turn evidence, call AdvanceHookState, and assert the reset clears StateSessionID, LastTurnStartedGeneration, and LastTurnCompletedGeneration while preserving successful error handling.internal/session/hook_state_store.go (1)
39-46: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCorrupt existing state file permanently blocks future writes.
If the on-disk document fails to decode,
writeHookStateAtaborts entirely instead of continuing with a fresh/reset document. This turns a one-time corruption into a permanent stuck state for that instance until manual intervention.♻️ Suggested fallback on decode failure
statePath := filepath.Join(hooksDir, instanceID+".json") previous, err := readHookStateAt(statePath) - if err != nil && !os.IsNotExist(err) { - return fmt.Errorf("read hook state: %w", err) - } + if err != nil && !os.IsNotExist(err) { + // A corrupt document should not permanently block future updates; + // start from a fresh document instead. + previous = HookStateDocument{} + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/session/hook_state_store.go` around lines 39 - 46, The write flow around readHookStateAt should distinguish a decode failure from other read errors and continue with a fresh/reset hook state instead of returning the error. Preserve the existing not-found handling and still propagate unrelated I/O errors; then pass the reset state to AdvanceHookState so writeHookStateAt can recover from corrupt on-disk state.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/session/instance.go`:
- Around line 4951-4974: Restore the retained Codex hook evidence before
returning from the shouldRejectCodexSubagentRebind branch: reinstate the prior
hook state session ID, hook generation, started generation, and completed
generation values captured before processing status. Keep the rejection
lifecycle event and debug logging unchanged, and ensure
hasMatchingCodexCompletionLocked continues to see the main thread’s retained
completion evidence.
---
Nitpick comments:
In `@internal/session/hook_state_store.go`:
- Around line 39-46: The write flow around readHookStateAt should distinguish a
decode failure from other read errors and continue with a fresh/reset hook state
instead of returning the error. Preserve the existing not-found handling and
still propagate unrelated I/O errors; then pass the reset state to
AdvanceHookState so writeHookStateAt can recover from corrupt on-disk state.
In `@internal/session/hook_state_test.go`:
- Around line 50-70: Add a focused test alongside
TestAdvanceHookStateNewSessionCannotReusePriorCompletion for a HookStatusOnly
event with Status "dead" and no SessionID. Seed retained session and turn
evidence, call AdvanceHookState, and assert the reset clears StateSessionID,
LastTurnStartedGeneration, and LastTurnCompletedGeneration while preserving
successful error handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f1180d98-6f64-4898-90a8-bd6992ed0747
📒 Files selected for processing (10)
cmd/agent-deck/codex_hooks_cmd_test.gocmd/agent-deck/hook_handler.gointernal/session/codex_completion_evidence_test.gointernal/session/hook_state.gointernal/session/hook_state_store.gointernal/session/hook_state_store_test.gointernal/session/hook_state_test.gointernal/session/hook_watcher.gointernal/session/instance.gointernal/session/transition_daemon.go
|
Addressed all three CodeRabbit findings in
Validation: focused |
asheshgoplani
left a comment
asheshgoplani
left a comment
There was a problem hiding this comment.
Thanks for the fast turnaround on the CodeRabbit findings — the subagent-rebind restoration, the scoped corruption recovery, and the session-dead clear test all check out on 77b5158.
CI is green on this head (full suite, race tests, govulncheck, CodeQL, CodeRabbit checks all passed). Two independent reviews (Claude + Codex) still found gaps in the core goal of this PR (retaining completion evidence across a fresh CLI/daemon process), so holding before merge:
-
internal/web/session_data_service.go(~285-291, ~351) still decodes the legacyrawHookStatusshape instead ofsession.HookStateDocument. Every other hook-file reader (hook_watcher.go,transition_daemon.go,hook_handler.go) was migrated, but the web/headless reader wasn't, so it builds a zeroedsession.HookStatus{}(emptyStateSessionID, generation 0).refreshStatusesthen callsUpdateHookStatus, andinstance.go(~4818-4822) assigns those fields unconditionally — so each web-surface refresh tick clobbers the retained evidence thatStatusFileWatcherjust wrote, undoing the fix for any process serving the web/headless surface. -
The completion-match gate in
codexCompletionMatchesBoundSession/hasMatchingCodexCompletionLocked(instance.go~4131-4139) has no freshness bound. It'sstateSessionID == boundSessionID && completed > 0 && completed >= started, with no check that the completion is the current turn. If a later turn's start event is ever missed (dropped notify, etc.),startedstays stale and the bypass stays true indefinitely — so a single transient "waiting" pane read mid-turn skips the debounce and reports a false completion.hookGenerationis already tracked (assigned in several places ininstance.go) but never read; gating oncompleted == generation(or boundingLastTurnCompletedAt) would close this without new plumbing. Codex's own review converged on this same line independently. -
Subagent hook writes aren't gated before they hit disk (
internal/session/hook_state.go~76-83). The in-memory guard atinstance.go~4959-4966 stops a subagent event from replacing the bound instance's state in memory, but the on-disk writer path (codex_hooks_cmd.go→hook_handler.go→WriteHookState) has no equivalent gate, so a subagent notify under the inheritedAGENTDECK_INSTANCE_IDcan still overwrite the main thread's persisted generations via the session-mismatch reset inhook_state.go. That's the exact cold-process scenario this PR targets, and the new test only covers the in-memory path, not the on-disk one.
Smaller / non-blocking, worth a look before or shortly after merge:
instance.go~4594-4599: the new early-return onmatchingCompletion && Status == Waitingalso skips the Codex session-ID resync and Claude prompt refresh in the metadata-sync block, not just the DetectTool probes the comment describes.hook_state_store.go: the per-instance.lockfile and the new randomized.tmpnames aren't covered by the existing stale-file reaper (hook_handler.gocleanStaleHookFiles), so crashed writers now leak files instead of reusing one slot.- Two fsyncs were added to the synchronous hook-write path (previously write+rename only) — likely fine given the durability goal, just flagging for a conscious sign-off given prior latency sensitivity there (#1225).
Happy to re-review once (1) and (2) are addressed — those two are what would keep #1792 from actually being fixed on cold processes and the web surface. (3) matters for the same reason but is scoped tighter (subagent-under-inherited-env case).
2 participants
What problem does this solve?
Fresh
agent-deck list --jsonprocesses can report a completed Codex session asrunningforever because every process forgets the first half of therunning-to-waiting debounce. Fixes #1792.
Why this change
Codex completion hooks are explicit evidence, but the current status file keeps
only the latest event and orders events with Unix-second timestamps. This
extends that file compatibly with a monotonic generation plus retained
last-started/last-completed generations bound to one Codex thread.
Hook writers now serialize read-modify-write with a per-instance
flockandrandomized same-directory atomic replacement. A stale matching completion may
bypass only the first pane debounce sample when the pane also reports waiting.
Pane-only evidence, a different thread, or a newer retained start keeps the
existing debounce behavior. This is the status-only first slice of the design
discussed in #1794; it does not change prompt delivery, queues, or hook
installation.
User impact
A Codex turn with matching completion evidence settles from persisted
runningtowaitingon the first fresh status poll. An idle-looking panewithout that evidence still cannot manufacture a completion.
Legacy hook JSON remains readable, and older Agent Deck versions ignore the
new additive fields.
Evidence
Clean-main real-Codex reproduction and environment details are recorded in
#1792. For the initial tested branch head, I built commit
7885cfe7as Agent Deck v1.10.11:One isolated real-tmux fixture used a scratch HOME/XDG/profile/socket, a fake
Codex prompt, a persisted
runningrow, and a matching completion aged 300s.The same fixture and same three fresh-process polls produced:
Hostile inverse: after retaining a newer start (
started_generation=9,completed_generation=8) while the fake pane still looked idle, three fresh PRpolls remained
running.Hot-path timing over 20 fresh polls on the matching-completion fixture:
Tests:
Automated-review follow-up
77b51588restores retained main-thread evidence after a rejected subagent hook, recovers safely from corrupt JSON state while preserving I/O/symlink errors, and covers the existing dead-session reset. Its subagent and corruption tests each failed before their production fix. The updated session package passed in 144.288s (excluding the clean-main Cursor failure); the focused cases passed under-race -count=10in 1.833s;go vet ./...andgo build ./...remained green.A serial sandboxed repository run passed every package except three tests that
fail identically on clean
origin/main:TestCanRestartCursor(tmux/Cursor)and the web keepalive tests
TestWSKeepaliveReapsDeadPeerandTestWSKeepaliveHealthyClientSurvives. The web package passes with those twoclean-main failures excluded. The statedb performance gate, which exceeded its
wall-clock threshold on the host's slow
/varfilesystem, passes on tmpfs(insert 6.60 ms under its 630 ms budget; delete 1.96 ms under its 510 ms
budget).
Revert-check: copying the changed tests onto
origin/mainfails to compile onthe missing hook-state and completion-evidence symbols, which is the expected
failure without the production change.
AI disclosure
Model(s), if AI helped: gpt-5-codex
Prompt / session log (optional): —
What actually bothered you
My human asked: “Implement the approved plan test-first, beginning with the
contracts and schema lane specified by the plan.” The plan was triggered by
Codex sessions that had visibly completed but stayed
runningacross everyfresh Agent Deck poll.
Checklist
HOME=$(mktemp -d) XDG_CONFIG_HOME= XDG_DATA_HOME= XDG_CACHE_HOME= go test ./...(three failures reproduce on cleanorigin/main; details above)Summary by CodeRabbit
Bug Fixes
Reliability & Security