mirror of
https://github.com/RooVetGit/Roo-Code.git
synced 2026-09-09 22:31:08 +00:00
fix: make applyCacheBreakpoints non-mutating to prevent cache flag leakage
applyCacheBreakpoints() previously mutated message objects in-place, setting providerOptions with UNIVERSAL_CACHE_OPTIONS. Because the message pipeline in Task.ts passes through shared object references, these mutations leaked into apiConversationHistory and were persisted to api_conversation_history.json. Changes: - applyCacheBreakpoints() now returns a new array instead of void - Only targeted messages (typically 2) are shallow-cloned with cache providerOptions; non-targeted messages keep original references - Input parameter uses readonly T[] for compile-time mutation protection - Task.ts call site captures return value as messagesForApi and passes it to createMessage() instead of the original cleanConversationHistory - All 13 existing tests updated to assert on return value - New non-mutation test verifies originals are unchanged
This commit is contained in:
Hannes Rudolph
parent
b7857bcd6a
commit
342d99961e
3 changed files with 92 additions and 64 deletions
|
|
@ -20,42 +20,42 @@ describe("UNIVERSAL_CACHE_OPTIONS", () => {
|
|||
describe("applyCacheBreakpoints", () => {
|
||||
it("is a no-op for empty message array", () => {
|
||||
const messages: TestMessage[] = []
|
||||
applyCacheBreakpoints(messages)
|
||||
expect(messages).toEqual([])
|
||||
const result = applyCacheBreakpoints(messages)
|
||||
expect(result).toEqual([])
|
||||
})
|
||||
|
||||
it("is a no-op when all messages are assistant or system", () => {
|
||||
const messages: TestMessage[] = [{ role: "system" }, { role: "assistant" }, { role: "assistant" }]
|
||||
applyCacheBreakpoints(messages)
|
||||
expect(messages[0].providerOptions).toBeUndefined()
|
||||
expect(messages[1].providerOptions).toBeUndefined()
|
||||
expect(messages[2].providerOptions).toBeUndefined()
|
||||
const result = applyCacheBreakpoints(messages)
|
||||
expect(result[0].providerOptions).toBeUndefined()
|
||||
expect(result[1].providerOptions).toBeUndefined()
|
||||
expect(result[2].providerOptions).toBeUndefined()
|
||||
})
|
||||
|
||||
it("places 1 breakpoint on a single user message", () => {
|
||||
const messages: TestMessage[] = [{ role: "user" }]
|
||||
applyCacheBreakpoints(messages)
|
||||
expect(messages[0].providerOptions).toEqual(UNIVERSAL_CACHE_OPTIONS)
|
||||
const result = applyCacheBreakpoints(messages)
|
||||
expect(result[0].providerOptions).toEqual(UNIVERSAL_CACHE_OPTIONS)
|
||||
})
|
||||
|
||||
it("places 1 breakpoint on a single tool message", () => {
|
||||
const messages: TestMessage[] = [{ role: "tool" }]
|
||||
applyCacheBreakpoints(messages)
|
||||
expect(messages[0].providerOptions).toEqual(UNIVERSAL_CACHE_OPTIONS)
|
||||
const result = applyCacheBreakpoints(messages)
|
||||
expect(result[0].providerOptions).toEqual(UNIVERSAL_CACHE_OPTIONS)
|
||||
})
|
||||
|
||||
it("places 2 breakpoints on 2 user messages", () => {
|
||||
const messages: TestMessage[] = [{ role: "user" }, { role: "user" }]
|
||||
applyCacheBreakpoints(messages)
|
||||
expect(messages[0].providerOptions).toEqual(UNIVERSAL_CACHE_OPTIONS)
|
||||
expect(messages[1].providerOptions).toEqual(UNIVERSAL_CACHE_OPTIONS)
|
||||
const result = applyCacheBreakpoints(messages)
|
||||
expect(result[0].providerOptions).toEqual(UNIVERSAL_CACHE_OPTIONS)
|
||||
expect(result[1].providerOptions).toEqual(UNIVERSAL_CACHE_OPTIONS)
|
||||
})
|
||||
|
||||
it("places 2 breakpoints on 2 tool messages", () => {
|
||||
const messages: TestMessage[] = [{ role: "tool" }, { role: "tool" }]
|
||||
applyCacheBreakpoints(messages)
|
||||
expect(messages[0].providerOptions).toEqual(UNIVERSAL_CACHE_OPTIONS)
|
||||
expect(messages[1].providerOptions).toEqual(UNIVERSAL_CACHE_OPTIONS)
|
||||
const result = applyCacheBreakpoints(messages)
|
||||
expect(result[0].providerOptions).toEqual(UNIVERSAL_CACHE_OPTIONS)
|
||||
expect(result[1].providerOptions).toEqual(UNIVERSAL_CACHE_OPTIONS)
|
||||
})
|
||||
|
||||
it("targets last 2 non-assistant messages in a mixed conversation", () => {
|
||||
|
|
@ -66,13 +66,13 @@ describe("applyCacheBreakpoints", () => {
|
|||
{ role: "assistant" },
|
||||
{ role: "tool" },
|
||||
]
|
||||
applyCacheBreakpoints(messages)
|
||||
const result = applyCacheBreakpoints(messages)
|
||||
// Last 2 non-assistant: index 2 (user) and index 4 (tool)
|
||||
expect(messages[0].providerOptions).toBeUndefined()
|
||||
expect(messages[1].providerOptions).toBeUndefined()
|
||||
expect(messages[2].providerOptions).toEqual(UNIVERSAL_CACHE_OPTIONS)
|
||||
expect(messages[3].providerOptions).toBeUndefined()
|
||||
expect(messages[4].providerOptions).toEqual(UNIVERSAL_CACHE_OPTIONS)
|
||||
expect(result[0].providerOptions).toBeUndefined()
|
||||
expect(result[1].providerOptions).toBeUndefined()
|
||||
expect(result[2].providerOptions).toEqual(UNIVERSAL_CACHE_OPTIONS)
|
||||
expect(result[3].providerOptions).toBeUndefined()
|
||||
expect(result[4].providerOptions).toEqual(UNIVERSAL_CACHE_OPTIONS)
|
||||
})
|
||||
|
||||
it("targets indices 3 and 5 in [user, assistant, tool, user, assistant, tool]", () => {
|
||||
|
|
@ -84,21 +84,21 @@ describe("applyCacheBreakpoints", () => {
|
|||
{ role: "assistant" },
|
||||
{ role: "tool" },
|
||||
]
|
||||
applyCacheBreakpoints(messages)
|
||||
expect(messages[0].providerOptions).toBeUndefined()
|
||||
expect(messages[1].providerOptions).toBeUndefined()
|
||||
expect(messages[2].providerOptions).toBeUndefined()
|
||||
expect(messages[3].providerOptions).toEqual(UNIVERSAL_CACHE_OPTIONS)
|
||||
expect(messages[4].providerOptions).toBeUndefined()
|
||||
expect(messages[5].providerOptions).toEqual(UNIVERSAL_CACHE_OPTIONS)
|
||||
const result = applyCacheBreakpoints(messages)
|
||||
expect(result[0].providerOptions).toBeUndefined()
|
||||
expect(result[1].providerOptions).toBeUndefined()
|
||||
expect(result[2].providerOptions).toBeUndefined()
|
||||
expect(result[3].providerOptions).toEqual(UNIVERSAL_CACHE_OPTIONS)
|
||||
expect(result[4].providerOptions).toBeUndefined()
|
||||
expect(result[5].providerOptions).toEqual(UNIVERSAL_CACHE_OPTIONS)
|
||||
})
|
||||
|
||||
it("never targets system messages", () => {
|
||||
const messages: TestMessage[] = [{ role: "system" }, { role: "user" }, { role: "assistant" }, { role: "user" }]
|
||||
applyCacheBreakpoints(messages)
|
||||
expect(messages[0].providerOptions).toBeUndefined()
|
||||
expect(messages[1].providerOptions).toEqual(UNIVERSAL_CACHE_OPTIONS)
|
||||
expect(messages[3].providerOptions).toEqual(UNIVERSAL_CACHE_OPTIONS)
|
||||
const result = applyCacheBreakpoints(messages)
|
||||
expect(result[0].providerOptions).toBeUndefined()
|
||||
expect(result[1].providerOptions).toEqual(UNIVERSAL_CACHE_OPTIONS)
|
||||
expect(result[3].providerOptions).toEqual(UNIVERSAL_CACHE_OPTIONS)
|
||||
})
|
||||
|
||||
it("never targets assistant messages", () => {
|
||||
|
|
@ -108,9 +108,9 @@ describe("applyCacheBreakpoints", () => {
|
|||
{ role: "assistant" },
|
||||
{ role: "user" },
|
||||
]
|
||||
applyCacheBreakpoints(messages)
|
||||
expect(messages[1].providerOptions).toBeUndefined()
|
||||
expect(messages[2].providerOptions).toBeUndefined()
|
||||
const result = applyCacheBreakpoints(messages)
|
||||
expect(result[1].providerOptions).toBeUndefined()
|
||||
expect(result[2].providerOptions).toBeUndefined()
|
||||
})
|
||||
|
||||
it("preserves existing providerOptions via spread", () => {
|
||||
|
|
@ -122,8 +122,8 @@ describe("applyCacheBreakpoints", () => {
|
|||
},
|
||||
},
|
||||
]
|
||||
applyCacheBreakpoints(messages)
|
||||
expect(messages[0].providerOptions).toEqual({
|
||||
const result = applyCacheBreakpoints(messages)
|
||||
expect(result[0].providerOptions).toEqual({
|
||||
openai: { customField: "keep-me" },
|
||||
...UNIVERSAL_CACHE_OPTIONS,
|
||||
})
|
||||
|
|
@ -146,17 +146,17 @@ describe("applyCacheBreakpoints", () => {
|
|||
{ role: "assistant" }, // 9
|
||||
{ role: "user" }, // 10 - nonAssistant[5] <- last 2
|
||||
]
|
||||
applyCacheBreakpoints(messages, { useAnchor: true })
|
||||
const result = applyCacheBreakpoints(messages, { useAnchor: true })
|
||||
|
||||
// Should have 3 breakpoints: indices 4, 8, 10
|
||||
expect(messages[4].providerOptions).toEqual(UNIVERSAL_CACHE_OPTIONS)
|
||||
expect(messages[8].providerOptions).toEqual(UNIVERSAL_CACHE_OPTIONS)
|
||||
expect(messages[10].providerOptions).toEqual(UNIVERSAL_CACHE_OPTIONS)
|
||||
expect(result[4].providerOptions).toEqual(UNIVERSAL_CACHE_OPTIONS)
|
||||
expect(result[8].providerOptions).toEqual(UNIVERSAL_CACHE_OPTIONS)
|
||||
expect(result[10].providerOptions).toEqual(UNIVERSAL_CACHE_OPTIONS)
|
||||
|
||||
// Others should NOT have breakpoints
|
||||
expect(messages[0].providerOptions).toBeUndefined()
|
||||
expect(messages[2].providerOptions).toBeUndefined()
|
||||
expect(messages[6].providerOptions).toBeUndefined()
|
||||
expect(result[0].providerOptions).toBeUndefined()
|
||||
expect(result[2].providerOptions).toBeUndefined()
|
||||
expect(result[6].providerOptions).toBeUndefined()
|
||||
})
|
||||
|
||||
it("does not add anchor when below anchorThreshold", () => {
|
||||
|
|
@ -168,19 +168,37 @@ describe("applyCacheBreakpoints", () => {
|
|||
{ role: "user" },
|
||||
]
|
||||
// 3 non-assistant messages, below default threshold of 5
|
||||
applyCacheBreakpoints(messages, { useAnchor: true })
|
||||
const result = applyCacheBreakpoints(messages, { useAnchor: true })
|
||||
|
||||
// Last 2 only: indices 2 and 4
|
||||
expect(messages[0].providerOptions).toBeUndefined()
|
||||
expect(messages[2].providerOptions).toEqual(UNIVERSAL_CACHE_OPTIONS)
|
||||
expect(messages[4].providerOptions).toEqual(UNIVERSAL_CACHE_OPTIONS)
|
||||
expect(result[0].providerOptions).toBeUndefined()
|
||||
expect(result[2].providerOptions).toEqual(UNIVERSAL_CACHE_OPTIONS)
|
||||
expect(result[4].providerOptions).toEqual(UNIVERSAL_CACHE_OPTIONS)
|
||||
})
|
||||
|
||||
it("universal options include both anthropic and bedrock namespaces", () => {
|
||||
const messages: TestMessage[] = [{ role: "user" }]
|
||||
applyCacheBreakpoints(messages)
|
||||
expect(messages[0].providerOptions).toHaveProperty("anthropic")
|
||||
expect(messages[0].providerOptions).toHaveProperty("bedrock")
|
||||
const result = applyCacheBreakpoints(messages)
|
||||
expect(result[0].providerOptions).toHaveProperty("anthropic")
|
||||
expect(result[0].providerOptions).toHaveProperty("bedrock")
|
||||
})
|
||||
|
||||
it("should not mutate original messages", () => {
|
||||
const messages: TestMessage[] = [{ role: "user" }, { role: "assistant" }, { role: "user" }]
|
||||
const originalFirstUser = messages[0]
|
||||
const originalSecondUser = messages[2]
|
||||
const result = applyCacheBreakpoints(messages)
|
||||
// Original objects should not have providerOptions added
|
||||
expect(messages[0].providerOptions).toBeUndefined()
|
||||
expect(messages[2].providerOptions).toBeUndefined()
|
||||
// Original references should be unchanged
|
||||
expect(messages[0]).toBe(originalFirstUser)
|
||||
expect(messages[2]).toBe(originalSecondUser)
|
||||
// Result should have the cache options
|
||||
expect(result[0].providerOptions).toEqual(expect.objectContaining(UNIVERSAL_CACHE_OPTIONS))
|
||||
expect(result[2].providerOptions).toEqual(expect.objectContaining(UNIVERSAL_CACHE_OPTIONS))
|
||||
// Non-targeted messages should keep same references
|
||||
expect(result[1]).toBe(messages[1])
|
||||
})
|
||||
})
|
||||
|
||||
|
|
|
|||
|
|
@ -23,18 +23,22 @@ export interface CacheBreakpointTargeting {
|
|||
/**
|
||||
* Apply cache breakpoints to AI SDK messages with ALL provider namespaces.
|
||||
*
|
||||
* Returns a **new** array — original messages are never mutated.
|
||||
* Only targeted messages are shallow-cloned; non-targeted messages keep
|
||||
* their original object references.
|
||||
*
|
||||
* 4-breakpoint strategy:
|
||||
* 1. System prompt — passed as first message in messages[] with providerOptions
|
||||
* 2. Tool definitions — handled externally via `toolProviderOptions` in `streamText()`
|
||||
* 3-4. Last 2 non-assistant messages — this function handles these
|
||||
*
|
||||
* @param messages - The AI SDK message array (mutated in place)
|
||||
* @param messages - The AI SDK message array (NOT mutated)
|
||||
* @param targeting - Optional targeting options (defaults: 2 breakpoints, no anchor)
|
||||
* @returns A new array with cache breakpoints applied to targeted messages
|
||||
*/
|
||||
export function applyCacheBreakpoints(
|
||||
messages: { role: string; providerOptions?: Record<string, Record<string, unknown>> }[],
|
||||
targeting: CacheBreakpointTargeting = {},
|
||||
): void {
|
||||
export function applyCacheBreakpoints<
|
||||
T extends { role: string; providerOptions?: Record<string, Record<string, unknown>> },
|
||||
>(messages: readonly T[], targeting: CacheBreakpointTargeting = {}): T[] {
|
||||
const { maxBreakpoints = 2, useAnchor = false, anchorThreshold = 5 } = targeting
|
||||
|
||||
// 1. Collect non-assistant message indices (user | tool roles)
|
||||
|
|
@ -45,7 +49,7 @@ export function applyCacheBreakpoints(
|
|||
}
|
||||
}
|
||||
|
||||
if (nonAssistantIndices.length === 0) return
|
||||
if (nonAssistantIndices.length === 0) return [...messages]
|
||||
|
||||
// 2. Target last N non-assistant messages
|
||||
const targetIndices = new Set<number>()
|
||||
|
|
@ -59,15 +63,21 @@ export function applyCacheBreakpoints(
|
|||
targetIndices.add(nonAssistantIndices[anchorIdx])
|
||||
}
|
||||
|
||||
// 4. Apply UNIVERSAL cache options to targeted messages
|
||||
// 4. Shallow-copy the array, cloning ONLY targeted messages with cache options
|
||||
const result = [...messages]
|
||||
for (const idx of targetIndices) {
|
||||
if (idx >= 0 && idx < messages.length) {
|
||||
messages[idx].providerOptions = {
|
||||
...messages[idx].providerOptions,
|
||||
...UNIVERSAL_CACHE_OPTIONS,
|
||||
result[idx] = {
|
||||
...messages[idx],
|
||||
providerOptions: {
|
||||
...messages[idx].providerOptions,
|
||||
...UNIVERSAL_CACHE_OPTIONS,
|
||||
},
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
return result
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
|
|||
|
|
@ -4412,7 +4412,7 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
|
|||
const cleanConversationHistory = this.buildCleanConversationHistory(messagesWithoutImages)
|
||||
|
||||
// Breakpoints 3-4: Apply cache breakpoints to the last 2 non-assistant messages
|
||||
applyCacheBreakpoints(cleanConversationHistory.filter(isRooRoleMessage))
|
||||
const messagesForApi = applyCacheBreakpoints(cleanConversationHistory.filter(isRooRoleMessage))
|
||||
|
||||
// Check auto-approval limits
|
||||
const approvalResult = await this.autoApprovalHandler.checkAutoApprovalLimits(
|
||||
|
|
@ -4495,7 +4495,7 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
|
|||
// Reset the flag after using it
|
||||
this.skipPrevResponseIdOnce = false
|
||||
|
||||
const stream = this.api.createMessage(systemPrompt, cleanConversationHistory, metadata)
|
||||
const stream = this.api.createMessage(systemPrompt, messagesForApi, metadata)
|
||||
const iterator = stream[Symbol.asyncIterator]()
|
||||
|
||||
// Set up abort handling - when the signal is aborted, clean up the controller reference
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue