fix: check merge success before firing /update-docs requirement #159

Merged

forgejo_admin merged 1 commit from 134-fix-post-merge-hook into main

2026-03-26 03:47:19 +00:00

forgejo_admin commented

2026-03-25 13:38:22 +00:00

Contributor

Summary

The remind-update-docs.sh PostToolUse hook fired unconditionally after every mcp__forgejo__merge_approved_pr call, including failed merges (405, 409). This told agents to run /update-docs for merges that never happened -- observed 3 consecutive false fires during PR #124.

Changes

hooks/remind-update-docs.sh -- Read tool_response.merged from stdin before deciding. Failed merges now emit a failure notice instead of the blocking /update-docs requirement. Matches the guard pattern already used by board-item-on-merge.sh and post-mcp-merge-rebase.sh.

Test Plan

Verified with simulated failed merge input ({"tool_response": {"error": true, "status_code": 405}}) -- emits failure notice, no /update-docs block
Verified with simulated successful merge input ({"tool_response": {"merged": true}}) -- emits blocking /update-docs requirement as before
Verified with empty input ({}) -- emits failure notice (safe default)

Review Checklist

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

Closes #134

## Summary The `remind-update-docs.sh` PostToolUse hook fired unconditionally after every `mcp__forgejo__merge_approved_pr` call, including failed merges (405, 409). This told agents to run `/update-docs` for merges that never happened -- observed 3 consecutive false fires during PR #124. ## Changes - `hooks/remind-update-docs.sh` -- Read `tool_response.merged` from stdin before deciding. Failed merges now emit a failure notice instead of the blocking `/update-docs` requirement. Matches the guard pattern already used by `board-item-on-merge.sh` and `post-mcp-merge-rebase.sh`. ## Test Plan - [x] Verified with simulated failed merge input (`{"tool_response": {"error": true, "status_code": 405}}`) -- emits failure notice, no `/update-docs` block - [x] Verified with simulated successful merge input (`{"tool_response": {"merged": true}}`) -- emits blocking `/update-docs` requirement as before - [x] Verified with empty input (`{}`) -- emits failure notice (safe default) ## Review Checklist - [x] Passed automated review-fix loop - [x] No secrets committed - [x] No unnecessary file changes - [x] Commit messages are descriptive ## Related - Closes #134

forgejo_admin added 1 commit

2026-03-25 13:38:22 +00:00

fix: check merge success before firing /update-docs requirement (#134 ) abf63ad57a

The remind-update-docs.sh PostToolUse hook fired unconditionally after
every mcp__forgejo__merge_approved_pr call, including failed merges
(405, 409). This told agents to run /update-docs for merges that never
happened.

Now reads tool_response.merged from stdin before deciding. Failed
merges emit a failure notice instead of the blocking /update-docs
requirement. Matches the pattern already used by board-item-on-merge.sh
and post-mcp-merge-rebase.sh.

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

forgejo_admin commented

2026-03-25 13:38:52 +00:00

Author

Contributor

Review -- PASS

Files reviewed: hooks/remind-update-docs.sh (18 additions, 0 deletions)

Findings: None.

Guard pattern (jq -e '.tool_response.merged // false' piped to grep -q 'true') is identical to the proven pattern in board-item-on-merge.sh and post-mcp-merge-rebase.sh. Consistent across all three PostToolUse merge hooks.
INPUT=$(cat) reads stdin once; success-path jq -n uses null input. No stdin consumption issue.
Edge cases (empty input, missing tool_response, merged: false, error responses) all fall to failure branch. Safe default.
Success-path output is byte-identical to the original. No regression risk.

Verdict: Approved. Clean single-file fix, well-tested.

## Review -- PASS **Files reviewed:** `hooks/remind-update-docs.sh` (18 additions, 0 deletions) **Findings:** None. - Guard pattern (`jq -e '.tool_response.merged // false'` piped to `grep -q 'true'`) is identical to the proven pattern in `board-item-on-merge.sh` and `post-mcp-merge-rebase.sh`. Consistent across all three PostToolUse merge hooks. - `INPUT=$(cat)` reads stdin once; success-path `jq -n` uses null input. No stdin consumption issue. - Edge cases (empty input, missing `tool_response`, `merged: false`, error responses) all fall to failure branch. Safe default. - Success-path output is byte-identical to the original. No regression risk. **Verdict:** Approved. Clean single-file fix, well-tested.

forgejo_admin commented

2026-03-25 13:40:35 +00:00

Author

Contributor

PR #159 Review

DOMAIN REVIEW

Tech stack: Bash / jq hooks for the Claude Code PostToolUse hook system.

The change adds a merge-success guard to hooks/remind-update-docs.sh. This hook was the only one of three mcp__forgejo__merge_approved_pr PostToolUse hooks that fired unconditionally -- both board-item-on-merge.sh (line 36) and post-mcp-merge-rebase.sh (line 12) already guard on .tool_response.merged. The PR closes that gap.

Correctness: The guard logic is identical to the established pattern:

if ! echo "$INPUT" | jq -e '.tool_response.merged // false' 2>/dev/null | grep -q 'true'; then

jq -e exits non-zero when the expression evaluates to false or null.
// false provides a safe default when the key is missing entirely.
grep -q 'true' is the secondary check.
2>/dev/null suppresses jq errors on malformed input.

This correctly handles all three failure modes documented in the PR body: HTTP 405 (merge blocked), HTTP 409 (conflict), and empty/missing response.

Failure path: Emits a structured hookSpecificOutput JSON notice telling the agent the merge failed and not to run /update-docs. This is the right behavior -- it gives the agent actionable context without issuing a false blocking requirement.

Success path: Falls through to the existing jq -n block that emits the BLOCKING REQUIREMENT. No change to existing behavior on successful merges.

stdin handling: The new INPUT=$(cat) is consistent with all other hooks in this repo. Claude Code provides each hook its own stdin copy (verified by the existing pattern across 29+ hooks in the repo).

Hook ordering in settings.json (lines 211-226): post-mcp-merge-rebase.sh fires first, then remind-update-docs.sh, then board-item-on-merge.sh. All three now guard on .tool_response.merged. Order is fine -- no dependencies between them.

BLOCKERS

None.

No new functionality requiring tests (this is a shell hook guard, tested manually per the test plan).
No user input handling (hook reads tool_response from Claude Code framework).
No secrets or credentials in code.
No duplicated auth/security logic -- this REMOVES a DRY violation by aligning with the existing guard pattern.

NITS

Minor: The failure-path additionalContext string is generic: "Merge was not successful." The sibling hooks (board-item-on-merge.sh, post-mcp-merge-rebase.sh) exit silently on failure (no output). This hook emits a notice instead, which is arguably better for agent awareness, but the inconsistency is worth noting. Not blocking.

SOP COMPLIANCE

Branch named after issue: 134-fix-post-merge-hook references issue #134
PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related sections present
Related references issue: "Closes #134"
No secrets committed
No unnecessary file changes (1 file, 18 additions, 0 deletions)
Commit messages are descriptive
Related section does not reference a plan slug -- acceptable since this is a standalone bug fix, not plan work

PROCESS OBSERVATIONS

This fix addresses a real production incident (3 consecutive false fires during PR #124 as documented in the issue). The change failure risk is very low -- it only adds a guard before existing logic, with no modifications to the success path. The pattern is battle-tested in two sibling hooks.

VERDICT: APPROVED

## PR #159 Review ### DOMAIN REVIEW **Tech stack**: Bash / jq hooks for the Claude Code PostToolUse hook system. The change adds a merge-success guard to `hooks/remind-update-docs.sh`. This hook was the only one of three `mcp__forgejo__merge_approved_pr` PostToolUse hooks that fired unconditionally -- both `board-item-on-merge.sh` (line 36) and `post-mcp-merge-rebase.sh` (line 12) already guard on `.tool_response.merged`. The PR closes that gap. **Correctness**: The guard logic is identical to the established pattern: ```bash if ! echo "$INPUT" | jq -e '.tool_response.merged // false' 2>/dev/null | grep -q 'true'; then ``` - `jq -e` exits non-zero when the expression evaluates to `false` or `null`. - `// false` provides a safe default when the key is missing entirely. - `grep -q 'true'` is the secondary check. - `2>/dev/null` suppresses jq errors on malformed input. This correctly handles all three failure modes documented in the PR body: HTTP 405 (merge blocked), HTTP 409 (conflict), and empty/missing response. **Failure path**: Emits a structured `hookSpecificOutput` JSON notice telling the agent the merge failed and not to run `/update-docs`. This is the right behavior -- it gives the agent actionable context without issuing a false blocking requirement. **Success path**: Falls through to the existing `jq -n` block that emits the BLOCKING REQUIREMENT. No change to existing behavior on successful merges. **stdin handling**: The new `INPUT=$(cat)` is consistent with all other hooks in this repo. Claude Code provides each hook its own stdin copy (verified by the existing pattern across 29+ hooks in the repo). **Hook ordering in settings.json** (lines 211-226): `post-mcp-merge-rebase.sh` fires first, then `remind-update-docs.sh`, then `board-item-on-merge.sh`. All three now guard on `.tool_response.merged`. Order is fine -- no dependencies between them. ### BLOCKERS None. - No new functionality requiring tests (this is a shell hook guard, tested manually per the test plan). - No user input handling (hook reads tool_response from Claude Code framework). - No secrets or credentials in code. - No duplicated auth/security logic -- this REMOVES a DRY violation by aligning with the existing guard pattern. ### NITS 1. **Minor**: The failure-path `additionalContext` string is generic: "Merge was not successful." The sibling hooks (`board-item-on-merge.sh`, `post-mcp-merge-rebase.sh`) exit silently on failure (no output). This hook emits a notice instead, which is arguably better for agent awareness, but the inconsistency is worth noting. Not blocking. ### SOP COMPLIANCE - [x] Branch named after issue: `134-fix-post-merge-hook` references issue #134 - [x] PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related sections present - [x] Related references issue: "Closes #134" - [x] No secrets committed - [x] No unnecessary file changes (1 file, 18 additions, 0 deletions) - [x] Commit messages are descriptive - [ ] Related section does not reference a plan slug -- acceptable since this is a standalone bug fix, not plan work ### PROCESS OBSERVATIONS This fix addresses a real production incident (3 consecutive false fires during PR #124 as documented in the issue). The change failure risk is very low -- it only adds a guard before existing logic, with no modifications to the success path. The pattern is battle-tested in two sibling hooks. ### VERDICT: APPROVED