diff --git a/README.md b/README.md index 33758759..63ec8260 100644 --- a/README.md +++ b/README.md @@ -1126,7 +1126,12 @@ base gained since, which a diff against its tip shows as reverted. The first round runs the reviewer's own review, `codex exec review --base ` or Claude Code's `/code-review ...HEAD`, then resumes that session for one turn that applies the base's `REVIEW.md`, if any, and reports the findings -in one JSON schema. Later rounds resume the same session with the new head and +in one JSON schema, with a score from 0 to 10 for how ready the head is to land +and one sentence on why: 10 lands as is, 7 to 9 lands after the findings are +fixed, 4 to 6 needs rework, and 0 to 3 is the wrong approach. A head with no +findings can still score below 10. A reply without a valid score fails like any +malformed reply. The score informs; the findings and their refutations still +decide when the loop ends. Later rounds resume the same session with the new head and `--responses`, the author's free-text fixes and refutations, which a round that reported findings requires. A refutation stands only when it cites evidence the reviewer can check. A resumed session keeps the instructions it started with, diff --git a/assets/CONTEXT.md b/assets/CONTEXT.md index 8c1f4e9d..699ce76b 100644 --- a/assets/CONTEXT.md +++ b/assets/CONTEXT.md @@ -133,7 +133,8 @@ Once the reset passes or the login is restored, the item has cleared: whoever fi The PR body's `## Models` section records the PR's size and each implementing session's harness, model, and effort. A quest PR's size is its quest's; a skill that opens its own PR, such as `/quest-plan`, sizes it as a quest is sized. -The PR body's `## Review` section records the latest round: the reviewer harness, model, and effort, the reviewed head SHA, and each finding's disposition (fixed, or refuted with the reason), or the skip and its reason. +The PR body's `## Review` section records the latest round: the reviewer harness, model, and effort, the reviewed head SHA, the round's score out of 10 with its rationale, and each finding's disposition (fixed, or refuted with the reason), or the skip and its reason. +Never block on a score: the findings and their refutations decide when the review is done. Rewrite it after each round. A review counts for the head whose SHA it names. diff --git a/assets/skills/review.md b/assets/skills/review.md index 9f08b3df..332380e6 100644 --- a/assets/skills/review.md +++ b/assets/skills/review.md @@ -30,7 +30,8 @@ Push, then flip the PR to "Ready for Review" with `quest ready `, wh For your own draft, mark it ready for review yourself. Do not wait for online reviewers. -Summarize the review and CI from `quest status `, naming any decisions it raised without answering them, then interactively prompt the user with your recommended action: +Summarize the review, with its last round's score and rationale, and summarize CI from `quest status `. +Name any decisions the review raised without answering them, then interactively prompt the user with your recommended action: - `/quest-merge`: If the PR is landable as `/quest-merge` defines it, or will be once fixed with no decision left for the user, such as failing checks or unaddressed automated review findings; `/quest-merge` runs `/quest-iterate` on it first. - `/quest-iterate`: If it raised significant issues or decisions, which it settles with the grill skill's interview. - skip: If the PR should not be merged yet. diff --git a/cloudflare/GITHUB.md b/cloudflare/GITHUB.md index 7f53d50a..f0657824 100644 --- a/cloudflare/GITHUB.md +++ b/cloudflare/GITHUB.md @@ -376,16 +376,17 @@ reports the round over `POST /api/runner/review`. The agent never touches the report, so it can neither skip nor fake the review. A merge job publishes its fixes, has them reviewed, and lands only a head whose review found nothing. -kix records the round as the App's `quest review` check run on the commit, -with the reviewer, model, effort, and round as its summary and each finding as -plain text: `success` when the round found nothing, `failure` otherwise. The -agent fixes or refutes each finding and delivers again; the next round resumes -the reviewer's session with its responses. kix records a report only from the -job's current attempt, only on the head kix itself last published for the job -and GitHub attributes to a bot, not one a merge job continued onto, -and only from another vendor's model than the job's account runs, and writes it -with `checks: write` on behalf of the job's requester, after rechecking their -maintainer authority. +kix records the round as the App's `quest review` check run on the commit, with +the reviewer, model, effort, and round as its summary, the round's score out of +10 in its title, and the score's rationale and each finding as plain text, +cutting any too long to fit rather than refusing the report: `success` when the +round found nothing, `failure` otherwise, whatever the score. The agent fixes or +refutes each finding and delivers again; the next round resumes the reviewer's +session with its responses. kix records a report only from the job's current +attempt, only on the head kix itself last published for the job and GitHub +attributes to a bot, not one a merge job continued onto, and only from another +vendor's model than the job's account runs, and writes it with `checks: write` +on behalf of the job's requester, after rechecking their maintainer authority. Claude Code reviews with one of the runner's Claude Code accounts that is not limited. Runners hold no Codex accounts, so Codex reviews with the login of diff --git a/cloudflare/src/jobs/review.ts b/cloudflare/src/jobs/review.ts index 9af34d9e..05a33347 100644 --- a/cloudflare/src/jobs/review.ts +++ b/cloudflare/src/jobs/review.ts @@ -29,15 +29,31 @@ export type ReviewReport = { model: string; effort: string; round: number; + /** How ready the head is to land, from 0 to 10, and why. */ + score: number; + rationale: string; findings: Finding[]; }; -/** Bounds a report: GitHub keeps at most 65,535 characters of a check run's text. */ -export const REVIEW_LIMITS = { findings: 100, title: 300, body: 8000 }; +/** + * Bounds a report: GitHub keeps at most 65,535 characters of a check run's + * text. Only that text shows a report's prose, so prose past its bound is cut + * to fit rather than refused, which would leave a head `quest review` accepted + * unrecorded. + */ +export const REVIEW_LIMITS = { findings: 100, file: 1024, title: 300, body: 8000, rationale: 8000 }; const count = (value: unknown, min: number) => typeof value === "number" && Number.isSafeInteger(value) && value >= min ? value : null; +/** A string cut to at most `max` characters, ending in an ellipsis when cut, or null for any other value. */ +function clip(value: unknown, max: number) { + if (typeof value !== "string") return null; + if (value.length <= max) return value; + // Never leave half of a surrogate pair at the cut. + return `${value.slice(0, max - 1).replace(/[\ud800-\udbff]$/, "")}…`; +} + /** Parse a runner's review report, or throw a JobError the runner can read. */ export function parseReview(body: Record): ReviewReport { const job = id(body.job); @@ -51,19 +67,23 @@ export function parseReview(body: Record): ReviewReport { const effort = text(body.effort, 50); const round = count(body.round, 1); if (!model || !effort || !round) throw new JobError("The reviewer's model, effort, and round are required"); + const score = count(body.score, 0); + const rationale = clip(body.rationale, REVIEW_LIMITS.rationale); + if (score === null || score > 10 || rationale === null || !(body.rationale as string).trim()) + throw new JobError("A review needs a score from 0 to 10 and a rationale"); if (!Array.isArray(body.findings) || body.findings.length > REVIEW_LIMITS.findings) throw new JobError(`At most ${REVIEW_LIMITS.findings} findings per review`); const findings = body.findings.map((item: Record | null): Finding => { - const file = text(item?.file, 1024); - const title = text(item?.title, REVIEW_LIMITS.title); - const detail = typeof item?.body === "string" && item.body.length <= REVIEW_LIMITS.body ? item.body : null; + const file = clip(item?.file, REVIEW_LIMITS.file); + const title = clip(item?.title, REVIEW_LIMITS.title); + const detail = clip(item?.body, REVIEW_LIMITS.body); const line = item?.line === null ? null : count(item?.line, 1); const severity = SEVERITIES.find((s) => s === item?.severity); - if (!file || !title || detail === null || (item?.line !== null && line === null) || !severity) + if (file === null || title === null || detail === null || (item?.line !== null && line === null) || !severity) throw new JobError("Each finding needs a file, line or null, severity, title, and body"); return { file, line, severity, title, body: detail }; }); - return { job, attempt, head: body.head, reviewer, model, effort, round, findings }; + return { job, attempt, head: body.head, reviewer, model, effort, round, score, rationale, findings }; } /** Untrusted text inside a fence no run of backticks in it can close. */ @@ -73,17 +93,21 @@ function fenced(value: string) { return `${fence}text\n${value.replace(/[\x00-\x08\x0b-\x1f\x7f]/g, "")}\n${fence}`; } -/** The check run's output: a title, a summary naming the reviewer, and the findings as plain text. */ +/** + * The check run's output: a title with the findings count and score, a + * summary naming the reviewer, and the score's rationale and the findings as + * plain text. + */ export function checkOutput(report: ReviewReport) { const n = report.findings.length; - const title = n ? `${n} finding${n === 1 ? "" : "s"}` : "No findings"; + const title = `${n ? `${n} finding${n === 1 ? "" : "s"}` : "No findings"}, scored ${report.score}/10`; const summary = `A kix runner reviewed this commit with \`quest review\`: ${report.reviewer}, ${report.model} at ${report.effort} effort, round ${report.round}.`; const findings = report.findings.map((f) => fenced(`${f.severity} ${f.line === null ? f.file : `${f.file}:${f.line}`} ${f.title}\n\n${f.body}`), ); - let text = ""; + let text = fenced(`score ${report.score}/10: ${report.rationale}`); for (const [i, finding] of findings.entries()) { - const next = `${text}${text ? "\n\n" : ""}${finding}`; + const next = `${text}\n\n${finding}`; // GitHub refuses a check run's text past 65,535 characters. if (next.length > 60_000) { text += `\n\n${findings.length - i} more not shown.`; @@ -91,7 +115,7 @@ export function checkOutput(report: ReviewReport) { } text = next; } - return { title, summary, ...(text && { text }) }; + return { title, summary, text }; } /** diff --git a/cloudflare/test/publish.test.ts b/cloudflare/test/publish.test.ts index c863e87b..91654bd6 100644 --- a/cloudflare/test/publish.test.ts +++ b/cloudflare/test/publish.test.ts @@ -5,6 +5,7 @@ import { GitHubService } from "../src/github/service"; import { app } from "../src/index"; import * as landing from "../src/jobs/landing"; import { refusal, restart, validPath, validateContinuation } from "../src/jobs/publish"; +import { REVIEW_LIMITS } from "../src/jobs/review"; import * as store from "../src/jobs/store"; import { type Repo, type World, blob, commitOf, fakeGitHub, intercept as around } from "../dev/github"; import { privateKey } from "./github-key"; @@ -1811,6 +1812,8 @@ describe("merge jobs", () => { model: "gpt-6.1-sol", effort: "medium", round: 1, + score: 10, + rationale: "Lands as is.", findings: [], ...body, }); @@ -1851,7 +1854,13 @@ describe("merge jobs", () => { it("records findings as a failing check, which lands nothing", async () => { const id = await merging(false); const head = await fix(id); - const failed = await report({ job: id, head, findings: [finding] }); + const failed = await report({ + job: id, + head, + score: 4, + rationale: "Breaks on `null`.", + findings: [finding], + }); expect(await failed.json()).toEqual({ conclusion: "failure" }); expect(project.checkRuns).toEqual([ { @@ -1860,10 +1869,12 @@ describe("merge jobs", () => { conclusion: "failure", app: Number(env.GITHUB_APP_ID), output: { - title: "1 finding", + title: "1 finding, scored 4/10", summary: "A kix runner reviewed this commit with `quest review`: codex, gpt-6.1-sol at medium effort, round 1.", - text: "````text\nhigh a:1 Breaks on null\n\nEnds a fence: ``` and mentions @maintainer\n````", + text: + "```text\nscore 4/10: Breaks on `null`.\n```\n\n" + + "````text\nhigh a:1 Breaks on null\n\nEnds a fence: ``` and mentions @maintainer\n````", }, }, ]); @@ -1887,14 +1898,44 @@ describe("merge jobs", () => { const passed = await report({ job: id, head, round: 2 }); expect(await passed.json()).toEqual({ conclusion: "success" }); expect(project.checkRuns!.at(-1)!.output).toEqual({ - title: "No findings", + title: "No findings, scored 10/10", summary: "A kix runner reviewed this commit with `quest review`: codex, gpt-6.1-sol at medium effort, round 2.", + text: "```text\nscore 10/10: Lands as is.\n```", }); expect(await land(id, head)).toBeNull(); expect(lands().map((l) => l.variables.sha)).toEqual([head]); }); + it("cuts prose too long for the check's text instead of refusing the report", async () => { + const id = await merging(false); + const head = await fix(id); + const long = (n: number) => "x".repeat(n + 1); + const cut = (n: number) => `${"x".repeat(n - 1)}…`; + const failed = await report({ + job: id, + head, + score: 2, + rationale: long(REVIEW_LIMITS.rationale), + findings: [ + { + ...finding, + file: long(REVIEW_LIMITS.file), + line: null, + title: long(REVIEW_LIMITS.title), + body: "", + }, + { ...finding, title: "", body: long(REVIEW_LIMITS.body) }, + ], + }); + expect(await failed.json()).toEqual({ conclusion: "failure" }); + expect(project.checkRuns!.at(-1)!.output.text).toBe( + `\`\`\`text\nscore 2/10: ${cut(REVIEW_LIMITS.rationale)}\n\`\`\`\n\n` + + `\`\`\`text\nhigh ${cut(REVIEW_LIMITS.file)} ${cut(REVIEW_LIMITS.title)}\n\n\n\`\`\`\n\n` + + `\`\`\`text\nhigh a:1 \n\n${cut(REVIEW_LIMITS.body)}\n\`\`\``, + ); + }); + it("refuses a head the job did not publish, a stale attempt, its own vendor, and malformed findings", async () => { const id = await merging(false); const refused = async (body: Record, error: string) => { @@ -1917,6 +1958,11 @@ describe("merge jobs", () => { "Each finding needs a file, line or null, severity, title, and body", ); await refused({ head, reviewer: "gemini" }, "The reviewer is claude or codex"); + const unscored = "A review needs a score from 0 to 10 and a rationale"; + await refused({ head, score: 11 }, unscored); + await refused({ head, score: 7.5 }, unscored); + await refused({ head, score: undefined }, unscored); + await refused({ head, rationale: " " }, unscored); // A head someone else committed, which the job's branch holds after a merge job continues onto it. project.commits[head]!.committer = "contributor"; const foreign = await report({ job: id, head }); diff --git a/quest/m0/model-routing/README.md b/quest/m0/model-routing/README.md index b2c8577e..faad6130 100644 --- a/quest/m0/model-routing/README.md +++ b/quest/m0/model-routing/README.md @@ -78,6 +78,5 @@ are mostly judgment. - [Grok agents](/quest/m0/model-routing/agent-harnesses.md) - `quest agent` starts Grok as well as Claude Code and Codex - [Route by size](/quest/m0/model-routing/models.md) - `/quest-spawn` starts each quest on the first entry of its size's ranked list that can run -- [Review score](/quest/m0/model-routing/review-score.md) - every `quest review` round scores the change out of 10 - [Commits and PRs record their models](/quest/m0/model-routing/implementer.md) - commit trailers and PR records name each harness, model, and effort, and the PR's size - [moq-dev pins](/quest/m0/model-routing/pins.md) - moq-dev/moq and moq-dev/moq.pro bump their `quest` pin to pick up the routing diff --git a/quest/m0/model-routing/review-score.md b/quest/m0/model-routing/review-score.md deleted file mode 100644 index dc4a586e..00000000 --- a/quest/m0/model-routing/review-score.md +++ /dev/null @@ -1,29 +0,0 @@ -# [S] Reviews score the change out of 10 - -## Goal - -Every `quest review` round reports, beside its findings, a score from 0 to -10 for how ready the reviewed head is to land, with one sentence on why. -The PR body's `## Review` record shows each round's score, so the -[model check](/quest/m1/model-check.md) can compare implementers and -reviewers by score as well as by rounds and findings. - -## Plan - -Decided 2026-11-04 (user, /quest-plan of `/quest-council`): each -`/quest-review` includes a score out of 10. It applies to single reviews -now; [the council](/quest/m2/council.md) reports each member's score. - -- Add a required integer `score` (0 to 10) and a `rationale` string to - `SCHEMA` in `src/cli/review.ts`, asked for in the schema turn, and print - `score ` beside `findings `. A reply without a valid score fails - as a malformed reply does. -- Define the scale in the reviewer prompt so every reviewer reads it the - same way: 10 lands as is, 7 to 9 lands after the findings are fixed, - 4 to 6 needs rework, 0 to 3 is the wrong approach. A round with no - unrefuted findings can still score below 10, for example a working - change that is harder to maintain than it needs to be. -- `quest context`'s Reviews section adds the score to the `## Review` - record and to the score the user sees in `/quest-review`'s summary. - Never block on a score: the findings and their refutations still decide - the loop. diff --git a/quest/m1/usage.md b/quest/m1/usage.md index b7d173ae..b313496c 100644 --- a/quest/m1/usage.md +++ b/quest/m1/usage.md @@ -40,8 +40,7 @@ Agent's choices, for the implementer to revisit: and branch: a quest branch names its quest, and a PR's head branch names the PR. Sessions on no quest or PR branch roll up as unattributed. - Accuracy comes from the records: commit trailers and the PR body's - size, implementers, and each `## Review` round's reviewer, score - ([review score](/quest/m0/model-routing/review-score.md)), and findings + size, implementers, and each `## Review` round's reviewer, score, and findings with their dispositions; follow-up fixes are later PRs that list it under `## Caused by` (`quest context`'s Pull requests section). - Prices are a pinned table beside the model lists, by model and token diff --git a/quest/m2/council.md b/quest/m2/council.md index 968b882e..d1a4a2cf 100644 --- a/quest/m2/council.md +++ b/quest/m2/council.md @@ -22,8 +22,8 @@ a token cost the user chooses per PR: - Independent only - How are findings combined? - ✅ Merged, duplicates folded, each tagged with its agreement count - (user); every review also scores the change out of 10 - ([review score](/quest/m0/model-routing/review-score.md)) + (user); every review also scores the change out of 10, as + `quest review` already does - Merged, then a blind vote that drops findings most members reject - Who sits on it? - ✅ One reviewer per maker, minus the implementers' makers: Sol and @@ -62,7 +62,6 @@ Agent's choices, for the implementer to revisit: ## Required - [Grok and opencode reviewers](/quest/m2/review-harnesses.md) - the council's xAI and DeepSeek members -- [Review score](/quest/m0/model-routing/review-score.md) - the score each member reports ## Related diff --git a/src/cli/main.ts b/src/cli/main.ts index 9dce84e6..46c5b3e6 100644 --- a/src/cli/main.ts +++ b/src/cli/main.ts @@ -1010,7 +1010,9 @@ export function main(argv: string[], out: Output): number | Promise { "each directory holding a changed path, never the checkout's, so a change cannot instruct its own " + "reviewer. The base is fetched first, since the sandbox has no network.\n\n" + "Prints the reviewer, model, and effort, the size and where it came from, session, round, base, head, a usage line per turn (input tokens, the " + - "part read from the prompt cache, and output), the finding count, then each finding. --json prints " + + "part read from the prompt cache, and output), a score from 0 to 10 for how ready the head is to land with " + + "its one-sentence rationale, the finding count, then each finding. A reply without a valid score " + + "fails as any malformed reply does. --json prints " + "the same as one object. Exits 0 once the round finishes, whatever it found, 2 for invalid arguments " + "or a checkout or CLI that cannot be reviewed, including a login the API rejects, 3 when the reviewer's " + "account hit its usage limit, and 1 for other failures. A usage limit prints one line, " + diff --git a/src/cli/review.ts b/src/cli/review.ts index 1b4faaec..92d3bb5c 100644 --- a/src/cli/review.ts +++ b/src/cli/review.ts @@ -3,7 +3,7 @@ // // A branch's first round runs the reviewer CLI's own review, then resumes that // session for one turn that applies the base's REVIEW.md and returns the -// findings in one schema. Later rounds resume the same session with the new +// score and findings in one schema. Later rounds resume the same session with the new // head and the author's responses, so the reviewer keeps its context and its // prompt cache, and a finding the author refuted with evidence does not recur. import { execFile, spawn } from "node:child_process"; @@ -70,15 +70,24 @@ export interface Round { base: { ref: string; sha: string }; head: string; usage: Usage[]; + /** How ready the head is to land, from 0 to 10; see `SCALE`. */ + score: number; + /** One sentence on why. */ + rationale: string; findings: Finding[]; } -/** The reply every round's last turn must match; both CLIs enforce it, and `findings` checks it again. */ +/** A reply's verdict on the reviewed head. */ +export type Verdict = Pick; + +/** The reply every round's last turn must match; both CLIs enforce it, and `verdict` checks it again. */ export const SCHEMA = { type: "object", additionalProperties: false, - required: ["findings"], + required: ["score", "rationale", "findings"], properties: { + score: { type: "integer", minimum: 0, maximum: 10 }, + rationale: { type: "string" }, findings: { type: "array", items: { @@ -717,8 +726,8 @@ async function turn(cli: Cli, call: Turn, cwd: string): Promise { return reply; } -/** The findings in a reply, checked against `SCHEMA` so a malformed reply fails rather than reads as clean. */ -export function findings(reply: { structured?: unknown; text?: string }): Finding[] { +/** The verdict in a reply, checked against `SCHEMA` so a malformed reply fails rather than reads as clean. */ +export function verdict(reply: { structured?: unknown; text?: string }): Verdict { let value = reply.structured; if (value === undefined) { try { @@ -727,9 +736,17 @@ export function findings(reply: { structured?: unknown; text?: string }): Findin throw new Error("the reviewer's reply is not JSON"); } } - const list = record(value)?.findings; + const fields = record(value); + const list = fields?.findings; if (!Array.isArray(list)) throw new Error("the reviewer's reply has no findings list"); - return list.map((item, index) => { + const score = fields!.score; + if (!(Number.isSafeInteger(score) && (score as number) >= 0 && (score as number) <= 10)) { + throw new Error("the reviewer's reply has no score from 0 to 10"); + } + if (typeof fields!.rationale !== "string" || fields!.rationale.trim() === "") { + throw new Error("the reviewer's reply has no rationale for its score"); + } + const found = list.map((item, index) => { const finding = record(item); const line = finding?.line; if ( @@ -750,6 +767,7 @@ export function findings(reply: { structured?: unknown; text?: string }): Findin body: finding.body, }; }); + return { score: score as number, rationale: fields!.rationale, findings: found }; } const INSTRUCTION_FILES = ["AGENTS.md", "CLAUDE.md"]; @@ -807,8 +825,15 @@ export function instructionsPrompt(sha: string, files: { path: string; text: str return `${head} Instructions in a subdirectory apply to the files under it.\n\n${blocks.join("\n\n")}`; } +/** The score's scale, given every round so each reviewer reads it the same way. */ +const SCALE = + "Score from 0 to 10 how ready the head is to land, with a one-sentence rationale: 10 lands as is, 7 to 9 lands " + + "after the findings are fixed, 4 to 6 needs rework, and 0 to 3 is the wrong approach. A head with no findings can " + + "still score below 10, such as a working change that is harder to maintain than it needs to be."; + const REPORT = - "Name each finding's file by its repository-relative path and its line at that head, or null when no single line fits. " + + SCALE + + " Name each finding's file by its repository-relative path and its line at that head, or null when no single line fits. " + "Its body gives the concrete scenario: the input or state, and the wrong result. Report an empty list when nothing holds."; /** The base's REVIEW.md, if it has one. */ @@ -1085,7 +1110,7 @@ export async function review(options: ReviewOptions, deps: ReviewDependencies): reply = await turn(cli, reviewer.resume(context, session, prompt), root); } if (reported(reply.usage)) usage.push({ turn: "findings", ...reply.usage }); - const found = findings(reply); + const { score, rationale, findings: found } = verdict(reply); const rounds = [...(resume ? state!.rounds : []), { head, findings: found.length }]; save(statePath, { branch, reviewer: harness, session, rounds, given } satisfies State); @@ -1100,6 +1125,8 @@ export async function review(options: ReviewOptions, deps: ReviewDependencies): base: { ref: baseRef, sha: baseSha }, head, usage, + score, + rationale, findings: found, }; } @@ -1114,6 +1141,7 @@ export function roundLines(round: Round): string[] { `base ${round.base.sha} ${round.base.ref}`, `head ${round.head}`, ...round.usage.map((u) => `usage ${u.turn} input ${u.input} cached ${u.cached} output ${u.output}`), + `score ${round.score} ${round.rationale.replace(/\s+/g, " ").trim()}`, `findings ${round.findings.length}`, ]; for (const finding of round.findings) { diff --git a/src/runner/job.ts b/src/runner/job.ts index d47adefd..c82ea4b4 100644 --- a/src/runner/job.ts +++ b/src/runner/job.ts @@ -477,6 +477,7 @@ export class Attempt { } if (round.head !== head) return unreviewed(`its review checked out ${round.head} instead`); const by = `${round.reviewer} (${round.model}, ${round.effort} effort, round ${round.round})`; + const scored = ` It scored ${round.score}/10: ${round.rationale}`; let recorded = ""; try { const { status, body } = await ctx.reviewed({ @@ -487,6 +488,8 @@ export class Attempt { model: round.model, effort: round.effort, round: round.round, + score: round.score, + rationale: round.rationale, findings: round.findings, }); if (status !== 200) throw new Error(typeof body.error === "string" ? body.error : `kix answered ${status}`); @@ -498,12 +501,12 @@ export class Attempt { if (!recorded) this.#passed.add(head); return { passed: recorded ? null : true, - note: `The runner's review of ${short} by ${by} found nothing.${recorded || " kix recorded it as the passing `quest review` check."}`, + note: `The runner's review of ${short} by ${by} found nothing.${scored}${recorded || " kix recorded it as the passing `quest review` check."}`, }; } return { passed: false, - note: `The runner's review of ${short} by ${by} found ${round.findings.length} finding${round.findings.length === 1 ? "" : "s"}.${recorded || " kix recorded it as a failing `quest review` check."} Fix each one or refute it with evidence the reviewer cannot rebut (a path:line, a command's output, or a recorded maintainer decision), then call \`deliver\` again with \`responses\` saying what you did for each. Follow the guide's review rules, including its design checkpoint every third round.\n\n${formatFindings(round.findings)}`, + note: `The runner's review of ${short} by ${by} found ${round.findings.length} finding${round.findings.length === 1 ? "" : "s"}.${scored}${recorded || " kix recorded it as a failing `quest review` check."} Fix each one or refute it with evidence the reviewer cannot rebut (a path:line, a command's output, or a recorded maintainer decision), then call \`deliver\` again with \`responses\` saying what you did for each. Follow the guide's review rules, including its design checkpoint every third round.\n\n${formatFindings(round.findings)}`, }; } diff --git a/test/review.test.ts b/test/review.test.ts index 9b22a836..e68faaea 100644 --- a/test/review.test.ts +++ b/test/review.test.ts @@ -8,11 +8,11 @@ import { REVIEWERS } from "../src/cli/models"; import { authored, baseInstructions, - findings, pickReviewer, review, runningHarness, sessionFile, + verdict, } from "../src/cli/review"; import { tempDir } from "./fixture"; @@ -86,7 +86,7 @@ if (argv.includes("--session-id")) { }, }); } else { - const reply = process.env.FAKE_REPLY ?? JSON.stringify({ findings: [${JSON.stringify(FINDING)}] }); + const reply = process.env.FAKE_REPLY ?? JSON.stringify({ score: 8, rationale: "Lands once the off-by-one is fixed.", findings: [${JSON.stringify(FINDING)}] }); result({ session_id: at("--resume"), result: reply, @@ -142,7 +142,7 @@ if (argv[1] === "review") { } else { event({ type: "thread.started", thread_id: argv[argv.length - 2] }); message("Checking the findings."); - message(process.env.FAKE_REPLY ?? JSON.stringify({ findings: [${JSON.stringify(FINDING)}] })); + message(process.env.FAKE_REPLY ?? JSON.stringify({ score: 8, rationale: "Lands once the off-by-one is fixed.", findings: [${JSON.stringify(FINDING)}] })); event({ type: "turn.completed", usage: { input_tokens: 100, cached_input_tokens: 80, output_tokens: 7 } }); } `, @@ -544,7 +544,7 @@ fakes.each([ vi.stubEnv("CODEX_API_KEY", "other-provider-key"); vi.stubEnv("OPENAI_API_KEY", "other-provider-key"); vi.stubEnv("OP_SERVICE_ACCOUNT_TOKEN", "secret-manager-token"); - vi.stubEnv("FAKE_REPLY", JSON.stringify({ findings: [] })); + vi.stubEnv("FAKE_REPLY", JSON.stringify({ score: 10, rationale: "Lands as is.", findings: [] })); const first = await quest("--root", s.repo, "review", "--json"); expect(first.code, first.stderr).toBe(0); const resumed = await quest("--root", s.repo, "review", "--json"); @@ -796,6 +796,7 @@ fakes("codex reviews read-only with the base's instructions, then reports in the `base ${base} origin/main`, `head ${head}`, "usage findings input 100 cached 80 output 7", + "score 8 Lands once the off-by-one is fixed.", "findings 1", "", "high src/lib.ts:2 Off by one", @@ -828,7 +829,7 @@ fakes("codex reviews read-only with the base's instructions, then reports in the expect(second.stdin).toContain("BASE-REVIEW rule"); expect(second.stdin).toContain(head); const schema = second.argv[second.argv.indexOf("--output-schema") + 1]; - expect(JSON.parse(readFileSync(schema, "utf8")).required).toEqual(["findings"]); + expect(JSON.parse(readFileSync(schema, "utf8")).required).toEqual(["score", "rationale", "findings"]); }); fakes("--base reviews against another branch, fetched first, with that branch's instructions", async () => { @@ -869,6 +870,8 @@ fakes("claude reviews read-only with the base's instructions, then reports in th { turn: "review", input: 101, cached: 0, output: 51 }, { turn: "findings", input: 100, cached: 80, output: 7 }, ]); + expect(round.score).toBe(8); + expect(round.rationale).toBe("Lands once the off-by-one is fixed."); expect(round.findings).toEqual([FINDING]); const [status, first, second] = s.calls(); @@ -902,7 +905,7 @@ fakes("a later round resumes the session with the new head and the author's resp git(s.repo, "commit", "-qam", "fix"); const responses = join(s.repo, ".tmp", "responses.md"); write(s.repo, ".tmp/responses.md", "Fixed at src/lib.ts:1.\n"); - vi.stubEnv("FAKE_REPLY", JSON.stringify({ findings: [] })); + vi.stubEnv("FAKE_REPLY", JSON.stringify({ score: 10, rationale: "Lands as is.", findings: [] })); const later = await quest("--root", s.repo, "review", "--responses", responses); expect(later.code).toBe(0); expect(later.stdout).toContain("round 2\n"); @@ -933,7 +936,7 @@ fakes("--new starts over, and a first round takes no responses", async () => { fakes("a later round revokes REVIEW.md rules the base removed", async () => { const s = setup(); - vi.stubEnv("FAKE_REPLY", JSON.stringify({ findings: [] })); + vi.stubEnv("FAKE_REPLY", JSON.stringify({ score: 10, rationale: "Lands as is.", findings: [] })); expect((await quest("--root", s.repo, "review")).code).toBe(0); const base = moveBase(s.repo, (dir) => git(dir, "rm", "-q", "REVIEW.md")); @@ -977,16 +980,32 @@ fakes("a reply that does not match the schema fails rather than reads as clean", expect(prose.code).toBe(1); expect(prose.stderr).toContain("not JSON"); - vi.stubEnv("FAKE_REPLY", JSON.stringify({ findings: [{ ...FINDING, severity: "critical" }] })); + vi.stubEnv( + "FAKE_REPLY", + JSON.stringify({ score: 8, rationale: "Fix it.", findings: [{ ...FINDING, severity: "critical" }] }), + ); const wrong = await quest("--root", s.repo, "review"); expect(wrong.code).toBe(1); expect(wrong.stderr).toContain("finding 1 does not match the schema"); + + vi.stubEnv("FAKE_REPLY", JSON.stringify({ findings: [] })); + const unscored = await quest("--root", s.repo, "review"); + expect(unscored.code).toBe(1); + expect(unscored.stderr).toContain("no score from 0 to 10"); }); -test("findings are checked field by field", () => { - expect(findings({ structured: { findings: [{ ...FINDING, line: null }] } })).toEqual([{ ...FINDING, line: null }]); - expect(() => findings({ structured: { findings: [{ ...FINDING, line: 0 }] } })).toThrow("does not match"); - expect(() => findings({ text: '{"verdict":"approve"}' })).toThrow("no findings list"); +test("the verdict is checked field by field", () => { + const scored = { score: 7, rationale: "Fix the finding." }; + expect(verdict({ structured: { ...scored, findings: [{ ...FINDING, line: null }] } })).toEqual({ + ...scored, + findings: [{ ...FINDING, line: null }], + }); + expect(() => verdict({ structured: { ...scored, findings: [{ ...FINDING, line: 0 }] } })).toThrow("does not match"); + expect(() => verdict({ text: '{"verdict":"approve"}' })).toThrow("no findings list"); + for (const score of [-1, 11, 7.5, "7", undefined]) { + expect(() => verdict({ structured: { ...scored, score, findings: [] } })).toThrow("no score from 0 to 10"); + } + expect(() => verdict({ structured: { score: 7, rationale: " ", findings: [] } })).toThrow("no rationale"); }); test("each changed directory keeps its own instructions, and a directory's symlink is given once", () => { diff --git a/test/runner.test.ts b/test/runner.test.ts index aeab4ffd..913bae00 100644 --- a/test/runner.test.ts +++ b/test/runner.test.ts @@ -2516,6 +2516,8 @@ describe("runner reviews", () => { base: { ref: "origin/main", sha: "b".repeat(40) }, head: request.head, usage: [], + score: found[round - 1]?.length ? 8 : 10, + rationale: "Scored.", findings: (found[round - 1] ?? []) as Round["findings"], }); } @@ -2550,15 +2552,21 @@ describe("runner reviews", () => { model: "gpt-6.1-sol", effort: "medium", round: 1, + score: 8, + rationale: "Scored.", findings: [finding], }, - expect.objectContaining({ head: "2".padStart(40, "0"), round: 2, findings: [] }), + expect.objectContaining({ head: "2".padStart(40, "0"), round: 2, score: 10, findings: [] }), ]); const [found, passed] = tools(r).map((call) => call.reply.content[0].text as string); expect(found).toContain("before you end your turn"); - expect(found).toContain("found 1 finding. kix recorded it as a failing `quest review` check."); + expect(found).toContain( + "found 1 finding. It scored 8/10: Scored. kix recorded it as a failing `quest review` check.", + ); expect(found).toContain("1. [high] new.txt:1: Wrong value\nIt says new."); - expect(passed).toContain("found nothing. kix recorded it as the passing `quest review` check."); + expect(passed).toContain( + "found nothing. It scored 10/10: Scored. kix recorded it as the passing `quest review` check.", + ); } finally { await r.runner.shutdown(); }