pal-e-docs

Review: Bug: merge hook false positive

review-426-2026-03-27 Review

review ready
Project

Verdict: READY

Template Completeness

  • [x] Type — Bug
  • [x] Lineage — "Discovered during pal-e-platform PR #192 merge (2026-03-27)"
  • [x] Repo — forgejo_admin/claude-custom
  • [x] What Broke — clear description of false positive on successful merge
  • [x] Repro Steps — 4 steps, concrete
  • [x] Expected Behavior — clear
  • [x] Environment — local, current main, no alerts
  • [x] Acceptance Criteria — 3 criteria, all testable
  • [x] Related — project + originating PR referenced

Traceability

  • [ ] story:X label — Missing. However, this is foundational hooks infrastructure. Acceptable for a discovered-scope bug, but should be tagged story:dev-execute (merge hooks serve the developer execution workflow) before moving to next_up.
  • [x] arch:hooks label — present, correct component
  • [x] Forgejo issue — forgejo_admin/claude-custom#173, open

File Targets

The issue does not name specific file paths, but the bug description points to the PostToolUse:mcp__forgejo__merge_approved_pr hook. Three hooks are registered for that matcher in settings.json (lines 216-230):
  • [x] hooks/remind-update-docs.sh — verified: line 20 uses .tool_response.merged. This is the hook that emits the false positive "Merge was not successful" message described in the ticket.
  • [x] hooks/post-mcp-merge-rebase.sh — verified: line 12 uses identical pattern. Silently exits on false negative (no user-visible error, but skips the local fast-forward).
  • [x] hooks/board-item-on-merge.sh — verified: line 36 uses identical pattern. Silently exits on false negative (skips auto-move to done).

Root Cause Analysis

All three hooks check jq -e '.tool_response.merged // false'. The MCP tool (forgejo-mcp/src/forgejo_mcp/tools/workflows.py line 263) returns json.dumps({"merged": True, ...}) — a JSON string, not a parsed object. Claude Code's PostToolUse hook receives the MCP response as .tool_response.result (a string), not as parsed fields at .tool_response.*.
Evidence: label-on-pr.sh (lines 30-34) and label-on-branch.sh (lines 24-28) both demonstrate the correct pattern — they try .tool_response.field first, then fall back to parsing .tool_response.result as a string. The merge hooks never implemented that fallback.
The fix requires either:
  • Parsing .tool_response.result as JSON and checking .merged inside it, OR
  • Using .tool_response.result | fromjson | .merged in the jq expression

Repo Placement

Correct. Issue filed on forgejo_admin/claude-custom, and all three affected hooks live in ~/claude-custom/hooks/. No cross-repo concern.

Dependencies

  • Board item #230 (claude-custom#134) — "Post-merge hook fires on failed merges" — is the predecessor bug (now done/closed). The fix for #134 introduced the .tool_response.merged check that is now itself broken. No blocking dependency.
  • No in_progress items block this work.

Acceptance Criteria

  • [x] "Hook correctly detects merged: true in MCP response" — testable by running the hook script with a mock JSON input piped to stdin
  • [x] "Hook does NOT emit false positive failure on successful squash merge" — testable end-to-end by merging a test PR
  • [x] "Hook still correctly reports actual merge failures (405, 409, etc.)" — testable with mock input containing error responses
All criteria are verifiable. No test infrastructure exists (no tests/ directory in claude-custom), so verification will be manual stdin piping. Consider adding a test harness as discovered scope.

Blast Radius

  • 3 hooks affected, not 1. The ticket describes remind-update-docs.sh but the identical bug exists in post-mcp-merge-rebase.sh and board-item-on-merge.sh. All three must be fixed together.
  • post-mcp-merge-rebase.sh silently failing means local main never fast-forwards after MCP merges — agents may be working on stale main.
  • board-item-on-merge.sh silently failing means board items are NOT being auto-moved to done after MCP merges.
  • The block-mcp-merge.sh PreToolUse hook is NOT affected (it reads .tool_input, not .tool_response).
  • The post-merge-rebase.sh Bash-matcher hook is NOT affected (it checks .tool_response.exitCode for gh pr merge, a different code path).

Decomposition (5-minute rule)

  • 3 file targets, 1 repo — under threshold
  • 3 acceptance criteria — under threshold
  • All three files need the same one-line jq fix — well under 5 minutes
  • No decomposition needed

Recommendation

  • Add story:dev-execute label to the board item before moving to next_up (traceability gap).
  • Update the issue body to name all 3 affected files explicitly (currently only describes the symptom from remind-update-docs.sh).
  • Fix must cover all 3 hooks, not just the one producing visible output.
After those two refinements, this ticket is ready for agent execution.