Fix JSON parsing errors in Claude Code hooks#34
Conversation
Fix critical JSON generation issues in git hooks that caused parsing failures
when processing Write/Edit operations with multiline content.
## Issues Fixed
### validate-agent-templates.sh
- Replaced manual JSON string construction with jq-based generation
- Lines 69 & 87 now use: `jq -n --arg msg "$MESSAGE" '{decision: "block", message: $msg}'`
- Fixes RFC 8259 violation (unescaped newlines in JSON strings)
### stop.sh
- Added JSON validation before processing: `if ! jq empty <<< "$INPUT"`
- Replaced `echo "$INPUT" | jq` with heredoc pattern `jq <<< "$INPUT"`
- Gracefully handles malformed JSON input
### settings.hooks.json
- Standardized hook matchers to consistent `Edit|Write` pattern
## Testing
Created comprehensive test suites:
- tests/test_validate_hook_json.sh - Validates JSON with multiline messages (3 tests)
- tests/test_hooks_e2e.sh - E2E validation for all hooks (8 tests)
- All tests pass ✅
## Documentation
- docs/hooks-json-parsing-errors.md - Root cause analysis with RFC 8259 references,
reproducible examples, and solution patterns
## Root Cause
Bash `echo` cannot escape newlines in JSON strings, violating RFC 8259 Section 7.
Using jq with `--arg` flag ensures proper escaping and RFC compliance.
## Impact
- Prevents hook failures when processing multiline content
- Ensures all hook output is valid JSON
- Adds regression tests to prevent future issues
The root cause analysis documentation was overly verbose for the codebase. The fix is straightforward and test suites provide sufficient validation.
Add test-hooks job to validate Claude Code hooks infrastructure: - tests/test_validate_hook_json.sh - validates JSON output with multiline content - tests/test_hooks_e2e.sh - E2E tests for all hooks (8 tests) Runs on ubuntu-latest after version validation, ensuring hooks work correctly before changes are merged.
There was a problem hiding this comment.
Pull Request Overview
This PR fixes JSON parsing errors in Claude Code hooks by replacing unsafe manual JSON string concatenation with proper jq commands and adds comprehensive test coverage to validate the fixes.
- Replaced manual JSON construction with
jq -n --argin validate-agent-templates.sh - Fixed JSON input parsing in stop.sh using heredoc syntax instead of echo piping
- Added input validation in stop.sh to handle malformed JSON gracefully
- Created extensive test suites to verify JSON handling with multiline content and edge cases
- Documented the root cause analysis of the original JSON parsing issues
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| .claude/hooks/validate-agent-templates.sh | Fixed JSON output generation to use jq for proper escaping of newlines and special characters |
| .claude/hooks/stop.sh | Added JSON input validation and replaced echo piping with heredoc syntax for safer jq processing |
| .claude/hooks/settings.hooks.json | Reordered matcher pattern from "Write|Edit" to "Edit|Write" for alphabetical consistency |
| tests/test_validate_hook_json.sh | Added test suite to verify JSON output validity with multiline messages and edge cases |
| tests/test_hooks_e2e.sh | Created comprehensive end-to-end tests for all hooks with various input scenarios |
| docs/hooks-json-parsing-errors.md | Added detailed root cause analysis documenting the JSON parsing issues and solutions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| local exit_code | ||
|
|
||
| set +e | ||
| output=$(echo "$input_json" | bash "$hook_path" 2>/dev/null) |
There was a problem hiding this comment.
The test output is captured with 2>/dev/null, which suppresses all error messages from the hook. This makes debugging test failures difficult because stderr output (including error messages and warnings) is hidden. Consider capturing stderr separately or redirecting it to stdout with 2>&1 to help diagnose test failures.
| output=$(echo "$input_json" | bash "$hook_path" 2>/dev/null) | |
| output=$(echo "$input_json" | bash "$hook_path" 2>&1) |
| fi | ||
|
|
||
| # Cleanup | ||
| rm -f "$TEMP_AGENT_FILE" |
There was a problem hiding this comment.
The test creates a temporary file at $TEMP_AGENT_FILE (.claude/agents/test-temp-agent.md) and cleans it up at line 152. However, if any test fails before reaching the cleanup (e.g., lines 46, 62, 133, or 148 call exit 1), the temporary file will not be cleaned up. Consider using a trap statement to ensure cleanup happens even on early exit: trap 'rm -f "$TEMP_AGENT_FILE"' EXIT.
| TEST_INPUT_2=$(cat <<'EOF' | ||
| { | ||
| "tool": "Edit", | ||
| "parameters": { | ||
| "file_path": ".claude/agents/actor.md", | ||
| "old_string": "dummy", | ||
| "new_string": "# Actor\n\n{{language}}\n{{project_name}}\n{{#if playbook_bullets}}test{{/if}}\n{{#if feedback}}test{{/if}}\n{{subtask_description}}\n\nShort content that removed many lines." | ||
| } | ||
| } | ||
| EOF | ||
| ) |
There was a problem hiding this comment.
The variable TEST_INPUT_2 is defined but never used. The test continues with TEST_INPUT_3 instead. Consider either removing TEST_INPUT_2 or using it for an actual test case.
| TEST_INPUT_2=$(cat <<'EOF' | |
| { | |
| "tool": "Edit", | |
| "parameters": { | |
| "file_path": ".claude/agents/actor.md", | |
| "old_string": "dummy", | |
| "new_string": "# Actor\n\n{{language}}\n{{project_name}}\n{{#if playbook_bullets}}test{{/if}}\n{{#if feedback}}test{{/if}}\n{{subtask_description}}\n\nShort content that removed many lines." | |
| } | |
| } | |
| EOF | |
| ) | |
| # (Removed unused TEST_INPUT_2 definition) |
| echo "{{#if playbook_bullets}}test{{/if}}" | ||
| echo "{{#if feedback}}test{{/if}}" | ||
| echo "{{subtask_description}}" | ||
| for i in {1..600}; do |
There was a problem hiding this comment.
The brace expansion {1..600} is a bashism that may not work in POSIX sh or other shells. While the shebang specifies #!/bin/bash, consider using seq for better portability: for i in $(seq 1 600); do. This is especially important if the test might be run in different environments.
| for i in {1..600}; do | |
| for i in $(seq 1 600); do |
| echo "[stop/quality-gates] Hook triggered" >&2 | ||
|
|
||
| # Validate JSON input before processing | ||
| if ! echo "$INPUT" | jq empty 2>/dev/null; then |
There was a problem hiding this comment.
The JSON validation check uses echo "$INPUT" | jq empty, but this could potentially have the same issues that the rest of the code was trying to fix. If $INPUT contains special characters or control sequences, echo might not preserve them correctly. Consider using jq empty <<< "$INPUT" for consistency with the other changes in this file, or use jq empty < <(printf '%s' "$INPUT") for safer handling.
| if ! echo "$INPUT" | jq empty 2>/dev/null; then | |
| if ! jq empty <<< "$INPUT" 2>/dev/null; then |
| "Stop": [ | ||
| { | ||
| "matcher": "Write|Edit", | ||
| "matcher": "Edit|Write", |
There was a problem hiding this comment.
The matcher value changed from "Write|Edit" to "Edit|Write". While both patterns match the same tools, this change appears to be purely cosmetic and doesn't affect functionality. Consider whether this reordering is intentional or if it adds value. If it's just to maintain alphabetical order, document this convention for consistency.
Address all Copilot reviewer comments:
1. tests/test_hooks_e2e.sh:
- Change stderr handling from 2>/dev/null to 2>&1 for better debugging
- Makes error messages and warnings visible in test failures
2. tests/test_validate_hook_json.sh:
- Add trap for automatic cleanup of temporary files on exit
- Prevents temp file leaks if tests exit early due to failures
- Remove manual cleanup (now handled by trap)
3. tests/test_validate_hook_json.sh:
- Remove unused TEST_INPUT_2 variable (was defined but never used)
4. tests/test_validate_hook_json.sh:
- Replace bashism {1..600} with seq command for better portability
- Works in POSIX sh and other shells
5. .claude/hooks/stop.sh:
- Use heredoc pattern 'jq <<< "$INPUT"' instead of 'echo | jq'
- Consistent with other changes in this file, safer handling of special chars
6. .claude/hooks/settings.hooks.json:
- Matcher reordering (Write|Edit -> Edit|Write) is intentional
- Standardizes pattern across all hooks for consistency
- Both patterns match the same tools, no functional change
Addresses test-hooks CI failure caused by mixing debug logs with JSON output after implementing Copilot's suggestion to use 2>&1 instead of 2>/dev/null. **Problem:** - stop.sh outputs debug logs to stderr: "[stop/quality-gates] Hook triggered" - With 2>&1, stderr mixed with stdout - jq parsing failed on combined output **Solution:** - Redirect stderr to temp file - Capture stdout (JSON) separately for validation - Combine both streams in $full_output for error display - This balances reviewer's request (preserve stderr for debugging) with test requirement (parse clean JSON) **Testing:** - All 8 E2E tests pass locally - JSON validation works correctly - Stderr preserved for debugging on failures
…-35) MEDIUM fixes: - #8: Remove dead RETRY_LOOP phase from orchestrator STEP_PHASES - #10: Fix plan path to branch-scoped .map/<branch>/task_plan_<branch>.md - #11: Fix findings path to branch-scoped .map/<branch>/findings_<branch>.md - #12: Remove references to non-existent ralph-loop-config.json - #13/#14: Rewrite map-resume to use step_state.json instead of progress.md - #15: Fix INIT_PLAN heading format (### ST-XXX with - **Status:** prefix) - #16: Fix regex in step_runner to match plan format (### heading, - **Status:**) - #17: Fix map-learn contradiction about automatic learning LOW fixes: - #9/#31: Document dual state file system (step_state.json vs workflow_state.json) - #19: Document intentional Evaluator/Reflector/Curator omission in map-efficient - #20: Fix line count reference (~150 → ~540 lines) - #21: Standardize all AskUserQuestion to Python function call syntax - #22: Rename Steps 2.5/2.6 to 2a/2b to avoid phase number collision - #23/#24: Fix map-debate comparison table (map-efficient uses single Actor) - #25: Replace cat commands with Read tool comments in map-check - #28/#29: Replace undefined thrashing_detected()/max_redecompositions - #30: Add - **Status:** pending field to map-plan template - #32: Note map-fast max 3 vs map-efficient max 5 intentional difference - #33: Remove Evaluator from map-fast skipped agents list - #34: Move AskUserQuestion to "Built-in Tools" section in map-release - #35: Replace parallel bash & processes with sequential && in map-release Template sync: All .claude/ changes mirrored to src/mapify_cli/templates/
Summary
Fixes critical JSON generation issues in git hooks that caused parsing failures when processing Write/Edit operations with multiline content.
Issues Fixed
🔧 validate-agent-templates.sh
echocaused parse errors with multiline messagesechodoesn't escape newlines (violates RFC 8259)jq -n --arg msg "$MESSAGE" '{decision: "block", message: $msg}'🔧 stop.sh
echo "$INPUT" | jqpattern could fail silently with malformed JSONif ! echo "$INPUT" | jq empty 2>/dev/nulljq <<< "$INPUT"to avoid echo interpretation🔧 settings.hooks.json
Edit|WritevsWrite|Edit)Edit|Writeacross all hooksTesting
Created comprehensive test suites:
tests/test_validate_hook_json.sh- Validates JSON with multiline messages (3 tests)tests/test_hooks_e2e.sh- E2E validation for all hooks (8 tests)Documentation
docs/hooks-json-parsing-errors.md- Complete root cause analysis with:Root Cause
Bash
echocannot escape newlines in JSON strings, violating RFC 8259 Section 7. Using jq with--argflag ensures proper escaping and RFC compliance.Impact
Files Changed
.claude/hooks/validate-agent-templates.sh- jq-based JSON generation.claude/hooks/stop.sh- Defensive JSON validation.claude/hooks/settings.hooks.json- Standardized matchersdocs/hooks-json-parsing-errors.md- Root cause documentationtests/test_validate_hook_json.sh- Unit teststests/test_hooks_e2e.sh- E2E testsTest Plan
All tests should pass ✅