Repository navigation
Reset stale terminal modes after dead TUI replay - #16379
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change adds shell ownership tracking for PTY sessions. Foreground confirmation now flows through daemon, provider, runtime, and session layers. Lifecycle scanners and recovery barriers handle clean and unclean TUI termination. Snapshots and checkpoints preserve or revoke ownership metadata. Renderer replay selects reset profiles using ownership and alternate-screen state. Tests cover persistence, protocol compatibility, replay behavior, process validation, and end-to-end child-TUI termination. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description provides detailed scope, rationale, behavior, testing, compatibility, risks, and evidence. However, it omits required template information, including a linked issue, visual proof or an explicit N/A explanation, the testing checklist, agent-skill boundary status, and the remaining checklist sections. Resolution Add a valid linked issue after “Fixes #”. Add visual proof or write exactly “N/A” with a reason if no visual or interaction change applies. Complete the Testing, Agent skill upstream boundary, Notes, and Checklist sections, including the required test and platform information. Full details: Docstring CoverageExplanation Docstring coverage is 11.90% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 42 functions across 59 files. (3 skipped: 1 unsupported, 2 too large.) ✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 66e23839-296a-4dfa-bd87-71715136425e
📒 Files selected for processing (5)
src/renderer/src/components/terminal-pane/pty-connection-hidden-codex-queries.test.tssrc/renderer/src/components/terminal-pane/pty-connection/hidden-output-restore-snapshot.tssrc/renderer/src/components/terminal-pane/terminal-replay-cursor-state.test.tssrc/shared/terminal-mode-reset-profiles.tstests/e2e/terminal-hidden-child-tui-kill-mode-reset.spec.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.
| const processName = await window.api.pty.getForegroundProcess(ptyId) | ||
| return processName?.toLowerCase() ?? null | ||
| }, tuiPtyId!), | ||
| { timeout: 8_000 } | ||
| ) | ||
| .toMatch(/^(bash|zsh|sh|fish)(\.exe)?$/) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- target test ---'
sed -n '1,130p' tests/e2e/terminal-hidden-child-tui-kill-mode-reset.spec.ts
printf '%s\n' '--- shell-related test configuration and predicates ---'
rg -n -i --glob '!node_modules' --glob '!dist' --glob '!build' \
'getForegroundProcess|powershell\.exe|pwsh\.exe|cmd\.exe|foreground.*shell|shell.*predicate|E2E.*shell|process\.env\.ComSpec|ComSpec|SHELL' \
tests src package.json playwright.config.* .github 2>/dev/null | head -n 250
printf '%s\n' '--- relevant file inventory ---'
git ls-files | rg '(^|/)(playwright|.*e2e.*config|.*shell.*|.*terminal.*spec).*'Repository: stablyai/orca
Length of output: 46543
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- E2E configuration ---'
sed -n '1,260p' tests/playwright.config.ts
sed -n '1,220p' src/main/e2e-config.ts
sed -n '1,180p' src/preload/e2e-config.ts
sed -n '1,180p' src/shared/e2e-config.ts
printf '%s\n' '--- Windows shell selection ---'
ast-grep outline src/main/ipc/pty-windows-shell-selection.test.ts --view expanded
ast-grep outline src/main/ipc/shell.ts --view expanded
ast-grep outline src/main/providers/windows-powershell-executable.ts --view expanded
ast-grep outline src/main/providers/windows-shell-fallback-chain.ts --view expanded
sed -n '1,260p' src/main/ipc/pty-windows-shell-selection.test.ts
sed -n '1,240p' src/main/ipc/shell.ts
sed -n '1,220p' src/main/providers/windows-powershell-executable.ts
sed -n '1,240p' src/main/providers/windows-shell-fallback-chain.ts
printf '%s\n' '--- foreground-process implementation and callers ---'
rg -n -C 6 'getForegroundProcess|foregroundProcess|foreground process' src tests/e2e \
--glob '*.{ts,tsx,js,jsx,mjs}' | head -n 300Repository: stablyai/orca
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- exact foreground-process handlers ---'
rg -l --glob '*.{ts,tsx}' 'getForegroundProcess|pty:getForegroundProcess' src tests/e2e | sort
rg -n -C 12 --glob '*.{ts,tsx}' 'pty:getForegroundProcess|getForegroundProcess\s*=' src/main src/preload src/shared | head -n 260
printf '%s\n' '--- terminal Windows shell defaults and settings ---'
rg -n -C 8 --glob '*.{ts,tsx}' \
'terminalWindowsShell|resolveWindowsPowerShellSpawnChain|getWindowsCmdPath|COMSPEC|ComSpec' \
src/main src/shared src/renderer tests/e2e | head -n 360
printf '%s\n' '--- E2E workflow/platform matrix ---'
rg -n -C 10 --glob '*.yml' --glob '*.yaml' --glob '*.json' --glob 'package.json' \
'test:e2e|playwright test|windows-latest|runs-on:.*windows|electron-headless|electron-headful' \
.github package.json | head -n 320Repository: stablyai/orca
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- local PTY process inspection ---'
sed -n '1,260p' src/main/providers/local-pty-provider-foreground-process.test.ts
rg -n -C 12 'getForegroundProcess|foregroundProcess' \
src/main/providers/local-pty-provider.ts \
src/main/providers/pty-process-inspection.ts \
src/main/daemon/pty-subprocess.ts \
src/main/daemon/pty-subprocess-foreground-identity.test.ts \
src/main/daemon/pty-subprocess-foreground-scan-cadence.test.ts
printf '%s\n' '--- Windows shell setting resolution ---'
rg -n -C 10 --glob '*.{ts,tsx}' \
'terminalWindowsShell' src/main src/renderer/src/store src/shared | \
rg -v 'node_modules' | head -n 260
printf '%s\n' '--- Windows workflow E2E invocations ---'
rg -n -C 15 --glob '*.yml' --glob '*.yaml' \
'runs-on:.*windows|windows-latest|test:e2e|playwright test|tests/playwright.config.ts' \
.github/workflows | head -n 360Repository: stablyai/orca
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- shell-name fallback and normalization ---'
rg -n -C 8 \
'function resolveForegroundFallbackProcess|resolveForegroundFallbackProcess|ptyShellName|shellPath.*basename|basename\(.*shell|shellName' \
src/main/providers/local-pty-provider.ts src/main/daemon/pty-subprocess.ts
printf '%s\n' '--- relevant Windows shell defaults ---'
rg -n -C 12 \
'terminalWindowsShell|DEFAULT.*SHELL|default.*shell|powershell\.exe|cmd\.exe|git-bash' \
src/main/providers/local-pty-provider.ts src/main/daemon/pty-subprocess.ts \
src/main/ipc/pty/register-handlers.ts src/main/ipc/pty/runtime \
src/renderer/src/store src/shared | head -n 300
printf '%s\n' '--- workflow files and Windows E2E references ---'
for file in .github/workflows/*.yml .github/workflows/*.yaml; do
[ -f "$file" ] || continue
if rg -q -i 'windows|test:e2e|playwright' "$file"; then
echo "### $file"
rg -n -C 8 -i 'windows|test:e2e|playwright|runs-on|shell:' "$file" | head -n 180
fi
doneRepository: stablyai/orca
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- getWindowsShell wiring ---'
rg -n -C 14 'getWindowsShell\s*[:=]|getWindowsShell\(|terminalWindowsShell' \
src/main src/renderer/src --glob '*.{ts,tsx}' | \
rg -v 'test\.ts|\.test\.tsx' | head -n 220
printf '%s\n' '--- Windows shell setting defaults ---'
rg -n -C 10 \
'terminalWindowsShell' \
src --glob '*.{ts,tsx}' | \
rg -v 'test\.ts|\.test\.tsx' | head -n 220
printf '%s\n' '--- workflow files that run E2E tests ---'
rg -l -i --glob '*.yml' --glob '*.yaml' 'test:e2e|playwright test' .github/workflows | sort
for file in $(rg -l -i --glob '*.yml' --glob '*.yaml' 'test:e2e|playwright test' .github/workflows); do
echo "### $file"
rg -n -C 12 -i 'test:e2e|playwright test|runs-on|windows|ORCA_E2E' "$file" | head -n 180
doneRepository: stablyai/orca
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -e
python3 - <<'PY'
from pathlib import Path
import re
test = Path("tests/e2e/terminal-hidden-child-tui-kill-mode-reset.spec.ts").read_text()
constants = Path("src/shared/constants.ts").read_text()
provider = Path("src/main/providers/local-pty-provider.ts").read_text()
workflow = Path(".github/workflows/windows-terminal-restart-e2e.yml").read_text()
pattern = re.search(r"\.toMatch\(/([^/]+)/\)", test).group(1)
default_shell = re.search(r"terminalWindowsShell:\s*'([^']+)'", constants).group(1)
fallback = re.search(
r"args\.shellOverride.*?process\.env\.COMSPEC\s*\|\|\s*'([^']+)'",
provider,
re.S,
).group(1)
candidates = ["bash", "zsh", "sh", "fish", "powershell.exe", "pwsh.exe", "cmd.exe"]
print("assertion:", pattern)
print("default Windows shell:", default_shell)
print("provider fallback shell:", fallback)
for name in candidates:
print(f"{name}: {'matches' if re.fullmatch(pattern, name) else 'does not match'}")
print("target test is in Windows restart workflow:", "terminal-hidden-child-tui-kill-mode-reset.spec.ts" in workflow)
PYRepository: stablyai/orca
Length of output: 452
Accept supported Windows shell names.
When this spec runs on Windows, terminalWindowsShell defaults to powershell.exe. The POSIX-only matcher rejects powershell.exe, pwsh.exe, and cmd.exe, so the poll times out. Match supported Windows shell basenames.
126541b to
c96c15a
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
c96c15a to
9f85d22
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
config/reliability-gates.jsonc (1)
5023-5033: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRecord an evidence run for the deterministic and mixed-version commands.
The gate registers four commands but only one
evidenceRunsentry, for the Playwright journey. ThecoverageNotesandredGreenEvidenceclaim deterministic host/parser, renderer, and mixed-version wire coverage, so those runs are the main support for the gate. Add oneevidenceRunsentry for the focused vitest command and one forcross-version-terminal-wire.unit.test.ts, matching the sibling gates that pair each command with a recorded run.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f5c986f7-55b8-43fd-ad39-28cf34d69116
📥 Commits
Reviewing files that changed from the base of the PR and between 290f192 and c96c15aea55d803afdc603479d3e7d387896bd4b.
📒 Files selected for processing (74)
config/reliability-gates.jsoncsrc/main/daemon/daemon-checkpoint-file.tssrc/main/daemon/daemon-durable-history-snapshot.tssrc/main/daemon/daemon-foreground-process-protocol.tssrc/main/daemon/daemon-pty-adapter-test-harness.tssrc/main/daemon/daemon-pty-adapter.test.tssrc/main/daemon/daemon-pty-buffer-snapshots.tssrc/main/daemon/daemon-pty-process-inspection.tssrc/main/daemon/daemon-pty-router.tssrc/main/daemon/daemon-pty-spawn-result.tssrc/main/daemon/daemon-request-router.tssrc/main/daemon/daemon-restore-scrollback-depth.test.tssrc/main/daemon/degraded-daemon-pty-provider.tssrc/main/daemon/headless-emulator.test.tssrc/main/daemon/headless-emulator.tssrc/main/daemon/pty-subprocess/foreground-process-tracker.tssrc/main/daemon/pty-subprocess/subprocess-handle.tssrc/main/daemon/session-output-plane.tssrc/main/daemon/session-shell-foreground-confirmation.tssrc/main/daemon/session-subprocess-handle.tssrc/main/daemon/session.test.tssrc/main/daemon/session.tssrc/main/daemon/terminal-checkpoint-serializer.test.tssrc/main/daemon/terminal-checkpoint-serializer.tssrc/main/daemon/terminal-history-checkpoint-reader.tssrc/main/daemon/terminal-history-cold-restore-info.tssrc/main/daemon/terminal-host-concurrent-create.test.tssrc/main/daemon/terminal-host-session-create.tssrc/main/daemon/terminal-host.tssrc/main/daemon/terminal-shell-ownership.tssrc/main/daemon/terminal-snapshot.tssrc/main/daemon/types.tssrc/main/ipc/pty/ipc/snapshot.tssrc/main/ipc/pty/ipc/spawn-commit.tssrc/main/ipc/pty/runtime/controller.tssrc/main/ipc/pty/runtime/operations.tssrc/main/providers/agent-foreground-process.test.tssrc/main/providers/agent-foreground-process.tssrc/main/providers/local-pty-provider.tssrc/main/providers/pty-provider-contract.tssrc/main/providers/pty-spawn-result.tssrc/main/providers/ssh-pty-provider.test.tssrc/main/runtime/orca-runtime.test.tssrc/main/runtime/orca-runtime.tssrc/main/runtime/rpc/methods/terminal/terminal-legacy-subscribe-snapshot.tssrc/main/runtime/rpc/methods/terminal/terminal-multiplex-flow-control.tssrc/main/runtime/rpc/methods/terminal/terminal-multiplex-initial-snapshot.tssrc/main/runtime/rpc/methods/terminal/terminal-multiplex-slot-frames.tssrc/main/runtime/rpc/methods/terminal/terminal-snapshot-publication.tssrc/main/runtime/rpc/methods/terminal/terminal-stream-types.tssrc/main/runtime/rpc/terminal-multiplex-escape-tail.test.tssrc/main/runtime/rpc/terminal-subscribe-buffer.test.tssrc/preload/api/pty-api.tssrc/preload/index.tssrc/renderer/src/components/terminal-pane/ipc-pty-connect-result.tssrc/renderer/src/components/terminal-pane/pty-connection-daemon-snapshot-replay.test.tssrc/renderer/src/components/terminal-pane/pty-connection-hidden-codex-queries.test.tssrc/renderer/src/components/terminal-pane/pty-connection-reattach-mode-reset.test.tssrc/renderer/src/components/terminal-pane/pty-connection/apply-reattach-payload.tssrc/renderer/src/components/terminal-pane/pty-connection/deferred-cold-restore-and-snapshot.tssrc/renderer/src/components/terminal-pane/pty-connection/fresh-spawn-follow-reset.tssrc/renderer/src/components/terminal-pane/pty-connection/hidden-output-restore-snapshot.tssrc/renderer/src/components/terminal-pane/pty-connection/replay-data-drain.tssrc/renderer/src/components/terminal-pane/pty-output-processor.tssrc/renderer/src/components/terminal-pane/pty-transport-types.tssrc/renderer/src/components/terminal-pane/remote-runtime-pty-snapshot-escape-tail.test.tssrc/renderer/src/components/terminal-pane/remote-runtime-pty-transport.tssrc/renderer/src/components/terminal-pane/terminal-replay-cursor-state.test.tssrc/renderer/src/runtime/remote-runtime-terminal-multiplexer.tssrc/shared/terminal-mode-reset-profiles.tssrc/shared/terminal-owner.tstests/e2e/cross-version-wire/cross-version-terminal-wire.unit.test.tstests/e2e/cross-version-wire/host-terminal-runtime-stub.tstests/e2e/terminal-hidden-child-tui-kill-mode-reset.spec.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/shared/terminal-mode-reset-profiles.ts
- src/renderer/src/components/terminal-pane/terminal-replay-cursor-state.test.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.
| private installShellOwnershipConfirmation(ptyId: string, state: RuntimeHeadlessTerminal): void { | ||
| state.emulator.setShellOwnershipConfirmation(async () => { | ||
| const controller = this.ptyController | ||
| const lifecycleGeneration = this.getPtyLifecycleGeneration(ptyId) | ||
| if (!controller?.confirmShellForeground || this.headlessTerminals.get(ptyId) !== state) { | ||
| return false | ||
| } | ||
| const confirmed = await controller.confirmShellForeground(ptyId) | ||
| return ( | ||
| confirmed && | ||
| this.headlessTerminals.get(ptyId) === state && | ||
| this.getPtyLifecycleGeneration(ptyId) === lifecycleGeneration | ||
| ) | ||
| }) | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Wrap controller.confirmShellForeground(ptyId) in a try/catch.
The confirmation callback awaits controller.confirmShellForeground(ptyId) with no error handling. If the call rejects (for example, a dropped SSH or daemon connection), the rejection propagates out of this callback. Every other PTY controller call in this file that can reject is wrapped in a try/catch (for example getForegroundProcess callers and hasPty callers). Without the same protection here, a rejected confirmation turns "missing proof" into a thrown error instead of the graceful false the design requires for revoked or unavailable proof.
Add a try/catch around the call and return false on error.
🛡️ Proposed fix
if (!controller?.confirmShellForeground || this.headlessTerminals.get(ptyId) !== state) {
return false
}
- const confirmed = await controller.confirmShellForeground(ptyId)
+ let confirmed = false
+ try {
+ confirmed = await controller.confirmShellForeground(ptyId)
+ } catch {
+ return false
+ }
return (
confirmed &&
this.headlessTerminals.get(ptyId) === state &&
this.getPtyLifecycleGeneration(ptyId) === lifecycleGeneration
)📝 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.
| private installShellOwnershipConfirmation(ptyId: string, state: RuntimeHeadlessTerminal): void { | |
| state.emulator.setShellOwnershipConfirmation(async () => { | |
| const controller = this.ptyController | |
| const lifecycleGeneration = this.getPtyLifecycleGeneration(ptyId) | |
| if (!controller?.confirmShellForeground || this.headlessTerminals.get(ptyId) !== state) { | |
| return false | |
| } | |
| const confirmed = await controller.confirmShellForeground(ptyId) | |
| return ( | |
| confirmed && | |
| this.headlessTerminals.get(ptyId) === state && | |
| this.getPtyLifecycleGeneration(ptyId) === lifecycleGeneration | |
| ) | |
| }) | |
| } | |
| private installShellOwnershipConfirmation(ptyId: string, state: RuntimeHeadlessTerminal): void { | |
| state.emulator.setShellOwnershipConfirmation(async () => { | |
| const controller = this.ptyController | |
| const lifecycleGeneration = this.getPtyLifecycleGeneration(ptyId) | |
| if (!controller?.confirmShellForeground || this.headlessTerminals.get(ptyId) !== state) { | |
| return false | |
| } | |
| let confirmed = false | |
| try { | |
| confirmed = await controller.confirmShellForeground(ptyId) | |
| } catch { | |
| return false | |
| } | |
| return ( | |
| confirmed && | |
| this.headlessTerminals.get(ptyId) === state && | |
| this.getPtyLifecycleGeneration(ptyId) === lifecycleGeneration | |
| ) | |
| }) | |
| } |
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Re-check controller identity after the await.
The callback captures controller = this.ptyController before awaiting controller.confirmShellForeground(ptyId), then after the await checks only this.headlessTerminals.get(ptyId) === state and this.getPtyLifecycleGeneration(ptyId) === lifecycleGeneration. Neither check catches a controller swap: setPtyController reassigns this.ptyController without bumping any PTY's lifecycle generation, and preserveLivePtysDuringReload can keep the same headless state object alive across a renderer reload. So a confirmation result obtained from a now-replaced controller can still pass validation.
This file already has the exact defensive pattern needed for this case in loadPtyForegroundAgentFromController, which checks result.controller === this.ptyController after its own await. Add the same check here.
🛡️ Proposed fix
const confirmed = await controller.confirmShellForeground(ptyId)
return (
confirmed &&
+ controller === this.ptyController &&
this.headlessTerminals.get(ptyId) === state &&
this.getPtyLifecycleGeneration(ptyId) === lifecycleGeneration
)📝 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.
| private installShellOwnershipConfirmation(ptyId: string, state: RuntimeHeadlessTerminal): void { | |
| state.emulator.setShellOwnershipConfirmation(async () => { | |
| const controller = this.ptyController | |
| const lifecycleGeneration = this.getPtyLifecycleGeneration(ptyId) | |
| if (!controller?.confirmShellForeground || this.headlessTerminals.get(ptyId) !== state) { | |
| return false | |
| } | |
| const confirmed = await controller.confirmShellForeground(ptyId) | |
| return ( | |
| confirmed && | |
| this.headlessTerminals.get(ptyId) === state && | |
| this.getPtyLifecycleGeneration(ptyId) === lifecycleGeneration | |
| ) | |
| }) | |
| } | |
| private installShellOwnershipConfirmation(ptyId: string, state: RuntimeHeadlessTerminal): void { | |
| state.emulator.setShellOwnershipConfirmation(async () => { | |
| const controller = this.ptyController | |
| const lifecycleGeneration = this.getPtyLifecycleGeneration(ptyId) | |
| if (!controller?.confirmShellForeground || this.headlessTerminals.get(ptyId) !== state) { | |
| return false | |
| } | |
| const confirmed = await controller.confirmShellForeground(ptyId) | |
| return ( | |
| confirmed && | |
| controller === this.ptyController && | |
| this.headlessTerminals.get(ptyId) === state && | |
| this.getPtyLifecycleGeneration(ptyId) === lifecycleGeneration | |
| ) | |
| }) | |
| } |
9f85d22 to
f1f786c
Compare
Replaces the snapshot-time normalization design: getSnapshot() mutated the live emulator by injecting reset bytes, which corrupted split escape sequences, discarded the shell's post-kill prompt with the dead alt frame, and let proof and parser state advance at different speeds. Recovery now lives in the daemon session's ordered data path (TerminalShellRecoveryBarrier): an OSC 133;D completing while the alternate screen is still active pauses the stream at that exact byte boundary, the execution host proves the shell owns the PTY foreground, and on proof the reset is injected as in-stream output — every consumer (daemon emulator, runtime mirror, attached renderers, history, mobile and old-version clients) converges by parsing the same bytes, and the queued prompt lands on the normal buffer instead of being discarded. Clean alt-screen exits prove ownership without pausing. Any failure (refuted proof, 750ms bail, 256KiB queue cap, death, disposal) flushes the queue unmodified. Snapshots are pure reads; ownership comes from a chunk-split-safe lifecycle scanner shared by the barrier, the runtime mirror, and durable replay. Carve-outs: bytes the dying command printed before its 133;D are discarded with the dead frame (only post-boundary output is preservable); direct local/degraded sessions get proof + renderer reset but no in-stream normalization; direct SSH and WSL fail closed (pinned by test). Hardened by two executed adversarial review rounds: drain try/finally + per-emission and head-release throw guards (a throwing client transport can no longer drop queued bytes, skip the episode, or wedge createOrAttach), one-shot trigger arming per alt occupancy (a refuted proof cannot storm pause-and-inspect episodes on every later prompt), bounded settle deadlines so attach/serialize never inherit a slow inspection, seeded alternate-screen state so a mirror reattached mid-TUI still arms recovery, and a synchronous teardown flush so shutdown checkpoints keep the queued prompt. Evidence: scanner/barrier/mirror/session suites (session-shell-recovery.test pins the two original defect repros as regression oracles), 7752 main-side + 3794 renderer-side tests green, and the two-case Electron journey passes with a new prompt-visibility oracle on normal exit.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/main/daemon/terminal-shell-lifecycle-scanner.ts (1)
138-141: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winClamp the trigger end to 1 so consumers always make progress.
The clamp guards an invariant that two consumer loops depend on for termination.
consumeForStateOnlyinterminal-shell-recovery-barrier.tsandscanAllForOwnershipindaemon-durable-history-snapshot.tsboth runwhile (rest.length > 0)and slice by this value. A returned0makes those loops spin forever on the daemon output path. A lower bound of 1 removes that failure mode without changing current behavior, because the value is already> 0by construction.♻️ Proposed clamp change
- events.uncleanDeathTriggerEnd = Math.max( - 0, - match.index + match[0].length - previousTailLength - ) + // Clamp to 1: consumers slice by this value in a loop, so zero progress must be impossible. + events.uncleanDeathTriggerEnd = Math.max( + 1, + match.index + match[0].length - previousTailLength + )
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f49b3a58-ad5e-4d88-802a-bc4dc387cec1
📒 Files selected for processing (21)
config/reliability-gates.jsoncsrc/main/daemon/daemon-durable-history-snapshot.tssrc/main/daemon/daemon-pty-adapter.test.tssrc/main/daemon/session-output-pipeline.tssrc/main/daemon/session-output-plane.tssrc/main/daemon/session-shell-recovery.test.tssrc/main/daemon/session.test.tssrc/main/daemon/session.tssrc/main/daemon/terminal-checkpoint-serializer.tssrc/main/daemon/terminal-shell-lifecycle-scanner.test.tssrc/main/daemon/terminal-shell-lifecycle-scanner.tssrc/main/daemon/terminal-shell-recovery-barrier.test.tssrc/main/daemon/terminal-shell-recovery-barrier.tssrc/main/providers/agent-foreground-process.test.tssrc/main/runtime/mobile-subscribe-integration.test.tssrc/main/runtime/orca-runtime.test.tssrc/main/runtime/orca-runtime.tssrc/main/runtime/pty-shell-ownership-mirror.test.tssrc/main/runtime/pty-shell-ownership-mirror.tssrc/renderer/src/components/terminal-pane/pty-connection/hidden-output-restore-snapshot.tstests/e2e/terminal-hidden-child-tui-kill-mode-reset.spec.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review.
| try { | ||
| if (confirmed && this.isAlive()) { | ||
| // Scanned before release so alt-state stays honest; the reset bytes are | ||
| // deliberately inert for ownership (no OSC 133, no TUI mode enables). | ||
| this.scanner.scan(POST_REPLAY_DEAD_TUI_RESET) | ||
| try { | ||
| this.releaseDownstream({ | ||
| data: POST_REPLAY_DEAD_TUI_RESET, | ||
| rawStartSeq: this.pendingRawSeq, | ||
| rawEndSeq: this.pendingRawSeq, | ||
| transformed: true | ||
| }) | ||
| } catch { | ||
| // Why swallowed: a throwing downstream client must not strand the | ||
| // queued prompt bytes below. | ||
| } | ||
| this.scanner.trySetOwner(this.pendingGeneration) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
# Description: Show the reset profile definitions and check for ownership-revoking sequences.
set -euo pipefail
fd -t f 'terminal-mode-reset-profiles.ts' --exec cat -n {}
echo '--- revoking-sequence candidates in the profiles file ---'
fd -t f 'terminal-mode-reset-profiles.ts' --exec rg -nP '\\x1bc|\\u001bc|\\x1b\[\?(47|1000|1002|1003|1004|1005|1006|1015|1016|1047|1049)[0-9;]*h' {}Repository: stablyai/orca
Length of output: 8631
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- repository conventions and learnings ---'
for f in /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/*/*.md; do
case "$f" in
*/src-main-daemon/*|*/learnings/*) head -80 "$f" ;;
esac
done
echo '--- scanner ownership logic ---'
cat -n src/main/daemon/terminal-shell-lifecycle-scanner.ts | sed -n '1,130p'
echo '--- reset profile and barrier call site ---'
cat -n src/main/daemon/terminal-mode-reset-profiles.ts | sed -n '1,45p'
cat -n src/main/daemon/terminal-shell-recovery-barrier.ts | sed -n '250,290p'Repository: stablyai/orca
Length of output: 45366
🏁 Script executed:
#!/bin/bash
set -euo pipefail
profile="$(fd -t f -a 'terminal-mode-reset-profiles.ts' | head -n 1)"
printf '%s\n' "--- profile: $profile ---"
cat -n "$profile" | sed -n '1,45p'
echo '--- scanner private-mode and revoke implementation ---'
cat -n src/main/daemon/terminal-shell-lifecycle-scanner.ts | sed -n '125,190p'Repository: stablyai/orca
Length of output: 6955
Prevent the reset from revoking ownership. POST_REPLAY_DEAD_TUI_RESET emits RESET_KITTY_KEYBOARD_PROTOCOL (\x1b[<99u). TerminalShellLifecycleScanner.scan() treats its 99 parameter as an ownership revocation, increments the generation, and makes trySetOwner(this.pendingGeneration) fail. The reset is still released, so the confirmed shell owner is lost.
…fallback - Retire hung ownership confirmations (barrier clean-exit path and runtime mirror) behind the existing deadline so a never-settling proof cannot wedge confirmation for the rest of the session; late verdicts from retired attempts are inert. Also guard a synchronously throwing confirm callback. - Honor attach cancellation that lands during the ownership-settle await so a canceled reattach no longer detaches the live viewer. - Recheck session map identity after the confirmShellForeground RPC await. - Fail closed when Windows PTY job inspection rejects. - Seed durable-checkpoint ownership from the live snapshot when there is no disk history; the record re-scan can revoke but never mint, so rebuilds no longer drop a valid proof. - Fix max-lines: extract confirmPtyShellForeground from the foreground process tracker.
…w F1) An adversarial review found a confirmed byte-loss regression: a shell exiting while an ownership proof was in flight reached handleSubprocessExit -> reapSession -> dispose() without ever flushing the barrier's queue, so every byte after the OSC 133;D boundary (recovery prompt, exit banner) was dropped from clients, records, and durable history — strictly worse than incumbent behavior for the exact death case the feature targets. Both death paths now flush synchronously (no injection: the episode resolves as refuted) before clients detach, with a Session-level regression test modeling the reaper's synchronous exit->dispose chain. Also from the review: getSettledSnapshot no longer nulls out a pre-exit snapshot when the session dies mid-settle (matches the old sync semantics); gate knownGaps document the stopped-descendant proof refutation and worst-case settle latency stacking; a comment pins the load-bearing terminalOwner conjunct on alternateScreen publication.
…s drain The re-verification pass found flushPending() unwound only one nesting level: queue replay can re-enter pending on a nested trigger, so three alt-screen death cycles inside one proof window still lost the tail bytes at teardown. flushPending now drains to completion (bounded at 16 passes — each pass strictly releases the head, but a pathological stream must not spin teardown). Both teardown flushes also moved after startupIngress.drainAndClose so held ingress bytes that re-enter the barrier are delivered by the same flush. Session-level regression test: three stacked episodes, shell exits mid-proof, every marker reaches clients with no injection.
…leteness Final adversarial pass (fresh reviewer) found four non-blocking defects, all fixed here: - A shell exiting inside the attach path's new settle window threw SessionNotFoundError where the pre-settle sync path respawned a fresh shell. The post-settle guard now distinguishes staleness that means "not yours" (replaced session, terminating, teardown-owned) from a plain death, which falls through to the existing dead-session respawn. The prior characterization test pinning the throw is rewritten to pin the respawn. - flushPending's 16-pass rescan bound silently dropped the queue tail past depth 16; it now releases the remainder verbatim after the bound — the bound trades scanner-model fidelity at teardown, never bytes. - prepareForFinalSnapshot flushed before snapshotBarrier, inverting the order the teardown paths established; snapshotBarrier can emit held ingress spans into the barrier, so the flush now runs last. - Deleted the barrier's uncalled seedOwner. Clean-exit confirmation extracted to terminal-shell-clean-exit-confirmation.ts (pure refactor) to keep the barrier under the max-lines cap.
The respawn fall-through treated every mid-settle death alike, but a kill that COMPLETES during the settle leaves no isTerminating or teardown trace (exit clears the flag, the sweep entry deletes itself) — so a reattach parked in the settle window resurrected a session the user had just killed from another surface. The kill tombstone (set at kill, cleared only by the next create) is the one signal that survives; the fall-through now checks it and throws. Pinned with a host.kill()-during-settle test asserting no respawn. Also corrected the storm-flush comment: a checkpoint after the verbatim release can read a stale owner.
…eal the first frame An attach that landed inside an in-flight ownership proof previously waited (bounded 750ms) so its first snapshot reflected post-recovery state. That wait created a race window — cancel, exit, and kill during the await each produced a regression across review rounds. Attach is now synchronous again (pre-rework contract): a viewer connecting inside the ~100ms proof window gets the pre-reset snapshot and the injected reset arrives in order over its stream, self-healing the first frame. Checkpoint/serialize readers keep their bounded settle via getSettledSnapshot. The settle-window guards (replaced-session/cancel/tombstone) and their tests are deleted with the window itself.
… suite The branch's two-line addition pushed mobile-subscribe-integration.test.ts past the 800-effective-line test cap in full lint; the coalescing describe block is a separate concern and moves to its own file unchanged.
(cherry picked from commit d60a3c9)
Summary
A TUI can leave a hidden terminal's viewer in alternate-screen and mouse-tracking mode after the TUI process has exited or been killed. When the surviving shell is revealed or reattached, pointer input is then encoded as terminal mouse reports and appears as raw
M.../ CSI fragments instead of normal shell interaction.This change makes recovery event-driven and host-owned. It adds no reveal timer, polling loop, periodic reset, global remount, or steady-state typing/output delay.
Reliability invariant
After a TUI exits or is killed, reveal, reattach, snapshot replay, or renderer remount must not deliver terminal-owned mouse or alternate-screen protocol bytes to the surviving shell. Recovery is permitted only when ordered shell completion and fresh execution-host process inspection establish sequence-bound shell ownership. Missing or revoked proof preserves incumbent behavior.
Root cause and fix
The first incorrect boundary was renderer restore precedence: stale viewer-side agent status/title metadata could outrank fresh execution-host evidence that the shell had regained the PTY.
The fix establishes one ownership chain:
OSC 133;Dcommand completion.terminalOwner: 'shell'plus normal-buffer state.Ordinary output and live-TUI frames perform zero ownership inspections. A poisoned mode episode performs one inspection, not one after every later plain command. Direct SSH exposes no authoritative confirmation API, so it fails closed immediately to incumbent behavior instead of adding a network wait or guessing from relay state.
Deterministic reproduction
tests/e2e/terminal-hidden-child-tui-kill-mode-reset.spec.tsruns the same journey for normal exit and cleanup-free SIGKILL:With only the focused host-proven recovery disabled, the normal-exit case retains any-event mouse mode and the SIGKILL case retains alternate screen plus mouse mode; both fail. The candidate passes both.
Mixed-version behavior
The wire change is optional-field compatible. The existing
alternateScreenfield now carries the normalized host truth after proof, so old clients can benefit from a new host without understandingterminalOwner. Local daemon and paired runtime paths use the shared proof. Direct SSH intentionally retains incumbent behavior. Mobile layout re-streams do not publish ownership on the unrelated layout sequence.Validation
Risks and gaps
No issue, message, deployment, or merge action was performed. This PR is not merged.