From 548177e3f4ce0d026413a394c4ee3dcab517dd41 Mon Sep 17 00:00:00 2001 From: wassname <1103714+wassname@users.noreply.github.com> Date: Sat, 12 Sep 2026 22:22:37 +0800 Subject: [PATCH] Require review before final goal completion --- README.md | 2 +- src/index.ts | 44 ++++++++++++++++++++++++++++--- src/prompts.ts | 9 ++++++- test/goals.test.ts | 64 ++++++++++++++++++++++++++++++++++++++++------ 4 files changed, 105 insertions(+), 14 deletions(-) diff --git a/README.md b/README.md index bf47812..568d8a1 100644 --- a/README.md +++ b/README.md @@ -153,7 +153,7 @@ pi ## Context delivery -Startup, session restore and successful compaction inject the complete current plan document at the next ordinary prompt. Other ordinary plan-context messages inject the title, introductory paragraph and `## User-visible result`; plan-change and manual-review messages carry that short view directly rather than only a path. After eight unchanged turns, the next ordinary prompt carries a medium view: the short view, verbatim `## User voice`, and goal headings with their checkbox status. It omits task and evidence details. A Log heading at any Markdown heading level starts history for these short and medium views. The complete refresh retains the entire document, including Log history. Compaction uses Pi's configured threshold; this plugin does not set a separate 150k limit. Supervisor upkeep cycles through curated nudges, advancing only when delivered; the editable hourly `schedule_prompt` check-in is unchanged. A full plan refresh replaces pending upkeep; edits, pause, exit and session navigation invalidate obsolete reminders. Failed or cancelled compaction does not schedule another refresh or consume pending upkeep. Missing plans are retried without discarding progress. +Startup, session restore and successful compaction inject the complete current plan document at the next ordinary prompt. Other ordinary plan-context messages inject the title, introductory paragraph and `## User-visible result`; plan-change and manual-review messages carry that short view directly rather than only a path. After eight unchanged turns, the next ordinary prompt carries a medium view: the short view, verbatim `## User voice`, and goal headings with their checkbox status. It omits task and evidence details. A Log heading at any Markdown heading level starts history for these short and medium views. The complete refresh retains the entire document, including Log history. The first request to complete the final non-cancelled goal queues a full-plan review without recording sign-off; only a second completion request in that review turn can record it. A plan edit invalidates the queued review. Compaction uses Pi's configured threshold; this plugin does not set a separate 150k limit. Supervisor upkeep cycles through curated nudges, advancing only when delivered; the editable hourly `schedule_prompt` check-in is unchanged. A full plan refresh replaces pending upkeep; edits, pause, exit and session navigation invalidate obsolete reminders. Failed or cancelled compaction does not schedule another refresh or consume pending upkeep. Missing plans are retried without discarding progress. This is deliberately passive on Pi 0.85.1: tool-loop continuations, overflow retries and already-queued user messages keep Pi's existing role and compacted context, without an extra model turn just to repeat the plan. They do not receive a newly read plan until ordinary prompt preparation. Pi's `triggerTurn: false` mid-run path can save a message absent from the live request snapshot; steering can instead force an unwanted turn. We use neither path for upkeep. Passive pause notices use `nextTurn`, with immediate UI feedback; stopping remains local and remote termination is unconfirmed. Quit sends no model message. diff --git a/src/index.ts b/src/index.ts index f375d62..cd5461e 100644 --- a/src/index.ts +++ b/src/index.ts @@ -18,6 +18,9 @@ import { discuss, emptyEvidence, evidenceUnavailable, + finalReview, + finalReviewInvalidated, + finalReviewQueued, goalToolBlocked, manualReview, messages, @@ -52,6 +55,7 @@ interface State { workerStopped?: boolean; pausedFrom?: "solo" | "supervising"; signoffs: Record; + finalReview?: { planDigest: string }; child?: boolean; } const initial = (): State => ({ mode: "chat", helpers: [], signoffs: {} }); @@ -71,6 +75,7 @@ export default function mainSupervisor(pi: ExtensionAPI) { let state = initial(); let generation = 0; let workerRevision = 0; + let finalReviewTurnDigest: string | undefined; const pendingLaunches = new Map(); let notice = true; let fullPlanContextDue = true; @@ -93,6 +98,12 @@ export default function mainSupervisor(pi: ExtensionAPI) { if (snapshot.text === undefined) throw new Error(snapshot.error); return snapshot.text; }; + const clearChangedFinalReview = (text: string) => { + if (!state.finalReview || state.finalReview.planDigest === digest(text)) return false; + state.finalReview = undefined; + save(); + return true; + }; let turnsStale = 0; let upkeepRound = 0; let lastWorkingSet = ""; @@ -151,6 +162,7 @@ export default function mainSupervisor(pi: ExtensionAPI) { if (stamp !== generation || state.mode !== "supervising") return; const snapshot = readPlan(); if (snapshot.text === undefined) { ctx.ui.notify(snapshot.error!, "warning"); return; } + clearChangedFinalReview(snapshot.text); refresh(ctx); const hash = digest(planViews(snapshot.text).notify); if (hash === planHash) return; @@ -180,6 +192,7 @@ export default function mainSupervisor(pi: ExtensionAPI) { upkeepRound = 0; lastWorkingSet = ""; pendingUpkeep = undefined; + finalReviewTurnDigest = undefined; fullPlanContextDue = true; refresh(ctx); watchPlan(ctx); @@ -238,11 +251,12 @@ export default function mainSupervisor(pi: ExtensionAPI) { pi.on("session_start", (_e, ctx) => restore(ctx)); pi.on("session_tree", (_e, ctx) => restore(ctx)); - pi.on("session_shutdown", () => { generation++; planWatcher?.close(); planWatcher = undefined; clearTimeout(planEditTimer); planEditTimer = undefined; }); + pi.on("session_shutdown", () => { generation++; finalReviewTurnDigest = undefined; planWatcher?.close(); planWatcher = undefined; clearTimeout(planEditTimer); planEditTimer = undefined; }); // Only successful compaction needs resync; failed/cancelled attempts leave pending context alone. // Defer to prompt preparation: same-run continuation retains Pi's current role/context. pi.on("session_compact", () => { notice = true; fullPlanContextDue = true; }); pi.on("turn_end", (_event, ctx) => { + finalReviewTurnDigest = undefined; if (!["supervising", "solo"].includes(state.mode)) return; const snapshot = readPlan(); if (snapshot.text === undefined) { notice = true; return; } @@ -279,15 +293,20 @@ export default function mainSupervisor(pi: ExtensionAPI) { notice = true; // Retry resync on the next turn; do not consume a failed snapshot. return { systemPrompt: `${event.systemPrompt}\n\n${state.child ? childPlanRole : ""}\n${snapshot.error}` }; } + clearChangedFinalReview(snapshot.text); const role = state.child ? childPlanRole : state.mode === "supervising" ? supervisor(WORKER, state.plan!, ctx.sessionManager.getSessionId()) : state.mode === "planning" ? planning(state.plan!) : state.mode === "paused" ? pausedRole : soloRole; + const pendingFinalReview = state.finalReview; + if (pendingFinalReview) finalReviewTurnDigest = pendingFinalReview.planDigest; // Returned messages enter both Pi's prompt snapshot and saved history together. // Unlike nextTurn, retaining intent here lets a fresh plan resync supersede upkeep, // and drops obsolete reminders after edits, takeover, pause or session navigation. - const message = notice - ? { customType: "pi-goals-plan", content: planContext(state.child ? "worker" : state.mode, state.plan, snapshot.text, fullPlanContextDue ? "full" : "short"), display: false } - : pendingUpkeep?.generation === generation && pendingUpkeep.workingSet === foldPlan(snapshot.text) + const message = pendingFinalReview + ? { customType: "pi-goals-final-review", content: finalReview(state.plan!, snapshot.text), display: false } + : notice + ? { customType: "pi-goals-plan", content: planContext(state.child ? "worker" : state.mode, state.plan, snapshot.text, fullPlanContextDue ? "full" : "short"), display: false } + : pendingUpkeep?.generation === generation && pendingUpkeep.workingSet === foldPlan(snapshot.text) && ["supervising", "solo"].includes(state.mode) && goals(snapshot.text).some(g => g.status === "open" || g.status === "active") ? { customType: "pi-goals-upkeep", content: upkeep(state.plan!, snapshot.text, state.mode === "supervising" ? upkeepRound : undefined), display: false } : undefined; if (message?.customType === "pi-goals-upkeep" && state.mode === "supervising") upkeepRound++; @@ -514,6 +533,21 @@ export default function mainSupervisor(pi: ExtensionAPI) { const evidence = params.evidence.map((file) => isAbsolute(file) ? file : resolve(ctx.cwd, file)); try { for (const file of evidence) if (!readFileSync(file).length) throw new Error(emptyEvidence(file)); } catch (error) { return result(evidenceUnavailable(error)); } + const subject = key(matches[0].subject); + const othersAccepted = goals(text).every((goal) => goal.status === "cancelled" || key(goal.subject) === subject || (goal.status === "done" && state.signoffs[key(goal.subject)])); + if (othersAccepted && !(matches[0].status === "done" && state.signoffs[subject])) { + if (clearChangedFinalReview(text)) return result(finalReviewInvalidated); + if (finalReviewTurnDigest !== digest(text)) { + if (!state.finalReview) { + state.finalReview = { planDigest: digest(text) }; + notice = true; + fullPlanContextDue = true; + save(); + send(finalReview(path, text)); + } + return result(finalReviewQueued(matches[0].subject)); + } + } const lines = text.split("\n"); lines[matches[0].index] = lines[matches[0].index].replace(/\[[ xX/-]\]/, "[x]"); let log = lines.findIndex(line => FOLD_LINE.test(line)); @@ -521,6 +555,8 @@ export default function mainSupervisor(pi: ExtensionAPI) { lines.splice(log + 1, 0, "", completionLog(params.goal, params.observation, evidence, state.mode === "solo")); writeFileSync(path, `${lines.join("\n").trimEnd()}\n`); state.signoffs[key(matches[0].subject)] = { evidence, observation: params.observation, signature: goalAcceptanceSignature(text, matches[0].subject)! }; + state.finalReview = undefined; + finalReviewTurnDigest = undefined; planHash = digest(planViews(planText()).notify); save(); refresh(ctx); const remaining = goals(planText()).some((goal) => goal.status !== "cancelled" && (goal.status !== "done" || !state.signoffs[key(goal.subject)])); diff --git a/src/prompts.ts b/src/prompts.ts index fc58ba5..c731a4a 100644 --- a/src/prompts.ts +++ b/src/prompts.ts @@ -193,6 +193,9 @@ export function planChangedReview(planPath: string, text: string): string { export function manualReview(planPath: string, text: string): string { return `${supervisorJob}\nReview the current plan, worker progress and actual evidence.\n\n${planContextView(text, "short")}\n\nPlan file (audit or edit link): ${planPath}. Do not launch a duplicate writer.`; } +export function finalReview(planPath: string, text: string): string { + return `Final completion review. The preceding CompleteGoal request did not record approval. Read the complete embedded plan, including goal requirements, evidence and Log. Inspect the cited artifacts yourself. Only after this review, call CompleteGoal again with the exact remaining goal and evidence; if the plan changed, inspect the changed plan instead.\n\n${planContextView(text, "full")}\n\nPlan file (audit or edit link): ${planPath}.`; +} // Check-ins. The installed scheduler owns storage/timing/UI. Removal guidance must never add jobs. export function removeGoalSchedule(sessionId: string): string { @@ -203,7 +206,7 @@ export function scheduleCheckIn(sessionId: string, planPath: string): string { } // Completion and runtime errors. Tool returns are model-facing too. -export const completeGoalDescription = "Parent supervisor or solo self-verification only. Inspect the actual artifact and saved verification first; cite nonempty evidence files and describe what you observed. Exact goal subject required. Manual ticks and worker reports are claims; ignored/uncommitted evidence is allowed. This records judgment, not an independent judge."; +export const completeGoalDescription = "Parent supervisor or solo self-verification only. Inspect the actual artifact and saved verification first; cite nonempty evidence files and describe what you observed. Exact goal subject required. The final remaining goal first queues a full-plan review; call CompleteGoal again from that review to record it. Manual ticks and worker reports are claims; ignored/uncommitted evidence is allowed. This records judgment, not an independent judge."; export const messages = { noPlan: "no plan attached", emptyPlan: "empty plan (save may be in progress)", @@ -221,6 +224,10 @@ export const childPlanAttached = (path: string) => `Attached worker plan ${path} export function completionLog(goal: string, observation: string, evidence: string[], solo: boolean): string { return `- ${solo ? "Solo self-verification" : "Parent review"}: ${JSON.stringify(goal)}; ${JSON.stringify(observation)}; evidence ${JSON.stringify(evidence)}`; } +export function finalReviewQueued(goal: string): string { + return `Final review queued for ${goal}; no sign-off recorded. Read the complete embedded plan and actual evidence in that review turn, then call CompleteGoal again with the exact goal and evidence.`; +} +export const finalReviewInvalidated = "The plan changed since the final review was queued; no sign-off recorded. Inspect the current plan and request completion again to queue a new final review."; export function completionResult(goal: string, sessionId: string, remaining: boolean, solo: boolean): string { return `Recorded ${solo ? "solo self-verification" : "parent judgment"} for ${goal}; not independent verification. ${remaining ? "Continue only remaining open or unsigned goals in your current role." : `All non-cancelled goals are reviewed. ${removeGoalSchedule(sessionId)}`}`; } diff --git a/test/goals.test.ts b/test/goals.test.ts index 4729883..c9f1f2a 100644 --- a/test/goals.test.ts +++ b/test/goals.test.ts @@ -243,7 +243,12 @@ it.each(["FIRST OUTPUT", "renamed output", "duplicate", "historical"])("completi const history = "## Log\n- [ ] goal: first output\n"; writeFileSync(f.path, "- [ ] goal: first output\n - [ ] unrelated task\n" + suffix + history); const before = readFileSync(f.path, "utf8"); - await f.tools.get("CompleteGoal").execute("c", { goal: subject === "duplicate" || subject === "historical" ? "first output" : subject, evidence: [evidence], observation: "Read actual output" }, undefined, undefined, f.ctx); + const params = { goal: subject === "duplicate" || subject === "historical" ? "first output" : subject, evidence: [evidence], observation: "Read actual output" }; + const first = await f.tools.get("CompleteGoal").execute("c", params, undefined, undefined, f.ctx); + if (first.content[0].text.includes("Final review queued")) { + f.hooks.get("before_agent_start")({ systemPrompt: "base" }, f.ctx); + await f.tools.get("CompleteGoal").execute("c", params, undefined, undefined, f.ctx); + } const after = readFileSync(f.path, "utf8"); if (subject === "renamed output" || subject === "duplicate") expect(after).toBe(before); else { expect(after).toContain("- [x] goal: first output"); expect(after.split("## Log")[1]).toContain("\n- [ ] goal: first output\n"); expect(after).toContain("- [ ] unrelated task"); } @@ -359,19 +364,58 @@ it("gives pause scheduler guidance but clears on exit without a model prompt", a expect(f.entries.at(-1).data).toEqual({ mode: "chat", helpers: [], signoffs: {} }); }); -it("tells the model to remove only its own job after the final review", async () => { - const f = fixture(); await f.draft(); await f.command("ready"); +it("requires a full-plan review turn before recording the final goal", async () => { + const f = fixture(); await f.draft(); + const plan = `# Final review fixture +- [ ] goal: first output + - discriminator: first output has exact saved bytes +- [ ] goal: second output + - discriminator: second output has exact saved bytes + +## Log +- worker evidence: keep this history in the final review`; + writeFileSync(f.path, plan); await f.command("ready"); mkdirSync(join(f.ctx.cwd, "evidence")); writeFileSync(join(f.ctx.cwd, "evidence/pass.log"), "bytes\n"); - let finalText = ""; - for (const goal of ["first output", "second output"]) { - finalText = (await f.tools.get("CompleteGoal").execute("t", { goal, evidence: ["evidence/pass.log"], observation: "inspected" }, undefined, undefined, f.ctx)).content[0].text; - } + const complete = (goal: string) => f.tools.get("CompleteGoal").execute("t", { goal, evidence: ["evidence/pass.log"], observation: "inspected" }, undefined, undefined, f.ctx); + await complete("first output"); + const queued = await complete("second output"); + expect(queued.content[0].text).toContain("Final review queued"); + expect(readFileSync(f.path, "utf8")).toContain("- [ ] goal: second output"); + const direct = f.messages.at(-1); + expect(direct.savedPrompt).toBe(true); + expect(direct.message.content).toContain("second output has exact saved bytes"); + expect(direct.message.content).toContain("worker evidence: keep this history"); + const review = f.hooks.get("before_agent_start")({ systemPrompt: "base" }, f.ctx).message; + expect(review).toMatchObject({ customType: "pi-goals-final-review" }); + expect(review.content).toContain("first output has exact saved bytes"); + expect(review.content).toContain("worker evidence: keep this history"); + const finalText = (await complete("second output")).content[0].text; expect(finalText).toContain("All non-cancelled goals are reviewed."); expect(finalText).toContain('job named "goals-copy-only"'); expect(finalText).toContain("leave other jobs untouched"); f.shutdown(); }); +it("recovers a queued final review and invalidates it when the plan changes", async () => { + const f = fixture(); await f.draft(); await f.command("ready"); + writeFileSync(join(f.ctx.cwd, "proof.log"), "PASS\n"); + const complete = (goal: string) => f.tools.get("CompleteGoal").execute("t", { goal, evidence: ["proof.log"], observation: "inspected" }, undefined, undefined, f.ctx); + await complete("first output"); + await complete("second output"); + f.hooks.get("session_start")({}, f.ctx); + const recovered = f.hooks.get("before_agent_start")({ systemPrompt: "base" }, f.ctx).message; + expect(recovered).toMatchObject({ customType: "pi-goals-final-review" }); + expect(recovered.content).toContain("- [ ] goal: second output"); + writeFileSync(f.path, readFileSync(f.path, "utf8").replace("second output", "revised second output")); + const invalidated = await complete("revised second output"); + expect(invalidated.content[0].text).toContain("plan changed since the final review"); + const changed = await complete("revised second output"); + expect(changed.content[0].text).toContain("Final review queued"); + expect(readFileSync(f.path, "utf8")).toContain("- [ ] goal: revised second output"); + expect(f.messages.at(-1).message.content).toContain("revised second output"); + f.shutdown(); +}); + it("restores the complete plan document after session restore", async () => { const f = fixture(); await f.draft(); const plan = `${f.plan.replace("## Log", "## User voice\n- > \"Keep the user voice after restore.\"\n## Log")}old progress`; @@ -585,7 +629,11 @@ it("cancelled goals do not prevent final cleanup, and solo writes self-verificat writeFileSync(f.path, f.plan.replace("[ ] goal: second", "[-] goal: second") + "\n## Appendix\nPreserved context\n"); f.ctx.ui.select.mockResolvedValueOnce("Worker confirmed stopped"); await f.command("solo"); writeFileSync(join(f.ctx.cwd, "proof.log"), "PASS\n"); - const done = await f.tools.get("CompleteGoal").execute("c", { goal: "first output", evidence: ["proof.log"], observation: "Exact bytes observed" }, undefined, undefined, f.ctx); + const params = { goal: "first output", evidence: ["proof.log"], observation: "Exact bytes observed" }; + const queued = await f.tools.get("CompleteGoal").execute("c", params, undefined, undefined, f.ctx); + expect(queued.content[0].text).toContain("Final review queued"); + f.hooks.get("before_agent_start")({ systemPrompt: "base" }, f.ctx); + const done = await f.tools.get("CompleteGoal").execute("c", params, undefined, undefined, f.ctx); expect(done.content[0].text).toContain("All non-cancelled goals are reviewed"); const text = readFileSync(f.path, "utf8"); expect(text).toContain("Solo self-verification:");