diff --git a/README.md b/README.md index 7d91856..1804344 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` | @@ -102,24 +102,24 @@ 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 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 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, deduplicates overlapping findings within the run, and caps if configured +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 (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,18 +128,28 @@ 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-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 …` - 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. 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` (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 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 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 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 71ac805..b02f40d 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, @@ -805,7 +806,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, @@ -827,10 +828,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", @@ -839,6 +842,151 @@ 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", () => { + 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, + conventionsFile = "AGENTS.md", + }: { + diffExcludePathPatterns: string[] + conventionsFile?: 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, + 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({ + diffExcludePathPatterns: ["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 excluded from the review diff"), + ).toEqual([ + { + level: "warn", + message: "dropping finding: file excluded from the review diff", + data: { + phase: "combined", + file: "AGENTS.md", + line: 1, + category: conventionsFinding.category, + }, + }, + ]) + 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( + /\n# Test conventions\n<\/conventions-\1 path="AGENTS\.md">/, + ), + ]) + }) + + 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 excluded from the review diff"), + ).toEqual([ + { + level: "warn", + message: "dropping finding: file excluded from the review diff", + data: { + phase: "combined", + file: "AGENTS.md", + line: 1, + category: conventionsFinding.category, + }, + }, + ]) + 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 () => { + const { stubs } = makeConventionsFindingDeps({ diffExcludePathPatterns: [] }) + 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 () => { @@ -2705,6 +2853,120 @@ 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, "per-phase finding filters applied to model output")).toEqual([ + { + level: "info", + message: "per-phase finding filters applied to model output", + data: { + totalFromModel: 2, + kept: 1, + droppedAsNonFinding: 0, + droppedAsUnknownFile: 1, + droppedAsExcludedFile: 0, + duplicatesAcrossPhases: 0, + }, + }, + ]) + }) + + 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: { @@ -2938,15 +3200,16 @@ 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, droppedAsNonFinding: 1, droppedAsUnknownFile: 0, + droppedAsExcludedFile: 0, duplicatesAcrossPhases: 0, }, }, @@ -3062,15 +3325,16 @@ 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, droppedAsNonFinding: 0, droppedAsUnknownFile: 1, + droppedAsExcludedFile: 0, duplicatesAcrossPhases: 0, }, }, @@ -3361,6 +3625,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/orchestrate.ts b/src/orchestrate.ts index 75188be..29342db 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,24 +434,36 @@ type FilteredPhaseFindings = { findings: AttributedFinding[] droppedAsNonFinding: number droppedAsUnknownFile: number + droppedAsExcludedFile: 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[] }, + { + outcome, + filterFindingFiles, + }: { + outcome: CompletedPhase + filterFindingFiles: (findings: Finding[]) => UnknownFileFilterResult + }, logger: Logger, ): FilteredPhaseFindings => { - const { findings: nonFindingFiltered, droppedAsNonFinding } = filterNonFindings( + const { findings: realFindings, droppedAsNonFinding } = filterNonFindings( outcome.result.review.findings, ) - const { findings, droppedAsUnknownFile } = filterUnknownFileFindings({ - findings: nonFindingFiltered, - knownPaths, - }) + const { + findings, + droppedAsUnknownFile: unknownFileFindings, + droppedAsExcludedFile: excludedFileFindings, + unescapedFileRewrites, + } = filterFindingFiles(realFindings) // 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, @@ -456,13 +471,39 @@ const filterPhaseFindings = ( category: finding.category, }) } + + // 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. + 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, modelUsed: outcome.result.modelUsed, })), droppedAsNonFinding, - droppedAsUnknownFile: droppedAsUnknownFile.length, + droppedAsUnknownFile: unknownFileFindings.length, + droppedAsExcludedFile: excludedFileFindings.length, } } @@ -726,9 +767,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 @@ -871,19 +912,34 @@ 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 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", { @@ -966,7 +1022,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, ) @@ -982,20 +1040,29 @@ 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), ) - logger.info("non-finding filter applied to model output", { + logger.info("per-phase finding filters applied to model output", { totalFromModel, kept: realFindings.length, droppedAsNonFinding, droppedAsUnknownFile, + droppedAsExcludedFile, duplicatesAcrossPhases, }) @@ -1161,6 +1228,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 b60fbe6..977dc3e 100644 --- a/src/review/__tests__/filter-unknown-file-findings.test.ts +++ b/src/review/__tests__/filter-unknown-file-findings.test.ts @@ -1,17 +1,25 @@ 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" 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"], + excludedPaths: [], }), - ).toEqual({ findings: [finding], droppedAsUnknownFile: [] }) + ).toEqual({ + findings: [finding], + droppedAsUnknownFile: [unknownFinding], + droppedAsExcludedFile: [], + unescapedFileRewrites: [], + }) }) it("drops a finding whose file is a known path with prose appended", () => { @@ -20,13 +28,20 @@ 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"], + excludedPaths: [], }), - ).toEqual({ findings: [], droppedAsUnknownFile: [finding] }) + ).toEqual({ + findings: [exactFinding], + droppedAsUnknownFile: [finding], + droppedAsExcludedFile: [], + unescapedFileRewrites: [], + }) }) it("drops a finding whose file matches no known path", () => { @@ -36,8 +51,14 @@ describe("filterUnknownFileFindings", () => { filterUnknownFileFindings({ findings: [finding], knownPaths: ["src/greeter.ts"], + excludedPaths: [], }), - ).toEqual({ findings: [], droppedAsUnknownFile: [finding] }) + ).toEqual({ + findings: [], + droppedAsUnknownFile: [finding], + droppedAsExcludedFile: [], + unescapedFileRewrites: [], + }) }) it("filters selectively in a mixed set, preserving order", () => { @@ -49,94 +70,400 @@ describe("filterUnknownFileFindings", () => { filterUnknownFileFindings({ findings: [first, unknown, third], knownPaths: ["src/greeter.ts"], + excludedPaths: [], }), - ).toEqual({ findings: [first, third], droppedAsUnknownFile: [unknown] }) + ).toEqual({ + findings: [first, third], + droppedAsUnknownFile: [unknown], + droppedAsExcludedFile: [], + unescapedFileRewrites: [], + }) }) 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"], + excludedPaths: [], }), - ).toEqual({ findings: [finding], droppedAsUnknownFile: [] }) + ).toEqual({ + findings: [finding], + droppedAsUnknownFile: [unknownFinding], + droppedAsExcludedFile: [], + 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"], + excludedPaths: [], }), - ).toEqual({ findings: [finding], droppedAsUnknownFile: [] }) + ).toEqual({ + findings: [finding], + droppedAsUnknownFile: [unknownFinding], + droppedAsExcludedFile: [], + unescapedFileRewrites: [], + }) + }) + + 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], + excludedPaths: [], + }), + ).toEqual({ + findings: [finding], + droppedAsUnknownFile: [unknownFinding], + droppedAsExcludedFile: [], + 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"], + excludedPaths: [], }), - ).toEqual({ findings: [finding], droppedAsUnknownFile: [] }) + ).toEqual({ + findings: [finding], + droppedAsUnknownFile: [unknownFinding], + droppedAsExcludedFile: [], + 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"], + excludedPaths: [], }), - ).toEqual({ findings: [finding], droppedAsUnknownFile: [] }) + ).toEqual({ + findings: [finding], + droppedAsUnknownFile: [unknownFinding], + droppedAsExcludedFile: [], + 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"], + excludedPaths: [], + }), + ).toEqual({ + findings: [finding], + droppedAsUnknownFile: [unknownFinding], + droppedAsExcludedFile: [], + 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, 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") + }) + + 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, unknownFinding], + knownPaths: ['docs/a&"b.md'], + excludedPaths: [], + }), + ).toEqual({ + findings: [resolvedFinding], + droppedAsUnknownFile: [unknownFinding], + droppedAsExcludedFile: [], + 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, unknownFinding], + knownPaths: ['docs/a"b.md', "docs/a"b.md"], + excludedPaths: [], + }), + ).toEqual({ + findings: [finding], + droppedAsUnknownFile: [unknownFinding], + droppedAsExcludedFile: [], + unescapedFileRewrites: [], + }) + }) + + 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, escapedFinding], + knownPaths: ['docs/a"b.md', "docs/cd.md"], + excludedPaths: [], + }), + ).toEqual({ + findings: [resolvedFinding], + droppedAsUnknownFile: [finding], + droppedAsExcludedFile: [], + unescapedFileRewrites: [{ writtenFile: "docs/a"b.md", finding: resolvedFinding }], + }) + }) + + it("keeps a finding whose file is copied from a rendered path attribute", () => { + const relatedDoc = { + // 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", + } + 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", + }) + + /** The file block's path attribute, exactly as the model reads it. */ + const fileBlockPathAttribute = / { const finding = makeFinding({ file: "greeter.ts" }) + const fullPathFinding = makeFinding({ line: 12 }) expect( filterUnknownFileFindings({ - findings: [finding], + findings: [finding, fullPathFinding], knownPaths: ["src/greeter.ts"], + excludedPaths: [], }), - ).toEqual({ findings: [], droppedAsUnknownFile: [finding] }) + ).toEqual({ + findings: [fullPathFinding], + droppedAsUnknownFile: [finding], + droppedAsExcludedFile: [], + 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"], + excludedPaths: [], }), - ).toEqual({ findings: [], droppedAsUnknownFile: [finding] }) + ).toEqual({ + findings: [fullPathFinding], + droppedAsUnknownFile: [finding], + droppedAsExcludedFile: [], + unescapedFileRewrites: [], + }) }) 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: [], }) }) @@ -145,7 +472,13 @@ describe("filterUnknownFileFindings", () => { filterUnknownFileFindings({ findings: [], knownPaths: ["src/greeter.ts"], + excludedPaths: [], }), - ).toEqual({ findings: [], droppedAsUnknownFile: [] }) + ).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 4521549..763ca2e 100644 --- a/src/review/filter-unknown-file-findings.ts +++ b/src/review/filter-unknown-file-findings.ts @@ -1,31 +1,105 @@ import type { Finding } from "./finding.js" +import { unescapeAttributeValue } from "./prompt.js" import { normalizeWorkspacePath } from "./workspace-path.js" +/** 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 + finding: Finding +} + 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[] } -/** 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: "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`, 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 isKnownFile = (finding: Finding): boolean => { - return knownPathSet.has(normalizeWorkspacePath(finding.file)) + 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 + 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, + } + } + + 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: findings.filter(isKnownFile), - droppedAsUnknownFile: findings.filter((finding) => !isKnownFile(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/prompt.ts b/src/review/prompt.ts index 87aac2a..4bc435e 100644 --- a/src/review/prompt.ts +++ b/src/review/prompt.ts @@ -29,8 +29,8 @@ export const conventionsCharacterCap = (conventionsBudgetTokens: number): number /** * Whether the conventions section will carry the file's complete text rather - * than a truncated head. The orchestrator uses it to send one full copy of the - * conventions file: + * than a truncated head. The orchestrator uses it to send no more than one + * full copy of the conventions file: * - true: the section is the full copy. readPriorityDocs and the related-file * scan get the path as an exclusion and skip the file, and a changed * conventions file's block is sent diff-only. @@ -142,6 +142,10 @@ move on — do not emit a finding for it.` * renderExcludedFilesNote (exclusion.ts) writes the "changed file(s) excluded * from review" trailer. A wording change there must update this text. * + * 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. + * * The File attribution paragraph covers three cases: * - A finding inside the annotated diff needs no quote line. The paragraph's * first "Boundary:" states this case. @@ -228,10 +232,18 @@ 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. */ +/** 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 + * 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 => { const fileTag = `file-${delimiterNonce}` 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} |`,