Windows · Reviewed · PR 9476
The sidecar preserves terminal activity without recurring process probes
139 focused TypeScript tests and 17 Rust tests pass on Windows x64. Twelve native process tables contain the correct parent relationship, and a real terminal follows Node activity through start and stop. The live server associates its HTTP listener with the terminal and removes the closed port. No blocking defect was found in the polling change; rapid terminal close exposes a separately reproduced, pre-existing PTY crash.
The review decision follows the changed behavior
Source: focused TypeScript and Rust results, extra review tests, native request probe. Latency is a local sample, not a CPU benchmark.
Approve the polling change. The tested native process-table path preserves terminal behavior. Track the unrelated Windows PTY crash separately.
Reviewed PR #9476 at e298346f84613e632cabb21c55b4e268d786cdee, against common base 761d4bac1c238ea7af4dd36b56719ad5e30771c3. The dedicated detached worktree isolates this head from the current working branch.
Numbered findings distinguish regressions from follow-ups
1. The polling change is ready for approval
Verified · within scope The manager shares a single process snapshot across terminal sessions. Native failure falls back to the existing process probe while still increasing the retry delay. Success resets the failure counter. The dedicated sysinfo::System prevents process-table reads from resetting the telemetry CPU baseline.
Native process ancestry and live start, stop, ownership, and port removal all passed. Existing terminal tests cover shared snapshots and fallback backoff. The source keeps the protocol bump aligned between contracts and the Rust executable.
Source: Manager.ts, Rust process-table collector, focused tests and native/live evidence below.
2. Track the existing Windows PTY close crash separately
Follow-up · reproduced Rapid restart followed by close crashes the server with Signals not supported on windows. The unchanged adapter forwards SIGTERM to node-pty@1.1.0. Before its socket is ready, node-pty defers that call and throws later, outside the manager's synchronous error handling.
A standalone reproduction using the same dependency exits with code 1. The adapter and dependency lockfile are unchanged from the common base. The manager's explicit signal path also predates these changes. Use the Windows PTY termination API without POSIX signals and cover close-before-ready in a separate fix. The operation acknowledgements passed before cleanup, so they must not be described as a clean shutdown test.
Source: baseline NodePtyAdapter.kill and Manager kill escalation; standalone reproduction and live-server crash on both PRs.
3. Tighten request lifecycle tests
Nonblocking The new request method repeats the existing deferred-map, timeout, and finalizer sequence. A small shared request helper would reduce drift if more sidecar commands are planned. Add direct request/response, timeout, interruption, and sidecar-restart coverage for the new commands. The existing NativeTelemetryClient suite passes, but this PR does not add tests to that file.
Source: NativeTelemetryClient.ts and the PR file list.
The design keeps process discovery inside the sidecar
Process-table request path
Source: reviewed manager/scanner, NativeTelemetryClient and native/resource-monitor at the pinned head.
Protocol v3 adds processTable. The sidecar enumerates process IDs, parent IDs and names using a dedicated System. The client correlates each reply with its request. The manager derives all terminal descendants from one shared table, then publishes activity and registers process ownership. A failed native read can still supply fallback data while extending the next poll delay.
Keep native binary and contract versions aligned in release packaging. A mismatched or unavailable binary must take the fallback path without corrupting the last authoritative state. Scope remains server-side; this review did not change product code.
Windows execution supports the verdict
| Check | Result | Evidence boundary |
|---|---|---|
| bun fmt | Pass | No tracked formatting changes |
| bun lint | Pass | Existing warnings remain; exit 0 |
| bun typecheck | Pass | 15 workspace tasks; exit 0 |
| Focused TypeScript suite | 139 passed | 7 files, including terminal, preview, telemetry and diagnostics |
| Rust tests and release build | 17 passed | Native Windows x64; cargo fmt --check also passes |
| Native protocol probe | Pass | 12 tables; correct sidecar PID and parent; no invalid zero PID |
| Live server / real PTY | Activity and discovery pass | Node label, terminal ownership, Ctrl+C idle, port removal, restart acknowledgement |
| Rapid restart / close | Existing crash reproduced | Separate PTY signal path; not a clean shutdown pass |
| Windows UI smoke | Pass | Paired local app, added isolated worktree, opened terminal drawer |
Source: commands and raw outputs in the downloadable evidence bundle.
The isolated app opens its Windows terminal
The browser smoke test used Windows computer use. It completed local setup, added only the review worktree, and opened its PowerShell terminal drawer. Child-process commands ran through the test server RPC for the live checks.
Source: Windows computer-use capture of localhost:5910; browser chrome cropped from the evidence.
Confidence is high for the tested Windows x64 native path and the reproduced failure cases. No macOS, Linux or Windows ARM64 run was performed. Installer packaging, a sustained CPU/fan comparison, and a long soak were not measured. The UI smoke covers the local web client; it is not an Electron installer acceptance test.
The browser extension could not attach through its Firefox compatibility layer, and its emulated tab failed in a matchMedia listener. An ordinary tab operated through Windows computer use loaded successfully. These tooling errors are excluded from PR findings.
Raw evidence makes the checks reproducible
The bundle includes the native probe, live RPC test, focused test outputs and standalone PTY reproduction. Credentials and dev-server logs are excluded. The live RPC script needs a freshly issued local test credential; no token is distributed.
native-9476.json · full captured output
{
"version": 3,
"platform": "win32",
"architecture": "x64",
"node": "v24.13.1",
"requests": [
{
"type": "processTable",
"ms": 57.92,
"count": 414
},
{
"type": "processTable",
"ms": 27.477,
"count": 414
},
{
"type": "processTable",
"ms": 23.417,
"count": 414
},
{
"type": "processTable",
"ms": 27.059,
"count": 414
},
{
"type": "processTable",
"ms": 25.491,
"count": 415
},
{
"type": "processTable",
"ms": 25.249,
"count": 415
},
{
"type": "processTable",
"ms": 23.241,
"count": 414
},
{
"type": "processTable",
"ms": 24.174,
"count": 414
},
{
"type": "processTable",
"ms": 24.783,
"count": 414
},
{
"type": "processTable",
"ms": 26.618,
"count": 414
},
{
"type": "processTable",
"ms": 21.942,
"count": 415
},
{
"type": "processTable",
"ms": 33.17,
"count": 416
}
],
"checks": [
"12 full process tables include the sidecar with its correct parent"
],
"pass": true,
"stderr": "",
"unsolicitedEvents": [
"hello"
]
}live-9476.json · full captured output
{
"number": "9476",
"checks": [
{
"name": "real Windows PTY opened",
"status": "running"
},
{
"name": "running child updates terminal activity",
"label": "node"
},
{
"name": "HTTP fixture discovered with terminal ownership",
"port": 19476,
"processName": "node"
},
{
"name": "stopped child returns terminal to idle"
},
{
"name": "closed listener disappears from discovery"
},
{
"name": "terminal restarts",
"status": "running",
"newPid": true
}
],
"pass": true,
"cleanup": "Server crashed after rapid restart then close with node-pty Signals not supported on windows. Reproduced independently in the unchanged baseline adapter/dependency path.",
"passScope": "Listed operation acknowledgements and observed activity/discovery transitions; not clean end-to-end shutdown."
}
tests-9476.log · full captured output
$ vp test run src/terminal/Manager.test.ts src/preview/PortScanner.test.ts src/resourceTelemetry/NativeTelemetryClient.test.ts src/resourceTelemetry/ResourceTelemetry.test.ts src/resourceTelemetry/ResourceTelemetryHistory.test.ts src/resourceTelemetry/Model.test.ts src/diagnostics/ProcessDiagnostics.test.ts
RUN v4.1.11 E:/Projects/t3code-review-9476/apps/server
(node:48160) ExperimentalWarning: SQLite is an experimental feature and might change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
Test Files 7 passed (7)
Tests 139 passed (139)
Start at 14:14:10
Duration 21.71s (transform 6.18s, setup 961ms, import 15.65s, tests 2.67s, environment 1ms)
pty-close-baseline.log · full captured output
PTY spawned, forwarding the SIGTERM used by unchanged NodePtyAdapter.kill
E:\Projects\t3code-review-9476\node_modules\.pnpm\node-pty@1.1.0\node_modules\node-pty\lib\windowsTerminal.js:151
throw new Error('Signals not supported on windows.');
^
Error: Signals not supported on windows.
at WindowsTerminal.<anonymous> (E:\Projects\t3code-review-9476\node_modules\.pnpm\node-pty@1.1.0\node_modules\node-pty\lib\windowsTerminal.js:151:23)
at Object.run (E:\Projects\t3code-review-9476\node_modules\.pnpm\node-pty@1.1.0\node_modules\node-pty\lib\windowsTerminal.js:166:50)
at E:\Projects\t3code-review-9476\node_modules\.pnpm\node-pty@1.1.0\node_modules\node-pty\lib\windowsTerminal.js:73:28
at Array.forEach (<anonymous>)
at Socket.<anonymous> (E:\Projects\t3code-review-9476\node_modules\.pnpm\node-pty@1.1.0\node_modules\node-pty\lib\windowsTerminal.js:68:38)
at Object.onceWrapper (node:events:623:26)
at Socket.emit (node:events:520:35)
at addChunk (node:internal/streams/readable:559:12)
at readableAddChunkPushByteMode (node:internal/streams/readable:510:3)
at Readable.push (node:internal/streams/readable:390:5)
Node.js v24.13.1
Generated 2026-09-05 · Codex review for Dara · Windows x64, Node v24.13.1 · commit e298346f84613e632cabb21c55b4e268d786cdee · revision v1