fix(auth): implement automatic OAuth token refresh #591
Conversation
Implement automatic token refresh to prevent 401 authentication errors during long-running agent sessions. Changes: - Add token refresh logic to core/auth.py with platform-specific credential handling (Linux, macOS, Windows) - Add proactive token check in client.py before creating sessions - Add 401 error detection and retry in session.py - Add unit tests for token refresh functionality - Add requests dependency for HTTP token refresh calls The implementation: - Reads full OAuth credentials including refreshToken and expiresAt - Proactively refreshes tokens 5 minutes before expiry - Uses Anthropic's OAuth endpoint for token refresh - Saves refreshed tokens back to credential store - Configuration is customizable via environment variables: - CLAUDE_OAUTH_TOKEN_URL - CLAUDE_OAUTH_CLIENT_ID - CLAUDE_TOKEN_REFRESH_BUFFER_SECONDS Fixes AndyMik90#563 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> Signed-off-by: wzm <[email protected]>
|
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds end-to-end OAuth token refresh: new env vars and requests dependency; core.auth gains credential discovery, expiry detection, refresh and cross‑platform persistence; session logic performs guarded refresh on 401s and tests for refresh flows and store interactions. (≤50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Session
participant Auth as "Auth Module"
participant Store as "Credential Store"
participant OAuth as "OAuth Server"
rect rgb(232,245,233)
Note over Client,Auth: Client creation / eager token retrieval
Client->>Auth: create_client() / require_auth_token(verbose=True)
Auth->>Store: get_full_credentials()
Store-->>Auth: credentials (accessToken, refreshToken, expiresAt)
Auth->>Auth: is_token_expired() (apply buffer)
alt token expired
Auth->>OAuth: POST /token (grant_type=refresh_token, refresh_token, client_id)
OAuth-->>Auth: new access_token, refresh_token, expires_in
Auth->>Store: save_credentials(new_creds)
Auth-->>Client: accessToken (refreshed)
else token valid
Auth-->>Client: accessToken (existing)
end
end
rect rgb(255,243,224)
Note over Session,Auth: Reactive refresh on 401 during runtime
Session->>Session: request -> 401 auth error
Session->>Auth: trigger refresh flow
Auth->>Store: get_full_credentials()
Store-->>Auth: credentials (refreshToken)
Auth->>OAuth: POST /token (refresh_token)
alt refresh success
OAuth-->>Auth: new tokens
Auth->>Store: save_credentials(new_creds)
Auth-->>Session: indicate refreshed -> prompt user to retry
else refresh failure
Auth-->>Session: return error/guidance (prompt re-login)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
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 |
Summary of ChangesHello @WaaromZoMoeilijk, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses the issue of OAuth tokens expiring during extended agent sessions, which previously led to disruptive 401 authentication errors. The solution introduces an automatic token refresh mechanism that proactively checks for token expiration and refreshes them using Anthropic's OAuth endpoint. This ensures continuous operation without manual re-authentication, significantly improving the reliability of long-running workflows. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🎉 Thanks for your first PR!
A maintainer will review it soon. Please make sure:
- Your branch is synced with
develop - CI checks pass
- You've followed our contribution guide
Welcome to the Auto Claude community!
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable feature for automatically refreshing OAuth tokens, addressing potential authentication failures during long-running sessions. The implementation is comprehensive, with both proactive and reactive refresh mechanisms, platform-specific credential handling, and a solid suite of new tests.
My review focuses on several key areas. I've identified a critical logic flaw in the error handling that could prevent the refresh mechanism from working as intended, along with a high-severity issue in the token expiration check. I've also pointed out significant code duplication and redundant logic that should be addressed to improve maintainability. Additionally, there are a few medium-severity suggestions to improve logging, code clarity, and adherence to best practices like avoiding local imports. Overall, this is a great addition, and addressing these points will make it more robust and easier to maintain.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
apps/backend/.env.exampleapps/backend/agents/session.pyapps/backend/core/auth.pyapps/backend/core/client.pyapps/backend/requirements.txttests/test_auth_token_refresh.py
🧰 Additional context used
📓 Path-based instructions (6)
apps/backend/.env*
📄 CodeRabbit inference engine (CLAUDE.md)
apps/backend/.env*: Enable Electron MCP for E2E testing by settingELECTRON_MCP_ENABLED=truein.envand starting the Electron app withnpm run dev
Configure memory system credentials inapps/backend/.envand validate withgraphiti_config.py
Files:
apps/backend/.env.example
tests/**
⚙️ CodeRabbit configuration file
tests/**: Ensure tests are comprehensive and follow pytest conventions.
Check for proper mocking and test isolation.
Files:
tests/test_auth_token_refresh.py
apps/backend/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
apps/backend/**/*.py: Always use the Claude Agent SDK (claude-agent-sdkpackage) for all AI interactions, never use the Anthropic API directly
Use thecreate_client()function fromapps/backend/core/client.pyto instantiate Claude SDK clients, not directClaudeSDKClientinitialization
Files:
apps/backend/core/client.pyapps/backend/core/auth.pyapps/backend/agents/session.py
⚙️ CodeRabbit configuration file
apps/backend/**/*.py: Focus on Python best practices, type hints, and async patterns.
Check for proper error handling and security considerations.
Verify compatibility with Python 3.12+.
Files:
apps/backend/core/client.pyapps/backend/core/auth.pyapps/backend/agents/session.py
apps/backend/core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Implement dynamic command allowlisting based on detected project stack using
core/security.pyfor bash command validation
Files:
apps/backend/core/client.pyapps/backend/core/auth.py
apps/backend/core/client.py
📄 CodeRabbit inference engine (CLAUDE.md)
Implement agent-specific tool permissions in the Claude SDK client based on agent role (planner, coder, qa_reviewer, qa_fixer)
Files:
apps/backend/core/client.py
apps/backend/agents/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use Graphiti memory system (
integrations/graphiti/) for cross-session context and knowledge graph management in agents
Files:
apps/backend/agents/session.py
🧠 Learnings (3)
📚 Learning: 2025-12-30T16:38:36.314Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T16:38:36.314Z
Learning: Applies to apps/backend/**/*.py : Always use the Claude Agent SDK (`claude-agent-sdk` package) for all AI interactions, never use the Anthropic API directly
Applied to files:
apps/backend/requirements.txtapps/backend/core/client.py
📚 Learning: 2025-12-30T16:38:36.314Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T16:38:36.314Z
Learning: Applies to apps/backend/core/client.py : Implement agent-specific tool permissions in the Claude SDK client based on agent role (planner, coder, qa_reviewer, qa_fixer)
Applied to files:
apps/backend/requirements.txtapps/backend/core/client.pyapps/backend/agents/session.py
📚 Learning: 2025-12-30T16:38:36.314Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T16:38:36.314Z
Learning: Applies to apps/backend/**/*.py : Use the `create_client()` function from `apps/backend/core/client.py` to instantiate Claude SDK clients, not direct `ClaudeSDKClient` initialization
Applied to files:
apps/backend/core/client.py
🧬 Code graph analysis (3)
tests/test_auth_token_refresh.py (1)
apps/backend/core/auth.py (6)
is_token_expired(277-293)refresh_oauth_token(296-334)get_full_credentials(160-173)save_credentials(342-359)_get_full_credentials_linux(188-210)_save_credentials_linux(362-389)
apps/backend/core/client.py (1)
apps/backend/core/auth.py (4)
get_full_credentials(160-173)is_token_expired(277-293)refresh_oauth_token(296-334)save_credentials(342-359)
apps/backend/agents/session.py (1)
apps/backend/core/auth.py (3)
get_full_credentials(160-173)refresh_oauth_token(296-334)save_credentials(342-359)
🪛 Gitleaks (8.30.0)
apps/backend/.env.example
[high] 29-29: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (6)
apps/backend/requirements.txt (1)
4-4: LGTM - appropriate dependency for OAuth refresh.The
requestslibrary is a reasonable choice for the HTTP calls needed for OAuth token refresh. The version constraint>=2.28.0allows flexibility while ensuring a stable baseline.apps/backend/.env.example (1)
18-34: LGTM - well-documented OAuth configuration.The OAuth token refresh configuration section is clear and appropriately marked as optional for advanced/enterprise use. The static analysis hint flagging line 29 as a "Generic API Key" is a false positive -
9d1c250a-e61b-44d9-88ed-5944d1962f5eis a public OAuth client ID for the Claude Code CLI, not a secret. OAuth public client IDs are intentionally public (used in PKCE flows).apps/backend/agents/session.py (1)
554-594: Approve refresh-on-error flow with note on retry behavior.The token refresh logic is well-structured with appropriate error handling and user guidance. Importantly, after a successful refresh, the code correctly instructs the user to retry rather than automatically retrying, which avoids potential infinite loops or unexpected behavior.
One consideration: the refreshed token is saved but
os.environ["CLAUDE_CODE_OAUTH_TOKEN"]is not updated here (unlike inclient.py), so the user truly must restart or retry for the new token to be picked up by subsequent operations.apps/backend/core/client.py (1)
484-512: LGTM - proactive token refresh implementation.The proactive token refresh before client creation is well-designed:
- ✓ Checks expiration with buffer period (5 min default)
- ✓ Updates
os.environ["CLAUDE_CODE_OAUTH_TOKEN"]on successful refresh (even if credential save fails), ensuring the current session uses the fresh token- ✓ Fails gracefully - if refresh fails, continues with existing token (which may still work or will trigger 401 handling in session.py)
The import inside the function (lines 486-491) is acceptable here to avoid potential circular import issues with
core.auth.apps/backend/core/auth.py (2)
22-35: LGTM - well-structured OAuth configuration constants.The environment-driven configuration with sensible defaults is a good pattern. The reference comments to upstream issues provide helpful context for the OAuth endpoint verification.
362-389: Good security practice with file permissions.The
os.chmod(cred_path, 0o600)ensures credentials are only readable by the owner. The read-modify-write pattern correctly preserves other data in the credentials file.
Fixes based on Gemini Code Assist review: CRITICAL: - session.py: Add return statements after 401 handling to prevent fall-through to general error handler HIGH: - auth.py: Handle expires_at == 0 in is_token_expired() - client.py: Remove duplicate refresh logic, use centralized get_auth_token(verbose=True) instead MEDIUM: - session.py: Move auth imports to top of file (PEP 8) - auth.py: Log exceptions in _get_full_credentials_macos() - auth.py: Add verbose parameter to get_auth_token() for user feedback 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> Signed-off-by: wzm <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
apps/backend/core/auth.py (5)
188-210: Consider extracting shared credential file reading logic.
_get_full_credentials_linuxand_get_full_credentials_windowsshare nearly identical logic—only the credential paths differ. A shared helper would reduce duplication.🔎 Suggested refactor
def _get_full_credentials_from_paths(cred_paths: list[str]) -> dict | None: """Get full credentials from a list of file paths.""" for cred_path in cred_paths: if os.path.exists(cred_path): try: with open(cred_path, encoding="utf-8") as f: data = json.load(f) oauth = data.get("claudeAiOauth", {}) access_token = oauth.get("accessToken") if access_token and access_token.startswith("sk-ant-oat01-"): return { "accessToken": access_token, "refreshToken": oauth.get("refreshToken"), "expiresAt": oauth.get("expiresAt"), } except (json.JSONDecodeError, KeyError): continue return NoneThen simplify the platform functions to just define paths and delegate.
Also applies to: 246-270
288-294: Add a clarifying comment for the milliseconds heuristic.The
expires_at > 1e12check correctly distinguishes milliseconds from seconds timestamps, but this isn't immediately obvious. A brief comment would aid future maintainers.🔎 Suggested improvement
expires_at = credentials.get("expiresAt") if not expires_at or expires_at == 0: return False # Can't determine, assume valid - # expiresAt is in milliseconds + # expiresAt may be in milliseconds or seconds depending on source + # Heuristic: timestamps > 1e12 are milliseconds (1e12 ms ≈ year 2001 in seconds) expires_at_sec = expires_at / 1000 if expires_at > 1e12 else expires_at
393-416: Windows credential file lacks permission restrictions.The Linux implementation sets
os.chmod(cred_path, 0o600)to restrict file access, but the Windows version doesn't apply equivalent ACL restrictions. On multi-user Windows systems, credentials may be readable by other users.🔎 Suggested improvement
Consider using
pywin32oricaclsto restrict file permissions on Windows, or document this as a known limitation. At minimum, the file should be created in a user-specific location (which%USERPROFILE%already provides some protection).
447-473: Redundant delete and missing success verification for macOS Keychain.Two issues flagged in previous reviews remain:
The
delete-generic-passwordcall (lines 448-457) is redundant sinceadd-generic-passwordwith-Uflag already updates existing entries.The
add-generic-passwordcall doesn't verifyreturncode, so failures (keychain locked, permission denied) are silently ignored while returningTrue.🔎 Suggested fix
- # Delete old entry - subprocess.run( - [ - "/usr/bin/security", - "delete-generic-password", - "-s", - "Claude Code-credentials", - ], - capture_output=True, - timeout=5, - ) - # Add new entry new_json = json.dumps(existing) - subprocess.run( + result = subprocess.run( [ "/usr/bin/security", "add-generic-password", "-s", "Claude Code-credentials", "-w", new_json, "-U", ], capture_output=True, timeout=5, ) + + if result.returncode != 0: + logger.warning(f"Failed to save to Keychain: {result.stderr}") + return False return True
538-543: Consider returning the original token as a fallback when refresh fails.When the token appears expired but refresh fails (no refresh token, network error), the function returns
Noneeven though the original token might still work briefly due to clock skew or network latency. This causes immediate failure instead of allowing the API call to be attempted.🔎 Suggested improvement for graceful degradation
else: if verbose: print("⚠ Failed to refresh token") - logger.warning("Token expired and refresh failed or no refresh token available") - return None + # Refresh failed - return original token anyway as fallback + # It might still work briefly (clock skew, network latency) + # If it fails with 401, session.py error handler will provide guidance + logger.warning("Token appears expired, refresh failed - trying original token") + return creds["accessToken"] return creds["accessToken"]apps/backend/agents/session.py (1)
550-558: 401 detection via string matching is fragile.The current detection checks for
"401"or"authentication_error"in the lowercased exception string. This could miss variations like"unauthorized","invalid_token", or"token expired", and could false-positive on unrelated messages containing "401".🔎 Suggested improvement
error_str = str(e).lower() # Check for OAuth token expiration (401 authentication error) - if "401" in error_str or "authentication_error" in error_str: + auth_indicators = ( + "401", + "authentication_error", + "unauthorized", + "invalid_token", + "token expired", + "oauth", + ) + if any(indicator in error_str for indicator in auth_indicators):
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
apps/backend/agents/session.pyapps/backend/core/auth.pyapps/backend/core/client.py
🧰 Additional context used
📓 Path-based instructions (4)
apps/backend/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
apps/backend/**/*.py: Always use the Claude Agent SDK (claude-agent-sdkpackage) for all AI interactions, never use the Anthropic API directly
Use thecreate_client()function fromapps/backend/core/client.pyto instantiate Claude SDK clients, not directClaudeSDKClientinitialization
Files:
apps/backend/core/client.pyapps/backend/agents/session.pyapps/backend/core/auth.py
⚙️ CodeRabbit configuration file
apps/backend/**/*.py: Focus on Python best practices, type hints, and async patterns.
Check for proper error handling and security considerations.
Verify compatibility with Python 3.12+.
Files:
apps/backend/core/client.pyapps/backend/agents/session.pyapps/backend/core/auth.py
apps/backend/core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Implement dynamic command allowlisting based on detected project stack using
core/security.pyfor bash command validation
Files:
apps/backend/core/client.pyapps/backend/core/auth.py
apps/backend/core/client.py
📄 CodeRabbit inference engine (CLAUDE.md)
Implement agent-specific tool permissions in the Claude SDK client based on agent role (planner, coder, qa_reviewer, qa_fixer)
Files:
apps/backend/core/client.py
apps/backend/agents/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use Graphiti memory system (
integrations/graphiti/) for cross-session context and knowledge graph management in agents
Files:
apps/backend/agents/session.py
🧠 Learnings (3)
📚 Learning: 2025-12-30T16:38:36.314Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T16:38:36.314Z
Learning: Applies to apps/backend/**/*.py : Use the `create_client()` function from `apps/backend/core/client.py` to instantiate Claude SDK clients, not direct `ClaudeSDKClient` initialization
Applied to files:
apps/backend/core/client.py
📚 Learning: 2025-12-30T16:38:36.314Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T16:38:36.314Z
Learning: Applies to apps/backend/core/client.py : Implement agent-specific tool permissions in the Claude SDK client based on agent role (planner, coder, qa_reviewer, qa_fixer)
Applied to files:
apps/backend/core/client.py
📚 Learning: 2025-12-30T16:38:36.314Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T16:38:36.314Z
Learning: Applies to apps/backend/**/*.py : Always use the Claude Agent SDK (`claude-agent-sdk` package) for all AI interactions, never use the Anthropic API directly
Applied to files:
apps/backend/core/client.py
🧬 Code graph analysis (2)
apps/backend/core/client.py (1)
apps/backend/core/auth.py (1)
require_auth_token(568-609)
apps/backend/agents/session.py (1)
apps/backend/core/auth.py (3)
get_full_credentials(160-173)refresh_oauth_token(297-335)save_credentials(343-360)
🔇 Additional comments (4)
apps/backend/core/client.py (1)
484-486: LGTM! Centralized token refresh logic.The change to
require_auth_token(verbose=True)correctly delegates all token refresh logic to the centralized implementation incore/auth.py, addressing the previous review feedback about duplicate refresh code. Theverbose=Trueparameter enables user-facing feedback during proactive token refresh.apps/backend/core/auth.py (1)
22-35: LGTM! Well-structured OAuth configuration.The environment-driven configuration with sensible defaults allows flexibility for different environments while maintaining secure defaults. The 5-minute buffer for proactive refresh is reasonable.
apps/backend/agents/session.py (2)
46-51: LGTM! Imports moved to file top per PEP 8.The auth imports are now at module level with a clarifying comment, addressing the previous review feedback about local imports.
562-599: LGTM! Proper return statements after 401 handling.Each logical path in the token refresh block now correctly returns, preventing fall-through to the general exception handler. This addresses the critical issue from the previous review.
- Line 573: Returns after successful refresh + save
- Line 578: Returns after refresh success but save failure
- Line 585: Returns after refresh failure
- Line 595: Returns when no credentials/refresh token
- Line 599: Returns on refresh exception
- session.py: More robust 401 detection - check for "unauthorized", "token expired", "invalid token" in addition to "401" and "authentication_error" - auth.py: Add clarifying comment for ms/seconds timestamp heuristic - auth.py: Check returncode of macOS Keychain add-generic-password - auth.py: Remove redundant delete before add (-U flag handles update) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> Signed-off-by: wzm <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
apps/backend/core/auth.py (4)
188-270: Code duplication between Linux and Windows credential functions.
_get_full_credentials_linux()and_get_full_credentials_windows()share identical logic except for the credential paths. Consider extracting a common helper to reduce duplication.🔎 Proposed refactor
def _get_full_credentials_from_paths(cred_paths: list[str]) -> dict | None: """Get full credentials from a list of file paths.""" for cred_path in cred_paths: if os.path.exists(cred_path): try: with open(cred_path, encoding="utf-8") as f: data = json.load(f) oauth = data.get("claudeAiOauth", {}) access_token = oauth.get("accessToken") if access_token and access_token.startswith("sk-ant-oat01-"): return { "accessToken": access_token, "refreshToken": oauth.get("refreshToken"), "expiresAt": oauth.get("expiresAt"), } except (json.JSONDecodeError, KeyError): continue return NoneThen call it from both platform-specific functions with their respective paths.
324-331: Missing validation foraccess_tokenin response.If the OAuth endpoint returns a 200 response but with a missing or null
access_token, this function returns a dict withaccessToken: None, which could cause downstream issues.🔎 Suggested fix
if response.status_code == 200: data = response.json() + access_token = data.get("access_token") + if not access_token: + logger.warning("Token refresh returned empty access_token") + return None expires_in = data.get("expires_in", 28800) # Default 8 hours return { - "accessToken": data.get("access_token"), + "accessToken": access_token, "refreshToken": data.get("refresh_token"), "expiresAt": int((time.time() + expires_in) * 1000), }
365-418: Code duplication between Linux and Windows save functions.
_save_credentials_linux()and_save_credentials_windows()share nearly identical logic. The only differences are the credential path and theos.chmodcall in Linux.🔎 Proposed refactor
def _save_credentials_to_file(credentials: dict, cred_path: str, set_permissions: bool = False) -> bool: """Save credentials to a JSON file.""" try: existing = {} if os.path.exists(cred_path): with open(cred_path, encoding="utf-8") as f: existing = json.load(f) existing["claudeAiOauth"] = { "accessToken": credentials["accessToken"], "refreshToken": credentials["refreshToken"], "expiresAt": credentials["expiresAt"], } os.makedirs(os.path.dirname(cred_path), exist_ok=True) with open(cred_path, "w", encoding="utf-8") as f: json.dump(existing, f, indent=2) if set_permissions: os.chmod(cred_path, 0o600) return True except Exception as e: logger.warning(f"Failed to save credentials: {e}") return False
512-537: Consider returning original token as fallback when refresh fails.If the token appears expired but refresh fails (no refresh token, network error), the function returns
Noneeven though the original token might still work briefly (clock skew, network latency). Consider returning the original token as a fallback attempt, letting the API call fail naturally if it's truly expired.🔎 Alternative approach
logger.warning("Token expired and refresh failed or no refresh token available") - return None + # Refresh failed - return original token anyway + # It might still work briefly (clock skew, network latency) + logger.info("Returning original token as fallback") + return creds["accessToken"]This allows graceful degradation—the API call may still succeed, and if it fails with 401, the session.py error handler will attempt refresh and provide guidance.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
apps/backend/agents/session.pyapps/backend/core/auth.py
🧰 Additional context used
📓 Path-based instructions (3)
apps/backend/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
apps/backend/**/*.py: Always use the Claude Agent SDK (claude-agent-sdkpackage) for all AI interactions, never use the Anthropic API directly
Use thecreate_client()function fromapps/backend/core/client.pyto instantiate Claude SDK clients, not directClaudeSDKClientinitialization
Files:
apps/backend/agents/session.pyapps/backend/core/auth.py
⚙️ CodeRabbit configuration file
apps/backend/**/*.py: Focus on Python best practices, type hints, and async patterns.
Check for proper error handling and security considerations.
Verify compatibility with Python 3.12+.
Files:
apps/backend/agents/session.pyapps/backend/core/auth.py
apps/backend/agents/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use Graphiti memory system (
integrations/graphiti/) for cross-session context and knowledge graph management in agents
Files:
apps/backend/agents/session.py
apps/backend/core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Implement dynamic command allowlisting based on detected project stack using
core/security.pyfor bash command validation
Files:
apps/backend/core/auth.py
🧬 Code graph analysis (1)
apps/backend/agents/session.py (1)
apps/backend/core/auth.py (3)
get_full_credentials(160-173)refresh_oauth_token(299-337)save_credentials(345-362)
🔇 Additional comments (6)
apps/backend/core/auth.py (3)
278-297: LGTM!The token expiration check now properly handles
expires_at == 0and includes a clear comment explaining the milliseconds vs seconds heuristic.
451-467: LGTM!The macOS Keychain save now properly checks
returncodeand logs failures, addressing the previous concern about silently ignoring errors.
562-603: LGTM!The
require_auth_tokenfunction now accepts averboseparameter and provides platform-specific guidance, which aligns well with the token refresh flow.apps/backend/agents/session.py (3)
46-52: LGTM!Auth imports are now properly at module level with a clear comment explaining their purpose, addressing the previous concern about local imports.
549-604: Well-implemented 401 error handling with token refresh.The authentication error handling is now robust:
- Multiple error indicators cover various SDK/API error formats
- Each code path has proper
returnstatements, preventing fall-through to the general exception handler (addressing the previous critical issue)- Clear user messaging with actionable guidance
606-616: LGTM!The general exception handler is properly separated from the auth error handling and will only execute for non-authentication errors.
|
/gemini review |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable feature: automatic OAuth token refresh. This will significantly improve the user experience for long-running agent sessions by preventing interruptions due to expired tokens. The implementation is robust, covering both proactive checks before token expiry and reactive handling of 401 errors. The addition of platform-specific credential handling and a comprehensive suite of unit tests is commendable.
I've identified one critical issue with the credential saving logic on Windows that could lead to a refresh loop, and have also suggested some refactoring opportunities to improve code clarity and reduce duplication in both core/auth.py and agents/session.py. Once these points are addressed, this will be an excellent addition to the codebase.
Critical fix: - Windows _save_credentials_windows() now uses .credentials.json (with leading dot) to match Claude Code's primary read path in _get_full_credentials_windows(). This prevents a refresh loop bug where tokens would be read from .credentials.json but saved to credentials.json, causing perpetual refreshes on every run. Defensive improvement: - TOKEN_REFRESH_BUFFER_SECONDS env var parsing now gracefully handles invalid values (e.g., "abc") by falling back to 300 seconds default instead of raising ValueError at module import time. Addresses: - Gemini critical: Windows credential path inconsistency - CodeRabbit nitpick: Graceful handling for invalid env var 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> Signed-off-by: wzm <[email protected]>
|
/gemini review |
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
Merge Verdict: 🟠 NEEDS REVISION
5 issue(s) must be addressed (0 required, 5 recommended), 5 suggestions
Risk Assessment
| Factor | Level | Notes |
|---|---|---|
| Complexity | High | Based on lines changed |
| Security Impact | Low | Based on security findings |
| Scope Coherence | Good | Based on structural review |
Findings Summary
- Medium: 5 issue(s)
- Low: 5 issue(s)
Generated by Auto Claude PR Review
Findings (10 selected of 10 total)
🟡 [MEDIUM] Missing validation for access_token in OAuth refresh response
📁 apps/backend/core/auth.py:327
When the OAuth endpoint returns HTTP 200 but the response JSON is missing 'access_token' (e.g., {'error': 'something'}), the function returns a dict with accessToken=None. While the caller at line 520 checks for this, it's a fragile pattern. A 200 response with no access_token should be treated as a failure, not a success.
Suggested fix:
Add explicit validation after line 325: `if not data.get('access_token'): logger.warning('OAuth response missing access_token'); return None`
🟡 [MEDIUM] 401 handler returns without retrying the failed operation
📁 apps/backend/agents/session.py:578
When a 401 authentication error is detected and the token is successfully refreshed, the function returns ('error', 'Token refreshed. Please retry your command.') instead of automatically retrying the failed operation. This requires manual user intervention when the refresh could be seamless.
Suggested fix:
Either (1) return a distinct status like 'retry' that the caller can handle to retry automatically, or (2) if retry is complex, improve the messaging to explain why manual retry is needed.
🟡 [MEDIUM] Test mock_open with json.load does not work correctly
📁 tests/test_auth_token_refresh.py:256
The test uses mock_open().return_value.read.return_value = json.dumps(cred_data), but json.load() uses file iteration not .read() directly. Additionally, the test uses 'linux-access-token' which would fail the 'sk-ant-oat01-' prefix validation in the actual function. This test may pass but isn't testing the intended behavior.
Suggested fix:
Use mock_open(read_data=json.dumps(cred_data)) and update the token format to 'sk-ant-oat01-test-linux-token' to match the function's validation requirements.
🟡 [MEDIUM] Module reload in tests can cause side effects
📁 tests/test_auth_token_refresh.py:467
The test uses importlib.reload(core.auth) to test environment variable changes. If the test fails before the reset at lines 480-481, module state remains corrupted for subsequent tests. This can cause flaky test failures that are hard to debug.
Suggested fix:
Use a pytest fixture with try/finally cleanup that stores and restores the original module-level constants (OAUTH_TOKEN_URL, OAUTH_CLIENT_ID, TOKEN_REFRESH_BUFFER_SECONDS) regardless of test outcome.
🟡 [MEDIUM] Non-integer CLAUDE_TOKEN_REFRESH_BUFFER_SECONDS causes crash
📁 apps/backend/core/auth.py:33
TOKEN_REFRESH_BUFFER_SECONDS = int(os.environ.get(..., '300')) will raise ValueError if the environment variable contains a non-integer string (e.g., 'abc'). This would crash the application on startup due to misconfiguration.
Suggested fix:
Add try/except with fallback: `try: TOKEN_REFRESH_BUFFER_SECONDS = int(os.environ.get('CLAUDE_TOKEN_REFRESH_BUFFER_SECONDS', '300')) except ValueError: TOKEN_REFRESH_BUFFER_SECONDS = 300; logger.warning('Invalid CLAUDE_TOKEN_REFRESH_BUFFER_SECONDS, using default 300')`
🔵 [LOW] Overly broad 401 detection may trigger on non-auth errors
📁 apps/backend/agents/session.py:554
The 401 detection checks for substrings '401', 'authentication_error', 'unauthorized', 'token expired', 'invalid token' in the lowercased error string. This could false-positive on errors like 'processed 401 records' or 'error in file line 401'. The string '401' alone is particularly prone to false positives.
Suggested fix:
Use more specific patterns: check for 'status 401', 'http 401', '401 unauthorized', or add word boundary checks. Alternatively, check the exception type directly if available.
🔵 [LOW] Significant code duplication between credential getter functions
📁 apps/backend/core/auth.py:188
_get_full_credentials_linux and _get_full_credentials_windows contain nearly identical logic: iterate paths, check existence, read JSON, validate token format, extract credentials. Only the path lists differ. This duplication means bug fixes must be applied to both functions, increasing maintenance burden.
Suggested fix:
Extract shared logic to a helper function _get_full_credentials_from_file_paths(cred_paths: list[str]) that handles iteration, file reading, and credential extraction. Platform functions just call this with their path lists.
🔵 [LOW] Significant code duplication between credential saver functions
📁 apps/backend/core/auth.py:365
_save_credentials_linux and _save_credentials_windows share almost identical logic: read existing file, update OAuth section, create directory, write JSON. Key difference is Linux sets chmod 0600 while Windows doesn't. Same maintenance burden as the getter functions.
Suggested fix:
Extract shared logic to _save_credentials_to_file(cred_path, credentials, chmod_mode=None). Platform functions call this with their path and optional permissions.
🔵 [LOW] Race condition in Linux credential file creation with permissions
📁 apps/backend/core/auth.py:384
In _save_credentials_linux(), the file is created via open() at line 385, then permissions set via os.chmod() at line 387. Between these operations, there's a window where the file exists with default permissions (typically 0o644), potentially exposing credentials briefly. This is a TOCTOU vulnerability.
Suggested fix:
Set umask before creating the file for atomic creation with correct permissions: `old_umask = os.umask(0o077); try: <write file>; finally: os.umask(old_umask)`. Or use os.open() with mode flags.
🔵 [LOW] Windows credential file saved without restrictive permissions
📁 apps/backend/core/auth.py:395
The _save_credentials_windows() function writes OAuth credentials to disk without setting restrictive file ACLs. Unlike Linux which sets 0o600 permissions, Windows implementation has no equivalent protection. Credentials could be readable by other users on shared Windows systems.
Suggested fix:
Use Windows ACLs to restrict file access to current user only, or use icacls command via subprocess to set restrictive permissions after file creation.
This review was generated by Auto Claude.
There was a problem hiding this comment.
Code Review
This pull request introduces a robust mechanism for automatic OAuth token refresh, addressing a critical issue with long-running sessions. The implementation is comprehensive, covering proactive checks, reactive 401 handling, and platform-specific credential storage for macOS, Windows, and Linux. The addition of a full suite of unit and integration tests is excellent and ensures the reliability of this new feature. My review focuses on a few opportunities to improve maintainability by reducing code duplication and refining exception handling. Overall, this is a high-quality contribution.
Auto Claude PR Review fixes: - MEDIUM: Add explicit validation for access_token in OAuth response (returns None if 200 response lacks access_token) - MEDIUM: Improve 401 detection with specific patterns to avoid false positives (use "status 401", "http 401" not bare "401") - MEDIUM: Return "retry" status on successful refresh with clearer messaging about why manual retry is needed - MEDIUM: Fix test mock_open to use read_data param (json.load compat) - MEDIUM: Fix test tokens to use valid sk-ant-oat01- prefix - MEDIUM: Add try/finally cleanup for module reload tests - LOW: Fix Linux TOCTOU race condition with umask(0o077) for atomic file creation with restrictive permissions Gemini Code Assist fixes: - MEDIUM: Log response body on token refresh failure for debugging - MEDIUM: Use requests.exceptions.RequestException instead of bare Exception for more specific error handling - MEDIUM: Refactor nested error handling in session.py using guard clauses for clearer flow - MEDIUM: Extract _get_full_credentials_from_file() and _get_full_credentials_from_paths() helpers to reduce code duplication between Linux and Windows credential getters - MEDIUM: Extract _save_credentials_to_file() helper to reduce code duplication between Linux and Windows credential savers New test: - test_refresh_missing_access_token_in_response for edge case 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> Signed-off-by: wzm <[email protected]>
|
/gemini review |
|
@AndyMik90 Could you please trigger another Auto Claude PR review? All 10 issues from the previous review have been addressed in commit 45a84a3. |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/backend/agents/session.py (1)
339-343: Undocumented return status "retry" lacks explicit caller handling.The function docstring documents return statuses as
"continue","complete", or"error", but line 612 returns"retry". Neithercoder.pynorplanner.pyexplicitly handle this status. Incoder.py, the "retry" value falls through all three status checks without acknowledgment. Inplanner.py, it's implicitly treated as success since only"error"is checked. Update the docstring to document"retry"as a valid return status, and consider adding explicit handling in callers to properly acknowledge token refresh recovery.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
apps/backend/agents/session.pyapps/backend/core/auth.pytests/test_auth_token_refresh.py
🧰 Additional context used
📓 Path-based instructions (4)
apps/backend/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
apps/backend/**/*.py: Always use the Claude Agent SDK (claude-agent-sdkpackage) for all AI interactions, never use the Anthropic API directly
Use thecreate_client()function fromapps/backend/core/client.pyto instantiate Claude SDK clients, not directClaudeSDKClientinitialization
Files:
apps/backend/agents/session.pyapps/backend/core/auth.py
⚙️ CodeRabbit configuration file
apps/backend/**/*.py: Focus on Python best practices, type hints, and async patterns.
Check for proper error handling and security considerations.
Verify compatibility with Python 3.12+.
Files:
apps/backend/agents/session.pyapps/backend/core/auth.py
apps/backend/agents/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use Graphiti memory system (
integrations/graphiti/) for cross-session context and knowledge graph management in agents
Files:
apps/backend/agents/session.py
tests/**
⚙️ CodeRabbit configuration file
tests/**: Ensure tests are comprehensive and follow pytest conventions.
Check for proper mocking and test isolation.
Files:
tests/test_auth_token_refresh.py
apps/backend/core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Implement dynamic command allowlisting based on detected project stack using
core/security.pyfor bash command validation
Files:
apps/backend/core/auth.py
🧬 Code graph analysis (2)
apps/backend/agents/session.py (1)
apps/backend/core/auth.py (3)
get_full_credentials(168-181)refresh_oauth_token(319-361)save_credentials(369-386)
tests/test_auth_token_refresh.py (1)
apps/backend/core/auth.py (6)
is_token_expired(298-316)refresh_oauth_token(319-361)get_full_credentials(168-181)save_credentials(369-386)_get_full_credentials_linux(242-247)_save_credentials_linux(432-442)
🔇 Additional comments (11)
apps/backend/agents/session.py (2)
46-51: LGTM! Auth imports are properly placed at module level.The imports for
get_full_credentials,refresh_oauth_token, andsave_credentialsare correctly placed at the top of the file, addressing the previous review concern about local imports.
549-566: LGTM! Pattern-based auth error detection is robust.The specific patterns like
"status 401","http 401","401 unauthorized"effectively avoid false positives (e.g., "processed 401 records") while catching common authentication error formats. This addresses the previous review feedback.apps/backend/core/auth.py (6)
35-43: LGTM! Graceful handling for invalid environment variable.The
_parse_buffer_seconds()function properly handles invalidCLAUDE_TOKEN_REFRESH_BUFFER_SECONDSvalues by catchingValueErrorand falling back to the default 300 seconds. This addresses the previous review feedback.
196-239: LGTM! Good refactoring to reduce code duplication.The helper functions
_get_full_credentials_from_file()and_get_full_credentials_from_paths()eliminate the duplication between Linux and Windows credential reading, addressing the previous review feedback about code reuse.
308-316: LGTM! Token expiration logic is correct and well-documented.The handling of
Noneand0expiry values, along with the heuristic comment explaining milliseconds vs seconds detection, addresses previous review feedback.
344-361: LGTM! Token refresh implementation is robust.The function properly validates
access_tokenpresence in the response (addressing previous review), uses specificRequestExceptionhandling, and logs helpful debugging information including status code and response text on failure.
480-503: LGTM! macOS Keychain save properly verifies success.The implementation now checks the
returncodeafteradd-generic-passwordand logs the error viaresult.stderr, addressing the previous review feedback about silent failures.
432-442: LGTM! Secure file creation with umask.Using
umask(0o077)before file creation prevents the TOCTOU race condition where the file could be briefly world-readable. Thefinallyblock ensures the original umask is always restored.tests/test_auth_token_refresh.py (3)
268-297: LGTM! Linux credential read test is properly structured.The test correctly uses
mock_open(read_data=json.dumps(cred_data))which works withjson.load(), and uses valid token format (sk-ant-oat01-...) required by the token validation. This addresses the previous review concern about mock setup.
482-507: LGTM! Module reload test has proper cleanup.The
try/finallyblock ensures the environment variable is removed and the module is reloaded even if the test fails. The additional fallback restoration (lines 504-507) provides extra safety.
376-377: Thetemp_dirfixture is already defined intests/conftest.py(line 164) as a proper pytest fixture that creates and cleans up a temporary directory. The test file correctly uses it as a parameter, which pytest automatically injects. No action is required.
There was a problem hiding this comment.
Code Review
This pull request introduces a crucial feature for long-running sessions: automatic OAuth token refresh. The implementation is comprehensive, covering proactive refresh before API calls and reactive refresh upon 401 errors. It includes platform-specific credential handling for macOS, Windows, and Linux, and adds extensive unit tests.
Overall, the changes are well-structured and robust. I've identified a few areas for improvement:
- A high-severity logic issue where the refresh mechanism can be defeated if an expired OAuth token is set as an environment variable. I've provided suggestions to refactor the credential retrieval logic to fix this.
- Some minor maintainability improvements, such as adding logging to silent exception handlers and reducing code duplication in error messages.
Once the high-severity issue with environment variable handling is addressed, this will be a solid addition to improve the agent's reliability.
Gemini HIGH priority fixes: - Fix get_full_credentials() priority logic: ANTHROPIC_AUTH_TOKEN (enterprise) takes top priority with no refresh; for OAuth, prefer store to get refresh token, CLAUDE_CODE_OAUTH_TOKEN can override access token while preserving store's refresh capability - Remove redundant env var loop in get_auth_token() - now handled by get_full_credentials() with proper priority CodeRabbit fixes: - Use print_status() for consistency with rest of session.py - Fix integration test token formats to use sk-ant-oat01- prefix - Add graceful degradation: return original token as fallback when refresh fails (clock skew, network latency may still allow usage) - Add TestGetAuthTokenWithRefresh test class with 5 tests: - test_token_not_expired_returns_as_is - test_expired_token_refresh_success - test_expired_token_refresh_fails_returns_original - test_expired_token_no_refresh_token_returns_original - test_no_credentials_returns_none Test updates: - Split test_env_var_oauth_token into two cases for clarity - Add assertion for enterprise token having no refreshToken - Total: 37 tests now covering all token refresh scenarios 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> Signed-off-by: wzm <[email protected]>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a robust mechanism for automatic OAuth token refreshing, which is a critical fix for long-running agent sessions. The implementation is well-structured, with clear separation of concerns for credential retrieval, validation, refreshing, and saving across different platforms. The addition of comprehensive unit tests is excellent and greatly increases confidence in the new functionality. I've identified a few areas for improvement, mainly around making exception handling more specific and resolving an inconsistency in the credential source reporting logic. Overall, this is a high-quality contribution that significantly improves the application's resilience.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
apps/backend/agents/session.pyapps/backend/core/auth.pytests/test_auth_token_refresh.py
🧰 Additional context used
📓 Path-based instructions (4)
tests/**
⚙️ CodeRabbit configuration file
tests/**: Ensure tests are comprehensive and follow pytest conventions.
Check for proper mocking and test isolation.
Files:
tests/test_auth_token_refresh.py
apps/backend/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
apps/backend/**/*.py: Always use the Claude Agent SDK (claude-agent-sdkpackage) for all AI interactions, never use the Anthropic API directly
Use thecreate_client()function fromapps/backend/core/client.pyto instantiate Claude SDK clients, not directClaudeSDKClientinitialization
Files:
apps/backend/agents/session.pyapps/backend/core/auth.py
⚙️ CodeRabbit configuration file
apps/backend/**/*.py: Focus on Python best practices, type hints, and async patterns.
Check for proper error handling and security considerations.
Verify compatibility with Python 3.12+.
Files:
apps/backend/agents/session.pyapps/backend/core/auth.py
apps/backend/agents/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use Graphiti memory system (
integrations/graphiti/) for cross-session context and knowledge graph management in agents
Files:
apps/backend/agents/session.py
apps/backend/core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Implement dynamic command allowlisting based on detected project stack using
core/security.pyfor bash command validation
Files:
apps/backend/core/auth.py
🧬 Code graph analysis (2)
tests/test_auth_token_refresh.py (1)
apps/backend/core/auth.py (7)
is_token_expired(315-333)refresh_oauth_token(336-378)get_full_credentials(168-198)save_credentials(386-403)get_auth_token(528-584)_get_full_credentials_linux(259-264)_save_credentials_linux(449-459)
apps/backend/agents/session.py (1)
apps/backend/core/auth.py (3)
get_full_credentials(168-198)refresh_oauth_token(336-378)save_credentials(386-403)
🔇 Additional comments (10)
apps/backend/agents/session.py (1)
46-51: LGTM: Auth imports are properly placed at module level.The imports for OAuth refresh functionality are correctly positioned at the top of the file with a clear comment explaining their purpose.
tests/test_auth_token_refresh.py (1)
1-637: Excellent test coverage for OAuth refresh functionality.This test suite is comprehensive and well-structured:
- Covers all major functions:
is_token_expired,refresh_oauth_token,get_full_credentials,get_auth_token, and credential storage operations- Tests edge cases: expired tokens, missing fields, network errors, invalid values
- Includes integration tests for Linux credential file operations with proper permission validation
- Uses proper mocking and isolation techniques
- Handles cleanup correctly (e.g., try/finally for module reload)
- All previously flagged issues have been addressed
apps/backend/core/auth.py (8)
22-43: LGTM: OAuth configuration with proper defensive parsing.The configuration handles environment variables correctly:
- Provides sensible defaults for OAuth URL and client ID
- Includes graceful fallback for invalid buffer values via
_parse_buffer_seconds()- Silent fallback to 300 seconds is appropriate for this use case
168-308: Well-structured credential retrieval with proper platform abstraction.The implementation demonstrates good design:
- Clear priority order: enterprise token → credential store → env var override
- Shared helpers (
_get_full_credentials_from_file,_get_full_credentials_from_paths) eliminate code duplication between platforms- Proper error handling with logging (e.g., line 296 for macOS Keychain failures)
- Token format validation ensures only valid OAuth tokens are accepted
The
CLAUDE_CODE_OAUTH_TOKENoverride behavior (lines 188-196) is particularly clever: it allows temporary token override while preserving refresh capability from the store.
315-333: LGTM: Robust token expiry detection with clear heuristic.The implementation correctly handles:
- Missing or zero
expiresAtvalues (graceful fallback)- Both milliseconds and seconds timestamp formats with a well-documented heuristic
- Proactive refresh using the configured buffer period
The comment at lines 329-331 clearly explains the 1e12 threshold, making the code maintainable.
336-378: Robust OAuth token refresh implementation with proper validation.The implementation follows best practices:
- Early validation of refresh token (line 347)
- Proper timeout and headers for the HTTP request
- Validates
access_tokenpresence before returning (lines 364-366)- Comprehensive error logging with response details (line 374)
- Catches specific exception type (
requests.exceptions.RequestException) rather than bareExceptionAll previously identified issues have been addressed.
386-521: Secure credential persistence with proper platform-specific handling.The implementation demonstrates strong security practices:
Linux (lines 449-459):
- Uses
umask(0o077)to create file with restrictive permissions atomically- Prevents TOCTOU race condition where file could be briefly world-readable
Windows (lines 462-466):
- Uses
.credentials.json(with leading dot) consistently with read paths- Addresses the previously flagged path inconsistency issue
macOS (lines 469-520):
- Uses
-Uflag to atomically update existing keychain entries- Validates
returncodeafter keychain operations (line 513-515)- Proper error logging for failures
Shared logic (lines 406-446):
- Preserves existing data when updating OAuth credentials
- Creates parent directories as needed
- Comprehensive error handling with logging
All previously identified issues have been addressed.
528-584: Excellent automatic refresh implementation with graceful degradation.The implementation balances robustness and user experience:
Proactive refresh (lines 555-577):
- Detects expiry before it causes failures
- Attempts refresh when refresh token is available
- Saves new credentials back to store
- Updates
CLAUDE_CODE_OAUTH_TOKENenv var for subsequent calls in the same process (line 572)- Provides optional verbose user feedback
Graceful fallback (lines 578-582):
- Returns original token if refresh fails rather than None
- Allows operations to attempt with potentially-still-valid token (clock skew, network latency)
- If token truly fails, the 401 handler in
session.pywill catch it and provide guidanceThis addresses the previously suggested improvement for fallback behavior.
607-648: LGTM: Verbose parameter enables better user feedback.The addition of the
verboseparameter (line 607) allows the authentication flow to provide user-friendly progress messages during token refresh. The platform-specific error messages (lines 619-647) give clear, actionable guidance for each OS.
1-681: Comprehensive OAuth refresh implementation with excellent security practices.This PR successfully implements automatic OAuth token refresh with:
Security:
- Atomic file creation with restrictive permissions (Linux umask approach)
- Proper credential validation and format checking
- Secure keychain integration for macOS
Reliability:
- Graceful degradation when refresh fails
- Proper priority ordering (enterprise → store → env override)
- Comprehensive error handling and logging
Maintainability:
- Shared helpers eliminate code duplication
- Clear separation of platform-specific logic
- Well-documented heuristics and edge cases
Compatibility:
- Type hints compatible with Python 3.12+
- Platform-specific handling for macOS, Windows, and Linux
- Proper integration with existing credential stores
The implementation addresses all previously identified issues and follows Python best practices.
Address Gemini feedback: log JSONDecodeError/KeyError when parsing credential files to aid debugging malformed credential files. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
apps/backend/agents/session.pytests/test_auth_token_refresh.py
🧰 Additional context used
📓 Path-based instructions (3)
tests/**
⚙️ CodeRabbit configuration file
tests/**: Ensure tests are comprehensive and follow pytest conventions.
Check for proper mocking and test isolation.
Files:
tests/test_auth_token_refresh.py
apps/backend/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
apps/backend/**/*.py: Always use the Claude Agent SDK (claude-agent-sdkpackage) for all AI interactions, never use the Anthropic API directly
Use thecreate_client()function fromapps/backend/core/client.pyto instantiate Claude SDK clients, not directClaudeSDKClientinitialization
Files:
apps/backend/agents/session.py
⚙️ CodeRabbit configuration file
apps/backend/**/*.py: Focus on Python best practices, type hints, and async patterns.
Check for proper error handling and security considerations.
Verify compatibility with Python 3.12+.
Files:
apps/backend/agents/session.py
apps/backend/agents/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use Graphiti memory system (
integrations/graphiti/) for cross-session context and knowledge graph management in agents
Files:
apps/backend/agents/session.py
🧠 Learnings (2)
📚 Learning: 2025-12-30T16:38:36.314Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T16:38:36.314Z
Learning: Applies to apps/backend/core/client.py : Implement agent-specific tool permissions in the Claude SDK client based on agent role (planner, coder, qa_reviewer, qa_fixer)
Applied to files:
apps/backend/agents/session.py
📚 Learning: 2025-12-30T16:38:36.314Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T16:38:36.314Z
Learning: Applies to apps/backend/**/*.py : Always use the Claude Agent SDK (`claude-agent-sdk` package) for all AI interactions, never use the Anthropic API directly
Applied to files:
apps/backend/agents/session.py
🧬 Code graph analysis (2)
tests/test_auth_token_refresh.py (1)
apps/backend/core/auth.py (7)
is_token_expired(329-347)refresh_oauth_token(350-392)get_full_credentials(168-201)save_credentials(400-417)get_auth_token(561-622)_get_full_credentials_linux(267-272)_save_credentials_linux(476-486)
apps/backend/agents/session.py (1)
apps/backend/core/auth.py (3)
get_full_credentials(168-201)refresh_oauth_token(350-392)save_credentials(400-417)
🔇 Additional comments (11)
tests/test_auth_token_refresh.py (5)
57-111: Well-structured boundary condition tests.The
TestIsTokenExpiredclass provides excellent coverage of edge cases including exact buffer boundaries and handling of missing/None/zero expiry values. The tests are isolated and follow pytest conventions.
171-178: Network error test correctly uses specific exception type.The test now properly uses
requests.exceptions.ConnectionErrorinstead of a genericException, which accurately tests the intended behavior whererefresh_oauth_token()catchesRequestExceptionand its subclasses.
239-303: Credential priority tests are comprehensive.The tests correctly verify the priority chain (ANTHROPIC_AUTH_TOKEN → CLAUDE_CODE_OAUTH_TOKEN → store) and the merging behavior when env vars override store tokens while preserving refresh capabilities.
311-411: Integration tests forget_auth_token()refresh flow are well-designed.The tests cover the critical path that combines expiry detection, refresh, and credential saving. The test cases properly verify:
- Non-expired tokens return without refresh attempt
- Expired tokens trigger refresh and save new credentials
- Graceful degradation when refresh fails or no refresh token exists
- None returned when no credentials available
422-451: Mock setup for Linux credential reading is correct.The test now properly uses
mock_open(read_data=...)with correctly formatted JSON including thesk-ant-oat01-token prefix, ensuringjson.load()reads the mock data correctly and token validation passes.apps/backend/agents/session.py (6)
48-53: Auth imports correctly placed at module top.The imports for
get_full_credentials,refresh_oauth_token, andsave_credentialsfromcore.authare appropriately placed at the top of the file, addressing the previous review concern about local imports.
58-62: DRY helper for authentication failure guidance.The
_print_auth_failure_guidancehelper consolidates the repeated auth failure messaging pattern, improving maintainability and ensuring consistent user guidance across different failure scenarios.
348-352: Docstring accurately documents return statuses.The docstring now correctly clarifies that "error" includes the token refresh case where the user must retry. This addresses the previous concern about undocumented return statuses.
563-575: Robust 401 detection with specific patterns.The auth error detection uses specific patterns like
"status 401","http 401", and"401 unauthorized"instead of a bare"401"string, effectively avoiding false positives from unrelated numeric values (e.g., "processed 401 records").
584-634: Well-structured token refresh flow with guard clauses.The implementation correctly uses guard clauses for clear, flat error handling:
- Guards against missing credentials and refresh tokens with early returns
- Attempts refresh and handles failure gracefully
- Updates the environment variable for same-process callers on success
- Provides distinct warnings when credential save fails
- Uses specific exception types before the catch-all for better diagnostics
The return of
"error"after successful token refresh is the correct approach—the operation did fail mid-request, and the user should retry with the fresh token.
9-14: Imports follow coding guidelines.The file correctly uses
ClaudeSDKClientfromclaude_agent_sdkas required by the coding guidelines ("Always use the Claude Agent SDK for all AI interactions"). The client is passed as a parameter torun_agent_session, which is the appropriate pattern for dependency injection.
1. Replace undefined temp_dir with pytest's built-in tmp_path fixture 2. Config tests now use module reload to get true defaults regardless of environment state at import time 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces automatic OAuth token refresh functionality. Key changes include adding new environment variables for token refresh configuration in .env.example, and a significant refactor of core/auth.py to handle full credential retrieval, token expiry checks, API calls for refreshing tokens, and saving updated credentials to platform-specific stores (macOS Keychain, Windows Credential Manager, Linux files). The get_auth_token function now automatically refreshes tokens when they are expired or nearing expiration. Error handling for authentication failures, including user guidance, is implemented in agents/session.py, and core/client.py is updated to enable verbose output during refresh. A new requests dependency is added to requirements.txt, and a comprehensive test suite (tests/test_auth_token_refresh.py) validates the new logic. A review comment suggests improving error handling in core/auth.py by explicitly catching OSError in macOS credential functions for better encapsulation.
Handle case when /usr/bin/security is not found (OSError) in both _get_full_credentials_macos and _save_credentials_macos functions. This improves encapsulation by making error handling self-contained within auth.py rather than relying on fallback handling in callers. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/backend/core/auth.py (1)
73-161: Consider deprecating legacy token functions.The functions
get_token_from_keychain(),_get_token_from_macos_keychain(), and_get_token_from_windows_credential_files()appear to be superseded by the newget_full_credentials()infrastructure. They use broad exception handlers and return only access tokens without refresh capability.If these are kept only for backward compatibility with external callers, consider adding deprecation warnings or docstring notes to guide users toward
get_full_credentials()orget_auth_token().🔎 Suggested improvement
def get_token_from_keychain() -> str | None: """ Get authentication token from system credential store. + + .. deprecated:: + Use get_auth_token() instead, which provides automatic token refresh + and better error handling. Reads Claude Code credentials from: - macOS: Keychain - Windows: Credential Manager - Linux: Not yet supported (use env var) Returns: Token string if found, None otherwise """
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
apps/backend/core/auth.py
🧰 Additional context used
📓 Path-based instructions (2)
apps/backend/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
apps/backend/**/*.py: Always use the Claude Agent SDK (claude-agent-sdkpackage) for all AI interactions, never use the Anthropic API directly
Use thecreate_client()function fromapps/backend/core/client.pyto instantiate Claude SDK clients, not directClaudeSDKClientinitialization
Files:
apps/backend/core/auth.py
⚙️ CodeRabbit configuration file
apps/backend/**/*.py: Focus on Python best practices, type hints, and async patterns.
Check for proper error handling and security considerations.
Verify compatibility with Python 3.12+.
Files:
apps/backend/core/auth.py
apps/backend/core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Implement dynamic command allowlisting based on detected project stack using
core/security.pyfor bash command validation
Files:
apps/backend/core/auth.py
🔇 Additional comments (5)
apps/backend/core/auth.py (5)
168-322: Excellent credential handling implementation.The full credentials infrastructure is well-designed:
- ✅ Clear priority: enterprise token → store (for refresh) → env override
- ✅ Specific exception handling distinguishes timeouts, subprocess errors, parse errors, and file I/O issues
- ✅ Platform-specific implementations properly encapsulated with DRY helpers
- ✅ Warning-level logging for unexpected failures aids troubleshooting
- ✅ OSError handling covers missing
/usr/bin/securityon macOSThe implementation successfully addresses all prior review feedback.
329-393: Token refresh implementation looks solid.The expiration detection and refresh logic is well-implemented:
- ✅ Handles both milliseconds and seconds timestamps with clear heuristic
- ✅ Treats
expires_at == 0as "no expiry info" (safe default)- ✅ Validates OAuth response contains
access_tokenbefore returning- ✅ Logs response body on failure for troubleshooting
- ✅ Uses specific
RequestExceptioninstead of broadException- ✅ Reasonable 30-second timeout for network call
The implementation addresses all prior review concerns.
395-554: Credential persistence is well-architected.The save implementation demonstrates excellent attention to detail:
- ✅ DRY helper (
_save_credentials_to_file) eliminates code duplication- ✅ Linux:
umask(0o077)prevents TOCTOU where file is briefly world-readable- ✅ Windows: Uses
.credentials.jsonpath matching Claude Code's primary location- ✅ macOS:
-Uflag for atomic update, proper returncode validation- ✅ Design note documents rationale for failing on malformed files (guides users to
claude setup-token)- ✅ Specific exception handling (OSError, IOError, JSONDecodeError, KeyError) with appropriate logging
The security posture is strong, and the cross-platform handling is robust.
561-622: Token retrieval with automatic refresh is well-implemented.The main token retrieval logic successfully integrates the refresh workflow:
- ✅ Uses
get_full_credentials()for consistent priority handling- ✅ Proactively checks expiration before API calls (with 5-minute buffer)
- ✅ Refreshes token, saves to store, and updates env var in-process
- ✅ Graceful degradation: returns original token if refresh fails (handles clock skew)
- ✅ Verbose mode provides user-friendly feedback
- ✅ Side effects properly documented in docstring
Note on thread safety: Line 610 updates
os.environ["CLAUDE_CODE_OAUTH_TOKEN"]which is not thread-safe. For the current single-threaded CLI use case this is fine. If this code is ever used in a multi-threaded context (e.g., web server), consider using a lock around the refresh flow.
625-697: Token source tracking and error guidance are excellent.The helper functions properly support the new workflow:
- ✅
get_auth_token_source()priority matchesget_full_credentials()implementation- ✅ Source detection correctly identifies enterprise token vs store vs env override
- ✅
require_auth_token()threads verbose parameter through the call chain- ✅ Error messages provide clear, platform-specific setup instructions
- ✅ Guidance directs users to
claude setup-tokenas the canonical setup pathThe integration is cohesive and the error messages will help users quickly resolve authentication issues.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
apps/backend/core/auth.py
🧰 Additional context used
📓 Path-based instructions (2)
apps/backend/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
apps/backend/**/*.py: Always use the Claude Agent SDK (claude-agent-sdkpackage) for all AI interactions, never use the Anthropic API directly
Use thecreate_client()function fromapps/backend/core/client.pyto instantiate Claude SDK clients, not directClaudeSDKClientinitialization
Files:
apps/backend/core/auth.py
⚙️ CodeRabbit configuration file
apps/backend/**/*.py: Focus on Python best practices, type hints, and async patterns.
Check for proper error handling and security considerations.
Verify compatibility with Python 3.12+.
Files:
apps/backend/core/auth.py
apps/backend/core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Implement dynamic command allowlisting based on detected project stack using
core/security.pyfor bash command validation
Files:
apps/backend/core/auth.py
… fails Add logger.warning that includes the actual invalid value when CLAUDE_TOKEN_REFRESH_BUFFER_SECONDS parsing fails, helping users diagnose configuration issues. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
WaaromZoMoeilijk
force-pushed
the
fix/oauth-token-refresh
branch
from
21dcf0d to
4141d48
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces automatic OAuth token refresh functionality. Key changes include adding new environment variables for OAuth configuration in .env.example, modifying session.py to detect 401 authentication errors and initiate token refresh, and extensively refactoring core/auth.py to manage full credential retrieval, token expiry checks, refresh requests using the newly added requests library, and persistence of updated tokens to platform-specific credential stores. A new, comprehensive test file (test_auth_token_refresh.py) was added to validate the new token refresh logic. The reviewer praised the thoroughness of these tests but recommended adding further tests for get_auth_token_source() to verify its priority logic and prevent regressions, suggesting this would have helped identify existing priority inconsistencies.
Add TestGetAuthTokenSource class with 6 tests covering: - ANTHROPIC_AUTH_TOKEN has highest priority - CLAUDE_CODE_OAUTH_TOKEN priority over store - Linux/macOS/Windows credential store sources - Returns None when no sources available 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/backend/core/auth.py (1)
629-657: Test failure: Mismatched return values forget_auth_token_source().The return values don't match the test expectations in
tests/test_auth_token_refresh.py, which will cause CI failures:
- Line 651: Returns
"System Credential Store"but test (line 702) expects"Linux credential file"- Line 649: Returns
"Windows Credential Files"(plural) but test (line 724) expects"Windows credential file"(singular)🔎 Fix return values to match test expectations
creds = _get_full_credentials_from_store() if creds and creds.get("accessToken"): system = platform.system() if system == "Darwin": return "macOS Keychain" elif system == "Windows": - return "Windows Credential Files" + return "Windows credential file" else: - return "System Credential Store" + return "Linux credential file"Alternatively, update the test expectations to match the current return values if these strings are the intended user-facing messages.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
apps/backend/core/auth.pytests/test_auth_token_refresh.py
🧰 Additional context used
📓 Path-based instructions (3)
tests/**
⚙️ CodeRabbit configuration file
tests/**: Ensure tests are comprehensive and follow pytest conventions.
Check for proper mocking and test isolation.
Files:
tests/test_auth_token_refresh.py
apps/backend/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
apps/backend/**/*.py: Always use the Claude Agent SDK (claude-agent-sdkpackage) for all AI interactions, never use the Anthropic API directly
Use thecreate_client()function fromapps/backend/core/client.pyto instantiate Claude SDK clients, not directClaudeSDKClientinitialization
Files:
apps/backend/core/auth.py
⚙️ CodeRabbit configuration file
apps/backend/**/*.py: Focus on Python best practices, type hints, and async patterns.
Check for proper error handling and security considerations.
Verify compatibility with Python 3.12+.
Files:
apps/backend/core/auth.py
apps/backend/core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Implement dynamic command allowlisting based on detected project stack using
core/security.pyfor bash command validation
Files:
apps/backend/core/auth.py
🧬 Code graph analysis (1)
tests/test_auth_token_refresh.py (1)
apps/backend/core/auth.py (8)
is_token_expired(333-351)refresh_oauth_token(354-396)get_full_credentials(172-205)get_auth_token_source(629-657)save_credentials(404-421)get_auth_token(565-626)_get_full_credentials_linux(271-276)_save_credentials_linux(480-490)
🪛 Gitleaks (8.30.0)
tests/test_auth_token_refresh.py
[high] 644-644: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (2)
tests/test_auth_token_refresh.py (2)
644-644: Static analysis false positive: OAuth client ID is not a secret.The Gitleaks tool flagged this line as a "Generic API Key", but this is a false positive. The value
9d1c250a-e61b-44d9-88ed-5944d1962f5eis the public OAuth client ID for the Claude Code CLI application (as documented in the code comments and referenced in the PR description).OAuth client IDs are designed to be public identifiers—they're included in authorization URLs and redirect flows. Only the client secret (which is not present in this codebase) would be sensitive.
Note: The string mismatch issues in
test_linux_store_when_no_env_vars(line 702) andtest_windows_store_when_no_env_vars(line 724) are covered in the review comment forapps/backend/core/auth.pylines 629-657.
1-733: Excellent test coverage with proper pytest patterns.This test file demonstrates strong testing practices:
- ✅ Comprehensive coverage of token expiry scenarios, refresh flows, and credential store operations
- ✅ Proper use of pytest fixtures, mocks, and
tmp_pathfor isolation- ✅ Platform-specific tests for Linux, macOS, and Windows credential stores
- ✅ Integration tests validating end-to-end flows with file permissions
- ✅ Module reload tests properly wrapped in try/finally for cleanup
- ✅ Clear test organization with descriptive class and method names
The test suite thoroughly validates the OAuth token refresh implementation and should catch regressions effectively.
Local Test ResultsRan the OAuth token refresh test suite locally on Linux. Initial Run: 40/44 passedFound 4 test failures - all were test bugs, not implementation issues. Fixes Applied (commit cf7b020)
After Fixes: 44/44 passed ✅Live Testing ✅Verified auth module works on Linux host: >>> from core.auth import get_auth_token_source, get_full_credentials, require_auth_token
>>> get_auth_token_source()
'Linux credential file'
>>> creds = get_full_credentials()
>>> creds.get('accessToken')[:20]
'sk-ant-oat01-HsV8vlH...'
>>> bool(creds.get('refreshToken'))
True
>>> require_auth_token(verbose=True)[:30]
'sk-ant-oat01-HsV8vlHstjLaIYza-...'All core functionality verified:
Status: Running extended live testing before merge. |
- Fix get_auth_token_source() priority to match get_full_credentials(): 1. ANTHROPIC_AUTH_TOKEN (enterprise) 2. CLAUDE_CODE_OAUTH_TOKEN (env var override) 3. System credential store (fallback) - Fix platform-specific source labels to match test expectations - Fix test_token_expired_exactly_at_buffer timing edge case by adding 2-second buffer to avoid race conditions between test setup and check
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| "accessToken": access_token, | ||
| "refreshToken": data.get("refresh_token"), | ||
| "expiresAt": int((time.time() + expires_in) * 1000), | ||
| } |
There was a problem hiding this comment.
Refresh token lost when server omits new refresh token
The refresh_oauth_token() function sets refreshToken to data.get("refresh_token"), which can be None if the OAuth server doesn't return a new refresh token (this is valid per RFC 6749 - the refresh_token field is optional). When save_credentials() is subsequently called with this result, the None value overwrites the original valid refresh token in the credential store, permanently breaking future refresh capability until the user re-authenticates.
| expires_in = data.get("expires_in", 28800) # Default 8 hours | ||
| return { | ||
| "accessToken": access_token, | ||
| "refreshToken": data.get("refresh_token"), |
There was a problem hiding this comment.
Refresh token lost when OAuth server doesn't return new one
When refresh_oauth_token() succeeds, line 388 sets refreshToken to data.get("refresh_token"), which can be None if the OAuth server doesn't return a new refresh token (some servers reuse refresh tokens without returning them). When these credentials are saved, the original refresh token is overwritten with None, permanently breaking future token refresh capability. The function receives the old refresh token as a parameter but doesn't preserve it as a fallback.
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @apps/backend/agents/session.py:
- Around line 559-575: Replace the inline string-based auth detection with a
small helper to check exception attributes/types first: implement a function
named _is_authentication_error(exception: Exception) that checks common
attributes (status_code, code, status, response and nested response.status_code)
for 401, then checks the exception type name for authentication/unauthorized
hints, and finally falls back to the current message-pattern matching; then in
the exception handling replace the current is_auth_error block (which inspects
error_str and patterns) with a call to _is_authentication_error(e) so the code
uses attribute-based detection when available and falls back to string matching.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
apps/backend/agents/session.py
🧰 Additional context used
📓 Path-based instructions (2)
apps/backend/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
apps/backend/**/*.py: Always use the Claude Agent SDK (claude-agent-sdkpackage) for all AI interactions, never use the Anthropic API directly
Use thecreate_client()function fromapps/backend/core/client.pyto instantiate Claude SDK clients, not directClaudeSDKClientinitialization
Files:
apps/backend/agents/session.py
⚙️ CodeRabbit configuration file
apps/backend/**/*.py: Focus on Python best practices, type hints, and async patterns.
Check for proper error handling and security considerations.
Verify compatibility with Python 3.12+.
Files:
apps/backend/agents/session.py
apps/backend/agents/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use Graphiti memory system (
integrations/graphiti/) for cross-session context and knowledge graph management in agents
Files:
apps/backend/agents/session.py
🧠 Learnings (2)
📚 Learning: 2025-12-30T16:38:36.314Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T16:38:36.314Z
Learning: Applies to apps/backend/core/client.py : Implement agent-specific tool permissions in the Claude SDK client based on agent role (planner, coder, qa_reviewer, qa_fixer)
Applied to files:
apps/backend/agents/session.py
📚 Learning: 2025-12-30T16:38:36.314Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T16:38:36.314Z
Learning: Applies to apps/backend/**/*.py : Always use the Claude Agent SDK (`claude-agent-sdk` package) for all AI interactions, never use the Anthropic API directly
Applied to files:
apps/backend/agents/session.py
🧬 Code graph analysis (1)
apps/backend/agents/session.py (1)
apps/backend/core/auth.py (3)
get_full_credentials(172-205)refresh_oauth_token(354-396)save_credentials(404-421)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (4)
apps/backend/agents/session.py (4)
9-11: LGTM: Imports are well-organized and necessary.The added imports (
json,os, and auth functions) are all used in the token refresh logic. The explanatory comment on line 48 clearly documents the purpose of the auth imports.Also applies to: 48-53
58-63: LGTM: DRY helper function is well-implemented.This helper eliminates duplication across multiple auth failure scenarios and maintains consistency with the UI module's
print_statusfunction. Type hints are appropriate.
351-351: LGTM: Docstring accurately documents error behavior.The clarification that "error" includes the token refresh case (where the user must retry) correctly documents the function's behavior and aligns with the implementation.
558-635: LGTM: Reactive 401 handling is well-implemented.The token refresh logic correctly implements reactive authentication error handling with clear guard clauses, specific exception handling, and proper user guidance. The code successfully addresses all previous review feedback:
- Uses guard clauses for flat error flow
- Catches specific exceptions before catch-all
- Updates
CLAUDE_CODE_OAUTH_TOKENenvironment variable after successful save- Returns "error" status with clear retry guidance
The authentication error detection patterns (lines 563-574) are appropriately specific to avoid false positives.
Proactive token refresh is implemented. The
require_auth_token()call increate_client()(client.py:486) chains toget_auth_token()in core/auth.py, which proactively checks token expiration with the 5-minute buffer (TOKEN_REFRESH_BUFFER_SECONDS = 300 seconds, line 47) viais_token_expired()and refreshes automatically before returning the token. This ensures authentication failures are minimized during normal operation.
| "accessToken": access_token, | ||
| "refreshToken": data.get("refresh_token"), | ||
| "expiresAt": int((time.time() + expires_in) * 1000), | ||
| } |
There was a problem hiding this comment.
Lost refresh token when OAuth server doesn't return new one
When refresh_oauth_token() receives a response without a new refresh_token (which is valid per OAuth 2.0 spec), it returns {"refreshToken": None, ...}. This is then passed to save_credentials() which overwrites the existing refresh token with None. The user permanently loses refresh capability and must manually re-authenticate. The code should preserve the original refresh token when the server doesn't provide a new one.
Additional Locations (1)
| logger.warning( | ||
| f"Invalid CLAUDE_TOKEN_REFRESH_BUFFER_SECONDS='{raw_value}', using default 300 seconds" | ||
| ) | ||
| return 300 # Default 5 minutes if invalid value |
There was a problem hiding this comment.
Negative buffer seconds causes delayed token refresh
_parse_buffer_seconds() accepts negative integers without validation. Setting CLAUDE_TOKEN_REFRESH_BUFFER_SECONDS=-300 parses successfully but inverts the refresh logic: tokens would only be considered expired 300 seconds AFTER actual expiry (time.time() > expires_at_sec + 300), causing 401 errors instead of proactive refresh.
| if creds: | ||
| # Use env token but keep refresh capability from store | ||
| creds["accessToken"] = env_oauth_token | ||
| return creds |
There was a problem hiding this comment.
Env var token uses wrong expiry from store
When CLAUDE_CODE_OAUTH_TOKEN overrides the access token, get_full_credentials() keeps the store's expiresAt. This expiry doesn't correspond to the env var token. If the store's expiresAt is in the past, a perfectly valid env var token gets incorrectly flagged as expired, triggering an unnecessary refresh that may replace the user's intentionally-set token with a different one from the store's OAuth session.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| """ | ||
|
|
||
| import json | ||
| import os |
Check notice
Code scanning / CodeQL
Unused import Note test
| import json | ||
| import os | ||
| import time | ||
| from pathlib import Path |
Check notice
Code scanning / CodeQL
Unused import Note test
Address CodeRabbit review feedback: - Create helper function for authentication error detection - Use attribute-based detection first (status_code, code, status, response) - Check exception type name for auth hints - Fall back to string-based pattern matching This improves accuracy of 401 detection and follows Python best practices. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @apps/backend/agents/session.py:
- Around line 642-646: The token refresh branch updates
os.environ["CLAUDE_CODE_OAUTH_TOKEN"] only on successful save; modify the else
branch (where save_credentials fails) to also set
os.environ["CLAUDE_CODE_OAUTH_TOKEN"] = new_creds["accessToken"] so the
in-process token is current even if persistence failed, keeping the existing
logger.warning/print_status behavior and return flow in the function that
handles credential refresh/save (refer to save_credentials, new_creds, and the
token refresh block).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
apps/backend/agents/session.py
🧰 Additional context used
📓 Path-based instructions (2)
apps/backend/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
apps/backend/**/*.py: Always use the Claude Agent SDK (claude-agent-sdkpackage) for all AI interactions, never use the Anthropic API directly
Use thecreate_client()function fromapps/backend/core/client.pyto instantiate Claude SDK clients, not directClaudeSDKClientinitialization
Files:
apps/backend/agents/session.py
⚙️ CodeRabbit configuration file
apps/backend/**/*.py: Focus on Python best practices, type hints, and async patterns.
Check for proper error handling and security considerations.
Verify compatibility with Python 3.12+.
Files:
apps/backend/agents/session.py
apps/backend/agents/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use Graphiti memory system (
integrations/graphiti/) for cross-session context and knowledge graph management in agents
Files:
apps/backend/agents/session.py
🧠 Learnings (2)
📚 Learning: 2025-12-30T16:38:36.314Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T16:38:36.314Z
Learning: Applies to apps/backend/core/client.py : Implement agent-specific tool permissions in the Claude SDK client based on agent role (planner, coder, qa_reviewer, qa_fixer)
Applied to files:
apps/backend/agents/session.py
📚 Learning: 2025-12-30T16:38:36.314Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T16:38:36.314Z
Learning: Applies to apps/backend/**/*.py : Always use the Claude Agent SDK (`claude-agent-sdk` package) for all AI interactions, never use the Anthropic API directly
Applied to files:
apps/backend/agents/session.py
🧬 Code graph analysis (1)
apps/backend/agents/session.py (1)
apps/backend/core/auth.py (3)
get_full_credentials(172-205)refresh_oauth_token(354-396)save_credentials(404-421)
🔇 Additional comments (5)
apps/backend/agents/session.py (5)
9-11: LGTM: Import organization is clean.The new imports are well-organized and appropriately placed at module level. The auth helpers are imported from
core.auth(not direct Anthropic API usage), which aligns with coding guidelines. The explanatory comment at line 48 provides helpful context.Also applies to: 48-53
58-63: LGTM: Clean DRY helper for auth failures.This helper effectively eliminates message duplication across the three auth failure scenarios. The implementation is straightforward and provides consistent, actionable guidance to users.
65-110: LGTM: Robust multi-layered authentication error detection.This implementation excels at detecting 401 errors through multiple strategies:
- Attribute-based detection first (most reliable) checking
status_code,code,status, and nestedresponseobjects- Exception type name hints as a middle-ground fallback
- Specific message patterns to avoid false positives (e.g., "status 401" vs bare "401")
The layered approach effectively balances accuracy with broad compatibility across different HTTP/SDK libraries. This addresses prior review feedback perfectly.
606-665: LGTM: Token refresh implementation is solid.The token refresh logic is well-structured with clear guard clauses, comprehensive error handling, and helpful user messaging. Key strengths:
- Guard clauses make the control flow easy to follow (lines 620-627)
- Specific exception handling for expected failure modes (line 654)
- Defensive catch-all for unexpected errors with clear rationale (lines 659-665)
- Environment variable update after successful credential save (line 642)
All past review feedback has been properly addressed. The implementation balances robustness with maintainability.
398-398: LGTM: Docstring accurately documents token refresh behavior.The updated documentation clearly communicates that the "error" status includes the token refresh scenario where users must retry their command. This makes the function contract explicit for callers.
| os.environ["CLAUDE_CODE_OAUTH_TOKEN"] = new_creds["accessToken"] | ||
| else: | ||
| logger.warning("Token refreshed but failed to save") | ||
| print_status("Token refreshed but failed to save to credential store", "warning") | ||
| print(" You may need to re-authenticate on next run.") |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Optional: Consider updating environment variable in both success and failure branches.
Currently, os.environ["CLAUDE_CODE_OAUTH_TOKEN"] is only updated when save_credentials succeeds (line 642). When save fails, the refreshed token is valid but the environment variable remains stale.
While the function returns "error" status regardless, updating the environment variable in both branches could improve resilience:
- If callers implement retry logic, the current process would have the valid token
- The token IS valid even if persistence failed
- Provides better immediate experience even though next run needs re-authentication
🔎 Optional enhancement
# Success: Save and notify user
+ # Update env var in both branches since token is valid
+ os.environ["CLAUDE_CODE_OAUTH_TOKEN"] = new_creds["accessToken"]
+
if save_credentials(new_creds):
logger.info("Token refreshed successfully")
print_status("OAuth token was expired and has been refreshed", "success")
- # Update env var for any same-process callers
- os.environ["CLAUDE_CODE_OAUTH_TOKEN"] = new_creds["accessToken"]
else:
logger.warning("Token refreshed but failed to save")
print_status("Token refreshed but failed to save to credential store", "warning")
print(" You may need to re-authenticate on next run.")This is a minor enhancement since the current implementation is defensible—if persistence fails, not updating the env var makes the failure more visible.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @apps/backend/agents/session.py around lines 642-646, The token refresh
branch updates os.environ["CLAUDE_CODE_OAUTH_TOKEN"] only on successful save;
modify the else branch (where save_credentials fails) to also set
os.environ["CLAUDE_CODE_OAUTH_TOKEN"] = new_creds["accessToken"] so the
in-process token is current even if persistence failed, keeping the existing
logger.warning/print_status behavior and return flow in the function that
handles credential refresh/save (refer to save_credentials, new_creds, and the
token refresh block).
| expires_in = data.get("expires_in", 28800) # Default 8 hours | ||
| return { | ||
| "accessToken": access_token, | ||
| "refreshToken": data.get("refresh_token"), |
There was a problem hiding this comment.
Refresh token lost when OAuth server doesn't rotate tokens
When refresh_oauth_token is called, it returns data.get("refresh_token") which will be None if the OAuth server doesn't include a new refresh token in its response (rotating refresh tokens is optional per OAuth2 spec). This None value gets saved to the credential store via save_credentials, permanently overwriting the original refresh token. After one successful refresh, subsequent refresh attempts will fail because refreshToken is now None. The fix is to preserve the original refresh token if no new one is returned: data.get("refresh_token") or refresh_token.
| "accessToken": access_token, | ||
| "refreshToken": data.get("refresh_token"), | ||
| "expiresAt": int((time.time() + expires_in) * 1000), | ||
| } |
There was a problem hiding this comment.
Refresh token lost if OAuth response omits new token
The refresh_oauth_token function returns data.get("refresh_token") directly from the OAuth response at line 388. According to OAuth 2.0 RFC 6749, servers MAY issue a new refresh token but are not required to. If the server doesn't return a new refresh_token in the response, this value becomes None. When save_credentials is subsequently called (in both get_auth_token and run_agent_session), this None overwrites the previously valid refresh token in the credential store. On the next token expiration, the code will fail with "no refresh token available" because the original valid refresh token was lost. The fix would be to fall back to the original refresh token when the response doesn't include a new one.
| else: | ||
| logger.warning("Token refreshed but failed to save") | ||
| print_status("Token refreshed but failed to save to credential store", "warning") | ||
| print(" You may need to re-authenticate on next run.") |
There was a problem hiding this comment.
New token lost when credential save fails in session
In session.py, when token refresh succeeds but save_credentials() fails, the environment variable CLAUDE_CODE_OAUTH_TOKEN is not updated (line 642 is inside the success branch). This is inconsistent with get_auth_token() in auth.py where the env var update happens unconditionally after successful refresh (line 614 is outside the if/else). When save fails in session.py, the valid new token is discarded, and same-process retries will continue using the old expired token, causing repeated 401 errors. The comment at line 651 incorrectly states "Token is already refreshed and saved" which may not be true.
Additional Locations (1)
4 participants
Summary
Fixes #563 - Implement automatic OAuth token refresh to prevent 401 authentication errors during long-running agent sessions.
Problem
When OAuth tokens expire (after ~8 hours), Auto-Claude fails with:
Users must manually run
/loginto re-authenticate, disrupting autonomous workflows.Solution
refreshTokenandexpiresAtFiles Changed
apps/backend/core/auth.pyapps/backend/agents/session.pyapps/backend/core/client.pyrequire_auth_token(verbose=True)apps/backend/.env.exampleapps/backend/requirements.txtrequestsdependencytests/test_auth_token_refresh.pyKey Implementation Details
status 401,http 401,401 unauthorized) to avoid false positives_get_full_credentials_from_file(),_get_full_credentials_from_paths(),_save_credentials_to_file(),_print_auth_failure_guidance()umask(0o077)for atomic file permissions (prevents TOCTOU race)-Uflag for atomic update (no delete + add)subprocess.TimeoutExpired,subprocess.SubprocessError,OSError,json.JSONDecodeError,KeyErrorlogger.warningCLAUDE_CODE_OAUTH_TOKENafter successful refresh for same-process callersCommits
f5a5634f2fb5eeaf798bd59abdca_parse_buffer_seconds()45a84a3f5feba3963249a5029f90get_auth_token_source()c3543f8a6fd9746571a440f6fc43640a31da84217f69f680a4141d48e6a70d7Reviewer Feedback Addressed
Gemini Code Assist (27 issues) ✅
-Uflag)requests.exceptions.RequestExceptionusedexcept Exceptionreplaced with specific exceptionsget_auth_token_source()synced with new priority logic_print_auth_failure_guidance()DRY helper for auth failure messageslogger.warninglogger.warningget_auth_token_source()priority logicCodeRabbit (28 issues) ✅
_parse_buffer_seconds()print_status()used for UI consistencyget_auth_token()refresh flowsk-ant-oat01-formattemp_dirwith pytesttmp_pathSummary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.