Skip to content

fix(skill-management): PR-open hook matcher never matched MCP create_pull_request tools - #550

Merged
wkoutre merged 1 commit into
nextfrom
nickkoutrelakos/fix-skill-mgmt-hook-mcp-matcher
Jul 16, 2026
Merged

fix(skill-management): PR-open hook matcher never matched MCP create_pull_request tools#550
wkoutre merged 1 commit into
nextfrom
nickkoutrelakos/fix-skill-mgmt-hook-mcp-matcher

Conversation

@wkoutre

@wkoutre wkoutre commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Summary

The pr-skill-doctor-prompt.cjs PostToolUse hook is supposed to fire when a PR is opened via gh pr create/gt submit (Bash) or via a GitHub MCP create_pull_request tool. The MCP path has never worked: the matcher in hooks/hooks.json was "Bash|create_pull_request", and per the Claude Code hooks docs, a matcher composed only of letters, digits, _, -, spaces, ,, and | is evaluated as a list of exact tool names, not a regex. Regex mode activates only when the pattern contains a character outside that set.

No MCP tool is named bare create_pull_request — they are namespaced (mcp__github__create_pull_request, mcp__plugin_uniswap-integrations_github__create_pull_request, etc.) — so the hook never fired for MCP-created PRs, the script's toolName.endsWith('create_pull_request') branch was unreachable dead code, and the once-per-session /skill-mine nudge was silently lost whenever a PR was opened through MCP.

Fix

One line: matcher becomes "Bash|mcp__.+__create_pull_request". The .+ metacharacters flip the whole matcher into (unanchored) JavaScript regex mode, so it matches Bash and any mcp__<server>__create_pull_request tool name.

Edge case checked: in regex mode the unanchored Bash alternative also matches BashOutput, but isPrOpen() in the hook script requires toolName === 'Bash' exactly (else the endsWith check), so the only cost is a no-op hook invocation — no spurious prompts.

Also in this PR:

  • skill-management plugin version 1.0.1 → 1.0.2 (patch, per repo semver policy) + root CLAUDE.md version table
  • Plugin CLAUDE.md updated to cite the new matcher and document why the .+ is load-bearing, so this doesn't regress in a future "simplification"

Same fix already shipped and verified in Utility-NYC/closet-ai#57 (plugins/closet-ai-meta/hooks/hooks.json).

Test plan

  • Matcher semantics verified against the Claude Code hooks docs (exact-string-list vs regex-mode split, unanchored RegExp.test)
  • node scripts/validate-plugin.cjs packages/plugins/skill-management passes
  • bunx nx affected --target=lint --base=origin/next green; bunx markdownlint-cli2 0 errors
  • Independent code-reviewer agent pass on the diff: READY TO MERGE, zero findings

…reate_pull_request tools

The PostToolUse matcher "Bash|create_pull_request" contains only
exact-match characters, so Claude Code evaluates it as a list of exact
tool names, not a regex. No MCP tool is named bare create_pull_request
(they are namespaced, e.g. mcp__github__create_pull_request), so the
MCP branch of pr-skill-doctor-prompt.cjs was unreachable and the
/skill-mine nudge was silently lost for MCP-created PRs.

"Bash|mcp__.+__create_pull_request" contains regex metacharacters,
which flips the whole matcher into (unanchored) regex mode and matches
any mcp__<server>__create_pull_request tool name.

Bumps skill-management to 1.0.2.
@wkoutre
wkoutre requested a review from a team as a code owner July 16, 2026 13:30
@vercel

vercel Bot commented Jul 16, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
ai-toolkit-slack-oauth-backend Ready Ready Preview, Comment Jul 16, 2026 1:32pm

Request Review

@github-actions

github-actions Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

🤖 Claude Code Review

Review complete

Summary

Small, correct fix. This changes the PostToolUse hook matcher from Bash|create_pull_request to Bash|mcp__.+__create_pull_request so the "offer /skill-mine" nudge actually fires when a PR is opened via a namespaced MCP tool.

Why the fix is correct

  • The old matcher contained only "simple" characters (letters, digits, _, -, |), so it was evaluated as a list of exact tool names. create_pull_request never equals mcp__github__create_pull_request, so MCP-driven PR opens silently skipped the hook.
  • Adding .+ introduces regex metacharacters, switching the matcher to (unanchored) regex mode, which correctly matches mcp__<server>__create_pull_request.
  • The hook script's own isPrOpen guard (toolName.endsWith('create_pull_request'), pr-skill-doctor-prompt.cjs:84) then does the precise per-invocation filtering, so any over-broad prefilter matches are harmless no-ops.
  • Verified the greedy .+ handles both observed name shapes (mcp__github__create_pull_request and mcp__plugin_uniswap-integrations_github__create_pull_request).

Version bump (1.0.1 → 1.0.2, patch) and the CLAUDE.md version table are both updated, consistent with the repo's mandatory version-bump rule. The added CLAUDE.md paragraph documenting the load-bearing .+ is a genuinely useful note for future maintainers.

Non-blocking observation

Because the matcher is now unanchored regex, the Bash alternative also substring-matches sibling tools like BashOutput and KillBash, so the hook process will now spawn on those calls too. It's functionally harmless — isPrOpen returns false for them and the hook exits 0 — but if you want to avoid the extra process spawns you could anchor the alternatives, e.g. ^Bash$|^mcp__.+__create_pull_request$ (anchors keep it in regex mode while restoring exact Bash matching). Purely optional.

No bugs, security, or data-loss concerns. Safe to merge.

Files reviewed

  • packages/plugins/skill-management/hooks/hooks.json
  • packages/plugins/skill-management/hooks/pr-skill-doctor-prompt.cjs (context)
  • packages/plugins/skill-management/CLAUDE.md
  • CLAUDE.md
  • packages/plugins/skill-management/.claude-plugin/plugin.json

💡 Want a fresh review? Add a comment containing @request-claude-review to trigger a new review at any time.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📋 Review verdict: APPROVE

👆 The main review comment above is the source of truth for this PR review. It is automatically updated on each review cycle, so always refer to it for the most current feedback.

This formal review submission is for the verdict only.

@wkoutre

wkoutre commented Jul 16, 2026

Copy link
Copy Markdown
Contributor Author

Re the non-blocking observation on anchoring (^Bash$|^mcp__.+__create_pull_request$): agreed it would avoid the no-op hook spawns on BashOutput/KillBash, but keeping the unanchored form as-is for this PR. Two reasons: it matches the form already shipped and verified in Utility-NYC/closet-ai#57, and the extra matches are exit-0 no-ops that the script's isPrOpen guard filters (as you traced), so the anchor buys only a few avoided node process spawns per session. Happy to anchor in a follow-up if hook spawn overhead ever becomes measurable.

Note on the red docs-check: it failed on infrastructure, not this diff. The action is pinned to model claude-sonnet-4-6, which the API rejected ("may not exist or you may not have access to it"), so the check errored before producing any docs verdict. This PR satisfies the check's stated rules (plugin version bumped, docs updated in the same commit). The model pin needs a separate fix since it will fail every PR identically.

@wkoutre
wkoutre merged commit 6c0b8a8 into next Jul 16, 2026
19 of 20 checks passed
@wkoutre
wkoutre deleted the nickkoutrelakos/fix-skill-mgmt-hook-mcp-matcher branch July 16, 2026 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant