|
1 | 1 | # TODOS |
2 | 2 |
|
| 3 | +## browse server: terminal-agent teardown follow-ups (filed v1.41 via /plan-eng-review) |
| 4 | + |
| 5 | +### P3: Identity-based terminal-agent kill (replace pkill regex with PID) |
| 6 | + |
| 7 | +**What:** Record the spawned terminal-agent PID at `browse/src/cli.ts:1057` and |
| 8 | +replace `pkill -f terminal-agent\.ts` at both `cli.ts:1047` and |
| 9 | +`server.ts:1281` (now inside the `if (ownsTerminalAgent)` gate) with |
| 10 | +`process.kill(pid, signal)` against the recorded PID. |
| 11 | + |
| 12 | +**Why:** `pkill -f terminal-agent\.ts` matches by command-line regex, so today |
| 13 | +it can kill ANY process whose argv contains `terminal-agent.ts` — sibling |
| 14 | +gstack sessions, editor processes that have the file open, a second gstack |
| 15 | +run on the same host. Latent footgun for the CLI path, not just embedders. |
| 16 | + |
| 17 | +**Pros:** Removes a real cross-session foot-cannon. PID-based kill is the |
| 18 | +correct identity primitive. Lets us tighten `pkill -f`'s broad-match warning |
| 19 | +in the new `ownsTerminalAgent` JSDoc to "historical" rather than "current". |
| 20 | + |
| 21 | +**Cons:** Requires threading the PID through the CLI-to-server state path |
| 22 | +(currently the parent server reads `terminal-port` to discover the agent; it |
| 23 | +would also need `terminal-agent-pid`). Touches `cli.ts`, `server.ts`, and |
| 24 | +`terminal-agent.ts` together — bigger surface than the v1.41 fix. |
| 25 | + |
| 26 | +**Context:** Surfaced by both Codex and Claude subagent during /autoplan |
| 27 | +review of the `ownsTerminalAgent` gate. Currently documented as out-of-scope |
| 28 | +in `browse/src/server.ts` JSDoc for `ServerConfig.ownsTerminalAgent`. The |
| 29 | +embedder fix (ownsTerminalAgent: false) means embedders don't hit this; CLI |
| 30 | +users still do. |
| 31 | + |
| 32 | +**Depends on:** None. |
| 33 | + |
| 34 | +--- |
| 35 | + |
| 36 | +### P3: shutdown() reads module-level `config`, not `cfg.config` (composition gap) |
| 37 | + |
| 38 | +**What:** `browse/src/server.ts:shutdown()` reads `path.dirname(config.stateFile)` |
| 39 | +where `config` is the module-level value resolved at import time, not the |
| 40 | +`cfg.config` passed into `buildFetchHandler`. Same gap applies to |
| 41 | +`cleanSingletonLocks(resolveChromiumProfile())` at server.ts:1298 — should |
| 42 | +read `cfg.chromiumProfile`. |
| 43 | + |
| 44 | +**Why:** Embedders today happen to share state-dir resolution with the CLI |
| 45 | +(both go through `resolveConfig()` against the same env), so this doesn't |
| 46 | +bite. But if an embedder ever passes a divergent `cfg.config` (e.g., a test |
| 47 | +harness pointing at a temp dir), shutdown will operate on the wrong paths. |
| 48 | +The `ownsTerminalAgent` flag exposes the problem without fixing it. |
| 49 | + |
| 50 | +**Pros:** Closes the embedder-composition story properly. Pairs with |
| 51 | +`cfg.chromiumProfile` to give a single coherent "this factory teardown |
| 52 | +respects cfg" contract. |
| 53 | + |
| 54 | +**Cons:** Pre-existing — not a regression. Two call sites today (1285 for |
| 55 | +terminal files, 1298 for chromium locks). Threading `cfg.config` and |
| 56 | +`cfg.chromiumProfile` into the right closures is straightforward but |
| 57 | +broader than the v1.41 fix. |
| 58 | + |
| 59 | +**Context:** Flagged by both Codex and Claude subagent in the /plan-eng-review |
| 60 | +dual voices. Documented as out-of-scope in the v1.41 plan; same shape as the |
| 61 | +`chromiumProfile` PR-body note to the gbrowser team. |
| 62 | + |
| 63 | +**Depends on:** None. |
| 64 | + |
| 65 | +--- |
| 66 | + |
| 67 | +### P3: Ownership-object refactor if a 4th caller-owned teardown gate appears |
| 68 | + |
| 69 | +**What:** Today `ServerConfig` has three caller-owned teardown gates: |
| 70 | +`xvfb?` (presence ⇒ don't close), `proxyBridge?` (same), and now |
| 71 | +`ownsTerminalAgent` (explicit boolean). If a 4th gate appears, collapse to |
| 72 | +`cfg.callerOwns?: Set<'terminalAgent' | 'xvfb' | 'proxyBridge' | ...>` or |
| 73 | +similar. |
| 74 | + |
| 75 | +**Why:** Three independent flags is below the refactor threshold — each |
| 76 | +field has clear, distinct semantics and the JSDoc voice is consistent. A |
| 77 | +fourth tips the cost balance: the per-field surface gets noisy, and |
| 78 | +"what does this factory own?" becomes a question you have to ask of three |
| 79 | +or four scattered fields instead of one explicit set. |
| 80 | + |
| 81 | +**Pros:** Single source of truth for "what gstack tears down". Trivial |
| 82 | +extension surface for future caller-owned resources. Easier to assert in |
| 83 | +tests ("the set should contain X, not Y"). |
| 84 | + |
| 85 | +**Cons:** Premature today. The polarity-inversion note in the |
| 86 | +`ownsTerminalAgent` JSDoc only hurts a little — it's one anomaly, not a |
| 87 | +pattern. Refactoring now to an ownership object would touch every embedder. |
| 88 | + |
| 89 | +**Context:** Recommended by Claude subagent during /plan-ceo-review dual |
| 90 | +voice (autoplan). Trigger: a 4th caller-owned teardown gate in this same |
| 91 | +`ServerConfig` shape. |
| 92 | + |
| 93 | +**Depends on:** A 4th gate to motivate the refactor. |
| 94 | + |
| 95 | +--- |
| 96 | + |
3 | 97 | ## /sync-gbrain memory stage perf follow-up |
4 | 98 |
|
5 | 99 | ### P2: Investigate `gbrain import` perf on large staging dirs |
|
0 commit comments