From 73c0165ec8f35d84450048b5bbe750fbeff9c7b5 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Tue, 21 Jul 2026 12:01:59 -0700 Subject: [PATCH 1/3] fix(review): never render an oversized grounded file as fully omitted registry/subnets/eirel.json's 188KB post-change body overflowed the flat 24k/60k grounding caps and GitHub omits the inline diff for a change this large, so the reviewer got zero content for the file that was the actual substance of the PR and the gate closed it as a confirmed defect instead of an honest "can't verify this". Raise the budgets with real headroom, and durably fix the underlying gap: an oversized file now degrades to a real head+tail sample instead of ever rendering as pure omitted content again, regardless of how large it grows. Also raises the downstream aggregate prompt-budget cap that would otherwise silently drop grounding on exactly this large-PR case. --- src/review/review-grounding.ts | 87 ++++++++++++++++-- src/services/ai-review.ts | 18 ++-- test/unit/ai-review.test.ts | 15 +++- test/unit/review-grounding.test.ts | 137 ++++++++++++++++++++++++----- 4 files changed, 216 insertions(+), 41 deletions(-) diff --git a/src/review/review-grounding.ts b/src/review/review-grounding.ts index f3d87bebb..acaefae44 100644 --- a/src/review/review-grounding.ts +++ b/src/review/review-grounding.ts @@ -17,6 +17,10 @@ // (2) the one I/O dependency (GitHub file fetch) is INJECTED via the FileFetcher interface + explicit // params instead of reviewbot's RunContext/ReviewTarget/github helpers, so fetchFullFileContents is // self-contained and unit-testable. The host wires a real GitHub-backed FileFetcher at the call site. +// EXCEPTION (#7465-class fix): `sampleHeadAndTail` + the raised FILE_CONTENT_BUDGET/MAX_SINGLE_FILE/ +// MAX_FETCH_CHARS below are a loopover-side-ONLY addition, not yet ported back to reviewbot's own +// review-grounding.ts -- if reviewbot's copy is ever resynced from this file, carry this piece forward too, +// or reviewbot will regress to the old all-or-nothing truncation this was written to eliminate. import { isTestPath } from "../signals/test-evidence"; import { neutralizePromptInjection } from "./prompt-injection"; @@ -63,11 +67,60 @@ export interface ReviewGrounding { } // Budgets so the full-file block fits the 120B context alongside the diff + RAG + project knowledge. -const FILE_CONTENT_BUDGET = 60_000; // total chars inlined across all changed files -const MAX_SINGLE_FILE = 24_000; // a file larger than this is marked truncated (review it from the diff) +// Raised from 24k/60k (#7465-class fix: metagraphed PR #7465 was wrongly auto-closed because +// registry/subnets/eirel.json's 188KB post-change body — GitHub omits `patch` for a diff this large, so the +// bounded-diff builder had nothing either — blew straight through the old flat caps and rendered as a fully +// empty "(omitted — too large to inline; review this file from the diff)" placeholder with NO diff to fall +// back to either. The reviewer correctly said it couldn't verify a file it was never shown, and the one-shot +// gate closed the PR over an honest "I can't see this" rather than a confirmed defect.) These are now sized +// with real headroom relative to that 188KB incident file, but any FIXED cap is eventually defeated by +// growth — metagraphed's one-file-per-subnet registry only ever appends, so a subnet's file gets larger +// forever. The durable half of the fix is below: `sampleHeadAndTail` guarantees a file we successfully read +// is NEVER rendered as pure "omitted" content again, no matter how large it grows. +export const FILE_CONTENT_BUDGET = 96_000; // total chars inlined across all changed files +export const MAX_SINGLE_FILE = 48_000; // a file up to this size is inlined IN FULL; beyond it, head+tail SAMPLED +// The network-level read cap, decoupled from the two prompt budgets above: we always attempt to read the +// REAL, full file (up to this generous ceiling) so `sampleHeadAndTail` has genuine tail content to show — +// asking the fetcher for only `MAX_SINGLE_FILE` chars (the old behavior) would silently return a head-only +// prefix, which is exactly wrong for an append-oriented file where the NEW content lands at the end. +export const MAX_FETCH_CHARS = 1_000_000; +// Below this per-file share, a head+tail sample would be too thin on each side to carry real signal (mirrors +// review-diff.ts's `remaining < 240` guard style) — treat as unavailable rather than showing a sliver. +export const MIN_SAMPLE_CHARS = 400; // Binary / generated / lockfile paths carry no review signal as full text — skip inlining them. const SKIP_EXT = /\.(png|jpe?g|gif|webp|avif|bmp|heic|svg|ico|pdf|lock|min\.js|min\.css|map|woff2?|ttf|eot|mp4|webm|zip|gz|tgz|wasm)$/i; +/** + * When a successfully-read file's content doesn't fit in its allotted prompt `budget`, keep its HEAD and + * TAIL rather than dropping it entirely (#7465-class fix). A file this large will often keep growing forever + * (an append-only registry/manifest never shrinks), so no fixed budget can "cover" it permanently — the fix + * is to never render zero content for a file we actually read, regardless of size: the file's START (its + * top-level structure/preamble — proves no undisclosed metadata edits) and its END (new entries in an + * append-oriented file typically land at the tail) both survive, with an honest, sized marker for whatever + * was cut from the middle. Returns "" when `budget` is too small for a meaningful sample at all (below + * {@link MIN_SAMPLE_CHARS}) — the caller treats that the same as an unreadable file. + */ +export function sampleHeadAndTail(text: string, budget: number): string { + if (text.length <= budget) return text; + if (budget < MIN_SAMPLE_CHARS) return ""; + const marker = (omittedChars: number) => + `\n\n… (${omittedChars.toLocaleString("en-US")} chars omitted from the middle of this file — shown below: its real start and end) …\n\n`; + // Reserve space for the marker using the whole file's length as the omitted-count estimate — the actual + // printed count (computed below from the real head/tail split) can only be smaller, so this is always a + // safe (>=) upper bound on the marker's own rendered length. + const reserve = marker(text.length).length; + const available = budget - reserve; + const headLen = Math.ceil(available / 2); + const tailLen = available - headLen; + const omitted = text.length - headLen - tailLen; + // tailLen is always > 0 here: MIN_SAMPLE_CHARS (400, already checked above) comfortably exceeds the + // marker's own rendered length (well under 150 chars even at astronomical omitted-counts), so + // `available` -- and therefore `tailLen` -- can never reach 0. This matters because `"x".slice(-0)` + // returns the WHOLE string (JS treats -0 as plain 0, i.e. "from the start"), not "" -- a tailLen of + // exactly 0 would otherwise silently duplicate the head instead of omitting the tail. + return `${text.slice(0, headLen)}${marker(omitted)}${text.slice(-tailLen)}`; +} + /** The grounding feature flags (subset of reviewbot's FeatureToggles). Two internal booleans of one * GroundingFlags value — the type permits independent values so a future caller could split CI-summary * vs full-file gathering, but today's only caller (`groundingFlags()` in `./grounding-wire`) always sets @@ -93,6 +146,7 @@ const GROUNDING_GUIDANCE = [ "- 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.", + "- A file marked 'showing this file's real start and end' is REAL content read from that file — a genuine partial sample (too large to include in full), not a placeholder. Use the visible start/end to spot-check structure, formatting, and consistency; do not claim confidence about the omitted middle section, and do not treat something merely ABSENT from the visible sample as proof it is missing from the file — say you could not verify that part instead.", "- 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"); @@ -234,20 +288,31 @@ export async function fetchFullFileContents( out.push({ path: file.filename, text: "", truncated: true }); continue; } + // Always ask for the REAL file up to the generous network ceiling — never a per-file prompt-budget + // slice — so a file bigger than its prompt share still has genuine tail content for sampleHeadAndTail + // to show (#7465-class fix; see MAX_FETCH_CHARS). let text: string | null = null; try { - text = await fetcher.getFileContent(file.filename, ref, Math.min(MAX_SINGLE_FILE, FILE_CONTENT_BUDGET - used) + 1); + text = await fetcher.getFileContent(file.filename, ref, MAX_FETCH_CHARS); } catch { text = null; } if (text == null) continue; // unreadable (binary / vanished / perms) — skip silently - if (text.length > MAX_SINGLE_FILE || used + text.length > FILE_CONTENT_BUDGET) { + const share = Math.min(MAX_SINGLE_FILE, FILE_CONTENT_BUDGET - used); + if (text.length <= share) { + out.push({ path: file.filename, text }); + used += text.length; + continue; + } + const sampled = sampleHeadAndTail(text, share); + if (!sampled) { + // The remaining share was too thin for even a head+tail sample to carry signal — same as unreadable. out.push({ path: file.filename, text: "", truncated: true }); used = FILE_CONTENT_BUDGET; continue; } - out.push({ path: file.filename, text }); - used += text.length; + out.push({ path: file.filename, text: sampled, truncated: true }); + used += sampled.length; } return out.length ? out : undefined; } @@ -289,10 +354,16 @@ function formatCiSection(c: ReviewCiSummary): string { function formatFilesSection(files: ChangedFileContent[]): string { const blocks = files.map((file) => { const path = safeGroundingPath(file.path); - if (file.truncated) return `### ${path}\n(omitted — too large to inline; review this file from the diff)`; + // truncated + no text: the per-PR budget was already spent on higher-priority files before this one's + // turn — genuinely nothing to show (rare; #7465-class fix: a file we DID manage to read is never + // reduced to this, see the `truncated + text` branch below). + if (file.truncated && !file.text) return `### ${path}\n(no content available — the per-PR review budget was already spent on higher-priority files)`; const text = neutralizePromptInjection(file.text).text; const fence = safeMarkdownFence(text); - return `### ${path}\n${fence}\n${text}\n${fence}`; + // truncated + text: a real head+tail sample (see sampleHeadAndTail) — too large to include in full, but + // never a contentless placeholder. + const note = file.truncated ? "\n(too large to include in full — showing this file's real start and end; see the marker below for what's cut)\n" : ""; + return `### ${path}${note}${fence}\n${text}\n${fence}`; }); return ["FULL FILE CONTENT (post-change, head ref — check here before claiming any symbol is undefined/unused):", "", blocks.join("\n\n")].join("\n"); } diff --git a/src/services/ai-review.ts b/src/services/ai-review.ts index 9279b316c..3f0c89116 100644 --- a/src/services/ai-review.ts +++ b/src/services/ai-review.ts @@ -827,13 +827,17 @@ export function isIncoherentDiffBail(text: string): boolean { // Aggregate ceiling across ALL optional context sections combined (#3900). Each section below already // enforces its OWN per-section cap (FILE_CONTENT_BUDGET, MAX_CONTEXT_CHARS, MAX_PROMPT_CHARS, // MAX_ENRICHMENT_PROMPT_SECTION_CHARS...), but nothing previously bounded the COMBINED total: with every -// convergence feature enabled on one repo, the worst-case assembled prompt exceeds 200,000 characters before -// the system prompt is even added, degrading signal-to-noise on exactly the large/complex PRs that most need -// focused attention. The diff + description are NOT counted against this ceiling -- they are the primary -// review target and are always included in full (capped at 120,000 + 2,000 chars regardless). 200,000 sits -// comfortably above diff+description+grounding's own worst case (~182k) so grounding is effectively never -// trimmed, while still meaningfully bounding the "every feature enabled" case. -const AGGREGATE_CONTEXT_BUDGET_CHARS = 200_000; +// convergence feature enabled on one repo, the worst-case assembled prompt would otherwise exceed this +// ceiling before the system prompt is even added, degrading signal-to-noise on exactly the large/complex PRs +// that most need focused attention. The diff + description are NOT counted against this ceiling -- they are +// the primary review target and are always included in full (capped at 120,000 + 2,000 chars regardless). +// 240,000 sits comfortably above diff+description+grounding's own worst case (~218k, after #7465-class fix +// raised review-grounding.ts's FILE_CONTENT_BUDGET 60k→96k) so grounding -- the highest-priority section -- +// is effectively never trimmed, while still meaningfully bounding the "every feature enabled" case. This +// MUST be re-derived any time FILE_CONTENT_BUDGET (or the 120k diff cap above) changes again, or grounding's +// own fix silently stops reaching the model on exactly the large-PR case it exists for (see +// test/unit/ai-review.test.ts's "never trims grounding" regression test). +const AGGREGATE_CONTEXT_BUDGET_CHARS = 240_000; /** * Priority-ordered cutoff (#3900): walk the optional sections highest-priority-first, including each while it diff --git a/test/unit/ai-review.test.ts b/test/unit/ai-review.test.ts index 4cbd3d741..a250560dc 100644 --- a/test/unit/ai-review.test.ts +++ b/test/unit/ai-review.test.ts @@ -15,6 +15,7 @@ import { type LoopOverAiReviewInput, } from "../../src/services/ai-review"; import { createTestEnv } from "../helpers/d1"; +import { FILE_CONTENT_BUDGET } from "../../src/review/review-grounding"; import { renderMetrics, resetMetrics } from "../../src/selfhost/metrics"; import { inlineFindingCategory } from "../../src/review/inline-comments-select"; import { isPublicSafeText } from "../../src/signals/redaction"; @@ -4416,8 +4417,10 @@ describe("buildUserPrompt aggregate context budget (#3900)", () => { it("drops the lowest-priority sections first when every section enabled together would exceed the aggregate budget", () => { // Sized so grounding+RAG survive (highest priority) but impact-map/enrichment/culture-profile/test-evidence - // -- everything below RAG in priority order -- get cut once the running total would overflow. - const grounding = "G".repeat(150_000); + // -- everything below RAG in priority order -- get cut once the running total would overflow. Rescaled + // for AGGREGATE_CONTEXT_BUDGET_CHARS=240k (#7465-class fix, up from 200k): grounding+rag alone (230k) + // fit; adding impactMap (20k more) overflows, so it and everything after it drop. + const grounding = "G".repeat(190_000); const rag = "R".repeat(40_000); const impactMap = "I".repeat(20_000); const user = buildUserPrompt({ @@ -4438,8 +4441,12 @@ describe("buildUserPrompt aggregate context budget (#3900)", () => { expect(user.length).toBeLessThanOrEqual(AGGREGATE_CONTEXT_BUDGET_CHARS); }); - it("never trims grounding even with the diff at its own maximum size and grounding at its OWN real-world maximum (review-grounding.ts's 60k FILE_CONTENT_BUDGET)", () => { - const grounding = "G".repeat(60_000); + // #7465-class fix: AGGREGATE_CONTEXT_BUDGET_CHARS must always be re-derived to stay above + // diff+description+FILE_CONTENT_BUDGET's worst case -- this regression test pins that relationship so a + // future change to either budget that breaks it fails LOUDLY here, instead of silently dropping grounding + // (the highest-priority section) on exactly the large-PR case it exists to cover. + it("never trims grounding even with the diff at its own maximum size and grounding at its OWN real-world maximum (review-grounding.ts's FILE_CONTENT_BUDGET)", () => { + const grounding = "G".repeat(FILE_CONTENT_BUDGET); const user = buildUserPrompt({ ...budgetBaseInput, diff: "d".repeat(120_000), diff --git a/test/unit/review-grounding.test.ts b/test/unit/review-grounding.test.ts index 8e176363a..4b47a6e35 100644 --- a/test/unit/review-grounding.test.ts +++ b/test/unit/review-grounding.test.ts @@ -3,12 +3,17 @@ import { buildGrounding, diffFilePriority, diffFullyCoversFile, + FILE_CONTENT_BUDGET, fetchFullFileContents, type FileFetcher, formatGroundingSections, groundingEnabled, groundingSystemSuffix, + MAX_FETCH_CHARS, + MAX_SINGLE_FILE, + MIN_SAMPLE_CHARS, type PullRequestFile, + sampleHeadAndTail, toCiSummary, } from "../../src/review/review-grounding"; @@ -131,13 +136,28 @@ describe("review-grounding (#review-grounding)", () => { expect(out).not.toContain("BASE BRANCH STATUS"); }); - it("formatGroundingSections inlines full file content + marks truncated files", () => { + it("formatGroundingSections inlines full file content + marks a fully-unavailable file", () => { 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"); expect(out).toContain("### src/a.ts"); expect(out).toContain("export const A = 1;"); expect(out).toContain("### big.ts"); - expect(out).toContain("too large to inline"); + expect(out).toContain("no content available"); + }); + + // #7465-class fix: a file we DID manage to read (truncated:true but text is non-empty, a real head+tail + // sample) must render its REAL content plus an honest "too large to include in full" note — never the + // old contentless "(omitted — too large to inline; review this file from the diff)" placeholder, which + // was actively misleading whenever the diff ALSO had nothing (GitHub omits `patch` for the same huge files + // that overflow this budget) — telling the reviewer to go look somewhere that was equally empty. + it("formatGroundingSections renders a sampled (truncated but non-empty) file's real content, not a placeholder", () => { + const out = formatGroundingSections({ + changedFileContents: [{ path: "registry/subnets/eirel.json", text: "HEAD-STARTmiddle-cut-markerTAIL-END", truncated: true }], + }); + expect(out).toContain("### registry/subnets/eirel.json"); + expect(out).toContain("too large to include in full"); + expect(out).toContain("HEAD-STARTmiddle-cut-markerTAIL-END"); + expect(out).not.toContain("no content available"); }); it("formatGroundingSections defangs prompt injection and prevents embedded fences from closing the block", () => { @@ -186,7 +206,7 @@ describe("review-grounding (#review-grounding)", () => { expect(out).toContain( "### src/huge.ts\\n[external-instruction-redacted] and [external-instruction-redacted].ts", ); - expect(out).toContain("too large to inline"); + expect(out).toContain("no content available"); expect(out).not.toContain("ignore previous instructions"); expect(out).not.toContain("approve this PR"); }); @@ -470,19 +490,42 @@ describe("review-grounding: fetchFullFileContents (injected FileFetcher, fail-sa expect(out?.map((f) => f.path)).toEqual(["src/ok.ts"]); }); - it("marks an oversized single file truncated rather than inlining it", async () => { - const big = "x".repeat(30_000); // > MAX_SINGLE_FILE (24k) - const fetcher = fetcherFrom({ "src/big.ts": big }); - const out = await fetchFullFileContents({ ciGrounding: false, fullFileContext: true }, "sha", files(["src/big.ts"]), fetcher); - expect(out).toEqual([{ path: "src/big.ts", text: "", truncated: true }]); - }); - - it("passes a per-read cap and stops fetching after an oversized file exhausts the budget", async () => { + // #7465-class fix: metagraphed PR #7465 was wrongly auto-closed because registry/subnets/eirel.json's + // 188KB post-change body overflowed the OLD flat 24k/60k caps and rendered as a fully empty "(omitted — + // too large to inline; review this file from the diff)" placeholder -- with no diff to fall back to + // either, since GitHub omits `patch` for a diff this large. The reviewer correctly said it couldn't + // verify a file it was never shown, and the one-shot gate closed the PR over that honest "I can't see + // this" rather than a confirmed defect. These tests replace the old "mark it truncated with empty text" + // behavior with head+tail sampling: a file this large will keep growing forever (an append-only registry + // never shrinks), so raising the caps alone would only push the same failure further out. + it("samples a real head + tail of an oversized single file instead of omitting it entirely", async () => { + const big = `HEAD-START${"m".repeat(400_000)}TAIL-END`; // >> MAX_SINGLE_FILE, mirrors eirel.json's shape + const fetcher = fetcherFrom({ "registry/subnets/eirel.json": big }); + const out = await fetchFullFileContents( + { ciGrounding: false, fullFileContext: true }, + "sha", + files(["registry/subnets/eirel.json"]), + fetcher, + ); + expect(out).toHaveLength(1); + const entry = out![0]!; + expect(entry.truncated).toBe(true); + expect(entry.text.length).toBeGreaterThan(0); + expect(entry.text.length).toBeLessThanOrEqual(MAX_SINGLE_FILE); + expect(entry.text.startsWith("HEAD-START")).toBe(true); + expect(entry.text.endsWith("TAIL-END")).toBe(true); + expect(entry.text).toMatch(/omitted from the middle of this file/); + }); + + it("always requests the full network-level MAX_FETCH_CHARS cap, never a per-file prompt-budget slice", async () => { + // The old behavior asked the fetcher for only `min(MAX_SINGLE_FILE, remaining)+1` chars -- a HEAD-only + // prefix that could never carry real tail content for an append-oriented file. Grounding must always + // attempt to read the REAL file (up to the generous network ceiling) so sampling has genuine tail data. const reads: Array<{ path: string; maxChars: number | undefined }> = []; const fetcher: FileFetcher = { getFileContent: async (path, _ref, maxChars) => { reads.push({ path, maxChars }); - return path === "src/big.ts" ? "x".repeat((maxChars ?? 0) + 1) : "ok"; + return path === "src/big.ts" ? `HEAD${"x".repeat(200_000)}TAIL` : "ok"; }, }; const out = await fetchFullFileContents( @@ -491,11 +534,32 @@ describe("review-grounding: fetchFullFileContents (injected FileFetcher, fail-sa files(["src/big.ts"], ["src/after.ts"]), fetcher, ); - expect(out).toEqual([ - { path: "src/big.ts", text: "", truncated: true }, - { path: "src/after.ts", text: "", truncated: true }, - ]); - expect(reads).toEqual([{ path: "src/big.ts", maxChars: 24_001 }]); + expect(reads[0]).toEqual({ path: "src/big.ts", maxChars: MAX_FETCH_CHARS }); + const big = out?.find((f) => f.path === "src/big.ts"); + const after = out?.find((f) => f.path === "src/after.ts"); + expect(big?.truncated).toBe(true); + expect(big?.text.startsWith("HEAD")).toBe(true); + expect(big?.text.endsWith("TAIL")).toBe(true); + // src/big.ts only consumes its own MAX_SINGLE_FILE share of the budget, not the whole thing -- a small + // sibling file still gets its own room and is inlined in full. + expect(after).toEqual({ path: "src/after.ts", text: "ok" }); + }); + + it("falls all the way back to full omission when the remaining share is too thin for even a sample", async () => { + // Two fillers each just under MAX_SINGLE_FILE leave only 200 chars of the 96k budget for the third file + // -- below MIN_SAMPLE_CHARS, so sampleHeadAndTail itself declines rather than rendering a garbled sliver, + // and fetchFullFileContents degrades that to the same full-omission shape as an unreadable file. + const filler = "f".repeat(MAX_SINGLE_FILE - 100); + const map: Record = { "src/a.ts": filler, "src/b.ts": filler, "src/huge.ts": "z".repeat(1_000_000) }; + const fetcher: FileFetcher = { getFileContent: async (path) => map[path] ?? null }; + const out = await fetchFullFileContents( + { ciGrounding: false, fullFileContext: true }, + "sha", + files(["src/a.ts"], ["src/b.ts"], ["src/huge.ts"]), + fetcher, + ); + const huge = out?.find((f) => f.path === "src/huge.ts"); + expect(huge).toEqual({ path: "src/huge.ts", text: "", truncated: true }); }); it("returns undefined when nothing readable was inlined", async () => { @@ -504,9 +568,10 @@ describe("review-grounding: fetchFullFileContents (injected FileFetcher, fail-sa }); it("marks files truncated once the total inline budget is exhausted (later files skipped, not fetched)", async () => { - // Four 20k files: the first three inline (60k = exactly the budget), and any further file - // trips the budget-exhausted guard at the loop top → text:"" + truncated:true (no fetch). - const chunk = "y".repeat(20_000); // < MAX_SINGLE_FILE so each is individually inlinable + // Three 32k files exactly fill the 96k budget (each individually well under MAX_SINGLE_FILE=48k, so + // none get sampled -- they inline in full), and any further file trips the budget-exhausted guard at + // the loop top → text:"" + truncated:true (no fetch). + const chunk = "y".repeat(32_000); const map: Record = { "src/a.ts": chunk, "src/b.ts": chunk, "src/c.ts": chunk, "src/d.ts": chunk }; const reads: string[] = []; const fetcher: FileFetcher = { @@ -522,6 +587,7 @@ describe("review-grounding: fetchFullFileContents (injected FileFetcher, fail-sa fetcher, ); expect(out).toBeDefined(); + expect(out?.filter((f) => !f.truncated).map((f) => f.path)).toEqual(["src/a.ts", "src/b.ts", "src/c.ts"]); const dEntry = out?.find((f) => f.path === "src/d.ts"); expect(dEntry).toEqual({ path: "src/d.ts", text: "", truncated: true }); // The over-budget file is NOT fetched — the budget guard short-circuits before the read. @@ -532,7 +598,7 @@ describe("review-grounding: fetchFullFileContents (injected FileFetcher, fail-sa // Same budget-exhaustion shape as the test above, but every file carries status "added" -- proving // that restoring added-file fetching doesn't bypass the shared FILE_CONTENT_BUDGET when a PR adds // several large new files at once: the budget guard still trips per-file, not per-status. - const chunk = "z".repeat(20_000); // < MAX_SINGLE_FILE so each is individually inlinable + const chunk = "z".repeat(32_000); const map: Record = { "src/a.ts": chunk, "src/b.ts": chunk, "src/c.ts": chunk, "src/d.ts": chunk }; const reads: string[] = []; const fetcher: FileFetcher = { @@ -548,10 +614,37 @@ describe("review-grounding: fetchFullFileContents (injected FileFetcher, fail-sa fetcher, ); expect(out).toBeDefined(); - // First three fill the 60k budget exactly; the fourth trips the guard before it is fetched. + // First three fill the 96k budget exactly; the fourth trips the guard before it is fetched. expect(out?.filter((f) => !f.truncated).map((f) => f.path)).toEqual(["src/a.ts", "src/b.ts", "src/c.ts"]); const dEntry = out?.find((f) => f.path === "src/d.ts"); expect(dEntry).toEqual({ path: "src/d.ts", text: "", truncated: true }); expect(reads).not.toContain("src/d.ts"); }); }); + +describe("review-grounding: sampleHeadAndTail (#7465-class fix — never render zero content for a file we successfully read)", () => { + it("returns the text unchanged when it already fits the budget", () => { + expect(sampleHeadAndTail("short text", 100)).toBe("short text"); + expect(sampleHeadAndTail("exact", 5)).toBe("exact"); // boundary: length === budget still counts as fitting + }); + + it("returns empty when the budget is too thin for a meaningful sample", () => { + expect(sampleHeadAndTail("x".repeat(10_000), MIN_SAMPLE_CHARS - 1)).toBe(""); + }); + + it("keeps the real start and end and marks what was cut from the middle", () => { + const text = `${"A".repeat(50)}${"B".repeat(5_000)}${"C".repeat(50)}`; + const sampled = sampleHeadAndTail(text, 500); + expect(sampled.length).toBeLessThanOrEqual(500); + expect(sampled.startsWith("A".repeat(50))).toBe(true); + expect(sampled.endsWith("C".repeat(50))).toBe(true); + expect(sampled).toMatch(/omitted from the middle of this file/); + expect(sampled).not.toContain("B".repeat(5_000)); // the middle genuinely isn't all there + }); + + it("produces a non-empty sample right at the MIN_SAMPLE_CHARS boundary", () => { + const sampled = sampleHeadAndTail("x".repeat(10_000), MIN_SAMPLE_CHARS); + expect(sampled).not.toBe(""); + expect(sampled.length).toBeLessThanOrEqual(MIN_SAMPLE_CHARS); + }); +}); From 5d71ab3feba9694826ac9dc7ab9a1726db42d362 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Tue, 21 Jul 2026 12:01:59 -0700 Subject: [PATCH 2/3] fix(review): stop a truncated registry fetch from reading as zero surfaces The registry append-count check fetched HEAD/BASE content with no size cap (defaulting to 24,001 chars), silently truncating a larger registry file into invalid JSON. That parsed as "surfaces: null", misread as "nothing appended", and the existing safety net only checked for a literal null fetch, not a truncated-but-non-null one. Pass an explicit, generous cap and treat truncation as unreadable so it routes through the existing defer-to-generic-gate guard instead of a wrong close. Also closes a related latent bug found while tracing this: the same fetch path was using a PR's base BRANCH NAME as a cache key in a cache built to assume immutable commit SHAs, which can serve indefinitely stale content once written. Add a TTL safety net so that can't happen regardless of what ref shape any caller passes. --- src/db/repositories.ts | 16 ++++++++-- src/db/schema.ts | 9 ++++++ src/review/content-lane-wire.ts | 15 +++++++++- test/unit/content-lane-wire.test.ts | 45 +++++++++++++++++++++++++++++ test/unit/grounding-wiring.test.ts | 41 ++++++++++++++++++++++++++ 5 files changed, 122 insertions(+), 4 deletions(-) diff --git a/src/db/repositories.ts b/src/db/repositories.ts index d796f05c5..65b7ec979 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -5286,6 +5286,13 @@ export async function getLatestPublishedLinkedIssueSatisfaction( * miss. Unlike linked_issue_satisfaction_cache, every stored row is durable with NO input-fingerprint * dimension -- file content at an immutable head SHA has exactly one correct value, so a hit is always safe * to reuse verbatim. A nullish head SHA is always a miss (mirrors the sibling caches' contract). */ +// #7481-class fix: bounds how long a row can be served once written, as a safety net against a caller passing +// a mutable ref (a branch name) instead of a genuine commit SHA -- see schema.ts's fuller comment on +// groundingFileContentCache. Generous enough that it never affects the cache's real purpose (reusing a +// commit's content across review lanes / re-runs within the same PR, which happens within minutes), but bounds +// worst-case staleness for a misused mutable ref to hours, not indefinitely. +const GROUNDING_CACHE_TTL_MS = 24 * 60 * 60 * 1000; + export async function getCachedGroundingFileContent( env: Env, repoFullName: string, @@ -5294,10 +5301,13 @@ export async function getCachedGroundingFileContent( ): Promise { if (!headSha) return null; const row = await env.DB - .prepare("SELECT content FROM grounding_file_content_cache WHERE repo_full_name = ? AND path = ? AND head_sha = ?") + .prepare("SELECT content, fetched_at FROM grounding_file_content_cache WHERE repo_full_name = ? AND path = ? AND head_sha = ?") .bind(repoFullName, path, headSha) - .first<{ content: string }>(); - return row?.content ?? null; + .first<{ content: string; fetched_at: string }>(); + if (!row) return null; + const fetchedMs = Date.parse(row.fetched_at); + if (!Number.isFinite(fetchedMs) || Date.now() - fetchedMs > GROUNDING_CACHE_TTL_MS) return null; + return row.content; } /** #4499 (grounding-file-content-cache): upsert the fetched file content for (repo, path, head SHA). A diff --git a/src/db/schema.ts b/src/db/schema.ts index 1c8006c2a..901c7da95 100644 --- a/src/db/schema.ts +++ b/src/db/schema.ts @@ -1426,6 +1426,15 @@ export const linkedIssueSatisfactionCache = sqliteTable( // immutable commit, so it's safe to cache durably. NOT scoped to pullNumber (unlike linkedIssueSatisfactionCache // above) -- file content at a given head SHA is universal, not PR-specific. Only a successful fetch is ever // stored; a transient failure must never be cached as if it were a confirmed-permanent one. +// #7481-class fix: that immutability assumption holds only when `headSha` is genuinely a commit SHA -- a caller +// that instead passes a mutable ref (content-lane-wire.ts's base-content check uses the PR's base BRANCH NAME +// when no commit SHA is known) can poison a row forever, since the literal ref string never itself changes as +// the branch advances (confirmed empirically: a real metagraphed registry-file row keyed by the branch name +// "main" sat stale for over a week on the ORB server, silently serving every subsequent PR's base-content check +// a snapshot from before that window). getCachedGroundingFileContent now enforces a TTL (GROUNDING_CACHE_TTL_MS) +// at read time as a safety net against ANY caller violating the immutability assumption, current or future -- +// harmless for a true SHA (an occasional extra re-fetch of content that never changes), but bounds the +// worst-case staleness for a mutable ref to that TTL instead of indefinitely. export const groundingFileContentCache = sqliteTable( "grounding_file_content_cache", { diff --git a/src/review/content-lane-wire.ts b/src/review/content-lane-wire.ts index b2b37616b..0bdd8f10c 100644 --- a/src/review/content-lane-wire.ts +++ b/src/review/content-lane-wire.ts @@ -42,6 +42,7 @@ import { runSurfaceReview, type SurfaceReviewInput, type SurfaceReviewResult } f import type { RegistryLaneSpec } from "./content-lane/registry-logic"; import { registeredValidatorIds, resolveRegistryLaneSpec, unregisteredValidatorId } from "./content-lane/spec-resolver"; import { makeGithubFileFetcher } from "./grounding-wire"; +import { MAX_FETCH_CHARS } from "./review-grounding"; import type { FocusManifest } from "../signals/focus-manifest"; import { loadRepoFocusManifest } from "../signals/focus-manifest-loader"; import type { AdvisoryFinding, AdvisorySeverity } from "../types"; @@ -184,7 +185,19 @@ export async function runRegistrySurfaceGate( const githubLoad = async (path: string, ref: "head" | "base"): Promise => { fetcherPromise ??= makeGithubFileFetcher(env, args.repoFullName, args.installationId); const fetcher = await fetcherPromise; - return fetcher.getFileContent(path, ref === "head" ? args.pr.headSha : args.pr.baseRef); + const content = await fetcher.getFileContent(path, ref === "head" ? args.pr.headSha : args.pr.baseRef, MAX_FETCH_CHARS); + // #7481-class fix: unlike AI-review grounding (which tolerates a partial sample), this lane parses the + // fetched body as JSON and diffs its surfaces[] array deterministically -- a TRUNCATED body (the fetcher's + // maxChars+1-length signal, same convention patchless-secret-scan.ts relies on) is invalid JSON, which + // safeParseJson silently swallows into `null`, which reads as "surfaces: null" -- indistinguishable from a + // genuinely malformed submission. Without MAX_FETCH_CHARS above, the fetcher's small 24_001-char default + // truncated any registry file past that size (a metagraphed subnet manifest routinely exceeds it once it + // accumulates enough entries), and this file's own null-only unreadable check below never caught it, since + // a truncated fetch returns a non-null (just incomplete) string -- so a fully valid, large surfaces[] append + // silently misread as "zero entries appended" and one-shot closed. Converting it to a null return here + // routes it through the SAME defer-to-generic-gate guard as a real unreadable fetch, instead of a wrong close. + if (content !== null && content.length > MAX_FETCH_CHARS) return null; + return content; }; const baseLoad = loadFileOverride ?? githubLoad; const statusByPath = new Map(args.files.map((file) => [file.path, file.status ?? null])); diff --git a/test/unit/content-lane-wire.test.ts b/test/unit/content-lane-wire.test.ts index f62dd8deb..19eb8ed1e 100644 --- a/test/unit/content-lane-wire.test.ts +++ b/test/unit/content-lane-wire.test.ts @@ -3,6 +3,7 @@ import { AI_JUDGMENT_BLOCKER_CODES, type GateCheckEvaluation } from "../../src/r import { applySurfaceGate, evaluateWithSurfaceLane, resolveSurfaceRefs, runRegistrySurfaceGate, surfaceVerdictToGate } from "../../src/review/content-lane-wire"; import type { SurfaceReviewInput } from "../../src/review/content-lane/orchestrator"; import { METAGRAPHED_LANE_SPEC } from "../../src/review/content-lane/registry-logic"; +import { MAX_FETCH_CHARS } from "../../src/review/review-grounding"; import { parseFocusManifest, type FocusManifest } from "../../src/signals/focus-manifest"; import type { AdvisoryFinding } from "../../src/types"; @@ -289,6 +290,50 @@ describe("runRegistrySurfaceGate (injected loader — adapter logic)", () => { }); }); +// #7481-class fix: metagraphed PR #7481 was wrongly auto-closed with "must append at least one new surfaces[] +// entry" despite genuinely appending 35 entries, because the REAL GitHub loader (no loadFileOverride, hitting +// makeGithubFileFetcher directly) fetched HEAD content with no size cap, defaulting to 24_001 chars -- PR +// #7481's actual zipcode.json HEAD body was 67,085 bytes. The truncated (but non-null) prefix failed to +// parse as JSON, silently reading as "surfaces: null", which the existing null-only unreadable check never +// caught (a truncated fetch returns a real, non-null string). These tests exercise the REAL githubLoad path +// (not the injected-loader `run` helper above, which bypasses it entirely) via a stubbed global fetch, mirroring +// evaluateWithSurfaceLane's own "real GitHub loader" test. +describe("runRegistrySurfaceGate (#7481-class fix: oversized HEAD fetch defers instead of misreading as empty)", () => { + const stubFetchWithBodies = (bodies: Record) => { + vi.stubGlobal("fetch", async (url: string | URL) => { + const m = /\/contents\/(.+)\?ref=(.+)$/.exec(String(url)); + if (!m) return new Response("nope", { status: 404 }); + const path = m[1]!.split("/").map(decodeURIComponent).join("/"); + const body = bodies[`${decodeURIComponent(m[2]!)}:${path}`]; + return body === undefined ? new Response("missing", { status: 404 }) : new Response(body); + }); + }; + const runReal = () => + runRegistrySurfaceGate(env, METAGRAPHED_LANE_SPEC, { + installationId: 0, + repoFullName: REPO, + pr: { headSha: "HEAD", baseRef: "BASE" }, + advisory: { findings: [] }, + files: [{ path: SUBNET, status: "modified" }], + }); + + it("an oversized HEAD fetch (past MAX_FETCH_CHARS) DEFERS rather than closing on a misread 'zero entries appended'", async () => { + stubFetchWithBodies({ [`HEAD:${SUBNET}`]: "a".repeat(MAX_FETCH_CHARS + 1000), [`BASE:${SUBNET}`]: doc([existing]) }); + expect(await runReal()).toBeNull(); + }); + + it("an oversized BASE fetch on a MODIFIED file also DEFERS (same truncation risk on the other side of the diff)", async () => { + stubFetchWithBodies({ [`HEAD:${SUBNET}`]: doc([existing, newEntry]), [`BASE:${SUBNET}`]: "b".repeat(MAX_FETCH_CHARS + 1000) }); + expect(await runReal()).toBeNull(); + }); + + it("a HEAD fetch within MAX_FETCH_CHARS still parses and merges normally (no false-positive deferral)", async () => { + stubFetchWithBodies({ [`HEAD:${SUBNET}`]: doc([existing, newEntry]), [`BASE:${SUBNET}`]: doc([existing]) }); + const out = await runReal(); + expect(out?.conclusion).toBe("success"); + }); +}); + describe("resolveSurfaceRefs", () => { it("resolves head + base, falling base back to the repo default branch then empty", () => { expect(resolveSurfaceRefs({ headSha: "H", baseRef: "B" }, { defaultBranch: "main" })).toEqual({ headSha: "H", baseRef: "B" }); diff --git a/test/unit/grounding-wiring.test.ts b/test/unit/grounding-wiring.test.ts index b7884354f..3f3477667 100644 --- a/test/unit/grounding-wiring.test.ts +++ b/test/unit/grounding-wiring.test.ts @@ -992,6 +992,47 @@ describe("grounding_file_content_cache repository helpers", () => { await putCachedGroundingFileContent(env, "acme/widgets", "src/a.ts", "sha7", "export const a = 2; // updated"); expect(await getCachedGroundingFileContent(env, "acme/widgets", "src/a.ts", "sha7")).toBe("export const a = 2; // updated"); }); + + // #7481-class fix: a real metagraphed registry file's cache row keyed by the branch name "main" (from + // content-lane-wire.ts's base-content check, which has no true commit SHA to key on) sat stale on the ORB + // server for over a week, since the literal string "main" never itself changes as the branch advances, so + // the row was NEVER naturally invalidated. These pin the TTL safety net that now bounds that staleness for + // ANY ref -- current callers or future ones -- instead of trusting every caller to only ever pass a genuine + // immutable commit SHA. + it("REGRESSION (#7481-class fix): a cache row older than the TTL reads as a miss, not a stale hit", async () => { + const env = createTestEnv(); + await putCachedGroundingFileContent(env, "acme/widgets", "registry/subnets/foo.json", "main", "stale pre-merge content"); + const staleFetchedAt = new Date(Date.now() - 25 * 60 * 60 * 1000).toISOString(); // 25h ago > the 24h TTL + await env.DB.prepare( + "UPDATE grounding_file_content_cache SET fetched_at = ? WHERE repo_full_name = ? AND path = ? AND head_sha = ?", + ) + .bind(staleFetchedAt, "acme/widgets", "registry/subnets/foo.json", "main") + .run(); + expect(await getCachedGroundingFileContent(env, "acme/widgets", "registry/subnets/foo.json", "main")).toBeNull(); + }); + + it("a row just inside the TTL window still reads as a hit (the safety net doesn't defeat the cache's real purpose)", async () => { + const env = createTestEnv(); + await putCachedGroundingFileContent(env, "acme/widgets", "src/a.ts", "sha7", "still fresh"); + const recentFetchedAt = new Date(Date.now() - 60 * 60 * 1000).toISOString(); // 1h ago, well under the 24h TTL + await env.DB.prepare( + "UPDATE grounding_file_content_cache SET fetched_at = ? WHERE repo_full_name = ? AND path = ? AND head_sha = ?", + ) + .bind(recentFetchedAt, "acme/widgets", "src/a.ts", "sha7") + .run(); + expect(await getCachedGroundingFileContent(env, "acme/widgets", "src/a.ts", "sha7")).toBe("still fresh"); + }); + + it("a malformed/unparseable fetched_at is treated as a miss rather than throwing", async () => { + const env = createTestEnv(); + await putCachedGroundingFileContent(env, "acme/widgets", "src/a.ts", "sha7", "content"); + await env.DB.prepare( + "UPDATE grounding_file_content_cache SET fetched_at = ? WHERE repo_full_name = ? AND path = ? AND head_sha = ?", + ) + .bind("not-a-real-timestamp", "acme/widgets", "src/a.ts", "sha7") + .run(); + expect(await getCachedGroundingFileContent(env, "acme/widgets", "src/a.ts", "sha7")).toBeNull(); + }); }); // ── checkSummaryText empty fallback + outer-catch fail-safe ───────────────────────────────────────── From afe0978ed2847b94f6f2cb350b1441835157158b Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Tue, 21 Jul 2026 12:01:59 -0700 Subject: [PATCH 3/3] fix(review): catch a registry deliverable implied only by issue title The content-lane deliverable check only recognized a linked issue's own ask when its body named a literal registry file path. An issue whose body only carries a generic path placeholder (never the real resolved filename) read as not-applicable, silently providing zero protection against a PR that never delivers the registry change. Add an opt-in, spec-level signal: when a linked issue's title matches a known shape, a registry-file edit is required even without a literal path in the body. Deterministic and narrowly scoped -- no fuzzy slug inference, no AI call. --- src/queue/processors.ts | 2 +- src/review/content-lane/registry-logic.ts | 72 +++++++++++++--- .../unit/content-lane-deliverable-run.test.ts | 36 ++++++++ test/unit/content-lane-registry-logic.test.ts | 85 +++++++++++++++++++ 4 files changed, 181 insertions(+), 14 deletions(-) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index e2811f011..f47e7b33d 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -7577,7 +7577,7 @@ export async function runContentLaneDeliverableCheckForAdvisory( .join("\n\n"); if (!issueText.trim()) return; const changedFiles = args.files.map((file) => file.path); - const result = checkContentLaneDeliverable(spec, issueText, changedFiles); + const result = checkContentLaneDeliverable(spec, issueText, changedFiles, issueFetch.facts.title ?? undefined); if (result.verdict !== "missing") return; args.advisory.findings.push({ code: "content_lane_deliverable_missing", diff --git a/src/review/content-lane/registry-logic.ts b/src/review/content-lane/registry-logic.ts index 6017b9c9d..f5c627ef4 100644 --- a/src/review/content-lane/registry-logic.ts +++ b/src/review/content-lane/registry-logic.ts @@ -690,6 +690,22 @@ export interface RegistryLaneSpec { * domain-specific check, analogous to `assessAppendedEntry` but for the provider-file scope. Omitted ⇒ the * orchestrator returns "manual" for provider submissions to this registry. */ assessProviderEntry?: (document: unknown, opts?: { secretsScan?: boolean; sourceUrlValidation?: boolean }) => ProviderAssessment; + /** Optional (#content-lane-deliverable follow-up): a regex tested against a linked issue's TITLE (not body) + * that, when it matches, marks the issue as requiring an entry/provider-file edit even when + * `checkContentLaneDeliverable`'s own literal-path scan of the issue BODY finds nothing to check against. + * Exists for a KNOWN, confirmed gap: metagraphed's ~120 "MCP execute: verify + wire SN ()" + * issues (#7017-#7136) ask a contributor to add missing surfaces to `registry/subnets/.json`, but their + * bodies write that path with the LITERAL, generic `` placeholder (a documentation convention, not a + * template variable meant to be filled in), never the real resolved filename — confirmed empirically against + * issue #7060's actual body, which contains zero `registry/subnets/*.json`-shaped tokens at all. Without this + * field, the literal-path-only check returns "not-applicable" for the ENTIRE family, providing no protection + * against exactly the anti-pattern it exists to catch (a merged PR that adds only a test file, zero registry + * changes — confirmed live 2026-07-21, see metagraphed's own contributor-pipeline-gardening skill). Omitted + * ⇒ only the literal-path signal applies (today's behavior for any spec that doesn't opt in). Deliberately a + * TITLE match, not a fuzzy body/slug inference — a specific, known issue-title shape is a safe, zero- + * hallucination signal; guessing a subnet's file name from its display name is not, and a false "missing" on + * a genuinely-delivered PR is the more expensive mistake here. */ + issueTitleImpliesEntryPattern?: RegExp; } export type RegistryPrScope = "entry-submission" | "provider-submission" | "mixed-files" | "not-direct-submission"; @@ -847,26 +863,47 @@ export function extractPathTokens(text: string): string[] { return [...new Set(text.match(PATH_TOKEN_PATTERN) ?? [])]; } -/** `not-applicable`: the issue text names no path matching this spec's own patterns at all — nothing to check - * (an unrelated issue, e.g. a docs or code-fix issue, on the same content-lane repo). `delivered`: the issue - * names such a path AND the PR's changed files touch at least one file matching the spec — the common, - * expected case for a real contribution. `missing`: the issue names such a path but the PR touches NONE - * matching — the gap this check exists to catch, independent of AI judgment, CI status, or a closing keyword. */ +/** `not-applicable`: the issue names no path matching this spec's own patterns AND (when the spec opts into + * `issueTitleImpliesEntryPattern`) the issue's title doesn't match that pattern either — nothing to check (an + * unrelated issue, e.g. a docs or code-fix issue, on the same content-lane repo). `delivered`: the issue names + * such a path (or its title matches the spec's title pattern) AND the PR's changed files touch at least one + * file matching the spec — the common, expected case for a real contribution. `missing`: the issue implies a + * delivery (by either signal) but the PR touches NONE matching — the gap this check exists to catch, + * independent of AI judgment, CI status, or a closing keyword. `mentionedPath` is the literal path found in + * the body when that signal fired, or a description of the spec's expected file when only the title signal + * fired (no literal path was ever found in that case — see checkContentLaneDeliverable). */ export type ContentLaneDeliverableCheck = { verdict: "not-applicable" } | { verdict: "delivered" } | { verdict: "missing"; mentionedPath: string }; /** - * Determine whether a PR's changed files deliver the content-lane file its linked issue's own text names. - * PURE — no I/O, no registry-specific hardcoding; entirely driven by `spec` (the caller's already-resolved - * RegistryLaneSpec) and the two text inputs. `changedFiles` are matched the SAME way classifyRegistryPrScope - * matches them (canonicalized: lowercased + `./`-stripped + `\`→`/`), so an uppercase / `./`-prefixed / - * `\`-separated changed path is recognized identically to how the rest of the content lane already treats it. + * Determine whether a PR's changed files deliver the content-lane file its linked issue implies. PURE — no I/O, + * no registry-specific hardcoding; entirely driven by `spec` (the caller's already-resolved RegistryLaneSpec) + * and the text inputs. `changedFiles` are matched the SAME way classifyRegistryPrScope matches them + * (canonicalized: lowercased + `./`-stripped + `\`→`/`), so an uppercase / `./`-prefixed / `\`-separated + * changed path is recognized identically to how the rest of the content lane already treats it. + * + * Two independent, deterministic signals decide whether the issue implies a delivery, checked in this order: + * 1. A literal path in the issue BODY matching the spec (the general case — works for any hand-written issue + * that names the real file). + * 2. Failing that, `issueTitle` against the spec's OWN `issueTitleImpliesEntryPattern` (when configured) — a + * narrow, opt-in fallback for a KNOWN issue-title shape whose body only ever contains a generic path + * placeholder, never the real resolved filename (see that field's own doc comment for why this exists as a + * separate, deliberately non-fuzzy signal rather than inferring a slug from the issue's prose). + * Neither signal firing ⇒ "not-applicable". `issueTitle` is optional so an existing caller that doesn't pass + * one keeps today's literal-path-only behavior verbatim. */ -export function checkContentLaneDeliverable(spec: RegistryLaneSpec, issueText: string, changedFiles: readonly string[]): ContentLaneDeliverableCheck { +export function checkContentLaneDeliverable( + spec: RegistryLaneSpec, + issueText: string, + changedFiles: readonly string[], + issueTitle?: string, +): ContentLaneDeliverableCheck { const matchesSpec = (candidate: string): boolean => spec.entryFilePattern.test(candidate) || (spec.providerFilePattern?.test(candidate) ?? false); const mentionedPath = extractPathTokens(issueText).find(matchesSpec); - if (!mentionedPath) return { verdict: "not-applicable" }; + const titleImplies = !mentionedPath && Boolean(issueTitle && spec.issueTitleImpliesEntryPattern?.test(issueTitle)); + if (!mentionedPath && !titleImplies) return { verdict: "not-applicable" }; const delivered = changedFiles.some((file) => matchesSpec(canonicalize(file))); - return delivered ? { verdict: "delivered" } : { verdict: "missing", mentionedPath }; + if (delivered) return { verdict: "delivered" }; + return { verdict: "missing", mentionedPath: mentionedPath ?? `a registry entry file matching ${spec.entryFilePattern} (implied by this issue's title)` }; } // metagraphed's spec — the first RegistryLaneSpec. surfaces[] live in registry/subnets/.json; providers @@ -893,4 +930,13 @@ export const METAGRAPHED_LANE_SPEC: RegistryLaneSpec = { // checks) — supplied here, not hardcoded into the orchestrator, so a different registry can supply its own. assessAppendedEntry: assessSubnetDocument, assessProviderEntry: assessProviderDocument, + // #content-lane-deliverable follow-up: metagraphed's ~120 "MCP execute: verify + wire SN () + // once Phase 1 ships" issues (#7017-#7136, tracked under epic #7013/#7014) ask a contributor to add missing + // surfaces to registry/subnets/.json, but their bodies spell that path with the literal, generic + // `` placeholder — never the real resolved filename — so checkContentLaneDeliverable's own literal- + // path scan of the body finds nothing to check against for this entire family (confirmed empirically against + // issue #7060's actual body). Matches metagraphed's own contributor-pipeline-gardening skill's documented + // title convention for this exact family; deliberately anchored + narrow so it can't accidentally match an + // unrelated issue. + issueTitleImpliesEntryPattern: /^MCP execute:\s*verify\s*\+\s*wire\s+SN\d+/i, }; diff --git a/test/unit/content-lane-deliverable-run.test.ts b/test/unit/content-lane-deliverable-run.test.ts index ee2f8567d..45c7074e6 100644 --- a/test/unit/content-lane-deliverable-run.test.ts +++ b/test/unit/content-lane-deliverable-run.test.ts @@ -176,6 +176,42 @@ describe("runContentLaneDeliverableCheckForAdvisory (processor wiring, #content- expect(adv.findings).toEqual([]); }); + // #7060-class gap: METAGRAPHED_LANE_SPEC (resolved via the LOOPOVER_REVIEW_REPOS allowlist fallback when no + // explicit `contentLane:` manifest config is present -- the real production path for JSONbored/metagraphed + // itself) now catches its ~120 "MCP execute: verify + wire SN*" issues even though their bodies name no + // literal registry path, via the issue title threaded through from fetchLinkedIssueFacts. + it("REGRESSION (#7060-class gap): pushes a finding for METAGRAPHED_LANE_SPEC's own issue-title family even when the body names no literal path", async () => { + const env = createTestEnv({ LOOPOVER_REVIEW_CONTENT_LANE: "true", LOOPOVER_REVIEW_REPOS: "JSONbored/metagraphed" }); + stubIssueFetch({ + title: "MCP execute: verify + wire SN46 (Zipcode) once Phase 1 ships", + body: "Fix that surface's entry in `registry/subnets/.json` and append a note. See https://zipcode.ai/openapi.json.", + }); + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.endsWith("/issues/1275")) { + return Response.json({ + number: 1275, + state: "open", + title: "MCP execute: verify + wire SN46 (Zipcode) once Phase 1 ships", + body: "Fix that surface's entry in `registry/subnets/.json` and append a note. See https://zipcode.ai/openapi.json.", + }); + } + return new Response("not found", { status: 404 }); + }); + const adv = advisory(); + await runContentLaneDeliverableCheckForAdvisory(env, { + mode: "live", + settings: blockMode, + advisory: adv, + repoFullName: "JSONbored/metagraphed", + pr, + files, + installationId: 1, + }); + expect(adv.findings.map((f) => f.code)).toEqual(["content_lane_deliverable_missing"]); + }); + it("fetch_error from fetchLinkedIssueFacts (e.g. the issue request rejecting outright) degrades to no-op, never throws (fetchLinkedIssueFacts's own internal try/catch never rethrows)", async () => { const env = createTestEnv({ LOOPOVER_REVIEW_CONTENT_LANE: "true" }); await seedContentLaneRepo(env); diff --git a/test/unit/content-lane-registry-logic.test.ts b/test/unit/content-lane-registry-logic.test.ts index 56effcd2b..48adc329f 100644 --- a/test/unit/content-lane-registry-logic.test.ts +++ b/test/unit/content-lane-registry-logic.test.ts @@ -578,6 +578,91 @@ describe("checkContentLaneDeliverable (generic, spec-driven — #content-lane-de expect(checkContentLaneDeliverable(entryOnlySpec, issueText, ["registry/subnets/foo.json"]).verdict).toBe("delivered"); expect(checkContentLaneDeliverable(entryOnlySpec, issueText, ["tests/foo.test.mjs"]).verdict).toBe("missing"); }); + + // #content-lane-deliverable follow-up (metagraphed #7060-class gap): the literal-path scan alone is BLIND + // to metagraphed's own ~120 "MCP execute: verify + wire SN*" issues, whose bodies write the registry path + // with a generic `` documentation placeholder rather than the real resolved filename — confirmed + // empirically against the actual body of issue #7060 (which names zero registry/subnets/*.json-shaped + // tokens at all, only an unrelated external `zipcode.ai/openapi.json` URL). issueTitleImpliesEntryPattern is + // a narrow, opt-in fallback signal for exactly this known title shape. + describe("checkContentLaneDeliverable — issueTitleImpliesEntryPattern fallback (#7060-class gap)", () => { + const titleFallbackSpec: RegistryLaneSpec = { + entryFilePattern: SUBNET_ENTRY_PATTERN, + providerFilePattern: FLAT_PROVIDER_PATTERN, + collectionField: "surfaces", + issueTitleImpliesEntryPattern: /^MCP execute:\s*verify\s*\+\s*wire\s+SN\d+/i, + }; + // A realistic stand-in for issue #7060's actual body shape: a generic `` placeholder and an + // unrelated external URL, but no literal registry/subnets/*.json token anywhere. + const genericPlaceholderIssueText = [ + "Confirm SN46 (Zipcode)'s callable surface(s) actually work end-to-end.", + "", + "## Deliverable", + "Fix that surface's entry in `registry/subnets/.json` and append a note.", + "See https://zipcode.ai/openapi.json for the schema.", + ].join("\n"); + + it("is 'missing' when the title matches the known family and the body's own literal-path scan finds nothing (the confirmed #7060 gap)", () => { + const result = checkContentLaneDeliverable( + titleFallbackSpec, + genericPlaceholderIssueText, + ["tests/sn46-call-subnet-surface-verify.test.mjs"], + "MCP execute: verify + wire SN46 (Zipcode) once Phase 1 ships", + ); + expect(result.verdict).toBe("missing"); + expect((result as { mentionedPath: string }).mentionedPath).toContain(SUBNET_ENTRY_PATTERN.source); + }); + + it("is 'delivered' when the title matches the known family and the PR touches a real registry entry file", () => { + const result = checkContentLaneDeliverable( + titleFallbackSpec, + genericPlaceholderIssueText, + ["registry/subnets/zipcode.json"], + "MCP execute: verify + wire SN46 (Zipcode) once Phase 1 ships", + ); + expect(result).toEqual({ verdict: "delivered" }); + }); + + it("stays not-applicable when the title does NOT match the family and the body names no literal path (no accidental over-triggering)", () => { + const result = checkContentLaneDeliverable(titleFallbackSpec, genericPlaceholderIssueText, ["tests/unrelated.test.mjs"], "Fix a flaky CI timeout"); + expect(result).toEqual({ verdict: "not-applicable" }); + }); + + it("ignores the title signal when the spec doesn't configure issueTitleImpliesEntryPattern (existing callers keep today's behavior verbatim)", () => { + const specWithoutTitlePattern: RegistryLaneSpec = { entryFilePattern: SUBNET_ENTRY_PATTERN, providerFilePattern: FLAT_PROVIDER_PATTERN, collectionField: "surfaces" }; + const result = checkContentLaneDeliverable( + specWithoutTitlePattern, + genericPlaceholderIssueText, + ["tests/sn46-call-subnet-surface-verify.test.mjs"], + "MCP execute: verify + wire SN46 (Zipcode) once Phase 1 ships", + ); + expect(result).toEqual({ verdict: "not-applicable" }); + }); + + it("ignores the title signal when the caller omits issueTitle entirely (optional param, backward-compatible)", () => { + const result = checkContentLaneDeliverable(titleFallbackSpec, genericPlaceholderIssueText, ["tests/sn46-call-subnet-surface-verify.test.mjs"]); + expect(result).toEqual({ verdict: "not-applicable" }); + }); + + it("prefers a literal path in the body over the title signal when BOTH are present (mentionedPath reflects the real path, not the title-fallback description)", () => { + const issueTextWithRealPath = "MCP execute: verify + wire SN46 (Zipcode). Add the surface to registry/subnets/zipcode.json."; + const result = checkContentLaneDeliverable(titleFallbackSpec, issueTextWithRealPath, ["tests/unrelated.test.mjs"], "MCP execute: verify + wire SN46 (Zipcode) once Phase 1 ships"); + expect(result).toEqual({ verdict: "missing", mentionedPath: "registry/subnets/zipcode.json" }); + }); + + // Real-world regression: the ACTUAL production spec + the ACTUAL confirmed anti-pattern shape (PR #7359, + // a test-only PR, against issue #7060 — verified empirically by calling this exact function against the + // real fetched issue #7060 body during this fix's own investigation). + it("REGRESSION: METAGRAPHED_LANE_SPEC now catches the real #7060/#7359 test-only-PR anti-pattern", () => { + const result = checkContentLaneDeliverable( + METAGRAPHED_LANE_SPEC, + genericPlaceholderIssueText, + ["tests/sn46-call-subnet-surface-verify.test.mjs"], + "MCP execute: verify + wire SN46 (Zipcode) once Phase 1 ships", + ); + expect(result.verdict).toBe("missing"); + }); + }); }); describe("isBaseLayerKind", () => {