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.
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(): merge auth.ProxyURL into config when global proxy-url is not set, so per-account proxy takes effect for refresh - 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 <noreply@anthropic.com>
Builds linux/amd64 binary on push to fix/* and feat/* branches. Uploads as GitHub Actions artifact for easy deployment to VPS. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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 |
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.
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