Skip to content
Merged
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
14 changes: 14 additions & 0 deletions src/queue/processors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand Down
30 changes: 29 additions & 1 deletion src/review/unified-comment-bridge.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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. */
Expand Down Expand Up @@ -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;
Expand Down
119 changes: 119 additions & 0 deletions test/unit/fix-handoff-collapsible.test.ts
Original file line number Diff line number Diff line change
@@ -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> = {}): 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("<!-- gittensory:fix-handoff -->");
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(/<details><summary><b>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");
});
});
73 changes: 73 additions & 0 deletions test/unit/queue.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down