diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 798aeba04d..405a7d1a71 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -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, diff --git a/src/review/fix-handoff-render.ts b/src/review/fix-handoff-render.ts index a01f01d8ac..9b135a1adc 100644 --- a/src/review/fix-handoff-render.ts +++ b/src/review/fix-handoff-render.ts @@ -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 @@ -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 = ""; +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) @@ -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)); } diff --git a/test/unit/fix-handoff-render.test.ts b/test/unit/fix-handoff-render.test.ts index 5a2b37cc15..18063ff487 100644 --- a/test/unit/fix-handoff-render.test.ts +++ b/test/unit/fix-handoff-render.test.ts @@ -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)", () => { diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index 76b7d8bc59..4eac6c6105 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -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 }, @@ -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: [] }); @@ -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