From 5b55be1f7e0bf82829dbe923840792c4ab376a80 Mon Sep 17 00:00:00 2001 From: Denis Zjukow Date: Fri, 28 Aug 2026 11:53:31 +0200 Subject: [PATCH] fix(open-gstack-browser): pre-flight cleanup never killed the stale daemon MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Step 0 read the old pid with `grep -o '"pid":[0-9]*'` and Step 2 read the port the same way. Neither can match. Every writer of that file in browse/src/server.ts serializes with `JSON.stringify(state, null, 2)`, so the bytes on disk are `"pid": 12060` — colon, space, digits. The failure was silent in the worst way. `_OLD_PID` came back empty, the kill never ran, browse.json was deleted anyway, and the next `connect` died with "existing daemon has different config (proxy/headed mismatch)" — an error pointing at proxy/headed flags rather than at the cleanup that no-opped. Caught against a daemon left over from a reboot: the operator was told to check flags they had never passed. Both patterns now accept optional whitespace. The new tripwire does not match strings — it RUNS the snippets the skill hands the agent, against a state file written exactly the way the server writes one, and asserts pid and port come back out. A third case pins the coupling to `JSON.stringify(state, null, 2)`, so a switch to compact JSON surfaces as a failing expectation rather than as silence. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0111Mq3JGwZDcstn5wYcbhSw --- open-gstack-browser/SKILL.md | 4 +- open-gstack-browser/SKILL.md.tmpl | 4 +- test/skill-browse-state-extraction.test.ts | 77 ++++++++++++++++++++++ 3 files changed, 81 insertions(+), 4 deletions(-) create mode 100644 test/skill-browse-state-extraction.test.ts diff --git a/open-gstack-browser/SKILL.md b/open-gstack-browser/SKILL.md index 44b569087..846d90243 100644 --- a/open-gstack-browser/SKILL.md +++ b/open-gstack-browser/SKILL.md @@ -210,7 +210,7 @@ positives and Chromium profile lock conflicts. ```bash # Kill any existing browse server if [ -f "$(git rev-parse --show-toplevel 2>/dev/null)/.gstack/browse.json" ]; then - _OLD_PID=$(cat "$(git rev-parse --show-toplevel)/.gstack/browse.json" 2>/dev/null | grep -o '"pid":[0-9]*' | grep -o '[0-9]*') + _OLD_PID=$(cat "$(git rev-parse --show-toplevel)/.gstack/browse.json" 2>/dev/null | grep -o '"pid":[[:space:]]*[0-9]*' | grep -o '[0-9]*') [ -n "$_OLD_PID" ] && kill "$_OLD_PID" 2>/dev/null || true sleep 1 [ -n "$_OLD_PID" ] && kill -9 "$_OLD_PID" 2>/dev/null || true @@ -255,7 +255,7 @@ $B status Confirm the output shows `Mode: headed`. Read the port from the state file: ```bash -cat "$(git rev-parse --show-toplevel 2>/dev/null)/.gstack/browse.json" 2>/dev/null | grep -o '"port":[0-9]*' | grep -o '[0-9]*' +cat "$(git rev-parse --show-toplevel 2>/dev/null)/.gstack/browse.json" 2>/dev/null | grep -o '"port":[[:space:]]*[0-9]*' | grep -o '[0-9]*' ``` The port should be **34567**. If it's different, note it — the user may need it diff --git a/open-gstack-browser/SKILL.md.tmpl b/open-gstack-browser/SKILL.md.tmpl index 67369cb30..bb6b8893d 100644 --- a/open-gstack-browser/SKILL.md.tmpl +++ b/open-gstack-browser/SKILL.md.tmpl @@ -39,7 +39,7 @@ positives and Chromium profile lock conflicts. ```bash # Kill any existing browse server if [ -f "$(git rev-parse --show-toplevel 2>/dev/null)/.gstack/browse.json" ]; then - _OLD_PID=$(cat "$(git rev-parse --show-toplevel)/.gstack/browse.json" 2>/dev/null | grep -o '"pid":[0-9]*' | grep -o '[0-9]*') + _OLD_PID=$(cat "$(git rev-parse --show-toplevel)/.gstack/browse.json" 2>/dev/null | grep -o '"pid":[[:space:]]*[0-9]*' | grep -o '[0-9]*') [ -n "$_OLD_PID" ] && kill "$_OLD_PID" 2>/dev/null || true sleep 1 [ -n "$_OLD_PID" ] && kill -9 "$_OLD_PID" 2>/dev/null || true @@ -84,7 +84,7 @@ $B status Confirm the output shows `Mode: headed`. Read the port from the state file: ```bash -cat "$(git rev-parse --show-toplevel 2>/dev/null)/.gstack/browse.json" 2>/dev/null | grep -o '"port":[0-9]*' | grep -o '[0-9]*' +cat "$(git rev-parse --show-toplevel 2>/dev/null)/.gstack/browse.json" 2>/dev/null | grep -o '"port":[[:space:]]*[0-9]*' | grep -o '[0-9]*' ``` The port should be **34567**. If it's different, note it — the user may need it diff --git a/test/skill-browse-state-extraction.test.ts b/test/skill-browse-state-extraction.test.ts new file mode 100644 index 000000000..b3e781fd5 --- /dev/null +++ b/test/skill-browse-state-extraction.test.ts @@ -0,0 +1,77 @@ +import { describe, test, expect } from 'bun:test'; +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; +import { execFileSync } from 'child_process'; + +// Tripwire for the pid/port extraction snippets in /open-gstack-browser. +// +// Step 0 (pre-flight cleanup) reads the stale daemon's pid out of +// .gstack/browse.json to kill it; Step 2 reads the port back to tell the user +// which one the Side Panel needs. Both used `grep -o '"pid":[0-9]*'`, which +// cannot match: every writer of that file in browse/src/server.ts serializes +// with `JSON.stringify(state, null, 2)`, so the real bytes are `"pid": 12060` +// — colon, SPACE, digits. +// +// The failure was silent in the worst way. `_OLD_PID` came back empty, the +// `kill` never ran, browse.json was deleted anyway, and the next `connect` +// died with "existing daemon has different config (proxy/headed mismatch)" +// — an error that points at proxy/headed flags, not at the cleanup that +// no-opped. Observed 2026-08-28 against a daemon left over from a reboot. +// +// So this test does not match strings; it RUNS the snippets the skill tells +// the agent to run, against a state file written exactly the way the server +// writes one, and asserts the values come back out. + +const ROOT = path.resolve(import.meta.dir, '..'); +const SKILL = path.join(ROOT, 'open-gstack-browser', 'SKILL.md'); +const TMPL = path.join(ROOT, 'open-gstack-browser', 'SKILL.md.tmpl'); + +/** The exact shape browse/src/server.ts writes (JSON.stringify(state, null, 2)). */ +function writeStateFile(dir: string, pid: number, port: number): string { + const file = path.join(dir, 'browse.json'); + fs.writeFileSync( + file, + JSON.stringify({ pid, port, token: 'not-a-real-token', mode: 'headed' }, null, 2), + ); + return file; +} + +/** Pull the grep pipeline for `field` out of the skill prose and run it. */ +function extractViaSkill(source: string, field: 'pid' | 'port', stateFile: string): string { + const line = source + .split('\n') + .find((l) => l.includes(`grep -o '"${field}":`)); + expect(line, `no ${field} extraction line found in the skill`).toBeDefined(); + + // Keep only the pipeline itself: everything from the first `grep` on, so the + // surrounding shell (cat of a git-root path, variable assignment) does not + // have to be reproduced here. + const pipeline = line!.slice(line!.indexOf('grep -o')); + const script = `cat ${JSON.stringify(stateFile)} | ${pipeline.replace(/\)$/, '')}`; + return execFileSync('bash', ['-c', script], { encoding: 'utf-8' }).trim(); +} + +describe('/open-gstack-browser state-file extraction', () => { + for (const [label, file] of [['generated', SKILL], ['template', TMPL]] as const) { + test(`${label}: pid and port survive the pretty-printed state file`, () => { + const source = fs.readFileSync(file, 'utf-8'); + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'browse-state-')); + try { + const stateFile = writeStateFile(dir, 12060, 34567); + expect(extractViaSkill(source, 'pid', stateFile)).toBe('12060'); + expect(extractViaSkill(source, 'port', stateFile)).toBe('34567'); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + } + + test('server.ts still writes the state file pretty-printed', () => { + // If a refactor ever switches to compact JSON, the snippets above keep + // working (the pattern tolerates zero spaces too) — but the reason this + // test exists changes, so make the coupling visible instead of implicit. + const server = fs.readFileSync(path.join(ROOT, 'browse', 'src', 'server.ts'), 'utf-8'); + expect(server).toContain('JSON.stringify(state, null, 2)'); + }); +});