Add comprehensive codebase audit with optimization plan

.github/workflows/ci.yml

@@ -44,7 +44,6 @@ jobs:
 
       - name: MyPy type check
         run: mypy app/ --ignore-missing-imports
-        continue-on-error: true  # SQLAlchemy Column[T] vs T false positives until models use Mapped[T]
 
   # ──────────────────────────────────────────
   # Backend: Tests
@@ -149,7 +148,7 @@ jobs:
         run: npm ci
 
       - name: Run tests
-        run: npx vitest run --passWithNoTests
+        run: npx vitest run
 
   # ──────────────────────────────────────────
   # Frontend: Build

app/database.py

@@ -3,6 +3,7 @@
 Uses async SQLAlchemy for PostgreSQL connections.
 """
 
+import logging
 from collections.abc import AsyncGenerator
 from contextlib import asynccontextmanager
 
@@ -17,6 +18,8 @@
 
 from app.config import get_settings
 
+logger = logging.getLogger(__name__)
+
 
 class Base(DeclarativeBase):
     """Base class for all SQLAlchemy models."""
@@ -154,7 +157,7 @@ async def init_db() -> None:
     async with engine.begin() as conn:
         await conn.run_sync(lambda _: None)  # Simple connectivity test
 
-    print("Database connection established successfully")
+    logger.info("Database connection established successfully")
 
 
 async def close_db() -> None:
@@ -166,7 +169,7 @@ async def close_db() -> None:
         _engine = None
         _async_session_factory = None
 
-    print("Database connections closed")
+    logger.info("Database connections closed")
 
 
 # ---------------------------------------------------------

app/exceptions.py

@@ -0,0 +1,88 @@
+"""
+Custom exception hierarchy for consistent error responses.
+
+Usage:
+    from app.exceptions import NotFoundError, ForbiddenError, ConflictError
+
+    raise NotFoundError("User", user_id)
+    raise ForbiddenError("Admin access required")
+    raise ConflictError("A user with this email already exists")
+    raise ValidationError("Invalid slug format")
+
+These exceptions are caught by the handler registered in main.py and
+converted to consistent JSON error responses with the shape:
+    {"error": "<message>", "detail": "<optional extra info>"}
+"""
+
+from fastapi import HTTPException, status
+
+
+class AppError(HTTPException):
+    """Base application error with a default status code."""
+
+    status_code: int = status.HTTP_500_INTERNAL_SERVER_ERROR
+
+    def __init__(self, message: str, detail: str | None = None):
+        super().__init__(
+            status_code=self.__class__.status_code,
+            detail=message,
+        )
+        self.extra_detail = detail
+
+
+class NotFoundError(AppError):
+    """Resource not found (404)."""
+
+    status_code = status.HTTP_404_NOT_FOUND
+
+    def __init__(self, resource: str, resource_id: int | str | None = None):
+        if resource_id is not None:
+            message = f"{resource} not found (id={resource_id})"
+        else:
+            message = f"{resource} not found"
+        super().__init__(message)
+
+
+class ForbiddenError(AppError):
+    """Forbidden access (403)."""
+
+    status_code = status.HTTP_403_FORBIDDEN
+
+    def __init__(self, message: str = "Access denied"):
+        super().__init__(message)
+
+
+class UnauthorizedError(AppError):
+    """Unauthorized access (401)."""
+
+    status_code = status.HTTP_401_UNAUTHORIZED
+
+    def __init__(self, message: str = "Authentication required"):
+        super().__init__(message)
+
+
+class ConflictError(AppError):
+    """Resource conflict (409)."""
+
+    status_code = status.HTTP_409_CONFLICT
+
+    def __init__(self, message: str):
+        super().__init__(message)
+
+
+class ValidationError(AppError):
+    """Validation error (400)."""
+
+    status_code = status.HTTP_400_BAD_REQUEST
+
+    def __init__(self, message: str):
+        super().__init__(message)
+
+
+class ServiceError(AppError):
+    """Internal service error (500)."""
+
+    status_code = status.HTTP_500_INTERNAL_SERVER_ERROR
+
+    def __init__(self, message: str = "Internal server error"):
+        super().__init__(message)

app/main.py

@@ -3,9 +3,10 @@
 """
 
 import json
+import logging
 from collections.abc import AsyncGenerator
 from contextlib import asynccontextmanager
-from datetime import UTC, date, datetime
+from datetime import date, datetime
 from pathlib import Path
 from typing import Any
 
@@ -17,51 +18,22 @@
 from app import __version__
 from app.config import get_settings
 from app.database import close_db, init_db
+from app.schemas.base import serialize_date_simple, serialize_datetime_js
 
+logger = logging.getLogger(__name__)
 
-def format_datetime_js(dt: datetime) -> str:
-    """
-    Format datetime to match JavaScript's toISOString().
-
-    Node.js outputs: "2025-12-08T09:01:16.715Z"
-    """
-    if dt is None:
-        return None
-
-    if dt.tzinfo is None:
-        # Assume UTC for naive datetimes
-        formatted = dt.strftime("%Y-%m-%dT%H:%M:%S")
-        ms = dt.microsecond // 1000
-        return f"{formatted}.{ms:03d}Z"
-    else:
-        # Convert to UTC and format
-        utc_dt = dt.astimezone(UTC)
-        formatted = utc_dt.strftime("%Y-%m-%dT%H:%M:%S")
-        ms = utc_dt.microsecond // 1000
-        return f"{formatted}.{ms:03d}Z"
-
-
-def format_date_js(d: date) -> str:
-    """
-    Format date to match Node.js date serialization.
 
-    Node.js stores dates as timestamps at midnight UTC, which when
-    serialized becomes the previous day at 23:00:00.000Z due to timezone.
+def custom_json_serializer(obj: Any) -> Any:
+    """Custom JSON serializer for non-standard types.
 
-    For consistency, we'll output as ISO date string since that's what
-    the database actually stores, and it's cleaner.
+    Delegates to the canonical serializers in app.schemas.base to ensure
+    consistent datetime/date formatting across dict-based responses and
+    Pydantic model responses.
     """
-    if d is None:
-        return None
-    return d.isoformat()
-
-
-def custom_json_serializer(obj: Any) -> Any:
-    """Custom JSON serializer for non-standard types."""
     if isinstance(obj, datetime):
-        return format_datetime_js(obj)
+        return serialize_datetime_js(obj)
     if isinstance(obj, date):
-        return format_date_js(obj)
+        return serialize_date_simple(obj)
     raise TypeError(f"Object of type {type(obj)} is not JSON serializable")
 
 
@@ -107,21 +79,21 @@ async def lifespan(app: FastAPI) -> AsyncGenerator[None, None]:
     """
     # Startup
     settings = get_settings()
-    print(f"Starting Milestone API v{__version__}")
-    print(f"Mode: {'Multi-Tenant' if settings.multi_tenant else 'Single-Tenant'}")
-    print(f"Debug: {settings.debug}")
+    logger.info("Starting Milestone API v%s", __version__)
+    logger.info("Mode: %s", "Multi-Tenant" if settings.multi_tenant else "Single-Tenant")
+    logger.info("Debug: %s", settings.debug)
 
     # Log proxy configuration
     if settings.https_proxy or settings.http_proxy or settings.proxy_pac_url:
-        print("Proxy Configuration:")
+        logger.info("Proxy Configuration:")
         if settings.https_proxy:
-            print(f"  HTTPS_PROXY: {settings.https_proxy}")
+            logger.info("  HTTPS_PROXY: %s", settings.https_proxy)
         if settings.http_proxy:
-            print(f"  HTTP_PROXY: {settings.http_proxy}")
+            logger.info("  HTTP_PROXY: %s", settings.http_proxy)
         if settings.proxy_pac_url:
-            print(f"  PROXY_PAC_URL: {settings.proxy_pac_url}")
+            logger.info("  PROXY_PAC_URL: %s", settings.proxy_pac_url)
     else:
-        print("Proxy Configuration: None")
+        logger.info("Proxy Configuration: None")
 
     # Initialize database connections
     if settings.multi_tenant:
@@ -132,7 +104,7 @@ async def lifespan(app: FastAPI) -> AsyncGenerator[None, None]:
         await master_db.init_db()
         await master_db.verify_admin_exists()
         tenant_connection_manager.start_cleanup_task()
-        print("Master database initialized")
+        logger.info("Master database initialized")
     else:
         # Single-tenant: connect to the default tenant database
         await init_db()
@@ -149,7 +121,7 @@ async def lifespan(app: FastAPI) -> AsyncGenerator[None, None]:
         await tenant_connection_manager.close_all()
         await master_db.close()
 
-    print("Milestone API shutdown complete")
+    logger.info("Milestone API shutdown complete")
 
 
 def create_app() -> FastAPI:
@@ -191,7 +163,7 @@ def create_app() -> FastAPI:
         CORSMiddleware,
         allow_origins=allowed_origins,
         allow_credentials=True,
-        allow_methods=["GET", "POST", "PUT", "DELETE", "OPTIONS"],
+        allow_methods=["GET", "POST", "PUT", "DELETE", "PATCH", "OPTIONS"],
         allow_headers=["Content-Type", "Accept", "Authorization"],
     )
 
@@ -377,16 +349,20 @@ async def serve_admin():
             return JSONResponse(status_code=404, content={"error": "Admin panel not found"})
 
         # SPA fallback - serve index.html for non-API routes
-        @app.get("/{full_path:path}")
+        # Handle all methods to return 404 instead of 405 for unmatched routes
+        @app.api_route("/{full_path:path}", methods=["GET", "POST", "PUT", "DELETE", "PATCH"])
         async def serve_spa(request: Request, full_path: str):
             """Serve the SPA frontend for non-API routes."""
-            # Debug: log all requests hitting this endpoint
-            print(f"[SPA Fallback] path={full_path}, method={request.method}")
+            logger.debug("SPA Fallback: path=%s, method=%s", full_path, request.method)
 
             # Don't serve index.html for API routes
             if full_path.startswith("api/"):
                 return JSONResponse(status_code=404, content={"error": "Not found"})
 
+            # Only serve the SPA for GET requests
+            if request.method != "GET":
+                return JSONResponse(status_code=404, content={"error": "Not found"})
+
             # Don't serve index.html for WebSocket endpoint
             # Handle both /ws and /t/{tenant}/ws patterns
             if (
@@ -395,7 +371,7 @@ async def serve_spa(request: Request, full_path: str):
                 or full_path.endswith("/ws")
                 or "/ws" in full_path
             ):
-                print(f"[SPA Fallback] WebSocket path detected: {full_path}, returning 426")
+                logger.debug("SPA Fallback: WebSocket path detected: %s, returning 426", full_path)
                 return JSONResponse(
                     status_code=426,  # Upgrade Required
                     content={"error": "WebSocket endpoint - use ws:// or wss:// protocol"},
@@ -428,10 +404,7 @@ async def no_frontend():
     @app.exception_handler(Exception)
     async def global_exception_handler(request: Request, exc: Exception):
         """Handle uncaught exceptions."""
-        import traceback
-
-        print(f"Unhandled exception on {request.method} {request.url.path}: {exc}")
-        traceback.print_exc()
+        logger.exception("Unhandled exception on %s %s", request.method, request.url.path)
         # Don't leak internal error details to clients
         return JSONResponse(
             status_code=500,
@@ -446,17 +419,17 @@ def create_wrapped_app():
     app = create_app()
     settings = get_settings()
 
-    print(f"[App Startup] MULTI_TENANT setting = {settings.multi_tenant}")
+    logger.info("MULTI_TENANT setting = %s", settings.multi_tenant)
 
     if settings.multi_tenant:
-        print("[App Startup] Wrapping app with TenantMiddleware")
+        logger.info("Wrapping app with TenantMiddleware")
         from app.middleware.tenant import TenantMiddleware
 
         # Wrap the entire app with tenant middleware
         # This ensures URL rewriting happens BEFORE FastAPI routing
         return TenantMiddleware(app)
 
-    print("[App Startup] Running in single-tenant mode (no middleware)")
+    logger.info("Running in single-tenant mode (no middleware)")
     return app