From 6955dfa3487a86a5960deaf37fd3274ec370fa96 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Mon, 17 Aug 2026 10:43:54 -0700 Subject: [PATCH] fix(pair-agent): consent before killing a healthy headless daemon MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The pair-agent headed switch spawned 'connect --force-restart' unconditionally — auto-killing a live headless daemon (open tabs, cookies, logins) in direct contradiction of the iron rule it sits beside ('only an explicit --force-restart may kill a live daemon'). The CLI now captures daemon liveness BEFORE ensureServer (which can itself boot a fresh daemon) and relaunches only when the user passed --force-restart to pair-agent; otherwise it prints the tab count and continues against the existing daemon. The /pair-agent skill gains a matching one-way-door consent question (template half rides the wave's template block). Co-Authored-By: Claude Fable 5 --- browse/src/cli.ts | 85 +++++++++++++++++------ browse/test/busy-daemon-iron-rule.test.ts | 38 ++++++++++ browse/test/pair-agent-optin-gate.test.ts | 37 ++++++++++ 3 files changed, 139 insertions(+), 21 deletions(-) diff --git a/browse/src/cli.ts b/browse/src/cli.ts index d308ff938..0827c8ae6 100644 --- a/browse/src/cli.ts +++ b/browse/src/cli.ts @@ -159,6 +159,22 @@ export async function isServerHealthy(port: number, timeoutMs = 2000): Promise { + try { + const resp = await fetch(`http://127.0.0.1:${port}/health`, { + signal: AbortSignal.timeout(timeoutMs), + }); + if (!resp.ok) return null; + const health = await resp.json() as any; + return typeof health.tabs === 'number' ? health.tabs : null; + } catch { + return null; + } +} + // ─── Process Management ───────────────────────────────────────── async function killServer(pid: number): Promise { if (!isProcessAlive(pid)) return; @@ -1631,6 +1647,19 @@ Refs: After 'snapshot', use @e1, @e2... as selectors: commandArgs.push(stdin.trim()); } + // #2219 IRON RULE (pair-agent leg): capture whether a LIVE daemon predates + // this invocation BEFORE ensureServer() can start a fresh one. pair-agent's + // headed switch below replaces the daemon via `connect --force-restart` — + // a kill that loses tabs/cookies/logins — so a PRE-EXISTING live daemon may + // only be replaced with the user's explicit --force-restart consent. A + // daemon that ensureServer just booted for this invocation holds no session + // state, so replacing it kills nothing the user had. + let pairAgentPreexistingDaemonAlive = false; + if (command === 'pair-agent') { + const preState = readState(); + pairAgentPreexistingDaemonAlive = Boolean(preState?.pid && isProcessAlive(preState.pid)); + } + let state = await ensureServer(globalFlags); // ─── Pair-Agent (post-server, pre-dispatch) ────────────── @@ -1638,28 +1667,42 @@ Refs: After 'snapshot', use @e1, @e2... as selectors: // Ensure headed mode — the user should see the browser window // when sharing it with another agent. Feels safer, more impressive. if (state.mode !== 'headed' && !hasFlag(commandArgs, '--headless')) { - console.log('[browse] Opening GStack Browser so you can see what the remote agent does...'); - // In compiled binaries, process.argv[1] is /$bunfs/... (virtual). - // Use process.execPath which is the real binary on disk. - const browseBin = process.execPath; - // --force-restart: the headed switch is this command's explicit purpose - // (the user asked to SEE the shared browser), and connect's #2219 guard - // would otherwise refuse to replace the healthy headless daemon. - const connectProc = Bun.spawn([browseBin, 'connect', '--force-restart'], { - windowsHide: true, - cwd: process.cwd(), - stdio: ['ignore', 'inherit', 'inherit'], - // Disable parent-PID monitoring: pair-agent needs the server to outlive - // the connect subprocess. Setting to 0 tells the server not to self-terminate. - env: { ...process.env, BROWSE_PARENT_PID: '0' }, - }); - await connectProc.exited; - // Re-read state after headed mode switch - const newState = readState(); - if (newState && await isServerHealthy(newState.port)) { - state = newState as ServerState; + if (pairAgentPreexistingDaemonAlive && !globalFlags.forceRestart) { + // #2219 IRON RULE: only an explicit --force-restart may kill a live + // daemon. The headed switch is nice-to-have; the user's open tabs, + // cookies, and logins are not. Continue against the live headless + // daemon and tell the user how to opt into the headed relaunch. + const tabCount = await fetchDaemonTabCount(state.port); + const tabsPhrase = tabCount === null + ? 'open tabs' + : `${tabCount} tab${tabCount === 1 ? '' : 's'}`; + console.warn(`[browse] Live headless daemon has ${tabsPhrase}; continuing against it — pass --force-restart to relaunch headed, losing tabs/cookies.`); } else { - console.warn('[browse] Could not switch to headed mode. Continuing headless.'); + console.log('[browse] Opening GStack Browser so you can see what the remote agent does...'); + // In compiled binaries, process.argv[1] is /$bunfs/... (virtual). + // Use process.execPath which is the real binary on disk. + const browseBin = process.execPath; + // --force-restart: reaching this branch means either no live daemon + // predated this invocation (nothing of the user's dies) or the user + // explicitly passed --force-restart to pair-agent (consent given). + // connect's #2219 guard would otherwise refuse to replace the + // healthy headless daemon ensureServer just returned. + const connectProc = Bun.spawn([browseBin, 'connect', '--force-restart'], { + windowsHide: true, + cwd: process.cwd(), + stdio: ['ignore', 'inherit', 'inherit'], + // Disable parent-PID monitoring: pair-agent needs the server to outlive + // the connect subprocess. Setting to 0 tells the server not to self-terminate. + env: { ...process.env, BROWSE_PARENT_PID: '0' }, + }); + await connectProc.exited; + // Re-read state after headed mode switch + const newState = readState(); + if (newState && await isServerHealthy(newState.port)) { + state = newState as ServerState; + } else { + console.warn('[browse] Could not switch to headed mode. Continuing headless.'); + } } } await handlePairAgent(state, commandArgs); diff --git a/browse/test/busy-daemon-iron-rule.test.ts b/browse/test/busy-daemon-iron-rule.test.ts index 53149cb31..57aa7777c 100644 --- a/browse/test/busy-daemon-iron-rule.test.ts +++ b/browse/test/busy-daemon-iron-rule.test.ts @@ -146,6 +146,44 @@ describe('#2219 iron rule (CLI integration)', () => { } }, 30_000); + test('pair-agent over a live headless daemon WITHOUT --force-restart → daemon survives, notice printed, no headed relaunch', async () => { + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'browse-iron-')); + const stateFile = path.join(tmpDir, 'browse.json'); + const daemon = await startHealthyDaemon(); + try { + pidChild = spawn('sleep', ['60'], { stdio: 'ignore' }); + const daemonPid = pidChild.pid!; + const stateContent = { + pid: daemonPid, + port: daemon.port, + token: 'iron-rule-token', + startedAt: new Date().toISOString(), + serverPath: '', + mode: 'launched' as const, + }; + fs.writeFileSync(stateFile, JSON.stringify(stateContent, null, 2)); + + // Exit code is NOT asserted: the fake daemon answers /pair with + // non-JSON so handlePairAgent fails later — the iron rule under test + // is everything that happens BEFORE that: no kill, no headed relaunch. + const result = await runCli(['pair-agent'], baseEnv(stateFile)); + + // The consent notice: live session named, opt-in flag named. + expect(result.stderr).toContain('continuing against it'); + expect(result.stderr).toContain('--force-restart'); + // No headed relaunch was attempted. + const combined = result.stdout + result.stderr; + expect(combined).not.toContain('Opening GStack Browser'); + // THE IRON RULE: the daemon process was not killed. + expect(isProcessAlive(daemonPid)).toBe(true); + // And the state file was not clobbered. + expect(JSON.parse(fs.readFileSync(stateFile, 'utf-8'))).toEqual(stateContent); + } finally { + await daemon.close(); + fs.rmSync(tmpDir, { recursive: true, force: true }); + } + }, 30_000); + test('wedged-alive daemon + plain command → busy report + nonzero exit, NO kill', async () => { const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'browse-iron-')); const stateFile = path.join(tmpDir, 'browse.json'); diff --git a/browse/test/pair-agent-optin-gate.test.ts b/browse/test/pair-agent-optin-gate.test.ts index cd795a989..7e89d4b42 100644 --- a/browse/test/pair-agent-optin-gate.test.ts +++ b/browse/test/pair-agent-optin-gate.test.ts @@ -122,3 +122,40 @@ describe('gate wiring — every tunnel activation point consults the guard', () expect(SERVER_SRC).toContain("process.env.BROWSE_TUNNEL === '1' && !isPairAgentEnabled()"); }); }); + +describe('pair-agent headed switch — #2219 iron-rule consent gate', () => { + // The behavioral leg (live daemon + no flag → notice, no kill) lives in + // busy-daemon-iron-rule.test.ts. These source pins cover the wiring the + // integration test can't exercise cheaply: the explicit-flag path and the + // capture-before-ensureServer ordering. + + test('headed relaunch of a pre-existing live daemon is gated on explicit --force-restart', () => { + const gateAt = CLI_SRC.indexOf('if (pairAgentPreexistingDaemonAlive && !globalFlags.forceRestart) {'); + expect(gateAt).toBeGreaterThan(-1); + // Refusal branch: notice printed, connect never spawned. + const elseAt = CLI_SRC.indexOf('} else {', gateAt); + expect(elseAt).toBeGreaterThan(gateAt); + const refusalBranch = CLI_SRC.slice(gateAt, elseAt); + expect(refusalBranch).toContain('continuing against it'); + expect(refusalBranch).toContain('--force-restart to relaunch headed'); + expect(refusalBranch).not.toContain('Bun.spawn'); + // Consented branch (no pre-existing daemon OR explicit flag): the spawn + // of `connect --force-restart` lives here and ONLY here. + const branchEnd = CLI_SRC.indexOf('await handlePairAgent(state, commandArgs);', gateAt); + expect(branchEnd).toBeGreaterThan(elseAt); + const consentedBranch = CLI_SRC.slice(elseAt, branchEnd); + expect(consentedBranch).toContain("Bun.spawn([browseBin, 'connect', '--force-restart']"); + // No second spawn site outside the gated block. + expect(CLI_SRC.indexOf("'connect', '--force-restart'")).toBe(CLI_SRC.lastIndexOf("'connect', '--force-restart'")); + }); + + test('pre-existing liveness is captured BEFORE ensureServer can boot a fresh daemon', () => { + // If the capture ran after ensureServer, a freshly-booted daemon would be + // indistinguishable from a session the user cares about — the gate would + // then refuse the headed switch even on a clean machine. + const captureAt = CLI_SRC.indexOf('pairAgentPreexistingDaemonAlive = Boolean(preState?.pid && isProcessAlive(preState.pid));'); + const ensureAt = CLI_SRC.indexOf('let state = await ensureServer(globalFlags);'); + expect(captureAt).toBeGreaterThan(-1); + expect(ensureAt).toBeGreaterThan(captureAt); + }); +});