fix(coding-agent): re-inject approved plan reference after compaction
After plan approval the executor delivers the plan-mode-reference exactly once and sets `#planReferenceSent = true`. Both compaction paths — `compact()` and `#runAutoCompaction()` — replace the conversation history that carried that reference but never cleared the flag, so `#buildPlanReferenceMessage()` short-circuited to null on every subsequent turn and the executor permanently lost the plan it was working on (exactly the long-session failure reported). Clear `#planReferenceSent` right after `replaceMessages()` in both paths so the next turn re-reads the plan from disk and re-injects it. The reset is a no-op for ordinary sessions: the default plan path (PLAN.md in session-local scratch) has no file on disk, so `#buildPlanReferenceMessage()` still returns null there. Adds a deterministic regression test (short-circuited compaction, mock stream) that fails before this change and passes after, plus a guard proving normal sessions get no spurious plan injection. Fixes #1246
This commit is contained in:
can1357
@@ -7543,6 +7543,10 @@ export class AgentSession {
|
||||
const newEntries = this.sessionManager.getEntries();
|
||||
const sessionContext = this.buildDisplaySessionContext();
|
||||
this.agent.replaceMessages(sessionContext.messages);
|
||||
// Compaction discarded the conversation history that carried the approved
|
||||
// plan reference. Clear the sent-flag so #buildPlanReferenceMessage re-reads
|
||||
// the plan from disk and re-injects it on the next turn (issue #1246).
|
||||
this.#planReferenceSent = false;
|
||||
this.#advisorRuntime?.reset();
|
||||
this.#syncTodoPhasesFromBranch();
|
||||
this.#closeCodexProviderSessionsForHistoryRewrite();
|
||||
@@ -9409,6 +9413,10 @@ export class AgentSession {
|
||||
const newEntries = this.sessionManager.getEntries();
|
||||
const sessionContext = this.buildDisplaySessionContext();
|
||||
this.agent.replaceMessages(sessionContext.messages);
|
||||
// Compaction discarded the conversation history that carried the approved
|
||||
// plan reference. Clear the sent-flag so #buildPlanReferenceMessage re-reads
|
||||
// the plan from disk and re-injects it on the next turn (issue #1246).
|
||||
this.#planReferenceSent = false;
|
||||
this.#advisorRuntime?.reset();
|
||||
this.#syncTodoPhasesFromBranch();
|
||||
this.#closeCodexProviderSessionsForHistoryRewrite();
|
||||
|
||||
@@ -0,0 +1,258 @@
|
||||
/**
|
||||
* Regression test for issue #1246: "Approved plan file invisible to executor
|
||||
* after compaction".
|
||||
*
|
||||
* After a plan is approved, the executor session delivers the plan reference
|
||||
* (`plan-mode-reference`) exactly once, then marks `#planReferenceSent = true`.
|
||||
* When auto-compaction later fires, it replaces the conversation history —
|
||||
* dropping the delivered reference — but never clears that flag, so
|
||||
* `#buildPlanReferenceMessage()` short-circuits to `null` forever and the
|
||||
* executor permanently loses the plan it was working on.
|
||||
*
|
||||
* Contract: the auto-continuation turn that runs immediately after compaction
|
||||
* MUST carry the approved plan reference again (re-read from disk).
|
||||
*/
|
||||
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from "bun:test";
|
||||
import * as fs from "node:fs";
|
||||
import * as path from "node:path";
|
||||
import { Agent, type AgentMessage } from "@oh-my-pi/pi-agent-core";
|
||||
import * as compactionModule from "@oh-my-pi/pi-agent-core/compaction";
|
||||
import type { TextContent } from "@oh-my-pi/pi-ai";
|
||||
import { AssistantMessageEventStream } from "@oh-my-pi/pi-ai/utils/event-stream";
|
||||
import { getBundledModel } from "@oh-my-pi/pi-catalog/models";
|
||||
// NOTE: coding-agent modules are imported via relative ../src paths (not the
|
||||
// @oh-my-pi/pi-coding-agent alias) because in this worktree node_modules is a
|
||||
// symlink to the primary checkout, so the package alias would resolve to the
|
||||
// primary copy and bypass the fix under test. Cross-package deps stay on their
|
||||
// aliases — the worktree's agent-session imports them the same way.
|
||||
import { ModelRegistry } from "../src/config/model-registry";
|
||||
import { Settings } from "../src/config/settings";
|
||||
import { resolveLocalUrlToPath } from "../src/internal-urls";
|
||||
import { AgentSession } from "../src/session/agent-session";
|
||||
import { AuthStorage } from "../src/session/auth-storage";
|
||||
import { convertToLlm } from "../src/session/messages";
|
||||
import { SessionManager } from "../src/session/session-manager";
|
||||
import { TempDir } from "@oh-my-pi/pi-utils";
|
||||
|
||||
const CONTINUE_MARKER = "Resume work on the user's most recent intent";
|
||||
|
||||
type ObservedPromptCall = { messageTexts: string[] };
|
||||
|
||||
type Harness = {
|
||||
session: AgentSession;
|
||||
sessionManager: SessionManager;
|
||||
observedCalls: ObservedPromptCall[];
|
||||
waitForCall: (predicate: (call: ObservedPromptCall) => boolean) => Promise<ObservedPromptCall>;
|
||||
};
|
||||
|
||||
function isTextContentBlock(value: unknown): value is TextContent {
|
||||
if (!value || typeof value !== "object") return false;
|
||||
return (value as TextContent).type === "text" && typeof (value as TextContent).text === "string";
|
||||
}
|
||||
|
||||
function getMessageText(message: AgentMessage): string {
|
||||
if (!("content" in message)) return "";
|
||||
if (typeof message.content === "string") return message.content;
|
||||
if (!Array.isArray(message.content)) return "";
|
||||
return message.content
|
||||
.filter(isTextContentBlock)
|
||||
.map(content => content.text)
|
||||
.join("\n");
|
||||
}
|
||||
|
||||
function createAssistantResponse(text: string) {
|
||||
return {
|
||||
role: "assistant" as const,
|
||||
content: [{ type: "text" as const, text }],
|
||||
api: "anthropic-messages" as const,
|
||||
provider: "anthropic" as const,
|
||||
model: "claude-sonnet-4-5",
|
||||
usage: {
|
||||
input: 0,
|
||||
output: 0,
|
||||
cacheRead: 0,
|
||||
cacheWrite: 0,
|
||||
totalTokens: 0,
|
||||
cost: { input: 0, output: 0, cacheRead: 0, cacheWrite: 0, total: 0 },
|
||||
},
|
||||
stopReason: "stop" as const,
|
||||
timestamp: Date.now(),
|
||||
};
|
||||
}
|
||||
|
||||
/** Short-circuit the LLM summary so compaction completes without a network call. */
|
||||
function stubCompaction(): void {
|
||||
vi.spyOn(compactionModule, "compact").mockImplementation(async preparation => ({
|
||||
summary: "compacted",
|
||||
shortSummary: undefined,
|
||||
firstKeptEntryId: preparation.firstKeptEntryId,
|
||||
tokensBefore: preparation.tokensBefore,
|
||||
details: {},
|
||||
}));
|
||||
}
|
||||
|
||||
/** Emit a high-usage assistant turn to drive threshold (context-full) auto-compaction. */
|
||||
function emitHighUsageTurn(session: AgentSession): void {
|
||||
const assistantMsg = {
|
||||
role: "assistant" as const,
|
||||
content: [{ type: "text" as const, text: "Done." }],
|
||||
api: "anthropic-messages" as const,
|
||||
provider: "anthropic" as const,
|
||||
model: "claude-sonnet-4-5",
|
||||
stopReason: "stop" as const,
|
||||
usage: {
|
||||
input: 190_000,
|
||||
output: 1_000,
|
||||
cacheRead: 0,
|
||||
cacheWrite: 0,
|
||||
totalTokens: 191_000,
|
||||
cost: { input: 0, output: 0, cacheRead: 0, cacheWrite: 0, total: 0 },
|
||||
},
|
||||
timestamp: Date.now(),
|
||||
};
|
||||
session.agent.emitExternalEvent({ type: "message_end", message: assistantMsg });
|
||||
session.agent.emitExternalEvent({ type: "agent_end", messages: [assistantMsg] });
|
||||
}
|
||||
|
||||
describe("AgentSession approved-plan reference re-injection after compaction (issue #1246)", () => {
|
||||
let tempDir: TempDir;
|
||||
const cleanups: Array<() => Promise<void>> = [];
|
||||
|
||||
beforeEach(() => {
|
||||
tempDir = TempDir.createSync("@pi-agent-session-plan-ref-compaction-");
|
||||
cleanups.length = 0;
|
||||
});
|
||||
|
||||
afterEach(async () => {
|
||||
for (const cleanup of cleanups) await cleanup();
|
||||
cleanups.length = 0;
|
||||
tempDir.removeSync();
|
||||
vi.restoreAllMocks();
|
||||
});
|
||||
|
||||
async function createHarness(): Promise<Harness> {
|
||||
const observedCalls: ObservedPromptCall[] = [];
|
||||
const waiters: Array<{
|
||||
predicate: (call: ObservedPromptCall) => boolean;
|
||||
resolve: (call: ObservedPromptCall) => void;
|
||||
}> = [];
|
||||
|
||||
const model = getBundledModel("anthropic", "claude-sonnet-4-5");
|
||||
if (!model) throw new Error("Expected claude-sonnet-4-5 model to exist");
|
||||
|
||||
const authStorage = await AuthStorage.create(path.join(tempDir.path(), `testauth-${cleanups.length}.db`));
|
||||
authStorage.setRuntimeApiKey("anthropic", "test-key");
|
||||
const modelRegistry = new ModelRegistry(authStorage, path.join(tempDir.path(), `models-${cleanups.length}.yml`));
|
||||
const settings = Settings.isolated({
|
||||
"compaction.enabled": true,
|
||||
"compaction.autoContinue": true,
|
||||
"compaction.strategy": "context-full",
|
||||
"task.eager": "default",
|
||||
"todo.enabled": false,
|
||||
"todo.eager": "default",
|
||||
"todo.reminders": false,
|
||||
});
|
||||
const sessionManager = SessionManager.inMemory(tempDir.path());
|
||||
|
||||
let session: AgentSession;
|
||||
const agent = new Agent({
|
||||
getApiKey: () => "test-key",
|
||||
initialState: { model, systemPrompt: ["Test"], tools: [], messages: [] },
|
||||
convertToLlm,
|
||||
getToolChoice: () => session?.nextToolChoice(),
|
||||
streamFn: (_model, context) => {
|
||||
observedCalls.push({ messageTexts: context.messages.map(message => getMessageText(message)) });
|
||||
const call = observedCalls[observedCalls.length - 1];
|
||||
for (let i = waiters.length - 1; i >= 0; i--) {
|
||||
const waiter = waiters[i];
|
||||
if (waiter?.predicate(call)) {
|
||||
waiter.resolve(call);
|
||||
waiters.splice(i, 1);
|
||||
}
|
||||
}
|
||||
const response = createAssistantResponse("done");
|
||||
const stream = new AssistantMessageEventStream();
|
||||
queueMicrotask(() => {
|
||||
stream.push({ type: "start", partial: response });
|
||||
stream.push({ type: "done", reason: "stop", message: response });
|
||||
});
|
||||
return stream;
|
||||
},
|
||||
});
|
||||
|
||||
session = new AgentSession({ agent, sessionManager, settings, modelRegistry });
|
||||
|
||||
const waitForCall = (predicate: (call: ObservedPromptCall) => boolean) => {
|
||||
const existing = observedCalls.find(predicate);
|
||||
if (existing) return Promise.resolve(existing);
|
||||
const { promise, resolve } = Promise.withResolvers<ObservedPromptCall>();
|
||||
waiters.push({ predicate, resolve });
|
||||
return promise;
|
||||
};
|
||||
|
||||
cleanups.push(async () => {
|
||||
await session.dispose();
|
||||
authStorage.close();
|
||||
});
|
||||
return { session, sessionManager, observedCalls, waitForCall };
|
||||
}
|
||||
|
||||
/** Write a plan file to the session-scoped local root the executor reads from. */
|
||||
function writePlanFile(sessionManager: SessionManager, url: string, content: string): void {
|
||||
const resolved = resolveLocalUrlToPath(url, {
|
||||
getArtifactsDir: () => sessionManager.getArtifactsDir(),
|
||||
getSessionId: () => sessionManager.getSessionId(),
|
||||
});
|
||||
fs.mkdirSync(path.dirname(resolved), { recursive: true });
|
||||
fs.writeFileSync(resolved, content);
|
||||
}
|
||||
|
||||
it("re-injects the approved plan reference on the auto-continuation turn", async () => {
|
||||
const { session, sessionManager, observedCalls, waitForCall } = await createHarness();
|
||||
|
||||
const planUrl = "local://approved-plan.md";
|
||||
const planMarker = "PHASE-6-ARCHITECTURE-PLAN-MARKER";
|
||||
writePlanFile(sessionManager, planUrl, `# Approved Plan\n\n${planMarker}\n`);
|
||||
|
||||
// Simulate the executor: plan mode is already exited and the plan reference
|
||||
// has already been delivered once (the in-history copy that compaction drops).
|
||||
session.setPlanReferencePath(planUrl);
|
||||
session.markPlanReferenceSent();
|
||||
|
||||
stubCompaction();
|
||||
|
||||
// First executor turn: reference already sent, so it is NOT re-delivered here.
|
||||
await session.prompt("continue executing the approved plan");
|
||||
const firstCall = observedCalls[0];
|
||||
expect(firstCall).toBeDefined();
|
||||
expect(firstCall.messageTexts.some(text => text.includes(planMarker))).toBe(false);
|
||||
|
||||
// Auto-compaction fires, replacing history (dropping the delivered reference),
|
||||
// then schedules the auto-continuation turn.
|
||||
emitHighUsageTurn(session);
|
||||
const continuation = await waitForCall(call =>
|
||||
call.messageTexts.some(text => text.includes(CONTINUE_MARKER)),
|
||||
);
|
||||
|
||||
// The post-compaction continuation MUST carry the plan reference again.
|
||||
expect(continuation.messageTexts.some(text => text.includes(planMarker))).toBe(true);
|
||||
expect(continuation.messageTexts.some(text => text.includes(`<plan path="${planUrl}">`))).toBe(true);
|
||||
});
|
||||
|
||||
// Blast-radius guard: clearing the flag on every compaction must NOT start
|
||||
// injecting a plan reference into ordinary (non-plan) sessions, where the
|
||||
// default `local://PLAN.md` path has no file on disk.
|
||||
it("does not inject a plan reference after compaction when no plan file exists", async () => {
|
||||
const { session, waitForCall } = await createHarness();
|
||||
stubCompaction();
|
||||
|
||||
await session.prompt("do some ordinary work");
|
||||
emitHighUsageTurn(session);
|
||||
const continuation = await waitForCall(call =>
|
||||
call.messageTexts.some(text => text.includes(CONTINUE_MARKER)),
|
||||
);
|
||||
|
||||
expect(continuation.messageTexts.some(text => text.includes("## Existing Plan"))).toBe(false);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user