swadhin / opencodehub public

Enforce required reviewer approvals in merge gates

swadhinbiswas committed 8 months ago

8e0d0e7

1 parent df9f4e4

commit 8e0d0e7

2 files changed

+324 -131

Files

src

lib

merge-queue.ts +122 -131

tests

unit

merge-gates.test.ts +202

Showing 2 changed files+324-131

src/lib/merge-queue.ts tests/unit/merge-gates.test.ts

▼src/lib/merge-queue.ts

+122-131

@@ -4,7 +4,7 @@
4	4	* Uses distributed locking for multi-instance safety
5	5	*/
6	6
7		-import { eq, and, asc, desc, lt, gt } from "drizzle-orm";
	7	+import { eq, and, asc, desc, inArray, lt, gt } from "drizzle-orm";
8	8	import type { NodePgDatabase } from "drizzle-orm/node-postgres";
9	9	import { getDatabase, schema } from "@/db";
10	10	import { generateId } from "./utils";
@@ -333,26 +333,14 @@ export async function canMerge(pullRequestId: string): Promise<{ canMerge: boole
333	333	}
334	334	}
335	335
336		- // Check local CI status (using workflow_runs table)
337		- // We want the latest run for this PR's head SHA
338		- const latestRun = await db.query.workflowRuns.findFirst({
	336	+ const runsForHead = await db.query.workflowRuns.findMany({
339	337	where: and(
340	338	eq(schema.workflowRuns.pullRequestId, pr.id),
341		- eq(schema.workflowRuns.headSha, pr.headSha) // Ensure it's for the latest commit
342		- // eq(schema.workflowRuns.headBranch, pr.headBranch) // Optional extra check
	339	+ eq(schema.workflowRuns.headSha, pr.headSha)
343	340	),
344		- orderBy: (runs, { desc }) => [desc(runs.createdAt)],
	341	+ orderBy: [desc(schema.workflowRuns.createdAt)],
345	342	});
346	343
347		- if (latestRun && latestRun.status !== "success") {
348		- if (latestRun.status === "running" \|\| latestRun.status === "queued") {
349		- return { canMerge: false, reason: "CI is still running" };
350		- }
351		- if (latestRun.status === "failed") {
352		- return { canMerge: false, reason: "CI checks failed" };
353		- }
354		- }
355		-
356	344	// Check Branch Protection Rules
357	345	const rules = await db.query.branchProtection.findMany({
358	346	where: and(
@@ -378,144 +366,147 @@ export async function canMerge(pullRequestId: string): Promise<{ canMerge: boole
378	366	matchingRule ? (matchingRule.requiredApprovals ?? 1) : 0
379	367	);
380	368
381		- // Check for approvals
382		- const approvals = await db.query.pullRequestReviews.findMany({
383		- where: and(
384		- eq(schema.pullRequestReviews.pullRequestId, pr.id),
385		- eq(schema.pullRequestReviews.state, "approved")
386		- ),
	369	+ // Evaluate by each reviewer's latest review state.
	370	+ const allReviews = await db.query.pullRequestReviews.findMany({
	371	+ where: eq(schema.pullRequestReviews.pullRequestId, pr.id),
	372	+ orderBy: [desc(schema.pullRequestReviews.submittedAt)],
387	373	});
	374	+ const latestReviewsByUser = new Map<string, typeof allReviews[number]>();
	375	+ for (const review of allReviews) {
	376	+ if (!latestReviewsByUser.has(review.reviewerId)) {
	377	+ latestReviewsByUser.set(review.reviewerId, review);
	378	+ }
	379	+ }
	380	+ const latestApprovedReviews = Array.from(latestReviewsByUser.values()).filter((review) => review.state === "approved");
	381	+ const latestBlockingReviews = Array.from(latestReviewsByUser.values()).filter((review) => review.state === "changes_requested");
388	382
389		- if (approvals.length < requiredApprovals) {
	383	+ if (latestApprovedReviews.length < requiredApprovals) {
390	384	return {
391	385	canMerge: false,
392		- reason: `At least ${requiredApprovals} approval(s) required (has ${approvals.length})`
	386	+ reason: `At least ${requiredApprovals} approval(s) required (has ${latestApprovedReviews.length})`
393	387	};
394	388	}
395	389
396		- // Check for blocking reviews (changes requested)
397		- const blockingReviews = await db.query.pullRequestReviews.findMany({
	390	+ if (latestBlockingReviews.length > 0) {
	391	+ return { canMerge: false, reason: "Changes requested by reviewer" };
	392	+ }
	393	+
	394	+ // Enforce explicitly required reviewers (multi-reviewer rule depth).
	395	+ const requiredReviewers = await db.query.pullRequestReviewers.findMany({
398	396	where: and(
399		- eq(schema.pullRequestReviews.pullRequestId, pr.id),
400		- eq(schema.pullRequestReviews.state, "changes_requested")
	397	+ eq(schema.pullRequestReviewers.pullRequestId, pr.id),
	398	+ eq(schema.pullRequestReviewers.isRequired, true)
401	399	),
402	400	});
	401	+ if (requiredReviewers.length > 0) {
	402	+ const missingRequiredApprovals: string[] = [];
	403	+ for (const reviewer of requiredReviewers) {
	404	+ const latestReview = latestReviewsByUser.get(reviewer.userId);
	405	+ if (!latestReview \|\| latestReview.state !== "approved") {
	406	+ missingRequiredApprovals.push(reviewer.userId);
	407	+ }
	408	+ }
403	409
404		- if (blockingReviews.length > 0) {
405		- return { canMerge: false, reason: "Changes requested by reviewer" };
	410	+ if (missingRequiredApprovals.length > 0) {
	411	+ return {
	412	+ canMerge: false,
	413	+ reason: `Required reviewer approvals missing (${missingRequiredApprovals.length})`
	414	+ };
	415	+ }
406	416	}
407	417
408		- // --- CODE OWNERS ENFORCEMENT ---
409		- if (reviewRequirements?.requireCodeOwner) {
410		- const repo = await db.query.repositories.findFirst({
411		- where: eq(schema.repositories.id, pr.repositoryId)
	418	+ const statusChecks = await db.query.requiredStatusChecks.findMany({
	419	+ where: and(
	420	+ eq(schema.requiredStatusChecks.repositoryId, pr.repositoryId),
	421	+ eq(schema.requiredStatusChecks.isRequired, true)
	422	+ ),
	423	+ });
	424	+ const requiredChecksForBranch = statusChecks.filter((check) => {
	425	+ if (check.branch === pr.baseBranch) return true;
	426	+ if (check.branch.endsWith("*")) {
	427	+ return pr.baseBranch.startsWith(check.branch.slice(0, -1));
	428	+ }
	429	+ return false;
412	430	});
413	431
414		- if (repo && repo.diskPath) {
415		- try {
416		- const localRepoPath = await resolveRepoPath(repo.diskPath);
417		- // 1. Get changed files
418		- const { getChangedFiles } = await import("./git");
419		- const changedFiles = await getChangedFiles(localRepoPath, pr.baseBranch, pr.headBranch);
420		-
421		- if (changedFiles.length > 0) {
422		- const fileApprovals = await db.query.fileApprovals.findMany({
423		- where: eq(schema.fileApprovals.pullRequestId, pr.id),
424		- });
	432	+ if (requiredChecksForBranch.length > 0) {
	433	+ if (runsForHead.length === 0) {
	434	+ return { canMerge: false, reason: "Required status checks not found for latest commit" };
	435	+ }
425	436
426		- if (fileApprovals.length > 0) {
427		- const approvalMap = new Map<string, boolean>();
	437	+ const successfulRunNames = new Set(
	438	+ runsForHead
	439	+ .filter((run) => {
	440	+ if (run.status === "success") return true;
	441	+ if (run.status === "completed" && run.conclusion === "success") return true;
	442	+ return false;
	443	+ })
	444	+ .map((run) => run.name)
	445	+ );
428	446
429		- for (const approval of fileApprovals) {
430		- if (approval.commitSha === pr.headSha) {
431		- approvalMap.set(approval.path, true);
432		- }
433		- }
	447	+ for (const requiredCheck of requiredChecksForBranch) {
	448	+ if (!successfulRunNames.has(requiredCheck.checkName)) {
	449	+ return {
	450	+ canMerge: false,
	451	+ reason: `Required status check "${requiredCheck.checkName}" has not succeeded`
	452	+ };
	453	+ }
	454	+ }
	455	+ } else {
	456	+ // Fallback: if there is a latest run for this head, it must not be failing.
	457	+ const latestRun = runsForHead[0];
	458	+ if (latestRun) {
	459	+ const running = latestRun.status === "running" \|\| latestRun.status === "queued" \|\| latestRun.status === "in_progress";
	460	+ const failed =
	461	+ latestRun.status === "failed" \|\|
	462	+ (latestRun.status === "completed" && latestRun.conclusion === "failure");
	463	+ if (running) return { canMerge: false, reason: "CI is still running" };
	464	+ if (failed) return { canMerge: false, reason: "CI checks failed" };
	465	+ }
	466	+ }
434	467
435		- for (const file of changedFiles) {
436		- if (!approvalMap.get(file)) {
437		- return {
438		- canMerge: false,
439		- reason: `File approval required for ${file}`
440		- };
441		- }
442		- }
443		- }
	468	+ const externalConfigs = await db.query.externalCIConfigs.findMany({
	469	+ where: and(
	470	+ eq(schema.externalCIConfigs.repositoryId, pr.repositoryId),
	471	+ eq(schema.externalCIConfigs.isEnabled, true),
	472	+ eq(schema.externalCIConfigs.syncStatus, true)
	473	+ ),
	474	+ });
444	475
445		- // 2. Read CODEOWNERS
446		- const { parseCodeOwners, findOwnersForFiles, getSuggestedReviewers, expandOwnersToUsernames } = await import("./codeowners");
447		- // Try reading from typical locations
448		- const fs = await import("fs/promises");
449		- const path = await import("path");
450		-
451		- let codeOwnersContent = null;
452		- const possiblePaths = ["CODEOWNERS", ".github/CODEOWNERS", "docs/CODEOWNERS"];
453		-
454		- for (const p of possiblePaths) {
455		- try {
456		- codeOwnersContent = await fs.readFile(path.join(localRepoPath, p), "utf-8");
457		- break;
458		- } catch (e) {
459		- // Ignore missing file
460		- }
461		- }
	476	+ if (externalConfigs.length > 0) {
	477	+ const configIds = externalConfigs.map((config) => config.id);
	478	+ const externalBuilds = await db.query.externalBuilds.findMany({
	479	+ where: and(
	480	+ eq(schema.externalBuilds.pullRequestId, pr.id),
	481	+ inArray(schema.externalBuilds.configId, configIds)
	482	+ ),
	483	+ orderBy: [desc(schema.externalBuilds.createdAt)],
	484	+ });
462	485
463		- if (codeOwnersContent) {
464		- const codeOwners = parseCodeOwners(codeOwnersContent);
465		- const requiredOwners = findOwnersForFiles(codeOwners, changedFiles);
466		-
467		- // If file has owners, we must ensure at least one of them approved?
468		- // Or do we need approval from ALL affected files' owners?
469		- // GitHub behavior: for each matching pattern, at least one owner must approve.
470		- // Here `findOwnersForFiles` returns unique owners of ANY matching file.
471		- // Strict implementation:
472		- // For each file, find its owners. If it has owners, at least one of THEM must have approved.
473		-
474		- // Let's iterate files to be strict.
475		- for (const file of changedFiles) {
476		- const owners = await import("./codeowners").then(m => m.findOwnersForFile(codeOwners, file));
477		- if (owners.length === 0) continue;
478		-
479		- // Check if ANY of these owners approved
480		- // We need the approver IDs. `approvals` contains the review objects.
481		- // But reviews link to userIds. We need to map owners (usernames/teams) to userIds.
482		-
483		- // This is complex because owners are usernames.
484		- // We need to resolve usernames to IDs.
485		-
486		- // Optimization: Get all approver usernames first.
487		- const approverUserIds = approvals.map(r => r.reviewerId);
488		- const approverUsers = await db.query.users.findMany({
489		- where: (users, { inArray }) => inArray(users.id, approverUserIds)
490		- });
491		- const approverUsernames = new Set(approverUsers.map(u => u.username));
492		-
493		- const expandedOwners = await expandOwnersToUsernames({
494		- db,
495		- repository: repo,
496		- owners,
497		- });
498		-
499		- const hasOwnerApproval = Array.from(expandedOwners).some((owner) =>
500		- approverUsernames.has(owner)
501		- );
502		-
503		- if (!hasOwnerApproval) {
504		- return {
505		- canMerge: false,
506		- reason: `Missing Code Owner approval for ${file} (requires: ${owners.join(", ")})`
507		- };
508		- }
509		- }
510		- }
	486	+ for (const config of externalConfigs) {
	487	+ const latestBuild = externalBuilds.find((build) => build.configId === config.id);
	488	+ if (!latestBuild) {
	489	+ return {
	490	+ canMerge: false,
	491	+ reason: `External CI "${config.name}" has not reported status for this pull request`
	492	+ };
	493	+ }
	494	+ const status = (latestBuild.status \|\| "").toLowerCase();
	495	+ if (status === "pending" \|\| status === "running" \|\| status === "queued" \|\| status === "in_progress") {
	496	+ return { canMerge: false, reason: `External CI "${config.name}" is still running` };
	497	+ }
	498	+ if (status !== "success" && status !== "passed") {
	499	+ return { canMerge: false, reason: `External CI "${config.name}" did not pass` };
511	500	}
512		- } catch (e) {
513		- logger.error({ error: e, prId: pr.id }, "Failed to check Code Owners");
514		- // Don't block on system error, or do? Safer to log and proceed if it's just a check failure?
515		- // "Fail open" or "Fail closed"?
516		- // Better to fail open (allow merge) if just git error, but warn.
517	501	}
518	502	}
	503	+
	504	+ if ((reviewRequirements?.requireCodeOwner ?? false) \|\| (matchingRule?.requireCodeOwnerReviews ?? false)) {
	505	+ const { checkCodeOwnerApprovalsForPR } = await import("./pr-codeowner");
	506	+ const codeOwnerCheck = await checkCodeOwnerApprovalsForPR(db, pr.id);
	507	+ if (!codeOwnerCheck.ok) {
	508	+ return { canMerge: false, reason: codeOwnerCheck.reason \|\| "Code owner approval required" };
	509	+ }
519	510	}
520	511
521	512	if (reviewRequirements?.requireReReviewOnPush) {
522	513

▼tests/unit/merge-gates.test.ts

+202-0

@@ -0,0 +1,202 @@
	1	+import { beforeEach, describe, expect, it, vi } from "vitest";
	2	+
	3	+let mockDb: any;
	4	+const getDatabaseMock = vi.fn(() => mockDb);
	5	+const getStackForPrMock = vi.fn(async () => null);
	6	+const checkCodeOwnerApprovalsForPRMock = vi.fn(async () => ({ ok: true }));
	7	+
	8	+const fakeSchema: any = {
	9	+ pullRequests: { id: {}, repositoryId: {} },
	10	+ workflowRuns: { pullRequestId: {}, headSha: {}, createdAt: {} },
	11	+ branchProtection: { repositoryId: {}, active: {} },
	12	+ reviewRequirements: { repositoryId: {} },
	13	+ pullRequestReviews: { pullRequestId: {}, submittedAt: {}, state: {} },
	14	+ pullRequestReviewers: { pullRequestId: {}, isRequired: {} },
	15	+ requiredStatusChecks: { repositoryId: {}, isRequired: {} },
	16	+ externalCIConfigs: { repositoryId: {}, isEnabled: {}, syncStatus: {}, id: {} },
	17	+ externalBuilds: { pullRequestId: {}, configId: {}, createdAt: {} },
	18	+};
	19	+
	20	+vi.mock("@/db", () => ({
	21	+ getDatabase: getDatabaseMock,
	22	+ schema: fakeSchema,
	23	+}));
	24	+
	25	+vi.mock("@/lib/stacks", () => ({
	26	+ getStackForPr: getStackForPrMock,
	27	+ getStack: vi.fn(),
	28	+}));
	29	+
	30	+vi.mock("@/lib/pr-codeowner", () => ({
	31	+ checkCodeOwnerApprovalsForPR: checkCodeOwnerApprovalsForPRMock,
	32	+}));
	33	+
	34	+const { canMerge } = await import("@/lib/merge-queue");
	35	+
	36	+function makeDb(overrides: Partial<Record<string, unknown>> = {}) {
	37	+ const pr = (overrides.pr as any) \|\| {
	38	+ id: "pr1",
	39	+ repositoryId: "repo1",
	40	+ state: "open",
	41	+ isMerged: false,
	42	+ headSha: "sha1",
	43	+ baseBranch: "main",
	44	+ authorId: "author1",
	45	+ };
	46	+
	47	+ const reviewRequirements = (overrides.reviewRequirements as any) \|\| {
	48	+ minApprovals: 1,
	49	+ requireCodeOwner: false,
	50	+ requireReReviewOnPush: false,
	51	+ dismissStaleReviews: false,
	52	+ };
	53	+
	54	+ return {
	55	+ query: {
	56	+ pullRequests: {
	57	+ findFirst: vi.fn(async () => pr),
	58	+ },
	59	+ workflowRuns: {
	60	+ findMany: vi.fn(async () => (overrides.workflowRuns as any[]) \|\| []),
	61	+ },
	62	+ branchProtection: {
	63	+ findMany: vi.fn(async () => (overrides.branchProtection as any[]) \|\| []),
	64	+ },
	65	+ reviewRequirements: {
	66	+ findFirst: vi.fn(async () => reviewRequirements),
	67	+ },
	68	+ pullRequestReviews: {
	69	+ findMany: vi.fn(async () => (overrides.reviews as any[]) \|\| []),
	70	+ },
	71	+ pullRequestReviewers: {
	72	+ findMany: vi.fn(async () => (overrides.requiredReviewers as any[]) \|\| []),
	73	+ },
	74	+ requiredStatusChecks: {
	75	+ findMany: vi.fn(async () => (overrides.requiredStatusChecks as any[]) \|\| []),
	76	+ },
	77	+ externalCIConfigs: {
	78	+ findMany: vi.fn(async () => (overrides.externalCiConfigs as any[]) \|\| []),
	79	+ },
	80	+ externalBuilds: {
	81	+ findMany: vi.fn(async () => (overrides.externalBuilds as any[]) \|\| []),
	82	+ },
	83	+ },
	84	+ };
	85	+}
	86	+
	87	+describe("merge gate enforcement", () => {
	88	+ beforeEach(() => {
	89	+ getStackForPrMock.mockResolvedValue(null);
	90	+ checkCodeOwnerApprovalsForPRMock.mockResolvedValue({ ok: true });
	91	+ });
	92	+
	93	+ it("blocks when required status check is missing", async () => {
	94	+ mockDb = makeDb({
	95	+ reviews: [{ reviewerId: "u1", state: "approved", submittedAt: new Date() }],
	96	+ requiredStatusChecks: [{ checkName: "build", branch: "main", isRequired: true }],
	97	+ workflowRuns: [{ name: "test", status: "success", conclusion: "success", createdAt: new Date() }],
	98	+ });
	99	+
	100	+ const result = await canMerge("pr1");
	101	+ expect(result.canMerge).toBe(false);
	102	+ expect(result.reason).toContain('Required status check "build"');
	103	+ });
	104	+
	105	+ it("blocks when external CI is still running", async () => {
	106	+ mockDb = makeDb({
	107	+ reviews: [{ reviewerId: "u1", state: "approved", submittedAt: new Date() }],
	108	+ externalCiConfigs: [{ id: "cfg1", name: "Jenkins", isEnabled: true, syncStatus: true }],
	109	+ externalBuilds: [{ configId: "cfg1", status: "running", createdAt: new Date() }],
	110	+ });
	111	+
	112	+ const result = await canMerge("pr1");
	113	+ expect(result.canMerge).toBe(false);
	114	+ expect(result.reason).toContain('External CI "Jenkins" is still running');
	115	+ });
	116	+
	117	+ it("blocks when latest review from reviewer is changes_requested", async () => {
	118	+ mockDb = makeDb({
	119	+ reviewRequirements: {
	120	+ minApprovals: 0,
	121	+ requireCodeOwner: false,
	122	+ requireReReviewOnPush: false,
	123	+ dismissStaleReviews: false,
	124	+ },
	125	+ reviews: [
	126	+ { reviewerId: "u1", state: "changes_requested", submittedAt: new Date("2026-02-18T10:00:00Z") },
	127	+ { reviewerId: "u1", state: "approved", submittedAt: new Date("2026-02-18T09:00:00Z") },
	128	+ ],
	129	+ });
	130	+
	131	+ const result = await canMerge("pr1");
	132	+ expect(result.canMerge).toBe(false);
	133	+ expect(result.reason).toContain("Changes requested");
	134	+ });
	135	+
	136	+ it("blocks when codeowner approvals are required but missing", async () => {
	137	+ checkCodeOwnerApprovalsForPRMock.mockResolvedValue({
	138	+ ok: false,
	139	+ reason: "Code owner approval required",
	140	+ });
	141	+
	142	+ mockDb = makeDb({
	143	+ reviewRequirements: {
	144	+ minApprovals: 1,
	145	+ requireCodeOwner: true,
	146	+ requireReReviewOnPush: false,
	147	+ dismissStaleReviews: false,
	148	+ },
	149	+ reviews: [{ reviewerId: "u1", state: "approved", submittedAt: new Date() }],
	150	+ });
	151	+
	152	+ const result = await canMerge("pr1");
	153	+ expect(result.canMerge).toBe(false);
	154	+ expect(result.reason).toContain("Code owner approval required");
	155	+ });
	156	+
	157	+ it("blocks when required reviewer has not approved", async () => {
	158	+ mockDb = makeDb({
	159	+ reviewRequirements: {
	160	+ minApprovals: 0,
	161	+ requireCodeOwner: false,
	162	+ requireReReviewOnPush: false,
	163	+ dismissStaleReviews: false,
	164	+ },
	165	+ requiredReviewers: [{ userId: "u_required", isRequired: true }],
	166	+ reviews: [{ reviewerId: "u_other", state: "approved", submittedAt: new Date() }],
	167	+ });
	168	+
	169	+ const result = await canMerge("pr1");
	170	+ expect(result.canMerge).toBe(false);
	171	+ expect(result.reason).toContain("Required reviewer approvals missing");
	172	+ });
	173	+
	174	+ it("passes when required reviewer latest review is approved", async () => {
	175	+ mockDb = makeDb({
	176	+ reviewRequirements: {
	177	+ minApprovals: 0,
	178	+ requireCodeOwner: false,
	179	+ requireReReviewOnPush: false,
	180	+ dismissStaleReviews: false,
	181	+ },
	182	+ requiredReviewers: [{ userId: "u_required", isRequired: true }],
	183	+ reviews: [{ reviewerId: "u_required", state: "approved", submittedAt: new Date() }],
	184	+ });
	185	+
	186	+ const result = await canMerge("pr1");
	187	+ expect(result.canMerge).toBe(true);
	188	+ });
	189	+
	190	+ it("passes when required checks, external CI, and approvals are satisfied", async () => {
	191	+ mockDb = makeDb({
	192	+ reviews: [{ reviewerId: "u1", state: "approved", submittedAt: new Date() }],
	193	+ requiredStatusChecks: [{ checkName: "build", branch: "main", isRequired: true }],
	194	+ workflowRuns: [{ name: "build", status: "success", conclusion: "success", createdAt: new Date() }],
	195	+ externalCiConfigs: [{ id: "cfg1", name: "Jenkins", isEnabled: true, syncStatus: true }],
	196	+ externalBuilds: [{ configId: "cfg1", status: "success", createdAt: new Date() }],
	197	+ });
	198	+
	199	+ const result = await canMerge("pr1");
	200	+ expect(result.canMerge).toBe(true);
	201	+ });
	202	+});
0	203