Windows · Re-reviewed · Changes requested
The original fallback fix passes, but degraded discovery still loses state
150 focused TypeScript tests and 18 Rust tests pass on Windows x64. The two original regression cases now pass, as does successful fallback pacing. Two additional cases fail: native results are not retained across discovery failure, and a slow failed probe can consume its cooldown before completing. Keep changes requested for those remaining paths.
Two reproduced edge cases remain
Source: focused Vitest and cargo output, plus five review cases in the evidence bundle.
Request changes. The native happy path works. Retain authoritative native state and measure failure cooldown from completion before approval.
PR #9520, head b58b4cbc43edf9a73f95a388be9ea6dc2bcc8c85, base bde39d4d7977ce85d6ea396a983d6b6a25bf7e07. The prior head was c27fa4e1b131311051d609a344ca2c6f5e7262a2. This review checks both the fixes since that head and the resulting diff against the new base. #9476 is already merged and its reviewed head has not changed.
Numbered findings track the prior review
1. The original result-validation finding is resolved
The scanner now rejects nonzero, timed-out, and truncated results before replacing a valid fallback snapshot. Both original regression cases pass. The request lifecycle helper is extracted. Successful fallback pacing also passes a new test: a 5-second query waits 20 seconds from completion.
Source: result classification, review cases, and previous review.
2. Retain native results when both discovery paths fail
P2 · reproduced The native branch returns its result without saving it. The retained state belongs only to PowerShell. The reproduction first finds a healthy native listener on port 43123, then makes native and PowerShell discovery fail. The next scan returns zero servers because common-port probing cannot rediscover that non-common port.
Retain the latest authoritative result from either discovery path, including a successful empty result. Apply current terminal owners when serving retained data. This is a remaining PR defect, not a claim that the latest fix introduced it.
Source: native selection, degraded fallback, failing native-transition review case.
3. Start failure cooldown when the probe completes
P2 · reproduced The failure branch adds its delay to the pre-launch timestamp. A 15-second failure consumes the initial 3-, 6-, or 12-second cooldown. Two immediate scans launch PowerShell twice in the reproduction. The background loop still has its ordinary 3-second interval, so this is an expired failure delay rather than an unbounded busy loop.
Read the completion time before storing the failure deadline, as the successful path already does. The increased timeout makes this path relevant to the slow-machine scenario the update addresses.
Source: failure deadline, slow-failure review case with a 15-second simulated runner.
4. Distinguish helper tests from sidecar restart integration
Nonblocking The four new tests directly exercise the shared helper. The restart test manually fails a Deferred and then runs another helper call. It does not launch or replace a sidecar, dispatch protocol responses, or exercise production pending-map draining. Add that coverage or narrow the post-restart testing claim.
Source: new request helper tests.
Preserve state at the discovery boundary
Recommended state boundary
Source: review recommendation based on PortScanner state and selection paths. The shared retention boundary is proposed, not implemented by this review.
The protocol remains v4, with request IDs correlating responses. The native Rust source is unchanged from the prior reviewed head. The new shared request helper removes repeated timeout and finalizer code. The requested corrections belong in scanner state and retry deadlines, without changing the native command format.
Windows checks support the scoped findings
| Check | Result | Evidence |
|---|---|---|
| Original two regression cases | Pass | Failed/truncated PowerShell results retain valid fallback state |
| New success pacing case | Pass | 5-second probe waits 20 seconds after completion |
| Native-to-fallback transition | Fail | Known port 43123 disappears after both discovery paths fail |
| Slow failed fallback cooldown | Fail | 15-second failure permits an immediate second launch |
| Focused TypeScript suite | 150 passed | 7 files, terminal, preview, telemetry and diagnostics |
| Rust tests / formatting / release build | 18 passed | Windows x64 native executable |
| bun fmt / bun lint / bun typecheck | Pass | Clean PR head; existing lint warnings and type suggestions |
| Native protocol probes | Pass | 12 process tables; 6 IPv4/IPv6 listener snapshots; removal after close |
| Live PTY and RPC | Pass | Activity, ownership, Ctrl+C, listener removal |
| Windows computer use | Pass, limited smoke | App loads and terminal drawer opens in review worktree |
Source: downloadable evidence bundle. Counts exclude skipped cases.
Source: Windows computer-use capture; browser chrome cropped. Commands and lifecycle assertions ran separately through public RPC.
Confidence is high in the two deterministic failure reproductions and the tested native path. The initial live probe timed out while the dev watcher restarted; a stable retry passed. An initial typecheck overlapped temporary review fixtures and reported fixture-only fetch typing errors; the clean-head rerun passed. The browser briefly reconnected, then opened the correct worktree terminal.
The prior Windows PTY restart/close crash remains a separate follow-up and was not re-exercised. No installer, ARM64, POSIX, sustained CPU comparison, or long soak was run. The browser smoke and RPC checks do not establish full frontend or packaging acceptance.
Evidence is pinned to the reviewed commit
The bundle contains focused checks, regression cases, native probes and live results. Credentials, private app state, and dev-server logs are excluded. Copy the review test beside PortScanner.test.ts in the pinned checkout and run bun run test src/preview/PortScanner.rereview.test.ts -t 'review:' from apps/server, then remove the temporary file before clean-head checks. Live RPC probes require a freshly issued local test credential.
tests.log, full 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-9520/apps/server
(node:52120) 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 150 passed (150)
Start at 13:35:06
Duration 24.85s (transform 7.03s, setup 1.33s, import 17.94s, tests 2.68s, environment 1ms)
regressions.log, full output
$ vp test run src/preview/PortScanner.rereview.test.ts -t review:
RUN v4.1.11 E:/Projects/t3code-review-9520/apps/server
stdout | src/preview/PortScanner.rereview.test.ts > review: slow failed fallback waits after completion
[13:40:53.288] ERROR (#48): Error [AssertionError]: expected 2 to be 1 // Object.is equality
at Array.<anonymous> (E:/Projects/t3code-review-9520/apps/server/src/preview/PortScanner.rereview.test.ts:1015:18) {
actual: 2,
expected: 1,
showDiff: true,
operator: 'strictEqual',
[cause]: undefined
}
stdout | src/preview/PortScanner.rereview.test.ts > review: native snapshot survives unavailable fallback
[13:40:53.297] ERROR (#96): Error [AssertionError]: expected [] to have a length of 1 but got +0
at Proxy.<anonymous> (file:///E:/Projects/t3code-review-9520/node_modules/.pnpm/@vitest+expect@4.1.11/node_modules/@vitest/expect/dist/index.js:1389:20)
at Proxy.<anonymous> (file:///E:/Projects/t3code-review-9520/node_modules/.pnpm/@vitest+expect@4.1.11/node_modules/@vitest/expect/dist/index.js:1156:15)
at Proxy.methodWrapper (file:///E:/Projects/t3code-review-9520/node_modules/.pnpm/chai@6.2.2/node_modules/chai/index.js:1700:25)
at Array.<anonymous> (E:/Projects/t3code-review-9520/apps/server/src/preview/PortScanner.rereview.test.ts:1031:35) {
actual: 0,
expected: 1,
showDiff: true,
[cause]: undefined
}
❯ src/preview/PortScanner.rereview.test.ts (31 tests | 2 failed | 26 skipped) 69ms
× review: slow failed fallback waits after completion 16ms
× review: native snapshot survives unavailable fallback 6ms
⎯⎯⎯⎯⎯⎯⎯ Failed Tests 2 ⎯⎯⎯⎯⎯⎯⎯
FAIL src/preview/PortScanner.rereview.test.ts > review: slow failed fallback waits after completion
AssertionError: expected 2 to be 1 // Object.is equality
- Expected
+ Received
- 1
+ 2
❯ Array.<anonymous> src/preview/PortScanner.rereview.test.ts:1015:18
1013| yield* scanner.scan();
1014| yield* scanner.scan();
1015| expect(runs).toBe(1);
| ^
1016| }).pipe(Effect.provide(layer));
1017| });
❯ Object.~effect/Effect/successCont ../../node_modules/.pnpm/effect@4.0.0-rc.112_patch_h_a125b9c38e631202ae4829151783d82d/node_modules/effect/src/internal/effect.ts:1365:25
❯ Object.~effect/Effect/evaluate ../../node_modules/.pnpm/effect@4.0.0-rc.112_patch_h_a125b9c38e631202ae4829151783d82d/node_modules/effect/src/internal/core.ts:514:30
❯ FiberImpl.runLoop ../../node_modules/.pnpm/effect@4.0.0-rc.112_patch_h_a125b9c38e631202ae4829151783d82d/node_modules/effect/src/internal/effect.ts:655:39
❯ FiberImpl.evaluate ../../node_modules/.pnpm/effect@4.0.0-rc.112_patch_h_a125b9c38e631202ae4829151783d82d/node_modules/effect/src/internal/effect.ts:607:22
❯ Array.<anonymous> ../../node_modules/.pnpm/effect@4.0.0-rc.112_patch_h_a125b9c38e631202ae4829151783d82d/node_modules/effect/src/internal/effect.ts:988:12
❯ MixedSchedulerDispatcher.runTasks ../../node_modules/.pnpm/effect@4.0.0-rc.112_patch_h_a125b9c38e631202ae4829151783d82d/node_modules/effect/src/Scheduler.ts:230:16
❯ Immediate.afterScheduled [as _onImmediate] ../../node_modules/.pnpm/effect@4.0.0-rc.112_patch_h_a125b9c38e631202ae4829151783d82d/node_modules/effect/src/Scheduler.ts:219:9
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[1/2]⎯
FAIL src/preview/PortScanner.rereview.test.ts > review: native snapshot survives unavailable fallback
AssertionError: expected [] to have a length of 1 but got +0
- Expected
+ Received
- 1
+ 0
❯ Array.<anonymous> src/preview/PortScanner.rereview.test.ts:1031:35
1029| const scanner = yield* PortScanner.PortDiscovery;
1030| expect(yield* scanner.scan()).toHaveLength(1);
1031| expect(yield* scanner.scan()).toHaveLength(1);
| ^
1032| }).pipe(Effect.provide(layer));
1033| });
❯ Object.~effect/Effect/successCont ../../node_modules/.pnpm/effect@4.0.0-rc.112_patch_h_a125b9c38e631202ae4829151783d82d/node_modules/effect/src/internal/effect.ts:1365:25
❯ Object.~effect/Effect/evaluate ../../node_modules/.pnpm/effect@4.0.0-rc.112_patch_h_a125b9c38e631202ae4829151783d82d/node_modules/effect/src/internal/core.ts:514:30
❯ FiberImpl.runLoop ../../node_modules/.pnpm/effect@4.0.0-rc.112_patch_h_a125b9c38e631202ae4829151783d82d/node_modules/effect/src/internal/effect.ts:655:39
❯ FiberImpl.evaluate ../../node_modules/.pnpm/effect@4.0.0-rc.112_patch_h_a125b9c38e631202ae4829151783d82d/node_modules/effect/src/internal/effect.ts:607:22
❯ ../../node_modules/.pnpm/effect@4.0.0-rc.112_patch_h_a125b9c38e631202ae4829151783d82d/node_modules/effect/src/internal/effect.ts:1121:14
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[2/2]⎯
Test Files 1 failed (1)
Tests 2 failed | 3 passed | 26 skipped (31)
Start at 13:40:50
Duration 3.17s (transform 761ms, setup 195ms, import 2.17s, tests 69ms, environment 0ms)
error: script "test" exited with code 1
rust-tests.log, full output
Finished `test` profile [unoptimized + debuginfo] target(s) in 1.59s
Running unittests src\main.rs (native\resource-monitor\target\debug\deps\t3_resource_monitor-fed045c766d21202.exe)
running 18 tests
test tests::accepts_clock_shifts_without_accepting_reused_process_starts ... ok
test tests::counts_selected_processes_that_could_not_be_materialized ... ok
test tests::excludes_and_trims_future_history_after_the_clock_moves_backward ... ok
test tests::bounds_history_by_estimated_process_bytes ... ok
test tests::counts_external_processes_toward_history_limits ... ok
test tests::clamps_sample_interval ... ok
test tests::ignores_missing_roots ... ok
test tests::decodes_protocol_commands ... ok
test tests::validates_external_process_start_identity ... ok
test tests::rejects_descendants_older_than_a_reused_parent_pid ... ok
test tests::sample_now_does_not_postpone_an_existing_periodic_deadline ... ok
test tests::truncates_process_strings_at_utf8_boundaries ... ok
test tests::refreshes_commands_without_enumerating_linux_tasks ... ok
test tests::selects_roots_and_all_descendants ... ok
test tests::waits_for_a_cpu_measurement_window_after_priming ... ok
test tests::reads_windows_listener_table ... ok
test tests::retains_bounded_history_without_request_ids ... ok
test tests::loads_details_when_an_existing_process_becomes_selected ... ok
test result: ok. 18 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.35s
native.json, full output
{
"version": 4,
"platform": "win32",
"architecture": "x64",
"node": "v24.13.1",
"requests": [
{
"type": "processTable",
"ms": 47.682,
"count": 482
},
{
"type": "processTable",
"ms": 22.951,
"count": 482
},
{
"type": "processTable",
"ms": 24.626,
"count": 482
},
{
"type": "processTable",
"ms": 23.604,
"count": 482
},
{
"type": "processTable",
"ms": 23.407,
"count": 482
},
{
"type": "processTable",
"ms": 17.504,
"count": 482
},
{
"type": "processTable",
"ms": 20.212,
"count": 482
},
{
"type": "processTable",
"ms": 20.847,
"count": 482
},
{
"type": "processTable",
"ms": 25.75,
"count": 482
},
{
"type": "processTable",
"ms": 24.042,
"count": 482
},
{
"type": "processTable",
"ms": 21.321,
"count": 482
},
{
"type": "processTable",
"ms": 21.744,
"count": 482
},
{
"type": "windowsListeners",
"ms": 22.778,
"count": 41
},
{
"type": "windowsListeners",
"ms": 23.516,
"count": 41
},
{
"type": "windowsListeners",
"ms": 24.324,
"count": 41
},
{
"type": "windowsListeners",
"ms": 22.361,
"count": 41
},
{
"type": "windowsListeners",
"ms": 26.701,
"count": 41
},
{
"type": "windowsListeners",
"ms": 27.233,
"count": 41
}
],
"checks": [
"12 full process tables include the sidecar with its correct parent",
"IPv4 and IPv6 loopback listeners found with correct PID and process name in 6 snapshots",
"Closed listeners disappear on the next native snapshot"
],
"pass": true,
"stderr": "",
"unsolicitedEvents": [
"hello"
]
}live-9520.json, full output
{
"number": "9520",
"checks": [
{
"name": "real Windows PTY opened",
"status": "running"
},
{
"name": "running child updates terminal activity",
"label": "node"
},
{
"name": "HTTP fixture discovered with terminal ownership",
"port": 19520,
"processName": "node.exe"
},
{
"name": "stopped child returns terminal to idle"
},
{
"name": "closed listener disappears from discovery"
}
],
"pass": true
}Generated 2026-09-08 · Codex review for Dara · Windows x64, Node v24.13.1 · b58b4cbc43edf9a73f95a388be9ea6dc2bcc8c85 · revision v2