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
2 changes: 1 addition & 1 deletion src/queue/processors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10179,7 +10179,7 @@ async function maybePublishPrPublicSurface(
// present ONLY on a cache-miss review with inline comments enabled — so a cache hit never re-emits them,
// exactly like findingCategories above. Flag-OFF ⇒ omitted ⇒ the rendered comment is byte-identical.
...(fixHandoffEnabledForReview && aiReview?.inlineFindings?.length
? { fixHandoffBlocks: buildFixHandoffBlocks(aiReview.inlineFindings) }
? { fixHandoffBlocks: buildFixHandoffBlocks(aiReview.inlineFindings, { repoFullName }) }
: {}),
maxFindingsCaps: reviewConfig.maxFindings,
commentVerbosity: reviewConfig.commentVerbosity,
Expand Down
28 changes: 24 additions & 4 deletions src/review/fix-handoff-render.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,14 @@
// already produced through the public-safe filter (InlineFinding.body/suggestion are sanitized upstream by
// composeInlineFindings before they ever reach here — this module adds no new free text of its own beyond the
// fixed label/marker strings below).
import { LOCAL_WRITE_BOUNDARY } from "../mcp/local-write-tools";
import { buildFollowUpIssueSpec, LOCAL_WRITE_BOUNDARY } from "../mcp/local-write-tools";
import type { InlineFinding } from "../services/ai-review";

export type FixHandoffBuildOptions = {
/** When set, nit findings also carry a runnable follow-up-issue command (#2177 / #1962). Omitted ⇒ byte-identical. */
repoFullName?: string | undefined;
};

/** A single finding rendered as a structured, LOCAL-execution fix-handoff block. `line` is `0` when the
* finding has no commentable diff line (mirrors the codebase's existing path-only sentinel — see
* `secretLeakFinding`/`scanDiffForSecretsWithLocations` in review/safety.ts, review/secrets-scan.ts) so the
Expand All @@ -33,21 +38,36 @@ export type FixHandoffBlock = {
* parse fix-handoff blocks in a comment body without depending on markdown structure alone. */
const FIX_HANDOFF_MARKER = "<!-- gittensory:fix-handoff -->";

function followUpIssueSection(finding: InlineFinding, repoFullName: string, line: number): string {
const spec = buildFollowUpIssueSpec({
repoFullName,
path: finding.path,
...(line > 0 ? { line } : {}),
finding: finding.body,
});
return `\n\n**Or file a follow-up issue** (track this instead of fixing in this PR):\n\`\`\`bash\n${spec.command}\n\`\`\``;
}

/** PURE: build a single finding's fix-handoff block. Never throws; a finding whose `line` is not a positive
* integer (0, negative, non-finite — i.e. "no commentable line") still yields a valid PATH-ONLY block rather
* than being dropped, since the finding itself is still actionable context even without a line anchor. */
export function buildFixHandoffBlock(finding: InlineFinding): FixHandoffBlock {
export function buildFixHandoffBlock(finding: InlineFinding, options?: FixHandoffBuildOptions): FixHandoffBlock {
const hasLine = Number.isInteger(finding.line) && finding.line > 0;
const line = hasLine ? finding.line : 0;
const location = hasLine ? `${finding.path}:${line}` : `${finding.path} (no specific line)`;
const label = finding.severity === "blocker" ? "Blocker" : "Nit";
const suggestedChange = finding.suggestion?.trim() || undefined;
const suggestionBlock = suggestedChange ? `\n\nSuggested change:\n\`\`\`\n${suggestedChange}\n\`\`\`` : "";
const followUpBlock =
finding.severity === "nit" && options?.repoFullName
? followUpIssueSection(finding, options.repoFullName, line)
: "";
const body = [
FIX_HANDOFF_MARKER,
`**Fix handoff — ${label} at \`${location}\`**`,
finding.body,
suggestionBlock,
followUpBlock,
`\n_${LOCAL_WRITE_BOUNDARY}_`,
]
.filter((part) => part.length > 0)
Expand All @@ -65,6 +85,6 @@ export function buildFixHandoffBlock(finding: InlineFinding): FixHandoffBlock {

/** PURE: build a fix-handoff block for every finding in order. Empty in ⇒ empty out — no-op when there is
* nothing to hand off. */
export function buildFixHandoffBlocks(findings: InlineFinding[]): FixHandoffBlock[] {
return findings.map((finding) => buildFixHandoffBlock(finding));
export function buildFixHandoffBlocks(findings: InlineFinding[], options?: FixHandoffBuildOptions): FixHandoffBlock[] {
return findings.map((finding) => buildFixHandoffBlock(finding, options));
}
29 changes: 29 additions & 0 deletions test/unit/fix-handoff-render.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,35 @@ 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("appends a follow-up-issue command for nit findings when repoFullName is supplied (#2177)", () => {
const block = buildFixHandoffBlock(finding({ severity: "nit", body: "Rename this helper for clarity." }), {
repoFullName: "o/r",
});
expect(block.body).toContain("Or file a follow-up issue");
expect(block.body).toContain("gh issue create");
expect(block.body).toContain("Follow up: src/a.ts:12");
expect(block.body).toContain("Rename this helper for clarity.");
});

it("does NOT append a follow-up-issue command for blockers (fix in this PR, not defer)", () => {
const block = buildFixHandoffBlock(finding({ severity: "blocker" }), { repoFullName: "o/r" });
expect(block.body).not.toContain("Or file a follow-up issue");
expect(block.body).not.toContain("gh issue create");
});

it("omits the follow-up-issue command when repoFullName is absent (flag-OFF / caller parity)", () => {
const block = buildFixHandoffBlock(finding({ severity: "nit" }));
expect(block.body).not.toContain("Or file a follow-up issue");
});

it("follow-up-issue command uses the path-only location when the nit has no commentable line", () => {
const block = buildFixHandoffBlock(finding({ severity: "nit", line: 0, body: "Consider splitting this module." }), {
repoFullName: "o/r",
});
expect(block.body).toContain("Follow up: src/a.ts'");
expect(block.body).not.toContain("src/a.ts:0");
});
});

describe("buildFixHandoffBlocks (#2175)", () => {
Expand Down
14 changes: 13 additions & 1 deletion test/unit/queue.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18276,6 +18276,7 @@ describe("queue processors", () => {
suggestions: [],
inlineFindings: [
{ path: "src/db.ts", line: 2, severity: "blocker", body: "This query is vulnerable to SQL injection.", suggestion: "Use a parameterized query." },
{ path: "src/util.ts", line: 1, severity: "nit", body: "Consider extracting this into a shared helper." },
],
}),
}) as { response: string },
Expand All @@ -18296,7 +18297,10 @@ describe("queue processors", () => {
return new Response("review:\n inline_comments: true\n fixHandoff: true\n");
}
if (url.includes("/pulls/9/files"))
return Response.json([{ filename: "src/db.ts", status: "modified", additions: 1, deletions: 0, changes: 1, patch: "@@ -1,1 +1,2 @@\n ctx\n+export const ok = true;" }]);
return Response.json([
{ filename: "src/db.ts", status: "modified", additions: 1, deletions: 0, changes: 1, patch: "@@ -1,1 +1,2 @@\n ctx\n+export const ok = true;" },
{ filename: "src/util.ts", status: "added", additions: 1, deletions: 0, changes: 1, patch: "@@ -0,0 +1,1 @@\n+export const helper = true;" },
]);
if (url.endsWith("/pulls/9")) return Response.json({ number: 9, title: "Add query helper", state: "open", user: { login: "contributor" }, head: { sha: "a9" }, labels: [], body: "Closes #1", mergeable_state: "clean" });
if (url.includes("/commits/a9/check-runs")) return Response.json({ total_count: 0, check_runs: [] });
if (url.includes("/commits/a9/status")) return Response.json({ state: "success", statuses: [] });
Expand Down Expand Up @@ -18327,6 +18331,14 @@ describe("queue processors", () => {
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
expect(unifiedCommentBody).toContain("Or file a follow-up issue"); // nit findings carry the follow-up-issue local-write command
expect(unifiedCommentBody).toContain("gh issue create --repo 'JSONbored/gittensory'");
expect(unifiedCommentBody).toContain("Consider extracting this into a shared helper.");
expect(unifiedCommentBody).toContain("Fix handoff — Nit at `src/util.ts:1`");
// the blocker block itself does not defer — follow-up text appears only on the nit block below it
expect(unifiedCommentBody.indexOf("Or file a follow-up issue")).toBeGreaterThan(
unifiedCommentBody.indexOf("This query is vulnerable to SQL injection."),
);
});

// FIX B + FIX D3 at the processor call site: a unified comment for a PR whose CI has a FAILED check, with the
Expand Down