Repository navigation
fix: close readiness gaps found by merged-change audit - #17159
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 changes add keyboard-interactive SSH authentication with abortable credential requests and updated verification UI. SSH install locks track host boot identity, and relay reattachment refuses to revive tombstoned surfaces. PTY shutdown and daemon session control now enforce incarnation fencing, and the runtime service fences handle replacement and rejects duplicate graph tab IDs. Remote workspace sync waits for delayed worktree placement, retries stale snapshot applications, and validates upload authority. Terminal tab registration and CLI split routing are now worktree-scoped, and web-session terminal orphan recovery is restructured into dedicated modules. Native PTY handling, platform awake assertions, pricing, editor state, cleanup, and CI workflow caching receive related updates, with corresponding unit and end-to-end test coverage. Merge Risk: 🟡 Moderate · up to The PR improves SSH terminal lifecycle, authentication, remote-workspace synchronization, terminal routing, and Windows PTY handling, but the current head still has bounded correctness and failure-handling risks that could misclassify reconnect failures, misroute terminal state, falsely report workspace synchronization, suppress unrelated daemon errors, or allow incomplete hydration validation. Merge should wait for these issues to be fixed or explicitly accepted. 🚥 Pre-merge checks | ✅ 1 | ❌ 4❌ Failed checks (4 warnings)
✅ Passed checks (1 passed)
Full details: Linked Issues checkExplanation The changes address the core requirements for Resolution Implement the Full details: Out of Scope Changes checkExplanation The PR includes changes unrelated to the linked issue objectives, including macOS sleep assertion behavior, quick-open directory cleanup, deletion of the Linux proc scanner, Sonnet 4.6 pricing, Node-next native caching, skill-guide cleanup, Markdown styling, and other unrelated UI and test changes. Resolution Split unrelated fixes into separate pull requests with their own linked issues, or add explicit linked issue context that justifies each change. Keep this PR limited to the SSH lifecycle, terminal hydration and split recovery, remote-runtime reconciliation, and directly required supporting changes. Full details: Docstring CoverageExplanation Docstring coverage is 15.69% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 51 functions across 51 files. (1 skipped: 1 unsupported.) Full details: Description checkExplanation The description gives detailed scope, behavior contracts, linked issues, verification results, residual risks, and test evidence. However, it omits several template sections, including the required Visual Proof section, explicit ELI5/What Changed/Why sections, AI Disclosure, Agent skill upstream boundary, Notes, and the final Checklist. Resolution Add the missing template sections. Include 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 |
7220d4a to
362bbc3
Compare
Greptile SummaryThis remediation PR strengthens SSH lifecycle, authentication, remote-workspace hydration, and terminal-platform behavior, with supporting regression coverage.
Confidence Score: 4/5The PR is not yet safe to merge because snapshot retry exhaustion can still strand a connected SSH target in pulling state with its remote tabs unavailable. The attempted fix retries stale snapshot preparation only three times and then returns without applying the snapshot, updating sync state, or arranging another reconciliation. Files Needing Attention: src/renderer/src/hooks/remote-workspace-target-sync.ts
|
| Filename | Overview |
|---|---|
| src/renderer/src/hooks/remote-workspace-target-sync.ts | Adds bounded stale-token retries, but exhausting all attempts still leaves the current snapshot unresolved and the target pulling. |
| src/renderer/src/hooks/remote-workspace-snapshot-apply.ts | Returns explicit apply outcomes and waits for late worktree placement before merging remote terminal state. |
| src/renderer/src/hooks/remote-workspace-snapshot-placement.ts | Adds bounded, subscription-driven worktree-path placement checks scoped to the SSH execution host. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Receive current remote snapshot] --> B[Apply with preparation token]
B -->|Applied or failed| C[Resolve sync state]
B -->|Stale| D[Refresh catalog and prepare]
D --> E{Attempts remaining?}
E -->|Yes| B
E -->|No| F[Current code returns silently]
F --> G[Target remains pulling and unhydrated]
Reviews (4): Last reviewed commit: "test(wire): pin pre-metadata release cov..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| for (let attempt = 0; attempt < MAX_SNAPSHOT_APPLY_ATTEMPTS; attempt += 1) { | ||
| const result = await applyDirectSshRemoteWorkspaceSnapshot({ | ||
| store: deps.store, | ||
| snapshot, | ||
| token: applyToken, | ||
| arrival, | ||
| isArrivalCurrent, | ||
| isPreparationTokenCurrent: deps.isPreparationTokenCurrent, | ||
| waitForWorkspaceSessionReady, | ||
| finalizeHydratedTerminals: deps.finalizeHydratedTerminals | ||
| }) | ||
| if (result !== 'stale' || !isArrivalCurrent(authority.targetId, arrival) || stopped) { | ||
| return | ||
| } | ||
| const input = await deps.capturePreparationInput( | ||
| authority, | ||
| 'workspace-snapshot', | ||
| snapshot.revision | ||
| ) | ||
| if (!input || !isArrivalCurrent(authority.targetId, arrival)) { | ||
| return | ||
| } | ||
| const prepared = await deps.prepareOnly(input) | ||
| if ( | ||
| !prepared.token || | ||
| !deps.isPreparationTokenCurrent(prepared.token) || | ||
| !isArrivalCurrent(authority.targetId, arrival) | ||
| ) { | ||
| return | ||
| } | ||
| const refreshedToken = buildDirectSshSnapshotApplyToken(prepared.token, snapshot.revision) | ||
| if (!refreshedToken) { | ||
| return | ||
| } | ||
| applyToken = refreshedToken | ||
| } | ||
| } |
There was a problem hiding this comment.
When the worktree catalog changes during all three preparation attempts, the final stale result falls through without applying the snapshot, updating the sync status, or scheduling another reconciliation. The target remains pulling and unhydrated, so its remote tabs remain unavailable until another snapshot event or reconnect occurs.
Knowledge Base Used:
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2ee90cce-35d3-48ef-aa98-17e9cc890110
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (70)
.github/workflows/node-next-compat.ymlconfig/patches/node-pty@1.1.0.patchconfig/reliability-gates.jsoncconfig/scripts/generate-bundled-skill-guides.test.mjsconfig/scripts/pr-workflow-parallelism.test.mjsskill-guides/orca-per-workspace-env.mdsrc/cli/bundled-skill-guides.tssrc/main/agent-awake-service-platform-assertions.test.tssrc/main/agent-awake-service.test.tssrc/main/agent-awake-service.tssrc/main/claude-usage/claude-model-pricing.test.tssrc/main/claude-usage/claude-model-pricing.tssrc/main/claude-usage/store.test.tssrc/main/daemon/daemon-entry.tssrc/main/daemon/daemon-native-pty-exception.test.tssrc/main/daemon/daemon-native-pty-exception.tssrc/main/daemon/node-pty-windows-input-error.win32.test.tssrc/main/ipc/ssh-connection-state-callbacks.tssrc/main/ipc/ssh-passphrase.test.tssrc/main/ipc/ssh-passphrase.tssrc/main/ipc/ssh.tssrc/main/macos-system-sleep-assertion.tssrc/main/persistence-ssh-pending-pty-kill.test.tssrc/main/persistence/leasing-ssh-ptys/ssh-pty-kill-intent-operations.tssrc/main/persistence/loading-store/pty-binding-persistence.tssrc/main/providers/pty-provider-contract.tssrc/main/providers/ssh-pty-provider.test.tssrc/main/providers/ssh-pty-provider.tssrc/main/ssh-reattach-pane-cardinality.test.tssrc/main/ssh/ssh-connection-auth-fallback.test.tssrc/main/ssh/ssh-connection-gssapi-fallback.test.tssrc/main/ssh/ssh-connection-host-key-store-wiring.test.tssrc/main/ssh/ssh-connection-test-harness.tssrc/main/ssh/ssh-connection-utils.tssrc/main/ssh/ssh-connection.test.tssrc/main/ssh/ssh-connection.tssrc/main/ssh/ssh-multi-key-authentication.test.tssrc/main/ssh/ssh-pending-pty-kill-replay.test.tssrc/main/ssh/ssh-pending-pty-kill-replay.tssrc/main/ssh/ssh-relay-cross-version-isolation.test.tssrc/main/ssh/ssh-relay-install-lock-commands.tssrc/main/ssh/ssh-relay-install-lock.tssrc/main/ssh/ssh-relay-session-pending-kill-replay.test.tssrc/main/ssh/ssh-relay-session-reconnect-incarnation.test.tssrc/main/ssh/ssh-relay-session-test-fixtures.tssrc/main/ssh/ssh-relay-session.tssrc/main/ssh/ssh-relay-upload-cancel.docker.test.tssrc/main/ssh/ssh-remote-commands.test.tssrc/preload/api/ssh-api.tssrc/preload/index.tssrc/relay/pty-handler-retired-pane-surface.test.tssrc/relay/pty-handler.tssrc/renderer/src/app-shell/remote-workspace-unplaced-upload-suppression.test.tsxsrc/renderer/src/components/editor/EditorMarkdownFileSurface.tsxsrc/renderer/src/components/editor/EditorPanel.tsxsrc/renderer/src/components/editor/editor-panel-render-model.test.tssrc/renderer/src/components/editor/editor-panel-render-model.tssrc/renderer/src/components/settings/SshPassphraseDialog.tsxsrc/renderer/src/hooks/remote-workspace-snapshot-apply.tssrc/renderer/src/hooks/remote-workspace-snapshot-placement.tssrc/renderer/src/hooks/remote-workspace-target-sync.test.tssrc/renderer/src/hooks/remote-workspace-target-sync.tssrc/renderer/src/i18n/locales/en.jsonsrc/renderer/src/store/slices/ssh.tssrc/shared/linux-proc-socket-owner-scanner.test.tssrc/shared/linux-proc-socket-owner-scanner.tssrc/shared/quick-open-directory-reader.tssrc/shared/quick-open-readdir-walk.test.tssrc/shared/ssh-pending-pty-kill.tstests/e2e/ssh-cold-hydration-gap-tab-seeding.spec.ts
💤 Files with no reviewable changes (2)
- src/shared/linux-proc-socket-owner-scanner.test.ts
- src/shared/linux-proc-socket-owner-scanner.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review.
| setTimeout( | ||
| () => emitSshEvent('error', new Error('Timed out while waiting for handshake')), | ||
| readyTimeout | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Scope mock lifecycle events to each MockSshClient.
eventHandlers is a module-level map, but the new pending timeout and destroy() emit through emitSshEvent. If a reconnect or test creates more than one client, one client's timeout or close event is delivered to every client's listener. The pending timer also remains active after destroy(). This can reject or close the wrong connection and make lifecycle tests pass or fail for the wrong reason. Store handlers and the handshake timer on each MockSshClient, emit only to that client's handlers, and clear the timer in destroy().
Also applies to: 212-212
| const error = new Error('Timed out while waiting for SSH authentication') | ||
| Object.assign(error, { level: 'client-timeout' }) | ||
| onStartupError(error) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
# Description: Check how transient-error classification treats timeout errors.
set -euo pipefail
fd -t f 'ssh-connection-utils.ts' | while IFS= read -r f; do
rg -n -C 12 'isTransientError|isTransientReconnectError|client-timeout|Timed out' "$f"
done
# Also find any other definitions of these predicates.
rg -nP -C 12 'function (isTransientError|isTransientReconnectError)\b' --type=tsRepository: stablyai/orca
Length of output: 1008
🏁 Script executed:
#!/bin/bash
set -euo pipefail
utils="$(fd -t f 'ssh-connection-utils.ts' | head -n 1)"
printf '%s\n' "=== $utils ==="
cat -n "$utils" | sed -n '1,125p'
printf '%s\n' '=== predicate definitions and call sites ==='
rg -n -C 8 'isTransientError|isTransientReconnectError' src/main/ssh --type ts
printf '%s\n' '=== changed timeout path ==='
cat -n src/main/ssh/ssh-connection.ts | sed -n '1390,1460p'
printf '%s\n' '=== applicable repository conventions ==='
find /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4 -maxdepth 2 -type f -name '*.md' -print \
| while IFS= read -r f; do
case "$f" in
*/conventions/*ssh*.md|*/conventions/*SSH*.md|*/architecture/*ssh*.md|*/architecture/*SSH*.md|*/learnings/*ssh*.md|*/learnings/*SSH*.md)
printf '%s\n' "=== $f ==="
cat "$f"
;;
esac
doneRepository: stablyai/orca
Length of output: 40511
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '=== reconnect classifier ==='
cat -n src/main/ssh/ssh-reconnect-error-classification.ts | sed -n '1,78p'
printf '%s\n' '=== connect retry path ==='
cat -n src/main/ssh/ssh-connection.ts | sed -n '650,715p'
printf '%s\n' '=== startup error wiring and reconnect path ==='
rg -n -C 14 'onStartupError|runReconnectAttempt|connect\(' src/main/ssh/ssh-connection.tsRepository: stablyai/orca
Length of output: 24812
Classify the replacement timeout as transient.
isTransientError ignores level: 'client-timeout' and does not match Timed out while waiting for SSH authentication. isTransientReconnectError also does not match this message. Both paths can therefore enter the terminal error state instead of retrying.
| [ | ||
| `cat ${shellQuote(`${remoteRelayDir}/.install-lock/.boot-id`)}`, | ||
| `cat ${shellQuote(`${remoteRelayDir}/relay.js`)}`, | ||
| `find ${shellQuote('/root/.orca-remote')} -maxdepth 1 -name 'relay-reboot-lock-oracle.tombstone.*' -print | wc -l | tr -d ' '` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Check the actual tombstone location.
tryStealInstallLockCommand creates ${remoteRelayDir}/.install-lock.tombstone.*. This command scans /root/.orca-remote for relay-reboot-lock-oracle.tombstone.*, so it always reports zero. Scan ${remoteRelayDir} for .install-lock.tombstone.*.
Proposed fix
- `find ${shellQuote('/root/.orca-remote')} -maxdepth 1 -name 'relay-reboot-lock-oracle.tombstone.*' -print | wc -l | tr -d ' '`
+ `find ${shellQuote(remoteRelayDir)} -maxdepth 1 -name '.install-lock.tombstone.*' -print | wc -l | tr -d ' '`📝 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.
| `find ${shellQuote('/root/.orca-remote')} -maxdepth 1 -name 'relay-reboot-lock-oracle.tombstone.*' -print | wc -l | tr -d ' '` | |
| `find ${shellQuote(remoteRelayDir)} -maxdepth 1 -name '.install-lock.tombstone.*' -print | wc -l | tr -d ' '` |
🧰 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, spawnSync } from 'node:child_process'
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
| initialToken: DirectSshSnapshotApplyToken | ||
| ): Promise<void> => { | ||
| let applyToken = initialToken | ||
| for (let attempt = 0; attempt < MAX_SNAPSHOT_APPLY_ATTEMPTS; attempt += 1) { |
There was a problem hiding this comment.
When catalog or preparation-token changes make all three snapshot application attempts stale while the arrival remains current, this loop exits without applying the snapshot, resolving the pulling status, or scheduling reconciliation. The target remains unhydrated with its remote tabs unavailable until another snapshot event or reconnect occurs.
Knowledge Base Used:
d33f993 to
c5872e5
Compare
|
Too many files changed for review (284 files, 100 file limit). Bypass the limit by tagging |
|
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. |
c5872e5 to
03531c7
Compare
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (4)
tests/e2e/helpers/paired-terminal-restart-renderer-probes.ts (1)
170-178: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExclude
stackfrom the deduplication key.The
keyincludes thestackvalue. A store subscription runs from different dispatch call sites, so the captured stack text can change while the binding state stays the same. The probe then records duplicate transitions andtransitionsgrows during the long restart test. Build the key from the state fields only, and keepstackas diagnostic data.♻️ Proposed refactor
- const key = JSON.stringify(transition) + const { stack: _stack, ...state } = transition + const key = JSON.stringify(state) if (key !== probe.lastKey) { probe.lastKey = key probe.transitions.push(transition) }tests/e2e/paired-remote-terminal-serve-restart-binding.spec.ts (1)
587-594: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCompute
invalidRestartTransitionsat assertion time.The filter runs at Line 587, but the assertion runs at Line 634. The probe phase stays
'restart'until Line 640, so transitions recorded during the post-restart output, resize, and keyboard steps are captured by the probe and never inspected. Re-read the transitions immediately before the assertion so the invariant covers the whole restart phase.♻️ Proposed refactor
- const invalidRestartTransitions = restartTransitions.filter( - (transition) => - !transition.tabPresent || - !transition.layoutPresent || - transition.binding === null || - transition.bindings.length === 0 || - transition.tabPtyId !== transition.binding - ) + const collectInvalidRestartTransitions = async () => + (await readPairedTerminalBindingTransitions(client!.page)).filter( + (transition) => + transition.phase === 'restart' && + (!transition.tabPresent || + !transition.layoutPresent || + transition.binding === null || + transition.bindings.length === 0 || + transition.tabPtyId !== transition.binding) + )Then use it at the assertion site:
expect( - invalidRestartTransitions, + await collectInvalidRestartTransitions(), 'A pending-handle publication erased or removed the verified mirrored binding' ).toEqual([])src/renderer/src/hooks/remote-workspace-snapshot-apply.ts (1)
162-169: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated session-import step.
Lines 146-150 and 165-169 build
remoteSessionwith identical options. Only the collectedunplacedTabWorktreePathsarray differs. A local closure removes the copy and keeps the two passes in sync if the option set grows.♻️ Proposed refactor
+ const importAgainstCurrentState = (): { + state: AppState + worktreeIds: ReadonlySet<string> + unplaced: string[] + remoteSession: ReturnType<typeof importRemoteWorkspaceSession> + } => { + const nextState = store.getState() + const nextWorktreeIds = resolveDirectSshSnapshotWorktreeIds(nextState, authority) + const unplaced: string[] = [] + return { + state: nextState, + worktreeIds: nextWorktreeIds, + unplaced, + remoteSession: importRemoteWorkspaceSession(snapshot.session, { + resolveWorktreeId: uniqueWorktreeIdByPath(nextWorktreeIds), + executionHostId: toSshExecutionHostId(authority.targetId), + onUnplacedTerminalTabs: (worktreePath) => unplaced.push(worktreePath) + }) + } + }src/main/runtime/orca-runtime-terminal-close-continuity-graph-fixture.ts (1)
147-149: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep canary rows in
syncSplitFixtureGraph.When
includeCanaryis true, this helper dropscanarySessionLayout,canarySyncedTab, andcanarySyncedLeaf. Preserve the existing layouts and include the canary rows as the other graph helpers do.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 025f9da6-f136-4065-82cf-f0d68045f447
📥 Commits
Reviewing files that changed from the base of the PR and between 7f822a7 and c5872e57e13def17267a68278d3a832eb57ee520.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (260)
.github/workflows/node-next-compat.ymlconfig/patches/node-pty@1.1.0.patchconfig/reliability-gates.jsoncconfig/scripts/generate-bundled-skill-guides.test.mjsconfig/scripts/pr-e2e-gate-contract.test.mjsconfig/scripts/pr-e2e-source-routing.mjsconfig/scripts/pr-workflow-parallelism.test.mjsskill-guides/orca-per-workspace-env.mdsrc/cli/bundled-skill-guides.tssrc/cli/handlers/terminal.test.tssrc/cli/handlers/terminal.tssrc/main/agent-awake-service-platform-assertions.test.tssrc/main/agent-awake-service.test.tssrc/main/agent-awake-service.tssrc/main/claude-usage/claude-model-pricing.test.tssrc/main/claude-usage/claude-model-pricing.tssrc/main/claude-usage/store.test.tssrc/main/daemon/daemon-attach-only-retirement.tssrc/main/daemon/daemon-entry.tssrc/main/daemon/daemon-native-pty-exception.test.tssrc/main/daemon/daemon-native-pty-exception.tssrc/main/daemon/daemon-pty-adapter-cold-restore-reanchor.test.tssrc/main/daemon/daemon-pty-adapter-protocol-compatibility.test.tssrc/main/daemon/daemon-pty-adapter-replacement-exit-race.test.tssrc/main/daemon/daemon-pty-adapter-session-adoption.test.tssrc/main/daemon/daemon-pty-adapter.test.tssrc/main/daemon/daemon-pty-adapter.tssrc/main/daemon/daemon-pty-applied-size.tssrc/main/daemon/daemon-pty-connection-lifecycle.tssrc/main/daemon/daemon-pty-runtime-state.tssrc/main/daemon/daemon-pty-session-control.tssrc/main/daemon/daemon-pty-session-spawn.tssrc/main/daemon/daemon-pty-spawn-request.tssrc/main/daemon/daemon-pty-spawn-result.tssrc/main/daemon/node-pty-windows-input-error.win32.test.tssrc/main/ipc/pty/ipc/spawn-commit.tssrc/main/ipc/pty/pane/stable-pane-relay-absence-respawn.test.tssrc/main/ipc/pty/runtime/spawn-commit.tssrc/main/ipc/remote-workspace-cache.test.tssrc/main/ipc/remote-workspace-patch-queue.test.tssrc/main/ipc/remote-workspace-relay-sync.tssrc/main/ipc/remote-workspace-snapshot-cache.tssrc/main/ipc/remote-workspace.test.tssrc/main/ipc/remote-workspace.tssrc/main/ipc/ssh-connection-state-callbacks.tssrc/main/ipc/ssh-passphrase.test.tssrc/main/ipc/ssh-passphrase.tssrc/main/ipc/ssh.tssrc/main/macos-system-sleep-assertion.tssrc/main/persistence-ssh-pending-pty-kill.test.tssrc/main/persistence-ssh-remote-pty-binding-replay.test.tssrc/main/persistence/leasing-ssh-ptys/ssh-pty-kill-intent-operations.tssrc/main/persistence/loading-store/pty-binding-persistence.tssrc/main/persistence/loading-store/session-host-partitions.tssrc/main/persistence/loading-store/store-domain-composition.tssrc/main/persistence/loading-store/workspace-session-snapshot-publication.tssrc/main/persistence/loading-store/workspace-session-terminal-binding-replay.test.tssrc/main/persistence/loading-store/workspace-session-terminal-binding-replay.tssrc/main/providers/pty-provider-contract.tssrc/main/providers/pty-spawn-result.tssrc/main/providers/ssh-pty-provider.test.tssrc/main/providers/ssh-pty-provider.tssrc/main/runtime/mobile-session-terminal-retirement-proof.test.tssrc/main/runtime/mobile-session-terminal-retirement-proof.tssrc/main/runtime/mobile-session-terminal-retirement.tssrc/main/runtime/orca-runtime-tab-id-collision.test.tssrc/main/runtime/orca-runtime-terminal-close-continuity-fixtures.tssrc/main/runtime/orca-runtime-terminal-close-continuity-graph-fixture.tssrc/main/runtime/orca-runtime-terminal-close-continuity-state-fixture.tssrc/main/runtime/orca-runtime-terminal-close-continuity.test.tssrc/main/runtime/orca-runtime-terminal-handle-incarnation.test.tssrc/main/runtime/orca-runtime-terminal-retirement.test.tssrc/main/runtime/orca-runtime-terminal-split-authority.test.tssrc/main/runtime/orca-runtime.test.tssrc/main/runtime/orca-runtime.tssrc/main/runtime/rpc/methods/terminal/terminal-multiplex-cleanup.tssrc/main/runtime/rpc/methods/terminal/terminal-multiplex-connection.tssrc/main/runtime/rpc/methods/terminal/terminal-multiplex-flow-control.tssrc/main/runtime/rpc/methods/terminal/terminal-multiplex-frame-delivery.tssrc/main/runtime/rpc/methods/terminal/terminal-multiplex-live-stream.tssrc/main/runtime/rpc/methods/terminal/terminal-multiplex-slot-frames.tssrc/main/runtime/rpc/methods/terminal/terminal-multiplex-subscribe-frame.tssrc/main/runtime/rpc/methods/terminal/terminal-multiplex-subscribe-resolution.tssrc/main/runtime/rpc/terminal-multiplex-end-verdict.test.tssrc/main/ssh-reattach-pane-cardinality.test.tssrc/main/ssh/ssh-connection-auth-fallback.test.tssrc/main/ssh/ssh-connection-gssapi-fallback.test.tssrc/main/ssh/ssh-connection-host-key-store-wiring.test.tssrc/main/ssh/ssh-connection-test-client.tssrc/main/ssh/ssh-connection-test-harness.tssrc/main/ssh/ssh-connection-utils.test.tssrc/main/ssh/ssh-connection-utils.tssrc/main/ssh/ssh-connection.test.tssrc/main/ssh/ssh-connection.tssrc/main/ssh/ssh-multi-key-authentication.test.tssrc/main/ssh/ssh-pending-pty-kill-replay.test.tssrc/main/ssh/ssh-pending-pty-kill-replay.tssrc/main/ssh/ssh-reconnect-error-classification.test.tssrc/main/ssh/ssh-relay-cross-version-isolation.test.tssrc/main/ssh/ssh-relay-install-lock-commands.tssrc/main/ssh/ssh-relay-install-lock.tssrc/main/ssh/ssh-relay-session-pending-kill-replay.test.tssrc/main/ssh/ssh-relay-session-reconnect-incarnation.test.tssrc/main/ssh/ssh-relay-session-test-fixtures.tssrc/main/ssh/ssh-relay-session.tssrc/main/ssh/ssh-relay-upload-cancel.docker.test.tssrc/main/ssh/ssh-remote-commands.test.tssrc/main/window/runtime-window-lifecycle.tssrc/preload/api/ssh-api.tssrc/preload/api/ui-command-event-api.tssrc/preload/api/workspace-session-api.tssrc/preload/index.tssrc/relay/pty-handler-retired-pane-surface.test.tssrc/relay/pty-handler.tssrc/renderer/src/app-shell/reconcile-hydrated-workspace-tab-models.test.tssrc/renderer/src/app-shell/reconcile-hydrated-workspace-tab-models.tssrc/renderer/src/app-shell/remote-workspace-unplaced-upload-suppression.test.tsxsrc/renderer/src/app-shell/use-app-session-persistence.tssrc/renderer/src/app-shell/use-app-startup-hydration.tssrc/renderer/src/components/Terminal.tsxsrc/renderer/src/components/editor/EditorContent.markdown-classification.test.tsxsrc/renderer/src/components/editor/EditorMarkdownFileSurface.tsxsrc/renderer/src/components/editor/EditorPanel.tsxsrc/renderer/src/components/editor/editor-panel-render-model.test.tssrc/renderer/src/components/editor/editor-panel-render-model.tssrc/renderer/src/components/floating-terminal/FloatingTerminalPanel.tsxsrc/renderer/src/components/settings/SshPassphraseDialog.tsxsrc/renderer/src/components/terminal-pane/TerminalErrorToast.tsxsrc/renderer/src/components/terminal-pane/TerminalOverlaySlot.tsxsrc/renderer/src/components/terminal-pane/TerminalPane.tsxsrc/renderer/src/components/terminal-pane/TerminalPaneOverlayLayer.react185.test.tsxsrc/renderer/src/components/terminal-pane/codex-detached-pane-restart.tssrc/renderer/src/components/terminal-pane/pty-connection-pty-exit-teardown.test.tssrc/renderer/src/components/terminal-pane/pty-connection-session-liveness.test.tssrc/renderer/src/components/terminal-pane/pty-connection-types.tssrc/renderer/src/components/terminal-pane/pty-connection/pty-exit-hibernate.tssrc/renderer/src/components/terminal-pane/pty-connection/session-reconcile-dispose.tssrc/renderer/src/components/terminal-pane/pty-connection/transport-output-callbacks.tssrc/renderer/src/components/terminal-pane/pty-transport-types.tssrc/renderer/src/components/terminal-pane/remote-runtime-pty-transport-attach-subscription.test.tssrc/renderer/src/components/terminal-pane/remote-runtime-pty-transport-end-verdict.test.tssrc/renderer/src/components/terminal-pane/remote-runtime-pty-transport-expired-pane-recovery.test.tssrc/renderer/src/components/terminal-pane/remote-runtime-pty-transport-pane-handle-resolution.test.tssrc/renderer/src/components/terminal-pane/remote-runtime-pty-transport-stale-handle-recovery.test.tssrc/renderer/src/components/terminal-pane/remote-runtime-pty-transport.tssrc/renderer/src/components/terminal-pane/remote-runtime-resubscribe-failure-recovery-routing.test.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-error-accumulation.test.tssrc/renderer/src/components/terminal-pane/terminal-error-accumulation.tssrc/renderer/src/components/terminal-pane/terminal-pane-recovery.test.tssrc/renderer/src/components/terminal-pane/terminal-pane-recovery.tssrc/renderer/src/components/terminal-pane/terminal-pane-split-request-routing.test.tssrc/renderer/src/components/terminal-pane/terminal-pane-split-request-routing.tssrc/renderer/src/components/terminal-pane/terminal-parked-pty-watcher.tssrc/renderer/src/components/terminal-pane/terminal-parked-tab-eviction-exemption.test.tssrc/renderer/src/components/terminal-pane/terminal-parked-tab-watchers.test.tssrc/renderer/src/components/terminal-pane/terminal-startup-grid-settle.test.tssrc/renderer/src/components/terminal-pane/terminal-startup-grid-settle.tssrc/renderer/src/components/terminal-pane/use-terminal-pane-lifecycle.tssrc/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/components/terminal/background-terminal-worktree-mount.test.tssrc/renderer/src/components/terminal/background-terminal-worktree-mount.tssrc/renderer/src/constants/terminal.tssrc/renderer/src/hooks/ipc-events/terminal-command-state.tssrc/renderer/src/hooks/ipc-events/terminal-presentation-ipc-bridge.tssrc/renderer/src/hooks/ipc-events/terminal-request-ipc-bridge.tssrc/renderer/src/hooks/ipc-events/terminal-ui-routing-ipc-bridge-split.test.tssrc/renderer/src/hooks/ipc-events/terminal-ui-routing-ipc-bridge.tssrc/renderer/src/hooks/ipc-tab-switch-group-order-hydration.test.tssrc/renderer/src/hooks/ipc-tab-switch.test.tssrc/renderer/src/hooks/ipc-tab-switch.tssrc/renderer/src/hooks/remote-workspace-push-status.tssrc/renderer/src/hooks/remote-workspace-session-readiness.tssrc/renderer/src/hooks/remote-workspace-snapshot-apply-deferred-session-write.test.tssrc/renderer/src/hooks/remote-workspace-snapshot-apply.tssrc/renderer/src/hooks/remote-workspace-snapshot-arrival-coordinator.test.tssrc/renderer/src/hooks/remote-workspace-snapshot-arrival-coordinator.tssrc/renderer/src/hooks/remote-workspace-snapshot-duplicate-tab-repair.test.tssrc/renderer/src/hooks/remote-workspace-snapshot-fresh-client-tab-seeding.test.tssrc/renderer/src/hooks/remote-workspace-snapshot-local-tab-survival.test.tssrc/renderer/src/hooks/remote-workspace-snapshot-placement.tssrc/renderer/src/hooks/remote-workspace-snapshot-unplaced-tab-adoption.test.tssrc/renderer/src/hooks/remote-workspace-target-sync-test-harness.tssrc/renderer/src/hooks/remote-workspace-target-sync.test.tssrc/renderer/src/hooks/remote-workspace-target-sync.tssrc/renderer/src/hooks/useIpcEvents-terminal-create-surfacing.test.tssrc/renderer/src/i18n/locales/en.jsonsrc/renderer/src/lib/mobile-terminal-tab-mount.test.tssrc/renderer/src/lib/mobile-terminal-tab-mount.tssrc/renderer/src/lib/workspace-activation-terminal-focus.test.tssrc/renderer/src/lib/workspace-activation-terminal-focus.tssrc/renderer/src/runtime/host-session-mirror-hydration-frame-ordering.test.tsxsrc/renderer/src/runtime/remote-runtime-terminal-end-verdict.test.tssrc/renderer/src/runtime/remote-runtime-terminal-multiplexer-types.tssrc/renderer/src/runtime/remote-runtime-terminal-response-controller.tssrc/renderer/src/runtime/sync-runtime-graph-terminal-registration-ownership.test.tssrc/renderer/src/runtime/sync-runtime-graph.tssrc/renderer/src/runtime/web-runtime-session-browser-create-focus.test.tssrc/renderer/src/runtime/web-runtime-session-snapshot.tssrc/renderer/src/runtime/web-runtime-session-tab-activate-close.test.tssrc/renderer/src/runtime/web-runtime-session-test-harness.tssrc/renderer/src/runtime/web-runtime-session.test.tssrc/renderer/src/runtime/web-session-tabs-sync-mirror-identity.test.tssrc/renderer/src/runtime/web-session-tabs-sync-terminal-mirroring.test.tssrc/renderer/src/runtime/web-session-tabs-sync.test.tssrc/renderer/src/runtime/web-session-tabs-sync.tssrc/renderer/src/runtime/web-session-terminal-orphan-inventory-retry.test.tssrc/renderer/src/runtime/web-session-terminal-orphan-mixed-version.test.tssrc/renderer/src/runtime/web-session-terminal-orphan-recovery-adoption-regressions.test.tssrc/renderer/src/runtime/web-session-terminal-orphan-recovery-adoption.tssrc/renderer/src/runtime/web-session-terminal-orphan-recovery-cache.tssrc/renderer/src/runtime/web-session-terminal-orphan-recovery-inventory-validation.tssrc/renderer/src/runtime/web-session-terminal-orphan-recovery-inventory.tssrc/renderer/src/runtime/web-session-terminal-orphan-recovery-pane.tssrc/renderer/src/runtime/web-session-terminal-orphan-recovery-queue.tssrc/renderer/src/runtime/web-session-terminal-orphan-recovery-regression-fixtures.tssrc/renderer/src/runtime/web-session-terminal-orphan-recovery-regressions.test.tssrc/renderer/src/runtime/web-session-terminal-orphan-recovery-rpc-lane.tssrc/renderer/src/runtime/web-session-terminal-orphan-recovery-surface-index.tssrc/renderer/src/runtime/web-session-terminal-orphan-recovery-surface.tssrc/renderer/src/runtime/web-session-terminal-orphan-recovery-topology-fence.test.tssrc/renderer/src/runtime/web-session-terminal-orphan-recovery.test.tssrc/renderer/src/runtime/web-session-terminal-orphan-recovery.tssrc/renderer/src/runtime/web-session-terminal-orphan-topology.tssrc/renderer/src/runtime/web-session-terminal-pending-handle-recovery.test.tssrc/renderer/src/store/slices/ssh.tssrc/renderer/src/store/slices/store-session-workspace-hydration.test.tssrc/renderer/src/store/slices/tabs-model-reconciliation.test.tssrc/renderer/src/store/slices/tabs.tssrc/renderer/src/store/slices/terminal-orphan-helpers.test.tssrc/renderer/src/store/slices/terminal-orphan-helpers.tssrc/renderer/src/store/slices/terminals-hydration-canonical-rows.test.tssrc/renderer/src/store/slices/terminals.tssrc/renderer/src/store/slices/worktrees/teardown/remove-worktree-store-cleanup.tssrc/renderer/src/store/slices/worktrees/teardown/worktree-purge-state.tssrc/renderer/src/store/terminals/terminal-actions.tssrc/renderer/src/store/terminals/terminal-pty-bindings.tssrc/renderer/src/store/terminals/terminal-state.tssrc/renderer/src/store/terminals/terminal-tab-close.tssrc/renderer/src/store/terminals/terminal-unverified-pty-loss.tssrc/renderer/src/store/terminals/workspace-terminal-hydration-patch.tssrc/renderer/src/store/terminals/workspace-terminal-hydration.tssrc/shared/linux-proc-socket-owner-scanner.test.tssrc/shared/linux-proc-socket-owner-scanner.tssrc/shared/quick-open-directory-reader.tssrc/shared/quick-open-readdir-walk.test.tssrc/shared/remote-workspace-types.tssrc/shared/runtime-session-contracts.tssrc/shared/runtime-types.tssrc/shared/ssh-pending-pty-kill.tssrc/shared/terminal-exit-cause.test.tssrc/shared/terminal-exit-cause.tssrc/shared/terminal-stream-end-verdict.tstests/e2e/cross-version-wire/cross-version-terminal-wire.unit.test.tstests/e2e/helpers/paired-terminal-restart-renderer-probes.tstests/e2e/paired-remote-terminal-serve-restart-binding.spec.tstests/e2e/ssh-cold-hydration-gap-tab-seeding.spec.tstests/e2e/terminal-parked-cli-split.spec.ts
💤 Files with no reviewable changes (1)
- src/main/daemon/daemon-pty-spawn-request.ts
🚧 Files skipped from review as they are similar to previous changes (51)
- src/main/ssh/ssh-relay-session-test-fixtures.ts
- src/main/ssh/ssh-relay-session-pending-kill-replay.test.ts
- src/main/claude-usage/store.test.ts
- src/main/ipc/ssh-passphrase.test.ts
- src/main/daemon/node-pty-windows-input-error.win32.test.ts
- src/relay/pty-handler.ts
- src/main/ssh/ssh-relay-cross-version-isolation.test.ts
- src/main/ssh/ssh-multi-key-authentication.test.ts
- skill-guides/orca-per-workspace-env.md
- src/main/providers/ssh-pty-provider.ts
- src/renderer/src/components/editor/EditorPanel.tsx
- src/main/ssh/ssh-connection-gssapi-fallback.test.ts
- src/renderer/src/i18n/locales/en.json
- src/renderer/src/components/settings/SshPassphraseDialog.tsx
- src/renderer/src/components/editor/editor-panel-render-model.test.ts
- src/main/ssh/ssh-relay-install-lock.ts
- src/main/ssh/ssh-connection-auth-fallback.test.ts
- src/main/persistence/loading-store/pty-binding-persistence.ts
- src/main/daemon/daemon-entry.ts
- src/main/ipc/ssh-connection-state-callbacks.ts
- src/main/daemon/daemon-native-pty-exception.test.ts
- .github/workflows/node-next-compat.yml
- src/main/providers/ssh-pty-provider.test.ts
- src/main/agent-awake-service.ts
- src/main/ssh/ssh-connection-host-key-store-wiring.test.ts
- src/preload/api/ssh-api.ts
- src/main/ipc/ssh.ts
- src/main/ssh/ssh-pending-pty-kill-replay.test.ts
- src/main/providers/pty-provider-contract.ts
- src/renderer/src/components/editor/editor-panel-render-model.ts
- src/shared/ssh-pending-pty-kill.ts
- src/shared/quick-open-readdir-walk.test.ts
- src/renderer/src/components/editor/EditorMarkdownFileSurface.tsx
- src/shared/quick-open-directory-reader.ts
- src/relay/pty-handler-retired-pane-surface.test.ts
- src/renderer/src/store/slices/ssh.ts
- src/main/claude-usage/claude-model-pricing.ts
- src/main/agent-awake-service.test.ts
- src/main/ssh-reattach-pane-cardinality.test.ts
- src/main/persistence-ssh-pending-pty-kill.test.ts
- src/main/claude-usage/claude-model-pricing.test.ts
- config/scripts/generate-bundled-skill-guides.test.mjs
- src/main/ssh/ssh-relay-session.ts
- src/main/ssh/ssh-connection.ts
- src/main/ssh/ssh-pending-pty-kill-replay.ts
- tests/e2e/cross-version-wire/cross-version-terminal-wire.unit.test.ts
- src/main/ipc/ssh-passphrase.ts
- src/main/ssh/ssh-relay-install-lock-commands.ts
- src/renderer/src/hooks/remote-workspace-snapshot-placement.ts
- src/main/agent-awake-service-platform-assertions.test.ts
- config/patches/node-pty@1.1.0.patch
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review.
| "command": "pnpm exec vitest run --config config/vitest.config.ts src/renderer/src/components/terminal-pane/terminal-parked-tab-watchers.test.ts src/renderer/src/components/terminal-pane/terminal-pane-split-request-routing.test.ts src/renderer/src/components/terminal-pane/use-terminal-tab-cold-parking.test.ts src/renderer/src/hooks/ipc-events/terminal-ui-routing-ipc-bridge-split.test.ts src/main/runtime/orca-runtime-terminal-split-authority.test.ts src/renderer/src/runtime/sync-runtime-graph-parked-leaf.test.ts src/renderer/src/runtime/web-runtime-session.test.ts src/renderer/src/runtime/web-session-terminal-orphan-mixed-version.test.ts src/renderer/src/runtime/web-session-terminal-orphan-recovery.test.ts src/renderer/src/runtime/web-session-terminal-orphan-recovery-regressions.test.ts src/renderer/src/runtime/web-session-terminal-orphan-recovery-adoption-regressions.test.ts src/renderer/src/runtime/web-session-terminal-orphan-inventory-retry.test.ts src/renderer/src/runtime/web-session-terminal-pending-handle-recovery.test.ts tests/e2e/host-cold-park-remote-subscriber.unit.test.ts --reporter=dot", | ||
| "durationSeconds": 8, | ||
| "summary": "Three focused files passed 64 watcher, multi-pane runtime-graph, cold-park policy, and authoritative-stream tests." | ||
| "summary": "Fifteen deterministic files passed watcher, exact cold-parked split routing, multi-pane graph, pending-binding retention, exact adoption, mismatch quarantine, consecutive-absence confirmation, queue/cache bound, revision-fence, and eager-refresh tests." |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fix the file count in the evidence-run summary; it does not match the command.
The command field at line 17149 lists 14 test files. The summary field at line 17151 states "Fifteen deterministic files passed." Count the files in the command string:
terminal-parked-tab-watchers.test.tsterminal-pane-split-request-routing.test.tsuse-terminal-tab-cold-parking.test.tsterminal-ui-routing-ipc-bridge-split.test.tsorca-runtime-terminal-split-authority.test.tssync-runtime-graph-parked-leaf.test.tsweb-runtime-session.test.tsweb-session-terminal-orphan-mixed-version.test.tsweb-session-terminal-orphan-recovery.test.tsweb-session-terminal-orphan-recovery-regressions.test.tsweb-session-terminal-orphan-recovery-adoption-regressions.test.tsweb-session-terminal-orphan-inventory-retry.test.tsweb-session-terminal-pending-handle-recovery.test.tshost-cold-park-remote-subscriber.unit.test.ts
This is 14 files, not 15. This reliability gate exists to provide a precise audit trail for promotion decisions. Correct the summary to "Fourteen" or add the missing file to the command, whichever matches the actual run.
Proposed fix
- "summary": "Fifteen deterministic files passed watcher, exact cold-parked split routing, multi-pane graph, pending-binding retention, exact adoption, mismatch quarantine, consecutive-absence confirmation, queue/cache bound, revision-fence, and eager-refresh tests."
+ "summary": "Fourteen deterministic files passed watcher, exact cold-parked split routing, multi-pane graph, pending-binding retention, exact adoption, mismatch quarantine, consecutive-absence confirmation, queue/cache bound, revision-fence, and eager-refresh tests."📝 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.
| "command": "pnpm exec vitest run --config config/vitest.config.ts src/renderer/src/components/terminal-pane/terminal-parked-tab-watchers.test.ts src/renderer/src/components/terminal-pane/terminal-pane-split-request-routing.test.ts src/renderer/src/components/terminal-pane/use-terminal-tab-cold-parking.test.ts src/renderer/src/hooks/ipc-events/terminal-ui-routing-ipc-bridge-split.test.ts src/main/runtime/orca-runtime-terminal-split-authority.test.ts src/renderer/src/runtime/sync-runtime-graph-parked-leaf.test.ts src/renderer/src/runtime/web-runtime-session.test.ts src/renderer/src/runtime/web-session-terminal-orphan-mixed-version.test.ts src/renderer/src/runtime/web-session-terminal-orphan-recovery.test.ts src/renderer/src/runtime/web-session-terminal-orphan-recovery-regressions.test.ts src/renderer/src/runtime/web-session-terminal-orphan-recovery-adoption-regressions.test.ts src/renderer/src/runtime/web-session-terminal-orphan-inventory-retry.test.ts src/renderer/src/runtime/web-session-terminal-pending-handle-recovery.test.ts tests/e2e/host-cold-park-remote-subscriber.unit.test.ts --reporter=dot", | |
| "durationSeconds": 8, | |
| "summary": "Three focused files passed 64 watcher, multi-pane runtime-graph, cold-park policy, and authoritative-stream tests." | |
| "summary": "Fifteen deterministic files passed watcher, exact cold-parked split routing, multi-pane graph, pending-binding retention, exact adoption, mismatch quarantine, consecutive-absence confirmation, queue/cache bound, revision-fence, and eager-refresh tests." | |
| "command": "pnpm exec vitest run --config config/vitest.config.ts src/renderer/src/components/terminal-pane/terminal-parked-tab-watchers.test.ts src/renderer/src/components/terminal-pane/terminal-pane-split-request-routing.test.ts src/renderer/src/components/terminal-pane/use-terminal-tab-cold-parking.test.ts src/renderer/src/hooks/ipc-events/terminal-ui-routing-ipc-bridge-split.test.ts src/main/runtime/orca-runtime-terminal-split-authority.test.ts src/renderer/src/runtime/sync-runtime-graph-parked-leaf.test.ts src/renderer/src/runtime/web-runtime-session.test.ts src/renderer/src/runtime/web-session-terminal-orphan-mixed-version.test.ts src/renderer/src/runtime/web-session-terminal-orphan-recovery.test.ts src/renderer/src/runtime/web-session-terminal-orphan-recovery-regressions.test.ts src/renderer/src/runtime/web-session-terminal-orphan-recovery-adoption-regressions.test.ts src/renderer/src/runtime/web-session-terminal-orphan-inventory-retry.test.ts src/renderer/src/runtime/web-session-terminal-pending-handle-recovery.test.ts tests/e2e/host-cold-park-remote-subscriber.unit.test.ts --reporter=dot", | |
| "durationSeconds": 8, | |
| "summary": "Fourteen deterministic files passed watcher, exact cold-parked split routing, multi-pane graph, pending-binding retention, exact adoption, mismatch quarantine, consecutive-absence confirmation, queue/cache bound, revision-fence, and eager-refresh tests." |
| NATIVE_PTY_MESSAGE_PATTERN.test(error.message) || | ||
| (NODE_PTY_STACK_PATTERN.test(error.stack ?? '') && | ||
| NATIVE_PTY_ERROR_CODE_PATTERN.test(code ?? error.message)) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- applicable repository guidance ---'
find /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4 -type f -name '*.md' -print \
| while read -r f; do
case "$f" in
*/learnings/*|*/architecture/*) ;;
*) head -20 "$f";;
esac
done
printf '%s\n' '--- reviewed file ---'
cat -n src/main/daemon/daemon-native-pty-exception.ts
printf '%s\n' '--- daemon-entry bindings and call sites ---'
rg -n -C 8 'isNativePtyException|uncaughtException|onData|onExit|spawn|node-pty' src/main/daemon/daemon-entry.ts src/main/daemon
printf '%s\n' '--- related tests and definitions ---'
rg -n -C 5 'NATIVE_PTY_|isNativePtyException|native pty|EPIPE|EAGAIN|node-pty' src test tests 2>/dev/null || trueRepository: stablyai/orca
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- guidance files ---'
find /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4 -type f -name '*.md' \
! -path '*/learnings/*' ! -path '*/architecture/*' -print
printf '%s\n' '--- classifier ---'
cat -n src/main/daemon/daemon-native-pty-exception.ts
printf '%s\n' '--- uncaught-exception consumer ---'
rg -n -C 12 'isNativePtyException|uncaughtException|uncaughtExceptionMonitor' src/main/daemon/daemon-entry.ts
printf '%s\n' '--- PTY listener registrations in daemon scope ---'
rg -n -C 10 --glob '*.ts' --glob '*.tsx' 'onData|onExit|addListener|register.*Pty|spawn\\(' src/main/daemon
printf '%s\n' '--- focused existing tests ---'
rg -n -C 8 --glob '*.test.ts' --glob '*.spec.ts' 'daemon-native-pty-exception|isNativePtyException|NATIVE_PTY_MESSAGE_PATTERN|NATIVE_PTY_ERROR_CODE_PATTERN' srcRepository: stablyai/orca
Length of output: 5253
🌐 Web query:
node-pty 1.1.0 eventEmitter2.ts onData onExit synchronous listener implementation
💡 Result:
In node-pty 1.1.0, the onData and onExit events are implemented using a custom EventEmitter2 class defined in src/eventEmitter2.ts [1][2]. This implementation is synchronous by design [1][2]. When the fire() method is called on an EventEmitter2 instance, it iterates through its internal list of listeners and executes them sequentially in the same execution context [1][2]: 1. Snapshotting: The fire(data: T) method first creates a shallow copy of the current _listeners array into a local queue variable [1][2]. 2. Synchronous Execution: It then iterates through this queue and calls each listener synchronously using queue[i].call(undefined, data) [1][2]. Because the listeners are called directly within the loop, any errors thrown by a listener during execution will propagate up the stack and interrupt the event-firing process for any remaining listeners in the queue, unless explicitly caught within the listener itself [1][2]. In the context of the Terminal class, onData and onExit are exposed as IEvent types, which are initialized as EventEmitter2 instances [3][4]. The _forwardEvents method in Terminal connects the underlying platform events to these emitters: - onData is triggered when data is returned from the pty [5][6]. - onExit is triggered when the pty process exits [5][6]. This design ensures that event propagation occurs immediately upon the event being fired, without waiting for the next tick of the Node.js event loop [1][2].
Citations:
- 1: https://github.com/microsoft/node-pty/blob/d738123f/src/eventEmitter2.ts
- 2: https://app.unpkg.com/node-pty@1.1.0/files/src/eventEmitter2.ts
- 3: https://github.com/microsoft/node-pty/blob/d738123f/src/terminal.ts
- 4: https://github.com/microsoft/node-pty/blob/master/src/terminal.ts
- 5: https://app.unpkg.com/node-pty@1.1.0/files/src/terminal.ts
- 6: https://www.jsdocs.io/package/node-pty
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- daemon convention ---'
cat /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/conventions/src-main-daemon.md
printf '%s\n' '--- daemon files ---'
git ls-files 'src/main/daemon'
printf '%s\n' '--- daemon PTY/event registrations ---'
rg -n -F -e 'onData' -e 'onExit' -e '.on(' -e 'addListener' src/main/daemon
printf '%s\n' '--- daemon error creation and propagation ---'
rg -n -C 5 -F -e 'EPIPE' -e 'EAGAIN' -e 'EBADF' -e 'EIO' -e 'throw ' -e 'Promise.reject' src/main/daemonRepository: stablyai/orca
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- daemon source PTY registrations ---'
rg -n -C 12 --glob '*.ts' --glob '*.tsx' --glob '!*.test.ts' --glob '!*.spec.ts' \
-F -e 'onData' -e 'onExit' -e 'node-pty' src/main/daemon
printf '%s\n' '--- PTY subprocess source outline ---'
find src/main/daemon/pty-subprocess -maxdepth 2 -type f -name '*.ts' -print
printf '%s\n' '--- exact daemon convention body ---'
sed -n '1,220p' /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/conventions/src-main-daemon.mdRepository: stablyai/orca
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- subprocess handle contract and implementations ---'
cat -n src/main/daemon/session-subprocess-handle.ts
find src/main/daemon/pty-subprocess -maxdepth 2 -type f -name '*.ts' -print
rg -n -C 16 --glob '*.ts' --glob '!*.test.ts' --glob '!*.spec.ts' \
-F -e 'onData(' -e 'onExit(' -e 'terminal.onData' -e 'terminal.onExit' \
-e 'new Error(' src/main/daemon/pty-subprocess src/main/daemon/session.ts
printf '%s\n' '--- exact classifier messages across source ---'
rg -n -F -e 'Pty process exited' -e 'Invalid pty handle' \
-e 'Cannot resize a pty that has already exited' -e 'ioctl(2) failed' srcRepository: stablyai/orca
Length of output: 21529
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- event dispatch implementation ---'
cat -n src/main/daemon/pty-subprocess/pre-listener-events.ts
printf '%s\n' '--- classifier tests ---'
cat -n src/main/daemon/daemon-native-pty-exception.test.ts
printf '%s\n' '--- session callback dependencies ---'
sed -n '44,105p' src/main/daemon/session.ts
rg -n -C 18 --glob '*.ts' --glob '!*.test.ts' --glob '!*.spec.ts' \
-F -e 'ingestSubprocessData' -e 'acceptStartupIngress' -e 'startupIngress.accept' \
-e 'onEmission' -e 'recoveryBarrier.accept' src/main/daemonRepository: stablyai/orca
Length of output: 20135
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- startup ingress implementation ---'
rg -n -C 20 --glob '*.ts' --glob '!*.test.ts' --glob '!*.spec.ts' \
-F -e 'class PtyStartupIngress' -e 'export class PtyStartupIngress' -e 'write(' -e 'onEmission' src/shared src/main/daemon
printf '%s\n' '--- session exit path ---'
rg -n -C 28 --glob '*.ts' --glob '!*.test.ts' --glob '!*.spec.ts' \
-F -e 'handleSubprocessExit' -e 'broadcastExit' -e 'client.onExit' src/main/daemon
printf '%s\n' '--- concrete subprocess write binding ---'
sed -n '60,190p' src/main/daemon/pty-subprocess/subprocess-handle.tsRepository: stablyai/orca
Length of output: 50370
Require explicit PTY-origin evidence before suppressing errors.
isNativePtyException uses a node-pty stack frame as origin evidence, but PtyPreListenerEvents invokes Session callbacks directly from the synchronous node-pty event path. If such a callback throws an Error with code EPIPE or EAGAIN, this classifier can suppress it even when the error did not originate in a native PTY operation.
Carry an explicit native-origin marker, or classify errors at the native-operation call site. Add regression tests for callback-thrown EPIPE and EAGAIN errors with a lower node-pty frame. Both tests must expect false.
| hostVerifier?.(presentedHostKey, (ok) => { | ||
| ssh2Mock.lastHostKeyAccepted = ok | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
printf '%s\n' '--- conventions ---'
head -5 /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/*/*.md 2>/dev/null || true
printf '%s\n' '--- target outline ---'
ast-grep outline src/main/ssh/ssh-connection-test-client.ts
printf '%s\n' '--- target implementation ---'
cat -n src/main/ssh/ssh-connection-test-client.ts | sed -n '1,170p'
printf '%s\n' '--- relevant symbols ---'
rg -n --glob '!node_modules' 'hostVerifier|lastHostKeyAccepted|ssh-connection-test-client' src test tests 2>/dev/null | head -120Repository: stablyai/orca
Length of output: 22597
🏁 Script executed:
printf '%s\n' '--- applicable convention ---'
cat /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/conventions/src.md
printf '%s\n' '--- SSH learning ---'
cat /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/learnings/src-main-ssh.md
printf '%s\n' '--- host-key verifier contract and implementation ---'
cat -n src/main/ssh/ssh-host-key-verifier.ts | sed -n '145,190p'
printf '%s\n' '--- host-key verification tests ---'
cat -n src/main/ssh/ssh-connection-host-key-verification.test.ts | sed -n '1,145p'
printf '%s\n' '--- package pin ---'
rg -n '"ssh2"|ssh2@' package.json package-lock.json pnpm-lock.yaml yarn.lock 2>/dev/null | head -30Repository: stablyai/orca
Length of output: 11874
🏁 Script executed:
printf '%s\n' '--- complete verifier decision paths ---'
cat -n src/main/ssh/ssh-host-key-verifier.ts | sed -n '171,285p'
printf '%s\n' '--- production binding ---'
cat -n src/main/ssh/ssh-connection.ts | sed -n '1428,1455p'
printf '%s\n' '--- direct mock contract tests ---'
cat -n src/main/ssh/ssh-connection.test.ts | sed -n '45,90p'
printf '%s\n' '--- all hostVerifier assignments ---'
rg -n -C 3 --glob '*.ts' 'hostVerifier\s*:' src testsRepository: stablyai/orca
Length of output: 9582
🌐 Web query:
ssh2 1.17.0 Client hostVerifier synchronous return value verify callback source
💡 Result:
In the ssh2 library (including version 1.17.0), the hostVerifier function is used to verify the server's host key [1][2]. The function signature is (key[, callback]), where the key is provided as a Buffer (or a hex string if hostHash is configured) [1][3]. The hostVerifier determines whether to continue the handshake based on its return value or by using the provided callback [1][2]. Synchronous Verification To perform synchronous verification, return a boolean value directly from the hostVerifier function [1][2]: - Return true to accept the key and continue the handshake. - Return false to reject the key and disconnect [1][3]. Asynchronous Verification If you need to perform asynchronous verification (e.g., an external database lookup), you must not return a boolean value [1][4]. Instead: - Ensure the function returns undefined (do not use the async keyword, as it implicitly returns a Promise) [4]. - Call the callback argument with true to accept or false to reject [1][3][4]. Note: Using the async keyword on the hostVerifier function will cause it to return a Promise, which is not interpreted as a valid boolean check, potentially causing the library to proceed incorrectly [4]. If you wish to use async/await syntax, you should wrap the logic in an immediately invoked async function expression (IIFE) within the hostVerifier [4].
Citations:
- 1: https://npmx.dev/package/ssh2/v/%5E1.17.0
- 2: https://github.com/mscdex/ssh2?tab=readme-ov-file
- 3: https://github.com/mscdex/ssh2/blob/master/README.md
- 4: GitHub issue 1268 in mscdex/ssh2 (link omitted to avoid creating a cross-reference)
Wait for both host-verifier result forms before continuing.
ssh2 accepts a synchronous boolean or a callback result. This mock ignores a synchronous false and schedules ready before a delayed callback runs. Route both forms through one continuation before scheduling connection behavior.
| const resultState = useAppStore.getState() | ||
| const currentAuthorityByTargetId = new Map( | ||
| currentAuthorities.map((authority) => [authority.targetId, authority]) | ||
| ) | ||
| for (const { targetId, result } of results ?? []) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Mark each authority offline when no upload result exists.
If window.api.remoteWorkspace is unavailable, the optional call returns undefined. This loop then skips all targets. A target can retain a synced status although no patch request was sent. Iterate currentAuthorities and pass an undefined result to applyRemoteWorkspacePushStatus when the response has no entry for that target.
| const onPtyErrorClearedRef = useRef((paneId: number, message?: string) => { | ||
| setTerminalErrorsByPaneId((current) => clearPaneTerminalError(current, paneId, message)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Clear the same error representation that was stored.
When SSH reconnect owns errors, onPtyErrorRef stores the stripped message. This callback receives the original transport message. The exact-match clear then fails, so a retained non-SSH error line stays visible after the transport clears it.
First try to clear the original message. If it was not stored, clear its stripped form.
Proposed fix
const onPtyErrorClearedRef = useRef((paneId: number, message?: string) => {
- setTerminalErrorsByPaneId((current) => clearPaneTerminalError(current, paneId, message))
+ setTerminalErrorsByPaneId((current) => {
+ const clearedOriginal = clearPaneTerminalError(current, paneId, message)
+ if (message === undefined || clearedOriginal !== current) {
+ return clearedOriginal
+ }
+ const stripped = stripSshReconnectOwnedErrorLines(message)
+ return stripped === null
+ ? clearedOriginal
+ : clearPaneTerminalError(clearedOriginal, paneId, stripped)
+ })
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const onPtyErrorClearedRef = useRef((paneId: number, message?: string) => { | |
| setTerminalErrorsByPaneId((current) => clearPaneTerminalError(current, paneId, message)) | |
| const onPtyErrorClearedRef = useRef((paneId: number, message?: string) => { | |
| setTerminalErrorsByPaneId((current) => { | |
| const clearedOriginal = clearPaneTerminalError(current, paneId, message) | |
| if (message === undefined || clearedOriginal !== current) { | |
| return clearedOriginal | |
| } | |
| const stripped = stripSshReconnectOwnedErrorLines(message) | |
| return stripped === null | |
| ? clearedOriginal | |
| : clearPaneTerminalError(clearedOriginal, paneId, stripped) | |
| }) | |
| }) |
| } | ||
| seenEntityIds.add(tab.id) | ||
| const tabId = resolveTabId(tab.id) | ||
| result.push({ type: 'terminal', id: tab.id, ...(tabId ? { tabId } : {}) }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Activate the resolved split tab.
This fallback row retains tabId, but handleSwitchTerminalTab later calls only setActiveTab(next.id). For duplicate terminal entity IDs, that legacy lookup can select the first split copy instead of the resolved active-group copy.
Route the selected row through activateCyclableTab(store, next).
Proposed fix
- store.setActiveTab(next.id)
- store.setActiveTabType('terminal')
+ activateCyclableTab(store, next)
return true| activeGroupTerminalTabs.length >= 2 || | ||
| worktreeTerminalTabs.length <= activeGroupTerminalTabs.length |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Check all short active-group projections.
This early return bypasses the stale-row checks at Lines 250-251. If an active group has three terminal tabs but hydration exposes only two, keyboard cycling omits the third tab until hydration repairs.
Remove the activeGroupTerminalTabs.length >= 2 condition. Keep the later group-ownership checks to preserve valid split-local behavior.
Proposed fix
- if (
- activeGroupTerminalTabs.length >= 2 ||
- worktreeTerminalTabs.length <= activeGroupTerminalTabs.length
- ) {
+ if (worktreeTerminalTabs.length <= activeGroupTerminalTabs.length) {
return false
}📝 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.
| activeGroupTerminalTabs.length >= 2 || | |
| worktreeTerminalTabs.length <= activeGroupTerminalTabs.length | |
| worktreeTerminalTabs.length <= activeGroupTerminalTabs.length |
| const authority = | ||
| result.snapshot ?? currentTransientAuthority(store, targetId, fallbackAuthority) | ||
| store.setRemoteWorkspaceSyncStatus(targetId, { | ||
| phase: result.reason === 'stale-revision' ? 'conflict' : 'offline', | ||
| direction: 'push', | ||
| ...authority, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
# Confirm the sync-status type and the failure-variant snapshot shape.
set -euo pipefail
fd -t f 'remote-workspace-types.ts' -x cat -n {}
echo '--- setRemoteWorkspaceSyncStatus signature and status type ---'
rg -nP -C 8 'setRemoteWorkspaceSyncStatus\s*:' --type=ts
echo '--- other callers of applyRemoteWorkspacePushStatus ---'
rg -nP -C 4 '\bapplyRemoteWorkspacePushStatus\s*\(' --type=tsRepository: stablyai/orca
Length of output: 2424
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- repository conventions and learnings ---'
find /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4 -maxdepth 2 -type f -name '*.md' -print
echo '--- relevant source files ---'
fd -t f 'remote-workspace-(push-status|target-sync|types|snapshot-apply)\.ts$' src
echo '--- push status ---'
cat -n src/renderer/src/hooks/remote-workspace-push-status.ts | sed -n '1,120p'
echo '--- target sync call site ---'
cat -n src/renderer/src/hooks/remote-workspace-target-sync.ts | sed -n '200,255p'
echo '--- status definitions and setter ---'
rg -n -P -C 10 'RemoteWorkspaceSyncStatus|setRemoteWorkspaceSyncStatus|RemoteWorkspacePushAuthority|currentTransientAuthority' src/renderer/srcRepository: stablyai/orca
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- renderer conventions ---'
cat /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/conventions/src-renderer.md
echo '--- exact helper references ---'
rg -n -C 5 '\bapplyRemoteWorkspacePushStatus\b|\bRemoteWorkspacePushAuthority\b' src/renderer/src/hooks src/renderer/src/app-shell
echo '--- target-sync imports and authority setup ---'
cat -n src/renderer/src/hooks/remote-workspace-target-sync.ts | sed -n '1,110p'Repository: stablyai/orca
Length of output: 13961
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- renderer conventions ---'
cat /tmp/coderabbit-repo-knowledge/stablyai/orca-89dc44e4/conventions/src-renderer.md
echo '--- exact helper references ---'
rg -n -C 5 '\bapplyRemoteWorkspacePushStatus\b|\bRemoteWorkspacePushAuthority\b' src/renderer/src/hooks src/renderer/src/app-shell
echo '--- target-sync imports and authority setup ---'
cat -n src/renderer/src/hooks/remote-workspace-target-sync.ts | sed -n '1,110p'Repository: stablyai/orca
Length of output: 294
Project observed snapshots to RemoteWorkspacePushAuthority before storing sync status. applyRemoteWorkspacePushStatus spreads result.snapshot on the failure branch, and remote-workspace-target-sync.ts passes a full RemoteWorkspaceObservedSnapshot as fallbackAuthority. Because setRemoteWorkspaceSyncStatus stores the object directly, namespace, schemaVersion, and session can enter RemoteWorkspaceSyncStatus. Use { revision, updatedAt, hostObservationToken } at both boundaries, as the success branch already does.
📍 Affects 2 files
src/renderer/src/hooks/remote-workspace-push-status.ts#L53-L58(this comment)src/renderer/src/hooks/remote-workspace-target-sync.ts#L237-L237
| const splitPromise = runParkedSplitCli(userDataDir, sourceTerminal.handle) | ||
| let mountedDuringSplit: string[] = [] | ||
| await expect | ||
| .poll( | ||
| async () => { | ||
| mountedDuringSplit = await orcaPage.evaluate(() => | ||
| Array.from(window.__paneManagers?.keys() ?? []).sort() | ||
| ) | ||
| return mountedDuringSplit.includes(targetTabId) | ||
| }, | ||
| { timeout: HISTORICAL_SPLIT_TIMEOUT_MS, message: 'CLI did not remount its parked target' } | ||
| ) | ||
| .toBe(true) | ||
| const splitRun = await splitPromise |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Attach a rejection handler to splitPromise before the poll.
If the remount poll at Line 248 fails, the test throws before Line 259 awaits splitPromise. A later CLI failure then rejects an unobserved promise. Node reports an unhandled rejection, which can replace the clear poll failure with a confusing crash. Attach a no-op catch, or capture the settled result first.
🛠️ Proposed fix
const splitPromise = runParkedSplitCli(userDataDir, sourceTerminal.handle)
+ const settledSplit = splitPromise.catch((error: unknown) => error)
let mountedDuringSplit: string[] = []
@@
.toBe(true)
- const splitRun = await splitPromise
+ const settled = await settledSplit
+ if (settled instanceof Error) {
+ throw settled
+ }
+ const splitRun = settled🧰 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 { execFile } from 'node:child_process'
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
f05724d to
3015a12
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. |
88b9db5 to
2337035
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. |
b05bd9b to
69e1b67
Compare
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)
tests/e2e/ssh-cold-hydration-gap-tab-seeding.spec.ts (1)
203-210: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the seeded baseline size.
BASELINE_TAB_COUNTis used only in the failure message. The current assertion passes whenremote.tabIdscontains fewer than three tabs, including zero tabs. The later adoption assertion can then validate an incomplete baseline. Add an explicit cardinality assertion.Suggested test guard
expect( capturedTabIds, `the captured host snapshot ${snapshotPath} is not parseable JSON, so replaying it proves nothing: ${JSON.stringify(saved.slice(0, 200))}` ).not.toBeNull() + expect(remote.tabIds).toHaveLength(BASELINE_TAB_COUNT) expect( remote.tabIds.filter((id) => !(capturedTabIds ?? []).includes(id)),
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 863cb698-9e28-431f-a7d9-fc0e3a0a5967
📥 Commits
Reviewing files that changed from the base of the PR and between 233703586d0cda226ddce60b9334d5e8a3a38790 and 69e1b67.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
config/patches/node-pty@1.1.0.patchsrc/main/daemon/node-pty-windows-input-error.win32.test.tstests/e2e/ssh-cold-hydration-gap-tab-seeding.spec.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review.
9e889d9 to
e5eea62
Compare
* fix(ssh): fence stale kills and retired pane replay * fix(ssh): support cancellable interactive authentication * fix(ssh): await remote catalog before snapshot adoption * fix(pty): contain Windows ConPTY input failures * fix(power): avoid redundant macOS display blocking * perf(editor): narrow markdown override subscriptions * fix(quick-open): close directory handles after reads * refactor(linux): remove unused proc socket scanner * fix(usage): apply flat Sonnet 4.6 pricing * ci: prime Node next native test cache * docs(skills): resolve snapshot cleanup data path * fix(ssh): recover install locks after host reboot * test(ssh): recognize boot-aware install locks * test(ssh): prove previous-boot lock recovery live * test(wire): pin pre-metadata release coverage * fix(terminal): preserve remote tab ownership through recovery races * test(runtime): fence replaced terminal handles in agent guard * fix(ssh): preserve remote snapshot authority across polls * fix(pty): contain late ConPTY output EPIPE * test(pty): register Windows exit watcher before kill * fix: close SSH and tab readiness race gaps * fix(tabs): retain headless order and placeholder titles * fix(build): avoid parallel electron-vite config race * test(windows): avoid MSYS temp path rewriting * test(windows): avoid killing exited PTY * fix(pty): avoid late ConPTY input teardown race * fix(terminal): sync reconnect error ownership after commit * fix(runtime): use canonical worktree identity comparison * test(ssh): assert complete cold-hydration baseline * test(windows): invoke quoted retention fixture via PowerShell * test(windows): read ConPTY grid through mode con * fix(terminal): publish PTY replacements atomically * fix(terminal): infer stale identity on reattach * fix(terminal): fence stale pane PTY callbacks * fix(terminal): fence stale pane binds after rebind * fix(terminal): reject stale pane transport callbacks * fix(terminal): fence mirrored reattach spawn callbacks * fix(terminal): replace stale pane PTYs on remount * fix(ci): size the Windows launcher-compile test budget from measurement `native-smoke (windows-latest)` fails ~4.5% of runs on `preserves a multiline argument through the compiled remote launcher` with "Test timed out in 15000ms" — on unrelated PRs, for reasons that have nothing to do with them. Across 176 sampled attempts it is the only red that job produced, and it hit seven different PRs in two days: stablyai#16900, stablyai#16904, stablyai#16915, stablyai#16955 (twice), stablyai#16979, stablyai#17014, stablyai#17085. The test is six process creations: powershell.exe forks csc.exe, then the freshly compiled orca.exe forks node.exe, twice. Hosted Windows runners periodically slow process creation down, and this test amplifies that far harder than anything else in the job. Comparing the 80 attempts where it ran under 3s against the 12 where it ran over 12s, its own median goes 2198ms -> 15917ms (7.2x) while the same file's powershell-only test moves 556 -> 686ms (1.2x), the cmd.exe and Git Bash process tests in the neighbouring file move 1.4x, and the other 35 files put together move 1.5x. Measured across those 176 attempts: 1881ms to 35438ms, p50 4264ms, correlation +0.881 with the job's total Vitest duration. 8 of 176 (4.5%) exceeded the 15s cap; 2 of 176 (1.1%) also exceeded the shared 30s testTimeout, so deleting the override and inheriting the config is not enough on its own. 60s clears all 176 with 1.7x headroom on the worst. This is slow, not hung. Every body here is synchronous spawnSync, so Vitest cannot interrupt one — the timer fires only after the body returns and the reported duration is real elapsed time. That is why a failure reads `× ... 22464ms` under `Test timed out in 15000ms`. The work finished; the stopwatch was short. Seven reruns at one identical head measured 2053 / 4680 / 5551 / 8732 / 13506 / 14868 / 21937ms — the last of those would have been red on code that had not changed. The 15s came from stablyai#8897, which raised this test off Vitest's built-in 5s default because the job then ran bare `pnpm vitest run`. stablyai#8909 landed 3h27m later and pointed the job at config/vitest.config.ts, which is the real fix for that. The constant stayed behind and has been the binding budget ever since. * fix(terminal): fence stale remount reattach ownership * fix(terminal): reconcile mounted pane identity after replacement * fix(terminal): fence stale reattach fallback ownership * fix(terminal): fence deferred SSH reattach ownership * fix(terminal): fence stale split pane ownership callbacks * fix(terminal): keep stale spawns from consuming startup --------- Co-authored-by: Brennan Benson <79079362+brennanb2025@users.noreply.github.com>
* perf(terminal): let a worktree's terminal spawns run concurrently
The per-worktree terminal mutation guard was a FIFO mutex, so activating a
multi-tab worktree made each tab wait for every predecessor's whole spawn.
Measured with ORCA_PTY_SPAWN_TIMING=1 on a 4-tab worktree, the `options`
phase was a pure-queueing staircase: 0 / 125 / 212 / 291ms.
The invariant that guard protects is spawn-vs-sleep exclusion, never
spawn-vs-spawn. Replace it with a writer-preferring shared/exclusive lock:
spawns share, sleep still excludes, and a queued sleep blocks later spawns
so a stream of spawns cannot starve it into its 12s deadline.
Same worktree after: options = 2 / 3 / 12 / 34ms, and stable_adoption
flattens from 65/398/398/312ms to a steady ~253ms.
The existing folder-workspace control assertion asserted the FIFO behavior
this removes, so it is re-based on sleep-vs-spawn (which still queues) and
joined by a case asserting concurrent same-worktree spawns.
* perf(terminal): memoize the headless snapshot per mutation epoch
Attaching a viewer serializes the session's whole headless buffer
synchronously on the daemon event loop, so every reattach of a quiescent
session re-serialized identical bytes. Measured with ORCA_PTY_SPAWN_TIMING=1,
stable_adoption was 253-281ms per session on reattach.
Move snapshot assembly into HeadlessSnapshotCache and memoize its expensive
parts (the serialize, the OSC link walk, the frame-restore fields) on a
mutation epoch that every emulator state mutation bumps, so a cache hit is
byte-identical by construction rather than merely fresh-enough. The async
write path bumps on entry and again in the parse-completion callback, so a
snapshot taken mid-parse can never be retained.
Reattach of a quiescent session after: stable_adoption 12ms. Sessions with
output since their last snapshot re-serialize exactly as before.
Retention is capped: an entry is held for the session's lifetime once it goes
quiescent, and a renderer may request 50k scrollback rows, so oversized
payloads serve normally but are not retained. Cache hits clone nested values
so a caller mutating its snapshot cannot corrupt later ones.
* perf(terminal): keep the snapshot cache warm across zero-byte parse fences
Review follow-up. flushParsedWrites() is write(''), used purely as a parse
fence, and every getSettledSnapshot runs one — so the epoch bump on an empty
write evicted the attach entry on each checkpoint read, defeating the cache
for any session that gets checkpointed.
Zero bytes cannot mutate the buffer: the OSC and mouse-mode scans are no-ops
on '' and the partial-escape tail is idempotent, so skip the bump for empty
data. Real writes still bracket themselves, and any write a fence orders
behind has already bumped on its own completion.
Also invalidate on dispose, so a post-dispose read can never be served a
pre-dispose entry, and freeze the emulator's public method surface in a test:
the cache's correctness rests on every mutator calling markMutated(), which is
convention rather than a type, so a new method should be a deliberate decision
about invalidation instead of a silent stale-snapshot bug.
* refactor(terminal): apply elegance review to the attach-latency fixes
Reuse: waitForMutationGrant hand-rolled the deadline race that
settleBeforeDeadline (same directory, four existing callers) already owns.
Using it also picks up the timer.unref() the local copy lacked, which was
keeping the Node event loop alive for up to the 12s sleep deadline.
Simplify: drop the waiter `abandoned` flag. The timeout path sets it and
splices the waiter out in the same synchronous block, so no queued waiter can
ever be observed abandoned and both reads were unreachable. The splice is what
actually does the work; the comment now carries why that makes a
grant-after-timeout unrepresentable. drain() then collapses into its loop
condition and reuses markActive() instead of inlining it twice.
Extract markWritten() so the zero-byte parse-fence rationale lives at one
mutation gate instead of being restated at three call sites.
Match the daemon's byte-accounting convention: the retention cap is now
expressed in bytes with code-unit sizing, like MAX_COLD_RESTORE_CACHE_BYTES
next door, so the two retention budgets read in one unit. Same effective cap.
The public-surface guard test caught markWritten on the first run, which is
the behavior it was added for.
* perf(terminal): stop discarding the snapshot cache on non-memoized fields
Second elegance round, and it found the same class of bug as the parse-fence
one: cwd and lastTitle are read fresh on every build and were never memoized,
yet setCwd/setLastTitle bumped the epoch — discarding a whole serialize to
update a field the cache does not hold. OSC 7 cwd updates land on every `cd`,
so this was a live cost on exactly the busy sessions the cache targets. The
invariant is "every mutation of a memoized part", not "every state mutation";
both docblocks said the latter and are corrected.
Drop the dispose bump too. A post-dispose getSnapshot re-serializes the
disposed terminal to byte-identical content, so the bump bought nothing and
only reached into a disposed xterm — verified by probe, not assumed. Its test
asserted zero serializations after dispose, which no implementation could
violate; it passed with the bump deleted.
Also memoize rehydrateSequences (a string, so no clone needed) instead of
rebuilding it on every hit, derive the frameRestore type from
buildFrameRestoreSnapshotFields so a new field cannot flow through at runtime
while the type omits it, inline the single-caller resolve() into build(), and
move the write() entry bump after the sync early-return so the three bumps map
1:1 onto sync / async-pre-parse / async-post-parse.
The surface guard is sorted in source and renamed to say what it freezes: the
prototype, TS-private members included.
* fix(terminal): correct the fence justification and key the cache by window
The markWritten docblock claimed "zero bytes cannot mutate the buffer". That
is false, and I verified it: `_core.writeSync('')` drains xterm's pending queue
and applies it. The exemption is still correct, but for a different reason —
a fence cannot introduce an *unattributed* mutation, because any bytes it
drains belong to a queued async write whose own completion callback bumps
first. The two write regimes are exhaustive: with writeSync present nothing
can queue, without it every write is async and self-bumps. A comment asserting
a false invariant is worse than no comment, since the next change may rely on
it, so it now states the real one.
Key the cache by scrollback window instead of a single slot. Consumers ask for
different windows against the same emulator — attach passes the full window
while agent/text reads pass 0 — so one slot thrashed to a 0% hit rate whenever
they alternated, silently removing the benefit on runtime-side emulators. Two
entries cover every caller pair in the tree.
Drop the epoch counter: markMutated already nulls the retained entry and an
entry is only ever stored under the current epoch, so the comparison could
never fail. Invalidation is simply "clear the cache".
* refactor(terminal): name the lock sides shared/exclusive for a third caller
Rebasing onto main surfaced a semantic conflict the merge applied cleanly:
main added runWorktreeTerminalMutation (terminal orphan adoption, stablyai#17159) as a
third caller of the guard this PR changed. It needs the exclusive side —
adoption reconciles a worktree's terminal records, so it must not interleave
with a spawn registering a pty or with a sleep, which is exactly the semantics
it was written under when the guard was a plain mutex.
With three operations, naming the sides after two of them no longer fits, so
the kinds are now `shared` (spawn) and `exclusive` (sleep, adoption) — what
they do rather than who calls them.
* perf(terminal): do not invalidate the snapshot cache on a no-op resize
Re-measuring the final rebased build caught the cache barely working on the
path it exists for. Every attach re-asserts the pane's dimensions, and resize()
bumped unconditionally, so a reattach of a fully idle session missed its own
cached snapshot and re-serialized.
Measured on the same 4-tab worktree, reattach of sessions verified quiescent
(no buffer change over 4s), stable_adoption per session:
before this commit: 98 / 382 / 395 / 418 ms
after: 16 / 67 / 78 / 87 ms
A resize to the size already applied changes nothing the snapshot reads, so it
now returns early — which also stops it clearing restoredOscLinks, correct
since no rows shifted.
stablyai#17159 skipped Electron prevent-display-sleep whenever caffeinate was active. caffeinate -i/-s does not pin the display, and -s is a no-op on battery, so lid-closed Macs fell into Clamshell Sleep mid-agent-run. Start the existing display blocker only while keep-awake is blocking and the host is on battery. AC still uses caffeinate alone; idle still releases the blocker. Fixes stablyai#18427
stablyai#17159 skipped Electron prevent-display-sleep whenever caffeinate was active. caffeinate -i/-s does not pin the display, and -s is a no-op on battery, so lid-closed Macs fell into Clamshell Sleep mid-agent-run. Start the existing display blocker only while keep-awake is blocking and the host is on battery. AC still uses caffeinate alone; idle still releases the blocker. Fixes stablyai#18427
Summary
This branch closes the SSH/session-tab race gaps found in the readiness audit and fixes #12115. It preserves remote PTY ownership across reconnect/restart, prevents stale shutdown/remount callbacks from mutating replacement panes, and keeps split-pane PTY bindings keyed by durable leaf identity instead of reusable numeric pane slots.
The same ownership fencing covers delayed SSH attach/reattach, transport stream errors, teardown cleanup, and queued startup consumption. Existing headless tab order/title fixes remain included for the directly related parked-tab regressions.
Fixes #17144, #12115, #17297, and #15710.
Behavior contract
unverifiable, never evidence that the PTY exited. Only a confirmed process exit publishesexited.Verification
Final pushed head:
e202461110290cea32650984044096ccb988bc1b(rebased onorigin/main212c0e42a6518bc558ba75a61f8b76af64b894d2).pnpm tc— passed after final patch.pnpm run check:code-quality:changed— 0 new findings (including type-aware quality and React Doctor).oxfmt --checkandgit diff --check— passed.electron-vite build --mode e2e) — passed.terminal-parked-cli-split.spec.ts) — passed 1/1 on the final build.SKIP_BUILD=1run against a production build timed out before test setup becausewindow.__storeis intentionally exposed only by the E2E/development build; the correctly built E2E run passed.Readiness checklist
Scope and residual issues
This PR is the merge candidate for #17159; it does not claim every open GitHub tab report is fixed. Unrelated/provider-specific/mobile tab reports remain separate follow-ups. The only known release-validation gap is physical WSL reboot/sleep-resume coverage.