Skip to content

fix: preserve original first message context during conversation condensing #7939

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Sep 12, 2025
Merged

fix: preserve original first message context during conversation condensing #7939

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
76cf136
fix: preserve original first message context during conversation cond…
daniel-lxs Sep 12, 2025
0ba4d14
test: update condense.spec.ts to expect preserved first message
daniel-lxs Sep 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions src/core/condense/__tests__/condense.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,9 +229,9 @@ describe("Condense", () => {

const result = getMessagesSinceLastSummary(messages)

// Should include a user message prefix for Bedrock compatibility, the summary, and messages after
// Should include the original first user message for context preservation, the summary, and messages after
expect(result[0].role).toBe("user")
expect(result[0].content).toBe("Please continue from the following summary:")
expect(result[0].content).toBe("First message") // Preserves original first message
expect(result[1]).toEqual(messages[2]) // The summary
expect(result[2]).toEqual(messages[3])
expect(result[3]).toEqual(messages[4])
Expand All @@ -249,9 +249,9 @@ describe("Condense", () => {

const result = getMessagesSinceLastSummary(messages)

// Should only include from the last summary
// Should only include from the last summary with original first message preserved
expect(result[0].role).toBe("user")
expect(result[0].content).toBe("Please continue from the following summary:")
expect(result[0].content).toBe("First message") // Preserves original first message
expect(result[1]).toEqual(messages[3]) // Second summary
expect(result[2]).toEqual(messages[4])
expect(result[3]).toEqual(messages[5])
Expand Down
8 changes: 4 additions & 4 deletions src/core/condense/__tests__/index.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ describe("getMessagesSinceLastSummary", () => {
expect(result).toEqual(messages)
})

it("should return messages since the last summary with prepended user message", () => {
it("should return messages since the last summary with original first user message", () => {
Copy link

@roomote roomote bot Sep 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice test update! Consider adding an explicit test case for when the original messages array doesn't start with a user message to ensure the fallback behavior is properly covered.

const messages: ApiMessage[] = [
{ role: "user", content: "Hello", ts: 1 },
{ role: "assistant", content: "Hi there", ts: 2 },
Expand All @@ -47,14 +47,14 @@ describe("getMessagesSinceLastSummary", () => {

const result = getMessagesSinceLastSummary(messages)
expect(result).toEqual([
{ role: "user", content: "Please continue from the following summary:", ts: 0 },
{ role: "user", content: "Hello", ts: 1 },
{ role: "assistant", content: "Summary of conversation", ts: 3, isSummary: true },
{ role: "user", content: "How are you?", ts: 4 },
{ role: "assistant", content: "I'm good", ts: 5 },
])
})

it("should handle multiple summary messages and return since the last one with prepended user message", () => {
it("should handle multiple summary messages and return since the last one with original first user message", () => {
const messages: ApiMessage[] = [
{ role: "user", content: "Hello", ts: 1 },
{ role: "assistant", content: "First summary", ts: 2, isSummary: true },
Expand All @@ -65,7 +65,7 @@ describe("getMessagesSinceLastSummary", () => {

const result = getMessagesSinceLastSummary(messages)
expect(result).toEqual([
{ role: "user", content: "Please continue from the following summary:", ts: 0 },
{ role: "user", content: "Hello", ts: 1 },
{ role: "assistant", content: "Second summary", ts: 4, isSummary: true },
{ role: "user", content: "What's new?", ts: 5 },
])
Expand Down
23 changes: 18 additions & 5 deletions src/core/condense/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,11 +223,24 @@ export function getMessagesSinceLastSummary(messages: ApiMessage[]): ApiMessage[
const messagesSinceSummary = messages.slice(lastSummaryIndex)

// Bedrock requires the first message to be a user message.
// We preserve the original first message to maintain context.
Copy link

@roomote roomote bot Sep 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we expand this comment to explain why preserving the original first message is important for maintaining task context? This would help future developers understand the reasoning beyond just Bedrock compatibility.

// See https://github.com/RooCodeInc/Roo-Code/issues/4147
const userMessage: ApiMessage = {
role: "user",
content: "Please continue from the following summary:",
ts: messages[0]?.ts ? messages[0].ts - 1 : Date.now(),
if (messagesSinceSummary.length > 0 && messagesSinceSummary[0].role !== "user") {
// Get the original first message (should always be a user message with the task)
const originalFirstMessage = messages[0]
Copy link

@roomote roomote bot Sep 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this edge case intentional? The function checks if the first message after a summary is not a user message, but what happens if the original messages array itself doesn't start with a user message? While the fallback handles this gracefully, could we add a validation or warning when the original conversation doesn't follow the expected pattern?

if (originalFirstMessage && originalFirstMessage.role === "user") {
// Use the original first message unchanged to maintain full context
return [originalFirstMessage, ...messagesSinceSummary]
Copy link

@roomote roomote bot Sep 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we consider extracting this logic into a helper function for better readability? Something like:

This would make the intent clearer and potentially allow for reuse.

} else {
// Fallback to generic message if no original first message exists (shouldn't happen)
const userMessage: ApiMessage = {
role: "user",
content: "Please continue from the following summary:",
ts: messages[0]?.ts ? messages[0].ts - 1 : Date.now(),
}
return [userMessage, ...messagesSinceSummary]
}
}
return [userMessage, ...messagesSinceSummary]

return messagesSinceSummary
}
Loading