fix(diff): decode git-quoted paths from the PR diff

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
 ```
 

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 · <model>` 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 `&quot;` 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 `&quot;` into `file`, so a `file` that matches no given path as written, but matches once each `&quot;` 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 `&quot;` 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 `&quot;` into `file`, so a `file` that matches no given path as written, but matches once each `&quot;` 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

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([