diff --git a/.agents/skills/quest-review/SKILL.md b/.agents/skills/quest-review/SKILL.md index 84906a4c..39c62d32 100644 --- a/.agents/skills/quest-review/SKILL.md +++ b/.agents/skills/quest-review/SKILL.md @@ -1,6 +1,6 @@ --- name: quest-review -description: Review a PR's branch locally with a model that wrote none of it, then mark it ready for review. +description: Review a PR's branch locally with a model other than its latest writer, then mark it ready for review. --- Run `quest skill review` and follow its output. diff --git a/.claude/skills/quest-review/SKILL.md b/.claude/skills/quest-review/SKILL.md index a6945773..7d817fc9 100644 --- a/.claude/skills/quest-review/SKILL.md +++ b/.claude/skills/quest-review/SKILL.md @@ -1,6 +1,6 @@ --- name: quest-review -description: Review a PR's branch locally with a model that wrote none of it, then mark it ready for review. +description: Review a PR's branch locally with a model other than its latest writer, then mark it ready for review. allowed-tools: Bash(quest skill review) PowerShell(quest skill review) --- diff --git a/README.md b/README.md index b3077d16..1f9708b6 100644 --- a/README.md +++ b/README.md @@ -1210,11 +1210,11 @@ whole branch at the PR's size. ## Review locally -`quest review` has a model that wrote none of the code review the checkout's +`quest review` has a model other than the latest writer review the checkout's committed branch before it is pushed for review, at an effort set by the PR's size. Each size ranks its reviewers, the same for every repository: -| PR size | Reviewer | When Sol wrote the code | +| PR size | Reviewer | When Sol changed it last | |---|---|---| | XS | Codex `gpt-6.1-sol` low | Claude Code `claude-opus-5-5` low | | S | Codex `gpt-6.1-sol` medium | Claude Code `claude-opus-5-5` low | @@ -1226,14 +1226,17 @@ A quest branch takes its quest's size, read from the base branch's tip so a change cannot lower its own review; any other PR, such as a skill's own, takes the `Size:` line of the `## Models` record in its open PR's body, and a PR with neither gets M's reviewers. `--size` overrides both. The first reviewer whose -model wrote none of the code reviews: the record's `Implementer:` lines name -the models that did, or `--exclude-model` names each instead. Unless the record -names every one, with no `unknown` model, the harness running the command is -taken to have written it too, so Codex reviews from Claude Code and Claude Code -from Codex. `--harness` names the reviewer instead, at its entry's model and +model is not the latest writer's reviews: the record's last `Implementer:` line +names it, since each session moves its line last whenever it pushes, or +`--exclude-model` names each model to avoid instead. Earlier writers may review. +Without a record, or when its last line names an `unknown` model, or when the +body's and kix's sessions comment's last lines name different models, whose +order cannot be told, the harness running the command, usually the session that +just pushed, is taken to be the latest writer, so Codex reviews from Claude Code +and Claude Code from Codex. `--harness` names the reviewer instead, at its entry's model and effort, and `--model` and `--effort` override those, as a repository whose `AGENTS.md` names other reviewers needs. An unknown harness, a size whose -every reviewer wrote the code, or a missing or logged-out CLI, is an error +every reviewer is avoided, or a missing or logged-out CLI, is an error rather than a same-model review, as is a login the API rejects mid-round, such as an expired or revoked token. The reviewer reaches its model as the user's own CLI does: Claude Code keeps `ANTHROPIC_BASE_URL`, `ANTHROPIC_CUSTOM_HEADERS`, and its @@ -1742,7 +1745,7 @@ arrives with the guide sections and other skills it builds on, in one call: | `quest-start` | Claim a ready quest, complete it, and open a PR. | | `quest-spawn` | Triage ready quests and send several agents out in parallel. | | `quest-iterate` | Iterate on an open PR to settle its open decisions, fix CI and review findings, and push. | -| `quest-review` | Review a PR's branch locally with a model that wrote none of it, then mark it ready for review. | +| `quest-review` | Review a PR's branch locally with a model other than its latest writer, then mark it ready for review. | | `quest-merge` | Queue a PR to land, running `quest-iterate` first when it needs work. | | `quest-complete` | Decide which open PRs to merge, then queue them. | | `quest-afk` | Keep starting agreed work while you are away, leaving every decision for your return. | diff --git a/assets/CONTEXT.md b/assets/CONTEXT.md index bc96ce56..18bc7922 100644 --- a/assets/CONTEXT.md +++ b/assets/CONTEXT.md @@ -94,9 +94,9 @@ A PR that another session pushed to in the last hour is theirs: report its event Before each push to a PR's branch, confirm the PR is still open, since a push after it merges restores the deleted branch. Run the repository's checks locally while the PR is a draft; CI may run only on PRs ready for review. -Before a PR is marked ready for review, and before each push to one that is, a model that wrote none of its code reviews the committed branch locally, as `/quest-review` describes. +Before a PR is marked ready for review, and before each push to one that is, a model other than the one that changed it last reviews the committed branch locally, as `/quest-review` describes. -The PR body's `## Models` section records its size as `Size: M` (its quest's, or for a skill's own PR, as a quest is sized) and one `Implementer: Codex, gpt-6.1-sol, high effort` line per implementing session, added when it first pushes; kix records one that pushes to an open PR in its own comment. +The PR body's `## Models` section records its size as `Size: M` (its quest's, or for a skill's own PR, as a quest is sized) and one `Implementer: Codex, gpt-6.1-sol, high effort` line per implementing session, moved last whenever it pushes; kix records one that pushes to an open PR in its own comment. Read your model and effort rather than guess, writing `unknown` for any you cannot find: Claude Code knows its model and exports `CLAUDE_EFFORT`, and Codex records `model` and `effort` in the last `turn_context` of `~/.codex/sessions/*/*/*/rollout-*-$CODEX_THREAD_ID.jsonl`. GitHub gives secrets to workflows on branches pushed to the repository, as quest branches usually are, so keep pull request CI, which runs unreviewed code, read-only: a read-only `GITHUB_TOKEN`, and write credentials only in environments limited to protected branches. diff --git a/assets/skills/review.md b/assets/skills/review.md index 4edac7bc..a15ab7d2 100644 --- a/assets/skills/review.md +++ b/assets/skills/review.md @@ -1,5 +1,5 @@ --- -description: Review a PR's branch locally with a model that wrote none of it, then mark it ready for review. +description: Review a PR's branch locally with a model other than its latest writer, then mark it ready for review. guide: Execution, Summaries, Questions skills: merge --- @@ -21,8 +21,8 @@ Never alter the repository's harness to get a pass, such as by disabling a tool, If a `## Review` record that `/quest-merge` counts names the committed head, or its review exemptions cover the changes since the head it names, and it leaves no finding unrefuted, run no new round and go on to pushing and marking it ready below. If you judge the change too trivial to review, skip the review instead and record the skip and its reason in `## Review`. -Otherwise, review the committed branch with `quest review --base /`, which picks a reviewer that wrote none of the code, and its effort, by the PR's size; `quest review --help` explains the round. -It reads the implementing models, and on a branch that is not a quest's the size, from the PR body's `## Models` record and kix's sessions comment, so write that record before the first round; a quest branch's size is read from its quest. +Otherwise, review the committed branch with `quest review --base /`, which picks a reviewer other than the model that changed it last, and its effort, by the PR's size; `quest review --help` explains the round. +It reads the latest implementing model, and on a branch that is not a quest's the size, from the PR body's `## Models` record and kix's sessions comment, so write that record before the first round; a quest branch's size is read from its quest. Pass `--exclude-model` or `--size` only for a PR whose body cannot hold the record. Where the repository's root instructions name other reviewers, pass them with `--harness`, `--model`, and `--effort`. Run it in the foreground; a round takes minutes. diff --git a/cloudflare/src/github/service.ts b/cloudflare/src/github/service.ts index bd7878c1..733101d9 100644 --- a/cloudflare/src/github/service.ts +++ b/cloudflare/src/github/service.ts @@ -2239,8 +2239,9 @@ export class GitHubService { if (pull?.state !== "open") throw error; } } - // A later session's body keeps the sessions that delivered before it in its `## Models` record. - const body = implementers(pull.body ?? "").reduce(withModels, input.body); + // A later session's body keeps the sessions that delivered before it in its `## Models` record, + // with its own last, since the last names the latest writer. + const body = [...implementers(pull.body ?? ""), ...implementers(input.body)].reduce(withModels, input.body); if (body.length > BODY_LIMIT) throw new GitHubError("The pull request body is too long"); if (pull.title !== input.title || pull.body !== body) pull = await this.#api(token, `${pulls}/${number(pull.number)}`, "PATCH", { diff --git a/cloudflare/test/publish.test.ts b/cloudflare/test/publish.test.ts index e744fd89..dcefb178 100644 --- a/cloudflare/test/publish.test.ts +++ b/cloudflare/test/publish.test.ts @@ -1110,10 +1110,13 @@ describe("runner publication", () => { await send(one); await send(one); await send(two); - const sessions = project.pulls[0]!.comments!.filter((c) => c.author === "quest-test[bot]"); - expect(sessions.map((c) => c.body)).toEqual([ - `\nThe sessions that published to this pull request after it opened.\n\n## Models\n\n- ${one}\n- ${two}\n`, - ]); + const sessions = () => project.pulls[0]!.comments!.filter((c) => c.author === "quest-test[bot]"); + const record = (...lines: string[]) => + `\nThe sessions that published to this pull request after it opened.\n\n## Models\n\n${lines.map((l) => `- ${l}\n`).join("")}`; + expect(sessions().map((c) => c.body)).toEqual([record(one, two)]); + // A session that pushes again moves to the end, so the last line names the latest writer. + await send(one); + expect(sessions().map((c) => c.body)).toEqual([record(two, one)]); // The body, which holds the author's own review record, is untouched. expect(project.pulls[0]!.body).toBe(body); diff --git a/cloudflare/test/seed.test.ts b/cloudflare/test/seed.test.ts index 004920cc..4e41a443 100644 --- a/cloudflare/test/seed.test.ts +++ b/cloudflare/test/seed.test.ts @@ -481,8 +481,9 @@ it("publishes jobs' work end to end, through the upstream branch to the pull req head: first.head, pull: models("- Size: S\n- Implementer: opencode, second, high effort"), }); + // The delivering session's own line stays last, since the last names the latest writer. expect(rocket.pulls.at(-1)!.body).toBe( - models("- Size: S\n- Implementer: opencode, second, high effort\n- Implementer: claude, first, high effort") + models("- Size: S\n- Implementer: claude, first, high effort\n- Implementer: opencode, second, high effort") .body, ); diff --git a/docs/getting-started.md b/docs/getting-started.md index 07208c05..d3638310 100644 --- a/docs/getting-started.md +++ b/docs/getting-started.md @@ -59,7 +59,7 @@ a fork, already exists. `quest unblocked --unclaimed` lists the ready quests nobody holds. kix.quest records each active job in its database and reassigns a job whose runner goes offline; check it for a live runner lock before starting. -You review each change: `/quest-review` has a model that wrote none of it review a +You review each change: `/quest-review` has a model other than its latest writer review a draft locally and marks it ready, and `/quest-merge` (`$quest-merge` in Codex) queues it to land once a review covers its head and checks pass. `/quest-complete` queues a writer's PR that changes no user-facing behavior, diff --git a/quest/m0/model-routing/README.md b/quest/m0/model-routing/README.md index 4e5bce63..bb377e34 100644 --- a/quest/m0/model-routing/README.md +++ b/quest/m0/model-routing/README.md @@ -68,8 +68,12 @@ are mostly judgment. models: Opus writes the code, since it does a much better job. XS: Grok high, then Sol medium. S: Opus medium, then Sol high. M: Opus high, then Sol xhigh. L and XL: Opus xhigh, no fallback. Reviews: Sol low, medium, - high, xhigh, xhigh for XS to XL, never the model that wrote the code; + high, xhigh, xhigh for XS to XL, never the model that changed it last; Pullfrog and CodeRabbit add free reviews on top. +- Revised 2026-11-07 (user): a review avoids only the latest writer, the + record's last Implementer, which each session moves last whenever it pushes. + Avoiding every writer left no reviewer once Sol and Opus had both written a + PR, and agents then picked an unlisted model themselves. - Every PR records its models and efforts, and every fix links the PR that caused it, so the rankings can be audited (user). The PR record alone, without commit trailers (user, 2026-11-04 design checkpoint of diff --git a/src/cli/main.ts b/src/cli/main.ts index 61cf9a7e..631a2916 100644 --- a/src/cli/main.ts +++ b/src/cli/main.ts @@ -1043,15 +1043,16 @@ export function main(argv: string[], out: Output): number | Promise { ).join("; "); program .command("review") - .summary("Have a model that wrote none of the code review the committed branch, read-only") + .summary("Have a model other than the latest writer review the committed branch, read-only") .description( - "Review the checkout's committed branch against its base with a reviewer its size picks, never a model " + - `that wrote the code. Each size ranks its reviewers (harness, model, effort): ${reviewers}. The first ` + - "whose model wrote none of the code reviews. The models that wrote it are --exclude-model's; without " + - "it, they are those the Implementer lines of the `## Models` record in the body of the branch's open PR, and of kix's sessions comment on it, " + - "name and, unless those name every one with no `unknown` model or --harness names the reviewer, any " + - "model of the harness running this command, detected from the variables each sets, so Codex reviews " + - "from Claude Code and Claude Code from Codex. " + + "Review the checkout's committed branch against its base with a reviewer its size picks, never the " + + `model that changed it last. Each size ranks its reviewers (harness, model, effort): ${reviewers}. The ` + + "first whose model is not avoided reviews. --exclude-model names each model to avoid; without it, the " + + "latest writer is avoided: the model the last Implementer line names in the `## Models` record of the body of the branch's open PR " + + "and of kix's sessions comment on it. When they name none, an `unknown` model, or two different models, " + + "it is any model of the harness running this command, usually the session that just pushed, detected from the " + + "variables each sets, so Codex reviews from Claude Code and Claude Code from Codex; --harness instead avoids " + + "every model they name. " + "--harness names the reviewer instead, at its entry's model and effort, and --model and --effort " + "override the model and effort; a repository whose AGENTS.md names other reviewers passes them. The " + "size is --size, else the size of the quest a quest/ branch names, read from the base's tip so a " + @@ -1116,7 +1117,7 @@ export function main(argv: string[], out: Output): number | Promise { .option("--effort ", "Reasoning effort the reviewer runs at instead") .option( "--exclude-model ", - "A model that wrote the code, never picked to review it, instead of the PR's record; repeat for each", + "A model never picked to review it, instead of the PR's latest writer; repeat for each", (value: string, previous: string[] = []) => [...previous, value], ) .addOption( diff --git a/src/cli/models.ts b/src/cli/models.ts index afa9bd6e..dd1af733 100644 --- a/src/cli/models.ts +++ b/src/cli/models.ts @@ -33,8 +33,8 @@ export const WRITERS: Record = { }; /** - * Each PR size's reviewers, ranked: the first whose model wrote none of the - * code reviews it, so Opus reviews what Sol wrote, at its own write efforts. + * Each PR size's reviewers, ranked: the first whose model is not the latest + * writer's reviews it, so Opus reviews what Sol changed last, at its own write efforts. */ export const REVIEWERS: Record = { XS: [sol("low"), opus("low")], diff --git a/src/cli/review-record.ts b/src/cli/review-record.ts index ccd2f4f1..cd706a34 100644 --- a/src/cli/review-record.ts +++ b/src/cli/review-record.ts @@ -5,7 +5,7 @@ import { type Comment, recorded, sessionsComment } from "../github/models"; import { type ClaimDependencies, repositoryPath, upstream } from "./claims"; import { git } from "./git"; import { requireRemote, string } from "./github"; -import { type Round, located, parseBase } from "./review"; +import { type PullModels, type Round, located, parseBase } from "./review"; const HARNESSES: Record = { claude: "Claude Code", codex: "Codex" }; @@ -101,7 +101,7 @@ export async function pullModels( baseRef: string, branch: string, deps: ClaimDependencies, -): Promise | null> { +): Promise { const pull = await openPull(root, baseRef, branch, deps); if ("skipped" in pull) return null; // Sessions that published to the PR after it opened are on kix's sessions comment rather than in the body. @@ -110,7 +110,9 @@ export async function pullModels( )) as Comment[]; const own = recorded(pull.body); const later = recorded(sessionsComment(comments)?.body ?? ""); - return { sizes: own.sizes, implementers: [...own.implementers, ...later.implementers] }; + // Each records its sessions in the order they last pushed, but which record a later push went to cannot be told. + const latest = [own, later].flatMap((record) => record.implementers.at(-1)?.model ?? []); + return { sizes: own.sizes, implementers: [...own.implementers, ...later.implementers], latest }; } /** Record `round` in the `## Review` section of its branch's open PR. The edit is the token's user's, which the land gate counts only when that user wrote the PR. */ diff --git a/src/cli/review.ts b/src/cli/review.ts index a80a8c5a..5b87b416 100644 --- a/src/cli/review.ts +++ b/src/cli/review.ts @@ -1,4 +1,4 @@ -// `quest review`: a model that wrote none of the code reviews the committed branch, read-only, +// `quest review`: a model other than the latest writer reviews the committed branch, read-only, // picked by the PR's size from the lists in `models.ts`. // // A branch's first round runs the reviewer CLI's own review, given the scope, the @@ -165,7 +165,7 @@ export function runningHarness(env: NodeJS.ProcessEnv): HarnessName { if (claude) return "claude"; if (codex) return "codex"; throw new ReviewInputError( - "not running under Claude Code or Codex; pass --harness to name the reviewer, or --exclude-model for each model that wrote the code", + "not running under Claude Code or Codex; pass --harness to name the reviewer, or --exclude-model for the model that changed it last", ); } @@ -174,39 +174,43 @@ export interface Choice { harness?: HarnessName; model?: string; effort?: string; - /** The models that wrote the code; without any, the running harness's are excluded. */ + /** The models never to pick; without any, the latest writer's is avoided. */ exclude?: string[]; } /** - * Whether an entry's model wrote the code: one of `exclude`, or else one the - * PR's `## Models` record names (`recorded`), and, unless `--harness` names the - * reviewer or the record names every implementing model, any model of the - * harness running this command, since its own model is not known. + * Whether an entry's model is the one to avoid: one of `exclude`, or else the + * latest writer. That is the model of the last Implementer line the PR's + * `## Models` records name (`latest`, one per record: the body's and kix's + * sessions comment's). When they name none, an `unknown` model, or two + * different models, whose order cannot be told, it is the harness running this + * command, which is usually the session that just pushed, unless `--harness` + * names the reviewer, which then avoids every model they name. */ -export function authored(choice: Choice, env: NodeJS.ProcessEnv, recorded: string[] = []): (entry: Model) => boolean { +export function authored(choice: Choice, env: NodeJS.ProcessEnv, latest: string[] = []): (entry: Model) => boolean { const exclude = choice.exclude ?? []; if (exclude.length > 0) return (entry) => exclude.includes(entry.model); - const known = recorded.filter((model) => model !== "unknown"); - const wrote = (entry: Model) => known.includes(entry.model); - if (choice.harness !== undefined || (known.length > 0 && known.length === recorded.length)) return wrote; + const named = [...new Set(latest)]; + const known = named.filter((model) => model !== "unknown"); + if (named.length === 1 && known.length === 1) return (entry) => entry.model === known[0]; + if (choice.harness !== undefined) return (entry) => known.includes(entry.model); const running = runningHarness(env); - return (entry) => wrote(entry) || entry.harness === running; + return (entry) => entry.harness === running; } /** * The reviewer for a PR of `size`: the first entry of its list whose model - * wrote none of the code, or `--harness`'s entry, with `--model` and - * `--effort` overriding its model and effort. An override never brings back - * a model that wrote the code. + * is not avoided, or `--harness`'s entry, with `--model` and `--effort` + * overriding its model and effort. An override never brings back an avoided + * model. */ -export function pickReviewer(size: Size, choice: Choice, wrote: (entry: Model) => boolean): Model { +export function pickReviewer(size: Size, choice: Choice, avoided: (entry: Model) => boolean): Model { const list = REVIEWERS[size]; const entry = - choice.harness === undefined ? list.find((e) => !wrote(e)) : list.find((e) => e.harness === choice.harness); + choice.harness === undefined ? list.find((e) => !avoided(e)) : list.find((e) => e.harness === choice.harness); if (entry === undefined && choice.harness === undefined) { const models = [...new Set(list.map((e) => e.model))].join(", "); - throw new ReviewInputError(`every ${size} reviewer (${models}) wrote the code; pass --harness and --model`); + throw new ReviewInputError(`every ${size} reviewer (${models}) is avoided; pass --harness and --model`); } const harness = choice.harness ?? entry!.harness; const model = choice.model ?? entry?.model; @@ -215,8 +219,8 @@ export function pickReviewer(size: Size, choice: Choice, wrote: (entry: Model) = throw new ReviewInputError(`no ${harness} reviewer for size ${size}; pass --model and --effort`); } const picked = { harness, model, effort }; - if (wrote(picked)) - throw new ReviewInputError(`${model} wrote the code; pass --model to name another ${harness} model`); + if (avoided(picked)) + throw new ReviewInputError(`${model} is avoided; pass --model to name another ${harness} model`); return picked; } @@ -253,11 +257,11 @@ type BaseOptions = { base?: string; fetched?: false } | { base: string; fetched: export type ReviewOptions = BaseOptions & { /** The checkout to review. */ root: string; - /** The reviewer; defaults to the first of the size's list whose model wrote none of the code. */ + /** The reviewer; defaults to the first of the size's list whose model is not the latest writer's. */ harness?: HarnessName; model?: string; effort?: string; - /** The models that wrote the code; without any, the running harness's are excluded. */ + /** The models never to pick; without any, the latest writer's is avoided. */ exclude?: string[]; /** The PR's size, instead of its quest's or the default. */ size?: Size; @@ -285,7 +289,7 @@ export interface ReviewDependencies { * The `## Models` record of the open PR from `branch` into `base`, or null when it has none; without * it, as in the runner, only the options and the running harness pick the reviewer. */ - models?: (root: string, base: string, branch: string) => Promise | null>; + models?: (root: string, base: string, branch: string) => Promise; } export const processDependencies = (): ReviewDependencies => ({ env: process.env }); @@ -1226,7 +1230,10 @@ function detachedMessage(root: string, baseBranch: string): string { return `${message}, such as ${[...names].map((n) => `--branch ${n}`).join(" or ")}`; } -type ModelsRecord = ReturnType | null | undefined; +/** A PR's `## Models` records: the body's and kix's sessions comment's together, and each one's latest model. */ +export type PullModels = ReturnType & { latest: string[] }; + +type ModelsRecord = PullModels | null | undefined; /** The size the PR's record names, checked only when no `--size` replaces it. */ function recordedSize(record: ModelsRecord, options: ReviewOptions): Size | null { @@ -1241,13 +1248,13 @@ function recordedSize(record: ModelsRecord, options: ReviewOptions): Size | null return size as Size; } -/** The implementing models the PR's record names, checked only when no `--exclude-model` replaces them. */ -function recordedModels(record: ModelsRecord, options: ReviewOptions): string[] { +/** The latest writer each of the PR's records names, checked only when no `--exclude-model` replaces them. */ +function recordedLatest(record: ModelsRecord, options: ReviewOptions): string[] { if (record == null || (options.exclude ?? []).length > 0) return []; - return record.implementers.map(({ item, model }) => { + for (const { item, model } of record.implementers) { if (model === null) throw new ReviewInputError(`the PR's ## Models record names no model in: ${item}`); - return model; - }); + } + return record.latest; } /** Review the checkout's committed branch: one round, resuming the branch's session unless `fresh`. */ @@ -1273,7 +1280,7 @@ export async function review(options: ReviewOptions, deps: ReviewDependencies): const needed = options.size === undefined || (options.exclude ?? []).length === 0; const record = needed ? await deps.models?.(root, baseRef, branch) : undefined; const sized = reviewSize(root, baseSha, options.sizeBranch ?? branch, options.size, recordedSize(record, options)); - const picked = pickReviewer(sized.size, options, authored(options, deps.env, recordedModels(record, options))); + const picked = pickReviewer(sized.size, options, authored(options, deps.env, recordedLatest(record, options))); const harness = picked.harness; const reviewer = CLIS[harness]; diff --git a/src/github/models.ts b/src/github/models.ts index bd47dc04..3dd06b0f 100644 --- a/src/github/models.ts +++ b/src/github/models.ts @@ -41,21 +41,34 @@ export function recorded(body: string): { sizes: string[]; implementers: { item: }; } -/** `body` with `line` as an item of its `## Models` section, which it gains when it has none. */ +/** + * `body` with `line` as the last item of its `## Models` section, which it gains when it has none, so the record's + * last Implementer names the latest session to push. An earlier copy of `line` moves rather than repeats. + */ export function withModels(body: string, line: string): string { const item = `- ${line}`; - if (body.split(/\r?\n/).some((each) => each.trim() === item)) return body; const found = section(body); if (!found) return `${body.trimEnd()}\n\n## Models\n\n${item}\n`; const { start, end } = found; - const gap = body.slice(start, end).trim() ? "\n" : "\n\n"; - return `${body.slice(0, end).trimEnd()}${gap}${item}\n${end < body.length ? "\n" : ""}${body.slice(end)}`; + const lines = body.slice(start, end).split(/\r?\n/); + if ( + lines + .filter((each) => each.trim() !== "") + .at(-1) + ?.trim() === item + ) + return body; + const kept = lines.some((each) => each.trim() === item) + ? lines.filter((each) => each.trim() !== item).join("\n") + : body.slice(start, end); + const gap = kept.trim() ? "\n" : "\n\n"; + return `${(body.slice(0, start) + kept).trimEnd()}${gap}${item}\n${end < body.length ? "\n" : ""}${body.slice(end)}`; } /** The hidden marker of the one PR comment where kix records the sessions that published to an existing PR. */ export const SESSIONS = ""; -/** The sessions comment `existing` with `line` in its `## Models` record, or null when it already has it. */ +/** The sessions comment `existing` with `line` last in its `## Models` record, or null when it already ends with it. */ export function withSession(existing: string | null, line: string): string | null { const body = existing ?? `${SESSIONS}\nThe sessions that published to this pull request after it opened.`; const next = withModels(body, line); diff --git a/test/review.test.ts b/test/review.test.ts index 709f441d..c61f3f26 100644 --- a/test/review.test.ts +++ b/test/review.test.ts @@ -18,7 +18,7 @@ import { verdict, } from "../src/cli/review"; import { withReview } from "../src/cli/review-record"; -import { recorded } from "../src/github/models"; +import { recorded, withModels } from "../src/github/models"; import { tempDir } from "./fixture"; afterEach(() => { @@ -290,7 +290,7 @@ test("the reviewer is the harness this command is not running under", () => { expect(() => runningHarness({ CLAUDECODE: "1", CODEX_THREAD_ID: "t" })).toThrow("both"); }); -test("each size's reviewer is the first whose model wrote none of the code", () => { +test("each size's reviewer is the first whose model is not the latest writer's", () => { const claude = authored({}, { CLAUDECODE: "1" }); const codex = authored({}, { CODEX_THREAD_ID: "t" }); const at = (size: keyof typeof REVIEWERS, wrote: typeof claude) => { @@ -309,29 +309,45 @@ test("each size's reviewer is the first whose model wrote none of the code", () expect(at("L", codex)).toBe("claude claude-opus-5-5 high"); expect(at("XL", codex)).toBe("claude claude-opus-5-5 xhigh"); - // The PR's recorded implementers replace the running harness: Sol reviews Opus's code from Claude Code. + // The PR's latest recorded implementer replaces the running harness, and only it is avoided. + expect(at("S", authored({}, { CLAUDECODE: "1" }, ["claude-opus-5-5"]))).toBe("codex gpt-6.1-sol medium"); + expect(at("S", authored({}, { CLAUDECODE: "1" }, ["gpt-6.1-sol"]))).toBe("claude claude-opus-5-5 low"); + expect(at("S", authored({}, { CLAUDECODE: "1" }, ["gpt-6.1-sol", "gpt-6.1-sol"]))).toBe( + "claude claude-opus-5-5 low", + ); + // When the body's and the sessions comment's latest differ, or one is unknown, their order cannot be told, + // so the running harness, usually the session that just pushed, is taken to be the latest writer. + expect(at("M", authored({}, { CODEX_THREAD_ID: "t" }, ["gpt-6.1-sol", "claude-opus-5-5"]))).toBe( + "claude claude-opus-5-5 medium", + ); + expect(at("M", authored({}, { CLAUDECODE: "1" }, ["gpt-6.1-sol", "claude-opus-5-5"]))).toBe( + "codex gpt-6.1-sol medium", + ); + expect(at("M", authored({}, { CLAUDECODE: "1" }, ["gpt-6.1-sol", "unknown"]))).toBe("codex gpt-6.1-sol medium"); + expect(at("M", authored({}, { CODEX_THREAD_ID: "t" }, ["unknown"]))).toBe("claude claude-opus-5-5 medium"); + // --harness then avoids every model they name. + const either = authored({ harness: "claude" }, {}, ["gpt-6.1-sol", "claude-opus-5-5"]); + expect(() => pickReviewer("M", { harness: "claude" }, either)).toThrow("claude-opus-5-5 is avoided"); expect(at("S", authored({ exclude: ["claude-opus-5-5"] }, { CLAUDECODE: "1" }))).toBe("codex gpt-6.1-sol medium"); expect(at("S", authored({ exclude: ["gpt-6.1-sol"] }, { CLAUDECODE: "1" }))).toBe("claude claude-opus-5-5 low"); // No harness need be detected once the implementers are named. expect(at("M", authored({ exclude: ["other-model"] }, {}))).toBe("codex gpt-6.1-sol medium"); const both = authored({ exclude: ["gpt-6.1-sol", "claude-opus-5-5"] }, {}); - expect(() => pickReviewer("M", {}, both)).toThrow("wrote the code; pass --harness and --model"); + expect(() => pickReviewer("M", {}, both)).toThrow("is avoided; pass --harness and --model"); - // --harness names the reviewer at its entry, unless that entry's model wrote the code. + // --harness names the reviewer at its entry, unless that entry's model is avoided. expect(pickReviewer("L", { harness: "claude" }, authored({ harness: "claude" }, { CLAUDECODE: "1" }))).toEqual({ harness: "claude", model: "claude-opus-5-5", effort: "high", }); const sol = authored({ harness: "codex", exclude: ["gpt-6.1-sol"] }, {}); - expect(() => pickReviewer("M", { harness: "codex" }, sol)).toThrow("gpt-6.1-sol wrote the code; pass --model"); + expect(() => pickReviewer("M", { harness: "codex" }, sol)).toThrow("gpt-6.1-sol is avoided; pass --model"); expect(pickReviewer("M", { harness: "codex", model: "gpt-6-astra" }, sol).model).toBe("gpt-6-astra"); - // Nor does --model bring back a model that wrote the code. - expect(() => pickReviewer("M", { harness: "codex", model: "gpt-6.1-sol" }, sol)).toThrow( - "gpt-6.1-sol wrote the code", - ); + // Nor does --model bring back an avoided model. + expect(() => pickReviewer("M", { harness: "codex", model: "gpt-6.1-sol" }, sol)).toThrow("gpt-6.1-sol is avoided"); const opusWrote = authored({ exclude: ["claude-opus-5-5"] }, {}); - expect(() => pickReviewer("M", { model: "claude-opus-5-5" }, opusWrote)).toThrow("claude-opus-5-5 wrote the code"); + expect(() => pickReviewer("M", { model: "claude-opus-5-5" }, opusWrote)).toThrow("claude-opus-5-5 is avoided"); // --model and --effort override the picked entry. expect(pickReviewer("XS", { effort: "high" }, claude)).toEqual({ harness: "codex", @@ -394,7 +410,7 @@ fakes("--exclude-model picks the reviewer from the PR's implementers instead of const both = ["--exclude-model", "gpt-6.1-sol", "--exclude-model", "claude-opus-5-5"]; const none = await quest("--root", s.repo, "review", ...both, "--new"); expect(none.code).toBe(2); - expect(none.stderr).toContain("every M reviewer (gpt-6.1-sol, claude-opus-5-5) wrote the code"); + expect(none.stderr).toContain("every M reviewer (gpt-6.1-sol, claude-opus-5-5) is avoided"); }); fakes("an unknown harness is an error, never a same-model review", async () => { @@ -1320,7 +1336,7 @@ fakes("a later round checks only what changed since the reviewed head, and the o expect(prompt).toContain("1. high src/lib.ts:2 Off by one"); }); -fakes("the PR's Models record sizes the review and names the models that wrote it", async () => { +fakes("the PR's Models record sizes the review and names its latest writer", async () => { const s = setup(); const body = (...lines: string[]) => ["Body.", "", "## Models", "", ...lines].join("\n"); const pull: FakePull = { number: 9, body: body("- Size: L", "- Implementer: Codex, gpt-6.1-sol, high effort") }; @@ -1331,16 +1347,16 @@ fakes("the PR's Models record sizes the review and names the models that wrote i expect(sol.stderr).toBe(""); expect(sol.stdout).toContain("reviewer claude claude-opus-5-5 high\nsize L ## Models\n"); - // A kix session that pushed later is on the App's sessions comment, so no L reviewer is left; - // a forged copy of the comment is ignored. + // A kix session on the App's sessions comment names another latest writer, and which pushed last cannot + // be told, so the running harness's model is avoided; a forged copy of the comment is ignored. const sessions = (app: { id: number } | null) => ({ body: "\n\n## Models\n\n- Implementer: Claude Code, claude-opus-5-5, high effort\n", performed_via_github_app: app, }); pull.comments = [sessions({ id: 1 })]; - const covered = await quest("--root", s.repo, "review", "--new"); - expect(covered.code).toBe(2); - expect(covered.stderr).toContain("claude-opus-5-5"); + expect((await quest("--root", s.repo, "review", "--new")).stdout).toContain( + "reviewer codex gpt-6.1-sol high\n", + ); pull.comments = [sessions(null)]; expect((await quest("--root", s.repo, "review", "--new")).stdout).toContain( "reviewer claude claude-opus-5-5 high\n", @@ -1351,10 +1367,13 @@ fakes("the PR's Models record sizes the review and names the models that wrote i const flagged = await quest("--root", s.repo, "review", "--new", "--size", "XS", "--exclude-model", "x"); expect(flagged.stdout).toContain("reviewer codex gpt-6.1-sol low\nsize XS --size\n"); - // An implementer whose model is unknown may be the running harness's, which the review then skips. + // Only the latest implementer is avoided, whoever wrote earlier. pull.body = body("- Implementer: Codex, unknown, unknown effort", "- Implementer: Codex, gpt-6.1-sol, high"); - expect((await quest("--root", s.repo, "review", "--new")).code).toBe(2); - pull.body = body("- Implementer: Codex, unknown, unknown effort"); + expect((await quest("--root", s.repo, "review", "--new")).stdout).toContain( + "reviewer claude claude-opus-5-5 medium\n", + ); + // A latest implementer whose model is unknown may be the running harness's, which the review then skips. + pull.body = body("- Implementer: Codex, gpt-6.1-sol, high", "- Implementer: Codex, unknown, unknown effort"); expect((await quest("--root", s.repo, "review", "--new")).stdout).toContain( "reviewer codex gpt-6.1-sol medium\n", ); @@ -1392,6 +1411,17 @@ fakes("the PR's Models record sizes the review and names the models that wrote i } }); +test("a session that pushes again moves its Models line last", () => { + const one = "Implementer: Claude Code, claude-opus-5-5, high effort"; + const two = "Implementer: Codex, gpt-6.1-sol, high effort"; + const body = (...lines: string[]) => + `Body.\n\n## Models\n\n${lines.map((l) => `- ${l}\n`).join("")}\n## Review\n\nNone\n`; + expect(withModels("Body.", one)).toBe(`Body.\n\n## Models\n\n- ${one}\n`); + expect(withModels(body("Size: S", one), two)).toBe(body("Size: S", one, two)); + expect(withModels(body("Size: S", one, two), one)).toBe(body("Size: S", two, one)); + expect(withModels(body(one, two), two)).toBe(body(one, two)); +}); + test("a Models record reads its size and every implementing model", () => { const body = "x\n\n## Models\n\nSize: S\n- Implementer: Claude Code, claude-opus-5-5, high effort\n* Implementer: Codex, unknown, unknown effort\n\n## Review\n\n- Size: XL\n";