diff --git a/AGENTS.md b/AGENTS.md index d74b6317..e111a6d7 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -231,7 +231,7 @@ Codex sandbox runs go through the Agent Client Protocol: `lib/harness/acp/` hold ### Decision layer -`lib/decisions/` asks an evaluation model closed questions about program state (command risk, hidden tool failures, unsupported claims, loops, memory promotion, and, observing only, which skills and injected memories a request needed) and records every answer in `decision_events`. Read [docs/decisions.md](./docs/decisions.md) before touching it. The binding rules: `decide()` fails open and never throws; a decision may only add caution (an approval, a note) and never relaxes `policy.ts` or the shell guard, except that `review_format` may reword a published PR review after `review_rewrite_faithful` confirms it kept every claim; nothing in it may end or shorten a run; question wording and thresholds live only in `definitions.ts` and `definitions-selection.ts` with a `version` bump on every change; every call site puts `teamId` in its scope so the account's Run checks switch applies; customer-facing text never names the evaluation provider. The one authored decision is the automation Classify node (`classify()` in `lib/decisions/classify.ts`): its question lives in the customer's flow graph, and it does not fail open, because the author chose that node to pick a branch. An outage fails the node so the flow's `error` handle can route it. Tests inject evaluators through `deps`; the real evaluator is inert under Vitest. +`lib/decisions/` asks an evaluation model closed questions about program state (command risk, hidden tool failures, unsupported claims, loops, memory promotion, and, observing only, which skills and injected memories a request needed) and records every answer in `decision_events`. Read [docs/decisions.md](./docs/decisions.md) before touching it. The binding rules: `decide()` fails open and never throws; a decision may only add caution (an approval, a note) and never relaxes `policy.ts` or the shell guard, except that `review_format` sends a flagged native review draft back to the reviewer that wrote it (the revision must keep the verdict and every finding) and may reword a published PR review after `review_rewrite_faithful` confirms it kept every claim; nothing in it may end or shorten a run; question wording and thresholds live only in `definitions.ts` and `definitions-selection.ts` with a `version` bump on every change; every call site puts `teamId` in its scope so the account's Run checks switch applies; customer-facing text never names the evaluation provider. The one authored decision is the automation Classify node (`classify()` in `lib/decisions/classify.ts`): its question lives in the customer's flow graph, and it does not fail open, because the author chose that node to pick a branch. An outage fails the node so the flow's `error` handle can route it. Tests inject evaluators through `deps`; the real evaluator is inert under Vitest. ## Core Commands diff --git a/docs/decisions.md b/docs/decisions.md index 29872390..af7af661 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -20,7 +20,7 @@ Code acts. | `memory_promotion_gate` | Before memory promotion's extraction call | `shadow` | In `enforce`, skips extraction when the record holds nothing durable | | `skill_selection` | When a run or turn has skills in play, meaning a roster agent's linked skills, the user's skill catalog, or both: the sandbox harness route (`harness`), the native runner (`agent_run`), Control (`control`), and workspace chat (`chat`) | `shadow` | Records only. Linked skills are still all loaded and the catalog is still offered as an index; the row says which skills the task needed. `metadata.sources` tells a linked skill from a catalog one, `metadata.invoked` lists what the user named, and `baseline.loaded` against `baseline.offered` shows how much was delivered in full | | `memory_relevance` | Every Control turn that injects memories, beside the turn | `shadow` | Records only. The prompt is already built; the row says which injected memories bear on the request | -| `review_format` | Every structured PR review, before the check run, native review, and timeline comment are published (`pr_review`) | `enforce` | A dense paragraph, tool or report-field chatter, a pointer to details that are not stated, or unformatted code sends the text to a platform model for a formatting rewrite. The rewrite is published only if `review_rewrite_faithful` accepts it | +| `review_format` | A native review's draft inside the run, and at publish time any structured PR review not already passed there, before the check run, native review, and timeline comment are published (`pr_review`) | `enforce` | A dense paragraph, tool or report-field chatter, a pointer to details that are not stated, or unformatted code. In a native review the reviewer that wrote the draft is told what is wrong and files its report again in the same conversation; its revision replaces the draft if it keeps the verdict and every finding. A review still flagged at publish time (a harness review, or a revision that did not fix it) goes to a platform model for a formatting rewrite, published only if `review_rewrite_faithful` accepts it | | `review_rewrite_faithful` | After a `review_format` rewrite | `enforce` | Accepts the rewrite. Anything else, including an outage, publishes the reviewer's original text | | `flow_classify` | Every automation Classify node | always acts | The run takes the answered branch. The question is authored in the flow graph | @@ -105,13 +105,19 @@ Settings, Account (`PATCH /api/settings/decision-checks`). turned its checks off, makes it fail, not pass. - **Only add caution.** A decision can add an approval or a note. It can never relax `policy.ts`, a protected-branch rule, or the shell guard. The one - decision that changes output is `review_format`: it may replace the wording - of a published review, never its verdict, check conclusion, or a finding's - title, severity, path, or line, and only after `review_rewrite_faithful` - confirms the rewrite keeps every statement about the code. -- **Free to the account.** Every call, including the `review_format` rewrite - (`anthropic/claude-sonnet-5`, `PR_REVIEW_REWRITE_MODEL` overrides it), runs - on the platform credential and is never debited from the account. + decision that changes output is `review_format`. It sends a flagged native + draft back to its reviewer, whose revision is accepted only when it keeps + the verdict and every finding; it can add findings the draft referred to + without stating, which no rewrite could supply. At publish time it may + replace the wording of a review, never its verdict, check conclusion, or a + finding's title, severity, path, or line, and only after + `review_rewrite_faithful` confirms the rewrite keeps every statement about + the code. +- **Free to the account.** Every decision call, including the `review_format` + rewrite (`anthropic/claude-sonnet-5`, `PR_REVIEW_REWRITE_MODEL` overrides + it), runs on the platform credential and is never debited from the account. + A reviewer's revision is a turn of the review itself, on the review's model, + and is billed like the rest of the review. - **Never end a run.** The agent execution policy forbids iteration budgets. `loop_check` records what it sees and nothing more. - **Second opinion on the uncertain band.** When a definition sets `escalate` diff --git a/lib/decisions/definitions.ts b/lib/decisions/definitions.ts index 4f7b0a81..9a7fc803 100644 --- a/lib/decisions/definitions.ts +++ b/lib/decisions/definitions.ts @@ -245,9 +245,10 @@ const memoryPromotionGate: DecisionDefinition = { /** * Decisions that guard what Mogplex publishes on a pull request. The review - * model writes the text; these check its shape before it reaches GitHub and - * that a platform rewrite kept every claim. Both run on the platform's - * credential and are never billed to the account. + * model writes the text; these check its shape before it reaches GitHub, so + * a native reviewer can fix its own draft, and that a platform rewrite kept + * every claim. Both run on the platform's credential and are never billed to + * the account. * * Thresholds come from the 2026-09-28 replay of seven published reviews: a * well-formatted control scored under 0.3 on every question, and each real @@ -277,8 +278,9 @@ export function reviewFormatProblems( const reviewFormat: DecisionDefinition = { id: "review_format", version: "2026-09-28.1", - // Acting means a platform-paid rewrite that a second check must accept, - // and the original is published whenever anything fails. + // Acting sends a native draft back to its reviewer, or at publish time + // means a platform-paid rewrite that a second check must accept; the + // original is published whenever anything fails. defaultMode: "enforce", timeoutMs: 4000, escalate: false, diff --git a/lib/workflows/automation-job-agent-runners-shared.ts b/lib/workflows/automation-job-agent-runners-shared.ts index 362d256b..515f572c 100644 --- a/lib/workflows/automation-job-agent-runners-shared.ts +++ b/lib/workflows/automation-job-agent-runners-shared.ts @@ -24,7 +24,13 @@ import { type AutomationLanguageModel, type JobContext, } from "@/lib/workflows/automation-job-types"; +import type { ReviewFormatProblem } from "@/lib/decisions/definitions"; import { resolveAutomationSkills } from "@/lib/workflows/automation-job-skills"; +import { + findReviewFormatProblems, + prReviewDecisionScope, +} from "@/lib/workflows/pr-review-format-check"; +import type { PrReviewHarnessResult } from "@/lib/workflows/pr-review-harness-types"; import { normalizeAutomationAssignmentType } from "@/lib/workflows/automation-job-utils"; import { getAutomationGenerateTimeoutMs } from "@/lib/workflows/automation-model-defaults"; @@ -38,6 +44,11 @@ export type AutomationAgentDeps = { loadApprovalSpentWaitMs: typeof loadToolApprovalSpentWaitMs; /** The node's skills and its agent's attachments; never rejects. */ resolveSkills: typeof resolveAutomationSkills; + /** The review_format check on a reviewer's draft; null when it did not run. */ + judgeReviewFormat: ( + draft: PrReviewHarnessResult, + context: JobContext + ) => Promise; }; export const defaultAutomationAgentDeps: AutomationAgentDeps = { @@ -46,6 +57,14 @@ export const defaultAutomationAgentDeps: AutomationAgentDeps = { waitStore: supabaseWaitStore, loadApprovalSpentWaitMs: loadToolApprovalSpentWaitMs, resolveSkills: resolveAutomationSkills, + // A flow run carries its job run id in metadata; a standalone review's is + // not in its context, and its rows are found by repo, PR, and time. + judgeReviewFormat: (draft, context) => + findReviewFormatProblems(draft, prReviewDecisionScope(context, null), { + job_run_id: context.metadata.flow_job_run_id ?? null, + pr_number: context.metadata.pr_number ?? null, + stage: "reviewer_draft", + }), }; // Applies the tool-approval gate when the flow agent node opted in via diff --git a/lib/workflows/automation-job-agent-runners.ts b/lib/workflows/automation-job-agent-runners.ts index 4365671a..e914512a 100644 --- a/lib/workflows/automation-job-agent-runners.ts +++ b/lib/workflows/automation-job-agent-runners.ts @@ -27,10 +27,8 @@ import { splitRepoFullName, } from "@/lib/workflows/automation-job-utils"; import { normalizeAutomationAgentResult } from "@/lib/workflows/automation-job-metadata"; -import { - fileMissingReviewReport, - type ReportRepairRequest, -} from "@/lib/workflows/pr-review-report-repair"; +import type { ReportRepairRequest } from "@/lib/workflows/pr-review-report-repair"; +import { finishPrReview } from "@/lib/workflows/pr-review-self-revision"; import { appendToRunSpec, buildJobRunSpec, @@ -314,13 +312,13 @@ export function createAutomationAgentRunner( }); if (assignmentType !== "pr_review") return review.normalized; - // A reviewer that ends without its report leaves no verdict; ask for it. - return fileMissingReviewReport({ + return finishPrReview({ result: review.normalized, responseMessages: review.responseMessages, prompt: runSpec.prompt, tools, generate: async (request) => (await generate(request)).normalized, + judge: (draft) => deps.judgeReviewFormat(draft, context), }); }; } diff --git a/lib/workflows/automation-job-pr-review-finalize.ts b/lib/workflows/automation-job-pr-review-finalize.ts index 1386ab8c..4e16996f 100644 --- a/lib/workflows/automation-job-pr-review-finalize.ts +++ b/lib/workflows/automation-job-pr-review-finalize.ts @@ -19,8 +19,10 @@ import type { import { isPrReviewVerdictMissing } from "./pr-review-harness-extraction"; import type { persistJobReviewFindings } from "./automation-job-persistence"; import type { PrReviewReporterState } from "./automation-job-pr-review-reporter"; -import { metadataTeamId } from "./automation-job-classify"; -import { polishPrReviewForPublish } from "./pr-review-format-check"; +import { + polishPrReviewForPublish, + prReviewDecisionScope, +} from "./pr-review-format-check"; export type FinalizePrReviewSuccessResult = | { @@ -167,16 +169,7 @@ export async function finalizePrReviewSuccess( deps.polishPrReviewForPublish ?? polishPrReviewForPublish )( input.reviewHarnessResult, - { - surface: "pr_review", - userId: context.repo.user_id, - teamId: - metadataTeamId(context.metadata) ?? - context.repo.product_team_id ?? - null, - repoId: context.repo.id, - aiCallId: input.result.aiCallId ?? null, - }, + prReviewDecisionScope(context, input.result.aiCallId ?? null), { job_run_id: input.jobRunId, pr_number: reviewPrNumber } ); const publishedOutcome = diff --git a/lib/workflows/automation-job-types.ts b/lib/workflows/automation-job-types.ts index 45974966..6c083e2c 100644 --- a/lib/workflows/automation-job-types.ts +++ b/lib/workflows/automation-job-types.ts @@ -140,6 +140,8 @@ export type AutomationAgentResult = { usage: AutomationAgentUsage | null; execution?: AutomationModelExecutionMetadata | null; aiCallId?: string | null; + /** The review's report was judged and passed the format check inside the run, so publishing does not check it again. */ + reviewFormatPassed?: boolean; }; // Carries the model id that actually executed. It can differ from the pinned diff --git a/lib/workflows/pr-review-format-check.test.ts b/lib/workflows/pr-review-format-check.test.ts index 1491f212..036ea289 100644 --- a/lib/workflows/pr-review-format-check.test.ts +++ b/lib/workflows/pr-review-format-check.test.ts @@ -8,6 +8,7 @@ import { import type { PrReviewReporterState } from "./automation-job-pr-review-reporter"; import type { JobContext } from "./automation-job-types"; import { + findReviewFormatProblems, polishPrReviewForPublish, type PrReviewFormatDeps, type PrReviewRewrite, @@ -57,11 +58,20 @@ const withFinding: PrReviewHarnessResult = { }, }; -function handle(id: DecisionId, act: boolean, answers = {}): DecisionHandle { +const ENFORCED: Pick = { + mode: "enforce", + status: "ok", +}; + +function handle( + id: DecisionId, + act: boolean, + answers = {}, + judged = ENFORCED +): DecisionHandle { return { id, - mode: "enforce", - status: "ok", + ...judged, verdict: act ? "act" : "pass", act, escalated: false, @@ -258,6 +268,78 @@ describe("polishPrReviewForPublish", () => { }); }); +describe("findReviewFormatProblems", () => { + const judge = ( + judged: Pick, + act = false, + available = true + ) => + findReviewFormatProblems( + denseClean, + scope, + {}, + { + available: () => available, + decide: async (id) => handle(id, act, flagged, judged), + } + ); + + it("should name the problems of text it judged and flagged", async () => { + await expect( + judge({ mode: "enforce", status: "ok" }, true) + ).resolves.toEqual(["denseParagraph", "danglingReference"]); + }); + + it("should pass text it judged and did not flag", async () => { + await expect(judge({ mode: "enforce", status: "ok" })).resolves.toEqual([]); + }); + + it("should never report a pass for text it did not judge", async () => { + await expect( + judge({ mode: "enforce", status: "unavailable" }) + ).resolves.toBeNull(); + await expect(judge({ mode: "off", status: "off" })).resolves.toBeNull(); + await expect(judge({ mode: "shadow", status: "ok" })).resolves.toBeNull(); + await expect( + judge({ mode: "enforce", status: "ok" }, false, false) + ).resolves.toBeNull(); + }); +}); + +describe("polishPrReviewForPublish and a review passed inside the run", () => { + it("should publish it without checking it again", async () => { + const { deps, calls } = makeDeps({ formatActs: true }); + const passed = { ...denseClean, formatPassed: true }; + + await expect( + polishPrReviewForPublish(passed, scope, {}, deps) + ).resolves.toBe(passed); + expect(calls).toEqual([]); + }); + + it("should read the pass from the reviewer's result", () => { + const reported = { + text: "Done.", + steps: [ + { + toolCalls: [ + { + toolName: "reportReview", + input: { hasIssues: false, summary: "Fine." }, + }, + ], + }, + ], + }; + + expect( + extractPrReviewHarnessResult({ ...reported, reviewFormatPassed: true }) + .formatPassed + ).toBe(true); + expect(extractPrReviewHarnessResult(reported).formatPassed).toBeUndefined(); + }); +}); + describe("what a clean structured review publishes", () => { it("should publish the report's summary and commentBody, never the closing chat message", () => { const harnessResult = extractPrReviewHarnessResult({ diff --git a/lib/workflows/pr-review-format-check.ts b/lib/workflows/pr-review-format-check.ts index 6eebd9fc..eb825ca3 100644 --- a/lib/workflows/pr-review-format-check.ts +++ b/lib/workflows/pr-review-format-check.ts @@ -11,10 +11,15 @@ import { generateText, Output } from "ai"; import { z } from "zod"; -import { decide } from "@/lib/decisions/decide"; -import { reviewFormatProblems } from "@/lib/decisions/definitions"; +import { decide, type DecisionHandle } from "@/lib/decisions/decide"; +import { + reviewFormatProblems, + type ReviewFormatProblem, +} from "@/lib/decisions/definitions"; import { resolveDecisionGateway } from "@/lib/decisions/evaluator"; import type { DecisionScope } from "@/lib/decisions/types"; +import { metadataTeamId } from "./automation-job-classify"; +import type { JobContext } from "./automation-job-types"; import { buildPrReviewCheckText } from "./pr-review-harness"; import type { PrReviewHarnessResult, @@ -117,6 +122,53 @@ function renderForJudging(harnessResult: PrReviewHarnessResult) { }); } +/** The account and repo a PR review's decisions are recorded under. */ +export function prReviewDecisionScope( + context: JobContext, + aiCallId: string | null +): DecisionScope { + return { + surface: "pr_review", + userId: context.repo.user_id, + teamId: + metadataTeamId(context.metadata) ?? context.repo.product_team_id ?? null, + repoId: context.repo.id, + aiCallId, + }; +} + +/** + * What the format check found wrong with the review as it would be + * published, or null when it did not judge the text: no platform credential, + * checks off for the account, an evaluator outage, or a mode that does not + * act. Null is never a pass. + */ +export async function findReviewFormatProblems( + harnessResult: PrReviewHarnessResult, + scope: DecisionScope, + metadata: Record, + deps: Pick = defaultDeps +): Promise { + if (!deps.available()) return null; + const format = await deps.decide( + "review_format", + { review_markdown: renderForJudging(harnessResult) }, + scope, + { metadata } + ); + if (!judgedTheText(format)) return null; + return format.act && format.answers + ? reviewFormatProblems(format.answers) + : []; +} + +function judgedTheText(format: Pick) { + return ( + format.status === "ok" && + (format.mode === "enforce" || format.mode === "advise") + ); +} + /** Titles, paths, lines, and severities stay the reviewer's own. */ function applyRewrite( harnessResult: PrReviewHarnessResult, @@ -144,6 +196,18 @@ function applyRewrite( }; } +/** A structured review not already passed inside the run, with a credential to check it. */ +function needsPublishCheck( + harnessResult: PrReviewHarnessResult | null, + deps: Pick +): harnessResult is PrReviewHarnessResult { + return ( + harnessResult?.source === "structured" && + harnessResult.formatPassed !== true && + deps.available() + ); +} + /** * Returns the result to publish: the input unchanged unless the format check * flagged it and a faithful rewrite exists. @@ -154,21 +218,18 @@ export async function polishPrReviewForPublish( metadata: Record = {}, deps: PrReviewFormatDeps = defaultDeps ): Promise { - if (harnessResult?.source !== "structured" || !deps.available()) { - return harnessResult; - } + if (!needsPublishCheck(harnessResult, deps)) return harnessResult; try { - const original = renderForJudging(harnessResult); - const format = await deps.decide( - "review_format", - { review_markdown: original }, + const problems = await findReviewFormatProblems( + harnessResult, scope, - { metadata } + metadata, + deps ); - if (!format.act || !format.answers) return harnessResult; + if (!problems?.length) return harnessResult; - const problems = reviewFormatProblems(format.answers); + const original = renderForJudging(harnessResult); const rewritten = await deps.rewrite({ outcome: harnessResult.reviewOutcome, problems, diff --git a/lib/workflows/pr-review-harness-extraction.ts b/lib/workflows/pr-review-harness-extraction.ts index 44ea5f0c..6c998b34 100644 --- a/lib/workflows/pr-review-harness-extraction.ts +++ b/lib/workflows/pr-review-harness-extraction.ts @@ -179,6 +179,7 @@ function buildReportedHarnessResult( return { source: reportState.kind === "filed" ? "structured" : "dropped_findings", fallbackText: toOptionalString(result.text), + ...(result.reviewFormatPassed === true ? { formatPassed: true } : {}), reviewOutcome: { hasIssues: report.hasIssues === true, summary: toOptionalString(report.summary) ?? result.text ?? "", diff --git a/lib/workflows/pr-review-harness-types.ts b/lib/workflows/pr-review-harness-types.ts index a245b57d..8acc6c92 100644 --- a/lib/workflows/pr-review-harness-types.ts +++ b/lib/workflows/pr-review-harness-types.ts @@ -2,6 +2,7 @@ import type { ReviewFinding } from "@/lib/types"; export type AutomationAgentReviewResult = { text: string; + reviewFormatPassed?: boolean; steps: Array<{ toolCalls?: Array<{ toolName: string; input: unknown; invalid?: boolean }>; toolResults?: unknown[]; @@ -40,6 +41,8 @@ export type PrReviewHarnessResult = { source: PrReviewContractSource; reviewOutcome: ReviewOutcome; fallbackText: string | null; + /** The report passed the format check inside the run; publishing does not check it again. */ + formatPassed?: boolean; autofix?: PrAutofixOutcome | null; }; diff --git a/lib/workflows/pr-review-report-repair.ts b/lib/workflows/pr-review-report-repair.ts index 4de501de..00326aba 100644 --- a/lib/workflows/pr-review-report-repair.ts +++ b/lib/workflows/pr-review-report-repair.ts @@ -14,7 +14,7 @@ export const PR_REVIEW_REPORT_REPAIR_PROMPT = /** 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."; + "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. If you are sure there are no issues after all, report that, and the review will be published as incomplete. Do not start a new review and do not call any other tool."; const REPAIR_PROMPTS: Record< Exclude, @@ -81,50 +81,70 @@ function isRejectedRequest(error: unknown): boolean { ); } -/** - * 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 - * request cannot be made or throws, the result is returned as it was and is - * published as incomplete. - */ -export async function fileMissingReviewReport(input: { +export type ReviewerFollowUp = { result: AutomationAgentResult; responseMessages: ModelMessage[] | undefined; prompt: string; tools: ToolSet; generate: (request: ReportRepairRequest) => Promise; -}): Promise { - const state = readReviewReportState(input.result.steps); - if (state.kind === "filed") return input.result; +}; + +/** + * Continues the reviewer's own conversation with one request whose only + * possible answer is a report. 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. Returns the review with the answer + * appended and the reviewer's closing text kept, or null when the toolset has + * no report tool. Throws when the model call fails. + */ +export async function askReviewerForReport( + input: ReviewerFollowUp & { request: string } +): Promise { const tools = buildReportOnlyTools(input.tools); - if (!tools) return input.result; + if (!tools) return null; const messages = buildReportRepairMessages({ prompt: input.prompt, responseMessages: input.responseMessages, text: input.result.text, - request: REPAIR_PROMPTS[state.kind], + request: input.request, }); const ask = (toolChoice: ReportRepairRequest["toolChoice"]) => input.generate({ tools, toolChoice, messages }); + const answer = await ask(FORCED_REPORT_CHOICE).catch((error: unknown) => { + if (!isRejectedRequest(error)) throw error; + console.warn( + "[pr-review] provider rejected a forced report; asking without forcing", + { error } + ); + return ask("auto"); + }); + return { + ...mergeAutomationAgentResults([input.result, answer]), + text: input.result.text, + }; +} + +/** + * 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. This never fails a + * review that otherwise completed: if the request cannot be made or throws, + * the result is returned as it was and is published as incomplete. + */ +export async function fileMissingReviewReport( + input: ReviewerFollowUp +): Promise { + const state = readReviewReportState(input.result.steps); + if (state.kind === "filed") return input.result; try { - const repair = await ask(FORCED_REPORT_CHOICE).catch((error: unknown) => { - if (!isRejectedRequest(error)) throw error; - console.warn( - "[pr-review] provider rejected a forced report; asking without forcing", - { error } - ); - return ask("auto"); - }); - return { - ...mergeAutomationAgentResults([input.result, repair]), - text: input.result.text, - }; + return ( + (await askReviewerForReport({ + ...input, + request: REPAIR_PROMPTS[state.kind], + })) ?? input.result + ); } catch (error) { console.warn("[pr-review] could not recover the missing review report", { error, diff --git a/lib/workflows/pr-review-self-revision.test.ts b/lib/workflows/pr-review-self-revision.test.ts new file mode 100644 index 00000000..53530c43 --- /dev/null +++ b/lib/workflows/pr-review-self-revision.test.ts @@ -0,0 +1,409 @@ +import { tool } from "ai"; +import { describe, expect, it, vi } from "vitest"; +import { z } from "zod"; +import type { ReviewFormatProblem } from "@/lib/decisions/definitions"; +import type { AutomationAgentResult } from "./automation-job-types"; +import { extractPrReviewHarnessResult } from "./pr-review-harness-extraction"; +import type { PrReviewHarnessResult } from "./pr-review-harness-types"; +import type { ReportRepairRequest } from "./pr-review-report-repair"; +import { + finishPrReview, + REVIEW_FORMAT_FEEDBACK, + reviseFlaggedReview, +} from "./pr-review-self-revision"; + +const warning = { + severity: "warning", + title: "Guard nullable lookup", + body: "`lookup()` can return undefined.", + path: "src/widget.ts", +}; +const suggestion = { + severity: "suggestion", + title: "Name the retry budget", + body: "The 3 in `retry()` is a magic number.", + path: "src/retry.ts", +}; + +function reported(input: Record): AutomationAgentResult { + return { + text: "Reviewed.", + steps: [{ toolCalls: [{ toolName: "reportReview", input }] }], + usage: null, + }; +} + +/** The #535 draft: it mentions suggestions it never lists. */ +const danglingDraft = reported({ + hasIssues: false, + summary: "Approve. Three minor suggestions, detailed in the comment.", +}); + +const listedRevision = reported({ + hasIssues: false, + summary: "Approve. Three minor suggestions follow.", + findings: [suggestion], +}); + +type Judged = { summary: string; findings: number }; + +/** A judge that flags the given problems for any review whose summary matches. */ +function judgeFlagging( + flagged: RegExp, + problems: ReviewFormatProblem[] = ["danglingReference"] +) { + const judged: Judged[] = []; + const judge = async (harnessResult: PrReviewHarnessResult) => { + judged.push({ + summary: harnessResult.reviewOutcome.summary, + findings: harnessResult.reviewOutcome.findings.length, + }); + return flagged.test(harnessResult.reviewOutcome.summary) ? problems : []; + }; + return { judge, judged }; +} + +function reviewer(answers: Array) { + const requests: ReportRepairRequest[] = []; + const generate = async (request: ReportRepairRequest) => { + requests.push(request); + const answer = answers[requests.length - 1]; + if (!answer || answer instanceof Error) { + throw answer ?? new Error("no scripted answer"); + } + return answer; + }; + return { generate, requests }; +} + +const followUp = { + responseMessages: undefined, + prompt: "Review PR #42.", + tools: { + reportReview: tool({ + inputSchema: z.object({ hasIssues: z.boolean() }), + execute: async (input) => input, + }), + }, +}; + +function published(result: AutomationAgentResult) { + return extractPrReviewHarnessResult(result).reviewOutcome; +} + +describe("reviseFlaggedReview", () => { + it("should mark a draft the format check passes without asking the reviewer anything", async () => { + const { judge } = judgeFlagging(/never/); + const { generate, requests } = reviewer([]); + + const result = await reviseFlaggedReview({ + ...followUp, + result: danglingDraft, + generate, + judge, + }); + + expect(requests).toEqual([]); + expect(result.reviewFormatPassed).toBe(true); + }); + + it("should have the reviewer fix a flagged draft in its own conversation and publish its revision", async () => { + const { judge, judged } = judgeFlagging(/detailed in the comment/); + const { generate, requests } = reviewer([listedRevision]); + + const result = await reviseFlaggedReview({ + ...followUp, + result: danglingDraft, + generate, + judge, + }); + + expect(requests).toHaveLength(1); + const [request] = requests; + expect(Object.keys(request.tools)).toEqual(["reportReview"]); + expect(request.messages[0]).toEqual({ + role: "user", + content: "Review PR #42.", + }); + const ask = String(request.messages.at(-1)?.content); + expect(ask).toContain(REVIEW_FORMAT_FEEDBACK.danglingReference); + expect(ask).toContain("detailed in the comment"); + expect(published(result).findings).toHaveLength(1); + expect(published(result).summary).toBe( + "Approve. Three minor suggestions follow." + ); + // The revision was judged too, and it passed. + expect(judged.map((entry) => entry.findings)).toEqual([0, 1]); + expect(result.reviewFormatPassed).toBe(true); + }); + + it("should publish a revision the check still flags without marking it passed", async () => { + const { judge } = judgeFlagging(/suggestions/); + const { generate } = reviewer([listedRevision]); + + const result = await reviseFlaggedReview({ + ...followUp, + result: danglingDraft, + generate, + judge, + }); + + expect(published(result).findings).toHaveLength(1); + expect(result.reviewFormatPassed).toBeUndefined(); + }); + + it("should keep the draft when the revision drops a finding", async () => { + const draft = reported({ + hasIssues: true, + summary: "One warning; reportReview filed.", + findings: [warning, suggestion], + }); + const { judge } = judgeFlagging(/reportReview/, ["processTalk"]); + const { generate } = reviewer([ + reported({ + hasIssues: true, + summary: "One warning.", + findings: [warning], + }), + ]); + + const result = await reviseFlaggedReview({ + ...followUp, + result: draft, + generate, + judge, + }); + + expect(result).toBe(draft); + }); + + it("should keep the draft when the revision clears the issues it reported", async () => { + const draft = reported({ + hasIssues: true, + summary: "One warning; reportReview filed.", + findings: [warning], + }); + const { judge } = judgeFlagging(/reportReview/, ["processTalk"]); + const { generate } = reviewer([ + reported({ hasIssues: false, summary: "Fine." }), + ]); + + const result = await reviseFlaggedReview({ + ...followUp, + result: draft, + generate, + judge, + }); + + expect(result).toBe(draft); + }); + + it("should keep the draft when the revision flips the verdict but keeps its findings", async () => { + const draft = reported({ + hasIssues: true, + summary: "One warning; reportReview filed.", + findings: [warning], + }); + const { judge } = judgeFlagging(/reportReview/, ["processTalk"]); + const { generate } = reviewer([ + reported({ + hasIssues: false, + summary: "Approve with one note.", + findings: [warning], + }), + ]); + + const result = await reviseFlaggedReview({ + ...followUp, + result: draft, + generate, + judge, + }); + + expect(result).toBe(draft); + }); + + it("should keep the draft when the revision swaps a finding for another", async () => { + const draft = reported({ + hasIssues: true, + summary: "One warning; reportReview filed.", + findings: [warning], + }); + const { judge } = judgeFlagging(/reportReview/, ["processTalk"]); + const { generate } = reviewer([ + reported({ + hasIssues: true, + summary: "One warning.", + findings: [{ ...warning, path: "src/other.ts" }], + }), + ]); + + const result = await reviseFlaggedReview({ + ...followUp, + result: draft, + generate, + judge, + }); + + expect(result).toBe(draft); + }); + + it("should accept a revision that rewords a finding it keeps", async () => { + const draft = reported({ + hasIssues: true, + summary: "One warning; reportReview filed.", + findings: [warning], + }); + const { judge } = judgeFlagging(/reportReview/, ["processTalk"]); + const { generate } = reviewer([ + reported({ + hasIssues: true, + summary: "One warning.", + findings: [{ ...warning, title: "Guard the nullable `lookup()`" }], + }), + ]); + + const result = await reviseFlaggedReview({ + ...followUp, + result: draft, + generate, + judge, + }); + + expect(published(result).findings[0]?.title).toBe( + "Guard the nullable `lookup()`" + ); + expect(result.reviewFormatPassed).toBe(true); + }); + + it("should leave a draft the check did not judge unmarked and unrevised", async () => { + const { generate, requests } = reviewer([listedRevision]); + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + + const result = await reviseFlaggedReview({ + ...followUp, + result: danglingDraft, + generate, + judge: async () => null, + }); + + expect(requests).toEqual([]); + expect(result).toBe(danglingDraft); + // Not judging is a normal outcome, not a failed revision. + expect(warn).not.toHaveBeenCalled(); + warn.mockRestore(); + }); + + it("should not mark a revision the check could not judge again", async () => { + let judgments = 0; + const { generate } = reviewer([listedRevision]); + + const result = await reviseFlaggedReview({ + ...followUp, + result: danglingDraft, + generate, + judge: async () => (++judgments === 1 ? ["danglingReference"] : null), + }); + + expect(published(result).findings).toHaveLength(1); + expect(result.reviewFormatPassed).toBeUndefined(); + }); + + it("should keep the draft when the revised report is rejected", async () => { + const { judge } = judgeFlagging(/detailed/); + const { generate } = reviewer([ + { + text: "", + steps: [ + { + toolCalls: [ + { + toolName: "reportReview", + input: { hasIssues: true, summary: "Three suggestions." }, + invalid: true, + }, + ], + }, + ], + usage: null, + }, + ]); + + const result = await reviseFlaggedReview({ + ...followUp, + result: danglingDraft, + generate, + judge, + }); + + expect(result).toBe(danglingDraft); + }); + + it("should keep the draft when the reviewer cannot be asked or the check fails", async () => { + const { judge } = judgeFlagging(/detailed/); + const failing = reviewer([new Error("model unavailable")]); + const throwingJudge = async () => { + throw new Error("evaluator down"); + }; + + const afterModelError = await reviseFlaggedReview({ + ...followUp, + result: danglingDraft, + generate: failing.generate, + judge, + }); + const afterJudgeError = await reviseFlaggedReview({ + ...followUp, + result: danglingDraft, + generate: reviewer([]).generate, + judge: throwingJudge, + }); + + expect(afterModelError).toBe(danglingDraft); + expect(afterJudgeError).toBe(danglingDraft); + }); + + it("should leave a review without a trusted report to the incomplete path", async () => { + const { judge, judged } = judgeFlagging(/./); + const unreported: AutomationAgentResult = { + text: "Reviewed.", + steps: [], + usage: null, + }; + + const result = await reviseFlaggedReview({ + ...followUp, + result: unreported, + generate: reviewer([]).generate, + judge, + }); + + expect(result).toBe(unreported); + expect(judged).toEqual([]); + }); +}); + +describe("finishPrReview", () => { + it("should file a missing report first and then check the report it got", async () => { + const { judge, judged } = judgeFlagging(/detailed in the comment/); + const { generate, requests } = reviewer([danglingDraft, listedRevision]); + + const result = await finishPrReview({ + ...followUp, + result: { text: "Reviewed.", steps: [], usage: null }, + generate, + judge, + }); + + expect(requests).toHaveLength(2); + expect(String(requests[0].messages.at(-1)?.content)).toMatch( + /without calling reportReview/ + ); + expect(String(requests[1].messages.at(-1)?.content)).toContain( + REVIEW_FORMAT_FEEDBACK.danglingReference + ); + expect(judged).toHaveLength(2); + expect(published(result).findings).toHaveLength(1); + expect(result.reviewFormatPassed).toBe(true); + }); +}); diff --git a/lib/workflows/pr-review-self-revision.ts b/lib/workflows/pr-review-self-revision.ts new file mode 100644 index 00000000..9848c165 --- /dev/null +++ b/lib/workflows/pr-review-self-revision.ts @@ -0,0 +1,133 @@ +import type { ReviewFormatProblem } from "@/lib/decisions/definitions"; +import type { AutomationAgentResult } from "./automation-job-types"; +import { extractPrReviewHarnessResult } from "./pr-review-harness-extraction"; +import type { ReviewFinding } from "@/lib/types"; +import type { PrReviewHarnessResult } from "./pr-review-harness-types"; +import { + askReviewerForReport, + fileMissingReviewReport, + type ReviewerFollowUp, +} from "./pr-review-report-repair"; +import { readReviewReportState } from "./pr-review-report-state"; + +/** What the reviewer is told for each problem the format check can find. */ +export const REVIEW_FORMAT_FEEDBACK: Readonly< + Record +> = { + denseParagraph: + "A paragraph covers several separate checks or findings. Put them in a bullet list, one point per bullet.", + processTalk: + "The review names internal tools or report fields, such as reportReview, hasIssues, commentBody, or findings. Describe the code, not the report.", + danglingReference: + "The review says suggestions, notes, or details exist without stating them. State each one; file every suggestion as a finding with severity suggestion.", + bareCode: + "File paths, functions, or identifiers appear as plain text. Wrap each one in backticks.", +}; + +export function buildReviewRevisionRequest( + problems: readonly ReviewFormatProblem[], + report: Record +): string { + return [ + "Mogplex checked the review you filed before publishing it and found these problems:", + ...problems.map((problem) => `- ${REVIEW_FORMAT_FEEDBACK[problem]}`), + `The report you filed: ${JSON.stringify(report)}`, + "Call reportReview again with the corrected review. Keep your verdict (hasIssues) and every finding with its severity, path, and line; add any you referred to without stating. Do not start a new review and do not call any other tool.", + ].join("\n"); +} + +/** Null when the check did not judge the text; that is never a pass. */ +export type ReviewFormatJudge = ( + harnessResult: PrReviewHarnessResult +) => Promise; + +/** What identifies a finding across a formatting fix, which may reword its title and body. */ +function findingKey(finding: ReviewFinding) { + return JSON.stringify([finding.severity, finding.path, finding.line]); +} + +function keepsEveryFinding(draft: ReviewFinding[], revised: ReviewFinding[]) { + const remaining = revised.map(findingKey); + return draft.every((finding) => { + const index = remaining.indexOf(findingKey(finding)); + if (index === -1) return false; + remaining.splice(index, 1); + return true; + }); +} + +/** + * The revision replaces the draft only if it is a report the pipeline + * trusts, keeps the verdict, and keeps every finding. It may add findings: + * that is how a draft that mentioned suggestions without stating them gets + * fixed. + */ +function acceptsRevision( + draft: PrReviewHarnessResult, + revised: PrReviewHarnessResult +): boolean { + return ( + revised.source === "structured" && + revised.reviewOutcome.hasIssues === draft.reviewOutcome.hasIssues && + keepsEveryFinding( + draft.reviewOutcome.findings, + revised.reviewOutcome.findings + ) + ); +} + +/** + * Checks a native review's report before it leaves the run, while the + * reviewer's conversation still exists. When the format check flags it, the + * reviewer that wrote it is told what is wrong and files it again. Only the + * author can fix some problems: a review that mentions suggestions without + * stating them needs the suggestions, which no rewrite of the text can + * supply. A result whose report the check judged and passed, as drafted or + * as revised, carries `reviewFormatPassed` so publishing does not judge it + * again. + * + * Never fails the review: a check that did not run, a revision that fails, + * or a revision that loses the verdict or a finding leaves the draft + * unmarked, and publishing checks it again and can still rewrite it. + */ +export async function reviseFlaggedReview( + input: ReviewerFollowUp & { judge: ReviewFormatJudge } +): Promise { + const state = readReviewReportState(input.result.steps); + if (state.kind !== "filed") return input.result; + const draft = extractPrReviewHarnessResult(input.result); + + try { + const problems = await input.judge(draft); + if (!problems) return input.result; + if (problems.length === 0) { + return { ...input.result, reviewFormatPassed: true }; + } + const revision = await askReviewerForReport({ + ...input, + request: buildReviewRevisionRequest(problems, state.report), + }); + if (!revision) return input.result; + const revised = extractPrReviewHarnessResult(revision); + if (!acceptsRevision(draft, revised)) return input.result; + const remaining = await input.judge(revised); + return remaining?.length === 0 + ? { ...revision, reviewFormatPassed: true } + : revision; + } catch (error) { + console.warn("[pr-review] review revision failed open", { error }); + return input.result; + } +} + +/** + * Asks for a missing report, then has the reviewer fix what the format check + * flags. Both follow-ups continue the review's own transcript; a repair turn + * is not in it, so the revision request quotes the report the reviewer filed. + */ +export async function finishPrReview( + input: ReviewerFollowUp & { judge: ReviewFormatJudge } +): Promise { + const reported = await fileMissingReviewReport(input); + return reviseFlaggedReview({ ...input, result: reported }); +} diff --git a/tests/unit/pr-review-report-repair.test.ts b/tests/unit/pr-review-report-repair.test.ts index 9b52a64f..276831f5 100644 --- a/tests/unit/pr-review-report-repair.test.ts +++ b/tests/unit/pr-review-report-repair.test.ts @@ -1,4 +1,5 @@ import assert from "node:assert/strict"; +import type { AutomationAgentDeps } from "../../lib/workflows/automation-job-agent-runners-shared"; import test from "node:test"; import { loadAutomationJobWorkflowModule, @@ -74,7 +75,10 @@ const FILED_REPORT = { totalUsage: { inputTokens: 1100, outputTokens: 30 }, }; -async function runReview(replies: Array) { +async function runReview( + replies: Array, + judgeReviewFormat?: AutomationAgentDeps["judgeReviewFormat"] +) { const { createAutomationAgentRunner } = await loadAutomationJobWorkflowModule(); const { extractPrReviewHarnessResult, isPrReviewVerdictMissing } = @@ -83,6 +87,7 @@ async function runReview(replies: Array) { const mockedGithubFetch = mockGithubPullRequestFetch([42]); try { const runAutomationAgent = createAutomationAgentRunner({ + ...(judgeReviewFormat ? { judgeReviewFormat } : {}), generateText: async (input) => { calls.push(input as unknown as Captured); const reply = replies[calls.length - 1]; @@ -360,3 +365,43 @@ test("a repair whose own report is rejected leaves the review without a verdict assert.equal(harness.source, "dropped_findings"); assert.equal(verdictMissing, true); }); + +test("a filed report the format check flags goes back to the reviewer, and its fix is published", async () => { + const fixed = { + ...FILED_REPORT, + steps: [ + makeStep({ + toolCalls: [ + { + toolName: "reportReview", + input: { + ...(FILED_REPORT.steps[0].toolCalls[0].input as Record< + string, + unknown + >), + summary: "The retry path in `sandbox.ts` skips the check.", + }, + }, + ], + }), + ], + }; + const judged: string[] = []; + const { calls, harness, result } = await runReview( + [FILED_REPORT, fixed], + async (draft) => { + judged.push(draft.reviewOutcome.summary); + return draft.reviewOutcome.summary.includes("`") ? [] : ["bareCode"]; + } + ); + + assert.equal(calls.length, 2); + assert.match(String(calls[1].messages?.at(-1)?.content), /backticks/); + assert.deepEqual(Object.keys(calls[1].tools ?? {}), ["reportReview"]); + assert.equal( + harness.reviewOutcome.summary, + "The retry path in `sandbox.ts` skips the check." + ); + assert.equal(judged.length, 2); + assert.equal(result.reviewFormatPassed, true); +});