Skip to content

Fix critical bugs in string truncation and context window lookup - #1194

Open
pavankumar-vh wants to merge 1 commit into
CodebuffAI:mainfrom
pavankumar-vh:fix/bug-fixes-only
Open

Fix critical bugs in string truncation and context window lookup#1194
pavankumar-vh wants to merge 1 commit into
CodebuffAI:mainfrom
pavankumar-vh:fix/bug-fixes-only

Conversation

@pavankumar-vh

Copy link
Copy Markdown

Overview

Fix two critical bugs that could cause runtime errors and improve code safety.

Bug Fixes

1. Fix context window lookup in base-chat.ts

Issue: The code used CONTEXT_WINDOWS[model ?? ''] which converts undefined model to empty string and unnecessarily performs a lookup.

Fix: Changed to (model && CONTEXT_WINDOWS[model]) ?? DEFAULT_CONTEXT_WINDOW which:

  • Avoids unnecessary lookup when model is undefined
  • More clearly expresses intent
  • Falls back to default window correctly

2. Fix critical bug in truncateStringWithMessage

Issue: When maxLength is smaller than the message length, the function calculates negative slice indices, causing unexpected behavior.

Fix: Added Math.max(0, ...) guards to prevent negative slice lengths:

  • END mode: Math.max(0, maxLength - suffix.length)
  • START mode: Math.max(0, maxLength - prefix.length)
  • MIDDLE mode: Math.max(0, Math.floor((maxLength - middle.length) / 2))

This prevents potential runtime errors when truncating strings with very small maxLength values.

Files Changed

  • agents/base-chat.ts - Improved context window lookup logic
  • common/src/util/string.ts - Added safety guards to prevent negative slice indices

Scope

This change only touches agents/ and common/ which are approved contribution areas per the Contributing Guide.

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.
@codebuff-team

Copy link
Copy Markdown
Contributor

Nice catch on truncateStringWithMessage in common/src/util/string.ts. 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. Your Math.max(0, ...) guards fix that correctly for END/START/MIDDLE, and the new tests in string.test.ts cover the negative and zero-availableLength cases well. This part is a solid, well-scoped port candidate.

The base-chat.ts change is a different story: CONTEXT_WINDOWS[model ?? ''] and (model && CONTEXT_WINDOWS[model]) ?? DEFAULT_CONTEXT_WINDOW are behaviorally identical — CONTEXT_WINDOWS[''] is just undefined, which falls through to DEFAULT_CONTEXT_WINDOW either way. It's a marginally clearer expression of intent, but it isn't a bug fix and doesn't belong under the "critical bugs" framing in the title/description. I'd drop that hunk or reframe it as a pure style nit rather than claim it prevents a runtime error, since it doesn't.

Also note the test file has two it('should truncate from start correctly', ...) blocks with identical names/bodies (lines ~257-266 and ~283-292) — dedupe one of those.

Overall: keep the string.ts fix, drop or relabel the base-chat.ts change, and remove the duplicate test.

@codebuff-team codebuff-team added bot:triaged Classified by the community triage bot pr:port-candidate Worth porting into the private source tree labels Sep 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot:triaged Classified by the community triage bot pr:port-candidate Worth porting into the private source tree

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants