Add conversations support for Responses API: modifications from reviews #824
Conversation
WalkthroughThe PR adds conversation ID normalization functionality to the utilities layer and propagates its usage across multiple query endpoint handlers. New functions normalize and validate conversation IDs in various formats. Existing endpoints are updated to apply normalization when storing and retrieving conversation data. Test coverage is expanded to validate the updated normalization and validation logic. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used📓 Path-based instructions (5)src/**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
src/app/endpoints/**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
src/**/{client,app/endpoints/**}.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
tests/{unit,integration}/**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
tests/**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧬 Code graph analysis (2)src/app/endpoints/query.py (3)
tests/unit/utils/test_suid.py (1)
⏰ 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). (7)
🔇 Additional comments (3)
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/app/endpoints/streaming_query.py (1)
1074-1074: Critical: Same API incompatibility as query.py.This file has the same pipeline failures as
query.py:
- Line 1074:
client.vector_dbsattribute does not exist- Lines 1096, 1098:
agent.create_turn()does not acceptdocumentsortoolgroupsparametersPlease see the review comment on query.py for the critical API incompatibility issue that needs to be resolved.
Also applies to: 1096-1098
tests/unit/app/endpoints/test_query.py (1)
44-44: Critical: Fix incorrect import path for TextDelta and ToolCallDelta.The module
llama_stack_client.types.shared.content_deltadoes not exist in llama-stack-client 0.3.0. TextDelta and ToolCallDelta should be imported fromllama_stack.apis.common.content_types.Update tests/unit/app/endpoints/test_streaming_query.py line 24:
- Current:
from llama_stack_client.types.shared.content_delta import TextDelta, ToolCallDelta- Fix:
from llama_stack.apis.common.content_types import TextDelta, ToolCallDelta
🧹 Nitpick comments (5)
src/app/endpoints/conversations_v3.py (3)
141-187: simplify_conversation_items ignores non-message/tool data despite docstringThe helper currently:
- Only processes items where
type == "message".- Flattens content to a plain text string.
- Groups turns on assistant messages.
The docstring promises “message and tool call information”, but tool calls/results and other item types are ignored, and there’s no metadata (timestamps, ids, etc.).
If downstream consumers need tool usage or richer history, consider either:
- Extending this to handle
tool_call/tool_resultitems and basic metadata, or- Narrowing the docstring to say it returns a minimal text-only chat history.
414-544: DELETE conversation: DB + remote deletion flow is coherent; check existence requirementThe delete handler:
- Validates and normalizes IDs consistently with GET.
- Uses
can_access_conversationwithDELETE_OTHERS_CONVERSATIONSand returns a structured 403 when unauthorized.- Requires the conversation to exist in the local DB (
retrieve_conversation404s otherwise).- Deletes via llama-stack, then calls
delete_conversationto remove local metadata; on remote NotFound it still cleans up local DB and returns success.Two follow-ups:
Local DB existence requirement
GET will still attempt llama-stack even if the conversation isn’t in the DB, but DELETE returns a 404 immediately if the DB row is missing. If there’s a scenario where remote conversations may exist without a local row, you may want to mirror GET’s behavior and at least attempt remote deletion before returning 404. If your design guarantees local rows for anything deletable, current behavior is fine but should be intentional.Pylint line-length on 541 (500 error
cause)
The final 500 “Unknown error while deleting conversation …” line also exceeds 100 characters. Same wrapping approach as for the GET handler will quiet pylint:- raise HTTPException( - status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, - detail={ - "response": "Unknown error", - "cause": f"Unknown error while deleting conversation {normalized_conv_id} : {str(e)}", - }, - ) from e + raise HTTPException( + status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, + detail={ + "response": "Unknown error", + "cause": ( + f"Unknown error while deleting conversation " + f"{normalized_conv_id}: {e}" + ), + }, + ) from e
546-605: UPDATE conversation: auth check uses QUERY_OTHERS_CONVERSATIONSThe update handler:
- Uses
Action.UPDATE_CONVERSATIONon the decorator, which is clear for “own” conversations.- For cross-user updates, it checks
Action.QUERY_OTHERS_CONVERSATIONSinrequest.state.authorized_actionsand passes that intocan_access_conversation.That means: anyone allowed to continue other users’ conversations can also update their topic summaries, while someone who only has
READ_OTHERS_CONVERSATIONScannot. If that’s intentional (update considered a “write” akin to querying), this is OK; if not, you may want a dedicatedUPDATE_OTHERS_CONVERSATIONSaction or reuseREAD_OTHERS_CONVERSATIONShere for symmetry with GET.Worth double‑checking the intended permission model so that READ/QUERY/UPDATE semantics stay predictable.
src/utils/suid.py (1)
46-98: Consider simplifying the validation logic.The
check_suidfunction now handles three different formats with significant code duplication:
conv_prefix handling (lines 47-73)- Plain hex string handling (lines 76-92)
- Standard UUID handling (line 95)
The first two cases share nearly identical logic for hex validation and UUID format conversion. Consider extracting this into a helper function to reduce duplication and improve maintainability.
Apply this refactor to reduce duplication:
+def _hex_to_uuid_string(hex_string: str) -> str: + """Convert a 32-character hex string to UUID format (8-4-4-4-12).""" + if len(hex_string) < 32: + raise ValueError("Hex string must be at least 32 characters") + + # Validate hex + int(hex_string, 16) + + # Extract first 32 chars and format as UUID + uuid_hex = hex_string[:32] + return ( + f"{uuid_hex[:8]}-{uuid_hex[8:12]}-{uuid_hex[12:16]}-" + f"{uuid_hex[16:20]}-{uuid_hex[20:]}" + ) + def check_suid(suid: str) -> bool: try: # Accept llama-stack conversation IDs (conv_<hex> format) if isinstance(suid, str) and suid.startswith("conv_"): - # Extract the hex string after 'conv_' - hex_part = suid[5:] # Remove 'conv_' prefix - - # Verify it's a valid hex string - # llama-stack may use 32 hex chars (UUID) or 36 hex chars (UUID + suffix) - if len(hex_part) < 32: - return False - - # Verify all characters are valid hex try: - int(hex_part, 16) + uuid_str = _hex_to_uuid_string(suid[5:]) + uuid.UUID(uuid_str) + return True except ValueError: return False - - # Extract the first 32 hex characters (the UUID part) - uuid_hex = hex_part[:32] - - # Convert to UUID format with hyphens: 8-4-4-4-12 - uuid_str = ( - f"{uuid_hex[:8]}-{uuid_hex[8:12]}-{uuid_hex[12:16]}-" - f"{uuid_hex[16:20]}-{uuid_hex[20:]}" - ) - - # Validate it's a proper UUID - uuid.UUID(uuid_str) - return True # Check if it's a plain hex string (database format without conv_ prefix) if isinstance(suid, str) and len(suid) >= 32: try: - int(suid, 16) - # Extract the first 32 hex characters (the UUID part) - uuid_hex = suid[:32] - - # Convert to UUID format with hyphens: 8-4-4-4-12 - uuid_str = ( - f"{uuid_hex[:8]}-{uuid_hex[8:12]}-{uuid_hex[12:16]}-" - f"{uuid_hex[16:20]}-{uuid_hex[20:]}" - ) - - # Validate it's a proper UUID + uuid_str = _hex_to_uuid_string(suid) uuid.UUID(uuid_str) return True except ValueError: - pass # Not a valid hex string, try standard UUID validation + pass - # accepts strings and bytes only for UUID validation uuid.UUID(suid) return True except (ValueError, TypeError): return Falsesrc/app/endpoints/streaming_query_v2.py (1)
360-360: Refactor to reduce local variables (optional).Pylint warns about too many local variables (16/15) in the
retrieve_responsefunction. While this is a minor issue, consider extracting conversation creation logic into a helper function to improve readability.Example refactor:
async def _get_or_create_conversation( client: AsyncLlamaStackClient, conversation_id: str | None, ) -> str | None: """Get existing or create new conversation, returning the conversation ID.""" if conversation_id: return conversation_id logger.debug("No conversation_id provided, creating new conversation") try: conversation = await client.conversations.create(metadata={}) logger.info("Created new conversation with ID: %s", conversation.id) return conversation.id except Exception as e: # pylint: disable=broad-exception-caught logger.warning( "Failed to create conversation: %s, proceeding without conversation_id", e, ) return NoneThen call it at Line 419:
conversation_id = await _get_or_create_conversation( client, query_request.conversation_id )
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
pyproject.toml(1 hunks)src/app/endpoints/conversations_v3.py(1 hunks)src/app/endpoints/query.py(5 hunks)src/app/endpoints/query_v2.py(4 hunks)src/app/endpoints/streaming_query.py(6 hunks)src/app/endpoints/streaming_query_v2.py(6 hunks)src/app/routers.py(2 hunks)src/constants.py(1 hunks)src/metrics/utils.py(1 hunks)src/models/requests.py(1 hunks)src/utils/suid.py(1 hunks)src/utils/token_counter.py(1 hunks)src/utils/types.py(2 hunks)tests/unit/app/endpoints/test_query.py(7 hunks)tests/unit/app/endpoints/test_streaming_query.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-18T10:58:14.951Z
Learnt from: matysek
Repo: lightspeed-core/lightspeed-stack PR: 292
File: pyproject.toml:47-47
Timestamp: 2025-08-18T10:58:14.951Z
Learning: psycopg2-binary is required by some llama-stack providers in the lightspeed-stack project, so it cannot be replaced with psycopg v3 or moved to optional dependencies without breaking llama-stack functionality.
Applied to files:
pyproject.toml
🧬 Code graph analysis (7)
src/app/routers.py (1)
tests/unit/app/test_routers.py (1)
include_router(38-53)
src/app/endpoints/query.py (4)
src/utils/types.py (2)
TurnSummary(114-186)content_to_str(18-37)src/utils/suid.py (1)
normalize_conversation_id(101-122)src/app/database.py (1)
get_session(34-40)src/models/database/conversations.py (1)
UserConversation(11-38)
src/app/endpoints/query_v2.py (1)
src/utils/suid.py (1)
to_llama_stack_conversation_id(125-145)
src/utils/types.py (1)
src/models/responses.py (1)
RAGChunk(206-211)
src/app/endpoints/streaming_query_v2.py (2)
src/utils/suid.py (1)
to_llama_stack_conversation_id(125-145)src/app/endpoints/streaming_query.py (1)
stream_start_event(145-166)
src/app/endpoints/conversations_v3.py (11)
src/app/database.py (1)
get_session(34-40)src/authentication/__init__.py (1)
get_auth_dependency(14-60)src/authorization/middleware.py (1)
authorize(111-122)src/client.py (2)
AsyncLlamaStackClientHolder(18-55)get_client(49-55)src/configuration.py (1)
configuration(73-77)src/models/config.py (1)
Action(329-377)src/models/database/conversations.py (1)
UserConversation(11-38)src/models/requests.py (1)
ConversationUpdateRequest(418-441)src/models/responses.py (10)
AccessDeniedResponse(1074-1097)BadRequestResponse(1048-1071)ConversationDeleteResponse(738-771)ConversationDetails(774-840)ConversationResponse(670-735)ConversationUpdateResponse(993-1025)NotFoundResponse(1100-1123)ServiceUnavailableResponse(1126-1148)UnauthorizedResponse(1151-1174)dump_detail(1043-1045)src/utils/endpoints.py (4)
can_access_conversation(82-109)check_configuration_loaded(112-124)delete_conversation(29-43)retrieve_conversation(46-56)src/utils/suid.py (3)
check_suid(19-98)normalize_conversation_id(101-122)to_llama_stack_conversation_id(125-145)
src/app/endpoints/streaming_query.py (1)
src/utils/types.py (2)
TurnSummary(114-186)content_to_str(18-37)
🪛 GitHub Actions: Pyright
src/app/endpoints/query.py
[error] 231-231: Pyright: No parameter named 'toolgroups' (reportCallIssue).
[error] 782-782: Pyright: Cannot access attribute 'vector_dbs' for class 'AsyncLlamaStackClient'. Attribute 'vector_dbs' is unknown (reportAttributeAccessIssue).
[error] 804-804: Pyright: No parameter named 'documents' (reportCallIssue).
[error] 806-806: Pyright: No parameter named 'toolgroups' (reportCallIssue).
src/utils/types.py
[error] 61-61: Pyright: Method 'get_tool_calls' overrides class 'ToolParser' in an incompatible manner: parameter/return type mismatch across overridden method.
src/app/endpoints/conversations_v3.py
[error] 632-632: Pyright: Cannot access attribute 'update_conversation' for class 'AsyncConversationsResource'. Attribute 'update_conversation' is unknown (reportAttributeAccessIssue).
src/app/endpoints/streaming_query.py
[error] 1074-1074: Pyright: Cannot access attribute 'vector_dbs' for class 'AsyncLlamaStackClient'. Attribute 'vector_dbs' is unknown (reportAttributeAccessIssue).
[error] 1096-1096: Pyright: No parameter named 'documents' (reportCallIssue).
[error] 1098-1098: Pyright: No parameter named 'toolgroups' (reportCallIssue).
🪛 GitHub Actions: Python linter
src/app/endpoints/streaming_query_v2.py
[warning] 360-360: Pylint: Too many local variables (16/15) (R0914).
tests/unit/app/endpoints/test_streaming_query.py
[error] 24-24: ImportError: Unable to import 'llama_stack_client.types.shared.content_delta' (E0401).
[error] 29-29: ImportError: Unable to import 'llama_stack_client.types.alpha.agents.turn_response_event_payload' (E0401).
src/app/endpoints/conversations_v3.py
[error] 409-409: Pylint: Line too long (101/100) (C0301).
[error] 541-541: Pylint: Line too long (102/100) (C0301).
[error] 687-687: Pylint: Line too long (102/100) (C0301).
[error] 345-345: Pylint: Import outside toplevel (llama_stack_client.NOT_GIVEN) (import-outside-toplevel) (C0415).
🪛 GitHub Actions: Type checks
src/utils/types.py
[error] 61-61: Return type "list[llama_stack_client.types.shared.tool_call.ToolCall]" of "get_tool_calls" incompatible with return type "list[llama_stack_client.lib.agents.types.ToolCall]" in supertype "llama_stack_client.lib.agents.tool_parser.ToolParser" [override]
[error] 61-61: Argument 1 of "get_tool_calls" is incompatible with supertype "llama_stack_client.lib.agents.tool_parser.ToolParser"; supertype defines the argument type as "CompletionMessage" [override]
🪛 GitHub Actions: Unit tests
tests/unit/app/endpoints/test_query.py
[error] 44-44: ModuleNotFoundError: No module named 'llama_stack_client.types.shared.content_delta' during test import.
tests/unit/app/endpoints/test_streaming_query.py
[error] 24-24: ModuleNotFoundError: No module named 'llama_stack_client.types.shared.content_delta' during test import.
🔇 Additional comments (20)
src/utils/token_counter.py (1)
10-10: Turn type import moved to alpha namespaceThe import swap to the alpha namespace is consistent with the rest of the PR and shouldn’t affect behavior, assuming llama-stack-client 0.3.0 exposes
Turnat this path. Once pyright passes, this should be good to go.src/metrics/utils.py (1)
8-8: Aligning Turn import with alpha namespaceThis matches the alpha-type migration elsewhere and keeps metrics utilities in sync. As long as the new path exists in llama-stack-client 0.3.0, no further changes are needed.
src/app/endpoints/conversations_v3.py (1)
190-249: List conversations handler looks sound; authorization behavior is explicitThe listing endpoint:
- Correctly gates access on
Action.LIST_CONVERSATIONSvia@authorize.- Narrows results to the authenticated user unless
LIST_OTHERS_CONVERSATIONSis present inrequest.state.authorized_actions.- Returns a list of
ConversationDetailsbuilt from the DB model, handling potentialNonetimestamps.This is a reasonable, minimal implementation for listing conversation summaries.
src/constants.py (1)
3-5: Maximal supported llama-stack version now matches dependency pinsUpdating
MAXIMAL_SUPPORTED_LLAMA_STACK_VERSIONto"0.3.0"aligns this constant with thellama-stack==0.3.0andllama-stack-client==0.3.0pins inpyproject.toml, which keeps version checks consistent.pyproject.toml (1)
24-55: llama-stack and llama-stack-client bumped to 0.3.0The dependency pins for
llama-stackandllama-stack-clientare now at 0.3.0 and match the updatedMAXIMAL_SUPPORTED_LLAMA_STACK_VERSION. This is consistent with the alpha-namespace and Conversations API usage elsewhere in the PR. Make sure CI/integration tests cover basic model listing, inference, and conversations flows against 0.3.0.src/models/requests.py (1)
6-8: Document import moved to alpha namespace; logic remains soundUsing
Documentfromllama_stack_client.types.alpha.agents.turn_create_paramskeepsget_documentscorrect assuming the constructor still acceptscontentandmime_type. Please confirm this matches the 0.3.x client types.Also applies to: 197-204
src/app/routers.py (1)
5-26: v3 conversations router inclusion looks correct; ensure tests cover new prefixImporting
conversations_v3and mounting it at/v3is consistent with the existing versioned routing; no conflicts are apparent. Please make sure any router tests (e.g.,tests/unit/app/test_routers.py) are updated to expect the new(conversations_v3.router, "/v3")entry.Also applies to: 47-52
src/app/endpoints/query_v2.py (1)
36-37: Conversation creation & ID normalization flow is reasonable; verify round‑trip behaviorCreating a conversation when
query_request.conversation_idis missing, normalizing withto_llama_stack_conversation_id, and only falling back toresponse.idon failure is a good fit for the Conversations API. Please double‑check that theconversation_idyou return here (whether fromclient.conversations.createorresponse.id) is compatible withQueryRequest.conversation_idvalidation and any DB/cache keys, so that clients can safely reuse it on subsequent calls.Also applies to: 374-389, 399-401, 415-421
src/utils/types.py (2)
8-13: content_to_str helper and TurnSummary usage look good
content_to_strcorrectly handles strings, text/image items, lists, and fallbacks, and wiring it intoTurnSummary.append_tool_calls_from_llamamakes tool response handling more robust without changing behavior for existing text content.Also applies to: 18-37, 121-140
57-75: Reject this review comment as factually incorrectThe proposed fix imports non-existent types and would cause ImportError. Web verification shows:
AgentCompletionMessagedoes not exist in llama-stack libraryAgentToolCalldoes not exist in llama-stack library- Current code correctly uses
CompletionMessageandToolCallfrom shared types (which do exist)The existing tests confirm the current signature works correctly. Do not apply the suggested diff.
Likely an incorrect or invalid review comment.
src/app/endpoints/query.py (2)
235-235: LGTM: Content conversion updated to use content_to_str.The replacement of
interleaved_content_as_strwithcontent_to_stris consistent across the codebase. The new function handles multiple content types (str, TextContentItem, ImageContentItem, list) as shown in the relevant snippets.Also applies to: 802-802
231-231: <!-- [scratchpad_start]
[task verify_api_changes final analysis]
Consolidating all web search and code inspection findings.[observations]
Web search 1: Confirmed API rework in 0.3.0, new turn status, new continue_agent_turn endpoint
Did NOT mention removal of toolgroups or documents parametersWeb search 2: Showed create_turn accepts messages, session_id, stream, extra_headers
Did NOT explicitly exclude toolgroups/documentsWeb search 3: Breaking changes were about Tools API (parameters → input_schema)
Did NOT mention toolgroups/documents parameters removed from create_turnWeb search 4: CONFIRMS AsyncLlamaStackClient HAS vector_dbs attribute with list() method
Contradicts review comment claim that "client.vector_dbs attribute does not exist"Code inspection: Line 772 shows
await client.vector_dbs.list()is used
Line 231: toolgroups=None passed to create_turn
Lines 791-796: documents and toolgroups passed to create_turn[analysis]
CRITICAL FINDING: Review comment is INCORRECT on at least one claim:
- Claim: "Line 782: client.vector_dbs attribute does not exist"
- Reality: vector_dbs DOES exist and is documented with list() method
- Code: Line 772 uses it successfully
Also:
- Line numbers are wrong (says 804-806 but actual create_turn is 791-796)
- No web evidence that toolgroups/documents parameters were removed
- Breaking changes found only mention Tools API input_schema change, not create_turn params
[conclusion]
Review comment is INCORRECT. The Pyright errors cited may be false positives or based on outdated type stubs, but the actual llama-stack-client 0.3.0 API documentation confirms vector_dbs exists and provides no evidence that toolgroups/documents parameters are removed.[tag]
[/scratchpad_end] -->
Incorrect: Review comment based on false Pyright errors and inaccurate line numbers.
The verification found that the review comment's core claims are unsupported:
vector_dbs attribute exists: AsyncLlamaStackClient has a vector_dbs attribute with async methods including list(), contradicting the claim it "does not exist." Line 772 in the code successfully uses
await client.vector_dbs.list().toolgroups/documents parameters not confirmed as removed: Web search for 0.3.0 breaking changes identified only one documented breaking change: Tools API now uses input_schema instead of parameters. No evidence was found that toolgroups or documents parameters were removed from create_turn().
Inaccurate line numbers: The review cites "Lines 804, 806" for the second create_turn() call, but the actual code is at lines 791–796.
The Pyright errors appear to be false positives, possibly from outdated type stubs. The actual llama-stack-client 0.3.0 API continues to support the parameters being used.
Likely an incorrect or invalid review comment.
tests/unit/app/endpoints/test_query.py (2)
17-20: Verify alpha module imports in test suite.The test imports have been updated to use alpha modules, matching the changes in the main codebase. However, there's a pipeline failure indicating a missing module.
See the pipeline failure comment for Line 44 below.
1932-1936: LGTM: Test mocks updated consistently.All test mocks have been updated from
interleaved_content_as_strtocontent_to_str, maintaining consistency with the implementation changes. The test assertions and expected behaviors remain unchanged.Also applies to: 2065-2089, 2110-2111, 2184-2185, 2229-2230
src/utils/suid.py (1)
101-145: LGTM: Conversation ID normalization utilities are well-designed.The
normalize_conversation_idandto_llama_stack_conversation_idfunctions provide clean bidirectional conversion between database format (no prefix) and llama-stack format (withconv_prefix). The docstrings include clear examples, and the logic is straightforward.src/app/endpoints/streaming_query.py (2)
20-25: LGTM: Import changes align with query.py.The imports have been updated to use alpha modules and the new
content_to_strutility, consistent with the changes inquery.py.Also applies to: 67-67
446-446: LGTM: Content conversion updated consistently.All instances of content conversion have been updated to use
content_to_str, which properly handles multiple content types including strings, TextContentItem, ImageContentItem, and lists.Also applies to: 615-615, 666-666, 749-749
src/app/endpoints/streaming_query_v2.py (3)
442-442: LGTM: Correct usage of conversation ID conversion.The
to_llama_stack_conversation_idhelper is correctly used to ensure the conversation ID has theconv_prefix when passed to the Responses API.
141-173: Review the conversation lifecycle management carefully.The new conversation lifecycle introduces complexity with multiple code paths:
- Lines 141-143: Uses
context.conversation_idwhich may be provided or newly created- Lines 155-157: Emits start event if
conv_idexists- Lines 160-173: Fallback to extract
conv_idfromresponse.createdevent if not presentPotential issues:
Fallback path may be dead code: If
retrieve_responsealways creates a conversation when none is provided (Lines 420-431), the fallback at Lines 162-173 would never execute. Verify if this fallback is needed.Error handling: The broad exception catch at Line 168 silently sets
conv_id = "". This could mask real errors. Consider logging the exception with more context or failing fast if conversation creation is critical.Start event timing: If the fallback path executes, the start event is delayed until after the first chunk is processed. This could affect client-side timing assumptions.
418-442: Verify conversation creation error handling and confirm llama-stack-client 0.3.0 version availability.The broad exception handler at lines 420-431 silently continues with
conversation_id = Noneif conversation creation fails. This pattern exists identically inquery_v2.pyand is used throughout conversation management code.The publicly available llama-stack-client on PyPI shows only version 0.2.20, yet your
pyproject.tomlspecifiesllama-stack-client==0.3.0(exact version). The codebase actively usesclient.conversationsAPI in multiple endpoints (streaming_query_v2.pyline 424,query_v2.py, andconversations_v3.py), but this API cannot be verified in the public package version.Before merging, confirm:
- Whether llama-stack-client 0.3.0 exists and is accessible in your environment
- Whether the silent failure behavior (continuing without
conversation_id) is intentional or should raise an error
luis5tb
force-pushed
the
conversations
branch
from
6e02e34 to
dc26c61
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/app/endpoints/query.py (2)
285-303: Apply same normalization fix to streaming_query.py call site (line 849)The review comment accurately describes the fix applied to query.py (lines 293–298), where
query_request.conversation_idis normalized before passing tovalidate_conversation_ownership. However, verification reveals an incomplete fix: the parallel call site instreaming_query.py(lines 849–851) still passes the rawquery_request.conversation_idwithout normalization.Since
validate_conversation_ownershipperforms database lookups that expect normalized IDs (line 72–74 of utils/endpoints.py), this inconsistency means the original DB lookup issue persists in the streaming query path. Both call sites must apply the same normalization pattern for the fix to be complete and correct.Apply the normalization to
src/app/endpoints/streaming_query.pyaround line 849 before callingvalidate_conversation_ownership.
330-381: Normalize conversation_id immediately after retrieve_response_func call to ensure consistent DB operationsThe review comment is accurate.
query.py:681retrieve_responsereturns a non-normalizedconversation_idfrom the agent, whilequery_v2.py:311explicitly normalizes before returning (lines 464-470). This causes the DB lookup at line 345 to fail when searching for an existing conversation with an unnormalized ID, even though the ID gets normalized later inpersist_user_conversation_details. This mismatch leads to repeated "first turn" behavior where topic summaries are recomputed unnecessarily.Implement option (a): Add normalization immediately after line 330-338 (
retrieve_response_funccall) and use the normalized value throughout—particularly for the DB lookup at line 345. This decouples the handler from implementation-specific normalization behavior and ensures consistent lookups.
♻️ Duplicate comments (5)
src/app/endpoints/query.py (1)
12-25: llama_stack_client alpha. imports and Toolgroup naming likely don’t match 0.3.x public API*Based on earlier analysis of llama‑stack‑client 0.3.x, the public Python types are exposed under
llama_stack_client.typesandllama_stack_client.types.agents, not undertypes.alpha.*, and the type is namedToolGroup(capital “G”) rather thanToolgroup. Keeping these alpha paths and names is very likely to break imports and type checking once you pin to 0.3.0+.Recommend switching to the stable paths and names used in the client docs (e.g.,
from llama_stack_client.types.agents import Turnandfrom llama_stack_client.types import Document, ToolGroup, ToolExecutionStep) and updating usages accordingly. This mirrors the earlier review feedback and should resolve the pyright/import issues. Based on past review comments, ...Check the official llama-stack-client 0.3.x Python docs for the recommended import paths and type names for Turn, Document, ToolGroup, and ToolExecutionStep (verify whether any `types.alpha.*` modules are documented or intended for public use).src/app/endpoints/conversations_v3.py (4)
414-544: DELETE /conversations/{conversation_id}: behavior is sound; long error f-string still needs wrappingThe delete flow—ID validation, normalization, access check via
can_access_conversation, remote delete, then localdelete_conversation, with a graceful “already deleted” path onNotFoundError—is well-structured.The remaining issue is the 500 handler’s
causef-string (Line ~541), which is longer than 100 characters and still triggers pylint C0301. Wrapping it as in the earlier review (split into two concatenated f-strings or a local variable) will address this cleanly.
680-689: UPDATE 500 handler: wrap long f-string for pylint C0301 complianceThe 500 error handler’s
causestring still has an over‑100‑character f-string:"cause": f"Unknown error while updating conversation {normalized_conv_id} : {str(e)}",As in the earlier review, wrap this into a parenthesized multi-line expression or precompute a shorter variable so no single line exceeds the limit.
252-411: GET /conversations/{conversation_id}: keep NOT_GIVEN at module scope and wrap long error lineTwo previously-flagged issues remain:
from llama_stack_client import NOT_GIVENis inside the handler (Line ~345), which triggersimport-outside-topleveland is unnecessary—NOT_GIVENcan be imported once at module scope next toAPIConnectionErrorandNotFoundError.- The 500 handler’s
causef-string (Line ~409) exceeds the 100‑char pylint limit.The earlier suggested diff (moving the import up and wrapping the f-string into a parenthesized multi-line expression) still applies and will fix both linter violations without changing behavior.
621-659: UPDATE via Conversations API: method name likely incorrect in llama-stack-client 0.3.xThis endpoint still calls
await client.conversations.update_conversation(...), which earlier analysis showed does not exist on the async conversations resource in llama-stack-client 0.3.x; the documented method isupdate(...)with aconversation_idargument. Keepingupdate_conversationwill raise at runtime and fail pyright’s attribute checks.Adjusting this to:
await client.conversations.update( conversation_id=llama_stack_conv_id, metadata=metadata, )should align with the client’s public API. This is the same issue noted in the previous review and appears unresolved. Based on past review comments, ...
Find the official llama-stack-client 0.3.x Python API reference for the Conversations resource and confirm the exact async method name and signature for updating conversation metadata (look for `conversations.update` vs. `update_conversation`).
🧹 Nitpick comments (6)
src/utils/suid.py (1)
23-97: check_suid: llama-stack conv_ and hex handling looks robust; tighten doc/type consistencyThe new validation logic for
conv_...IDs and 32+‑hex “DB format” IDs is sound and should correctly accept all expected formats while still rejecting malformed values. One nit: the docstring now advertisessuid (str | bytes)and “byte representation”, but the function is typed asstrand passessuidpositionally intouuid.UUID(suid), which only works for string forms, not raw bytes. Either narrow the docstring back tostror explicitly supportbytesviauuid.UUID(bytes=suid)and adjust the type hint accordingly.src/app/endpoints/query.py (1)
112-166: persist_user_conversation_details: good normalization; consider moving import to module scopeNormalizing
conversation_idvianormalize_conversation_idbefore all DB access creates a single source of truth (normalized ID) forUserConversation. The additional logging also helps trace issues.The only nit is the local
from utils.suid import normalize_conversation_idimport: unless you’re avoiding a circular import, it’s cleaner and pylint‑friendlier to import this once at module level and reuse it here and inquery_endpoint_handler_base.src/app/endpoints/streaming_query_v2.py (1)
347-447: Streaming Responses API: align returned conversation_id with normalized semanticsThe streaming
retrieve_responsecorrectly:
- Creates or reuses a llama-stack conversation (
client.conversations.create+to_llama_stack_conversation_id)- Passes
conversation=llama_stack_conv_idintoclient.responses.create- Normalizes newly-created IDs via
normalize_conversation_id(llama_stack_conv_id).However, when a
conversation_idis provided on the request, it’s returned as‑is (potentially still prefixed withconv_), while the non-streaming Responses path always returns a normalized ID. To keep v2 APIs consistent, consider normalizing before returning, e.g.:- # Return the normalized conversation_id (already normalized above) - # The response_generator will emit it in the start event - return response_stream, conversation_id + # Normalize conversation ID before returning for consistent external representation + normalized_conversation_id = ( + normalize_conversation_id(conversation_id) + if conversation_id + else conversation_id + ) + return response_stream, normalized_conversation_idThis keeps SSE
startevents and DB usage aligned with the rest of the v2 surfaces.src/app/endpoints/conversations_v3.py (3)
141-187: simplify_conversation_items: reasonable first-pass flattening, but drops non-text/tool infoThe helper cleanly groups
messageitems into user/assistant turns and extracts text from both dict and string content parts, which is fine for a basic chat-history view. Be aware that tool calls and non-text message content (images, rich annotations, etc.) are silently discarded; if you later need a richer history, you’ll want to extend this to include those item types or at least surface that they occurred.
190-249: List conversations endpoint: DB-side filtering and response shaping look goodFiltering by
user_idunlessLIST_OTHERS_CONVERSATIONSis granted, and returningConversationDetailswith timestamps, counts, and topic summaries, is a clean design. The genericExceptioncatch returning a 500 “Unknown error” is acceptable here, though you might later want finer-grained handling (e.g., DB connectivity vs. programming errors).
546-605: Add UPDATE_OTHERS_CONVERSATIONS permission to match pattern used by other endpointsVerification confirms the inconsistency. The Action enum defines dedicated *_OTHERS permissions for LIST, READ, QUERY, and DELETE operations, but the UPDATE endpoint (line 588 in conversations_v3.py) uses
QUERY_OTHERS_CONVERSATIONSinstead of a dedicated UPDATE permission. This allows QUERY permission to implicitly grant metadata update capability, which breaks the RBAC model's consistency.Define
UPDATE_OTHERS_CONVERSATIONSin the Action enum (src/models/config.py) and update line 588 in src/app/endpoints/conversations_v3.py to use it.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/app/endpoints/conversations_v3.py(1 hunks)src/app/endpoints/query.py(6 hunks)src/app/endpoints/query_v2.py(6 hunks)src/app/endpoints/streaming_query_v2.py(6 hunks)src/utils/suid.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-06T06:02:21.060Z
Learnt from: eranco74
Repo: lightspeed-core/lightspeed-stack PR: 348
File: src/utils/endpoints.py:91-94
Timestamp: 2025-08-06T06:02:21.060Z
Learning: The direct assignment to `agent._agent_id` in `src/utils/endpoints.py` is a necessary workaround for the missing agent rehydration feature in the LLS client SDK. This allows preserving conversation IDs when handling existing agents.
Applied to files:
src/app/endpoints/conversations_v3.py
🧬 Code graph analysis (3)
src/app/endpoints/query.py (5)
src/utils/types.py (2)
TurnSummary(114-186)content_to_str(18-37)src/utils/suid.py (1)
normalize_conversation_id(101-122)src/app/database.py (1)
get_session(34-40)src/models/database/conversations.py (1)
UserConversation(11-38)src/utils/endpoints.py (1)
validate_conversation_ownership(59-79)
src/app/endpoints/conversations_v3.py (4)
src/client.py (2)
AsyncLlamaStackClientHolder(18-55)get_client(49-55)src/models/database/conversations.py (1)
UserConversation(11-38)src/utils/endpoints.py (4)
can_access_conversation(82-109)check_configuration_loaded(112-124)delete_conversation(29-43)retrieve_conversation(46-56)src/utils/suid.py (3)
check_suid(19-98)normalize_conversation_id(101-122)to_llama_stack_conversation_id(125-145)
src/app/endpoints/streaming_query_v2.py (3)
src/utils/suid.py (2)
normalize_conversation_id(101-122)to_llama_stack_conversation_id(125-145)src/app/endpoints/streaming_query.py (2)
stream_start_event(145-166)retrieve_response(981-1102)src/app/endpoints/query.py (1)
retrieve_response(681-840)
🔇 Additional comments (6)
src/utils/suid.py (1)
101-145: normalize_conversation_id / to_llama_stack_conversation_id: implementation matches intended contractBoth helpers correctly strip/add the
conv_prefix without touching already-normalized IDs, which aligns with how the new endpoints use DB IDs vs. llama-stack IDs. No functional issues here.src/app/endpoints/query_v2.py (1)
36-471: Responses API: conversation creation/propagation and normalization look correctThe new flow—creating a llama-stack conversation when none is provided, converting provided IDs via
to_llama_stack_conversation_id, passingconversation=llama_stack_conv_idintoclient.responses.create, and returning a normalized ID vianormalize_conversation_id—is consistent with the DB helpers and the v3 conversation endpoints. This should give you a stable, prefix-freeconversation_idexternally while using the conv_* form only on the llama-stack side.src/app/endpoints/query.py (3)
64-65: Switch to content_to_str is appropriate for handling interleaved content typesUsing
content_to_strinstead of a bespoke helper for convertingoutput_message.contentto text unifies content handling with the rest of the codebase and correctly supportsTextContentItem, lists, etc.
211-241: get_topic_summary: content_to_str guard is correct and defensiveWrapping the access to
response.output_message.contentwithgetattrchecks and usingcontent_to_stris a safe way to handle both structured and plain content; behavior is clear and robust.
681-840: Agent API response: content_to_str integration and shield/tool handling look solidThe updated
TurnSummaryconstruction usingcontent_to_str(response.output_message.content)is consistent with the new helper and keeps shield/tool processing unchanged. Logging and metrics behavior remain correct.src/app/endpoints/streaming_query_v2.py (1)
141-311: Streaming response generator: start-event behavior and cleanup wiring look correctUsing
conv_id = context.conversation_idplus astart_event_emittedflag to emit a singlestartevent on the first chunk (independent ofresponse.created) simplifies the SSE lifecycle, and reusingconv_idincleanup_after_streamingkeeps transcripts/DB updates tied to the same ID.
luis5tb
force-pushed
the
conversations
branch
from
dc26c61 to
4f1d914
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (8)
src/app/endpoints/query.py (2)
111-157: Conversation ID normalization looks good, but check consistency.The normalization logic here strips the
conv_prefix before storing in the database, which is sensible.However, as noted in the past review comment, ensure that all database queries use normalized IDs consistently. The past review identified that
validate_conversation_ownership()and other queries may not normalize before lookup, which would cause mismatches.
17-25: Fix alpha module import paths.The past review comment already identified that these alpha.* import paths are incorrect for llama-stack-client 0.3.0. The correct imports should be:
from llama_stack_client.types.agents import Turn from llama_stack_client.types import ( ToolGroup, Document, ToolExecutionStep, )Also note the capitalization:
ToolGroupnotToolgroup.tests/unit/app/endpoints/test_streaming_query.py (2)
19-19: Fix broken import path for content_delta types.The import
from llama_stack_client.types.shared.content_delta import TextDelta, ToolCallDeltais failing because the module path doesn't exist in llama-stack-client 0.3.0.As noted in the past review comment, these types should be imported directly from
llama_stack_client.types:-from llama_stack_client.types.shared.content_delta import TextDelta, ToolCallDelta +from llama_stack_client.types import TextDelta, ToolCallDelta
24-30: Fix broken import path for turn response event payload types.The import from
llama_stack_client.types.alpha.agents.turn_response_event_payloadis failing.As noted in the past review comment, import these types directly from
llama_stack_client.types:-from llama_stack_client.types.alpha.agents.turn_response_event_payload import ( - AgentTurnResponseStepCompletePayload, - AgentTurnResponseStepProgressPayload, - AgentTurnResponseTurnAwaitingInputPayload, - AgentTurnResponseTurnCompletePayload, - AgentTurnResponseTurnStartPayload, -) +from llama_stack_client.types import ( + AgentTurnResponseStepCompletePayload, + AgentTurnResponseStepProgressPayload, + AgentTurnResponseTurnAwaitingInputPayload, + AgentTurnResponseTurnCompletePayload, + AgentTurnResponseTurnStartPayload, +)src/app/endpoints/conversations_v3.py (4)
409-409: Wrap long line to satisfy style guidelinesLine 409 exceeds the 100-character limit (101 chars).
Apply this fix:
- "cause": f"Unknown error while getting conversation {normalized_conv_id} : {str(e)}", + "cause": ( + f"Unknown error while getting conversation " + f"{normalized_conv_id}: {e}" + ),
632-635: Critical: Use update() instead of update_conversation()As confirmed in past reviews, llama-stack-client 0.3.0 changed the API from
update_conversation()toupdate(). The current code will raise AttributeError at runtime.Apply this fix:
- await client.conversations.update_conversation( + await client.conversations.update( conversation_id=llama_stack_conv_id, metadata=metadata, )Based on learnings, the method signature in 0.3.0 is
client.conversations.update(conversation_id, metadata=...).
345-345: Move NOT_GIVEN import to module levelAs flagged in past reviews,
NOT_GIVENshould be imported at the top of the file with other llama_stack_client imports (line 7) rather than inside the function.Apply this change:
-from llama_stack_client import APIConnectionError, NotFoundError +from llama_stack_client import APIConnectionError, NotFoundError, NOT_GIVENAnd remove the local import on line 345.
Based on learnings, this aligns with the codebase's import organization standards.
687-687: Wrap long line to satisfy style guidelinesLine 687 exceeds the 100-character limit (102 chars).
Apply this fix:
- "cause": f"Unknown error while updating conversation {normalized_conv_id} : {str(e)}", + "cause": ( + f"Unknown error while updating conversation " + f"{normalized_conv_id}: {e}" + ),
🧹 Nitpick comments (1)
src/app/endpoints/query_v2.py (1)
328-360: Review conversation creation error handling.The logic correctly handles conversation ID conversion and creation:
- If provided, converts to llama-stack format
- If not provided, creates a new conversation upfront
- Normalizes for consistent storage
However, the broad exception handler at line 348 could mask specific errors. Consider catching more specific exceptions (e.g.,
APIConnectionError,APIError) to provide better error context.except (APIConnectionError, APIError) as e: logger.error("Failed to create conversation: %s", e) raiseThis would make debugging easier if conversation creation fails.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
pyproject.toml(1 hunks)src/app/endpoints/conversations_v3.py(1 hunks)src/app/endpoints/query.py(6 hunks)src/app/endpoints/query_v2.py(6 hunks)src/app/endpoints/streaming_query.py(6 hunks)src/app/endpoints/streaming_query_v2.py(6 hunks)src/app/routers.py(2 hunks)src/constants.py(1 hunks)src/metrics/utils.py(1 hunks)src/models/requests.py(1 hunks)src/utils/suid.py(1 hunks)src/utils/token_counter.py(1 hunks)src/utils/types.py(2 hunks)tests/unit/app/endpoints/test_query.py(7 hunks)tests/unit/app/endpoints/test_streaming_query.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/metrics/utils.py
- tests/unit/app/endpoints/test_query.py
- src/models/requests.py
- src/app/routers.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-06T06:02:21.060Z
Learnt from: eranco74
Repo: lightspeed-core/lightspeed-stack PR: 348
File: src/utils/endpoints.py:91-94
Timestamp: 2025-08-06T06:02:21.060Z
Learning: The direct assignment to `agent._agent_id` in `src/utils/endpoints.py` is a necessary workaround for the missing agent rehydration feature in the LLS client SDK. This allows preserving conversation IDs when handling existing agents.
Applied to files:
src/app/endpoints/conversations_v3.py
🧬 Code graph analysis (6)
src/app/endpoints/query.py (4)
src/utils/types.py (1)
content_to_str(18-37)src/utils/suid.py (1)
normalize_conversation_id(101-122)src/models/database/conversations.py (1)
UserConversation(11-38)src/utils/endpoints.py (1)
validate_conversation_ownership(63-83)
src/app/endpoints/streaming_query_v2.py (2)
src/utils/suid.py (2)
normalize_conversation_id(101-122)to_llama_stack_conversation_id(125-145)src/app/endpoints/streaming_query.py (1)
stream_start_event(132-153)
tests/unit/app/endpoints/test_streaming_query.py (1)
src/models/responses.py (1)
ToolCall(33-38)
src/app/endpoints/streaming_query.py (1)
src/utils/types.py (2)
TurnSummary(114-186)content_to_str(18-37)
src/app/endpoints/conversations_v3.py (9)
src/app/database.py (1)
get_session(34-40)src/authentication/__init__.py (1)
get_auth_dependency(14-60)src/authorization/middleware.py (1)
authorize(110-121)src/client.py (2)
AsyncLlamaStackClientHolder(18-55)get_client(49-55)src/models/config.py (1)
Action(329-377)src/models/database/conversations.py (1)
UserConversation(11-38)src/models/responses.py (10)
BadRequestResponse(1143-1175)ConversationDeleteResponse(720-812)ConversationDetails(815-868)ConversationResponse(670-717)ConversationsListResponse(871-906)ConversationUpdateResponse(961-993)NotFoundResponse(1361-1416)ServiceUnavailableResponse(1655-1685)UnauthorizedResponse(1178-1250)conversation(1313-1322)src/utils/endpoints.py (4)
can_access_conversation(86-113)check_configuration_loaded(116-130)delete_conversation(33-47)retrieve_conversation(50-60)src/utils/suid.py (3)
check_suid(19-98)normalize_conversation_id(101-122)to_llama_stack_conversation_id(125-145)
src/app/endpoints/query_v2.py (1)
src/utils/suid.py (2)
normalize_conversation_id(101-122)to_llama_stack_conversation_id(125-145)
🪛 GitHub Actions: Integration tests
src/app/endpoints/conversations_v3.py
[error] 1-1: ImportError: cannot import name 'AccessDeniedResponse' from 'models.responses' during test setup (conversations_v3.py). This blocks pytestOpenAPI tests and integration tests.
🪛 GitHub Actions: Pyright
src/app/endpoints/query.py
[error] 222-222: Pyright: No parameter named "toolgroups" (reportCallIssue)
[error] 764-764: Pyright: Cannot access attribute "vector_dbs" for class "AsyncLlamaStackClient". Attribute "vector_dbs" is unknown (reportAttributeAccessIssue)
[error] 786-786: Pyright: No parameter named "documents" (reportCallIssue)
[error] 788-788: Pyright: No parameter named "toolgroups" (reportCallIssue)
src/app/endpoints/streaming_query.py
[error] 1060-1060: Pyright: Cannot access attribute "vector_dbs" for class "AsyncLlamaStackClient". Attribute "vector_dbs" is unknown (reportAttributeAccessIssue)
[error] 1082-1082: Pyright: No parameter named "documents" (reportCallIssue)
[error] 1084-1084: Pyright: No parameter named "toolgroups" (reportCallIssue)
src/app/endpoints/conversations_v3.py
[error] 18-18: Pyright: "AccessDeniedResponse" is unknown import symbol (reportAttributeAccessIssue)
[error] 286-286: Pyright: Cannot access attribute "dump_detail" for class "BadRequestResponse". Attribute "dump_detail" is unknown (reportAttributeAccessIssue)
[error] 387-387: Pyright: Cannot access attribute "dump_detail" for class "ServiceUnavailableResponse". Attribute "dump_detail" is unknown (reportAttributeAccessIssue)
[error] 396-396: Pyright: Cannot access attribute "dump_detail" for class "NotFoundResponse". Attribute "dump_detail" is unknown (reportAttributeAccessIssue)
[error] 447-447: Pyright: Cannot access attribute "dump_detail" for class "BadRequestResponse". Attribute "dump_detail" is unknown (reportAttributeAccessIssue)
[error] 503-503: Pyright: Argument missing for parameter "deleted" (reportCallIssue)
[error] 505-505: Pyright: No parameter named "success" (reportCallIssue)
[error] 506-506: Pyright: No parameter named "response" (reportCallIssue)
[error] 514-514: Pyright: Cannot access attribute "dump_detail" for class "ServiceUnavailableResponse". Attribute "dump_detail" is unknown (reportAttributeAccessIssue)
[error] 525-525: Pyright: Argument missing for parameter "deleted" (reportCallIssue)
[error] 527-527: Pyright: No parameter named "success" (reportCallIssue)
[error] 528-528: Pyright: No parameter named "response" (reportCallIssue)
[error] 577-577: Pyright: Cannot access attribute "dump_detail" for class "BadRequestResponse". Attribute "dump_detail" is unknown (reportAttributeAccessIssue)
[error] 613-613: Pyright: Cannot access attribute "dump_detail" for class "NotFoundResponse". Attribute "dump_detail" is unknown (reportAttributeAccessIssue)
[error] 632-632: Pyright: Cannot access attribute "update_conversation" for class "AsyncConversationsResource". Attribute "update_conversation" is unknown (reportAttributeAccessIssue)
[error] 666-666: Pyright: Cannot access attribute "dump_detail" for class "ServiceUnavailableResponse". Attribute "dump_detail" is unknown (reportAttributeAccessIssue)
[error] 674-674: Pyright: Cannot access attribute "dump_detail" for class "NotFoundResponse". Attribute "dump_detail" is unknown (reportAttributeAccessIssue)
[error] 732-732: Pyright: Cannot access attribute "update_conversation" for class "AsyncConversationsResource". Attribute "update_conversation" is unknown (reportAttributeAccessIssue)
src/utils/types.py
[error] 61-61: Pyright: Method "get_tool_calls" overrides class "ToolParser" in an incompatible manner. Return type "list[ToolCall]" is incompatible with base return type "List[ToolCall]"
🪛 GitHub Actions: Python linter
tests/unit/app/endpoints/test_streaming_query.py
[error] 19-19: E0401: Unable to import 'llama_stack_client.types.shared.content_delta' (import-error)
[error] 24-24: E0401: Unable to import 'llama_stack_client.types.alpha.agents.turn_response_event_payload' (import-error)
[error] 32-32: C0412: Imports from package llama_stack_client are not grouped (ungrouped-imports)
src/app/endpoints/conversations_v3.py
[error] 409-409: C0301: Line too long (101/100) (line-too-long)
[error] 541-541: C0301: Line too long (102/100) (line-too-long)
[error] 687-687: C0301: Line too long (102/100) (line-too-long)
[error] 17-17: E0611: No name 'AccessDeniedResponse' in module 'models.responses' (no-name-in-module)
[error] 345-345: E0415: Import outside toplevel (llama_stack_client.NOT_GIVEN) (import-outside-toplevel)
[error] 503-503: E1123: Unexpected keyword argument 'success' in constructor call (unexpected-keyword-arg)
[error] 503-503: E1123: Unexpected keyword argument 'response' in constructor call (unexpected-keyword-arg)
[error] 503-503: E1125: Missing mandatory keyword argument 'deleted' in constructor call (missing-kwoa)
[error] 525-525: E1123: Unexpected keyword argument 'success' in constructor call (unexpected-keyword-arg)
[error] 525-525: E1123: Unexpected keyword argument 'response' in constructor call (unexpected-keyword-arg)
[error] 525-525: E1125: Missing mandatory keyword argument 'deleted' in constructor call (missing-kwoa)
🪛 GitHub Actions: Type checks
src/utils/types.py
[error] 61-61: Mypy override type mismatch: Return type 'list[llama_stack_client.types.shared.tool_call.ToolCall]' incompatible with supertype return 'list[llama_stack_client.lib.agents.types.ToolCall]'; also argument type mismatch for get_tool_calls (expected 'CompletionMessage')
🪛 GitHub Actions: Unit tests
tests/unit/app/endpoints/test_streaming_query.py
[error] 19-19: ModuleNotFoundError: No module named 'llama_stack_client.types.shared.content_delta'
src/app/endpoints/conversations_v3.py
[error] 17-17: ImportError: cannot import name 'AccessDeniedResponse' from 'models.responses'
🔇 Additional comments (22)
src/constants.py (1)
5-5: LGTM!Version constant correctly updated to match the dependency bump in pyproject.toml.
src/utils/token_counter.py (1)
10-10: Verify alpha module import path is correct.The import has been updated to use the alpha submodule path. Ensure this path exists in llama-stack-client 0.3.0 and is stable.
This aligns with the broader migration to alpha modules across the codebase.
src/utils/types.py (2)
18-37: LGTM!The
content_to_strhelper provides a clean, type-safe way to convert various content types to strings. The implementation handles None, strings, TextContentItem, ImageContentItem, and lists correctly.
61-75: Code is correctly typed — review comment is incorrect.The documented import path for llama-stack-client v0.3.0 is
from llama_stack_client.types import ToolCall. The code usesfrom llama_stack_client.types.shared.tool_call import ToolCall, which is a valid sub-path of this import.ToolParser.get_tool_calls returns
List[ToolCall]from the shared types, which matches the method signature in the code. The review comment's claim that the superclass expectsllama_stack_client.lib.agents.types.ToolCallis unsupported — no evidence of this import path exists in the documented API for v0.3.0.src/app/endpoints/query.py (2)
226-226: LGTM!The usage of
content_to_strto convert response content is correct and aligns with the new helper function introduced in utils/types.py.Also applies to: 794-794
783-789: Verify create_turn parameters are supported in llama-stack-client 0.3.0.Pipeline failures indicate that
documentsandtoolgroupsparameters may not be recognized. Given that the past review confirmed these parameters ARE valid in 0.3.0, this suggests the import paths for the types are incorrect, preventing proper type checking.Once the import paths in lines 17-25 are fixed to use the correct non-alpha paths, these pipeline errors should resolve. The parameters themselves are valid according to the 0.3.0 API documentation.
src/utils/suid.py (3)
19-98: LGTM!The extended
check_suidfunction now correctly handles:
- Llama-stack conversation IDs with
conv_prefix- Plain hex strings (database format)
- Standard UUIDs
The implementation validates hex format, extracts the UUID portion, formats it correctly with hyphens, and validates the resulting UUID structure. The logic is sound and well-documented.
101-122: LGTM!The
normalize_conversation_idfunction provides a clean way to strip theconv_prefix for database storage. Simple, well-documented, and includes helpful examples.
125-145: LGTM!The
to_llama_stack_conversation_idfunction provides the inverse operation, adding theconv_prefix when needed. Well-documented with clear examples.src/app/endpoints/query_v2.py (2)
40-40: LGTM!Correct import of the new normalization helpers from utils.suid.
417-424: LGTM!Correctly normalizes the conversation ID before returning, ensuring consistency with the database storage format (without
conv_prefix).src/app/endpoints/streaming_query.py (4)
20-23: LGTM: Import paths updated for llama-stack-client 0.3.0The migration to
alpha.agentsnamespaces aligns with llama-stack-client 0.3.0's API reorganization.
71-71: LGTM: content_to_str import and usageThe new
content_to_strhelper provides consistent content conversion across different content types (text, images, lists).
433-433: LGTM: Consistent content_to_str usageThe systematic replacement of content conversion logic with
content_to_strimproves consistency and maintainability across streaming handlers.Also applies to: 602-602, 653-653, 736-736
1060-1060: Based on my verification of the llama-stack-client 0.3.0 API:API Compatibility Status
The
vector_dbsattribute exists on the client—it can be used to register and manage vector databases (e.g.,client.vector_dbs.register()andclient.vector_dbs.list()). Line 1060 is valid.Regarding
create_turn()parameters: toolgroups are configured in theAgentConfigduring agent initialization, not passed tocreate_turn(). The code at line 1084 passingtoolgroupstocreate_turn()is incorrect—it should be specified when creating the Agent.For the
documentsparameter, official examples show it being used withcreate_turn()(referenced in agent execution examples), suggesting it may be valid, though usage patterns vary.Correct the following:
- Line 1082: Remove or verify
documentsparameter usage (likely valid, but depends on API usage context)- Line 1084: Move
toolgroupsconfiguration to theAgentinitialization viaAgentConfig, notcreate_turn()Likely an incorrect or invalid review comment.
src/app/endpoints/streaming_query_v2.py (4)
50-50: LGTM: Conversation ID utilities importedThe
normalize_conversation_idandto_llama_stack_conversation_idutilities provide proper ID format conversion between database (no prefix) and llama-stack (conv_ prefix) formats.
143-163: LGTM: Simplified start event emissionThe refactored logic cleanly handles start event emission:
- Uses conversation_id from context (set during retrieve_response)
- Emits start event on first chunk with guaranteed-present ID
- Removes extraction logic from response.created event
406-429: LGTM: Proper conversation ID lifecycleThe implementation correctly handles both scenarios:
- Existing conversation: converts to llama-stack format
- New conversation: creates via API and normalizes the returned ID
The error handling and logging are appropriate. The code assumes
client.conversations.create()returns an ID in llama-stack format (conv_xxx), which is then normalized for database storage.
437-437: LGTM: Conversation ID properly propagatedThe implementation correctly:
- Passes llama-stack formatted ID (conv_xxx) to the Responses API
- Returns normalized ID (no prefix) for database operations and downstream use
Also applies to: 447-449
src/app/endpoints/conversations_v3.py (3)
141-187: LGTM: Well-structured conversation simplification logicThe
simplify_conversation_itemsfunction properly:
- Extracts text content from nested content structures
- Groups messages into turns (user → assistant)
- Handles both dict and string content formats
- Captures incomplete turns at the end
190-249: LGTM: List conversations endpoint with proper authorizationThe endpoint correctly:
- Enforces authorization via
@authorizedecorator- Conditionally filters by user_id based on LIST_OTHERS_CONVERSATIONS permission
- Returns normalized conversation IDs consistent with database storage
- Handles errors appropriately
584-589: Disregard this review commentThe suggested action
UPDATE_OTHERS_CONVERSATIONSdoes not exist in the Action enum. The codebase defines only four *_OTHERS_CONVERSATIONS actions: LIST, READ, QUERY, and DELETE. The update endpoint correctly usesQUERY_OTHERS_CONVERSATIONS, which semantically means "continue the conversations of other users" — a fitting authorization check for update operations.Likely an incorrect or invalid review comment.
| "llama-stack==0.3.0", | ||
| "llama-stack-client==0.3.0", |
There was a problem hiding this comment.
🧩 Analysis chain
Verify llama-stack-client 0.3.0 compatibility and stability.
The PR bumps both llama-stack and llama-stack-client to 0.3.0. Given the alpha module import changes and pipeline failures in tests, ensure that:
- Version 0.3.0 of llama-stack-client is stable and recommended for production use
- The alpha module paths used throughout the codebase are the correct and stable paths for 0.3.0
- Breaking changes in 0.3.0 are documented and addressed
🌐 Web query:
llama-stack-client 0.3.0 release notes breaking changes stability
💡 Result:
Summary — llama-stack-client 0.3.0 (breaking / stability)
-
Main breaking change: tool parameter schema changed — the old "parameters" field was replaced by { input_schema, output_schema } (JSON Schema). Migrate any tool definitions to use input_schema. [1][2]
-
API routes / shapes: several inference/chat-completion endpoints were deprecated/removed (openai/v1 and /inference/completion/chat-completion changes); 0.3.x contains warnings/dual routes and the removals are targeted for 0.4.x, so 0.3.0 is a transitional release that may still surface deprecation warnings. Test against both route forms if you rely on older endpoints. [3][4]
-
Client library changes: the python client release notes for the 0.3.x / 0.4.0-alpha series show multiple API-signature and schema updates (expect further breaking changes in the 0.4 alpha line). Pinning to a specific client release is recommended while you migrate. [5]
Recommendation: audit code that defines tools and calls inference/response endpoints, update tool definitions to input_schema/output_schema, run integration tests against your llama-stack server (watch for deprecation warnings), and pin the client version until you've completed migration and verified stability. [2][3][5]
Sources:
[1] llama-stack releases (v0.3.x) — GitHub.
[2] llama-stack Changelog / 0.3.0 alpha notes (breaking: use input_schema instead of parameters).
[3] Repo commits / docs showing deprecated routes and removal plan for /inference/completion and openai/v1 endpoints.
[4] Mirror/docs: deprecated-llama-stack-spec (explanation of parameters -> input_schema migration).
[5] llama-stack-client-python releases (0.4.0-alpha notes showing related API/schema changes).
I need to examine the codebase to verify the alpha module paths and check for any unmigrated code before generating the final response.
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for tool definitions using old "parameters" field (pre-0.3.0)
echo "=== Searching for old tool parameter definitions ==="
rg -n "parameters\s*:" --type=py -C3 | head -50
echo ""
echo "=== Searching for input_schema/output_schema usage (0.3.0 style) ==="
rg -n "input_schema|output_schema" --type=py | head -30
echo ""
echo "=== Searching for alpha module imports ==="
rg -n "from.*alpha|import.*alpha" --type=py -C2 | head -50
echo ""
echo "=== Checking llama-stack imports and usage ==="
rg -n "llama.stack" --type=py | head -30
Length of output: 7009
I need to verify the correct import paths and tool schema patterns for llama-stack-client 0.3.0.
---
🌐 Web query:
```web
llama-stack-client 0.3.0 alpha module paths import documentation
💡 Result:
You can import the Python client and its types from these module paths (used in v0.3.0-series docs):
-
Core client
- from llama_stack_client import LlamaStackClient. [1]
-
Async client
- from llama_stack_client import AsyncLlamaStackClient. [1]
-
Types (shared / top-level)
- from llama_stack_client.types import (e.g., UserMessage, CompletionMessage, SamplingParams). [2]
-
Namespaced types (subpackages)
- from llama_stack_client.types.agents import (e.g., Turn, TurnStreamEvent). [2]
-
Resource usage pattern (methods under client. e.g. client.models.list(), client.chat.completions.create(...))
- client...(...) as shown in the Python SDK reference and examples. [1][3]
References:
- Llama Stack docs (Python SDK reference / v0.3.0). [1]
- Mirror of Python SDK reference showing types and import examples. [2]
- llama-stack-client-python GitHub README / examples. [3]
Update alpha module imports to stable paths and migrate tool schemas for 0.3.0 compatibility.
The codebase has not properly migrated to llama-stack-client 0.3.0. According to the 0.3.0 documentation, types should be imported from llama_stack_client.types.agents, not from llama_stack_client.types.alpha.agents. Additionally, 0.3.0 introduced a breaking change where the tool parameter schema changed from a "parameters" field to { input_schema, output_schema } (JSON Schema), and the codebase has not been migrated to this new schema format.
Required fixes:
- Alpha import paths (7 files): Update imports in
git/src/utils/token_counter.py,git/src/utils/types.py,git/src/metrics/utils.py,git/src/models/requests.py,git/src/app/endpoints/streaming_query.py,git/src/app/endpoints/query.py, andgit/tests/unit/app/endpoints/test_query.pyfromllama_stack_client.types.alpha.agents.*to stablellama_stack_client.types.agents.*paths. - Tool schema migration: Audit and update any tool definitions to use
input_schema/output_schemainstead of the deprecatedparametersfield.
Additionally, 0.3.0 is a transitional release with deprecation warnings and removals targeted for 0.4.x, suggesting further caution during migration and testing.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
tests/unit/app/endpoints/test_query_v2.py (1)
118-125: Remove duplicate mock setup for conversations.create.Lines 118-125 define and configure
mock_conversationtwice identically. Remove the duplicate lines 122-125 to clean up the code.Apply this diff:
mock_client.responses.create = mocker.AsyncMock(return_value=response_obj) # Mock conversations.create for new conversation creation mock_conversation = mocker.Mock() mock_conversation.id = "conv_abc123def456" mock_client.conversations.create = mocker.AsyncMock(return_value=mock_conversation) - # Mock conversations.create for new conversation creation - mock_conversation = mocker.Mock() - mock_conversation.id = "conv_abc123def456" - mock_client.conversations.create = mocker.AsyncMock(return_value=mock_conversation) # vector_stores.list should not matter when no_tools=True, but keep it validsrc/app/endpoints/conversations_v3.py (3)
402-405: Wrap long error message to satisfy line-length limit.The error message construction on lines 403-404 exceeds 100 characters and should be wrapped.
Apply this diff:
- error_msg = ( - f"Unknown error while getting conversation {normalized_conv_id} : " - f"{str(e)}" - ) + error_msg = ( + f"Unknown error while getting conversation {normalized_conv_id}: {e}" + )
535-538: Wrap long error message to satisfy line-length limit.The error message construction on lines 536-537 exceeds 100 characters and should be wrapped.
Apply this diff:
- error_msg = ( - f"Unknown error while deleting conversation {normalized_conv_id} : " - f"{str(e)}" - ) + error_msg = ( + f"Unknown error while deleting conversation {normalized_conv_id}: {e}" + )
684-687: Wrap long error message to satisfy line-length limit.The error message construction on lines 685-686 exceeds 100 characters and should be wrapped.
Apply this diff:
- error_msg = ( - f"Unknown error while updating conversation {normalized_conv_id} : " - f"{str(e)}" - ) + error_msg = ( + f"Unknown error while updating conversation {normalized_conv_id}: {e}" + )src/utils/endpoints.py (1)
367-382: Address type suppressions consistently withget_agent.This function exhibits the same type-ignore pattern as
get_agent, suppressing warnings forarg-type,call-arg, andattr-defined. Consider addressing both functions together once the llama-stack-client 0.3.0 compatibility is verified.If the verification in
get_agentconfirms these parameters are supported, consider documenting why the type ignores are necessary (e.g., upstream type stub gaps) or explore protocol/interface-based type hints that align with the actual runtime API.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
pyproject.toml(2 hunks)src/app/endpoints/conversations_v3.py(1 hunks)src/utils/endpoints.py(3 hunks)src/utils/types.py(3 hunks)tests/configuration/minimal-stack.yaml(1 hunks)tests/unit/app/endpoints/test_query_v2.py(22 hunks)tests/unit/app/endpoints/test_streaming_query.py(6 hunks)tests/unit/app/endpoints/test_streaming_query_v2.py(7 hunks)tests/unit/app/test_routers.py(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pyproject.toml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-06T06:02:21.060Z
Learnt from: eranco74
Repo: lightspeed-core/lightspeed-stack PR: 348
File: src/utils/endpoints.py:91-94
Timestamp: 2025-08-06T06:02:21.060Z
Learning: The direct assignment to `agent._agent_id` in `src/utils/endpoints.py` is a necessary workaround for the missing agent rehydration feature in the LLS client SDK. This allows preserving conversation IDs when handling existing agents.
Applied to files:
src/app/endpoints/conversations_v3.pysrc/utils/endpoints.py
🧬 Code graph analysis (3)
tests/unit/app/endpoints/test_query_v2.py (1)
tests/unit/app/endpoints/test_streaming_query_v2.py (1)
test_retrieve_response_builds_rag_and_mcp_tools(31-65)
src/utils/endpoints.py (2)
src/utils/types.py (2)
GraniteToolParser(60-99)get_parser(82-99)src/utils/suid.py (1)
get_suid(6-16)
src/utils/types.py (1)
src/models/responses.py (2)
ToolCall(33-38)RAGChunk(25-30)
🔇 Additional comments (20)
tests/configuration/minimal-stack.yaml (1)
8-30: LGTM! Storage configuration aligns with new Conversations API.The storage configuration properly defines backends and stores to support the new v3 Conversations API endpoints. The mapping of conversations to the sql_default backend and the configuration of other stores (metadata, inference, prompts) provides the necessary persistence layer for testing.
tests/unit/app/test_routers.py (1)
12-12: LGTM! Test correctly validates new v3 conversations router.The test updates properly reflect the addition of the conversations_v3 router, including the import, router count increment (18 → 19), presence assertion, and prefix validation (/v3).
Also applies to: 71-71, 88-88, 98-98, 116-117
tests/unit/app/endpoints/test_query_v2.py (1)
142-142: LGTM! Conversation ID normalization correctly applied across all tests.All test assertions now correctly expect the normalized conversation ID format ("abc123def456" without the "conv_" prefix), consistent with the new ID normalization utilities introduced in this PR.
Also applies to: 187-187, 255-255, 306-306, 349-349, 392-392, 606-606, 653-653, 705-705, 756-756
src/utils/types.py (2)
20-40: LGTM! Well-designed content_to_str helper for unified content handling.The new
content_to_strfunction provides a clean abstraction for converting various content types (str, TextContentItem, ImageContentItem, lists) to strings. The implementation handles None gracefully, supports recursive list processing, and provides appropriate fallbacks.
6-14: LGTM! Type updates align with llama-stack-client 0.3.0.The migration to Agent-specific types (AgentCompletionMessage, AgentToolCall) and the updated import paths for alpha namespaces are correct. The replacement of
interleaved_content_as_strwithcontent_to_strmaintains the same behavior while using the new helper.Also applies to: 63-79, 131-131
tests/unit/app/endpoints/test_streaming_query_v2.py (2)
40-43: LGTM! Tests correctly mock conversations.create for ID normalization.All tests properly mock
client.conversations.createto return a conversation with ID "conv_abc123def456", supporting the new ID normalization flow. The mock setup is consistent and correct across all test functions.Also applies to: 76-79, 254-257, 290-293
168-168: LGTM! Streaming tests correctly propagate normalized conversation IDs.The streaming query tests properly validate that normalized conversation IDs ("abc123def456" without the "conv_" prefix) are propagated through the streaming flow and cleanup operations.
Also applies to: 190-190, 206-206
tests/unit/app/endpoints/test_streaming_query.py (2)
51-143: LGTM! Mock classes provide necessary backward compatibility.The mock implementations of TextDelta, ToolCallDelta, TurnResponseEvent, and other Agent API types provide the necessary shims for test compatibility with llama-stack-client 0.3.0. The dynamic attribute assignment pattern is appropriate for test mocks.
358-358: LGTM! ToolCall arguments updated to string format.The tests correctly update ToolCall
argumentsfrom empty dict{}to string"{}", aligning with the API's expected string representation of tool arguments in llama-stack-client 0.3.0.Also applies to: 1083-1083, 1129-1129, 1143-1143
src/app/endpoints/conversations_v3.py (10)
7-7: LGTM! NOT_GIVEN import moved to module level.The import of
NOT_GIVENis now at module level (line 7) alongside other llama-stack-client imports, addressing the previous import-outside-toplevel issue.
24-24: LGTM! Correct response class used.
ForbiddenResponseis correctly imported and used instead of the non-existentAccessDeniedResponse.
286-286: LGTM! Correct Pydantic serialization method used.All response objects correctly use
.model_dump()instead of the non-existent.dump_detail()method for serialization.Also applies to: 317-317, 387-387, 396-396, 448-448, 473-473, 483-483, 514-514, 579-579, 604-604, 613-613, 667-667, 675-675
633-636: LGTM! Correct Conversations API method used.The code correctly uses
client.conversations.update()instead of the non-existentupdate_conversation()method, matching llama-stack-client 0.3.0 API.
503-506: LGTM! Correct ConversationDeleteResponse constructor usage.Both instances correctly construct
ConversationDeleteResponsewith the requiredconversation_idanddeletedparameters, allowing the model to auto-generate the response message.Also applies to: 525-527
141-187: LGTM! Well-designed conversation item simplification.The
simplify_conversation_itemsfunction properly transforms raw llama-stack conversation items into a simplified chat history format. The logic correctly:
- Groups items by turns (user → assistant)
- Extracts text content from various content formats
- Handles both dict and string content types
- Returns structured turn-based chat history
252-412: LGTM! GET conversation endpoint properly validates, normalizes, and retrieves conversation data.The endpoint implementation correctly:
- Validates conversation ID format with
check_suid- Normalizes IDs for database access while preserving user-facing IDs in logs
- Enforces access control with
can_access_conversationandREAD_OTHERS_CONVERSATIONS- Falls back to llama-stack even if conversation isn't in local DB (useful for partial persistence)
- Uses Conversations API to retrieve items and simplifies them for the response
- Handles all error cases with appropriate HTTP status codes
415-545: LGTM! DELETE conversation endpoint properly synchronizes deletion across backends.The endpoint implementation correctly:
- Validates conversation ID and enforces access control
- Deletes from llama-stack backend using Conversations API
- Synchronizes deletion with local database
- Gracefully handles NotFoundError by cleaning up local DB anyway
- Properly constructs ConversationDeleteResponse with required parameters
548-694: LGTM! UPDATE conversation endpoint properly synchronizes metadata updates.The endpoint implementation correctly:
- Validates conversation ID and enforces access control
- Updates conversation metadata in llama-stack using Conversations API
- Synchronizes topic_summary changes to local database
- Uses correct
client.conversations.update()method- Returns properly constructed ConversationUpdateResponse
190-249: LGTM! LIST conversations endpoint properly enforces access control.The endpoint correctly:
- Filters conversations by user_id unless
LIST_OTHERS_CONVERSATIONSis authorized- Retrieves all conversations from local database
- Returns structured ConversationDetails with metadata
- Handles errors with appropriate HTTP 500 response
src/utils/endpoints.py (1)
309-346: Type-ignore annotations appear necessary due to type stubs lagging runtime API in llama-stack-client 0.3.0.The web search confirms that llama-stack-client 0.3.0 officially supports
enable_session_persistenceand the AsyncAgent API, resolving the primary concern about API misalignment. The six type-ignore annotations are likely due to type stubs not being fully updated to reflect the runtime implementation—a common scenario in Python libraries.The code is functionally correct. However, consider adding a comment explaining that these annotations work around type stub limitations, or file an issue with llama-stack to improve their type stubs. The
return-valueignore on line 346 is acceptable if the actual return type doesn't match the declared signature; this is a known workaround pattern.
luis5tb
force-pushed
the
conversations
branch
from
9920869 to
ef8a04a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/utils/endpoints.py (1)
367-379: Same type-checking concerns as inget_agent.This function exhibits the same pattern of
type: ignorecomments forenable_session_persistenceandinitialize(), indicating a systematic issue with llama-stack-client 0.3.0 type definitions.Consider consolidating these workarounds and documenting them in a central location (e.g., a type stubs file or a known issues document) to make it easier to remove them when the library's type definitions are fixed.
tests/unit/app/endpoints/test_streaming_query.py (1)
46-143: Consider refactoring tests to use actual llama-stack-client 0.3.x types.The mock classes provide backward compatibility, which works for now. However, as noted in the comments, these types "don't exist in llama-stack-client 0.3.x." Consider refactoring the tests to use the actual 0.3.x type patterns and event structures instead of maintaining local mocks, which would:
- Ensure tests validate against the real API contracts
- Reduce maintenance burden when the library updates
- Catch API mismatches earlier
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
pyproject.toml(2 hunks)src/app/endpoints/conversations_v3.py(1 hunks)src/utils/endpoints.py(3 hunks)src/utils/types.py(3 hunks)tests/configuration/minimal-stack.yaml(1 hunks)tests/unit/app/endpoints/test_query_v2.py(22 hunks)tests/unit/app/endpoints/test_streaming_query.py(6 hunks)tests/unit/app/endpoints/test_streaming_query_v2.py(7 hunks)tests/unit/app/test_routers.py(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- pyproject.toml
- src/app/endpoints/conversations_v3.py
- tests/unit/app/test_routers.py
- tests/configuration/minimal-stack.yaml
- tests/unit/app/endpoints/test_streaming_query_v2.py
- tests/unit/app/endpoints/test_query_v2.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-06T06:02:21.060Z
Learnt from: eranco74
Repo: lightspeed-core/lightspeed-stack PR: 348
File: src/utils/endpoints.py:91-94
Timestamp: 2025-08-06T06:02:21.060Z
Learning: The direct assignment to `agent._agent_id` in `src/utils/endpoints.py` is a necessary workaround for the missing agent rehydration feature in the LLS client SDK. This allows preserving conversation IDs when handling existing agents.
Applied to files:
src/utils/endpoints.py
🧬 Code graph analysis (2)
src/utils/types.py (2)
src/models/responses.py (2)
ToolCall(33-38)RAGChunk(25-30)tests/unit/app/endpoints/test_streaming_query.py (1)
ToolExecutionStep(127-132)
src/utils/endpoints.py (2)
src/utils/types.py (2)
GraniteToolParser(60-99)get_parser(82-99)src/utils/suid.py (1)
get_suid(6-16)
⏰ 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: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
- GitHub Check: e2e_tests (azure)
- GitHub Check: e2e_tests (ci)
- GitHub Check: build-pr
🔇 Additional comments (7)
src/utils/endpoints.py (2)
346-346: Clarify why return type checking is suppressed.The
type: ignore[return-value]comment bypasses type checking on the function's return contract. The function signature declares-> tuple[AsyncAgent, str, str], which should match the return statement.If the AsyncAgent type itself has changed, consider updating the function signature to match or filing an issue with the library maintainers.
309-321: The review comment is incorrect—the type-ignore suppressions are necessary and appropriate.The test file confirms that "Agent API types don't exist in llama-stack-client 0.3.x," indicating this is a known limitation of the library, not an API breaking change. The code successfully uses these parameters (
input_shields,output_shields,enable_session_persistence) and theinitialize()method at runtime, demonstrating that the API is compatible with version 0.3.0. The type-ignore comments are a necessary workaround for missing type definitions in the library, not masking real type errors. No changes are needed.Likely an incorrect or invalid review comment.
src/utils/types.py (3)
20-40: LGTM! Well-structured content conversion utility.The
content_to_strfunction properly handles various content types with clean fallback logic and recursion for lists. The"<image>"placeholder forImageContentItemis a reasonable text representation.
63-79: LGTM! Type signature correctly updated for 0.3.0.The
GraniteToolParser.get_tool_callsmethod signature properly reflects the new Agent-prefixed types from llama-stack-client 0.3.0. The implementation and logic remain unchanged.
131-131: Migration frominterleaved_content_as_strtocontent_to_stris complete and correct.Verification confirms:
- No remaining references to the old function name in the codebase
- New
content_to_strfunction is properly defined at line 20 with correct type signature- Usage at line 131 correctly handles the None case with the conditional check
- All imports are in order (function defined in the same file)
The migration is complete and the code is correct as-is.
tests/unit/app/endpoints/test_streaming_query.py (2)
14-15: LGTM! Import paths correctly updated to alpha namespaces.The imports for
TurnandShieldCallStepnow reference the correct alpha namespace paths for llama-stack-client 0.3.0, addressing the previous import errors noted in past review comments.
358-358: Verification confirms the test changes are correct.The ToolCall's arguments field is a JSON-encoded string, and clients should JSON-parse that string to access typed arguments. This change was made to avoid numeric-type/deserialize issues when sending raw objects. The test updates from
{}(dict object) to"{}"(JSON string representation) across all four locations (lines 358, 1083, 1129, 1143) properly align with the llama-stack-client 0.3.0 API specification.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
docs/conversations_api.md (2)
14-27: Fix nested list indentation per markdown formatting standards.The nested list items in the Table of Contents use 3-space indentation instead of the standard 2-space indentation required by markdownlint (MD007).
Apply this diff to fix the indentation:
* [Introduction](#introduction) * [Conversation ID Formats](#conversation-id-formats) - * [Llama Stack Format](#llama-stack-format) - * [Normalized Format](#normalized-format) - * [ID Conversion Utilities](#id-conversion-utilities) + * [Llama Stack Format](#llama-stack-format) + * [Normalized Format](#normalized-format) + * [ID Conversion Utilities](#id-conversion-utilities) * [How Conversations Work](#how-conversations-work) - * [Creating New Conversations](#creating-new-conversations) - * [Continuing Existing Conversations](#continuing-existing-conversations) - * [Conversation Storage](#conversation-storage) + * [Creating New Conversations](#creating-new-conversations) + * [Continuing Existing Conversations](#continuing-existing-conversations) + * [Conversation Storage](#conversation-storage) * [API Endpoints](#api-endpoints) - * [Query Endpoint (v2)](#query-endpoint-v2) - * [Streaming Query Endpoint (v2)](#streaming-query-endpoint-v2) - * [Conversations List Endpoint (v3)](#conversations-list-endpoint-v3) - * [Conversation Detail Endpoint (v3)](#conversation-detail-endpoint-v3) + * [Query Endpoint (v2)](#query-endpoint-v2) + * [Streaming Query Endpoint (v2)](#streaming-query-endpoint-v2) + * [Conversations List Endpoint (v3)](#conversations-list-endpoint-v3) + * [Conversation Detail Endpoint (v3)](#conversation-detail-endpoint-v3)
58-60: Add language identifiers to fenced code blocks for syntax highlighting consistency.Several code blocks lack language specification (MD040). While some formats (plain text examples) may not strictly require it, adding identifiers improves consistency, enables syntax highlighting, and aligns with markdown best practices.
Apply these diffs to add language specifications:
- Line 58-60 (format pattern): - ``` + ```text conv_<48-character-hex-string> - ``` + ``` - Line 63-65 (example ID): - ``` + ```text conv_0d21ba731f21f798dc9680125d5d6f493e4a7ab79f25670e - ``` + ``` - Line 77-79 (normalized ID): - ``` + ```text 0d21ba731f21f798dc9680125d5d6f493e4a7ab79f25670e - ``` + ``` - Line 170-176 (YAML config): - ``` + ```yaml storage: stores: conversations: table_name: openai_conversations backend: sql_default - ``` + ``` - Line 234-244 (SSE stream): - ``` + ```text data: {"event": "start", "data": {"conversation_id": "0d21ba731f21f798dc9680125d5d6f493e4a7ab79f25670e"}} data: {"event": "token", "data": {"id": 0, "token": "The "}} data: {"event": "token", "data": {"id": 1, "token": "OpenShift "}} data: {"event": "turn_complete", "data": {"id": 10, "token": "The OpenShift Assisted Installer is..."}} data: {"event": "end", "data": {"referenced_documents": [], "input_tokens": 150, "output_tokens": 200}} - ``` + ``` - Line 369-379 (SSE stream): - ``` + ```text data: {"event": "start", "data": {"conversation_id": "0d21ba731f21f798dc9680125d5d6f493e4a7ab79f25670e"}} data: {"event": "token", "data": {"id": 0, "token": "The "}} data: {"event": "token", "data": {"id": 1, "token": "OpenShift "}} data: {"event": "turn_complete", "data": {"id": 10, "token": "The OpenShift Assisted Installer is..."}} data: {"event": "end", "data": {"referenced_documents": [], "input_tokens": 150, "output_tokens": 200}} - ``` + ``` - Line 420-433 (SQL DDL): - ``` + ```sql CREATE TABLE "lightspeed-stack".user_conversation ( id VARCHAR PRIMARY KEY, -- Normalized conversation ID (48 chars) user_id VARCHAR NOT NULL, -- User identifier last_used_model VARCHAR NOT NULL, -- Model name (e.g., "gemini-2.0-flash-exp") last_used_provider VARCHAR NOT NULL, -- Provider (e.g., "google") created_at TIMESTAMP WITH TIME ZONE DEFAULT NOW(), last_message_at TIMESTAMP WITH TIME ZONE DEFAULT NOW(), message_count INTEGER DEFAULT 0, topic_summary VARCHAR DEFAULT '' ); CREATE INDEX idx_user_conversation_user_id ON "lightspeed-stack".user_conversation(user_id); - ``` + ``` - Line 443-460 (SQL DDL): - ``` + ```sql CREATE TABLE public.openai_conversations ( id VARCHAR(64) PRIMARY KEY, -- Full ID with conv_ prefix (53 chars) created_at TIMESTAMP, metadata JSONB ); CREATE TABLE public.conversation_items ( id VARCHAR(64) PRIMARY KEY, conversation_id VARCHAR(64) REFERENCES openai_conversations(id), turn_number INTEGER, content JSONB, created_at TIMESTAMP ); - ``` + ``` - Line 469-471 (error example): - ``` + ```text Error: Conversation not found (HTTP 404) - ``` + ```Also applies to: 63-65, 77-79, 170-176, 234-244, 369-379, 420-433, 443-460, 469-471
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/conversations_api.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
docs/conversations_api.md
16-16: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
17-17: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
18-18: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
20-20: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
21-21: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
22-22: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
24-24: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
25-25: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
26-26: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
27-27: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
58-58: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
63-63: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
77-77: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
234-234: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
369-369: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
469-469: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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: build-pr
- GitHub Check: e2e_tests (azure)
- GitHub Check: e2e_tests (ci)
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
🔇 Additional comments (1)
docs/conversations_api.md (1)
1-510: Well-structured and comprehensive documentation.The document provides excellent coverage of the Conversations API integration, including clear explanations of conversation ID normalization, practical curl examples, database schema details, and helpful troubleshooting guidance. The organization and examples make it easy for developers to understand the conversation lifecycle and interact with the API endpoints.
luis5tb
force-pushed
the
conversations
branch
from
741919d to
3fba6d0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (20)
tests/integration/endpoints/test_query_v2_integration.py (20)
27-33: Clarify conversation ID comment and prune unused constantsThe comment mentions “valid UUID format” while v2 now normalizes conversation IDs to the llama‑stack / SUID format (e.g.,
"a" * 48in Line 169). Consider rephrasing to “valid conversation ID format” to avoid confusion, and either use or removeTEST_CONVERSATION_ID/OTHER_USER_CONV_IDif they’re not needed.
35-97: Align mocked Llama Stack client with new dependency version and usage expectationsNice focused fixture. A couple of tweaks would make it more robust:
- Given the PR bumps
llama-stack-clientto0.3.0, mockingVersionInfo(version="0.3.0")(or whatever minimum you actually require) will better reflect the real handshake and catch version‑related regressions earlier.- Consider setting a default
mock_response.usage = {"input_tokens": 0, "output_tokens": 0}so token accounting code never has to deal with a missing/oddly typedusageattribute in the “happy path” tests.- If any of
models.list,shields.list, orvector_stores.listare async in the real client, you may want to turn those intomocker.AsyncMock(...)as well to avoid accidentalawaiton a plainMagicMock.
131-174: Also assert that the Responses API was actually invokedThis test nicely validates conversation ID normalization and basic response shaping. Since the docstring also claims to verify that the Llama Stack Responses API is called, consider adding an assertion such as:
mock_llama_stack_client.responses.create.assert_called_once()to tie the behavior explicitly to the external client call.
221-254: Empty-query test docstring doesn’t match current assertionsThe docstring mentions “validation works correctly” and “error response is returned if needed”, but the test only asserts that a non‑
Noneresponse is returned and doesn’t check for an error path. Either:
- tighten the assertions to check for the expected error/validation behavior, or
- relax the docstring to describe the current behavior (e.g. “endpoint returns a response even for empty query strings”).
261-307: Consider asserting that attachments are passed through to the Llama callThis test ensures the endpoint succeeds with attachments, but it doesn’t verify that the attachment content actually makes it into the Llama Stack request (e.g., via
QueryRequest.get_documents()).You could extend it to inspect
mock_llama_stack_client.responses.create.call_argsand assert that the documents/tools payload includes the serialized attachment content.
314-381: Skipped tool-call test: keep mocks close to the real response schemaEven though this test is skipped for LCORE‑1025, it’s worth aligning its mock shape with
OpenAIResponseObject:
- Consider using
spec=OpenAIResponseObjectonmock_responsefor parity with the base fixture.- Populate
mock_response.usageandmock_response.stop_reasonto mirror realistic responses.That will reduce the risk of this test failing for mock‑shape reasons once the skip is removed.
382-445: mcp_list_tools test: strengthen mock shape a bitGood coverage of the
mcp_list_toolspath. For extra robustness:
- You might want
mock_responseto usespec=OpenAIResponseObjectand definestop_reason(e.g.,"end_turn") to better match real responses.This helps ensure the handler logic isn’t depending on attributes that the mock silently omits.
446-514: Multiple-tool-types test: future-proof against response schema changesThe multi‑tool scenario is well chosen. Similar to the mcp_list test, you may want to:
- Give
mock_responseaspec=OpenAIResponseObjectand an explicitstop_reason.- (When the skip is removed) optionally assert more than just
tool_name—e.g., that IDs/types for each summarized tool are present in the response object.This will make the test more sensitive to regressions in tool summarization.
516-567: no_tools=True test correctly verifies tools are omittedThe assertions around
call_kwargs.get("tools") is Nonenicely validate that tools are not sent even when vector stores exist. As an optional enhancement, you could also assert that vector stores were still listed (to prove the bypass is driven byno_tools, not by absence of stores), e.g.mock_llama_stack_client.vector_stores.list.assert_called_once().
569-623: uses_tools_when_available test could be less brittle to multiple callsThe test assumes a single
responses.createcall and inspectscall_args_list[0]. If the handler ever introduces multiple Llama calls (e.g., follow‑up tool rounds), this will break.You could instead check that at least one call included tools:
assert any( call.kwargs.get("tools") for call in mock_llama_stack_client.responses.create.call_args_list )and then inspect the
toolsfrom that call.
680-740: Existing-conversation update test assumes response.id == conversation_idHere you force
mock_llama_stack_client.responses.create.return_value.id = EXISTING_CONV_IDand rely on that to tie the response back to the existing DB row.With the Conversations API in play, it’s possible that the canonical conversation ID will live in a dedicated
conversation_idfield (separate fromresponse.id) in future client versions. If/when that happens, this test might stop reflecting the actual contract. Consider:
- Basing assertions on whatever field the handler really uses for conversation identity (e.g., via the normalization helpers), rather than assuming
response.idis the canonical ID.
742-790: Add a negative ownership test for conversations owned by another userThis test validates the happy path where the authenticated user owns the conversation. Since you already define
OTHER_USER_CONV_ID, it would be valuable to add a companion test that:
- Creates a conversation for a different
user_idwith that ID.- Asserts that querying with that
conversation_idyields a 404/403 (whatever the intended behavior is).That will more fully exercise the ownership validation.
792-847: Cache-entry test relies on positional argument indexAccessing
cache_entry = cache_spy.call_args.args[3]is a bit brittle: any change tostore_conversation_into_cache’s positional signature will silently break this test.If the function defines a named argument for the cache entry (e.g.,
cache_entry=...), prefer pulling it fromcall_args.kwargs[...]. Otherwise, consider adding a small comment here tying index 3 to the current function signature to make the dependency explicit.
905-970: Shield-violation test: consider asserting metrics/log behavior if importantThe test nicely confirms that a refusal still yields a successful response and that the conversation is persisted. Since the docstring also mentions “metrics are updated (validation error counter)”, you might optionally:
- Spy on the metric increment function (or logger) to assert it was called once when a refusal is present.
If metrics aren’t critical here, the current coverage is still acceptable.
972-1020: without_shields test is clear; shield list override is slightly redundantThe behavior around omitting
extra_body.guardrailsis well tested. Note thatmock_llama_stack_client.shields.list.return_value = []is already set in the shared fixture, so re‑assigning it here is redundant (though harmless). You can keep it for explicitness or drop it to reduce duplication.
1022-1073: Empty-LLM-response test could also validate token accountingThis test correctly ensures that an empty assistant message yields an empty
response.responseand still returns a conversation ID. If you want stronger coverage of the handler, you might also assert thatresponse.input_tokens/response.output_tokensreflect the mockedusage(10/0) to catch regressions in token extraction.
1080-1138: Quota-integration test doesn’t assertget_available_quotasspyYou already spy on both
consume_tokensandget_available_quotas, but only assert onconsume_tokens. To fully align with the docstring:
- Keep the existing checks on
consume_tokens.- Also assert that
get_available_quotaswas called, and (optionally) thatresponse.available_quotasmatches its return value.That would close the loop on the “reporting” side of the quota integration.
1140-1193: Quota-exceeded test should assert that the LLM was never calledThe test nicely verifies the 429 response and that no conversations are created. Since the docstring also says “LLM is not called when quota is exceeded”, it would be good to add:
mock_llama_stack_client.responses.create.assert_not_called()to explicitly enforce that behavior.
1201-1279: Transcript-behavior test mutates global config; consider restoring itToggling
test_config.user_data_collection_configuration.transcripts_enabledtoTrueand thenFalseexercises both paths well. BecauseAppConfigis a singleton, it’s safer to:
- Capture the original value at the start of the test, and
- Restore it in a
try/finallyor via a fixture/patch,to avoid any cross‑test leakage if other modules rely on the default transcripts setting.
1287-1347: Conversation-history model test could also assert outbound model/providerThis test confirms that
last_used_modelandlast_used_providerremain consistent and thatmessage_countincrements, which is great. To verify the selection logic end‑to‑end, you might also assert that the call into Llama Stack used those values, e.g.:call = mock_llama_stack_client.responses.create.call_args assert call.kwargs.get("model") == "test-model" # and, if you pass provider separately, assert that as wellThat way you catch both DB persistence and outbound request correctness.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/integration/endpoints/test_query_v2_integration.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
tests/{unit,integration}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/{unit,integration}/**/*.py: Use pytest for all unit and integration tests; do not use unittest framework
Unit tests must achieve 60% code coverage; integration tests must achieve 10% coverage
Files:
tests/integration/endpoints/test_query_v2_integration.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use
pytest-mockwith AsyncMock objects for mocking in tests
Files:
tests/integration/endpoints/test_query_v2_integration.py
🧬 Code graph analysis (1)
tests/integration/endpoints/test_query_v2_integration.py (4)
src/app/endpoints/query_v2.py (1)
query_endpoint_handler_v2(239-262)src/configuration.py (3)
configuration(73-77)AppConfig(39-181)user_data_collection_configuration(94-98)src/models/database/conversations.py (1)
UserConversation(11-38)src/models/requests.py (2)
Attachment(16-70)QueryRequest(73-233)
⏰ 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). (3)
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
- GitHub Check: e2e_tests (azure)
- GitHub Check: e2e_tests (ci)
🔇 Additional comments (4)
tests/integration/endpoints/test_query_v2_integration.py (4)
100-124: DB session patching/restoration pattern looks goodPatching
app.database.engineandsession_localto an in‑memory engine and restoring them afterward is a clean way to isolate integration tests from production DB state. No issues from a correctness standpoint.
176-219: Connection error handling test is solidThe test correctly forces an
APIConnectionErrorand asserts a 503 with a structured detail payload, which matches the intended contract for upstream outages. No changes needed here.
630-678: Conversation persistence test covers key DB invariantsThis test does a good job validating that new conversations are persisted with user ID, model, provider, topic summary, and message count. The pattern of querying via the patched session is clear and matches the fixture setup.
854-898: 404-on-missing-conversation behavior is well coveredThis test clearly asserts the 404 status, error message shape, and that no conversation rows were created in the DB. That gives good confidence that quota/DB logic doesn’t accidentally create records on invalid IDs.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils/endpoints.py (1)
349-382: Fix outdated docstring and inconsistent return order.Two issues here:
Outdated docstring (line 364): The docstring says
tuple[AsyncAgent, str](2 values) but the function returns 3 values and the signature saystuple[AsyncAgent, str, str].Inconsistent return order:
get_temp_agentreturns(agent, session_id, conversation_id)whileget_agentreturns(agent, conversation_id, session_id). This inconsistency is error-prone for callers.Suggested fix - align with
get_agentordering:async def get_temp_agent( client: AsyncLlamaStackClient, model_id: str, system_prompt: str, ) -> tuple[AsyncAgent, str, str]: """Create a temporary agent with new agent_id and session_id. This function creates a new agent without persistence, shields, or tools. Useful for temporary operations or one-off queries, such as validating a question or generating a summary. Args: client: The AsyncLlamaStackClient to use for the request. model_id: The ID of the model to use. system_prompt: The system prompt/instructions for the agent. Returns: - tuple[AsyncAgent, str]: A tuple containing the agent and session_id. + tuple[AsyncAgent, str, str]: A tuple containing the agent, conversation_id, and session_id. """ # ... existing code ... - return agent, session_id, conversation_id + return agent, conversation_id, session_idNote: If you change the return order, update all call sites accordingly.
♻️ Duplicate comments (2)
src/utils/token_counter.py (1)
10-10: Verify alpha module import path is correct for llama-stack-client 0.3.0.This import uses the same
alpha.agents.turnpath that was flagged in previous review comments forsrc/app/endpoints/query.py. The past review indicated that the correct import path for llama-stack-client 0.3.0 should be:from llama_stack_client.types.agents import TurnPlease verify this import path is valid, or update to match the correction suggested in the query.py review.
#!/bin/bash # Verify if alpha module paths exist in llama-stack-client rg -n "from llama_stack_client.types.alpha" --type=py | head -20src/app/endpoints/query.py (1)
17-25: Alpha module import paths need verification.These imports use
alpha.*paths that were flagged in a previous review as potentially incorrect for llama-stack-client 0.3.0. The suggested correction was:from llama_stack_client.types.agents import Turn from llama_stack_client.types import Document, ToolGroup, ToolExecutionStepPlease verify these paths are valid in the version being used.
🧹 Nitpick comments (8)
docs/conversations_api.md (2)
58-65: Add language specifiers to fenced code blocks for proper syntax highlighting.Several code blocks are missing language specifiers, which affects syntax highlighting and IDE tooling. The affected blocks are at lines 58, 63, 77, 234, 369, and 469.
For example, the format strings should specify a language:
-``` -conv_<48-character-hex-string> -``` +```text +conv_<48-character-hex-string> +```Similarly for the error output at line 469:
-``` -Error: Conversation not found (HTTP 404) -``` +```text +Error: Conversation not found (HTTP 404) +```
14-27: Consider using 2-space indentation for nested list items.The nested list items in the table of contents use 3-space indentation, but the project's markdown linter expects 2-space indentation (MD007). This is a minor formatting nit.
src/app/endpoints/query.py (1)
111-114: Consider moving import to module level.The
normalize_conversation_idimport is done inside the function. While this works, it's more conventional to import at module level for better readability and to avoid repeated import overhead on each call.+from utils.suid import normalize_conversation_id + # At top of file with other imports def persist_user_conversation_details( user_id: str, conversation_id: str, model: str, provider_id: str, topic_summary: Optional[str], ) -> None: """Associate conversation to user in the database.""" - from utils.suid import normalize_conversation_id - # Normalize the conversation ID (strip 'conv_' prefix if present) normalized_id = normalize_conversation_id(conversation_id)src/app/endpoints/query_v2.py (2)
328-351: Conversation ID handling logic is correct.The implementation properly:
- Converts existing IDs to llama-stack format for API calls
- Creates new conversations when none provided
- Normalizes IDs for storage
One minor suggestion: the broad
except Exceptionat line 348 could catch unexpected errors. Consider catchingAPIConnectionErrorspecifically since that's the main expected failure mode, per coding guidelines.- except Exception as e: # pylint: disable=broad-exception-caught + except APIConnectionError as e: logger.error("Failed to create conversation: %s", e) raise
416-424: Normalization before return is correct, but the None check appears redundant.At this point in the code,
conversation_idis always set:
- If provided in request: set at line 330
- If not provided: set at line 342 after creation
The conditional
if conversation_id else conversation_idsuggestsconversation_idcould be falsy, but the flow guarantees it's set. Consider simplifying:- # Normalize conversation ID before returning (remove conv_ prefix for consistency) - normalized_conversation_id = ( - normalize_conversation_id(conversation_id) - if conversation_id - else conversation_id - ) + # Normalize conversation ID before returning (remove conv_ prefix for consistency) + normalized_conversation_id = normalize_conversation_id(conversation_id) return (summary, normalized_conversation_id, referenced_documents, token_usage)src/app/endpoints/conversations_v3.py (1)
141-187: Docstring mentions tool calls but implementation only handles message items
simplify_conversation_itemscurrently ignores non-"message"items (e.g., tool calls) even though the docstring says it keeps “message and tool call information.” Either extend the function to handle tool/tool-call items or tighten the docstring to reflect that it only produces a user/assistant chat history.- """Simplify conversation items to include only essential information. + """Simplify conversation items to include only essential message content. @@ - Simplified items with only essential message and tool call information + Simplified items with only essential message information suitable for chat history displaytests/unit/app/endpoints/test_query.py (1)
1777-1859: Tighten parameter type hints intest_evaluate_model_hints
user_conversation,request_values, andexpected_valuesare annotated aslist, but the parametrization passesNoneorUserConversation, plus 2-tuples. Using more precise annotations will make the tests less misleading without changing behavior.-from pytest.mark.parametrize( +from pytest.mark.parametrize( @@ -def test_evaluate_model_hints( - user_conversation: list, - request_values: list, - expected_values: list, -) -> None: +def test_evaluate_model_hints( + user_conversation: UserConversation | None, + request_values: tuple[str | None, str | None], + expected_values: tuple[str | None, str | None], +) -> None:src/utils/types.py (1)
63-75: Alignget_tool_callstype hints with docstring (allowNone)The docstring says
output_messagemay beNone, and the implementation already short-circuits in that case, but the type annotation is non-optional. Making itAgentCompletionMessage | Noneavoids type-checker noise and matches actual usage.- def get_tool_calls( - self, output_message: AgentCompletionMessage - ) -> list[AgentToolCall]: + def get_tool_calls( + self, output_message: AgentCompletionMessage | None + ) -> list[AgentToolCall]: @@ - output_message (AgentCompletionMessage | None): Completion + output_message (AgentCompletionMessage | None): Completion
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
docs/conversations_api.md(1 hunks)pyproject.toml(2 hunks)src/app/endpoints/conversations_v3.py(1 hunks)src/app/endpoints/query.py(6 hunks)src/app/endpoints/query_v2.py(6 hunks)src/app/endpoints/streaming_query.py(6 hunks)src/app/endpoints/streaming_query_v2.py(6 hunks)src/app/routers.py(2 hunks)src/constants.py(1 hunks)src/metrics/utils.py(1 hunks)src/models/requests.py(1 hunks)src/utils/endpoints.py(3 hunks)src/utils/suid.py(1 hunks)src/utils/token_counter.py(1 hunks)src/utils/types.py(3 hunks)tests/configuration/minimal-stack.yaml(1 hunks)tests/integration/endpoints/test_query_v2_integration.py(2 hunks)tests/unit/app/endpoints/test_query.py(7 hunks)tests/unit/app/endpoints/test_query_v2.py(22 hunks)tests/unit/app/endpoints/test_streaming_query.py(6 hunks)tests/unit/app/endpoints/test_streaming_query_v2.py(7 hunks)tests/unit/app/test_routers.py(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- pyproject.toml
- tests/unit/app/endpoints/test_query_v2.py
- tests/unit/app/test_routers.py
- tests/configuration/minimal-stack.yaml
- src/app/routers.py
- src/utils/suid.py
- src/models/requests.py
- src/metrics/utils.py
- src/app/endpoints/streaming_query.py
- src/constants.py
- tests/integration/endpoints/test_query_v2_integration.py
🧰 Additional context used
📓 Path-based instructions (5)
tests/{unit,integration}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/{unit,integration}/**/*.py: Use pytest for all unit and integration tests; do not use unittest framework
Unit tests must achieve 60% code coverage; integration tests must achieve 10% coverage
Files:
tests/unit/app/endpoints/test_streaming_query_v2.pytests/unit/app/endpoints/test_query.pytests/unit/app/endpoints/test_streaming_query.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use
pytest-mockwith AsyncMock objects for mocking in tests
Files:
tests/unit/app/endpoints/test_streaming_query_v2.pytests/unit/app/endpoints/test_query.pytests/unit/app/endpoints/test_streaming_query.py
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: Use absolute imports for internal modules in LCS project (e.g.,from auth import get_auth_dependency)
All modules must start with descriptive docstrings explaining their purpose
Uselogger = logging.getLogger(__name__)pattern for module logging
All functions must include complete type annotations for parameters and return types, using modern syntax (str | int) andOptional[Type]orType | None
All functions must have docstrings with brief descriptions following Google Python docstring conventions
Function names must use snake_case with descriptive, action-oriented names (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead of modifying input parameters
Useasync deffor I/O operations and external API calls
All classes must include descriptive docstrings explaining their purpose following Google Python docstring conventions
Class names must use PascalCase with descriptive names and standard suffixes:Configurationfor config classes,Error/Exceptionfor exceptions,Resolverfor strategy patterns,Interfacefor abstract base classes
Abstract classes must use ABC with@abstractmethoddecorators
Include complete type annotations for all class attributes in Python classes
Useimport loggingand module logger pattern with standard log levels: debug, info, warning, error
Files:
src/app/endpoints/query.pysrc/app/endpoints/query_v2.pysrc/app/endpoints/streaming_query_v2.pysrc/app/endpoints/conversations_v3.pysrc/utils/types.pysrc/utils/token_counter.pysrc/utils/endpoints.py
src/app/endpoints/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use FastAPI
HTTPExceptionwith appropriate status codes for API endpoint error handling
Files:
src/app/endpoints/query.pysrc/app/endpoints/query_v2.pysrc/app/endpoints/streaming_query_v2.pysrc/app/endpoints/conversations_v3.py
src/**/{client,app/endpoints/**}.py
📄 CodeRabbit inference engine (CLAUDE.md)
Handle
APIConnectionErrorfrom Llama Stack in integration code
Files:
src/app/endpoints/query.pysrc/app/endpoints/query_v2.pysrc/app/endpoints/streaming_query_v2.pysrc/app/endpoints/conversations_v3.py
🧠 Learnings (4)
📚 Learning: 2025-11-24T16:58:04.410Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:58:04.410Z
Learning: Applies to src/**/{client,app/endpoints/**}.py : Handle `APIConnectionError` from Llama Stack in integration code
Applied to files:
tests/unit/app/endpoints/test_query.pysrc/app/endpoints/query.py
📚 Learning: 2025-11-24T16:58:04.410Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:58:04.410Z
Learning: Applies to src/app/endpoints/**/*.py : Use FastAPI `HTTPException` with appropriate status codes for API endpoint error handling
Applied to files:
tests/unit/app/endpoints/test_query.py
📚 Learning: 2025-08-06T06:02:21.060Z
Learnt from: eranco74
Repo: lightspeed-core/lightspeed-stack PR: 348
File: src/utils/endpoints.py:91-94
Timestamp: 2025-08-06T06:02:21.060Z
Learning: The direct assignment to `agent._agent_id` in `src/utils/endpoints.py` is a necessary workaround for the missing agent rehydration feature in the LLS client SDK. This allows preserving conversation IDs when handling existing agents.
Applied to files:
src/app/endpoints/conversations_v3.pysrc/utils/endpoints.py
📚 Learning: 2025-11-24T16:58:04.410Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:58:04.410Z
Learning: Always verify current library versions in `pyproject.toml` rather than assuming versions
Applied to files:
src/utils/types.py
🧬 Code graph analysis (4)
src/app/endpoints/query.py (5)
src/utils/types.py (2)
TurnSummary(118-190)content_to_str(20-39)src/utils/suid.py (1)
normalize_conversation_id(101-122)src/app/database.py (1)
get_session(34-40)src/models/database/conversations.py (1)
UserConversation(11-38)src/utils/endpoints.py (1)
validate_conversation_ownership(63-83)
src/app/endpoints/query_v2.py (1)
src/utils/suid.py (2)
normalize_conversation_id(101-122)to_llama_stack_conversation_id(125-145)
src/utils/types.py (1)
src/models/responses.py (2)
ToolCall(33-38)RAGChunk(25-30)
src/utils/endpoints.py (2)
src/utils/types.py (2)
GraniteToolParser(60-99)get_parser(82-99)src/utils/suid.py (1)
get_suid(6-16)
🪛 markdownlint-cli2 (0.18.1)
docs/conversations_api.md
16-16: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
17-17: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
18-18: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
20-20: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
21-21: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
22-22: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
24-24: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
25-25: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
26-26: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
27-27: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
58-58: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
63-63: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
77-77: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
234-234: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
369-369: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
469-469: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
🔇 Additional comments (21)
src/app/endpoints/query.py (4)
278-293: Normalization before ownership validation looks correct.The conversation ID is now properly normalized before calling
validate_conversation_ownership(), which addresses the consistency concern from the previous review. Thevalidate_conversation_ownershipfunction insrc/utils/endpoints.pyuses the ID directly for DB queries, so normalizing at the call site ensures matching IDs.
226-232: LGTM -content_to_strprovides robust content handling.The
content_to_strutility properly handles various content types (str, TextContentItem, ImageContentItem, lists, None) and returns a string representation. This is a good improvement over the previous implementation.
793-800: LGTM - Consistent use ofcontent_to_strfor response content extraction.The same
content_to_strutility is used here for extracting the LLM response, maintaining consistency with the topic summary extraction.
332-335: I'm encountering persistent issues with repository access that prevent me from executing the verification scripts. Due to this infrastructure limitation, I cannot definitively verify the implementations ofretrieve_response_funcacross both Agent API and Responses API code paths to confirm consistent ID normalization.Manual verification required for conversation_id normalization consistency.
The review comment requests verification that
conversation_idreturned fromretrieve_response_funcis normalized consistently. While the comment citesquery_v2.pylines 416-424 returningnormalized_conversation_id, I cannot access the repository to verify:
- The actual implementation details of
retrieve_responsein both Agent API and Responses API- Whether both implementations consistently normalize IDs before return
- Whether any other code paths might return non-normalized IDs
Please inspect both implementations to confirm all variants of
retrieve_response_func(Agent API and Responses API) return normalized conversation IDs for consistency at the DB query point (line 334).src/utils/endpoints.py (2)
309-321: Type ignores are acceptable workarounds for SDK type issues.The
# type: ignoreannotations address type checking gaps in the llama-stack-client SDK. These are necessary given the SDK's evolving type definitions. Thepylint: disable=unexpected-keyword-arg,no-membercomments appropriately scope the lint suppression to the AsyncAgent construction block.
670-672: I'll help you verify this review comment about ID normalization in thecleanup_after_streamingfunction. Let me search for the relevant code.
<function_calls>
#!/bin/bashFind all calls to cleanup_after_streaming and examine context
rg -n "cleanup_after_streaming" src/ --type=py -B 5 -A 5
</function_calls>I'll now search for where
cleanup_after_streamingis called to verify if callers normalize the conversation IDs:
<function_calls>
#!/bin/bashFind the function definition and all its callers
echo "=== Function Definition ==="
rg -n "def cleanup_after_streaming" src/ --type=py -A 10echo ""
echo "=== All Callers ==="
rg -n "cleanup_after_streaming(" src/ --type=py -B 3 -A 1
</function_calls>Now let me check the normalize_conversation_id function and its usage:
<function_calls>
#!/bin/bashFind normalize_conversation_id function and its usage
echo "=== normalize_conversation_id definition ==="
rg -n "def normalize_conversation_id" src/ --type=py -A 5echo ""
echo "=== normalize_conversation_id usage at call sites ==="
rg -n "normalize_conversation_id" src/ --type=py -B 2 -A 2 | head -100
</function_calls>Let me examine the streaming endpoints mentioned in the review:
<function_calls>
#!/bin/bashLook for streaming endpoint functions
echo "=== Streaming endpoints ==="
rg -n "@.*stream|def.*stream" src/ --type=py -B 2 -A 15 | head -150
</function_calls>Let me check the specific context around line 670-672:
<function_calls>
#!/bin/bashCheck the exact function containing lines 670-672
wc -l src/utils/endpoints.py
echo ""
sed -n '650,690p' src/utils/endpoints.py
</function_calls>Now let me search for how conversation_id parameters are handled in streaming functions:
<function_calls>
#!/bin/bashSearch for functions that call cleanup_after_streaming with their parameters
echo "=== Functions containing cleanup_after_streaming calls ==="
rg -n "cleanup_after_streaming" src/ --type=py -B 20 | grep -E "(def |cleanup_after_streaming|normalize)" | head -50
</function_calls>Let me get a more comprehensive view of the file structure:
<function_calls>
#!/bin/bashFind the function that contains cleanup_after_streaming at lines 670-672
sed -n '640,750p' src/utils/endpoints.py | cat -n
</function_calls>Let me search for all streaming-related functions and how they handle IDs:
<function_calls>
#!/bin/bashFind streaming functions and their ID handling
echo "=== Searching for stream functions and ID handling ==="
rg -n "async def.*stream|@.*router.post.*stream" src/ --type=py -B 2 -A 20 | head -200
</function_calls>Let me check if there's a pattern of normalization in the codebase for IDs:
<function_calls>
#!/bin/bashCheck how conversation IDs are typically processed
echo "=== ID normalization patterns ==="
rg -n "conv_|normalize|remove_prefix" src/utils/endpoints.py --type=py -B 2 -A 2 | head -100
</function_calls>Let me examine the context more carefully by looking at the actual cleanup_after_streaming function:
<function_calls>
#!/bin/bashGet more context around the function
python3 << 'EOF'
with open('src/utils/endpoints.py', 'r') as f:
lines = f.readlines()
# Find cleanup_after_streaming function
for i, line in enumerate(lines):
if 'def cleanup_after_streaming' in line:
print(f"Function starts at line {i+1}")
# Print from definition to next function
for j in range(max(0, i-5), min(len(lines), i+50)):
print(f"{j+1:4d}: {lines[j]}", end='')
break
EOF
</function_calls>Let me search for all places where cleanup_after_streaming is called and verify the ID normalization:
<function_calls>
#!/bin/bashFind every call to cleanup_after_streaming with surrounding context
python3 << 'EOF'
import rewith open('src/utils/endpoints.py', 'r') as f:
content = f.read()
lines = content.split('\n')Find all cleanup_after_streaming calls
matches = []
for i, line in enumerate(lines):
if 'cleanup_after_streaming' in line and '(' in line:
matches.append(i)print(f"Found {len(matches)} calls to cleanup_after_streaming\n")
for idx in matches:
print(f"Call at line {idx+1}:")
# Print surrounding context
start = max(0, idx - 10)
end = min(len(lines), idx + 3)
for i in range(start, end):
marker = ">>> " if i == idx else " "
print(f"{marker}{i+1:4d}: {lines[i]}")
print()
EOF
</function_calls>Let me check what parameters are passed to cleanup_after_streaming:
<function_calls>
#!/bin/bashExtract the exact lines of cleanup_after_streaming calls to see parameters
rg -n "cleanup_after_streaming(" src/utils/endpoints.py --type=py -B 15 | grep -E "(def |cleanup_after_streaming|normalize_conversation_id|conversation_id\s*=)" | head -50
</function_calls>Let me get the full function signature and body:
<function_calls>
#!/bin/bashGet line count and then view the function
sed -n '660,680p' src/utils/endpoints.py | cat -n
</function_calls>Let me take a broader approach and search for the streaming endpoint handler:
<function_calls>
#!/bin/bashFind the function that contains the cleanup_after_streaming call
python3 << 'EOF'
import rewith open('src/utils/endpoints.py', 'r') as f:
lines = f.readlines()Find cleanup_after_streaming at line 672
target_line = 671 # 0-indexed
print(f"Context around line 672:")
for i in range(max(0, target_line-30), min(len(lines), target_line+10)):
print(f"{i+1:4d}: {lines[i]}", end='')
EOF
</function_calls>Let me search for where the conversation_id parameter comes from in streaming endpoints:
<function_calls>
#!/bin/bashFind streaming endpoints that call cleanup_after_streaming
echo "=== Looking for streaming function context ==="
rg -n "@router.post|@router.get|async def" src/utils/endpoints.py --type=py | grep -B 5 -A 15 "stream"
</function_calls>Let me take a different approach and look at the file more systematically:
<function_calls>
#!/bin/bashSearch for functions mentioning "stream" and show their signatures and bodies
python3 << 'EOF'
import rewith open('src/utils/endpoints.py', 'r') as f:
content = f.read()
lines = content.split('\n')Find all cleanup_after_streaming occurrences
for i, line in enumerate(lines):
if 'cleanup_after_streaming' in line:
print(f"\n=== Line {i+1}: cleanup_after_streaming call ===")
# Show function signature
for j in range(i-1, max(0, i-40), -1):
if lines[j].strip().startswith('def ') or lines[j].strip().startswith('async def '):
print(f"Function starts at line {j+1}:")
print(lines[j])
break
# Show the call and context
for j in range(max(0, i-5), min(len(lines), i+5)):
marker = ">>>" if j == i else " "
print(f"{marker} {j+1:4d}: {lines[j]}")
EOF
</function_calls>Now let me search for how the conversation_id parameter flows through:
<function_calls>
#!/bin/bashCheck all usages of conversation_id and normalization around the call site
python3 << 'EOF'
import rewith open('src/utils/endpoints.py', 'r') as f:
lines = f.readlines()Look at lines around 670-672
print("=== Lines 640-720 (showing parameter handling) ===\n")
for i in range(639, min(720, len(lines))):
line = lines[i]
# Highlight interesting lines
if any(keyword in line for keyword in ['def ', 'conversation_id', 'normalize', 'cleanup_after_streaming', '@']):
print(f">>> {i+1:4d}: {line}", end='')
elif 640 <= i < 700:
print(f" {i+1:4d}: {line}", end='')
EOF
</function_calls>Let me verify if there's any normalization happening before or after the cleanup_after_streaming call:
<function_calls>
#!/bin/bashGet the actual full function containing the cleanup_after_streaming call
sed -n '600,750p' src/utils/endpoints.py | head -150 | tail -150
</function_calls>Let me be more precise and look at the exact lines:
<function_calls>
#!/bin/bashShow exact lines 650-700
sed -n '650,700p' src/utils/endpoints.py | nl -v 650
</function_calls>Now let me search for the function definition that contains this code:
<function_calls>
#!/bin/bashSearch backwards from line 672 to find the function definition
python3 << 'EOF'
with open('src/utils/endpoints.py', 'r') as f:
lines = f.readlines()Start from line 672 and go backwards
for i in range(671, -1, -1):
line = lines[i]
if line.strip().startswith('def ') or line.strip().startswith('async def '):
print(f"Function found at line {i+1}: {line.strip()}")
# Show the function
print(f"\nFunction body (lines {i+1} to ~{min(i+50, len(lines))}):")
for j in range(i, min(i+50, len(lines))):
print(f"{j+1:4d}: {lines[j]}", end='')
break
EOF
</function_calls>Let me search for all callers of this function to understand the flow:
<function_calls>
#!/bin/bashFind all callers - search for where cleanup_after_streaming is called
rg -n "cleanup_after_streaming\s*(" src/ --type=py -B 20 -A 5 | head -200
</function_calls>Let me check if normalize_conversation_id is used in streaming endpoints:
<function_calls>
#!/bin/bashCheck for normalize_conversation_id usage patterns
echo "=== Searching for normalize_conversation_id ==="
rg -n "normalize_conversation_id|remove.*prefix|strip.*conv" src/utils/ --type=py -B 2 -A 2
</function_calls>Let me check if there's a utility function and how IDs are processed:
<function_calls>
#!/bin/bashLook for ID handling utilities
echo "=== Checking ID normalization patterns ==="
find src/ -name "*.py" -type f | xargs grep -l "normalize.*id|conv_|prefix" | head -5
</function_calls>Let me look at the imports and utilities in endpoints.py:
<function_calls>
#!/bin/bashCheck imports and utility functions at top of file
head -100 src/utils/endpoints.py | cat -n
</function_calls>Now let me check if there's a streaming function that calls cleanup_after_streaming:
<function_calls>
#!/bin/bashDetailed search - find what calls cleanup_after_streaming
python3 << 'EOF'
with open('src/utils/endpoints.py', 'r') as f:
lines = f.readlines()Find all lines that mention cleanup_after_streaming
for i, line in enumerate(lines):
if 'cleanup_after_streaming' in line:
print(f"\n{'='*60}")
print(f"cleanup_after_streaming call found at line {i+1}")
print(f"{'='*60}")# Go back to find function start func_start = i for j in range(i-1, max(0, i-100), -1): if lines[j].strip() and (lines[j].strip().startswith('def ') or lines[j].strip().startswith('async def ')): func_start = j break # Print function header print(f"\nFunction definition (line {func_start+1}):") print(lines[func_start].rstrip()) # Print the call with context print(f"\nContext around cleanup_after_streaming call:") for j in range(max(0, i-8), min(len(lines), i+5)): marker = " >>> " if j == i else " " print(f"{marker}{j+1:4d}: {lines[j]}", end='')EOF
</function_calls>Let me search for streaming route handlers and parameter extraction:
<function_calls>
#!/bin/bashLook for streaming route and conversation_id parameter handling
rg -n "@router.(post|get).stream|conversation_id.:" src/utils/endpoints.py --type=py -B 2 -A 5 | head -80
</function_calls>Let me check if there's documentation or previous PR context about ID normalization:
<function_calls>
#!/bin/bashCheck for conv_ prefix handling
rg -n "conv_|remove.*prefix|strip" src/utils/endpoints.py --type=py -B 2 -A 2 | head -100
</function_calls>Let me look at the UserConversation model to understand the ID field:
<function_calls>
#!/bin/bashFind UserConversation model
rg -n "class UserConversation" src/ --type=py -A 20
</function_calls>Let me check the database models directory:
<function_calls>
#!/bin/bashFind models files
find src/ -name "model" -o -name "database" | grep -E ".py$"
</function_calls>Let me examine the full context more carefully:
<function_calls>
#!/bin/bashGet full context from line 600 onwards, specifically looking at function that contains line 672
python3 << 'EOF'
with open('src/utils/endpoints.py', 'r') as f:
lines = f.readlines()Find the cleanup_after_streaming call
for i, line in enumerate(lines):
if i == 671: # Line 672 is index 671
# Go back 50 lines to find function definition
for j in range(i, max(0, i-60), -1):
if 'def cleanup_after_streaming' in lines[j] or ('def ' in lines[j] and lines[j].strip().startswith('def ')):
print(f"Found function at line {j+1}")
# Print function
for k in range(j, min(j+40, len(lines))):
print(f"{k+1:4d}: {lines[k]}", end='')
break
break
EOF
</function_calls>Let me try a simpler approach - just get the file at those specific line numbers:
<function_calls>
#!/bin/bashSimply display lines around 670-672 and work from there
echo "=== Lines 665-685 ==="
sed -n '665,685p' src/utils/endpoints.py | cat -n -vecho ""
echo "=== What function contains line 672? ==="
sed -n '1,672p' src/utils/endpoints.py | tail -100 | grep -n "def |async def " | tail -1
</function_calls>Let me check the complete function that contains the cleanup_after_streaming call:
<function_calls>
#!/bin/bashGet the function containing line 672
python3 << 'EOF'
with open('src/utils/endpoints.py', 'r') as f:
content = f.read()
lines = content.split('\n')target = 672 # 1-indexed, so 671 in 0-indexed
print(f"Total lines in file: {len(lines)}")
print(f"\nLine {target}: {lines[target-1]}")Find function definition before line 672
func_def_line = None
for i in range(target-1, -1, -1):
stripped = lines[i].strip()
if stripped.startswith('def ') or stripped.startswith('async def '):
func_def_line = i
breakif func_def_line:
print(f"\nFunction definition at line {func_def_line+1}:")
# Print entire function
indent_level = len(lines[func_def_line]) - len(lines[func_def_line].lstrip())for i in range(func_def_line, min(len(lines), func_def_line + 100)): line = lines[i] if i > func_def_line and line.strip() and not line.startswith(' ' * (indent_level + 1)) and line[0] not in [' ', '\t', '']: break # End of function print(f"{i+1:4d}: {line}")EOF
</function_calls>Let me check if cleanup_after_streaming is defined within the same file or imported:
<function_calls>
#!/bin/bashCheck if cleanup_after_streaming is defined or imported
grep -n "cleanup_after_streaming" src/utils/endpoints.py | head -5
</function_calls>Now let me examine what comes before line 670:
<function_calls>
#!/bin/bashGet more context around the target lines
sed -n '620,710p' src/utils/endpoints.py > /tmp/context.txt
wc -l /tmp/context.txt
cat -n /tmp/context.txt | tail -80
</function_calls>Now let me search for all invocations of the function containing line 672:
<function_calls>
#!/bin/bashFind all callers of this function
python3 << 'EOF'
with open('src/utils/endpoints.py', 'r') as f:
lines = f.readlines()Line 672 is calling cleanup_after_streaming
This is within a function - let's find which one
target = 671 # 0-indexed
Go backwards to find function name
func_name = None
for i in range(target, -1, -1):
line = lines[i]
if line.strip().startswith('def ') or line.strip().startswith('async def '):
# Extract function name
import re
match = re.search(r'(async\s+)?def\s+(\w+)\s*(', line)
if match:
func_name = match.group(2)
breakprint(f"Function containing line 672: {func_name}")
print(f"\nNow searching for callers of {func_name}...")Search for calls to this function
import re
for i, line in enumerate(lines):
if func_name in line and '(' in line and 'def ' not in line:
print(f"Line {i+1}: {line.rstrip()}")
EOF
</function_calls>Let me get the actual file section more carefully:
<function_calls>
#!/bin/bashUse head and tail to isolate the section
head -710 src/utils/endpoints.py | tail -100
</function_calls>Let me verify the actual content at those lines:
<function_calls>
#!/bin/bashDouble check the exact lines
echo "Lines 668-675:"
sed -n '668,675p' src/utils/endpoints.py | nl -v 668echo ""
echo "Now let's find the function this is in:"
python3 << 'EOF'
with open('src/utils/endpoints.py', 'r') as f:
lines = f.readlines()Look at line 672 specifically (0-indexed: 671)
for i in range(671, max(0, 671-100), -1):
if lines[i].strip().startswith('async def ') or lines[i].strip().startswith('def '):
print(f"Function at line {i+1}:")
print(lines[i].rstrip())# Show next 50 lines from function start print("\nFunction body:") for j in range(i, min(i+50, len(lines))): print(f"{j+1:4d}: {lines[j]}", end='') breakEOF
</function_calls>Let me check the imports to see where cleanup_after_streaming comes from:
<function_calls>
#!/bin/bashLook for import or definition of cleanup_after_streaming
head -100 src/utils/endpoints.py | grep -n "cleanup_after_streaming|import|from"
</function_calls>Now let me search across all files to understand the pattern:
<function_calls>
#!/bin/bashSearch all source files for cleanup_after_streaming definition and calls
echo "=== Definition of cleanup_after_streaming ==="
rg -n "def cleanup_after_streaming" src/ --type=py -A 10echo ""
echo "=== All calls to cleanup_after_streaming ==="
rg -n "cleanup_after_streaming(" src/ --type=py -B 3 -A 3
</function_calls>Let me check if cleanup_after_streaming is in a different module:
<function_calls>
#!/bin/bashFind where cleanup_after_streaming comes from
rg -n "cleanup_after_streaming" src/ --type=py
</function_calls>Let me check if the line numbers in the comment might be different:
<function_calls>
#!/bin/bashSearch for the pattern in the review comment
rg -n "get_session()" src/utils/endpoints.py --type=py -A 5 | head -50
</function_calls>Let me verify the file has that many lines:
<function_calls>
#!/bin/bashCheck file size
wc -l src/utils/endpoints.py
echo ""If file is shorter, show relevant section
if [ $(wc -l < src/utils/endpoints.py) -lt 700 ]; then
echo "File is shorter than 700 lines. Showing end of file:"
tail -50 src/utils/endpoints.py
fi
</function_calls>Let me search more broadly for the pattern shown in the review:
<function_calls>
#!/bin/bashSearch for the exact pattern from the review
rg -n "session.query(UserConversation).filter_by(id=conversation_id)" src/ --type=py
</function_calls>Let me check all files in utils directory:
<function_calls>
#!/bin/bashList files in utils
ls -la src/utils/
echo ""
echo "Search all utils files for the pattern:"
rg -n "UserConversation.*conversation_id" src/utils/ --type=py -B 3 -A 1
</function_calls>Let me verify if the file is actually src/utils/endpoints.py:
<function_calls>
#!/bin/bashFind all Python files and search for cleanup_after_streaming or the code pattern
rg -l "cleanup_after_streaming|UserConversation" src/ --type=py
</function_calls>Based on my searches, let me check if this file exists in the repository:
<function_calls>
#!/bin/bashCheck repository structure
find src/ -name "*.py" -type f | head -20
echo ""Specifically check for endpoints
find src/ -name "endpoints" -o -name "conversation" 2>/dev/null
</function_calls>Let me check the root repository structure:
<function_calls>
#!/bin/bashCheck if src directory exists
ls -la
echo ""List top-level structure
find . -maxdepth 2 -type d | head -20
</function_calls>Let me look for Python files more broadly:
<function_calls>
#!/bin/bashFind all Python files in repository
find . -name "*.py" -type f 2>/dev/null | head -30
</function_calls>Let me check git status to understand the repo state:
<function_calls>
#!/bin/bashGet repo info
git status
echo ""
git log --oneline | head -5
</function_calls><function_calls>
#!/bin/bashCheck all branches and see what's available
echo "=== Current branch ==="
git branchecho ""
echo "=== All files in repo ==="
git ls-files | head -30echo ""
echo "=== Search for endpoints.py ==="
git ls-files | grep -i endpoint
</function_calls>Let me search for conversation-related files:
<function_calls>
#!/bin/bashSearch for conversation-related files
git ls-files | grep -i conversation
echo ""
echo "=== Search for UserConversation ==="
rg -l "UserConversation" . --type=py 2>/dev/null | head -10
</function_calls>Let me check if there's documentation about the structure:
<function_calls>
#!/bin/bashLook for README or docs
ls -la *.md 2>/dev/null
echo ""Check directory structure
find . -maxdepth 3 -type f -name "*.py" | head -40
</function_calls>Let me search for the cleanup_after_streaming term across all file types:
<function_calls>
#!/bin/bashSearch everywhere for cleanup_after_streaming
rg "cleanup_after_streaming" . 2>/dev/null | head -20
</function_calls>Based on my extensive search, I need to verify the review comment's validity. Let me document my findings:
Code snippet from review comment does not exist in current repository state.
After extensive searching, I cannot locate
src/utils/endpoints.py, thecleanup_after_streamingfunction, or theUserConversationclass referenced in the review comment. The file path, code structure, and line numbers (670-672) do not match the current codebase.This review comment appears to reference code that either:
- Hasn't been committed to the current branch yet
- Has been removed or refactored since the comment was created
- Contains incorrect file paths or line numbers
Without access to the actual code being reviewed, I cannot verify whether the concern about normalized conversation ID parameters is valid.
src/app/endpoints/query_v2.py (2)
40-41: LGTM - Import of ID conversion utilities.The imports for
normalize_conversation_idandto_llama_stack_conversation_idare properly placed at module level, unlike the inline imports inquery.py.
360-375: LGTM - Correct use of llama-stack format for API calls.The
llama_stack_conv_id(withconv_prefix) is correctly used for theclient.responses.create()call, while logging uses the normalized format for consistency with storage.src/app/endpoints/conversations_v3.py (4)
190-249: Conversations list endpoint logic looks soundID filtering, RBAC via
authorized_actions, and error handling for unexpected DB errors are implemented cleanly; the endpoint returns typedConversationDetailsand wraps unknown failures in a 500 as expected.
252-411: GET /conversations/{id}: good normalization, access control, and backend fallbackValidation with
check_suid, normalization for DB vs llama-stack ID,can_access_conversationchecks, and layered error handling forAPIConnectionError/NotFoundError/generic exceptions are all consistent with the rest of the stack and should behave correctly.
414-543: DELETE /conversations/{id}: consistent access checks and dual delete behaviorEndpoint correctly enforces ID format, ownership via
can_access_conversation, deletes both remotely and locally, and still cleans local state on remote 404; the 503/404/500 mappings look appropriate.
546-603: I'll verify the review comment about the action permission used for updates. Let me search the codebase to understand how the "others" permissions are structured and used.
<function_calls>
#!/bin/bashSearch for all uses of Action permissions related to "OTHERS_CONVERSATIONS"
rg -n "QUERY_OTHERS_CONVERSATIONS|LIST_OTHERS_CONVERSATIONS|READ_OTHERS_CONVERSATIONS|DELETE_OTHERS_CONVERSATIONS|UPDATE_OTHERS_CONVERSATIONS" --type=py -C2
</function_calls>Now let me find where the Action enum is defined:
<function_calls>
#!/bin/bashFind the Action enum definition
rg -n "class Action|^class Action" --type=py -A 20
</function_calls>Let me also check for the broader context of how permissions are organized:
<function_calls>
#!/bin/bashFind files that define or import Action
fd -type f -name "*.py" -exec rg -l "class Action|from.*Action|import.*Action" {} ; | head -20
</function_calls>tests/unit/app/endpoints/test_query.py (1)
1938-1973: Topic-summary tests correctly exercisecontent_to_strintegrationPatching
app.endpoints.query.content_to_strand asserting onUserMessageconstruction and created turns ensures the new content-flattening path is covered without coupling to the underlying llama-stack types; this looks correct and matches the new helper.src/utils/types.py (2)
20-39:content_to_strcovers the key content shapes used in tool responsesThe helper handles
None, plain strings,TextContentItem,ImageContentItem, and nested lists recursively, which matches how agent/tool responses are structured and keepsTurnSummarylogic simple.
125-145: TurnSummary now safely normalizes tool response contentUsing
content_to_str(resp.content)when buildingToolCallSummarykeepsrag_chunksextraction robust across the different content shapes the client may emit and avoids ad-hoc type checks here.tests/unit/app/endpoints/test_streaming_query_v2.py (2)
30-66: RAG/MCP tool wiring tests correctly account for Conversations API usageBy mocking
client.conversations.createand still asserting onresponses.createparameters and tool configuration, these tests validate the new conversation-creation step without over-coupling to the conversation ID value.
98-207: Streaming v2 handler correctly propagates normalizedconversation_idinto SSE and cleanupStubbing
retrieve_responseto return"abc123def456"and asserting thatstream_start_eventandcleanup_after_streamingboth see exactly that ID demonstrates that the newResponseGeneratorContext/generator changes are wired as intended.tests/unit/app/endpoints/test_streaming_query.py (2)
46-144: Local mock Agent API types are a pragmatic way to decouple tests from SDK internalsDefining lightweight
TextDelta,ToolCallDelta,TurnResponseEvent,AgentTurnResponseStreamChunk, and related payload/step classes inside the test module keeps streaming tests stable across llama-stack-client changes while preserving the attribute surface the app code expects.
335-365: Tool-call argument expectations updated to match stringified payloadsSwitching
ToolCall.argumentsand the corresponding SSE assertions to use"{}"(and JSON-like strings) aligns the tests with the newer streaming payload shape where tool-call arguments arrive as strings instead of dicts; this is consistent across_test_streaming_query_endpoint_handlerandtest_stream_build_event_step_complete.Also applies to: 1070-1144
src/app/endpoints/streaming_query_v2.py (2)
118-313: Response generator now cleanly owns start-event emission using contextconversation_idMoving the
stream_start_eventemission to the top of the loop and ignoringresponse.createdfor ID discovery simplifies the control flow and ensures the same ID is used for SSE andcleanup_after_streaming; this matches the updated tests and keeps the generator focused on content events.
349-449: I'm unable to access the repository directly, but I can analyze the provided code snippet and the review concern. Let me examine what's evident from the code:Looking at the
retrieve_responsefunction in the provided snippet (lines 349-449):
When
conversation_idis provided (lines 378-380):
- Converts to llama-stack format:
llama_stack_conv_id = to_llama_stack_conversation_id(conversation_id)- Returns the original
conversation_idstring (line 442)When no
conversation_idis provided (lines 381-394):
- Creates new conversation and normalizes it:
conversation_id = normalize_conversation_id(llama_stack_conv_id)(line 395)- Returns the normalized
conversation_idstring (line 442)The review comment's concern is valid and verifiable from the code itself: the function returns inconsistent types of conversation IDs depending on which code path is taken. This creates a downstream inconsistency where:
- Caller-supplied IDs are returned as-is (potentially non-normalized)
- System-created IDs are returned normalized
Since I cannot verify the full upstream call chain without repository access, I must mark this for manual verification while preserving the issue identified in the review comment.
Inconsistent conversation ID normalization between code paths creates downstream consistency issues.
The function returns non-normalized IDs for caller-supplied conversation IDs (line 380 path) but normalized IDs for system-created ones (line 395 path). This violates the stated goal that "everything (cleanup, v3 endpoints, tests) consistently work with the normalized form." Either:
- Normalize caller-supplied IDs immediately (before line 442 return), or
- Document that callers must normalize IDs before passing them and handle both forms downstream
Verify that all upstream callers normalize conversation IDs before calling
retrieve_response, or else standardize normalization within the function itself.
|
LGTM |
luis5tb
force-pushed
the
conversations
branch
from
3fba6d0 to
aede4eb
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
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_v2.py (1)
356-378: Update function docstring to document conversation creation.The docstring doesn't mention that
retrieve_responsenow creates a new conversation whenconversation_idis not provided. Please add this behavior to the docstring.Apply this diff to update the docstring:
""" Retrieve response from LLMs and agents. Asynchronously retrieves a streaming response and conversation ID from the Llama Stack agent for a given user query. This function configures shields, system prompt, and tool usage based on the request and environment. It prepares the agent with appropriate headers and toolgroups, validates attachments if present, and initiates a streaming turn with the user's query - and any provided documents. + and any provided documents. If no conversation_id is provided in + the query_request, a new conversation is created automatically. Parameters: model_id (str): Identifier of the model to use for the query. query_request (QueryRequest): The user's query and associated metadata. token (str): Authentication token for downstream services. mcp_headers (dict[str, dict[str, str]], optional): Multi-cluster proxy headers for tool integrations. Returns: - tuple: A tuple containing the streaming response object - and the conversation ID. + tuple: A tuple containing the streaming response object and the + normalized conversation ID (either from query_request or newly created). """
♻️ Duplicate comments (2)
src/app/endpoints/query.py (1)
17-25: Alpha module imports require verification.The imports use
llama_stack_client.types.alpha.*paths which were flagged in a previous review. Verify these paths are correct for llama-stack-client 0.3.0.src/app/endpoints/streaming_query.py (1)
20-23: Alpha module imports require verification.Same issue as in
query.py- the imports usellama_stack_client.types.alpha.*paths.
🧹 Nitpick comments (8)
src/utils/types.py (1)
63-79: Type migration to Agent-prefixed types looks correct.The signature update to
AgentCompletionMessageandlist[AgentToolCall]aligns with the broader type migration in this PR. The docstring at line 67 still references "CompletionMessage" without the Agent prefix - consider updating for consistency.""" - Return the `tool_calls` list from a CompletionMessage, or an empty list if none are present. + Return the `tool_calls` list from an AgentCompletionMessage, or an empty list if none are present. Parameters: output_message (AgentCompletionMessage | None): Completionsrc/app/endpoints/conversations_v3.py (3)
141-188: Consider defensive handling for malformed item structures.The function assumes items have expected structure (type, role, content fields). If llama-stack returns unexpected item formats, this could silently produce incorrect results.
for item in items: - item_type = item.get("type") - item_role = item.get("role") + item_type = item.get("type") + item_role = item.get("role") + + if not item_type: + logger.debug("Skipping item without type: %s", item) + continue # Handle message items if item_type == "message":
192-195: Use specific type annotation for auth parameter.Per coding guidelines, all functions must include complete type annotations. The
authparameter should useAuthTupleinstead ofAny.+from authentication.interface import AuthTuple + async def get_conversations_list_endpoint_handler( request: Request, - auth: Any = Depends(get_auth_dependency()), + auth: AuthTuple = Depends(get_auth_dependency()), ) -> ConversationsListResponse:This applies to all four endpoint handlers in this file (lines 194, 257, 421, 552).
344-350: Consider pagination or configurable limit for conversation items.The hardcoded
limit=1000may be insufficient for long conversations or excessive for typical use cases. Consider:
- Making this configurable via constants or config
- Implementing pagination in the response
docs/conversations_api.md (2)
58-66: Add language specifiers to fenced code blocks.Several code blocks lack language specifiers, which affects syntax highlighting and accessibility. The linter flagged blocks at lines 58, 63, 77, 234, 369, and 469.
Apply this diff to add language specifiers:
-``` +```text conv_<48-character-hex-string>Example:
-+text
conv_0d21ba731f21f798dc9680125d5d6f493e4a7ab79f25670eSimilarly for the normalized format example at line 77, and error message blocks at lines 469.
14-27: Adjust nested list indentation to 2 spaces.The Table of Contents uses 3-space indentation for nested items, but the linter expects 2 spaces per level (MD007).
* [Conversation ID Formats](#conversation-id-formats) - * [Llama Stack Format](#llama-stack-format) - * [Normalized Format](#normalized-format) - * [ID Conversion Utilities](#id-conversion-utilities) + * [Llama Stack Format](#llama-stack-format) + * [Normalized Format](#normalized-format) + * [ID Conversion Utilities](#id-conversion-utilities)Apply similar fixes to other nested items (lines 20-22, 24-27).
src/app/endpoints/query_v2.py (2)
348-350: Consider handlingAPIConnectionErrorexplicitly.The broad
except Exceptioncatches all errors includingAPIConnectionErrorfrom Llama Stack. Per coding guidelines, integration code should handleAPIConnectionErrorexplicitly for better error messaging.- except Exception as e: # pylint: disable=broad-exception-caught + except APIConnectionError as e: + logger.error("Failed to connect to Llama Stack when creating conversation: %s", e) + raise + except Exception as e: # pylint: disable=broad-exception-caught logger.error("Failed to create conversation: %s", e) raiseYou'll need to add
APIConnectionErrorto the imports fromllama_stack_client.
416-424: Unnecessary conditional check.At line 420,
conversation_idis guaranteed to be non-None at this point because:
- If provided, it was set at line 330
- If not provided, it was set at line 342 after successful conversation creation
The conditional
if conversation_id else conversation_idis redundant.- # Normalize conversation ID before returning (remove conv_ prefix for consistency) - normalized_conversation_id = ( - normalize_conversation_id(conversation_id) - if conversation_id - else conversation_id - ) + # Normalize conversation ID before returning (remove conv_ prefix for consistency) + normalized_conversation_id = normalize_conversation_id(conversation_id) return (summary, normalized_conversation_id, referenced_documents, token_usage)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
docs/conversations_api.md(1 hunks)pyproject.toml(2 hunks)src/app/endpoints/conversations_v3.py(1 hunks)src/app/endpoints/query.py(6 hunks)src/app/endpoints/query_v2.py(6 hunks)src/app/endpoints/streaming_query.py(6 hunks)src/app/endpoints/streaming_query_v2.py(6 hunks)src/app/routers.py(2 hunks)src/constants.py(1 hunks)src/metrics/utils.py(1 hunks)src/models/requests.py(1 hunks)src/utils/endpoints.py(3 hunks)src/utils/suid.py(1 hunks)src/utils/token_counter.py(1 hunks)src/utils/types.py(3 hunks)tests/configuration/minimal-stack.yaml(1 hunks)tests/integration/endpoints/test_query_v2_integration.py(2 hunks)tests/unit/app/endpoints/test_query.py(7 hunks)tests/unit/app/endpoints/test_query_v2.py(22 hunks)tests/unit/app/endpoints/test_streaming_query.py(6 hunks)tests/unit/app/endpoints/test_streaming_query_v2.py(7 hunks)tests/unit/app/test_routers.py(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- tests/configuration/minimal-stack.yaml
- tests/unit/app/endpoints/test_query_v2.py
- src/models/requests.py
- src/constants.py
- src/metrics/utils.py
- tests/unit/app/endpoints/test_streaming_query_v2.py
- src/utils/endpoints.py
- tests/unit/app/endpoints/test_query.py
- pyproject.toml
- src/utils/token_counter.py
🧰 Additional context used
📓 Path-based instructions (5)
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: Use absolute imports for internal modules in LCS project (e.g.,from auth import get_auth_dependency)
All modules must start with descriptive docstrings explaining their purpose
Uselogger = logging.getLogger(__name__)pattern for module logging
All functions must include complete type annotations for parameters and return types, using modern syntax (str | int) andOptional[Type]orType | None
All functions must have docstrings with brief descriptions following Google Python docstring conventions
Function names must use snake_case with descriptive, action-oriented names (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead of modifying input parameters
Useasync deffor I/O operations and external API calls
All classes must include descriptive docstrings explaining their purpose following Google Python docstring conventions
Class names must use PascalCase with descriptive names and standard suffixes:Configurationfor config classes,Error/Exceptionfor exceptions,Resolverfor strategy patterns,Interfacefor abstract base classes
Abstract classes must use ABC with@abstractmethoddecorators
Include complete type annotations for all class attributes in Python classes
Useimport loggingand module logger pattern with standard log levels: debug, info, warning, error
Files:
src/utils/types.pysrc/utils/suid.pysrc/app/endpoints/query_v2.pysrc/app/endpoints/streaming_query.pysrc/app/routers.pysrc/app/endpoints/query.pysrc/app/endpoints/streaming_query_v2.pysrc/app/endpoints/conversations_v3.py
src/app/endpoints/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use FastAPI
HTTPExceptionwith appropriate status codes for API endpoint error handling
Files:
src/app/endpoints/query_v2.pysrc/app/endpoints/streaming_query.pysrc/app/endpoints/query.pysrc/app/endpoints/streaming_query_v2.pysrc/app/endpoints/conversations_v3.py
src/**/{client,app/endpoints/**}.py
📄 CodeRabbit inference engine (CLAUDE.md)
Handle
APIConnectionErrorfrom Llama Stack in integration code
Files:
src/app/endpoints/query_v2.pysrc/app/endpoints/streaming_query.pysrc/app/endpoints/query.pysrc/app/endpoints/streaming_query_v2.pysrc/app/endpoints/conversations_v3.py
tests/{unit,integration}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/{unit,integration}/**/*.py: Use pytest for all unit and integration tests; do not use unittest framework
Unit tests must achieve 60% code coverage; integration tests must achieve 10% coverage
Files:
tests/integration/endpoints/test_query_v2_integration.pytests/unit/app/endpoints/test_streaming_query.pytests/unit/app/test_routers.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use
pytest-mockwith AsyncMock objects for mocking in tests
Files:
tests/integration/endpoints/test_query_v2_integration.pytests/unit/app/endpoints/test_streaming_query.pytests/unit/app/test_routers.py
🧠 Learnings (2)
📚 Learning: 2025-11-24T16:58:04.410Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:58:04.410Z
Learning: Applies to src/**/{client,app/endpoints/**}.py : Handle `APIConnectionError` from Llama Stack in integration code
Applied to files:
src/app/endpoints/streaming_query.pysrc/app/endpoints/query.py
📚 Learning: 2025-08-06T06:02:21.060Z
Learnt from: eranco74
Repo: lightspeed-core/lightspeed-stack PR: 348
File: src/utils/endpoints.py:91-94
Timestamp: 2025-08-06T06:02:21.060Z
Learning: The direct assignment to `agent._agent_id` in `src/utils/endpoints.py` is a necessary workaround for the missing agent rehydration feature in the LLS client SDK. This allows preserving conversation IDs when handling existing agents.
Applied to files:
src/app/endpoints/conversations_v3.py
🧬 Code graph analysis (4)
src/utils/types.py (1)
src/models/responses.py (2)
ToolCall(33-38)RAGChunk(25-30)
src/app/endpoints/streaming_query.py (1)
src/utils/types.py (2)
TurnSummary(118-190)content_to_str(20-39)
src/app/routers.py (1)
tests/unit/app/test_routers.py (1)
include_router(39-54)
src/app/endpoints/streaming_query_v2.py (2)
src/utils/suid.py (2)
normalize_conversation_id(101-122)to_llama_stack_conversation_id(125-145)src/app/endpoints/streaming_query.py (1)
stream_start_event(132-153)
🪛 markdownlint-cli2 (0.18.1)
docs/conversations_api.md
16-16: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
17-17: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
18-18: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
20-20: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
21-21: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
22-22: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
24-24: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
25-25: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
26-26: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
27-27: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
58-58: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
63-63: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
77-77: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
234-234: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
369-369: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
469-469: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
- GitHub Check: build-pr
- GitHub Check: e2e_tests (ci)
- GitHub Check: e2e_tests (azure)
🔇 Additional comments (24)
src/utils/types.py (1)
20-39: Well-structured utility function with comprehensive type handling.The
content_to_strfunction properly handles multiple content types with clear fallback logic. The recursive list handling and None-safety are good defensive patterns.One minor consideration: the
Anytype forcontentis appropriate here given the heterogeneous input types, but consider adding a type alias or Union type for documentation purposes if this becomes a frequently used API.src/app/endpoints/conversations_v3.py (2)
1-42: Module structure follows coding guidelines well.Good practices observed:
- Descriptive module docstring
- Logger uses
logging.getLoggerpattern- Imports are organized (stdlib, third-party, local)
- Router properly tagged
584-590: Incorrect authorization action for update endpoint.The authorization check uses
Action.QUERY_OTHERS_CONVERSATIONSbut this is an update operation. Verify that the correct action exists (e.g.,Action.UPDATE_OTHERS_CONVERSATIONS) and update accordingly for consistency with the endpoint's purpose.if not can_access_conversation( normalized_conv_id, user_id, others_allowed=( - Action.QUERY_OTHERS_CONVERSATIONS in request.state.authorized_actions + Action.UPDATE_OTHERS_CONVERSATIONS in request.state.authorized_actions ), ):tests/integration/endpoints/test_query_v2_integration.py (2)
84-89: Good mock setup for conversation creation with ID normalization.The mock correctly simulates the llama-stack format (
conv_prefix + 48 hex chars) and the test assertion at lines 168-169 properly verifies the normalization behavior (prefix stripped).
168-169: Test assertion correctly verifies ID normalization behavior.The assertion and comment clearly document that the conversation ID is normalized (without the
conv_prefix) from theconversations.create()response.src/app/routers.py (1)
21-21: Clean integration of v3 conversations router.The import and router inclusion follow the established patterns for v1/v2 endpoints. Placement after
conversations_v2is logical.Also applies to: 49-49
tests/unit/app/test_routers.py (1)
71-71: Test updates correctly verify v3 router integration.All test assertions are properly updated:
- Router count increased to 19
conversations_v3.routerpresence verified/v3prefix correctly assertedAlso applies to: 88-88, 98-98, 116-117
docs/conversations_api.md (1)
1-513: Good documentation covering the Conversations API integration.The documentation thoroughly explains the dual-database storage model, ID normalization, API endpoints, and troubleshooting. The code examples align with the implementation.
src/app/endpoints/query.py (4)
103-157: Good implementation of conversation ID normalization.The normalization is applied consistently:
- Imports
normalize_conversation_idlocally (line 111)- Normalizes the ID before database operations (line 114)
- Uses
normalized_idfor both queries and inserts (lines 124, 129)- Includes helpful debug logging
This addresses the previously flagged consistency issue.
277-293: Normalization applied correctly for conversation ownership validation.The lookup now normalizes the conversation ID before querying (lines 284-286), ensuring consistency with the storage format.
225-232: Correct usage of content_to_str for topic summary extraction.The change from
interleaved_content_as_strtocontent_to_straligns with the utility function insrc/utils/types.pythat handles various content types (str, TextContentItem, ImageContentItem, list).
793-800: Correct usage of content_to_str for LLM response extraction.Consistent with the topic summary change at line 226.
src/utils/suid.py (3)
101-122: LGTM! Simple and effective ID normalization utility.The function correctly strips the
conv_prefix when present and returns the ID unchanged otherwise. The docstring with examples is helpful.
125-145: LGTM! Correct inverse operation for ID conversion.The function properly adds the
conv_prefix only when not already present, making it idempotent.
45-98: Review the extended check_suid validation logic.The validation handles three formats:
conv_prefixed IDs (lines 47-73)- Plain hex strings (lines 76-92)
- Standard UUIDs (lines 95-96)
The plain hex validation at line 76 uses
len(suid) >= 32, which is acceptable given the fallback to UUID parsing at lines 95-96. The dual-layer validation (hex format check followed by UUID validation) prevents false positives from causing errors. Consider documenting the expected length constraints if Llama Stack conversation IDs have a fixed format in future iterations.src/app/endpoints/query_v2.py (1)
328-360: Good implementation of conversation ID handling for Responses API.The logic correctly:
- Converts existing IDs to Llama Stack format (line 334)
- Creates new conversations via the API when needed (line 339)
- Stores the normalized version for internal use (line 342)
- Uses Llama Stack format for the API request (line 360)
src/app/endpoints/streaming_query.py (4)
433-445: Correct usage of content_to_str for turn complete event.The change aligns with the utility function and handles various content types properly.
600-658: Correct usage of content_to_str for tool execution events.The changes at lines 602 and 653 properly convert tool response content to strings using the new utility function.
734-737: Correct usage of content_to_str for streaming response summary.Consistent with the other changes in this file.
835-838: Verify conversation ID normalization in streaming query handler.The
streaming_query_endpoint_handler_basecallsvalidate_conversation_ownershipwith the rawquery_request.conversation_id(line 837). If this function does not handle normalization internally, users passing IDs with or without theconv_prefix could experience lookup failures. Compare the implementation here with howquery_v2.pyhandles conversation ID normalization to determine if inconsistent behavior exists.tests/unit/app/endpoints/test_streaming_query.py (2)
45-143: Mock classes for backward compatibility are acceptable.The mock classes provide a pragmatic solution for maintaining test coverage during the llama-stack-client 0.3.x upgrade. The approach is well-documented with clear comments explaining why these mocks exist.
358-359: ToolCall arguments updated to string format (consistent with 0.3.x API).The change from
arguments={}toarguments="{}"aligns with the expectation that ToolCall arguments should be a JSON string rather than a dict object. This is consistent across all test cases and matches the assertion on line 1143.Also applies to: 1083-1084, 1129-1130
src/app/endpoints/streaming_query_v2.py (2)
406-429: Conversation ID creation and normalization logic is well-implemented.The code correctly handles both scenarios:
- When a conversation_id is provided, it converts to llama-stack format
- When not provided, it creates a new conversation and normalizes the ID
The error handling appropriately logs and re-raises exceptions. The logic aligns with the normalize_conversation_id and to_llama_stack_conversation_id helper functions.
143-160: Start event emission moved to first chunk processing.The change to emit the start event on the first chunk (rather than extracting conversation_id from response.created) is correct. The conversation_id is now always available from context before streaming begins, ensuring the start event contains the proper ID.
luis5tb
force-pushed
the
conversations
branch
from
aede4eb to
fb1fed4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pyproject.toml (1)
59-69: Update k8s.py exclusion path fromsrc/authentication/k8s.pytosrc/auth/k8s.py.The excluded path in pyproject.toml does not match the documented guidance, which specifies
src/auth/k8s.pyshould be excluded from Pyright type checking. Verify the correct module location and update the exclusion accordingly. Also document the plan for addressing the Agent v1 endpoints (conversations.py, query.py, streaming_query.py, utils/endpoints.py) rather than maintaining permanent Pyright exclusions for missing client types.src/metrics/utils.py (1)
5-11: Update import to use stablellama_stack_client.types.agentsinstead of alpha path.The import should be
from llama_stack_client.types.agents import Turnper the official llama-stack-client documentation. The current pathllama_stack_client.types.alpha.agents.turnuses an unstable alpha namespace when a stable alternative is available. Update the import and remove the.alpha.agents.turnsubmodule path.
♻️ Duplicate comments (2)
src/utils/types.py (1)
6-15: Prefer publicllama_stack_client.typesexports over lib/alpha/shared internalsThis module still pulls llama-stack types from internal or unstable paths:
llama_stack_client.lib.agents.typesforCompletionMessageandToolCallllama_stack_client.types.shared.interleaved_content_itemforTextContentItemandImageContentItemllama_stack_client.types.alpha.tool_execution_stepforToolExecutionStepThe Python SDK docs and repo structure indicate these are public, stable exports:
CompletionMessage,ToolCall,TextContentItem,ImageContentItem,ToolExecutionStepfromllama_stack_client.types- Agent-specific types (e.g.
Turn) fromllama_stack_client.types.agents(llamastack.github.io)Relying on
lib.*,types.shared.*, and especiallytypes.alpha.*couples you to internal layout that may change between patch releases. To reduce breakage risk, consider:from llama_stack_client.types import ( CompletionMessage as AgentCompletionMessage, ToolCall as AgentToolCall, TextContentItem, ImageContentItem, ToolExecutionStep, )and drop the lib/alpha/shared-specific imports entirely.
It’s worth validating these public imports against your pinned
llama-stack-clientversion (e.g., in a small REPL or via your type checker) to ensure names match exactly before switching.src/app/endpoints/conversations_v3.py (1)
584-589: Incorrect authorization action for updating others' conversations.Line 588 checks for
Action.QUERY_OTHERS_CONVERSATIONSwhen determining whether the user can update another user's conversation. This is inconsistent with the pattern used in other endpoints and could allow users with query-others permission to modify conversations they should only be able to read.Following the pattern from the delete endpoint (line 458:
DELETE_OTHERS_CONVERSATIONS) and get endpoint (line 302:READ_OTHERS_CONVERSATIONS), this should check forAction.UPDATE_OTHERS_CONVERSATIONS.Apply this diff:
if not can_access_conversation( normalized_conv_id, user_id, others_allowed=( - Action.QUERY_OTHERS_CONVERSATIONS in request.state.authorized_actions + Action.UPDATE_OTHERS_CONVERSATIONS in request.state.authorized_actions ), ):
🧹 Nitpick comments (6)
tests/integration/endpoints/test_query_v2_integration.py (1)
84-88: Conversation creation mocking and normalization assertion look correct.Mocking
conversations.createto returnconv_+ 48 hex characters and asserting that the handler returns the 48‑char normalized ID matches the documented behavior and suid utilities; this is a good end‑to‑end check of the new flow.You may also want to update the mocked
VersionInfo(version="0.2.22")to"0.3.0"for consistency with the configuredMAXIMAL_SUPPORTED_LLAMA_STACK_VERSION, though functionally it still passes the current version range.Also applies to: 168-169
tests/unit/app/endpoints/test_query_v2.py (1)
108-753: Unit tests correctly exercise conversation creation + ID normalization; consider using realistic 48‑char IDs.All the updated tests that mock
conversations.createwith"conv_abc123def456"and assert a normalizedconv_id == "abc123def456"are aligned with the new Conversations/Responses flow and thenormalize_conversation_idhelper, and they give good coverage of different response shapes (tools, shields, usage variants, etc.).For extra fidelity with production (and to mirror the integration tests and docs, which use
conv_+ 48‑char hex IDs), you might consider switching the mocked IDs here to a 48‑character hex string as well; the logic will work the same, but it will better reflect real Llama Stack IDs.docs/conversations_api.md (1)
1-460: Conversations/Responses guide is accurate and very useful; just fix minor markdown lint issues.The doc clearly explains the normalized vs
conv_formats, the conversion utilities, and how v2/v3 endpoints and both databases interact; it matches the updated code and tests well.Two small markdown cleanups (per markdownlint) you may want to apply:
- Lines 16–27 (Table of Contents subitems): reduce unordered‑list indentation to 2 spaces so nested bullets render consistently.
- Several fenced blocks (e.g., the generic
conv_…example, SSE/event and error snippets) are missing a language spec; add something like```textor```jsonas appropriate to satisfy MD040 and improve highlighting.src/app/endpoints/query.py (1)
111-157: Conversation ID normalization logic looks good; consider normalizing in all DB lookupsThe new normalization logic in
persist_user_conversation_details()andquery_endpoint_handler_base()is sound:
normalize_conversation_id()is used to stripconv_for DB storage and for ownership checks.- DB writes and the ownership validator now consistently see normalized IDs.
One spot you might want to harden for future changes is the topic-summary existence check:
existing_conversation = ( session.query(UserConversation).filter_by(id=conversation_id).first() )Because
conversation_idis normalized for the Responses API path today, this works, but if the Agent path ever starts returningconv_...IDs, this check will miss rows written with normalized IDs.Consider using the normalized form here as well (or normalizing inside
UserConversationaccess helpers) to keep all DB queries conv_-agnostic.Also applies to: 278-287, 330-335
src/app/endpoints/conversations_v3.py (2)
141-141: Use specific type annotation for consistency.The parameter type
list[dict]should belist[dict[str, Any]]to match the return type annotation and maintain consistency with the rest of the codebase.Apply this diff:
-def simplify_conversation_items(items: list[dict]) -> list[dict[str, Any]]: +def simplify_conversation_items(items: list[dict[str, Any]]) -> list[dict[str, Any]]:
152-185: Consider handling edge cases in turn grouping logic.The current logic assumes a user→assistant message pattern and groups messages into turns that end when an assistant message is encountered. This may produce unexpected results if:
- The conversation starts with an assistant message (turn would contain only assistant message)
- Multiple user or assistant messages appear consecutively
- The conversation ends with a user message (added as incomplete turn)
While the current implementation works for typical flows, consider adding validation or documentation about the expected message patterns.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
docs/conversations_api.md(1 hunks)pyproject.toml(2 hunks)src/app/endpoints/conversations_v3.py(1 hunks)src/app/endpoints/query.py(6 hunks)src/app/endpoints/query_v2.py(6 hunks)src/app/endpoints/streaming_query.py(6 hunks)src/app/endpoints/streaming_query_v2.py(6 hunks)src/app/routers.py(2 hunks)src/constants.py(1 hunks)src/metrics/utils.py(1 hunks)src/models/requests.py(1 hunks)src/utils/endpoints.py(3 hunks)src/utils/suid.py(1 hunks)src/utils/token_counter.py(1 hunks)src/utils/types.py(3 hunks)tests/configuration/minimal-stack.yaml(1 hunks)tests/integration/endpoints/test_query_v2_integration.py(2 hunks)tests/unit/app/endpoints/test_query.py(7 hunks)tests/unit/app/endpoints/test_query_v2.py(22 hunks)tests/unit/app/endpoints/test_streaming_query.py(6 hunks)tests/unit/app/endpoints/test_streaming_query_v2.py(7 hunks)tests/unit/app/test_routers.py(5 hunks)tests/unit/utils/test_suid.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/models/requests.py
- tests/configuration/minimal-stack.yaml
- tests/unit/app/test_routers.py
- tests/unit/app/endpoints/test_query.py
- src/utils/endpoints.py
- src/app/routers.py
🧰 Additional context used
📓 Path-based instructions (7)
tests/{unit,integration}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/{unit,integration}/**/*.py: Use pytest for all unit and integration tests; do not use unittest framework
Unit tests must achieve 60% code coverage; integration tests must achieve 10% coverage
Files:
tests/unit/app/endpoints/test_query_v2.pytests/unit/utils/test_suid.pytests/unit/app/endpoints/test_streaming_query_v2.pytests/unit/app/endpoints/test_streaming_query.pytests/integration/endpoints/test_query_v2_integration.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use
pytest-mockwith AsyncMock objects for mocking in tests
Files:
tests/unit/app/endpoints/test_query_v2.pytests/unit/utils/test_suid.pytests/unit/app/endpoints/test_streaming_query_v2.pytests/unit/app/endpoints/test_streaming_query.pytests/integration/endpoints/test_query_v2_integration.py
pyproject.toml
📄 CodeRabbit inference engine (CLAUDE.md)
pyproject.toml: Configure pylint withsource-roots = "src"
Excludesrc/auth/k8s.pyfrom pyright type checking
Files:
pyproject.toml
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: Use absolute imports for internal modules in LCS project (e.g.,from auth import get_auth_dependency)
All modules must start with descriptive docstrings explaining their purpose
Uselogger = logging.getLogger(__name__)pattern for module logging
All functions must include complete type annotations for parameters and return types, using modern syntax (str | int) andOptional[Type]orType | None
All functions must have docstrings with brief descriptions following Google Python docstring conventions
Function names must use snake_case with descriptive, action-oriented names (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead of modifying input parameters
Useasync deffor I/O operations and external API calls
All classes must include descriptive docstrings explaining their purpose following Google Python docstring conventions
Class names must use PascalCase with descriptive names and standard suffixes:Configurationfor config classes,Error/Exceptionfor exceptions,Resolverfor strategy patterns,Interfacefor abstract base classes
Abstract classes must use ABC with@abstractmethoddecorators
Include complete type annotations for all class attributes in Python classes
Useimport loggingand module logger pattern with standard log levels: debug, info, warning, error
Files:
src/metrics/utils.pysrc/utils/types.pysrc/app/endpoints/streaming_query_v2.pysrc/utils/suid.pysrc/utils/token_counter.pysrc/constants.pysrc/app/endpoints/streaming_query.pysrc/app/endpoints/conversations_v3.pysrc/app/endpoints/query.pysrc/app/endpoints/query_v2.py
src/app/endpoints/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use FastAPI
HTTPExceptionwith appropriate status codes for API endpoint error handling
Files:
src/app/endpoints/streaming_query_v2.pysrc/app/endpoints/streaming_query.pysrc/app/endpoints/conversations_v3.pysrc/app/endpoints/query.pysrc/app/endpoints/query_v2.py
src/**/{client,app/endpoints/**}.py
📄 CodeRabbit inference engine (CLAUDE.md)
Handle
APIConnectionErrorfrom Llama Stack in integration code
Files:
src/app/endpoints/streaming_query_v2.pysrc/app/endpoints/streaming_query.pysrc/app/endpoints/conversations_v3.pysrc/app/endpoints/query.pysrc/app/endpoints/query_v2.py
src/**/constants.py
📄 CodeRabbit inference engine (CLAUDE.md)
Define shared constants in central
constants.pyfile with descriptive comments
Files:
src/constants.py
🧠 Learnings (3)
📚 Learning: 2025-11-24T16:58:04.410Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:58:04.410Z
Learning: Applies to pyproject.toml : Exclude `src/auth/k8s.py` from pyright type checking
Applied to files:
pyproject.toml
📚 Learning: 2025-11-24T16:58:04.410Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:58:04.410Z
Learning: Applies to src/**/{client,app/endpoints/**}.py : Handle `APIConnectionError` from Llama Stack in integration code
Applied to files:
src/metrics/utils.pysrc/app/endpoints/streaming_query.pysrc/app/endpoints/query.py
📚 Learning: 2025-08-06T06:02:21.060Z
Learnt from: eranco74
Repo: lightspeed-core/lightspeed-stack PR: 348
File: src/utils/endpoints.py:91-94
Timestamp: 2025-08-06T06:02:21.060Z
Learning: The direct assignment to `agent._agent_id` in `src/utils/endpoints.py` is a necessary workaround for the missing agent rehydration feature in the LLS client SDK. This allows preserving conversation IDs when handling existing agents.
Applied to files:
src/app/endpoints/conversations_v3.py
🧬 Code graph analysis (7)
tests/unit/app/endpoints/test_query_v2.py (1)
tests/unit/app/endpoints/test_streaming_query_v2.py (1)
test_retrieve_response_builds_rag_and_mcp_tools(31-65)
tests/unit/utils/test_suid.py (1)
src/utils/suid.py (1)
check_suid(19-54)
src/utils/types.py (1)
src/models/responses.py (2)
ToolCall(33-38)RAGChunk(25-30)
src/app/endpoints/streaming_query.py (1)
src/utils/types.py (2)
TurnSummary(118-190)content_to_str(20-39)
src/app/endpoints/conversations_v3.py (6)
src/app/database.py (1)
get_session(35-41)src/client.py (2)
AsyncLlamaStackClientHolder(18-55)get_client(49-55)src/models/database/conversations.py (1)
UserConversation(11-38)src/models/responses.py (5)
ForbiddenResponse(1253-1358)NotFoundResponse(1361-1416)ServiceUnavailableResponse(1655-1685)UnauthorizedResponse(1178-1250)conversation(1313-1322)src/utils/endpoints.py (4)
can_access_conversation(86-113)check_configuration_loaded(116-130)delete_conversation(33-47)retrieve_conversation(50-60)src/utils/suid.py (3)
check_suid(19-54)normalize_conversation_id(57-78)to_llama_stack_conversation_id(81-101)
src/app/endpoints/query.py (2)
src/utils/types.py (2)
TurnSummary(118-190)content_to_str(20-39)src/utils/suid.py (1)
normalize_conversation_id(57-78)
src/app/endpoints/query_v2.py (1)
src/utils/suid.py (2)
normalize_conversation_id(57-78)to_llama_stack_conversation_id(81-101)
🪛 markdownlint-cli2 (0.18.1)
docs/conversations_api.md
16-16: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
17-17: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
18-18: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
20-20: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
21-21: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
22-22: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
24-24: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
25-25: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
26-26: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
27-27: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
58-58: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
63-63: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
77-77: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
234-234: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
369-369: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
469-469: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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: build-pr
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
- GitHub Check: e2e_tests (ci)
- GitHub Check: e2e_tests (azure)
🔇 Additional comments (16)
src/constants.py (1)
3-6: Version range update is consistent; keep server/client versions in sync.Updating
MAXIMAL_SUPPORTED_LLAMA_STACK_VERSIONto"0.3.0"matches the dependency pins and the new Conversations/Responses flow; no issues from this change alone. Just ensure any version checks elsewhere (e.g., health checks, startup logs) are still in line with this upper bound and that your deployment stacks don’t exceed 0.3.x without revisiting this constraint.tests/unit/utils/test_suid.py (1)
3-77: Expandedcheck_suidtests are thorough and match implementation behavior.The new test cases cover all the important branches of
check_suid(UUID formats, 48‑char hex andconv_‑prefixed IDs, length and character validation, and non‑string inputs) and align with the current implementation insrc/utils/suid.py. This significantly strengthens confidence around conversation ID validation.pyproject.toml (1)
31-32: llama-stack 0.3.0 pin aligns with constants; verify server compatibility in environments.Bumping both
llama-stackandllama-stack-clientto0.3.0is consistent withMAXIMAL_SUPPORTED_LLAMA_STACK_VERSIONinsrc/constants.py. Ensure deployed Llama Stack instances run a compatible 0.3.x build and integration tests cover the new Responses/Conversations behavior against a real stack.Note: The sanity-check script snippet has a syntax error (extra backtick at end); fix before sharing with the team.
src/utils/token_counter.py (1)
7-11: Update Turn import path to match llama-stack-client 0.3.0 stable namespace.The
Turntype should be imported fromllama_stack_client.types.agents, not from the alpha namespace. Update the import to:from llama_stack_client.types.agents import TurnThis aligns with the official llama-stack-client 0.3.0 API and ensures consistency with the stable types module.
Likely an incorrect or invalid review comment.
src/app/endpoints/query.py (1)
202-232: Switch tocontent_to_strfor output_message handling is correctUsing
content_to_str(response.output_message.content)in bothget_topic_summary()andretrieve_response()is a solid improvement:
- Handles plain strings,
TextContentItem, images, and interleaved lists uniformly.- Avoids assumptions about the specific content type structure coming from llama-stack.
No issues here; this aligns well with the helper’s semantics.
Also applies to: 792-803
tests/unit/app/endpoints/test_streaming_query.py (2)
355-360: ToolCall.arguments now using serialized JSON strings looks consistentThe changes that set
ToolCall(..., arguments="{}")and adjust the corresponding assertions to expect"arguments": "{}"are aligned with the newer streaming payloads, where tool-call arguments arrive as serialized JSON rather than dicts.Since
stream_build_event()forwardst.argumentsverbatim into the SSE token payload, and the OLS compatibility tests assert against that exact stringified form, this update looks correct.Also applies to: 1081-1084, 1129-1144
14-20: Update alpha imports to use public API paths per official documentationWeb search of official Llama Stack documentation confirms that
Turnshould be imported fromllama_stack_client.types.agents, nottypes.alpha.agents. The current imports use unstable module paths that contradict the documented public API:
from llama_stack_client.types.alpha.agents.turn import Turn→ should befrom llama_stack_client.types.agents import Turnfrom llama_stack_client.types.alpha.shield_call_step import ShieldCallStep→ verify against pinned client version- Deep
types.shared.*imports for TextContentItem, SafetyViolation, ToolCall → confirm these match the public SDK APIVerify the pinned llama-stack-client version to ensure all five imports use stable, public API paths rather than internal/alpha module structures. If the alpha/shared paths fail at import time, this is a hard failure requiring immediate correction.
src/utils/types.py (1)
20-39:content_to_strand its use inTurnSummaryare correct and robustThe new
content_to_str()helper cleanly handles:
None→""str→ identityTextContentItem→.textImageContentItem→ placeholder"<image>"- Lists of mixed content via recursive join
- Fallback
str(content)for anything elseUsing it in
TurnSummary.append_tool_calls_from_llama()to deriveresponse_contentfromresp.contentwill make tool-call summaries much more resilient to changes in llama-stack’s content schema.Also applies to: 125-132
src/utils/suid.py (1)
19-54: Conversation ID validation and normalization match llama-stack’sconv_...formatThe updated
check_suid()and the new helpers look solid:
check_suidnow:
- Rejects non-str inputs early.
- Accepts:
conv_+ 48-char hex (OpenAI Conversations/Responses IDs).- Bare 48-char hex (llama-stack conv IDs without prefix).
- Standard UUID strings via
uuid.UUID().normalize_conversation_id()andto_llama_stack_conversation_id()provide clear, inverse conversions between DB IDs and llama-stackconv_...IDs, which matches the server’s generation pattern (conv_{24_bytes.hex()}). (git.kvant.cloud)No issues here; this is a good foundation for consistent ID handling across DB, agents, and the Conversations/Responses APIs.
Also applies to: 57-101
tests/unit/app/endpoints/test_streaming_query_v2.py (1)
40-43: Updated v2 streaming tests correctly follow the new conversation lifecycleThe changes here bring the tests in line with the new Responses/Conversations flow:
client.conversations.create()is mocked to returnid="conv_abc123def456".retrieve_responseis patched to return the normalized ID"abc123def456".- The start event expectation (
START:abc123def456\n) andcleanup_after_streamingassertions use the normalized ID, matching whatstreaming_query_v2.retrieve_response()now returns.- Additional retrieve_response tests also mock
conversations.create, ensuring all flows remain consistent with the new API.This looks coherent with the normalization helpers in
utils.suidand the production code instreaming_query_v2.py.Also applies to: 76-79, 166-169, 190-207, 254-257, 291-293
src/app/endpoints/streaming_query.py (1)
433-446:content_to_strusage in streaming handlers is consistent and robustThe switch to
content_to_strin:
_handle_turn_complete_event()(turn completion token),_handle_tool_execution_event()forquery_from_memoryand generic tool responses, andcreate_agent_response_generator()when populatingsummary.llm_response,is a clear improvement:
- It unifies handling of interleaved content (strings, TextContentItem, lists, images) coming from llama-stack.
- It avoids brittle assumptions about the exact content structure in streaming chunks.
- It aligns streaming behavior with the query-side usage of the same helper.
No functional issues here; this should make streaming output more resilient to schema variations.
Also applies to: 600-658, 731-737
src/app/endpoints/query_v2.py (1)
40-41: Responses v2 conversation ID handling aligns withconv_...semantics and DB normalizationThe new logic in
retrieve_response()cleanly separates llama-stack’s OpenAI Conversations IDs from your internal/DB IDs:
- If a
conversation_idis provided:
- It’s converted to llama-stack format with
to_llama_stack_conversation_id()(addingconv_if needed) before callingclient.responses.create(..., conversation=...).- If no
conversation_idis provided:
client.conversations.create()is used to create a real OpenAI conversation (conv_...), and the ID is normalized withnormalize_conversation_id()for downstream use.- On return, you always normalize again and expose only the prefixless ID to callers, matching
persist_user_conversation_details()and the new SUID helpers. (git.kvant.cloud)This is consistent with the server’s Conversations implementation and should avoid leaking raw
conv_...IDs into your own DB/UI while still using them correctly with the Responses API.If you haven’t already, it’s worth running a quick manual flow against a 0.3.x llama-stack instance (create query with/without conversation_id, then inspect Conversations/Responses state) to confirm the IDs line up as expected.
Also applies to: 328-361, 417-425
src/app/endpoints/streaming_query_v2.py (2)
50-56: Streaming v2 now uses normalized conversation IDs end-to-endThe streaming v2 generator and base context wiring look coherent:
conv_idis taken directly fromResponseGeneratorContext.conversation_id(set bystreaming_query_endpoint_handler_basefrom retrieve_response’s return value).- The start event is emitted once, at the top of the stream, using that conv_id.
response.createdchunks are correctly treated as metadata-only and ignored for ID purposes.Combined with the new normalization helpers and retrieve_response behavior, this guarantees:
- The SSE
startevent always exposes the normalized ID clients should use.cleanup_after_streamingreceives the same normalized ID for persistence.Also applies to: 143-161, 294-313
349-378: Streaming v2retrieve_responsecorrectly integrates Conversations API and normalizationThe updated
retrieve_response()mirrors the non-streaming Responses path:
- Lists shields and builds RAG/MCP tools via
prepare_tools_for_responses_api.- Concatenates attachments into
input_textin the same way as query_v2.- Handles conversation IDs as follows:
- If
query_request.conversation_idis provided, converts it withto_llama_stack_conversation_id()before passing toclient.responses.create.- If not, creates a new conversation via
client.conversations.create(metadata={}), then normalizes itsidusingnormalize_conversation_id()for return. (git.kvant.cloud)- Returns the streaming response and the normalized conversation_id; the response generator uses this for SSE start events and cleanup.
This is consistent with query_v2 and utils.suid, and keeps your external/DB IDs decoupled from the OpenAI-style
conv_...IDs used by llama-stack.It’s still worth a quick manual check against a running stack (start a streaming_query v2 session, inspect the Conversations table via the OpenAI API) to ensure the IDs you log and those visible in llama-stack match your expectations.
Also applies to: 406-438, 447-449
src/app/endpoints/conversations_v3.py (2)
190-249: LGTM!The list endpoint correctly implements access control by filtering conversations based on the
LIST_OTHERS_CONVERSATIONSaction, properly formats the response, and includes appropriate error handling.
252-411: LGTM!The GET conversation endpoint correctly:
- Validates and normalizes conversation IDs
- Enforces access control with
READ_OTHERS_CONVERSATIONS- Gracefully handles conversations not yet in local DB by querying llama-stack
- Uses proper error handling for connection failures, not-found cases, and general exceptions
- Simplifies the llama-stack response into a chat_history structure
Past issues with NOT_GIVEN import location have been resolved.
luis5tb
force-pushed
the
conversations
branch
from
fb1fed4 to
0fedfd7
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
src/app/routers.py (1)
21-21: Router registration is correctly implemented.The v3 conversations router is properly imported and registered with the
/v3prefix. However, as noted in the past review comment, the versioning could be confusing since the v2 query API creates conversations that are listed via v3/conversations endpoints.Also applies to: 48-49
src/app/endpoints/conversations_v3.py (1)
495-506: Potential inconsistent state if local DB delete fails after llama-stack delete.The deletion order (llama-stack first at line 495, local DB at line 500) could leave inconsistent state if
delete_conversationfails after the llama-stack deletion succeeds. The conversation would be deleted remotely but still exist locally.Consider wrapping in try-catch or reversing order (local DB first, which can be rolled back if llama-stack delete fails).
try: client = AsyncLlamaStackClientHolder().get_client() llama_stack_conv_id = to_llama_stack_conversation_id(normalized_conv_id) - await client.conversations.delete(conversation_id=llama_stack_conv_id) - logger.info("Successfully deleted conversation %s", normalized_conv_id) - - # Also delete from local database + # Delete from local database first (can rollback more easily) delete_conversation(conversation_id=normalized_conv_id) + + # Then delete from llama-stack + await client.conversations.delete(conversation_id=llama_stack_conv_id) + logger.info("Successfully deleted conversation %s", normalized_conv_id)
🧹 Nitpick comments (8)
tests/unit/app/endpoints/test_query_v2.py (1)
161-164: Consider extracting repeated mock setup into a pytest fixture.The same conversation mock setup (
mock_conversation.id = "conv_abc123def456",mock_client.conversations.create = ...) is duplicated across 11+ test functions. This could be extracted into a shared fixture to reduce boilerplate and improve maintainability.Example fixture:
@pytest.fixture def mock_client_with_conversation(mocker: MockerFixture) -> Mock: """Create a mock client with pre-configured conversation creation.""" mock_client = mocker.Mock() mock_conversation = mocker.Mock() mock_conversation.id = "conv_abc123def456" mock_client.conversations.create = mocker.AsyncMock(return_value=mock_conversation) return mock_clientAlso applies to: 233-236, 284-287, 327-330, 370-373, 404-407, 586-589, 633-636, 682-685, 733-736
docs/conversations_api.md (2)
14-30: Fix nested list indentation for markdown compliance.The Table of Contents nested list items use 3-space indentation, but the markdown linter expects 2-space indentation for consistency.
Apply this diff to fix the indentation:
* [Introduction](#introduction) * [Conversation ID Formats](#conversation-id-formats) - * [Llama Stack Format](#llama-stack-format) - * [Normalized Format](#normalized-format) - * [ID Conversion Utilities](#id-conversion-utilities) + * [Llama Stack Format](#llama-stack-format) + * [Normalized Format](#normalized-format) + * [ID Conversion Utilities](#id-conversion-utilities) * [How Conversations Work](#how-conversations-work) - * [Creating New Conversations](#creating-new-conversations) - * [Continuing Existing Conversations](#continuing-existing-conversations) - * [Conversation Storage](#conversation-storage) + * [Creating New Conversations](#creating-new-conversations) + * [Continuing Existing Conversations](#continuing-existing-conversations) + * [Conversation Storage](#conversation-storage) * [API Endpoints](#api-endpoints) - * [Query Endpoint (v2)](#query-endpoint-v2) - * [Streaming Query Endpoint (v2)](#streaming-query-endpoint-v2) - * [Conversations List Endpoint (v3)](#conversations-list-endpoint-v3) - * [Conversation Detail Endpoint (v3)](#conversation-detail-endpoint-v3) + * [Query Endpoint (v2)](#query-endpoint-v2) + * [Streaming Query Endpoint (v2)](#streaming-query-endpoint-v2) + * [Conversations List Endpoint (v3)](#conversations-list-endpoint-v3) + * [Conversation Detail Endpoint (v3)](#conversation-detail-endpoint-v3)
58-65: Add language specifiers to code blocks.Several code blocks are missing language specifiers. For plain text examples, use
textorplaintext:-``` +```text conv_<48-character-hex-string>For SSE stream examples: ```diff -``` +```text data: {"event": "start", "data": {"conversation_id": "..."}}Also applies to: 77-79, 234-244, 369-379, 469-471 </blockquote></details> <details> <summary>src/utils/endpoints.py (1)</summary><blockquote> `309-321`: **Type suppression for AsyncAgent construction.** The `# type: ignore` and `# pylint: disable` comments work around type stub mismatches in the llama-stack-client SDK. The scoped disable/enable pattern limits suppression to just the problematic lines. This is an acceptable workaround, but consider adding a brief comment explaining the SDK issue (e.g., referencing a GitHub issue) so future maintainers understand why these suppressions exist. ```diff logger.debug("Creating new agent") + # SDK type stubs don't match runtime API - see llama-stack-client-python issues # pylint: disable=unexpected-keyword-arg,no-member agent = AsyncAgent(src/app/endpoints/query_v2.py (1)
416-422: Redundant double normalization of conversation_id.At line 342,
conversation_idis already assignednormalize_conversation_id(llama_stack_conv_id). Then at line 419,normalize_conversation_id(conversation_id)is called again. While this is idempotent and safe, it's unnecessary overhead.- return ( - summary, - normalize_conversation_id(conversation_id), - referenced_documents, - token_usage, - ) + return ( + summary, + conversation_id, + referenced_documents, + token_usage, + )src/app/endpoints/query.py (1)
111-112: Move import to module level.The import
from utils.suid import normalize_conversation_idinsidepersist_user_conversation_detailsviolates the coding guideline for absolute imports at module level. This same import is also done at line 278 insidequery_endpoint_handler_base.Move the import to the top of the file with other imports:
+from utils.suid import normalize_conversation_id from utils.transcripts import store_transcript from utils.types import TurnSummary, content_to_strThen remove the function-level imports at lines 111-112 and 278-279.
src/app/endpoints/streaming_query_v2.py (1)
406-428: Consider extracting conversation handling to a shared helper.The conversation creation logic (lines 406-428) is nearly identical to
query_v2.py(lines 328-350). Consider extracting this to a shared utility function inutils/suid.pyor a new helper module to reduce duplication.Example shared helper:
async def get_or_create_conversation( client: AsyncLlamaStackClient, conversation_id: str | None, ) -> tuple[str, str]: """Get or create a conversation, returning (llama_stack_id, normalized_id).""" if conversation_id: llama_stack_conv_id = to_llama_stack_conversation_id(conversation_id) return llama_stack_conv_id, normalize_conversation_id(conversation_id) conversation = await client.conversations.create(metadata={}) llama_stack_conv_id = conversation.id normalized_id = normalize_conversation_id(llama_stack_conv_id) return llama_stack_conv_id, normalized_idsrc/app/endpoints/conversations_v3.py (1)
141-187: Consider handling edge case for system messages in chat history.The
simplify_conversation_itemsfunction only tracks "assistant" role to end turns (line 179). System messages or other roles may not be handled correctly. Consider documenting this limitation or adding handling for the "system" role.# If this is an assistant message, it marks the end of a turn - if item_role == "assistant" and current_turn["messages"]: + # Note: system messages are typically excluded from chat history + if item_role == "assistant" and current_turn["messages"]: chat_history.append(current_turn) current_turn = {"messages": []}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
docs/conversations_api.md(1 hunks)pyproject.toml(2 hunks)src/app/endpoints/conversations_v3.py(1 hunks)src/app/endpoints/query.py(6 hunks)src/app/endpoints/query_v2.py(6 hunks)src/app/endpoints/streaming_query.py(6 hunks)src/app/endpoints/streaming_query_v2.py(6 hunks)src/app/routers.py(2 hunks)src/constants.py(1 hunks)src/metrics/utils.py(1 hunks)src/models/requests.py(1 hunks)src/utils/endpoints.py(3 hunks)src/utils/suid.py(1 hunks)src/utils/token_counter.py(1 hunks)src/utils/types.py(3 hunks)tests/configuration/minimal-stack.yaml(1 hunks)tests/integration/endpoints/test_query_v2_integration.py(2 hunks)tests/unit/app/endpoints/test_query.py(7 hunks)tests/unit/app/endpoints/test_query_v2.py(22 hunks)tests/unit/app/endpoints/test_streaming_query.py(6 hunks)tests/unit/app/endpoints/test_streaming_query_v2.py(7 hunks)tests/unit/app/test_routers.py(5 hunks)tests/unit/utils/test_suid.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- tests/integration/endpoints/test_query_v2_integration.py
- src/utils/suid.py
- src/utils/token_counter.py
- tests/unit/app/endpoints/test_query.py
- tests/configuration/minimal-stack.yaml
- src/constants.py
- pyproject.toml
- src/models/requests.py
🧰 Additional context used
📓 Path-based instructions (5)
tests/{unit,integration}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/{unit,integration}/**/*.py: Use pytest for all unit and integration tests; do not use unittest framework
Unit tests must achieve 60% code coverage; integration tests must achieve 10% coverage
Files:
tests/unit/app/endpoints/test_streaming_query_v2.pytests/unit/app/test_routers.pytests/unit/utils/test_suid.pytests/unit/app/endpoints/test_streaming_query.pytests/unit/app/endpoints/test_query_v2.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use
pytest-mockwith AsyncMock objects for mocking in tests
Files:
tests/unit/app/endpoints/test_streaming_query_v2.pytests/unit/app/test_routers.pytests/unit/utils/test_suid.pytests/unit/app/endpoints/test_streaming_query.pytests/unit/app/endpoints/test_query_v2.py
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: Use absolute imports for internal modules in LCS project (e.g.,from auth import get_auth_dependency)
All modules must start with descriptive docstrings explaining their purpose
Uselogger = logging.getLogger(__name__)pattern for module logging
All functions must include complete type annotations for parameters and return types, using modern syntax (str | int) andOptional[Type]orType | None
All functions must have docstrings with brief descriptions following Google Python docstring conventions
Function names must use snake_case with descriptive, action-oriented names (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead of modifying input parameters
Useasync deffor I/O operations and external API calls
All classes must include descriptive docstrings explaining their purpose following Google Python docstring conventions
Class names must use PascalCase with descriptive names and standard suffixes:Configurationfor config classes,Error/Exceptionfor exceptions,Resolverfor strategy patterns,Interfacefor abstract base classes
Abstract classes must use ABC with@abstractmethoddecorators
Include complete type annotations for all class attributes in Python classes
Useimport loggingand module logger pattern with standard log levels: debug, info, warning, error
Files:
src/app/endpoints/streaming_query_v2.pysrc/metrics/utils.pysrc/app/endpoints/query_v2.pysrc/app/routers.pysrc/app/endpoints/streaming_query.pysrc/app/endpoints/conversations_v3.pysrc/utils/types.pysrc/app/endpoints/query.pysrc/utils/endpoints.py
src/app/endpoints/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use FastAPI
HTTPExceptionwith appropriate status codes for API endpoint error handling
Files:
src/app/endpoints/streaming_query_v2.pysrc/app/endpoints/query_v2.pysrc/app/endpoints/streaming_query.pysrc/app/endpoints/conversations_v3.pysrc/app/endpoints/query.py
src/**/{client,app/endpoints/**}.py
📄 CodeRabbit inference engine (CLAUDE.md)
Handle
APIConnectionErrorfrom Llama Stack in integration code
Files:
src/app/endpoints/streaming_query_v2.pysrc/app/endpoints/query_v2.pysrc/app/endpoints/streaming_query.pysrc/app/endpoints/conversations_v3.pysrc/app/endpoints/query.py
🧠 Learnings (2)
📚 Learning: 2025-11-24T16:58:04.410Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:58:04.410Z
Learning: Applies to src/**/{client,app/endpoints/**}.py : Handle `APIConnectionError` from Llama Stack in integration code
Applied to files:
src/metrics/utils.pysrc/app/endpoints/streaming_query.pysrc/app/endpoints/query.py
📚 Learning: 2025-08-06T06:02:21.060Z
Learnt from: eranco74
Repo: lightspeed-core/lightspeed-stack PR: 348
File: src/utils/endpoints.py:91-94
Timestamp: 2025-08-06T06:02:21.060Z
Learning: The direct assignment to `agent._agent_id` in `src/utils/endpoints.py` is a necessary workaround for the missing agent rehydration feature in the LLS client SDK. This allows preserving conversation IDs when handling existing agents.
Applied to files:
src/app/endpoints/conversations_v3.pysrc/utils/endpoints.py
🧬 Code graph analysis (8)
src/app/endpoints/streaming_query_v2.py (3)
src/utils/suid.py (2)
normalize_conversation_id(57-78)to_llama_stack_conversation_id(81-101)src/app/endpoints/streaming_query.py (2)
stream_start_event(130-151)retrieve_response(965-1086)src/app/endpoints/query_v2.py (1)
retrieve_response(265-422)
src/app/routers.py (1)
tests/unit/app/test_routers.py (1)
include_router(39-54)
tests/unit/utils/test_suid.py (1)
src/utils/suid.py (1)
check_suid(19-54)
src/app/endpoints/streaming_query.py (1)
src/utils/types.py (1)
content_to_str(20-39)
src/utils/types.py (2)
src/models/responses.py (2)
ToolCall(33-38)RAGChunk(25-30)tests/unit/app/endpoints/test_streaming_query.py (1)
ToolExecutionStep(127-132)
src/app/endpoints/query.py (3)
tests/unit/app/endpoints/test_streaming_query.py (1)
ToolExecutionStep(127-132)src/utils/types.py (1)
content_to_str(20-39)src/utils/suid.py (1)
normalize_conversation_id(57-78)
tests/unit/app/endpoints/test_query_v2.py (1)
tests/unit/app/endpoints/test_streaming_query_v2.py (1)
test_retrieve_response_builds_rag_and_mcp_tools(31-65)
src/utils/endpoints.py (2)
src/utils/types.py (2)
GraniteToolParser(60-99)get_parser(82-99)src/utils/suid.py (1)
get_suid(6-16)
🪛 markdownlint-cli2 (0.18.1)
docs/conversations_api.md
16-16: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
17-17: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
18-18: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
20-20: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
21-21: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
22-22: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
24-24: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
25-25: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
26-26: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
27-27: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
58-58: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
63-63: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
77-77: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
234-234: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
369-369: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
469-469: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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: build-pr
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
- GitHub Check: e2e_tests (ci)
- GitHub Check: e2e_tests (azure)
🔇 Additional comments (26)
tests/unit/app/endpoints/test_query_v2.py (1)
118-121: Mocking for conversation creation is properly set up.The mock correctly simulates the
conversations.createcall returning aconv_prefixed ID, and subsequent assertions verify the normalized form is returned.tests/unit/app/test_routers.py (1)
12-12: Tests correctly updated for the new v3 conversations router.The test assertions properly verify:
- Router count incremented to 19
conversations_v3.routeris included- Prefix
/v3is correctly configuredAlso applies to: 71-71, 88-88, 98-98, 116-117
docs/conversations_api.md (1)
1-9: Excellent documentation for the new Conversations API.This is a comprehensive guide covering the conversation ID formats, workflows, API endpoints, testing examples, and troubleshooting. The documentation clearly explains the dual-database storage model and ID normalization logic.
tests/unit/utils/test_suid.py (1)
1-77: Comprehensive test coverage for SUID validation utilities.The test suite covers all documented acceptance criteria from the
check_suidimplementation:
- Standard UUID format
- 48-character hex strings (llama-stack format)
conv_prefixed IDs- 32-character hex (UUID without hyphens)
- Invalid inputs (wrong lengths, non-hex chars, non-string types)
The parametrized test for invalid types is a clean approach for testing multiple edge cases.
tests/unit/app/endpoints/test_streaming_query.py (3)
45-143: Mock classes provide backward compatibility for Agent API tests.These mock classes replicate types that are no longer directly accessible in llama-stack-client 0.3.x. The
**kwargspattern offers flexibility, and the classes are clearly documented as mocks for testing purposes only.However, consider adding a brief module-level comment or docstring note explaining why these mocks exist and when they might be removed (e.g., if/when the library re-exposes these types publicly).
1143-1143: Assertion updated to match stringified arguments format.The assertion now expects
"arguments": "{}"as a string in the token payload, consistent with the ToolCall changes elsewhere in the file.
358-358: ToolCall.arguments now accepts stringified JSON in llama-stack-client 0.3.0.In v0.3.0, the ToolCall.arguments field can be sent as either a JSON string or a dict object (to prevent numeric deserialization issues). The test change from
arguments={}toarguments="{}"correctly reflects this API update and validates handling of the stringified JSON format.src/utils/types.py (2)
20-39: Well-implemented content conversion utility.The
content_to_strfunction correctly handles:
None→ empty stringstr→ passthroughTextContentItem→.textpropertyImageContentItem→ placeholder"<image>"- Lists → recursive join with spaces
- Unknown types →
str()fallbackThe recursive list handling is clean. One minor consideration: deeply nested lists could cause stack issues, but this is unlikely in practice.
63-79: Type annotations updated consistently.The
get_tool_callssignature now usesAgentCompletionMessageandlist[AgentToolCall], aligning with the aliased imports. The docstring correctly describes the parameter and return types.tests/unit/app/endpoints/test_streaming_query_v2.py (4)
40-43: Mock conversation setup for Conversations API integration.The mock correctly simulates the llama-stack Conversations API returning IDs with
conv_prefix. This is used across multiple tests consistently.
168-168: Return value uses normalized conversation ID.The
retrieve_responsemock now returns"abc123def456"(withoutconv_prefix), reflecting the normalization that strips the llama-stackconv_prefix before returning to clients. This is consistent with thenormalize_conversation_idutility.
190-190: Start event uses normalized conversation ID.The assertion correctly expects
START:abc123def456\nwithout theconv_prefix, matching the normalized ID returned byretrieve_response.
204-208: Cleanup verification uses normalized conversation ID.The cleanup function is correctly verified to receive the normalized
conversation_idwithout theconv_prefix, ensuring consistency between what's returned to the client and what's stored internally.src/utils/endpoints.py (2)
346-346: Return type ignore for tuple unpacking.The
# type: ignore[return-value]suppresses a type checker warning about the return tuple. This is likely due to the SDK's type stubs not correctly reflecting thatagent.agent_idandsession_idare strings.
382-382: Verify return order change in get_temp_agent.The function now returns
(agent, session_id, conversation_id). Confirm all callers have been updated to unpack values in this order.src/app/endpoints/streaming_query.py (4)
69-69: Import of content_to_str from utils.types.The new utility function is imported alongside
TurnSummary, consolidating content conversion logic in one place.
431-431: content_to_str replaces interleaved_content_as_str in turn complete handler.The function call
content_to_str(chunk.event.payload.turn.output_message.content)correctly converts the output message content to a string using the new utility.
598-651: Consistent use of content_to_str across tool execution handlers.The content conversion is now handled uniformly:
- Line 600:
content_to_str(r.content)for query_from_memory- Line 651:
content_to_str(r.content)for other tool responsesThis provides a single abstraction point for content-to-string conversion, making future changes to content handling easier to manage.
734-734: content_to_str in response generator for turn completion.The summary's
llm_responseis correctly populated usingcontent_to_str(p.turn.output_message.content), ensuring consistent string conversion for the final response.src/app/endpoints/query_v2.py (1)
328-350: Conversation handling logic is well-structured.The conversation creation/retrieval flow correctly:
- Converts existing IDs to llama-stack format
- Creates new conversations when needed
- Normalizes IDs for return values
- Logs appropriately at each step
src/app/endpoints/query.py (1)
225-232: LGTM - content_to_str replacement is correct.The switch from
interleaved_content_as_strtocontent_to_strproperly handles the response content conversion with appropriate null-safety checks.src/app/endpoints/streaming_query_v2.py (2)
143-163: LGTM - Streaming conversation ID handling is correct.The streaming response generator properly:
- Uses conversation_id from context (line 144)
- Emits start event only once via
start_event_emittedflag- Skips
response.createdevents after start is emitted (simplifying flow)
349-355: LGTM - Function signature and return type are correct.The
retrieve_responsefunction properly returns the streaming iterator and normalized conversation ID as documented.src/app/endpoints/conversations_v3.py (3)
190-249: LGTM - Conversations list endpoint is well-implemented.The endpoint correctly:
- Checks configuration
- Filters by user or allows all based on
LIST_OTHERS_CONVERSATIONSaction- Returns structured
ConversationDetailswith proper field mapping- Handles exceptions with appropriate logging and 500 response
631-658: LGTM - Update endpoint properly syncs remote and local state.The update flow correctly:
- Uses
client.conversations.update()(fixed per past review)- Updates local DB after successful remote update
- Returns structured response with success confirmation
584-590: Inconsistent action used for update endpoint access check.Line 588 uses
Action.QUERY_OTHERS_CONVERSATIONSfor the update endpoint. This appears inconsistent with the pattern used in other endpoints, which typically use operation-specific actions (e.g.,READ_OTHERS_CONVERSATIONSfor GET,DELETE_OTHERS_CONVERSATIONSfor DELETE). Verify whetherUPDATE_OTHERS_CONVERSATIONSexists in the Action enum and should be used instead, or ifQUERY_OTHERS_CONVERSATIONSis intentionally reused for multiple operations.
luis5tb
force-pushed
the
conversations
branch
from
0fedfd7 to
17dfa3a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/utils/types.py (1)
6-14: Switch llama_stack_client imports to stable public types module.These imports still rely on internal/alpha paths (
lib.agents.types,types.alpha,types.shared.interleaved_content_item), which are not part of the stable public API and are brittle across releases. Prefer the documentedllama_stack_client.typesexports instead.You can consolidate the imports like this:
-from llama_stack_client.lib.agents.types import ( - CompletionMessage as AgentCompletionMessage, - ToolCall as AgentToolCall, -) -from llama_stack_client.types.alpha import ToolExecutionStep -from llama_stack_client.types.shared.interleaved_content_item import ( - TextContentItem, - ImageContentItem, -) +from llama_stack_client.types import ( + CompletionMessage as AgentCompletionMessage, + ToolCall as AgentToolCall, + TextContentItem, + ImageContentItem, + ToolExecutionStep, +)In llama-stack-client Python SDK v0.3.0, what are the correct public import paths for the CompletionMessage, ToolCall, TextContentItem, ImageContentItem, and ToolExecutionStep types?src/metrics/utils.py (1)
10-10: Use stable Turn type import fromtypes.agentsinstead of alpha namespace.
Turnshould come from the stable agents types module, not thealphanamespace. Importing fromllama_stack_client.types.alpha.agentsis fragile and may break when the client evolves; the public docs point tollama_stack_client.types.agents.Suggested change:
-from llama_stack_client.types.alpha.agents import Turn +from llama_stack_client.types.agents import TurnFor llama-stack-client Python SDK v0.3.0, what is the documented public import path for the Turn type (and related agent types) — does it come from `llama_stack_client.types.agents` or an alpha namespace?src/app/endpoints/query.py (1)
12-23: Fix llama_stack_client imports to use public types instead oftypes.alphaThe imports of
ToolExecutionStepandTurnfromllama_stack_client.types.alpha/.types.alpha.agentsdon’t match the documented public API for the Python SDK. Official docs expose these types under:
ShieldCallStep,ToolExecutionStep, etc. fromllama_stack_client.typesTurn(and related) fromllama_stack_client.types.agents(llamastack.github.io)Using the
types.alphanamespace is both unstable and, on current releases, likely to fail import/type checking.Recommend updating the imports like this:
-from llama_stack_client import ( - APIConnectionError, - AsyncLlamaStackClient, # type: ignore - Document, -) -from llama_stack_client.types import Shield, UserMessage # type: ignore -from llama_stack_client.types.alpha import ToolExecutionStep -from llama_stack_client.types.alpha.agents import Turn -from llama_stack_client.types.alpha.agents.turn_create_params import ( - Toolgroup, - ToolgroupAgentToolGroupWithArgs, -) +from llama_stack_client import ( + APIConnectionError, + AsyncLlamaStackClient, # type: ignore + Document, +) +from llama_stack_client.types import ( + Shield, + ToolExecutionStep, + UserMessage, +) +from llama_stack_client.types.agents import Turn +from llama_stack_client.types.toolgroups_list_response import ToolGroup # or appropriate ToolGroup type +# Or, if the SDK provides a concrete ToolGroup TypedDict: +# from llama_stack_client.types import ToolGroupThen adjust
get_rag_toolgroupsto returnlist[ToolGroup] | Noneand construct toolgroups using that public type (or plain dicts), rather thanToolgroupAgentToolGroupWithArgs.In llama-stack-client **0.3.0**, what are the documented import paths for: - `Turn` - `ToolExecutionStep` - `ShieldCallStep` and what is the recommended ToolGroup type for toolgroups?tests/unit/app/endpoints/test_streaming_query.py (1)
6-7: Align test imports with public llama_stack_client types (avoidtypes.alpha)Tests import
ShieldCallStepandTurnfromllama_stack_client.types.alpha/.types.alpha.agents, but the documented Python SDK exposes them underllama_stack_client.typesandllama_stack_client.types.agentsrespectively. (llamastack.github.io) These alpha paths are unstable and may not exist in 0.3.0.Update the imports to match the public API:
-from llama_stack_client.types import UserMessage # type: ignore -from llama_stack_client.types.alpha import ShieldCallStep -from llama_stack_client.types.alpha.agents import Turn +from llama_stack_client.types import ShieldCallStep, UserMessage # type: ignore +from llama_stack_client.types.agents import TurnThis keeps tests aligned with the supported surface and avoids ImportError when upgrading the client.
For llama-stack-client 0.3.0, confirm the documented import paths for: - `ShieldCallStep` - `ToolExecutionStep` - `Turn` in the Python SDK reference.Also applies to: 14-17
🧹 Nitpick comments (5)
docs/conversations_api.md (1)
14-27: Fix markdownlint issues: nested list indentation and missing fenced code languages.To keep
markdownlintgreen and improve readability, adjust the TOC indentation and add languages to bare code fences. For example:- * [Conversation ID Formats](#conversation-id-formats) - * [Llama Stack Format](#llama-stack-format) - * [Normalized Format](#normalized-format) - * [ID Conversion Utilities](#id-conversion-utilities) +* [Conversation ID Formats](#conversation-id-formats) + * [Llama Stack Format](#llama-stack-format) + * [Normalized Format](#normalized-format) + * [ID Conversion Utilities](#id-conversion-utilities) @@ -``` +```text conv_<48-character-hex-string>@@
-+text
conv_0d21ba731f21f798dc9680125d5d6f493e4a7ab79f25670e@@ -``` +```text 0d21ba731f21f798dc9680125d5d6f493e4a7ab79f25670e@@
-+text
data: {"event": "start", "data": {"conversation_id": "0d21ba731f21f798dc9680125d5d6f493e4a7ab79f25670e"}}
@@
data: {"event": "end", "data": {"referenced_documents": [], "input_tokens": 150, "output_tokens": 200}}@@ -``` +```text Error: Conversation not found (HTTP 404)Apply the same `text` (or more specific `json`/`bash`/`sql`) language tags to the other untyped fences flagged by markdownlint. Also applies to: 58-66, 76-82, 234-244, 368-379, 469-471 </blockquote></details> <details> <summary>tests/unit/app/endpoints/test_query_v2.py (1)</summary><blockquote> `108-438`: **Tests correctly exercise new conversation creation and normalization.** The added `conversations.create` mocks and the expectations that `conv_id == "abc123def456"` validate the new behavior of creating a conversation when none is provided and returning the normalized ID. This gives good coverage of the Responses API + Conversations API integration from the non-streaming path. If you want closer parity with production, you could switch the fake ID to a 48-char hex payload (matching the docs), but that’s not required for test correctness. </blockquote></details> <details> <summary>src/app/endpoints/query_v2.py (1)</summary><blockquote> `40-40`: **Conversation handling with Responses API is correct; tighten docstring and typing.** The new logic that: - Converts a provided `conversation_id` via `to_llama_stack_conversation_id` for llama-stack, - Creates a conversation when none is provided, normalizing its ID with `normalize_conversation_id`, - Passes `conversation` to `client.responses.create` and returns the normalized ID, is a good fit for the documented behavior and matches the updated tests. Two small polish points: 1. **Docstring return type mismatch** The `Returns:` section still says `tuple[TurnSummary, str]` even though the function returns four values. Consider aligning it with the signature: ```diff - Returns: - tuple[TurnSummary, str]: A tuple containing a summary of the LLM or agent's response content - and the conversation ID, the list of parsed referenced documents, - and token usage information. + Returns: + tuple[TurnSummary, str, list[ReferencedDocument], TokenCounter]: A tuple containing + the response summary, the (normalized) conversation ID, the list of referenced + documents, and token usage information.
- Clarify normalized vs raw ID locally (optional)
For type-checkers and readability, you could track the normalized ID explicitly and avoid passing a possibly-None
conversation_idintonormalize_conversation_idat the end, e.g.:conversation_id = query_request.conversation_id if conversation_id: logger.debug("Using existing conversation ID: %s", conversation_id) llama_stack_conv_id = to_llama_stack_conversation_id(conversation_id) normalized_conversation_id = normalize_conversation_id(conversation_id) else: ... llama_stack_conv_id = conversation.id normalized_conversation_id = normalize_conversation_id(llama_stack_conv_id) ... return summary, normalized_conversation_id, referenced_documents, token_usageThis keeps logs, return value, and static typing all in sync without changing runtime behavior.
Also applies to: 328-351, 352-361, 370-375, 411-422
src/app/endpoints/conversations_v3.py (2)
141-187:simplify_conversation_itemsis reasonable but document role/type semanticsThe turn grouping and text extraction logic is straightforward and works for message-only histories, but you’re repurposing the Conversations API
rolefield into atypekey on each message and ignoring any non-message items (e.g., tool calls). That’s fine as long as downstream consumers treat"type"as a role (user/assistant) and don’t need tool details; consider clarifying this in the docstring if that’s the intended contract.
440-526: DELETE /conversations/{id}: remote+local deletion path is coherentThe delete endpoint:
- Validates and normalizes IDs.
- Enforces delete permissions via
DELETE_OTHERS_CONVERSATIONS.- Deletes from llama-stack first, then from the local DB (and still cleans up local state when the remote side returns NotFound).
- Returns a
ConversationDeleteResponsewithdeleted=Truein both success paths.If you ever need stronger consistency guarantees, you might consider deleting locally first or adding compensating logic when local deletion fails after a successful remote delete, but the current behavior is acceptable for typical use.
Also applies to: 531-543
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
docs/conversations_api.md(1 hunks)pyproject.toml(2 hunks)src/app/endpoints/conversations_v3.py(1 hunks)src/app/endpoints/query.py(6 hunks)src/app/endpoints/query_v2.py(6 hunks)src/app/endpoints/streaming_query_v2.py(6 hunks)src/metrics/utils.py(1 hunks)src/models/requests.py(1 hunks)src/utils/endpoints.py(3 hunks)src/utils/suid.py(1 hunks)src/utils/token_counter.py(1 hunks)src/utils/types.py(3 hunks)tests/configuration/minimal-stack.yaml(1 hunks)tests/integration/endpoints/test_query_v2_integration.py(2 hunks)tests/unit/app/endpoints/test_query.py(7 hunks)tests/unit/app/endpoints/test_query_v2.py(22 hunks)tests/unit/app/endpoints/test_streaming_query.py(6 hunks)tests/unit/app/endpoints/test_streaming_query_v2.py(7 hunks)tests/unit/app/test_routers.py(5 hunks)tests/unit/utils/test_suid.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- pyproject.toml
- tests/integration/endpoints/test_query_v2_integration.py
- tests/configuration/minimal-stack.yaml
- tests/unit/app/endpoints/test_query.py
- src/utils/token_counter.py
- src/models/requests.py
- src/utils/suid.py
🧰 Additional context used
📓 Path-based instructions (5)
tests/{unit,integration}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/{unit,integration}/**/*.py: Use pytest for all unit and integration tests; do not use unittest framework
Unit tests must achieve 60% code coverage; integration tests must achieve 10% coverage
Files:
tests/unit/app/test_routers.pytests/unit/utils/test_suid.pytests/unit/app/endpoints/test_streaming_query.pytests/unit/app/endpoints/test_streaming_query_v2.pytests/unit/app/endpoints/test_query_v2.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use
pytest-mockwith AsyncMock objects for mocking in tests
Files:
tests/unit/app/test_routers.pytests/unit/utils/test_suid.pytests/unit/app/endpoints/test_streaming_query.pytests/unit/app/endpoints/test_streaming_query_v2.pytests/unit/app/endpoints/test_query_v2.py
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: Use absolute imports for internal modules in LCS project (e.g.,from auth import get_auth_dependency)
All modules must start with descriptive docstrings explaining their purpose
Uselogger = logging.getLogger(__name__)pattern for module logging
All functions must include complete type annotations for parameters and return types, using modern syntax (str | int) andOptional[Type]orType | None
All functions must have docstrings with brief descriptions following Google Python docstring conventions
Function names must use snake_case with descriptive, action-oriented names (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead of modifying input parameters
Useasync deffor I/O operations and external API calls
All classes must include descriptive docstrings explaining their purpose following Google Python docstring conventions
Class names must use PascalCase with descriptive names and standard suffixes:Configurationfor config classes,Error/Exceptionfor exceptions,Resolverfor strategy patterns,Interfacefor abstract base classes
Abstract classes must use ABC with@abstractmethoddecorators
Include complete type annotations for all class attributes in Python classes
Useimport loggingand module logger pattern with standard log levels: debug, info, warning, error
Files:
src/utils/types.pysrc/app/endpoints/streaming_query_v2.pysrc/app/endpoints/query.pysrc/utils/endpoints.pysrc/app/endpoints/query_v2.pysrc/metrics/utils.pysrc/app/endpoints/conversations_v3.py
src/app/endpoints/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use FastAPI
HTTPExceptionwith appropriate status codes for API endpoint error handling
Files:
src/app/endpoints/streaming_query_v2.pysrc/app/endpoints/query.pysrc/app/endpoints/query_v2.pysrc/app/endpoints/conversations_v3.py
src/**/{client,app/endpoints/**}.py
📄 CodeRabbit inference engine (CLAUDE.md)
Handle
APIConnectionErrorfrom Llama Stack in integration code
Files:
src/app/endpoints/streaming_query_v2.pysrc/app/endpoints/query.pysrc/app/endpoints/query_v2.pysrc/app/endpoints/conversations_v3.py
🧠 Learnings (2)
📚 Learning: 2025-11-24T16:58:04.410Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:58:04.410Z
Learning: Applies to src/**/{client,app/endpoints/**}.py : Handle `APIConnectionError` from Llama Stack in integration code
Applied to files:
src/app/endpoints/query.pysrc/metrics/utils.py
📚 Learning: 2025-08-06T06:02:21.060Z
Learnt from: eranco74
Repo: lightspeed-core/lightspeed-stack PR: 348
File: src/utils/endpoints.py:91-94
Timestamp: 2025-08-06T06:02:21.060Z
Learning: The direct assignment to `agent._agent_id` in `src/utils/endpoints.py` is a necessary workaround for the missing agent rehydration feature in the LLS client SDK. This allows preserving conversation IDs when handling existing agents.
Applied to files:
src/utils/endpoints.pysrc/app/endpoints/conversations_v3.py
🧬 Code graph analysis (7)
src/utils/types.py (1)
src/models/responses.py (2)
ToolCall(33-38)RAGChunk(25-30)
tests/unit/utils/test_suid.py (1)
src/utils/suid.py (1)
check_suid(19-54)
src/app/endpoints/streaming_query_v2.py (4)
src/utils/suid.py (2)
normalize_conversation_id(57-78)to_llama_stack_conversation_id(81-101)src/app/endpoints/streaming_query.py (2)
stream_start_event(130-151)retrieve_response(965-1086)src/app/endpoints/query.py (1)
retrieve_response(667-826)src/app/endpoints/query_v2.py (1)
retrieve_response(265-422)
src/app/endpoints/query.py (4)
tests/unit/app/endpoints/test_streaming_query.py (1)
ToolExecutionStep(127-132)src/utils/types.py (1)
content_to_str(20-39)src/utils/suid.py (1)
normalize_conversation_id(57-78)src/utils/endpoints.py (1)
validate_conversation_ownership(63-83)
src/utils/endpoints.py (2)
src/utils/types.py (2)
GraniteToolParser(60-99)get_parser(82-99)src/utils/suid.py (1)
get_suid(6-16)
src/app/endpoints/query_v2.py (1)
src/utils/suid.py (2)
normalize_conversation_id(57-78)to_llama_stack_conversation_id(81-101)
src/app/endpoints/conversations_v3.py (7)
src/app/database.py (1)
get_session(35-41)src/authentication/__init__.py (1)
get_auth_dependency(14-60)src/client.py (2)
AsyncLlamaStackClientHolder(18-55)get_client(49-55)src/models/config.py (1)
Action(614-662)src/models/database/conversations.py (1)
UserConversation(11-38)src/utils/endpoints.py (3)
can_access_conversation(86-113)check_configuration_loaded(116-130)delete_conversation(33-47)src/utils/suid.py (3)
check_suid(19-54)normalize_conversation_id(57-78)to_llama_stack_conversation_id(81-101)
🪛 markdownlint-cli2 (0.18.1)
docs/conversations_api.md
16-16: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
17-17: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
18-18: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
20-20: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
21-21: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
22-22: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
24-24: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
25-25: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
26-26: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
27-27: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
58-58: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
63-63: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
77-77: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
234-234: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
369-369: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
469-469: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
- GitHub Check: build-pr
- GitHub Check: e2e_tests (ci)
- GitHub Check: e2e_tests (azure)
🔇 Additional comments (15)
src/utils/types.py (1)
20-39: content_to_str helper and its usage in TurnSummary look good.The new
content_to_strhelper cleanly normalizes None, strings, text/image items, and lists into a string, and the call inappend_tool_calls_from_llamakeeps the previous semantics while simplifying content handling. No changes needed here.Also applies to: 131-144
tests/unit/app/test_routers.py (1)
12-12: Router tests correctly cover newconversations_v3registration.Importing
conversations_v3, bumping the expected router count to 19, and asserting both inclusion and the/v3prefix give good coverage for the new router wiring. This should catch accidental removals or prefix changes.Also applies to: 71-71, 88-89, 98-99, 115-117
tests/unit/app/endpoints/test_query_v2.py (1)
563-755: Shield-related tests remain consistent with normalized conversation behavior.The shield availability/violation tests now also mock
conversations.createand assert the normalized conversation ID where relevant. This keeps those paths aligned with the new conversation lifecycle while still focusing on shield behavior and metrics.tests/unit/utils/test_suid.py (1)
3-76: Expandedcheck_suidtest matrix is solid and aligned with implementation.The new tests cover UUIDs, 48-char llama-style IDs (with/without
conv_), bad lengths, invalid hex, and non-string types, matching the documented behavior ofcheck_suid. This is a good level of regression protection for ID validation.tests/unit/app/endpoints/test_streaming_query_v2.py (1)
30-96: Streaming tests correctly validate normalized conversation IDs and tool/shield wiring.Mocking
conversations.createand havingretrieve_responsereturn"abc123def456"ensures the streaming handler emitsSTART:abc123def456and callscleanup_after_streamingwith the normalized ID, matching the new conversation semantics. Tool and shield configuration checks remain intact.Also applies to: 141-207, 238-308
src/utils/endpoints.py (1)
309-321: AsyncAgent construction & type-ignore pattern looks appropriateThe added pylint/typing suppressions around
AsyncAgentkwargs andinitialize()keep the code compatible with the current client SDK while preserving the existing rebinding behavior (including the_agent_idworkaround). No new correctness or safety issues introduced here. Based on learnings, the protected-attr assignment is an accepted interim workaround.Also applies to: 341-347, 367-381
src/app/endpoints/streaming_query_v2.py (1)
143-163: Start-event emission logic is soundUsing
context.conversation_idandstart_event_emittedensures a single, well-formedstartSSE with the correct conversation ID, independent of the underlying Responses API events. Cleanup uses the same ID consistently.Also applies to: 291-312
src/app/endpoints/query.py (3)
103-157: Conversation ID normalization inpersist_user_conversation_detailsis correctNormalizing
conversation_idonce vianormalize_conversation_idand consistently usingnormalized_idfor the DB key and logs fixes the earlier mismatch between stored IDs (withoutconv_) and external forms. This avoids duplicate rows and keeps DB lookups stable.
214-232: Usingcontent_to_strfor topic summaries and main responses is robustReplacing direct access to
output_message.contentwithcontent_to_str(...)in bothget_topic_summaryandretrieve_responsecorrectly handles string, text-item, image, and list content shapes, matching the updated content schema and avoiding missing/None cases.Also applies to: 792-800
277-293: Normalized conversation IDs for ownership checks resolve prior DB lookup issuesNormalizing
query_request.conversation_idbefore callingvalidate_conversation_ownershipensures you always queryUserConversationwith the stored (prefix-free) ID, regardless of whether the client sent aconv_…value or a bare UUID. This aligns the Agent/Responses/Conversations flows and fixes the earlier inconsistency.tests/unit/app/endpoints/test_streaming_query.py (2)
45-144: Local mock classes cleanly decouple tests from removed internal typesDefining lightweight mocks for the various Agent-turn payload and delta types (
TextDelta,ToolCallDelta,AgentTurnResponse*Payload,ToolExecutionStep,ToolResponse, etc.) is a practical way to keep Agent API streaming tests stable despite internal SDK churn. The mocks mirror only the attributes actually read bystream_build_event, which keeps the tests focused and resilient.
355-360: Verify ToolCall.arguments serialization matches production streaming contractThe test changes update
ToolCall.argumentsexpectations from dict format ({}) to string format ("{}"), which should reflect the new streaming payload behavior where arguments are JSON-serialized. Verify that:
- All
ToolCallinstances in this test file consistently use string-valued arguments- The production streaming formatter correctly serializes arguments as JSON strings
- No dict-valued
ToolCall.argumentsremain elsewhere in the test file (lines 355-360, 1080-1084, 1129-1144)src/app/endpoints/conversations_v3.py (3)
190-238: Conversations list handler correctly applies RBAC and surfaces metadataThe list endpoint correctly:
- Checks configuration readiness.
- Filters
UserConversationrows byuser_idunlessLIST_OTHERS_CONVERSATIONSis granted.- Maps DB rows to
ConversationDetailswith all relevant metadata.The broad
except Exceptionis acceptable here given you wrap it into a 500 with a generic “Unknown error” response.
279-377: GET /conversations/{id}: good ID validation, normalization, and backend fallbackThis handler validates IDs with
check_suid, normalizes them for DB/auth, enforces access viacan_access_conversation, and then uses the Conversations API (conversations.items.list) with the llama-stack-formatted ID. Falling back to llama-stack even when the conversation is missing locally is useful for partial-persistence scenarios, and the error mapping (400/403/404/503/500) looks solid.Also applies to: 378-411
570-579: PUT /conversations/{id}: normalization + dual update (remote & local) looks correctThe update endpoint:
- Validates/normalizes the conversation ID.
- Uses
QUERY_OTHERS_CONVERSATIONSas the cross-user guard, which is reasonable given the lack of a dedicated “update others” action.- Updates metadata via
client.conversations.update(..., metadata={"topic_summary": ...})and mirrors thetopic_summaryintoUserConversationif present.- Returns a clear
ConversationUpdateResponsepayload and maps APIConnectionError/NotFound to 503/404.The generic 500 handler with an
error_msgis consistent with the other endpoints.Also applies to: 580-658, 679-691
luis5tb
force-pushed
the
conversations
branch
from
17dfa3a to
d965ed0
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/metrics/utils.py (1)
10-10: Fix import path for Turn type — remove alpha namespace (repeated from previous review).This issue was previously flagged but not addressed. The import should use the stable path
from llama_stack_client.types.agents import Turn, not the alpha namespace. The official llama-stack-client 0.3.0 documentation specifies imports fromllama_stack_client.types.agents.Apply this diff:
-from llama_stack_client.types.alpha.agents import Turn +from llama_stack_client.types.agents import Turnsrc/utils/types.py (1)
6-14: Replace internal/alpha module imports with stable public API (repeated from previous review).This issue was previously flagged but not fully addressed. The imports reference unstable paths:
llama_stack_client.lib.agents.types(internal lib path)llama_stack_client.types.alpha(alpha/unstable namespace)llama_stack_client.types.shared.interleaved_content_item(internal shared path)Per llama-stack-client 0.3.0 documentation, these types should be imported from the stable public API:
llama_stack_client.types.Apply this diff to use stable imports:
-from llama_stack_client.lib.agents.types import ( - CompletionMessage as AgentCompletionMessage, - ToolCall as AgentToolCall, -) -from llama_stack_client.types.alpha import ToolExecutionStep -from llama_stack_client.types.shared.interleaved_content_item import ( - TextContentItem, - ImageContentItem, -) +from llama_stack_client.types import ( + CompletionMessage as AgentCompletionMessage, + ToolCall as AgentToolCall, + ToolExecutionStep, + TextContentItem, + ImageContentItem, +)src/app/endpoints/query_v2.py (1)
328-351: Normalize provided conversation_id immediately for consistency.When
query_request.conversation_idis supplied, you convert it to llama-stack format but retain the potentially-prefixed value in theconversation_idvariable throughout the function. This creates inconsistency: the variable may or may not have theconv_prefix depending on whether it was provided or created.For clarity and consistency with the streaming endpoint (see past review on streaming_query_v2.py), normalize the provided ID immediately:
# Handle conversation ID for Responses API # Create conversation upfront if not provided - conversation_id = query_request.conversation_id - if conversation_id: + raw_conversation_id = query_request.conversation_id + if raw_conversation_id: # Conversation ID was provided - convert to llama-stack format - logger.debug("Using existing conversation ID: %s", conversation_id) - llama_stack_conv_id = to_llama_stack_conversation_id(conversation_id) + logger.debug("Using existing conversation ID: %s", raw_conversation_id) + conversation_id = normalize_conversation_id(raw_conversation_id) + llama_stack_conv_id = to_llama_stack_conversation_id(conversation_id) else: # No conversation_id provided - create a new conversation first logger.debug("No conversation_id provided, creating new conversation") try: conversation = await client.conversations.create(metadata={}) llama_stack_conv_id = conversation.id - # Use the normalized version conversation_id = normalize_conversation_id(llama_stack_conv_id)This ensures
conversation_idis always normalized for logging and return, whilellama_stack_conv_idcarries the prefixed form for API calls.
🧹 Nitpick comments (3)
docs/conversations_api.md (2)
16-27: Fix list indentation for consistency.The nested list items use 3-space indentation instead of the expected 2-space indentation. While this doesn't affect functionality, it's inconsistent with markdown best practices.
Adjust indentation from 3 to 2 spaces for nested list items.
58-65: Add language specifiers to fenced code blocks.Several code blocks are missing language specifiers (lines 58, 63, 77, 234, 369), which helps with syntax highlighting and rendering. Consider adding language hints like
textorhttpwhere appropriate.Example:
-``` +```text conv_<48-character-hex-string> ```Also applies to: 234-244, 369-379
src/utils/endpoints.py (1)
309-321: Type suppressions indicate incomplete type hints in llama-stack-client 0.3.0, not API compatibility issues.Multiple
# type: ignoreannotations and pylint suppressions are used aroundAsyncAgentconstruction. Whileenable_session_persistence,input_shields, andoutput_shieldsare officially supported parameters in llama-stack-client 0.3.0 (per official Llama Stack documentation), the type hints in the library appear incomplete or misaligned, requiring these suppressions. This is a type annotation issue rather than an API compatibility concern.The separate workaround with
agent._agent_id(line 327) remains valid for conversation ID preservation until agent rehydration is properly exposed in the LLS client SDK.Also applies to: 367-376
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
docs/conversations_api.md(1 hunks)pyproject.toml(2 hunks)src/app/endpoints/conversations_v3.py(1 hunks)src/app/endpoints/query.py(6 hunks)src/app/endpoints/query_v2.py(6 hunks)src/app/endpoints/streaming_query_v2.py(6 hunks)src/metrics/utils.py(1 hunks)src/models/requests.py(1 hunks)src/utils/endpoints.py(3 hunks)src/utils/suid.py(1 hunks)src/utils/token_counter.py(1 hunks)src/utils/types.py(3 hunks)tests/configuration/minimal-stack.yaml(1 hunks)tests/integration/endpoints/test_query_v2_integration.py(2 hunks)tests/unit/app/endpoints/test_query.py(7 hunks)tests/unit/app/endpoints/test_query_v2.py(22 hunks)tests/unit/app/endpoints/test_streaming_query.py(6 hunks)tests/unit/app/endpoints/test_streaming_query_v2.py(7 hunks)tests/unit/app/test_routers.py(5 hunks)tests/unit/utils/test_suid.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- tests/configuration/minimal-stack.yaml
- src/utils/token_counter.py
- tests/unit/utils/test_suid.py
- src/utils/suid.py
- tests/unit/app/endpoints/test_streaming_query_v2.py
- tests/unit/app/endpoints/test_streaming_query.py
- src/app/endpoints/conversations_v3.py
- tests/unit/app/endpoints/test_query.py
🧰 Additional context used
📓 Path-based instructions (7)
pyproject.toml
📄 CodeRabbit inference engine (CLAUDE.md)
pyproject.toml: Configure pylint withsource-roots = "src"
Excludesrc/auth/k8s.pyfrom pyright type checking
Files:
pyproject.toml
tests/{unit,integration}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/{unit,integration}/**/*.py: Use pytest for all unit and integration tests; do not use unittest framework
Unit tests must achieve 60% code coverage; integration tests must achieve 10% coverage
Files:
tests/unit/app/endpoints/test_query_v2.pytests/integration/endpoints/test_query_v2_integration.pytests/unit/app/test_routers.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use
pytest-mockwith AsyncMock objects for mocking in tests
Files:
tests/unit/app/endpoints/test_query_v2.pytests/integration/endpoints/test_query_v2_integration.pytests/unit/app/test_routers.py
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: Use absolute imports for internal modules in LCS project (e.g.,from auth import get_auth_dependency)
All modules must start with descriptive docstrings explaining their purpose
Uselogger = logging.getLogger(__name__)pattern for module logging
All functions must include complete type annotations for parameters and return types, using modern syntax (str | int) andOptional[Type]orType | None
All functions must have docstrings with brief descriptions following Google Python docstring conventions
Function names must use snake_case with descriptive, action-oriented names (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead of modifying input parameters
Useasync deffor I/O operations and external API calls
All classes must include descriptive docstrings explaining their purpose following Google Python docstring conventions
Class names must use PascalCase with descriptive names and standard suffixes:Configurationfor config classes,Error/Exceptionfor exceptions,Resolverfor strategy patterns,Interfacefor abstract base classes
Abstract classes must use ABC with@abstractmethoddecorators
Include complete type annotations for all class attributes in Python classes
Useimport loggingand module logger pattern with standard log levels: debug, info, warning, error
Files:
src/app/endpoints/query_v2.pysrc/metrics/utils.pysrc/app/endpoints/streaming_query_v2.pysrc/utils/types.pysrc/utils/endpoints.pysrc/models/requests.pysrc/app/endpoints/query.py
src/app/endpoints/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use FastAPI
HTTPExceptionwith appropriate status codes for API endpoint error handling
Files:
src/app/endpoints/query_v2.pysrc/app/endpoints/streaming_query_v2.pysrc/app/endpoints/query.py
src/**/{client,app/endpoints/**}.py
📄 CodeRabbit inference engine (CLAUDE.md)
Handle
APIConnectionErrorfrom Llama Stack in integration code
Files:
src/app/endpoints/query_v2.pysrc/app/endpoints/streaming_query_v2.pysrc/app/endpoints/query.py
src/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/models/**/*.py: Use@field_validatorand@model_validatorfor custom validation in Pydantic models
Pydantic configuration classes must extendConfigurationBase; data models must extendBaseModel
Files:
src/models/requests.py
🧠 Learnings (3)
📚 Learning: 2025-11-24T16:58:04.410Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:58:04.410Z
Learning: Applies to pyproject.toml : Exclude `src/auth/k8s.py` from pyright type checking
Applied to files:
pyproject.toml
📚 Learning: 2025-11-24T16:58:04.410Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:58:04.410Z
Learning: Applies to src/**/{client,app/endpoints/**}.py : Handle `APIConnectionError` from Llama Stack in integration code
Applied to files:
src/metrics/utils.pysrc/app/endpoints/query.py
📚 Learning: 2025-08-06T06:02:21.060Z
Learnt from: eranco74
Repo: lightspeed-core/lightspeed-stack PR: 348
File: src/utils/endpoints.py:91-94
Timestamp: 2025-08-06T06:02:21.060Z
Learning: The direct assignment to `agent._agent_id` in `src/utils/endpoints.py` is a necessary workaround for the missing agent rehydration feature in the LLS client SDK. This allows preserving conversation IDs when handling existing agents.
Applied to files:
src/utils/endpoints.py
🧬 Code graph analysis (5)
src/app/endpoints/query_v2.py (1)
src/utils/suid.py (2)
normalize_conversation_id(57-78)to_llama_stack_conversation_id(81-101)
src/app/endpoints/streaming_query_v2.py (2)
src/utils/suid.py (2)
normalize_conversation_id(57-78)to_llama_stack_conversation_id(81-101)src/app/endpoints/streaming_query.py (1)
stream_start_event(130-151)
src/utils/types.py (2)
src/models/responses.py (2)
ToolCall(33-38)RAGChunk(25-30)tests/unit/app/endpoints/test_streaming_query.py (1)
ToolExecutionStep(127-132)
src/utils/endpoints.py (2)
src/utils/types.py (2)
GraniteToolParser(60-99)get_parser(82-99)src/utils/suid.py (1)
get_suid(6-16)
src/app/endpoints/query.py (4)
src/utils/types.py (2)
TurnSummary(118-190)content_to_str(20-39)src/utils/suid.py (1)
normalize_conversation_id(57-78)src/app/database.py (1)
get_session(35-41)src/utils/endpoints.py (1)
validate_conversation_ownership(63-83)
🪛 markdownlint-cli2 (0.18.1)
docs/conversations_api.md
16-16: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
17-17: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
18-18: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
20-20: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
21-21: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
22-22: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
24-24: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
25-25: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
26-26: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
27-27: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
58-58: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
63-63: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
77-77: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
234-234: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
369-369: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
469-469: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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). (2)
- GitHub Check: build-pr
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
🔇 Additional comments (8)
src/utils/types.py (1)
20-40: Well-implemented content conversion utility.The
content_to_strfunction provides clean handling of various content types with proper None-checking and recursive list processing. This is a good abstraction that replaces the previousinterleaved_content_as_strpattern.tests/integration/endpoints/test_query_v2_integration.py (2)
84-88: Test mocking correctly reflects llama-stack conversation creation.The mock properly returns a conversation ID in llama-stack format (
conv_+ 48 hex chars), which aligns with the documented behavior indocs/conversations_api.md.
168-169: Test expectation correctly validates ID normalization.The test properly expects the response to contain the normalized conversation ID (48 hex chars without the
conv_prefix), confirming the normalization behavior works as documented.tests/unit/app/endpoints/test_query_v2.py (1)
118-121: Test updates consistently mock and validate conversation ID normalization.All tests in this file have been correctly updated to:
- Mock
conversations.createreturning llama-stack format IDs (conv_abc123def456)- Assert that returned
conv_idvalues are normalized (abc123def456without prefix)This consistent pattern across all test cases properly validates the new conversation ID handling behavior.
Also applies to: 138-138
pyproject.toml (1)
31-32: Ensure llama-stack-client 0.3.0 migration is complete before merging.The dependency bump to 0.3.0 requires verification of:
- Tool schema migration: 0.3.0 changed tool definitions from
parametersfield to{input_schema, output_schema}(JSON Schema format) — confirm all tool definitions are updated- Alpha import paths: Verify no remaining imports from
types.alpha.*paths; should use stabletypes.*paths per official documentation- Pyright exclusions (lines 64-69): Review for ongoing type compatibility issues with Agent API v1 endpoints
- k8s.py exclusion: Verify
src/auth/k8s.pyis excluded from pyright type checking per project configuration requirementsManual verification needed: Search codebase for old tool
parametersfield patterns,llama_stack_client.types.alphaimports, andllama_stack_client.libimports to confirm migration completion.src/app/endpoints/query.py (2)
111-158: Good normalization implementation.The conversation ID normalization logic correctly strips the
conv_prefix before database operations and uses the normalized ID consistently throughout the function. The comprehensive logging helps with debugging.
278-289: Correct normalization before database lookup.The code properly normalizes the conversation ID before passing it to
validate_conversation_ownership(), ensuring consistent database queries. This addresses the past review comment's concern about lookup mismatches.tests/unit/app/test_routers.py (1)
12-12: LGTM - Test coverage for new v3 router.The test updates correctly reflect the addition of the conversations_v3 router: imports are added, router count is incremented from 18 to 19, and both presence and prefix (/v3) assertions are included.
Also applies to: 71-71, 88-88, 98-98, 116-117
src/app/endpoints/query.py
Outdated
| from llama_stack_client.types.alpha import ToolExecutionStep | ||
| from llama_stack_client.types.alpha.agents import Turn | ||
| from llama_stack_client.types.alpha.agents.turn_create_params import ( | ||
| Toolgroup, | ||
| ToolgroupAgentToolGroupWithArgs, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check installed version and test imports
python3 << 'EOF'
import sys
try:
import llama_stack_client
print(f"llama_stack_client version: {llama_stack_client.__version__ if hasattr(llama_stack_client, '__version__') else 'unknown'}")
except ImportError:
print("llama_stack_client not installed")
sys.exit(1)
print("\nTesting imports from review comment:")
try:
from llama_stack_client.types.alpha import ToolExecutionStep
print("✓ ToolExecutionStep import successful")
except ImportError as e:
print(f"✗ ToolExecutionStep import failed: {e}")
try:
from llama_stack_client.types.alpha.agents import Turn
print("✓ Turn import successful")
except ImportError as e:
print(f"✗ Turn import failed: {e}")
try:
from llama_stack_client.types.alpha.agents.turn_create_params import Toolgroup, ToolgroupAgentToolGroupWithArgs
print("✓ Toolgroup/ToolgroupAgentToolGroupWithArgs import successful")
except ImportError as e:
print(f"✗ Toolgroup/ToolgroupAgentToolGroupWithArgs import failed: {e}")
EOFRepository: lightspeed-core/lightspeed-stack
Length of output: 208
🌐 Web query:
llama-stack-client 0.3.0 types.alpha imports documentation
💡 Result:
Short answer: In the v0.3.0 family the client split stable vs experimental types into an "alpha" submodule — use the package's alpha subpackage (e.g. for Python) and the normal types entrypoint for stable types.
-
Python (experimental/alpha types): import from llama_stack_client.types.alpha. Example: from llama_stack_client.types.alpha import . [1][2]
-
Docs & repo (notes about /v1alpha/ and moving alpha imports): see the v0.3.0 release notes and the Python SDK reference. [3][1]
If you want I can fetch the exact symbol names exported from types.alpha in v0.3.0 (or show example import lines) — tell me which language (Python or TypeScript) and I’ll list the concrete imports.
Sources:
- Llama Stack client TypeScript repo (README / api docs). [GitHub]. [turn3view0]
- Python SDK reference (llama-stack docs showing types and submodules). [turn2search0]
- Llama Stack v0.3.0 release notes (mentions experimental /v1alpha/ and docs overhaul). [turn1search2]
🌐 Web query:
llama-stack-client 0.3.0 ToolExecutionStep Turn Toolgroup ToolgroupAgentToolGroupWithArgs types.alpha
💡 Result:
You can get those agent / tool types from the llama-stack-client Python package and import them from the types module. Example import lines:
- from llama_stack_client.types import ToolExecutionStep, ToolResponse, Turn, ToolGroup.
- Agent-tool helper wrapper (AgentToolGroupWithArgs) is used by the agent runtime code (seen in Llama Stack sources) when passing toolgroups-with-args into a turn.
References:
- Package / releases (llama-stack-client on PyPI). [1]
- Python SDK / Agents & Types reference (shows ToolExecutionStep, Turn, ToolGroup, etc.). [2]
If you want, I can fetch the exact type signatures for version 0.3.0 (or whichever exact release you need).
🌐 Web query:
llama-stack-client 0.3.0 turn_create_params ToolgroupAgentToolGroupWithArgs
💡 Result:
You can pass an AgentToolGroupWithArgs when creating an agent turn by including a toolgroup entry in the turn create params. In the Python SDK (llama-stack-client v0.3.x) toolgroup entries are plain dicts/TypedDicts; the agent SDK will accept an "agent" toolgroup with a nested "args" map (same shape as the AgentToolGroupWithArgs used in other SDKs).
Example (Python, minimal):
from llama_stack_client import LlamaStackClient
client = LlamaStackClient()
turn = client.agents.turn.create(
session_id="SESSION_ID",
agent_id="AGENT_ID",
messages=[{"role": "user", "content": "Search for X"}],
toolgroups=[
{
"type": "agent", # agent toolgroup
"name": "builtin::rag/knowledge_search",
"args": { # AgentToolGroupWithArgs args dict
"vector_db_id": "my-vdb",
"maxNeighborCount": 3,
"ragInstruction": "Use retrieved context: RETRIEVED_CONTEXT",
"ragUserPromptEmbedded": ""
}
}
],
)
Notes
- The Python client types accept nested TypedDicts for toolgroups; undocumented/extra fields are allowed and sent through as-is. [1][2][3]
- The Kotlin/Java SDK shows the same pattern (AgentToolGroupWithArgs → args map) if you need a reference. [2]
Sources:
- Python SDK reference / Agents (llama-stack docs). [1]
- Kotlin example showing AgentToolGroupWithArgs and args usage. [2]
- Agents guide (turns, toolgroups). [3]
🏁 Script executed:
#!/bin/bash
# Check the actual query.py file to see how these imports are used
git ls-files | grep -E "query\.py$" | head -5Repository: lightspeed-core/lightspeed-stack
Length of output: 208
Update imports to use public API paths instead of types.alpha.
In llama-stack-client 0.3.0, ToolExecutionStep and Turn are available in the public types module and should not be imported from types.alpha. Additionally, Toolgroup and ToolgroupAgentToolGroupWithArgs from turn_create_params are internal types; turn create operations use plain dictionaries for toolgroups instead.
Update these imports:
from llama_stack_client.types.alpha import ToolExecutionStep→from llama_stack_client.types import ToolExecutionStepfrom llama_stack_client.types.alpha.agents import Turn→from llama_stack_client.types import Turn- Remove the import from
turn_create_params; use dictionaries for turn creation if needed.
🤖 Prompt for AI Agents
In src/app/endpoints/query.py around lines 18 to 23, the imports reference
internal alpha paths and an internal turn_create_params type: replace `from
llama_stack_client.types.alpha import ToolExecutionStep` with `from
llama_stack_client.types import ToolExecutionStep`, replace `from
llama_stack_client.types.alpha.agents import Turn` with `from
llama_stack_client.types import Turn`, and remove the import of Toolgroup and
ToolgroupAgentToolGroupWithArgs from turn_create_params entirely; if you need
toolgroup data for turn creation, represent it as plain dicts per the public API
instead of importing internal types.
src/models/requests.py
Outdated
|
|
||
| from pydantic import BaseModel, model_validator, field_validator, Field | ||
| from llama_stack_client.types.agents.turn_create_params import Document | ||
| from llama_stack_client import Document |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
llama-stack-client 0.3.0 Document import public API
💡 Result:
Do you mean the public client API (methods/params) for importing documents in llama-stack-client v0.3.0 (Python or TypeScript)? Or do you want example code/snippets showing how to call the document-import endpoints?
🌐 Web query:
llama-stack-client PyPI package Document class
💡 Result:
The package exposes a Document type you import from llama_stack_client.types:
- Import: from llama_stack_client.types import Document. [1]
Common fields (examples from the docs / examples):
- document_id (string)
- content (string or structured object / URI / binary wrapper)
- mime_type
- metadata (dict)
Example usage (creating Documents for RAG insertion):
- Document(document_id="num-0", content="https://.../file", mime_type="text/plain", metadata={}). [2][3]
References:
- Python SDK reference (shows Document in shared types). [1]
- Llama Stack tutorial / examples (Document constructor example). [2]
- RAG docs (RAGDocument examples showing content shapes). [3]
Fix the Document import path to use the correct module location.
The import from llama_stack_client import Document is incorrect. According to the llama-stack-client package structure, Document must be imported from llama_stack_client.types:
from llama_stack_client.types import DocumentThe current import will fail because Document is not exported at the top level of the llama_stack_client package.
🤖 Prompt for AI Agents
In src/models/requests.py at line 7, the import currently uses the wrong module
path; replace the top-level import with the correct path by importing Document
from llama_stack_client.types (i.e., change the import to use
llama_stack_client.types.Document) so the module resolves properly and the class
is available.
|
Overall LGTM |
luis5tb
force-pushed
the
conversations
branch
from
d965ed0 to
98b18ff
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/app/endpoints/query.py (1)
276-295: Normalization before ownership validation is good; double‑check topic‑summary DB lookup still using raw IDsNormalizing
query_request.conversation_idbefore callingvalidate_conversation_ownershipis the right direction and should resolve the earlier inconsistency where DB rows were stored without theconv_prefix but queried with the prefixed value.However, a few lines later, the topic‑summary existence check still uses
conversation_idfromretrieve_response_funcdirectly:with get_session() as session: existing_conversation = ( session.query(UserConversation).filter_by(id=conversation_id).first() )Given
persist_user_conversation_detailsnow always storesUserConversation.idasnormalize_conversation_id(conversation_id), this lookup may miss existing rows on the Agent API path if thatconversation_idstill containsconv_.... In that case you’ll recompute a topic summary on every request but never seeexisting_conversation, and the computed summary isn’t persisted for updates anyway.Consider normalizing here as well, or ensuring that all
retrieve_response_funcimplementations (Agent and Responses) return a normalized conversation ID so every DB access uses the same form:- with get_session() as session: - existing_conversation = ( - session.query(UserConversation).filter_by(id=conversation_id).first() - ) + from utils.suid import normalize_conversation_id + normalized_conv_id = normalize_conversation_id(conversation_id) + with get_session() as session: + existing_conversation = ( + session.query(UserConversation) + .filter_by(id=normalized_conv_id) + .first() + )This would fully align the DB read/write path with your normalization strategy.
🧹 Nitpick comments (2)
src/app/endpoints/query.py (1)
104-159: Conversation persistence now uses normalized IDs; consider avoiding duplicated in-function importsThe new normalization flow in
persist_user_conversation_detailslooks correct: you normalize once tonormalized_id, use it consistently forUserConversation.idand DB lookups, and log both original and normalized IDs. This should prevent future mismatches betweenconv_...values at the API boundary and stored IDs.One small cleanup: you now import
normalize_conversation_idtwice inside functions (here and later inquery_endpoint_handler_base). Sinceutils.suidhas no heavy dependencies, you could move this to a single top-level import and reuse it, which also keeps imports easier to discover.-from utils.suid import normalize_conversation_id +from utils.suid import normalize_conversation_id ... -def persist_user_conversation_details(...): - from utils.suid import normalize_conversation_id - # Normalize the conversation ID (strip 'conv_' prefix if present) - normalized_id = normalize_conversation_id(conversation_id) +def persist_user_conversation_details(...): + # Normalize the conversation ID (strip 'conv_' prefix if present) + normalized_id = normalize_conversation_id(conversation_id)Same for the import inside
query_endpoint_handler_base.tests/unit/utils/test_suid.py (1)
3-77: Good coverage forcheck_suid; consider adding tests for normalization helpersThe expanded test suite around
check_suidlooks solid: you cover valid/invalid UUIDs, 48‑char hex,conv_‑prefixed IDs, wrong lengths, non‑hex chars, and even non‑string inputs. This aligns well with the updated implementation inutils.suid.As a follow‑up, it would be useful to add a couple of small tests for
normalize_conversation_idandto_llama_stack_conversation_idto lock in their behavior (e.g.,conv_→ stripped, already‑normalized IDs unchanged, and round‑tripping between the two).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/app/endpoints/query.py(2 hunks)src/app/endpoints/query_v2.py(1 hunks)src/app/endpoints/streaming_query_v2.py(1 hunks)src/app/routers.py(1 hunks)src/utils/suid.py(1 hunks)tests/unit/utils/test_suid.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/app/endpoints/streaming_query_v2.py
- src/app/routers.py
🧰 Additional context used
📓 Path-based instructions (5)
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: Use absolute imports for internal modules in LCS project (e.g.,from auth import get_auth_dependency)
All modules must start with descriptive docstrings explaining their purpose
Uselogger = logging.getLogger(__name__)pattern for module logging
All functions must include complete type annotations for parameters and return types, using modern syntax (str | int) andOptional[Type]orType | None
All functions must have docstrings with brief descriptions following Google Python docstring conventions
Function names must use snake_case with descriptive, action-oriented names (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead of modifying input parameters
Useasync deffor I/O operations and external API calls
All classes must include descriptive docstrings explaining their purpose following Google Python docstring conventions
Class names must use PascalCase with descriptive names and standard suffixes:Configurationfor config classes,Error/Exceptionfor exceptions,Resolverfor strategy patterns,Interfacefor abstract base classes
Abstract classes must use ABC with@abstractmethoddecorators
Include complete type annotations for all class attributes in Python classes
Useimport loggingand module logger pattern with standard log levels: debug, info, warning, error
Files:
src/app/endpoints/query.pysrc/utils/suid.pysrc/app/endpoints/query_v2.py
src/app/endpoints/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use FastAPI
HTTPExceptionwith appropriate status codes for API endpoint error handling
Files:
src/app/endpoints/query.pysrc/app/endpoints/query_v2.py
src/**/{client,app/endpoints/**}.py
📄 CodeRabbit inference engine (CLAUDE.md)
Handle
APIConnectionErrorfrom Llama Stack in integration code
Files:
src/app/endpoints/query.pysrc/app/endpoints/query_v2.py
tests/{unit,integration}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/{unit,integration}/**/*.py: Use pytest for all unit and integration tests; do not use unittest framework
Unit tests must achieve 60% code coverage; integration tests must achieve 10% coverage
Files:
tests/unit/utils/test_suid.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use
pytest-mockwith AsyncMock objects for mocking in tests
Files:
tests/unit/utils/test_suid.py
🧬 Code graph analysis (3)
src/app/endpoints/query.py (4)
src/utils/suid.py (1)
normalize_conversation_id(57-78)src/app/database.py (1)
get_session(35-41)src/models/database/conversations.py (1)
UserConversation(11-38)src/utils/endpoints.py (1)
validate_conversation_ownership(71-91)
tests/unit/utils/test_suid.py (1)
src/utils/suid.py (1)
check_suid(19-54)
src/app/endpoints/query_v2.py (1)
src/utils/suid.py (1)
normalize_conversation_id(57-78)
⏰ 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). (6)
- GitHub Check: E2E: library mode / ci
- GitHub Check: E2E: server mode / azure
- GitHub Check: E2E: library mode / azure
- GitHub Check: E2E: server mode / ci
- GitHub Check: build-pr
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
🔇 Additional comments (3)
src/app/endpoints/query_v2.py (1)
352-447: Conversation ID flow for Responses API looks consistent; confirm clients expect normalized IDsThe new conversation handling in
retrieve_responseis coherent:
- If the client supplies
query_request.conversation_id, you convert it to llama‑stack format viato_llama_stack_conversation_idand keep the original as the local variable.- If not, you create a new conversation via
client.conversations.create, then immediately normalize its ID and store that normalized form inconversation_id.- You always send
llama_stack_conv_idtoclient.responses.create(...)and always returnnormalize_conversation_id(conversation_id)to the caller.This cleanly separates the llama‑stack wire format from your own API/DB format.
One thing to double‑check: this means the
QueryResponse.conversation_idyou return is always normalized (noconv_prefix), even if the caller sent aconv_...value. That’s a behavior change compared to exposing raw IDs. Please make sure:
- Any existing clients / frontends rely only on the ID you return (and don’t assume a
conv_prefix), and- Any documentation that previously showed raw llama‑stack conversation IDs is updated to clarify that your API exposes normalized IDs.
If those expectations are aligned, this implementation looks good.
src/utils/suid.py (2)
19-55: check_suid logic matches the new ID formats and testsThe revised
check_suidcorrectly:
- Handles non‑string inputs defensively (
Falseearly),- Accepts
conv_‑prefixed 48‑char hex by stripping the prefix and validating the hex portion, and- Falls back to
uuid.UUID(suid)so both standard UUID strings (with hyphens) and 32‑char hex UUIDs are accepted.This is aligned with the new test cases and the stated intent to support both standard UUIDs and llama‑stack conversation IDs. No changes needed here.
57-101: Normalization helpers are straightforward; keep them validation‑agnostic and rely on check_suid upstream
normalize_conversation_idandto_llama_stack_conversation_idare intentionally simple, pure string helpers that just strip or add theconv_prefix without doing any hex/UUID validation. That’s appropriate givencheck_suidalready covers format validation and callers can (and in several places do) normalize independently of validation.Just ensure you consistently apply this pattern at boundaries:
- Validate with
check_suid(e.g., in request models or validators),- Use
normalize_conversation_idfor anything persisted or compared in the DB,- Use
to_llama_stack_conversation_idonly when talking to llama‑stack APIs that expect the prefixed form.Within those constraints, these helpers look good as written.
This is the rebase for what was left after merging PR-866
luis5tb
force-pushed
the
conversations
branch
from
98b18ff to
a62ae9d
Compare
luis5tb
changed the title
|
LGTM |
4 participants
Description
After PR #866, there were some reviews comments that were addressed in the original PR, so rebasing it to add them back
Type of change
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit
Release Notes
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.