From e53f20d51bd0da9e295b1d0322ce607d3c5c8a0e Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Wed, 30 Sep 2026 17:24:47 -0400 Subject: [PATCH 1/8] fix(review): describe both doc inclusion reasons in the related-docs header Co-Authored-By: Claude Opus 5.5 --- src/review/__tests__/prompt.test.ts | 20 ++++++++++++++++---- src/review/prompt.ts | 2 +- 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/src/review/__tests__/prompt.test.ts b/src/review/__tests__/prompt.test.ts index c4dd476..d3ac262 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 — priority documentation is sent on every review, whether or not the diff touches it, and every other doc mentions a changed file; each block's reason attribute says which (flag any claims that have become stale):", + '\n# Greeter\n', + '\n# API\n', + ].join("\n\n"), ) }) diff --git a/src/review/prompt.ts b/src/review/prompt.ts index d893f88..1637e3d 100644 --- a/src/review/prompt.ts +++ b/src/review/prompt.ts @@ -286,7 +286,7 @@ export const buildUserPrompt = ({ relatedDocs.length === 0 ? "" : [ - "Documentation that may describe changed code (flag any claims that have become stale):", + "Documentation provided as context — priority documentation is sent on every review, whether or not the diff touches it, and every other doc mentions a changed file; each block's reason attribute says which (flag any claims that have become stale):", ...relatedDocs.map((relatedDoc) => renderFileBlock(relatedDoc, delimiterNonce)), ].join("\n\n") From cd6bfc0151f5b3dffe39e5c6701fd452de4405b5 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Wed, 30 Sep 2026 17:25:57 -0400 Subject: [PATCH 2/8] test: plant a stale README default for the live doc check (revert before merge) Co-Authored-By: Claude Opus 5.5 --- src/config.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/config.ts b/src/config.ts index ab92772..a8decf9 100644 --- a/src/config.ts +++ b/src/config.ts @@ -153,7 +153,7 @@ const configSchema = z.object({ maxScanFiles: positiveIntegerOrDefault(5_000), maxScanBytes: positiveIntegerOrDefault(524_288), maxRelatedFiles: positiveIntegerOrDefault(15), - maxRelatedDocs: positiveIntegerOrDefault(10), + maxRelatedDocs: positiveIntegerOrDefault(12), // Empty is not the default here. action.yml defaults this input to // README.md, and an explicit empty value disables priority docs. // normalizeWorkspacePath("") yields ".", so it is stripped with empty entries From bd1214be9452d6b2bee3e5d6df3307562a2ce869 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Wed, 30 Sep 2026 17:27:33 -0400 Subject: [PATCH 3/8] Revert "test: plant a stale README default for the live doc check (revert before merge)" This reverts commit cd6bfc0151f5b3dffe39e5c6701fd452de4405b5. --- src/config.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/config.ts b/src/config.ts index a8decf9..ab92772 100644 --- a/src/config.ts +++ b/src/config.ts @@ -153,7 +153,7 @@ const configSchema = z.object({ maxScanFiles: positiveIntegerOrDefault(5_000), maxScanBytes: positiveIntegerOrDefault(524_288), maxRelatedFiles: positiveIntegerOrDefault(15), - maxRelatedDocs: positiveIntegerOrDefault(12), + maxRelatedDocs: positiveIntegerOrDefault(10), // Empty is not the default here. action.yml defaults this input to // README.md, and an explicit empty value disables priority docs. // normalizeWorkspacePath("") yields ".", so it is stripped with empty entries From a14d5804c5274ecb79c46bc75d7ac55be9da96ff Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Wed, 30 Sep 2026 17:29:55 -0400 Subject: [PATCH 4/8] fix(review): state priority-doc inclusion without an every-review guarantee MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Priority docs are read under a token budget and skipped when missing, diff-excluded, or already sent in full by another channel, so the header no longer claims they are sent on every review. Ship-Check: pr-review · claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 --- src/review/__tests__/prompt.test.ts | 2 +- src/review/prompt.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/review/__tests__/prompt.test.ts b/src/review/__tests__/prompt.test.ts index d3ac262..074b92d 100644 --- a/src/review/__tests__/prompt.test.ts +++ b/src/review/__tests__/prompt.test.ts @@ -308,7 +308,7 @@ describe("buildUserPrompt", () => { expect(userPrompt).toContain( [ - "Documentation provided as context — priority documentation is sent on every review, whether or not the diff touches it, and every other doc mentions a changed file; each block's reason attribute says which (flag any claims that have become stale):", + "Documentation provided as context — priority documentation is included whether or not the diff touches it, and every other doc mentions a changed file; each block's reason attribute says which (flag any claims that have become stale):", '\n# Greeter\n', '\n# API\n', ].join("\n\n"), diff --git a/src/review/prompt.ts b/src/review/prompt.ts index 1637e3d..4a360b9 100644 --- a/src/review/prompt.ts +++ b/src/review/prompt.ts @@ -286,7 +286,7 @@ export const buildUserPrompt = ({ relatedDocs.length === 0 ? "" : [ - "Documentation provided as context — priority documentation is sent on every review, whether or not the diff touches it, and every other doc mentions a changed file; each block's reason attribute says which (flag any claims that have become stale):", + "Documentation provided as context — priority documentation is included whether or not the diff touches it, and every other doc mentions a changed file; each block's reason attribute says which (flag any claims that have become stale):", ...relatedDocs.map((relatedDoc) => renderFileBlock(relatedDoc, delimiterNonce)), ].join("\n\n") From 5f59075aef6b8d5c9951c1de7d8b73d31c625795 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Wed, 30 Sep 2026 17:36:16 -0400 Subject: [PATCH 5/8] style: clarify prompt.ts cross-module contracts and section assembly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Point each prompt-text contract at the code that honors it (diff headers, excluded-files trailer, non-finding filter, phase merge, doc reason strings). List every conventions full-copy case in one place, and state why the system-prompt order matters. Reuse conventionsRenderInFull in truncateConventions. Build the conventions, diff, and prior sections as tag/body/tag joins. Render the reason attribute on diff-only blocks too. The prompt output is unchanged for every input callers produce. Ship-Check: code-quality · claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 --- src/review/prompt.ts | 102 ++++++++++++++++++++++++++++++++++--------- 1 file changed, 81 insertions(+), 21 deletions(-) diff --git a/src/review/prompt.ts b/src/review/prompt.ts index 4a360b9..ccbbec6 100644 --- a/src/review/prompt.ts +++ b/src/review/prompt.ts @@ -20,22 +20,32 @@ 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, so readPriorityDocs never receives + * 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 +105,9 @@ 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) drops findings whose fields + * start with the non-finding phrases quoted here, for when the model emits + * them anyway. A phrase added here needs a matching pattern there. */ 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 +134,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 +192,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 +208,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 +237,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 +267,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 +307,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,6 +320,9 @@ export const buildUserPrompt = ({ .map((relatedFile) => renderFileBlock(relatedFile, delimiterNonce)) .join("\n\n") + // The header's two inclusion reasons are the reason strings the context + // reader sets (context/workspace.ts). readPriorityDocs sets "priority + // documentation", and findRelatedDocs sets "mentions ". const relatedDocsSection = relatedDocs.length === 0 ? "" @@ -290,15 +331,32 @@ export const buildUserPrompt = ({ ...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 +364,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") } From 69dc8481eab1399034d4aa8d9c710bce626a2406 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Wed, 30 Sep 2026 17:37:38 -0400 Subject: [PATCH 6/8] fix(review): quote the priority-doc reason string in the related-docs header MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ship-Check: triage · claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 --- src/review/__tests__/prompt.test.ts | 2 +- src/review/prompt.ts | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/review/__tests__/prompt.test.ts b/src/review/__tests__/prompt.test.ts index 074b92d..5eddb03 100644 --- a/src/review/__tests__/prompt.test.ts +++ b/src/review/__tests__/prompt.test.ts @@ -308,7 +308,7 @@ describe("buildUserPrompt", () => { expect(userPrompt).toContain( [ - "Documentation provided as context — priority documentation is included whether or not the diff touches it, and every other doc mentions a changed file; each block's reason attribute says which (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"), diff --git a/src/review/prompt.ts b/src/review/prompt.ts index ccbbec6..a3b26b2 100644 --- a/src/review/prompt.ts +++ b/src/review/prompt.ts @@ -320,14 +320,14 @@ export const buildUserPrompt = ({ .map((relatedFile) => renderFileBlock(relatedFile, delimiterNonce)) .join("\n\n") - // The header's two inclusion reasons are the reason strings the context - // reader sets (context/workspace.ts). readPriorityDocs sets "priority - // documentation", and findRelatedDocs sets "mentions ". + // 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 provided as context — priority documentation is included whether or not the diff touches it, and every other doc mentions a changed file; each block's reason attribute says which (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") From 68c538aedbf413d8c212f2cd2b31dda2091a2835 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Wed, 30 Sep 2026 17:40:46 -0400 Subject: [PATCH 7/8] test(review): assert prompt sections whole and guard the priority-doc reason quote MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - A diff-only file block keeps its reason attribute. - The diff and prior-findings sections are asserted as whole sections. - Conventions exactly at the cap render in full, matching conventionsRenderInFull. - Truncation, metadata and related-file tests assert whole sections, not fragments. - The related-docs header's quoted reason is checked against what readPriorityDocs sets. Ship-Check: test-audit · claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 --- src/context/__tests__/workspace.test.ts | 52 ++++++++++- src/review/__tests__/prompt.test.ts | 116 ++++++++++++++++++++++-- 2 files changed, 157 insertions(+), 11 deletions(-) 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/review/__tests__/prompt.test.ts b/src/review/__tests__/prompt.test.ts index 5eddb03..6c0bc0b 100644 --- a/src/review/__tests__/prompt.test.ts +++ b/src/review/__tests__/prompt.test.ts @@ -374,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", () => { @@ -407,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()) @@ -421,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", () => { @@ -433,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"), ) }) @@ -586,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", () => { @@ -608,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"), + ) }) }) From 2ff5b6ac7e4a3131238c7f01813f58636beae646 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Wed, 30 Sep 2026 17:47:18 -0400 Subject: [PATCH 8/8] fix: keep a fully rendered conventions file out of the related-file scan MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The related-file scan did not receive the conventions path when the conventions section carried the whole file, so a JS/TS conventions file that imports a changed file was sent twice. Correct two prompt.ts comments: readPriorityDocs skips the conventions file by exclusion rather than never receiving it, and filterNonFindings covers most, not all, of the quoted non-finding phrases. Ship-Check: bug-check · claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 --- src/__tests__/orchestrate.test.ts | 59 +++++++++++++++++++++++++++++-- src/orchestrate.ts | 6 +++- src/review/prompt.ts | 12 ++++--- 3 files changed, 69 insertions(+), 8 deletions(-) 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/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/prompt.ts b/src/review/prompt.ts index a3b26b2..87aac2a 100644 --- a/src/review/prompt.ts +++ b/src/review/prompt.ts @@ -31,8 +31,9 @@ export const conventionsCharacterCap = (conventionsBudgetTokens: number): number * 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, so readPriorityDocs never receives - * the file and a changed conventions file's block is sent diff-only. + * - 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 @@ -105,9 +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) drops findings whose fields - * start with the non-finding phrases quoted here, for when the model emits - * them anyway. A phrase added here needs a matching pattern there. */ +/** 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