From de0e96f5132e33f58c30b86a0ebc97080ce0b273 Mon Sep 17 00:00:00 2001 From: NiveditJain Date: Mon, 27 Apr 2026 13:42:44 -0700 Subject: [PATCH] [luv-198] fix: skip require-no-conflicts-before-stop when no OPEN PR exists MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The policy used to run its local git merge-tree probe (Layer 1) unconditionally, so a branch with no PR on GitHub — or only a closed/merged one — could still receive a deny referencing a PR URL the user never created. Promote the gh + PR-state precheck above Layer 1 so the policy short-circuits to allow whenever there is no confirmable OPEN merge target, and reuse the fetched pr in Layer 2 to avoid a second gh round-trip. When gh CLI is unavailable, the entire policy is skipped (per user direction) — without gh we cannot confirm a PR exists. Co-Authored-By: Claude Opus 4.7 --- CHANGELOG.md | 3 + __tests__/hooks/builtin-policies.test.ts | 40 ++++++++---- docs/built-in-policies.mdx | 13 ++-- src/hooks/builtin-policies.ts | 78 +++++++++++------------- 4 files changed, 73 insertions(+), 61 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e8436379..3531f0aa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,9 @@ ## Unreleased +### Fixes +- Skip `require-no-conflicts-before-stop` entirely when no OPEN PR exists for the current branch (or when `gh` CLI is unavailable to check). The policy no longer runs Layer 1's local `git merge-tree` probe in those cases — without a confirmable merge target there is nothing to enforce (#198). + ## 0.0.6 — 2026-04-27 ### Features diff --git a/__tests__/hooks/builtin-policies.test.ts b/__tests__/hooks/builtin-policies.test.ts index 5704dd53..ba18ea75 100644 --- a/__tests__/hooks/builtin-policies.test.ts +++ b/__tests__/hooks/builtin-policies.test.ts @@ -2670,11 +2670,12 @@ describe("hooks/builtin-policies", () => { return "/usr/bin/gh\n"; } if (typeof cmd === "string" && cmd.includes("gh pr view")) { - if (opts.prResult === null || opts.prResult === undefined) { + if (opts.prResult === null) { throw new Error("no pull requests found"); } if (opts.prResult === "invalid-json") return "not-json"; - return JSON.stringify({ state: "OPEN", ...opts.prResult }); + const prData = opts.prResult ?? { mergeable: "MERGEABLE", number: 1, url: "https://github.com/org/repo/pull/1" }; + return JSON.stringify({ state: "OPEN", ...prData }); } return ""; }); @@ -2836,22 +2837,22 @@ describe("hooks/builtin-policies", () => { expect(result.reason).toContain("skipping conflict check"); }); - it("allows with local+no-gh message when Layer 1 clean and gh not installed", async () => { - mockConflictsScenario({ mergeTreeStatus: 0, ghInstalled: false }); + it("skips entirely when gh is not installed (no Layer 1 even if local would conflict)", async () => { + mockConflictsScenario({ mergeTreeStatus: 1, ghInstalled: false }); const ctx = makeCtx({ eventType: "Stop", session: { cwd: "/repo" } }); const result = await policy.fn(ctx); expect(result.decision).toBe("allow"); - expect(result.reason).toContain("merges cleanly with main locally"); expect(result.reason).toContain("gh CLI not installed"); + expect(result.reason).toContain("skipping conflict check"); }); - it("allows with local+no-PR message when Layer 1 clean and no PR exists", async () => { - mockConflictsScenario({ mergeTreeStatus: 0, prResult: null }); + it("skips entirely when no PR exists (no Layer 1 even if local would conflict)", async () => { + mockConflictsScenario({ mergeTreeStatus: 1, prResult: null }); const ctx = makeCtx({ eventType: "Stop", session: { cwd: "/repo" } }); const result = await policy.fn(ctx); expect(result.decision).toBe("allow"); - expect(result.reason).toContain("merges cleanly with main locally"); - expect(result.reason).toContain("no PR to verify"); + expect(result.reason).toContain("No pull request found for branch"); + expect(result.reason).toContain("skipping conflict check"); }); it("falls through to Layer 2 when origin/main ref missing; denies if gh reports CONFLICTING", async () => { @@ -2876,11 +2877,28 @@ describe("hooks/builtin-policies", () => { expect(result.reason).toContain("PR #42"); }); - it("fails open when Layer 1 fails (exit != 0/1), then allows if both layers skipped", async () => { - mockConflictsScenario({ mergeTreeStatus: "error", ghInstalled: false }); + it("Layer 1 non-conflict failures fall through to Layer 2 using the cached PR data", async () => { + mockConflictsScenario({ + mergeTreeStatus: "error", + prResult: { mergeable: "MERGEABLE", number: 42, url: "https://github.com/org/repo/pull/42" }, + }); + const ctx = makeCtx({ eventType: "Stop", session: { cwd: "/repo" } }); + const result = await policy.fn(ctx); + expect(result.decision).toBe("allow"); + expect(result.reason).toContain("PR #42"); + expect(result.reason).toContain("merges cleanly per GitHub"); + }); + + it("skips Layer 1 when PR exists but is CLOSED (even if local would conflict)", async () => { + mockConflictsScenario({ + mergeTreeStatus: 1, + prResult: { mergeable: "UNKNOWN", state: "CLOSED", number: 42, url: "https://github.com/org/repo/pull/42" }, + }); const ctx = makeCtx({ eventType: "Stop", session: { cwd: "/repo" } }); const result = await policy.fn(ctx); expect(result.decision).toBe("allow"); + expect(result.reason).toContain("PR #42"); + expect(result.reason).toContain("closed"); expect(result.reason).toContain("skipping conflict check"); }); diff --git a/docs/built-in-policies.mdx b/docs/built-in-policies.mdx index fafe7eec..7f336ff8 100644 --- a/docs/built-in-policies.mdx +++ b/docs/built-in-policies.mdx @@ -567,12 +567,12 @@ pull requests. If `gh` is not installed or not authenticated, the policy fails o ### `require-no-conflicts-before-stop` **Event:** Stop -**Default:** Denies stopping when the current branch cannot cleanly merge into the base branch. Runs two independent probes: +**Default:** Denies stopping when the current branch cannot cleanly merge into the base branch. The policy first confirms there is an `OPEN` PR on GitHub for the branch — without one, there is no merge target to enforce, so the entire policy short-circuits to allow. Once an `OPEN` PR is confirmed, two independent probes run: 1. **Local** — `git merge-tree --write-tree --name-only origin/ HEAD`. On conflict, the deny message names the conflicted files so Claude knows exactly what to resolve. -2. **GitHub** — `gh pr view --json mergeable,state`. Catches conflicts that a stale local `origin/` would miss (e.g. someone landed a conflicting PR on `main` since the last fetch). A `CONFLICTING` result denies. An `UNKNOWN` result on an `OPEN` PR (GitHub still computing) also denies and instructs Claude to wait ~10 seconds and re-check before attempting to stop again — this prevents false negatives while GitHub recomputes. The check is skipped entirely for any PR whose state is not `OPEN` (e.g. `MERGED`, `CLOSED`), since GitHub stops computing mergeability for non-open PRs. +2. **GitHub** — reuses the `gh pr view --json mergeable,state` result already fetched in the precheck. Catches conflicts that a stale local `origin/` would miss (e.g. someone landed a conflicting PR on `main` since the last fetch). A `CONFLICTING` result denies. An `UNKNOWN` result also denies and instructs Claude to wait ~10 seconds and re-check before attempting to stop again — this prevents false negatives while GitHub recomputes. -Fails open when `origin/` is missing locally, when no commits are ahead of base, when `gh` is not installed, when no PR exists for the branch, or when the PR is not OPEN. +Skips entirely (allows) when: `gh` is not installed, no PR exists for the branch, the PR's state is not `OPEN` (e.g. `MERGED`, `CLOSED`), or `gh pr view` returns unparseable output. Also fails open when `origin/` is missing locally or when no commits are ahead of base — those Layer 1 fall-throughs still consult the cached PR mergeability before allowing. **Parameters:** @@ -581,9 +581,10 @@ Fails open when `origin/` is missing locally, when no commits are ah | `baseBranch` | `string` | `"main"` | Base branch to check for conflicts against. | -GitHub CLI (`gh`) is optional but recommended for this policy — it provides the second -detection layer that catches conflicts a stale local `origin/` would miss. -Without `gh`, only the local `git merge-tree` check runs. +GitHub CLI (`gh`) is required for this policy. The policy uses `gh pr view` to confirm +an `OPEN` PR exists before running any conflict probe — without `gh`, the policy +short-circuits to allow. Run `gh auth login` with a personal access token that has +`repo` scope for read access to pull requests. --- diff --git a/src/hooks/builtin-policies.ts b/src/hooks/builtin-policies.ts index e07c512a..967f4f30 100644 --- a/src/hooks/builtin-policies.ts +++ b/src/hooks/builtin-policies.ts @@ -1197,8 +1197,37 @@ function requireNoConflictsBeforeStop(ctx: PolicyContext): PolicyResult { return allow(`On base branch "${baseBranch}", skipping conflict check.`); } + // -- Precheck: only enforce when an OPEN PR exists on GitHub. Without a + // confirmable merge target there is nothing to enforce, so we skip both + // the local merge-tree probe and the GitHub mergeability probe. + try { + execSync("gh --version", { cwd, encoding: "utf8", stdio: ["pipe", "pipe", "pipe"], timeout: 3000 }); + } catch { + return allow("gh CLI not installed, skipping conflict check."); + } + + let prJson: string; + try { + prJson = execSync("gh pr view --json mergeable,number,url,state", { + cwd, encoding: "utf8", stdio: ["pipe", "pipe", "pipe"], timeout: 15000, + }).trim(); + } catch { + return allow("No pull request found for branch, skipping conflict check."); + } + + let pr: { mergeable: string; number: number; url: string; state: string }; + try { + pr = JSON.parse(prJson); + } catch { + return allow("Could not parse gh pr view output, skipping conflict check."); + } + + // GitHub stops computing mergeability for non-OPEN PRs (returns UNKNOWN forever). + if (pr.state !== "OPEN") { + return allow(`PR #${pr.number} is ${pr.state.toLowerCase()}; skipping conflict check.`); + } + // -- Layer 1: local git merge-tree -- - let localSkipped = false; try { execFileSync("git", ["rev-parse", "--verify", `origin/${baseBranch}`], { cwd, encoding: "utf8", stdio: ["pipe", "pipe", "pipe"], timeout: 3000, @@ -1209,17 +1238,14 @@ function requireNoConflictsBeforeStop(ctx: PolicyContext): PolicyResult { { cwd, encoding: "utf8", stdio: ["pipe", "pipe", "pipe"], timeout: 5000 }, ).trim(); - if (!ahead) { - // Nothing ahead of base — Layer 1 doesn't apply, fall through to Layer 2. - localSkipped = true; - } else { + if (ahead) { execFileSync( "git", ["merge-tree", "--write-tree", "--name-only", `origin/${baseBranch}`, "HEAD"], { cwd, encoding: "utf8", stdio: ["pipe", "pipe", "pipe"], timeout: 10000 }, ); - // exit 0 → clean merge, fall through to Layer 2 } + // !ahead or merge-tree exit 0 → fall through to Layer 2 } catch (err) { const e = err as { status?: number; stdout?: string | Buffer }; if (e.status === 1) { @@ -1238,46 +1264,10 @@ function requireNoConflictsBeforeStop(ctx: PolicyContext): PolicyResult { `Rebase or merge origin/${baseBranch} now and resolve the conflicts.`, ); } - localSkipped = true; - } - - // -- Layer 2: GitHub PR mergeability -- - try { - execSync("gh --version", { cwd, encoding: "utf8", stdio: ["pipe", "pipe", "pipe"], timeout: 3000 }); - } catch { - return allow( - localSkipped - ? "Local conflict check skipped and gh CLI not installed, skipping conflict check." - : `Branch "${branch}" merges cleanly with ${baseBranch} locally (gh CLI not installed, PR mergeability not verified).`, - ); - } - - let prJson: string; - try { - prJson = execSync("gh pr view --json mergeable,number,url,state", { - cwd, encoding: "utf8", stdio: ["pipe", "pipe", "pipe"], timeout: 15000, - }).trim(); - } catch { - return allow( - localSkipped - ? "No pull request found for branch, skipping conflict check." - : `Branch "${branch}" merges cleanly with ${baseBranch} locally (no PR to verify against).`, - ); - } - - let pr: { mergeable: string; number: number; url: string; state: string }; - try { - pr = JSON.parse(prJson); - } catch { - return allow("Could not parse gh pr view output, skipping PR mergeability check."); - } - - // GitHub stops computing mergeability for non-OPEN PRs (returns UNKNOWN forever). - // Skip the check entirely so a merged or closed PR doesn't trap Stop in a wait loop. - if (pr.state !== "OPEN") { - return allow(`PR #${pr.number} is ${pr.state.toLowerCase()}; skipping conflict check.`); + // any other failure (e.g. missing origin/, log failure) → fall through } + // -- Layer 2: GitHub PR mergeability (reuses pr from precheck) -- if (pr.mergeable === "CONFLICTING") { return deny( `PR #${pr.number} has merge conflicts per GitHub (${pr.url}). ` +