diff --git a/src/services/ai-review.ts b/src/services/ai-review.ts index 8079824f7e..373840c9ae 100644 --- a/src/services/ai-review.ts +++ b/src/services/ai-review.ts @@ -673,6 +673,40 @@ export function parseModelReview(text: string): ModelReview | null { } } +// 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; + +/** + * Priority-ordered cutoff (#3900): walk the optional sections highest-priority-first, including each while it + * still fits under the remaining budget, and stop entirely (dropping this section AND every lower-priority + * one after it) the moment one would not fit. A simple, predictable priority cutoff -- not a bin-packing + * optimization that could skip a large section to squeeze in a smaller, lower-priority one instead. + */ +function selectContextSectionsWithinBudget( + sections: ReadonlyArray<{ key: string; text: string | null | undefined }>, + usedChars: number, + budgetChars: number, +): Set { + const included = new Set(); + let running = usedChars; + for (const section of sections) { + if (!section.text) continue; + const addedChars = section.text.length + 2; // +2 for the blank-line separator `lines.push("", text)` adds + if (running + addedChars > budgetChars) break; + included.add(section.key); + running += addedChars; + } + return included; +} + function buildUserPrompt(input: GittensoryAiReviewInput): string { const lines = [ `Repository: ${input.repoFullName}`, @@ -687,33 +721,50 @@ function buildUserPrompt(input: GittensoryAiReviewInput): string { // review — self-host reviewers are configured with at least as much room). (#extensive-reviews) input.diff.slice(0, 120000), ]; - // Convergence (grounding): append the FINISHED CI status + FULL file content when the caller supplied them - // (flag GITTENSORY_REVIEW_GROUNDING on). Absent/empty (the default) → the prompt is byte-identical to today. + // Convergence (grounding): the FINISHED CI status + FULL file content when the caller supplied them (flag + // GITTENSORY_REVIEW_GROUNDING on). Absent/empty (the default) → the prompt is byte-identical to today. const groundingSection = input.grounding?.promptSection; - if (groundingSection) lines.push("", groundingSection); - // Convergence (RAG retrieval): append the retrieved RELEVANT EXISTING CODE / DOCS block when the caller - // supplied one (flag GITTENSORY_REVIEW_RAG on AND an index exists). Absent/empty (the default) → byte-identical. + // Convergence (RAG retrieval): the retrieved RELEVANT EXISTING CODE / DOCS block when the caller supplied + // one (flag GITTENSORY_REVIEW_RAG on AND an index exists). Absent/empty (the default) → byte-identical. const ragSection = input.ragContext; - if (ragSection) lines.push("", ragSection); - // Deterministic impact map (#2186): append the "IMPACT MAP" block when the caller supplied one (BOTH + // Deterministic impact map (#2186): the "IMPACT MAP" block when the caller supplied one (BOTH // GITTENSORY_REVIEW_IMPACT_MAP AND the per-repo review.impact_map opt-in on, AND the computation found at // least one affected module). Absent/empty (the default) → the prompt is byte-identical to today. const impactMapSection = input.impactMapContext; - if (impactMapSection) lines.push("", impactMapSection); - // Repo quality-culture profile (#2995): append the ADDITIVE "REPO QUALITY-CULTURE PROFILE" reference block - // when the caller supplied one (flag GITTENSORY_REVIEW_CULTURE_PROFILE + review.culture_profile both on). - // Absent/empty (the default) → the prompt is byte-identical. Reference-only grounding, never a gate input. - const cultureProfileSection = input.cultureProfileContext; - if (cultureProfileSection) lines.push("", cultureProfileSection); - // Review-enrichment brief (#1472): append the external REES analysis block when the caller supplied one (flag + // Review-enrichment brief (#1472): the external REES analysis block when the caller supplied one (flag // GITTENSORY_REVIEW_ENRICHMENT on AND REES_URL set). Absent/empty (the default) → the prompt is byte-identical. const enrichmentSection = input.enrichment?.promptSection; - if (enrichmentSection) lines.push("", enrichmentSection); - // Test-evidence classifier (#2558): ground the reviewer's test-adequacy judgment in the engine's own + // Repo quality-culture profile (#2995): the ADDITIVE "REPO QUALITY-CULTURE PROFILE" reference block when + // the caller supplied one (flag GITTENSORY_REVIEW_CULTURE_PROFILE + review.culture_profile both on). + // Absent/empty (the default) → the prompt is byte-identical. Reference-only grounding, never a gate input. + const cultureProfileSection = input.cultureProfileContext; + // Test-evidence classifier (#2558): grounds the reviewer's test-adequacy judgment in the engine's own // deterministic classification instead of eyeballing the diff. Absent/no changed code files without test // evidence ⇒ the prompt is byte-identical. const testEvidenceSection = buildTestEvidencePromptSection(input.changedFiles ?? []); - if (testEvidenceSection) lines.push("", testEvidenceSection); + + // Priority order (highest first): grounding (CI truth + full-file content) > RAG (codebase context) > + // impact map (deterministic blast-radius) > enrichment (external analyzer brief) > culture profile (soft + // house-style reference) > test-evidence flag (smallest, narrowest signal). + const included = selectContextSectionsWithinBudget( + [ + { key: "grounding", text: groundingSection }, + { key: "rag", text: ragSection }, + { key: "impactMap", text: impactMapSection }, + { key: "enrichment", text: enrichmentSection }, + { key: "cultureProfile", text: cultureProfileSection }, + { key: "testEvidence", text: testEvidenceSection }, + ], + lines.join("\n").length, + AGGREGATE_CONTEXT_BUDGET_CHARS, + ); + + if (groundingSection && included.has("grounding")) lines.push("", groundingSection); + if (ragSection && included.has("rag")) lines.push("", ragSection); + if (impactMapSection && included.has("impactMap")) lines.push("", impactMapSection); + if (cultureProfileSection && included.has("cultureProfile")) lines.push("", cultureProfileSection); + if (enrichmentSection && included.has("enrichment")) lines.push("", enrichmentSection); + if (testEvidenceSection && included.has("testEvidence")) lines.push("", testEvidenceSection); return lines.join("\n"); } @@ -2178,4 +2229,7 @@ export const __aiReviewInternals = { runWorkersOpinion, coerceAiUsage, aggregateActualUsage, + buildUserPrompt, + selectContextSectionsWithinBudget, + AGGREGATE_CONTEXT_BUDGET_CHARS, }; diff --git a/test/unit/ai-review.test.ts b/test/unit/ai-review.test.ts index baeedb8dcf..4717ec7f91 100644 --- a/test/unit/ai-review.test.ts +++ b/test/unit/ai-review.test.ts @@ -32,6 +32,9 @@ const { runWorkersOpinion, coerceAiUsage, aggregateActualUsage, + buildUserPrompt, + selectContextSectionsWithinBudget, + AGGREGATE_CONTEXT_BUDGET_CHARS, } = __aiReviewInternals; type InlineFinding = { @@ -3451,6 +3454,123 @@ describe("buildTestEvidencePromptSection (#2558)", () => { }); }); +describe("selectContextSectionsWithinBudget (#3900)", () => { + it("includes every present section when the total comfortably fits the budget", () => { + const included = selectContextSectionsWithinBudget( + [ + { key: "a", text: "x".repeat(100) }, + { key: "b", text: "y".repeat(100) }, + { key: "c", text: undefined }, + ], + 0, + 1000, + ); + expect(included).toEqual(new Set(["a", "b"])); + }); + + it("stops at the first section that would overflow and drops every lower-priority section after it, even one that would individually fit", () => { + const included = selectContextSectionsWithinBudget( + [ + { key: "first", text: "a".repeat(500) }, + { key: "second", text: "b".repeat(600) }, // 500+600=1100 > 1000 -- overflows here + { key: "third", text: "c".repeat(10) }, // would individually fit (500+10=510 <= 1000), but must NOT be + // included: a hard priority cutoff, not a bin-packing optimization that skips a large blocked section + // to squeeze in a smaller lower-priority one. + ], + 0, + 1000, + ); + expect(included).toEqual(new Set(["first"])); + }); + + it("skips an absent (undefined) section without consuming budget or affecting later decisions", () => { + const included = selectContextSectionsWithinBudget( + [ + { key: "present-1", text: "a".repeat(400) }, + { key: "absent", text: undefined }, + { key: "present-2", text: "b".repeat(400) }, + ], + 0, + 1000, + ); + expect(included).toEqual(new Set(["present-1", "present-2"])); + }); + + it("includes a section landing exactly on the budget boundary, excludes one that overflows by a single character", () => { + const exact = selectContextSectionsWithinBudget([{ key: "a", text: "x".repeat(8) }], 0, 10); // 0+8+2=10 <= 10 + expect(exact).toEqual(new Set(["a"])); + const over = selectContextSectionsWithinBudget([{ key: "a", text: "x".repeat(9) }], 0, 10); // 0+9+2=11 > 10 + expect(over).toEqual(new Set()); + }); + + it("accounts for chars already used (e.g. the diff/description) before evaluating the first section", () => { + const included = selectContextSectionsWithinBudget([{ key: "a", text: "x".repeat(100) }], 950, 1000); // 950+100+2 > 1000 + expect(included).toEqual(new Set()); + }); +}); + +describe("buildUserPrompt aggregate context budget (#3900)", () => { + const budgetBaseInput: GittensoryAiReviewInput = { + repoFullName: "owner/repo", + prNumber: 1, + title: "PR", + diff: "diff content", + mode: "advisory", + }; + + it("includes every optional section when everything is enabled but comfortably under budget", () => { + const user = buildUserPrompt({ + ...budgetBaseInput, + grounding: { promptSection: "GROUNDING-SECTION" }, + ragContext: "RAG-SECTION", + impactMapContext: "IMPACT-MAP-SECTION", + enrichment: { promptSection: "ENRICHMENT-SECTION" }, + cultureProfileContext: "CULTURE-PROFILE-SECTION", + changedFiles: [{ path: "src/a.ts" }], + }); + expect(user).toContain("GROUNDING-SECTION"); + expect(user).toContain("RAG-SECTION"); + expect(user).toContain("IMPACT-MAP-SECTION"); + expect(user).toContain("ENRICHMENT-SECTION"); + expect(user).toContain("CULTURE-PROFILE-SECTION"); + expect(user).toContain("zero test-path evidence"); + }); + + 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); + const rag = "R".repeat(40_000); + const impactMap = "I".repeat(20_000); + const user = buildUserPrompt({ + ...budgetBaseInput, + grounding: { promptSection: grounding }, + ragContext: rag, + impactMapContext: impactMap, + enrichment: { promptSection: "ENRICHMENT-SECTION" }, + cultureProfileContext: "CULTURE-PROFILE-SECTION", + changedFiles: [{ path: "src/a.ts" }], + }); + expect(user).toContain(grounding); + expect(user).toContain(rag); + expect(user).not.toContain(impactMap); + expect(user).not.toContain("ENRICHMENT-SECTION"); + expect(user).not.toContain("CULTURE-PROFILE-SECTION"); + expect(user).not.toContain("zero test-path evidence"); + 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); + const user = buildUserPrompt({ + ...budgetBaseInput, + diff: "d".repeat(120_000), + grounding: { promptSection: grounding }, + }); + expect(user).toContain(grounding); + }); +}); + describe("REVIEW_SYSTEM_PROMPT performance-regression instruction (#2559)", () => { it("instructs the model to treat a genuine algorithmic/performance regression as a blocker category", async () => { const run = vi.fn(