v1.42.1.0 feat: gate terminal-agent teardown on ServerConfig.ownsTerminalAgent (unblocks gbrowser embedder) (garrytan#1615)

* feat: gate terminal-agent teardown on ServerConfig.ownsTerminalAgent

Adds ownsTerminalAgent?: boolean to ServerConfig (default true). Wraps the
three shutdown side effects (pkill -f terminal-agent\.ts + 2 safeUnlinkQuiet
calls for terminal-port and terminal-internal-token) inside a single
if (ownsTerminalAgent) block. Embedders (gbrowser phoenix overlay) pass
false to keep their own PTY lifecycle intact across gstack's teardown.

CLI start() call site passes ownsTerminalAgent: true explicitly; static-grep
test in the new test file catches a refactor that drops it.

Strict opt-out: only explicit false flips the gate (cfg.ownsTerminalAgent
=== false ? false : true). Defends against JS callers passing truthy non-bool
values.

Adds __resetShuttingDown test-only export mirroring __resetRegistry. The
module-scoped isShuttingDown latch otherwise silently no-ops a second
shutdown() in the same process.

Drops dead try/catch wrappers around safeUnlinkQuiet inside the new gate —
safeUnlinkQuiet already swallows all errors internally.

New test file (4 cases) stubs both process.exit AND child_process.spawnSync
so a real pkill -f terminal-agent\.ts never fires on the developer machine.
beforeAll/afterAll save and restore real-daemon file contents in the state
dir so the test cannot clobber a running gstack session.

* chore: file followup TODOs (identity-based pkill, cfg.config composition gap, ownership-object trigger)

Three P3 followups surfaced by /autoplan + /plan-eng-review while reviewing
the ownsTerminalAgent gate:

- Identity-based terminal-agent kill: pkill -f terminal-agent\.ts is a latent
  CLI footgun (regex match kills sibling gstack sessions, editor processes,
  etc.). Replace with PID-tracked process.kill at both cli.ts:1047 and
  server.ts:1281.

- shutdown() reads module-level config, not cfg.config (pre-existing
  composition gap). Same gap applies to cleanSingletonLocks(resolveChromiumProfile())
  at server.ts:1298 (should be cfg.chromiumProfile). Both are followup work
  for the embedder-composition story.

- 4th caller-owned teardown gate trigger: today ServerConfig has 3 (xvfb?,
  proxyBridge?, ownsTerminalAgent). If a 4th appears, collapse to
  cfg.callerOwns?: Set<...> ownership object.

* chore: bump version and changelog (v1.42.1.0)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* docs: note ServerConfig.ownsTerminalAgent in CLAUDE.md sidebar block

Adds a one-paragraph reference for the v1.42.1.0 embedder teardown gate
right after the Sidebar architecture block. Covers default semantics,
when embedders must pass `false`, polarity inversion vs xvfb?/proxyBridge?,
and the static-grep CI test that pins the CLI call site.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: garrytan

CHANGELOG.md

@@ -1,5 +1,47 @@
 # Changelog
 
+## [1.42.1.0] - 2026-05-19
+
+## **Embedder PTY teardown stops clobbering — gbrowser's phoenix overlay survives every shutdown.**
+## **`buildFetchHandler` gains an explicit ownership flag for terminal-agent files; CLI behavior preserved bit-for-bit.**
+
+`browse/src/server.ts` factory shutdown unconditionally killed the terminal-agent and unlinked its discovery files on every teardown. Correct for gstack's CLI path, wrong for embedders that pass their own pre-launched `BrowserManager` and run their own PTY server. Their `terminal-port` file got clobbered every cycle, `/health.terminalPort` reported null until the overlay rewrote it. gbrowser's phoenix overlay shipped a client-side mitigation; with this PR landed, that mitigation becomes redundant. The new `ServerConfig.ownsTerminalAgent?: boolean` (default `true`) gates the three teardown side effects together: `pkill -f terminal-agent\.ts`, `safeUnlinkQuiet(<stateDir>/terminal-port)`, `safeUnlinkQuiet(<stateDir>/terminal-internal-token)`. Embedders pass `false` to keep their PTY lifecycle intact.
+
+### The numbers that matter
+
+Source: `bun test browse/test/server-embedder-terminal-port.test.ts browse/test/server-factory.test.ts` — 32 tests, all green. Static-grep test pins the CLI `start()` call site so a refactor that drops the explicit `: true` fails CI.
+
+| Surface | Before | After |
+|---|---|---|
+| gbrowser phoenix overlay teardown | `terminal-port` unlinked every cycle; `/health.terminalPort: null` until overlay rewrites; client-side mitigation required | Pass `ownsTerminalAgent: false` — files untouched, embedder owns full lifecycle |
+| gstack CLI shutdown | `pkill` + 2 unlinks fire | Identical (default `true`, explicit `: true` at `start()` call site documents intent + static-grep test) |
+| Test runner safety | n/a | `spawnSync` stubbed in all 4 cases so real `pkill -f terminal-agent\.ts` cannot run on developer machine |
+| Multi-case shutdown tests | Module-scoped `isShuttingDown` silently no-ops 2nd shutdown | New `__resetShuttingDown` test-only export mirrors `__resetRegistry` precedent |
+| Real-daemon collision risk | Test mutates `~/.gstack/.../terminal-port` — would clobber a running developer daemon | `beforeAll` saves real contents, `afterAll` restores; tests safe to run while gstack is alive |
+
+### What this means for builders
+
+If you embed gstack's `buildFetchHandler` and run your own PTY server, pass `ownsTerminalAgent: false` in your cfg and your `terminal-port`/`terminal-internal-token` files survive every gstack teardown — no more client-side rewrite mitigation. If you use the gstack CLI, nothing changes. The flag is the third caller-owned teardown gate in `ServerConfig` (joining `xvfb?` and `proxyBridge?`); if a fourth appears we collapse to an ownership object.
+
+### Itemized changes
+
+**Added**
+- `ServerConfig.ownsTerminalAgent?: boolean` in `browse/src/server.ts` (default `true`). JSDoc enumerates all three gated side effects, the pkill regex breadth caveat, and the polarity inversion vs `xvfb?`/`proxyBridge?` (which gate by *presence* of caller-owned handles)
+- `__resetShuttingDown()` test-only export in `browse/src/server.ts`, mirroring `__resetRegistry` precedent in `token-registry.ts`. JSDoc warns about production-import footgun
+- `browse/test/server-embedder-terminal-port.test.ts` (4 tests): `ownsTerminalAgent: false` preserves files + skips pkill, explicit `true` deletes + invokes pkill, unset defaults to `true`, static-grep test asserts CLI call site documents intent. Tests save+restore real-daemon `terminal-port`/`terminal-internal-token` contents in `beforeAll`/`afterAll` so a running developer session is never clobbered
+
+**Changed**
+- `buildFetchHandler` JSDoc references the new field alongside `beforeRoute` and `browserManager` in the embedder-composition paragraph
+- CLI `start()` call site explicitly passes `ownsTerminalAgent: true` with a comment pointing at `cli.ts:1037-1063`. Documents intent + caught by the new static-grep test if a refactor drops it
+- Strict opt-out semantics: `cfg.ownsTerminalAgent === false ? false : true` — only explicit `false` flips the gate. Defends against JS callers bypassing TS and passing truthy non-bool values
+
+**Removed**
+- Dead `try { safeUnlinkQuiet(...) } catch {}` wrappers inside the new gate. `safeUnlinkQuiet` already swallows all errors internally; the outer try/catch was slop-scan flagged dead code
+
+**For contributors**
+- Followup TODOs filed in `TODOS.md`: identity-based terminal-agent kill (replace `pkill -f` with PID-tracked `process.kill`), the pre-existing `shutdown()` reads module-level `config` (composition gap with parallel `chromiumProfile` gap), and the 4th-gate-collapse-to-ownership-object trigger
+- Plan + reviews under `~/.gstack/projects/garrytan-gstack/`: autoplan CEO + Eng dual voices (Codex + Claude subagent), interactive `/plan-eng-review` (D3: drop dead try/catch), `/ship` adversarial pass (strict-bool + JSDoc hardening + test save/restore)
+
 ## [1.42.0.0] - 2026-05-19
 
 ## **Daegu wave: 23 community-filed bugs land as one bisect-clean PR with the documented sidebar security stack finally enforced.**

CLAUDE.md

@@ -236,6 +236,20 @@ Activity / Refs / Inspector as debug overlays behind the footer's
 flow, dual-token model, and threat-model boundary — silent failures
 here usually trace to not understanding the cross-component flow.
 
+**Embedder terminal-agent ownership** (v1.42.1.0+). `buildFetchHandler`
+in `browse/src/server.ts` accepts `ServerConfig.ownsTerminalAgent?:
+boolean` (default `true`). When `true`, factory shutdown runs the full
+teardown: `pkill -f terminal-agent\.ts` plus `safeUnlinkQuiet` on
+`<stateDir>/terminal-port` and `<stateDir>/terminal-internal-token`.
+Embedders (e.g. the gbrowser phoenix overlay) that pre-launch their
+own PTY server must pass `false` so their discovery files survive
+gstack teardown cycles. The flag is the third caller-owned teardown
+gate in `ServerConfig` (alongside `xvfb?` and `proxyBridge?`); polarity
+is inverted (explicit bool vs presence) and documented in the field's
+JSDoc. CLI `start()` always passes `true` explicitly — the static-grep
+test in `browse/test/server-embedder-terminal-port.test.ts` fails CI
+if a refactor drops it.
+
 **WebSocket auth uses Sec-WebSocket-Protocol, not cookies.** Browsers
 can't set `Authorization` on a WebSocket upgrade, but they CAN set
 `Sec-WebSocket-Protocol` via `new WebSocket(url, [token])`. The agent

TODOS.md

@@ -1,5 +1,99 @@
 # TODOS
 
+## browse server: terminal-agent teardown follow-ups (filed v1.41 via /plan-eng-review)
+
+### P3: Identity-based terminal-agent kill (replace pkill regex with PID)
+
+**What:** Record the spawned terminal-agent PID at `browse/src/cli.ts:1057` and
+replace `pkill -f terminal-agent\.ts` at both `cli.ts:1047` and
+`server.ts:1281` (now inside the `if (ownsTerminalAgent)` gate) with
+`process.kill(pid, signal)` against the recorded PID.
+
+**Why:** `pkill -f terminal-agent\.ts` matches by command-line regex, so today
+it can kill ANY process whose argv contains `terminal-agent.ts` — sibling
+gstack sessions, editor processes that have the file open, a second gstack
+run on the same host. Latent footgun for the CLI path, not just embedders.
+
+**Pros:** Removes a real cross-session foot-cannon. PID-based kill is the
+correct identity primitive. Lets us tighten `pkill -f`'s broad-match warning
+in the new `ownsTerminalAgent` JSDoc to "historical" rather than "current".
+
+**Cons:** Requires threading the PID through the CLI-to-server state path
+(currently the parent server reads `terminal-port` to discover the agent; it
+would also need `terminal-agent-pid`). Touches `cli.ts`, `server.ts`, and
+`terminal-agent.ts` together — bigger surface than the v1.41 fix.
+
+**Context:** Surfaced by both Codex and Claude subagent during /autoplan
+review of the `ownsTerminalAgent` gate. Currently documented as out-of-scope
+in `browse/src/server.ts` JSDoc for `ServerConfig.ownsTerminalAgent`. The
+embedder fix (ownsTerminalAgent: false) means embedders don't hit this; CLI
+users still do.
+
+**Depends on:** None.
+
+---
+
+### P3: shutdown() reads module-level `config`, not `cfg.config` (composition gap)
+
+**What:** `browse/src/server.ts:shutdown()` reads `path.dirname(config.stateFile)`
+where `config` is the module-level value resolved at import time, not the
+`cfg.config` passed into `buildFetchHandler`. Same gap applies to
+`cleanSingletonLocks(resolveChromiumProfile())` at server.ts:1298 — should
+read `cfg.chromiumProfile`.
+
+**Why:** Embedders today happen to share state-dir resolution with the CLI
+(both go through `resolveConfig()` against the same env), so this doesn't
+bite. But if an embedder ever passes a divergent `cfg.config` (e.g., a test
+harness pointing at a temp dir), shutdown will operate on the wrong paths.
+The `ownsTerminalAgent` flag exposes the problem without fixing it.
+
+**Pros:** Closes the embedder-composition story properly. Pairs with
+`cfg.chromiumProfile` to give a single coherent "this factory teardown
+respects cfg" contract.
+
+**Cons:** Pre-existing — not a regression. Two call sites today (1285 for
+terminal files, 1298 for chromium locks). Threading `cfg.config` and
+`cfg.chromiumProfile` into the right closures is straightforward but
+broader than the v1.41 fix.
+
+**Context:** Flagged by both Codex and Claude subagent in the /plan-eng-review
+dual voices. Documented as out-of-scope in the v1.41 plan; same shape as the
+`chromiumProfile` PR-body note to the gbrowser team.
+
+**Depends on:** None.
+
+---
+
+### P3: Ownership-object refactor if a 4th caller-owned teardown gate appears
+
+**What:** Today `ServerConfig` has three caller-owned teardown gates:
+`xvfb?` (presence ⇒ don't close), `proxyBridge?` (same), and now
+`ownsTerminalAgent` (explicit boolean). If a 4th gate appears, collapse to
+`cfg.callerOwns?: Set<'terminalAgent' | 'xvfb' | 'proxyBridge' | ...>` or
+similar.
+
+**Why:** Three independent flags is below the refactor threshold — each
+field has clear, distinct semantics and the JSDoc voice is consistent. A
+fourth tips the cost balance: the per-field surface gets noisy, and
+"what does this factory own?" becomes a question you have to ask of three
+or four scattered fields instead of one explicit set.
+
+**Pros:** Single source of truth for "what gstack tears down". Trivial
+extension surface for future caller-owned resources. Easier to assert in
+tests ("the set should contain X, not Y").
+
+**Cons:** Premature today. The polarity-inversion note in the
+`ownsTerminalAgent` JSDoc only hurts a little — it's one anomaly, not a
+pattern. Refactoring now to an ownership object would touch every embedder.
+
+**Context:** Recommended by Claude subagent during /plan-ceo-review dual
+voice (autoplan). Trigger: a 4th caller-owned teardown gate in this same
+`ServerConfig` shape.
+
+**Depends on:** A 4th gate to motivate the refactor.
+
+---
+
 ## /sync-gbrain memory stage perf follow-up
 
 ### P2: Investigate `gbrain import` perf on large staging dirs

VERSION

@@ -1 +1 @@
-1.42.0.0
+1.42.1.0

browse/src/server.ts

@@ -205,6 +205,35 @@ export interface ServerConfig {
    * dispatch; returning null falls through.
    */
   beforeRoute?: (req: Request, surface: Surface, auth: TokenInfo | null) => Promise<Response | null>;
+  /**
+   * Whether gstack owns the lifecycle of the terminal-agent process and its
+   * discovery files (`<stateDir>/terminal-port`, `<stateDir>/terminal-internal-token`).
+   *
+   * When true (default), shutdown() runs three side effects:
+   *   1. `pkill -f terminal-agent\.ts`  — regex-broad, matches ANY process whose
+   *      command line contains `terminal-agent.ts` on this host (including
+   *      sibling gstack sessions). Pre-existing CLI behavior, not introduced by
+   *      this flag. Identity-based PID kill is a separate followup (see TODOS).
+   *   2. `safeUnlinkQuiet(<stateDir>/terminal-port)`
+   *   3. `safeUnlinkQuiet(<stateDir>/terminal-internal-token)`
+   *
+   * This is correct for gstack's CLI path, which spawns `terminal-agent.ts` as
+   * the producer of those files (see cli.ts:1037-1063).
+   *
+   * Embedders (gbrowser phoenix overlay, future hosts) that run their own PTY
+   * server and write those files themselves should pass `false`. When `false`,
+   * the embedder owns BOTH the agent process AND both discovery files —
+   * terminal-agent.ts's own SIGTERM cleanup only removes `terminal-port`
+   * (see terminal-agent.ts:558), so the internal-token file is the embedder's
+   * full responsibility.
+   *
+   * Polarity note: this differs from `xvfb?` and `proxyBridge?`, which gate by
+   * the *presence* of a caller-owned handle (presence ⇒ don't close). This
+   * field gates by an explicit boolean because there is no handle object —
+   * the terminal-agent is started elsewhere (cli.ts), and shutdown's only
+   * reference is the regex-based pkill + the file paths.
+   */
+  ownsTerminalAgent?: boolean;
 }
 
 /**
@@ -1229,8 +1258,11 @@ if (import.meta.main) {
 /**
  * Build a request handler set for the browse daemon. Embedders (gbrowser
  * phoenix overlay) call this directly with their own cfg to compose overlay
- * routes via cfg.beforeRoute. The CLI path calls it through start() with
- * env-derived defaults — externally-observable behavior is identical.
+ * routes via cfg.beforeRoute, pass a pre-launched cfg.browserManager, and
+ * opt out of terminal-agent teardown via cfg.ownsTerminalAgent (default
+ * true, set to false when the embedder runs its own PTY server). The CLI
+ * path calls this through start() with env-derived defaults and explicit
+ * cfg.ownsTerminalAgent: true — externally-observable behavior is identical.
  *
  * Auth state lives ENTIRELY inside the factory closure: cfg.authToken is the
  * single source of truth for the bearer secret, factory-scoped validateAuth
@@ -1260,6 +1292,11 @@ export function buildFetchHandler(cfg: ServerConfig): ServerHandle {
   initRegistry(cfg.authToken);
 
   const { authToken, browserManager: cfgBrowserManager, startTime, beforeRoute, browsePort } = cfg;
+  // Strict opt-out: only explicit `false` flips the gate. Any other value
+  // (undefined, truthy non-bool from a JS caller bypassing TS, etc.) defaults
+  // to gstack-owns. Matches the "default-true preserves CLI bit-for-bit"
+  // premise even under malformed cfg.
+  const ownsTerminalAgent = cfg.ownsTerminalAgent === false ? false : true;
 
   // Factory-scoped validateAuth. Closes over cfg.authToken so every internal
   // auth check sees the same token the routes receive. Module-level
@@ -1277,14 +1314,16 @@ export function buildFetchHandler(cfg: ServerConfig): ServerHandle {
     isShuttingDown = true;
 
     console.log('[browse] Shutting down...');
-    try {
-      const { spawnSync } = require('child_process');
-      spawnSync('pkill', ['-f', 'terminal-agent\\.ts'], { stdio: 'ignore', timeout: 3000 });
-    } catch (err: any) {
-      console.warn('[browse] Failed to kill terminal-agent:', err.message);
+    if (ownsTerminalAgent) {
+      try {
+        const { spawnSync } = require('child_process');
+        spawnSync('pkill', ['-f', 'terminal-agent\\.ts'], { stdio: 'ignore', timeout: 3000 });
+      } catch (err: any) {
+        console.warn('[browse] Failed to kill terminal-agent:', err.message);
+      }
+      safeUnlinkQuiet(path.join(path.dirname(config.stateFile), 'terminal-port'));
+      safeUnlinkQuiet(path.join(path.dirname(config.stateFile), 'terminal-internal-token'));
     }
-    try { safeUnlinkQuiet(path.join(path.dirname(config.stateFile), 'terminal-port')); } catch {}
-    try { safeUnlinkQuiet(path.join(path.dirname(config.stateFile), 'terminal-internal-token')); } catch {}
     try { detachSession(); } catch (err: any) {
       console.warn('[browse] Failed to detach CDP session:', err.message);
     }
@@ -2541,6 +2580,7 @@ export async function start() {
     xvfb,
     proxyBridge,
     startTime,
+    ownsTerminalAgent: true, // CLI spawns terminal-agent.ts itself (see cli.ts:1037-1063)
   });
 
   const server = Bun.serve({
@@ -2686,6 +2726,23 @@ export async function start() {
   }
 }
 
+/**
+ * Test-only. Resets the module-level shutdown latch so a second test case
+ * can exercise shutdown() in the same process. Mirrors __resetRegistry in
+ * token-registry.ts. shutdown() short-circuits when isShuttingDown is true
+ * (see line near the start of shutdown), so without this, tests that call
+ * shutdown() more than once silently no-op after the first call.
+ *
+ * DO NOT call from production code. Defeats the shutdown re-entry guard,
+ * which can race process.exit with cfgBrowserManager.close() and the pkill /
+ * safeUnlinkQuiet side effects. The `__` prefix is the convention; nothing
+ * enforces it. If you find yourself reaching for this outside a test file,
+ * the right fix is to make isShuttingDown factory-scoped instead.
+ */
+export function __resetShuttingDown(): void {
+  isShuttingDown = false;
+}
+
 // Auto-kickoff only when this module is the entry point. Embedders
 // (gbrowser phoenix overlay) import { start, buildFetchHandler, ... }
 // without triggering the listener-binding side effects.