From 747aa65bd67962c97222c3c7a8882c2518ea2073 Mon Sep 17 00:00:00 2001 From: Robin Newhouse Date: Fri, 19 Jun 2026 12:06:24 -0700 Subject: [PATCH] Address compaction sidecar review feedback --- .../interactive/session-runtime.test.ts | 53 +++++++---- .../runtime/interactive/session-runtime.ts | 93 ++++++++++--------- apps/cli/src/tui/utils/compaction-status.ts | 2 +- sdk/packages/agents/README.md | 24 +++-- .../hub/server/handlers/session-handlers.ts | 10 +- .../runtime/host/local-runtime-host.test.ts | 76 +++++++++++++++ .../src/runtime/host/local-runtime-host.ts | 13 ++- 7 files changed, 188 insertions(+), 83 deletions(-) diff --git a/apps/cli/src/runtime/interactive/session-runtime.test.ts b/apps/cli/src/runtime/interactive/session-runtime.test.ts index 5b76d44e88..3407743531 100644 --- a/apps/cli/src/runtime/interactive/session-runtime.test.ts +++ b/apps/cli/src/runtime/interactive/session-runtime.test.ts @@ -83,12 +83,12 @@ function createConfig(): Config { }; } -function createChatCommandState(): ChatCommandState { +function createChatCommandState(config = createConfig()): ChatCommandState { return { - enableTools: true, - autoApproveTools: true, - cwd: "/tmp/project", - workspaceRoot: "/tmp/project", + enableTools: config.enableTools, + autoApproveTools: config.defaultToolAutoApprove, + cwd: config.cwd, + workspaceRoot: config.workspaceRoot?.trim() || config.cwd, }; } @@ -132,7 +132,7 @@ function makeSwitchToActModeTool(): AgentTool { function makeManager() { let startCount = 0; - const start = vi.fn(async () => { + const start = vi.fn(async (_input?: unknown) => { startCount += 1; const sessionId = `session-${startCount}`; return { @@ -192,18 +192,19 @@ function deferred() { async function makeRuntime( manager: ReturnType, options: { + config?: Config; resumeSessionId?: string; resolveToolPolicy?: (toolName: string) => Config["toolPolicies"][string]; } = {}, ) { createCliCoreMock.mockResolvedValue(manager); - const config = createConfig(); + const config = options.config ?? createConfig(); const { createInteractiveSessionRuntime } = await importRuntime(); return createInteractiveSessionRuntime({ config, providerSettingsManager: createProviderSettingsManager(), resumeSessionId: options.resumeSessionId, - chatCommandState: createChatCommandState(), + chatCommandState: createChatCommandState(config), requestToolApproval: async ( _request: ToolApprovalRequest, ): Promise => ({ approved: true }), @@ -285,6 +286,7 @@ describe("createInteractiveSessionRuntime", () => { providerSettingsManager: createProviderSettingsManager(), chatCommandState: createChatCommandState(), requestToolApproval: vi.fn(), + resolveToolPolicy: () => ({ autoApprove: true }), askQuestionRef: { current: null }, resolveMistakeLimitDecision: undefined, switchToActModeTool: {} as never, @@ -358,6 +360,7 @@ describe("createInteractiveSessionRuntime", () => { providerSettingsManager: createProviderSettingsManager(), chatCommandState: createChatCommandState(), requestToolApproval: vi.fn(), + resolveToolPolicy: () => ({ autoApprove: true }), askQuestionRef: { current: null }, resolveMistakeLimitDecision: undefined, switchToActModeTool: {} as never, @@ -377,6 +380,21 @@ describe("createInteractiveSessionRuntime", () => { expect(manager.updateSessionCompactionState).not.toHaveBeenCalled(); }); + it("rejects manual compact when compaction is disabled", async () => { + const manager = makeManager(); + const config = createConfig(); + config.compaction = { enabled: false }; + const runtime = await makeRuntime(manager, { config }); + + await runtime.ensureReady(); + + await expect(runtime.compactCurrentSession()).rejects.toThrow( + "compaction is off", + ); + expect(compactInteractiveMessagesMock).not.toHaveBeenCalled(); + expect(manager.updateSessionCompactionState).not.toHaveBeenCalled(); + }); + it("carries compacted working context across mode-switch restarts", async () => { const firstSessionId = "sess-mode-before"; const secondSessionId = "sess-mode-after"; @@ -439,6 +457,7 @@ describe("createInteractiveSessionRuntime", () => { providerSettingsManager: createProviderSettingsManager(), chatCommandState: createChatCommandState(), requestToolApproval: vi.fn(), + resolveToolPolicy: () => ({ autoApprove: true }), askQuestionRef: { current: null }, resolveMistakeLimitDecision: undefined, switchToActModeTool: {} as never, @@ -459,17 +478,16 @@ describe("createInteractiveSessionRuntime", () => { const restartInput = manager.start.mock.calls[1]?.[0]; expect(restartInput).toMatchObject({ initialMessages: messages, - }); - expect(restartInput).not.toHaveProperty("initialCompactionState"); - expect(manager.updateSessionCompactionState).toHaveBeenCalledWith( - secondSessionId, - expect.objectContaining({ - conversation_id: secondSessionId, + initialCompactionState: expect.objectContaining({ source_message_count: messages.length, messages: [summaryMessage, tailMessage], system_prompt: "compacted system", }), + }); + expect(restartInput.initialCompactionState).not.toHaveProperty( + "conversation_id", ); + expect(manager.updateSessionCompactionState).not.toHaveBeenCalled(); expect(runtime.getActiveSessionId()).toBe(secondSessionId); }); @@ -505,6 +523,7 @@ describe("createInteractiveSessionRuntime", () => { providerSettingsManager: createProviderSettingsManager(), chatCommandState: createChatCommandState(), requestToolApproval: vi.fn(), + resolveToolPolicy: () => ({ autoApprove: true }), askQuestionRef: { current: null }, resolveMistakeLimitDecision: undefined, switchToActModeTool: {} as never, @@ -536,13 +555,13 @@ describe("createInteractiveSessionRuntime", () => { const upstreamBeforeTool = vi.fn(async () => ({ input: { text: "updated" }, })); - mockCreateRuntimeHooks.mockReturnValueOnce({ + createRuntimeHooksMock.mockReturnValueOnce({ hooks: { beforeTool: upstreamBeforeTool, }, shutdown: vi.fn(async () => {}), }); - const runtime = makeRuntime(manager, { + const runtime = await makeRuntime(manager, { resolveToolPolicy: (toolName) => ({ autoApprove: toolName === "echo", }), @@ -626,6 +645,7 @@ describe("createInteractiveSessionRuntime", () => { resumeSessionId: "resumed-session", chatCommandState: createChatCommandState(), requestToolApproval: vi.fn(), + resolveToolPolicy: () => ({ autoApprove: true }), askQuestionRef: { current: null }, resolveMistakeLimitDecision: undefined, switchToActModeTool: {} as never, @@ -700,6 +720,7 @@ describe("createInteractiveSessionRuntime", () => { providerSettingsManager: createProviderSettingsManager(), chatCommandState: createChatCommandState(), requestToolApproval: vi.fn(), + resolveToolPolicy: () => ({ autoApprove: true }), askQuestionRef: { current: null }, resolveMistakeLimitDecision: undefined, switchToActModeTool: {} as never, diff --git a/apps/cli/src/runtime/interactive/session-runtime.ts b/apps/cli/src/runtime/interactive/session-runtime.ts index 905ce8698f..d68263a015 100644 --- a/apps/cli/src/runtime/interactive/session-runtime.ts +++ b/apps/cli/src/runtime/interactive/session-runtime.ts @@ -288,6 +288,25 @@ export function createInteractiveSessionRuntime(input: { return (await sessionManager.readMessages(activeSessionId)) ?? []; }; + const readCompactionState = async ( + sessionId: string, + ): Promise => { + const manager = sessionManager; + if (!manager) { + return undefined; + } + try { + return await manager.readSessionCompactionState(sessionId); + } catch (error) { + input.config.logger?.log?.("Failed to read session compaction state", { + sessionId, + error, + severity: "warn", + }); + return undefined; + } + }; + const recoverMissingActiveSession = async (error: unknown): Promise => { if (missingSessionRecoveryPromise) { return await missingSessionRecoveryPromise; @@ -322,12 +341,10 @@ export function createInteractiveSessionRuntime(input: { const readCurrentCompactionState = async (): Promise< SessionCompactionState | undefined > => { - if (!sessionManager || !activeSessionId) { + if (!activeSessionId) { return undefined; } - return await sessionManager - .readSessionCompactionState(activeSessionId) - .catch(() => undefined); + return await readCompactionState(activeSessionId); }; const stopCurrentSession = async (): Promise => { @@ -386,26 +403,17 @@ export function createInteractiveSessionRuntime(input: { const projectedMessages = compactionState ? projectSessionCompactionState(compactionState, messages) : undefined; - await restartWithMessages(messages); - if (!projectedMessages || !sessionManager || !activeSessionId) { - return; - } - const reanchoredCompactionState = createSessionCompactionState({ - sourceMessages: messages, - compactedMessages: projectedMessages, - conversationId: activeSessionId, - systemPrompt: compactionState?.system_prompt, - }); - const updated = await sessionManager.updateSessionCompactionState( - activeSessionId, - reanchoredCompactionState, + await restartWithMessages( + messages, + undefined, + projectedMessages + ? createSessionCompactionState({ + sourceMessages: messages, + compactedMessages: projectedMessages, + systemPrompt: compactionState?.system_prompt, + }) + : undefined, ); - if (!updated.updated) { - input.config.logger?.log?.( - "Skipped re-anchoring session compaction state after restart", - { sessionId: activeSessionId }, - ); - } }; const restartEmpty = async (): Promise => { @@ -519,9 +527,7 @@ export function createInteractiveSessionRuntime(input: { if (messages.length === 0) { throw new Error("Cannot fork an empty session."); } - const compactionState = await manager - .readSessionCompactionState(forkedFromSessionId) - .catch(() => undefined); + const compactionState = await readCompactionState(forkedFromSessionId); const projectedMessages = compactionState ? projectSessionCompactionState(compactionState, messages) : undefined; @@ -532,25 +538,17 @@ export function createInteractiveSessionRuntime(input: { sourceSession: sessionRecord, messages, }); - await startFreshSession(messages, forkMetadata); - if (projectedMessages && activeSessionId) { - const reanchoredCompactionState = createSessionCompactionState({ - sourceMessages: messages, - compactedMessages: projectedMessages, - conversationId: activeSessionId, - systemPrompt: compactionState?.system_prompt, - }); - const updated = await manager.updateSessionCompactionState( - activeSessionId, - reanchoredCompactionState, - ); - if (!updated.updated) { - input.config.logger?.log?.( - "Skipped re-anchoring session compaction state after fork", - { sessionId: activeSessionId }, - ); - } - } + await startFreshSession( + messages, + forkMetadata, + projectedMessages + ? createSessionCompactionState({ + sourceMessages: messages, + compactedMessages: projectedMessages, + systemPrompt: compactionState?.system_prompt, + }) + : undefined, + ); return { forkedFromSessionId, newSessionId: activeSessionId }; }; @@ -575,6 +573,11 @@ export function createInteractiveSessionRuntime(input: { workingContextMessagesAfter?: number; compacted: boolean; }> => { + if (input.config.compaction?.enabled === false) { + throw new Error( + "Cannot compact because compaction is off for this session.", + ); + } const manager = sessionManager; const sourceSessionId = activeSessionId; if (!manager || !sourceSessionId) { diff --git a/apps/cli/src/tui/utils/compaction-status.ts b/apps/cli/src/tui/utils/compaction-status.ts index ca5185c539..5d9b10f618 100644 --- a/apps/cli/src/tui/utils/compaction-status.ts +++ b/apps/cli/src/tui/utils/compaction-status.ts @@ -14,7 +14,7 @@ export function formatCompactionStatus( return "No compaction needed."; } if (typeof result.workingContextMessagesAfter === "number") { - return `Compacted working context to ${formatMessageCount(result.workingContextMessagesAfter)}; canonical history remains ${formatMessageCount(result.messagesAfter)}.`; + return `Compacted working context to ${formatMessageCount(result.workingContextMessagesAfter)}; saved history remains ${formatMessageCount(result.messagesAfter)}.`; } if (result.messagesBefore === result.messagesAfter) { return `Compacted context; message count stayed at ${formatMessageCount(result.messagesAfter)}.`; diff --git a/sdk/packages/agents/README.md b/sdk/packages/agents/README.md index d2afa25c3a..aa7fff78f8 100644 --- a/sdk/packages/agents/README.md +++ b/sdk/packages/agents/README.md @@ -206,39 +206,37 @@ new Agent({ For richer, host-side hook orchestration (15-stage `HookEngine`, subprocess-backed hooks, MCP extensions), use `@cline/core`. -### Request Projection with `prepareTurn` +### Preparing Requests with `prepareTurn` -`prepareTurn` is a request projection hook. It runs during turn preparation and -may return a different message list or system prompt for the next provider -request: +`prepareTurn` runs before messages are sent to the provider. It can rewrite the +messages or system prompt for the next request: ```text -canonical transcript +saved transcript | | turn preparation v prepareTurn | v -provider request +prepared provider request ``` Returned messages affect only the provider request for the current model call. -They do not replace the runtime's canonical transcript, are not persisted as -session history, and are not returned from `AgentRunResult.messages`. +They do not replace saved history and are not returned from +`AgentRunResult.messages`. ```text -prepareTurn returns projected messages +prepareTurn returns prepared messages | +--> provider request: yes - +--> canonical transcript: no - +--> persisted transcript: no + +--> saved transcript: no +--> AgentRunResult.messages: no ``` -This is intentionally different from a transcript rewrite. Hosts that need +This is intentionally different from changing saved history. Hosts that need durable redaction, normalization, or policy filtering must apply that change -before a message enters the agent's canonical transcript. +before a message enters the transcript. ### Plugins diff --git a/sdk/packages/core/src/hub/server/handlers/session-handlers.ts b/sdk/packages/core/src/hub/server/handlers/session-handlers.ts index b2dbe30c9c..68c4317c95 100644 --- a/sdk/packages/core/src/hub/server/handlers/session-handlers.ts +++ b/sdk/packages/core/src/hub/server/handlers/session-handlers.ts @@ -36,16 +36,16 @@ function getCapabilityOwnerClientId( ctx: HubTransportContext, sessionId: string, ): string | undefined { - // Compaction sidecar access is intentionally tied to live hub session - // ownership, not mutable persisted metadata. Sessions restored after the - // hub forgets their live owner must be recreated or backfilled before using - // these owner-scoped sidecar endpoints. + // Sidecar access follows the live hub owner, not persisted metadata clients + // can replay or edit. return ctx.sessionState.get(sessionId)?.createdByClientId; } function stripServerOwnedSessionMetadata( metadata: Record | undefined, ): Record | undefined { + // Clients may echo old records back through session.update; keep ownership + // on the live hub state only. if (!metadata || !(CAPABILITY_OWNER_METADATA_KEY in metadata)) { return metadata; } @@ -65,7 +65,7 @@ function authorizeSessionCompactionAccess(input: { return errorReply( input.envelope, "session_wrong_client", - `Session ${input.sessionId} has no authorized owner`, + `Session ${input.sessionId} has no owner`, ); } if (ownerClientId !== input.clientId) { diff --git a/sdk/packages/core/src/runtime/host/local-runtime-host.test.ts b/sdk/packages/core/src/runtime/host/local-runtime-host.test.ts index 5059a9eb4a..892323c213 100644 --- a/sdk/packages/core/src/runtime/host/local-runtime-host.test.ts +++ b/sdk/packages/core/src/runtime/host/local-runtime-host.test.ts @@ -4337,6 +4337,82 @@ describe("LocalRuntimeHost", () => { ); }); + it("binds unowned initial compaction state to the started session", async () => { + const sessionId = "sess-compaction-initial"; + const manifest = createManifest(sessionId); + const initialMessages: MessageWithMetadata[] = [ + { role: "user", content: "large source" }, + ]; + const initialCompactionState = createSessionCompactionState({ + sourceMessages: initialMessages, + compactedMessages: [{ role: "user", content: "summary" }], + updatedAt: "2026-01-01T00:00:00.000Z", + }); + const sessionService = { + ensureSessionsDir: vi.fn().mockReturnValue("/tmp/sessions"), + createRootSessionWithArtifacts: vi.fn().mockResolvedValue({ + manifestPath: "/tmp/manifest-compaction-initial.json", + messagesPath: "/tmp/messages-compaction-initial.json", + manifest, + }), + persistSessionMessages: vi.fn(), + persistSessionCompactionState: vi.fn(), + updateSessionStatus: vi.fn().mockResolvedValue({ updated: true }), + writeSessionManifest: vi.fn(), + listSessions: vi.fn().mockResolvedValue([]), + deleteSession: vi.fn().mockResolvedValue({ deleted: true }), + }; + const run = vi.fn().mockResolvedValue(createResult()); + const createAgent = vi.fn().mockReturnValue({ + run, + continue: vi.fn(), + abort: vi.fn(), + subscribeEvents: vi.fn().mockReturnValue(() => {}), + canStartRun: vi.fn().mockReturnValue(true), + getAgentId: vi.fn().mockReturnValue("agent-root-1"), + getConversationId: vi.fn().mockReturnValue(sessionId), + restore: vi.fn(), + shutdown: vi.fn().mockResolvedValue(undefined), + getMessages: vi.fn().mockReturnValue(initialMessages), + messages: initialMessages, + }); + const manager = new RuntimeHostUnderTest({ + distinctId, + sessionService: sessionService as never, + runtimeBuilder: { + build: vi.fn().mockReturnValue({ + tools: [], + shutdown: vi.fn(), + }), + }, + createAgent: createAgent as never, + }); + + await manager.startSession( + normalizeStartInput({ + config: createConfig({ + sessionId, + compaction: { + enabled: true, + strategy: "basic", + compact: vi.fn(), + }, + }), + initialMessages, + initialCompactionState, + interactive: true, + }), + ); + + expect(sessionService.persistSessionCompactionState).toHaveBeenCalledWith( + sessionId, + expect.objectContaining({ + conversation_id: sessionId, + messages: initialCompactionState.messages, + }), + ); + }); + it("does not project compaction state when compaction is disabled", async () => { const sessionId = "sess-compaction-disabled"; const manifest = createManifest(sessionId); diff --git a/sdk/packages/core/src/runtime/host/local-runtime-host.ts b/sdk/packages/core/src/runtime/host/local-runtime-host.ts index 8a4f6011b3..60b55b9b9f 100644 --- a/sdk/packages/core/src/runtime/host/local-runtime-host.ts +++ b/sdk/packages/core/src/runtime/host/local-runtime-host.ts @@ -485,9 +485,16 @@ export class LocalRuntimeHost implements RuntimeHost { const explicitInitialCompactionState = startInput.initialCompactionState; let activeSessionRef: ActiveSession | undefined; const compact = createContextCompactionPrepareTurn(configWithProvider); - const initialCompactionState = compact - ? (explicitInitialCompactionState ?? resumedCompactionState) - : undefined; + const rawInitialCompactionState = + explicitInitialCompactionState ?? resumedCompactionState; + const initialCompactionState = + compact && rawInitialCompactionState + ? { + ...rawInitialCompactionState, + conversation_id: + rawInitialCompactionState.conversation_id?.trim() || sessionId, + } + : undefined; const prepareTurn = compact ? createCompactionStateAwarePrepareTurn({ compact,