Add SubscriptionExhausted hook for auth rotation - #1
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between cb6533a211b6a180d914c0b7df0bc1029e54ebca and b860208. ⛔ Files ignored due to path filters (7)
📒 Files selected for processing (38)
📝 WalkthroughWalkthroughA new subrouter rotation feature is introduced that automatically rotates authentication credentials when specific quota or usage limit errors occur. The feature includes error detection, rotation logic via subprocess execution, integration into the turn processing error handler, and comprehensive integration tests with environment variable management. ChangesSubrouter Rotation Feature
Sequence DiagramsequenceDiagram
actor Client
participant Turn as run_turn()
participant SubRouter as subrouter_rotation
participant SubProcess as Subrouter CMD
participant AuthMgr as AuthManager
Client->>Turn: Request processing
Turn->>Turn: Sample/get response
Turn-->>Turn: HTTP 429 (UsageLimitReached)
Turn->>SubRouter: should_rotate_for_error()
SubRouter-->>Turn: true (matches UsageLimitReached)
Turn->>SubRouter: rotate_if_available()
SubRouter->>AuthMgr: Check auto-rotate enabled<br/>& eligible auth
AuthMgr-->>SubRouter: Valid cached auth
SubRouter->>SubRouter: Snapshot current auth state
SubRouter->>SubProcess: Spawn subrouter (30s timeout)
SubProcess-->>SubRouter: Updated auth in CODEX_HOME
SubRouter->>AuthMgr: Reload auth from storage
AuthMgr-->>SubRouter: New auth state
SubRouter-->>Turn: Ok(true, auth changed)
Turn->>Turn: Reset websocket session
Turn->>Turn: Emit rotation success warning
Turn->>Turn: continue (retry turn loop)
Turn->>Client: New request with rotated token
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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. Review rate limit: 0/1 reviews remaining, refill in 42 minutes and 35 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@codex-rs/core/src/subrouter_rotation.rs`:
- Around line 56-62: After reload(), do not treat any changed auth snapshot as a
successful rotation without enforcing the AuthManager's
forced_chatgpt_workspace_id restriction: after obtaining auth_after via
auth_manager.auth_cached(), check the manager's forced_chatgpt_workspace_id (or
call an existing AuthManager method that indicates a forced workspace) and
compare it to the workspace/account identifier in auth_after (or compare to the
workspace in the original before snapshot); if the forced value exists and the
workspace changed to a different id, treat this as not a valid rotation and
return Ok(false); otherwise proceed with the existing auth_snapshot(&auth_after)
!= before comparison.
- Around line 65-72: The auto_rotate_enabled() function currently defaults to
true when the CODEX_SUBROUTER_AUTO_ROTATE env var is unset, causing unexpected
side effects; change its behavior so rotation is opt-in by returning false on
Err(_) (unset/missing) and only returning true when the env var explicitly
contains a truthy value (e.g., not "0"/"false"/"no"/"off")—update the match in
auto_rotate_enabled() to treat Err(_) as false so rotation only occurs when the
environment variable is explicitly set to an enabled value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 43f2c702-62a8-4bfd-99d0-651445f17af3
📥 Commits
Reviewing files that changed from the base of the PR and between c8c30d9 and cb6533a211b6a180d914c0b7df0bc1029e54ebca.
📒 Files selected for processing (6)
codex-rs/core/src/lib.rscodex-rs/core/src/session/turn.rscodex-rs/core/src/subrouter_rotation.rscodex-rs/core/src/thread_manager.rscodex-rs/core/tests/suite/quota_exceeded.rscodex-rs/login/src/auth/manager.rs
| auth_manager.reload().await; | ||
|
|
||
| let Some(auth_after) = auth_manager.auth_cached() else { | ||
| return Ok(false); | ||
| }; | ||
|
|
||
| Ok(auth_snapshot(&auth_after) != before) |
There was a problem hiding this comment.
Preserve forced workspace restrictions after rotation.
This path accepts any changed auth snapshot after reload(), but AuthManager can be configured with forced_chatgpt_workspace_id. That means a quota-triggered Subrouter switch can move the session onto a different workspace/account even though codex-rs/login/src/auth/manager.rs:1842-1851 explicitly rejects that case for other refresh flows. Please make the reload here restriction-aware before treating the rotation as successful and retrying the turn.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@codex-rs/core/src/subrouter_rotation.rs` around lines 56 - 62, After
reload(), do not treat any changed auth snapshot as a successful rotation
without enforcing the AuthManager's forced_chatgpt_workspace_id restriction:
after obtaining auth_after via auth_manager.auth_cached(), check the manager's
forced_chatgpt_workspace_id (or call an existing AuthManager method that
indicates a forced workspace) and compare it to the workspace/account identifier
in auth_after (or compare to the workspace in the original before snapshot); if
the forced value exists and the workspace changed to a different id, treat this
as not a valid rotation and return Ok(false); otherwise proceed with the
existing auth_snapshot(&auth_after) != before comparison.
| fn auto_rotate_enabled() -> bool { | ||
| match std::env::var(AUTO_ROTATE_ENV) { | ||
| Ok(value) => { | ||
| let value = value.trim().to_ascii_lowercase(); | ||
| !matches!(value.as_str(), "0" | "false" | "no" | "off") | ||
| } | ||
| Err(_) => true, | ||
| } |
There was a problem hiding this comment.
Default Subrouter rotation to opt-in.
When CODEX_SUBROUTER_AUTO_ROTATE is unset, this still returns true, so any quota/usage-limit error on ChatGPT auth will try to execute sr if it happens to be on PATH. That is a surprising external side effect on a production error path and makes behavior depend on the host environment rather than explicit config.
Suggested fix
fn auto_rotate_enabled() -> bool {
match std::env::var(AUTO_ROTATE_ENV) {
Ok(value) => {
let value = value.trim().to_ascii_lowercase();
!matches!(value.as_str(), "0" | "false" | "no" | "off")
}
- Err(_) => true,
+ Err(_) => false,
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn auto_rotate_enabled() -> bool { | |
| match std::env::var(AUTO_ROTATE_ENV) { | |
| Ok(value) => { | |
| let value = value.trim().to_ascii_lowercase(); | |
| !matches!(value.as_str(), "0" | "false" | "no" | "off") | |
| } | |
| Err(_) => true, | |
| } | |
| fn auto_rotate_enabled() -> bool { | |
| match std::env::var(AUTO_ROTATE_ENV) { | |
| Ok(value) => { | |
| let value = value.trim().to_ascii_lowercase(); | |
| !matches!(value.as_str(), "0" | "false" | "no" | "off") | |
| } | |
| Err(_) => false, | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@codex-rs/core/src/subrouter_rotation.rs` around lines 65 - 72, The
auto_rotate_enabled() function currently defaults to true when the
CODEX_SUBROUTER_AUTO_ROTATE env var is unset, causing unexpected side effects;
change its behavior so rotation is opt-in by returning false on Err(_)
(unset/missing) and only returning true when the env var explicitly contains a
truthy value (e.g., not "0"/"false"/"no"/"off")—update the match in
auto_rotate_enabled() to treat Err(_) as false so rotation only occurs when the
environment variable is explicitly set to an enabled value.
cb6533a to
e895b84
Compare
lawrencecchen
changed the title
e895b84 to
1bea7dd
Compare
![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: 1bea7dd173
ℹ️ 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".
| let Some(auth_before) = auth_manager.auth_cached() else { | ||
| return false; | ||
| }; | ||
|
|
||
| if !can_refresh_managed_chatgpt_auth(&auth_before) { | ||
| return false; |
There was a problem hiding this comment.
This branch uses auth_cached() to decide whether a SubscriptionExhausted hook can refresh credentials, but request auth can be overridden by external API-key auth (AuthManager::auth()/get_api_auth_mode() prefer external API key mode). In sessions where a user has both cached ChatGPT auth and an active external API key, quota errors from the API key path will incorrectly run the subscription hook and may emit a misleading “refreshed auth, retrying” retry even though the active auth source cannot be changed by rotating auth.json. Please gate on the effective auth mode (or resolved auth) before attempting the hook/retry path.
Useful? React with 👍 / 👎.
1bea7dd to
b860208
Compare
1 participant
Summary:
SubscriptionExhaustedlifecycle hook.UsageLimitReached/QuotaExceeded, reload managed ChatGPT auth, and retry when at least one hook succeeds.x-codex-turn-staterouting state before the retry sosr pickcan move the next request to a fresh route.Example hook:
Tests:
sr --helpsr pick --helpcargo test -p codex-core --test all quota_exceeded -- --nocapturecargo test -p codex-hooks schema -- --nocapturejust fix -p codex-core -p codex-hooks -p codex-config -p codex-protocol -p codex-app-server-protocol -p codex-analytics -p codex-tui -p codex-app-server