Gap remediation pass O-01..O-12 and ADR-0001

.github/workflows/quality-gate.yml

@@ -41,6 +41,10 @@ jobs:
       - name: Component health check
         run: make health
 
+      # Tier X (cross-engine) is deliberately absent: multi-engine support is
+      # deferred by ADR-0001 (PostgreSQL first), so this pipeline is PostgreSQL
+      # only. The scenario still exists and passes — run it with `make
+      # evals-engines`. Re-add `,x` here when multi-engine work resumes.
       - name: Evals — Tier P (offline validator scenarios)
         run: python3 evals/runner.py --tiers p --verbose
 
@@ -138,8 +142,12 @@ jobs:
             fi
           done
 
-      - name: Evals — Tiers P, I, S
-        run: python3 evals/runner.py --tiers p,i,s --verbose
+      # Tier E deploys all four environments and compares their schemas, so it
+      # runs here where the postgres service exists. Order matters: E deploys
+      # every environment, which the cross-env parity pytest markers then rely on.
+      # Tier X is excluded per ADR-0001 — see the free-tier job's note.
+      - name: Evals — Tiers P, I, S, E
+        run: python3 evals/runner.py --tiers p,i,s,e --verbose
 
       - name: Deploy all environments (for cross-env parity)
         run: |
@@ -150,7 +158,11 @@ jobs:
       # Prints a final block accounting for every test: PASSED / FAILED /
       # ERROR / SKIPPED (with reasons) / NOT RUN (deselected). --strict fails
       # the build on any skip.
+      # The API reads PGDATABASE (api/config.py). The schema lives in
+      # te_mgmt_dev, not the job-level default "postgres" (issue #45).
       - name: Full test suite — final result with skip accounting
+        env:
+          PGDATABASE: te_mgmt_dev
         run: python3 scripts/test_report.py --strict
 
       - name: Upload eval reports

Makefile

@@ -19,7 +19,7 @@ test-gate:
 # Evals: Tier P (offline) + Tier I/S (skip cleanly if no PostgreSQL).
 # Mirrors: bun run test:evals
 test-evals:
-	python3 evals/runner.py --tiers p,i,s --verbose
+	python3 evals/runner.py --tiers p,i,s,e --verbose
 
 # E2E: full pipeline tests that require a running PostgreSQL instance.
 # Mirrors: bun run test:e2e
@@ -30,7 +30,7 @@ test-e2e:
 # Mirrors: bun run test:evals:all
 test-all:
 	pytest tests/ --cov=build/csv --cov=evals --cov-report=term-missing
-	python3 evals/runner.py --tiers p,i,s --verbose
+	python3 evals/runner.py --tiers p,i,s,e --verbose
 
 # ── Code quality ──────────────────────────────────────────────────────────────
 
@@ -94,3 +94,11 @@ csv-demo:
 	bash build/csv_loader.sh build/csv/samples/orders.csv    --env $(or $(ENV),dev)
 	bash build/csv_loader.sh build/csv/samples/inventory.csv --env $(or $(ENV),dev)
 	bash build/csv_utilise.sh list --env $(or $(ENV),dev)
+
+# ── Cross-engine evals (opt-in) ───────────────────────────────────────────────
+# Multi-engine support is deferred by docs/adr/0001-postgresql-first.md, so Tier X
+# is not part of the default tiers or of CI. It needs no database service, so it
+# can be run on demand at any time.
+.PHONY: evals-engines
+evals-engines:
+	python3 evals/runner.py --tiers x --verbose

api/auth.py

@@ -1,4 +1,15 @@
-"""API key authentication dependency, applied globally in api/main.py."""
+"""API key authentication dependency.
+
+Applied per-router — see the ``dependencies=[Depends(require_api_key)]`` argument
+in ``api/routers/{csv,te,audit}_routes.py``. It is deliberately NOT applied at app
+level so ``/api/health`` stays reachable for uptime probes without the shared
+secret (BUG-035).
+
+Fail-closed behaviour: an empty ``settings.API_KEY`` leaves these routers open.
+That is tolerated only when ``API_ENV`` is ``dev``; any other value makes a
+missing key a startup failure in ``api/main.py`` lifespan (O-03), so this
+function can never silently wave traffic through in a deployed environment.
+"""
 
 import secrets
 

api/config.py

@@ -55,6 +55,24 @@ class Settings:
     # beyond localhost.
     API_KEY: str = os.environ.get("API_KEY", "")
 
+    # O-03: which deployment this process believes it is. Only "dev" tolerates an
+    # empty API_KEY; every other value makes a missing key a startup failure
+    # (see the fail-closed check in api/main.py lifespan). Keeping the default at
+    # "dev" means local workflows are unchanged, while any deployed environment
+    # must set both API_ENV and API_KEY or refuse to boot.
+    API_ENV: str = os.environ.get("API_ENV", "dev").strip().lower()
+
+    # O-03: per-client sliding-window rate limits, requests per minute.
+    # Writes are budgeted far lower than reads because an upload costs orders of
+    # magnitude more server work than a SELECT. Set either to 0 to disable that
+    # limit (documented escape hatch for load testing).
+    RATE_LIMIT_READS_PER_MINUTE: int = int(
+        os.environ.get("API_RATE_LIMIT_READS", "120")
+    )
+    RATE_LIMIT_WRITES_PER_MINUTE: int = int(
+        os.environ.get("API_RATE_LIMIT_WRITES", "20")
+    )
+
 
 settings = Settings()
 
@@ -73,4 +91,3 @@ class Settings:
     "defect_reports",
     "evidence_artifacts",
 ]
-

api/main.py

@@ -17,18 +17,39 @@
 
 from api import db
 from api.config import settings
+from api.ratelimit import rate_limit_middleware
 from api.routers import audit_routes, csv_routes, te_routes
 
 logger = logging.getLogger(__name__)
 
 
+def assert_auth_configured(api_env: str, api_key: str) -> None:
+    """Fail closed when a deployed environment has no API key (O-03).
+
+    Split out of ``lifespan`` so the rule can be unit-tested without a database:
+    ``lifespan`` also opens the connection pool, which would make every test of
+    this check an integration test.
+
+    Only ``dev`` may run unauthenticated. Raises RuntimeError otherwise.
+    """
+    if not api_key and api_env != "dev":
+        raise RuntimeError(
+            f"API_KEY is not set but API_ENV is '{api_env}'. "
+            "Refusing to start: every endpoint would be unauthenticated. "
+            "Set API_KEY, or set API_ENV=dev for local development."
+        )
+
+
 @asynccontextmanager
 async def lifespan(app: FastAPI):
+    # O-03: fail closed. A warning was not enough — an unset key left every
+    # router open, and a warning in a container log is easy to miss.
+    assert_auth_configured(settings.API_ENV, settings.API_KEY)
     if not settings.API_KEY:
         logger.warning(
             "API_KEY is not set - every endpoint is unauthenticated. "
-            "Set the API_KEY environment variable before deploying anywhere "
-            "reachable beyond localhost."
+            "Tolerated because API_ENV=dev. Set the API_KEY environment "
+            "variable before deploying anywhere reachable beyond localhost."
         )
     else:
         # Fingerprint (first 4 chars + length) so mismatches with the frontend's
@@ -68,6 +89,11 @@ async def lifespan(app: FastAPI):
     allow_headers=["Content-Type", "X-API-Key"],
 )
 
+# O-03: rate limiting. Registered after CORSMiddleware so it sits outside it in
+# Starlette's onion (last added = outermost) and a 429 still carries CORS headers,
+# which the browser needs in order to read the status at all.
+app.middleware("http")(rate_limit_middleware)
+
 app.include_router(csv_routes.router)
 app.include_router(te_routes.router)
 app.include_router(audit_routes.router)

api/ratelimit.py

@@ -0,0 +1,133 @@
+"""Dependency-free per-client sliding-window rate limiting (O-03).
+
+Why hand-rolled rather than slowapi
+-----------------------------------
+The API runs as a single Container App instance behind an environment boundary
+(``external_enabled = false``), so a shared Redis-backed limiter would add a
+dependency and a network hop for no extra protection. This module needs no
+third-party package, which also keeps the air-gapped install path simple.
+
+If the deployment ever scales past one instance, replace this with a
+Redis-backed limiter: per-instance counters stop being a global limit.
+
+Algorithm
+---------
+A sliding window per (client, scope). Each request appends ``now`` to a deque
+and drops entries older than ``window_seconds``; the request is refused when the
+deque is already at ``max_requests``. Memory is bounded by evicting idle client
+buckets once ``_MAX_TRACKED_CLIENTS`` is exceeded, so a spray of spoofed IPs
+cannot grow the table without limit.
+
+Mutating routes get a tighter budget than reads because an upload costs far more
+server work than a ``SELECT``.
+"""
+
+from __future__ import annotations
+
+import threading
+import time
+from collections import OrderedDict, deque
+
+from fastapi import Request
+from fastapi.responses import JSONResponse
+
+from api.config import settings
+
+# Hard cap on distinct client buckets held in memory.
+_MAX_TRACKED_CLIENTS = 4096
+
+
+class SlidingWindowLimiter:
+    """Thread-safe sliding-window counter keyed by (client, scope)."""
+
+    def __init__(self) -> None:
+        self._buckets: OrderedDict[tuple[str, str], deque[float]] = OrderedDict()
+        self._lock = threading.Lock()
+
+    def check(self, client: str, scope: str, max_requests: int,
+              window_seconds: float, now: float | None = None) -> tuple[bool, float]:
+        """Record a hit and report whether it is allowed.
+
+        Returns ``(allowed, retry_after_seconds)``. ``retry_after_seconds`` is 0
+        when allowed. A non-positive ``max_requests`` disables the limit.
+        """
+        if max_requests <= 0:
+            return True, 0.0
+        stamp = time.monotonic() if now is None else now
+        cutoff = stamp - window_seconds
+        key = (client, scope)
+        with self._lock:
+            hits = self._buckets.get(key)
+            if hits is None:
+                hits = deque()
+                self._buckets[key] = hits
+            self._buckets.move_to_end(key)
+            while hits and hits[0] <= cutoff:
+                hits.popleft()
+            if len(hits) >= max_requests:
+                # Oldest hit in the window decides when a slot frees up.
+                retry_after = max(0.0, hits[0] + window_seconds - stamp)
+                return False, retry_after
+            hits.append(stamp)
+            # Evict least-recently-used idle buckets to bound memory.
+            while len(self._buckets) > _MAX_TRACKED_CLIENTS:
+                self._buckets.popitem(last=False)
+            return True, 0.0
+
+    def reset(self) -> None:
+        """Drop all state. Used by tests to isolate cases."""
+        with self._lock:
+            self._buckets.clear()
+
+
+limiter = SlidingWindowLimiter()
+
+# Never rate-limited: uptime probes must not be throttled, and the OpenAPI docs
+# are static. /api/health is already unauthenticated by design (BUG-035).
+_EXEMPT_PATHS = frozenset({"/api/health", "/openapi.json", "/docs", "/redoc"})
+
+
+def _client_id(request: Request) -> str:
+    """Identify the caller.
+
+    Prefers the API key (a deployed caller's stable identity) over the socket
+    address, because several callers can share one NAT address. Falls back to
+    the peer IP. ``X-Forwarded-For`` is deliberately ignored: it is
+    client-controlled, so trusting it would let anyone bypass the limit by
+    varying the header.
+    """
+    api_key = request.headers.get("x-api-key")
+    if api_key:
+        return f"key:{api_key[:8]}"
+    client = request.client
+    return f"ip:{client.host}" if client and client.host else "ip:unknown"
+
+
+async def rate_limit_middleware(request: Request, call_next):
+    """Apply the read or write budget, then hand off to the route."""
+    if request.url.path in _EXEMPT_PATHS:
+        return await call_next(request)
+
+    if request.method in ("POST", "PUT", "PATCH", "DELETE"):
+        scope, budget = "write", settings.RATE_LIMIT_WRITES_PER_MINUTE
+    else:
+        scope, budget = "read", settings.RATE_LIMIT_READS_PER_MINUTE
+
+    allowed, retry_after = limiter.check(
+        _client_id(request), scope, budget, 60.0
+    )
+    if not allowed:
+        # Integer seconds, minimum 1 — Retry-After: 0 tells a client to retry
+        # instantly, which would defeat the limit.
+        seconds = max(1, int(retry_after + 0.999))
+        return JSONResponse(
+            status_code=429,
+            content={
+                "detail": (
+                    f"Rate limit exceeded for {scope} requests "
+                    f"({budget}/minute). Retry in {seconds}s."
+                )
+            },
+            headers={"Retry-After": str(seconds)},
+        )
+    return await call_next(request)

api/routers/audit_routes.py

@@ -1,6 +1,11 @@
 """BUG-041: csv_uploads.audit_log was write-only (populated by delete_file in
 csv_routes.py, never read back). This router surfaces it as a paginated,
-API-key-guarded feed so the deletion trail is actually consultable."""
+API-key-guarded feed so the trail is actually consultable.
+
+O-04 update: the table is no longer delete-only. Successful uploads also write a
+row, through the shared writer in api/services/audit.py, so this feed now covers
+creations as well as destructions. Integration coverage lives in
+tests/test_api_integration.py::AuditLogTrail."""
 
 from __future__ import annotations
 

api/routers/csv_routes.py

@@ -11,6 +11,7 @@
 from api.auth import require_api_key
 from api.config import settings
 from api.db import Conn
+from api.services.audit import coerce_file_id, record_audit, write_audit
 from api.services.csv_parse import build_preview
 from api.services.dynamic_loader import upload_dynamic
 from api.services.te_loader import match_te_table, upload_te
@@ -72,10 +73,25 @@ def upload(req: UploadRequest) -> dict:
     if req.mode == "te":
         if not req.targetTable:
             raise HTTPException(422, "targetTable is required when mode is 'te'")
-        return upload_te(req.fileName, req.content, req.targetTable)
-    if req.mode != "dynamic":
+        result = upload_te(req.fileName, req.content, req.targetTable)
+    elif req.mode == "dynamic":
+        result = upload_dynamic(req.fileName, req.content, req.types, req.overwrite)
+    else:
         raise HTTPException(422, f"Unknown mode '{req.mode}' — use 'dynamic' or 'te'")
-    return upload_dynamic(req.fileName, req.content, req.types, req.overwrite)
+
+    # O-04: record the load so audit_log covers creations, not just deletions.
+    # Only a genuine load is audited: "duplicate_file" and "error" outcomes
+    # changed nothing, so logging them would make the trail read as if data had
+    # landed when it had not.
+    if result.get("status") == "ok":
+        record_audit(
+            action="upload",
+            file_id=coerce_file_id(result.get("fileId")),
+            file_name=req.fileName,
+            table_name=result.get("tableName"),
+            mode=req.mode,
+        )
+    return result
 
 
 @router.get("/files")
@@ -176,14 +192,9 @@ def delete_file(file_id: int) -> dict:
                 ),
                 (file_id,),
             )
-            # Persist audit record so deletions are traceable after the table is gone.
-            cur.execute(
-                sql.SQL(
-                    "INSERT INTO {}.audit_log "
-                    "(action, file_id, file_name, table_name, mode) "
-                    "VALUES ('delete', %s, %s, %s, %s)"
-                ).format(sql.Identifier(settings.UPLOADS_SCHEMA)),
-                (file_id, file_name, table_name, mode),
-            )
+            # Persist audit record so deletions are traceable after the table is
+            # gone. Shares the single writer in api/services/audit.py (O-04), and
+            # rides this transaction so the row and the delete commit together.
+            write_audit(cur, "delete", file_id, file_name, table_name, mode)
             conn.commit()
             return {"status": "ok", "deleted": file_id}