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
19 changes: 12 additions & 7 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 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 === "<" ? "&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 @@ -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}_`,
Expand Down Expand Up @@ -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
Expand Down
8 changes: 5 additions & 3 deletions test/unit/fix-handoff-collapsible.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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", () => {
Expand All @@ -55,7 +55,9 @@ 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 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) \\| &lt;tag&gt; ``:7");
expect(block?.body).not.toContain("`src/x` [Review required](https://evil.example/phish) | <tag>:7`");
});

Expand Down
32 changes: 25 additions & 7 deletions test/unit/fix-handoff-render.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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/<a>|b.ts", line: 12 }));
expect(block.body).toContain("` src/&lt;a&gt;\\|b.ts `:12");
});

it("renders a nit label", () => {
Expand Down Expand Up @@ -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");
});

Expand Down Expand Up @@ -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", () => {
Expand All @@ -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");
});

Expand Down
2 changes: 1 addition & 1 deletion test/unit/queue-4.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
});
Expand Down
Loading