From 197ce430444c8abc9f16daf1f6760b7302294d66 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 10 Dec 2025 04:26:34 +0000 Subject: [PATCH 1/3] fix: Add graceful database error handling to project creation API - 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. --- codeframe/ui/server.py | 31 +++++++--- tests/api/test_project_creation_api.py | 85 +++++++++++++++++++++++--- 2 files changed, 99 insertions(+), 17 deletions(-) diff --git a/codeframe/ui/server.py b/codeframe/ui/server.py index 17ab72c0..9ac88d81 100644 --- a/codeframe/ui/server.py +++ b/codeframe/ui/server.py @@ -13,6 +13,7 @@ import logging import os import shutil +import sqlite3 from codeframe.core.models import ( ProjectStatus, @@ -326,21 +327,28 @@ async def create_project(request: ProjectCreateRequest): ) # Check for duplicate project name - existing_projects = app.state.db.list_projects() + try: + existing_projects = app.state.db.list_projects() + except sqlite3.Error as e: + raise HTTPException(status_code=500, detail=f"Database error: {str(e)}") + if any(p["name"] == request.name for p in existing_projects): raise HTTPException( status_code=409, detail=f"Project with name '{request.name}' already exists" ) # Create project record first (to get ID) - 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="", # Will be updated after workspace creation - ) + 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="", # Will be updated after workspace creation + ) + except sqlite3.Error as e: + raise HTTPException(status_code=500, detail=f"Database error: {str(e)}") # Create workspace try: @@ -374,7 +382,10 @@ async def create_project(request: ProjectCreateRequest): raise HTTPException(status_code=500, detail=f"Workspace creation failed: {str(e)}") # Return project details - project = app.state.db.get_project(project_id) + 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)}") return ProjectResponse( id=project["id"], diff --git a/tests/api/test_project_creation_api.py b/tests/api/test_project_creation_api.py index f384fca1..cd229438 100644 --- a/tests/api/test_project_creation_api.py +++ b/tests/api/test_project_creation_api.py @@ -9,6 +9,9 @@ 3. REFACTOR: Clean up while keeping tests green """ +import sqlite3 +from unittest.mock import patch + import pytest @@ -212,13 +215,81 @@ def test_create_project_via_api_then_get_status(self, api_client): class TestProjectCreationErrorHandling: """Test error handling for project creation API.""" - @pytest.mark.skip( - reason="Database close() creates ungraceful crashes, not 500 errors. This test design is flawed." - ) - def test_create_project_handles_database_errors(self, api_client): - """Test that database errors are handled gracefully (500 Internal Server Error).""" - # This test is skipped - see reason above - pass + 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() + + 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() + + 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() + + 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() def test_create_project_with_extra_fields(self, api_client): """Test that extra fields in request are ignored.""" From 18adf2becf1b3cdfc7f8278d9008c30e5d4f21bc Mon Sep 17 00:00:00 2001 From: frankbria Date: Wed, 10 Dec 2025 17:28:03 -0700 Subject: [PATCH 2/3] Add comprehensive error handling for database operations in cleanup paths --- codeframe/ui/server.py | 35 +++++++++++++++++++++--- tests/api/test_project_creation_api.py | 38 ++++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 4 deletions(-) diff --git a/codeframe/ui/server.py b/codeframe/ui/server.py index 9ac88d81..af7b1699 100644 --- a/codeframe/ui/server.py +++ b/codeframe/ui/server.py @@ -360,13 +360,40 @@ async def create_project(request: ProjectCreateRequest): ) # Update project with workspace path and git status - app.state.db.update_project( - project_id, {"workspace_path": str(workspace_path), "git_initialized": True} - ) + try: + app.state.db.update_project( + project_id, {"workspace_path": str(workspace_path), "git_initialized": True} + ) + except sqlite3.Error as db_error: + # Database error during update - cleanup and fail + logger.error(f"Database error updating project {project_id}: {db_error}") + + # Best-effort cleanup: delete project record + try: + app.state.db.delete_project(project_id) + except sqlite3.Error as cleanup_db_error: + logger.error(f"Failed to delete project {project_id} during cleanup: {cleanup_db_error}") + + # Best-effort cleanup: remove workspace directory + workspace_dir = app.state.workspace_manager.workspace_root / str(project_id) + if workspace_dir.exists(): + try: + shutil.rmtree(workspace_dir) + logger.info(f"Cleaned up workspace directory: {workspace_dir}") + except Exception as cleanup_fs_error: + logger.error(f"Failed to clean up workspace {workspace_dir}: {cleanup_fs_error}") + + raise HTTPException(status_code=500, detail=f"Database error: {str(db_error)}") except Exception as e: # Cleanup: delete project and workspace if creation fails - app.state.db.delete_project(project_id) + logger.error(f"Workspace creation failed for project {project_id}: {e}") + + # Best-effort cleanup: delete project record + try: + app.state.db.delete_project(project_id) + except sqlite3.Error as cleanup_db_error: + logger.error(f"Failed to delete project {project_id} during cleanup: {cleanup_db_error}") # Explicitly clean up workspace directory if it exists # (Defense in depth: WorkspaceManager has cleanup, but this ensures diff --git a/tests/api/test_project_creation_api.py b/tests/api/test_project_creation_api.py index cd229438..3c9242c3 100644 --- a/tests/api/test_project_creation_api.py +++ b/tests/api/test_project_creation_api.py @@ -291,6 +291,44 @@ def test_create_project_list_projects_database_error(self, api_client): assert "detail" in data assert "database" in data["detail"].lower() + def test_create_project_update_project_database_error(self, api_client): + """Test that database error during update_project returns 500 Internal Server Error.""" + from codeframe.ui import server + + with patch.object( + server.app.state.db, + "update_project", + side_effect=sqlite3.OperationalError("database is locked"), + ): + response = api_client.post( + "/api/projects", + json={"name": "test-update-error", "description": "Test project"}, + ) + + assert response.status_code == 500 + data = response.json() + assert "detail" in data + assert "database" in data["detail"].lower() + + def test_create_project_get_project_database_error(self, api_client): + """Test that database error during get_project returns 500 Internal Server Error.""" + from codeframe.ui import server + + with patch.object( + server.app.state.db, + "get_project", + side_effect=sqlite3.OperationalError("database is locked"), + ): + response = api_client.post( + "/api/projects", + json={"name": "test-get-error", "description": "Test project"}, + ) + + assert response.status_code == 500 + data = response.json() + assert "detail" in data + assert "database" in data["detail"].lower() + def test_create_project_with_extra_fields(self, api_client): """Test that extra fields in request are ignored.""" # ACT From 46794ccb4ef526b945850c16d5d410a009b7e381 Mon Sep 17 00:00:00 2001 From: frankbria Date: Wed, 10 Dec 2025 17:36:31 -0700 Subject: [PATCH 3/3] Sanitize database error messages to prevent information disclosure 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 --- codeframe/ui/server.py | 42 +++++++++++++++++++++++++++++------------- 1 file changed, 29 insertions(+), 13 deletions(-) diff --git a/codeframe/ui/server.py b/codeframe/ui/server.py index af7b1699..cf783746 100644 --- a/codeframe/ui/server.py +++ b/codeframe/ui/server.py @@ -330,7 +330,10 @@ async def create_project(request: ProjectCreateRequest): 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 listing projects: {str(e)}") + raise HTTPException( + status_code=500, detail="Database error occurred. Please try again later." + ) if any(p["name"] == request.name for p in existing_projects): raise HTTPException( @@ -348,7 +351,10 @@ async def create_project(request: ProjectCreateRequest): workspace_path="", # Will be updated after workspace creation ) except sqlite3.Error as e: - raise HTTPException(status_code=500, detail=f"Database error: {str(e)}") + logger.error(f"Database error creating project: {str(e)}") + raise HTTPException( + status_code=500, detail="Database error occurred. Please try again later." + ) # Create workspace try: @@ -374,16 +380,21 @@ async def create_project(request: ProjectCreateRequest): except sqlite3.Error as cleanup_db_error: logger.error(f"Failed to delete project {project_id} during cleanup: {cleanup_db_error}") - # Best-effort cleanup: remove workspace directory - workspace_dir = app.state.workspace_manager.workspace_root / str(project_id) - if workspace_dir.exists(): + # Best-effort cleanup: remove workspace directory (use actual workspace_path) + if workspace_path.exists(): try: - shutil.rmtree(workspace_dir) - logger.info(f"Cleaned up workspace directory: {workspace_dir}") - except Exception as cleanup_fs_error: - logger.error(f"Failed to clean up workspace {workspace_dir}: {cleanup_fs_error}") + shutil.rmtree(workspace_path) + logger.info(f"Cleaned up workspace directory: {workspace_path}") + except (OSError, PermissionError) as cleanup_fs_error: + logger.error(f"Failed to clean up workspace {workspace_path}: {cleanup_fs_error}") - raise HTTPException(status_code=500, detail=f"Database error: {str(db_error)}") + raise HTTPException( + status_code=500, detail="Database error occurred. Please try again later." + ) + + except HTTPException: + # Re-raise HTTPException from database error handling above + raise except Exception as e: # Cleanup: delete project and workspace if creation fails @@ -403,16 +414,21 @@ async def create_project(request: ProjectCreateRequest): try: shutil.rmtree(workspace_path) logger.info(f"Cleaned up orphaned workspace: {workspace_path}") - except Exception as cleanup_error: + except (OSError, PermissionError) as cleanup_error: logger.error(f"Failed to clean up workspace {workspace_path}: {cleanup_error}") - raise HTTPException(status_code=500, detail=f"Workspace creation failed: {str(e)}") + raise HTTPException( + status_code=500, detail="Workspace creation failed. Please try again later." + ) # Return project 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 retrieving project {project_id}: {str(e)}") + raise HTTPException( + status_code=500, detail="Database error occurred. Please try again later." + ) return ProjectResponse( id=project["id"],