From 632012cc6ec045557f36c486f235ffff3af01e90 Mon Sep 17 00:00:00 2001
From: wassname <1103714+wassname@users.noreply.github.com>
Date: Fri, 3 Jul 2026 10:55:45 +0800
Subject: [PATCH] judge: extract decideSignOff so inconclusive always means
 'ran but failed'

CompleteGoal.execute now delegates to a pure decideSignOff(input, signal,
runJudgeFn) that takes an injected judge runner. judgeModel is never checked
pre-emptively: null just omits --model (buildJudgeArgs), so pi's configured
default runs the judge. The only producers of accepted_inconclusive are now the
judge-error and no-VERDICT paths -- i.e. 'the judge ran but failed', never 'no
model'. Wording (log/result/docstring/README) updated to say 'ran but failed'.

Adds test/decide-signoff.test.ts: null judgeModel still reaches runJudge (no
pre-emptive return); judge error/timeout/no-VERDICT -> accepted_inconclusive
with a 'ran but failed' reason. 11 tests pass, typecheck + lint clean.
---
 README.md                   |   6 +-
 src/index.ts                | 124 ++++++++++++++++++++++++++----------
 test/decide-signoff.test.ts |  63 ++++++++++++++++++
 3 files changed, 157 insertions(+), 36 deletions(-)
 create mode 100644 test/decide-signoff.test.ts

diff --git a/README.md b/README.md
index 385fb07..25ce5b5 100644
--- a/README.md
+++ b/README.md
@@ -93,8 +93,10 @@ plus what's missing.
 - accept: a sign-off line is appended to `## Log` and the agent ticks the goal `[x]` itself. The
   tool-written log line is the audit trail; a hand-tick without one shows in the diff.
 - reject: the goal stays open and the agent gets the missing list.
-- judge unavailable (timeout, transport, no VERDICT line): accepted inconclusive, logged as such.
-  The working agent is never blocked on judge infra.
+- judge ran but failed/errored/timed out, or returned no VERDICT line: accepted inconclusive,
+  logged as such. There is no pre-emptive "no model" path -- a null judgeModel just omits
+  `--model` so pi's configured default runs the judge, so inconclusive always means "ran but
+  failed", never "couldn't start". The working agent is never blocked on judge infra.
 
 ## Prompts
 
diff --git a/src/index.ts b/src/index.ts
index b48add5..5922a5c 100644
--- a/src/index.ts
+++ b/src/index.ts
@@ -18,8 +18,10 @@
  * write is appending a sign-off line to ## Log — the audit trail. The agent ticks [x] itself; a
  * hand-tick without a matching tool-written log line is visible in the diff.
  *
- * Judge unavailable (timeout/transport) => accepted_inconclusive: the working agent is never
- * blocked on judge infra; the log line says the judge didn't run.
+ * Judge ran but failed/errored/timed out, or returned no VERDICT line => accepted_inconclusive: the
+ * working agent is never blocked on judge infra; the log line says the judge ran but failed. There
+ * is no pre-emptive "no model" path -- a null judgeModel just omits --model so pi's configured
+ * default runs the judge, so inconclusive always means "ran but failed", never "couldn't start".
  *
  * All model-facing text lives in prompts.ts, in flow order.
  */
@@ -215,39 +217,19 @@ export default function piGoalsExtension(pi: ExtensionAPI): void {
 
 			const judgeModel = state.judgeModel ?? (ctx.model ? `${ctx.model.provider}/${ctx.model.id}` : null);
 			onUpdate?.({ content: [{ type: "text", text: `Read-only judge (${judgeModel ?? "pi default"}) inspecting: ${params.goal}` }], details: {} });
-			const judge = await runJudge(judgeUser({ goal: params.goal, plan, planPath: PLAN_REL }), judgeModel, ctx.cwd, signal);
-
-			if (signal?.aborted) return result("Sign-off aborted.", true);
-
-			const log = (entry: string) => writePlan(ctx, appendLog(readPlan(ctx), `${stamp()} ${entry}`));
-			// Judge infra failure must not block the working agent: accept inconclusive, say so in the log.
-			if (judge.error) {
-				log(`signed off "${params.goal}" (judge unavailable: ${oneLine(judge.error)})`);
+			// decideSignOff runs the judge and derives the outcome + the one log line. judgeModel is never
+			// checked pre-emptively: null just means pi's configured default runs (buildJudgeArgs omits
+			// --model), so accepted_inconclusive always means "the judge ran but failed", never "no model".
+			const outcome = await decideSignOff(
+				{ goal: params.goal, plan, judgeModel },
+				signal,
+				(task) => runJudge(task, judgeModel, ctx.cwd, signal),
+			);
+			if (outcome.logEntry) {
+				writePlan(ctx, appendLog(readPlan(ctx), `${stamp()} ${outcome.logEntry}`));
 				updateWidget(ctx);
-				return result(
-					`Judge unavailable (${judge.error}). Accepted inconclusive — logged. Tick the goal [x] in ${PLAN_REL} yourself.${judge.output ? `\n\npartial judge output:\n${judge.output}` : ""}`,
-				);
 			}
-
-			const verdictLine = judge.output.split("\n").find((l) => /^\s*VERDICT\s*:/i.test(l)) ?? "";
-			const verdict = /^\s*VERDICT\s*:\s*(accept|reject)\s*$/i.exec(verdictLine)?.[1]?.toLowerCase();
-			const reasoning = judge.output.length > 2000 ? `...\n${judge.output.slice(-2000)}` : judge.output;
-
-			if (verdict === "accept") {
-				log(`signed off "${params.goal}" (judge accept)`);
-				updateWidget(ctx);
-				return result(`Sign-off ACCEPTED. Tick the goal [x] in ${PLAN_REL} (the log line is already appended).\n\n--- judge ---\n${reasoning}`);
-			}
-			if (verdict === "reject") {
-				const missing = judge.output.match(/missing\s*:\s*([\s\S]*)$/i)?.[1].trim() || judge.output.slice(-500);
-				log(`reject "${params.goal}": ${oneLine(missing)}`);
-				updateWidget(ctx);
-				return result(`Sign-off REJECTED. Missing:\n${missing}\n\n--- judge ---\n${reasoning}`, true);
-			}
-			// No VERDICT line: same fail-forward as judge-unavailable.
-			log(`signed off "${params.goal}" (judge inconclusive: no VERDICT line)`);
-			updateWidget(ctx);
-			return result(`Judge returned no VERDICT line. Accepted inconclusive — logged. Tick the goal [x] in ${PLAN_REL} yourself.\n\n--- judge ---\n${reasoning || "(no output)"}`);
+			return result(outcome.resultText, outcome.isError);
 		},
 	});
 }
@@ -266,6 +248,80 @@ function oneLine(s: string): string {
 	return s.replace(/\s+/g, " ").trim().slice(0, 200);
 }
 
+/** A judge run's result: stdout output, plus an error string when the subprocess failed/timed out. */
+export interface JudgeResult {
+	output: string;
+	error?: string;
+}
+
+/** Inputs to a sign-off decision. judgeModel is null when no explicit/session model is set. */
+export interface SignOffInput {
+	goal: string;
+	plan: string;
+	judgeModel: string | null;
+}
+
+/** The outcome of a sign-off: the reply text, whether it's a hard error, and the one ## Log line to
+ *  append (null when nothing should be written, e.g. aborted before any verdict). */
+export interface SignOffOutcome {
+	resultText: string;
+	isError: boolean;
+	logEntry: string | null;
+}
+
+/** Run the judge and decide accept / reject / accepted_inconclusive. Pure aside from the injected
+ *  judge runner, so the unit test can lock the fail-forward invariant: judgeModel is NEVER checked
+ *  here, so a null model still reaches runJudge (pi's configured default runs it), and the only
+ *  producers of accepted_inconclusive are the judge-error and no-VERDICT paths -- i.e. "the judge
+ *  ran but failed", never "no model". The execute() wrapper does the plan-file write + widget.
+ *  Exported for the unit test that locks this invariant. */
+export async function decideSignOff(
+	input: SignOffInput,
+	signal: AbortSignal | undefined,
+	runJudgeFn: (task: string) => Promise<JudgeResult>,
+): Promise<SignOffOutcome> {
+	const task = judgeUser({ goal: input.goal, plan: input.plan, planPath: PLAN_REL });
+	const judge = await runJudgeFn(task);
+
+	if (signal?.aborted) return { resultText: "Sign-off aborted.", isError: true, logEntry: null };
+
+	// Judge ran but failed/errored/timed out: fail forward, say so in the log.
+	if (judge.error) {
+		const partial = judge.output ? `\n\npartial judge output:\n${judge.output}` : "";
+		return {
+			resultText: `Judge ran but failed (${judge.error}). Accepted inconclusive — logged. Tick the goal [x] in ${PLAN_REL} yourself.${partial}`,
+			isError: false,
+			logEntry: `signed off "${input.goal}" (judge inconclusive: ran but failed: ${oneLine(judge.error)})`,
+		};
+	}
+
+	const verdictLine = judge.output.split("\n").find((l) => /^\s*VERDICT\s*:/i.test(l)) ?? "";
+	const verdict = /^\s*VERDICT\s*:\s*(accept|reject)\s*$/i.exec(verdictLine)?.[1]?.toLowerCase();
+	const reasoning = judge.output.length > 2000 ? `...\n${judge.output.slice(-2000)}` : judge.output;
+
+	if (verdict === "accept") {
+		return {
+			resultText: `Sign-off ACCEPTED. Tick the goal [x] in ${PLAN_REL} (the log line is already appended).\n\n--- judge ---\n${reasoning}`,
+			isError: false,
+			logEntry: `signed off "${input.goal}" (judge accept)`,
+		};
+	}
+	if (verdict === "reject") {
+		const missing = judge.output.match(/missing\s*:\s*([\s\S]*)$/i)?.[1].trim() || judge.output.slice(-500);
+		return {
+			resultText: `Sign-off REJECTED. Missing:\n${missing}\n\n--- judge ---\n${reasoning}`,
+			isError: true,
+			logEntry: `reject "${input.goal}": ${oneLine(missing)}`,
+		};
+	}
+	// No VERDICT line: same fail-forward as a judge error -- the judge ran but didn't answer.
+	return {
+		resultText: `Judge returned no VERDICT line. Accepted inconclusive — logged. Tick the goal [x] in ${PLAN_REL} yourself.\n\n--- judge ---\n${reasoning || "(no output)"}`,
+		isError: false,
+		logEntry: `signed off "${input.goal}" (judge inconclusive: no VERDICT line)`,
+	};
+}
+
 /** Append one line under ## Log (creating the section at EOF if absent). The extension's only write. */
 export function appendLog(text: string, entry: string): string {
 	const lines = text.split("\n");
@@ -307,7 +363,7 @@ async function runJudge(
 	judgeModel: string | null,
 	cwd: string,
 	signal: AbortSignal | undefined,
-): Promise<{ output: string; error?: string }> {
+): Promise<JudgeResult> {
 	const args = buildJudgeArgs(judgeModel);
 	args.push(task);
 	const inv = getPiInvocation(args);
diff --git a/test/decide-signoff.test.ts b/test/decide-signoff.test.ts
new file mode 100644
index 0000000..a2d4532
--- /dev/null
+++ b/test/decide-signoff.test.ts
@@ -0,0 +1,63 @@
+import { describe, expect, it, vi } from "vitest";
+import { decideSignOff, type JudgeResult } from "../src/index.js";
+
+// decideSignOff is the fail-forward invariant: judgeModel is NEVER checked pre-emptively, so a null
+// model still reaches runJudge (pi's configured default runs it), and the only producers of
+// accepted_inconclusive are the judge-error and no-VERDICT paths -- i.e. "the judge ran but failed",
+// never "no model". The judge runner is injected so these tests never spawn a real subprocess.
+describe("decideSignOff (fail-forward invariant)", () => {
+	it("proceeds to runJudge even when judgeModel is null (no pre-emptive 'no model' inconclusive)", async () => {
+		const runJudge = vi.fn().mockResolvedValue({ output: "VERDICT: accept\nall good" });
+		const out = await decideSignOff({ goal: "x", plan: "# plan\n1. [ ] goal: x\n", judgeModel: null }, undefined, runJudge);
+		expect(runJudge).toHaveBeenCalledOnce(); // reached the judge -- no pre-emptive return on null model
+		expect(out.isError).toBe(false);
+		expect(out.logEntry).toContain("judge accept");
+	});
+
+	it("a judge-subprocess error yields accepted_inconclusive with a 'ran but failed' reason", async () => {
+		const runJudge = vi.fn().mockResolvedValue({ output: "", error: "judge subprocess exited 1" } satisfies JudgeResult);
+		const out = await decideSignOff({ goal: "x", plan: "# plan\n", judgeModel: null }, undefined, runJudge);
+		expect(runJudge).toHaveBeenCalledOnce();
+		expect(out.isError).toBe(false); // accepted inconclusive, not a hard error that blocks the agent
+		expect(out.resultText.toLowerCase()).toContain("accepted inconclusive");
+		expect(out.resultText).toContain("ran but failed"); // inconclusive means ran but failed, not "no model"
+		expect(out.logEntry).toContain("ran but failed");
+		expect(out.logEntry).toContain("subprocess exited 1");
+	});
+
+	it("a judge timeout is also accepted_inconclusive (ran but failed)", async () => {
+		const runJudge = vi.fn().mockResolvedValue({ output: "partial", error: "judge timed out after 600s" });
+		const out = await decideSignOff({ goal: "x", plan: "# plan\n", judgeModel: null }, undefined, runJudge);
+		expect(out.isError).toBe(false);
+		expect(out.resultText.toLowerCase()).toContain("accepted inconclusive");
+		expect(out.logEntry).toContain("ran but failed");
+		expect(out.logEntry).toContain("timed out");
+		expect(out.resultText).toContain("partial judge output:\npartial");
+	});
+
+	it("no VERDICT line is accepted_inconclusive too (judge ran but didn't answer)", async () => {
+		const runJudge = vi.fn().mockResolvedValue({ output: "I looked but forgot the verdict line" });
+		const out = await decideSignOff({ goal: "x", plan: "# plan\n", judgeModel: null }, undefined, runJudge);
+		expect(out.isError).toBe(false);
+		expect(out.resultText).toContain("no VERDICT line");
+		expect(out.logEntry).toContain("no VERDICT line");
+	});
+
+	it("rejects when the judge returns VERDICT: reject", async () => {
+		const runJudge = vi.fn().mockResolvedValue({ output: "VERDICT: reject\nmissing: evidence, tests" });
+		const out = await decideSignOff({ goal: "x", plan: "# plan\n", judgeModel: "openrouter/claude" }, undefined, runJudge);
+		expect(out.isError).toBe(true);
+		expect(out.resultText).toContain("REJECTED");
+		expect(out.resultText).toContain("evidence, tests");
+		expect(out.logEntry).toContain("reject");
+	});
+
+	it("writes nothing when aborted after the judge ran", async () => {
+		const runJudge = vi.fn().mockResolvedValue({ output: "VERDICT: accept" });
+		const ctrl = new AbortController();
+		ctrl.abort();
+		const out = await decideSignOff({ goal: "x", plan: "# plan\n", judgeModel: null }, ctrl.signal, runJudge);
+		expect(out.logEntry).toBeNull();
+		expect(out.isError).toBe(true);
+	});
+});