From 8f93367da5ce3b4925f9209da75587c6465c1854 Mon Sep 17 00:00:00 2001 From: real-venus Date: Tue, 7 Jul 2026 08:41:31 -0700 Subject: [PATCH] feat(review): emit the fix-handoff blocks into the unified review comment (#1962) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The fix-handoff renderer (#2175), its config/gate resolver (#2176), the review.fixHandoff manifest toggle, and the follow-up-issue local-write action (#2177/#2185) all landed, but buildFixHandoffBlocks had no caller — the blocks were never emitted. Wire the final piece: append a "Fix handoff" collapsible to the unified review comment, one machine-readable block per inline finding a contributor's own local coding agent can consume (content only, no server-side write — every block carries the LOCAL_WRITE_BOUNDARY). Mirrors the impact-map / finding-category collapsible slices exactly: a pure buildFixHandoffCollapsible in the bridge (null on empty) plus an optional fixHandoffBlocks arg, fed at the publish site from THIS pass's inline findings (same source as findingCategories) only when shouldEmitFixHandoff ANDs the operator GITTENSORY_REVIEW_FIX_HANDOFF flag, the per-repo review.fixHandoff manifest opt-in, and the convergence allowlist. Default-OFF ⇒ the processor passes nothing ⇒ the rendered comment is byte-identical. Closes #1962. --- src/queue/processors.ts | 14 +++ src/review/unified-comment-bridge.ts | 30 +++++- test/unit/fix-handoff-collapsible.test.ts | 119 ++++++++++++++++++++++ test/unit/queue.test.ts | 73 +++++++++++++ 4 files changed, 235 insertions(+), 1 deletion(-) create mode 100644 test/unit/fix-handoff-collapsible.test.ts 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