diff --git a/src/__tests__/orchestrate.test.ts b/src/__tests__/orchestrate.test.ts index 24a422c..71ac805 100644 --- a/src/__tests__/orchestrate.test.ts +++ b/src/__tests__/orchestrate.test.ts @@ -774,7 +774,10 @@ describe("orchestrate", () => { await orchestrate(stubs.deps, logger) - expect(first(stubs.findRelatedFilesCalls).excludePaths).toEqual(["assets/logo.png"]) + expect(first(stubs.findRelatedFilesCalls).excludePaths).toEqual([ + "assets/logo.png", + "AGENTS.md", + ]) expect(first(stubs.findRelatedDocsCalls).excludePaths).toEqual(["assets/logo.png"]) }) @@ -1212,7 +1215,9 @@ describe("orchestrate", () => { await orchestrate(stubs.deps, createTestLogger()) - expect(relatedFileCalls.map((call) => call.excludePaths)).toEqual([[priorityDoc.path]]) + expect(relatedFileCalls.map((call) => call.excludePaths)).toEqual([ + [priorityDoc.path, "AGENTS.md"], + ]) const reviewContext = first(stubs.generateFindingsCalls) expect(reviewContext.relatedFiles).toEqual([]) expect(reviewContext.relatedDocs).toEqual([priorityDoc]) @@ -1853,6 +1858,56 @@ describe("orchestrate", () => { ]) }) + it("keeps a conventions file its own section carries whole out of the related-file scan", async () => { + // A JS/TS conventions file that imports a changed file is a related-file + // candidate. Its section already sends the whole text, so a related-file + // block would send it a second time. + const conventionsImporter: PromptFile = { + path: "conventions.ts", + content: "import { greet } from './src/greeter.js'", + includedAs: "full", + reason: "imports src/greeter.ts", + } + const relatedFileCalls: FindRelatedFilesParams[] = [] + const stubs = makeOrchestrateDeps({ + config: { conventionsFile: conventionsImporter.path }, + contextReader: { + readConventions: async () => conventionsImporter.content, + findRelatedFiles: async (params) => { + relatedFileCalls.push(params) + return { + files: params.excludePaths.includes(conventionsImporter.path) + ? [] + : [conventionsImporter], + excludedByCapPaths: [], + } + }, + }, + }) + + await orchestrate(stubs.deps, createTestLogger()) + + expect(relatedFileCalls.map((call) => call.excludePaths)).toEqual([ + [conventionsImporter.path], + ]) + expect(first(stubs.generateFindingsCalls).relatedFiles).toEqual([]) + }) + + it("leaves a truncated conventions file eligible for the related-file scan", async () => { + // Over the 8k-token cap the section holds only a head, so a related-file + // block is the one channel that can still send the whole text. + const stubs = makeOrchestrateDeps({ + config: { conventionsFile: "conventions.ts" }, + contextReader: { + readConventions: async () => "c".repeat(32_001), + }, + }) + + await orchestrate(stubs.deps, createTestLogger()) + + expect(stubs.findRelatedFilesCalls.map((call) => call.excludePaths)).toEqual([[]]) + }) + it("excludes an over-cap conventions file from priority docs when it is changed in the PR", async () => { // Over the 8k-token cap + changed + in priority_docs: the changed-files // channel keeps the full copy (conventions section truncates), and the diff --git a/src/context/__tests__/workspace.test.ts b/src/context/__tests__/workspace.test.ts index c527905..c622f65 100644 --- a/src/context/__tests__/workspace.test.ts +++ b/src/context/__tests__/workspace.test.ts @@ -5,7 +5,7 @@ import path from "node:path" import { fileURLToPath } from "node:url" import { describe, expect, it, vi } from "vitest" import { createTestLogger, logsWithMessage } from "../../__tests__/test-logger.js" -import { estimateTokens } from "../../review/prompt.js" +import { buildUserPrompt, estimateTokens } from "../../review/prompt.js" import { createContextReader, type ContextReaderConfig } from "../workspace.js" vi.mock("node:fs/promises", async (importOriginal) => { @@ -33,8 +33,9 @@ const defaultConfig = (workspaceRoot: string): ContextReaderConfig => ({ const workspaceRoot = fileURLToPath(new URL("../../../fixtures/workspace", import.meta.url)) -const readFixture = (workspaceRelativePath: string): string => - readFileSync(path.join(workspaceRoot, workspaceRelativePath), "utf8") +const readFixture = (workspaceRelativePath: string): string => { + return readFileSync(path.join(workspaceRoot, workspaceRelativePath), "utf8") +} const greeterContent = readFixture("src/greeter.ts") const registryContent = readFixture("src/registry.ts") @@ -1395,6 +1396,51 @@ describe("readPriorityDocs", () => { } }) + // The related-docs header quotes the priority-doc reason, so renaming the + // reason without the header leaves the model a label that matches no block. + it("sets the reason that the related-docs header quotes", async () => { + const { root, cleanup } = await makeTempWorkspace({ "README.md": "# Greeter" }) + try { + const reader = createContextReader(defaultConfig(root), createTestLogger()) + + const { files } = await reader.readPriorityDocs({ + priorityDocs: ["README.md"], + budgetTokens: 100_000, + excludePaths: [], + }) + + const userPrompt = buildUserPrompt({ + prContext: { + prNumber: 1, + title: "docs: greeter", + body: null, + headSha: "abc123", + headRef: "docs/greeter", + baseRef: "main", + }, + conventions: null, + conventionsFile: "AGENTS.md", + conventionsBudgetTokens: 8_000, + changedFiles: [], + relatedFiles: [], + relatedDocs: files, + annotatedDiff: "", + priorFindings: [], + priorBotComments: [], + delimiterNonce: "abc123def456", + }) + + expect(userPrompt).toContain( + [ + 'Documentation provided as context — flag any claims that have become stale. A block whose reason is "priority documentation" is included whether or not the diff touches it; every other block\'s reason names changed files it mentions:', + '\n# Greeter\n', + ].join("\n\n"), + ) + } finally { + await cleanup() + } + }) + it("skips a doc whose byte size exceeds the byte budget before reading it", async () => { // Multibyte content: 400 chars fit a 100-token budget, but 1200 UTF-8 // bytes exceed it — only the stat-first byte guard skips this doc; diff --git a/src/orchestrate.ts b/src/orchestrate.ts index bb78a6f..75188be 100644 --- a/src/orchestrate.ts +++ b/src/orchestrate.ts @@ -739,7 +739,11 @@ const runReviewPipeline = async ( ? await contextReader.findRelatedFiles({ changedPaths, budgetTokens: relatedFilesBudgetTokens, - excludePaths: [...diffExcludedPaths, ...earlyPriorityDocFiles.map((file) => file.path)], + excludePaths: [ + ...diffExcludedPaths, + ...earlyPriorityDocFiles.map((file) => file.path), + ...conventionsFullCopyPaths, + ], }) : { files: [], excludedByCapPaths: [] } diff --git a/src/review/__tests__/prompt.test.ts b/src/review/__tests__/prompt.test.ts index c4dd476..6c0bc0b 100644 --- a/src/review/__tests__/prompt.test.ts +++ b/src/review/__tests__/prompt.test.ts @@ -266,7 +266,7 @@ describe("buildUserPrompt", () => { const conventionsIndex = userPrompt.indexOf('') const changedFileIndex = userPrompt.indexOf('') const relatedFileIndex = userPrompt.indexOf(' { it("omits the related docs section when relatedDocs is empty", () => { const userPrompt = buildUserPrompt(makeUserPromptParts()) - expect(userPrompt).not.toContain("Documentation that may describe changed code") + // Positive anchor first — an empty prompt would also pass the negative check + expect(userPrompt).toContain(" { + it("introduces priority and mention-matched docs under one header that covers both inclusion reasons", () => { const userPrompt = buildUserPrompt({ ...makeUserPromptParts(), relatedDocs: [ + { + path: "README.md", + content: "# Greeter", + includedAs: "full" as const, + reason: "priority documentation", + }, { path: "docs/api.md", content: "# API", @@ -299,7 +307,11 @@ describe("buildUserPrompt", () => { }) expect(userPrompt).toContain( - "Documentation that may describe changed code (flag any claims that have become stale):", + [ + 'Documentation provided as context — flag any claims that have become stale. A block whose reason is "priority documentation" is included whether or not the diff touches it; every other block\'s reason names changed files it mentions:', + '\n# Greeter\n', + '\n# API\n', + ].join("\n\n"), ) }) @@ -362,7 +374,16 @@ describe("buildUserPrompt", () => { }) expect(userPrompt.isWellFormed()).toBe(true) - expect(userPrompt).toContain("[conventions truncated at ~8000 tokens]") + // toWellFormed replaces the orphaned high surrogate with U+FFFD + expect(userPrompt).toContain( + [ + '', + `${"x".repeat(31_999)}�`, + "", + "[conventions truncated at ~8000 tokens]", + '', + ].join("\n"), + ) }) it("truncates oversized conventions with an explicit notice", () => { @@ -395,6 +416,38 @@ describe("buildUserPrompt", () => { ) }) + it("keeps the reason attribute on a diff-only file block", () => { + const userPrompt = buildUserPrompt({ + ...makeUserPromptParts(), + relatedFiles: [ + { + path: "src/caller.ts", + content: "", + includedAs: "diff-only" as const, + reason: "references changed-file src/greeter.ts", + }, + ], + }) + + expect(userPrompt).toContain( + '\n', + ) + }) + + it("wraps the annotated diff in the nonce-tagged diff section", () => { + const userPrompt = buildUserPrompt(makeUserPromptParts()) + + expect(userPrompt).toContain( + [ + '', + "=== src/greeter.ts ===", + "@@ -1,1 +1,1 @@", + " 1 + export const greet", + "", + ].join("\n"), + ) + }) + it("omits the prior_findings section when there are none", () => { const userPrompt = buildUserPrompt(makeUserPromptParts()) @@ -409,7 +462,15 @@ describe("buildUserPrompt", () => { prContext: { ...prContext, body: null }, }) - expect(userPrompt).toContain("PR description:\n(none)") + expect(userPrompt).toContain( + [ + "", + "PR title: feat: trim names before greeting", + "Branch: feat/trim-names → main", + "PR description:\n(none)", + "", + ].join("\n"), + ) }) it("includes prior findings with a do-not-re-report instruction when present", () => { @@ -421,14 +482,32 @@ describe("buildUserPrompt", () => { }) expect(userPrompt).toContain( - '', + [ + '', + "[", + " {", + ' "file": "src/greeter.ts",', + ' "line": 145,', + ' "end_line": null,', + ' "category": "correctness",', + ' "severity": "medium",', + ' "confidence": "high",', + ' "title": "Whitespace-only keys pass the empty-key guard",', + ' "description": "The guard rejects only the exact empty string.",', + ' "suggestion": null,', + ' "failure_scenario": "register(\\" \\", \\"value\\") succeeds and the entry is orphaned."', + " }", + "]", + "", + ].join("\n"), ) - expect(userPrompt).toContain(priorFinding.title) }) it("omits the prior bot comments section when empty", () => { const userPrompt = buildUserPrompt(makeUserPromptParts()) + // Positive anchor first — an empty prompt would also pass the negative check + expect(userPrompt).toContain(" { const userPrompt = buildUserPrompt(makeUserPromptParts()) expect(userPrompt).toContain( - '', + [ + '', + 'import { greet } from "./greeter.js"', + '', + ].join("\n"), ) }) @@ -574,7 +657,29 @@ describe("conventionsRenderInFull", () => { }) expect(conventionsRenderInFull(oversizedConventions, budgetTokens)).toBe(false) - expect(prompt).toContain("[conventions truncated at ~8000 tokens]") + expect(prompt).toContain( + [ + '', + "c".repeat(conventionsCharacterCap), + "", + "[conventions truncated at ~8000 tokens]", + '', + ].join("\n"), + ) + }) + + it("agrees with buildUserPrompt rendering conventions exactly at the cap in full", () => { + const conventionsAtCap = "c".repeat(conventionsCharacterCap) + + const prompt = buildUserPrompt({ + ...makeUserPromptParts(), + conventions: conventionsAtCap, + }) + + expect(conventionsRenderInFull(conventionsAtCap, budgetTokens)).toBe(true) + expect(prompt).toContain( + `\n${conventionsAtCap}\n`, + ) }) it("respects a custom budget", () => { @@ -596,7 +701,14 @@ describe("conventionsRenderInFull", () => { conventionsBudgetTokens: customBudget, }) - expect(prompt).toContain("[conventions truncated at ~16000 tokens]") - expect(prompt).not.toContain("[conventions truncated at ~8000 tokens]") + expect(prompt).toContain( + [ + '', + "c".repeat(customCharacterCap), + "", + "[conventions truncated at ~16000 tokens]", + '', + ].join("\n"), + ) }) }) diff --git a/src/review/prompt.ts b/src/review/prompt.ts index d893f88..87aac2a 100644 --- a/src/review/prompt.ts +++ b/src/review/prompt.ts @@ -20,22 +20,33 @@ export type PromptFile = { export const CHARS_PER_TOKEN = 4 /** Longest conventions text, in characters, the conventions section carries - * before truncating. The render check, the prompt truncation, and the - * truncation report all read it, so they cannot drift apart. */ + * before truncating. conventionsRenderInFull, truncateConventions, and + * classifyConventionsCoverage (context-notes.ts) all read it, so they cannot + * drift apart. */ export const conventionsCharacterCap = (conventionsBudgetTokens: number): number => { return conventionsBudgetTokens * CHARS_PER_TOKEN } -/** Whether the conventions section will carry the file's complete text rather - * than a truncated head. A conventions file that also changed in the PR is - * rendered by the changed-files channel too — the caller uses this to decide - * which of the two copies is the full one, so exactly one full copy is ever - * sent. */ +/** + * Whether the conventions section will carry the file's complete text rather + * than a truncated head. The orchestrator uses it to send one full copy of the + * conventions file: + * - true: the section is the full copy. readPriorityDocs and the related-file + * scan get the path as an exclusion and skip the file, and a changed + * conventions file's block is sent diff-only. + * - false: the section carries a truncated head, and a full copy can arrive + * another way. When a priority-doc block (reason "priority documentation") + * carries it, the orchestrator swaps the section's text for a placeholder + * that points to that block. When a changed-file or related-file block, or + * the diff of a newly added file, carries it, the truncated section stays. + */ export const conventionsRenderInFull = ( conventions: string, conventionsBudgetTokens: number, ): boolean => conventions.length <= conventionsCharacterCap(conventionsBudgetTokens) +/** No code parses the "Pre-existing:" description prefix. It labels the + * finding for the person reading the posted comment. */ const IDENTITY_AND_SCOPE = `You are umm-actually, a code review bot. You review the changes in a pull request. You are skeptical: code being in the diff is not evidence it is correct. @@ -95,6 +106,10 @@ Every key is required on every finding. When there is nothing to report, "findings" must be the empty array [] — never omit the key. Output only the JSON object: no markdown fences, no text before or after it.` +/** filterNonFindings (filter-non-findings.ts) is the backstop for when the + * model emits a non-finding anyway. Its patterns cover most phrases quoted + * here, but not "Verify X handles Y", because a real fix title can also start + * with "Verify". When a quoted phrase changes, check its pattern there too. */ const OUTPUT_DISCIPLINE = `OUTPUT DISCIPLINE — field constraints: - "title": imperative fix statement, under 80 characters (e.g. "Trim keys before inserting into the registry"). Do not start with "Issue:" or @@ -121,6 +136,21 @@ the code is correct — including that a previously posted bot comment has been resolved by this revision — record that conclusion in "analysis" and move on — do not emit a finding for it.` +/** + * This text quotes two strings that the diff module writes. annotateDiff + * (annotate-diff.ts) writes the "=== path ===" headers, and + * renderExcludedFilesNote (exclusion.ts) writes the "changed file(s) excluded + * from review" trailer. A wording change there must update this text. + * + * The File attribution paragraph covers three cases: + * - A finding inside the annotated diff needs no quote line. The paragraph's + * first "Boundary:" states this case. + * - A finding outside the diff quotes its passage and is filed on the path + * whose block holds that passage. + * - A finding about missing content quotes the nearest heading, marked + * "(missing here)". The paragraph's second "Boundary:" keeps this case from + * covering text that sits in another file's block. + */ const ANCHORING_CONTRACT = `Prompt sections: the annotated diff is the section, a file block is one section, and the conventions section is the section. @@ -164,6 +194,10 @@ trailer at the end of the diff are not in context — their content was not provided. Do not report findings on excluded file paths.` export const buildSystemPrompt = ({ phase }: { phase: ReviewPhase }): string => { + // Order matters because sections cite each other by position. PROOF_OF_WORK + // points "above" to the DIMENSION sections in phase.instructionSections and + // "below" to File attribution in ANCHORING_CONTRACT, which points back + // "above" to PROOF_OF_WORK's path: "sentence" form. return [ IDENTITY_AND_SCOPE, ...phase.instructionSections, @@ -176,9 +210,10 @@ export const buildSystemPrompt = ({ phase }: { phase: ReviewPhase }): string => } const truncateConventions = (conventions: string, conventionsBudgetTokens: number): string => { + if (conventionsRenderInFull(conventions, conventionsBudgetTokens)) return conventions + const characterCap = conventionsCharacterCap(conventionsBudgetTokens) - if (conventions.length <= characterCap) return conventions // toWellFormed: a cut mid-surrogate-pair would leave a lone surrogate, // which some HTTP stacks and providers reject in the request body return `${conventions.slice(0, characterCap).toWellFormed()}\n\n[conventions truncated at ~${conventionsBudgetTokens} tokens]` @@ -204,20 +239,19 @@ const renderFileBlock = (file: PromptFile, delimiterNonce: string): string => { // tag is tens of KB away, and models then attribute the text to a nearby file const pathAttribute = `path="${escapeAttributeValue(file.path)}"` - // Only changed files are sent diff-only, and a changed file needs no reason - // attribute to explain why it is in the prompt + const reasonAttribute = file.reason ? ` reason="${escapeAttributeValue(file.reason)}"` : "" + if (file.includedAs === "diff-only") { - return `<${fileTag} ${pathAttribute} note="full content omitted — see diff">\n` + return `<${fileTag} ${pathAttribute}${reasonAttribute} note="full content omitted — see diff">\n` } - const reasonAttribute = file.reason ? ` reason="${escapeAttributeValue(file.reason)}"` : "" return `<${fileTag} ${pathAttribute}${reasonAttribute}>\n${file.content}\n` } /** * Assembles the user message. Section order is stable → volatile so prefix - * caching can help on providers that support it: metadata, conventions, file - * contents, related files, related docs, diff, then prior bot comments and + * caching can help on providers that support it: metadata, conventions, + * changed files, related files, related docs, diff, then prior bot comments and * prior findings (most volatile — change between runs). The diff is never * truncated here — oversized PRs are skipped upstream rather than reviewed badly. */ @@ -235,9 +269,11 @@ export const buildUserPrompt = ({ delimiterNonce, }: { prContext: PrContext - /** The conventions file's text, or a placeholder sentence pointing to its - * full copy in a priority-doc file block. That block's path normalizes to - * the same file, so either way the section's path attribute names it. */ + /** The conventions file's text, or a placeholder sentence when a + * priority-doc block already carries the full text (every case is listed + * at conventionsRenderInFull). That block's path equals conventionsFile + * after posix.normalize, so either way the section's path attribute names + * the file. */ conventions: string | null /** Repo-relative path of the conventions file — the section's path attribute. */ conventionsFile: string @@ -273,7 +309,11 @@ export const buildUserPrompt = ({ const conventionsSection = conventions === null ? `<${conventionsTag}>\n(no conventions file found in this repository)\n` - : `<${conventionsTag} ${conventionsPathAttribute}>\n${truncateConventions(conventions, conventionsBudgetTokens)}\n` + : [ + `<${conventionsTag} ${conventionsPathAttribute}>`, + truncateConventions(conventions, conventionsBudgetTokens), + ``, + ].join("\n") const changedFilesSection = changedFiles .map((changedFile) => renderFileBlock(changedFile, delimiterNonce)) @@ -282,23 +322,43 @@ export const buildUserPrompt = ({ .map((relatedFile) => renderFileBlock(relatedFile, delimiterNonce)) .join("\n\n") + // The header quotes the reason string that readPriorityDocs sets + // (context/workspace.ts), so a change to that string must change the header + // too. findRelatedDocs sets "mentions " on every other doc. const relatedDocsSection = relatedDocs.length === 0 ? "" : [ - "Documentation that may describe changed code (flag any claims that have become stale):", + 'Documentation provided as context — flag any claims that have become stale. A block whose reason is "priority documentation" is included whether or not the diff touches it; every other block\'s reason names changed files it mentions:', ...relatedDocs.map((relatedDoc) => renderFileBlock(relatedDoc, delimiterNonce)), ].join("\n\n") + const diffSection = [ + `<${diffTag} note="line numbers shown are new-file line numbers">`, + annotatedDiff, + ``, + ].join("\n") + + // mergePhaseFindings (merge-phase-findings.ts) implements the + // keep-the-higher-severity rule this note states. Change the note whenever + // that rule changes. const priorFindingsSection = priorFindings.length === 0 ? "" - : `<${priorFindingsTag} note="already reported by earlier phases — do not re-report them; when a later report overlaps one of these lines, only the higher-severity finding of the two is kept">\n${JSON.stringify(priorFindings, null, 2)}\n` + : [ + `<${priorFindingsTag} note="already reported by earlier phases — do not re-report them; when a later report overlaps one of these lines, only the higher-severity finding of the two is kept">`, + JSON.stringify(priorFindings, null, 2), + ``, + ].join("\n") const priorBotCommentsSection = priorBotComments.length === 0 ? "" - : `<${priorBotCommentsTag} note="findings already posted on this PR — do not re-report the same issues, even at different locations, and do not report that any of them are now addressed">\n${priorBotComments.join("\n\n---\n\n")}\n` + : [ + `<${priorBotCommentsTag} note="findings already posted on this PR — do not re-report the same issues, even at different locations, and do not report that any of them are now addressed">`, + priorBotComments.join("\n\n---\n\n"), + ``, + ].join("\n") const sections = [ metadataSection, @@ -306,11 +366,13 @@ export const buildUserPrompt = ({ changedFilesSection, relatedFilesSection, relatedDocsSection, - `<${diffTag} note="line numbers shown are new-file line numbers">\n${annotatedDiff}\n`, + diffSection, priorBotCommentsSection, priorFindingsSection, ] + // An empty input list renders its section as "", so dropping those leaves + // no blank gap between the sections that remain return sections.filter((section) => section !== "").join("\n\n") }