Răsfoiți Sursa

fix(brainstorm-server): address adversarial review findings

From a two-reviewer adversarial pass:

- [High] EADDRINUSE fallback clobbered the shared .last-port: onListen wrote the
  bound port unconditionally, so a fallback to a random port overwrote the
  preferred port another live session still owns — stranding that session's open
  tab forever. Now persist only when we bound the preferred port (not on
  fallback). The fallback test now asserts .last-port integrity (teeth-verified).

- [Medium] maybeOpenBrowser ran the URL through a shell (exec + JSON.stringify),
  which does NOT neutralize $(...) in a url-host. Platform launchers now use
  execFile with the URL as an argv element (no shell). The operator-set
  BRAINSTORM_OPEN_CMD path stays shell-based (trusted input).

- [Medium] --open was a silent no-op on native Windows (no win32 branch). Added.

- [Medium] helper.js reconnect/status/tombstone had only substring-grep tests.
  Added behavioral tests driving the state machine against a mocked browser:
  Reconnecting+backoff (500->1000->2000), tombstone after the grace period, and
  reload-on-recovery.

- [Low] status pill showed a false 'Connected' before the socket opened; now
  starts 'Connecting…' until onopen.

Not changed (flagged): stop-server.sh's PID-ownership check still matches any
'node ... server.cjs' (narrow residual — a recycled PID onto an unrelated node
server.cjs); robust fix needs fragile cross-platform process introspection.
Jesse Vincent 3 luni în urmă
părinte
comite
bac46a5dcb

+ 1 - 1
skills/brainstorming/scripts/frame-template.html

@@ -197,7 +197,7 @@
 <body>
   <div class="header">
     <h1><a href="https://github.com/obra/superpowers" style="color: inherit; text-decoration: none;">Superpowers Brainstorming</a></h1>
-    <div class="status">Connected</div>
+    <div class="status">Connecting…</div>
   </div>
 
   <div class="main">

+ 2 - 1
skills/brainstorming/scripts/helper.js

@@ -28,6 +28,7 @@
     const el = document.querySelector('.status');
     if (!el) return;
     const map = {
+      connecting:   ['Connecting…',   'var(--text-tertiary)'],
       connected:    ['Connected',     'var(--success)'],
       reconnecting: ['Reconnecting…', 'var(--warning)'],
       disconnected: ['Disconnected',  'var(--error)']
@@ -55,7 +56,7 @@
 
   function connect() {
     if (reconnectTimer) { clearTimeout(reconnectTimer); reconnectTimer = null; }
-    setStatus(everConnected ? 'reconnecting' : 'connected');
+    setStatus(everConnected ? 'reconnecting' : 'connecting');
     ws = new WebSocket(WS_URL);
 
     ws.onopen = () => {

+ 23 - 12
skills/brainstorming/scripts/server.cjs

@@ -278,14 +278,21 @@ function maybeOpenBrowser() {
   if (HOST !== '127.0.0.1' && HOST !== 'localhost') return;
   if (clients.size > 0) return; // the user already opened it
   const url = 'http://' + URL_HOST + ':' + PORT;
-  let cmd = process.env.BRAINSTORM_OPEN_CMD;
-  if (!cmd) {
-    if (process.platform === 'darwin') cmd = 'open';
-    else if (/microsoft/i.test(require('os').release())) cmd = 'cmd.exe /c start ""'; // WSL → Windows browser
-    else if (process.env.DISPLAY || process.env.WAYLAND_DISPLAY) cmd = 'xdg-open';
-    else return; // headless: nothing to open
+  const cp = require('child_process');
+  // Operator-provided launcher: run as given (this env var is trusted operator input).
+  if (process.env.BRAINSTORM_OPEN_CMD) {
+    try { cp.exec(process.env.BRAINSTORM_OPEN_CMD + ' ' + JSON.stringify(url), () => {}); } catch (e) { /* best effort */ }
+    return;
   }
-  try { require('child_process').exec(cmd + ' ' + JSON.stringify(url), () => {}); } catch (e) { /* best effort */ }
+  // Platform launchers: pass the URL as an argv element via execFile (no shell),
+  // so a url-host containing shell metacharacters can't inject a command.
+  const isWSL = process.platform === 'linux' && /microsoft/i.test(require('os').release());
+  let bin, args;
+  if (process.platform === 'darwin') { bin = 'open'; args = [url]; }
+  else if (process.platform === 'win32' || isWSL) { bin = 'cmd.exe'; args = ['/c', 'start', '', url]; }
+  else if (process.env.DISPLAY || process.env.WAYLAND_DISPLAY) { bin = 'xdg-open'; args = [url]; }
+  else return; // headless: nothing to open
+  try { cp.execFile(bin, args, () => {}); } catch (e) { /* best effort */ }
 }
 
 // ========== Activity Tracking ==========
@@ -397,9 +404,16 @@ function startServer() {
     }
   }
 
+  // If the preferred port is already taken (e.g. a previous server is still
+  // alive), fall back to a random port once instead of failing.
+  let triedFallback = false;
+
   function onListen() {
-    // Record the bound port so the next restart of this session can reuse it.
-    if (PORT_FILE) {
+    // Record the bound port so the next restart of this session reuses it — but
+    // ONLY when we got our preferred port. On a fallback we bound a *different*
+    // port because someone else holds the preferred one; persisting it would
+    // overwrite the shared .last-port and strand that other session's open tab.
+    if (PORT_FILE && !triedFallback) {
       try { fs.writeFileSync(PORT_FILE, String(PORT)); } catch (e) { /* best effort */ }
     }
     const info = JSON.stringify({
@@ -411,9 +425,6 @@ function startServer() {
     fs.writeFileSync(path.join(STATE_DIR, 'server-info'), info + '\n');
   }
 
-  // If the preferred port is already taken (e.g. a previous server is still
-  // alive), fall back to a random port once instead of failing.
-  let triedFallback = false;
   server.on('error', (err) => {
     if (err.code === 'EADDRINUSE' && !triedFallback) {
       triedFallback = true;

+ 79 - 0
tests/brainstorm-server/helper.test.js

@@ -80,5 +80,84 @@ test('reloads on recovery and on reload messages', () => {
   assert(/location\.reload\(\)/.test(src), 'reloads to pick up restarted/updated content');
 });
 
+console.log('\n--- Reconnect state machine (mocked browser) ---');
+
+// Drive helper.js's browser code against mocked DOM/WebSocket/timers/clock so we
+// can exercise the actual reconnect/status/tombstone behaviour, not just grep it.
+function makeEnv() {
+  const state = { now: 1000, timers: [], reloads: 0, appended: [] };
+  const sockets = [];
+  const statusEl = { textContent: '', style: { setProperty() {} } };
+  class FakeWS {
+    constructor(url) { this.url = url; this.readyState = 0; this.onopen = this.onclose = this.onmessage = this.onerror = null; sockets.push(this); }
+    send() {}
+    close() { this.readyState = 3; if (this.onclose) this.onclose(); }
+    open() { this.readyState = 1; if (this.onopen) this.onopen(); }
+  }
+  FakeWS.OPEN = 1;
+  const env = {
+    module: { exports: {} },
+    window: { location: { host: 'localhost:7777', reload() { state.reloads++; } } },
+    document: {
+      querySelector: (s) => s === '.status' ? statusEl : null,
+      getElementById: () => null,
+      createElement: () => ({ style: {}, id: '' }),
+      addEventListener() {},
+      body: { appendChild: (el) => state.appended.push(el) }
+    },
+    WebSocket: FakeWS,
+    setTimeout: (fn, ms) => { state.timers.push({ fn, ms, fired: false, cleared: false }); return state.timers.length; },
+    clearTimeout: (id) => { if (state.timers[id - 1]) state.timers[id - 1].cleared = true; },
+    Date: { now: () => state.now },
+    console
+  };
+  return {
+    state, statusEl, sockets,
+    boot() { new Function(...Object.keys(env), src)(...Object.values(env)); },
+    advance(ms) { state.now += ms; },
+    last() { return sockets[sockets.length - 1]; },
+    fireReconnect() {
+      const t = [...state.timers].reverse().find(x => !x.fired && !x.cleared);
+      if (!t) throw new Error('no reconnect scheduled');
+      t.fired = true; t.fn();
+    }
+  };
+}
+
+test('on disconnect shows Reconnecting and schedules a 500ms reconnect', () => {
+  const e = makeEnv(); e.boot();
+  e.last().open();
+  assert.strictEqual(e.statusEl.textContent, 'Connected');
+  e.last().close();
+  assert.strictEqual(e.statusEl.textContent, 'Reconnecting…');
+  assert.strictEqual(e.state.timers[e.state.timers.length - 1].ms, 500);
+});
+
+test('reconnect delay backs off 500 -> 1000 -> 2000', () => {
+  const e = makeEnv(); e.boot();
+  e.last().open(); e.last().close();
+  e.fireReconnect(); e.last().close();
+  e.fireReconnect(); e.last().close();
+  assert.deepStrictEqual(e.state.timers.map(t => t.ms).slice(0, 3), [500, 1000, 2000]);
+});
+
+test('shows the tombstone and Disconnected after the grace period', () => {
+  const e = makeEnv(); e.boot();
+  e.last().open(); e.last().close();
+  e.advance(20000);          // past TOMBSTONE_AFTER_MS while still down
+  e.fireReconnect(); e.last().close();
+  assert.strictEqual(e.statusEl.textContent, 'Disconnected');
+  assert.strictEqual(e.state.appended.length, 1, 'tombstone appended exactly once');
+});
+
+test('reloads to recover when a tombstoned connection comes back', () => {
+  const e = makeEnv(); e.boot();
+  e.last().open(); e.last().close();
+  e.advance(20000); e.fireReconnect(); e.last().close(); // tombstone now shown
+  assert.strictEqual(e.state.reloads, 0);
+  e.fireReconnect(); e.last().open();                    // server back (e.g. same-port restart)
+  assert.strictEqual(e.state.reloads, 1, 'reloads once on recovery');
+});
+
 console.log(`\n--- Results: ${passed} passed, ${failed} failed ---`);
 if (failed > 0) process.exit(1);

+ 4 - 0
tests/brainstorm-server/lifecycle.test.js

@@ -117,11 +117,15 @@ async function runTests() {
     let outB = ''; b.stdout.on('data', d => outB += d.toString());
     for (let i = 0; i < 60 && !outB.includes('server-started'); i++) await sleep(50);
     const portB = firstServerStarted(outB).port;
+    const persisted = fs.readFileSync(portFile, 'utf8').trim();
 
     a.kill(); b.kill(); await sleep(100); fs.rmSync(dir, { recursive: true, force: true });
 
     assert.notStrictEqual(portB, 3415, 'must not bind the already-taken port');
     assert(portB >= 49152, 'should fall back to a random high port');
+    // The fallback must NOT clobber the shared port file — A still owns 3415 and
+    // its open tab must keep reconnecting there.
+    assert.strictEqual(persisted, '3415', 'fallback must not overwrite .last-port');
   });
 
   await test('auto-opens the browser once, on the first screen', async () => {