OpenAI Conversation: Lookup #297
Conversation
WalkthroughThis change integrates robust conversation ancestry tracking and persistence into the OpenAI conversation response flow. It introduces a new utility for determining ancestor response IDs, updates API endpoints to persist conversation metadata, and adds comprehensive tests for both core logic and API interactions. Minor import order adjustments and formatting cleanups are also included. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant DB
participant OpenAI
participant Tracer
Client->>API: POST /responses or /responses/sync
API->>DB: Query latest conversation by ancestor ID (if provided)
API->>OpenAI: Generate response (with previous_response_id if found)
OpenAI-->>API: Return response (with response_id)
API->>DB: get_ancestor_id_from_response (resolve ancestry)
API->>DB: Create conversation record (with metadata)
API->>Tracer: Log trace (optional)
API-->>Client: Return response (with conversation info)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🔭 Outside diff range comments (1)
backend/app/api/routes/responses.py (1)
80-88: Critical bug in get_file_search_results functionThe function iterates over
resultsinstead of the actual search results, which will cause incorrect behavior.def get_file_search_results(response): results: list[FileResultChunk] = [] for tool_call in response.output: if tool_call.type == "file_search_call": results.extend( - [FileResultChunk(score=hit.score, text=hit.text) for hit in results] + [FileResultChunk(score=hit.score, text=hit.text) for hit in tool_call.results] ) return results
🧹 Nitpick comments (10)
backend/app/tests/utils/conversation.py (2)
18-41: Fix boolean comparison style and improve import organization.The function logic is sound, but there are style issues that should be addressed:
Apply this diff to improve the boolean comparisons:
- OpenAIConversation.is_deleted == False, + not OpenAIConversation.is_deleted,- OpenAIConversation.is_deleted == False, + not OpenAIConversation.is_deleted,- .where(OpenAIConversation.is_deleted == False) + .where(not OpenAIConversation.is_deleted)
47-47: Consider moving imports to module level.The import statement inside the function works but is unconventional. Consider moving utility imports to the top of the file for better readability and consistency.
+from app.tests.utils.utils import get_project, get_organization + def get_conversation( session: Session, response_id: str | None = None, project_id: int | None = None ) -> OpenAIConversation: """ Retrieve an active conversation from the database. ... if not conversation: # Create a new conversation if none exists - from app.tests.utils.utils import get_project, get_organizationbackend/app/api/routes/openai_conversation.py (1)
16-16: Remove unused import.The
OpenAIConversationCreateimport is not used in this module and should be removed.- OpenAIConversationCreate,backend/app/models/openai_conversation.py (1)
11-11: Consider using modern type annotation syntaxThe static analysis tool suggests using the newer
X | Nonesyntax instead ofOptional[X]for Python 3.10+. This is more concise and aligns with modern Python conventions.-from typing import Optional +from typing import TYPE_CHECKING - ancestor_response_id: Optional[str] = Field( + ancestor_response_id: str | None = Field( - previous_response_id: Optional[str] = Field( + previous_response_id: str | None = Field( - response: Optional[str] = Field(default=None, description="AI response") + response: str | None = Field(default=None, description="AI response") - deleted_at: Optional[datetime] = Field(default=None, nullable=True) + deleted_at: datetime | None = Field(default=None, nullable=True)Also applies to: 16-16, 20-20, 38-38, 47-47, 50-50, 54-54
backend/app/crud/openai_conversation.py (2)
2-2: Modernize type annotationsUpdate type annotations to use modern Python syntax.
-from typing import List, Optional +from typing import Optional -) -> Optional[OpenAIConversation]: +) -> OpenAIConversation | None: -) -> List[OpenAIConversation]: +) -> list[OpenAIConversation]:Also applies to: 12-12, 27-27, 42-42, 58-58, 101-101, 149-149
19-19: Use idiomatic boolean comparisonsReplace explicit
== Falsecomparisons with more idiomaticnotchecks.- OpenAIConversation.is_deleted == False, + not OpenAIConversation.is_deleted,Also applies to: 34-34, 49-49, 109-109
backend/app/tests/crud/test_openai_conversation.py (2)
1-1: Remove unused importThe
pytestimport is not used in this file.-import pytest from uuid import uuid4
291-291: Remove unused variable assignmentsThese variables are assigned but never used in the tests.
- ancestor_conversation = create_conversation( + create_conversation( - previous_conversation = create_conversation( + create_conversation(Also applies to: 310-310, 346-346
backend/app/tests/api/routes/test_openai_conversation.py (1)
1-1: Remove unused importsThese imports are not used in the test file.
-import pytest from uuid import uuid4 from sqlmodel import Session -from fastapi import HTTPException from fastapi.testclient import TestClientAlso applies to: 4-4
backend/app/api/routes/responses.py (1)
414-447: Consider extracting common conversation creation logicBoth async and sync endpoints have identical conversation creation logic. Consider extracting this into a shared function to follow DRY principles.
Would you like me to create a helper function to reduce code duplication between the async and sync endpoints?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
backend/app/alembic/versions/e9dd35eff62c_add_openai_conversation_table.py(1 hunks)backend/app/api/main.py(2 hunks)backend/app/api/routes/openai_conversation.py(1 hunks)backend/app/api/routes/responses.py(5 hunks)backend/app/api/routes/threads.py(1 hunks)backend/app/crud/__init__.py(1 hunks)backend/app/crud/openai_conversation.py(1 hunks)backend/app/models/__init__.py(1 hunks)backend/app/models/openai_conversation.py(1 hunks)backend/app/models/organization.py(2 hunks)backend/app/models/project.py(1 hunks)backend/app/tests/api/routes/test_openai_conversation.py(1 hunks)backend/app/tests/crud/test_openai_conversation.py(1 hunks)backend/app/tests/utils/conversation.py(1 hunks)backend/app/tests/utils/utils.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (7)
backend/app/tests/utils/utils.py (5)
backend/app/models/project.py (1)
Project(28-54)backend/app/models/assistants.py (1)
Assistant(29-40)backend/app/models/organization.py (1)
Organization(35-58)backend/app/seed_data/seed_data.py (1)
create_organization(82-93)backend/app/crud/organization.py (2)
get_organization_by_name(28-30)get_organization_by_id(23-25)
backend/app/crud/__init__.py (1)
backend/app/crud/openai_conversation.py (6)
get_conversation_by_id(10-22)get_conversation_by_response_id(25-37)get_conversation_by_ancestor_id(40-52)get_conversations_by_project(96-116)create_conversation(119-142)delete_conversation(145-168)
backend/app/models/organization.py (1)
backend/app/models/openai_conversation.py (1)
OpenAIConversation(31-42)
backend/app/alembic/versions/e9dd35eff62c_add_openai_conversation_table.py (3)
backend/app/alembic/versions/8eefcfedc409_create_assistant_table.py (1)
upgrade(20-40)backend/app/alembic/versions/99f4fc325617_add_organization_project_setup.py (1)
upgrade(20-72)backend/app/alembic/versions/e8ee93526b37_add_is_deleted_column_in_assistant_table.py (1)
upgrade(19-25)
backend/app/tests/crud/test_openai_conversation.py (4)
backend/app/crud/openai_conversation.py (7)
get_conversation_by_id(10-22)get_conversation_by_response_id(25-37)get_conversation_by_ancestor_id(40-52)get_conversations_by_project(96-116)create_conversation(119-142)delete_conversation(145-168)set_ancestor_response_id(55-93)backend/app/models/openai_conversation.py (1)
OpenAIConversationCreate(45-58)backend/app/tests/utils/conversation.py (1)
get_conversation(8-80)backend/app/tests/utils/utils.py (2)
get_project(70-89)get_organization(116-137)
backend/app/models/openai_conversation.py (4)
backend/app/models/threads.py (2)
OpenAIThreadBase(6-11)OpenAI_Thread(18-21)backend/app/models/assistants.py (1)
Assistant(29-40)backend/app/models/collection.py (1)
Collection(21-53)backend/app/alembic/versions/e8ee93526b37_add_is_deleted_column_in_assistant_table.py (1)
upgrade(19-25)
backend/app/api/routes/responses.py (3)
backend/app/crud/openai_conversation.py (2)
create_conversation(119-142)set_ancestor_response_id(55-93)backend/app/models/openai_conversation.py (1)
OpenAIConversationCreate(45-58)backend/app/utils.py (1)
mask_string(156-165)
🪛 Ruff (0.12.2)
backend/app/crud/__init__.py
59-59: .openai_conversation.get_conversation_by_id imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
60-60: .openai_conversation.get_conversation_by_response_id imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
61-61: .openai_conversation.get_conversation_by_ancestor_id imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
62-62: .openai_conversation.get_conversations_by_project imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
63-63: .openai_conversation.create_conversation imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
64-64: .openai_conversation.delete_conversation imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
backend/app/models/project.py
52-52: Undefined name OpenAIConversation
(F821)
backend/app/models/__init__.py
60-60: .openai_conversation.OpenAIConversation imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
61-61: .openai_conversation.OpenAIConversationBase imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
62-62: .openai_conversation.OpenAIConversationCreate imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
backend/app/tests/utils/conversation.py
23-23: Avoid equality comparisons to False; use not OpenAIConversation.is_deleted: for false checks
Replace with not OpenAIConversation.is_deleted
(E712)
32-32: Avoid equality comparisons to False; use not OpenAIConversation.is_deleted: for false checks
Replace with not OpenAIConversation.is_deleted
(E712)
39-39: Avoid equality comparisons to False; use not OpenAIConversation.is_deleted: for false checks
Replace with not OpenAIConversation.is_deleted
(E712)
backend/app/tests/api/routes/test_openai_conversation.py
1-1: pytest imported but unused
Remove unused import: pytest
(F401)
4-4: fastapi.HTTPException imported but unused
Remove unused import: fastapi.HTTPException
(F401)
backend/app/tests/crud/test_openai_conversation.py
1-1: pytest imported but unused
Remove unused import: pytest
(F401)
291-291: Local variable ancestor_conversation is assigned to but never used
Remove assignment to unused variable ancestor_conversation
(F841)
310-310: Local variable previous_conversation is assigned to but never used
Remove assignment to unused variable previous_conversation
(F841)
346-346: Local variable previous_conversation is assigned to but never used
Remove assignment to unused variable previous_conversation
(F841)
backend/app/models/openai_conversation.py
11-11: Use X | None for type annotations
Convert to X | None
(UP045)
16-16: Use X | None for type annotations
Convert to X | None
(UP045)
20-20: Use X | None for type annotations
Convert to X | None
(UP045)
38-38: Use X | None for type annotations
Convert to X | None
(UP045)
41-41: Undefined name Project
(F821)
42-42: Undefined name Organization
(F821)
47-47: Use X | None for type annotations
Convert to X | None
(UP045)
50-50: Use X | None for type annotations
Convert to X | None
(UP045)
54-54: Use X | None for type annotations
Convert to X | None
(UP045)
backend/app/api/routes/openai_conversation.py
16-16: app.models.OpenAIConversationCreate imported but unused
Remove unused import: app.models.OpenAIConversationCreate
(F401)
backend/app/crud/openai_conversation.py
2-2: typing.List is deprecated, use list instead
(UP035)
12-12: Use X | None for type annotations
Convert to X | None
(UP045)
19-19: Avoid equality comparisons to False; use not OpenAIConversation.is_deleted: for false checks
Replace with not OpenAIConversation.is_deleted
(E712)
27-27: Use X | None for type annotations
Convert to X | None
(UP045)
34-34: Avoid equality comparisons to False; use not OpenAIConversation.is_deleted: for false checks
Replace with not OpenAIConversation.is_deleted
(E712)
42-42: Use X | None for type annotations
Convert to X | None
(UP045)
49-49: Avoid equality comparisons to False; use not OpenAIConversation.is_deleted: for false checks
Replace with not OpenAIConversation.is_deleted
(E712)
58-58: Use X | None for type annotations
Convert to X | None
(UP045)
101-101: Use list instead of List for type annotation
Replace with list
(UP006)
109-109: Avoid equality comparisons to False; use not OpenAIConversation.is_deleted: for false checks
Replace with not OpenAIConversation.is_deleted
(E712)
149-149: Use X | None for type annotations
Convert to X | None
(UP045)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.11.7, 6)
🔇 Additional comments (22)
backend/app/api/main.py (1)
10-10: LGTM! Clean integration of the new OpenAI conversation routes.The import and router registration follow the established patterns and maintain proper alphabetical ordering. The integration is consistent with other route modules.
Also applies to: 31-31
backend/app/models/__init__.py (1)
59-63: LGTM! Model imports follow established patterns.The OpenAI conversation model imports are correctly added and follow the same pattern as other model imports in this
__init__.pyfile. The static analysis warnings about unused imports are false positives - these are re-exports that provide a clean import interface for the models package.backend/app/models/organization.py (1)
14-14: LGTM! Clean implementation of the OpenAI conversation relationship.The TYPE_CHECKING import prevents circular dependencies while maintaining type safety, and the relationship configuration follows the established pattern with proper
back_populatesandcascade_delete=Truesettings consistent with other relationships in the model.Also applies to: 56-58
backend/app/crud/__init__.py (1)
58-65: LGTM! Import additions follow established pattern.The new OpenAI conversation CRUD function imports are properly structured and consistent with the existing import organization in this module. The static analysis "unused import" warnings are false positives since this module serves as a re-export interface for CRUD operations.
backend/app/tests/utils/utils.py (2)
15-15: LGTM! Import addition is appropriate.The Organization import is correctly added to support the new utility function.
116-137: LGTM! Utility function follows established pattern.The
get_organizationfunction is well-implemented and consistent with the existingget_projectandget_assistantutility functions. It properly handles both named and generic organization retrieval with appropriate error handling.backend/app/tests/utils/conversation.py (1)
63-78: LGTM! Conversation creation logic is well-designed.The fallback logic to create a new conversation when none exists is appropriate for test utilities. The use of UUIDs for test data and proper integration with project/organization context is well-implemented.
backend/app/alembic/versions/e9dd35eff62c_add_openai_conversation_table.py (3)
21-46: LGTM! Well-designed table schema with proper constraints.The table schema is comprehensive and well-structured:
- Includes all necessary fields for conversation tracking and threading
- Proper foreign key relationships with CASCADE delete
- Supports soft deletion pattern
- Includes appropriate timestamp fields
47-70: Excellent indexing strategy for query optimization.The indexes on
response_id,previous_response_id, andancestor_response_idare well-chosen for the expected query patterns. The foreign key constraints are properly configured.
73-87: LGTM! Proper downgrade implementation.The downgrade function correctly reverses all changes made in the upgrade, following the proper order of operations (constraints first, then indexes, then table).
backend/app/api/routes/openai_conversation.py (5)
24-44: LGTM! Well-structured conversation retrieval endpoint.The endpoint properly handles conversation retrieval by ID with appropriate error handling and user context validation.
47-68: LGTM! Response ID lookup endpoint is well-implemented.The endpoint follows the same pattern as the ID lookup with proper path parameter handling and error responses.
71-94: LGTM! Ancestor lookup endpoint supports conversation threading.This endpoint enables conversation threading functionality by allowing lookups via ancestor response IDs, which is essential for the conversation tracking feature.
97-114: LGTM! Listing endpoint with proper pagination controls.The pagination parameters are well-configured with sensible defaults and limits. The endpoint properly scopes results to the current user's project.
117-139: LGTM! Soft delete endpoint with proper error handling.The delete endpoint correctly implements soft deletion and provides meaningful success/error responses. The use of
Annotatedfor the path parameter is a good practice.backend/app/models/openai_conversation.py (2)
9-29: Well-structured base model with appropriate indexesThe base model is well-designed with:
- Proper indexes on response ID fields for efficient lookups
- CASCADE delete on foreign keys ensuring referential integrity
- Clear field descriptions
45-58: Good validation constraints on create schemaThe create schema properly validates input with
min_lengthconstraints on required string fields, ensuring data quality.backend/app/crud/openai_conversation.py (1)
96-117: Well-implemented pagination with sensible defaultsThe function properly implements pagination with:
- Reasonable default limits
- Proper ordering by insertion time
- Efficient query construction
backend/app/tests/crud/test_openai_conversation.py (1)
125-158: Good test coverage for listing and paginationThe tests properly verify:
- Multiple conversation creation
- Filtering by project
- Soft-delete exclusion
- Pagination limits
backend/app/tests/api/routes/test_openai_conversation.py (2)
10-35: Well-structured API test with proper project scopingThe test correctly:
- Gets project ID from API key
- Creates conversation in the same project
- Verifies response structure
179-199: Ignore project_id concern for test data creationThe
get_conversationhelper inbackend/app/tests/utils/conversation.pyfalls back to the active project viaget_project(session)when noproject_idis provided. This guarantees that all calls toget_conversation(db)will create or return conversations in the same project, so there’s no risk of them landing in different projects.Likely an incorrect or invalid review comment.
backend/app/api/routes/responses.py (1)
175-208: Good integration with proper error handlingThe conversation creation is well-integrated:
- Properly sets ancestor response ID
- Includes comprehensive error handling that doesn't interrupt response flow
- Logs both success and failure cases
backend/app/api/routes/threads.py
Outdated
| session = requests.Session() | ||
| # uncomment this to run locally without SSL | ||
| # session.verify = False | ||
| session.verify = False |
There was a problem hiding this comment.
Critical security issue: SSL verification disabled unconditionally.
Disabling SSL certificate verification (session.verify = False) creates a serious security vulnerability that allows man-in-the-middle attacks. While the comment suggests this is for local development, it will apply to all environments including production.
Apply this diff to make SSL verification conditional based on environment:
- session.verify = False
+ # Disable SSL verification only in local development
+ if settings.ENVIRONMENT == "local":
+ session.verify = FalseAlternatively, consider adding a specific configuration setting for SSL verification:
- session.verify = False
+ session.verify = getattr(settings, 'VERIFY_SSL_CERTIFICATES', True)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In backend/app/api/routes/threads.py at line 42, the code disables SSL
verification unconditionally by setting session.verify = False, which is a
critical security risk. Modify the code to conditionally disable SSL
verification only in local development or non-production environments by
checking an environment variable or configuration setting. Implement a
configuration flag that controls SSL verification and set session.verify
accordingly, ensuring SSL verification remains enabled in production.
| project: "Project" = Relationship(back_populates="openai_conversations") | ||
| organization: "Organization" = Relationship(back_populates="openai_conversations") |
There was a problem hiding this comment.
Add TYPE_CHECKING import for relationship type hints
The static analysis correctly identifies that Project and Organization are undefined. Use TYPE_CHECKING to import these types without causing circular imports.
-from typing import Optional
+from typing import TYPE_CHECKING, Optional
+
+if TYPE_CHECKING:
+ from app.models import Project, Organization
from sqlmodel import Field, Relationship, SQLModel📝 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.
| project: "Project" = Relationship(back_populates="openai_conversations") | |
| organization: "Organization" = Relationship(back_populates="openai_conversations") | |
| from typing import TYPE_CHECKING, Optional | |
| if TYPE_CHECKING: | |
| from app.models import Project, Organization | |
| from sqlmodel import Field, Relationship, SQLModel |
🧰 Tools
🪛 Ruff (0.12.2)
41-41: Undefined name Project
(F821)
42-42: Undefined name Organization
(F821)
🤖 Prompt for AI Agents
In backend/app/models/openai_conversation.py around lines 41 to 42, the type
hints for Project and Organization cause static analysis errors due to undefined
names. To fix this, import Project and Organization inside a TYPE_CHECKING block
from typing to avoid circular imports. Add "from typing import TYPE_CHECKING" at
the top, then conditionally import Project and Organization only when
TYPE_CHECKING is True.
| openai_conversations: list["OpenAIConversation"] = Relationship( | ||
| back_populates="project", cascade_delete=True | ||
| ) |
There was a problem hiding this comment.
Missing import for OpenAIConversation type annotation.
The OpenAIConversation type is referenced in the relationship but not imported, which will cause runtime errors. Following the pattern used in organization.py, add the import under TYPE_CHECKING.
Apply this diff to add the missing import:
+from typing import Optional, List, TYPE_CHECKING
-from typing import Optional, List
from sqlmodel import Field, Relationship, SQLModel
from app.core.util import now
+
+if TYPE_CHECKING:
+ from .openai_conversation import OpenAIConversationAnd update the existing TYPE_CHECKING block if it exists, or create it as shown above.
📝 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.
| openai_conversations: list["OpenAIConversation"] = Relationship( | |
| back_populates="project", cascade_delete=True | |
| ) | |
| from typing import Optional, List, TYPE_CHECKING | |
| from sqlmodel import Field, Relationship, SQLModel | |
| from app.core.util import now | |
| if TYPE_CHECKING: | |
| from .openai_conversation import OpenAIConversation | |
| openai_conversations: list["OpenAIConversation"] = Relationship( | |
| back_populates="project", cascade_delete=True | |
| ) |
🧰 Tools
🪛 Ruff (0.12.2)
52-52: Undefined name OpenAIConversation
(F821)
🤖 Prompt for AI Agents
In backend/app/models/project.py around lines 52 to 54, the OpenAIConversation
type used in the relationship annotation is not imported, causing runtime
errors. To fix this, add an import for OpenAIConversation inside a TYPE_CHECKING
block at the top of the file, following the pattern used in organization.py. If
a TYPE_CHECKING block already exists, add the import there; otherwise, create a
new TYPE_CHECKING block and import OpenAIConversation from its module.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/app/crud/openai_conversation.py (1)
55-94: The previous inconsistency has been resolved.The comment and implementation are now consistent - both indicate that when the previous conversation is not found,
previous_response_idis returned as the ancestor. The logic is correct.However, consider updating the type annotation:
def set_ancestor_response_id( session: Session, current_response_id: str, - previous_response_id: Optional[str], + previous_response_id: str | None, project_id: int, ) -> str:
🧹 Nitpick comments (6)
backend/app/crud/openai_conversation.py (6)
2-2: Modernize type annotations.The
typing.Listimport is deprecated in favor of the built-inlisttype for Python 3.9+.-from typing import List, Optional +from typing import Optional
10-22: Apply modern type annotation and comparison style.The function logic is correct, but consider these style improvements:
def get_conversation_by_id( - session: Session, conversation_id: int, project_id: int -) -> Optional[OpenAIConversation]: + session: Session, conversation_id: int, project_id: int +) -> OpenAIConversation | None: """ Return a conversation by its ID and project. """ statement = select(OpenAIConversation).where( OpenAIConversation.id == conversation_id, OpenAIConversation.project_id == project_id, - OpenAIConversation.is_deleted == False, + not OpenAIConversation.is_deleted, )
25-37: Apply modern type annotation and comparison style.Same style improvements as the previous function:
def get_conversation_by_response_id( - session: Session, response_id: str, project_id: int -) -> Optional[OpenAIConversation]: + session: Session, response_id: str, project_id: int +) -> OpenAIConversation | None: """ Return a conversation by its OpenAI response ID and project. """ statement = select(OpenAIConversation).where( OpenAIConversation.response_id == response_id, OpenAIConversation.project_id == project_id, - OpenAIConversation.is_deleted == False, + not OpenAIConversation.is_deleted, )
40-52: Apply modern type annotation and comparison style.Same style improvements needed:
def get_conversation_by_ancestor_id( - session: Session, ancestor_response_id: str, project_id: int -) -> Optional[OpenAIConversation]: + session: Session, ancestor_response_id: str, project_id: int +) -> OpenAIConversation | None: """ Return a conversation by its ancestor response ID and project. """ statement = select(OpenAIConversation).where( OpenAIConversation.ancestor_response_id == ancestor_response_id, OpenAIConversation.project_id == project_id, - OpenAIConversation.is_deleted == False, + not OpenAIConversation.is_deleted, )
96-116: Apply modern type annotation and comparison style.The function logic and pagination approach are good. Apply these style improvements:
def get_conversations_by_project( session: Session, project_id: int, skip: int = 0, limit: int = 100, -) -> List[OpenAIConversation]: +) -> list[OpenAIConversation]: """ Return all conversations for a given project, with optional pagination. """ statement = ( select(OpenAIConversation) .where( OpenAIConversation.project_id == project_id, - OpenAIConversation.is_deleted == False, + not OpenAIConversation.is_deleted, )
145-168: Well-implemented soft delete pattern.The soft delete logic is correct with proper error handling and logging. Consider updating the type annotation for consistency:
def delete_conversation( session: Session, conversation_id: int, project_id: int, -) -> Optional[OpenAIConversation]: +) -> OpenAIConversation | None:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/crud/openai_conversation.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.12.2)
backend/app/crud/openai_conversation.py
2-2: typing.List is deprecated, use list instead
(UP035)
12-12: Use X | None for type annotations
Convert to X | None
(UP045)
19-19: Avoid equality comparisons to False; use not OpenAIConversation.is_deleted: for false checks
Replace with not OpenAIConversation.is_deleted
(E712)
27-27: Use X | None for type annotations
Convert to X | None
(UP045)
34-34: Avoid equality comparisons to False; use not OpenAIConversation.is_deleted: for false checks
Replace with not OpenAIConversation.is_deleted
(E712)
42-42: Use X | None for type annotations
Convert to X | None
(UP045)
49-49: Avoid equality comparisons to False; use not OpenAIConversation.is_deleted: for false checks
Replace with not OpenAIConversation.is_deleted
(E712)
58-58: Use X | None for type annotations
Convert to X | None
(UP045)
101-101: Use list instead of List for type annotation
Replace with list
(UP006)
109-109: Avoid equality comparisons to False; use not OpenAIConversation.is_deleted: for false checks
Replace with not OpenAIConversation.is_deleted
(E712)
149-149: Use X | None for type annotations
Convert to X | None
(UP045)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.11.7, 6)
🔇 Additional comments (1)
backend/app/crud/openai_conversation.py (1)
119-142: LGTM!The conversation creation logic is well-implemented with proper session management, transaction handling, and logging for traceability.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
backend/app/crud/openai_conversation.py (1)
2-4: Remove unused import and use modern type annotations.The static analysis correctly identifies issues with the imports and type annotations.
Apply this diff to fix the import and type annotation issues:
-from typing import List, Optional +from typing import Optional - previous_response_id: Optional[str], + previous_response_id: str | None,The
Listimport is unused, andOptional[str]should be updated to the modernstr | Nonesyntax.backend/app/api/routes/openai_conversation.py (1)
18-19: Remove unused imports.The static analysis correctly identifies that
OpenAIConversationCreateandOpenAIConversationimports are not used in this file.Apply this diff to remove the unused imports:
from app.models import ( UserProjectOrg, - OpenAIConversationCreate, - OpenAIConversation, OpenAIConversationPublic, )backend/app/models/openai_conversation.py (1)
114-114: Add missing newline at end of file.- use_enum_values = True + use_enum_values = True +backend/app/tests/api/routes/test_openai_conversation.py (2)
1-4: Remove unused imports.The static analysis correctly identifies several unused imports.
Apply this diff to remove the unused imports:
-import pytest -from uuid import uuid4 from sqlmodel import Session -from fastapi import HTTPException from fastapi.testclient import TestClient
532-532: Add missing newline at end of file.assert response.status_code == 401 +backend/app/tests/crud/test_openai_conversation.py (1)
41-41: Clean up whitespace in blank lines.Remove whitespace from these blank lines:
) - + retrieved_conversation = get_conversation_by_id() - + deleted_conversation = delete_conversation(ancestor_response_id=None, - +Also applies to: 249-249, 644-644
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/app/api/routes/openai_conversation.py(1 hunks)backend/app/crud/openai_conversation.py(2 hunks)backend/app/models/openai_conversation.py(2 hunks)backend/app/tests/api/routes/test_openai_conversation.py(3 hunks)backend/app/tests/crud/test_openai_conversation.py(6 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
backend/app/api/routes/openai_conversation.py (1)
backend/app/alembic/versions/e9dd35eff62c_add_openai_conversation_table.py (1)
upgrade(20-70)
backend/app/tests/api/routes/test_openai_conversation.py (2)
backend/app/crud/openai_conversation.py (1)
create_conversation(138-161)backend/app/models/api_key.py (1)
APIKeyPublic(23-25)
🪛 Ruff (0.12.2)
backend/app/tests/crud/test_openai_conversation.py
41-41: Blank line contains whitespace
(W293)
249-249: Blank line contains whitespace
(W293)
644-644: Blank line contains whitespace
(W293)
backend/app/api/routes/openai_conversation.py
18-18: app.models.OpenAIConversationCreate imported but unused
Remove unused import
(F401)
19-19: app.models.OpenAIConversation imported but unused
Remove unused import
(F401)
backend/app/crud/openai_conversation.py
2-2: typing.List is deprecated, use list instead
(UP035)
2-2: typing.List imported but unused
Remove unused import: typing.List
(F401)
63-63: Use X | None for type annotations
Convert to X | None
(UP045)
backend/app/models/openai_conversation.py
114-114: No newline at end of file
(W292)
backend/app/tests/api/routes/test_openai_conversation.py
1-1: pytest imported but unused
Remove unused import: pytest
(F401)
2-2: uuid.uuid4 imported but unused
Remove unused import: uuid.uuid4
(F401)
4-4: fastapi.HTTPException imported but unused
Remove unused import: fastapi.HTTPException
(F401)
532-532: No newline at end of file
Add trailing newline
(W292)
🪛 GitHub Actions: AI Platform CI
backend/app/models/openai_conversation.py
[error] 12-12: SyntaxError: '(' was never closed at line 12.
🔇 Additional comments (5)
backend/app/crud/openai_conversation.py (1)
60-98: LGTM! Well-implemented ancestor resolution logic.The
set_ancestor_response_idfunction implements sound conversation threading logic:
- Returns
current_response_idwhen starting a new conversation chain- Properly queries for previous conversations within project scope
- Inherits ancestor from previous conversation or falls back to previous_response_id
The function correctly handles all edge cases and maintains conversation ancestry consistency.
backend/app/tests/crud/test_openai_conversation.py (4)
326-338: LGTM! Good test coverage for no previous response scenario.The test correctly verifies that when
previous_response_idis None, the function returnscurrent_response_idas the ancestor. This matches the expected behavior for starting a new conversation chain.
341-356: LGTM! Good test coverage for previous response not found scenario.The test correctly verifies that when
previous_response_idis not found in the database, the function returnsprevious_response_iditself as the ancestor. This is the appropriate fallback behavior.
358-470: LGTM! Comprehensive test coverage for conversation chain scenarios.These tests thoroughly cover:
- Previous conversation found with ancestor (inherits ancestor)
- Previous conversation found without ancestor (returns None)
- Cross-project scoping verification
- Complex conversation chains
The test logic correctly matches the implementation behavior.
636-787: LGTM! Good test coverage for conversation creation.The tests comprehensively verify conversation creation with and without ancestor/previous response IDs, ensuring all fields are properly set and database constraints are maintained.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
backend/app/tests/crud/test_openai_conversation.py (2)
379-384: Remove unused variable assignmentsThe
ancestor_conversationandprevious_conversationvariables are assigned but never used. You can remove these assignments sincecreate_conversationis only called for its side effects.- ancestor_conversation = create_conversation( + create_conversation( session=db, conversation=ancestor_conversation_data, project_id=project.id, organization_id=organization.id, ) # ... later in the code ... - previous_conversation = create_conversation( + create_conversation( session=db, conversation=previous_conversation_data, project_id=project.id, organization_id=organization.id, )Also applies to: 398-403
508-513: Remove unused variable assignmentThe
previous_conversationvariable is assigned but never used.- previous_conversation = create_conversation( + create_conversation( session=db, conversation=previous_conversation_data, project_id=project.id, organization_id=organization.id, )backend/app/tests/api/routes/test_responses.py (1)
1-10: Remove unused importsSeveral imports are not used in the test file.
-from unittest.mock import MagicMock, patch, AsyncMock +from unittest.mock import MagicMock, patch import pytest from fastapi import FastAPI from fastapi.testclient import TestClient from sqlmodel import select import openai from app.api.routes.responses import router -from app.models import Project, Assistant -from app.core.exception_handlers import HTTPException +from app.models import Project
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/app/api/routes/responses.py(6 hunks)backend/app/models/openai_conversation.py(1 hunks)backend/app/tests/api/routes/test_responses.py(5 hunks)backend/app/tests/crud/test_openai_conversation.py(4 hunks)backend/app/tests/scripts/test_backend_pre_start.py(1 hunks)backend/app/tests/scripts/test_test_pre_start.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- backend/app/models/openai_conversation.py
🧰 Additional context used
🪛 Ruff (0.12.2)
backend/app/tests/api/routes/test_responses.py
1-1: unittest.mock.AsyncMock imported but unused
Remove unused import: unittest.mock.AsyncMock
(F401)
9-9: app.models.Assistant imported but unused
Remove unused import: app.models.Assistant
(F401)
10-10: app.core.exception_handlers.HTTPException imported but unused
Remove unused import: app.core.exception_handlers.HTTPException
(F401)
179-179: Unused function argument: db
(ARG001)
203-203: Unused function argument: db
(ARG001)
236-236: Unused function argument: db
(ARG001)
275-275: Unused function argument: db
(ARG001)
334-334: Unused function argument: db
(ARG001)
366-366: Unused function argument: db
(ARG001)
backend/app/tests/crud/test_openai_conversation.py
379-379: Local variable ancestor_conversation is assigned to but never used
Remove assignment to unused variable ancestor_conversation
(F841)
398-398: Local variable previous_conversation is assigned to but never used
Remove assignment to unused variable previous_conversation
(F841)
508-508: Local variable previous_conversation is assigned to but never used
Remove assignment to unused variable previous_conversation
(F841)
backend/app/api/routes/responses.py
40-40: Do not use bare except
(E722)
🔇 Additional comments (3)
backend/app/tests/scripts/test_test_pre_start.py (1)
31-33: Good fix for the mock assertion!Correctly replaced the non-existent
called_once_withattribute with the properassert_called_once_withmethod from the mock library.backend/app/tests/scripts/test_backend_pre_start.py (1)
31-33: Consistent fix across test files!Same correction as in
test_test_pre_start.py- properly usingassert_called_once_withfor mock assertion.backend/app/tests/api/routes/test_responses.py (1)
18-495: Excellent test coverage!The tests comprehensively cover various scenarios including success paths, error handling, and edge cases for both async and sync endpoints. Good use of mocking and assertions.
| def handle_openai_error(e: openai.OpenAIError) -> str: | ||
| """Extract error message from OpenAI error.""" | ||
| if isinstance(e.body, dict) and "message" in e.body: | ||
| # Try to get error message from different possible attributes | ||
| if hasattr(e, "body") and isinstance(e.body, dict) and "message" in e.body: | ||
| return e.body["message"] | ||
| elif hasattr(e, "message"): | ||
| return e.message | ||
| elif hasattr(e, "response") and hasattr(e.response, "json"): | ||
| try: | ||
| error_data = e.response.json() | ||
| if isinstance(error_data, dict) and "error" in error_data: | ||
| error_info = error_data["error"] | ||
| if isinstance(error_info, dict) and "message" in error_info: | ||
| return error_info["message"] | ||
| except: | ||
| pass | ||
| return str(e) |
There was a problem hiding this comment.
Improve exception handling specificity
The enhanced error message extraction logic is good, but the bare except clause should be more specific to avoid catching system exceptions.
if isinstance(error_info, dict) and "message" in error_info:
return error_info["message"]
- except:
+ except (AttributeError, TypeError, ValueError):
pass📝 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.
| def handle_openai_error(e: openai.OpenAIError) -> str: | |
| """Extract error message from OpenAI error.""" | |
| if isinstance(e.body, dict) and "message" in e.body: | |
| # Try to get error message from different possible attributes | |
| if hasattr(e, "body") and isinstance(e.body, dict) and "message" in e.body: | |
| return e.body["message"] | |
| elif hasattr(e, "message"): | |
| return e.message | |
| elif hasattr(e, "response") and hasattr(e.response, "json"): | |
| try: | |
| error_data = e.response.json() | |
| if isinstance(error_data, dict) and "error" in error_data: | |
| error_info = error_data["error"] | |
| if isinstance(error_info, dict) and "message" in error_info: | |
| return error_info["message"] | |
| except: | |
| pass | |
| return str(e) | |
| def handle_openai_error(e: openai.OpenAIError) -> str: | |
| """Extract error message from OpenAI error.""" | |
| # Try to get error message from different possible attributes | |
| if hasattr(e, "body") and isinstance(e.body, dict) and "message" in e.body: | |
| return e.body["message"] | |
| elif hasattr(e, "message"): | |
| return e.message | |
| elif hasattr(e, "response") and hasattr(e.response, "json"): | |
| try: | |
| error_data = e.response.json() | |
| if isinstance(error_data, dict) and "error" in error_data: | |
| error_info = error_data["error"] | |
| if isinstance(error_info, dict) and "message" in error_info: | |
| return error_info["message"] | |
| except (AttributeError, TypeError, ValueError): | |
| pass | |
| return str(e) |
🧰 Tools
🪛 Ruff (0.12.2)
40-40: Do not use bare except
(E722)
🤖 Prompt for AI Agents
In backend/app/api/routes/responses.py between lines 26 and 42, the bare except
clause in the error handling should be replaced with a more specific exception,
such as catching JSONDecodeError or a relevant exception type from the
response.json() call, to avoid unintentionally catching system-level exceptions.
Update the except clause to catch only expected exceptions related to JSON
parsing or attribute errors.
…rojectTech4DevAI/ai-platform into enhancement/openai-conversation-lookup
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/app/api/routes/responses.py (1)
27-43: Address the bare except clause (duplicate of past review)The enhanced error message extraction logic is good, but the bare
exceptclause should be more specific to avoid catching system exceptions, as noted in the previous review.Apply this fix:
if isinstance(error_info, dict) and "message" in error_info: return error_info["message"] - except: + except (AttributeError, TypeError, ValueError, KeyError): pass
🧹 Nitpick comments (3)
backend/app/tests/scripts/test_backend_pre_start.py (1)
23-23: Formatting fix applied correctly.The missing trailing newline has been automatically corrected by the pre-commit hook 'end-of-file-fixer', which aligns with Python code style standards (PEP 8 recommends files end with a newline).
backend/app/tests/api/routes/test_responses.py (2)
1-11: Clean up unused importsSeveral imports are flagged as unused by static analysis and should be removed to improve code quality.
Remove these unused imports:
-from unittest.mock import MagicMock, patch, AsyncMock +from unittest.mock import MagicMock, patch import pytest from fastapi import FastAPI from fastapi.testclient import TestClient from sqlmodel import select import openai from app.api.routes.responses import router -from app.models import Project, Assistant +from app.models import Project -from app.core.exception_handlers import HTTPException
26-1054: Consider cleaning up unused mock argumentsStatic analysis flags several unused function arguments in test functions. While these don't affect functionality, removing unused mocks would improve code quality.
For example, in some tests like
test_responses_endpoint_success,mock_get_conversation_by_ancestor_idis mocked but never used because those tests don't provide aresponse_id. Consider only mocking what's actually needed in each test, or use# noqa: ARG001comments to suppress the warnings if the comprehensive mocking is intentional for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/app/api/routes/responses.py(8 hunks)backend/app/tests/api/routes/test_responses.py(5 hunks)backend/app/tests/scripts/test_backend_pre_start.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.12.2)
backend/app/api/routes/responses.py
41-41: Do not use bare except
(E722)
backend/app/tests/api/routes/test_responses.py
1-1: unittest.mock.AsyncMock imported but unused
Remove unused import: unittest.mock.AsyncMock
(F401)
9-9: app.models.Assistant imported but unused
Remove unused import: app.models.Assistant
(F401)
10-10: app.core.exception_handlers.HTTPException imported but unused
Remove unused import: app.core.exception_handlers.HTTPException
(F401)
26-26: Unused function argument: mock_get_conversation_by_ancestor_id
(ARG001)
103-103: Unused function argument: mock_get_conversation_by_ancestor_id
(ARG001)
183-183: Unused function argument: db
(ARG001)
207-207: Unused function argument: db
(ARG001)
240-240: Unused function argument: db
(ARG001)
275-275: Unused function argument: mock_get_conversation_by_ancestor_id
(ARG001)
281-281: Unused function argument: db
(ARG001)
340-340: Unused function argument: db
(ARG001)
372-372: Unused function argument: db
(ARG001)
414-414: Unused function argument: mock_get_conversation_by_ancestor_id
(ARG001)
backend/app/tests/scripts/test_backend_pre_start.py
23-23: No newline at end of file
Add trailing newline
(W292)
🪛 GitHub Actions: AI Platform CI
backend/app/tests/scripts/test_backend_pre_start.py
[error] 1-1: Pre-commit hook 'end-of-file-fixer' modified this file to fix end of file issues.
🔇 Additional comments (13)
backend/app/api/routes/responses.py (7)
14-19: LGTM: Well-organized conversation tracking importsThe new imports properly support the conversation ancestry tracking feature. The import structure is clean and follows good practices.
112-120: LGTM: Proper function signature extensionThe function signature correctly extends to include the necessary parameters for conversation tracking and persistence.
139-149: LGTM: Sound ancestry resolution logicThe conversation ancestry resolution logic is well-implemented:
- Properly handles the case when response_id is provided
- Retrieves the latest conversation by ancestor ID
- Falls back to original response_id if no conversation found
- This ensures proper conversation chaining
199-232: LGTM: Robust conversation persistence with proper error handlingThe conversation creation logic is well-structured:
- Uses the CRUD function to set ancestor response ID
- Creates proper conversation data model
- Handles database errors gracefully without breaking the main flow
- Includes comprehensive logging for both success and failure cases
331-333: LGTM: Correct parameter passing to background taskThe endpoint correctly passes the new required parameters to the process_response function for background processing.
397-407: LGTM: Consistent ancestry resolution in sync endpointThe ancestry resolution logic in the sync endpoint matches the async endpoint implementation, ensuring consistent behavior across both endpoints.
449-482: LGTM: Appropriate sync conversation persistenceThe conversation persistence logic is well-adapted for the sync endpoint:
- Uses "sync_request" as assistant_id since no assistant is involved
- Maintains the same error handling and logging patterns
- Consistent with the async endpoint approach
backend/app/tests/api/routes/test_responses.py (6)
18-94: LGTM: Comprehensive test setup with realistic mocksThe test setup is excellent:
- All necessary new mocks added for conversation tracking
- Realistic response ID formats using proper OpenAI format
- Comprehensive mock setup covering all aspects of the new functionality
- Proper verification of the background task scheduling
180-266: LGTM: Comprehensive error scenario coverageThe new error tests provide excellent coverage:
- Assistant not found scenario with proper 404 response
- Missing credentials scenario
- Invalid credentials (missing api_key) scenario
- Each test properly verifies the expected error responses
268-404: LGTM: Thorough sync endpoint test coverageThe sync endpoint tests provide comprehensive coverage:
- Success scenario with proper response verification
- Missing credentials error handling
- OpenAI API error handling
- Consistent mock setup and assertion patterns
- Proper verification of conversation persistence with "sync_request" assistant_id
406-503: LGTM: Excellent file search functionality testThe file search test is well-structured:
- Realistic mock setup for file search results with proper score and text
- Comprehensive verification of OpenAI client calls with tools and includes
- Proper integration with conversation tracking functionality
- Good assertions for the complete flow
512-697: LGTM: Comprehensive ancestry resolution test coverageThe ancestry conversation tests provide excellent coverage:
- Tests both "found" and "not found" scenarios for ancestor conversations
- Proper verification of CRUD function calls with correct parameters
- Correct assertion of previous_response_id usage in OpenAI API calls
- Good edge case coverage when no response_id is provided
785-971: LGTM: Consistent sync ancestry test coverageThe sync endpoint ancestry tests mirror the async tests perfectly:
- Same comprehensive coverage of ancestry resolution scenarios
- Proper verification of sync-specific conversation persistence
- Consistent mock setup and assertion patterns
- Complete coverage of edge cases
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
backend/app/tests/api/routes/test_openai_conversation.py (1)
1-8: Remove unused imports identified by static analysis.The static analysis tool has identified that
pytest,uuid4, andHTTPExceptionimports are not used in this test file. These should be removed to keep the imports clean.Apply this diff to remove the unused imports:
-import pytest -from uuid import uuid4 from sqlmodel import Session -from fastapi import HTTPException from fastapi.testclient import TestClient from app.crud.openai_conversation import create_conversation from app.models import APIKeyPublicbackend/app/crud/openai_conversation.py (1)
64-64: Modernize type annotation syntax.Consider using the modern union syntax
str | Noneinstead ofOptional[str]for consistency with the rest of the codebase.Apply this diff to modernize the type annotation:
- previous_response_id: Optional[str], + previous_response_id: str | None,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/app/crud/openai_conversation.py(2 hunks)backend/app/tests/api/routes/test_openai_conversation.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
backend/app/crud/openai_conversation.py (3)
backend/app/tests/crud/test_openai_conversation.py (3)
test_get_conversation_by_ancestor_id_success(112-144)test_get_conversation_by_response_id_success(65-96)test_conversation_soft_delete_behavior(275-324)backend/app/api/routes/openai_conversation.py (1)
get_conversation_by_ancestor_id_route(77-95)backend/app/models/openai_conversation.py (1)
OpenAIConversationBase(22-55)
backend/app/tests/api/routes/test_openai_conversation.py (2)
backend/app/crud/openai_conversation.py (1)
create_conversation(140-163)backend/app/tests/crud/test_openai_conversation.py (1)
test_delete_conversation_success(227-259)
🪛 Ruff (0.12.2)
backend/app/crud/openai_conversation.py
64-64: Use X | None for type annotations
Convert to X | None
(UP045)
backend/app/tests/api/routes/test_openai_conversation.py
1-1: pytest imported but unused
Remove unused import: pytest
(F401)
2-2: uuid.uuid4 imported but unused
Remove unused import: uuid.uuid4
(F401)
4-4: fastapi.HTTPException imported but unused
Remove unused import: fastapi.HTTPException
(F401)
🔇 Additional comments (2)
backend/app/tests/api/routes/test_openai_conversation.py (1)
437-437: LGTM! Good formatting cleanup.Removing the trailing blank line improves code consistency.
backend/app/crud/openai_conversation.py (1)
61-100: LGTM! Well-implemented conversation ancestry logic.The
set_ancestor_response_idfunction correctly implements conversation threading logic with clear documentation that matches the implementation. The three-case logic handles:
- New conversations (no previous response)
- Continuing conversations (inheriting ancestor from previous)
- Missing previous conversations (fallback to previous response ID)
This provides robust ancestry tracking for conversation threads.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
backend/app/api/routes/responses.py (1)
41-42: Fix the bare except clause as previously suggested.The bare
exceptclause should be replaced with specific exception types to avoid catching system-level exceptions.Apply the previously suggested fix:
- except: + except (AttributeError, TypeError, ValueError, JSONDecodeError): passYou'll also need to import
JSONDecodeError:+from json import JSONDecodeError
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/api/routes/responses.py(8 hunks)
🧰 Additional context used
🪛 Ruff (0.12.2)
backend/app/api/routes/responses.py
41-41: Do not use bare except
(E722)
🔇 Additional comments (2)
backend/app/api/routes/responses.py (2)
317-318: LGTM!The addition of
organization_idand_sessionparameters to theprocess_responsecall is correct and necessary for the conversation persistence functionality.
454-467: Good error handling for conversation creation.The try-catch block with detailed logging for conversation creation failures is a good practice, especially for the sync endpoint where immediate feedback is valuable.
backend/app/api/routes/responses.py
Outdated
| # Set ancestor_response_id using CRUD function for sync endpoint | ||
| ancestor_response_id = get_ancestor_id_from_response( | ||
| session=_session, | ||
| current_response_id=response.id, | ||
| previous_response_id=response.previous_response_id, | ||
| project_id=project_id, | ||
| ) | ||
|
|
||
| # Create conversation record in database | ||
| conversation_data = OpenAIConversationCreate( | ||
| response_id=response.id, | ||
| previous_response_id=request.response_id, | ||
| ancestor_response_id=ancestor_response_id, | ||
| user_question=request.question, | ||
| response=response.output_text, | ||
| model=response.model, | ||
| assistant_id="sync_request", # For sync requests, we don't have assistant_id | ||
| ) |
There was a problem hiding this comment.
Fix previous_response_id inconsistency.
You correctly compute the previous_response_id on lines 384-392, but then use request.response_id instead of the computed value when creating the conversation data.
Apply this fix:
conversation_data = OpenAIConversationCreate(
response_id=response.id,
- previous_response_id=request.response_id,
+ previous_response_id=previous_response_id,
ancestor_response_id=ancestor_response_id,
user_question=request.question,
response=response.output_text,
model=response.model,
assistant_id="sync_request",
)This ensures consistency with the logic that retrieves the latest conversation to determine the correct previous response.
📝 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.
| # Set ancestor_response_id using CRUD function for sync endpoint | |
| ancestor_response_id = get_ancestor_id_from_response( | |
| session=_session, | |
| current_response_id=response.id, | |
| previous_response_id=response.previous_response_id, | |
| project_id=project_id, | |
| ) | |
| # Create conversation record in database | |
| conversation_data = OpenAIConversationCreate( | |
| response_id=response.id, | |
| previous_response_id=request.response_id, | |
| ancestor_response_id=ancestor_response_id, | |
| user_question=request.question, | |
| response=response.output_text, | |
| model=response.model, | |
| assistant_id="sync_request", # For sync requests, we don't have assistant_id | |
| ) | |
| # Set ancestor_response_id using CRUD function for sync endpoint | |
| ancestor_response_id = get_ancestor_id_from_response( | |
| session=_session, | |
| current_response_id=response.id, | |
| previous_response_id=response.previous_response_id, | |
| project_id=project_id, | |
| ) | |
| # Create conversation record in database | |
| conversation_data = OpenAIConversationCreate( | |
| response_id=response.id, | |
| previous_response_id=previous_response_id, | |
| ancestor_response_id=ancestor_response_id, | |
| user_question=request.question, | |
| response=response.output_text, | |
| model=response.model, | |
| assistant_id="sync_request", # For sync requests, we don't have assistant_id | |
| ) |
🤖 Prompt for AI Agents
In backend/app/api/routes/responses.py around lines 435 to 452, the code uses
request.response_id as previous_response_id when creating the conversation_data,
but it should use the previously computed previous_response_id variable to
maintain consistency. Replace request.response_id with the computed
previous_response_id in the OpenAIConversationCreate call to fix the
inconsistency.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/app/tests/api/routes/test_responses.py (1)
1-10: Remove unused imports to clean up the code.Static analysis correctly identifies several unused imports that should be removed for better code hygiene.
-from unittest.mock import MagicMock, patch, AsyncMock +from unittest.mock import MagicMock, patch import pytest from fastapi import FastAPI from fastapi.testclient import TestClient from sqlmodel import select import openai from app.api.routes.responses import router -from app.models import Project, Assistant -from app.core.exception_handlers import HTTPException +from app.models import Project
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/tests/api/routes/test_responses.py(5 hunks)
🧰 Additional context used
🪛 Ruff (0.12.2)
backend/app/tests/api/routes/test_responses.py
1-1: unittest.mock.AsyncMock imported but unused
Remove unused import: unittest.mock.AsyncMock
(F401)
9-9: app.models.Assistant imported but unused
Remove unused import: app.models.Assistant
(F401)
10-10: app.core.exception_handlers.HTTPException imported but unused
Remove unused import: app.core.exception_handlers.HTTPException
(F401)
26-26: Unused function argument: mock_get_conversation_by_ancestor_id
(ARG001)
103-103: Unused function argument: mock_get_conversation_by_ancestor_id
(ARG001)
183-183: Unused function argument: db
(ARG001)
207-207: Unused function argument: db
(ARG001)
240-240: Unused function argument: db
(ARG001)
275-275: Unused function argument: mock_get_conversation_by_ancestor_id
(ARG001)
281-281: Unused function argument: db
(ARG001)
340-340: Unused function argument: db
(ARG001)
372-372: Unused function argument: db
(ARG001)
414-414: Unused function argument: mock_get_conversation_by_ancestor_id
(ARG001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.11.7, 6)
🔇 Additional comments (10)
backend/app/tests/api/routes/test_responses.py (10)
25-93: LGTM! Comprehensive test coverage for successful response creation.The test properly validates the successful flow of the
/responsesendpoint with appropriate mocking of all dependencies. The mock setup for conversation ancestry tracking is present even though it's not exercised in this specific scenario (noresponse_idprovided), which maintains consistency with other test functions.
102-178: Excellent test for vector store conditional logic.This test effectively validates that when an assistant has no vector store configured, the OpenAI client is called without tools. The explicit verification of the OpenAI client call parameters is particularly valuable for ensuring the conditional logic works correctly.
180-200: LGTM! Clean error handling test.This test effectively validates the 404 error case when an assistant is not found. The mock setup and assertions are appropriate for testing this error scenario.
202-233: Good coverage of credential validation logic.This test properly validates the graceful handling of missing OpenAI credentials. The approach of returning
success=Falserather than throwing an HTTP exception provides a better user experience.
235-266: Excellent edge case coverage for incomplete credentials.This test validates an important edge case where credentials exist but are missing the required
api_keyfield. This type of partial configuration scenario is realistic and the test ensures proper error handling.
274-335: Solid test coverage for synchronous endpoint.This test properly validates the
/responses/syncendpoint with appropriate mock setup and response verification. The test structure is consistent with the async endpoint tests while properly accounting for the synchronous nature of this endpoint.
337-404: Comprehensive error handling tests for sync endpoint.These tests provide excellent coverage of error scenarios for the
/responses/syncendpoint, including missing credentials and OpenAI API errors. The consistent error handling approach between async and sync endpoints is well-validated.
406-503: Excellent coverage of file search functionality.This test provides comprehensive validation of the file search feature integration, including proper mock setup for search results and verification of the tools configuration. The attention to detail in testing the
includeparameter and tools structure is particularly valuable.
505-784: Comprehensive coverage of conversation ancestry tracking.These tests excellently validate the conversation ancestry tracking feature across all scenarios:
- Found conversation: Properly uses the conversation's
response_idasprevious_response_id- Not found conversation: Falls back to using the original
response_id- No response_id: Correctly passes
Noneasprevious_response_idThe verification of
get_conversation_by_ancestor_idcall parameters and the correctprevious_response_idbeing passed to OpenAI is thorough and ensures the feature works as intended.
786-1055: Complete conversation ancestry coverage for sync endpoint.These tests mirror the async endpoint conversation ancestry tests and provide complete coverage of the feature for the
/responses/syncendpoint. The consistency in testing approach between async and sync endpoints ensures both implementations work correctly and maintain feature parity.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/app/tests/api/routes/test_responses.py (1)
1-1: Remove unused imports to clean up the code.Static analysis correctly identifies several unused imports that should be removed for cleaner code.
-from unittest.mock import MagicMock, patch, AsyncMock +from unittest.mock import MagicMock, patch-from app.models import Project, Assistant -from app.core.exception_handlers import HTTPException +from app.models import ProjectAlso applies to: 9-10
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/tests/api/routes/test_responses.py(5 hunks)
🧰 Additional context used
🪛 Ruff (0.12.2)
backend/app/tests/api/routes/test_responses.py
1-1: unittest.mock.AsyncMock imported but unused
Remove unused import: unittest.mock.AsyncMock
(F401)
9-9: app.models.Assistant imported but unused
Remove unused import: app.models.Assistant
(F401)
10-10: app.core.exception_handlers.HTTPException imported but unused
Remove unused import: app.core.exception_handlers.HTTPException
(F401)
26-26: Unused function argument: mock_get_conversation_by_ancestor_id
(ARG001)
103-103: Unused function argument: mock_get_conversation_by_ancestor_id
(ARG001)
183-183: Unused function argument: db
(ARG001)
207-207: Unused function argument: db
(ARG001)
240-240: Unused function argument: db
(ARG001)
275-275: Unused function argument: mock_get_conversation_by_ancestor_id
(ARG001)
281-281: Unused function argument: db
(ARG001)
340-340: Unused function argument: db
(ARG001)
372-372: Unused function argument: db
(ARG001)
414-414: Unused function argument: mock_get_conversation_by_ancestor_id
(ARG001)
🔇 Additional comments (14)
backend/app/tests/api/routes/test_responses.py (14)
18-93: LGTM! Comprehensive test setup for success scenario.The test properly covers the successful response creation path with appropriate mocks for the new conversation ancestry features. The unused
mock_get_conversation_by_ancestor_idparameter is expected since this test focuses on the basic success case without ancestor conversation lookup.
95-178: LGTM! Proper test coverage for vector store-less scenario.The test correctly verifies that when an assistant has no vector store configured, the OpenAI client is called without the
toolsparameter. The assertion on lines 171-177 effectively validates this behavior.
180-200: LGTM! Good error handling test coverage.This test properly validates the 404 error response when an assistant is not found. The test is focused and effectively covers this edge case.
202-233: LGTM! Proper credentials validation test.The test correctly validates the behavior when OpenAI credentials are not configured, ensuring the API returns a structured error response rather than an HTTP error. This is good API design for credential validation.
235-266: LGTM! Good edge case coverage for incomplete credentials.This test properly distinguishes between missing credentials and incomplete credentials, both resulting in appropriate error responses. The test coverage for credential validation scenarios is comprehensive.
268-335: LGTM! Good test coverage for sync endpoint.The test properly validates the synchronous responses endpoint behavior, ensuring it returns the response data immediately rather than just acknowledging processing start. The test structure is consistent with async endpoint tests.
337-363: LGTM! Consistent error handling between sync and async endpoints.The test ensures that credential validation works the same way for both sync and async endpoints, maintaining API consistency.
365-404: LGTM! Proper external API error handling test.The test correctly validates error handling when the OpenAI API fails, ensuring errors are caught and returned as structured responses rather than causing unhandled exceptions.
406-503: LGTM! Comprehensive file search functionality test.The test thoroughly validates file search integration, properly mocking search results and verifying that the OpenAI client receives the correct tools and include parameters. The assertions effectively ensure proper file search configuration.
505-601: LGTM! Core conversation ancestry feature test.This test properly validates the conversation ancestry tracking functionality when an ancestor conversation is found. The assertions correctly verify that the conversation lookup is called with proper parameters and that the found conversation's response_id is used as previous_response_id in the OpenAI call.
603-698: LGTM! Proper fallback behavior test for conversation ancestry.The test correctly validates the fallback scenario when an ancestor conversation is not found, ensuring the system gracefully falls back to using the original response_id as previous_response_id.
700-784: LGTM! Baseline scenario test without conversation ancestry.The test properly validates the behavior when no response_id is provided, ensuring conversation lookup is not called and the system works correctly without ancestry tracking.
786-855: LGTM! Sync endpoint ancestry handling test.The test correctly validates that the sync endpoint uses a simpler approach to conversation ancestry - directly using the provided response_id as previous_response_id without performing database lookups. This design choice makes sense for performance reasons in synchronous operations.
857-994: LGTM! Complete sync endpoint test coverage.These final tests complete the test coverage for the sync endpoint scenarios, maintaining consistency with the simpler design approach for synchronous operations. The test naming could be slightly misleading (e.g., "ancestor_conversation_not_found" when sync doesn't do lookups), but the test logic is correct.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/app/tests/api/routes/test_responses.py (1)
93-176: Well-structured test with minor cleanup opportunity.The test properly covers the scenario without vector store configuration. The mock setup is appropriate and assertions are comprehensive.
Note: The
mock_get_conversation_by_ancestor_idparameter appears unused but may be intentionally included for decorator consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/tests/api/routes/test_responses.py(5 hunks)
🧰 Additional context used
🪛 Ruff (0.12.2)
backend/app/tests/api/routes/test_responses.py
101-101: Unused function argument: mock_get_conversation_by_ancestor_id
(ARG001)
181-181: Unused function argument: db
(ARG001)
205-205: Unused function argument: db
(ARG001)
238-238: Unused function argument: db
(ARG001)
273-273: Unused function argument: mock_get_conversation_by_ancestor_id
(ARG001)
279-279: Unused function argument: db
(ARG001)
338-338: Unused function argument: db
(ARG001)
370-370: Unused function argument: db
(ARG001)
412-412: Unused function argument: mock_get_conversation_by_ancestor_id
(ARG001)
🔇 Additional comments (12)
backend/app/tests/api/routes/test_responses.py (12)
6-6: LGTM! OpenAI import supports error handling tests.The addition of the
openaiimport is appropriate for the new tests that handleopenai.OpenAIErrorexceptions.
17-91: Excellent test enhancement with comprehensive mocking.The test has been properly expanded to cover the new conversation ancestry tracking features. The mock setup is thorough and includes:
- Realistic response ID formats
- Proper assistant configuration mocking
- CRUD function mocking for conversation handling
- Langfuse tracer integration
The test structure maintains good readability while covering all the new functionality.
178-198: Excellent error case coverage.This test properly validates the 404 error scenario when an assistant is not found. The assertions correctly check both the status code and the specific error message.
200-231: Good error handling test for missing credentials.This test appropriately covers the scenario when OpenAI credentials are not configured. The validation of
success=falseand the error message provides good coverage for this failure case.
233-264: Valuable edge case test for incomplete credentials.This test covers an important edge case where credentials exist but are missing the required
api_keyfield. The test setup and assertions are appropriate for this scenario.
266-333: Comprehensive sync endpoint test coverage.This test properly validates the synchronous endpoint functionality with appropriate mock setup and assertions. The test structure is consistent with the async endpoint tests while covering sync-specific behavior.
335-361: Good error handling for sync endpoint.This test appropriately covers the missing credentials scenario for the synchronous endpoint, maintaining consistency with the async endpoint error handling tests.
363-402: Excellent error simulation test.This test effectively covers OpenAI API error scenarios using
side_effectto simulate the exception. The error handling validation is comprehensive and appropriate.
404-501: Sophisticated test for file search functionality.This test provides excellent coverage of the file search feature with:
- Detailed mock setup for search results with scores and text
- Proper validation of OpenAI client tool configuration
- Verification of include parameters for file search results
The test is comprehensive and well-structured.
503-599: Excellent test for conversation ancestry tracking.This test thoroughly validates the conversation ancestry feature when an ancestor conversation is found. Key validations include:
- Proper calling of
get_conversation_by_ancestor_idwith correct parameters- Using the conversation's
response_idasprevious_response_idfor continuity- Comprehensive mock setup for the conversation object
601-696: Good coverage of ancestor conversation not found scenario.This test properly validates the fallback behavior when no ancestor conversation is found. The test ensures that:
get_conversation_by_ancestor_idis called with correct parameters- The original
response_idis used asprevious_response_idwhen no conversation is found- The system handles the not-found case gracefully
698-991: Comprehensive test coverage for remaining scenarios.These tests provide excellent coverage for:
- No response_id scenario: Properly validates that conversation lookup is skipped and
previous_response_idis None- Sync endpoint variations: Appropriately test sync endpoint behavior without conversation lookups (as noted in comments)
The test patterns are consistent and the behavioral differences between sync and async endpoints are properly validated.
Note: The sync endpoint tests correctly don't perform conversation lookups, which is an appropriate architectural difference from the async endpoint.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/tests/api/routes/test_responses.py(5 hunks)
🧰 Additional context used
🪛 Ruff (0.12.2)
backend/app/tests/api/routes/test_responses.py
25-25: Unused function argument: mock_get_conversation_by_ancestor_id
(ARG001)
38-38: Unused function argument: args
(ARG001)
61-61: Unused function argument: args
(ARG001)
61-61: Unused function argument: kwargs
(ARG001)
120-120: Unused function argument: mock_get_conversation_by_ancestor_id
(ARG001)
200-200: Unused function argument: db
(ARG001)
224-224: Unused function argument: db
(ARG001)
257-257: Unused function argument: db
(ARG001)
292-292: Unused function argument: mock_get_conversation_by_ancestor_id
(ARG001)
298-298: Unused function argument: db
(ARG001)
357-357: Unused function argument: db
(ARG001)
389-389: Unused function argument: db
(ARG001)
431-431: Unused function argument: mock_get_conversation_by_ancestor_id
(ARG001)
🔇 Additional comments (15)
backend/app/tests/api/routes/test_responses.py (15)
6-6: LGTM! Import added for error handling tests.The
openaiimport is appropriately added to support mocking OpenAI-specific errors in test cases.
17-110: Well-structured test with comprehensive mocking.The test properly validates the successful response creation flow with realistic mock data and appropriate assertions. The credential side effect function correctly handles different provider types.
112-195: Good edge case coverage for assistants without vector stores.The test correctly validates that the OpenAI client is called without tools when the assistant has no vector store configured.
197-217: Proper error handling test for missing assistant.The test correctly validates the 404 response when an assistant is not found.
219-250: Good coverage for missing credentials scenario.The test appropriately validates the error response when OpenAI credentials are not configured.
252-283: Excellent edge case coverage for malformed credentials.The test properly validates that missing api_key in credentials is handled the same as missing credentials entirely.
285-352: Well-structured test for synchronous endpoint.The test comprehensively validates the successful synchronous response creation with appropriate assertions.
354-380: Consistent error handling for sync endpoint.The test appropriately validates credential error handling for the synchronous endpoint.
382-421: Proper OpenAI error handling test.The test correctly simulates and validates handling of OpenAI API errors using the appropriate exception type.
423-520: Comprehensive test for file search functionality.The test excellently validates file search integration with realistic mock data and proper assertions for tool configuration.
522-618: Excellent test for conversation ancestry tracking.The test properly validates that when an ancestor conversation is found, the latest response_id from that conversation is used as previous_response_id, maintaining conversation continuity.
620-715: Good fallback behavior test.The test correctly validates that when no ancestor conversation is found, the original response_id is used as previous_response_id.
717-801: Proper handling of new conversations.The test correctly validates that when no response_id is provided, no ancestor lookup occurs and previous_response_id is None.
803-872: Clear test showing sync endpoint behavior.The test properly validates that the sync endpoint bypasses conversation lookup and uses the provided response_id directly as previous_response_id. The explanatory comment is helpful.
945-1011: Good test for sync endpoint without response_id.The test correctly validates that when no response_id is provided to the sync endpoint, previous_response_id is None.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/app/tests/api/routes/test_responses.py (1)
877-943: Remove duplicate test with misleading name.This test is a duplicate of
test_responses_sync_endpoint_with_ancestor_conversation_foundand has a misleading name. The sync endpoint doesn't perform conversation lookups, so having separate "found" and "not_found" tests is confusing and redundant.Remove this entire test function as it duplicates the functionality tested in the previous sync endpoint test.
🧹 Nitpick comments (11)
backend/app/tests/api/routes/test_responses.py (11)
24-34: Remove unused mock parameter.The
mock_get_conversation_by_ancestor_idparameter is not used in this test function, as indicated by static analysis.def test_responses_endpoint_success( - mock_get_conversation_by_ancestor_id, mock_create_conversation, mock_get_ancestor_id_from_response, mock_tracer_class, mock_get_assistant, mock_get_credential, mock_openai, db, user_api_key_header: dict[str, str], ):
38-48: Remove unused parameter from mock function.The
argsparameter is not used in the mock credentials function.- def mock_get_credentials_by_provider(*args, **kwargs): + def mock_get_credentials_by_provider(**kwargs):
61-64: Remove unused parameters from mock function.The
argsandkwargsparameters are not used in this mock function.- def return_mock_assistant(*args, **kwargs): + def return_mock_assistant():
119-129: Remove unused mock parameter.The
mock_get_conversation_by_ancestor_idparameter is not used in this test function.def test_responses_endpoint_without_vector_store( - mock_get_conversation_by_ancestor_id, mock_create_conversation, mock_get_ancestor_id_from_response, mock_tracer_class, mock_get_assistant, mock_get_credential, mock_openai, db, user_api_key_header, ):
198-202: Remove unused db parameter.The
dbparameter is not used in this test function.def test_responses_endpoint_assistant_not_found( mock_get_assistant, - db, user_api_key_header, ):
221-226: Remove unused db parameter.The
dbparameter is not used in this test function.def test_responses_endpoint_no_openai_credentials( mock_get_assistant, mock_get_credential, - db, user_api_key_header, ):
254-259: Remove unused db parameter.The
dbparameter is not used in this test function.def test_responses_endpoint_missing_api_key_in_credentials( mock_get_assistant, mock_get_credential, - db, user_api_key_header, ):
291-300: Remove unused mock parameter.The
mock_get_conversation_by_ancestor_idparameter is not used in this sync endpoint test.def test_responses_sync_endpoint_success( - mock_get_conversation_by_ancestor_id, mock_create_conversation, mock_get_ancestor_id_from_response, mock_tracer_class, mock_get_credential, mock_openai, - db, user_api_key_header, ):
355-359: Remove unused db parameter.The
dbparameter is not used in this test function.def test_responses_sync_endpoint_no_openai_credentials( mock_get_credential, - db, user_api_key_header, ):
385-391: Remove unused db parameter.The
dbparameter is not used in this test function.def test_responses_sync_endpoint_openai_error( mock_tracer_class, mock_get_credential, mock_openai, - db, user_api_key_header, ):
430-440: Remove unused mock parameter.The
mock_get_conversation_by_ancestor_idparameter is not used in this test function.def test_responses_endpoint_with_file_search_results( - mock_get_conversation_by_ancestor_id, mock_create_conversation, mock_get_ancestor_id_from_response, mock_tracer_class, mock_get_assistant, mock_get_credential, mock_openai, db, user_api_key_header, ):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/tests/api/routes/test_responses.py(5 hunks)
🧰 Additional context used
🪛 Ruff (0.12.2)
backend/app/tests/api/routes/test_responses.py
25-25: Unused function argument: mock_get_conversation_by_ancestor_id
(ARG001)
38-38: Unused function argument: args
(ARG001)
61-61: Unused function argument: args
(ARG001)
61-61: Unused function argument: kwargs
(ARG001)
120-120: Unused function argument: mock_get_conversation_by_ancestor_id
(ARG001)
200-200: Unused function argument: db
(ARG001)
224-224: Unused function argument: db
(ARG001)
257-257: Unused function argument: db
(ARG001)
292-292: Unused function argument: mock_get_conversation_by_ancestor_id
(ARG001)
298-298: Unused function argument: db
(ARG001)
357-357: Unused function argument: db
(ARG001)
389-389: Unused function argument: db
(ARG001)
431-431: Unused function argument: mock_get_conversation_by_ancestor_id
(ARG001)
🔇 Additional comments (3)
backend/app/tests/api/routes/test_responses.py (3)
6-6: LGTM!The OpenAI import addition is appropriate for the enhanced test coverage that includes OpenAI error handling scenarios.
529-801: Excellent test coverage for conversation ancestry tracking.These tests comprehensively cover the conversation ancestry scenarios:
- When ancestor conversation is found
- When ancestor conversation is not found
- When no response_id is provided
The test logic properly verifies the correct calls to
get_conversation_by_ancestor_idand ensures the rightprevious_response_idvalues are passed to the OpenAI client.
948-1011: Good test coverage for sync endpoint without response_id.This test properly verifies that when no response_id is provided to the sync endpoint, the OpenAI client is called with
Noneas theprevious_response_id.
AkhileshNegi
force-pushed
the
enhancement/openai-conversation-lookup
branch
from
6769ece to
aeb224c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/app/tests/api/routes/test_responses.py (1)
6-6: Remove unused importThe
openaimodule is imported but never used in the test file.-import openai
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/tests/api/routes/test_responses.py(5 hunks)
🧰 Additional context used
🪛 Ruff (0.12.2)
backend/app/tests/api/routes/test_responses.py
6-6: openai imported but unused
Remove unused import: openai
(F401)
25-25: Unused function argument: mock_get_conversation_by_ancestor_id
(ARG001)
38-38: Unused function argument: args
(ARG001)
61-61: Unused function argument: args
(ARG001)
61-61: Unused function argument: kwargs
(ARG001)
120-120: Unused function argument: mock_get_conversation_by_ancestor_id
(ARG001)
200-200: Unused function argument: db
(ARG001)
224-224: Unused function argument: db
(ARG001)
257-257: Unused function argument: db
(ARG001)
293-293: Unused function argument: mock_get_conversation_by_ancestor_id
(ARG001)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/app/api/routes/responses.py (1)
41-41: Fix the bare except clause.The bare
exceptclause should be more specific to avoid catching system exceptions.Apply this fix:
- except: + except (AttributeError, TypeError, ValueError, KeyError): pass
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/api/routes/responses.py(6 hunks)
🧰 Additional context used
🪛 Ruff (0.12.2)
backend/app/api/routes/responses.py
41-41: Do not use bare except
(E722)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.11.7, 6)
🔇 Additional comments (4)
backend/app/api/routes/responses.py (4)
14-19: LGTM!The new imports are well-organized and necessary for the conversation persistence functionality.
211-227: LGTM!The conversation persistence logic is well-implemented. It properly creates the conversation record with all necessary metadata and persists it to the database using the appropriate CRUD function.
327-328: LGTM!The endpoint correctly passes the required
organization_idand_sessionparameters to support conversation persistence functionality.
347-455: Consider conversation persistence consistency.The sync endpoint doesn't persist conversation data like the async endpoint does. If this endpoint is part of the same conversation system, it should also integrate conversation tracking for consistency.
Is this intentional because it's for benchmarking purposes, or should the sync endpoint also persist conversations? Please clarify the expected behavior.
| if latest_conversation: | ||
| ancestor_id = latest_conversation.response_id |
There was a problem hiding this comment.
Potential logic error in ancestor_id reassignment.
Line 149 reassigns ancestor_id = latest_conversation.response_id, but this contradicts the variable name and purpose. The ancestor_id should remain as the original request.response_id to maintain the root of the conversation chain.
Apply this fix:
- if latest_conversation:
- ancestor_id = latest_conversation.response_id
+ # ancestor_id should remain as request.response_id (the root)
+ # latest_conversation.response_id will be used as previous_response_id insteadThen update line 153 to use the correct previous response ID:
- "previous_response_id": ancestor_id,
+ "previous_response_id": latest_conversation.response_id if latest_conversation else ancestor_id,📝 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.
| if latest_conversation: | |
| ancestor_id = latest_conversation.response_id | |
| # keep ancestor_id as the root (request.response_id) only | |
| ancestor_id = request.response_id | |
| latest_conversation = ( | |
| session.query(OpenAIConversation) | |
| .filter_by(ancestor_response_id=ancestor_id) | |
| .order_by(OpenAIConversation.created_at.desc()) | |
| .first() | |
| ) | |
| # ancestor_id should remain as request.response_id (the root) | |
| # latest_conversation.response_id will be used as previous_response_id instead | |
| conversation_data = OpenAIConversationCreate( | |
| response_id=response.id, | |
| - previous_response_id=ancestor_id, | |
| + previous_response_id=latest_conversation.response_id | |
| + if latest_conversation | |
| + else ancestor_id, | |
| ancestor_response_id=request.response_id, | |
| user_question=request.question, | |
| response=response.output_text, | |
| model=response.model, | |
| assistant_id=request.assistant_id, | |
| ) |
🤖 Prompt for AI Agents
In backend/app/api/routes/responses.py around lines 148 to 149, the assignment
of ancestor_id to latest_conversation.response_id is incorrect because
ancestor_id should keep the original request.response_id to represent the root
of the conversation chain. Fix this by not reassigning ancestor_id here and
ensure that subsequent logic, such as on line 153, uses the correct previous
response ID instead of ancestor_id.
* added migration * first stab at CRUD * added ancestor id fetching * removing create endpoint * initiating populating data * added lookup logic * updated business logic and testcases * revert few updates * updated testcases * fixing testcases * added logic for ancestor lookup * added logic for ancestor lookup * removed xml file * cleanups * coderabbit cleanups * consistency updating db entry * renaming function * cleanups based on review * cleanup testcases * reducing db calls * remove unnecessary code * cleanups * fixing CI * reverting testcases for response/sync * coderabbit cleanups * cleanup testcases * renaming * removed /sync testcases * cleanups
2 participants
Summary
Target issue is #304
Explain the motivation for making this change. What existing problem does the pull request solve?
Checklist
Before submitting a pull request, please ensure that you mark these task.
fastapi run --reload app/main.pyordocker compose upin the repository root and test.Notes
Please add here if any other information is required for the reviewer.
Summary by CodeRabbit
New Features
Bug Fixes
Tests