Maintainability: production safety gate, API response models, security rationale, PR template

.github/pull_request_template.md

@@ -0,0 +1,51 @@
+# Pull Request
+
+## Summary
+
+<!-- What does this PR change and why? Link related issues. -->
+
+## Type of Change
+
+- [ ] Bug fix
+- [ ] New feature
+- [ ] Refactor / code quality
+- [ ] CI / tooling
+- [ ] Documentation
+
+## Checklist
+
+### Tests Added
+
+- [ ] New/changed behaviour is covered by unit tests
+- [ ] Full backend test suite passes locally (`pytest backend/tests/`)
+- [ ] Legacy migration tests still pass where applicable
+      (`pytest backend/migration/tests/`)
+
+### Verified Workflow Paths
+
+- [ ] I ran the workflow path verifier and it passed:
+      `python3 tools/verify_workflow_paths.py`
+      (verifies every file/script referenced by CI workflows exists)
+- [ ] Any moved/renamed files are updated in workflows, scripts
+      (`scripts/`, `preflight.*`), and docs
+
+### Safety & Security
+
+- [ ] No new `# nosec` / `# noqa` suppressions — or each new suppression is
+      documented in `docs/security/Rationale.md`
+- [ ] No secrets, credentials, or internal paths added to code or logs
+- [ ] Shell scripts validate their inputs (env names, table names, paths)
+
+### API & Data
+
+- [ ] New/changed endpoints declare a Pydantic `response_model`
+- [ ] Database state transitions follow the `RunStatus` lifecycle
+      (`ALLOWED_TRANSITIONS` in `backend/database/models.py`)
+
+## Screenshots / Output
+
+<!-- Test runs, CLI output, or UI screenshots where relevant. -->
+
+## Notes for Reviewers
+
+<!-- Anything reviewers should pay special attention to. -->

backend/api/routes/dashboard.py

@@ -5,13 +5,14 @@
 from fastapi import APIRouter, Depends
 from sqlalchemy.orm import Session
 
+from api.schemas import DashboardStats
 from database.connection import get_db
 from services import dashboard_service
 
 router = APIRouter(tags=["dashboard"])
 
 
-@router.get("/dashboard")
+@router.get("/dashboard", response_model=DashboardStats)
 def get_dashboard(db: Session = Depends(get_db)):
     """Return aggregate dashboard statistics."""
     return dashboard_service.get_dashboard_stats(db)

backend/api/routes/execution.py

@@ -6,13 +6,14 @@
 from fastapi import APIRouter, Depends, HTTPException
 from sqlalchemy.orm import Session
 
+from api.schemas import EvaluationResponse, MigrationExecuteResponse
 from database.connection import get_db
 from services import execution_service, evaluation_service
 
 router = APIRouter(prefix="/migrations", tags=["execution"])
 
 
-@router.post("/{run_id}/execute")
+@router.post("/{run_id}/execute", response_model=MigrationExecuteResponse)
 def execute_migration(run_id: int, db: Session = Depends(get_db)):
     """Execute migration — create staging tables and load CSV data."""
     result = execution_service.execute_migration(db, run_id)
@@ -21,7 +22,7 @@ def execute_migration(run_id: int, db: Session = Depends(get_db)):
     return result
 
 
-@router.post("/{run_id}/evaluate")
+@router.post("/{run_id}/evaluate", response_model=EvaluationResponse)
 def evaluate_migration(run_id: int, db: Session = Depends(get_db)):
     """Evaluate data quality of migrated data vs source CSVs."""
     result = evaluation_service.evaluate_run(db, run_id)

backend/api/routes/health.py

@@ -1,6 +1,7 @@
 """Health-check route for the Migration Evaluation Platform (MEP)."""
 from fastapi import APIRouter
 
+from api.schemas import HealthResponse
 from config import settings
 from database.connection import check_db_connection
 
@@ -10,17 +11,17 @@
 VERSION = "0.1.0"
 
 
-@router.get("/health")
-def health() -> dict:
+@router.get("/health", response_model=HealthResponse)
+def health() -> HealthResponse:
     """Report service health.
 
     Always returns HTTP 200. If the database is unreachable, the response still
     succeeds but reports ``"database": "disconnected"``.
     """
     db_connected = check_db_connection()
-    return {
-        "status": "healthy",
-        "version": VERSION,
-        "environment": settings.APP_ENV,
-        "database": "connected" if db_connected else "disconnected",
-    }
+    return HealthResponse(
+        status="healthy",
+        version=VERSION,
+        environment=settings.APP_ENV,
+        database="connected" if db_connected else "disconnected",
+    )

backend/api/routes/reports.py

@@ -7,13 +7,17 @@
 from fastapi.responses import FileResponse
 from sqlalchemy.orm import Session
 
+from api.schemas import ReportResponse
 from database.connection import get_db
-from services import report_service, schema_service, execution_service, evaluation_service
+from services import report_service, schema_service, evaluation_service
 
 router = APIRouter(prefix="/reports", tags=["reports"])
 
 
-@router.post("/{run_id}/generate")
+# ``response_model=ReportResponse`` intentionally filters out the internal
+# ``path`` key returned by report_service so server filesystem paths are not
+# leaked to API clients (the frontend only consumes ``download_url``).
+@router.post("/{run_id}/generate", response_model=ReportResponse)
 def generate_report(
     run_id: int,
     format: str = Query("json", pattern="^(json|html)$"),

backend/api/schemas.py

@@ -10,6 +10,24 @@
 from pydantic import BaseModel, Field
 
 
+# ---------------------------------------------------------------------------
+# Service Meta (root & health)
+# ---------------------------------------------------------------------------
+
+class RootResponse(BaseModel):
+    """Root route payload confirming the API is running."""
+    message: str
+    version: str
+
+
+class HealthResponse(BaseModel):
+    """Health-check payload."""
+    status: str = Field(..., examples=["healthy"])
+    version: str
+    environment: str
+    database: str = Field(..., examples=["connected", "disconnected"])
+
+
 # ---------------------------------------------------------------------------
 # Migration Run
 # ---------------------------------------------------------------------------

backend/config.py

@@ -3,6 +3,7 @@
 Settings are loaded from environment variables and/or a local ``.env`` file
 using pydantic-settings.
 """
+from pydantic import model_validator
 from pydantic_settings import BaseSettings, SettingsConfigDict
 
 
@@ -19,6 +20,31 @@ class Settings(BaseSettings):
     # Enable debug behaviour (verbose logging, etc.)
     DEBUG: bool = True
 
+    # Allow the application to auto-create database tables on startup via
+    # ``Base.metadata.create_all``. This is a development convenience only —
+    # production schema changes MUST go through explicit, reviewed migrations
+    # (e.g. Alembic). Startup fails fast if this is enabled in production.
+    ALLOW_SCHEMA_AUTO_CREATE: bool = True
+
+    @model_validator(mode="after")
+    def _forbid_auto_create_in_production(self) -> "Settings":
+        """Fail fast when schema auto-creation is enabled in production.
+
+        Auto-creating tables in production can silently mutate the schema,
+        mask missing migrations, and cause data loss on model drift. We treat
+        this combination as a fatal misconfiguration so the service refuses to
+        start rather than running unsafely.
+        """
+        if self.APP_ENV.strip().lower() == "production" and self.ALLOW_SCHEMA_AUTO_CREATE:
+            raise ValueError(
+                "Unsafe configuration: ALLOW_SCHEMA_AUTO_CREATE must be false when "
+                "APP_ENV=production. Schema changes in production must be applied "
+                "through explicit migrations, not automatic table creation. "
+                "Set ALLOW_SCHEMA_AUTO_CREATE=false (and use a migration tool) "
+                "to start the service in production."
+            )
+        return self
+
     model_config = SettingsConfigDict(
         env_file=".env",
         env_file_encoding="utf-8",

backend/main.py

@@ -11,6 +11,7 @@
 from fastapi.middleware.cors import CORSMiddleware
 
 from api.routes import health, migrations, validation, execution, reports, dashboard
+from api.schemas import RootResponse
 from config import settings
 from database.connection import check_db_connection, engine
 from database.models import Base
@@ -47,14 +48,19 @@ def on_startup() -> None:
     logger.info("Environment: %s | Debug: %s", settings.APP_ENV, settings.DEBUG)
     if check_db_connection():
         logger.info("Database connection: OK")
-        # Auto-create tables in dev (use Alembic migrations in production)
-        Base.metadata.create_all(bind=engine)
-        logger.info("Database tables ensured")
+        if settings.ALLOW_SCHEMA_AUTO_CREATE:
+            # Auto-create tables in dev only. Production must use explicit
+            # migrations; config.Settings refuses to start with this flag
+            # enabled when APP_ENV=production.
+            Base.metadata.create_all(bind=engine)
+            logger.info("Database tables ensured (schema auto-create enabled)")
+        else:
+            logger.info("Schema auto-create disabled; expecting managed migrations")
     else:
         logger.warning("Database connection: UNAVAILABLE")
 
 
-@app.get("/")
-def root() -> dict:
+@app.get("/", response_model=RootResponse)
+def root() -> RootResponse:
     """Root route confirming the API is running."""
-    return {"message": "MEP API is running", "version": VERSION}
+    return RootResponse(message="MEP API is running", version=VERSION)

backend/tests/test_config_safety.py

@@ -0,0 +1,62 @@
+"""Tests for production configuration safety gates in ``config.Settings``.
+
+The key invariant: the application must REFUSE to start when
+``APP_ENV=production`` and ``ALLOW_SCHEMA_AUTO_CREATE=true``, because schema
+auto-creation in production can silently mutate the database schema.
+"""
+import pytest
+from pydantic import ValidationError
+
+from config import Settings
+
+
+def make_settings(**overrides):
+    """Build Settings isolated from any local .env file."""
+    return Settings(_env_file=None, **overrides)
+
+
+class TestProductionSafetyGate:
+    def test_production_with_auto_create_fails(self):
+        """production + auto-create enabled must raise at construction time."""
+        with pytest.raises(ValidationError) as exc_info:
+            make_settings(APP_ENV="production", ALLOW_SCHEMA_AUTO_CREATE=True)
+        assert "ALLOW_SCHEMA_AUTO_CREATE" in str(exc_info.value)
+
+    def test_production_case_insensitive(self):
+        """The gate must not be bypassable via casing or whitespace."""
+        for env in ("PRODUCTION", "Production", " production "):
+            with pytest.raises(ValidationError):
+                make_settings(APP_ENV=env, ALLOW_SCHEMA_AUTO_CREATE=True)
+
+    def test_production_with_auto_create_disabled_ok(self):
+        """production + auto-create disabled is a valid configuration."""
+        s = make_settings(APP_ENV="production", ALLOW_SCHEMA_AUTO_CREATE=False)
+        assert s.APP_ENV == "production"
+        assert s.ALLOW_SCHEMA_AUTO_CREATE is False
+
+    def test_development_with_auto_create_ok(self):
+        """development keeps the convenient auto-create default."""
+        s = make_settings(APP_ENV="development", ALLOW_SCHEMA_AUTO_CREATE=True)
+        assert s.ALLOW_SCHEMA_AUTO_CREATE is True
+
+    def test_staging_with_auto_create_ok(self):
+        """staging is not gated (only production is fatal)."""
+        s = make_settings(APP_ENV="staging", ALLOW_SCHEMA_AUTO_CREATE=True)
+        assert s.ALLOW_SCHEMA_AUTO_CREATE is True
+
+    def test_default_is_development_with_auto_create(self):
+        """Defaults stay developer-friendly."""
+        s = make_settings()
+        assert s.APP_ENV == "development"
+        assert s.ALLOW_SCHEMA_AUTO_CREATE is True
+
+    def test_env_variable_string_coercion(self, monkeypatch):
+        """Values coming from environment variables are honoured too."""
+        monkeypatch.setenv("APP_ENV", "production")
+        monkeypatch.setenv("ALLOW_SCHEMA_AUTO_CREATE", "true")
+        with pytest.raises(ValidationError):
+            Settings(_env_file=None)
+
+        monkeypatch.setenv("ALLOW_SCHEMA_AUTO_CREATE", "false")
+        s = Settings(_env_file=None)
+        assert s.ALLOW_SCHEMA_AUTO_CREATE is False

docs/security/Rationale.md

@@ -0,0 +1,84 @@
+# Security Suppression Rationale
+
+This document records every linter/security-scanner suppression (`# nosec`,
+`# noqa`, `shellcheck disable`, etc.) in the codebase, with a technical
+justification for why the finding is a false positive or an accepted risk.
+
+**Policy:** a suppression comment is only acceptable when it is accompanied by
+an entry in this file. PRs that add a new suppression must update this document
+(see the PR template checklist).
+
+---
+
+## Active Suppressions
+
+### 1. `backend/database/connection.py` — `# noqa: BLE001` (broad exception in health check)
+
+```python
+except Exception as exc:  # noqa: BLE001 - health check must never raise
+```
+
+- **Rule suppressed:** BLE001 (blind `except Exception`).
+- **Justification:** `check_db_connection()` is used by the `/api/health`
+  endpoint and the startup hook. Its contract is *"return False on any
+  failure, never raise"*. The set of exceptions SQLAlchemy/psycopg2 can raise
+  during a connection attempt is broad and driver-dependent (OperationalError,
+  DBAPIError, socket/DNS errors, ...). Catching a narrower set risks the
+  health endpoint returning HTTP 500 instead of a controlled
+  `"database": "disconnected"` payload.
+- **Risk accepted:** silently swallowing an unexpected programming error in
+  this small, side-effect-free function. Mitigated by the failure being
+  surfaced in the health payload and startup logs.
+
+### 2. `backend/migration/evals/runner.py` — `# nosec B608` (SQL string composition)
+
+```python
+# runner.py:309 (inside _count_dev_rows)
+query = 'SELECT count(*) FROM te_dev."' + tbl + '";'  # nosec B608
+```
+
+- **Rule suppressed:** Bandit B608 (possible SQL injection via string
+  concatenation).
+- **Justification:** `tbl` is not user input. It iterates over the hardcoded
+  `_DEV_SEED_TABLES` constant defined in the same module; no external or
+  request-derived value can reach this string. Table names cannot be bound as
+  query parameters in PostgreSQL, so string composition is the only option
+  here, and the identifier is double-quoted against a fixed allowlist.
+- **Risk accepted:** none in the current call graph. If the eval runner is
+  ever refactored to accept table names from configuration or user input,
+  this suppression MUST be revisited and the identifier quoted/validated
+  (e.g. `psycopg2.sql.Identifier`).
+
+### 3. `backend/migration/evals/runner.py` — `# noqa: BLE001` (broad exception in eval loop)
+
+- **Rule suppressed:** BLE001 (blind `except Exception`).
+- **Justification:** the eval runner executes a batch of independent eval
+  cases; one case failing for any reason (bad fixture, DB hiccup, assertion
+  bug) must be recorded as a failed case rather than aborting the whole run.
+  The exception is captured into the eval report, not discarded.
+- **Risk accepted:** an unexpected error is downgraded to a failed eval case.
+  Acceptable for a developer-facing tool; the full traceback is preserved in
+  the report output.
+
+---
+
+## Adding a New Suppression (template)
+
+Copy this template into the "Active Suppressions" section:
+
+```markdown
+### N. `<file>:<line>` — `<suppression comment>` (<short description>)
+
+- **Rule suppressed:** <rule id and meaning>.
+- **Justification:** <why the finding is a false positive, or why the pattern
+  is required here>.
+- **Risk accepted:** <what residual risk remains and how it is mitigated>.
+```
+
+Guidelines:
+
+1. Prefer fixing the finding over suppressing it.
+2. Scope suppressions to a single line — never file-wide or rule-wide
+   disables.
+3. Include the rule ID in the comment (`# noqa: BLE001`, not bare `# noqa`).
+4. Re-review entries here whenever the suppressed code changes.