From fecf8c0386cf80bbf705448a74b8f9e5b570f5ea Mon Sep 17 00:00:00 2001 From: nordicnode Date: Mon, 31 Aug 2026 13:49:41 -0700 Subject: [PATCH] fix(agent-runtime): never wipe chat history on compaction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two verified paths replaced the model's entire context with nothing: 1. /compact built its replacement summary from fullResponse without checking it was non-empty. A silent empty stop (finish part with a real reason, no content) yields no recovery chunk, so the whole history was replaced by a single message whose summary carried nothing — every earlier turn gone. Guard the replacement on fullResponse.trim() and keep the history (the forced next step retries the summary) when the model returned nothing. 2. Compaction identified its own summary by content: a user message containing and the header sentence. A user message quoting those markers (asking about this very mechanism, pasting a summary back) matched, findLast picked it over the real summary, and the real summary was then neither re-parsed nor kept — every earlier turn vanished at the next compaction. The inlined copy in the context-pruner matched the bare tag alone, so a message merely mentioning the tag wiped memory there too. Stamp summaries with a CONVERSATION_SUMMARY tag and recognize them by provenance; keep a legacy fallback requiring the full envelope (open+close tag, header, ) so pre-tag summaries still fold in. The pruner's tag-alone match — the parity test's deliberate divergence — closes: a bare tag mention no longer qualifies in either implementation. Refs #1166 --- agents/context-pruner.ts | 16 ++- .../src/__tests__/compact-history.test.ts | 61 ++++++++++++ .../__tests__/context-pruner-parity.test.ts | 18 ++-- .../src/__tests__/main-prompt.test.ts | 97 +++++++++++++++++++ packages/agent-runtime/src/compact-history.ts | 37 +++++-- 5 files changed, 210 insertions(+), 19 deletions(-) diff --git a/agents/context-pruner.ts b/agents/context-pruner.ts index 187077c476..42634ef6dd 100644 --- a/agents/context-pruner.ts +++ b/agents/context-pruner.ts @@ -449,7 +449,20 @@ const definition: AgentDefinition = { function isConversationSummary(message: Message): boolean { if (message.role !== 'user') return false - return getTextContent(message).includes('') + // Provenance tag first — see CONVERSATION_SUMMARY_TAG in + // packages/agent-runtime/src/compact-history.ts for why identity by + // content alone is unsafe (a user message quoting the markers used to + // steal the summary's identity and erase the older memory). + if (message.tags?.includes('CONVERSATION_SUMMARY')) return true + // Legacy fallback for summaries written before the tag existed: require + // the full envelope, not just the bare tag a user can easily send. + const text = getTextContent(message) + return ( + text.includes('') && + text.includes('') && + text.includes(SUMMARY_HEADER) && + text.includes('') + ) } function extractSummaryContent(message: Message): string { @@ -854,6 +867,7 @@ ${SUMMARY_DISCLAIMER}`, role: 'user', content: summaryContentParts, sentAt: now, + tags: ['CONVERSATION_SUMMARY'], } const continuationMessage: UserMessage = { diff --git a/packages/agent-runtime/src/__tests__/compact-history.test.ts b/packages/agent-runtime/src/__tests__/compact-history.test.ts index b38e8b3ac0..71ff43e44f 100644 --- a/packages/agent-runtime/src/__tests__/compact-history.test.ts +++ b/packages/agent-runtime/src/__tests__/compact-history.test.ts @@ -188,6 +188,67 @@ describe('compactMessages', () => { ).toHaveLength(1) }) + it('does not let a user message quoting the markers steal the summary identity', () => { + // The 2026-08-31 wipe: a user message containing BOTH the tag and the + // header (asking about this very mechanism, pasting a summary back) + // matched isConversationSummary, so findLast picked the quote over the + // real summary. The real summary was then neither re-parsed nor kept as + // history — every earlier turn vanished from the model's context. + const first = compact([ + user('the original request about auth', ['USER_PROMPT']), + assistant('refactored the auth module'), + ]) + const quote = user( + 'what is ? e.g. "This is a summary of the conversation so far. The original messages have been condensed to save context space." — explain it', + ['USER_PROMPT'], + ) + const second = compactMessages({ + messages: [...first, assistant('more work'), quote], + }) + + // The real memory survives the second compaction. + expect(second.stats.previous_summary_entry_count).toBeGreaterThan(0) + expect(textOf(second.messages[0])).toContain('the original request about auth') + // The quote is the live prompt, preserved as a real message — not eaten. + // (It comes back re-stamped with a fresh sentAt, so compare content.) + expect(textOf(second.messages.at(-1)!)).toContain('what is ') + }) + + it('still recognizes a legacy summary by its full envelope', () => { + // Summaries written before the CONVERSATION_SUMMARY tag existed carry no + // tag, so identity falls back to the full envelope — open tag, header, + // close tag AND . A bare tag-plus-header quote must + // not qualify. + const legacySummary = user( + '\nThis is a summary of the conversation so far. The original messages have been condensed to save context space.\n\n\n[USER]\nthe legacy request\n\n', + ) + const result = compactMessages({ + messages: [legacySummary, assistant('and then some work')], + }) + + expect(result.stats.previous_summary_entry_count).toBeGreaterThan(0) + expect(textOf(result.messages[0])).toContain('the legacy request') + }) + + it('does not fold in a quote that has the tag and header but no memory block', () => { + // A tag-plus-header quote reproduces the pre-tag identity check. It lacks + // , so the legacy fallback must reject it — the memory + // it would have "been" belongs to the real, newer summary. + const first = compact([ + user('the original request', ['USER_PROMPT']), + assistant('working on it'), + ]) + const quote = user( + '\nThis is a summary of the conversation so far. The original messages have been condensed to save context space.', + ) + const result = compactMessages({ + messages: [...first, quote], + }) + + expect(result.stats.previous_summary_entry_count).toBeGreaterThan(0) + expect(textOf(result.messages[0])).toContain('the original request') + }) + it('spends the two budgets independently: a flood of tool work keeps user prompts', () => { const { messages, stats } = compactMessages({ messages: [ diff --git a/packages/agent-runtime/src/__tests__/context-pruner-parity.test.ts b/packages/agent-runtime/src/__tests__/context-pruner-parity.test.ts index 7ea7854e84..9fc4ba160a 100644 --- a/packages/agent-runtime/src/__tests__/context-pruner-parity.test.ts +++ b/packages/agent-runtime/src/__tests__/context-pruner-parity.test.ts @@ -392,14 +392,16 @@ describe('context-pruner parity', () => { }) /** - * Deliberate divergence #2. The pruner treats any user message containing - * `` as a memory artifact — dropping it from the - * history and re-parsing it as entries — which silently eats a user message - * that merely mentions the tag. The runtime additionally requires the header - * its own envelope always carries. This matters more here because the - * cache-expiry trigger compacts on ordinary idle turns. + * Former divergence #2, closed. The pruner used to treat any user message + * containing `` as a memory artifact — dropping it + * from the history and re-parsing it as entries, which silently ate a user + * message that merely mentions the tag, and (as findLast picks the LAST + * match) let a quoting message steal the real summary's identity and erase + * the older memory. Both implementations now recognize summaries by the + * CONVERSATION_SUMMARY tag they stamp, with a legacy full-envelope fallback + * that a bare tag mention does not satisfy. */ - it('keeps a user message that only mentions the tag, where the pruner eats it', () => { + it('keeps a user message that only mentions the tag, in both implementations', () => { const history: Message[] = [ user('why does it emit around the memory?'), assistant('because the model needs a delimiter'), @@ -412,7 +414,7 @@ describe('context-pruner parity', () => { const prunerMemory = textOfFirst(runPruner(history)) expect(runtimeMemory).toContain('why does it emit') - expect(prunerMemory).not.toContain('why does it emit') + expect(prunerMemory).toContain('why does it emit') }) it('matches the pruner when a budget evicts old entries', () => { diff --git a/packages/agent-runtime/src/__tests__/main-prompt.test.ts b/packages/agent-runtime/src/__tests__/main-prompt.test.ts index 841c5953a0..3028c69201 100644 --- a/packages/agent-runtime/src/__tests__/main-prompt.test.ts +++ b/packages/agent-runtime/src/__tests__/main-prompt.test.ts @@ -445,4 +445,101 @@ describe('mainPrompt', () => { expect(output.type).toBeDefined() // Output should exist even for empty response }) + + it('does not replace the history with an empty summary on /compact', async () => { + // A silent empty stop yields no recovery chunk, so an unguarded /compact + // replacement would collapse the whole history into one summary message + // carrying nothing — every earlier turn gone from the model's context. + mockAgentStream([]) + + const sessionState = getInitialSessionState(mockFileContext) + sessionState.mainAgentState.messageHistory = [ + { + role: 'user' as const, + content: [{ type: 'text' as const, text: 'earlier turn: fix the login bug' }], + sentAt: 1, + }, + { + role: 'assistant' as const, + content: [{ type: 'text' as const, text: 'fixed it' }], + sentAt: 2, + }, + ] + const action = { + type: 'prompt' as const, + prompt: '/compact', + sessionState, + fingerprintId: 'test', + costMode: 'normal' as const, + promptId: 'test', + toolResults: [], + } + + const { sessionState: newSessionState } = await mainPrompt({ + ...mainPromptBaseParams, + action, + localAgentTemplates: mockLocalAgentTemplates, + }) + + // The history survives: no empty summary message, earlier turns intact. + const history = newSessionState.mainAgentState.messageHistory + expect( + history.some((m) => textOfHistoryMessage(m).includes('The following is a summary')), + ).toBe(false) + expect( + history.some((m) => textOfHistoryMessage(m).includes('earlier turn: fix the login bug')), + ).toBe(true) + }) + + it('still replaces the history on /compact when the model produced a summary', async () => { + mockAgentStream([{ type: 'text', text: 'Summary: the user asked to fix the login bug, which was fixed.' }]) + + const sessionState = getInitialSessionState(mockFileContext) + sessionState.mainAgentState.messageHistory = [ + { + role: 'user' as const, + content: [{ type: 'text' as const, text: 'earlier turn: fix the login bug' }], + sentAt: 1, + }, + { + role: 'assistant' as const, + content: [{ type: 'text' as const, text: 'fixed it' }], + sentAt: 2, + }, + ] + const action = { + type: 'prompt' as const, + prompt: '/compact', + sessionState, + fingerprintId: 'test', + costMode: 'normal' as const, + promptId: 'test', + toolResults: [], + } + + const { sessionState: newSessionState } = await mainPrompt({ + ...mainPromptBaseParams, + action, + localAgentTemplates: mockLocalAgentTemplates, + }) + + const history = newSessionState.mainAgentState.messageHistory + expect(history).toHaveLength(1) + expect(textOfHistoryMessage(history[0])).toContain( + 'Summary: the user asked to fix the login bug', + ) + }) }) + +function textOfHistoryMessage(message: { content: unknown }): string { + const content = message.content as unknown + if (typeof content === 'string') return content + if (Array.isArray(content)) { + return content + .map((part: { type?: string; text?: string }) => + part.type === 'text' && typeof part.text === 'string' ? part.text : '', + ) + .join('\n') + } + return '' +} diff --git a/packages/agent-runtime/src/compact-history.ts b/packages/agent-runtime/src/compact-history.ts index cbf5d8ac56..601b4f4119 100644 --- a/packages/agent-runtime/src/compact-history.ts +++ b/packages/agent-runtime/src/compact-history.ts @@ -366,25 +366,41 @@ const SCAFFOLDING_TAGS = [ 'SUBAGENT_SPAWN', ] +/** Message tag stamped on the summary message this module (and the inlined + * copy in agents/context-pruner.ts) produces, so the next compaction finds + * the real memory artifact by provenance instead of by content. Identity by + * content is unsafe: a USER message that quotes the summary markers — asking + * about this very mechanism, pasting an old summary back — used to be taken + * for the real one. The `findLast` then picked the quote, the actual summary + * was dropped with the rest of the history, and every earlier turn vanished + * from the model's context at the next compaction. */ +export const CONVERSATION_SUMMARY_TAG = 'CONVERSATION_SUMMARY' + /** * Recognizes a memory artifact this module (or the context-pruner) produced. * - * Both markers are required, and that is the point. A summary is dropped from - * the history and re-parsed into entries, so anything mistaken for one is - * silently eaten — and the bare `` tag is a string a user - * can easily send, most obviously when asking about this very code. Requiring - * the header too means only text that reproduces our envelope qualifies. + * The tag is the identity: only messages this pass itself produced carry it, + * and a user message can never gain it by content alone. The envelope check + * below is a legacy fallback only — summaries written before the tag existed + * carry no marker, so re-summarizing them (which preserves their text as an + * entry, nested once) beats losing them. It requires the FULL envelope + * including ``: a user message that merely quotes the tag + * and the header must not qualify. * - * The context-pruner matches on the tag alone. That is a deliberate divergence - * (see the parity test): it matters much more here, because the cache-expiry - * trigger compacts on ordinary idle turns rather than only near the context - * limit, so a user message can meet a compaction pass within minutes. + * The context-pruner matches on the tag alone in its pre-tag copy. That is a + * deliberate divergence to port (see the parity test): a bare + * `` in a user message is easy to send, most obviously + * when asking about this very code. */ function isConversationSummary(message: Message): boolean { if (message.role !== 'user') return false + if (message.tags?.includes(CONVERSATION_SUMMARY_TAG)) return true const text = getTextContent(message) return ( - text.includes('') && text.includes(SUMMARY_HEADER) + text.includes('') && + text.includes('') && + text.includes(SUMMARY_HEADER) && + text.includes('') ) } @@ -743,6 +759,7 @@ ${SUMMARY_DISCLAIMER}`, role: 'user', content: [textPart, ...imageParts], sentAt, + tags: [CONVERSATION_SUMMARY_TAG], } }