pal-e-docs

Review: Remove gh CLI references, fix post-merge hook for Forgejo MCP merges

review-1529-2026-06-20 Review

review needs-refinement
Project

Verdict: NEEDS_REFINEMENT

Template Completeness

  • [x] Type
  • [x] Lineage
  • [x] Repo
  • [x] User Story
  • [x] Context
  • [x] File Targets
  • [x] Feature Flag
  • [x] Acceptance Criteria
  • [x] Test Expectations
  • [x] Constraints
  • [x] Checklist
  • [x] Related

Traceability

  • [x] story:operational-reference label -- verified on project-claude-custom user-stories section
  • [x] story note verified -- "operational-reference" entry found in project-claude-custom user-stories section
  • [ ] arch note MISSING -- [SCOPE] Create architecture note arch-hooks for component hooks. Search for "arch-hooks" returned no results in pal-e-docs.
  • [x] arch:hooks label present on board item
  • [x] Forgejo issue -- https://forgejo.tail5b443a.ts.net/ldraney/claude-custom/issues/268, open

File Targets

  • [ ] hooks/post-merge-rebase.sh -- ISSUE: The issue says to "rewrite trigger: match on MCP tool response JSON" and update settings.json wiring, but hooks/post-mcp-merge-rebase.sh ALREADY EXISTS and is already wired as a PostToolUse hook on mcp__forgejo__merge_approved_pr in settings.json (line 267). AC1 and AC2 (fast-forward + worktree cleanup after MCP merge) are already working. The real scope here is just DELETING post-merge-rebase.sh (dead code that only fires on gh pr merge Bash commands) and removing its settings.json entry from the Bash PostToolUse matcher (line 228).
  • [x] hooks/check-issue.sh -- verified: lines 106-123 contain GitHub/gh issue view code paths (github case + unknown platform fallback). Lines 178, 222, 242 contain gh CLI references in error messages.
  • [ ] hooks/block-pr-merge.sh -- ISSUE: The issue says "remove gh pr merge detection, evaluate if this hook is still needed." It also handles Forgejo curl-based merges (lines 25-35). However, hooks/block-mcp-merge.sh is already wired for the MCP merge path in settings.json. The curl-based Forgejo merge path in block-pr-merge.sh would only fire if someone used curl directly instead of MCP -- likely dead code. Clarify: should the entire hook be deleted, or should the Forgejo curl path be preserved?
  • [x] hooks/block-upstream.sh -- verified: entire hook blocks gh pr/issue create commands. All functionality is dead code since we use Forgejo MCP tools.
  • [ ] hooks/remind-review-loop.sh -- PARTIAL ISSUE: The only gh CLI reference is in an advisory string in the Forgejo path (line 30: "use Forgejo API (curl) instead of gh CLI"). The GitHub PR creation trigger (lines 14-19, gh pr create) is dead code. The Forgejo curl-based PR creation trigger (lines 24-35) may also be dead code since remind-mcp-review-loop.sh already exists for the MCP PR submission path. Clarify: delete entire hook or just the GitHub path?
  • [x] settings.local.json -- verified: 15 Bash(gh ...:*) entries in allow + 1 Bash(gh pr merge:*) in ask = 16 total entries to remove
  • [x] settings.json -- verified: line 4 has Bash(gh api:*) permission to remove. WebFetch domains for github.com and raw.githubusercontent.com present (lines 6-7) -- issue correctly notes these should be evaluated for keeping.
  • [ ] settings.json hook wiring -- ISSUE: The issue says "post-merge-rebase.sh matcher should change from Bash to mcp__forgejo__merge_approved_pr in PostToolUse." This is WRONG -- the MCP matcher already exists (line 263) using post-mcp-merge-rebase.sh. The actual work is to REMOVE post-merge-rebase.sh from the Bash PostToolUse matcher (line 228), not move it.

Repo Placement

OK -- all changes are in ldraney/claude-custom, matching the Forgejo issue repo.

Dependencies

No blocking dependencies found. The only other board item is #1526 (docs directory, already in done). The existing post-mcp-merge-rebase.sh and block-mcp-merge.sh hooks are dependencies that are already in place and must NOT be modified.

Acceptance Criteria

  • AC1 "After mcp merge, local main fast-forwarded" -- ALREADY WORKS via post-mcp-merge-rebase.sh. This AC is misleading because it implies it's broken. It works. The ticket should reframe this as "verify existing behavior is preserved after dead code removal."
  • AC2 "After MCP merge, worktree cleaned up" -- ALREADY WORKS via post-mcp-merge-rebase.sh lines 80-94. Same issue as AC1.
  • AC3 "No gh CLI calls remain in any hook" -- verifiable, clear
  • AC4 "settings.local.json has no Bash(gh) permissions" -- verifiable, clear
  • AC5 "settings.json has no Bash(gh api) permission" -- verifiable, clear
  • AC6 "All modified hooks still parse valid JSON" -- verifiable via bash -n

Blast Radius

  • hooks/remind-mcp-review-loop.sh (NOT in file targets) also contains an advisory mention of "gh CLI" in its output string (line 10). Harmless but inconsistent if the goal is to eliminate all gh references.
  • hooks/forgejo-helper.sh has is_github_repo() -- correctly excluded from scope.
  • hooks/session-start-context.sh -- correctly excluded from scope.
  • Removing block-upstream.sh and block-pr-merge.sh from settings.json hooks section will also be required -- the issue doesn't mention updating settings.json hook wiring for these two hooks, only for post-merge-rebase.sh.

Decomposition Assessment

8 file targets across 1 repo, 6 acceptance criteria. The work is straightforward deletion/cleanup (not complex logic), and is scoped to a single repo. Despite the high file count, an agent can complete this in a single pass under 5 minutes. No decomposition needed.

Recommendations

  • [BODY] Fix file target for post-merge-rebase.sh: The scope should say "DELETE this hook (dead code) and remove its entry from settings.json Bash PostToolUse matcher (line 228)." Do NOT say "rewrite trigger" -- the MCP trigger already exists as post-mcp-merge-rebase.sh.
  • [BODY] Fix settings.json hook wiring description: Remove "post-merge-rebase.sh matcher should change from Bash to mcp__forgejo__merge_approved_pr" -- this is already done. Replace with "remove post-merge-rebase.sh from Bash PostToolUse hooks, remove block-pr-merge.sh from Bash PreToolUse hooks, remove block-upstream.sh from Bash PreToolUse hooks, remove remind-review-loop.sh from Bash PostToolUse hooks."
  • [BODY] Clarify block-pr-merge.sh scope: Should the Forgejo curl merge path (lines 25-35) be preserved or is it dead code now that block-mcp-merge.sh handles MCP merges?
  • [BODY] Clarify remind-review-loop.sh scope: Should the Forgejo curl PR creation trigger (lines 24-35) be preserved or is it dead code now that remind-mcp-review-loop.sh handles MCP PR submissions?
  • [BODY] Reframe AC1 and AC2: These features already work. Rewrite as "verify post-mcp-merge-rebase.sh still functions correctly after dead code removal" rather than implying they need to be built.
  • [BODY] Add settings.json hook wiring cleanup for block-upstream.sh, block-pr-merge.sh, and remind-review-loop.sh to the File Targets section -- currently only post-merge-rebase.sh wiring change is mentioned.
  • [SCOPE] Create architecture note arch-hooks for component hooks.