fix(terminal): release exited sessions without waiting for descendants

[Raghav-Bansal-15]

Terminal process and output resources were unbounded, and a read failure could close the output pipe while its child was still alive.

Retain bounded admission/output and close the read pipe only after actual process exit. Preserve the reviewer’s empty-argument fix and make its fixture failure legible.

Validation at 23db7498: 12 terminal tests and ktlint/detekt pass locally. The new premature-close assertion fails on the previous head. Current-head Windows validation passed.

Original and reviewer commits are preserved. Upstream workflow approval and required checks are separate gates; no merge or release.

Hosted validation: all current-head fork workflows passed at 23db7498: 🔨 Build & Test. Upstream approvals and the stated non-CI gates remain separate.

[kshivang]

@claude review this PR for correctness, cancellation/process lifecycle, bounded resource behavior and regression coverage against current dev.

[claude]

Claude finished @kshivang's task in 7m 8s —— View job

---

Review: PR #570 - Bound terminal sessions and clean up cancelled process creation

• Gather context (verified the diff is exactly origin/dev..HEAD for modules/boss-app-terminal)
• Review correctness of session/output bounds
• Review cancellation + process lifecycle
• Review bounded resource behavior
• Review regression coverage vs current dev
• Post review

---

This is a solid, carefully-reasoned change. Three things are genuinely well done and I want to name them before the findings, because they're the parts that are easy to get wrong:

• The delivered flag and the synchronized(lock) spanning admission→launch→registration. The comment at TerminalServiceImpl.kt:44 ("shutdown cannot overlook an admitted launch between process creation and registration") is correct, and close() deliberately does its join() outside the lock (:106-107) so there's no deadlock against a launch holding it. The claim at :61-62 - that withContext discards its result when the outer job was cancelled - is accurate, and the PausedDispatcher test pins exactly that race rather than approximating it.
• TerminalOutputBuffer.stream() never emits while holding lock (:69 is outside the synchronized block, and updates.first {} at :72 likewise). Suspending inside a monitor is the obvious trap here and it's avoided.
• BOSS_PROCESS_TOKEN stripping after the caller's overrides (TerminalSession.kt:105-107). The ordering is the whole point, ignoreCase covers Windows' case-insensitive env map, and keys.filter{} copies so there's no CME. This also implements a requirement ProcessTokenRegistry's KDoc already stated ("plugin runtimes should strip BOSS_PROCESS_TOKEN before launching unrelated subprocesses") and nothing enforced.

I could not run :boss-app-terminal:test, ktlintCheck or detekt in this sandbox (gradle invocation isn't permitted), so I'm relying on your report for those. I did verify statically that the new tests need no build.gradle.kts change: io.grpc.Status/ServerBuilder/StatusException arrive through api(libs.grpc.stub) in boss-ipc, grpc-netty reaches testRuntimeClasspath as a transitive implementation dependency so ServerBuilder.forPort(0) finds a provider, and src/test/java is compiled by the java plugin with TerminalTestProcess landing in build/classes/java/test - which is what makes codeSource.location a usable -cp with no dependencies.

---

1. High - the admission slot is released by the pump thread, not by process death

TerminalSession.kt:53-54 releases the slot (onStopped) and clears active only after the finally in startPump runs, which is reached only after reader.read() returns -1. With redirectErrorStream(true) (:100), every descendant inherits that pipe's write end. So an ordinary shell interaction - sleep 1000 & then exit, or npm run dev & - leaves the backgrounded child holding stdout open after the shell itself exits and is reparented away from process.descendants(). Then:

• reader.read() blocks for as long as the grandchild lives; active stays true; activeSlots is never released.
• closeSession → terminate() (TerminalServiceImpl.kt:143) does not release it either - it only destroys the observable tree, and the KDoc at TerminalSession.kt:76 already concedes detached descendants are out of scope.
• retain (:92-98) only evicts !active, so the entry is permanently unevictable too.

The user sees listSessions reporting isAlive=false while createSession returns RESOURCE_EXHAUSTED, and after 16 of these the service refuses every launch for the life of the process. That is the unbounded-session failure this PR sets out to fix, relocated one level down.

Note the two facts have to move together: the current active = false before onStopped() ordering is what keeps sessions bounded by historyLimit (if a slot could free while historyLimit entries still read active, retain would grow the map past its limit). So key both the release and active off process.onExit(), make the release idempotent, and consider closing process.inputStream in terminate() so the blocked read unblocks. That invariant deserves a comment either way - it's load-bearing and currently implicit.

Fix this →

2. Medium - every new validation guard surfaces as gRPC UNKNOWN with no description

All the new limits use require/check, which throw IllegalArgumentException/IllegalStateException. grpc-kotlin maps a non-Status throwable through Status.fromThrowable → UNKNOWN, and does not forward the message to the client. So:

Call	Client sees
createSession with cols = 5000 (TerminalSession.kt:99)	UNKNOWN, no reason
createSession with a bad command	success = false + errorMessage
createSession after close() (:86)	UNKNOWN, not UNAVAILABLE
sendInput over 64 KiB (:112)	UNKNOWN, no reason
sendInput to an unknown session	clean NOT_FOUND
resize with cols = 0 (:133)	UNKNOWN, no reason

The file already reaches for Status.X.asRuntimeException() where it matters, so this is an inconsistency rather than not knowing the mechanism. Two knock-ons worth deciding explicitly:

• resize previously accepted cols = 0 silently; it now errors. If any client sends 0 to mean "unset" - which createSession itself treats as "use the default" at TerminalSession.kt:97-98 - that's a divergence between the two RPCs' reading of the same field.
• logger.warn("Terminal launch failed: {}", failure.javaClass.simpleName) (:66) drops both the message and the stack trace. Withholding the message from the client is a reasonable call, but "IOException" alone in the server log is close to useless for "why won't my terminal start", and dev at least returned e.message. Keep the full exception in the log.

Suggest INVALID_ARGUMENT for the size/dimension guards and UNAVAILABLE for post-close(), and log failure rather than its class name.

Fix this →

3. Medium - sendInput's write is the one remaining unbounded wait

TerminalSession.send (:60-73) holds inputLock across write + flush. A child that never drains stdin fills the pipe buffer (~64 KiB) and the write blocks indefinitely, parking a Dispatchers.IO thread (default cap 64) and making every later sendInput for that session return RESOURCE_EXHAUSTED - indistinguishable from genuine concurrent-input contention. For a PR whose subject is bounding resources this is the conspicuous gap. A dedicated writer thread with a bounded queue, or at minimum a documented acknowledgement, would close it.

4. Medium - close() joins with no timeout, on a JVM shutdown hook

TerminalServiceMain.kt:91 registers close() as a shutdown hook, and close() does active.forEach { it.process.onExit().join() } (TerminalServiceImpl.kt:107) unbounded. destroyForcibly is SIGKILL on POSIX so this normally returns immediately, but a process wedged in uninterruptible I/O (NFS, a stuck FUSE mount) blocks the hook forever and shutdown hooks have no timeout - the JVM never exits. onExit().get(timeout) with a warn-and-continue is the safer shape; the repo already uses "warn once after ten seconds and keep waiting" in BrowserNativeDisposal, though notably not on a shutdown hook.

Also: close() is invoked twice (hook at :91 and finally at :97) and is only accidentally idempotent - the second call's filter { it.active } happens to be empty. Worth a line saying so, since nothing enforces it. The hook is also never removed.

5. Low/Medium - the service-wide lock is held across ProcessBuilder.start()

TerminalServiceImpl.kt:45-59 forks/execs inside synchronized(lock). fork/exec from a large JVM is milliseconds to tens of milliseconds, and while held, sendInput, streamOutput subscription, resize, closeSession, listSessions and close() all block in session(id). The atomicity requirement is real and correctly identified; the same invariant can be had by reserving the slot and inserting a placeholder under the lock, then launching outside it. As written, a burst of 16 launches serializes all terminal input.

6. Low - assorted

• TerminalOutputBuffer.kt:39: finished = chunk.isExit is an assignment, not a latch. It's only safe because check(!finished) at :34 guards it, so it reads as a bug even though it isn't. finished = finished || chunk.isExit costs nothing.
• TerminalOutputBuffer.kt:56: "reconnect to resume" promises something the wire format can't deliver - I checked terminal.proto, StreamOutputRequest carries only session_id, no cursor. Reconnecting restarts at the oldest retained chunk; the gap is permanent. Either reword to "reconnect to continue from the retained window" or add a from_sequence field.
• TerminalServiceImpl.kt:25: streamSlots = Semaphore(32) is hard-coded while the other two limits are constructor parameters, and it's global rather than per-session - so 32 readers on one session starve streamOutput for every other session. It's also the only limit with no test at all.
• TerminalServiceImpl.kt:73-80: on the cancellation path with pumping == true, the session is terminated but stays in sessions as an inactive history entry no client has an id for. Harmless (it's evictable, and the cancellation test relies on listSessions finding it), but a cancellation storm fills historyLimit with orphans.

7. Wire-behavior change with no consumer yet - worth writing down

I grepped for TerminalServiceGrpc: there is no in-repo client outside TerminalLimitsTest, so nothing breaks today. But sendInput/resize/streamOutput went from a logged no-op on an unknown session to NOT_FOUND, streamOutput went from an empty flow to a thrown status, and a slow reader now gets the stream terminated with OUT_OF_RANGE where dev dropped chunks silently. The KERNEL-mode consumer, when it lands, has to handle StatusException on all four. No proto change so no IpcVersion bump is needed, but a note in the PR body or beside the RPCs in terminal.proto would save the future client author the discovery.

---

Regression coverage

Real strengths: the PausedDispatcher test pins the precise race delivered exists for and proves slot release by launching a replacement under activeLimit = 1; retained.size == 2 pins the one-eviction-per-retain behavior (which is an incidental property of the loop's size re-check, so pinning it is right); using a real child JVM rather than a mock is the correct call for process lifecycle; and the slow-reader test's UNDISPATCHED + CompletableDeferred construction reliably produces the eviction-past-cursor state.

Not covered:

• streamSlots exhaustion (33rd reader → RESOURCE_EXHAUSTED). The only limit with zero coverage.
• The sendInput 64 KiB cap and the createSession 128 KiB cap.
• resize dimension validation and launch's cols <= 1000, including the cols = 0 divergence in finding 2.
• send to an exited session → FAILED_PRECONDITION; send under inputLock contention → RESOURCE_EXHAUSTED.
• NOT_FOUND on any of the four session-addressed RPCs.
• TerminalOutputBuffer happy path. Only the gap is tested. The batch-of-16 cursor arithmetic in stream() (:60-72) is the subtlest code in the PR and its in-order/no-loss behavior for a reader that keeps up is only covered incidentally through the late-subscriber tests. Also untested: the bytes > 1_048_576 vs entries.size > 256 eviction accounting (which is what keeps removeFirst() at :38 from hitting an empty deque), and that append after the exit chunk is rejected.
• Finding 1's scenario - a session whose grandchild holds stdout open still releasing its slot. Untested because it currently doesn't hold.
• close() called twice, and whether in-flight streamOutput collectors complete rather than hang across shutdown.

One test-hygiene note: awaitExit uses sessionsList.single { it.sessionId == id }, which throws NoSuchElementException if retain has already evicted the session. With historyLimit = 2 that holds today, but it couples a generic helper to the eviction policy - firstOrNull plus an explicit assertion would fail far more legibly when someone changes retain.

---

Findings 1 and 2 are the ones I'd want addressed before merge; 1 because it reintroduces the exhaustion this PR exists to prevent through an ordinary cmd & / exit, and 2 because a client currently cannot tell a rejected argument from a server fault. 3-7 are fine as follow-ups if you'd rather keep this extraction tight.
• branch fix/security-terminal-limits

[kshivang]

Claude findings disposition for @Raghav-Bansal-15's useful terminal limits/cancellation work:

• Fixed the inherited-output-pipe admission leak in 1caee55. The pump reads only currently available bytes while the shell lives and drains a bounded snapshot after shell exit. It then publishes exit, clears active and releases admission in the existing order. Releasing admission from onExit alone would leave unbounded blocked pump threads, so I preserved the pump-and-process accounting instead. The new real-process regression leaves a reparented descendant alive while asserting exit delivery and replacement admission under activeLimit=1.
• Fixed external validation statuses: oversized launch/input and invalid dimensions return INVALID_ARGUMENT, and closed service returns UNAVAILABLE. Tests exercise these through gRPC. Zero-sized resize remains rejected; creation retains its existing default-dimension convention.
• Changed the replay-gap text to say retained output, not promise cursor-based resumption. Byte chunks now preserve original process bytes rather than decoding/re-encoding split UTF-8 code points.
• Did not add raw IOException logging: launch commands/paths may contain private data, and the original sanitized diagnostic is intentional.
• The review's shutdown-hook assertion does not match this diff: TerminalServiceMain closes the service in finally, not in a newly registered hook. Uninterruptible OS process termination remains a limitation; adding a timeout that releases admission while work remains would weaken the bounds.
• A non-reading child can block its one admitted stdin write; inputLock prevents additional writers and max-16 active sessions bounds those writes. A cancellable input transport is a separate follow-up, not a reason to replace this focused patch with an unbounded queue.
• Lock-held launch is deliberate to prevent shutdown overlooking admitted children. Placeholder-based launch accounting is a possible later optimization, not a proven defect in the present invariant. The finished flag assignment is guarded by check(!finished); adding an OR latch is cosmetic.

Consumers must handle NOT_FOUND on unknown sessions, INVALID_ARGUMENT on invalid requests, RESOURCE_EXHAUSTED for admission/readers, FAILED_PRECONDITION on input after exit, UNAVAILABLE after close and OUT_OF_RANGE on replay gaps. The wire request has no resume cursor. The current in-repo tests are the only terminal gRPC consumers; this does not claim changes to the in-process terminal-tab plugin or full PTY support.

Original author commits and credit are preserved. Current repair validation and hosted CI remain pending and will be recorded separately; the earlier pre-repair seven tests and root ktlintCheck/detekt passed. No repeated Claude request is being made.

[kshivang]

@Raghav-Bansal-15 - thank you for the concrete resource-boundary work here: the bounded replay/admission design and deterministic cancellation-return regression address real service failure modes. Your original commits and contribution remain intact.

This is a validation/infrastructure handoff, not a claim that queued CI is an author defect. I addressed Claude's descendant-held-stdout and gRPC-status findings in 1caee55 (plus regression formatting eaa4e74), with the full reasoning and rejected suggestions in my prior disposition comment. The repair still needs execution evidence. The earlier integrated head 1ce165e passed all 7 terminal tests and root ktlintCheck/detekt; that does not validate these later repairs.

The shared local build lock did not admit the repaired-head invocation during the bounded review window, and hosted full CI remained queued after fork-run approval. I stopped only this group's queued wrapper after verifying it had no Gradle child, so it cannot later modify the worktree. No app/other worker was stopped. I am converting this open PR to draft until current-head validation is established; there is no merge or check bypass.

Required next steps:

1. On this branch run python3 /Users/kshivang/Development/Boss/.pr-review-sessions/with-build-lock.py ./gradlew :boss-app-terminal:ktlintFormat :boss-app-terminal:test ktlintCheck detekt --console=plain locally (or the corresponding tasks in isolated CI). Review/commit any formatter changes. The two added regressions must pass: parent exit while a descendant holds stdout still delivers exit and admits a replacement, and invalid requests return explicit gRPC statuses. Also retain all original tests.
2. Inspect the output-pump change on Linux, macOS and Windows; it must keep the pump/process admission bound and drain parent output without waiting for descendant EOF. Do not free slots while leaving blocked reader threads behind.
3. Require all required hosted checks to pass on the resulting exact head, refresh dev and reconcile any overlapping changes. fix(ipc): authenticate process calls and survive a dead repair client #575 has a separate ownership implementation that must be integrated deliberately.

Original and agent work are scored separately in the local inventory. This handoff does not downgrade the contributor for infrastructure queueing, but the original source findings remain part of the provisional original-submission assessment. Ready-for-review can be restored when the repair and CI have that evidence.

[Raghav-Bansal-15]

Review follow-up: Bound replay and admission, drain a bounded snapshot after shell exit, preserve output bytes, and return explicit request errors. Wrap the diagnostics and lifecycle assertions that failed static analysis; no checks or assertions were removed.

Validation: 9 terminal tests plus formatting/static analysis pass locally. Latest-head cross-platform CI is pending.

The PR is ready for review and the existing author/reviewer commits are retained. Upstream CI needs maintainer approval. I am following the latest-head checks and have not merged or released anything.

[kshivang]

@claude Please review the current revised head against dev, focusing on whether the prior lifecycle/native-platform findings are resolved and any remaining correctness regressions. One review request for this revision.

[claude]

Claude finished @kshivang's task in 8m 42s —— View job

---

Review: PR #570 revised head vs dev

• Gather context (diff is exactly origin/dev..HEAD, 778/165 across 8 files)
• Verify finding 1 (descendant-held stdout / admission leak) is resolved
• Verify finding 2 (gRPC statuses) is resolved
• Review the new pump loop for cross-platform / native behaviour
• Look for new correctness regressions in this revision
• Review regression coverage
• Post review

Sandbox limits, stated up front: gradle, javac and git fetch are all blocked in this runner, so I diffed against the already-fetched origin/dev and could not run :boss-app-terminal:test, ktlintCheck, detekt, or compile an empirical pipe-semantics probe. Every platform claim below is labelled reasoned (from OpenJDK / OS API semantics) vs verified in the diff. I'm relying on your report for the 9-test run.

---

Verdict on the prior findings

Finding 1 (admission leak behind a descendant-held stdout) is resolved, and resolved in the right place. The pump is now keyed on process.isAlive (TerminalSession.kt:34) rather than on EOF, so reader.read() can no longer park forever on a reparented grandchild's write end. Three things about the shape are correct and worth naming, because each was a way to get this wrong:

• The slot release and active = false stayed together, in the pump's finally (:66-67). Your disposition is right that releasing from onExit() alone would free admission while leaving a blocked reader thread behind - that would have bounded the semaphore and unbounded the threads, which is the same bug wearing a different hat. Keying the loop on liveness instead means the thread genuinely ends at the same moment the slot frees, so the pairing that keeps retain bounded (TerminalServiceImpl.kt:96-102 can only ever see ≤ activeLimit active entries) survives untouched.
• read(buffer, 0, minOf(available, buffer.size)) can never block, because this is the only reader, so available > 0 guarantees the read returns without waiting. The comment at :32-33 states exactly that precondition. Correct, and non-obvious enough to deserve the comment.
• The post-exit drain is bounded and snapshot-based (:44), so a chatty descendant cannot make shutdown of one session unbounded.

The regression test is a real one, not a proxy: an exited shell releases capacity while a descendant still holds stdout (TerminalLimitsTest.kt:236) spawns a genuine reparented grandchild via inheritIO(), asserts it is still alive, asserts the exit chunk still arrives, and then proves the slot was freed by launching a replacement under activeLimit = 1. That last step is the assertion that actually pins the bug. It also implicitly pins thread termination, since onStopped() and the end of the thread are the same statement.

An unstated benefit worth recording. On POSIX, OpenJDK's process reaper calls ProcessPipeInputStream.processExited(), which is synchronized on the same stream the pump reads. On dev, a pump blocked in read() held that monitor, so every session with a surviving descendant also wedged a process-reaper thread permanently. Letting the loop exit at process death releases that too. (Reasoned from OpenJDK ProcessImpl; not measured here.)

Finding 2 (opaque statuses) is resolved for the cases it named. INVALID_ARGUMENT on the 128 KiB request cap (TerminalServiceImpl.kt:42), the 64 KiB input cap (:116), resize dimensions (:139) and the launch dimension cap (TerminalSession.kt:114); UNAVAILABLE from reserveSlot (:90). All five are exercised end-to-end through a real gRPC channel (TerminalLimitsTest.kt:270, 216), which is the right level - require/check would have passed a unit test on the impl and still reached the client as UNKNOWN.

One thing that test pins without saying so, and which is the sharper half of it: the cols = 1001 rejection happens after reserveSlot() but before builder.start(), so the finally at :80-84 has to release the slot for a session that was never created. The subsequent start("wait") under activeLimit = 1 is what proves it - worth a comment on that line, because it currently reads as incidental sequencing.

Also better than dev, unprompted: dev logged createSession: id={}, workdir={}, command={} at INFO - the full argv and working directory on every launch. The new code logs the id alone (TerminalServiceImpl.kt:57). That materially strengthens your reason for declining the raw-IOException logging request; the two decisions are now consistent rather than one being an exception to the other.

The byte-preserving chunks (ByteString.copyFrom(buffer, 0, count)) also fix a real dev defect the earlier review didn't call out: dev decoded through InputStreamReader with a 4096-char buffer, so any multi-byte sequence straddling a read boundary was replaced with U+FFFD. hello café 世界 in the fixture is a deliberate and good choice of payload.

---

Residual findings on this revision

1. Medium - on Windows, available() on a broken pipe throws, and the catch kills a still-live child

TerminalSession.kt:52-53 is catch (_: IOException) { process.destroyForcibly() }, and it wraps both the poll loop and the drain.

On Windows, FileInputStream.available() for a pipe handle goes through handleNonSeekAvailable → PeekNamedPipe, which fails with ERROR_BROKEN_PIPE once the write end is closed, and the JDK turns that failure into an IOException rather than returning 0. On POSIX the same situation returns 0 (FIONREAD on a pipe with no writer is not an error). So the two platforms take different branches from the same state:

Child closes stdout but keeps running	dev	this PR
Linux / macOS	pump exits at EOF, process left alive, slot freed early	available() returns 0, pump polls at 10 ms until real exit ✅
Windows	pump exits at EOF, process left alive, slot freed early	available() throws → destroyForcibly() kills the live child

Reachable from an ordinary cmd.exe session that runs something which closes handle 1, or any child that execs with stdout redirected. Killing it is worse than either previous behaviour, and it is silent - the operator sees [Process exited with code 1] with nothing to say BOSS did it.

The narrow fix is to treat a broken pipe as "no more output from this pipe" rather than as a fault: stop reading, fall through to the finally, and only destroyForcibly() when the process is genuinely still alive and you intend to reap it. At minimum log which branch was taken, because right now the two platforms are indistinguishable in the log. This is also precisely the item in your step 2 ("inspect the output-pump change on Linux, macOS and Windows") that a Linux-and-macOS-only run will not surface. (Reasoned from io_util_md.c / PeekNamedPipe semantics - I could not compile a probe here, so please confirm on a Windows runner before merge.)

Fix this →

2. Medium - TerminalServiceMain.kt:91 does register a shutdown hook, and close() still joins unbounded

Flagging this only because the disposition recorded the opposite ("TerminalServiceMain closes the service in finally, not in a newly registered hook"). The diff adds both:

Runtime.getRuntime().addShutdownHook(Thread({ terminalService.close() }, "terminal-session-cleanup"))
try { ... } finally { terminalService.close(); connection.shutdown() }

and close() ends in active.forEach { it.process.onExit().join() } (TerminalServiceImpl.kt:111) with no timeout. destroyForcibly is SIGKILL on POSIX so this returns promptly in the normal case; the residual is a process wedged in uninterruptible I/O, where a shutdown hook that never returns means the JVM never exits. I take your point that a timeout which frees admission while work remains would weaken the bound - but on the shutdown path there is no admission left to protect, so a bounded onExit().get(n, SECONDS) with a warn-and-continue costs nothing there. The repo's own precedent (BrowserNativeDisposal: warn once at ten seconds and keep waiting) is a reasonable middle.

Two smaller things in the same place: the hook is never removed, so the normal finally path leaves it registered; and close() is double-invoked and only accidentally idempotent (the second call's filter { it.active } is empty - and strictly it's racy, since close() returns when the process exits while active is cleared later by the pump thread, so a second close() can re-terminate() an already-dead process). Harmless today, worth one line of comment since nothing enforces it.

Fix this →

3. Medium - two client-triggerable paths still surface as UNKNOWN

The five statuses you added cover the size and dimension guards, but two request-shaped failures still reach the client with no code and no description:

• A malformed environment variable name. builder.environment().putAll(request.environmentMap) (TerminalSession.kt:124) goes through ProcessEnvironment, which throws IllegalArgumentException for a name containing = or NUL - on both POSIX and Windows. protobuf map<string,string> permits both, so environment { "A=B": "x" } is a plain bad request that arrives as UNKNOWN. It also throws after reserveSlot(), so it exercises the same release path as the dimension cap (that part is correct).
• sendInput to a child that closed stdin. send checks !process.isAlive (:78) and then writes (:81), and the IOException from a broken stdin is uncaught - so the check-then-write race, and a child that closed handle 0 while running, both yield UNKNOWN where FAILED_PRECONDITION is already the established answer three lines up.

Both are small and in the spirit of the change rather than beyond it: the stated goal is that a consumer can tell a rejected request from a server fault, and these two are the remaining places where it can't.

Fix this →

4. Medium - the 10 ms poll loop trades a blocking read for per-session idle wakeups

available() / sleep(10) (TerminalSession.kt:34-42) means every live session wakes 100×/s and issues an ioctl/PeekNamedPipe syscall per wake, whether or not there is output. At the shipped activeLimit = 16 that is ~1600 syscalls/s and 16 thread wakeups per 10 ms window at complete idle, where dev cost zero. It also puts a 10 ms floor on output latency, which for an interactive terminal is noticeable at the margin (keystroke echo).

There is a shape that gets the same liveness guarantee with neither cost: keep the blocking read(), and register process.onExit().thenRun { process.inputStream.close() } so the blocked read is unblocked by the close instead of by a descendant's EOF. ProcessPipeInputStream has a closeLock precisely so close() can interrupt a concurrent read. That is a larger change than this PR needs and I'd take it as a follow-up rather than a blocker - but the polling cost should be written down somewhere, because it is the one place this PR makes an unconditional resource worse in exchange for bounding a conditional one.

5. Low/Medium - the post-exit drain can silently truncate, and the reaper's own drain is unbounded

Two things on TerminalSession.kt:44:

• minOf(input.available(), 65_536) discards anything beyond 64 KiB with no marker in the stream. Bounding is right; being silent about it is the part I'd change, since [Process exited with code 0] immediately after truncated output reads as the program having produced less than it did. One [output truncated] chunk when available() > 65_536 costs nothing.
• The bound does not actually cap what the JVM buffers. On POSIX, ProcessPipeInputStream.processExited() runs drainInputStream, which loops while (available() > 0) growing a byte[] by Arrays.copyOf each iteration - so a descendant writing hard after parent exit is buffered unboundedly, on the shared process-reaper thread, with O(n²) copying, entirely outside TerminalOutputBuffer's 1 MiB / 256-entry bounds. dev never reached this because the blocked pump held the monitor. Not a defect in your code, and not obviously fixable from here (closing the stream first races the reaper and loses the stragglers), but it belongs in the KDoc next to the drain, because the file currently reads as though 64 KiB is the ceiling. (Reasoned from OpenJDK ProcessImpl; not measured here.)

6. Low - .github/workflows/build.yml change does not belong in this PR

-    branches: [ main, develop, dev ]
+    branches: [ main, develop, dev, 'fix/security-*', 'fix/filesystem-service-*' ]

This is a permanent CI-trigger widening that would live in dev after merge, and one of the two patterns (fix/filesystem-service-*) is for a different change entirely. Branch-scoped push triggers added to get a fork run going are the kind of thing that is never removed. I'd drop it and rely on the pull_request trigger, or split it into its own PR so the decision is visible. (I can't edit workflow files myself - GitHub App permissions don't allow it.)

7. Low - carried over, still open, all fine as follow-ups

• streamSlots = Semaphore(32) (TerminalServiceImpl.kt:25) is still hard-coded while activeLimit/historyLimit are parameters, still global rather than per-session (32 readers on one session starve every other session's streamOutput), and still the only limit with zero test coverage.
• sendInput's blocking write parking a Dispatchers.IO thread on a non-draining child. Your bound (one writer per session via inputLock, ≤ 16 sessions) is a real bound and I accept the deferral; noting it so the follow-up isn't lost.
• finished = chunk.isExit (TerminalOutputBuffer.kt:39) - agreed the check(!finished) guard makes the OR latch cosmetic. Leaving it as noted, not as a finding.
• Cancellation orphans: TerminalServiceImpl.kt:76-85 with pumping == true leaves an inactive history entry no client holds an id for. Evictable, so bounded; a cancellation storm just fills historyLimit with them.
• createSession catches only IOException (:69), so any other ProcessBuilder.start() failure becomes UNKNOWN rather than success = false - same family as finding 3.

---

Regression coverage

Strong where it counts. The descendant test is a true regression test for finding 1 (real grandchild, liveness asserted, replacement admitted under activeLimit = 1); the PausedDispatcher test still pins the exact return-dispatch race delivered exists for; the status test drives all five new codes through a real channel rather than the impl; and retained.size == 2 plus assertFalse(retained.any { it.sessionId == first }) pins eviction order, not just count. Using a real child JVM instead of a mock remains the right call.

Gaps, in the order I'd close them:

• streamSlots exhaustion - 33rd concurrent reader → RESOURCE_EXHAUSTED. Still the only limit with no test at all.
• NOT_FOUND on any of the four session-addressed RPCs, and FAILED_PRECONDITION for sendInput after exit. Both are in the consumer contract you wrote out in the disposition, and neither is asserted anywhere.
• TerminalOutputBuffer happy path. TerminalOutputBufferTest has exactly one test and it's the gap case. The batch-of-16 cursor arithmetic in stream() (:60-73) is the subtlest code in the PR and its in-order/no-loss behaviour for a reader that keeps up is only covered incidentally. Also untested: the bytes > 1_048_576 vs entries.size > 256 eviction accounting (which is what keeps removeFirst() at :38 off an empty deque), and that append after the exit chunk is rejected.
• Finding 1's Windows sibling - a child that closes stdout and stays alive. A TerminalTestProcess mode for it would cover finding 1 above on all three runners.
• close() called twice, and whether in-flight streamOutput collectors complete rather than hang across shutdown.

Two test-hygiene notes:

• awaitExit still uses sessionsList.single { it.sessionId == id } (:310), which throws NoSuchElementException if retain evicted the session first. It holds at historyLimit = 2 today, but it couples a generic helper to the eviction policy - firstOrNull plus an explicit assertion fails far more legibly when someone changes retain.
• an exited shell releases capacity while a descendant still holds stdout uses withTimeout(5_000) where every other test uses 15 s, and it is the one test that starts two JVMs (fixture + grandchild) and waits on a pid file plus a 200 ms sleep, with start() retrying against activeLimit = 1. On a loaded Windows runner that is the tightest budget in the file guarding the slowest test. I'd raise it to 15 s for consistency - a flake here reads as the fix being wrong.

---

Summary

The two blocking findings are genuinely fixed, and fixed in a way that preserves the pump/process accounting rather than trading one unbounded resource for another - the reasoning in your disposition about not releasing from onExit() alone is correct and the code matches it. Of what's left, finding 1 (Windows broken-pipe → destroyForcibly) is the one I'd want resolved or explicitly confirmed on a Windows runner before merge, because it is a new way to kill a user's process and it is exactly the platform your step-2 checklist calls out. Finding 2 is a factual correction to the disposition rather than a new defect; findings 3-5 are small and in scope; finding 6 is a hygiene ask about the CI trigger. Everything else is reasonable as a follow-up.

I could not execute the validation you asked for in step 1 - gradle is not invocable in this runner - so the 9-test / ktlint / detekt result still needs to come from your local run or hosted CI on this exact head.
• branch fix/security-terminal-limits

[kshivang]

@Raghav-Bansal-15 Thank you for following through on the terminal resource limits and retaining the original/reviewer contributions. At 78be8747422deec41c27821456f667f51814850d, the pump now reads available bytes and a bounded post-exit snapshot, releases admission while an inherited-stdout descendant is still alive, and preserves raw output bytes. The explicit gRPC validation statuses and cancellation-return ownership regression are retained; the earlier formatting handoff is resolved.

Independent local validation passed: all 9 terminal tests, :boss-app-terminal:ktlintCheck, and :boss-app-terminal:detekt, under the shared build lock. I also verified the exact-head fork Build & Test run succeeded. The current PR cleanly merges with dev 600423381; no source repair was needed in this pass.

The remaining gate is upstream validation, not an author defect. I approved upstream run 34870814991, which still has queued/running required jobs, and requested one current-head Claude review (in progress). I cannot call this merge-ready or substitute fork CI for those gates. Under the bounded-review policy I am returning this to draft for the validation handoff, without asking you to rewrite the implementation.

To restore ready: let the current-head upstream checks finish, address any actual failures or independently verified review findings, and rerun the focused terminal tests if the head changes. Then refresh dev and verify integration before an exact-head squash merge into dev. Do not bypass checks. The prior suggestion to log raw launch exceptions remains rejected because commands/paths may contain sensitive data; the old shutdown-hook assertion did not match the source. One blocked stdin writer per admitted session remains bounded and can be followed up separately.

Placement/overlap: this is the host OOP terminal service, not the in-process terminal-tab UI. #574 manages process logs and #575 manages identity; they are distinct. No live UI check was performed. Operator follow-up should exercise normal shell input, background-child/parent-exit replacement admission, replay overflow status, and service shutdown through an OOP consumer.

[Raghav-Bansal-15]

September 15 follow-up: Original and reviewer commits are retained; current dev is integrated.

A broken output pipe no longer kills a still-live child. Admission remains held until that child exits. Shutdown waiting is bounded; invalid environment and closed-input failures have explicit statuses. Includes a broken-pipe lifecycle regression.

Validation: All 11 terminal tests, ktlint and detekt pass locally.

Current head 417d7894: fresh fork CI is queued/running; no final-head all-green claim. Upstream checks and approval remain separate merge gates. No merge or release performed.

[Raghav-Bansal-15]

Windows CI exposed inconsistent JDK validation of malformed environment entries. Command and environment input is now explicitly validated before launch; expanded NUL/name regressions and all 11 terminal tests plus quality checks pass locally. A fresh Windows run is required.

Head 45e6d1f9 is pushed. Fresh CI pending; no merge or release.

[Raghav-Bansal-15]

The next Windows run passed environment validation but timed out at the replacement JVM output assertion (TerminalLimitsTest line 280), after parent cleanup and replacement admission had succeeded. Fixture startup now has a separate readiness deadline; parent cleanup/admission still has five seconds, and replacement execution has its own deadline. All 11 tests and quality checks pass locally; fresh Windows verification remains pending.

New head 8db3582a.

[kshivang]

@Raghav-Bansal-15 I verified the September 15 follow-ups against the previous draft handoff: the broken-output-pipe path retains admission without killing the child; malformed environment values receive INVALID_ARGUMENT; shutdown has a shared deadline; the Windows regression separates fixture startup from cleanup. These are your follow-up changes, not new reviewer work.

I merged the rebased dev 77c4a4071 preserving existing authorship, then made one narrow repair: only argv[0] must be nonempty. Empty later arguments are valid and must reach the child unchanged. TerminalLimitsTest now exercises a real child with an empty argument and verifies its length. Reviewer head dc0d9d5f9 is pushed; focused module tests/ktlint/detekt are queued under the shared build lock, so no new local pass is claimed yet. Existing scores remain unchanged.

@claude Please review this current head once against dev, focusing on the prior pipe/shutdown/status findings and the empty-argument regression. Do not edit code. Upstream exact-head checks and this review are merge gates; pending infrastructure is not an author defect. No merge or app launch performed.

[claude]

Claude finished @kshivang's task in 6m 42s —— View job

---

Review: PR #570 at dc0d9d5f9 vs dev

• Confirm the diff scope (origin/dev vs HEAD, module only)
• Verify the broken-output-pipe finding is resolved
• Verify the bounded-shutdown finding is resolved
• Verify the remaining UNKNOWN-status findings (environment, closed stdin) are resolved
• Review the empty-argument regression
• Look for new correctness regressions in this head
• Review regression coverage
• Post review

Sandbox limits, stated up front. gradle, unzip of the JDK src.zip and git fetch are all blocked in this runner, so I could not run :boss-app-terminal:test, ktlintCheck or detekt, and could not read the JDK sources to settle the one Windows question below empirically. The clone is shallow, so origin/dev...HEAD has no merge base; I used the two-dot git diff origin/dev HEAD, which shows 933/164 across exactly the 8 modules/boss-app-terminal files and nothing else (the composeApp/docs lines in the full two-dot diff are dev being ahead, not PR changes). So I can confirm the dev merge is clean in the sense you described. Every platform claim below is labelled verified in the diff vs reasoned.

---

Prior findings

Broken output pipe (previous finding 1) is resolved, and resolved correctly. TerminalSession.kt:52-55 no longer calls destroyForcibly(); it records [Terminal output pipe closed] in the session's own output and falls through to the finally, which holds the slot in process.onExit().join() (:59) until the child genuinely exits. The important property is that the fix is now correct under either platform premise - POSIX available() returning 0 keeps the poll loop going, Windows PeekNamedPipe failing takes the catch - and neither branch kills a live child. That removes my dependence on the ERROR_BROKEN_PIPE claim I could not measure last time.

TerminalPipeFailureTest is the right shape for this: a fake Process whose available() throws is the only way to exercise the Windows branch on a Linux runner, and it asserts the three things that matter separately - killed == false, admission not released (stopped.get(100ms) times out), and released once exit completes. A test that only checked the exit path would pass against the old destroyForcibly.

Bounded shutdown (previous finding 2) is resolved. TerminalServiceImpl.close() (:115-124) now computes one 5-second deadline and spends it across all active processes with waitFor(remaining, NANOSECONDS), coerceAtLeast(0). The JVM-never-exits risk from the shutdown hook is gone, and the bound is shared rather than per-process, which is the stronger reading. Two nits stand, both cosmetic: the hook at TerminalServiceMain.kt:91 is still never removed on the normal finally path, and close() is still only accidentally idempotent (the second call re-terminate()s already-dead processes and waitFor returns immediately, so it is harmless - but nothing states that). The warn at :122 also names no session or pid, so an operator cannot tell which terminal missed the deadline.

The two remaining UNKNOWN paths (previous finding 3) are resolved. Environment and command validation moved ahead of ProcessBuilder into validateLaunchInput (TerminalSession.kt:115-124) and createSession maps IllegalArgumentException to INVALID_ARGUMENT (TerminalServiceImpl.kt:69-72); a closed child stdin is now FAILED_PRECONDITION via writeInput's catch (:93-95), matching the exited-process branch three lines up. I traced the admission path for the pre-launch rejection: reserveSlot() has already acquired, session is still null, so the finally at :80-88 takes the !pumping branch and releases - assertTrue(start("echo").isNotBlank()) under activeLimit = 1 is what proves it, which is the right assertion.

Validating '=' !in name yourself is also the right call and worth recording: Java's Windows ProcessEnvironment.validateName permits a leading = (the magic =C: variables) where the Unix one rejects any =, so the JDK itself is the source of the inconsistency Windows CI surfaced. Pre-validating to the stricter Unix rule makes the two platforms agree, at the cost of never being able to set a Windows magic variable through this service - which is a fine trade for an OOP terminal launcher. (Reasoned from ProcessEnvironment; I could not extract the sources here.)

Previous finding 6 (CI trigger widening) is resolved - .github/ is byte-identical to dev in this head.

---

The empty-argument regression

The predicate is correct. require(request.commandList.firstOrNull()?.isNotEmpty() != false) (TerminalSession.kt:117) gives exactly the three-way answer you described:

commandList	result	why that is right
empty	passes (null != false)	the default-shell substitution at :130-133 supplies argv[0]
["", …]	rejected → INVALID_ARGUMENT	argv[0] must name something
["java", …, ""]	passes	an empty later argument is a valid argument

The test is a real one rather than a proxy: addCommand("") lands as args[2], the fixture prints args[2].length(), and the assertion is on the child's own observation (argument-length=0), not on what the service recorded. Asserting the length rather than the presence is the detail that makes it meaningful - a dropped argument and a preserved-but-mangled one are different failures and this distinguishes them.

Two things about it I would change, one of which is specifically about the platform this PR keeps tripping over:

• On Windows the failure mode is a stack trace, not an assertion. Windows has no argv; the JDK reassembles a single command line, and whether an empty argument survives depends on ProcessImpl.needsEscaping special-casing the empty string so it is emitted as "". Modern JDKs do quote it (reasoned - I could not read the Windows ProcessImpl, and a Linux src.zip would not contain it), so I expect this to pass. But if it does not, the fixture takes args[2] on a 2-element array and dies with ArrayIndexOutOfBoundsException, so the Windows run reports a missing argument-length=0 string with the real cause buried in the child's stderr. args.length > 2 ? args[2].length() : -1 - or printing args.length alongside - turns that into a legible failure. Given this is the one change in the head with no cross-platform evidence yet, that is worth doing before the Windows run rather than after it.
• The predicate reads as three-valued logic. ?.isNotEmpty() != false makes the reader derive that the empty-list case is deliberately allowed; require(request.commandList.isEmpty() || request.commandList.first().isNotEmpty()) states the rule. For a security-validation line whose whole point is that one of three cases is different, saying it directly is worth the extra characters. require here also carries no message, which is consistent with the generic status but means the server log has nothing either.

Minor: the head now has 12 @Test methods (10 in TerminalLimitsTest, 1 each in the other two), so the "all 11 terminal tests" figure in the PR body is stale by one.

---

New findings on this head

1. Low/Medium - the broken-pipe branch closes the read end while the child is still alive

process.inputStream.use { … } (TerminalSession.kt:30) closes the stream on the exception path too (Kotlin's use calls close() and suppresses a close failure into the original). So in the IOException branch the service closes its read end of the pipe and then waits for a child that is still running.

If the branch fired because the child had already closed its write end, nothing is harmed - that is the Windows case it exists for. But if it fired for any other reason while the child is still writing (a transient read fault, an EIO), then closing the read end gives the next child write EPIPE/SIGPIPE, which on POSIX kills it by default. That is the same outcome - a dead live child - that removing destroyForcibly() was meant to prevent, arriving by a different door.

The narrow fix is to keep the stream open in that branch and let the finally close it after onExit(), or to state in the comment that the branch is only reachable once the write end is already gone. Either is fine; what is not fine is that the code currently reads as though closing is free.

Fix this →

2. Low - the pump writes its diagnosis into the session's output and nowhere else

TerminalSession has no logger, so a broken pipe leaves nothing in the service log - the only record is [Terminal output pipe closed] inside that session's replay buffer, which is bounded and is gone once the session is evicted. That is exactly the platform-divergent path you most want evidence for after a Windows incident, and it is also the one place where "which branch did the pump take" is unanswerable after the fact. A single logger.warn with the session id (no command, no paths - consistent with your rejection of raw IOException logging) would close it without weakening the sanitisation stance.

3. Low - workingDirectory is not NUL-validated while command and environment are

validateLaunchInput checks NUL in every command element and in every environment name and value, but request.workingDirectory goes straight to File(directory) (:141). On POSIX the path is handed to exec as a C string, so "/tmp/elsewhere" launches in /tmp while listSessions keeps reporting the full string; Windows rejects or truncates it differently. (Reasoned; not measured.) Not a security boundary - the caller chose the directory either way - but it is the one field left out of a function whose stated purpose is "validate before ProcessBuilder: native environment validation differs between OS/JDK implementations", and a mismatch between the launched cwd and the reported one is a debugging trap.

4. Low - the IllegalArgumentException catch is wider than the validation it serves

catch (_: IllegalArgumentException) at TerminalServiceImpl.kt:69 spans the whole withContext block, so any IAE from retain, startPump or ProcessBuilder.start() is also reported to the client as Invalid terminal command or environment. Nothing in that span throws IAE today, so this is a readability/futureproofing note rather than a live defect - but the message asserts a cause the catch does not establish. Validating and mapping at the validateLaunchInput call site (throwing Status.INVALID_ARGUMENT directly, as launch already does for dimensions) would make the claim true by construction.

5. Carried over, unchanged, and fine as follow-ups

• The 10 ms poll loop (:34-42) still costs ~100 wakeups/s per live session (≈1600/s at activeLimit = 16) and puts a 10 ms floor on output latency, where dev blocked for free. Worth writing down somewhere, because it is the one unconditional resource this PR makes worse in exchange for bounding a conditional one. One new wrinkle for whoever takes the follow-up: the obvious replacement (blocking read() plus onExit().thenRun { inputStream.close() }) makes the normal exit path throw IOException: Stream closed straight into the new broken-pipe catch, so the two changes are now entangled and the catch would have to distinguish them.
• The post-exit drain is silently truncated at 64 KiB (:44) - one [output truncated] marker would stop [Process exited with code 0] reading as the program having produced less than it did. And on POSIX the JVM's own process reaper runs drainInputStream in an unbounded while (available() > 0) loop at process exit, outside TerminalOutputBuffer's 1 MiB bound, so 64 KiB is not actually the ceiling on what gets buffered. (Reasoned from OpenJDK ProcessImpl.) Belongs in the KDoc beside the drain.
• streamSlots = Semaphore(32) (:25) is still hard-coded while activeLimit/historyLimit are parameters, still global rather than per-session, and still the only limit with zero test coverage.
• sendInput's blocking write on a non-draining child. Your bound (one writer per session via inputLock, ≤ 16 sessions) is real; noting it so the follow-up is not lost.
• The active = false before onStopped() ordering (:68-69) is what keeps sessions at or below historyLimit, since retain can only evict entries that read inactive and a freed slot must therefore correspond to an already-inactive entry. I re-derived it and it holds. The comment at :57 covers why capacity spans the pump, not this ordering - it is still the load-bearing implicit invariant the first review asked to have written down.
• Cancellation orphans (:80-88 with pumping == true): an inactive history entry no client holds an id for. Evictable, so bounded.

Style nit: java.util.concurrent.TimeUnit is fully qualified inline at :117-121 (and wraps awkwardly) while Semaphore is imported - an import would read better, unless ktlint drove it.

---

Regression coverage

Good where it counts, and the two new tests are both real regressions rather than proxies. The descendant test now separates three budgets deliberately - 30 s for fixture JVM startup, 5 s for the parent-exit cleanup and admission reuse, 10 s for the replacement's cold execution - and the middle one is an assertion about the bound, not a generic anti-hang timeout, which is the right way round and answers my earlier "raise it to 15 s" note. The comments at :274, :294 and :297 say which is which. My residual concern is only that the 5 s window still contains a full gRPC streamOutput collection plus a start() that retries at 10 ms, so a loaded Windows runner failing there will read as the fix being wrong rather than as the runner being slow; if it flakes, split the stream collection out rather than raising the number.

Still uncovered, in the order I would close it:

• streamSlots exhaustion (33rd concurrent reader → RESOURCE_EXHAUSTED).
• NOT_FOUND on any of the four session-addressed RPCs, and FAILED_PRECONDITION for sendInput after exit or to a closed stdin. Both are in the consumer contract you wrote out, and the closed-stdin one is new code in this head with no test.
• TerminalOutputBuffer happy path. TerminalOutputBufferTest is still one test and it is the gap case. The batch-of-16 cursor arithmetic in stream() (:61-73) remains the subtlest code in the PR; its in-order/no-loss behaviour for a reader that keeps up is covered only incidentally, and the bytes > 1_048_576 vs entries.size > 256 eviction accounting that keeps removeFirst() off an empty deque is untested, as is append after the exit chunk.
• A child that closes stdout and stays alive, end to end - TerminalPipeFailureTest covers the branch with a fake Process, which is right, but a TerminalTestProcess mode that closes handle 1 and sleeps would cover finding 1 above and the real OS semantics on all three runners.
• close() called twice, and whether in-flight streamOutput collectors complete rather than hang across shutdown.

awaitExit still uses sessionsList.single { it.sessionId == id } (:356). I traced every call site against retain at historyLimit = 2 and it holds in all five, but it still couples a generic helper to the eviction policy; firstOrNull plus an explicit assertion fails far more legibly when someone changes retain.

---

Summary

All three of the September 15 follow-ups hold up: the broken-pipe path retains the child and its admission until real exit and is tested at the branch level, shutdown is bounded by one shared 5-second deadline, and the malformed-environment and closed-stdin paths now carry explicit statuses. The empty-argument repair is logically correct across all three cases and the test asserts the child's own view of the argument rather than the service's.

Nothing here is a merge blocker in the sense the previous two reviews were. The one thing I would do before the Windows run rather than after is make the argument fixture fail legibly (args.length > 2 ? … : -1), since a Windows JDK that drops the empty argument would surface as a confusing missing-string assertion with the real cause in the child's stderr - and that is the only claim in this head with no cross-platform evidence. Finding 1 (the use {} close on a live child) is the only new correctness item and it is narrow; findings 2-5 are small or carried over.

I could not execute :boss-app-terminal:test, ktlintCheck or detekt here, so the focused-module result still has to come from your queued run or upstream CI on this exact head. I made no code changes, per your instruction.
• branch fix/security-terminal-limits

[Raghav-Bansal-15]

Follow-up at 23db7498: The read end now remains open until actual child exit after an output read failure. The premature-close regression fails before this fix. Preserved the reviewer’s valid-empty-argument change and improved its fixture diagnostic. All 12 terminal tests and quality checks pass locally.

Original and reviewer commits are preserved. Remaining current-head hosted checks and upstream approval are separate gates; no merge or release.

[kshivang]

@Raghav-Bansal-15 I reviewed 23db7498129681f7f5baac6d95e54790c0577501 and the completed Claude review. Your follow-up keeps the read end open after a read failure until actual child exit; the regression explicitly rejects premature close and verifies close after exit. The valid-empty-argument reviewer fix is preserved, and your fixture now reports a missing argument clearly.

I independently downloaded current-head fork JUnit reports: all 12 terminal tests pass on Linux, macOS and Windows, zero failures/errors/skips. Current-head fork code-quality, compatibility and all platform jobs passed (run 34977367741). The prior actionable pipe/status/shutdown findings are addressed. Polling cost, bounded final-output truncation and additional status/reader-quota coverage remain follow-up scope, not reasons for another speculative rewrite.

No new reviewer code changes or score change this pass. Current dev merge-tree is clean. Upstream run 34977237169 still requires explicit workflow-execution approval; fork success does not replace that gate. Keeping this useful PR ready, not drafting it for infrastructure. No merge or release.