Skip to content
Merged
59 changes: 57 additions & 2 deletions src/__tests__/orchestrate.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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"])
})

Expand Down Expand Up @@ -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])
Expand Down Expand Up @@ -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
Expand Down
52 changes: 49 additions & 3 deletions src/context/__tests__/workspace.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) => {
Expand Down Expand Up @@ -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")
Expand Down Expand Up @@ -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:',
'<file-abc123def456 path="README.md" reason="priority documentation">\n# Greeter\n</file-abc123def456 path="README.md">',
].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;
Expand Down
6 changes: 5 additions & 1 deletion src/orchestrate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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: [] }

Expand Down
136 changes: 124 additions & 12 deletions src/review/__tests__/prompt.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,7 @@ describe("buildUserPrompt", () => {
const conventionsIndex = userPrompt.indexOf('<conventions-abc123def456 path="AGENTS.md">')
const changedFileIndex = userPrompt.indexOf('<file-abc123def456 path="src/greeter.ts">')
const relatedFileIndex = userPrompt.indexOf('<file-abc123def456 path="src/caller.ts"')
const relatedDocsIndex = userPrompt.indexOf("Documentation that may describe changed code")
const relatedDocsIndex = userPrompt.indexOf("Documentation provided as context")
const docFileIndex = userPrompt.indexOf('<file-abc123def456 path="docs/api.md"')
const diffIndex = userPrompt.indexOf("<diff-abc123def456")

Expand All @@ -282,13 +282,21 @@ describe("buildUserPrompt", () => {
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("<diff-abc123def456")
expect(userPrompt).not.toContain("Documentation provided as context")
})

it("renders the staleness instruction header in the related docs section", () => {
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",
Expand All @@ -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:',
'<file-abc123def456 path="README.md" reason="priority documentation">\n# Greeter\n</file-abc123def456 path="README.md">',
'<file-abc123def456 path="docs/api.md" reason="mentions src/greeter.ts">\n# API\n</file-abc123def456 path="docs/api.md">',
].join("\n\n"),
)
})

Expand Down Expand Up @@ -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(
[
'<conventions-abc123def456 path="AGENTS.md">',
`${"x".repeat(31_999)}�`,
"",
"[conventions truncated at ~8000 tokens]",
'</conventions-abc123def456 path="AGENTS.md">',
].join("\n"),
)
})

it("truncates oversized conventions with an explicit notice", () => {
Expand Down Expand Up @@ -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(
'<file-abc123def456 path="src/caller.ts" reason="references changed-file src/greeter.ts" note="full content omitted — see diff">\n</file-abc123def456 path="src/caller.ts">',
)
})

it("wraps the annotated diff in the nonce-tagged diff section", () => {
const userPrompt = buildUserPrompt(makeUserPromptParts())

expect(userPrompt).toContain(
[
'<diff-abc123def456 note="line numbers shown are new-file line numbers">',
"=== src/greeter.ts ===",
"@@ -1,1 +1,1 @@",
" 1 + export const greet",
"</diff-abc123def456>",
].join("\n"),
)
})

it("omits the prior_findings section when there are none", () => {
const userPrompt = buildUserPrompt(makeUserPromptParts())

Expand All @@ -409,7 +462,15 @@ describe("buildUserPrompt", () => {
prContext: { ...prContext, body: null },
})

expect(userPrompt).toContain("PR description:\n(none)")
expect(userPrompt).toContain(
[
"<pr_metadata-abc123def456>",
"PR title: feat: trim names before greeting",
"Branch: feat/trim-names → main",
"PR description:\n(none)",
"</pr_metadata-abc123def456>",
].join("\n"),
)
})

it("includes prior findings with a do-not-re-report instruction when present", () => {
Expand All @@ -421,14 +482,32 @@ describe("buildUserPrompt", () => {
})

expect(userPrompt).toContain(
'<prior_findings-abc123def456 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">',
[
'<prior_findings-abc123def456 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">',
"[",
" {",
' "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."',
" }",
"]",
"</prior_findings-abc123def456>",
].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("<diff-abc123def456")
expect(userPrompt).not.toContain("prior_bot_comments")
})

Expand Down Expand Up @@ -475,7 +554,11 @@ describe("buildUserPrompt", () => {
const userPrompt = buildUserPrompt(makeUserPromptParts())

expect(userPrompt).toContain(
'<file-abc123def456 path="src/caller.ts" reason="references changed-file src/greeter.ts">',
[
'<file-abc123def456 path="src/caller.ts" reason="references changed-file src/greeter.ts">',
'import { greet } from "./greeter.js"',
'</file-abc123def456 path="src/caller.ts">',
].join("\n"),
)
})

Expand Down Expand Up @@ -574,7 +657,29 @@ describe("conventionsRenderInFull", () => {
})

expect(conventionsRenderInFull(oversizedConventions, budgetTokens)).toBe(false)
expect(prompt).toContain("[conventions truncated at ~8000 tokens]")
expect(prompt).toContain(
[
'<conventions-abc123def456 path="AGENTS.md">',
"c".repeat(conventionsCharacterCap),
"",
"[conventions truncated at ~8000 tokens]",
'</conventions-abc123def456 path="AGENTS.md">',
].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(
`<conventions-abc123def456 path="AGENTS.md">\n${conventionsAtCap}\n</conventions-abc123def456 path="AGENTS.md">`,
)
})

it("respects a custom budget", () => {
Expand All @@ -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(
[
'<conventions-abc123def456 path="AGENTS.md">',
"c".repeat(customCharacterCap),
"",
"[conventions truncated at ~16000 tokens]",
'</conventions-abc123def456 path="AGENTS.md">',
].join("\n"),
)
})
})
Loading
Loading