Skip to content

fix: use per-account proxy_url for OAuth token refresh#1947

Open
lichengzhe wants to merge 2 commits intorouter-for-me:mainfrom
lichengzhe:fix/oauth-refresh-proxy-url
Open

fix: use per-account proxy_url for OAuth token refresh#1947
lichengzhe wants to merge 2 commits intorouter-for-me:mainfrom
lichengzhe:fix/oauth-refresh-proxy-url

Conversation

@lichengzhe
Copy link

@lichengzhe lichengzhe commented Mar 8, 2026

Summary

  • ClaudeExecutor.Refresh() now merges auth.ProxyURL (from credential JSON proxy_url) into config when global proxy-url is empty, so per-account proxy takes effect for token refresh
  • conductor.refreshAuth() logs refresh failures at WARN level instead of DEBUG, making them visible without debug: true
  • config.example.yaml documents proxy resolution priority and the caveat that OAuth refresh's custom TLS transport does not read HTTPS_PROXY env vars

Problem

When deploying on a VPS with proxy routing via environment variables (HTTPS_PROXY), API requests work correctly because newProxyAwareHTTPClient uses standard http.Transport which reads env vars. However, OAuth token refresh uses NewAnthropicHttpClient with a custom utls transport that does not read env vars — it only reads cfg.SDKConfig.ProxyURL.

If config.yaml has no proxy-url field (relying on env vars) and the credential file has proxy_url set, Refresh() ignores auth.ProxyURL and creates a direct-connecting client. This causes silent refresh failures — tokens expire and auto-refresh silently fails because errors are logged at DEBUG level (invisible with debug: false).

Proxy resolution priority for API requests vs token refresh before this fix:

Source API requests Token refresh
auth.ProxyURL (credential file) ✅ Priority 1 ❌ Ignored
cfg.ProxyURL (config.yaml) ✅ Priority 2 ✅ Used
HTTPS_PROXY env var ✅ Fallback ❌ Not read by utls

After this fix, token refresh also respects auth.ProxyURL.

Changes

File Change
internal/runtime/executor/claude_executor.go Merge auth.ProxyURL into config copy when global proxy-url is empty
sdk/cliproxy/auth/conductor.go Log refresh failures at WARN instead of DEBUG
config.example.yaml Document proxy resolution priority
.github/workflows/build-custom.yml CI workflow for fork branch builds

Test plan

  • Deploy with proxy-url empty in config.yaml, proxy_url set in credential JSON → verified token refresh succeeds through proxy
  • Deploy with global proxy-url set in config.yaml → verified it still takes effect (no regression)
  • Set debug: false and trigger a refresh failure → verified warning appears in logs

🤖 Generated with Claude Code

@gemini-code-assist
Copy link
Contributor

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

Copy link

@xkonjin xkonjin left a comment

Choose a reason for hiding this comment

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

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.

lichengzhe and others added 2 commits March 8, 2026 16:12
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>
@lichengzhe lichengzhe force-pushed the fix/oauth-refresh-proxy-url branch from efb25c2 to 616dea5 Compare March 8, 2026 08:12
@lichengzhe
Copy link
Author

@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:

File Change
claude_executor.go Merge auth.ProxyURL into config when global proxy-url is empty
conductor.go Elevate refresh failure logs from DEBUGWARN
config.example.yaml Document proxy resolution priority
build-custom.yml Fork CI workflow (unrelated but lightweight)

Re: unhappy path testing

The core change in claude_executor.go is a 7-line config-copy pattern — when auth.ProxyURL != "" and cfg.SDKConfig.ProxyURL == "", a shallow copy is made with the proxy field set. The unhappy paths are:

  • auth.ProxyURL empty → no-op, original cfg used (existing behavior)
  • cfg.SDKConfig.ProxyURL already set → global takes precedence, no copy made
  • Both set → global wins (intentional, documented in config.example.yaml)

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 conductor.go change is log-level only (no behavioral change), and config.example.yaml is documentation. Neither introduces new failure modes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove this file.

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.

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.

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.

3 participants