LCORE-383: Removing attachment metadata at conversation end point and streamlining o… #983
Conversation
|
Hi @sriroopar. Thanks for your PR. I'm waiting for a lightspeed-core member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
WalkthroughPropagates attachments end-to-end: request attachments are added to transformed user messages, included in CacheEntry, persisted in cache backends (SQLite/Postgres) as JSON, and surfaced in conversation response examples. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Endpoint as Query/Conversation Endpoint
participant Transformer as transform_chat_message
participant Cache as Cache Layer (SQLite/Postgres)
participant DB as Database
Client->>Endpoint: Send request (includes attachments)
Endpoint->>Transformer: Build user message (attach attachments JSON)
Transformer->>Cache: Create CacheEntry (attachments included)
Cache->>DB: INSERT row with attachments JSON
DB-->>Cache: Return row (attachments JSON)
Cache->>Cache: Deserialize attachments -> Attachment objects
Cache-->>Endpoint: Return CacheEntry with attachments
Endpoint-->>Client: Respond with chat history including attachments
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (4)
🧰 Additional context used📓 Path-based instructions (3)**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
src/app/**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
src/models/**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (1)📚 Learning: 2026-01-12T10:58:40.230ZApplied to files:
🧬 Code graph analysis (1)src/cache/postgres_cache.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). (2)
🔇 Additional comments (7)
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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cache/sqlite_cache.py (1)
47-62: Add schema migration to support existing databases with the newattachmentscolumn.The
CREATE TABLE IF NOT EXISTSstatement won't add the newattachmentscolumn to existing tables. When existing databases execute theSELECT_CONVERSATION_HISTORY_STATEMENT(line 80) orINSERT_CONVERSATION_HISTORY_STATEMENT(line 88), they will fail with a "no such column" error.Add an
ALTER TABLE cache ADD COLUMN IF NOT EXISTS attachments textstatement in theinitialize_cache()method after the CREATE TABLE statements to handle schema evolution for existing deployments.
🧹 Nitpick comments (1)
src/cache/sqlite_cache.py (1)
234-258: LGTM!The deserialization logic correctly follows the established pattern used for
referenced_documents. Error handling gracefully degrades toNoneon failure with appropriate warning logs.Minor style note: Line 245 uses implicit string concatenation (
"Failed to deserialize attachments for " "conversation %s: %s"). This is valid Python but could use a single string for consistency with the rest of the codebase.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/app/endpoints/conversations_v2.pysrc/app/endpoints/query.pysrc/cache/sqlite_cache.pysrc/models/cache_entry.pysrc/models/responses.pysrc/utils/endpoints.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use absolute imports for internal modules:from authentication import get_auth_dependency
Use FastAPI dependencies:from fastapi import APIRouter, HTTPException, Request, status, Depends
Use Llama Stack imports:from llama_stack_client import AsyncLlamaStackClient
Checkconstants.pyfor shared constants before defining new ones
All modules start with descriptive docstrings explaining purpose
Uselogger = logging.getLogger(__name__)pattern for module logging
Type aliases defined at module level for clarity
All functions require docstrings with brief descriptions
Complete type annotations for function parameters and return types, usingtyping_extensions.Selffor model validators
Use union types with modern syntax:str | intinstead ofUnion[str, int]
UseOptional[Type]for optional parameters
Use snake_case with descriptive, action-oriented function names (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead
Useasync deffor I/O operations and external API calls
HandleAPIConnectionErrorfrom Llama Stack in error handling
Uselogger.debug()for detailed diagnostic information
Uselogger.info()for general information about program execution
Uselogger.warning()for unexpected events or potential problems
Uselogger.error()for serious problems that prevented function execution
All classes require descriptive docstrings explaining purpose
Use PascalCase for class names with descriptive names and standard suffixes:Configuration,Error/Exception,Resolver,Interface
Use ABC for abstract base classes with@abstractmethoddecorators
Complete type annotations for all class attributes
Follow Google Python docstring conventions (https://google.github.io/styleguide/pyguide.html) with sections: Args, Returns, Raises, Attributes
Runuv run make formatto auto-format code with black and ruff before completion
Runuv run make verifyto run all linters (black, pyl...
Files:
src/utils/endpoints.pysrc/models/cache_entry.pysrc/models/responses.pysrc/app/endpoints/query.pysrc/app/endpoints/conversations_v2.pysrc/cache/sqlite_cache.py
src/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/models/**/*.py: Use@field_validatorand@model_validatorfor custom validation in Pydantic models
ExtendBaseModelfor data Pydantic models
Use@model_validatorand@field_validatorfor Pydantic model validation
Files:
src/models/cache_entry.pysrc/models/responses.py
src/app/**/*.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/conversations_v2.py
🧠 Learnings (1)
📚 Learning: 2026-01-12T10:58:40.230Z
Learnt from: blublinsky
Repo: lightspeed-core/lightspeed-stack PR: 972
File: src/models/config.py:459-513
Timestamp: 2026-01-12T10:58:40.230Z
Learning: In lightspeed-core/lightspeed-stack, for Python files under src/models, when a user claims a fix is done but the issue persists, verify the current code state before accepting the fix. Steps: review the diff, fetch the latest changes, run relevant tests, reproduce the issue, search the codebase for lingering references to the original problem, confirm the fix is applied and not undone by subsequent commits, and validate with local checks to ensure the issue is resolved.
Applied to files:
src/models/cache_entry.pysrc/models/responses.py
🧬 Code graph analysis (2)
src/models/cache_entry.py (1)
src/models/requests.py (1)
Attachment(16-70)
src/cache/sqlite_cache.py (1)
src/models/requests.py (1)
Attachment(16-70)
⏰ 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: E2E: library mode / ci
- GitHub Check: E2E: server mode / ci
🔇 Additional comments (11)
src/app/endpoints/conversations_v2.py (1)
243-250: LGTM! Attachments handling follows existing patterns.The implementation correctly:
- Uses explicit type annotation for the dict
- Conditionally adds attachments only when present
- Uses
model_dump(mode="json")consistently with howreferenced_documentsis serialized (lines 255-257)src/app/endpoints/query.py (1)
378-387: LGTM! Attachments properly propagated to cache entry.The
attachments=query_request.attachments or Nonepattern correctly handles the case where attachments might be an empty list, converting it toNonefor consistency with the CacheEntry model's optional field definition.src/models/cache_entry.py (2)
6-6: LGTM! Import follows project conventions.Absolute import from
models.requestsfollows the coding guidelines.
18-28: LGTM! Field definition follows best practices.The new
attachmentsfield:
- Uses modern union syntax
list[Attachment] | Noneas per guidelines- Has proper default value for optional field
- Is documented in the class docstring
src/utils/endpoints.py (1)
801-810: LGTM! Streaming path matches non-streaming implementation.The attachments handling in
cleanup_after_streamingmirrors the implementation inquery.py(line 386), ensuring consistent behavior between streaming and non-streaming query responses.src/models/responses.py (2)
759-782: LGTM! Documentation accurately reflects new attachments capability.The updated description and examples correctly demonstrate:
- Attachments are optional within user messages
- The attachment structure matches the
Attachmentmodel (attachment_type, content_type, content)- Examples are realistic and helpful for API consumers
784-823: LGTM! Model config examples are comprehensive.The examples properly show:
- A conversation turn with attachments (first entry)
- A conversation turn without attachments (second entry)
This demonstrates both use cases clearly for API documentation.
src/cache/sqlite_cache.py (4)
12-12: LGTM!Import follows the absolute import pattern specified in the coding guidelines and correctly references the
Attachmentmodel frommodels.requests.
79-84: LGTM!The SELECT statement correctly adds the
attachmentscolumn, and the column index (7) aligns with the deserialization code below.
86-90: LGTM!The INSERT statement correctly includes the
attachmentscolumn with matching placeholder count (11 columns, 11 placeholders).
304-331: LGTM with a note on error handling.The serialization logic correctly follows the established pattern for
referenced_documents.Note: If serialization fails,
attachments_jsonremainsNoneand the data is silently lost. This matches the existing behavior forreferenced_documents, but consider whether attachment data loss should be logged at a higher level (e.g.,logger.error) or propagate an exception depending on business requirements.
|
@sriroopar: Cannot trigger testing until a trusted user reviews the PR and leaves an DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/retest |
|
@sriroopar: Cannot trigger testing until a trusted user reviews the PR and leaves an DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Hi @sriroopar, could you fix the black and lint errors please.
Once you are done before you commit make sure you run Thanks! |
sriroopar
force-pushed
the
conversations_attachment_metadata
branch
from
48b6aa0 to
32af4ec
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cache/sqlite_cache.py (1)
345-358: Critical bug: INSERT statement parameter mismatch causes runtime error.The INSERT statement expects 13 parameters (line 96), but only 10 values are provided. Missing serialization and insertion of
tool_calls,tool_results, andattachments. This will raisesqlite3.ProgrammingError: Incorrect number of bindings supplied.🐛 Proposed fix - serialize and include missing fields
referenced_documents_json = None if cache_entry.referenced_documents: try: docs_as_dicts = [ doc.model_dump(mode="json") for doc in cache_entry.referenced_documents ] referenced_documents_json = json.dumps(docs_as_dicts) except (TypeError, ValueError) as e: logger.warning( "Failed to serialize referenced_documents for " "conversation %s: %s", conversation_id, e, ) + tool_calls_json = None + if cache_entry.tool_calls: + try: + tool_calls_json = json.dumps( + [tc.model_dump(mode="json") for tc in cache_entry.tool_calls] + ) + except (TypeError, ValueError) as e: + logger.warning( + "Failed to serialize tool_calls for conversation %s: %s", + conversation_id, + e, + ) + + tool_results_json = None + if cache_entry.tool_results: + try: + tool_results_json = json.dumps( + [tr.model_dump(mode="json") for tr in cache_entry.tool_results] + ) + except (TypeError, ValueError) as e: + logger.warning( + "Failed to serialize tool_results for conversation %s: %s", + conversation_id, + e, + ) + + attachments_json = None + if cache_entry.attachments: + try: + attachments_json = json.dumps( + [att.model_dump(mode="json") for att in cache_entry.attachments] + ) + except (TypeError, ValueError) as e: + logger.warning( + "Failed to serialize attachments for conversation %s: %s", + conversation_id, + e, + ) + cursor.execute( self.INSERT_CONVERSATION_HISTORY_STATEMENT, ( user_id, conversation_id, current_time, cache_entry.started_at, cache_entry.completed_at, cache_entry.query, cache_entry.response, cache_entry.provider, cache_entry.model, referenced_documents_json, + tool_calls_json, + tool_results_json, + attachments_json, ), )
🤖 Fix all issues with AI agents
In @src/cache/sqlite_cache.py:
- Around line 274-287: The code is reading attachments from the wrong tuple
index (using conversation_entry[7] which is tool_calls); update the attachments
deserialization to use the correct index (conversation_entry[9]) so
attachments_json_str points to the attachments column, and keep the existing
json.loads and Attachment.model_validate logic intact; locate the attachments
handling block that defines attachments_json_str and attachments_obj (variables
conversation_entry, attachments_json_str, attachments_obj) and change the index
from 7 to 9.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/app/endpoints/conversations_v2.pysrc/app/endpoints/query.pysrc/cache/sqlite_cache.pysrc/models/cache_entry.pysrc/models/responses.pysrc/utils/endpoints.py
🚧 Files skipped from review as they are similar to previous changes (3)
- src/models/responses.py
- src/models/cache_entry.py
- src/app/endpoints/conversations_v2.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use absolute imports for internal modules:from authentication import get_auth_dependency
Use FastAPI dependencies:from fastapi import APIRouter, HTTPException, Request, status, Depends
Use Llama Stack imports:from llama_stack_client import AsyncLlamaStackClient
Checkconstants.pyfor shared constants before defining new ones
All modules start with descriptive docstrings explaining purpose
Uselogger = logging.getLogger(__name__)pattern for module logging
Type aliases defined at module level for clarity
All functions require docstrings with brief descriptions
Complete type annotations for function parameters and return types, usingtyping_extensions.Selffor model validators
Use union types with modern syntax:str | intinstead ofUnion[str, int]
UseOptional[Type]for optional parameters
Use snake_case with descriptive, action-oriented function names (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead
Useasync deffor I/O operations and external API calls
HandleAPIConnectionErrorfrom Llama Stack in error handling
Uselogger.debug()for detailed diagnostic information
Uselogger.info()for general information about program execution
Uselogger.warning()for unexpected events or potential problems
Uselogger.error()for serious problems that prevented function execution
All classes require descriptive docstrings explaining purpose
Use PascalCase for class names with descriptive names and standard suffixes:Configuration,Error/Exception,Resolver,Interface
Use ABC for abstract base classes with@abstractmethoddecorators
Complete type annotations for all class attributes
Follow Google Python docstring conventions (https://google.github.io/styleguide/pyguide.html) with sections: Args, Returns, Raises, Attributes
Runuv run make formatto auto-format code with black and ruff before completion
Runuv run make verifyto run all linters (black, pyl...
Files:
src/app/endpoints/query.pysrc/utils/endpoints.pysrc/cache/sqlite_cache.py
src/app/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use FastAPI
HTTPExceptionwith appropriate status codes for API endpoint error handling
Files:
src/app/endpoints/query.py
🧬 Code graph analysis (1)
src/cache/sqlite_cache.py (1)
src/models/requests.py (1)
Attachment(16-70)
⏰ 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: E2E: library mode / ci
- GitHub Check: E2E: server mode / ci
🔇 Additional comments (2)
src/app/endpoints/query.py (1)
377-388: LGTM!The attachments field is correctly added to the CacheEntry construction, using the
or Nonepattern consistent with other optional fields likereferenced_documents,tool_calls, andtool_results.src/utils/endpoints.py (1)
801-812: LGTM!The attachments field is correctly added to the CacheEntry construction in the streaming cleanup path, maintaining consistency with the non-streaming query endpoint implementation.
sriroopar
force-pushed
the
conversations_attachment_metadata
branch
from
32af4ec to
d5254da
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/cache/sqlite_cache.py:
- Around line 92-97: INSERT_CONVERSATION_HISTORY_STATEMENT declares 13 columns
but the call to cursor.execute only supplies 10 values, causing a binding error;
update the insert call (the cursor.execute in the conversation history insertion
routine) to serialize and include tool_calls, tool_results, and attachments
(e.g., json.dumps on those objects or "null"/"[]" as appropriate) so the
arguments list has 13 items and the order matches the INSERT column order
(user_id, conversation_id, created_at, started_at, completed_at, query,
response, provider, model, referenced_documents, tool_calls, tool_results,
attachments).
🧹 Nitpick comments (1)
src/cache/sqlite_cache.py (1)
50-67: Database migration required for existing deployments.The
CREATE TABLE IF NOT EXISTSstatement won't add the newattachmentscolumn to existing databases. Existing deployments will encountersqlite3.OperationalError: table cache has no column named attachmentswhen inserting or selecting.Consider adding an
ALTER TABLEmigration or handling the schema upgrade gracefully.💡 Example migration approach
# Add to initialize_cache() after CREATE_CACHE_TABLE execution try: cursor.execute("ALTER TABLE cache ADD COLUMN attachments text") logger.info("Added attachments column to cache table") except sqlite3.OperationalError: # Column already exists pass
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/app/endpoints/conversations_v2.pysrc/app/endpoints/query.pysrc/cache/sqlite_cache.pysrc/models/cache_entry.pysrc/models/responses.pysrc/utils/endpoints.py
🚧 Files skipped from review as they are similar to previous changes (4)
- src/utils/endpoints.py
- src/app/endpoints/conversations_v2.py
- src/models/responses.py
- src/app/endpoints/query.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use absolute imports for internal modules:from authentication import get_auth_dependency
Use FastAPI dependencies:from fastapi import APIRouter, HTTPException, Request, status, Depends
Use Llama Stack imports:from llama_stack_client import AsyncLlamaStackClient
Checkconstants.pyfor shared constants before defining new ones
All modules start with descriptive docstrings explaining purpose
Uselogger = logging.getLogger(__name__)pattern for module logging
Type aliases defined at module level for clarity
All functions require docstrings with brief descriptions
Complete type annotations for function parameters and return types, usingtyping_extensions.Selffor model validators
Use union types with modern syntax:str | intinstead ofUnion[str, int]
UseOptional[Type]for optional parameters
Use snake_case with descriptive, action-oriented function names (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead
Useasync deffor I/O operations and external API calls
HandleAPIConnectionErrorfrom Llama Stack in error handling
Uselogger.debug()for detailed diagnostic information
Uselogger.info()for general information about program execution
Uselogger.warning()for unexpected events or potential problems
Uselogger.error()for serious problems that prevented function execution
All classes require descriptive docstrings explaining purpose
Use PascalCase for class names with descriptive names and standard suffixes:Configuration,Error/Exception,Resolver,Interface
Use ABC for abstract base classes with@abstractmethoddecorators
Complete type annotations for all class attributes
Follow Google Python docstring conventions (https://google.github.io/styleguide/pyguide.html) with sections: Args, Returns, Raises, Attributes
Runuv run make formatto auto-format code with black and ruff before completion
Runuv run make verifyto run all linters (black, pyl...
Files:
src/models/cache_entry.pysrc/cache/sqlite_cache.py
src/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/models/**/*.py: Use@field_validatorand@model_validatorfor custom validation in Pydantic models
ExtendBaseModelfor data Pydantic models
Use@model_validatorand@field_validatorfor Pydantic model validation
Files:
src/models/cache_entry.py
🧠 Learnings (1)
📚 Learning: 2026-01-12T10:58:40.230Z
Learnt from: blublinsky
Repo: lightspeed-core/lightspeed-stack PR: 972
File: src/models/config.py:459-513
Timestamp: 2026-01-12T10:58:40.230Z
Learning: In lightspeed-core/lightspeed-stack, for Python files under src/models, when a user claims a fix is done but the issue persists, verify the current code state before accepting the fix. Steps: review the diff, fetch the latest changes, run relevant tests, reproduce the issue, search the codebase for lingering references to the original problem, confirm the fix is applied and not undone by subsequent commits, and validate with local checks to ensure the issue is resolved.
Applied to files:
src/models/cache_entry.py
🧬 Code graph analysis (2)
src/models/cache_entry.py (1)
src/models/requests.py (1)
Attachment(16-70)
src/cache/sqlite_cache.py (1)
src/models/requests.py (1)
Attachment(16-70)
⏰ 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: E2E: library mode / ci
- GitHub Check: E2E: server mode / ci
🔇 Additional comments (2)
src/models/cache_entry.py (1)
7-7: LGTM!The new
attachmentsfield is well-integrated:
- Uses absolute import as per coding guidelines.
- Follows the same pattern as other optional list fields (
tool_calls,tool_results).- Modern union syntax
list[Attachment] | Nonealigns with the coding guidelines.- Docstring is updated appropriately.
Also applies to: 23-23, 35-35
src/cache/sqlite_cache.py (1)
274-299: LGTM!The deserialization logic for
attachmentsfollows the established pattern used forreferenced_documents,tool_calls, andtool_results:
- Consistent error handling with
json.JSONDecodeErrorandValueError.- Appropriate warning logs on failure.
- Uses
Attachment.model_validatefor proper Pydantic deserialization.
sriroopar
force-pushed
the
conversations_attachment_metadata
branch
from
d5254da to
334ada9
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 (1)
src/cache/postgres_cache.py (1)
51-68: Add database migration for the newattachmentscolumn in existing PostgreSQL deployments.The
CREATE TABLE IF NOT EXISTSstatement only creates the table on first run. Existing PostgreSQL deployments that already have thecachetable will not automatically receive the newattachmentscolumn, causing INSERT and SELECT statements to fail at runtime.Provide a migration path for existing deployments—either an
ALTER TABLEstatement with aCREATE TABLE IF NOT EXISTSfallback, or documentation instructing users to manually add the column before upgrading.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/app/endpoints/conversations_v2.pysrc/app/endpoints/query.pysrc/cache/postgres_cache.pysrc/cache/sqlite_cache.pysrc/models/cache_entry.pysrc/models/responses.pysrc/utils/endpoints.py
🚧 Files skipped from review as they are similar to previous changes (4)
- src/app/endpoints/query.py
- src/utils/endpoints.py
- src/cache/sqlite_cache.py
- src/models/cache_entry.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use absolute imports for internal modules:from authentication import get_auth_dependency
Use FastAPI dependencies:from fastapi import APIRouter, HTTPException, Request, status, Depends
Use Llama Stack imports:from llama_stack_client import AsyncLlamaStackClient
Checkconstants.pyfor shared constants before defining new ones
All modules start with descriptive docstrings explaining purpose
Uselogger = logging.getLogger(__name__)pattern for module logging
Type aliases defined at module level for clarity
All functions require docstrings with brief descriptions
Complete type annotations for function parameters and return types, usingtyping_extensions.Selffor model validators
Use union types with modern syntax:str | intinstead ofUnion[str, int]
UseOptional[Type]for optional parameters
Use snake_case with descriptive, action-oriented function names (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead
Useasync deffor I/O operations and external API calls
HandleAPIConnectionErrorfrom Llama Stack in error handling
Uselogger.debug()for detailed diagnostic information
Uselogger.info()for general information about program execution
Uselogger.warning()for unexpected events or potential problems
Uselogger.error()for serious problems that prevented function execution
All classes require descriptive docstrings explaining purpose
Use PascalCase for class names with descriptive names and standard suffixes:Configuration,Error/Exception,Resolver,Interface
Use ABC for abstract base classes with@abstractmethoddecorators
Complete type annotations for all class attributes
Follow Google Python docstring conventions (https://google.github.io/styleguide/pyguide.html) with sections: Args, Returns, Raises, Attributes
Runuv run make formatto auto-format code with black and ruff before completion
Runuv run make verifyto run all linters (black, pyl...
Files:
src/app/endpoints/conversations_v2.pysrc/models/responses.pysrc/cache/postgres_cache.py
src/app/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use FastAPI
HTTPExceptionwith appropriate status codes for API endpoint error handling
Files:
src/app/endpoints/conversations_v2.py
src/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/models/**/*.py: Use@field_validatorand@model_validatorfor custom validation in Pydantic models
ExtendBaseModelfor data Pydantic models
Use@model_validatorand@field_validatorfor Pydantic model validation
Files:
src/models/responses.py
🧠 Learnings (1)
📚 Learning: 2026-01-12T10:58:40.230Z
Learnt from: blublinsky
Repo: lightspeed-core/lightspeed-stack PR: 972
File: src/models/config.py:459-513
Timestamp: 2026-01-12T10:58:40.230Z
Learning: In lightspeed-core/lightspeed-stack, for Python files under src/models, when a user claims a fix is done but the issue persists, verify the current code state before accepting the fix. Steps: review the diff, fetch the latest changes, run relevant tests, reproduce the issue, search the codebase for lingering references to the original problem, confirm the fix is applied and not undone by subsequent commits, and validate with local checks to ensure the issue is resolved.
Applied to files:
src/models/responses.py
🧬 Code graph analysis (1)
src/cache/postgres_cache.py (1)
src/models/requests.py (1)
Attachment(16-70)
⏰ 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: E2E: library mode / ci
- GitHub Check: E2E: server mode / ci
🔇 Additional comments (7)
src/models/responses.py (2)
761-784: LGTM! Documentation and field example updated correctly for attachment support.The description and example accurately reflect that user messages can now include attachments, and the example structure (
attachment_type,content_type,content) matches theAttachmentmodel defined insrc/models/requests.py.
793-823: LGTM! Model config examples properly demonstrate attachment-containing messages.The nested example in
model_configcorrectly shows the new chat history format with attachments in user messages, providing clear OpenAPI documentation for API consumers.src/cache/postgres_cache.py (4)
85-91: Verify SELECT column index alignment for attachments field.The SELECT statement retrieves 10 columns (query, response, provider, model, started_at, completed_at, referenced_documents, tool_calls, tool_results, attachments). The code at line 308 accesses
conversation_entry[9]for attachments, which is correct (0-indexed).
93-98: LGTM! INSERT statement correctly includes attachments column.The INSERT statement has 13 columns and 13 placeholders (
%s), and theattachmentscolumn is properly added at the end.
307-334: LGTM! Attachments deserialization follows established pattern.The deserialization logic for attachments mirrors the existing patterns for
referenced_documents,tool_calls, andtool_results- usingAttachment.model_validate()with appropriate error handling and warning logs on failure.
405-436: LGTM! Attachments serialization follows established pattern.The serialization logic for attachments correctly uses
model_dump(mode="json")andjson.dumps(), consistent with the handling of other JSONB fields. The parameter is properly passed to the cursor execute call.src/app/endpoints/conversations_v2.py (1)
241-265: LGTM! Attachments handling in transform_chat_message is well-implemented.The implementation:
- Adds proper type annotation for
user_message- Conditionally includes attachments only when present (avoids sending null/empty arrays)
- Uses
model_dump(mode="json")consistent with thereferenced_documentsserialization patternThis aligns well with the API response schema updates in
responses.py.
|
Hey @jrobertboos, thank you for your comment, fixed the lint errors. Is it sufficient that the attachment meta data is represented at the conversation endpoint, or do we want it to be retained in the DB as a new column? Just wanted to confirm the scope of the attachment data. Ty! |
|
I am not 100% sure I think that it should just be represented at the conversation endpoint just so that we are not messing with the DB but idk. @tisnik wdyt? |
2 participants
…utput.
Description
Changes are added to derive better insights from the conversation metadata using new fields to better represent attachments related information.
Type of change
Tools used to create PR
Identify any AI code assistants used in this PR (for transparency and review context)
Cursor
Claude Code (minimal use)
Related Tickets & Documents
Checklist before requesting a review
Testing
Please provide detailed steps to perform tests related to this code change.
A query with attachment was sent to /conversations and chat_history struct was manually verified.
How were the fix/results from this change verified? Please provide relevant screenshots or results.
A query with attachment was sent to /conversations and chat_history struct was manually verified. Also re-verified through Swagger UI.
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.