Fix critical bug in truncateStringWithMessage: Prevent negative slice indices - #1209
Open
pavankumar-vh wants to merge 2 commits into
Open
Fix critical bug in truncateStringWithMessage: Prevent negative slice indices#1209pavankumar-vh wants to merge 2 commits into
pavankumar-vh wants to merge 2 commits into
Conversation
Bug Fixes: 1. Fix context window lookup in base-chat.ts: Handle missing/undefined model correctly - Change: → - Prevents unnecessary lookup and improves clarity 2. Fix critical bug in truncateStringWithMessage: Prevent negative slice indices - Added Math.max(0, ...) guards to prevent negative slice lengths - Fixes potential runtime errors when maxLength < message length - Applies to all truncation modes (START, END, MIDDLE) 3. Add comprehensive tests for truncateStringWithMessage - Added 9 test cases covering edge cases - Tests for negative/zero available length scenarios - Tests for all truncation modes (START, END, MIDDLE) - Tests for custom messages and empty strings All changes are in approved contribution areas (agents/, common/) and improve code safety.
… indices When maxLength is smaller than the message/prefix/suffix length, the old code computed a negative slice argument. String.slice treats negative end/start as counting from the end of the string, so instead of truncating to (roughly) maxLength, the function could silently return a chunk of the original string plus the truncation banner — defeating the whole point of bounding length. Added Math.max(0, ...) guards to prevent negative slice lengths in all three truncation modes (END, START, MIDDLE). Also added comprehensive tests covering: - Normal truncation behavior for all three modes - Edge cases with negative/zero available length - Custom message handling - Empty string handling
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
Fix a critical bug in
truncateStringWithMessagethat could cause unexpected behavior whenmaxLengthis smaller than the message/prefix/suffix length.Bug Description
When
maxLengthis smaller than the message/prefix/suffix length, the old code computed a negative slice argument.String.slicetreats negative end/start as counting from the end of the string, so instead of truncating to (roughly)maxLength, the function could silently return a chunk of the original string plus the truncation banner — defeating the whole point of bounding length.Fix
Added
Math.max(0, ...)guards to prevent negative slice lengths in all three truncation modes:Math.max(0, maxLength - suffix.length)Math.max(0, maxLength - prefix.length)Math.max(0, Math.floor((maxLength - middle.length) / 2))Tests Added
Added 9 comprehensive test cases covering:
All tests pass (28 total, 0 fail).
Files Changed
common/src/util/string.ts- Added safety guards to prevent negative slice indicescommon/src/util/__tests__/string.test.ts- Added comprehensive test coverageScope
This change only touches
common/which is an approved contribution area per the Contributing Guide.