fix(review): widen fix-handoff code-span delimiter for backtick paths (#9289)

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 = "<!-- loopover:fix-handoff -->";
 
-/** 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 === "<" ? "&lt;" : "&gt;"));
+  const safeValue = value.replace(/\|/g, "\\|").replace(/[<>]/g, (char) => (char === "<" ? "&lt;" : "&gt;"));
+  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

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("<!-- loopover:fix-handoff -->");
-    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) | <tag>");
-    expect(block?.body).toContain("`src/x\\` [Review required](https://evil.example/phish) \\| &lt;tag&gt;: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) \\| &lt;tag&gt; ``: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) | <tag>:7`");
   });
 

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: "a<b>c|d.ts", line: 5 }));
+    expect(block.body).toContain("**Fix handoff — Blocker at ` a&lt;b&gt;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");
   });
 

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
   });