fix: use per-account proxy_url for OAuth token refresh#1947
fix: use per-account proxy_url for OAuth token refresh#1947lichengzhe wants to merge 2 commits intorouter-for-me:mainfrom
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
xkonjin
left a comment
There was a problem hiding this comment.
Quick review pass:
- Main risk area here is auth/session state and stale credential handling.
- Good to see test coverage move with the code; I’d still make sure it exercises the unhappy path around auth/session state and stale credential handling rather than only the happy path.
- Before merge, I’d smoke-test the behavior touched by main.go, config.example.yaml, server.go (+5 more) with malformed input / retry / rollback cases, since that’s where this class of change usually breaks.
efb25c2 to
616dea5
Compare
|
@xkonjin Thanks for the review! Addressed: PR scope cleanup You're right that this PR had too much going on. I've rebased and removed the unrelated auto-update code that was accidentally included — it already has its own PR in #1942. The diff is now +68/-3 across 4 files, focused purely on the proxy fix:
Re: unhappy path testing The core change in
These are straightforward conditional branches. The real validation is deployment-level: token refresh succeeds through the per-account proxy when no global proxy-url is configured. This has been verified on our production VPS. Re: malformed input / retry / rollback The |
.github/workflows/build-custom.yml
Outdated
luispater
left a comment
There was a problem hiding this comment.
Summary
The refresh-path fix is heading in the right direction, but the proxy precedence is still inconsistent with the rest of the codebase and with the new documentation.
Blocking
- internal/runtime/executor/claude_executor.go:584 only applies auth.ProxyURL when cfg.SDKConfig.ProxyURL is empty. That means token refresh still uses the global proxy whenever both are set.
- Normal API requests already prefer auth.ProxyURL over cfg.ProxyURL in internal/runtime/executor/proxy_helpers.go, Auth.ProxyURL is documented as an override, and config.example.yaml now says per-account proxy_url is priority 1.
- As a result, request traffic and refresh traffic can still go through different proxies for the same credential.
Test plan
- Reviewed the diff and compared refresh behavior against the existing request-side proxy resolution.
- Verified CI checks are green.
- Verified local go build succeeds.
ClaudeExecutor.Refresh() only read the global proxy-url from config.yaml when creating the HTTP client for token refresh. When proxy-url was empty (relying on HTTPS_PROXY env var or per-account proxy_url in credential files), the custom utls transport would attempt direct connections since it does not read environment variables. This caused silent token refresh failures in deployments that depend on proxy routing (e.g. VPS → residential IP) — the token would expire and auto-refresh would silently fail because errors were logged at DEBUG level. Changes: - ClaudeExecutor.Refresh(): always prefer auth.ProxyURL over cfg.ProxyURL, matching the priority in proxy_helpers.go (auth > cfg > env) - conductor.refreshAuth(): log refresh failures at WARN level instead of DEBUG, making them visible without enabling debug mode - config.example.yaml: document proxy resolution priority and the caveat that OAuth refresh does not read HTTPS_PROXY env vars Co-Authored-By: Claude Opus 4.6 <[email protected]>
616dea5 to
4e1009f
Compare
Extract ResolveProxyURL() in proxy_helpers.go as the single source of truth for proxy priority (auth > config > env). Both the request path (newProxyAwareHTTPClient) and the refresh path (claude_executor.Refresh) now call this shared function, eliminating the config-copy approach that could diverge from the documented priority. NewClaudeAuth gains an optional proxyURL parameter so callers can pass the pre-resolved URL without mutating the config struct. Addresses review feedback from @luispater on router-for-me#1947. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
|
@luispater Thanks for the detailed review! You're right that the config-copy approach didn't properly align with the request path's proxy resolution. I've pushed a fix that addresses the blocking issue: What changed:
This ensures request traffic and refresh traffic always go through the same proxy for a given credential. Note: |
Summary
auth.ProxyURL(from credential JSONproxy_url) into config when globalproxy-urlis empty, so per-account proxy takes effect for token refreshWARNlevel instead ofDEBUG, making them visible withoutdebug: trueHTTPS_PROXYenv varsProblem
When deploying on a VPS with proxy routing via environment variables (
HTTPS_PROXY), API requests work correctly becausenewProxyAwareHTTPClientuses standardhttp.Transportwhich reads env vars. However, OAuth token refresh usesNewAnthropicHttpClientwith a custom utls transport that does not read env vars — it only readscfg.SDKConfig.ProxyURL.If
config.yamlhas noproxy-urlfield (relying on env vars) and the credential file hasproxy_urlset,Refresh()ignoresauth.ProxyURLand creates a direct-connecting client. This causes silent refresh failures — tokens expire and auto-refresh silently fails because errors are logged atDEBUGlevel (invisible withdebug: false).Proxy resolution priority for API requests vs token refresh before this fix:
auth.ProxyURL(credential file)cfg.ProxyURL(config.yaml)HTTPS_PROXYenv varAfter this fix, token refresh also respects
auth.ProxyURL.Changes
internal/runtime/executor/claude_executor.goauth.ProxyURLinto config copy when global proxy-url is emptysdk/cliproxy/auth/conductor.goconfig.example.yaml.github/workflows/build-custom.ymlTest plan
proxy-urlempty in config.yaml,proxy_urlset in credential JSON → verified token refresh succeeds through proxyproxy-urlset in config.yaml → verified it still takes effect (no regression)debug: falseand trigger a refresh failure → verified warning appears in logs🤖 Generated with Claude Code