Ensure fallback request managers reuse DI wire capture#609
Conversation
WalkthroughControllers now attempt to resolve an optional IWireCapture from local or global providers and pass it to BackendRequestManager. BackendRequestManager’s constructor adds a wire_capture parameter. Resolution is wrapped in try/except to avoid failures. Invocation sites in Anthropic and Chat controllers are updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Controller as Chat/Anthropic Controller
participant LocalSP as Local ServiceProvider
participant GlobalSP as Global ServiceProvider
participant BRM as BackendRequestManager
participant WireCap as IWireCapture (optional)
User->>Controller: Initiate request
Controller->>LocalSP: resolve(IWireCapture)
alt Found locally
LocalSP-->>Controller: WireCapture instance
else Not found / error
Controller->>GlobalSP: resolve(IWireCapture)
alt Found globally
GlobalSP-->>Controller: WireCapture instance
else Not found
Note over Controller: wire_capture = None
end
end
Controller->>BRM: new(backend_processor, response_proc, wire_capture)
BRM-->>Controller: instance
Controller->>BRM: process request
BRM-->>Controller: response
Controller-->>User: response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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: 2
♻️ Duplicate comments (1)
src/core/app/controllers/chat_controller.py (1)
798-828: Extract duplicated wire_capture resolution logic.This wire_capture resolution logic is identical to the code in
anthropic_controller.py(lines 536-566). See the refactoring suggestion in that file's review comments.
🧹 Nitpick comments (1)
src/core/app/controllers/anthropic_controller.py (1)
545-545: Consider adding debug logging for wire_capture resolution failures.While broad exception handling is acceptable here given that wire_capture is optional, adding debug-level logging would improve observability and align with the pattern used for translation_service resolution in
chat_controller.py(lines 72-79).Apply this pattern to each exception handler:
except Exception: + logger.debug( + "Wire capture service resolution failed from local provider", + exc_info=True, + ) wire_capture = NoneAlso applies to: 553-553, 564-564
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/core/app/controllers/anthropic_controller.py(1 hunks)src/core/app/controllers/chat_controller.py(1 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/core/app/controllers/chat_controller.pysrc/core/app/controllers/anthropic_controller.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/core/app/controllers/chat_controller.pysrc/core/app/controllers/anthropic_controller.py
🧬 Code graph analysis (2)
src/core/app/controllers/chat_controller.py (3)
src/core/interfaces/wire_capture_interface.py (1)
IWireCapture(10-66)src/core/di/services.py (2)
get_service(1309-1319)get_service_provider(187-194)src/core/services/backend_request_manager_service.py (1)
BackendRequestManager(27-332)
src/core/app/controllers/anthropic_controller.py (3)
src/core/interfaces/wire_capture_interface.py (1)
IWireCapture(10-66)src/core/di/services.py (2)
get_service(1309-1319)get_service_provider(187-194)src/core/services/backend_request_manager_service.py (1)
BackendRequestManager(27-332)
🪛 Ruff (0.13.3)
src/core/app/controllers/chat_controller.py
807-807: Do not catch blind exception: Exception
(BLE001)
815-815: Do not catch blind exception: Exception
(BLE001)
826-826: Do not catch blind exception: Exception
(BLE001)
src/core/app/controllers/anthropic_controller.py
545-545: Do not catch blind exception: Exception
(BLE001)
553-553: Do not catch blind exception: Exception
(BLE001)
564-564: 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 (1)
src/core/app/controllers/chat_controller.py (1)
829-831: LGTM: BackendRequestManager now receives wire_capture.The wire_capture parameter is correctly passed to BackendRequestManager, enabling proxy-wide request capture when controllers self-heal missing backend request manager registrations.
| wire_capture = None | ||
| try: | ||
| from src.core.interfaces.wire_capture_interface import ( | ||
| IWireCapture as _IWireCapture, | ||
| ) | ||
|
|
||
| wire_capture = service_provider.get_service( | ||
| cast(type, _IWireCapture) | ||
| ) | ||
| except Exception: | ||
| wire_capture = None | ||
|
|
||
| if wire_capture is None: | ||
| try: | ||
| from src.core.di.services import get_service_provider | ||
|
|
||
| global_provider = get_service_provider() | ||
| except Exception: | ||
| global_provider = None | ||
| else: | ||
| if ( | ||
| global_provider is not None | ||
| and global_provider is not service_provider | ||
| ): | ||
| try: | ||
| wire_capture = global_provider.get_service( | ||
| cast(type, _IWireCapture) | ||
| ) | ||
| except Exception: | ||
| wire_capture = None | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract wire_capture resolution to a shared helper method.
The wire_capture resolution logic is duplicated verbatim in chat_controller.py (lines 798-828). Extract this pattern to a shared helper function to adhere to DRY principles.
Consider creating a helper in a shared location:
def resolve_wire_capture_service(
service_provider: IServiceProvider,
) -> Any | None:
"""Resolve IWireCapture from local or global service provider.
Returns None if wire_capture is not available, allowing graceful degradation.
"""
from typing import cast
wire_capture = None
try:
from src.core.interfaces.wire_capture_interface import (
IWireCapture as _IWireCapture,
)
wire_capture = service_provider.get_service(
cast(type, _IWireCapture)
)
except Exception:
wire_capture = None
if wire_capture is None:
try:
from src.core.di.services import get_service_provider
global_provider = get_service_provider()
except Exception:
global_provider = None
else:
if (
global_provider is not None
and global_provider is not service_provider
):
try:
wire_capture = global_provider.get_service(
cast(type, _IWireCapture)
)
except Exception:
wire_capture = None
return wire_captureThen use it in both controllers:
- wire_capture = None
- try:
- from src.core.interfaces.wire_capture_interface import (
- IWireCapture as _IWireCapture,
- )
-
- wire_capture = service_provider.get_service(
- cast(type, _IWireCapture)
- )
- except Exception:
- wire_capture = None
-
- if wire_capture is None:
- try:
- from src.core.di.services import get_service_provider
-
- global_provider = get_service_provider()
- except Exception:
- global_provider = None
- else:
- if (
- global_provider is not None
- and global_provider is not service_provider
- ):
- try:
- wire_capture = global_provider.get_service(
- cast(type, _IWireCapture)
- )
- except Exception:
- wire_capture = None
+ wire_capture = resolve_wire_capture_service(service_provider)🧰 Tools
🪛 Ruff (0.13.3)
545-545: Do not catch blind exception: Exception
(BLE001)
553-553: Do not catch blind exception: Exception
(BLE001)
564-564: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In src/core/app/controllers/anthropic_controller.py around lines 536-566 the
wire_capture resolution block is duplicated elsewhere (chat_controller.py lines
~798-828); extract that logic into a shared helper (e.g.,
src/core/app/utils/service_resolution.py) named
resolve_wire_capture_service(service_provider: IServiceProvider) -> Any | None
that performs the same try/except resolution (local service_provider then
fallback to global get_service_provider, using cast for _IWireCapture) and
returns None on failure, then replace the duplicated block here and in
chat_controller.py with a single call to that helper and import it; keep
typing/cast usage and preserve the exact failure semantics (silently return
None) and any necessary imports.
| backend_request_manager = BackendRequestManager( | ||
| backend_processor, response_proc | ||
| backend_processor, response_proc, wire_capture | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
Verify wire_capture integration preserves proxy-wide request capture.
The wire_capture parameter is now passed to BackendRequestManager. Ensure that when wire_capture is successfully resolved, the BackendService (which is called by BackendProcessor) properly applies the capture logic for both streaming and non-streaming requests.
Run the following script to verify wire_capture is properly utilized in the BackendService implementation:
🏁 Script executed:
#!/bin/bash
# Description: Check how wire_capture is used in BackendService to ensure it captures requests/responses
# Search for wire_capture usage in BackendService
ast-grep --pattern $'class BackendService {
$$$
}'
# Search for capture method calls in backend service
rg -nP --type=py 'capture_(outbound_request|inbound_response|inbound_stream)' -A 5 -B 2Length of output: 25011
Add streaming wire_capture.capture_inbound_stream in BackendService
In src/core/services/backend_service.py, non-streaming flows correctly invoke capture_outbound_request and capture_inbound_response, but the streaming branch does not call capture_inbound_stream—please add it to preserve proxy-wide capture.
🤖 Prompt for AI Agents
In src/core/services/backend_service.py (streaming response branch), the code
currently omits calling wire_capture.capture_inbound_stream — add a call to
wire_capture.capture_inbound_stream around the streaming iterator/response
processor before it is returned to the client so the proxy-wide wire capture
records inbound stream data; ensure you pass the same context/metadata used in
capture_outbound_request/capture_inbound_response and replace the raw stream
with the wrapped/returned iterator from capture_inbound_stream (and
await/iterate it as required) so streaming behavior and backpressure remain
unchanged.
|
✅ PR approved and safe to merge. Review Summary:
Testing Results:
Ready to merge. |
1 participant
Summary
Testing
python -m pytest -o addopts="" tests/integration/test_direct_controllers.py -k chat(fails: ModuleNotFoundError: No module named 'pytest_asyncio')https://chatgpt.com/codex/tasks/task_e_68ec24c75d088333bc0d3e5950ad0e4f
Summary by CodeRabbit