diff --git a/docs/superpowers/plans/2026-06-09-visual-companion-issues.md b/docs/superpowers/plans/2026-06-09-visual-companion-issues.md new file mode 100644 index 00000000..82802336 --- /dev/null +++ b/docs/superpowers/plans/2026-06-09-visual-companion-issues.md @@ -0,0 +1,350 @@ +# Visual Brainstorming Companion — Issue & Change Catalog + +**Date:** 2026-06-09 +**Status:** Analysis / triage. We are implementing these ourselves; the referenced +community PRs are evidence and reference material, **not** code we intend to merge. + +## Purpose + +A single place that captures every open issue and PR touching the visual +brainstorming companion (the local server in `skills/brainstorming/scripts/`), +distilled to the underlying problem and the change we'd make. Each item is +grounded against the current code, not the PR author's description. + +## Scope decisions (Jesse, 2026-06-09) + +- **Not vendoring Alpine.js.** PR #1639 (interactive mockups via a vendored + Alpine build) is **dropped**. See E3. +- **E1 (terminal-vs-HTML hard gate) is a workshop item.** We'll design it + together; it is not specced here. +- **E2 (storage location, #975/#977) is deferred** for now. +- **Remote serving is a first-class scenario.** Superpowers is general-purpose; + users connect from remote (SSH tunnel, Tailscale, `--host 0.0.0.0`). The + security fix MUST protect those users, not just loopback. **Decision: a + per-session secret key**, not a Host/Origin allowlist. The allowlist only + defends the loopback browser-confused-deputy; a direct remote client just + sends the expected `Host`, so the allowlist is theater for remote exposure. A + secret key is the only thing that authenticates a client uniformly across + loopback, tunnel, and direct-remote, and it also defeats DNS rebinding. See A1. + +## Component map + +| File | Role | +|------|------| +| `skills/brainstorming/scripts/server.cjs` | Zero-dep HTTP + WebSocket server (RFC 6455 hand-rolled). Serves the newest screen, watches `content/`, records events to `state/events`. | +| `skills/brainstorming/scripts/helper.js` | Injected into every page. WebSocket client, click capture, `window.brainstorm` API. | +| `skills/brainstorming/scripts/frame-template.html` | Frame (header, theme CSS, status dot, indicator bar) wrapped around content fragments. | +| `skills/brainstorming/scripts/start-server.sh` | Launch wrapper. Session dir, host/url-host, owner-PID resolution, platform backgrounding. | +| `skills/brainstorming/scripts/stop-server.sh` | Kills the server by PID file, cleans `/tmp` sessions. | +| `skills/brainstorming/visual-companion.md` | Operator guide the agent reads when it accepts the companion. | +| `skills/brainstorming/SKILL.md` | Where the companion is offered and the per-question decision lives. | + +## Disposition summary + +| ID | Item | Source | Disposition | +|----|------|--------|-------------| +| A1 | Per-session secret key on `/`, `/files/*`, and WS (supersedes Host/Origin) | issues #1014, PRs #1110/#1553 | **Do** — chosen approach | +| A2 | ~~Host/Origin allowlist~~ | PRs #1110/#1553 | Dropped — subsumed by A1 | +| A3 | Crash on `null` / non-object WS payload | PR #1504 | Do | +| A4 | Frame-length bound in `decodeFrame` | issue #1446 | Already fixed — verify/close | +| B1 | Dotfile screens served as content (`._*.html`) | PR #950 | Do | +| B2 | `stop-server.sh` kills reused/stale PID | PR #1703 | Do | +| B3 | WS client reconnect backoff + status indicator | PR #856 | Do | +| C1 | Idle timeout too short / not configurable; WS not closed on shutdown | issue #1237 (PR #1689) | Do | +| C2 | Server death is invisible to user/agent | issue #1237 (residual) | Do | +| D1 | Permanent opt-out of the companion | issue #892 | Do — design choice open | +| D2 | Free-text feedback from the browser | issue #957 | Do — needs server change too | +| D3 | Auto-open the companion URL | PR #759 (#755) | Do | +| D4 | Light/dark contrast helpers in the frame | PR #1683 | Do | +| E1 | Hard-gate terminal-vs-HTML per question | PR #1037 | **Workshop** | +| E2 | Move session state out of the working tree | issue #975 (PR #977) | **Deferred** | +| E3 | Vendor Alpine.js for interactive mockups | PR #1639 | **Dropped** | +| E4 | Shell-lint warnings in start/stop scripts | PR #1677 | Opportunistic only | + +--- + +## A. Server security hardening (`server.cjs`) + +### A1 — Per-session secret key (chosen approach) + +**Threat model.** Two assets: confidentiality of the served screen (`/`) and +files (`/files/*`), and integrity of `state/events` — a WebSocket client with a +truthy `choice` writes there (`server.cjs:243-246`), and the agent reads it next +turn as the user's selection, i.e. **prompt injection into a live session with +full tool access**. Reachers: with the default `127.0.0.1` bind, a malicious +page in the user's browser (a confused deputy — runs attacker JS *and* can reach +loopback); with a remote bind (`--host 0.0.0.0`, tailnet/LAN), any host that can +route to the port, directly, with no same-origin policy in the way. Today +`handleUpgrade` (`server.cjs:176`) checks only `Sec-WebSocket-Key`, and +`handleRequest` (`server.cjs:138`) checks nothing — both are wide open. + +**Why a key, not an allowlist.** A Host/Origin allowlist only defends the +loopback browser-deputy. A direct remote client just sends the expected `Host` +and forges/omits `Origin`, so the allowlist is theater for exactly the remote +case we must protect. A per-session secret authenticates the client uniformly +across loopback, SSH tunnel, and direct-remote, and it also kills DNS rebinding +(the rebound page neither knows the key nor receives the host-scoped cookie). +So the key **supersedes** A1/A2's allowlist entirely — no `BRAINSTORM_ALLOWED_HOSTS`. + +**Design.** Random token (`crypto.randomBytes(32)` hex), generated in +`server.cjs` at startup (overridable via `BRAINSTORM_TOKEN` for deterministic +tests): + +1. **URL carries it** as `?key=`. The server already builds `url` in its + `server-started` JSON (`server.cjs:351`) and writes it to `state/server-info` + — appending `?key=` there means `start-server.sh` (greps and prints that + JSON) and the skill (hands the user that URL) need **no change**. +2. **Cookie bootstrap.** A valid `?key` on `/` sets + `brainstorm-key-=; HttpOnly; SameSite=Strict; Path=/`. The + browser then auto-attaches it to same-origin subresources (`/files/*`) and + the WebSocket handshake, so the agent can write any URL style and it works, + and `helper.js` needs no change. Cookie name is **per-port** to avoid the + Jupyter multi-server collision (cookies aren't port-scoped). + `SameSite=Strict` is safe for CDN/Unsplash content — that cookie is host- + scoped, so outbound CDN requests never carry it; SameSite only governs + requests back to our origin, which are all same-site. +3. **Auth gate** = valid `?key` **OR** valid cookie (compared with + `crypto.timingSafeEqual`) on `/`, `/files/*`, and the WS upgrade. Missing/bad + key → friendly **403 HTML page** ("this page needs the full URL your coding + agent gave you, including `?key=…`" — generic "coding agent", not "Claude", + since this ships on Codex/Gemini/Copilot too). WS upgrade → destroy socket. + +The query token is the source of truth; the cookie is a convenience that never +bears initial-auth load. + +**Blast radius.** `server.cjs` (all logic). `helper.js` optional one-liner +(append `?key=` from `location.search` to the WS URL as a cookie-blocked +fallback). `start-server.sh` none. `visual-companion.md` doc note (URL now has +`?key=`; don't strip it). Tests updated to pass the token. + +### A2 — Host/Origin allowlist — DROPPED + +Subsumed by A1. The secret key closes the WS-injection vector (#1014), the +HTTP/WS DNS-rebinding read vector (PR #1553), and the cross-origin WS vector +(PR #1110) in one mechanism, and unlike an allowlist it actually protects the +remote-bind case. No `Host`/`Origin` validation, no `BRAINSTORM_ALLOWED_HOSTS`. + +### A3 — Server crashes on `null` / primitive WS payload + +**Problem.** `handleMessage` (`server.cjs:233`) does `JSON.parse(text)` then +`if (event.choice)` at `server.cjs:243`. A client that sends the 4-byte text +frame `null` yields `event === null`, and `null.choice` throws. The throw is +**not** caught — `handleMessage` is called from the `socket.on('data')` handler +(`server.cjs:207`) outside the `try/catch`, which only wraps `decodeFrame`. The +result is an uncaught exception and process exit. Any local client can kill the +server. + +**Change.** Guard the access: `if (event && event.choice)`. Minimal and exact — +`JSON.parse` can't produce `undefined`, and primitives return `undefined` for +`.choice` without throwing, so only `null` is the live hazard. (Avoid the +broader fixes — a top-level `try/catch` or `process.on('uncaughtException')` +would mask other bugs.) + +### A4 — Frame-length bound in `decodeFrame` (adjacent) + +Referenced by PR #1504 as #1446. The current code **already** bounds extended +frame lengths: `MAX_FRAME_PAYLOAD_BYTES = 10MB` (`server.cjs:10`) is enforced at +`server.cjs:58-67` before any `Buffer.alloc`. Action: verify #1446 against +current `dev` and close if already resolved, rather than re-implementing. + +--- + +## B. Server robustness / correctness + +### B1 — macOS resource-fork dotfiles served as screen content + +**Problem.** The newest-screen selector filters on `f.endsWith('.html')` only +(`server.cjs:127-128`). On macOS/ExFAT, `._screen.html` resource-fork files pass +that filter and, being written alongside the real file, can sort newest — so the +browser gets binary metadata instead of the mockup. Four read sites share the +weak filter: `getNewestScreen` (`server.cjs:127`), `knownFiles` init +(`server.cjs:279`), the `fs.watch` handler (`server.cjs:286`), and the `/files/` +endpoint (`server.cjs:154-156`). + +**Change.** Reject dotfiles (`!f.startsWith('.')`) at all four sites. Covers +`._*`, `.DS_Store`, etc. + +### B2 — `stop-server.sh` can kill a reused PID + +**Problem.** `stop-server.sh` reads the PID from `state/server.pid` +(`stop-server.sh:20`) and `kill`s it (`:23`, escalating to `-9` at `:35`) +without confirming the PID still belongs to our server. After a reboot or PID +wraparound the file can point at an unrelated process, which we'd then SIGKILL. + +**Change.** Before signalling, verify ownership — the PID's command is `node` +running our `server.cjs`, ideally matching this session. If ownership can't be +proven, fail closed (report `stale_pid`, don't kill). Keep the existing +`stopped` / `not_running` outputs for the real cases. + +### B3 — WebSocket client: silent reconnect, stale "Connected" + +**Problem.** `helper.js` reconnects on a fixed 1s timer (`helper.js:21-23`), +has no `onerror` handler, never nulls `ws` on close, and never clears a pending +reconnect timer. The frame's status element is hardcoded to "Connected" with the +dot pinned to `var(--success)` (`frame-template.html:77,200`). When the laptop +sleeps or the server restarts, the page shows "Connected" over a dead socket and +queues events with no feedback. + +**Change.** +- `helper.js`: exponential backoff (500ms → ×2 → cap 30s, reset on open); + `onerror` delegating to `onclose`; `ws = null` on close; `clearTimeout` before + reconnecting. +- `frame-template.html`: drive the status dot from a `--status-color` custom + property so JS can switch Connected (green) / Reconnecting (yellow) / + Disconnected (red). + +--- + +## C. Lifecycle / timeout (issue #1237) + +### C1 — Idle timeout too short, not configurable, WS keeps process alive + +**Problem.** `IDLE_TIMEOUT_MS` is hardcoded to 30 minutes (`server.cjs:258`), +enforced by the 60s lifecycle check (`server.cjs:329-332`). A single brainstorm +question can sit longer than 30 min while the user thinks or steps away, so the +server dies mid-session. Separately, `shutdown()` (`server.cjs:310-321`) calls +`server.close()` but never closes the upgraded sockets in `clients` +(`server.cjs:174`), so an open browser connection can keep the Node process +alive past shutdown. + +**Change.** +- Raise the default (≈2h) and make it configurable: + `--idle-timeout-minutes` in `start-server.sh` → an env var → `IDLE_TIMEOUT_MS`, + with validation against Node timer overflow. +- Expose the effective timeout in the startup JSON / `state/server-info`. +- In `shutdown()`, close every socket in `clients` so the process actually + exits. + +### C2 — Server death is invisible + +**Problem.** When the server exits it writes `state/server-stopped` and removes +`state/server-info` (`server.cjs:312-317`), and the skill is *told* to check +those files (`visual-companion.md:98`) — but it's soft guidance the model skips, +and the browser just shows a generic "can't be reached." The user diagnoses it +manually; the agent keeps referring to a dead URL. + +**Change (two parts, independent of C1):** +- **Browser-facing tombstone.** Leave something at the last-served URL that says + "this companion expired — ask Claude to restart it" instead of a connection + error. Options to weigh: `helper.js` rendering a banner when the socket stays + down past backoff (works only while the page is loaded), vs. a more involved + approach that keeps a minimal responder alive to serve a tombstone page. +- **Harder skill check.** Tighten `visual-companion.md` / `SKILL.md` so + "check `server-info`/`server-stopped` before referring to the URL or pushing a + screen" is a required step, not a note. Keep it lightweight — possibly a + one-line helper the agent always runs. + +--- + +## D. Features + +### D1 — Permanent opt-out of the visual companion (issue #892) + +**Problem.** The companion is offered as its own message every session +(`SKILL.md:25,151-152`). A user who never wants it pays that round-trip — and +HTML generation — every time. There's no way to say "never offer this." + +**Change.** Before the offer step, the skill checks a user-level setting and +skips the offer entirely when opt-out is set. + +**Design choice open.** Mechanism isn't settled: +- Env var (e.g. `SUPERPOWERS_VISUAL_COMPANION=off`) the skill is told to read — + simplest, matches what the issue asks for, lives in `.zshrc`. +- A plugin-settings file (`.claude/superpowers.local.md` frontmatter) — more + structured, per-project capable, but heavier and project-scoped. +- Reliability caveat from the issue: a separate "no-companion" skill competes on + trigger words and isn't reliable — rejected. + +Pick the mechanism, then it's a small `SKILL.md` change plus a documented knob. + +### D2 — Free-text feedback from the browser (issue #957) + +**Problem.** The client only captures clicks on `[data-choice]` +(`helper.js:36-62`). A user who wants to annotate a mockup ("wrong shade of +blue") has to switch to the terminal, breaking the visual flow. + +**Change.** Add a feedback `