-
Notifications
You must be signed in to change notification settings - Fork 8.5k
fix(cli): Banish trailing input when using -p in loops (#10556) #10610
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
fix(cli): Banish trailing input when using -p in loops (#10556) #10610
Conversation
Only read stdin when no explicit prompt provided via CLI arguments.
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 @maruthi-raj, 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 resolves a critical bug in the 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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request correctly addresses an issue where the CLI would consume stdin when used with prompt flags inside a shell loop. The approach of checking for an explicit prompt before reading from stdin is sound. However, the implementation has a subtle bug: it doesn't correctly handle cases where an empty prompt is provided (e.g., -p ""
). In these scenarios, the CLI incorrectly determines that no prompt was given and consumes stdin. I've provided two high-severity comments with suggestions to fix this logic in both places it appears.
packages/cli/src/gemini.tsx
Outdated
const hasExplicitPrompt = !!( | ||
argv.prompt || | ||
argv.promptInteractive || | ||
argv.query | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check for an explicit prompt is not quite correct. If a user provides an empty prompt (e.g., gemini -p ""
), argv.prompt
will be ""
, which is falsy. This will cause hasExplicitPrompt
to be false
, and the CLI will incorrectly attempt to read from stdin
. The check should verify the argument was provided, regardless of its value.
Additionally, checking argv.query
is redundant because parseArguments
ensures that if argv.query
is present, either argv.prompt
or argv.promptInteractive
will also be set.
const hasExplicitPrompt =
argv.prompt !== undefined || argv.promptInteractive !== undefined;
packages/cli/src/gemini.tsx
Outdated
if (!process.stdin.isTTY && !input) { | ||
const stdinData = await readStdin(); | ||
if (stdinData) { | ||
input = `${stdinData}\n\n${input}`; | ||
input = stdinData; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check using !input
has a bug. If a user provides an empty prompt like gemini -p ""
, config.getQuestion()
will return ""
, making !input
true and causing the CLI to incorrectly consume stdin
. To fix this, we should explicitly check argv
for the presence of prompt-related arguments, similar to the logic in the sandboxing path.
const hasExplicitPrompt =
argv.prompt !== undefined || argv.promptInteractive !== undefined;
if (!process.stdin.isTTY && !hasExplicitPrompt) {
const stdinData = await readStdin();
if (stdinData) {
input = stdinData;
}
}
Fix: Prevent stdin consumption when explicit prompts provided via CLI args
Only read stdin when no explicit prompt provided via CLI arguments.
TLDR
Fixes the "banishing trailing input" issue where using
-p
option in shell loops caused subsequent input to disappear. The CLI now only reads from stdin when no explicit prompt is provided via command-line arguments (-p
,-i
, or positional args), preventing it from consuming piped input that should be available to subsequent commands in loops.Dive Deeper
Problem: When running Gemini CLI with the
-p
option inside shell loops that process piped input, the CLI was consuming stdin even though a prompt was explicitly provided via command-line arguments. This caused trailing input to "disappear" because the CLI consumed it unnecessarily.Example of the bug: