From 39c1d7ef0bb6695e802825fd0ecc63f68fd9d60d Mon Sep 17 00:00:00 2001 From: Charles Howard <96023061+charlesrhoward@users.noreply.github.com> Date: Tue, 29 Sep 2026 10:15:12 -0400 Subject: [PATCH 1/7] fix(pr-review): never publish a clean verdict when the reviewer drops its findings --- lib/agents/pr-reviewer.ts | 28 +++- lib/workflows/automation-job-metadata.ts | 3 +- lib/workflows/automation-job-types.ts | 4 +- lib/workflows/pr-review-harness-extraction.ts | 14 +- lib/workflows/pr-review-harness-formatting.ts | 2 + lib/workflows/pr-review-harness-types.ts | 3 +- lib/workflows/pr-review-report-repair.ts | 41 +++--- lib/workflows/pr-review-report-state.test.ts | 132 ++++++++++++++++++ lib/workflows/pr-review-report-state.ts | 47 +++++++ tests/unit/helpers/automation-job-fixtures.ts | 2 +- tests/unit/pr-review-report-repair.test.ts | 91 ++++++++++++ .../unit/pr-reviewer-dropped-findings.test.ts | 102 ++++++++++++++ 12 files changed, 442 insertions(+), 27 deletions(-) create mode 100644 lib/workflows/pr-review-report-state.test.ts create mode 100644 lib/workflows/pr-review-report-state.ts create mode 100644 tests/unit/pr-reviewer-dropped-findings.test.ts diff --git a/lib/agents/pr-reviewer.ts b/lib/agents/pr-reviewer.ts index ec2f4be3..a95a6aa9 100644 --- a/lib/agents/pr-reviewer.ts +++ b/lib/agents/pr-reviewer.ts @@ -12,6 +12,7 @@ import { prepareGitHubTextFile, type GitHubFileContent, } from "@/lib/agents/github-file-content"; +import { clearsReviewWithoutFindings } from "@/lib/workflows/pr-review-report-state"; const PR_REVIEW_FILE_CONTENT_CHAR_LIMIT = GITHUB_FILE_CONTENT_CHAR_LIMIT; const PR_REVIEW_PATCH_CHAR_LIMIT = 4_000; @@ -58,6 +59,8 @@ export function buildPRReviewTools(config: { const request = config.fetch ?? fetch; const contentOwner = config.headOwner ?? config.owner; const contentRepo = config.headRepo ?? config.repo; + let claimedIssues = false; + let acceptedReport: Record | null = null; const reviewFindingSchema = z.object({ severity: z.enum(["critical", "warning", "suggestion"]), title: z.string(), @@ -83,16 +86,32 @@ export function buildPRReviewTools(config: { findings: z.array(reviewFindingSchema).max(20).optional(), }) .superRefine((value, ctx) => { + // Runs for every report, including ones the schema rejects, which + // never reach `execute`. + if (value.hasIssues) claimedIssues = true; if (value.hasIssues && (!value.findings || value.findings.length === 0)) { ctx.addIssue({ code: z.ZodIssueCode.custom, path: ["findings"], message: - "findings must include at least one entry when hasIssues=true", + "findings must include at least one entry when hasIssues=true. Call reportReview again with each issue as an entry in findings. Setting hasIssues=false without listing the issues leaves the review without a verdict.", }); } }); + // A report that clears the review after an earlier one claimed issues has + // dropped them, so it cannot authorize a merge. + const droppedFindingsRefusal = () => + claimedIssues && + acceptedReport !== null && + clearsReviewWithoutFindings(acceptedReport) + ? { + success: false as const, + error: + "Not merged: this review said it found issues but its report lists none. Report each issue as a finding.", + } + : null; + const tools = { getPullRequest: tool({ description: @@ -222,7 +241,10 @@ export function buildPRReviewTools(config: { description: "Record the structured review result for workflow orchestration. Call exactly once after analysis.", inputSchema: reportReviewInputSchema, - execute: async (input) => input, + execute: async (input) => { + acceptedReport = input; + return input; + }, }), }; @@ -236,6 +258,7 @@ export function buildPRReviewTools(config: { commitTitle: z.string().optional(), }), execute: async ({ commitTitle }) => + droppedFindingsRefusal() ?? mergePullRequestIfSafe({ githubToken: config.githubToken, owner: config.owner, @@ -252,6 +275,7 @@ export function buildPRReviewTools(config: { commitTitle: z.string().optional(), }), execute: async ({ commitTitle }) => + droppedFindingsRefusal() ?? queuePullRequestAutoMerge({ githubToken: config.githubToken, owner: config.owner, diff --git a/lib/workflows/automation-job-metadata.ts b/lib/workflows/automation-job-metadata.ts index 4678d37b..4de95ccc 100644 --- a/lib/workflows/automation-job-metadata.ts +++ b/lib/workflows/automation-job-metadata.ts @@ -240,7 +240,7 @@ export function normalizeAutomationAgentResult(result: { inputTokens?: number | null; outputTokens?: number | null; } | null; - toolCalls?: Array<{ toolName: string; input: unknown }>; + toolCalls?: Array<{ toolName: string; input: unknown; invalid?: boolean }>; toolResults?: unknown[]; }>; }): AutomationAgentResult { @@ -252,6 +252,7 @@ export function normalizeAutomationAgentResult(result: { toolCalls: (step.toolCalls || []).map((toolCall) => ({ toolName: toolCall.toolName, input: toolCall.input, + ...(toolCall.invalid === true ? { invalid: true } : {}), })), toolResults: step.toolResults, })), diff --git a/lib/workflows/automation-job-types.ts b/lib/workflows/automation-job-types.ts index 105eaaee..45974966 100644 --- a/lib/workflows/automation-job-types.ts +++ b/lib/workflows/automation-job-types.ts @@ -132,7 +132,9 @@ export type AutomationAgentUsage = { export type AutomationAgentResult = { text: string; steps: Array<{ - toolCalls?: Array<{ toolName: string; input: unknown }>; + // `invalid` marks a call the SDK rejected (its input failed the tool's + // schema); the call is kept so a reader can tell what was attempted. + toolCalls?: Array<{ toolName: string; input: unknown; invalid?: boolean }>; toolResults?: unknown[]; }>; usage: AutomationAgentUsage | null; diff --git a/lib/workflows/pr-review-harness-extraction.ts b/lib/workflows/pr-review-harness-extraction.ts index 32962854..4570e849 100644 --- a/lib/workflows/pr-review-harness-extraction.ts +++ b/lib/workflows/pr-review-harness-extraction.ts @@ -10,6 +10,7 @@ import { toReviewFindings, toStringArray, } from "./pr-review-harness-utils"; +import { readReviewReportState } from "./pr-review-report-state"; function extractLastToolInput( result: AutomationAgentReviewResult, @@ -153,8 +154,9 @@ export function extractPrAutofixOutcome( /** * True when the reviewer finished without filing `reportReview` and nothing - * else reported a problem. The "no issues" in that result is a default, not a - * finding, so it must never be published as a clean verdict or allow a merge. + * else reported a problem, or filed one that dropped the issues it had + * claimed. The "no issues" in that result is a default, not a finding, so it + * must never be published as a clean verdict or allow a merge. */ export function isPrReviewVerdictMissing( harnessResult: Pick | null @@ -169,11 +171,13 @@ export function isPrReviewVerdictMissing( export function extractPrReviewHarnessResult( result: AutomationAgentReviewResult ): PrReviewHarnessResult { - const report = extractLastToolInput(result, "reportReview"); + const reportState = readReviewReportState(result.steps); const autofix = extractPrAutofixOutcome(result); - if (report) { + if (reportState.kind !== "missing") { + const { report } = reportState; return { - source: "structured", + source: + reportState.kind === "filed" ? "structured" : "dropped_findings", fallbackText: toOptionalString(result.text), reviewOutcome: { hasIssues: report.hasIssues === true, diff --git a/lib/workflows/pr-review-harness-formatting.ts b/lib/workflows/pr-review-harness-formatting.ts index f8a49f9f..7d3d7430 100644 --- a/lib/workflows/pr-review-harness-formatting.ts +++ b/lib/workflows/pr-review-harness-formatting.ts @@ -13,6 +13,8 @@ export function buildPrReviewContractNote(source: PrReviewContractSource) { switch (source) { case "legacy_post_comment": return "Note: Structured review output was missing, so Mogplex used the legacy review comment as fallback output."; + case "dropped_findings": + return "Note: The reviewer said it found issues but its report did not list them, so there is no verdict. The text below is its summary only. Rerun the review for a full result."; case "legacy_text": return "Note: The reviewer finished without filing its structured report, so there is no verdict. The text below is its closing summary only. Rerun the review for a full result."; case "structured": diff --git a/lib/workflows/pr-review-harness-types.ts b/lib/workflows/pr-review-harness-types.ts index 02a1bce2..a245b57d 100644 --- a/lib/workflows/pr-review-harness-types.ts +++ b/lib/workflows/pr-review-harness-types.ts @@ -3,7 +3,7 @@ import type { ReviewFinding } from "@/lib/types"; export type AutomationAgentReviewResult = { text: string; steps: Array<{ - toolCalls?: Array<{ toolName: string; input: unknown }>; + toolCalls?: Array<{ toolName: string; input: unknown; invalid?: boolean }>; toolResults?: unknown[]; }>; }; @@ -32,6 +32,7 @@ export type PrAutofixOutcome = { export type PrReviewContractSource = | "structured" + | "dropped_findings" | "legacy_post_comment" | "legacy_text"; diff --git a/lib/workflows/pr-review-report-repair.ts b/lib/workflows/pr-review-report-repair.ts index c079736c..4de501de 100644 --- a/lib/workflows/pr-review-report-repair.ts +++ b/lib/workflows/pr-review-report-repair.ts @@ -2,13 +2,28 @@ import type { ModelMessage, ToolSet } from "ai"; import type { AutomationAgentResult } from "./automation-job-types"; import { mergeAutomationAgentResults } from "./automation-job-metadata"; import { AutomationModelExecutionError } from "./automation-model-execution-errors"; - -export const REPORT_REVIEW_TOOL_NAME = "reportReview"; +import { + readReviewReportState, + REPORT_REVIEW_TOOL_NAME, + type ReviewReportState, +} from "./pr-review-report-state"; /** Asks for the record of the review that already happened, not a new one. */ export const PR_REVIEW_REPORT_REPAIR_PROMPT = "You finished this review without calling reportReview, so nothing was recorded and the pull request has no verdict. Call reportReview now with the verdict and findings from the review you just did. Do not start a new review and do not call any other tool."; +/** Asks for the findings a report claimed and then dropped. */ +export const PR_REVIEW_DROPPED_FINDINGS_PROMPT = + "Your review said it found issues, but no report you filed lists them, so the pull request has no verdict. Call reportReview now with every issue from the review you just did as an entry in findings, each with severity, title, body, and path; non-blocking ones are severity suggestion and can go with hasIssues=false. Do not start a new review and do not call any other tool."; + +const REPAIR_PROMPTS: Record< + Exclude, + string +> = { + missing: PR_REVIEW_REPORT_REPAIR_PROMPT, + dropped_findings: PR_REVIEW_DROPPED_FINDINGS_PROMPT, +}; + const FORCED_REPORT_CHOICE = { type: "tool", toolName: REPORT_REVIEW_TOOL_NAME, @@ -20,16 +35,6 @@ export type ReportRepairRequest = { messages: ModelMessage[]; }; -export function hasFiledReviewReport( - steps: AutomationAgentResult["steps"] -): boolean { - return steps.some((step) => - (step.toolCalls ?? []).some( - (toolCall) => toolCall.toolName === REPORT_REVIEW_TOOL_NAME - ) - ); -} - /** * The report tool alone, without its `execute`. A tool the SDK cannot run * ends the call as soon as the model has filled it in, so the forced call @@ -49,6 +54,7 @@ export function buildReportRepairMessages(input: { prompt: string; responseMessages: ModelMessage[] | undefined; text: string; + request: string; }): ModelMessage[] { const transcript: ModelMessage[] = input.responseMessages?.length ? input.responseMessages @@ -56,7 +62,7 @@ export function buildReportRepairMessages(input: { return [ { role: "user", content: input.prompt }, ...transcript, - { role: "user", content: PR_REVIEW_REPORT_REPAIR_PROMPT }, + { role: "user", content: input.request }, ]; } @@ -76,8 +82,9 @@ function isRejectedRequest(error: unknown): boolean { } /** - * When a review ends without its structured report, ask the same model once - * to file it from the work it already did. The report is forced where the + * When a review ends without a structured report it can trust (none was + * accepted, or the accepted one dropped the issues it claimed), ask the same + * model once to file it from the work it already did. The report is forced where the * provider allows it; a provider that rejects a forced tool choice is asked * again with the report as the only tool on offer. The reviewer's own closing * text is kept. This never fails a review that otherwise completed: if the @@ -91,7 +98,8 @@ export async function fileMissingReviewReport(input: { tools: ToolSet; generate: (request: ReportRepairRequest) => Promise; }): Promise { - if (hasFiledReviewReport(input.result.steps)) return input.result; + const state = readReviewReportState(input.result.steps); + if (state.kind === "filed") return input.result; const tools = buildReportOnlyTools(input.tools); if (!tools) return input.result; @@ -99,6 +107,7 @@ export async function fileMissingReviewReport(input: { prompt: input.prompt, responseMessages: input.responseMessages, text: input.result.text, + request: REPAIR_PROMPTS[state.kind], }); const ask = (toolChoice: ReportRepairRequest["toolChoice"]) => input.generate({ tools, toolChoice, messages }); diff --git a/lib/workflows/pr-review-report-state.test.ts b/lib/workflows/pr-review-report-state.test.ts new file mode 100644 index 00000000..0c14ada8 --- /dev/null +++ b/lib/workflows/pr-review-report-state.test.ts @@ -0,0 +1,132 @@ +import { describe, expect, it } from "vitest"; +import { getPrReviewAutoMergeBlockReason } from "./automation-job-auto-merge"; +import { + extractPrReviewHarnessResult, + isPrReviewVerdictMissing, +} from "./pr-review-harness-extraction"; +import { + buildPrReviewCheckTitle, + buildPrReviewContractNote, + buildPrReviewStatusHeading, +} from "./pr-review-harness-formatting"; +import { buildPrReviewTimelineCommentBody } from "./pr-review-harness"; +import { readReviewReportState } from "./pr-review-report-state"; + +const CLAIM = "One warning and three suggestions, detailed in the comment."; + +/** What the SDK hands back for a report whose input failed the schema. */ +function rejected(input: Record) { + return { toolName: "reportReview", input, invalid: true }; +} + +function accepted(input: Record) { + return { toolName: "reportReview", input }; +} + +function review(...toolCalls: Array>) { + return { text: "Reviewed.", steps: [{ toolCalls, toolResults: [] }] }; +} + +const suggestion = { + severity: "suggestion", + title: "Name the retry budget", + body: "The 3 is a magic number.", + path: "lib/retry.ts", +}; + +/** The #535 shape: rejected issue reports, then a clean one with nothing listed. */ +const droppedFindings = review( + rejected({ hasIssues: true, summary: CLAIM }), + rejected({ hasIssues: true, summary: CLAIM }), + accepted({ hasIssues: false, summary: CLAIM }) +); + +describe("readReviewReportState", () => { + it("should treat a clean report after a rejected issue report as dropped findings", () => { + expect(readReviewReportState(droppedFindings.steps).kind).toBe( + "dropped_findings" + ); + }); + + it("should treat a clean report that clears earlier listed findings as dropped findings", () => { + const cleared = review( + accepted({ hasIssues: true, summary: CLAIM, findings: [suggestion] }), + accepted({ hasIssues: false, summary: "Fine." }) + ); + + expect(readReviewReportState(cleared.steps).kind).toBe("dropped_findings"); + }); + + it("should treat a review whose every report was rejected as missing", () => { + const allRejected = review( + rejected({ hasIssues: true, summary: CLAIM }), + rejected({ hasIssues: true, summary: CLAIM }) + ); + + expect(readReviewReportState(allRejected.steps).kind).toBe("missing"); + }); + + it("should use the accepted report, not a rejected one after it", () => { + const state = readReviewReportState( + review( + accepted({ hasIssues: true, summary: CLAIM, findings: [suggestion] }), + rejected({ hasIssues: true, summary: "Retry." }) + ).steps + ); + + expect(state.kind).toBe("filed"); + expect(state.kind === "filed" && state.report.summary).toBe(CLAIM); + }); + + it("should accept a clean report that lists its suggestions after a rejected claim", () => { + const listed = review( + rejected({ hasIssues: true, summary: CLAIM }), + accepted({ hasIssues: false, summary: CLAIM, findings: [suggestion] }) + ); + + expect(readReviewReportState(listed.steps).kind).toBe("filed"); + }); + + it("should accept a clean report nothing contradicted", () => { + const clean = review(accepted({ hasIssues: false, summary: "Fine." })); + + expect(readReviewReportState(clean.steps).kind).toBe("filed"); + }); +}); + +describe("what a review that dropped its findings publishes", () => { + const harnessResult = extractPrReviewHarnessResult(droppedFindings); + + it("should have no verdict", () => { + expect(harnessResult.source).toBe("dropped_findings"); + expect(isPrReviewVerdictMissing(harnessResult)).toBe(true); + }); + + it("should say the review is incomplete and why, never that no issues were found", () => { + const body = buildPrReviewTimelineCommentBody({ + harnessResult, + fallbackText: droppedFindings.text, + conclusion: "neutral", + }); + + expect( + buildPrReviewStatusHeading({ harnessResult, conclusion: "neutral" }) + ).toBe("**Status:** Review incomplete"); + expect( + buildPrReviewCheckTitle({ harnessResult, conclusion: "neutral" }) + ).toBe("Review incomplete"); + expect(body).toContain(buildPrReviewContractNote("dropped_findings")); + expect(body).not.toContain("No material issues found"); + }); + + it("should block an auto-merge", () => { + expect( + getPrReviewAutoMergeBlockReason({ + reviewOutcome: harnessResult.reviewOutcome, + verdictMissing: isPrReviewVerdictMissing(harnessResult), + requestedPrNumber: 42, + reviewedPrNumber: 42, + }) + ).toBe("Mogplex review finished without a structured verdict"); + }); +}); diff --git a/lib/workflows/pr-review-report-state.ts b/lib/workflows/pr-review-report-state.ts new file mode 100644 index 00000000..8ffc8098 --- /dev/null +++ b/lib/workflows/pr-review-report-state.ts @@ -0,0 +1,47 @@ +import type { AutomationAgentReviewResult } from "./pr-review-harness-types"; +import { isRecord, toReviewFindings } from "./pr-review-harness-utils"; + +export const REPORT_REVIEW_TOOL_NAME = "reportReview"; + +export type ReviewReportState = + | { kind: "missing" } + | { kind: "filed"; report: Record } + | { kind: "dropped_findings"; report: Record }; + +/** A report that clears the review: no issues and nothing listed. */ +export function clearsReviewWithoutFindings( + report: Record +): boolean { + return ( + report.hasIssues !== true && toReviewFindings(report.findings).length === 0 + ); +} + +/** + * The reviewer's report as the pipeline should trust it. Only calls the SDK + * accepted count as filed; a call whose input failed the schema was never + * recorded. A report that clears the review after an earlier call said there + * were issues has lost them: reviewers told that hasIssues=true needs + * findings sometimes flip hasIssues to false instead of listing them, and + * that report would otherwise publish as a clean verdict with its warnings + * gone. An earlier report that listed findings and a later one that clears + * them is treated the same way, since nothing says the findings were wrong. + */ +export function readReviewReportState( + steps: AutomationAgentReviewResult["steps"] +): ReviewReportState { + let report: Record | null = null; + let claimedIssues = false; + for (const step of steps) { + for (const toolCall of step.toolCalls ?? []) { + if (toolCall.toolName !== REPORT_REVIEW_TOOL_NAME) continue; + if (!isRecord(toolCall.input)) continue; + if (toolCall.input.hasIssues === true) claimedIssues = true; + if (toolCall.invalid !== true) report = toolCall.input; + } + } + if (!report) return { kind: "missing" }; + return claimedIssues && clearsReviewWithoutFindings(report) + ? { kind: "dropped_findings", report } + : { kind: "filed", report }; +} diff --git a/tests/unit/helpers/automation-job-fixtures.ts b/tests/unit/helpers/automation-job-fixtures.ts index 5cd38c7d..05a49bef 100644 --- a/tests/unit/helpers/automation-job-fixtures.ts +++ b/tests/unit/helpers/automation-job-fixtures.ts @@ -86,7 +86,7 @@ export function makeStep(input: { text?: string; inputTokens?: number; outputTokens?: number; - toolCalls?: Array<{ toolName: string; input: unknown }>; + toolCalls?: Array<{ toolName: string; input: unknown; invalid?: boolean }>; toolResults?: unknown[]; }) { return { diff --git a/tests/unit/pr-review-report-repair.test.ts b/tests/unit/pr-review-report-repair.test.ts index e101e00f..970d6b49 100644 --- a/tests/unit/pr-review-report-repair.test.ts +++ b/tests/unit/pr-review-report-repair.test.ts @@ -243,3 +243,94 @@ test("a rejected forced report whose unforced ask also fails stays without a ver assert.equal(result.text, FORGOT_TO_REPORT.text); assert.equal(verdictMissing, true); }); + +const CLAIM = "One warning and three suggestions, detailed in the comment."; + +// Webrenew/vmotif#1647: every report claimed issues without listing them, so +// the SDK rejected each one and nothing was ever filed. +const EVERY_REPORT_REJECTED = { + text: "Reviewed PR 42.", + steps: [ + makeStep({ + inputTokens: 900, + outputTokens: 40, + toolCalls: [ + { + toolName: "reportReview", + input: { hasIssues: true, summary: CLAIM }, + invalid: true, + }, + ], + }), + ], + totalUsage: { inputTokens: 900, outputTokens: 40 }, +}; + +// Mogplex/mogplex#535: after the rejections, the reviewer flipped hasIssues +// to get a report accepted, and the findings were never listed anywhere. +const DROPPED_FINDINGS = { + ...EVERY_REPORT_REJECTED, + steps: [ + ...EVERY_REPORT_REJECTED.steps, + makeStep({ + toolCalls: [ + { + toolName: "reportReview", + input: { hasIssues: false, summary: CLAIM }, + }, + ], + }), + ], +}; + +test("a reviewer whose every report was rejected is asked for its report", async () => { + const { calls, harness, verdictMissing } = await runReview([ + EVERY_REPORT_REJECTED, + FILED_REPORT, + ]); + + assert.equal(calls.length, 2); + assert.match(String(calls[1].messages?.[2].content), /Call reportReview now/); + assert.equal(harness.source, "structured"); + assert.equal(verdictMissing, false); +}); + +test("a reviewer that dropped the findings it claimed is asked to list them", async () => { + const { calls, harness, verdictMissing } = await runReview([ + DROPPED_FINDINGS, + FILED_REPORT, + ]); + + assert.equal(calls.length, 2); + assert.match( + String(calls[1].messages?.[2].content), + /no report you filed lists them/ + ); + assert.equal(harness.source, "structured"); + assert.equal(verdictMissing, false); + assert.equal(harness.reviewOutcome.findings.length, 1); +}); + +test("a reviewer that clears the review again without listing anything stays without a verdict", async () => { + const clearedAgain = { + text: "", + steps: [ + makeStep({ + toolCalls: [ + { + toolName: "reportReview", + input: { hasIssues: false, summary: CLAIM }, + }, + ], + }), + ], + }; + const { calls, harness, verdictMissing } = await runReview([ + DROPPED_FINDINGS, + clearedAgain, + ]); + + assert.equal(calls.length, 2); + assert.equal(harness.source, "dropped_findings"); + assert.equal(verdictMissing, true); +}); diff --git a/tests/unit/pr-reviewer-dropped-findings.test.ts b/tests/unit/pr-reviewer-dropped-findings.test.ts new file mode 100644 index 00000000..ca3da3d7 --- /dev/null +++ b/tests/unit/pr-reviewer-dropped-findings.test.ts @@ -0,0 +1,102 @@ +import assert from "node:assert/strict"; +import test from "node:test"; +import type { z } from "zod"; + +type ReviewTools = ReturnType< + Awaited["buildPRReviewTools"] +>; +type Executable = { + inputSchema: z.ZodType; + execute: (input: unknown) => Promise>; +}; + +const CLAIM = "One warning and three suggestions, none blocking."; + +async function buildTools(fetch: typeof globalThis.fetch) { + const { buildPRReviewTools } = await import("../../lib/agents/pr-reviewer"); + return buildPRReviewTools({ + fetch, + githubToken: "github-token", + owner: "acme", + repo: "widgets", + prNumber: 42, + allowPrLifecycle: true, + }); +} + +function toolOf(tools: ReviewTools, name: string) { + return (tools as unknown as Record)[name]; +} + +/** What the SDK does with a report: validate, then run it only if valid. */ +async function fileReport(tools: ReviewTools, input: Record) { + const report = toolOf(tools, "reportReview"); + const parsed = report.inputSchema.safeParse(input); + if (parsed.success) await report.execute(parsed.data); + return parsed.success; +} + +/** GitHub reports the PR closed, so a merge attempt stops after one read. */ +function recordGithubRequests() { + const urls: string[] = []; + const fetch = (async (input: RequestInfo | URL) => { + urls.push(String(input)); + return new Response(JSON.stringify({ state: "closed" }), { + status: 200, + headers: { "Content-Type": "application/json" }, + }); + }) as typeof globalThis.fetch; + return { urls, fetch }; +} + +for (const name of ["mergePullRequest", "queuePullRequestForMerge"]) { + test(`${name} refuses a review that dropped the issues it claimed`, async () => { + const github = recordGithubRequests(); + const tools = await buildTools(github.fetch); + assert.equal( + await fileReport(tools, { hasIssues: true, summary: CLAIM }), + false + ); + assert.equal( + await fileReport(tools, { hasIssues: false, summary: CLAIM }), + true + ); + + const outcome = await toolOf(tools, name).execute({}); + + assert.equal(outcome.success, false); + assert.match(String(outcome.error), /lists none/); + assert.deepEqual(github.urls, []); + }); + + test(`${name} proceeds when the clean report lists its suggestions`, async () => { + const github = recordGithubRequests(); + const tools = await buildTools(github.fetch); + await fileReport(tools, { hasIssues: true, summary: CLAIM }); + await fileReport(tools, { + hasIssues: false, + summary: CLAIM, + findings: [ + { severity: "suggestion", title: "Name the budget", body: "Magic 3." }, + ], + }); + + await toolOf(tools, name).execute({}); + + assert.ok(github.urls.some((url) => url.endsWith("/pulls/42"))); + }); +} + +test("the rejection tells the reviewer to list the findings, not to clear the review", async () => { + const tools = await buildTools(recordGithubRequests().fetch); + const parsed = toolOf(tools, "reportReview").inputSchema.safeParse({ + hasIssues: true, + summary: CLAIM, + }); + + assert.equal(parsed.success, false); + assert.match( + String(parsed.error), + /Setting hasIssues=false without listing the issues leaves the review without a verdict/ + ); +}); From 9968598c54f72386627ffffce1f73ebc417ea55b Mon Sep 17 00:00:00 2001 From: Charles Howard <96023061+charlesrhoward@users.noreply.github.com> Date: Tue, 29 Sep 2026 10:21:52 -0400 Subject: [PATCH 2/7] refactor(pr-review): keep report-state helpers under the complexity cap --- lib/workflows/pr-review-harness-extraction.ts | 35 ++++++++++++------- lib/workflows/pr-review-report-state.ts | 23 ++++++------ 2 files changed, 36 insertions(+), 22 deletions(-) diff --git a/lib/workflows/pr-review-harness-extraction.ts b/lib/workflows/pr-review-harness-extraction.ts index 4570e849..44ea5f0c 100644 --- a/lib/workflows/pr-review-harness-extraction.ts +++ b/lib/workflows/pr-review-harness-extraction.ts @@ -10,7 +10,10 @@ import { toReviewFindings, toStringArray, } from "./pr-review-harness-utils"; -import { readReviewReportState } from "./pr-review-report-state"; +import { + readReviewReportState, + type ReviewReportState, +} from "./pr-review-report-state"; function extractLastToolInput( result: AutomationAgentReviewResult, @@ -168,24 +171,32 @@ export function isPrReviewVerdictMissing( ); } +function buildReportedHarnessResult( + result: AutomationAgentReviewResult, + reportState: Exclude +): PrReviewHarnessResult { + const { report } = reportState; + return { + source: reportState.kind === "filed" ? "structured" : "dropped_findings", + fallbackText: toOptionalString(result.text), + reviewOutcome: { + hasIssues: report.hasIssues === true, + summary: toOptionalString(report.summary) ?? result.text ?? "", + commentBody: toOptionalString(report.commentBody), + affectedFiles: toStringArray(report.affectedFiles), + findings: toReviewFindings(report.findings), + }, + }; +} + export function extractPrReviewHarnessResult( result: AutomationAgentReviewResult ): PrReviewHarnessResult { const reportState = readReviewReportState(result.steps); const autofix = extractPrAutofixOutcome(result); if (reportState.kind !== "missing") { - const { report } = reportState; return { - source: - reportState.kind === "filed" ? "structured" : "dropped_findings", - fallbackText: toOptionalString(result.text), - reviewOutcome: { - hasIssues: report.hasIssues === true, - summary: toOptionalString(report.summary) ?? result.text ?? "", - commentBody: toOptionalString(report.commentBody), - affectedFiles: toStringArray(report.affectedFiles), - findings: toReviewFindings(report.findings), - }, + ...buildReportedHarnessResult(result, reportState), ...(autofix ? { autofix } : {}), }; } diff --git a/lib/workflows/pr-review-report-state.ts b/lib/workflows/pr-review-report-state.ts index 8ffc8098..b44790d1 100644 --- a/lib/workflows/pr-review-report-state.ts +++ b/lib/workflows/pr-review-report-state.ts @@ -30,18 +30,21 @@ export function clearsReviewWithoutFindings( export function readReviewReportState( steps: AutomationAgentReviewResult["steps"] ): ReviewReportState { - let report: Record | null = null; - let claimedIssues = false; - for (const step of steps) { - for (const toolCall of step.toolCalls ?? []) { - if (toolCall.toolName !== REPORT_REVIEW_TOOL_NAME) continue; - if (!isRecord(toolCall.input)) continue; - if (toolCall.input.hasIssues === true) claimedIssues = true; - if (toolCall.invalid !== true) report = toolCall.input; - } - } + const calls = reportCalls(steps); + const claimedIssues = calls.some((call) => call.input.hasIssues === true); + const report = calls.findLast((call) => !call.invalid)?.input; if (!report) return { kind: "missing" }; return claimedIssues && clearsReviewWithoutFindings(report) ? { kind: "dropped_findings", report } : { kind: "filed", report }; } + +function reportCalls(steps: AutomationAgentReviewResult["steps"]) { + return steps.flatMap((step) => + (step.toolCalls ?? []).flatMap((toolCall) => + toolCall.toolName === REPORT_REVIEW_TOOL_NAME && isRecord(toolCall.input) + ? [{ input: toolCall.input, invalid: toolCall.invalid === true }] + : [] + ) + ); +} From 3006956fc2aa2ca091b5c23bdf437696bf104006 Mon Sep 17 00:00:00 2001 From: Charles Howard <96023061+charlesrhoward@users.noreply.github.com> Date: Tue, 29 Sep 2026 10:38:30 -0400 Subject: [PATCH 3/7] fix(pr-review): refuse in-run merges when every issue report was rejected --- lib/agents/pr-reviewer.ts | 14 +++++----- lib/workflows/pr-review-report-state.ts | 13 +++++----- tests/unit/pr-review-report-repair.test.ts | 26 +++++++++++++++++++ .../unit/pr-reviewer-dropped-findings.test.ts | 23 ++++++++++++++++ 4 files changed, 64 insertions(+), 12 deletions(-) diff --git a/lib/agents/pr-reviewer.ts b/lib/agents/pr-reviewer.ts index a95a6aa9..e2d034fa 100644 --- a/lib/agents/pr-reviewer.ts +++ b/lib/agents/pr-reviewer.ts @@ -86,8 +86,10 @@ export function buildPRReviewTools(config: { findings: z.array(reviewFindingSchema).max(20).optional(), }) .superRefine((value, ctx) => { - // Runs for every report, including ones the schema rejects, which - // never reach `execute`. + // Runs for reports the schema rejects here, which never reach + // `execute`. Zod skips it for input missing a required field, so a + // malformed claim goes unseen; the published verdict does not depend + // on this, because readReviewReportState reads the raw tool calls. if (value.hasIssues) claimedIssues = true; if (value.hasIssues && (!value.findings || value.findings.length === 0)) { ctx.addIssue({ @@ -99,12 +101,12 @@ export function buildPRReviewTools(config: { } }); - // A report that clears the review after an earlier one claimed issues has - // dropped them, so it cannot authorize a merge. + // Once a report has claimed issues, only an accepted report that lists + // findings can authorize a merge: none accepted means every claim was + // rejected, and one that clears the review has dropped them. const droppedFindingsRefusal = () => claimedIssues && - acceptedReport !== null && - clearsReviewWithoutFindings(acceptedReport) + (acceptedReport === null || clearsReviewWithoutFindings(acceptedReport)) ? { success: false as const, error: diff --git a/lib/workflows/pr-review-report-state.ts b/lib/workflows/pr-review-report-state.ts index b44790d1..82a9394a 100644 --- a/lib/workflows/pr-review-report-state.ts +++ b/lib/workflows/pr-review-report-state.ts @@ -20,12 +20,13 @@ export function clearsReviewWithoutFindings( /** * The reviewer's report as the pipeline should trust it. Only calls the SDK * accepted count as filed; a call whose input failed the schema was never - * recorded. A report that clears the review after an earlier call said there - * were issues has lost them: reviewers told that hasIssues=true needs - * findings sometimes flip hasIssues to false instead of listing them, and - * that report would otherwise publish as a clean verdict with its warnings - * gone. An earlier report that listed findings and a later one that clears - * them is treated the same way, since nothing says the findings were wrong. + * recorded. A report that clears the review when any call in the run, + * accepted or rejected, before or after it, said there were issues has lost + * them: reviewers told that hasIssues=true needs findings sometimes flip + * hasIssues to false instead of listing them, and that report would + * otherwise publish as a clean verdict with its warnings gone. A report that + * listed findings followed by one that clears them is treated the same way, + * since nothing says the findings were wrong. */ export function readReviewReportState( steps: AutomationAgentReviewResult["steps"] diff --git a/tests/unit/pr-review-report-repair.test.ts b/tests/unit/pr-review-report-repair.test.ts index 970d6b49..9b52a64f 100644 --- a/tests/unit/pr-review-report-repair.test.ts +++ b/tests/unit/pr-review-report-repair.test.ts @@ -334,3 +334,29 @@ test("a reviewer that clears the review again without listing anything stays wit assert.equal(harness.source, "dropped_findings"); assert.equal(verdictMissing, true); }); + +test("a repair whose own report is rejected leaves the review without a verdict and is not asked again", async () => { + const rejectedRepair = { + text: "", + steps: [ + makeStep({ + toolCalls: [ + { + toolName: "reportReview", + input: { hasIssues: true, summary: CLAIM }, + invalid: true, + }, + ], + }), + ], + }; + const { calls, harness, verdictMissing } = await runReview([ + DROPPED_FINDINGS, + rejectedRepair, + FILED_REPORT, + ]); + + assert.equal(calls.length, 2); + assert.equal(harness.source, "dropped_findings"); + assert.equal(verdictMissing, true); +}); diff --git a/tests/unit/pr-reviewer-dropped-findings.test.ts b/tests/unit/pr-reviewer-dropped-findings.test.ts index ca3da3d7..0070b87d 100644 --- a/tests/unit/pr-reviewer-dropped-findings.test.ts +++ b/tests/unit/pr-reviewer-dropped-findings.test.ts @@ -69,6 +69,29 @@ for (const name of ["mergePullRequest", "queuePullRequestForMerge"]) { assert.deepEqual(github.urls, []); }); + test(`${name} refuses a review whose every issue report was rejected`, async () => { + const github = recordGithubRequests(); + const tools = await buildTools(github.fetch); + assert.equal( + await fileReport(tools, { hasIssues: true, summary: CLAIM }), + false + ); + + const outcome = await toolOf(tools, name).execute({}); + + assert.equal(outcome.success, false); + assert.deepEqual(github.urls, []); + }); + + test(`${name} proceeds before any report when nothing claimed issues`, async () => { + const github = recordGithubRequests(); + const tools = await buildTools(github.fetch); + + await toolOf(tools, name).execute({}); + + assert.ok(github.urls.some((url) => url.endsWith("/pulls/42"))); + }); + test(`${name} proceeds when the clean report lists its suggestions`, async () => { const github = recordGithubRequests(); const tools = await buildTools(github.fetch); From b41a436741c45c29f88aeb97f586916bc779c33a Mon Sep 17 00:00:00 2001 From: Charles Howard <96023061+charlesrhoward@users.noreply.github.com> Date: Tue, 29 Sep 2026 13:58:31 -0400 Subject: [PATCH 4/7] fix(pr-review): read a flow's verdict from its last review node, not the merged transcript --- lib/workflows/automation-job-agent-node.ts | 4 +- lib/workflows/automation-job-executor-core.ts | 1 + lib/workflows/automation-job-flow-executor.ts | 2 + .../automation-job-flow-run-state.ts | 14 +- lib/workflows/automation-job-flow-success.ts | 5 +- ...ation-job-workflow-review-rereview.test.ts | 335 ++++++++++++++++++ 6 files changed, 358 insertions(+), 3 deletions(-) create mode 100644 tests/unit/automation-job-workflow-review-rereview.test.ts diff --git a/lib/workflows/automation-job-agent-node.ts b/lib/workflows/automation-job-agent-node.ts index ab78948b..898d6c9e 100644 --- a/lib/workflows/automation-job-agent-node.ts +++ b/lib/workflows/automation-job-agent-node.ts @@ -476,7 +476,9 @@ export async function executeFlowAgentNode( const handoffText = JSON.stringify(handoff); state.outputs.set(node.id, { label, text: handoffText, handoff }); - state.results.push({ ...result, text: handoff.summary }); + const nodeResult = { ...result, text: handoff.summary }; + state.results.push(nodeResult); + if (nodeRole === "review") state.reviewResult = nodeResult; return { ok: true as const, diff --git a/lib/workflows/automation-job-executor-core.ts b/lib/workflows/automation-job-executor-core.ts index 1ef7cc7c..f2d65add 100644 --- a/lib/workflows/automation-job-executor-core.ts +++ b/lib/workflows/automation-job-executor-core.ts @@ -332,6 +332,7 @@ export async function runAutomationJob( releasedScope: input.releasedScope, context, finalResult: flowExecution.result, + reviewResult: flowExecution.reviewResult ?? null, autoMergeRequest: flowExecution.autoMergeRequest ?? null, observabilityError: flowExecution.observabilityError, isPrReview, diff --git a/lib/workflows/automation-job-flow-executor.ts b/lib/workflows/automation-job-flow-executor.ts index 72cdcabd..c8d504c6 100644 --- a/lib/workflows/automation-job-flow-executor.ts +++ b/lib/workflows/automation-job-flow-executor.ts @@ -143,6 +143,7 @@ export type FlowExecutorDeps = { export type FlowExecutionSuccess = { success: true; result: AutomationAgentResult; + reviewResult: AutomationAgentResult | null; autoMergeRequest: FlowAutoMergeRequest | null; observabilityError: string | null; }; @@ -486,6 +487,7 @@ export async function executeResolvedFlow(input: { result: mergeAutomationAgentResults( state.results as AutomationAgentResult[] ), + reviewResult: state.reviewResult, autoMergeRequest: state.autoMergeRequest, observabilityError: state.observabilityError, }; diff --git a/lib/workflows/automation-job-flow-run-state.ts b/lib/workflows/automation-job-flow-run-state.ts index 97f650f1..8b961187 100644 --- a/lib/workflows/automation-job-flow-run-state.ts +++ b/lib/workflows/automation-job-flow-run-state.ts @@ -8,7 +8,10 @@ */ import type { FlowGraph } from "@/lib/types"; -import type { FlowExecutionToken } from "@/lib/workflows/automation-job-types"; +import type { + AutomationAgentResult, + FlowExecutionToken, +} from "@/lib/workflows/automation-job-types"; import { FAILURE_HANDLE_ID, getOutgoingEdges } from "@/lib/flows/graph"; import type { FlowReportHandoff } from "./flow-report-handoff"; @@ -49,6 +52,14 @@ export type FlowRunState = { /** Aggregated agent results */ results: unknown[]; + /** + * The last review node's own result. The flow's verdict comes from it, not + * from the merged transcript: a later review node re-reviews what an + * earlier one reported, and its clean report must not read as one that + * dropped the earlier node's findings. + */ + reviewResult: AutomationAgentResult | null; + /** First observability error encountered */ observabilityError: string | null; @@ -86,6 +97,7 @@ export function createFlowRunState(input: { incomingCounts, processed: new Set(), results: [], + reviewResult: null, observabilityError: null, autoMergeRequest: null, expectedTriggerHeadSha: input.initialHeadSha, diff --git a/lib/workflows/automation-job-flow-success.ts b/lib/workflows/automation-job-flow-success.ts index 530137bf..72d2975c 100644 --- a/lib/workflows/automation-job-flow-success.ts +++ b/lib/workflows/automation-job-flow-success.ts @@ -44,6 +44,8 @@ export type FlowSuccessFinalizationInput = { releasedScope: ReleasedAutomationScope; context: JobContext; finalResult: AutomationAgentResult; + /** The last review node's own result; the verdict is read from it. */ + reviewResult?: AutomationAgentResult | null; autoMergeRequest: FlowAutoMergeRequest | null; observabilityError?: string | null; isPrReview: boolean; @@ -83,6 +85,7 @@ export async function finalizeFlowSuccess( releasedScope, context, finalResult, + reviewResult, autoMergeRequest, observabilityError, isPrReview, @@ -94,7 +97,7 @@ export async function finalizeFlowSuccess( } = input; const reviewHarnessResult = isPrReview - ? extractPrReviewHarnessResult(finalResult) + ? extractPrReviewHarnessResult(reviewResult ?? finalResult) : null; const reviewOutcome = reviewHarnessResult?.reviewOutcome ?? null; const reviewCommentPosted = isPrReview diff --git a/tests/unit/automation-job-workflow-review-rereview.test.ts b/tests/unit/automation-job-workflow-review-rereview.test.ts new file mode 100644 index 00000000..04e9b554 --- /dev/null +++ b/tests/unit/automation-job-workflow-review-rereview.test.ts @@ -0,0 +1,335 @@ +import assert from "node:assert/strict"; +import test from "node:test"; +import { + type CapturedControlDispatchEvent, + loadAutomationJobWorkflowModule, + makeStep, + makePersistedReviewFindingsResult, +} from "./helpers/automation-job-fixtures"; + +// A review -> review flow: the second node re-reviews the PR the first one +// reported on. Its clean report is the flow's verdict; read against the +// merged transcript it would look like a report that dropped the first +// node's findings, and the merge would be blocked as incomplete. +test("a flow's clean re-review is its verdict even after an earlier review node reported issues", async () => { + const { createAutomationJobTask } = await loadAutomationJobWorkflowModule(); + + const originalFetch = globalThis.fetch; + // Ordered trace of the calls that matter: in repos that require the Mogplex + // PR Review check, merging before completePrReviewCheckRun can never see + // mergeable_state "clean", so the merge gate must run strictly after it. + const events: string[] = []; + let nodeRunSequence = 0; + let reviewCount = 0; + let controlDispatchEvent: CapturedControlDispatchEvent | null = null; + + const workflow = createAutomationJobTask({ + // The pre-flight PR liveness check is orthogonal to this ordering test and + // would otherwise hit the mocked PR endpoint ahead of the merge gate. + fetchPrLiveness: async () => ({ alive: true }), + resolveJobContext: async () => ({ + context: { + metadata: { + pr_number: 143, + head_sha: "beaded42", + head_ref: "feat/auto-merge-ordering", + base_ref: "main", + head_repo_full_name: "acme/widgets", + base_repo_full_name: "acme/widgets", + }, + assignmentType: "pr_review", + skillId: null, + agent: { + model: "openai/gpt-5.4", + system_prompt: null, + }, + repo: { + id: "repo-123", + user_id: "user-123", + full_name: "acme/widgets", + default_branch: "main", + github_installation_id: 123, + }, + }, + flow: { + flowId: "flow-143", + flowVersionId: "flow-version-143", + graph: { + nodes: [ + { + id: "start-1", + type: "start", + position: { x: 0, y: 0 }, + data: { + label: "Start", + event: "pr_opened", + isDefault: false, + }, + }, + { + id: "agent-1", + type: "agent", + position: { x: 240, y: 0 }, + data: { + label: "PR Review Agent", + agentId: "agent-base-1", + role: "review", + autofix: false, + autoMerge: false, + modelOverride: "openai/gpt-5.4", + }, + }, + { + id: "agent-2", + type: "agent", + position: { x: 360, y: 0 }, + data: { + label: "PR Re-review Agent", + agentId: "agent-base-1", + role: "review", + autofix: false, + autoMerge: false, + modelOverride: "openai/gpt-5.4", + }, + }, + { + id: "merge-1", + type: "action", + position: { x: 480, y: 0 }, + data: { + label: "Merge when safe", + operation: "github.merge_pull_request", + pullRequestNumber: null, + commitTitle: "Workflow merge", + }, + }, + { + id: "end-1", + type: "end", + position: { x: 720, y: 0 }, + data: { + label: "End", + }, + }, + ], + edges: [ + { + id: "edge-start-agent", + source: "start-1", + target: "agent-1", + }, + { + id: "edge-agent-agent", + source: "agent-1", + target: "agent-2", + }, + { + id: "edge-agent-merge", + source: "agent-2", + target: "merge-1", + }, + { + id: "edge-merge-end", + source: "merge-1", + target: "end-1", + }, + ], + }, + agentsById: new Map([ + [ + "agent-base-1", + { + id: "agent-base-1", + name: "PR Review Agent", + slug: "pr-review-agent", + model: "openai/gpt-5.4", + system_prompt: null, + timeout_ms: null, + }, + ], + ]), + }, + }), + resolveGithubToken: async () => "github-token", + resolveAutomationModel: async () => ({ + model: "openai/gpt-5.4", + effectiveModelId: "openai/gpt-5.4", + }), + runAutomationAgent: async () => { + reviewCount += 1; + const input = + reviewCount === 1 + ? { + hasIssues: true, + summary: "The retry path skips the check.", + findings: [ + { + severity: "warning", + title: "Retry path skips the check", + body: "A replayed command is returned unchecked.", + path: "src/retry.ts", + }, + ], + } + : { + hasIssues: false, + summary: "No material issues found.", + findings: [], + }; + return { + text: input.summary, + usage: { inputTokens: 9, outputTokens: 5 }, + steps: [ + makeStep({ + toolCalls: [{ toolName: "reportReview", input }], + toolResults: [input], + }), + ], + }; + }, + loadPullRequestDetails: async () => ({ + number: 143, + title: "feat: auto-merge ordering", + body: null, + headRef: "feat/auto-merge-ordering", + headSha: "beaded42", + headRepoFullName: "acme/widgets", + baseRef: "main", + baseSha: null, + baseRepoFullName: "acme/widgets", + }), + createPrReviewCheckRun: async () => ({ + id: 143, + htmlUrl: "https://github.com/acme/widgets/runs/143", + }), + completePrReviewCheckRun: async (input) => { + events.push("check_run_completed"); + return { + id: input.checkRunId, + htmlUrl: "https://github.com/acme/widgets/runs/143", + }; + }, + upsertPrReviewTimelineComment: async () => ({ + id: 5143, + htmlUrl: "https://github.com/acme/widgets/pull/143#issuecomment-5143", + created: true, + }), + clearPrReviewTimelineComment: async () => ({ + deleted: false, + id: null, + htmlUrl: null, + }), + persistJobSuccess: async () => true, + persistJobReviewFindings: async () => makePersistedReviewFindingsResult(0), + getDurationMs: async () => 444, + tryLogAiCall: async () => null, + recordControlDispatchEvent: async (input) => { + controlDispatchEvent = { + outcome: input.outcome, + reason: input.reason, + metadata: input.metadata, + }; + }, + releaseQueuedJobs: async () => [], + isJobRunCancellationRequested: async () => false, + throwIfJobRunCancelled: async () => {}, + persistJobFailure: async () => { + throw new Error("persistJobFailure should not be called"); + }, + }); + + globalThis.fetch = async (input, init) => { + const requestUrl = + typeof input === "string" + ? input + : input instanceof URL + ? input.toString() + : input.url; + const method = (init?.method ?? "GET").toUpperCase(); + + if ( + requestUrl === "https://api.github.com/repos/acme/widgets/pulls/143" && + method === "GET" + ) { + events.push("merge_gate_read"); + return Response.json({ + state: "open", + draft: false, + mergeable: true, + mergeable_state: "clean", + head: { sha: "beaded42" }, + }); + } + + if ( + requestUrl === + "https://api.github.com/repos/acme/widgets/pulls/143/merge" && + method === "PUT" + ) { + events.push("merge_put"); + return Response.json({ merged: true, sha: "merge-sha" }); + } + + if (requestUrl.startsWith("https://example.supabase.co")) { + if (requestUrl.includes("/rest/v1/flow_node_runs")) { + if (method === "POST") { + nodeRunSequence += 1; + return Response.json( + { + id: `node-run-${nodeRunSequence}`, + started_at: "2026-07-06T20:00:00.000Z", + }, + { status: 201 } + ); + } + if (method === "PATCH") { + return Response.json({ id: "node-run-updated" }); + } + } + + if (requestUrl.includes("/rest/v1/job_runs")) { + return Response.json( + { + status: "running", + cancel_requested_at: null, + cancelled_at: null, + }, + { status: 200 } + ); + } + } + + throw new Error(`Unhandled fetch in test: ${method} ${requestUrl}`); + }; + + try { + const result = await workflow({ + jobRunId: "job-flow-auto-merge-ordering", + startedAt: "2026-07-06T20:00:00.000Z", + releasedScope: { + sourceKind: "flow", + sourceType: "pr_opened", + sourceId: "flow-143", + repoId: "repo-123", + installationId: 123, + }, + }); + + assert.equal(result.success, true); + assert.equal(reviewCount, 2); + + assert.ok(events.includes("merge_put"), events.join(", ")); + + assert.ok(controlDispatchEvent); + const dispatched = controlDispatchEvent as CapturedControlDispatchEvent; + assert.equal(dispatched.outcome, "completed"); + assert.equal(dispatched.reason, "PR_REVIEW_NO_FINDINGS"); + assert.deepEqual(dispatched.metadata?.auto_merge, { + merged: true, + reason: "Merged after clean review", + sha: "merge-sha", + }); + } finally { + globalThis.fetch = originalFetch; + } +}); From 532505151e362509614d1154a77dc3a918f6d107 Mon Sep 17 00:00:00 2001 From: Charles Howard <96023061+charlesrhoward@users.noreply.github.com> Date: Tue, 29 Sep 2026 14:00:15 -0400 Subject: [PATCH 5/7] fix(pr-review): count issue claims the report schema rejects for any reason --- lib/agents/pr-reviewer.ts | 16 +++++--- lib/workflows/pr-review-report-state.ts | 6 ++- .../unit/pr-reviewer-dropped-findings.test.ts | 38 +++++++++++++++++++ 3 files changed, 53 insertions(+), 7 deletions(-) diff --git a/lib/agents/pr-reviewer.ts b/lib/agents/pr-reviewer.ts index e2d034fa..a712d337 100644 --- a/lib/agents/pr-reviewer.ts +++ b/lib/agents/pr-reviewer.ts @@ -12,6 +12,7 @@ import { prepareGitHubTextFile, type GitHubFileContent, } from "@/lib/agents/github-file-content"; +import { isRecord } from "@/lib/workflows/pr-review-harness-utils"; import { clearsReviewWithoutFindings } from "@/lib/workflows/pr-review-report-state"; const PR_REVIEW_FILE_CONTENT_CHAR_LIMIT = GITHUB_FILE_CONTENT_CHAR_LIMIT; @@ -68,7 +69,7 @@ export function buildPRReviewTools(config: { path: z.string().optional(), line: z.number().int().positive().optional(), }); - const reportReviewInputSchema = z + const reportObjectSchema = z .object({ hasIssues: z.boolean(), summary: z @@ -86,11 +87,6 @@ export function buildPRReviewTools(config: { findings: z.array(reviewFindingSchema).max(20).optional(), }) .superRefine((value, ctx) => { - // Runs for reports the schema rejects here, which never reach - // `execute`. Zod skips it for input missing a required field, so a - // malformed claim goes unseen; the published verdict does not depend - // on this, because readReviewReportState reads the raw tool calls. - if (value.hasIssues) claimedIssues = true; if (value.hasIssues && (!value.findings || value.findings.length === 0)) { ctx.addIssue({ code: z.ZodIssueCode.custom, @@ -100,6 +96,14 @@ export function buildPRReviewTools(config: { }); } }); + // The claim is read before validation, so a report the schema rejects for + // any reason (a missing summary, an unknown severity) still counts; such + // reports never reach `execute`. The JSON schema the model sees is the + // object schema's own. + const reportReviewInputSchema = z.preprocess((input) => { + if (isRecord(input) && input.hasIssues === true) claimedIssues = true; + return input; + }, reportObjectSchema); // Once a report has claimed issues, only an accepted report that lists // findings can authorize a merge: none accepted means every claim was diff --git a/lib/workflows/pr-review-report-state.ts b/lib/workflows/pr-review-report-state.ts index 82a9394a..d7a1ec9e 100644 --- a/lib/workflows/pr-review-report-state.ts +++ b/lib/workflows/pr-review-report-state.ts @@ -26,7 +26,11 @@ export function clearsReviewWithoutFindings( * hasIssues to false instead of listing them, and that report would * otherwise publish as a clean verdict with its warnings gone. A report that * listed findings followed by one that clears them is treated the same way, - * since nothing says the findings were wrong. + * since nothing says the findings were wrong. The rule is about lost + * findings, not the verdict: an accepted report that lists findings (say, + * hasIssues=false with suggestions) has lost nothing, so a claim before or + * after it leaves it filed. Steps are one review session's, including its + * follow-ups; a flow's later review node is judged on its own steps. */ export function readReviewReportState( steps: AutomationAgentReviewResult["steps"] diff --git a/tests/unit/pr-reviewer-dropped-findings.test.ts b/tests/unit/pr-reviewer-dropped-findings.test.ts index 0070b87d..87b5271c 100644 --- a/tests/unit/pr-reviewer-dropped-findings.test.ts +++ b/tests/unit/pr-reviewer-dropped-findings.test.ts @@ -83,6 +83,26 @@ for (const name of ["mergePullRequest", "queuePullRequestForMerge"]) { assert.deepEqual(github.urls, []); }); + test(`${name} refuses after an issue report the base schema rejected`, async () => { + const github = recordGithubRequests(); + const tools = await buildTools(github.fetch); + const malformed = { + hasIssues: true, + summary: CLAIM, + findings: [{ severity: "blocker", title: "Retry", body: "Unchecked." }], + }; + assert.equal(await fileReport(tools, malformed), false); + assert.equal( + await fileReport(tools, { hasIssues: false, summary: CLAIM }), + true + ); + + const outcome = await toolOf(tools, name).execute({}); + + assert.equal(outcome.success, false); + assert.deepEqual(github.urls, []); + }); + test(`${name} proceeds before any report when nothing claimed issues`, async () => { const github = recordGithubRequests(); const tools = await buildTools(github.fetch); @@ -123,3 +143,21 @@ test("the rejection tells the reviewer to list the findings, not to clear the re /Setting hasIssues=false without listing the issues leaves the review without a verdict/ ); }); + +test("the report schema the model sees still requires a summary and names every severity", async () => { + const { asSchema } = await import("ai"); + const tools = await buildTools(recordGithubRequests().fetch); + const schema = (await asSchema(toolOf(tools, "reportReview").inputSchema) + .jsonSchema) as { + required?: string[]; + properties: { + findings: { items: { properties: Record } }; + }; + }; + + assert.deepEqual(schema.required, ["hasIssues", "summary"]); + assert.deepEqual(schema.properties.findings.items.properties.severity, { + type: "string", + enum: ["critical", "warning", "suggestion"], + }); +}); From a857ac8a73ef8c7a55a228d740abf1912a593848 Mon Sep 17 00:00:00 2001 From: Charles Howard <96023061+charlesrhoward@users.noreply.github.com> Date: Tue, 29 Sep 2026 14:06:35 -0400 Subject: [PATCH 6/7] fix(pr-review): keep a flow's autofix outcome beside its review node's verdict --- lib/workflows/automation-job-flow-success.ts | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/lib/workflows/automation-job-flow-success.ts b/lib/workflows/automation-job-flow-success.ts index 72d2975c..a87a7570 100644 --- a/lib/workflows/automation-job-flow-success.ts +++ b/lib/workflows/automation-job-flow-success.ts @@ -71,6 +71,23 @@ export type FlowSuccessFinalizationInput = { ) => Promise; }; +/** + * The verdict is the last review node's own; what the flow's fix nodes + * applied comes from the whole run. + */ +function extractFlowReviewResult( + finalResult: AutomationAgentResult, + reviewResult: AutomationAgentResult | null +) { + const flow = extractPrReviewHarnessResult(finalResult); + if (!reviewResult) return flow; + const { autofix } = flow; + return { + ...extractPrReviewHarnessResult(reviewResult), + ...(autofix ? { autofix } : {}), + }; +} + /** * Finalize a successful flow execution. * @@ -97,7 +114,7 @@ export async function finalizeFlowSuccess( } = input; const reviewHarnessResult = isPrReview - ? extractPrReviewHarnessResult(reviewResult ?? finalResult) + ? extractFlowReviewResult(finalResult, reviewResult ?? null) : null; const reviewOutcome = reviewHarnessResult?.reviewOutcome ?? null; const reviewCommentPosted = isPrReview From 974cd9c69fafded2ce52e9fd5b4bfebe1ad80cf9 Mon Sep 17 00:00:00 2001 From: Charles Howard <96023061+charlesrhoward@users.noreply.github.com> Date: Tue, 29 Sep 2026 14:15:07 -0400 Subject: [PATCH 7/7] refactor(pr-review): keep flow functions at their existing complexity --- lib/workflows/automation-job-agent-node.ts | 5 ++--- lib/workflows/automation-job-executor-core.ts | 2 +- lib/workflows/automation-job-flow-run-state.ts | 10 ++++++++++ lib/workflows/automation-job-flow-success.ts | 4 ++-- 4 files changed, 15 insertions(+), 6 deletions(-) diff --git a/lib/workflows/automation-job-agent-node.ts b/lib/workflows/automation-job-agent-node.ts index 898d6c9e..f5ec0b38 100644 --- a/lib/workflows/automation-job-agent-node.ts +++ b/lib/workflows/automation-job-agent-node.ts @@ -38,6 +38,7 @@ import { resolveAutomationModel } from "@/lib/workflows/automation-job-model-res import { emitToOutgoing, noteObservabilityError, + recordFlowNodeResult, } from "@/lib/workflows/automation-job-flow-run-state"; import { executeEditRoleNode, @@ -476,9 +477,7 @@ export async function executeFlowAgentNode( const handoffText = JSON.stringify(handoff); state.outputs.set(node.id, { label, text: handoffText, handoff }); - const nodeResult = { ...result, text: handoff.summary }; - state.results.push(nodeResult); - if (nodeRole === "review") state.reviewResult = nodeResult; + recordFlowNodeResult(state, { ...result, text: handoff.summary }, nodeRole); return { ok: true as const, diff --git a/lib/workflows/automation-job-executor-core.ts b/lib/workflows/automation-job-executor-core.ts index f2d65add..1a56eafa 100644 --- a/lib/workflows/automation-job-executor-core.ts +++ b/lib/workflows/automation-job-executor-core.ts @@ -332,7 +332,7 @@ export async function runAutomationJob( releasedScope: input.releasedScope, context, finalResult: flowExecution.result, - reviewResult: flowExecution.reviewResult ?? null, + reviewResult: flowExecution.reviewResult, autoMergeRequest: flowExecution.autoMergeRequest ?? null, observabilityError: flowExecution.observabilityError, isPrReview, diff --git a/lib/workflows/automation-job-flow-run-state.ts b/lib/workflows/automation-job-flow-run-state.ts index 8b961187..5fe415eb 100644 --- a/lib/workflows/automation-job-flow-run-state.ts +++ b/lib/workflows/automation-job-flow-run-state.ts @@ -74,6 +74,16 @@ export type FlowRunState = { expectedTriggerHeadSha: string | null; }; +/** Adds an agent node's result; a review node's also becomes the verdict. */ +export function recordFlowNodeResult( + state: FlowRunState, + result: AutomationAgentResult, + role: string | null +) { + state.results.push(result); + if (role === "review") state.reviewResult = result; +} + /** * Creates an initialized flow-run state container. */ diff --git a/lib/workflows/automation-job-flow-success.ts b/lib/workflows/automation-job-flow-success.ts index a87a7570..5265ad76 100644 --- a/lib/workflows/automation-job-flow-success.ts +++ b/lib/workflows/automation-job-flow-success.ts @@ -77,7 +77,7 @@ export type FlowSuccessFinalizationInput = { */ function extractFlowReviewResult( finalResult: AutomationAgentResult, - reviewResult: AutomationAgentResult | null + reviewResult: AutomationAgentResult | null | undefined ) { const flow = extractPrReviewHarnessResult(finalResult); if (!reviewResult) return flow; @@ -114,7 +114,7 @@ export async function finalizeFlowSuccess( } = input; const reviewHarnessResult = isPrReview - ? extractFlowReviewResult(finalResult, reviewResult ?? null) + ? extractFlowReviewResult(finalResult, reviewResult) : null; const reviewOutcome = reviewHarnessResult?.reviewOutcome ?? null; const reviewCommentPosted = isPrReview