diff --git a/.agents/skills/oat-brainstorm/SKILL.md b/.agents/skills/oat-brainstorm/SKILL.md index 40e6b74440..c830e794f4 100644 --- a/.agents/skills/oat-brainstorm/SKILL.md +++ b/.agents/skills/oat-brainstorm/SKILL.md @@ -1,6 +1,6 @@ --- name: oat-brainstorm -version: 1.1.0 +version: 1.2.0 description: Use when the user explicitly invokes the `brainstorm` verb, including `/oat-brainstorm`, "let's brainstorm", "brainstorm this", "can we brainstorm X", or "help me brainstorm X". For ambiguous exploratory phrasing ("I've been thinking", "what if", "help me think through"), do NOT auto-enter; respond conversationally and offer mode only after ≥2 sustained exploratory turns. Do NOT use for review, debug, PR, status, implementation, or active-workflow questions. disable-model-invocation: false user-invocable: true @@ -198,7 +198,11 @@ command -v node >/dev/null 2>&1 && echo "available" || echo "missing" Wait for the user's response. -- **Accept** → start the visual companion via `${SKILL_DIR}/scripts/start-server.sh` and read `${SKILL_DIR}/references/visual-companion.md` for the detailed usage guide before serving any content. Set `VISUAL_COMPANION = "active"`. Persistence paths follow the bundled `start-server.sh` resolution (repo-scope `.oat/brainstorm//`, user-scope `~/.oat/brainstorm//`, or `--project-dir ` override). +- **Accept** → start the visual companion via `${SKILL_DIR}/scripts/start-server.sh --open` (pass `--open` now that the user has explicitly approved the companion — it auto-opens the keyed URL in their browser on the first screen) and read `${SKILL_DIR}/references/visual-companion.md` for the detailed usage guide before serving any content. Set `VISUAL_COMPANION = "active"`. Persistence paths follow the bundled `start-server.sh` resolution (repo-scope `.oat/brainstorm//`, user-scope `~/.oat/brainstorm//`, or `--project-dir ` override). + + **Preserve the keyed URL.** The startup JSON's `url` field carries the session key as `?key=...`. Always hand the user that exact `url` value — never strip the query string — and re-share the full keyed URL any time you remind them where to look; a bare `http://localhost:/` is rejected by the server. See `references/visual-companion.md` for the full usage loop. + + **Restart semantics.** If the server needs to restart mid-session (it exited, was stopped, or the idle timeout fired — 4 hours by default), re-invoke `start-server.sh` with the **same** `--project-dir`/repo-context path resolution used the first time. That reuses the previously bound port and session key, so an already-open browser tab reconnects on its own and no new URL needs to be issued. `--open` is not required on a mid-session restart; only pass it again if you need to force the browser open. **Resolving `${SKILL_DIR}`:** the brainstorm pack defaults to user scope, so a fresh install puts the bundled scripts under `~/.agents/skills/oat-brainstorm/`, not the current repo. Resolve the loaded skill directory before invoking any script. In order: 1. If your provider exposes the loaded skill's resolved path (for example via a `${SKILL_DIR}` environment variable, `Skill` invocation context, or equivalent), use it directly. diff --git a/.agents/skills/oat-brainstorm/references/visual-companion.md b/.agents/skills/oat-brainstorm/references/visual-companion.md index b63a16ce98..6c8ba4c1d0 100644 --- a/.agents/skills/oat-brainstorm/references/visual-companion.md +++ b/.agents/skills/oat-brainstorm/references/visual-companion.md @@ -40,25 +40,41 @@ The bundled `start-server.sh` resolves session paths through OAT-managed prefixe OAT's repo-root `.gitignore` already covers `.oat/` so repo-scoped sessions are not tracked by default. +The same resolved root also stores `.last-port` and `.last-token` (sibling to the session directories). See **Session Authentication and the Companion URL** below for how those get reused on restart. + +## Session Authentication and the Companion URL + +Every request to the companion server requires the session key. `start-server.sh` embeds it in the returned URL as `?key=` — always hand the user the **full URL from the JSON response, verbatim**. Do not strip or shorten the query string. + +- **First load:** the keyed URL bootstraps the browser and mirrors the key into an `HttpOnly` session cookie, so subsequent same-origin requests (page assets, `/files/*`, the WebSocket upgrade) authenticate via the cookie without repeating the key in the address bar. +- **Unauthenticated requests** (missing/incorrect key and no valid cookie) get a `403` response — the server never serves content to an unkeyed request. +- **Restarting the server** with the same `--project-dir` (or the same repo/user-scope context) reuses the previously bound port and session key, persisted at `.last-port` / `.last-token` under the resolved OAT root (see Persistence Paths above). If a browser tab from the prior session is still open, it reconnects automatically and does not need a new URL — see **Connection Status and Reconnection** below. Only give the user a new URL if the port or key actually changed (for example, a different `--project-dir` or a fresh user-scope session with no prior state). + ## Starting a Session The scripts ship inside the skill directory. Invoke them via paths relative to the installed skill location (`.agents/skills/oat-brainstorm/scripts/` or the user-scope equivalent `~/.agents/skills/oat-brainstorm/scripts/`). ```bash -# Start server with project-dir override (paths land under that directory) -.agents/skills/oat-brainstorm/scripts/start-server.sh --project-dir /path/to/project +# Start server with project-dir override (paths land under that directory). +# Pass --open only after the user has already accepted the visual-companion +# offer — it auto-opens the keyed URL in their browser on the first screen. +.agents/skills/oat-brainstorm/scripts/start-server.sh --project-dir /path/to/project --open -# Returns: {"type":"server-started","port":52341,"url":"http://localhost:52341", +# Returns: {"type":"server-started","port":52341, +# "url":"http://localhost:52341/?key=3f9c1a7b2e...", # "screen_dir":"/path/to/project/.oat/brainstorm/12345-1706000000/content", -# "state_dir":"/path/to/project/.oat/brainstorm/12345-1706000000/state"} +# "state_dir":"/path/to/project/.oat/brainstorm/12345-1706000000/state", +# "idle_timeout_ms":14400000} ``` -Save `screen_dir` and `state_dir` from the response. Tell user to open the URL. +Save `screen_dir` and `state_dir` from the response. Tell the user to open the full `url` value exactly as returned — the `?key=...` suffix is required; a bare `http://localhost:/` gets a `403`. **Finding connection info:** The server writes its startup JSON to `$STATE_DIR/server-info`. If you launched the server in the background and didn't capture stdout, read that file to get the URL and port. Inside an OAT repo (without `--project-dir`), the session lives under `/.oat/brainstorm/`. Outside an OAT repo, check `~/.oat/brainstorm/`. **Note:** Repo-scoped sessions persist under `.oat/brainstorm/` (covered by OAT's default `.gitignore` for `.oat/`). User-scope fallback sessions persist under `~/.oat/brainstorm/`. There is no `/tmp` default — sessions are always recoverable. +**Idle timeout:** the server shuts itself down after **4 hours** of inactivity by default (`idle_timeout_ms` in the startup JSON). Pass `--idle-timeout-minutes ` to `start-server.sh` to override — useful for deliberately short-lived sessions or for extending past 4 hours on a long-running brainstorm. + **Launching the server by platform:** **Claude Code (macOS / Linux):** @@ -108,10 +124,20 @@ If the URL is unreachable from your browser (common in remote/containerized setu Use `--url-host` to control what hostname is printed in the returned URL JSON. +## Connection Status and Reconnection + +The browser page shows a connection-status indicator (`connecting` / `connected` / `reconnecting` / `disconnected`) driven by the WebSocket link back to the server. This runs automatically in the client — no agent action needed: + +- **Brief disconnects reconnect automatically** using exponential backoff. +- **Extended disconnects** (roughly 15+ seconds) show a "Companion paused" overlay telling the user the agent will bring it back. Expect this during a deliberate restart — it is not an error condition to react to. +- **After the server comes back** on the same port (for example, you restarted it with the same `--project-dir`/repo context per Session Authentication above), the page detects the recovery and reloads through the keyed bootstrap automatically, so the open tab keeps working without a new URL. + +Because reconnection is automatic, you do not need to tell the user to refresh after a restart. Only send a new URL if the port or key actually changed. + ## 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. +1. **Pre-flight alive check**, 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. Restarting with the same `--project-dir`/repo context reuses the last bound port and session key (see Session Authentication above), so an already-open browser tab reconnects on its own — see Connection Status and Reconnection. The server auto-exits after 4 hours of inactivity by default (override with `--idle-timeout-minutes`). - 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/.agents/skills/oat-brainstorm/scripts/frame-template.html b/.agents/skills/oat-brainstorm/scripts/frame-template.html index dcfe01817e..f0087cce2f 100644 --- a/.agents/skills/oat-brainstorm/scripts/frame-template.html +++ b/.agents/skills/oat-brainstorm/scripts/frame-template.html @@ -2,14 +2,14 @@ - Superpowers Brainstorming + OAT Brainstorm

Brainstorm Companion

Waiting for the agent to push a screen...

`; +const FORBIDDEN_PAGE = ` + +Session key required + + +

Session key required

+

This page needs the full URL your coding agent gave you, including the +?key=… part. Copy the complete URL and open it again.

`; + +function bootstrapPage(key) { + const jsonKey = JSON.stringify(String(key)); + return ` + +Opening Brainstorm Companion + + + +`; +} + const frameTemplate = fs.readFileSync(path.join(__dirname, 'frame-template.html'), 'utf-8'); const helperScript = fs.readFileSync(path.join(__dirname, 'helper.js'), 'utf-8'); const helperInjection = ''; @@ -115,20 +200,144 @@ function wrapInFrame(content) { function getNewestScreen() { const files = fs.readdirSync(CONTENT_DIR) - .filter(f => f.endsWith('.html')) + .filter(f => !f.startsWith('.') && f.endsWith('.html')) .map(f => { const fp = path.join(CONTENT_DIR, f); + if (!isRegularFileInsideContentDir(fp)) return null; return { path: fp, mtime: fs.statSync(fp).mtime.getTime() }; }) + .filter(Boolean) .sort((a, b) => b.mtime - a.mtime); return files.length > 0 ? files[0].path : null; } +function urlHostForHttp(host) { + const h = String(host); + if (h.startsWith('[') && h.endsWith(']')) return h; + return h.includes(':') ? '[' + h + ']' : h; +} + +function companionUrl() { + return 'http://' + urlHostForHttp(URL_HOST) + ':' + PORT + '/?key=' + TOKEN; +} + +function browserLauncherForPlatform(url, { + platform = process.platform, + osRelease = require('os').release(), + env = process.env +} = {}) { + const isWSL = platform === 'linux' && /microsoft/i.test(osRelease); + if (platform === 'darwin') return { bin: 'open', args: [url] }; + if (platform === 'win32' || isWSL) { + return { bin: 'rundll32.exe', args: ['url.dll,FileProtocolHandler', url] }; + } + if (env.DISPLAY || env.WAYLAND_DISPLAY) return { bin: 'xdg-open', args: [url] }; + return null; +} + +function isRegularFileInsideContentDir(filePath) { + let stat, realContentDir, realFilePath; + try { + stat = fs.lstatSync(filePath); + if (stat.isSymbolicLink()) return false; + if (!stat.isFile()) return false; + if (stat.nlink !== 1) return false; + realContentDir = fs.realpathSync(CONTENT_DIR); + realFilePath = fs.realpathSync(filePath); + } catch (e) { + return false; + } + return realFilePath.startsWith(realContentDir + path.sep); +} + +// ========== Authentication ========== + +function timingSafeEqualStr(a, b) { + const ab = Buffer.from(String(a)); + const bb = Buffer.from(String(b)); + if (ab.length !== bb.length) return false; + return crypto.timingSafeEqual(ab, bb); +} + +function parseCookies(header) { + const out = {}; + if (!header) return out; + for (const part of header.split(';')) { + const eq = part.indexOf('='); + if (eq < 0) continue; + out[part.slice(0, eq).trim()] = part.slice(eq + 1).trim(); + } + return out; +} + +// A request is authorized if it carries the session key as ?key= or as the +// session cookie. Both are compared in constant time. +function isAuthorized(req) { + const q = req.url.indexOf('?'); + if (q >= 0) { + const params = new URLSearchParams(req.url.slice(q + 1)); + if (params.has('key')) { + const key = params.get('key'); + return Boolean(key && timingSafeEqualStr(key, TOKEN)); + } + } + const cookie = parseCookies(req.headers['cookie'])[COOKIE_NAME]; + if (cookie && timingSafeEqualStr(cookie, TOKEN)) return true; + return false; +} + +function pathnameOf(url) { + const q = url.indexOf('?'); + return q >= 0 ? url.slice(0, q) : url; +} + +function queryKey(url) { + const q = url.indexOf('?'); + if (q < 0) return null; + return new URLSearchParams(url.slice(q + 1)).get('key'); +} + +function securityHeaders(headers = {}) { + return { + 'Referrer-Policy': 'no-referrer', + 'Cache-Control': 'no-store', + 'X-Frame-Options': 'DENY', + 'Content-Security-Policy': "frame-ancestors 'none'", + 'Cross-Origin-Resource-Policy': 'same-origin', + ...headers + }; +} + +function isAllowedWebSocketOrigin(req) { + const origin = req.headers.origin; + if (!origin) return true; + const host = req.headers.host; + if (!host) return false; + return origin === 'http://' + host; +} + // ========== HTTP Request Handler ========== function handleRequest(req, res) { - touchActivity(); - if (req.method === 'GET' && req.url === '/') { + if (!isAuthorized(req)) { + res.writeHead(403, securityHeaders({ 'Content-Type': 'text/html; charset=utf-8' })); + res.end(FORBIDDEN_PAGE); + return; + } + touchActivity(); // only authorized requests count as activity + + // Mirror the key into a cookie so same-origin subresources (/files/*) can + // authenticate after bootstrap. HttpOnly keeps it away from page scripts; the + // WebSocket Origin check below is what blocks cross-origin localhost injection. + res.setHeader('Set-Cookie', + COOKIE_NAME + '=' + TOKEN + '; HttpOnly; SameSite=Strict; Path=/'); + + const pathname = pathnameOf(req.url); + const keyFromQuery = queryKey(req.url); + if (req.method === 'GET' && pathname === '/' && keyFromQuery && timingSafeEqualStr(keyFromQuery, TOKEN)) { + res.writeHead(200, securityHeaders({ 'Content-Type': 'text/html; charset=utf-8' })); + res.end(bootstrapPage(keyFromQuery)); + } else if (req.method === 'GET' && pathname === '/') { const screenFile = getNewestScreen(); let html = screenFile ? (raw => isFullDocument(raw) ? raw : wrapInFrame(raw))(fs.readFileSync(screenFile, 'utf-8')) @@ -140,22 +349,24 @@ function handleRequest(req, res) { html += helperInjection; } - res.writeHead(200, { 'Content-Type': 'text/html; charset=utf-8' }); + res.writeHead(200, securityHeaders({ 'Content-Type': 'text/html; charset=utf-8' })); res.end(html); - } else if (req.method === 'GET' && req.url.startsWith('/files/')) { - const fileName = req.url.slice(7); - const filePath = path.join(CONTENT_DIR, path.basename(fileName)); - if (!fs.existsSync(filePath)) { - res.writeHead(404); + } else if (req.method === 'GET' && pathname.startsWith('/files/')) { + const fileName = path.basename(pathname.slice(7)); + const filePath = path.join(CONTENT_DIR, fileName); + // Reject empty/dotfile names and anything that isn't a regular file — + // `/files/` would otherwise resolve to CONTENT_DIR and crash readFileSync (EISDIR). + if (!fileName || fileName.startsWith('.') || !isRegularFileInsideContentDir(filePath)) { + res.writeHead(404, securityHeaders()); res.end('Not found'); return; } const ext = path.extname(filePath).toLowerCase(); const contentType = MIME_TYPES[ext] || 'application/octet-stream'; - res.writeHead(200, { 'Content-Type': contentType }); + res.writeHead(200, securityHeaders({ 'Content-Type': contentType })); res.end(fs.readFileSync(filePath)); } else { - res.writeHead(404); + res.writeHead(404, securityHeaders()); res.end('Not found'); } } @@ -165,6 +376,8 @@ function handleRequest(req, res) { const clients = new Set(); function handleUpgrade(req, socket) { + if (!isAuthorized(req) || !isAllowedWebSocketOrigin(req)) { socket.destroy(); return; } + const key = req.headers['sec-websocket-key']; if (!key) { socket.destroy(); return; } @@ -231,7 +444,7 @@ function handleMessage(text) { } touchActivity(); console.log(JSON.stringify({ source: 'user-event', ...event })); - if (event.choice) { + if (event && event.choice) { const eventsFile = path.join(STATE_DIR, 'events'); fs.appendFileSync(eventsFile, JSON.stringify(event) + '\n'); } @@ -244,9 +457,44 @@ function broadcast(msg) { } } +// Best-effort: open the user's browser the first time a screen is actually ready +// to show. Skips when disabled, on a non-loopback (remote) bind, or when a +// browser is already connected. Override the launcher with BRAINSTORM_OPEN_CMD. +let browserOpened = false; +function maybeOpenBrowser() { + if (browserOpened) return; + browserOpened = true; + if (!process.env.BRAINSTORM_OPEN) return; // opt-in: only after the user approves the companion + if (HOST !== '127.0.0.1' && HOST !== 'localhost') return; + if (clients.size > 0) return; // the user already opened it + const url = companionUrl(); // must carry the key or the gate 403s it + 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; + } + // 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 launcher = browserLauncherForPlatform(url); + if (!launcher) return; // headless: nothing to open + try { cp.execFile(launcher.bin, launcher.args, () => {}); } catch (e) { /* best effort */ } +} + // ========== Activity Tracking ========== -const IDLE_TIMEOUT_MS = 30 * 60 * 1000; // 30 minutes +// Idle timeout: shut down after this long with no activity. Default 4 hours; +// override with BRAINSTORM_IDLE_TIMEOUT_MS (start-server.sh: --idle-timeout-minutes). +const IDLE_TIMEOUT_MS = (() => { + const ms = Number(process.env.BRAINSTORM_IDLE_TIMEOUT_MS); + return Number.isFinite(ms) && ms > 0 ? ms : 4 * 60 * 60 * 1000; +})(); +// How often the watchdog checks for owner-death / idleness. Configurable mainly +// so tests can run fast; production default is 60s. +const LIFECYCLE_CHECK_MS = (() => { + const ms = Number(process.env.BRAINSTORM_LIFECYCLE_CHECK_MS); + return Number.isFinite(ms) && ms > 0 ? ms : 60 * 1000; +})(); let lastActivity = Date.now(); function touchActivity() { @@ -267,14 +515,14 @@ function startServer() { // macOS fs.watch reports 'rename' for both new files and overwrites, // so we can't rely on eventType alone. const knownFiles = new Set( - fs.readdirSync(CONTENT_DIR).filter(f => f.endsWith('.html')) + fs.readdirSync(CONTENT_DIR).filter(f => !f.startsWith('.') && f.endsWith('.html')) ); const server = http.createServer(handleRequest); server.on('upgrade', handleUpgrade); const watcher = fs.watch(CONTENT_DIR, (eventType, filename) => { - if (!filename || !filename.endsWith('.html')) return; + if (!filename || filename.startsWith('.') || !filename.endsWith('.html')) return; if (debounceTimers.has(filename)) clearTimeout(debounceTimers.get(filename)); debounceTimers.set(filename, setTimeout(() => { @@ -289,6 +537,7 @@ function startServer() { const eventsFile = path.join(STATE_DIR, 'events'); if (fs.existsSync(eventsFile)) fs.unlinkSync(eventsFile); console.log(JSON.stringify({ type: 'screen-added', file: filePath })); + maybeOpenBrowser(); } else { console.log(JSON.stringify({ type: 'screen-updated', file: filePath })); } @@ -308,6 +557,11 @@ function startServer() { ); watcher.close(); clearInterval(lifecycleCheck); + // Close any upgraded WebSocket sockets so server.close() can complete and + // the process actually exits instead of lingering on an open connection. + for (const socket of clients) { + try { socket.destroy(); } catch (e) { /* already gone */ } + } server.close(() => process.exit(0)); } @@ -316,11 +570,11 @@ 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 + // Periodically exit if the owner process died or we've been idle too long. const lifecycleCheck = setInterval(() => { if (!ownerAlive()) shutdown('owner process exited'); else if (Date.now() - lastActivity > IDLE_TIMEOUT_MS) shutdown('idle timeout'); - }, 60 * 1000); + }, LIFECYCLE_CHECK_MS); lifecycleCheck.unref(); // Validate owner PID at startup. If it's already dead, the PID resolution @@ -336,19 +590,68 @@ function startServer() { } } - server.listen(PORT, HOST, () => { + // 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() { + // Cookie name keys on the ACTUAL bound port (may differ from the preferred + // one after an EADDRINUSE fallback) so it can't collide with another server's + // cookie in the shared localhost jar. + COOKIE_NAME = 'brainstorm-key-' + PORT; + // Record the bound port AND token so the next restart of this session reuses + // them — but ONLY when we got our preferred port. On a fallback we bound a + // *different* port because someone else holds the preferred one; persisting + // would overwrite the shared files and strand that other session's open tab. + if (PORT_FILE && !triedFallback) { + try { fs.writeFileSync(PORT_FILE, String(PORT)); } catch (e) { /* best effort */ } + if (TOKEN_FILE) { + try { + fs.writeFileSync(TOKEN_FILE, TOKEN, { mode: 0o600 }); + chmodOwnerOnly(TOKEN_FILE); + } catch (e) { /* best effort */ } + } + } 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 + url_host: URL_HOST, url: companionUrl(), + 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'); + // server-info embeds the key — keep it owner-only. + fs.writeFileSync(path.join(STATE_DIR, 'server-info'), info + '\n', { mode: 0o600 }); + } + + server.on('error', (err) => { + if (err.code === 'EADDRINUSE' && !triedFallback) { + if (tokenSource === 'env') { + console.error('Server failed to bind: preferred port is in use and BRAINSTORM_TOKEN is set; refusing fallback with explicit token'); + process.exit(1); + } + triedFallback = true; + PORT = randomPort(); + if (tokenSource === 'file') { + TOKEN = generateToken(); + tokenSource = 'generated-fallback'; + } + server.listen(PORT, HOST, onListen); + } else { + console.error('Server failed to bind:', err.message); + process.exit(1); + } }); + server.listen(PORT, HOST, onListen); } if (require.main === module) { startServer(); } -module.exports = { computeAcceptKey, encodeFrame, decodeFrame, OPCODES }; +module.exports = { + computeAcceptKey, + encodeFrame, + decodeFrame, + browserLauncherForPlatform, + OPCODES, + MAX_FRAME_PAYLOAD_BYTES +}; diff --git a/.agents/skills/oat-brainstorm/scripts/start-server.sh b/.agents/skills/oat-brainstorm/scripts/start-server.sh index 843e57008b..844c53973f 100755 --- a/.agents/skills/oat-brainstorm/scripts/start-server.sh +++ b/.agents/skills/oat-brainstorm/scripts/start-server.sh @@ -1,6 +1,7 @@ #!/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 ] [--open] [--foreground] [--background] # # Starts server on a random high port, outputs JSON with URL. # Each session gets its own directory to avoid conflicts. @@ -11,25 +12,49 @@ # → /.oat/brainstorm// # 3. fallback → ~/.oat/brainstorm// # -# Files persist after server stops in all three cases — no /tmp default. +# Files persist after server stops in all three cases — no /tmp default. The +# bound port and session key are recorded at /.last-port and +# .last-token (sibling to the session dirs, under the SAME resolved root as +# above — never under .superpowers/) so restarting from the same +# --project-dir or repo context reuses them and an already-open browser tab +# reconnects with a valid cookie. # # Options: -# --project-dir Store session files under /.oat/brainstorm/. -# Overrides repo walk-up detection. -# --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. -# --foreground Run server in the current terminal (no backgrounding). -# --background Force background mode (overrides Codex auto-foreground). +# --project-dir Store session files under /.oat/brainstorm/. +# Overrides repo walk-up detection. +# --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 Shut down after n minutes idle (default 240 = 4h). +# --open Auto-open the browser on the first screen (use only +# after the user approves the visual companion). +# --foreground Run server in the current terminal (no backgrounding). +# --background Force background mode (overrides Codex auto-foreground). SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" +is_windows_like_shell() { + case "${OSTYPE:-}" in + msys*|cygwin*|mingw*) return 0 ;; + esac + if [[ -n "${MSYSTEM:-}" ]]; then + return 0 + fi + local uname_s + uname_s="$(uname -s 2>/dev/null || true)" + case "$uname_s" in + MSYS*|MINGW*|CYGWIN*) return 0 ;; + esac + return 1 +} + # Parse arguments PROJECT_DIR="" 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) @@ -44,6 +69,14 @@ while [[ $# -gt 0 ]]; do URL_HOST="$2" shift 2 ;; + --idle-timeout-minutes) + IDLE_TIMEOUT_MINUTES="$2" + shift 2 + ;; + --open) + export BRAINSTORM_OPEN=1 + shift + ;; --foreground|--no-daemon) FOREGROUND="true" shift @@ -67,6 +100,17 @@ if [[ -z "$URL_HOST" ]]; then fi fi +IDLE_TIMEOUT_MS="" +if [[ -n "$IDLE_TIMEOUT_MINUTES" ]]; then + if ! [[ "$IDLE_TIMEOUT_MINUTES" =~ ^[0-9]+$ ]] || [[ "$IDLE_TIMEOUT_MINUTES" -lt 1 ]]; then + echo "{\"error\": \"--idle-timeout-minutes must be a positive integer\"}" + exit 1 + fi + # server.cjs reads BRAINSTORM_IDLE_TIMEOUT_MS in milliseconds; convert here so + # the launcher and server agree on units. + IDLE_TIMEOUT_MS=$(( IDLE_TIMEOUT_MINUTES * 60 * 1000 )) +fi + # Some environments reap detached/background processes. Auto-foreground when detected. if [[ -n "${CODEX_CI:-}" && "$FOREGROUND" != "true" && "$FORCE_BACKGROUND" != "true" ]]; then FOREGROUND="true" @@ -74,24 +118,25 @@ fi # Windows/Git Bash reaps nohup background processes. Auto-foreground when detected. if [[ "$FOREGROUND" != "true" && "$FORCE_BACKGROUND" != "true" ]]; then - case "${OSTYPE:-}" in - msys*|cygwin*|mingw*) FOREGROUND="true" ;; - esac - if [[ -n "${MSYSTEM:-}" ]]; then + if is_windows_like_shell; then FOREGROUND="true" fi fi +# Session files (server.log, server-info, .last-token) embed the session key — +# keep everything this script and the server create owner-only. +umask 077 + # Generate unique session directory SESSION_ID="$$-$(date +%s)" -# Resolve OAT-managed persistence prefix. -# 1. --project-dir → /.oat/brainstorm// +# Resolve OAT-managed persistence root. +# 1. --project-dir → /.oat/brainstorm/ # 2. invoked inside an OAT repo (walk-up detection of .oat/ directory) -# → /.oat/brainstorm// -# 3. fallback → ~/.oat/brainstorm// +# → /.oat/brainstorm/ +# 3. fallback → ~/.oat/brainstorm/ if [[ -n "$PROJECT_DIR" ]]; then - SESSION_DIR="${PROJECT_DIR}/.oat/brainstorm/${SESSION_ID}" + BRAINSTORM_ROOT="${PROJECT_DIR}/.oat/brainstorm" else # Walk up from the current working directory looking for a .oat/ dir. REPO_ROOT="" @@ -105,19 +150,38 @@ else done if [[ -n "$REPO_ROOT" ]]; then - SESSION_DIR="${REPO_ROOT}/.oat/brainstorm/${SESSION_ID}" + BRAINSTORM_ROOT="${REPO_ROOT}/.oat/brainstorm" else - SESSION_DIR="${HOME}/.oat/brainstorm/${SESSION_ID}" + BRAINSTORM_ROOT="${HOME}/.oat/brainstorm" fi fi +SESSION_DIR="${BRAINSTORM_ROOT}/${SESSION_ID}" STATE_DIR="${SESSION_DIR}/state" PID_FILE="${STATE_DIR}/server.pid" LOG_FILE="${STATE_DIR}/server.log" +SERVER_ID_FILE="${STATE_DIR}/server-instance-id" + +# Persist the bound port and key at the resolved OAT root (sibling to the +# session dirs, same prefix used above) so a restart from the same +# --project-dir or repo context reuses them and an already-open browser tab +# reconnects to the same URL with a valid cookie. +PORT_FILE="${BRAINSTORM_ROOT}/.last-port" +TOKEN_FILE="${BRAINSTORM_ROOT}/.last-token" # Create fresh session directory with content and state peers mkdir -p "${SESSION_DIR}/content" "$STATE_DIR" +SERVER_ID="" +if [[ -r /dev/urandom ]]; then + SERVER_ID="$(od -An -N24 -tx1 /dev/urandom 2>/dev/null | tr -d ' \n' || true)" +fi +if ! [[ "$SERVER_ID" =~ ^[A-Za-z0-9_-]{32,64}$ ]]; then + SERVER_ID="$(printf '%08x%08x%08x%08x' "$$" "$(date +%s)" "${RANDOM:-0}" "${RANDOM:-0}")" +fi +printf '%s\n' "$SERVER_ID" > "$SERVER_ID_FILE" +chmod 600 "$SERVER_ID_FILE" 2>/dev/null || true + # Kill any existing server if [[ -f "$PID_FILE" ]]; then old_pid=$(cat "$PID_FILE") @@ -125,7 +189,7 @@ if [[ -f "$PID_FILE" ]]; then rm -f "$PID_FILE" fi -cd "$SCRIPT_DIR" +cd "$SCRIPT_DIR" || exit 1 # Resolve the harness PID (grandparent of this script). # $PPID is the ephemeral shell the harness spawned to run us — it dies @@ -135,22 +199,50 @@ if [[ -z "$OWNER_PID" || "$OWNER_PID" == "1" ]]; then OWNER_PID="$PPID" fi +# Windows/MSYS2: Node.js cannot see POSIX PIDs from the MSYS2 namespace. +# Passing a PID node cannot verify causes server to log owner-pid-invalid +# and self-terminate at the 60-second lifecycle check. Clear it so the +# watchdog is disabled and the idle timeout becomes the only shutdown trigger. +if is_windows_like_shell; then + OWNER_PID="" +fi + +# Build the server environment. BRAINSTORM_PORT_FILE/TOKEN_FILE always point at +# the resolved OAT root so restarts reuse the port/key; BRAINSTORM_IDLE_TIMEOUT_MS +# is only set when --idle-timeout-minutes overrides the server's 4h default. +SERVER_ENV=( + "BRAINSTORM_DIR=$SESSION_DIR" + "BRAINSTORM_HOST=$BIND_HOST" + "BRAINSTORM_URL_HOST=$URL_HOST" + "BRAINSTORM_OWNER_PID=$OWNER_PID" + "BRAINSTORM_PORT_FILE=$PORT_FILE" + "BRAINSTORM_TOKEN_FILE=$TOKEN_FILE" +) +if [[ -n "$IDLE_TIMEOUT_MS" ]]; then + SERVER_ENV+=("BRAINSTORM_IDLE_TIMEOUT_MS=$IDLE_TIMEOUT_MS") +fi +if [[ -n "${BRAINSTORM_OPEN:-}" ]]; then + SERVER_ENV+=("BRAINSTORM_OPEN=$BRAINSTORM_OPEN") +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 "--brainstorm-server-id=$SERVER_ID" & + SERVER_PID=$! + echo "$SERVER_PID" > "$PID_FILE" + wait "$SERVER_PID" 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 "--brainstorm-server-id=$SERVER_ID" > "$LOG_FILE" 2>&1 & SERVER_PID=$! disown "$SERVER_PID" 2>/dev/null echo "$SERVER_PID" > "$PID_FILE" # Wait for server-started message (check log file) -for i in {1..50}; do +for _ in {1..50}; do if grep -q "server-started" "$LOG_FILE" 2>/dev/null; then # Verify server is still alive after a short window (catches process reapers) alive="true" diff --git a/.agents/skills/oat-brainstorm/scripts/stop-server.sh b/.agents/skills/oat-brainstorm/scripts/stop-server.sh index a6b94e6532..98f9d35420 100755 --- a/.agents/skills/oat-brainstorm/scripts/stop-server.sh +++ b/.agents/skills/oat-brainstorm/scripts/stop-server.sh @@ -3,7 +3,7 @@ # Usage: stop-server.sh # # Kills the server process. Only deletes session directory if it's -# under /tmp (ephemeral). Persistent directories (.superpowers/) are +# under /tmp (ephemeral). Persistent OAT directories (.oat/brainstorm/) are # kept so mockups can be reviewed later. SESSION_DIR="$1" @@ -15,15 +15,78 @@ fi STATE_DIR="${SESSION_DIR}/state" PID_FILE="${STATE_DIR}/server.pid" +SERVER_ID_FILE="${STATE_DIR}/server-instance-id" + +mark_stopped() { + local reason="$1" + rm -f "${STATE_DIR}/server-info" + printf '{"reason":"%s","timestamp":%s}\n' "$reason" "$(date +%s)" > "${STATE_DIR}/server-stopped" +} + +read_expected_server_id() { + [[ -f "$SERVER_ID_FILE" ]] || return 1 + local id + id="$(tr -d '\r\n' < "$SERVER_ID_FILE" 2>/dev/null || true)" + [[ "$id" =~ ^[A-Za-z0-9_-]{32,64}$ ]] || return 1 + printf '%s\n' "$id" +} + +command_line_for_pid() { + local pid="$1" + if [[ -r "/proc/$pid/cmdline" ]]; then + tr '\0' '\n' < "/proc/$pid/cmdline" 2>/dev/null || true + return 0 + fi + ps -ww -p "$pid" -o command= 2>/dev/null || ps -f -p "$pid" 2>/dev/null | sed '1d' || true +} + +command_has_server_id() { + local pid="$1" + local expected="$2" + local expected_arg="--brainstorm-server-id=$expected" + if [[ -r "/proc/$pid/cmdline" ]]; then + local arg + while IFS= read -r -d '' arg || [[ -n "$arg" ]]; do + [[ "$arg" == "$expected_arg" ]] && return 0 + done < "/proc/$pid/cmdline" + return 1 + fi + local command_line + command_line="$(command_line_for_pid "$pid")" + [[ -n "$command_line" ]] || return 1 + case " $command_line " in + *" $expected_arg "*) return 0 ;; + *) return 1 ;; + esac +} + +# Confirm a PID has this session's per-start instance id, not just a familiar +# process name. Ambiguous or legacy metadata fails closed as stale_pid. +is_brainstorm_server() { + kill -0 "$1" 2>/dev/null || return 1 + local expected_id + expected_id="$(read_expected_server_id)" || return 1 + command_has_server_id "$1" "$expected_id" || return 1 + return 0 +} if [[ -f "$PID_FILE" ]]; then pid=$(cat "$PID_FILE") + # Refuse to signal a PID we can't prove is our server. A stale pid file may + # point at an unrelated process after a reboot/PID wraparound. + if ! is_brainstorm_server "$pid"; then + rm -f "$PID_FILE" "$SERVER_ID_FILE" + mark_stopped "stale_pid" + echo '{"status": "stale_pid"}' + exit 0 + fi + # Try to stop gracefully, fallback to force if still alive kill "$pid" 2>/dev/null || true # Wait for graceful shutdown (up to ~2s) - for i in {1..20}; do + for _ in {1..20}; do if ! kill -0 "$pid" 2>/dev/null; then break fi @@ -43,7 +106,8 @@ if [[ -f "$PID_FILE" ]]; then exit 1 fi - rm -f "$PID_FILE" "${STATE_DIR}/server.log" + rm -f "$PID_FILE" "$SERVER_ID_FILE" "${STATE_DIR}/server.log" + mark_stopped "stop-server.sh" # Only delete ephemeral /tmp directories if [[ "$SESSION_DIR" == /tmp/* ]]; then diff --git a/.oat/projects/shared/brainstorm-visual-companion-v6/discovery.md b/.oat/projects/shared/brainstorm-visual-companion-v6/discovery.md new file mode 100644 index 0000000000..7ad367cf86 --- /dev/null +++ b/.oat/projects/shared/brainstorm-visual-companion-v6/discovery.md @@ -0,0 +1,175 @@ +--- +oat_status: complete +oat_ready_for: oat-project-quick-start +oat_blockers: [] +oat_last_updated: 2026-06-26 +oat_generated: false +--- + +# Discovery: brainstorm-visual-companion-v6 + +## Phase Guardrails (Discovery) + +Discovery captures outcomes and constraints. Implementation file names appear only where they clarify scope boundaries agreed in the prior release review. + +## Initial Request + +Review Superpowers GitHub releases (currently through v6.0.3) for visual brainstorming functionality that OAT has largely ported one-to-one, identify gaps worth incorporating into the OAT brainstorm skillset, and execute the work via quick-start planning (discovery + plan, no full spec-driven workflow). + +Session context: OAT's visual companion bundle was ported from **Superpowers v5.0.7** (see repo `NOTICES.md` and the `independent-brainstorming` project summary). Superpowers v6.0.0 shipped a major visual-companion security and resilience rewrite; v5.0.6 shipped server lifecycle fixes OAT already has; v6.0.3 is SDD-only and out of scope here. + +## Clarifying Questions + +### Question 1: Workflow shape + +**Q:** Proceed without OAT project tracking, or use quick-start to capture discovery and a plan? + +**A:** Use OAT quick start — capture discovery and write a plan. + +**Decision:** This project (`brainstorm-visual-companion-v6`) tracks the work; implementation follows `plan.md` via `oat-project-implement`. + +### Question 2: Scope boundary + +**Q:** What belongs in this project vs. other OAT skills? + +**A:** (Inferred from release review and user focus on visual brainstorming.) + +**Decision:** Primary scope is the **visual companion Node bundle** and its **skill/reference docs** under `oat-brainstorm`. Superpowers SDD, writing-plans, harness bootstraps, and brainstorming→writing-plans terminal flow are out of scope. Optional skill-content improvements (e.g. synthesis self-review) are deferred unless trivial. + +## Solution Space + +OAT already diverges productively from upstream: Activation Contract, pack-aware destinations, conditional visual offer, and OAT persistence paths (`.oat/brainstorm/`). The gap is **technical parity** with Superpowers v6.0.x for the local web server, not re-adopting Superpowers' single-pipeline brainstorm model. + +### Approach 1: Full v6.0.3 bundle port with OAT path adaptations _(Recommended)_ + +**Description:** Cherry-pick Superpowers v6.0.3 visual-companion scripts (`server.cjs`, `helper.js`, `start-server.sh`, `stop-server.sh`, `frame-template.html`) and adapt persistence/token paths from `.superpowers/brainstorm/` to OAT conventions (`.oat/brainstorm/.last-port`, `.last-token`, repo walk-up, user-scope fallback). Update `references/visual-companion.md` and `SKILL.md` step 3 for session keys, `--open`, restart semantics, and 4h idle default. + +**When this is the right choice:** Security and reconnect behavior are the headline v6 changes; partial ports leave known vulnerabilities and brittle restarts. + +**Tradeoffs:** Breaks byte-for-byte parity with v5.0.7 (document in `NOTICES.md`). Touches publishable bundled assets → lockstep public package version bump and `pnpm release:validate`. + +### Approach 2: Security-only minimal patch + +**Description:** Port only session-key auth, file sandbox, and security headers into existing OAT files without full helper/start-server v6 features. + +**When this is the right choice:** If schedule is extremely tight and resilience features can wait. + +**Tradeoffs:** Leaves restart persistence, stop-server instance-ID guard, status pill/tombstone UI, and Windows hardening gaps; higher merge cost later. + +### Approach 3: Reimplement from scratch in OAT CLI + +**Description:** Move server into `packages/cli` as `oat brainstorm visual-server` with TypeScript. + +**When this is the right choice:** Long-term if the bash/Node bundle becomes unmaintainable. + +**Tradeoffs:** Large scope; contradicts "ported not reimplemented" decision in `independent-brainstorming`; not justified for this increment. + +### Chosen Direction + +**Approach:** Full v6.0.3 bundle port with OAT path adaptations (Approach 1). + +**Rationale:** v6 security model closes real local/remote exposure; resilience features match how agents actually use long brainstorm sessions; upstream MIT code is battle-tested. OAT-specific path and offer semantics stay as-is. + +**User validated:** Yes — user chose quick-start to plan this work after the release review. + +## Options Considered + +### Option A: Pin upstream at v6.0.3 tag + +**Description:** Copy files from `obra/superpowers` tag `v6.0.3` and apply mechanical `.superpowers` → `.oat` substitutions plus repo/user persistence rules from current OAT `start-server.sh`. + +**Pros:** + +- Clear provenance line in `NOTICES.md` +- Matches release notes the review was based on + +**Cons:** + +- Must re-apply OAT-only behaviors (repo walk-up, `~/.oat/brainstorm/` fallback, Codex foreground auto-detect) + +**Chosen:** A + +**Summary:** Port from v6.0.3 tag; preserve OAT persistence resolution order from current bundle. + +### Option B: Skip Superpowers telemetry/branding + +**Description:** Omit upstream telemetry hooks and Superpowers logo URLs when adapting `server.cjs`. + +**Pros:** Keeps OAT neutral branding (frame template already says "Superpowers Brainstorming" — may rebrand to OAT in same pass) + +**Cons:** Minor diff from upstream + +**Chosen:** B for telemetry; rebrand frame header to OAT in the same implementation phase (cosmetic, low risk). + +## Key Decisions + +1. **Upstream version target:** Superpowers **v6.0.3** visual-companion scripts and docs (brainstorming skill server bundle only). +2. **OAT persistence:** Keep three-tier resolution (`--project-dir` → repo `.oat/` walk-up → `~/.oat/brainstorm/`); store `.last-port` / `.last-token` under the same prefix as sessions (repo or user scope), not `.superpowers/`. +3. **Skill behavior preserved:** Do not regress Activation Contract, conditional visual offer, or destination dispatcher — only update companion startup/usage prose and flags (`--open`, same `--project-dir` on restart). +4. **Publishable guardrail:** Bundled skill asset changes require lockstep bump of all five public packages and `pnpm release:validate` before merge. +5. **Testing:** Extend existing `visual-companion-smoke.test.ts` for session key, restart reuse, and stop-server instance-ID behavior. +6. **Deferred:** Inline synthesis self-review in `oat-brainstorm` step 8 (Superpowers v5.0.6 brainstorming skill change) — already lives in `oat-project-design`; not required for visual parity. + +## Constraints + +- Minimize scope: visual bundle + references + smoke tests + NOTICES/version/docs touchpoints only. +- Do not reformat upstream script files for oxfmt (existing ignore patterns for bundled scripts). +- Provider-linked skill views sync from `.agents/skills/oat-brainstorm/` via normal OAT sync — canonical edits under `.agents/skills/`. +- Live dogfood (`bl-7d5b`) remains a follow-up; this project delivers the port and automated smoke coverage. + +## Success Criteria + +- Visual companion requires session key on HTTP and WebSocket; file server rejects traversal/symlinks/dotfiles; security headers present. +- Restart with same `--project-dir` (or same repo context) reuses port/key so browser tab reconnects. +- Idle default 4 hours (configurable via `--idle-timeout-minutes`); stop-server verifies server instance ID before kill. +- Client shows connection status and paused overlay after extended disconnect (v6 helper behavior). +- `SKILL.md` and `visual-companion.md` document key URL, `--open`, restart, and idle semantics. +- `pnpm test` visual-companion smoke passes; `pnpm release:validate` passes after version bump. +- `NOTICES.md` updated to reference Superpowers v6.0.3 adapted port (no longer claims byte-for-byte v5.0.7 for all files). + +## Out of Scope + +- Superpowers subagent-driven development, writing-plans, task-reviewer, SDD workspace changes (v6.0.0–v6.0.3 non-visual release notes). +- Harness-specific bootstrap/tool-mapping (Kimi, Pi, Antigravity, Copilot SessionStart). +- Replacing OAT destination model with Superpowers design→spec→writing-plans pipeline. +- `oat brainstorm visual-server` CLI wrapper (deferred in original project; raw scripts remain). +- Live LLM dogfood scenarios (`bl-7d5b`) — separate backlog item. +- Synthesis self-review checklist in `oat-brainstorm` step 8 (deferred). + +## Deferred Ideas + +- **Synthesis self-review before handoff** — Superpowers v5.0.6 inline spec self-review adapted for `oat-brainstorm` step 8; low priority once visual parity ships. +- **CLI wrapper for visual server** — convenience command; not needed for parity. +- **Frame template full OAT visual rebrand** — beyond header text if design wants a distinct theme. + +## Open Questions + +- **User-scope token persistence:** When fallback is `~/.oat/brainstorm/`, should `.last-port`/`.last-token` live at `~/.oat/brainstorm/` root (mirroring repo layout)? **Assumption:** yes, for symmetric restart behavior outside a repo. +- **Frame title branding:** Rename "Superpowers Brainstorming" to "OAT Brainstorm" in frame template during port? **Assumption:** yes, cosmetic alignment with OAT product. + +## Assumptions + +- Superpowers v6.0.3 `skills/brainstorming/scripts/*` is the canonical upstream source set. +- Existing smoke test harness can assert keyed URLs and auth rejection without a full browser. +- No breaking change to agent workflows beyond requiring agents to use the keyed URL from `server-started` JSON (already how v6 upstream works). + +## Risks + +- **Regression in OAT persistence paths** + - **Likelihood:** Medium + - **Impact:** Medium + - **Mitigation:** Preserve current smoke tests for repo/user/`--project-dir` paths; add restart/key tests. + +- **Publishable release validation failure** + - **Likelihood:** Low + - **Impact:** High (blocks merge) + - **Mitigation:** Plan includes lockstep version bump + `pnpm release:validate` task. + +- **Provider skill sync drift** + - **Likelihood:** Low + - **Impact:** Low + - **Mitigation:** Bump `oat-brainstorm` skill `version:` in frontmatter; run `oat sync --scope all` before PR if needed. + +## Next Steps + +Proceed to quick-start **plan generation** (straight to plan — architecture is understood from release review; lightweight design skipped by user intent). diff --git a/.oat/projects/shared/brainstorm-visual-companion-v6/implementation.md b/.oat/projects/shared/brainstorm-visual-companion-v6/implementation.md new file mode 100644 index 0000000000..352795bc91 --- /dev/null +++ b/.oat/projects/shared/brainstorm-visual-companion-v6/implementation.md @@ -0,0 +1,258 @@ +--- +oat_status: complete +oat_ready_for: null +oat_blockers: [] +oat_last_updated: 2026-07-02 +oat_current_task_id: null +oat_generated: false +--- + +# Implementation: brainstorm-visual-companion-v6 + +**Started:** 2026-07-02 +**Last Updated:** 2026-07-02 + +> Resume pointer: all tasks complete — awaiting final review. + +## Progress Overview + +| Phase | Status | Tasks | Completed | +| ------- | -------- | ----- | --------- | +| Phase 1 | complete | 4 | 4/4 | +| Phase 2 | complete | 2 | 2/2 | +| Phase 3 | complete | 2 | 2/2 | +| Phase 4 | complete | 1 | 1/1 | + +**Total:** 9/9 tasks resolved (p04-t01 intentionally skipped — no user-facing doc delta) + +--- + +## Phase 1: Port Superpowers v6 visual bundle + +**Status:** complete + +### Task p01-t01: Import and adapt server.cjs + +**Status:** completed (`feat(p01-t01)` — e7500d71) + +Ported v6.0.3 `server.cjs`: session-key auth (HTTP + WebSocket, `?key=`/cookie bootstrap), sandboxed `/files/` serving (traversal/symlink/dotfile rejection), security headers (`Cache-Control: no-store`, `X-Frame-Options: DENY`, plus additive `Referrer-Policy` / `CSP frame-ancestors 'none'` / `Cross-Origin-Resource-Policy`), WebSocket max-frame guard, `BRAINSTORM_PORT_FILE` reuse, `BRAINSTORM_IDLE_TIMEOUT_MS` (default 4h), `--open`. Stripped Superpowers telemetry/branding. Verify: `node --check` pass. + +### Task p01-t02: Port helper.js and frame-template.html + +**Status:** completed (`feat(p01-t02)` — a06beecc) + +Ported v6 client resilience (exponential-backoff reconnect, status pill states, paused/tombstone overlay, keyed reload-after-recovery). Rebranded frame ``/header to "OAT Brainstorm". Retained OAT `.indicator-bar`/`#indicator-text` UI (see Deviations). Verify: `.status`/`setStatus` presence confirmed. + +### Task p01-t03: Adapt start-server.sh for OAT persistence + v6 flags + +**Status:** completed (`feat(p01-t03)` — 5ed75687) + +Merged v6 launcher into OAT 3-tier persistence (`--project-dir` → repo walk-up → `~/.oat/brainstorm/`); `.last-port`/`.last-token` under `.oat/brainstorm/`. Added `--idle-timeout-minutes` (min→ms, exports `BRAINSTORM_IDLE_TIMEOUT_MS`), `--open`, `umask 077`, `server-instance-id` + `--brainstorm-server-id`, Windows shell handling + OWNER_PID clearing. Verify: `bash -n` pass. + +### Task p01-t04: Port stop-server.sh instance-ID guard + +**Status:** completed (`feat(p01-t04)` — ef7b1b9f) + +Ported v6 instance-ID guard: verifies PID cmdline carries matching `--brainstorm-server-id` before signaling; writes `server-stopped` marker; retains persistent (non-`/tmp`) session dirs. Verify: `bash -n` pass; stale-PID guard confirmed to fail closed. + +### Phase 1 Summary + +- **Outcome:** OAT brainstorm visual companion bundle brought to Superpowers v6.0.3 security/resilience parity — session-key auth on HTTP+WS, sandboxed static file serving, security headers, resilient reconnecting client, OAT-native port/token persistence, and a safe instance-verified stop path. +- **Key files:** `.agents/skills/oat-brainstorm/scripts/{server.cjs,helper.js,frame-template.html,start-server.sh,stop-server.sh}` +- **Verification:** per-task static checks (`node --check`, `bash -n`, selector grep) + implementer end-to-end smoke (403 unauth / keyed 200 with headers / traversal+dotfile 404 / restart port reuse / stale-PID guard) + reviewer empirical re-verification (auth HTTP+WS, sandbox, instance guard fail-closed). Pre-existing `visual-companion-smoke.test.ts` (5 tests) still passes unmodified. +- **Notable decisions/deviations:** retained OAT indicator-bar UI alongside v6 status pill (see Deviations); added defensive security headers beyond the two named in plan (additive). + +--- + +## Phase 2: Update skill and reference docs + +**Status:** complete + +### Task p02-t01: Update visual-companion.md + +**Status:** completed (`docs(p02-t01)` — 5ea69291) + +Documented v6 session-key URL/cookie auth (403 on unauth), `--open`, restart port/key reuse, 4h idle default + `--idle-timeout-minutes`, status-pill/paused-overlay/backoff-reconnect behavior. Fixed a stale "30 minutes" idle claim to the actual 4h default. OAT persistence-path examples preserved. Verify: `pnpm oat:validate-skills` → OK (53 skills). + +### Task p02-t02: Update oat-brainstorm SKILL.md step 3 + +**Status:** completed (`docs(p02-t02)` — 60740f71) + +Updated Step 3 accept branch (pass `--open`, preserve keyed `url`, restart reuse, 4h idle note). Activation Contract + destination logic untouched (diff-verified). Bumped frontmatter `version: 1.1.0 → 1.2.0` (sole owner of the skill version bump per plan). Verify: `pnpm oat:validate-skills` → OK. + +### Phase 2 Summary + +- **Outcome:** Skill reference + activation prose now describe the v6 companion (keyed URL, `--open`, restart reuse, idle) accurately against shipped code; canonical skill version bumped for the release guardrail. +- **Key files:** `.agents/skills/oat-brainstorm/references/visual-companion.md`, `.agents/skills/oat-brainstorm/SKILL.md` +- **Verification:** `pnpm oat:validate-skills` (OK); docs fact-checked against `server.cjs`/`start-server.sh`/`helper.js`; Activation Contract preserved. +- **Notable decisions/deviations:** none. + +--- + +## Phase 3: Tests and release validation + +**Status:** complete + +### Task p03-t01: Extend visual-companion smoke tests + +**Status:** completed (`test(p03-t01)` — e1ea29bd; hardened in fix 1f1e623f) + +Added 5 v6-hardening smoke tests (keyed URL, unauth 403 + security headers, `/files/` traversal+dotfile 4xx, `/files/` symlink-escape 4xx, restart port+key reuse, stop-server stale-instance guard). Review found the traversal/dotfile assertions were initially vacuous (payloads hit nonexistent files); fix `1f1e623f` replaced them with real fixtures (sentinel above `CONTENT_DIR`, real `.hidden.html`) asserting 4xx + no content leak, and added the restart key-reuse assertion. RED/GREEN verified by removing the server guard (test fails) and restoring (test passes). Verify: 10/10 tests pass. + +### Task p03-t02: NOTICES, lockstep versions, release validate, provider sync + +**Status:** completed (`chore(p03-t02)` — 4f993aa8) + +`NOTICES.md` updated to v6.0.3 adapted-port provenance (per-file adaptation notes, no longer byte-for-byte v5.0.7). Lockstep bump of all 5 public packages to `0.1.34` (branch already carried 0.1.33) + `public-package-versions.json`. Ran `pnpm run cli -- sync --scope all` (provider views: "No changes required"; only `.oat/sync/manifest.json` `oatVersion` changed). Verify: `pnpm release:validate` PASSED (5 packages). + +### Phase 3 Summary + +- **Outcome:** Automated regression coverage for the v6 security/resilience/lifecycle guarantees (auth, headers, traversal/dotfile/symlink sandbox, restart port+key reuse, stop-server instance guard) — now genuinely load-bearing after the fix — plus release provenance and a validated lockstep version bump. +- **Key files:** `packages/cli/src/integration/visual-companion-smoke.test.ts`, `NOTICES.md`, 5 `package.json`, `packages/cli/assets/public-package-versions.json`, `.oat/sync/manifest.json` +- **Verification:** 10/10 smoke tests pass; `pnpm release:validate` passes (5 packages @ 0.1.34); traversal/dotfile/symlink tests RED/GREEN-proven; lint + format clean. +- **Notable decisions/deviations:** version landed on 0.1.34 (branch pre-carried 0.1.33); 1 fix iteration to make traversal/dotfile tests real. + +--- + +## Phase 4: Optional docs touchpoint + +**Status:** complete (skipped — no delta) + +### Task p04-t01: Update tool-packs docs if companion behavior changed materially + +**Status:** skipped — no user-facing doc delta + +Assessed `apps/oat-docs/docs/cli-utilities/tool-packs.md` (Brainstorm pack "Visual companion" bullet). It is a high-level pack-lifecycle description that already defers operational/security details (session-key auth, `--open`, restart reuse, idle) to `.agents/skills/oat-brainstorm/references/visual-companion.md` (updated in p02-t01) and its persistence-path claims remain accurate under v6. No stale or missing user-facing content, so per the plan's explicit conditional-skip path no edit was made. No commit. + +### Phase 4 Summary + +- **Outcome:** No docs change required — the tool-packs overview already points to the (now v6-accurate) reference doc; forcing a duplicate note would add redundancy, not coverage. +- **Key files:** none. +- **Verification:** read-only assessment of `tool-packs.md` vs the shipped v6 behavior and the updated reference doc. +- **Notable decisions/deviations:** intentional no-op skip per plan Phase 4 design. + +--- + +## Orchestration Runs + +<!-- orchestration-runs-start --> + +### Run 1 — 2026-07-02 + +**Branch:** parity-check +**Tier:** 1 (subagents) +**Policy:** merge-strategy=sequential, retry-limit=2, dispatch-ceiling=sonnet +**Phases:** 4 executed, 4 passed, 0 failed, 0 stopped + +#### Phase Outcomes + +| Phase | Implementer | Review | Fix Iterations | Disposition | +| ----- | ----------- | ------------- | -------------- | ------------------ | +| p01 | DONE | pass | 0/2 | committed | +| p02 | DONE | pass | 0/2 | committed | +| p03 | DONE | fail→pass | 1/2 | committed | +| p04 | DONE (skip) | n/a (no diff) | 0/2 | skipped (no delta) | + +#### Parallel Groups + +- None — all phases sequential (`oat_plan_parallel_groups: []`). + +#### Dispatch Notes + +- Dispatch: p01 implementation + review via Claude Code Task subagents, `model_axis=selected:sonnet`, `effort_axis=not-applicable`, ceiling sonnet (enforced — Task model arg). No escalation needed. +- Upstream v6.0.3 source staged locally (scratchpad clone of `obra/superpowers` @ `v6.0.3`) and provided to the implementer; no network fetch inside the subagent. +- Discovered an unprocessed plan re-review (`artifact-plan-review-2026-06-28-v2.md`, 0C/1I/1M/0m) that post-dated the first receive. Folded both findings into the plan before reaching p03: symlink `/files/` regression test → `p03-t01`; provider-view sync (`oat sync --scope all`) → `p03-t02`. v2 review archived; plan row → `fixes_completed`. + +#### Outstanding Items + +- All carried minors (p01×2, p02×1) plus 1 new final-review minor were dispositioned at final review as **accept-defer** with rationale — see `## Review Received: final` → Deferred Findings. None blocking; final review passed. + +#### Artifact / Design Deltas + +Run-scoped snapshot only. Durable record is `## Deviations` below. + +| Task / Review | Source Artifact | Planned / Documented | Actual / Accepted | Reason | Source of Truth | Follow-up | +| ------------- | -------------------------------------------- | --------------------------------------------------------- | --------------------------------------------------- | ----------------------------------------------------------------- | --------------- | ------------------------------------------- | +| p01-t02 | plan.md p01-t02 / v6.0.3 frame-template.html | v6 upstream removes `.indicator-bar` selection-summary UI | OAT indicator-bar retained alongside v6 status pill | plan only asked to add v6 resilience + rebrand, not remove OAT UI | implementation | none (deliberate; documented in Deviations) | + +<!-- orchestration-runs-end --> + +--- + +## Final Summary (for PR/docs) + +**What shipped:** OAT's `oat-brainstorm` visual companion is now at Superpowers v6.0.3 security/resilience/lifecycle parity while preserving OAT's persistence paths and conditional-offer skill semantics: + +- **Security:** session-key auth on HTTP + WebSocket (`?key=` + cookie bootstrap); sandboxed `/files/` serving that rejects path traversal, dotfiles, and symlink escape; security headers (`Cache-Control: no-store`, `X-Frame-Options: DENY`, plus additive `Referrer-Policy` / CSP `frame-ancestors 'none'` / `Cross-Origin-Resource-Policy`); WebSocket max-frame guard. +- **Resilience (client):** exponential-backoff reconnect, status-pill states, paused/tombstone overlay, keyed reload after server recovery; frame rebranded "OAT Brainstorm". OAT's existing selection-summary indicator bar retained alongside the v6 status pill. +- **Lifecycle:** OAT three-tier persistence (`--project-dir` → repo walk-up → `~/.oat/brainstorm/`) with `.last-port`/`.last-token`; restart with the same `--project-dir` reuses port + key; `--open`; configurable idle timeout (`--idle-timeout-minutes` → `BRAINSTORM_IDLE_TIMEOUT_MS`, 4h default); `umask 077`; instance-verified `stop-server` (matches `--brainstorm-server-id` before signaling, writes `server-stopped`). +- **Docs/provenance:** `references/visual-companion.md` + `SKILL.md` step 3 document the v6 behavior; SKILL.md `version` 1.1.0 → 1.2.0; `NOTICES.md` updated to v6.0.3 adapted-port provenance. +- **Release:** lockstep bump of all 5 public packages to 0.1.34; provider views synced (no drift); `pnpm release:validate` passes. + +**Key files/modules:** `.agents/skills/oat-brainstorm/scripts/{server.cjs,helper.js,frame-template.html,start-server.sh,stop-server.sh}`, `.agents/skills/oat-brainstorm/{SKILL.md,references/visual-companion.md}`, `packages/cli/src/integration/visual-companion-smoke.test.ts`, `NOTICES.md`, 5 public `package.json` + `public-package-versions.json`. + +**Verification performed:** 10/10 visual-companion smoke tests pass (auth/headers, traversal/dotfile/symlink sandbox — RED/GREEN-proven load-bearing, restart port+key reuse, stop-server instance guard); `pnpm release:validate` passes (5 packages @ 0.1.34); `pnpm oat:validate-skills` passes; lint + format clean; implementer + reviewer both ran live end-to-end server exercises. + +**Design deltas:** OAT indicator-bar UI retained vs v6 upstream removal (see Deviations). Carried non-blocking minors for final-review disposition: p01 — plaintext-HTTP WS assumption for an unsupported HTTPS-tunnel mode (fails closed) + a pre-existing dead "kill existing server" block; p02 — WS-auth doc wording nit. + +--- + +## Review Received: final + +**Date:** 2026-07-02 +**Review artifact:** reviews/final-review-2026-07-02.md +**Verdict:** passed (0 Critical, 0 Important, 4 Minor) + +All 7 discovery Success Criteria (SC#1–SC#7) confirmed implemented via code reading + live spot-check (start-server → curl unauth/authed/traversal/dotfile → stop-server). Tests 10/10; `release:validate` passes (5 packages @ 0.1.34); `oat:validate-skills` OK. No Critical/Important findings; no fix tasks created. + +### Deferred Findings (Minor — accept-defer, auto-disposition) + +All four minors were dispositioned as **accept-defer** with rationale (not converted to fix tasks — converting would be scope creep on a passing implementation; several are explicitly outside declared success criteria): + +- **p01-m1** — WebSocket Origin check + client `ws://` URL assume plaintext HTTP. Defer: fails closed, HTTPS-tunnel deployment is not a stated success criterion, no current use. Follow-up trigger: if an HTTPS/tunnel deployment mode is ever added. +- **p01-m2** — vestigial dead "kill any existing server" block in `start-server.sh`. Defer: unreachable by construction and pre-existing before this project (not introduced here); zero functional impact. Follow-up: opportunistic cleanup. +- **p02-m1** — WS-auth doc wording implies cookie-only reliance. Defer: doc-precision nit; does not misstate security behavior. Follow-up: tidy on next docs pass. +- **final-m (new)** — `nextReconnectDelay` exported "for unit tests" but has no test. Defer: SC#4 client runtime behavior is explicitly manual-scope per plan; low value. Follow-up: add if a client unit-test harness is introduced. + +**Next:** implementation complete and final review passed → generate summary / open PR (`oat-project-summary` → `oat-project-pr-final`). + +--- + +## Deviations + +### Phase 1: indicator-bar UI retained alongside v6 status pill + +**Source of truth:** discovery.md Key Decisions #3 ("do not regress ... only +update companion startup/usage prose and flags") and the general instruction +to preserve existing OAT behavior except where plan/discovery calls for a +change. + +**What diverged:** Superpowers v6.0.3 upstream removed the `.indicator-bar` / +`#indicator-text` selection-summary UI entirely from `frame-template.html` and +`helper.js` in favor of the new status pill. Plan p01-t02 only asked to port +the v6 resilience features (status pill, backoff reconnect, paused overlay, +keyed reload) and to rebrand the header — it did not ask to remove the +existing indicator bar. A literal "replace the whole file with upstream" +port would have silently dropped that OAT UI behavior. + +**Resolution:** Kept the pre-existing OAT indicator-bar markup/CSS in +`frame-template.html` (`#claude-content` id retained, not renamed to v6's +`#frame-content`) and re-added the original click-handler block in +`helper.js` that updates `#indicator-text`, merged alongside v6's new +`setStatus`/backoff/tombstone logic. Net effect: OAT gets the full v6 +resilience/security UI plus the existing selection-summary bar, with no +functionality regression either direction. + +**Follow-up disposition:** No further action expected; flagging for reviewer +awareness since it's a deliberate non-byte-for-byte deviation from the v6.0.3 +source file (already anticipated by discovery.md: "Breaks byte-for-byte +parity with v5.0.7 (document in NOTICES.md)" — this is an additional, +smaller byte-for-byte divergence from v6.0.3 itself, on top of the OAT +persistence-path/branding differences already called out in plan.md). + +--- + +## References + +- Plan: `plan.md` +- Discovery: `discovery.md` diff --git a/.oat/projects/shared/brainstorm-visual-companion-v6/plan.md b/.oat/projects/shared/brainstorm-visual-companion-v6/plan.md new file mode 100644 index 0000000000..ecd860efcb --- /dev/null +++ b/.oat/projects/shared/brainstorm-visual-companion-v6/plan.md @@ -0,0 +1,378 @@ +--- +oat_status: complete +oat_ready_for: oat-project-implement +oat_blockers: [] +oat_last_updated: 2026-06-28 +oat_phase: plan +oat_phase_status: complete +oat_plan_hill_phases: ['p04'] +oat_plan_parallel_groups: [] +oat_auto_review_at_hill_checkpoints: true +oat_plan_source: quick +oat_import_reference: null +oat_import_source_path: null +oat_import_provider: null +oat_generated: false +--- + +# Implementation Plan: brainstorm-visual-companion-v6 + +> Execute this plan using `oat-project-implement`. + +**Goal:** Bring OAT's `oat-brainstorm` visual companion to parity with Superpowers v6.0.3 security, resilience, and lifecycle behavior while preserving OAT persistence paths and conditional-offer skill semantics. + +**Architecture:** Replace the v5.0.7 Node/bash bundle under `.agents/skills/oat-brainstorm/scripts/` with an adapted Superpowers v6.0.3 port (session-key auth, sandboxed static serving, port/token persistence, resilient client UI, safe stop-server). Update skill/reference prose and integration smoke tests; bump bundled-asset version metadata and run release validation. + +**Tech Stack:** Node.js (server.cjs), bash (start/stop scripts), HTML/JS (frame + helper), Vitest integration smoke test, OAT skill frontmatter, lockstep npm packages. + +**Commit Convention:** `feat(pNN-tNN): {description}` for functional port tasks; `chore(pNN-tNN): {description}` for bookkeeping/docs/version tasks. + +## Planning Checklist + +- [x] Confirmed HiLL checkpoints with user (none — quick mode, empty `oat_plan_hill_phases`) +- [x] Set `oat_plan_hill_phases` in frontmatter +- [x] Evaluated phases for parallelism opportunities +- [x] Set `oat_plan_parallel_groups` in frontmatter (sequential — shared skill directory) + +--- + +## Parallelism + +All phases modify the same skill bundle or its tests/docs. Phases run **sequentially** (`oat_plan_parallel_groups: []`). Phase 1 must land before smoke tests in Phase 3 can assert v6 behavior. + +--- + +## Phase 1: Port Superpowers v6 visual bundle + +### Task p01-t01: Import and adapt server.cjs + +**Files:** + +- Modify: `.agents/skills/oat-brainstorm/scripts/server.cjs` + +**Step 1: Baseline check (GREEN baseline)** + +Run: `pnpm --filter @open-agent-toolkit/cli exec vitest run src/integration/visual-companion-smoke.test.ts` +Expected: Current tests pass on v5.0.7 baseline (establishes green before port). + +**Step 2: Port v6 server (GREEN)** + +Copy `skills/brainstorming/scripts/server.cjs` from `obra/superpowers` tag `v6.0.3` and adapt: + +- Session-key auth on HTTP + WebSocket (`?key=`, cookie bootstrap) +- Sandboxed `/files/` serving (no symlinks, dotfiles, traversal) +- Security headers (`Cache-Control: no-store`, `X-Frame-Options: DENY`) +- Configurable idle timeout via `BRAINSTORM_IDLE_TIMEOUT_MS` (default 4h) +- Port reuse via `BRAINSTORM_PORT_FILE` +- WebSocket max frame size guard +- Omit Superpowers telemetry/branding hooks + +Preserve OAT `CONTENT_DIR` / `STATE_DIR` layout and existing JSON log events consumed by agents. + +**Step 3: Verify** + +Run: `node --check .agents/skills/oat-brainstorm/scripts/server.cjs` +Expected: Syntax OK. + +**Step 4: Commit** + +```bash +git add .agents/skills/oat-brainstorm/scripts/server.cjs +git commit -m "feat(p01-t01): port v6 visual companion server with session auth" +``` + +--- + +### Task p01-t02: Port helper.js and frame-template.html + +**Files:** + +- Modify: `.agents/skills/oat-brainstorm/scripts/helper.js` +- Modify: `.agents/skills/oat-brainstorm/scripts/frame-template.html` + +**Step 1: Port client resilience (GREEN)** + +From v6.0.3: + +- Session key in WebSocket URL from `sessionStorage` +- Status pill states (connecting/connected/reconnecting/disconnected) +- Exponential backoff reconnect + paused overlay after disconnect +- Keyed reload after server recovery + +Update frame template header branding to **OAT Brainstorm** (remove Superpowers title); ensure `.status` element matches helper expectations. + +**Step 2: Verify** + +Manual: grep frame for `.status` and helper for `setStatus` — both present. + +Note: reconnect/status-pill/paused-overlay runtime behavior (SC#4) is verified manually in a browser and is intentionally outside automated smoke scope (the smoke harness cannot drive a browser). + +**Step 3: Commit** + +```bash +git add .agents/skills/oat-brainstorm/scripts/helper.js .agents/skills/oat-brainstorm/scripts/frame-template.html +git commit -m "feat(p01-t02): port v6 companion client UI and OAT frame branding" +``` + +--- + +### Task p01-t03: Adapt start-server.sh for OAT persistence + v6 flags + +**Files:** + +- Modify: `.agents/skills/oat-brainstorm/scripts/start-server.sh` + +**Step 1: Port v6 launcher behaviors (GREEN)** + +Merge v6.0.3 `start-server.sh` capabilities with OAT persistence resolution: + +1. `--project-dir` → `<path>/.oat/brainstorm/<session>/` and `<path>/.oat/brainstorm/.last-port` + `.last-token` +2. Repo walk-up → `<repo>/.oat/brainstorm/...` +3. Fallback → `~/.oat/brainstorm/...` with token files at `~/.oat/brainstorm/.last-port` / `.last-token` + +Add v6 flags: `--open`, `--idle-timeout-minutes`, `umask 077`, `server-instance-id`, Windows `is_windows_like_shell` + clear `OWNER_PID`, pass `--brainstorm-server-id` to node. + +`--idle-timeout-minutes` must be converted to milliseconds and exported as `BRAINSTORM_IDLE_TIMEOUT_MS` so the launcher and `server.cjs` agree on units (server reads ms; default 4h). + +**Step 2: Verify** + +Run: `bash -n .agents/skills/oat-brainstorm/scripts/start-server.sh` +Expected: No syntax errors. + +**Step 3: Commit** + +```bash +git add .agents/skills/oat-brainstorm/scripts/start-server.sh +git commit -m "feat(p01-t03): adapt v6 start-server for OAT paths and restart reuse" +``` + +--- + +### Task p01-t04: Port stop-server.sh instance-ID guard + +**Files:** + +- Modify: `.agents/skills/oat-brainstorm/scripts/stop-server.sh` + +**Step 1: Port safe shutdown (GREEN)** + +From v6.0.3: verify PID carries matching `--brainstorm-server-id` before signaling; write `server-stopped` marker; keep OAT behavior of retaining persistent session dirs (not only `/tmp`). + +**Step 2: Verify** + +Run: `bash -n .agents/skills/oat-brainstorm/scripts/stop-server.sh` +Expected: No syntax errors. + +**Step 3: Commit** + +```bash +git add .agents/skills/oat-brainstorm/scripts/stop-server.sh +git commit -m "feat(p01-t04): port v6 stop-server instance verification" +``` + +--- + +## Phase 2: Update skill and reference docs + +### Task p02-t01: Update visual-companion.md + +**Files:** + +- Modify: `.agents/skills/oat-brainstorm/references/visual-companion.md` + +**Step 1: Align reference with v6 behavior (GREEN)** + +Document: + +- Session key in URL and cookie auth requirement +- `--open` after user accepts companion +- Restart with same `--project-dir`/repo context reuses port/key (no new URL) +- 4h idle default and `--idle-timeout-minutes` +- Paused overlay / reconnect behavior +- Pre-flight alive check before pushing screens + +Keep OAT persistence path examples (`.oat/brainstorm/`, user-scope fallback). + +**Step 2: Verify** + +Run: `pnpm oat:validate-skills` (or repo equivalent skill validation) +Expected: Skill pack validates. + +**Step 3: Commit** + +```bash +git add .agents/skills/oat-brainstorm/references/visual-companion.md +git commit -m "docs(p02-t01): document v6 visual companion behavior in reference" +``` + +--- + +### Task p02-t02: Update oat-brainstorm SKILL.md step 3 + +**Files:** + +- Modify: `.agents/skills/oat-brainstorm/SKILL.md` + +**Step 1: Update activation/handoff prose (GREEN)** + +In Step 3 (visual companion offer/accept): + +- On accept, pass `--open` when starting server (after user approval) +- Instruct agent to preserve keyed URL from JSON; restart with same path resolution +- Note 4h idle / restart semantics +- Bump skill frontmatter `version:` here — this task is the sole owner of the skill version bump (required for the skill change; not repeated in p03-t02) + +Do **not** change Activation Contract or destination logic. + +**Step 2: Verify** + +Run: `pnpm oat:validate-skills` +Expected: Pass including version/frontmatter checks. + +**Step 3: Commit** + +```bash +git add .agents/skills/oat-brainstorm/SKILL.md +git commit -m "docs(p02-t02): align brainstorm skill with v6 companion startup" +``` + +--- + +## Phase 3: Tests and release validation + +### Task p03-t01: Extend visual-companion smoke tests + +**Files:** + +- Modify: `packages/cli/src/integration/visual-companion-smoke.test.ts` + +**Step 1: Add failing assertions (RED)** + +Add cases for: + +- `server-started` JSON includes keyed URL (`?key=`) +- Unauthenticated GET `/` returns 401/403 (per server behavior) +- Security headers present on responses: `Cache-Control: no-store` and `X-Frame-Options: DENY` (SC#1) +- Sandboxed `/files/` rejects traversal/dotfile payloads: an authenticated `GET /files/../server.cjs?key=<key>` (or a dotfile path) returns 4xx (SC#1) +- Sandboxed `/files/` rejects symlink escape: create a symlink fixture inside the served area pointing outside it, then assert an authenticated `GET /files/<symlink>?key=<key>` returns 4xx (SC#1). If symlink rejection is platform-limited, state that explicitly here and reconcile the success criterion. +- Restart with same `--project-dir` reuses port (read `.last-port`) +- `stop-server.sh` refuses stale/wrong instance (mock or integration-safe check) + +**Step 2: Implement until green (GREEN)** + +Run: `pnpm --filter @open-agent-toolkit/cli exec vitest run src/integration/visual-companion-smoke.test.ts` +Expected: All tests pass. + +**Step 3: Commit** + +```bash +git add packages/cli/src/integration/visual-companion-smoke.test.ts +git commit -m "test(p03-t01): cover v6 visual companion auth and restart behavior" +``` + +--- + +### Task p03-t02: NOTICES, lockstep versions, release validate + +**Files:** + +- Modify: `NOTICES.md` +- Modify: `packages/cli/package.json`, `packages/control-plane/package.json`, `packages/docs-config/package.json`, `packages/docs-theme/package.json`, `packages/docs-transforms/package.json` (lockstep bump) +- Sync (generated): provider views + `.oat/sync/manifest.json` (`.claude/`, `.cursor/`, etc.) if `oat sync` reports changes after the skill edits + +**Step 1: Update provenance (GREEN)** + +`NOTICES.md`: change referenced Superpowers version to v6.0.3; note adapted port (no longer byte-for-byte v5.0.7 for all script files). + +Bump all five public packages together per repo guardrail. + +**Step 1b: Refresh provider views (required — canonical skill changed)** + +The canonical `oat-brainstorm` skill/bundle changed in p01–p02, so provider-linked views must be refreshed per AGENTS.md. + +Run: `pnpm run cli -- sync --scope all` +Then stage and commit any resulting provider-view / `.oat/sync/manifest.json` changes alongside this task's release bookkeeping. + +**Step 2: Verify** + +Run: `pnpm release:validate` +Expected: Pass. + +Run: `pnpm --filter @open-agent-toolkit/cli exec vitest run src/integration/visual-companion-smoke.test.ts` (or full `pnpm --filter @open-agent-toolkit/cli test`) +Expected: Pass. + +**Step 3: Commit** + +```bash +git add NOTICES.md packages/cli/package.json packages/control-plane/package.json packages/docs-config/package.json packages/docs-theme/package.json packages/docs-transforms/package.json +# Also stage provider-view/sync outputs if `oat sync --scope all` changed them: +git add .oat/sync/manifest.json .claude .cursor 2>/dev/null || true +git commit -m "chore(p03-t02): record v6 port provenance and bump public packages" +``` + +--- + +## Phase 4: Optional docs touchpoint + +### Task p04-t01: Update tool-packs docs if companion behavior changed materially + +**Files:** + +- Modify: `apps/oat-docs/docs/cli-utilities/tool-packs.md` (only if step 3 / security semantics need user-facing mention) + +**Step 1: Assess diff** + +If brainstorm pack section still accurate after p02 changes, skip with note in implementation log. + +**Step 2: Update if needed (GREEN)** + +Add brief note: session-key URL, `--open`, restart reuse — only if missing. + +Run: `pnpm build:docs` (if docs changed) +Expected: Build passes. + +**Step 3: Commit (if changed)** + +```bash +git add apps/oat-docs/docs/cli-utilities/tool-packs.md +git commit -m "docs(p04-t01): note v6 visual companion security and restart behavior" +``` + +If skipped: record "skipped — no user-facing doc delta" in `implementation.md`. + +--- + +## Reviews + +| Cycle | Scope | Status | Date | Notes | +| ----- | ----- | --------------- | ---------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------- | +| plan | plan | fixes_completed | 2026-07-02 | artifact re-review 0C/1I/1M/0m resolved in-plan (symlink test → p03-t01; provider sync → p03-t02) — reviews/archived/artifact-plan-review-2026-06-28-v2.md | +| p01 | code | passed | 2026-07-02 | phase gate 0C/0I/0M/2m (minors non-blocking, carried to final) — reviews/p01-review-2026-07-02.md | +| p02 | code | passed | 2026-07-02 | phase gate 0C/0I/0M/1m (doc wording nit, carried to final) — reviews/p02-review-2026-07-02.md | +| p03 | code | passed | 2026-07-02 | gate 1I/1M (vacuous traversal/dotfile test + restart key-reuse) fixed in 1f1e623f, re-review pass — reviews/p03-review-2026-07-02-v2.md | +| p04 | code | passed | 2026-07-02 | no-op skip (docs touchpoint, no diff) — no gate review needed | +| final | code | passed | 2026-07-02 | 0C/0I/4m; SC#1–SC#7 all implemented (live-verified); 4 minors accept-deferred w/ rationale — reviews/final-review-2026-07-02.md | + +--- + +## Implementation Complete + +**Summary:** + +- Phase 1: 4 tasks — v6 server bundle port with OAT paths +- Phase 2: 2 tasks — skill/reference doc alignment +- Phase 3: 2 tasks — smoke tests + release validation +- Phase 4: 1 task — optional docs touchpoint + +**Total: 9 tasks** + +Ready for `oat-project-implement`. + +--- + +## References + +- Discovery: `discovery.md` +- Upstream: https://github.com/obra/superpowers/releases (v6.0.0 visual companion notes; v6.0.3 tag for file source) +- Prior OAT project: `.oat/repo/reference/project-summaries/20260507-independent-brainstorming.md` +- Live dogfood follow-up: `.oat/repo/reference/backlog/items/live-dogfood-oat-brainstorm.md` (`bl-7d5b`) diff --git a/.oat/projects/shared/brainstorm-visual-companion-v6/reviews/final-review-2026-07-02.md b/.oat/projects/shared/brainstorm-visual-companion-v6/reviews/final-review-2026-07-02.md new file mode 100644 index 0000000000..5935d9a6dd --- /dev/null +++ b/.oat/projects/shared/brainstorm-visual-companion-v6/reviews/final-review-2026-07-02.md @@ -0,0 +1,120 @@ +--- +oat_generated: true +oat_generated_at: 2026-07-02 +oat_review_scope: final +oat_review_type: code +oat_review_invocation: auto +oat_project: .oat/projects/shared/brainstorm-visual-companion-v6 +--- + +# Code Review: final (whole implementation) + +**Reviewed:** 2026-07-02 +**Scope:** Final holistic review of the entire `brainstorm-visual-companion-v6` project — Superpowers v6.0.3 visual-companion security/resilience/lifecycle parity port into `.agents/skills/oat-brainstorm/`, plus doc updates, smoke tests, and release provenance/versioning. +**Files reviewed:** 16 (5 server/client bundle files, `SKILL.md`, `references/visual-companion.md`, 1 smoke-test file, `NOTICES.md`, 5 public `package.json`, `public-package-versions.json`, `.oat/sync/manifest.json`) +**Commits:** `5d9bf2d9..d5b44d86` (18 commits spanning p01–p04 bookkeeping) + +## Summary + +This final pass independently re-verified every discovery Success Criterion against the shipped code (both by static reading and live server exercise — auth, headers, traversal/dotfile sandbox, and clean stop-server all reproduced exactly as the phase reviews reported), re-ran the two required verification commands (10/10 smoke tests pass; `pnpm release:validate` passes for 5 packages at 0.1.34), and confirmed the diff surface exactly matches the declared code-files-in-scope list with no undeclared drift (docs-index, provider views, and `tool-packs.md` are all correctly untouched). All three carried-forward Minor findings from the phase reviews are still present in the code exactly as described and remain genuinely Minor — none blocks merge. One new Minor observation is added here (an unused test seam), also non-blocking. No Critical or Important findings. + +## Findings + +### Critical + +None. + +### Important + +None. + +### Minor + +- **(Carried forward, p01-m1) WebSocket auth hardcodes plaintext-HTTP scheme, narrowing an anticipated deployment mode** (`.agents/skills/oat-brainstorm/scripts/server.cjs:311-317`, `.agents/skills/oat-brainstorm/scripts/helper.js:32-35`) + - Issue: `isAllowedWebSocketOrigin()` only accepts `origin === 'http://' + host`, and the client's `websocketUrl()` hardcodes `'ws://' + window.location.host`. If the companion were fronted by an HTTPS-terminating tunnel/reverse proxy, the browser's real Origin would be `https://...` and the browser would block a mixed-content `ws://` attempt from an `https:` page — the WS leg would fail even with a valid session key. Confirmed unchanged from the p01 review; still present verbatim at the cited lines. Fails closed (no auth bypass); HTTPS-tunnel support is not a discovery.md Success Criterion. + - **Disposition: accept-defer.** Not a regression against any stated SC (SC#1 only requires session-key auth on plaintext loopback/tunnel/remote HTTP binds, which this satisfies), and it fails closed rather than open. Fixing it (deriving scheme from `window.location.protocol` and accepting both `http://`/`https://` origins) is a reasonable follow-up if/when HTTPS-tunnel deployment becomes a real use case, but there is no evidence that mode is used today. No plan task needed for this project; worth a one-line backlog note if HTTPS-tunnel support is ever requested. + +- **(Carried forward, p01-m2) Vestigial dead-code block carried through the rewrite** (`.agents/skills/oat-brainstorm/scripts/start-server.sh:185-190`) + - Issue: The "Kill any existing server" block guards on `[[ -f "$PID_FILE" ]]`, but `PID_FILE` lives under a `SESSION_DIR` that is always freshly generated (`SESSION_ID="$$-$(date +%s)"`) and `mkdir -p`'d immediately beforehand, so the file can never exist at this point — the block is dead code. Confirmed unchanged and still present verbatim at lines 185-190. This dead block predates the v6 port (present in the pre-port baseline at `cff2146f`); p01-t03 rewrote the surrounding launcher without removing it. Restart/port-reuse safety is correctly handled elsewhere (`server.cjs`'s `EADDRINUSE` fallback logic). + - **Disposition: accept-defer.** Purely cosmetic/misleading-to-future-readers; zero functional or security impact (confirmed dead by construction, not just empirically). Not worth a dedicated fix-now task for this project; fine to clean up opportunistically the next time `start-server.sh` is touched. + +- **(Carried forward, p02-m1) WS-authentication doc wording implies cookie-only reliance** (`.agents/skills/oat-brainstorm/references/visual-companion.md:49`) + - Issue: The doc states the WebSocket upgrade authenticates "via the cookie without repeating the key in the address bar." In the shipped client, `websocketUrl()` (`helper.js:32-35`) still appends `?key=<sessionKey>` to the `ws://` URL whenever a session key is present in `sessionStorage` — the WS handshake carries the key redundantly alongside the cookie; it does not rely on the cookie exclusively. The statement is only true of the visible address bar, not of the WS request itself. Confirmed the sentence is unchanged since the p02 review. + - **Disposition: accept-defer.** Does not mislead an agent operationally (the URL to hand the user is unaffected either way) and does not describe incorrect security behavior — only imprecise mechanism wording for a future doc maintainer. Low priority; fine to tighten in a future doc pass touching this file, consistent with the p02 review's own suggested rewrite. + +- **(New) `nextReconnectDelay` is exported "for unit tests" but has no unit test** (`.agents/skills/oat-brainstorm/scripts/helper.js:6-12`) + - Issue: The comment above `nextReconnectDelay` explicitly says "Exported for unit tests," and the file has a `module.exports` branch specifically to support that (`typeof module !== 'undefined' && module.exports`). No test file (`helper.test.ts` or similar) exercises this export anywhere in the repo (checked `packages/cli/src` for any reference to `nextReconnectDelay` — none found beyond `helper.js` itself). The function is a small pure backoff-doubling calculation, so the risk of an undetected regression is low, and SC#4 (client status pill/paused overlay) is explicitly scoped as manual-verification-only per `plan.md` p01-t02 Step 2 — this is consistent with that decision, just noting the test seam the code deliberately built is currently unused. + - Fix: Optional — add a small `helper.test.ts` asserting `nextReconnectDelay(500, 30000)` doubles and caps at `MAX_RECONNECT_MS`. Low priority; does not block this project's closeout since it's a pure, low-risk function and outside the declared automated-smoke scope for SC#4. + +## Requirements/Design Alignment + +**Evidence sources used:** `discovery.md` (Success Criteria — quick-mode requirement source), `plan.md` (task specs and Reviews table), `implementation.md` (Deviations, per-phase summaries), all three prior phase review artifacts (`p01-review-2026-07-02.md`, `p02-review-2026-07-02.md`, `p03-review-2026-07-02.md` + `-v2.md`), direct reads of all 7 code/doc files under `.agents/skills/oat-brainstorm/`, direct read of `packages/cli/src/integration/visual-companion-smoke.test.ts`, direct read of `NOTICES.md`/`package.json`×5/`public-package-versions.json`, live server exercise (start-server.sh → curl auth/headers/traversal/dotfile → stop-server.sh, full cycle reproduced and cleaned up), and direct execution of both required verification commands plus `pnpm oat:validate-skills`. + +### Success Criteria Coverage (discovery.md SC#1–SC#7) + +| Success Criterion | Status | Notes | +| ---------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| SC#1 — session-key auth (HTTP+WS) + `/files/` sandbox (traversal/symlink/dotfile) + security headers | **implemented** | Re-verified live: unauth `GET /` → `403`; authed `GET /?key=<key>` → `200` with `Cache-Control: no-store`, `X-Frame-Options: DENY` (plus additive `Referrer-Policy`, CSP `frame-ancestors 'none'`, `Cross-Origin-Resource-Policy`); `GET /files/../server.cjs?key=<key>` and `GET /files/.bashrc?key=<key>` both → `404`. Smoke suite independently covers traversal/dotfile/symlink with real leak-check fixtures (not vacuous — empirically red/green-proven per p03 re-review) and WS auth via `isAllowedWebSocketOrigin`/`isAuthorized`. WS Origin check narrows to plaintext-HTTP only (carried Minor, accept-defer — see Findings). | +| SC#2 — restart reuses port/key | **implemented** | `server.cjs` `preferredPort()`/`initialToken()` read `.last-port`/`.last-token` under the OAT-resolved root; only written back on non-fallback bind (avoids clobbering a live sibling session). Smoke test `reuses the recorded port on restart...` asserts both port reuse and, since the p03 fix, session-key reuse via `keyFromServerUrl` equality — genuinely closes the SC#2 coverage gap flagged in the original p03 review. | +| SC#3 — idle 4h configurable + stop-server instance guard | **implemented** | `IDLE_TIMEOUT_MS` defaults to `4*60*60*1000`, overridable via `BRAINSTORM_IDLE_TIMEOUT_MS` (`start-server.sh` converts `--idle-timeout-minutes` → ms). Live-verified `--idle-timeout-minutes 1` → `idle_timeout_ms:60000` in this review's spot-check. `stop-server.sh`'s `is_brainstorm_server()` requires a live PID **and** a matching `--brainstorm-server-id` argv before signaling; smoke test tampers the recorded instance id and asserts `{"status":"stale_pid"}` while the real process survives — matches fail-closed design. | +| SC#4 — client status pill/paused overlay (manual-verified) | **implemented (code-verified; runtime behavior manual per plan)** | `helper.js` implements `setStatus()` (connecting/connected/reconnecting/disconnected), `nextReconnectDelay()` exponential backoff (500ms→30s cap), `showTombstone()` after `TOMBSTONE_AFTER_MS` (15s), and `reloadAfterRecovery()`; `frame-template.html` has the matching `.status` element and CSS. `plan.md` p01-t02 explicitly scopes actual browser-rendered reconnect/status-pill/tombstone behavior as manual verification, outside automated smoke — consistent with the phase-1 review's disposition. This final review re-confirmed the code is present and internally consistent but did not re-run a live browser session (same manual-scope boundary the plan set). | +| SC#5 — SKILL.md + reference docs | **implemented** | `SKILL.md` step 3 accept branch documents `--open`, keyed-URL preservation, restart semantics, 4h idle note; frontmatter `version: 1.1.0 → 1.2.0` bumped (sole owner per plan). `references/visual-companion.md` documents session-key/cookie auth, `403` on missing key, `--open`, restart/port/key reuse, 4h idle default + override, reconnect/paused-overlay behavior, and the pre-flight alive check. One doc-precision Minor carried forward (WS auth cookie-only wording) — accept-defer, does not misstate operational behavior. | +| SC#6 — smoke tests + `release:validate` pass | **implemented** | Re-ran both in this review: `vitest run src/integration/visual-companion-smoke.test.ts` → **10/10 passed**; `pnpm release:validate` → **passed** for all 5 public packages at `0.1.34`. `pnpm oat:validate-skills` also re-run → `OK: validated 53 oat-* skills`. | +| SC#7 — NOTICES v6.0.3 | **implemented** | `NOTICES.md` "brainstorming skill — visual companion" section correctly references v6.0.3, describes a per-file adapted port (not byte-for-byte), and lists all 5 adapted files plus `references/visual-companion.md`; the separate `brainstorming` skill section correctly retains its own 5.0.7 reference for the unrelated prose it documents. No longer claims byte-for-byte v5.0.7 parity for the visual-companion files. | + +**All 7 Success Criteria: implemented.** No partial or missing criteria found in this final pass. + +### Deferred Minor Findings Ledger — Dispositions + +| ID | Finding | Disposition | Rationale | +| ------ | -------------------------------------------------------------------- | ---------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| p01-m1 | WebSocket Origin check + client `ws://` assume plaintext HTTP | **accept-defer** | Fails closed; not a stated SC; only affects an unsupported HTTPS-tunnel deployment mode with no evidence of current use. Fix is a small, well-scoped follow-up if that mode is ever requested. | +| p01-m2 | Vestigial dead "kill any existing server" block in `start-server.sh` | **accept-defer** | Confirmed genuinely unreachable by construction (not just empirically); zero functional/security impact; pre-existing before this project's rewrite. Cosmetic cleanup only, fine to defer to a future touch of this file. | +| p02-m1 | WS-auth doc wording implies cookie-only reliance | **accept-defer** | Doc-precision nit only; does not misstate security behavior or mislead an agent's operational handling of the URL. Low priority for a future doc pass. | + +None of the three carried-forward Minors block merge, and none has regressed or worsened since its originating phase review — all are confirmed present exactly as previously described, at the same locations, with the same (already-assessed) low impact. + +### Extra Work (not in declared requirements) + +- `BRAINSTORM_LIFECYCLE_CHECK_MS` test-speed seam (`server.cjs`) — already flagged and accepted as additive/non-scope-creep in the p01 review; re-confirmed harmless. +- Additive security headers beyond the two named in `plan.md` (`Referrer-Policy`, CSP `frame-ancestors 'none'`, `Cross-Origin-Resource-Policy`) — already flagged and accepted as additive/harmless in the p01 review. +- OAT indicator-bar UI retained alongside the v6 status pill (deliberate deviation, documented in `implementation.md` § Deviations and in `NOTICES.md`) — traced to discovery.md Key Decision #3 ("do not regress" existing behavior); correctly disclosed, not scope creep. + +No other unrequested functionality found in the full diff (`git diff --stat 5d9bf2d9..d5b44d86` excluding `.oat/projects/**` bookkeeping returns exactly the 16 files declared in scope — no undeclared provider-view drift, no `tool-packs.md` change despite p04 being available to make one, matching the documented intentional no-op skip). + +## Cross-Cutting Observations (holistic pass beyond phase-scoped reviews) + +- **Diff-surface integrity:** Confirmed the full commit-range diff (`5d9bf2d9..d5b44d86`, excluding `.oat/projects/**`) touches exactly the 16 files enumerated in this review's scope block — no more, no less. Provider-linked views (`.claude/`, `.cursor/`, etc.) show zero diff in-range, consistent with `implementation.md`'s "No changes required" sync-run note. +- **Version consistency:** All 5 public packages and `public-package-versions.json` agree on `0.1.34`; `public-package-versions.json` intentionally omits `control-plane` — confirmed this is a pre-existing convention (unchanged from the pre-project baseline `5d9bf2d9`), not a gap introduced by this project. +- **Live reproduction:** Independently reproduced (outside the phase reviews) a full start → auth-check → traversal/dotfile-check → stop cycle against the shipped `start-server.sh`/`server.cjs`/`stop-server.sh`, with identical results to what the phase reviews reported. No drift between what was reviewed per-phase and what is on `d5b44d86` today. +- **No stray files:** `git status --porcelain` was clean before and after this review's spot-check and command runs; no experiment artifacts were left behind. + +## Verification Commands + +```bash +pnpm --filter @open-agent-toolkit/cli exec vitest run src/integration/visual-companion-smoke.test.ts +pnpm release:validate +pnpm oat:validate-skills +node --check .agents/skills/oat-brainstorm/scripts/server.cjs +bash -n .agents/skills/oat-brainstorm/scripts/start-server.sh +bash -n .agents/skills/oat-brainstorm/scripts/stop-server.sh +``` + +Manual end-to-end spot-check (reproduced in this review; already reverted/cleaned up): + +```bash +bash .agents/skills/oat-brainstorm/scripts/start-server.sh --project-dir <tmp-dir> --idle-timeout-minutes 1 +curl -s -o /dev/null -w "%{http_code}\n" "http://127.0.0.1:<port>/" # expect 403 +curl -s -D - -o /dev/null "http://127.0.0.1:<port>/?key=<key>" # expect 200 + security headers +curl -s -o /dev/null -w "%{http_code}\n" --path-as-is "http://127.0.0.1:<port>/files/../server.cjs?key=<key>" # expect 404 +curl -s -o /dev/null -w "%{http_code}\n" "http://127.0.0.1:<port>/files/.bashrc?key=<key>" # expect 404 +bash .agents/skills/oat-brainstorm/scripts/stop-server.sh <session_dir> +``` + +## Verdict + +**VERDICT: pass** (0 Critical, 0 Important, 4 Minor — all either previously-assessed carried-forward items disposed as accept-defer, or a new low-priority optional test-coverage note) + +## Recommended Next Step + +Run the `oat-project-review-receive` skill to close out this project. No plan tasks are required for the deferred Minors (all dispositioned accept-defer above); optionally log the WebSocket HTTPS-tunnel gap (p01-m1) and the missing `nextReconnectDelay` unit test as standalone backlog notes if either becomes relevant later. diff --git a/.oat/projects/shared/brainstorm-visual-companion-v6/reviews/p01-review-2026-07-02.md b/.oat/projects/shared/brainstorm-visual-companion-v6/reviews/p01-review-2026-07-02.md new file mode 100644 index 0000000000..92f0e3dd10 --- /dev/null +++ b/.oat/projects/shared/brainstorm-visual-companion-v6/reviews/p01-review-2026-07-02.md @@ -0,0 +1,98 @@ +--- +oat_generated: true +oat_generated_at: 2026-07-02 +oat_review_scope: p01 +oat_review_type: code +oat_review_invocation: auto +oat_project: .oat/projects/shared/brainstorm-visual-companion-v6 +--- + +# Code Review: p01 + +**Reviewed:** 2026-07-02 +**Scope:** Phase 1 (p01-t01..p01-t04) — port of Superpowers v6.0.3 visual-companion security/resilience bundle into `.agents/skills/oat-brainstorm/scripts/` +**Files reviewed:** 5 (`server.cjs`, `helper.js`, `frame-template.html`, `start-server.sh`, `stop-server.sh`) +**Commits:** `cff2146f..ef7b1b9f` (4 commits: e7500d71, a06beecc, 5ed75687, ef7b1b9f) + +## Summary + +The v6.0.3 port is implemented correctly and matches the headline security/resilience requirements. I statically reviewed each file against the pre-port baseline, then empirically verified the live server: session-key auth is enforced on both HTTP and the WebSocket upgrade path (verified 403/socket-destroy for missing/wrong key, 200/101 for correct key or cookie), `/files/` correctly rejects traversal (`../`), symlink escapes, dotfiles, and unauthenticated access, all responses carry the required security headers, the idle-timeout minutes→ms unit conversion is verified end-to-end (`--idle-timeout-minutes 1` → `idle_timeout_ms:60000`; default → `14400000` = 4h), and `stop-server.sh`'s instance-ID guard correctly refuses to signal a PID it cannot verify (confirmed a tampered PID file yields `stale_pid` without touching the unrelated live process) while still cleanly stopping a genuine, matching server. No Critical or Important findings. Two Minor observations are noted below (a narrow HTTPS-tunnel WS gap and pre-existing dead code carried through the rewrite), neither of which regresses baseline behavior or blocks this phase. + +## Findings + +### Critical + +None. + +### Important + +None. + +### Minor + +- **WebSocket auth hardcodes plaintext-HTTP scheme, narrowing an anticipated deployment mode** (`.agents/skills/oat-brainstorm/scripts/server.cjs:311-317`, `.agents/skills/oat-brainstorm/scripts/helper.js:32-35`) + - Issue: `isAllowedWebSocketOrigin()` only accepts `origin === 'http://' + host`, and the client's `websocketUrl()` hardcodes `'ws://' + window.location.host`. The `server.cjs` comment directly above the key-auth rationale explicitly calls out supporting "loopback, tunnel, and remote binds" as a reason the key (not an Origin allowlist) is the primary gate — but if the companion is fronted by an HTTPS-terminating tunnel/reverse proxy, the browser's real Origin would be `https://...` and browsers block `wss:`-required mixed content from an `https:` page trying `ws://`, so the WS leg would fail even though the session key is valid. This fails closed (no auth bypass), and HTTPS-tunnel support is not a stated success criterion in `discovery.md`, so this is not a regression against any declared requirement — just a latent gap relative to the code's own stated design intent. + - Fix: If tunnel/remote-bind support is intended to extend to the WebSocket leg in a later phase, derive the scheme from `window.location.protocol` (`wss:` when `https:`) in `helper.js`, and accept both `http://`+host and `https://`+host origins (or drop the scheme from the comparison) in `isAllowedWebSocketOrigin()`. No action needed for p01 sign-off. + +- **Vestigial dead-code block carried through the rewrite** (`.agents/skills/oat-brainstorm/scripts/start-server.sh:185-190`) + - Issue: The "Kill any existing server" block checks `[[ -f "$PID_FILE" ]]` where `PID_FILE="${SESSION_DIR}/state/server.pid"` and `SESSION_DIR` is always a freshly-generated, never-before-seen directory (`SESSION_ID="$$-$(date +%s)"`, `mkdir -p "${SESSION_DIR}/content" "$STATE_DIR"` runs immediately before). This condition can never be true, so the block is unreachable. Confirmed via diff against the pre-port baseline (`cff2146f`) that this exact dead block already existed before this task — it is not a new regression — but p01-t03 substantially rewrote this file around it and left it in place. (Actual restart/port-reuse safety is correctly handled elsewhere, via `server.cjs`'s `EADDRINUSE` fallback logic that avoids clobbering a still-live prior session's persisted port/token — verified by reading; this dead block has no bearing on that correctness.) + - Suggestion: Remove the block (or replace the comment) in a future pass touching this file, to avoid misleading future readers into thinking it does something. + +## Requirements/Design Alignment + +**Evidence sources used:** `discovery.md` (requirements source, quick-mode), `plan.md` Phase 1 task specs (p01-t01..p01-t04), `implementation.md` (Deviations section), pre-port baseline at `cff2146f` for all 5 files (diffed to confirm no unintended regressions and to distinguish pre-existing conditions from new ones). + +### Requirements Coverage (discovery.md Success Criteria, Phase-1-relevant subset) + +| Requirement | Status | Notes | +| --------------------------------------------------------------------------------- | ----------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | +| Session key required on HTTP and WebSocket | implemented | Verified live: unauthenticated/`403`+destroyed-socket on missing or wrong key (HTTP `GET /`, `/files/*`, and WS upgrade); `200`/`101` on correct `?key=` or cookie. | +| `/files/` rejects traversal/symlinks/dotfiles | implemented | Verified live: `../secret-outside.txt` (with and without `--path-as-is`), a symlink pointing outside `CONTENT_DIR`, and a dotfile all return `404`; a legitimate in-dir file returns `200`. | +| Security headers present | implemented | Verified live on `200`/`403`/`404` responses: `Cache-Control: no-store`, `X-Frame-Options: DENY`, plus additive `Referrer-Policy`, CSP `frame-ancestors 'none'`, and `Cross-Origin-Resource-Policy` (all correct/harmless per known context). | +| Idle default 4h, configurable via `--idle-timeout-minutes` | implemented | Verified live: default `idle_timeout_ms:14400000`; `--idle-timeout-minutes 1` → `idle_timeout_ms:60000`. Units agree between `start-server.sh` (minutes→ms conversion) and `server.cjs` (reads ms directly). | +| Restart with same `--project-dir` reuses port/key | implemented | `BRAINSTORM_PORT_FILE`/`BRAINSTORM_TOKEN_FILE` persisted at `${BRAINSTORM_ROOT}/.last-port` / `.last-token` (sibling to session dirs, same OAT-resolved root); `server.cjs` reads them via `preferredPort()`/`initialToken()` and only writes them back when it got its preferred port (avoids clobbering a still-live prior session on `EADDRINUSE` fallback). | +| OAT 3-tier persistence (`--project-dir` → repo walk-up → `~/.oat/brainstorm/`) | implemented | Code-read confirms all three tiers in `start-server.sh`; existing smoke tests (all 5, unmodified in this phase) still pass, covering all three paths end-to-end. No `.superpowers/` references anywhere in the 5 files (grep-confirmed; the one hit is a comment explicitly disclaiming it). | +| `stop-server.sh` verifies instance ID before kill | implemented | Verified live: a `server.pid` tampered to point at an unrelated live process yields `{"status":"stale_pid"}` and does **not** signal that process; a genuine, untouched session (matching PID + `server-instance-id` in the process's own `/proc`/`ps` cmdline) is cleanly stopped (`{"status":"stopped"}`, process confirmed dead, `server-stopped` marker written, URL unreachable afterward). | +| WS max-frame-size guard | implemented | `MAX_FRAME_PAYLOAD_BYTES` (10MB) checked against the 64-bit extended length as a `BigInt` comparison before any `Number()` conversion (no precision-loss window), and buffer growth for a single frame is bounded by this check before `decodeFrame` waits for the rest of the payload. | +| No regression to OAT JSON log events (`server-started`/`server-stopped`, etc.) | implemented | Diffed against baseline `cff2146f`: all previously-emitted `type` values (`server-started`, `server-stopped`, `screen-added`, `screen-updated`, `owner-pid-invalid`, `user-event`/`source`) are unchanged; `server-started` gained an additive `idle_timeout_ms` field and the `url` now carries `?key=` (both intentional per discovery.md's stated assumption that agents must adopt the keyed URL). | +| Client resilience: status pill, backoff reconnect, paused overlay, keyed reload | implemented | `helper.js` adds `setStatus()`, `nextReconnectDelay()` (exported for unit tests), `showTombstone()` after `TOMBSTONE_AFTER_MS`, and `reloadAfterRecovery()`. Per plan p01-t02 Step 2, the actual browser-rendered reconnect/status-pill/tombstone behavior is out of automated smoke scope for this phase (manual verification) — not re-flagged here. | +| Frame rebrand to "OAT Brainstorm"; telemetry/branding stripped | implemented | `<title>`/`<h1>` now read "OAT Brainstorm"; the old Superpowers GitHub link in the header is removed. | +| Deviation: retain OAT `.indicator-bar`/`#indicator-text` alongside v6 status pill | implemented, documented | Confirmed against both the v6-style new code and the pre-port baseline: the indicator-bar markup/CSS and its click-handler block are the same code that existed pre-port, correctly merged alongside the new `setStatus`/backoff/tombstone logic. `#claude-content` id retained (not renamed to v6's `#frame-content`), matching `implementation.md`'s Deviations entry exactly. | + +### Extra Work (not in declared requirements) + +- `BRAINSTORM_LIFECYCLE_CHECK_MS` env var (`server.cjs`) — a new, minimal test-speed seam for the lifecycle watchdog interval (defaults to 60s in production). Small, additive, and directly useful for Phase 3's forthcoming smoke tests; not scope creep. +- Everything else beyond the two headers explicitly named in `plan.md` (extra security headers, `Referrer-Policy`/CSP/CORP) was already flagged as expected/acceptable in the review dispatch's known-context and is additive/harmless — not re-litigated here. + +No other unrequested functionality found. + +## Verification Commands + +```bash +node --check .agents/skills/oat-brainstorm/scripts/server.cjs +bash -n .agents/skills/oat-brainstorm/scripts/start-server.sh +bash -n .agents/skills/oat-brainstorm/scripts/stop-server.sh +pnpm --filter @open-agent-toolkit/cli exec vitest run src/integration/visual-companion-smoke.test.ts +``` + +Manual end-to-end auth/sandbox/idle-timeout/instance-guard verification (already run during this review; re-run to reproduce): + +```bash +# Start a server and capture server-started JSON (includes ?key=<token> and idle_timeout_ms) +bash .agents/skills/oat-brainstorm/scripts/start-server.sh --project-dir /tmp/oat-brainstorm-manual-check --idle-timeout-minutes 1 + +# Auth checks (expect 403 without key, 200 with correct key) +curl -s -o /dev/null -w "%{http_code}\n" "http://127.0.0.1:<port>/" +curl -s -o /dev/null -w "%{http_code}\n" "http://127.0.0.1:<port>/?key=<token>" + +# /files/ traversal/symlink/dotfile checks (expect 404), using a cookie jar from an authorized request +curl -s -c /tmp/cookies.txt -o /dev/null "http://127.0.0.1:<port>/?key=<token>" +curl -s -b /tmp/cookies.txt -o /dev/null -w "%{http_code}\n" --path-as-is "http://127.0.0.1:<port>/files/../server.cjs" + +# Stop and confirm the process actually terminates +bash .agents/skills/oat-brainstorm/scripts/stop-server.sh <session_dir> +``` + +## Recommended Next Step + +Run the `oat-project-review-receive` skill to convert findings into plan tasks. diff --git a/.oat/projects/shared/brainstorm-visual-companion-v6/reviews/p02-review-2026-07-02.md b/.oat/projects/shared/brainstorm-visual-companion-v6/reviews/p02-review-2026-07-02.md new file mode 100644 index 0000000000..a803207c28 --- /dev/null +++ b/.oat/projects/shared/brainstorm-visual-companion-v6/reviews/p02-review-2026-07-02.md @@ -0,0 +1,68 @@ +--- +oat_generated: true +oat_generated_at: 2026-07-02 +oat_review_scope: p02 +oat_review_type: code +oat_review_invocation: auto +oat_project: .oat/projects/shared/brainstorm-visual-companion-v6 +--- + +# Code Review: p02 + +**Reviewed:** 2026-07-02 +**Scope:** Phase 2 (p02-t01, p02-t02) — documentation-only update of `visual-companion.md` and `SKILL.md` step 3 to reflect the v6 visual-companion port (server auth, restart/reuse, idle timeout, client reconnect) delivered in Phase 1. +**Files reviewed:** 2 +**Commits:** `a0275f8e..60740f71` (2 commits: `5ea69291` docs(p02-t01), `60740f71` docs(p02-t02)) + +## Summary + +Both doc updates accurately describe the shipped v6 server/client behavior — 403 auth failure, keyed `?key=` URL, 4-hour idle default, `--open` semantics, and restart port/key reuse all check out against `server.cjs`, `start-server.sh`, and `helper.js`. `SKILL.md`'s Activation Contract and destination-dispatch logic are untouched (diff confirmed to two isolated hunks: the version bump and the Step 3 accept-prose addition), and the frontmatter `version:` was correctly bumped `1.1.0 → 1.2.0`. `pnpm oat:validate-skills` passes. One Minor wording nit noted below; no Critical or Important findings. + +## Findings + +### Critical + +None + +### Important + +None + +### Minor + +- **WS-authentication phrasing implies cookie-only auth, but the client also resends the key** (`.agents/skills/oat-brainstorm/references/visual-companion.md:49`) + - Issue: The doc states subsequent same-origin requests, "including the WebSocket upgrade," authenticate "via the cookie without repeating the key in the address bar." In the shipped client (`.agents/skills/oat-brainstorm/scripts/helper.js:32-35`), `websocketUrl()` still appends `?key=<sessionKey>` to the `ws://` URL whenever a session key is present in `sessionStorage` — the WS handshake carries the key redundantly alongside the cookie, it does not rely on the cookie exclusively. The phrase "without repeating the key" is only true of the visible address bar (which never shows the WS connection), not of the WS request itself, so a future doc maintainer skimming this sentence could conclude the key is dropped from the WS URL and be surprised by the actual `server.cjs` `isAuthorized()` check (`.agents/skills/oat-brainstorm/scripts/server.cjs:275-287`), which accepts either the query param or the cookie. + - Suggestion: Tighten the sentence to something like: "…so the address bar never needs to show the key again; the WebSocket connection re-sends the key as `?key=` from the bootstrapped `sessionStorage` value (falling back to the cookie for plain page/`/files/*` requests)." Low priority — does not affect correctness of the shipped behavior or mislead an agent following the doc operationally, only a documentation-precision nit for future maintainers. + +## Requirements/Design Alignment + +**Evidence sources used:** `discovery.md` (Success Criteria, Key Decisions), `plan.md` (Phase 2 task specs for p02-t01/p02-t02), and the shipped Phase 1 code (`.agents/skills/oat-brainstorm/scripts/server.cjs`, `start-server.sh`, `helper.js`) as the ground truth the docs must not contradict. No `spec.md`/`design.md` — quick-mode project, not applicable. + +### Requirements Coverage + +| Requirement | Status | Notes | +| ----------------------------------------------------------------------------------- | ----------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| SC#5 — docs must state session key + URL semantics | implemented | `visual-companion.md` "Session Authentication and the Companion URL" section documents `?key=` bootstrap, cookie mirroring, and 403 on missing/invalid key — matches `server.cjs:275-287,321-326`. | +| SC#5 — docs must state `--open` flag | implemented | Both `SKILL.md` (step 3 accept branch) and `visual-companion.md` "Starting a Session" section document `--open` and the "only after user approval" gating — matches `start-server.sh:76-79` and `server.cjs:463-482` (`BRAINSTORM_OPEN` opt-in gate). | +| SC#5 — docs must state restart/port/key reuse | implemented | `visual-companion.md` "Session Authentication…" + "Connection Status and Reconnection" sections and `SKILL.md`'s new "Restart semantics" paragraph both describe same-`--project-dir`/repo-context reuse — matches `.last-port`/`.last-token` persistence in `start-server.sh:165-170` and `server.cjs:596-614`. | +| SC#5 — docs must state idle default | implemented | `visual-companion.md` "Idle timeout" bullet states 4-hour default, `--idle-timeout-minutes` override — matches `server.cjs:486-491` (`4 * 60 * 60 * 1000` default) and `start-server.sh:103-112` (minutes→ms conversion). | +| Discovery Key Decision #3 — do not regress Activation Contract/destination dispatch | implemented | Diff isolated to two hunks (version bump + Step 3 accept prose); `## Activation Contract` and destination-dispatch sections (grep-verified, lines ~14-16, ~269-392) are byte-identical to pre-change. | +| Repo guardrail — canonical skill `version:` bump on `SKILL.md` change | implemented | `version: 1.1.0 → 1.2.0` confirmed in the diff and in the current file (`SKILL.md:3`). | +| Plan verification step — `pnpm oat:validate-skills` passes | implemented | Ran locally: `OK: validated 53 oat-* skills`. | + +### Extra Work (not in declared requirements) + +None — diff is exactly the two files/two tasks declared in scope (`git diff --name-only a0275f8e..60740f71` returns only `SKILL.md` and `references/visual-companion.md`). + +## Verification Commands + +```bash +git diff a0275f8e..60740f71 -- .agents/skills/oat-brainstorm/SKILL.md .agents/skills/oat-brainstorm/references/visual-companion.md +pnpm oat:validate-skills +node --check .agents/skills/oat-brainstorm/scripts/server.cjs +grep -n "^version:" .agents/skills/oat-brainstorm/SKILL.md +``` + +## Recommended Next Step + +Run the `oat-project-review-receive` skill to convert findings into plan tasks. diff --git a/.oat/projects/shared/brainstorm-visual-companion-v6/reviews/p03-review-2026-07-02-v2.md b/.oat/projects/shared/brainstorm-visual-companion-v6/reviews/p03-review-2026-07-02-v2.md new file mode 100644 index 0000000000..81e18b808d --- /dev/null +++ b/.oat/projects/shared/brainstorm-visual-companion-v6/reviews/p03-review-2026-07-02-v2.md @@ -0,0 +1,100 @@ +--- +oat_generated: true +oat_generated_at: 2026-07-02 +oat_review_scope: p03 +oat_review_type: code +oat_review_invocation: auto +oat_project: .oat/projects/shared/brainstorm-visual-companion-v6 +--- + +# Code Review: p03 (re-review of fix commit `1f1e623f`) + +**Reviewed:** 2026-07-02 +**Scope:** Re-review of the fix commit addressing the two findings in the prior `p03-review-2026-07-02.md` (Important: vacuous traversal/dotfile test; Minor: restart test didn't verify key reuse). Scope limited to `p03-t01`/`p03-t02`. +**Files reviewed:** 1 (`packages/cli/src/integration/visual-companion-smoke.test.ts`) +**Commits:** `5d168445..1f1e623f`, fix commit under review: `1f1e623f` + +## Summary + +Both prior findings are genuinely resolved. The traversal/dotfile test now uses real fixtures (a marker one level above `CONTENT_DIR` and a real `.hidden.html` inside it) and asserts both 4xx status and that the response body does not leak the marker content; empirical red-teaming confirms the test now fails when the server's `/files/` sandboxing is weakened, and passes cleanly against the real server. The restart test now also asserts session-key reuse via `keyFromServerUrl`. The fix commit touches only the test file, all 10 suite tests pass, and `pnpm release:validate` still passes. + +## Findings + +### Critical + +None. + +### Important + +None. Prior Important finding is resolved (see verification below). + +### Minor + +None. Prior Minor finding is resolved (see verification below). + +## Verification of Prior Findings + +### 1. Important — traversal/dotfile test was vacuous → RESOLVED + +The fix (`packages/cli/src/integration/visual-companion-smoke.test.ts:429-454`) now: + +- Writes a real marker file one level above `CONTENT_DIR` (`join(sessionDir, 'sentinel.html')`, where `sessionDir` is `CONTENT_DIR`'s parent — matches `server.cjs`'s `CONTENT_DIR = path.join(SESSION_DIR, 'content')`) before issuing `GET /files/../sentinel.html?key=<key>`, and asserts both `status >= 400 && < 500` **and** `body` does not contain `'traversal-should-be-blocked'`. +- Writes a real dotfile inside `info.screen_dir` (`.hidden.html`) before issuing `GET /files/.hidden.html?key=<key>`, and asserts the same 4xx + non-leak pattern. + +**Empirical red-team performed (two rounds), both fully reverted:** + +- Round 1 (as specified in the review scope — remove only `path.basename()` normalization and the `fileName.startsWith('.')` dotfile guard from `server.cjs`'s `/files/` handler, keeping `isRegularFileInsideContentDir`'s realpath-containment check intact): reran the suite. Result: **RED** — the test failed with `AssertionError: expected 200 to be greater than or equal to 400` at line 452 (the dotfile assertion). The traversal assertion (line 444) did not fail in this specific configuration because `isRegularFileInsideContentDir`'s independent realpath-containment check still blocked the traversal payload — this is expected defense-in-depth, not a test defect. +- Round 2 (additional check, to isolate the traversal assertion specifically — fully removed the `/files/` sandbox chain: no basename normalization, no dotfile guard, no containment check, replaced with a raw `fs.existsSync`/`isDirectory` check): reran the suite. Result: **RED** — the test failed with `AssertionError: expected 200 to be greater than or equal to 400` at line 444 (the traversal assertion, failing first as expected since it runs before the dotfile assertion). +- Restore: `cp` original `server.cjs` back after each round; `git status`/`git diff` confirmed clean (no residual `server.cjs` change) after each restore. +- Baseline (real, unmodified `server.cjs`): full suite reran, **GREEN**, 10/10 passing. + +This confirms the test now genuinely depends on the server's `/files/` traversal and dotfile sandboxing — it fails when that protection is weakened or removed, and passes against the real, hardened implementation. Finding is resolved. + +### 2. Minor — restart test didn't verify key reuse → RESOLVED + +`packages/cli/src/integration/visual-companion-smoke.test.ts:532-538` now adds: + +```ts +expect(keyFromServerUrl(second.info.url)).toBe( + keyFromServerUrl(first.info.url), +); +``` + +to the restart test, immediately after the existing `.last-port` reuse assertions. `keyFromServerUrl` (`:217-225`) parses the `key` query param and throws loudly if absent, so the equality check cannot silently pass on two empty/undefined values. This closes the SC#2 coverage gap for session-key persistence across restart (backed by `server.cjs`'s `BRAINSTORM_TOKEN_FILE`/`initialToken()` persistence, confirmed present at `server.cjs:113-140,608-611`). Finding is resolved. + +## Regression Guard + +- Full suite: `pnpm --filter @open-agent-toolkit/cli exec vitest run src/integration/visual-companion-smoke.test.ts` → **10/10 passed** (0 failed, 0 skipped). +- `pnpm release:validate` → **passed** ("release validation passed for 5 public packages", all at 0.1.34). +- Fix commit `1f1e623f` diff scope: `git show --stat 1f1e623f` shows exactly one file changed — `packages/cli/src/integration/visual-companion-smoke.test.ts` (29 insertions(+), 3 deletions(-)). `server.cjs` and all other files (NOTICES.md, package.json versions, manifest.json) are untouched by this fix commit; those were already reviewed and passed in the prior `p03-review-2026-07-02.md` pass and are unaffected by this fix. +- No new findings introduced by the fix; no scope creep. + +## Requirements/Design Alignment + +**Evidence sources used:** prior review artifact (`reviews/p03-review-2026-07-02.md`), `discovery.md` SC#1/SC#2, direct reads of `.agents/skills/oat-brainstorm/scripts/server.cjs`, direct test execution including two rounds of empirical red-teaming, `pnpm release:validate`. + +### Requirements Coverage (delta from prior review only) + +| Requirement (discovery.md) | Status | Notes | +| ------------------------------------------ | ----------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| SC#1 — `/files/` rejects traversal/dotfile | implemented | Was `partial` in prior review; now real fixtures + non-leak assertions, empirically confirmed to fail when the guard is removed (two red-team rounds, both reverted cleanly). | +| SC#2 — restart reuses key | implemented | Was `partial` in prior review; now asserted directly via `keyFromServerUrl` equality. | + +All other requirements coverage from the prior review (SC#1 auth/headers, SC#1 symlink escape, SC#2 port reuse, SC#3 stale-PID guard, SC#6 suite/release-validate pass, SC#7 NOTICES provenance, lockstep version bump) is unchanged and was not re-litigated per re-review scope. + +### Extra Work (not in declared requirements) + +None identified — the fix commit's changes map directly to closing the two prior findings. + +## Verification Commands + +```bash +pnpm --filter @open-agent-toolkit/cli exec vitest run src/integration/visual-companion-smoke.test.ts +pnpm release:validate +git show --stat 1f1e623f # confirm only the test file changed +git diff --stat # confirm no residual server.cjs changes from red-team experiments +``` + +## Recommended Next Step + +Findings resolved — no further action required for p03-t01/p03-t02. If desired, run the `oat-project-review-receive` skill to close out any tracking tasks tied to the prior review's findings. diff --git a/.oat/projects/shared/brainstorm-visual-companion-v6/reviews/p03-review-2026-07-02.md b/.oat/projects/shared/brainstorm-visual-companion-v6/reviews/p03-review-2026-07-02.md new file mode 100644 index 0000000000..939f762835 --- /dev/null +++ b/.oat/projects/shared/brainstorm-visual-companion-v6/reviews/p03-review-2026-07-02.md @@ -0,0 +1,79 @@ +--- +oat_generated: true +oat_generated_at: 2026-07-02 +oat_review_scope: p03 +oat_review_type: code +oat_review_invocation: auto +oat_project: .oat/projects/shared/brainstorm-visual-companion-v6 +--- + +# Code Review: p03 + +**Reviewed:** 2026-07-02 +**Scope:** Phase 3 — Tests and release validation (`p03-t01` extend visual-companion smoke tests, `p03-t02` NOTICES/lockstep version bump/release validate) +**Files reviewed:** 9 (1 test file, `NOTICES.md`, 5 `package.json`, `public-package-versions.json`, `.oat/sync/manifest.json`) +**Commits:** `5d168445..4f993aa8` (`e1ea29bd`, `4f993aa8`) + +## Summary + +The five new v6-hardening smoke tests are largely real, HTTP-exercising, non-mocked assertions — the auth/headers, symlink-escape, restart-port-reuse, and stale-PID-guard tests all genuinely exercise `server.cjs`/`stop-server.sh` and would fail if the underlying control were removed (verified empirically below). The one exception is the "path traversal and dotfile access" test: it is constructed so that both its assertions pass even with the `path.basename()`/dotfile guard fully removed from `server.cjs`, because neither payload resolves to a file that exists in the test fixture either way. Release mechanics (lockstep version bump to 0.1.34, `public-package-versions.json`, `pnpm release:validate`, `NOTICES.md` provenance update) are all correct and verified by direct execution. + +## Findings + +### Critical + +None. + +### Important + +- **"sandboxes /files/ against path traversal and dotfile access" test does not actually verify the traversal/dotfile guard** (`packages/cli/src/integration/visual-companion-smoke.test.ts:417-442`) + - Issue: The test correctly uses `http.request` with an explicit `path` (`rawHttpGet`) to defeat client-side `..` normalization, so the _request_ reaches the server as a literal `..`/dotfile payload — that part is sound. However, neither assertion actually depends on the server's sandboxing logic in `handleRequest`'s `/files/` branch (`.agents/skills/oat-brainstorm/scripts/server.cjs:354-368`, specifically `path.basename(pathname.slice(7))` and `fileName.startsWith('.')`): + - The traversal payload `GET /files/../server.cjs?key=<key>` resolves (via `path.basename`) to `CONTENT_DIR/server.cjs`, which never exists in the test's temp content dir — so it 404s regardless of whether `..` is sanitized. + - The dotfile payload `GET /files/.hidden?key=<key>` targets `CONTENT_DIR/.hidden`, which is never created by the test — so it 404s regardless of whether the `fileName.startsWith('.')` guard exists. + - I verified this empirically: with `path.basename()` and the `fileName.startsWith('.')` check removed from `server.cjs` (replaced with a raw `fs.existsSync`/`isDirectory` check, `isRegularFileInsideContentDir`'s symlink guard also removed), the full suite was rerun — the "path traversal and dotfile access" test still passed (2589ms, green), while the separate symlink-escape test correctly failed (`expected 200 to be greater than or equal to 400`) as intended. This confirms the traversal/dotfile test does not distinguish a hardened server from a vulnerable one; only the symlink test in this file does. (`server.cjs` was restored byte-for-byte after the experiment; `git status`/`git diff` confirm no residual change.) + - Root cause note: `plan.md` p03-t01 step 1 literally specifies this weak payload (`GET /files/../server.cjs?key=<key>` "or a dotfile path"), so the implementer followed the plan as written — the gap originates in the plan's suggested payload, not implementer deviation from it. It still needs closing before this counts as real coverage of discovery.md SC#1 ("file server rejects traversal/symlinks/dotfiles"). + - Fix: Mirror the technique the symlink test (`:444-478`) already uses — target real files that traversal/dotfile access would actually reach if unprotected, and assert the response does not leak them: + - Traversal: write a marker file one level above `CONTENT_DIR` (e.g. `join(projectDir, '.oat', 'brainstorm', <session>, 'sentinel.html')` or, simpler, reuse `info.screen_dir`'s parent) and request `/files/../sentinel.html?key=<key>`; assert 4xx and `body` does not contain the marker content. + - Dotfile: `writeFileSync(join(info.screen_dir, '.hidden.html'), 'dotfile-should-be-hidden\n')` before the request, then request `/files/.hidden.html?key=<key>`; assert 4xx and `body` does not contain the marker content. + - Requirement: discovery.md SC#1 ("file server rejects traversal/symlinks/dotfiles"); plan.md p03-t01. + +### Minor + +- **Restart test only proves port reuse, not key/token reuse** (`packages/cli/src/integration/visual-companion-smoke.test.ts:491`) + - Issue: discovery.md SC#2 requires "Restart with same `--project-dir` ... reuses port/key so browser tab reconnects." The new restart test (`reuses the recorded port on restart with the same --project-dir (reads .last-port)`) only asserts `second.info.port === first.info.port` and the `.last-port` file contents; it never asserts that `second.info.url`'s `key` matches `first`'s (i.e. that `.last-token` / `BRAINSTORM_TOKEN_FILE` reuse actually keeps the same session key across restart, which is what lets an already-open browser tab's cookie keep validating). No other test in the suite covers token persistence across restart either — this is a genuine coverage gap for that half of SC#2, not just a redundant assertion. `plan.md` p03-t01 also only asked for `.last-port` reuse, so this is a plan-scope narrowing rather than an implementer miss. + - Suggestion: add `expect(keyFromServerUrl(second.info.url)).toBe(keyFromServerUrl(first.info.url))` (or equivalent `.last-token` file comparison) to the same test. + +## Requirements/Design Alignment + +**Evidence sources used:** `discovery.md`, `plan.md` (quick-mode project; no `spec.md`/`design.md`), `implementation.md`, direct reads of `.agents/skills/oat-brainstorm/scripts/{server.cjs,stop-server.sh}`, direct test execution, `pnpm release:validate`. + +### Requirements Coverage (Phase 3 scope only) + +| Requirement (discovery.md) | Status | Notes | +| -------------------------------------------------------------------------- | ----------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| SC#1 — session key required on HTTP; security headers present | implemented | `:353-393` real 403/200 + header assertions, verified passing. | +| SC#1 — `/files/` rejects traversal/dotfile | partial | Test exists but is vacuous per Important finding above; the underlying server-side guard (`server.cjs:354-368`) is correctly implemented (p01, already reviewed) — only the _test_ fails to prove it. | +| SC#1 — `/files/` rejects symlink escape | implemented | Real symlink fixture + real HTTP request; empirically confirmed to fail when the guard is removed. | +| SC#2 — restart reuses port | implemented | Real two-server-spawn test, reads `.last-port`. | +| SC#2 — restart reuses key | partial | No automated assertion; see Minor finding. | +| SC#3 — stop-server verifies instance ID before kill | implemented | Real subprocess + tampered `server-instance-id` file; asserts `status: "stale_pid"` and that the real process survives. Matches `stop-server.sh:63-71` fail-closed logic. | +| SC#6 — `pnpm test` visual-companion smoke passes | implemented | Confirmed via direct execution: 10/10 tests pass (`vitest run src/integration/visual-companion-smoke.test.ts`). | +| SC#6 — `pnpm release:validate` passes after version bump | implemented | Confirmed via direct execution: "release validation passed for 5 public packages" at 0.1.34. | +| SC#7 — NOTICES.md references v6.0.3 adapted port, not byte-for-byte v5.0.7 | implemented | `NOTICES.md` diff now describes per-file adaptation (session-key auth, sandboxing, headers, etc.) instead of "lifted...verbatim"; version references correctly split 5.0.7 (brainstorming skill) vs 6.0.3 (visual companion). | +| Lockstep version bump (AGENTS.md guardrail) | implemented | All 5 public packages at `0.1.34`; `public-package-versions.json` matches; `.oat/sync/manifest.json` only changed `oatVersion` (expected, no provider-view drift). | + +### Extra Work (not in declared requirements) + +None identified — all changed files map to p03-t01/p03-t02. + +## Verification Commands + +```bash +pnpm --filter @open-agent-toolkit/cli exec vitest run src/integration/visual-companion-smoke.test.ts +pnpm release:validate +git diff --stat 5d168445..4f993aa8 -- .oat/sync/manifest.json # confirm only oatVersion changed +``` + +## Recommended Next Step + +Run the `oat-project-review-receive` skill to convert findings into plan tasks. diff --git a/.oat/projects/shared/brainstorm-visual-companion-v6/state.md b/.oat/projects/shared/brainstorm-visual-companion-v6/state.md new file mode 100644 index 0000000000..153104ff02 --- /dev/null +++ b/.oat/projects/shared/brainstorm-visual-companion-v6/state.md @@ -0,0 +1,76 @@ +--- +oat_current_task: null +oat_last_commit: 1f1e623f +oat_blockers: [] +associated_issues: [] +oat_kind: implementation +oat_parent: null +oat_siblings: [] +oat_depends_on: [] +oat_children: [] +oat_hill_checkpoints: [] +oat_hill_completed: [] +oat_parallel_execution: false +oat_phase: implement +oat_phase_status: pr_open +oat_workflow_mode: quick +oat_workflow_origin: native +oat_docs_updated: complete +oat_pr_status: open +oat_pr_url: 'https://github.com/voxmedia/open-agent-toolkit/pull/125' +oat_project_created: '2026-06-26T19:52:43.792Z' +oat_project_completed: null +oat_project_state_updated: '2026-07-02T02:20:00.000Z' +oat_dispatch_ceiling: + preset: balanced + providers: + codex: high + claude: sonnet + source: project-state +oat_generated: false +--- + +# Project State: brainstorm-visual-companion-v6 + +**Status:** Implementation in progress +**Started:** 2026-06-26 +**Last Updated:** 2026-07-02 + +## Current Phase + +Implementation — PR open, awaiting human review. PR #125 (parity-check → main). + +## Artifacts + +- **Discovery:** `discovery.md` (complete) +- **Spec:** N/A (quick mode) +- **Design:** N/A (skipped — scope clear from release review) +- **Plan:** `plan.md` (complete, `oat_ready_for: oat-project-implement`) +- **Implementation:** `implementation.md` (initialized) + +## Progress + +- ✓ Project scaffolded (quick mode) +- ✓ Discovery captured from Superpowers v6 release review +- ✓ Plan generated (9 tasks, 4 phases, sequential) +- ✓ Plan artifact review (fixes_completed; v2 re-review folded into p03 tasks) +- ✓ Phase 1 complete (v6 bundle port) — gate review passed +- ✓ Phase 2 complete (skill + reference docs) — gate review passed +- ✓ Phase 3 complete (tests + release validation) — gate passed after 1 fix; release:validate green +- ✓ Phase 4 complete (docs touchpoint — skipped, no delta) +- ✓ All 9 implementation tasks resolved +- ✓ Final review passed (0C/0I/4m deferred; SC#1–SC#7 all met) +- ✓ Docs sync (no delta) + summary generated +- ✓ PR created (#125) +- ⧗ Awaiting human review + +## Blockers + +None + +## Next Milestone + +PR is open for review (#125). + +- To incorporate feedback: run `oat-project-revise` +- When approved: run `oat-project-complete` diff --git a/.oat/projects/shared/brainstorm-visual-companion-v6/summary.md b/.oat/projects/shared/brainstorm-visual-companion-v6/summary.md new file mode 100644 index 0000000000..6e1c505cf3 --- /dev/null +++ b/.oat/projects/shared/brainstorm-visual-companion-v6/summary.md @@ -0,0 +1,38 @@ +--- +oat_generated: true +oat_generated_at: 2026-07-02 +oat_kind: summary +oat_project: brainstorm-visual-companion-v6 +oat_workflow_mode: quick +--- + +# Project Summary: brainstorm-visual-companion-v6 + +## Overview + +Brought OAT's `oat-brainstorm` **visual companion** bundle to **Superpowers v6.0.3** security, resilience, and lifecycle parity, while preserving OAT's persistence paths (`.oat/brainstorm/`), neutral branding, and conditional-offer skill semantics. The prior bundle was a v5.0.7 port; Superpowers v6 shipped a security/resilience rewrite of the local web companion that OAT had not yet adopted. Quick-mode project: discovery + plan, 4 phases, 9 tasks. + +## What Was Implemented + +- **Security (headline):** session-key auth on HTTP **and** WebSocket (`?key=` + cookie bootstrap); sandboxed `/files/` static serving that rejects path traversal, dotfiles, **and symlink escape**; security headers (`Cache-Control: no-store`, `X-Frame-Options: DENY`, plus additive `Referrer-Policy` / CSP `frame-ancestors 'none'` / `Cross-Origin-Resource-Policy`); WebSocket max-frame guard. +- **Resilience (client):** exponential-backoff reconnect, status-pill states, paused/tombstone overlay, keyed reload after server recovery; frame rebranded "OAT Brainstorm". OAT's existing selection-summary indicator bar was retained alongside the new v6 status pill (deliberate deviation from upstream, which removed it). +- **Lifecycle:** OAT three-tier persistence (`--project-dir` → repo walk-up → `~/.oat/brainstorm/`) with `.last-port`/`.last-token`; restart with the same `--project-dir` reuses port **and** key; `--open`; configurable idle timeout (`--idle-timeout-minutes` → `BRAINSTORM_IDLE_TIMEOUT_MS`, 4h default); `umask 077`; instance-verified `stop-server` (matches `--brainstorm-server-id` before signaling, writes `server-stopped`). +- **Docs / provenance:** `references/visual-companion.md` + `SKILL.md` step 3 document the v6 behavior; SKILL.md `version` 1.1.0 → 1.2.0; `NOTICES.md` updated to v6.0.3 adapted-port provenance. +- **Release:** lockstep bump of all 5 public packages to 0.1.34; provider views synced (no drift); `pnpm release:validate` passes. + +## Verification + +- 10/10 visual-companion smoke tests (`packages/cli/src/integration/visual-companion-smoke.test.ts`) — auth/headers, `/files/` traversal + dotfile + symlink sandbox (RED/GREEN-proven load-bearing during review), restart port+key reuse, stop-server stale-instance guard. +- `pnpm release:validate` passes (5 public packages @ 0.1.34); `pnpm oat:validate-skills` OK; `pnpm type-check` + `pnpm lint` clean. +- Implementer and reviewers independently ran live end-to-end server exercises (start → curl unauth/authed/traversal/dotfile/symlink → stop). + +## Reviews + +Per-phase gate reviews + a final review, all via fresh-context subagents (Tier 1, ceiling sonnet): + +- Plan artifact review (2 cycles) — findings resolved in-plan (symlink test, provider sync). +- p01 pass · p02 pass · **p03 fail → fix → pass** (1 iteration; caught a vacuous traversal/dotfile test and made it real) · p04 skipped (no-op docs) · **final pass** (0 Critical / 0 Important; 4 minors accept-deferred with rationale). + +## Deferred / Follow-ups + +Four Minor findings accept-deferred (rationale in `implementation.md`): plaintext-HTTP WS assumption for an unsupported HTTPS-tunnel mode (fails closed); pre-existing dead code block in `start-server.sh`; a WS-auth doc wording nit; an untested `nextReconnectDelay` export. Separately, `bl-7d5b` (live-dogfood `oat-brainstorm`) remains open — live dogfooding was out of scope for this port. diff --git a/.oat/sync/manifest.json b/.oat/sync/manifest.json index 7a87dd407d..a63541dcf6 100644 --- a/.oat/sync/manifest.json +++ b/.oat/sync/manifest.json @@ -1,6 +1,6 @@ { "version": 1, - "oatVersion": "0.1.38", + "oatVersion": "0.1.41", "entries": [ { "canonicalPath": ".agents/agents/oat-codebase-mapper.md", diff --git a/NOTICES.md b/NOTICES.md index 980a1ca614..cf9f260536 100644 --- a/NOTICES.md +++ b/NOTICES.md @@ -9,10 +9,11 @@ add attribution footers to the skill files themselves. **Source:** https://github.com/obra/superpowers **License:** MIT -**Version referenced:** 5.0.7 ### `brainstorming` skill +**Version referenced:** 5.0.7 + Source file: `skills/brainstorming/SKILL.md` Passages adapted or lifted verbatim into OAT: @@ -28,20 +29,36 @@ Consumer OAT skills: `oat-project-design`, `oat-project-quick-start` ### `brainstorming` skill — visual companion +**Version referenced:** 6.0.3 + Source files: `skills/brainstorming/scripts/{server.cjs, start-server.sh, stop-server.sh, frame-template.html, helper.js}` and `skills/brainstorming/visual-companion.md`. -Files lifted into OAT (under `.agents/skills/oat-brainstorm/`): +Files adapted into OAT (under `.agents/skills/oat-brainstorm/`) — this is a +security/resilience-parity port from the v6.0.3 tag, not a byte-for-byte +carry-over of any single upstream file: -- `scripts/server.cjs`, `scripts/stop-server.sh`, `scripts/frame-template.html`, - `scripts/helper.js` — verbatim from upstream. -- `scripts/start-server.sh` — verbatim except for default persistence-path - changes (`.superpowers/brainstorm/` → OAT-managed prefixes: - `<project>/.oat/brainstorm/`, `<repo-root>/.oat/brainstorm/`, - `~/.oat/brainstorm/`). -- `references/visual-companion.md` — adapted prose: persistence paths and - example invocations updated to OAT conventions. +- `scripts/server.cjs` — v6.0.3 session-key auth (HTTP + WebSocket), sandboxed + `/files/` serving (no symlinks/dotfiles/traversal), security headers, + WebSocket max-frame guard, and configurable idle timeout; adapted to OAT's + `CONTENT_DIR`/`STATE_DIR` layout and existing JSON log events, with + upstream telemetry/branding hooks omitted. +- `scripts/helper.js`, `scripts/frame-template.html` — v6.0.3 client + resilience (status pill, exponential-backoff reconnect, paused overlay, + keyed reload) merged alongside OAT's pre-existing indicator-bar UI (kept, + not removed, unlike upstream v6.0.3); header rebranded to "OAT Brainstorm". +- `scripts/start-server.sh` — v6.0.3 launcher flags (`--open`, + `--idle-timeout-minutes`, `umask 077`, per-start server-instance-id, + Windows-shell handling) merged into OAT's three-tier persistence + resolution (`--project-dir` → repo walk-up → `~/.oat/brainstorm/`), + replacing the prior `.superpowers/brainstorm/`-style defaults. +- `scripts/stop-server.sh` — v6.0.3 instance-ID verification before + signaling a PID, adapted to retain OAT's persistent (non-`/tmp`) session + directories rather than deleting them. +- `references/visual-companion.md` — adapted prose: session-key URL, + `--open`, restart/port reuse, idle default, and OAT persistence-path + examples. Consumer OAT skills: `oat-brainstorm`. diff --git a/packages/cli/assets/public-package-versions.json b/packages/cli/assets/public-package-versions.json index 74870d1d63..8274f1ed85 100644 --- a/packages/cli/assets/public-package-versions.json +++ b/packages/cli/assets/public-package-versions.json @@ -1,6 +1,6 @@ { - "cli": "0.1.40", - "docs-config": "0.1.40", - "docs-theme": "0.1.40", - "docs-transforms": "0.1.40" + "cli": "0.1.41", + "docs-config": "0.1.41", + "docs-theme": "0.1.41", + "docs-transforms": "0.1.41" } diff --git a/packages/cli/package.json b/packages/cli/package.json index 2403bb9fdd..0c57314d27 100644 --- a/packages/cli/package.json +++ b/packages/cli/package.json @@ -1,6 +1,6 @@ { "name": "@open-agent-toolkit/cli", - "version": "0.1.40", + "version": "0.1.41", "private": false, "description": "Open Agent Toolkit CLI", "homepage": "https://github.com/voxmedia/open-agent-toolkit/tree/main/packages/cli", diff --git a/packages/cli/src/integration/visual-companion-smoke.test.ts b/packages/cli/src/integration/visual-companion-smoke.test.ts index 6cfcc45f7c..4940fab7a2 100644 --- a/packages/cli/src/integration/visual-companion-smoke.test.ts +++ b/packages/cli/src/integration/visual-companion-smoke.test.ts @@ -12,8 +12,16 @@ * - fallback to $HOME/.oat/brainstorm/ */ import { spawn } from 'node:child_process'; -import { existsSync, mkdirSync, readFileSync, realpathSync } from 'node:fs'; +import { + existsSync, + mkdirSync, + readFileSync, + realpathSync, + symlinkSync, + writeFileSync, +} from 'node:fs'; import { mkdtemp, rm } from 'node:fs/promises'; +import { request as httpRequest } from 'node:http'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; @@ -129,6 +137,93 @@ async function stopServer(sessionDir: string): Promise<void> { }); } +interface StopServerResult { + status: string; + error?: string; +} + +// Like stopServer(), but captures and parses stop-server.sh's JSON stdout so +// tests can assert on the reported status (e.g. "stale_pid") instead of only +// observing side effects. +async function runStopServer(sessionDir: string): Promise<StopServerResult> { + return await new Promise((resolve, reject) => { + let stdout = ''; + const child = spawn( + 'bash', + [join(SCRIPTS_DIR, 'stop-server.sh'), sessionDir], + { stdio: ['ignore', 'pipe', 'ignore'] }, + ); + child.stdout.on('data', (chunk: Buffer) => { + stdout += chunk.toString('utf8'); + }); + child.on('error', reject); + child.on('close', () => { + try { + resolve(JSON.parse(stdout.trim()) as StopServerResult); + } catch (err) { + reject( + new Error( + `stop-server.sh output not JSON-parsable: ${stdout} (${(err as Error).message})`, + ), + ); + } + }); + }); +} + +interface RawHttpResponse { + status: number; + headers: Record<string, string | string[] | undefined>; + body: string; +} + +// Issues a GET with an explicit request-target (pathOverride), bypassing the +// WHATWG URL parser's dot-segment normalization that `fetch()`/`new URL()` +// would otherwise apply. This lets tests send a literal `/files/../x` or +// dotfile path to exercise server.cjs's own sandboxing logic rather than +// having the client silently rewrite the path before it is sent. +async function rawHttpGet( + baseUrl: string, + pathOverride: string, +): Promise<RawHttpResponse> { + const target = new URL(baseUrl); + return await new Promise((resolve, reject) => { + const req = httpRequest( + { + hostname: target.hostname, + port: target.port, + path: pathOverride, + method: 'GET', + }, + (res) => { + let body = ''; + res.on('data', (chunk: Buffer) => { + body += chunk.toString('utf8'); + }); + res.on('end', () => { + resolve({ + status: res.statusCode ?? 0, + headers: res.headers, + body, + }); + }); + }, + ); + req.on('error', reject); + req.end(); + }); +} + +function keyFromServerUrl(url: string): string { + const key = new URL(url).searchParams.get('key'); + if (!key) { + throw new Error( + `expected server-started URL to include ?key=, got: ${url}`, + ); + } + return key; +} + describe('visual-companion server smoke test', () => { afterEach(async () => { await Promise.all( @@ -279,3 +374,208 @@ describe('start-server.sh persistence-path resolution', () => { } }, 30_000); }); + +describe('visual-companion server v6 security hardening', () => { + afterEach(async () => { + await Promise.all( + tempDirs.map(async (dir) => { + await rm(dir, { recursive: true, force: true }); + }), + ); + tempDirs.length = 0; + }); + + it('issues a session-keyed server-started URL and enforces auth + security headers on /', async () => { + const projectDir = await makeTempProjectDir(); + const { info } = await startServer({ projectDir }); + const key = keyFromServerUrl(info.url); + + try { + // server-started JSON must carry the session key on the URL so the + // agent can hand a working link straight to the user. + expect(new URL(info.url).searchParams.get('key')).toBe(key); + + // Unauthenticated root request: the shipped server rejects with 403 + // (not 401 — there is no WWW-Authenticate challenge, just a gate). + const unauth = await rawHttpGet(info.url, '/'); + expect(unauth.status).toBe(403); + expect(unauth.headers['cache-control']).toBe('no-store'); + expect(unauth.headers['x-frame-options']).toBe('DENY'); + + // Authenticated request succeeds and still carries the same security + // headers (they're applied uniformly via securityHeaders()). + const auth = await rawHttpGet(info.url, `/?key=${key}`); + expect(auth.status).toBe(200); + expect(auth.headers['cache-control']).toBe('no-store'); + expect(auth.headers['x-frame-options']).toBe('DENY'); + } finally { + const sessionDir = info.state_dir.replace(/\/state\/?$/, ''); + await stopServer(sessionDir); + } + }, 30_000); + + it('sandboxes /files/ against path traversal and dotfile access', async () => { + const projectDir = await makeTempProjectDir(); + const { info } = await startServer({ projectDir }); + const key = keyFromServerUrl(info.url); + const sessionDir = info.state_dir.replace(/\/state\/?$/, ''); + + // Real fixtures the guard would actually leak if it were absent: a + // marker one level above CONTENT_DIR (screen_dir) that a `..` traversal + // would reach, and a real dotfile *inside* CONTENT_DIR. Targeting files + // that never exist (e.g. `../server.cjs`, `.hidden`) 404s whether or + // not the guard exists and proves nothing — mirror the real-fixture + // technique the symlink-escape test below already uses. + const traversalMarker = join(sessionDir, 'sentinel.html'); + writeFileSync(traversalMarker, 'traversal-should-be-blocked\n'); + const dotfileMarker = join(info.screen_dir, '.hidden.html'); + writeFileSync(dotfileMarker, 'dotfile-should-be-hidden\n'); + + try { + // http.request with an explicit `path` preserves the literal ".." — + // unlike fetch()/new URL(), it does not collapse dot-segments before + // the request is sent — so this exercises server.cjs's own + // path.basename()-based sandboxing of /files/ rather than relying on + // client-side URL normalization to do the job. + const traversal = await rawHttpGet( + info.url, + `/files/../sentinel.html?key=${key}`, + ); + expect(traversal.status).toBeGreaterThanOrEqual(400); + expect(traversal.status).toBeLessThan(500); + expect(traversal.body).not.toContain('traversal-should-be-blocked'); + + const dotfile = await rawHttpGet( + info.url, + `/files/.hidden.html?key=${key}`, + ); + expect(dotfile.status).toBeGreaterThanOrEqual(400); + expect(dotfile.status).toBeLessThan(500); + expect(dotfile.body).not.toContain('dotfile-should-be-hidden'); + } finally { + await stopServer(sessionDir); + } + }, 30_000); + + it('sandboxes /files/ against symlink escape out of the content dir', async () => { + const projectDir = await makeTempProjectDir(); + const { info } = await startServer({ projectDir }); + const key = keyFromServerUrl(info.url); + + // Fixture living outside the served content dir (screen_dir). + const secretPath = join(projectDir, 'secret-outside-content-dir.txt'); + writeFileSync(secretPath, 'outside-content-dir\n'); + const symlinkPath = join(info.screen_dir, 'escape-link.html'); + + try { + // Real symlink-escape fixture: a symlink living *inside* the served + // content dir that resolves to a file *outside* it. server.cjs's + // isRegularFileInsideContentDir() rejects any lstat().isSymbolicLink() + // before ever resolving the real path, so this is a genuine assertion + // of the sandbox rejecting the escape (not merely a 404 for a missing + // file) on the platform this suite runs on (macOS/Linux). If symlink + // creation itself is unsupported in a given CI sandbox (e.g. Windows + // without developer mode), fs.symlinkSync throws before the HTTP + // assertion runs, which fails the test loudly rather than silently + // skipping the security assertion. + symlinkSync(secretPath, symlinkPath); + + const response = await rawHttpGet( + info.url, + `/files/escape-link.html?key=${key}`, + ); + expect(response.status).toBeGreaterThanOrEqual(400); + expect(response.status).toBeLessThan(500); + expect(response.body).not.toContain('outside-content-dir'); + } finally { + const sessionDir = info.state_dir.replace(/\/state\/?$/, ''); + await stopServer(sessionDir); + } + }, 30_000); +}); + +describe('visual-companion server v6 lifecycle', () => { + afterEach(async () => { + await Promise.all( + tempDirs.map(async (dir) => { + await rm(dir, { recursive: true, force: true }); + }), + ); + tempDirs.length = 0; + }); + + it('reuses the recorded port on restart with the same --project-dir (reads .last-port)', async () => { + const projectDir = await makeTempProjectDir(); + const lastPortFile = join(projectDir, '.oat', 'brainstorm', '.last-port'); + + const first = await startServer({ projectDir }); + try { + expect(existsSync(lastPortFile)).toBe(true); + expect(readFileSync(lastPortFile, 'utf8').trim()).toBe( + String(first.info.port), + ); + } finally { + const sessionDir = first.info.state_dir.replace(/\/state\/?$/, ''); + await stopServer(sessionDir); + } + + // Give the OS a moment to free the bound port before restarting, so the + // second server actually reclaims it instead of falling back. + await sleep(500); + + const second = await startServer({ projectDir }); + try { + expect(second.info.port).toBe(first.info.port); + expect(readFileSync(lastPortFile, 'utf8').trim()).toBe( + String(second.info.port), + ); + // SC#2 also requires the session key (not just the port) to survive a + // restart, so an already-open browser tab's cookie keeps validating. + // The key is persisted via BRAINSTORM_TOKEN_FILE (.last-token) and + // re-read on the next start (see server.cjs initialToken()). + expect(keyFromServerUrl(second.info.url)).toBe( + keyFromServerUrl(first.info.url), + ); + } finally { + const sessionDir = second.info.state_dir.replace(/\/state\/?$/, ''); + await stopServer(sessionDir); + } + }, 30_000); + + it('stop-server.sh refuses to signal a process whose recorded server-instance-id no longer matches (fails closed)', async () => { + const projectDir = await makeTempProjectDir(); + const { info } = await startServer({ projectDir }); + const sessionDir = info.state_dir.replace(/\/state\/?$/, ''); + const pidFile = join(info.state_dir, 'server.pid'); + const serverIdFile = join(info.state_dir, 'server-instance-id'); + const realPid = readFileSync(pidFile, 'utf8').trim(); + + try { + // Corrupt the recorded instance id so it no longer matches the running + // process's --brainstorm-server-id argv. stop-server.sh must fail + // closed rather than trust the pid file alone (guards against a stale + // pid file pointing at an unrelated process after reuse/wraparound). + writeFileSync(serverIdFile, '0'.repeat(40)); + + const result = await runStopServer(sessionDir); + expect(result.status).toBe('stale_pid'); + + // The real server process must still be alive and serving — the guard + // refused to signal it rather than killing an unverified pid. + const stillUp = await rawHttpGet( + info.url, + `/?key=${keyFromServerUrl(info.url)}`, + ); + expect(stillUp.status).toBe(200); + } finally { + // The guard intentionally left the real process running (and removed + // the pid file), so stop-server.sh can no longer prove ownership after + // the tampering above. Clean up the still-running process directly. + try { + process.kill(Number(realPid), 'SIGTERM'); + } catch { + /* already gone */ + } + } + }, 30_000); +}); diff --git a/packages/control-plane/package.json b/packages/control-plane/package.json index 908330f741..84f259987a 100644 --- a/packages/control-plane/package.json +++ b/packages/control-plane/package.json @@ -1,6 +1,6 @@ { "name": "@open-agent-toolkit/control-plane", - "version": "0.1.40", + "version": "0.1.41", "private": false, "description": "Read-only OAT control-plane library for structured project state", "homepage": "https://github.com/voxmedia/open-agent-toolkit/tree/main/packages/control-plane", diff --git a/packages/docs-config/package.json b/packages/docs-config/package.json index b25de4f920..d0a4d459b1 100644 --- a/packages/docs-config/package.json +++ b/packages/docs-config/package.json @@ -1,6 +1,6 @@ { "name": "@open-agent-toolkit/docs-config", - "version": "0.1.40", + "version": "0.1.41", "private": false, "description": "Configuration factories for OAT documentation sites", "homepage": "https://github.com/voxmedia/open-agent-toolkit/tree/main/packages/docs-config", diff --git a/packages/docs-theme/package.json b/packages/docs-theme/package.json index f0bbab0362..7b71fe1ab3 100644 --- a/packages/docs-theme/package.json +++ b/packages/docs-theme/package.json @@ -1,6 +1,6 @@ { "name": "@open-agent-toolkit/docs-theme", - "version": "0.1.40", + "version": "0.1.41", "private": false, "description": "Theme components for OAT documentation sites", "homepage": "https://github.com/voxmedia/open-agent-toolkit/tree/main/packages/docs-theme", diff --git a/packages/docs-transforms/package.json b/packages/docs-transforms/package.json index 39210f87f8..22b6bfe299 100644 --- a/packages/docs-transforms/package.json +++ b/packages/docs-transforms/package.json @@ -1,6 +1,6 @@ { "name": "@open-agent-toolkit/docs-transforms", - "version": "0.1.40", + "version": "0.1.41", "private": false, "description": "Remark/unified transforms for OAT documentation", "homepage": "https://github.com/voxmedia/open-agent-toolkit/tree/main/packages/docs-transforms",