can1357
a14a2c402c
- Pinned final plan path before handleCompactCommand so queued messages use approved plan, not draft. - Added regression coverage for setPlanReferencePath timing before compaction queue flush.
365 lines
14 KiB
TypeScript
365 lines
14 KiB
TypeScript
import { afterEach, beforeAll, beforeEach, describe, expect, it, vi } from "bun:test";
|
|
import * as path from "node:path";
|
|
import { Agent } from "@oh-my-pi/pi-agent-core";
|
|
import { _resetSettingsForTest, Settings } from "@oh-my-pi/pi-coding-agent/config/settings";
|
|
import { resolveLocalUrlToPath } from "@oh-my-pi/pi-coding-agent/internal-urls";
|
|
import { initTheme } from "@oh-my-pi/pi-coding-agent/modes/theme/theme";
|
|
import { Text } from "@oh-my-pi/pi-tui";
|
|
import { TempDir } from "@oh-my-pi/pi-utils";
|
|
import { ModelRegistry } from "../src/config/model-registry";
|
|
import { InteractiveMode } from "../src/modes/interactive-mode";
|
|
import { AgentSession } from "../src/session/agent-session";
|
|
import { AuthStorage } from "../src/session/auth-storage";
|
|
import { SessionManager } from "../src/session/session-manager";
|
|
|
|
/**
|
|
* Matches the plan-approved synthetic-prompt dispatch. `#approvePlan` calls
|
|
* `session.prompt(rendered, { synthetic: true })` exclusively for that case,
|
|
* so the `synthetic: true` option flag is the unique discriminator.
|
|
*/
|
|
const isPlanApprovedCall = (args: unknown[]): boolean =>
|
|
args.length >= 2 &&
|
|
typeof args[0] === "string" &&
|
|
typeof args[1] === "object" &&
|
|
args[1] !== null &&
|
|
(args[1] as { synthetic?: boolean }).synthetic === true;
|
|
|
|
describe("InteractiveMode plan review rendering", () => {
|
|
let tempDir: TempDir;
|
|
let authStorage: AuthStorage;
|
|
let session: AgentSession;
|
|
let mode: InteractiveMode;
|
|
|
|
beforeAll(() => {
|
|
initTheme();
|
|
});
|
|
|
|
beforeEach(async () => {
|
|
_resetSettingsForTest();
|
|
tempDir = TempDir.createSync("@pi-plan-review-");
|
|
await Settings.init({ inMemory: true, cwd: tempDir.path() });
|
|
authStorage = await AuthStorage.create(path.join(tempDir.path(), "testauth.db"));
|
|
const modelRegistry = new ModelRegistry(authStorage);
|
|
const model = modelRegistry.find("anthropic", "claude-sonnet-4-5");
|
|
if (!model) {
|
|
throw new Error("Expected claude-sonnet-4-5 to exist in registry");
|
|
}
|
|
|
|
session = new AgentSession({
|
|
agent: new Agent({
|
|
initialState: {
|
|
model,
|
|
systemPrompt: ["Test"],
|
|
tools: [],
|
|
messages: [],
|
|
},
|
|
}),
|
|
sessionManager: SessionManager.create(tempDir.path(), tempDir.path()),
|
|
settings: Settings.isolated(),
|
|
modelRegistry,
|
|
});
|
|
mode = new InteractiveMode(session, "test");
|
|
});
|
|
|
|
afterEach(async () => {
|
|
vi.restoreAllMocks();
|
|
mode?.stop();
|
|
await session?.dispose();
|
|
authStorage?.close();
|
|
tempDir?.removeSync();
|
|
_resetSettingsForTest();
|
|
});
|
|
|
|
it("appends each submitted plan review preview to preserve scrollback", async () => {
|
|
const planFilePath = "local://PLAN.md";
|
|
const resolvedPlanPath = resolveLocalUrlToPath(planFilePath, {
|
|
getArtifactsDir: () => session.sessionManager.getArtifactsDir(),
|
|
getSessionId: () => session.sessionManager.getSessionId(),
|
|
});
|
|
await Bun.write(resolvedPlanPath, "# First plan\n\nalpha");
|
|
|
|
mode.planModeEnabled = true;
|
|
mode.planModePlanFilePath = planFilePath;
|
|
vi.spyOn(mode, "showHookSelector").mockResolvedValue("Stay in plan mode");
|
|
|
|
await mode.handleExitPlanModeTool({
|
|
planFilePath,
|
|
planExists: true,
|
|
title: "PLAN",
|
|
finalPlanFilePath: "local://PLAN.md",
|
|
});
|
|
|
|
const firstPreview = mode.chatContainer.children.at(-1);
|
|
expect(firstPreview).toBeDefined();
|
|
expect(firstPreview!.render(120).join("\n")).toContain("First plan");
|
|
|
|
const marker = new Text("MARKER", 0, 0);
|
|
mode.chatContainer.addChild(marker);
|
|
await Bun.write(resolvedPlanPath, "# Second plan\n\nbeta");
|
|
|
|
await mode.handleExitPlanModeTool({
|
|
planFilePath,
|
|
planExists: true,
|
|
title: "PLAN",
|
|
finalPlanFilePath: "local://PLAN.md",
|
|
});
|
|
|
|
const secondPreview = mode.chatContainer.children.at(-1);
|
|
expect(secondPreview).toBeDefined();
|
|
expect(secondPreview).not.toBe(firstPreview);
|
|
expect(mode.chatContainer.children.at(-2)).toBe(marker);
|
|
expect(mode.chatContainer.children.at(-3)).toBe(firstPreview);
|
|
expect(firstPreview!.render(120).join("\n")).toContain("First plan");
|
|
expect(firstPreview!.render(120).join("\n")).not.toContain("Second plan");
|
|
expect(secondPreview!.render(120).join("\n")).toContain("Second plan");
|
|
});
|
|
|
|
it("offers approve-and-keep-context as a distinct plan approval path", async () => {
|
|
const planFilePath = "local://PLAN.md";
|
|
const resolvedPlanPath = resolveLocalUrlToPath(planFilePath, {
|
|
getArtifactsDir: () => session.sessionManager.getArtifactsDir(),
|
|
getSessionId: () => session.sessionManager.getSessionId(),
|
|
});
|
|
await Bun.write(resolvedPlanPath, "# Plan\n\nDo the thing.");
|
|
|
|
mode.planModeEnabled = true;
|
|
mode.planModePlanFilePath = planFilePath;
|
|
const selector = vi.spyOn(mode, "showHookSelector").mockResolvedValue("Stay in plan mode");
|
|
|
|
await mode.handleExitPlanModeTool({
|
|
planFilePath,
|
|
planExists: true,
|
|
title: "PLAN",
|
|
finalPlanFilePath: "local://APPROVED.md",
|
|
});
|
|
|
|
expect(selector).toHaveBeenCalledWith(
|
|
"Plan mode - next step",
|
|
[
|
|
"Approve and execute",
|
|
"Approve and compact context",
|
|
"Approve and keep context",
|
|
"Refine plan",
|
|
"Stay in plan mode",
|
|
],
|
|
expect.any(Object),
|
|
);
|
|
});
|
|
|
|
it("approves a plan without clearing the session when keeping context", async () => {
|
|
const planFilePath = "local://PLAN.md";
|
|
const finalPlanFilePath = "local://APPROVED.md";
|
|
const resolvedPlanPath = resolveLocalUrlToPath(planFilePath, {
|
|
getArtifactsDir: () => session.sessionManager.getArtifactsDir(),
|
|
getSessionId: () => session.sessionManager.getSessionId(),
|
|
});
|
|
const resolvedFinalPlanPath = resolveLocalUrlToPath(finalPlanFilePath, {
|
|
getArtifactsDir: () => session.sessionManager.getArtifactsDir(),
|
|
getSessionId: () => session.sessionManager.getSessionId(),
|
|
});
|
|
await Bun.write(resolvedPlanPath, "# Plan\n\nKeep context.");
|
|
|
|
mode.planModeEnabled = true;
|
|
mode.planModePlanFilePath = planFilePath;
|
|
vi.spyOn(mode, "showHookSelector").mockResolvedValue("Approve and keep context");
|
|
const clear = vi.spyOn(mode, "handleClearCommand").mockResolvedValue();
|
|
const prompt = vi.spyOn(session, "prompt").mockResolvedValue(undefined as never);
|
|
|
|
await mode.handleExitPlanModeTool({
|
|
planFilePath,
|
|
planExists: true,
|
|
title: "PLAN",
|
|
finalPlanFilePath,
|
|
});
|
|
|
|
expect(clear).not.toHaveBeenCalled();
|
|
expect(await Bun.file(resolvedFinalPlanPath).text()).toBe("# Plan\n\nKeep context.");
|
|
expect(prompt).toHaveBeenCalledWith(expect.any(String), {
|
|
synthetic: true,
|
|
});
|
|
});
|
|
|
|
it("keeps the existing approve-and-execute path clearing the session", async () => {
|
|
const planFilePath = "local://PLAN.md";
|
|
const finalPlanFilePath = "local://APPROVED.md";
|
|
const resolvedPlanPath = resolveLocalUrlToPath(planFilePath, {
|
|
getArtifactsDir: () => session.sessionManager.getArtifactsDir(),
|
|
getSessionId: () => session.sessionManager.getSessionId(),
|
|
});
|
|
await Bun.write(resolvedPlanPath, "# Plan\n\nClear context.");
|
|
|
|
mode.planModeEnabled = true;
|
|
mode.planModePlanFilePath = planFilePath;
|
|
vi.spyOn(mode, "showHookSelector").mockResolvedValue("Approve and execute");
|
|
const clear = vi.spyOn(mode, "handleClearCommand").mockResolvedValue();
|
|
const prompt = vi.spyOn(session, "prompt").mockResolvedValue(undefined as never);
|
|
|
|
await mode.handleExitPlanModeTool({
|
|
planFilePath,
|
|
planExists: true,
|
|
title: "PLAN",
|
|
finalPlanFilePath,
|
|
});
|
|
|
|
expect(clear).toHaveBeenCalledTimes(1);
|
|
expect(prompt).toHaveBeenCalledWith(expect.any(String), {
|
|
synthetic: true,
|
|
});
|
|
});
|
|
|
|
it("Approve and compact context: ok outcome dispatches plan-approved after compaction", async () => {
|
|
const planFilePath = "local://PLAN.md";
|
|
const finalPlanFilePath = "local://APPROVED.md";
|
|
const resolvedPlanPath = resolveLocalUrlToPath(planFilePath, {
|
|
getArtifactsDir: () => session.sessionManager.getArtifactsDir(),
|
|
getSessionId: () => session.sessionManager.getSessionId(),
|
|
});
|
|
await Bun.write(resolvedPlanPath, "# Plan\n\nCompact and execute.");
|
|
|
|
mode.planModeEnabled = true;
|
|
mode.planModePlanFilePath = planFilePath;
|
|
vi.spyOn(mode, "showHookSelector").mockResolvedValue("Approve and compact context");
|
|
const compactSpy = vi.spyOn(mode, "handleCompactCommand").mockResolvedValue("ok");
|
|
const markSentSpy = vi.spyOn(session, "markPlanReferenceSent");
|
|
const promptSpy = vi.spyOn(session, "prompt").mockResolvedValue(undefined as never);
|
|
|
|
await mode.handleExitPlanModeTool({
|
|
planFilePath,
|
|
planExists: true,
|
|
title: "PLAN",
|
|
finalPlanFilePath,
|
|
});
|
|
|
|
// Compaction was run with the rendered planning-specific custom instruction.
|
|
expect(compactSpy).toHaveBeenCalledTimes(1);
|
|
const [compactInstruction] = compactSpy.mock.calls[0]!;
|
|
expect(typeof compactInstruction).toBe("string");
|
|
expect(compactInstruction as string).toContain("Preparing to execute the approved plan");
|
|
expect(compactInstruction as string).toContain(finalPlanFilePath);
|
|
|
|
// Plan-approved synthetic prompt was dispatched.
|
|
const planApprovedIdx = promptSpy.mock.calls.findIndex(isPlanApprovedCall);
|
|
expect(planApprovedIdx).toBeGreaterThanOrEqual(0);
|
|
|
|
// markPlanReferenceSent fires on the dispatch path so the executor's first
|
|
// turn doesn't double-inject the plan reference (it was just dispatched
|
|
// inside the synthetic prompt).
|
|
expect(markSentSpy).toHaveBeenCalledTimes(1);
|
|
});
|
|
|
|
it("Approve and compact context: cancelled outcome skips plan-approved dispatch", async () => {
|
|
// Mock `handleCompactCommand` to surface the "cancelled" outcome directly.
|
|
// (Testing the consumer — `#approvePlan`'s outcome handling — at the
|
|
// CompactionOutcome boundary; the underlying executeCompaction → sentinel
|
|
// classification path is producer-layer and not under T3's contract.)
|
|
const planFilePath = "local://PLAN.md";
|
|
const finalPlanFilePath = "local://APPROVED.md";
|
|
const resolvedPlanPath = resolveLocalUrlToPath(planFilePath, {
|
|
getArtifactsDir: () => session.sessionManager.getArtifactsDir(),
|
|
getSessionId: () => session.sessionManager.getSessionId(),
|
|
});
|
|
await Bun.write(resolvedPlanPath, "# Plan\n\nCancel mid-compact.");
|
|
|
|
mode.planModeEnabled = true;
|
|
mode.planModePlanFilePath = planFilePath;
|
|
vi.spyOn(mode, "showHookSelector").mockResolvedValue("Approve and compact context");
|
|
vi.spyOn(mode, "handleCompactCommand").mockResolvedValue("cancelled");
|
|
const showWarningSpy = vi.spyOn(mode, "showWarning");
|
|
const setPlanRefSpy = vi.spyOn(session, "setPlanReferencePath");
|
|
const markSentSpy = vi.spyOn(session, "markPlanReferenceSent");
|
|
const promptSpy = vi.spyOn(session, "prompt").mockResolvedValue(undefined as never);
|
|
|
|
await mode.handleExitPlanModeTool({
|
|
planFilePath,
|
|
planExists: true,
|
|
title: "PLAN",
|
|
finalPlanFilePath,
|
|
});
|
|
|
|
// Operator was told the dispatch was deferred.
|
|
expect(showWarningSpy).toHaveBeenCalledWith(
|
|
expect.stringContaining("Plan approved, but compaction was cancelled"),
|
|
);
|
|
// Plan reference path was recorded so the session knows about the approved
|
|
// plan at its final destination …
|
|
expect(setPlanRefSpy).toHaveBeenCalledWith(finalPlanFilePath);
|
|
// … but markPlanReferenceSent was NOT called, so the next operator turn
|
|
// will inject the reference fresh via #buildPlanReferenceMessage. This is
|
|
// the load-bearing assertion that the cancel path leaves the executor
|
|
// with the plan in its first turn.
|
|
expect(markSentSpy).not.toHaveBeenCalled();
|
|
// And — the contract — the plan-approved synthetic prompt was NOT dispatched.
|
|
expect(promptSpy.mock.calls.some(isPlanApprovedCall)).toBe(false);
|
|
});
|
|
|
|
it("Approve and compact context: failed outcome still dispatches plan-approved (best-effort)", async () => {
|
|
// Mock `handleCompactCommand` to surface the "failed" outcome directly.
|
|
// Failure → approval intent stands → synthetic dispatch fires.
|
|
const planFilePath = "local://PLAN.md";
|
|
const finalPlanFilePath = "local://APPROVED.md";
|
|
const resolvedPlanPath = resolveLocalUrlToPath(planFilePath, {
|
|
getArtifactsDir: () => session.sessionManager.getArtifactsDir(),
|
|
getSessionId: () => session.sessionManager.getSessionId(),
|
|
});
|
|
await Bun.write(resolvedPlanPath, "# Plan\n\nFail mid-compact.");
|
|
|
|
mode.planModeEnabled = true;
|
|
mode.planModePlanFilePath = planFilePath;
|
|
vi.spyOn(mode, "showHookSelector").mockResolvedValue("Approve and compact context");
|
|
vi.spyOn(mode, "handleCompactCommand").mockResolvedValue("failed");
|
|
const markSentSpy = vi.spyOn(session, "markPlanReferenceSent");
|
|
const promptSpy = vi.spyOn(session, "prompt").mockResolvedValue(undefined as never);
|
|
|
|
await mode.handleExitPlanModeTool({
|
|
planFilePath,
|
|
planExists: true,
|
|
title: "PLAN",
|
|
finalPlanFilePath,
|
|
});
|
|
|
|
// Plan-approved synthetic prompt WAS dispatched despite the failure.
|
|
expect(promptSpy.mock.calls.some(isPlanApprovedCall)).toBe(true);
|
|
// markPlanReferenceSent fires on this dispatch path.
|
|
expect(markSentSpy).toHaveBeenCalledTimes(1);
|
|
});
|
|
it("Approve and compact context: setPlanReferencePath is pinned BEFORE compaction flushes the queue", async () => {
|
|
// Regression: handleCompactCommand internally awaits flushCompactionQueue,
|
|
// which can deliver a user-queued message back to the session. If
|
|
// setPlanReferencePath had not been called yet, that queued turn would
|
|
// hit #buildPlanReferenceMessage with the stale plan-mode path. Pin it
|
|
// before the compaction await.
|
|
const planFilePath = "local://PLAN.md";
|
|
const finalPlanFilePath = "local://APPROVED.md";
|
|
const resolvedPlanPath = resolveLocalUrlToPath(planFilePath, {
|
|
getArtifactsDir: () => session.sessionManager.getArtifactsDir(),
|
|
getSessionId: () => session.sessionManager.getSessionId(),
|
|
});
|
|
await Bun.write(resolvedPlanPath, "# Plan\n\nQueue race.");
|
|
|
|
mode.planModeEnabled = true;
|
|
mode.planModePlanFilePath = planFilePath;
|
|
vi.spyOn(mode, "showHookSelector").mockResolvedValue("Approve and compact context");
|
|
vi.spyOn(session, "prompt").mockResolvedValue(undefined as never);
|
|
|
|
const setPlanRefSpy = vi.spyOn(session, "setPlanReferencePath");
|
|
let planRefSetWhenCompactionRan = false;
|
|
vi.spyOn(mode, "handleCompactCommand").mockImplementation(async () => {
|
|
planRefSetWhenCompactionRan = setPlanRefSpy.mock.calls.some(call => call[0] === finalPlanFilePath);
|
|
return "ok";
|
|
});
|
|
|
|
await mode.handleExitPlanModeTool({
|
|
planFilePath,
|
|
planExists: true,
|
|
title: "PLAN",
|
|
finalPlanFilePath,
|
|
});
|
|
|
|
// The contract: by the time handleCompactCommand runs (and flushes the
|
|
// compaction queue inside), setPlanReferencePath has already pinned the
|
|
// approved plan path, so any user message queued during compaction is
|
|
// dispatched against the approved plan, not the plan-mode draft.
|
|
expect(planRefSetWhenCompactionRan).toBe(true);
|
|
});
|
|
});
|