diff --git a/src/review/content-lane-wire.ts b/src/review/content-lane-wire.ts index 0a3fd09cbe..106cb4d4be 100644 --- a/src/review/content-lane-wire.ts +++ b/src/review/content-lane-wire.ts @@ -12,10 +12,10 @@ // verdict NEVER depends on an AI model, so this is independent of the AI-reviewer accuracy work (the surface lane // emits none of the AI_JUDGMENT_BLOCKER_CODES). // -// SAFETY (three deliberate guards): -// 1. A generic HARD (non-AI-judgment) blocker — e.g. a committed secret detected before this runs — is PRESERVED: -// a surface "merge" can never clear a real critical the generic gate already raised (applySurfaceGate unions -// them). +// SAFETY (four deliberate guards): +// 1. A generic HARD (non-AI-judgment, non-warning-only) blocker — e.g. a committed secret detected before this +// runs — is PRESERVED: a surface "merge" can never clear a real critical the generic gate already raised +// (applySurfaceGate unions them). // 2. An unreadable head — or a null base on a file GitHub marks "modified" (whose base MUST exist, so a null // read is a transient blip, not an absent base) — defers to the generic gate rather than auto-closing a good // PR on a spurious "the submission looks empty/invalid" read. (A null base on an ADDED file is the expected @@ -24,7 +24,18 @@ // NOT override a decisive surface verdict (applySurfaceGate). The surface lane is the sole, AI-free // adjudicator for this structured data — an AI opinion has no standing to veto it, only a real deterministic // blocker does (see guard #1). -import { AI_JUDGMENT_BLOCKER_CODES, type GateCheckEvaluation, isAiJudgmentOnlyFailure } from "../rules/advisory"; +// 4. A generic failure caused SOLELY by a same-linked-issue `duplicate_pr_risk` finding escalated by +// `duplicatePrGateMode: "block"` does NOT one-shot-close a decisive surface merge either (applySurfaceGate). +// That finding is advisory by nature (severity "warning" — a lead for a human, not proof of a defect): it +// downgrades the conclusion to a HOLD (neutral, never auto-merged, never a hard failure) with the finding +// still visible in the comment, rather than either silently clearing it (losing the signal) or letting it +// alone close a submission whose own structured content is clean. This is scoped to EXACTLY that finding code +// (see isDuplicateOnlyFailure / DUPLICATE_ONLY_BLOCKER_CODES) — NOT every warning-severity finding — because +// several OTHER findings (missing_linked_issue, self_authored_linked_issue, manifest_linked_issue_required, +// manifest_missing_tests) are also severity "warning" but block-mode-escalatable via their OWN independent +// maintainer-configured gate, and that explicit opt-in must still close a PR outright (see guard #1 for why a +// genuinely critical finding, or one of these other configured gates, still wins outright). +import { AI_JUDGMENT_BLOCKER_CODES, type GateCheckEvaluation, isAiJudgmentOnlyFailure, isDuplicateOnlyFailure } from "../rules/advisory"; import { GITTENSORY_GATE_CHECK_NAME } from "./check-names"; import { isContentLaneEnabled } from "./content-lane/flag"; import { runSurfaceReview, type SurfaceReviewInput, type SurfaceReviewResult } from "./content-lane/orchestrator"; @@ -89,7 +100,20 @@ export function surfaceVerdictToGate(result: SurfaceReviewResult): { * A real (non-AI) blocker in the mix still falls through to the union below and blocks. The generic gate's * OTHER (non-blocker) warnings are unrelated to the discarded AI blocker and are preserved onto the surface * result rather than silently dropped — see `evaluateWithSurfaceLane` for the companion `advisory.findings` - * cleanup that keeps the public comment from re-surfacing the overridden AI defect via a separate path. */ + * cleanup that keeps the public comment from re-surfacing the overridden AI defect via a separate path. + * + * A second, analogous exception (guard #4) applies when the generic gate's blockers are ALL duplicate-only + * (a same-linked-issue `duplicate_pr_risk` finding escalated into a blocker by `duplicatePrGateMode: "block"`, + * see `isDuplicateOnlyFailure`): a decisive surface merge downgrades that failure to a HOLD (neutral) rather than + * either overriding it outright (losing the signal a maintainer should still see) or letting it one-shot-close a + * submission whose own structured content is clean. The generic blockers are folded into `warnings` (same shape + * `surfaceVerdictToGate` already uses for its own "manual" verdict) so the concern stays visible in the public + * comment, and the title/summary are rewritten to name the actual hold reason (the blocker's own detail) rather + * than inheriting the surface's clean-merge text, which would otherwise leave the posted check-run silent about + * why it's held. A blocker set that mixes a duplicate-only finding with a genuinely critical one, OR with another + * maintainer-configured block-mode finding (missing_linked_issue, self_authored_linked_issue, etc.), is NOT + * duplicate-only (`isDuplicateOnlyFailure` requires EVERY blocker to be exactly `duplicate_pr_risk`) and still + * falls through to the unconditional union+failure below. */ export function applySurfaceGate( generic: GateCheckEvaluation | undefined, surface: GateCheckEvaluation | null, @@ -106,6 +130,16 @@ export function applySurfaceGate( if (isAiJudgmentOnlyFailure(generic) && surface.conclusion === "success") { return { ...surface, warnings: [...generic.warnings, ...surface.warnings] }; } + if (isDuplicateOnlyFailure(generic) && surface.conclusion === "success") { + const heldReason = generic.blockers.map((blocker) => blocker.detail || blocker.title).join(" "); + return { + ...surface, + conclusion: "neutral", + title: `${GITTENSORY_GATE_CHECK_NAME} — held for review`, + summary: heldReason, + warnings: [...generic.blockers, ...generic.warnings, ...surface.warnings], + }; + } return { enabled: true, conclusion: "failure", diff --git a/src/review/content-lane/orchestrator.ts b/src/review/content-lane/orchestrator.ts index c1c82efeef..1063a53477 100644 --- a/src/review/content-lane/orchestrator.ts +++ b/src/review/content-lane/orchestrator.ts @@ -8,7 +8,11 @@ // 5. validates EACH remaining appended entry independently via the spec's OWN assessAppendedEntry / // assessProviderEntry validators (the orchestrator never hardcodes a domain-specific validator — a spec // with no validator configured gets "manual") and returns one aggregate verdict: close if any entry is -// invalid, manual if any (remaining) needs manual review, merge only when every entry is clean. +// invalid, manual if any (remaining) needs manual review, merge only when every entry is clean, and +// 6. for an entry submission riding alongside a path-shaped provider companion file, confirms the companion is +// actually a DEBUT (absent at base — an edit to an already-registered provider routes to manual instead), +// then validates it via assessProviderEntry and combines it with the entry's own result — merge only when +// BOTH sides are clean. // Pure + injectable: unit tests pass a loadFile stub, so no network. The live wiring (a per-repo, // flag-gated branch in the review body) is a separate follow-up. import { @@ -95,6 +99,41 @@ function duplicateEntryCloseSummary(): string { // orchestrator can't itself judge the entry's content — route to manual review rather than merge or close. const NO_VALIDATOR_ENTRY_SUMMARY = "No validator is configured for this registry's surface entries — routing to review."; const NO_VALIDATOR_PROVIDER_SUMMARY = "No validator is configured for this registry's provider submissions — routing to review."; +// classifyRegistryPrScope identifies a provider companion by FILE PATH alone (it does no I/O); that only proves +// the file is shaped like a provider submission, not that it's a genuine DEBUT (a brand-new provider, not an edit +// to one already in the registry). The orchestrator independently confirms debut-ness once it has the fetched +// content — see the base-presence check in runSurfaceReview. +const NON_DEBUT_COMPANION_SUMMARY = + "Registry submission's provider companion already exists in the registry — this isn't a debut provider, so it needs a human to review the edit alongside the entry."; + +/** + * Combines an entry-submission's aggregate Assessment with its companion debut-provider file's ProviderAssessment + * into ONE SurfaceReviewResult — the "entry + debut provider in the same PR" flow. `providerRaw` is already loaded + * by the caller (in parallel with the entry's own head/base fetches — see runSurfaceReview), `assessProvider` + * already confirmed present, and the companion already confirmed to actually BE a debut (absent at base) — so + * this function does no I/O and can't itself punt to "no validator configured" or "not actually a debut". + * Reuses `fromProvider` for the provider's own ok/close mapping — the same conversion the standalone provider- + * submission scope uses — so the two paths can never silently drift apart. Decisive: close if EITHER side is + * invalid, manual if the entry needs manual review and the provider is clean (a provider assessment is itself + * always decisive — merge or close, never manual — so it can never be the source of a manual verdict here), merge + * only when both are clean (forwarding the provider's own merge summary, same as a standalone provider merge). + */ +function assessEntryWithProviderCompanion( + assessProvider: NonNullable, + entryAssessment: Assessment, + providerRaw: string | null, + opts: SurfaceReviewInput["opts"], +): SurfaceReviewResult { + if (entryAssessment.verdict === "closed") { + return { verdict: "close", summary: entryAssessment.summary, reason: entryAssessment.reason }; + } + const providerResult = fromProvider(assessProvider(safeParseJson(providerRaw), opts)); + if (providerResult.verdict === "close") return providerResult; + if (entryAssessment.verdict === "manual-review") { + return { verdict: "manual", summary: entryAssessment.summary }; + } + return providerResult; +} /** * Aggregate N independent per-entry assessments into ONE verdict: close if ANY entry is invalid, manual if ANY @@ -147,20 +186,48 @@ export async function runSurfaceReview(spec: RegistryLaneSpec, input: SurfaceRev } // A submission scope (entry/provider) always carries a directFile (classifier invariant; see classifyRegistryPrScope). const directFile = scope.directFile as string; + const companionProviderFile = scope.providerCompanionFile; + // Anything besides the direct file must be a companion the classifier already approved: the recognized debut- + // provider companion (validated below) or a spec.artifactPattern match (a generated build artifact — allowed + // as-is, never validated). Anything else here is an unrecognized/ambiguous shape (classifyRegistryPrScope only + // reaches this scope when every file matched SOME allowed pattern, so this is the residual "which companion is + // it" case, e.g. more than one provider companion) — fall back to routing it to manual review. for (const file of input.changedFiles) { const normalized = file.trim(); - if (normalized === "" || normalized === directFile) continue; + if (normalized === "" || normalized === directFile || normalized === companionProviderFile) continue; + if (spec.artifactPattern?.test(normalized)) continue; return { verdict: "manual", summary: "Registry submission includes companion file changes — routing to review." }; } - const headRaw = await input.loadFile(directFile, "head"); if (scope.isProvider) { const assessProvider = spec.assessProviderEntry; if (!assessProvider) { return { verdict: "manual", summary: NO_VALIDATOR_PROVIDER_SUMMARY }; } + const headRaw = await input.loadFile(directFile, "head"); return fromProvider(assessProvider(safeParseJson(headRaw), input.opts)); } - const baseRaw = await input.loadFile(directFile, "base"); + // A companion provider file with no configured validator can never be judged, whatever the entry itself turns + // out to be — hold before paying for the entry-side fetch + diff + per-entry assessment pipeline below. + if (companionProviderFile !== null && !spec.assessProviderEntry) { + return { verdict: "manual", summary: NO_VALIDATOR_PROVIDER_SUMMARY }; + } + // The entry's head/base fetch and the companion provider's head/base fetch (when present) are up to four + // independent GitHub-Contents reads with no data dependency on each other — resolve them concurrently rather + // than paying for sequential round-trips. + const [headRaw, baseRaw, providerHeadRaw, providerBaseRaw] = await Promise.all([ + input.loadFile(directFile, "head"), + input.loadFile(directFile, "base"), + companionProviderFile !== null ? input.loadFile(companionProviderFile, "head") : Promise.resolve(null), + companionProviderFile !== null ? input.loadFile(companionProviderFile, "base") : Promise.resolve(null), + ]); + // A companion recognized by path alone (see classifyRegistryPrScope) is only a genuine DEBUT provider when it's + // absent at base — the same "null base ⇒ brand-new file" convention diffAppendedSurfaceEntries already applies + // to the entry file itself. A non-null base means this PR is editing an existing, already-registered provider + // record alongside an unrelated entry — a materially different, more sensitive shape that needs a human, not + // the automatic debut-provider merge/close flow below. + if (companionProviderFile !== null && providerBaseRaw !== null) { + return { verdict: "manual", summary: NON_DEBUT_COMPANION_SUMMARY }; + } const appendedEntries = diffAppendedSurfaceEntries(headRaw, baseRaw, spec.collectionField); const maxAppendedEntries = spec.maxAppendedEntries ?? DEFAULT_MAX_APPENDED_ENTRIES; if (appendedEntries === null || appendedEntries.length === 0 || appendedEntries.length > maxAppendedEntries) { @@ -179,5 +246,10 @@ export async function runSurfaceReview(spec: RegistryLaneSpec, input: SurfaceRev const assessment = pickAggregateAssessment( appendedEntries.map((appendedEntry) => assessEntry(headDoc, { ...input.opts, appendedEntry })), ); + if (companionProviderFile !== null) { + // Guaranteed non-null: the no-validator short-circuit above already returned when this spec lacks one. + const assessProvider = spec.assessProviderEntry as NonNullable; + return assessEntryWithProviderCompanion(assessProvider, assessment, providerHeadRaw, input.opts); + } return { verdict: toCoreVerdict(assessment.verdict), summary: assessment.summary, reason: assessment.reason }; } diff --git a/src/review/content-lane/registry-logic.ts b/src/review/content-lane/registry-logic.ts index cd5f5039d1..7296d6b87d 100644 --- a/src/review/content-lane/registry-logic.ts +++ b/src/review/content-lane/registry-logic.ts @@ -702,6 +702,17 @@ export interface RegistryScopeResult { scope: RegistryPrScope; directFile: string | null; isProvider: boolean; + /** For an "entry-submission" scope only: a companion file that is PATH-SHAPED like a provider submission + * (matches spec.providerFilePattern) riding along with the entry in the same PR, set only when exactly one such + * companion is present. This is classification only — a pure, I/O-free path match — and does NOT by itself + * prove the companion is a genuine DEBUT (a brand-new provider, not an edit to one already in the registry); + * the orchestrator independently confirms that once it has fetched content (see runSurfaceReview's base- + * presence check) before treating it as the debut-provider companion flow. An entry file always wins the scope + * classification over a provider file present in the same PR (so "provider-submission scope with a companion + * entry file" cannot occur — the entry file becomes directFile and the provider file becomes this field + * instead). Null when there is no provider companion, when there is more than one (ambiguous — ordinary + * companion-file review still applies), or when the scope isn't entry-submission. */ + providerCompanionFile: string | null; } /** @@ -715,25 +726,36 @@ export function classifyRegistryPrScope(spec: RegistryLaneSpec, changedFiles: st const entryFiles = files.filter((f) => spec.entryFilePattern.test(f)); const providerFiles = spec.providerFilePattern ? files.filter((f) => spec.providerFilePattern!.test(f)) : []; if (entryFiles.length > 1 || (entryFiles.length === 0 && providerFiles.length > 1)) { - return { scope: "mixed-files", directFile: null, isProvider: false }; + return { scope: "mixed-files", directFile: null, isProvider: false, providerCompanionFile: null }; } const isEntryPr = entryFiles.length === 1; const isProviderPr = entryFiles.length === 0 && providerFiles.length === 1; if (!isEntryPr && !isProviderPr) { - return { scope: "not-direct-submission", directFile: null, isProvider: false }; + return { scope: "not-direct-submission", directFile: null, isProvider: false, providerCompanionFile: null }; } const isAllowed = (f: string): boolean => spec.entryFilePattern.test(f) || (spec.providerFilePattern?.test(f) ?? false) || (spec.artifactPattern?.test(f) ?? false); if (files.some((f) => !isAllowed(f))) { - return { scope: "mixed-files", directFile: null, isProvider: false }; + return { scope: "mixed-files", directFile: null, isProvider: false, providerCompanionFile: null }; } // isEntryPr/isProviderPr each guarantee exactly one match (guarded by the early return), so [0] is always - // defined; the `?? null` only satisfies noUncheckedIndexedAccess and can never fire. - /* v8 ignore start */ - return isProviderPr - ? { scope: "provider-submission", directFile: providerFiles[0] ?? null, isProvider: true } - : { scope: "entry-submission", directFile: entryFiles[0] ?? null, isProvider: false }; - /* v8 ignore stop */ + // defined; the `?? null` fallbacks below only satisfy noUncheckedIndexedAccess and can never fire. + if (isProviderPr) { + /* v8 ignore next */ + return { scope: "provider-submission", directFile: providerFiles[0] ?? null, isProvider: true, providerCompanionFile: null }; + } + // isEntryPr: a debut-provider companion is exactly one OTHER providerFilePattern match riding along with the + // entry file — more than one is an unrecognized shape (ambiguous which is "the" debut provider) and falls back + // to the ordinary companion-file-changes review path in the orchestrator, same as before this field existed. + const hasSingleProviderCompanion = providerFiles.length === 1; + return { + scope: "entry-submission", + /* v8 ignore next */ + directFile: entryFiles[0] ?? null, + isProvider: false, + /* v8 ignore next -- hasSingleProviderCompanion guarantees providerFiles[0] is defined; the ?? null only satisfies noUncheckedIndexedAccess. */ + providerCompanionFile: hasSingleProviderCompanion ? (providerFiles[0] ?? null) : null, + }; } export function isRegistrySubmissionScope(scope: RegistryPrScope): boolean { diff --git a/src/rules/advisory.ts b/src/rules/advisory.ts index 09db8a585b..4ab817adfc 100644 --- a/src/rules/advisory.ts +++ b/src/rules/advisory.ts @@ -109,6 +109,29 @@ export function isAiJudgmentOnlyFailure(evaluation: GateCheckEvaluation): boolea return evaluation.conclusion === "failure" && evaluation.blockers.length > 0 && evaluation.blockers.every((blocker) => AI_JUDGMENT_BLOCKER_CODES.has(blocker.code)); } +// DUPLICATE-ONLY blocker codes: findings whose own severity is always "warning" (advisory by nature — a +// same-linked-issue overlap is a lead for a human, not proof of a defect) but that a per-repo gate-mode config +// can still escalate into a hard blocker (`duplicate_pr_risk` under `duplicatePrGateMode: "block"`, its ONLY +// escalation path — see isConfiguredGateBlocker). Kept to exactly this code, NOT "every warning-severity finding": +// `missing_linked_issue`, `self_authored_linked_issue`, `manifest_linked_issue_required`, and +// `manifest_missing_tests` are ALSO severity "warning" and ALSO block-mode-escalatable via their own maintainer- +// configured gate (linkedIssueGateMode / selfAuthoredLinkedIssueGateMode / manifestPolicyGateMode), and a +// maintainer who explicitly opted one of THOSE into "block" must have it still close a PR outright — only the +// same-linked-issue overlap concern is meant to downgrade to a hold for a decisive surface-lane merge. +export const DUPLICATE_ONLY_BLOCKER_CODES = new Set(["duplicate_pr_risk"]); + +/** True when the gate FAILED *solely* because of duplicate-only blockers (every blocker is in + * DUPLICATE_ONLY_BLOCKER_CODES) — i.e. the failure was produced entirely by a same-linked-issue overlap + * escalated into blocker status by `duplicatePrGateMode: "block"`, never a genuinely critical finding (a + * committed secret, an unsafe URL, ...) or another maintainer-configured block-mode gate. A caller merging this + * evaluation against an independent, decisive, AI-free verdict (see `applySurfaceGate`) can use this to tell "a + * real defect, or another gate the maintainer explicitly opted into blocking" apart from "an advisory-by-nature + * overlap concern" before letting it override that verdict outright. An empty blocker list is NOT a + * duplicate-only failure. PURE. */ +export function isDuplicateOnlyFailure(evaluation: GateCheckEvaluation): boolean { + return evaluation.conclusion === "failure" && evaluation.blockers.length > 0 && evaluation.blockers.every((blocker) => DUPLICATE_ONLY_BLOCKER_CODES.has(blocker.code)); +} + /** * Historical compatibility shim for the old green-CI AI refutation path. The gate verdict is now authoritative: * if an AI review finding is configured as blocking, green CI cannot rewrite it to success. This keeps the public diff --git a/test/unit/content-lane-orchestrator.test.ts b/test/unit/content-lane-orchestrator.test.ts index 974ea4d8ba..1135baf60b 100644 --- a/test/unit/content-lane-orchestrator.test.ts +++ b/test/unit/content-lane-orchestrator.test.ts @@ -94,21 +94,156 @@ describe("runSurfaceReview (deterministic + decisive: merge/close, rarely manual expect(r?.verdict).toBe("merge"); }); - it("routes companion provider or artifact changes to manual review without trusting the entry verdict", async () => { + // Bug #1 (confirmed live on metagraphed PR #2654): a genuine "entry + its debut provider in the same PR" + // companion is already APPROVED by classifyRegistryPrScope (isAllowed matches providerFilePattern), but this + // used to be thrown away and routed to manual regardless. It must now be validated via assessProviderEntry and + // combined with the entry's own assessment: merge only when BOTH sides are clean. + const ARTIFACT = "public/metagraph/index.json"; + const validProviderDoc = JSON.stringify({ provider: { id: "acme", name: "Acme", website_url: "https://acme.example" } }); + const invalidProviderDoc = JSON.stringify({ provider: { name: "Acme", website_url: "https://acme.example" } }); // missing id + + it("[test 1] merges an entry submission whose companion is a genuine, valid debut provider", async () => { + const r = await runSurfaceReview(METAGRAPHED_LANE_SPEC, { + changedFiles: [SUBNET, PROVIDER], + loadFile: (path, ref) => + Promise.resolve(path === PROVIDER ? (ref === "head" ? validProviderDoc : null) : ref === "head" ? doc([existing, newEntry]) : doc([existing])), + }); + expect(r?.verdict).toBe("merge"); + }); + + it("[test 2a] closes when the entry is valid but its debut-provider companion is invalid", async () => { + const r = await runSurfaceReview(METAGRAPHED_LANE_SPEC, { + changedFiles: [SUBNET, PROVIDER], + loadFile: (path, ref) => + Promise.resolve(path === PROVIDER ? (ref === "head" ? invalidProviderDoc : null) : ref === "head" ? doc([existing, newEntry]) : doc([existing])), + }); + expect(r?.verdict).toBe("close"); + }); + + it("routes to MANUAL when the entry needs manual review (auth_required) and its debut-provider companion is valid", async () => { + const authEntry = { ...newEntry, auth_required: true }; + const r = await runSurfaceReview(METAGRAPHED_LANE_SPEC, { + changedFiles: [SUBNET, PROVIDER], + loadFile: (path, ref) => + Promise.resolve(path === PROVIDER ? (ref === "head" ? validProviderDoc : null) : ref === "head" ? doc([existing, authEntry]) : doc([existing])), + }); + expect(r?.verdict).toBe("manual"); + expect(r?.summary).toBe( + "Authenticated interface — routing to review to confirm the declared auth scheme is documented publicly (verifiable without any secret) before it can be accepted.", + ); + }); + + it("[test 2b] closes when the debut-provider companion is valid but the entry itself is invalid", async () => { + const badEntry = { ...newEntry, public_safe: false }; + const r = await runSurfaceReview(METAGRAPHED_LANE_SPEC, { + changedFiles: [SUBNET, PROVIDER], + loadFile: (path, ref) => + Promise.resolve(path === PROVIDER ? (ref === "head" ? validProviderDoc : null) : ref === "head" ? doc([existing, badEntry]) : doc([existing])), + }); + expect(r?.verdict).toBe("close"); + }); + + // A companion file that MATCHES providerFilePattern is only trustworthy as "the debut provider" once the + // orchestrator itself confirms it's genuinely new (absent at base) — classifyRegistryPrScope's path match alone + // proves nothing about whether this provider already exists in the registry. + it("routes to MANUAL when the 'companion' provider file already exists at base (an edit, not a debut) — even though the entry itself is clean", async () => { const calls: string[] = []; const r = await runSurfaceReview(METAGRAPHED_LANE_SPEC, { changedFiles: [SUBNET, PROVIDER], loadFile: (path, ref) => { calls.push(`${ref}:${path}`); + if (path === PROVIDER) return Promise.resolve(ref === "head" ? validProviderDoc : validProviderDoc); // present at BOTH refs — an edit return Promise.resolve(ref === "head" ? doc([existing, newEntry]) : doc([existing])); }, }); + expect(r).toEqual({ + verdict: "manual", + summary: + "Registry submission's provider companion already exists in the registry — this isn't a debut provider, so it needs a human to review the edit alongside the entry.", + }); + // The entry's own content is never even assessed once the companion is confirmed non-debut (nothing else could + // change the outcome), but ALL FOUR fetches (entry head/base, provider head/base) already ran concurrently. + expect(calls.sort()).toEqual([`base:${PROVIDER}`, `base:${SUBNET}`, `head:${PROVIDER}`, `head:${SUBNET}`]); + }); + it("still merges the SAME entry when its provider companion is genuinely new (absent at base) — the debut check does not false-positive on a clean debut", async () => { + const r = await runSurfaceReview(METAGRAPHED_LANE_SPEC, { + changedFiles: [SUBNET, PROVIDER], + loadFile: (path, ref) => + Promise.resolve(path === PROVIDER ? (ref === "head" ? validProviderDoc : null) : ref === "head" ? doc([existing, newEntry]) : doc([existing])), + }); + expect(r?.verdict).toBe("merge"); + }); + + it("[test 3] merges an entry submission accompanied ONLY by an artifactPattern companion, without validating it as a provider", async () => { + const calls: string[] = []; + const r = await runSurfaceReview(METAGRAPHED_LANE_SPEC, { + changedFiles: [SUBNET, ARTIFACT], + loadFile: (path, ref) => { + calls.push(`${ref}:${path}`); + if (path === ARTIFACT) return Promise.resolve("<>"); // garbage content, never read + return Promise.resolve(ref === "head" ? doc([existing, newEntry]) : doc([existing])); + }, + }); + expect(r?.verdict).toBe("merge"); + // The artifact companion is allowed as-is (generated build output) — never loaded/validated. + expect(calls.some((c) => c.includes(ARTIFACT))).toBe(false); + }); + + it("[test 4] a companion file matching NEITHER providerFilePattern NOR artifactPattern is still mixed-files (close), unchanged", async () => { + const r = await runSurfaceReview(METAGRAPHED_LANE_SPEC, { changedFiles: [SUBNET, "src/index.ts"], loadFile: () => Promise.resolve(null) }); + expect(r?.verdict).toBe("close"); + expect(r?.summary).toContain("must not bundle other file changes"); + }); + + it("[test 5] an entry file always wins scope over an accompanying provider file — end to end, this still merges through the companion-validation path, not the old manual punt", async () => { + // registry order shouldn't matter: providerFile listed BEFORE the entry file still resolves to entry-submission + // scope with the provider as its companion (classifyRegistryPrScope's own invariant — see the registry-logic + // test suite for the scope-classification assertion itself). + const r = await runSurfaceReview(METAGRAPHED_LANE_SPEC, { + changedFiles: [PROVIDER, SUBNET], + loadFile: (path, ref) => + Promise.resolve(path === PROVIDER ? (ref === "head" ? validProviderDoc : null) : ref === "head" ? doc([existing, newEntry]) : doc([existing])), + }); + expect(r?.verdict).toBe("merge"); + }); + + it("does not recognize a companion when more than one provider file rides along an entry — falls back to manual (ambiguous shape)", async () => { + const calls: string[] = []; + const r = await runSurfaceReview(METAGRAPHED_LANE_SPEC, { + changedFiles: [SUBNET, PROVIDER, "registry/providers/second.json"], + loadFile: (path, ref) => { + calls.push(`${ref}:${path}`); + return Promise.resolve(null); + }, + }); expect(r).toEqual({ verdict: "manual", summary: "Registry submission includes companion file changes — routing to review.", }); - expect(calls).toEqual([]); + expect(calls).toEqual([]); // never even loads the direct file once an unrecognized companion trips the guard + }); + + it("routes a valid entry + companion provider file to MANUAL when the spec has no assessProviderEntry configured", async () => { + // A literal spec (rather than spreading METAGRAPHED_LANE_SPEC) so assessProviderEntry is simply OMITTED, not + // set to undefined — exactOptionalPropertyTypes rejects an explicit `undefined` for an optional field. + const spec: RegistryLaneSpec = { + entryFilePattern: SUBNET_ENTRY_PATTERN, + providerFilePattern: FLAT_PROVIDER_PATTERN, + artifactPattern: ARTIFACT_PATTERN, + collectionField: "surfaces", + maxAppendedEntries: Infinity, + duplicateKeyFields: ["url"], + assessAppendedEntry: assessSubnetDocument, + }; + const r = await runSurfaceReview(spec, { + changedFiles: [SUBNET, PROVIDER], + loadFile: (path, ref) => Promise.resolve(ref === "head" ? (path === PROVIDER ? validProviderDoc : doc([existing, newEntry])) : doc([existing])), + }); + expect(r).toEqual({ + verdict: "manual", + summary: "No validator is configured for this registry's provider submissions — routing to review.", + }); }); it("ignores blank changed-file entries while enforcing the direct-file-only invariant", async () => { diff --git a/test/unit/content-lane-registry-logic.test.ts b/test/unit/content-lane-registry-logic.test.ts index b5d2672030..81282302f1 100644 --- a/test/unit/content-lane-registry-logic.test.ts +++ b/test/unit/content-lane-registry-logic.test.ts @@ -281,10 +281,11 @@ describe("classifyRegistryPrScope (generic surface model, metagraphed spec)", () expect(isRegistrySubmissionScope(r.scope)).toBe(true); }); - it("recognizes an entry-submission whose flat debut provider is an allowed companion", () => { + it("recognizes an entry-submission whose flat debut provider is an allowed companion, and identifies it", () => { const r = classifyRegistryPrScope(spec, ["registry/subnets/allways.json", "registry/providers/allways.json"]); expect(r.scope).toBe("entry-submission"); expect(r.directFile).toBe("registry/subnets/allways.json"); + expect(r.providerCompanionFile).toBe("registry/providers/allways.json"); }); it("recognizes a standalone flat provider-submission (no subnet file)", () => { @@ -292,6 +293,35 @@ describe("classifyRegistryPrScope (generic surface model, metagraphed spec)", () expect(r.scope).toBe("provider-submission"); expect(r.directFile).toBe("registry/providers/cacheon.json"); expect(r.isProvider).toBe(true); + expect(r.providerCompanionFile).toBeNull(); + }); + + it("has no provider companion when the entry submission travels alone", () => { + const r = classifyRegistryPrScope(spec, ["registry/subnets/actual.json"]); + expect(r.scope).toBe("entry-submission"); + expect(r.providerCompanionFile).toBeNull(); + }); + + // Symmetric case explicitly documented: an entry file present alongside a provider file ALWAYS resolves to + // entry-submission scope with the provider file as its companion — there is no "provider-submission scope with + // a companion entry file" (the entry file's presence forecloses isProviderPr, which requires zero entry files). + it("an entry file always wins scope over an accompanying provider file — never provider-submission with an entry companion", () => { + const r = classifyRegistryPrScope(spec, ["registry/providers/allways.json", "registry/subnets/allways.json"]); + expect(r.scope).toBe("entry-submission"); + expect(r.isProvider).toBe(false); + expect(r.directFile).toBe("registry/subnets/allways.json"); + expect(r.providerCompanionFile).toBe("registry/providers/allways.json"); + }); + + it("does not recognize a companion when MORE THAN ONE provider file rides along an entry (ambiguous — which is 'the' debut provider?)", () => { + const r = classifyRegistryPrScope(spec, ["registry/subnets/actual.json", "registry/providers/a.json", "registry/providers/b.json"]); + expect(r.scope).toBe("entry-submission"); + expect(r.providerCompanionFile).toBeNull(); + }); + + it("mixed-files and not-direct-submission never carry a provider companion", () => { + expect(classifyRegistryPrScope(spec, ["registry/subnets/actual.json", "src/index.ts"]).providerCompanionFile).toBeNull(); + expect(classifyRegistryPrScope(spec, ["README.md"]).providerCompanionFile).toBeNull(); }); it("is mixed-files when an out-of-scope file rides along", () => { diff --git a/test/unit/content-lane-wire.test.ts b/test/unit/content-lane-wire.test.ts index b110aec6aa..7807d92794 100644 --- a/test/unit/content-lane-wire.test.ts +++ b/test/unit/content-lane-wire.test.ts @@ -139,6 +139,82 @@ describe("applySurfaceGate", () => { expect(out?.conclusion).toBe("failure"); // the real blocker means this is NOT an AI-judgment-only failure expect(out?.blockers).toEqual([aiConsensusDefect, secret]); }); + + // Bug #2 (confirmed live on metagraphed PR #2680): a duplicate_pr_risk finding (severity "warning"), escalated + // into a blocker by duplicatePrGateMode: "block", must not singlehandedly one-shot-close a PR whose OWN + // deterministic surface-lane result is a clean merge — it downgrades to a HOLD instead, same spirit as the + // AI-judgment-only carve-out above, but keyed on this EXACT finding code (isDuplicateOnlyFailure), not severity. + it("REGRESSION (#2680): a duplicate_pr_risk-only generic failure downgrades a clean surface merge to a HOLD, not a close — the finding stays visible with a held-for-review title/summary", () => { + const duplicatePrRisk: AdvisoryFinding = { + code: "duplicate_pr_risk", + title: "Linked issue overlaps another open PR", + severity: "warning", + detail: "Other open pull requests reference the same linked issue set: #2654.", + }; + const genericDuplicateOnly = gate({ conclusion: "failure", blockers: [duplicatePrRisk], warnings: [] }); + const surfaceMerge = gate({ conclusion: "success", title: "Surface", summary: "valid entry" }); + const out = applySurfaceGate(genericDuplicateOnly, surfaceMerge); + expect(out?.conclusion).toBe("neutral"); // held for review, not closed + expect(out?.blockers).toEqual([]); // no longer a hard blocker + expect(out?.warnings).toEqual([duplicatePrRisk]); // still visible to a human reviewer + // The posted title/summary must name the actual hold reason, not silently inherit the surface's clean-merge text. + expect(out?.title).toMatch(/held for review/i); + expect(out?.summary).toBe(duplicatePrRisk.detail); + }); + + it("the held-for-review summary falls back to the blocker's title when its detail is blank", () => { + const duplicatePrRisk: AdvisoryFinding = { code: "duplicate_pr_risk", title: "Linked issue overlaps another open PR", severity: "warning", detail: "" }; + const genericDuplicateOnly = gate({ conclusion: "failure", blockers: [duplicatePrRisk], warnings: [] }); + const surfaceMerge = gate({ conclusion: "success", title: "Surface", summary: "valid entry" }); + const out = applySurfaceGate(genericDuplicateOnly, surfaceMerge); + expect(out?.summary).toBe(duplicatePrRisk.title); + }); + + it("a duplicate-only generic failure downgrade preserves the generic gate's OTHER pre-existing warnings alongside the demoted blocker", () => { + const duplicatePrRisk: AdvisoryFinding = { code: "duplicate_pr_risk", title: "Duplicate", severity: "warning", detail: "Overlaps #99." }; + const readiness: AdvisoryFinding = { code: "quality_readiness_low", title: "Readiness is low", severity: "warning", detail: "" }; + const genericDuplicateOnly = gate({ conclusion: "failure", blockers: [duplicatePrRisk], warnings: [readiness] }); + const surfaceMerge = gate({ conclusion: "success", title: "Surface", summary: "valid entry", warnings: [] }); + const out = applySurfaceGate(genericDuplicateOnly, surfaceMerge); + expect(out?.conclusion).toBe("neutral"); + expect(out?.warnings).toEqual([duplicatePrRisk, readiness]); + }); + + it("a MIXED generic failure (duplicate_pr_risk plus a genuinely critical finding) is NOT duplicate-only — still overrides a surface merge", () => { + const duplicatePrRisk: AdvisoryFinding = { code: "duplicate_pr_risk", title: "Duplicate", severity: "warning", detail: "" }; + const secret: AdvisoryFinding = { code: "secret_leak", title: "Secret", severity: "critical", detail: "leaked key" }; + const genericMixed = gate({ conclusion: "failure", blockers: [duplicatePrRisk, secret], warnings: [] }); + const surfaceMerge = gate({ conclusion: "success", title: "Surface", summary: "valid entry" }); + const out = applySurfaceGate(genericMixed, surfaceMerge); + expect(out?.conclusion).toBe("failure"); // the real secret means this is NOT a duplicate-only failure + expect(out?.blockers).toEqual([duplicatePrRisk, secret]); + }); + + it("a duplicate-only generic failure still fails when the surface ITSELF closes (the downgrade only applies to a surface MERGE)", () => { + const duplicatePrRisk: AdvisoryFinding = { code: "duplicate_pr_risk", title: "Duplicate", severity: "warning", detail: "" }; + const genericDuplicateOnly = gate({ conclusion: "failure", blockers: [duplicatePrRisk], warnings: [] }); + const out = applySurfaceGate(genericDuplicateOnly, surfaceClose); + expect(out?.conclusion).toBe("failure"); + expect(out?.blockers).toEqual([duplicatePrRisk, ...surfaceClose.blockers]); // union — no downgrade without a surface merge + }); + + // REGRESSION (scope-creep guard): several OTHER findings are ALSO severity "warning" and ALSO block-mode- + // escalatable via their OWN independent maintainer-configured gate (linkedIssueGateMode / + // selfAuthoredLinkedIssueGateMode / manifestPolicyGateMode). Guard #4 must be scoped to EXACTLY duplicate_pr_risk + // — a maintainer who explicitly opted one of these into "block" must still have it close a clean-surface PR + // outright, not get silently downgraded to a hold just because the finding happens to share duplicate_pr_risk's + // "warning" severity. + it.each(["missing_linked_issue", "self_authored_linked_issue", "manifest_linked_issue_required", "manifest_missing_tests"])( + "REGRESSION: a %s-only generic failure (also severity warning, but a DIFFERENT maintainer-configured gate) still overrides a surface merge — not swept into the duplicate-only carve-out", + (code) => { + const otherWarningBlocker: AdvisoryFinding = { code, title: "t", severity: "warning", detail: "d" }; + const genericOtherOnly = gate({ conclusion: "failure", blockers: [otherWarningBlocker], warnings: [] }); + const surfaceMerge = gate({ conclusion: "success", title: "Surface", summary: "valid entry" }); + const out = applySurfaceGate(genericOtherOnly, surfaceMerge); + expect(out?.conclusion).toBe("failure"); + expect(out?.blockers).toEqual([otherWarningBlocker]); + }, + ); }); describe("runRegistrySurfaceGate (injected loader — adapter logic)", () => { diff --git a/test/unit/rules.test.ts b/test/unit/rules.test.ts index 9941d47e9e..453d1bbd52 100644 --- a/test/unit/rules.test.ts +++ b/test/unit/rules.test.ts @@ -10,6 +10,7 @@ import { formatCheckRunOutput, formatGateCheckOutput, isAiJudgmentOnlyFailure, + isDuplicateOnlyFailure, reconcileGateEvaluationForGreenCi, } from "../../src/rules/advisory"; import type { CollisionReport } from "../../src/signals/engine"; @@ -1181,6 +1182,26 @@ describe("green-CI compatibility reconciliation of the public comment gate", () expect(isAiJudgmentOnlyFailure({ ...failure(["ai_consensus_defect"]), conclusion: "success" })).toBe(false); }); + it("isDuplicateOnlyFailure: true only when EVERY blocker is EXACTLY duplicate_pr_risk", () => { + expect(isDuplicateOnlyFailure(failure(["duplicate_pr_risk"]))).toBe(true); + // An empty blocker list is not a duplicate-only failure. + expect(isDuplicateOnlyFailure({ ...failure([]), conclusion: "failure" })).toBe(false); + // A non-failure conclusion is never a duplicate-only failure. + expect(isDuplicateOnlyFailure({ ...failure(["duplicate_pr_risk"]), conclusion: "success" })).toBe(false); + // A single genuinely critical blocker (e.g. a committed secret) disqualifies the whole set, mixed or alone. + expect(isDuplicateOnlyFailure(failure(["secret_leak"]))).toBe(false); + expect(isDuplicateOnlyFailure(failure(["duplicate_pr_risk", "secret_leak"]))).toBe(false); + // REGRESSION: other findings are ALSO severity "warning" and ALSO block-mode-escalatable via their own + // independent maintainer-configured gate (linkedIssueGateMode / selfAuthoredLinkedIssueGateMode / + // manifestPolicyGateMode) — a scope-creep bug would let applySurfaceGate silently downgrade THOSE deliberate + // block-mode opt-ins to a hold too. None of them may ever satisfy isDuplicateOnlyFailure, alone or mixed with + // duplicate_pr_risk. + for (const code of ["missing_linked_issue", "self_authored_linked_issue", "manifest_linked_issue_required", "manifest_missing_tests"]) { + expect(isDuplicateOnlyFailure(failure([code]))).toBe(false); + expect(isDuplicateOnlyFailure(failure(["duplicate_pr_risk", code]))).toBe(false); + } + }); + it("enabled + green CI + AI-judgment-only failure stays a failure", () => { const fail = failure(["ai_consensus_defect"]); expect(reconcileGateEvaluationForGreenCi(fail, "passed", true)).toBe(fail);