Repository navigation
fix(ssh): apply node-pty console fallback to Windows relays - #9638
Conversation
📝 WalkthroughWalkthroughAdds a SHA-256-guarded patch for 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
config/scripts/build-relay.mjs (1)
29-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPatch filename literal is duplicated across three files.
'node-pty-1.1.0-console-list-agent-patch.cjs'is hardcoded independently in the build script, the deploy source, and the repro tool; a rename or version bump can silently desync them since nothing but the string-matching contract test would catch a partial update.
config/scripts/build-relay.mjs#L29-L35: source of truth forNODE_PTY_CONSOLE_LIST_PATCH_FILENAME; consider moving this (and the version) to a small shared module importable by all three consumers.src/main/ssh/ssh-relay-deploy.ts#L506-L507: replace the locally re-declaredNODE_PTY_CONSOLE_LIST_PATCH_FILENAMEliteral with the shared constant.config/scripts/windows-ssh-attach-console-repro.mjs#L12-L12: replaceNODE_PTY_PATCH_FILENAME's literal with the same shared constant instead of a third independent copy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f511d799-d4d8-444a-9d24-b54563ddeb46
📒 Files selected for processing (6)
config/relay-assets/node-pty-1.1.0-console-list-agent-patch.cjsconfig/scripts/build-relay.mjsconfig/scripts/node-pty-console-list-agent-patch.test.mjsconfig/scripts/package-electron-runtime-contract.test.mjsconfig/scripts/windows-ssh-attach-console-repro.mjssrc/main/ssh/ssh-relay-deploy.ts
Co-authored-by: OrcaWin <293788423+OrcaWin@users.noreply.github.com>
Summary
node-pty@1.1.0console-list fallback to the independently installed Windows SSH relay dependency.[shellPid]cleanup fallback without upgrading to a broad node-pty beta or adding a process-wide exception handler.Closes #9586.
Screenshots
No visual change.
Testing
pnpm lint— targeted lint passes; the full command reaches two unchanged switch-exhaustiveness failures indaemon-server.tsandskill-freshness-group.tsx.pnpm typecheck—pnpm run typecheck:nodepasses; web/CLI projects were not changed.pnpm test— focused suites pass; the full suite was not run.pnpm build—pnpm run build:relaypasses for all relay targets; the full desktop/native build was not run.Focused results:
node-pty@1.1.0, detached throughWin32_Process.Create: real ConPTY spawn/shutdown succeeds and deterministically emitsAttachConsole failedfrom helper SHA-2560d010879...c85e52.ssh2.Serveron127.0.0.1, realSshConnection/SFTP): Node 22 detection, upload, native install, patch, detached WMI launch, named-pipe bridge/handshake, and cmd + Windows PowerShell ConPTY spawn/shutdown all pass; relay status returns zero active PTYs and both relay logs contain noAttachConsole failed(1/1 passed, 15.49s). The host had nosshd, so no service, firewall, account, or credential configuration was changed.84df20cf...dcd857and the native binding is present.pnpm exec vitest run --config config/vitest.config.ts config/scripts/node-pty-console-list-agent-patch.test.mjs src/main/ssh/ssh-relay-native-deps-install.test.ts config/scripts/package-electron-runtime-contract.test.mjs— 53 passed.pnpm run typecheck:node, targeted oxlint, oxfmt check, 44 reliability gates, and max-lines ratchet pass..versionmatches its recomputed content hash.AI Review Report
Three merge-base-anchored adversarial rounds reviewed the complete diff and the real Windows evidence. Review covered dependency ownership, exact upstream release contents, old-Windows/WinPTY behavior, cmd/PowerShell shells, ARM64/x64 packaging, process-tree cleanup, Node/Electron ABI stability, reconnect/install recovery, source drift, macOS/Linux no-op behavior, and packaging contents.
The first rounds flagged two hardening opportunities that were implemented: validate the entire published helper by exact hashes, and replace it atomically so an interrupted remote install remains recoverable. The final round found no remaining in-scope issues. There is no terminal hot-path change; reconnect adds only an ~800-byte helper hash inside the existing native-dependency probe.
A
node-ptybeta bump was rejected because beta.11 spans 108 commits/222 files, removes the WinPTY fallback, weakens this failure fallback to[], and predates later ConPTY lifecycle/leak fixes. Forcing bundled ConPTY was also rejected because it changes backend and process-tree cleanup semantics.Cross-platform review confirmed the patch/install command is Windows-gated, the asset is shipped only in Windows relay outputs, local terminals are unchanged, and macOS/Linux relay dependency installation remains unchanged.
Security Audit
node-pty@1.1.0original/patched source hashes and fails closed on version or source drift.uncaughtExceptionhandler.Notes
The reproduction corrects part of the issue report's causal chain: the console-list helper is forked during PTY kill, and its failure does not itself crash the relay/socket. The baseline relay remained alive and the bridge exited successfully. This PR fixes the verified missing relay patch and AttachConsole error; it does not claim that this helper exception independently explains every reported client startup failure.