Enforce label and review merge gate evaluation
swadhinbiswas committed 8 months ago
c6f115d
1 parent c6a96ef
commit c6f115d
3 files changed
+234 -11
Showing 3 changed files+234-11
▼doc/feature_audit.md
+1-1
| @@ -94,7 +94,7 @@ | ||
94 | 94 | | External CI integration | ⚠️ | Provider-aware integration APIs and normalized status ingestion are implemented, with repo-level integration health/status + token rotate/disable APIs (`external-ci`, `external-ci/checks`); provider-specific setup UX still maturing | |
95 | 95 | | Self-hosted runners | ✅ | `runner/` with Docker executor | |
96 | 96 | | Secrets management | ✅ | Schema + UI for repo/org secrets | |
97 | -| Merge checks & gates | ⚠️ | Required status checks + external CI gate readiness are enforced, with explicit PR gate introspection via `pulls/{number}/merge-readiness` and repository-level merge gate management APIs (`/repos/{owner}/{repo}/merge-gates`, `/repos/{owner}/{repo}/merge-gates/{id}`); broader policy UX remains | | |
97 | +| Merge checks & gates | ⚠️ | Required status checks + external CI gate readiness are enforced, with explicit PR gate introspection via `pulls/{number}/merge-readiness`, active label/review custom gate evaluation in `ci-gates.ts`, and repository-level gate management APIs (`/repos/{owner}/{repo}/merge-gates`, `/repos/{owner}/{repo}/merge-gates/{id}`); broader policy UX remains | | |
98 | 98 | | Automation rules engine | ⚠️ | Conditions/actions active with retry + dead-letter audit logging; advanced orchestration still maturing | |
99 | 99 | | Webhooks | ✅ | Full implementation with UI | |
100 | 100 | | Workflow templates | ⚠️ | Template library plus repo-level listing/apply APIs are now implemented (`workflow/templates`); broader UX/workflow adoption still maturing | |
101 | 101 | |
▼src/lib/ci-gates.ts
+79-10
| @@ -55,6 +55,11 @@ export interface GateResult { | ||
55 | 55 | details?: Record<string, unknown>; |
56 | 56 | } |
57 | 57 | |
58 | +type EvaluatablePullRequest = typeof schema.pullRequests.$inferSelect & { | |
59 | + reviews: Array<typeof schema.pullRequestReviews.$inferSelect>; | |
60 | + checks: Array<typeof schema.pullRequestChecks.$inferSelect>; | |
61 | +}; | |
62 | + | |
58 | 63 | /** |
59 | 64 | * Add required status check |
60 | 65 | */ |
| @@ -221,17 +226,53 @@ export async function evaluateGates(prId: string): Promise<{ | ||
221 | 226 | */ |
222 | 227 | async function evaluateSingleGate( |
223 | 228 | gate: MergeGate, |
224 | - pr: typeof schema.pullRequests.$inferSelect | |
229 | + pr: EvaluatablePullRequest | |
225 | 230 | ): Promise<GateResult> { |
231 | + const db = getDatabase(); | |
226 | 232 | const config = gate.config ? JSON.parse(gate.config) : {}; |
227 | 233 | |
228 | 234 | switch (gate.gateType) { |
229 | 235 | case "label": { |
230 | - // Require or block specific labels | |
231 | - const requiredLabel = config.required; | |
232 | - const blockedLabel = config.blocked; | |
233 | - | |
234 | - // Would need to fetch labels for PR | |
236 | + const labelLinks = await db.query.pullRequestLabels?.findMany({ | |
237 | + where: eq(schema.pullRequestLabels.pullRequestId, pr.id), | |
238 | + with: { label: true }, | |
239 | + }) || []; | |
240 | + const labelNames = new Set( | |
241 | + labelLinks | |
242 | + .map((item) => item.label?.name) | |
243 | + .filter((name): name is string => Boolean(name)) | |
244 | + ); | |
245 | + | |
246 | + const requiredLabelsRaw = config.required; | |
247 | + const blockedLabelsRaw = config.blocked; | |
248 | + const requiredLabels = Array.isArray(requiredLabelsRaw) | |
249 | + ? requiredLabelsRaw.filter((value): value is string => typeof value === "string" && value.length > 0) | |
250 | + : typeof requiredLabelsRaw === "string" && requiredLabelsRaw.length > 0 | |
251 | + ? [requiredLabelsRaw] | |
252 | + : []; | |
253 | + const blockedLabels = Array.isArray(blockedLabelsRaw) | |
254 | + ? blockedLabelsRaw.filter((value): value is string => typeof value === "string" && value.length > 0) | |
255 | + : typeof blockedLabelsRaw === "string" && blockedLabelsRaw.length > 0 | |
256 | + ? [blockedLabelsRaw] | |
257 | + : []; | |
258 | + | |
259 | + const missingRequired = requiredLabels.filter((label) => !labelNames.has(label)); | |
260 | + const presentBlocked = blockedLabels.filter((label) => labelNames.has(label)); | |
261 | + | |
262 | + if (missingRequired.length > 0) { | |
263 | + return { | |
264 | + passed: false, | |
265 | + gateName: gate.name, | |
266 | + message: `Missing required labels: ${missingRequired.join(", ")}`, | |
267 | + }; | |
268 | + } | |
269 | + if (presentBlocked.length > 0) { | |
270 | + return { | |
271 | + passed: false, | |
272 | + gateName: gate.name, | |
273 | + message: `Blocked labels present: ${presentBlocked.join(", ")}`, | |
274 | + }; | |
275 | + } | |
235 | 276 | return { |
236 | 277 | passed: true, |
237 | 278 | gateName: gate.name, |
| @@ -240,13 +281,41 @@ async function evaluateSingleGate( | ||
240 | 281 | } |
241 | 282 | |
242 | 283 | case "review": { |
243 | - // Custom review requirements | |
244 | - const minReviews = config.minReviews || 1; | |
245 | - // Would check against actual review count | |
284 | + const minReviews = typeof config.minReviews === "number" && config.minReviews > 0 | |
285 | + ? Math.floor(config.minReviews) | |
286 | + : 1; | |
287 | + const latestByReviewer = new Map<string, typeof pr.reviews[number]>(); | |
288 | + for (const review of pr.reviews || []) { | |
289 | + const previous = latestByReviewer.get(review.reviewerId); | |
290 | + const reviewTime = review.submittedAt || review.createdAt || new Date(0); | |
291 | + const previousTime = previous ? (previous.submittedAt || previous.createdAt || new Date(0)) : new Date(0); | |
292 | + if (!previous || reviewTime >= previousTime) { | |
293 | + latestByReviewer.set(review.reviewerId, review); | |
294 | + } | |
295 | + } | |
296 | + | |
297 | + const latestReviews = [...latestByReviewer.values()]; | |
298 | + const approvalCount = latestReviews.filter((review) => review.state === "approved").length; | |
299 | + const hasChangesRequested = latestReviews.some((review) => review.state === "changes_requested"); | |
300 | + | |
301 | + if (hasChangesRequested) { | |
302 | + return { | |
303 | + passed: false, | |
304 | + gateName: gate.name, | |
305 | + message: "Changes requested by reviewer", | |
306 | + }; | |
307 | + } | |
308 | + if (approvalCount < minReviews) { | |
309 | + return { | |
310 | + passed: false, | |
311 | + gateName: gate.name, | |
312 | + message: `Needs ${minReviews - approvalCount} more approval(s)`, | |
313 | + }; | |
314 | + } | |
246 | 315 | return { |
247 | 316 | passed: true, |
248 | 317 | gateName: gate.name, |
249 | - message: `Review requirements met`, | |
318 | + message: `Review requirements met (${approvalCount}/${minReviews})`, | |
250 | 319 | }; |
251 | 320 | } |
252 | 321 | |
253 | 322 | |
▼tests/unit/ci-gates-evaluation.test.ts
+154-0
| @@ -0,0 +1,154 @@ | ||
1 | +import { beforeEach, describe, expect, it, vi } from "vitest"; | |
2 | + | |
3 | +let mockDb: any; | |
4 | +const getDatabaseMock = vi.fn(() => mockDb); | |
5 | + | |
6 | +const fakeSchema: any = { | |
7 | + pullRequests: { id: {} }, | |
8 | + requiredStatusChecks: { repositoryId: {} }, | |
9 | + mergeGates: { repositoryId: {} }, | |
10 | + pullRequestLabels: { pullRequestId: {} }, | |
11 | +}; | |
12 | + | |
13 | +vi.mock("@/db", () => ({ | |
14 | + getDatabase: getDatabaseMock, | |
15 | + schema: fakeSchema, | |
16 | +})); | |
17 | + | |
18 | +vi.mock("@/lib/logger", () => ({ | |
19 | + logger: { | |
20 | + info: vi.fn(), | |
21 | + error: vi.fn(), | |
22 | + }, | |
23 | +})); | |
24 | + | |
25 | +const { evaluateGates } = await import("@/lib/ci-gates"); | |
26 | + | |
27 | +function makeDb(overrides: Partial<Record<string, unknown>> = {}) { | |
28 | + const pr = (overrides.pr as any) || { | |
29 | + id: "pr-1", | |
30 | + repositoryId: "repo-1", | |
31 | + baseBranch: "main", | |
32 | + mergeable: true, | |
33 | + checks: [], | |
34 | + reviews: [], | |
35 | + }; | |
36 | + | |
37 | + return { | |
38 | + query: { | |
39 | + pullRequests: { | |
40 | + findFirst: vi.fn(async () => pr), | |
41 | + }, | |
42 | + requiredStatusChecks: { | |
43 | + findMany: vi.fn(async () => (overrides.requiredChecks as any[]) || []), | |
44 | + }, | |
45 | + mergeGates: { | |
46 | + findMany: vi.fn(async () => (overrides.mergeGates as any[]) || []), | |
47 | + }, | |
48 | + pullRequestLabels: { | |
49 | + findMany: vi.fn(async () => (overrides.prLabels as any[]) || []), | |
50 | + }, | |
51 | + }, | |
52 | + }; | |
53 | +} | |
54 | + | |
55 | +describe("ci gate evaluation", () => { | |
56 | + beforeEach(() => { | |
57 | + vi.clearAllMocks(); | |
58 | + }); | |
59 | + | |
60 | + it("fails label gate when required labels are missing", async () => { | |
61 | + mockDb = makeDb({ | |
62 | + mergeGates: [ | |
63 | + { | |
64 | + id: "gate-1", | |
65 | + repositoryId: "repo-1", | |
66 | + name: "Label gate", | |
67 | + gateType: "label", | |
68 | + config: JSON.stringify({ required: ["ready"] }), | |
69 | + conditionScript: null, | |
70 | + isEnabled: true, | |
71 | + order: 0, | |
72 | + }, | |
73 | + ], | |
74 | + prLabels: [{ label: { name: "needs-review" } }], | |
75 | + pr: { | |
76 | + id: "pr-1", | |
77 | + repositoryId: "repo-1", | |
78 | + baseBranch: "main", | |
79 | + mergeable: true, | |
80 | + checks: [], | |
81 | + reviews: [{ reviewerId: "u1", state: "approved", submittedAt: new Date() }], | |
82 | + }, | |
83 | + }); | |
84 | + | |
85 | + const result = await evaluateGates("pr-1"); | |
86 | + expect(result.canMerge).toBe(false); | |
87 | + expect(result.results.some((gate) => gate.message.includes("Missing required labels"))).toBe(true); | |
88 | + }); | |
89 | + | |
90 | + it("fails review gate when latest review is changes requested", async () => { | |
91 | + mockDb = makeDb({ | |
92 | + mergeGates: [ | |
93 | + { | |
94 | + id: "gate-2", | |
95 | + repositoryId: "repo-1", | |
96 | + name: "Review gate", | |
97 | + gateType: "review", | |
98 | + config: JSON.stringify({ minReviews: 1 }), | |
99 | + conditionScript: null, | |
100 | + isEnabled: true, | |
101 | + order: 0, | |
102 | + }, | |
103 | + ], | |
104 | + pr: { | |
105 | + id: "pr-1", | |
106 | + repositoryId: "repo-1", | |
107 | + baseBranch: "main", | |
108 | + mergeable: true, | |
109 | + checks: [], | |
110 | + reviews: [ | |
111 | + { reviewerId: "u1", state: "approved", submittedAt: new Date("2026-02-18T10:00:00Z"), createdAt: new Date("2026-02-18T10:00:00Z") }, | |
112 | + { reviewerId: "u1", state: "changes_requested", submittedAt: new Date("2026-02-18T11:00:00Z"), createdAt: new Date("2026-02-18T11:00:00Z") }, | |
113 | + ], | |
114 | + }, | |
115 | + }); | |
116 | + | |
117 | + const result = await evaluateGates("pr-1"); | |
118 | + expect(result.canMerge).toBe(false); | |
119 | + expect(result.results.some((gate) => gate.message.includes("Changes requested"))).toBe(true); | |
120 | + }); | |
121 | + | |
122 | + it("passes review gate when minimum approvals are met with latest states", async () => { | |
123 | + mockDb = makeDb({ | |
124 | + mergeGates: [ | |
125 | + { | |
126 | + id: "gate-3", | |
127 | + repositoryId: "repo-1", | |
128 | + name: "Review gate", | |
129 | + gateType: "review", | |
130 | + config: JSON.stringify({ minReviews: 2 }), | |
131 | + conditionScript: null, | |
132 | + isEnabled: true, | |
133 | + order: 0, | |
134 | + }, | |
135 | + ], | |
136 | + pr: { | |
137 | + id: "pr-1", | |
138 | + repositoryId: "repo-1", | |
139 | + baseBranch: "main", | |
140 | + mergeable: true, | |
141 | + checks: [], | |
142 | + reviews: [ | |
143 | + { reviewerId: "u1", state: "approved", submittedAt: new Date("2026-02-18T10:00:00Z"), createdAt: new Date("2026-02-18T10:00:00Z") }, | |
144 | + { reviewerId: "u2", state: "commented", submittedAt: new Date("2026-02-18T09:00:00Z"), createdAt: new Date("2026-02-18T09:00:00Z") }, | |
145 | + { reviewerId: "u2", state: "approved", submittedAt: new Date("2026-02-18T12:00:00Z"), createdAt: new Date("2026-02-18T12:00:00Z") }, | |
146 | + ], | |
147 | + }, | |
148 | + }); | |
149 | + | |
150 | + const result = await evaluateGates("pr-1"); | |
151 | + expect(result.canMerge).toBe(true); | |
152 | + expect(result.results.some((gate) => gate.message.includes("Review requirements met"))).toBe(true); | |
153 | + }); | |
154 | +}); | |
0 | 155 | |