fix: handle double-stringified merge response in _parse_merged_status #219

Merged

forgejo_admin merged 1 commit from 216-fix-merge-parse into main

2026-03-28 23:18:21 +00:00

forgejo_admin commented

2026-03-28 22:55:29 +00:00

Contributor

Summary

Fixes false negative in _parse_merged_status() where all 8 PR merges during session 2026-03-28 reported "Merge was not successful" despite the Forgejo API returning merged: true. The root cause: Claude Code MCP wraps merge_approved_pr responses in double-stringified JSON that none of the existing 5 shapes could parse.

Changes

hooks/forgejo-helper.sh -- Added Shape 4b to _parse_merged_status(): after parsing .tool_response as a string (Shape 4), also parse .result inside for .merged. Updated header comment to document all 6 shapes including 4b and the existing Shape 5.
tests/test_parse_merged_status.sh -- Added 2 test cases for Shape 4b: double-stringified merged: true and merged: false, using the exact payload format captured from /tmp/hook-debug-merge.json.

Test Plan

bash tests/test_parse_merged_status.sh -- 15/15 pass (13 existing + 2 new)
Verified fix against exact real-world payload from /tmp/hook-debug-merge.json (Python-generated replica)
Existing shapes 1-5 remain unchanged and all pass

Review Checklist

Only _parse_merged_status() modified -- no other functions touched
No changes to how hooks call this function
All 15 tests pass (13 existing + 2 new)
Shape 4b inserted before Shape 5 (grep fallback) to maintain parse order

None

Closes #216

Board: board-pal-e-agency, item 585
Root cause: session 2026-03-28 -- 8 false negatives on merge detection

## Summary Fixes false negative in `_parse_merged_status()` where all 8 PR merges during session 2026-03-28 reported "Merge was not successful" despite the Forgejo API returning `merged: true`. The root cause: Claude Code MCP wraps `merge_approved_pr` responses in double-stringified JSON that none of the existing 5 shapes could parse. ## Changes - `hooks/forgejo-helper.sh` -- Added Shape 4b to `_parse_merged_status()`: after parsing `.tool_response` as a string (Shape 4), also parse `.result` inside for `.merged`. Updated header comment to document all 6 shapes including 4b and the existing Shape 5. - `tests/test_parse_merged_status.sh` -- Added 2 test cases for Shape 4b: double-stringified `merged: true` and `merged: false`, using the exact payload format captured from `/tmp/hook-debug-merge.json`. ## Test Plan - `bash tests/test_parse_merged_status.sh` -- 15/15 pass (13 existing + 2 new) - Verified fix against exact real-world payload from `/tmp/hook-debug-merge.json` (Python-generated replica) - Existing shapes 1-5 remain unchanged and all pass ## Review Checklist - [x] Only `_parse_merged_status()` modified -- no other functions touched - [x] No changes to how hooks call this function - [x] All 15 tests pass (13 existing + 2 new) - [x] Shape 4b inserted before Shape 5 (grep fallback) to maintain parse order ## Related Notes - None ## Related Closes #216 - Board: board-pal-e-agency, item 585 - Root cause: session 2026-03-28 -- 8 false negatives on merge detection

forgejo_admin added 1 commit

2026-03-28 22:55:29 +00:00

fix: handle double-stringified merge response in _parse_merged_status 902c90490f

Claude Code MCP wraps merge_approved_pr responses in double-stringified
JSON: tool_response is a string containing {"result": "{...merged: true...}"}.
Existing shapes 1-4 all missed this because none parsed through both
string layers. Add Shape 4b to unwrap tool_response -> .result -> .merged.

Closes #216

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

forgejo_admin commented

2026-03-28 22:56:02 +00:00

Author

Contributor

QA Review

VERDICT: APPROVED

Diff Analysis (2 files, +27/-0)

hooks/forgejo-helper.sh

Shape 4b correctly placed between Shape 4 (string tool_response) and Shape 5 (grep fallback)
Three-stage jq pipe (tool_response -> result -> merged) matches the exact double-stringified structure from the real-world debug capture
Code style matches existing shapes exactly (same merged=$() / if / echo / return pattern)
Header comment updated to document 4b and the previously unlisted Shape 5

tests/test_parse_merged_status.sh

2 new test cases: double-stringified merged: true and merged: false
Payload format matches the real-world capture from /tmp/hook-debug-merge.json
Correctly placed between Shape 4 and Shape 5 test sections

Checklist

Only _parse_merged_status() modified -- no other functions touched
No changes to caller contracts
15/15 tests pass (13 existing + 2 new)
Parse order preserved (structured parsing before grep fallback)
No scope creep

## QA Review **VERDICT: APPROVED** ### Diff Analysis (2 files, +27/-0) **`hooks/forgejo-helper.sh`** - Shape 4b correctly placed between Shape 4 (string tool_response) and Shape 5 (grep fallback) - Three-stage jq pipe (`tool_response` -> `result` -> `merged`) matches the exact double-stringified structure from the real-world debug capture - Code style matches existing shapes exactly (same `merged=$()` / `if` / `echo` / `return` pattern) - Header comment updated to document 4b and the previously unlisted Shape 5 **`tests/test_parse_merged_status.sh`** - 2 new test cases: double-stringified `merged: true` and `merged: false` - Payload format matches the real-world capture from `/tmp/hook-debug-merge.json` - Correctly placed between Shape 4 and Shape 5 test sections ### Checklist - [x] Only `_parse_merged_status()` modified -- no other functions touched - [x] No changes to caller contracts - [x] 15/15 tests pass (13 existing + 2 new) - [x] Parse order preserved (structured parsing before grep fallback) - [x] No scope creep

forgejo_admin commented

2026-03-28 23:11:44 +00:00

Author

Contributor

PR #219 Review

DOMAIN REVIEW

Tech stack: Bash shell scripting, jq JSON processing, unit tests (bash-based).

Shape 4b jq pipeline correctness: The three-stage pipeline is correct for the double-stringified case:

jq -r '.tool_response // empty' -- extracts the string value of .tool_response, outputting raw content (one level of JSON unescaping)
jq -r '.result // empty' -- parses that raw content as JSON, extracts .result string, outputs raw
jq -r '.merged // empty' -- parses the innermost string as JSON, extracts .merged

Each jq invocation acts as an implicit fromjson for the next stage because jq -r on a string value outputs the raw string content, which the next jq instance re-parses. The pipeline correctly unwraps two levels of stringification.

Shape ordering: Shape 4b is placed after Shape 4 and before Shape 5 (grep fallback). This is correct. Shape 4 handles single-stringified .tool_response with .merged at the top level. Shape 4b handles single-stringified .tool_response where .merged is nested inside a second stringified .result. Neither interferes with earlier shapes because Shapes 1-4 would all fail (return empty) before reaching 4b for this payload structure.

No regression risk to Shapes 1-5: Shape 4b only executes when Shapes 1-4 have all failed to find merged: true. Its jq pipeline with // empty at each stage ensures any parsing failure produces empty strings rather than errors, falling through gracefully. The 2>/dev/null on each jq call suppresses parse errors for non-matching shapes.

Error suppression pattern: Consistent with existing shapes -- each jq call has 2>/dev/null and uses // empty as a null coalescing fallback. This is the established pattern in this function.

Observation on Shape 5 overlap: Shape 5 (grep fallback) likely already matches the double-stringified merged: true case in isolation, since jq -r '.tool_response' would output the raw string content containing "merged": true after one level of unescaping. However, adding Shape 4b for explicit structured parsing is the right call -- grep is a blunt instrument and structured parsing is more reliable. If the real-world payload had additional escaping or whitespace that defeated grep, Shape 4b would still work.

Header comment update: The diff correctly adds Shape 4b and Shape 5 to the header comment block. Shape 5 existed in code but was undocumented in the header -- this is a good documentation fix included alongside the feature.

BLOCKERS

None.

New functionality (Shape 4b) has test coverage: 2 test cases (true and false)
No user input validation concerns (this function processes hook stdin, not external user input)
No secrets or credentials in code
No DRY violations -- the pattern is consistent with existing shapes

NITS

Shape 4 missing merged:false test: The existing Shape 4 (line 73) only tests merged:true but not merged:false. Shape 4b correctly tests both. Consider adding a merged:false test for Shape 4 in a follow-up for test parity across all shapes. (Pre-existing gap, not introduced by this PR.)
Comment notation: The header comment says .tool_response | strings | fromjson | .result | fromjson | .merged but the actual code uses jq -r '.tool_response // empty' | jq -r '.result // empty' | jq -r '.merged // empty' (three separate jq invocations with // empty). This is functionally equivalent but the comment notation implies a single jq pipeline. Consistent with how Shape 4 is already documented, so this is not a divergence from established convention.

SOP COMPLIANCE

Branch named after issue: 216-fix-merge-parse follows {issue-number}-{kebab-case-purpose}
PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related sections all present
Related references Closes #216 and board item
No secrets committed
No scope creep: only _parse_merged_status() modified + corresponding tests
Commit messages descriptive (single commit implied by 27 additions, 0 deletions)
Related section does not reference a plan slug (minor -- this is a bugfix, not plan work, so acceptable)

PROCESS OBSERVATIONS

DORA impact: This fix directly improves MTTR (Mean Time to Recover). The false negative caused 8 merges to report failure, which would trigger unnecessary investigation by operators. With this fix, the post-merge hooks (board-item-on-merge, post-mcp-merge-rebase, remind-update-docs) will correctly detect merges for the double-stringified shape.
Change failure risk: Low. The fix is additive (27 additions, 0 deletions), inserted in the correct position in the parse chain, and tested with both true/false payloads. Existing test cases are not modified.
Defensive depth: The function now has 6 parsing strategies. The comment in the code about Claude Code changing PostToolUse JSON shape across versions is well-justified -- this is the exact kind of shape drift that caused the original 8 false negatives.

VERDICT: APPROVED

## PR #219 Review ### DOMAIN REVIEW **Tech stack**: Bash shell scripting, jq JSON processing, unit tests (bash-based). **Shape 4b jq pipeline correctness**: The three-stage pipeline is correct for the double-stringified case: 1. `jq -r '.tool_response // empty'` -- extracts the string value of `.tool_response`, outputting raw content (one level of JSON unescaping) 2. `jq -r '.result // empty'` -- parses that raw content as JSON, extracts `.result` string, outputs raw 3. `jq -r '.merged // empty'` -- parses the innermost string as JSON, extracts `.merged` Each `jq` invocation acts as an implicit `fromjson` for the next stage because `jq -r` on a string value outputs the raw string content, which the next `jq` instance re-parses. The pipeline correctly unwraps two levels of stringification. **Shape ordering**: Shape 4b is placed after Shape 4 and before Shape 5 (grep fallback). This is correct. Shape 4 handles single-stringified `.tool_response` with `.merged` at the top level. Shape 4b handles single-stringified `.tool_response` where `.merged` is nested inside a second stringified `.result`. Neither interferes with earlier shapes because Shapes 1-4 would all fail (return empty) before reaching 4b for this payload structure. **No regression risk to Shapes 1-5**: Shape 4b only executes when Shapes 1-4 have all failed to find `merged: true`. Its jq pipeline with `// empty` at each stage ensures any parsing failure produces empty strings rather than errors, falling through gracefully. The `2>/dev/null` on each jq call suppresses parse errors for non-matching shapes. **Error suppression pattern**: Consistent with existing shapes -- each jq call has `2>/dev/null` and uses `// empty` as a null coalescing fallback. This is the established pattern in this function. **Observation on Shape 5 overlap**: Shape 5 (grep fallback) likely already matches the double-stringified `merged: true` case in isolation, since `jq -r '.tool_response'` would output the raw string content containing `"merged": true` after one level of unescaping. However, adding Shape 4b for explicit structured parsing is the right call -- grep is a blunt instrument and structured parsing is more reliable. If the real-world payload had additional escaping or whitespace that defeated grep, Shape 4b would still work. **Header comment update**: The diff correctly adds Shape 4b and Shape 5 to the header comment block. Shape 5 existed in code but was undocumented in the header -- this is a good documentation fix included alongside the feature. ### BLOCKERS None. - New functionality (Shape 4b) has test coverage: 2 test cases (true and false) - No user input validation concerns (this function processes hook stdin, not external user input) - No secrets or credentials in code - No DRY violations -- the pattern is consistent with existing shapes ### NITS 1. **Shape 4 missing `merged:false` test**: The existing Shape 4 (line 73) only tests `merged:true` but not `merged:false`. Shape 4b correctly tests both. Consider adding a `merged:false` test for Shape 4 in a follow-up for test parity across all shapes. (Pre-existing gap, not introduced by this PR.) 2. **Comment notation**: The header comment says `.tool_response | strings | fromjson | .result | fromjson | .merged` but the actual code uses `jq -r '.tool_response // empty' | jq -r '.result // empty' | jq -r '.merged // empty'` (three separate jq invocations with `// empty`). This is functionally equivalent but the comment notation implies a single jq pipeline. Consistent with how Shape 4 is already documented, so this is not a divergence from established convention. ### SOP COMPLIANCE - [x] Branch named after issue: `216-fix-merge-parse` follows `{issue-number}-{kebab-case-purpose}` - [x] PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related sections all present - [x] Related references Closes #216 and board item - [x] No secrets committed - [x] No scope creep: only `_parse_merged_status()` modified + corresponding tests - [x] Commit messages descriptive (single commit implied by 27 additions, 0 deletions) - [ ] Related section does not reference a plan slug (minor -- this is a bugfix, not plan work, so acceptable) ### PROCESS OBSERVATIONS - **DORA impact**: This fix directly improves MTTR (Mean Time to Recover). The false negative caused 8 merges to report failure, which would trigger unnecessary investigation by operators. With this fix, the post-merge hooks (board-item-on-merge, post-mcp-merge-rebase, remind-update-docs) will correctly detect merges for the double-stringified shape. - **Change failure risk**: Low. The fix is additive (27 additions, 0 deletions), inserted in the correct position in the parse chain, and tested with both true/false payloads. Existing test cases are not modified. - **Defensive depth**: The function now has 6 parsing strategies. The comment in the code about Claude Code changing PostToolUse JSON shape across versions is well-justified -- this is the exact kind of shape drift that caused the original 8 false negatives. ### VERDICT: APPROVED