Track terminal listening ports in activity events and thread state#43

Windows only collects direct children, not the full process tree

collectWindowsChildPids queries Win32_Process with ParentProcessId = ${terminalPid}, which only returns direct child processes. In contrast, collectPosixProcessFamilyPids recursively walks the entire process tree (BFS traversal building processFamily). This means on Windows, if a terminal runs npm start → node server.js → listens on port 3000, the port won't be detected because node is a grandchild.

Consider recursively collecting descendant PIDs on Windows as well — for example by iterating the initial children and querying their children in a loop, similar to the POSIX BFS approach.

lsof exit code 1 unnecessarily falls through to ss

lsof returns exit code 1 when no matching files/connections are found (i.e., none of the specified PIDs have listening sockets). This is a normal "no results" outcome, not a failure. With the current logic, result.code === 0 will be false, causing an unnecessary fallback to ss which will also find nothing. The ss fallback should only be needed when lsof is unavailable (the catch path).

Consider also returning early when result.code === 1:

src/terminalManager.ts:1000

Consider throttling or batching the subprocess activity checks. Running subprocessInspector concurrently for all sessions via Promise.all spawns heavy external processes (e.g., powershell.exe, ps) in parallel every polling interval, which could cause resource pressure with many open terminals.

No longer relevant as of 05a7018f0302121cfc15ce9bf2db72498f5e09d4

src/terminalManager.ts:371 On Windows, collectWindowsChildPids only retrieves immediate children, unlike the POSIX path which recursively collects the entire process family. Consider implementing recursive traversal (similar to collectPosixProcessFamilyPids) to detect ports opened by grandchild processes.

Code refactored as of 259251c84327b88fcc0bc31bb6b6be9212f2799b

⚠️ Potential issue | 🔴 Critical

Ensure terminal events are validated against the Zod schema before being processed.

The code accesses action.event.runningPorts without defensive handling, but more critically, events flowing from the WebSocket are not validated against terminalEventSchema before reaching the reducer. This violates the requirement to use Zod schemas from packages/contracts for type contracts. Even though the schema defines runningPorts with a .default([]), that validation never executes in the current implementation. Add validation in the event handler (App.tsx or wsNativeApi.ts) to parse events through the appropriate Zod schema before dispatch.

⚠️ Potential issue | 🟡 Minor

Incorrect callback signature for server.listen().

The listen callback receives no arguments. Errors during binding are emitted via the 'error' event on the server, not passed to the callback. While this doesn't break the test (the parameter is simply unused), the signature is misleading.

src/terminalManager.ts:461 Calling res.destroy() prevents the 'end' event from firing, so the promise only resolves when the fallback timer fires (~550ms later). Consider calling settle() immediately after res.destroy().

Code refactored as of 259251c84327b88fcc0bc31bb6b6be9212f2799b

🟢 Low

terminalProcessInspector/win32.ts:21 When stdoutTruncated is true, the last PID may be partially cut off and parsed as a different valid number. Consider discarding the final line when truncation occurs, or switching to outputMode: "error" to reject truncated output entirely.

🚀 Want me to fix this? Reply ex: "fix it for me".

⚠️ Potential issue | 🟡 Minor

Verify Node.js version requirement for toSorted or add runtime compatibility fallback.

The code targets ES2023 with toSorted, which requires Node.js 20+. However, package.json engines specifies only bun: >=1.3.9 with no Node.js constraint. In production, this runs on node dist/index.mjs and will fail at runtime on Node < 20 with "toSorted is not a function", violating predictable behavior requirements.

Either add "node": ">=20" to the engines field or use .slice().sort(...) for compatibility:

⚠️ Potential issue | 🟠 Major

Fall back to ss on lsof failures other than "no results".

Currently, any non-zero lsof exit returns [] and skips the ss fallback. The comment states that exit code 1 means "no matching files/sockets", but lsof exit code 1 actually indicates broader search failures—it can mean no matches, failed to locate requested items, or incomplete results. Other exit codes indicate errors like lsof being unavailable. Treat exit code 1 as a "no results" outcome and fall through to ss for other errors.

⚠️ Potential issue | 🟠 Major

Settle truncated responses immediately instead of waiting for timeout.

When the body cap is reached, res.destroy() is called without settling. Since Node.js HTTP responses do not emit the 'end' event after destroy(), the settle() function is never called for truncated responses. The promise then waits for the fallback timeout (550ms), causing large HTML responses to be misclassified as failures. Settle with the partial response before destroying.

src/terminalManager.ts:844

Stale update risk: after await this.detectWebPorts(...), the session may have restarted. Suggest re-checking that liveSession is still current (e.g., status === 'running' && pid === terminalPid) before updating subprocess state or emitting the activity event.

No longer relevant as of 05a7018f0302121cfc15ce9bf2db72498f5e09d4

⚠️ Potential issue | 🟡 Minor

Remove redundant normalizeRunningPorts() call at line 841.

The double normalization is unnecessary. The inner call at line 842 normalizes activity.runningPorts, then detectWebPorts() returns an already-sorted array via .toSorted(), making the outer normalizeRunningPorts() at line 841 redundant. Simplify to: await this.detectWebPorts(normalizeRunningPorts(activity.runningPorts)).

src/terminal.ts:92

.default([]) shares the same array instance across all parsed objects. Consider using .default(() => []) to return a fresh array each time.

No longer relevant as of 05a7018f0302121cfc15ce9bf2db72498f5e09d4

src/terminalManager.ts:839 The detectWebPorts call on line 842 isn't wrapped in a try/catch like the subprocessInspector call above. If it throws, the unhandled rejection could crash the process. Consider wrapping it similarly.

✅ Addressed in c11027be4d9183c9028f91bf68c0abb5934b103b

🟢 Low

https://github.com/pingdotgg/t3code/blob/05a7018f0302121cfc15ce9bf2db72498f5e09d4/apps/web/src/components/ChatView.tsx#L435

selectThreadTerminalState returns raw persisted ThreadTerminalState without normalization, so when old persisted data lacks runningTerminalPorts, terminalState.runningTerminalPorts is undefined. This undefined is then passed directly to ThreadTerminalDrawer at line 562, likely causing runtime errors when the component expects a Record<string, number[]>. The persist middleware has no migration to add the missing field to existing data.

✅ Resolved in 037a7719fa567006c0ddafe4137a7b162326593e

Triplicated normalizeRunningPorts across web app files

Low Severity

normalizeRunningPorts is independently defined with near-identical logic in Sidebar.tsx, ThreadTerminalDrawer.tsx, and terminalStateStore.ts. All three deduplicate, filter, and sort port arrays the same way. If the normalization rules change (e.g., port range), three places need updating, risking inconsistency.

^{Reviewed by Cursor Bugbot for commit 05a7018f0302121cfc15ce9bf2db72498f5e09d4. Configure here.}

Exported subprocessCheckerToInspector is never imported or used

Low Severity

subprocessCheckerToInspector is exported from the terminalProcessInspector module but is never imported anywhere in the codebase. Manager.ts (the sole consumer of this module) implements the same adapter inline when options.subprocessChecker is provided. This is dead code that also pulls in the unused TerminalSubprocessChecker type from types.ts.

^{Reviewed by Cursor Bugbot for commit 05a7018f0302121cfc15ce9bf2db72498f5e09d4. Configure here.}

Windows misses grandchild ports unlike POSIX full-tree traversal

Medium Severity

collectWindowsChildPids only queries direct children via ParentProcessId = ${terminalPid}, while collectPosixProcessFamilyPids performs a full BFS traversal to collect the entire descendant tree. In a typical dev workflow (e.g., terminal → npm → node listening on port 3000), the Windows path misses the grandchild node process, so checkWindowsListeningPorts won't detect port 3000. The PR description states it "detects process families" on both platforms, but only POSIX actually walks the tree.

^{Reviewed by Cursor Bugbot for commit 05a7018f0302121cfc15ce9bf2db72498f5e09d4. Configure here.}

🟢 Low terminalProcessInspector/index.test.ts:20

If the effect returned by stopProcess is interrupted before the process exits, the setTimeout at line 28 keeps running and the exit listener at line 34 remains attached — the cleanup function at line 86 isn't returned, so interruption leaks resources. Consider returning Effect.sync(cleanup) so interruption cancels the timeout and removes the listener.

No longer relevant as of bc30ac51f5dd298fd122031d1a70989b6b6a242f

🟢 Low src/webPortInspector.test.ts:24

The server.listen callback signature is wrong: Node.js passes no arguments to the success callback, so error?: Error will always be undefined. Listen failures (e.g., port already in use) emit an 'error' event instead, which this code doesn't handle. The Effect will hang forever on listen failures because resume is never invoked.

No longer relevant as of 6aac79caff618d7affa32076be551dea38b49819

Web port probe cache grows without bound

Low Severity

The webPortProbeCache map is never pruned — entries are added on each probe but never removed, even when their TTL expires. Over a long-running server session with many ephemeral dev-server ports, stale entries accumulate indefinitely. Adding eviction of expired entries during cache reads or periodically would keep memory bounded.

^{Reviewed by Cursor Bugbot for commit bc30ac51f5dd298fd122031d1a70989b6b6a242f. Configure here.}

extraWebPortCount computed but never used in rendering

Low Severity

The TerminalRuntimeStatus interface declares extraWebPortCount (line 81) and terminalRuntimeStatus computes it (line 102), but it's never consumed in the rendering code. The runtimeStatus object is only accessed for primaryWebPort and label in the JSX, making extraWebPortCount dead code.

^{Reviewed by Cursor Bugbot for commit 6aac79caff618d7affa32076be551dea38b49819. Configure here.}

Timeout errors double-wrapped by subsequent mapError

Medium Severity

The Effect.mapError at line 130 catches all errors flowing through the pipeline, including the WebPortInspectionError already created by the timeout handler at line 120–126. When a probe times out, the error with detail "HTTP probe timed out." gets wrapped again into a new WebPortInspectionError with detail "Failed to execute HTTP probe request.", burying the original message in cause. This obscures the real failure reason in logs and error reporting.

^{Reviewed by Cursor Bugbot for commit 6aac79caff618d7affa32076be551dea38b49819. Configure here.}

Windows subprocess inspection misses grandchild processes unlike POSIX

Medium Severity

collectWindowsChildPids only queries direct children via ParentProcessId = ${terminalPid}, while collectPosixProcessFamilyPids performs a full BFS traversal of the process tree. On Windows, grandchild processes (e.g., shell → npm → node listening on a port) won't be detected for subprocess activity or port scanning, causing inconsistent behavior across platforms.

^{Reviewed by Cursor Bugbot for commit 0236cc845e0621a281ad3cac36c5bd341594236f. Configure here.}

Negative web port probe results are cached for TTL

Low Severity

inspectWebPortCached catches all webPortInspector failures and caches the result as isWeb: false for the full TTL (default 10 seconds). If a web server is detected as listening but its first HTTP probe fails transiently (e.g., brief startup race, temporary overload), the port won't be classified as a web port until the cache entry expires — even though subsequent probes would succeed.

^{Reviewed by Cursor Bugbot for commit da81a167eac8b111b129cc7ed989b0b7c8f62dde. Configure here.}

Duplicated normalizeRunningPorts across server and web packages

Low Severity

normalizeRunningPorts is implemented twice with nearly identical logic — once in apps/server/src/process/utils.ts and again in apps/web/src/terminalStateStore.ts. Both deduplicate, filter invalid ports, and sort. Since the @t3tools/contracts package already defines TerminalPortSchema with the same validation range (1–65535), this shared utility belongs there to avoid divergence over time.

^{Reviewed by Cursor Bugbot for commit da81a167eac8b111b129cc7ed989b0b7c8f62dde. Configure here.}

Duplicated port label construction in sidebar and drawer

Low Severity

The port label format string logic (including the ternary for single vs. multiple ports and the http://localhost:${port} template) is duplicated across terminalRuntimeStatus in ThreadTerminalDrawer.tsx and terminalStatusFromTerminalState in Sidebar.tsx. If the label format needs to change, both locations must be updated in lockstep.

^{Reviewed by Cursor Bugbot for commit da81a167eac8b111b129cc7ed989b0b7c8f62dde. Configure here.}

🟢 Low components/ThreadTerminalDrawer.tsx:1394

The close terminal button's XIcon is placed outside the render prop's button element, inside PopoverTrigger instead. This renders an empty button with no visible icon. Move <XIcon className="size-2.5" /> inside the button element in the render prop, following the pattern used for the Globe and TerminalSquare icons on lines 1369-1378 and 1344-1351.