Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThe changes introduce session key resolution mechanisms across two middleware classes to derive stable session identifiers from multiple possible fields (session_id, stream_id, id, request_id, conversation_id, thread_id, message_id) and update state management to use these resolved keys instead of raw session_ids. New unit tests validate fallback behavior when identifiers are missing and stream isolation semantics. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Middleware
participant StateStorage
participant LogicProcessor
Caller->>Middleware: process(session_id=null, context={stream_id: "s1"}, metadata)
Middleware->>Middleware: _resolve_session_key(null, {stream_id: "s1"}, metadata)
Note over Middleware: Scan _SESSION_KEY_FIELDS<br/>session_id empty → check stream_id<br/>found "s1"
Middleware-->>Middleware: returns "s1"
Middleware->>StateStorage: access/update with key="s1"
StateStorage-->>Middleware: state updated
Middleware->>LogicProcessor: process with resolved key
LogicProcessor-->>Middleware: result
Middleware-->>Caller: response
rect rgb(200, 220, 255)
Note over Middleware,StateStorage: Fallback scenario: no identifiers found<br/>_resolve_session_key returns None
Middleware->>Middleware: _resolve_session_key(null, {}, {})
Note over Middleware: No fields found
Middleware-->>Middleware: returns None
Middleware->>Middleware: early return with debug log
Middleware-->>Caller: continue without state updates
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)
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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| if is_done: | ||
| if alias_value is not None: | ||
| self._accumulated_content.pop(alias_value, None) | ||
| self._anonymous_session_aliases.pop(alias_id, None) |
There was a problem hiding this comment.
Bug: Session Cleanup Fails for Metadata-Resolved Sessions
The _cleanup_session_state method fails to clean up accumulated content when sessions are resolved via metadata fields (like stream_id) rather than via stop_event. The cleanup logic checks _anonymous_session_aliases to find which session to clean, but this dictionary is only populated when stop_event is used as the fallback identifier in _resolve_session_key. When a session is resolved earlier via metadata, the mapping is never created, causing accumulated content to persist indefinitely even after the stream completes, resulting in a memory leak.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 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 "@codex address that feedback".
| def _cleanup_session_state( | ||
| self, | ||
| resolved_session_id: str, | ||
| ephemeral_key: bool, | ||
| stop_event: Any, | ||
| ) -> None: | ||
| if ephemeral_key: | ||
| self._accumulated_content.pop(resolved_session_id, None) | ||
| return | ||
| if stop_event is None: | ||
| return | ||
| alias_id = id(stop_event) | ||
| alias_value = self._anonymous_session_aliases.get(alias_id) | ||
| try: | ||
| is_done = bool(stop_event.is_set()) # type: ignore[attr-defined] | ||
| except AttributeError: | ||
| is_done = False | ||
| if is_done: | ||
| if alias_value is not None: | ||
| self._accumulated_content.pop(alias_value, None) | ||
| self._anonymous_session_aliases.pop(alias_id, None) |
There was a problem hiding this comment.
The new cleanup logic only removes _accumulated_content when an alias was registered for the stop_event, but when the session key comes from metadata (e.g. stream_id) no alias exists. After stop_event.is_set() the method returns without popping the resolved key, so each anonymous stream leaves its accumulated text in memory forever. On a busy server this dictionary will grow unbounded and can also contaminate later streams if an identifier is reused. Consider popping resolved_session_id whenever the stop event finishes, even when alias_value is None.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/core/services/response_middleware.py (1)
117-160: Code duplication: candidate_fields defined in multiple files.The
candidate_fieldstuple (lines 124-132) is duplicated insrc/core/services/edit_precision_response_middleware.py(lines 50-58, defined as_SESSION_KEY_FIELDS). Consider extracting this to a shared constant or utility module to maintain consistency.Example approach:
# In src/core/common/constants.py or similar SESSION_KEY_FIELDS = ( "session_id", "stream_id", "id", "request_id", "conversation_id", "thread_id", "message_id", )Then import and use in both middleware classes.
src/core/services/edit_precision_response_middleware.py (2)
50-58: Add type hint for class constant.Per coding guidelines, add a type hint for the class-level constant:
- _SESSION_KEY_FIELDS = ( + _SESSION_KEY_FIELDS: tuple[str, ...] = ( "session_id", "stream_id", "id",As per coding guidelines.
192-200: Redundant null check after early return.Line 192 checks
if session_key:, but this is always true at this point since lines 144-148 return early ifsession_key is None.Consider removing the redundant check:
- if session_key: - if active_disable_map.get(session_key): + if active_disable_map.get(session_key):Then outdent the entire block from lines 193-277 by one level.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/core/services/edit_precision_response_middleware.py(7 hunks)src/core/services/response_middleware.py(3 hunks)tests/unit/core/services/test_edit_precision_response_middleware.py(1 hunks)tests/unit/core/services/test_response_middleware.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:
tests/unit/core/services/test_edit_precision_response_middleware.pytests/unit/core/services/test_response_middleware.pysrc/core/services/edit_precision_response_middleware.pysrc/core/services/response_middleware.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/services/edit_precision_response_middleware.pysrc/core/services/response_middleware.py
🧬 Code graph analysis (4)
tests/unit/core/services/test_edit_precision_response_middleware.py (3)
src/core/services/application_state_service.py (1)
get_setting(154-159)src/core/services/streaming/middleware_application_processor.py (1)
process(38-78)src/core/ports/streaming.py (1)
StreamingContent(11-234)
tests/unit/core/services/test_response_middleware.py (3)
tests/unit/core/test_utilities.py (1)
check_for_loops(364-374)src/core/interfaces/response_processor_interface.py (1)
ProcessedResponse(8-30)src/core/services/response_middleware.py (3)
process(20-54)process(60-101)process(184-226)
src/core/services/edit_precision_response_middleware.py (4)
src/core/services/response_middleware.py (1)
_resolve_session_key(117-160)src/core/interfaces/application_state_interface.py (1)
set_setting(108-114)src/core/services/application_state_service.py (1)
set_setting(161-165)src/core/services/app_settings_service.py (1)
set_setting(101-118)
src/core/services/response_middleware.py (4)
src/core/services/edit_precision_response_middleware.py (1)
_resolve_session_key(92-124)tests/unit/core/services/test_response_middleware.py (1)
is_set(218-219)src/loop_detection/detector.py (1)
check_for_loops(215-260)src/core/common/exceptions.py (1)
LoopDetectionError(154-163)
⏰ 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). (4)
- GitHub Check: Cursor Bugbot
- GitHub Check: test (3.10)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Analyze (python)
🔇 Additional comments (10)
tests/unit/core/services/test_edit_precision_response_middleware.py (2)
67-84: LGTM! Good test coverage for stream_id fallback.This test correctly validates that when
session_idis missing, the middleware falls back tostream_idfrom metadata for session key resolution.
87-105: LGTM! Validates no-op behavior when identifiers are absent.This test correctly ensures that when no session identifiers can be resolved, the middleware skips state updates entirely, preventing orphaned state entries.
tests/unit/core/services/test_response_middleware.py (1)
205-261: LGTM! Comprehensive test for stream isolation.This test effectively validates that anonymous streaming sessions remain isolated when using
stop_eventandstream_idfor session resolution. The assertions correctly verify that content from one stream does not leak into another.src/core/services/response_middleware.py (5)
5-5: LGTM! Import supports anonymous session aliasing.
111-111: LGTM! Proper state tracking for anonymous sessions.
162-182: LGTM! Proper cleanup logic for session state.The cleanup correctly handles ephemeral keys, stop_event-based sessions, and gracefully handles missing
is_set()method.
184-226: LGTM! Process method correctly uses resolved session key.The refactored
processmethod correctly:
- Resolves session keys with fallback logic
- Uses resolved keys for content accumulation
- Ensures cleanup in finally block
- Includes resolved session ID in error details
228-234: LGTM! Reset correctly cleans up anonymous aliases.The updated
reset_sessionmethod properly removes both accumulated content and any anonymous session aliases, preventing stale aliases from lingering.src/core/services/edit_precision_response_middleware.py (2)
92-124: LGTM! Session key resolution correctly implemented.The method properly resolves session keys with appropriate fallback logic for this middleware's needs. The simpler signature (no stop_event, no ephemeral keys) is appropriate since this middleware doesn't need anonymous aliasing.
143-148: LGTM! Early return when session key cannot be resolved.This correctly prevents state updates when no session identifier can be determined, avoiding orphaned state entries.
1 participant
Summary
Testing
Codex Task
Note
Derives stable session/stream keys for loop detection and edit-precision middlewares, isolates anonymous streams, skips unscoped updates, and adds unit tests for these behaviors.
src/core/services/response_middleware.py(LoopDetectionMiddleware):_resolve_session_keyto derive stable keys fromsession_id,context/metadatafields, orstop_event(with anonymous alias viauuid4).session_idin loop errordetails._cleanup_session_stateto clear anonymous/ephemeral state when streaming ends.uuid4and track_anonymous_session_aliases.src/core/services/edit_precision_response_middleware.py:_resolve_session_keyusing common identifier fields; skip state updates if no key found.pending/activemaps and stream de-duplication to resolved session key.stream_idwhen nosession_id, and skipping updates with no identifiers.Written by Cursor Bugbot for commit 1c57d35. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
Release Notes
Refactor
Tests