diff --git a/quest/m0/README.md b/quest/m0/README.md index b4e213e6..4f549088 100644 --- a/quest/m0/README.md +++ b/quest/m0/README.md @@ -95,7 +95,6 @@ ownership-manifest design: ## Required - [Land through a serial merge queue with an async fixer](/quest/m0/queue-watcher/README.md) - GitHub's merge queue, or the watcher's own where GitHub has none, lands one PR at a time, and a host watcher fixes and re-enqueues each PR it drops -- [quest review continues older sessions](/quest/m0/review-old-state.md) - a review session saved by an older quest continues, or is refused before the reviewer runs - [Kix decision UI](/quest/m0/kix/README.md) - planning and implementation through prompts and summaries, run on the maintainer's own runners - [Route quests to models by size](/quest/m0/model-routing/README.md) - `/quest-spawn` starts each quest on the first model of its size's ranked list that can run, reviews rank the same way, and PRs record size, implementer, and reviewer - [Repositories and hosts follow quest's main](/quest/m0/pin-sync.md) - every push to main reaches each enrolled repository and host through a bot-queued pin bump diff --git a/quest/m0/review-old-state.md b/quest/m0/review-old-state.md deleted file mode 100644 index 436b18f0..00000000 --- a/quest/m0/review-old-state.md +++ /dev/null @@ -1,27 +0,0 @@ -# [S] quest review continues sessions saved by an older quest - -## Goal - -A `quest review` round on a branch whose review session was saved by an -older `quest` either continues it, settling the last round's findings, or -refuses before the reviewer runs with an error naming the fix, such as -`--new`. It never spends a reviewer round and then fails. - -## Plan - -Decided 2026-11-07 (coordinator, follow-up of -[moq-dev/quest#517](https://github.com/moq-dev/quest/pull/517) and -[moq-dev/quest#538](https://github.com/moq-dev/quest/pull/538)): both -iterate agents hit `the reviewer's reply does not settle each of the last -round's undefined finding(s) once` (`src/cli/review.ts`, the `earlier` count), -after the reviewer had already run. #517's session predated -[moq-dev/quest#514](https://github.com/moq-dev/quest/pull/514); #538's -branch was new that day, so reproduce both cases before fixing. - -- Find where the last round's finding count comes from and why it is - `undefined`; `git blame` it to the PR that changed the saved format and - list it under `## Caused by`. -- Validate the saved session when it is loaded, before the reviewer starts. - Read the older format when it carries what the round needs; otherwise - refuse with the error above. -- A test covers a session file in each format the reviewer can meet. diff --git a/src/cli/review.ts b/src/cli/review.ts index de64ab6d..a80a8c5a 100644 --- a/src/cli/review.ts +++ b/src/cli/review.ts @@ -1132,15 +1132,42 @@ export function sessionFile(common: string, branch: string): string { return join(common, "quest", "review", `${digest(branch)}.json`); } -function load(path: string): State | undefined { +function load(path: string): unknown { try { - return JSON.parse(readFileSync(path, "utf8")) as State; + return JSON.parse(readFileSync(path, "utf8")); } catch (error) { if ((error as NodeJS.ErrnoException).code === "ENOENT") return undefined; throw error; } } +/** + * A saved session, read before its reviewer runs. A session saved before + * moq-dev/quest#508 kept only each round's finding count, which no round can + * settle: one with findings is refused, naming `--new`, rather than failing + * after the reviewer spent its round. One saved before moq-dev/quest#514 has + * no `last` round, so it never resumes and its next round reviews anew. + */ +export function parseState(value: unknown): State { + const state = value as Partial | null; + const refuse = (why: string) => + new ReviewInputError(`the saved review session ${why}; pass --new to start a new session`); + if (typeof state !== "object" || state === null || !Array.isArray(state.rounds)) { + throw refuse("is not one quest review reads"); + } + const rounds = state.rounds.map((round) => { + const findings: unknown = round?.findings; + if (Array.isArray(findings)) return round; + if (findings === 0) return { ...round, findings: [] }; + throw refuse( + typeof findings === "number" + ? `was saved by an older quest and keeps only its last round's finding count (${findings}), not the findings to settle` + : "has a round with no findings list", + ); + }); + return { ...state, rounds } as State; +} + /** * The branch whose session a round continues: `name`, checked as a branch * name, or else the checked-out branch's upstream branch, without its remote, @@ -1253,8 +1280,11 @@ export async function review(options: ReviewOptions, deps: ReviewDependencies): const statePath = sessionFile(git(root, ["rev-parse", "--path-format=absolute", "--git-common-dir"]), branch); const dir = dirname(statePath); mkdirSync(dir, { recursive: true }); - const stored = load(statePath); - const state = options.fresh || stored?.reviewer !== harness ? undefined : stored; + const stored = options.fresh ? undefined : load(statePath); + const state = + stored === undefined || (stored as Partial | null)?.reviewer !== harness + ? undefined + : parseState(stored); const previous = state?.rounds.at(-1); if (previous === undefined && options.responses !== undefined) { throw new ResponsesError("--responses answers a previous round, and this branch has none to resume"); diff --git a/test/review.test.ts b/test/review.test.ts index 93bd1824..709f441d 100644 --- a/test/review.test.ts +++ b/test/review.test.ts @@ -13,6 +13,7 @@ import { pickReviewer, review, runningHarness, + parseState, sessionFile, verdict, } from "../src/cli/review"; @@ -1552,3 +1553,31 @@ test("the next step stops for the design checkpoint on every third round that re expect(nextStep(round(3, 2, "low"))).toMatch(/^next finish: /); expect(nextStep(round(4, 1, "low"))).toMatch(/^next finish: /); }); + +test("a saved session reads in each format an older quest wrote, or is refused naming --new", () => { + const finding = { severity: "low", file: "a.ts", line: 1, title: "t", body: "b" }; + // Before moq-dev/quest#508: each round kept only its finding count. + expect(parseState({ rounds: [{ head: "a", findings: 0 }] }).rounds[0].findings).toEqual([]); + expect(() => parseState({ rounds: [{ head: "a", findings: 2 }] })).toThrow(/finding count \(2\).*--new/); + // Before moq-dev/quest#514: findings but no merge base or last round. + expect(parseState({ rounds: [{ head: "a", findings: [finding] }] }).rounds[0].findings).toEqual([finding]); + // The current format. + const current = { rounds: [{ head: "a", mergeBase: "m", findings: [finding] }], last: {} }; + expect(parseState(current)).toEqual(current); + expect(() => parseState(null)).toThrow(/--new/); + expect(() => parseState({ rounds: [{ head: "a" }] })).toThrow(/no findings list.*--new/); +}); + +fakes("a session saved by an older quest is refused before the reviewer runs", async () => { + const s = setup(); + expect((await quest("--root", s.repo, "review")).code).toBe(0); + const path = sessionFile(join(s.repo, ".git"), "feature"); + const saved = JSON.parse(readFileSync(path, "utf8")); + writeFileSync(path, JSON.stringify({ ...saved, rounds: [{ head: saved.rounds[0].head, findings: 1 }] })); + const calls = s.calls().length; + const refused = await quest("--root", s.repo, "review", "--responses", path); + expect(refused.code).not.toBe(0); + expect(refused.stderr).toMatch(/--new/); + expect(s.calls().length).toBe(calls); + expect((await quest("--root", s.repo, "review", "--new")).code).toBe(0); +});