Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 18 additions & 11 deletions src/review/fix-handoff-render.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 = "<!-- 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 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 === "<" ? "&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
Expand All @@ -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
Expand All @@ -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}_`,
Expand Down Expand Up @@ -105,14 +111,15 @@ const FIX_HANDOFF_AGGREGATE_MARKER = "<!-- loopover:fix-handoff-aggregate -->";
* 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
Expand Down
6 changes: 4 additions & 2 deletions test/unit/fix-handoff-collapsible.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +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`");
expect(block?.body).not.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) \\| &lt;tag&gt;:7``");
expect(block?.body).not.toContain("\\`"); // no backslash-escaped backtick anymore
});

it("carries the no-server-side-write local-execution boundary on every block", () => {
Expand Down
17 changes: 17 additions & 0 deletions test/unit/fix-handoff-render.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)", () => {
Expand Down Expand Up @@ -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 ```");
Expand Down
Loading