diff --git a/AGENTS.md b/AGENTS.md index 2399024..9cab67b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -27,9 +27,9 @@ src/ logger.ts # structured JSON logger — levels, child contexts, lazy props github/ # GitHub I/O: event payload → PrContext, octokit wrappers (diff fetch, review posting) openrouter/ # OpenRouter I/O: @openrouter/sdk wrapper, per-attempt and shared review deadlines, retry and fallback ladder (HTTP errors, timeouts, invalid structured output), cost summary - diff/ # pure transforms over parse-diff output + diff-level exclusion (patterns, gitattributes linguist rules, wildcard safety cap) + diff/ # pure transforms over parse-diff output: git-quoted path decoding, annotation, commentable lines, diff-level exclusion (patterns, gitattributes linguist rules, wildcard safety cap) context/ # workspace I/O: conventions file, root .gitattributes, changed files, import-trace scan, doc-mention scan, priority docs - review/ # pure review logic: finding schema, phases + stage dispatch, prompt, non-finding filter, unknown-file filter, cross-phase merge, path normalization, selection, comment mapping, title similarity, context notes, summary + review/ # pure review logic: finding schema, phases + stage dispatch, prompt, non-finding filter, unknown-file filter, cross-phase merge, path normalization, selection, comment mapping, markdown code spans, title similarity, context notes, summary orchestrate.ts # pipeline + createPromptedGenerateFindings — fully testable with stub clients ``` diff --git a/README.md b/README.md index 1804344..9d3e920 100644 --- a/README.md +++ b/README.md @@ -10,7 +10,7 @@ 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 outside the diff on files the model was given (e.g. callers of changed code) are posted as standalone comments on the PR +- Findings outside the diff on files the model was given (e.g. callers of changed code), findings in a changed file whose diff path can't take an inline comment, and every in-diff finding when GitHub rejects the inline review 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, the token budget breakdown, and how many findings each filter step dropped @@ -119,7 +119,7 @@ An empty value, such as an unset repo variable, selects the input's default, so 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 +9. Posts one review with inline comments (its body is only a hidden marker); beyond-diff findings, findings in a changed file whose diff path can't take an inline comment, and every in-diff finding when GitHub rejects the inline review 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 @@ -148,7 +148,7 @@ A finding's `file` (the path it is filed on) must name a file the model was give 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. +- 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 decoded path, so each `"` appears as written, and the filter matches that spelling as written. A path whose decoding was rejected keeps the diff's escaped spelling in its header. 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/src/__tests__/orchestrate.test.ts b/src/__tests__/orchestrate.test.ts index b02f40d..ad64cf5 100644 --- a/src/__tests__/orchestrate.test.ts +++ b/src/__tests__/orchestrate.test.ts @@ -22,6 +22,7 @@ import { computeAnchorKey, mapFindingsToReview, renderBeyondDiffFinding, + renderRejectedPathFinding, renderReroutedFinding, REVIEW_MARKER, STATUS_ANCHOR, @@ -782,6 +783,46 @@ describe("orchestrate", () => { expect(first(stubs.findRelatedDocsCalls).excludePaths).toEqual(["assets/logo.png"]) }) + it("normalizes a doubled-slash excluded diff path before matching priority docs and scan exclusions", async () => { + // Orchestrate compares excluded paths to priority docs as received, so + // this fails if partitioning ever stops normalizing them + const unnormalizedPathDiff = `diff --git a/assets//guide.md b/assets//guide.md +index 1111111..2222222 100644 +--- a/assets//guide.md ++++ b/assets//guide.md +@@ -1 +1 @@ +-old guide ++new guide +diff --git a/src/app.ts b/src/app.ts +index 3333333..4444444 100644 +--- a/src/app.ts ++++ b/src/app.ts +@@ -1 +1 @@ +-old line ++new line +` + const stubs = makeOrchestrateDeps({ + githubClient: { + fetchDiff: async () => ({ kind: "ok" as const, diff: unnormalizedPathDiff }), + }, + config: { + priorityDocs: ["assets/guide.md", "docs/guide.md"], + diffExcludePaths: { + defaultPatterns: [], + diffExcludePathPatterns: ["assets/**"], + }, + }, + }) + + await orchestrate(stubs.deps, createTestLogger()) + + expect(first(stubs.readPriorityDocsCalls).priorityDocs).toEqual(["docs/guide.md"]) + expect(first(stubs.findRelatedFilesCalls).excludePaths).toEqual([ + "assets/guide.md", + "AGENTS.md", + ]) + }) + it("passes the budget check when the oversized files are all excluded", async () => { // Budget 220 (half = 110) fails against the full fixture diff (341 // tokens); with every src/ file excluded only the binary asset header @@ -1128,6 +1169,196 @@ describe("orchestrate", () => { }) describe("context wiring", () => { + it("reads and comments on a git-quoted non-ASCII path under its decoded name", async () => { + const quotedPathDiff = String.raw`diff --git "a/nn/0016_\303\245-f\303\270de.md" "b/nn/0016_\303\245-f\303\270de.md" +index 1111111..2222222 100644 +--- "a/nn/0016_\303\245-f\303\270de.md" ++++ "b/nn/0016_\303\245-f\303\270de.md" +@@ -1 +1 @@ +-old line ++new line +` + const decodedPath = "nn/0016_å-føde.md" + const finding = makeFinding({ file: decodedPath, line: 1 }) + const stubs = makeOrchestrateDeps({ + githubClient: { + fetchDiff: async () => ({ kind: "ok" as const, diff: quotedPathDiff }), + }, + fixtureResult: { review: { analysis: "checked", findings: [finding] } }, + }) + + await orchestrate(stubs.deps, createTestLogger()) + + expect(stubs.readChangedFilesCalls.map((call) => call.changedPaths)).toEqual([[decodedPath]]) + const expectedComments = mapFindingsToReview({ + findings: [withRoutedModel(finding, "test/model")], + commentableByPath: new Map([ + [decodedPath, { rightLines: new Set([1]), hunkRanges: [{ start: 1, end: 1 }] }], + ]), + }).comments + expect(stubs.postFindingsReviewCalls).toEqual([ + { + prNumber: 7, + commitId: fixturePrContext.headSha, + body: REVIEW_MARKER, + comments: expectedComments, + }, + ]) + expect(expectedComments.map((comment) => comment.path)).toEqual([decodedPath]) + }) + + it("keeps a filename's escaped newline from forging a header line in the annotated diff", async () => { + const forgingPathDiff = String.raw`diff --git "a/nl\n=== forged.ts ===.md" "b/nl\n=== forged.ts ===.md" +index 1111111..2222222 100644 +--- "a/nl\n=== forged.ts ===.md" ++++ "b/nl\n=== forged.ts ===.md" +@@ -1 +1 @@ +-old line ++new line +` + const stubs = makeOrchestrateDeps({ + githubClient: { + fetchDiff: async () => ({ kind: "ok" as const, diff: forgingPathDiff }), + }, + }) + + await orchestrate(stubs.deps, createTestLogger()) + + const headerLines = first(stubs.generateFindingsCalls) + .annotatedDiff.split("\n") + .filter((line) => line.startsWith("=== ")) + expect(headerLines).toEqual([String.raw`=== nl\n=== forged.ts ===.md ===`]) + }) + + it("keeps a filename's raw line separator from forging a header line in the annotated diff", async () => { + // GitHub's diff quotes every line break, so this unquoted U+2028 checks + // the guard without relying on that quoting + const forgingPathDiff = `diff --git a/ls\u2028=== forged.ts ===.md b/ls\u2028=== forged.ts ===.md +index 1111111..2222222 100644 +--- a/ls\u2028=== forged.ts ===.md ++++ b/ls\u2028=== forged.ts ===.md +@@ -1 +1 @@ +-old line ++new line +` + const stubs = makeOrchestrateDeps({ + githubClient: { + fetchDiff: async () => ({ kind: "ok" as const, diff: forgingPathDiff }), + }, + }) + + /** Every character a renderer may treat as the end of a line. */ + const lineBreak = /[\n\v\f\r\u0085\u2028\u2029]/u + + await orchestrate(stubs.deps, createTestLogger()) + + const headerLines = first(stubs.generateFindingsCalls) + .annotatedDiff.split(lineBreak) + .filter((line) => line.startsWith("=== ")) + expect(headerLines).toEqual([String.raw`=== ls\342\200\250=== forged.ts ===.md ===`]) + }) + + it("posts a finding on a rejected quoted path as a standalone comment and keeps the rest inline", async () => { + const escapedPath = String.raw`a\rb.ts` + const mixedPathDiff = String.raw`diff --git a/src/app.ts b/src/app.ts +index 1111111..2222222 100644 +--- a/src/app.ts ++++ b/src/app.ts +@@ -1 +1 @@ +-old app line ++new app line +diff --git "a/a\rb.ts" "b/a\rb.ts" +index 3333333..4444444 100644 +--- "a/a\rb.ts" ++++ "b/a\rb.ts" +@@ -1 +1 @@ +-old carriage-return line ++new carriage-return line +` + const normalFinding = makeFinding({ file: "src/app.ts", line: 1, title: "App line bug" }) + const rejectedPathFinding = makeFinding({ + file: escapedPath, + line: 1, + title: "Carriage-return bug", + }) + const stubs = makeOrchestrateDeps({ + githubClient: { + fetchDiff: async () => ({ kind: "ok" as const, diff: mixedPathDiff }), + }, + fixtureResult: { + review: { analysis: "checked", findings: [normalFinding, rejectedPathFinding] }, + }, + }) + const logger = createTestLogger() + + await orchestrate(stubs.deps, logger) + + const expectedComments = mapFindingsToReview({ + findings: [withRoutedModel(normalFinding, "test/model")], + commentableByPath: new Map([ + ["src/app.ts", { rightLines: new Set([1]), hunkRanges: [{ start: 1, end: 1 }] }], + ]), + }).comments + expect(stubs.postFindingsReviewCalls).toEqual([ + { + prNumber: 7, + commitId: fixturePrContext.headSha, + body: REVIEW_MARKER, + comments: expectedComments, + }, + ]) + expect(expectedComments.map((comment) => comment.path)).toEqual(["src/app.ts"]) + expect(stubs.postIssueCommentCalls).toEqual([ + { + prNumber: 7, + body: renderRejectedPathFinding(withRoutedModel(rejectedPathFinding, "test/model")), + }, + ]) + expect( + logsWithMessage( + logger, + "file left out of inline comments because its diff path was rejected", + ), + ).toEqual([ + { + level: "debug", + message: "file left out of inline comments because its diff path was rejected", + data: { path: escapedPath }, + }, + ]) + }) + + it("gives the rejected-path note to a finding that spells the rejected path with a leading ./", async () => { + const rejectedPathDiff = String.raw`diff --git "a/a\rb.ts" "b/a\rb.ts" +index 3333333..4444444 100644 +--- "a/a\rb.ts" ++++ "b/a\rb.ts" +@@ -1 +1 @@ +-old carriage-return line ++new carriage-return line +` + const dotPrefixedFinding = makeFinding({ + file: String.raw`./a\rb.ts`, + line: 1, + title: "Carriage-return bug", + }) + const stubs = makeOrchestrateDeps({ + githubClient: { + fetchDiff: async () => ({ kind: "ok" as const, diff: rejectedPathDiff }), + }, + fixtureResult: { review: { analysis: "checked", findings: [dotPrefixedFinding] } }, + }) + + await orchestrate(stubs.deps, createTestLogger()) + + expect(stubs.postIssueCommentCalls).toEqual([ + { + prNumber: 7, + body: renderRejectedPathFinding(withRoutedModel(dotPrefixedFinding, "test/model")), + }, + ]) + }) + it("keeps a priority doc in the rendered prompt when changed files use the rest of the budget", async () => { const priorityDocContent = "# Review reference\nCheck API behavior." const priorityDocTokens = estimateTokens(priorityDocContent) @@ -2927,11 +3158,12 @@ 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 + // Git C-quotes a path holding a double quote, and the diff decoder turns + // it back into docs/a"b.md. The prompt escapes the quote in path + // attributes, so the model reports the file as 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 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: { @@ -2953,7 +3185,9 @@ describe("orchestrate", () => { const expectedMapped = mapFindingsToReview({ findings: findingsWithRoutedModel([{ ...escapedFinding, file: decodedPath }], "test/model"), - commentableByPath: computeCommentableLines(parseDiff(quotedPathDiff)), + commentableByPath: new Map([ + [decodedPath, { rightLines: new Set([2]), hunkRanges: [{ start: 1, end: 2 }] }], + ]), }) expect(expectedMapped.comments.map((comment) => comment.path)).toEqual([decodedPath]) expect(stubs.postFindingsReviewCalls).toEqual([ diff --git a/src/diff/__tests__/quoted-paths.test.ts b/src/diff/__tests__/quoted-paths.test.ts new file mode 100644 index 0000000..00805b1 --- /dev/null +++ b/src/diff/__tests__/quoted-paths.test.ts @@ -0,0 +1,320 @@ +import parseDiff, { type File } from "parse-diff" +import { describe, expect, it } from "vitest" +import { createTestLogger } from "../../__tests__/test-logger.js" +import { decodeQuotedFilePaths, decodeQuotedPath } from "../quoted-paths.js" + +const makeFile = (overrides: Partial = {}): File => ({ + chunks: [], + additions: 1, + deletions: 1, + from: "src/app.ts", + to: "src/app.ts", + ...overrides, +}) + +describe("decodeQuotedPath", () => { + it.each([ + { + label: "two-byte UTF-8 octal escapes", + path: String.raw`nn/0016_\303\245-f\303\270de.md`, + expected: "nn/0016_å-føde.md", + }, + { + label: "three-byte UTF-8 octal escapes", + path: String.raw`docs/\341\213\265.md`, + expected: "docs/ድ.md", + }, + { + label: "escaped double quotes", + path: String.raw`say \"hi\".md`, + expected: 'say "hi".md', + }, + { + label: "an escaped backslash", + path: String.raw`back\\slash.md`, + expected: "back\\slash.md", + }, + { + label: "raw non-ASCII text beside an escape", + path: String.raw`å\\b.md`, + expected: "å\\b.md", + }, + { + label: "an escaped backslash directly before octal escapes", + path: String.raw`x\\\303\245.md`, + expected: "x\\å.md", + }, + { + // The two backslashes form one escape, so the "303" after them is literal text + label: "an escaped backslash followed by octal-looking digits", + path: String.raw`x\\303.md`, + expected: "x\\303.md", + }, + { + // parse-diff drops the second backslash of a quoted path's final escaped backslash + label: "a lone trailing backslash as an escaped backslash", + path: "dir\\\\sub\\\\ends-with\\", + expected: "dir\\sub\\ends-with\\", + }, + { + label: "an escaped backslash before a lone trailing backslash", + path: "two\\\\\\", + expected: "two\\\\", + }, + ])("decodes $label", ({ path, expected }) => { + expect(decodeQuotedPath(path)).toEqual({ kind: "decoded", path: expected }) + }) + + it.each([ + { label: "a tab escape", path: String.raw`notes\told.md`, expected: "notes\told.md" }, + { label: "a bell escape", path: String.raw`bel\ax.md`, expected: "bel\x07x.md" }, + { label: "a backspace escape", path: String.raw`bs\bx.md`, expected: "bs\x08x.md" }, + { label: "an octal NUL escape", path: String.raw`nul\000x.md`, expected: "nul\x00x.md" }, + { label: "an octal ESC escape", path: String.raw`esc\033x.md`, expected: "esc\x1bx.md" }, + { label: "an octal DEL escape", path: String.raw`del\177x.md`, expected: "del\x7fx.md" }, + ])("decodes $label, a control character that breaks no line", ({ path, expected }) => { + expect(decodeQuotedPath(path)).toEqual({ kind: "decoded", path: expected }) + }) + + it.each([ + { label: "a plain path", path: "src/app.ts" }, + { label: "a path with spaces", path: "docs/plain space.md" }, + { label: "the /dev/null placeholder", path: "/dev/null" }, + ])("reports $label as unquoted", ({ path }) => { + expect(decodeQuotedPath(path)).toEqual({ kind: "unquoted" }) + }) + + it.each([ + { + label: "an unknown escape character", + path: String.raw`bad\x.md`, + reason: "unrecognized escape", + }, + { + label: "an octal escape above one byte", + path: String.raw`bad\400.md`, + reason: "unrecognized escape", + }, + { + label: "an octal escape with too few digits", + path: String.raw`bad\30.md`, + reason: "unrecognized escape", + }, + { + label: "escaped bytes that are not UTF-8", + path: String.raw`caf\351.md`, + reason: "escaped bytes are not valid UTF-8", + }, + ])("rejects $label as malformed", ({ path, reason }) => { + expect(decodeQuotedPath(path)).toEqual({ kind: "rejected", reason }) + }) + + it.each([ + { label: "a newline escape", path: String.raw`nl\n=== forged.ts ===.md`, codePoint: "000A" }, + { label: "a carriage-return escape", path: String.raw`cr\rx.md`, codePoint: "000D" }, + { label: "a vertical-tab escape", path: String.raw`vt\vx.md`, codePoint: "000B" }, + { label: "a form-feed escape", path: String.raw`ff\fx.md`, codePoint: "000C" }, + { label: "an escaped NEL (U+0085)", path: String.raw`nel\302\205x.md`, codePoint: "0085" }, + { + label: "an escaped line separator (U+2028)", + path: String.raw`ls\342\200\250x.md`, + codePoint: "2028", + }, + { + label: "an escaped paragraph separator (U+2029)", + path: String.raw`ps\342\200\251x.md`, + codePoint: "2029", + }, + ])("rejects $label that would decode to a line break", ({ path, codePoint }) => { + expect(decodeQuotedPath(path)).toEqual({ + kind: "rejected", + reason: `path contains line-break character U+${codePoint}`, + }) + }) + + it("rejects an unquoted path that holds a raw line separator (U+2028)", () => { + expect(decodeQuotedPath("ls\u2028x.md")).toEqual({ + kind: "rejected", + reason: "path contains line-break character U+2028", + }) + }) +}) + +describe("decodeQuotedFilePaths", () => { + it("decodes both paths of a quoted rename parsed by parse-diff", () => { + const renameDiff = String.raw`diff --git "a/docs/\341\213\265.md" "b/docs/\341\213\265-new.md" +similarity index 100% +rename from "docs/\341\213\265.md" +rename to "docs/\341\213\265-new.md"` + + const decoded = decodeQuotedFilePaths(parseDiff(renameDiff), createTestLogger()) + + expect(decoded).toEqual({ + files: [{ chunks: [], additions: 0, deletions: 0, from: "docs/ድ.md", to: "docs/ድ-new.md" }], + rejectedPaths: new Set(), + }) + }) + + it("decodes a path ending in a backslash from both diff header forms", () => { + const quotedPath = "dir\\\\sub\\\\" + const editedFileDiff = [ + `diff --git "a/${quotedPath}" "b/${quotedPath}"`, + "index 1111111..2222222 100644", + `--- "a/${quotedPath}"`, + `+++ "b/${quotedPath}"`, + "@@ -1 +1 @@", + "-old", + "+new", + ].join("\n") + const renamedFileDiff = [ + `diff --git "a/${quotedPath}" "b/${quotedPath}x"`, + "similarity index 100%", + ].join("\n") + + const decodedPaths = decodeQuotedFilePaths( + parseDiff(`${editedFileDiff}\n${renamedFileDiff}`), + createTestLogger(), + ).files.map(({ from, to }) => ({ from, to })) + + expect(decodedPaths).toEqual([ + { from: "dir\\sub\\", to: "dir\\sub\\" }, + { from: "dir\\sub\\", to: "dir\\sub\\x" }, + ]) + }) + + it("decodes an added file and keeps its /dev/null old path", () => { + const file = makeFile({ new: true, from: "/dev/null", to: String.raw`\303\245.md` }) + + expect(decodeQuotedFilePaths([file], createTestLogger())).toEqual({ + files: [{ ...file, from: "/dev/null", to: "å.md" }], + rejectedPaths: new Set(), + }) + }) + + it("decodes a deleted file and keeps its /dev/null new path", () => { + const file = makeFile({ deleted: true, from: String.raw`\303\245.md`, to: "/dev/null" }) + + expect(decodeQuotedFilePaths([file], createTestLogger())).toEqual({ + files: [{ ...file, from: "å.md", to: "/dev/null" }], + rejectedPaths: new Set(), + }) + }) + + it("leaves an absent old path absent", () => { + const file: File = { chunks: [], additions: 0, deletions: 0, to: String.raw`\303\245.png` } + + expect(decodeQuotedFilePaths([file], createTestLogger())).toStrictEqual({ + files: [{ chunks: [], additions: 0, deletions: 0, to: "å.png" }], + rejectedPaths: new Set(), + }) + }) + + it("leaves an absent new path absent", () => { + const file: File = { chunks: [], additions: 0, deletions: 0, from: String.raw`\303\245.png` } + + expect(decodeQuotedFilePaths([file], createTestLogger())).toStrictEqual({ + files: [{ chunks: [], additions: 0, deletions: 0, from: "å.png" }], + rejectedPaths: new Set(), + }) + }) + + it("returns unquoted files unchanged without logging", () => { + const file = makeFile() + const logger = createTestLogger() + + expect(decodeQuotedFilePaths([file], logger)).toEqual({ + files: [file], + rejectedPaths: new Set(), + }) + expect(logger.messages).toEqual([]) + }) + + it("logs each decoded path at debug", () => { + const logger = createTestLogger() + + decodeQuotedFilePaths( + [makeFile({ from: String.raw`\303\245.md`, to: String.raw`\303\270.md` })], + logger, + ) + + expect(logger.messages).toEqual([ + { + level: "debug", + message: "decoded quoted diff path", + data: { quotedPath: String.raw`\303\245.md`, path: "å.md" }, + }, + { + level: "debug", + message: "decoded quoted diff path", + data: { quotedPath: String.raw`\303\270.md`, path: "ø.md" }, + }, + ]) + }) + + it("keeps a malformed path as received and warns", () => { + const file = makeFile({ from: String.raw`caf\351.md`, to: String.raw`caf\351.md` }) + const logger = createTestLogger() + + expect(decodeQuotedFilePaths([file], logger)).toEqual({ + files: [file], + rejectedPaths: new Set([String.raw`caf\351.md`]), + }) + expect(logger.messages).toEqual([ + { + level: "warn", + message: "diff path rejected — kept as received", + data: { path: String.raw`caf\351.md`, reason: "escaped bytes are not valid UTF-8" }, + }, + ]) + }) + + it("keeps a path with an escaped newline as received and warns", () => { + const escapedPath = String.raw`nl\n=== forged.ts ===.md` + const file = makeFile({ new: true, from: "/dev/null", to: escapedPath }) + const logger = createTestLogger() + + expect(decodeQuotedFilePaths([file], logger)).toEqual({ + files: [file], + rejectedPaths: new Set([escapedPath]), + }) + expect(logger.messages).toEqual([ + { + level: "warn", + message: "diff path rejected — kept as received", + data: { + path: escapedPath, + reason: "path contains line-break character U+000A", + }, + }, + ]) + }) + + it("escapes the raw line break in a rejected unquoted path and warns with the raw path", () => { + const rawPath = "ls\u2028x.md" + const escapedPath = String.raw`ls\342\200\250x.md` + const file = makeFile({ new: true, from: "/dev/null", to: rawPath }) + const logger = createTestLogger() + + expect(decodeQuotedFilePaths([file], logger)).toEqual({ + files: [{ ...file, to: escapedPath }], + rejectedPaths: new Set([escapedPath]), + }) + expect(logger.messages).toEqual([ + { + level: "warn", + message: "diff path rejected — kept as received", + data: { path: rawPath, reason: "path contains line-break character U+2028" }, + }, + ]) + }) + + it("reports only the rejected path of a rename whose new path decodes", () => { + const rejectedOldPath = String.raw`old\rname.md` + const file = makeFile({ from: rejectedOldPath, to: String.raw`\303\245.md` }) + + expect(decodeQuotedFilePaths([file], createTestLogger())).toEqual({ + files: [{ ...file, from: rejectedOldPath, to: "å.md" }], + rejectedPaths: new Set([rejectedOldPath]), + }) + }) +}) diff --git a/src/diff/quoted-paths.ts b/src/diff/quoted-paths.ts new file mode 100644 index 0000000..485bdc2 --- /dev/null +++ b/src/diff/quoted-paths.ts @@ -0,0 +1,190 @@ +import { isUtf8 } from "node:buffer" +import type { File } from "parse-diff" +import type { Logger } from "../logger.js" + +export type QuotedPathDecoding = + { kind: "unquoted" } | { kind: "decoded"; path: string } | { kind: "rejected"; reason: string } + +/** + * One token of a path in git's C-style quoting, matching what git's + * unquote_c_style accepts. + * - literal: a run of characters with no backslash. + * - octal: three octal digits for one byte. Git writes the first digit as 0-3. + * - The unnamed last branch takes any other backslash and the character after + * it. A backslash at the end of the path matches alone. The s flag lets that + * character be a newline. CHARACTER_ESCAPE_BYTES decides whether a + * two-character match is a valid escape. + * + * Every character starts one of the three branches, so matchAll consumes the + * whole path and skips nothing. + */ +const QUOTED_PATH_TOKEN = /(?[^\\]+)|\\(?[0-3][0-7]{2})|\\.?/gs + +/** Git's single-character escapes and the byte each stands for. Each key is + * the escape as written in the path, backslash included. */ +const CHARACTER_ESCAPE_BYTES = new Map([ + ["\\a", 0x07], + ["\\b", 0x08], + ["\\t", 0x09], + ["\\n", 0x0a], + ["\\v", 0x0b], + ["\\f", 0x0c], + ["\\r", 0x0d], + ['\\"', 0x22], + ["\\\\", 0x5c], +]) + +/** A character that ends a line: LF, VT, FF, CR, NEL (U+0085), or a Unicode + * line or paragraph separator (U+2028, U+2029). */ +const LINE_BREAK_CHARACTERS = /[\n\v\f\r\u0085\u2028\u2029]/gu + +/** The bytes one token stands for, or null for an escape git never writes. */ +const getTokenBytes = (match: RegExpExecArray): Buffer | null => { + const literal = match.groups?.literal + const octal = match.groups?.octal + + if (literal) return Buffer.from(literal, "utf8") + if (octal) return Buffer.of(Number.parseInt(octal, 8)) + + // parse-diff's `---` and `+++` parsing strips a trailing `\"` as the closing + // quote, so a path ending in an escaped backslash arrives with only its first + // backslash. A lone backslash can only match at the end of the path, and it + // stands for that escaped backslash. + if (match[0] === "\\") return Buffer.of(0x5c) + + const escapeByte = CHARACTER_ESCAPE_BYTES.get(match[0]) + + // 0x00 is a valid byte, so only a missing key marks an unrecognized escape + if (escapeByte === undefined) return null + return Buffer.of(escapeByte) +} + +/** + * A rejection naming the path's first line break, or null when it has none. + * - The path is PR-author-controlled. It is rendered raw in markdown and in the + * "=== path ===" line annotateDiff writes above each file's hunks. A line + * break would let a filename forge such a line or split a markdown row. + * - Other control characters, such as tab, NUL, ESC, and DEL, break no line, + * so they decode. The logger's JSON escapes every C0 character, and DEL + * prints as an invisible byte. Markdown and the prompt keep each one inside + * its line. A workspace read the filesystem refuses falls back to the diff. + */ +const getLineBreakRejection = (path: string): QuotedPathDecoding | null => { + const lineBreak = path.match(LINE_BREAK_CHARACTERS)?.[0] + + if (!lineBreak) return null + + const codePointHex = lineBreak.charCodeAt(0).toString(16).toUpperCase().padStart(4, "0") + return { kind: "rejected", reason: `path contains line-break character U+${codePointHex}` } +} + +/** The path with each line break written as git's octal escapes of its UTF-8 + * bytes, so the path breaks no line. */ +export const escapeLineBreaks = (path: string): string => { + return path.replaceAll(LINE_BREAK_CHARACTERS, (lineBreak) => { + const octalEscapes = Array.from(Buffer.from(lineBreak, "utf8"), (byte) => { + return `\\${byte.toString(8).padStart(3, "0")}` + }) + return octalEscapes.join("") + }) +} + +/** Decodes a path parse-diff returned with git's quotes removed but its + * escapes kept. Escaped bytes decode as UTF-8. */ +export const decodeQuotedPath = (path: string): QuotedPathDecoding => { + // Git quotes any path containing a backslash, whatever core.quotePath says, + // and every character it quotes becomes an escape that starts with one. So + // a backslash is present exactly when the path was quoted. GitHub's diff + // quotes every line break too, but the check still runs on an unquoted path + // so the guard does not depend on that quoting. + if (!path.includes("\\")) return getLineBreakRejection(path) ?? { kind: "unquoted" } + + const byteChunks = Array.from(path.matchAll(QUOTED_PATH_TOKEN), getTokenBytes) + + if (!byteChunks.every((chunk) => chunk !== null)) { + return { kind: "rejected", reason: "unrecognized escape" } + } + + const bytes = Buffer.concat(byteChunks) + + if (!isUtf8(bytes)) return { kind: "rejected", reason: "escaped bytes are not valid UTF-8" } + + const decodedPath = bytes.toString("utf8") + + // The escaped form of a line break breaks no line, so a rejected path stays + // escaped + return getLineBreakRejection(decodedPath) ?? { kind: "decoded", path: decodedPath } +} + +export type DecodedDiffFiles = { + files: File[] + /** Paths whose decoding was rejected. Each stays as the diff spelled it, + * except that a raw line break becomes an octal escape. A quoted one keeps + * its escapes and names no file in the checkout or on GitHub. */ + rejectedPaths: ReadonlySet +} + +/** + * Decodes the quoted from and to paths of parsed diff files. GitHub's diff + * quotes every non-ASCII path and any path with a quote, a backslash, or a + * control character. parse-diff keeps the escapes, so without this step the + * workspace read, diff exclusion, and inline comments all see a path that + * does not exist. + */ +export const decodeQuotedFilePaths = ( + files: ReadonlyArray, + logger: Logger, +): DecodedDiffFiles => { + const decodePath = (rawPath: string): QuotedPathDecoding => { + const decoding = decodeQuotedPath(rawPath) + + // A rejected path stays as GitHub's diff shows it, with any raw line break + // escaped, because a guessed decoding would name a file that does not exist. + // - The checkout has no file at that path, so the file is reviewed from its + // diff alone. + // - GitHub rejects a whole review when one inline comment names such a + // path, so the caller keeps the file out of inline comments. + if (decoding.kind === "rejected") { + logger.warn("diff path rejected — kept as received", { + path: rawPath, + reason: decoding.reason, + }) + } + + if (decoding.kind === "decoded") { + logger.debug("decoded quoted diff path", { quotedPath: rawPath, path: decoding.path }) + } + + return decoding + } + + // A modified file lists one path as both from and to. Each distinct path + // decodes and logs once. + const rawPaths = Array.from(new Set(files.flatMap((file) => [file.from, file.to]))).filter( + (rawPath) => rawPath !== undefined, + ) + const decodingByRawPath = new Map( + rawPaths.map((rawPath): [string, QuotedPathDecoding] => [rawPath, decodePath(rawPath)]), + ) + + const getResolvedPath = (rawPath: string): string => { + const decoding = decodingByRawPath.get(rawPath) + + if (decoding?.kind === "decoded") return decoding.path + if (decoding?.kind === "rejected") return escapeLineBreaks(rawPath) + return rawPath + } + + const isRejectedPath = (rawPath: string): boolean => { + return decodingByRawPath.get(rawPath)?.kind === "rejected" + } + + return { + files: files.map((file) => ({ + ...file, + ...(file.from && { from: getResolvedPath(file.from) }), + ...(file.to && { to: getResolvedPath(file.to) }), + })), + rejectedPaths: new Set(rawPaths.filter(isRejectedPath).map(getResolvedPath)), + } +} diff --git a/src/orchestrate.ts b/src/orchestrate.ts index 29342db..e165fb4 100644 --- a/src/orchestrate.ts +++ b/src/orchestrate.ts @@ -10,6 +10,7 @@ import { renderExcludedFilesNote, summarizeExclusionSources, } from "./diff/exclusion.js" +import { decodeQuotedFilePaths } from "./diff/quoted-paths.js" import { describeError, type Logger } from "./logger.js" import type { CheckRunConclusion, CheckRunOutput, GithubClient } from "./github/client.js" import { resolvePullRequestEvent, type PrContext } from "./github/event.js" @@ -27,6 +28,7 @@ import { classifyDuplicate, mapFindingsToReview, renderBeyondDiffFinding, + renderRejectedPathFinding, renderReroutedFinding, REVIEW_MARKER, STATUS_ANCHOR, @@ -69,6 +71,7 @@ import { type RunPhase, } from "./review/run-stages.js" import { selectFindings } from "./review/select-findings.js" +import { normalizeWorkspacePath } from "./review/workspace-path.js" /** Priority docs use this share before full changed-file reads. */ const PRIORITY_DOCS_BUDGET_FLOOR_RATIO = 0.1 @@ -601,7 +604,9 @@ const runReviewPipeline = async ( return postSkipReview({ reason: "diff exceeds GitHub's diff API limits" }) } - const files = parseDiff(diffResult.diff) + // parse-diff keeps git's escapes in quoted paths. Decoding them once here + // gives every later step, from exclusion to inline comments, the real path. + const { files, rejectedPaths } = decodeQuotedFilePaths(parseDiff(diffResult.diff), logger) if (files.length === 0) { return postSkipReview({ reason: "empty diff" }) @@ -617,6 +622,10 @@ const runReviewPipeline = async ( { ...config.diffExcludePaths, gitAttributesContent }, logger, ) + + // Each excluded entry is a summary rather than a parse-diff file. Its path is + // the new path, or the old path for a deleted file, already posix-normalized + // with no leading "/". Its source names the rule that excluded it. const { kept: reviewableFiles, excluded: excludedDiffFiles } = partitionExcludedFiles({ files, matcher: exclusionMatcher, @@ -628,6 +637,7 @@ const runReviewPipeline = async ( excludedPaths: excludedDiffFiles.map((file) => `${file.path} (${file.source})`).join(", "), }) } + if (reviewableFiles.length === 0) { // Reason names the layers that actually excluded (an operator on default // inputs never set diff_exclude_paths); the body names every file. @@ -655,15 +665,30 @@ const runReviewPipeline = async ( }) } - const commentableByPath = computeCommentableLines(reviewableFiles) + // A rejected path keeps git's escapes, and any raw line break in it is + // escaped too, so it names no file GitHub knows. GitHub fails the whole + // review when one inline comment names such a path, so the file gets no + // commentable lines and its findings post as standalone comments. + const diffCommentableByPath = computeCommentableLines(reviewableFiles) + const commentableByPath = new Map( + Array.from(diffCommentableByPath).filter(([path]) => !rejectedPaths.has(path)), + ) + + for (const path of diffCommentableByPath.keys()) { + if (rejectedPaths.has(path)) { + logger.debug("file left out of inline comments because its diff path was rejected", { + path, + }) + } + } // Diff-excluded files must stay out of every context channel: the trailer // told the model their content is not shown, so neither the related-file // and doc scans nor the priority-doc read may pull that content back in. + // Excluded paths arrive normalized, so only the configured doc paths are + // normalized here. const diffExcludedPaths = excludedDiffFiles.map((file) => file.path) - const diffExcludedPathSet = new Set( - diffExcludedPaths.map((excludedPath) => posix.normalize(excludedPath)), - ) + const diffExcludedPathSet = new Set(diffExcludedPaths) const reviewablePriorityDocs = config.priorityDocs.filter( (docPath) => !diffExcludedPathSet.has(posix.normalize(docPath)), ) @@ -674,12 +699,14 @@ const runReviewPipeline = async ( const toPath = newFilePath(file) const fromPath = file.from - // A deleted file has no new path. Its old path joins the prompt's file - // paths through deletedPaths below - if (toPath === null) return [] + // A deleted file has no new path, because parse-diff sets `to` to + // "/dev/null" whenever it sets `deleted`. Its old path joins the prompt's + // file paths through deletedPaths below + if (!toPath) return [] - // parse-diff: from can be undefined for a binary file and is "/dev/null" - // for an added file (binary or not) — neither is a pre-rename path worth tracing + // parse-diff sets `from` to "/dev/null" for an added file, as it sets `to` + // for a deleted one. `from` is undefined only when parse-diff cannot read + // the file's diff header. Neither value is a pre-rename path worth tracing if (!fromPath || fromPath === "/dev/null" || fromPath === toPath) return [toPath] return [toPath, fromPath] @@ -832,10 +859,14 @@ const runReviewPipeline = async ( // diff has no hunks, so it carries nothing. const conventionsAddedInDiff = reviewableFiles.some((file) => { const toPath = newFilePath(file) + + // Every added file has a new path, so this check skips no added file. It + // narrows toPath's type for the comparison below. + if (!toPath) return false + return ( Boolean(file.new) && file.chunks.length > 0 && - toPath !== null && posix.normalize(toPath) === posix.normalize(config.conventionsFile) ) }) @@ -1143,11 +1174,22 @@ const runReviewPipeline = async ( logger, ) - // Beyond-diff findings, plus every in-diff finding when GitHub rejected the - // inline review, since one bad anchor fails the whole review. Each keeps a - // location note that matches where it sits. + // The unknown-file filter keeps a finding whose path matches a changed file + // only after normalizing, such as `./a\rb.ts`. The rejected-path check + // compares normalized paths too, so such a finding still gets its note. + const normalizedRejectedPaths = new Set(Array.from(rejectedPaths, normalizeWorkspacePath)) + + // Beyond-diff findings and findings on a rejected diff path, plus every + // in-diff finding when GitHub rejected the inline review, since one bad + // anchor fails the whole review. Each keeps a location note that matches + // where it sits. const issueCommentPosts = [ - ...unanchoredFindings.map((finding) => ({ finding, body: renderBeyondDiffFinding(finding) })), + ...unanchoredFindings.map((finding) => ({ + finding, + body: normalizedRejectedPaths.has(normalizeWorkspacePath(finding.file)) + ? renderRejectedPathFinding(finding) + : renderBeyondDiffFinding(finding), + })), ...inlineOutcome.rerouted.map((finding) => ({ finding, body: renderReroutedFinding(finding) })), ] diff --git a/src/review/__tests__/comment-mapping.test.ts b/src/review/__tests__/comment-mapping.test.ts index ab7786d..8a10af8 100644 --- a/src/review/__tests__/comment-mapping.test.ts +++ b/src/review/__tests__/comment-mapping.test.ts @@ -9,6 +9,7 @@ import { isDuplicateFinding, mapFindingsToReview, renderBeyondDiffFinding, + renderRejectedPathFinding, renderReroutedFinding, STATUS_ANCHOR, type AnchorSource, @@ -435,6 +436,48 @@ The guard rejects only the exact empty string. }) }) +describe("renderRejectedPathFinding", () => { + it("places the finding in a changed file whose path cannot take an inline comment", () => { + const finding = makeFinding({ file: "src/rejected.ts", line: 1 }) + + const body = renderRejectedPathFinding(finding) + + expect(body).toBe(`**Whitespace-only keys pass the empty-key guard** +Medium severity · correctness · high confidence + +\`src/rejected.ts:1\` — in a changed file whose path cannot take an inline comment. + +The guard rejects only the exact empty string. + +**Failure scenario:** register(" ", "value") succeeds and the entry is orphaned. + +--- +*umm-actually · test/model* + +`) + }) + + it("keeps the location span whole when the path contains a backtick", () => { + const finding = makeFinding({ file: "src/a`b.ts", line: 1 }) + + const body = renderRejectedPathFinding(finding) + + expect(body).toBe(`**Whitespace-only keys pass the empty-key guard** +Medium severity · correctness · high confidence + +\`\`src/a\`b.ts:1\`\` — in a changed file whose path cannot take an inline comment. + +The guard rejects only the exact empty string. + +**Failure scenario:** register(" ", "value") succeeds and the entry is orphaned. + +--- +*umm-actually · test/model* + +`) + }) +}) + describe("renderReroutedFinding", () => { it("places the finding at or near a changed line and says why it posted as its own comment", () => { const finding = makeFinding({ file: "src/greeter.ts", line: 145 }) diff --git a/src/review/__tests__/context-notes.test.ts b/src/review/__tests__/context-notes.test.ts index 2525678..9d18b68 100644 --- a/src/review/__tests__/context-notes.test.ts +++ b/src/review/__tests__/context-notes.test.ts @@ -184,6 +184,20 @@ describe("buildContextNotes", () => { ]) }) + it("keeps a diff-excluded path's code span whole when the path contains a backtick", () => { + const notes = buildContextNotes( + makeInput({ + diffExcludedFiles: [ + { path: "gen/a`b.json", additions: 1, deletions: 0, source: "diff_exclude_paths" }, + ], + }), + ) + + expect(notes).toEqual([ + "1 changed file(s) excluded from review: ``gen/a`b.json`` (diff_exclude_paths input)", + ]) + }) + it("orders in-context before not-included before related files before related docs before diff exclusions", () => { const notes = buildContextNotes( makeInput({ diff --git a/src/review/__tests__/markdown.test.ts b/src/review/__tests__/markdown.test.ts new file mode 100644 index 0000000..adb4095 --- /dev/null +++ b/src/review/__tests__/markdown.test.ts @@ -0,0 +1,24 @@ +import { describe, expect, it } from "vitest" +import { renderCodeSpan } from "../markdown.js" + +describe("renderCodeSpan", () => { + it.each([ + { label: "a plain path", text: "src/app.ts:12", expected: "`src/app.ts:12`" }, + { label: "a path with one backtick", text: "src/a`b.ts", expected: "``src/a`b.ts``" }, + { + label: "a path whose longest backtick run is two", + text: "a`b``c.ts", + expected: "```a`b``c.ts```", + }, + { label: "a path that starts with a backtick", text: "`a.ts", expected: "`` `a.ts ``" }, + { label: "a path that ends with a backtick", text: "a.ts`", expected: "`` a.ts` ``" }, + { label: "a path that starts with a space", text: " a.ts", expected: "` a.ts `" }, + { label: "a path that ends with a space", text: "a.ts ", expected: "` a.ts `" }, + ])("wraps $label so no backtick inside closes the span", ({ text, expected }) => { + expect(renderCodeSpan(text)).toBe(expected) + }) + + it("writes a line break as its octal escape so a blank line cannot end the span", () => { + expect(renderCodeSpan("docs/a\n\n# forged.md")).toBe("`docs/a\\012\\012# forged.md`") + }) +}) diff --git a/src/review/__tests__/review-summary.test.ts b/src/review/__tests__/review-summary.test.ts index 13646fa..363e6ae 100644 --- a/src/review/__tests__/review-summary.test.ts +++ b/src/review/__tests__/review-summary.test.ts @@ -362,6 +362,15 @@ describe("renderReviewSummary", () => { expect(summary).not.toContain(baseStats.prContext.headSha) }) + it("keeps a branch name's code span whole when it contains a backtick", () => { + const summary = renderReviewSummary({ + ...baseStats, + prContext: { ...baseStats.prContext, headRef: "chore/fix`doc" }, + }) + + expect(summary.split("\n")[2]).toBe("PR #7 · ``chore/fix`doc`` → `main` · `abc123d`") + }) + it("escapes pipe characters in paths so they cannot break the markdown table", () => { const summary = renderReviewSummary({ ...baseStats, @@ -371,4 +380,35 @@ describe("renderReviewSummary", () => { expect(summary.split("\n")[12]).toBe("| Changed files | 1 | src/a\\|b.ts |") expect(summary).not.toContain("| src/a|b.ts |") }) + + it("escapes a backslash before a pipe so the pipe stays escaped", () => { + const summary = renderReviewSummary({ + ...baseStats, + changedFilePaths: [String.raw`src/a\|b.ts`], + }) + + expect(summary.split("\n")[12]).toBe(String.raw`| Changed files | 1 | src/a\\\|b.ts |`) + }) + + it("escapes backticks in paths so two in one cell cannot open a code span", () => { + const summary = renderReviewSummary({ + ...baseStats, + changedFilePaths: ["src/a`b.ts", "src/c`d.ts"], + }) + + expect(summary.split("\n")[12]).toBe( + String.raw`| Changed files | 2 | src/a\`b.ts, src/c\`d.ts |`, + ) + }) + + it("escapes a line break in a path so it cannot split the table row", () => { + const summary = renderReviewSummary({ + ...baseStats, + changedFilePaths: ["src/x\n| injected |.ts"], + }) + + expect(summary.split("\n")[12]).toBe( + String.raw`| Changed files | 1 | src/x\\012\| injected \|.ts |`, + ) + }) }) diff --git a/src/review/comment-mapping.ts b/src/review/comment-mapping.ts index 3af9c53..7203b30 100644 --- a/src/review/comment-mapping.ts +++ b/src/review/comment-mapping.ts @@ -1,5 +1,6 @@ import type { CommentableFile } from "../diff/commentable-lines.js" import type { AttributedFinding, Finding } from "./finding.js" +import { renderCodeSpan } from "./markdown.js" import { normalizeTitle, titleSimilarity } from "./title-similarity.js" /** Wire shape for POST /pulls/{n}/reviews comments[] entries. */ @@ -395,7 +396,7 @@ const renderIssueCommentFinding = ({ }): string => { return `${findingHeader(finding)} -\`${finding.file}:${finding.line}\` — ${locationNote} +${renderCodeSpan(`${finding.file}:${finding.line}`)} — ${locationNote} ${finding.description} @@ -413,6 +414,15 @@ export const renderBeyondDiffFinding = (finding: AttributedFinding): string => { }) } +/** Renders a finding in a changed file whose diff path was rejected. That path + * names no file GitHub knows, so the finding cannot post inline. */ +export const renderRejectedPathFinding = (finding: AttributedFinding): string => { + return renderIssueCommentFinding({ + finding, + locationNote: "in a changed file whose path cannot take an inline comment.", + }) +} + /** Renders an in-diff finding posted after GitHub rejected the inline review. * Its line is at or near a changed line, not beyond the diff. */ export const renderReroutedFinding = (finding: AttributedFinding): string => { @@ -504,7 +514,7 @@ export const buildStatusComment = ({ const capNote = droppedByCap.length === 0 ? "" - : `_${droppedByCap.length} lower-severity finding(s) omitted by the max_findings cap: ${droppedByCap.map((finding) => `\`${finding.file}:${finding.line}\``).join(", ")}_` + : `_${droppedByCap.length} lower-severity finding(s) omitted by the max_findings cap: ${droppedByCap.map((finding) => renderCodeSpan(`${finding.file}:${finding.line}`)).join(", ")}_` const incompleteNote = [ buildIncompleteNote(incompletePhases), ...(reviewDeadlineExceeded diff --git a/src/review/context-notes.ts b/src/review/context-notes.ts index f7c426e..d3aff74 100644 --- a/src/review/context-notes.ts +++ b/src/review/context-notes.ts @@ -1,5 +1,6 @@ import { posix } from "node:path" import { describeExclusionSource, type ExcludedDiffFile } from "../diff/exclusion.js" +import { renderCodeSpan } from "./markdown.js" import { conventionsCharacterCap, conventionsRenderInFull, type PromptFile } from "./prompt.js" export type ContextNotesInput = { @@ -22,11 +23,11 @@ export type ContextNotesInput = { const normalizePath = (filePath: string): string => posix.normalize(filePath) const renderCodePaths = (paths: string[]): string => { - return paths.map((filePath) => `\`${filePath}\``).join(", ") + return paths.map(renderCodeSpan).join(", ") } const renderExcludedFile = (file: ExcludedDiffFile): string => { - return `\`${file.path}\` (${describeExclusionSource(file.source)})` + return `${renderCodeSpan(file.path)} (${describeExclusionSource(file.source)})` } /** Priority docs satisfied by a higher-priority channel (changed files, @@ -181,7 +182,7 @@ export const buildConventionsNote = ({ if (conventionsCoverage.status !== "truncated") return null const { fullCopyChannel, characterCap, totalCharacters } = conventionsCoverage - const fileLabel = `\`${conventionsFile}\`` + const fileLabel = renderCodeSpan(conventionsFile) // priority_docs retries a listed file on every PR, and a later PR that cannot // fit it gets its own no-copy note, so a listed file needs no warn-ahead diff --git a/src/review/markdown.ts b/src/review/markdown.ts new file mode 100644 index 0000000..e72c3cd --- /dev/null +++ b/src/review/markdown.ts @@ -0,0 +1,25 @@ +import { escapeLineBreaks } from "../diff/quoted-paths.js" + +/** Each run of consecutive backticks. */ +const BACKTICK_RUN = /`+/g + +/** A backtick or a space at the start or the end of the text. */ +const EDGE_BACKTICK_OR_SPACE = /^[` ]|[` ]$/ + +/** + * Wraps text in an inline code span that nothing inside the text can close. + * File paths reach comments as written. Git never quotes a backtick, and a + * workspace-scan path never passes the diff decoder's line-break check. + * - Line breaks become octal escapes, since a blank line ends the span. + * - The delimiter is one backtick longer than the longest backtick run in the + * text. + * - Text that starts or ends with a backtick or a space gets one space on each + * side. CommonMark strips that pair, so the rendered text is unchanged. + */ +export const renderCodeSpan = (text: string): string => { + const singleLineText = escapeLineBreaks(text) + const backtickRunLengths = (singleLineText.match(BACKTICK_RUN) ?? []).map((run) => run.length) + const delimiter = "`".repeat(Math.max(0, ...backtickRunLengths) + 1) + const padding = EDGE_BACKTICK_OR_SPACE.test(singleLineText) ? " " : "" + return `${delimiter}${padding}${singleLineText}${padding}${delimiter}` +} diff --git a/src/review/review-summary.ts b/src/review/review-summary.ts index e5ccc9d..cee8ffa 100644 --- a/src/review/review-summary.ts +++ b/src/review/review-summary.ts @@ -1,5 +1,7 @@ +import { escapeLineBreaks } from "../diff/quoted-paths.js" import type { PrContext } from "../github/event.js" import type { ConventionsCoverage, ConventionsFullCopyChannel } from "./context-notes.js" +import { renderCodeSpan } from "./markdown.js" export type ReviewSummaryStats = { prContext: PrContext @@ -23,6 +25,8 @@ export type ReviewSummaryStats = { docsExcludedPaths: string[] tokenBudgetTotal: number tokenBudgetUsedByDiff: number + /** Tokens reserved for priority docs: what the early priority-doc read spent + * plus what was held back from related files. */ tokenBudgetPriorityDocFloor: number tokenBudgetRemainingForDocs: number totalFromModel: number @@ -33,6 +37,7 @@ export type ReviewSummaryStats = { droppedAsExcludedFile: number /** Findings two phases reported on overlapping lines of one file. */ duplicatesAcrossPhases: number + /** Findings dropped because an earlier run already posted them. */ duplicatesRemoved: number droppedBelowThreshold: number droppedAsOverlapping: number @@ -41,11 +46,27 @@ export type ReviewSummaryStats = { } /** Comma-joined items for one markdown line or table cell — em-dash when - * empty so cells are never blank. Pipes are escaped so an item can't break - * a table row. */ + * empty so cells are never blank. Line breaks, backslashes, backticks, and + * pipes are escaped, so no item can split the row or format its text. */ const renderCommaList = (items: string[]): string => { if (items.length === 0) return "—" - return items.map((item) => item.replaceAll("|", "\\|")).join(", ") + + // 1. Line breaks become octal escapes first. Workspace-scan paths never pass + // the diff decoder's line-break check, so a raw one would split the row. + // 2. Backslashes are escaped next, including the ones step 1 wrote, so each + // renders as written. + // 3. Backticks and pipes are escaped last. Git never quotes a backtick, and + // two in one cell would open a code span. Escaping either before + // backslashes would double its escape's own backslash and leave it bare. + // For example, `a\|b` renders as `a\\\|b`. + return items + .map((item) => { + return escapeLineBreaks(item) + .replaceAll("\\", "\\\\") + .replaceAll("`", "\\`") + .replaceAll("|", "\\|") + }) + .join(", ") } /** The conventions file and how much of it reached the model. */ @@ -54,12 +75,14 @@ const renderConventionsCoverage = ({ conventionsCoverage, }: Pick): string => { if (conventionsCoverage.status === "not-found") return "none" + // The size against the cap shows how close a fitting file is to truncating if (conventionsCoverage.status === "full") { const { characterCap, totalCharacters } = conventionsCoverage return `${conventionsFile} (${totalCharacters} characters, within the ${characterCap}-character cap)` } + // Only the truncated status remains const { fullCopyChannel, characterCap, totalCharacters } = conventionsCoverage // A priority-doc copy replaces the truncated section, so no head was sent @@ -94,7 +117,7 @@ export const renderReviewSummary = (stats: ReviewSummaryStats): string => { return [ "### umm-actually review summary", "", - `PR #${stats.prContext.prNumber} · \`${stats.prContext.headRef}\` → \`${stats.prContext.baseRef}\` · \`${sha}\``, + `PR #${stats.prContext.prNumber} · ${renderCodeSpan(stats.prContext.headRef)} → ${renderCodeSpan(stats.prContext.baseRef)} · \`${sha}\``, "", `**Conventions:** ${renderConventionsCoverage(stats)}`, "",