diff --git a/lib/agents/pr-reviewer.ts b/lib/agents/pr-reviewer.ts index ec2f4be3..a712d337 100644 --- a/lib/agents/pr-reviewer.ts +++ b/lib/agents/pr-reviewer.ts @@ -12,6 +12,8 @@ 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; const PR_REVIEW_PATCH_CHAR_LIMIT = 4_000; @@ -58,6 +60,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(), @@ -65,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 @@ -88,10 +92,31 @@ export function buildPRReviewTools(config: { 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.", }); } }); + // 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 + // rejected, and one that clears the review has dropped them. + 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({ @@ -222,7 +247,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 +264,7 @@ export function buildPRReviewTools(config: { commitTitle: z.string().optional(), }), execute: async ({ commitTitle }) => + droppedFindingsRefusal() ?? mergePullRequestIfSafe({ githubToken: config.githubToken, owner: config.owner, @@ -252,6 +281,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-agent-node.ts b/lib/workflows/automation-job-agent-node.ts index ab78948b..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,7 +477,7 @@ 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 }); + 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 1ef7cc7c..1a56eafa 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, 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..5fe415eb 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; @@ -63,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. */ @@ -86,6 +107,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..5265ad76 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; @@ -69,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 | undefined +) { + const flow = extractPrReviewHarnessResult(finalResult); + if (!reviewResult) return flow; + const { autofix } = flow; + return { + ...extractPrReviewHarnessResult(reviewResult), + ...(autofix ? { autofix } : {}), + }; +} + /** * Finalize a successful flow execution. * @@ -83,6 +102,7 @@ export async function finalizeFlowSuccess( releasedScope, context, finalResult, + reviewResult, autoMergeRequest, observabilityError, isPrReview, @@ -94,7 +114,7 @@ export async function finalizeFlowSuccess( } = input; const reviewHarnessResult = isPrReview - ? extractPrReviewHarnessResult(finalResult) + ? extractFlowReviewResult(finalResult, reviewResult) : null; const reviewOutcome = reviewHarnessResult?.reviewOutcome ?? null; const reviewCommentPosted = isPrReview 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..44ea5f0c 100644 --- a/lib/workflows/pr-review-harness-extraction.ts +++ b/lib/workflows/pr-review-harness-extraction.ts @@ -10,6 +10,10 @@ import { toReviewFindings, toStringArray, } from "./pr-review-harness-utils"; +import { + readReviewReportState, + type ReviewReportState, +} from "./pr-review-report-state"; function extractLastToolInput( result: AutomationAgentReviewResult, @@ -153,8 +157,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 @@ -166,22 +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 report = extractLastToolInput(result, "reportReview"); + const reportState = readReviewReportState(result.steps); const autofix = extractPrAutofixOutcome(result); - if (report) { + if (reportState.kind !== "missing") { return { - source: "structured", - 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-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..d7a1ec9e --- /dev/null +++ b/lib/workflows/pr-review-report-state.ts @@ -0,0 +1,55 @@ +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 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. 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"] +): ReviewReportState { + 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 }] + : [] + ) + ); +} 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; + } +}); 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..9b52a64f 100644 --- a/tests/unit/pr-review-report-repair.test.ts +++ b/tests/unit/pr-review-report-repair.test.ts @@ -243,3 +243,120 @@ 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); +}); + +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 new file mode 100644 index 00000000..87b5271c --- /dev/null +++ b/tests/unit/pr-reviewer-dropped-findings.test.ts @@ -0,0 +1,163 @@ +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} 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} 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); + + 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); + 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/ + ); +}); + +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"], + }); +});