Skip to content

Conversation

@shirishgone
Copy link

@shirishgone shirishgone commented Aug 10, 2025

…- Fixes #4430

This PR implements a skipContextCompression flag to prevent unwanted context compression during multi-file read operations and settings save operations. The fix addresses the issue where context compression was being triggered inappropriately, causing performance issues and unexpected behavior.

Key implementation details:

  • Added skipContextCompression parameter to truncateConversationIfNeeded function
  • When the flag is true, the function returns early without performing compression, even if the context usage exceeds the threshold
  • The flag is designed to be used in scenarios where compression should be temporarily disabled (e.g., during multi-file reads, settings operations)
  • Maintains backward compatibility by making the parameter optional with a default value of false

Reviewers should pay attention to:

  • The early return logic in the sliding window function
  • Test coverage for both skip and normal compression scenarios
  • Integration points where the flag needs to be passed through

Test Procedure

How I tested this implementation:

  1. Created comprehensive unit tests covering:

    • Skip functionality when flag is true
    • Normal compression when flag is false
    • Edge cases with different threshold values
    • Multi-file read scenario simulation
  2. All existing tests continue to pass, ensuring no regression

How reviewers can verify:

  1. Run the test suite: pnpm test
  2. Check the new test file: src/core/sliding-window/__tests__/context-compression-fix.test.ts
  3. Verify that the skipContextCompression parameter is properly handled in the main function

Testing environment:

  • Node.js 20.19.2
  • All tests passing with vitest framework

Pre-Submission Checklist

  • Issue Linked: This PR is linked to an approved GitHub Issue ( [BUG] Context condensing triggers on settings Save during active task #4430)
  • Scope: My changes are focused on the linked issue (context compression skip functionality)
  • Self-Review: I have performed a thorough self-review of my code
  • Testing: New comprehensive tests have been added to cover the skip functionality
  • Documentation Impact: No user-facing documentation updates required (internal API change)
  • Contribution Guidelines: I have read and agree to the Contributor Guidelines

Screenshots / Videos

Documentation Updates

  • No documentation updates are required (internal API enhancement)

Additional Notes

This fix specifically addresses the performance issue mentioned in #4430 where context compression was being triggered during multi-file read operations when maxConcurrentFileReads > 1. The skipContextCompression flag provides a clean way to temporarily disable compression during these operations while maintaining the existing compression logic for normal use cases.

The implementation is designed to be:

  • Safe: Backward compatible with existing code
  • Targeted: Only affects compression behavior when explicitly requested
  • Testable: Comprehensive test coverage for all scenarios

Get in Touch

[email protected]


Important

Introduces skipContextCompression flag to control context compression, preventing unwanted compression during multi-file reads and settings operations.

  • Behavior:
    • Adds skipContextCompression flag to truncateConversationIfNeeded in index.ts to prevent unwanted context compression.
    • When true, skips compression even if context usage exceeds threshold.
    • Used during multi-file reads and settings operations to avoid performance issues.
    • Backward compatible with default false value.
  • Integration:
    • Task.ts: Passes skipContextCompression flag in truncateConversationIfNeeded calls.
    • webviewMessageHandler.ts: Sets _skipNextContextCompressionCheck when maxConcurrentFileReads > 1.
  • Testing:
    • New tests in context-compression-fix.test.ts for scenarios with and without the flag.
    • Tests cover edge cases and multi-file read scenarios.

This description was created by Ellipsis for eb95b87. You can customize this summary. It will automatically update as commits are pushed.

…Fixes RooCodeInc#4430

- Add skipContextCompression parameter to truncateConversationIfNeeded function
- Skip context compression when flag is true, even if threshold is exceeded
- Add comprehensive tests covering the skip functionality
- Fixes issue where multi-file read operations triggered unwanted compression
@shirishgone shirishgone requested review from cte, jr and mrubens as code owners August 10, 2025 07:29
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. bug Something isn't working labels Aug 10, 2025
@hannesrudolph hannesrudolph added the Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. label Aug 10, 2025
Copy link
Contributor

@roomote roomote bot left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution! I've reviewed the changes and found some issues that need attention. The implementation addresses the core problem but has some code quality concerns that should be resolved.

… feedback

- Replace unsafe 'any' casting with proper TypeScript types
- Add proper class methods for managing skip flag (setSkipNextContextCompression, getAndClearSkipContextCompression)
- Clean up TruncateOptions interface by removing unused properties
- Fix missing parameter extraction in truncateConversationIfNeeded function
- Extend fix to handle multiple settings that could trigger unwanted compression:
  * maxReadFileLine - file reading settings
  * includeDiagnosticMessages - diagnostic settings
  * maxDiagnosticMessages - diagnostic settings
- Add comprehensive test to verify flag is properly cleared after use
- Ensure type safety and maintainability throughout the implementation

Addresses all feedback from PR review for Issue RooCodeInc#4430
@daniel-lxs
Copy link
Member

Hey @shirishgone, Thank you for your contribution. I think we should try and address the root cause of the bug rather than trying to mitigate the symptoms. The main concern is that this will prevent condensing when it's actually needed if the files read exceed the context window of the model.

I'll close this PR for now, but feel free to continue the discussion. I would like to know what you think.

@daniel-lxs daniel-lxs closed this Aug 12, 2025
@github-project-automation github-project-automation bot moved this from New to Done in Roo Code Roadmap Aug 12, 2025
@github-project-automation github-project-automation bot moved this from Triage to Done in Roo Code Roadmap Aug 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. size:L This PR changes 100-499 lines, ignoring generated files.

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

[BUG] Context condensing triggers on settings Save during active task

4 participants