fix(codex): deterministic attention bell — thread-scoped, status-guarded turn completion - #597
Merged
Conversation
…idation findings Load-bearing assumption validation (12 verified, 2 falsified, both fixed): - A3 falsified: Node fresh-create binds only via rollout proof, so the entire first fresh turn runs pre-bind with no tracker state -- decision #2's rationale rewritten to status-quo parity (Rust half confirmed: the identity gate holds turn/start until adoption binds). - A9 falsified: Rust rebind kept stale current_proxy_turn_id / last_proxy_started_at -- added design decision #7, Task 2 Step 4d (clear both on rebind) and the rebind_clears_stale_in_flight_proxy_ turn_state test. - Task 7 Step 3: dropped t2-codex-equivalence-rust.test.ts (silently deselected under vitest.server.config.ts; env-gated live-codex skip); documented why no coverage is lost. - Global constraints: worktree npm ci prerequisite (freshell-ws integration binaries need worktree-local tsx); baseline verified green. - Added validated residual-risk notes (hard-kill missing turn_aborted, id casing, detached review canary, status vocabulary).
…plan
Fresh-eyes review iteration 1 found two executable-plan defects in
verification gates:
- Task 3 Step 3: the mutation red-check disabled only the thread-scope
guard, but the turn-id dedupe guard structurally masks the red (both
tests drive the foreign completion with a different turn id than the
in-flight one). The mutation now disables BOTH guards and explains why.
- Task 5: the plan missed the five pre-existing bare
{ terminalId, at } call sites in the tracker test's app-server-lane
suite ('turn.complete emission (server-authoritative)'), which the new
thread guard would silently drop (3 test failures) and Task 4's
required threadId would fail typecheck. Step 2 now updates those five
call sites with threadId: 'session-1'; Step 3 and Step 5 expectations
corrected accordingly.
…hread-scope plan - Task 3 Step 3: split the mutation red-check into two cargo test invocations (cargo accepts only one positional test-name filter; the two-positional form errored before running) - Task 5/7: cover the six remaining bare onTurnStarted/onTurnCompleted call sites in the pinned turn-completion-snapshots.test.ts (:65-68 term-1/session-1, :71-72 term-2/session-2) with a deliberate, documented protocol-decision fixture update adding the matching threadId; pinned assertions stay byte-identical and must never be re-pinned. Added the file to Task 5's file list, RED run, commit step, and Task 7 Step 3's targeted suite list.
…heyes iteration 3) - Add a bounded no-ring check between the foreign-thread and bound-thread completions so a regressed thread guard fails the test instead of being masked by the exactly-one tail. - Add a bound-thread nested inProgress completion (with its own no-ring check) that pins the router's freshell_codex::turn_status extraction of the NESTED params.turn.status: a router forwarding None records and rings prematurely. - Step 3: name each test's expected failing assertion under the dual-guard mutation, and add a second directed mutation (turn_status -> None in the TurnCompleted arm) with an expected RED at the inProgress no-ring check.
…ex proxy turn lane
Incorporates the validated assumption audit (9 verified / 8 falsified / 1 accepted): Task 0 baseline repair (inherited auto_resume_e2e regression), corrected death-bell engagement ontology (input-only pending excluded, pending approvals included), shutdownGracefully requested-close discrimination, legacy conversationId approval scoping, full approval resolution signal set (error responses, serverRequest/resolved, restart clears), reconcile-lane guards during approval pauses, corrected decision-4 resume rationale, abort-reason plumbing rationale, and read-before-teardown ordering for the Rust exit bell.
Fresh-eyes review found Task 7 Step 3.4 pinned the pending_approvals / resume_busy_after_approval clear to the Pending and Busy|Unknown arms of note_proxy_turn_completed, but a turn completing during an approval pause routes through the Idle arm (the approval demoted the phase), so the step's own turn_completion_clears_pending_approvals test could never pass: the stale approval's late resolve would flip the pane Busy and emit a Changed effect. The clear now lives once on the accepted completion path BEFORE the phase match, covering every accepted arm.
…t activity events
…nses in the proxy
…covery exit payloads
…s; gate never does)
…-id cleanup (Rust)
Two defects in note_proxy_turn_completed, both reachable when a turn ends
while an approval pause holds the phase at Idle:
1. BEL-echo double-ring: the empty `CodexPhase::Idle => {}` arm left
accepted_start_at / pending_submit_at dangling and armed no swallow
flags. Sequence: turn busy -> approval request demotes Busy->Idle and
the bell rings (deliberate) -> the turn's own task_started folds
mid-pause (audit A9 branch plants accepted_start_at) -> turn/completed
lands in the Idle arm (silent, correct) -> the codex TUI's
turn-complete BEL arrives on the PTY -> consume_turn_complete_signal
keys on the surviving anchor -> transition_after_turn_clear(record)
mints a spurious TurnComplete -> a SECOND terminal.idle for one
attention episode. The Idle arm now performs a silent claim: retire
both anchors, claim the turn key with the Busy arm's derivation
(last_proxy_started_at.or(pending_submit_at)), and arm
swallow_next_bel + swallow_next_reconcile_clear. No completion, no
boundary. reconcile_rollout is unaffected: its clear guard requires
Busy|Unknown and unwraps the anchor to false when absent.
2. Turn-id parity gap: current_proxy_turn_id was cleared per-arm
(Pending, Busy|Unknown) but not in the Idle arm nor on swallowed
echoes, while Node clears its currentTurnId unconditionally right
after the stale-id guard. A surviving id could wrongly drop a later
real completion via the stale-id guard if its turn/started was missed
(proxy reconnect / fork windows). The clear is now hoisted to
immediately after the stale-id guard passes, covering every arm and
the swallow path; the per-arm clears are removed.
Hardening: note_approval_requested pushed the AttentionBoundary even when
the request_id was already in pending_approvals, so a duplicate request
frame (proxy retry / reconnect replay) re-armed the gate and re-rang the
same pause. The boundary now emits only for a newly inserted id.
Tests: tracker-level pins for the silent claim + cleared anchors, the
swallowed/Idle-arm id retirement, and the duplicate-request guard; plus a
hub-level episode test proving approval pause + mid-pause turn end + BEL
echo yield exactly ONE terminal.idle frame. All confirmed RED before the
fix (BEL echo minted a completion; id survived as Some("turn-1"); the
duplicate re-armed; the hub episode rang twice).
Generated with Amplifier
Co-Authored-By: Amplifier <[email protected]>
…pauses onTurnCompleted dispatched on anchors, not phase: after an approval request demoted busy->idle, acceptedStartAt survived (planted by onTurnStarted's promoteBusy or a mid-pause reconcile fold). A turn/completed arriving while the approval was still pending -- e.g. status 'failed', or the serverRequest/resolved vs turn/completed wire race -- fell into the `acceptedStartAt !== undefined` branch and ran transitionAfterTurnClear(record=true), minting a second 'turn.complete' and thus a second terminal.idle for one attention episode (the approval bell had already rung at grace expiry). The Rust mirror (note_proxy_turn_completed) dispatches on phase and stays silent for the identical order; the Node reconcile lane already phase-guarded -- only the proxy lane forgot. The dispatch now mirrors Rust: the accepted-anchor branch requires phase busy|unknown, and a thread-scoped, turn-id-matched, non-inProgress completion landing at phase idle (the pause case, or a stale echo of an already-closed turn) performs a SILENT CLAIM -- clear acceptedStartAt, pendingSubmitAt, and the latent anchor, claim lastEmittedTurnKey with the same key the busy/pending paths use (acceptedStartAt ?? pendingSubmitAt), keep currentTurnId cleared. No completion recorded, no event emitted, so a later PTY BEL echo keyed on those anchors cannot re-mint the turn via consumeTurnCompleteSignal. The latent branch stays reachable for its non-idle (pending-phase) cases; at idle the silent claim subsumes it -- the latent transition was already silent, but bypassing the claim would have left the accepted anchor live for the BEL echo. Hardening (Rust parity): onApprovalRequested emitted 'attention.boundary' even when the requestId was already pending, so a duplicate request frame re-armed the truly-idle gate and re-rang the same pause. The boundary now emits only for a newly inserted id. Tests: new approval-pause tests attach the completion collector BEFORE onTurnCompleted (the existing pause test attached it after, which masked the bug and is left untouched): mid-pause completion with status 'completed' and 'failed' record nothing; a subsequent BEL output chunk mints nothing and the anchors are cleared; a duplicate approval request arms exactly one boundary. All four confirmed RED before the fix. Generated with Amplifier Co-Authored-By: Amplifier <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Codex panes often turned green and rang the bell while the agent was still working. A live spike against the real codex app-server (0.146.0) proved the root cause: freshell's proxy lane discarded
threadId/turnId/statusfromturn/started/turn/completed, so any thread's turn end on the shared app-server connection (sub-agent, review, fork threads) flipped the terminal idle — and the armed swallow flags then ate the real completion.What this PR does
Part 1 — thread-scoped, status-guarded turn completion (
fix/codex-turn-thread-scopecommits):threadId/turnId/statusthrough the proxy event path (Rust + Node)turn_abortedclears phase without recording a completionPart 2 — attention-bell policy (
feat/codex-attention-bellcommits): one bell means "come look at me"; one icon state means "I'm not doing anything for you".terminal.idlenow rings for all non-human stopping causes:failedturns ring (queued follow-up suppresses — work continues)/quit) stay silentPart 3 — review-driven hardening (final two commits):
Contract
Zero wire-shape changes —
terminal.idlestays{terminalId, at, reason: 'grace'|'queue-empty'}; contract freeze green. Doc comments updated to the new policy (never after a human-requested stop; rings on failed/death/approval causes).Validation
freshell-activity133,freshell-ws377, integration bins), fmt/clippy cleannpm run checkincl. contract freezeGenerated with Amplifier
Co-Authored-By: Amplifier [email protected]