Bound duplicated text-heavy tool output#85
Conversation
📝 WalkthroughWalkthroughAdds configurable inline output limits, a preview/metadata utility, and bounded response shaping for shell, read, grep, glob, and ls tools. CLI configuration handling, documentation, tests, and widget-aware metadata are updated accordingly. ChangesInline output previews
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/tool-output.ts (1)
64-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUntested fallback branch.
The
maxCharacters <= markerCharactershead-only fallback (reachable with a small but valid configured limit) isn't exercised intool-output.test.ts. Worth a regression test to lock in this edge case, since a small user-configuredinlineOutputCharacterswould hit it silently.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tool-output.ts` around lines 64 - 74, Add a regression test in tool-output.test.ts covering the maxCharacters <= markerCharacters fallback in the tool-output formatting flow. Configure a small valid inlineOutputCharacters limit that triggers the head-only path, then assert the returned text, character counts, omittedCharacters, and truncated flag match the expected behavior.src/server.ts (1)
1028-1058: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRepeated bounded-response assembly across 5 tool handlers.
read(here),grep(1400-1429),glob(1470-1499),ls(1540-1565), andshell(1626-1657) each hand-roll the same sequence: bound error content viaboundResponseContent, then on success buildbounded, mergeboundedSummaryinto a tool-specific summary, calltoolResultMeta, and setstructuredContentviaboundedStructuredContent.processToolResponse(lines 581-609) already shows the pattern of extracting this into one shared helper forexec_command/write_stdin— applying the same treatment here would remove ~5x near-duplicated blocks and centralize future changes to the bounding contract.A shared helper could take
(config, kind, tool, workspaceId, path, response, extraSummaryFields)and return the final{ content, _meta, structuredContent }shape, with each handler only supplying its tool-specific summary fields (pattern/scope, offset/limited, command/workingDirectory, etc.).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server.ts` around lines 1028 - 1058, Extract the repeated bounded-response assembly from the read, grep, glob, ls, and shell handlers into a shared helper, following the existing processToolResponse pattern. Have the helper accept the config, tool kind/name, workspace and path context, response, and tool-specific summary fields, then centralize boundResponseContent, boundedSummary, toolResultMeta, and boundedStructuredContent while preserving each handler’s existing metadata.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/tool-output.ts`:
- Around line 15-93: Optimize createOutputPreview by avoiding repeated
full-string code-point materialization: first use value.length as a safe fast
path when the string is within maxCharacters, otherwise create one reusable
code-point array and use it for the original character count and head/tail
slicing. Update takeHead and takeTail or replace their use within
createOutputPreview so they consume the shared array, while preserving
lineCount, truncation boundaries, and reported counts.
---
Nitpick comments:
In `@src/server.ts`:
- Around line 1028-1058: Extract the repeated bounded-response assembly from the
read, grep, glob, ls, and shell handlers into a shared helper, following the
existing processToolResponse pattern. Have the helper accept the config, tool
kind/name, workspace and path context, response, and tool-specific summary
fields, then centralize boundResponseContent, boundedSummary, toolResultMeta,
and boundedStructuredContent while preserving each handler’s existing metadata.
In `@src/tool-output.ts`:
- Around line 64-74: Add a regression test in tool-output.test.ts covering the
maxCharacters <= markerCharacters fallback in the tool-output formatting flow.
Configure a small valid inlineOutputCharacters limit that triggers the head-only
path, then assert the returned text, character counts, omittedCharacters, and
truncated flag match the expected behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 84b23e28-2318-4415-8b94-1ffa2549c6e9
📒 Files selected for processing (9)
docs/configuration.mdpackage.jsonsrc/cli.tssrc/config.test.tssrc/config.tssrc/server.tssrc/tool-output.test.tssrc/tool-output.tssrc/user-config.ts
| function codePoints(value: string): string[] { | ||
| return Array.from(value); | ||
| } | ||
|
|
||
| function characterLength(value: string): number { | ||
| return codePoints(value).length; | ||
| } | ||
|
|
||
| function lineCount(value: string): number { | ||
| if (value.length === 0) return 0; | ||
| return value.endsWith("\n") | ||
| ? value.slice(0, -1).split("\n").length | ||
| : value.split("\n").length; | ||
| } | ||
|
|
||
| function takeHead(value: string, count: number): string { | ||
| if (count <= 0) return ""; | ||
| return codePoints(value).slice(0, count).join(""); | ||
| } | ||
|
|
||
| function takeTail(value: string, count: number): string { | ||
| if (count <= 0) return ""; | ||
| const characters = codePoints(value); | ||
| return characters.slice(Math.max(0, characters.length - count)).join(""); | ||
| } | ||
|
|
||
| export function createOutputPreview( | ||
| value: string, | ||
| maxCharacters = DEFAULT_INLINE_OUTPUT_CHARACTERS, | ||
| ): OutputPreview { | ||
| if (!Number.isInteger(maxCharacters) || maxCharacters < 1) { | ||
| throw new Error("Inline output limit must be a positive integer."); | ||
| } | ||
|
|
||
| const originalCharacters = characterLength(value); | ||
| const originalLines = lineCount(value); | ||
|
|
||
| if (originalCharacters <= maxCharacters) { | ||
| return { | ||
| text: value, | ||
| originalCharacters, | ||
| originalLines, | ||
| inlineCharacters: originalCharacters, | ||
| omittedCharacters: 0, | ||
| truncated: false, | ||
| }; | ||
| } | ||
|
|
||
| const markerCharacters = characterLength(TRUNCATION_MARKER); | ||
| if (maxCharacters <= markerCharacters) { | ||
| const text = takeHead(value, maxCharacters); | ||
| return { | ||
| text, | ||
| originalCharacters, | ||
| originalLines, | ||
| inlineCharacters: characterLength(text), | ||
| omittedCharacters: originalCharacters - characterLength(text), | ||
| truncated: true, | ||
| }; | ||
| } | ||
|
|
||
| const available = maxCharacters - markerCharacters; | ||
| const headCharacters = Math.ceil(available * 0.65); | ||
| const tailCharacters = available - headCharacters; | ||
| const text = | ||
| takeHead(value, headCharacters) + | ||
| TRUNCATION_MARKER + | ||
| takeTail(value, tailCharacters); | ||
| const inlineCharacters = characterLength(text); | ||
|
|
||
| return { | ||
| text, | ||
| originalCharacters, | ||
| originalLines, | ||
| inlineCharacters, | ||
| omittedCharacters: originalCharacters - headCharacters - tailCharacters, | ||
| truncated: true, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Multiple full-string passes on the truncation hot path.
For large value (the exact case this feature targets — big file reads, verbose grep/ls/shell output), createOutputPreview performs several independent O(n) full-string materializations: characterLength(value) (line 49), lineCount(value)'s .split("\n") (line 50), then takeHead(value, ...) and takeTail(value, ...) each call codePoints(value) again (lines 65/80-82). Each of these builds a brand-new array holding every code point of the entire original string, even though only a ~12,000-character preview is ultimately needed. This scales poorly with input size and works against the PR's own goal of bounding cost for text-heavy tools.
A low-risk improvement: compute the code-point array once and reuse it for length, head, and tail instead of re-deriving it 3+ times; additionally, a fast path using value.length (UTF‑16 length is always ≥ code-point length) can skip the expensive path entirely whenever the string is already within bounds.
♻️ Suggested optimization sketch
function createOutputPreview(
value: string,
maxCharacters = DEFAULT_INLINE_OUTPUT_CHARACTERS,
): OutputPreview {
if (!Number.isInteger(maxCharacters) || maxCharacters < 1) {
throw new Error("Inline output limit must be a positive integer.");
}
- const originalCharacters = characterLength(value);
- const originalLines = lineCount(value);
-
- if (originalCharacters <= maxCharacters) {
+ const originalLines = lineCount(value);
+
+ // Fast path: UTF-16 length is always >= code-point length, so if it already
+ // fits, we know the code-point count fits too without materializing the array.
+ if (value.length <= maxCharacters) {
+ const originalCharacters = characterLength(value);
+ if (originalCharacters <= maxCharacters) {
return {
text: value,
originalCharacters,
originalLines,
inlineCharacters: originalCharacters,
omittedCharacters: 0,
truncated: false,
};
+ }
}
+ const points = codePoints(value); // single materialization, reused below
+ const originalCharacters = points.length;
const markerCharacters = characterLength(TRUNCATION_MARKER);
...
- const text = takeHead(value, headCharacters) + TRUNCATION_MARKER + takeTail(value, tailCharacters);
+ const text =
+ points.slice(0, headCharacters).join("") +
+ TRUNCATION_MARKER +
+ points.slice(Math.max(0, points.length - tailCharacters)).join("");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/tool-output.ts` around lines 15 - 93, Optimize createOutputPreview by
avoiding repeated full-string code-point materialization: first use value.length
as a safe fast path when the string is within maxCharacters, otherwise create
one reusable code-point array and use it for the original character count and
head/tail slicing. Update takeHead and takeTail or replace their use within
createOutputPreview so they consume the shared array, while preserving
lineCount, truncation boundaries, and reported counts.
|
I apologize, my agent got a little trigger happy and sent out a PR without my approval, I told him to fork it not send you a PR. I will continue to work on this but I wasn't planning on sending out a PR until I reached a solid implementation of the issue I was running into. You can ignore or review, up to you. |
1 participant
Problem
Text-heavy tools copied the same output into content, structuredContent.result, and the widget card payload. Large shell and process responses therefore amplified one result across model-visible transcript and UI channels.
Fix
Regression coverage
A deterministic 50000-character response test confirms the previous three-channel payload exceeds 150 KB while the bounded full-widget response stays below 30 KB and below one quarter of the original size.
Validation
Summary by CodeRabbit