Protect stdout and stderr so JavaScript code can't accidentally write to stdout corrupting ink rendering#13247
Protect stdout and stderr so JavaScript code can't accidentally write to stdout corrupting ink rendering#13247
Conversation
Summary of ChangesHello @jacob314, 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 significantly enhances the stability and integrity of the terminal UI by preventing arbitrary JavaScript code from directly writing to standard output and error streams. By introducing a robust interception layer for 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. 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 introduces a robust mechanism to protect stdout and stderr from accidental writes that could corrupt the Ink-rendered UI. This is achieved by monkey-patching process.stdout.write and process.stderr.write to redirect output through a new centralized stdio utility. The changes are well-structured and significantly improve the application's resilience against UI corruption from stray logs.
The refactoring of console.* patching and debugLogger to use a coreEvents bus is a great architectural improvement, decoupling logging from the UI. Similarly, updating the DebugProfiler to rely on Ink's onRender callback instead of its own stdout patch makes frame counting more accurate.
I have one high-severity concern regarding the implementation in stdio.ts, where the patched stream write methods do not correctly handle backpressure. This could lead to performance issues or excessive memory usage under certain conditions. Please see the detailed comment.
|
Size Change: +5.33 kB (+0.03%) Total Size: 21.1 MB
ℹ️ View Unchanged
|
jacob314
force-pushed
the
safe_stdout
branch
from
1fbc4be to
29a6067
Compare
jacob314
force-pushed
the
safe_stdout
branch
3 times, most recently
from
f4fcf96 to
3384c63
Compare
| return; | ||
| } | ||
| const now = Date.now(); | ||
| // Simple frame detection logic (a write after at least 16ms is a new frame) |
There was a problem hiding this comment.
this hacky logic depended on stdout as in't needed now that we get actual render events.
|
|
||
| }, [setHistoryRemountKey, isAlternateBuffer, stdout]); | ||
| const handleEditorClose = useCallback(() => { | ||
| if (isAlternateBuffer) { |
There was a problem hiding this comment.
this is the other core change forcing re-enter of alternate buffer mode when the editor is closed as vim exits it.
jacob314
force-pushed
the
safe_stdout
branch
3 times, most recently
from
4789cfc to
4ff11da
Compare
…hem. This avoids rendering artifacts particularly problematic in alternate buffer mode. Fix prop drilling for onEditorClose and call it when we spawn editors from text-buffer as well. refactor: implement backlog for console logs in CoreEventEmitter - Introduce _consoleLogBacklog to buffer console logs before listeners are attached. - Create _emitOrQueue generic helper to handle backlog logic for both feedback and console logs. - Rename drainFeedbackBacklog to drainBacklogs to reflect that it now flushes both backlogs. - Update consumers and tests. feat: add Output event with backlog to CoreEventEmitter - Introduce OutputPayload and CoreEvent.Output. - Add emitOutput method to broadcast stdout/stderr chunks. - Implement _outputBacklog to buffer output when no listeners are present. - Update drainBacklogs to flush output backlog. - Add tests for Output event. refactor: unify event backlogs into a single _eventBacklog in CoreEventEmitter - Replace , , and with . - Store event type and payload in . - Update to push to the single backlog. - Update to process the unified backlog in FIFO order across all event types. refactor: use strict types in CoreEventEmitter backlog to avoid any - Introduce EventBacklogItem discriminated union. - Update _eventBacklog to store EventBacklogItem. - Update _emitOrQueue to use spread arguments matching generic emit. - Update drainBacklogs to use strict cast for emit call.
…alls. Please review.
jacob314
force-pushed
the
safe_stdout
branch
from
4ff11da to
efb6d9e
Compare
…can't accidentally write to stdout corrupting ink rendering (google-gemini#13247)
… to stdout corrupting ink rendering (#13247) Bypassing rules as link checker failure is spurious.
… to stdout corrupting ink rendering (google-gemini#13247) Bypassing rules as link checker failure is spurious.
3 participants
Summary
This avoids rendering artifacts particularly problematic in alternate buffer mode.
Also fix issue where running vim to edit a prompt or confirm a diff was corrupting the alternate buffer. Note that there was a bug and we were cleaning up when vim was used to confirm an edit.
Details
This pr is big but most changes are just fixing tests to expect the more modern libraries rather than watching for stdout.
Test changes can almost all be lightly skimmed as they are purely mechanical with no interesting deltas.
Similarly to how we monkey patched console.log we now monkey patch stdout and stderr. We get clever about ensuring we still write them out to the output when we know we are in non interactive mode and we write them to the debug console if the interactive mode was set.
Simplified console.log monkey patching as the code initially had multiple infinite loop bugs due to complexity of how we were doing this.
Things to test.
non-interactive mode still works with proper stdout even though we are trapping stdout at the beginning. Verify that output still streams fine.
Example show that debug output now gets through to the debug console in gemini cli even if it was rendered before entering the alternate buffer: