diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 220bf8ad0..f0d002c8a 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Fixed plan-mode "Approve and compact context" leaking the internal plan-distillation prompt through the public `customInstructions` field on the `session_before_compact` extension hook. The guidance now rides through a private `CompactOptions.internalGuidance` channel that native summarization sees but extensions do not, so hooks treating `customInstructions` as user focus no longer produce query-biased compactions of plan-mode boilerplate ([#4359](https://github.com/can1357/oh-my-pi/issues/4359)). + ## [16.3.3] - 2026-07-02 ### Breaking Changes diff --git a/packages/coding-agent/src/extensibility/extensions/types.ts b/packages/coding-agent/src/extensibility/extensions/types.ts index b93535142..305c0b384 100644 --- a/packages/coding-agent/src/extensibility/extensions/types.ts +++ b/packages/coding-agent/src/extensibility/extensions/types.ts @@ -294,6 +294,18 @@ export interface CompactOptions { * subcommands: `soft` | `remote` | `snapcompact`). Omitted = configured behavior. */ mode?: CompactMode; + /** + * Internal summarizer guidance — piped only to native summarization, never + * exposed as `customInstructions` on the `session_before_compact` extension + * hook. Used by plan-mode "Approve and compact context" so extensions that + * treat `customInstructions` as user focus don't mistake plan-mode + * boilerplate for the operator's intent (issue #4359). + * + * When both `customInstructions` and `internalGuidance` are set, the + * summarizer uses `internalGuidance`; the hook still sees only the public + * `customInstructions`. + */ + internalGuidance?: string; } /** diff --git a/packages/coding-agent/src/modes/controllers/command-controller.ts b/packages/coding-agent/src/modes/controllers/command-controller.ts index 1dbc0caf8..1bd82c0bd 100644 --- a/packages/coding-agent/src/modes/controllers/command-controller.ts +++ b/packages/coding-agent/src/modes/controllers/command-controller.ts @@ -1124,6 +1124,7 @@ export class CommandController { customInstructions?: string, mode?: CompactMode, beforeFlush?: (outcome: CompactionOutcome) => void | Promise, + internalGuidance?: string, ): Promise { const entries = this.ctx.sessionManager.getEntries(); const messageCount = entries.filter(e => e.type === "message").length; @@ -1133,6 +1134,15 @@ export class CommandController { return "ok"; } + // `internalGuidance` is a private summarizer directive (plan-mode + // "Approve and compact context") that MUST stay off the public + // `customInstructions` channel of the `session_before_compact` extension + // hook — extensions treat that field as user focus and would otherwise + // bias the summary toward the plan boilerplate (issue #4359). Ride it + // through as a CompactOptions field instead. + if (internalGuidance) { + return this.executeCompaction({ internalGuidance, ...(mode ? { mode } : {}) }, false, beforeFlush, mode); + } return this.executeCompaction(customInstructions, false, beforeFlush, mode); } diff --git a/packages/coding-agent/src/modes/interactive-mode.ts b/packages/coding-agent/src/modes/interactive-mode.ts index a36defebc..3880e99c1 100644 --- a/packages/coding-agent/src/modes/interactive-mode.ts +++ b/packages/coding-agent/src/modes/interactive-mode.ts @@ -2606,8 +2606,16 @@ export class InteractiveMode implements InteractiveModeContext { // the try/finally is idempotent and kept for the !compactBeforeExecute // branch. this.session.setPlanReferencePath(options.planFilePath); - compactOutcome = await this.handleCompactCommand(compactionPrompt, undefined, outcome => - this.#applyDeferredPlanModelTransition(outcome, options.executionModel), + // Ride the plan-mode distillation prompt through as `internalGuidance` + // so it reaches native summarization without leaking into the public + // `customInstructions` channel on `session_before_compact` — extensions + // there treat that field as user focus and would query-bias the + // summary toward the plan boilerplate (issue #4359). + compactOutcome = await this.handleCompactCommand( + undefined, + undefined, + outcome => this.#applyDeferredPlanModelTransition(outcome, options.executionModel), + compactionPrompt, ); } } finally { @@ -3900,8 +3908,9 @@ export class InteractiveMode implements InteractiveModeContext { customInstructions?: string, mode?: CompactMode, beforeFlush?: (outcome: CompactionOutcome) => void | Promise, + internalGuidance?: string, ): Promise { - return this.#commandController.handleCompactCommand(customInstructions, mode, beforeFlush); + return this.#commandController.handleCompactCommand(customInstructions, mode, beforeFlush, internalGuidance); } handleHandoffCommand(customInstructions?: string): Promise { diff --git a/packages/coding-agent/src/session/agent-session.ts b/packages/coding-agent/src/session/agent-session.ts index f271f21bf..64910e9b0 100644 --- a/packages/coding-agent/src/session/agent-session.ts +++ b/packages/coding-agent/src/session/agent-session.ts @@ -9417,7 +9417,11 @@ export class AgentSession { // Modes that produce no LLM summary (snapcompact) have nothing to focus. // Reject focus text loudly so programmatic callers don't silently lose // instructions (the slash path pre-validates via parseCompactArgs). - if (compactMode?.rejectsFocus && customInstructions) { + // `internalGuidance` counts the same way — plan-mode approval never + // combines with a rejects-focus mode, but reject early if a caller ever + // wires it up so we don't silently drop the directive on the snapcompact + // fallback (issue #4359). + if (compactMode?.rejectsFocus && (customInstructions || options?.internalGuidance)) { throw new Error(`/compact ${compactMode.name} does not take focus instructions.`); } const compactionAbortController = new AbortController(); @@ -9498,12 +9502,16 @@ export class AgentSession { const compactionPrep = await this.#prepareCompactionFromHooks(preparation, hookCompaction); - // Strategy honored on manual /compact too. Custom instructions imply a - // directed LLM summary; a text-only model cannot read snapcompact frames. - // When snapcompact itself was requested, fail locally instead of silently + // Strategy honored on manual /compact too. Custom instructions (public + // user focus OR internal plan-mode guidance) imply a directed LLM + // summary; a text-only model cannot read snapcompact frames. When + // snapcompact itself was requested, fail locally instead of silently // converting the "no LLM call" path into a provider-backed summary. const wantsSnapcompact = - compactionPrep.kind !== "fromHook" && effectiveSettings.strategy === "snapcompact" && !customInstructions; + compactionPrep.kind !== "fromHook" && + effectiveSettings.strategy === "snapcompact" && + !customInstructions && + !options?.internalGuidance; const snapcompactReady = wantsSnapcompact; const snapcompactShapeSetting = this.settings.get("snapcompact.shape"); let snapcompactShape: snapcompact.Shape | undefined; @@ -9636,7 +9644,7 @@ export class AgentSession { try { const result = await this.#compactWithFallbackModel( preparation, - customInstructions, + options?.internalGuidance ?? customInstructions, compactionAbortController.signal, { promptOverride: this.#obfuscateTextForProvider(compactionPrep.hookPrompt), diff --git a/packages/coding-agent/test/agent-session-plan-compact-hook-instructions.test.ts b/packages/coding-agent/test/agent-session-plan-compact-hook-instructions.test.ts new file mode 100644 index 000000000..77ee77b05 --- /dev/null +++ b/packages/coding-agent/test/agent-session-plan-compact-hook-instructions.test.ts @@ -0,0 +1,224 @@ +/** + * Regression test for issue #4359: "Keep plan compaction guidance out of hook + * custom instructions". + * + * The plan-approval compaction path used to route the internal + * `plan-mode-compact-instructions` prompt through the public + * `customInstructions` argument of {@link AgentSession.compact}, and from there + * into the `session_before_compact` extension hook. Extensions that treat that + * field as "user focus" — e.g. to bias a query-focused summary — would then + * see plan-mode boilerplate instead of the operator's intent and produce + * query-biased compactions. + * + * Contract: + * - Plan-mode compaction MUST call {@link AgentSession.compact} with + * `customInstructions: undefined` and pass the guidance via + * `CompactOptions.internalGuidance` instead. + * - The `session_before_compact` hook event MUST see + * `customInstructions: undefined` for internal-guidance compactions. + * - The native summarizer (invoked via `@oh-my-pi/pi-agent-core/compaction`) + * MUST still receive the plan guidance so the summary is directed. + */ + +import { afterEach, beforeEach, describe, expect, it, vi } from "bun:test"; +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"; +import { TempDir } from "@oh-my-pi/pi-utils"; +import { ModelRegistry } from "../src/config/model-registry"; +import { Settings } from "../src/config/settings"; +import type { SessionBeforeCompactEvent } from "../src/extensibility/shared-events"; +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"; + +type Harness = { + session: AgentSession; + sessionManager: SessionManager; + beforeCompactEvents: SessionBeforeCompactEvent[]; + summarizerCalls: Array<{ customInstructions: string | undefined }>; +}; + +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(), + }; +} + +describe("AgentSession plan-mode compaction hook contract (issue #4359)", () => { + let tempDir: TempDir; + const cleanups: Array<() => Promise> = []; + + beforeEach(() => { + tempDir = TempDir.createSync("@pi-agent-session-plan-compact-hook-"); + cleanups.length = 0; + }); + + afterEach(async () => { + for (const cleanup of cleanups) await cleanup(); + cleanups.length = 0; + tempDir.removeSync(); + vi.restoreAllMocks(); + }); + + async function createHarness(): Promise { + 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.strategy": "context-full", + // Aggressive keep-recent budget so the small seeded conversation still + // yields a non-empty messagesToSummarize window (prepareCompaction + // otherwise short-circuits with "Nothing to compact"). + "compaction.keepRecentTokens": 1, + "todo.enabled": false, + "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?.nextToolChoiceDirective(), + streamFn: () => { + 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; + }, + }); + + // Stub the underlying LLM summary so compaction completes without a network + // call, and capture what customInstructions the native summarizer received. + const summarizerCalls: Array<{ customInstructions: string | undefined }> = []; + vi.spyOn(compactionModule, "compact").mockImplementation( + async (preparation, _model, _resolver, customInstructions) => { + summarizerCalls.push({ customInstructions }); + return { + summary: "compacted", + shortSummary: undefined, + firstKeptEntryId: preparation.firstKeptEntryId, + tokensBefore: preparation.tokensBefore, + details: {}, + }; + }, + ); + + // Minimal ExtensionRunner shim: AgentSession only calls hasHandlers() + + // emit() on it. Casting keeps the test focused on the hook payload. + const beforeCompactEvents: SessionBeforeCompactEvent[] = []; + const extensionRunner = { + hasHandlers: (type: string) => type === "session_before_compact", + emit: async (event: { type: string } & Record) => { + if (event.type === "session_before_compact") { + beforeCompactEvents.push(event as unknown as SessionBeforeCompactEvent); + } + return undefined; + }, + // AgentSession.#promptWithMessage always awaits this before agent_start + // when an extensionRunner is present; the shim mirrors the no-op path. + emitBeforeAgentStart: async () => undefined, + }; + session = new AgentSession({ + agent, + sessionManager, + settings, + modelRegistry, + extensionRunner: extensionRunner as never, + }); + + // Seed enough conversation so prepareCompaction has something to summarize. + await session.prompt("plan out the change"); + await session.prompt("here is the discovery I did while planning"); + + cleanups.push(async () => { + await session.dispose(); + authStorage.close(); + }); + return { session, sessionManager, beforeCompactEvents, summarizerCalls }; + } + + it("routes internalGuidance to the summarizer without exposing it to session_before_compact", async () => { + const { session, beforeCompactEvents, summarizerCalls } = await createHarness(); + const planGuidance = "Preparing to execute the approved plan. You MUST distill the plan-mode discussion."; + + await session.compact(undefined, { internalGuidance: planGuidance }); + + // Public hook channel: never carries internal plan guidance. + expect(beforeCompactEvents.length).toBe(1); + expect(beforeCompactEvents[0]?.customInstructions).toBeUndefined(); + + // Native summarizer still receives the guidance so the summary is directed. + expect(summarizerCalls.length).toBe(1); + expect(summarizerCalls[0]?.customInstructions).toBe(planGuidance); + }); + + it("still forwards a user /compact focus verbatim to the hook", async () => { + const { session, beforeCompactEvents, summarizerCalls } = await createHarness(); + const userFocus = "focus on the auth refactor"; + + await session.compact(userFocus); + + // User focus is public: extensions see it (they may interpret it as + // intent). + expect(beforeCompactEvents.length).toBe(1); + expect(beforeCompactEvents[0]?.customInstructions).toBe(userFocus); + expect(summarizerCalls[0]?.customInstructions).toBe(userFocus); + }); + + it("prefers internalGuidance over customInstructions in the summarizer when both are set", async () => { + // Belt-and-suspenders: internal guidance always wins for the summary so a + // caller cannot accidentally leak the plan prompt by also passing a user + // focus string, and hook visibility is unchanged. + const { session, beforeCompactEvents, summarizerCalls } = await createHarness(); + const userFocus = "focus on the auth refactor"; + const planGuidance = "distill the plan-mode discussion"; + + await session.compact(userFocus, { internalGuidance: planGuidance }); + + expect(beforeCompactEvents[0]?.customInstructions).toBe(userFocus); + expect(summarizerCalls[0]?.customInstructions).toBe(planGuidance); + }); +}); diff --git a/packages/coding-agent/test/interactive-mode-plan-review.test.ts b/packages/coding-agent/test/interactive-mode-plan-review.test.ts index b7f14a199..e24b51be3 100644 --- a/packages/coding-agent/test/interactive-mode-plan-review.test.ts +++ b/packages/coding-agent/test/interactive-mode-plan-review.test.ts @@ -1203,11 +1203,17 @@ describe("InteractiveMode plan review rendering", () => { title: "PLAN", }); - // Compaction was run with the rendered planning-specific custom instruction. + // Plan-mode compaction rides through as `internalGuidance` (arg 4) so it + // reaches native summarization without leaking into the public + // `customInstructions` channel of the `session_before_compact` hook — + // extensions there treat that field as user focus (issue #4359). expect(compactSpy).toHaveBeenCalledTimes(1); - const [compactInstruction] = compactSpy.mock.calls[0]!; - expect(typeof compactInstruction).toBe("string"); - expect(compactInstruction as string).toContain(planFilePath); + const [customInstructions, mode_, beforeFlush, internalGuidance] = compactSpy.mock.calls[0]!; + expect(customInstructions).toBeUndefined(); + expect(mode_).toBeUndefined(); + expect(typeof beforeFlush).toBe("function"); + expect(typeof internalGuidance).toBe("string"); + expect(internalGuidance as string).toContain(planFilePath); // Plan-approved synthetic prompt was dispatched. const planApprovedIdx = promptSpy.mock.calls.findIndex(isPlanApprovedCall);