fix(review): stop the surface lane from auto-closing clean registry submissions

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",

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<RegistryLaneSpec["assessProviderEntry"]>,
+  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<RegistryLaneSpec["assessProviderEntry"]>;
+    return assessEntryWithProviderCompanion(assessProvider, assessment, providerHeadRaw, input.opts);
+  }
   return { verdict: toCoreVerdict(assessment.verdict), summary: assessment.summary, reason: assessment.reason };
 }

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 {