diff --git a/CHANGELOG.md b/CHANGELOG.md index a99986f2..728bf376 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ ### Features - Add cloud platform client: `login`, `logout`, `whoami`, `relay start|stop|status`, and `sync` subcommands. Hook events are appended to a local queue and streamed to the failproofai cloud server via a background relay daemon that lazy-starts from the hook handler and survives reboots (#132) +- Add `require-no-conflicts-before-stop` builtin workflow policy that denies Stop until the current branch merges cleanly with the base branch. Runs a local `git merge-tree` probe (names the conflicted files) and an optional `gh pr view --json mergeable` probe that catches conflicts a stale local `origin/` would miss (#176) ### Fixes - Stop stderr leakage from workflow policies (`require-push-before-stop`, `require-pr-before-stop`, `require-ci-green-before-stop`, etc.): git probes that are expected to sometimes fail no longer leak "fatal: Needed a single revision" or similar messages to the user's terminal (#132) diff --git a/__tests__/hooks/builtin-policies.test.ts b/__tests__/hooks/builtin-policies.test.ts index 10f03b2c..2a171a95 100644 --- a/__tests__/hooks/builtin-policies.test.ts +++ b/__tests__/hooks/builtin-policies.test.ts @@ -34,8 +34,8 @@ describe("hooks/builtin-policies", () => { }); describe("BUILTIN_POLICIES", () => { - it("has 31 built-in policies", () => { - expect(BUILTIN_POLICIES).toHaveLength(31); + it("has 32 built-in policies", () => { + expect(BUILTIN_POLICIES).toHaveLength(32); }); it("has 11 default-enabled policies", () => { @@ -1917,12 +1917,13 @@ describe("hooks/builtin-policies", () => { describe("workflow policy metadata", () => { const workflowPolicies = BUILTIN_POLICIES.filter((p) => p.category === "Workflow"); - it("all 4 workflow policies exist", () => { - expect(workflowPolicies).toHaveLength(4); + it("all 5 workflow policies exist", () => { + expect(workflowPolicies).toHaveLength(5); const names = workflowPolicies.map((p) => p.name).sort(); expect(names).toEqual([ "require-ci-green-before-stop", "require-commit-before-stop", + "require-no-conflicts-before-stop", "require-pr-before-stop", "require-push-before-stop", ]); @@ -1946,11 +1947,15 @@ describe("hooks/builtin-policies", () => { } }); - it("require-push-before-stop and require-pr-before-stop have params schemas", () => { + it("require-push-before-stop, require-pr-before-stop, and require-no-conflicts-before-stop have params schemas", () => { const withParams = workflowPolicies.filter((p) => p.params); - expect(withParams).toHaveLength(2); + expect(withParams).toHaveLength(3); const names = withParams.map((p) => p.name).sort(); - expect(names).toEqual(["require-pr-before-stop", "require-push-before-stop"]); + expect(names).toEqual([ + "require-no-conflicts-before-stop", + "require-pr-before-stop", + "require-push-before-stop", + ]); const pushPolicy = withParams.find((p) => p.name === "require-push-before-stop")!; expect(pushPolicy.params!.remote).toBeDefined(); @@ -1961,6 +1966,10 @@ describe("hooks/builtin-policies", () => { const prPolicy = withParams.find((p) => p.name === "require-pr-before-stop")!; expect(prPolicy.params!.baseBranch).toBeDefined(); expect(prPolicy.params!.baseBranch.default).toBe("main"); + + const conflictsPolicy = withParams.find((p) => p.name === "require-no-conflicts-before-stop")!; + expect(conflictsPolicy.params!.baseBranch).toBeDefined(); + expect(conflictsPolicy.params!.baseBranch.default).toBe("main"); }); }); @@ -2607,6 +2616,238 @@ describe("hooks/builtin-policies", () => { }); }); + describe("require-no-conflicts-before-stop", () => { + const policy = BUILTIN_POLICIES.find((p) => p.name === "require-no-conflicts-before-stop")!; + + afterEach(() => { + vi.mocked(execSync).mockReset(); + vi.mocked(execFileSync).mockReset(); + clearGitBranchCache(); + }); + + /** + * Mock helper: sets up execSync (for getCurrentBranch + gh commands) and + * execFileSync (for git rev-parse, git log, git merge-tree) around the + * common scenarios this policy handles. + */ + function mockConflictsScenario(opts: { + branch?: string; + baseRefExists?: boolean; + commitsAhead?: string; + mergeTreeStatus?: 0 | 1 | "error"; + mergeTreeStdout?: string; + ghInstalled?: boolean; + prResult?: { mergeable: string; number: number; url: string } | null | "invalid-json"; + }) { + const branch = opts.branch ?? "feat/branch"; + const ghInstalled = opts.ghInstalled ?? true; + + vi.mocked(execSync).mockImplementation((cmd: string) => { + if (typeof cmd === "string" && cmd.includes("rev-parse --abbrev-ref")) { + return `${branch}\n`; + } + if (typeof cmd === "string" && cmd.includes("gh --version")) { + if (!ghInstalled) throw new Error("not found"); + return "/usr/bin/gh\n"; + } + if (typeof cmd === "string" && cmd.includes("gh pr view")) { + if (opts.prResult === null || opts.prResult === undefined) { + throw new Error("no pull requests found"); + } + if (opts.prResult === "invalid-json") return "not-json"; + return JSON.stringify(opts.prResult); + } + return ""; + }); + + vi.mocked(execFileSync).mockImplementation((_cmd: string, args?: readonly string[]) => { + const joined = args?.join(" ") ?? ""; + if (joined.includes("rev-parse") && joined.includes("--verify")) { + if (opts.baseRefExists === false) throw new Error("unknown revision"); + return ""; + } + if (joined.includes("log") && joined.includes("..HEAD")) { + return opts.commitsAhead ?? "abc123 commit\n"; + } + if (joined.includes("merge-tree")) { + if (opts.mergeTreeStatus === 1) { + const err = new Error("conflict") as Error & { status: number; stdout: string }; + err.status = 1; + err.stdout = opts.mergeTreeStdout ?? "treeoid\na.ts\nb.ts\n\nCONFLICT (content): Merge conflict in a.ts\n"; + throw err; + } + if (opts.mergeTreeStatus === "error") { + const err = new Error("merge-tree failed") as Error & { status: number }; + err.status = 128; + throw err; + } + return "treeoid\n"; + } + return ""; + }); + } + + it("allows when session.cwd is missing", async () => { + const ctx = makeCtx({ eventType: "Stop", session: {} }); + const result = await policy.fn(ctx); + expect(result.decision).toBe("allow"); + expect(result.reason).toContain("No working directory"); + }); + + it("allows on detached HEAD", async () => { + vi.mocked(execSync).mockImplementation((cmd: string) => { + if (typeof cmd === "string" && cmd.includes("rev-parse --abbrev-ref")) return "HEAD\n"; + return ""; + }); + const ctx = makeCtx({ eventType: "Stop", session: { cwd: "/repo" } }); + const result = await policy.fn(ctx); + expect(result.decision).toBe("allow"); + expect(result.reason).toContain("Detached HEAD"); + }); + + it("allows when on base branch", async () => { + mockConflictsScenario({ branch: "main" }); + const ctx = makeCtx({ eventType: "Stop", session: { cwd: "/repo" } }); + const result = await policy.fn(ctx); + expect(result.decision).toBe("allow"); + expect(result.reason).toContain('On base branch "main"'); + }); + + it("denies with conflict file list when merge-tree exits 1", async () => { + mockConflictsScenario({ + mergeTreeStatus: 1, + mergeTreeStdout: "treeoid123\nsrc/app.ts\nsrc/lib.ts\n\nCONFLICT (content): ...\n", + }); + const ctx = makeCtx({ eventType: "Stop", session: { cwd: "/repo" } }); + const result = await policy.fn(ctx); + expect(result.decision).toBe("deny"); + expect(result.reason).toContain('"feat/branch"'); + expect(result.reason).toContain("src/app.ts, src/lib.ts"); + expect(result.reason).toContain("Rebase or merge origin/main"); + }); + + it("deny reason falls back to \"one or more files\" when stdout parse yields no files", async () => { + mockConflictsScenario({ + mergeTreeStatus: 1, + mergeTreeStdout: "treeoid-only-no-newline", + }); + const ctx = makeCtx({ eventType: "Stop", session: { cwd: "/repo" } }); + const result = await policy.fn(ctx); + expect(result.decision).toBe("deny"); + expect(result.reason).toContain("one or more files"); + }); + + it("custom baseBranch param is honored", async () => { + mockConflictsScenario({ + mergeTreeStatus: 1, + mergeTreeStdout: "treeoid\nfoo.ts\n\n", + }); + const ctx = makeCtx({ + eventType: "Stop", + session: { cwd: "/repo" }, + params: { baseBranch: "develop" }, + }); + const result = await policy.fn(ctx); + expect(result.decision).toBe("deny"); + expect(result.reason).toContain("conflicts with develop"); + expect(result.reason).toContain("origin/develop"); + }); + + it("allows with MERGEABLE message when Layer 1 clean and gh reports MERGEABLE", async () => { + mockConflictsScenario({ + mergeTreeStatus: 0, + 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("denies when Layer 1 clean but gh reports CONFLICTING (catches stale-origin case)", async () => { + mockConflictsScenario({ + mergeTreeStatus: 0, + prResult: { mergeable: "CONFLICTING", 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("deny"); + expect(result.reason).toContain("PR #42"); + expect(result.reason).toContain("conflicts per GitHub"); + expect(result.reason).toContain("https://github.com/org/repo/pull/42"); + }); + + it("denies with wait+retry when gh reports UNKNOWN", async () => { + mockConflictsScenario({ + mergeTreeStatus: 0, + prResult: { mergeable: "UNKNOWN", 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("deny"); + expect(result.reason).toContain("still computing mergeability"); + expect(result.reason).toContain("Wait"); + expect(result.reason).toContain("gh pr view --json mergeable"); + }); + + it("allows with local+no-gh message when Layer 1 clean and gh not installed", async () => { + mockConflictsScenario({ mergeTreeStatus: 0, 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"); + }); + + it("allows with local+no-PR message when Layer 1 clean and no PR exists", async () => { + mockConflictsScenario({ mergeTreeStatus: 0, 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"); + }); + + it("falls through to Layer 2 when origin/main ref missing; denies if gh reports CONFLICTING", async () => { + mockConflictsScenario({ + baseRefExists: false, + prResult: { mergeable: "CONFLICTING", 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("deny"); + expect(result.reason).toContain("conflicts per GitHub"); + }); + + it("falls through to Layer 2 when branch has no commits ahead; allows if gh reports MERGEABLE", async () => { + mockConflictsScenario({ + commitsAhead: "", + 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"); + }); + + it("fails open when Layer 1 fails (exit != 0/1), then allows if both layers skipped", async () => { + mockConflictsScenario({ mergeTreeStatus: "error", 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("skipping conflict check"); + }); + + it("allows when gh pr view returns invalid JSON (fail-open)", async () => { + mockConflictsScenario({ mergeTreeStatus: 0, prResult: "invalid-json" }); + const ctx = makeCtx({ eventType: "Stop", session: { cwd: "/repo" } }); + const result = await policy.fn(ctx); + expect(result.decision).toBe("allow"); + expect(result.reason).toContain("Could not parse gh pr view output"); + }); + }); + describe("require-ci-green-before-stop", () => { const policy = BUILTIN_POLICIES.find((p) => p.name === "require-ci-green-before-stop")!; diff --git a/docs/built-in-policies.mdx b/docs/built-in-policies.mdx index 361aa4bd..1088459b 100644 --- a/docs/built-in-policies.mdx +++ b/docs/built-in-policies.mdx @@ -22,7 +22,7 @@ Policies are grouped into categories: | [Database](#database) | warn-destructive-sql, warn-schema-alteration | PreToolUse | | [Warnings](#warnings) | warn-large-file-write, warn-package-publish, warn-background-process, warn-global-package-install | PreToolUse | | [Package managers](#package-managers) | prefer-package-manager | PreToolUse | -| [Workflow](#workflow) | require-commit-before-stop, require-push-before-stop, require-pr-before-stop, require-ci-green-before-stop | Stop | +| [Workflow](#workflow) | require-commit-before-stop, require-push-before-stop, require-pr-before-stop, require-no-conflicts-before-stop, require-ci-green-before-stop | Stop | - **`block-`** — stop the agent from proceeding. - **`warn-`** — give the agent additional context so it can self-correct. @@ -541,6 +541,30 @@ 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: + +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`. 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 (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. + +Fails open when `origin/` is missing locally, when no commits are ahead of base, when `gh` is not installed, or when no PR exists for the branch. + +**Parameters:** + +| Param | Type | Default | Description | +|-------|------|---------|-------------| +| `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. + + +--- + ### `require-ci-green-before-stop` **Event:** Stop diff --git a/src/hooks/builtin-policies.ts b/src/hooks/builtin-policies.ts index 0555220c..f77884f3 100644 --- a/src/hooks/builtin-policies.ts +++ b/src/hooks/builtin-policies.ts @@ -1185,6 +1185,108 @@ function requirePrBeforeStop(ctx: PolicyContext): PolicyResult { } } +function requireNoConflictsBeforeStop(ctx: PolicyContext): PolicyResult { + const cwd = ctx.session?.cwd; + if (!cwd) return allow("No working directory available, skipping conflict check."); + + const branch = getCurrentBranch(cwd); + if (!branch || branch === "HEAD") return allow("Detached HEAD, skipping conflict check."); + + const baseBranch = (ctx.params?.baseBranch as string) ?? "main"; + if (branch === baseBranch) { + return allow(`On base branch "${baseBranch}", 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, + }); + + const ahead = execFileSync( + "git", ["log", `origin/${baseBranch}..HEAD`, "--oneline"], + { 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 { + 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 + } + } catch (err) { + const e = err as { status?: number; stdout?: string | Buffer }; + if (e.status === 1) { + // git merge-tree exit 1 = conflicts. stdout: \n\n\n\n + const out = (typeof e.stdout === "string" ? e.stdout : e.stdout?.toString("utf8") ?? "").trim(); + const lines = out.split("\n"); + const files: string[] = []; + for (let i = 1; i < lines.length; i++) { + const line = lines[i]; + if (line === "") break; + files.push(line); + } + const fileList = files.length ? files.join(", ") : "one or more files"; + return deny( + `Branch "${branch}" has merge conflicts with ${baseBranch} in: ${fileList}. ` + + `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", { + 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 }; + try { + pr = JSON.parse(prJson); + } catch { + return allow("Could not parse gh pr view output, skipping PR mergeability check."); + } + + if (pr.mergeable === "CONFLICTING") { + return deny( + `PR #${pr.number} has merge conflicts per GitHub (${pr.url}). ` + + `Rebase or merge origin/${baseBranch} now and resolve the conflicts.`, + ); + } + if (pr.mergeable === "UNKNOWN") { + return deny( + `GitHub is still computing mergeability for PR #${pr.number} (${pr.url}). ` + + `Wait ~10 seconds, then re-check with \`gh pr view --json mergeable\` before attempting to stop again.`, + ); + } + return allow(`PR #${pr.number} merges cleanly per GitHub.`); +} + function requireCiGreenBeforeStop(ctx: PolicyContext): PolicyResult { const cwd = ctx.session?.cwd; if (!cwd) return allow("No working directory available, skipping CI check."); @@ -1590,6 +1692,21 @@ export const BUILTIN_POLICIES: BuiltinPolicyDefinition[] = [ }, } satisfies PolicyParamsSchema, }, + { + name: "require-no-conflicts-before-stop", + description: "Require the current branch to merge cleanly with the base branch before Claude stops", + fn: requireNoConflictsBeforeStop, + match: { events: ["Stop"] }, + defaultEnabled: false, + category: "Workflow", + params: { + baseBranch: { + type: "string", + description: "Base branch to check for conflicts against (default: main)", + default: "main", + }, + } satisfies PolicyParamsSchema, + }, { name: "require-ci-green-before-stop", description: "Require CI checks to pass on the current branch before Claude stops",