Support documents in conversation #478
Conversation
WalkthroughThe PR extends multimodal attachment support to include text-based documents (PDFs, spreadsheets, code files) by introducing text extraction, token counting services, and context-usage validation. New Alembic migrations convert attachment types to lowercase and add document format enums. Document uploads now extract and store text inline or in cloud storage, with token estimates embedded in metadata. Token validation gates attachments against model context limits before LLM calls. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Router as ConversationsRouter
participant Service as ConversationService
participant LLM as LLM Provider
participant Counter as TokenCounter
User->>Router: POST /message with attachment_ids
Router->>Router: Parse attachment_ids from Form
Router->>Service: Create message with parsed_attachment_ids
Service->>Service: _prepare_text_attachments(attachment_ids)
Service->>Service: Extract text from PDFs, docs, code files
Service->>Service: Build additional_context string
rect rgb(200, 220, 255)
Note over Service,Counter: Context Token Validation
Service->>Counter: count_messages_tokens(history, model)
Service->>Counter: count_tokens(additional_context, model)
Service->>Counter: Sum with attachment token counts
Service->>Service: Validate total ≤ model.context_limit
end
alt Tokens Valid
Service->>LLM: Stream with additional_context
LLM-->>User: Return response
else Tokens Exceed Limit
Service-->>User: 400 Context Limit Exceeded
end
sequenceDiagram
participant User
participant Router as MediaRouter
participant Controller as MediaController
participant Service as MediaService
participant Extractor as TextExtractionService
participant Counter as TokenCounter
participant Storage as Cloud Storage
User->>Router: POST /media/validate-document<br/>(file_size, mime_type, etc.)
Router->>Controller: validate_document_upload(...)
Controller->>Service: Retrieve conversation context
Controller->>Service: Get model config & context_window
Controller->>Counter: count_messages_tokens(history)
Controller->>Counter: estimate_file_tokens(file_size, mime_type)
Controller->>Controller: Calculate: current_tokens + file_tokens vs. context_limit
alt Can Upload
Controller-->>Router: {can_upload: true, tokens: {...}, remaining: ...}
Router-->>User: 200 OK
else Exceeds Limit
Controller-->>Router: {can_upload: false, excess: ...}
Router-->>User: 400 Context Limit Exceeded
end
opt If Validation Passes & User Uploads
User->>Router: POST /media/attachments (file)
Router->>Controller: upload_document(file)
Controller->>Service: upload_document(file)
Service->>Extractor: extract_text(file_data, mime_type)
Extractor-->>Service: (extracted_text, metadata)
Service->>Counter: count_tokens(extracted_text)
Service->>Storage: Upload file & text (inline or reference)
Service-->>Router: AttachmentUploadResponse
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/modules/conversations/conversation/conversation_service.py (1)
739-778: Align token counting with the history we actually send
history_tokensis computed over the entirevalidated_history, but the request only shipsvalidated_history[-8:]to the LLM. On longer chats this inflatestotal_tokensand will reject perfectly valid uploads with the “exceeds context window” error. Count tokens on the same slice you pass to the provider and reuse it when buildingChatContextso validation reflects reality.- history_tokens = token_counter.count_messages_tokens( - validated_history, model - ) + history_subset = validated_history[-8:] + history_tokens = token_counter.count_messages_tokens( + history_subset, model + ) ... - chat_context = ChatContext( + chat_context = ChatContext( ... - history=validated_history[-8:], + history=history_subset,app/modules/conversations/conversations_router.py (1)
267-296: Critical: Cleanup logic incorrectly deletes pre-existing attachments.The cleanup logic at line 288 iterates over
parsed_attachment_ids, which includes both pre-existing attachment IDs passed via theattachment_idsform parameter and newly uploaded image IDs. If an image upload fails, this will incorrectly delete attachments that were merely being referenced, not newly created.Scenario:
- User sends
attachment_ids='["existing-doc-id"]'+images=[new-image.jpg, bad-image.jpg]parsed_attachment_ids = ["existing-doc-id"]after parsing- First image uploads successfully:
["existing-doc-id", "new-img-id"]- Second image upload fails
- Cleanup deletes both
"existing-doc-id"(shouldn't delete!) and"new-img-id"Apply this diff to track and clean up only newly uploaded images:
# Parse attachment_ids from form data if provided parsed_attachment_ids = [] + newly_uploaded_image_ids = [] if attachment_ids: try: parsed_attachment_ids = json.loads(attachment_ids) except json.JSONDecodeError: raise HTTPException( status_code=400, detail="Invalid attachment_ids format" ) # Process images if present and add to attachment_ids if images: media_service = MediaService(db) for i, image in enumerate(images): # Check if image has content by checking filename and content_type if image.filename and image.content_type: try: # Read file data first and pass as bytes to avoid UploadFile issues file_content = await image.read() upload_result = await media_service.upload_image( file=file_content, file_name=image.filename, mime_type=image.content_type, message_id=None, # Will be linked after message creation ) parsed_attachment_ids.append(upload_result.id) + newly_uploaded_image_ids.append(upload_result.id) except Exception as e: logger.error( f"Failed to upload image {image.filename}: {str(e)}" ) # Clean up any successfully uploaded attachments - for uploaded_id in parsed_attachment_ids: + for uploaded_id in newly_uploaded_image_ids: try: await media_service.delete_attachment(uploaded_id) except: pass raise HTTPException( status_code=400, detail=f"Failed to upload image {image.filename}: {str(e)}", )
🧹 Nitpick comments (4)
app/modules/conversations/conversations_router.py (4)
257-265: Consider chaining the exception per static analysis.The exception handling is functional, but following the static analysis suggestion to use
raise ... from errorraise ... from Nonewould clarify that this is an intentional re-raise rather than an error in exception handling.Apply this diff:
try: parsed_attachment_ids = json.loads(attachment_ids) except json.JSONDecodeError: raise HTTPException( status_code=400, detail="Invalid attachment_ids format" - ) + ) from None
680-682: Static analysis: Avoid function calls in argument defaults.The
Depends()calls in the function signature defaults trigger static analysis warnings. While this pattern is common in FastAPI and works correctly, some linters flag it as a potential issue. Since this is a FastAPI convention and the warnings are false positives in this context, this can be safely ignored.Based on learnings
726-748: Consider handling messages with missing content.The code appends
f"{msg.type}: {msg.content}"to history without checking ifmsg.contentcould be None or empty beyond theif msg.content:check at line 739. While the check exists, consider whether empty strings should contribute to the token count or be filtered out.
783-787: Static analysis: Chain the exception.Following the static analysis suggestion, use
raise ... from errto preserve the exception chain for better debugging context.Apply this diff:
except HTTPException: raise except Exception as e: logger.error(f"Error getting context usage: {e}", exc_info=True) - raise HTTPException(status_code=500, detail=str(e)) + raise HTTPException(status_code=500, detail=str(e)) from e
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.gitignore(1 hunks)app/alembic/versions/1d75bb639a7e_convert_attachment_types_to_lowercase.py(1 hunks)app/alembic/versions/20251106_add_document_attachment_types.py(1 hunks)app/modules/conversations/conversation/conversation_service.py(5 hunks)app/modules/conversations/conversations_router.py(8 hunks)app/modules/intelligence/provider/llm_config.py(18 hunks)app/modules/intelligence/provider/token_counter.py(1 hunks)app/modules/media/media_controller.py(2 hunks)app/modules/media/media_model.py(2 hunks)app/modules/media/media_router.py(3 hunks)app/modules/media/media_service.py(4 hunks)app/modules/media/text_extraction_service.py(1 hunks)requirements.txt(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
app/modules/media/media_controller.py (7)
app/modules/media/media_service.py (3)
upload_document(197-329)_check_multimodal_enabled(102-105)MediaServiceError(30-33)app/modules/media/media_schema.py (1)
AttachmentUploadResponse(25-31)app/modules/media/media_router.py (1)
validate_document_upload(60-95)app/modules/intelligence/agents/agents_service.py (2)
AgentsService(43-186)validate_agent_id(180-186)app/modules/intelligence/provider/provider_service.py (1)
ProviderService(371-1144)app/modules/intelligence/provider/token_counter.py (4)
get_token_counter(141-146)count_messages_tokens(66-86)estimate_file_tokens(98-115)get_context_limit(88-96)app/modules/intelligence/agents/custom_agents/custom_agents_service.py (1)
get_agent_model(69-86)
app/alembic/versions/1d75bb639a7e_convert_attachment_types_to_lowercase.py (1)
app/alembic/versions/20251106_add_document_attachment_types.py (2)
upgrade(19-23)downgrade(26-30)
app/modules/conversations/conversation/conversation_service.py (4)
app/modules/intelligence/provider/token_counter.py (4)
get_token_counter(141-146)count_messages_tokens(66-86)count_tokens(53-64)get_context_limit(88-96)app/modules/intelligence/provider/provider_service.py (1)
ProviderService(371-1144)app/modules/media/media_model.py (1)
AttachmentType(10-17)app/modules/media/media_service.py (2)
get_attachment(579-590)get_extracted_text(391-431)
app/alembic/versions/20251106_add_document_attachment_types.py (1)
app/alembic/versions/1d75bb639a7e_convert_attachment_types_to_lowercase.py (2)
upgrade(21-34)downgrade(37-45)
app/modules/intelligence/provider/token_counter.py (1)
app/modules/intelligence/provider/llm_config.py (1)
get_config_for_model(267-304)
app/modules/media/media_service.py (4)
app/modules/media/text_extraction_service.py (5)
TextExtractionService(17-299)TextExtractionError(11-14)extract_text(65-100)should_store_inline(297-299)_is_code_file(249-287)app/modules/intelligence/provider/token_counter.py (2)
get_token_counter(141-146)count_tokens(53-64)app/modules/media/media_model.py (3)
StorageProvider(20-24)MessageAttachment(27-52)AttachmentType(10-17)app/modules/media/media_controller.py (2)
upload_document(76-104)_check_multimodal_enabled(32-41)
app/modules/media/text_extraction_service.py (1)
app/modules/media/media_service.py (1)
_is_code_file(353-389)
app/modules/media/media_router.py (3)
app/core/database.py (1)
get_async_db(109-116)app/modules/auth/auth_service.py (2)
AuthService(14-98)check_auth(49-98)app/modules/media/media_controller.py (4)
MediaController(24-452)upload_image(43-74)upload_document(76-104)validate_document_upload(257-392)
app/modules/conversations/conversations_router.py (4)
app/modules/intelligence/provider/token_counter.py (3)
get_token_counter(141-146)count_messages_tokens(66-86)get_context_limit(88-96)app/modules/conversations/conversation/conversation_controller.py (3)
ConversationController(33-223)get_conversation_info(76-89)get_conversation_messages(91-104)app/modules/intelligence/agents/agents_service.py (2)
AgentsService(43-186)validate_agent_id(180-186)app/modules/intelligence/provider/provider_service.py (1)
ProviderService(371-1144)
🪛 Ruff (0.14.3)
app/modules/media/media_controller.py
84-84: Abstract raise to an inner function
(TRY301)
95-95: Consider moving this statement to an else block
(TRY300)
98-98: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
98-98: Use explicit conversion flag
Replace with conversion flag
(RUF010)
99-99: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
102-102: Do not catch blind exception: Exception
(BLE001)
103-103: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
103-103: Use explicit conversion flag
Replace with conversion flag
(RUF010)
104-104: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
261-261: Unused method argument: file_name
(ARG002)
297-297: Abstract raise to an inner function
(TRY301)
386-386: Consider moving this statement to an else block
(TRY300)
391-391: Use explicit conversion flag
Replace with conversion flag
(RUF010)
392-392: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
app/modules/conversations/conversation/conversation_service.py
673-673: Loop control variable att_id not used within loop body
Rename unused att_id to _att_id
(B007)
755-759: Abstract raise to an inner function
(TRY301)
755-759: Avoid specifying long messages outside the exception class
(TRY003)
966-966: Do not catch blind exception: Exception
(BLE001)
967-969: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
968-968: Use explicit conversion flag
Replace with conversion flag
(RUF010)
972-972: Consider moving this statement to an else block
(TRY300)
974-974: Do not catch blind exception: Exception
(BLE001)
975-975: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
975-975: Use explicit conversion flag
Replace with conversion flag
(RUF010)
app/modules/intelligence/provider/token_counter.py
45-45: Do not catch blind exception: Exception
(BLE001)
61-61: Do not catch blind exception: Exception
(BLE001)
62-62: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
71-71: Unused method argument: tokens_per_name
(ARG002)
app/modules/media/media_service.py
218-220: Abstract raise to an inner function
(TRY301)
218-220: Avoid specifying long messages outside the exception class
(TRY003)
224-224: Abstract raise to an inner function
(TRY301)
224-224: Avoid specifying long messages outside the exception class
(TRY003)
228-231: Abstract raise to an inner function
(TRY301)
244-244: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
245-247: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
246-246: Use explicit conversion flag
Replace with conversion flag
(RUF010)
326-326: Use explicit conversion flag
Replace with conversion flag
(RUF010)
329-329: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
329-329: Avoid specifying long messages outside the exception class
(TRY003)
329-329: Use explicit conversion flag
Replace with conversion flag
(RUF010)
396-396: Abstract raise to an inner function
(TRY301)
408-408: Abstract raise to an inner function
(TRY301)
408-408: Avoid specifying long messages outside the exception class
(TRY003)
417-417: Do not catch blind exception: Exception
(BLE001)
418-418: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
419-421: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
419-421: Avoid specifying long messages outside the exception class
(TRY003)
420-420: Use explicit conversion flag
Replace with conversion flag
(RUF010)
429-429: Do not catch blind exception: Exception
(BLE001)
430-430: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
430-430: Use explicit conversion flag
Replace with conversion flag
(RUF010)
431-431: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
431-431: Avoid specifying long messages outside the exception class
(TRY003)
431-431: Use explicit conversion flag
Replace with conversion flag
(RUF010)
app/modules/media/text_extraction_service.py
38-38: Avoid specifying long messages outside the exception class
(TRY003)
50-50: Avoid specifying long messages outside the exception class
(TRY003)
62-62: Avoid specifying long messages outside the exception class
(TRY003)
94-94: Abstract raise to an inner function
(TRY301)
94-94: Avoid specifying long messages outside the exception class
(TRY003)
100-100: Avoid specifying long messages outside the exception class
(TRY003)
100-100: Use explicit conversion flag
Replace with conversion flag
(RUF010)
122-122: Consider moving this statement to an else block
(TRY300)
125-125: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
126-126: Avoid specifying long messages outside the exception class
(TRY003)
126-126: Use explicit conversion flag
Replace with conversion flag
(RUF010)
157-157: Consider moving this statement to an else block
(TRY300)
160-160: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
161-161: Avoid specifying long messages outside the exception class
(TRY003)
161-161: Use explicit conversion flag
Replace with conversion flag
(RUF010)
192-192: Consider moving this statement to an else block
(TRY300)
195-195: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
196-196: Avoid specifying long messages outside the exception class
(TRY003)
196-196: Use explicit conversion flag
Replace with conversion flag
(RUF010)
222-222: Consider moving this statement to an else block
(TRY300)
225-225: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
226-226: Avoid specifying long messages outside the exception class
(TRY003)
226-226: Use explicit conversion flag
Replace with conversion flag
(RUF010)
228-228: Unused method argument: mime_type
(ARG002)
243-243: Consider moving this statement to an else block
(TRY300)
246-246: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
247-247: Avoid specifying long messages outside the exception class
(TRY003)
247-247: Use explicit conversion flag
Replace with conversion flag
(RUF010)
app/modules/media/media_router.py
25-25: Do not perform function call File in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
29-29: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
30-30: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
65-65: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
66-66: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
67-67: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
app/modules/conversations/conversations_router.py
263-265: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
283-283: Do not catch blind exception: Exception
(BLE001)
284-286: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
285-285: Use explicit conversion flag
Replace with conversion flag
(RUF010)
680-680: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
681-681: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
682-682: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
699-699: Abstract raise to an inner function
(TRY301)
787-787: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (6)
.gitignore (1)
71-71: Good addition for consistency with existing AI tooling ignores.The
CLAUDE.mdentry aligns well with the existing ignore patterns for AI-related artifacts (.claude/,.cursor/,.cursorrules), keeping local development tooling out of version control.app/modules/conversations/conversations_router.py (5)
12-12: LGTM!The import is correctly added to support token counting in the new context-usage endpoint.
307-311: LGTM!The MessageRequest construction correctly uses
parsed_attachment_ids, maintaining consistency with the attachment handling flow.
353-380: LGTM!Background task wiring correctly propagates
attachment_ids_listderived fromparsed_attachment_idsto the Celery task.
677-788: Well-structured context usage reporting.Despite the pattern inconsistency noted earlier, the endpoint provides valuable context usage metrics with clear warning levels and token breakdowns. The logic for aggregating tokens from conversation history and attachments is sound, and the response format is informative.
714-724: Review comment verified but clarification needed on fallback logic.The verification confirms that
CustomAgentdoes not have amodelfield—it contains onlyid,user_id,role,goal,backstory,system_prompt,tasks,deployment_url,deployment_status,visibility, and timestamps. Therefore, usingprovider_service.chat_config.modelfor custom agents is by design, not an oversight.However, the code's defensive fallback to
"openai/gpt-4o"whencustom_agentis falsy contradicts the earlier validation check (agent_type == "CUSTOM_AGENT"). This suggests either:
validate_agent_id()returning"CUSTOM_AGENT"does not guaranteeget_agent_model()will succeed (possible permission or state issue)- The fallback handles an undocumented edge case
For clarity, add a comment explaining why the fallback exists or ensure the agent validation guarantees object retrieval.
| def upgrade() -> None: | ||
| # Note: Lowercase enum values (image, video, audio, document) must be added | ||
| # manually before running this migration using: | ||
| # ALTER TYPE attachmenttype ADD VALUE IF NOT EXISTS '<value>'; | ||
| # This is because PostgreSQL requires enum additions to be committed before use. | ||
|
|
||
| # Convert existing uppercase values to lowercase in message_attachments table | ||
| op.execute( | ||
| """ | ||
| UPDATE message_attachments | ||
| SET attachment_type = LOWER(attachment_type::text)::attachmenttype | ||
| WHERE attachment_type::text IN ('IMAGE', 'VIDEO', 'AUDIO', 'DOCUMENT') | ||
| """ | ||
| ) |
There was a problem hiding this comment.
Add enum values inside the migration instead of a manual pre-step.
The cast to attachmenttype will throw unless 'image', 'video', 'audio', and 'document' already exist on the enum. Relying on manual SQL before the migration makes deploys fragile and will fail in CI. Declare those ALTER TYPE ... ADD VALUE statements here so the migration is self-contained.
- # Note: Lowercase enum values (image, video, audio, document) must be added
- # manually before running this migration using:
- # ALTER TYPE attachmenttype ADD VALUE IF NOT EXISTS '<value>';
- # This is because PostgreSQL requires enum additions to be committed before use.
-
- # Convert existing uppercase values to lowercase in message_attachments table
+ op.execute("ALTER TYPE attachmenttype ADD VALUE IF NOT EXISTS 'image'")
+ op.execute("ALTER TYPE attachmenttype ADD VALUE IF NOT EXISTS 'video'")
+ op.execute("ALTER TYPE attachmenttype ADD VALUE IF NOT EXISTS 'audio'")
+ op.execute("ALTER TYPE attachmenttype ADD VALUE IF NOT EXISTS 'document'")
+
+ # Convert existing uppercase values to lowercase in message_attachments table🤖 Prompt for AI Agents
In app/alembic/versions/1d75bb639a7e_convert_attachment_types_to_lowercase.py
around lines 21 to 34, the migration updates attachment_type to lowercase but
assumes the lowercase enum members already exist; add explicit ALTER TYPE ...
ADD VALUE IF NOT EXISTS statements for 'image', 'video', 'audio', and 'document'
before the UPDATE so the enum values are created in the migration itself (use
op.execute with ALTER TYPE attachmenttype ADD VALUE IF NOT EXISTS '<value>'; for
each value), then run the existing UPDATE as-is.
| @router.get("/conversations/{conversation_id}/context-usage") | ||
| async def get_context_usage( | ||
| conversation_id: str, | ||
| db: Session = Depends(get_db), | ||
| async_db: AsyncSession = Depends(get_async_db), | ||
| user=Depends(AuthService.check_auth), | ||
| ): |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Inconsistent endpoint definition pattern.
All other endpoints in this file are defined as static methods within the ConversationAPI class, but this new endpoint is defined as a standalone function. This creates inconsistency in the codebase structure and makes the API organization less predictable.
Move this endpoint into the ConversationAPI class as a static method to maintain consistency:
+ @staticmethod
-@router.get("/conversations/{conversation_id}/context-usage")
+ @router.get("/conversations/{conversation_id}/context-usage")
-async def get_context_usage(
+ async def get_context_usage(Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.14.3)
680-680: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
681-681: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
682-682: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
🤖 Prompt for AI Agents
In app/modules/conversations/conversations_router.py around lines 677-683, the
new GET endpoint is defined as a standalone function while all other endpoints
are static methods on the ConversationAPI class; move this function into the
ConversationAPI class as a @staticmethod: cut the function body and its
@router.get decorator from the top-level, paste it inside the ConversationAPI
class, add the @staticmethod decorator above the method, keep the existing
@router.get(...) decorator, preserve the same signature (conversation_id: str,
db: Session = Depends(get_db), async_db: AsyncSession = Depends(get_async_db),
user=Depends(AuthService.check_auth)), adjust indentation to class scope, and
remove the original top-level function to restore consistent endpoint
organization.
| if encoding_name not in self._encoders: | ||
| try: | ||
| actual_encoding = encoding_map.get(encoding_name, "cl100k_base") | ||
| self._encoders[encoding_name] = tiktoken.get_encoding(actual_encoding) | ||
| except Exception as e: | ||
| logger.warning( | ||
| f"Failed to get encoding {encoding_name}: {e}, using cl100k_base" | ||
| ) | ||
| self._encoders[encoding_name] = tiktoken.get_encoding("cl100k_base") |
There was a problem hiding this comment.
Restore correct encoder lookup for GPT-4o models.
encoding_name ends up holding the encoder string ("o200k_base"), but we then look it up again in encoding_map. Because "o200k_base" is not a key, we always fall back to "cl100k_base", so GPT-4o/O4 models get tokenized with the wrong encoding. That undercounts tokens for 200K-context models and can let oversized attachments slip past validation. Cache by the model key and pass the real encoding string to tiktoken.get_encoding.
- encoding_name = "default"
+ encoding_key = "default"
model_lower = model.lower()
- for key, enc_name in encoding_map.items():
+ for key, enc_name in encoding_map.items():
if key in model_lower:
- encoding_name = enc_name
+ encoding_key = key
break
-
- # Cache encoder
- if encoding_name not in self._encoders:
+ encoding_name = encoding_map[encoding_key]
+
+ if encoding_name not in self._encoders:
try:
- actual_encoding = encoding_map.get(encoding_name, "cl100k_base")
- self._encoders[encoding_name] = tiktoken.get_encoding(actual_encoding)
+ self._encoders[encoding_name] = tiktoken.get_encoding(encoding_name)Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.14.3)
45-45: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In app/modules/intelligence/provider/token_counter.py around lines 41-49, the
code incorrectly looks up encoding_name in encoding_map (treating an encoding
string like "o200k_base" as a model key) causing fallback to cl100k_base;
instead, cache encoders by the model key and resolve the actual encoding string
correctly by: use the model identifier as the cache key, and when computing
actual_encoding, if encoding_name corresponds to a model key use
encoding_map[model_key] else treat encoding_name as already the encoding string
(i.e., actual_encoding = encoding_map.get(model_key, encoding_name)); then call
tiktoken.get_encoding(actual_encoding) and store it under the model key in
self._encoders.
| LOCAL = "LOCAL" | ||
| GCS = "GCS" | ||
| S3 = "S3" # Future extension | ||
| AZURE = "AZURE" # Future extension |
There was a problem hiding this comment.
Revert StorageProvider value change or add a migration.
Switching the enum values to uppercase breaks persistence: the existing rows contain 'local'/'gcs', and the PostgreSQL enum backing this column only knows the lowercase literals. After this change SQLAlchemy will raise on load ('gcs' is not a valid StorageProvider) and any new write of 'GCS' will fail because the database enum lacks that value. Keep the lowercase values or ship a migration that alters the enum type and backfills the data before merging.
- LOCAL = "LOCAL"
- GCS = "GCS"
- S3 = "S3" # Future extension
- AZURE = "AZURE" # Future extension
+ LOCAL = "local"
+ GCS = "gcs"
+ S3 = "s3" # Future extension
+ AZURE = "azure" # Future extension📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| LOCAL = "LOCAL" | |
| GCS = "GCS" | |
| S3 = "S3" # Future extension | |
| AZURE = "AZURE" # Future extension | |
| LOCAL = "local" | |
| GCS = "gcs" | |
| S3 = "s3" # Future extension | |
| AZURE = "azure" # Future extension |
| elif "csv" in mime_type or mime_type == "text/csv": | ||
| return self._extract_csv(file_data) | ||
| elif ( | ||
| "spreadsheet" in mime_type | ||
| or "vnd.openxmlformats-officedocument.spreadsheetml" in mime_type | ||
| ): | ||
| return self._extract_xlsx(file_data) | ||
| elif mime_type == "text/plain" or mime_type.startswith("text/"): | ||
| return self._extract_text_file(file_data, mime_type) | ||
| elif self._is_code_file(file_name, mime_type): | ||
| return self._extract_code_file(file_data) | ||
| else: |
There was a problem hiding this comment.
Check for code files before the generic text branch.
Because the text/* clause executes first, .py/.ts/.js uploads that arrive as text/x-* are always treated as plain text, so we never set the code_decode metadata or is_code=True. Downstream logic relying on that flag will never see code attachments. Evaluate _is_code_file before the generic text/ handler.
- elif "csv" in mime_type or mime_type == "text/csv":
+ elif "csv" in mime_type or mime_type == "text/csv":
return self._extract_csv(file_data)
elif (
"spreadsheet" in mime_type
or "vnd.openxmlformats-officedocument.spreadsheetml" in mime_type
):
return self._extract_xlsx(file_data)
- elif mime_type == "text/plain" or mime_type.startswith("text/"):
- return self._extract_text_file(file_data, mime_type)
elif self._is_code_file(file_name, mime_type):
return self._extract_code_file(file_data)
+ elif mime_type == "text/plain" or mime_type.startswith("text/"):
+ return self._extract_text_file(file_data, mime_type)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| elif "csv" in mime_type or mime_type == "text/csv": | |
| return self._extract_csv(file_data) | |
| elif ( | |
| "spreadsheet" in mime_type | |
| or "vnd.openxmlformats-officedocument.spreadsheetml" in mime_type | |
| ): | |
| return self._extract_xlsx(file_data) | |
| elif mime_type == "text/plain" or mime_type.startswith("text/"): | |
| return self._extract_text_file(file_data, mime_type) | |
| elif self._is_code_file(file_name, mime_type): | |
| return self._extract_code_file(file_data) | |
| else: | |
| elif "csv" in mime_type or mime_type == "text/csv": | |
| return self._extract_csv(file_data) | |
| elif ( | |
| "spreadsheet" in mime_type | |
| or "vnd.openxmlformats-officedocument.spreadsheetml" in mime_type | |
| ): | |
| return self._extract_xlsx(file_data) | |
| elif self._is_code_file(file_name, mime_type): | |
| return self._extract_code_file(file_data) | |
| elif mime_type == "text/plain" or mime_type.startswith("text/"): | |
| return self._extract_text_file(file_data, mime_type) | |
| else: |
🤖 Prompt for AI Agents
In app/modules/media/text_extraction_service.py around lines 82 to 93, the
branch that treats "text/*" mime types runs before the _is_code_file check,
causing code files with mime types like text/x-python to be handled as generic
text and never get code_decode/is_code metadata; move the
_is_code_file(mime_type, file_name) check so it executes before the generic
text/plain or text/* branch and, when true, call _extract_code_file(file_data)
(retaining existing csv/xlsx checks order), ensuring code files are detected and
processed with code-specific metadata.
Summary by CodeRabbit
Release Notes