diff --git a/docs/reviews/goals_menu2.md b/docs/reviews/goals_menu2.md new file mode 100644 index 0000000..783f189 --- /dev/null +++ b/docs/reviews/goals_menu2.md @@ -0,0 +1,66 @@ +Let me get the diff to see what changed: + +Now let me check the git history to see what changedNow I have full context. Let me verify the pi API behavior by checking the relevant source files to confirm the claims about `sendMessage`, `compact`, and event orderingNow I have all the context needed. Let me verify the `sendMessage` path more carefully: + +I now have a complete picture. Here is the review. + +--- + +## Code Review: ready menu — print plan + add "Ready + compact" + +### Summary +The `agent_end` handler now prints the plan working set into the transcript before the menu, and adds a 4th option ("Ready + compact") that compacts the planning chatter away before the work turn. The logic is correct and well-considered, with one structural race and one stale-widget gap. + +### Important (should fix) + +- **`src/index.ts:~248` (the `agent_end` handler) — race: plan mode is turned off before compaction completes.** + `state = { ...state, isPlanMode: false }` executes before `ctx.compact()` is called (let alone finished). The compaction is fire-and-forget. If the human types and sends a message in the window between the state flip and the compaction callback firing, the agent starts a turn in normal mode *without* the work instruction. The work instruction arrives later (via `sendUserMessage` in `onComplete`/`onError`), triggering a second, redundant turn. + The window is small (compaction is fast) but the behavior is undefined — the agent could start executing before the work instruction lands. + **Fix**: flip `isPlanMode` inside the callbacks, not before. Move `state = { ...state, isPlanMode: false }; persist(); updateWidget(ctx);` into both `onComplete` and `onError`, and also into the non-compact branch (where it already is, effectively). The `work` string can be defined before the branch. + + ```typescript + if (!choice.includes("compact")) { + state = { ...state, isPlanMode: false }; + persist(); + updateWidget(ctx); + pi.sendUserMessage(work, { deliverAs: "followUp" }); + return; + } + ctx.compact({ + customInstructions: `...`, + onComplete: () => { + state = { ...state, isPlanMode: false }; + persist(); + updateWidget(ctx); + pi.sendUserMessage(work, { deliverAs: "followUp" }); + }, + onError: (e) => { + ctx.ui.notify(`Compaction failed (${e.message}); starting work anyway.`, "warning"); + state = { ...state, isPlanMode: false }; + persist(); + updateWidget(ctx); + pi.sendUserMessage(work, { deliverAs: "followUp" }); + }, + }); + ``` + + This also means the widget stays in "planning" mode during compaction, which is truthful — compaction hasn't finished yet. + +### Suggestions + +- **`src/index.ts:~248` — widget not refreshed after `$EDITOR`.** + When the human chooses "Open in $EDITOR", `spawnSync` blocks, then `continue` re-enters the loop. The plan is re-read and potentially re-printed, but `updateWidget` is not called. If the human changed goal statuses (e.g. ticked a checkbox), the widget stays stale until the next `turn_end`. + Add `updateWidget(ctx);` after the `spawnSync` line (or inside the `continue` branch before the continue). + +- **`src/index.ts:~248` — `spawnSync` blocks the event loop.** + `spawnSync(process.env.EDITOR || ...)` is a synchronous blocking call. While the editor is open, no async work (including compaction from a previous iteration, timers, etc.) can proceed. This is fine for a local TUI tool, but worth noting — if the editor hangs or the human walks away, the entire pi process is frozen. + +### Positive + +- **De-duplication is correct.** `printed` is a local variable, fresh per `agent_end` call, and correctly suppresses re-printing when the working set hasn't changed across editor passes. The `while` loop exit condition (`scanGoals(...).length > 0`) correctly handles the human deleting all goals in the editor. +- **String matching is safe.** `choice?.startsWith("Ready")` gates both Ready options, then `choice.includes("compact")` distinguishes them. The word "compact" appears only in the "Ready + compact" string. No ambiguity. +- **Both compaction callbacks queue the work turn.** `onComplete` and `onError` both call `pi.sendUserMessage(work, ...)`. A failed compaction does not strand the session — work starts anyway, with a notification. +- **`session_compact` → `resyncReason` → injection chain is correct.** The pi source confirms `session_compact` fires (and is awaited) *before* `this.compact()` resolves and `onComplete` fires. So `resyncReason` is set before the next LLM call, and the full plan file is re-injected. The compaction summarizes away the exploration; the plan itself survives. + +### Verdict +**REQUEST CHANGES** — the race between `isPlanMode = false` and compaction completion is a real timing bug that can cause the agent to start a turn without the work instruction. The fix is straightforward: move the state flip into the callbacks. \ No newline at end of file diff --git a/docs/reviews/goals_menu2_r2.md b/docs/reviews/goals_menu2_r2.md new file mode 100644 index 0000000..6d5c6f8 --- /dev/null +++ b/docs/reviews/goals_menu2_r2.md @@ -0,0 +1,22 @@ +You're right on both counts. Let me trace through each. + +**Finding 1, withdrawn.** Keeping `isPlanMode` true during compaction would mean any message the human sends while compaction runs gets processed in plan mode: blocked tools (`tool_call` hook), a "You are still in plan mode" injection (`context` hook), and a second Ready menu on `agent_end`. The current order (flip first, then compact) means that same message is processed in work mode — the destination the session is heading to. The cost is at worst one redundant turn when the `onComplete` follow-up lands. Your order is the correct one. + +**Finding 2, withdrawn.** `updateWidget` with `isPlanMode === true` hits the early return that sets two fixed strings already on screen. It is a pure no-op. + +--- + +## Code Review: ready menu — print plan + add "Ready + compact" + +### Summary +The `agent_end` handler now prints the plan working set into the transcript before the menu, and adds a 4th option ("Ready + compact") that compacts the planning chatter away before the work turn. The logic is correct and well-considered. + +### Positive +- **De-duplication is correct.** `printed` is a local variable, fresh per `agent_end` call, and suppresses re-printing when the working set hasn't changed across `$EDITOR` passes. The `while` loop exits cleanly if the human deletes all goals in the editor. +- **String matching is safe.** `choice?.startsWith("Ready")` gates both Ready options, then `choice.includes("compact")` distinguishes them. The word "compact" appears only in the one label. +- **Both compaction callbacks queue the work turn.** `onComplete` and `onError` both call `pi.sendUserMessage(work, ...)`. A failed compaction does not strand the session. +- **The `session_compact` → `resyncReason` → injection chain is correct.** The pi source confirms `session_compact` fires (and is awaited) before `this.compact()` resolves and `onComplete` fires. The full plan file is re-injected on the next LLM call; the compaction summarizes away the exploration, not the plan. +- **The state-flip-before-compaction order is deliberate and correct.** Flipping `isPlanMode` first means any message the human sends during compaction is handled in work mode, not plan mode (where tools would be blocked and a contradictory injection would fire). + +### Verdict +**APPROVE** — no bugs found. The two concerns raised initially were incorrect; the code handles the interleaving correctly. \ No newline at end of file diff --git a/src/index.ts b/src/index.ts index 7cc319b..ef86516 100644 --- a/src/index.ts +++ b/src/index.ts @@ -282,6 +282,9 @@ export default function piGoalsExtension(pi: ExtensionAPI): void { continue; } 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 + // "still in plan mode" injection, and a second Ready menu at agent_end. state = { ...state, isPlanMode: false }; persist(); updateWidget(ctx);