feat: Nous Research provider support with OAuth auto-refresh#737
Open
phuongvm wants to merge 2 commits into
Open
feat: Nous Research provider support with OAuth auto-refresh#737phuongvm wants to merge 2 commits into
phuongvm wants to merge 2 commits into
Conversation
Adds 'nous' as a supported LLM transport with automatic OAuth credential refresh on 401 AuthenticationError. - New 'nous' transport in ModelTransport - OAuth refresh module (src/llm/nous_refresh.py) with secure state files - OpenAIBackend autorefresh integration (parse/create/stream paths) - NousAuthError exception class - Config: NOUS_API_KEY, NOUS_BASE_URL defaults - Fix missing imports (BadRequestError, LengthFinishReasonError) - Includes tests for autorefresh and credential loading.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
WalkthroughThis PR integrates Nous as a supported LLM transport by adding OAuth credential auto-refresh to the OpenAI backend: configuration and exception types, a Nous refresh orchestrator module, backend interception and retry on 401 when enabled, and tests for both refresh logic and backend retry behavior. ChangesNous credential auto-refresh with OpenAI backend integration
Sequence DiagramsequenceDiagram
participant Client
participant OpenAIBackend
participant refresh_nous_credentials
participant OpenAI_API
Client->>OpenAIBackend: complete/stream with is_nous=true
OpenAIBackend->>OpenAI_API: chat.completions request
OpenAI_API-->>OpenAIBackend: AuthenticationError (401)
OpenAIBackend->>refresh_nous_credentials: get new agent_key
refresh_nous_credentials-->>OpenAIBackend: agent_key
OpenAIBackend->>OpenAI_API: update client.api_key
OpenAIBackend->>OpenAI_API: retry chat.completions
OpenAI_API-->>OpenAIBackend: response
OpenAIBackend-->>Client: return content
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
tests/llm/test_nous_refresh.py (1)
156-159: ⚡ Quick winMove these imports to the top of the file.
SimpleNamespace,Mock, andsysare imported at the bottom but used in the test at lines 111-141. It only works because names resolve at call time; placing them at module top is clearer and isort-compliant.♻️ Proposed change
import json import os +import sys from pathlib import Path -from unittest.mock import AsyncMock, MagicMock, patch +from types import SimpleNamespace +from unittest.mock import AsyncMock, MagicMock, Mock, patch- - -# Helper for SimpleNamespace -from types import SimpleNamespace -from unittest.mock import Mock -import sysAs per coding guidelines: "Follow isort conventions with absolute imports preferred in Python code".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/llm/test_nous_refresh.py` around lines 156 - 159, Move the three imports (SimpleNamespace, Mock, and sys) from their current bottom location into the module-level import block at the top of the test file so they are declared with the other imports and conform to isort conventions; specifically add "from types import SimpleNamespace", "from unittest.mock import Mock", and "import sys" to the top import section so tests referencing SimpleNamespace, Mock, and sys in the test_nous_refresh functions (lines around 111–141) resolve clearly and remain isort-compliant.src/llm/nous_refresh.py (2)
50-54: 💤 Low valueAvoid catching blind
Exception; narrow to the expected error types.The bare
except Exceptionhere (and the same pattern at lines 101 and 122) swallows unexpected errors. For these file/JSON reads, catch(OSError, json.JSONDecodeError)instead so genuine programming errors surface.♻️ Proposed change for load_state
if STATE_FILE.exists(): try: return json.loads(STATE_FILE.read_text()) - except Exception as exc: + except (OSError, json.JSONDecodeError) as exc: logger.warning("Failed to parse %s: %s", STATE_FILE, exc)As per coding guidelines: "Use explicit error handling with appropriate exception types in Python code".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/llm/nous_refresh.py` around lines 50 - 54, Replace the broad except Exception handlers around state file reading/parsing with explicit exceptions to avoid swallowing unrelated errors: catch (OSError, json.JSONDecodeError) for the blocks that call STATE_FILE.read_text() and json.loads(...) (the current try/except around STATE_FILE.exists() and the similar handlers later in the file). Update the except clauses to "except (OSError, json.JSONDecodeError) as exc" and keep the same logger.warning message, so filesystem and JSON parse errors are handled but other exceptions still surface.
162-177: 💤 Low valueHandle missing
refresh_tokenin Nous refresh responseIn
src/llm/nous_refresh.py(lines 162-177),data["refresh_token"]can raiseKeyErrorif the token endpoint omitsrefresh_token, aborting the refresh even though a newaccess_tokenwas obtained. Keep the existingrefresh_tokenwhen it’s absent.♻️ Proposed change
data = resp.json() - return data["access_token"], data["refresh_token"] + return data["access_token"], data.get("refresh_token", refresh_token)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/llm/nous_refresh.py` around lines 162 - 177, In refresh_access_token(refresh_token: str) update the return logic to handle a missing refresh_token in the response: after parsing data = resp.json(), extract access = data["access_token"] and set new_refresh = data.get("refresh_token", refresh_token) so that if the token endpoint omits refresh_token we keep the existing refresh_token parameter; return (access, new_refresh) and avoid raising KeyError when refresh_token is absent (keep existing variable names: resp, data, refresh_token, refresh_access_token).tests/llm/test_backends/test_nous_autorefresh.py (1)
119-180: ⚡ Quick winAdd coverage for the structured
parse()auto-refresh branch.The create and stream paths are exercised, but the structured-output path (
response_format=<BaseModel subclass>) has its own 401 refresh branch (openai.py Lines 177-193) wrapped by additionalLengthFinishReasonError/BadRequestErrorhandlers. That branch is currently untested, so a regression there (e.g. when consolidating onto_call_with_autorefresh) wouldn't be caught.Want me to add a
test_nous_backend_parse_auto_refreshcase that callscomplete(..., response_format=SomeModel)withparseraising 401 then succeeding?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/llm/test_backends/test_nous_autorefresh.py` around lines 119 - 180, Add a new async test (e.g., test_nous_backend_parse_auto_refresh) that mirrors the existing stream/create auto-refresh test but exercises the structured-output (response_format) branch: mock OpenAIBackend.complete to call into the path that uses response_format=<BaseModel subclass> and arrange for the first parse attempt to raise an AuthenticationError (401) and subsequent call to succeed; ensure refresh_nous_credentials is patched to return "new_key", assert mock_client.api_key updated, that refresh was called once, and validate the final parsed output and token/done semantics; reference OpenAIBackend.complete, the parse() call and the refresh_nous_credentials patch, and include handling of the LengthFinishReasonError/BadRequestError wrappers so the test specifically triggers the 401 inside parse and then verifies retry behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/config.py`:
- Line 25: The _normalize_model_transport function currently rejects shorthand
forms like "nous/<model>" even though ModelTransport includes "nous"; update
_normalize_model_transport to treat "nous" like the other providers by accepting
and normalizing shorthand transport strings that start with "nous/" (or equal
"nous") to the canonical transport and model parts. Locate the
_normalize_model_transport function and add "nous" to the accepted transport
list/regex and the mapping logic so inputs such as "nous/whatever" are parsed
and returned in the same normalized tuple/format as "anthropic/...",
"openai/...", and "gemini/...".
In `@src/exceptions.py`:
- Around line 136-140: Change NousAuthError to subclass HonchoException instead
of Exception so FastAPI maps it to the HonchoException handler; set its
status_code to 401 and detail to "Nous authentication failed". Locate the class
NousAuthError in src/exceptions.py, replace its base class with HonchoException
and add the status_code and detail attributes (keeping the docstring), ensuring
the exception will be handled by the HonchoException handler.
In `@src/llm/backends/openai.py`:
- Around line 122-144: The helper _call_with_autorefresh is never used and the
refresh-and-retry logic is copy-pasted into the parse/create/stream paths;
remove those inline retry blocks and call
_call_with_autorefresh(use_parse=True/False, params=params) from the parse,
create and stream code paths so all retries go through the single helper; inside
the helper keep the deferred import but change it to an absolute import (from
src.llm.nous_refresh import refresh_nous_credentials) and ensure its behavior
(setting self._client.api_key, updating settings.LLM.NOUS_API_KEY under
contextlib.suppress, logging, and re-raising if refresh fails) matches the
original inline logic so exceptions like LengthFinishReasonError still propagate
to the outer handlers.
In `@src/llm/nous_refresh.py`:
- Around line 78-84: The directory creation for STATE_FILE currently uses
STATE_FILE.parent.mkdir(parents=True, exist_ok=True) but does not enforce the
requested 0o700 permissions; explicitly set the directory mode to 0o700 after
ensuring it exists (e.g., call os.chmod on STATE_FILE.parent to 0o700) so both
newly created and pre-existing directories are tightened, then continue with the
existing atomic write flow (tmp = STATE_FILE.with_suffix(".tmp"),
tmp.write_text(...), os.chmod(tmp, 0o600), tmp.replace(STATE_FILE),
os.chmod(STATE_FILE, 0o600)).
In `@tests/llm/test_nous_refresh.py`:
- Around line 89-153: The test fails because STATE_FILE is bound at import and
load_state() is never stubbed so refresh_nous_credentials() sees no
refresh_token; fix by stubbing load_state or overriding STATE_FILE in the
imported module before calling refresh_nous_credentials(): e.g., in the test set
refresh_mod.load_state to a callable that returns {"refresh_token":
"some_token"} (or monkeypatch refresh_mod.STATE_FILE to point to the
tmp_path/state.json and write that file) so refresh_nous_credentials() has a
refresh_token to use; ensure you patch before awaiting
refresh_nous_credentials() and restore originals after the test.
---
Nitpick comments:
In `@src/llm/nous_refresh.py`:
- Around line 50-54: Replace the broad except Exception handlers around state
file reading/parsing with explicit exceptions to avoid swallowing unrelated
errors: catch (OSError, json.JSONDecodeError) for the blocks that call
STATE_FILE.read_text() and json.loads(...) (the current try/except around
STATE_FILE.exists() and the similar handlers later in the file). Update the
except clauses to "except (OSError, json.JSONDecodeError) as exc" and keep the
same logger.warning message, so filesystem and JSON parse errors are handled but
other exceptions still surface.
- Around line 162-177: In refresh_access_token(refresh_token: str) update the
return logic to handle a missing refresh_token in the response: after parsing
data = resp.json(), extract access = data["access_token"] and set new_refresh =
data.get("refresh_token", refresh_token) so that if the token endpoint omits
refresh_token we keep the existing refresh_token parameter; return (access,
new_refresh) and avoid raising KeyError when refresh_token is absent (keep
existing variable names: resp, data, refresh_token, refresh_access_token).
In `@tests/llm/test_backends/test_nous_autorefresh.py`:
- Around line 119-180: Add a new async test (e.g.,
test_nous_backend_parse_auto_refresh) that mirrors the existing stream/create
auto-refresh test but exercises the structured-output (response_format) branch:
mock OpenAIBackend.complete to call into the path that uses
response_format=<BaseModel subclass> and arrange for the first parse attempt to
raise an AuthenticationError (401) and subsequent call to succeed; ensure
refresh_nous_credentials is patched to return "new_key", assert
mock_client.api_key updated, that refresh was called once, and validate the
final parsed output and token/done semantics; reference OpenAIBackend.complete,
the parse() call and the refresh_nous_credentials patch, and include handling of
the LengthFinishReasonError/BadRequestError wrappers so the test specifically
triggers the 401 inside parse and then verifies retry behavior.
In `@tests/llm/test_nous_refresh.py`:
- Around line 156-159: Move the three imports (SimpleNamespace, Mock, and sys)
from their current bottom location into the module-level import block at the top
of the test file so they are declared with the other imports and conform to
isort conventions; specifically add "from types import SimpleNamespace", "from
unittest.mock import Mock", and "import sys" to the top import section so tests
referencing SimpleNamespace, Mock, and sys in the test_nous_refresh functions
(lines around 111–141) resolve clearly and remain isort-compliant.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5cbf1bc4-0295-4b81-89fe-d745cf9b81db
📒 Files selected for processing (7)
src/config.pysrc/exceptions.pysrc/llm/backends/openai.pysrc/llm/credentials.pysrc/llm/nous_refresh.pytests/llm/test_backends/test_nous_autorefresh.pytests/llm/test_nous_refresh.py
- Add 'nous' and 'lmstudio' to _normalize_model_transport shorthand (config.py) - NousAuthError subclasses HonchoException with status_code=401 (exceptions.py) - Refactor 3 inline retry blocks into _call_with_autorefresh helper (openai.py) - Narrow except Exception to (OSError, json.JSONDecodeError) (nous_refresh.py) - Set state directory permissions to 0o700 (nous_refresh.py) - Handle missing refresh_token in response with .get() fallback (nous_refresh.py) - Move imports to top of test file, stub load_state (test_nous_refresh.py)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds
nousas a supported LLM transport with automatic OAuth credential refresh on 401 AuthenticationError.Key Changes
nousadded toModelTransportliteralsrc/llm/nous_refresh.py— handles token refresh, secure state file management (0700/0600 permissions), and agent key minting_call_with_autorefresh()helper eliminates DRY violation; used in parse, create, and stream pathsBadRequestError,LengthFinishReasonErrornow properly importedNOUS_API_KEY,NOUS_BASE_URL(defaults tohttps://inference-api.nousresearch.com/v1)NousAuthErrorfor OAuth-specific failuresConfiguration
NOUS_API_KEY=*** NOUS_BASE_URL=https://inference-api.nousresearch.com/v1Tests (11 tests)
Split from #692 per maintainer request to separate Nous provider support from LLM fallback.
Summary by CodeRabbit
New Features
Bug Fixes
Tests