fix: resilient merge status parsing for PostToolUse hooks

forgejo_admin commented

2026-03-28 11:30:12 +00:00

Author

Contributor

PR #201 Review

DOMAIN REVIEW

Tech stack: Bash / shell hooks (PostToolUse hooks for Claude Code), jq JSON parsing, unit tests in bash.

Correctness

The core change is sound. Three hooks (remind-update-docs.sh, post-mcp-merge-rebase.sh, board-item-on-merge.sh) each had identical inline two-stage jq parsing for detecting merge success. When Claude Code changed the PostToolUse stdin JSON shape, all three broke identically. Extracting _parse_merged_status() into forgejo-helper.sh is the correct DRY fix.

The function tries 5 shapes in priority order with early return on match, falling through to a grep-based last resort and finally returning "false". This is defensively coded.

Observations on _parse_merged_status() implementation:

Debug capture to /tmp/hook-debug-merge.json -- This is a reasonable diagnostic aid for future regressions. The 2>/dev/null || true guard is correct. However, note that all three hooks fire on the same merge_approved_pr event, so they will race to write this file. The last one wins, but since they all receive the same stdin content, this is a non-issue in practice.
Shape 4 has a subtle interaction with Shape 1: When .tool_response is an object (not a string), jq -r '.tool_response // empty' will output the object as a JSON string, then the second jq -r '.merged // empty' will parse it and find .merged. This means Shape 4 could match cases that Shape 1 already handles. Not a bug (Shape 1 returns first), but worth noting that Shape 4 is not purely for "string tool_response" -- it would also catch any object with a merged field at the top level of tool_response. The comment says "string" but it works on objects too.
Shape 5 grep fallback -- grep -q '"merged"[[:space:]]*:[[:space:]]*true' on the raw .tool_response output is a reasonable last resort. The risk of false-positive matching on an unrelated "merged": true buried in some nested structure is extremely low for this particular API response shape.

Source ordering in post-mcp-merge-rebase.sh: The PR correctly moves the source forgejo-helper.sh to the top (after INPUT=$(cat)) and removes the duplicate source at line 64-65. This is clean -- the helper is sourced once, used twice (for _parse_merged_status and later for remove_worktree_for_branch).

Source addition in remind-update-docs.sh: This hook did not previously source forgejo-helper.sh. The PR adds it purely to access _parse_merged_status(). Since forgejo-helper.sh is purely function definitions with no side effects on source, this is safe. The HOOK_DIR variable is also new and correctly defined.

BLOCKERS

None.

Test coverage: 13 unit tests covering all 5 shapes, both positive and negative cases, plus edge cases (error, empty, missing, non-JSON content). This exceeds the minimum bar.
No secrets committed: No credentials in the diff. The debug file path /tmp/hook-debug-merge.json contains only the PostToolUse JSON payload (tool name, tool input, tool response), not credentials.
No unvalidated user input: The function processes stdin JSON that comes from Claude Code's PostToolUse contract, not from external users.
DRY in security/auth path: The old inline duplication across 3 hooks is exactly what this PR eliminates. The fix is a single shared function.

NITS

Missing Shape 4 false test: The test file has explicit false-case tests for Shapes 1, 2, and 3, but not for Shape 4 (string tool_response with "merged": false). Adding one would make the test suite more symmetric. Non-blocking since the code path does fall through correctly to return "false" when .merged is not "true".
Comment accuracy on Shape 4: The doc comment says ".tool_response | strings | fromjson | .merged" but the actual implementation uses .tool_response // empty (not strings). The filter // empty applied to a non-string .tool_response will output the object as JSON, which then gets parsed by the second jq. The doc comment describes intended behavior; the implementation is slightly broader. Minor clarity gap.
No set -euo pipefail in _parse_merged_status: The function is designed fail-open (all 2>/dev/null, always returns 0), so this is by design. Just noting that callers must be aware it never returns non-zero.

SOP COMPLIANCE

Branch named after issue (189-fix-merge-hook-false-alarm -> issue #189)
PR body has: Summary, Changes, Test Plan, Related
Related references project-pal-e-agency plan slug
No secrets committed
No unnecessary file changes (5 files: 1 helper, 3 hooks, 1 test file -- all directly related)
Commit messages are descriptive
Closes #189 present in PR body

PROCESS OBSERVATIONS

DORA: Change Failure Rate -- This is a regression fix for a regression (#177 fixed #173, then regressed when the JSON shape changed again). The root cause is that Claude Code's PostToolUse stdin contract is unstable and undocumented. The 5-shape fallback with debug capture is an appropriate defensive response to an unstable external interface. The /tmp/hook-debug-merge.json debug capture will make the next regression significantly faster to diagnose.
DORA: Mean Time to Recovery -- The shared function means the next JSON shape change requires a fix in exactly one place instead of three.
Test maturity -- This repo now has 3 test suites (validate_branch_name: 14, block_groupme_send: 21, parse_merged_status: 13). Good trajectory.

VERDICT: APPROVED

## PR #201 Review ### DOMAIN REVIEW **Tech stack**: Bash / shell hooks (PostToolUse hooks for Claude Code), jq JSON parsing, unit tests in bash. **Correctness** The core change is sound. Three hooks (`remind-update-docs.sh`, `post-mcp-merge-rebase.sh`, `board-item-on-merge.sh`) each had identical inline two-stage jq parsing for detecting merge success. When Claude Code changed the PostToolUse stdin JSON shape, all three broke identically. Extracting `_parse_merged_status()` into `forgejo-helper.sh` is the correct DRY fix. The function tries 5 shapes in priority order with early return on match, falling through to a grep-based last resort and finally returning "false". This is defensively coded. **Observations on `_parse_merged_status()` implementation:** 1. **Debug capture to `/tmp/hook-debug-merge.json`** -- This is a reasonable diagnostic aid for future regressions. The `2>/dev/null || true` guard is correct. However, note that all three hooks fire on the same `merge_approved_pr` event, so they will race to write this file. The last one wins, but since they all receive the same stdin content, this is a non-issue in practice. 2. **Shape 4 has a subtle interaction with Shape 1**: When `.tool_response` is an object (not a string), `jq -r '.tool_response // empty'` will output the object as a JSON string, then the second `jq -r '.merged // empty'` will parse it and find `.merged`. This means Shape 4 could match cases that Shape 1 already handles. Not a bug (Shape 1 returns first), but worth noting that Shape 4 is not purely for "string tool_response" -- it would also catch any object with a `merged` field at the top level of `tool_response`. The comment says "string" but it works on objects too. 3. **Shape 5 grep fallback** -- `grep -q '"merged"[[:space:]]*:[[:space:]]*true'` on the raw `.tool_response` output is a reasonable last resort. The risk of false-positive matching on an unrelated `"merged": true` buried in some nested structure is extremely low for this particular API response shape. **Source ordering in `post-mcp-merge-rebase.sh`**: The PR correctly moves the `source forgejo-helper.sh` to the top (after `INPUT=$(cat)`) and removes the duplicate source at line 64-65. This is clean -- the helper is sourced once, used twice (for `_parse_merged_status` and later for `remove_worktree_for_branch`). **Source addition in `remind-update-docs.sh`**: This hook did not previously source `forgejo-helper.sh`. The PR adds it purely to access `_parse_merged_status()`. Since `forgejo-helper.sh` is purely function definitions with no side effects on source, this is safe. The `HOOK_DIR` variable is also new and correctly defined. ### BLOCKERS None. - **Test coverage**: 13 unit tests covering all 5 shapes, both positive and negative cases, plus edge cases (error, empty, missing, non-JSON content). This exceeds the minimum bar. - **No secrets committed**: No credentials in the diff. The debug file path `/tmp/hook-debug-merge.json` contains only the PostToolUse JSON payload (tool name, tool input, tool response), not credentials. - **No unvalidated user input**: The function processes stdin JSON that comes from Claude Code's PostToolUse contract, not from external users. - **DRY in security/auth path**: The old inline duplication across 3 hooks is exactly what this PR eliminates. The fix is a single shared function. ### NITS 1. **Missing Shape 4 false test**: The test file has explicit false-case tests for Shapes 1, 2, and 3, but not for Shape 4 (string tool_response with `"merged": false`). Adding one would make the test suite more symmetric. Non-blocking since the code path does fall through correctly to return "false" when `.merged` is not "true". 2. **Comment accuracy on Shape 4**: The doc comment says "`.tool_response | strings | fromjson | .merged`" but the actual implementation uses `.tool_response // empty` (not `strings`). The filter `// empty` applied to a non-string `.tool_response` will output the object as JSON, which then gets parsed by the second `jq`. The doc comment describes intended behavior; the implementation is slightly broader. Minor clarity gap. 3. **No `set -euo pipefail` in `_parse_merged_status`**: The function is designed fail-open (all `2>/dev/null`, always returns 0), so this is by design. Just noting that callers must be aware it never returns non-zero. ### SOP COMPLIANCE - [x] Branch named after issue (`189-fix-merge-hook-false-alarm` -> issue #189) - [x] PR body has: Summary, Changes, Test Plan, Related - [x] Related references `project-pal-e-agency` plan slug - [x] No secrets committed - [x] No unnecessary file changes (5 files: 1 helper, 3 hooks, 1 test file -- all directly related) - [x] Commit messages are descriptive - [x] `Closes #189` present in PR body ### PROCESS OBSERVATIONS - **DORA: Change Failure Rate** -- This is a regression fix for a regression (#177 fixed #173, then regressed when the JSON shape changed again). The root cause is that Claude Code's PostToolUse stdin contract is unstable and undocumented. The 5-shape fallback with debug capture is an appropriate defensive response to an unstable external interface. The `/tmp/hook-debug-merge.json` debug capture will make the next regression significantly faster to diagnose. - **DORA: Mean Time to Recovery** -- The shared function means the next JSON shape change requires a fix in exactly one place instead of three. - **Test maturity** -- This repo now has 3 test suites (validate_branch_name: 14, block_groupme_send: 21, parse_merged_status: 13). Good trajectory. ### VERDICT: APPROVED

2026-03-28 06:03:07 +00:00

Summary

Three PostToolUse hooks firing on mcp__forgejo__merge_approved_pr used inline two-stage jq parsing that broke when Claude Code changed the PostToolUse stdin JSON shape. This caused false "merge not successful" messages on successful squash merges. The fix extracts a shared _parse_merged_status() function that handles all known JSON shapes.

Changes

hooks/forgejo-helper.sh -- Added _parse_merged_status() function that tries 5 JSON shapes: direct field, stringified result, MCP content array, string response, and grep fallback. Includes debug capture to /tmp/hook-debug-merge.json.
hooks/remind-update-docs.sh -- Replaced inline parsing with shared _parse_merged_status() call
hooks/post-mcp-merge-rebase.sh -- Replaced inline parsing with shared _parse_merged_status() call, removed redundant forgejo-helper.sh double-source
hooks/board-item-on-merge.sh -- Replaced inline parsing with shared _parse_merged_status() call
tests/test_parse_merged_status.sh -- 13 unit tests covering all 5 shapes + edge cases (error responses, empty/missing tool_response, non-JSON content)

Test Plan

13 unit tests pass: bash tests/test_parse_merged_status.sh
Existing test suites pass (validate_branch_name: 14, block_groupme_send: 21)
Syntax check passes on all 4 modified shell files
Next real merge via MCP will confirm end-to-end (debug capture to /tmp/hook-debug-merge.json will reveal actual shape)

Review Checklist

Passed automated review-fix loop
No secrets committed
No unnecessary file changes
Commit messages are descriptive

project-pal-e-agency

Forgejo issue: #189
Regression of: #173 (fixed in PR #177, regressed due to stdin shape change)

Closes #189

## Summary Three PostToolUse hooks firing on `mcp__forgejo__merge_approved_pr` used inline two-stage jq parsing that broke when Claude Code changed the PostToolUse stdin JSON shape. This caused false "merge not successful" messages on successful squash merges. The fix extracts a shared `_parse_merged_status()` function that handles all known JSON shapes. ## Changes - `hooks/forgejo-helper.sh` -- Added `_parse_merged_status()` function that tries 5 JSON shapes: direct field, stringified result, MCP content array, string response, and grep fallback. Includes debug capture to `/tmp/hook-debug-merge.json`. - `hooks/remind-update-docs.sh` -- Replaced inline parsing with shared `_parse_merged_status()` call - `hooks/post-mcp-merge-rebase.sh` -- Replaced inline parsing with shared `_parse_merged_status()` call, removed redundant `forgejo-helper.sh` double-source - `hooks/board-item-on-merge.sh` -- Replaced inline parsing with shared `_parse_merged_status()` call - `tests/test_parse_merged_status.sh` -- 13 unit tests covering all 5 shapes + edge cases (error responses, empty/missing tool_response, non-JSON content) ## Test Plan - [x] 13 unit tests pass: `bash tests/test_parse_merged_status.sh` - [x] Existing test suites pass (validate_branch_name: 14, block_groupme_send: 21) - [x] Syntax check passes on all 4 modified shell files - [ ] Next real merge via MCP will confirm end-to-end (debug capture to `/tmp/hook-debug-merge.json` will reveal actual shape) ## Review Checklist - [x] Passed automated review-fix loop - [x] No secrets committed - [x] No unnecessary file changes - [x] Commit messages are descriptive ## Related Notes - `project-pal-e-agency` ## Related - Forgejo issue: #189 - Regression of: #173 (fixed in PR #177, regressed due to stdin shape change) Closes #189

forgejo_admin added 1 commit

fix: resilient merge status parsing for PostToolUse hooks cbcbdec148

The three merge hooks (remind-update-docs, post-mcp-merge-rebase,
board-item-on-merge) used two-stage jq parsing that failed when
Claude Code changed the PostToolUse stdin JSON shape. Extracted
a shared _parse_merged_status() function in forgejo-helper.sh that
tries 5 different JSON shapes: direct field, stringified result,
MCP content array, string response, and grep fallback.

Adds debug capture to /tmp/hook-debug-merge.json for future
regression diagnosis. 13 unit tests cover all shapes + edge cases.

Closes #189

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>