From e069f59ac027529edbbb96f8e55dd63c63d77ce9 Mon Sep 17 00:00:00 2001 From: Andriy Polanski Date: Mon, 27 Jul 2026 16:22:47 +0000 Subject: [PATCH] fix(review): widen fix-handoff code-span delimiter for backtick paths (#9289) markdownPathCodeText backslash-escaped backticks before callers wrapped the location in a single-backtick span, but CommonMark/GFM code spans do not honor backslash escapes. Mirror markdownPathCode: longest backtick run + 1 delimiter (with padding spaces), and consume that span directly at both call sites. Co-authored-by: Cursor --- src/review/fix-handoff-render.ts | 28 ++++++++++++--------- test/unit/fix-handoff-collapsible.test.ts | 9 ++++--- test/unit/fix-handoff-render.test.ts | 30 ++++++++++++++++++++--- test/unit/queue-4.test.ts | 2 +- 4 files changed, 50 insertions(+), 19 deletions(-) diff --git a/src/review/fix-handoff-render.ts b/src/review/fix-handoff-render.ts index 4dd86ecbd2..3a4cc97305 100644 --- a/src/review/fix-handoff-render.ts +++ b/src/review/fix-handoff-render.ts @@ -37,15 +37,19 @@ 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 choose a code-span delimiter longer than any backtick run inside the value instead of trying + * to backslash-escape backticks (Markdown does not honor that inside code spans) — mirroring `markdownPathCode` + * in `src/review/unified-comment-bridge.ts`. Entity-escape `|`/`<>` (and backslashes) first, then return the + * full delimiter-wrapped span (with surrounding spaces) so callers must not re-wrap in a fixed single 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 @@ -54,8 +58,8 @@ function markdownPathCodeText(value: string): string { export function buildFixHandoffBlock(finding: InlineFinding): FixHandoffBlock { const hasLine = Number.isInteger(finding.line) && finding.line > 0; const line = hasLine ? finding.line : 0; - const safePath = markdownPathCodeText(finding.path); - const location = hasLine ? `${safePath}:${line}` : `${safePath} (no specific line)`; + // markdownPathCodeText returns the full delimiter-wrapped span — consume it directly (do not re-wrap). + const location = markdownPathCodeText(hasLine ? `${finding.path}:${line}` : `${finding.path} (no specific line)`); const label = finding.severity === "blocker" ? "Blocker" : "Nit"; const suggestedChange = finding.suggestion?.trim() || undefined; // Skip the fenced block when the suggestion itself contains a ``` sequence, which would close the outer fence @@ -64,7 +68,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}_`, @@ -105,14 +109,16 @@ const FIX_HANDOFF_AGGREGATE_MARKER = ""; * buildFixHandoffBlock, just indented under a shared numbered list instead of standing alone. */ function fixHandoffAggregateItem(finding: InlineFinding, index: number): string { const hasLine = Number.isInteger(finding.line) && finding.line > 0; - const safePath = markdownPathCodeText(finding.path); - const location = hasLine ? `${safePath}:${finding.line}` : `${safePath} (no specific line)`; + // Consume markdownPathCodeText's own delimiter-wrapped span directly (see buildFixHandoffBlock). + const location = markdownPathCodeText( + hasLine ? `${finding.path}:${finding.line}` : `${finding.path} (no specific line)`, + ); const label = finding.severity === "blocker" ? "Blocker" : "Nit"; const suggestion = finding.suggestion?.trim(); // 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..e2f7d7bf1f 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 literal backtick in the path is neutralized by choosing a longer code-span delimiter (``), not by + // backslash-escaping it (Markdown ignores that inside code spans) — pipe/angle-brackets stay entity-escaped (#9289). + expect(block?.body).toContain("`` src/x` [Review required](https://evil.example/phish) \\| <tag>:7 ``"); + expect(block?.body).not.toContain("\\`"); // no backslash-escaped backtick anymore 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..a5f8b9baca 100644 --- a/test/unit/fix-handoff-render.test.ts +++ b/test/unit/fix-handoff-render.test.ts @@ -12,7 +12,7 @@ describe("buildFixHandoffBlock (#2175)", () => { 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("**Fix handoff — Blocker at ` src/a.ts:12 `**"); }); it("renders a nit label", () => { @@ -76,6 +76,20 @@ describe("buildFixHandoffBlock (#2175)", () => { expect(block.instruction).toBe("Add a guard for the empty-array case."); expect(block.body).toContain("Add a guard for the empty-array case."); }); + + it("renders a backtick-containing path as one unbroken code span with a longer delimiter (#9289)", () => { + // A backtick in the path can't be neutralized by backslash-escaping inside a markdown code span, so the + // location is wrapped in a delimiter (here ``) longer than the longest backtick run in the value instead. + const block = buildFixHandoffBlock(finding({ path: "src/we`ird.ts", line: 5 })); + expect(block.body).toContain("**Fix handoff — Blocker at `` src/we`ird.ts:5 ``**"); + expect(block.body).not.toContain("\\`"); // backticks are NOT backslash-escaped (markdown ignores that in spans) + expect(block.path).toBe("src/we`ird.ts"); + }); + + it("preserves pipe/angle-bracket entity escaping alongside the widened delimiter (#9289)", () => { + const block = buildFixHandoffBlock(finding({ path: "src/a|b.ts", line: 1 })); + expect(block.body).toContain("**Fix handoff — Blocker at ` src/a\\|b<c>.ts:1 `**"); + }); }); describe("buildFixHandoffBlocks (#2175)", () => { @@ -101,7 +115,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,8 +125,8 @@ 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", () => { @@ -121,6 +135,14 @@ describe("buildFixHandoffAggregateBlock (#5102)", () => { expect(block?.body).not.toContain("src/a.ts:0"); }); + it("renders a backtick-containing path in the aggregate item as one unbroken code span (#9289)", () => { + // A run of two backticks forces a three-backtick delimiter (longest run + 1), keeping the span unbroken + // where a fixed single backtick would be closed prematurely. + const block = buildFixHandoffAggregateBlock([finding({ path: "src/a``b.ts", line: 3 })]); + expect(block?.body).toContain("1. **Blocker at ``` src/a``b.ts:3 ```** — Null check missing before dereference."); + expect(block?.body).not.toContain("\\`"); + }); + it("includes a fenced suggestion block, indented under its list item, when present", () => { const block = buildFixHandoffAggregateBlock([finding({ suggestion: "if (!value) return null;" })]); expect(block?.body).toContain("```\n if (!value) return null;\n ```"); diff --git a/test/unit/queue-4.test.ts b/test/unit/queue-4.test.ts index 710c0a29a9..c7b8b66b9a 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 });