Skip to content
36 changes: 33 additions & 3 deletions lib/agents/pr-reviewer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -58,14 +60,16 @@ export function buildPRReviewTools(config: {
const request = config.fetch ?? fetch;
const contentOwner = config.headOwner ?? config.owner;
const contentRepo = config.headRepo ?? config.repo;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Warning: In-run merge guard blind spot: claims rejected by base schema never set claimedIssues

claimedIssues is set inside reportReviewInputSchema's superRefine (lib/agents/pr-reviewer.ts:L61-L79), which Zod only runs when the base object parses successfully. A reportReview call that fails base validation — missing summary, an invalid severity string like "blocker" in findings, line: 0, or more than 20 findings — never sets claimedIssues, so droppedFindingsRefusal() returns null and mergePullRequest/queuePullRequestForMerge proceed even though the reviewer claimed issues in this run (e.g., malformed-issues report, then clean accepted report, then merge). Post-run publication still detects dropped_findings via readReviewReportState, but the merge will already have executed before the post-run gate can block it — the in-run guard is the only control that acts before the merge.

The in-code comment acknowledges the blind spot ("Zod skips it for input missing a required field... the published verdict does not depend on this"), but that reassurance covers publication only; the merge refusal does depend on it. Observed retry behavior usually re-triggers superRefine and sets the flag, so this needs a single malformed claim followed by a flip to clean to slip through.

Suggestion: make the claim observable before strict validation rejects it — e.g., move required-field and finding-shape enforcement into superRefine itself (with the base object typed permissively so superRefine can set claimedIssues before adding its rejection issues), or record the flag from raw step data outside the schema. A regression test with a claim containing an invalid severity would pin the guarantee either way.

let claimedIssues = false;
let acceptedReport: Record<string, unknown> | null = null;
const reviewFindingSchema = z.object({
severity: z.enum(["critical", "warning", "suggestion"]),
title: z.string(),
body: z.string(),
path: z.string().optional(),
line: z.number().int().positive().optional(),
});
const reportReviewInputSchema = z
const reportObjectSchema = z
.object({
hasIssues: z.boolean(),
summary: z
Expand All @@ -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({
Expand Down Expand Up @@ -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;
},
}),
};

Expand All @@ -236,6 +264,7 @@ export function buildPRReviewTools(config: {
commitTitle: z.string().optional(),
}),
execute: async ({ commitTitle }) =>
droppedFindingsRefusal() ??
mergePullRequestIfSafe({
githubToken: config.githubToken,
owner: config.owner,
Expand All @@ -252,6 +281,7 @@ export function buildPRReviewTools(config: {
commitTitle: z.string().optional(),
}),
execute: async ({ commitTitle }) =>
droppedFindingsRefusal() ??
queuePullRequestAutoMerge({
githubToken: config.githubToken,
owner: config.owner,
Expand Down
3 changes: 2 additions & 1 deletion lib/workflows/automation-job-agent-node.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
1 change: 1 addition & 0 deletions lib/workflows/automation-job-executor-core.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
2 changes: 2 additions & 0 deletions lib/workflows/automation-job-flow-executor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ export type FlowExecutorDeps = {
export type FlowExecutionSuccess = {
success: true;
result: AutomationAgentResult;
reviewResult: AutomationAgentResult | null;
autoMergeRequest: FlowAutoMergeRequest | null;
observabilityError: string | null;
};
Expand Down Expand Up @@ -486,6 +487,7 @@ export async function executeResolvedFlow(input: {
result: mergeAutomationAgentResults(
state.results as AutomationAgentResult[]
),
reviewResult: state.reviewResult,
autoMergeRequest: state.autoMergeRequest,
observabilityError: state.observabilityError,
};
Expand Down
24 changes: 23 additions & 1 deletion lib/workflows/automation-job-flow-run-state.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";

Expand Down Expand Up @@ -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;

Expand All @@ -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.
*/
Expand All @@ -86,6 +107,7 @@ export function createFlowRunState(input: {
incomingCounts,
processed: new Set(),
results: [],
reviewResult: null,
observabilityError: null,
autoMergeRequest: null,
expectedTriggerHeadSha: input.initialHeadSha,
Expand Down
22 changes: 21 additions & 1 deletion lib/workflows/automation-job-flow-success.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -69,6 +71,23 @@ export type FlowSuccessFinalizationInput = {
) => Promise<AutomationJobRunResult>;
};

/**
* 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.
*
Expand All @@ -83,6 +102,7 @@ export async function finalizeFlowSuccess(
releasedScope,
context,
finalResult,
reviewResult,
autoMergeRequest,
observabilityError,
isPrReview,
Expand All @@ -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
Expand Down
3 changes: 2 additions & 1 deletion lib/workflows/automation-job-metadata.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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,
})),
Expand Down
4 changes: 3 additions & 1 deletion lib/workflows/automation-job-types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
41 changes: 28 additions & 13 deletions lib/workflows/pr-review-harness-extraction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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<PrReviewHarnessResult, "source" | "reviewOutcome"> | null
Expand All @@ -166,22 +171,32 @@ export function isPrReviewVerdictMissing(
);
}

function buildReportedHarnessResult(
result: AutomationAgentReviewResult,
reportState: Exclude<ReviewReportState, { kind: "missing" }>
): 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 } : {}),
};
}
Expand Down
2 changes: 2 additions & 0 deletions lib/workflows/pr-review-harness-formatting.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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":
Expand Down
3 changes: 2 additions & 1 deletion lib/workflows/pr-review-harness-types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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[];
}>;
};
Expand Down Expand Up @@ -32,6 +32,7 @@ export type PrAutofixOutcome = {

export type PrReviewContractSource =
| "structured"
| "dropped_findings"
| "legacy_post_comment"
| "legacy_text";

Expand Down
Loading
Loading