From a83fe439288e04e3563c709589b08c26309ace22 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 2 Jul 2026 03:40:12 -0700 Subject: [PATCH 1/2] fix(review): stop the surface lane from auto-closing clean registry submissions Two bugs in the registry surface-review lane were closing structurally clean registry submissions: 1. classifyRegistryPrScope already approves a genuine debut-provider companion file riding alongside an entry submission (isAllowed matches providerFilePattern), but runSurfaceReview threw that away and routed any companion file straight to manual review. It now validates the companion via the spec's assessProviderEntry and combines it with the entry's own assessment: merge only when both sides are clean, close if either is invalid. artifactPattern companions (generated build output) are still allowed as-is with no validation attempted. The entry/base/companion fetches run concurrently instead of sequentially. 2. A duplicate_pr_risk finding (severity "warning") escalated into a hard blocker by duplicatePrGateMode: "block" was able to singlehandedly override a clean, deterministic surface-lane merge and force the whole PR closed via applySurfaceGate's unconditional union. It now downgrades to a neutral hold instead, mirroring the existing AI-judgment-only carve-out, and the held check-run's title/summary name the actual reason. This is scoped to exactly duplicate_pr_risk (not every warning-severity finding), since missing_linked_issue / self_authored_linked_issue / manifest_linked_issue_required / manifest_missing_tests are also warning-severity but block-mode-escalatable via their own independent maintainer-configured gate and must still close outright when a maintainer opts into that. Both were confirmed live against JSONbored/metagraphed: PR #2654 (an entry + debut-provider companion) and PR #2680 (a provider-only resubmission sharing #2654's linked issue) now resolve to merge and a held-for-review neutral hold respectively, instead of reject/close. --- src/review/content-lane-wire.ts | 46 +++++++- src/review/content-lane/orchestrator.ts | 63 +++++++++- src/review/content-lane/registry-logic.ts | 36 ++++-- src/rules/advisory.ts | 23 ++++ test/unit/content-lane-orchestrator.test.ts | 109 +++++++++++++++++- test/unit/content-lane-registry-logic.test.ts | 32 ++++- test/unit/content-lane-wire.test.ts | 76 ++++++++++++ test/unit/rules.test.ts | 21 ++++ 8 files changed, 383 insertions(+), 23 deletions(-) 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..e9a5c9a559 100644 --- a/src/review/content-lane/orchestrator.ts +++ b/src/review/content-lane/orchestrator.ts @@ -8,7 +8,10 @@ // 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 genuine debut-provider companion file (an "entry + its first +// provider in the same PR" contribution), also validates that companion 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 { @@ -96,6 +99,34 @@ function duplicateEntryCloseSummary(): string { 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."; +/** + * 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) and `assessProvider` + * already confirmed present, so this function does no I/O and can't itself punt to "no validator configured". + * 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 * (of the remainder) needs manual review (e.g. auth_required), merge only if EVERY entry is clean — mirroring the @@ -147,20 +178,39 @@ 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 fetch (when present) are up to three independent + // GitHub-Contents reads with no data dependency on each other — resolve them concurrently rather than paying + // for three sequential round-trips. + const [headRaw, baseRaw, providerRaw] = await Promise.all([ + input.loadFile(directFile, "head"), + input.loadFile(directFile, "base"), + companionProviderFile !== null ? input.loadFile(companionProviderFile, "head") : Promise.resolve(null), + ]); 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 +229,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, providerRaw, 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..0159a817f7 100644 --- a/src/review/content-lane/registry-logic.ts +++ b/src/review/content-lane/registry-logic.ts @@ -702,6 +702,13 @@ export interface RegistryScopeResult { scope: RegistryPrScope; directFile: string | null; isProvider: boolean; + /** For an "entry-submission" scope only: a companion file that is a genuine DEBUT provider submission riding + * along with the entry in the same PR (spec.providerFilePattern), set only when exactly one such companion is + * present. 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 +722,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..7004e5ffc1 100644 --- a/test/unit/content-lane-orchestrator.test.ts +++ b/test/unit/content-lane-orchestrator.test.ts @@ -94,21 +94,124 @@ 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 () => { - const calls: string[] = []; + // 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"); + }); + + 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); From 304d945440fcefb5644eaa2edd401b431045f1c7 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 2 Jul 2026 03:56:13 -0700 Subject: [PATCH 2/2] fix(review): confirm a provider companion is actually a debut before merging it classifyRegistryPrScope identifies a companion by file path alone (providerFilePattern), which 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 registered). runSurfaceReview now also fetches the companion's base content and only runs it through the debut-provider merge/close flow when base is absent; a companion that already exists at base routes to manual instead, since editing an existing, unrelated provider record alongside an entry submission is a more sensitive shape that needs a human. The entry and both companion refs are still fetched in one concurrent round-trip. --- src/review/content-lane/orchestrator.ts | 37 +++++++++++++++------ src/review/content-lane/registry-logic.ts | 16 +++++---- test/unit/content-lane-orchestrator.test.ts | 32 ++++++++++++++++++ 3 files changed, 69 insertions(+), 16 deletions(-) diff --git a/src/review/content-lane/orchestrator.ts b/src/review/content-lane/orchestrator.ts index e9a5c9a559..1063a53477 100644 --- a/src/review/content-lane/orchestrator.ts +++ b/src/review/content-lane/orchestrator.ts @@ -9,9 +9,10 @@ // 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, and -// 6. for an entry submission riding alongside a genuine debut-provider companion file (an "entry + its first -// provider in the same PR" contribution), also validates that companion via assessProviderEntry and combines -// it with the entry's own result — merge only when BOTH sides are clean. +// 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 { @@ -98,12 +99,19 @@ 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) and `assessProvider` - * already confirmed present, so this function does no I/O and can't itself punt to "no validator configured". + * 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 @@ -203,14 +211,23 @@ export async function runSurfaceReview(spec: RegistryLaneSpec, input: SurfaceRev if (companionProviderFile !== null && !spec.assessProviderEntry) { return { verdict: "manual", summary: NO_VALIDATOR_PROVIDER_SUMMARY }; } - // The entry's head/base fetch and the companion provider's fetch (when present) are up to three independent - // GitHub-Contents reads with no data dependency on each other — resolve them concurrently rather than paying - // for three sequential round-trips. - const [headRaw, baseRaw, providerRaw] = await Promise.all([ + // 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) { @@ -232,7 +249,7 @@ export async function runSurfaceReview(spec: RegistryLaneSpec, input: SurfaceRev 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, providerRaw, input.opts); + 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 0159a817f7..7296d6b87d 100644 --- a/src/review/content-lane/registry-logic.ts +++ b/src/review/content-lane/registry-logic.ts @@ -702,12 +702,16 @@ export interface RegistryScopeResult { scope: RegistryPrScope; directFile: string | null; isProvider: boolean; - /** For an "entry-submission" scope only: a companion file that is a genuine DEBUT provider submission riding - * along with the entry in the same PR (spec.providerFilePattern), set only when exactly one such companion is - * present. 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. */ + /** 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; } diff --git a/test/unit/content-lane-orchestrator.test.ts b/test/unit/content-lane-orchestrator.test.ts index 7004e5ffc1..1135baf60b 100644 --- a/test/unit/content-lane-orchestrator.test.ts +++ b/test/unit/content-lane-orchestrator.test.ts @@ -143,6 +143,38 @@ describe("runSurfaceReview (deterministic + decisive: merge/close, rarely manual 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, {