RHIDP-12952: persist interrupted conversation#1971
Conversation
|
Warning Review limit reached
More reviews will be available in 49 minutes and 6 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe PR replaces the static interrupted-response message with interruption handling that rebuilds text from streamed partial tokens, repairs open Markdown, tracks SSE chunk ids, and emits a final token suffix on cancellation in both agent and endpoint streaming paths. ChangesStructured Interrupted Response from Partial Tokens
Sequence Diagram(s)sequenceDiagram
participant StreamingPath
participant build_interrupted_response
participant close_open_markdown
participant persist_interrupted_turn
participant Client
StreamingPath->>StreamingPath: collect partial_tokens and next_chunk_id
StreamingPath->>build_interrupted_response: partial_tokens on CancelledError
build_interrupted_response->>close_open_markdown: joined text
close_open_markdown-->>build_interrupted_response: repair suffix
build_interrupted_response-->>StreamingPath: full_text and suffix
StreamingPath->>persist_interrupted_turn: llm_response = full_text
StreamingPath-->>Client: final token SSE event with suffix
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app/endpoints/streaming_query.py`:
- Around line 637-640: The build_interrupted_response call at line 637 relies
solely on turn_summary.partial_tokens, which may be empty or incomplete if
cancellation occurs after response.output_text.done has populated
turn_summary.llm_response but before all deltas are processed. Modify the
build_interrupted_response call to use turn_summary.llm_response as a fallback
when partial_tokens is empty, ensuring that model output is not lost when
interrupted responses are reconstructed and persisted.
In `@src/utils/markdown_repair.py`:
- Around line 75-90: In the fence closing logic (the elif condition checking
`char == fence_char and len(matched_group) >= fence_len`), add validation to
ensure that any trailing content after the fence marker contains only whitespace
characters (spaces and tabs). Extract the remainder of the line after the
matched fence group and check that it either doesn't exist or contains only
whitespace using a string method like strip() or a regex check. Only allow the
fence to close if this whitespace validation passes, otherwise treat the line as
regular content inside the code block.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 093e2b47-86b6-41de-a11e-9ff652c597b9
📒 Files selected for processing (10)
src/app/endpoints/streaming_query.pysrc/constants.pysrc/models/common/turn_summary.pysrc/utils/agents/streaming.pysrc/utils/markdown_repair.pysrc/utils/stream_interrupts.pytests/unit/app/endpoints/test_streaming_query.pytests/unit/utils/agents/test_streaming.pytests/unit/utils/test_markdown_repair.pytests/unit/utils/test_stream_interrupts.py
📜 Review details
⏰ Context from checks skipped due to timeout. (12)
- GitHub Check: E2E: library mode / ci / group 3
- GitHub Check: E2E: server mode / ci / group 3
- GitHub Check: E2E: library mode / ci / group 2
- GitHub Check: E2E: server mode / ci / group 2
- GitHub Check: E2E: server mode / ci / group 1
- GitHub Check: E2E: library mode / ci / group 1
- GitHub Check: build-pr
- GitHub Check: E2E Tests for Lightspeed Evaluation job
- GitHub Check: integration_tests (3.13)
- GitHub Check: integration_tests (3.12)
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-0-6-on-pull-request
🧰 Additional context used
📓 Path-based instructions (5)
src/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.py: Use absolute imports for internal modules:from authentication import get_auth_dependency
Llama Stack imports: Usefrom llama_stack_client import AsyncLlamaStackClient
Checkconstants.pyfor shared constants before defining new ones
All modules must start with descriptive docstrings explaining purpose
Uselogger = get_logger(__name__)fromlog.pyfor module logging
All functions must have complete type annotations for parameters and return types, use modern syntax (str | int), and include descriptive docstrings
Use snake_case with descriptive, action-oriented names for functions (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead of modifying function parameters
Useasync deffor I/O operations and external API calls
Use standard log levels with clear purposes:debug()for diagnostic info,info()for program execution,warning()for unexpected events,error()for serious problems
All classes must have descriptive docstrings explaining purpose and use PascalCase with standard suffixes:Configuration,Error/Exception,Resolver,Interface
Abstract classes must use ABC with@abstractmethoddecorators
Follow Google Python docstring conventions with required sections: Parameters, Returns, Raises, and Attributes for classes
Files:
src/constants.pysrc/models/common/turn_summary.pysrc/utils/markdown_repair.pysrc/utils/stream_interrupts.pysrc/app/endpoints/streaming_query.pysrc/utils/agents/streaming.py
src/constants.py
📄 CodeRabbit inference engine (AGENTS.md)
Use
constants.pyfor shared constants with descriptive comments and type hints usingFinal[type]
Files:
src/constants.py
src/models/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Pydantic models must use
@model_validatorand@field_validatorfor validation and complete type annotations for all attributes, avoidingAnytype
Files:
src/models/common/turn_summary.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.py: Use pytest for all unit and integration tests; do not use unittest
Usepytest.mark.asynciomarker for async tests
Files:
tests/unit/utils/test_markdown_repair.pytests/unit/app/endpoints/test_streaming_query.pytests/unit/utils/agents/test_streaming.pytests/unit/utils/test_stream_interrupts.py
src/app/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/app/**/*.py: FastAPI dependencies: Import fromfastapimodule forAPIRouter,HTTPException,Request,status,Depends
Use FastAPIHTTPExceptionwith appropriate status codes for API endpoints and handleAPIConnectionErrorfrom Llama Stack
Files:
src/app/endpoints/streaming_query.py
🧠 Learnings (3)
📚 Learning: 2026-01-12T10:58:40.230Z
Learnt from: blublinsky
Repo: lightspeed-core/lightspeed-stack PR: 972
File: src/models/config.py:459-513
Timestamp: 2026-01-12T10:58:40.230Z
Learning: In lightspeed-core/lightspeed-stack, for Python files under src/models, when a user claims a fix is done but the issue persists, verify the current code state before accepting the fix. Steps: review the diff, fetch the latest changes, run relevant tests, reproduce the issue, search the codebase for lingering references to the original problem, confirm the fix is applied and not undone by subsequent commits, and validate with local checks to ensure the issue is resolved.
Applied to files:
src/models/common/turn_summary.py
📚 Learning: 2026-02-25T07:46:33.545Z
Learnt from: asimurka
Repo: lightspeed-core/lightspeed-stack PR: 1211
File: src/models/responses.py:8-16
Timestamp: 2026-02-25T07:46:33.545Z
Learning: In the Python codebase, requests.py should use OpenAIResponseInputTool as Tool while responses.py uses OpenAIResponseTool as Tool. This difference is intentional due to differing schemas for input vs output tools in llama-stack-api. Apply this distinction consistently to other models under src/models (e.g., ensure request-related tools use the InputTool variant and response-related tools use the ResponseTool variant). If adding new tools, choose the corresponding InputTool or Tool class based on whether the tool represents input or output, and document the rationale in code comments.
Applied to files:
src/models/common/turn_summary.py
📚 Learning: 2026-04-06T20:18:07.852Z
Learnt from: major
Repo: lightspeed-core/lightspeed-stack PR: 1463
File: src/app/endpoints/rlsapi_v1.py:266-271
Timestamp: 2026-04-06T20:18:07.852Z
Learning: In the lightspeed-stack codebase, within `src/app/endpoints/` inference/MCP endpoints, treat `tools: Optional[list[Any]]` in MCP tool definitions as an intentional, consistent typing pattern (used across `query`, `responses`, `streaming_query`, `rlsapi_v1`). Do not raise or suggest this as a typing issue during code review; changing it in isolation could break endpoint typing consistency across the codebase.
Applied to files:
src/app/endpoints/streaming_query.py
🔇 Additional comments (7)
src/models/common/turn_summary.py (1)
117-126: LGTM!src/constants.py (1)
15-15: LGTM!src/utils/stream_interrupts.py (1)
23-23: LGTM!Also applies to: 219-239, 277-277, 286-286, 368-369
tests/unit/utils/test_stream_interrupts.py (1)
8-21: LGTM!Also applies to: 49-49, 71-72, 100-100, 169-193
src/utils/agents/streaming.py (1)
28-28: LGTM!Also applies to: 68-68, 201-217, 358-364, 415-415
tests/unit/utils/agents/test_streaming.py (1)
67-68: LGTM!Also applies to: 720-722, 813-813, 966-1106
tests/unit/app/endpoints/test_streaming_query.py (1)
54-54: LGTM!Also applies to: 74-75, 1385-1385, 1394-1400
|
/cc @tisnik |
Jdubrick
force-pushed
the
interrupt-message-persistence
branch
from
969f5d6 to
13718e0
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/endpoints/streaming_query.py (1)
774-783: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAdvance
next_chunk_idbefore yielding each SSE chunk.Because
response_generatorsuspends atyield, cancellation immediately aftergenerate_responsere-yields a token can skip Lines 782-783 or 808-809. The cancellation handler then emits the interruption suffix with the staleturn_summary.next_chunk_id, duplicating the ID of the just-emitted token. Move the ID advancement before eachyieldwhile preserving the event’s current ID.Proposed fix
if event_type == "response.content_part.added": + event_id = chunk_id + chunk_id += 1 + turn_summary.next_chunk_id = chunk_id yield stream_event( { - "id": chunk_id, + "id": event_id, "token": "", }, LLM_TOKEN_EVENT, media_type, ) - chunk_id += 1 - turn_summary.next_chunk_id = chunk_id # Text streaming - emit token delta elif event_type == "response.output_text.delta": delta_chunk = cast(TextDeltaChunk, chunk) text_parts.append(delta_chunk.delta) turn_summary.partial_tokens.append(delta_chunk.delta) + event_id = chunk_id + chunk_id += 1 + turn_summary.next_chunk_id = chunk_id yield stream_event( { - "id": chunk_id, + "id": event_id, "token": delta_chunk.delta, }, LLM_TOKEN_EVENT, media_type, ) - chunk_id += 1 - turn_summary.next_chunk_id = chunk_id @@ elif event_type == "response.completed": latest_response_object = cast( OpenAIResponseObject, getattr(chunk, "response"), # noqa: B009 ) turn_summary.llm_response = turn_summary.llm_response or "".join(text_parts) # Capture structured output items for compacted-mode turn storage # (LCORE-1572), so the persisted turn keeps non-text output items # rather than being flattened to the response text. turn_summary.output_items = list(latest_response_object.output or []) + event_id = chunk_id + chunk_id += 1 + turn_summary.next_chunk_id = chunk_id yield stream_event( { - "id": chunk_id, + "id": event_id, "token": turn_summary.llm_response, }, LLM_TURN_COMPLETE_EVENT, media_type, ) - chunk_id += 1 - turn_summary.next_chunk_id = chunk_idAlso applies to: 800-809, 889-898
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/endpoints/streaming_query.py` around lines 774 - 783, The ID advancement in the stream_event yielding blocks happens after the yield statement, which can cause the increment to be skipped during cancellation. Move the `chunk_id += 1` and `turn_summary.next_chunk_id = chunk_id` statements to occur before each `yield stream_event()` call while preserving the event's ID field to use the current chunk_id value at the time of emission. This pattern appears in three locations: the stream_event yielding block around lines 774-783, the similar block around lines 800-809, and the block around lines 889-898. Save the current chunk_id value before incrementing, use the saved value in the event's ID field, and increment the counters before yielding to ensure the ID advancement cannot be skipped by cancellation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/utils/markdown_repair.py`:
- Around line 30-50: The function `_process_html_tags` currently mutates its
`html_stack` parameter in-place using `.pop()` and `.append()` operations.
Refactor this function to return the updated stack instead of modifying the
parameter directly. Change the function signature to return a list (the updated
html_stack) and update all callers to capture the returned value rather than
relying on side effects. This applies to both the main function definition and
any other similar patterns mentioned at lines 77-79.
- Around line 31-36: The docstring for the function starting at line 31 uses
"Parameters:" section header, but Google Python docstring conventions require
"Args:" instead. Update the docstring header from "Parameters:" to "Args:" in
the function at line 31-36, and apply the same fix to the other docstring
mentioned at lines 53-64. Additionally, review both docstrings to ensure they
include all required Google format sections (Args, Returns, Raises) where
applicable based on what each function actually does.
In `@src/utils/stream_interrupts.py`:
- Around line 219-232: The docstring for the build_interrupted_response function
uses "Parameters:" and "Returns:" sections, but the repository standard requires
Google-style docstring format with "Args", "Returns", and "Raises" sections.
Update the docstring by renaming the "Parameters:" section to "Args:" to match
the required convention. Additionally, add a "Raises:" section if the function
can raise any exceptions during execution, following the repository's Google
Python docstring conventions.
---
Outside diff comments:
In `@src/app/endpoints/streaming_query.py`:
- Around line 774-783: The ID advancement in the stream_event yielding blocks
happens after the yield statement, which can cause the increment to be skipped
during cancellation. Move the `chunk_id += 1` and `turn_summary.next_chunk_id =
chunk_id` statements to occur before each `yield stream_event()` call while
preserving the event's ID field to use the current chunk_id value at the time of
emission. This pattern appears in three locations: the stream_event yielding
block around lines 774-783, the similar block around lines 800-809, and the
block around lines 889-898. Save the current chunk_id value before incrementing,
use the saved value in the event's ID field, and increment the counters before
yielding to ensure the ID advancement cannot be skipped by cancellation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 187ae3b8-ac25-444d-befd-f5f266b56a23
📒 Files selected for processing (10)
src/app/endpoints/streaming_query.pysrc/constants.pysrc/models/common/turn_summary.pysrc/utils/agents/streaming.pysrc/utils/markdown_repair.pysrc/utils/stream_interrupts.pytests/unit/app/endpoints/test_streaming_query.pytests/unit/utils/agents/test_streaming.pytests/unit/utils/test_markdown_repair.pytests/unit/utils/test_stream_interrupts.py
📜 Review details
⏰ Context from checks skipped due to timeout. (14)
- GitHub Check: unit_tests (3.13)
- GitHub Check: unit_tests (3.12)
- GitHub Check: build-pr
- GitHub Check: E2E Tests for Lightspeed Evaluation job
- GitHub Check: integration_tests (3.13)
- GitHub Check: integration_tests (3.12)
- GitHub Check: E2E: library mode / ci / group 2
- GitHub Check: E2E: server mode / ci / group 3
- GitHub Check: E2E: server mode / ci / group 2
- GitHub Check: E2E: server mode / ci / group 1
- GitHub Check: E2E: library mode / ci / group 1
- GitHub Check: E2E: library mode / ci / group 3
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-0-6-on-pull-request
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
🧰 Additional context used
📓 Path-based instructions (5)
src/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.py: Use absolute imports for internal modules:from authentication import get_auth_dependency
Llama Stack imports: Usefrom llama_stack_client import AsyncLlamaStackClient
Checkconstants.pyfor shared constants before defining new ones
All modules must start with descriptive docstrings explaining purpose
Uselogger = get_logger(__name__)fromlog.pyfor module logging
All functions must have complete type annotations for parameters and return types, use modern syntax (str | int), and include descriptive docstrings
Use snake_case with descriptive, action-oriented names for functions (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead of modifying function parameters
Useasync deffor I/O operations and external API calls
Use standard log levels with clear purposes:debug()for diagnostic info,info()for program execution,warning()for unexpected events,error()for serious problems
All classes must have descriptive docstrings explaining purpose and use PascalCase with standard suffixes:Configuration,Error/Exception,Resolver,Interface
Abstract classes must use ABC with@abstractmethoddecorators
Follow Google Python docstring conventions with required sections: Parameters, Returns, Raises, and Attributes for classes
Files:
src/utils/markdown_repair.pysrc/constants.pysrc/models/common/turn_summary.pysrc/utils/agents/streaming.pysrc/utils/stream_interrupts.pysrc/app/endpoints/streaming_query.py
src/constants.py
📄 CodeRabbit inference engine (AGENTS.md)
Use
constants.pyfor shared constants with descriptive comments and type hints usingFinal[type]
Files:
src/constants.py
src/models/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Pydantic models must use
@model_validatorand@field_validatorfor validation and complete type annotations for all attributes, avoidingAnytype
Files:
src/models/common/turn_summary.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.py: Use pytest for all unit and integration tests; do not use unittest
Usepytest.mark.asynciomarker for async tests
Files:
tests/unit/app/endpoints/test_streaming_query.pytests/unit/utils/test_markdown_repair.pytests/unit/utils/agents/test_streaming.pytests/unit/utils/test_stream_interrupts.py
src/app/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/app/**/*.py: FastAPI dependencies: Import fromfastapimodule forAPIRouter,HTTPException,Request,status,Depends
Use FastAPIHTTPExceptionwith appropriate status codes for API endpoints and handleAPIConnectionErrorfrom Llama Stack
Files:
src/app/endpoints/streaming_query.py
🧠 Learnings (3)
📚 Learning: 2026-01-12T10:58:40.230Z
Learnt from: blublinsky
Repo: lightspeed-core/lightspeed-stack PR: 972
File: src/models/config.py:459-513
Timestamp: 2026-01-12T10:58:40.230Z
Learning: In lightspeed-core/lightspeed-stack, for Python files under src/models, when a user claims a fix is done but the issue persists, verify the current code state before accepting the fix. Steps: review the diff, fetch the latest changes, run relevant tests, reproduce the issue, search the codebase for lingering references to the original problem, confirm the fix is applied and not undone by subsequent commits, and validate with local checks to ensure the issue is resolved.
Applied to files:
src/models/common/turn_summary.py
📚 Learning: 2026-02-25T07:46:33.545Z
Learnt from: asimurka
Repo: lightspeed-core/lightspeed-stack PR: 1211
File: src/models/responses.py:8-16
Timestamp: 2026-02-25T07:46:33.545Z
Learning: In the Python codebase, requests.py should use OpenAIResponseInputTool as Tool while responses.py uses OpenAIResponseTool as Tool. This difference is intentional due to differing schemas for input vs output tools in llama-stack-api. Apply this distinction consistently to other models under src/models (e.g., ensure request-related tools use the InputTool variant and response-related tools use the ResponseTool variant). If adding new tools, choose the corresponding InputTool or Tool class based on whether the tool represents input or output, and document the rationale in code comments.
Applied to files:
src/models/common/turn_summary.py
📚 Learning: 2026-04-06T20:18:07.852Z
Learnt from: major
Repo: lightspeed-core/lightspeed-stack PR: 1463
File: src/app/endpoints/rlsapi_v1.py:266-271
Timestamp: 2026-04-06T20:18:07.852Z
Learning: In the lightspeed-stack codebase, within `src/app/endpoints/` inference/MCP endpoints, treat `tools: Optional[list[Any]]` in MCP tool definitions as an intentional, consistent typing pattern (used across `query`, `responses`, `streaming_query`, `rlsapi_v1`). Do not raise or suggest this as a typing issue during code review; changing it in isolation could break endpoint typing consistency across the codebase.
Applied to files:
src/app/endpoints/streaming_query.py
🔇 Additional comments (13)
src/models/common/turn_summary.py (1)
117-126: LGTM!tests/unit/utils/test_markdown_repair.py (1)
1-175: LGTM!src/constants.py (1)
15-15: LGTM!src/utils/stream_interrupts.py (1)
23-23: LGTM!Also applies to: 277-287, 368-369
tests/unit/utils/test_stream_interrupts.py (1)
8-21: LGTM!Also applies to: 49-49, 71-73, 100-100, 169-193
src/utils/agents/streaming.py (3)
28-28: LGTM!Also applies to: 68-68
201-217: LGTM!
358-364: LGTM!Also applies to: 415-415
tests/unit/utils/agents/test_streaming.py (2)
67-68: LGTM!Also applies to: 720-722, 813-813
966-1106: LGTM!src/app/endpoints/streaming_query.py (1)
124-124: LGTM!Also applies to: 637-652
tests/unit/app/endpoints/test_streaming_query.py (2)
54-54: LGTM!Also applies to: 74-75
1385-1400: LGTM!
Signed-off-by: Jordan Dubrick <jdubrick@redhat.com>
Signed-off-by: Jordan Dubrick <jdubrick@redhat.com>
Signed-off-by: Jordan Dubrick <jdubrick@redhat.com>
Signed-off-by: Jordan Dubrick <jdubrick@redhat.com>
Signed-off-by: Jordan Dubrick <jdubrick@redhat.com>
Signed-off-by: Jordan Dubrick <jdubrick@redhat.com>
Jdubrick
force-pushed
the
interrupt-message-persistence
branch
from
13718e0 to
a23ccbb
Compare
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 (2)
tests/unit/app/endpoints/test_streaming_query.py (1)
1385-1400: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCover the real delta path and suffix payload.
This mock generator never exercises
response.output_text.delta, so the newpartial_tokensandnext_chunk_idendpoint logic can regress while this test still passes. Add a cancellation case throughresponse_generatorand assert the final token payload/id before"interrupted".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/app/endpoints/test_streaming_query.py` around lines 1385 - 1400, The streaming query test is only covering the interrupted path and is missing the real delta payload behavior for response.output_text.delta. Update the test around response_generator to include a cancellation case that exercises the partial_tokens/next_chunk_id logic, then assert the emitted final token payload and chunk id before the "interrupted" event so regressions in the endpoint flow are caught.src/app/endpoints/streaming_query.py (1)
774-783: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAdvance
next_chunk_idbefore yielding SSE events.These updates run only after the inner generator resumes. If cancellation hits while
generate_responseis paused yielding that SSE event, the interrupt suffix uses the stalenext_chunk_idand can duplicate the last token id.Proposed fix
if event_type == "response.content_part.added": + event_id = chunk_id + chunk_id += 1 + turn_summary.next_chunk_id = chunk_id yield stream_event( { - "id": chunk_id, + "id": event_id, "token": "", }, LLM_TOKEN_EVENT, media_type, ) - chunk_id += 1 - turn_summary.next_chunk_id = chunk_id @@ elif event_type == "response.output_text.delta": delta_chunk = cast(TextDeltaChunk, chunk) text_parts.append(delta_chunk.delta) turn_summary.partial_tokens.append(delta_chunk.delta) + event_id = chunk_id + chunk_id += 1 + turn_summary.next_chunk_id = chunk_id yield stream_event( { - "id": chunk_id, + "id": event_id, "token": delta_chunk.delta, }, LLM_TOKEN_EVENT, media_type, ) - chunk_id += 1 - turn_summary.next_chunk_id = chunk_id @@ turn_summary.output_items = list(latest_response_object.output or []) + event_id = chunk_id + chunk_id += 1 + turn_summary.next_chunk_id = chunk_id yield stream_event( { - "id": chunk_id, + "id": event_id, "token": turn_summary.llm_response, }, LLM_TURN_COMPLETE_EVENT, media_type, ) - chunk_id += 1 - turn_summary.next_chunk_id = chunk_idAlso applies to: 800-809, 889-898
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/endpoints/streaming_query.py` around lines 774 - 783, Advance the turn_summary.next_chunk_id update in generate_response before each yield_stream_event call, using the chunk_id assigned for the SSE token event; this ensures the state is already updated if cancellation interrupts while the generator is paused. Apply the same ordering fix in the other matching send paths as well, so the interrupt suffix logic reads the latest next_chunk_id instead of reusing the last emitted token id.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/unit/utils/agents/test_streaming.py`:
- Around line 1054-1057: The chunk ID assertions in the token event test only
verify ordering and non-negativity, so duplicates can still slip through; update
the checks around the token_events-derived chunk_ids to assert the exact
contiguous sequence starting at 0 with no gaps or repeats. Use the existing
chunk_ids logic in test_streaming.py to compare against the expected range based
on the list length, so the test catches skipped or duplicated SSE chunk ids.
---
Outside diff comments:
In `@src/app/endpoints/streaming_query.py`:
- Around line 774-783: Advance the turn_summary.next_chunk_id update in
generate_response before each yield_stream_event call, using the chunk_id
assigned for the SSE token event; this ensures the state is already updated if
cancellation interrupts while the generator is paused. Apply the same ordering
fix in the other matching send paths as well, so the interrupt suffix logic
reads the latest next_chunk_id instead of reusing the last emitted token id.
In `@tests/unit/app/endpoints/test_streaming_query.py`:
- Around line 1385-1400: The streaming query test is only covering the
interrupted path and is missing the real delta payload behavior for
response.output_text.delta. Update the test around response_generator to include
a cancellation case that exercises the partial_tokens/next_chunk_id logic, then
assert the emitted final token payload and chunk id before the "interrupted"
event so regressions in the endpoint flow are caught.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c294eef1-573b-43fe-861a-22502d9a87a5
📒 Files selected for processing (10)
src/app/endpoints/streaming_query.pysrc/constants.pysrc/models/common/turn_summary.pysrc/utils/agents/streaming.pysrc/utils/markdown_repair.pysrc/utils/stream_interrupts.pytests/unit/app/endpoints/test_streaming_query.pytests/unit/utils/agents/test_streaming.pytests/unit/utils/test_markdown_repair.pytests/unit/utils/test_stream_interrupts.py
📜 Review details
⏰ Context from checks skipped due to timeout. (13)
- GitHub Check: unit_tests (3.12)
- GitHub Check: build-pr
- GitHub Check: integration_tests (3.13)
- GitHub Check: integration_tests (3.12)
- GitHub Check: E2E: server mode / ci / group 3
- GitHub Check: E2E: library mode / ci / group 2
- GitHub Check: E2E: server mode / ci / group 2
- GitHub Check: E2E: library mode / ci / group 1
- GitHub Check: E2E: server mode / ci / group 1
- GitHub Check: E2E: library mode / ci / group 3
- GitHub Check: E2E Tests for Lightspeed Evaluation job
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-0-6-on-pull-request
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
🧰 Additional context used
📓 Path-based instructions (5)
src/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.py: Use absolute imports for internal modules:from authentication import get_auth_dependency
Llama Stack imports: Usefrom llama_stack_client import AsyncLlamaStackClient
Checkconstants.pyfor shared constants before defining new ones
All modules must start with descriptive docstrings explaining purpose
Uselogger = get_logger(__name__)fromlog.pyfor module logging
All functions must have complete type annotations for parameters and return types, use modern syntax (str | int), and include descriptive docstrings
Use snake_case with descriptive, action-oriented names for functions (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead of modifying function parameters
Useasync deffor I/O operations and external API calls
Use standard log levels with clear purposes:debug()for diagnostic info,info()for program execution,warning()for unexpected events,error()for serious problems
All classes must have descriptive docstrings explaining purpose and use PascalCase with standard suffixes:Configuration,Error/Exception,Resolver,Interface
Abstract classes must use ABC with@abstractmethoddecorators
Follow Google Python docstring conventions with required sections: Parameters, Returns, Raises, and Attributes for classes
Files:
src/models/common/turn_summary.pysrc/utils/markdown_repair.pysrc/constants.pysrc/utils/agents/streaming.pysrc/utils/stream_interrupts.pysrc/app/endpoints/streaming_query.py
src/models/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Pydantic models must use
@model_validatorand@field_validatorfor validation and complete type annotations for all attributes, avoidingAnytype
Files:
src/models/common/turn_summary.py
src/constants.py
📄 CodeRabbit inference engine (AGENTS.md)
Use
constants.pyfor shared constants with descriptive comments and type hints usingFinal[type]
Files:
src/constants.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.py: Use pytest for all unit and integration tests; do not use unittest
Usepytest.mark.asynciomarker for async tests
Files:
tests/unit/app/endpoints/test_streaming_query.pytests/unit/utils/agents/test_streaming.pytests/unit/utils/test_stream_interrupts.pytests/unit/utils/test_markdown_repair.py
src/app/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/app/**/*.py: FastAPI dependencies: Import fromfastapimodule forAPIRouter,HTTPException,Request,status,Depends
Use FastAPIHTTPExceptionwith appropriate status codes for API endpoints and handleAPIConnectionErrorfrom Llama Stack
Files:
src/app/endpoints/streaming_query.py
🧠 Learnings (3)
📚 Learning: 2026-01-12T10:58:40.230Z
Learnt from: blublinsky
Repo: lightspeed-core/lightspeed-stack PR: 972
File: src/models/config.py:459-513
Timestamp: 2026-01-12T10:58:40.230Z
Learning: In lightspeed-core/lightspeed-stack, for Python files under src/models, when a user claims a fix is done but the issue persists, verify the current code state before accepting the fix. Steps: review the diff, fetch the latest changes, run relevant tests, reproduce the issue, search the codebase for lingering references to the original problem, confirm the fix is applied and not undone by subsequent commits, and validate with local checks to ensure the issue is resolved.
Applied to files:
src/models/common/turn_summary.py
📚 Learning: 2026-02-25T07:46:33.545Z
Learnt from: asimurka
Repo: lightspeed-core/lightspeed-stack PR: 1211
File: src/models/responses.py:8-16
Timestamp: 2026-02-25T07:46:33.545Z
Learning: In the Python codebase, requests.py should use OpenAIResponseInputTool as Tool while responses.py uses OpenAIResponseTool as Tool. This difference is intentional due to differing schemas for input vs output tools in llama-stack-api. Apply this distinction consistently to other models under src/models (e.g., ensure request-related tools use the InputTool variant and response-related tools use the ResponseTool variant). If adding new tools, choose the corresponding InputTool or Tool class based on whether the tool represents input or output, and document the rationale in code comments.
Applied to files:
src/models/common/turn_summary.py
📚 Learning: 2026-04-06T20:18:07.852Z
Learnt from: major
Repo: lightspeed-core/lightspeed-stack PR: 1463
File: src/app/endpoints/rlsapi_v1.py:266-271
Timestamp: 2026-04-06T20:18:07.852Z
Learning: In the lightspeed-stack codebase, within `src/app/endpoints/` inference/MCP endpoints, treat `tools: Optional[list[Any]]` in MCP tool definitions as an intentional, consistent typing pattern (used across `query`, `responses`, `streaming_query`, `rlsapi_v1`). Do not raise or suggest this as a typing issue during code review; changing it in isolation could break endpoint typing consistency across the codebase.
Applied to files:
src/app/endpoints/streaming_query.py
🔇 Additional comments (13)
src/utils/markdown_repair.py (3)
30-50: 📐 Maintainability & Code Quality | 💤 Low valueIn-place mutation of
html_stackparameter.
_process_html_tagsstill mutates itshtml_stackargument via.pop()/.append(). As per coding guidelines, "Avoid in-place parameter modification anti-patterns; return new data structures instead of modifying function parameters."Source: Coding guidelines
33-36: 📐 Maintainability & Code Quality | 💤 Low valueDocstring sections should use Google
Args/Returns.Both
_process_html_tagsandclose_open_markdownuse aParameters:heading. As per coding guidelines, "Follow Google Python docstring conventions with required sections: Parameters, Returns, Raises, and Attributes for classes."Also applies to: 59-64
Source: Coding guidelines
89-96: LGTM!src/utils/stream_interrupts.py (2)
225-231: 📐 Maintainability & Code Quality | 💤 Low valueDocstring should use Google
Args/Returnsheadings.
build_interrupted_responseuses aParameters:heading. As per coding guidelines, "Follow Google Python docstring conventions with required sections: Parameters, Returns, Raises, and Attributes for classes."Source: Coding guidelines
233-238: LGTM!Also applies to: 277-277, 286-286, 368-369
src/models/common/turn_summary.py (1)
117-126: LGTM!tests/unit/utils/test_markdown_repair.py (1)
1-175: LGTM!src/constants.py (1)
28-28: LGTM!tests/unit/utils/test_stream_interrupts.py (1)
8-20: LGTM!Also applies to: 49-49, 71-72, 100-100, 171-193
src/utils/agents/streaming.py (1)
28-68: LGTM!Also applies to: 201-217, 358-364, 415-415
tests/unit/utils/agents/test_streaming.py (1)
67-68: LGTM!Also applies to: 720-722, 813-813, 966-1053, 1059-1106
src/app/endpoints/streaming_query.py (1)
124-124: LGTM!Also applies to: 637-652
tests/unit/app/endpoints/test_streaming_query.py (1)
54-54: LGTM!Also applies to: 74-75, 1394-1400
…ace with yield Signed-off-by: Jordan Dubrick <jdubrick@redhat.com>
Signed-off-by: Jordan Dubrick <jdubrick@redhat.com>
tisnik
left a comment
tisnik
left a comment
There was a problem hiding this comment.
LGTM (have some comments to HTML tags processing/closing, but IMHO it's ok for now)
| "tbody", | ||
| "details", | ||
| "summary", | ||
| "pre", |
There was a problem hiding this comment.
there's a lot more, but if it's ok for your use case...
| if html_stack and html_stack[-1] == tag_name: | ||
| html_stack.pop() | ||
| else: | ||
| html_stack.append(tag_name) |
There was a problem hiding this comment.
I guess it can be break by using comments etc. but again ... if it's ok for your case...
Signed-off-by: Jordan Dubrick <jdubrick@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/utils/markdown_repair.py`:
- Around line 42-72: The _strip_comments helper in markdown_repair is unused and
should be removed. Delete the _strip_comments function entirely, and keep
close_open_markdown and _strip_comments_with_zone_priority as the only
comment-stripping path so the module no longer carries dead code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8d3f6593-9aec-4fc9-ad4a-3c66d14eb875
📒 Files selected for processing (4)
src/app/endpoints/streaming_query.pysrc/utils/markdown_repair.pytests/unit/utils/agents/test_streaming.pytests/unit/utils/test_markdown_repair.py
📜 Review details
⏰ Context from checks skipped due to timeout. (13)
- GitHub Check: Pylinter
- GitHub Check: integration_tests (3.12)
- GitHub Check: integration_tests (3.13)
- GitHub Check: build-pr
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-0-6-on-pull-request
- GitHub Check: E2E: server mode / ci / group 1
- GitHub Check: E2E Tests for Lightspeed Evaluation job
- GitHub Check: E2E: server mode / ci / group 3
- GitHub Check: E2E: server mode / ci / group 2
- GitHub Check: E2E: library mode / ci / group 2
- GitHub Check: E2E: library mode / ci / group 1
- GitHub Check: E2E: library mode / ci / group 3
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.py: Use absolute imports for internal modules:from authentication import get_auth_dependency
Llama Stack imports: Usefrom llama_stack_client import AsyncLlamaStackClient
Checkconstants.pyfor shared constants before defining new ones
All modules must start with descriptive docstrings explaining purpose
Uselogger = get_logger(__name__)fromlog.pyfor module logging
All functions must have complete type annotations for parameters and return types, use modern syntax (str | int), and include descriptive docstrings
Use snake_case with descriptive, action-oriented names for functions (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead of modifying function parameters
Useasync deffor I/O operations and external API calls
Use standard log levels with clear purposes:debug()for diagnostic info,info()for program execution,warning()for unexpected events,error()for serious problems
All classes must have descriptive docstrings explaining purpose and use PascalCase with standard suffixes:Configuration,Error/Exception,Resolver,Interface
Abstract classes must use ABC with@abstractmethoddecorators
Follow Google Python docstring conventions with required sections: Parameters, Returns, Raises, and Attributes for classes
Files:
src/app/endpoints/streaming_query.pysrc/utils/markdown_repair.py
src/app/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/app/**/*.py: FastAPI dependencies: Import fromfastapimodule forAPIRouter,HTTPException,Request,status,Depends
Use FastAPIHTTPExceptionwith appropriate status codes for API endpoints and handleAPIConnectionErrorfrom Llama Stack
Files:
src/app/endpoints/streaming_query.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.py: Use pytest for all unit and integration tests; do not use unittest
Usepytest.mark.asynciomarker for async tests
Files:
tests/unit/utils/agents/test_streaming.pytests/unit/utils/test_markdown_repair.py
🧠 Learnings (2)
📚 Learning: 2026-04-06T20:18:07.852Z
Learnt from: major
Repo: lightspeed-core/lightspeed-stack PR: 1463
File: src/app/endpoints/rlsapi_v1.py:266-271
Timestamp: 2026-04-06T20:18:07.852Z
Learning: In the lightspeed-stack codebase, within `src/app/endpoints/` inference/MCP endpoints, treat `tools: Optional[list[Any]]` in MCP tool definitions as an intentional, consistent typing pattern (used across `query`, `responses`, `streaming_query`, `rlsapi_v1`). Do not raise or suggest this as a typing issue during code review; changing it in isolation could break endpoint typing consistency across the codebase.
Applied to files:
src/app/endpoints/streaming_query.py
📚 Learning: 2026-06-24T13:45:37.249Z
Learnt from: Jdubrick
Repo: lightspeed-core/lightspeed-stack PR: 1971
File: src/utils/markdown_repair.py:31-36
Timestamp: 2026-06-24T13:45:37.249Z
Learning: In the lightspeed-stack repository, docstrings must use the section header name "Parameters:" (not "Args:") for function arguments, even if the project references Google Python docstring conventions. Ensure docstrings follow the project’s established "Parameters:" header format for any documented function parameters.
Applied to files:
src/app/endpoints/streaming_query.pysrc/utils/markdown_repair.pytests/unit/utils/agents/test_streaming.pytests/unit/utils/test_markdown_repair.py
🪛 ast-grep (0.44.0)
src/utils/markdown_repair.py
[warning] 58-58: XPath query is request-/variable-derived; use parameterized XPath to prevent injection.
Context: line.find(_COMMENT_CLOSE, i)
Note: [CWE-643] Improper Neutralization of Data within XPath Expressions ('XPath Injection').
(xpath-injection-python)
[warning] 64-64: XPath query is request-/variable-derived; use parameterized XPath to prevent injection.
Context: line.find(_COMMENT_OPEN, i)
Note: [CWE-643] Improper Neutralization of Data within XPath Expressions ('XPath Injection').
(xpath-injection-python)
[warning] 189-189: XPath query is request-/variable-derived; use parameterized XPath to prevent injection.
Context: remaining.find(_COMMENT_CLOSE)
Note: [CWE-643] Improper Neutralization of Data within XPath Expressions ('XPath Injection').
(xpath-injection-python)
[warning] 200-200: XPath query is request-/variable-derived; use parameterized XPath to prevent injection.
Context: remaining.find(_COMMENT_OPEN)
Note: [CWE-643] Improper Neutralization of Data within XPath Expressions ('XPath Injection').
(xpath-injection-python)
🔇 Additional comments (8)
src/utils/markdown_repair.py (2)
217-247: LGTM!
250-313: LGTM!tests/unit/utils/test_markdown_repair.py (1)
133-472: LGTM!tests/unit/utils/agents/test_streaming.py (2)
1055-1061: Chunk-id assertions are sufficient for contiguity.
sorted+ non-negative + no-duplicates +chunk_ids[-1] == num_chunks - 1together force the set to be exactly{0..num_chunks-1}, so this correctly catches skipped or duplicated SSE ids.
720-723: LGTM!Also applies to: 813-814, 1063-1108
src/app/endpoints/streaming_query.py (3)
773-784: LGTM!
800-811: Increment-before-yield correctly avoids the duplicate chunk-id race.Appending to
partial_tokensand advancingnext_chunk_idprior to theyieldensures that if cancellation lands at the yield point, the interrupt handler emits the suffix with a fresh, non-colliding id and the delta is retained.
891-901: LGTM!
Signed-off-by: Jordan Dubrick <jdubrick@redhat.com>
2 participants
Description
Type of change
Tools used to create PR
Identify any AI code assistants used in this PR (for transparency and review context)
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Updates
Tests