From 626500a1a9b5ae00a17a5d19a448fa00d8aa039d Mon Sep 17 00:00:00 2001 From: Konstantin Tenman Date: Sat, 23 May 2026 10:26:50 +0300 Subject: [PATCH] Incorporate verified upstream fixes (obra/superpowers #1504, #1596, #1562) Cherry-picks three verified fixes from obra/superpowers open PRs into the fork. - #1504: guard handleMessage against null/primitive WebSocket payloads. JSON.parse('null') returns null; reading .choice on it threw a TypeError that escaped the socket 'data' handler and killed the server process. - #1596: make the brainstorm server idle timeout configurable via --idle-timeout-minutes (BRAINSTORM_IDLE_TIMEOUT_MS) and raise the default from 30 minutes to 2 hours. Surface idle_timeout_ms in server-info. - #1562: detect the package manager by lockfile in using-git-worktrees Step 3 (pnpm/yarn/bun/npm for Node, uv/poetry/pip for Python) instead of running npm install / poetry install unconditionally. Verified: brainstorm-server suite 32/32 green (includes new null-crash and idle-timeout regression tests); --idle-timeout-minutes checked end-to-end (5 -> 300000ms, default -> 7200000ms, 0/abc -> error + exit 1). --- skills/brainstorming/scripts/server.cjs | 21 +++-- skills/brainstorming/scripts/start-server.sh | 32 +++++++- skills/brainstorming/visual-companion.md | 3 +- skills/using-git-worktrees/SKILL.md | 20 ++++- tests/brainstorm-server/server.test.js | 81 ++++++++++++++++++++ 5 files changed, 144 insertions(+), 13 deletions(-) diff --git a/skills/brainstorming/scripts/server.cjs b/skills/brainstorming/scripts/server.cjs index 562c17f893..564a499575 100644 --- a/skills/brainstorming/scripts/server.cjs +++ b/skills/brainstorming/scripts/server.cjs @@ -81,6 +81,14 @@ const CONTENT_DIR = path.join(SESSION_DIR, 'content'); const STATE_DIR = path.join(SESSION_DIR, 'state'); let ownerPid = process.env.BRAINSTORM_OWNER_PID ? Number(process.env.BRAINSTORM_OWNER_PID) : null; +const DEFAULT_IDLE_TIMEOUT_MS = 2 * 60 * 60 * 1000; // 2 hours + +function getIdleTimeoutMs(value = process.env.BRAINSTORM_IDLE_TIMEOUT_MS) { + const parsed = Number(value); + if (!Number.isSafeInteger(parsed) || parsed <= 0) return DEFAULT_IDLE_TIMEOUT_MS; + return parsed; +} + const MIME_TYPES = { '.html': 'text/html', '.css': 'text/css', '.js': 'application/javascript', '.json': 'application/json', '.png': 'image/png', '.jpg': 'image/jpeg', @@ -231,7 +239,9 @@ function handleMessage(text) { } touchActivity(); console.log(JSON.stringify({ source: 'user-event', ...event })); - if (event.choice) { + // event may be null (JSON.parse('null')); property access on null throws + // and would escape the socket 'data' handler as an uncaughtException. + if (event && event.choice) { const eventsFile = path.join(STATE_DIR, 'events'); fs.appendFileSync(eventsFile, JSON.stringify(event) + '\n'); } @@ -246,7 +256,7 @@ function broadcast(msg) { // ========== Activity Tracking ========== -const IDLE_TIMEOUT_MS = 30 * 60 * 1000; // 30 minutes +const IDLE_TIMEOUT_MS = getIdleTimeoutMs(); let lastActivity = Date.now(); function touchActivity() { @@ -316,7 +326,7 @@ function startServer() { try { process.kill(ownerPid, 0); return true; } catch (e) { return e.code === 'EPERM'; } } - // Check every 60s: exit if owner process died or idle for 30 minutes + // Check every 60s: exit if owner process died or the session is idle. const lifecycleCheck = setInterval(() => { if (!ownerAlive()) shutdown('owner process exited'); else if (Date.now() - lastActivity > IDLE_TIMEOUT_MS) shutdown('idle timeout'); @@ -340,7 +350,8 @@ function startServer() { const info = JSON.stringify({ type: 'server-started', port: Number(PORT), host: HOST, url_host: URL_HOST, url: 'http://' + URL_HOST + ':' + PORT, - screen_dir: CONTENT_DIR, state_dir: STATE_DIR + screen_dir: CONTENT_DIR, state_dir: STATE_DIR, + idle_timeout_ms: IDLE_TIMEOUT_MS }); console.log(info); fs.writeFileSync(path.join(STATE_DIR, 'server-info'), info + '\n'); @@ -351,4 +362,4 @@ if (require.main === module) { startServer(); } -module.exports = { computeAcceptKey, encodeFrame, decodeFrame, OPCODES }; +module.exports = { computeAcceptKey, encodeFrame, decodeFrame, getIdleTimeoutMs, OPCODES }; diff --git a/skills/brainstorming/scripts/start-server.sh b/skills/brainstorming/scripts/start-server.sh index 9ef6dcb991..a1821db9e4 100755 --- a/skills/brainstorming/scripts/start-server.sh +++ b/skills/brainstorming/scripts/start-server.sh @@ -1,6 +1,6 @@ #!/usr/bin/env bash # Start the brainstorm server and output connection info -# Usage: start-server.sh [--project-dir ] [--host ] [--url-host ] [--foreground] [--background] +# Usage: start-server.sh [--project-dir ] [--host ] [--url-host ] [--idle-timeout-minutes ] [--foreground] [--background] # # Starts server on a random high port, outputs JSON with URL. # Each session gets its own directory to avoid conflicts. @@ -11,6 +11,8 @@ # --host Host/interface to bind (default: 127.0.0.1). # Use 0.0.0.0 in remote/containerized environments. # --url-host Hostname shown in returned URL JSON. +# --idle-timeout-minutes +# Override inactivity timeout (default: 120). # --foreground Run server in the current terminal (no backgrounding). # --background Force background mode (overrides Codex auto-foreground). @@ -22,6 +24,7 @@ FOREGROUND="false" FORCE_BACKGROUND="false" BIND_HOST="127.0.0.1" URL_HOST="" +IDLE_TIMEOUT_MINUTES="" while [[ $# -gt 0 ]]; do case "$1" in --project-dir) @@ -36,6 +39,10 @@ while [[ $# -gt 0 ]]; do URL_HOST="$2" shift 2 ;; + --idle-timeout-minutes) + IDLE_TIMEOUT_MINUTES="$2" + shift 2 + ;; --foreground|--no-daemon) FOREGROUND="true" shift @@ -51,6 +58,15 @@ while [[ $# -gt 0 ]]; do esac done +IDLE_TIMEOUT_MS="" +if [[ -n "$IDLE_TIMEOUT_MINUTES" ]]; then + if ! [[ "$IDLE_TIMEOUT_MINUTES" =~ ^[0-9]+$ ]] || [[ "$IDLE_TIMEOUT_MINUTES" -eq 0 ]]; then + echo "{\"error\": \"--idle-timeout-minutes must be a positive integer\"}" + exit 1 + fi + IDLE_TIMEOUT_MS=$((IDLE_TIMEOUT_MINUTES * 60 * 1000)) +fi + if [[ -z "$URL_HOST" ]]; then if [[ "$BIND_HOST" == "127.0.0.1" || "$BIND_HOST" == "localhost" ]]; then URL_HOST="localhost" @@ -107,16 +123,26 @@ if [[ -z "$OWNER_PID" || "$OWNER_PID" == "1" ]]; then OWNER_PID="$PPID" fi +SERVER_ENV=( + BRAINSTORM_DIR="$SESSION_DIR" + BRAINSTORM_HOST="$BIND_HOST" + BRAINSTORM_URL_HOST="$URL_HOST" + BRAINSTORM_OWNER_PID="$OWNER_PID" +) +if [[ -n "$IDLE_TIMEOUT_MS" ]]; then + SERVER_ENV+=(BRAINSTORM_IDLE_TIMEOUT_MS="$IDLE_TIMEOUT_MS") +fi + # Foreground mode for environments that reap detached/background processes. if [[ "$FOREGROUND" == "true" ]]; then echo "$$" > "$PID_FILE" - env BRAINSTORM_DIR="$SESSION_DIR" BRAINSTORM_HOST="$BIND_HOST" BRAINSTORM_URL_HOST="$URL_HOST" BRAINSTORM_OWNER_PID="$OWNER_PID" node server.cjs + env "${SERVER_ENV[@]}" node server.cjs exit $? fi # Start server, capturing output to log file # Use nohup to survive shell exit; disown to remove from job table -nohup env BRAINSTORM_DIR="$SESSION_DIR" BRAINSTORM_HOST="$BIND_HOST" BRAINSTORM_URL_HOST="$URL_HOST" BRAINSTORM_OWNER_PID="$OWNER_PID" node server.cjs > "$LOG_FILE" 2>&1 & +nohup env "${SERVER_ENV[@]}" node server.cjs > "$LOG_FILE" 2>&1 & SERVER_PID=$! disown "$SERVER_PID" 2>/dev/null echo "$SERVER_PID" > "$PID_FILE" diff --git a/skills/brainstorming/visual-companion.md b/skills/brainstorming/visual-companion.md index 2113863d1e..da3779cbc8 100644 --- a/skills/brainstorming/visual-companion.md +++ b/skills/brainstorming/visual-companion.md @@ -90,11 +90,12 @@ scripts/start-server.sh \ ``` Use `--url-host` to control what hostname is printed in the returned URL JSON. +Use `--idle-timeout-minutes` when a session may sit idle for longer than the 2-hour default. ## The Loop 1. **Check server is alive**, then **write HTML** to a new file in `screen_dir`: - - Before each write, check that `$STATE_DIR/server-info` exists. If it doesn't (or `$STATE_DIR/server-stopped` exists), the server has shut down — restart it with `start-server.sh` before continuing. The server auto-exits after 30 minutes of inactivity. + - Before each write, check that `$STATE_DIR/server-info` exists. If it doesn't (or `$STATE_DIR/server-stopped` exists), the server has shut down — restart it with `start-server.sh` before continuing. The server auto-exits after 2 hours of inactivity by default; restart with `--idle-timeout-minutes` if the session needs a longer window. - Use semantic filenames: `platform.html`, `visual-style.html`, `layout.html` - **Never reuse filenames** — each screen gets a fresh file - Use Write tool — **never use cat/heredoc** (dumps noise into terminal) diff --git a/skills/using-git-worktrees/SKILL.md b/skills/using-git-worktrees/SKILL.md index 134d37140b..4996cf5f95 100644 --- a/skills/using-git-worktrees/SKILL.md +++ b/skills/using-git-worktrees/SKILL.md @@ -116,15 +116,27 @@ cd "$path" Auto-detect and run appropriate setup: ```bash -# Node.js -if [ -f package.json ]; then npm install; fi +# Node.js — detect package manager via lockfile. +# Critical for pnpm/yarn workspaces: `npm install` ignores workspace +# protocols, won't create `node_modules/` symlinks for workspace +# packages, and can leave the tree in a state where tools that resolve +# imports through `node_modules` (LSP, type checkers) silently break. +if [ -f pnpm-lock.yaml ]; then pnpm install +elif [ -f yarn.lock ]; then yarn install +elif [ -f bun.lockb ]; then bun install +elif [ -f package-lock.json ]; then npm install +elif [ -f package.json ]; then npm install +fi # Rust if [ -f Cargo.toml ]; then cargo build; fi # Python -if [ -f requirements.txt ]; then pip install -r requirements.txt; fi -if [ -f pyproject.toml ]; then poetry install; fi +if [ -f uv.lock ]; then uv sync +elif [ -f poetry.lock ]; then poetry install +elif [ -f pyproject.toml ]; then poetry install +elif [ -f requirements.txt ]; then pip install -r requirements.txt +fi # Go if [ -f go.mod ]; then go mod download; fi diff --git a/tests/brainstorm-server/server.test.js b/tests/brainstorm-server/server.test.js index 4797cbb943..24a812524a 100644 --- a/tests/brainstorm-server/server.test.js +++ b/tests/brainstorm-server/server.test.js @@ -16,6 +16,7 @@ const path = require('path'); const assert = require('assert'); const SERVER_PATH = path.join(__dirname, '../../skills/brainstorming/scripts/server.cjs'); +const { getIdleTimeoutMs } = require(SERVER_PATH); const TEST_PORT = 3334; const TEST_DIR = '/tmp/brainstorm-test'; const CONTENT_DIR = path.join(TEST_DIR, 'content'); @@ -112,6 +113,26 @@ async function runTests() { assert.strictEqual(info.port, TEST_PORT); assert.strictEqual(info.screen_dir, CONTENT_DIR, 'screen_dir should point to content/'); assert.strictEqual(info.state_dir, STATE_DIR, 'state_dir should point to state/'); + assert.strictEqual(info.idle_timeout_ms, 2 * 60 * 60 * 1000, 'should report default idle timeout'); + return Promise.resolve(); + }); + + await test('defaults idle timeout to 2 hours', () => { + assert.strictEqual(getIdleTimeoutMs(undefined), 2 * 60 * 60 * 1000); + assert.strictEqual(getIdleTimeoutMs(''), 2 * 60 * 60 * 1000); + return Promise.resolve(); + }); + + await test('accepts positive BRAINSTORM_IDLE_TIMEOUT_MS values', () => { + assert.strictEqual(getIdleTimeoutMs('900000'), 900000); + assert.strictEqual(getIdleTimeoutMs('1'), 1); + return Promise.resolve(); + }); + + await test('ignores invalid BRAINSTORM_IDLE_TIMEOUT_MS values', () => { + assert.strictEqual(getIdleTimeoutMs('0'), 2 * 60 * 60 * 1000); + assert.strictEqual(getIdleTimeoutMs('-1'), 2 * 60 * 60 * 1000); + assert.strictEqual(getIdleTimeoutMs('not-a-number'), 2 * 60 * 60 * 1000); return Promise.resolve(); }); @@ -295,6 +316,53 @@ async function runTests() { ws.close(); }); + await test('handles JSON null from client without crashing', async () => { + // JSON.parse('null') yields null; reading .choice on null throws and, + // before the handleMessage guard, killed the server process. + const ws = new WebSocket(`ws://localhost:${TEST_PORT}`); + await new Promise(resolve => ws.on('open', resolve)); + + ws.send('null'); + await sleep(300); + + const res = await fetch(`http://localhost:${TEST_PORT}/`); + assert.strictEqual(res.status, 200, 'Server should still be running after JSON null'); + ws.close(); + }); + + await test('handles JSON primitive values from client without crashing', async () => { + // Numbers, strings, booleans are valid JSON top-level values. They + // should be ignored (no .choice, so no events file write) without + // throwing. + const ws = new WebSocket(`ws://localhost:${TEST_PORT}`); + await new Promise(resolve => ws.on('open', resolve)); + + ws.send('42'); + ws.send('"hello"'); + ws.send('true'); + await sleep(300); + + const res = await fetch(`http://localhost:${TEST_PORT}/`); + assert.strictEqual(res.status, 200, 'Server should still be running after primitive values'); + ws.close(); + }); + + await test('does NOT write events file for JSON null', async () => { + // Regression for the null guard: ensure null doesn't slip past the + // truthiness check and write a stray events file. + const eventsFile = path.join(STATE_DIR, 'events'); + if (fs.existsSync(eventsFile)) fs.unlinkSync(eventsFile); + + const ws = new WebSocket(`ws://localhost:${TEST_PORT}`); + await new Promise(resolve => ws.on('open', resolve)); + + ws.send('null'); + await sleep(300); + + assert(!fs.existsSync(eventsFile), 'state/events should not exist for null payload'); + ws.close(); + }); + // ========== File Watching ========== console.log('\n--- File Watching ---'); @@ -410,6 +478,19 @@ async function runTests() { return Promise.resolve(); }); + // ========== start-server.sh Configuration ========== + console.log('\n--- start-server.sh Configuration ---'); + + await test('start-server.sh exposes idle timeout configuration', () => { + const script = fs.readFileSync( + path.join(__dirname, '../../skills/brainstorming/scripts/start-server.sh'), 'utf-8' + ); + assert(script.includes('--idle-timeout-minutes'), 'Should document idle timeout flag'); + assert(script.includes('IDLE_TIMEOUT_MINUTES'), 'Should parse timeout minutes'); + assert(script.includes('BRAINSTORM_IDLE_TIMEOUT_MS'), 'Should pass timeout to server process'); + return Promise.resolve(); + }); + // ========== Summary ========== console.log(`\n--- Results: ${passed} passed, ${failed} failed ---`); if (failed > 0) process.exit(1);