rogee/superpowers
1
0
Fork 0
mirror of https://github.com/obra/superpowers.git synced 2026-06-12 21:59:04 +08:00
Code Issues Packages Projects Releases Wiki Activity

fix(brainstorm-server): address adversarial review findings

Browse Source 
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.
This commit is contained in:
Jesse Vincent
2026-06-09 15:59:59 -07:00
parent 7b815ed8c8
commit f8f87ff43a
5 changed files with 109 additions and 14 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
skills/brainstorming/scripts/frame-template.html
View File

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

3
skills/brainstorming/scripts/helper.js
View File

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

35
skills/brainstorming/scripts/server.cjs
View File

@@ -278,14 +278,21 @@ function maybeOpenBrowser() {
if (HOST !== '127.0.0.1' && HOST !== 'localhost') return; if (HOST !== '127.0.0.1' && HOST !== 'localhost') return;
if (clients.size > 0) return; // the user already opened it if (clients.size > 0) return; // the user already opened it
const url = 'http://' + URL_HOST + ':' + PORT; const url = 'http://' + URL_HOST + ':' + PORT;
let cmd = process.env.BRAINSTORM_OPEN_CMD; const cp = require('child_process');
if (!cmd) { // Operator-provided launcher: run as given (this env var is trusted operator input).
if (process.platform === 'darwin') cmd = 'open'; if (process.env.BRAINSTORM_OPEN_CMD) {
else if (/microsoft/i.test(require('os').release())) cmd = 'cmd.exe /c start ""'; // WSL → Windows browser try { cp.exec(process.env.BRAINSTORM_OPEN_CMD + ' ' + JSON.stringify(url), () => {}); } catch (e) { /* best effort */ }
else if (process.env.DISPLAY || process.env.WAYLAND_DISPLAY) cmd = 'xdg-open'; return;
else return; // headless: nothing to open
} }
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 ========== // ========== 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() { function onListen() {
// Record the bound port so the next restart of this session can reuse it. // Record the bound port so the next restart of this session reuses it — but
if (PORT_FILE) { // 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 */ } try { fs.writeFileSync(PORT_FILE, String(PORT)); } catch (e) { /* best effort */ }
} }
const info = JSON.stringify({ const info = JSON.stringify({
@@ -411,9 +425,6 @@ function startServer() {
fs.writeFileSync(path.join(STATE_DIR, 'server-info'), info + '\n'); 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) => { server.on('error', (err) => {
if (err.code === 'EADDRINUSE' && !triedFallback) { if (err.code === 'EADDRINUSE' && !triedFallback) {
triedFallback = true; triedFallback = true;

79
tests/brainstorm-server/helper.test.js
View File

@@ -80,5 +80,84 @@ test('reloads on recovery and on reload messages', () => {
assert(/location\.reload\(\)/.test(src), 'reloads to pick up restarted/updated content'); 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 ---`); console.log(`\n--- Results: ${passed} passed, ${failed} failed ---`);
if (failed > 0) process.exit(1); if (failed > 0) process.exit(1);

4
tests/brainstorm-server/lifecycle.test.js
View File

@@ -117,11 +117,15 @@ async function runTests() {
let outB = ''; b.stdout.on('data', d => outB += d.toString()); let outB = ''; b.stdout.on('data', d => outB += d.toString());
for (let i = 0; i < 60 && !outB.includes('server-started'); i++) await sleep(50); for (let i = 0; i < 60 && !outB.includes('server-started'); i++) await sleep(50);
const portB = firstServerStarted(outB).port; 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 }); 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.notStrictEqual(portB, 3415, 'must not bind the already-taken port');
assert(portB >= 49152, 'should fall back to a random high 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 () => { await test('auto-opens the browser once, on the first screen', async () => {