pal-e-docs

Review: Merge hook false negative bug (#216)

review-585-2026-03-28 Doc

review needs-refinement
Project

Verdict: NEEDS_REFINEMENT

Template Completeness

Issue uses a hybrid format -- some bug template sections, some feature template sections. Checking against the canonical issue template (template-issue):
  • [x] Type -- present (but says "Feature" instead of "Bug" -- contradicts board label type:bug and title prefix "Bug:")
  • [x] Lineage -- present
  • [x] Repo -- present
  • [x] User Story -- present, clear who/what/why
  • [x] Context -- present, detailed session context with 8 false negatives
  • [/] File Targets -- present but WRONG primary target (see File Targets section below)
  • [x] Acceptance Criteria -- present, 3 criteria
  • [x] Test Expectations -- present, 3 test cases
  • [x] Constraints -- present
  • [x] Checklist -- present
  • [x] Related -- present
Since this is typed as a Bug on the board, it should follow template-issue-bug which requires What Broke, Repro Steps, Expected Behavior, and Environment. Those are missing but the equivalent info exists in the User Story and Context sections. The Type mismatch (body says Feature, board says Bug) is the primary template issue.

Traceability

  • [x] story:pm-scope -- PM scope management story
  • [x] arch:hooks -- hooks architecture component
  • [x] Forgejo issue -- forgejo_admin/claude-custom#216, open

File Targets

  • [x] hooks/post-mcp-merge-rebase.sh -- verified exists, but this is NOT the right fix target. This file calls _parse_merged_status at line 16 but does not define it.
  • [ ] hooks/forgejo-helper.sh -- MISSING FROM TICKET. This is where _parse_merged_status lives (lines 333-380). This is the actual file needing the fix. Bug confirmed: the function does not handle the double-stringified shape where tool_response is a string containing JSON with a result key that is itself a string containing {"merged": true}.
  • [ ] tests/test_parse_merged_status.sh -- MISSING FROM TICKET. Existing test file with 13 passing tests. Needs a new test case for the double-stringified shape. The debug capture at /tmp/hook-debug-merge.json provides the exact shape to test.
Root cause verified via codebase: _parse_merged_status returns "false" for the actual PostToolUse JSON shape captured in /tmp/hook-debug-merge.json. The shape is: tool_response (string) -> parse -> {"result": "..."} -> parse result -> {"merged": true}. None of the 5 existing parsing shapes handle this two-level unwrap.

Repo Placement

OK. Issue filed on claude-custom, fix targets are in claude-custom. Single-repo scope.

Dependencies

  • [x] Board item #505 (issue #189, "post-merge hook false alarm on squash merge") -- satisfied (done). This was the predecessor bug that introduced _parse_merged_status with 5 shapes. The current bug is a regression from an unhandled 6th shape.
  • No blocking items found in in_progress or next_up columns.

Acceptance Criteria

3 criteria from the issue:
  • AC1: "Hook correctly detects merged: true in tool output and reports success" -- testable but vague. Should specify the double-stringified shape explicitly. Unit test against _parse_merged_status with the exact /tmp/hook-debug-merge.json shape.
  • AC2: "Hook correctly detects actual merge failures and reports failure" -- already covered by existing tests (9 edge cases pass). Low risk of regression.
  • AC3: "/update-docs reminder fires on successful merges" -- integration-level. Cannot be verified without a real merge. Should be split: unit test (parse function) + manual validation (real merge).

Blast Radius

Three hooks share _parse_merged_status via forgejo-helper.sh:
  • remind-update-docs.sh -- emits false "Merge was not successful" message (the visible symptom)
  • post-mcp-merge-rebase.sh -- silently skips local main fast-forward after merge
  • board-item-on-merge.sh -- silently skips board item auto-move to done
Fixing _parse_merged_status in forgejo-helper.sh fixes all three. No other consumers found. Rollback is straightforward (revert single function change).

Decomposition Assessment

1 primary file to change (hooks/forgejo-helper.sh), 1 test file to update (tests/test_parse_merged_status.sh). 3 acceptance criteria. Well under the three-thing limit and five-minute rule. No decomposition needed. No independent subtasks to parallelize.

Recommendation

  • [BODY] Fix Type header: change "Feature" to "Bug" to match board label and title.
  • [BODY] Fix primary file target: change hooks/post-mcp-merge-rebase.sh to hooks/forgejo-helper.sh (lines 333-380, _parse_merged_status function). The hook scripts themselves need no changes.
  • [BODY] Add file target: tests/test_parse_merged_status.sh -- add test case for double-stringified shape (string tool_response containing JSON with string result field).
  • [BODY] Add root cause to Context: the shape is tool_response(string) -> parse -> {result: string} -> parse -> {merged: true}. None of the 5 existing shapes handle the two-level unwrap. Reference /tmp/hook-debug-merge.json for the exact payload.
  • [BODY] Clarify AC1: specify that the double-stringified shape must be detected, not just generic "tool output".