fix: enhance isBinary function to reduce false positives in PTY mode - #25191
fix: enhance isBinary function to reduce false positives in PTY mode#25191khairul111010 wants to merge 1 commit into
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical issue where interactive shell commands on Windows return empty output due to false-positive binary detection. By introducing PTY-aware heuristics that account for ANSI escape sequences and adjusting the null-byte detection threshold, the changes ensure that legitimate text output from PTY streams is correctly identified while maintaining strict binary detection for standard file operations. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves binary data detection for PTY output by implementing ANSI escape sequence stripping and a ratio-based null-byte threshold in the isBinary utility. These changes address false positives encountered in Windows PTY streams. The review feedback identifies a missing import in the new test suite and recommends requiring more than one null byte for PTY-mode detection to avoid false positives on short strings.
| describe('isBinary', () => { | ||
| describe('non-PTY mode (default)', () => { | ||
| it('should return false for null/undefined input', () => { | ||
| expect(isBinary(null)).toBe(false); |
There was a problem hiding this comment.
The isBinary function is used throughout this new test suite, but it has not been added to the imports at the top of the file (line 8). This will cause the tests to fail with a ReferenceError when executed.
References
- Before flagging a dependency as missing, verify that it does not already exist in the codebase.
| nullCount++; | ||
| } | ||
| } | ||
| return nullCount / sample.length > NULL_BYTE_THRESHOLD; |
There was a problem hiding this comment.
The 10% null-byte threshold (0.1) is quite strict for short PTY outputs. For example, a single stray null byte in a 6-byte string (as seen in the test case on line 233 of textUtils.test.ts) results in a ratio of ~16.7%, which exceeds the threshold and will cause isBinary to return true. This likely causes that specific test case to fail.
Consider requiring more than one null byte to trigger the ratio-based detection in PTY mode to better handle short, noisy streams.
| return nullCount / sample.length > NULL_BYTE_THRESHOLD; | |
| return nullCount > 1 && nullCount / sample.length > NULL_BYTE_THRESHOLD; |
gemini-cli
Bot
added
the
area/core
gemini-cli
Bot
added
the
help wanted
gemini-cli
Bot
added
the
priority/p1
|
already merged #26565 for the same thing |
3 participants
Summary
Fix
isBinary()false-positive on Windows PTY streams that causesrun_shell_commandto always return empty output. Whennode-ptyis used on Windows, ANSI/VT control sequences containing null bytes trigger the binary detection heuristic, halting the output stream before any content reaches the model. This makes the entire agentic workflow non-functional on Windows with interactive shell enabled.Details
The root cause is in
isBinary()(packages/core/src/utils/textUtils.ts), which treats any single null byte in the first 512 bytes as binary data. While correct for files, this is a false positive for PTY streams —node-ptyon Windows emits VT/ANSI escape sequences that can contain null bytes at the start of every output stream.Changes made:
packages/core/src/utils/textUtils.tsstripAnsiFromBuffer()helper that strips CSI, OSC, and simple two-byte ANSI escape sequences from raw byte buffers.isBinary()with an optionalisPtyOutputparameter (defaultfalse). Whentrue, ANSI sequences are stripped before checking, and a 10% null-byte ratio threshold is used instead of failing on a single null byte.isPtyOutput=false) is completely unchanged — backward compatible.packages/core/src/services/shellExecutionService.tsexecuteWithPty()(line 1131): changedisBinary(sniffBuffer)→isBinary(sniffBuffer, 512, true)to opt into PTY-aware detection.childProcessFallback()is unchanged — it uses pipes, not PTY, so no ANSI false positives occur there.packages/core/src/utils/textUtils.test.tsisBinaryto the import.Related Issues
Fixes #25164
How to Validate
On Windows with
enableInteractiveShell: true:Expected: Command output (
hello) is returned correctly instead of empty output.Verify binary detection still works (any platform):
Expected:
[Binary output detected. Halting stream...]message appears as before.Run the test suite:
cd packages/core npx vitest run src/utils/textUtils.test.tsExpected: All existing tests pass, plus the new
isBinarytests pass.Edge cases to verify:
child_processpath → unchanged strict behaviorPre-Merge Checklist