mirror of
https://github.com/wassname/pi-plan.git
synced 2026-09-10 12:36:10 +08:00
Version plans and expose judge review
This commit is contained in:
@@ -1,8 +1,8 @@
|
||||
# pi-goals
|
||||
|
||||
Instead of a long plan, make a short list of goal in a one page markdown file. This way it's easy for you to review, and a subagent can just if each goal has been achieved.
|
||||
Make a short list of goals in one Markdown plan file. This is easy to review, and a subagent can check whether each goal is complete.
|
||||
|
||||
The `goals.md` file looks something like this
|
||||
The plan file looks like this:
|
||||
|
||||
```md
|
||||
## <short plan title>
|
||||
@@ -23,6 +23,12 @@ The `goals.md` file looks something like this
|
||||
- evidence: (empty until sign-off)
|
||||
|
||||
### Future work / out of scope
|
||||
|
||||
### Log
|
||||
|
||||
### Learnings
|
||||
|
||||
### Papercuts - problems, gotchas, suggestions
|
||||
```
|
||||
|
||||

|
||||
@@ -55,15 +61,17 @@ pi -e ./src/index.ts
|
||||
|
||||
`/goals` enters plan mode and starts a conversation; the objective is an optional seed. From there:
|
||||
|
||||
1. Plan. The agent explores read-only
|
||||
2. Review. The working set is printed in the transcript, then a menu asks Ready, Ready + compact,
|
||||
open in `$EDITOR`, or keep planning.
|
||||
1. Plan. The agent explores read-only and drafts the plan.
|
||||
2. Review. The full plan is printed in the transcript, then a menu asks Ready, Ready + compact,
|
||||
Grill me, open in `$EDITOR`, or keep planning. Grill me asks one understanding-check question and
|
||||
keeps plan mode active.
|
||||
3. Work. The agent ticks subtasks, appends to `## Log` and `## Learnings`, fills `evidence:`, and
|
||||
calls `CompleteGoal` when a discriminator is satisfied. If it leaves the plan untouched for two
|
||||
turns, the working set is sent back with a short upkeep reminder.
|
||||
|
||||
Other commands: `/goals clear` deletes this session's plan file; `/goals judge <model-ref>` picks a
|
||||
specific model for the sign-off judge (default: your current session model, else pi's default).
|
||||
Other commands: `/goals --clear` deletes this session's active plan file; `/goals --judge <model-ref>`
|
||||
picks a sign-off judge model (default: your current session model, else pi's default). The `--` prefix
|
||||
keeps ordinary objectives such as `judge model quality` from being parsed as commands.
|
||||
|
||||
## Prompts
|
||||
|
||||
|
||||
@@ -0,0 +1,31 @@
|
||||
I only have a file-read tool here, so I’ll start from the repo files I can open and look for the plan-versioning and judge-related changes.Continuing through the judge path and tests.Reading the rest of the judge prompt and the tests that cover these behaviors.Looking for the test files.Trying common test filenames under `src` and `test`.Checking other likely files mentioned in the README.I couldn’t run `git diff` (no shell tool). From the files I could read, these are the concrete defects against the required behaviors:
|
||||
|
||||
**1. Uncommitted conflict markers in `README.md`**
|
||||
The working tree still has `<<<<<<< Updated upstream` / `=======` / `>>>>>>> Stashed changes` (and a truncated upstream judge sentence). That is not a reviewable change set.
|
||||
|
||||
**2. Judge “checked artifacts” gate is too brittle**
|
||||
`decideSignOff` only accepts:
|
||||
|
||||
```ts
|
||||
/^#{0,6}\s*(?:\*\*)?checks(?:\*\*)?:\s*\n(?:[-*]\s+.+\n?)+\s*VERDICT:/im
|
||||
```
|
||||
|
||||
The prompt asks for a `checks:` heading plus bullets, then **exactly two lines**:
|
||||
|
||||
```
|
||||
VERDICT: accept | reject
|
||||
missing: ...
|
||||
```
|
||||
|
||||
Any extra blank line, `Checks` with other wrapping, or a sentence between the last bullet and `VERDICT:` rejects a real accept. That’s stricter than “must show artifact quotes,” and can fail a judge that did the right thing.
|
||||
|
||||
**3. Grill-me vs Ready is prompt-only**
|
||||
`Grill me` sets `skipReadyMenu` for **one** `agent_end`, then the next turn with any goal lines shows Ready again. Nothing checks that the understanding question was asked **and answered**. If the model asks and offers Ready in the same turn, or the human never replies, Ready is still offered. Required: no Ready until that question has completed.
|
||||
|
||||
**4. Legacy unversioned plans are dropped with no migration path in code**
|
||||
Only `<session_id>-vN.md` is selected; `planVersion` restore is the only resume hook. That’s the intended break, but I found no test that old `.pi/plan/*.md` names stay unused and that a new `/goals` always writes a fresh `-vN` without rewriting prior files.
|
||||
|
||||
**5. Could not verify the focused tests or `git diff`**
|
||||
No test file opened (`src/index.test.ts`, `test/*.test.ts`, etc. all missing from this view). I could not confirm coverage for version reservation, visible plan print, grill-me sequencing, or reject-accept-without-checks.
|
||||
|
||||
I would **not APPROVE** this set as-is. Fix the README conflicts, tighten grill-me so Ready stays suppressed until after the human answers, and either loosen the checks regex to the documented “quotes present before VERDICT” rule or lock the exact judge format in a test.
|
||||
@@ -0,0 +1,41 @@
|
||||
I only have the `read` tool available, so let me locate the changed files by probing the repository structure.The extension source is clear. Let me verify the pi API usage (`sendMessage` with `customType`/`display`, `appendEntry`, `getSessionId`, `ctx.compact`) against the pi docs, since several required behaviors hinge on them.I can't run `git diff` or the test suite — this review environment only exposes a `read` tool, and the test directory can't be enumerated (every probe under `test/` returned ENOENT, and directories can't be listed). So I reviewed the changed files directly in the working tree (`src/index.ts`, `src/prompts.ts`, `README.md`, `scripts/check-judge-footprint.sh`) against the five required behaviors, and verified every pi API the behaviors depend on (`pi.sendMessage` with `customType`/`display`, `pi.appendEntry` restore via `session_start`, `pi.sendUserMessage` followUp, `ctx.compact` with onComplete/onError) against the installed pi docs. Findings below.
|
||||
|
||||
## Defects
|
||||
|
||||
**1. `/goals` arg parsing swallows ordinary objectives — src/index.ts, `/goals` handler**
|
||||
|
||||
```ts
|
||||
if (arg.startsWith("judge")) {
|
||||
```
|
||||
|
||||
Any objective whose first word is "judge" — e.g. `/goals judge the vendor options`, `/goals judge readiness of the export` — is treated as the judge-model subcommand: it sets `judgeModel` to "the vendor options" and **does not reserve a new `.pi/plan/<session_id>-vN.md`**, violating "every ordinary `/goals` invocation reserves a new file". Same class of issue for an objective that is exactly `clear` (deletes the plan instead of planning). Use exact-match subcommand parsing (`arg === "judge"` / `arg.startsWith("judge ")` at minimum, and even then an objective like "judge model quality" is unreachable — a `:` or `--` separator would be safer).
|
||||
|
||||
**2. The checked-artifact-list regex false-rejects compliant judges — `decideSignOff`, src/index.ts**
|
||||
|
||||
```ts
|
||||
const checks = /^checks:\s*\n(?:-\s+.+\n)+VERDICT:/im.test(judge.output);
|
||||
```
|
||||
|
||||
This requires the last `- ` bullet to be *immediately* followed by `VERDICT:` with no blank line, and requires the heading to be exactly `checks:` at line start. Nothing in `judgeSystem` tells the judge not to separate sections with a blank line (models habitually emit `…bullet\n\nVERDICT: accept`), and a judge writing `## checks:` or `**checks:**` also fails. Result: a valid accept *with* a real checked-artifact list is rejected, and the working agent gets a "Missing: checked-artifact list" reply it already satisfied — a retry loop against a nondeterministic judge. It's fail-closed (never accepts without the list, so the hard requirement holds), but as written it will produce systematic false rejects. Allow optional blank lines / formatting, e.g. `/^#*\s*checks:\s*\n(?:[-*]\s+.+\n)+\s*VERDICT:/im`.
|
||||
|
||||
**3. Judge transcript files collide within the same minute — CompleteGoal `execute`, src/index.ts**
|
||||
|
||||
```ts
|
||||
const rel = `.pi/judge/${stamp().replace(/[: ]/g, "-")}.md`;
|
||||
```
|
||||
|
||||
`stamp()` has minute resolution, so two sign-offs in the same minute (two goals signed off back-to-back — the common case) write the same `.pi/judge/<stamp>.md` path and the second silently overwrites the first's full transcript, contradicting "every run saves the judge's full transcript … referenced from the log line" (both log lines then cite one file containing only the second run). Add seconds or a short unique suffix.
|
||||
|
||||
## Minor / cosmetic
|
||||
|
||||
- Misindented closing `}` of the `if (!checks)` block in `decideSignOff` (extra indentation on the `}` line) — likely flagged by `biome check src/`, which `prepublishOnly` runs.
|
||||
- In plan mode, if the agent drafts a plan with no recognizable `goal:` checkbox line, the `agent_end` `while (scanGoals(...).length > 0)` loop never shows the Ready menu and plan mode can't be exited via the menu; the "no recognizable goal line" nudge in `dueInjection` only fires in the *non*-plan-mode branch. Not one of the five required behaviors, but a reachable dead end (escape hatch is `/goals clear`).
|
||||
|
||||
## Behaviors verified as correct
|
||||
|
||||
- **Versioning**: each ordinary `/goals` writes a fresh `<session_id>-vN.md` (empty reservation) via `nextPlanVersion` (max+1, so no clobbering); prior versions are never touched; legacy unversioned names are ignored by `nextPlanVersion` and never read — consistent with the intentional breaking change. `planVersion` is persisted via `appendEntry` and restored in `session_start`, so it survives resume/reload.
|
||||
- **Visible plan before Ready**: `pi.sendMessage({ customType: "plan", content: plan, display: true })` fires before `ctx.ui.select(...)`, and re-prints inside the `$EDITOR` loop when the text changed. `sendMessage`+`display: true` is the documented visible-transcript mechanism.
|
||||
- **Grill me**: sets `skipReadyMenu`, queues the single understanding-check question as a followUp user message, and consumes the skip at the next `agent_end` — so no Ready offer exists between asking the question and the human's reply being processed. `skipReadyMenu` is persisted, so it survives resume mid-grill.
|
||||
- **Judge**: `buildJudgeArgs` yields `pi -p --no-session --no-extensions --tools read,grep,find,ls --exclude-tools edit,write`; `judgeUser` embeds the complete plan file; `judgeSystem` demands per-artifact verbatim quotes framed as an inspectable record ("not hidden reasoning", "no bullet for a file you did not open"); an accept without the `checks:` list is rejected in `decideSignOff` (subject to defect 2); full transcript persisted per run (subject to defect 3).
|
||||
|
||||
Main caveat: I could not execute `git diff` or `npm test` here, so I can't confirm the exact diff boundaries or that the focused vitest cases (judge argv invariants, `appendLog`, `decideSignOff` fail-forward) pass — defects 1–3 are from source inspection, not reproduced runs. Not approved as-is; defects 1 and 2 are worth fixing before merge.
|
||||
@@ -0,0 +1,33 @@
|
||||
# Plan flow and judge review
|
||||
|
||||
- [x] goal: Each new `/goals` draft uses a fresh session-plan version
|
||||
- [x] Persist the selected `-vN` name so resume, reminders, Ready, and sign-off use one file.
|
||||
- [x] Keep earlier versioned files unchanged.
|
||||
- [x] Reserve `--clear` and `--judge` for commands so normal objectives are always new drafts.
|
||||
- failure mode: a second `/goals`, including an objective that begins with `judge`, changes the earlier plan or does not make a draft.
|
||||
- deliverable: [goals-flow.test.ts](../../../test/goals-flow.test.ts) shows an unchanged legacy file and `v1`, new `v2`, and `judge the vendor options` in new `v3`.
|
||||
- [x] goal: Plan review asks and displays the needed context
|
||||
- [x] Add `Grill me` to the Ready menu and queue an understanding-check interview turn.
|
||||
- [x] Keep one short goal subject with its full indented context block.
|
||||
- [x] Keep visible plan output before the Ready dialog.
|
||||
- failure mode: Grill me starts work or the plan is only hidden in an edit call.
|
||||
- deliverable: [goals-flow.test.ts](../../../test/goals-flow.test.ts) records display before dialog and the grill follow-up.
|
||||
- [x] goal: Judge review is visible without being confused with agent evidence
|
||||
- [x] Require concise observed checks before the verdict.
|
||||
- [x] Save the full judge reply under a unique path and link it from the plan log.
|
||||
- [x] Accept a headed check list with normal Markdown spacing, but reject an accept with no list.
|
||||
- failure mode: provider-private reasoning is claimed as evidence, the review is not inspectable, or a correct judge reply is rejected for blank-line formatting.
|
||||
- deliverable: [decide-signoff.test.ts](../../../test/decide-signoff.test.ts) locks the checked-artifact review contract, including a Markdown heading and blank line before the verdict.
|
||||
|
||||
## UAT / Verification
|
||||
|
||||
Observed 2026-08-24: `npm test` reported `Test Files 6 passed (6)` and `Tests 22 passed (22)`.
|
||||
`npm run typecheck`, `npm run lint`, and `git diff --check` exited 0. The focused flow test proves
|
||||
plan versioning, visible plan-before-dialog ordering, Grill me behavior, and objectives beginning
|
||||
with `judge`.
|
||||
|
||||
## Appendix (context, not approved)
|
||||
|
||||
Issue #1 has a 600 second judge timeout now. The judge stays a separate read-only `pi -p --no-session` subprocess. Intercom is unsuitable because it has no equivalent isolation boundary.
|
||||
|
||||
External review: [Kimi K3](../../reviews/pi-goals-kimi-k3.md) found the command-prefix, check-list formatting, and transcript-path defects; all were fixed. [Grok 4.6](../../reviews/pi-goals-grok-4-6-retry.md) confirmed the check-list concern. Its Grill me concern does not apply: `skipReadyMenu` suppresses the menu after the generated follow-up, and the next `agent_end` follows the human reply.
|
||||
+75
-28
@@ -1,14 +1,12 @@
|
||||
/**
|
||||
* pi-goals v2 — plan mode drafts goals into .pi/plan/<session_id>.md, the agent works them with its
|
||||
* PI: pi-goals v2 drafts goals into .pi/plan/<session_id>-vN.md, the agent works them with its
|
||||
* normal Edit tool, and a fresh read-only judge signs each goal off through the one blessed tool,
|
||||
* CompleteGoal.
|
||||
*
|
||||
* One plan file per session, not per repo: two windows on one checkout, and any subagent (pi spawns
|
||||
* those with --no-session, extensions ON), each resolve a different path, so they can't read or
|
||||
* stomp each other's plan. The file name is also the arm switch: a session that never ran /goals has
|
||||
* no file at its path, so the widget, the injections and CompleteGoal all stay silent. The id is
|
||||
* stable exactly where it must be -- a resume reads it back from the session header, and a
|
||||
* compaction keeps it; only an explicit fork/new session gets a new one.
|
||||
* PI: Each /goals call makes a new plan version, `.pi/plan/<session_id>-vN.md`. The selected version
|
||||
* stays in session state across resume and compaction. Old drafts stay available but inert, so a new
|
||||
* conversation cannot silently edit them. The filename is the arm switch: a session that never ran
|
||||
* /goals has no active plan, so the widget, injections, and CompleteGoal all stay silent.
|
||||
*
|
||||
* The v1 lesson: the parser existed so TypeScript could read the plan, but almost every reader is a
|
||||
* model. So v2 has NO parser and no schema. The harness does exactly three things for a
|
||||
@@ -40,18 +38,18 @@
|
||||
*/
|
||||
|
||||
import { spawn, spawnSync } from "node:child_process";
|
||||
import { existsSync, mkdirSync, readFileSync, rmSync, writeFileSync } from "node:fs";
|
||||
import { existsSync, mkdirSync, readdirSync, readFileSync, rmSync, writeFileSync } from "node:fs";
|
||||
import { basename, join, resolve } from "node:path";
|
||||
import type { ExtensionAPI, ExtensionContext } from "@earendil-works/pi-coding-agent";
|
||||
import { Type } from "typebox";
|
||||
import { completeGoalDescription, completeGoalParamDescription, judgeSystem, judgeUser, planDrafting, reminder, resync } from "./prompts.js";
|
||||
import { completeGoalDescription, completeGoalParamDescription, grillMe, judgeSystem, judgeUser, planDrafting, reminder, resync } from "./prompts.js";
|
||||
|
||||
const STATE = "pi-goals-state";
|
||||
const STATUS_KEY = "pi-goals";
|
||||
const WIDGET_KEY = "pi-goals-widget";
|
||||
const PLAN_DIR = ".pi/plan";
|
||||
// For static text (the /goals description) where there is no ctx to resolve the session id.
|
||||
const PLAN_SHAPE = `${PLAN_DIR}/<session_id>.md`;
|
||||
const PLAN_SHAPE = `${PLAN_DIR}/<session_id>-vN.md`;
|
||||
// Judge toolset: strictly read-only, NO bash -- the judge can never execute or mutate anything, and
|
||||
// in particular never re-runs a verify command (which may be a 10-hour training job). The agent runs
|
||||
// verify itself and saves the output as evidence; the judge reads it. Names match pi's tool registry.
|
||||
@@ -104,14 +102,26 @@ export function openSubtasks(plan: string, goalLine: number): string[] {
|
||||
return out;
|
||||
}
|
||||
|
||||
export function nextPlanVersion(planNames: string[], sessionId: string): number {
|
||||
const prefix = `${sessionId}-v`;
|
||||
const versions = planNames.flatMap((name) => {
|
||||
if (!name.startsWith(prefix) || !name.endsWith(".md")) return [];
|
||||
const version = Number(name.slice(prefix.length, -".md".length));
|
||||
return Number.isInteger(version) && version > 0 ? [version] : [];
|
||||
});
|
||||
return Math.max(0, ...versions) + 1;
|
||||
}
|
||||
|
||||
interface PlanState {
|
||||
isPlanMode: boolean;
|
||||
/** Optional model ref for the sign-off judge; unset => current session model, else pi's default. */
|
||||
judgeModel: string | null;
|
||||
planVersion: number | null;
|
||||
skipReadyMenu: boolean;
|
||||
}
|
||||
|
||||
export default function piGoalsExtension(pi: ExtensionAPI): void {
|
||||
let state: PlanState = { isPlanMode: false, judgeModel: null };
|
||||
let state: PlanState = { isPlanMode: false, judgeModel: null, planVersion: null, skipReadyMenu: false };
|
||||
// Reminder cadence (pi-tasks style): the plan is re-sent only after it has gone untouched for
|
||||
// STALE_TURNS turns, and editing it resets the clock -- an agent that is maintaining the file
|
||||
// doesn't need to be told to. In-memory, like pi-tasks: a new session starts fresh.
|
||||
@@ -121,13 +131,18 @@ export default function piGoalsExtension(pi: ExtensionAPI): void {
|
||||
// the WHOLE file (appendix included) instead of just the working set.
|
||||
let resyncReason: string | null = "New session.";
|
||||
|
||||
const planRel = (ctx: ExtensionContext) => `${PLAN_DIR}/${ctx.sessionManager.getSessionId()}.md`;
|
||||
const planPath = (ctx: ExtensionContext) => join(ctx.cwd, planRel(ctx));
|
||||
const readPlan = (ctx: ExtensionContext): string => (existsSync(planPath(ctx)) ? readFileSync(planPath(ctx), "utf-8") : "");
|
||||
const planRel = (ctx: ExtensionContext) => (state.planVersion === null ? PLAN_SHAPE : `${PLAN_DIR}/${ctx.sessionManager.getSessionId()}-v${state.planVersion}.md`);
|
||||
const planPath = (ctx: ExtensionContext) => {
|
||||
if (state.planVersion === null) throw new Error("No active plan version.");
|
||||
return join(ctx.cwd, planRel(ctx));
|
||||
};
|
||||
const readPlan = (ctx: ExtensionContext): string => (state.planVersion !== null && existsSync(planPath(ctx)) ? readFileSync(planPath(ctx), "utf-8") : "");
|
||||
const writePlan = (ctx: ExtensionContext, content: string): void => {
|
||||
mkdirSync(join(ctx.cwd, PLAN_DIR), { recursive: true });
|
||||
writeFileSync(planPath(ctx), content);
|
||||
};
|
||||
const nextVersion = (ctx: ExtensionContext): number =>
|
||||
nextPlanVersion(existsSync(join(ctx.cwd, PLAN_DIR)) ? readdirSync(join(ctx.cwd, PLAN_DIR)) : [], ctx.sessionManager.getSessionId());
|
||||
|
||||
function persist(): void {
|
||||
pi.appendEntry<PlanState>(STATE, state);
|
||||
@@ -164,25 +179,31 @@ export default function piGoalsExtension(pi: ExtensionAPI): void {
|
||||
// --- /goals: enter plan mode (or clear / set judge) --------------------------------------------
|
||||
|
||||
pi.registerCommand("goals", {
|
||||
description: `Plan mode: draft goals into ${PLAN_SHAPE}, review, then work them. /goals <objective> | /goals clear | /goals judge <model>`,
|
||||
description: `Plan mode: draft goals into ${PLAN_SHAPE}, review, then work them. /goals <objective> | /goals --clear | /goals --judge <model>`,
|
||||
handler: async (args, ctx) => {
|
||||
const arg = args.trim();
|
||||
if (arg === "clear") {
|
||||
if (arg === "--clear") {
|
||||
if (state.planVersion === null) {
|
||||
ctx.ui.notify("No active plan to delete.", "info");
|
||||
return;
|
||||
}
|
||||
const currentPlan = planRel(ctx);
|
||||
rmSync(planPath(ctx), { force: true });
|
||||
state = { ...state, isPlanMode: false };
|
||||
state = { ...state, isPlanMode: false, planVersion: null, skipReadyMenu: false };
|
||||
persist();
|
||||
updateWidget(ctx);
|
||||
ctx.ui.notify(`Deleted ${planRel(ctx)}.`, "info");
|
||||
ctx.ui.notify(`Deleted ${currentPlan}.`, "info");
|
||||
return;
|
||||
}
|
||||
if (arg.startsWith("judge")) {
|
||||
const ref = arg.slice("judge".length).trim();
|
||||
if (arg === "--judge" || arg.startsWith("--judge ")) {
|
||||
const ref = arg.slice("--judge".length).trim();
|
||||
state = { ...state, judgeModel: ref || null };
|
||||
persist();
|
||||
ctx.ui.notify(ref ? `Sign-off judge model set to ${ref}` : "Sign-off judge reset to the session model", "info");
|
||||
return;
|
||||
}
|
||||
state = { ...state, isPlanMode: true };
|
||||
state = { ...state, isPlanMode: true, planVersion: nextVersion(ctx), skipReadyMenu: false };
|
||||
writePlan(ctx, "");
|
||||
persist();
|
||||
updateWidget(ctx);
|
||||
// The drafting rules are sent ONCE, with the seed. v2 re-injected them every turn, which is
|
||||
@@ -258,22 +279,28 @@ export default function piGoalsExtension(pi: ExtensionAPI): void {
|
||||
}
|
||||
});
|
||||
|
||||
// After a plan-mode turn: if goals were drafted, print the working set and offer Ready. The plan
|
||||
// PI: After a plan-mode turn, print the full plan and offer Ready. The plan
|
||||
// is printed because "Ready?" over an unread file is not a review: the only other copy is inside
|
||||
// a collapsed edit tool call. Reprinted after an $EDITOR pass only if the text changed.
|
||||
// The human then says go, edits it, or keeps talking to revise it (menu shape borrowed from pi-plan).
|
||||
pi.on("agent_end", async (_event, ctx) => {
|
||||
if (!state.isPlanMode || !ctx.hasUI) return;
|
||||
if (state.skipReadyMenu) {
|
||||
state = { ...state, skipReadyMenu: false };
|
||||
persist();
|
||||
return;
|
||||
}
|
||||
let printed = "";
|
||||
while (scanGoals(readPlan(ctx)).length > 0) {
|
||||
const working = foldPlan(readPlan(ctx));
|
||||
if (working !== printed) {
|
||||
printed = working;
|
||||
pi.sendMessage({ customType: "plan", content: working, display: true });
|
||||
const plan = readPlan(ctx);
|
||||
if (plan !== printed) {
|
||||
printed = plan;
|
||||
pi.sendMessage({ customType: "plan", content: plan, display: true });
|
||||
}
|
||||
const choice = await ctx.ui.select(`Plan drafted in ${planRel(ctx)}. Ready?`, [
|
||||
"Ready — start working the plan",
|
||||
"Ready + compact — the same, but summarize the planning chatter away first",
|
||||
"Grill me — ask one question that tests your plan understanding",
|
||||
"Open in $EDITOR — edit it myself",
|
||||
"Keep planning (reply to revise)",
|
||||
]);
|
||||
@@ -281,6 +308,12 @@ export default function piGoalsExtension(pi: ExtensionAPI): void {
|
||||
spawnSync(process.env.EDITOR || process.env.VISUAL || "vi", [planPath(ctx)], { stdio: "inherit" });
|
||||
continue;
|
||||
}
|
||||
if (choice?.startsWith("Grill me")) {
|
||||
state = { ...state, skipReadyMenu: true };
|
||||
persist();
|
||||
pi.sendUserMessage(grillMe, { deliverAs: "followUp" });
|
||||
return;
|
||||
}
|
||||
if (!choice?.startsWith("Ready")) return;
|
||||
// Plan mode goes off BEFORE the compaction, not in its callback: a message typed while
|
||||
// the summary runs is then handled in work mode. Left on, it would hit blocked tools, a
|
||||
@@ -313,7 +346,12 @@ export default function piGoalsExtension(pi: ExtensionAPI): void {
|
||||
.getEntries()
|
||||
.filter((e: { type?: string; customType?: string }) => e.type === "custom" && e.customType === STATE)
|
||||
.pop() as { data?: PlanState } | undefined;
|
||||
if (last?.data) state = { ...state, ...last.data };
|
||||
state = {
|
||||
isPlanMode: last?.data?.isPlanMode ?? false,
|
||||
judgeModel: last?.data?.judgeModel ?? null,
|
||||
planVersion: last?.data?.planVersion ?? null,
|
||||
skipReadyMenu: last?.data?.skipReadyMenu ?? false,
|
||||
};
|
||||
lastSeenPlan = readPlan(ctx);
|
||||
resyncReason = "New session.";
|
||||
updateWidget(ctx);
|
||||
@@ -348,7 +386,7 @@ export default function piGoalsExtension(pi: ExtensionAPI): void {
|
||||
if (judgeRaw !== null) {
|
||||
const raw: JudgeResult = judgeRaw;
|
||||
mkdirSync(join(ctx.cwd, ".pi", "judge"), { recursive: true });
|
||||
const rel = `.pi/judge/${stamp().replace(/[: ]/g, "-")}.md`;
|
||||
const rel = `.pi/judge/${stamp().replace(/[: ]/g, "-")}-${process.hrtime.bigint()}.md`;
|
||||
writeFileSync(join(ctx.cwd, rel), `goal: ${params.goal}\nmodel: ${judgeModel ?? "pi default"}\nerror: ${raw.error ?? "none"}\n\n${raw.output}\n`);
|
||||
transcriptNote = ` (${rel})`;
|
||||
}
|
||||
@@ -446,6 +484,15 @@ export async function decideSignOff(
|
||||
const reasoning = judge.output.length > 2000 ? `...\n${judge.output.slice(-2000)}` : judge.output;
|
||||
|
||||
if (verdict === "accept") {
|
||||
const beforeVerdict = judge.output.slice(0, judge.output.indexOf(verdictLine));
|
||||
const checks = /^#{0,6}\s*(?:\*\*)?checks(?:\*\*)?:\s*$[\s\S]*^[-*]\s+.+$/im.test(beforeVerdict);
|
||||
if (!checks) {
|
||||
return {
|
||||
resultText: `Sign-off REJECTED. Missing:\nchecked-artifact list before VERDICT: accept\n\n--- judge ---\n${reasoning}`,
|
||||
isError: true,
|
||||
logEntry: `reject "${input.goal}": judge accept had no checked-artifact list`,
|
||||
};
|
||||
}
|
||||
return {
|
||||
resultText: `Sign-off ACCEPTED (log line appended).\n\n--- judge ---\n${reasoning}`,
|
||||
isError: false,
|
||||
|
||||
+10
-2
@@ -27,7 +27,7 @@
|
||||
* 1. planDrafting — SETUP, plan mode (read-only: edit/write blocked except the plan file)
|
||||
* ──────────────────────────────────────────────────────────────────────── */
|
||||
export const planDrafting = `\
|
||||
You are in plan mode. You are making a short judgable plan the captures the user true goals, arrives at and checked by conversation.
|
||||
You are in plan mode. You are making a short judgeable plan that captures the user's real goals, then tests it in conversation.
|
||||
|
||||
1. Explore the repository read-only first: resolve discoverable facts by looking them up, and only ask
|
||||
the human when the answer is a genuine intent or preference choice. Do not write or run code in this
|
||||
@@ -42,7 +42,7 @@ requirement arrives, fold it in, say what changed, and say the plan is final aga
|
||||
doesn't change a goal or a discriminator belongs in the appendix, not in the goals.
|
||||
|
||||
Right-size it:
|
||||
- One goal per distinct judgable outcome. Group related goals when it helps judge them together
|
||||
- One goal per distinct judgeable outcome. Group related goals when it helps judge them together
|
||||
and readability. The count flows from the outcomes.
|
||||
- Describe outcomes in qualitative terms the judge and user can discriminate.
|
||||
- Use the users language or more precise don't transform "MV" into "knob" as it looses precision and is overloaded
|
||||
@@ -50,6 +50,7 @@ Right-size it:
|
||||
- Quantitative gates are fine only when you are certain they survive contact with reality.
|
||||
- Subtasks are the steps inside a goal; add them when a goal has 3+ distinct steps, skip otherwise.
|
||||
- Two goals that share one discriminator are one goal. Merge them.
|
||||
- Keep the goal subject short. Put its important scope, failure modes, discriminator, tasks, and evidence in the indented block beneath it. The judge reads the whole block and the whole plan.
|
||||
- Everything above "## Log" is the part the model updated while it works, and the user reviews. Keep it under 50 lines,
|
||||
reviewable in one pass. Everything below "## Log" is unlimited.
|
||||
|
||||
@@ -122,6 +123,9 @@ When the goals are drafted, present them and say the plan is final. Do not begin
|
||||
* model to ignore the task block" (tintinweb/pi-tasks CHANGELOG.md:149). Carries the folded plan
|
||||
* (above ## Log), because a nudge with no plan in it makes the model go read the file anyway.
|
||||
* ──────────────────────────────────────────────────────────────────────── */
|
||||
export const grillMe = `\
|
||||
Stay in plan mode. Ask the human the single most useful question that tests whether your understanding of the plan is correct. Name the interpretation you are testing. Do not start work, change the plan, or offer Ready until the human replies.`;
|
||||
|
||||
export function reminder(foldedPlan: string, planRel: string): string {
|
||||
return `\
|
||||
<system-reminder>
|
||||
@@ -204,6 +208,10 @@ job; the agent must bring you its saved output. Your job is evidence discipline,
|
||||
not just unmentioned? If the goal names a verify: command, its saved output must be among the
|
||||
evidence, and the command must actually test the discriminator rather than pass tautologically.
|
||||
|
||||
Before the verdict, write this heading: checks:. Put one concise bullet under it for each artifact you actually read:
|
||||
path, verbatim observed quote, and what that observation establishes. This is an inspectable review
|
||||
record, not hidden reasoning. Do not write a checks bullet for a file you did not open.
|
||||
|
||||
Finish with exactly these two lines and nothing after:
|
||||
VERDICT: accept | reject
|
||||
missing: <empty if accept; otherwise a short list of what's needed before this can be accepted>`;
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
import { describe, expect, it, vi } from "vitest";
|
||||
import { decideSignOff, type JudgeResult } from "../src/index.js";
|
||||
import { judgeSystem } from "../src/prompts.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
|
||||
@@ -9,11 +10,21 @@ const input = { goal: "x", plan: "# plan\n", planRel: ".pi/plan/s1.md", judgeMod
|
||||
|
||||
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 output = "## checks:\n- evidence.txt: `PASS`; the saved check passed\n\nThe artifact proves the gate passed.\nVERDICT: accept\nmissing:";
|
||||
const runJudge = vi.fn().mockResolvedValue({ output });
|
||||
const out = await decideSignOff({ ...input, plan: "# plan\n1. [ ] goal: x\n" }, 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");
|
||||
expect(out.resultText).toContain("evidence.txt: `PASS`");
|
||||
});
|
||||
|
||||
it("rejects an accept verdict without a checked-artifact list", async () => {
|
||||
const runJudge = vi.fn().mockResolvedValue({ output: "VERDICT: accept\nmissing:" });
|
||||
const out = await decideSignOff(input, undefined, runJudge);
|
||||
expect(out.isError).toBe(true);
|
||||
expect(out.resultText).toContain("checked-artifact list");
|
||||
expect(out.logEntry).toContain("no checked-artifact list");
|
||||
});
|
||||
|
||||
it("a judge-subprocess error yields accepted_inconclusive with a 'ran but failed' reason", async () => {
|
||||
@@ -54,6 +65,11 @@ describe("decideSignOff (fail-forward invariant)", () => {
|
||||
expect(out.logEntry).toContain("reject");
|
||||
});
|
||||
|
||||
it("requires a concise checked-artifact review, not private reasoning", () => {
|
||||
expect(judgeSystem).toContain("checks:");
|
||||
expect(judgeSystem).toContain("not hidden reasoning");
|
||||
});
|
||||
|
||||
it("writes nothing when aborted after the judge ran", async () => {
|
||||
const runJudge = vi.fn().mockResolvedValue({ output: "VERDICT: accept" });
|
||||
const ctrl = new AbortController();
|
||||
|
||||
@@ -0,0 +1,80 @@
|
||||
import { mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs";
|
||||
import { tmpdir } from "node:os";
|
||||
import { join } from "node:path";
|
||||
import type { ExtensionAPI } from "@earendil-works/pi-coding-agent";
|
||||
import { describe, expect, it } from "vitest";
|
||||
import piGoalsExtension from "../src/index.js";
|
||||
|
||||
function setup(selectChoices: Array<string | undefined>) {
|
||||
const cwd = mkdtempSync(join(tmpdir(), "pi-goals-flow-"));
|
||||
const commands = new Map<string, any>();
|
||||
const hooks = new Map<string, any>();
|
||||
const events: string[] = [];
|
||||
const messages: Array<{ content: string; display?: boolean }> = [];
|
||||
const ctx = {
|
||||
cwd,
|
||||
hasUI: true,
|
||||
sessionManager: { getSessionId: () => "session-a", getEntries: () => [] },
|
||||
ui: {
|
||||
theme: { fg: (_kind: string, text: string) => text },
|
||||
setStatus: () => {},
|
||||
setWidget: () => {},
|
||||
notify: () => {},
|
||||
select: async () => {
|
||||
events.push("select");
|
||||
return selectChoices.shift();
|
||||
},
|
||||
},
|
||||
};
|
||||
const pi = {
|
||||
registerCommand: (name: string, command: any) => commands.set(name, command),
|
||||
on: (name: string, handler: any) => hooks.set(name, handler),
|
||||
appendEntry: () => {},
|
||||
registerTool: () => {},
|
||||
sendMessage: (message: { content: string; display?: boolean }) => {
|
||||
events.push("display");
|
||||
messages.push(message);
|
||||
},
|
||||
sendUserMessage: (message: string) => messages.push({ content: message }),
|
||||
};
|
||||
piGoalsExtension(pi as unknown as ExtensionAPI);
|
||||
return { commands, ctx, cwd, events, hooks, messages };
|
||||
}
|
||||
|
||||
describe("/goals draft flow", () => {
|
||||
it("creates a new version, preserves the prior draft, displays the full plan before Ready, and keeps Grill me in plan mode", async () => {
|
||||
const flow = setup(["Grill me — ask one question that tests your plan understanding"]);
|
||||
try {
|
||||
const legacy = join(flow.cwd, ".pi/plan/session-a.md");
|
||||
mkdirSync(join(flow.cwd, ".pi/plan"), { recursive: true });
|
||||
writeFileSync(legacy, "old plan");
|
||||
await flow.commands.get("goals").handler("first objective", flow.ctx);
|
||||
const v1 = join(flow.cwd, ".pi/plan/session-a-v1.md");
|
||||
expect(readFileSync(v1, "utf-8")).toBe("");
|
||||
expect(readFileSync(legacy, "utf-8")).toBe("old plan");
|
||||
const plan = "# First plan\n\n## Goals\n\n1. [ ] goal: preserve this\n\n## Appendix (context, not approved)\nold context\n";
|
||||
mkdirSync(join(flow.cwd, ".pi/plan"), { recursive: true });
|
||||
writeFileSync(v1, plan);
|
||||
|
||||
await flow.hooks.get("agent_end")({}, flow.ctx);
|
||||
expect(flow.events).toEqual(["display", "select"]);
|
||||
expect(flow.messages.at(-1)?.content).toContain("Ask the human the single most useful question");
|
||||
expect(flow.messages.find((message) => message.display)?.content).toBe(plan);
|
||||
await flow.hooks.get("agent_end")({}, flow.ctx);
|
||||
expect(flow.events).toEqual(["display", "select"]);
|
||||
const blocked = await flow.hooks.get("tool_call")({ toolName: "edit", input: { path: "README.md" } }, flow.ctx);
|
||||
expect(blocked?.block).toBe(true);
|
||||
|
||||
await flow.commands.get("goals").handler("second objective", flow.ctx);
|
||||
expect(readFileSync(v1, "utf-8")).toBe(plan);
|
||||
expect(readFileSync(join(flow.cwd, ".pi/plan/session-a-v2.md"), "utf-8")).toBe("");
|
||||
expect(flow.messages.at(-1)?.content).toContain("session-a-v2.md");
|
||||
|
||||
await flow.commands.get("goals").handler("judge the vendor options", flow.ctx);
|
||||
expect(readFileSync(join(flow.cwd, ".pi/plan/session-a-v3.md"), "utf-8")).toBe("");
|
||||
expect(flow.messages.at(-1)?.content).toContain("Objective: judge the vendor options");
|
||||
} finally {
|
||||
rmSync(flow.cwd, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user