Skip to content

fix(hooks): scope PR --base guard to the configured project repo#2014

Open
RapierCraft wants to merge 1 commit into
mainfrom
fix/pr-base-guard-external-repo
Open

fix(hooks): scope PR --base guard to the configured project repo#2014
RapierCraft wants to merge 1 commit into
mainfrom
fix/pr-base-guard-external-repo

Conversation

@RapierCraft

Copy link
Copy Markdown
Contributor

Problem

The PreToolUse hook's --base guard blocks every gh pr create --base main, regardless of which repository is targeted. checkPrTarget() extracts --base but never --repo/-R, so it has no notion of repo scope and governs every repo on the machine.

The rule exists to protect this project's deploy pipeline, where main is the deploy trigger. Applied to a third-party repo it is a guaranteed false positive — an upstream awesome-list or a docs typo fix typically has main as its only valid base and no staging branch at all. The result: legitimate external contribution is impossible from any ForgeDock-managed directory.

Found in the wild: an AlterLab issue whose entire remaining work was a one-line PR to punkpeye/awesome-mcp-servers sat stranded at needs-human, with its own devdoc recording the cause — "blocked by this repo's own main-branch pipeline guardrail (which hard-blocks any --base main, including on unrelated external repos)". Several sibling registry-submission issues are stuck the same way.

This is invisible to ForgeDock's own maintainers because this repo's worktree has no forge.yaml (gitignored), so isForgeDockManagedCwd() returns false and the hook self-exempts.

Fix

Skip the guard when an explicit --repo/-R names a repo other than project.owner/project.repo from the cwd's forge.yaml.

Deliberately fail-closed. An undeterminable slug (no forge.yaml, or no project.owner/repo in it) falls through to enforcement rather than skipping it, so a malformed config can never silently disarm the guard on the repo it is meant to protect. Only a positive mismatch — an explicit -R naming a repo demonstrably not ours — is exempted.

Slug reading is a line-anchored regex, not a YAML parse: this hook runs on every tool call and must stay synchronous and dependency-free. The ^\s*owner: anchor cannot match a commented # owner: line, so the commented repos: template block forgedock init emits is ignored (covered by a test).

Verification

node --test bin/tests/pre-tool-use.test.mjs57/57 pass (50 pre-existing + 7 new, no regressions).

Verified against a real managed project (rapiercraftstudios/alterlab):

Case Before After
-R punkpeye/awesome-mcp-servers --base main BLOCKED allowed
-R RapierCraftStudios/AlterLab --base main blocked still blocked
bare --base main (implicit own repo) blocked still blocked
-R RapierCraftStudios/AlterLab --base staging allowed still allowed

This PR is itself end-to-end proof: opening it required the patched hook, since the stock hook blocks --base main on this repo when invoked from a managed cwd.

Tests added

7 subprocess regression tests (against the real hook via runHook(), not the duplicated pure-logic copy):

  • external repo exempt — --repo long form and -R short form
  • own repo still blocked — explicit, case-insensitive, and implicit (no -R)
  • undeterminable slug fails closed
  • commented-out repos: template block does not poison slug parsing

Note for maintainers

bin/tests/pre-tool-use.test.mjs keeps its own duplicate copy of checkPrTarget for the "pure logic" describe block, so those tests exercise a copy that can drift from the real hook. I left it alone and put all new coverage in the runHook() subprocess block, which tests the real file — but the duplication looks worth collapsing separately.

The PreToolUse --base guard blocked every 'gh pr create --base main',
regardless of which repository was targeted. It extracted --base but never
--repo/-R, so it had no concept of repo scope.

The rule exists to protect the project's own deploy pipeline, where main is
the deploy trigger. Applied to a third-party repo it is a guaranteed false
positive: an upstream awesome-list or docs fix typically has main as its only
valid base and no staging branch at all, making legitimate external
contribution impossible.

Skip the guard when an explicit --repo/-R names a repo other than
project.owner/project.repo from the cwd's forge.yaml. An undeterminable slug
falls through to enforcement, so a malformed config cannot silently disarm the
guard on the repo it protects.

Adds 7 subprocess regression tests covering the external-repo exemption, the
own-repo (explicit, case-insensitive, and implicit) still-blocked paths, the
fail-closed undeterminable-slug case, and the commented-out repos: template
block that forgedock init emits.
@RapierCraft

Copy link
Copy Markdown
Contributor Author

Temporarily closing/reopening to trigger a fresh DCO run — verifying that the inline DCO rewrite (now on main via the #2204 deploy) fixes the pull_request_target bootstrap tracked in #2155. No change to this PR's content.

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