From 9abd2d35f458c8386e190e487567aa1bfd3195be Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Wed, 4 Nov 2026 12:39:23 -0800 Subject: [PATCH 1/8] chore(quest): claim quest/m0/model-routing/review-score.md Quest-Claim: fc93f724f94e7842 From af4524722dfc32f5e79bacb7ce8ed73bc81d0104 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Wed, 4 Nov 2026 13:02:15 -0800 Subject: [PATCH 2/8] quest review: score each round out of 10 The reviewer's schema turn now reports a required integer score from 0 to 10 for how ready the head is to land, with a one-sentence rationale, on a scale every prompt defines: 10 lands as is, 7 to 9 after the findings are fixed, 4 to 6 needs rework, 0 to 3 is the wrong approach. A reply without a valid score fails like any malformed reply. `quest review` prints a `score` line beside `findings`, `--json` carries both fields, the runner's note to the agent names the score, and the `## Review` record and /quest-review's summary include it. The findings still decide the loop. Agent-Harness: claude Agent-Model: claude-opus-5-5 Agent-Effort: medium Co-Authored-By: Claude Opus 5.5 --- README.md | 7 ++++++- assets/CONTEXT.md | 3 ++- assets/skills/review.md | 2 +- src/cli/main.ts | 4 +++- src/cli/review.ts | 44 +++++++++++++++++++++++++++++++++-------- src/runner/job.ts | 5 +++-- test/review.test.ts | 43 +++++++++++++++++++++++++++++----------- test/runner.test.ts | 10 ++++++++-- 8 files changed, 90 insertions(+), 28 deletions(-) diff --git a/README.md b/README.md index 9de74eeb..9de44cbf 100644 --- a/README.md +++ b/README.md @@ -1100,7 +1100,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 d0b99ced..6349c625 100644 --- a/assets/CONTEXT.md +++ b/assets/CONTEXT.md @@ -124,7 +124,8 @@ A reviewer that is out of quota or logged out is an outage, not a skip: keep the Give the reset time when the error names one; a logged-out reviewer waits on `Maintainer:`, with the login command `quest review` prints. Once the reset passes or the login is restored, the item has cleared: whoever finds it runs `/quest-review` on the PR. -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 27ea987b..20f600bb 100644 --- a/assets/skills/review.md +++ b/assets/skills/review.md @@ -30,7 +30,7 @@ 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 the last round's score and rationale, and CI from `quest status `, naming any decisions it 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/src/cli/main.ts b/src/cli/main.ts index ac5179c1..4ce059d1 100644 --- a/src/cli/main.ts +++ b/src/cli/main.ts @@ -1000,7 +1000,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, 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, and 1 for other failures.", ) diff --git a/src/cli/review.ts b/src/cli/review.ts index 97ad2105..54a054b3 100644 --- a/src/cli/review.ts +++ b/src/cli/review.ts @@ -2,7 +2,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"; @@ -59,15 +59,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[]; } +/** 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 `findings` 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: { @@ -610,8 +619,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 { @@ -620,9 +629,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 ( @@ -643,6 +660,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"]; @@ -700,8 +718,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. */ @@ -975,7 +1000,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); @@ -988,6 +1013,8 @@ export async function review(options: ReviewOptions, deps: ReviewDependencies): base: { ref: baseRef, sha: baseSha }, head, usage, + score, + rationale, findings: found, }; } @@ -1001,6 +1028,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 89ec8186..95c6cb02 100644 --- a/src/runner/job.ts +++ b/src/runner/job.ts @@ -476,6 +476,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({ @@ -497,12 +498,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 fourth 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 fourth round.\n\n${formatFindings(round.findings)}`, }; } diff --git a/test/review.test.ts b/test/review.test.ts index a4dbf2f7..162e435f 100644 --- a/test/review.test.ts +++ b/test/review.test.ts @@ -4,7 +4,7 @@ import { join } from "node:path"; import { afterEach, expect, test, vi } from "vitest"; import { type Output, main } from "../src/cli/main"; -import { baseInstructions, findings, review, runningHarness, sessionFile } from "../src/cli/review"; +import { baseInstructions, review, runningHarness, sessionFile, verdict } from "../src/cli/review"; import { tempDir } from "./fixture"; afterEach(() => { @@ -67,7 +67,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, @@ -114,7 +114,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 } }); } `, @@ -389,7 +389,7 @@ fakes.each([ vi.stubEnv("OPENAI_API_KEY", "other-provider-key"); vi.stubEnv("QUEST_API_KEYS", "other-agent-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"); @@ -640,6 +640,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", @@ -672,7 +673,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 () => { @@ -713,6 +714,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(); @@ -746,7 +749,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"); @@ -777,7 +780,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")); @@ -821,16 +824,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 d0c24858..0012f383 100644 --- a/test/runner.test.ts +++ b/test/runner.test.ts @@ -2503,6 +2503,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"], }); } @@ -2543,9 +2545,13 @@ describe("runner reviews", () => { ]); 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(); } From 8d0f4722f9094866c8552e53b7fbfe64da1b7c5a Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Wed, 4 Nov 2026 13:02:31 -0800 Subject: [PATCH 3/8] quest: finish the review score quest Agent-Harness: claude Agent-Model: claude-opus-5-5 Agent-Effort: medium Co-Authored-By: Claude Opus 5.5 --- quest/m0/model-routing/README.md | 1 - quest/m0/model-routing/review-score.md | 29 -------------------------- quest/m1/usage.md | 3 +-- quest/m2/council.md | 5 ++--- 4 files changed, 3 insertions(+), 35 deletions(-) delete mode 100644 quest/m0/model-routing/review-score.md diff --git a/quest/m0/model-routing/README.md b/quest/m0/model-routing/README.md index 132490e0..1580012e 100644 --- a/quest/m0/model-routing/README.md +++ b/quest/m0/model-routing/README.md @@ -79,7 +79,6 @@ 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 by size](/quest/m0/model-routing/review-effort.md) - Sol reviews at an effort set by size, and Opus reviews what Sol wrote -- [Review score](/quest/m0/model-routing/review-score.md) - every `quest review` round scores the change out of 10 - [Sizes include judgment](/quest/m0/model-routing/size-judgment.md) - the guide and `/quest-plan` size a quest by the agent's effort, judgment included - [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 - [Fixes link their culprits](/quest/m0/model-routing/culprits.md) - a fix lists the PR that introduced the problem under `## Caused by` 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 3b8d8a1f..ede9bc61 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` ([culprits](/quest/m0/model-routing/culprits.md)). - 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 00b0addb..8056961b 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 @@ -63,7 +63,6 @@ Agent's choices, for the implementer to revisit: - [Grok and opencode reviewers](/quest/m2/review-harnesses.md) - the council's xAI and DeepSeek members - [Review by size](/quest/m0/model-routing/review-effort.md) - the Sol and Opus review efforts the council's members use -- [Review score](/quest/m0/model-routing/review-score.md) - the score each member reports ## Related From bdfdac88579eac3a31b1e424d99f866a71cd9252 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Wed, 4 Nov 2026 13:16:52 -0800 Subject: [PATCH 4/8] quest review: fix the verdict doc comment and the summary's score source Co-Authored-By: Claude Opus 5.5 --- assets/skills/review.md | 3 ++- src/cli/review.ts | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/assets/skills/review.md b/assets/skills/review.md index 20f600bb..2df09653 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, with the last round's score and rationale, 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/src/cli/review.ts b/src/cli/review.ts index 54a054b3..f81dcdf0 100644 --- a/src/cli/review.ts +++ b/src/cli/review.ts @@ -69,7 +69,7 @@ export interface Round { /** 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 `findings` checks it again. */ +/** 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, From 28e66b66121f93529c5f2c0f8998b04e0dbc46fc Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Wed, 4 Nov 2026 13:32:39 -0800 Subject: [PATCH 5/8] kix: record a runner review's score and rationale in its check run The runner sends the round's score and rationale with its report, and the Worker puts the score in the quest review check's title and the rationale, fenced, ahead of the findings. Reports from runners without scores still record. Co-Authored-By: Claude Opus 5.5 --- cloudflare/GITHUB.md | 13 +++++++------ cloudflare/src/jobs/review.ts | 26 +++++++++++++++++++++----- cloudflare/test/publish.test.ts | 14 +++++++++++--- src/runner/job.ts | 2 ++ test/runner.test.ts | 4 +++- 5 files changed, 44 insertions(+), 15 deletions(-) diff --git a/cloudflare/GITHUB.md b/cloudflare/GITHUB.md index 7f53d50a..48bb3640 100644 --- a/cloudflare/GITHUB.md +++ b/cloudflare/GITHUB.md @@ -377,12 +377,13 @@ 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, +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: `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. diff --git a/cloudflare/src/jobs/review.ts b/cloudflare/src/jobs/review.ts index 9af34d9e..8eb5f6b4 100644 --- a/cloudflare/src/jobs/review.ts +++ b/cloudflare/src/jobs/review.ts @@ -29,11 +29,14 @@ export type ReviewReport = { model: string; effort: string; round: number; + /** The round's 0 to 10 readiness score and its rationale; runners from before scores existed send neither. */ + 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 }; +export const REVIEW_LIMITS = { findings: 100, title: 300, body: 8000, rationale: 8000 }; const count = (value: unknown, min: number) => typeof value === "number" && Number.isSafeInteger(value) && value >= min ? value : null; @@ -51,6 +54,14 @@ 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"); + let scored: Pick = {}; + if (body.score !== undefined || body.rationale !== undefined) { + const score = count(body.score, 0); + const rationale = text(body.rationale, REVIEW_LIMITS.rationale); + if (score === null || score > 10 || !rationale) + throw new JobError("A score is an integer from 0 to 10 with a rationale"); + scored = { score, 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 => { @@ -63,7 +74,7 @@ export function parseReview(body: Record): ReviewReport { 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, ...scored, findings }; } /** Untrusted text inside a fence no run of backticks in it can close. */ @@ -73,15 +84,20 @@ 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 any 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 counted = n ? `${n} finding${n === 1 ? "" : "s"}` : "No findings"; + const title = report.score === undefined ? counted : `${counted}, 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 = report.rationale === undefined ? "" : fenced(`score ${report.score}/10: ${report.rationale}`); for (const [i, finding] of findings.entries()) { const next = `${text}${text ? "\n\n" : ""}${finding}`; // GitHub refuses a check run's text past 65,535 characters. diff --git a/cloudflare/test/publish.test.ts b/cloudflare/test/publish.test.ts index c863e87b..6d62e659 100644 --- a/cloudflare/test/publish.test.ts +++ b/cloudflare/test/publish.test.ts @@ -1851,7 +1851,7 @@ 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 +1860,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````", }, }, ]); @@ -1884,6 +1886,7 @@ describe("merge jobs", () => { const id = await merging(false); const head = await fix(id); await report({ job: id, head, findings: [finding] }); + // A runner from before scores existed sends none, and its review still counts. const passed = await report({ job: id, head, round: 2 }); expect(await passed.json()).toEqual({ conclusion: "success" }); expect(project.checkRuns!.at(-1)!.output).toEqual({ @@ -1917,6 +1920,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 score is an integer from 0 to 10 with a rationale"; + await refused({ head, score: 11, rationale: "Too high." }, unscored); + await refused({ head, score: 7.5, rationale: "Not whole." }, unscored); + await refused({ head, score: 7 }, unscored); + await refused({ head, rationale: "No score." }, 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/src/runner/job.ts b/src/runner/job.ts index 95c6cb02..64699069 100644 --- a/src/runner/job.ts +++ b/src/runner/job.ts @@ -487,6 +487,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}`); diff --git a/test/runner.test.ts b/test/runner.test.ts index 0012f383..09049cb4 100644 --- a/test/runner.test.ts +++ b/test/runner.test.ts @@ -2539,9 +2539,11 @@ 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"); From e655e469a8d0067f9800f78ef2d5141a97907e6c Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Wed, 4 Nov 2026 13:42:47 -0800 Subject: [PATCH 6/8] kix: format the review score test Co-Authored-By: Claude Opus 5.5 --- cloudflare/test/publish.test.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/cloudflare/test/publish.test.ts b/cloudflare/test/publish.test.ts index 6d62e659..2c4cdf8c 100644 --- a/cloudflare/test/publish.test.ts +++ b/cloudflare/test/publish.test.ts @@ -1851,7 +1851,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, score: 4, rationale: "Breaks on `null`.", 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([ { From 57a556c038819b2508ecb7f83d220aef403e97d2 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Wed, 4 Nov 2026 13:52:03 -0800 Subject: [PATCH 7/8] kix: require a score on every runner review report Nothing is in production, so the Worker no longer accepts unscored reports. Co-Authored-By: Claude Opus 5.5 --- cloudflare/src/jobs/review.ts | 31 +++++++++++++------------------ cloudflare/test/publish.test.ts | 16 +++++++++------- 2 files changed, 22 insertions(+), 25 deletions(-) diff --git a/cloudflare/src/jobs/review.ts b/cloudflare/src/jobs/review.ts index 8eb5f6b4..3148a9b3 100644 --- a/cloudflare/src/jobs/review.ts +++ b/cloudflare/src/jobs/review.ts @@ -29,9 +29,9 @@ export type ReviewReport = { model: string; effort: string; round: number; - /** The round's 0 to 10 readiness score and its rationale; runners from before scores existed send neither. */ - score?: number; - rationale?: string; + /** How ready the head is to land, from 0 to 10, and why. */ + score: number; + rationale: string; findings: Finding[]; }; @@ -54,14 +54,10 @@ 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"); - let scored: Pick = {}; - if (body.score !== undefined || body.rationale !== undefined) { - const score = count(body.score, 0); - const rationale = text(body.rationale, REVIEW_LIMITS.rationale); - if (score === null || score > 10 || !rationale) - throw new JobError("A score is an integer from 0 to 10 with a rationale"); - scored = { score, rationale }; - } + const score = count(body.score, 0); + const rationale = text(body.rationale, REVIEW_LIMITS.rationale); + if (score === null || score > 10 || !rationale) + 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 => { @@ -74,7 +70,7 @@ export function parseReview(body: Record): ReviewReport { 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, ...scored, 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. */ @@ -85,21 +81,20 @@ function fenced(value: string) { } /** - * The check run's output: a title with the findings count and any score, a + * 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 counted = n ? `${n} finding${n === 1 ? "" : "s"}` : "No findings"; - const title = report.score === undefined ? counted : `${counted}, scored ${report.score}/10`; + 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 = report.rationale === undefined ? "" : fenced(`score ${report.score}/10: ${report.rationale}`); + 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.`; @@ -107,7 +102,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 2c4cdf8c..7950547f 100644 --- a/cloudflare/test/publish.test.ts +++ b/cloudflare/test/publish.test.ts @@ -1811,6 +1811,8 @@ describe("merge jobs", () => { model: "gpt-6.1-sol", effort: "medium", round: 1, + score: 10, + rationale: "Lands as is.", findings: [], ...body, }); @@ -1892,13 +1894,13 @@ describe("merge jobs", () => { const id = await merging(false); const head = await fix(id); await report({ job: id, head, findings: [finding] }); - // A runner from before scores existed sends none, and its review still counts. 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]); @@ -1926,11 +1928,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 score is an integer from 0 to 10 with a rationale"; - await refused({ head, score: 11, rationale: "Too high." }, unscored); - await refused({ head, score: 7.5, rationale: "Not whole." }, unscored); - await refused({ head, score: 7 }, unscored); - await refused({ head, rationale: "No score." }, unscored); + 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 }); From 08585c77bd7cf5ead8b0c0acd0574f63b04ba2ea Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Wed, 4 Nov 2026 14:15:13 -0800 Subject: [PATCH 8/8] Cut overlong review prose instead of refusing the report The Worker refused a runner's review report whose rationale passed 8,000 characters, or whose finding file, title, or body passed its bound or was blank, though `quest review` accepts all of them, leaving that head unrecorded. That prose only fills the check run's text, so kix now cuts it to fit. Co-Authored-By: Claude Opus 5.5 --- cloudflare/GITHUB.md | 22 +++++++++++----------- cloudflare/src/jobs/review.ts | 29 +++++++++++++++++++++-------- cloudflare/test/publish.test.ts | 30 ++++++++++++++++++++++++++++++ 3 files changed, 62 insertions(+), 19 deletions(-) diff --git a/cloudflare/GITHUB.md b/cloudflare/GITHUB.md index 48bb3640..f0657824 100644 --- a/cloudflare/GITHUB.md +++ b/cloudflare/GITHUB.md @@ -376,17 +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, the round's score -out of 10 in its title, and the score's rationale and each finding as plain -text: `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. +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 3148a9b3..05a33347 100644 --- a/cloudflare/src/jobs/review.ts +++ b/cloudflare/src/jobs/review.ts @@ -35,12 +35,25 @@ export type ReviewReport = { 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, rationale: 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); @@ -55,18 +68,18 @@ export function parseReview(body: Record): ReviewReport { 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 = text(body.rationale, REVIEW_LIMITS.rationale); - if (score === null || score > 10 || !rationale) + 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 }; }); diff --git a/cloudflare/test/publish.test.ts b/cloudflare/test/publish.test.ts index 7950547f..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"; @@ -1906,6 +1907,35 @@ describe("merge jobs", () => { 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) => {