Repository navigation
fix(agent-status): one owner per pane status row, so a nested agent can't relabel it (STA-9582) - #26408
fix(agent-status): one owner per pane status row, so a nested agent can't relabel it (STA-9582)#26408Jinwoo-H wants to merge 6 commits into
Conversation
…tion host Hook events carry a producer identity (agent, session, process, nestedIn); the pane's agentPresence records the owner and the sessions it has held. transitionHookPresence classifies each producer as owner, guest or claim (rules 1-5), shared by main for local panes and the relay for SSH/WSL panes. Guests leave the row unchanged and start one owner check per owner, and only the guest whose event proved the owner exited takes the pane. Main adopts relayed rows as given. Deletes the agent-type identity guess and the Codex-only root keeper on the host; the owner's sparse events carry its model and session forward instead.
…l a held owner's row A terminal signal (OSC, title, process-derived) that names a different agent than a held owner now leaves the row unchanged and only starts the owner check, matching the old identity fence. Once the check releases the pane the next signal writes normally; same-type signals and ownerless panes are unchanged. Main's pane presence decision moves to server-pane-presence.ts.
- One held event per pane, replayed on any exited verdict; replay re-runs the rule, so classification and the check's row-identity guard gate it. Fixes a lost handover when a terminal- or same-type-started check put the guest's own check in cooldown. - transitionHookPresence returns write or skip; probe on skip means hold and check, set only for a live unproven guest whose owner has a process. - PaneOwnerProbes.admit applies a transition for both main and the relay; the relay's checkPresence gate is gone. - The terminal fence moves to terminal ingest as a predicate; a released owner plus a signal naming another agent leaves the pane ownerless. - One currentOwner(row); owner fields never carry to a restarted process. - The relay's loopback bind moves to agent-hook-loopback-listener.ts.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 4 remain after this review. 📝 WalkthroughWalkthroughHook events now use pane ownership state to classify owner, nested, and guest producers. Local and relay paths use owner-process probes to decide whether held guest events can be applied. Presence records include normalized session history. Relayed rows use a separate adoption path. Terminal signals can preserve, release, or transfer ownership based on the current owner and process verdict. Priority: ⬇️ Low Merge Risk: 🟡 Moderate · up to A restarted agent may temporarily disappear from its pane status, and a failed owner check may interrupt relay status reporting. Resolve or explicitly accept these risks before merging. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description is detailed and covers the required change, rationale, testing, visual proof, compatibility, and checklist sections. The Linked Issue section names STA-9582 and STA-3754 but does not provide an actual issue link, which the template explicitly requires.
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
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.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
d7b6b407-85b2-4ce7-b260-710353aaf094
📒 Files selected for processing (21)
AGENTS.mdsrc/main/agent-hooks/server-agent-presence.test.tssrc/main/agent-hooks/server-ingest-remote.test.tssrc/main/agent-hooks/server-pane-owner-relayed.test.tssrc/main/agent-hooks/server-pane-owner.test.tssrc/main/agent-hooks/server/server-agent-presence.tssrc/main/agent-hooks/server/server-ingest-terminal.tssrc/main/agent-hooks/server/server-lifecycle.tssrc/main/agent-hooks/server/server-state.tssrc/main/agent-hooks/server/server-status-update.tssrc/relay/agent-hook-event-admission.tssrc/relay/agent-hook-loopback-listener.tssrc/relay/agent-hook-server.tssrc/relay/relay-agent-presence.tssrc/shared/agent-hook-listener/listener-event.tssrc/shared/agent-hook-presence-transition.test.tssrc/shared/agent-hook-presence-transition.tssrc/shared/agent-pane-owner-probes.test.tssrc/shared/agent-pane-owner-probes.tssrc/shared/agent-process-presence.tssrc/shared/agent-session-resume.ts
💤 Files with no reviewable changes (2)
- src/main/agent-hooks/server/server-state.ts
- src/main/agent-hooks/server/server-lifecycle.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review.
| const written = host.write(transition.event) | ||
| // Why after the write: the host's check is bound to the row it reads, so it must read this one. | ||
| if (transition.probe) { | ||
| this.probe(paneKey, transition.probe) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Hold the written event when a write also probes the owner.
A write with a probe happens when a different process of the owner's type reports into the pane. Example: Claude process A crashes without SessionEnd, and Claude process B starts in the same pane. transitionHookPresence classifies B as owner and writes B's event under A's presence. It also returns probe: A. If the check returns exited, the host check ends the pane. Main calls reconcileEndedProcessForPaneKeys. The relay publishes AgentProcessExit. This removes the working row that B just wrote. admit holds nothing on the write path, so no event is replayed. B gets the pane back only on its next hook. An idle B therefore stays invisible until the user prompts it again.
Register host.reapply in held on this path too. After the exit, the replay runs through the classifier again. The owner is now ended and sameProcess(ended.process, B) is false, so B claims the pane immediately.
Proposed fix
--- "a/src/shared/agent-pane-owner-probes.ts"
+++ "b/src/shared/agent-pane-owner-probes.ts"
@@ -40,11 +40,13 @@
}
return undefined
}
const written = host.write(transition.event)
// Why after the write: the host's check is bound to the row it reads, so it must read this one.
if (transition.probe) {
+ // Why: an exited verdict ends the row this write just produced; replay lets its producer claim it.
+ this.held.set(paneKey, { heldAt: this.now(), reapply: host.reapply })
this.probe(paneKey, transition.probe)
}
return written
}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const written = host.write(transition.event) | |
| // Why after the write: the host's check is bound to the row it reads, so it must read this one. | |
| if (transition.probe) { | |
| this.probe(paneKey, transition.probe) | |
| } | |
| const written = host.write(transition.event) | |
| // Why after the write: the host's check is bound to the row it reads, so it must read this one. | |
| if (transition.probe) { | |
| // Why: an exited verdict ends the row this write just produced; replay lets its producer claim it. | |
| this.held.set(paneKey, { heldAt: this.now(), reapply: host.reapply }) | |
| this.probe(paneKey, transition.probe) | |
| } |
| void this.deps.checkOwner(paneKey).then((verdict) => { | ||
| const held = this.held.get(paneKey) | ||
| if (verdict !== 'exited' || !held) { | ||
| return | ||
| } | ||
| this.held.delete(paneKey) | ||
| // Why: replay re-classifies against the released row, so it only lands if it may claim it. | ||
| if (this.now() - held.heldAt <= HELD_GUEST_WINDOW_MS) { | ||
| held.reapply() | ||
| } | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Catch rejections from the discarded owner-check chain.
void this.deps.checkOwner(paneKey).then(...) has no rejection handler. This chain runs host code that can throw:
- Main:
reconcileEndedProcessForPaneKeysinsidecheckAgentPresence. - Relay:
publish→applyEventinsideRelayAgentPresence.check. - Both hosts:
held.reapply(), which re-enters full admission and writes the row.
If any of these throws, the rejection is unhandled. The manifest declares Node 24. Since Node 15, Node terminates the process on an unhandled rejection by default. The relay is a standalone Node process, so a single bad replay can stop status reporting for every remote pane. Add a .catch that logs the error.
Proposed fix
--- "a/src/shared/agent-pane-owner-probes.ts"
+++ "b/src/shared/agent-pane-owner-probes.ts"
@@ -56,17 +56,23 @@
return
}
this.probes.set(paneKey, { owner: key, startedAt: now })
- void this.deps.checkOwner(paneKey).then((verdict) => {
- const held = this.held.get(paneKey)
- if (verdict !== 'exited' || !held) {
- return
- }
- this.held.delete(paneKey)
- // Why: replay re-classifies against the released row, so it only lands if it may claim it.
- if (this.now() - held.heldAt <= HELD_GUEST_WINDOW_MS) {
- held.reapply()
- }
- })
+ void this.deps
+ .checkOwner(paneKey)
+ .then((verdict) => {
+ const held = this.held.get(paneKey)
+ if (verdict !== 'exited' || !held) {
+ return
+ }
+ this.held.delete(paneKey)
+ // Why: replay re-classifies against the released row, so it only lands if it may claim it.
+ if (this.now() - held.heldAt <= HELD_GUEST_WINDOW_MS) {
+ held.reapply()
+ }
+ })
+ .catch((error: unknown) => {
+ // Why: ingest never awaits this chain, so a throw here must not become an unhandled rejection.
+ console.error('[pane-owner-probes] owner check failed', error)
+ })
}
private sweep(now: number): void {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void this.deps.checkOwner(paneKey).then((verdict) => { | |
| const held = this.held.get(paneKey) | |
| if (verdict !== 'exited' || !held) { | |
| return | |
| } | |
| this.held.delete(paneKey) | |
| // Why: replay re-classifies against the released row, so it only lands if it may claim it. | |
| if (this.now() - held.heldAt <= HELD_GUEST_WINDOW_MS) { | |
| held.reapply() | |
| } | |
| }) | |
| void this.deps | |
| .checkOwner(paneKey) | |
| .then((verdict) => { | |
| const held = this.held.get(paneKey) | |
| if (verdict !== 'exited' || !held) { | |
| return | |
| } | |
| this.held.delete(paneKey) | |
| // Why: replay re-classifies against the released row, so it only lands if it may claim it. | |
| if (this.now() - held.heldAt <= HELD_GUEST_WINDOW_MS) { | |
| held.reapply() | |
| } | |
| }) | |
| .catch((error: unknown) => { | |
| // Why: ingest never awaits this chain, so a throw here must not become an unhandled rejection. | |
| console.error('[pane-owner-probes] owner check failed', error) | |
| }) |
Source: Learnings
…surface - An event naming no agent (OSC without agentType, a hook with 'unknown') writes under the owner's agent type and keeps its model. - classifyTerminalSignal replaces the two terminal helpers; a relayed owner main cannot check falls back to freshness instead of blocking forever. - A held guest is tied to the owner it waited behind and replays only when that owner is found exited. - Every host check (runtime liveness, relay command-finished and reconnect, startup spool) goes through PaneOwnerProbes.check; the raw probe is private. - Relay publish reports whether the exit was applied, so exited always means released.
The SSH notification adapter copied a fixed field list into ingestRemote and left out agentPresence, so a relayed exit was never adopted over SSH and saved SSH rows had no owner or session. WSL already passed it through.
ELI5
Every terminal pane has one status row: the agent's icon, model, prompt, state, and the session Orca resumes later. Any agent started inside that pane (for example Claude running
codex exec) writes to the same row, and until now nothing said whose row it was, so the last writer won, field by field. This PR gives each row exactly one owner, the agent that claimed it first. Only the owner's events change the row. Another agent's events are "guests": they leave the row alone until the owner is proven gone.What Changed
Before (as the user sees it):
gpt-6-astra) and Codex's session id after Claude ran Codex as a tool (STA-9582).After:
Stopsettles the row.exited, or (for an owner whose process the host cannot check, e.g. Codex today or Windows) the same freshness rule used before.Mechanism:
nestedIn(the agent sessions it runs inside; the field and its rules exist now, a follow-up makes hook scripts send it).agentPresence) now also stores the owner's current session and up to 4 sessions it held before (for example across Claude/clear). It is persisted and validated with the row, and old peers ignore the new fields.transitionHookPresence(src/shared/agent-hook-presence-transition.ts), sorts every hook event, first match wins: same session or process as the owner → owner; nested inside a session the owner holds → guest; same agent type → owner (as before); different type → guest, unless the owner is restored from disk, or cannot be checked and is stale or done. A producer nested in another agent's session never claims an empty pane.PaneOwnerProbes.checkinsrc/shared/agent-pane-owner-probes.ts): hook-triggered checks, the runtime's liveness checks, the relay's command-finished and reconnect checks, and the startup spool check. When any of them finds that same owner exited and releases the pane, the held event is replayed through the same rule, so it lands only if it may now take the pane. A guest held behind an owner that has since left (for example on its own clean exit) is never replayed onto a later owner. A proven nested guest needs no check and is not held.agentPresence) to the desktop, as WSL already did; before, SSH dropped it, so a relayed exit was never adopted and saved SSH rows carried no owner. The relay forwards only the rows it builds, never guest events. The desktop adopts relayed rows as given, including their exit, and never re-decides them. A disconnect releases nothing; a replayed row gets the same verdict as the live one.resolveAgentStatusIdentity/shouldSuppressInheritedTerminalStatusinserver-status-update.ts) and the Codex-only block that kept the root model and session. The owner's own events now carry its model and session forward when they omit them, which covers the same case (child hooks, a relay that restarted) for every agent; a restarted process of the same type starts clean. The desktop renderer's own identity re-guess (agent-status-event-applicator.ts,agent-status-live-entry-builder.ts) is untouched; a later cleanup removes it.Why
The row had several partial owner concepts competing: a type-plus-freshness guess, a Codex-only keeper, Claude's process capture, and per-agent patches. Each fixed one field for one agent. One owner, decided once by the machine that can check the process, removes the guessing instead of adding another case. Alternatives considered:
Known limits (stated plainly):
Linked Issue
STA-9582 (also STA-3754).
Visual Proof
N/A. This changes what the host stores and publishes for status rows; the automated tests below drive real hook bodies through the local hook server and through a real relay into the desktop.
Testing
New:
server-pane-owner.test.ts(local hook server, real hook bodies):/clearsession history;New: the SSH session forwarding test adopts the relay's owner and its exit (through the real SSH notification adapter).
New:
server-pane-owner-relayed.test.ts, a real relay forwarding into the desktop'singestRemote: the STA-9582 sequence, a nested Claude's exit inside a working Codex not ending the pane (fix(agent-status): preserve hook presence when process checks cannot answer (step 1 of 3) #23947), guest promotion on the relay, replay after a disconnect giving the live verdict, and a relayed owner with its session surviving save and restore.New: unit tests for the classifier and the owner-check limiter.
Updated:
Ran: agent-hook, relay, shared, runtime and parity suites;
pnpm tc; oxlint; the changed-code quality gate.I manually tested these changes locally
Automated tests added/updated, or explained why not below
Review
Agent skill upstream boundary
Notes
agentPresence. Old readers ignore them.Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)