Code Issues Pull Requests Actions Security InsightsSettings

fix(gate): an outage of ours must not block a customer's merge #5580

MergedXSccantynz wants to mergefix/outage-must-not-block-merge→mainopened 2d ago

6 changed files+222−5

Addedsrc/__tests__/required-check-outage-credit.test.ts+103−0View fileUnified Split

@@ -0,0 +1,103 @@

1/**

2 * An outage must not block a merge — and must not become a way past a gate.

3 *

4 * Today the Anthropic account ran out of credit. The merge gate handled it

5 * correctly: gate.ts records "AI Review" as SKIPPED with "not blocking the

6 * merge". But passingCheckNames() re-read that persisted row and admitted

7 * only `passed`/`repaired`, so any repo that made "AI Review" a REQUIRED

8 * status check had every merge blocked, with no override — while the

9 * required-checks UI suggested that exact string as an example.

10 *

11 * The customer never earned that verdict and could not clear it. Blocking

12 * them on an outage of ours is the platform failing and then charging them

13 * for it.

14 *

15 * The fix is deliberately narrow, and the exclusions below are the point:

16 * "we could not look" is not "we looked and it was fine" for any check where

17 * the skip is something other than our own provider being down.

18 */

20import { describe, it, expect } from "bun:test";

21import { creditableOutageSkip } from "../lib/branch-protection";

23const outage = JSON.stringify({ skipReason: "ai_unavailable" });

24const disabled = JSON.stringify({ skipReason: "not_configured" });

26describe("a skipped gate row satisfying a required check", () => {

27 it("credits AI Review skipped because the provider was unavailable", () => {

28 expect(creditableOutageSkip("AI Review", outage)).toBe(true);

29 });

31 it("does NOT credit AI Review disabled in settings", () => {

32 // Otherwise an owner satisfies a branch-protection rule by flipping a

33 // repo setting — routing around the rule rather than passing it.

34 expect(creditableOutageSkip("AI Review", disabled)).toBe(false);

35 });

37 it("does NOT credit a skipped CI check, whatever the reason", () => {

38 // CI fails open in three places (no runs for the sha, stale, DB error).

39 // This matrix is the last thing that actually enforces CI; crediting a

40 // skipped CI row would delete that enforcement entirely.

41 expect(creditableOutageSkip("CI", outage)).toBe(false);

42 expect(creditableOutageSkip("CI", disabled)).toBe(false);

43 });

45 it("does NOT credit other gates that can skip for non-AI reasons", () => {

46 // Security scan skips for "disabled" and "nothing to scan" too; GateTest

47 // skips on a third-party outage; Merge check on an unreadable repo.

48 for (const name of ["Security scan", "GateTest", "Merge check", "Secret scan"]) {

49 expect(creditableOutageSkip(name, outage)).toBe(false);

50 }

51 });

53 it("does not treat missing or malformed details as permission", () => {

54 // Every gate row written before this change has details = NULL. None of

55 // them may retroactively satisfy a required check.

56 expect(creditableOutageSkip("AI Review", null)).toBe(false);

57 expect(creditableOutageSkip("AI Review", "")).toBe(false);

58 expect(creditableOutageSkip("AI Review", "not json")).toBe(false);

59 expect(creditableOutageSkip("AI Review", "{}")).toBe(false);

60 expect(creditableOutageSkip("AI Review", '{"skipReason":"something_else"}')).toBe(false);

61 });

62});

64describe("the gate records WHY it skipped", () => {

65 it("distinguishes an outage from an opt-out when writing the row", async () => {

66 const src = await Bun.file(require("path").resolve("src/lib/gate.ts")).text();

67 // Both reasons must be set, or the consumer above cannot tell them apart

68 // and the whole distinction collapses back to a bare `skipped`.

69 expect(src).toContain('skipReason: !runAiReview');

70 expect(src).toContain('"not_configured"');

71 expect(src).toContain('"ai_unavailable"');

72 // And it must actually be persisted, not just computed.

73 expect(src).toMatch(/details: check\.skipReason/);

74 });

75});

77describe("auto-merge reads the tri-state, not the prose", () => {

78 it("uses aiReviewGateState so an unavailable review fails open", async () => {

79 const src = await Bun.file(require("path").resolve("src/lib/auto-merge.ts")).text();

80 expect(src).toMatch(/await aiReviewGateState\(ctx\.pullRequestId\)\) !== false/);

81 // The old string-sniffing path must no longer decide a merge.

82 const decide = src.slice(src.indexOf("const aiApproved ="), src.indexOf("const aiApproved =") + 400);

83 expect(decide).not.toContain("aiApprovedForPr");

84 });

85});

87describe("a dead worker is our failure, not the author's", () => {

88 it('classifies "abandoned" as infrastructure', async () => {

89 const { classifyRun, isInfraConclusion, infraFailureLabel } = await import(

90 "../lib/ci-outcome"

91 );

92 // reapStuckRuns writes this for a run whose worker died holding it. It was

93 // absent from INFRA_CONCLUSIONS, so classifyRun returned "other" — which

94 // counted it against the repo in successRateExcludingInfra, the one

95 // statistic the module exists to protect, and made the merge gate say

96 // "CI failed (abandoned)".

97 expect(isInfraConclusion("abandoned")).toBe(true);

98 expect(classifyRun("failure", "abandoned")).toBe("infra");

99 expect(infraFailureLabel("abandoned")).toContain("not your code");

100 // A genuine red build is still a genuine red build.

101 expect(classifyRun("failure", "failure")).toBe("failure");

102 });

103});

Modifiedsrc/lib/auto-merge.ts+22−2View fileUnified Split

@@ -46,7 +46,7 @@ import {

4646 matchProtection,

4747 passingCheckNames,

4848} from "./branch-protection";

49import { AI_REVIEW_MARKER } from "./ai-review";

49import { AI_REVIEW_MARKER, aiReviewGateState } from "./ai-review";

5050import { audit } from "./notify";

5151import { getRepoPath, refRange} from "../git/repository";

5252import { getLatestCachedPrRisk } from "./pr-risk";

@@ -220,6 +220,13 @@ export function decideAutoMerge(args: {

220220 * If future reviewers do emit `severity: blocking`, that branch still

221221 * matches via the case-insensitive substring rule.

222222 */

223/**

224 * @deprecated Superseded by `aiReviewGateState()` in ./ai-review, which is a

225 * tri-state and treats an unavailable review as not-blocking. This function

226 * returned `false` for an outage, which blocked auto-merge every time the AI

227 * provider was down. Retained only for its existing tests; it has no

228 * production callers.

229 */

223230export function aiCommentLooksApproved(body: string): boolean {

224231 if (!body) return false;

225232 const lower = body.toLowerCase();

@@ -331,8 +338,21 @@ export async function evaluateAutoMerge(

331338

332339 // 2. Source the AI-approval signal only if the rule actually requires

333340 // it. Avoids the DB hit on rules that don't care.

341 // Tri-state, not comment sniffing. aiCommentLooksApproved() read

342 // "AI review unavailable" as NOT approved while ai-review.ts read the same

343 // string as not-blocking — two functions, one comment, opposite verdicts —

344 // so an AI outage silently stalled every auto-merge under

345 // requireAiApproval. This is the same expression the manual merge button

346 // (pulls.tsx) and the gated chain (pr-merge-gated.ts) already use:

347 // "unavailable" fails OPEN, matching the gate's own verdict.

348 //

349 // It also fixes two quieter bugs for free: aiApprovedForPr used .some(),

350 // so a stale approving comment outvoted a newer blocking one, and it

351 // returned false when there was no review comment at all.

334352 const aiApproved =

335 rule && rule.requireAiApproval ? await aiApprovedForPr(ctx.pullRequestId) : true;

353 rule && rule.requireAiApproval

354 ? (await aiReviewGateState(ctx.pullRequestId)) !== false

355 : true;

336356

337357 // 3. Human approvals — same query the manual-merge path uses.

338358 const humanApprovalCount = await countHumanApprovals(ctx.pullRequestId);

Modifiedsrc/lib/branch-protection.ts+50−1View fileUnified Split

@@ -212,6 +212,47 @@ export async function listRequiredChecks(

212212 }

213213}

214214

215/**

216 * Does a SKIPPED gate row still satisfy a required status check?

217 *

218 * Almost never. A required check exists to be enforced, and "we could not

219 * look" is not "we looked and it was fine". The single exception is an AI

220 * Review that could not run because the AI provider was unavailable:

221 *

222 * - The gate itself already fails open for it (gate.ts:859-870 records it

223 * skipped, "not blocking the merge"), but this matrix re-read that row

224 * and blocked anyway — so a repo that made "AI Review" a required check

225 * had EVERY merge blocked, with no override, the moment the Anthropic

226 * balance hit zero. The required-checks UI suggests that exact string.

227 * - The verdict was never the customer's to earn. Blocking them on an

228 * outage of ours is the platform failing, then billing them for it.

229 *

230 * Deliberately narrow, and each exclusion is load-bearing:

231 * - `CI` skipped is NOT credited. It fails open in three places (no runs

232 * for the sha, stale, DB error) and this matrix is the last thing that

233 * actually enforces CI. Crediting it would delete that enforcement.

234 * - `Security scan` skipped is NOT credited: its skip flag also covers

235 * "disabled in settings" and "nothing to scan".

236 * - `GateTest` skipped is NOT credited — a third-party outage is still a

237 * check that did not run, and the repo made it required on purpose.

238 * - AI Review skipped as `not_configured` is NOT credited: that would let

239 * an owner satisfy a branch-protection rule by flipping a repo setting,

240 * which is routing around the rule rather than passing it.

241 */

242export function creditableOutageSkip(

243 gateName: string,

244 details: string | null

245): boolean {

246 if (gateName !== "AI Review") return false;

247 if (!details) return false;

248 try {

249 const parsed = JSON.parse(details) as { skipReason?: string };

250 return parsed?.skipReason === "ai_unavailable";

251 } catch {

252 return false; // unparseable details is not a licence to pass

253 }

254}

255

215256/**

216257 * Compute the set of check names that have a passing latest result for this

217258 * repo + commit. A "check" is either:

@@ -244,7 +285,11 @@ export async function passingCheckNames(

244285 )

245286 : eq(gateRuns.repositoryId, repositoryId);

246287 const gRows = await db

247 .select({ name: gateRuns.gateName, status: gateRuns.status })

288 .select({

289 name: gateRuns.gateName,

290 status: gateRuns.status,

291 details: gateRuns.details,

292 })

248293 .from(gateRuns)

249294 .where(whereClause)

250295 .orderBy(desc(gateRuns.createdAt))

@@ -252,6 +297,10 @@ export async function passingCheckNames(

252297 for (const r of gRows) {

253298 if (r.status === "passed" || r.status === "repaired") {

254299 names.add(r.name);

300 continue;

301 }

302 if (r.status === "skipped" && creditableOutageSkip(r.name, r.details)) {

303 names.add(r.name);

255304 }

256305 }

257306 } catch {

Modifiedsrc/lib/ci-outcome.ts+12−0View fileUnified Split

@@ -17,6 +17,16 @@ export const INFRA_CONCLUSIONS: ReadonlySet<string> = new Set([

1717 "runner_restarted",

1818 "runner_lost",

1919 "infra_failed",

20 // "abandoned" is written by reapStuckRuns (workflow-runner.ts) for a run the

21 // worker died holding — as pure an infrastructure fault as the three above,

22 // and it was missing here. The cost was not cosmetic: classifyRun() returned

23 // "other", so successRateExcludingInfra() counted a dead container against

24 // the repo in the very statistic this module exists to protect, and the

25 // merge gate told the author "CI failed (abandoned)" — blaming their code

26 // for our worker. Separating platform failure from your failure is a stated

27 // product promise (docs/research/COMPETITOR-PAIN-2026-08.md), so this was a

28 // regression against it, not a nit.

29 "abandoned",

2030]);

2131

2232export type CiOutcomeKind = "success" | "failure" | "infra" | "pending" | "other";

@@ -36,6 +46,8 @@ export function isInfraConclusion(conclusion: string | null | undefined): boolea

3646/** Human wording for an infra interruption — never blame the code. */

3747export function infraFailureLabel(conclusion: string | null | undefined): string {

3848 switch (conclusion) {

49 case "abandoned":

50 return "the worker running it died — not your code";

3951 case "runner_restarted":

4052 return "interrupted by a platform restart — not your code";

4153 case "runner_lost":

Modifiedsrc/lib/gate.ts+26−0View fileUnified Split

@@ -43,6 +43,17 @@ export interface GateCheckResult {

4343 passed: boolean;

4444 details: string;

4545 skipped?: boolean;

46 /**

47 * Machine-readable reason a check was skipped.

48 *

49 * `skipped` alone is ambiguous — "the AI provider is down" and "the owner

50 * turned this off in settings" both produce it, and they must NOT be

51 * treated alike: the first is an outage nobody chose, the second is a repo

52 * setting that would otherwise let someone route around a branch-protection

53 * rule by flipping a switch. Persisted into the existing (and until now

54 * always-NULL) `gate_runs.details` column, so no migration is needed.

55 */

56 skipReason?: "ai_unavailable" | "not_configured";

4657 repaired?: boolean;

4758 repairCommitSha?: string;

4859}

@@ -860,6 +871,14 @@ export async function runAllGateChecks(

860871 name: "AI Review",

861872 passed: !runAiReview || aiReviewApproved !== false,

862873 skipped: !runAiReview || aiReviewApproved === "unavailable",

874 // Distinguishes an outage from a deliberate opt-out. Only the outage is

875 // allowed to satisfy a REQUIRED status check downstream; see

876 // passingCheckNames() in branch-protection.ts.

877 skipReason: !runAiReview

878 ? "not_configured"

879 : aiReviewApproved === "unavailable"

880 ? "ai_unavailable"

881 : undefined,

863882 details: !runAiReview

864883 ? "Disabled in settings"

865884 : aiReviewApproved === "unavailable"

@@ -917,6 +936,13 @@ export async function runAllGateChecks(

917936 ? "passed"

918937 : "failed",

919938 summary: check.details,

939 // The `details` column has existed since migration 0001 and has

940 // been NULL for every gate row ever written. It is the discriminator

941 // store: no migration, no new status value (which eight status

942 // enumerators would silently mis-bucket).

943 details: check.skipReason

944 ? { skipReason: check.skipReason }

945 : undefined,

920946 repairAttempted: !!check.repaired,

921947 repairSucceeded: !!check.repaired,

922948 repairCommitSha: check.repairCommitSha,

Modifiedsrc/routes/required-checks.tsx+9−2View fileUnified Split

@@ -853,8 +853,15 @@ required.get(

853853 class="rc-input"

854854 />

855855 <div class="rc-form-hint">

856 Examples: <code>GateTest</code>, <code>AI Review</code>,{" "}

857 <code>Secret Scan</code>, <code>Type Check</code>.

856 {/* These must be names the gate actually emits, matched

857 exactly. "Secret Scan" and "Type Check" were suggested

858 here for months and neither is ever produced — the gate

859 emits "Secret scan" (lowercase s) and no type check at

860 all — so anyone who followed this hint gave themselves a

861 required check that can never pass. */}

862 Examples: <code>GateTest</code>, <code>CI</code>,{" "}

863 <code>Secret scan</code>, <code>Security scan</code>,{" "}

864 <code>AI Review</code>, <code>Merge check</code>.

858865 </div>

859866 </div>

860867 <button type="submit" class="rc-btn rc-btn-primary">

861868

c comment · e edit title · m merge · a approve · r request changes · ? shortcuts