Conversation
There was a problem hiding this comment.
Pull request overview
Adds configurable proxy support for the web search toolchain by threading a proxy string from config/env into each search provider and creating HTTP clients/transports that honor SOCKS5 or HTTP(S) proxy settings.
Changes:
- Add
ProxytoWebToolsConfig(JSON + env var) and default it inDefaultConfig(). - Pass the configured proxy into web search providers (Brave/Tavily/DuckDuckGo/Perplexity).
- Introduce
clientForProxy()helper to build anhttp.Clientusing either SOCKS5 or HTTP proxy.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| pkg/tools/web.go | Adds per-provider proxy support and a shared clientForProxy() factory. |
| pkg/config/config.go | Extends WebToolsConfig with Proxy (JSON/env). |
| pkg/config/defaults.go | Sets default Tools.Web.Proxy to empty string. |
| pkg/agent/loop.go | Wires cfg.Tools.Web.Proxy into WebSearchToolOptions. |
Comments suppressed due to low confidence (1)
pkg/agent/loop.go:113
cfg.Tools.Web.Proxyis passed intoNewWebSearchTool, butweb_fetchis still registered with a fixedhttp.Clientthat does not use this proxy config. If the intent is to support proxies for all web tools, consider plumbing the proxy option intoWebFetchToolas well (or clarify the config name/scope).
Proxy: cfg.Tools.Web.Proxy,
}); searchTool != nil {
agent.Tools.Register(searchTool)
}
agent.Tools.Register(tools.NewWebFetchTool(50000))
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| transport.DialContext = func(ctx context.Context, network, addr string) (net.Conn, error) { | ||
| return dialer.Dial(network, addr) | ||
| } | ||
| return &http.Client{Timeout: timeout, Transport: transport}, nil |
There was a problem hiding this comment.
This DialContext implementation ignores the provided ctx and calls dialer.Dial directly. That prevents request cancellation/timeouts from aborting an in-progress SOCKS5 dial, which can cause hangs under network issues. Prefer using a dialer that supports context (e.g., if the returned dialer implements proxy.ContextDialer, use DialContext), or otherwise implement a ctx-aware cancellation wrapper.
| transport.DialContext = func(ctx context.Context, network, addr string) (net.Conn, error) { | ||
| return dialer.Dial(network, addr) | ||
| } |
There was a problem hiding this comment.
Same issue here: DialContext ignores ctx, so cancelling the request context may not stop the SOCKS5 connection attempt. Consider using proxy.ContextDialer when available or otherwise honoring ctx to avoid stuck dials.
| // clientForProxy returns an *http.Client honoring the provided proxy string. | ||
| // Supported formats: | ||
| // - "socks5://host:port" or plain "host:port" (treated as socks5) | ||
| // - any URL with scheme (http://, https://) will be used as an HTTP proxy via ProxyURL | ||
| func clientForProxy(proxyStr string, timeout time.Duration) (*http.Client, error) { | ||
| transport := &http.Transport{ | ||
| TLSHandshakeTimeout: 15 * time.Second, | ||
| MaxIdleConns: 10, | ||
| IdleConnTimeout: 30 * time.Second, | ||
| DisableCompression: false, | ||
| } |
There was a problem hiding this comment.
Proxy support is newly introduced here (via Proxy option and clientForProxy), but the existing pkg/tools/web_test.go suite doesn’t appear to cover any proxy behaviors (e.g., HTTP proxy URL parsing, SOCKS5 dialer selection, or error handling on invalid proxy strings). Adding focused unit tests for clientForProxy (and at least one provider using it) would help prevent regressions.
| PerplexityAPIKey: cfg.Tools.Web.Perplexity.APIKey, | ||
| PerplexityMaxResults: cfg.Tools.Web.Perplexity.MaxResults, | ||
| PerplexityEnabled: cfg.Tools.Web.Perplexity.Enabled, | ||
| Proxy: cfg.Tools.Web.Proxy, |
There was a problem hiding this comment.
The Proxy: field in this struct literal is mis-indented compared to the other fields and will be reformatted by gofmt; if CI enforces formatting, this may fail. Please run gofmt (or fix indentation) so the struct literal field alignment is consistent.
| Proxy: cfg.Tools.Web.Proxy, | |
| Proxy: cfg.Tools.Web.Proxy, |
|
Same as #587 |
📝 Description
add proxy support in web tool
🗣️ Type of Change
🤖 AI Code Generation
🔗 Related Issue
📚 Technical Context (Skip for Docs)
🧪 Test Environment
📸 Evidence (Optional)
Click to view Logs/Screenshots
☑️ Checklist