fix: require-pr-before-stop falsely denies on merged PR with stale origin

[NiveditJain]

Summary

• When a PR is merged but origin/main hasn't been fetched since, the require-pr-before-stop policy incorrectly denies and instructs Claude to create a new PR
• Root cause: the early-exit checks (git log / git diff) compare against a stale origin/{baseBranch} ref that doesn't reflect the merge
• Fix: when gh pr view returns MERGED state, fetch origin/{baseBranch} with an explicit refspec and re-verify the divergence checks before denying

Test plan

• New unit test: allows when PR is merged and branch is up to date after fetch (regular merge)
• New unit test: allows when PR is merged and no file diff after fetch (squash merge)
• New unit test: denies when PR is merged but new work exists after fetch
• New unit test: falls through to deny when fetch fails on merged PR
• Existing test updated: "denies when PR is merged and file changes exist after fetch" still passes
• All 937 unit tests pass
• Build succeeds
• CHANGELOG updated for 0.0.6-beta.0

🤖 Generated with Claude Code

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR fixes the require-pr-before-stop policy to correctly handle merged PRs when the remote is stale. After an initial check fails, the policy now runs git fetch and re-verifies whether the branch is up to date by checking commit differences and file changes, returning allow if applicable.

Changes

Cohort / File(s)	Summary
Bug Fix Implementation src/hooks/builtin-policies.ts	Added re-verification logic to requirePrBeforeStop for merged PRs: runs git fetch origin <baseBranch>, checks if commits exist ahead, and verifies file changes using git diff --stat. Returns allow if branch is up to date; otherwise falls through to deny.
Test Updates __tests__/hooks/builtin-policies.test.ts	Renamed existing test for clarity and added three new test cases: merged PR with fetch (allows with "up to date"), squash-merged PR (allows with "no file changes"), and fetch failure scenario (denies). Expanded mocking to cover fetch, commit checks, and diff operations.
Documentation CHANGELOG.md	Added entry under "Unreleased → Fixes" noting correction to require-pr-before-stop behavior for merged PRs with stale remote branches.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• PR #109: Modifies the same requirePrBeforeStop policy and builtin-policies implementation (rewording deny messages).
• PR #63: Originally added the require-pr-before-stop policy that this PR extends with new merge-recheck logic.
• PR #71: Implements similar upstream-vs-HEAD verification (commit-ahead and diff --stat checks) for other stop policies.

Poem

🐰 A PR once merged, yet denied to pause,
Stale origin caused quite the clause!
Now fetch and check make truth prevail,
Branch up to date? No more sad tail!
Hoppy fix for mixed git state! 🌱

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely summarizes the main fix: addressing a false denial in require-pr-before-stop when a PR is merged but origin is stale.
Description check	✅ Passed	The PR description clearly explains the issue, root cause, fix, and test plan with checkmarks indicating all tests pass and checklist items are verified.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch luv-112

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

__tests__/hooks/builtin-policies.test.ts (1)

2190-2200: Assert the exact fetch target in the merged-PR tests.

These mocks treat any fetch as refreshing origin/main, so they won’t catch a regression where the command fetches only FETCH_HEAD/uses the wrong refmap. Please assert the fetch updates refs/remotes/origin/${baseBranch} explicitly, especially if the production code switches to an explicit refspec.

🧪 Example assertion

       const result = await policy.fn(ctx);
       expect(result.decision).toBe("allow");
+      expect(execFileSync).toHaveBeenCalledWith(
+        "git",
+        ["fetch", "origin", "+refs/heads/main:refs/remotes/origin/main"],
+        expect.objectContaining({ cwd: "/repo" }),
+      );
       expect(result.reason).toContain("was merged");

As per coding guidelines, **/__tests__/**/*.{ts,tsx,js,jsx}: “Always add unit tests for new behaviour. Place tests in tests/.”

Also applies to: 2218-2228, 2245-2251

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@__tests__/hooks/builtin-policies.test.ts` around lines 2190 - 2200, The
execFileSync test mock treats any fetch as a refresh and should assert the exact
refspec; update the mockImplementation for execFileSync so it only sets the
fetched flag when the args include both "fetch" and the explicit ref target
(refs/remotes/origin/${baseBranch} or the exact refspec your production code
should send), i.e., check args.join(" ") contains the expected refspec before
setting fetched = true; apply the same stricter check to the other similar mocks
(the ones around lines referencing execFileSync/fetched noted in the comment) so
tests will fail if code fetches FETCH_HEAD or the wrong refmap.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/hooks/builtin-policies.ts`:
- Around line 1071-1087: The git fetch call using just the branch name doesn't
update refs/remotes/origin/${baseBranch}, causing subsequent execFileSync checks
(the git log that builds freshAhead and the git diff that builds freshDiff) to
operate on a stale remote-tracking ref; change the execFileSync("git", ["fetch",
"origin", baseBranch], ...) invocation to fetch with an explicit refspec (e.g.
refs/heads/${baseBranch}:refs/remotes/origin/${baseBranch}) while preserving
cwd/encoding/timeout so that origin/${baseBranch} is guaranteed to be updated
before the git log and git diff checks that decide whether to call allow(`PR
#${pr.number} ...`).

---

Nitpick comments:
In `@__tests__/hooks/builtin-policies.test.ts`:
- Around line 2190-2200: The execFileSync test mock treats any fetch as a
refresh and should assert the exact refspec; update the mockImplementation for
execFileSync so it only sets the fetched flag when the args include both "fetch"
and the explicit ref target (refs/remotes/origin/${baseBranch} or the exact
refspec your production code should send), i.e., check args.join(" ") contains
the expected refspec before setting fetched = true; apply the same stricter
check to the other similar mocks (the ones around lines referencing
execFileSync/fetched noted in the comment) so tests will fail if code fetches
FETCH_HEAD or the wrong refmap.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c94c1863-5395-46ce-bc19-bdb5211d137f

📥 Commits

Reviewing files that changed from the base of the PR and between e8d45e7 and 69fec11.

📒 Files selected for processing (3)

• CHANGELOG.md
• __tests__/hooks/builtin-policies.test.ts
• src/hooks/builtin-policies.ts