Respect Claude OAuth app quota#40
Conversation
📝 WalkthroughWalkthroughAdds a new "oauth-apps-weekly" Claude usage window (construction, detection, labeling, grid column, and scoring) across ChangesClaude OAuth apps weekly usage window
Strip outbound forwarding headers in proxy
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Proxy as HTTPProxy
participant Upstream
Client->>Proxy: Request with Forwarded/X-Forwarded-*/X-Real-IP headers
Proxy->>Proxy: Rewrite: set upstream URL
Proxy->>Proxy: stripOutboundForwardingHeaders(req.Header)
Proxy->>Upstream: Forward request without client IP headers
Upstream-->>Proxy: Response
Proxy-->>Client: Response
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2770090d7f
ℹ️ 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".
| Rewrite: func(pr *httputil.ProxyRequest) { | ||
| pr.SetURL(upstream) |
There was a problem hiding this comment.
Switching this handler to ReverseProxy.Rewrite changes query forwarding semantics: httputil cleans Out.URL.RawQuery before this callback runs, whereas the previous Director-based NewSingleHostReverseProxy path preserved the raw query here. For proxied API requests whose query contains a raw semicolon, such as cursor/filter values that were not percent-encoded, Go treats that parameter as unparsable and it is now dropped before reaching the upstream. If the goal is only to suppress forwarding headers, copy the inbound raw query back after SetURL (or otherwise preserve it) so the proxy remains transparent for those requests.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/subrouter/sr.go (1)
1269-1301: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftDuplicate
claudeUsageWindowsimplementation risks drift.This closure is now byte-for-byte equivalent to
claudeUsageWindowsininternal/proxy/proxy.go(lines 687-730, see relevant snippet), down to thewindowSecondsargument, the fixed 5h/7d constants, and theoauth-apps-weeklyentry added by this very PR. Having two copies of this logic means every future window addition (or bucket removal) must be manually kept in sync acrosscmd/subrouter/sr.goandinternal/proxy/proxy.go— exactly the kind of drift this PR is fixing (the routing side started respecting the OAuth-apps bucket whilesr statusdisplay lagged behind). Consider moving this into a shared package (e.g.internal/accountsor a newinternal/agents/claudehelper) that bothsr.goandproxy.goimport.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/subrouter/sr.go` around lines 1269 - 1301, The `claudeUsageWindows` logic in `cmd/subrouter/sr.go` is duplicated from the same helper in `internal/proxy/proxy.go`, so future usage-window changes will drift between the two call sites. Extract the shared conversion logic into a common helper in a reusable package such as `internal/accounts` or `internal/agents/claude`, then update both `claudeUsageWindows` and the proxy-side caller to use that single implementation while preserving the existing window names, constants, and `ExtraUsage` handling.
🧹 Nitpick comments (1)
internal/proxy/claude_ratelimit_routing_test.go (1)
141-202: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider adding WebSocket coverage for header stripping.
This test only covers the HTTP proxy path.
proxyWebSocketalso callsstripOutboundForwardingHeaders(proxy.go line 1594) for the dial headers, but that path isn't exercised by a test here. A WebSocket-based variant (dialing through the subrouter to a mock upstream WS server and inspecting the handshake headers it receives) would close this gap.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/proxy/claude_ratelimit_routing_test.go` around lines 141 - 202, The current coverage only verifies header stripping in the HTTP proxy path; `proxyWebSocket` also relies on `stripOutboundForwardingHeaders`, but that behavior is untested. Add a WebSocket test alongside `TestHTTPProxyDoesNotForwardClientIPHeaders` that dials through `Server.Handler()` to a mock upstream WS server, inspects the handshake headers received by the upstream, and asserts the same client IP forwarding headers are removed. Use `proxyWebSocket` and `stripOutboundForwardingHeaders` as the key symbols to locate the affected path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@cmd/subrouter/sr.go`:
- Around line 1269-1301: The `claudeUsageWindows` logic in `cmd/subrouter/sr.go`
is duplicated from the same helper in `internal/proxy/proxy.go`, so future
usage-window changes will drift between the two call sites. Extract the shared
conversion logic into a common helper in a reusable package such as
`internal/accounts` or `internal/agents/claude`, then update both
`claudeUsageWindows` and the proxy-side caller to use that single implementation
while preserving the existing window names, constants, and `ExtraUsage`
handling.
---
Nitpick comments:
In `@internal/proxy/claude_ratelimit_routing_test.go`:
- Around line 141-202: The current coverage only verifies header stripping in
the HTTP proxy path; `proxyWebSocket` also relies on
`stripOutboundForwardingHeaders`, but that behavior is untested. Add a WebSocket
test alongside `TestHTTPProxyDoesNotForwardClientIPHeaders` that dials through
`Server.Handler()` to a mock upstream WS server, inspects the handshake headers
received by the upstream, and asserts the same client IP forwarding headers are
removed. Use `proxyWebSocket` and `stripOutboundForwardingHeaders` as the key
symbols to locate the affected path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: df5eb648-2a8d-462b-be2c-9d65644517a7
📒 Files selected for processing (4)
cmd/subrouter/sr.gocmd/subrouter/sr_test.gointernal/proxy/claude_ratelimit_routing_test.gointernal/proxy/proxy.go
1 participant
Summary
Tests
Live logs showed Anthropic rejecting claude requests on anthropic-ratelimit-unified-7d_oi while sr only displayed the ordinary weekly bucket.
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Respect Claude OAuth app weekly quota in routing and status to prevent 429s when the OAuth bucket is exhausted. Also stop forwarding client IP headers to upstream for cleaner attribution and privacy.
SevenDayOAuthAppswindow in scheduling and scoring, with a 7-day duration; route away when saturated.srstatus: new "OAuth wk" column and "OAuth apps (weekly)" label.Written for commit 2770090. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes