diff --git a/src/review/fix-handoff-render.ts b/src/review/fix-handoff-render.ts index 4dd86ecbd2..0c40d94a11 100644 --- a/src/review/fix-handoff-render.ts +++ b/src/review/fix-handoff-render.ts @@ -37,15 +37,20 @@ 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 comments still render markdown inside + * collapsibles, so neutralize delimiters that can break out of table-like contexts -- but a code span's own + * backtick delimiter can't be neutralized by backslash-escaping (Markdown does not honor that inside code + * spans), so choose a delimiter longer than any backtick run inside the value instead, matching + * unified-comment-bridge.ts's markdownPathCode. Returns the full delimiter-wrapped span; callers must not + * re-wrap the result in a hardcoded backtick. */ function markdownPathCodeText(value: string): string { - return value + const safeValue = value .replace(/\\/g, "\\\\") - .replace(/`/g, "\\`") .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 +69,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 +117,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..77728240d7 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,9 @@ 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 path contains a literal backtick, so the delimiter widens to 2 backticks (#9289) instead of the old, + // Markdown-incorrect backslash-escape of the embedded backtick. + expect(block?.body).toContain("`` src/x` [Review required](https://evil.example/phish) \\| <tag> ``:7"); 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..1e5dc0aa28 100644 --- a/test/unit/fix-handoff-render.test.ts +++ b/test/unit/fix-handoff-render.test.ts @@ -11,8 +11,26 @@ 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**"); + }); + + // #9289: a literal backtick in the path used to be backslash-escaped, which Markdown does not honor inside + // a code span -- the escaped backtick still closed the span early, corrupting the rendered block. The path + // must instead be wrapped in a delimiter longer than any backtick run it contains (matching + // unified-comment-bridge.ts's markdownPathCode), so a single embedded backtick renders as an unbroken span. + it("renders an unbroken code span for a path containing a literal backtick (#9289)", () => { + const block = buildFixHandoffBlock(finding({ path: "src/a`b.ts", line: 12 })); + expect(block.body).toContain("**Fix handoff — Blocker at `` src/a`b.ts ``:12**"); + // The old backslash-escape approach would have left a stray "\`" in the body -- confirm it's gone. + expect(block.body).not.toContain("\\`"); + }); + + // Preserved entity-escaping behavior (#9289's own requirement: only the backtick strategy changes) -- + // both angle-bracket arms of the escape and the pipe-escape must still fire. + it("still entity-escapes < and > and backslash-escapes | in the path", () => { + const block = buildFixHandoffBlock(finding({ path: "src/|b.ts", line: 12 })); + expect(block.body).toContain("` src/<a>\\|b.ts `:12"); }); it("renders a nit label", () => { @@ -50,7 +68,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 +119,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 +129,13 @@ 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("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 });