fix(cli): tighten terminal command broker protocol path handling

Return reapOwnProcessGroup() directly from the protocol write failure path so a failed write never outlives a successful one, and flush the stderr marker through a bounded wait before the group is killed. On POSIX process.stderr is an async pipe, so the previous fire-and-forget write could lose the failure reason to the immediate SIGKILL.

Remove protocolWriteTargets(): the raw-environment fallback skipped temp-directory validation, and its rejection case could not be reproduced because the broker inherits the parent environment. Compare the protocol directory after resolving symlinks, fold case on Windows, and cover both spellings plus the reaping path with tests.

Author: ayaan0209

cli/src/utils/__tests__/terminal-command-broker.test.ts

@@ -1,6 +1,14 @@
 import { describe, expect, test } from 'bun:test'
 import { spawn, spawnSync } from 'child_process'
-import { existsSync, mkdtempSync, readdirSync, readFileSync, rmSync } from 'fs'
+import {
+  existsSync,
+  mkdtempSync,
+  readdirSync,
+  readFileSync,
+  rmSync,
+  symlinkSync,
+  writeFileSync,
+} from 'fs'
 import { tmpdir } from 'os'
 import path from 'path'
 
@@ -173,6 +181,54 @@ describe('terminal command broker', () => {
     ).toThrow('terminal command broker protocol path was invalid')
   })
 
+  test('accepts a protocol path spelled through a symlinked temp directory', () => {
+    const linkPath = path.join(
+      tmpdir(),
+      `freebuff-broker-tmp-link-${crypto.randomUUID()}`,
+    )
+    try {
+      symlinkSync(tmpdir(), linkPath, 'dir')
+    } catch {
+      // Hosts without symlink permission cannot exercise this spelling.
+      return
+    }
+    const validName = `freebuff-terminal-command-broker-${process.pid}-${crypto.randomUUID()}.json`
+    try {
+      expect(
+        protocolPathFromEnv({
+          CODEBUFF_TERMINAL_COMMAND_BROKER_PROTOCOL: path.join(
+            linkPath,
+            validName,
+          ),
+        }),
+      ).toBe(path.join(linkPath, validName))
+      expect(() =>
+        protocolPathFromEnv({
+          CODEBUFF_TERMINAL_COMMAND_BROKER_PROTOCOL: path.join(
+            linkPath,
+            'wrong-prefix.json',
+          ),
+        }),
+      ).toThrow('terminal command broker protocol path was invalid')
+    } finally {
+      rmSync(linkPath, { force: true })
+    }
+  })
+
+  test('compares the protocol directory case-insensitively on Windows', () => {
+    if (process.platform !== 'win32') return
+    const validName = `freebuff-terminal-command-broker-${process.pid}-${crypto.randomUUID()}.json`
+    const upperCasedDirectory = tmpdir().toUpperCase()
+    expect(
+      protocolPathFromEnv({
+        CODEBUFF_TERMINAL_COMMAND_BROKER_PROTOCOL: path.join(
+          upperCasedDirectory,
+          validName,
+        ),
+      }),
+    ).toBe(path.join(upperCasedDirectory, validName))
+  })
+
   test('relays stdout, stderr, and the command exit code', async () => {
     const [{ value }] = await runTerminalCommand({
       command: `printf 'OUT'; printf 'ERR' >&2; exit 7`,
@@ -419,6 +475,70 @@ describe('terminal command broker', () => {
     expect(result[0].value).toMatchObject({ exitCode: 0 })
   })
 
+  test('reaps the shell tree promptly when the protocol write fails', async () => {
+    if (process.platform === 'win32') return
+    const tempDir = mkdtempSync(path.join(tmpdir(), 'codebuff-broker-reap-'))
+    const commandPidPath = path.join(tempDir, 'command.pid')
+    const protocolPath = path.join(
+      tmpdir(),
+      `freebuff-terminal-command-broker-${process.pid}-${crypto.randomUUID()}.json`,
+    )
+    // `wx` refuses to replace an existing file, so the one-shot write fails
+    // while the path is still the validated temp path.
+    writeFileSync(protocolPath, 'already here')
+    const broker = spawn(process.execPath, [brokerFixture], {
+      env: {
+        ...process.env,
+        CODEBUFF_TERMINAL_COMMAND_BROKER: '1',
+        CODEBUFF_TERMINAL_COMMAND_BROKER_PROTOCOL: protocolPath,
+      },
+      stdio: ['pipe', 'pipe', 'pipe'],
+      detached: true,
+    })
+    const brokerPid = broker.pid as number
+    let stderr = ''
+    broker.stderr?.on('data', (chunk: Buffer) => {
+      stderr += chunk.toString('utf8')
+    })
+    const brokerClosed = new Promise<void>((resolve) => {
+      broker.once('close', () => resolve())
+    })
+    broker.stdin?.end(
+      JSON.stringify({
+        executable: '/bin/sh',
+        args: ['-c', `sleep 30 & echo $! > ${JSON.stringify(commandPidPath)}`],
+        cwd: process.cwd(),
+        env: { PATH: process.env.PATH ?? '' },
+      }),
+    )
+
+    const commandPid = (await waitFor(() => existsSync(commandPidPath), 3_000))
+      ? Number(readFileSync(commandPidPath, 'utf8'))
+      : 0
+    try {
+      await Promise.race([brokerClosed, Bun.sleep(3_000)])
+      expect(commandPid).toBeGreaterThan(0)
+      expect(await waitFor(() => !isProcessRunning(commandPid), 3_000)).toBe(
+        true,
+      )
+      expect(() => process.kill(-brokerPid, 0)).toThrow()
+      expect(stderr).toContain('[freebuff-broker] protocol write failed:')
+    } finally {
+      try {
+        process.kill(-brokerPid, 'SIGKILL')
+      } catch {
+        // The broker already reaped its whole process group.
+      }
+      try {
+        process.kill(commandPid, 'SIGKILL')
+      } catch {
+        // The command descendant was reaped together with the broker group.
+      }
+      rmSync(protocolPath, { force: true })
+      rmSync(tempDir, { recursive: true, force: true })
+    }
+  })
+
   test('does not add broker recovery guidance to a command spawn failure', async () => {
     const missingCwd = path.join(
       tmpdir(),

cli/src/utils/terminal-command-broker.ts

@@ -1,5 +1,5 @@
 import { spawn, spawnSync } from 'child_process'
-import { readFileSync, rmSync, writeFileSync } from 'fs'
+import { readFileSync, realpathSync, rmSync, writeFileSync } from 'fs'
 import os from 'os'
 import path from 'path'
 
@@ -29,6 +29,9 @@ const MAX_BROKER_STDERR_TAIL_BYTES = 4 * 1024
 // Cross-process contract: the detached broker emits this marker on stderr
 // before reaping itself so the parent can surface the write failure reason.
 const BROKER_STDERR_MARKER = '[freebuff-broker] protocol write failed:'
+// The marker must be flushed before the broker SIGKILLs its own process group,
+// but never at the cost of holding the shell tree open indefinitely.
+const BROKER_STDERR_FLUSH_TIMEOUT_MS = 200
 
 export type TerminalBrokerFailureStage = 'spawn' | 'stdio' | 'completion'
 export type TerminalBrokerFailureCode =
@@ -128,6 +131,22 @@ function isSpawnRequest(value: unknown): value is TerminalCommandSpawnRequest {
   )
 }
 
+/**
+ * Compare the protocol directory after resolving symlinks and Windows casing:
+ * a raw string compare rejects the parent's own path when the temp dir is
+ * reached through a link (macOS `/var` -> `/private/var`) or a differently
+ * cased spelling.
+ */
+function normalizeProtocolDirectory(directory: string): string {
+  let resolved = path.resolve(directory)
+  try {
+    resolved = realpathSync.native(resolved)
+  } catch {
+    // The directory may not exist yet; the raw resolution stays comparable.
+  }
+  return process.platform === 'win32' ? resolved.toLowerCase() : resolved
+}
+
 export function protocolPathFromEnv(
   env: NodeJS.ProcessEnv = getSystemProcessEnv(),
 ): string {
@@ -138,7 +157,8 @@ export function protocolPathFromEnv(
 
   const resolvedProtocolPath = path.resolve(protocolPath)
   if (
-    path.dirname(resolvedProtocolPath) !== path.resolve(os.tmpdir()) ||
+    normalizeProtocolDirectory(path.dirname(resolvedProtocolPath)) !==
+      normalizeProtocolDirectory(os.tmpdir()) ||
     !path.basename(resolvedProtocolPath).startsWith(PROTOCOL_FILE_PREFIX)
   ) {
     throw new Error('terminal command broker protocol path was invalid')
@@ -162,41 +182,21 @@ function removeProtocolFile(protocolPath: string): void {
   }
 }
 
-function protocolWriteTargets(): string[] {
-  try {
-    return [protocolPathFromEnv()]
-  } catch {
-    // Path validation can only reject a broken environment; still attempt the
-    // parent-provided path so a valid response is not lost to over-strictness.
-    const raw = getSystemProcessEnv()[TERMINAL_COMMAND_BROKER_PROTOCOL_ENV]
-    return raw ? [path.resolve(raw)] : []
-  }
-}
-
 function writeProtocol(message: BrokerProtocol): void {
   const payload = `${JSON.stringify(message)}\n`
   if (Buffer.byteLength(payload) > MAX_PROTOCOL_BYTES) {
     throw new Error('terminal command broker response was too large')
   }
-  let lastError: unknown = new Error(
-    'terminal command broker protocol path was invalid',
-  )
-  for (const target of protocolWriteTargets()) {
-    try {
-      // A constrained one-shot file avoids Bun's unreliable custom stdio pipes on
-      // Windows. `wx` ensures even an accidentally reused path is never replaced.
-      writeFileSync(target, payload, {
-        encoding: 'utf8',
-        flag: 'wx',
-        mode: 0o600,
-      })
-      return
-    } catch (error) {
-      lastError = error
-    }
-  }
-  if (lastError instanceof Error) throw lastError
-  throw new Error(errorMessage(lastError))
+  // A constrained one-shot file avoids Bun's unreliable custom stdio pipes on
+  // Windows. `wx` ensures even an accidentally reused path is never replaced.
+  // Only the validated path is written: an unvalidated fallback would let a
+  // corrupted environment place the protocol file outside the temp directory,
+  // and a rejected path now surfaces on stderr instead of losing the response.
+  writeFileSync(protocolPathFromEnv(), payload, {
+    encoding: 'utf8',
+    flag: 'wx',
+    mode: 0o600,
+  })
 }
 
 function waitForParentDisconnect(): Promise<void> {
@@ -265,6 +265,35 @@ async function readRequest(): Promise<TerminalCommandSpawnRequest> {
   return value
 }
 
+/**
+ * Deliver the write-failure marker before the detached broker kills its own
+ * process group. `process.stderr` is an asynchronous pipe on POSIX, so without
+ * waiting for the write callback the reason can be lost with the process; the
+ * wait is bounded so shell-tree containment never stalls behind it.
+ */
+function flushBrokerStderrMarker(error: unknown): Promise<void> {
+  return new Promise((resolve) => {
+    let settled = false
+    let timer: ReturnType<typeof setTimeout> | undefined
+    const finish = () => {
+      if (settled) return
+      settled = true
+      if (timer) clearTimeout(timer)
+      resolve()
+    }
+    timer = setTimeout(finish, BROKER_STDERR_FLUSH_TIMEOUT_MS)
+    try {
+      process.stderr.write(
+        `\n${BROKER_STDERR_MARKER} ${errorMessage(error)}\n`,
+        finish,
+      )
+    } catch {
+      // The pipe may already be gone; the parent still sees the missing file.
+      finish()
+    }
+  })
+}
+
 /** Run inside the detached helper process. It never initializes OpenTUI. */
 export async function serveTerminalCommandBroker(): Promise<void> {
   const parentDisconnected = waitForParentDisconnect()
@@ -301,11 +330,10 @@ export async function serveTerminalCommandBroker(): Promise<void> {
   } catch (error) {
     // Never vanish silently: relay the write failure on stderr so the parent
     // can explain the missing protocol instead of a generic ENOENT message.
-    try {
-      process.stderr.write(`\n${BROKER_STDERR_MARKER} ${errorMessage(error)}\n`)
-    } catch {
-      // The pipe may already be gone; the parent still sees the missing file.
-    }
+    await flushBrokerStderrMarker(error)
+    // Reap here explicitly: a failed write must not leave the detached shell
+    // tree alive any longer than a successful one.
+    return reapOwnProcessGroup()
   }
 
   // Normal cleanup belongs to this detached process. In particular, Windows