This commit is contained in:
wassname
2026-07-02 06:29:20 +08:00
6 changed files with 268 additions and 36 deletions
+10 -5
View File
@@ -134,15 +134,20 @@ Latency target came from the SLO review; keep the existing client API.
else is the agent editing the file. It reads the goal's `evidence:` block from `.pi/goals.md`, then:
1. If the goal has a `verify:` command, it runs. A non-zero exit rejects right away, no model call.
2. Otherwise a read-only `pi` subprocess (a fresh `--no-session` context, so it never sees the working
2. Then a read-only `pi` subprocess (a fresh `--no-session` context, so it never sees the working
agent's transcript) inspects the `evidence:` against the repo, the `discriminator`, and the
`subtle failure mode`. It re-derives from the cited artifacts rather than trusting the claim, so
list real artifacts, not assertions.
3. On accept, the goal flips to `[x]` and a `## Log` line is written. On reject, it stays open and the
agent is told what is missing. Either way the judge's reasoning comes back in the result.
3. On accept, the goal flips to `[x]` and a `## Log` line is written. On judge reject, it stays open
and the agent is told what is missing. If the judge subprocess times out or its transport/model
fails after any `verify:` command has passed, the goal still flips to `[x]` with a `judge
inconclusive` log line and any partial judge output in the result. Either way the judge's
reasoning comes back in the result.
The judge defaults to your current model (a fresh context, same weights). Point it at another with
`/goals judge <provider/model>` for an independent cross-family check.
The judge defaults to the current session model and streams partial output while it runs. If the
current model is not visible to the extension, `CompleteGoal` does not fall back to Pi's implicit
default; it signs off as `judge inconclusive` and tells you to set `/goals judge <provider/model>`.
Point it at another model for an independent cross-family check.
## Prompts
@@ -0,0 +1,71 @@
# CompleteGoal fail-forward on judge failure
## Goal
Make `CompleteGoal` stop rejecting verified goals just because the read-only judge subprocess times out. Keep the judge useful when it works, and make failures explicit in the log/result.
## Scope
In: `CompleteGoal` sign-off behavior, judge transport, tests, docs.
Out: broader autonomous loop work, plan-mode UX, model auto-selection.
## Requirements
- R1: If `verify:` fails, the goal is rejected immediately. Done means: existing `verify_failed` behavior remains. VERIFY: unit test for pure sign-off record still passes.
- R2: If `verify:` passes and the judge accepts, mark the goal done as before. Done means: log records normal judge accept. VERIFY: unit test for accepted sign-off still passes.
- R3: If any `verify:` command passes but the judge times out or subprocess/model transport fails, mark the goal done with an explicit inconclusive-judge log. Goals without `verify:` use the same fail-forward rule once evidence exists. VERIFY: a unit test records accepted status and a log line containing `judge inconclusive`.
- R4: Judge transport should parse `pi --mode json` message events instead of raw `-p` terminal output. Done means: code captures final assistant text and provider stop errors distinctly. VERIFY: `npm run typecheck` and tests pass.
- R5: The judge should behave like oracle where it matters: explicit model, live streamed progress, and a timeout large enough for a cold reasoning turn. Done means: unset `/goals judge` resolves to the current session model when visible; if no model is visible, no implicit Pi default is used and sign-off is `judge inconclusive`. `message_update` emits throttled progress, and timeout is 600s. VERIFY: `npm run typecheck` and fresh-eyes diff review.
## Tasks
- [x] T1 (R3): Add an accepted-with-warning sign-off outcome.
- verify: `npm test`
- success: test shows status `[x]` plus `judge inconclusive` in `## Log`
- likely_fail: timeout still records `reject`
- sneaky_fail: accepted status lands but log hides judge failure
- UAT: [test/plan-file.test.ts](/home/wassname/.pi/agent/git/github.com/wassname/pi-plan/test/plan-file.test.ts)
- [x] T2 (R4): Switch judge subprocess to JSON-mode parsing.
- verify: `npm run typecheck`
- success: no TypeScript errors, judge code has no ANSI-terminal parsing dependency
- likely_fail: compile errors around streamed event shape
- sneaky_fail: model error produces empty output and gets parsed as reject instead of transport failure
- UAT: [src/index.ts](/home/wassname/.pi/agent/git/github.com/wassname/pi-plan/src/index.ts)
- [x] T3 (docs): Update README sign-off semantics.
- verify: `rg "inconclusive|timeout|judge accept" README.md src test`
- success: docs name fail-forward behavior
- likely_fail: README still says all rejects keep goal open
- sneaky_fail: docs imply subagent evidence was accepted when it timed out
- UAT: [README.md](/home/wassname/.pi/agent/git/github.com/wassname/pi-plan/README.md)
- [x] T4 (R5): Copy oracle's reliability shape for model/progress.
- verify: `npm run typecheck`
- success: `CompleteGoal` passes the current session model to the judge when no override is set, never spawns without `--model`, and streamed judge deltas are surfaced through `onUpdate`
- likely_fail: judge still runs without `--model`
- sneaky_fail: user sees no progress for several minutes and kills a working judge
- UAT: [src/index.ts](/home/wassname/.pi/agent/git/github.com/wassname/pi-plan/src/index.ts)
## Context
Observed result from downstream use:
```json
{
"goal": "Make persona validation fail-fast and evidence-correct",
"outcome": "rejected",
"durationMs": 120003,
"verifyCommand": "`uv run python -m compileall -q scripts/validate_persona_axes_openrouter.py`",
"reasoning": "VERDICT: reject\nmissing: judge timed out after 120s",
"isError": true
}
```
Interpretation: latest surfaced output proves the internal judge timed out. It does not prove the verify command passed, though earlier logs indicated that pattern.
## Log
- 2026-06-29 current `runJudge` uses raw `pi -p --no-session` output plus ANSI stripping; oracle uses `--mode json` and parses message events, which is likely more reliable.
- 2026-06-29 unset `/goals judge` spawns the judge without `--model`, so Pi resolves its configured default model; do not describe this as the current session model.
- 2026-06-29 timeout/transport failure now maps to `accepted_inconclusive`, preserving partial output in reasoning when available.
- 2026-06-29 fresh-eyes review found loose `/accept/i` verdict parsing and caller-abort fail-forward risk; fixed exact verdict parsing and made caller abort reject.
- 2026-06-29 oracle comparison suggests the important reliability pieces are explicit model selection, JSON streaming, live partial output, and no short wrapper timeout; updated CompleteGoal to use the current session model when visible, never spawn without `--model`, stream throttled progress, and wait 600s.
## TODO
- Consider making `CompleteGoal` expose `verifyExitCode: 0` and `judgeOutcome` separately in details.
## Errors
| Task | Error | Resolution |
|------|-------|------------|
+160 -28
View File
@@ -59,6 +59,7 @@ const WIDGET_KEY = "pi-goals-widget";
// Names match pi's internal tool registry (grep→ffgrep, find→fffind, etc.).
const JUDGE_TOOLS = ["read", "bash", "grep", "find", "ls"];
const JUDGE_BLOCKED_TOOLS = ["edit", "write"];
const JUDGE_TIMEOUT_MS = 600_000;
// File mutators blocked while drafting goals (read-only plan mode, like narumiruna/pi-plan-mode), so
// code isn't written before goals are agreed. The one allowed write is goals.md itself (the
// deliverable). A read-only task (a pure search) can still be explored in plan mode by nature.
@@ -86,7 +87,7 @@ const PLAN_REL = ".pi/goals.md"; // project-local, gitignored (pi-tasks conventi
interface PlanState {
isPlanMode: boolean;
objective: string | null;
/** Optional model ref for the sign-off judge; unset => the subprocess uses pi's default model. */
/** Optional model ref for the sign-off judge; unset => use the current session model. */
judgeModel: string | null;
}
@@ -184,7 +185,7 @@ export default function piGoalsExtension(pi: ExtensionAPI): void {
function setJudge(ref: string, ctx: ExtensionContext): void {
state = { ...state, judgeModel: ref || null };
persist();
ctx.ui.notify(ref ? `Sign-off judge model set to ${ref}` : "Sign-off judge reset to the default model", "info");
ctx.ui.notify(ref ? `Sign-off judge model set to ${ref}` : "Sign-off judge reset to the current session model", "info");
}
async function clearPlan(ctx: ExtensionContext): Promise<void> {
@@ -314,19 +315,24 @@ export default function piGoalsExtension(pi: ExtensionAPI): void {
onUpdate?.(partial);
};
const { outcome, reasoning, durationMs } = await decideSignOff(goal, goal.evidence.join("\n"), goal.evidence, state.judgeModel, ctx.cwd, signal, handleUpdate);
const judgeModel = state.judgeModel ?? currentSessionModelRef(ctx);
const { outcome, reasoning, durationMs } = await decideSignOff(goal, goal.evidence.join("\n"), goal.evidence, judgeModel, ctx.cwd, signal, handleUpdate);
const res = recordSignOff(content, goal.subject, stamp(), outcome);
if (res.content !== content) writePlan(ctx, res.content);
updateWidget(ctx);
const detail = reasoning ? `\n\n--- sign-off judge ---\n${reasoning}` : "";
const outcomeLabel = outcome.kind === "accepted" ? "accepted" : outcome.kind === "verify_failed" ? "verify_failed" : "rejected";
const outcomeLabel =
outcome.kind === "accepted" ? "accepted" :
outcome.kind === "accepted_inconclusive" ? "accepted_inconclusive" :
outcome.kind === "verify_failed" ? "verify_failed" :
"rejected";
const details: SignOffDetails = {
goal: goal.subject,
outcome: outcomeLabel,
durationMs,
verifyCommand: goal.verify ?? undefined,
verifyExitCode: outcome.kind === "verify_failed" ? outcome.exitCode : undefined,
judgeModel: state.judgeModel ?? undefined,
judgeModel: judgeModel ?? "no explicit judge model",
reasoning,
isError: res.isError,
};
@@ -346,8 +352,13 @@ export default function piGoalsExtension(pi: ExtensionAPI): void {
const body = result.content[0]?.type === "text" ? result.content[0].text : "(no output)";
if (!details || details.outcome === "running") return new Text(body, 0, 0);
const icon = details.outcome === "accepted" ? theme.fg("success", "✔") : theme.fg("error", "✗");
const outcomeText = details.outcome === "accepted" ? "accepted" : details.outcome === "verify_failed" ? `verify failed (exit ${details.verifyExitCode})` : "rejected";
const accepted = details.outcome === "accepted" || details.outcome === "accepted_inconclusive";
const icon = accepted ? theme.fg("success", "✔") : theme.fg("error", "✗");
const outcomeText =
details.outcome === "accepted" ? "accepted" :
details.outcome === "accepted_inconclusive" ? "accepted (judge inconclusive)" :
details.outcome === "verify_failed" ? `verify failed (exit ${details.verifyExitCode})` :
"rejected";
const header = `${icon} ${theme.fg("toolTitle", theme.bold("goal signoff "))}${theme.fg("accent", outcomeText)}`;
const duration = details.durationMs < 1000 ? `${details.durationMs}ms` : `${(details.durationMs / 1000).toFixed(1)}s`;
const sub = [details.judgeModel, duration].filter(Boolean).join(" · ");
@@ -461,7 +472,7 @@ export default function piGoalsExtension(pi: ExtensionAPI): void {
/** Structured details returned by CompleteGoal so renderCall/renderResult can show metadata. */
interface SignOffDetails {
goal: string;
outcome: "accepted" | "rejected" | "verify_failed" | "running";
outcome: "accepted" | "accepted_inconclusive" | "rejected" | "verify_failed" | "running";
phase?: string; // "verifying" | "spawning" | "judging" — while running
durationMs: number;
verifyCommand?: string;
@@ -483,6 +494,10 @@ function stamp(): string {
return new Date().toISOString().slice(0, 16).replace("T", " ");
}
function currentSessionModelRef(ctx: ExtensionContext): string | null {
return ctx.model ? `${ctx.model.provider}/${ctx.model.id}` : null;
}
/** Decide a sign-off: deterministic verify first (cheap; skip the model call if it fails), then the judge.
* Returns the outcome plus the judge's (or verify's) reasoning so CompleteGoal can show WHY. */
async function decideSignOff(
@@ -513,8 +528,21 @@ async function decideSignOff(
};
}
}
if (!judgeModel) {
const reason = "no explicit judge model available; set /goals judge <provider/model>";
return {
outcome: { kind: "accepted_inconclusive", reason },
reasoning: `VERDICT: inconclusive\nreason: ${reason}`,
durationMs: Date.now() - startedAt,
};
}
const verdict = await runJudge(goal, evidence, paths, verifyResult, judgeModel, cwd, signal, onUpdate);
const outcome: SignOff = verdict.accept ? { kind: "accepted" } : { kind: "rejected", missing: verdict.missing };
const outcome: SignOff =
verdict.kind === "accepted"
? { kind: "accepted" }
: verdict.kind === "inconclusive"
? { kind: "accepted_inconclusive", reason: verdict.reason }
: { kind: "rejected", missing: verdict.missing };
return { outcome, reasoning: verdict.reasoning, durationMs: verdict.durationMs };
}
@@ -534,17 +562,34 @@ function getPiInvocation(args: string[]): { command: string; args: string[] } {
return { command: "pi", args };
}
function extractTextFromContent(content: unknown): string {
if (!Array.isArray(content)) return "";
const parts: string[] = [];
for (const part of content) {
if (part && typeof part === "object" && (part as { type?: string }).type === "text") {
const text = (part as { text?: string }).text;
if (typeof text === "string" && text.trim()) parts.push(text);
}
}
return parts.join("\n\n").trim();
}
type JudgeResult =
| { kind: "accepted"; reasoning: string; durationMs: number }
| { kind: "rejected"; missing: string; reasoning: string; durationMs: number }
| { kind: "inconclusive"; reason: string; reasoning: string; durationMs: number };
/** Stage 2: a read-only pi subprocess inspects the evidence against the repo and returns a verdict. */
async function runJudge(
goal: Goal,
evidence: string,
paths: string[],
verifyResult: { command: string; exitCode: number; outputTail: string } | null,
judgeModel: string | null,
judgeModel: string,
cwd: string,
signal: AbortSignal | undefined,
onUpdate?: (partial: { content: Array<{ type: "text"; text: string }>; details: SignOffDetails }) => void,
): Promise<{ accept: boolean; missing: string; reasoning: string; durationMs: number }> {
): Promise<JudgeResult> {
const startedAt = Date.now();
const emit = (phase: string, text: string) => {
onUpdate?.({
@@ -561,8 +606,7 @@ async function runJudge(
evidence,
paths,
});
const args = ["-p", "--no-session", "--tools", JUDGE_TOOLS.join(","), "--exclude-tools", JUDGE_BLOCKED_TOOLS.join(","), "--append-system-prompt", evidenceJudgeSystem];
if (judgeModel) args.push("--model", judgeModel);
const args = ["--mode", "json", "-p", "--no-session", "--model", judgeModel, "--tools", JUDGE_TOOLS.join(","), "--exclude-tools", JUDGE_BLOCKED_TOOLS.join(","), "--append-system-prompt", evidenceJudgeSystem];
args.push(task);
emit("spawning", `Spawning read-only judge for: ${goal.subject}`);
@@ -570,46 +614,134 @@ async function runJudge(
// FIXME(side-effect): pi -p --no-session clones the repo into the PARENT of cwd (so alongside
// the working dir), leaving a stale directory. The judge should run in a temp dir or inside the
// existing repo checkout so it doesn't pollute the user's workspace.
const JUDGE_TIMEOUT_MS = 120_000;
const output = await new Promise<string>((resolve) => {
const judge = await new Promise<{ output: string; error?: string; aborted?: boolean }>((resolve) => {
let settled = false;
let stdoutBuffer = "";
let finalOutput = "";
let currentText = "";
let stderr = "";
let lastStopReason: string | undefined;
let lastErrorMessage: string | undefined;
let lastProgressAt = 0;
const partialOutput = (): string => [finalOutput, currentText, stderr.trim()].filter(Boolean).join("\n\n").trim();
const emitProgress = (text: string, force = false) => {
const now = Date.now();
if (!force && now - lastProgressAt < 750) return;
lastProgressAt = now;
emit("judging", text);
};
const proc = spawn(inv.command, inv.args, { cwd, shell: false, stdio: ["ignore", "pipe", "pipe"], signal });
if (!proc.stdout || !proc.stderr) throw new Error("judge subprocess stdio must be piped");
const timer = setTimeout(() => {
if (!settled) {
settled = true;
proc.kill();
resolve(`VERDICT: reject\nmissing: judge timed out after ${JUDGE_TIMEOUT_MS / 1000}s`);
resolve({ output: partialOutput(), error: `judge timed out after ${JUDGE_TIMEOUT_MS / 1000}s` });
}
}, JUDGE_TIMEOUT_MS);
const proc = spawn(inv.command, inv.args, { cwd, shell: false, stdio: ["ignore", "pipe", "pipe"], signal });
let out = "";
proc.stdout.on("data", (d) => (out += d));
proc.stderr.on("data", (d) => (out += d));
proc.on("close", () => {
const processLine = (line: string) => {
if (!line.trim()) return;
let event: any;
try {
event = JSON.parse(line);
} catch {
return;
}
if (event.type === "message_start" && event.message?.role === "assistant") {
currentText = "";
emitProgress("Judge is reading evidence...", true);
return;
}
if (event.type === "message_update" && event.assistantMessageEvent?.type === "text_delta") {
currentText += event.assistantMessageEvent.delta ?? "";
emitProgress(currentText || "Judge is reading evidence...");
return;
}
if (event.type === "message_end" && event.message?.role === "assistant") {
const text = extractTextFromContent(event.message.content) || currentText;
if (text) finalOutput = text;
currentText = "";
lastStopReason = typeof event.message.stopReason === "string" ? event.message.stopReason : undefined;
lastErrorMessage = typeof event.message.errorMessage === "string" ? event.message.errorMessage : undefined;
emitProgress(finalOutput || "Judge finished without text.", true);
}
};
proc.stdout.on("data", (data) => {
stdoutBuffer += data.toString();
const lines = stdoutBuffer.split("\n");
stdoutBuffer = lines.pop() || "";
for (const line of lines) processLine(line);
});
proc.stderr.on("data", (data) => {
stderr += data.toString();
emitProgress(partialOutput() || "Judge subprocess wrote stderr.");
});
proc.on("close", (code) => {
if (!settled) {
settled = true;
clearTimeout(timer);
resolve(out);
if (stdoutBuffer.trim()) processLine(stdoutBuffer);
if (lastStopReason === "error" || lastStopReason === "aborted") {
resolve({ output: finalOutput, error: lastErrorMessage || `judge model ${lastStopReason}`, aborted: signal?.aborted });
return;
}
if ((code ?? 0) !== 0) {
resolve({ output: finalOutput, error: stderr.trim() || finalOutput || `judge subprocess exited ${code ?? 1}` });
return;
}
resolve({ output: finalOutput });
}
});
proc.on("error", (e) => {
if (!settled) {
settled = true;
clearTimeout(timer);
resolve(`VERDICT: reject\nmissing: judge subprocess failed: ${e.message}`);
resolve({ output: "", error: `judge subprocess failed: ${e.message}`, aborted: signal?.aborted });
}
});
});
// The subprocess emits ANSI/CSI control codes in -p mode; strip them so they don't leak into `missing`.
const clean = output.replace(/\u001b\[[0-9;?]*[ -/]*[@-~]/g, "");
if (judge.error) {
const partial = judge.output.trim();
if (judge.aborted) {
return {
kind: "rejected",
missing: judge.error,
reasoning: `VERDICT: reject\nmissing: ${judge.error}`,
durationMs: Date.now() - startedAt,
};
}
return {
kind: "inconclusive",
reason: judge.error,
reasoning: partial
? `VERDICT: inconclusive\nreason: ${judge.error}\n\npartial judge output:\n${partial}`
: `VERDICT: inconclusive\nreason: ${judge.error}`,
durationMs: Date.now() - startedAt,
};
}
const clean = judge.output.trim();
const verdictLine = clean.split("\n").find((l) => /^\s*VERDICT\s*:/i.test(l)) ?? "";
const accept = /accept/i.test(verdictLine);
const verdictMatch = /^\s*VERDICT\s*:\s*(accept|reject)\s*$/i.exec(verdictLine);
if (!verdictMatch) {
return {
kind: "inconclusive",
reason: clean ? "judge returned no exact VERDICT line" : "judge finished without returning any text",
reasoning: clean || "VERDICT: inconclusive\nreason: judge finished without returning any text",
durationMs: Date.now() - startedAt,
};
}
const missingMatch = clean.match(/missing\s*:\s*([\s\S]*)$/i);
const missing = accept ? "" : (missingMatch?.[1].trim() || clean.trim().slice(-500) || "judge gave no reason");
// The judge's own words (inspection + verdict), so CompleteGoal can show them. The verdict is at the
// end, so keep the tail when it's long.
const trimmed = clean.trim();
const reasoning = trimmed.length > 1800 ? `...\n${trimmed.slice(-1800)}` : trimmed;
return { accept, missing, reasoning, durationMs: Date.now() - startedAt };
if (verdictMatch[1].toLowerCase() === "accept") return { kind: "accepted", reasoning, durationMs: Date.now() - startedAt };
const missing = missingMatch?.[1].trim() || clean.slice(-500) || "judge gave no reason";
return { kind: "rejected", missing, reasoning, durationMs: Date.now() - startedAt };
}
+11 -1
View File
@@ -272,7 +272,8 @@ export function pruneCompleted(text: string): string {
export type SignOff =
| { kind: "verify_failed"; exitCode: number; outputTail: string }
| { kind: "rejected"; missing: string }
| { kind: "accepted" };
| { kind: "accepted" }
| { kind: "accepted_inconclusive"; reason: string };
/** Apply a sign-off outcome to goals.md text: accept flips the goal checkbox to [x] + logs; reject only logs. Pure. */
export function recordSignOff(
@@ -294,6 +295,15 @@ export function recordSignOff(
return { content, message: `Sign-off rejected. Missing:\n${outcome.missing}`, isError: true };
}
const flipped = setGoalStatus(text, subject, "done");
if (outcome.kind === "accepted_inconclusive") {
const oneLine = outcome.reason.replace(/\s+/g, " ").trim().slice(0, 200);
const content = appendLog(flipped, `${when} signed off "${subject}" (judge inconclusive: ${oneLine})`);
return {
content,
message: `Signed off "${subject}". Marked done in goals.md.\nJudge inconclusive: ${outcome.reason}`,
isError: false,
};
}
const content = appendLog(flipped, `${when} signed off "${subject}" (judge accept)`);
return { content, message: `Signed off "${subject}". Marked done in goals.md.`, isError: false };
}
+4 -2
View File
@@ -211,8 +211,10 @@ export const completeGoalDescription =
"was avoided; ruling out the failure modes is necessary but not sufficient. Then call this with the " +
"goal's desc (the text after 'goal:'). Runs the goal's verify command (if any) then a read-only " +
"subagent that inspects that evidence against the repo and the discriminator. On accept, the goal is " +
"marked done and logged; on reject, it stays open and you get what is missing. The subagent's " +
"reasoning is returned either way.";
"marked done and logged; on judge reject, it stays open and you get what is missing. If the " +
"subagent times out or its transport/model fails after any verify command has passed, the goal is " +
"marked done with a judge-inconclusive log line. The subagent's reasoning or partial output is " +
"returned either way.";
export const completeGoalParamDescription = "The goal's desc: the exact text after 'goal:' in its line.";
+12
View File
@@ -148,6 +148,18 @@ describe("recordSignOff (CompleteGoal's pure record logic)", () => {
expect(doc.log.at(-1)).toBe(`- ${WHEN} signed off "Implement cache layer" (judge accept)`);
});
it("accepted_inconclusive still marks done and logs the judge failure", () => {
const r = recordSignOff(SAMPLE, "Implement cache layer", WHEN, {
kind: "accepted_inconclusive",
reason: "judge timed out after 120s",
});
expect(r.isError).toBe(false);
const doc = parse(r.content);
expect(findGoal(doc, "Implement cache layer")?.status).toBe("done");
expect(doc.log.at(-1)).toBe(`- ${WHEN} signed off "Implement cache layer" (judge inconclusive: judge timed out after 120s)`);
expect(r.message).toContain("Judge inconclusive: judge timed out after 120s");
});
it("verify_failed only logs a reject line, status stays active", () => {
const r = recordSignOff(SAMPLE, "Implement cache layer", WHEN, { kind: "verify_failed", exitCode: 1, outputTail: "boom" });
expect(r.isError).toBe(true);