diff --git a/.loopover.yml.example b/.loopover.yml.example index 9d7ae80587..d979123fe1 100644 --- a/.loopover.yml.example +++ b/.loopover.yml.example @@ -252,6 +252,17 @@ gate: - build - test + # Third-party check-runs that must never gate CI pass/fail or count as pending, but whose non-pass + # conclusions should route an otherwise-green PR to manual review instead of merging through a flag the + # operator installed an app to raise (#4372). Each entry matches a check-run by exact name AND trusted app + # slug (same trust model as gate.cla.checkRunName/checkRunAppSlug). A completed advisory check with + # success/neutral/skipped is settled and ignored; failure/action_required/etc. holds for manual review. + # List of { name, appSlug } mappings, or omit. Default: not configured (byte-identical to today's behavior). + # Config-as-code only — no DB column or dashboard toggle. + # advisoryCheckRuns: + # - name: Example trust scan + # appSlug: example-trust-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/config/examples/loopover.full.yml b/config/examples/loopover.full.yml index 02eed60e56..cd16d23488 100644 --- a/config/examples/loopover.full.yml +++ b/config/examples/loopover.full.yml @@ -266,6 +266,17 @@ gate: - build - test + # Third-party check-runs that must never gate CI pass/fail or count as pending, but whose non-pass + # conclusions should route an otherwise-green PR to manual review instead of merging through a flag the + # operator installed an app to raise (#4372). Each entry matches a check-run by exact name AND trusted app + # slug (same trust model as gate.cla.checkRunName/checkRunAppSlug). A completed advisory check with + # success/neutral/skipped is settled and ignored; failure/action_required/etc. holds for manual review. + # List of { name, appSlug } mappings, or omit. Default: not configured (byte-identical to today's behavior). + # Config-as-code only — no DB column or dashboard toggle. + # advisoryCheckRuns: + # - name: Example trust scan + # appSlug: example-trust-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 8ba9da8c98..0a7a3312a8 100644 --- a/packages/loopover-engine/src/focus-manifest.ts +++ b/packages/loopover-engine/src/focus-manifest.ts @@ -73,6 +73,11 @@ export type FocusManifestIssueDiscoveryPolicy = "encouraged" | "neutral" | "disc * axis entirely: whether/how the "LoopOver Orb Review Agent" check-RUN publishes, independent of gate * evaluation itself (which always runs regardless of `checkMode`/`enabled`) — see {@link ReviewCheckMode}. */ +export type AdvisoryCheckRunSpec = { + name: string; + appSlug: string; +}; + export type FocusManifestGateConfig = { present: boolean; /** `gate.enabled` (legacy): a boolean shorthand for `checkMode` below -- `true` maps to `"required"`, @@ -186,6 +191,10 @@ 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 that must never gate CI pass/fail or count as + * "still running"; a completed non-pass routes to manual-review instead. null (unset) ⇒ byte-identical to + * today's behavior for repos that do not configure it. */ + advisoryCheckRuns: ReadonlyArray | 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 @@ -997,6 +1006,7 @@ const EMPTY_GATE_CONFIG: FocusManifestGateConfig = { claCheckRunName: null, claCheckRunAppSlug: null, expectedCiContexts: null, + advisoryCheckRuns: null, aiJudgmentBlockersMode: null, copycatMode: null, copycatMinScore: null, @@ -1281,6 +1291,31 @@ function normalizeOptionalReviewers( return out.length > 0 ? out : null; } +function normalizeAdvisoryCheckRunsList( + value: JsonValue | undefined, + field: string, + warnings: string[], +): ReadonlyArray | null { + if (value === undefined || value === null) return null; + if (!Array.isArray(value)) { + warnings.push(`Manifest field "${field}" must be a list; ignoring it.`); + return null; + } + const out: AdvisoryCheckRunSpec[] = []; + for (const entry of value) { + if (typeof entry !== "object" || entry === null || Array.isArray(entry)) { + warnings.push(`Manifest field "${field}" has a non-mapping entry; dropping it.`); + continue; + } + const record = entry as Record; + const name = parsePublicSafeText(record.name, `${field}[].name`, warnings); + const appSlug = parsePublicSafeText(record.appSlug, `${field}[].appSlug`, warnings); + if (name === null || appSlug === null) 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. @@ -1364,6 +1399,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: normalizeAdvisoryCheckRunsList(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), @@ -1424,6 +1460,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; @@ -1503,6 +1540,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((entry) => ({ name: entry.name, appSlug: entry.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/index.ts b/packages/loopover-engine/src/index.ts index c05cd2a1d1..33b920b26c 100644 --- a/packages/loopover-engine/src/index.ts +++ b/packages/loopover-engine/src/index.ts @@ -649,6 +649,7 @@ export { REVIEW_PROFILES, type AutoReviewConfig, type CommentVerbosity, + type AdvisoryCheckRunSpec, type ConvergedFeatureKey, type ExperimentalPluginKey, type FocusManifest, diff --git a/src/github/advisory-check-runs.ts b/src/github/advisory-check-runs.ts new file mode 100644 index 0000000000..4340c42d38 --- /dev/null +++ b/src/github/advisory-check-runs.ts @@ -0,0 +1,51 @@ +import type { AdvisoryCheckRunSpec } from "@loopover/engine"; + +/** True when `run` matches a configured `gate.advisoryCheckRuns` entry (name + app slug, case-insensitive). */ +export function matchesConfiguredAdvisoryCheckRun( + run: { name: string; app?: { slug?: string | null } | null }, + specs: ReadonlyArray | null | undefined, +): boolean { + if (!specs || specs.length === 0) return false; + const runName = run.name.trim().toLowerCase(); + const runSlug = (run.app?.slug ?? "").trim().toLowerCase(); + if (!runName || !runSlug) return false; + return specs.some( + (spec) => spec.name.trim().toLowerCase() === runName && spec.appSlug.trim().toLowerCase() === runSlug, + ); +} + +const ADVISORY_PASSING_CONCLUSIONS = new Set(["success", "neutral", "skipped"]); + +/** A completed advisory check-run with one of these conclusions is settled and needs no hold. */ +export function isAdvisoryCheckRunSettledPass(conclusion: string): boolean { + return ADVISORY_PASSING_CONCLUSIONS.has(conclusion.trim().toLowerCase()); +} + +/** Stable cache-key fragment for a repo's configured advisory check-run list (#4372). */ +export function advisoryCheckRunsKeyPart(specs: ReadonlyArray | null | undefined): string { + if (!specs || specs.length === 0) return ""; + return JSON.stringify( + [...specs] + .map((spec) => ({ name: spec.name, appSlug: spec.appSlug })) + .sort((left, right) => `${left.appSlug}/${left.name}`.localeCompare(`${right.appSlug}/${right.name}`)), + ); +} + +export function resolveAdvisoryCheckHold( + advisoryHoldDetails: ReadonlyArray<{ name: string; summary?: string; appSlug?: string }> | undefined, + advisoryCheckRuns: ReadonlyArray | null | undefined, +): { checkNames: readonly string[]; reason: string; comment: string } | undefined { + if (!advisoryCheckRuns?.length || !advisoryHoldDetails?.length) return undefined; + const checkNames = advisoryHoldDetails.map((detail) => detail.name); + const lines = advisoryHoldDetails.map((detail) => { + const app = detail.appSlug?.trim(); + const summary = detail.summary?.trim(); + const prefix = app ? `${detail.name} (${app})` : detail.name; + return summary ? `- ${prefix}: ${summary}` : `- ${prefix}`; + }); + return { + checkNames, + reason: `advisory check-run hold (${checkNames.join(", ")})`, + comment: `LoopOver: a configured advisory check-run needs maintainer action before this PR can merge:\n${lines.join("\n")}`, + }; +} diff --git a/src/github/backfill.ts b/src/github/backfill.ts index 5e612bd90e..9ebf89cf7f 100644 --- a/src/github/backfill.ts +++ b/src/github/backfill.ts @@ -83,6 +83,8 @@ import { type GitHubRateLimitAdmissionKey, } from "./client"; import { fetchCachedGitHubGraphQl } from "./graphql-cache"; +import { isAdvisoryCheckRunSettledPass, matchesConfiguredAdvisoryCheckRun } from "./advisory-check-runs.js"; +import type { AdvisoryCheckRunSpec } from "@loopover/engine"; import { incr } from "../selfhost/metrics"; import { fetchBrokeredInstallationToken, isOrbBrokerMode } from "../orb/broker-client"; type GitHubLabelPayload = { @@ -2572,6 +2574,9 @@ 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 }>; + /** Configured advisory check-runs (#4372) that completed with a non-pass conclusion — routed to manual-review, + * never folded into ciState/hasPending. Empty when the repo did not opt in via `gate.advisoryCheckRuns`. */ + advisoryHoldDetails?: Array<{ name: string; summary?: string; detailsUrl?: string; appSlug?: 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 @@ -2799,9 +2804,10 @@ async function reduceLiveCiAggregate( checkRunsIncomplete: boolean; statusIncomplete: boolean; fetchSuites: () => Promise | null>; + advisoryCheckRuns?: ReadonlyArray | null | undefined; }, ): Promise { - const { checkRuns, statuses, requiredContexts, checkRunsIncomplete, statusIncomplete, fetchSuites } = inputs; + const { checkRuns, statuses, requiredContexts, checkRunsIncomplete, statusIncomplete, fetchSuites, advisoryCheckRuns } = 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 +2825,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; @@ -2836,10 +2843,22 @@ async function reduceLiveCiAggregate( seenContextNames.add(run.name); // mark BEFORE bot-check skip: a bot-owned required context is "seen" 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 - total += 1; const conclusion = (run.conclusion ?? "").toLowerCase(); const status = (run.status ?? "").toLowerCase(); + if (isOwnGitHubAppCheckRun(env, run)) continue; // never wait on the bot's own Gate/Context check-runs + if (matchesConfiguredAdvisoryCheckRun(run, advisoryCheckRuns)) { + if (status === "completed" && conclusion && !isAdvisoryCheckRunSettledPass(conclusion)) { + const summary = checkRunSummary(run); + advisoryHoldDetails.push({ + name: run.name, + ...(summary ? { summary } : {}), + ...(run.details_url ? { detailsUrl: run.details_url } : {}), + ...(appSlug ? { appSlug } : {}), + }); + } + continue; + } + total += 1; // A THIRD-PARTY app's OWN action_required verdict on an already-COMPLETED check-run (for example, a // security/check tool asking for human review) is a settled, terminal adverse result -- but ONLY when that // check is actually a REQUIRED context. A non-required third-party check must never hard-fail/auto-close the @@ -2947,7 +2966,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 +2987,9 @@ 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, + advisoryCheckRuns?: ReadonlyArray | 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[] = []; @@ -3014,6 +3034,7 @@ export async function fetchLiveCiAggregate( requiredContexts, checkRunsIncomplete, statusIncomplete, + advisoryCheckRuns, // Lazily read the check-SUITES backstop only when the reducer finds the cheaper sources fully settled; a fetch // error returns null so the reducer fails closed exactly as the inline path did. fetchSuites: async () => { @@ -3051,6 +3072,7 @@ export async function fetchLiveCiAggregateViaGraphQl( token: string | undefined, requiredContexts?: ReadonlySet | null, admissionKey?: GitHubRateLimitAdmissionKey, + advisoryCheckRuns?: ReadonlyArray | null, ): Promise { if (!headSha || !token) return null; const [owner, name] = repoFullName.split("/"); @@ -3132,6 +3154,7 @@ export async function fetchLiveCiAggregateViaGraphQl( requiredContexts, checkRunsIncomplete: false, statusIncomplete: false, + advisoryCheckRuns, fetchSuites: async () => suites, // already fetched in the same query — never a second round-trip }); } @@ -3149,14 +3172,13 @@ export async function fetchLiveCiAggregatePreferGraphQl( token: string | undefined, requiredContexts?: ReadonlySet | null, admissionKey?: GitHubRateLimitAdmissionKey, + advisoryCheckRuns?: ReadonlyArray | 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 +3650,7 @@ export function deserializeCachedCiAggregate( hasMissingRequiredContext: cached.ciHasMissingRequiredContext ?? false, failingDetails, nonRequiredFailingDetails, + advisoryHoldDetails: [], ciCompletenessWarning: cached.ciCompletenessWarning ?? null, }; } catch { diff --git a/src/queue/ci-resolution.ts b/src/queue/ci-resolution.ts index 98eaee9b4b..228695c9e2 100644 --- a/src/queue/ci-resolution.ts +++ b/src/queue/ci-resolution.ts @@ -14,7 +14,8 @@ // fetchLiveCiAggregateWithRequiredContexts, expectedCiContextsKeyPart, resolvedRequiredContextsKeyPart, // evictLiveFactOnReject) stay unexported, matching their original (never-exported) visibility. -import { getPullRequestDetailSyncState } from "../db/repositories"; +import { advisoryCheckRunsKeyPart } from "../github/advisory-check-runs.js"; +import type { AdvisoryCheckRunSpec } from "@loopover/engine"; import { cachedFetchLivePullRequestMergeState, CI_STATE_CACHE_METRIC, @@ -64,6 +65,15 @@ function resolvedRequiredContextsKeyPart(requiredContexts: ReadonlySet | return JSON.stringify([...requiredContexts].sort()); } +function liveCiAggregateConfigKeyPart( + expectedCiContexts: ReadonlyArray | null | undefined, + advisoryCheckRuns: ReadonlyArray | null | undefined, +): string { + const advisoryPart = advisoryCheckRunsKeyPart(advisoryCheckRuns); + const contextsPart = expectedCiContextsKeyPart(expectedCiContexts); + return advisoryPart ? `${contextsPart}|advisory:${advisoryPart}` : contextsPart; +} + // 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 @@ -148,10 +158,14 @@ async function cachedFetchLiveCiAggregate( // ci-verification gate review finding). The live-fetched aggregate is still returned to THIS caller either way. requiredContextsResolved: boolean; admissionKey?: GitHubRateLimitAdmissionKey | undefined; + advisoryCheckRuns?: ReadonlyArray | null | undefined; }, ): Promise { const cached = await getPullRequestDetailSyncState(env, args.repoFullName, args.prNumber).catch(() => null); - if (!args.forceRefresh && cached && isCiStateCacheFresh(cached, args.headSha, args.requiredContextsKey)) { + // #4372: the durable cache does not persist advisoryHoldDetails yet, so repos that opt into + // gate.advisoryCheckRuns must always take the live path — otherwise a cache hit would drop the hold signal. + const skipDurableCache = Boolean(args.advisoryCheckRuns?.length); + if (!args.forceRefresh && !skipDurableCache && cached && isCiStateCacheFresh(cached, args.headSha, args.requiredContextsKey)) { const deserialized = deserializeCachedCiAggregate(cached); if (deserialized) { incr(CI_STATE_CACHE_METRIC, { field: "aggregate", result: "hit" }); @@ -159,7 +173,15 @@ 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); } @@ -178,6 +200,7 @@ function fetchLiveCiAggregateWithRequiredContexts( expectedCiContexts: ReadonlyArray | null | undefined; forceRefresh: boolean; admissionKey?: GitHubRateLimitAdmissionKey | undefined; + advisoryCheckRuns?: ReadonlyArray | null | undefined; }, ): Promise { // CI refresh callers need fresh check/status state; branch protection contexts move slowly enough to stay @@ -194,10 +217,11 @@ function fetchLiveCiAggregateWithRequiredContexts( headSha: args.headSha, token: args.token, requiredContexts, - requiredContextsKey: resolvedRequiredContextsKeyPart(requiredContexts), + requiredContextsKey: `${resolvedRequiredContextsKeyPart(requiredContexts)}|cfg:${liveCiAggregateConfigKeyPart(args.expectedCiContexts, args.advisoryCheckRuns)}`, forceRefresh: args.forceRefresh, requiredContextsResolved: resolved, admissionKey: args.admissionKey, + advisoryCheckRuns: args.advisoryCheckRuns, }), ); } @@ -213,9 +237,16 @@ export function cachedLiveCiAggregate( token: string | undefined; expectedCiContexts: ReadonlyArray | null | undefined; admissionKey?: GitHubRateLimitAdmissionKey | undefined; + advisoryCheckRuns?: ReadonlyArray | null | 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), + liveCiAggregateConfigKeyPart(args.expectedCiContexts, args.advisoryCheckRuns), + ); const cached = args.facts.ciAggregates.get(key); if (cached) return cached; const next = evictLiveFactOnReject( @@ -231,6 +262,7 @@ export function cachedLiveCiAggregate( expectedCiContexts: args.expectedCiContexts, forceRefresh: false, admissionKey: args.admissionKey, + advisoryCheckRuns: args.advisoryCheckRuns, }), ); args.facts.ciAggregates.set(key, next); @@ -248,9 +280,16 @@ export function refreshLiveCiAggregate( token: string | undefined; expectedCiContexts: ReadonlyArray | null | undefined; admissionKey?: GitHubRateLimitAdmissionKey | undefined; + advisoryCheckRuns?: ReadonlyArray | null | 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), + liveCiAggregateConfigKeyPart(args.expectedCiContexts, args.advisoryCheckRuns), + ); const next = evictLiveFactOnReject( args.facts.ciAggregates, key, @@ -264,6 +303,7 @@ export function refreshLiveCiAggregate( expectedCiContexts: args.expectedCiContexts, forceRefresh: true, admissionKey: args.admissionKey, + advisoryCheckRuns: args.advisoryCheckRuns, }), ); args.facts.ciAggregates.set(key, next); @@ -355,9 +395,26 @@ export function reuseOrRefreshLiveCiAggregate( token: string | undefined, expectedCiContexts: ReadonlyArray | null | undefined, admissionKey?: GitHubRateLimitAdmissionKey, + advisoryCheckRuns?: ReadonlyArray | null | undefined, ): Promise { - const key = liveFactKey(repoFullName, headSha, baseRef, liveFactTokenPart(token), expectedCiContextsKeyPart(expectedCiContexts)); + const key = liveFactKey( + repoFullName, + headSha, + baseRef, + liveFactTokenPart(token), + liveCiAggregateConfigKeyPart(expectedCiContexts, 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, + admissionKey, + advisoryCheckRuns, + }); } diff --git a/src/queue/processors.ts b/src/queue/processors.ts index e5999a0f93..2b3f4f2378 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -112,6 +112,7 @@ import { primeDurablePrStateCache, refreshPullRequestDetails, } from "../github/backfill"; +import { resolveAdvisoryCheckHold } from "../github/advisory-check-runs.js"; import { contributorRepoStatsFromGittensor, fetchGittensorContributorSnapshot, @@ -2286,6 +2287,7 @@ function buildAgentMaintenancePlanInput(args: { unlinkedIssueMatchHold: AgentActionPlanInput["unlinkedIssueMatchHold"]; aiReviewLowConfidenceHold: AgentActionPlanInput["aiReviewLowConfidenceHold"]; unlinkedIssueMatchClose: AgentActionPlanInput["unlinkedIssueMatchClose"]; + advisoryCheckHold: AgentActionPlanInput["advisoryCheckHold"]; liveMergeState: string | undefined; liveReviewDecision: string | undefined; pr: PullRequestRecord; @@ -2311,6 +2313,7 @@ function buildAgentMaintenancePlanInput(args: { unlinkedIssueMatchHold, aiReviewLowConfidenceHold, unlinkedIssueMatchClose, + advisoryCheckHold, liveMergeState, liveReviewDecision, pr, @@ -2362,6 +2365,7 @@ function buildAgentMaintenancePlanInput(args: { ...(unlinkedIssueMatchHold !== undefined ? { unlinkedIssueMatchHold } : {}), ...(aiReviewLowConfidenceHold !== undefined ? { aiReviewLowConfidenceHold } : {}), ...(unlinkedIssueMatchClose !== undefined ? { unlinkedIssueMatchClose } : {}), + ...(advisoryCheckHold !== undefined ? { advisoryCheckHold } : {}), pr: { mergeableState: liveMergeState ?? pr.mergeableState, reviewDecision: liveReviewDecision ?? pr.reviewDecision, @@ -2477,6 +2481,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 @@ -2796,6 +2801,7 @@ async function runAgentMaintenancePlanAndExecute( // gate failed SOLELY on a sub-aiReviewCloseConfidence-floor ai_consensus_defect/ai_review_split finding under // the (default) hold_for_review disposition. See resolveAiReviewLowConfidenceHold's own doc comment. const aiReviewLowConfidenceHold = resolveAiReviewLowConfidenceHold(gate, settings); + const advisoryCheckHold = resolveAdvisoryCheckHold(ciAggregate.advisoryHoldDetails ?? [], settings.advisoryCheckRuns); const planned = planAgentMaintenanceActions( buildAgentMaintenancePlanInput({ gate, @@ -2816,6 +2822,7 @@ async function runAgentMaintenancePlanAndExecute( unlinkedIssueMatchHold, aiReviewLowConfidenceHold, unlinkedIssueMatchClose, + advisoryCheckHold, liveMergeState, liveReviewDecision, pr, @@ -3401,6 +3408,7 @@ async function prReadyForReview( token, expectedCiContexts: settings.expectedCiContexts, admissionKey, + advisoryCheckRuns: settings.advisoryCheckRuns, }).catch(() => undefined); if (ci?.hasPending) { // Staleness cap: inferred or unreadable pending CI can otherwise defer FOREVER (orphaned required context, @@ -7300,6 +7308,7 @@ async function resolveManifestPassedValidationCount( liveFacts: LiveGithubFacts; testExpectationsConfigured: boolean; testFileCount: number; + advisoryCheckRuns?: ReadonlyArray | null | undefined; }, ): Promise { if (hasValidationNote(args.body ?? "")) return 1; @@ -7324,6 +7333,7 @@ async function resolveManifestPassedValidationCount( token, expectedCiContexts: args.expectedCiContexts, admissionKey, + advisoryCheckRuns: args.advisoryCheckRuns, }); return liveCi.ciState === "passed" ? 1 : 0; } @@ -7387,6 +7397,7 @@ async function maybeApplyManifestPolicyGate( liveFacts: args.webhook.liveFacts, testExpectationsConfigured: manifest.testExpectations.length > 0, testFileCount, + advisoryCheckRuns: args.settings.advisoryCheckRuns, }); const guidance = buildFocusManifestGuidance({ manifest, @@ -9828,6 +9839,7 @@ async function maybePublishPrPublicSurface( token, expectedCiContexts: settings.expectedCiContexts, admissionKey, + advisoryCheckRuns: settings.advisoryCheckRuns, }); // Live merge-state too — the SAME source the disposition uses (planAgentMaintenanceActions reads liveMergeState). // The stored pr.mergeableState lags GitHub's async recompute, and the gate's own check/review publication can @@ -9858,6 +9870,13 @@ async function maybePublishPrPublicSurface( ...(detail.detailsUrl ? { detailsUrl: detail.detailsUrl } : {}), }), ); + const advisoryHoldDetails: CheckFailureDetail[] = (liveCi.advisoryHoldDetails ?? []).map( + (detail) => ({ + name: detail.name, + ...(detail.summary ? { summary: detail.summary } : {}), + ...(detail.detailsUrl ? { detailsUrl: detail.detailsUrl } : {}), + }), + ); const mergeReadiness: MergeReadiness = { ciState, ...(mergeStateLabel ? { mergeStateLabel } : {}), @@ -9866,6 +9885,7 @@ async function maybePublishPrPublicSurface( : {}), ...(failingDetails.length > 0 ? { failingDetails } : {}), ...(nonRequiredFailingDetails.length > 0 ? { nonRequiredFailingDetails } : {}), + ...(advisoryHoldDetails.length > 0 ? { advisoryHoldDetails } : {}), }; // The public comment must match the authoritative Gate check-run conclusion. const commentGate = gateEvaluation; @@ -9880,10 +9900,12 @@ async function maybePublishPrPublicSurface( // must render "held for review", not "✅ safe to merge". Compute the SAME guardrail-hit the disposition uses // (shared isGuardrailHit) and thread it so the signal and the action agree (the #4220 class, clean variant). const commentHardGuardrailGlobs = resolveHardGuardrailGlobs(settings); - const heldForReview = isGuardrailHit( - changedPathsForGuardrail(unifiedFiles), - commentHardGuardrailGlobs, - ); + const heldForReview = + isGuardrailHit( + changedPathsForGuardrail(unifiedFiles), + commentHardGuardrailGlobs, + ) || + resolveAdvisoryCheckHold(liveCi.advisoryHoldDetails ?? [], settings.advisoryCheckRuns) !== undefined; // Held-vs-closed parity (#8/#9): the disposition NEVER auto-closes an owner / automation-bot PR, so a gate // "close" verdict on one must headline "held", not "Closed". Compute the same author classification the // planner uses (repo-owner login match + protected automation author) and thread it to the comment. diff --git a/src/review/unified-comment.ts b/src/review/unified-comment.ts index 9952f39ed1..bb6b8b8d6a 100644 --- a/src/review/unified-comment.ts +++ b/src/review/unified-comment.ts @@ -97,6 +97,10 @@ export interface MergeReadiness { * neither auto-close the PR nor vanish without a trace). Rendered as its own non-blocking collapsible, * independent of `ciState`. */ nonRequiredFailingDetails?: CheckFailureDetail[]; + /** Configured advisory check-runs (#4372) that completed with a non-pass conclusion -- excluded from + * `ciState`/pending but routed to manual review; surfaced here so the maintainer sees which check/app + * triggered the hold. */ + advisoryHoldDetails?: CheckFailureDetail[]; } /** The structured synthesis of the reviewers' notes that drives BOTH the legacy unified comment @@ -568,6 +572,19 @@ function nonRequiredFailingChecksBlock(readiness: MergeReadiness | undefined): s return lines.join("\n"); } +function advisoryHoldChecksBlock(readiness: MergeReadiness | undefined): string { + const details = readiness?.advisoryHoldDetails ?? []; + const lines = details + .map((detail) => { + const name = escapePublicHtmlAngles(detail.name.trim()); + if (!name) return ""; + const reason = detail.summary?.trim() ? ` — ${escapePublicHtmlAngles(detail.summary.trim())}` : ""; + return `- ${name}${reason}`; + }) + .filter((line) => line.length > 0); + return lines.join("\n"); +} + function signalTable(input: UnifiedReviewInput, ctx: UnifiedCommentContext): string { const blockerCount = (input.blockers ?? []).length; const reviewerEvidence = @@ -706,6 +723,11 @@ export function renderUnifiedReviewComment(input: UnifiedReviewInput, ctx: Unifi blocks.push(details("Flagged checks (non-blocking)", nonRequiredFailingChecks, undefined, collapsiblesOpen)); } + const advisoryHoldChecks = advisoryHoldChecksBlock(input.readiness); + if (advisoryHoldChecks && verbosity !== "quiet") { + blocks.push(details("Advisory check-runs (manual review)", advisoryHoldChecks, undefined, collapsiblesOpen)); + } + blocks.push(signalTable(input, ctx)); // Linked-issue satisfaction advisory (#2174): additive, collapsed section — omitted entirely when the host diff --git a/src/settings/agent-actions.ts b/src/settings/agent-actions.ts index 39fba4573e..10e769d79d 100644 --- a/src/settings/agent-actions.ts +++ b/src/settings/agent-actions.ts @@ -380,6 +380,11 @@ export type AgentActionPlanInput = { // still closes normally -- only the "verdict=failure, driven solely by this AI-judgment blocker" path is held. The // gate check itself still reports failure (the merge stays blocked) -- only the one-shot CLOSE is suppressed. aiReviewLowConfidenceHold?: { reason: string; comment: string } | undefined; + // Configured advisory check-run hold (#4372): a third-party app's completed, non-pass check-run that must never + // block CI pass/fail/pending but must route an otherwise-green PR to manual review instead of merging through + // a flag the operator installed an app to raise. Resolved by the trigger from the live CI aggregate's + // advisoryHoldDetails + settings.advisoryCheckRuns. + advisoryCheckHold?: { checkNames: readonly string[]; reason: string; comment: string } | undefined; // Screenshot-table gate (#2006): a DETERMINISTIC verdict (no AI, zero hallucination risk) that an in-scope // visual/frontend PR's body is missing a before/after screenshot table (or has an image outside a table, or // a screenshot committed to the repo instead of uploaded to the PR) AND (#4110) the bot's own visual-capture @@ -813,6 +818,7 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne guardrailHit || input.migrationCollisionHold !== undefined || input.unlinkedIssueMatchHold !== undefined || + input.advisoryCheckHold !== undefined || (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 @@ -955,6 +961,21 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne }); } + // 1e) advisory-check-run manual-review fallback (#4372) — same shape as the migration/unlinked fallbacks + // above: when review_state_label is OFF, a configured advisory check-run's non-pass conclusion still suppresses + // merge and must surface manual-review + an actionable comment naming the triggering check/app. + if (reviewGood && input.advisoryCheckHold !== undefined && !acting("review_state_label") && labels.manualReview !== null && acting("merge") && !hasLabelOrPlanned(input.pr.labels, actions, labels.manualReview)) { + actions.push({ + actionClass: "label", + autonomyClass: "merge", + requiresApproval: approval("merge"), + reason: `verdict=${conclusion}; ${input.advisoryCheckHold.reason}`, + label: labels.manualReview, + labelOp: "add", + comment: sanitizePublicComment(input.advisoryCheckHold.comment), + }); + } + // 1e) unlinked-issue-match REPEAT manual-review fallback when `close` autonomy can't act (gate-review // finding): mirrors 1d, but for the escalated-repeat case folded into `heldForManualReview` above only // when close isn't acting — without this, a confirmed repeat would silently MERGE with no visible signal @@ -1000,6 +1021,8 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne ? `verdict=${conclusion}; ${input.migrationCollisionHold.reason}` : input.unlinkedIssueMatchHold !== undefined ? `verdict=${conclusion}; ${input.unlinkedIssueMatchHold.reason}` + : input.advisoryCheckHold !== undefined + ? `verdict=${conclusion}; ${input.advisoryCheckHold.reason}` : input.unlinkedIssueMatchClose !== undefined ? `verdict=${conclusion}; ${input.unlinkedIssueMatchClose.reason}` : heldForManualReview @@ -1021,6 +1044,8 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne ? { comment: sanitizePublicComment(input.migrationCollisionHold.comment) } : !linkedIssueCloseInFlight && !unlinkedIssueMatchViolated && reviewGood && input.unlinkedIssueMatchHold !== undefined ? { comment: sanitizePublicComment(input.unlinkedIssueMatchHold.comment) } + : !linkedIssueCloseInFlight && !unlinkedIssueMatchViolated && reviewGood && input.advisoryCheckHold !== undefined + ? { comment: sanitizePublicComment(input.advisoryCheckHold.comment) } : !linkedIssueCloseInFlight && !unlinkedIssueMatchViolated && reviewGood && input.unlinkedIssueMatchClose !== undefined ? { comment: sanitizePublicComment(input.unlinkedIssueMatchClose.comment) } : {}), diff --git a/src/signals/focus-manifest.ts b/src/signals/focus-manifest.ts index fcdeee392d..43a4e69011 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 41d8577931..be2c403b7d 100644 --- a/src/types.ts +++ b/src/types.ts @@ -792,6 +792,9 @@ 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 excluded from CI pass/fail/pending aggregation; + * a completed non-pass routes to manual-review. Config-as-code only — set via `.loopover.yml`. */ + advisoryCheckRuns?: ReadonlyArray | 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/advisory-check-runs.test.ts b/test/unit/advisory-check-runs.test.ts new file mode 100644 index 0000000000..28dc7db859 --- /dev/null +++ b/test/unit/advisory-check-runs.test.ts @@ -0,0 +1,54 @@ +import { describe, expect, it } from "vitest"; +import { + advisoryCheckRunsKeyPart, + isAdvisoryCheckRunSettledPass, + matchesConfiguredAdvisoryCheckRun, + resolveAdvisoryCheckHold, +} from "../../src/github/advisory-check-runs.js"; + +const EXAMPLE_SPECS = [{ name: "Example trust scan", appSlug: "example-trust-app" }] as const; + +describe("advisory-check-runs (#4372)", () => { + it("matches configured check-runs by name + app slug case-insensitively", () => { + expect( + matchesConfiguredAdvisoryCheckRun( + { name: " Example Trust Scan ", app: { slug: "Example-Trust-App" } }, + EXAMPLE_SPECS, + ), + ).toBe(true); + expect(matchesConfiguredAdvisoryCheckRun({ name: "Example trust scan", app: { slug: "other-app" } }, EXAMPLE_SPECS)).toBe(false); + expect(matchesConfiguredAdvisoryCheckRun({ name: "Other scan", app: { slug: "example-trust-app" } }, EXAMPLE_SPECS)).toBe(false); + }); + + it("treats success/neutral/skipped as settled pass conclusions", () => { + expect(isAdvisoryCheckRunSettledPass("success")).toBe(true); + expect(isAdvisoryCheckRunSettledPass("NEUTRAL")).toBe(true); + expect(isAdvisoryCheckRunSettledPass("skipped")).toBe(true); + expect(isAdvisoryCheckRunSettledPass("action_required")).toBe(false); + expect(isAdvisoryCheckRunSettledPass("failure")).toBe(false); + }); + + it("builds a stable cache-key fragment for configured specs", () => { + expect(advisoryCheckRunsKeyPart([])).toBe(""); + expect(advisoryCheckRunsKeyPart(null)).toBe(""); + expect(advisoryCheckRunsKeyPart([{ name: "B", appSlug: "b-app" }, { name: "A", appSlug: "a-app" }])).toBe( + JSON.stringify([ + { name: "A", appSlug: "a-app" }, + { name: "B", appSlug: "b-app" }, + ]), + ); + }); + + it("resolves a manual-review hold with a public-safe comment naming the check/app", () => { + const hold = resolveAdvisoryCheckHold( + [{ name: "Example trust scan", appSlug: "example-trust-app", summary: "Needs operator review" }], + EXAMPLE_SPECS, + ); + expect(hold?.checkNames).toEqual(["Example trust scan"]); + expect(hold?.reason).toContain("Example trust scan"); + expect(hold?.comment).toContain("example-trust-app"); + expect(hold?.comment).toContain("Needs operator review"); + expect(resolveAdvisoryCheckHold([], EXAMPLE_SPECS)).toBeUndefined(); + expect(resolveAdvisoryCheckHold([{ name: "Example trust scan" }], null)).toBeUndefined(); + }); +}); diff --git a/test/unit/agent-actions.test.ts b/test/unit/agent-actions.test.ts index e0d1091342..1eaa420ba2 100644 --- a/test/unit/agent-actions.test.ts +++ b/test/unit/agent-actions.test.ts @@ -64,6 +64,20 @@ describe("planAgentMaintenanceActions (#778)", () => { const collision = planAgentMaintenanceActions(input({ conclusion: "success", autonomy: { merge: "auto", review_state_label: "auto" }, migrationCollisionLabel: "migration-review", migrationCollisionHold: { reason: "live migrations/** collision", comment: "Please rebase." }, pr: { labels: [], mergeableState: "clean" } })); expect(collision.some((a) => a.actionClass === "label" && a.label === "migration-review")).toBe(true); expect(classes(collision)).not.toContain("merge"); + + const advisory = planAgentMaintenanceActions(input({ + conclusion: "success", + autonomy: { merge: "auto", review_state_label: "auto" }, + manualReviewLabel: "manual-review", + advisoryCheckHold: { + checkNames: ["Example trust scan"], + reason: "advisory check-run hold (Example trust scan)", + comment: "LoopOver: a configured advisory check-run needs maintainer action before this PR can merge:\n- Example trust scan (example-trust-app)", + }, + pr: { labels: [], mergeableState: "clean", reviewDecision: "APPROVED" }, + })); + expect(advisory.some((a) => a.actionClass === "label" && a.label === "manual-review")).toBe(true); + expect(classes(advisory)).not.toContain("merge"); }); it("uses manualReviewLabel for manual holds without enabling ready/changes review-state labels", () => { diff --git a/test/unit/backfill-2.test.ts b/test/unit/backfill-2.test.ts index 9b7b638d25..2c8af69cb0 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,101 @@ describe("GitHub backfill", () => { ]); }); + it("configured advisory check-runs (#4372) are excluded from ciState/hasPending and routed to advisoryHoldDetails when non-pass", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url.includes("/check-runs?")) { + return Response.json({ + check_runs: [ + { name: "validate", status: "completed", conclusion: "success", app: { slug: "github-actions" } }, + { + name: "Example trust scan", + status: "completed", + conclusion: "action_required", + app: { slug: "example-trust-app" }, + output: { title: "Needs operator review" }, + details_url: "https://example.test/checks/trust", + }, + { + name: "Example trust scan", + status: "in_progress", + conclusion: null, + app: { slug: "example-trust-app" }, + }, + ], + }); + } + 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 }); + }); + + const advisoryCheckRuns = [{ name: "Example trust scan", appSlug: "example-trust-app" }]; + const aggregate = await fetchLiveCiAggregate( + env, + "owner/example-repo", + "sha4372", + "public-token", + new Set(["validate"]), + undefined, + advisoryCheckRuns, + ); + + expect(aggregate.ciState).toBe("passed"); + expect(aggregate.hasPending).toBe(false); + expect(aggregate.hasVisiblePending).toBe(false); + expect(aggregate.failingDetails).toEqual([]); + expect(aggregate.nonRequiredFailingDetails).toEqual([]); + expect(aggregate.advisoryHoldDetails).toEqual([ + { + name: "Example trust scan", + summary: "Needs operator review", + detailsUrl: "https://example.test/checks/trust", + appSlug: "example-trust-app", + }, + ]); + }); + + it("configured advisory check-runs stay byte-identical for unconfigured repos and settled pass conclusions", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url.includes("/check-runs?")) { + return Response.json({ + check_runs: [ + { name: "validate", status: "completed", conclusion: "success", app: { slug: "github-actions" } }, + { + name: "Example trust scan", + status: "completed", + conclusion: "success", + app: { slug: "example-trust-app" }, + }, + ], + }); + } + 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 }); + }); + + const unconfigured = await fetchLiveCiAggregate(env, "owner/example-repo", "sha4372b", "public-token", new Set(["validate"])); + expect(unconfigured.advisoryHoldDetails).toEqual([]); + expect(unconfigured.ciState).toBe("passed"); + + const configured = await fetchLiveCiAggregate( + env, + "owner/example-repo", + "sha4372b", + "public-token", + new Set(["validate"]), + undefined, + [{ name: "Example trust scan", appSlug: "example-trust-app" }], + ); + expect(configured.advisoryHoldDetails).toEqual([]); + expect(configured.ciState).toBe("passed"); + }); + 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..1ec0bd5064 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(); diff --git a/test/unit/focus-manifest.test.ts b/test/unit/focus-manifest.test.ts index 6dc50e0aca..ec01d38e89 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:", @@ -5090,6 +5091,29 @@ describe("gate.claMode / gate.cla CLA / license-compatibility gate config (#2564 }); }); +describe("gate.advisoryCheckRuns (#4372)", () => { + it("parses a clean list of name/appSlug pairs, sets present, and round-trips", () => { + const m = parseFocusManifest({ + gate: { + advisoryCheckRuns: [{ name: "Example trust scan", appSlug: "example-trust-app" }], + }, + }); + expect(m.gate.advisoryCheckRuns).toEqual([{ name: "Example trust scan", appSlug: "example-trust-app" }]); + expect(m.gate.present).toBe(true); + const round = parseFocusManifest({ gate: gateConfigToJson(m.gate) as Record }); + expect(round.gate.advisoryCheckRuns).toEqual(m.gate.advisoryCheckRuns); + }); + + it("drops entries missing name or appSlug and keeps valid ones", () => { + const m = parseFocusManifest({ + gate: { + advisoryCheckRuns: [{ name: "Example trust scan" }, { name: "Other", appSlug: "other-app" }] as never, + }, + }); + expect(m.gate.advisoryCheckRuns).toEqual([{ name: "Other", appSlug: "other-app" }]); + }); +}); + describe("gate.expectedCiContexts (#selfhost-ci-verification)", () => { it("parses a clean list, sets present, and preserves order", () => { const m = parseFocusManifest({ gate: { expectedCiContexts: ["build", "test"] } }); diff --git a/test/unit/unified-comment.test.ts b/test/unit/unified-comment.test.ts index 102dc4f125..fc0ce2dbe4 100644 --- a/test/unit/unified-comment.test.ts +++ b/test/unit/unified-comment.test.ts @@ -410,6 +410,22 @@ describe("renderUnifiedReviewComment", () => { expect(md).not.toContain("**CI checks failing**"); }); + it("renders configured advisory check-run holds as a manual-review section (#4372)", () => { + const md = renderUnifiedReviewComment( + { + ...base, + readiness: { + ciState: "passed", + advisoryHoldDetails: [{ name: "Example trust scan", summary: "Needs operator review" }], + }, + }, + {}, + ); + expect(md).toContain("Advisory check-runs (manual review)"); + expect(md).toContain("- Example trust scan — Needs operator review"); + expect(md).not.toContain("**CI checks failing**"); + }); + it("omits the 'Flagged checks' section when nonRequiredFailingDetails is absent/empty (default, byte-identical)", () => { expect(renderUnifiedReviewComment({ ...base, readiness: { ciState: "passed" } }, {})).not.toContain("Flagged checks"); expect(renderUnifiedReviewComment({ ...base, readiness: { ciState: "passed", nonRequiredFailingDetails: [] } }, {})).not.toContain("Flagged checks");