Fix OpenRouter header provider argument handling #647
Conversation
WalkthroughThe change adds enhanced header resolution and authentication validation in src/connectors/openrouter.py, including a helper to verify Authorization headers, a reworked provider-based header resolver with error aggregation, and per-call overrides in chat_completions for headers, keys, and base URL, adjusting request URL/payload for streaming and non-streaming. Changes
Sequence Diagram(s)sequenceDiagram autonumber participant C as Caller participant ORC as OpenRouterConnector.chat_completions participant HP as HeadersProvider (optional) participant OR as OpenRouter API C->>ORC: chat_completions(prompt, kwargs) Note over ORC: Resolve headers alt Per-call provider/key overrides ORC->>HP: Try provider with (key_name | api_key | context) HP-->>ORC: headers or error alt Valid headers with API key in Authorization ORC->>ORC: Merge with identity-derived headers (override rules) else Invalid/missing Authorization ORC->>ORC: Aggregate errors ORC-->>C: AuthenticationError end else No overrides ORC->>ORC: Use default/identity headers end Note over ORC: Build URL & payload alt stream=True ORC->>OR: POST /.../chat/completions (stream) OR-->>ORC: SSE stream ORC-->>C: Streamed tokens/events else stream=False ORC->>OR: POST /.../chat/completions (non-stream) OR-->>ORC: JSON response ORC-->>C: Completion result end Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/connectors/openrouter.py (1)
248-259: Critical: Silent authentication failure may cause obscure 401 errors.Lines 250-253 catch
AuthenticationErrorand silently continue by settingheaders_override = None. This means if the headers provider fails authentication, the request proceeds with incomplete headers, potentially leading to confusing 401 errors from the OpenRouter backend.If the provider is configured but fails, this should be a hard failure. The fallback logic on Lines 258-259 adds a default Authorization header, but if the provider failed due to invalid credentials or configuration, the request will still fail downstream with less clear error messages.
Apply this diff to fail fast on authentication errors:
headers_override: dict[str, str] | None = None if self.headers_provider: - try: - headers_override = dict(self._resolve_headers_from_provider()) - except AuthenticationError: - headers_override = None + headers_override = dict(self._resolve_headers_from_provider()) if headers_override is None: headers_override = {}If you need graceful degradation, consider logging at WARNING level before continuing:
headers_override: dict[str, str] | None = None if self.headers_provider: try: headers_override = dict(self._resolve_headers_from_provider()) except AuthenticationError as exc: + logger.warning( + "Headers provider failed, falling back to basic auth: %s", + exc, + exc_info=True + ) headers_override = None
🧹 Nitpick comments (1)
src/connectors/openrouter.py (1)
61-75: LGTM! Consider more precise validation.The validation logic correctly handles case-insensitive header matching and includes type safety. The substring check on Line 72 is simple but adequate for Authorization headers, though it could theoretically match the API key appearing in comments or other contexts.
Consider using a more precise pattern like checking for
f"Bearer {api_key}"if the Authorization header format is well-defined, but the current approach is acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/connectors/openrouter.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Avoid using emojis in Python code comments or docstrings
Follow PEP 8 and use type hints for all functions
Import order must be: standard library, third-party, then local imports, separated by blank lines
Naming conventions: snake_case for variables and functions; PascalCase for classes
Error handling: use specific exceptions and include meaningful error messages
Prefer f-strings for string formatting
Files:
src/connectors/openrouter.py
src/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.py: Catch specific exceptions; avoid broad except Exception blocks
If a broad exception must be caught, log with exc_info=True and re-raise a specific custom exception
Use the most specific exception class from src.core.common.exceptions that accurately describes the error
Provide clear, helpful error messages and include relevant details in the details dictionary of custom exceptions
Files:
src/connectors/openrouter.py
🧬 Code graph analysis (1)
src/connectors/openrouter.py (1)
src/core/common/exceptions.py (1)
AuthenticationError(56-65)
🪛 Ruff (0.13.3)
src/connectors/openrouter.py
94-94: Do not catch blind exception: Exception
(BLE001)
⏰ 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: test (3.10)
🔇 Additional comments (4)
src/connectors/openrouter.py (4)
4-4: LGTM!The
Mappingimport is correctly placed and necessary for the type checking in_try_provider_call.
261-277: LGTM! Well-structured per-call override pattern.The per-call override mechanism is well-implemented:
- Identity headers are extracted with defensive exception handling (Lines 261-267)
- Headers override is set to
Noneif empty (Lines 269-270), which likely signals to the parent class to use defaults- Overrides are correctly passed through kwargs to parent methods (Lines 275-277)
The try-finally pattern ensures instance state is properly restored even if exceptions occur.
282-308: LGTM! Comprehensive payload construction.The payload building correctly handles all OpenRouter-specific parameters while maintaining compatibility with the domain model. The conditional checks ensure only non-None values are added, and the extra_body handling provides flexibility for custom parameters.
310-329: LGTM! Request execution paths are correct.The URL construction and request execution are properly implemented:
- URL is correctly built from api_base with path joining (Line 311)
- Both streaming (Lines 314-325) and non-streaming (Lines 327-329) paths pass the correct parameters
- Headers override and payload are consistently applied
| def _try_provider_call(*args: Any) -> dict[str, str] | None: | ||
| try: | ||
| return provider(self.key_name, self.api_key) | ||
| result = provider(*args) | ||
| except (AttributeError, TypeError) as exc: | ||
| errors.append(exc) | ||
| return None | ||
| except Exception as exc: | ||
| errors.append(exc) | ||
| return None | ||
| | ||
| context = self._build_openrouter_header_context() | ||
| try: | ||
| return provider(context, self.api_key) | ||
| except Exception as exc: # pragma: no cover - should not happen in normal flow | ||
| if errors: | ||
| logger.debug( | ||
| "Headers provider rejected key_name input: %s", | ||
| errors[-1], | ||
| exc_info=True, | ||
| if not isinstance(result, Mapping): | ||
| errors.append( | ||
| TypeError("OpenRouter headers provider must return a mapping."), | ||
| ) | ||
| raise AuthenticationError( | ||
| message="OpenRouter headers provider failed to produce headers.", | ||
| code="missing_credentials", | ||
| ) from exc | ||
| return None | ||
| | ||
| headers = dict(result) | ||
| if not self._authorization_includes_api_key(headers, self.api_key): | ||
| errors.append( | ||
| ValueError( | ||
| "OpenRouter headers provider did not include API key in Authorization header.", | ||
| ) | ||
| ) | ||
| return None | ||
| | ||
| return headers | ||
| | ||
| if self.key_name is not None: | ||
| headers = _try_provider_call(self.key_name, self.api_key) | ||
| if headers is not None: | ||
| return headers | ||
| | ||
| headers = _try_provider_call(self.api_key, self.key_name) | ||
| if headers is not None: | ||
| return headers | ||
| | ||
| context = self._build_openrouter_header_context() | ||
| headers = _try_provider_call(context, self.api_key) | ||
| if headers is not None: | ||
| return headers | ||
| | ||
| if errors: | ||
| logger.debug( | ||
| "Headers provider attempts failed: %s", | ||
| errors[-1], | ||
| exc_info=True, | ||
| ) | ||
| raise AuthenticationError( | ||
| message="OpenRouter headers provider failed to produce headers.", | ||
| code="missing_credentials", | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Improve error handling to follow coding guidelines.
The provider invocation logic with multiple fallback patterns is well-designed. However, there are coding guideline violations in the error handling:
-
Line 94: The broad
Exceptioncatch violates the guideline that states "If a broad exception must be caught, log with exc_info=True and re-raise a specific custom exception." While errors are collected and logged later (Lines 130-134), the intermediate exceptions lose their context. -
Lines 100, 108: Error messages don't include details about what the provider returned or which invocation pattern failed, making debugging difficult.
Consider this refactor to improve error context and align with guidelines:
def _try_provider_call(*args: Any) -> dict[str, str] | None: try: result = provider(*args) except (AttributeError, TypeError) as exc: errors.append(exc) return None except Exception as exc: - errors.append(exc) + logger.debug( + "Provider call with args %s raised unexpected exception", + args, + exc_info=True + ) + errors.append( + AuthenticationError( + message=f"Provider raised unexpected error: {type(exc).__name__}", + details={"original_error": str(exc), "args": str(args)}, + ) + ) return None if not isinstance(result, Mapping): errors.append( - TypeError("OpenRouter headers provider must return a mapping."), + TypeError( + f"OpenRouter headers provider must return a mapping, got {type(result).__name__} with args {args}" + ), ) return None headers = dict(result) if not self._authorization_includes_api_key(headers, self.api_key): errors.append( ValueError( - "OpenRouter headers provider did not include API key in Authorization header.", + f"OpenRouter headers provider did not include API key in Authorization header. " + f"Headers: {list(headers.keys())}, Args: {args}" ) ) return None return headersAs per coding guidelines
🏁 Script executed:
#!/bin/bash set -e # Locate the definition and exception handling in _try_provider_call rg -nP "def _try_provider_call" -C3 src/connectors/openrouter.py # Find broad exception catch rg -nP "except Exception" -C3 src/connectors/openrouter.py # Check mapping validation error message rg -nP 'TypeError\("OpenRouter headers provider must return a mapping' -C1 src/connectors/openrouter.py # Check debug logging placement rg -nP "logger.debug" -C3 src/connectors/openrouter.pyLength of output: 1221
Refactor broad exception catch and enrich error context.
- In
_try_provider_call(around line 94), avoid a bareexcept Exception; log withexc_info=Trueand wrap or re-raise a specific connector exception per guidelines. - Enhance the mapping and authorization error messages to include the actual result type and provider args for clearer debugging.
🧰 Tools
🪛 Ruff (0.13.3)
94-94: Do not catch blind exception: Exception
(BLE001)
1 participant
Summary
Testing
pytest --override-ini addopts="" tests/unit/openrouter_connector_tests/test_headers_provider_config_dict.py::test_openrouter_headers_provider_accepts_config_dict -q(fails: ModuleNotFoundError: No module named 'pytest_httpx')https://chatgpt.com/codex/tasks/task_e_68ec26beb3b0833397f9204b4e11ff80
Summary by CodeRabbit
New Features
Bug Fixes
Refactor