Skip to content

fix: prevent context truncation when condensing is disabled (threshold=100%) #7812

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

Closed
wants to merge 2 commits into from
Closed

fix: prevent context truncation when condensing is disabled (threshold=100%) #7812

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
848fd82
fix: prevent context truncation when condensing is disabled (threshol…
roomote Sep 9, 2025
ec0a03d
fix: use autoCondenseContext checkbox instead of 100% threshold check
roomote Sep 10, 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
46 changes: 44 additions & 2 deletions src/core/sliding-window/__tests__/sliding-window.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -577,7 +577,7 @@ describe("Sliding Window", () => {
maxTokens: modelInfo.maxTokens,
apiHandler: mockApiHandler,
autoCondenseContext: true,
autoCondenseContextPercent: 100,
autoCondenseContextPercent: 50, // Use a threshold less than 100%
systemPrompt: "System prompt",
taskId,
profileThresholds: {},
Expand Down Expand Up @@ -644,7 +644,7 @@ describe("Sliding Window", () => {
maxTokens: modelInfo.maxTokens,
apiHandler: mockApiHandler,
autoCondenseContext: true,
autoCondenseContextPercent: 100,
autoCondenseContextPercent: 50, // Use a threshold less than 100%
systemPrompt: "System prompt",
taskId,
profileThresholds: {},
Expand Down Expand Up @@ -821,6 +821,48 @@ describe("Sliding Window", () => {
// Clean up
summarizeSpy.mockRestore()
})

it("should not truncate when autoCondenseContext is true and threshold is 100% even if tokens exceed allowedTokens", async () => {
const modelInfo = createModelInfo(100000, 30000)
const totalTokens = 75000 // This exceeds allowedTokens (60000) but should not truncate when disabled

const messagesWithSmallContent = [
...messages.slice(0, -1),
{ ...messages[messages.length - 1], content: "" },
]

// Spy on summarizeConversation to ensure it's not called
const summarizeSpy = vi.spyOn(condenseModule, "summarizeConversation")

const result = await truncateConversationIfNeeded({
messages: messagesWithSmallContent,
totalTokens,
contextWindow: modelInfo.contextWindow,
maxTokens: modelInfo.maxTokens,
apiHandler: mockApiHandler,
autoCondenseContext: true, // Enabled but with 100% threshold
autoCondenseContextPercent: 100, // 100% threshold means never condense
systemPrompt: "System prompt",
taskId,
profileThresholds: {},
currentProfileId: "default",
})

// Verify summarizeConversation was NOT called when threshold is 100%
expect(summarizeSpy).not.toHaveBeenCalled()

// Should NOT truncate even though tokens exceed allowedTokens when threshold is 100%
expect(result).toEqual({
messages: messagesWithSmallContent,
summary: "",
cost: 0,
prevContextTokens: totalTokens,
error: undefined,
})

// Clean up
summarizeSpy.mockRestore()
})
Copy link
Contributor Author

@roomote roomote bot Sep 9, 2025

Choose a reason for hiding this comment

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

Great test coverage for the main scenario! Would it be worth adding an edge case test for threshold values slightly above 100 (e.g., 101%) to ensure the >= 100 condition in the implementation handles these cases correctly? Even though such values shouldn't occur in practice, defensive testing could catch potential issues.

})

/**
Expand Down
10 changes: 9 additions & 1 deletion src/core/sliding-window/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ export async function truncateConversationIfNeeded({
}
// If no specific threshold is found for the profile, fall back to global setting

if (autoCondenseContext) {
if (autoCondenseContext && effectiveThreshold < 100) {
Copy link
Contributor Author

@roomote roomote bot Sep 9, 2025

Choose a reason for hiding this comment

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

Consider adding a comment near the function signature or at the start of the function body documenting that a threshold of 100% has special meaning - it completely disables both condensing and truncation. This would make the API contract clearer for callers.

const contextPercent = (100 * prevContextTokens) / contextWindow
if (contextPercent >= effectiveThreshold || prevContextTokens > allowedTokens) {
// Attempt to intelligently condense the context
Expand All @@ -166,7 +166,15 @@ export async function truncateConversationIfNeeded({
}

// Fall back to sliding window truncation if needed
// Exception: When context condensing is explicitly disabled (threshold = 100%), don't truncate
Copy link
Contributor Author

@roomote roomote bot Sep 9, 2025

Choose a reason for hiding this comment

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

This comment could be expanded to explain why we're making this exception. Something like:

Suggested change
// Exception: When context condensing is explicitly disabled (threshold = 100%), don't truncate
// Exception: When context condensing is explicitly disabled (threshold = 100%), don't truncate
// This respects the user's explicit choice to maintain full conversation history,
// even at the risk of hitting API token limits

This would help future maintainers understand the design decision.

if (prevContextTokens > allowedTokens) {
// Check if condensing is explicitly disabled (threshold is 100% and autoCondenseContext is true)
// This means the user has set the threshold to 100% to disable condensing
if (autoCondenseContext && effectiveThreshold >= 100) {
Copy link
Contributor Author

@roomote roomote bot Sep 9, 2025

Choose a reason for hiding this comment

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

Is using >= 100 intentional here? Since the issue specifically mentions setting the threshold to "100%" to disable condensing, would it be clearer to check for exactly 100? Values above 100% don't really make semantic sense in this context.

Consider:

Suggested change
if (autoCondenseContext && effectiveThreshold >= 100) {
if (autoCondenseContext && effectiveThreshold === 100) {

// Context condensing is explicitly disabled by user, don't truncate
return { messages, summary: "", cost, prevContextTokens, error }
}
// Apply sliding window truncation in all other cases
const truncatedMessages = truncateConversation(messages, 0.5, taskId)
return { messages: truncatedMessages, prevContextTokens, summary: "", cost, error }
}
Expand Down
Loading