fix(cli): prevent ACP stdout pollution from SessionEnd hooks#26125
fix(cli): prevent ACP stdout pollution from SessionEnd hooks#26125
Conversation
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 an issue where debug logs were polluting stdout during the shutdown sequence of ACP sessions. By ensuring the console remains patched until after session hooks have completed, the integrity of the NDJSON protocol is preserved. 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
|
|
Size Change: +38 B (0%) Total Size: 33.9 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Code Review
This pull request introduces logic to skip the registration of ConsolePatcher cleanup when the CLI is operating in ACP mode, accompanied by new unit tests to verify this behavior. Feedback indicates that the manual flag check for ACP mode duplicates existing logic and should be replaced by the centralized Config object's state to prevent potential inconsistencies.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request modifies the CLI's main entry point to conditionally register the ConsolePatcher cleanup based on whether ACP mode is active. It also introduces unit tests to verify this behavior and adds necessary mocks for the ConsolePatcher and ACP client. Feedback suggests that the current placement of the cleanup registration may still lead to stdout pollution in certain modes (such as JSON output) because it occurs before the SessionEnd hook registration; moving the cleanup registration after the hook is recommended to ensure shutdown logs are correctly redirected to stderr.
…correct cleanup order
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the cleanup logic in the Gemini CLI to conditionally register the ConsolePatcher cleanup. Specifically, it ensures that the cleanup is only registered when not in ACP mode, which helps maintain correct log redirection to stderr during shutdown. Corresponding unit tests have been added to verify this behavior, and the mock configuration has been cleaned up by removing the unused getToolRegistry method. I have no feedback to provide as the changes are clear and well-tested.
Summary
In ACP mode, stdout can be polluted by non-JSON debug lines when project hooks are enabled and the ACP session closes. This happens because the global ConsolePatcher is unpatched before the SessionEnd hooks have finished executing.
This PR ensures that in ACP mode, the ConsolePatcher is not unpatched during the final cleanup sequence. This keeps debug logs redirected to stderr and maintains NDJSON protocol integrity on stdout.
Changes
packages/cli/src/gemini.tsxto skipConsolePatcher.cleanup()registration when in ACP mode.packages/cli/src/acp/acpConsolePatcher.test.tsto verify the conditional cleanup logic.Verification
Fixes #25345