feat(review): worker self-check gate before review_step; reviewer round semantics (#657)

CHANGELOG.md

@@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### New
+
+- **Worker self-check before review; reviewer round semantics** (#657). Review cycles, not authoring, dominated task wall time (penster TP-1921: ~52 min authoring vs ~117 min in review cycles; every gate went to the 2-round cap, and the round-2 findings were invariants the worker had written in its own Step 0 design). Two base-template changes: (1) `review_step` (code/test) now **refuses** unless STATUS.md carries a `### Self-check (Step N)` table placed after the step's checkboxes — one row per outcome / Step 0 design decision / Completion Criterion (and prior-review finding on round ≥ 2) with `file:line` evidence; opt out with `taskRunner.worker.requireSelfCheck: false`. (2) Every review request states its **round** and, from round 2, the prior review path: round 1 is exhaustive, round ≥ 2 verifies the fold; a new finding that is a genuine defect (regression from the fold, bug, unmet requirement) still blocks at any severity, while other new observations block only at the top severity label (`taskRunner.reviewer.round2NewFindings`: `"p0-only"` default | `"any"`) and otherwise go to Suggestions. A round only counts as "round 2" when a prior REVISE/RETHINK review of the gate exists. Worker/reviewer templates and the reviewer tool guidance carry the contract.
+
 ### Fixed
 
 - **A released hold left the task badged `held` and let its lane successor show `running` with the wrong telemetry** (#651). Three writers: (1) the strict hold-store write that persists a release (or an open) now projects the bound task's record in the same write — open → `held`, released → `running` with the stale hold-timeout reason cleared — instead of waiting for the next unrelated task-transition persist; (2) resume reconciliation decides "session alive" per **task** from the registry manifests' task ids, so a not-yet-started successor sharing the lane's session name stays `pending` (legacy registries without task ids keep the old session semantics); (3) the dashboard attaches a lane's live worker telemetry to the task named by the lane snapshot — including a row still badged `held` — never to a sibling that merely has `status: running`.

docs/reference/configuration/task-runner.yaml.md

@@ -88,6 +88,7 @@ If a task path matches a configured task area, that area's override applies.
 | `worker.spawn_mode` | `"subprocess"` \| `"tmux"` | commented in template | Optional spawn mode override for task-runner. |
 | `worker.exit_intercept_timeout_sec` | number | `60` (15..1800) | How long the lane waits for a supervisor reply when it intercepts a worker's premature exit before letting the session close. Raise it when the supervisor is often inside long tool calls (a blocking `--wait` cannot answer in 60 s). In `.pi/taskplane-config.json` (the preferred project config) the key is camelCase: `taskRunner.worker.exitInterceptTimeoutSec`. |
 | `worker.hold_timeout_minutes` | number | `240` (5..10080) | How long a lane may stay **held** (worker escalated, waiting for a typed ruling, no worker process running) before the batch parks with pause cause `hold-timeout`. Acknowledgements never extend it; the hold stays open when it expires — rule, then `orch_resume(force=true)`. JSON key: `taskRunner.worker.holdTimeoutMinutes`. |
+| `worker.require_self_check` | boolean | `true` | `review_step` (code/test reviews) refuses unless STATUS.md has a `### Self-check (Step N)` table for the step, placed after its checkboxes: one row per outcome / Step 0 design decision / Completion Criterion (and, on round ≥ 2, per prior finding) with `file:line` evidence. Most round-2 findings in the field were invariants the worker had already written down. Set `false` to opt out. JSON key: `taskRunner.worker.requireSelfCheck`. |
 
 Notes:
 - `spawn_mode` defaults to `subprocess` when not set.
@@ -101,6 +102,7 @@ Notes:
 | `reviewer.model` | string | `""` | Reviewer model (empty = inherit session model). |
 | `reviewer.tools` | string | `"read,write,bash,grep,find,ls"` | Tool allowlist for reviewer agent. |
 | `reviewer.thinking` | string | `"off"` | Thinking mode for reviewer. Empty string (or explicit `"inherit"`) = inherit active session thinking. |
+| `reviewer.round2_new_findings` | `"p0-only"` \| `"any"` | `"p0-only"` | Round semantics for a gate that already has a REVISE. Round 1 is exhaustive; round ≥ 2 verifies the fold, and a *new* finding is blocking only at the top severity label (`p0-only`) or at any severity (`any`) — everything else goes to Suggestions with an APPROVE verdict. The review request states the round and the prior review path. JSON key: `taskRunner.reviewer.round2NewFindings`. |
 
 ### `context`
 

extensions/reviewer-extension.ts

@@ -55,6 +55,7 @@ export default function reviewerExtension(pi: ExtensionAPI) {
 			"After writing your review to the specified output file, call wait_for_review() again.",
 			"When it returns 'SHUTDOWN', exit cleanly — the task is complete.",
 			"Reference your previous reviews when relevant (e.g., 'I flagged X in Step 1 — checking if addressed').",
+			"Round semantics (#657): each review request states its round. Round 1 is EXHAUSTIVE — list every finding now. Round ≥ 2 VERIFIES THE FOLD against the prior review. A new finding that is a genuine defect (regression from the fold, bug, unmet requirement) blocks at ANY severity; other new observations block only at the top severity label unless the request says otherwise and otherwise go to Suggestions. Never manufacture an APPROVE — an exhausted round budget is the worker's escalation, not your approval.",
 		],
 		parameters: Type.Object({}),
 		async execute() {

extensions/taskplane/agent-bridge-extension.ts

@@ -32,7 +32,7 @@ import {
 	renameSync,
 	unlinkSync,
 } from "fs";
-import { join, dirname } from "path";
+import { join } from "path";
 import { spawn as nodeSpawn } from "child_process";
 import { resolvePiCliPath, resolveTaskplaneAgentTemplate } from "./path-resolver.ts";
 import { loadPiSettingsPackages, filterExcludedExtensions } from "./settings-loader.ts";
@@ -216,6 +216,131 @@ export function hasOutstandingNonApproveReview(
 	}
 }
 
+/**
+ * #657: the worker's pre-review self-check. A `### Self-check (Step N)` section
+ * (heading level 3–4, case-insensitive) must exist in STATUS.md, contain at
+ * least one table data row or list item, and appear AFTER the step's last
+ * checked checkbox in the file — i.e. it was written once the step's work was
+ * done, not pasted in early. Returns null when satisfied, else the refusal.
+ */
+export function selfCheckRefusal(statusContent: string, stepNum: number): string | null {
+	const text = statusContent.replace(/\r\n/g, "\n");
+	const headingRe = new RegExp(
+		"^#{3,4}\\s+Self-check\\s*\\(\\s*Step\\s+" + String(stepNum) + "\\s*\\)\\s*$",
+		"im",
+	);
+	const m = headingRe.exec(text);
+	if (!m) {
+		return `no "### Self-check (Step ${stepNum})" section in STATUS.md`;
+	}
+	const sectionStart = m.index + m[0].length;
+	const rel = text.slice(sectionStart).search(/^#{1,4}\s/m);
+	const body = text.slice(sectionStart, rel === -1 ? undefined : sectionStart + rel);
+	// Data rows: table rows that are neither a separator (|---|) nor the header
+	// row directly above a separator; plus list items. Structural, not keyword-based.
+	const bodyLines = body.split("\n");
+	const isSep = (l: string) => /^\s*\|?\s*:?-{3,}/.test(l);
+	const isTableRow = (l: string) => /^\s*\|.*\|\s*$/.test(l);
+	const dataRows = bodyLines.filter((l, idx) => {
+		if (isSep(l)) return false;
+		if (isTableRow(l)) {
+			let k = idx + 1;
+			while (k < bodyLines.length && bodyLines[k].trim() === "") k++;
+			return !(k < bodyLines.length && isSep(bodyLines[k])); // header row sits above the separator
+		}
+		return /^\s*[-*]\s+\S/.test(l);
+	});
+	if (dataRows.length === 0) {
+		return `"### Self-check (Step ${stepNum})" section has no rows — list each outcome / design decision / completion criterion with file:line evidence`;
+	}
+	// Freshness: the section must come after the step's last checked box.
+	const stepHeadRe = new RegExp("^###\\s+Step\\s+" + String(stepNum) + "\\b", "m");
+	const sh = stepHeadRe.exec(text);
+	if (sh) {
+		const stepStart = sh.index;
+		const stepEndRel = text.slice(stepStart + 1).search(/^###\s+Step\s+\d+/m);
+		const stepBody = text.slice(
+			stepStart,
+			stepEndRel === -1 ? undefined : stepStart + 1 + stepEndRel,
+		);
+		let lastChecked = -1;
+		const cb = /^\s*-\s*\[x\]/gim;
+		let cm: RegExpExecArray | null = cb.exec(stepBody);
+		while (cm !== null) {
+			lastChecked = stepStart + cm.index;
+			cm = cb.exec(stepBody);
+		}
+		if (lastChecked !== -1 && m.index < lastChecked) {
+			return `"### Self-check (Step ${stepNum})" appears before the step's last checked box — re-run the self-check after finishing the step's work`;
+		}
+	}
+	return null;
+}
+
+/**
+ * #657: round number for this gate = 1 + the number of prior reviews of this
+ * gate that actually returned REVISE/RETHINK. An APPROVE, an incomplete file
+ * or an unparseable verdict is not evidence of a completed exhaustive review
+ * of the current work, so it does not narrow the next review's scope (Sage).
+ */
+export function reviewRoundForGate(
+	reviewsDir: string,
+	reviewType: string,
+	stepNum: number,
+): { round: number; priorReviewPath: string | null } {
+	try {
+		if (!existsSync(reviewsDir)) return { round: 1, priorReviewPath: null };
+		const suffix = `-${reviewType.toLowerCase()}-step${stepNum}.md`;
+		const revised = readdirSync(reviewsDir)
+			.filter((f) => /^R\d{3}-/.test(f) && f.toLowerCase().endsWith(suffix))
+			.sort()
+			.filter((f) => {
+				try {
+					const v = parseReviewVerdict(readFileSync(join(reviewsDir, f), "utf-8"));
+					return v === "REVISE" || v === "RETHINK";
+				} catch {
+					return false;
+				}
+			});
+		return {
+			round: revised.length + 1,
+			priorReviewPath: revised.length > 0 ? join(reviewsDir, revised[revised.length - 1]) : null,
+		};
+	} catch {
+		return { round: 1, priorReviewPath: null };
+	}
+}
+
+/** #657: lines appended to the reviewer request describing the round's scope. */
+export function buildReviewRoundLines(
+	round: number,
+	priorReviewPath: string | null,
+	round2Mode: "p0-only" | "any",
+): string[] {
+	if (round === 1) {
+		return [
+			"## Review round: 1 of 2 (exhaustive)",
+			"This is the FIRST review of this gate. List EVERY finding now, with severity. A REVISE that " +
+				"withholds findings for a later round is a defect: the gate has a hard 2-round cap and round 2 " +
+				"is verify-only.",
+		];
+	}
+	return [
+		`## Review round: ${round} (verify the fold)`,
+		`Prior review for this gate: \`${priorReviewPath ?? "(unknown)"}\``,
+		"Scope: verify that each finding in the prior review is addressed (cite file:line).",
+		"A NEW finding is BLOCKING (REVISE) at any severity label when it is a genuine correctness defect: " +
+			"a regression introduced by the fold, a bug, or a stated requirement that is not met. Never downgrade " +
+			"a defect to a Suggestion because of the round.",
+		round2Mode === "any"
+			? "Other new observations (hardening, style, extra tests, robustness beyond the stated requirements) may also be raised as findings."
+			: "Other new observations (hardening, style, extra tests, robustness beyond the stated requirements) are " +
+				'blocking ONLY at the top severity label (P0 / critical); below that they go in "### Suggestions" and do not change the verdict.',
+		"Verdict: REVISE if any prior finding is unaddressed or any blocking finding remains; otherwise APPROVE. " +
+			"If the round budget is exhausted the worker escalates — do not manufacture an APPROVE.",
+	];
+}
+
 export function isStepMarkedComplete(statusPath: string, stepNum: number): boolean {
 	let content: string;
 	try {
@@ -899,6 +1024,44 @@ export default function (pi: ExtensionAPI) {
 				/* default 0 */
 			}
 
+			// ── #657: self-check gate (code/test reviews) ─────────────────
+			// The worker must have diffed its draft against its own design, the
+			// step's outcomes and the Completion Criteria before asking for a
+			// review. Opt-out: taskRunner.worker.requireSelfCheck = false.
+			if (reviewType !== "plan" && process.env.TASKPLANE_REQUIRE_SELF_CHECK !== "0") {
+				let statusForCheck = "";
+				try {
+					statusForCheck = readFileSync(statusPath, "utf-8");
+				} catch {
+					/* fall through — refusal below names the missing section */
+				}
+				const refusal = selfCheckRefusal(statusForCheck, stepNum);
+				if (refusal) {
+					return {
+						content: [
+							{
+								type: "text" as const,
+								text:
+									`REFUSED: self-check missing — ${refusal}.\n\n` +
+									`Before requesting a code review, add a "### Self-check (Step ${stepNum})" section to STATUS.md ` +
+									`(directly under the step, AFTER its checkboxes) with one row per: (a) step checkbox/outcome, ` +
+									`(b) each Step 0 design decision this step implements, (c) each PROMPT Completion Criterion it ` +
+									`touches, and — on round ≥ 2 — (d) each finding from the prior review file. Each row carries ` +
+									`file:line evidence and OK / "fixed in <hash>". Fix anything not OK FIRST, then call review_step ` +
+									`again. Most round-2 findings in the field were invariants the worker had already written down.`,
+							},
+						],
+						details: undefined,
+					};
+				}
+			}
+
+			// #657: round semantics for the reviewer prompt
+			const roundInfo = reviewRoundForGate(reviewsDir, reviewType, stepNum);
+			const round2Mode: "p0-only" | "any" =
+				process.env.TASKPLANE_REVIEW_ROUND2_NEW_FINDINGS === "any" ? "any" : "p0-only";
+			const roundLines = buildReviewRoundLines(roundInfo.round, roundInfo.priorReviewPath, round2Mode);
+
 			reviewCounter++;
 			const num = String(reviewCounter).padStart(3, "0");
 			const outputPath = join(reviewsDir, `R${num}-${reviewType}-step${stepNum}.md`);
@@ -945,6 +1108,8 @@ export default function (pi: ExtensionAPI) {
 					`2. Read STATUS.md for progress so far`,
 					`3. Evaluate the plan for this step`,
 					``,
+					...roundLines,
+					``,
 					`## Output`,
 					`Write your review to: \`${outputPath}\``,
 				].join("\n");
@@ -965,6 +1130,9 @@ export default function (pi: ExtensionAPI) {
 					`1. Run \`${diffNamesCmd}\` to see changed files`,
 					`2. Run \`${diffCmd}\` for the full diff`,
 					`3. Read changed files for context`,
+					`4. Read the worker's "### Self-check (Step ${stepNum})" table in STATUS.md — verify its evidence rather than re-deriving it`,
+					``,
+					...roundLines,
 					``,
 					`## Output`,
 					`Write your review to: \`${outputPath}\``,

extensions/taskplane/config-loader.ts

@@ -1274,13 +1274,19 @@ export function toTaskRunnerConfig(config: TaskplaneConfig): import("./types.ts"
 			...(typeof config.taskRunner.worker.holdTimeoutMinutes === "number"
 				? { holdTimeoutMinutes: config.taskRunner.worker.holdTimeoutMinutes }
 				: {}),
+			...(typeof config.taskRunner.worker.requireSelfCheck === "boolean"
+				? { requireSelfCheck: config.taskRunner.worker.requireSelfCheck }
+				: {}),
 		},
 		model_fallback: config.taskRunner.modelFallback ?? "inherit",
 		reviewer: {
 			model: config.taskRunner.reviewer.model,
 			thinking: config.taskRunner.reviewer.thinking,
 			tools: config.taskRunner.reviewer.tools,
 			excludeExtensions: [...(config.taskRunner.reviewer.excludeExtensions ?? [])],
+			...(config.taskRunner.reviewer.round2NewFindings
+				? { round2NewFindings: config.taskRunner.reviewer.round2NewFindings }
+				: {}),
 			...(config.taskRunner.reviewer.severityLabels
 				? { severityLabels: [...config.taskRunner.reviewer.severityLabels] }
 				: {}),

extensions/taskplane/config-schema.ts

@@ -132,6 +132,15 @@ export interface WorkerConfig {
 	 * bounded to 5..10080 (7 days).
 	 */
 	holdTimeoutMinutes?: number;
+	/**
+	 * #657: `review_step` refuses when STATUS.md has no `### Self-check (Step N)`
+	 * section for the step (or the section is older than the step's last
+	 * checkbox flip). The worker must diff its draft against its own design,
+	 * the step's outcomes and the Completion Criteria BEFORE asking for a
+	 * review — most round-2 findings in the field were invariants the worker
+	 * had written down itself. Default true.
+	 */
+	requireSelfCheck?: boolean;
 }
 
 /** Reviewer agent configuration */
@@ -170,6 +179,13 @@ export interface ReviewerConfig {
 	severityLabels: string[];
 	/** Revision-spiral detection tuning. */
 	spiral: ReviewSpiralConfig;
+	/**
+	 * #657: round semantics for a gate that already has a REVISE. Round 1 is
+	 * exhaustive; round 2 verifies the fold. New findings in round ≥ 2 are
+	 * blocking only at the top severity label ("p0-only", default) or always
+	 * ("any"). Everything else goes to Suggestions with an APPROVE verdict.
+	 */
+	round2NewFindings?: "p0-only" | "any";
 }
 
 /** Context/resource limits for task execution */

extensions/taskplane/engine.ts

@@ -2112,9 +2112,15 @@ async function attemptModelFallbackRetry(
 			// Pass TASKPLANE_MODEL_FALLBACK=1 as extra env var to signal
 			// the task-runner to use the session model instead of configured model.
 			// TP-089: Also include ORCH_BATCH_ID so mailbox steering works for retries.
+			// #657 (Sage): forward every NON-model worker setting (self-check opt-out,
+			// intercept window, hold timeout, tools, thinking) exactly as the normal
+			// wave does — only the configured model is withheld so the retry uses the
+			// session model instead of the one that just failed.
+			const { model: _unavailableModel, ...workerWithoutModel } = runnerConfig?.worker ?? {};
 			const modelFallbackEnv = {
 				TASKPLANE_MODEL_FALLBACK: "1",
 				ORCH_BATCH_ID: batchState.batchId,
+				...buildWorkerEnv(runnerConfig?.worker ? workerWithoutModel : undefined),
 				...buildReviewerEnv(runnerConfig?.reviewer),
 				...buildWorkerExcludeEnv(runnerConfig?.workerExcludeExtensions),
 			};
@@ -2447,13 +2453,17 @@ async function attemptStaleWorktreeRecovery(
 			excludeExtensions: runnerConfig?.reviewer?.excludeExtensions ?? [],
 			severityLabels: runnerConfig?.reviewer?.severityLabels,
 			spiral: runnerConfig?.reviewer?.spiral,
+			round2NewFindings: runnerConfig?.reviewer?.round2NewFindings,
 		},
 		runnerConfig?.worker
 			? {
 					model: runnerConfig.worker.model || "",
 					thinking: runnerConfig.worker.thinking || "",
 					tools: runnerConfig.worker.tools || "",
 					excludeExtensions: runnerConfig.worker.excludeExtensions ?? [],
+					exitInterceptTimeoutSec: runnerConfig.worker.exitInterceptTimeoutSec,
+					holdTimeoutMinutes: runnerConfig.worker.holdTimeoutMinutes,
+					requireSelfCheck: runnerConfig.worker.requireSelfCheck,
 				}
 			: undefined,
 		runnerConfig?.workerExcludeExtensions ?? [],
@@ -3241,13 +3251,17 @@ export async function executeOrchBatch(
 				excludeExtensions: runnerConfig?.reviewer?.excludeExtensions ?? [],
 				severityLabels: runnerConfig?.reviewer?.severityLabels,
 				spiral: runnerConfig?.reviewer?.spiral,
+				round2NewFindings: runnerConfig?.reviewer?.round2NewFindings,
 			},
 			runnerConfig?.worker
 				? {
 						model: runnerConfig.worker.model || "",
 						thinking: runnerConfig.worker.thinking || "",
 						tools: runnerConfig.worker.tools || "",
 						excludeExtensions: runnerConfig.worker.excludeExtensions ?? [],
+						exitInterceptTimeoutSec: runnerConfig.worker.exitInterceptTimeoutSec,
+						holdTimeoutMinutes: runnerConfig.worker.holdTimeoutMinutes,
+						requireSelfCheck: runnerConfig.worker.requireSelfCheck,
 					}
 				: undefined,
 			runnerConfig?.workerExcludeExtensions ?? [],