Fix database error handling in project creation - #79
Conversation
- Add sqlite3 import and try-except blocks around database calls in create_project endpoint (list_projects, create_project, get_project) - Replace skipped database error test with 4 mocked tests using unittest.mock.patch to simulate SQLite errors: - test_create_project_database_locked_error - test_create_project_disk_full_error - test_create_project_integrity_error - test_create_project_list_projects_database_error - All database errors now return HTTP 500 with descriptive error messages - All 15 tests in test_project_creation_api.py pass This ensures database failures are handled gracefully instead of causing crashes, improving API reliability and user experience.
WalkthroughAdds explicit sqlite3-based error handling around database interactions in the project creation endpoint, updates workspace creation flow to capture workspace paths, and enhances failure handling with cleanup logic. Corresponding test coverage for database error scenarios during project creation is added. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Code Review: Database Error Handling in Project CreationSummaryThis PR adds graceful error handling for SQLite database operations in the project creation API endpoint. The changes prevent crashes and provide meaningful error messages to users when database operations fail. ✅ Strengths
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
tests/api/test_project_creation_api.py (2)
12-13: Consider pytest fixtures over unittest.mock for database testing.The tests use
unittest.mock.patchto simulate database errors, but this approach doesn't test the actual database error handling flow. Consider using pytest fixtures with a real database to trigger actual sqlite3 errors, which would provide more comprehensive coverage and align with the project's testing patterns.Based on learnings, the codebase prefers pytest fixtures and avoiding over-mocking.
</review_comment_end>
218-293: Consider adding test coverage for workspace creation errors.The new tests cover database errors during
list_projects,create_project, andget_project, but thecreate_projectendpoint also includes workspace creation logic (lines 354-382 in server.py) that can fail. Consider adding a test that verifies error handling when workspace creation fails after the project is created in the database.</review_comment_end>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
codeframe/ui/server.py(3 hunks)tests/api/test_project_creation_api.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use Python 3.11+ with async/await syntax, asyncio, and AsyncAnthropic for AI integrations in backend code
Lint code with ruff check . before committing; fix all style violations to maintain consistent code quality across the Python codebase
Use type hints with Python 3.11+ throughout all Python code; run type checking as part of quality gates to catch type errors before task completion
Files:
tests/api/test_project_creation_api.pycodeframe/ui/server.py
🧠 Learnings (3)
📚 Learning: 2025-11-25T19:08:37.203Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: docs/CLAUDE.md:0-0
Timestamp: 2025-11-25T19:08:37.203Z
Learning: Applies to docs/tests/**/*.py : Use pytest fixtures for Python testing and avoid over-mocking
Applied to files:
tests/api/test_project_creation_api.py
📚 Learning: 2025-12-05T05:44:48.066Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-05T05:44:48.066Z
Learning: Applies to tests/test_*worker_agent.py : Run async worker agent tests with pytest using: pytest tests/test_*worker_agent.py; ensure all async context methods are properly tested with database fixtures
Applied to files:
tests/api/test_project_creation_api.py
📚 Learning: 2025-12-05T05:44:48.066Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-05T05:44:48.066Z
Learning: Applies to codeframe/persistence/database.py : Use SQLite with async support (aiosqlite) for all database operations; maintain schema tables: context_items (with agent_id scoping), blockers, code_reviews, token_usage, tasks, checkpoints
Applied to files:
codeframe/ui/server.py
🧬 Code graph analysis (2)
tests/api/test_project_creation_api.py (1)
tests/api/conftest.py (1)
api_client(42-89)
codeframe/ui/server.py (1)
codeframe/persistence/database.py (3)
list_projects(1093-1117)create_project(549-594)get_project(596-601)
⏰ 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). (3)
- GitHub Check: Backend Unit Tests
- GitHub Check: Frontend Unit Tests
- GitHub Check: claude-review
🔇 Additional comments (1)
codeframe/ui/server.py (1)
16-16: Consider migrating to async database operations.The codebase uses synchronous sqlite3 operations, but the learning notes recommend using aiosqlite for async support. This PR adds error handling for sync operations, which is appropriate for the current implementation. However, consider planning a future migration to async database operations for better concurrency and performance.
Based on learnings: "Use SQLite with async support (aiosqlite) for all database operations."
</review_comment_end>
| def test_create_project_database_locked_error(self, api_client): | ||
| """Test that database locked error returns 500 Internal Server Error.""" | ||
| from codeframe.ui import server | ||
|
|
||
| with patch.object( | ||
| server.app.state.db, | ||
| "create_project", | ||
| side_effect=sqlite3.OperationalError("database is locked"), | ||
| ): | ||
| response = api_client.post( | ||
| "/api/projects", | ||
| json={"name": "test-db-locked", "description": "Test project"}, | ||
| ) | ||
|
|
||
| assert response.status_code == 500 | ||
| data = response.json() | ||
| assert "detail" in data | ||
| assert "database" in data["detail"].lower() | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add type hints to test methods.
The test method lacks type hints for the api_client parameter. As per coding guidelines, use type hints throughout all Python code.
Apply this diff:
- def test_create_project_database_locked_error(self, api_client):
+ def test_create_project_database_locked_error(self, api_client: TestClient) -> None:
"""Test that database locked error returns 500 Internal Server Error."""You'll need to add this import at the top:
from starlette.testclient import TestClient</review_comment_end>
🤖 Prompt for AI Agents
In tests/api/test_project_creation_api.py around lines 218 to 236, the test
method test_create_project_database_locked_error lacks a type hint for the
api_client parameter; add the type annotation api_client: TestClient to the
method signature and ensure TestClient is imported at the top of the file (from
starlette.testclient import TestClient) so the hint resolves.
| def test_create_project_disk_full_error(self, api_client): | ||
| """Test that disk I/O error returns 500 Internal Server Error.""" | ||
| from codeframe.ui import server | ||
|
|
||
| with patch.object( | ||
| server.app.state.db, | ||
| "create_project", | ||
| side_effect=sqlite3.OperationalError("disk I/O error"), | ||
| ): | ||
| response = api_client.post( | ||
| "/api/projects", | ||
| json={"name": "test-disk-full", "description": "Test project"}, | ||
| ) | ||
|
|
||
| assert response.status_code == 500 | ||
| data = response.json() | ||
| assert "detail" in data | ||
| assert "database" in data["detail"].lower() or "i/o" in data["detail"].lower() | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add type hints to test methods.
Missing type hints for the api_client parameter.
Apply this diff:
- def test_create_project_disk_full_error(self, api_client):
+ def test_create_project_disk_full_error(self, api_client: TestClient) -> None:
"""Test that disk I/O error returns 500 Internal Server Error."""</review_comment_end>
🤖 Prompt for AI Agents
In tests/api/test_project_creation_api.py around lines 237 to 255, the test
method test_create_project_disk_full_error is missing a type hint for the
api_client parameter; update the signature to include the correct type hint
(e.g., api_client: TestClient or the project’s fixture type) so the test
function is properly typed, and run tests to ensure the import/type is available
or add an import/forward reference for the TestClient type if needed.
| def test_create_project_integrity_error(self, api_client): | ||
| """Test that constraint violation error returns 500 Internal Server Error.""" | ||
| from codeframe.ui import server | ||
|
|
||
| with patch.object( | ||
| server.app.state.db, | ||
| "create_project", | ||
| side_effect=sqlite3.IntegrityError("UNIQUE constraint failed"), | ||
| ): | ||
| response = api_client.post( | ||
| "/api/projects", | ||
| json={"name": "test-integrity", "description": "Test project"}, | ||
| ) | ||
|
|
||
| assert response.status_code == 500 | ||
| data = response.json() | ||
| assert "detail" in data | ||
| assert "database" in data["detail"].lower() or "constraint" in data["detail"].lower() | ||
|
|
There was a problem hiding this comment.
Clarify IntegrityError test scenario and add type hints.
This test simulates an IntegrityError during create_project, but the duplicate name check at lines 335-338 in server.py should prevent most IntegrityError cases from reaching create_project. Consider clarifying the test scenario (e.g., race condition between duplicate check and insert) or documenting that this tests edge cases where the duplicate check is bypassed.
Also missing type hints for the api_client parameter.
Apply this diff:
- def test_create_project_integrity_error(self, api_client):
- """Test that constraint violation error returns 500 Internal Server Error."""
+ def test_create_project_integrity_error(self, api_client: TestClient) -> None:
+ """Test that constraint violation error returns 500 Internal Server Error.
+
+ This tests edge cases where IntegrityError occurs despite duplicate name checks,
+ such as race conditions or other constraint violations.
+ """</review_comment_end>
🤖 Prompt for AI Agents
In tests/api/test_project_creation_api.py around lines 256 to 274, clarify the
IntegrityError test by updating the docstring or adding an inline comment
stating this simulates an edge-case race condition where the duplicate-name
pre-check is bypassed (e.g., another insert occurs between check and insert),
and add a type hint for the api_client parameter (e.g., api_client: TestClient
or the appropriate fixture type) to the test signature so the test function is
typed.
| def test_create_project_list_projects_database_error(self, api_client): | ||
| """Test that database error during list_projects returns 500 Internal Server Error.""" | ||
| from codeframe.ui import server | ||
|
|
||
| with patch.object( | ||
| server.app.state.db, | ||
| "list_projects", | ||
| side_effect=sqlite3.OperationalError("database is locked"), | ||
| ): | ||
| response = api_client.post( | ||
| "/api/projects", | ||
| json={"name": "test-list-error", "description": "Test project"}, | ||
| ) | ||
|
|
||
| assert response.status_code == 500 | ||
| data = response.json() | ||
| assert "detail" in data | ||
| assert "database" in data["detail"].lower() | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add type hints to test methods.
Missing type hints for the api_client parameter.
Apply this diff:
- def test_create_project_list_projects_database_error(self, api_client):
+ def test_create_project_list_projects_database_error(self, api_client: TestClient) -> None:
"""Test that database error during list_projects returns 500 Internal Server Error."""</review_comment_end>
🤖 Prompt for AI Agents
In tests/api/test_project_creation_api.py around lines 275 to 293, the test
function is missing a type hint for the api_client parameter; add an explicit
type annotation (for example api_client: TestClient) and import the matching
TestClient type (from fastapi.testclient import TestClient or the project's
fixture type) at the top of the file so the test signatures are fully typed and
consistent with other tests.
Code Review: Fix database error handling in project creationSummaryThis PR improves error handling for database operations in the project creation endpoint. The changes replace a skipped test with comprehensive mocked error scenarios and add proper try-except blocks around all database calls. ✅ Strengths
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
tests/api/test_project_creation_api.py (4)
218-236: Add type hints to test method.Missing type hints for the
api_clientparameter. As per coding guidelines, use type hints throughout all Python code.Apply this diff:
- def test_create_project_database_locked_error(self, api_client): + def test_create_project_database_locked_error(self, api_client: TestClient) -> None: """Test that database locked error returns 500 Internal Server Error."""Add this import at the top of the file:
from starlette.testclient import TestClient
237-255: Add type hints to test method.Missing type hints for the
api_clientparameter.- def test_create_project_disk_full_error(self, api_client): + def test_create_project_disk_full_error(self, api_client: TestClient) -> None:
256-274: Clarify IntegrityError test scenario and add type hints.This test is valid for edge cases like race conditions where the duplicate check passes but a constraint violation occurs during insert. Consider clarifying this in the docstring. Also missing type hints.
- def test_create_project_integrity_error(self, api_client): - """Test that constraint violation error returns 500 Internal Server Error.""" + def test_create_project_integrity_error(self, api_client: TestClient) -> None: + """Test that constraint violation error returns 500 Internal Server Error. + + Tests edge cases where IntegrityError occurs despite duplicate name checks, + such as race conditions or other constraint violations. + """
275-293: Add type hints to test method.Missing type hints for the
api_clientparameter.- def test_create_project_list_projects_database_error(self, api_client): + def test_create_project_list_projects_database_error(self, api_client: TestClient) -> None:codeframe/ui/server.py (3)
330-334: Sanitize error messages to avoid information disclosure.The error handler exposes internal database error details directly to the API response. This could leak sensitive information about the database schema, file paths, or internal state.
Consider using a generic error message for production:
try: existing_projects = app.state.db.list_projects() except sqlite3.Error as e: - raise HTTPException(status_code=500, detail=f"Database error: {str(e)}") + logger.error(f"Database error in list_projects: {str(e)}") + raise HTTPException(status_code=500, detail="Failed to retrieve projects from database")
341-351: Sanitize error messages to avoid information disclosure.Similar to the list_projects error handler, this exposes internal database error details. Use a generic error message and log the details instead.
try: project_id = app.state.db.create_project( name=request.name, description=request.description, source_type=request.source_type.value, source_location=request.source_location, source_branch=request.source_branch, workspace_path="", ) except sqlite3.Error as e: - raise HTTPException(status_code=500, detail=f"Database error: {str(e)}") + logger.error(f"Database error in create_project: {str(e)}") + raise HTTPException(status_code=500, detail="Failed to create project in database")
412-415: Sanitize error messages to avoid information disclosure.This error handler also exposes internal database details. Use a generic error message and log the details.
try: project = app.state.db.get_project(project_id) except sqlite3.Error as e: - raise HTTPException(status_code=500, detail=f"Database error: {str(e)}") + logger.error(f"Database error in get_project for project {project_id}: {str(e)}") + raise HTTPException(status_code=500, detail="Failed to retrieve project from database")
🧹 Nitpick comments (3)
tests/api/test_project_creation_api.py (2)
294-311: Good test coverage; add type hints.This test properly covers the update_project error path added in server.py. However, it's missing type hints for consistency with coding guidelines.
- def test_create_project_update_project_database_error(self, api_client): + def test_create_project_update_project_database_error(self, api_client: TestClient) -> None:
313-330: Good test coverage; add type hints.This test properly covers the get_project error path. Missing type hints for consistency.
- def test_create_project_get_project_database_error(self, api_client): + def test_create_project_get_project_database_error(self, api_client: TestClient) -> None:codeframe/ui/server.py (1)
388-409: Improved cleanup handling; consider sanitizing workspace error message.The cleanup logic properly wraps the delete_project call in a try-except to handle cases where the database error might occur during cleanup. The workspace path sanitization on line 409 could potentially leak internal paths.
For consistency with the other error handlers, consider sanitizing:
- raise HTTPException(status_code=500, detail=f"Workspace creation failed: {str(e)}") + raise HTTPException(status_code=500, detail="Workspace creation failed")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
codeframe/ui/server.py(4 hunks)tests/api/test_project_creation_api.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use Python 3.11+ with async/await syntax, asyncio, and AsyncAnthropic for AI integrations in backend code
Lint code with ruff check . before committing; fix all style violations to maintain consistent code quality across the Python codebase
Use type hints with Python 3.11+ throughout all Python code; run type checking as part of quality gates to catch type errors before task completion
Files:
tests/api/test_project_creation_api.pycodeframe/ui/server.py
🧠 Learnings (3)
📚 Learning: 2025-11-25T19:08:37.203Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: docs/CLAUDE.md:0-0
Timestamp: 2025-11-25T19:08:37.203Z
Learning: Applies to docs/tests/**/*.py : Use pytest fixtures for Python testing and avoid over-mocking
Applied to files:
tests/api/test_project_creation_api.py
📚 Learning: 2025-12-05T05:44:48.066Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-05T05:44:48.066Z
Learning: Applies to tests/test_*worker_agent.py : Run async worker agent tests with pytest using: pytest tests/test_*worker_agent.py; ensure all async context methods are properly tested with database fixtures
Applied to files:
tests/api/test_project_creation_api.py
📚 Learning: 2025-12-05T05:44:48.066Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-05T05:44:48.066Z
Learning: Applies to codeframe/persistence/database.py : Use SQLite with async support (aiosqlite) for all database operations; maintain schema tables: context_items (with agent_id scoping), blockers, code_reviews, token_usage, tasks, checkpoints
Applied to files:
codeframe/ui/server.py
🧬 Code graph analysis (2)
tests/api/test_project_creation_api.py (1)
tests/api/conftest.py (1)
api_client(42-89)
codeframe/ui/server.py (1)
codeframe/persistence/database.py (5)
list_projects(1093-1117)create_project(549-594)update_project(1157-1189)delete_project(1191-1199)get_project(596-601)
⏰ 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). (3)
- GitHub Check: Frontend Unit Tests
- GitHub Check: Backend Unit Tests
- GitHub Check: claude-review
🔇 Additional comments (2)
tests/api/test_project_creation_api.py (1)
12-14: Imports are appropriate for the database error simulation approach.The
sqlite3import enables raising specific SQLite exceptions, andunittest.mock.patchis standard for patching object methods. This approach is acceptable for simulating database errors that are difficult to trigger with real database operations.codeframe/ui/server.py (1)
16-16: Import addition is appropriate.The
sqlite3import is necessary for catching specific SQLite exceptions in the new error handling code.
Addresses security concerns from code review: **Issue #1 (SECURITY - MUST FIX): Information Disclosure Risk** - Replace raw SQLite error exposure with generic user-facing messages - Log detailed errors server-side for debugging - Prevents leaking database paths, schema details, and internal state - Changes: - list_projects(): "Database error occurred. Please try again later." - create_project(): "Database error occurred. Please try again later." - update_project(): "Database error occurred. Please try again later." - get_project(): "Database error occurred. Please try again later." - Workspace creation: "Workspace creation failed. Please try again later." **Issue #2 (BUG - SHOULD FIX): Incomplete Cleanup Path** - Use actual workspace_path variable from create_workspace() return value - Previously reconstructed path: workspace_root / str(project_id) - Now uses the actual path returned by WorkspaceManager - Ensures correct cleanup even if path construction logic differs **Issue #3 (CODE QUALITY): Inconsistent Exception Handling** - Replace broad Exception with specific (OSError, PermissionError) - Consistent with sqlite3.Error specificity used elsewhere - Better error handling and debugging **Fix: HTTPException Re-raising** - Add explicit except HTTPException clause to prevent double-wrapping - Database errors from update_project() now properly propagate - Prevents "Workspace creation failed" masking "Database error" Test results: 17/17 passing (100%) All error messages now production-safe while maintaining debuggability
Code Review - PR #79: Fix database error handling in project creationSummaryThis PR adds comprehensive database error handling to the project creation endpoint with proper cleanup mechanisms and test coverage. Overall, this is a solid improvement to API reliability with good test coverage. ✅ Strengths
🔍 Issues & Suggestions1. Inconsistent Exception Handling Pattern (Minor)Location: except HTTPException:
# Re-raise HTTPException from database error handling above
raiseIssue: This pattern only re-raises Suggestion: This is actually correct as-is, but the comment could be clearer: except HTTPException:
# Re-raise HTTPException from update_project database error (already cleaned up)
raise2. Missing Error Handling in Other Endpoints (Medium Priority)Locations: Other endpoints also call database methods without error handling:
Suggestion: While this PR focuses on project creation (and does it well), consider adding similar error handling to other endpoints in a follow-up PR to ensure consistent error handling across the API. 3. Test Assertion Could Be More Specific (Minor)Location: assert "database" in data["detail"].lower() or "i/o" in data["detail"].lower()Issue: The assertion for Suggestion: Since all database errors return the same generic message, simplify to: assert "database" in data["detail"].lower()Same applies to line 273 in 4. Potential Race Condition in Cleanup (Low Priority)Location: workspace_path = app.state.workspace_manager.workspace_root / str(project_id)
if workspace_path.exists():
try:
shutil.rmtree(workspace_path)Issue: If workspace creation partially succeeds but uses a different path than expected, this hardcoded path reconstruction might miss it. (Though this is unlikely in practice given the current Suggestion: Consider capturing 5. Documentation Could Mention Cleanup Behavior (Minor)Location: Suggestion: Add a note in the docstring about transactional behavior: """Create a new project with automatic workspace initialization.
Creates a project record and workspace atomically. On failure, automatically
cleans up any partial state (database records + filesystem).
Args:
request: Project creation request with name, description, source details
Returns:
Created project details
Raises:
HTTPException: 409 if project name already exists
HTTPException: 500 if database or workspace creation fails
"""🔒 Security Considerations✅ No security issues found. Generic error messages prevent information leakage while detailed errors are logged server-side. 🚀 Performance Considerations✅ No performance impact. Exception handling has negligible overhead in the happy path. 📊 Test CoverageExcellent test coverage with 6 new mocked database error tests covering all failure points. The tests properly use Suggested addition: Consider adding one integration test that actually triggers a real database error (e.g., by setting database file to read-only) to validate the full error handling path. The current tests only mock the database methods. 📝 Final RecommendationApprove with minor suggestions. This PR significantly improves error handling and API reliability. The issues noted above are minor and could be addressed in follow-up PRs. Priority follow-ups:
Great work on the comprehensive test coverage! 🎉 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
codeframe/ui/server.py (4)
329-336: Good, DB errors during duplicate-name pre-check are now sanitized and loggedCatching
sqlite3.Error, logging the details, and returning a generic 500 response avoids leaking DB internals while still failing fast when the project list can’t be read. Consider addingexc_info=Trueto capture stack traces for debugging:- except sqlite3.Error as e: - logger.error(f"Database error listing projects: {str(e)}") + except sqlite3.Error as e: + logger.error(f"Database error listing projects: {e}", exc_info=True)This is optional but improves diagnosability.
344-357: Create-project insert error handling is correct; optional refinement for loggingWrapping
app.state.db.create_projectin asqlite3.Errorhandler and returning a generic 500 with a user-friendly message is aligned with the PR goal and avoids exposing raw DB messages. As above, you could log withexc_info=Truefor better traces:- except sqlite3.Error as e: - logger.error(f"Database error creating project: {str(e)}") + except sqlite3.Error as e: + logger.error(f"Database error creating project: {e}", exc_info=True)Functionally this block looks solid.
399-422: Workspace-creation failure path is defensive and well-containedCatching generic exceptions from
create_workspace, logging, then best-effort deleting the project row and workspace directory before raising a sanitized 500 gives a clean failure mode for callers without leaking implementation details. Usingworkspace_root / str(project_id)as a fallback path for cleanup is a reasonable defensive measure.If you want richer diagnostics, you could also add
exc_info=Trueto the main error log at Line 401, but the current behavior is already acceptable.
425-431: Final DB read is now guarded; consider handling aNoneproject explicitlyWrapping
get_project(project_id)in asqlite3.Errorhandler and returning a generic 500 avoids exposing DB internals, consistent with the rest of the flow. One small resilience improvement would be to guard against an unexpectedNoneresult (e.g., if the row were deleted between insert and read) instead of letting aTypeErrorbubble from theProjectResponseconstructor:- try: - project = app.state.db.get_project(project_id) - except sqlite3.Error as e: - logger.error(f"Database error retrieving project {project_id}: {str(e)}") - raise HTTPException( - status_code=500, detail="Database error occurred. Please try again later." - ) - - return ProjectResponse( + try: + project = app.state.db.get_project(project_id) + except sqlite3.Error as e: + logger.error(f"Database error retrieving project {project_id}: {e}", exc_info=True) + raise HTTPException( + status_code=500, detail="Database error occurred. Please try again later." + ) + + if not project: + # Extremely unlikely immediately after creation, but avoids obscure errors + raise HTTPException( + status_code=500, + detail="Project could not be retrieved after creation. Please try again later.", + ) + + return ProjectResponse( id=project["id"], name=project["name"], status=project.get("status", "init"), phase=project.get("phase", "discovery"), created_at=project["created_at"], config=project.get("config"), )Not strictly required for correctness in the common path, but it makes the endpoint more robust to rare race conditions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
codeframe/ui/server.py(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use Python 3.11+ with async/await syntax, asyncio, and AsyncAnthropic for AI integrations in backend code
Lint code with ruff check . before committing; fix all style violations to maintain consistent code quality across the Python codebase
Use type hints with Python 3.11+ throughout all Python code; run type checking as part of quality gates to catch type errors before task completion
Files:
codeframe/ui/server.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-05T05:44:48.066Z
Learning: Applies to codeframe/persistence/database.py : Use SQLite with async support (aiosqlite) for all database operations; maintain schema tables: context_items (with agent_id scoping), blockers, code_reviews, token_usage, tasks, checkpoints
📚 Learning: 2025-12-05T05:44:48.066Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-05T05:44:48.066Z
Learning: Applies to codeframe/persistence/database.py : Use SQLite with async support (aiosqlite) for all database operations; maintain schema tables: context_items (with agent_id scoping), blockers, code_reviews, token_usage, tasks, checkpoints
Applied to files:
codeframe/ui/server.py
🧬 Code graph analysis (1)
codeframe/ui/server.py (1)
codeframe/persistence/database.py (5)
list_projects(1093-1117)create_project(549-594)update_project(1157-1189)delete_project(1191-1199)get_project(596-601)
⏰ 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). (3)
- GitHub Check: Frontend Unit Tests
- GitHub Check: Backend Unit Tests
- GitHub Check: claude-review
🔇 Additional comments (2)
codeframe/ui/server.py (2)
16-16: Importingsqlite3for explicit DB error handling is appropriateUsing
sqlite3.Erroras the common base for DB failures matches the currentDatabaseimplementation and is a clean way to centralize DB-related exception handling.
369-393: Robust cleanup on workspace-path update failuresThe nested try/except around
update_projectplus best-effort cleanup of both the DB row and workspace directory is well-structured and avoids leaving partially initialized projects behind. The final HTTP 500 response uses a generic message, so DB internals are not exposed.No correctness issues spotted here; the control flow (log → cleanup → raise 500) is clear and safe.
Response to Code Review FeedbackThank you for the thorough reviews! I've addressed all the critical and recommended issues. ✅ First Review Issues (All Addressed)Issue #1 - Missing Error Handling in Cleanup Path ✓
Issue #3 - Missing Error Handling on update_project() ✓
Issue #4 - Test Coverage Gap ✓
Issue #6 - Error Message Exposure ✓ (see details below) ✅ Second Review Issues (All MUST FIX & SHOULD FIX Addressed)Issue #1 - SECURITY: Information Disclosure Risk ✓ [MUST FIX] Problem: Raw SQLite errors exposed database paths, schema, and internal details Solution:
Before: raise HTTPException(status_code=500, detail=f"Database error: {str(e)}")
# Exposed: "Database error: database is locked at /path/to/db"After: logger.error(f"Database error listing projects: {str(e)}") # Server logs only
raise HTTPException(
status_code=500, detail="Database error occurred. Please try again later."
)Issue #2 - BUG: Incomplete Cleanup Path ✓ [SHOULD FIX] Problem: Cleanup reconstructed path instead of using actual Solution:
Before: workspace_dir = app.state.workspace_manager.workspace_root / str(project_id) # Reconstructed
if workspace_dir.exists():After: if workspace_path.exists(): # Uses actual path from create_workspace() return valueIssue #3 - CODE QUALITY: Inconsistent Exception Handling ✓ [NICE TO HAVE] Problem: Used broad Solution:
Before: except Exception as cleanup_error:After: except (OSError, PermissionError) as cleanup_error:Bonus Fix: HTTPException Re-raising Problem: Database errors from Solution:
except HTTPException:
# Re-raise HTTPException from database error handling above
raise
except Exception as e:
# Handle workspace creation errors only
...📊 Test ResultsAll 17/17 tests passing (100%):
🔒 Security Improvements Summary
📝 Other Review PointsIssue #5 (Database Connection Context): Issues #4 & #5 from Second Review (Test Quality):
I'm happy to address these in a follow-up PR if desired, but they're not critical for this security/reliability improvement. Ready for merge! 🚀 All critical security issues addressed. Error handling is comprehensive, production-safe, and fully tested. |
2 participants
This ensures database failures are handled gracefully instead of causing crashes, improving API reliability and user experience.
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.