Skip to content

fix: escape SessionStart JSON control characters#1590

Open
YOMXXX wants to merge 1 commit into
obra:devfrom
YOMXXX:fix/session-start-json-control-escaping
Open

fix: escape SessionStart JSON control characters#1590
YOMXXX wants to merge 1 commit into
obra:devfrom
YOMXXX:fix/session-start-json-control-escaping

Conversation

@YOMXXX
Copy link
Copy Markdown

@YOMXXX YOMXXX commented May 21, 2026

What problem are you trying to solve?

Issue #1446 reports three independent latent bugs. This PR fixes only the first one: hooks/session-start has a hand-written JSON string escaper that handles backslash, double quote, newline, carriage return, and tab, but leaves other C0 control characters raw.

Raw U+0000 through U+001F characters are not valid inside JSON strings. If skills/using-superpowers/SKILL.md ever contains a raw backspace, form feed, vertical tab, unit separator, or similar control character, the SessionStart hook emits invalid JSON and the harness cannot parse the payload. The likely user-facing failure is Superpowers not activating even though the plugin is installed.

hooks/session-start-codex carries the same escape helper, so it has the same failure mode.

What does this PR change?

Escapes every C0 control character that Bash strings can carry in both SessionStart hooks:

  • keeps short escapes for \b, \f, \n, \r, and \t
  • uses \u0001 through \u001f for the remaining representable controls
  • leaves hook output shape unchanged

It also extends the SessionStart hook tests with temporary plugin fixtures whose using-superpowers skill content contains U+0001 through U+001F, then parses the real hook output with JSON.parse.

Is this change appropriate for the core library?

Yes. SessionStart bootstrap output is core plugin infrastructure used before any skill can run. The fix is general-purpose, dependency-free, and applies to both the standard SessionStart hook and the Codex-specific SessionStart hook.

What alternatives did you consider?

  1. Use Python/Node/jq as a real JSON encoder. Rejected because the hook is intentionally shell-based and dependency-light, and adding runtime dependencies to SessionStart increases bootstrap fragility.
  2. Fix only hooks/session-start. Rejected because hooks/session-start-codex duplicates the same escape helper and would keep the latent bug.
  3. Address all three v5.0.7: three latent bugs found in static review (escape_for_json control-char gap, server.cjs payload-size DoS, find-polluter.sh unquoted loop) #1446 reports in one PR. Rejected because they are independent. The WebSocket payload-size issue already has open PR fix(brainstorming): cap WebSocket frame payloads #1555, and the find-polluter.sh path-splitting issue should be a separate narrow PR if pursued.

Does this PR contain multiple unrelated changes?

No. It only fixes SessionStart JSON escaping for control characters and adds a regression test/spec/plan for that one bug.

Existing PRs

#134 is prior art for Windows/plugin hook support. #1555 covers a different #1446 sub-issue: WebSocket payload-size caps in the brainstorm server. Searches for 1446 escape_for_json, escape_for_json control char, and related terms did not find a duplicate PR for this SessionStart JSON escaping fix.

Environment tested

Harness (e.g. Claude Code, Cursor) Harness version Model Model version/ID
Shell hook tests macOS zsh/bash n/a n/a
OpenCode plugin tests local test harness n/a n/a
Codex plugin sync tests local test harness n/a n/a

New harness support (required if this PR adds a new harness)

Not applicable. This PR does not add support for a new harness.

Clean-session transcript for "Let's make a react todo list"
N/A - no new harness support.

Evaluation

Verification commands run fresh before PR creation:

bash tests/hooks/test-session-start.sh
bash tests/opencode/run-tests.sh
bash tests/codex-plugin-sync/test-sync-to-codex-plugin.sh
bash -n hooks/session-start hooks/session-start-codex tests/hooks/test-session-start.sh
git diff --check HEAD~1 HEAD

All exited 0.

Rigor

  • If this is a skills change: I used superpowers:writing-skills and completed adversarial pressure testing (paste results below)
  • This change was tested adversarially, not just on the happy path
  • I did not modify carefully-tuned content (Red Flags table, rationalizations, "human partner" language) without extensive evals showing the change is an improvement

This is hook infrastructure, not a skill behavior change. The adversarial test uses a real hook output path with all representable C0 controls in the injected skill content and verifies JSON parsing plus decoded context preservation.

Human review

  • A human has reviewed the COMPLETE proposed diff before submission

Human partner reviewed the fork compare diff for YOMXXX:fix/session-start-json-control-escaping and explicitly approved opening the PR. They also gave standing approval to open future PRs after the complete diff and verification summary are shown.

@YOMXXX
Copy link
Copy Markdown
Author

YOMXXX commented May 21, 2026

Reviewer note

This PR is one of six small, independent PRs opened from the same triage pass. They can be reviewed and merged independently:

Focus for this PR: harden SessionStart JSON output so all representable C0 control characters in injected skill content are escaped before the harness parses the hook payload.

Review surface: hooks/session-start, hooks/session-start-codex, and hook tests. It keeps the hook output shape unchanged and does not add runtime dependencies.

Verification: hook tests use temporary skill fixtures containing U+0001 through U+001F and parse the real hook output with JSON.parse; OpenCode tests and Codex plugin sync tests also passed.

Risk to check: shell escaping correctness and whether both SessionStart variants stay in sync.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant