Repository navigation
fix(agents): lift the pane retirement fence when a live PTY re-attaches (STA-4114) - #14624
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan includes up to 10 reviews per rolling hour; 1 remains after this review. 📝 WalkthroughWalkthroughAdds pane authority restoration across the main process, IPC and preload APIs, renderer state, and PTY reattachment paths. Restoration removes pane retirement fences unless the tab remains closed. PTY binding triggers restoration before later agent hooks or status updates. Tests cover active and idle reattachments, closed-tab suppression, sibling-pane isolation, and direct SSH reattachment. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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 |
…es (STA-4114) A detach/reattach cycle retires the pane on both sides — the main hook server's closedAgentStatusPaneKeys and the renderer's recentlyRetiredAgentStatusPaneKeys — and nothing ever cleared either one. The pane then rejected every later working/done event for the rest of its life while Pi kept running normally in the same PTY. Bind the fence to the fact it asserts: retirement claims the pane is gone, and binding a live PTY to that exact pane disproves it. Clear both tombstones at the spawn/attach chokepoint and at the daemon-backed reattach path, so recovery does not depend on the agent starting another turn — a pane re-attached mid-turn only has agent_end left to report, and one re-attached while idle emits nothing at all. Closed-tab tombstones are a separate, stronger claim and are deliberately left standing.
e95c3f5 to
2c45061
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
…dary fix The idle re-attach assertion posted only before_agent_start, which #14626 turns into a fence-lifting turn boundary. Under that change the test passes whether or not restorePaneAuthority runs, so it stops pinning this PR's mechanism. Assert first on agent_end — a non-turn event — so the test proves the fence was already down when the hook arrived. Verified: with restorePaneAuthority neutered AND before_agent_start added to the restart predicate, the old assertion passes and the new one fails.
…-4114) Retirement fences the pane, its resolved owner, and every alias of it, then deletes those aliases. Restoring only the key handed to us left the rest standing — and a detached pane's process keeps posting the key it launched under (server.ts:1614), so the canonical re-attach case stayed suppressed with the fence apparently lifted. Verified against the real omp binary: the row came back under the stale launch pane instead of the detached owner. Record what each retirement fenced and replay it as a unit, rebuilding the aliases it deleted. Keys and aliases belonging to a closed tab are skipped, so the stronger claim survives and a live process is never routed back into a closed tab. The record is indexed by every fenced key and bounded at 1024 like the maps it mirrors; an evicted record degrades to the old behaviour. Also records why the renderer's restore IPC is deliberately unguarded: that map is not a mirror of main's (retirePtyAgentLaunchAuthority fences main directly on command-finished and PTY exit, and nothing pushes it back), and it is per-window and non-persisted, so gating the send on a local tombstone reintroduces this bug for exactly those panes.
Review pass: live
|
| Phase | Result |
|---|---|
| Baseline, no fence | Real row: agentType:"omp", real providerSession.session_id, lastAssistantMessage |
retirePaneAuthority |
A complete real omp run produced zero events — STA-4114 reproduced live |
restorePaneAuthority |
Next real run recovered fully |
Defect found and fixed: the fence was only half lifted
retirePaneAuthority fences the pane, its resolved owner, and every alias of it, then deletes those aliases. restorePaneAuthority lifted only the key it was handed. A detached pane's process keeps posting the key it launched under (see the comment at server.ts:1614), so the alias was the only thing routing it — and that key stayed fenced forever.
This is the canonical re-attach case, i.e. the one this PR's title names. Measured live, with the alias replay disabled, the recovered row landed under the stale launch pane instead of the detached owner — a split-brain row. With the fix it lands as one row under the correct owner.
The fix records what each retirement fenced and replays it as a unit, rebuilding the deleted aliases. Keys and aliases belonging to a closed tab are skipped, so the stronger claim survives and a live process is never routed back into a closed tab. The record is indexed by every fenced key and bounded at 1024, matching the caps on the maps it mirrors; an evicted record simply degrades to the previous behaviour.
The restore IPC is deliberately unguarded
The IPC fires on every bind, including when the renderer holds no tombstone. That looks like an obvious one-line fix and it is not — moving it inside the guard reintroduces this bug. recentlyRetiredAgentStatusPaneKeys is not a mirror of main's closedAgentStatusPaneKeys:
retirePtyAgentLaunchAuthority(orca-runtime.ts:13511) fences main directly viamain/index.ts:2586, fromonCommandFinished, the daemoncommand-finishedfact, andonPtyExit. Nothing pushes that back to the renderer — there is no main→renderer retirement channel at all.- The renderer map is per-window and non-persisted; main's is process-lifetime. A renderer reload empties one and not the other, and the daemon-backed reattach path is precisely the one that runs after a renderer restart against a surviving main.
Applying the guard leaves the PR's own suite fully green, so the tests pin neither behaviour — a comment now records the reasoning at the call site.
⚠️ Trap for anyone testing hooks locally
resolveHookCoords() in the generated extension prefers the endpoint file over ORCA_AGENT_HOOK_PORT. A shell launched inside Orca exports ORCA_AGENT_HOOK_ENDPOINT, so a test harness that only sets ORCA_AGENT_HOOK_PORT will silently post into your running Orca app instead of the server under test. Blank ORCA_AGENT_HOOK_ENDPOINT explicitly when driving hooks in a harness. I hit this before noticing it.
Test changes
- Re-armed the idle re-attach test. It asserted only on
before_agent_start, which fix(agent-hooks): reopen a retired pane on any provider's turn boundary (#12686) #14626 turns into a fence-lifting turn boundary — under that change it passes whether or notrestorePaneAuthorityruns. It now asserts first onagent_end, a non-turn event. Proven both ways: withrestorePaneAuthorityneutered andbefore_agent_startadded to the restart predicate, the old assertion passes and the new one fails. - Five new tests for the alias fix: detached-pane recovery routed to the owner key, legacy-numeric alias recovery, no alias rebuild into a closed tab, a closed tab's pane staying fenced after its tab id is LRU-evicted, and a newer alias not being clobbered by a replay.
Every new assertion was mutation-proven red-first — six mutations, each caught by its own test. Two of them initially caught nothing, which is how the closed-tab and eviction cases got real tests rather than unreachable defensive branches.
Verification
src/main/agent-hooks/andsrc/renderer/src/store/slices/— 325 files, 3240 passedtsc --noEmiton the node and web projects — both clean- oxlint, oxfmt, React Doctor, and
check-changed-code-quality.mjs— 0 new findings across 12 changed files git merge-treeclean against freshly fetchedorigin/main
Pre-existing failure, unrelated and not from this branch: terminal-ime-xterm-cancelled-preedit-visibility.test.ts (2 tests) fails identically at unmodified HEAD in a separate worktree.
…es (STA-4114) (stablyai#14624) * fix(agents): lift the pane retirement fence when a live PTY re-attaches (STA-4114) A detach/reattach cycle retires the pane on both sides — the main hook server's closedAgentStatusPaneKeys and the renderer's recentlyRetiredAgentStatusPaneKeys — and nothing ever cleared either one. The pane then rejected every later working/done event for the rest of its life while Pi kept running normally in the same PTY. Bind the fence to the fact it asserts: retirement claims the pane is gone, and binding a live PTY to that exact pane disproves it. Clear both tombstones at the spawn/attach chokepoint and at the daemon-backed reattach path, so recovery does not depend on the agent starting another turn — a pane re-attached mid-turn only has agent_end left to report, and one re-attached while idle emits nothing at all. Closed-tab tombstones are a separate, stronger claim and are deliberately left standing. * test(agent-hooks): re-arm the idle re-attach test against a turn-boundary fix The idle re-attach assertion posted only before_agent_start, which stablyai#14626 turns into a fence-lifting turn boundary. Under that change the test passes whether or not restorePaneAuthority runs, so it stops pinning this PR's mechanism. Assert first on agent_end — a non-turn event — so the test proves the fence was already down when the hook arrived. Verified: with restorePaneAuthority neutered AND before_agent_start added to the restart predicate, the old assertion passes and the new one fails. * fix(agents): lift a retired pane's whole fence, aliases included (STA-4114) Retirement fences the pane, its resolved owner, and every alias of it, then deletes those aliases. Restoring only the key handed to us left the rest standing — and a detached pane's process keeps posting the key it launched under (server.ts:1614), so the canonical re-attach case stayed suppressed with the fence apparently lifted. Verified against the real omp binary: the row came back under the stale launch pane instead of the detached owner. Record what each retirement fenced and replay it as a unit, rebuilding the aliases it deleted. Keys and aliases belonging to a closed tab are skipped, so the stronger claim survives and a live process is never routed back into a closed tab. The record is indexed by every fenced key and bounded at 1024 like the maps it mirrors; an evicted record degrades to the old behaviour. Also records why the renderer's restore IPC is deliberately unguarded: that map is not a mirror of main's (retirePtyAgentLaunchAuthority fences main directly on command-finished and PTY exit, and nothing pushes it back), and it is per-window and non-persisted, so gating the send on a local tombstone reintroduces this bug for exactly those panes.
…es (STA-4114) (stablyai#14624) * fix(agents): lift the pane retirement fence when a live PTY re-attaches (STA-4114) A detach/reattach cycle retires the pane on both sides — the main hook server's closedAgentStatusPaneKeys and the renderer's recentlyRetiredAgentStatusPaneKeys — and nothing ever cleared either one. The pane then rejected every later working/done event for the rest of its life while Pi kept running normally in the same PTY. Bind the fence to the fact it asserts: retirement claims the pane is gone, and binding a live PTY to that exact pane disproves it. Clear both tombstones at the spawn/attach chokepoint and at the daemon-backed reattach path, so recovery does not depend on the agent starting another turn — a pane re-attached mid-turn only has agent_end left to report, and one re-attached while idle emits nothing at all. Closed-tab tombstones are a separate, stronger claim and are deliberately left standing. * test(agent-hooks): re-arm the idle re-attach test against a turn-boundary fix The idle re-attach assertion posted only before_agent_start, which stablyai#14626 turns into a fence-lifting turn boundary. Under that change the test passes whether or not restorePaneAuthority runs, so it stops pinning this PR's mechanism. Assert first on agent_end — a non-turn event — so the test proves the fence was already down when the hook arrived. Verified: with restorePaneAuthority neutered AND before_agent_start added to the restart predicate, the old assertion passes and the new one fails. * fix(agents): lift a retired pane's whole fence, aliases included (STA-4114) Retirement fences the pane, its resolved owner, and every alias of it, then deletes those aliases. Restoring only the key handed to us left the rest standing — and a detached pane's process keeps posting the key it launched under (server.ts:1614), so the canonical re-attach case stayed suppressed with the fence apparently lifted. Verified against the real omp binary: the row came back under the stale launch pane instead of the detached owner. Record what each retirement fenced and replay it as a unit, rebuilding the aliases it deleted. Keys and aliases belonging to a closed tab are skipped, so the stronger claim survives and a live process is never routed back into a closed tab. The record is indexed by every fenced key and bounded at 1024 like the maps it mirrors; an evicted record degrades to the old behaviour. Also records why the renderer's restore IPC is deliberately unguarded: that map is not a mirror of main's (retirePtyAgentLaunchAuthority fences main directly on command-finished and PTY exit, and nothing pushes it back), and it is per-window and non-persisted, so gating the send on a local tombstone reintroduces this bug for exactly those panes.
ELI5
When a terminal pane is detached and re-attached, Orca had written down "this pane is gone" in two places and never crossed it out. Pi kept running fine in the same PTY, but every status update it sent was thrown away forever. Now the note gets crossed out the moment a live PTY re-attaches to the pane — because that is the moment it stops being true.
What Changed
Two independent tombstones both survived the re-attach, so a fix on either side alone still leaves the other suppressing. Both are now lifted at re-attach:
AgentHookServer.restorePaneAuthority(paneKey)clearsclosedAgentStatusPaneKeysfor the pane and its resolved owner alias.restoreAgentPaneAuthority(paneKey)store action clearsrecentlyRetiredAgentStatusPaneKeys, and forwards to main over a newagentStatus:restorePaneAuthoritychannel (preload + web-preload stub wired to match the existing retire path).bindActivePanePty) and the daemon-backed reattach path inpty-connection.ts, which inlines its own bind rather than calling the chokepoint — that second one is the exact path in this report.Why
Retirement is a claim: "this pane is gone." Binding a live PTY to that exact pane is proof the claim is false, available at the precise moment it stops being true. So the fence lifts on that proof.
The question this PR has to answer: is reviving on a live new turn sufficient, or should re-attach itself clear the tombstone? Re-attach itself — reactive revival is not sufficient. Turn-triggered revival infers pane liveness from a downstream symptom, and the inference has a gap the authoritative signal does not:
agent_end, not a new-turn event — so nothing revives the pane, and the turn's completion checkmark never lands. If that turn runs for twenty minutes, the pane reads dead for twenty minutes.before_agent_startis the only pi/omp/prime-agent new-turn event (isNewTurnEvent,agent-hook-listener.ts:2421). Every other pi lifecycle event a re-attached pane emits stays suppressed.Waiting for a hook to prove what the attach already proved is the same shape as the spinner bug in #14618: the fact was available at a lifecycle boundary, and the code chose to rediscover it later from a symptom. Bind to the boundary.
This is also the contract the codebase already uses elsewhere —
server.tsclearsclosedAgentStatusPaneKeyswhen a pane key migrates to a new owner (the alias-transfer path). Authoritative re-binding already lifts the fence there; the detach/reattach path simply never got wired up. This PR makes the two consistent.On the alternative in #14560 (revive on a live new turn): its instinct is right and its supporting work is genuinely good — reusing the shared source-aware
isNewTurnEventinstead of the hardcoded Claude predicate, and splitting classification from mutation ingetAgentStatusDispositionso a remote envelope that later fails payload validation cannot desync main from renderer (a race this report did not identify). But it is the wrong primary contract for the reasons above, and it carries a compatibility risk worth flagging: its new guard requiresevent.source !== undefined, whilesourceis optional on the payload type (agent-hook-listener.ts:329). A source-less remote envelope that previously restarted a retired pane would now suppress — its own test diff had to addsource: 'claude'to three previously source-lessingestRemotecalls. Perdocs/reference/remote-wire-compatibility.mdthat is a mixed-version concern.This PR does not supersede #14560. It deliberately does not touch
getAgentStatusDisposition's event predicate, so the two are orthogonal: proactive clearing is the primary contract, and a live-new-turn revival remains a reasonable secondary net for panes whose re-attach one side never observed (a remote relay, or main restarted without a renderer re-attach). If #14560 lands, thesource-optional guard should be resolved first.Deliberate non-goal: restoring does not bring back the rows retirement dropped — those are genuinely stale. It only re-opens the pane to future status.
Linked Issue
Fixes #14236
Visual Proof
N/A — no UI surface changed. The visible effect is that an existing surface (the working spinner / completion checkmark on a re-attached Pi pane) stops being suppressed. Capturing it needs a live detach/reattach of a running Pi process, which I could not stage here (see Testing).
Testing
Limitation stated plainly: the pi CLI is not installed on this machine, so this was fixed without a live reproduction. I did not observe the suppression or the fix in a running Pi pane and am not claiming otherwise. The manual-testing box below is unchecked.
To compensate, every new assertion was proven non-vacuous by mutating the source and confirming the test fails:
pty-connection.tscall sites: the reattach wiring assertion fails, confirming the attach path really is what lifts the fence.Coverage, main (
server.test.ts): re-attach restores status without a new turn for pi, omp, and prime-agent (each posts to its own/hook/<kind>and assertsagent_endalone is suppressed while retired, then accepted after restore); re-attach while idle re-opens the pane for a much later first turn; re-attach lifts neither the pane nor tab fence once the tab is closed.Coverage, renderer (
agent-pane-authority.test.ts): fence lifts so an in-flight turn can still reportdone; idle re-attach re-opens for a later turn; closed-tab tombstone is not lifted and main is not notified; sibling panes stay fenced when one pane is restored.vitest run src/main/agent-hooks/server.test.ts src/renderer/src/store/slices/ src/renderer/src/hooks/useIpcEvents.test.ts— 2699 + 302 passed.vitest run src/renderer/src/components/terminal-pane/pty-connection.test.ts— 579 passed.pnpm run typecheck:nodeandpnpm run typecheck:web— both pass. The third project was deliberately not run (OOM risk on this machine); no CLI code changed.oxlint+oxfmton all changed files, plus pre-commit hooks including React Doctor — clean.One robustness note found by the tests: the attach path treats a throw here as a failed reattach ("Reattach FAILED"), so the store call is an optional call — a status side effect must never be able to break terminal attach.
AI Disclosure
Review
isValidPaneKeyguard as the existing retire channel, and can only narrow suppression for a pane the renderer is actively binding. It cannot resurrect a closed tab.retirePaneAuthorityalready behaves there.closedAgentStatusPaneKeysis in-memory only (never persisted or hydrated), so there is no stored state to migrate.Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred) — scoped checks passed (see Testing); full suites left to CIAuthor