Repository navigation
Conversation
…ly home The resume fence that stops a settled orchestration worker being relaunched had two homes and a directional gap between them. `sleepingRecordFromEntry` derived `automaticResumeBlockedBy` solely from `automaticResumeBlockedPaneKeys`, which starts empty on every renderer boot and is never persisted, so the first `setAgentStatus` after any renderer start rebuilt the record without the flag. The hibernation planner then admitted the pane, killed a live worker PTY, and worktree activation relaunched it with `--resume`. The record is now the durable home. Every rebuild path resolves the flag through one predicate that reads the record first and the pane-key map second, gated on session identity, so `carryOverAutomaticResumeBlock` and the open-coded carry in `recordAgentProviderSession` collapse into it. `recoveryRecordMatches` treats the flag as significant, so a rebuild that would drop it is no longer accepted as an equal record. The planner and the hibernation coordinator's re-check now read the pane-key map too, which closes the window where a pane is fenced before any record exists. Main stops announcing each fence once per process. `prepare` re-announces every blocked pane on every pass and the renderer-startup handshake returns the fenced pane set, so a renderer that reloads, crashes or opens a second window re-derives its whole map instead of losing the fence. The announce and the lift edge are now committed only after the session write succeeds, so a staging failure retries rather than dropping the fence forever.
A settled orchestration worker's pane (automaticResumeBlockedBy: 'legacy-orchestration-worker') attached renderer-only: transport.attach registered IPC listeners without ever asking main to attach the daemon session. Main is the daemon's only client, so after an app restart its adapter never learned the id, pty:hasPty answered a fabricated false, and the visibility reconciler tore the tab down while the process still ran. The fence's intent is right, the mechanism was wrong. A fenced pane now takes the SAME deferred reattach as every other restored pane and carries the fence as attachOnly on the connect: main attaches the live session or refuses to mint one. A session its owner proves absent comes back as the existing exitedBeforeAttach terminal state, so the pane neither respawns nor retires a binding it may never replace. Deletes the renderer-only attach path (retained-legacy-pty-attach.ts, the legacyAttachOnlyPtyId short-circuit, and the SSH gate that skipped the deferred flow for these panes). Pre-commit hook skipped: it runs `pnpm install`, which fails on this worktree's symlinked node_modules. Its checks (oxlint, react-doctor oxlint, oxfmt) were run manually on every staged file.
…ed state Codex review of #19344 found the startup reply could re-fence a retired pane. Two paths, one defect: the reply was built somewhere other than the object that owns fence publication. The runner read `plan.blockedPanes` captured before it awaited workspace resolution, inventory and persistence, so a release or takeover landing inside that window produced a reply older than its own lift; and a pass whose `setWorkspaceSession` threw published nothing through the push channel yet still returned those keys, fencing a pane no later pass could retire. The persistence object is now the only publisher. It exposes the set it has actually committed, counted by a commit generation, and the runner reads that after the pass finishes rather than deriving anything from the starting plan. A pass that commits nothing reports the previous committed state, which also keeps an unreadable plan from ever reading as "no pane needs its fence". The generation travels with both channels, so the renderer applies main's whole committed set and drops a reply that a newer lift has already overtaken. That makes the seed authoritative instead of additive, which is what finally retires a fence on a pane that has no sleeping record to sweep. Manual sleep and hibernation rollback both committed a record captured before the await, erasing a fence delivered during the stop. Both now resolve the flag from state at commit time through the same predicate as every other rebuild. Also: the degraded-startup routing gate searched for the call the seed helper replaced, so its ordering assertion silently passed on -1; it now follows the helper and asserts the indirection still reaches main. The shared fake-CLI e2e fixture is re-materialized on demand, because the first spec file's teardown deleted it out from under the next file in the same worker.
Both specs that import completed-worker-retirement-fixture registered a
per-file `test.afterAll(cleanupCompletedWorkerFixture)`. That helper does
`rmSync(fakeCliDir, { recursive: true })`, and fakeCliDir holds the fake
`codex` executable itself, not just the ledger. Playwright loads the
module once per worker, so with `--workers=1` whichever spec file
finished first deleted the binary out from under every later file in the
run: the next spec launched a deleted path, wrote no lifecycle events,
and timed out waiting for them. Proven with a probe at the failing poll:
fakeCliDirExists false, ledgerExists false, ledger [].
The temp dir's lifetime is the worker process's, so the module now owns
its own teardown via process.once('exit') and no spec can end it. Ledger
accumulation was NOT the cause; both specs already truncate per test.
Separately, `liveness before reveal` sampled pty:hasPty once right after
backgroundMountTab, which only waits for the pane manager to exist. Main
learns the session when the pane's deferred reattach lands, so the sample
raced it (2 failures in 10 runs, order-independent). It now polls, and
stays soft so a slow attach cannot short-circuit the tab-teardown
assertion that follows. A restarted main that never attaches stays false
for the whole 20s window, so both assertions still fail on unfixed code.
Pre-commit hook skipped (ERR_PNPM_CMD_SHIM_CHMOD on this worktree's
symlinked node_modules); oxlint and oxfmt run manually on all three files.
…one home The fence was main-owned state stored inside a renderer-owned field. `sleepingAgentSessionsByPaneKey` is co-authored: the renderer emits the whole map and main applies it as a wholesale replace, so main's stamp survived only until the renderer's next write. That inversion is what produced the original P0 — the fence was erased on disk, not just in memory — and every guard added since was compensation for it: a carry predicate at each record builder, two equality hunks behind that predicate, a volatile pane-key map with a seeded startup reply, and main-side bookkeeping to synthesise lift edges for panes with no record. The fence now lives in `legacyWorkerResumeFencesByPaneKey`, a runtime-authored session field classified like `clientHostedBrowserPagesByWorktree`. It is absent from the renderer's patch builder, so the renderer cannot write it, and the persistence layer carries it across any write that never mentions it. Main rewrites the whole set every recovery pass, record or no record. Level-triggered: no announcement to consume once, no edge to miss, no generation to compare. The renderer reads it through ordinary session hydration and re-reads it when main pings that it changed; the ping carries no state, and reads are coalesced so the last read is the last write. Hibernation eligibility, the resume decision, the reattach choice and the park exemption all consult that one field. `automaticResumeBlockedBy` stays on the record as an outbound projection for clients too old to read the new field, stamped in `buildSleepingAgentSessionData` alone — the single function both persistence paths build records through. Deleted: the pane-key map and its writer, the startup seed helper and the reply shape it needed, `carriesAutomaticResumeBlock` and its call in every record writer, both equivalence hunks, `lastPlanBlockedPaneKeys`, the fence generation, and the record-sweeping half of `liftRetiredFences`. All five Codex findings and all five Opus findings are now unreachable rather than handled.
Remove renderer-declared attachOnly policy. Main reads the persisted host pane fence before early adoption, provider spawn and retirement, and sends structured observed-exit or unverifiable results without deleting history. Keep binding retirement coupled to replacement eligibility, preserve paired host evidence and recovery error routing, and cover absent-daemon restart.
…rce (7f36880) for the unification pass
|
Too many files changed for review (160 files, 100 file limit). Bypass the limit by tagging |
![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 — the unification pass that moves the last two settled-worker fence decisions into main and demotes the renderer's fence copy to advisory.
- Resume admission in main —
isSleepingAgentResumeBlockedmatches a spawn'sresumeProviderSessionagainst the host partition's sleeping records and returnsreattachUnverifiable/exitedBeforeAttachfor a fenced source, enforced atpty:spawn(begin + execute), the runtime controller (entry + claim path, rechecked after owner reconciliation), and pairedterminal.create/createAgentSession/ensureAgentSession. - Hibernation admission in main —
assertPtyHibernationAllowedthrowsagent_hibernation_automatic_resume_blockedfor akeepHistorykill of a fenced pane; non-reversible (user close) kills are unchanged. - Single persisted home — new runtime-authored
legacyWorkerResumeFencesByPaneKeyfield, written level-triggered per host byRuntimeLegacyWorkerTerminalRecoveryPersistence.prepare()and preserved across renderer session writes. - Advisory renderer mirror —
legacyWorkerResumeFencesByPaneKeystore state refreshed via a pull IPC on an invalidation ping; hydration installs fence + records atomically; the sleeping record is kept untilsettleAutomaticResumeSpawnruns after main admits or refuses. - Old-client negotiation —
negotiateTerminalCreateRefusalmaps a refusal toremote_runtime_unavailablefor clients withoutTERMINAL_FENCED_CREATE_RUNTIME_CAPABILITY; new optional wire fieldsexitedBeforeAttach/reattachUnverifiable. - Cleanup — renderer pending-read lane, refresh serialization,
setSleepingAgentAutomaticResumeBlocked,attachRetainedLegacyPty, and three ordering-only test files deleted.
ℹ️ Nitpicks
src/main/ssh/ssh-pending-pty-kill-replay.ts:38(and the mirror comment atssh-pending-pty-kill-replay.test.ts:207) still documentisPtyAlreadyGoneErroras matching/Session not found/i. That predicate now delegates toisSessionNotFoundRefusal(case-sensitive'Session not found: '), so the quoted regex is stale — the intent (matches on message text) still holds, but the literal is wrong.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (13)
💤 Files with no reviewable changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughThis change moves settled-worker resume fences into runtime-authored workspace-session state. It persists, hydrates, projects, and refreshes those fences across host partitions and renderer writes. PTY flows now attach existing sessions without replacement spawns and return structured refusal outcomes when absence cannot be verified. Renderer and RPC paths preserve pane bindings and negotiate compatibility through a new runtime capability. Tests cover local, SSH, remote-runtime, daemon-restart, persistence, and end-to-end restart scenarios. Merge Risk: 🟡 Moderate · up to This change centralizes resume-fence ownership and session recovery, but unresolved concerns remain around refusal cleanup, attach classification, retained bindings, and validation reliability. These should be addressed or explicitly accepted before merge. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description gives detailed change rationale, compatibility information, and verification results, but it omits several required template sections. It does not include the required Linked Issue entry, Visual Proof or an explicit N/A statement, Testing checklist, AI Disclosure, Review, Agent skill boundary, Notes, or Checklist. Resolution Add the missing template sections. Include a linked issue in the required format, provide before/after visual proof or write
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (6)
src/preload/api/pty-api.ts (1)
68-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the two new outcome fields on the shared contract.
The surrounding reply fields carry doc comments that state their meaning and host-version semantics. These two fields drive the renderer's most consequential decision in this PR:
reattachUnverifiablemust not be read as proof that the process died, whileexitedBeforeAttachis that proof. Record the distinction here, because this type is the contract every transport and renderer path reads.♻️ Proposed documentation
+ /** Main proved the named session was gone before the attach; no replacement was created. */ exitedBeforeAttach?: boolean + /** Main could not prove the session's state. Never a claim that the process died. */ reattachUnverifiable?: booleansrc/shared/workspace-session-resume-fences.ts (1)
13-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExport the fence sentinel as a shared constant.
The string
'legacy-orchestration-worker'is the persisted sentinel that decides whether a pane is fenced. It is repeated as an inline literal in this predicate and across the cohort's fixtures, for examplesrc/main/ipc/pty-ssh-undelivered-kill.test.tsline 132 andsrc/main/ipc/pty-attach-only-fenced-pane-spawn.test.tsline 87. A typo in the producer or in this comparison silently disables a fence, and no type check catches it.Export a named constant from this module and compare against it.
♻️ Proposed refactor
+export const LEGACY_ORCHESTRATION_WORKER_RESUME_FENCE = 'legacy-orchestration-worker' as const + export function readWorkspaceSessionResumeFences(- .filter(([, record]) => record.automaticResumeBlockedBy === 'legacy-orchestration-worker') + .filter( + ([, record]) => + record.automaticResumeBlockedBy === LEGACY_ORCHESTRATION_WORKER_RESUME_FENCE + )src/renderer/src/store/slices/store-sleep-pane-hibernation.test.ts (1)
220-220: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the test name with what the test asserts.
The name claims an empty advisory fence, but the body asserts only suppression rollback, the sleeping record, and the retained status row. It never reads
legacyWorkerResumeFencesByPaneKey. Either assert the fence set stays empty after the refusal, or rename the case to describe the rollback it verifies.src/main/runtime/runtime-legacy-worker-terminal-recovery-persistence.ts (1)
92-92: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMove the invalidation ping out of the write
tryblock.The session write at Line 91 already succeeded when
notifyFenceChangedruns. If the notifier throws, thecatchat Line 94 logs a write failure and returns, so the remaining hosts are never written. The missed ping is also sticky: the next pass seessameFenceSettrue for the committed host and does not ping again, so a live renderer keeps a stale fence set until reload.Isolate the ping so a notifier failure cannot be read as a write failure.
♻️ Proposed change
try { for (const [hostId, fenced] of fencedByHost) { const current = store.getWorkspaceSession(hostId) if (!current || sameFenceSet(current, fenced)) { continue } store.setWorkspaceSession({ ...current, legacyWorkerResumeFencesByPaneKey: fenced }, hostId) - this.notifyFenceChanged?.() + try { + this.notifyFenceChanged?.() + } catch (error) { + console.warn('[orchestration] legacy worker resume fence ping failed', error) + } } } catch (error) {src/renderer/src/components/terminal-pane/pr-base-pty-transport-test-loader.ts (1)
3-3: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDeclare
esbuildas a direct development dependency.The loader imports
esbuilddirectly, but the root package does not declare it. The current install resolves it throughelectron-viteandshamefullyHoist: true; this does not provide explicit dependency ownership.tests/e2e/settled-worker-resume-fence-restart.spec.ts (1)
31-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffExtract the duplicated settled-worker setup into the shared fixture module. Both new specs copy
findSecondaryWorktreeandbackgroundMountTabverbatim, then repeat the same launch-to-worker_donesequence: settings update,terminal.resolvePane,orchestration.runCreate,orchestration.taskCreate,orchestration.workerStart, worker PTY discovery, transcript seeding, thesetAgentStatusworking/done pair, and theorchestration.sendcompletion. Both files already import./helpers/completed-worker-retirement-fixture, so that module is the natural owner. A future change to the orchestration RPC shape currently needs two edits that can drift apart.
tests/e2e/settled-worker-resume-fence-restart.spec.ts#L31-L88: delete the localfindSecondaryWorktreeandbackgroundMountTabdefinitions and import them from the shared fixture module; call a shared setup helper for the launch-to-worker_donesequence and keep only the fence-persistence and reload assertions in this spec.tests/e2e/settled-worker-tab-survives-restart.spec.ts#L35-L88: delete the identical local helper definitions and use the same shared imports and setup helper; keep only the daemon-session-gone branching, the reveal transition capture, and the persisted-layout assertions in this spec.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Team
Run ID: 1eba99fe-6fdc-48fe-a59f-05f440b03638
📒 Files selected for processing (156)
config/reliability-gates.jsoncsrc/main/daemon/daemon-errors.tssrc/main/daemon/daemon-pty-adapter-attach-only-ownership.test.tssrc/main/ipc/pty-attach-only-fenced-pane-spawn.test.tssrc/main/ipc/pty-resume-claim-ensure-fence.test.tssrc/main/ipc/pty-runtime-kill-and-exit.test.tssrc/main/ipc/pty-ssh-undelivered-kill.test.tssrc/main/ipc/pty/ipc/renderer-kill.tssrc/main/ipc/pty/ipc/spawn-begin.tssrc/main/ipc/pty/ipc/spawn-execute.tssrc/main/ipc/pty/ipc/spawn-run.tssrc/main/ipc/pty/pane/hibernation-admission.tssrc/main/ipc/pty/pane/stable-owner-retirement.tssrc/main/ipc/pty/pane/stable-owner.tssrc/main/ipc/pty/pane/stable-pane-fenced-evidence.test.tssrc/main/ipc/pty/pane/stable-pane-final-fence-recheck.test.tssrc/main/ipc/pty/pane/stable-pane-resume-fence-host.test.tssrc/main/ipc/pty/pane/stable-pane-resume-fence.tssrc/main/ipc/pty/provider/liveness.tssrc/main/ipc/pty/runtime/kill.tssrc/main/ipc/pty/runtime/spawn-early.tssrc/main/ipc/pty/runtime/spawn-execute.tssrc/main/ipc/pty/runtime/spawn.tssrc/main/orca-profiles/profile-project-session-field-disposition.tssrc/main/persistence/legacy-worker-session-projection.tssrc/main/persistence/loading-store/session-host-partitions.tssrc/main/persistence/loading-store/store-runtime-authored-session-fences.test.tssrc/main/persistence/restoring-sessions/session-worktree-ownership.tssrc/main/persistence/runtime-authored-workspace-session-fields.test.tssrc/main/persistence/runtime-authored-workspace-session-fields.tssrc/main/providers/pty-spawn-result.tssrc/main/runtime/orca-runtime-create-terminal.tssrc/main/runtime/orca-runtime-fence-automation-owner.tssrc/main/runtime/orca-runtime-get-agent-session-execution-namespace.tssrc/main/runtime/orca-runtime-preserved-branch-cleanup.tssrc/main/runtime/orca-runtime-terminal-attach-refusal.test.tssrc/main/runtime/orca-runtime-tests/terminal-output-and-worker-recovery-part-02.spec.tssrc/main/runtime/orca-runtime-tests/terminal-output-and-worker-recovery-part-03.spec.tssrc/main/runtime/orca-runtime-tests/terminal-output-and-worker-recovery-part-04.spec.tssrc/main/runtime/orca-runtime-tests/terminal-output-and-worker-recovery-part-05.spec.tssrc/main/runtime/orca-runtime-tests/terminal-output-and-worker-recovery-part-06.spec.tssrc/main/runtime/rpc/methods/agent-session.tssrc/main/runtime/rpc/methods/terminal/terminal-lifecycle-methods.tssrc/main/runtime/rpc/terminal-create-refusal-negotiation.test.tssrc/main/runtime/rpc/terminal-create-refusal-negotiation.tssrc/main/runtime/runtime-legacy-worker-host-write-publication.test.tssrc/main/runtime/runtime-legacy-worker-recovery-publication-order.test.tssrc/main/runtime/runtime-legacy-worker-session-publication.test.tssrc/main/runtime/runtime-legacy-worker-terminal-recovery-persistence.tssrc/main/runtime/runtime-legacy-worker-terminal-resume-fence.test.tssrc/main/runtime/runtime-notifier-contract.tssrc/main/runtime/runtime-pty-controller-contract.tssrc/main/runtime/terminal-attach-refusal.tssrc/main/runtime/terminal-create-pane-identity.tssrc/main/runtime/terminal-create-reveal.tssrc/main/startup/main-process-ipc-bootstrap.tssrc/main/window/runtime-window-lifecycle.tssrc/preload/api/agent-status-api.tssrc/preload/api/agent-status-bridge.tssrc/preload/api/app-api.tssrc/preload/api/app-bridge.tssrc/preload/api/pty-api.tssrc/renderer/src/components/terminal-pane/ipc-pty-attach-evidence.test.tssrc/renderer/src/components/terminal-pane/ipc-pty-connect.tssrc/renderer/src/components/terminal-pane/pr-base-pty-transport-test-loader.tssrc/renderer/src/components/terminal-pane/pty-connection-agent-session-resume.test.tssrc/renderer/src/components/terminal-pane/pty-connection-test-store-fixtures.tssrc/renderer/src/components/terminal-pane/pty-connection/deferred-session-attach.tssrc/renderer/src/components/terminal-pane/pty-connection/deferred-session-reattach-choice.test.tssrc/renderer/src/components/terminal-pane/pty-connection/deferred-session-reattach-choice.tssrc/renderer/src/components/terminal-pane/pty-connection/deferred-session-reattach-connect.tssrc/renderer/src/components/terminal-pane/pty-connection/fresh-spawn-eligibility.tssrc/renderer/src/components/terminal-pane/pty-connection/fresh-spawn-start.tssrc/renderer/src/components/terminal-pane/pty-connection/reattach-result-handler.tssrc/renderer/src/components/terminal-pane/pty-connection/reattach-unverifiable-result.test.tssrc/renderer/src/components/terminal-pane/pty-connection/retained-legacy-pty-attach.tssrc/renderer/src/components/terminal-pane/pty-connection/run-deferred-connect.tssrc/renderer/src/components/terminal-pane/pty-connection/sleeping-record-access-recordless-fence.test.tssrc/renderer/src/components/terminal-pane/pty-connection/sleeping-record-access.tssrc/renderer/src/components/terminal-pane/pty-connection/unverifiable-reattach-recovery.test.tssrc/renderer/src/components/terminal-pane/pty-connection/unverifiable-reattach-recovery.tssrc/renderer/src/components/terminal-pane/pty-transport-spawn-errors.test.tssrc/renderer/src/components/terminal-pane/pty-transport-types.tssrc/renderer/src/components/terminal-pane/remote-hidden-output-restore-outcomes.test.tssrc/renderer/src/components/terminal-pane/remote-hidden-output-restore-unavailable-banner.repro.test.tssrc/renderer/src/components/terminal-pane/remote-runtime-pr-base-refusal-compatibility.test.tssrc/renderer/src/components/terminal-pane/remote-runtime-pty-transport-fenced-outcome.test.tssrc/renderer/src/components/terminal-pane/remote-runtime-pty-transport.tssrc/renderer/src/components/terminal-pane/sleeping-record-park-exemption.test.tssrc/renderer/src/components/terminal-pane/sleeping-record-park-exemption.tssrc/renderer/src/components/terminal-pane/terminal-cold-park-subscription-narrowing.react185.test.tsxsrc/renderer/src/components/terminal-pane/use-terminal-tab-cold-parking.test.tssrc/renderer/src/components/terminal-pane/use-terminal-tab-cold-parking.tssrc/renderer/src/hooks/ipc-events/agent-status-listeners.tssrc/renderer/src/hooks/useIpcEvents-lifecycle.test.tssrc/renderer/src/lib/agent-hibernation-coordinator.tssrc/renderer/src/lib/agent-hibernation-pane-eligibility-resume-fence.test.tssrc/renderer/src/lib/agent-hibernation-pane-eligibility.tssrc/renderer/src/lib/agent-hibernation-planner-snapshot.tssrc/renderer/src/lib/agent-hibernation-planner.test.tssrc/renderer/src/lib/agent-hibernation-planner.tssrc/renderer/src/lib/automatic-resume-spawn-settlement.test.tssrc/renderer/src/lib/automatic-resume-spawn-settlement.tssrc/renderer/src/lib/host-mirrored-pane-resume-replay.test.tssrc/renderer/src/lib/legacy-worker-old-source-resume.test.tssrc/renderer/src/lib/legacy-worker-resume-fence-refresh.test.tssrc/renderer/src/lib/legacy-worker-resume-fence-refresh.tssrc/renderer/src/lib/live-resume-anchor-record.tssrc/renderer/src/lib/pi-session-resume-wake.test.tssrc/renderer/src/lib/resume-sleeping-agent-session-direct-ssh-hydration-gap.test.tssrc/renderer/src/lib/resume-sleeping-agent-session-legacy-worker.test.tssrc/renderer/src/lib/resume-sleeping-agent-session-remote-compat.test.tssrc/renderer/src/lib/resume-sleeping-agent-session-replay.test.tssrc/renderer/src/lib/resume-sleeping-agent-session.test.tssrc/renderer/src/lib/resume-sleeping-agent-session.tssrc/renderer/src/lib/sleeping-agent-session-launch.tssrc/renderer/src/lib/workspace-session-resume-fence-hydration.test.tssrc/renderer/src/lib/workspace-session-snapshot.tssrc/renderer/src/lib/workspace-session.tssrc/renderer/src/lib/worktree-reactivation-preserved-pane-replacement.test.tssrc/renderer/src/lib/worktree-reactivation-tab-forkbomb.test.tssrc/renderer/src/store/slices/agent-sleep-resume-fence-races.test.tssrc/renderer/src/store/slices/agent-status-manual-sleep-capture.test.tssrc/renderer/src/store/slices/agent-status-open-tab-resume-fence.test.tssrc/renderer/src/store/slices/agent-status-provider-session-actions.tssrc/renderer/src/store/slices/agent-status-provider-session.test.tssrc/renderer/src/store/slices/agent-status-recovery-actions.tssrc/renderer/src/store/slices/agent-status-recovery-collection.tssrc/renderer/src/store/slices/agent-status-sleeping-records.tssrc/renderer/src/store/slices/agent-status-slice-contract.tssrc/renderer/src/store/slices/agent-status.tssrc/renderer/src/store/slices/store-sleep-pane-hibernation.test.tssrc/renderer/src/store/slices/workspace-scoped-resume-fence-hydration.test.tssrc/renderer/src/store/terminals/terminal-pane-hibernation.tssrc/renderer/src/store/terminals/workspace-terminal-hydration-patch.tssrc/renderer/src/store/terminals/workspace-terminal-hydration.tssrc/renderer/src/web/preload-api/web-agent-status-api.tssrc/renderer/src/web/preload-api/web-app-api.tssrc/shared/protocol-version.tssrc/shared/pty-attach-absence-evidence.tssrc/shared/remote-runtime-client-capabilities.tssrc/shared/runtime-terminal-contracts.tssrc/shared/terminal-fenced-create-capability.test.tssrc/shared/terminal-pane-resume-fence.test.tssrc/shared/terminal-pane-resume-fence.tssrc/shared/workspace-session-host-field-ownership.tssrc/shared/workspace-session-resume-fences.tssrc/shared/workspace-session-schema-field-coverage.test.tssrc/shared/workspace-session-schema.tssrc/shared/workspace-session-state-types.tstests/e2e/completed-worker-retirement-resume.spec.tstests/e2e/helpers/completed-worker-retirement-fixture.tstests/e2e/settled-worker-resume-fence-restart.spec.tstests/e2e/settled-worker-tab-survives-restart.spec.tstests/fixtures/terminal/README.mdtests/fixtures/terminal/pr-base-1d1b73c40850-pty-transport.txt
💤 Files with no reviewable changes (7)
- src/renderer/src/lib/sleeping-agent-session-launch.ts
- src/renderer/src/store/slices/agent-status-provider-session-actions.ts
- tests/e2e/completed-worker-retirement-resume.spec.ts
- src/renderer/src/store/slices/agent-status-recovery-actions.ts
- src/renderer/src/components/terminal-pane/pty-connection/retained-legacy-pty-attach.ts
- src/renderer/src/components/terminal-pane/pty-connection/run-deferred-connect.ts
- src/renderer/src/store/slices/agent-status-recovery-collection.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.
| if (ctx.result.exitedBeforeAttach || ctx.result.reattachUnverifiable) { | ||
| return | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Reset the provisional execution context before this return.
This return bypasses the cleanup at Lines 123-130. If preparePtyExecutionContext ran and spawnForStablePane returns exitedBeforeAttach or reattachUnverifiable, the runtime retains the provisional context for ctx.effectiveSessionAppId.
Apply the same reset used by the catch path before returning. This prevents stale execution state after an attach-only outcome.
Proposed fix
if (ctx.result.exitedBeforeAttach || ctx.result.reattachUnverifiable) {
+ if (
+ (ctx.isMintedSessionId || ctx.preparedProvisionalExecutionContext) &&
+ ctx.effectiveSessionAppId
+ ) {
+ ctx.deps.runtime?.preparePtyExecutionContext?.(ctx.effectiveSessionAppId, null, {
+ resetIncarnation: true
+ })
+ }
return
}📝 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.
| if (ctx.result.exitedBeforeAttach || ctx.result.reattachUnverifiable) { | |
| return | |
| } | |
| if (ctx.result.exitedBeforeAttach || ctx.result.reattachUnverifiable) { | |
| if ( | |
| (ctx.isMintedSessionId || ctx.preparedProvisionalExecutionContext) && | |
| ctx.effectiveSessionAppId | |
| ) { | |
| ctx.deps.runtime?.preparePtyExecutionContext?.(ctx.effectiveSessionAppId, null, { | |
| resetIncarnation: true | |
| }) | |
| } | |
| return | |
| } |
| blockedPanes: [{ paneKey: 'a', worktreeId: 'local', contractVersion: 1, settled: true }] | ||
| } | ||
| p.prepare() | ||
| expect(notify).toHaveBeenCalled() |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Make the retry assertion detect the second announcement.
Line 49 already asserts one notify call. After the second prepare(), expect(notify).toHaveBeenCalled() passes from that first call alone. The retry announcement is not verified, so a regression that stops publishing after the sibling host disappears still passes.
Assert the exact call count, or reset the mock before the second pass.
💚 Proposed test fix
fail = false
plan = {
...plan,
blockedPanes: [{ paneKey: 'a', worktreeId: 'local', contractVersion: 1, settled: true }]
}
+ notify.mockClear()
p.prepare()
- expect(notify).toHaveBeenCalled()
+ expect(notify).toHaveBeenCalledOnce()| if (spawnResult.exitedBeforeAttach || spawnResult.reattachUnverifiable) { | ||
| return context.isDestroyed() ? undefined : spawnResult | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
# Description: Resolve PtyConnectResult and the projection helper to check the refusal return shape.
set -euo pipefail
# Definition of the transport result contract.
fd -t f 'pty-transport-types.ts' src/renderer --exec ast-grep outline {} --items all
echo '=== PtyConnectResult declaration ==='
rg -nP -A30 'type\s+PtyConnectResult\b' --type=ts src/renderer
echo '=== projectIpcPtyConnectResult implementation ==='
ast-grep run --lang typescript --pattern 'function projectIpcPtyConnectResult($$$) { $$$ }' src/renderer
echo '=== consumers that read the refusal flags ==='
rg -nP -C3 '\b(reattachUnverifiable|exitedBeforeAttach)\b' --type=ts src/renderer src/sharedRepository: stablyai/orca
Length of output: 5065
🤖 get_repo_knowledge executed:
get_repo_knowledge stablyai/orca /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/learnings
Length of output: 34610
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '=== changed connect path ==='
cat -n src/renderer/src/components/terminal-pane/ipc-pty-connect.ts | sed -n '45,180p'
echo '=== projection helper definitions and call sites ==='
rg -n -C5 'projectIpcPtyConnectResult' src
echo '=== refusal flag usages ==='
rg -n -C4 '\b(reattachUnverifiable|exitedBeforeAttach)\b' src/renderer/src/components/terminal-pane src/sharedRepository: stablyai/orca
Length of output: 47075
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '=== projection implementation ==='
cat -n src/renderer/src/components/terminal-pane/ipc-pty-connect-result.ts | sed -n '1,120p'
echo '=== spawn result contract and refusal construction ==='
rg -n -C8 'spawnIpcPty|IpcPtySpawnResult|exitedBeforeAttach|reattachUnverifiable' \
src/renderer/src/components/terminal-pane/ipc-pty-spawn-request.ts \
src/renderer/src/components/terminal-pane/ipc-pty-connect-result.tsRepository: stablyai/orca
Length of output: 6832
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n src/renderer/src/components/terminal-pane/ipc-pty-connect-result.tsRepository: stablyai/orca
Length of output: 3154
🏁 Script executed:
cat -n src/renderer/src/components/terminal-pane/ipc-pty-connect-result.tsRepository: stablyai/orca
Length of output: 3154
Return a narrow refusal result.
PtyConnectResult declares both refusal flags. This branch returns IpcPtySpawnResponse before projectIpcPtyConnectResult, so it can forward optional snapshot, replay, cold-restore, or internal incarnation fields. Return only id and the applicable refusal flag, consistent with the other refusal branch.
| ) | ||
| const legacyWorkerOwnsPane = session.isLegacyWorkerAutomaticResumeBlocked() | ||
| if (gate.enterDeferredFlow && (!legacyWorkerOwnsPane || !gate.sshConnected)) { | ||
| if (gate.enterDeferredFlow) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep a fenced binding until main admits replacement.
Line 63 now sends fenced SSH panes into this flow. If the output callback records isSshSessionGoneError and connect() resolves undefined, the later branch clears the layout and tab PTY binding before it starts recovery. A fence can then reject replacement and leave the pane without its retained binding.
Route this fenced outcome through recoverUnverifiableReattach, or defer binding removal until main returns an explicit admitted or terminal result.
| return (error instanceof Error ? error.message : String(error)).includes( | ||
| SESSION_NOT_FOUND_MESSAGE_PREFIX |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Match the owner-authored marker at the start of the message.
decodeDaemonResponseError and SessionNotFoundError treat SESSION_NOT_FOUND_MESSAGE_PREFIX as a prefix, but this helper accepts it anywhere. A different provider or transport error can therefore pass isPtyAlreadyGoneError as definitive session absence and enter the wrong recovery path. Use startsWith here, or preserve an explicit owner marker through every wrapper.
Proposed fix
- return (error instanceof Error ? error.message : String(error)).includes(
+ return (error instanceof Error ? error.message : String(error)).startsWith(📝 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.
| return (error instanceof Error ? error.message : String(error)).includes( | |
| SESSION_NOT_FOUND_MESSAGE_PREFIX | |
| return (error instanceof Error ? error.message : String(error)).startsWith( | |
| SESSION_NOT_FOUND_MESSAGE_PREFIX |
| return ( | ||
| session.legacyWorkerResumeFencesByPaneKey ?? | ||
| Object.fromEntries( | ||
| Object.entries(session.sleepingAgentSessionsByPaneKey ?? {}) | ||
| .filter(([, record]) => record.automaticResumeBlockedBy === 'legacy-orchestration-worker') | ||
| .map(([key]) => [key, true as const]) | ||
| ) | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
# Description: Find callers of readWorkspaceSessionResumeFences that mutate or store the returned map.
set -euo pipefail
echo '=== call sites ==='
rg -nP -C6 '\breadWorkspaceSessionResumeFences\s*\(' --type=ts
echo '=== direct writes to the canonical field ==='
rg -nP -C4 'legacyWorkerResumeFencesByPaneKey\s*(\[|\.|=)' --type=tsRepository: stablyai/orca
Length of output: 170
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '=== target file ==='
cat -n src/shared/workspace-session-resume-fences.ts
echo '=== matching symbol and related field references ==='
rg -n -P -C5 'readWorkspaceSessionResumeFences|legacyWorkerResumeFencesByPaneKey|sleepingAgentSessionsByPaneKey' --type=ts .Repository: stablyai/orca
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
log=/tmp/coderabbit-shell-logs/shell-output-thjD5A
echo '=== target implementation from captured output ==='
sed -n '1,80p' "$log"
echo '=== reader references only ==='
rg -n -C8 'readWorkspaceSessionResumeFences' "$log" || true
echo '=== canonical field references in non-test source ==='
rg -n -C5 'legacyWorkerResumeFencesByPaneKey' src --type=ts \
-g '!*.test.ts' -g '!*.test.tsx' | head -n 240Repository: stablyai/orca
Length of output: 47246
🤖 get_repo_knowledge executed:
get_repo_knowledge stablyai/orca /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/learnings /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/conventions
Length of output: 14187
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '=== hydration caller ==='
sed -n '1,225p' src/renderer/src/store/terminals/workspace-terminal-hydration.ts
echo '=== persistence caller and writer ==='
sed -n '160,245p' src/main/runtime/runtime-legacy-worker-terminal-recovery-persistence.ts
echo '=== shared predicate caller ==='
cat -n src/shared/terminal-pane-resume-fence.ts
echo '=== non-test renderer updates to the canonical field ==='
rg -n -C6 'legacyWorkerResumeFencesByPaneKey' src/renderer/src --type=ts \
-g '!*.test.ts' -g '!*.test.tsx' | head -n 320Repository: stablyai/orca
Length of output: 44220
Return a copy on the canonical branch so callers cannot mutate the source session.
readWorkspaceSessionResumeFences returns session.legacyWorkerResumeFencesByPaneKey by reference when present, but creates a new object on the fallback branch. Because the return type is mutable, a caller that mutates the result can modify the source session in place. Return a copy on both branches to keep the contract uniform.
🛡️ Proposed fix
): Record<string, true> {
- return (
- session.legacyWorkerResumeFencesByPaneKey ??
- Object.fromEntries(
- Object.entries(session.sleepingAgentSessionsByPaneKey ?? {})
- .filter(([, record]) => record.automaticResumeBlockedBy === 'legacy-orchestration-worker')
- .map(([key]) => [key, true as const])
- )
- )
+ const canonical = session.legacyWorkerResumeFencesByPaneKey
+ if (canonical) {
+ return { ...canonical }
+ }
+ return Object.fromEntries(
+ Object.entries(session.sleepingAgentSessionsByPaneKey ?? {})
+ .filter(([, record]) => record.automaticResumeBlockedBy === 'legacy-orchestration-worker')
+ .map(([key]) => [key, true as const])
+ )
}📝 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.
| return ( | |
| session.legacyWorkerResumeFencesByPaneKey ?? | |
| Object.fromEntries( | |
| Object.entries(session.sleepingAgentSessionsByPaneKey ?? {}) | |
| .filter(([, record]) => record.automaticResumeBlockedBy === 'legacy-orchestration-worker') | |
| .map(([key]) => [key, true as const]) | |
| ) | |
| ) | |
| const canonical = session.legacyWorkerResumeFencesByPaneKey | |
| if (canonical) { | |
| return { ...canonical } | |
| } | |
| return Object.fromEntries( | |
| Object.entries(session.sleepingAgentSessionsByPaneKey ?? {}) | |
| .filter(([, record]) => record.automaticResumeBlockedBy === 'legacy-orchestration-worker') | |
| .map(([key]) => [key, true as const]) | |
| ) |
| // Why re-materialized rather than written once at import: the module is shared across spec files in | ||
| // a worker, and the first file's `cleanupCompletedWorkerFixture` removes the whole directory. A | ||
| // later file in the same worker would otherwise launch a `codex` that no longer exists. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the stale reference to the removed cleanupCompletedWorkerFixture export.
This change deletes cleanupCompletedWorkerFixture and replaces it with the process.once('exit', ...) handler at Line 111. The comment still names that export as the reason for re-materializing the directory. It also contradicts the comment at Lines 106-110, which states that only the process may end the directory's lifetime. A reader can conclude that a per-file teardown still exists and re-add one.
📝 Proposed comment fix
-// Why re-materialized rather than written once at import: the module is shared across spec files in
-// a worker, and the first file's `cleanupCompletedWorkerFixture` removes the whole directory. A
-// later file in the same worker would otherwise launch a `codex` that no longer exists.
+// Why re-materialized rather than written once at import: the module is shared across spec files in
+// a worker, and the temp directory can be gone by the time a later spec runs (an OS temp sweep, or a
+// stray teardown). A later file in the same worker would otherwise launch a `codex` that no longer
+// exists. Only the process-exit handler below may remove the directory.📝 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.
| // Why re-materialized rather than written once at import: the module is shared across spec files in | |
| // a worker, and the first file's `cleanupCompletedWorkerFixture` removes the whole directory. A | |
| // later file in the same worker would otherwise launch a `codex` that no longer exists. | |
| // Why re-materialized rather than written once at import: the module is shared across spec files in | |
| // a worker, and the temp directory can be gone by the time a later spec runs (an OS temp sweep, or a | |
| // stray teardown). A later file in the same worker would otherwise launch a `codex` that no longer | |
| // exists. Only the process-exit handler below may remove the directory. |
🧰 Tools
🪛 ast-grep (0.45.2)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFileSync } from 'node:child_process'
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
|
|
||
| await session.close(firstApp) | ||
| firstApp = null | ||
| expect(readPersistedWorkerResumeFence(session.userDataDir, workerPaneKey)).toBe(true) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
# Description: Determine whether the restart helper's close() awaits the profile session flush.
set -euo pipefail
fd -t f 'orca-restart' tests/e2e/helpers | while IFS= read -r file; do
echo "===== $file ====="
cat -n "$file"
done
# Show every other e2e read of the persisted fence, polled or sampled.
rg -n -C4 'readPersistedWorkerResumeFence' testsRepository: stablyai/orca
Length of output: 18868
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -t f . tests/e2e/helpers | rg 'electron-process-shutdown|completed-worker-retirement-fixture'
for file in $(fd -t f . tests/e2e/helpers | rg 'electron-process-shutdown|completed-worker-retirement-fixture'); do
echo "===== $file ====="
ast-grep outline "$file" || true
done
echo "===== shutdown helper implementation ====="
file=$(fd -t f . tests/e2e/helpers | rg 'electron-process-shutdown' | head -n1)
cat -n "$file"
echo "===== persisted fence reader ====="
file=$(fd -t f . tests/e2e/helpers | rg 'completed-worker-retirement-fixture' | head -n1)
sed -n '190,225p' "$file"Repository: stablyai/orca
Length of output: 12939
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "===== closeElectronAppForE2E references ====="
rg -n -C5 'function closeElectronAppForE2E|const closeElectronAppForE2E|export .*closeElectronAppForE2E|closeElectronAppForE2E' tests src
echo "===== Electron close contract references ====="
rg -n -C4 '\.close\(\)|app\.close|beforeunload|session.*flush|persist.*session|orca-data\.json' tests/e2e src | head -n 300Repository: stablyai/orca
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "===== fence/session persistence definitions ====="
rg -n -C6 'legacyWorkerResumeFencesByPaneKey|resumeFence|beforeunload|workspaceSession' src tests/e2e \
-g '*.ts' -g '*.tsx' | head -n 400
echo "===== app quit and session persistence wiring ====="
rg -n -C8 'app\.on\(['"'"'"]before-quit|window-all-closed|will-quit|beforeunload|persist.*session|session.*persist|writeFile.*orca-data|orca-data\.json' src \
-g '*.ts' -g '*.tsx' | head -n 500Repository: stablyai/orca
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "===== targeted fence/session persistence matches ====="
rg -n -C5 'legacyWorkerResumeFencesByPaneKey|resumeFence' src tests/e2e -g '*.ts' -g '*.tsx'
echo "===== targeted quit/persistence matches ====="
rg -n -C6 'beforeunload|before-quit|will-quit|persist.*session|session.*persist|orca-data\.json' src -g '*.ts' -g '*.tsx'Repository: stablyai/orca
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "===== all declarations related to persisted worker fences ====="
rg -n -C3 'legacyWorkerResumeFencesByPaneKey|set.*ResumeFence|resume.*Fence|worker.*Fence' src -g '*.ts' -g '*.tsx' | head -n 500
echo "===== renderer-to-main persistence/quit hooks ====="
rg -n -C5 'beforeunload|sendBeacon|ipcRenderer|contextBridge|app\.quit|app\.exit|BrowserWindow.*close|close.*window|persist' src/main src/renderer -g '*.ts' -g '*.tsx' 2>/dev/null | head -n 500Repository: stablyai/orca
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "===== renderer shutdown persistence ====="
sed -n '90,220p' src/renderer/src/app-shell/use-app-session-persistence.ts
echo "===== shutdown checkpoint implementation ====="
cat -n src/renderer/src/app-shell/shutdown-checkpoint-persist.ts
cat -n src/renderer/src/app-shell/shutdown-checkpoint-guard.ts | sed -n '1,260p'
echo "===== runtime fence write and store flush contract ====="
cat -n src/main/runtime/runtime-legacy-worker-terminal-recovery-persistence.ts | sed -n '1,125p'
rg -n -C8 'setWorkspaceSession|flushPendingOrThrowAsync|waitForPendingWrite|stageWorkspaceSessionBeforeUnload' src/main/persistence src/main -g '*.ts' | head -n 350Repository: stablyai/orca
Length of output: 12541
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "===== stageBeforeUnloadSync bindings ====="
rg -n -C10 'stageBeforeUnloadSync' src -g '*.ts' -g '*.tsx'
echo "===== shutdown handler registration ====="
sed -n '220,330p' src/renderer/src/app-shell/use-app-session-persistence.ts
rg -n -C8 'createShutdownCheckpointBeforeUnloadHandler|onWindowCloseRequested|beforeunload' src/renderer/src/app-shell src/main -g '*.ts' -g '*.tsx' | head -n 300
echo "===== runtime session write scheduling ====="
rg -n -C10 'setWorkspaceSession\s*=|setWorkspaceSession\(|flushPendingOrThrowAsync|waitForPendingWrite|writeWorkspaceSession|stageWorkspaceSessionBeforeUnload' src/main/persistence src/main/runtime -g '*.ts' | head -n 400Repository: stablyai/orca
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "===== preload app bridge ====="
file=$(rg -l 'stageBeforeUnloadSync' src/preload src/main src/renderer/src/web -g '*.ts' | head -n 1)
echo "$file"
cat -n "$file"
echo "===== main IPC registration ====="
rg -l 'stageBeforeUnloadSync|stage-before-unload|beforeUnloadSync' src/main src/preload -g '*.ts' | while IFS= read -r file; do
echo "===== $file ====="
rg -n -C12 'stageBeforeUnloadSync|stage-before-unload|beforeUnloadSync' "$file"
done
echo "===== Store synchronous staging implementation ====="
rg -l 'stageWorkspaceSessionBeforeUnload|stageBeforeUnload' src/main -g '*.ts' | while IFS= read -r file; do
echo "===== $file ====="
rg -n -C15 'stageWorkspaceSessionBeforeUnload|stageBeforeUnload' "$file"
doneRepository: stablyai/orca
Length of output: 36642
Poll the persisted fence read instead of sampling it once.
session.close waits for app close and process exit, but the shutdown handler only starts the asynchronous persistence flush. It does not await app:await-before-unload-checkpoint. The single read can therefore observe stale disk state.
♻️ Proposed change
- expect(readPersistedWorkerResumeFence(session.userDataDir, workerPaneKey)).toBe(true)
+ await expect
+ .poll(() => readPersistedWorkerResumeFence(session.userDataDir, workerPaneKey), {
+ message: 'the runtime-authored fence never reached disk'
+ })
+ .toBe(true)📝 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.
| expect(readPersistedWorkerResumeFence(session.userDataDir, workerPaneKey)).toBe(true) | |
| await expect | |
| .poll(() => readPersistedWorkerResumeFence(session.userDataDir, workerPaneKey), { | |
| message: 'the runtime-authored fence never reached disk' | |
| }) | |
| .toBe(true) |
| @@ -0,0 +1,10 @@ | |||
| `pr-base-1d1b73c40850-pty-transport.txt` is the exact | |||
| `src/renderer/src/components/terminal-pane/remote-runtime-pty-transport.ts` from PR | |||
| #19358's base commit `1d1b73c40850`, excluding only its leading max-lines lint | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Reflow Line 3 so the issue reference does not start the line.
#19358 begins Line 3, so markdownlint parses it as a malformed ATX heading and reports MD018. Move the reference off the line start to clear the rule without changing the text.
🔧 Proposed fix
`pr-base-1d1b73c40850-pty-transport.txt` is the exact
`src/renderer/src/components/terminal-pane/remote-runtime-pty-transport.ts` from PR
-#19358's base commit `1d1b73c40850`, excluding only its leading max-lines lint
-suppression comment. Keep this historical reader frozen.
+#19358's base commit `1d1b73c40850`, excluding only its leading
+max-lines lint suppression comment. Keep this historical reader frozen.The reference still needs to leave the line start. Use this form instead:
`pr-base-1d1b73c40850-pty-transport.txt` is the exact
`src/renderer/src/components/terminal-pane/remote-runtime-pty-transport.ts` from
PR `#19358`'s base commit `1d1b73c40850`, excluding only its leading max-lines
lint suppression comment. Keep this historical reader frozen.🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 3-3: No space after hash on atx style heading
(MD018, no-missing-space-atx)
Source: Linters/SAST tools
… module A dynamic store import inside the refresh was the workaround for the store -> hydration -> refresh -> store cycle. The counter itself has no dependencies, so it lives alone and both writers import it statically.
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)
src/main/ipc/pty/runtime/spawn-execute.ts (1)
217-220: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClear the provisional execution context on
StablePaneResumeBlockedError.
cancelPendingPtyRegistrationonly removes registration state. It does not clear the execution context prepared forctx.effectiveSessionAppId. This return bypasses the guarded cleanup that callspreparePtyExecutionContext(..., null, { resetIncarnation: true }), so WSL and headless-terminal state can remain after refusal. Run the same guarded cleanup before returning. Extend the fresh-pane fence test to assert this cleanup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Team
Run ID: e5dec47a-6c4c-4cbb-af02-a8e18970a5e8
📒 Files selected for processing (23)
src/main/ipc/pty-attach-only-fenced-pane-spawn.test.tssrc/main/ipc/pty-resume-claim-ensure-fence.test.tssrc/main/ipc/pty-ssh-undelivered-kill.test.tssrc/main/ipc/pty/ipc/renderer-kill.tssrc/main/ipc/pty/ipc/spawn-begin.tssrc/main/ipc/pty/ipc/spawn-execute.tssrc/main/ipc/pty/pane/hibernation-admission.tssrc/main/ipc/pty/pane/stable-owner.tssrc/main/ipc/pty/pane/stable-pane-resume-fence-host.test.tssrc/main/ipc/pty/pane/stable-pane-resume-fence.tssrc/main/ipc/pty/runtime/kill.tssrc/main/ipc/pty/runtime/spawn-execute.tssrc/main/ipc/pty/runtime/spawn.tssrc/main/persistence/loading-store/store-runtime-authored-session-fences.test.tssrc/main/persistence/runtime-authored-workspace-session-fields.tssrc/main/runtime/mobile-session-terminal-persistence-retirement.tssrc/renderer/src/components/terminal-pane/pty-connection/reattach-result-handler.tssrc/renderer/src/lib/automatic-resume-spawn-settlement.test.tssrc/renderer/src/lib/automatic-resume-spawn-settlement.tssrc/renderer/src/lib/legacy-worker-resume-fence-generation.tssrc/renderer/src/lib/legacy-worker-resume-fence-refresh.test.tssrc/renderer/src/lib/legacy-worker-resume-fence-refresh.tssrc/renderer/src/store/terminals/workspace-terminal-hydration.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.
There was a problem hiding this comment.
ℹ️ No critical issues — minor suggestions inline.
Reviewed changes — the round-1 follow-ups that move main's fence decisions onto the layer that owns each identity.
- Consolidated resume admission —
isFreshPaneResumeBlocked(source or destination) now runs at the provider.spawn site (spawnForStablePane) and in theagentSessionEnsureclaim callback, replacing the four redundant entry checks;stablePaneOwnerCandidateis now resolved for claims too. - Retained fenced identity at the write boundary —
preserveRuntimeAuthoredWorkspaceSessionFieldsre-adds a fenced pane's sleeping record when a renderer write drops it, so duplicate cleanup can no longer delete the identity main uses to map a resume source;retireTerminalSurfaceFromPersistencedrops the fence entry so the record can be removed afterward. - Hardened hibernation admission —
assertPtyHibernationAllowediterates fenced pane keys directly and refuses when either the live runtime owner or the persisted owner is the target PTY, so a stale persisted binding can no longer hide the live owner. - Kept user-owned tabs on refusal —
handleReattachResultclears only the resume claim (keeping the tab) when input was already accepted or preconnect input exists, instead of closing the tab. - Generation-based reply supersession — the renderer fence refresh drops replies older than a later request or hydration via an import-free generation counter, so a stale positive fence reply cannot overwrite a newer hydration.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — the round-2 follow-up that removes the renderer's fence veto and preserves resume ownership.
- Switched the resume veto to the durable record flag —
resumeSleepingAgentSessionsForWorktreenow skips a record onautomaticResumeBlockedBy === 'legacy-orchestration-worker'(hydrated atomically with the record) instead of the advisorylegacyWorkerResumeFencesByPaneKeyset, so the renderer no longer depends on a racy fence mirror. - Reverted the generation counter —
legacy-worker-resume-fence-generation.tsis deleted andrefreshLegacyWorkerResumeFencesno longer drops stale replies; with the fence set now purely advisory, a stale reply can only cause reversible misses, and main remains the authority on launch/kill. - Preserved resume ownership —
settleAutomaticResumeSpawnwriteslaunchAgent+resumeProviderSessiononto the admitted tab (new optional persisted field onTerminalTab, schema.catch(undefined)), andactiveOrQueuedResumeClaimsProviderSessionmatches tab/pending-startup/claim ownership so the sweep dedupes correctly after the claim clears. - Hardened hibernation admission —
assertPtyHibernationAllowedresolves the live owner viaruntime.resolveTerminalPane(paneKey)and throwsterminal_pane_owner_host_mismatchon a host mismatch, orStablePaneResumeBlockedErrorfor a fenced owner, falling back to the persisted owner.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
|
Superseded: the settled-worker resume fence is being deleted as a product decision (finished workers resume exactly like any other agent terminal). Replacement branch: settled-worker-policy. |
1 participant
Summary
Replaces #19344 and #19358 as a single PR. Both branches are merged in unchanged (their heads
2197d23bbaand34a70e8f25are ancestors of this branch), then one commit moves the last two fence decisions into main and deletes the renderer's fence-consistency lane.Invariant. A worker fenced by orchestration (
legacy-orchestration-worker) is never relaunched and never killed by automation. Owner. Main, which persists the fence. After this PR the renderer's copy of the fence set is advisory: a stale copy can no longer launch or kill anything.Why a unification pass
Five review rounds across #19344 and #19358 each found a race in the renderer's mirror of the fence (pending read vs hydration vs scoped snapshot vs reply supersession). The mirror had to be race-free because two destructive renderer decisions trusted it and main did not check them:
pty:killwithkeepHistory; main's kill path never consulted the fence.What changed (commit 2120f09)
isSleepingAgentResumeBlockedmatches the spawn'sresumeProviderSessionagainst the host partition's sleeping records and refuses (structuredreattachUnverifiable) when the source pane is fenced. Applied at IPCpty:spawn(begin + execute), the runtime controller (entry + claim path, rechecked after async owner reconciliation), and therefore pairedterminal.create/createAgentSession. No new wire field: the existing resume metadata identifies the record.assertPtyHibernationAllowedresolves the persisted pane for akeepHistorykill and throws the existingagent_hibernation_automatic_resume_blockederror for a fenced pane. Non-reversible (user close) kills are unchanged.automatic-resume-spawn-settlement.ts).runtime-session-application.ts(pending-read lane, supersession, consumed-object tracking), refresh serialization, renderer-side legacy-fence normalization for authority, and three ordering-only test files. Hydration now sets the fence in the same store transition as the records and otherwise last-write-wins.isPaneAutomaticResumeBlockednow reads the canonical set (readWorkspaceSessionResumeFences) so recordless canonical fences and explicit retirement are honoured in main.TERMINAL_FENCED_CREATE_RUNTIME_CAPABILITY's client-sidestatus.getgate: it protects new client + old host, the direction host-side negotiation cannot cover.Net this commit: +207 / -281 non-test source lines. Full report with the wire decision, capability reasoning, and probe census:
~/orca/review-archive/followups/fence-unify-report.md(local archive).Compatibility
Verification (lead)
Round-1 review follow-ups (dc6390b, 69ea26a)
Codex round 1 found five real defects: main decided on identity it did not own. Fixes, each at the owning layer:
isFreshPaneResumeBlocked, source or destination) now runs at the provider.spawn site and in the claim callback; the four redundant entry checks are deleted.Verification: round-1 probes pass at this head; ten lead mutations plus the worker's 28 all killed; six-directory gate 24,776 tests; both typechecks; changed-code quality; reliability manifest 111 gates; e2e 5/5 both orders on a fresh build. Net non-test source delta this round: 16 files, +125/-117.