Skip to content

fix(terminal): let main own a fenced worker's PTY session - #19358

Closed
Jinwoo-H wants to merge 5 commits into
mainfrom
fix/settled-worker-tab-root
Closed

Jinwoo-H wants to merge 5 commits into
mainfrom
fix/settled-worker-tab-root

Conversation

@Jinwoo-H

@Jinwoo-H Jinwoo-H commented Sep 7, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 22 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​4859 $\color{#cf222e}{\Huge{\mathbf{−}}}$​90 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​4769
Prod 40 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​731 $\color{#cf222e}{\Huge{\mathbf{−}}}$​274 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​457

Summary

Fixes a settled orchestration worker tab disappearing on reveal after restart. Main now owns the fence and the attach decision, preserving the historical tab and binding when the worker exited or its host cannot verify attachment.

Fix

  • Delete the renderer-only retained-worker attach and renderer-declared attachOnly flag end to end. Every stable-pane attach uses main's ordinary provider ownership path.
  • Main reads automaticResumeBlockedBy from the host-scoped persisted pane record, including early adoption and runtime terminal.create. A fenced pane can attach but cannot spawn or resume; missing binding stays unverifiable.
  • Decide refusal before retirement: exactly one provider attach attempt returns structured observed-exit or unverifiable evidence, without clearing tabs/layouts/incarnations or calling the provider again. IPC/runtime callers stop before registration/commit and clean provisional state.
  • Preserve SSH proven-exit versus unmarked absence through structured results; the renderer never derives exitedBeforeAttach from error text. Keep the shared local session-not-found prefix and fence-aware sleeping-session choice.
  • Generalize failed-reattach recovery across hosts. Binding retirement requires a replacement in the same transition, sharing actual fresh-spawn eligibility; ordinary local fallback remains intact, and explicit unverifiable results retain binding.
  • Paired clients resolve/adopt encoded handles and preserve unsupported/unavailable outcomes. Only the host's fenced create boundary can produce the structured exit refusal. Existing bounded retry and stale-handle error routing remain active.

Round-1 review follow-ups

Main owns the invariant at its spawn/adopt boundary rather than accepting renderer policy or repairing persistence afterward. Rejected alternatives: cache-miss liveness probes, client attach-only heuristics, SSH-only binding preservation, and string-based death certificates.

Reviewer probes: paired 2/2 unchanged; main 3/3, local 10/10 and SSH 3/3 after documented contract adaptations. Main fixtures now seed the durable fence and assert a structured resolution instead of rejection; local assertions stop requiring the deleted flag; the SSH marked row supplies structured evidence because its original marked/unmarked rows used identical rejected messages but expected opposite answers. Persistence and binding-preservation properties remain asserted. Actual SSH error classes are covered at the owner boundary.

24 independently killed mutation behaviors cover fence lookup, attach-only enforcement, refusal propagation and cleanup, evidence discrimination, replacement eligibility, fence-aware choice, paired outcomes, and retained retry-error routing. Full renderer testing caught and fixed swallowed recovery errors; the final suite passes.

Validation

  • Main/daemon/runtime/provider integration: 80 suites, 719 passed, 6 skipped.
  • Full terminal renderer: 445 suites, 4,244 passed, 1 skipped.
  • tc:node, tc:web, changed-code quality, formatting, max-lines ratchet and reliability manifest checks pass.
  • Hidden Electron restart has live-session and daemon-session-gone cases: exact tab and binding survive reveal and another persistence flush; no extra agent launch. Main's actual spawn reply is observed for exited evidence. Both windows remain hidden/unfocused.
  • Restart spec alone and paired with completed-worker retirement in both actual execution orders pass; restart-first uses a throwaway path-sorted copy removed afterward. Fixture process-exit cleanup is retained.

Compatibility and remaining coverage

Reliability gate: terminal-session.settled-worker-main-owned-attach (experimental). One host-scoped fence lookup and one provider attach attempt; no new polling or subprocess mechanism. Existing recovery scheduler remains bounded.

Wire additions are optional JSON result fields, with no stream opcode. New client/old host unsupported-resolution behavior is unit-tested and retains binding; encoded handles still subscribe. Old clients ignore new fields. Real paired two-process, live SSH, Linux, Windows and WSL rendering remain untested; macOS hidden Electron covers daemon restart, and unit contracts cover SSH/paired evidence. Folder and git workspaces share the pane/workspace identity lookup.

Pre-v31 daemon attach-only emulation is unchanged and may briefly create/kill a bare shell; it does not silently accept a replacement. The exited screenshot shows a retained tab with an empty viewport; transcript retention is not claimed.

Part of the fix-forward set for #16904 with #19337, #19339, #19344, #19347. The lead transferred the round-2 commit to the PR branch before the round-2 review.

Reviewed round-2 base: c742825444a9077d011edddf0fee5565f0be580b.

Round-2 review follow-ups

Main now rereads persisted host policy after provider attachment and immediately before fresh fallback/claim spawn. A fence committed during attachment or claim reconciliation returns unverifiable without retiring the tab or spawning a replacement. Stable owner snapshots contain identity only.

Paired restored panes require the host's advertised terminal.fenced-create.v1 capability before owner-absent resolution can proceed to create. Older resolve-capable hosts stay unverifiable; existing unsupported-resolution handle subscription remains available. This reuses status.get and the existing host/client capability lists.

Permanent boundary tests close all six review mutation survivors: distinct SSH/local policy partitions, IPC/runtime refusal ordering, runtime unverifiable projection, pre-adopted refusal, and both actual kill-error paths. All 32 reviewer mutations are killed, plus new late-claim and capability-negotiation mutations. The three round-2 probes pass unmodified; all 18 archived round-1 adapted/paired cases pass.

Validation: 36 touched suites / 289 tests and 21 paired transport suites / 161 tests pass. Node/web typechecks, changed-code quality, direct lint, max-lines, reliability manifest and no-stash precommit checks pass. A fresh build passes both hidden Electron specs individually and together in both actual orders (2 + 2 + 4 + 4 cases). Screenshots confirm retained live-output and exited worker tabs. Live SSH, two-binary paired rendering, Windows/Linux/WSL rendering and exited transcript retention remain unverified.

The lead verified and transferred round-3 commit 2315bc2a394fb3b8f985d9b70784d2d3bf659963 before the round-3 review.

Local round-3 commit: 2315bc2a394fb3b8f985d9b70784d2d3bf659963.

Round-3 review follow-ups

Host RPCs now negotiate refusal representation using the authenticated client's existing terminal.fenced-create.v1 capability. terminal.create, terminal.createAgentSession, and terminal.ensureAgentSession return structured refusal only to supporting clients. Older clients receive the existing recoverable remote_runtime_unavailable error, so they cannot publish a fake spawn or subscribe to an unregistered handle. Runtime refusal and ordinary create success are unchanged; negotiation runs outside deduplication.

Checked in both reviewer scenarios: the frozen PR-base reader composed with the real create handler/error mapper, and the final fence recheck after non-null owner adoption retires while unfenced and returns null. The narrow !args.owner && mutation is killed; removing RPC negotiation reproduces the fake spawn. No renderer policy, liveness probe, compensation, new opcode, or source line-cap exemption.

The archived old-client oracle's structured-return assertion is impossible for its pre-PR reader, whose catch returns undefined. Its no-fake-spawn assertion now passes unmodified; the permanent test expects undefined plus conservative recovery and no spawn/subscription/exit/error callbacks. The current-client control now advertises the capability current clients send in production. These two fixture/assertion corrections are documented in the Round 4 report, with the original failure logs retained.

Round-1 adapted probes: 18 passed. Round-2 reproductions: 3 passed. Round-3 capability/order probes: 5 passed. Node/web typechecks, changed-code quality, max-lines, reliability manifest, and no-stash precommit checks pass. Full suite and hidden Electron matrix results are recorded below.

Historical-client coverage is source-level composition with current transitive dependencies, not two live historical binaries. Live SSH and Windows/Linux/WSL rendered validation remain unverified; exited transcript retention is not claimed. Round-4 work is local for the lead to transfer; this worker did not push or merge.

Final round-4 validation: 717 suites passed, 6,570 tests passed / 2 skipped. Fresh hidden Electron matrix passes 2 + 2 + 4 + 4 cases; both actual paired orders verified and temporary spec removed.

Local round-4 commit: 34a70e8f255237593c9636e419bbe3e5cc18e526 (not pushed by this worker).

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.
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.
@coderabbitai

coderabbitai Bot commented Sep 7, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The change adds shared session-absence detection and explicit PTY attach outcome flags. Main-process resume fences preserve stable pane ownership and stop unverified replacements. Renderer reattachment uses standard transport flows and retains bindings after unverifiable results. Terminal creation returns attach refusals before registration or publication. Tests cover daemon ownership, fenced spawning, renderer recovery, and worker-tab survival across restart with live or missing daemon sessions.

Merge Risk: 🟡 Moderate · up to 34a70

The terminal attach-refusal flow can still leave terminals stuck connecting, use stale ownership, or retain provisional state after a refusal; the fixture documentation also needs a small lint correction. These issues should be addressed before merging.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 10.20% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 49 functions across 59 files. (1 skipped:… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the primary change: main now owns the PTY session for a fenced worker.
Description check ✅ Passed The description is detailed and relevant. It explains the problem, implementation, compatibility behavior, validation results, and known coverage gaps. It does not reproduce every template heading, an…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

Docstring coverage is 10.20% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 49 functions across 59 files. (1 skipped: 1 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Team

Run ID: 342ceb71-ff10-446a-a90b-d2204c2ede00

📥 Commits

Reviewing files that changed from the base of the PR and between d0506bf and 50b9327.

📒 Files selected for processing (21)
  • src/main/daemon/daemon-errors.ts
  • src/main/daemon/daemon-pty-adapter-attach-only-ownership.test.ts
  • src/main/ipc/pty-attach-only-fenced-pane-spawn.test.ts
  • src/main/ipc/pty/ipc/spawn-options.ts
  • src/main/ipc/pty/ipc/spawn-types.ts
  • src/main/ipc/pty/provider/liveness.ts
  • src/preload/api/pty-api.ts
  • src/renderer/src/components/terminal-pane/ipc-pty-connect.ts
  • src/renderer/src/components/terminal-pane/ipc-pty-spawn-request.ts
  • src/renderer/src/components/terminal-pane/pty-connection-agent-session-resume.test.ts
  • src/renderer/src/components/terminal-pane/pty-connection/deferred-session-attach.ts
  • src/renderer/src/components/terminal-pane/pty-connection/deferred-session-reattach-choice.ts
  • src/renderer/src/components/terminal-pane/pty-connection/deferred-session-reattach-connect.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/components/terminal-pane/pty-transport-types.ts
  • src/renderer/src/components/terminal-pane/remote-runtime-pty-transport.ts
  • src/shared/pty-attach-absence-evidence.ts
  • tests/e2e/completed-worker-retirement-resume.spec.ts
  • tests/e2e/helpers/completed-worker-retirement-fixture.ts
  • tests/e2e/settled-worker-tab-survives-restart.spec.ts
💤 Files with no reviewable changes (3)
  • src/renderer/src/components/terminal-pane/pty-connection/run-deferred-connect.ts
  • tests/e2e/completed-worker-retirement-resume.spec.ts
  • src/renderer/src/components/terminal-pane/pty-connection/retained-legacy-pty-attach.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review.

Comment thread src/renderer/src/components/terminal-pane/remote-runtime-pty-transport.ts Outdated

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

The local-daemon fenced-worker fix is sound and well-tested, but the paired web/mobile remote-runtime-pty-transport change reports a proven-absent verdict without contacting the host — see the inline comment.

Reviewed changes

  • Delete the renderer-only legacy attach path — removes retained-legacy-pty-attach.ts, the legacyAttachOnlyPtyId short-circuit, and both SSH fence special-cases, so fenced panes now take the standard startDeferredSessionReattach / deferred-SSH flow with attachOnly: true.
  • Plumb attachOnly renderer → preload → main → PtySpawnOptions — main honours it only alongside a caller-supplied non-minted session id, so a bare flag can never refuse a fresh tab.
  • Proven-absence contract — SESSION_NOT_FOUND_MESSAGE_PREFIX in src/shared/pty-attach-absence-evidence.ts consolidates three drifting copies of the /Session not found/ matcher, and attachOnly connect errors return { id, exitedBeforeAttach: true } (terminal) rather than sessionExpired.
  • Reattach choice — a fenced record no longer steers the pane into the cold-restore path (hasResumableSleepingAgentSession).
  • Tests + e2e — unit suites for the IPC/daemon plumbing, a real-daemon adapter ownership test, and an A/B e2e settled-worker-tab-survives-restart.spec.ts; the second commit fixes a shared fake-codex fixture teardown bug.

ℹ️ Nitpicks

  • src/main/ssh/ssh-pending-pty-kill-replay.ts (and its test at ssh-pending-pty-kill-replay.test.ts:207) still reference the old matcher as /Session not found/i; isPtyAlreadyGoneError now routes through isSessionNotFoundRefusal (case-sensitive, requires the : suffix). Worth a one-line comment refresh, though the surrounding reasoning ("matches on message text, not the class") remains accurate.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using DeepSeek Pro (free via Pullfrog for OSS) | 𝕏

Comment thread src/renderer/src/components/terminal-pane/remote-runtime-pty-transport.ts Outdated
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.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes (incremental delta against the prior pullfrog review at 50b9327)

  • Moved the fence into main — deleted the renderer-declared attachOnly flag end to end (spawn-options.ts, spawn-types.ts, ipc-pty-spawn-request.ts) and the retained-legacy-pty-attach.ts helper; main now reads the persisted automaticResumeBlockedBy: 'legacy-orchestration-worker' record via isStablePaneResumeBlocked.
  • Refused ownerless fenced panes at the spawn boundary — spawnForStablePane, spawn-begin.ts, and runtime/spawn.ts short-circuit a fenced pane with no resolvable binding to reattachUnverifiable without calling the provider, so a fenced pane can never fresh-spawn or resume.
  • Structured, typed attach evidence — PtySpawnResult and RuntimeTerminalCreate now carry exitedBeforeAttach / reattachUnverifiable, produced only in attachStablePaneOwner's fenced catch via isObservedPtyExitEvidence (SshPtyProvenExitedOnRelayError | SessionNotFoundError); raw message text never certifies exit.
  • Generalized recovery across hosts — direct-ssh-reattach-recovery became unverifiable-reattach-recovery with mayRetireBindingAfterFailedReattach (local, unfenced, not deleting), and reattach-result-handler.ts routes unverifiable / no-pty-id outcomes to bonded recovery rather than retirement.
  • Corrected the paired client — resolvePersistedHostPane is now a discriminated union and exitedBeforeAttach / reattachUnverifiable surface only from the host's structured terminal.create response, resolving the prior round-1 concern about fabricating a proven-absent verdict.
  • Broadened e2e + gate — the restart spec now observes main's real pty:spawn reply for exited evidence and asserts the tab/binding survive across another persistence flush; a terminal-session.settled-worker-main-owned-attach reliability gate was added.

Pullfrog  | View workflow run | Using DeepSeek Pro (free via Pullfrog for OSS) | 𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes (incremental delta against the prior pullfrog review at c742825)

  • Revalidated the fence at the attach boundary — attachStablePaneOwner now snapshots blockedAtAttach before the provider call and re-reads persisted policy in the catch, so a fence committed mid-attach downgrades to reattachUnverifiable instead of certifying exitedBeforeAttach; spawnForStablePane re-checks the fence after a retired attach before any fresh spawn.
  • Guarded claim reconciliation against a late fence — executeRuntimePtySpawn re-reads isStablePaneResumeBlocked around agentSessionOwners.ensure and throws StablePaneResumeBlockedError from the spawn callback, projecting reattachUnverifiable with no registration, commit, or retirement.
  • Negotiated host fenced-create semantics on the paired client — a new terminal.fenced-create.v1 capability, advertised in both RUNTIME_CAPABILITIES and remoteRuntimeClientCapabilities(), gates the owner-absent → terminal.create fallback; older resolve-capable hosts stay reattachUnverifiable rather than minting a replacement.
  • Narrowed the owner snapshot — StablePaneOwner drops automaticResumeBlocked; the fence is always read from host-scoped persisted policy at the decision point.
  • Broadened mutation coverage — new cases for fence-committed-during-attach/claim, runtime-controller unverifiable projection, SSH-vs-local host policy partition, and old-host refusal, each asserting terminal.create is never reached when it must not be.

The round-3 delta closes the two race windows the round-2 review left open (fence committing during attach and during claim reconciliation) and adds the capability negotiation needed to keep an older paired host from silently fresh-spawning a replacement for a fenced pane. The structured-exit downgrade (blockedAtAttach && isObservedPtyExitEvidence) is conservative and correct, and the new capability is a wire-safe optional field with no new stream opcode.

Pullfrog  | View workflow run | Using DeepSeek Pro (free via Pullfrog for OSS) | 𝕏

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
src/renderer/src/components/terminal-pane/remote-runtime-pty-transport.ts (2)

2210-2215: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Require host-side identity evidence before adopting a legacy persisted handle.

getRemoteRuntimeTerminalHandle extracts only the handle and environment ID. When terminal.resolvePane is unavailable, this branch passes that handle directly to adoptResolvedHostPane. The runtime’s recovery contract treats duplicate handles as ambiguous and defers them without exact PTY identity evidence. An environment match therefore cannot identify the persisted PTY. Require matching pane/process identity before adoption; otherwise return reattachUnverifiable.


2554-2559: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Finalize the unresolved attach state before returning.

When resolvePersistedHostPane() returns no terminal, attach() leaves connecting = true, connected = false, and handle = persistedHandle. Recovery is idle, attachment waiters remain unsettled, and retryRecovery() cannot revive this attach. Set the state to unavailable before returning:

Proposed state cleanup
         if (!resolved) {
+          connecting = false
+          setAttachmentUnavailable()
+          emitRecoveryState()
           surfaceErrorMessage(
             resolution.kind === 'owner-absent'
               ? 'Remote terminal was closed.'

owner-absent and the legacy-runtime environment mismatch are not retryable results, so do not schedule a retry in this branch.

src/main/ipc/pty/runtime/spawn-execute.ts (1)

173-175: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Reset the provisional PTY execution context on fenced exits.

The outer flow clears the pending registration and restores the provisional PTY size, but it does not clear the context created by preparePtyExecutionContext. Reset that context for the provisional PTY with resetIncarnation: true before both refusal returns.


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Team

Run ID: 7e8c4636-9e8e-4880-af15-653027030470

📥 Commits

Reviewing files that changed from the base of the PR and between c742825 and 2315bc2.

📒 Files selected for processing (13)
  • config/reliability-gates.jsonc
  • src/main/ipc/pty-attach-only-fenced-pane-spawn.test.ts
  • src/main/ipc/pty-runtime-kill-and-exit.test.ts
  • src/main/ipc/pty/pane/stable-owner.ts
  • src/main/ipc/pty/pane/stable-pane-fenced-evidence.test.ts
  • src/main/ipc/pty/pane/stable-pane-resume-fence-host.test.ts
  • src/main/ipc/pty/pane/stable-pane-resume-fence.ts
  • src/main/ipc/pty/runtime/spawn-execute.ts
  • src/renderer/src/components/terminal-pane/remote-runtime-pty-transport-fenced-outcome.test.ts
  • src/renderer/src/components/terminal-pane/remote-runtime-pty-transport.ts
  • src/shared/protocol-version.ts
  • src/shared/remote-runtime-client-capabilities.ts
  • src/shared/terminal-fenced-create-capability.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/main/ipc/pty/pane/stable-pane-resume-fence.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 4 remain after this review.

Comment on lines +207 to +214
if (isStablePaneResumeBlocked(args.store, paneKey, args.worktreeId, args.connectionId)) {
return {
owner,
result: {
id: owner.ptyId,
...(blockedAtAttach && isObservedPtyExitEvidence(error)
? { exitedBeforeAttach: true as const }
: { reattachUnverifiable: true as const })

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Recheck the owner before returning the fenced result.

attachStablePaneOwner awaits provider.spawn, but the fenced catch returns the captured owner without running the existing args.resolveOwner?.() comparison. If another operation changes the binding during the await, this path returns the old ptyId and incarnation with reattachUnverifiable. Apply the same owner-change validation before this return and throw terminal_pane_owner_changed when the binding changed.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Team

Run ID: 486f3bdf-2557-42fb-b60d-afc59db5d4ed

📥 Commits

Reviewing files that changed from the base of the PR and between 2315bc2 and 34a70e8.

📒 Files selected for processing (9)
  • src/main/ipc/pty/pane/stable-pane-final-fence-recheck.test.ts
  • src/main/runtime/rpc/methods/agent-session.ts
  • src/main/runtime/rpc/methods/terminal/terminal-lifecycle-methods.ts
  • src/main/runtime/rpc/terminal-create-refusal-negotiation.test.ts
  • src/main/runtime/rpc/terminal-create-refusal-negotiation.ts
  • src/renderer/src/components/terminal-pane/pr-base-pty-transport-test-loader.ts
  • src/renderer/src/components/terminal-pane/remote-runtime-pr-base-refusal-compatibility.test.ts
  • tests/fixtures/terminal/README.md
  • tests/fixtures/terminal/pr-base-1d1b73c40850-pty-transport.txt

Included review availability: Your plan provides up to 10 included reviews per hour; 4 remain after this review.

@@ -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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Fix the Markdown lint warning.

Line 3 starts with #19358's, which triggers MD018. Use PR 19358 or escape the number sign.

Proposed fix
-#19358's base commit `1d1b73c40850`, excluding only its leading max-lines lint
+PR 19358's base commit `1d1b73c40850`, excluding only its leading max-lines lint
📝 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.

Suggested change
#19358's base commit `1d1b73c40850`, excluding only its leading max-lines lint
PR 19358's base commit `1d1b73c40850`, excluding only its leading max-lines lint
🧰 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

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes (incremental delta against the prior pullfrog review at 2315bc2)

  • Negotiated the refusal response for older clients — a new negotiateTerminalCreateRefusal helper converts a structured exitedBeforeAttach / reattachUnverifiable result into a recoverable remote_runtime_unavailable error whenever the authenticated caller lacks the terminal.fenced-create.v1 capability, so a pre-capability client cannot treat a fenced refusal as a successful create and publish a fake binding.
  • Wrapped all three create RPCs — terminal.create, terminal.createAgentSession, and terminal.ensureAgentSession now negotiate using the caller's authenticated clientCapabilities, closing every RuntimeTerminalCreate entry point; ordinary success passes through unchanged.
  • Added a frozen historical-client compatibility suite — a PR-base remote-runtime-pty-transport.ts fixture is compiled at test time by pr-base-pty-transport-test-loader.ts and composed with the current create handler, asserting an old client stays disconnected with no spawn, subscription, or error callbacks, while a current client receives the structured refusal.

The round-4 delta closes the last mixed-version gap from round 3: the client side already gated its terminal.create fallback behind the host capability, but the host could still hand an old client a structured refusal it would misread. Negotiating on the response side (using the same authenticated capability) makes the old-client path conservatively unverifiable with no fake spawn, and the throw lands after dedupeTerminalCreate has settled, so the idempotency in-flight entry is already dropped and nothing leaks.

Pullfrog  | View workflow run | Using DeepSeek Pro (free via Pullfrog for OSS) | 𝕏

@Jinwoo-H

Jinwoo-H commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant