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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe changes extend the caching and message infrastructure to support attachments. A new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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? |
sriroopar
force-pushed
the
conversations_attachment_metadata
branch
2 times, most recently
from
261fdf4 to
df699f0
Compare
sriroopar
force-pushed
the
conversations_attachment_metadata
branch
from
df699f0 to
fb2772f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/cache/sqlite_cache.py (2)
49-66: Schema migration required for existing deployments.
CREATE TABLE IF NOT EXISTSsilently skips creation when the table already exists. Any deployment that upgrades with an existingcachetable (without theattachmentscolumn) will receiveOperationalError: no such column: attachmentson the firstSELECTorINSERT. Consider adding anALTER TABLEmigration step ininitialize_cache()to add the column when missing, or providing a standalone migration script.# Example migration guard in initialize_cache(): cursor.execute( "ALTER TABLE cache ADD COLUMN attachments text" ) # Ignore error if column already exists (sqlite3.OperationalError)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cache/sqlite_cache.py` around lines 49 - 66, The CREATE_CACHE_TABLE uses IF NOT EXISTS so existing DBs may lack the new attachments column which causes OperationalError; update initialize_cache() to run an ALTER TABLE cache ADD COLUMN attachments text migration (using the same DB cursor/connection used for CREATE_CACHE_TABLE) before any selects/inserts, and catch sqlite3.OperationalError (or check PRAGMA table_info('cache')) so the code ignores the error if the column already exists; ensure this migration runs once during initialization so subsequent calls to functions that reference the attachments column succeed.
311-312: MoveAttachmentimport to module level.Importing inside the method body means the module is re-imported on every
get()call. There is no circular-import risk (models.requestsdoes not import fromcache.*), so this can be a standard top-level import.♻️ Proposed refactor
+from models.requests import Attachment from models.cache_entry import CacheEntry from models.config import SQLiteDatabaseConfiguration from models.responses import ConversationDataThen in
get():- # Parse attachments back into Attachment objects - attachments_json_str = conversation_entry[9] - attachments_obj = None - if attachments_json_str: - try: - from models.requests import Attachment - - attachments_data = json.loads(attachments_json_str) + # Parse attachments back into Attachment objects + attachments_json_str = conversation_entry[9] + attachments_obj = None + if attachments_json_str: + try: + attachments_data = json.loads(attachments_json_str)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cache/sqlite_cache.py` around lines 311 - 312, The Attachment class is imported inside the get() method causing repeated imports; move the import to module level by adding "from models.requests import Attachment" at the top of src/cache/sqlite_cache.py and remove the in-method import inside the get() function so get() uses the already-imported Attachment symbol.src/cache/postgres_cache.py (2)
50-67: Schema migration required for existing PostgreSQL deployments.Same concern as
sqlite_cache.py:CREATE TABLE IF NOT EXISTSis a no-op when the table exists. Existing databases without theattachmentscolumn will fail withpsycopg2.errors.UndefinedColumnon the first query. Provide anALTER TABLE cache ADD COLUMN IF NOT EXISTS attachments jsonb;migration step ininitialize_cache()or a separate migration script.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cache/postgres_cache.py` around lines 50 - 67, The cache schema SQL (CREATE_CACHE_TABLE) only creates the table if missing but doesn't add new columns to existing tables, causing UndefinedColumn errors for the attachments column; update initialize_cache() (or add a migration step run during startup) to execute an ALTER TABLE cache ADD COLUMN IF NOT EXISTS attachments jsonb; (and any other new jsonb columns like tool_results/tool_calls if needed) after running CREATE_CACHE_TABLE so existing PostgreSQL deployments get the new column without manual migration; ensure this runs using the same DB connection logic in initialize_cache() and handle/log any errors.
326-342: MoveAttachmentimport to module level (mirrors the same issue insqlite_cache.py).♻️ Proposed refactor
+from models.requests import Attachment from models.cache_entry import CacheEntry from models.config import PostgreSQLDatabaseConfiguration from models.responses import ConversationDataThen remove the inline import from
get():- try: - from models.requests import Attachment - - attachments_obj = [ + try: + attachments_obj = [🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cache/postgres_cache.py` around lines 326 - 342, The inline import of Attachment inside the get() block should be moved to the module level: add "from models.requests import Attachment" to the top-level imports and remove the inline "from models.requests import Attachment" from the block that parses attachments (the code referencing attachments_data, attachments_obj and conversation_entry). Keep the existing try/except and logging behavior but reference the module-level Attachment when calling Attachment.model_validate to deserialize attachments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/models/responses.py`:
- Around line 16-18: The runtime PydanticUndefinedAnnotation occurs because
Attachment is only imported inside the TYPE_CHECKING block so it's not present
when Message.model_json_schema() is called; remove the if TYPE_CHECKING guard
and import Attachment at module level in models.responses, then change any
string forward-reference annotations (e.g., "Attachment") to the actual
Attachment type on the Message (or related) field so Pydantic can resolve it at
runtime (this fixes the error raised when calling Message.model_json_schema()).
---
Nitpick comments:
In `@src/cache/postgres_cache.py`:
- Around line 50-67: The cache schema SQL (CREATE_CACHE_TABLE) only creates the
table if missing but doesn't add new columns to existing tables, causing
UndefinedColumn errors for the attachments column; update initialize_cache() (or
add a migration step run during startup) to execute an ALTER TABLE cache ADD
COLUMN IF NOT EXISTS attachments jsonb; (and any other new jsonb columns like
tool_results/tool_calls if needed) after running CREATE_CACHE_TABLE so existing
PostgreSQL deployments get the new column without manual migration; ensure this
runs using the same DB connection logic in initialize_cache() and handle/log any
errors.
- Around line 326-342: The inline import of Attachment inside the get() block
should be moved to the module level: add "from models.requests import
Attachment" to the top-level imports and remove the inline "from models.requests
import Attachment" from the block that parses attachments (the code referencing
attachments_data, attachments_obj and conversation_entry). Keep the existing
try/except and logging behavior but reference the module-level Attachment when
calling Attachment.model_validate to deserialize attachments.
In `@src/cache/sqlite_cache.py`:
- Around line 49-66: The CREATE_CACHE_TABLE uses IF NOT EXISTS so existing DBs
may lack the new attachments column which causes OperationalError; update
initialize_cache() to run an ALTER TABLE cache ADD COLUMN attachments text
migration (using the same DB cursor/connection used for CREATE_CACHE_TABLE)
before any selects/inserts, and catch sqlite3.OperationalError (or check PRAGMA
table_info('cache')) so the code ignores the error if the column already exists;
ensure this migration runs once during initialization so subsequent calls to
functions that reference the attachments column succeed.
- Around line 311-312: The Attachment class is imported inside the get() method
causing repeated imports; move the import to module level by adding "from
models.requests import Attachment" at the top of src/cache/sqlite_cache.py and
remove the in-method import inside the get() function so get() uses the
already-imported Attachment symbol.
| if TYPE_CHECKING: | ||
| from models.requests import Attachment | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify models/requests.py does NOT import from models/responses.py (circular import check)
echo "=== Imports in models/requests.py ==="
rg -n "from models.responses\|import models.responses" --type py
# Verify no model_rebuild() call exists for Message
echo ""
echo "=== model_rebuild calls ==="
rg -n "model_rebuild" --type py src/Repository: lightspeed-core/lightspeed-stack
Length of output: 142
🏁 Script executed:
#!/bin/bash
echo "=== Lines 1-30 of src/models/responses.py ==="
head -30 src/models/responses.py | cat -n
echo ""
echo "=== Lines 825-835 of src/models/responses.py ==="
sed -n '825,835p' src/models/responses.py | cat -n
echo ""
echo "=== Check if Attachment is used in responses.py ==="
rg -n "Attachment" src/models/responses.pyRepository: lightspeed-core/lightspeed-stack
Length of output: 2073
🏁 Script executed:
#!/bin/bash
echo "=== All TYPE_CHECKING imports in responses.py ==="
rg -n "if TYPE_CHECKING" src/models/responses.py -A 2
echo ""
echo "=== Verify Message class definition ==="
rg -n "^class Message" src/models/responses.py -A 5Repository: lightspeed-core/lightspeed-stack
Length of output: 437
Remove TYPE_CHECKING and directly import Attachment to fix runtime PydanticUndefinedAnnotation error.
Attachment is imported only inside if TYPE_CHECKING:, so it is never in models.responses's global namespace at runtime. Pydantic v2 resolves the string forward-reference "Attachment" by looking it up in the defining module's __dict__, so calling Message.model_json_schema() (which FastAPI does at startup to generate OpenAPI docs) will raise PydanticUndefinedAnnotation.
Verification confirms no circular import exists; models/requests.py does not import from models/responses.py. The fix is to directly import Attachment at module level and update the field annotation:
Fix
-from typing import Any, ClassVar, Literal, Optional, Union, TYPE_CHECKING
+from typing import Any, ClassVar, Literal, Optional, Union
from fastapi import status
from pydantic import BaseModel, Field
from pydantic_core import SchemaError
from constants import MEDIA_TYPE_EVENT_STREAM
from models.config import Action, Configuration
+from models.requests import Attachment
from quota.quota_exceed_error import QuotaExceedError
from utils.types import RAGChunk, ReferencedDocument, ToolCallSummary, ToolResultSummary
-if TYPE_CHECKING:
- from models.requests import Attachment
-Update line 829:
- attachments: Optional[list["Attachment"]] = Field(
+ attachments: Optional[list[Attachment]] = Field(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/models/responses.py` around lines 16 - 18, The runtime
PydanticUndefinedAnnotation occurs because Attachment is only imported inside
the TYPE_CHECKING block so it's not present when Message.model_json_schema() is
called; remove the if TYPE_CHECKING guard and import Attachment at module level
in models.responses, then change any string forward-reference annotations (e.g.,
"Attachment") to the actual Attachment type on the Message (or related) field so
Pydantic can resolve it at runtime (this fixes the error raised when calling
Message.model_json_schema()).
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