From 33da03e172f92d84895eb1a775f18aa14205aeda Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Wed, 30 Sep 2026 18:07:38 -0400 Subject: [PATCH 01/14] fix(review): keep findings whose file copies an escaped path attribute The prompt writes a double quote in a path attribute as ", and the model copies that attribute into a finding's file. The unknown-file filter compared that spelling against the raw paths and dropped the finding. It now keeps a finding whose file matches only after decoding ", rewrites file to the decoded path, and logs each rewrite at debug level. A path that matches as written is never rewritten. Co-Authored-By: Claude Opus 5.5 --- README.md | 6 +- src/__tests__/orchestrate.test.ts | 72 +++++++++ src/orchestrate.ts | 13 +- .../filter-unknown-file-findings.test.ts | 139 ++++++++++++++++-- src/review/filter-unknown-file-findings.ts | 65 ++++++-- src/review/prompt.ts | 8 +- 6 files changed, 279 insertions(+), 24 deletions(-) diff --git a/README.md b/README.md index 7d91856..f505676 100644 --- a/README.md +++ b/README.md @@ -139,7 +139,11 @@ The patterns are deliberately anchored (start-of-field, end-of-field, or separat ## Unknown-file filter -A finding's `file` must name a file the model was given: a diff header (including renamed-from and deleted paths), a changed, related, or priority-doc file block, or the conventions file. A finding on any other path is ungrounded — the model saw nothing there — and is dropped before threshold, dedup, and cap. Paths are normalized before comparison (`./src/x.ts` and `src/x.ts` match), but only whole paths match: a bare filename or a directory prefix does not. Each drop is logged as a warning (`dropping finding: file not in prompt context`) with the file, line, and category; the per-run count appears in the job summary. +A finding's `file` must name a file the model was given: a diff header (including renamed-from and deleted paths), a changed, related, or priority-doc file block, or the conventions file. A finding on any other path is ungrounded — the model saw nothing there — and is dropped before threshold, dedup, and cap. + +- Paths are normalized before comparison (`./src/x.ts` and `src/x.ts` match), but only whole paths match: a bare filename or a directory prefix does not. +- File blocks and the conventions section write a `"` in their path as `"`. A `file` that matches only after each `"` is read as `"` is kept and posts under that decoded path. Each rewrite is logged at debug level (`resolved escaped finding file to a prompt path`) with both spellings. +- Each drop is logged as a warning (`dropping finding: file not in prompt context`) with the file, line, and category; the per-run count appears in the job summary. ## Debug logging diff --git a/src/__tests__/orchestrate.test.ts b/src/__tests__/orchestrate.test.ts index 24a422c..0e4a1dd 100644 --- a/src/__tests__/orchestrate.test.ts +++ b/src/__tests__/orchestrate.test.ts @@ -838,6 +838,78 @@ describe("orchestrate", () => { ]) }) + it("keeps a finding on an escaped related-doc path under the decoded path and drops one no spelling matches", async () => { + const escapedFinding = makeFinding({ file: "docs/a"b.md", line: 3 }) + const unknownFinding = makeFinding({ file: "docs/c"d.md", line: 5 }) + const stubs = makeOrchestrateDeps({ + fixtureResult: { + review: { analysis: "checked", findings: [escapedFinding, unknownFinding] }, + }, + contextReader: { + findRelatedDocs: async () => ({ + files: [ + { + path: 'docs/a"b.md', + content: "# Greeter\n\nCall greet() with a name.", + includedAs: "full", + reason: "mentions src/greeter.ts", + }, + ], + excludedByCapPaths: [], + }), + }, + }) + const logger = createTestLogger() + + await orchestrate(stubs.deps, logger) + + expect(stubs.postIssueCommentCalls).toEqual([ + { + prNumber: 7, + body: renderBeyondDiffFinding( + withRoutedModel({ ...escapedFinding, file: 'docs/a"b.md' }, "test/model"), + ), + }, + ]) + expect(logsWithMessage(logger, "resolved escaped finding file to a prompt path")).toEqual([ + { + level: "debug", + message: "resolved escaped finding file to a prompt path", + data: { + phase: "combined", + writtenFile: "docs/a"b.md", + resolvedFile: 'docs/a"b.md', + line: 3, + }, + }, + ]) + expect(logsWithMessage(logger, "dropping finding: file not in prompt context")).toEqual([ + { + level: "warn", + message: "dropping finding: file not in prompt context", + data: { + phase: "combined", + file: "docs/c"d.md", + line: 5, + category: unknownFinding.category, + }, + }, + ]) + expect(logsWithMessage(logger, "non-finding filter applied to model output")).toEqual([ + { + level: "info", + message: "non-finding filter applied to model output", + data: { + totalFromModel: 2, + kept: 1, + droppedAsNonFinding: 0, + droppedAsUnknownFile: 1, + duplicatesAcrossPhases: 0, + }, + }, + ]) + }) + it("excludes files the repo marks linguist-generated", async () => { const stubs = makeOrchestrateDeps({ contextReader: { diff --git a/src/orchestrate.ts b/src/orchestrate.ts index bb78a6f..7203ecb 100644 --- a/src/orchestrate.ts +++ b/src/orchestrate.ts @@ -441,7 +441,7 @@ const filterPhaseFindings = ( const { findings: nonFindingFiltered, droppedAsNonFinding } = filterNonFindings( outcome.result.review.findings, ) - const { findings, droppedAsUnknownFile } = filterUnknownFileFindings({ + const { findings, droppedAsUnknownFile, unescapedFileRewrites } = filterUnknownFileFindings({ findings: nonFindingFiltered, knownPaths, }) @@ -456,6 +456,17 @@ const filterPhaseFindings = ( category: finding.category, }) } + + // A rewritten file changes where the finding posts and its dedup anchor, so + // the log keeps both spellings + for (const { writtenFile, finding } of unescapedFileRewrites) { + logger.debug("resolved escaped finding file to a prompt path", { + phase: outcome.phase.id, + writtenFile, + resolvedFile: finding.file, + line: finding.line, + }) + } return { findings: findings.map((finding) => ({ ...finding, diff --git a/src/review/__tests__/filter-unknown-file-findings.test.ts b/src/review/__tests__/filter-unknown-file-findings.test.ts index b60fbe6..e7bf201 100644 --- a/src/review/__tests__/filter-unknown-file-findings.test.ts +++ b/src/review/__tests__/filter-unknown-file-findings.test.ts @@ -1,7 +1,18 @@ import { describe, expect, it } from "vitest" import { filterUnknownFileFindings } from "../filter-unknown-file-findings.js" +import { buildUserPrompt } from "../prompt.js" import { makeFinding } from "./make-finding.js" +/** The path attribute of the first file block, exactly as the model reads it. */ +const FILE_BLOCK_PATH_ATTRIBUTE = / { + const path = FILE_BLOCK_PATH_ATTRIBUTE.exec(userPrompt)?.groups?.path + + if (!path) throw new Error("the prompt has no file block path attribute") + return path +} + describe("filterUnknownFileFindings", () => { it("keeps a finding whose file is a known path", () => { const finding = makeFinding() @@ -11,7 +22,7 @@ describe("filterUnknownFileFindings", () => { findings: [finding], knownPaths: ["src/greeter.ts"], }), - ).toEqual({ findings: [finding], droppedAsUnknownFile: [] }) + ).toEqual({ findings: [finding], droppedAsUnknownFile: [], unescapedFileRewrites: [] }) }) it("drops a finding whose file is a known path with prose appended", () => { @@ -26,7 +37,7 @@ describe("filterUnknownFileFindings", () => { findings: [finding], knownPaths: ["deploy/railway/README.md"], }), - ).toEqual({ findings: [], droppedAsUnknownFile: [finding] }) + ).toEqual({ findings: [], droppedAsUnknownFile: [finding], unescapedFileRewrites: [] }) }) it("drops a finding whose file matches no known path", () => { @@ -37,7 +48,7 @@ describe("filterUnknownFileFindings", () => { findings: [finding], knownPaths: ["src/greeter.ts"], }), - ).toEqual({ findings: [], droppedAsUnknownFile: [finding] }) + ).toEqual({ findings: [], droppedAsUnknownFile: [finding], unescapedFileRewrites: [] }) }) it("filters selectively in a mixed set, preserving order", () => { @@ -50,7 +61,11 @@ describe("filterUnknownFileFindings", () => { findings: [first, unknown, third], knownPaths: ["src/greeter.ts"], }), - ).toEqual({ findings: [first, third], droppedAsUnknownFile: [unknown] }) + ).toEqual({ + findings: [first, third], + droppedAsUnknownFile: [unknown], + unescapedFileRewrites: [], + }) }) it("matches a ./-prefixed finding file against the bare known path without rewriting it", () => { @@ -61,7 +76,7 @@ describe("filterUnknownFileFindings", () => { findings: [finding], knownPaths: ["src/greeter.ts"], }), - ).toEqual({ findings: [finding], droppedAsUnknownFile: [] }) + ).toEqual({ findings: [finding], droppedAsUnknownFile: [], unescapedFileRewrites: [] }) expect(finding.file).toBe("./src/greeter.ts") }) @@ -73,7 +88,7 @@ describe("filterUnknownFileFindings", () => { findings: [finding], knownPaths: ["./src/greeter.ts"], }), - ).toEqual({ findings: [finding], droppedAsUnknownFile: [] }) + ).toEqual({ findings: [finding], droppedAsUnknownFile: [], unescapedFileRewrites: [] }) }) it("normalizes redundant segments before comparing", () => { @@ -84,7 +99,7 @@ describe("filterUnknownFileFindings", () => { findings: [finding], knownPaths: ["src/greeter.ts"], }), - ).toEqual({ findings: [finding], droppedAsUnknownFile: [] }) + ).toEqual({ findings: [finding], droppedAsUnknownFile: [], unescapedFileRewrites: [] }) }) it("ignores surrounding whitespace in the finding file", () => { @@ -95,7 +110,7 @@ describe("filterUnknownFileFindings", () => { findings: [finding], knownPaths: ["src/greeter.ts"], }), - ).toEqual({ findings: [finding], droppedAsUnknownFile: [] }) + ).toEqual({ findings: [finding], droppedAsUnknownFile: [], unescapedFileRewrites: [] }) }) it("keeps a beyond-diff finding on a related file", () => { @@ -106,7 +121,106 @@ describe("filterUnknownFileFindings", () => { findings: [finding], knownPaths: ["src/greeter.ts", "src/caller.ts"], }), - ).toEqual({ findings: [finding], droppedAsUnknownFile: [] }) + ).toEqual({ findings: [finding], droppedAsUnknownFile: [], unescapedFileRewrites: [] }) + }) + + it("keeps a finding whose file escapes a known path's quote as ", under the decoded path", () => { + const escapedFinding = makeFinding({ file: "docs/a"b.md", line: 12 }) + const unrelatedFinding = makeFinding({ line: 30 }) + const resolvedFinding = { ...escapedFinding, file: 'docs/a"b.md' } + + expect( + filterUnknownFileFindings({ + findings: [escapedFinding, unrelatedFinding], + knownPaths: ["src/greeter.ts", 'docs/a"b.md'], + }), + ).toEqual({ + findings: [resolvedFinding, unrelatedFinding], + droppedAsUnknownFile: [], + unescapedFileRewrites: [{ writtenFile: "docs/a"b.md", finding: resolvedFinding }], + }) + expect(escapedFinding.file).toBe("docs/a"b.md") + }) + + it("decodes only " and keeps the rest of the written spelling", () => { + const escapedFinding = makeFinding({ file: "./docs/a"b.md" }) + const resolvedFinding = { ...escapedFinding, file: './docs/a"b.md' } + + expect( + filterUnknownFileFindings({ + findings: [escapedFinding], + knownPaths: ['docs/a"b.md'], + }), + ).toEqual({ + findings: [resolvedFinding], + droppedAsUnknownFile: [], + unescapedFileRewrites: [{ writtenFile: "./docs/a"b.md", finding: resolvedFinding }], + }) + }) + + it("keeps a known path containing a literal " as written when its decoded spelling is also known", () => { + const finding = makeFinding({ file: "docs/a"b.md" }) + + expect( + filterUnknownFileFindings({ + findings: [finding], + knownPaths: ['docs/a"b.md', "docs/a"b.md"], + }), + ).toEqual({ findings: [finding], droppedAsUnknownFile: [], unescapedFileRewrites: [] }) + }) + + it("drops a finding whose file matches no known path before or after decoding "", () => { + const finding = makeFinding({ file: "docs/c"d.md" }) + + expect( + filterUnknownFileFindings({ + findings: [finding], + knownPaths: ['docs/a"b.md', "docs/cd.md"], + }), + ).toEqual({ findings: [], droppedAsUnknownFile: [finding], unescapedFileRewrites: [] }) + }) + + it("keeps a finding whose file is copied from a rendered path attribute", () => { + const relatedDoc = { + path: 'docs/a"b.md', + content: "# Quoted", + includedAs: "full" as const, + reason: "mentions src/greeter.ts", + } + const userPrompt = buildUserPrompt({ + prContext: { + prNumber: 7, + title: "fix: trim names", + body: null, + headSha: "abc123", + headRef: "fix/trim-names", + baseRef: "main", + }, + conventions: null, + conventionsFile: "AGENTS.md", + conventionsBudgetTokens: 8_000, + changedFiles: [], + relatedFiles: [], + relatedDocs: [relatedDoc], + annotatedDiff: "", + priorFindings: [], + priorBotComments: [], + delimiterNonce: "abc123def456", + }) + const writtenFile = getFileBlockPathAttribute(userPrompt) + const finding = makeFinding({ file: writtenFile, line: 1 }) + const resolvedFinding = { ...finding, file: 'docs/a"b.md' } + + expect( + filterUnknownFileFindings({ + findings: [finding], + knownPaths: [relatedDoc.path], + }), + ).toEqual({ + findings: [resolvedFinding], + droppedAsUnknownFile: [], + unescapedFileRewrites: [{ writtenFile, finding: resolvedFinding }], + }) }) it("does not match on basename alone", () => { @@ -117,7 +231,7 @@ describe("filterUnknownFileFindings", () => { findings: [finding], knownPaths: ["src/greeter.ts"], }), - ).toEqual({ findings: [], droppedAsUnknownFile: [finding] }) + ).toEqual({ findings: [], droppedAsUnknownFile: [finding], unescapedFileRewrites: [] }) }) it("does not match on a directory prefix", () => { @@ -128,7 +242,7 @@ describe("filterUnknownFileFindings", () => { findings: [finding], knownPaths: ["src/greeter.ts"], }), - ).toEqual({ findings: [], droppedAsUnknownFile: [finding] }) + ).toEqual({ findings: [], droppedAsUnknownFile: [finding], unescapedFileRewrites: [] }) }) it("drops every finding when no paths are known", () => { @@ -137,6 +251,7 @@ describe("filterUnknownFileFindings", () => { expect(filterUnknownFileFindings({ findings: [finding], knownPaths: [] })).toEqual({ findings: [], droppedAsUnknownFile: [finding], + unescapedFileRewrites: [], }) }) @@ -146,6 +261,6 @@ describe("filterUnknownFileFindings", () => { findings: [], knownPaths: ["src/greeter.ts"], }), - ).toEqual({ findings: [], droppedAsUnknownFile: [] }) + ).toEqual({ findings: [], droppedAsUnknownFile: [], unescapedFileRewrites: [] }) }) }) diff --git a/src/review/filter-unknown-file-findings.ts b/src/review/filter-unknown-file-findings.ts index 4521549..366aba8 100644 --- a/src/review/filter-unknown-file-findings.ts +++ b/src/review/filter-unknown-file-findings.ts @@ -1,17 +1,38 @@ import type { Finding } from "./finding.js" +import { unescapeAttributeValue } from "./prompt.js" import { normalizeWorkspacePath } from "./workspace-path.js" +/** A kept finding whose `file` was rewritten from its escaped spelling. */ +export type UnescapedFileRewrite = { + /** `file` exactly as the model wrote it, `"` included. */ + writtenFile: string + finding: Finding +} + export type UnknownFileFilterResult = { findings: Finding[] /** Whole findings, not a count — the caller logs each drop. */ droppedAsUnknownFile: Finding[] + /** Each rewritten finding is also in `findings`. The caller logs each rewrite. */ + unescapedFileRewrites: UnescapedFileRewrite[] } -/** Drops findings whose `file` names no file the model was given. A finding - * on a path the model never saw is ungrounded by construction; without this - * gate it would route to a beyond-diff comment. Exact membership after - * normalization — never basename or prefix matching. `file` is left as the - * model wrote it because cross-run dedup anchors key on it. */ +type FileResolution = + | { kind: "known"; finding: Finding } + | { kind: "unescaped"; finding: Finding; writtenFile: string } + | { kind: "unknown"; finding: Finding } + +/** + * Drops findings whose `file` names no file the model was given. A finding on + * a path the model never saw is ungrounded by construction; without this gate + * it would route to a beyond-diff comment. + * - Membership is exact after normalization, never by basename or prefix. + * - A `file` that matches as written is kept unchanged, even when it contains + * `"`, because cross-run dedup anchors key on it. + * - A `file` that matches only after each `"` becomes `"` is kept with + * that decoded spelling. Path attributes in the prompt escape `"` as + * `"`, and the model copies the attribute verbatim. + */ export const filterUnknownFileFindings = ({ findings, knownPaths, @@ -20,12 +41,38 @@ export const filterUnknownFileFindings = ({ knownPaths: string[] }): UnknownFileFilterResult => { const knownPathSet = new Set(knownPaths.map(normalizeWorkspacePath)) - const isKnownFile = (finding: Finding): boolean => { - return knownPathSet.has(normalizeWorkspacePath(finding.file)) + const isKnownPath = (filePath: string): boolean => { + return knownPathSet.has(normalizeWorkspacePath(filePath)) + } + + // The written spelling is checked first, so a known path that literally + // contains """ is never rewritten + const resolveFile = (finding: Finding): FileResolution => { + if (isKnownPath(finding.file)) return { kind: "known", finding } + + const unescapedFile = unescapeAttributeValue(finding.file) + + if (isKnownPath(unescapedFile)) { + return { + kind: "unescaped", + finding: { ...finding, file: unescapedFile }, + writtenFile: finding.file, + } + } + return { kind: "unknown", finding } } + const resolutions = findings.map(resolveFile) + return { - findings: findings.filter(isKnownFile), - droppedAsUnknownFile: findings.filter((finding) => !isKnownFile(finding)), + findings: resolutions + .filter((resolution) => resolution.kind !== "unknown") + .map((resolution) => resolution.finding), + droppedAsUnknownFile: resolutions + .filter((resolution) => resolution.kind === "unknown") + .map((resolution) => resolution.finding), + unescapedFileRewrites: resolutions + .filter((resolution) => resolution.kind === "unescaped") + .map(({ finding, writtenFile }) => ({ writtenFile, finding })), } } diff --git a/src/review/prompt.ts b/src/review/prompt.ts index d893f88..a7d71a7 100644 --- a/src/review/prompt.ts +++ b/src/review/prompt.ts @@ -194,9 +194,15 @@ const truncateConventions = (conventions: string, conventionsBudgetTokens: numbe export const generateDelimiterNonce = (): string => randomBytes(6).toString("hex") /** A double quote would terminate the surrounding attribute — nothing else is - * structural inside a quoted attribute value. */ + * structural inside a quoted attribute value. Change it together with + * unescapeAttributeValue, which inverts it. */ const escapeAttributeValue = (value: string): string => value.replaceAll('"', """) +/** Inverts escapeAttributeValue for a path the model copied out of a path + * attribute. It decodes only the one entity escapeAttributeValue writes, never + * general HTML entities. */ +export const unescapeAttributeValue = (value: string): string => value.replaceAll(""", '"') + const renderFileBlock = (file: PromptFile, delimiterNonce: string): string => { const fileTag = `file-${delimiterNonce}` From 1d36e06444cfd5713b1484c8498691a55ce7776b Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Wed, 30 Sep 2026 18:09:06 -0400 Subject: [PATCH 02/14] test(orchestrate): group the escaped-path posting test with the beyond-diff tests Co-Authored-By: Claude Opus 5.5 --- src/__tests__/orchestrate.test.ts | 144 +++++++++++++++--------------- 1 file changed, 72 insertions(+), 72 deletions(-) diff --git a/src/__tests__/orchestrate.test.ts b/src/__tests__/orchestrate.test.ts index 0e4a1dd..72e3f83 100644 --- a/src/__tests__/orchestrate.test.ts +++ b/src/__tests__/orchestrate.test.ts @@ -838,78 +838,6 @@ describe("orchestrate", () => { ]) }) - it("keeps a finding on an escaped related-doc path under the decoded path and drops one no spelling matches", async () => { - const escapedFinding = makeFinding({ file: "docs/a"b.md", line: 3 }) - const unknownFinding = makeFinding({ file: "docs/c"d.md", line: 5 }) - const stubs = makeOrchestrateDeps({ - fixtureResult: { - review: { analysis: "checked", findings: [escapedFinding, unknownFinding] }, - }, - contextReader: { - findRelatedDocs: async () => ({ - files: [ - { - path: 'docs/a"b.md', - content: "# Greeter\n\nCall greet() with a name.", - includedAs: "full", - reason: "mentions src/greeter.ts", - }, - ], - excludedByCapPaths: [], - }), - }, - }) - const logger = createTestLogger() - - await orchestrate(stubs.deps, logger) - - expect(stubs.postIssueCommentCalls).toEqual([ - { - prNumber: 7, - body: renderBeyondDiffFinding( - withRoutedModel({ ...escapedFinding, file: 'docs/a"b.md' }, "test/model"), - ), - }, - ]) - expect(logsWithMessage(logger, "resolved escaped finding file to a prompt path")).toEqual([ - { - level: "debug", - message: "resolved escaped finding file to a prompt path", - data: { - phase: "combined", - writtenFile: "docs/a"b.md", - resolvedFile: 'docs/a"b.md', - line: 3, - }, - }, - ]) - expect(logsWithMessage(logger, "dropping finding: file not in prompt context")).toEqual([ - { - level: "warn", - message: "dropping finding: file not in prompt context", - data: { - phase: "combined", - file: "docs/c"d.md", - line: 5, - category: unknownFinding.category, - }, - }, - ]) - expect(logsWithMessage(logger, "non-finding filter applied to model output")).toEqual([ - { - level: "info", - message: "non-finding filter applied to model output", - data: { - totalFromModel: 2, - kept: 1, - droppedAsNonFinding: 0, - droppedAsUnknownFile: 1, - duplicatesAcrossPhases: 0, - }, - }, - ]) - }) - it("excludes files the repo marks linguist-generated", async () => { const stubs = makeOrchestrateDeps({ contextReader: { @@ -2722,6 +2650,78 @@ describe("orchestrate", () => { ]) }) + it("keeps a finding on an escaped related-doc path under the decoded path and drops one no spelling matches", async () => { + const escapedFinding = makeFinding({ file: "docs/a"b.md", line: 3 }) + const unknownFinding = makeFinding({ file: "docs/c"d.md", line: 5 }) + const stubs = makeOrchestrateDeps({ + fixtureResult: { + review: { analysis: "checked", findings: [escapedFinding, unknownFinding] }, + }, + contextReader: { + findRelatedDocs: async () => ({ + files: [ + { + path: 'docs/a"b.md', + content: "# Greeter\n\nCall greet() with a name.", + includedAs: "full", + reason: "mentions src/greeter.ts", + }, + ], + excludedByCapPaths: [], + }), + }, + }) + const logger = createTestLogger() + + await orchestrate(stubs.deps, logger) + + expect(stubs.postIssueCommentCalls).toEqual([ + { + prNumber: 7, + body: renderBeyondDiffFinding( + withRoutedModel({ ...escapedFinding, file: 'docs/a"b.md' }, "test/model"), + ), + }, + ]) + expect(logsWithMessage(logger, "resolved escaped finding file to a prompt path")).toEqual([ + { + level: "debug", + message: "resolved escaped finding file to a prompt path", + data: { + phase: "combined", + writtenFile: "docs/a"b.md", + resolvedFile: 'docs/a"b.md', + line: 3, + }, + }, + ]) + expect(logsWithMessage(logger, "dropping finding: file not in prompt context")).toEqual([ + { + level: "warn", + message: "dropping finding: file not in prompt context", + data: { + phase: "combined", + file: "docs/c"d.md", + line: 5, + category: unknownFinding.category, + }, + }, + ]) + expect(logsWithMessage(logger, "non-finding filter applied to model output")).toEqual([ + { + level: "info", + message: "non-finding filter applied to model output", + data: { + totalFromModel: 2, + kept: 1, + droppedAsNonFinding: 0, + droppedAsUnknownFile: 1, + duplicatesAcrossPhases: 0, + }, + }, + ]) + }) + it("continues when the findings review post throws", async () => { const stubs = makeOrchestrateDeps({ githubClient: { From 0fad946de2118a15e2614cecc4b269cfb2f51b22 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Wed, 30 Sep 2026 18:14:08 -0400 Subject: [PATCH 03/14] docs(review): state that the attribute decoder is not a true inverse MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ship-Check: pr-review · claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 --- src/review/prompt.ts | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/review/prompt.ts b/src/review/prompt.ts index a7d71a7..7eed0ef 100644 --- a/src/review/prompt.ts +++ b/src/review/prompt.ts @@ -195,12 +195,13 @@ export const generateDelimiterNonce = (): string => randomBytes(6).toString("hex /** A double quote would terminate the surrounding attribute — nothing else is * structural inside a quoted attribute value. Change it together with - * unescapeAttributeValue, which inverts it. */ + * unescapeAttributeValue, which decodes it. */ const escapeAttributeValue = (value: string): string => value.replaceAll('"', """) -/** Inverts escapeAttributeValue for a path the model copied out of a path - * attribute. It decodes only the one entity escapeAttributeValue writes, never - * general HTML entities. */ +/** Decodes the one entity escapeAttributeValue writes, never general HTML + * entities. It is not a true inverse. The encoder leaves "&" alone, so a path + * holding a literal """ decodes to a different path, and callers must try + * the written spelling first. */ export const unescapeAttributeValue = (value: string): string => value.replaceAll(""", '"') const renderFileBlock = (file: PromptFile, delimiterNonce: string): string => { From f0d565033edad0c2aacc1ead7f736a0b29293d27 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Wed, 30 Sep 2026 18:21:44 -0400 Subject: [PATCH 04/14] style: clear readability pauses in the unknown-file filter and its callers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Name the normalization the filter applies, state that kept findings keep the model's spelling, and give the real reason the written spelling is checked first. - State why only a double quote is escaped in path attributes, and link the file-anchoring instruction to the decode in the filter. - Destructure the dropped findings under their own name so the returned count no longer shares it, and state why drops warn while rewrites log at debug. - Make the prompt-path comment true for renamed paths and note which normalization the filter applies; restate the deadline-coverage comment. - Drop a length check that the following .some() already covers. - Rewrite the README escaped-path bullet to lead with the scenario. Ship-Check: code-quality · claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 --- README.md | 2 +- src/orchestrate.ts | 46 +++++++++++++++------- src/review/filter-unknown-file-findings.ts | 20 ++++++---- src/review/prompt.ts | 25 +++++++----- 4 files changed, 59 insertions(+), 34 deletions(-) diff --git a/README.md b/README.md index f505676..29f2d68 100644 --- a/README.md +++ b/README.md @@ -142,7 +142,7 @@ The patterns are deliberately anchored (start-of-field, end-of-field, or separat A finding's `file` must name a file the model was given: a diff header (including renamed-from and deleted paths), a changed, related, or priority-doc file block, or the conventions file. A finding on any other path is ungrounded — the model saw nothing there — and is dropped before threshold, dedup, and cap. - Paths are normalized before comparison (`./src/x.ts` and `src/x.ts` match), but only whole paths match: a bare filename or a directory prefix does not. -- File blocks and the conventions section write a `"` in their path as `"`. A `file` that matches only after each `"` is read as `"` is kept and posts under that decoded path. Each rewrite is logged at debug level (`resolved escaped finding file to a prompt path`) with both spellings. +- A path containing `"` reaches the model with each `"` written as `"` in the tags that wrap each file's content and the conventions file; diff headers print the path unchanged. A model that copies the path from a tag writes `"` into `file`, so a `file` that matches no given path as written, but matches once each `"` becomes `"`, is kept and posts under that decoded path. Each such rewrite is logged at debug level (`resolved escaped finding file to a prompt path`) with the model's spelling and the decoded path. - Each drop is logged as a warning (`dropping finding: file not in prompt context`) with the file, line, and category; the per-run count appears in the job summary. ## Debug logging diff --git a/src/orchestrate.ts b/src/orchestrate.ts index 7203ecb..86b245f 100644 --- a/src/orchestrate.ts +++ b/src/orchestrate.ts @@ -433,22 +433,29 @@ type FilteredPhaseFindings = { droppedAsUnknownFile: number } -/** Drops non-findings and findings on files the model never saw. */ +/** Drops non-findings and findings on files the model never saw. A kept + * finding whose `file` matched only once `"` was decoded comes back + * with the decoded `file`. */ const filterPhaseFindings = ( { outcome, knownPaths }: { outcome: CompletedPhase; knownPaths: string[] }, logger: Logger, ): FilteredPhaseFindings => { - const { findings: nonFindingFiltered, droppedAsNonFinding } = filterNonFindings( + const { findings: realFindings, droppedAsNonFinding } = filterNonFindings( outcome.result.review.findings, ) - const { findings, droppedAsUnknownFile, unescapedFileRewrites } = filterUnknownFileFindings({ - findings: nonFindingFiltered, + const { + findings, + droppedAsUnknownFile: unknownFileFindings, + unescapedFileRewrites, + } = filterUnknownFileFindings({ + findings: realFindings, knownPaths, }) // Each drop warns on its own because it is a model-quality event, not loop - // chatter. The title is omitted because it may be garbage. - for (const finding of droppedAsUnknownFile) { + // chatter. The title is left out because the model wrote it about a file it + // was never given, so it is unverified text. + for (const finding of unknownFileFindings) { logger.warn("dropping finding: file not in prompt context", { phase: outcome.phase.id, file: finding.file, @@ -458,7 +465,8 @@ const filterPhaseFindings = ( } // A rewritten file changes where the finding posts and its dedup anchor, so - // the log keeps both spellings + // the log keeps both spellings. It logs at debug, not warn, because the + // finding still posts, on the path the prompt gave. for (const { writtenFile, finding } of unescapedFileRewrites) { logger.debug("resolved escaped finding file to a prompt path", { phase: outcome.phase.id, @@ -467,13 +475,14 @@ const filterPhaseFindings = ( line: finding.line, }) } + return { findings: findings.map((finding) => ({ ...finding, modelUsed: outcome.result.modelUsed, })), droppedAsNonFinding, - droppedAsUnknownFile: droppedAsUnknownFile.length, + droppedAsUnknownFile: unknownFileFindings.length, } } @@ -737,9 +746,9 @@ const runReviewPipeline = async ( .map((file) => posix.normalize(file.path)), ...conventionsFullCopyPaths.map((conventionsPath) => posix.normalize(conventionsPath)), ]) - const needsPriorityDocFloor = - reviewablePriorityDocs.length > 0 && - reviewablePriorityDocs.some((docPath) => !preFloorInContext.has(posix.normalize(docPath))) + const needsPriorityDocFloor = reviewablePriorityDocs.some( + (docPath) => !preFloorInContext.has(posix.normalize(docPath)), + ) const remainingPriorityDocFloor = needsPriorityDocFloor ? Math.min(priorityDocFloorLimit - earlyPriorityDocTokens, remainingTokens) : 0 @@ -878,9 +887,14 @@ const runReviewPipeline = async ( const relatedDocs = [...priorityDocFiles, ...mentionMatchedDocsResult.files] - // Every path the model can see: diff headers (deleted files render a - // header but have no new path, so they are added here), file blocks, and - // the conventions section when the file was found. + // Every path the model can see: + // - Diff headers. changedPaths holds each new path and a rename's old path, + // which the header prints as "renamed from". A deleted file's header + // names a path changedPaths lacks, so it is added here. + // - File blocks, and the conventions section when the file was found. + // These paths go in as written: filterUnknownFileFindings normalizes both + // sides with normalizeWorkspacePath, which also strips a leading or + // trailing "/" that the posix.normalize sets above keep. const deletedPaths = reviewableFiles.flatMap((file) => { return file.deleted && file.from ? [file.from] : [] }) @@ -973,7 +987,9 @@ const runReviewPipeline = async ( ) const completedPhases = phaseOutcomes.filter((outcome) => outcome.status === "completed") const phases = phaseOutcomes.map(describePhaseOutcome) - /** Cost lookup expiry alone does not lose review coverage. */ + + // Only a phase that failed on the deadline lost coverage. A completed phase + // whose cost lookup the deadline skipped still delivered its findings. const coverageLostToReviewDeadline = phaseOutcomes.some( (outcome) => outcome.status === "failed" && outcome.deadlineExceeded, ) diff --git a/src/review/filter-unknown-file-findings.ts b/src/review/filter-unknown-file-findings.ts index 366aba8..1f64eb4 100644 --- a/src/review/filter-unknown-file-findings.ts +++ b/src/review/filter-unknown-file-findings.ts @@ -2,7 +2,8 @@ import type { Finding } from "./finding.js" import { unescapeAttributeValue } from "./prompt.js" import { normalizeWorkspacePath } from "./workspace-path.js" -/** A kept finding whose `file` was rewritten from its escaped spelling. */ +/** A kept finding whose `file` matched a known path only after each `"` + * in it was decoded to `"`. */ export type UnescapedFileRewrite = { /** `file` exactly as the model wrote it, `"` included. */ writtenFile: string @@ -26,12 +27,15 @@ type FileResolution = * Drops findings whose `file` names no file the model was given. A finding on * a path the model never saw is ungrounded by construction; without this gate * it would route to a beyond-diff comment. - * - Membership is exact after normalization, never by basename or prefix. - * - A `file` that matches as written is kept unchanged, even when it contains - * `"`, because cross-run dedup anchors key on it. + * - `file` and each known path pass through normalizeWorkspacePath (trim, + * resolve `.` and `..` segments, collapse repeated `/`, strip a leading or + * trailing `/`). After that, membership is exact, never by basename or + * prefix. + * - Only the comparison is normalized. A `file` that matches as written is + * kept exactly as the model wrote it. * - A `file` that matches only after each `"` becomes `"` is kept with - * that decoded spelling. Path attributes in the prompt escape `"` as - * `"`, and the model copies the attribute verbatim. + * that decoded spelling. File-block and conventions path attributes escape + * `"` as `"`, and the model copies the attribute verbatim. */ export const filterUnknownFileFindings = ({ findings, @@ -45,8 +49,8 @@ export const filterUnknownFileFindings = ({ return knownPathSet.has(normalizeWorkspacePath(filePath)) } - // The written spelling is checked first, so a known path that literally - // contains """ is never rewritten + // A real file name can contain the literal text """, and decoding it + // would name a different file, so the written spelling is checked first const resolveFile = (finding: Finding): FileResolution => { if (isKnownPath(finding.file)) return { kind: "known", finding } diff --git a/src/review/prompt.ts b/src/review/prompt.ts index 7eed0ef..bc7d912 100644 --- a/src/review/prompt.ts +++ b/src/review/prompt.ts @@ -27,10 +27,9 @@ export const conventionsCharacterCap = (conventionsBudgetTokens: number): number } /** 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. */ + * than a truncated head. A conventions file that also changed in the PR gets + * a changed-file block too — the caller uses this to decide which of the two + * copies is the full one, so exactly one full copy is ever sent. */ export const conventionsRenderInFull = ( conventions: string, conventionsBudgetTokens: number, @@ -121,6 +120,9 @@ 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.` +/** File anchoring asks the model to copy a path attribute verbatim, and + * escapeAttributeValue writes each `"` in that attribute as `"`, so the + * copied `file` can carry `"`. filterUnknownFileFindings decodes it. */ const ANCHORING_CONTRACT = `Prompt sections: the annotated diff is the section, a file block is one section, and the conventions section is the section. @@ -179,6 +181,7 @@ const truncateConventions = (conventions: string, conventionsBudgetTokens: numbe 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]` @@ -193,9 +196,10 @@ const truncateConventions = (conventions: string, conventionsBudgetTokens: numbe */ export const generateDelimiterNonce = (): string => randomBytes(6).toString("hex") -/** A double quote would terminate the surrounding attribute — nothing else is - * structural inside a quoted attribute value. Change it together with - * unescapeAttributeValue, which decodes it. */ +/** A double quote would terminate the surrounding attribute. Nothing else, + * `&` included, is structural inside a quoted attribute value, so nothing + * else is escaped. Change it together with unescapeAttributeValue, which + * decodes it. */ const escapeAttributeValue = (value: string): string => value.replaceAll('"', """) /** Decodes the one entity escapeAttributeValue writes, never general HTML @@ -242,9 +246,10 @@ 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 file block carries the full file instead. Either way the + * section's path attribute is `conventionsFile`, the same file that block + * holds. */ conventions: string | null /** Repo-relative path of the conventions file — the section's path attribute. */ conventionsFile: string From 5f045229b7ffb59e1c3d66dcb3cafeec24dcd5ba Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Wed, 30 Sep 2026 18:28:15 -0400 Subject: [PATCH 05/14] test(review): seed both sides of every unknown-file rule test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Each matching-rule test now seeds a finding the rule keeps and one it drops, so a filter that keeps or drops everything fails that test on its own. - The "decodes only "" test carries an "&" in its path, so a decoder that decodes other entities fails it. - The rendered-attribute test uses a path with "&" and '"', so an encoder that starts escaping "&" without a matching decoder fails it. - The attribute regex is scoped to the one test that reads it. Ship-Check: test-audit · claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 --- .../filter-unknown-file-findings.test.ts | 119 ++++++++++++------ 1 file changed, 82 insertions(+), 37 deletions(-) diff --git a/src/review/__tests__/filter-unknown-file-findings.test.ts b/src/review/__tests__/filter-unknown-file-findings.test.ts index e7bf201..c52b56b 100644 --- a/src/review/__tests__/filter-unknown-file-findings.test.ts +++ b/src/review/__tests__/filter-unknown-file-findings.test.ts @@ -3,16 +3,6 @@ import { filterUnknownFileFindings } from "../filter-unknown-file-findings.js" import { buildUserPrompt } from "../prompt.js" import { makeFinding } from "./make-finding.js" -/** The path attribute of the first file block, exactly as the model reads it. */ -const FILE_BLOCK_PATH_ATTRIBUTE = / { - const path = FILE_BLOCK_PATH_ATTRIBUTE.exec(userPrompt)?.groups?.path - - if (!path) throw new Error("the prompt has no file block path attribute") - return path -} - describe("filterUnknownFileFindings", () => { it("keeps a finding whose file is a known path", () => { const finding = makeFinding() @@ -31,13 +21,18 @@ describe("filterUnknownFileFindings", () => { line: 493, suggestion: "not emitted", }) + const exactFinding = makeFinding({ file: "deploy/railway/README.md", line: 12 }) expect( filterUnknownFileFindings({ - findings: [finding], + findings: [finding, exactFinding], knownPaths: ["deploy/railway/README.md"], }), - ).toEqual({ findings: [], droppedAsUnknownFile: [finding], unescapedFileRewrites: [] }) + ).toEqual({ + findings: [exactFinding], + droppedAsUnknownFile: [finding], + unescapedFileRewrites: [], + }) }) it("drops a finding whose file matches no known path", () => { @@ -70,91 +65,117 @@ describe("filterUnknownFileFindings", () => { it("matches a ./-prefixed finding file against the bare known path without rewriting it", () => { const finding = makeFinding({ file: "./src/greeter.ts" }) + const unknownFinding = makeFinding({ file: "./src/imagined.ts" }) expect( filterUnknownFileFindings({ - findings: [finding], + findings: [finding, unknownFinding], knownPaths: ["src/greeter.ts"], }), - ).toEqual({ findings: [finding], droppedAsUnknownFile: [], unescapedFileRewrites: [] }) + ).toEqual({ + findings: [finding], + droppedAsUnknownFile: [unknownFinding], + unescapedFileRewrites: [], + }) expect(finding.file).toBe("./src/greeter.ts") }) it("matches a bare finding file against a ./-prefixed known path", () => { const finding = makeFinding() + const unknownFinding = makeFinding({ file: "src/imagined.ts" }) expect( filterUnknownFileFindings({ - findings: [finding], + findings: [finding, unknownFinding], knownPaths: ["./src/greeter.ts"], }), - ).toEqual({ findings: [finding], droppedAsUnknownFile: [], unescapedFileRewrites: [] }) + ).toEqual({ + findings: [finding], + droppedAsUnknownFile: [unknownFinding], + unescapedFileRewrites: [], + }) }) it("normalizes redundant segments before comparing", () => { const finding = makeFinding({ file: "src/../src//greeter.ts" }) + const unknownFinding = makeFinding({ file: "src/../lib//greeter.ts" }) expect( filterUnknownFileFindings({ - findings: [finding], + findings: [finding, unknownFinding], knownPaths: ["src/greeter.ts"], }), - ).toEqual({ findings: [finding], droppedAsUnknownFile: [], unescapedFileRewrites: [] }) + ).toEqual({ + findings: [finding], + droppedAsUnknownFile: [unknownFinding], + unescapedFileRewrites: [], + }) }) it("ignores surrounding whitespace in the finding file", () => { const finding = makeFinding({ file: " src/greeter.ts " }) + const unknownFinding = makeFinding({ file: " src/imagined.ts " }) expect( filterUnknownFileFindings({ - findings: [finding], + findings: [finding, unknownFinding], knownPaths: ["src/greeter.ts"], }), - ).toEqual({ findings: [finding], droppedAsUnknownFile: [], unescapedFileRewrites: [] }) + ).toEqual({ + findings: [finding], + droppedAsUnknownFile: [unknownFinding], + unescapedFileRewrites: [], + }) }) it("keeps a beyond-diff finding on a related file", () => { const finding = makeFinding({ file: "src/caller.ts", line: 400 }) + const unknownFinding = makeFinding({ file: "src/imagined.ts", line: 400 }) expect( filterUnknownFileFindings({ - findings: [finding], + findings: [finding, unknownFinding], knownPaths: ["src/greeter.ts", "src/caller.ts"], }), - ).toEqual({ findings: [finding], droppedAsUnknownFile: [], unescapedFileRewrites: [] }) + ).toEqual({ + findings: [finding], + droppedAsUnknownFile: [unknownFinding], + unescapedFileRewrites: [], + }) }) it("keeps a finding whose file escapes a known path's quote as ", under the decoded path", () => { const escapedFinding = makeFinding({ file: "docs/a"b.md", line: 12 }) const unrelatedFinding = makeFinding({ line: 30 }) + const undecodableFinding = makeFinding({ file: "docs/a"c.md", line: 40 }) const resolvedFinding = { ...escapedFinding, file: 'docs/a"b.md' } expect( filterUnknownFileFindings({ - findings: [escapedFinding, unrelatedFinding], + findings: [escapedFinding, unrelatedFinding, undecodableFinding], knownPaths: ["src/greeter.ts", 'docs/a"b.md'], }), ).toEqual({ findings: [resolvedFinding, unrelatedFinding], - droppedAsUnknownFile: [], + droppedAsUnknownFile: [undecodableFinding], unescapedFileRewrites: [{ writtenFile: "docs/a"b.md", finding: resolvedFinding }], }) expect(escapedFinding.file).toBe("docs/a"b.md") }) it("decodes only " and keeps the rest of the written spelling", () => { - const escapedFinding = makeFinding({ file: "./docs/a"b.md" }) - const resolvedFinding = { ...escapedFinding, file: './docs/a"b.md' } + const escapedFinding = makeFinding({ file: "./docs/a&"b.md" }) + const resolvedFinding = { ...escapedFinding, file: './docs/a&"b.md' } expect( filterUnknownFileFindings({ findings: [escapedFinding], - knownPaths: ['docs/a"b.md'], + knownPaths: ['docs/a&"b.md'], }), ).toEqual({ findings: [resolvedFinding], droppedAsUnknownFile: [], - unescapedFileRewrites: [{ writtenFile: "./docs/a"b.md", finding: resolvedFinding }], + unescapedFileRewrites: [{ writtenFile: "./docs/a&"b.md", finding: resolvedFinding }], }) }) @@ -171,18 +192,26 @@ describe("filterUnknownFileFindings", () => { it("drops a finding whose file matches no known path before or after decoding "", () => { const finding = makeFinding({ file: "docs/c"d.md" }) + const escapedFinding = makeFinding({ file: "docs/a"b.md", line: 12 }) + const resolvedFinding = { ...escapedFinding, file: 'docs/a"b.md' } expect( filterUnknownFileFindings({ - findings: [finding], + findings: [finding, escapedFinding], knownPaths: ['docs/a"b.md', "docs/cd.md"], }), - ).toEqual({ findings: [], droppedAsUnknownFile: [finding], unescapedFileRewrites: [] }) + ).toEqual({ + findings: [resolvedFinding], + droppedAsUnknownFile: [finding], + unescapedFileRewrites: [{ writtenFile: "docs/a"b.md", finding: resolvedFinding }], + }) }) it("keeps a finding whose file is copied from a rendered path attribute", () => { const relatedDoc = { - path: 'docs/a"b.md', + // The "&" fails this test if the encoder starts escaping "&" and the + // decoder does not decode it. + path: 'docs/a&b"c.md', content: "# Quoted", includedAs: "full" as const, reason: "mentions src/greeter.ts", @@ -207,9 +236,15 @@ describe("filterUnknownFileFindings", () => { priorBotComments: [], delimiterNonce: "abc123def456", }) - const writtenFile = getFileBlockPathAttribute(userPrompt) + + /** The file block's path attribute, exactly as the model reads it. */ + const fileBlockPathAttribute = / { it("does not match on basename alone", () => { const finding = makeFinding({ file: "greeter.ts" }) + const fullPathFinding = makeFinding({ line: 12 }) expect( filterUnknownFileFindings({ - findings: [finding], + findings: [finding, fullPathFinding], knownPaths: ["src/greeter.ts"], }), - ).toEqual({ findings: [], droppedAsUnknownFile: [finding], unescapedFileRewrites: [] }) + ).toEqual({ + findings: [fullPathFinding], + droppedAsUnknownFile: [finding], + unescapedFileRewrites: [], + }) }) it("does not match on a directory prefix", () => { const finding = makeFinding({ file: "src" }) + const fullPathFinding = makeFinding({ line: 12 }) expect( filterUnknownFileFindings({ - findings: [finding], + findings: [finding, fullPathFinding], knownPaths: ["src/greeter.ts"], }), - ).toEqual({ findings: [], droppedAsUnknownFile: [finding], unescapedFileRewrites: [] }) + ).toEqual({ + findings: [fullPathFinding], + droppedAsUnknownFile: [finding], + unescapedFileRewrites: [], + }) }) it("drops every finding when no paths are known", () => { From e9dbfef2db4907466888e194bc4aaba05c0a55b8 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Wed, 30 Sep 2026 18:33:04 -0400 Subject: [PATCH 06/14] fix: correct the diff-header and full-copy claims in the escaped-path docs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Git C-quotes a path that contains a double quote, and parse-diff keeps the backslash escape, so a diff header does not print such a path unchanged. The README now says the header prints the diff's own spelling, which the filter matches as written. conventionsRenderInFull promised exactly one full copy, but a truncated conventions section with no budget left for the changed-file block sends none. It now promises no more than one. Ship-Check: bug-check · claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 --- README.md | 2 +- src/review/prompt.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 29f2d68..3d9d37b 100644 --- a/README.md +++ b/README.md @@ -142,7 +142,7 @@ The patterns are deliberately anchored (start-of-field, end-of-field, or separat A finding's `file` must name a file the model was given: a diff header (including renamed-from and deleted paths), a changed, related, or priority-doc file block, or the conventions file. A finding on any other path is ungrounded — the model saw nothing there — and is dropped before threshold, dedup, and cap. - Paths are normalized before comparison (`./src/x.ts` and `src/x.ts` match), but only whole paths match: a bare filename or a directory prefix does not. -- A path containing `"` reaches the model with each `"` written as `"` in the tags that wrap each file's content and the conventions file; diff headers print the path unchanged. A model that copies the path from a tag writes `"` into `file`, so a `file` that matches no given path as written, but matches once each `"` becomes `"`, is kept and posts under that decoded path. Each such rewrite is logged at debug level (`resolved escaped finding file to a prompt path`) with the model's spelling and the decoded path. +- A path containing `"` reaches the model with each `"` written as `"` in the tags that wrap each file's content and the conventions file. A diff header prints the path as the diff spells it, and the filter matches that spelling as written. A model that copies the path from a tag writes `"` into `file`, so a `file` that matches no given path as written, but matches once each `"` becomes `"`, is kept and posts under that decoded path. Each such rewrite is logged at debug level (`resolved escaped finding file to a prompt path`) with the model's spelling and the decoded path. - Each drop is logged as a warning (`dropping finding: file not in prompt context`) with the file, line, and category; the per-run count appears in the job summary. ## Debug logging diff --git a/src/review/prompt.ts b/src/review/prompt.ts index bc7d912..088c7bb 100644 --- a/src/review/prompt.ts +++ b/src/review/prompt.ts @@ -29,7 +29,7 @@ export const conventionsCharacterCap = (conventionsBudgetTokens: number): number /** 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 gets * a changed-file block too — the caller uses this to decide which of the two - * copies is the full one, so exactly one full copy is ever sent. */ + * copies is the full one, so no more than one full copy is ever sent. */ export const conventionsRenderInFull = ( conventions: string, conventionsBudgetTokens: number, From 6efc1d50f9e26af8c9d3edc70648a756cb88aeae Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Wed, 30 Sep 2026 18:34:55 -0400 Subject: [PATCH 07/14] docs: clarify the unknown-file filter and name the filter summary log for what it reports 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 --- README.md | 26 ++++++++++++++++---------- src/__tests__/orchestrate.test.ts | 12 ++++++------ src/orchestrate.ts | 2 +- 3 files changed, 23 insertions(+), 17 deletions(-) diff --git a/README.md b/README.md index 3d9d37b..e35e215 100644 --- a/README.md +++ b/README.md @@ -10,10 +10,10 @@ LLM-powered pull request review as a GitHub Action. One consolidated review per - Structured output end to end: every finding carries a category, severity, confidence, and a concrete failure scenario - **Drops non-findings before they post** — findings that conclude "no bug here" (an `N/A — …` title, a "no action needed" suggestion, a "…is correct" title) are filtered deterministically - Model-agnostic via OpenRouter — pick your model, see your per-call costs; every finding comment carries an `umm-actually · ` byline naming the model that produced that finding -- Findings that can't be anchored to the diff (e.g. callers outside the changed files) are posted as standalone comments on the PR +- Findings outside the diff on files the model was given (e.g. callers of changed code) are posted as standalone comments on the PR - PRs with oversized diffs are skipped gracefully with a body-only review stating the reason - Reports as its **own branded check run** in the PR checks list — the App's avatar, the outcome as the check title (findings count, clean pass, or skip reason), and a details page carrying the summary and per-run cost -- Surfaces the review context in the workflow job summary — files seen, how much of the conventions file reached the model, which `priority_docs` were included, and the token budget breakdown +- Surfaces the review context in the workflow job summary — files seen, how much of the conventions file reached the model, which `priority_docs` were included, the token budget breakdown, and how many findings each filter step dropped ## Setup @@ -85,7 +85,7 @@ An empty value, such as an unset repo variable, selects the input's default, so | `max_findings` | `""` _(uncapped)_ | Cap on posted findings, highest severity first. Empty = all validated findings post. | | `severity_threshold` | `low` | Minimum severity to post: `low` \| `medium` \| `high` \| `critical` | | `conventions_file` | `AGENTS.md` | Repo-relative path to the conventions file included in the prompt (truncated at `conventions_budget_tokens`). When the PR also changes the file, its full text is sent at most once, never twice | -| `conventions_budget_tokens` | `8000` | Approximate token budget for the conventions file section of the prompt. A file over the budget is truncated to its beginning, with a notice in the prompt. To send the whole file, also list it in `priority_docs`, which reads it in full when the context budget allows. The job summary reports every truncation. The status comment and the check run's details page also report it when the whole file never reached the model, or when it reached the model only because this PR changes the file (or, for a JavaScript/TypeScript conventions file, a file it imports) and the file is not listed in `priority_docs`, since later PRs will see just the truncated beginning. A listed file that stops fitting is reported on the PR where it does not fit. Empty = the 8000 default | +| `conventions_budget_tokens` | `8000` | Approximate token budget for the conventions file section of the prompt. A file over the budget is truncated to its beginning, with a notice in the prompt. To send the whole file, also list it in `priority_docs`, which reads it in full when the context budget allows. The job summary reports every truncation. The status comment and the check run's details page also report it when the whole file never reached the model. They also report it when the whole file reached the model only because this PR changes the file (or, for a JavaScript/TypeScript conventions file, a file it imports) and the file is not listed in `priority_docs`, since later PRs will see just the truncated beginning. A listed file that stops fitting is reported on the PR where it does not fit. Empty = the 8000 default | | `phases` | `combined` | How the review dimensions are split across model calls: `combined` (one call carrying every dimension), `parallel` (three focused calls at once — each reads deeper, wall-clock time is the slowest call, and prompt tokens roughly triple), or `sequential` (the same three calls in order, each seeing the earlier calls' findings). When two calls report findings on overlapping lines of the same file, only the higher-severity finding is kept. Empty = `combined` | | `context_budget_tokens` | `300000` | Approximate token budget for the diff plus every file sent in the prompt. A diff over half the budget skips the review. The conventions file has its own budget, `conventions_budget_tokens` | | `trace_related_files` | `true` | Add files beyond the diff to the prompt: JavaScript/TypeScript files that import a changed file by relative path (to catch caller regressions) and `.md`/`.json` files that mention a changed file (to catch stale docs). Does not affect `priority_docs` | @@ -114,12 +114,12 @@ An empty value, such as an unset repo variable, selects the input's default, so 1. Resolves the PR from the triggering event (supports `pull_request`, `pull_request_target`, and `issue_comment` events); once the PR context is known, opens a check run under the token's identity (best-effort — skipped when the token lacks `checks: write`) 2. Fetches the unified diff via the GitHub API — PRs that exceed the API's diff size limit are skipped 3. Reads the conventions file, priority docs within their reserved budget, and changed files; then traces imports and scans docs (`.md`, `.json`) with the remaining budget -4. Builds a structured prompt with randomized delimiter nonces (prompt injection defense); on re-runs, prior bot comment bodies are included so the model can self-suppress conceptual duplicates. Sends one request per review phase to OpenRouter (see the `phases` input for dispatch modes) +4. Builds a structured prompt that wraps PR content in tags with a random per-prompt suffix, so PR text can't forge a closing tag and inject instructions; on re-runs, prior bot comment bodies are included so the model can self-suppress conceptual duplicates. Sends one request per review phase to OpenRouter (see the `phases` input for dispatch modes) 5. Validates each response against a strict Zod schema. Each model gets up to two attempts, then `fallback_model`, if set, is tried (see that input for timeouts and auth errors). A phase whose attempts all fail, or that reaches the shared review deadline, is named on the status comment and the check run while the other phases' findings still post; the run fails only when no phase completes 6. Drops non-findings (see [Non-finding filter](#non-finding-filter)) and findings on files the model was never given (see [Unknown-file filter](#unknown-file-filter)), collapses findings that two phases reported on overlapping lines of the same file, then on re-runs deduplicates against previously posted bot comments (three-tier: positional match by hidden HTML anchor, content match by title similarity within 50 lines in the same file, or title-only match by high title similarity across any file) -7. Filters remaining findings by severity threshold, deduplicates overlapping findings within the run, and caps if configured +7. Filters remaining findings by severity threshold, drops a finding that overlaps a more severe finding of the same category in the same file, and caps if configured 8. Maps findings to inline PR review comments anchored to diff lines, with a snap-to-nearest-hunk fallback -9. Posts one review with inline comments (invisible body); beyond-diff findings post as standalone PR comments; every run upserts a status comment with cross-run totals +9. Posts one review with inline comments (its body is only a hidden marker); beyond-diff findings post as standalone PR comments; every run upserts a status comment with cross-run totals 10. Completes the check run with the outcome — the conclusion grades the run, not the code: - `success` for any completed review (with or without findings — the count is in the check title, and a review that lost a phase says so there too) - `neutral` for a skip @@ -128,20 +128,26 @@ An empty value, such as an unset repo variable, selects the input's default, so ## Non-finding filter -Models sometimes report "findings" that conclude the code is fine — titled `N/A — …` or `…is correct`, with suggestions like "No action needed". The system prompt prohibits these, but models don't always comply, so every finding also passes a deterministic filter before severity threshold, cross-run dedup, and cap. A finding is dropped when: +Models sometimes report "findings" that conclude the code is fine — titled `N/A — …` or `…is correct`, with suggestions like "No action needed". The system prompt prohibits these, but models don't always comply, so every finding also passes a deterministic filter before cross-run dedup, the severity threshold, and the cap. A finding is dropped when: - its **title**, **failure_scenario**, or **suggestion** starts with a non-finding signal — `N/A`, `not applicable`, `placeholder`, or a separator-delimited confirmation phrase (`none — …`, `not a finding — …`, `no failure — …`, `no concrete failure scenario — …`, `no bug — …`, `no (further) action needed — …`, `no change needed — …`) - its **title** ends with a declarative confirmation — `…is correct` or `…is accurate` — or starts with a prior-finding resolution confirmation — `Prior (bot) finding(s) addressed/resolved/fixed …` - its **title** contains a self-negating phrase followed by a separator or end-of-title — `…not actionable`, `…not a defect`, `…not a(n) (real) issue/bug/problem` - its **failure_scenario** ends with a leaked conclusion — `…no bug` / `…no bug here`, `…not actionable` / `…not a defect` / `…not a(n) (real) issue/bug/problem`, or `…analysis was wrong` -The patterns are deliberately anchored (start-of-field, end-of-field, or separator-delimited) so real findings survive: a scenario like "No failure occurs until the third retry…" or "None of the guards catch this input" never matches. Dropped counts are logged per run (`non-finding filter applied to model output`). +The patterns are deliberately anchored (start-of-field, end-of-field, or separator-delimited) so real findings survive: a scenario like "No failure occurs until the third retry…" or "None of the guards catch this input" never matches. Dropped counts are logged per run (`per-phase finding filters applied to model output`) and appear in the job summary. ## Unknown-file filter -A finding's `file` must name a file the model was given: a diff header (including renamed-from and deleted paths), a changed, related, or priority-doc file block, or the conventions file. A finding on any other path is ungrounded — the model saw nothing there — and is dropped before threshold, dedup, and cap. +A finding's `file` (the path it is filed on) must name a file the model was given. A finding on any other path is ungrounded, because the model saw nothing there, and is dropped before deduplication, the severity threshold, and the cap. The model was given: -- Paths are normalized before comparison (`./src/x.ts` and `src/x.ts` match), but only whole paths match: a bare filename or a directory prefix does not. +- paths in diff headers, including renamed-from and deleted paths (findings on those post as standalone comments) +- changed files, import-traced related files, mention-matched docs, and priority docs +- the conventions file + +Files removed by `diff_exclude_paths` or linguist rules were never sent, so findings on them are dropped too. The filter compares paths and reports drops as follows: + +- Paths are normalized before comparison: surrounding whitespace, `.` and `..` segments, repeated slashes, and a leading or trailing `/` don't affect the match (`./src/x.ts`, `/src/x.ts`, and `src/x.ts` match). Matching is case-sensitive, and only whole paths match: a bare filename or a directory prefix does not. - A path containing `"` reaches the model with each `"` written as `"` in the tags that wrap each file's content and the conventions file. A diff header prints the path as the diff spells it, and the filter matches that spelling as written. A model that copies the path from a tag writes `"` into `file`, so a `file` that matches no given path as written, but matches once each `"` becomes `"`, is kept and posts under that decoded path. Each such rewrite is logged at debug level (`resolved escaped finding file to a prompt path`) with the model's spelling and the decoded path. - Each drop is logged as a warning (`dropping finding: file not in prompt context`) with the file, line, and category; the per-run count appears in the job summary. diff --git a/src/__tests__/orchestrate.test.ts b/src/__tests__/orchestrate.test.ts index 72e3f83..f7ffe0c 100644 --- a/src/__tests__/orchestrate.test.ts +++ b/src/__tests__/orchestrate.test.ts @@ -2707,10 +2707,10 @@ describe("orchestrate", () => { }, }, ]) - expect(logsWithMessage(logger, "non-finding filter applied to model output")).toEqual([ + expect(logsWithMessage(logger, "per-phase finding filters applied to model output")).toEqual([ { level: "info", - message: "non-finding filter applied to model output", + message: "per-phase finding filters applied to model output", data: { totalFromModel: 2, kept: 1, @@ -2955,10 +2955,10 @@ describe("orchestrate", () => { }, ]) - expect(logsWithMessage(logger, "non-finding filter applied to model output")).toEqual([ + expect(logsWithMessage(logger, "per-phase finding filters applied to model output")).toEqual([ { level: "info", - message: "non-finding filter applied to model output", + message: "per-phase finding filters applied to model output", data: { totalFromModel: 2, kept: 1, @@ -3079,10 +3079,10 @@ describe("orchestrate", () => { }, }, ]) - expect(logsWithMessage(logger, "non-finding filter applied to model output")).toEqual([ + expect(logsWithMessage(logger, "per-phase finding filters applied to model output")).toEqual([ { level: "info", - message: "non-finding filter applied to model output", + message: "per-phase finding filters applied to model output", data: { totalFromModel: 2, kept: 1, diff --git a/src/orchestrate.ts b/src/orchestrate.ts index 86b245f..9628437 100644 --- a/src/orchestrate.ts +++ b/src/orchestrate.ts @@ -1014,7 +1014,7 @@ const runReviewPipeline = async ( const { findings: realFindings, duplicatesAcrossPhases } = mergePhaseFindings( filteredPhases.map((filtered) => filtered.findings), ) - logger.info("non-finding filter applied to model output", { + logger.info("per-phase finding filters applied to model output", { totalFromModel, kept: realFindings.length, droppedAsNonFinding, From faafef8856eb4f2af42547c373a472e44d526efd Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Wed, 30 Sep 2026 18:39:24 -0400 Subject: [PATCH 08/14] docs: state the overlap tie rule and the conventions-file exclusion exception MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The within-run overlap step also drops one of two same-severity findings, keeping the earlier line. The conventions file is read even when diff_exclude_paths or linguist rules exclude it, so findings on it are kept. Ship-Check: code-quality · claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 --- README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index e35e215..20c25cd 100644 --- a/README.md +++ b/README.md @@ -117,7 +117,7 @@ An empty value, such as an unset repo variable, selects the input's default, so 4. Builds a structured prompt that wraps PR content in tags with a random per-prompt suffix, so PR text can't forge a closing tag and inject instructions; on re-runs, prior bot comment bodies are included so the model can self-suppress conceptual duplicates. Sends one request per review phase to OpenRouter (see the `phases` input for dispatch modes) 5. Validates each response against a strict Zod schema. Each model gets up to two attempts, then `fallback_model`, if set, is tried (see that input for timeouts and auth errors). A phase whose attempts all fail, or that reaches the shared review deadline, is named on the status comment and the check run while the other phases' findings still post; the run fails only when no phase completes 6. Drops non-findings (see [Non-finding filter](#non-finding-filter)) and findings on files the model was never given (see [Unknown-file filter](#unknown-file-filter)), collapses findings that two phases reported on overlapping lines of the same file, then on re-runs deduplicates against previously posted bot comments (three-tier: positional match by hidden HTML anchor, content match by title similarity within 50 lines in the same file, or title-only match by high title similarity across any file) -7. Filters remaining findings by severity threshold, drops a finding that overlaps a more severe finding of the same category in the same file, and caps if configured +7. Filters remaining findings by severity threshold, drops the less severe of two same-category findings that overlap in the same file (on a tie, the one on the later line), and caps if configured 8. Maps findings to inline PR review comments anchored to diff lines, with a snap-to-nearest-hunk fallback 9. Posts one review with inline comments (its body is only a hidden marker); beyond-diff findings post as standalone PR comments; every run upserts a status comment with cross-run totals 10. Completes the check run with the outcome — the conclusion grades the run, not the code: @@ -145,7 +145,7 @@ A finding's `file` (the path it is filed on) must name a file the model was give - changed files, import-traced related files, mention-matched docs, and priority docs - the conventions file -Files removed by `diff_exclude_paths` or linguist rules were never sent, so findings on them are dropped too. The filter compares paths and reports drops as follows: +Other than the conventions file, files removed by `diff_exclude_paths` or linguist rules were never sent, so findings on them are dropped too. The filter compares paths and reports drops as follows: - Paths are normalized before comparison: surrounding whitespace, `.` and `..` segments, repeated slashes, and a leading or trailing `/` don't affect the match (`./src/x.ts`, `/src/x.ts`, and `src/x.ts` match). Matching is case-sensitive, and only whole paths match: a bare filename or a directory prefix does not. - A path containing `"` reaches the model with each `"` written as `"` in the tags that wrap each file's content and the conventions file. A diff header prints the path as the diff spells it, and the filter matches that spelling as written. A model that copies the path from a tag writes `"` into `file`, so a `file` that matches no given path as written, but matches once each `"` becomes `"`, is kept and posts under that decoded path. Each such rewrite is logged at debug level (`resolved escaped finding file to a prompt path`) with the model's spelling and the decoded path. From db9ec9fdd548ccb1fe3c02e8b1122276f0178b95 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Wed, 30 Sep 2026 18:40:18 -0400 Subject: [PATCH 09/14] docs: scope the prompt-tag and dot-segment claims to what the code does 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 --- README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 20c25cd..0493d41 100644 --- a/README.md +++ b/README.md @@ -114,7 +114,7 @@ An empty value, such as an unset repo variable, selects the input's default, so 1. Resolves the PR from the triggering event (supports `pull_request`, `pull_request_target`, and `issue_comment` events); once the PR context is known, opens a check run under the token's identity (best-effort — skipped when the token lacks `checks: write`) 2. Fetches the unified diff via the GitHub API — PRs that exceed the API's diff size limit are skipped 3. Reads the conventions file, priority docs within their reserved budget, and changed files; then traces imports and scans docs (`.md`, `.json`) with the remaining budget -4. Builds a structured prompt that wraps PR content in tags with a random per-prompt suffix, so PR text can't forge a closing tag and inject instructions; on re-runs, prior bot comment bodies are included so the model can self-suppress conceptual duplicates. Sends one request per review phase to OpenRouter (see the `phases` input for dispatch modes) +4. Builds a structured prompt that wraps PR content in tags with a random per-prompt suffix, so PR text can't forge a closing tag to place text outside its tag; on re-runs, prior bot comment bodies are included so the model can self-suppress conceptual duplicates. Sends one request per review phase to OpenRouter (see the `phases` input for dispatch modes) 5. Validates each response against a strict Zod schema. Each model gets up to two attempts, then `fallback_model`, if set, is tried (see that input for timeouts and auth errors). A phase whose attempts all fail, or that reaches the shared review deadline, is named on the status comment and the check run while the other phases' findings still post; the run fails only when no phase completes 6. Drops non-findings (see [Non-finding filter](#non-finding-filter)) and findings on files the model was never given (see [Unknown-file filter](#unknown-file-filter)), collapses findings that two phases reported on overlapping lines of the same file, then on re-runs deduplicates against previously posted bot comments (three-tier: positional match by hidden HTML anchor, content match by title similarity within 50 lines in the same file, or title-only match by high title similarity across any file) 7. Filters remaining findings by severity threshold, drops the less severe of two same-category findings that overlap in the same file (on a tie, the one on the later line), and caps if configured @@ -147,7 +147,7 @@ A finding's `file` (the path it is filed on) must name a file the model was give Other than the conventions file, files removed by `diff_exclude_paths` or linguist rules were never sent, so findings on them are dropped too. The filter compares paths and reports drops as follows: -- Paths are normalized before comparison: surrounding whitespace, `.` and `..` segments, repeated slashes, and a leading or trailing `/` don't affect the match (`./src/x.ts`, `/src/x.ts`, and `src/x.ts` match). Matching is case-sensitive, and only whole paths match: a bare filename or a directory prefix does not. +- Paths are normalized before comparison: surrounding whitespace, `.` segments, `..` segments that stay inside the repository, repeated slashes, and a leading or trailing `/` don't affect the match (`./src/x.ts`, `/src/x.ts`, and `src/x.ts` match). Matching is case-sensitive, and only whole paths match: a bare filename or a directory prefix does not. - A path containing `"` reaches the model with each `"` written as `"` in the tags that wrap each file's content and the conventions file. A diff header prints the path as the diff spells it, and the filter matches that spelling as written. A model that copies the path from a tag writes `"` into `file`, so a `file` that matches no given path as written, but matches once each `"` becomes `"`, is kept and posts under that decoded path. Each such rewrite is logged at debug level (`resolved escaped finding file to a prompt path`) with the model's spelling and the decoded path. - Each drop is logged as a warning (`dropping finding: file not in prompt context`) with the file, line, and category; the per-run count appears in the job summary. From d986264f9140a63c5973114aea6b16de5a0e5c99 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Wed, 30 Sep 2026 18:43:29 -0400 Subject: [PATCH 10/14] test: seed dropped near-misses in the unknown-file filter tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Each matching-rule filter test now also seeds a finding the rule drops, so a filter that keeps everything fails it. New cases cover a leading or trailing slash on either side of the match, and an orchestrate test shows a finding filed on an escaped path is deduplicated against a prior anchor on the decoded path. The README's unknown-file filter section now says excluded files are listed by path at the end of the diff with their content withheld, and that the conventions file is sent even when excluded. Ship-Check: pr-monitor · claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 --- README.md | 2 +- src/__tests__/orchestrate.test.ts | 48 +++++++++++++ .../filter-unknown-file-findings.test.ts | 69 ++++++++++++++++--- 3 files changed, 110 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index 0493d41..b8a5f22 100644 --- a/README.md +++ b/README.md @@ -145,7 +145,7 @@ A finding's `file` (the path it is filed on) must name a file the model was give - changed files, import-traced related files, mention-matched docs, and priority docs - the conventions file -Other than the conventions file, files removed by `diff_exclude_paths` or linguist rules were never sent, so findings on them are dropped too. The filter compares paths and reports drops as follows: +Files removed by `diff_exclude_paths` or linguist rules are listed by path at the end of the diff, but their content is withheld, so findings on them are dropped too. The conventions file is the exception: it is sent even when excluded. The filter compares paths and reports drops as follows: - Paths are normalized before comparison: surrounding whitespace, `.` segments, `..` segments that stay inside the repository, repeated slashes, and a leading or trailing `/` don't affect the match (`./src/x.ts`, `/src/x.ts`, and `src/x.ts` match). Matching is case-sensitive, and only whole paths match: a bare filename or a directory prefix does not. - A path containing `"` reaches the model with each `"` written as `"` in the tags that wrap each file's content and the conventions file. A diff header prints the path as the diff spells it, and the filter matches that spelling as written. A model that copies the path from a tag writes `"` into `file`, so a `file` that matches no given path as written, but matches once each `"` becomes `"`, is kept and posts under that decoded path. Each such rewrite is logged at debug level (`resolved escaped finding file to a prompt path`) with the model's spelling and the decoded path. diff --git a/src/__tests__/orchestrate.test.ts b/src/__tests__/orchestrate.test.ts index f7ffe0c..50dcb18 100644 --- a/src/__tests__/orchestrate.test.ts +++ b/src/__tests__/orchestrate.test.ts @@ -3378,6 +3378,54 @@ describe("orchestrate", () => { expect(stubs.postFindingsReviewCalls).toEqual([expectedFindingsReview(findings.slice(1))]) }) + it("dedups a finding filed on an escaped path against a prior anchor on the decoded path", async () => { + const escapedFinding = makeFinding({ file: "docs/a"b.md", line: 3 }) + const stubs = makeOrchestrateDeps({ + fixtureResult: { + review: { analysis: "checked", findings: [escapedFinding] }, + }, + contextReader: { + findRelatedDocs: async () => ({ + files: [ + { + path: 'docs/a"b.md', + content: "# Greeter\n\nCall greet() with a name.", + includedAs: "full", + reason: "mentions src/greeter.ts", + }, + ], + excludedByCapPaths: [], + }), + }, + githubClient: { + fetchBotIssueComments: async () => [issueFinding('docs/a"b.md:correctness:3')], + }, + }) + const logger = createTestLogger() + + const result = await orchestrate(stubs.deps, logger) + + expect(result.findingsCount).toBe(0) + expect(stubs.postIssueCommentCalls).toEqual([]) + expect(stubs.postFindingsReviewCalls).toEqual([]) + expect(logsWithMessage(logger, "cross-run dedup against prior bot comments")).toEqual([ + { + level: "info", + message: "cross-run dedup against prior bot comments", + data: { + statusCommentFound: false, + existingAnchorCount: 1, + priorBotCommentCount: 1, + findingsAfterFilter: 1, + findingsSurvivedDedup: 0, + droppedByPositional: 1, + droppedByContent: 0, + droppedByTitle: 0, + }, + }, + ]) + }) + it("dedups a finding whose reported line drifted within the window", async () => { const findings = fixtureReviewResponse.findings const driftedFinding = findings[0] diff --git a/src/review/__tests__/filter-unknown-file-findings.test.ts b/src/review/__tests__/filter-unknown-file-findings.test.ts index c52b56b..7a3216c 100644 --- a/src/review/__tests__/filter-unknown-file-findings.test.ts +++ b/src/review/__tests__/filter-unknown-file-findings.test.ts @@ -6,13 +6,18 @@ import { makeFinding } from "./make-finding.js" describe("filterUnknownFileFindings", () => { it("keeps a finding whose file is a known path", () => { const finding = makeFinding() + const unknownFinding = makeFinding({ file: "src/greeter.tsx" }) expect( filterUnknownFileFindings({ - findings: [finding], + findings: [finding, unknownFinding], knownPaths: ["src/greeter.ts"], }), - ).toEqual({ findings: [finding], droppedAsUnknownFile: [], unescapedFileRewrites: [] }) + ).toEqual({ + findings: [finding], + droppedAsUnknownFile: [unknownFinding], + unescapedFileRewrites: [], + }) }) it("drops a finding whose file is a known path with prose appended", () => { @@ -96,6 +101,47 @@ describe("filterUnknownFileFindings", () => { }) }) + it.each([ + { + label: "a leading / on the finding file", + findingFile: "/src/greeter.ts", + knownPath: "src/greeter.ts", + unknownFile: "/lib/greeter.ts", + }, + { + label: "a trailing / on the finding file", + findingFile: "src/greeter.ts/", + knownPath: "src/greeter.ts", + unknownFile: "lib/greeter.ts/", + }, + { + label: "a leading / on the known path", + findingFile: "src/greeter.ts", + knownPath: "/src/greeter.ts", + unknownFile: "lib/greeter.ts", + }, + { + label: "a trailing / on the known path", + findingFile: "src/greeter.ts", + knownPath: "src/greeter.ts/", + unknownFile: "lib/greeter.ts", + }, + ])("ignores $label when matching", ({ findingFile, knownPath, unknownFile }) => { + const finding = makeFinding({ file: findingFile }) + const unknownFinding = makeFinding({ file: unknownFile }) + + expect( + filterUnknownFileFindings({ + findings: [finding, unknownFinding], + knownPaths: [knownPath], + }), + ).toEqual({ + findings: [finding], + droppedAsUnknownFile: [unknownFinding], + unescapedFileRewrites: [], + }) + }) + it("normalizes redundant segments before comparing", () => { const finding = makeFinding({ file: "src/../src//greeter.ts" }) const unknownFinding = makeFinding({ file: "src/../lib//greeter.ts" }) @@ -165,29 +211,35 @@ describe("filterUnknownFileFindings", () => { it("decodes only " and keeps the rest of the written spelling", () => { const escapedFinding = makeFinding({ file: "./docs/a&"b.md" }) + const unknownFinding = makeFinding({ file: "./docs/a&"c.md", line: 40 }) const resolvedFinding = { ...escapedFinding, file: './docs/a&"b.md' } expect( filterUnknownFileFindings({ - findings: [escapedFinding], + findings: [escapedFinding, unknownFinding], knownPaths: ['docs/a&"b.md'], }), ).toEqual({ findings: [resolvedFinding], - droppedAsUnknownFile: [], + droppedAsUnknownFile: [unknownFinding], unescapedFileRewrites: [{ writtenFile: "./docs/a&"b.md", finding: resolvedFinding }], }) }) it("keeps a known path containing a literal " as written when its decoded spelling is also known", () => { const finding = makeFinding({ file: "docs/a"b.md" }) + const unknownFinding = makeFinding({ file: "docs/a"c.md", line: 40 }) expect( filterUnknownFileFindings({ - findings: [finding], + findings: [finding, unknownFinding], knownPaths: ['docs/a"b.md', "docs/a"b.md"], }), - ).toEqual({ findings: [finding], droppedAsUnknownFile: [], unescapedFileRewrites: [] }) + ).toEqual({ + findings: [finding], + droppedAsUnknownFile: [unknownFinding], + unescapedFileRewrites: [], + }) }) it("drops a finding whose file matches no known path before or after decoding "", () => { @@ -244,16 +296,17 @@ describe("filterUnknownFileFindings", () => { if (!writtenFile) throw new Error("the prompt has no file block path attribute") const finding = makeFinding({ file: writtenFile, line: 1 }) + const unknownFinding = makeFinding({ file: "docs/a&b"d.md", line: 2 }) const resolvedFinding = { ...finding, file: 'docs/a&b"c.md' } expect( filterUnknownFileFindings({ - findings: [finding], + findings: [finding, unknownFinding], knownPaths: [relatedDoc.path], }), ).toEqual({ findings: [resolvedFinding], - droppedAsUnknownFile: [], + droppedAsUnknownFile: [unknownFinding], unescapedFileRewrites: [{ writtenFile, finding: resolvedFinding }], }) }) From 62f339313823b245968184400de8793c00164a2b Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Wed, 30 Sep 2026 18:49:58 -0400 Subject: [PATCH 11/14] fix: drop findings on a diff-excluded conventions file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The conventions path joins the unknown-file filter's accepted paths only when diff exclusion did not remove the file. The conventions section is still sent as the rubric. README states the new behaviour and uses one order phrase for both filters. Ship-Check: triage · claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 --- README.md | 8 +-- src/__tests__/orchestrate.test.ts | 87 +++++++++++++++++++++++++++++++ src/orchestrate.ts | 14 ++++- 3 files changed, 103 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index b8a5f22..2710a87 100644 --- a/README.md +++ b/README.md @@ -128,7 +128,7 @@ An empty value, such as an unset repo variable, selects the input's default, so ## Non-finding filter -Models sometimes report "findings" that conclude the code is fine — titled `N/A — …` or `…is correct`, with suggestions like "No action needed". The system prompt prohibits these, but models don't always comply, so every finding also passes a deterministic filter before cross-run dedup, the severity threshold, and the cap. A finding is dropped when: +Models sometimes report "findings" that conclude the code is fine — titled `N/A — …` or `…is correct`, with suggestions like "No action needed". The system prompt prohibits these, but models don't always comply, so every finding also passes a deterministic filter before cross-phase and cross-run deduplication, the severity threshold, and the cap. A finding is dropped when: - its **title**, **failure_scenario**, or **suggestion** starts with a non-finding signal — `N/A`, `not applicable`, `placeholder`, or a separator-delimited confirmation phrase (`none — …`, `not a finding — …`, `no failure — …`, `no concrete failure scenario — …`, `no bug — …`, `no (further) action needed — …`, `no change needed — …`) - its **title** ends with a declarative confirmation — `…is correct` or `…is accurate` — or starts with a prior-finding resolution confirmation — `Prior (bot) finding(s) addressed/resolved/fixed …` @@ -139,13 +139,13 @@ The patterns are deliberately anchored (start-of-field, end-of-field, or separat ## Unknown-file filter -A finding's `file` (the path it is filed on) must name a file the model was given. A finding on any other path is ungrounded, because the model saw nothing there, and is dropped before deduplication, the severity threshold, and the cap. The model was given: +A finding's `file` (the path it is filed on) must name a file the model was given. A finding on any other path is ungrounded, because the model saw nothing there, and is dropped before cross-phase and cross-run deduplication, the severity threshold, and the cap. The model was given: -- paths in diff headers, including renamed-from and deleted paths (findings on those post as standalone comments) +- paths in diff headers, including renamed-from and deleted paths (findings on renamed-from and deleted paths post as standalone comments) - changed files, import-traced related files, mention-matched docs, and priority docs - the conventions file -Files removed by `diff_exclude_paths` or linguist rules are listed by path at the end of the diff, but their content is withheld, so findings on them are dropped too. The conventions file is the exception: it is sent even when excluded. The filter compares paths and reports drops as follows: +Files removed by `diff_exclude_paths` or linguist rules are listed by path at the end of the diff, but their content is withheld, so findings on them are dropped too. When the conventions file is excluded, it is still sent as the review's conventions, but findings on it are dropped like those on any other excluded file. The filter compares paths and reports drops as follows: - Paths are normalized before comparison: surrounding whitespace, `.` segments, `..` segments that stay inside the repository, repeated slashes, and a leading or trailing `/` don't affect the match (`./src/x.ts`, `/src/x.ts`, and `src/x.ts` match). Matching is case-sensitive, and only whole paths match: a bare filename or a directory prefix does not. - A path containing `"` reaches the model with each `"` written as `"` in the tags that wrap each file's content and the conventions file. A diff header prints the path as the diff spells it, and the filter matches that spelling as written. A model that copies the path from a tag writes `"` into `file`, so a `file` that matches no given path as written, but matches once each `"` becomes `"`, is kept and posts under that decoded path. Each such rewrite is logged at debug level (`resolved escaped finding file to a prompt path`) with the model's spelling and the decoded path. diff --git a/src/__tests__/orchestrate.test.ts b/src/__tests__/orchestrate.test.ts index 50dcb18..28bf640 100644 --- a/src/__tests__/orchestrate.test.ts +++ b/src/__tests__/orchestrate.test.ts @@ -838,6 +838,93 @@ describe("orchestrate", () => { ]) }) + describe("findings on a changed conventions file", () => { + const conventionsChangeDiff = `${sampleDiff}diff --git a/AGENTS.md b/AGENTS.md\nindex 1111111..4444444 100644\n--- a/AGENTS.md\n+++ b/AGENTS.md\n@@ -1 +1 @@\n-# Old conventions\n+# Test conventions\n` + const conventionsFinding = makeFinding({ file: "AGENTS.md", line: 1 }) + + /** Wires the real prompt builder over a stub client that records each + * user prompt and returns one finding on the conventions file. */ + const makeConventionsFindingDeps = (diffExcludePathPatterns: string[]) => { + const userPrompts: string[] = [] + const stubClient: OpenRouterClient = { + requestReview: async (params) => { + userPrompts.push(params.userPrompt) + return { + review: { analysis: "checked", findings: [conventionsFinding] }, + modelUsed: "test/model", + attempts: [fixtureAttempt], + } + }, + } + const stubs = makeOrchestrateDeps({ + config: { + conventionsFile: "AGENTS.md", + diffExcludePaths: { defaultPatterns: [], diffExcludePathPatterns }, + }, + githubClient: { + fetchDiff: async () => ({ kind: "ok" as const, diff: conventionsChangeDiff }), + }, + generateFindings: createPromptedGenerateFindings( + { openrouterClient: stubClient, model: "test/model", fallbackModel: null }, + createTestLogger(), + ), + }) + + return { stubs, userPrompts } + } + + it("drops a finding on a diff-excluded conventions file and still sends its section", async () => { + const { stubs, userPrompts } = makeConventionsFindingDeps(["AGENTS.md"]) + const logger = createTestLogger() + + const result = await orchestrate(stubs.deps, logger) + + expect(result.findingsCount).toBe(0) + expect(stubs.postFindingsReviewCalls).toEqual([]) + expect(stubs.postIssueCommentCalls).toEqual([]) + expect(logsWithMessage(logger, "dropping finding: file not in prompt context")).toEqual([ + { + level: "warn", + message: "dropping finding: file not in prompt context", + data: { + phase: "combined", + file: "AGENTS.md", + line: 1, + category: conventionsFinding.category, + }, + }, + ]) + // The nonce is random per call; the backreference pins both tags to the same one + expect(userPrompts).toEqual([ + expect.stringMatching( + /\n# Test conventions\n<\/conventions-\1 path="AGENTS\.md">/, + ), + ]) + }) + + it("keeps a finding on a conventions file that stays in the review diff", async () => { + const { stubs } = makeConventionsFindingDeps([]) + const logger = createTestLogger() + + const result = await orchestrate(stubs.deps, logger) + + const expectedMapped = mapFindingsToReview({ + findings: [withRoutedModel(conventionsFinding, "test/model")], + commentableByPath: computeCommentableLines(parseDiff(conventionsChangeDiff)), + }) + expect(result.findingsCount).toBe(1) + expect(stubs.postFindingsReviewCalls).toEqual([ + { + prNumber: 7, + commitId: fixturePrContext.headSha, + body: REVIEW_MARKER, + comments: expectedMapped.comments, + }, + ]) + expect(logsWithMessage(logger, "dropping finding: file not in prompt context")).toEqual([]) + }) + }) + it("excludes files the repo marks linguist-generated", async () => { const stubs = makeOrchestrateDeps({ contextReader: { diff --git a/src/orchestrate.ts b/src/orchestrate.ts index 9628437..f9b9535 100644 --- a/src/orchestrate.ts +++ b/src/orchestrate.ts @@ -891,20 +891,30 @@ const runReviewPipeline = async ( // - Diff headers. changedPaths holds each new path and a rename's old path, // which the header prints as "renamed from". A deleted file's header // names a path changedPaths lacks, so it is added here. - // - File blocks, and the conventions section when the file was found. + // - File blocks, and the conventions section when the file was found and + // diff exclusion did not remove it. // These paths go in as written: filterUnknownFileFindings normalizes both // sides with normalizeWorkspacePath, which also strips a leading or // trailing "/" that the posix.normalize sets above keep. const deletedPaths = reviewableFiles.flatMap((file) => { return file.deleted && file.from ? [file.from] : [] }) + + // A PR can change the conventions file while diff exclusion removes it from + // the review. The excluded-files note then tells the model the file's + // content was not shown, though the conventions section still sends it as + // the rubric. Leaving its path out here drops findings on it like findings + // on any other excluded file. Without this check, those findings would post. + const acceptsConventionsFindings = + conventions !== null && !diffExcludedPathSet.has(posix.normalize(config.conventionsFile)) + const promptFilePaths = [ ...changedPaths, ...deletedPaths, ...changedFiles.map((file) => file.path), ...relatedFiles.map((file) => file.path), ...relatedDocs.map((file) => file.path), - ...(conventions !== null ? [config.conventionsFile] : []), + ...(acceptsConventionsFindings ? [config.conventionsFile] : []), ] logger.info("context sent to model", { From e9e853e40f5f3fa33e8474ea2cc12aea98ca8207 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Wed, 30 Sep 2026 18:55:06 -0400 Subject: [PATCH 12/14] test(orchestrate): cover a ./-prefixed excluded conventions file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ship-Check: pr-monitor · claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 --- src/__tests__/orchestrate.test.ts | 42 ++++++++++++++++++++++++++++--- 1 file changed, 38 insertions(+), 4 deletions(-) diff --git a/src/__tests__/orchestrate.test.ts b/src/__tests__/orchestrate.test.ts index 28bf640..d6b1c9d 100644 --- a/src/__tests__/orchestrate.test.ts +++ b/src/__tests__/orchestrate.test.ts @@ -844,7 +844,13 @@ describe("orchestrate", () => { /** Wires the real prompt builder over a stub client that records each * user prompt and returns one finding on the conventions file. */ - const makeConventionsFindingDeps = (diffExcludePathPatterns: string[]) => { + const makeConventionsFindingDeps = ({ + diffExcludePathPatterns, + conventionsFile = "AGENTS.md", + }: { + diffExcludePathPatterns: string[] + conventionsFile?: string + }) => { const userPrompts: string[] = [] const stubClient: OpenRouterClient = { requestReview: async (params) => { @@ -858,7 +864,7 @@ describe("orchestrate", () => { } const stubs = makeOrchestrateDeps({ config: { - conventionsFile: "AGENTS.md", + conventionsFile, diffExcludePaths: { defaultPatterns: [], diffExcludePathPatterns }, }, githubClient: { @@ -874,7 +880,9 @@ describe("orchestrate", () => { } it("drops a finding on a diff-excluded conventions file and still sends its section", async () => { - const { stubs, userPrompts } = makeConventionsFindingDeps(["AGENTS.md"]) + const { stubs, userPrompts } = makeConventionsFindingDeps({ + diffExcludePathPatterns: ["AGENTS.md"], + }) const logger = createTestLogger() const result = await orchestrate(stubs.deps, logger) @@ -902,8 +910,34 @@ describe("orchestrate", () => { ]) }) + it("drops a finding on a diff-excluded conventions file configured with a ./ prefix", async () => { + const { stubs } = makeConventionsFindingDeps({ + diffExcludePathPatterns: ["AGENTS.md"], + conventionsFile: "./AGENTS.md", + }) + const logger = createTestLogger() + + const result = await orchestrate(stubs.deps, logger) + + expect(result.findingsCount).toBe(0) + expect(stubs.postFindingsReviewCalls).toEqual([]) + expect(stubs.postIssueCommentCalls).toEqual([]) + expect(logsWithMessage(logger, "dropping finding: file not in prompt context")).toEqual([ + { + level: "warn", + message: "dropping finding: file not in prompt context", + data: { + phase: "combined", + file: "AGENTS.md", + line: 1, + category: conventionsFinding.category, + }, + }, + ]) + }) + it("keeps a finding on a conventions file that stays in the review diff", async () => { - const { stubs } = makeConventionsFindingDeps([]) + const { stubs } = makeConventionsFindingDeps({ diffExcludePathPatterns: [] }) const logger = createTestLogger() const result = await orchestrate(stubs.deps, logger) From 2753a2c10bdecff41e2915c026ce1ebdb8014bb6 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Wed, 30 Sep 2026 19:03:22 -0400 Subject: [PATCH 13/14] fix: report findings on diff-excluded files apart from unknown-file drops MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A finding on a diff-excluded file, the conventions file included, was dropped with the unknown-file warning and counted as an unknown file. That pointed an operator at context assembly, though the path is listed in the excluded-files note. The filter now returns those findings in their own bucket, logged as `dropping finding: file excluded from the review diff` and counted in a `Dropped as excluded file` job-summary row and the per-phase filter log. Which findings are dropped is unchanged. Ship-Check: pr-monitor · claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 --- README.md | 18 +-- action.yml | 2 +- src/__tests__/orchestrate.test.ts | 44 +++++- src/orchestrate.ts | 45 ++++++- .../filter-unknown-file-findings.test.ts | 126 +++++++++++++++++- src/review/__tests__/review-summary.test.ts | 34 +++++ src/review/filter-unknown-file-findings.ts | 43 ++++-- src/review/review-summary.ts | 3 + 8 files changed, 278 insertions(+), 37 deletions(-) diff --git a/README.md b/README.md index 2710a87..1804344 100644 --- a/README.md +++ b/README.md @@ -102,12 +102,12 @@ An empty value, such as an unset repo variable, selects the input's default, so ## Outputs -| Output | Description | -| ---------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `findings_count` | Number of new findings posted this run. Non-findings, findings on files the model never saw, duplicates, findings already posted on an earlier run, and findings below `severity_threshold` are removed before `max_findings` applies; a finding whose post failed is not counted | -| `review_url` | URL of the submitted review; empty when no review was posted | -| `model_used` | Model(s) that answered, as named in OpenRouter's response for each completed phase (the routed model, so a phase that fell back reports the fallback model), comma-separated when phases used different models. Empty when the review was skipped | -| `skipped_reason` | Non-empty when the review was skipped (e.g. diff too large) | +| Output | Description | +| ---------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `findings_count` | Number of new findings posted this run. Non-findings, findings on files the model never saw or that diff exclusion removed, duplicates, findings already posted on an earlier run, and findings below `severity_threshold` are removed before `max_findings` applies; a finding whose post failed is not counted | +| `review_url` | URL of the submitted review; empty when no review was posted | +| `model_used` | Model(s) that answered, as named in OpenRouter's response for each completed phase (the routed model, so a phase that fell back reports the fallback model), comma-separated when phases used different models. Empty when the review was skipped | +| `skipped_reason` | Non-empty when the review was skipped (e.g. diff too large) | ## How it works @@ -116,7 +116,7 @@ An empty value, such as an unset repo variable, selects the input's default, so 3. Reads the conventions file, priority docs within their reserved budget, and changed files; then traces imports and scans docs (`.md`, `.json`) with the remaining budget 4. Builds a structured prompt that wraps PR content in tags with a random per-prompt suffix, so PR text can't forge a closing tag to place text outside its tag; on re-runs, prior bot comment bodies are included so the model can self-suppress conceptual duplicates. Sends one request per review phase to OpenRouter (see the `phases` input for dispatch modes) 5. Validates each response against a strict Zod schema. Each model gets up to two attempts, then `fallback_model`, if set, is tried (see that input for timeouts and auth errors). A phase whose attempts all fail, or that reaches the shared review deadline, is named on the status comment and the check run while the other phases' findings still post; the run fails only when no phase completes -6. Drops non-findings (see [Non-finding filter](#non-finding-filter)) and findings on files the model was never given (see [Unknown-file filter](#unknown-file-filter)), collapses findings that two phases reported on overlapping lines of the same file, then on re-runs deduplicates against previously posted bot comments (three-tier: positional match by hidden HTML anchor, content match by title similarity within 50 lines in the same file, or title-only match by high title similarity across any file) +6. Drops non-findings (see [Non-finding filter](#non-finding-filter)) and findings on files the model was never given or that diff exclusion removed (see [Unknown-file filter](#unknown-file-filter)), collapses findings that two phases reported on overlapping lines of the same file, then on re-runs deduplicates against previously posted bot comments (three-tier: positional match by hidden HTML anchor, content match by title similarity within 50 lines in the same file, or title-only match by high title similarity across any file) 7. Filters remaining findings by severity threshold, drops the less severe of two same-category findings that overlap in the same file (on a tie, the one on the later line), and caps if configured 8. Maps findings to inline PR review comments anchored to diff lines, with a snap-to-nearest-hunk fallback 9. Posts one review with inline comments (its body is only a hidden marker); beyond-diff findings post as standalone PR comments; every run upserts a status comment with cross-run totals @@ -145,11 +145,11 @@ A finding's `file` (the path it is filed on) must name a file the model was give - changed files, import-traced related files, mention-matched docs, and priority docs - the conventions file -Files removed by `diff_exclude_paths` or linguist rules are listed by path at the end of the diff, but their content is withheld, so findings on them are dropped too. When the conventions file is excluded, it is still sent as the review's conventions, but findings on it are dropped like those on any other excluded file. The filter compares paths and reports drops as follows: +Files removed by `diff_exclude_paths` or linguist rules are listed by path at the end of the diff, but their content is withheld, so findings on them are dropped too and reported as excluded-file drops. When the conventions file is excluded, it is still sent as the review's conventions, but findings on it are dropped like those on any other excluded file. The filter compares paths and reports drops as follows: - Paths are normalized before comparison: surrounding whitespace, `.` segments, `..` segments that stay inside the repository, repeated slashes, and a leading or trailing `/` don't affect the match (`./src/x.ts`, `/src/x.ts`, and `src/x.ts` match). Matching is case-sensitive, and only whole paths match: a bare filename or a directory prefix does not. - A path containing `"` reaches the model with each `"` written as `"` in the tags that wrap each file's content and the conventions file. A diff header prints the path as the diff spells it, and the filter matches that spelling as written. A model that copies the path from a tag writes `"` into `file`, so a `file` that matches no given path as written, but matches once each `"` becomes `"`, is kept and posts under that decoded path. Each such rewrite is logged at debug level (`resolved escaped finding file to a prompt path`) with the model's spelling and the decoded path. -- Each drop is logged as a warning (`dropping finding: file not in prompt context`) with the file, line, and category; the per-run count appears in the job summary. +- Each drop is logged as a warning with the file, line, and category: `dropping finding: file not in prompt context` for a file the model was never given, and `dropping finding: file excluded from the review diff` for an excluded file. The job summary counts each kind per run, in the `Dropped as unknown file` and `Dropped as excluded file` rows. ## Debug logging diff --git a/action.yml b/action.yml index 293cdec..9313c78 100644 --- a/action.yml +++ b/action.yml @@ -140,7 +140,7 @@ inputs: outputs: findings_count: - description: Number of new findings posted this run. Non-findings, findings on files the model never saw, duplicates, findings already posted on an earlier run, and findings below `severity_threshold` are removed before `max_findings` applies; a finding whose post failed is not counted + description: Number of new findings posted this run. Non-findings, findings on files the model never saw or that diff exclusion removed, duplicates, findings already posted on an earlier run, and findings below `severity_threshold` are removed before `max_findings` applies; a finding whose post failed is not counted review_url: description: URL of the submitted review; empty when no review was posted model_used: diff --git a/src/__tests__/orchestrate.test.ts b/src/__tests__/orchestrate.test.ts index d6b1c9d..c3bfdec 100644 --- a/src/__tests__/orchestrate.test.ts +++ b/src/__tests__/orchestrate.test.ts @@ -137,6 +137,7 @@ const expectedReviewSummary = (overrides: Partial = {}): str totalFromModel: fixtureReviewResponse.findings.length, droppedAsNonFinding: 0, droppedAsUnknownFile: 0, + droppedAsExcludedFile: 0, duplicatesAcrossPhases: 0, duplicatesRemoved: 0, droppedBelowThreshold: 0, @@ -802,7 +803,7 @@ describe("orchestrate", () => { expect(stubs.generateFindingsCalls).toHaveLength(1) }) - it("drops a finding naming an excluded file via the unknown-file filter", async () => { + it("drops a finding naming an excluded file and reports it as an excluded-file drop", async () => { const excludedFileFinding = makeFinding({ file: "assets/logo.png", line: 1, @@ -824,10 +825,12 @@ describe("orchestrate", () => { expect(result.findingsCount).toBe(0) expect(stubs.postFindingsReviewCalls).toHaveLength(0) - expect(logsWithMessage(logger, "dropping finding: file not in prompt context")).toEqual([ + expect( + logsWithMessage(logger, "dropping finding: file excluded from the review diff"), + ).toEqual([ { level: "warn", - message: "dropping finding: file not in prompt context", + message: "dropping finding: file excluded from the review diff", data: { phase: "combined", file: "assets/logo.png", @@ -836,6 +839,24 @@ describe("orchestrate", () => { }, }, ]) + expect(logsWithMessage(logger, "dropping finding: file not in prompt context")).toEqual([]) + expect(logsWithMessage(logger, "per-phase finding filters applied to model output")).toEqual([ + { + level: "info", + message: "per-phase finding filters applied to model output", + data: { + totalFromModel: 1, + kept: 0, + droppedAsNonFinding: 0, + droppedAsUnknownFile: 0, + droppedAsExcludedFile: 1, + duplicatesAcrossPhases: 0, + }, + }, + ]) + expect(result.reviewSummaryMarkdown).toContain( + "| Dropped as unknown file | 0 |\n| Dropped as excluded file | 1 |", + ) }) describe("findings on a changed conventions file", () => { @@ -890,10 +911,12 @@ describe("orchestrate", () => { expect(result.findingsCount).toBe(0) expect(stubs.postFindingsReviewCalls).toEqual([]) expect(stubs.postIssueCommentCalls).toEqual([]) - expect(logsWithMessage(logger, "dropping finding: file not in prompt context")).toEqual([ + expect( + logsWithMessage(logger, "dropping finding: file excluded from the review diff"), + ).toEqual([ { level: "warn", - message: "dropping finding: file not in prompt context", + message: "dropping finding: file excluded from the review diff", data: { phase: "combined", file: "AGENTS.md", @@ -902,6 +925,7 @@ describe("orchestrate", () => { }, }, ]) + expect(logsWithMessage(logger, "dropping finding: file not in prompt context")).toEqual([]) // The nonce is random per call; the backreference pins both tags to the same one expect(userPrompts).toEqual([ expect.stringMatching( @@ -922,10 +946,12 @@ describe("orchestrate", () => { expect(result.findingsCount).toBe(0) expect(stubs.postFindingsReviewCalls).toEqual([]) expect(stubs.postIssueCommentCalls).toEqual([]) - expect(logsWithMessage(logger, "dropping finding: file not in prompt context")).toEqual([ + expect( + logsWithMessage(logger, "dropping finding: file excluded from the review diff"), + ).toEqual([ { level: "warn", - message: "dropping finding: file not in prompt context", + message: "dropping finding: file excluded from the review diff", data: { phase: "combined", file: "AGENTS.md", @@ -934,6 +960,7 @@ describe("orchestrate", () => { }, }, ]) + expect(logsWithMessage(logger, "dropping finding: file not in prompt context")).toEqual([]) }) it("keeps a finding on a conventions file that stays in the review diff", async () => { @@ -2837,6 +2864,7 @@ describe("orchestrate", () => { kept: 1, droppedAsNonFinding: 0, droppedAsUnknownFile: 1, + droppedAsExcludedFile: 0, duplicatesAcrossPhases: 0, }, }, @@ -3085,6 +3113,7 @@ describe("orchestrate", () => { kept: 1, droppedAsNonFinding: 1, droppedAsUnknownFile: 0, + droppedAsExcludedFile: 0, duplicatesAcrossPhases: 0, }, }, @@ -3209,6 +3238,7 @@ describe("orchestrate", () => { kept: 1, droppedAsNonFinding: 0, droppedAsUnknownFile: 1, + droppedAsExcludedFile: 0, duplicatesAcrossPhases: 0, }, }, diff --git a/src/orchestrate.ts b/src/orchestrate.ts index f9b9535..7022f92 100644 --- a/src/orchestrate.ts +++ b/src/orchestrate.ts @@ -56,7 +56,10 @@ import { type PromptFile, } from "./review/prompt.js" import { filterNonFindings } from "./review/filter-non-findings.js" -import { filterUnknownFileFindings } from "./review/filter-unknown-file-findings.js" +import { + filterUnknownFileFindings, + type UnknownFileFilterResult, +} from "./review/filter-unknown-file-findings.js" import { mergePhaseFindings } from "./review/merge-phase-findings.js" import { renderReviewSummary } from "./review/review-summary.js" import { @@ -431,13 +434,20 @@ type FilteredPhaseFindings = { findings: AttributedFinding[] droppedAsNonFinding: number droppedAsUnknownFile: number + droppedAsExcludedFile: number } /** Drops non-findings and findings on files the model never saw. A kept * finding whose `file` matched only once `"` was decoded comes back * with the decoded `file`. */ const filterPhaseFindings = ( - { outcome, knownPaths }: { outcome: CompletedPhase; knownPaths: string[] }, + { + outcome, + filterFindingFiles, + }: { + outcome: CompletedPhase + filterFindingFiles: (findings: Finding[]) => UnknownFileFilterResult + }, logger: Logger, ): FilteredPhaseFindings => { const { findings: realFindings, droppedAsNonFinding } = filterNonFindings( @@ -446,11 +456,9 @@ const filterPhaseFindings = ( const { findings, droppedAsUnknownFile: unknownFileFindings, + droppedAsExcludedFile: excludedFileFindings, unescapedFileRewrites, - } = filterUnknownFileFindings({ - findings: realFindings, - knownPaths, - }) + } = filterFindingFiles(realFindings) // Each drop warns on its own because it is a model-quality event, not loop // chatter. The title is left out because the model wrote it about a file it @@ -464,6 +472,18 @@ const filterPhaseFindings = ( }) } + // An excluded file's path is in the excluded-files note, and an excluded + // conventions file's content is in the prompt. A separate message points + // the operator at the exclusion rule, not at context assembly. + for (const finding of excludedFileFindings) { + logger.warn("dropping finding: file excluded from the review diff", { + phase: outcome.phase.id, + file: finding.file, + line: finding.line, + category: finding.category, + }) + } + // A rewritten file changes where the finding posts and its dedup anchor, so // the log keeps both spellings. It logs at debug, not warn, because the // finding still posts, on the path the prompt gave. @@ -483,6 +503,7 @@ const filterPhaseFindings = ( })), droppedAsNonFinding, droppedAsUnknownFile: unknownFileFindings.length, + droppedAsExcludedFile: excludedFileFindings.length, } } @@ -1015,12 +1036,20 @@ const runReviewPipeline = async ( // Each phase drops non-findings and findings on files the model never saw, // then cross-phase duplicates collapse. All of it runs before selection so // cap slots aren't wasted + const filterFindingFiles = (findings: Finding[]): UnknownFileFilterResult => { + return filterUnknownFileFindings({ + findings, + knownPaths: promptFilePaths, + excludedPaths: diffExcludedPaths, + }) + } const filteredPhases = completedPhases.map((outcome) => { - return filterPhaseFindings({ outcome, knownPaths: promptFilePaths }, logger) + return filterPhaseFindings({ outcome, filterFindingFiles }, logger) }) const totalFromModel = sumBy(completedPhases, (outcome) => outcome.result.review.findings.length) const droppedAsNonFinding = sumBy(filteredPhases, (filtered) => filtered.droppedAsNonFinding) const droppedAsUnknownFile = sumBy(filteredPhases, (filtered) => filtered.droppedAsUnknownFile) + const droppedAsExcludedFile = sumBy(filteredPhases, (filtered) => filtered.droppedAsExcludedFile) const { findings: realFindings, duplicatesAcrossPhases } = mergePhaseFindings( filteredPhases.map((filtered) => filtered.findings), ) @@ -1029,6 +1058,7 @@ const runReviewPipeline = async ( kept: realFindings.length, droppedAsNonFinding, droppedAsUnknownFile, + droppedAsExcludedFile, duplicatesAcrossPhases, }) @@ -1194,6 +1224,7 @@ const runReviewPipeline = async ( totalFromModel, droppedAsNonFinding, droppedAsUnknownFile, + droppedAsExcludedFile, duplicatesAcrossPhases, duplicatesRemoved: realFindings.length - newFindings.length, droppedBelowThreshold, diff --git a/src/review/__tests__/filter-unknown-file-findings.test.ts b/src/review/__tests__/filter-unknown-file-findings.test.ts index 7a3216c..977dc3e 100644 --- a/src/review/__tests__/filter-unknown-file-findings.test.ts +++ b/src/review/__tests__/filter-unknown-file-findings.test.ts @@ -12,10 +12,12 @@ describe("filterUnknownFileFindings", () => { filterUnknownFileFindings({ findings: [finding, unknownFinding], knownPaths: ["src/greeter.ts"], + excludedPaths: [], }), ).toEqual({ findings: [finding], droppedAsUnknownFile: [unknownFinding], + droppedAsExcludedFile: [], unescapedFileRewrites: [], }) }) @@ -32,10 +34,12 @@ describe("filterUnknownFileFindings", () => { filterUnknownFileFindings({ findings: [finding, exactFinding], knownPaths: ["deploy/railway/README.md"], + excludedPaths: [], }), ).toEqual({ findings: [exactFinding], droppedAsUnknownFile: [finding], + droppedAsExcludedFile: [], unescapedFileRewrites: [], }) }) @@ -47,8 +51,14 @@ describe("filterUnknownFileFindings", () => { filterUnknownFileFindings({ findings: [finding], knownPaths: ["src/greeter.ts"], + excludedPaths: [], }), - ).toEqual({ findings: [], droppedAsUnknownFile: [finding], unescapedFileRewrites: [] }) + ).toEqual({ + findings: [], + droppedAsUnknownFile: [finding], + droppedAsExcludedFile: [], + unescapedFileRewrites: [], + }) }) it("filters selectively in a mixed set, preserving order", () => { @@ -60,10 +70,12 @@ describe("filterUnknownFileFindings", () => { filterUnknownFileFindings({ findings: [first, unknown, third], knownPaths: ["src/greeter.ts"], + excludedPaths: [], }), ).toEqual({ findings: [first, third], droppedAsUnknownFile: [unknown], + droppedAsExcludedFile: [], unescapedFileRewrites: [], }) }) @@ -76,10 +88,12 @@ describe("filterUnknownFileFindings", () => { filterUnknownFileFindings({ findings: [finding, unknownFinding], knownPaths: ["src/greeter.ts"], + excludedPaths: [], }), ).toEqual({ findings: [finding], droppedAsUnknownFile: [unknownFinding], + droppedAsExcludedFile: [], unescapedFileRewrites: [], }) expect(finding.file).toBe("./src/greeter.ts") @@ -93,10 +107,12 @@ describe("filterUnknownFileFindings", () => { filterUnknownFileFindings({ findings: [finding, unknownFinding], knownPaths: ["./src/greeter.ts"], + excludedPaths: [], }), ).toEqual({ findings: [finding], droppedAsUnknownFile: [unknownFinding], + droppedAsExcludedFile: [], unescapedFileRewrites: [], }) }) @@ -134,10 +150,12 @@ describe("filterUnknownFileFindings", () => { filterUnknownFileFindings({ findings: [finding, unknownFinding], knownPaths: [knownPath], + excludedPaths: [], }), ).toEqual({ findings: [finding], droppedAsUnknownFile: [unknownFinding], + droppedAsExcludedFile: [], unescapedFileRewrites: [], }) }) @@ -150,10 +168,12 @@ describe("filterUnknownFileFindings", () => { filterUnknownFileFindings({ findings: [finding, unknownFinding], knownPaths: ["src/greeter.ts"], + excludedPaths: [], }), ).toEqual({ findings: [finding], droppedAsUnknownFile: [unknownFinding], + droppedAsExcludedFile: [], unescapedFileRewrites: [], }) }) @@ -166,10 +186,12 @@ describe("filterUnknownFileFindings", () => { filterUnknownFileFindings({ findings: [finding, unknownFinding], knownPaths: ["src/greeter.ts"], + excludedPaths: [], }), ).toEqual({ findings: [finding], droppedAsUnknownFile: [unknownFinding], + droppedAsExcludedFile: [], unescapedFileRewrites: [], }) }) @@ -182,10 +204,12 @@ describe("filterUnknownFileFindings", () => { filterUnknownFileFindings({ findings: [finding, unknownFinding], knownPaths: ["src/greeter.ts", "src/caller.ts"], + excludedPaths: [], }), ).toEqual({ findings: [finding], droppedAsUnknownFile: [unknownFinding], + droppedAsExcludedFile: [], unescapedFileRewrites: [], }) }) @@ -200,10 +224,12 @@ describe("filterUnknownFileFindings", () => { filterUnknownFileFindings({ findings: [escapedFinding, unrelatedFinding, undecodableFinding], knownPaths: ["src/greeter.ts", 'docs/a"b.md'], + excludedPaths: [], }), ).toEqual({ findings: [resolvedFinding, unrelatedFinding], droppedAsUnknownFile: [undecodableFinding], + droppedAsExcludedFile: [], unescapedFileRewrites: [{ writtenFile: "docs/a"b.md", finding: resolvedFinding }], }) expect(escapedFinding.file).toBe("docs/a"b.md") @@ -218,10 +244,12 @@ describe("filterUnknownFileFindings", () => { filterUnknownFileFindings({ findings: [escapedFinding, unknownFinding], knownPaths: ['docs/a&"b.md'], + excludedPaths: [], }), ).toEqual({ findings: [resolvedFinding], droppedAsUnknownFile: [unknownFinding], + droppedAsExcludedFile: [], unescapedFileRewrites: [{ writtenFile: "./docs/a&"b.md", finding: resolvedFinding }], }) }) @@ -234,10 +262,12 @@ describe("filterUnknownFileFindings", () => { filterUnknownFileFindings({ findings: [finding, unknownFinding], knownPaths: ['docs/a"b.md', "docs/a"b.md"], + excludedPaths: [], }), ).toEqual({ findings: [finding], droppedAsUnknownFile: [unknownFinding], + droppedAsExcludedFile: [], unescapedFileRewrites: [], }) }) @@ -251,10 +281,12 @@ describe("filterUnknownFileFindings", () => { filterUnknownFileFindings({ findings: [finding, escapedFinding], knownPaths: ['docs/a"b.md', "docs/cd.md"], + excludedPaths: [], }), ).toEqual({ findings: [resolvedFinding], droppedAsUnknownFile: [finding], + droppedAsExcludedFile: [], unescapedFileRewrites: [{ writtenFile: "docs/a"b.md", finding: resolvedFinding }], }) }) @@ -303,10 +335,12 @@ describe("filterUnknownFileFindings", () => { filterUnknownFileFindings({ findings: [finding, unknownFinding], knownPaths: [relatedDoc.path], + excludedPaths: [], }), ).toEqual({ findings: [resolvedFinding], droppedAsUnknownFile: [unknownFinding], + droppedAsExcludedFile: [], unescapedFileRewrites: [{ writtenFile, finding: resolvedFinding }], }) }) @@ -319,10 +353,12 @@ describe("filterUnknownFileFindings", () => { filterUnknownFileFindings({ findings: [finding, fullPathFinding], knownPaths: ["src/greeter.ts"], + excludedPaths: [], }), ).toEqual({ findings: [fullPathFinding], droppedAsUnknownFile: [finding], + droppedAsExcludedFile: [], unescapedFileRewrites: [], }) }) @@ -335,10 +371,12 @@ describe("filterUnknownFileFindings", () => { filterUnknownFileFindings({ findings: [finding, fullPathFinding], knownPaths: ["src/greeter.ts"], + excludedPaths: [], }), ).toEqual({ findings: [fullPathFinding], droppedAsUnknownFile: [finding], + droppedAsExcludedFile: [], unescapedFileRewrites: [], }) }) @@ -346,9 +384,85 @@ describe("filterUnknownFileFindings", () => { it("drops every finding when no paths are known", () => { const finding = makeFinding() - expect(filterUnknownFileFindings({ findings: [finding], knownPaths: [] })).toEqual({ + expect( + filterUnknownFileFindings({ findings: [finding], knownPaths: [], excludedPaths: [] }), + ).toEqual({ findings: [], droppedAsUnknownFile: [finding], + droppedAsExcludedFile: [], + unescapedFileRewrites: [], + }) + }) + + it("drops a finding on an excluded path into its own bucket, apart from unknown-file drops", () => { + const keptFinding = makeFinding() + const excludedFinding = makeFinding({ file: "assets/logo.png", line: 1 }) + const unknownFinding = makeFinding({ file: "src/imagined.ts", line: 2 }) + + expect( + filterUnknownFileFindings({ + findings: [keptFinding, excludedFinding, unknownFinding], + knownPaths: ["src/greeter.ts"], + excludedPaths: ["assets/logo.png"], + }), + ).toEqual({ + findings: [keptFinding], + droppedAsUnknownFile: [unknownFinding], + droppedAsExcludedFile: [excludedFinding], + unescapedFileRewrites: [], + }) + }) + + it("normalizes excluded paths the same way as known paths", () => { + const excludedFinding = makeFinding({ file: "./AGENTS.md", line: 1 }) + const basenameFinding = makeFinding({ file: "logo.png", line: 2 }) + + expect( + filterUnknownFileFindings({ + findings: [excludedFinding, basenameFinding], + knownPaths: ["src/greeter.ts"], + excludedPaths: ["/AGENTS.md/", "assets/logo.png"], + }), + ).toEqual({ + findings: [], + droppedAsUnknownFile: [basenameFinding], + droppedAsExcludedFile: [excludedFinding], + unescapedFileRewrites: [], + }) + }) + + it("buckets a finding as excluded when its " spelling decodes to an excluded path, without rewriting it", () => { + const escapedFinding = makeFinding({ file: "docs/a"b.md", line: 1 }) + const unknownFinding = makeFinding({ file: "docs/a"c.md", line: 2 }) + + expect( + filterUnknownFileFindings({ + findings: [escapedFinding, unknownFinding], + knownPaths: ["src/greeter.ts"], + excludedPaths: ['docs/a"b.md'], + }), + ).toEqual({ + findings: [], + droppedAsUnknownFile: [unknownFinding], + droppedAsExcludedFile: [escapedFinding], + unescapedFileRewrites: [], + }) + }) + + it("keeps a finding on a path that is both known and excluded", () => { + const knownFinding = makeFinding({ file: "AGENTS.md", line: 1 }) + const excludedFinding = makeFinding({ file: "assets/logo.png", line: 2 }) + + expect( + filterUnknownFileFindings({ + findings: [knownFinding, excludedFinding], + knownPaths: ["AGENTS.md"], + excludedPaths: ["AGENTS.md", "assets/logo.png"], + }), + ).toEqual({ + findings: [knownFinding], + droppedAsUnknownFile: [], + droppedAsExcludedFile: [excludedFinding], unescapedFileRewrites: [], }) }) @@ -358,7 +472,13 @@ describe("filterUnknownFileFindings", () => { filterUnknownFileFindings({ findings: [], knownPaths: ["src/greeter.ts"], + excludedPaths: [], }), - ).toEqual({ findings: [], droppedAsUnknownFile: [], unescapedFileRewrites: [] }) + ).toEqual({ + findings: [], + droppedAsUnknownFile: [], + droppedAsExcludedFile: [], + unescapedFileRewrites: [], + }) }) }) diff --git a/src/review/__tests__/review-summary.test.ts b/src/review/__tests__/review-summary.test.ts index 288d744..13646fa 100644 --- a/src/review/__tests__/review-summary.test.ts +++ b/src/review/__tests__/review-summary.test.ts @@ -29,6 +29,7 @@ const baseStats: ReviewSummaryStats = { totalFromModel: 3, droppedAsNonFinding: 0, droppedAsUnknownFile: 0, + droppedAsExcludedFile: 0, duplicatesAcrossPhases: 0, duplicatesRemoved: 0, droppedBelowThreshold: 0, @@ -73,6 +74,7 @@ describe("renderReviewSummary", () => { "| Raw from model | 3 |", "| Dropped as non-findings | 0 |", "| Dropped as unknown file | 0 |", + "| Dropped as excluded file | 0 |", "| Duplicates (cross-phase) | 0 |", "| Duplicates (cross-run) | 0 |", "| Dropped below threshold | 0 |", @@ -128,6 +130,7 @@ describe("renderReviewSummary", () => { "| Raw from model | 3 |", "| Dropped as non-findings | 0 |", "| Dropped as unknown file | 0 |", + "| Dropped as excluded file | 0 |", "| Duplicates (cross-phase) | 0 |", "| Duplicates (cross-run) | 0 |", "| Dropped below threshold | 0 |", @@ -184,6 +187,7 @@ describe("renderReviewSummary", () => { "| Raw from model | 11 |", "| Dropped as non-findings | 2 |", "| Dropped as unknown file | 1 |", + "| Dropped as excluded file | 0 |", "| Duplicates (cross-phase) | 2 |", "| Duplicates (cross-run) | 3 |", "| Dropped below threshold | 1 |", @@ -194,6 +198,35 @@ describe("renderReviewSummary", () => { ) }) + it("reports excluded-file drops on their own row next to unknown-file drops", () => { + const summary = renderReviewSummary({ + ...baseStats, + totalFromModel: 6, + droppedAsUnknownFile: 1, + droppedAsExcludedFile: 2, + posted: 3, + }) + + expect(summary).toContain( + [ + "#### Findings pipeline", + "", + "| stage | count |", + "| --- | --- |", + "| Raw from model | 6 |", + "| Dropped as non-findings | 0 |", + "| Dropped as unknown file | 1 |", + "| Dropped as excluded file | 2 |", + "| Duplicates (cross-phase) | 0 |", + "| Duplicates (cross-run) | 0 |", + "| Dropped below threshold | 0 |", + "| Dropped as overlapping | 0 |", + "| Dropped by cap | 0 |", + "| **Posted** | **3** |", + ].join("\n"), + ) + }) + it.each([false, true])( "lists phase coverage and reports deadline expiry when reviewDeadlineExceeded is %s", (reviewDeadlineExceeded) => { @@ -239,6 +272,7 @@ describe("renderReviewSummary", () => { "| Raw from model | 3 |", "| Dropped as non-findings | 0 |", "| Dropped as unknown file | 0 |", + "| Dropped as excluded file | 0 |", "| Duplicates (cross-phase) | 0 |", "| Duplicates (cross-run) | 0 |", "| Dropped below threshold | 0 |", diff --git a/src/review/filter-unknown-file-findings.ts b/src/review/filter-unknown-file-findings.ts index 1f64eb4..763ca2e 100644 --- a/src/review/filter-unknown-file-findings.ts +++ b/src/review/filter-unknown-file-findings.ts @@ -14,6 +14,9 @@ export type UnknownFileFilterResult = { findings: Finding[] /** Whole findings, not a count — the caller logs each drop. */ droppedAsUnknownFile: Finding[] + /** Dropped findings whose file diff exclusion removed from the review. The + * caller logs them apart from unknown-file drops. */ + droppedAsExcludedFile: Finding[] /** Each rewritten finding is also in `findings`. The caller logs each rewrite. */ unescapedFileRewrites: UnescapedFileRewrite[] } @@ -21,33 +24,44 @@ export type UnknownFileFilterResult = { type FileResolution = | { kind: "known"; finding: Finding } | { kind: "unescaped"; finding: Finding; writtenFile: string } + | { kind: "excluded"; finding: Finding } | { kind: "unknown"; finding: Finding } /** * Drops findings whose `file` names no file the model was given. A finding on * a path the model never saw is ungrounded by construction; without this gate * it would route to a beyond-diff comment. - * - `file` and each known path pass through normalizeWorkspacePath (trim, - * resolve `.` and `..` segments, collapse repeated `/`, strip a leading or - * trailing `/`). After that, membership is exact, never by basename or - * prefix. + * - `file`, each known path, and each excluded path pass through + * normalizeWorkspacePath (trim, resolve `.` and `..` segments, collapse + * repeated `/`, strip a leading or trailing `/`). After that, membership is + * exact, never by basename or prefix. * - Only the comparison is normalized. A `file` that matches as written is * kept exactly as the model wrote it. * - A `file` that matches only after each `"` becomes `"` is kept with * that decoded spelling. File-block and conventions path attributes escape * `"` as `"`, and the model copies the attribute verbatim. + * - A dropped finding whose `file` is an excluded path, in either spelling, + * lands in `droppedAsExcludedFile` instead of `droppedAsUnknownFile`. The + * known check runs first, so a path both known and excluded keeps its + * finding. */ export const filterUnknownFileFindings = ({ findings, knownPaths, + excludedPaths, }: { findings: Finding[] knownPaths: string[] + excludedPaths: string[] }): UnknownFileFilterResult => { const knownPathSet = new Set(knownPaths.map(normalizeWorkspacePath)) const isKnownPath = (filePath: string): boolean => { return knownPathSet.has(normalizeWorkspacePath(filePath)) } + const excludedPathSet = new Set(excludedPaths.map(normalizeWorkspacePath)) + const isExcludedPath = (filePath: string): boolean => { + return excludedPathSet.has(normalizeWorkspacePath(filePath)) + } // A real file name can contain the literal text """, and decoding it // would name a different file, so the written spelling is checked first @@ -63,18 +77,27 @@ export const filterUnknownFileFindings = ({ writtenFile: finding.file, } } + + const namesExcludedPath = isExcludedPath(finding.file) || isExcludedPath(unescapedFile) + + if (namesExcludedPath) return { kind: "excluded", finding } return { kind: "unknown", finding } } const resolutions = findings.map(resolveFile) + const findingsOfKind = (kind: FileResolution["kind"]): Finding[] => { + return resolutions + .filter((resolution) => resolution.kind === kind) + .map((resolution) => resolution.finding) + } + const isKept = (resolution: FileResolution): boolean => { + return resolution.kind === "known" || resolution.kind === "unescaped" + } return { - findings: resolutions - .filter((resolution) => resolution.kind !== "unknown") - .map((resolution) => resolution.finding), - droppedAsUnknownFile: resolutions - .filter((resolution) => resolution.kind === "unknown") - .map((resolution) => resolution.finding), + findings: resolutions.filter(isKept).map((resolution) => resolution.finding), + droppedAsUnknownFile: findingsOfKind("unknown"), + droppedAsExcludedFile: findingsOfKind("excluded"), unescapedFileRewrites: resolutions .filter((resolution) => resolution.kind === "unescaped") .map(({ finding, writtenFile }) => ({ writtenFile, finding })), diff --git a/src/review/review-summary.ts b/src/review/review-summary.ts index a225111..e5ccc9d 100644 --- a/src/review/review-summary.ts +++ b/src/review/review-summary.ts @@ -29,6 +29,8 @@ export type ReviewSummaryStats = { droppedAsNonFinding: number /** Findings naming a file the model was never given. */ droppedAsUnknownFile: number + /** Findings on a changed file diff exclusion removed from the review. */ + droppedAsExcludedFile: number /** Findings two phases reported on overlapping lines of one file. */ duplicatesAcrossPhases: number duplicatesRemoved: number @@ -123,6 +125,7 @@ export const renderReviewSummary = (stats: ReviewSummaryStats): string => { `| Raw from model | ${stats.totalFromModel} |`, `| Dropped as non-findings | ${stats.droppedAsNonFinding} |`, `| Dropped as unknown file | ${stats.droppedAsUnknownFile} |`, + `| Dropped as excluded file | ${stats.droppedAsExcludedFile} |`, `| Duplicates (cross-phase) | ${stats.duplicatesAcrossPhases} |`, `| Duplicates (cross-run) | ${stats.duplicatesRemoved} |`, `| Dropped below threshold | ${stats.droppedBelowThreshold} |`, From d8f0e403857441a16c98f706446e9551cfd5c9a8 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Wed, 30 Sep 2026 19:58:23 -0400 Subject: [PATCH 14/14] test(orchestrate): post a decoded escaped changed-file finding inline Co-Authored-By: Claude Opus 5.5 --- src/__tests__/orchestrate.test.ts | 41 +++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/src/__tests__/orchestrate.test.ts b/src/__tests__/orchestrate.test.ts index a292ba1..b02f40d 100644 --- a/src/__tests__/orchestrate.test.ts +++ b/src/__tests__/orchestrate.test.ts @@ -2926,6 +2926,47 @@ describe("orchestrate", () => { ]) }) + it("posts a finding on an escaped changed-file path inline under the decoded diff path", async () => { + // Git C-quotes a path holding a double quote, and parse-diff keeps the + // backslash, so the diff path and the file block path are docs/a\"b.md + const quotedPathDiff = `${sampleDiff}diff --git "a/docs/a\\"b.md" "b/docs/a\\"b.md"\nindex 1111111..2222222 100644\n--- "a/docs/a\\"b.md"\n+++ "b/docs/a\\"b.md"\n@@ -1,2 +1,2 @@\n # Title\n-old line\n+new line\n` + const decodedPath = 'docs/a\\"b.md' + const escapedFinding = makeFinding({ file: "docs/a\\"b.md", line: 2 }) + const stubs = makeOrchestrateDeps({ + fixtureResult: { review: { analysis: "checked", findings: [escapedFinding] } }, + githubClient: { + fetchDiff: async () => ({ kind: "ok" as const, diff: quotedPathDiff }), + }, + contextReader: { + readChangedFiles: async () => ({ + files: [ + fixtureChangedFile, + { path: decodedPath, content: "# Title\nnew line", includedAs: "full" }, + ], + remainingTokens: 40_000, + }), + }, + }) + const logger = createTestLogger() + + await orchestrate(stubs.deps, logger) + + const expectedMapped = mapFindingsToReview({ + findings: findingsWithRoutedModel([{ ...escapedFinding, file: decodedPath }], "test/model"), + commentableByPath: computeCommentableLines(parseDiff(quotedPathDiff)), + }) + expect(expectedMapped.comments.map((comment) => comment.path)).toEqual([decodedPath]) + expect(stubs.postFindingsReviewCalls).toEqual([ + { + prNumber: 7, + commitId: fixturePrContext.headSha, + body: REVIEW_MARKER, + comments: expectedMapped.comments, + }, + ]) + expect(stubs.postIssueCommentCalls).toEqual([]) + }) + it("continues when the findings review post throws", async () => { const stubs = makeOrchestrateDeps({ githubClient: {