From 89091acb58cb6dbb46c6bcd26cb75c9a2d834a31 Mon Sep 17 00:00:00 2001 From: bitfathers94 <237535319+bitfathers94@users.noreply.github.com> Date: Mon, 27 Jul 2026 13:09:08 +0000 Subject: [PATCH] fix(review): render fix-handoff locations with a length-based code-span delimiter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit markdownPathCodeText backslash-escaped backticks, but Markdown code spans are delimited by a run of backticks and do not honor a preceding backslash, so a finding path containing a literal backtick corrupted the rendered fix-handoff block. Compute a delimiter longer than any backtick run in the value and wrap the whole location in it — mirroring markdownPathCode in unified-comment-bridge.ts — and let both call sites consume that span directly instead of re-wrapping in a fixed single backtick. --- src/review/fix-handoff-render.ts | 29 ++++++++++++++--------- test/unit/fix-handoff-collapsible.test.ts | 6 +++-- test/unit/fix-handoff-render.test.ts | 17 +++++++++++++ 3 files changed, 39 insertions(+), 13 deletions(-) diff --git a/src/review/fix-handoff-render.ts b/src/review/fix-handoff-render.ts index 4dd86ecbd2..e65e329773 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 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 the remaining table/HTML metacharacters (`|`, `<`, + * `>`) and backslashes first, then wrap the whole value in the computed delimiter so a path containing a literal + * backtick still renders as one unbroken code span. */ 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 +59,9 @@ 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 code span, so consume it directly instead of + // re-wrapping in a fixed single backtick (which a backtick in the path would break out of). + 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 +70,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 +111,15 @@ 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) rather than + // re-wrapping in a fixed single backtick. + 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..a3ce81ea41 100644 --- a/test/unit/fix-handoff-collapsible.test.ts +++ b/test/unit/fix-handoff-collapsible.test.ts @@ -55,8 +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`"); - expect(block?.body).not.toContain("`src/x` [Review required](https://evil.example/phish) | :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 }); it("carries the no-server-side-write local-execution boundary on every block", () => { diff --git a/test/unit/fix-handoff-render.test.ts b/test/unit/fix-handoff-render.test.ts index 647c067b3d..872fec28fb 100644 --- a/test/unit/fix-handoff-render.test.ts +++ b/test/unit/fix-handoff-render.test.ts @@ -76,6 +76,15 @@ 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"); + }); }); describe("buildFixHandoffBlocks (#2175)", () => { @@ -121,6 +130,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 ```");