fix(review): read or refuse sessions saved by an older quest

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

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<State> | 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<State> | 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");

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);
+});