diff --git a/src/github/backfill.ts b/src/github/backfill.ts index cbc18c8d33..d86a5d29a6 100644 --- a/src/github/backfill.ts +++ b/src/github/backfill.ts @@ -3279,6 +3279,36 @@ export async function fetchLiveBaseBranchAdvancedAt( return result?.data.commit?.committer?.date ?? undefined; } +/** + * How many commits the repo's CURRENT default branch has landed since this PR's own base commit, via REST + * `GET /compare/{baseSha}...{defaultBranchRef}` (`ahead_by` from `baseSha`'s perspective — #review-grounding + * stale-base fact). `mergeable_state: "behind"` (the signal `prReadyForReview`'s auto-rebase-before-review path + * already uses) only ever fires when the repo's branch protection has "require branches to be up to date before + * merging" enabled — a repo without that setting can have a branch genuinely dozens of commits behind and GitHub + * will still never report it as "behind". This compare-API read is unconditional: it works regardless of branch + * protection config, so it can ground the AI reviewer in the TRUE fact even on a repo where the mergeable_state + * signal never fires. Best-effort: any fetch/shape error returns undefined so the caller degrades to "unknown" + * (no stale-base fact rendered) rather than throwing or asserting a wrong number. + */ +export async function fetchBaseAheadBy( + env: Env, + repoFullName: string, + baseSha: string, + defaultBranchRef: string, + token: string | undefined, + admissionKey?: GitHubRateLimitAdmissionKey, +): Promise { + const result = await githubJsonWithHeaders<{ ahead_by?: number | null }>( + env, + repoFullName, + `/compare/${encodeURIComponent(baseSha)}...${encodeURIComponent(defaultBranchRef)}`, + token, + githubRateLimitOptions(admissionKey), + ).catch(() => undefined); + const aheadBy = result?.data.ahead_by; + return typeof aheadBy === "number" && Number.isFinite(aheadBy) ? aheadBy : undefined; +} + /** The PR's LIVE state ("open" / "closed") via REST `GET /pulls/{n}`. The stored open-PR cache lags GitHub, so a * sibling closed/merged on GitHub can still read `open` locally; the duplicate-winner election (#dup-winner / * audit #15) confirms a lower sibling's live state before treating this PR as a cluster loser. Best-effort: diff --git a/src/queue/ai-review-orchestration.ts b/src/queue/ai-review-orchestration.ts index cff1be99c1..a3867e94c5 100644 --- a/src/queue/ai-review-orchestration.ts +++ b/src/queue/ai-review-orchestration.ts @@ -535,19 +535,25 @@ export async function runAiReviewForAdvisory( // byte-identical to today. Fully fail-safe. const grounding = groundingActive - ? await buildReviewGroundingText(env, { - repoFullName: args.repoFullName, - headSha: args.advisory.headSha, - files, - checks: await listCheckSummaries( - env, - args.repoFullName, - args.pr.number, - ), - installationId: - (await getRepository(env, args.repoFullName))?.installationId ?? - null, - }) + ? await (async () => { + const repo = await getRepository(env, args.repoFullName); + return buildReviewGroundingText(env, { + repoFullName: args.repoFullName, + headSha: args.advisory.headSha, + files, + checks: await listCheckSummaries( + env, + args.repoFullName, + args.pr.number, + ), + installationId: repo?.installationId ?? null, + // #review-grounding stale-base fact (metagraphed #7305-class incident): both are additive — either + // absent (no baseSha on a rare malformed webhook record, or an unregistered repo with no stored + // defaultBranch) just skips the BASE BRANCH STATUS fact, same as before it existed. + baseSha: args.pr.baseSha, + defaultBranchRef: repo?.defaultBranch, + }); + })() : undefined; // RAG retrieval (convergence, flag-gated by LOOPOVER_REVIEW_RAG). Query the codebase vector index for code/docs // semantically related to the changed files and append them as additive reference context — exactly like diff --git a/src/review/content-lane/orchestrator.ts b/src/review/content-lane/orchestrator.ts index 9c352b04d6..110e1c3e8b 100644 --- a/src/review/content-lane/orchestrator.ts +++ b/src/review/content-lane/orchestrator.ts @@ -55,9 +55,11 @@ function surfacesOf(doc: unknown, field: string): unknown[] | null { /** * ALL surfaces[] entries present at head but absent at base — a pure head-vs-base structural diff. Returns null * when head is unreadable / has no surfaces[] array; returns an empty array when nothing was added (a - * reorder/reformat/edit of existing entries reads as zero "added"). A missing base file (a brand-new entry file) - * means every head entry is new. Makes no count judgement itself — the caller (runSurfaceReview) enforces the - * spec's maxAppendedEntries cap and the ≥1-entry requirement. + * byte-identical reorder reads as zero "added" — an actual FIELD EDIT to an existing entry does NOT: the edited + * entry's new content differs from every base entry, so it reads as one "added" entry, same as a brand-new + * append — see survivingExistingEntries below for how the caller tells the two apart before duplicate-checking). + * A missing base file (a brand-new entry file) means every head entry is new. Makes no count judgement itself — + * the caller (runSurfaceReview) enforces the spec's maxAppendedEntries cap and the ≥1-entry requirement. */ export function diffAppendedSurfaceEntries(headRaw: string | null, baseRaw: string | null, field: string): unknown[] | null { const headEntries = surfacesOf(safeParseJson(headRaw), field); @@ -67,6 +69,34 @@ export function diffAppendedSurfaceEntries(headRaw: string | null, baseRaw: stri return headEntries.filter((entry) => !baseKeys.has(JSON.stringify(entry))); } +/** + * Base surfaces[] entries that are STILL PRESENT, byte-identical, in the head document — i.e. entries this PR + * left completely untouched. Feeds findDuplicateAppendedEntry's `existingEntries` argument so a submission's + * duplicate check only ever collides against an entry that genuinely still occupies that identity in the + * registry, not one this very PR just edited away. + * + * Why this matters: an in-place edit (e.g. tightening `probe.expect` on an already-registered surface, keeping + * its url) makes diffAppendedSurfaceEntries read the edited entry as "added" (its new content differs from every + * base entry — see that function's own doc comment), and the edited entry's identity key (typically its url) + * still matches its OWN prior self in base. Passing the raw base surfaces[] array as `existingEntries` (the + * pre-fix behavior) makes an entry collide with its own now-superseded version and reads as a resubmitted + * duplicate, closing every legitimate "fix an existing surface" PR outright regardless of content correctness. + * Filtering to only the base entries that SURVIVE into head fixes this: an edited entry's old self is gone from + * head (replaced in place), so it is excluded here and can no longer collide with the edit. A genuine duplicate + * resubmission is unaffected — the untouched original entry remains in head, stays in this filtered set, and + * still collides with the newly appended entry sharing its identity, so that case still closes as before. + * + * Returns [] when head is unreadable (mirrors diffAppendedSurfaceEntries' own null-safety); the orchestrator only + * reaches this after diffAppendedSurfaceEntries has already confirmed head parses with a surfaces[] array. + */ +export function survivingExistingEntries(headRaw: string | null, baseRaw: string | null, field: string): unknown[] { + const headEntries = surfacesOf(safeParseJson(headRaw), field); + if (headEntries === null) return []; + const baseEntries = surfacesOf(safeParseJson(baseRaw), field) ?? []; + const headKeys = new Set(headEntries.map((entry) => JSON.stringify(entry))); + return baseEntries.filter((entry) => headKeys.has(JSON.stringify(entry))); +} + function fromProvider(assessment: ProviderAssessment): SurfaceReviewResult { // Decisive: a valid provider merges; an invalid one CLOSES (resubmit clean) — never a manual punt. return assessment.ok @@ -237,7 +267,9 @@ export async function runSurfaceReview(spec: RegistryLaneSpec, input: SurfaceRev if (appendedEntries === null || appendedEntries.length === 0 || appendedEntries.length > maxAppendedEntries) { return { verdict: "close", summary: appendCountCloseSummary(maxAppendedEntries) }; } - const existingEntries = surfacesOf(safeParseJson(baseRaw), spec.collectionField) ?? []; + // survivingExistingEntries, not the raw base array — an entry this PR edited in place must not collide with + // its own now-superseded prior self (see that function's doc comment for the full false-positive it fixes). + const existingEntries = survivingExistingEntries(headRaw, baseRaw, spec.collectionField); const duplicate = findDuplicateAppendedEntry(spec, appendedEntries, existingEntries); if (duplicate !== null) { return { verdict: "close", summary: duplicateEntryCloseSummary() }; diff --git a/src/review/grounding-wire.ts b/src/review/grounding-wire.ts index 878e2b9714..b2a5ecf481 100644 --- a/src/review/grounding-wire.ts +++ b/src/review/grounding-wire.ts @@ -12,6 +12,7 @@ // fail-safe: any missing CI data / fetch error degrades to "no grounding" and the review proceeds on the diff. import { createInstallationToken } from "../github/app"; +import { fetchBaseAheadBy } from "../github/backfill"; import { githubRateLimitAdmissionKeyForToken, PRODUCT_USER_AGENT, timeoutFetch, type GitHubRateLimitAdmissionKey } from "../github/client"; import { getCachedGroundingFileContent, putCachedGroundingFileContent, recordAuditEvent } from "../db/repositories"; import type { CheckSummaryRecord, PullRequestFileRecord } from "../types"; @@ -121,6 +122,27 @@ function toGroundingFiles(files: PullRequestFileRecord[]): PullRequestFile[] { }); } +/** + * Resolve (best-effort) the token to authenticate a grounding-wire GitHub read with: installation token > public + * token > none. `admissionKey` is derived from the FINAL token (#regression-safe-propagation), after the + * public-token fallback is applied -- computing it before that fallback (against the pre-fallback + * `installationId`-only branch) left every fallback call with `admissionKey: undefined` even though the actual + * token used (`GITHUB_PUBLIC_TOKEN`) has a perfectly nameable scope, silently dropping every such call into + * `key_scope="unknown"` on any rate-limited response. Shared by makeGithubFileFetcher's Contents-API reads and + * the base-branch staleness read (#review-grounding stale-base fact) so both authenticate identically without + * duplicating the fallback logic. + */ +async function resolveGroundingToken( + env: Env, + installationId: number | null | undefined, +): Promise<{ token: string | undefined; admissionKey: GitHubRateLimitAdmissionKey | undefined }> { + let token: string | undefined; + if (installationId) token = await createInstallationToken(env, installationId).catch(() => undefined); + token = token ?? env.GITHUB_PUBLIC_TOKEN; + const admissionKey: GitHubRateLimitAdmissionKey | undefined = githubRateLimitAdmissionKeyForToken(env, token, installationId); + return { token, admissionKey }; +} + /** * A {@link FileFetcher} backed by the GitHub Contents API. Authenticates with an installation token (so it * reads private repos), falling back to the public token, then to unauthenticated. Returns the raw file text, @@ -128,15 +150,7 @@ function toGroundingFiles(files: PullRequestFileRecord[]): PullRequestFile[] { * treats null as "skip this file" and degrades to no-grounding when nothing is readable. */ export async function makeGithubFileFetcher(env: Env, repoFullName: string, installationId: number | null | undefined): Promise { - // Resolve the token once (best-effort): installation token > public token > none. `admissionKey` is derived - // from the FINAL token (#regression-safe-propagation), after the public-token fallback is applied -- computing - // it before that fallback (against the pre-fallback `installationId`-only branch) left every fallback call - // with `admissionKey: undefined` even though the actual token used (`GITHUB_PUBLIC_TOKEN`) has a perfectly - // nameable scope, silently dropping every such call into `key_scope="unknown"` on any rate-limited response. - let token: string | undefined; - if (installationId) token = await createInstallationToken(env, installationId).catch(() => undefined); - token = token ?? env.GITHUB_PUBLIC_TOKEN; - const admissionKey: GitHubRateLimitAdmissionKey | undefined = githubRateLimitAdmissionKeyForToken(env, token, installationId); + const { token, admissionKey } = await resolveGroundingToken(env, installationId); const { owner, name } = repoParts(repoFullName); return { async getFileContent(path: string, ref: string, maxChars = 24_001): Promise { @@ -259,6 +273,12 @@ export async function buildReviewGroundingText( files: PullRequestFileRecord[]; checks: CheckSummaryRecord[]; installationId: number | null | undefined; + // #review-grounding stale-base fact (metagraphed #7305-class incident): when both are readable, an + // additional BASE BRANCH STATUS fact is folded into the SAME ciGrounding-gated section so an undetailed CI + // failure has a true, deterministic explanation available instead of an unverified guess. Either absent ⇒ + // this fact is simply skipped (byte-identical to before it existed) — it is additive, never required. + baseSha?: string | null | undefined; + defaultBranchRef?: string | null | undefined; }, ): Promise { const flags = groundingFlags(env); @@ -267,7 +287,16 @@ export async function buildReviewGroundingText( const aggregate = buildCheckAggregate(args.checks); const fetcher = await makeGithubFileFetcher(env, args.repoFullName, args.installationId); const fileContents = await fetchFullFileContents(flags, args.headSha ?? undefined, toGroundingFiles(args.files), fetcher); - const grounding = buildGrounding(flags, aggregate, fileContents); + const baseSha = args.baseSha; + const defaultBranchRef = args.defaultBranchRef; + const baseAheadBy = + flags.ciGrounding && baseSha && defaultBranchRef + ? await (async () => { + const { token, admissionKey } = await resolveGroundingToken(env, args.installationId); + return fetchBaseAheadBy(env, args.repoFullName, baseSha, defaultBranchRef, token, admissionKey); + })() + : undefined; + const grounding = buildGrounding(flags, aggregate, fileContents, baseAheadBy); const promptSection = formatGroundingSections(grounding); // Only attach the grounding-discipline system suffix when we actually produced grounding to verify // against; otherwise the prompt stays unchanged (no point telling the model to "check the file" with diff --git a/src/review/review-grounding.ts b/src/review/review-grounding.ts index 2f59051690..9f21581e75 100644 --- a/src/review/review-grounding.ts +++ b/src/review/review-grounding.ts @@ -54,6 +54,11 @@ export interface PullRequestFile { export interface ReviewGrounding { checks?: ReviewCiSummary; changedFileContents?: ChangedFileContent[]; + /** How many commits the repo's CURRENT default branch has landed since this PR's own base commit (#review- + * grounding stale-base fact, metagraphed #7305-class incident) — a TRUE, deterministic fact the reviewer can + * cite instead of guessing a content-level cause for an undetailed CI failure. Undefined when unreadable or + * zero (nothing to say); the caller only sets this when it is a positive number worth surfacing. */ + baseAheadBy?: number; } // Budgets so the full-file block fits the 120B context alongside the diff + RAG + project knowledge. @@ -84,6 +89,8 @@ const GROUNDING_GUIDANCE = [ "", "GROUNDING — verify every concern against the provided reality before raising it (you are a smaller model; do not guess):", "- CI has ALREADY finished on this commit; its results are given below as 'CI STATUS'. NEVER predict a CI / build / typecheck / test outcome. If a check is under PASSED, that path is verified — do not claim the change breaks it. Treat something as a CI failure ONLY if it appears under FAILED.", + "- A FAILED check marked '(no detail provided)' means you were given only its name, not its actual error output — you cannot know WHY it failed. Do not fill that gap with a guess. Writing something 'likely' failed for a specific content reason, or naming an example cause 'not visible in this diff', is STILL an unverified guess wearing a hedge — it is FORBIDDEN as a blocker, exactly like asserting a defect on a file you cannot see. State plainly that the check failed and its cause could not be verified from what you were given; do not name any hypothetical cause, hedged or not.", + "- If a 'BASE BRANCH STATUS' section is present below, this PR's branch is a KNOWN, measured number of commits behind the default branch. For an undetailed FAILED check, prefer citing that TRUE fact as the likely cause (and suggest rebasing onto the latest default branch) over guessing a content-level defect — this is a verified fact, not a guess, so it is the correct thing to say instead of staying silent about the cause.", "- The FULL post-change content of the changed files is given below as 'FULL FILE CONTENT'. Before claiming any symbol, import, type, or export is undefined / unused / missing / wrong-signature, CHECK that file — only flag it if it is genuinely absent there.", "- If verifying a concern needs a file that is NOT provided, say you could not verify it; do NOT assert a defect on code you cannot see.", ].join("\n"); @@ -107,11 +114,16 @@ export function toCiSummary(all: CheckAggregate): ReviewCiSummary { } /** Assemble the grounding the prompt renders from a lane's ALREADY-fetched CI (`checks`) + the centrally - * fetched full file contents (`fileContents`), each gated by its flag. No I/O — pure. */ -export function buildGrounding(f: GroundingFlags, checks?: CheckAggregate, fileContents?: ChangedFileContent[]): ReviewGrounding { + * fetched full file contents (`fileContents`) + the base-branch staleness fact (`baseAheadBy`), each gated by + * its flag. `baseAheadBy` rides the SAME `ciGrounding` flag as `checks` and additionally requires `checks` to + * be present too — it explains a CI STATUS section (formatBaseBranchSection reads "see CI STATUS above"), so + * rendering it with no CI section to point at would dangle. Only included when a positive number (0/undefined + * ⇒ nothing worth telling the reviewer). No I/O — pure. */ +export function buildGrounding(f: GroundingFlags, checks?: CheckAggregate, fileContents?: ChangedFileContent[], baseAheadBy?: number): ReviewGrounding { return { ...(f.ciGrounding && checks ? { checks: toCiSummary(checks) } : {}), ...(f.fullFileContext && fileContents?.length ? { changedFileContents: fileContents } : {}), + ...(f.ciGrounding && checks && typeof baseAheadBy === "number" && baseAheadBy > 0 ? { baseAheadBy } : {}), }; } @@ -244,14 +256,31 @@ export function formatGroundingSections(g?: ReviewGrounding): string { if (!g) return ""; const parts: string[] = []; if (g.checks) parts.push(formatCiSection(g.checks)); + if (typeof g.baseAheadBy === "number" && g.baseAheadBy > 0) parts.push(formatBaseBranchSection(g.baseAheadBy)); if (g.changedFileContents?.length) parts.push(formatFilesSection(g.changedFileContents)); return parts.join("\n\n"); } +/** BASE BRANCH STATUS section (metagraphed #7305-class incident): a TRUE, deterministic fact — this PR's base + * commit is measurably behind the repo's current default branch — that the reviewer can cite as the likely + * cause of an undetailed CI failure instead of guessing a content-level defect. Only rendered when the caller + * supplied a positive count (see buildGrounding). */ +function formatBaseBranchSection(aheadBy: number): string { + return [ + "BASE BRANCH STATUS:", + `- This PR's branch is based on a commit that is ${aheadBy} commit${aheadBy === 1 ? "" : "s"} behind the repository's current default branch.`, + "- A CI failure (see CI STATUS above) on a branch this far behind is frequently caused by code that landed on the default branch AFTER this PR's branch diverged — not a defect in this PR's own changes.", + ].join("\n"); +} + function formatCiSection(c: ReviewCiSummary): string { if (c.state === "pending") return "CI STATUS: checks still running on this commit — do not assume an outcome."; const passed = c.passing.length ? c.passing.join(", ") : "(none)"; - const failed = c.failing.length ? c.failing.map((x) => (x.summary ? `${x.name} — ${x.summary}` : x.name)).join("; ") : "(none)"; + // A failing check with no `summary` means the caller never got the check's own error output (e.g. a generic + // CI runner's check-run carries no output.title/summary beyond pass/fail) — mark that gap explicitly, in-line + // next to the fact itself, rather than relying on the model to remember a rule stated once in the system + // prompt. This is what GROUNDING_GUIDANCE's "(no detail provided)" instruction below reacts to. + const failed = c.failing.length ? c.failing.map((x) => (x.summary ? `${x.name} — ${x.summary}` : `${x.name} (no detail provided)`)).join("; ") : "(none)"; const verdict = c.state === "passed" ? "ALL checks PASSED — the build/typecheck/tests already succeeded on this exact commit." : "Some checks FAILED."; return ["CI STATUS (already finished on this commit — do NOT predict CI):", `- ${verdict}`, `- PASSED: ${passed}`, `- FAILED: ${failed}`].join("\n"); } diff --git a/src/review/unified-comment-bridge.ts b/src/review/unified-comment-bridge.ts index e1e6cc98a3..9fbcbc6eda 100644 --- a/src/review/unified-comment-bridge.ts +++ b/src/review/unified-comment-bridge.ts @@ -167,6 +167,28 @@ function holdWarningVerdictReason(finding: AdvisoryFinding): string { return detail.length > 0 ? `${title}: ${detail}` : title; } +/** Human-facing text for a gate HARD blocker finding: `title` alone when there's nothing more specific to add, + * or `title: detail` when detail says something the title doesn't already (mirrors holdWarningVerdictReason's + * shape immediately above, now applied to blockers too). Prefers `publicText` over `detail` when a producer set + * both (the same preference every other public-facing reader of AdvisoryFinding uses, e.g. local-branch.ts / + * github/commands.ts) — `publicSafeNit` below still scrubs the result either way as defense-in-depth. + * + * FIX D-detail: before this, gateBlockerLines rendered ONLY `finding.title`, dropping `detail`/`publicText` + * entirely for every hard-blocker close. Several finding producers deliberately give EVERY verdict from a given + * check the SAME constant title (e.g. the content lane's surface findings are all titled "Registry surface + * review" regardless of whether the actual reason is a duplicate resubmission, an unsafe URL, or a secret) so + * the check-run itself stays identifiable — the verdict-specific explanation lives entirely in detail/publicText. + * Title-only rendering made "Why this is blocked" read identically generic across every distinct close reason, + * hiding the one thing a contributor (or a maintainer auditing a close) actually needs to see. + * `.action`, when present, is still appended as its own suffix — a distinct call-to-action some finding + * producers set independently of detail/publicText. */ +function gateBlockerLine(finding: AdvisoryFinding): string { + const title = finding.title.trim(); + const reason = (finding.publicText ?? finding.detail).trim(); + const base = reason.length > 0 && reason !== title ? `${title}: ${reason}` : title; + return `${base}${finding.action ? ` — ${finding.action}` : ""}`.trim(); +} + function gateVerdictReason(gate: GateCheckEvaluation): string | undefined { const holdReasons = gate.warnings .filter((finding) => MANUAL_HOLD_WARNING_CODES.has(finding.code)) @@ -238,7 +260,7 @@ export function buildDualReviewNotes(args: { // and scrub each through the same public-safe boundary as Nits, DROPPING any that still leaks a private term. const gateBlockerLines = (args.gateBlockers ?? []) .filter((finding) => finding.code !== "ai_consensus_defect") - .map((finding) => `${finding.title}${finding.action ? ` — ${finding.action}` : ""}`.trim()) + .map(gateBlockerLine) .filter(Boolean) .map((line) => publicSafeNit(line)) .filter((line): line is string => line !== null); diff --git a/test/unit/backfill.test.ts b/test/unit/backfill.test.ts index cbfe52dfd4..2c4d23d5d0 100644 --- a/test/unit/backfill.test.ts +++ b/test/unit/backfill.test.ts @@ -39,6 +39,7 @@ import { enqueueRepositoryOpenDataBackfill, enrichInstallationHealth, fetchAndStorePullRequestFilesForReview, + fetchBaseAheadBy, fetchLinkedIssueClosedByPullRequest, fetchLinkedIssueFacts, fetchLiveBaseBranchAdvancedAt, @@ -314,6 +315,36 @@ describe("GitHub backfill", () => { expect(await listLatestGitHubRateLimitObservations(env)).toEqual([]); }); + it("fetches how far the default branch has advanced beyond this PR's base commit via the compare API", async () => { + const env = createTestEnv(); + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + expect(String(input)).toBe("https://api.github.com/repos/JSONbored/gittensory/compare/abc123...main"); + return Response.json({ ahead_by: 47, behind_by: 0 }); + }); + + await expect( + fetchBaseAheadBy(env, "JSONbored/gittensory", "abc123", "main", "tok", githubRateLimitAdmissionKeyForInstallation(123)), + ).resolves.toBe(47); + }); + + it("returns undefined (fails open) when the compare fetch errors", async () => { + const env = createTestEnv(); + vi.stubGlobal("fetch", async () => new Response("not found", { status: 404 })); + + await expect( + fetchBaseAheadBy(env, "JSONbored/gittensory", "abc123", "main", "tok", githubRateLimitAdmissionKeyForInstallation(123)), + ).resolves.toBeUndefined(); + }); + + it("returns undefined when the response has no numeric ahead_by", async () => { + const env = createTestEnv(); + vi.stubGlobal("fetch", async () => Response.json({ behind_by: 0 })); + + await expect( + fetchBaseAheadBy(env, "JSONbored/gittensory", "abc123", "main", "tok", githubRateLimitAdmissionKeyForInstallation(123)), + ).resolves.toBeUndefined(); + }); + it("stores bounded repo metadata, labels, issues, PR details, recent merges, and contributor stats", async () => { const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); await seedInstalledAndRegisteredRepo(env); diff --git a/test/unit/content-lane-orchestrator.test.ts b/test/unit/content-lane-orchestrator.test.ts index 3f09214b67..04ec893ff9 100644 --- a/test/unit/content-lane-orchestrator.test.ts +++ b/test/unit/content-lane-orchestrator.test.ts @@ -8,7 +8,7 @@ import { assessSubnetDocument, type RegistryLaneSpec, } from "../../src/review/content-lane/registry-logic"; -import { diffAppendedSurfaceEntries, runSurfaceReview, type SurfaceReviewInput } from "../../src/review/content-lane/orchestrator"; +import { diffAppendedSurfaceEntries, runSurfaceReview, survivingExistingEntries, type SurfaceReviewInput } from "../../src/review/content-lane/orchestrator"; const existing = { kind: "website", url: "https://old.example.ai", source_url: "https://github.com/a/b", public_safe: true }; const newEntry = { kind: "subnet-api", url: "https://api.example.ai", source_url: "https://github.com/x/y", public_safe: true }; @@ -57,6 +57,28 @@ describe("diffAppendedSurfaceEntries", () => { }); }); +describe("survivingExistingEntries", () => { + const doc = (surfaces: unknown[]) => JSON.stringify({ netuid: 14, surfaces }); + + it("returns base entries that are still present, byte-identical, in head", () => { + expect(survivingExistingEntries(doc([existing, newEntry]), doc([existing]), "surfaces")).toEqual([existing]); + }); + + it("excludes a base entry that was edited in place (no longer byte-identical in head)", () => { + const edited = { ...existing, notes: "changed" }; + expect(survivingExistingEntries(doc([edited]), doc([existing]), "surfaces")).toEqual([]); + }); + + it("returns [] when head is unreadable/malformed", () => { + expect(survivingExistingEntries("{not json", doc([existing]), "surfaces")).toEqual([]); + expect(survivingExistingEntries(JSON.stringify({ netuid: 14 }), doc([existing]), "surfaces")).toEqual([]); + }); + + it("returns [] when base is absent (nothing to survive)", () => { + expect(survivingExistingEntries(doc([existing]), null, "surfaces")).toEqual([]); + }); +}); + describe("runSurfaceReview (deterministic + decisive: merge/close, rarely manual)", () => { const doc = (surfaces: unknown[]) => JSON.stringify({ netuid: 14, surfaces }); @@ -433,6 +455,39 @@ describe("runSurfaceReview (deterministic + decisive: merge/close, rarely manual }); }); + // Regression (metagraphed #7291-class incident): a PR that only EDITS an already-registered surface (e.g. + // tightening probe.expect after live-verification) keeps the surface's own url unchanged — the entry's edited + // content collides with its OWN prior base self under the url identity key. Before survivingExistingEntries, + // that self-collision read as a resubmitted duplicate and closed every legitimate content fix outright, + // regardless of correctness. The old entry is gone from head (replaced in place, not resubmitted alongside + // itself), so it must not be treated as still "existing" for duplicate purposes. + it("regression: an in-place edit of an existing entry (same url, changed field) is not a duplicate — merges when the new content is itself valid", async () => { + const edited = { ...existing, notes: "Live-verified 2026-07-20: still a public JSON endpoint." }; + const r = await review([SUBNET], { [`head:${SUBNET}`]: doc([edited]), [`base:${SUBNET}`]: doc([existing]) }); + expect(r?.verdict).toBe("merge"); + }); + + it("regression: an in-place edit whose new content is itself invalid closes with the real validator reason, not the generic duplicate message", async () => { + const edited = { ...existing, public_safe: false }; + const r = await review([SUBNET], { [`head:${SUBNET}`]: doc([edited]), [`base:${SUBNET}`]: doc([existing]) }); + expect(r?.verdict).toBe("close"); + expect(r?.summary).toContain("public_safe=true"); + expect(r?.summary).not.toContain("duplicate"); + }); + + it("an edit landing alongside a GENUINE duplicate append still closes: the untouched original survives to collide with the new entry", async () => { + const edited = { ...existing, notes: "edited" }; // replaces `existing` in place — not itself a duplicate + const freshResubmission = { ...newEntry, id: "resubmitted-newEntry-url" }; // newEntry's ORIGINAL self is untouched below + const r = await review( + [SUBNET], + { [`head:${SUBNET}`]: doc([edited, newEntry, freshResubmission]), [`base:${SUBNET}`]: doc([existing, newEntry]) }, + ); + expect(r).toEqual({ + verdict: "close", + summary: "A surface submission must not duplicate an entry already in this PR or already in the registry — resubmit without the duplicate.", + }); + }); + it("does not echo unvalidated duplicate URLs into public close summaries", async () => { const unsafeUrl = "not-a-safe-url \n### injected markdown"; const duplicate = { ...newEntry, id: "unsafe-duplicate", url: unsafeUrl }; diff --git a/test/unit/grounding-wiring.test.ts b/test/unit/grounding-wiring.test.ts index dc6f380b74..6e629b4ab0 100644 --- a/test/unit/grounding-wiring.test.ts +++ b/test/unit/grounding-wiring.test.ts @@ -296,6 +296,112 @@ describe("review-grounding wired into the AI reviewer (flag LOOPOVER_REVIEW_GROU fetchSpy.mockRestore(); }); + // #review-grounding stale-base fact (metagraphed #7305-class incident): buildReviewGroundingText's OWN + // wiring of the compare-API staleness read, on top of review-grounding.ts's already-covered pure logic. + describe("buildReviewGroundingText baseAheadBy wiring", () => { + const failingNoDetailCheck = check({ name: "test", conclusion: "failure", payload: {} as Record }); + + it("folds BASE BRANCH STATUS into the prompt when baseSha/defaultBranchRef resolve a positive ahead_by", async () => { + const env = createTestEnv({ LOOPOVER_REVIEW_GROUNDING: "true", GITHUB_PUBLIC_TOKEN: "ghp_test" }); + const fetchSpy = vi.spyOn(globalThis, "fetch").mockImplementation(async (url) => { + const u = String(url); + if (u.includes("/compare/")) { + expect(u).toBe("https://api.github.com/repos/acme/widgets/compare/abc123...main"); + return Response.json({ ahead_by: 12 }); + } + return new Response("not found", { status: 404 }); + }); + const out = await buildReviewGroundingText(env, { + repoFullName: "acme/widgets", + headSha: "sha7", + files: [], + checks: [failingNoDetailCheck], + installationId: null, + baseSha: "abc123", + defaultBranchRef: "main", + }); + expect(out.promptSection).toContain("BASE BRANCH STATUS"); + expect(out.promptSection).toContain("12 commits behind"); + fetchSpy.mockRestore(); + }); + + it("omits BASE BRANCH STATUS when baseSha/defaultBranchRef are not provided (back-compat) — no compare fetch attempted", async () => { + const env = createTestEnv({ LOOPOVER_REVIEW_GROUNDING: "true", GITHUB_PUBLIC_TOKEN: "ghp_test" }); + const fetchSpy = vi.spyOn(globalThis, "fetch").mockResolvedValue(new Response("not found", { status: 404 })); + const out = await buildReviewGroundingText(env, { + repoFullName: "acme/widgets", + headSha: "sha7", + files: [], + checks: [failingNoDetailCheck], + installationId: null, + }); + expect(out.promptSection).toContain("CI STATUS"); + expect(out.promptSection).not.toContain("BASE BRANCH STATUS"); + expect(fetchSpy).not.toHaveBeenCalled(); + fetchSpy.mockRestore(); + }); + + it("fail-safe: a failing compare fetch degrades to no BASE BRANCH STATUS section, CI grounding still present", async () => { + const env = createTestEnv({ LOOPOVER_REVIEW_GROUNDING: "true", GITHUB_PUBLIC_TOKEN: "ghp_test" }); + const fetchSpy = vi.spyOn(globalThis, "fetch").mockResolvedValue(new Response("server error", { status: 500 })); + const out = await buildReviewGroundingText(env, { + repoFullName: "acme/widgets", + headSha: "sha7", + files: [], + checks: [failingNoDetailCheck], + installationId: null, + baseSha: "abc123", + defaultBranchRef: "main", + }); + expect(out.promptSection).toContain("CI STATUS"); + expect(out.promptSection).not.toContain("BASE BRANCH STATUS"); + fetchSpy.mockRestore(); + }); + + it("resolves an installation token for the compare read when installationId is set (mint success)", async () => { + const env = createTestEnv({ LOOPOVER_REVIEW_GROUNDING: "true", GITHUB_PUBLIC_TOKEN: "ghp_public" }); + const tokenSpy = vi.spyOn(githubApp, "createInstallationToken").mockResolvedValue("install-token"); + let sawAuth: string | null = null; + const fetchSpy = vi.spyOn(globalThis, "fetch").mockImplementation(async (url, init) => { + if (String(url).includes("/compare/")) { + sawAuth = new Headers(init?.headers).get("authorization"); + return Response.json({ ahead_by: 3 }); + } + return new Response("not found", { status: 404 }); + }); + const out = await buildReviewGroundingText(env, { + repoFullName: "acme/widgets", + headSha: "sha7", + files: [], + checks: [failingNoDetailCheck], + installationId: 12345, + baseSha: "abc123", + defaultBranchRef: "main", + }); + expect(sawAuth).toBe("Bearer install-token"); + expect(out.promptSection).toContain("3 commits behind"); + tokenSpy.mockRestore(); + fetchSpy.mockRestore(); + }); + + it("does not attempt a compare read when the flag is off, even with baseSha/defaultBranchRef set", async () => { + const env = createTestEnv({ LOOPOVER_REVIEW_GROUNDING: "false" }); + const fetchSpy = vi.spyOn(globalThis, "fetch"); + const out = await buildReviewGroundingText(env, { + repoFullName: "acme/widgets", + headSha: "sha7", + files: [], + checks: [failingNoDetailCheck], + installationId: null, + baseSha: "abc123", + defaultBranchRef: "main", + }); + expect(out).toEqual({ systemSuffix: "", promptSection: "" }); + expect(fetchSpy).not.toHaveBeenCalled(); + fetchSpy.mockRestore(); + }); + }); + it("FLAG-ON e2e: full-file content is fetched (capped/prioritized) and inlined into the prompt", async () => { const env = createTestEnv({ LOOPOVER_REVIEW_GROUNDING: "true", GITHUB_PUBLIC_TOKEN: "ghp_test" }); // Stub the GitHub Contents API so the real FileFetcher returns deterministic file text. diff --git a/test/unit/review-grounding.test.ts b/test/unit/review-grounding.test.ts index 316f1ed7bc..8e176363a4 100644 --- a/test/unit/review-grounding.test.ts +++ b/test/unit/review-grounding.test.ts @@ -42,6 +42,30 @@ describe("review-grounding (#review-grounding)", () => { expect(filesOnly.changedFileContents).toEqual(files); }); + describe("buildGrounding baseAheadBy (metagraphed #7305-class stale-base fact)", () => { + const checks = checksAgg({ state: "failed", failingDetails: [{ name: "test" }] }); + + it("includes baseAheadBy when ciGrounding is on, checks are present, and the count is positive", () => { + expect(buildGrounding({ ciGrounding: true, fullFileContext: false }, checks, undefined, 47).baseAheadBy).toBe(47); + }); + + it("omits baseAheadBy when it is zero (nothing to say)", () => { + expect(buildGrounding({ ciGrounding: true, fullFileContext: false }, checks, undefined, 0).baseAheadBy).toBeUndefined(); + }); + + it("omits baseAheadBy when it is undefined (unreadable)", () => { + expect(buildGrounding({ ciGrounding: true, fullFileContext: false }, checks, undefined, undefined).baseAheadBy).toBeUndefined(); + }); + + it("omits baseAheadBy when ciGrounding is off, even with a positive count", () => { + expect(buildGrounding({ ciGrounding: false, fullFileContext: true }, checks, undefined, 47).baseAheadBy).toBeUndefined(); + }); + + it("omits baseAheadBy when there are no checks to explain (nothing for it to dangle off of)", () => { + expect(buildGrounding({ ciGrounding: true, fullFileContext: false }, undefined, undefined, 47).baseAheadBy).toBeUndefined(); + }); + }); + it("toCiSummary maps passing names + failing reasons", () => { const s = toCiSummary(checksAgg({ state: "failed", passing: ["build"], failingDetails: [{ name: "codecov/patch", summary: "60% of diff hit (target 97%)" }] })); expect(s.state).toBe("failed"); @@ -63,6 +87,50 @@ describe("review-grounding (#review-grounding)", () => { expect(out).toContain("FAILED: test — 3 tests failed"); }); + // Regression (metagraphed #7305-class incident): a generic CI runner's check-run carries no output.title/ + // summary beyond pass/fail, so `summary` is absent here. Before this fix that rendered as the bare check name + // ("FAILED: test"), giving the model no explicit signal that it has NO real error detail to reason from — it + // would fill the gap with a guessed, confidently-hedged content diagnosis instead. Marking the gap in-line, + // next to the fact itself, is what GROUNDING_GUIDANCE's forbidding rule below reacts to. + it("formatGroundingSections marks a failing check with no summary as having no detail provided", () => { + const out = formatGroundingSections({ checks: toCiSummary(checksAgg({ state: "failed", passing: ["build"], failingDetails: [{ name: "test" }] })) }); + expect(out).toContain("FAILED: test (no detail provided)"); + }); + + it("groundingSystemSuffix forbids a hedged guess about why a no-detail CI failure happened", () => { + const suffix = groundingSystemSuffix({ ciGrounding: true, fullFileContext: false }); + expect(suffix).toContain("no detail provided"); + expect(suffix).toContain("FORBIDDEN"); + }); + + it("groundingSystemSuffix tells the reviewer to prefer a known BASE BRANCH STATUS fact over guessing", () => { + expect(groundingSystemSuffix({ ciGrounding: true, fullFileContext: false })).toContain("BASE BRANCH STATUS"); + }); + + it("formatGroundingSections renders BASE BRANCH STATUS after CI STATUS when the PR is behind", () => { + const out = formatGroundingSections({ + checks: toCiSummary(checksAgg({ state: "failed", failingDetails: [{ name: "test" }] })), + baseAheadBy: 47, + }); + expect(out).toContain("BASE BRANCH STATUS"); + expect(out).toContain("47 commits behind"); + expect(out.indexOf("CI STATUS")).toBeLessThan(out.indexOf("BASE BRANCH STATUS")); + }); + + it("formatGroundingSections uses singular 'commit' for exactly one", () => { + const out = formatGroundingSections({ + checks: toCiSummary(checksAgg({ state: "failed", failingDetails: [{ name: "test" }] })), + baseAheadBy: 1, + }); + expect(out).toContain("1 commit behind"); + expect(out).not.toContain("1 commits behind"); + }); + + it("formatGroundingSections omits BASE BRANCH STATUS when baseAheadBy is absent or zero", () => { + const out = formatGroundingSections({ checks: toCiSummary(checksAgg({ state: "passed" })) }); + expect(out).not.toContain("BASE BRANCH STATUS"); + }); + it("formatGroundingSections inlines full file content + marks truncated files", () => { const out = formatGroundingSections({ changedFileContents: [{ path: "src/a.ts", text: "export const A = 1;" }, { path: "big.ts", text: "", truncated: true }] }); expect(out).toContain("FULL FILE CONTENT"); diff --git a/test/unit/unified-comment-bridge.test.ts b/test/unit/unified-comment-bridge.test.ts index e8c4b75b67..33f087b96b 100644 --- a/test/unit/unified-comment-bridge.test.ts +++ b/test/unit/unified-comment-bridge.test.ts @@ -268,7 +268,8 @@ describe("buildDualReviewNotes", () => { recommendation: "merge", verdict: "merge", }); - expect(reviews[0]?.notes?.blockers).toEqual(["No linked issue"]); // the real (non-AI) gate blocker still blocks + // the real (non-AI) gate blocker still blocks, now WITH its detail (FIX D-detail — see gateBlockerLine) + expect(reviews[0]?.notes?.blockers).toEqual(["No linked issue: ..."]); expect(reviews[0]?.notes?.nits).toEqual(["Off-by-one: Loop bound is wrong. (advisory only — not configured to block merge)"]); }); }); @@ -306,6 +307,65 @@ describe("buildDualReviewNotes", () => { "Missing test — Add a test.", ]); }); + + // FIX D-detail: metagraphed #7291-class incident — every content-lane finding shares the SAME constant title + // ("Registry surface review") regardless of the actual reason, so title-only rendering made "Why this is + // blocked" identically generic across a duplicate resubmission, an unsafe URL, and a secret leak alike. The + // real, verdict-specific reason lives in detail/publicText and must now reach the rendered blocker line. + describe("gate blocker lines surface their detail, not just a constant title (FIX D-detail)", () => { + it("appends the detail when it says something the title doesn't", () => { + const reviews = buildDualReviewNotes({ + gateBlockers: [ + { code: "surface_lane_reject", severity: "critical", title: "Registry surface review", detail: "A surface submission must not duplicate an entry already in the registry — resubmit without the duplicate." }, + ], + recommendation: "close", + verdict: "close", + }); + expect(reviews[0]?.notes?.blockers).toEqual([ + "Registry surface review: A surface submission must not duplicate an entry already in the registry — resubmit without the duplicate.", + ]); + }); + + it("omits the ': detail' suffix when detail is empty", () => { + const reviews = buildDualReviewNotes({ + gateBlockers: [{ code: "c", severity: "critical", title: "Secret leak detected", detail: "" }], + recommendation: "close", + verdict: "close", + }); + expect(reviews[0]?.notes?.blockers).toEqual(["Secret leak detected"]); + }); + + it("omits the ': detail' suffix when detail is identical to the title (nothing new to add)", () => { + const reviews = buildDualReviewNotes({ + gateBlockers: [{ code: "c", severity: "critical", title: "No linked issue", detail: "No linked issue" }], + recommendation: "close", + verdict: "close", + }); + expect(reviews[0]?.notes?.blockers).toEqual(["No linked issue"]); + }); + + it("prefers publicText over detail when a producer sets both", () => { + const reviews = buildDualReviewNotes({ + gateBlockers: [ + { code: "c", severity: "critical", title: "Registry surface review", detail: "internal-only wording", publicText: "public-safe wording" }, + ], + recommendation: "close", + verdict: "close", + }); + expect(reviews[0]?.notes?.blockers).toEqual(["Registry surface review: public-safe wording"]); + }); + + it("still appends the action suffix after the combined title:detail", () => { + const reviews = buildDualReviewNotes({ + gateBlockers: [{ code: "c", severity: "critical", title: "Missing linked issue", detail: "No issue is linked in the PR body.", action: "Link an eligible open issue." }], + recommendation: "close", + verdict: "close", + }); + expect(reviews[0]?.notes?.blockers).toEqual([ + "Missing linked issue: No issue is linked in the PR body. — Link an eligible open issue.", + ]); + }); + }); }); describe("splitAiReviewNits", () => {