Uh oh!
There was an error while loading. Please reload this page.
fix(mcp): capText's truncation marker could push output past budget - #654
Open
teyrebaz33 wants to merge 2 commits into
Open
fix(mcp): capText's truncation marker could push output past budget#654teyrebaz33 wants to merge 2 commits into
teyrebaz33 wants to merge 2 commits into
Conversation
execution-projection.ts documents capText as a "hard char budget" primitive: "the result is bounded for ANY argument combination". The implementation didn't honor that -- it sliced text to exactly budget characters, then appended a truncation marker (e.g. "...[truncated 48000 chars -- open https://... for the full value]") AFTER the slice, so the returned string was budget + marker.length chars, not budget. Concretely: capText(text, 2000, someUrl) could return a 2094-char string. Verified with a 240-case brute-force sweep across budgets (including 0/1/5/10) and text lengths that the old implementation violated the bound in the small-budget cases exercised by PREVIEW_BUDGET (2000). Fix: since the marker's own length depends on the dropped-char count, which depends on where we slice, which depends on the marker's length, resolve this with a small converging loop that shrinks the slice point until slice + marker fits within budget, with a final .slice(0, budget) as a hard backstop for degenerate tiny budgets. Also fixes the reported dropped-char count, which the old version could get slightly wrong for the same reason. Added shared.test.ts (no test file existed for this module) with a regression test that fails 4/6 cases against the old implementation (verified via git stash) and passes 6/6 against the fix. No dependency or lockfile changes.
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 freeto 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.
Primary change type
Problem and motivation
execution-projection.tsdocumentscapText(packages/mcp/src/tools/shared.ts) as a "hard char budget" primitive: "the result is bounded for ANY argument combination." The implementation didn't honor that — it sliced text to exactlybudgetcharacters, then appended a truncation marker AFTER the slice, so the returned string wasbudget + marker.lengthcharacters, notbudget. Concretely,capText(text, 2000, someUrl)could return a 2094-char string.Summary and scope
Since the marker's own length depends on the dropped-char count, which depends on where we slice, which depends on the marker's length, this resolves it with a small converging loop that shrinks the slice point until slice + marker fits within
budget, with a final.slice(0, budget)backstop for degenerate tiny budgets. This also fixes the reported dropped-char count, which the old version could get slightly wrong for the same reason.Out of scope: no changes to
execution-projection.tsitself or to the field budgets it configures; no dependency changes.Related work
Related issue or discussion: #653
Validation
Tests and documentation
Added
packages/mcp/src/tools/shared.test.ts(no test file existed for this module before): a 240-case brute-force sweep across budgets (including 0/1/5/10, and the realPREVIEW_BUDGET/DEFAULT_FIELD_BUDGETvalues) confirming the fix never exceedsbudget, plus a regression test verified viagit stashto fail 4/6 cases against the pre-fix implementation and pass 6/6 against the fix. No user-facing documentation changes needed.Compatibility and release impact
capText's signature and return type are unchanged; only truncated outputs near a field's budget boundary get slightly shorter (by the marker's length) to actually respect the documented bound..changeset/fix-captext-budget-overrun.md), patch bump.Security
will follow the
Security Policy for
private reporting.
AI assistance
I used Claude (Anthropic) as a coding assistant throughout: it helped find the bug (including writing a small script that proved the overrun with real numbers), draft the fix and the brute-force test sweep, and run the verification commands quoted above. I reviewed and ran every command myself, read and understood the resulting diff line by line, and can explain and maintain every change in this PR.
Checklist
CONTRIBUTING.md, and this contribution follows the direct-PR or issue-first policy.any N/A checks above.