fix(brainstorm-server): guard handleMessage against null event payload#1504
Open
ambicuity wants to merge 1 commit into
Open
fix(brainstorm-server): guard handleMessage against null event payload#1504ambicuity wants to merge 1 commit into
ambicuity wants to merge 1 commit into
Conversation
JSON.parse('null') yields null. Reading .choice on null throws a
TypeError that escapes the socket 'data' handler as an
uncaughtException, terminating the brainstorm server process. Any
local WebSocket client (browser tab, Node REPL, etc.) on
127.0.0.1:<port> can trigger this with the 4-byte message `null`.
Other JSON values are unaffected: primitives have a property access
of undefined (no throw), and objects/arrays already reach the
intended `if (event.choice)` semantics.
Adds a truthiness guard before the property access and three
regression tests covering JSON null, JSON primitives, and the
absence of an events-file write for null payloads.
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Guards the brainstorm WebSocket server against JSON.parse('null') payloads that previously crashed the process, and adds regression tests to prevent reintroducing the issue.
Changes:
- Add a null-check before accessing
event.choiceinhandleMessage() - Add regression tests ensuring
nulland primitive JSON payloads don’t crash the server - Add a regression test ensuring
nullpayloads don’t create a straystate/eventsfile
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| skills/brainstorming/scripts/server.cjs | Adds a guard before event.choice access to prevent null payload crashes |
| tests/brainstorm-server/server.test.js | Adds regression tests covering null, primitives, and preventing unintended events writes |
| @@ -231,7 +231,9 @@ function handleMessage(text) { | |||
| } | |||
| touchActivity(); | |||
| console.log(JSON.stringify({ source: 'user-event', ...event })); | |||
Comment on lines
+304
to
+309
| ws.send('null'); | ||
| await sleep(300); | ||
|
|
||
| const res = await fetch(`http://localhost:${TEST_PORT}/`); | ||
| assert.strictEqual(res.status, 200, 'Server should still be running after JSON null'); | ||
| ws.close(); |
5 tasks
5 tasks
3 tasks
ktenman
added a commit
to ktenman/superpowers
that referenced
this pull request
May 23, 2026
…#1596, obra#1562) Cherry-picks three verified fixes from obra/superpowers open PRs into the fork. - obra#1504: guard handleMessage against null/primitive WebSocket payloads. JSON.parse('null') returns null; reading .choice on it threw a TypeError that escaped the socket 'data' handler and killed the server process. - obra#1596: make the brainstorm server idle timeout configurable via --idle-timeout-minutes (BRAINSTORM_IDLE_TIMEOUT_MS) and raise the default from 30 minutes to 2 hours. Surface idle_timeout_ms in server-info. - obra#1562: detect the package manager by lockfile in using-git-worktrees Step 3 (pnpm/yarn/bun/npm for Node, uv/poetry/pip for Python) instead of running npm install / poetry install unconditionally. Verified: brainstorm-server suite 32/32 green (includes new null-crash and idle-timeout regression tests); --idle-timeout-minutes checked end-to-end (5 -> 300000ms, default -> 7200000ms, 0/abc -> error + exit 1).
obra
added
bug
|
Review note after reading the PR body and diff: This looks like a narrow runtime bug fix with real regression coverage. The failure mode is concrete ( I did not see a code-level blocker in the patch. The Process items for reviewers:
This seems substantially easier to evaluate than the larger skill-prose PRs because it changes only runtime code and focused tests. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem are you trying to solve?
The brainstorm WebSocket server in `skills/brainstorming/scripts/server.cjs` crashes — process exits with code 1 — when a client sends the 4-byte JSON message `null`. `JSON.parse('null')` returns `null`, and `null.choice` (line 234) throws `TypeError: Cannot read properties of null (reading 'choice')`. The throw escapes the `socket.on('data')` handler as an uncaughtException, terminating the server.
Any local WebSocket client can trigger this — a browser tab loading a malicious page, any local process, or simply a misbehaving extension that sends an unexpected payload. The bind is `127.0.0.1`, so the threat is local-only, but it's a real availability bug for anyone who has the brainstorm companion running.
Live repro on `main` (commit f2cbfbe, v5.1.0) — start the server and send `null` over a fresh WebSocket:
```
{"type":"server-started","port":3399,"host":"127.0.0.1",...}
{"source":"user-event"}
/private/tmp/.../skills/brainstorming/scripts/server.cjs:234
if (event.choice) {
^
TypeError: Cannot read properties of null (reading 'choice')
at handleMessage (.../server.cjs:234:13)
at Socket. (.../server.cjs:198:11)
at Socket.emit (node:events:509:20)
...
*** SERVER EXITED (code=1) ***
```
What does this PR change?
Adds a truthiness check (`event &&`) before the `event.choice` access in `handleMessage()`. Adds three regression tests in `tests/brainstorm-server/server.test.js` covering: JSON `null` does not crash the server, JSON primitives (number/string/bool) do not crash the server, and JSON `null` does not produce a stray `state/events` write.
Is this change appropriate for the core library?
Yes. `server.cjs` is the WebSocket server backing the `brainstorming` skill — core infrastructure used by every Superpowers user who uses the brainstorming visual companion. The fix is one operator (`&&`) plus a comment explaining why; no new dependency, no behavioral change to legitimate object payloads.
What alternatives did you consider?
`if (event && typeof event === 'object' && event.choice)` — broader guard rejecting primitives outright. Discarded as over-engineering: the only JSON values that throw on property access are `null` and `undefined`. `undefined` cannot be produced by `JSON.parse`. Primitives yield `undefined` for unknown properties (no throw) and are correctly ignored by the existing falsy check. The minimal `event && event.choice` precisely covers the crash without changing semantics for any valid JSON value.
Wrap `handleMessage` in a top-level `try/catch` — would also prevent the crash, but it would mask other latent bugs by silently swallowing errors. The narrow guard at the actual throw site is cleaner and the test fixture (`uncaughtException` would propagate any other bug) keeps future regressions visible.
Add a `process.on('uncaughtException')` handler — addresses the symptom (process exit) rather than the cause (unsafe property access on `null`). Discarded for the same reason as 2.
Does this PR contain multiple unrelated changes?
No. One bug, one fix, plus regression tests for that fix. Both files modified are the bug site and its test suite.
Existing PRs
Environment tested
OS: macOS 25.3.0 (Darwin). Node: v25.9.0. Tests run as `node tests/brainstorm-server/server.test.js` and `node tests/brainstorm-server/ws-protocol.test.js` — both standalone, no harness needed for the test itself.
New harness support (required if this PR adds a new harness)
N/A — this PR does not add a new harness. The change is internal to an existing skill's runtime script.
Evaluation
Initial trigger: A code review of `server.cjs` traced the `socket.on('data')` handler call chain and identified that `handleMessage` had no guard before `event.choice`. Wrote a minimal Node script that opens a WebSocket and sends `null`. Confirmed the server crashed with the stack trace shown above on a fresh `main` checkout (commit `f2cbfbe`, v5.1.0).
After the fix: ran `node tests/brainstorm-server/server.test.js` — 28 passed, 0 failed (3 new tests added: 25→28). Ran `node tests/brainstorm-server/ws-protocol.test.js` — 31 passed, 0 failed (no change, sanity check that frame encode/decode wasn't affected). Re-ran the live repro script: server stays alive, the WebSocket connection survives the `null` payload, subsequent HTTP `GET /` returns 200.
Negative control: before opening this PR, I temporarily reverted the one-line fix (`git stash` of `server.cjs`) and re-ran the test suite. The new `handles JSON null from client without crashing` test fails — the server crashes during the test, the test runner reports `FAIL`, and remaining tests cannot run. This proves the new test actually catches the bug it claims to catch (i.e., the test isn't tautological).
Rigor
Human review
The diff is two files (4-line code change in `server.cjs` including a 2-line WHY comment, and 47 lines of regression tests in `server.test.js` mirroring the existing `handles malformed JSON from client gracefully` test pattern at line 284). The diff was reviewed in its entirety before push, including the captured live-repro stack trace and the negative-control test.