Feat: Add Prompt Management System with Versioning #319
Conversation
WalkthroughAdds Prompt and PromptVersion models, CRUD services, API routes, and an Alembic migration. Wires routers into the API. Implements project-scoped CRUD (create, list, get with versions, update, delete; version create/delete). Introduces comprehensive unit and API tests, plus test utilities; comments out existing response tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API Router
participant Prompts Route
participant CRUD
participant DB
Client->>API Router: POST /api/v1/prompts
API Router->>Prompts Route: dispatch
Prompts Route->>CRUD: create_prompt(project_id, PromptCreate)
CRUD->>DB: insert Prompt, insert PromptVersion (v1), set active_version
DB-->>CRUD: rows committed
CRUD-->>Prompts Route: (Prompt, PromptVersion)
Prompts Route-->>Client: 201 APIResponse(PromptWithVersion)
sequenceDiagram
participant Client
participant API Router
participant PromptVersions Route
participant CRUD
participant DB
Client->>API Router: POST /api/v1/prompts/{id}/versions
API Router->>PromptVersions Route: dispatch
PromptVersions Route->>CRUD: create_prompt_version(prompt_id, data, project_id)
CRUD->>DB: read latest, insert new version (v+1)
DB-->>CRUD: row committed
CRUD-->>PromptVersions Route: PromptVersion
PromptVersions Route-->>Client: 201 APIResponse(PromptVersionPublic)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
avirajsingh7
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (12)
backend/app/tests/api/routes/test_prompt_versions.py (1)
1-16: Remove unused imports to clean up the code.Several imports are not used in this test file and should be removed for clarity.
-import pytest from fastapi.testclient import TestClient from sqlmodel import Session from app.models import ( - Prompt, PromptCreate, PromptVersionCreate, - PromptVersion, APIKeyPublic, PromptVersionLabel, PromptVersionUpdate, ) from app.crud import create_prompt, create_prompt_version -from app.tests.utils.utils import get_projectbackend/app/crud/prompt.py (3)
13-23: Use Pythonic boolean comparisons.Replace equality comparison with
Falsefor better readability and Pythonic style.def get_prompt_by_name_in_project( session: Session, name: str, project_id: int ) -> Prompt | None: return session.exec( select(Prompt).where( Prompt.name == name, Prompt.project_id == project_id, - Prompt.is_deleted == False, + Prompt.is_deleted.is_(False), ) ).first()Note: Using
.is_(False)is the SQLAlchemy/SQLModel recommended way for boolean comparisons in queries.
25-35: Use Pythonic boolean comparisons.Apply the same boolean comparison fix here.
def get_prompt_by_id( session: Session, prompt_id: int, project_id: int ) -> Prompt | None: return session.exec( select(Prompt).where( Prompt.id == prompt_id, Prompt.project_id == project_id, - Prompt.is_deleted == False, + Prompt.is_deleted.is_(False), ) ).first()
37-56: Use Pythonic boolean comparisons in query functions.Fix boolean comparisons in both
get_prompt_by_projectandcount_prompts_by_project.def get_prompt_by_project( session: Session, project_id: int, skip: int = 0, limit: int = 100 ) -> list[Prompt]: return session.exec( select(Prompt) - .where(Prompt.project_id == project_id, Prompt.is_deleted == False) + .where(Prompt.project_id == project_id, Prompt.is_deleted.is_(False)) .offset(skip) .limit(limit) ).all() def count_prompts_by_project(session: Session, project_id: int) -> int: return session.exec( select(func.count()).select_from( select(Prompt) - .where(Prompt.project_id == project_id, Prompt.is_deleted == False) + .where(Prompt.project_id == project_id, Prompt.is_deleted.is_(False)) .subquery() ) ).one()backend/app/api/routes/prompts.py (2)
46-52: Consider standard body parameter instead of Depends().Using
Depends()for request body is unconventional. Consider using standard FastAPI body parameter.@router.patch("/{prompt_id}", response_model=APIResponse[PromptPublic]) def update_prompt_route( + prompt_update: PromptUpdate, prompt_id: int = Path(..., gt=0), - prompt_update: PromptUpdate = Depends(), session: Session = Depends(get_db), current_user: UserProjectOrg = Depends(get_current_user_org_project), ):
80-85: Consider removing redundant error logging.The error is likely already logged in the CRUD layer. Consider removing this duplicate logging to avoid cluttering logs.
if not prompt: - logger.error( - f"[get_prompt] Prompt not found | prompt_id={prompt_id}, project_id={current_user.project_id}" - ) raise HTTPException(status_code=404, detail="Prompt not found.")backend/app/tests/api/routes/test_prompts.py (1)
78-82: Consider removing unuseddbparameter from test functions.Several test functions receive the
dbfixture but don't use it directly. If the fixture isn't needed for setup/teardown, consider removing it.For example, in
test_update_prompt_duplicate_name:def test_update_prompt_duplicate_name( client: TestClient, - db: Session, user_api_key: APIKeyPublic, ):Apply similar changes to other test functions that don't use the
dbparameter.Also applies to: 111-115, 133-137, 161-165, 178-182
backend/app/crud/prompt_version.py (4)
77-92: Fix boolean comparisons in query.Use SQLAlchemy's
.is_()method for boolean comparisons.stmt = ( select(Prompt, PromptVersion) .outerjoin( PromptVersion, and_( Prompt.id == PromptVersion.prompt_id, PromptVersion.version == version, - PromptVersion.is_deleted == False, + PromptVersion.is_deleted.is_(False), ), ) .where( Prompt.id == prompt_id, Prompt.project_id == project_id, - Prompt.is_deleted == False, + Prompt.is_deleted.is_(False), ) )
123-131: Fix boolean comparison in query.stmt = ( select(PromptVersion) - .where(PromptVersion.prompt_id == prompt_id, PromptVersion.is_deleted == False) + .where(PromptVersion.prompt_id == prompt_id, PromptVersion.is_deleted.is_(False)) .order_by(PromptVersion.version.desc()) .offset(skip) .limit(limit) )
155-170: Fix boolean comparisons in production version query.stmt = ( select(Prompt, PromptVersion) .outerjoin( PromptVersion, and_( Prompt.id == PromptVersion.prompt_id, PromptVersion.label == PromptVersionLabel.PRODUCTION, - PromptVersion.is_deleted == False, + PromptVersion.is_deleted.is_(False), ), ) .where( Prompt.id == prompt_id, Prompt.project_id == project_id, - Prompt.is_deleted == False, + Prompt.is_deleted.is_(False), ) )
261-263: Fix version ID reference in log message.The log message references
prompt_version.idbut should referenceversionparameter for consistency.logger.info( - f"[delete_prompt_version] Deleted prompt version | Prompt ID: {prompt_id}, Version ID: {prompt_version.id}" + f"[delete_prompt_version] Deleted prompt version | Prompt ID: {prompt_id}, Version: {version}" )backend/app/tests/crud/test_prompt_version.py (1)
293-324: Fix unused variable in soft deletion test.The test logic is correct but contains an unused variable that should be removed per static analysis.
- v1 = create_prompt_version( + create_prompt_version( session=db, prompt_id=prompt.id, prompt_version_in=PromptVersionCreate(instruction="v1", commit_message="init"), project_id=prompt.project_id, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
backend/app/alembic/versions/913983f0a279_added_promptversion_table.py(1 hunks)backend/app/alembic/versions/c6a3abcd37bc_added_prompt_table.py(1 hunks)backend/app/api/main.py(2 hunks)backend/app/api/routes/prompt_versions.py(1 hunks)backend/app/api/routes/prompts.py(1 hunks)backend/app/crud/__init__.py(1 hunks)backend/app/crud/prompt.py(1 hunks)backend/app/crud/prompt_version.py(1 hunks)backend/app/models/__init__.py(1 hunks)backend/app/models/pagination.py(1 hunks)backend/app/models/prompt.py(1 hunks)backend/app/models/prompt_version.py(1 hunks)backend/app/tests/api/routes/test_prompt_versions.py(1 hunks)backend/app/tests/api/routes/test_prompts.py(1 hunks)backend/app/tests/crud/test_prompt.py(1 hunks)backend/app/tests/crud/test_prompt_version.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
backend/app/alembic/versions/913983f0a279_added_promptversion_table.py (2)
backend/app/alembic/versions/e9dd35eff62c_add_openai_conversation_table.py (1)
upgrade(20-70)backend/app/alembic/versions/99f4fc325617_add_organization_project_setup.py (1)
upgrade(20-72)
backend/app/tests/crud/test_prompt.py (4)
backend/app/crud/prompt.py (6)
get_prompt_by_id(25-34)get_prompt_by_name_in_project(13-22)get_prompt_by_project(37-45)create_prompt(58-82)update_prompt(85-128)delete_prompt(131-148)backend/app/models/prompt.py (3)
PromptCreate(29-30)PromptUpdate(33-38)Prompt(11-19)backend/app/tests/utils/utils.py (2)
get_project(70-89)get_non_existent_id(65-67)backend/app/tests/conftest.py (1)
db(18-35)
🪛 Ruff (0.12.2)
backend/app/crud/__init__.py
69-69: .prompt.count_prompts_by_project imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
70-70: .prompt.create_prompt imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
71-71: .prompt.delete_prompt imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
72-72: .prompt.get_prompt_by_id imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
73-73: .prompt.get_prompt_by_name_in_project imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
74-74: .prompt.get_prompt_by_project imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
75-75: .prompt.update_prompt imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
80-80: .prompt_version.create_prompt_version imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
81-81: .prompt_version.delete_prompt_version imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
82-82: .prompt_version.get_next_prompt_version imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
83-83: .prompt_version.get_prompt_version_by_id imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
84-84: .prompt_version.get_prompt_versions imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
85-85: .prompt_version.get_prompt_versions_count imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
86-86: .prompt_version.get_production_prompt_version imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
87-87: .prompt_version.update_prompt_version imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
backend/app/models/__init__.py
66-66: .pagination.Pagination imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
69-69: .prompt.Prompt imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
70-70: .prompt.PromptBase imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
71-71: .prompt.PromptCreate imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
72-72: .prompt.PromptPublic imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
73-73: .prompt.PromptUpdate imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
77-77: .prompt_version.PromptVersion imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
78-78: .prompt_version.PromptVersionBase imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
79-79: .prompt_version.PromptVersionCreate imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
80-80: .prompt_version.PromptVersionLabel imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
81-81: .prompt_version.PromptVersionPublic imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
82-82: .prompt_version.PromptVersionUpdate imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
backend/app/tests/api/routes/test_prompt_versions.py
1-1: pytest imported but unused
Remove unused import: pytest
(F401)
6-6: app.models.Prompt imported but unused
Remove unused import
(F401)
9-9: app.models.PromptVersion imported but unused
Remove unused import
(F401)
15-15: app.tests.utils.utils.get_project imported but unused
Remove unused import: app.tests.utils.utils.get_project
(F401)
backend/app/crud/prompt.py
20-20: Avoid equality comparisons to False; use not Prompt.is_deleted: for false checks
Replace with not Prompt.is_deleted
(E712)
32-32: Avoid equality comparisons to False; use not Prompt.is_deleted: for false checks
Replace with not Prompt.is_deleted
(E712)
42-42: Avoid equality comparisons to False; use not Prompt.is_deleted: for false checks
Replace with not Prompt.is_deleted
(E712)
52-52: Avoid equality comparisons to False; use not Prompt.is_deleted: for false checks
Replace with not Prompt.is_deleted
(E712)
backend/app/models/prompt_version.py
34-34: Undefined name Prompt
(F821)
backend/app/tests/api/routes/test_prompts.py
80-80: Unused function argument: db
(ARG001)
129-129: Module level import not at top of file
(E402)
129-129: Redefinition of unused PromptCreate from line 4
Remove definition: PromptCreate
(F811)
130-130: Module level import not at top of file
(E402)
130-130: Redefinition of unused get_prompt_by_name_in_project from line 5
Remove definition: get_prompt_by_name_in_project
(F811)
135-135: Unused function argument: db
(ARG001)
180-180: Unused function argument: db
(ARG001)
backend/app/tests/crud/test_prompt_version.py
295-295: Local variable v1 is assigned to but never used
Remove assignment to unused variable v1
(F841)
backend/app/models/prompt.py
19-19: Undefined name PromptVersion
(F821)
backend/app/crud/prompt_version.py
84-84: Avoid equality comparisons to False; use not PromptVersion.is_deleted: for false checks
Replace with not PromptVersion.is_deleted
(E712)
90-90: Avoid equality comparisons to False; use not Prompt.is_deleted: for false checks
Replace with not Prompt.is_deleted
(E712)
125-125: Avoid equality comparisons to False; use not PromptVersion.is_deleted: for false checks
Replace with not PromptVersion.is_deleted
(E712)
134-134: Unused function argument: project_id
(ARG001)
141-141: Avoid equality comparisons to False; use not PromptVersion.is_deleted: for false checks
Replace with not PromptVersion.is_deleted
(E712)
162-162: Avoid equality comparisons to False; use not PromptVersion.is_deleted: for false checks
Replace with not PromptVersion.is_deleted
(E712)
168-168: Avoid equality comparisons to False; use not Prompt.is_deleted: for false checks
Replace with not Prompt.is_deleted
(E712)
🔇 Additional comments (71)
backend/app/models/pagination.py (3)
1-8: LGTM!The imports and class definition follow standard SQLModel patterns with appropriate documentation.
9-13: Well-defined validation constraints.The field constraints are appropriate: non-negative totals and skip values, and reasonable limits (1-100) to prevent API abuse while ensuring usable pagination.
15-17: Clean factory method for building pagination responses.The
buildclass method provides a convenient way to construct pagination metadata in the expected response format. Usingmodel_dump()for serialization is the correct approach for Pydantic v2.backend/app/api/main.py (2)
13-14: LGTM!The new route imports are properly added and maintain the existing alphabetical ordering pattern.
37-38: LGTM!The router inclusions correctly register the new prompt and prompt version API endpoints following the established pattern.
backend/app/models/__init__.py (3)
66-66: LGTM!The Pagination import is correctly added for re-export. The static analysis warning about unused imports is a false positive - this is an
__init__.pyfile that serves as the public interface for the models package.
68-74: LGTM!The Prompt model imports follow the established pattern in the file with Base, Create, Public, and Update schemas. The static analysis warnings are false positives for
__init__.pyfiles that re-export models.
76-83: LGTM!The PromptVersion model imports are correctly structured, including the PromptVersionLabel enum. The static analysis warnings are expected false positives for model re-exports in
__init__.pyfiles.backend/app/alembic/versions/c6a3abcd37bc_added_prompt_table.py (4)
1-18: LGTM!The migration metadata is properly structured with clear title, unique revision ID, and correct revision chain.
22-39: Well-designed table structure.The prompt table includes appropriate fields with reasonable constraints:
- Name length (50 chars) is suitable for prompt names
- Optional description with sufficient space (500 chars)
- Proper foreign key relationship to project table
- Standard soft deletion and timestamp fields for audit trail
40-40: Good performance optimization.Creating an index on the
namecolumn is appropriate since prompt names will likely be used for filtering and searching operations.
44-48: LGTM!The downgrade function correctly reverses all changes by dropping the index before the table, enabling proper migration rollback.
backend/app/alembic/versions/913983f0a279_added_promptversion_table.py (4)
1-18: LGTM!The migration metadata correctly chains from the prompt table migration, establishing the proper dependency order.
22-47: Well-designed versioning table.The prompt_version table appropriately supports the versioning system with:
- Unlimited length instruction field for prompt content
- Reasonable commit message limit (512 chars)
- STAGING/PRODUCTION label enum for lifecycle management
- Proper foreign key relationship to prompt table
61-78: LGTM!The downgrade function correctly reverses all changes by recreating the foreign key constraints and dropping the prompt_version table, enabling proper migration rollback.
48-57: Confirm duplicate foreign key constraints onopenai_conversationbefore droppingI couldn’t find any migration that explicitly creates constraints named
openai_conversation_project_id_fkey1or
openai_conversation_organization_id_fkey1(nor the base names without “1”). The initialopenai_conversationmigration uses unnamedsa.ForeignKeyConstraint, so it’s unclear why the “1”-suffixed duplicates exist. Please verify:
- That your database actually has FK constraints named
…project_id_fkey1and…organization_id_fkey1.- That the original constraints (without the “1” suffix) remain intact and aren’t accidentally dropped.
- That this cleanup is intentional and not an unrelated change bundled with the
prompt_versiontable migration.backend/app/crud/__init__.py (1)
68-88: LGTM! Clean import organization for new prompt functionality.The new imports for prompt and prompt_version CRUD operations follow the established pattern and properly expose the necessary functions for the prompt management system.
backend/app/models/prompt.py (3)
1-4: LGTM! Proper imports and utilities for the model.The imports are correctly structured and the
nowutility provides consistent timestamp handling.
6-9: LGTM! Well-defined base class with proper validation.The validation constraints on
name(1-50 chars) anddescription(1-500 chars) are appropriate, and the field configurations follow SQLModel best practices.
22-39: LGTM! Complete API schemas following established patterns.The public, create, and update models are properly structured with appropriate field constraints and configuration.
backend/app/tests/api/routes/test_prompt_versions.py (6)
18-54: LGTM! Comprehensive test for prompt version creation.The test properly sets up the prompt, creates a version, and validates all expected fields including default values like
label: "staging"andversion: 1.
56-90: LGTM! Good test coverage for successful retrieval.The test validates both the API response structure and the data integrity of the retrieved prompt version.
92-109: LGTM! Proper error handling test for non-existent versions.The test correctly validates the 404 response and error message format.
111-161: LGTM! Thorough pagination testing.The test creates multiple versions, tests pagination parameters, and validates both data content and metadata structure.
163-199: LGTM! Tests the critical promotion functionality.The test validates the ability to promote a version to production, which is a key feature of the versioning system.
201-231: LGTM! Proper test for version deletion.The test validates both the API response and confirms the deletion occurred successfully.
backend/app/tests/crud/test_prompt.py (6)
1-15: LGTM! Proper imports for comprehensive CRUD testing.All imports are correctly used throughout the test file.
17-34: LGTM! Thorough test for successful prompt creation.The test validates all expected fields and behaviors of the created prompt.
36-52: LGTM! Proper test for duplicate name constraint.The test correctly validates that duplicate names within the same project raise a 409 error.
54-71: LGTM! Good test for successful prompt updates.The test validates that updates work correctly and modify the expected fields.
73-89: LGTM! Proper error handling test for non-existent prompts.The test correctly validates 404 errors when trying to update non-existent prompts.
126-248: LGTM! Comprehensive tests for retrieval, listing, and deletion.The tests properly cover:
- Retrieval by ID (success and failure cases)
- Retrieval by name within project (success and failure cases)
- Listing prompts by project
- Excluding soft-deleted prompts from listings
- Soft deletion functionality with proper flag setting
All tests follow good patterns and validate expected behaviors thoroughly.
backend/app/models/prompt_version.py (4)
1-5: LGTM! Clean imports and utility usage.The imports are properly organized and the
nowutility provides consistent timestamp handling.
7-10: LGTM! Well-defined enum for version labels.The
PromptVersionLabelenum clearly defines the two states with appropriate string values.
12-15: LGTM! Solid base class with proper validation.The field constraints are appropriate - required instruction with minimum length and optional commit message with reasonable max length.
37-52: LGTM! Complete API schemas with proper structure.The public, create, and update models are appropriately structured for their respective use cases.
backend/app/crud/prompt.py (2)
58-83: LGTM!The
create_promptfunction correctly validates uniqueness, handles errors appropriately, and maintains proper transaction boundaries.
131-149: LGTM!The soft deletion implementation is correct with proper timestamp tracking and error handling.
backend/app/api/routes/prompts.py (3)
30-44: LGTM!The create endpoint follows REST conventions with proper status code and delegates business logic to the CRUD layer.
89-107: LGTM!The list endpoint correctly implements pagination with proper metadata and query parameter validation.
109-124: LGTM!The delete endpoint follows REST conventions and returns an appropriate success message.
backend/app/tests/api/routes/test_prompts.py (1)
9-252: Good test coverage!The tests comprehensively cover all prompt CRUD operations with both success and failure scenarios. The assertions properly validate response data and database state.
backend/app/crud/prompt_version.py (3)
19-34: LGTM!Good design decision to include deleted versions when determining the next version number. This ensures version numbers are never reused.
36-69: LGTM!The version creation logic correctly validates the prompt existence and auto-increments the version number.
182-230: LGTM!Excellent implementation of the production version constraint. The logic correctly ensures only one production version exists by demoting the previous one.
backend/app/api/routes/prompt_versions.py (2)
29-46: LGTM!The version creation endpoint is well-implemented with proper validation and error handling delegated to the CRUD layer.
76-158: LGTM!The remaining endpoints (list, update, delete) are well-implemented with proper REST conventions, pagination support, and appropriate HTTP methods.
backend/app/tests/crud/test_prompt_version.py (24)
1-24: LGTM! Clean imports and comprehensive test setup.The imports are well-organized and include all necessary models, CRUD functions, and utilities for testing prompt version functionality. The import structure aligns with the FastAPI/SQLModel architecture.
27-37: LGTM! Well-designed fixture for test reusability.The prompt fixture provides a clean, reusable way to create test prompts with proper project association. This follows pytest best practices for test fixtures.
39-58: LGTM! Comprehensive test for successful prompt version creation.The test validates all essential aspects of prompt version creation including auto-incrementing version numbers, proper field assignment, and timestamp generation.
60-79: LGTM! Proper error handling verification.The test correctly validates that attempting to create a version for a non-existent prompt raises a 404 HTTPException with appropriate error messaging.
81-102: LGTM! Version increment logic validation.The test ensures that multiple prompt versions are correctly assigned incremental version numbers, which is crucial for the versioning system's integrity.
104-138: LGTM! Critical soft deletion behavior test.This test validates that soft-deleted versions don't interfere with version numbering, which is essential for maintaining version history integrity even after deletions.
140-166: LGTM! Next version calculation tests.Both tests properly validate the
get_next_prompt_versionfunction for empty and populated scenarios, ensuring correct version numbering logic.
168-192: LGTM! Soft deletion impact on versioning.The test correctly validates that soft-deleted versions are considered when calculating the next version number, preventing version conflicts.
194-217: LGTM! Successful retrieval test.The test validates successful prompt version retrieval with proper field validation and project scoping.
219-234: LGTM! Non-existent prompt error handling.The test properly validates 404 error handling when attempting to retrieve a version for a non-existent prompt.
236-262: LGTM! Soft deletion retrieval behavior.The test correctly validates that soft-deleted versions return None, ensuring they're properly hidden from normal operations.
264-291: LGTM! List functionality with ordering.The test validates that versions are returned in descending order, which is important for UI displaying the most recent versions first.
326-340: LGTM! Error handling for non-existent prompt.The test properly validates 404 error handling when listing versions for a non-existent prompt.
342-365: LGTM! Production version retrieval.The test correctly validates retrieval of production-labeled versions, which is crucial for the promotion/demotion system.
367-388: LGTM! No production version scenario.The test validates the None return when no production version exists, which is expected behavior for the system.
390-404: LGTM! Production version error handling.The test properly validates 404 error handling for production version retrieval with non-existent prompts.
406-440: LGTM! Version counting with soft deletion.The test validates that only non-deleted versions are counted, which is essential for accurate version management statistics.
442-463: LGTM! Label update functionality.The test validates successful label updates from None to STAGING, which is part of the version lifecycle management.
465-514: LGTM! Production promotion with demotion logic.This test validates the critical promotion/demotion workflow where promoting a version to PRODUCTION automatically demotes the previous production version to STAGING. This is essential for maintaining a single production version.
516-540: LGTM! Label update behavior.The test validates that label updates work correctly even when setting the same label, ensuring idempotent operations.
541-555: LGTM! Update error handling.The test properly validates 404 error handling when attempting to update a non-existent version.
557-584: LGTM! Soft deletion functionality.The test validates that deletion is soft (sets is_deleted=True) and that soft-deleted versions become inaccessible through normal retrieval methods.
586-610: LGTM! Production version deletion behavior.The test validates that when a production version is deleted, its label is demoted to STAGING before soft deletion, maintaining system consistency.
612-624: LGTM! Deletion error handling.The test properly validates 404 error handling when attempting to delete a non-existent version.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/app/tests/crud/test_prompt_version.py (1)
294-325: Remove unused variable assignment.The test logic is correct for validating exclusion of deleted versions, but there's an unused variable assignment.
Apply this diff to fix the unused variable:
- v1 = create_prompt_version( + create_prompt_version( session=db, prompt_id=prompt.id, prompt_version_in=PromptVersionCreate(instruction="v1", commit_message="init"), project_id=prompt.project_id, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/app/tests/api/routes/test_prompt_versions.py(1 hunks)backend/app/tests/crud/test_prompt.py(1 hunks)backend/app/tests/crud/test_prompt_version.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/app/tests/crud/test_prompt.py
- backend/app/tests/api/routes/test_prompt_versions.py
🧰 Additional context used
🪛 Ruff (0.12.2)
backend/app/tests/crud/test_prompt_version.py
296-296: Local variable v1 is assigned to but never used
Remove assignment to unused variable v1
(F841)
⏰ 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 (25)
backend/app/tests/crud/test_prompt_version.py (25)
1-25: LGTM! Clean imports and proper test structure.The imports are well-organized and cover all necessary components for comprehensive prompt version testing.
28-37: LGTM! Proper fixture setup.The prompt fixture correctly creates a reusable prompt instance for tests, following pytest best practices.
40-59: LGTM! Comprehensive creation test.The test properly validates all aspects of prompt version creation including auto-incrementing version numbers and timestamp generation.
61-80: LGTM! Proper error handling test.The test correctly validates that 404 exceptions are raised when attempting to create versions for non-existent prompts.
82-103: LGTM! Version increment logic validated.The test ensures version numbers increment correctly when multiple versions are created for the same prompt.
105-139: LGTM! Important versioning business rule validated.This test ensures that soft-deleted versions don't affect version numbering, which is crucial for maintaining version history integrity.
141-144: LGTM! Edge case handled properly.The test validates the initial state when no versions exist.
146-167: LGTM! Next version calculation validated.The test ensures proper version increment calculation when existing versions are present.
169-193: LGTM! Critical versioning behavior verified.This test confirms that deleted versions are included in version number calculation, preventing version number reuse.
195-218: LGTM! Successful retrieval test.The test validates correct prompt version retrieval by ID with proper assertions.
220-235: LGTM! Error handling for non-existent prompts.The test properly validates 404 exceptions when attempting to retrieve versions for non-existent prompts.
237-263: LGTM! Soft deletion behavior validated.The test ensures that soft-deleted versions return None when retrieved, maintaining data integrity.
265-292: LGTM! List functionality validated.The test confirms that versions are returned in descending order (newest first) and only non-deleted versions are included.
327-341: LGTM! Error handling for list operations.The test properly validates 404 exceptions when attempting to list versions for non-existent prompts.
343-366: LGTM! Production version retrieval validated.The test correctly validates retrieval of production-labeled prompt versions.
368-389: LGTM! Production version edge case handled.The test validates that None is returned when no production version exists, which is the correct behavior.
391-405: LGTM! Production version error handling.The test properly validates 404 exceptions for production version queries on non-existent prompts.
407-441: LGTM! Count functionality validated.The test ensures that only non-deleted versions are counted, which is crucial for pagination and statistics.
443-464: LGTM! Label update functionality validated.The test confirms that prompt version labels can be updated successfully.
466-515: LGTM! Critical production promotion logic validated.This test validates the important business rule that when a version is promoted to production, the existing production version is automatically demoted to staging. This ensures only one production version exists at a time.
517-541: LGTM! Label update optimization validated.The test validates the early return optimization when the label is unchanged, improving performance.
542-556: LGTM! Update error handling validated.The test properly validates 404 exceptions when attempting to update non-existent versions.
558-585: LGTM! Soft deletion validated.The test confirms that deletion is soft (sets is_deleted flag) rather than hard deletion, preserving data integrity.
587-611: LGTM! Production label reset on deletion validated.The test ensures that when a production version is deleted, its label is reset to staging, maintaining system consistency.
613-625: LGTM! Deletion error handling validated.The test properly validates 404 exceptions when attempting to delete non-existent versions.
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (2)
backend/app/crud/prompt_version.py (1)
271-273: Fix inconsistent logging of version identifierThe log message uses
prompt_version.idbut should useversionfor consistency with other log messages in this module.logger.info( - f"[delete_prompt_version] Deleted prompt version | Prompt ID: {prompt_id}, Version ID: {prompt_version.id}" + f"[delete_prompt_version] Deleted prompt version | Prompt ID: {prompt_id}, Version: {version}" )backend/app/tests/crud/test_prompt_version.py (1)
294-325: Remove unused variable assignmentThe variable
v1is assigned but never used in the test. Since you're only checking the returned versions list, you don't need to store the reference.def test_get_prompt_versions_excludes_deleted(db: Session, prompt: Prompt): """Should exclude soft-deleted prompt versions""" - v1 = create_prompt_version( + create_prompt_version( session=db, prompt_id=prompt.id, prompt_version_in=PromptVersionCreate(instruction="v1", commit_message="init"), project_id=prompt.project_id, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/app/api/routes/prompt_versions.py(1 hunks)backend/app/crud/__init__.py(1 hunks)backend/app/crud/prompt.py(1 hunks)backend/app/crud/prompt_version.py(1 hunks)backend/app/tests/crud/test_prompt_version.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/app/api/routes/prompt_versions.py
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: avirajsingh7
PR: ProjectTech4DevAI/ai-platform#319
File: backend/app/crud/prompt_version.py:134-146
Timestamp: 2025-08-06T05:33:19.776Z
Learning: In the prompt management system, `get_prompt_versions_count` is used only for pagination after `get_prompt_versions` which already validates the prompt exists, so duplicate validation is avoided for performance reasons.
📚 Learning: in the prompt management system, `get_prompt_versions_count` is used only for pagination after `get_...
Learnt from: avirajsingh7
PR: ProjectTech4DevAI/ai-platform#319
File: backend/app/crud/prompt_version.py:134-146
Timestamp: 2025-08-06T05:33:19.776Z
Learning: In the prompt management system, `get_prompt_versions_count` is used only for pagination after `get_prompt_versions` which already validates the prompt exists, so duplicate validation is avoided for performance reasons.
Applied to files:
backend/app/crud/prompt_version.pybackend/app/crud/__init__.pybackend/app/crud/prompt.pybackend/app/tests/crud/test_prompt_version.py
📚 Learning: in fastapi applications using pydantic/sqlmodel, when update model fields have min_length=1 validati...
Learnt from: avirajsingh7
PR: ProjectTech4DevAI/ai-platform#319
File: backend/app/crud/prompt.py:111-118
Timestamp: 2025-08-06T05:29:45.557Z
Learning: In FastAPI applications using Pydantic/SQLModel, when update model fields have min_length=1 validation, empty strings are rejected at the validation layer before reaching CRUD functions. This means checking `if field:` vs `if field is not None:` in CRUD operations should consider the model validation constraints.
Applied to files:
backend/app/crud/prompt.py
🧬 Code Graph Analysis (1)
backend/app/crud/prompt.py (2)
backend/app/models/prompt.py (3)
Prompt(17-25)PromptCreate(35-36)PromptUpdate(39-44)backend/app/tests/crud/test_prompt_version.py (1)
prompt(29-37)
🪛 Ruff (0.12.2)
backend/app/crud/prompt_version.py
84-84: Avoid equality comparisons to False; use not PromptVersion.is_deleted: for false checks
Replace with not PromptVersion.is_deleted
(E712)
90-90: Avoid equality comparisons to False; use not Prompt.is_deleted: for false checks
Replace with not Prompt.is_deleted
(E712)
125-125: Avoid equality comparisons to False; use not PromptVersion.is_deleted: for false checks
Replace with not PromptVersion.is_deleted
(E712)
151-151: Avoid equality comparisons to False; use not PromptVersion.is_deleted: for false checks
Replace with not PromptVersion.is_deleted
(E712)
172-172: Avoid equality comparisons to False; use not PromptVersion.is_deleted: for false checks
Replace with not PromptVersion.is_deleted
(E712)
178-178: Avoid equality comparisons to False; use not Prompt.is_deleted: for false checks
Replace with not Prompt.is_deleted
(E712)
backend/app/crud/__init__.py
69-69: .prompt.count_prompts_by_project imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
70-70: .prompt.create_prompt imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
71-71: .prompt.delete_prompt imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
72-72: .prompt.get_prompt_by_id imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
73-73: .prompt.get_prompt_by_name_in_project imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
74-74: .prompt.get_prompt_by_project imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
75-75: .prompt.update_prompt imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
80-80: .prompt_version.create_prompt_version imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
81-81: .prompt_version.delete_prompt_version imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
82-82: .prompt_version.get_next_prompt_version imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
83-83: .prompt_version.get_prompt_version_by_id imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
84-84: .prompt_version.get_prompt_versions imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
85-85: .prompt_version.get_prompt_versions_with_count imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
86-86: .prompt_version.get_production_prompt_version imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
87-87: .prompt_version.update_prompt_version imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
backend/app/crud/prompt.py
20-20: Avoid equality comparisons to False; use not Prompt.is_deleted: for false checks
Replace with not Prompt.is_deleted
(E712)
32-32: Avoid equality comparisons to False; use not Prompt.is_deleted: for false checks
Replace with not Prompt.is_deleted
(E712)
42-42: Avoid equality comparisons to False; use not Prompt.is_deleted: for false checks
Replace with not Prompt.is_deleted
(E712)
53-53: Avoid equality comparisons to False; use not Prompt.is_deleted: for false checks
Replace with not Prompt.is_deleted
(E712)
backend/app/tests/crud/test_prompt_version.py
296-296: Local variable v1 is assigned to but never used
Remove assignment to unused variable v1
(F841)
⏰ 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)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/app/tests/crud/test_prompt_version.py (1)
265-341: Fix unused variable assignment.The variable
v1is assigned but never used in the test.def test_get_prompt_versions_excludes_deleted(db: Session, prompt: Prompt): """Should exclude soft-deleted prompt versions""" - v1 = create_prompt_version( + create_prompt_version( session=db, prompt_id=prompt.id, prompt_version_in=PromptVersionCreate(instruction="v1", commit_message="init"), project_id=prompt.project_id, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/tests/crud/test_prompt_version.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: avirajsingh7
PR: ProjectTech4DevAI/ai-platform#319
File: backend/app/crud/prompt_version.py:134-146
Timestamp: 2025-08-06T05:33:19.776Z
Learning: In the prompt management system, `get_prompt_versions_count` is used only for pagination after `get_prompt_versions` which already validates the prompt exists, so duplicate validation is avoided for performance reasons.
📚 Learning: in the prompt management system, `get_prompt_versions_count` is used only for pagination after `get_...
Learnt from: avirajsingh7
PR: ProjectTech4DevAI/ai-platform#319
File: backend/app/crud/prompt_version.py:134-146
Timestamp: 2025-08-06T05:33:19.776Z
Learning: In the prompt management system, `get_prompt_versions_count` is used only for pagination after `get_prompt_versions` which already validates the prompt exists, so duplicate validation is avoided for performance reasons.
Applied to files:
backend/app/tests/crud/test_prompt_version.py
🪛 Ruff (0.12.2)
backend/app/tests/crud/test_prompt_version.py
296-296: Local variable v1 is assigned to but never used
Remove assignment to unused variable v1
(F841)
⏰ 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 (11)
backend/app/tests/crud/test_prompt_version.py (11)
1-37: LGTM! Well-structured test setup.The imports are comprehensive and the fixture design is clean, providing a reusable prompt instance for tests.
40-103: LGTM! Comprehensive creation tests.The test cases properly cover successful creation, version incrementing, and error handling for non-existent prompts. Good validation of the core versioning logic.
105-139: LGTM! Important edge case covered.This test correctly validates that soft-deleted versions don't affect the version numbering sequence, which is crucial for maintaining version consistency.
141-193: LGTM! Thorough next version number logic.Good coverage of the version number calculation including empty state, existing versions, and deleted versions scenarios.
195-263: LGTM! Complete get-by-ID test coverage.Tests properly validate retrieval success, handling of soft-deleted versions returning None, and appropriate 404 errors for missing prompts.
343-405: LGTM! Production version retrieval tests.Good coverage of production version retrieval including success case, None return when no production version exists, and 404 error handling.
407-446: LGTM! Pagination function properly tested.The test correctly validates that
get_prompt_versions_with_countexcludes deleted versions from both the list and count, which is essential for accurate pagination.
448-561: LGTM! Comprehensive update operation tests.Excellent coverage of label updates, promotion/demotion logic, early return optimization, and error handling. The promotion test properly validates that only one version can be PRODUCTION at a time.
563-591: LGTM! Soft deletion properly tested.The test correctly validates that deleted versions are excluded from retrieval via
get_prompt_version_by_idreturning None.
592-620: LGTM! Production label reset properly tested.This test correctly addresses the previous review concern by manually setting the label to PRODUCTION after creation (lines 604-607) rather than passing it to PromptVersionCreate. The test properly validates that deletion resets the label from PRODUCTION to STAGING.
622-634: LGTM! Error handling for non-existent versions.Proper validation of 404 error when attempting to delete a non-existent version.
backend/app/alembic/versions/c6a3abcd37bc_added_prompt_table.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
backend/app/crud/prompt.py (3)
17-22: Use SQLAlchemy boolean comparator instead of == FalseReplace direct boolean comparison with
.is_(False)to satisfy linters and generate correct SQL.- select(Prompt).where( - Prompt.name == name, - Prompt.project_id == project_id, - Prompt.is_deleted == False, - ) + select(Prompt).where( + Prompt.name == name, + Prompt.project_id == project_id, + Prompt.is_deleted.is_(False), + )
29-34: Use.is_(False)for boolean column in filtersSame fix here for consistency and correctness.
- select(Prompt).where( - Prompt.id == prompt_id, - Prompt.project_id == project_id, - Prompt.is_deleted == False, - ) + select(Prompt).where( + Prompt.id == prompt_id, + Prompt.project_id == project_id, + Prompt.is_deleted.is_(False), + )
41-46: Replace== Falsewith.is_(False)in list queryAlign with SQLAlchemy best practices.
- .where(Prompt.project_id == project_id, Prompt.is_deleted == False) + .where(Prompt.project_id == project_id, Prompt.is_deleted.is_(False))backend/app/tests/api/routes/test_prompts.py (1)
129-131: Remove duplicate imports in the middle of the fileThese shadow the top-level imports and violate import-at-top (E402) and redefinition (F811).
-from app.models import PromptCreate -from app.crud.prompt import get_prompt_by_name_in_project -
🧹 Nitpick comments (5)
backend/app/crud/prompt.py (4)
116-124: Avoid shadowing function name with local variable
update_prompt = prompt_update.model_dump(...)shadows the function name, hurting readability and tooling. Rename toupdate_data.- update_prompt = prompt_update.model_dump(exclude_unset=True) - if update_prompt: - for field, value in update_prompt.items(): + update_data = prompt_update.model_dump(exclude_unset=True) + if update_data: + for field, value in update_data.items(): setattr(prompt, field, value)
37-46: Optional: enforce a maximum page sizeConsider clamping limit to a MAX_PAGE_SIZE to avoid unbounded queries and to protect the DB.
-def get_prompt_by_project( - session: Session, project_id: int, skip: int = 0, limit: int = 100 -) -> list[Prompt]: +MAX_PAGE_SIZE = 100 + +def get_prompt_by_project( + session: Session, project_id: int, skip: int = 0, limit: int = MAX_PAGE_SIZE +) -> list[Prompt]: + limit = min(limit, MAX_PAGE_SIZE)
142-149: Optional: also bump updated_at on deleteYou already set deleted_at. If consumers sort by updated_at, consider updating it too for consistency.
prompt.is_deleted = True prompt.deleted_at = now() + prompt.updated_at = prompt.deleted_at
60-71: Confirm case sensitivity requirements for prompt namesCurrent checks are case-sensitive. If the product expects case-insensitive uniqueness within a project, use
func.lower(Prompt.name)in queries and create a corresponding index.I can update the query and provide an Alembic migration for a partial unique index on
(project_id, lower(name)) WHERE is_deleted = falseif needed.Also applies to: 98-114
backend/app/tests/api/routes/test_prompts.py (1)
79-83: Silence unused fixture warnings by prefixing with underscore
dbisn’t used in these tests; rename to_dbto make intent explicit and satisfy linters.def test_update_prompt_duplicate_name( client: TestClient, - db: Session, + _db: Session, user_api_key: APIKeyPublic, ): @@ def test_get_prompt_success( client: TestClient, - db: Session, + _db: Session, user_api_key: APIKeyPublic, ): @@ def test_list_prompts_success( client: TestClient, - db: Session, + _db: Session, user_api_key: APIKeyPublic, ):Also applies to: 134-137, 179-182
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/app/api/routes/prompt_versions.py(1 hunks)backend/app/api/routes/prompts.py(1 hunks)backend/app/crud/prompt.py(1 hunks)backend/app/tests/api/routes/test_prompt_versions.py(1 hunks)backend/app/tests/api/routes/test_prompts.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- backend/app/api/routes/prompt_versions.py
- backend/app/api/routes/prompts.py
- backend/app/tests/api/routes/test_prompt_versions.py
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: avirajsingh7
PR: ProjectTech4DevAI/ai-platform#319
File: backend/app/crud/prompt_version.py:134-146
Timestamp: 2025-08-06T05:33:19.776Z
Learning: In the prompt management system, `get_prompt_versions_count` is used only for pagination after `get_prompt_versions` which already validates the prompt exists, so duplicate validation is avoided for performance reasons.
📚 Learning: 2025-08-06T05:33:19.776Z
Learnt from: avirajsingh7
PR: ProjectTech4DevAI/ai-platform#319
File: backend/app/crud/prompt_version.py:134-146
Timestamp: 2025-08-06T05:33:19.776Z
Learning: In the prompt management system, `get_prompt_versions_count` is used only for pagination after `get_prompt_versions` which already validates the prompt exists, so duplicate validation is avoided for performance reasons.
Applied to files:
backend/app/crud/prompt.pybackend/app/tests/api/routes/test_prompts.py
📚 Learning: 2025-08-06T05:29:45.557Z
Learnt from: avirajsingh7
PR: ProjectTech4DevAI/ai-platform#319
File: backend/app/crud/prompt.py:111-118
Timestamp: 2025-08-06T05:29:45.557Z
Learning: In FastAPI applications using Pydantic/SQLModel, when update model fields have min_length=1 validation, empty strings are rejected at the validation layer before reaching CRUD functions. This means checking `if field:` vs `if field is not None:` in CRUD operations should consider the model validation constraints.
Applied to files:
backend/app/crud/prompt.py
🪛 Ruff (0.12.2)
backend/app/crud/prompt.py
20-20: Avoid equality comparisons to False; use not Prompt.is_deleted: for false checks
Replace with not Prompt.is_deleted
(E712)
32-32: Avoid equality comparisons to False; use not Prompt.is_deleted: for false checks
Replace with not Prompt.is_deleted
(E712)
42-42: Avoid equality comparisons to False; use not Prompt.is_deleted: for false checks
Replace with not Prompt.is_deleted
(E712)
53-53: Avoid equality comparisons to False; use not Prompt.is_deleted: for false checks
Replace with not Prompt.is_deleted
(E712)
backend/app/tests/api/routes/test_prompts.py
80-80: Unused function argument: db
(ARG001)
129-129: Module level import not at top of file
(E402)
129-129: Redefinition of unused PromptCreate from line 4
Remove definition: PromptCreate
(F811)
130-130: Module level import not at top of file
(E402)
130-130: Redefinition of unused get_prompt_by_name_in_project from line 5
Remove definition: get_prompt_by_name_in_project
(F811)
135-135: Unused function argument: db
(ARG001)
180-180: Unused function argument: db
(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)
avirajsingh7
force-pushed
the
feature/prompt-management
branch
from
4093314 to
db872b9
Compare
There was a problem hiding this comment.
Actionable comments posted: 20
🔭 Outside diff range comments (1)
backend/app/tests/api/routes/test_responses.py (1)
1-663: Do not comment out entire test modules — skip or remove themCommenting out the whole file hides intent and breaks test hygiene; mark the module skipped with a clear reason or remove/move it.
- backend/app/tests/api/routes/test_responses.py — entire file
Suggested minimal replacement:
-# 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 -# -# # Wrap the router in a FastAPI app instance -# app = FastAPI() -# app.include_router(router) -# client = TestClient(app) -# -# ... +import pytest + +# Temporarily disable responses tests while prompt/versioning API evolves (PR #319). +# TODO: Re-enable after rebasing tests on the updated responses API or remove if deprecated. +pytestmark = pytest.mark.skip(reason="Temporarily disabled: responses tests pending refactor in PR #319")Alternatively delete this file or move it to tests/_disabled/ to make the intent explicit.
Would you like me to rework these tests against the current responses API, or open a tracking issue to re-enable them post-merge?
♻️ Duplicate comments (1)
backend/app/crud/__init__.py (1)
67-77: Add file-level F401 suppression for export aggregator (repeat of prior feedback)As noted in earlier reviews, this file is an export hub. Add a ruff directive to avoid F401 errors.
+ # ruff: noqa: F401 # this module re-exports CRUD functions for the public API from .user import (
🧹 Nitpick comments (7)
backend/app/tests/api/routes/test_prompt_versions.py (2)
49-58: Prefer using the API or CRUD to create the second version; at minimum, refresh before using IDDirectly instantiating PromptVersion bypasses validations and future logic in create_prompt_version (e.g., timestamps, invariants). Two options:
- Preferred: call the API POST /prompts/{id}/versions to create v2.
- Or call create_prompt_version(session, prompt.id, ...).
If you keep the direct insert, refresh before using the ID to avoid ORM state edge cases across DB backends.
second_version = PromptVersion( prompt_id=prompt.id, instruction="Second instruction", commit_message="Second version", version=2, ) db.add(second_version) - db.commit() + db.commit() + db.refresh(second_version)Please confirm if you’d like me to convert this test to create the second version via the API route for end-to-end coverage.
64-68: Avoid brittle assertions on exact message stringsAsserting exact response messages can cause incidental failures on copy tweaks. Consider asserting success status and response shape instead.
Example:
- assert response_data["success"] is True - assert response_data["data"]["message"] == "Prompt version deleted successfully." + assert response_data["success"] is True + assert "data" in response_data and "message" in response_data["data"]backend/app/crud/prompts.py (2)
75-75: Use idiomatic boolean comparison.Replace the equality comparison with the more idiomatic
.is_(False)for consistency with the rest of the codebase.- .where(Prompt.project_id == project_id, Prompt.is_deleted == False) + .where(Prompt.project_id == project_id, Prompt.is_deleted.is_(False))
101-144: Consider optimizing the query for non-versioned retrieval.When
include_versions=False, the function still fetches all columns from both tables even though only one version is needed. Consider using a more targeted query to improve performance.For better performance when fetching only the active version, consider using separate optimized queries:
if not include_versions: # Optimized query for single active version stmt = ( select(Prompt, PromptVersion) .join( PromptVersion, PromptVersion.id == Prompt.active_version, ) .where( Prompt.id == prompt_id, Prompt.project_id == project_id, Prompt.is_deleted.is_(False), PromptVersion.is_deleted.is_(False), ) ) result = session.exec(stmt).first() if not result: logger.error(...) raise HTTPException(...) prompt, version = result return prompt, [version] if version else []backend/app/tests/crud/test_prompts.py (1)
114-117: Prefer using the CRUD delete for consistency.Directly toggling is_deleted bypasses business logic (timestamps, logs). Use delete_prompt for consistency and to exercise the intended path.
Apply this diff:
- prompt.is_deleted = True - db.add(prompt) - db.commit() + delete_prompt(db, prompt_id=prompt.id, project_id=project.id)backend/app/models/prompt.py (1)
29-40: Active version default may momentarily violate FK (relying on deferred checks).Using a random UUID as the default is acceptable only because the FK is initially DEFERRED and you update active_version before commit. Consider removing the default and setting active_version explicitly after creating the first version to avoid transient invalid references.
If you want to avoid the transient invalid FK while keeping NOT NULL:
- Keep the column without a default in the model (no default_factory), still NOT NULL in DB.
- In create flow, use a single transaction: create prompt (without active_version value), create version, then set prompt.active_version to the version’s id before the first flush/commit.
I can provide a concrete refactor if you want to pursue this.
backend/app/api/routes/prompts.py (1)
107-109: Docstring: mention active_version updates are supported.The route supports updating active_version, not only name/description.
Apply this diff:
- Update a prompt's name or description. + Update a prompt's name, description, or active_version.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
backend/app/alembic/versions/757f50ada8ef_add_prompt_and_version_table.py(1 hunks)backend/app/api/main.py(2 hunks)backend/app/api/routes/prompt_versions.py(1 hunks)backend/app/api/routes/prompts.py(1 hunks)backend/app/crud/__init__.py(1 hunks)backend/app/crud/prompt_versions.py(1 hunks)backend/app/crud/prompts.py(1 hunks)backend/app/models/__init__.py(1 hunks)backend/app/models/prompt.py(1 hunks)backend/app/models/prompt_version.py(1 hunks)backend/app/tests/api/routes/test_prompt_versions.py(1 hunks)backend/app/tests/api/routes/test_prompts.py(1 hunks)backend/app/tests/api/routes/test_responses.py(1 hunks)backend/app/tests/crud/test_prompt_versions.py(1 hunks)backend/app/tests/crud/test_prompts.py(1 hunks)backend/app/tests/utils/test_data.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/app/api/main.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-06T05:33:19.776Z
Learnt from: avirajsingh7
PR: ProjectTech4DevAI/ai-platform#319
File: backend/app/crud/prompt_version.py:134-146
Timestamp: 2025-08-06T05:33:19.776Z
Learning: In the prompt management system, `get_prompt_versions_count` is used only for pagination after `get_prompt_versions` which already validates the prompt exists, so duplicate validation is avoided for performance reasons.
Applied to files:
backend/app/crud/prompts.pybackend/app/crud/prompt_versions.pybackend/app/api/routes/prompt_versions.py
🧬 Code Graph Analysis (14)
backend/app/crud/prompts.py (2)
backend/app/models/prompt.py (5)
Prompt(20-50)PromptCreate(69-70)PromptUpdate(73-79)PromptWithVersion(61-62)PromptWithVersions(65-66)backend/app/models/prompt_version.py (1)
PromptVersion(18-38)
backend/app/tests/crud/test_prompt_versions.py (6)
backend/app/crud/prompt_versions.py (3)
create_prompt_version(31-59)delete_prompt_version(62-103)get_next_prompt_version(14-28)backend/app/crud/prompts.py (1)
create_prompt(20-51)backend/app/models/prompt.py (2)
Prompt(20-50)PromptCreate(69-70)backend/app/models/prompt_version.py (2)
PromptVersion(18-38)PromptVersionCreate(49-50)backend/app/tests/utils/utils.py (1)
get_project(70-89)backend/app/tests/utils/test_data.py (1)
create_test_prompt(120-135)
backend/app/crud/prompt_versions.py (2)
backend/app/crud/prompts.py (1)
prompt_exists(81-98)backend/app/models/prompt_version.py (2)
PromptVersion(18-38)PromptVersionCreate(49-50)
backend/app/alembic/versions/757f50ada8ef_add_prompt_and_version_table.py (4)
backend/app/alembic/versions/0f205e3779ee_add_api_key_table.py (1)
upgrade(20-39)backend/app/alembic/versions/c43313eca57d_add_document_tables.py (1)
upgrade(20-36)backend/app/alembic/versions/e2412789c190_initialize_models.py (1)
upgrade(19-46)backend/app/alembic/versions/8e7dc5eab0b0_add_fk_constraint_to_user_id_columns.py (1)
upgrade(20-42)
backend/app/api/routes/prompt_versions.py (5)
backend/app/api/deps.py (1)
get_db(33-35)backend/app/crud/prompt_versions.py (2)
create_prompt_version(31-59)delete_prompt_version(62-103)backend/app/models/prompt_version.py (2)
PromptVersionCreate(49-50)PromptVersionPublic(41-46)backend/app/utils.py (2)
APIResponse(27-48)success_response(34-37)backend/app/api/routes/assistants.py (1)
update_assistant_route(74-93)
backend/app/tests/crud/test_prompts.py (6)
backend/app/crud/prompts.py (7)
count_prompts_in_project(71-78)create_prompt(20-51)delete_prompt(181-191)get_prompt_by_id(101-143)get_prompts(54-68)prompt_exists(81-98)update_prompt(146-178)backend/app/models/prompt.py (3)
Prompt(20-50)PromptCreate(69-70)PromptUpdate(73-79)backend/app/models/prompt_version.py (1)
PromptVersion(18-38)backend/app/tests/utils/utils.py (1)
get_project(70-89)backend/app/tests/utils/test_data.py (1)
create_test_prompt(120-135)backend/app/tests/conftest.py (1)
db(18-35)
backend/app/tests/api/routes/test_prompts.py (8)
backend/app/models/api_key.py (1)
APIKeyPublic(23-25)backend/app/models/prompt.py (2)
PromptCreate(69-70)PromptUpdate(73-79)backend/app/models/prompt_version.py (1)
PromptVersion(18-38)backend/app/tests/utils/test_data.py (1)
create_test_prompt(120-135)backend/app/tests/conftest.py (2)
db(18-35)user_api_key(83-85)backend/app/tests/crud/test_prompt_versions.py (1)
prompt(20-24)backend/app/tests/crud/test_prompts.py (1)
prompt(23-27)backend/app/alembic/versions/0f205e3779ee_add_api_key_table.py (1)
upgrade(20-39)
backend/app/models/prompt_version.py (2)
backend/app/models/prompt.py (1)
Prompt(20-50)backend/app/models/project_user.py (1)
ProjectUserPublic(20-23)
backend/app/tests/api/routes/test_prompt_versions.py (4)
backend/app/crud/prompts.py (1)
create_prompt(20-51)backend/app/models/prompt.py (1)
PromptCreate(69-70)backend/app/models/prompt_version.py (2)
PromptVersion(18-38)PromptVersionCreate(49-50)backend/app/tests/utils/test_data.py (1)
create_test_prompt(120-135)
backend/app/models/prompt.py (1)
backend/app/models/prompt_version.py (3)
PromptVersion(18-38)PromptVersionCreate(49-50)PromptVersionPublic(41-46)
backend/app/tests/utils/test_data.py (5)
backend/app/models/prompt.py (1)
PromptCreate(69-70)backend/app/models/prompt_version.py (1)
PromptVersion(18-38)backend/app/models/credentials.py (1)
CredsCreate(19-28)backend/app/crud/prompts.py (1)
create_prompt(20-51)backend/app/tests/conftest.py (1)
db(18-35)
backend/app/api/routes/prompts.py (4)
backend/app/api/deps.py (1)
get_db(33-35)backend/app/crud/prompts.py (6)
create_prompt(20-51)delete_prompt(181-191)get_prompt_by_id(101-143)get_prompts(54-68)count_prompts_in_project(71-78)update_prompt(146-178)backend/app/models/prompt.py (5)
PromptCreate(69-70)PromptPublic(53-58)PromptUpdate(73-79)PromptWithVersion(61-62)PromptWithVersions(65-66)backend/app/utils.py (2)
APIResponse(27-48)success_response(34-37)
backend/app/crud/__init__.py (2)
backend/app/crud/prompt_versions.py (2)
create_prompt_version(31-59)delete_prompt_version(62-103)backend/app/crud/prompts.py (6)
create_prompt(20-51)count_prompts_in_project(71-78)delete_prompt(181-191)get_prompt_by_id(101-143)get_prompts(54-68)update_prompt(146-178)
backend/app/models/__init__.py (2)
backend/app/models/prompt.py (6)
Prompt(20-50)PromptCreate(69-70)PromptPublic(53-58)PromptUpdate(73-79)PromptWithVersion(61-62)PromptWithVersions(65-66)backend/app/models/prompt_version.py (3)
PromptVersion(18-38)PromptVersionCreate(49-50)PromptVersionPublic(41-46)
🪛 Ruff (0.12.2)
backend/app/crud/prompts.py
13-13: app.models.PromptWithVersion imported but unused
Remove unused import
(F401)
14-14: app.models.PromptWithVersions imported but unused
Remove unused import
(F401)
75-75: Avoid equality comparisons to False; use not Prompt.is_deleted: for false checks
Replace with not Prompt.is_deleted
(E712)
backend/app/tests/crud/test_prompt_versions.py
13-13: app.crud.prompts.create_prompt imported but unused
Remove unused import: app.crud.prompts.create_prompt
(F401)
14-14: app.models.PromptCreate imported but unused
Remove unused import: app.models.PromptCreate
(F401)
backend/app/tests/api/routes/test_prompts.py
12-12: Unused function argument: db
(ARG001)
61-61: f-string without any placeholders
Remove extraneous f prefix
(F541)
83-83: Unused function argument: db
(ARG001)
88-88: f-string without any placeholders
Remove extraneous f prefix
(F541)
113-113: f-string without any placeholders
Remove extraneous f prefix
(F541)
196-196: Unused function argument: db
(ARG001)
backend/app/tests/api/routes/test_prompt_versions.py
4-4: app.crud.prompts.create_prompt imported but unused
Remove unused import: app.crud.prompts.create_prompt
(F401)
5-5: app.models.PromptCreate imported but unused
Remove unused import: app.models.PromptCreate
(F401)
backend/app/crud/__init__.py
68-68: .prompt_versions.create_prompt_version imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
68-68: .prompt_versions.delete_prompt_version imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
71-71: .prompts.create_prompt imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
72-72: .prompts.count_prompts_in_project imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
73-73: .prompts.delete_prompt imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
74-74: .prompts.get_prompt_by_id imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
75-75: .prompts.get_prompts imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
76-76: .prompts.update_prompt imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
backend/app/models/__init__.py
67-67: .prompt.Prompt imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
68-68: .prompt.PromptCreate imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
69-69: .prompt.PromptPublic imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
70-70: .prompt.PromptUpdate imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
71-71: .prompt.PromptWithVersion imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
72-72: .prompt.PromptWithVersions imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
76-76: .prompt_version.PromptVersion imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
77-77: .prompt_version.PromptVersionCreate imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
78-78: .prompt_version.PromptVersionPublic imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
🔇 Additional comments (4)
backend/app/api/routes/prompt_versions.py (1)
1-56: LGTM! Clean and well-structured API routes.The implementation follows FastAPI best practices with proper:
- HTTP status codes (201 for creation, 200 for deletion)
- Dependency injection for authentication and database sessions
- Structured response models using APIResponse
- Error handling delegated to CRUD layer
backend/app/crud/prompt_versions.py (1)
62-104: Well-implemented soft delete with proper validation.The delete function correctly:
- Validates prompt existence
- Prevents deletion of active versions (409 Conflict)
- Performs soft delete with proper timestamps
- Handles not-found cases appropriately
backend/app/alembic/versions/757f50ada8ef_add_prompt_and_version_table.py (1)
22-47: Circular FK handled correctly with deferrable/use_alter.Declaring the active_version FK with deferrable and use_alter (and creating prompt before prompt_version) is the right approach for this cycle. After removing the extra op.create_foreign_key call, the migration should apply cleanly.
backend/app/api/routes/prompts.py (1)
8-15: Import CRUD functions from the concrete module (avoid relying on re-exports)Importing from
app.crudassumesapp/crud/__init__.pyre-exports these symbols. Prefer an explicit import fromapp.crud.promptsto avoid ImportError.
- Location: backend/app/api/routes/prompts.py (lines ~8–15)
Apply this diff:
-from app.crud import ( - create_prompt, - delete_prompt, - get_prompt_by_id, - get_prompts, - count_prompts_in_project, - update_prompt, -) +from app.crud.prompts import ( + create_prompt, + delete_prompt, + get_prompt_by_id, + get_prompts, + count_prompts_in_project, + update_prompt, +)Verification note: my attempt to check the repo failed because
app/crudwas not found in the workspace (script output showed "app/crud: No such file or directory"). Please confirm whetherapp/crud/__init__.pyre-exports these functions; if it does not, apply the diff above. To verify locally, run:
- rg -n '^def (create_prompt|get_prompts|count_prompts_in_project|get_prompt_by_id|update_prompt|delete_prompt)('
- rg -n 'from .prompts import' app/crud/init.py
| op.create_foreign_key( | ||
| None, | ||
| "prompt", | ||
| "prompt_version", | ||
| ["active_version"], | ||
| ["id"], | ||
| initially="DEFERRED", | ||
| deferrable=True, | ||
| use_alter=True, | ||
| ) |
There was a problem hiding this comment.
Duplicate/invalid FK creation: remove op.create_foreign_key and the unsupported use_alter argument.
You already declare the active_version FK in the prompt table with use_alter=True (which Alembic emits as a separate ALTER after both tables exist). The extra op.create_foreign_key block introduces a duplicate FK and includes an unsupported parameter (use_alter), which will raise a TypeError.
Apply this diff to remove the redundant/invalid FK creation:
- op.create_foreign_key(
- None,
- "prompt",
- "prompt_version",
- ["active_version"],
- ["id"],
- initially="DEFERRED",
- deferrable=True,
- use_alter=True,
- )📝 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.
| op.create_foreign_key( | |
| None, | |
| "prompt", | |
| "prompt_version", | |
| ["active_version"], | |
| ["id"], | |
| initially="DEFERRED", | |
| deferrable=True, | |
| use_alter=True, | |
| ) |
🤖 Prompt for AI Agents
In backend/app/alembic/versions/757f50ada8ef_add_prompt_and_version_table.py
around lines 80 to 89, there is a redundant/invalid op.create_foreign_key call
that duplicates the already-declared active_version FK and includes the
unsupported use_alter argument; remove the entire op.create_foreign_key block
(or at minimum drop the duplicate FK creation and the use_alter parameter) so
the FK is declared only once and no unsupported parameters are passed to
Alembic.
| from app.models import ( | ||
| PromptCreate, | ||
| PromptPublic, | ||
| PromptUpdate, | ||
| PromptWithVersion, | ||
| PromptWithVersions, | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing import for PromptVersionPublic used in response shaping below.
You construct responses with version(s). Import PromptVersionPublic to ensure explicit conversion and correct OpenAPI schema generation.
Apply this diff:
from app.models import (
PromptCreate,
PromptPublic,
PromptUpdate,
PromptWithVersion,
PromptWithVersions,
+ PromptVersionPublic,
)📝 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.
| from app.models import ( | |
| PromptCreate, | |
| PromptPublic, | |
| PromptUpdate, | |
| PromptWithVersion, | |
| PromptWithVersions, | |
| ) | |
| from app.models import ( | |
| PromptCreate, | |
| PromptPublic, | |
| PromptUpdate, | |
| PromptWithVersion, | |
| PromptWithVersions, | |
| PromptVersionPublic, | |
| ) |
🤖 Prompt for AI Agents
In backend/app/api/routes/prompts.py around lines 16 to 22, the import list for
models is missing PromptVersionPublic which is later used to shape responses;
update the import tuple to include PromptVersionPublic so responses explicitly
convert versions using that Pydantic model and the OpenAPI schema includes the
version shape.
| prompt_with_version = PromptWithVersion(**prompt.model_dump(), version=version) | ||
| return APIResponse.success_response(prompt_with_version) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure correct serialization for nested version in the create response.
Instantiate the public schema to avoid leaking internal fields and to ensure Pydantic validation is deterministic.
Apply this diff:
- prompt_with_version = PromptWithVersion(**prompt.model_dump(), version=version)
+ prompt_with_version = PromptWithVersion(
+ **prompt.model_dump(),
+ version=PromptVersionPublic.model_validate(version),
+ )📝 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.
| prompt_with_version = PromptWithVersion(**prompt.model_dump(), version=version) | |
| return APIResponse.success_response(prompt_with_version) | |
| prompt_with_version = PromptWithVersion( | |
| **prompt.model_dump(), | |
| version=PromptVersionPublic.model_validate(version), | |
| ) | |
| return APIResponse.success_response(prompt_with_version) |
🤖 Prompt for AI Agents
In backend/app/api/routes/prompts.py around lines 42 to 43, replace the current
creation of the response (which directly uses the internal Prompt model) with
instantiation of the public Pydantic response schema so internal fields are not
leaked and validation is deterministic; build a data dict from
prompt.model_dump(), add the version field, then pass that dict into the public
response schema class (instead of using the internal model) and return
APIResponse.success_response(the_public_schema_instance).
| prompt_with_versions = PromptWithVersions(**prompt.model_dump(), versions=versions) | ||
| return APIResponse.success_response(prompt_with_versions) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Serialize versions to their public schema for consistent API responses.
Convert ORM models to PromptVersionPublic to avoid accidental leakage of internal fields and to be robust to Pydantic parsing differences.
Apply this diff:
- prompt_with_versions = PromptWithVersions(**prompt.model_dump(), versions=versions)
+ versions_public = [PromptVersionPublic.model_validate(v) for v in versions]
+ prompt_with_versions = PromptWithVersions(
+ **prompt.model_dump(),
+ versions=versions_public,
+ )📝 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.
| prompt_with_versions = PromptWithVersions(**prompt.model_dump(), versions=versions) | |
| return APIResponse.success_response(prompt_with_versions) | |
| versions_public = [PromptVersionPublic.model_validate(v) for v in versions] | |
| prompt_with_versions = PromptWithVersions( | |
| **prompt.model_dump(), | |
| versions=versions_public, | |
| ) | |
| return APIResponse.success_response(prompt_with_versions) |
🤖 Prompt for AI Agents
In backend/app/api/routes/prompts.py around lines 96-97, the code returns
PromptWithVersions using raw ORM version objects which can leak internal fields
and be brittle; convert each version to the public schema before returning.
Replace building PromptWithVersions(..., versions=versions) with a serialized
list (e.g., map each version through PromptVersionPublic using the version's
model dump or model_validate) so versions passed to PromptWithVersions are
PromptVersionPublic instances (or validated dicts) and then return
APIResponse.success_response with that sanitized PromptWithVersions object.
| def get_next_prompt_version(session: Session, prompt_id: int) -> int: | ||
| """ | ||
| fetch the next prompt version for a given prompt_id and project_id |
There was a problem hiding this comment.
Fix type annotation and docstring mismatch.
The function signature has prompt_id: int but it should be prompt_id: UUID to match actual usage. Also, the docstring mentions project_id which is not a parameter.
-def get_next_prompt_version(session: Session, prompt_id: int) -> int:
+def get_next_prompt_version(session: Session, prompt_id: UUID) -> int:
"""
- fetch the next prompt version for a given prompt_id and project_id
+ Fetch the next prompt version for a given prompt_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.
| def get_next_prompt_version(session: Session, prompt_id: int) -> int: | |
| """ | |
| fetch the next prompt version for a given prompt_id and project_id | |
| def get_next_prompt_version(session: Session, prompt_id: UUID) -> int: | |
| """ | |
| Fetch the next prompt version for a given prompt_id. | |
| """ |
🤖 Prompt for AI Agents
In backend/app/crud/prompt_versions.py around lines 14 to 16, the function
signature and docstring are inconsistent: change the type annotation of
prompt_id from int to UUID (ensure UUID is imported from uuid or typing as used
elsewhere) and update the docstring to accurately describe that it fetches the
next prompt version for a given prompt_id (remove any mention of project_id);
keep the rest of the logic unchanged.
| from app.models import Prompt, PromptCreate, PromptVersion, PromptVersionCreate | ||
| from app.tests.utils.utils import get_project | ||
| from app.tests.utils.test_data import create_test_prompt |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix parameter type inconsistency in get_next_prompt_version.
The function get_next_prompt_version expects prompt_id as type int but it's being passed as UUID throughout the codebase. The function signature should be updated to accept UUID for consistency.
🏁 Script executed:
#!/bin/bash
# Verify the type mismatch issue with get_next_prompt_version
# Check the function signature
echo "=== Function signature in crud/prompt_versions.py ==="
rg -A 2 "def get_next_prompt_version" backend/app/crud/prompt_versions.py
# Check all usages of get_next_prompt_version
echo -e "\n=== All calls to get_next_prompt_version ==="
rg "get_next_prompt_version\(" --type py -A 1Length of output: 1428
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Locate Prompt model files ==="
rg --hidden --no-ignore -n "class\s+Prompt\b|Prompt\s*=\s*BaseModel" || true
echo -e "\n=== Show Prompt model definition files ==="
rg --hidden --no-ignore -n "class\s+Prompt\b" -S || true
# Print surrounding context for the Prompt model file(s)
prompt_files=$(rg --hidden --no-ignore -l "class\s+Prompt\b" || true)
for f in $prompt_files; do
echo -e "\n--- $f ---"
sed -n '1,240p' "$f"
done
echo -e "\n=== Search for 'id:' annotations referencing UUID or int ==="
rg --hidden --no-ignore -n "id:\s*(UUID|int|uuid|str)" || true
echo -e "\n=== Show Prompt model import aliases (if id defined in Base) ==="
rg --hidden --no-ignore -n "from .* import .*Prompt" || true
echo -e "\n=== Show tests that create Prompt to inspect prompt.id usage ==="
rg --hidden --no-ignore -n "create_test_prompt|prompt = create_test_prompt|prompt.id" || true
rg --hidden --no-ignore -n "create_test_prompt" -S || trueLength of output: 35586
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== backend/app/crud/prompt_versions.py ==="
sed -n '1,240p' backend/app/crud/prompt_versions.py || true
echo -e "\n=== Occurrences of 'prompt_id:' in crud/prompt_versions.py ==="
rg -n "prompt_id" backend/app/crud/prompt_versions.py -n || trueLength of output: 4466
Fix parameter type in get_next_prompt_version to UUID
get_next_prompt_version is annotated with prompt_id: int but callers (and Prompt/PromptVersion models) use UUID. Update the signature to accept UUID.
- File: backend/app/crud/prompt_versions.py (around line 14)
- Change:
def get_next_prompt_version(session: Session, prompt_id: int) -> int:
to:
def get_next_prompt_version(session: Session, prompt_id: UUID) -> int:
Note: UUID is already imported at the top of the file, so no additional import changes are needed.
🧰 Tools
🪛 Ruff (0.12.2)
14-14: app.models.PromptCreate imported but unused
Remove unused import: app.models.PromptCreate
(F401)
🤖 Prompt for AI Agents
In backend/app/crud/prompt_versions.py around line 14, the
get_next_prompt_version signature uses prompt_id: int but the rest of the code
and models use UUID; update the function signature to accept a UUID and keep the
return type as int (i.e. change prompt_id: int to prompt_id: UUID) so callers
and models match; no import changes needed since UUID is already imported.
| create_prompt( | ||
| db, | ||
| PromptCreate( | ||
| name="prompt1", | ||
| description="First prompt", | ||
| instruction="Instruction 1", | ||
| commit_message="Initial", | ||
| ), | ||
| project_id=project.id, | ||
| ) | ||
| create_prompt( | ||
| db, | ||
| PromptCreate( | ||
| name="prompt2", | ||
| description="Second prompt", | ||
| instruction="Instruction 2", | ||
| commit_message="Initial", | ||
| ), | ||
| project_id=project.id, | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make ordering assertions deterministic; updated_at can tie.
Both prompts can share the same timestamp resolution, making ordering by updated_at non-deterministic. Force an update on one entity to guarantee order.
Apply this diff to create deterministic ordering:
- create_prompt(
+ prompt1, _ = create_prompt(
db,
PromptCreate(
name="prompt1",
description="First prompt",
instruction="Instruction 1",
commit_message="Initial",
),
project_id=project.id,
)
- create_prompt(
+ prompt2, _ = create_prompt(
db,
PromptCreate(
name="prompt2",
description="Second prompt",
instruction="Instruction 2",
commit_message="Initial",
),
project_id=project.id,
)
+
+ # Force prompt2 to be most recently updated
+ update_prompt(
+ db,
+ prompt_id=prompt2.id,
+ project_id=project.id,
+ prompt_update=PromptUpdate(description="Second prompt (updated)"),
+ )📝 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.
| create_prompt( | |
| db, | |
| PromptCreate( | |
| name="prompt1", | |
| description="First prompt", | |
| instruction="Instruction 1", | |
| commit_message="Initial", | |
| ), | |
| project_id=project.id, | |
| ) | |
| create_prompt( | |
| db, | |
| PromptCreate( | |
| name="prompt2", | |
| description="Second prompt", | |
| instruction="Instruction 2", | |
| commit_message="Initial", | |
| ), | |
| project_id=project.id, | |
| ) | |
| prompt1, _ = create_prompt( | |
| db, | |
| PromptCreate( | |
| name="prompt1", | |
| description="First prompt", | |
| instruction="Instruction 1", | |
| commit_message="Initial", | |
| ), | |
| project_id=project.id, | |
| ) | |
| prompt2, _ = create_prompt( | |
| db, | |
| PromptCreate( | |
| name="prompt2", | |
| description="Second prompt", | |
| instruction="Instruction 2", | |
| commit_message="Initial", | |
| ), | |
| project_id=project.id, | |
| ) | |
| # Force prompt2 to be most recently updated | |
| update_prompt( | |
| db, | |
| prompt_id=prompt2.id, | |
| project_id=project.id, | |
| prompt_update=PromptUpdate(description="Second prompt (updated)"), | |
| ) |
🤖 Prompt for AI Agents
In backend/app/tests/crud/test_prompts.py around lines 62 to 81, the test
creates two prompts but relies on updated_at ordering which can tie; force a
deterministic order by performing an explicit update on one of the created
prompts (e.g., call the prompt update helper or modify a field like
commit_message and save) after creation so its updated_at changes, then proceed
with the ordering assertions; ensure the update is persisted in the same
db/session so the timestamps differ and assertions become deterministic.
| project = get_project(db) # Call get_project as a function | ||
| result = prompt_exists(db, prompt_id=prompt.id, project_id=project.id) | ||
|
|
||
| assert isinstance(result, Prompt) | ||
| assert result.id == prompt.id | ||
| assert result.project_id == project.id | ||
| assert result.name == prompt.name |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Stabilize project scoping in the test to avoid flakiness.
Avoid fetching a (potentially different) active project; use the prompt’s own project_id.
Apply this diff:
- project = get_project(db) # Call get_project as a function
- result = prompt_exists(db, prompt_id=prompt.id, project_id=project.id)
+ project_id = prompt.project_id
+ result = prompt_exists(db, prompt_id=prompt.id, project_id=project_id)
@@
- assert result.project_id == project.id
+ assert result.project_id == project_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.
| project = get_project(db) # Call get_project as a function | |
| result = prompt_exists(db, prompt_id=prompt.id, project_id=project.id) | |
| assert isinstance(result, Prompt) | |
| assert result.id == prompt.id | |
| assert result.project_id == project.id | |
| assert result.name == prompt.name | |
| project_id = prompt.project_id | |
| result = prompt_exists(db, prompt_id=prompt.id, project_id=project_id) | |
| assert isinstance(result, Prompt) | |
| assert result.id == prompt.id | |
| assert result.project_id == project_id | |
| assert result.name == prompt.name |
🤖 Prompt for AI Agents
In backend/app/tests/crud/test_prompts.py around lines 181 to 187, the test
currently calls get_project(db) and uses that project's id which can cause
flakiness; instead use the prompt's own project_id directly. Replace the
get_project(db) call and any use of project.id with prompt.project_id, pass
prompt.project_id into prompt_exists, and update the assertions to compare
against prompt.project_id so the test consistently scopes to the prompt's
project.
| PromptCreate, | ||
| PromptVersion, | ||
| CredsCreate, | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Export actual persisted Prompt model in imports
This module returns persisted Prompt objects, not PromptCreate schemas. Import Prompt to reflect that in type annotations.
-from app.models import (
- Organization,
- Project,
- APIKey,
- Credential,
- OrganizationCreate,
- ProjectCreate,
- PromptCreate,
- PromptVersion,
- CredsCreate,
-)
+from app.models import (
+ Organization,
+ Project,
+ APIKey,
+ Credential,
+ OrganizationCreate,
+ ProjectCreate,
+ Prompt, # persisted model
+ PromptCreate, # input schema
+ PromptVersion,
+ CredsCreate,
+)📝 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.
| PromptCreate, | |
| PromptVersion, | |
| CredsCreate, | |
| ) | |
| from app.models import ( | |
| Organization, | |
| Project, | |
| APIKey, | |
| Credential, | |
| OrganizationCreate, | |
| ProjectCreate, | |
| Prompt, # persisted model | |
| PromptCreate, # input schema | |
| PromptVersion, | |
| CredsCreate, | |
| ) |
🤖 Prompt for AI Agents
In backend/app/tests/utils/test_data.py around lines 10 to 13, the imports
currently list PromptCreate and PromptVersion but the test utilities return
persisted Prompt model instances; update the import list to import the actual
Prompt ORM/model class (instead of or in addition to PromptCreate) and adjust
any type annotations to use Prompt for persisted objects so the types reflect
returned persisted Prompt instances.
| def create_test_prompt( | ||
| db: Session, | ||
| project_id: str, | ||
| name: str = "test_prompt", | ||
| description: str = "Test prompt description", | ||
| instruction: str = "Test instruction", | ||
| commit_message: str = "Initial version", | ||
| ) -> tuple[PromptCreate, PromptVersion]: | ||
| """Helper function to create a PromptCreate object and persist it in the database.""" | ||
| prompt_in = PromptCreate( | ||
| name=name, | ||
| description=description, | ||
| instruction=instruction, | ||
| commit_message=commit_message, | ||
| ) | ||
| return create_prompt(db, prompt_in=prompt_in, project_id=project_id) |
There was a problem hiding this comment.
Fix parameter/return types and docstring for create_test_prompt
- project_id should be int (CRUD expects int).
- Return type should be tuple[Prompt, PromptVersion] (persisted models).
- Docstring mentions returning PromptCreate; correct it.
-def create_test_prompt(
- db: Session,
- project_id: str,
- name: str = "test_prompt",
- description: str = "Test prompt description",
- instruction: str = "Test instruction",
- commit_message: str = "Initial version",
-) -> tuple[PromptCreate, PromptVersion]:
- """Helper function to create a PromptCreate object and persist it in the database."""
+def create_test_prompt(
+ db: Session,
+ project_id: int,
+ name: str = "test_prompt",
+ description: str = "Test prompt description",
+ instruction: str = "Test instruction",
+ commit_message: str = "Initial version",
+) -> tuple[Prompt, PromptVersion]:
+ """Create and persist a Prompt with its initial PromptVersion; returns persisted models."""📝 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 create_test_prompt( | |
| db: Session, | |
| project_id: str, | |
| name: str = "test_prompt", | |
| description: str = "Test prompt description", | |
| instruction: str = "Test instruction", | |
| commit_message: str = "Initial version", | |
| ) -> tuple[PromptCreate, PromptVersion]: | |
| """Helper function to create a PromptCreate object and persist it in the database.""" | |
| prompt_in = PromptCreate( | |
| name=name, | |
| description=description, | |
| instruction=instruction, | |
| commit_message=commit_message, | |
| ) | |
| return create_prompt(db, prompt_in=prompt_in, project_id=project_id) | |
| def create_test_prompt( | |
| db: Session, | |
| project_id: int, | |
| name: str = "test_prompt", | |
| description: str = "Test prompt description", | |
| instruction: str = "Test instruction", | |
| commit_message: str = "Initial version", | |
| ) -> tuple[Prompt, PromptVersion]: | |
| """Create and persist a Prompt with its initial PromptVersion; returns persisted models.""" | |
| prompt_in = PromptCreate( | |
| name=name, | |
| description=description, | |
| instruction=instruction, | |
| commit_message=commit_message, | |
| ) | |
| return create_prompt(db, prompt_in=prompt_in, project_id=project_id) |
🤖 Prompt for AI Agents
In backend/app/tests/utils/test_data.py around lines 120 to 135, update
create_test_prompt to reflect that project_id is an int and that the function
returns persisted models: change the signature type annotation from project_id:
str to project_id: int and change the return annotation from tuple[PromptCreate,
PromptVersion] to tuple[Prompt, PromptVersion]; update the docstring to state it
returns the persisted Prompt and PromptVersion models (not a PromptCreate), and
ensure any related imports/typing refer to Prompt (the ORM model) rather than
PromptCreate.
There was a problem hiding this comment.
Actionable comments posted: 9
🔭 Outside diff range comments (3)
backend/app/tests/api/routes/test_responses.py (1)
1-663: Don’t comment out tests; use pytest skip/xfail to avoid bit-rot and keep intent explicitThe entire module is commented out. This silently drops coverage and makes future re-enablement error-prone. If these tests are temporarily incompatible, mark the module as skipped (or xfail specific tests) with a clear reason instead of commenting out the code.
Apply this minimal, reversible change to skip the module while preserving the code for future re-enable:
+import pytest +pytest.skip("Responses API tests temporarily skipped pending router refactor; restore in follow-up PR", allow_module_level=True) + # from unittest.mock import MagicMock, patch # import pytest # from fastapi import FastAPI # from fastapi.testclient import TestClient # from sqlmodel import select # import openaiOptionally, open a tracking issue and reference it in the skip reason. If you want, I can generate a follow-up PR to gradually re-enable these with updated mocks.
backend/app/tests/utils/test_data.py (2)
3-13: Import Prompt for accurate type annotationsThe helper returns a Prompt instance (from create_prompt), but Prompt isn't imported, leading to an incorrect annotation downstream.
from app.models import ( Organization, Project, APIKey, Credential, OrganizationCreate, ProjectCreate, - PromptCreate, + Prompt, + PromptCreate, PromptVersion, CredsCreate, )
120-136: Fix type hints: project_id is int; return type should be (Prompt, PromptVersion); update docstring
- project_id should be int (create_prompt expects int, and tests pass project.id as int).
- Return type is (Prompt, PromptVersion), not (PromptCreate, PromptVersion).
- Docstring currently says it “create[s] a PromptCreate object”; clarify it creates and persists a Prompt and its first version.
-def create_test_prompt( - db: Session, - project_id: str, - name: str = "test_prompt", - description: str = "Test prompt description", - instruction: str = "Test instruction", - commit_message: str = "Initial version", -) -> tuple[PromptCreate, PromptVersion]: - """Helper function to create a PromptCreate object and persist it in the database.""" +def create_test_prompt( + db: Session, + project_id: int, + name: str = "test_prompt", + description: str = "Test prompt description", + instruction: str = "Test instruction", + commit_message: str = "Initial version", +) -> tuple[Prompt, PromptVersion]: + """Create and persist a Prompt (and its initial version) for tests, returning (Prompt, PromptVersion).""" prompt_in = PromptCreate( name=name, description=description, instruction=instruction, commit_message=commit_message, ) return create_prompt(db, prompt_in=prompt_in, project_id=project_id)
♻️ Duplicate comments (1)
backend/app/crud/__init__.py (1)
67-77: Add file-level Ruff suppression for re-export hub (duplicate of prior feedback)This module aggregates public CRUD symbols and will trip Ruff F401 for “imported but unused.” Add a one-liner at the top to keep lint green. This mirrors the earlier suggestion noted in past review comments.
Place this at the very top of the file:
+# ruff: noqa: F401 # export aggregator: re-exports used externally from .user import (
🧹 Nitpick comments (12)
backend/app/tests/crud/test_prompt_versions.py (1)
13-16: Remove unused imports to satisfy Ruff F401 and keep tests tidy
- create_prompt is not used (you use create_test_prompt).
- PromptCreate is not used.
-from app.crud.prompts import create_prompt -from app.models import Prompt, PromptCreate, PromptVersion, PromptVersionCreate +from app.models import Prompt, PromptVersion, PromptVersionCreatebackend/app/tests/api/routes/test_prompt_versions.py (2)
4-5: Remove unused imports flagged by Ruff (F401).
create_promptandPromptCreateare not used in this test module. Clean them up to satisfy Ruff and keep the test lean.Apply this diff:
-from app.crud.prompts import create_prompt -from app.models import APIKeyPublic, PromptCreate, PromptVersion, PromptVersionCreate +from app.models import APIKeyPublic, PromptVersion, PromptVersionCreate
49-58: Prefer creating the second version via the public API or a helper to keep tests E2E.Directly inserting
PromptVersionbypasses validation and business rules. Consider using the POST route (or a helper wrapping it) to create the non-active version for deletion. This keeps the test end-to-end and closer to real usage.backend/app/api/routes/prompt_versions.py (1)
22-35: Add a short docstring for the create route.For parity with the delete route and better auto-docs, add a one-liner describing the operation and payload.
backend/app/tests/crud/test_prompts.py (1)
85-87: Potential ordering flakiness in assertions.Ordering by
updated_atcan be non-deterministic if timestamps match at microsecond precision. If you hit flakes, either:
- sort the result in-test by
updated_atbefore asserting order, or- assert the set of names and avoid strict ordering checks.
No change required if your test DB guarantees distinct timestamps.
backend/app/crud/prompts.py (5)
13-15: Remove unused imports flagged by Ruff (F401).PromptWithVersion and PromptWithVersions are not used in this module.
from app.models import ( Prompt, PromptCreate, PromptUpdate, PromptVersion, - PromptWithVersion, - PromptWithVersions, )
93-96: Fix incorrect log context in not-found branch.The log label says [update_prompt] while this is inside prompt_exists, which can be misleading in logs.
- logger.error( - f"[update_prompt] Prompt not found | prompt_id={prompt_id}, project_id={project_id}" - ) + logger.error( + f"[prompt_exists] Prompt not found | prompt_id={prompt_id}, project_id={project_id}" + )
104-107: Minor docstring capitalization.Small grammar nit for clarity.
- By default, Always returns the active version. + By default, always returns the active version.
150-171: Avoid shadowing function name with local variable.
update_prompt = prompt_update.model_dump(...)shadows the function name, which can confuse stack traces and readers.- update_prompt = prompt_update.model_dump(exclude_unset=True) + updates = prompt_update.model_dump(exclude_unset=True) - active_version = update_prompt.get("active_version") + active_version = updates.get("active_version") if active_version: stmt = select(PromptVersion).where( PromptVersion.id == active_version, PromptVersion.prompt_id == prompt.id, PromptVersion.is_deleted.is_(False), ) prompt_version = session.exec(stmt).first() if not prompt_version: logger.error( f"[update_prompt] Prompt version not found | version_id={active_version}, prompt_id={prompt.id}" ) raise HTTPException(status_code=404, detail="Invalid Active Version Id") - if update_prompt: - for field, value in update_prompt.items(): + if updates: + for field, value in updates.items(): setattr(prompt, field, value)
26-49: Create prompt and version atomically without interim flush and placeholder FK.Since both Prompt.id and PromptVersion.id are UUIDs generated client-side, you can pre-generate the version id, set active_version upfront, and add both objects before committing. This removes the need for intermediate flushes and avoids a transient invalid FK value.
- prompt = Prompt( - name=prompt_in.name, description=prompt_in.description, project_id=project_id - ) - session.add(prompt) - session.flush() - - version = PromptVersion( - prompt_id=prompt.id, - instruction=prompt_in.instruction, - commit_message=prompt_in.commit_message, - version=1, - ) - session.add(version) - session.flush() - - prompt.active_version = version.id - - session.commit() + # Pre-generate version id to set active_version upfront + version_id = uuid4() + prompt = Prompt( + name=prompt_in.name, + description=prompt_in.description, + project_id=project_id, + active_version=version_id, + ) + version = PromptVersion( + id=version_id, + prompt_id=prompt.id, + instruction=prompt_in.instruction, + commit_message=prompt_in.commit_message, + version=1, + ) + session.add(prompt) + session.add(version) + session.commit() session.refresh(prompt)Also add the missing import:
-from uuid import UUID +from uuid import UUID, uuid4backend/app/api/routes/prompts.py (1)
107-109: Docstring: mention active_version can be updated.API supports updating active_version; reflect that in the docstring.
- Update a prompt's name or description. + Update a prompt's name, description, or active_version.backend/app/models/prompt.py (1)
29-40: Optional: remove default_factory for active_version; set it explicitly in create flow.Currently active_version gets a random UUID by default which is later overwritten, relying on a deferred FK. You can avoid the placeholder value by removing the default and setting active_version explicitly in create_prompt (with a pre-generated version_id). This reduces transient inconsistencies and removes an extra flush.
- active_version: UUID = Field( - default_factory=uuid4, - sa_column=Column( + active_version: UUID = Field( + sa_column=Column( ForeignKey( "prompt_version.id", use_alter=True, deferrable=True, initially="DEFERRED", ), nullable=False, ), )Note: This change pairs with the suggested
create_promptrefactor that pre-generatesversion_idand passes it into both Prompt (asactive_version) and PromptVersion (asid).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
backend/app/alembic/versions/757f50ada8ef_add_prompt_and_version_table.py(1 hunks)backend/app/api/main.py(2 hunks)backend/app/api/routes/prompt_versions.py(1 hunks)backend/app/api/routes/prompts.py(1 hunks)backend/app/crud/__init__.py(1 hunks)backend/app/crud/prompt_versions.py(1 hunks)backend/app/crud/prompts.py(1 hunks)backend/app/models/__init__.py(1 hunks)backend/app/models/prompt.py(1 hunks)backend/app/models/prompt_version.py(1 hunks)backend/app/tests/api/routes/test_prompt_versions.py(1 hunks)backend/app/tests/api/routes/test_prompts.py(1 hunks)backend/app/tests/api/routes/test_responses.py(1 hunks)backend/app/tests/crud/test_prompt_versions.py(1 hunks)backend/app/tests/crud/test_prompts.py(1 hunks)backend/app/tests/utils/test_data.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/app/api/main.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-06T05:33:19.776Z
Learnt from: avirajsingh7
PR: ProjectTech4DevAI/ai-platform#319
File: backend/app/crud/prompt_version.py:134-146
Timestamp: 2025-08-06T05:33:19.776Z
Learning: In the prompt management system, `get_prompt_versions_count` is used only for pagination after `get_prompt_versions` which already validates the prompt exists, so duplicate validation is avoided for performance reasons.
Applied to files:
backend/app/crud/prompt_versions.pybackend/app/api/routes/prompt_versions.py
🧬 Code Graph Analysis (14)
backend/app/tests/crud/test_prompt_versions.py (8)
backend/app/crud/prompt_versions.py (3)
create_prompt_version(31-59)delete_prompt_version(62-103)get_next_prompt_version(14-28)backend/app/crud/prompts.py (1)
create_prompt(20-51)backend/app/models/prompt.py (2)
Prompt(20-50)PromptCreate(69-70)backend/app/models/prompt_version.py (2)
PromptVersion(18-38)PromptVersionCreate(49-50)backend/app/tests/utils/utils.py (1)
get_project(70-89)backend/app/tests/utils/test_data.py (1)
create_test_prompt(120-135)backend/app/tests/crud/test_prompts.py (1)
prompt(23-27)backend/app/tests/conftest.py (1)
db(18-35)
backend/app/crud/prompts.py (3)
backend/app/models/prompt.py (4)
Prompt(20-50)PromptCreate(69-70)PromptWithVersion(61-62)PromptWithVersions(65-66)backend/app/models/prompt_version.py (1)
PromptVersion(18-38)backend/app/crud/project.py (1)
create_project(13-23)
backend/app/crud/prompt_versions.py (2)
backend/app/crud/prompts.py (1)
prompt_exists(81-98)backend/app/models/prompt_version.py (2)
PromptVersion(18-38)PromptVersionCreate(49-50)
backend/app/tests/utils/test_data.py (3)
backend/app/models/prompt.py (1)
PromptCreate(69-70)backend/app/models/prompt_version.py (1)
PromptVersion(18-38)backend/app/crud/prompts.py (1)
create_prompt(20-51)
backend/app/tests/crud/test_prompts.py (5)
backend/app/crud/prompts.py (7)
count_prompts_in_project(71-78)create_prompt(20-51)delete_prompt(181-191)get_prompt_by_id(101-143)get_prompts(54-68)prompt_exists(81-98)update_prompt(146-178)backend/app/models/prompt.py (3)
Prompt(20-50)PromptCreate(69-70)PromptUpdate(73-79)backend/app/models/prompt_version.py (1)
PromptVersion(18-38)backend/app/tests/utils/utils.py (1)
get_project(70-89)backend/app/tests/utils/test_data.py (1)
create_test_prompt(120-135)
backend/app/tests/api/routes/test_prompt_versions.py (4)
backend/app/crud/prompts.py (1)
create_prompt(20-51)backend/app/models/prompt.py (1)
PromptCreate(69-70)backend/app/models/prompt_version.py (2)
PromptVersion(18-38)PromptVersionCreate(49-50)backend/app/tests/utils/test_data.py (1)
create_test_prompt(120-135)
backend/app/alembic/versions/757f50ada8ef_add_prompt_and_version_table.py (8)
backend/app/alembic/versions/e9dd35eff62c_add_openai_conversation_table.py (1)
upgrade(20-70)backend/app/alembic/versions/0f205e3779ee_add_api_key_table.py (1)
upgrade(20-39)backend/app/alembic/versions/c43313eca57d_add_document_tables.py (1)
upgrade(20-36)backend/app/alembic/versions/904ed70e7dab_added_provider_column_to_the_credential_.py (1)
upgrade(19-61)backend/app/alembic/versions/99f4fc325617_add_organization_project_setup.py (1)
upgrade(20-72)backend/app/alembic/versions/e2412789c190_initialize_models.py (1)
upgrade(19-46)backend/app/alembic/versions/8e7dc5eab0b0_add_fk_constraint_to_user_id_columns.py (1)
upgrade(20-42)backend/app/alembic/versions/8eefcfedc409_create_assistant_table.py (1)
upgrade(20-40)
backend/app/models/__init__.py (2)
backend/app/models/prompt.py (6)
Prompt(20-50)PromptCreate(69-70)PromptPublic(53-58)PromptUpdate(73-79)PromptWithVersion(61-62)PromptWithVersions(65-66)backend/app/models/prompt_version.py (3)
PromptVersion(18-38)PromptVersionCreate(49-50)PromptVersionPublic(41-46)
backend/app/models/prompt_version.py (1)
backend/app/models/prompt.py (1)
Prompt(20-50)
backend/app/api/routes/prompt_versions.py (5)
backend/app/api/deps.py (1)
get_db(33-35)backend/app/crud/prompt_versions.py (2)
create_prompt_version(31-59)delete_prompt_version(62-103)backend/app/models/prompt_version.py (2)
PromptVersionCreate(49-50)PromptVersionPublic(41-46)backend/app/utils.py (2)
APIResponse(27-48)success_response(34-37)backend/app/api/routes/assistants.py (1)
update_assistant_route(74-93)
backend/app/tests/api/routes/test_prompts.py (6)
backend/app/models/api_key.py (1)
APIKeyPublic(23-25)backend/app/models/prompt.py (2)
PromptCreate(69-70)PromptUpdate(73-79)backend/app/models/prompt_version.py (1)
PromptVersion(18-38)backend/app/tests/utils/test_data.py (1)
create_test_prompt(120-135)backend/app/tests/crud/test_prompt_versions.py (1)
prompt(20-24)backend/app/tests/crud/test_prompts.py (1)
prompt(23-27)
backend/app/models/prompt.py (1)
backend/app/models/prompt_version.py (3)
PromptVersion(18-38)PromptVersionCreate(49-50)PromptVersionPublic(41-46)
backend/app/api/routes/prompts.py (4)
backend/app/api/deps.py (1)
get_db(33-35)backend/app/crud/prompts.py (6)
create_prompt(20-51)delete_prompt(181-191)get_prompt_by_id(101-143)get_prompts(54-68)count_prompts_in_project(71-78)update_prompt(146-178)backend/app/models/prompt.py (5)
PromptCreate(69-70)PromptPublic(53-58)PromptUpdate(73-79)PromptWithVersion(61-62)PromptWithVersions(65-66)backend/app/utils.py (2)
APIResponse(27-48)success_response(34-37)
backend/app/crud/__init__.py (2)
backend/app/crud/prompt_versions.py (2)
create_prompt_version(31-59)delete_prompt_version(62-103)backend/app/crud/prompts.py (6)
create_prompt(20-51)count_prompts_in_project(71-78)delete_prompt(181-191)get_prompt_by_id(101-143)get_prompts(54-68)update_prompt(146-178)
🪛 Ruff (0.12.2)
backend/app/tests/crud/test_prompt_versions.py
13-13: app.crud.prompts.create_prompt imported but unused
Remove unused import: app.crud.prompts.create_prompt
(F401)
14-14: app.models.PromptCreate imported but unused
Remove unused import: app.models.PromptCreate
(F401)
backend/app/crud/prompts.py
13-13: app.models.PromptWithVersion imported but unused
Remove unused import
(F401)
14-14: app.models.PromptWithVersions imported but unused
Remove unused import
(F401)
75-75: Avoid equality comparisons to False; use not Prompt.is_deleted: for false checks
Replace with not Prompt.is_deleted
(E712)
backend/app/tests/api/routes/test_prompt_versions.py
4-4: app.crud.prompts.create_prompt imported but unused
Remove unused import: app.crud.prompts.create_prompt
(F401)
5-5: app.models.PromptCreate imported but unused
Remove unused import: app.models.PromptCreate
(F401)
backend/app/models/__init__.py
67-67: .prompt.Prompt imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
68-68: .prompt.PromptCreate imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
69-69: .prompt.PromptPublic imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
70-70: .prompt.PromptUpdate imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
71-71: .prompt.PromptWithVersion imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
72-72: .prompt.PromptWithVersions imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
76-76: .prompt_version.PromptVersion imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
77-77: .prompt_version.PromptVersionCreate imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
78-78: .prompt_version.PromptVersionPublic imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
backend/app/tests/api/routes/test_prompts.py
12-12: Unused function argument: db
(ARG001)
61-61: f-string without any placeholders
Remove extraneous f prefix
(F541)
83-83: Unused function argument: db
(ARG001)
88-88: f-string without any placeholders
Remove extraneous f prefix
(F541)
113-113: f-string without any placeholders
Remove extraneous f prefix
(F541)
196-196: Unused function argument: db
(ARG001)
backend/app/crud/__init__.py
68-68: .prompt_versions.create_prompt_version imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
68-68: .prompt_versions.delete_prompt_version imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
71-71: .prompts.create_prompt imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
72-72: .prompts.count_prompts_in_project imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
73-73: .prompts.delete_prompt imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
74-74: .prompts.get_prompt_by_id imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
75-75: .prompts.get_prompts imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
76-76: .prompts.update_prompt imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
🔇 Additional comments (5)
backend/app/api/routes/prompt_versions.py (1)
17-35: Create route looks correct and consistent (201 + typed APIResponse).Good use of
APIResponse[PromptVersionPublic], project-scoped dependency, and returning 201 on creation.backend/app/crud/prompt_versions.py (1)
73-78: Correctly prevents deletion of active version (409).Good guard and error signaling for business rule enforcement.
backend/app/alembic/versions/757f50ada8ef_add_prompt_and_version_table.py (1)
80-89: LGTM on deferrable FK added post table creation.Adding the FK via
op.create_foreign_keywithdeferrable=Trueandinitially="DEFERRED"is the correct approach given the creation flow.backend/app/models/prompt_version.py (1)
9-11: LGTM: TYPE_CHECKING forward reference prevents runtime import cycles.The TYPE_CHECKING import of Prompt cleanly resolves the relationship typing without circular import issues. Constraints and indexes look correct.
backend/app/tests/api/routes/test_prompts.py (1)
1-278: Skipping tests per PR instructions.Per the PR review plan, deferring test review to a later pass. No actionable comments here.
| sa.ForeignKeyConstraint( | ||
| ["active_version"], | ||
| ["prompt_version.id"], | ||
| initially="DEFERRED", | ||
| deferrable=True, | ||
| use_alter=True, | ||
| ), |
There was a problem hiding this comment.
Duplicate foreign key definition for prompt.active_version.
The FK from prompt.active_version to prompt_version.id is defined both inline in op.create_table("prompt", ...) and again via op.create_foreign_key(...) (Lines 80–89). This can lead to migration failures due to duplicate constraints.
Keep the explicit op.create_foreign_key(...) after both tables exist, and remove the inline FK from the create_table call.
Apply this diff:
- sa.ForeignKeyConstraint(
- ["active_version"],
- ["prompt_version.id"],
- initially="DEFERRED",
- deferrable=True,
- use_alter=True,
- ),📝 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.
| sa.ForeignKeyConstraint( | |
| ["active_version"], | |
| ["prompt_version.id"], | |
| initially="DEFERRED", | |
| deferrable=True, | |
| use_alter=True, | |
| ), |
🤖 Prompt for AI Agents
In backend/app/alembic/versions/757f50ada8ef_add_prompt_and_version_table.py
around lines 35 to 41, the foreign key from prompt.active_version to
prompt_version.id is declared inline inside op.create_table (the
sa.ForeignKeyConstraint block) and is duplicated later via
op.create_foreign_key; remove the inline sa.ForeignKeyConstraint lines (the
entire block at 35–41) from the create_table call so the table defines
active_version as a plain Column/Integer, and keep the existing
op.create_foreign_key(...) that runs after both tables exist to create the FK
constraint.
| from fastapi import HTTPException | ||
| from sqlmodel import Session, and_, select | ||
|
|
||
| from app.core.util import now | ||
| from app.crud.prompts import prompt_exists | ||
| from app.models import PromptVersion, PromptVersionCreate | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Prepare to handle concurrent version creation (import IntegrityError).
We’ll need this to gracefully handle unique-constraint conflicts.
-from fastapi import HTTPException
+from fastapi import HTTPException
+from sqlalchemy.exc import IntegrityError🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "PWD: $(pwd)"
echo "---- Find prompt_versions.py ----"
file=$(rg --files | rg 'backend/.*/prompt_versions.py' || true)
if [ -n "$file" ]; then
echo "Found: $file"
sed -n '1,240p' "$file"
else
echo "prompt_versions.py not found under backend/*"
fi
echo
echo "---- Search for PromptVersion model definition ----"
rg -n "class\s+PromptVersion" || true
rg -n "PromptVersion\s*=" || true
echo
echo "---- Search for PromptVersionCreate usages ----"
rg -n "PromptVersionCreate" || true
echo
echo "---- Search for IntegrityError usage / exception handling ----"
rg -n "IntegrityError" || true
rg -n "except .*IntegrityError" || true
echo
echo "---- Search for unique constraints / unique=True / UniqueConstraint in models ----"
rg -n "UniqueConstraint|unique=True|__table_args__|Index\(|index=True" backend/app || trueLength of output: 788
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "PWD: $(pwd)"
echo
echo "---- List files named prompt_versions.py ----"
rg --files | rg 'prompt_versions.py' || true
echo
echo "---- Show contents of each found prompt_versions.py (up to 500 lines) ----"
rg --files | rg 'prompt_versions.py' || true | while IFS= read -r f; do
echo
echo "----- $f -----"
sed -n '1,500p' "$f" || echo "failed to read $f"
done
echo
echo "---- Search for PromptVersion model/class definition ----"
rg -n --hidden "class\s+PromptVersion" || true
rg -n --hidden "PromptVersionCreate" || true
echo
echo "---- Find all references to PromptVersion/PromptVersionCreate ----"
rg -n "PromptVersion" || true
rg -n "PromptVersionCreate" || true
echo
echo "---- Search for IntegrityError usage / exception handling ----"
rg -n "IntegrityError" || true
rg -n "except .*IntegrityError" || true
echo
echo "---- Search for unique constraints / unique=True / UniqueConstraint / Index(...) in backend/app ----"
rg -n "UniqueConstraint|unique=True|__table_args__|Index\\(|index=True" backend/app || trueLength of output: 13697
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "----- backend/app/crud/prompt_versions.py -----"
sed -n '1,240p' backend/app/crud/prompt_versions.py || true
echo
echo "----- backend/app/models/prompt_version.py -----"
sed -n '1,240p' backend/app/models/prompt_version.py || true
echo
echo "----- backend/app/crud/prompts.py (prompt_exists) -----"
sed -n '1,240p' backend/app/crud/prompts.py || true
echo
echo "----- backend/app/tests/crud/test_prompt_versions.py -----"
sed -n '1,240p' backend/app/tests/crud/test_prompt_versions.py || trueLength of output: 18551
Handle concurrent version creation — catch IntegrityError and fail gracefully
PromptVersion defines a UniqueConstraint("prompt_id", "version"), and create_prompt_version computes the next version in two steps (read + insert). Under concurrent requests this can raise an IntegrityError on commit — we should catch it, rollback, and return a 409 (or retry).
Files to update:
- backend/app/crud/prompt_versions.py
- add import for IntegrityError
- wrap commit in try/except to rollback and return a 409 on unique-constraint conflict
Suggested minimal diff:
-from fastapi import HTTPException
+from fastapi import HTTPException
+from sqlalchemy.exc import IntegrityErrorAnd in create_prompt_version, replace the commit/refresh block with:
- session.add(prompt_version)
- session.commit()
- session.refresh(prompt_version)
+ session.add(prompt_version)
+ try:
+ session.commit()
+ except IntegrityError:
+ session.rollback()
+ logger.error(
+ f"[create_prompt_version] Concurrent version conflict | prompt_id={prompt_id}, attempted_version={next_version}"
+ )
+ raise HTTPException(status_code=409, detail="Prompt version conflict (concurrent creation)")
+ session.refresh(prompt_version)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In backend/app/crud/prompt_versions.py around lines 4 to 10, concurrent creation
can raise an IntegrityError because PromptVersion has a UniqueConstraint on
(prompt_id, version); add an import for IntegrityError from sqlalchemy.exc, then
wrap the session.commit()/session.refresh(instance) block in a try/except that
catches IntegrityError, calls session.rollback(), and raises
HTTPException(status_code=409, detail="Prompt version conflict") (or similar),
so the API fails gracefully on unique-constraint conflicts instead of crashing.
| def get_next_prompt_version(session: Session, prompt_id: int) -> int: | ||
| """ | ||
| fetch the next prompt version for a given prompt_id and project_id | ||
| """ | ||
|
|
||
| # Not filtering is_deleted here because we want to get the next version even if the latest version is deleted |
There was a problem hiding this comment.
Fix type annotation and docstring for get_next_prompt_version.
prompt_idis a UUID, not an int.- Docstring mentions
project_idwhich isn’t a parameter.
Apply this diff:
-def get_next_prompt_version(session: Session, prompt_id: int) -> int:
- """
- fetch the next prompt version for a given prompt_id and project_id
- """
+def get_next_prompt_version(session: Session, prompt_id: UUID) -> int:
+ """
+ Compute the next version number for a given prompt_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.
| def get_next_prompt_version(session: Session, prompt_id: int) -> int: | |
| """ | |
| fetch the next prompt version for a given prompt_id and project_id | |
| """ | |
| # Not filtering is_deleted here because we want to get the next version even if the latest version is deleted | |
| def get_next_prompt_version(session: Session, prompt_id: UUID) -> int: | |
| """ | |
| Compute the next version number for a given prompt_id. | |
| """ | |
| # Not filtering is_deleted here because we want to get the next version even if the latest version is deleted |
🤖 Prompt for AI Agents
In backend/app/crud/prompt_versions.py around lines 14 to 19, the function
signature and docstring are incorrect: change the type annotation of prompt_id
from int to UUID (from uuid import UUID) and update the docstring to remove the
incorrect reference to project_id and instead document that prompt_id is a UUID
and the function returns the next prompt version for that prompt; ensure any
necessary imports for UUID are added at the top of the file.
| next_version = get_next_prompt_version(session=session, prompt_id=prompt_id) | ||
| prompt_version = PromptVersion( | ||
| prompt_id=prompt_id, | ||
| version=next_version, | ||
| instruction=prompt_version_in.instruction, | ||
| commit_message=prompt_version_in.commit_message, | ||
| ) | ||
|
|
||
| session.add(prompt_version) | ||
| session.commit() | ||
| session.refresh(prompt_version) | ||
| logger.info( | ||
| f"[create_prompt_version] Created new version prompt_version | Prompt ID: {prompt_id}, Version: {prompt_version.version}" | ||
| ) | ||
| return prompt_version |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle race conditions when creating versions (unique constraint).
Two concurrent calls can compute the same next version, violating the (prompt_id, version) unique constraint and causing a 500. Retry on conflict and surface a 409 if contention persists.
Apply this diff:
- next_version = get_next_prompt_version(session=session, prompt_id=prompt_id)
- prompt_version = PromptVersion(
- prompt_id=prompt_id,
- version=next_version,
- instruction=prompt_version_in.instruction,
- commit_message=prompt_version_in.commit_message,
- )
-
- session.add(prompt_version)
- session.commit()
- session.refresh(prompt_version)
+ # Retry on unique constraint conflicts due to concurrent creations
+ max_attempts = 3
+ attempt = 0
+ while True:
+ attempt += 1
+ next_version = get_next_prompt_version(session=session, prompt_id=prompt_id)
+ prompt_version = PromptVersion(
+ prompt_id=prompt_id,
+ version=next_version,
+ instruction=prompt_version_in.instruction,
+ commit_message=prompt_version_in.commit_message,
+ )
+ session.add(prompt_version)
+ try:
+ session.commit()
+ session.refresh(prompt_version)
+ break
+ except IntegrityError:
+ session.rollback()
+ if attempt >= max_attempts:
+ logger.error(
+ f"[create_prompt_version] Version conflict after {attempt} attempts | prompt_id={prompt_id}"
+ )
+ raise HTTPException(
+ status_code=409, detail="Version conflict, please retry"
+ )
+ # Recompute and retry📝 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.
| next_version = get_next_prompt_version(session=session, prompt_id=prompt_id) | |
| prompt_version = PromptVersion( | |
| prompt_id=prompt_id, | |
| version=next_version, | |
| instruction=prompt_version_in.instruction, | |
| commit_message=prompt_version_in.commit_message, | |
| ) | |
| session.add(prompt_version) | |
| session.commit() | |
| session.refresh(prompt_version) | |
| logger.info( | |
| f"[create_prompt_version] Created new version prompt_version | Prompt ID: {prompt_id}, Version: {prompt_version.version}" | |
| ) | |
| return prompt_version | |
| # Retry on unique constraint conflicts due to concurrent creations | |
| max_attempts = 3 | |
| attempt = 0 | |
| while True: | |
| attempt += 1 | |
| next_version = get_next_prompt_version(session=session, prompt_id=prompt_id) | |
| prompt_version = PromptVersion( | |
| prompt_id=prompt_id, | |
| version=next_version, | |
| instruction=prompt_version_in.instruction, | |
| commit_message=prompt_version_in.commit_message, | |
| ) | |
| session.add(prompt_version) | |
| try: | |
| session.commit() | |
| session.refresh(prompt_version) | |
| break | |
| except IntegrityError: | |
| session.rollback() | |
| if attempt >= max_attempts: | |
| logger.error( | |
| f"[create_prompt_version] Version conflict after {attempt} attempts | prompt_id={prompt_id}" | |
| ) | |
| raise HTTPException( | |
| status_code=409, detail="Version conflict, please retry" | |
| ) | |
| # Recompute and retry | |
| logger.info( | |
| f"[create_prompt_version] Created new version prompt_version | Prompt ID: {prompt_id}, Version: {prompt_version.version}" | |
| ) | |
| return prompt_version |
|
|
||
| def count_prompts_in_project(session: Session, project_id: int) -> int: | ||
| return session.exec( | ||
| select(func.count()).select_from( | ||
| select(Prompt) | ||
| .where(Prompt.project_id == project_id, Prompt.is_deleted == False) | ||
| .subquery() | ||
| ) | ||
| ).one() |
There was a problem hiding this comment.
Fix count to return int and avoid E712; use scalar_one() and .is_(False).
Current code likely returns a Row/tuple and uses == False. Return a scalar int and use .is_(False) for SQLAlchemy.
-def count_prompts_in_project(session: Session, project_id: int) -> int:
- return session.exec(
- select(func.count()).select_from(
- select(Prompt)
- .where(Prompt.project_id == project_id, Prompt.is_deleted == False)
- .subquery()
- )
- ).one()
+def count_prompts_in_project(session: Session, project_id: int) -> int:
+ stmt = select(func.count()).where(
+ Prompt.project_id == project_id, Prompt.is_deleted.is_(False)
+ )
+ return session.exec(stmt).scalar_one()📝 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 count_prompts_in_project(session: Session, project_id: int) -> int: | |
| return session.exec( | |
| select(func.count()).select_from( | |
| select(Prompt) | |
| .where(Prompt.project_id == project_id, Prompt.is_deleted == False) | |
| .subquery() | |
| ) | |
| ).one() | |
| def count_prompts_in_project(session: Session, project_id: int) -> int: | |
| stmt = select(func.count()).where( | |
| Prompt.project_id == project_id, Prompt.is_deleted.is_(False) | |
| ) | |
| return session.exec(stmt).scalar_one() |
🧰 Tools
🪛 Ruff (0.12.2)
75-75: Avoid equality comparisons to False; use not Prompt.is_deleted: for false checks
Replace with not Prompt.is_deleted
(E712)
🤖 Prompt for AI Agents
In backend/app/crud/prompts.py around lines 70 to 78, the current count query
returns a Row/tuple and uses `== False` which triggers E712; change the
predicate to use `.is_(False)` and call `.scalar_one()` on the query result to
return an int (or wrap the result with int(...) if needed). Update the select so
the final call uses scalar_one() to produce a scalar integer count and replace
`Prompt.is_deleted == False` with `Prompt.is_deleted.is_(False)`.
| from .prompt import ( | ||
| Prompt, | ||
| PromptCreate, | ||
| PromptPublic, | ||
| PromptUpdate, | ||
| PromptWithVersion, | ||
| PromptWithVersions, | ||
| ) | ||
|
|
||
| from .prompt_version import ( | ||
| PromptVersion, | ||
| PromptVersionCreate, | ||
| PromptVersionPublic, | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Silence aggregator F401 warnings for new re-exports to keep CI green
This file intentionally re-exports public model symbols. Ruff flags these as “imported but unused” (F401). Add a file-level directive or define all to prevent lint failures.
Minimal fix (preferred):
+# ruff: noqa: F401 # this module re-exports public model symbols
from sqlmodel import SQLModelAlternative: declare an explicit all with the exported names. This is more verbose but documents the public surface.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.12.2)
67-67: .prompt.Prompt imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
68-68: .prompt.PromptCreate imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
69-69: .prompt.PromptPublic imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
70-70: .prompt.PromptUpdate imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
71-71: .prompt.PromptWithVersion imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
72-72: .prompt.PromptWithVersions imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
76-76: .prompt_version.PromptVersion imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
77-77: .prompt_version.PromptVersionCreate imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
78-78: .prompt_version.PromptVersionPublic imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
🤖 Prompt for AI Agents
In backend/app/models/__init__.py around lines 66 to 79, the re-exported model
symbols are being flagged by Ruff as F401 ("imported but unused"); silence that
by either adding a file-level directive (e.g., put a "# noqa: F401" on the
import block or at top of the file) or by explicitly declaring __all__ =
["Prompt","PromptCreate","PromptPublic","PromptUpdate","PromptWithVersion","PromptWithVersions","PromptVersion","PromptVersionCreate","PromptVersionPublic"]
so the linter recognizes these as the module's public API; update the file
accordingly and commit the change.
| project = get_project(db) # Call get_project as a function | ||
| result = prompt_exists(db, prompt_id=prompt.id, project_id=project.id) | ||
|
|
||
| assert isinstance(result, Prompt) | ||
| assert result.id == prompt.id | ||
| assert result.project_id == project.id |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Avoid re-fetching a random project; use the prompt’s project_id to prevent test flakiness.
get_project(db) may return a different active project than the one used to create the prompt fixture, causing intermittent failures. Use prompt.project_id.
Apply this diff:
- project = get_project(db) # Call get_project as a function
- result = prompt_exists(db, prompt_id=prompt.id, project_id=project.id)
+ result = prompt_exists(db, prompt_id=prompt.id, project_id=prompt.project_id)🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "----- Showing backend/app/tests/crud/test_prompts.py (lines 1-260) -----"
sed -n '1,260p' backend/app/tests/crud/test_prompts.py || true
echo
echo "----- Searching for occurrences of get_project -----"
rg --hidden -n "get_project" || true
echo
echo "----- Searching for definition(s) of prompt_exists -----"
rg --hidden -n "def prompt_exists" || true
echo
echo "----- Searching for usages of prompt.project_id in repo -----"
rg --hidden -n "project_id" || trueLength of output: 70216
Use prompt.project_id in tests that use the prompt fixture to avoid flakiness
get_project(db) can return a different active project than the one the prompt fixture created, causing intermittent failures. Use prompt.project_id instead.
- Files/locations to update:
- backend/app/tests/crud/test_prompts.py — test_prompt_exists_success (lines ~181–186)
- Also review similar tests in the same file that call get_project(db) while using the prompt fixture (e.g., the get_prompt_by_id tests around lines ~220–255).
Apply this diff:
- project = get_project(db) # Call get_project as a function
- result = prompt_exists(db, prompt_id=prompt.id, project_id=project.id)
+ result = prompt_exists(db, prompt_id=prompt.id, project_id=prompt.project_id)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In backend/app/tests/crud/test_prompts.py around lines 181 to 186, the test uses
get_project(db) which can return a different active project and cause flakiness;
replace project = get_project(db) and uses of project.id with the prompt
fixture's project id (prompt.project_id) so the test asserts against the
prompt's actual project; also scan the same file for other tests that call
get_project(db) while using the prompt fixture (notably the get_prompt_by_id
tests around lines ~220–255) and update them to use prompt.project_id to ensure
stable, deterministic assertions.
| """Retrieve a prompt by ID with only its active version""" | ||
| project = get_project(db) | ||
| retrieved_prompt, versions = get_prompt_by_id( | ||
| db, prompt_id=prompt.id, project_id=project.id, include_versions=False | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use prompt.project_id here as well to avoid project mismatch.
Apply this diff:
- project = get_project(db)
- retrieved_prompt, versions = get_prompt_by_id(
- db, prompt_id=prompt.id, project_id=project.id, include_versions=False
- )
+ retrieved_prompt, versions = get_prompt_by_id(
+ db, prompt_id=prompt.id, project_id=prompt.project_id, include_versions=False
+ )📝 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.
| """Retrieve a prompt by ID with only its active version""" | |
| project = get_project(db) | |
| retrieved_prompt, versions = get_prompt_by_id( | |
| db, prompt_id=prompt.id, project_id=project.id, include_versions=False | |
| ) | |
| """Retrieve a prompt by ID with only its active version""" | |
| retrieved_prompt, versions = get_prompt_by_id( | |
| db, prompt_id=prompt.id, project_id=prompt.project_id, include_versions=False | |
| ) |
🤖 Prompt for AI Agents
In backend/app/tests/crud/test_prompts.py around lines 219 to 223, the test
calls get_prompt_by_id with project_id=project.id which can mismatch the
prompt's actual project; change the call to pass project_id=prompt.project_id
instead so the prompt lookup uses the prompt's true project_id (replace
project_id=project.id with project_id=prompt.project_id).
| """Retrieve a prompt by ID with all its versions""" | ||
| project = get_project(db) | ||
|
|
||
| # Add another version | ||
| new_version = PromptVersion( | ||
| prompt_id=prompt.id, | ||
| instruction="Updated instruction", | ||
| commit_message="Second version", | ||
| version=2, | ||
| ) | ||
| db.add(new_version) | ||
| db.commit() | ||
|
|
||
| retrieved_prompt, versions = get_prompt_by_id( | ||
| db, prompt_id=prompt.id, project_id=project.id, include_versions=True | ||
| ) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same issue: don’t call get_project(db) for an existing prompt.
Apply this diff:
- project = get_project(db)
@@
- retrieved_prompt, versions = get_prompt_by_id(
- db, prompt_id=prompt.id, project_id=project.id, include_versions=True
- )
+ retrieved_prompt, versions = get_prompt_by_id(
+ db, prompt_id=prompt.id, project_id=prompt.project_id, include_versions=True
+ )📝 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.
| """Retrieve a prompt by ID with all its versions""" | |
| project = get_project(db) | |
| # Add another version | |
| new_version = PromptVersion( | |
| prompt_id=prompt.id, | |
| instruction="Updated instruction", | |
| commit_message="Second version", | |
| version=2, | |
| ) | |
| db.add(new_version) | |
| db.commit() | |
| retrieved_prompt, versions = get_prompt_by_id( | |
| db, prompt_id=prompt.id, project_id=project.id, include_versions=True | |
| ) | |
| """Retrieve a prompt by ID with all its versions""" | |
| # Add another version | |
| new_version = PromptVersion( | |
| prompt_id=prompt.id, | |
| instruction="Updated instruction", | |
| commit_message="Second version", | |
| version=2, | |
| ) | |
| db.add(new_version) | |
| db.commit() | |
| retrieved_prompt, versions = get_prompt_by_id( | |
| db, prompt_id=prompt.id, project_id=prompt.project_id, include_versions=True | |
| ) |
🤖 Prompt for AI Agents
In backend/app/tests/crud/test_prompts.py around lines 241 to 257, the test
incorrectly calls get_project(db) to obtain a project for an existing prompt
which can create a different project; instead use the prompt's existing project
(e.g., project = prompt.project or fetch by prompt.project_id) so the retrieved
prompt is looked up against the correct project; update the assignment and any
subsequent uses to reference the prompt's project rather than calling
get_project(db).
3 participants
Summary
This PR implements the core functionality for managing AI prompts with robust versioning, lifecycle control, and environment labeling. It ensures auditability, rollback support, and clean separation between staging and production versions.
API Contracts : Notion
Target issue is #316
Version Lifecycle Rules
📝 Order of Review
To make the review process efficient and focused, please follow this order:
🔽 Skip
tests/folder in the initial pass.1️⃣ Prompt Core Functionality
📁
models/prompt.pyPromptmodel.📁
alembic/versions/<timestamp>_add_prompt_table.pyprompttable.📁
api/routes/prompts.pyandcrud/prompt.py2️⃣ Prompt Versioning
📁
models/prompt_version.py📁
api/routes/prompt_versions.pyandcrud/prompt_version.py3️⃣ ✅ Tests
tests/for coverage and correctness.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