diff --git a/src/review/fix-handoff-render.ts b/src/review/fix-handoff-render.ts index 4dd86ecbd2..e7bcb7cc2b 100644 --- a/src/review/fix-handoff-render.ts +++ b/src/review/fix-handoff-render.ts @@ -37,15 +37,17 @@ export type FixHandoffBlock = { * parse fix-handoff blocks in a comment body without depending on markdown structure alone. */ const FIX_HANDOFF_MARKER = ""; -/** Public-safe inline-code escaping for a finding path/location. GitHub comments still render markdown inside - * collapsibles, so neutralize delimiters that can break out of the `...` span or table-like contexts before - * composing the location label. */ +/** Public-safe inline-code rendering for a finding path/location. GitHub renders markdown inside review + * comments, so choose a code-span delimiter longer than any backtick run inside the value instead of + * backslash-escaping backticks (Markdown does not honor that inside code spans — the same reason + * `markdownPathCode` in unified-comment-bridge.ts uses this approach). Entity-escape the other + * span/table-breaking metacharacters, then wrap the value in that delimiter (with the surrounding spaces a + * code span strips, so a value that itself starts/ends with a backtick still renders as one whole span). */ function markdownPathCodeText(value: string): string { - return value - .replace(/\\/g, "\\\\") - .replace(/`/g, "\\`") - .replace(/\|/g, "\\|") - .replace(/[<>]/g, (char) => (char === "<" ? "<" : ">")); + const safeValue = value.replace(/\|/g, "\\|").replace(/[<>]/g, (char) => (char === "<" ? "<" : ">")); + const longestBacktickRun = Math.max(0, ...Array.from(safeValue.matchAll(/`+/g), (match) => match[0].length)); + const delimiter = "`".repeat(longestBacktickRun + 1); + return `${delimiter} ${safeValue} ${delimiter}`; } /** PURE: build a single finding's fix-handoff block. Never throws; a finding whose `line` is not a positive @@ -64,7 +66,7 @@ export function buildFixHandoffBlock(finding: InlineFinding): FixHandoffBlock { suggestedChange && !suggestedChange.includes("```") ? `\n\nSuggested change:\n\`\`\`\n${suggestedChange}\n\`\`\`` : ""; const body = [ FIX_HANDOFF_MARKER, - `**Fix handoff — ${label} at \`${location}\`**`, + `**Fix handoff — ${label} at ${location}**`, finding.body, suggestionBlock, `\n_${LOCAL_WRITE_BOUNDARY}_`, @@ -112,7 +114,7 @@ function fixHandoffAggregateItem(finding: InlineFinding, index: number): string // Same fence-safety guard as buildFixHandoffBlock / safeSuggestionBlock: an embedded ``` would break the block. const suggestionBlock = suggestion && !suggestion.includes("```") ? `\n \`\`\`\n ${suggestion.replace(/\n/g, "\n ")}\n \`\`\`` : ""; - return `${index + 1}. **${label} at \`${location}\`** — ${finding.body}${suggestionBlock}`; + return `${index + 1}. **${label} at ${location}** — ${finding.body}${suggestionBlock}`; } /** PURE: combine every current finding into ONE fix-handoff block for a single local-agent run across the diff --git a/test/unit/fix-handoff-collapsible.test.ts b/test/unit/fix-handoff-collapsible.test.ts index 3e372e6c4e..a5333b6e8b 100644 --- a/test/unit/fix-handoff-collapsible.test.ts +++ b/test/unit/fix-handoff-collapsible.test.ts @@ -37,11 +37,11 @@ describe("buildFixHandoffCollapsible (#1962)", () => { expect(c).not.toBeNull(); expect(c?.title).toBe("Fix handoff"); expect(c?.body).toContain(""); - expect(c?.body).toContain("`src/a.ts:10`"); + expect(c?.body).toContain("` src/a.ts `:10"); expect(c?.body).toContain("Possible null dereference on the fetched record."); expect(c?.body).toContain("Suggested change:"); // the path-only (no commentable line) block still identifies WHERE to look - expect(c?.body).toContain("`src/b.ts (no specific line)`"); + expect(c?.body).toContain("` src/b.ts ` (no specific line)"); }); it("escapes adversarial paths before rendering inline-code locations", () => { @@ -55,7 +55,10 @@ describe("buildFixHandoffCollapsible (#1962)", () => { ]); expect(block?.path).toBe("src/x` [Review required](https://evil.example/phish) | "); - expect(block?.body).toContain("`src/x\\` [Review required](https://evil.example/phish) \\| <tag>:7`"); + // The backtick in the path is contained by a widened (double-backtick) delimiter rather than a broken + // backslash-escape; | and <> stay entity/pipe-escaped as before. + expect(block?.body).toContain("`` src/x` [Review required](https://evil.example/phish) \\| <tag> ``:7"); + expect(block?.body).not.toContain("\\`"); // never backslash-escapes the backtick (markdown ignores that in a code span) expect(block?.body).not.toContain("`src/x` [Review required](https://evil.example/phish) | :7`"); }); diff --git a/test/unit/fix-handoff-render.test.ts b/test/unit/fix-handoff-render.test.ts index 647c067b3d..4734a052c6 100644 --- a/test/unit/fix-handoff-render.test.ts +++ b/test/unit/fix-handoff-render.test.ts @@ -11,8 +11,23 @@ describe("buildFixHandoffBlock (#2175)", () => { it("renders a path:line location and blocker label", () => { const block = buildFixHandoffBlock(finding({ line: 12, severity: "blocker" })); expect(block).toMatchObject({ path: "src/a.ts", line: 12, severity: "blocker", instruction: "Null check missing before dereference." }); - expect(block.body).toContain("src/a.ts:12"); - expect(block.body).toContain("**Fix handoff — Blocker at `src/a.ts:12`**"); + expect(block.body).toContain("` src/a.ts `:12"); + expect(block.body).toContain("**Fix handoff — Blocker at ` src/a.ts `:12**"); + }); + + it("wraps a backtick-containing path in a longer code-span delimiter, not a broken single-backtick span (#9289)", () => { + // A literal backtick in the path would prematurely close a single-backtick span; markdown does not honor a + // backslash-escaped backtick inside a code span, so the fix widens the delimiter instead (matching + // markdownPathCode in unified-comment-bridge.ts) — the path renders as one unbroken span. + const block = buildFixHandoffBlock(finding({ path: "src/a`b.ts", line: 7 })); + expect(block.body).toContain("**Fix handoff — Blocker at `` src/a`b.ts ``:7**"); + expect(block.body).not.toContain("\\`"); // never falls back to the broken backslash-escape + }); + + it("preserves the existing entity/pipe escaping for <, >, and | in the path (#9289)", () => { + // Only the backtick strategy changed; <, >, | are still neutralized exactly as before. + const block = buildFixHandoffBlock(finding({ path: "ac|d.ts", line: 5 })); + expect(block.body).toContain("**Fix handoff — Blocker at ` a<b>c\\|d.ts `:5**"); }); it("renders a nit label", () => { @@ -50,7 +65,7 @@ describe("buildFixHandoffBlock (#2175)", () => { it("yields a path-only block (line 0) when the finding has no commentable line (line <= 0)", () => { const block = buildFixHandoffBlock(finding({ line: 0 })); expect(block.line).toBe(0); - expect(block.body).toContain("src/a.ts (no specific line)"); + expect(block.body).toContain("` src/a.ts ` (no specific line)"); expect(block.body).not.toContain("src/a.ts:0"); }); @@ -101,7 +116,7 @@ describe("buildFixHandoffAggregateBlock (#5102)", () => { const block = buildFixHandoffAggregateBlock([finding()]); expect(block?.findingCount).toBe(1); expect(block?.body).toContain("**Fix handoff — 1 finding across this PR**"); - expect(block?.body).toContain("1. **Blocker at `src/a.ts:12`** — Null check missing before dereference."); + expect(block?.body).toContain("1. **Blocker at ` src/a.ts `:12** — Null check missing before dereference."); }); it("combines multiple findings into one numbered block with plural wording", () => { @@ -111,13 +126,19 @@ describe("buildFixHandoffAggregateBlock (#5102)", () => { ]); expect(block?.findingCount).toBe(2); expect(block?.body).toContain("**Fix handoff — 2 findings across this PR**"); - expect(block?.body).toContain("1. **Blocker at `a.ts:1`**"); - expect(block?.body).toContain("2. **Nit at `b.ts:2`**"); + expect(block?.body).toContain("1. **Blocker at ` a.ts `:1**"); + expect(block?.body).toContain("2. **Nit at ` b.ts `:2**"); + }); + + it("wraps a backtick-containing path in a longer delimiter in aggregate items too (#9289)", () => { + const block = buildFixHandoffAggregateBlock([finding({ path: "pkg/x`y.ts", line: 3, severity: "nit" })]); + expect(block?.body).toContain("1. **Nit at `` pkg/x`y.ts ``:3**"); + expect(block?.body).not.toContain("\\`"); }); it("renders a path-only location when a finding has no commentable line", () => { const block = buildFixHandoffAggregateBlock([finding({ line: 0 })]); - expect(block?.body).toContain("src/a.ts (no specific line)"); + expect(block?.body).toContain("` src/a.ts ` (no specific line)"); expect(block?.body).not.toContain("src/a.ts:0"); }); diff --git a/test/unit/queue-4.test.ts b/test/unit/queue-4.test.ts index 710c0a29a9..233bb10f3f 100644 --- a/test/unit/queue-4.test.ts +++ b/test/unit/queue-4.test.ts @@ -6104,7 +6104,7 @@ describe("queue processors", () => { }); expect(unifiedCommentBody).toContain("Fix handoff"); // the collapsible section is emitted - expect(unifiedCommentBody).toContain("Fix handoff — Blocker at `src/db.ts:2`"); // the per-finding block header + location anchor + expect(unifiedCommentBody).toContain("Fix handoff — Blocker at ` src/db.ts `:2"); // the per-finding block header + location anchor expect(unifiedCommentBody).toContain("This query is vulnerable to SQL injection."); // the finding, handed off verbatim expect(unifiedCommentBody).toContain("Suggested change:"); // its suggestion carried through });