diff --git a/src/queue/processors.ts b/src/queue/processors.ts index f17204b4e9..798aeba04d 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -451,6 +451,8 @@ import { createReviewAdapters } from "../review/adapters"; import { extractChangedSymbols } from "../review/impact-symbols"; import { computeImpactMap } from "../review/impact-map"; import { formatImpactMapPromptSection, shouldComputeImpactMap } from "../review/impact-map-wire"; +import { shouldEmitFixHandoff } from "../review/fix-handoff"; +import { buildFixHandoffBlocks } from "../review/fix-handoff-render"; import { buildRepoCultureProfileContext, isRepoCultureProfileEnabled, @@ -8282,6 +8284,7 @@ async function maybePublishPrPublicSurface( let effortScoreEnabledForReview = false; let reviewMemoryEnabledForReview = false; let findingCategoriesEnabledForReview = false; + let fixHandoffEnabledForReview = false; let minFindingSeverityForReview: ReviewFindingSeverity | null = null; let inlineCommentsPerCategoryForReview: number | null = null; let aiReviewExpected = false; @@ -8804,6 +8807,11 @@ async function maybePublishPrPublicSurface( // pass). ANDed with the operator's GITTENSORY_REVIEW_MEMORY kill-switch at the actual apply site below // (shouldApplyReviewMemory) — this flag alone only carries the per-repo manifest opt-in. reviewMemoryEnabledForReview = shouldApplyReviewMemory(env, resolveReviewMemoryManifestToggle(reviewManifestForAutoReview)); + // review.fixHandoff emission (#1962): resolved the same unconditional way as the deterministic sections above, + // ANDing the per-repo `review.fixHandoff` manifest opt-in with the operator's GITTENSORY_REVIEW_FIX_HANDOFF + // kill-switch + convergence allowlist (shouldEmitFixHandoff). The blocks themselves are built from this pass's + // inline findings at the publish site below, mirroring findingCategories. + fixHandoffEnabledForReview = shouldEmitFixHandoff(env, repoFullName, reviewManifestForAutoReview?.review.fixHandoff ?? undefined); maybeAddRequiredAutoReviewSkipHold(env, { settings, advisory, @@ -10167,6 +10175,12 @@ async function maybePublishPrPublicSurface( ...(findingCategoriesEnabledForReview && aiReview?.inlineFindings?.length ? { findingCategories: aiReview.inlineFindings } : {}), + // review.fixHandoff emission (#1962): the SAME fresh inline findings feed the fix-handoff blocks — + // 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) } + : {}), maxFindingsCaps: reviewConfig.maxFindings, commentVerbosity: reviewConfig.commentVerbosity, // review-manifest validation (#2056): public PR comments may disclose only repo-published manifest diff --git a/src/review/unified-comment-bridge.ts b/src/review/unified-comment-bridge.ts index 0cc0f219a6..866bb8f3cb 100644 --- a/src/review/unified-comment-bridge.ts +++ b/src/review/unified-comment-bridge.ts @@ -31,6 +31,7 @@ import { GITTENSORY_GATE_CHECK_NAME } from "./check-names"; import { classifyChangedFile, type ReviewFileClass } from "./changed-files-classify"; import { githubPrFileDiffUrl } from "./changed-files-diff-link"; import { classifyFindingCategory, FINDING_CATEGORIES, type FindingCategory } from "./finding-category-classify"; +import type { FixHandoffBlock } from "./fix-handoff-render"; import { buildUnifiedReviewInput, renderUnifiedReviewComment, @@ -348,6 +349,12 @@ export type UnifiedCommentBridgeArgs = { * this only when BOTH the operator's GITTENSORY_REVIEW_IMPACT_MAP flag and the per-repo manifest opt-in * are on — see `shouldComputeImpactMap`, `src/review/impact-map-wire.ts`). */ impactMap?: ImpactMapSummaryInput[] | undefined; + /** review.fixHandoff emission (#1962): pre-rendered fix-handoff blocks (one per inline finding — a + * contributor's own local agent can consume them; content-only, no server-side write). When present and + * non-empty a "Fix handoff" collapsible is appended. Default OFF — the processor passes this only when the + * operator's GITTENSORY_REVIEW_FIX_HANDOFF flag AND the per-repo `review.fixHandoff` manifest opt-in are on + * (see `shouldEmitFixHandoff`, `src/review/fix-handoff.ts`), so the rendered comment is byte-identical when off. */ + fixHandoffBlocks?: FixHandoffBlock[] | undefined; /** The disposition holds this PR for owner review because its diff touches a hard-guardrail path — so an * otherwise-ready comment renders "held for review" instead of "safe to merge". (#guarded-hold-comment) */ heldForReview?: boolean | undefined; @@ -597,6 +604,19 @@ export function buildImpactMapCollapsible(entries: ImpactMapSummaryInput[]): Uni return { title: "Impact map", body }; } +/** + * Build the "Fix handoff" collapsible (#1962): the pre-rendered fix-handoff blocks for this review's inline + * findings, one per finding, so a contributor's OWN local coding agent can consume them. Pure rendering — each + * block's `.body` was already produced (and made public-safe) by `buildFixHandoffBlock` + * (src/review/fix-handoff-render.ts); this only stitches them under one collapsible. Returns null when there are + * no blocks (emission off, or no inline findings), so the caller chains it unconditionally exactly like the + * impact-map / finding-category collapsibles above. + */ +export function buildFixHandoffCollapsible(blocks: FixHandoffBlock[]): UnifiedCollapsible | null { + if (blocks.length === 0) return null; + return { title: "Fix handoff", body: blocks.map((block) => block.body).join("\n\n") }; +} + /** A finding's path + body — everything `buildFindingCategoryCollapsible` needs to use the finding's own * `category` when present, or fall back to `classifyFindingCategory` when it isn't. Deliberately narrower than * `InlineFinding` (no line/severity/suggestion) so the bridge's pure-rendering surface stays minimal. */ @@ -723,10 +743,18 @@ export function buildUnifiedCommentBody(args: UnifiedCommentBridgeArgs): string const impactMapCollapsible = args.impactMap && args.impactMap.length > 0 ? buildImpactMapCollapsible(args.impactMap) : null; const withImpactMap = impactMapCollapsible !== null ? [...(withFindingCategories ?? []), impactMapCollapsible] : withFindingCategories; + // review.fixHandoff emission (#1962): when the operator flag AND the manifest opt in, the processor hands us + // the pre-rendered fix-handoff blocks here; append the "Fix handoff" collapsible after Impact map (another + // structural, no-AI section) and ahead of the visual preview. Flag-OFF (the processor passes undefined) ⇒ + // extraCollapsibles is unchanged. + const fixHandoffCollapsible = + args.fixHandoffBlocks && args.fixHandoffBlocks.length > 0 ? buildFixHandoffCollapsible(args.fixHandoffBlocks) : null; + const withFixHandoff = + fixHandoffCollapsible !== null ? [...(withImpactMap ?? []), fixHandoffCollapsible] : withImpactMap; // Visual-capture port: when before/after routes are present, append a "Visual preview" collapsible to the // extra sections. Flag-OFF (the processor passes no beforeAfter) ⇒ extraCollapsibles is unchanged. const visualCollapsible = args.beforeAfter && args.beforeAfter.length > 0 ? buildBeforeAfterCollapsible(args.beforeAfter) : null; - const withVisual = visualCollapsible !== null ? [...(withImpactMap ?? []), visualCollapsible] : withImpactMap; + const withVisual = visualCollapsible !== null ? [...(withFixHandoff ?? []), visualCollapsible] : withFixHandoff; // #3612: "Scroll preview" renders ALONGSIDE "Visual preview" (never replacing it) — self-host + gif:true // only, so this is null (no section, no behavior change) for every repo that hasn't opted in. const scrollCollapsible = args.beforeAfter && args.beforeAfter.length > 0 ? buildScrollPreviewCollapsible(args.beforeAfter) : null; diff --git a/test/unit/fix-handoff-collapsible.test.ts b/test/unit/fix-handoff-collapsible.test.ts new file mode 100644 index 0000000000..25b41dcc11 --- /dev/null +++ b/test/unit/fix-handoff-collapsible.test.ts @@ -0,0 +1,119 @@ +import { describe, expect, it } from "vitest"; +import { buildFixHandoffCollapsible, buildUnifiedCommentBody, type ImpactMapSummaryInput } from "../../src/review/unified-comment-bridge"; +import { buildFixHandoffBlocks } from "../../src/review/fix-handoff-render"; +import { LOCAL_WRITE_BOUNDARY } from "../../src/mcp/local-write-tools"; +import type { InlineFinding } from "../../src/services/ai-review"; +import type { GateCheckEvaluation } from "../../src/rules/advisory"; +import type { PublicPrPanelSignalRow } from "../../src/signals/engine"; + +function gate(over: Partial = {}): GateCheckEvaluation { + return { + enabled: true, + conclusion: "success", + title: "Gittensory Orb Review Agent passed", + summary: "No configured hard blocker was found.", + blockers: [], + warnings: [], + ...over, + }; +} + +const panelRows: PublicPrPanelSignalRow[] = [ + { key: "gateResult", cells: ["Gate result", "✅ Passing", "No configured blocker found.", "No action."] }, +]; +const footer = "💰 Earn for open-source contributions. Checked by Gittensory."; + +// A blocker WITH a suggested change (line-anchored) and a nit WITHOUT one (path-only, line 0) — exercises both +// arms of buildFixHandoffBlock's suggestion + line rendering, which flow through the collapsible verbatim. +const findings: InlineFinding[] = [ + { path: "src/a.ts", line: 10, severity: "blocker", body: "Possible null dereference on the fetched record.", suggestion: "if (record) return record.value;" }, + { path: "src/b.ts", line: 0, severity: "nit", body: "Rename this helper for clarity." }, +]; +const blocks = buildFixHandoffBlocks(findings); + +describe("buildFixHandoffCollapsible (#1962)", () => { + it("renders one block per finding under a single Fix handoff collapsible", () => { + const c = buildFixHandoffCollapsible(blocks); + expect(c).not.toBeNull(); + expect(c?.title).toBe("Fix handoff"); + expect(c?.body).toContain(""); + 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)`"); + }); + + it("carries the no-server-side-write local-execution boundary on every block", () => { + expect(buildFixHandoffCollapsible(blocks)?.body).toContain(LOCAL_WRITE_BOUNDARY); + }); + + it("returns null for an empty block list (no empty section)", () => { + expect(buildFixHandoffCollapsible([])).toBeNull(); + }); + + it("is not marked as raw HTML (plain markdown body)", () => { + expect(buildFixHandoffCollapsible(blocks)?.rawHtml).toBeUndefined(); + }); +}); + +describe("buildUnifiedCommentBody fixHandoff wiring (#1962)", () => { + const base = { + gate: gate(), + panelRows, + readinessTotal: 90, + changedFiles: 2, + footerMarkdown: footer, + }; + const impactEntries: ImpactMapSummaryInput[] = [ + { changedModule: "src/a.ts", affectedModules: ["src/b.ts"], callers: ["doThing"] }, + ]; + + it("appends the Fix handoff section when fixHandoffBlocks is present + non-empty", () => { + const body = buildUnifiedCommentBody({ ...base, fixHandoffBlocks: blocks }); + expect(body).toContain("Fix handoff"); + expect(body).toMatch(/
Fix handoff<\/b><\/summary>/); + expect(body).toContain("Possible null dereference"); + }); + + it("does NOT add a Fix handoff section when fixHandoffBlocks is absent (flag-OFF parity)", () => { + expect(buildUnifiedCommentBody(base)).not.toContain("Fix handoff"); + }); + + it("does NOT add a Fix handoff section when fixHandoffBlocks is empty", () => { + expect(buildUnifiedCommentBody({ ...base, fixHandoffBlocks: [] })).not.toContain("Fix handoff"); + }); + + it("coexists with the Impact map and Visual preview sections (all render together)", () => { + const body = buildUnifiedCommentBody({ + ...base, + impactMap: impactEntries, + fixHandoffBlocks: blocks, + beforeAfter: [{ path: "/", afterUrl: "https://api.example.dev/gittensory/shot?key=gittensory/shots/x.png" }], + }); + expect(body).toContain("Impact map"); + expect(body).toContain("Fix handoff"); + expect(body).toContain("Visual preview"); + }); + + it("passes the fix-handoff chain through untouched when only a Visual preview is present (no fix-handoff blocks)", () => { + // Exercises the withVisual chain's `withFixHandoff ?? []` arm: no blocks ⇒ withFixHandoff is undefined, yet a + // Visual preview still renders — the fix-handoff link in the collapsible chain must not drop it. + const body = buildUnifiedCommentBody({ + ...base, + beforeAfter: [{ path: "/", afterUrl: "https://api.example.dev/gittensory/shot?key=gittensory/shots/x.png" }], + }); + expect(body).toContain("Visual preview"); + expect(body).not.toContain("Fix handoff"); + }); + + it("preserves pre-existing extraCollapsibles alongside the Fix handoff section", () => { + const body = buildUnifiedCommentBody({ + ...base, + extraCollapsibles: [{ title: "Signal definitions", body: "what each row means" }], + fixHandoffBlocks: blocks, + }); + expect(body).toContain("Signal definitions"); + expect(body).toContain("Fix handoff"); + }); +}); diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index 70dfb4aa87..76b7d8bc59 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -18256,6 +18256,79 @@ describe("queue processors", () => { expect(unifiedCommentBody).toContain("| Security | 1 |"); }); + // #1962: with BOTH the operator flag and the manifest opt-in on, the review emits a "Fix handoff" collapsible — + // one machine-readable block per inline finding a contributor's own local agent can consume — in the unified + // comment. Flag-OFF (every other review test) ⇒ no such section. + it("emits the Fix handoff collapsible in the unified comment when review.fixHandoff + the operator flag are on", async () => { + const env = createTestEnv({ + GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), + GITTENSORY_REVIEW_UNIFIED_COMMENT: "1", + GITTENSORY_REVIEW_INLINE_COMMENTS: "true", + GITTENSORY_REVIEW_FIX_HANDOFF: "true", + GITTENSORY_REVIEW_REPOS: "JSONbored/gittensory", + AI: { + run: async () => + ({ + response: JSON.stringify({ + assessment: "One real issue.", + blockers: [], + nits: [], + suggestions: [], + inlineFindings: [ + { path: "src/db.ts", line: 2, severity: "blocker", body: "This query is vulnerable to SQL injection.", suggestion: "Use a parameterized query." }, + ], + }), + }) as { response: string }, + } as unknown as Ai, + AI_SUMMARIES_ENABLED: "true", + AI_PUBLIC_COMMENTS_ENABLED: "true", + AI_DAILY_NEURON_BUDGET: "100000", + }); + await upsertRepositoryFromGitHub(env, { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, 123); + await upsertRepositorySettings(env, { repoFullName: "JSONbored/gittensory", commentMode: "all_prs", publicSurface: "comment_only", autoLabelEnabled: false, checkRunMode: "off", gateCheckMode: "enabled", aiReviewMode: "block", gatePack: "oss-anti-slop" }); + let unifiedCommentBody = ""; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url === "https://raw.githubusercontent.com/JSONbored/gittensory/HEAD/.gittensory.yml") { + // NOTE: the manifest key is camelCase `fixHandoff` (unlike snake-case `finding_categories`) — see focus-manifest parse. + 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;" }]); + 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: [] }); + if (url.includes("/issues/1")) return Response.json({ number: 1, title: "Issue", state: "open", labels: [], user: { login: "reporter" } }); + if (url.includes("/branches/")) return Response.json({ protected: false, protection: { required_status_checks: { contexts: [] } } }); + if (url.endsWith("/pulls/9/reviews") && method === "POST") return Response.json({ id: 55 }); + if (url.includes("/issues/9/comments") && method === "GET") return Response.json([]); + if (url.includes("/issues/9/comments") && method === "POST") { + unifiedCommentBody = String((JSON.parse(String(init?.body ?? "{}")) as { body?: string }).body ?? ""); + return Response.json({ id: 1 }, { status: 201 }); + } + return Response.json({}); + }); + + await processJob(env, { + type: "github-webhook", + deliveryId: "pr-fix-handoff", + eventName: "pull_request", + payload: { + action: "opened", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + pull_request: { number: 9, title: "Add query helper", state: "open", user: { login: "contributor" }, head: { sha: "a9" }, labels: [], body: "Closes #1" }, + }, + }); + + 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("This query is vulnerable to SQL injection."); // the finding, handed off verbatim + expect(unifiedCommentBody).toContain("Suggested change:"); // its suggestion carried through + }); + // FIX B + FIX D3 at the processor call site: a unified comment for a PR whose CI has a FAILED check, with the // PR's files only available from GitHub (stored rows empty) — proves (B) the inline file fetch populates the // real diff/changed-file count on the first review, and (D3) the failing check name + its per-check WHY render