From 3175d74823e030b8717da8ccf872ea8f670bb041 Mon Sep 17 00:00:00 2001 From: Nick M Date: Tue, 14 Jul 2026 11:32:34 -0500 Subject: [PATCH 1/2] feat(gate): configurable advisory check-runs so a stuck external status never freezes the gate (#4372) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a per-repo `gate.advisoryCheckRuns` field: a list of `{ name, appSlug }` third-party check-runs (a security scanner, a contributor-trust analyzer, a CLA bot) to treat as advisory. A durable non-standard conclusion (most often a terminal `action_required` only a human can clear in that app's own UI) today freezes gittensory's gate forever — the check never turns green, so the PR is held pending with no review, label, or signal. A listed check-run — matched by name AND the trusted producing app slug, the same spoof-resistant pattern as `cla.checkRunAppSlug` — is excluded from the CI aggregate entirely (never gates pass/fail, never counts as "still running"), so it can never block or stall the gate. It is not silently swallowed either: a non-passing conclusion routes the PR to the existing manual-review hold with the triggering check/app named in the label reason and public comment. Config-driven and generic: no vendor name is hardcoded in any behavior-triggering path. Empty/omitted list is byte-identical to today for every repo that doesn't opt in. Wires the field through the engine parser, resolved settings, resolver, OpenAPI, the CI aggregate (REST + GraphQL paths) and its cache keys, the disposition planner, and the executor/approval-queue re-checks, with docs in both example configs and full line+branch test coverage. --- .loopover.yml.example | 17 +++ apps/loopover-ui/public/openapi.json | 19 +++ config/examples/loopover.full.yml | 17 +++ .../loopover-engine/src/focus-manifest.ts | 55 +++++++++ .../src/types/manifest-deps-types.ts | 6 + .../src/types/predicted-gate-types.ts | 1 + src/github/backfill.ts | 61 +++++++++- src/openapi/schemas.ts | 1 + src/queue/ci-resolution.ts | 34 +++++- src/queue/processors.ts | 13 ++ src/services/agent-action-executor.ts | 7 +- src/services/agent-approval-queue.ts | 3 +- src/settings/agent-actions.ts | 32 ++++- src/signals/focus-manifest.ts | 1 + src/types.ts | 10 ++ test/unit/agent-action-executor.test.ts | 22 ++-- test/unit/agent-actions.test.ts | 27 +++++ test/unit/agent-approval-queue.test.ts | 15 +-- test/unit/backfill-2.test.ts | 113 +++++++++++++++++- test/unit/ci-resolution.test.ts | 29 +++++ test/unit/focus-manifest.test.ts | 36 +++++- test/unit/mcp-automation-state.test.ts | 2 +- test/unit/pr-detail-durable-cache.test.ts | 2 + test/unit/queue-2.test.ts | 1 + test/unit/queue-4.test.ts | 11 ++ test/unit/queue-5.test.ts | 1 + test/unit/queue.test.ts | 22 +++- test/unit/routes-agent-approval.test.ts | 2 +- 28 files changed, 519 insertions(+), 41 deletions(-) diff --git a/.loopover.yml.example b/.loopover.yml.example index 5ed5177d12..cd65173dc7 100644 --- a/.loopover.yml.example +++ b/.loopover.yml.example @@ -252,6 +252,23 @@ gate: - build - test + # Third-party check-runs to treat as ADVISORY (#4372). Any GitHub App you install alongside gittensory + # (a security scanner, a contributor-trust analyzer, a license/CLA bot, ...) can post a check-run whose + # terminal conclusion is outside GitHub's normal pass/fail vocabulary — most commonly a durable + # "action_required" that only a human can clear in that app's own UI, never on GitHub. Left alone, such a + # check freezes the gate: it never turns green, so the PR is held pending forever with no signal. List the + # check here (matched by name AND the trusted producing app slug — a name-only match is spoofable and is + # ignored, exactly like cla.checkRunAppSlug) to make it advisory: a matched, COMPLETED run is excluded from + # CI pass/fail entirely and never counts as "still running", so it can never block or stall the gate. It is + # NOT silently swallowed either — a non-passing conclusion routes the PR to the manual-review hold (the + # manualReviewLabel) with the triggering check/app named, so a maintainer can act on it. Generic: name the + # app(s) YOU run; gittensory hardcodes no vendor. List of { name, appSlug }, or omit. Default: not + # configured (byte-identical behavior for every repo that doesn't opt in). Config-as-code only — no DB + # column or dashboard toggle. + advisoryCheckRuns: + - name: Contributor trust + appSlug: example-security-app + # Promote a confident AI-judgment-only finding (one the reviewer itself placed under "Blockers", never # a "Nit") into a real, deterministic gate blocker instead of leaving it advisory (#3907). Only matters # for repos already running the registry content lane (see contentLane below) — content/registry repos diff --git a/apps/loopover-ui/public/openapi.json b/apps/loopover-ui/public/openapi.json index 65de5c204d..366b62ea0a 100644 --- a/apps/loopover-ui/public/openapi.json +++ b/apps/loopover-ui/public/openapi.json @@ -9640,6 +9640,25 @@ "aiReviewConfirmedContributorsOnly": { "type": "boolean", "nullable": true + }, + "advisoryCheckRuns": { + "type": "array", + "nullable": true, + "items": { + "type": "object", + "properties": { + "name": { + "type": "string" + }, + "appSlug": { + "type": "string" + } + }, + "required": [ + "name", + "appSlug" + ] + } } }, "required": [ diff --git a/config/examples/loopover.full.yml b/config/examples/loopover.full.yml index 3166bfd5af..6dc4672b75 100644 --- a/config/examples/loopover.full.yml +++ b/config/examples/loopover.full.yml @@ -266,6 +266,23 @@ gate: - build - test + # Third-party check-runs to treat as ADVISORY (#4372). Any GitHub App you install alongside gittensory + # (a security scanner, a contributor-trust analyzer, a license/CLA bot, ...) can post a check-run whose + # terminal conclusion is outside GitHub's normal pass/fail vocabulary — most commonly a durable + # "action_required" that only a human can clear in that app's own UI, never on GitHub. Left alone, such a + # check freezes the gate: it never turns green, so the PR is held pending forever with no signal. List the + # check here (matched by name AND the trusted producing app slug — a name-only match is spoofable and is + # ignored, exactly like cla.checkRunAppSlug) to make it advisory: a matched, COMPLETED run is excluded from + # CI pass/fail entirely and never counts as "still running", so it can never block or stall the gate. It is + # NOT silently swallowed either — a non-passing conclusion routes the PR to the manual-review hold (the + # manualReviewLabel) with the triggering check/app named, so a maintainer can act on it. Generic: name the + # app(s) YOU run; gittensory hardcodes no vendor. List of { name, appSlug }, or omit. Default: not + # configured (byte-identical behavior for every repo that doesn't opt in). Config-as-code only — no DB + # column or dashboard toggle. + advisoryCheckRuns: + - name: Contributor trust + appSlug: example-security-app + # Promote a confident AI-judgment-only finding (one the reviewer itself placed under "Blockers", never # a "Nit") into a real, deterministic gate blocker instead of leaving it advisory (#3907). Only matters # for repos already running the registry content lane (see contentLane below) — content/registry repos diff --git a/packages/loopover-engine/src/focus-manifest.ts b/packages/loopover-engine/src/focus-manifest.ts index 8f5e14e78d..3d75ee12e5 100644 --- a/packages/loopover-engine/src/focus-manifest.ts +++ b/packages/loopover-engine/src/focus-manifest.ts @@ -186,6 +186,15 @@ export type FocusManifestGateConfig = { * (unset) ⇒ no generic fallback configured — the live-CI aggregate keeps today's fold-all behavior * when branch protection is also unreadable. See {@link RepositorySettings.expectedCiContexts}. */ expectedCiContexts: ReadonlyArray | null; + /** `gate.advisoryCheckRuns` (#4372): third-party check-runs (a security scanner, a contributor-trust + * analyzer, etc.) whose terminal conclusion may be outside GitHub's pass/fail vocabulary (e.g. a durable + * `action_required`). Each `{ name, appSlug }` is matched by name and trusted only when produced by that + * app slug — the same spoof-resistant pattern as `cla.checkRunName`/`checkRunAppSlug`. A matched, COMPLETED + * run is excluded from the live-CI aggregate (never gates pass/fail, never counts as "still running"); a + * non-passing conclusion routes the PR to the manual-review hold instead of being swallowed. Generic and + * config-only — no vendor name is ever hardcoded in behavior. null/empty (unset) ⇒ byte-identical to today. + * See {@link RepositorySettings.advisoryCheckRuns}. */ + advisoryCheckRuns: ReadonlyArray<{ name: string; appSlug: string }> | null; /** `gate.aiJudgmentBlockers` (#3907): "gate" | "advisory", null (unset) ⇒ "advisory" (byte-identical to * today everywhere that doesn't opt in). Config-as-code only, YML-only (no DB column, no dashboard * toggle) — mirrors `contentLane`'s own YML-only shape, since this only has an effect for repos already @@ -996,6 +1005,7 @@ const EMPTY_GATE_CONFIG: FocusManifestGateConfig = { claCheckRunName: null, claCheckRunAppSlug: null, expectedCiContexts: null, + advisoryCheckRuns: null, aiJudgmentBlockersMode: null, copycatMode: null, copycatMinScore: null, @@ -1280,6 +1290,46 @@ function normalizeOptionalReviewers( return out.length > 0 ? out : null; } +const MAX_ADVISORY_CHECK_RUNS = 16; + +/** + * Normalize `gate.advisoryCheckRuns` (#4372): a list of `{ name, appSlug }` pairs identifying third-party + * check-runs to treat as advisory. Both fields are required non-empty strings — `appSlug` is mandatory (not + * optional like the reviewers `fallback`) for the same reason the CLA path requires it: matching a check-run + * by name alone is spoofable, so an entry missing its trusted app slug is dropped rather than trusted. + */ +function normalizeOptionalAdvisoryCheckRuns( + value: JsonValue | undefined, + field: string, + warnings: string[], +): ReadonlyArray<{ name: string; appSlug: string }> | null { + if (value === undefined || value === null) return null; + if (!Array.isArray(value)) { + warnings.push(`Manifest gate field "${field}" must be a list of { name, appSlug }; ignoring it.`); + return null; + } + const out: Array<{ name: string; appSlug: string }> = []; + for (const [index, entry] of value.entries()) { + if (out.length >= MAX_ADVISORY_CHECK_RUNS) { + warnings.push(`Manifest gate field "${field}" is capped at ${MAX_ADVISORY_CHECK_RUNS} entries; dropping the rest.`); + break; + } + if (entry === null || typeof entry !== "object" || Array.isArray(entry)) { + warnings.push(`Manifest gate field "${field}[${index}]" must be a mapping with "name" and "appSlug" strings; ignoring it.`); + continue; + } + const e = entry as Record; + const name = typeof e.name === "string" ? e.name.trim() : ""; + const appSlug = typeof e.appSlug === "string" ? e.appSlug.trim() : ""; + if (!name || !appSlug) { + warnings.push(`Manifest gate field "${field}[${index}]" needs a non-empty "name" AND "appSlug" (name-only matching is spoofable); ignoring the entry.`); + continue; + } + out.push({ name, appSlug }); + } + return out.length > 0 ? out : null; +} + /** * Parse the optional `gate:` mapping. Every field stays `null` when unset so the resolver can layer * this OVER DB settings without clobbering. A nested `readiness: { mode, minScore }` block is accepted. @@ -1363,6 +1413,7 @@ function parseGateConfig(value: JsonValue | undefined, warnings: string[]): Focu claCheckRunName: parsePublicSafeText(claRecord?.checkRunName, "gate.cla.checkRunName", warnings), claCheckRunAppSlug: parsePublicSafeText(claRecord?.checkRunAppSlug, "gate.cla.checkRunAppSlug", warnings), expectedCiContexts: normalizeOptionalStringList(record.expectedCiContexts, "gate.expectedCiContexts", warnings), + advisoryCheckRuns: normalizeOptionalAdvisoryCheckRuns(record.advisoryCheckRuns, "gate.advisoryCheckRuns", warnings), aiJudgmentBlockersMode: normalizeOptionalEnum(record.aiJudgmentBlockers, "gate.aiJudgmentBlockers", ["gate", "advisory"] as const, warnings), copycatMode: normalizeOptionalEnum(copycatRecord?.mode, "gate.copycat.mode", ["off", "warn", "label", "block"] as const, warnings), copycatMinScore: normalizeOptionalScore(copycatRecord?.minScore, "gate.copycat.minScore", warnings), @@ -1423,6 +1474,7 @@ function parseGateConfig(value: JsonValue | undefined, warnings: string[]): Focu gate.claCheckRunName !== null || gate.claCheckRunAppSlug !== null || gate.expectedCiContexts !== null || + gate.advisoryCheckRuns !== null || gate.aiJudgmentBlockersMode !== null || gate.copycatMode !== null || gate.copycatMinScore !== null; @@ -1502,6 +1554,9 @@ export function gateConfigToJson(gate: FocusManifestGateConfig): JsonValue { out.cla = cla; } if (gate.expectedCiContexts !== null) out.expectedCiContexts = gate.expectedCiContexts as JsonValue; + if (gate.advisoryCheckRuns !== null) { + out.advisoryCheckRuns = gate.advisoryCheckRuns.map((c) => ({ name: c.name, appSlug: c.appSlug })) as JsonValue; + } if (gate.aiJudgmentBlockersMode !== null) out.aiJudgmentBlockers = gate.aiJudgmentBlockersMode; if (gate.copycatMode !== null || gate.copycatMinScore !== null) { const copycat: Record = {}; diff --git a/packages/loopover-engine/src/types/manifest-deps-types.ts b/packages/loopover-engine/src/types/manifest-deps-types.ts index b21424f1e1..57f5ee0cf4 100644 --- a/packages/loopover-engine/src/types/manifest-deps-types.ts +++ b/packages/loopover-engine/src/types/manifest-deps-types.ts @@ -229,6 +229,12 @@ export type RepositorySettings = { * ⇒ verified passed (no `ciCompletenessWarning`). Config-as-code only — no DB column; set via * `.loopover.yml gate.expectedCiContexts`. */ expectedCiContexts?: ReadonlyArray | null | undefined; + /** `gate.advisoryCheckRuns` (#4372): third-party check-runs to treat as advisory — each `{ name, appSlug }` + * matched by name and trusted only when produced by that app slug (spoof-resistant, like the CLA check-run + * fields). A matched, COMPLETED run is excluded from the live-CI aggregate (never gates pass/fail, never + * counts as "still running"); a non-passing conclusion routes the PR to the manual-review hold instead of + * being swallowed. Config-as-code only — no DB column; set via `.loopover.yml gate.advisoryCheckRuns`. */ + advisoryCheckRuns?: ReadonlyArray<{ name: string; appSlug: string }> | null | undefined; /** Dry-run disposition (#gate-dryrun). When true, the gate renders the would-be merge/close/manual verdict (every * advisory sub-gate promoted to block) WITHOUT enforcing — the posted check stays non-blocking. Lets advisory mode * preview exactly what it would do before the maintainer flips to real enforcement. Default off. */ diff --git a/packages/loopover-engine/src/types/predicted-gate-types.ts b/packages/loopover-engine/src/types/predicted-gate-types.ts index f941878ffd..cbdd07bb53 100644 --- a/packages/loopover-engine/src/types/predicted-gate-types.ts +++ b/packages/loopover-engine/src/types/predicted-gate-types.ts @@ -314,6 +314,7 @@ export type FocusManifestGateConfig = { claCheckRunName: string | null; claCheckRunAppSlug: string | null; expectedCiContexts: ReadonlyArray | null; + advisoryCheckRuns: ReadonlyArray<{ name: string; appSlug: string }> | null; }; export type PreMergeCheck = { diff --git a/src/github/backfill.ts b/src/github/backfill.ts index 38c589da27..b0f8e5ae66 100644 --- a/src/github/backfill.ts +++ b/src/github/backfill.ts @@ -2533,6 +2533,21 @@ function isBotOwnedRequiredContextName(name: string): boolean { return BOT_OWNED_CHECK_NAMES.has(name); } +// #4372: does this check-run match a maintainer-declared advisory entry? Matched by name (case-insensitive) AND +// the trusted producing app slug — the SAME spoof-resistant pattern as the CLA check-run detection: a +// contributor-controlled same-name run from a different app must never be trusted as advisory. Generic — no +// vendor name is ever hardcoded here; the list comes entirely from `.loopover.yml gate.advisoryCheckRuns`. +function matchAdvisoryCheckRun( + run: { name: string; app?: { slug?: string | null } | null }, + advisoryCheckRuns: ReadonlyArray<{ name: string; appSlug: string }> | null | undefined, +): { name: string; appSlug: string } | undefined { + if (!advisoryCheckRuns || advisoryCheckRuns.length === 0) return undefined; + const nameLc = run.name.trim().toLowerCase(); + const appSlugLc = typeof run.app?.slug === "string" ? run.app.slug.trim().toLowerCase() : ""; + if (!appSlugLc) return undefined; // a spoofable name-only match is never trusted (mirrors the CLA path) + return advisoryCheckRuns.find((entry) => entry.name.trim().toLowerCase() === nameLc && entry.appSlug.trim().toLowerCase() === appSlugLc); +} + function normalizeCiContextName(name: string): string { const trimmed = name.trim(); const slashIndex = trimmed.lastIndexOf("/"); @@ -2572,6 +2587,12 @@ export type LiveCiAggregate = { failingDetails: Array<{ name: string; summary?: string; detailsUrl?: string }>; // Historical compatibility: non-required red checks are now folded into failingDetails so this stays empty. nonRequiredFailingDetails: Array<{ name: string; summary?: string; detailsUrl?: string }>; + // #4372: a maintainer-declared `gate.advisoryCheckRuns` check-run that resolved COMPLETED to a NON-passing + // conclusion (anything other than success/neutral/skipped — e.g. a scanner's durable `action_required`). Such + // a run is excluded from ciState/hasPending entirely (never gates CI, never counts as "still running"), but is + // surfaced here so the disposition planner can route the PR to a manual-review hold instead of silently + // swallowing a signal a maintainer installed a whole app to raise. Empty for every repo that doesn't opt in. + advisoryHoldDetails: Array<{ name: string; appSlug: string; conclusion: string }>; // Informational-only (#2137): set when the aggregate resolved to "passed" with no branch-protection required // contexts configured (`enforceRequiredOnly` false) — meaning a workflow that never triggers on this commit at // all (e.g. path-filtered out, or a broken YAML trigger) is indistinguishable from one that doesn't exist, and @@ -2796,12 +2817,13 @@ async function reduceLiveCiAggregate( checkRuns: ReadonlyArray; statuses: ReadonlyArray; requiredContexts: ReadonlySet | null | undefined; + advisoryCheckRuns?: ReadonlyArray<{ name: string; appSlug: string }> | null | undefined; checkRunsIncomplete: boolean; statusIncomplete: boolean; fetchSuites: () => Promise | null>; }, ): Promise { - const { checkRuns, statuses, requiredContexts, checkRunsIncomplete, statusIncomplete, fetchSuites } = inputs; + const { checkRuns, statuses, requiredContexts, advisoryCheckRuns, checkRunsIncomplete, statusIncomplete, fetchSuites } = inputs; const enforceRequiredOnly = requiredContexts != null && requiredContexts.size > 0; const isRequired = (name: string): boolean => !enforceRequiredOnly || requiredContexts!.has(name); // Deliberately the OPPOSITE unknown-case default from isRequired() above, and used ONLY for a third-party @@ -2819,6 +2841,7 @@ async function reduceLiveCiAggregate( const isConfirmedRequired = (name: string): boolean => enforceRequiredOnly && requiredContexts!.has(name); const failingDetails: LiveCiAggregate["failingDetails"] = []; const nonRequiredFailingDetails: LiveCiAggregate["nonRequiredFailingDetails"] = []; + const advisoryHoldDetails: LiveCiAggregate["advisoryHoldDetails"] = []; let total = 0; let anyPending = false; let anyVisiblePending = false; @@ -2837,6 +2860,21 @@ async function reduceLiveCiAggregate( const appSlug = (run.app?.slug ?? "").toLowerCase(); if (appSlug === "github-actions") sawFirstPartyCheckRun = true; if (isOwnGitHubAppCheckRun(env, run)) continue; // never wait on the bot's own Gate/Context check-runs + // #4372: a maintainer-declared advisory check-run is fully excluded from the CI aggregate — the SAME way + // bot-owned checks are above — so it never gates pass/fail and never counts as "still running" (fixing the + // permanent hold a durable non-standard conclusion causes). But it is not silently swallowed: once COMPLETED + // with a non-passing conclusion, it is recorded in advisoryHoldDetails so the disposition planner can route + // the PR to a manual-review hold. An advisory check still in progress is simply ignored (it may yet pass); + // nothing about it holds the gate either way. + const advisoryMatch = matchAdvisoryCheckRun(run, advisoryCheckRuns); + if (advisoryMatch) { + const advisoryConclusion = (run.conclusion ?? "").toLowerCase(); + const advisoryStatus = (run.status ?? "").toLowerCase(); + if (advisoryStatus === "completed" && advisoryConclusion !== "" && !CI_PASSING_CONCLUSIONS.has(advisoryConclusion)) { + advisoryHoldDetails.push({ name: advisoryMatch.name, appSlug: advisoryMatch.appSlug, conclusion: advisoryConclusion }); + } + continue; + } total += 1; const conclusion = (run.conclusion ?? "").toLowerCase(); const status = (run.status ?? "").toLowerCase(); @@ -2947,7 +2985,7 @@ async function reduceLiveCiAggregate( // A partial/paginated read can't tell "never appears" from "appears on a page we didn't fetch" -- only a // COMPLETE read's absence is a confident signal worth a short surfacing cap (#selfhost-ci-deferral-staleness). const hasMissingRequiredContext = anyMissingRequiredContext && !checkRunsIncomplete && !statusIncomplete; - return { ciState, hasPending, hasVisiblePending: anyRequiredVisiblePending, hasMissingRequiredContext, failingDetails, nonRequiredFailingDetails, ciCompletenessWarning }; + return { ciState, hasPending, hasVisiblePending: anyRequiredVisiblePending, hasMissingRequiredContext, failingDetails, nonRequiredFailingDetails, advisoryHoldDetails, ciCompletenessWarning }; } /** @@ -2968,8 +3006,11 @@ export async function fetchLiveCiAggregate( // Completed red checks/statuses still fail the aggregate even when they are not branch-protection-required. requiredContexts?: ReadonlySet | null, admissionKey?: GitHubRateLimitAdmissionKey, + // #4372: maintainer-declared advisory check-runs (trailing/optional — every existing positional caller passing + // requiredContexts+admissionKey stays byte-identical). Excluded from the aggregate; non-passing ⇒ advisoryHoldDetails. + advisoryCheckRuns?: ReadonlyArray<{ name: string; appSlug: string }> | null, ): Promise { - if (!headSha) return { ciState: "unverified", hasPending: false, hasVisiblePending: false, hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], ciCompletenessWarning: null }; + if (!headSha) return { ciState: "unverified", hasPending: false, hasVisiblePending: false, hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], advisoryHoldDetails: [], ciCompletenessWarning: null }; // Check-runs + classic statuses are accumulated across pages here; the single classification lives in // reduceLiveCiAggregate so the REST and GraphQL paths reach byte-identical verdicts (#1941). const checkRuns: LiveCiCheckRun[] = []; @@ -3012,6 +3053,7 @@ export async function fetchLiveCiAggregate( checkRuns, statuses, requiredContexts, + advisoryCheckRuns, checkRunsIncomplete, statusIncomplete, // Lazily read the check-SUITES backstop only when the reducer finds the cheaper sources fully settled; a fetch @@ -3051,6 +3093,7 @@ export async function fetchLiveCiAggregateViaGraphQl( token: string | undefined, requiredContexts?: ReadonlySet | null, admissionKey?: GitHubRateLimitAdmissionKey, + advisoryCheckRuns?: ReadonlyArray<{ name: string; appSlug: string }> | null, ): Promise { if (!headSha || !token) return null; const [owner, name] = repoFullName.split("/"); @@ -3130,6 +3173,7 @@ export async function fetchLiveCiAggregateViaGraphQl( checkRuns, statuses, requiredContexts, + advisoryCheckRuns, checkRunsIncomplete: false, statusIncomplete: false, fetchSuites: async () => suites, // already fetched in the same query — never a second round-trip @@ -3149,14 +3193,15 @@ export async function fetchLiveCiAggregatePreferGraphQl( token: string | undefined, requiredContexts?: ReadonlySet | null, admissionKey?: GitHubRateLimitAdmissionKey, + advisoryCheckRuns?: ReadonlyArray<{ name: string; appSlug: string }> | null, ): Promise { if (isStatusRollupGraphQlEnabled(env)) { // fetchLiveCiAggregateViaGraphQl handles all its own errors and returns null on any uncertainty (it never // rejects), so a null result — not a throw — is the fall-back-to-REST signal. - const rollup = await fetchLiveCiAggregateViaGraphQl(env, repoFullName, headSha, token, requiredContexts, admissionKey); + const rollup = await fetchLiveCiAggregateViaGraphQl(env, repoFullName, headSha, token, requiredContexts, admissionKey, advisoryCheckRuns); if (rollup) return rollup; } - return fetchLiveCiAggregate(env, repoFullName, headSha, token, requiredContexts, admissionKey); + return fetchLiveCiAggregate(env, repoFullName, headSha, token, requiredContexts, admissionKey, advisoryCheckRuns); } /** @@ -3628,6 +3673,12 @@ export function deserializeCachedCiAggregate( hasMissingRequiredContext: cached.ciHasMissingRequiredContext ?? false, failingDetails, nonRequiredFailingDetails, + // #4372: advisoryHoldDetails is NOT persisted in the durable cache (no column) — deliberately. The + // manual-review routing it drives fires on the fresh, webhook-invalidated live read that populates this + // cache entry (an advisory check_run completing invalidates the entry for that head SHA), and the hold + // label is applied idempotently there. The exclusion's effect on the disposition (a settled advisory check + // no longer holding the gate) DOES round-trip, because it is already baked into the cached `ciState`. + advisoryHoldDetails: [], ciCompletenessWarning: cached.ciCompletenessWarning ?? null, }; } catch { diff --git a/src/openapi/schemas.ts b/src/openapi/schemas.ts index c192992f5d..2337271c88 100644 --- a/src/openapi/schemas.ts +++ b/src/openapi/schemas.ts @@ -687,6 +687,7 @@ export const RepositorySettingsSchema = z claCheckRunName: z.string().nullable().optional(), claCheckRunAppSlug: z.string().nullable().optional(), expectedCiContexts: z.array(z.string()).optional(), + advisoryCheckRuns: z.array(z.object({ name: z.string(), appSlug: z.string() })).nullable().optional(), copycatGateMode: z.enum(["off", "warn", "label", "block"]).optional(), copycatGateMinScore: z.number().nullable().optional(), gateDryRun: z.boolean().optional(), diff --git a/src/queue/ci-resolution.ts b/src/queue/ci-resolution.ts index 98eaee9b4b..9cba4dd293 100644 --- a/src/queue/ci-resolution.ts +++ b/src/queue/ci-resolution.ts @@ -64,6 +64,15 @@ function resolvedRequiredContextsKeyPart(requiredContexts: ReadonlySet | return JSON.stringify([...requiredContexts].sort()); } +// #4372: stable, order-independent cache-key fragment for settings.advisoryCheckRuns. The advisory list changes +// the aggregate (which checks are excluded from ciState/hasPending), so a config change must invalidate both the +// request-scoped memo AND the durable cross-job CI-state cache — otherwise a stale entry from before the change +// would keep gating on (or excluding) the wrong checks. Two equal lists in any order collapse to one key. +function advisoryCheckRunsKeyPart(advisoryCheckRuns: ReadonlyArray<{ name: string; appSlug: string }> | null | undefined): string { + if (!advisoryCheckRuns || advisoryCheckRuns.length === 0) return ""; + return JSON.stringify(advisoryCheckRuns.map((c) => `${c.name}\0${c.appSlug}`).sort()); +} + // RC2 + #selfhost-ci-verification: the EFFECTIVE required-status-check contexts for this repo/baseRef, merging // live branch-protection required contexts with the maintainer-configured settings.expectedCiContexts fallback // (mergeRequiredCiContexts — branch protection stays authoritative when readable; expectedCiContexts is the @@ -141,6 +150,7 @@ async function cachedFetchLiveCiAggregate( token: string | undefined; requiredContexts: ReadonlySet | null | undefined; requiredContextsKey: string; + advisoryCheckRuns: ReadonlyArray<{ name: string; appSlug: string }> | null | undefined; forceRefresh: boolean; // False when the caller's own required-context lookup FAILED (not merely resolved to "none configured") -- // that fail-open aggregate must never be persisted under the normal key, or a transient lookup error would @@ -159,7 +169,7 @@ async function cachedFetchLiveCiAggregate( } } incr(CI_STATE_CACHE_METRIC, { field: "aggregate", result: args.forceRefresh ? "forced" : "miss" }); - const live = await fetchLiveCiAggregatePreferGraphQl(env, args.repoFullName, args.headSha, args.token, args.requiredContexts, args.admissionKey); + const live = await fetchLiveCiAggregatePreferGraphQl(env, args.repoFullName, args.headSha, args.token, args.requiredContexts, args.admissionKey, args.advisoryCheckRuns); if (args.requiredContextsResolved) { await writeThroughCiStateCache(env, args.repoFullName, args.prNumber, cached, args.headSha, args.requiredContextsKey, live); } @@ -176,6 +186,7 @@ function fetchLiveCiAggregateWithRequiredContexts( baseRef: string | null | undefined; token: string | undefined; expectedCiContexts: ReadonlyArray | null | undefined; + advisoryCheckRuns: ReadonlyArray<{ name: string; appSlug: string }> | null | undefined; forceRefresh: boolean; admissionKey?: GitHubRateLimitAdmissionKey | undefined; }, @@ -194,7 +205,11 @@ function fetchLiveCiAggregateWithRequiredContexts( headSha: args.headSha, token: args.token, requiredContexts, - requiredContextsKey: resolvedRequiredContextsKeyPart(requiredContexts), + // #4372: the advisory-check-runs config changes the aggregate but is NOT part of the resolved required + // contexts, so fold its fingerprint into the durable cache key alongside them — else a config change + // would keep serving a stale aggregate computed against the old advisory list. + requiredContextsKey: `${resolvedRequiredContextsKeyPart(requiredContexts)}|adv:${advisoryCheckRunsKeyPart(args.advisoryCheckRuns)}`, + advisoryCheckRuns: args.advisoryCheckRuns, forceRefresh: args.forceRefresh, requiredContextsResolved: resolved, admissionKey: args.admissionKey, @@ -212,10 +227,11 @@ export function cachedLiveCiAggregate( baseRef: string | null | undefined; token: string | undefined; expectedCiContexts: ReadonlyArray | null | undefined; + advisoryCheckRuns: ReadonlyArray<{ name: string; appSlug: string }> | null | undefined; admissionKey?: GitHubRateLimitAdmissionKey | undefined; }, ): Promise { - const key = liveFactKey(args.repoFullName, args.headSha, args.baseRef, liveFactTokenPart(args.token), expectedCiContextsKeyPart(args.expectedCiContexts)); + const key = liveFactKey(args.repoFullName, args.headSha, args.baseRef, liveFactTokenPart(args.token), `${expectedCiContextsKeyPart(args.expectedCiContexts)}|adv:${advisoryCheckRunsKeyPart(args.advisoryCheckRuns)}`); const cached = args.facts.ciAggregates.get(key); if (cached) return cached; const next = evictLiveFactOnReject( @@ -229,6 +245,7 @@ export function cachedLiveCiAggregate( baseRef: args.baseRef, token: args.token, expectedCiContexts: args.expectedCiContexts, + advisoryCheckRuns: args.advisoryCheckRuns, forceRefresh: false, admissionKey: args.admissionKey, }), @@ -247,10 +264,11 @@ export function refreshLiveCiAggregate( baseRef: string | null | undefined; token: string | undefined; expectedCiContexts: ReadonlyArray | null | undefined; + advisoryCheckRuns: ReadonlyArray<{ name: string; appSlug: string }> | null | undefined; admissionKey?: GitHubRateLimitAdmissionKey | undefined; }, ): Promise { - const key = liveFactKey(args.repoFullName, args.headSha, args.baseRef, liveFactTokenPart(args.token), expectedCiContextsKeyPart(args.expectedCiContexts)); + const key = liveFactKey(args.repoFullName, args.headSha, args.baseRef, liveFactTokenPart(args.token), `${expectedCiContextsKeyPart(args.expectedCiContexts)}|adv:${advisoryCheckRunsKeyPart(args.advisoryCheckRuns)}`); const next = evictLiveFactOnReject( args.facts.ciAggregates, key, @@ -262,6 +280,7 @@ export function refreshLiveCiAggregate( baseRef: args.baseRef, token: args.token, expectedCiContexts: args.expectedCiContexts, + advisoryCheckRuns: args.advisoryCheckRuns, forceRefresh: true, admissionKey: args.admissionKey, }), @@ -355,9 +374,12 @@ export function reuseOrRefreshLiveCiAggregate( token: string | undefined, expectedCiContexts: ReadonlyArray | null | undefined, admissionKey?: GitHubRateLimitAdmissionKey, + // #4372: trailing/optional so existing positional callers stay byte-identical (advisoryCheckRuns undefined ⇒ + // exclusion off, today's behavior). Folded into the memo key alongside expectedCiContexts, like the entry points. + advisoryCheckRuns?: ReadonlyArray<{ name: string; appSlug: string }> | null, ): Promise { - const key = liveFactKey(repoFullName, headSha, baseRef, liveFactTokenPart(token), expectedCiContextsKeyPart(expectedCiContexts)); + const key = liveFactKey(repoFullName, headSha, baseRef, liveFactTokenPart(token), `${expectedCiContextsKeyPart(expectedCiContexts)}|adv:${advisoryCheckRunsKeyPart(advisoryCheckRuns)}`); const cached = facts.forcedCiAggregateKeys.has(key) ? facts.ciAggregates.get(key) : undefined; if (cached) return cached; - return refreshLiveCiAggregate(env, { repoFullName, facts, prNumber, headSha, baseRef, token, expectedCiContexts, admissionKey }); + return refreshLiveCiAggregate(env, { repoFullName, facts, prNumber, headSha, baseRef, token, expectedCiContexts, advisoryCheckRuns, admissionKey }); } diff --git a/src/queue/processors.ts b/src/queue/processors.ts index af5ed3280a..6de89a7f52 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -2424,6 +2424,12 @@ function buildAgentMaintenancePlanInput(args: { ciHasPending: ciAggregate.hasPending, failingCheckNames: ciAggregate.failingDetails.map((detail) => detail.name), ciRequiredContextsVerified: hasVerifiedRequiredContexts(requiredContexts), + // #4372: any maintainer-declared advisory check-run that resolved to a non-passing terminal conclusion. It + // never gated CI (excluded from ciState above), but it must not be silently swallowed — surface it so the + // planner routes the PR to a manual-review HOLD naming the triggering check/app. Always threaded (the + // aggregate's field is always an array, [] when none); the planner applies its own length>0 gate, matching + // how failingCheckNames above is likewise threaded unconditionally. + advisoryCheckHold: ciAggregate.advisoryHoldDetails, ...(blacklistEntry !== null ? { blacklistMatch: { matched: true, reason: blacklistEntry.reason } } : {}), @@ -2560,6 +2566,7 @@ async function runAgentMaintenancePlanAndExecute( token, settings.expectedCiContexts, admissionKey, + settings.advisoryCheckRuns, ); // #2137: informational-only nudge for the operator — never affects the disposition below (ciState is // unchanged). recordAuditEvent is a DB write with its own internal failure handling; a failure here must @@ -3127,6 +3134,7 @@ async function runAgentMaintenancePlanAndExecute( // merge or a CI-driven close) must honor the same effective branch-protection-plus-expected contexts this // plan was evaluated against, or the two can disagree on ciState. requiredCiContexts: requiredContexts, + advisoryCheckRuns: settings.advisoryCheckRuns, // #4372: same exclusion the plan used, for step-8 re-verify // #3472 split-brain: the executor's own live manual-review hold guard (immediately before approve/merge) // must check the SAME configured label the planner itself resolves labels.manualReview from. manualReviewLabel: settings.manualReviewLabel, @@ -3481,6 +3489,7 @@ async function prReadyForReview( baseRef: pr.baseRef, token, expectedCiContexts: settings.expectedCiContexts, + advisoryCheckRuns: settings.advisoryCheckRuns, admissionKey, }).catch(() => undefined); if (ci?.hasPending) { @@ -7441,6 +7450,7 @@ async function resolveManifestPassedValidationCount( baseRef: string | null | undefined; body: string | null | undefined; expectedCiContexts: ReadonlyArray | null | undefined; + advisoryCheckRuns: ReadonlyArray<{ name: string; appSlug: string }> | null | undefined; liveFacts: LiveGithubFacts; testExpectationsConfigured: boolean; testFileCount: number; @@ -7467,6 +7477,7 @@ async function resolveManifestPassedValidationCount( baseRef: args.baseRef, token, expectedCiContexts: args.expectedCiContexts, + advisoryCheckRuns: args.advisoryCheckRuns, admissionKey, }); return liveCi.ciState === "passed" ? 1 : 0; @@ -7528,6 +7539,7 @@ async function maybeApplyManifestPolicyGate( baseRef: args.pr.baseRef ?? args.repo?.defaultBranch, body: args.pr.body, expectedCiContexts: args.settings.expectedCiContexts, + advisoryCheckRuns: args.settings.advisoryCheckRuns, liveFacts: args.webhook.liveFacts, testExpectationsConfigured: manifest.testExpectations.length > 0, testFileCount, @@ -9971,6 +9983,7 @@ async function maybePublishPrPublicSurface( baseRef, token, expectedCiContexts: settings.expectedCiContexts, + advisoryCheckRuns: settings.advisoryCheckRuns, admissionKey, }); // Live merge-state too — the SAME source the disposition uses (planAgentMaintenanceActions reads liveMergeState). diff --git a/src/services/agent-action-executor.ts b/src/services/agent-action-executor.ts index a618f93224..f2c8c21f83 100644 --- a/src/services/agent-action-executor.ts +++ b/src/services/agent-action-executor.ts @@ -176,6 +176,11 @@ export type AgentActionExecutionContext = { // must honor the SAME branch-protection-plus-expected required-contexts view the planning pass already // evaluated against. Absent/undefined ⇒ fold-all mode, unchanged from before this field existed. requiredCiContexts?: ReadonlySet | null | undefined; + // settings.advisoryCheckRuns (#4372), resolved by the CALLER (same "no settings access" shape as + // requiredCiContexts above): the step-8 live-CI re-verification must apply the SAME advisory-check-run + // exclusion the planning pass used — otherwise the executor could see a maintainer-declared advisory check as + // failing/pending and block a merge the planner already cleared. Absent ⇒ exclusion off, unchanged from before. + advisoryCheckRuns?: ReadonlyArray<{ name: string; appSlug: string }> | null | undefined; // settings.manualReviewLabel (#3472 split-brain), resolved by the CALLER (same "the executor has no settings // access" shape as requiredCiContexts above): the approve/merge live label guard (step 7b below) needs the // SAME configured label name the planner itself resolves labels.manualReview from (agent-actions.ts), so a @@ -422,7 +427,7 @@ export async function executeAgentMaintenanceActions(env: Env, ctx: AgentActionE const admissionKey = githubRateLimitAdmissionKeyForToken(env, ciToken, ctx.installationId); const [liveCi, liveMergeableState, liveThreadBlockers, liveWinnerState] = await Promise.all([ requiresLiveCiRecheck - ? fetchLiveCiAggregate(env, ctx.repoFullName, expectedHeadSha, ciToken, ctx.requiredCiContexts ?? null, admissionKey) + ? fetchLiveCiAggregate(env, ctx.repoFullName, expectedHeadSha, ciToken, ctx.requiredCiContexts ?? null, admissionKey, ctx.advisoryCheckRuns ?? null) : Promise.resolve(undefined), requiresLiveMergeableRecheck ? fetchLivePullRequestMergeState(env, ctx.repoFullName, ctx.pullNumber, ciToken, admissionKey) : Promise.resolve(undefined), requiresLiveThreadRecheck ? fetchLiveReviewThreadBlockers(env, ctx.repoFullName, ctx.pullNumber, ciToken, admissionKey) : Promise.resolve(undefined), diff --git a/src/services/agent-approval-queue.ts b/src/services/agent-approval-queue.ts index a061e386f7..7a929f7c52 100644 --- a/src/services/agent-approval-queue.ts +++ b/src/services/agent-approval-queue.ts @@ -234,7 +234,7 @@ export async function decidePendingAgentAction(env: Env, input: { id: string; de shouldRecheckLiveDisposition ? fetchRequiredStatusContexts(env, pending.repoFullName, pr!.baseRef, token, admissionKey) .then((branchProtectionContexts) => mergeRequiredCiContexts(branchProtectionContexts, settings.expectedCiContexts)) - .then((requiredContexts) => fetchLiveCiAggregate(env, pending.repoFullName, pr!.headSha, token, requiredContexts, admissionKey)) + .then((requiredContexts) => fetchLiveCiAggregate(env, pending.repoFullName, pr!.headSha, token, requiredContexts, admissionKey, settings.advisoryCheckRuns)) : Promise.resolve(undefined), shouldRecheckLiveDisposition ? fetchLivePullRequestMergeState(env, pending.repoFullName, pending.pullNumber, token, admissionKey) : Promise.resolve(undefined), shouldRecheckLiveDisposition ? fetchLivePullRequestReviewDecision(env, pending.repoFullName, pending.pullNumber, token, admissionKey) : Promise.resolve(undefined), @@ -419,6 +419,7 @@ export async function decidePendingAgentAction(env: Env, input: { id: string; de // executeAgentMaintenanceActions) needs the same effective required contexts this accept-time re-check // (above) evaluated against. Re-fetch so branch-protection changes remain authoritative at accept time. requiredCiContexts: executionRequiredContexts, + advisoryCheckRuns: settings.advisoryCheckRuns, // #4372: same exclusion the plan used, for step-8 re-verify // #3472 split-brain: a staged approve/merge can sit queued long enough for a SIBLING pass to publish a // manual-review hold on this same PR/head before the maintainer accepts — the executor's own live guard // (step 7b of executeAgentMaintenanceActions) needs the configured label to check for. diff --git a/src/settings/agent-actions.ts b/src/settings/agent-actions.ts index 1026772a8c..2604301475 100644 --- a/src/settings/agent-actions.ts +++ b/src/settings/agent-actions.ts @@ -356,6 +356,13 @@ export type AgentActionPlanInput = { // emitted migration-collision label so the contributor knows why. Never causes a CLOSE — only // ever downgrades a would-merge into a held-for-review state, same risk profile as the guardrail hold. migrationCollisionHold?: { reason: string; comment: string } | undefined; + // Advisory check-run hold (#4372). A maintainer-declared `gate.advisoryCheckRuns` entry resolved COMPLETED to a + // non-passing conclusion. The check was excluded from CI pass/fail entirely (it never blocks or freezes the + // gate), but it must not be silently swallowed either: same risk profile as migrationCollisionHold — SUPPRESSES + // the merge (folded into `heldForManualReview`), never causes a CLOSE, only downgrades a would-merge into a + // held-for-review state so a maintainer can act on the signal their installed app raised. Each entry names the + // triggering check/app/conclusion so the hold reason (and the manual-review label's comment) is actionable. + advisoryCheckHold?: ReadonlyArray<{ name: string; appSlug: string; conclusion: string }> | undefined; // Unlinked-issue guardrail (#unlinked-issue-guardrail, credibility-gate-farming defense). The trigger // (runAgentMaintenancePlanAndExecute) has already run the deterministic pre-filter + AI verification for a // PR that links NO issue -- this input is already the resolved "yes, hold this merge" verdict (or absent, @@ -586,6 +593,18 @@ function screenshotTableCloseMessage(reason: string): string { return `${reason} This is an automated maintenance action.`; } +// #4372: name the advisory check-run(s) that forced a manual-review hold, for the label reason (audit) and the +// public comment. No vendor name is hardcoded — the values come from the maintainer's own `gate.advisoryCheckRuns`. +function advisoryHoldReason(holds: ReadonlyArray<{ name: string; appSlug: string; conclusion: string }>): string { + const parts = holds.map((h) => `"${h.name}" (${h.appSlug}) concluded ${h.conclusion}`); + return `advisory check-run held for manual review: ${parts.join("; ")}`; +} + +function advisoryHoldComment(holds: ReadonlyArray<{ name: string; appSlug: string; conclusion: string }>): string { + const parts = holds.map((h) => `\`${h.name}\` (from \`${h.appSlug}\`) concluded \`${h.conclusion}\``); + return `Held for manual review: a maintainer-configured advisory check-run reported a non-passing result — ${parts.join("; ")}. This does not block CI, but a maintainer should review it. This is an automated maintenance action.`; +} + /** * Plan best-effort assignment of the PR's opening contributor (#3182), independent of merge/close/CI outcome. * MUST run before the CI-pending settle-before-decide return below (#assign-before-ci-pending) — a PR that has @@ -813,6 +832,7 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne guardrailHit || input.migrationCollisionHold !== undefined || input.unlinkedIssueMatchHold !== undefined || + (input.advisoryCheckHold !== undefined && input.advisoryCheckHold.length > 0) || (input.unlinkedIssueMatchClose !== undefined && !acting("close")); const labels = resolveAgentDispositionLabels(input); // Canonical (reviewbot non-content-gate) policy, tuned to the operator's minimize-manual goal: merge-or-close @@ -1002,9 +1022,11 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne ? `verdict=${conclusion}; ${input.unlinkedIssueMatchHold.reason}` : input.unlinkedIssueMatchClose !== undefined ? `verdict=${conclusion}; ${input.unlinkedIssueMatchClose.reason}` - : heldForManualReview - ? `verdict=${conclusion}; ${guardrailReason}` - : `verdict=${conclusion}; CI green`; + : input.advisoryCheckHold !== undefined && input.advisoryCheckHold.length > 0 + ? `verdict=${conclusion}; ${advisoryHoldReason(input.advisoryCheckHold)}` + : heldForManualReview + ? `verdict=${conclusion}; ${guardrailReason}` + : `verdict=${conclusion}; CI green`; if (label !== null && !hasLabelOrPlanned(input.pr.labels, actions, label)) { actions.push({ actionClass: "label", @@ -1023,7 +1045,9 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne ? { comment: sanitizePublicComment(input.unlinkedIssueMatchHold.comment) } : !linkedIssueCloseInFlight && !unlinkedIssueMatchViolated && reviewGood && input.unlinkedIssueMatchClose !== undefined ? { comment: sanitizePublicComment(input.unlinkedIssueMatchClose.comment) } - : {}), + : !linkedIssueCloseInFlight && !unlinkedIssueMatchViolated && reviewGood && input.advisoryCheckHold !== undefined && input.advisoryCheckHold.length > 0 + ? { comment: sanitizePublicComment(advisoryHoldComment(input.advisoryCheckHold)) } + : {}), }); } // Stale disposition-label cleanup (#stale-disposition-label-cleanup): the review-state labels below diff --git a/src/signals/focus-manifest.ts b/src/signals/focus-manifest.ts index 46d4a6523e..ec01850cc9 100644 --- a/src/signals/focus-manifest.ts +++ b/src/signals/focus-manifest.ts @@ -502,6 +502,7 @@ function applyGateConfigOverrides(effective: RepositorySettings, gate: FocusMani if (gate.claCheckRunName !== null) effective.claCheckRunName = gate.claCheckRunName; if (gate.claCheckRunAppSlug !== null) effective.claCheckRunAppSlug = gate.claCheckRunAppSlug; if (gate.expectedCiContexts !== null) effective.expectedCiContexts = gate.expectedCiContexts; + if (gate.advisoryCheckRuns !== null) effective.advisoryCheckRuns = gate.advisoryCheckRuns; if (gate.copycatMode !== null) effective.copycatGateMode = gate.copycatMode; if (gate.copycatMinScore !== null) effective.copycatGateMinScore = gate.copycatMinScore; } diff --git a/src/types.ts b/src/types.ts index 8b5ab18b5c..be3f73bae9 100644 --- a/src/types.ts +++ b/src/types.ts @@ -802,6 +802,16 @@ export type RepositorySettings = { * ⇒ verified passed (no `ciCompletenessWarning`). Config-as-code only — no DB column; set via * `.loopover.yml gate.expectedCiContexts`. */ expectedCiContexts?: ReadonlyArray | null | undefined; + /** `gate.advisoryCheckRuns` (#4372): third-party check-runs to treat as advisory — each `{ name, appSlug }` + * matched by name and trusted only when produced by that app slug (spoof-resistant, mirroring + * {@link claCheckRunName}/{@link claCheckRunAppSlug}). A matched, COMPLETED run is excluded from the live-CI + * aggregate entirely (never gates pass/fail, never counts as "still running"), fixing the permanent hold a + * durable non-standard conclusion (e.g. a scanner's terminal `action_required`) would otherwise cause; a + * non-passing conclusion (not `success`/`neutral`/`skipped`) instead routes the PR to the manual-review hold + * with the triggering check surfaced. Generic and config-only — no vendor name is hardcoded in behavior; + * `null`/absent/empty ⇒ byte-identical to today for every repo that doesn't opt in. Config-as-code only — + * no DB column; set via `.loopover.yml gate.advisoryCheckRuns`. */ + advisoryCheckRuns?: ReadonlyArray<{ name: string; appSlug: string }> | null | undefined; /** Dry-run disposition (#gate-dryrun). When true, the gate renders the would-be merge/close/manual verdict (every * advisory sub-gate promoted to block) WITHOUT enforcing — the posted check stays non-blocking. Lets advisory mode * preview exactly what it would do before the maintainer flips to real enforcement. Default off. diff --git a/test/unit/agent-action-executor.test.ts b/test/unit/agent-action-executor.test.ts index b27f6c315b..7be7add609 100644 --- a/test/unit/agent-action-executor.test.ts +++ b/test/unit/agent-action-executor.test.ts @@ -42,7 +42,7 @@ vi.mock("../../src/github/app", async (importOriginal) => ({ // named winning sibling is still open, i.e. the duplicate justification still holds) for the same reason. vi.mock("../../src/github/backfill", async (importOriginal) => ({ ...(await importOriginal()), - fetchLiveCiAggregate: vi.fn(async () => ({ ciState: "passed" as const, hasPending: false, hasVisiblePending: false, hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], ciCompletenessWarning: null })), + fetchLiveCiAggregate: vi.fn(async () => ({ ciState: "passed" as const, hasPending: false, hasVisiblePending: false, hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], advisoryHoldDetails: [], ciCompletenessWarning: null })), fetchLivePullRequestMergeState: vi.fn(async () => "dirty" as const), fetchLiveReviewThreadBlockers: vi.fn(async () => [{ title: "still unresolved", scannerFinding: false }]), fetchLivePullRequestState: vi.fn(async () => "open" as const), @@ -445,7 +445,7 @@ describe("executeAgentMaintenanceActions (#778 gate stack)", () => { it("LIVE heuristic close is denied when live CI has since turned green (#2128)", async () => { const env = createTestEnv({}); const heuristicClose: PlannedAgentAction = { actionClass: "close", requiresApproval: false, reason: "CI failed", closeComment: "closing", closeKind: "heuristic", closeRequiresCiState: "failed" }; - vi.mocked(fetchLiveCiAggregate).mockResolvedValueOnce({ ciState: "passed", hasPending: false, hasVisiblePending: false, hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], ciCompletenessWarning: null }); + vi.mocked(fetchLiveCiAggregate).mockResolvedValueOnce({ ciState: "passed", hasPending: false, hasVisiblePending: false, hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], advisoryHoldDetails: [], ciCompletenessWarning: null }); const outcomes = await executeAgentMaintenanceActions(env, ctx(), [heuristicClose]); expect(outcomes[0]?.outcome).toBe("denied"); expect(outcomes[0]?.detail).toContain("CI state changed since planning (now: passed)"); @@ -455,7 +455,7 @@ describe("executeAgentMaintenanceActions (#778 gate stack)", () => { it("LIVE heuristic close proceeds when live CI is still failing (#2128)", async () => { const env = createTestEnv({}); const heuristicClose: PlannedAgentAction = { actionClass: "close", requiresApproval: false, reason: "CI failed", closeComment: "closing", closeKind: "heuristic", closeRequiresCiState: "failed" }; - vi.mocked(fetchLiveCiAggregate).mockResolvedValueOnce({ ciState: "failed", hasPending: false, hasVisiblePending: false, hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], ciCompletenessWarning: null }); + vi.mocked(fetchLiveCiAggregate).mockResolvedValueOnce({ ciState: "failed", hasPending: false, hasVisiblePending: false, hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], advisoryHoldDetails: [], ciCompletenessWarning: null }); const outcomes = await executeAgentMaintenanceActions(env, ctx(), [heuristicClose]); expect(outcomes[0]?.outcome).toBe("completed"); expect(closePullRequest).toHaveBeenCalledWith(env, 123, "owner/repo", 7); @@ -471,7 +471,7 @@ describe("executeAgentMaintenanceActions (#778 gate stack)", () => { const replayed = pendingActionToPlanned({ actionClass: "close", params: persisted, reason: heuristicClose.reason }); expect(replayed.closeKind).toBe("heuristic"); expect(replayed.closeRequiresCiState).toBe("failed"); - vi.mocked(fetchLiveCiAggregate).mockResolvedValueOnce({ ciState: "passed", hasPending: false, hasVisiblePending: false, hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], ciCompletenessWarning: null }); + vi.mocked(fetchLiveCiAggregate).mockResolvedValueOnce({ ciState: "passed", hasPending: false, hasVisiblePending: false, hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], advisoryHoldDetails: [], ciCompletenessWarning: null }); const outcomes = await executeAgentMaintenanceActions(env, ctx(), [replayed]); expect(outcomes[0]?.outcome).toBe("denied"); expect(outcomes[0]?.detail).toContain("CI state changed since planning (now: passed)"); @@ -509,7 +509,7 @@ describe("executeAgentMaintenanceActions (#778 gate stack)", () => { // could execute after CI recovers. const env = createTestEnv({}); const legacyHeuristicClose: PlannedAgentAction = { actionClass: "close", requiresApproval: false, reason: "CI failed", closeComment: "closing", closeKind: "heuristic" }; - vi.mocked(fetchLiveCiAggregate).mockResolvedValueOnce({ ciState: "passed", hasPending: false, hasVisiblePending: false, hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], ciCompletenessWarning: null }); + vi.mocked(fetchLiveCiAggregate).mockResolvedValueOnce({ ciState: "passed", hasPending: false, hasVisiblePending: false, hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], advisoryHoldDetails: [], ciCompletenessWarning: null }); const outcomes = await executeAgentMaintenanceActions(env, ctx(), [legacyHeuristicClose]); expect(outcomes[0]?.outcome).toBe("denied"); expect(outcomes[0]?.detail).toContain("CI state changed since planning (now: passed)"); @@ -519,7 +519,7 @@ describe("executeAgentMaintenanceActions (#778 gate stack)", () => { it("a LEGACY heuristic close (closeRequiresCiState absent) still proceeds when live CI is genuinely still failing, matching the old pre-#2478 behavior", async () => { const env = createTestEnv({}); const legacyHeuristicClose: PlannedAgentAction = { actionClass: "close", requiresApproval: false, reason: "CI failed", closeComment: "closing", closeKind: "heuristic" }; - vi.mocked(fetchLiveCiAggregate).mockResolvedValueOnce({ ciState: "failed", hasPending: false, hasVisiblePending: false, hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], ciCompletenessWarning: null }); + vi.mocked(fetchLiveCiAggregate).mockResolvedValueOnce({ ciState: "failed", hasPending: false, hasVisiblePending: false, hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], advisoryHoldDetails: [], ciCompletenessWarning: null }); const outcomes = await executeAgentMaintenanceActions(env, ctx(), [legacyHeuristicClose]); expect(outcomes[0]?.outcome).toBe("completed"); expect(closePullRequest).toHaveBeenCalledWith(env, 123, "owner/repo", 7); @@ -765,7 +765,7 @@ describe("executeAgentMaintenanceActions (#778 gate stack)", () => { it("LIVE merge is denied when live CI has since turned failing (#2128)", async () => { const env = createTestEnv({}); - vi.mocked(fetchLiveCiAggregate).mockResolvedValueOnce({ ciState: "failed", hasPending: false, hasVisiblePending: false, hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], ciCompletenessWarning: null }); + vi.mocked(fetchLiveCiAggregate).mockResolvedValueOnce({ ciState: "failed", hasPending: false, hasVisiblePending: false, hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], advisoryHoldDetails: [], ciCompletenessWarning: null }); const outcomes = await executeAgentMaintenanceActions(env, ctx({ installationId: 127 }), [merge]); expect(outcomes[0]?.outcome).toBe("denied"); expect(outcomes[0]?.detail).toContain("live CI is no longer passing (now: failed)"); @@ -774,7 +774,7 @@ describe("executeAgentMaintenanceActions (#778 gate stack)", () => { it("REGRESSION (#2364): LIVE merge is denied when live CI has since become pending, not just failed", async () => { const env = createTestEnv({}); - vi.mocked(fetchLiveCiAggregate).mockResolvedValueOnce({ ciState: "pending", hasPending: true, hasVisiblePending: true, hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], ciCompletenessWarning: null }); + vi.mocked(fetchLiveCiAggregate).mockResolvedValueOnce({ ciState: "pending", hasPending: true, hasVisiblePending: true, hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], advisoryHoldDetails: [], ciCompletenessWarning: null }); const outcomes = await executeAgentMaintenanceActions(env, ctx(), [merge]); expect(outcomes[0]?.outcome).toBe("denied"); expect(outcomes[0]?.detail).toContain("live CI is no longer passing (now: pending)"); @@ -783,7 +783,7 @@ describe("executeAgentMaintenanceActions (#778 gate stack)", () => { it("REGRESSION (#2364): LIVE merge is denied when live CI has since become unverified (unreadable), not just failed", async () => { const env = createTestEnv({}); - vi.mocked(fetchLiveCiAggregate).mockResolvedValueOnce({ ciState: "unverified", hasPending: false, hasVisiblePending: false, hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], ciCompletenessWarning: null }); + vi.mocked(fetchLiveCiAggregate).mockResolvedValueOnce({ ciState: "unverified", hasPending: false, hasVisiblePending: false, hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], advisoryHoldDetails: [], ciCompletenessWarning: null }); const outcomes = await executeAgentMaintenanceActions(env, ctx(), [merge]); expect(outcomes[0]?.outcome).toBe("denied"); expect(outcomes[0]?.detail).toContain("live CI is no longer passing (now: unverified)"); @@ -797,14 +797,14 @@ describe("executeAgentMaintenanceActions (#778 gate stack)", () => { const env = createTestEnv({}); const outcomes = await executeAgentMaintenanceActions(env, ctx({ requiredCiContexts: new Set(["build", "test"]) }), [merge]); expect(outcomes[0]?.outcome).toBe("completed"); - expect(fetchLiveCiAggregate).toHaveBeenCalledWith(env, "owner/repo", "sha7", expect.any(String), new Set(["build", "test"]), expect.any(String)); + expect(fetchLiveCiAggregate).toHaveBeenCalledWith(env, "owner/repo", "sha7", expect.any(String), new Set(["build", "test"]), expect.any(String), null); }); it("passes null (fold-all) requiredContexts when ctx.requiredCiContexts is unset — unchanged pre-existing behavior", async () => { const env = createTestEnv({}); const outcomes = await executeAgentMaintenanceActions(env, ctx(), [merge]); expect(outcomes[0]?.outcome).toBe("completed"); - expect(fetchLiveCiAggregate).toHaveBeenCalledWith(env, "owner/repo", "sha7", expect.any(String), null, expect.any(String)); + expect(fetchLiveCiAggregate).toHaveBeenCalledWith(env, "owner/repo", "sha7", expect.any(String), null, expect.any(String), null); }); it("the live CI re-check fails open on a token-mint error — it is defense-in-depth, not the primary gate (#2128)", async () => { diff --git a/test/unit/agent-actions.test.ts b/test/unit/agent-actions.test.ts index e0d1091342..efb261589c 100644 --- a/test/unit/agent-actions.test.ts +++ b/test/unit/agent-actions.test.ts @@ -517,6 +517,33 @@ describe("planAgentMaintenanceActions (#778)", () => { }); }); + describe("advisory check-run hold: a maintainer-declared gate.advisoryCheckRuns entry concluded non-passing (#4372)", () => { + // Synthetic name/app — never a real vendor — so the suite hardcodes no third party. + const advisoryHold = { advisoryCheckHold: [{ name: "Third-Party Scan", appSlug: "example-scanner", conclusion: "action_required" }] }; + + it("does NOT auto-merge (and never closes) a clean+approved+passing PR when an advisory check-run hold is present", () => { + const plan = classes(planAgentMaintenanceActions(input({ conclusion: "success", autonomy: { merge: "auto", close: "auto" }, ...advisoryHold, pr: { labels: [], mergeableState: "clean", reviewDecision: "APPROVED" } }))); + expect(plan).not.toContain("merge"); + expect(plan).not.toContain("close"); // advisory hold only downgrades a would-merge into a hold, never closes + }); + + it("labels the PR manual-review with a reason + comment naming the triggering check/app/conclusion", () => { + const plan = planAgentMaintenanceActions(input({ conclusion: "success", autonomy: { review_state_label: "auto", merge: "auto" }, manualReviewLabel: "human-review", ...advisoryHold, pr: { labels: [], mergeableState: "clean", reviewDecision: "APPROVED" } })); + const label = plan.find((a) => a.actionClass === "label"); + expect(label?.label).toBe("human-review"); + expect(label?.reason).toContain("Third-Party Scan"); + expect(label?.reason).toContain("example-scanner"); + expect(label?.reason).toContain("action_required"); + expect(label?.comment).toContain("Third-Party Scan"); + expect(classes(plan)).not.toContain("merge"); + }); + + it("an EMPTY advisoryCheckHold array does not hold — a clean+approved+passing PR still auto-merges", () => { + const plan = classes(planAgentMaintenanceActions(input({ conclusion: "success", autonomy: { merge: "auto" }, advisoryCheckHold: [], pr: { labels: [], mergeableState: "clean", reviewDecision: "APPROVED" } }))); + expect(plan).toContain("merge"); + }); + }); + describe("live migration-collision hold: a live premerge recheck found a same-numbered sibling on the base branch (#2550)", () => { const collided = { migrationCollisionHold: { reason: "live migrations/** collision on main (0090: 0090_a.sql, 0090_b.sql)", comment: "LoopOver: a live check found a migration-number collision. Please rebase." } }; diff --git a/test/unit/agent-approval-queue.test.ts b/test/unit/agent-approval-queue.test.ts index 138c4dc3a6..f89bfc1395 100644 --- a/test/unit/agent-approval-queue.test.ts +++ b/test/unit/agent-approval-queue.test.ts @@ -31,7 +31,7 @@ vi.mock("../../src/github/app", async (importOriginal) => ({ // override these to exercise the staleness-supersede / staleness-denial paths. vi.mock("../../src/github/backfill", async (importOriginal) => ({ ...(await importOriginal()), - fetchLiveCiAggregate: vi.fn(async () => ({ ciState: "passed" as const, hasPending: false, hasVisiblePending: false, hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], ciCompletenessWarning: null })), + fetchLiveCiAggregate: vi.fn(async () => ({ ciState: "passed" as const, hasPending: false, hasVisiblePending: false, hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], advisoryHoldDetails: [], ciCompletenessWarning: null })), fetchRequiredStatusContexts: vi.fn(async () => null), fetchLivePullRequestMergeState: vi.fn(async () => "clean"), fetchLivePullRequestReviewDecision: vi.fn(async () => undefined), @@ -521,7 +521,7 @@ describe("agent approval queue (#779)", () => { await seedInstallation(env); await upsertPullRequestFromGitHub(env, "owner/repo", { number: 7, title: "PR", state: "open", user: { login: "contributor" }, head: { sha: "h7" }, labels: [], body: "x" }); const { action } = await createPendingAgentActionIfAbsent(env, { repoFullName: "owner/repo", pullNumber: 7, installationId: 5, actionClass: "merge", autonomyLevel: "auto_with_approval", params: { mergeMethod: "squash", expectedHeadSha: "h7" }, reason: "clean" }); - vi.mocked(fetchLiveCiAggregate).mockResolvedValueOnce({ ciState: "failed", hasPending: false, hasVisiblePending: false, hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], ciCompletenessWarning: null }); + vi.mocked(fetchLiveCiAggregate).mockResolvedValueOnce({ ciState: "failed", hasPending: false, hasVisiblePending: false, hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], advisoryHoldDetails: [], ciCompletenessWarning: null }); // Also exercise a best-effort-failed mergeable/review read (undefined) alongside the CI failure — the // audit metadata's nullish fallback must not throw, and ciState alone is still sufficient to deny. vi.mocked(fetchLivePullRequestMergeState).mockResolvedValueOnce(undefined); @@ -552,7 +552,7 @@ describe("agent approval queue (#779)", () => { expect(result.status).toBe("accepted"); expect(fetchRequiredStatusContexts).toHaveBeenCalledWith(env, "owner/repo", null, expect.any(String), expect.any(String)); - expect(fetchLiveCiAggregate).toHaveBeenCalledWith(env, "owner/repo", "h7", expect.any(String), new Set(["build", "test"]), expect.any(String)); + expect(fetchLiveCiAggregate).toHaveBeenCalledWith(env, "owner/repo", "h7", expect.any(String), new Set(["build", "test"]), expect.any(String), undefined); }); it("unions branch-protection contexts into the accept-time live CI re-check", async () => { @@ -568,7 +568,7 @@ describe("agent approval queue (#779)", () => { expect(result.status).toBe("accepted"); expect(fetchRequiredStatusContexts).toHaveBeenCalledWith(env, "owner/repo", "main", expect.any(String), expect.any(String)); - expect(fetchLiveCiAggregate).toHaveBeenCalledWith(env, "owner/repo", "h7", expect.any(String), new Set(["branch-required", "build"]), expect.any(String)); + expect(fetchLiveCiAggregate).toHaveBeenCalledWith(env, "owner/repo", "h7", expect.any(String), new Set(["branch-required", "build"]), expect.any(String), undefined); }); it("falls back to expectedCiContexts when the accept-time branch-protection read fails", async () => { @@ -583,7 +583,7 @@ describe("agent approval queue (#779)", () => { const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" }); expect(result.status).toBe("accepted"); - expect(fetchLiveCiAggregate).toHaveBeenCalledWith(env, "owner/repo", "h7", expect.any(String), new Set(["build"]), expect.any(String)); + expect(fetchLiveCiAggregate).toHaveBeenCalledWith(env, "owner/repo", "h7", expect.any(String), new Set(["build"]), expect.any(String), null); }); it("accept supersedes a staged merge when live CI has since turned pending, not just failed (#2126)", async () => { @@ -594,7 +594,7 @@ describe("agent approval queue (#779)", () => { const { action } = await createPendingAgentActionIfAbsent(env, { repoFullName: "owner/repo", pullNumber: 7, installationId: 5, actionClass: "merge", autonomyLevel: "auto_with_approval", params: { mergeMethod: "squash", expectedHeadSha: "h7" }, reason: "clean" }); // A FULFILLED "pending" read is a genuine non-passing signal — distinct from a REJECTED read (fail-open, // covered by the "ITSELF rejects" test below), which must NOT supersede. - vi.mocked(fetchLiveCiAggregate).mockResolvedValueOnce({ ciState: "pending", hasPending: true, hasVisiblePending: true, hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], ciCompletenessWarning: null }); + vi.mocked(fetchLiveCiAggregate).mockResolvedValueOnce({ ciState: "pending", hasPending: true, hasVisiblePending: true, hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], advisoryHoldDetails: [], ciCompletenessWarning: null }); const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" }); expect(result.status).toBe("rejected"); @@ -785,7 +785,7 @@ describe("agent approval queue (#779)", () => { await upsertPullRequestFromGitHub(env, "owner/repo", { number: 7, title: "PR", state: "open", user: { login: "contributor" }, head: { sha: "h7" }, labels: [], body: "x" }); const { action } = await createPendingAgentActionIfAbsent(env, { repoFullName: "owner/repo", pullNumber: 7, installationId: 5, actionClass: "merge", autonomyLevel: "auto_with_approval", params: { mergeMethod: "squash", expectedHeadSha: "h7" }, reason: "clean" }); vi.mocked(fetchLivePullRequestMergeState).mockRejectedValueOnce(new Error("GitHub API transient 502")); - vi.mocked(fetchLiveCiAggregate).mockResolvedValueOnce({ ciState: "failed", hasPending: false, hasVisiblePending: false, hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], ciCompletenessWarning: null }); + vi.mocked(fetchLiveCiAggregate).mockResolvedValueOnce({ ciState: "failed", hasPending: false, hasVisiblePending: false, hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], advisoryHoldDetails: [], ciCompletenessWarning: null }); const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" }); expect(result.status).toBe("rejected"); @@ -891,6 +891,7 @@ describe("agent approval queue (#779)", () => { hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], + advisoryHoldDetails: [], ciCompletenessWarning: null, }); const { action } = await createPendingAgentActionIfAbsent(env, { diff --git a/test/unit/backfill-2.test.ts b/test/unit/backfill-2.test.ts index 9b7b638d25..42d2df724b 100644 --- a/test/unit/backfill-2.test.ts +++ b/test/unit/backfill-2.test.ts @@ -198,7 +198,7 @@ describe("GitHub backfill", () => { const aggregate = await fetchLiveCiAggregate(env, "JSONbored/gittensory", null, "public-token", null); - expect(aggregate).toEqual({ ciState: "unverified", hasPending: false, hasVisiblePending: false, hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], ciCompletenessWarning: null }); + expect(aggregate).toEqual({ ciState: "unverified", hasPending: false, hasVisiblePending: false, hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], advisoryHoldDetails: [], ciCompletenessWarning: null }); expect(fetchSpy).not.toHaveBeenCalled(); }); @@ -367,6 +367,117 @@ describe("GitHub backfill", () => { ]); }); + // #4372: maintainer-declared advisory check-runs. Uses SYNTHETIC names ("Third-Party Scan" / "example-scanner"), + // never a real vendor, so the suite itself hardcodes no third party. + const ADVISORY_CONFIG = [{ name: "Third-Party Scan", appSlug: "example-scanner" }]; + const stubChecks = (runs: Array>) => + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url.includes("/check-runs?")) return Response.json({ check_runs: runs }); + if (url.includes("/status?")) return Response.json({ statuses: [] }); + if (url.includes("/check-suites?")) return Response.json({ check_suites: [{ status: "completed", app: { slug: "github-actions" } }] }); + return new Response("not found", { status: 404 }); + }); + + it("#4372: a configured advisory check-run that is COMPLETED with a non-passing conclusion is excluded from CI pass/fail (never stalls the gate) and surfaced in advisoryHoldDetails", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + // The advisory check IS listed as a required branch-protection context here — proving that even a REQUIRED + // durable action_required no longer fails/stalls the gate once declared advisory (the exact #4372 bug). + stubChecks([ + { name: "validate", status: "completed", conclusion: "success", app: { slug: "github-actions" } }, + { name: "Third-Party Scan", status: "completed", conclusion: "action_required", app: { slug: "example-scanner" } }, + ]); + const aggregate = await fetchLiveCiAggregate(env, "acme/widget", "sha1", "public-token", new Set(["validate", "Third-Party Scan"]), undefined, ADVISORY_CONFIG); + expect(aggregate.ciState).toBe("passed"); // excluded → not failed, not pending + expect(aggregate.hasPending).toBe(false); + expect(aggregate.failingDetails).toEqual([]); + expect(aggregate.nonRequiredFailingDetails).toEqual([]); + expect(aggregate.advisoryHoldDetails).toEqual([{ name: "Third-Party Scan", appSlug: "example-scanner", conclusion: "action_required" }]); + }); + + it("#4372: a configured advisory check-run that COMPLETED with a passing conclusion is excluded but raises NO hold", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + stubChecks([ + { name: "validate", status: "completed", conclusion: "success", app: { slug: "github-actions" } }, + { name: "Third-Party Scan", status: "completed", conclusion: "neutral", app: { slug: "example-scanner" } }, + ]); + const aggregate = await fetchLiveCiAggregate(env, "acme/widget", "sha2", "public-token", new Set(["validate"]), undefined, ADVISORY_CONFIG); + expect(aggregate.ciState).toBe("passed"); + expect(aggregate.advisoryHoldDetails).toEqual([]); + }); + + it("#4372: a configured advisory check-run still IN PROGRESS is excluded — it never counts as 'still running' and raises no hold yet", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + stubChecks([ + { name: "validate", status: "completed", conclusion: "success", app: { slug: "github-actions" } }, + { name: "Third-Party Scan", status: "in_progress", conclusion: null, app: { slug: "example-scanner" } }, + ]); + const aggregate = await fetchLiveCiAggregate(env, "acme/widget", "sha3", "public-token", new Set(["validate"]), undefined, ADVISORY_CONFIG); + expect(aggregate.ciState).toBe("passed"); // not "pending" despite the in-progress advisory check + expect(aggregate.hasPending).toBe(false); + expect(aggregate.advisoryHoldDetails).toEqual([]); + }); + + it("#4372: a check whose NAME matches but is produced by a DIFFERENT app slug is NOT treated as advisory (spoof-resistant) — it falls through to normal handling", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + stubChecks([ + { name: "validate", status: "completed", conclusion: "success", app: { slug: "github-actions" } }, + // Same name, WRONG app → not advisory. A completed red conclusion from a non-github-actions app on a + // required context is a normal failure, so this proves the mismatch path does not silently exclude it. + { name: "Third-Party Scan", status: "completed", conclusion: "failure", app: { slug: "impostor-app" } }, + ]); + const aggregate = await fetchLiveCiAggregate(env, "acme/widget", "sha4", "public-token", new Set(["validate", "Third-Party Scan"]), undefined, ADVISORY_CONFIG); + expect(aggregate.ciState).toBe("failed"); + expect(aggregate.failingDetails).toEqual([{ name: "Third-Party Scan" }]); + expect(aggregate.advisoryHoldDetails).toEqual([]); + }); + + it("#4372: with NO advisoryCheckRuns configured, behavior is byte-identical to today (the check is not excluded)", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + stubChecks([ + { name: "validate", status: "completed", conclusion: "success", app: { slug: "github-actions" } }, + { name: "Third-Party Scan", status: "completed", conclusion: "failure", app: { slug: "example-scanner" } }, + ]); + // No 7th arg → advisoryCheckRuns undefined → matcher short-circuits, the check fails the gate as before. + const aggregate = await fetchLiveCiAggregate(env, "acme/widget", "sha5", "public-token", new Set(["validate", "Third-Party Scan"])); + expect(aggregate.ciState).toBe("failed"); + expect(aggregate.advisoryHoldDetails).toEqual([]); + }); + + it("#4372: a run whose NAME matches but carries NO producing app slug is NOT treated as advisory (a name-only match is spoofable)", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + stubChecks([ + { name: "validate", status: "completed", conclusion: "success", app: { slug: "github-actions" } }, + // Same name as the configured advisory entry, but no `app` → no trusted slug → not advisory → normal failure. + { name: "Third-Party Scan", status: "completed", conclusion: "failure" }, + ]); + const aggregate = await fetchLiveCiAggregate(env, "acme/widget", "sha6", "public-token", new Set(["validate", "Third-Party Scan"]), undefined, ADVISORY_CONFIG); + expect(aggregate.ciState).toBe("failed"); + expect(aggregate.advisoryHoldDetails).toEqual([]); + }); + + it("#4372: a matched advisory run COMPLETED with NO conclusion is excluded and raises no hold (defensive: null must not push an empty conclusion)", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + stubChecks([ + { name: "validate", status: "completed", conclusion: "success", app: { slug: "github-actions" } }, + { name: "Third-Party Scan", status: "completed", conclusion: null, app: { slug: "example-scanner" } }, + ]); + const aggregate = await fetchLiveCiAggregate(env, "acme/widget", "sha7", "public-token", new Set(["validate"]), undefined, ADVISORY_CONFIG); + expect(aggregate.ciState).toBe("passed"); // excluded, not pending + expect(aggregate.advisoryHoldDetails).toEqual([]); // empty conclusion ⇒ no hold pushed + }); + + it("#4372: a matched advisory run with NO status field is excluded and raises no hold (defensive nullish handling)", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + stubChecks([ + { name: "validate", status: "completed", conclusion: "success", app: { slug: "github-actions" } }, + { name: "Third-Party Scan", conclusion: "failure", app: { slug: "example-scanner" } }, // no `status` → "" ≠ "completed" + ]); + const aggregate = await fetchLiveCiAggregate(env, "acme/widget", "sha8", "public-token", new Set(["validate"]), undefined, ADVISORY_CONFIG); + expect(aggregate.ciState).toBe("passed"); // excluded (not "completed" ⇒ no hold, still never gates) + expect(aggregate.advisoryHoldDetails).toEqual([]); + }); + it("REGRESSION (#4812): a third-party action_required check-run on a repo with NO branch-protection required contexts configured at all is still non-blocking, not folded into failingDetails by the 'assume required when unknown' fallback", async () => { // Reproduces PR #4812 (JSONbored/metagraphed) exactly: the repo's real branch protection returns // required_status_checks.contexts: [] (confirmed via the live GitHub API) -- fetchRequiredStatusContexts diff --git a/test/unit/ci-resolution.test.ts b/test/unit/ci-resolution.test.ts index 909d3481c3..b71ac07964 100644 --- a/test/unit/ci-resolution.test.ts +++ b/test/unit/ci-resolution.test.ts @@ -28,6 +28,7 @@ describe("cachedLiveCiAggregate request-scoped memoization (#4498)", () => { hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], + advisoryHoldDetails: [], ciCompletenessWarning: null, }); const facts = emptyFacts(); @@ -41,6 +42,7 @@ describe("cachedLiveCiAggregate request-scoped memoization (#4498)", () => { baseRef: null, token: "tok", expectedCiContexts: null, + advisoryCheckRuns: null, }; const first = await cachedLiveCiAggregate(env, args); @@ -49,4 +51,31 @@ describe("cachedLiveCiAggregate request-scoped memoization (#4498)", () => { expect(second).toEqual(first); expect(liveCiSpy).toHaveBeenCalledTimes(1); }); + + it("#4372: a DIFFERENT advisoryCheckRuns config produces a DIFFERENT cache key, so the aggregate is re-fetched (a config change never serves a stale entry)", async () => { + const env = createTestEnv(); + const liveCiSpy = vi.spyOn(backfillModule, "fetchLiveCiAggregatePreferGraphQl").mockResolvedValue({ + ciState: "passed", + hasPending: false, + hasVisiblePending: false, + hasMissingRequiredContext: false, + failingDetails: [], + nonRequiredFailingDetails: [], + advisoryHoldDetails: [], + ciCompletenessWarning: null, + }); + const facts = emptyFacts(); + const base = { repoFullName: "owner/repo", facts, prNumber: 7, headSha: "abc123", baseRef: null, token: "tok", expectedCiContexts: null }; + + await cachedLiveCiAggregate(env, { ...base, advisoryCheckRuns: null }); + await cachedLiveCiAggregate(env, { ...base, advisoryCheckRuns: [{ name: "Third-Party Scan", appSlug: "example-scanner" }] }); + // Distinct advisory config ⇒ distinct key ⇒ two live fetches (not one memoized). + expect(liveCiSpy).toHaveBeenCalledTimes(2); + + // The SAME advisory config in a different order still collapses to one key (order-independent fingerprint). + const twoEntry = [{ name: "A", appSlug: "app-a" }, { name: "B", appSlug: "app-b" }]; + await cachedLiveCiAggregate(env, { ...base, advisoryCheckRuns: twoEntry }); + await cachedLiveCiAggregate(env, { ...base, advisoryCheckRuns: [...twoEntry].reverse() }); + expect(liveCiSpy).toHaveBeenCalledTimes(3); // +1 only, the reversed list reused the key + }); }); diff --git a/test/unit/focus-manifest.test.ts b/test/unit/focus-manifest.test.ts index aa11bd7b4c..0a23a85b2d 100644 --- a/test/unit/focus-manifest.test.ts +++ b/test/unit/focus-manifest.test.ts @@ -281,6 +281,7 @@ describe(".loopover.yml.example field-exhaustiveness (#1670)", () => { claCheckRunName: "checkRunName:", claCheckRunAppSlug: "checkRunAppSlug:", expectedCiContexts: "expectedCiContexts:", + advisoryCheckRuns: "advisoryCheckRuns:", aiJudgmentBlockersMode: "aiJudgmentBlockers:", copycatMode: "copycat:", copycatMinScore: "copycat:", @@ -846,7 +847,7 @@ describe("compileFocusManifestPolicy", () => { issueDiscoveryPolicy: "neutral", maintainerNotes: [], publicNotes: ["Keep PRs focused.", "Maximize your reward payout"], - gate: { present: false, enabled: null, checkMode: null, pack: null, linkedIssue: null, duplicates: null, readinessMode: null, readinessMinScore: null, slopMode: null, slopMinScore: null, slopAiAdvisory: null, sizeMode: null, lockfileIntegrityMode: null, aiReviewMode: null, aiReviewByok: null, aiReviewProvider: null, aiReviewModel: null, aiReviewAllAuthors: null, aiReviewCloseConfidence: null, aiReviewLowConfidenceDisposition: null, aiReviewCombine: null, aiReviewOnMerge: null, aiReviewReviewers: null, mergeReadiness: null, selfAuthoredLinkedIssue: null, linkedIssueSatisfaction: null, manifestPolicy: null, dryRun: null, firstTimeContributorGrace: null, premergeContentRecheck: null, requireFreshRebaseWindowMinutes: null, claMode: null, claConsentPhrase: null, claCheckRunName: null, claCheckRunAppSlug: null, expectedCiContexts: null, aiJudgmentBlockersMode: null, copycatMode: null, copycatMinScore: null }, + gate: { present: false, enabled: null, checkMode: null, pack: null, linkedIssue: null, duplicates: null, readinessMode: null, readinessMinScore: null, slopMode: null, slopMinScore: null, slopAiAdvisory: null, sizeMode: null, lockfileIntegrityMode: null, aiReviewMode: null, aiReviewByok: null, aiReviewProvider: null, aiReviewModel: null, aiReviewAllAuthors: null, aiReviewCloseConfidence: null, aiReviewLowConfidenceDisposition: null, aiReviewCombine: null, aiReviewOnMerge: null, aiReviewReviewers: null, mergeReadiness: null, selfAuthoredLinkedIssue: null, linkedIssueSatisfaction: null, manifestPolicy: null, dryRun: null, firstTimeContributorGrace: null, premergeContentRecheck: null, requireFreshRebaseWindowMinutes: null, claMode: null, claConsentPhrase: null, claCheckRunName: null, claCheckRunAppSlug: null, expectedCiContexts: null, advisoryCheckRuns: null, aiJudgmentBlockersMode: null, copycatMode: null, copycatMinScore: null }, settings: {}, review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, fixHandoff: null, autoMergeSummary: null, suggestions: null, changedFilesSummary: null, effortScore: null, impactMap: null, cultureProfile: null, selftune: null, reviewMemory: null, findingCategories: null, inlineCommentsPerCategory: null, minFindingSeverity: null, maxFindings: { blockers: null, nits: null }, commentVerbosity: null, e2eTestDelivery: null, e2eTestAutoTrigger: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG }, visual: { ...EMPTY_VISUAL_CONFIG }, linkedIssueSatisfaction: null, sharedConfigSource: null }, features: { present: false, rag: null, reputation: null, unifiedComment: null, safety: null, grounding: null, e2eTests: null, screenshots: null, improvementSignal: null }, @@ -1158,7 +1159,7 @@ describe("parseFocusManifest gate config", () => { // the block→advisory deprecation-downgrade behavior itself is covered separately below. const m = parseFocusManifest({ gate: { linkedIssue: "block", duplicates: "advisory", readiness: { mode: "advisory", minScore: 70 } } }); expect(m.present).toBe(true); - expect(m.gate).toEqual({ present: true, enabled: null, checkMode: null, pack: null, linkedIssue: "block", duplicates: "advisory", readinessMode: "advisory", readinessMinScore: 70, slopMode: null, slopMinScore: null, slopAiAdvisory: null, sizeMode: null, lockfileIntegrityMode: null, aiReviewMode: null, aiReviewByok: null, aiReviewProvider: null, aiReviewModel: null, aiReviewAllAuthors: null, aiReviewCloseConfidence: null, aiReviewLowConfidenceDisposition: null, aiReviewCombine: null, aiReviewOnMerge: null, aiReviewReviewers: null, mergeReadiness: null, selfAuthoredLinkedIssue: null, linkedIssueSatisfaction: null, manifestPolicy: null, dryRun: null, firstTimeContributorGrace: null, premergeContentRecheck: null, requireFreshRebaseWindowMinutes: null, claMode: null, claConsentPhrase: null, claCheckRunName: null, claCheckRunAppSlug: null, expectedCiContexts: null, aiJudgmentBlockersMode: null, copycatMode: null, copycatMinScore: null }); + expect(m.gate).toEqual({ present: true, enabled: null, checkMode: null, pack: null, linkedIssue: "block", duplicates: "advisory", readinessMode: "advisory", readinessMinScore: 70, slopMode: null, slopMinScore: null, slopAiAdvisory: null, sizeMode: null, lockfileIntegrityMode: null, aiReviewMode: null, aiReviewByok: null, aiReviewProvider: null, aiReviewModel: null, aiReviewAllAuthors: null, aiReviewCloseConfidence: null, aiReviewLowConfidenceDisposition: null, aiReviewCombine: null, aiReviewOnMerge: null, aiReviewReviewers: null, mergeReadiness: null, selfAuthoredLinkedIssue: null, linkedIssueSatisfaction: null, manifestPolicy: null, dryRun: null, firstTimeContributorGrace: null, premergeContentRecheck: null, requireFreshRebaseWindowMinutes: null, claMode: null, claConsentPhrase: null, claCheckRunName: null, claCheckRunAppSlug: null, expectedCiContexts: null, advisoryCheckRuns: null, aiJudgmentBlockersMode: null, copycatMode: null, copycatMinScore: null }); }); it("parses gate.mergeReadiness + gate.firstTimeContributorGrace, round-trips them, and warns on bad values (#822)", () => { @@ -1601,6 +1602,37 @@ describe("parseFocusManifest gate config", () => { expect(over.warnings.some((w) => /gate\.aiReview\.reviewers" is capped/.test(w))).toBe(true); }); + it("parses gate.advisoryCheckRuns, makes the gate present, round-trips + resolves it, caps entries, and drops entries missing name/appSlug (#4372)", () => { + const m = parseFocusManifest({ gate: { advisoryCheckRuns: [{ name: "Third-Party Scan", appSlug: "example-scanner" }, { name: "Trust Check", appSlug: "example-trust" }] } }); + expect(m.gate.present).toBe(true); + expect(m.gate.advisoryCheckRuns).toEqual([{ name: "Third-Party Scan", appSlug: "example-scanner" }, { name: "Trust Check", appSlug: "example-trust" }]); + expect(parseFocusManifest({ gate: gateConfigToJson(m.gate) }).gate).toEqual(m.gate); // round-trips + const eff = resolveEffectiveSettings({ advisoryCheckRuns: undefined } as unknown as RepositorySettings, m); + expect(eff.advisoryCheckRuns).toEqual([{ name: "Third-Party Scan", appSlug: "example-scanner" }, { name: "Trust Check", appSlug: "example-trust" }]); + // Absent ⇒ null ⇒ the DB/default value is left untouched. + const noFlag = parseFocusManifest({ gate: { claMode: "advisory" } }); + expect(noFlag.gate.advisoryCheckRuns).toBeNull(); + expect(resolveEffectiveSettings({ advisoryCheckRuns: [{ name: "Existing", appSlug: "existing-app" }] } as unknown as RepositorySettings, noFlag).advisoryCheckRuns).toEqual([{ name: "Existing", appSlug: "existing-app" }]); + // Non-array ⇒ warns, stays null. + expect(parseFocusManifest({ gate: { advisoryCheckRuns: "Third-Party Scan" } }).warnings.some((w) => /gate\.advisoryCheckRuns/.test(w))).toBe(true); + expect(parseFocusManifest({ gate: { advisoryCheckRuns: "Third-Party Scan" } }).gate.advisoryCheckRuns).toBeNull(); + // A non-mapping entry, a name-only entry (spoofable → dropped), and an appSlug-only entry are dropped; valid siblings survive. + const mixed = parseFocusManifest({ gate: { advisoryCheckRuns: [{ name: "Good", appSlug: "good-app" }, "nope", { name: "NoSlug" }, { appSlug: "no-name" }, { name: " ", appSlug: "blank" }] } }); + expect(mixed.gate.advisoryCheckRuns).toEqual([{ name: "Good", appSlug: "good-app" }]); + expect(mixed.warnings.some((w) => /gate\.advisoryCheckRuns\[1\]/.test(w))).toBe(true); + expect(mixed.warnings.some((w) => /gate\.advisoryCheckRuns\[2\]/.test(w))).toBe(true); + expect(mixed.warnings.some((w) => /gate\.advisoryCheckRuns\[3\]/.test(w))).toBe(true); + expect(mixed.warnings.some((w) => /gate\.advisoryCheckRuns\[4\]/.test(w))).toBe(true); + // All-invalid list ⇒ null (not an empty array), matching every other manifest "absent means null" contract. + expect(parseFocusManifest({ gate: { advisoryCheckRuns: [{ name: "x" }] } }).gate.advisoryCheckRuns).toBeNull(); + // Over the cap (16): only the first 16 entries survive, with a warning. + const over = parseFocusManifest({ + gate: { advisoryCheckRuns: Array.from({ length: 18 }, (_, i) => ({ name: `Check ${i}`, appSlug: `app-${i}` })) }, + }); + expect(over.gate.advisoryCheckRuns).toHaveLength(16); + expect(over.warnings.some((w) => /gate\.advisoryCheckRuns" is capped/.test(w))).toBe(true); + }); + it("parses the features: block (per-repo converged-feature toggles), round-trips it, and makes the manifest present", () => { const m = parseFocusManifest({ features: { rag: true, reputation: false, unifiedComment: true } }); expect(m.present).toBe(true); diff --git a/test/unit/mcp-automation-state.test.ts b/test/unit/mcp-automation-state.test.ts index 806512470d..08cc2b4b15 100644 --- a/test/unit/mcp-automation-state.test.ts +++ b/test/unit/mcp-automation-state.test.ts @@ -42,7 +42,7 @@ vi.mock("../../src/github/pr-freshness", async (importOriginal) => { // dedicated staleness-supersede test coverage lives in agent-approval-queue.test.ts, not this MCP-surface file. vi.mock("../../src/github/backfill", async (importOriginal) => ({ ...(await importOriginal()), - fetchLiveCiAggregate: vi.fn(async () => ({ ciState: "passed" as const, hasPending: false, hasVisiblePending: false, hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], ciCompletenessWarning: null })), + fetchLiveCiAggregate: vi.fn(async () => ({ ciState: "passed" as const, hasPending: false, hasVisiblePending: false, hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], advisoryHoldDetails: [], ciCompletenessWarning: null })), fetchLivePullRequestMergeState: vi.fn(async () => "clean"), fetchLivePullRequestReviewDecision: vi.fn(async () => undefined), })); diff --git a/test/unit/pr-detail-durable-cache.test.ts b/test/unit/pr-detail-durable-cache.test.ts index fc179a55f9..782bfb4475 100644 --- a/test/unit/pr-detail-durable-cache.test.ts +++ b/test/unit/pr-detail-durable-cache.test.ts @@ -555,6 +555,7 @@ describe("durable CI-state cache (#selfhost-ci-verification)", () => { hasMissingRequiredContext: false, failingDetails: [{ name: "ci/build", summary: "failed", detailsUrl: "https://ci.example.test/1" }], nonRequiredFailingDetails: [], + advisoryHoldDetails: [], ciCompletenessWarning: null, }; @@ -694,6 +695,7 @@ describe("durable CI-state cache (#selfhost-ci-verification)", () => { hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], + advisoryHoldDetails: [], ciCompletenessWarning: null, }); }); diff --git a/test/unit/queue-2.test.ts b/test/unit/queue-2.test.ts index bc5b720d52..6eae15069c 100644 --- a/test/unit/queue-2.test.ts +++ b/test/unit/queue-2.test.ts @@ -1900,6 +1900,7 @@ describe("queue processors", () => { hasMissingRequiredContext: true, failingDetails: [], nonRequiredFailingDetails: [], + advisoryHoldDetails: [], ciCompletenessWarning: null, }); vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { diff --git a/test/unit/queue-4.test.ts b/test/unit/queue-4.test.ts index ad8d0c6333..393f8e8efc 100644 --- a/test/unit/queue-4.test.ts +++ b/test/unit/queue-4.test.ts @@ -2710,6 +2710,7 @@ describe("queue processors", () => { hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], + advisoryHoldDetails: [], ciCompletenessWarning: null, }); vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { @@ -2897,6 +2898,7 @@ describe("queue processors", () => { hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], + advisoryHoldDetails: [], ciCompletenessWarning: null, }); vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { @@ -3079,6 +3081,7 @@ describe("queue processors", () => { hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], + advisoryHoldDetails: [], ciCompletenessWarning: null, }); vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { @@ -3239,6 +3242,7 @@ describe("queue processors", () => { hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], + advisoryHoldDetails: [], ciCompletenessWarning: null, }); vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { @@ -3359,6 +3363,7 @@ describe("queue processors", () => { hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], + advisoryHoldDetails: [], ciCompletenessWarning: null, }); vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { @@ -3506,6 +3511,7 @@ describe("queue processors", () => { hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], + advisoryHoldDetails: [], ciCompletenessWarning: null, }); vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { @@ -3653,6 +3659,7 @@ describe("queue processors", () => { hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], + advisoryHoldDetails: [], ciCompletenessWarning: null, }); vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { @@ -3825,6 +3832,7 @@ describe("queue processors", () => { hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], + advisoryHoldDetails: [], ciCompletenessWarning: null, }); vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { @@ -4006,6 +4014,7 @@ describe("queue processors", () => { hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], + advisoryHoldDetails: [], ciCompletenessWarning: null, }); vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { @@ -4350,6 +4359,7 @@ describe("queue processors", () => { hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], + advisoryHoldDetails: [], ciCompletenessWarning: null, }); vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { @@ -4514,6 +4524,7 @@ describe("queue processors", () => { hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], + advisoryHoldDetails: [], ciCompletenessWarning: null, }); vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { diff --git a/test/unit/queue-5.test.ts b/test/unit/queue-5.test.ts index 904cbbf18e..1d8acb4247 100644 --- a/test/unit/queue-5.test.ts +++ b/test/unit/queue-5.test.ts @@ -5580,6 +5580,7 @@ describe("queue processors", () => { hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], + advisoryHoldDetails: [], ciCompletenessWarning: null, }); const posted = { count: 0, body: "" }; diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index 34965f1c4d..9f535f1e9c 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -1806,6 +1806,7 @@ describe("queue processors", () => { hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], + advisoryHoldDetails: [], ciCompletenessWarning: null, }); let gateChecks = 0; @@ -1856,6 +1857,7 @@ describe("queue processors", () => { hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], + advisoryHoldDetails: [], ciCompletenessWarning: null, }); let gateChecks = 0; @@ -1883,6 +1885,7 @@ describe("queue processors", () => { expect.any(String), new Set(["trusted-required-ci"]), "installation:9001", + undefined, // #4372: advisoryCheckRuns (unconfigured here) ); expect(gateChecks).toBeGreaterThan(0); const finalized = await env.DB.prepare("select count(*) as n from audit_events where event_type = ?") @@ -1915,6 +1918,7 @@ describe("queue processors", () => { hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], + advisoryHoldDetails: [], ciCompletenessWarning: null, }); let gateChecks = 0; @@ -1991,6 +1995,7 @@ describe("queue processors", () => { hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], + advisoryHoldDetails: [], ciCompletenessWarning: null, }); let liveHeadSha = "a7"; @@ -2061,6 +2066,7 @@ describe("queue processors", () => { hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], + advisoryHoldDetails: [], ciCompletenessWarning: null, }); let gateChecks = 0; @@ -2115,6 +2121,7 @@ describe("queue processors", () => { hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], + advisoryHoldDetails: [], ciCompletenessWarning: null, }); let gateChecks = 0; @@ -2177,6 +2184,7 @@ describe("queue processors", () => { hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], + advisoryHoldDetails: [], ciCompletenessWarning: null, }); let gateChecks = 0; @@ -2230,6 +2238,7 @@ describe("queue processors", () => { hasMissingRequiredContext: true, failingDetails: [], nonRequiredFailingDetails: [], + advisoryHoldDetails: [], ciCompletenessWarning: null, }); let gateChecks = 0; @@ -2287,6 +2296,7 @@ describe("queue processors", () => { hasMissingRequiredContext: true, failingDetails: [], nonRequiredFailingDetails: [], + advisoryHoldDetails: [], ciCompletenessWarning: null, }); let gateChecks = 0; @@ -2336,6 +2346,7 @@ describe("queue processors", () => { hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], + advisoryHoldDetails: [], ciCompletenessWarning: "CI resolved to passed with no branch-protection required checks configured — cannot verify every expected workflow ran.", }); vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { @@ -2378,6 +2389,7 @@ describe("queue processors", () => { hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], + advisoryHoldDetails: [], ciCompletenessWarning: null, }); vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { @@ -2473,6 +2485,7 @@ describe("queue processors", () => { hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], + advisoryHoldDetails: [], ciCompletenessWarning: null, }); @@ -2649,6 +2662,7 @@ describe("queue processors", () => { hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], + advisoryHoldDetails: [], ciCompletenessWarning: null, }); vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { @@ -2686,6 +2700,7 @@ describe("queue processors", () => { hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], + advisoryHoldDetails: [], ciCompletenessWarning: null, }); vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { @@ -2733,6 +2748,7 @@ describe("queue processors", () => { hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], + advisoryHoldDetails: [], ciCompletenessWarning: null, }); vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { @@ -2792,6 +2808,7 @@ describe("queue processors", () => { hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], + advisoryHoldDetails: [], ciCompletenessWarning: null, }); vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { @@ -2841,6 +2858,7 @@ describe("queue processors", () => { hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], + advisoryHoldDetails: [], ciCompletenessWarning: null, }); let branchProtectionReadable = false; @@ -2878,7 +2896,8 @@ describe("queue processors", () => { await processJob(env, { type: "agent-regate-pr", deliveryId: "required-contexts-lookup-recovers", repoFullName: "owner/agent-repo", prNumber: 7, installationId: 9001 }); expect(liveCiSpy.mock.calls.length).toBeGreaterThan(liveReadsAfterFailedLookup); expect(await renderMetrics()).toContain('loopover_ci_state_cache_total{field="aggregate",result="miss"} 1'); - expect(await getPullRequestDetailSyncState(env, "owner/agent-repo", 7)).toMatchObject({ ciState: "passed", ciRequiredContextsKey: JSON.stringify(["trusted-required-ci"]) }); + // #4372: the durable cache key now folds in the advisory-check-runs fingerprint (empty "|adv:" when unconfigured). + expect(await getPullRequestDetailSyncState(env, "owner/agent-repo", 7)).toMatchObject({ ciState: "passed", ciRequiredContextsKey: `${JSON.stringify(["trusted-required-ci"])}|adv:` }); } finally { liveCiSpy.mockRestore(); } @@ -2901,6 +2920,7 @@ describe("queue processors", () => { hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], + advisoryHoldDetails: [], ciCompletenessWarning: null, }); vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { diff --git a/test/unit/routes-agent-approval.test.ts b/test/unit/routes-agent-approval.test.ts index f86471f070..2e77b20fe8 100644 --- a/test/unit/routes-agent-approval.test.ts +++ b/test/unit/routes-agent-approval.test.ts @@ -31,7 +31,7 @@ vi.mock("../../src/github/backfill", async (importOriginal) => { const actual = await importOriginal(); return { ...actual, - fetchLiveCiAggregate: vi.fn(async () => ({ ciState: "passed" as const, hasPending: false, hasVisiblePending: false, hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], ciCompletenessWarning: null })), + fetchLiveCiAggregate: vi.fn(async () => ({ ciState: "passed" as const, hasPending: false, hasVisiblePending: false, hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], advisoryHoldDetails: [], ciCompletenessWarning: null })), }; }); From 1b33341f8ef51612ad2d847e702ff9ff4138e3c8 Mon Sep 17 00:00:00 2001 From: Nick M Date: Tue, 14 Jul 2026 11:54:47 -0500 Subject: [PATCH 2/2] test(gate): pin the advisory-hold cache-invalidation guarantee end-to-end (#4372) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address the review's one flagged design tradeoff: advisoryHoldDetails is not a persisted cache column, so a durable cache HIT reconstructs it as []. Add a test pinning both halves of why that is safe — deserialize returns [] on a hit, and invalidateCiStateCache (called app-agnostically by an advisory check_run's own `completed` webhook, before re-review) clears the entry so the next disposition read misses and re-fetches live, where the hold is re-derived. Tighten the deserialize comment to cite the exact invalidation handler and covering tests, so the guarantee is verifiable from the diff rather than asserted only in prose. --- src/github/backfill.ts | 11 +++++++--- test/unit/pr-detail-durable-cache.test.ts | 25 +++++++++++++++++++++++ 2 files changed, 33 insertions(+), 3 deletions(-) diff --git a/src/github/backfill.ts b/src/github/backfill.ts index b0f8e5ae66..ce637c318e 100644 --- a/src/github/backfill.ts +++ b/src/github/backfill.ts @@ -3675,9 +3675,14 @@ export function deserializeCachedCiAggregate( nonRequiredFailingDetails, // #4372: advisoryHoldDetails is NOT persisted in the durable cache (no column) — deliberately. The // manual-review routing it drives fires on the fresh, webhook-invalidated live read that populates this - // cache entry (an advisory check_run completing invalidates the entry for that head SHA), and the hold - // label is applied idempotently there. The exclusion's effect on the disposition (a settled advisory check - // no longer holding the gate) DOES round-trip, because it is already baked into the cached `ciState`. + // cache entry: an advisory check_run's own `completed` webhook runs maybeReReviewOnCiCompletion, which calls + // invalidateCiStateCache for that PR/head-SHA (app-agnostic — it skips only the bot's OWN checks via + // isSelfAuthoredCiCompletionWebhook, never a third party's) BEFORE re-reviewing, so the disposition pass sees + // a genuine miss and re-fetches live, where reduceLiveCiAggregate re-derives the hold and the label is applied + // idempotently. A later cache HIT reconstructing [] is therefore never the path that decides a hold. The + // exclusion's effect (a settled advisory check no longer holding the gate) DOES round-trip regardless, since + // it is already baked into the cached `ciState`. Pinned end-to-end by pr-detail-durable-cache.test.ts (the + // deserialize-[] + invalidation halves) and backfill-2.test.ts (the fresh read re-deriving the hold). advisoryHoldDetails: [], ciCompletenessWarning: cached.ciCompletenessWarning ?? null, }; diff --git a/test/unit/pr-detail-durable-cache.test.ts b/test/unit/pr-detail-durable-cache.test.ts index 782bfb4475..0bcf42c7ce 100644 --- a/test/unit/pr-detail-durable-cache.test.ts +++ b/test/unit/pr-detail-durable-cache.test.ts @@ -814,5 +814,30 @@ describe("durable CI-state cache (#selfhost-ci-verification)", () => { await expect(invalidateCiStateCache(readFailEnv, "owner/repo", 93)).resolves.toBeUndefined(); expect(await getPullRequestDetailSyncState(baseEnv, "owner/repo", 93)).toMatchObject({ status: "never_synced", ciState: null }); }); + + // #4372: closes the loop on the one design tradeoff of the advisory-check-runs feature — advisoryHoldDetails is + // NOT a persisted column, so a durable cache HIT reconstructs it as []. That is safe ONLY because an advisory + // check-run's own `completed` webhook invalidates this entry (maybeReReviewOnCiCompletion → invalidateCiStateCache, + // app-agnostic: it skips only the bot's OWN checks, never a third party's), forcing the next disposition read to + // MISS → re-fetch live → reduceLiveCiAggregate re-derives the non-empty hold (covered in backfill-2.test.ts). This + // test pins both halves so the guarantee is verifiable from the diff, not just asserted in prose. + it("advisoryHoldDetails is not persisted (a cache HIT reconstructs []), and invalidation clears the entry so the next read re-fetches live and re-derives the hold", async () => { + const env = createTestEnv(); + const withHold: LiveCiAggregate = { + ...sampleAggregate, + ciState: "passed", + failingDetails: [], + advisoryHoldDetails: [{ name: "Third-Party Scan", appSlug: "example-scanner", conclusion: "action_required" }], + }; + await writeThroughCiStateCache(env, "owner/repo", 94, null, "sha1", "req|adv:x", withHold); + // A cache HIT never carries the hold — the field has no column, so deserialize reconstructs [] deliberately. + const cached = await getPullRequestDetailSyncState(env, "owner/repo", 94); + expect(deserializeCachedCiAggregate(cached!)?.advisoryHoldDetails).toEqual([]); + // ...but the ciState the exclusion produced ("passed", not stuck "pending") DOES round-trip via its column. + expect(deserializeCachedCiAggregate(cached!)?.ciState).toBe("passed"); + // The advisory check's completion webhook invalidates the entry → the next read is a genuine miss → live re-fetch. + await invalidateCiStateCache(env, "owner/repo", 94); + expect(await getPullRequestDetailSyncState(env, "owner/repo", 94)).toMatchObject({ ciState: null, ciStateFetchedAt: null }); + }); }); });