fix: add output truncation to PTY execution path #989
Merged
Conversation
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
Apply the same 16MB sliding-window truncation used by the child_process path to the PTY (node-pty) execution path in ShellExecutionService. The PTY path previously accumulated output without any bounds via outputChunks.push() and headlessTerminal.write(), which could cause unbounded memory growth for commands producing large output. Changes: - Add truncatedOutput string and outputTruncated flag to PTY path - Apply appendAndTruncate() to decoded/stripped chunks - Use truncatedOutput for final result instead of unbounded terminal buffer - Display same truncation warning message as child_process path - Remove unused getFullText helper (now obsolete) closes #914
github-actions
bot
added
the
maintainer:e2e:ok
Contributor
WalkthroughAdds memory-bounded output handling to the PTY execution path using a sliding-window truncation mechanism matching the child_process path. Both paths now enforce a 16MB buffer limit, track truncation state, emit stripped output events, and append a truncation warning to the final result when limits are exceeded. Comprehensive tests validate truncation behavior for both execution methods. Changes
Sequence Diagram(s)sequenceDiagram
participant DataStream as Data Stream
participant Handler as Output Handler
participant Buffer as Truncation Buffer
participant Output as Final Output
Note over DataStream,Output: PTY & child_process unified flow
loop Each data chunk
DataStream->>Handler: emit chunk (raw)
Handler->>Handler: strip ANSI codes
Handler->>Buffer: appendAndTruncate(stripped)
alt Buffer size ≤ MAX_SIZE
Buffer-->>Handler: accepted
else Buffer size > MAX_SIZE
rect rgb(255, 200, 150)
Note over Buffer: sliding window truncate
Buffer-->>Handler: truncated (flag: true)
end
end
Handler->>Output: emit strippedChunk
end
rect rgb(200, 220, 255)
Note over Output: Finalization
alt outputTruncated == true
Handler->>Output: append "[LLXPRT_CODE_WARNING: ...]"
end
end
Output-->>Output: return final result with bounded output
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (4 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (1)**/*.{ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (4)📓 Common learnings📚 Learning: 2026-01-03T00:20:08.040ZApplied to files:
📚 Learning: 2026-01-03T00:20:08.040ZApplied to files:
📚 Learning: 2025-11-16T22:51:26.374ZApplied to files:
⏰ Context from checks skipped due to timeout of 270000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
🔇 Additional comments (8)
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 |
Contributor
|
I'll analyze this PR systematically, focusing on the PTY output truncation fix. Let me examine the shell execution service and tests to evaluate the implementation. LLxprt PR Review – PR #989Issue Alignment
Side Effects
Code Quality
Tests & Coverage
Verdict |
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-24.x-ubuntu-latest' artifact from the main CI run. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
This PR fixes issue #914 by adding output truncation to the PTY (node-pty) execution path in
ShellExecutionService.Problem
The PTY execution path (
executeWithPty) accumulated output without any bounds:outputChunks.push(data)grew without limit and was laterBuffer.concat()'dheadlessTerminal.write(...)fed the xterm state indefinitelygetFullText(headlessTerminal)returned the full terminal bufferThis caused unbounded memory growth for commands producing large output when
shouldUseNodePtyShellwas enabled, while thechild_processfallback path properly capped output at 16MB usingappendAndTruncate().Solution
Apply the same sliding-window truncation used by the
child_processpath:Added truncation tracking variables to the PTY path:
truncatedOutput: Holds the bounded string outputoutputTruncated: Boolean flag tracking if truncation occurredApplied
appendAndTruncate()to decoded/stripped chunks in the PTYhandleOutputfunction, capping atMAX_CHILD_PROCESS_BUFFER_SIZE(16MB)Updated
finalize()to usetruncatedOutputinstead of the unboundedgetFullText(headlessTerminal)Added truncation warning matching the
child_processpath when truncation occurs:Removed unused
getFullTexthelper (now obsolete since we no longer extract from the terminal buffer)Testing
Added a new test
should truncate PTY output using a sliding window and show a warningthat:All existing tests pass.
Verification
npm run lintpassesnpm run typecheckpassesnpm run formatpassesnpm run buildpassesnpm run testpassesnode scripts/start.js --profile-load synthetic "write me a haiku"worksRelated Issues
Closes #914
Notes
The deepthinker subagent reviewed this implementation and approved with minor notes:
outputChunks) is still unbounded (same aschild_processpath) - this is intentional as the raw buffer is used for binary detectionchild_processbehavior)These are existing design decisions that could be addressed in follow-up issues if needed.
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.