Skip to content

Conversation

matdev83
Copy link
Owner

@matdev83 matdev83 commented Oct 12, 2025

Summary

  • scope per-request identity headers in the OpenAI and OpenRouter connectors so concurrent calls cannot leak headers across sessions
  • add a regression test that issues concurrent chat completions to ensure identity headers remain isolated

Testing

  • python -m pytest -o addopts="" tests/unit/openai_connector_tests/test_identity_scoping.py
  • python -m pytest -o addopts=""
    • fails: missing optional test dependencies (pytest-asyncio, pytest_httpx, respx, hypothesis)

https://chatgpt.com/codex/tasks/task_e_68ec25089a4c8333a7c641442c526119

Summary by CodeRabbit

  • Refactor

    • Switched to per-request identity handling for API headers, eliminating shared state and improving isolation across requests.
    • Ensures identity-specific headers apply only to the current request, enhancing reliability in concurrent scenarios.
  • Tests

    • Added a concurrency test verifying identity header isolation across simultaneous requests.
  • Bug Fixes

    • Prevented unintended carryover of identity headers between requests by removing global identity state.
Copy link

coderabbitai bot commented Oct 12, 2025

Walkthrough

Per-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

Cohort / File(s) Summary
OpenAI per-request identity
src/connectors/openai.py
Refactored to remove cached self.identity; added identity parameter to get_headers, chat_completions, and responses; header construction now uses the provided per-call identity.
OpenRouter per-request identity
src/connectors/openrouter.py
get_headers now accepts optional identity and merges its headers; removed self.identity assignment; returns ensure_loop_guard_header(headers).
Concurrency test for identity isolation
tests/unit/openai_connector_tests/test_identity_scoping.py
Added async test asserting per-call header isolation across concurrent chat_completions using patched handler and synchronization.

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 
Loading
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 
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

I hop between requests, light on my feet,
No burrowed state, just headers neat.
Two trails run parallel, never collide,
Each carrot-tagged identity rides.
Async winds whistle—tests confirm the tune,
Per-call moons, no shared monsoon. 🌕🐇

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 36.36% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title accurately and succinctly summarizes the primary change of ensuring per-request identity scoping within the connectors, aligning with the removal of global identity state and the addition of tests for concurrent isolation.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch codex/fix-unexpected-state-changes-in-proxy-fj99mp

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a 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

Comment on lines 273 to +274
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)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 👍 / 👎.

Copy link

@coderabbitai coderabbitai bot left a 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's chat_completions method (line 264 in src/connectors/openai.py) will call get_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:

  1. Pass identity=None to the parent and rely entirely on the manually-built headers_override, or
  2. Omit identity headers from headers_override and let the parent handle them via get_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

📥 Commits

Reviewing files that changed from the base of the PR and between 4b959ef and e1b6472.

📒 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. The ensure_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 of asyncio.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 around identity.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-request identity parameter and consistently passes it to get_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-request identity parameter (line 538) and passes it to get_headers(identity) (line 607), maintaining consistency with the chat_completions implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

1 participant