diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 8c2ad5bd8..b238dcf7e 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -908,7 +908,11 @@ its tools are the host read bundle and repository reads confined to that folder. The consolidate step reads each finding's lines at the head and reports the findings it could show the model no line of; a comment whose group had no member read carries `unverified`, which the portal and the review sent to -GitHub show as "Not checked against the file". +GitHub show as "Not checked against the file". The check step reads every +consolidated group in one more model call each: the candidate, its quoted code +and the file lines around it, answered real or not real with a reason. Only +groups it finds real are written up and posted; the others stay in the +reported result's `dropped`, each with the verdict and reason. The portal's Code review page edits a repository's policy file without its text: it reads the file through the policy and policy-file reads, keeps a diff --git a/docs/Corbits Triage User Manual/Corbits Triage User Manual.md b/docs/Corbits Triage User Manual/Corbits Triage User Manual.md index 026a0dfcc..ad8a007b3 100644 --- a/docs/Corbits Triage User Manual/Corbits Triage User Manual.md +++ b/docs/Corbits Triage User Manual/Corbits Triage User Manual.md @@ -37,7 +37,7 @@ Nothing runs until you turn it on. Connecting GitHub or Bitbucket Data Center on ### Code review -Code review reads each new commit of a pull request for bugs: mistakes in the changed lines and breakage the change causes elsewhere in the repository. It is separate from the verdict and optional. +Code review reads each new commit of a pull request for bugs: mistakes in the changed lines and breakage the change causes elsewhere in the repository. A finding is posted only after a second read of the lines it points at confirms it. It is separate from the verdict and optional. - **Turning it on.** A repository's [policy file](#writing-the-file) turns it on: it then runs after triage, for the groups in `for` and on the verdicts and paths in `when`. A pull request they leave out, or one whose failed rules hold the review back, is not reviewed, and its **Code review** section says **Not reviewed:** and why, such as **Not reviewed: the author is not in the maintainers group**. It needs the workspace's Anthropic API key, saved on the [Code review page](#code-review-provider-and-places). - **What it costs.** Each commit it reviews is billed to your Anthropic account. It reviews a pull request when it is opened and after every push while it is not a draft, and **Review again**, offered once a pull request has a review, is one more review. A push that leaves the change exactly as it was reviewed, such as a rebase onto a newer base that changed none of the files it touches, is not reviewed again: the new commit keeps the last review, and the section says **Carried over from** the commit it was made for. **Review again** reviews it anyway. diff --git a/packages/pr-code-review-anthropic/evals/README.md b/packages/pr-code-review-anthropic/evals/README.md index c011b43ba..c4378ebb1 100644 --- a/packages/pr-code-review-anthropic/evals/README.md +++ b/packages/pr-code-review-anthropic/evals/README.md @@ -48,9 +48,13 @@ A clean case becomes a defect case only with a confirmed added bug. A bug is caught when a finding on its path overlaps any of its sites within three lines. A finding that hits no bug is a false alarm, so a complete key makes false alarms meaningful. A clean case is flagged when anything is -reported on it. `--e2e` scores the finders' union, the consolidated findings, -the confirmed ones (all of them unless `--check`), the final review, and the -review's comments found by both finders and by one. +reported on it. `--e2e` checks every finding, as every review does, and +scores the finders' union, the consolidated findings (what the review posted +before the check ran on every request), the ones the check confirmed, the +final review, and the review's comments found by both finders and by one. A +stage's precision is its findings that hit a bug over all its findings, so the +consolidated and final rows give precision and recall without the check and +with it from one run. ## Running @@ -64,8 +68,7 @@ bun run eval:review -- --e2e --out e2e.json Each step runs on the model it is deployed with: Opus 5.5 for the diff finder, Haiku 5.5 for the rest. `---model` overrides one. -`--cases a,b` runs a subset. `--check` turns the check step on, with -`--check-model` and `--check-effort` for it. Other modes, listed at the top of +`--cases a,b` runs a subset. Other modes, listed at the top of `review.eval.ts`: a finder alone with any prompt (`--prompt`) and rescoring saved finder runs (`--rescore`). diff --git a/packages/pr-code-review-anthropic/evals/review.eval.ts b/packages/pr-code-review-anthropic/evals/review.eval.ts index 15cf55979..03d746cd5 100644 --- a/packages/pr-code-review-anthropic/evals/review.eval.ts +++ b/packages/pr-code-review-anthropic/evals/review.eval.ts @@ -9,11 +9,12 @@ // bun run eval:review -- --rescore re-scores saved results without model calls // bun run eval:review -- --rescore [--out ] sweeps posting cutoffs over a saved --e2e run without // model calls (sweep.ts) and reports the loosest one at 85% precision and the precision/recall frontier. -// bun run eval:review -- --e2e [--cases a,b] [---model m] [---effort e] [--budget n] [--check] [--out ] [--baseline] +// bun run eval:review -- --e2e [--cases a,b] [---model m] [---effort e] [--budget n] [--out ] [--baseline] // with one of plan, diff-finder, repo-finder, consolidate, check, write; each role defaults to its deployed model; -// runs the whole pr-code-review workflow on each case's head and scores the finders' union, the consolidated findings, -// the confirmed ones (all of them unless --check), the final review, and its comments found by both finders and by -// one, with each step's tokens, dollars and seconds. --baseline, over every case, also writes +// runs the whole pr-code-review workflow on each case's head, checking every finding as every review does, and +// scores the finders' union, the consolidated findings (what the review posted before the check), the ones the check +// confirmed, the final review, and its comments found by both finders and by one, with each stage's precision and +// each step's tokens, dollars and seconds. --baseline, over every case, also writes // results/pr-code-review.baseline.json, refusing on a dirty working tree or when any case's repository is not public. // // Inference is native Anthropic when ANTHROPIC_API_KEY is set, else OpenCode Zen with OPENCODE_API_KEY. @@ -323,7 +324,6 @@ interface E2EConfig { models: Partial>; efforts: Record; budget?: number; - check: boolean; } function effortFlag(value: unknown, fallback: Effort): Effort { @@ -347,7 +347,6 @@ function e2eConfig(values: Record, provider: ProviderId): E2ECo models: Object.fromEntries(ROLES.flatMap(model)), efforts: Object.fromEntries(ROLES.map(effort)) as Record, ...(typeof values.budget === "string" && { budget: Number(values.budget) }), - check: values.check === true, }; } @@ -382,7 +381,7 @@ async function runE2ECase(testCase: Case, config: E2EConfig, source: Provider, g step.ms += ms; } const head = headOf(testCase.id); - const request = { kind: "code-review", title: testCase.title, description: testCase.description, diff: testCase.diff, checkout: { source: "github", ...head }, efforts: config.efforts, check: config.check, ...(config.budget && { budgets: { repoFinder: config.budget } }) }; + const request = { kind: "code-review", title: testCase.title, description: testCase.description, diff: testCase.diff, checkout: { source: "github", ...head }, efforts: config.efforts, ...(config.budget && { budgets: { repoFinder: config.budget } }) }; const started = performance.now(); const run = await runLocalWorkflow({ workflow: prCodeReview({ sources: GITHUB }), @@ -486,12 +485,15 @@ function e2eStageRow(stage: Stage, results: readonly E2EResult[], keys: readonly const ofReach = bugs.filter((bug) => bug.reach === reach); return [reach, ratio(ofReach.filter(isCaught).length, ofReach.length)]; } + const findings = sum(results, (r) => r.stages[stage]?.findings.length ?? 0); + const falseAlarms = sum(results, (r) => r.stages[stage]?.falseAlarms ?? 0); return { stage, bugsCaught: ratio(bugs.filter(isCaught).length, bugs.length), ...Object.fromEntries(REACHES.map(caughtOfReach)), - findings: sum(results, (r) => r.stages[stage]?.findings.length ?? 0), - falseAlarms: sum(results, (r) => r.stages[stage]?.falseAlarms ?? 0), + findings, + falseAlarms, + precision: percent(findings === 0 ? null : (findings - falseAlarms) / findings), cleanFlagged: ratio(results.filter((r) => r.stages[stage]?.flaggedClean).length, scored.filter((key) => key.kind === "clean").length), }; } @@ -616,7 +618,6 @@ async function main() { budget: { type: "string" }, e2e: { type: "boolean", default: false }, baseline: { type: "boolean", default: false }, - check: { type: "boolean", default: false }, ...Object.fromEntries(Object.values(ROLE_FLAGS).flatMap((flag) => [[`${flag}-model`, { type: "string" }], [`${flag}-effort`, { type: "string" }]])), }, }); diff --git a/packages/pr-code-review-anthropic/evals/sweep.test.ts b/packages/pr-code-review-anthropic/evals/sweep.test.ts index f216b5b98..41dbdfdfd 100644 --- a/packages/pr-code-review-anthropic/evals/sweep.test.ts +++ b/packages/pr-code-review-anthropic/evals/sweep.test.ts @@ -23,7 +23,6 @@ function saved(diff: object[], repo: object[], kept: Candidate[], groups: Group[ const consolidated = { groups, sites: kept.map(({ path, start, end }) => ({ path, start, end })), excerpts: kept.map(() => ""), unreadFindings: [] }; const findings = orderedGroups(kept, consolidated).map((o) => o.finding); return { - reviewInput: { request: {} }, findDiff, findRepo, candidates: { found: [...foundOf(findDiff, "diff"), ...foundOf(findRepo, "repo")], kept }, diff --git a/packages/pr-code-review-anthropic/evals/sweep.ts b/packages/pr-code-review-anthropic/evals/sweep.ts index a4127b513..acd59d9f0 100644 --- a/packages/pr-code-review-anthropic/evals/sweep.ts +++ b/packages/pr-code-review-anthropic/evals/sweep.ts @@ -41,7 +41,6 @@ export function posts(comment: Comment, cutoff: Cutoff): boolean { } export interface SavedOutputs { - reviewInput: { request: { check?: boolean } }; findDiff: { reply: string }; findRepo: { reply: string }; candidates: { found: Candidate[]; kept: Candidate[] }; @@ -95,7 +94,8 @@ function isReal(output: { reply: string }): boolean { export function commentsOf(outputs: SavedOutputs): Comment[] { const { kept } = outputs.candidates; const ordered = orderedGroups(kept, JSON.parse(outputs.consolidate.reply) as Consolidated); - const reported = outputs.reviewInput.request.check ? ordered.filter((_, i) => outputs.check[i] !== undefined && isReal(outputs.check[i])) : ordered; + // A run saved before every finding was checked has no check outputs and reported every group. + const reported = outputs.check.length === 0 ? ordered : ordered.filter((_, i) => outputs.check[i] !== undefined && isReal(outputs.check[i])); const confidences = keptConfidences(outputs); function comment(posted: SavedOutputs["review"]["comments"][number], index: number): Comment { const entry = reported[index]; diff --git a/packages/pr-code-review-anthropic/src/pr-code-review.test.ts b/packages/pr-code-review-anthropic/src/pr-code-review.test.ts index 1df4ea1c3..37770ef98 100644 --- a/packages/pr-code-review-anthropic/src/pr-code-review.test.ts +++ b/packages/pr-code-review-anthropic/src/pr-code-review.test.ts @@ -37,6 +37,8 @@ const SHA = "0123456789abcdef0123456789abcdef01234567"; const FILE = "export function add(a: number, b: number) {\n return a - b;\n}\n"; const DIFF = "diff --git a/src/a.ts b/src/a.ts\n--- a/src/a.ts\n+++ b/src/a.ts\n@@ -1,3 +1,3 @@\n export function add(a: number, b: number) {\n- return a + b;\n+ return a - b;\n }\n"; const FINDING = { path: "src/a.ts", start: 2, end: 2, priority: 1, confidence: 0.9, title: "add subtracts", why: "it uses -", trigger: "add(1, 1)", code: " return a - b;" }; +/** A candidate the check step answers not real. */ +const PLANTED = { path: "src/a.ts", start: 1, end: 1, priority: 2, confidence: 0.6, title: "add is not exported", why: "callers cannot import it", trigger: "import { add }", code: "export function add(a: number, b: number) {" }; let dir: string; let tarball: Uint8Array; @@ -50,6 +52,10 @@ let repoFinderProse = false; let planRefused = false; /** How many of the write step's next answers are prose with no JSON. */ let writeProse = 0; +/** Whether both finders also report `PLANTED`. */ +let planted = false; +/** How many of the check step's next answers name no verdict it knows. */ +let checkUnsure = 0; const allow: WorkflowAuthorizeFn = async function allow() { return { effect: "allow", matchingGrants: [], resolvedBy: null }; @@ -89,8 +95,13 @@ function answer(body: MessagesRequest) { return [{ type: "text", text: "One defect: add subtracts." }]; } if (system.includes("") && repoFinderProse) return [{ type: "text", text: "I read src/a.ts and ran out of budget before writing findings." }]; - let text: unknown = { findings: [FINDING] }; + let text: unknown = { findings: planted ? [FINDING, PLANTED] : [FINDING] }; if (system.includes("")) text = { groups: [{ defect: "add subtracts", members: [0], representative: 0 }] }; + if (sent.includes("Is this defect real?") && checkUnsure > 0) { + checkUnsure--; + return [{ type: "text", text: JSON.stringify({ verdict: "unsure" }) }]; + } + if (sent.includes("Is this defect real?")) text = sent.includes(PLANTED.title) ? { verdict: "not_real", reason: "Line 1 exports add." } : { verdict: "real", reason: "Line 2 subtracts." }; if (sent.includes("Plan the review")) text = { areas: [], outside_diff: [] }; if (sent.includes("Write the review.")) text = { summary: "One defect.", comments: [{ finding: 0, title: "add subtracts", body: "Use +." }] }; return [{ type: "text", text: JSON.stringify(text) }]; @@ -227,3 +238,27 @@ test("a write answer that isn't JSON fails its step, which asks again", async () expect(run.terminalStatus).toBe("completed"); expect(run.outputs["review"]).toMatchObject({ summary: "One defect." }); }); + +test("a candidate the check answers not real is left out of the review and kept with the check's verdict and reason", async () => { + planted = true; + try { + const { run, attempts } = await review(join(dir, "data")); + expect(run.terminalStatus).toBe("completed"); + expect(attempts["code-review-check"]).toBe(2); + expect(run.outputs["review"]).toMatchObject({ + comments: [{ path: "src/a.ts", start: 2, title: "add subtracts" }], + dropped: [{ path: "src/a.ts", start: 1, title: "add is not exported", verdict: "not_real", reason: "Line 1 exports add." }], + }); + } finally { + planted = false; + } +}); + +test("a check answer that is not a verdict and reason fails its step, which asks again", async () => { + checkUnsure = 1; + const { run, attempts } = await review(join(dir, "data")); + expect(checkUnsure).toBe(0); + expect(attempts["code-review-check"]).toBe(2); + expect(run.terminalStatus).toBe("completed"); + expect(run.outputs["review"]).toMatchObject({ comments: [{ path: "src/a.ts", start: 2 }], dropped: [] }); +}); diff --git a/packages/pr-code-review/README.md b/packages/pr-code-review/README.md index 35edbef62..f6fbd53cb 100644 --- a/packages/pr-code-review/README.md +++ b/packages/pr-code-review/README.md @@ -13,7 +13,7 @@ under `prefix`, as `codeReviewReviewer`, the review a pull request's manager dep The request is `{ kind: "code-review", title, description, diff, checkout }`. The title and description are as the host shows them, without hidden text. Optional fields are `number` (the pull request's), `efforts` per step, -`budgets.repoFinder`, `check`, and `cut` (`{ lines, of, unread? }`) when the sender cut the diff for size, `unread` +`budgets.repoFinder`, and `cut` (`{ lines, of, unread? }`) when the sender cut the diff for size, `unread` naming the files it cut part-way or left out; the review carries it and never puts those names in a prompt. ### Process @@ -32,9 +32,11 @@ naming the files it cut part-way or left out; the review carries it and never pu finders reported each finding. 4. **consolidate** reads the lines each finding cites at the head, moves it onto its quoted line and groups the findings by defect. A group none of whose findings had a line of its file shown to the model is marked `unverified`. -5. **check** confirms each group, only when the request sets `check`. -6. **write** comments on every group, most severe and most agreed-on first; each comment carries the code it quotes - and how many finders reported it. +5. **check** reads each group's candidate, the code it quotes and the file lines around it in one model call per + group, on every review, and answers real or not real with a reason. A group it finds not real is + left out of the review and kept in its `dropped`, in its finder's words with the verdict and reason. +6. **write** comments on every group left, most severe and most agreed-on first; each comment carries the code it + quotes and how many finders reported it. A failed model call fails its step, as does an answer that can't be read; every model step then tries again twice, 5 and 10 seconds later. A step that fails for good fails the review, which shows as not finished. diff --git a/packages/pr-code-review/src/actions/review.test.ts b/packages/pr-code-review/src/actions/review.test.ts index d66ee6107..3e032f295 100644 --- a/packages/pr-code-review/src/actions/review.test.ts +++ b/packages/pr-code-review/src/actions/review.test.ts @@ -36,7 +36,7 @@ function reply(value: unknown) { /** Every message the review builds for one pull request, its findings and the consolidate step's excerpts. */ async function messages(request: { title: string; description: string; diff: string; cut?: { lines: number; of: number; unread: string[] } }, plan: string, finding: Record, excerpt: string) { - const planned = await reviewInputFor({ efforts })(stockTriggerMail({ kind: "code-review", ...request, checkout, check: true })); + const planned = await reviewInputFor({ efforts })(stockTriggerMail({ kind: "code-review", ...request, checkout })); const reviewInput = { output: planned }; const finders = await findersInput({ reviewInput, plan: reply(plan) }); const folded = await candidates({ reviewInput, findDiff: reply({ findings: [finding] }), findRepo: reply({ findings: [] }) }); @@ -130,13 +130,13 @@ function readsFrom(files: Record) { }; } -/** The review of one diff finder reply over `files`, the consolidate model replying `groups`. */ +/** The review of one diff finder reply over `files`, the consolidate model replying `groups` and the check finding each real. */ async function reviewed(findings: unknown[], files: Record, groups: unknown[] = []) { const reviewInput = { output: await reviewInputFor({ efforts })(stockTriggerMail({ kind: "code-review", title: "t", description: "d", checkout, diff: diffOf("src/a.ts", "x") })) }; const folded = await candidates({ reviewInput, findDiff: reply({ findings }), findRepo: reply({ findings: [] }) }); const consolidate = reply((await consolidated(folded.consolidate, readsFrom(files), groups)).replied); const checked = await checks({ reviewInput, candidates: { output: folded }, consolidate }); - const written = await writeInput({ reviewInput, candidates: { output: folded }, checks: { output: checked }, check: { output: [] } }); + const written = await writeInput({ reviewInput, candidates: { output: folded }, checks: { output: checked }, check: { output: checked.checks.map(() => reply({ verdict: "real", reason: "r" }).output) } }); return review({ reviewInput, writeInput: { output: written }, write: reply({ summary: "One defect.", comments: [{ finding: 0, title: "wrong total", body: "b" }] }) }); } diff --git a/packages/pr-code-review/src/actions/review.ts b/packages/pr-code-review/src/actions/review.ts index aa4dba08a..8c9162fce 100644 --- a/packages/pr-code-review/src/actions/review.ts +++ b/packages/pr-code-review/src/actions/review.ts @@ -14,7 +14,6 @@ const ReviewRequest = type({ diff: "string", checkout: Checkout, "efforts?": { "plan?": Effort, "diffFinder?": Effort, "repoFinder?": Effort, "consolidate?": Effort, "check?": Effort, "write?": Effort }, - "check?": "boolean", "budgets?": { "repoFinder?": "number.integer > 0" }, /** The pull request's number, so a run can be found by it. */ "number?": "number.integer > 0", @@ -84,20 +83,24 @@ export interface Group { representative: number; } -interface Verdict { - verdict: "real" | "not_real"; - reason: string; -} +/** The check step's answer; its director refuses any other, so the step asks again. */ +export const Verdict = type({ verdict: "'real' | 'not_real'", reason: "string" }); +type Verdict = typeof Verdict.infer; interface Checks { findings: Reported[]; checks: { text: string; inference: StepInference }[]; } +/** A finding the check found not real: left out of the review, kept with the check's verdict and reason. */ +interface Dropped extends Verdict { + finding: Reported; +} + interface WriteInput { budgetExhausted: boolean; - verdicts: Verdict[]; findings: Reported[]; + dropped: Dropped[]; text: string; inference: StepInference; } @@ -293,7 +296,7 @@ export function orderedGroups(kept: readonly Candidate[], { groups, sites, excer return joinedAtAnchors(groupsOf(groups, kept.length), kept, sites).map(reported).sort((a, b) => bySeverityThenAgreement(a.finding, b.finding)); } -/** Every consolidated finding, ordered by severity then agreement, and one check of each when the request asks for the check. */ +/** Every consolidated finding, ordered by severity then agreement, and one check of each. */ export async function checks(steps: Steps): Promise { const { request, inference } = stepOutput(steps, "reviewInput"); const { kept } = stepOutput(steps, "candidates"); @@ -301,20 +304,22 @@ export async function checks(steps: Steps): Promise { function check({ finding, excerpt }: (typeof ordered)[number]) { return { text: checkMessage(request, finding, excerpt), inference: inference.check }; } - return { findings: ordered.map((o) => o.finding), checks: request.check ? ordered.map(check) : [] }; + return { findings: ordered.map((o) => o.finding), checks: ordered.map(check) }; } -/** The findings to report, all of them or, when the request asks for the check, those it found real, and the write step's user message. */ +/** The findings the check found real, to report, those it found not real with its verdict and reason, and the write step's user message. */ export async function writeInput(steps: Steps): Promise { const { request, inference } = stepOutput(steps, "reviewInput"); const { budgetExhausted } = stepOutput(steps, "candidates"); const { findings: checked } = stepOutput(steps, "checks"); const verdicts = stepOutput(steps, "check").map((output) => jsonReply(output)); - const findings = request.check ? checked.filter((_, i) => verdicts[i]?.verdict === "real") : checked; + const judged = checked.map((finding, i) => ({ finding, ...verdicts[i]! })); + const findings = judged.flatMap((j) => (j.verdict === "real" ? [j.finding] : [])); + const dropped = judged.filter((j) => j.verdict !== "real"); const listed = findings.map(({ path, start, end, title, why, trigger, code }, finding) => ({ finding, path, start, end, title, why, trigger, code })); const change = `\n${promptText(request.title)}\n${promptText(request.description)}\n\n${promptText(changedFiles(request.diff).join("\n"))}\n\n`; const text = `${change}\n\n\n${promptText(JSON.stringify(listed, null, 2))}\n\n\nWrite the review.`; - return { budgetExhausted, verdicts, findings, text, inference: inference.write }; + return { budgetExhausted, findings, dropped, text, inference: inference.write }; } interface Written { @@ -328,19 +333,27 @@ function ownWords({ title, defect, why, trigger, path, start }: Reported) { return { title: heading, body: [why, trigger].filter((part) => part !== undefined).join("\n\n") }; } +/** A finding as the review carries it, in the words given or else its finder's. */ +function commentOf(finding: Reported, { title, body }: { title: string; body: string }) { + const { path, start, end, agreement, priority, code, unverified } = finding; + return { path, start, end, title, body, ...(priority !== undefined && { priority }), ...(code !== undefined && { quoted: code }), agreement, ...(unverified && { unverified }) }; +} + /** * The review: the summary, one comment per finding in order, anchored where the finding is, with the code it quotes, its - * agreement and whether its lines went unread, and whether it stopped early. A finding the write step left without a - * comment gets one in its finder's words. + * agreement and whether its lines went unread, whether it stopped early, and each finding the check dropped, in its + * finder's words with the check's verdict and reason. A finding the write step left without a comment gets one in its + * finder's words. */ export async function review(steps: Steps) { const { request } = stepOutput(steps, "reviewInput"); - const { findings, budgetExhausted } = stepOutput(steps, "writeInput"); + const { findings, dropped, budgetExhausted } = stepOutput(steps, "writeInput"); const written = jsonReply(stepOutput(steps, "write")); function comment(finding: Reported, index: number) { - const { path, start, end, agreement, priority, code, unverified } = finding; - const { title, body } = written.comments.find((c) => c.finding === index) ?? ownWords(finding); - return { path, start, end, title, body, ...(priority !== undefined && { priority }), ...(code !== undefined && { quoted: code }), agreement, ...(unverified && { unverified }) }; + return commentOf(finding, written.comments.find((c) => c.finding === index) ?? ownWords(finding)); + } + function droppedOf({ finding, verdict, reason }: Dropped) { + return { ...commentOf(finding, ownWords(finding)), verdict, reason }; } - return { summary: written.summary, comments: findings.map(comment), budgetExhausted, ...(request.cut && { cut: request.cut }) }; + return { summary: written.summary, comments: findings.map(comment), dropped: dropped.map(droppedOf), budgetExhausted, ...(request.cut && { cut: request.cut }) }; } diff --git a/packages/pr-code-review/src/pr-code-review.ts b/packages/pr-code-review/src/pr-code-review.ts index 43a7cb1b4..8dc261405 100644 --- a/packages/pr-code-review/src/pr-code-review.ts +++ b/packages/pr-code-review/src/pr-code-review.ts @@ -30,8 +30,8 @@ export interface CodeReviewPlace { /** * reviewInput → plan ‖ checkout (→ lint) → findDiff ‖ findRepo → candidates (fold exact duplicates) → consolidate (read the cited - * lines at the head, group findings by defect) → checks (every group, by severity then agreement) → check (one call per group, - * only when the request asks for it) → writeInput → write → review (the review to post). + * lines at the head, group findings by defect) → checks (every group, by severity then agreement) → check (one call per + * group) → writeInput (the groups it found real) → write → review (the review to post, and the groups the check dropped). * Each model step's effort comes from the request, else the profile; its model is the one the profile names for it. * Under a prefix, every action's input also carries it as `$prefix`, so the action finds its steps' outputs. */ diff --git a/packages/pr-code-review/src/structured-director.ts b/packages/pr-code-review/src/structured-director.ts index d17a8a431..5fcfdd279 100644 --- a/packages/pr-code-review/src/structured-director.ts +++ b/packages/pr-code-review/src/structured-director.ts @@ -1,6 +1,7 @@ import { defineDirector } from "@intx/agent"; import type { AssistantTurn, ReactorCapabilities, ReactorDirector, ReactorInboundEvent } from "@intx/types/runtime"; -import { type } from "arktype"; +import { type, type Type } from "arktype"; +import { Verdict } from "./actions/review.js"; import { MODEL_FAILED } from "./failures.js"; import { packageName } from "./package-name.js"; import type { CodeReviewProfile } from "./profile.js"; @@ -10,22 +11,29 @@ export interface StructuredDirectorConfig { schema: Record; } +/** The steps whose answer must hold more than the response format guarantees. */ +const ANSWERS: Record = { check: Verdict }; + function turnText(turn: AssistantTurn): string { return turn.content.flatMap((b) => (b.type === "text" ? [b.text] : [])).join("\n").trim(); } /** * One model call with the agent's system prompt and no tools whose answer is decoded against the schema; the reply is - * the model's JSON. A failed model call or an answer that isn't JSON fails the step, so its retry asks again. + * the model's JSON. A failed model call, an answer that isn't JSON or, for the check, one that is not a verdict and + * reason fails the step, so its retry asks again. */ function structuredDirector(caps: ReactorCapabilities, systemPrompt: string, { name, schema }: StructuredDirectorConfig): ReactorDirector { function reply(turn: AssistantTurn) { const text = turnText(turn); + let answer: unknown; try { - JSON.parse(text); + answer = JSON.parse(text); } catch { throw new Error(`${name}: replied without JSON: ${text.slice(0, 200)}`); } + const checked = ANSWERS[name]?.(answer); + if (checked instanceof type.errors) throw new Error(`${name}: ${checked.summary}`); return caps.reply(text); } diff --git a/packages/triage-contracts/src/code-review.ts b/packages/triage-contracts/src/code-review.ts index 8b2c72644..c457450f5 100644 --- a/packages/triage-contracts/src/code-review.ts +++ b/packages/triage-contracts/src/code-review.ts @@ -24,8 +24,14 @@ export type CodeReviewComment = typeof CodeReviewComment.infer; /** A diff cut to its first `lines` of `of` lines for size, with the files it cut part-way or left out. */ export const DiffCut = type({ lines: "number", of: "number", "unread?": "string[]" }); -/** What a code review run answers: its summary, its findings, whether its reader stopped early and how its diff was cut. */ -export const CodeReviewOutput = type({ summary: "string", comments: CodeReviewComment.array(), budgetExhausted: "boolean", "cut?": DiffCut }); +/** A finding the review's check found not real, left out of the review with the check's reason. */ +const CodeReviewDropped = CodeReviewComment.and({ verdict: "'not_real'", reason: "string" }); + +/** + * What a code review run answers: its summary, its findings, the findings its check dropped, whether its reader stopped + * early and how its diff was cut. + */ +export const CodeReviewOutput = type({ summary: "string", comments: CodeReviewComment.array(), "dropped?": CodeReviewDropped.array(), budgetExhausted: "boolean", "cut?": DiffCut }); export type CodeReviewOutput = typeof CodeReviewOutput.infer; /** A finding's title as people read it, after its priority when it has one. */