Skip to content

fix: apiCallTransport fallback 应保留环境变量代理#2128

Open
zhoubeiqing wants to merge 3 commits intorouter-for-me:mainfrom
zhoubeiqing:fix/api-call-env-proxy
Open

fix: apiCallTransport fallback 应保留环境变量代理#2128
zhoubeiqing wants to merge 3 commits intorouter-for-me:mainfrom
zhoubeiqing:fix/api-call-env-proxy

Conversation

@zhoubeiqing
Copy link

问题

apiCallTransportapi_tools.go)在凭证和全局配置均未设置代理时,fallback 分支显式将 Proxy 设为 nil,禁用了环境变量代理(HTTP_PROXY / HTTPS_PROXY)。

newProxyAwareWebsocketDialercodex_websockets_executor.go)在相同场景下保留了 http.ProxyFromEnvironment 作为默认行为。

两者行为不一致,导致管理面板通过 /v0/management/api-call 代理的请求(如获取 Codex 额度)在仅配置了环境变量代理的部署环境中无法到达上游服务,触发超时。

修复

当没有显式代理配置时,直接返回 http.DefaultTransport(默认包含 ProxyFromEnvironment),与 WebSocket 执行器保持一致。

- transport, ok := http.DefaultTransport.(*http.Transport)
- if !ok || transport == nil {
-     return &http.Transport{Proxy: nil}
- }
- clone := transport.Clone()
- clone.Proxy = nil
- return clone
+ return http.DefaultTransport

🤖 Generated with Claude Code

当凭证和全局配置均未设置代理时,apiCallTransport 的 fallback
分支显式将 Proxy 设为 nil,禁用了环境变量代理(HTTP_PROXY/
HTTPS_PROXY)。而 codex_websockets_executor 中的
newProxyAwareWebsocketDialer 在相同场景下保留了
http.ProxyFromEnvironment 作为默认行为。

这一不一致导致管理面板通过 /v0/management/api-call 代理的
请求(如获取 Codex 额度)在仅配置了环境变量代理的部署环境中
无法到达上游服务,触发 30 秒超时。

修复方式:当没有显式代理配置时,直接返回 http.DefaultTransport,
保留其默认的 ProxyFromEnvironment 行为,与 WebSocket 执行器
保持一致。

Co-Authored-By: Claude Opus 4.6 <[email protected]>
@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello, 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!

此拉取请求解决了应用程序中代理处理逻辑不一致的问题。它确保了当没有明确配置代理时,apiCallTransport 函数能够正确地利用系统环境变量代理。这一改动使得管理面板的 API 调用在依赖环境变量代理的环境中能够正常运行,从而提高了系统的健壮性和兼容性。

Highlights

  • 代理行为一致性: 修复了 apiCallTransport 在未配置显式代理时,会禁用环境变量代理(HTTP_PROXY / HTTPS_PROXY)的问题,使其行为与 newProxyAwareWebsocketDialer 保持一致。
  • 环境变量代理支持: 确保了管理面板通过 /v0/management/api-call 代理的请求(如获取 Codex 额度)在仅配置了环境变量代理的部署环境中能够正常工作,避免了超时问题。
  • 代码简化: 简化了 apiCallTransport 函数中的代理 fallback 逻辑,直接返回 http.DefaultTransport

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Changelog
  • internal/api/handlers/management/api_tools.go
    • 简化了 apiCallTransport 函数中的代理 fallback 逻辑,直接返回 http.DefaultTransport 以保留环境变量代理行为。
Activity
  • 暂无活动记录。
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 by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

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 pull request 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 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

  1. 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.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

本次变更修复了 apiCallTransport 在没有配置代理时会禁用环境变量代理的问题。通过回退到 http.DefaultTransport,使得环境变量中设置的代理(如 HTTP_PROXY)能够生效,解决了行为不一致的问题。这是一个正确的修复。我另外提供了一个关于代码健壮性的建议,以避免直接使用全局共享实例可能带来的潜在风险。

Copy link
Collaborator

@luispater luispater left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix. The behavior change in apiCallTransport looks directionally correct and matches the websocket dialer by preserving ProxyFromEnvironment when no explicit proxy is configured.

I have one blocking concern before this should merge:

  • Please add a regression test for the exact scenario this PR is fixing: when both auth.ProxyURL and cfg.ProxyURL are empty, apiCallTransport should inherit the environment proxy settings instead of disabling them. Right now internal/api/handlers/management/api_tools_test.go only covers direct and the invalid-auth-falls-back-to-global-proxy path, so the reported bug can regress again without any test failing.

Test plan I ran:

  • go test ./internal/api/handlers/management
  • go test ./internal/runtime/executor
  • go test ./sdk/proxyutil

Once that coverage is in place, this looks good to me.

Copy link
Author

@zhoubeiqing zhoubeiqing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good

Copy link
Author

@zhoubeiqing zhoubeiqing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good

@zhoubeiqing
Copy link
Author

Thanks for the fix. The behavior change in apiCallTransport looks directionally correct and matches the websocket dialer by preserving ProxyFromEnvironment when no explicit proxy is configured.

I have one blocking concern before this should merge:

  • Please add a regression test for the exact scenario this PR is fixing: when both auth.ProxyURL and cfg.ProxyURL are empty, apiCallTransport should inherit the environment proxy settings instead of disabling them. Right now internal/api/handlers/management/api_tools_test.go only covers direct and the invalid-auth-falls-back-to-global-proxy path, so the reported bug can regress again without any test failing.

Test plan I ran:

  • go test ./internal/api/handlers/management
  • go test ./internal/runtime/executor
  • go test ./sdk/proxyutil

Once that coverage is in place, this looks good to me.

@luispater Added the regression test for the no-explicit-proxy case in api_tools_test.go.
When both auth.ProxyURL and cfg.ProxyURL are empty, apiCallTransport now inherits environment proxy settings.
Could you take another look?

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a4bf698a7d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".


rt := h.apiCallTransport(&coreauth.Auth{ProxyURL: ""})
transport, ok := rt.(*http.Transport)
if ok && transport.Proxy != nil {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Fail test when apiCallTransport does not expose env proxy

This guard makes the new regression test pass even when apiCallTransport returns the wrong transport (for example, a non-*http.Transport or a transport with Proxy == nil), because execution falls through to checking http.ProxyFromEnvironment directly instead of asserting on rt. In that scenario, the test validates only the standard library behavior and would miss a real regression in apiCallTransport itself.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants