Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions src/queue/ci-resolution.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@ import type { GitHubRateLimitAdmissionKey } from "../github/client";
import { incr } from "../selfhost/metrics";
import type { LiveGithubFacts, RequiredStatusContextsLookup } from "./processors";

/** Emitted when a live gate evaluation falls back to config-only required contexts because the branch-protection read failed. */
export const REQUIRED_CONTEXTS_UNRESOLVED_METRIC = "loopover_required_contexts_unresolved_total";

export function liveFactKey(...parts: Array<string | number | null | undefined>): string {
return JSON.stringify(parts.map((part) => [typeof part, part]));
}
Expand Down Expand Up @@ -105,6 +108,28 @@ export function cachedRequiredStatusContexts(
return next;
}

/**
* #8358: surface a previously unused `resolved: false` on RequiredStatusContextsLookup. Callers (the live
* gate path in processors.ts) must invoke this after cachedRequiredStatusContexts so a degraded
* branch-protection read is observable via metric + structured warn — gate disposition is unchanged.
*/
export function observeRequiredContextsLookup(
lookup: RequiredStatusContextsLookup,
meta: { repoFullName: string; pullNumber: number; baseRef: string | null | undefined },
): void {
if (lookup.resolved) return;
incr(REQUIRED_CONTEXTS_UNRESOLVED_METRIC);
console.warn(
JSON.stringify({
level: "warn",
event: "required_contexts_branch_protection_unresolved",
repoFullName: meta.repoFullName,
pullNumber: meta.pullNumber,
baseRef: meta.baseRef ?? null,
}),
);
}

function evictLiveFactOnReject<T>(
cache: Map<string, Promise<T>>,
key: string,
Expand Down
4 changes: 4 additions & 0 deletions src/queue/processors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -412,6 +412,7 @@ import {
cachedRequiredStatusContexts,
liveFactKey,
liveFactTokenPart,
observeRequiredContextsLookup,
refreshLiveCiAggregate,
refreshLiveMergeState,
reuseOrRefreshLiveCiAggregate,
Expand Down Expand Up @@ -2877,6 +2878,9 @@ async function runAgentMaintenancePlanAndExecute(
// is not re-reviewed for the same state.
fetchLivePullRequestReviewDecision(env, repoFullName, pr.number, token, admissionKey),
]);
// #8358: consume `.resolved` — a branch-protection read failure still yields a config-fallback set, but
// without this signal the degraded path was silent. Observability only; disposition is unchanged.
observeRequiredContextsLookup(requiredContextsLookup, { repoFullName, pullNumber: pr.number, baseRef });
const requiredContexts = requiredContextsLookup.requiredContexts;
// Same reuse-this-pass-else-refresh-live rationale as reuseOrRefreshLiveMergeState above (#4498).
const ciAggregate = await reuseOrRefreshLiveCiAggregate(
Expand Down
83 changes: 81 additions & 2 deletions test/unit/ci-resolution.test.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,13 @@
import { afterEach, describe, expect, it, vi } from "vitest";
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
import * as backfillModule from "../../src/github/backfill";
import { cachedLiveCiAggregate } from "../../src/queue/ci-resolution";
import {
cachedLiveCiAggregate,
cachedRequiredStatusContexts,
observeRequiredContextsLookup,
REQUIRED_CONTEXTS_UNRESOLVED_METRIC,
} from "../../src/queue/ci-resolution";
import type { LiveGithubFacts } from "../../src/queue/processors";
import { counterValue, resetMetrics } from "../../src/selfhost/metrics";
import { createTestEnv } from "../helpers/d1";

function emptyFacts(): LiveGithubFacts {
Expand Down Expand Up @@ -79,3 +85,76 @@
expect(liveCiSpy).toHaveBeenCalledTimes(3); // +1 only, the reversed list reused the key
});
});

describe("cachedRequiredStatusContexts resolved flag (#8358)", () => {
afterEach(() => {
vi.restoreAllMocks();
});

it("sets resolved:false when the live branch-protection read fails (onFetchFailure fires) and keeps the expectedCiContexts config fallback", async () => {
const env = createTestEnv();
vi.spyOn(backfillModule, "fetchRequiredStatusContexts").mockImplementation(
async (_env, _repo, _base, _token, _admission, onFetchFailure) => {
onFetchFailure(new Error("branch protection forbidden"));

Check failure on line 98 in test/unit/ci-resolution.test.ts

View workflow job for this annotation

GitHub Actions / validate-code

Cannot invoke an object which is possibly 'undefined'.
return null;
},
);
const lookup = await cachedRequiredStatusContexts(env, "owner/repo", emptyFacts(), "main", "tok", ["lint"]);
expect(lookup.resolved).toBe(false);
expect(lookup.requiredContexts).toEqual(new Set(["lint"]));
});

it("sets resolved:true when the live branch-protection read succeeds", async () => {
const env = createTestEnv();
vi.spyOn(backfillModule, "fetchRequiredStatusContexts").mockResolvedValue(new Set(["ci"]));
const lookup = await cachedRequiredStatusContexts(env, "owner/repo", emptyFacts(), "main", "tok", null);
expect(lookup.resolved).toBe(true);
expect(lookup.requiredContexts).toEqual(new Set(["ci"]));
});
});

describe("observeRequiredContextsLookup (#8358)", () => {
beforeEach(() => {
resetMetrics();
});

afterEach(() => {
vi.restoreAllMocks();
});

it("emits a structured warn and increments the unresolved metric when resolved is false", () => {
const warn = vi.spyOn(console, "warn").mockImplementation(() => undefined);
observeRequiredContextsLookup(
{ requiredContexts: new Set(["lint"]), resolved: false },
{ repoFullName: "owner/repo", pullNumber: 9, baseRef: "main" },
);
expect(counterValue(REQUIRED_CONTEXTS_UNRESOLVED_METRIC)).toBe(1);
expect(warn).toHaveBeenCalledTimes(1);
expect(JSON.parse(String(warn.mock.calls[0]?.[0]))).toEqual({
level: "warn",
event: "required_contexts_branch_protection_unresolved",
repoFullName: "owner/repo",
pullNumber: 9,
baseRef: "main",
});
});

it("coalesces a nullish baseRef to null in the warn payload", () => {
const warn = vi.spyOn(console, "warn").mockImplementation(() => undefined);
observeRequiredContextsLookup(
{ requiredContexts: null, resolved: false },
{ repoFullName: "owner/repo", pullNumber: 3, baseRef: undefined },
);
expect(JSON.parse(String(warn.mock.calls[0]?.[0]))).toMatchObject({ baseRef: null });
});

it("is a no-op when resolved is true (no metric, no warn)", () => {
const warn = vi.spyOn(console, "warn").mockImplementation(() => undefined);
observeRequiredContextsLookup(
{ requiredContexts: new Set(["ci"]), resolved: true },
{ repoFullName: "owner/repo", pullNumber: 9, baseRef: "main" },
);
expect(counterValue(REQUIRED_CONTEXTS_UNRESOLVED_METRIC)).toBe(0);
expect(warn).not.toHaveBeenCalled();
});
});
7 changes: 6 additions & 1 deletion test/unit/queue.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ import * as repositoriesModule from "../../src/db/repositories";
import * as reviewEffortModule from "../../src/review/review-effort";
import * as repositorySettingsModule from "../../src/settings/repository-settings";
import * as sentryModule from "../../src/selfhost/sentry";
import { renderMetrics, resetMetrics } from "../../src/selfhost/metrics";
import { counterValue, renderMetrics, resetMetrics } from "../../src/selfhost/metrics";
import { REQUIRED_CONTEXTS_UNRESOLVED_METRIC } from "../../src/queue/ci-resolution";
import { jobCoalesceKey } from "../../src/selfhost/queue-common";
import {
listCollisionEdges,
Expand Down Expand Up @@ -3241,6 +3242,7 @@ describe("queue processors", () => {
// existed (see the sibling "#audit-rate-headroom: the per-PR re-review refreshes..." dedup test above).
it("REGRESSION (#selfhost-ci-verification): expectedCiContexts in the cache key does not defeat within-job required-contexts memoization", async () => {
const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() });
resetMetrics();
await upsertInstallation(env, { action: "created", installation: { id: 9001, account: { login: "owner", id: 1, type: "Organization" }, target_type: "Organization", repository_selection: "selected", permissions: { pull_requests: "write", checks: "write" }, events: [] } });
await upsertRepositoryFromGitHub(env, { name: "agent-repo", full_name: "owner/agent-repo", private: false, owner: { login: "owner" } }, 9001);
await upsertRepositorySettings(env, { repoFullName: "owner/agent-repo", autonomy: { merge: "auto", update_branch: "auto" }, autoMaintain: { requireApprovals: 0, mergeMethod: "squash" }, gatePack: "oss-anti-slop" });
Expand Down Expand Up @@ -3271,6 +3273,9 @@ describe("queue processors", () => {

// One fetch for the whole job despite three internal call sites sharing the config-aware cache key.
expect(branchProtectionGets).toBe(1);
// #8358: the 403 branch-protection read sets resolved:false; runAgentMaintenancePlanAndExecute must
// observe that (metric + warn) rather than silently consuming only `.requiredContexts`.
expect(counterValue(REQUIRED_CONTEXTS_UNRESOLVED_METRIC)).toBeGreaterThanOrEqual(1);
const deferred = await env.DB.prepare("select count(*) as n from audit_events where event_type = ?")
.bind("github_app.review_deferred_ci_pending")
.first<{ n: number }>();
Expand Down
Loading