mirror of
https://github.com/wassname/pi-goals.git
synced 2026-07-24 13:10:41 +08:00
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.
This commit is contained in:
wassname
@@ -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
|
||||
|
||||
|
||||
+90
-34
@@ -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);
|
||||
|
||||
@@ -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);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user