fix(gate): the CI merge gate could be switched off by capitalising the repo name #5599
3 changed files+114−19
Modifiedsrc/__tests__/gate-ci-check.test.ts+65−2View fileUnifiedSplit
@@ -227,10 +227,73 @@ describe("CI gate wiring", () => {
227227 expect(src).toContain("checks.push(ciResult)");
228228 });
229229
230 it("a DB failure degrades to skipped — never blocks a merge on a lookup error", async () => {
230 /**
231 * REVERSED 2026-08-31, deliberately.
232 *
233 * This test used to assert the opposite: that a lookup failure degraded to
234 * `skipped` and never blocked a merge. That was a considered decision — a
235 * database hiccup should not wedge every merge on the platform — and it was
236 * wrong, for a reason that only became visible when it was exploited by
237 * accident.
238 *
239 * On 2026-08-31 four pull requests merged without their CI being consulted
240 * at all. `lookupRepo` was case-sensitive; a caller passing `gluecron.com`
241 * against a repo stored as `Gluecron.com` got null, and null was reported as
242 * a PASSING CI check. Two of those merges happened while their runs were
243 * still going. Nothing looked wrong in any response.
244 *
245 * The old direction has a cost that the reasoning behind it did not price:
246 * a merge that nothing checked. The new direction costs a merge that waits.
247 * Those are not comparable. And the platform's own attribution guarantee
248 * already settles it — where we cannot tell, we say we cannot tell, never a
249 * green that means "we did not look".
250 *
251 * The specific fear behind the old behaviour is also smaller than it looks:
252 * if the database is unreachable, the merge cannot write its own rows
253 * either, so it was never going to succeed.
254 */
255 it("a lookup failure blocks — 'we could not look' is not 'we looked and it was fine'", async () => {
231256 const src = await Bun.file("src/lib/gate.ts").text();
232257 const fn = src.slice(src.indexOf("export async function checkCiWorkflows"));
233 expect(fn).toContain("CI status unavailable");
234258 expect(fn).toContain("catch");
259 expect(fn).toContain("passed: false");
260 expect(fn).toContain("does not count as green");
261 // The old shape, in the same function, would silently restore the hole.
262 // Comments stripped first: the incident note directly above the return
263 // quotes `passed: true` as the thing that was wrong, and an assertion
264 // that reads prose is testing that we still describe the property rather
265 // than that we still have it.
266 const catchBlock = fn
267 .slice(fn.indexOf("} catch (err) {"), fn.indexOf("} catch (err) {") + 1200)
268 .split(/\r?\n/)
269 .filter((l) => !/^\s*\/\//.test(l))
270 .join("\n");
271 expect(catchBlock).not.toContain("passed: true");
272 });
273
274 it("an unresolvable repository blocks instead of disabling the CI gate", async () => {
275 // The actual 2026-08-31 vector. `repoRow` null meant the CI check was
276 // fabricated as passing, so any caller who spelled the repo name in a
277 // different case merged with no CI enforcement whatsoever.
278 const src = await Bun.file("src/lib/gate.ts").text();
279 const branch = src.slice(src.indexOf("? checkCiWorkflows(repoRow.id"));
280 expect(branch.slice(0, 1200)).toContain("CI could not be verified");
281 expect(branch.slice(0, 1200)).not.toContain("Repository not accessible");
282 });
283
284 it("the gate resolves repositories case-insensitively", async () => {
285 // Being unresolvable now blocks merges, which makes a case-sensitive
286 // lookup a denial of service rather than a silent bypass. Both halves
287 // have to hold together.
288 const src = await Bun.file("src/lib/gate.ts").text();
289 const fn = src.slice(src.indexOf("async function lookupRepo"), src.indexOf("Fire-and-forget post-receive"));
290 // Delegation, not a second copy. loadRepoByPath is case-insensitive AND
291 // resolves org-owned repos; a private lookup here was both case-sensitive
292 // and user-only, and once an unresolvable repo BLOCKS a merge, a user-only
293 // resolver would make every org repository permanently unmergeable. The
294 // two halves of this fix depend on each other.
295 expect(fn).toContain("loadRepoByPath(owner, repo)");
296 expect(fn).not.toContain("from(users)");
297 expect(fn).not.toContain("eq(repositories.name, repo)");
235298 });
236299});
Modifiedsrc/__tests__/repo-name-casing-ratchet.test.ts+2−2View fileUnifiedSplit
@@ -16,7 +16,7 @@
1616 *
1717 * `loadRepoByPath` in src/lib/namespace.ts is the correct resolver and has
1818 * been for months. The sweep to route everything through it has been pending
19 * since 2026-08-05 across 127 call sites, and a sweep that size is not
19 * since 2026-08-05 across 126 call sites, and a sweep that size is not
2020 * something to do blind.
2121 *
2222 * So: a ratchet, not a gate. It cannot fix what is there, but it stops the
@@ -40,7 +40,7 @@ import { join, resolve } from "path";
4040 * Lower this when you convert call sites. Never raise it: if a change needs a
4141 * new exact-name lookup, it almost certainly wants `loadRepoByPath` instead.
4242 */
43const BASELINE = 127;
43const BASELINE = 126;
4444
4545const SRC = resolve(import.meta.dir, "..");
4646const PATTERN = /eq\(\s*repositories\.name\s*,/g;
Modifiedsrc/lib/gate.ts+47−15View fileUnifiedSplit
@@ -13,7 +13,7 @@
1313 */
1414
1515import { isInfraConclusion, infraFailureLabel } from "./ci-outcome";
16import { and, desc, eq } from "drizzle-orm";
16import { and, desc, eq, sql } from "drizzle-orm";
1717import { config } from "./config";
1818import { db } from "../db";
1919import {
@@ -103,20 +103,40 @@ async function recordGateRun(opts: {
103103
104104/**
105105 * Look up the repository row by owner/name.
106 *
107 * CASE-INSENSITIVE, and that is not cosmetic here — this lookup decides
108 * whether the CI merge gate runs at all.
109 *
110 * INCIDENT 2026-08-31. This used exact equality on both the username and the
111 * repository name. The stored name is `Gluecron.com`; a caller passing
112 * `gluecron.com` — which every MCP merge in that session did — got null, and
113 * `checkCiWorkflows` turned null into `{ passed: true, skipped: true,
114 * "Repository not accessible" }`. The CI gate did not fail; it silently
115 * stopped existing. Four pull requests merged that day without their CI being
116 * consulted, two of them while their runs were still going, and the merge
117 * responses looked completely normal.
118 *
119 * Two things had to be true for that, and both are fixed: the lookup was
120 * case-sensitive, and its failure was reported as a pass. Either one alone
121 * would have been survivable.
106122 */
107123async function lookupRepo(
108124 owner: string,
109125 repo: string
110126): Promise<{ id: string } | null> {
111127 try {
112 const [u] = await db.select().from(users).where(eq(users.username, owner)).limit(1);
113 if (!u) return null;
114 const { and } = await import("drizzle-orm");
115 const [r] = await db
116 .select()
117 .from(repositories)
118 .where(and(eq(repositories.ownerId, u.id), eq(repositories.name, repo)))
119 .limit(1);
128 // Delegates to the platform's one real resolver rather than keeping a
129 // private copy. That is not tidiness: this function's own copy was
130 // case-sensitive AND user-only, and BOTH halves became dangerous the
131 // moment its failure started blocking merges instead of silently passing
132 // them. An org-owned repository resolves here now; under the old private
133 // lookup it returned null, which used to mean "skip CI" and would now
134 // mean "this repo can never merge".
135 //
136 // sql`lower(...)` matching lives in loadRepoByPath; keeping it in one
137 // place is what stops the next copy from drifting the same way.
138 const { loadRepoByPath } = await import("./namespace");
139 const r = await loadRepoByPath(owner, repo);
120140 return r ? { id: r.id } : null;
121141 } catch {
122142 return null;
@@ -519,11 +539,17 @@ export async function checkCiWorkflows(
519539 const expected = await expectedCiWorkflows(repositoryId, headBranch);
520540 return decideCiGate(rows, expected, now);
521541 } catch (err) {
542 // FAIL CLOSED, changed 2026-08-31. This returned `passed: true` — a
543 // database hiccup while reading run rows reported the CI gate as
544 // satisfied. The cost of the old direction is a merge that nothing
545 // checked; the cost of this one is a merge that waits. Those are not
546 // comparable, and this platform's own attribution guarantee already says
547 // which way to resolve it: an "unknown" renders as unknown, never as a
548 // green that means "we did not look".
522549 return {
523550 name: "CI",
524 passed: true,
525 skipped: true,
526 details: `CI status unavailable (${err instanceof Error ? err.message : "lookup failed"}) — not blocking`,
551 passed: false,
552 details: `CI status could not be read (${err instanceof Error ? err.message : "lookup failed"}) — nothing was checked, so this does not count as green. Retry; if it persists it is a platform fault.`,
527553 };
528554 }
529555}
@@ -855,10 +881,16 @@ export async function runAllGateChecks(
855881 repoRow
856882 ? checkCiWorkflows(repoRow.id, headSha, headBranch)
857883 : Promise.resolve<GateCheckResult>({
884 // FAIL CLOSED. This used to be `passed: true`, which meant that
885 // failing to resolve the repository silently disabled the CI gate
886 // rather than reporting a problem — see the incident note on
887 // lookupRepo. "We could not look" is not "we looked and it was
888 // fine", and the whole value of this gate is that the difference
889 // is visible.
858890 name: "CI",
859 passed: true,
860 skipped: true,
861 details: "Repository not accessible",
891 passed: false,
892 details:
893 "CI could not be verified — the repository could not be resolved, so no CI result was consulted. This is a platform problem, not your code; report it rather than working around it.",
862894 }),
863895 ]);
864896
865897
c comment · e edit title · m merge · a approve · r request changes · ? shortcuts