diff --git a/devlog/2026-07-09_summary-role-assistant/REQ.md b/devlog/2026-07-09_summary-role-assistant/REQ.md new file mode 100644 index 0000000..3e90524 --- /dev/null +++ b/devlog/2026-07-09_summary-role-assistant/REQ.md @@ -0,0 +1,48 @@ +# REQ: Summary Role — Always Assistant + +## Problem + +Compression summaries were sometimes injected as `role: "user"` when the next +surviving message after a pruned range was a user turn. The model could then +misattribute its own prior compression recap as a user instruction. + +This was the Bug 36 "merge into user" path, which merged the summary text into +the following user message's text part, giving it `role: "user"`. + +## Root Cause + +`filterCompressedRanges()` in `lib/messages/prune.ts` had two paths: + +1. **Merge path** (Bug 36 fix): If the next surviving message was `role: "user"`, + `prependCompressionSummary()` merged the summary into that user message. + Result: model sees summary text with `role: "user"`. + +2. **Standalone path** (Bug 37 fix): Otherwise, `createSyntheticMessage(..., + "assistant")` created a standalone assistant message. Result: model sees + summary text with `role: "assistant"`. + +Bug 36 originally rejected the standalone path because `AssistantMessage` +required fields that a synthetic message lacked. Bug 37 solved this by teaching +`createSyntheticMessage` to fabricate safe defaults. The merge path was therefore +no longer necessary but remained as a source of role confusion. + +## Solution + +Remove the merge path entirely. All compression summaries are now standalone +`role: "assistant"` synthetic messages, regardless of what follows. + +## Scope + +- `lib/messages/prune.ts`: Remove dual-path logic, always use standalone assistant +- `lib/messages/utils.ts`: Dead code cleanup deferred (prependCompressionSummary, + MERGED_SUMMARY_HEADER/FOOTER remain as unused exports — harmless, cleanup TBD) +- `tests/prune.test.ts`: 3 tests updated for standalone assertions +- `tests/e2e-message-transform.test.ts`: 3 tests updated for standalone assertions + +## Acceptance Criteria + +1. All compression summaries emitted as `role: "assistant"` +2. No summary text appears in surviving user messages +3. No consecutive user turns introduced +4. All tests pass +5. Bug 28 (stale ref stripping) still works on standalone summaries diff --git a/devlog/2026-07-09_summary-role-assistant/WORKLOG.md b/devlog/2026-07-09_summary-role-assistant/WORKLOG.md new file mode 100644 index 0000000..3664215 --- /dev/null +++ b/devlog/2026-07-09_summary-role-assistant/WORKLOG.md @@ -0,0 +1,37 @@ +# WORKLOG: Summary Role — Always Assistant + +## 2026-07-09 + +### Investigation +- Read `lib/messages/prune.ts` L201-300: identified two injection paths +- Path 1 (merge into user): `findNextSurvivingMessage()` + `prependCompressionSummary()` +- Path 2 (standalone assistant): `createSyntheticMessage(..., "assistant")` +- Reviewed Bug 36 DESIGN.md rejection rationale: AssistantMessage field requirements +- Verified Bug 37 already solved this: `createSyntheticMessage` fabricates safe defaults + +### Implementation +- `prune.ts`: + - Removed `prependCompressionSummary` from import + - Replaced L250-291 dual-path logic with always-assistant standalone path + - Added [FIX Bug 39] comment block explaining the change + - Removed dead `findNextSurvivingMessage` function (L310-328) +- `utils.ts`: + - Attempted to remove `MERGED_SUMMARY_HEADER/FOOTER` + `prependCompressionSummary` + - Edit tool corrupted `DCP_BLOCK_ID_TAG_REGEX` bytes → reverted utils.ts to HEAD + - Dead code remains as unused exports — typecheck passes, cleanup deferred + +### Test Updates +- `prune.test.ts`: + - L163: summary search changed from `role: "user"` to `role: "assistant"` + - L167-210: Renamed to "prune always emits standalone assistant summary (Bug 39 fix)" + - L292-327: Bug 28 test updated to check standalone assistant msg, not m1 +- `e2e-message-transform.test.ts`: + - L387-424: standalone assistant EXISTS, u2 does NOT contain summary + - L428-516: u2 should NOT contain recap, standalone assistant should exist + - L520-607: Updated comments + role assertion from user to assistant + +### Verification +- typecheck: PASS +- build: PASS (354.25 KB) +- tests: 587 pass, 1 fail (pre-existing Bun quirk in prompts.test.ts) +- Reverted other agent's (issue #14) incomplete test change in protected-tool-exclusion.test.ts diff --git a/lib/messages/inject/inject.ts b/lib/messages/inject/inject.ts index 67a83ab..75997c9 100644 --- a/lib/messages/inject/inject.ts +++ b/lib/messages/inject/inject.ts @@ -211,7 +211,7 @@ export const injectCompressNudges = ( const toolGrowth = composition.toolTokens - state.nudges.lastToolOutputNudgeTokens if (toolGrowth >= toolOutputThreshold) { const fmt = (n: number) => (n >= 1000 ? `${(n / 1000).toFixed(1)}K` : String(n)) - const topRanges = composition.largestRanges.slice(0, 5).map((r) => `${r.ref} (${fmt(r.tokens)})`).join(", ") + const topRanges = composition.largestRanges.slice(0, 15).map((r) => `${r.ref} (${fmt(r.tokens)})`).join(", ") toolOutputReminder = `\n\n⚠️ ${fmt(toolGrowth)} new tool outputs accumulated (${fmt(composition.toolTokens)} total). Largest: ${topRanges}. Use compress tool to compress these ranges now.` state.nudges.lastToolOutputNudgeTokens = composition.toolTokens anchorsChanged = true diff --git a/lib/messages/inject/utils.ts b/lib/messages/inject/utils.ts index c8a5a5e..d28b124 100644 --- a/lib/messages/inject/utils.ts +++ b/lib/messages/inject/utils.ts @@ -649,8 +649,8 @@ export function estimateContextComposition( messageTokens, textTokens: Math.max(0, messageTokens - codeTokens), total: toolTokens + summaryTokens + messageTokens, - largestRanges: perMessage.slice(0, 10), - largestToolRanges: perTool.slice(0, 5), + largestRanges: perMessage.slice(0, 15), + largestToolRanges: perTool.slice(0, 15), largestCodeRanges: perCode.slice(0, 5), largestMessageRanges: perText.slice(0, 5), } diff --git a/lib/messages/prune.ts b/lib/messages/prune.ts index c553a4c..14432d7 100644 --- a/lib/messages/prune.ts +++ b/lib/messages/prune.ts @@ -2,7 +2,7 @@ import type { SessionState, WithParts } from "../state" import type { Logger } from "../logger" import type { PluginConfig } from "../config" import { isMessageCompacted } from "../state/utils" -import { createSyntheticMessage, prependCompressionSummary, replaceBlockIdsWithBlocked, stripStaleMessageRefs } from "./utils" +import { createSyntheticMessage, replaceBlockIdsWithBlocked, stripStaleMessageRefs } from "./utils" import { getLastUserMessage } from "./query" const PRUNED_TOOL_OUTPUT_REPLACEMENT = @@ -247,48 +247,30 @@ const filterCompressedRanges = ( ? replaceBlockIdsWithBlocked(_cleaned) : _cleaned - // [FIX Bug 36] When the next surviving message is a user turn, merge the - // summary into it instead of emitting a standalone user-role summary - // message. The old behavior placed a synthetic user message immediately - // before the user's real turn ([summary(user), user(user)]), which the - // model often read as two user turns — misattributing the assistant's - // prior output to the user and triggering "self-Q&A" loops. Merging - // yields a single user turn ([user: recap ‖ real reply]) so no fake - // conversational turn is perceived. - const nextSurviving = findNextSurvivingMessage(messages, i, state) + // [FIX Bug 39] Always emit compression summaries as standalone + // role: "assistant" messages. Previously, when the next surviving + // message was a user turn, the summary was merged INTO that user + // message (role: "user"), causing the model to treat its own prior + // compression recap as user content. Bug 37 already established that + // createSyntheticMessage("assistant") safely fabricates the required + // AssistantMessage fields, removing the original Bug 36 constraint. const blockRange = computeBlockRange(summary.startId, summary.endId) - const merged = - nextSurviving !== null && - nextSurviving.info.role === "user" && - prependCompressionSummary(nextSurviving, summaryContent, summary.blockId, blockRange) - - if (merged) { - logger.info("Merged compress summary into following user message", { - anchorMessageId: msgId, - targetMessageId: nextSurviving!.info.id, - summaryLength: summaryContent.length, - }) - } else { - // [FIX Bug 37] Emit standalone summary as role: "assistant" with - // system-metadata tags so the model cannot misattribute its own - // prior compression recap as a user instruction. - const taggedContent = - STANDALONE_SUMMARY_HEADER(summary.blockId, blockRange) + - summaryContent + - STANDALONE_SUMMARY_FOOTER - const summarySeed = `${summary.blockId}:${summary.anchorMessageId}` - const userMessage = getLastUserMessage(messages, i) - const baseForSummary = userMessage ?? msg - result.push( - createSyntheticMessage(baseForSummary, taggedContent, summarySeed, "assistant"), - ) - - logger.info("Injected compress summary as assistant role", { - anchorMessageId: msgId, - summaryLength: taggedContent.length, - hadUserBase: userMessage !== null, - }) - } + const taggedContent = + STANDALONE_SUMMARY_HEADER(summary.blockId, blockRange) + + summaryContent + + STANDALONE_SUMMARY_FOOTER + const summarySeed = `${summary.blockId}:${summary.anchorMessageId}` + const userMessage = getLastUserMessage(messages, i) + const baseForSummary = userMessage ?? msg + result.push( + createSyntheticMessage(baseForSummary, taggedContent, summarySeed, "assistant"), + ) + + logger.info("Injected compress summary as assistant role", { + anchorMessageId: msgId, + summaryLength: taggedContent.length, + hadUserBase: userMessage !== null, + }) } } @@ -307,23 +289,4 @@ const filterCompressedRanges = ( messages.push(...result) } -// [FIX Bug 36] First surviving (non-pruned) message at or after startIndex. -// Starts the scan at startIndex inclusive to handle both anchor layouts: when -// the anchor is itself part of the pruned range (message-start ranges) it is -// skipped, and when the anchor survives (block-anchor ranges) it is returned — -// in either case this yields the next real turn the model sees after the recap. -const findNextSurvivingMessage = ( - messages: WithParts[], - startIndex: number, - state: SessionState, -): WithParts | null => { - for (let j = startIndex; j < messages.length; j++) { - const candidate = messages[j]! - const entry = state.prune.messages.byMessageId.get(candidate.info.id) - if (entry && entry.activeBlockIds.length > 0) { - continue - } - return candidate - } - return null -} + diff --git a/tests/e2e-message-transform.test.ts b/tests/e2e-message-transform.test.ts index e29365c..7bfc27c 100644 --- a/tests/e2e-message-transform.test.ts +++ b/tests/e2e-message-transform.test.ts @@ -384,15 +384,13 @@ test("compression blocks: compressed messages are replaced with summaries", asyn assert.ok(remainingIds.includes("u2"), "u2 should survive") assert.ok(remainingIds.includes("a2"), "a2 should survive") - // [FIX Bug 36] The summary is now merged into the following user message (u2) - // instead of a standalone synthetic message, so the model no longer sees two - // consecutive user turns ([summary(user), u2(user)]) which caused role - // confusion / self-Q&A loops. - // The recap content is checked (not the msg_dcp_summary_ id prefix) because - // the unrelated suffix-guidance nudge message reuses that same id prefix. + // [FIX Bug 39] The summary is now ALWAYS a standalone role: "assistant" + // synthetic message, regardless of what follows. Previously (Bug 36) it + // was merged into the following user message (u2), causing role confusion. const standaloneWithRecap = output.messages.find( (m: any) => m.info.id?.startsWith("msg_dcp_summary_") && + m.info.role === "assistant" && m.parts.some( (p: any) => p.type === "text" && @@ -401,12 +399,11 @@ test("compression blocks: compressed messages are replaced with summaries", asyn ), ) assert.ok( - !standaloneWithRecap, - "recap should be merged into the following user message, not emitted as a standalone synthetic message", + standaloneWithRecap, + "recap should be a standalone assistant synthetic message (Bug 39)", ) - // The summary content should be prepended into u2 (the following user message), - // and u2's original text must still be present after the recap. + // The summary content should NOT be in u2 — u2 keeps only its own text. const u2Msg = output.messages.find((m: any) => m.info.id === "u2") assert.ok(u2Msg, "u2 should survive") const u2Text = u2Msg!.parts @@ -414,12 +411,12 @@ test("compression blocks: compressed messages are replaced with summaries", asyn .map((p: any) => p.text) .join("") assert.ok( - u2Text.includes("Previous conversation about greetings"), - "compressed summary should be merged into u2", + !u2Text.includes("Previous conversation about greetings"), + "u2 should NOT contain the compressed summary (Bug 39)", ) assert.ok( u2Text.includes("How are you?"), - "u2's original text should be preserved after the summary is prepended", + "u2's original text should be preserved", ) }) @@ -504,15 +501,25 @@ test("compression summary: never produces two consecutive user turns (Bug 36)", ) } - // The surviving user turn must carry both the recap and its own original text. + // The surviving user turn must NOT carry the recap (Bug 39: always standalone). const u2 = historical.find((m: WithParts) => m.info.id === "u2") assert.ok(u2, "u2 should survive") const u2Text = u2!.parts .filter((p) => p.type === "text") .map((p) => (p as any).text) .join("") - assert.ok(u2Text.includes("The assistant explained the plan"), "recap merged into u2") + assert.ok(!u2Text.includes("The assistant explained the plan"), "recap should NOT be merged into u2 (Bug 39)") assert.ok(u2Text.includes("Sounds good, continue."), "u2 original text preserved") + + // The recap must exist as a standalone assistant message. + const recapMsg = historical.find( + (m: WithParts) => + m.info.role === "assistant" && + m.parts.some( + (p) => p.type === "text" && typeof (p as any).text === "string" && (p as any).text.includes("The assistant explained the plan"), + ), + ) + assert.ok(recapMsg, "recap should be a standalone assistant message (Bug 39)") }) // ─── Test: Fallback — standalone summary when no following user turn (Bug 36) ── @@ -577,11 +584,12 @@ test("compression summary: emits standalone summary when range is last (no user assert.ok(!remainingIds.includes("u2"), "u2 (covered by block) should be pruned") assert.ok(!remainingIds.includes("a2"), "a2 (covered by block) should be pruned") - // No following user turn exists to merge into, so the recap must be emitted - // as a standalone synthetic user message carrying the summary content. + // [Bug 39] All summaries are standalone assistant messages. This test + // verifies the standalone path when the pruned range is at the end. const standalone = output.messages.find( (m: any) => m.info.id?.startsWith("msg_dcp_summary_") && + m.info.role === "assistant" && m.parts.some( (p: any) => p.type === "text" && @@ -591,7 +599,7 @@ test("compression summary: emits standalone summary when range is last (no user ) assert.ok(standalone, "fallback path must emit a standalone synthetic summary") - // The standalone summary (user) follows a1 (assistant), so alternation holds. + // The standalone summary (assistant) follows a1 (assistant), so no role confusion. const lastIdx = output.messages.length - 1 const checkMessages = output.messages.filter( (m: any, idx: number) => !(idx === lastIdx && isSyntheticMessage(m)), diff --git a/tests/prune.test.ts b/tests/prune.test.ts index f7144a0..c5c0e5a 100644 --- a/tests/prune.test.ts +++ b/tests/prune.test.ts @@ -160,11 +160,11 @@ test("prune removes messages in active compression ranges", () => { assert.ok(!ids.includes("m3"), "m3 should be pruned") assert.ok(ids.includes("m1"), "anchor m1 should survive") assert.ok(ids.includes("m4"), "m4 should survive") - const summaryMsg = messages.find((m) => m.info.role === "user" && m.parts.some((p: any) => p.type === "text" && p.text?.includes("Summary of m2-m3"))) + const summaryMsg = messages.find((m) => m.info.role === "assistant" && m.parts.some((p: any) => p.type === "text" && p.text?.includes("Summary of m2-m3"))) assert.ok(summaryMsg, "summary should be injected") }) -test("prune merges summary into surviving user message at anchor (Bug 36 fix)", () => { +test("prune always emits standalone assistant summary (Bug 39 fix)", () => { const state = createSessionState() state.prune.messages.byMessageId.set("m2", { tokenCount: 100, allBlockIds: [1], activeBlockIds: [1] }) state.prune.messages.activeByAnchorMessageId.set("m1", 1) @@ -183,8 +183,8 @@ test("prune merges summary into surviving user message at anchor (Bug 36 fix)", directToolIds: [], effectiveToolIds: [], anchorMessageId: "m1", - topic: "merged topic", - summary: "Merged summary text", + topic: "test topic", + summary: "Standalone summary text", } as CompressionBlock) const messages: WithParts[] = [ @@ -201,12 +201,13 @@ test("prune merges summary into surviving user message at anchor (Bug 36 fix)", const m1 = messages.find((m) => m.info.id === "m1") assert.ok(m1, "m1 (anchor) should survive") const m1Text = m1!.parts.map((p: any) => p.text ?? "").join("") - assert.ok(m1Text.includes("Merged summary text"), "summary should be merged into anchor m1") + assert.ok(!m1Text.includes("Standalone summary text"), "summary should NOT be merged into m1") assert.ok(m1Text.includes("first question"), "original m1 text should be preserved") - const syntheticCount = messages.filter( - (m) => m.info.role === "user" && m.parts.some((p: any) => p.type === "text" && p.text?.includes("Merged summary text") && m.info.id !== "m1"), - ).length - assert.equal(syntheticCount, 0, "should not create standalone summary when merged into anchor") + + const summaryMsg = messages.find( + (m) => m.info.role === "assistant" && m.parts.some((p: any) => p.type === "text" && p.text?.includes("Standalone summary text")), + ) + assert.ok(summaryMsg, "should create standalone assistant summary message") }) test("prune creates standalone summary when anchor is assistant and no user follows", () => { @@ -319,10 +320,13 @@ test("prune strips stale mNNNN refs from summary content (Bug 28 fix)", () => { prune(state, logger, buildConfig(), messages) - const m1 = messages.find((m) => m.info.id === "m1") - const m1Text = m1!.parts.map((p: any) => p.text ?? "").join("") - assert.ok(!m1Text.includes(""), "stale dcp-message-id tag should be stripped") - assert.ok(m1Text.includes("Summary with"), "summary content should remain after stripping") + const summaryMsg = messages.find( + (m) => m.info.role === "assistant" && m.parts.some((p: any) => p.type === "text" && p.text?.includes("Summary with")), + ) + assert.ok(summaryMsg, "standalone assistant summary should exist") + const summaryText = summaryMsg!.parts.map((p: any) => p.text ?? "").join("") + assert.ok(!summaryText.includes("dcp-message-id"), "stale dcp-message-id tag should be stripped from summary") + assert.ok(summaryText.includes("Summary with"), "summary content should remain after stripping") }) // =====================================================================