Repository navigation
refactor(agent-status): delete the caller-less connection status clear - #22033
brennanb2025 wants to merge 5 commits into
Conversation
`clearStatusEntriesForConnection` treated a dropped SSH transport as proof that the agents behind it had stopped: it deleted every status row stamped with the connection id, wiped the Codex and Claude subagent rosters, lead state and background-work latches for those panes, dropped their current-runtime authority observations, and emitted a transient cutoff. That is the inference `docs/reference/ssh-execution-boundary.md` exists to forbid. Loss of contact is never evidence of `exited`; a transport failure can only ever produce `unverifiable`. The row must survive the disconnect so replay or certified host teardown can settle it. Its last production caller was removed in #19785, which replaced the call in `SshRelaySession` with exactly that reasoning and left the function behind with tests but no callers. Removing it closes the door on a caller being reintroduced. Verified zero production callers before deleting: a repo-wide scan for the name (including ignored files, string and re-export forms) found only the definition and its tests, while a positive control for `clearPaneState` in the same scan returned its nine production call sites. Removed the function and the tests whose subject was its behaviour. Three tests used it only to stand in for a disconnect; since a disconnect no longer clears a row, they drop the call and keep their assertions. Two re-point at live mechanisms so their gates keep discriminating: `dropStatusEntry` for the authority-preserving status-row clear, and per-pane teardown for the observation/commitment store separation. The `transient` member of `AgentStatusClearIpcPayload` and the renderer watermark pruning that consumes it now have no producer. Left in place: removing them reaches into the renderer and dashboard and belongs in its own change.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughThe server removes Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to This change removes an unused connection-wide cleanup method while preserving state after unverifiable SSH loss and retaining certified teardown cleanup. No actionable merge risk is established. Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 1 system. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
A pure dead-code deletion: clearStatusEntriesForConnection and the tests that only exercised it.
- Removed
clearStatusEntriesForConnection— the connection-scoped row walk that deleted statuses, wiped Codex/Claude rosters and latches, dropped current-runtime authority observations, and emitted atransientclear cutoff. Repo-wide search finds zero production callers; the last one was removed in #19785, andSshRelaySession.teardownnow keeps the last observation except onshutdown. - Removed its tests — connection-cleanup batching, cutoff ordering, Codex roster wipe, and Claude background-work wipe all asserted the removed semantics.
- Re-pointed surviving tests at live mechanisms —
server-authority-evidence.test.tsclears the duplicate pane viaclearPaneState,server-last-status-write.test.tsusesdropStatusEntry(samepreserveAuthority: truepath), andserver-replay-evidence-clock.test.tsdrops the now-impossible pre-replay clear while keeping the no-restamp assertions.
I verified the "no caller" claim directly (including partial/string forms), confirmed the corrected behaviour still has a live guard at src/main/ssh/ssh-relay-session-agent-hooks.integration.test.ts:415, and ran the six changed test files (36 passed).
ℹ️ The producer-less transient clear reaches further than the PR body notes
The body names AgentStatusClearIpcPayload.transient and the watermark-pruning branch in agent-status-listeners.ts as the deliberate leftover. The same orphaned subsystem also includes the store action that branch calls and its state slice, so the follow-up PR is a little larger than described. Nothing here needs to change — flagging it only so the follow-up's scope is known.
Technical details
# Producer-less `transient` clear consumers
## Affected sites
- `src/renderer/src/hooks/ipc-events/agent-status-listeners.ts:40` — the `transient === true` branch is now unreachable because main only emits `{ paneKey }` clears (`server-authority-fences.ts:212`, `server-tab-cleanup.ts:99,146`).
- `src/renderer/src/store/slices/agent-status-cleanup-actions.ts:171` — `clearTransientAgentStatuses` is called only from that branch (and tests).
- `src/renderer/src/store/slices/agent-status-slice-contract.ts:44` / `agent-status.ts:100` — `transientClearedAgentStatusConnectionIds` slice and its `agent-status-ssh-connection-clear.test.ts` coverage.
- `src/main/synthetic-title-spinner.test.ts:56` — still models a connection-scoped transient clear.
## Required outcome
- The follow-up that removes the `transient` payload member should sweep these consumers in the same change so no producer-less branch or store field is left behind.
## Open questions for the human
- Is the `transient` wire shape intended to return (e.g. a future disconnect protocol), or is it permanently gone? If permanently gone, the union member can likely collapse to `{ paneKey: string }`.DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
…onnection-status-clear
 |
…tes discriminating Both tests used the deleted connection clear to delete a row or advance the connection watermark. Without it they passed on the surviving row, so neither the remembered observation time nor the per-connection watermark was gated. Dismiss the row instead, and assert a second pane on the same connection sorts after the hydrated one.
…atus clear The main-process clear was the only producer of the `transient` clear payload. With it gone, every consumer was dead code that still implemented the rule the execution boundary forbids: on a connection-scoped clear the renderer deleted every row on that connection, blocked live status routing for it until reconnect, the dashboard dropped older rows, and stats closed the connection's sessions. The payload never leaves the process (main sends it over Electron IPC to its own windows and to in-process subscribers), so no older host can emit it and nothing needs to stay for mixed versions. `AgentStatusClearIpcPayload` is now only a pane-scoped teardown. Removed the renderer listener branch, its per-connection watermark and the applicator gate, `clearTransientAgentStatuses` with its store field and SSH resets, the helper that scoped worktrees to a connection for it, the dashboard watermark, the stats recorder's connection arm, and the spinner helper that filtered it out. Tests that exercised only that path are removed; the connection-stamp and routing tests that also covered live behaviour are kept and re-pointed. Comments that still described a transport clear as live now describe the per-connection ordering watermark and row dismissal that remain.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This run re-reviewed the commits since the prior pullfrog review — the renderer half of the connection status clear, plus a test-strengthening commit.
- Deleted the renderer half of the connection status clear — the
transientmember ofAgentStatusClearIpcPayload, the listener branch and per-connection watermark, the applicator drop gate,clearTransientAgentStatuses(with thetransientClearedAgentStatusConnectionIdsstore field and its SSH reconnect/reset handling),collectWorktreeIdsForConnection, the dashboard watermark, the stats recorder's connection arm, andgetSyntheticTitleSpinnerPaneKeyToStop. - Re-pointed the surviving tests so they still discriminate —
server-replay-evidence-clock.test.tsdismisses the row instead of clearing a connection, andserver-last-status-hydrate-confirmation.test.tsnow asserts a second pane on the same connection sorts after the first; both would fail if the per-connection ordering watermark or the evidence-age map stopped surviving. - Renamed the remaining vocabulary — comments,
server-types, the shared IPC payloads,orchestration-fleet-agent-status-evidence.ts, anddocs/reference/agent-status-store.mdnow say "connection ordering watermark" and "dismissed row" rather than describing a transport clear.
I verified the deletion is complete (zero remaining references to the removed symbols), that the clear payload is Electron-IPC-only — the paired-web preload maps onClear to noopUnsubscribe — so mixed host/client versions remain safe, and that pnpm tc passes with the changed agent-hooks tests green (13/13).
ℹ️ The PR description's "Known leftover" paragraph is now stale
The body lists the transient payload member and the renderer watermark-pruning branch as deliberately out of scope for a follow-up, but the new commit removes exactly those. Nothing in the code needs changing — only the description paragraph, so a reviewer reading it isn't misled about what shipped here.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
The removed transient-clear test was also the only one that resolved a status snapshot after its listener bridge was disposed. Keep that half: a remounted bridge ignores the old snapshot and applies its own. Also drop the stale shard timing for the deleted test file and a comment that still named connection teardown.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This run re-reviewed the delta since the prior pullfrog review: a test-only commit that restores coverage lost when the renderer half was deleted, plus two small cleanups.
- Restored the disposed-bridge snapshot gate test — the old
blocks cleared snapshots across remount…test was dropped outright by72543c8, which left thedisposed/snapshotRequestIdguard inagent-status-ipc-bridge.tsunpinned. The newignores the late snapshot of a disposed bridge and applies the remounted onemounts, disposes, remounts, and asserts the disposed bridge's late snapshot is discarded while the remounted one applies. - Corrected the
emitPaneStatusClearedfan-out comment — "pane/connection teardown paths" → "pane teardown paths", matching the payload now that connection-scoped clears are gone. - Removed the stale shard-timing entry —
ci-shard-timings.jsonno longer lists the deletedagent-status-ssh-connection-clear.test.ts.
I verified the new test is not theatre: it passes at head, and neutralizing the disposal guard at agent-status-ipc-bridge.ts:124 makes it fail with expected "vi.fn()" to be called once, but got 2 times. I also confirmed no other deleted file left a dangling timing entry (the other affected test files were modified, not deleted).
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
Review status: ready (not merged)Problem. Orca kept a dead code path that treated a dropped SSH connection as proof the remote agents had exited and wiped their status rows. That inference is forbidden by the SSH execution-boundary rule: losing contact makes status What users see differently. Nothing. Disconnecting and reconnecting an SSH workspace keeps its agent rows today, and still does with this PR. This removes the code that could have undone that. What the review changed (pushed):
Deferred (not blocking):
Verified: two review rounds found no P0/P1/P2 issues caused by the PR, and the readiness checklist passed 8/8. Zero production callers on Not verified: only a user-clicked Disconnect was exercised live, not an involuntary network drop. Neither path can reach the deleted code on Noticed, not caused by this PR: while an SSH workspace is disconnected, its agent row keeps the spinning "working" indicator with "now". The behaviour is identical on |
|
Closing this cleanup PR. The disconnect caller was removed in #19785, and main has since removed the old desktop SSH cleanup paths; the remaining server method has no production callers. |
ELI5
When an SSH connection drops, Orca used to have code that said "the connection is gone, so the agents on that machine must be dead" and wiped their status rows. That guess is wrong: the agents usually keep running on the remote machine. Nothing calls that code any more, but it was still sitting in the codebase, ready to be wired back in. This PR deletes all of it.
What Changed
User-facing change: none. Disconnecting and reconnecting an SSH workspace already keeps its agent rows (verified below, identical on
mainand this branch). This is a deletion of unreachable code so the wrong rule cannot come back.The main-process hook server method
clearStatusEntriesForConnectionis deleted. Given a connection id, it deleted every status row stamped with that connection, wiped the Codex and Claude subagent rosters, lead state and background-work latches for those panes, dropped their authority observations, and sent a "connection cleared" signal (thetransientclear payload) to the renderer.Everything that only existed to react to that signal is deleted too, because it had no other producer: the
transientvariant ofAgentStatusClearIpcPayload(it is now only a per-pane teardown), the renderer listener branch and its per-connection watermark, the event-applicator gate that dropped older rows,clearTransientAgentStatusesand itstransientClearedAgentStatusConnectionIdsstore field (plus its resets in the SSH slice and removed-target cleanup), the helper that scoped worktrees to a connection for it, the dashboard popout's watermark, the stats recorder's per-connection close, and the title-spinner helper that filtered the signal out.Comments that still described a "transport clear" as live now describe what remains: the per-connection ordering watermark, and user dismissal of a row.
Why
docs/reference/ssh-execution-boundary.mdforbids exactly this inference: loss of contact is never evidence that a remote process exited; it can only make statusunverifiable. Wiping rows on disconnect orphans live remote work and invites a duplicate agent to be started on the same worktree.The last production caller was removed in #19785, which replaced the call in
SshRelaySessionteardown with that reasoning. Since then the function and its whole downstream chain were kept alive only by tests. Deleting the producer alone would leave a renderer path that still deletes a connection's rows and blocks its status routing whenever anything sends that signal again, so the door would only be half closed. Removing the whole chain means there is no longer a code path, type, or store field that expresses "a connection drop clears agent state".The signal never leaves the process: main sends it to its own windows over Electron IPC (
main-window-agent-status.ts) and to in-process subscribers (stats, title spinner). The paired web client'sonClearis a no-op and mobile never sees it. So no older or newer peer can send it, and no mixed-version compatibility code is needed. A stray payload without apaneKeyis dropped by the existing guard in the renderer listener.Caller evidence, with a positive control
A repo-wide scan (ignored files included, plus string, dynamic property-access and re-export forms) finds zero production callers of
clearStatusEntriesForConnectiononmain: one definition and test calls only. The same scan findsclearPaneState's production call sites, so the instrument finds callers when they exist. Thetransientpayload's only producer was that function; every other emitter sends{ paneKey }(server-tab-cleanup.ts,server-authority-fences.ts).Linked Issue
None. Standalone cleanup; the architecture review of the agent status store recommended landing this deletion on its own.
Visual Proof
No visual change is intended. Live QA on a macOS machine ran both
main(9f43115) and this branch (74ec29b) against a real SSH connection (a rig-only sshd on 127.0.0.1), with an agent status row posted through the real remote hook relay. On both builds the row is present while connected, after the user clicks Disconnect, and after reconnecting.main, after Disconnect: the rowqa-22033 - Claudestays under the workspace next to the "Connect" chip.This branch, connected:
This branch, after Disconnect (same as
main):This branch, after reconnecting:
Testing
Tests whose only subject was the deleted path are removed. Tests that also covered live behaviour were kept and re-pointed, and three gates were restored where the deletion had silently made them vacuous; each was checked by breaking the code it guards and confirming the test fails:
server-replay-evidence-clock.test.ts).server-last-status-hydrate-confirmation.test.ts), now deterministic instead of depending on the wall clock ticking.useIpcEvents-agent-status-pane-teardown.test.ts).agent-status-connection-stamp.test.ts; SSH routing still requires the connection to be connected (agent-status-connection-ownership.test.ts).tc:nodeandtc:webpass; the affected test files (64) pass; CI is green.Review
Reviewed in two rounds plus a readiness checklist. Also known: open PR #23881 adds two new test calls to
clearStatusEntriesForConnectionas a disconnect stand-in; whichever lands second must replace them withdropStatusEntry(pane). Open PRs #14635 and #12070 fix consequences of the deleted clear and become obsolete.Agent skill upstream boundary
Notes
Security, cross-platform, SSH, mobile, backwards compatibility and performance considered: pure deletion of in-process code; no wire, persistence, or platform-specific change. One map lookup per status event is removed from the renderer apply path.
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)