-
Notifications
You must be signed in to change notification settings - Fork 6.8k
feat(cli): add explicit proxy option in cli #2526
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
feat(cli): add explicit proxy option in cli #2526
Conversation
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.
Summary of Changes
Hello @warjiang, 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 enhances the command-line interface (CLI) by introducing a dedicated --proxy
option. This change provides users with a more direct and explicit way to configure proxy settings for the gemini-client
, allowing them to override system-wide environment variables when needed.
Highlights
- New CLI Option: A
--proxy
command-line option has been added to the CLI, enabling users to specify a proxy address directly. - Explicit Proxy Configuration: Users can now explicitly set a proxy for the
gemini-client
via the command line, offering more granular control over network settings. - Configuration Priority: The newly introduced
--proxy
option takes precedence over existingHTTP(S)_PROXY
environment variables when determining the proxy to use, providing a clear override mechanism.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
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 .gemini/
folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and 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 to provide feedback.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
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
The pull request introduces a new --proxy
CLI option, allowing users to explicitly set a proxy for the gemini-client
. The changes correctly integrate the option into the CLI arguments and prioritize it over environment variables. However, a high
severity issue exists where an explicitly provided empty proxy string via the CLI argument will not override environment variables, leading to unexpected behavior.
11524fa
to
0da6cb0
Compare
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.
Thanks for adding this important feature! The implementation looks good. Before we can merge this, could you please add some tests to verify that the proxy setting is correctly applied? Also, could you please add documentation for the new --proxy option to the docs/cli/configuration.md file? Also the web-fetch tool has it's own fallback http fetch that should be included in these changes.
88aa0c3
to
c0680bf
Compare
thanks for your advice:
|
This is making good progress, but it looks like you have some failing tests, and need to resolve some merge conflicts. |
Yeah, i'm working on it ~ |
aee7385
to
3aebac1
Compare
@allenhutchison I've tested it locally, could plz re-run the ci task? |
Thanks for the great work on this PR, @warjiang! Adding proxy support is a fantastic improvement. I did a deeper dive into the codebase to see if there were any other places that make network requests, and I found a few spots that might also need to be updated to use the new proxy setting. Here are the files I noticed:
Let me know what you think! Happy to discuss further. |
316f835
to
8e3d6c4
Compare
thanks for you comprehensive advice,
|
ce7ad18
to
7cb4971
Compare
7cb4971
to
e02ac39
Compare
@allenhutchison could you help me recheck the PR. BTW the gemini-bot label the PR with |
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.
Hi, I saw your related PR at #3017. The functionality of these two PRs is indeed repetitive, but I prefer the implementation in 3017, and I've left some comments for you. 😂
getProxyAgent() { | ||
const proxyUrl = this.config?.getProxy(); | ||
if (!proxyUrl) return undefined; | ||
if (proxyUrl.startsWith('socks')) { |
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.
- Both http and https protocols can be handled in HttpsProxyAgent
- The
ProxyAgent
provided byundici
currently does not support the socks type. If you want to add support for the socks type proxy, it is not enough to handle it here.
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.
thanks for pointing that~(got the new usage of HttpsProxyAgent and HttpProxyAgent 👍 )
here use https
seems not very good, the reason I add the implementation of socks5 protocol here is that, I think the clearcut-logger
is an isolated implementation.
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.
Thanks for your reply.
What I mean is, if socks
support is to be added, then all requests within the CLI should support socks
proxy, as their proxy source is the same.
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.
@Dcatfly Agree with you, so I co-authored with you to re-use https-proxy-agent
for http/https
proxy protocol, for socks
related protocol, it will report the unsupported error
related commit=> e13ef75#diff-053150b640a7ce75eff69d1a22cae7f0f94ad64ce9a855db544dda0929316519
packages/core/src/tools/web-fetch.ts
Outdated
@@ -80,6 +81,9 @@ export class WebFetchTool extends BaseTool<WebFetchToolParams, ToolResult> { | |||
type: 'object', | |||
}, | |||
); | |||
if (config.getProxy()) { |
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.
setGlobalDispatcher
will intercept all node fetches in the current program.
setGlobalDispatcher
has already been called in core/client.ts, and the timing is early enough. Therefore, the undici-based proxy agent in this PR might be unnecessary.
@@ -237,6 +243,7 @@ export async function loadCliConfig( | |||
}, | |||
checkpointing: argv.checkpointing || settings.checkpointing?.enabled, | |||
proxy: | |||
argv.proxy || |
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.
Is it necessary to add a configuration to set whether the proxy is enabled? Maybe reading from the env is enough.
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.
Is it necessary to add a configuration to set whether the proxy is enabled? Maybe reading from the env is enough.
Use environment variable can works(I've tested it, it worked out ~), but provide explicit option will it easy to understand, also provide extra way to make gemini-cli works with other software, otherwise we need to set and unset the environment variable to achieve that.
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.
You make a good point, but this would require all request-making points in the project to actively add the proxy, including curl
in telemetry_utils
, as well as libraries that might be introduced in the future, or even AI-generated shells. This might require some trade-offs.
const client = new OAuth2Client({ | ||
clientId: OAUTH_CLIENT_ID, | ||
clientSecret: OAUTH_CLIENT_SECRET, | ||
transporter: new Gaxios({ |
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.
If transporter
is not set, OAuth2Client
will actually automatically read proxy settings from the environment. Both methods only support http(s).
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.
OAuth2Client
extends AuthClient
, the transporter
is set in the AuthClient
:
https://github.com/googleapis/google-auth-library-nodejs/blob/bd7e4e3465567bba6f2599f63ac8ed62c0027de9/src/auth/authclient.ts#L246-L275
if unset transporter
for OAuth2Client
, it will init a brand new instance using new Gaxios(opts.transporterOptions)
, but in Gaxios
it will not use environment variable directly.
https://github.com/googleapis/gaxios/blob/cdef1bb3f4f382d4581e21ebebfae2aca2f34029/src/gaxios.ts#L70-L76
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.
In the official doc for the google-auth-library, it is mentioned that proxy settings are obtained from the environment.
The specific code implementation is likely located around here.
@Dcatfly can we cooperate with the proxy feature. Apart from proxy for cli, I think there is need to add proxy feature for telemetry, like init the otel collector from github, report metrics to gcp endpoint, maybe there still need extra work about this. |
c9f8269
to
d545b45
Compare
… & https protocol Co-authored-by: Dcatfly <[email protected]>
Head branch was pushed to by a user without write access
1811937
to
7742d98
Compare
Never mind, I know it's really hard to maintain a popular oss repo, especially the repo also iterated in a high speed. |
@NTaylorMullen all conflicts have been resolved and re-tested in forked repo. Let's move forward ~ |
Co-authored-by: Dcatfly <[email protected]>
Co-authored-by: Dcatfly <[email protected]>
Co-authored-by: Dcatfly <[email protected]>
Co-authored-by: Dcatfly <[email protected]>
Co-authored-by: Dcatfly <[email protected]>
Co-authored-by: Dcatfly <[email protected]>
TLDR
gemini-cli
will reuse environmentHTTP(S)_PROXY
(both in upper and lower case), this PR will--proxy
option to set proxy for gemini-client.Dive Deeper
Reviewer Test Plan
Testing Matrix
Linked issues / bugs