refactor(stdio): always patch stdout and use createWorkingStdio for clean output#14159
refactor(stdio): always patch stdout and use createWorkingStdio for clean output#14159allenhutchison merged 13 commits intomainfrom
Conversation
…ation When running `gemini --experimental-acp` as a subprocess (as Zed does), the CLI would produce no output and hang indefinitely. This was caused by `patchStdio()` running at the start of `main()` and redirecting stdout/stderr to internal event handlers, breaking ACP mode which needs raw stdout for JSON-RPC communication. This fix detects the `--experimental-acp` flag early (before argument parsing) using `process.argv` and skips stdio patching when ACP mode is enabled. Fixes #13913
Summary of ChangesHello @allenhutchison, 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 bug where the CLI, when operating in Agent Communication Protocol (ACP) mode as a subprocess, would fail to produce output and hang. The core of the fix involves preventing the CLI's standard I/O patching mechanism from interfering with the raw stdout required for JSON-RPC communication in ACP mode. By introducing an early check for the ACP flag, the system can now conditionally bypass this patching, ensuring seamless operation for integrations that rely on ACP. 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 fix to prevent the CLI from hanging in ACP mode by skipping stdio patching. The approach of detecting the --experimental-acp flag early is sound. However, the detection mechanism is brittle and will fail for common argument patterns like --experimental-acp=true, which would reintroduce the bug. I've suggested a more robust implementation for the flag detection and an accompanying test case to ensure correctness. Addressing this will make the fix more reliable.
|
Size Change: +160 B (0%) Total Size: 21.5 MB ℹ️ View Unchanged
|
Address code review feedback:
- Change from `includes('--experimental-acp')` to
`some(arg => arg.startsWith('--experimental-acp'))` to handle
cases like `--experimental-acp=true`
- Add test case for `--experimental-acp=true`
Handle all yargs boolean flag formats: - `--experimental-acp` (true) - `--experimental-acp=true` (true) - `--experimental-acp=false` (false) - `--experimental-acp=0` (false) Added tests for false value cases.
|
Thanks so much. I ran into this problem with neovim/avante and started to solve it before noticing your work. I have one glitch I'm investigating which might be something my side. Otherwise, looks good for things like permissions flow etc. |
|
sorry to be clear I meant this fix works.. I just meant that gemini acp in general, the features I am using are around permissions flow, etc. |
|
I have an ACP fix waiting for this to be merged first #14190 |
allenhutchison
changed the title
…lean output This changes the approach from "skip patching in ACP mode" to "always patch, but use createWorkingStdio() for components needing clean output." Changes: - Rename createInkStdio → createWorkingStdio - Always call patchStdio() in gemini.tsx (remove ACP conditional) - Use createWorkingStdio().stdout for: - Ink UI rendering - ACP/Zed JSON-RPC output - Non-interactive CLI JSON output - Fix test environment pollution from ANTIGRAVITY_CLI_ALIAS env var Fixes #13913
Resolved conflicts in: - packages/cli/src/gemini.tsx: Always patch stdio, use startupProfiler - packages/cli/src/zed-integration/zedIntegration.ts: Import both createWorkingStdio and startupProfiler
The isAcpModeFromArgs() function was used to conditionally skip patchStdio() in ACP mode. With the new design (always patch, use createWorkingStdio for clean output), this function is no longer needed.
Revert changes to detect-ide.test.ts, clearcut-logger.test.ts, and EditorSettingsDialog snapshot that were fixing environment pollution issues unrelated to the stdio refactoring. Also remove the ACP mode stdio patching tests since patchStdio() is now always called and those tests no longer add value.
allenhutchison
force-pushed
the
fix/acp-stdio-patching
branch
from
e5c6eeb to
39ede22
Compare
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
@allenhutchison Heads up - this fix wasn't included in v0.19.1. The merge went to Tested v0.19.1 and ACP subprocess mode still hangs. See my comment on #13913 for details. |
|
still stuck in loading... guess another weekend with no fix :( |
|
PLEASE, this fix is entirely blocking Zed users for weeks! |
|
/patch preview |
|
✅ Patch workflow(s) dispatched successfully! 📋 Details:
🔗 Track Progress: |
|
🚀 Patch PR Created! 📋 Patch Details:
📝 Next Steps:
🔗 Track Progress: |
|
🚀 Patch Release Started! 📋 Release Details:
⏳ Status: The patch release is now running. You'll receive another update when it completes. 🔗 Track Progress: |
|
❌ Patch Release Failed! 📋 Details:
🔍 Next Steps:
🔗 Troubleshooting: |
|
❌ Patch Release Failed! 📋 Details:
🔍 Next Steps:
🔗 Troubleshooting: |
1 similar comment
|
❌ Patch Release Failed! 📋 Details:
🔍 Next Steps:
🔗 Troubleshooting: |
|
❌ Patch Release Failed! 📋 Details:
🔍 Next Steps:
🔗 Troubleshooting: |
|
has this fix been released in 0.20? it seems that the patch gh action failed |
|
the fix seems to be released in 0.20 |
|
it has, thanks. |
7 participants
Summary
createInkStdio→createWorkingStdiofor claritypatchStdio()and usecreateWorkingStdio()for clean outputProblem
When running
gemini --experimental-acpas a subprocess, the CLI hangs because logging interferes with JSON-RPC communication.Solution
Design pattern: Always patch stdout/stderr to capture logs, but provide
createWorkingStdio()as a bypass mechanism for components that need clean output (JSON-RPC, UI rendering).This is more robust than conditionally skipping patching because:
Changes
packages/core: RenamecreateInkStdio→createWorkingStdiopackages/cli/gemini.tsx: Always callpatchStdio(); usecreateWorkingStdio()for Inkpackages/cli/zedIntegration.ts: UsecreateWorkingStdio()for JSON outputpackages/cli/nonInteractiveCli.ts: UsecreateWorkingStdio()for JSON outputTest plan
patchStdioto be calledFixes #13913