- Notifications
You must be signed in to change notification settings - Fork 0
Fix connector identity scoping #673
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Conversation
WalkthroughPer-request identity handling replaces stored connector identity. get_headers now accepts an optional identity and merges its headers per call. chat_completions and responses propagate identity to header resolution. OpenRouterBackend mirrors this change and wraps headers with ensure_loop_guard_header. A new concurrent test validates isolation of identity-scoped headers. Changes
Sequence Diagram(s)sequenceDiagram participant Caller participant OpenAIConnector participant HeaderBuilder as get_headers(identity) participant OpenAIAPI as OpenAI API Caller->>OpenAIConnector: chat_completions(..., identity) OpenAIConnector->>HeaderBuilder: resolve headers(identity) Note right of HeaderBuilder: Merge base headers + identity.headers (if any) HeaderBuilder-->>OpenAIConnector: headers OpenAIConnector->>OpenAIAPI: POST /chat/completions with headers OpenAIAPI-->>OpenAIConnector: Response OpenAIConnector-->>Caller: Result sequenceDiagram participant Caller participant OpenRouter as OpenRouterBackend participant HeaderBuilder as get_headers(identity) participant Guard as ensure_loop_guard_header participant OpenRouterAPI as OpenRouter API Caller->>OpenRouter: chat_completions(..., identity) OpenRouter->>HeaderBuilder: resolve headers(identity) HeaderBuilder-->>OpenRouter: headers OpenRouter->>Guard: wrap headers Guard-->>OpenRouter: guarded headers OpenRouter->>OpenRouterAPI: POST with guarded headers OpenRouterAPI-->>OpenRouter: Response OpenRouter-->>Caller: Result 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting
headers = merged_headers | ||
else: | ||
try: | ||
# Always update the cached identity so that per-request | ||
# identity headers do not leak between calls. Downstream | ||
# callers rely on identity-specific headers being scoped to | ||
# a single request. | ||
self.identity = identity | ||
headers = self.get_headers() | ||
headers = self.get_headers(identity) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passing identity into get_headers breaks OpenAI subclasses
The new call sites invoke self.get_headers(identity)
so the identity never touches instance state anymore. However, several existing subclasses (ZAIConnector.get_headers
and QwenOAuthConnector.get_headers
) still override get_headers
without an identity parameter. Any call to chat_completions
or responses
on those backends will now raise TypeError: …get_headers() takes 1 positional argument but 2 were given
, even when no identity is supplied, because the base method unconditionally forwards the argument. This makes the ZAI and Qwen OAuth backends unusable. Consider updating the overrides or guarding the call so subclasses that don’t accept an identity remain compatible.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/connectors/openrouter.py (1)
214-220
: Consider removing redundant identity header merging.This code manually merges identity headers into
headers_override
, but the parent'schat_completions
method (line 264 insrc/connectors/openai.py
) will callget_headers(identity)
again, which also merges identity headers. While functionally correct (the second merge overwrites the first), this results in redundant computation of identity headers.Consider refactoring to avoid duplicate identity resolution. You could either:
- Pass
identity=None
to the parent and rely entirely on the manually-builtheaders_override
, or- Omit identity headers from
headers_override
and let the parent handle them viaget_headers(identity)
Option 2 would be cleaner and more maintainable:
if identity is not None: - try: - identity_headers = identity.get_resolved_headers(None) - except Exception: - identity_headers = {} - if identity_headers: - headers_override.update(identity_headers) + # Identity headers will be merged by parent's get_headers(identity) call + pass
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/connectors/openai.py
(4 hunks)src/connectors/openrouter.py
(1 hunks)tests/unit/openai_connector_tests/test_identity_scoping.py
(2 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:
tests/unit/openai_connector_tests/test_identity_scoping.py
src/connectors/openai.py
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/openai.py
src/connectors/openrouter.py
🧬 Code graph analysis (3)
tests/unit/openai_connector_tests/test_identity_scoping.py (2)
src/connectors/openai.py (1)
chat_completions
(223-302)src/core/domain/responses.py (1)
ResponseEnvelope
(10-23)
src/connectors/openai.py (5)
src/connectors/openrouter.py (1)
get_headers
(93-107)tests/mocks/backend_factory.py (1)
get_headers
(30-33)src/core/interfaces/configuration.py (2)
IAppIdentityConfig
(38-53)get_resolved_headers
(42-53)tests/unit/openai_connector_tests/test_identity_scoping.py (1)
get_resolved_headers
(20-23)src/core/domain/configuration/app_identity_config.py (1)
get_resolved_headers
(48-66)
src/connectors/openrouter.py (5)
src/connectors/openai.py (1)
get_headers
(97-115)tests/mocks/backend_factory.py (1)
get_headers
(30-33)src/core/interfaces/configuration.py (2)
IAppIdentityConfig
(38-53)get_resolved_headers
(42-53)tests/unit/openai_connector_tests/test_identity_scoping.py (1)
get_resolved_headers
(20-23)src/core/domain/configuration/app_identity_config.py (1)
get_resolved_headers
(48-66)
🪛 Ruff (0.13.3)
tests/unit/openai_connector_tests/test_identity_scoping.py
178-178: Unused function argument: self
(ARG001)
179-179: Unused function argument: url
(ARG001)
180-180: Unused function argument: payload
(ARG001)
⏰ 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 (5)
src/connectors/openrouter.py (1)
93-107
: LGTM! Per-request identity handling implemented correctly.The
get_headers
method now accepts an optional per-request identity and properly merges its headers into the resolved headers. Theensure_loop_guard_header
wrapper is correctly applied before returning.tests/unit/openai_connector_tests/test_identity_scoping.py (1)
165-214
: Excellent regression test for concurrent identity isolation!The test properly validates that identity headers remain isolated across concurrent
chat_completions
calls. The use ofasyncio.Event
for synchronization ensures both calls are truly concurrent when headers are captured, making this a robust test case.The static analysis hints about unused function arguments (lines 178-180) are false positives—these parameters are required to match the signature of the method being mocked.
src/connectors/openai.py (3)
97-115
: LGTM! Per-request identity handling implemented correctly.The
get_headers
method now properly accepts an optional per-request identity parameter and merges its headers into the request headers. The defensive try-except aroundidentity.get_resolved_headers(None)
ensures resilience when identity resolution fails, while missing authorization is still caught downstream (line 423).
223-302
: LGTM! Identity parameter threaded through correctly.The
chat_completions
method signature now accepts a per-requestidentity
parameter and consistently passes it toget_headers(identity)
in both the headers_override path (line 264) and the default path (line 274). This eliminates the risk of identity leakage across concurrent requests.
533-645
: LGTM! Responses API follows the same identity pattern.The
responses
method signature now accepts a per-requestidentity
parameter (line 538) and passes it toget_headers(identity)
(line 607), maintaining consistency with thechat_completions
implementation.
Summary
Testing
https://chatgpt.com/codex/tasks/task_e_68ec25089a4c8333a7c641442c526119
Summary by CodeRabbit
Refactor
Tests
Bug Fixes