feat: document review system and workflow enforcement #334
Conversation
📝 WalkthroughWalkthroughThis PR introduces a visual brainstorming companion system, implements document review workflows with subagents, and restructures the documentation directory. It adds a Node.js Express/WebSocket server for interactive UI feedback, creates spec and plan reviewer prompts, and migrates plans/specs from Changes
Sequence Diagram(s)sequenceDiagram
participant Claude as Claude Agent
participant Server as Brainstorm Server
participant Browser as Browser (WebSocket)
participant FileSystem as File System
Claude->>Server: POST /start-server.sh (session)
Server->>FileSystem: Create session dir, PID file
Server->>Server: Listen on port, watch screen dir
Note over Claude: Generate HTML screen
Claude->>FileSystem: Write screen.html to /tmp/brainstorm/
Server->>FileSystem: Detect new HTML file via chokidar
Server->>Browser: Broadcast reload message
Browser->>Browser: Reload and render HTML with injected helper.js
Browser->>Browser: Auto-capture click, input, form events
User->>Browser: Interact (click option, submit form)
Browser->>Server: Send event via WebSocket
Server->>FileSystem: Log event to stdout/log
Claude->>FileSystem: Poll for send-to-claude event
User->>Browser: Submit feedback
Browser->>Server: Send send-to-claude event
Server->>FileSystem: Log to stdout
Claude->>FileSystem: Capture feedback, process
Claude->>Claude: Decide: iterate (new screen) or proceed
opt Iterate
Claude->>FileSystem: Write new screen.html
Server->>Browser: Reload notification
Browser->>Browser: Re-render, repeat capture cycle
end
Claude->>Server: POST /stop-server.sh
Server->>FileSystem: Kill PID, remove session dir
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Rationale: The PR introduces multiple interconnected features (brainstorm server with WebSocket, review system workflows, directory restructuring) spanning 40+ files. While many changes are additive and follow consistent patterns (server implementation, review templates), the breadth includes logic-dense server code, new control flows (review loops with subagents), and pervasive path updates requiring careful verification. Homogeneous path-refactoring changes reduce effort, but the heterogeneous server logic, new workflows, and cross-cutting documentation updates demand focused review. Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Fix all issues with AI agents
In `@docs/README.opencode.md`:
- Around line 171-173: The fenced code blocks containing the examples "use skill
tool to list skills" and "use skill tool to load superpowers/brainstorming" are
missing language identifiers; update those backtick blocks to include a language
such as text (e.g., change ``` to ```text) for each occurrence (including the
second example referenced at 179-181) so markdownlint MD040 no longer flags
them. Ensure every fenced block that shows those exact example lines is updated
consistently to include the language identifier.
In `@docs/superpowers/specs/2026-01-22-document-review-system-design.md`:
- Around line 28-39: The fenced code block that starts with "## Spec Review" is
missing a language identifier (MD040); update both opening triple-backtick
fences to include a language (e.g., change ``` to ```text) for the block
containing "## Spec Review" and the later fenced block containing the workflow
line "brainstorming -> spec -> SPEC REVIEW LOOP -> writing-plans -> plan -> PLAN
REVIEW LOOP -> implementation" so markdownlint stops flagging them; ensure you
update the opening fences for the blocks referenced in the diff and the other
occurrence at the section noted as "Also applies to: 83-85".
In `@hooks/hooks.json`:
- Line 9: The command in hooks.json uses an unquoted expansion of
CLAUDE_PLUGIN_ROOT which can break when the path contains spaces; update the
"command" value from "${CLAUDE_PLUGIN_ROOT}/hooks/session-start.sh" to use
single quotes around the expansion (i.e.,
'${CLAUDE_PLUGIN_ROOT}/hooks/session-start.sh') so the shell receives the path
as a single argument and the session-start.sh hook runs reliably.
In `@lib/brainstorm-server/helper.js`:
- Around line 15-20: The ws.onmessage handler currently calls
JSON.parse(msg.data) without protection; wrap the parse in a try/catch inside
the ws.onmessage function to prevent malformed JSON from throwing and breaking
the handler, e.g., try to parse msg.data, verify the resulting object exists and
has data.type === 'reload' before calling window.location.reload(), and in the
catch block log the parsing error (using console.error or a logger) and return
silently so the handler continues to run safely.
In `@lib/brainstorm-server/index.js`:
- Around line 55-63: The ws.on('message') handler currently calls JSON.parse
directly which can throw on malformed payloads and crash the server; update the
message handler inside the wss.on('connection' /* connection callback */) to
wrap the JSON.parse(data.toString()) call in a try/catch, handle parse errors by
logging the parse failure (include the raw data and error) via console.error or
process logger, and skip processing that message (optionally send an error
response over ws) instead of letting the exception bubble up and crash the
process; keep the rest of the successful-path behavior (building event and
console.log) unchanged.
In `@lib/brainstorm-server/package.json`:
- Around line 1-11: Update package.json to set "private": true at the top level
and bump the dependency versions: change "express" to a version >= 4.20.0 (or
latest 4.x/5.x) and "ws" to >= 8.17.1 in the "dependencies" object (leave
"chokidar" unchanged); after editing package.json run npm install (or yarn
install) to update package-lock.json/yarn.lock and verify no breaking changes.
In `@lib/brainstorm-server/start-server.sh`:
- Around line 33-45: The timeout branch currently exits without cleaning up the
background server and temporary directory; capture the background server PID
when launching it (e.g., assign $! to SERVER_PID where the server is started)
and on timeout kill that PID (first try kill "$SERVER_PID" then fallback to kill
-9 if needed), wait for it to exit, and remove the temp directory (e.g., rm -rf
"$TEMP_DIR" or "$TMP_DIR") before printing the JSON error and exiting with
status 1; also handle cases where SERVER_PID or the temp-dir variable may be
empty to avoid errors.
In `@tests/brainstorm-server/server.test.js`:
- Around line 8-10: The test is setting BRAINSTORM_SCREEN and a TEST_SCREEN path
but the server expects BRAINSTORM_DIR and the test does not create the screen
directory, causing ENOENT; update the test to export the correct env var (set
process.env.BRAINSTORM_DIR to the directory portion of TEST_SCREEN or adjust
SERVER_PATH usage) and ensure the test creates the directory for TEST_SCREEN
before any file writes (mkdir -p semantics) and clean it up after; apply the
same changes where TEST_SCREEN/port/env are used (the other occurrences around
the blocks referenced) so the server reads the right env and the directory
exists during reload tests.
In `@tests/claude-code/test-document-review-system.sh`:
- Around line 92-98: The failing exit code of the pipeline is overwritten by the
subsequent echo, so capture the exit status immediately after running the
command and before any other commands; assign the exit status of the `timeout
120 claude -p "$PROMPT" --permission-mode bypassPermissions 2>&1 | tee
"$OUTPUT_FILE"` pipeline (i.e., `$?`) to a local variable (e.g., EXIT_CODE)
right after it fails, then use that variable in the `echo "EXECUTION FAILED
(exit code: $EXIT_CODE)"` and the `exit $EXIT_CODE` calls; reference the
pipeline invocation and the `$?` usage in your fix.
🧹 Nitpick comments (9)
.gitattributes (1)
1-17: LGTM! Consider adding a default text normalization rule.The line-ending configuration is correct:
- LF for shell scripts ensures cross-platform compatibility
- LF for .cmd files is appropriate given their polyglot nature (parsed by both cmd and bash)
- Text and binary file coverage is comprehensive
Optional enhancement: Consider adding a default text normalization rule at the beginning of the file:
+# Auto-detect text files and normalize line endings +* text=auto + # Ensure shell scripts always have LF line endings *.sh text eol=lfThis provides defense-in-depth for file types not explicitly listed.
lib/brainstorm-server/stop-server.sh (1)
14-19: Consider validating SCREEN_DIR beforerm -rf.The script deletes
$SCREEN_DIRunconditionally when the PID file exists. If this script is ever invoked with an incorrect or malicious path argument,rm -rfcould cause unintended data loss.Consider adding a sanity check to ensure
SCREEN_DIRis within an expected parent directory (e.g., a temp directory or a known session root):Suggested safety check
if [[ -f "$PID_FILE" ]]; then pid=$(cat "$PID_FILE") kill "$pid" 2>/dev/null + # Safety: only remove if SCREEN_DIR looks like a session directory + if [[ "$SCREEN_DIR" == /tmp/* || "$SCREEN_DIR" == */brainstorm-session-* ]]; then # Clean up session directory rm -rf "$SCREEN_DIR" + else + echo '{"warning": "skipped cleanup - unexpected directory path"}' + fi echo '{"status": "stopped"}'skills/writing-plans/plan-document-reviewer-prompt.md (1)
9-9: Add language specifier to fenced code block.The code block lacks a language specifier. Since this contains a YAML-like task tool definition, consider adding
yamlfor syntax highlighting.Suggested fix
-``` +```yaml Task tool (general-purpose):lib/brainstorm-server/frame-template.html (2)
1-2: Addlangattribute to<html>element for accessibility.The
<html>element is missing thelangattribute, which is important for screen readers and accessibility compliance (WCAG 2.1 Success Criterion 3.1.1).Suggested fix
<!DOCTYPE html> -<html> +<html lang="en">
247-256: Add defensive check forbrainstormobject.The
send()function callsbrainstorm.sendToClaude(payload)without checking if thebrainstormobject exists. Ifhelper.jsfails to load or inject the global, this will throw a runtime error.Suggested fix
function send() { const feedbackEl = document.getElementById('feedback'); const feedback = feedbackEl.value.trim(); const payload = {}; if (selectedChoice) payload.choice = selectedChoice; if (feedback) payload.feedback = feedback; if (Object.keys(payload).length === 0) return; + if (typeof brainstorm === 'undefined' || !brainstorm.sendToClaude) { + console.error('Brainstorm helper not loaded'); + return; + } brainstorm.sendToClaude(payload); feedbackEl.value = ''; }lib/brainstorm-server/wait-for-feedback.sh (1)
16-27: Consider handling missing log file and adding a timeout.Two potential reliability concerns:
Missing log file: If
.server.logdoesn't exist when the script starts,wc -lreturns 0 (good), buttailwill silently fail on each poll iteration until the file is created. Consider adding an initial wait for the file.No timeout: The script polls indefinitely. If the server crashes or the user abandons the session, this script hangs forever. Consider adding an optional timeout parameter.
♻️ Suggested improvements
# Record current position in log file +# Wait for log file to exist (server may still be starting) +TIMEOUT=10 +while [[ ! -f "$LOG_FILE" ]] && (( TIMEOUT-- > 0 )); do + sleep 0.5 +done + LOG_POS=$(wc -l < "$LOG_FILE" 2>/dev/null || echo 0) +# Optional: Add max iterations for timeout (e.g., 1800 iterations = 6 minutes) +MAX_ITERATIONS=${2:-0} # 0 = no limit +ITERATION=0 + # Poll for new lines containing the event while true; do RESULT=$(tail -n +$((LOG_POS + 1)) "$LOG_FILE" 2>/dev/null | grep -m 1 "send-to-claude") if [[ -n "$RESULT" ]]; then echo "$RESULT" exit 0 fi + if [[ $MAX_ITERATIONS -gt 0 ]] && (( ++ITERATION >= MAX_ITERATIONS )); then + echo '{"error": "Timeout waiting for feedback"}' >&2 + exit 2 + fi sleep 0.2 doneskills/brainstorming/visual-companion.md (1)
22-30: Clarify the relationship between wait-for-feedback.sh and TaskOutput.The quick start shows two waiting mechanisms:
- Line 23:
wait-for-feedback.sh(shell-based polling)- Line 28:
TaskOutput(task_id, block=true, timeout=600000)(tool-based blocking)It's unclear how these interact. Does
TaskOutputread from the watcher's output, or are these alternative approaches? A brief note explaining the connection would help Claude (and developers) understand the workflow.skills/brainstorming/spec-document-reviewer-prompt.md (1)
9-9: Add a language identifier to the fenced code block.The static analysis tool flagged MD040 for missing language specification. Since this is a prompt template, consider using
textormarkdownas the language identifier.♻️ Suggested fix
-``` +```text Task tool (general-purpose):lib/brainstorm-server/helper.js (1)
72-88: Single debounce timer loses rapid cross-field input events.The shared
inputTimeoutvariable means typing in field A, then quickly switching to field B (within 500ms), cancels field A's event. If multiple form fields are expected, consider per-element debouncing.Per-element debounce alternative
// Auto-capture input changes (debounced) - let inputTimeout = null; + const inputTimeouts = new WeakMap(); document.addEventListener('input', (e) => { const target = e.target; if (!target.matches('input, textarea, select')) return; - clearTimeout(inputTimeout); - inputTimeout = setTimeout(() => { + clearTimeout(inputTimeouts.get(target)); + inputTimeouts.set(target, setTimeout(() => { send({ type: 'input', name: target.name || null, id: target.id || null, value: target.value, inputType: target.type || target.tagName.toLowerCase() }); - }, 500); // 500ms debounce + }, 500)); });
| ``` | ||
| use find_skills tool | ||
| use skill tool to list skills | ||
| ``` |
There was a problem hiding this comment.
Add fenced code block language identifiers (MD040).
Markdownlint will continue to flag these examples without a language.
🧹 Suggested fix
-```
+```text
use skill tool to list skills
```diff
-```
+```text
use skill tool to load superpowers/brainstorming
</details>
Also applies to: 179-181
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.18.1)</summary>
171-171: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
In @docs/README.opencode.md around lines 171 - 173, The fenced code blocks
containing the examples "use skill tool to list skills" and "use skill tool to
load superpowers/brainstorming" are missing language identifiers; update those
backtick blocks to include a language such as text (e.g., change totext)
for each occurrence (including the second example referenced at 179-181) so
markdownlint MD040 no longer flags them. Ensure every fenced block that shows
those exact example lines is updated consistently to include the language
identifier.
</details>
<!-- fingerprinting:phantom:poseidon:eagle -->
<!-- This is an auto-generated comment by CodeRabbit -->
| **Output format:** | ||
| ``` | ||
| ## Spec Review | ||
| **Status:** Approved | Issues Found | ||
| **Issues (if any):** | ||
| - [Section X]: [issue] - [why it matters] | ||
| **Recommendations (advisory):** | ||
| - [suggestions that don't block approval] | ||
| ``` |
There was a problem hiding this comment.
Add language identifiers to fenced code blocks (MD040).
markdownlint flags code fences without a language spec.
🔧 Proposed fix
-```
+```text
## Spec Review
**Status:** Approved | Issues Found
@@
-```
+```text
brainstorming -> spec -> SPEC REVIEW LOOP -> writing-plans -> plan -> PLAN REVIEW LOOP -> implementation</details>
Also applies to: 83-85
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.18.1)</summary>
29-29: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
In @docs/superpowers/specs/2026-01-22-document-review-system-design.md around
lines 28 - 39, The fenced code block that starts with "## Spec Review" is
missing a language identifier (MD040); update both opening triple-backtick
fences to include a language (e.g., change totext) for the block
containing "## Spec Review" and the later fenced block containing the workflow
line "brainstorming -> spec -> SPEC REVIEW LOOP -> writing-plans -> plan -> PLAN
REVIEW LOOP -> implementation" so markdownlint stops flagging them; ensure you
update the opening fences for the blocks referenced in the diff and the other
occurrence at the section noted as "Also applies to: 83-85".
</details>
<!-- fingerprinting:phantom:poseidon:eagle -->
<!-- This is an auto-generated comment by CodeRabbit -->
| { | ||
| "type": "command", | ||
| "command": "\"${CLAUDE_PLUGIN_ROOT}/hooks/run-hook.cmd\" session-start.sh" | ||
| "command": "${CLAUDE_PLUGIN_ROOT}/hooks/session-start.sh" |
There was a problem hiding this comment.
Quote CLAUDE_PLUGIN_ROOT to avoid path splitting.
Claude Code expands variables before shell execution, so unquoted paths with spaces will break the hook. Please wrap the variable in single quotes. Based on learnings, this is required for reliable hook execution.
✅ Proposed fix
- "command": "${CLAUDE_PLUGIN_ROOT}/hooks/session-start.sh"
+ "command": "'${CLAUDE_PLUGIN_ROOT}/hooks/session-start.sh'"
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "command": "${CLAUDE_PLUGIN_ROOT}/hooks/session-start.sh" | |
| "command": "'${CLAUDE_PLUGIN_ROOT}/hooks/session-start.sh'" |
🤖 Prompt for AI Agents
In `@hooks/hooks.json` at line 9, The command in hooks.json uses an unquoted
expansion of CLAUDE_PLUGIN_ROOT which can break when the path contains spaces;
update the "command" value from "${CLAUDE_PLUGIN_ROOT}/hooks/session-start.sh"
to use single quotes around the expansion (i.e.,
'${CLAUDE_PLUGIN_ROOT}/hooks/session-start.sh') so the shell receives the path
as a single argument and the session-start.sh hook runs reliably.
| ws.onmessage = (msg) => { | ||
| const data = JSON.parse(msg.data); | ||
| if (data.type === 'reload') { | ||
| window.location.reload(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Add error handling for WebSocket message parsing.
If the server sends malformed JSON, JSON.parse will throw and crash the handler silently. This could break the reload functionality.
Proposed fix
ws.onmessage = (msg) => {
- const data = JSON.parse(msg.data);
- if (data.type === 'reload') {
- window.location.reload();
+ try {
+ const data = JSON.parse(msg.data);
+ if (data.type === 'reload') {
+ window.location.reload();
+ }
+ } catch (e) {
+ console.warn('Invalid message from server:', e);
}
};
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ws.onmessage = (msg) => { | |
| const data = JSON.parse(msg.data); | |
| if (data.type === 'reload') { | |
| window.location.reload(); | |
| } | |
| }; | |
| ws.onmessage = (msg) => { | |
| try { | |
| const data = JSON.parse(msg.data); | |
| if (data.type === 'reload') { | |
| window.location.reload(); | |
| } | |
| } catch (e) { | |
| console.warn('Invalid message from server:', e); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In `@lib/brainstorm-server/helper.js` around lines 15 - 20, The ws.onmessage
handler currently calls JSON.parse(msg.data) without protection; wrap the parse
in a try/catch inside the ws.onmessage function to prevent malformed JSON from
throwing and breaking the handler, e.g., try to parse msg.data, verify the
resulting object exists and has data.type === 'reload' before calling
window.location.reload(), and in the catch block log the parsing error (using
console.error or a logger) and return silently so the handler continues to run
safely.
| wss.on('connection', (ws) => { | ||
| clients.add(ws); | ||
| ws.on('close', () => clients.delete(ws)); | ||
|
|
||
| ws.on('message', (data) => { | ||
| // User interaction event - write to stdout for Claude | ||
| const event = JSON.parse(data.toString()); | ||
| console.log(JSON.stringify({ source: 'user-event', ...event })); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check file existence and view the specific lines with context
if [ -f lib/brainstorm-server/index.js ]; then
echo "=== Lines 50-70 of lib/brainstorm-server/index.js ==="
sed -n '50,70p' lib/brainstorm-server/index.js | cat -n
echo ""
echo "=== Checking for error handlers on ws/wss objects ==="
rg -n "ws\.on\('error'|wss\.on\('error'|\.on\('error'" lib/brainstorm-server/index.js
else
echo "File not found"
fiRepository: obra/superpowers
Length of output: 951
Guard against invalid JSON in WebSocket messages.
A malformed client payload will throw on JSON.parse and crash the server process.
🔧 Proposed fix
ws.on('message', (data) => {
// User interaction event - write to stdout for Claude
- const event = JSON.parse(data.toString());
- console.log(JSON.stringify({ source: 'user-event', ...event }));
+ let event;
+ try {
+ event = JSON.parse(data.toString());
+ } catch (err) {
+ console.warn(JSON.stringify({ type: 'invalid-user-event', error: err.message }));
+ return;
+ }
+ console.log(JSON.stringify({ source: 'user-event', ...event }));
});🤖 Prompt for AI Agents
In `@lib/brainstorm-server/index.js` around lines 55 - 63, The ws.on('message')
handler currently calls JSON.parse directly which can throw on malformed
payloads and crash the server; update the message handler inside the
wss.on('connection' /* connection callback */) to wrap the
JSON.parse(data.toString()) call in a try/catch, handle parse errors by logging
the parse failure (include the raw data and error) via console.error or process
logger, and skip processing that message (optionally send an error response over
ws) instead of letting the exception bubble up and crash the process; keep the
rest of the successful-path behavior (building event and console.log) unchanged.
| { | ||
| "name": "brainstorm-server", | ||
| "version": "1.0.0", | ||
| "description": "Visual brainstorming companion server for Claude Code", | ||
| "main": "index.js", | ||
| "dependencies": { | ||
| "chokidar": "^3.5.3", | ||
| "express": "^4.18.2", | ||
| "ws": "^8.14.2" | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
chokidar 3.5.3 security vulnerabilities CVE
💡 Result:
No CVEs are currently mapped as direct vulnerabilities in chokidar@3.5.3 in major vulnerability databases (i.e., no “package chokidar is vulnerable” CVE entries for that version). Snyk reports no direct vulnerabilities for chokidar (including the 3.x line). [1] Meterian likewise lists no known vulnerabilities for chokidar@3.5.3. [2]
Notes that often cause confusion:
- Older chokidar versions could pull in vulnerable dependencies (example: CVE-2020-28469 via
glob-parent), and upgrading away from older chokidar was cited as mitigation. [3] - Separate from CVEs, there have been typosquatting/malicious packages impersonating chokidar (e.g., look-alike package names). These are supply-chain threats, not CVEs in the legit
chokidarpackage. [4]
Sources: Snyk chokidar package page [1]; Meterian chokidar@3.5.3 page [2]; Drupal issue referencing CVE-2020-28469 via dependency chain [3]; Socket research on chokidar typosquatting packages [4].
🌐 Web query:
express 4.18.2 security vulnerabilities CVE
💡 Result:
Express 4.18.2 is affected by these published CVEs (directly and via its common bundled dependencies):
Express (core)
- CVE-2024-29041 – open redirect via malformed URLs in
res.location()/res.redirect()(affected < 4.19.2). Fix: upgrade to ≥ 4.19.2. [1] - CVE-2024-43796 – XSS / untrusted code execution risk when passing user input to
response.redirect()(affected < 4.20.0). Fix: upgrade to ≥ 4.20.0. [2]
Dependencies typically shipped with Express 4.18.x
- CVE-2024-43799 –
senduntrusted input toSendStream.redirect()(patched in send 0.19.0). Express 4.x updates this via later releases. [3] - CVE-2024-43800 –
serve-staticuntrusted input toredirect()(patched in serve-static 1.16.0). Express 4.x updates this via later releases. [4] - CVE-2024-45296 –
path-to-regexpReDoS (Express recommends upgrading the module; later Express 4.x releases update it). [5][6] - CVE-2024-47764 –
cookiecookie-field injection via crafted name/path/domain (patched in cookie 0.7.0; Express 4.21.1 updatescookie). [7][6]
Practical remediation
- If you must stay on Express 4.x, upgrade to the latest 4.x listed on Express’ security updates page (it includes dependency bumps for
send,serve-static,cookie, andpath-to-regexp). [6]
Sources: [1] [2] [3] [4] [5] [6] [7]
🌐 Web query:
ws 8.14.2 security vulnerabilities CVE
💡 Result:
For the Node.js ws (websockets/ws) library, version 8.14.2 falls in the affected range for:
- CVE-2024-37890 — Denial of Service (DoS): a request with too many HTTP headers (exceeding
server.maxHeadersCount) can crash awsserver. Affected versions includews>= 8.0.0 and < 8.17.1, so 8.14.2 is vulnerable. Fix: upgrade tows8.17.1+ (or use backports for older major versions). Sources: NVD and vendor advisory-derived databases. [1] [2]
Sources:
[1] https://nvd.nist.gov/vuln/detail/CVE-2024-37890
[2] https://cve.armis.com/cve-2024-37890
🏁 Script executed:
#!/bin/bash
# Check npm registry for current versions and basic info
echo "=== Checking npm registry for package information ==="
for pkg in "chokidar" "express" "ws"; do
echo ""
echo "--- $pkg ---"
npm view "$pkg" --json 2>/dev/null | jq '.version' 2>/dev/null || echo "Could not fetch from npm"
done
Repository: obra/superpowers
Length of output: 185
Upgrade express and ws for security fixes; add "private": true.
The package.json has known security vulnerabilities:
- express 4.18.2: affected by CVE-2024-29041 (open redirect), CVE-2024-43796 (XSS), and dependency vulnerabilities. Upgrade to ≥ 4.20.0 (or latest 4.x/5.x).
- ws 8.14.2: affected by CVE-2024-37890 (DoS via excessive HTTP headers). Upgrade to ≥ 8.17.1.
- chokidar 3.5.3: no known vulnerabilities.
Also add "private": true to prevent accidental npm publish of this internal tool.
🤖 Prompt for AI Agents
In `@lib/brainstorm-server/package.json` around lines 1 - 11, Update package.json
to set "private": true at the top level and bump the dependency versions: change
"express" to a version >= 4.20.0 (or latest 4.x/5.x) and "ws" to >= 8.17.1 in
the "dependencies" object (leave "chokidar" unchanged); after editing
package.json run npm install (or yarn install) to update
package-lock.json/yarn.lock and verify no breaking changes.
| # Wait for server-started message (check log file) | ||
| for i in {1..50}; do | ||
| if grep -q "server-started" "$LOG_FILE" 2>/dev/null; then | ||
| # Extract and output the server-started line | ||
| grep "server-started" "$LOG_FILE" | head -1 | ||
| exit 0 | ||
| fi | ||
| sleep 0.1 | ||
| done | ||
|
|
||
| # Timeout - server didn't start | ||
| echo '{"error": "Server failed to start within 5 seconds"}' | ||
| exit 1 |
There was a problem hiding this comment.
Clean up background server on startup timeout.
On timeout, the script exits but leaves the background server running and the temp dir behind, which can leak processes/ports across runs.
🔧 Proposed fix
# Timeout - server didn't start
-echo '{"error": "Server failed to start within 5 seconds"}'
-exit 1
+if kill -0 "$SERVER_PID" 2>/dev/null; then
+ kill "$SERVER_PID" 2>/dev/null
+fi
+rm -f "$PID_FILE"
+rm -rf "$SCREEN_DIR"
+echo '{"error": "Server failed to start within 5 seconds"}'
+exit 1
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Wait for server-started message (check log file) | |
| for i in {1..50}; do | |
| if grep -q "server-started" "$LOG_FILE" 2>/dev/null; then | |
| # Extract and output the server-started line | |
| grep "server-started" "$LOG_FILE" | head -1 | |
| exit 0 | |
| fi | |
| sleep 0.1 | |
| done | |
| # Timeout - server didn't start | |
| echo '{"error": "Server failed to start within 5 seconds"}' | |
| exit 1 | |
| # Wait for server-started message (check log file) | |
| for i in {1..50}; do | |
| if grep -q "server-started" "$LOG_FILE" 2>/dev/null; then | |
| # Extract and output the server-started line | |
| grep "server-started" "$LOG_FILE" | head -1 | |
| exit 0 | |
| fi | |
| sleep 0.1 | |
| done | |
| # Timeout - server didn't start | |
| if kill -0 "$SERVER_PID" 2>/dev/null; then | |
| kill "$SERVER_PID" 2>/dev/null | |
| fi | |
| rm -f "$PID_FILE" | |
| rm -rf "$SCREEN_DIR" | |
| echo '{"error": "Server failed to start within 5 seconds"}' | |
| exit 1 |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 34-34: i appears unused. Verify use (or export if used externally).
(SC2034)
🤖 Prompt for AI Agents
In `@lib/brainstorm-server/start-server.sh` around lines 33 - 45, The timeout
branch currently exits without cleaning up the background server and temporary
directory; capture the background server PID when launching it (e.g., assign $!
to SERVER_PID where the server is started) and on timeout kill that PID (first
try kill "$SERVER_PID" then fallback to kill -9 if needed), wait for it to exit,
and remove the temp directory (e.g., rm -rf "$TEMP_DIR" or "$TMP_DIR") before
printing the JSON error and exiting with status 1; also handle cases where
SERVER_PID or the temp-dir variable may be empty to avoid errors.
| const SERVER_PATH = path.join(__dirname, '../../lib/brainstorm-server/index.js'); | ||
| const TEST_PORT = 3334; | ||
| const TEST_SCREEN = '/tmp/brainstorm-test/screen.html'; |
There was a problem hiding this comment.
Server env var mismatch + missing screen directory.
The server reads BRAINSTORM_DIR, but the test sets BRAINSTORM_SCREEN and writes into a directory that isn’t created. This makes the reload test fail or throw ENOENT.
🔧 Proposed fix
-const TEST_SCREEN = '/tmp/brainstorm-test/screen.html';
+const TEST_SCREEN = '/tmp/brainstorm-test/screen.html';
+const TEST_SCREEN_DIR = path.dirname(TEST_SCREEN);
@@
- const server = spawn('node', [SERVER_PATH], {
- env: { ...process.env, BRAINSTORM_PORT: TEST_PORT, BRAINSTORM_SCREEN: TEST_SCREEN }
- });
+ fs.mkdirSync(TEST_SCREEN_DIR, { recursive: true });
+ const server = spawn('node', [SERVER_PATH], {
+ env: { ...process.env, BRAINSTORM_PORT: TEST_PORT, BRAINSTORM_DIR: TEST_SCREEN_DIR }
+ });
@@
- fs.writeFileSync(TEST_SCREEN, '<html><body>Updated</body></html>');
+ fs.writeFileSync(TEST_SCREEN, '<html><body>Updated</body></html>');
Also applies to: 37-39, 87-89
🤖 Prompt for AI Agents
In `@tests/brainstorm-server/server.test.js` around lines 8 - 10, The test is
setting BRAINSTORM_SCREEN and a TEST_SCREEN path but the server expects
BRAINSTORM_DIR and the test does not create the screen directory, causing
ENOENT; update the test to export the correct env var (set
process.env.BRAINSTORM_DIR to the directory portion of TEST_SCREEN or adjust
SERVER_PATH usage) and ensure the test creates the directory for TEST_SCREEN
before any file writes (mkdir -p semantics) and clean it up after; apply the
same changes where TEST_SCREEN/port/env are used (the other occurrences around
the blocks referenced) so the server reads the right env and the directory
exists during reload tests.
| echo "================================================================================" | ||
| cd "$SCRIPT_DIR/../.." && timeout 120 claude -p "$PROMPT" --permission-mode bypassPermissions 2>&1 | tee "$OUTPUT_FILE" || { | ||
| echo "" | ||
| echo "================================================================================" | ||
| echo "EXECUTION FAILED (exit code: $?)" | ||
| exit 1 | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n tests/claude-code/test-document-review-system.sh | sed -n '85,105p'
Repository: obra/superpowers
Length of output: 984
Capture the failing exit code before running echo statements.
$? on line 96 reflects the exit code of the preceding echo command (which succeeds with 0), not the failed command on line 93. This logs "EXECUTION FAILED (exit code: 0)" regardless of the actual failure status.
🔧 Proposed fix
cd "$SCRIPT_DIR/../.." && timeout 120 claude -p "$PROMPT" --permission-mode bypassPermissions 2>&1 | tee "$OUTPUT_FILE" || {
+ status=$?
echo ""
echo "================================================================================"
- echo "EXECUTION FAILED (exit code: $?)"
+ echo "EXECUTION FAILED (exit code: $status)"
exit 1
}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| echo "================================================================================" | |
| cd "$SCRIPT_DIR/../.." && timeout 120 claude -p "$PROMPT" --permission-mode bypassPermissions 2>&1 | tee "$OUTPUT_FILE" || { | |
| echo "" | |
| echo "================================================================================" | |
| echo "EXECUTION FAILED (exit code: $?)" | |
| exit 1 | |
| } | |
| echo "================================================================================" | |
| cd "$SCRIPT_DIR/../.." && timeout 120 claude -p "$PROMPT" --permission-mode bypassPermissions 2>&1 | tee "$OUTPUT_FILE" || { | |
| status=$? | |
| echo "" | |
| echo "================================================================================" | |
| echo "EXECUTION FAILED (exit code: $status)" | |
| exit 1 | |
| } |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 96-96: This $? refers to echo/printf, not a previous command. Assign to variable to avoid it being overwritten.
(SC2320)
🤖 Prompt for AI Agents
In `@tests/claude-code/test-document-review-system.sh` around lines 92 - 98, The
failing exit code of the pipeline is overwritten by the subsequent echo, so
capture the exit status immediately after running the command and before any
other commands; assign the exit status of the `timeout 120 claude -p "$PROMPT"
--permission-mode bypassPermissions 2>&1 | tee "$OUTPUT_FILE"` pipeline (i.e.,
`$?`) to a local variable (e.g., EXIT_CODE) right after it fails, then use that
variable in the `echo "EXECUTION FAILED (exit code: $EXIT_CODE)"` and the `exit
$EXIT_CODE` calls; reference the pipeline invocation and the `$?` usage in your
fix.
Create the initial server for the visual brainstorming companion: - Express server with WebSocket support for browser communication - File watcher (chokidar) to detect screen.html changes - Auto-injects helper.js into served HTML for event capture - Binds to localhost only (127.0.0.1) for security - Outputs JSON events to stdout for Claude consumption
The spread operator order was causing incoming event types to overwrite the user-event type marker.
- Add sendToClaude() function to browser helper that shows confirmation - Add wait-for-event.sh script for watching server output (tail -f | grep -m 1) - Enables clean event-driven loop: background bash waits for event, completion triggers Claude's turn
Adds browser-based mockup display to replace ASCII art during brainstorming sessions. Key components: - Frame template with OS-aware light/dark theming - CSS helpers for options, cards, mockups, split views - Server lifecycle scripts (start/stop with random high port) - Event watcher using tail+grep for feedback loop - Claude instructions for using the visual companion The skill now asks users if they want browser mockups and only runs in Claude Code environments.
- Each session gets unique temp directory (/tmp/brainstorm-{pid}-{timestamp})
- Server outputs screen_dir and screen_file in startup JSON
- stop-server.sh takes screen_dir arg and cleans up session directory
- Document blocking TaskOutput pattern: 10-min timeouts, retry up to 3x,
then prompt user "let me know when you want to continue"
- New show-and-wait.sh combines write + wait into one command - Uses polling instead of tail -f (which hangs on macOS) - Docs updated: start watcher BEFORE writing screen to avoid race - Reduces terminal noise by consolidating operations
Scripts: - Rename show-and-wait.sh -> wait-for-feedback.sh (just waits, no HTML piping) - Remove wait-for-event.sh (used hanging tail -f) - Workflow now: Write tool for HTML, wait-for-feedback.sh to block Documentation rewrite: - Broader "when to use" (UI, architecture, complex choices, spatial) - Always ask user first before starting - Scale fidelity to the question being asked - Explain the question on each page - Iterate before moving on - validate changes address feedback - Use real content (Unsplash images) when it matters
- Never use cat/heredoc for HTML (dumps noise into terminal) - Read screen_file first before Write tool to avoid errors - Remind user of URL on every step, not just first - Give text summary of what's on screen before they look
Server now watches directory for new .html files instead of a single screen file. Claude writes to semantically named files like platform.html, style.html, layout.html - each screen is a new file. Benefits: - No need to read before write (files are always new) - Semantic filenames describe what's on screen - History preserved in directory for debugging - Server serves newest file by mtime automatically Updated: index.js, start-server.sh, and all documentation.
Clarifies that user instructions (CLAUDE.md, direct requests) always take precedence over Superpowers skills, which in turn override default system prompt behavior. Ensures users remain in control. Also updates RELEASE-NOTES.md with unreleased changes including the visual companion feature.
* fix use_skill agent context (#290) * fix: respect OPENCODE_CONFIG_DIR for personal skills lookup (#297) * fix: respect OPENCODE_CONFIG_DIR for personal skills lookup The plugin was hardcoded to look for personal skills in ~/.config/opencode/skills, ignoring users who set OPENCODE_CONFIG_DIR to a custom path (e.g., for dotfiles management). Now uses OPENCODE_CONFIG_DIR if set, falling back to the default path. * fix: update help text to use dynamic paths Use configDir and personalSkillsDir variables in help text so paths are accurate when OPENCODE_CONFIG_DIR is set. * fix: normalize OPENCODE_CONFIG_DIR before use Handle edge cases where the env var might be: - Empty or whitespace-only - Using ~ for home directory (common in .env files) - A relative path Now trims, expands ~, and resolves to absolute path. * feat(opencode): use native skills and fix agent reset bug (#226) - Replace custom use_skill/find_skills tools with OpenCode's native skill tool - Use experimental.chat.system.transform hook instead of session.prompt (fixes #226 agent reset on first message) - Symlink skills directory into ~/.config/opencode/skills/superpowers/ - Update installation docs with comprehensive Windows support: - Command Prompt, PowerShell, and Git Bash instructions - Proper symlink vs junction handling - Reinstall safety with cleanup steps - Verification commands for each shell * Add OpenCode native skills changes to release notes Documents: - Breaking change: switch to native skill tool - Fix for agent reset bug (#226) - Fix for Windows installation (#232) --------- Co-authored-by: Vinicius da Motta <viniciusmotta8@gmail.com> Co-authored-by: oribi <oribarilan@gmail.com>
* fix: convert shell scripts from CRLF to LF line endings Add .gitattributes to enforce LF line endings for shell scripts, preventing bash errors like "/usr/bin/bash: line 1: : command not found" when scripts are checked out on Windows with CRLF. Fixes #317 (SessionStart hook fails due to CRLF line endings) Files converted: - hooks/session-start.sh - lib/brainstorm-server/start-server.sh - lib/brainstorm-server/stop-server.sh - lib/brainstorm-server/wait-for-feedback.sh - skills/systematic-debugging/find-polluter.sh Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: update Windows hook execution for Claude Code 2.1.x Claude Code 2.1.x changed the Windows execution model: it now auto-detects .sh files in hook commands and prepends "bash " automatically. This broke the polyglot wrapper because: Before: "run-hook.cmd" session-start.sh (wrapper executes) After: bash "run-hook.cmd" session-start.sh (bash can't run .cmd) Changes: - hooks.json now calls session-start.sh directly (Claude Code handles bash) - Added deprecation comment to run-hook.cmd explaining the change - Updated RELEASE-NOTES.md Fixes #317, #313, #275, #292 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
- Specs (brainstorming output) now go to docs/superpowers/specs/ - Plans (writing-plans output) now go to docs/superpowers/plans/ - User preferences for locations override these defaults - Update all skill references and test files Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Make writing-plans REQUIRED after design approval - Explicitly forbid platform planning features (EnterPlanMode, etc.) - Forbid direct implementation without writing-plans skill Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Subagent-driven-development is now mandatory when harness supports it - No longer offer choice between subagent-driven and executing-plans - Executing-plans reserved for harnesses without subagent capability - Update plan header to reference both execution paths Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Spec: docs/superpowers/specs/2026-01-22-document-review-system-design.md - Plan: docs/superpowers/plans/2026-01-22-document-review-system.md Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The `- [ ] ### Task N:` syntax was unusual and might not render correctly in all markdown parsers. Now only steps have checkboxes. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Tests verify: - Spec document reviewer checks (completeness, TODOs) - Plan document reviewer checks (spec alignment, task decomposition) - Review loops exist in brainstorming and writing-plans skills - Chunk-by-chunk review for plans with 1000-line limit - Iteration guidance (5 iterations, escalate to human) - Checkbox syntax on steps only (not task headings) - Correct directories (docs/superpowers/specs, docs/superpowers/plans) - Reviewers are advisory - Same agent fixes issues (preserves context) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Creates test project with spec containing intentional errors - Runs Claude to actually review using spec-document-reviewer template - Verifies reviewer catches TODO and "specified later" deferrals - Checks review format and verdict Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@docs/plans/2026-01-17-visual-brainstorming.md`:
- Around line 37-122: The snippet uses a single SCREEN_FILE and watches it,
which is outdated; update it to use SCREEN_DIR, watch the directory for 'add'
and 'change' events, and replace the simple file-serving logic with a
getNewestScreen() implementation that picks the newest .html by mtime and serves
that file; specifically, remove references to SCREEN_FILE usage, switch to
SCREEN_DIR and SCREEN_DIR env var handling, add a getNewestScreen() helper used
by the express route, and update the chokidar watcher to watch SCREEN_DIR and
handle both additions and changes so behavior matches the current implementation
in getNewestScreen and the directory-watching logic.
In `@lib/brainstorm-server/start-server.sh`:
- Around line 20-25: The cleanup block using PID_FILE derived from SESSION_ID
(in start-server.sh) is ineffective because SESSION_ID is unique per run; change
the cleanup to target a stable PID location or scan for stale PID files: either
set PID_FILE to a fixed path (e.g., /tmp/brainstorm-server.pid) and use that in
the existing removal/killing logic, or implement a scan that iterates
/tmp/brainstorm-*/ directories to read and validate each PID file and kill stale
PIDs before removing them; update references to PID_FILE and SESSION_ID
accordingly and ensure the new logic safely checks process liveness before kill
to avoid false positives.
- Line 28: The script currently runs cd "$SCRIPT_DIR" without checking if it
succeeded, so if the chdir fails the subsequent node index.js runs from the
wrong directory; update the start-up logic to test the exit status of cd (or use
a conditional like cd "$SCRIPT_DIR" || { ... }) and on failure log an error
mentioning SCRIPT_DIR and exit with a non-zero status before invoking node
index.js so the server never starts from the wrong location.
In `@RELEASE-NOTES.md`:
- Around line 26-30: The unreleased changelog contains a duplicate breaking
change entry "OpenCode: Switched to native skills system" that already appears
in the v4.1.0 section; remove the duplicate from the unreleased section (or mark
it explicitly as intentionally carried forward) so the unreleased section only
lists new, unreleased changes; locate the entry by searching for the exact
heading/text "OpenCode: Switched to native skills system" in RELEASE-NOTES.md
and either delete that block under the Unreleased heading or add a clear note
that it’s intentionally repeated.
♻️ Duplicate comments (3)
lib/brainstorm-server/package.json (1)
2-9: Duplicate: verify dependency versions and consider"private": true.Same as the earlier review note: the versions at Line 6–9 should be verified for current advisories/updates, and adding
"private": truewould prevent accidental publish.Are there any published security advisories affecting express 4.18.2 or ws 8.14.2, and what are the latest secure versions in their current major lines?lib/brainstorm-server/start-server.sh (1)
43-45: Clean up background server on startup timeout.On timeout, the script exits but leaves the background server running and the temp directory behind. This can leak processes and ports across runs.
🔧 Proposed fix
# Timeout - server didn't start +if kill -0 "$SERVER_PID" 2>/dev/null; then + kill "$SERVER_PID" 2>/dev/null +fi +rm -f "$PID_FILE" +rm -rf "$SCREEN_DIR" echo '{"error": "Server failed to start within 5 seconds"}' exit 1tests/claude-code/test-document-review-system.sh (1)
92-98: Capture the failing exit code before running echo statements.
$?on line 96 reflects the exit code of the precedingechocommand (which succeeds with 0), not the failed command on line 93. This logs "EXECUTION FAILED (exit code: 0)" regardless of the actual failure status.🔧 Proposed fix
-cd "$SCRIPT_DIR/../.." && timeout 120 claude -p "$PROMPT" --permission-mode bypassPermissions 2>&1 | tee "$OUTPUT_FILE" || { +cd "$SCRIPT_DIR/../.." && timeout 120 claude -p "$PROMPT" --permission-mode bypassPermissions 2>&1 | tee "$OUTPUT_FILE" || { + status=$? echo "" echo "================================================================================" - echo "EXECUTION FAILED (exit code: $?)" + echo "EXECUTION FAILED (exit code: $status)" exit 1 }
🧹 Nitpick comments (6)
lib/brainstorm-server/stop-server.sh (1)
14-19: Validate path beforerm -rfand verify process termination.The script removes
$SCREEN_DIRbased on user input without path validation. Consider:
- Verifying
SCREEN_DIRstarts with/tmp/brainstorm-to prevent accidental deletion of unrelated directories- Waiting briefly for the process to terminate before cleanup
🔧 Proposed fix
if [[ -f "$PID_FILE" ]]; then pid=$(cat "$PID_FILE") + # Validate SCREEN_DIR is a brainstorm temp directory + if [[ ! "$SCREEN_DIR" =~ ^/tmp/brainstorm- ]]; then + echo '{"error": "Invalid screen directory path"}' + exit 1 + fi kill "$pid" 2>/dev/null + # Brief wait for process to terminate + sleep 0.1 # Clean up session directory rm -rf "$SCREEN_DIR" echo '{"status": "stopped"}'skills/writing-plans/plan-document-reviewer-prompt.md (1)
40-40: Minor inconsistency with spec reviewer status format.The status format here (
Approved | Issues Found) differs from the spec reviewer template which uses emoji (✅ Approved | ❌ Issues Found). Consider aligning the formats for consistency across reviewer outputs.🔧 Proposed fix
- **Status:** Approved | Issues Found + **Status:** ✅ Approved | ❌ Issues Foundtests/claude-code/test-document-review-system.sh (1)
141-150: Test 4 logic has ambiguous success path.Lines 148-149 report
[PASS]with message "ambiguous format but found issues" when neither the explicit rejection patterns (Issues Found,❌,not approved) nor the explicit approval patterns (Approved,✅without issues) are matched. This could mask cases where the reviewer output is malformed or doesn't follow the expected template at all.Consider tightening the verification to require explicit issue detection rather than falling through to a pass:
🔧 Proposed fix
# Test 4: Reviewer did NOT approve (found issues) echo "Test 4: Reviewer verdict..." if grep -qi "Issues Found\|❌\|not approved\|issues found" "$OUTPUT_FILE"; then echo " [PASS] Reviewer correctly found issues (not approved)" elif grep -qi "Approved\|✅" "$OUTPUT_FILE" && ! grep -qi "Issues Found\|❌" "$OUTPUT_FILE"; then echo " [FAIL] Reviewer incorrectly approved spec with errors" FAILED=$((FAILED + 1)) else - echo " [PASS] Reviewer identified problems (ambiguous format but found issues)" + echo " [WARN] Reviewer output format unclear - manual verification needed" + echo " Neither explicit approval nor explicit rejection found" + # Don't fail, but flag for attention fidocs/superpowers/plans/2026-01-22-document-review-system.md (1)
26-75: Verify the nested code block structure in the template.The embedded template contains a markdown code fence (lines 26-35) followed by the actual prompt content (lines 36-71) followed by another code fence marker (line 72). This nested structure might be confusing when implementing.
Consider clarifying whether:
- The entire content (lines 27-74) should be written to
spec-document-reviewer-prompt.mdas-is- The Task tool prompt is the section from lines 38-71
The current structure mixes the template documentation with the actual prompt content, which could lead to implementation errors.
💡 Suggested clarification
Consider restructuring to clearly separate:
- What the markdown file should contain (the template)
- What the Task tool prompt should be (extracted from the template)
For example:
- [ ] **Step 1:** Create the reviewer prompt template file The file should contain: - Purpose and dispatch timing - A template showing how to use the Task tool - The actual prompt text to send to the reviewer subagent Create `skills/brainstorming/spec-document-reviewer-prompt.md` with this content: ```markdown [template content here]</details> </blockquote></details> <details> <summary>docs/plans/2026-01-17-visual-brainstorming.md (2)</summary><blockquote> `545-545`: **Add language specifier to code fence.** The code fence at line 545 (showing .gitignore content) should specify a language for better syntax highlighting and markdown compliance. <details> <summary>💡 Suggested fix</summary> ```diff -``` +```gitignore lib/brainstorm-server/node_modules/</details> --- `1-11`: **Update the execution requirement header to match the newer pattern.** Line 3 uses "REQUIRED SUB-SKILL: Use superpowers:executing-plans" which is inconsistent with the newer pattern established in the document-review-system plan. Consider updating to the more flexible pattern: ```markdown > **For Claude:** REQUIRED: Use superpowers:subagent-driven-development (if subagents available) or superpowers:executing-plans to implement this plan.This newer pattern supports both execution modes and is already adopted in the document-review-system plan (2026-01-22).
| ```javascript | ||
| const express = require('express'); | ||
| const http = require('http'); | ||
| const WebSocket = require('ws'); | ||
| const chokidar = require('chokidar'); | ||
| const fs = require('fs'); | ||
| const path = require('path'); | ||
|
|
||
| const PORT = process.env.BRAINSTORM_PORT || 3333; | ||
| const SCREEN_FILE = process.env.BRAINSTORM_SCREEN || '/tmp/brainstorm/screen.html'; | ||
| const SCREEN_DIR = path.dirname(SCREEN_FILE); | ||
|
|
||
| // Ensure screen directory exists | ||
| if (!fs.existsSync(SCREEN_DIR)) { | ||
| fs.mkdirSync(SCREEN_DIR, { recursive: true }); | ||
| } | ||
|
|
||
| // Create default screen if none exists | ||
| if (!fs.existsSync(SCREEN_FILE)) { | ||
| fs.writeFileSync(SCREEN_FILE, `<!DOCTYPE html> | ||
| <html> | ||
| <head> | ||
| <title>Brainstorm Companion</title> | ||
| <style> | ||
| body { font-family: system-ui, sans-serif; padding: 2rem; max-width: 800px; margin: 0 auto; } | ||
| h1 { color: #333; } | ||
| p { color: #666; } | ||
| </style> | ||
| </head> | ||
| <body> | ||
| <h1>Brainstorm Companion</h1> | ||
| <p>Waiting for Claude to push a screen...</p> | ||
| </body> | ||
| </html>`); | ||
| } | ||
|
|
||
| const app = express(); | ||
| const server = http.createServer(app); | ||
| const wss = new WebSocket.Server({ server }); | ||
|
|
||
| // Track connected browsers for reload notifications | ||
| const clients = new Set(); | ||
|
|
||
| wss.on('connection', (ws) => { | ||
| clients.add(ws); | ||
| ws.on('close', () => clients.delete(ws)); | ||
|
|
||
| ws.on('message', (data) => { | ||
| // User interaction event - write to stdout for Claude | ||
| const event = JSON.parse(data.toString()); | ||
| console.log(JSON.stringify({ type: 'user-event', ...event })); | ||
| }); | ||
| }); | ||
|
|
||
| // Serve current screen with helper.js injected | ||
| app.get('/', (req, res) => { | ||
| let html = fs.readFileSync(SCREEN_FILE, 'utf-8'); | ||
|
|
||
| // Inject helper script before </body> | ||
| const helperScript = fs.readFileSync(path.join(__dirname, 'helper.js'), 'utf-8'); | ||
| const injection = `<script>\n${helperScript}\n</script>`; | ||
|
|
||
| if (html.includes('</body>')) { | ||
| html = html.replace('</body>', `${injection}\n</body>`); | ||
| } else { | ||
| html += injection; | ||
| } | ||
|
|
||
| res.type('html').send(html); | ||
| }); | ||
|
|
||
| // Watch for screen file changes | ||
| chokidar.watch(SCREEN_FILE).on('change', () => { | ||
| console.log(JSON.stringify({ type: 'screen-updated', file: SCREEN_FILE })); | ||
| // Notify all browsers to reload | ||
| clients.forEach(ws => { | ||
| if (ws.readyState === WebSocket.OPEN) { | ||
| ws.send(JSON.stringify({ type: 'reload' })); | ||
| } | ||
| }); | ||
| }); | ||
|
|
||
| server.listen(PORT, '127.0.0.1', () => { | ||
| console.log(JSON.stringify({ type: 'server-started', port: PORT, url: `http://localhost:${PORT}` })); | ||
| }); | ||
| ``` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if lib/brainstorm-server/index.js exists and examine its structure
find . -name "index.js" -path "*/brainstorm*" -type f 2>/dev/null | head -20
Repository: obra/superpowers
Length of output: 92
🏁 Script executed:
# Check for visual-companion.md reference document
find . -name "*visual-companion*" -type f 2>/dev/null
Repository: obra/superpowers
Length of output: 102
🏁 Script executed:
# Check the structure of the docs/plans directory
ls -la docs/plans/ 2>/dev/null || echo "docs/plans directory not found"
Repository: obra/superpowers
Length of output: 545
🏁 Script executed:
# Read the visual-companion.md reference document to verify the claims
cat ./skills/brainstorming/visual-companion.md
Repository: obra/superpowers
Length of output: 3957
🏁 Script executed:
# Read the actual implementation
cat ./lib/brainstorm-server/index.js
Repository: obra/superpowers
Length of output: 3443
🏁 Script executed:
# Check the visual-brainstorming.md file to verify the snippet shown
cat ./docs/plans/2026-01-17-visual-brainstorming.md
Repository: obra/superpowers
Length of output: 15159
Update the code snippet to match the actual server implementation.
The planning document shows an outdated implementation that uses a single SCREEN_FILE and doesn't support the "serve newest file by modification time" behavior described in the reference docs. The actual implementation in lib/brainstorm-server/index.js correctly:
- Watches a
SCREEN_DIRinstead of a single file - Implements
getNewestScreen()to serve the newest .html by modification time - Watches for both file additions and changes in the directory
- Uses
SCREEN_DIRenvironment variable instead ofSCREEN_FILE
Replace the code snippet with the current implementation to match the reference documentation and actual codebase.
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
83-83: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In `@docs/plans/2026-01-17-visual-brainstorming.md` around lines 37 - 122, The
snippet uses a single SCREEN_FILE and watches it, which is outdated; update it
to use SCREEN_DIR, watch the directory for 'add' and 'change' events, and
replace the simple file-serving logic with a getNewestScreen() implementation
that picks the newest .html by mtime and serves that file; specifically, remove
references to SCREEN_FILE usage, switch to SCREEN_DIR and SCREEN_DIR env var
handling, add a getNewestScreen() helper used by the express route, and update
the chokidar watcher to watch SCREEN_DIR and handle both additions and changes
so behavior matches the current implementation in getNewestScreen and the
directory-watching logic.
| # Kill any existing server | ||
| if [[ -f "$PID_FILE" ]]; then | ||
| old_pid=$(cat "$PID_FILE") | ||
| kill "$old_pid" 2>/dev/null | ||
| rm -f "$PID_FILE" | ||
| fi |
There was a problem hiding this comment.
Old server cleanup logic is ineffective.
This block attempts to kill any existing server, but PID_FILE is derived from SESSION_ID which is unique per invocation ($$-$(date +%s)). The PID file will never exist for a new session, so this cleanup is a no-op. If the intent is to clean up orphaned servers from crashed sessions, consider either:
- Using a fixed PID file location (e.g.,
/tmp/brainstorm-server.pid) - Or scanning for any
/tmp/brainstorm-*/directories with stale PID files
🤖 Prompt for AI Agents
In `@lib/brainstorm-server/start-server.sh` around lines 20 - 25, The cleanup
block using PID_FILE derived from SESSION_ID (in start-server.sh) is ineffective
because SESSION_ID is unique per run; change the cleanup to target a stable PID
location or scan for stale PID files: either set PID_FILE to a fixed path (e.g.,
/tmp/brainstorm-server.pid) and use that in the existing removal/killing logic,
or implement a scan that iterates /tmp/brainstorm-*/ directories to read and
validate each PID file and kill stale PIDs before removing them; update
references to PID_FILE and SESSION_ID accordingly and ensure the new logic
safely checks process liveness before kill to avoid false positives.
| fi | ||
|
|
||
| # Start server, capturing output to log file | ||
| cd "$SCRIPT_DIR" |
There was a problem hiding this comment.
Handle cd failure.
If cd "$SCRIPT_DIR" fails, the script continues and runs node index.js from the wrong directory.
🔧 Proposed fix
-cd "$SCRIPT_DIR"
+cd "$SCRIPT_DIR" || { echo '{"error": "Failed to change to script directory"}'; exit 1; }
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cd "$SCRIPT_DIR" | |
| cd "$SCRIPT_DIR" || { echo '{"error": "Failed to change to script directory"}'; exit 1; } |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 28-28: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
🤖 Prompt for AI Agents
In `@lib/brainstorm-server/start-server.sh` at line 28, The script currently runs
cd "$SCRIPT_DIR" without checking if it succeeded, so if the chdir fails the
subsequent node index.js runs from the wrong directory; update the start-up
logic to test the exit status of cd (or use a conditional like cd "$SCRIPT_DIR"
|| { ... }) and on failure log an error mentioning SCRIPT_DIR and exit with a
non-zero status before invoking node index.js so the server never starts from
the wrong location.
| **OpenCode: Switched to native skills system** | ||
|
|
||
| Superpowers for OpenCode now uses OpenCode's native `skill` tool instead of custom `use_skill`/`find_skills` tools. This is a cleaner integration that works with OpenCode's built-in skill discovery. | ||
|
|
||
| **Migration required:** Skills must be symlinked to `~/.config/opencode/skills/superpowers/` (see updated installation docs). |
There was a problem hiding this comment.
Duplicate breaking change entry with v4.1.0.
The "OpenCode: Switched to native skills system" breaking change (lines 26-30) duplicates content from v4.1.0 (lines 105-109). If this is intentionally carried forward to unreleased for emphasis, consider removing it since it's already released. Otherwise, if v4.1.0 changes were already deployed, the unreleased section should only contain new changes.
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
26-26: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🤖 Prompt for AI Agents
In `@RELEASE-NOTES.md` around lines 26 - 30, The unreleased changelog contains a
duplicate breaking change entry "OpenCode: Switched to native skills system"
that already appears in the v4.1.0 section; remove the duplicate from the
unreleased section (or mark it explicitly as intentionally carried forward) so
the unreleased section only lists new, unreleased changes; locate the entry by
searching for the exact heading/text "OpenCode: Switched to native skills
system" in RELEASE-NOTES.md and either delete that block under the Unreleased
heading or add a clear note that it’s intentionally repeated.
Summary
docs/superpowers/specs/, plans todocs/superpowers/plans/Document Review System
skills/brainstorming/spec-document-reviewer-prompt.md) - reviews specs after brainstormingskills/writing-plans/plan-document-reviewer-prompt.md) - reviews plans chunk-by-chunkKey Files
skills/brainstorming/SKILL.md- added spec review loopskills/writing-plans/SKILL.md- added plan review loop, chunk boundaries, checkbox syntax on stepstests/claude-code/test-document-review-system.sh- integration test that creates spec with errors and verifies reviewer catches themTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Updates
✏️ Tip: You can customize this high-level summary in your review settings.