From e0339cb8716f8f7f19790aa4824287443bd3e547 Mon Sep 17 00:00:00 2001 From: amar-python Date: Thu, 10 Sep 2026 13:39:22 +1000 Subject: [PATCH 1/8] feat(api): fail closed without an API key; add rate limiting and upload audit An unset API_KEY previously left every router open behind nothing but a startup warning, which is easy to miss in a container log. api/main.py now calls assert_auth_configured() and refuses to boot when API_ENV is anything other than 'dev' and no key is set. The guard is a pure function so it can be unit-tested without opening a connection pool. Rate limiting is a hand-rolled sliding window rather than slowapi: the API runs as a single instance behind an external_enabled=false boundary, so a Redis-backed limiter would add a dependency and a network hop for no extra protection, and avoiding the package keeps the air-gapped install path simple. Replace it if this ever scales past one instance -- per-instance counters stop being a global limit. Writes get a tighter budget than reads because an upload costs far more server work than a SELECT. X-Forwarded-For is deliberately ignored, since trusting a client-supplied header would let anyone mint a fresh bucket on demand. csv_uploads.audit_log had exactly one writer (delete_file), so it recorded destructions but never creations -- an auditor could see that a table had been dropped and never see who loaded it. Uploads now write through a shared writer in api/services/audit.py. Known limitation, documented in that module: the loaders own their own transactions, so an upload's audit row lands after the load has already committed. A failed audit write therefore logs at ERROR rather than failing a load that actually succeeded; reporting failure for committed data would be worse than a missing log line. tests/conftest.py disables both budgets for the suite, mirroring how it already bypasses require_api_key. Left enabled, the write budget would start returning 429 partway through upload-heavy modules and fail tests for reasons unrelated to what they assert. api/auth.py's docstring claimed it was applied globally in api/main.py. Enforcement was real but per-router; only the documentation was wrong. Corrected. 24 tests in tests/test_api_hardening.py cover both concerns, which is why they land in one commit. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01V1v7ow7NxH9y1ELUyGgACn --- api/auth.py | 13 ++- api/config.py | 19 ++- api/main.py | 30 ++++- api/ratelimit.py | 133 +++++++++++++++++++++ api/routers/csv_routes.py | 35 ++++-- api/services/audit.py | 95 +++++++++++++++ tests/conftest.py | 22 ++++ tests/test_api_hardening.py | 222 ++++++++++++++++++++++++++++++++++++ 8 files changed, 553 insertions(+), 16 deletions(-) create mode 100644 api/ratelimit.py create mode 100644 api/services/audit.py create mode 100644 tests/test_api_hardening.py diff --git a/api/auth.py b/api/auth.py index d00bce8..71dccef 100644 --- a/api/auth.py +++ b/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 diff --git a/api/config.py b/api/config.py index 0e07680..b0fbf74 100644 --- a/api/config.py +++ b/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", ] - diff --git a/api/main.py b/api/main.py index 2281578..56a3301 100644 --- a/api/main.py +++ b/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) diff --git a/api/ratelimit.py b/api/ratelimit.py new file mode 100644 index 0000000..d1c4876 --- /dev/null +++ b/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) diff --git a/api/routers/csv_routes.py b/api/routers/csv_routes.py index eed3e37..baa4d86 100644 --- a/api/routers/csv_routes.py +++ b/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} diff --git a/api/services/audit.py b/api/services/audit.py new file mode 100644 index 0000000..f1d132f --- /dev/null +++ b/api/services/audit.py @@ -0,0 +1,95 @@ +"""Shared writer for ``csv_uploads.audit_log`` (O-04). + +Before this module the audit table had exactly one writer — ``delete_file`` in +``api/routers/csv_routes.py`` — so the log recorded destructions but not +creations. An auditor reading it could see that a table had been deleted and +never see who loaded it. Every mutating action now writes a row through here. + +Known limitation (deliberate, documented) +----------------------------------------- +The loaders in ``api/services/{dynamic,te}_loader.py`` own and commit their own +transactions, so the audit row for an upload is written *after* the load has +already committed rather than inside the same transaction. A crash in the narrow +window between the two leaves a load with no audit row. + +Because of that ordering, a failure to write the audit row must NOT fail the +request: the data is already in the database, and reporting failure for a load +that actually succeeded would be worse than a missing log line. The failure is +logged at ERROR so it is alertable. + +Closing that window properly means threading the caller's connection into the +loaders so the audit INSERT joins their transaction — worth doing, but it changes +the loader signatures and is tracked separately rather than smuggled in here. +""" + +from __future__ import annotations + +import logging + +from psycopg2 import sql + +from api.config import settings +from api.db import Conn + +logger = logging.getLogger(__name__) + +# Actions the log is allowed to record. A typo becomes a loud failure here rather +# than an unqueryable string in the table. +VALID_ACTIONS = frozenset({"upload", "delete"}) + + +def write_audit(cur, action: str, file_id: int | None, file_name: str | None, + table_name: str | None, mode: str | None) -> None: + """Insert one audit row using the caller's cursor. + + Use this when you already hold a transaction and want the audit row to + commit atomically with the change it describes. The caller commits. + """ + if action not in VALID_ACTIONS: + raise ValueError(f"Unknown audit action {action!r}; expected one of {sorted(VALID_ACTIONS)}") + cur.execute( + sql.SQL( + "INSERT INTO {}.audit_log " + "(action, file_id, file_name, table_name, mode) " + "VALUES (%s, %s, %s, %s, %s)" + ).format(sql.Identifier(settings.UPLOADS_SCHEMA)), + (action, file_id, file_name, table_name, mode), + ) + + +def record_audit(action: str, file_id: int | None, file_name: str | None, + table_name: str | None, mode: str | None) -> bool: + """Insert one audit row in its own transaction. Never raises. + + Returns True when the row was written. Used on the upload path, where the + load has already committed — see the module docstring for why a failure here + is logged rather than propagated. + """ + try: + with Conn() as conn: + with conn.cursor() as cur: + write_audit(cur, action, file_id, file_name, table_name, mode) + conn.commit() + return True + except Exception as exc: # noqa: BLE001 — audit must not break a committed load + logger.error( + "Failed to write audit row (action=%s file_id=%s file_name=%r): %s", + action, file_id, file_name, exc, + ) + return False + + +def coerce_file_id(raw: object) -> int | None: + """Best-effort conversion of a loader's ``fileId`` to the BIGINT column type. + + The loaders return it as a string. A malformed value must not cost us the + whole audit row, so this yields None and lets the row record the action with + a null id rather than raising. + """ + if raw is None: + return None + try: + return int(raw) + except (TypeError, ValueError): + logger.warning("Unparseable fileId %r in audit call; recording NULL", raw) + return None diff --git a/tests/conftest.py b/tests/conftest.py index 6406033..073df61 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -20,6 +20,28 @@ app.dependency_overrides[require_api_key] = lambda: None +# O-03: the rate-limit middleware is real for every TestClient request, and all +# tests share one bucket (TestClient always presents the same client identity). +# Left enabled, the write budget would start returning 429 partway through +# upload-heavy modules and fail tests for reasons unrelated to what they assert. +# Disable both budgets for the session — the same stance taken above for auth — +# and let tests that specifically exercise limiting set their own budgets. +from api.config import settings as _settings # noqa: E402 +from api.ratelimit import limiter as _limiter # noqa: E402 + + +_settings.RATE_LIMIT_READS_PER_MINUTE = 0 +_settings.RATE_LIMIT_WRITES_PER_MINUTE = 0 + + +@pytest.fixture(autouse=True) +def _reset_rate_limiter(): + """Clear limiter buckets so one test's request history cannot fail another.""" + _limiter.reset() + yield + _limiter.reset() + + # Env vars tests are allowed to mutate; restored after every test. _RESTORE_KEYS = ( "CSV_FILE", diff --git a/tests/test_api_hardening.py b/tests/test_api_hardening.py new file mode 100644 index 0000000..411b601 --- /dev/null +++ b/tests/test_api_hardening.py @@ -0,0 +1,222 @@ +"""Tests for the O-03 (auth fail-closed, rate limiting) and O-04 (audit) work. + +All tests here are `unit`: no database, no network. The rate limiter is exercised +directly where possible and through the middleware where the HTTP contract +matters (status code, Retry-After header, exempt paths). +""" + +from __future__ import annotations + +import unittest + +import pytest +from fastapi.testclient import TestClient + +from api.config import settings +from api.main import app, assert_auth_configured +from api.ratelimit import SlidingWindowLimiter, limiter +from api.services.audit import VALID_ACTIONS, coerce_file_id, write_audit + +client = TestClient(app) + + +@pytest.mark.unit +class AuthFailsClosed(unittest.TestCase): + """O-03: a deployed environment must not boot without an API key.""" + + def test_dev_may_run_without_a_key(self): + # The local workflow must keep working — this is why the gate keys on + # API_ENV rather than simply requiring a key everywhere. + assert_auth_configured("dev", "") + + def test_any_key_is_accepted_in_dev(self): + assert_auth_configured("dev", "s3cret") + + def test_prod_without_a_key_refuses_to_start(self): + with self.assertRaises(RuntimeError) as ctx: + assert_auth_configured("prod", "") + # The message must name both the cause and the two ways out, because it + # surfaces in a container log where nobody can ask a follow-up question. + msg = str(ctx.exception) + self.assertIn("API_KEY", msg) + self.assertIn("prod", msg) + self.assertIn("API_ENV=dev", msg) + + def test_staging_and_test_also_refuse(self): + for env in ("test", "staging", "production", "azure"): + with self.subTest(env=env): + with self.assertRaises(RuntimeError): + assert_auth_configured(env, "") + + def test_deployed_env_with_a_key_starts(self): + assert_auth_configured("prod", "a-real-key") + + +@pytest.mark.unit +class RateLimiterAlgorithm(unittest.TestCase): + """O-03: the sliding window itself, driven with an injected clock.""" + + def setUp(self): + self.lim = SlidingWindowLimiter() + + def test_requests_under_budget_are_allowed(self): + for i in range(5): + allowed, retry = self.lim.check("c", "read", 5, 60.0, now=100.0 + i) + self.assertTrue(allowed, f"request {i} should be allowed") + self.assertEqual(retry, 0.0) + + def test_request_over_budget_is_refused(self): + for i in range(3): + self.lim.check("c", "read", 3, 60.0, now=100.0 + i) + allowed, retry = self.lim.check("c", "read", 3, 60.0, now=103.0) + self.assertFalse(allowed) + self.assertGreater(retry, 0.0) + + def test_retry_after_points_at_the_oldest_hit_leaving_the_window(self): + # Oldest hit at t=100 with a 60s window frees a slot at t=160. + for i in range(2): + self.lim.check("c", "read", 2, 60.0, now=100.0 + i) + _, retry = self.lim.check("c", "read", 2, 60.0, now=110.0) + self.assertAlmostEqual(retry, 50.0, places=6) + + def test_window_expiry_frees_the_budget(self): + for i in range(2): + self.lim.check("c", "read", 2, 60.0, now=100.0 + i) + blocked, _ = self.lim.check("c", "read", 2, 60.0, now=120.0) + self.assertFalse(blocked) + # Past t=161 both original hits have aged out. + allowed, _ = self.lim.check("c", "read", 2, 60.0, now=162.0) + self.assertTrue(allowed) + + def test_zero_budget_disables_the_limit(self): + for i in range(50): + allowed, _ = self.lim.check("c", "read", 0, 60.0, now=100.0 + i) + self.assertTrue(allowed) + + def test_read_and_write_scopes_are_independent(self): + for i in range(2): + self.lim.check("c", "write", 2, 60.0, now=100.0 + i) + refused, _ = self.lim.check("c", "write", 2, 60.0, now=102.0) + self.assertFalse(refused) + # A spent write budget must not block reads. + allowed, _ = self.lim.check("c", "read", 2, 60.0, now=102.0) + self.assertTrue(allowed) + + def test_clients_are_independent(self): + for i in range(2): + self.lim.check("noisy", "read", 2, 60.0, now=100.0 + i) + refused, _ = self.lim.check("noisy", "read", 2, 60.0, now=102.0) + self.assertFalse(refused) + allowed, _ = self.lim.check("quiet", "read", 2, 60.0, now=102.0) + self.assertTrue(allowed, "one client's traffic must not limit another's") + + def test_bucket_table_is_bounded(self): + # A spray of spoofed identities must not grow memory without limit. + from api.ratelimit import _MAX_TRACKED_CLIENTS + for i in range(_MAX_TRACKED_CLIENTS + 250): + self.lim.check(f"client-{i}", "read", 5, 60.0, now=100.0) + self.assertLessEqual(len(self.lim._buckets), _MAX_TRACKED_CLIENTS) + + def test_reset_clears_state(self): + self.lim.check("c", "read", 1, 60.0, now=100.0) + refused, _ = self.lim.check("c", "read", 1, 60.0, now=101.0) + self.assertFalse(refused) + self.lim.reset() + allowed, _ = self.lim.check("c", "read", 1, 60.0, now=102.0) + self.assertTrue(allowed) + + +@pytest.mark.unit +class RateLimitMiddleware(unittest.TestCase): + """O-03: the HTTP contract the frontend and any gateway will see.""" + + def setUp(self): + limiter.reset() + self._saved = ( + settings.RATE_LIMIT_READS_PER_MINUTE, + settings.RATE_LIMIT_WRITES_PER_MINUTE, + ) + + def tearDown(self): + (settings.RATE_LIMIT_READS_PER_MINUTE, + settings.RATE_LIMIT_WRITES_PER_MINUTE) = self._saved + limiter.reset() + + def test_exceeding_the_write_budget_returns_429_with_retry_after(self): + settings.RATE_LIMIT_WRITES_PER_MINUTE = 2 + payload = {"fileName": "x.csv", "content": "a,b\n1,2\n"} + # The budget counts requests, not successes: a 422/500 still consumes a + # slot, which is the point — it is the work that needs bounding. + for _ in range(2): + client.post("/api/csv/preview", json=payload) + resp = client.post("/api/csv/preview", json=payload) + self.assertEqual(resp.status_code, 429) + self.assertIn("Retry-After", resp.headers) + # Retry-After: 0 would tell the client to retry instantly. + self.assertGreaterEqual(int(resp.headers["Retry-After"]), 1) + + def test_health_is_never_rate_limited(self): + settings.RATE_LIMIT_READS_PER_MINUTE = 1 + # An uptime probe polls constantly; throttling it would create false alarms. + for _ in range(5): + self.assertNotEqual(client.get("/api/health").status_code, 429) + + def test_openapi_is_never_rate_limited(self): + settings.RATE_LIMIT_READS_PER_MINUTE = 1 + for _ in range(4): + self.assertNotEqual(client.get("/openapi.json").status_code, 429) + + def test_disabled_budget_allows_everything(self): + settings.RATE_LIMIT_WRITES_PER_MINUTE = 0 + payload = {"fileName": "x.csv", "content": "a,b\n1,2\n"} + for _ in range(25): + self.assertNotEqual( + client.post("/api/csv/preview", json=payload).status_code, 429 + ) + + def test_forwarded_for_cannot_be_used_to_evade_the_limit(self): + settings.RATE_LIMIT_WRITES_PER_MINUTE = 2 + payload = {"fileName": "x.csv", "content": "a,b\n1,2\n"} + for i in range(2): + client.post("/api/csv/preview", json=payload, + headers={"X-Forwarded-For": f"10.0.0.{i}"}) + # A client-supplied header must not mint a fresh bucket on demand. + resp = client.post("/api/csv/preview", json=payload, + headers={"X-Forwarded-For": "10.0.0.99"}) + self.assertEqual(resp.status_code, 429) + + +@pytest.mark.unit +class AuditWriterContract(unittest.TestCase): + """O-04: the shared audit writer's guardrails, without touching a database.""" + + def test_only_known_actions_are_accepted(self): + self.assertEqual(VALID_ACTIONS, {"upload", "delete"}) + + def test_unknown_action_raises_before_any_sql_runs(self): + class ExplodingCursor: + def execute(self, *a, **k): + raise AssertionError("execute must not be reached for a bad action") + + with self.assertRaises(ValueError): + write_audit(ExplodingCursor(), "drop_everything", 1, "f.csv", "t", "dynamic") + + def test_valid_action_reaches_the_cursor_with_bound_parameters(self): + captured = {} + + class RecordingCursor: + def execute(self, statement, params=None): + captured["params"] = params + + write_audit(RecordingCursor(), "upload", 7, "f.csv", "csv_abc", "dynamic") + self.assertEqual(captured["params"], ("upload", 7, "f.csv", "csv_abc", "dynamic")) + + def test_file_id_is_coerced_from_the_loaders_string_form(self): + self.assertEqual(coerce_file_id("42"), 42) + self.assertEqual(coerce_file_id(42), 42) + + def test_unparseable_file_id_degrades_to_null_rather_than_raising(self): + # Losing the id is survivable; losing the whole audit row is not. + self.assertIsNone(coerce_file_id("not-a-number")) + self.assertIsNone(coerce_file_id(None)) + self.assertIsNone(coerce_file_id({"unexpected": "shape"})) From 7b5a455cd3af9208400a5ffcd67887f2905031f9 Mon Sep 17 00:00:00 2001 From: amar-python Date: Thu, 10 Sep 2026 13:39:23 +1000 Subject: [PATCH 2/8] test: pin sanitize_columns output so the reserved-column ordering cannot regress test_sanitize_columns_reserved_stems_get_suffix asserted only that no output name appeared in the reserved set. That also passed under the bug the test is named for: when the reserved check ran after underscores were stripped, '_id' became 'id', never matched _RESERVED, and lost its '_col' suffix -- and 'id' is not in the reserved set either, so the loop stayed green. The test could not detect its own bug. Pins the exact output instead, and adds the case that makes the bug bite: ['_id', 'id'] both sanitised to 'id' under the old ordering, so the de-duplicator renamed one to 'id_1' -- two distinct source columns silently became near-identical names, with the winner decided by header order. Verified against a reproduction of the old ordering: the new assertions fail on it, the old loop still passes. The fix itself is already in main for both csv_parse.py and csv-preview.ts; the TypeScript test had a proper assertion and the Python one did not. Recovered from uncommitted work in .claude/worktrees/velvety-chasing-badger before that worktree was removed. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01V1v7ow7NxH9y1ELUyGgACn --- tests/test_api_unit.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/tests/test_api_unit.py b/tests/test_api_unit.py index 79083f0..b217142 100644 --- a/tests/test_api_unit.py +++ b/tests/test_api_unit.py @@ -70,6 +70,20 @@ def test_sanitize_columns_reserved_stems_get_suffix(self): for name in out: self.assertNotIn(name, {"_id", "_row_hash", "_created_at"}) + # The assertNotIn above is too weak to be a regression guard: it also + # passed under the bug this test is named for. The reserved check once + # ran *after* underscores were stripped, so "_id" became "id", never + # matched _RESERVED, and lost its "_col" suffix — and "id" is not in the + # reserved set either, so the loop above stayed green. Pin the exact + # output so the ordering cannot silently regress. + self.assertEqual(out, ["id_col", "row_hash_col", "created_at_col"]) + + # The case that makes the bug bite. Under the old ordering "_id" and + # "id" both sanitised to "id", so the de-duplicator renamed one to + # "id_1" — two distinct source columns silently became near-identical + # names, and which one won depended on header order. + self.assertEqual(sanitize_columns(["_id", "id"]), ["id_col", "id"]) + @pytest.mark.unit class RequestValidation(unittest.TestCase): From e121998e47daa01f5dd5bf96bf32bf2f0013f87f Mon Sep 17 00:00:00 2001 From: amar-python Date: Thu, 10 Sep 2026 13:39:24 +1000 Subject: [PATCH 3/8] feat(evals): add Tier E cross-environment parity and Tier X SQLite structure Tier E closes VCRM BR-01. Only Dev was ever structurally verified; Test, Staging and Prod were deployable but nothing asserted they matched. An edit to env_staging.sql adding a column would deploy with exit 0 and stay invisible until production data met a schema no test had compared. Scenario 01 deploys all four environments and reduces each to a table -> ordered column:type:nullability fingerprint, excluding the schema name so the one difference BR-01 permits compares equal. Reordered columns count as drift, because ordinal position is part of the deployed contract. Tier X partially addresses BR-02. It executes build/schema/sqlite/te_core_schema.sql in an in-memory database via stdlib sqlite3, so no sqlite3 binary is needed. It targets the DDL rather than adapter_sqlite.sh because the DDL is what drifts; the adapter is a thin sed wrapper around it. That leaves the adapter scripts unexercised -- a smaller, separate gap, recorded rather than glossed over. Tier X measured a shortfall nobody had counted: PostgreSQL defines 12 tables, SQLite and MariaDB 7, Teradata 1, while InfluxDB and Redis ship seed data with no DDL. The scenario pins SQLite's current 7-table baseline rather than asserting 12, so CI is never permanently red for known-outstanding work, and a job that is always red stops being read. It still fails on drift in either direction. Per ADR-0001 (next commit) Tier X is NOT wired into CI -- multi-engine support is deferred, so the pipeline stays PostgreSQL only. Run it with 'make evals-engines'. Tier E does run in the integration-postgres job. 19 unit tests in tests/test_evals_tier_e_x.py cover both tiers' comparison logic, with negative controls for added column, changed type, changed nullability, reordered columns, and missing or extra tables -- a parity check that silently returns 'no differences' for two different schemas is worse than no check. scripts/lint.sh linted a hardcoded file list, so new modules escaped flake8 and bandit entirely. The four new files are now inside the gate. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01V1v7ow7NxH9y1ELUyGgACn --- .github/workflows/quality-gate.yml | 12 +- Makefile | 12 +- .../NOTES.txt | 53 +++ .../01_sqlite_schema_structure/NOTES.txt | 65 ++++ .../01_envs_have_identical_structure.json | 11 + .../tier_x/01_sqlite_schema_structure.json | 27 ++ evals/runner.py | 367 ++++++++++++++++++ scripts/lint.sh | 4 + tests/test_evals_tier_e_x.py | 180 +++++++++ 9 files changed, 727 insertions(+), 4 deletions(-) create mode 100644 evals/datasets/tier_e/01_envs_have_identical_structure/NOTES.txt create mode 100644 evals/datasets/tier_x/01_sqlite_schema_structure/NOTES.txt create mode 100644 evals/expected/tier_e/01_envs_have_identical_structure.json create mode 100644 evals/expected/tier_x/01_sqlite_schema_structure.json create mode 100644 tests/test_evals_tier_e_x.py diff --git a/.github/workflows/quality-gate.yml b/.github/workflows/quality-gate.yml index 3c48374..9a8cf79 100644 --- a/.github/workflows/quality-gate.yml +++ b/.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: | diff --git a/Makefile b/Makefile index 2efe6c9..f256b58 100644 --- a/Makefile +++ b/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 diff --git a/evals/datasets/tier_e/01_envs_have_identical_structure/NOTES.txt b/evals/datasets/tier_e/01_envs_have_identical_structure/NOTES.txt new file mode 100644 index 0000000..2237999 --- /dev/null +++ b/evals/datasets/tier_e/01_envs_have_identical_structure/NOTES.txt @@ -0,0 +1,53 @@ +Tier E scenario 01 — cross-environment structural parity +======================================================== + +Requirement under test +---------------------- +VCRM BR-01: "The framework shall deploy isolated Dev / Test / Staging / Prod +environments on a single PostgreSQL instance, each with its own database, +schema, app user, and connection limit." + +Gap this closes +--------------- +Before this scenario, only Dev was ever structurally verified. Test, Staging and +Prod were deployable, but nothing asserted they matched Dev. A hand-edit to +env_staging.sql that added a column, changed a type, or reordered the DDL would +deploy with exit 0 and stay invisible until production data met a schema no test +had ever compared. VCRM_GAPS.md rated this Medium and scoped it at ~1 day. + +What it does +------------ +1. Deploys each of the four environments from build/environments/env_.sql, + falling back to env_.example.sql (the concrete files are gitignored and + materialised from the templates — see FIXES_APPLIED.md F3). +2. Reads information_schema.columns for each environment's own schema. +3. Reduces each to a fingerprint: table -> ordered list of + "column:data_type:is_nullable". The schema name is deliberately excluded so + two environments with different namespaces compare equal when their structure + matches — that namespace difference is the one difference BR-01 permits. +4. Compares Test, Staging and Prod against Dev. Dev is the reference because it + is the environment the other 142 SQL assertions actually exercise, so "drift" + means "differs from what the suite validates". + +Why it fails rather than skips +------------------------------ +If PostgreSQL is unreachable the scenario FAILS. That follows the no-skip stance +adopted in FIXES_APPLIED.md F5: a green run must mean the comparison really ran. +An eval that quietly skips is worse than no eval, because it reports confidence +it has not earned. + +Ordinal order is treated as drift +--------------------------------- +Two environments with the same column set in a different order fail. Ordinal +position is part of the deployed contract: anything doing SELECT * or positional +COPY would behave differently between the two. + +Prerequisites +------------- + bash scripts/provision_full_test_env.sh + # or, minimally, a reachable PostgreSQL with PG* env vars set and the four + # te_mgmt_ databases created (see the CI job in quality-gate.yml). + +Run +--- + python3 evals/runner.py --tiers e --verbose diff --git a/evals/datasets/tier_x/01_sqlite_schema_structure/NOTES.txt b/evals/datasets/tier_x/01_sqlite_schema_structure/NOTES.txt new file mode 100644 index 0000000..1a0960e --- /dev/null +++ b/evals/datasets/tier_x/01_sqlite_schema_structure/NOTES.txt @@ -0,0 +1,65 @@ +Tier X scenario 01 — SQLite schema structure +============================================ + +Requirement under test +---------------------- +VCRM BR-02: "The framework shall support six database engines — PostgreSQL, +MariaDB, SQLite, InfluxDB, Redis, Teradata — through adapter scripts." + +Why SQLite first +---------------- +It is the only engine that needs no service: embedded, file- or memory-based, so +the scenario runs anywhere the runner runs. VCRM_GAPS.md Gap 2 ranked it first +for exactly this reason. + +What it does +------------ +1. Reads build/schema/sqlite/te_core_schema.sql. +2. Substitutes the {{TBL_*}} placeholders the way adapter_sqlite.sh's sed pass + does, then asserts none were left behind — an unsubstituted token would reach + sqlite3 as a literal and quietly create a wrongly-named table. +3. Executes the DDL in an in-memory SQLite database via Python's stdlib sqlite3. + A DDL that SQLite rejects is itself a finding, and is reported as one. +4. Reads the resulting structure back from sqlite_master and PRAGMA table_info, + and compares it against the expectation file. + +Deliberate scope limits +----------------------- +DDL, not adapter. This executes the schema file rather than shelling out to +adapter_sqlite.sh. The DDL is the artifact that drifts; the adapter is a thin +sed + sqlite3 wrapper around it. Avoiding the adapter also avoids needing a +sqlite3 binary on PATH, which means this scenario runs on the Windows CI job +where GAP_ANALYSIS.md G2 blocks anything requiring a database service. The +adapter script itself therefore remains unexercised — a smaller, separate gap, +recorded here rather than glossed over. + +Table and column names, not types. SQLite has no native UUID or TIMESTAMPTZ and +stores both as TEXT, so comparing declared types against PostgreSQL would fire +on every column while telling us nothing. Structural presence is the property +BR-02 actually asserts. + +Known shortfall — this is a baseline guard, not a parity assertion +----------------------------------------------------------------- +SQLite ships 7 of the 12 T&E tables. Missing: temp_documents, test_phases, +test_events, test_results, evidence_artifacts. + +expected_tables therefore pins the 7 that exist today rather than the 12 that +full parity would require. The scenario passes on the known state and fails on +drift in either direction: + + - a table disappearing from the SQLite DDL, or + - a table being added without updating expected_tables and + known_missing_tables. + +That choice is deliberate. Asserting 12 would make this permanently red for work +that is knowingly outstanding, and a CI job that is always red stops being read. +The shortfall stays visible instead in known_missing_tables, in every generated +report's `actual` block, and in docs/ENGINE_PARITY_TODO.md. + +When the missing tables are written, move them from known_missing_tables into +expected_tables in the same commit — the scenario will fail until you do, which +is the reminder working as intended. + +Run +--- + python3 evals/runner.py --tiers x --verbose diff --git a/evals/expected/tier_e/01_envs_have_identical_structure.json b/evals/expected/tier_e/01_envs_have_identical_structure.json new file mode 100644 index 0000000..adb8904 --- /dev/null +++ b/evals/expected/tier_e/01_envs_have_identical_structure.json @@ -0,0 +1,11 @@ +{ + "scenario": "01_envs_have_identical_structure", + "description": "Deploy all four environments and assert their deployed schemas are structurally identical; only the database/schema namespace may differ. Closes VCRM BR-01.", + "envs_under_test": ["dev", "test", "staging", "prod"], + "reference_env": "dev", + "expected": { + "all_envs_deploy_exit_zero": true, + "column_signature_identical_across_envs": true, + "min_tables_per_env": 12 + } +} diff --git a/evals/expected/tier_x/01_sqlite_schema_structure.json b/evals/expected/tier_x/01_sqlite_schema_structure.json new file mode 100644 index 0000000..0d0f0c0 --- /dev/null +++ b/evals/expected/tier_x/01_sqlite_schema_structure.json @@ -0,0 +1,27 @@ +{ + "scenario": "01_sqlite_schema_structure", + "description": "Execute build/schema/sqlite/te_core_schema.sql in an in-memory SQLite database and pin its structure. Baseline regression guard for VCRM BR-02; passes on the 7 tables SQLite ships today and fails on any drift in either direction.", + "engine": "sqlite", + "reference_engine": "postgresql", + "expected": { + "expected_tables": [ + "defect_reports", + "organisations", + "personnel", + "requirements", + "test_cases", + "test_programs", + "vcrm_entries" + ], + "expected_columns": { + "organisations": ["country", "created_at", "is_active", "name", "org_id", "org_type", "updated_at"] + }, + "known_missing_tables": [ + "evidence_artifacts", + "temp_documents", + "test_events", + "test_phases", + "test_results" + ] + } +} diff --git a/evals/runner.py b/evals/runner.py index 2e330fa..c191a49 100644 --- a/evals/runner.py +++ b/evals/runner.py @@ -18,6 +18,7 @@ import csv import json import os +import re import shutil import subprocess import sys @@ -541,6 +542,370 @@ def _run_fresh_deploy_then_tests( return result +# --------------------------------------------------------------------------- +# Tier E — Cross-environment structural parity (VCRM BR-01) +# +# BR-01 claims Dev/Test/Staging/Prod are deployed from one parameterised schema +# and differ only in their identifier namespace. Nothing asserted that. An edit +# to env_staging.sql that added a column, or dropped a constraint, would deploy +# cleanly and no test would notice until production data met a schema nobody had +# compared. This tier compares the four deployed schemas column by column. +# +# Only the identifier namespace (database + schema name) may differ. Everything +# structural — table set, column names, declared types, nullability, ordinal +# position — must be identical. + +ENV_TARGETS = [ + ("dev", "te_mgmt_dev", "te_dev"), + ("test", "te_mgmt_test", "te_test"), + ("staging", "te_mgmt_staging", "te_staging"), + ("prod", "te_mgmt_prod", "te_prod"), +] + + +def _deploy_env(env_name: str) -> Dict[str, Any]: + """Deploy one environment from its committed launcher. Returns exit info. + + env_.sql is gitignored and materialised from env_.example.sql (see + FIXES_APPLIED.md F3), so accept either and say which was used. + """ + env_dir = PROJECT_ROOT / "build" / "environments" + launcher = env_dir / ("env_" + env_name + ".sql") + if not launcher.exists(): + launcher = env_dir / ("env_" + env_name + ".example.sql") + if not launcher.exists(): + return {"env": env_name, "exit_code": None, + "error": "No env_" + env_name + ".sql or .example.sql found"} + r = subprocess.run( + ["psql", "-v", "ON_ERROR_STOP=1", "-f", str(launcher)], + env=_pg_env(), capture_output=True, text=True, timeout=180, + ) + return { + "env": env_name, + "launcher": launcher.name, + "exit_code": r.returncode, + "stderr_tail": r.stderr[-300:], + } + + +def _column_signature(db: str, schema: str) -> Dict[str, List[str]]: + """Structural fingerprint of one deployed schema. + + Maps table name -> ordered list of "column:type:nullability" strings. The + schema name is deliberately absent from the value so two environments with + different namespaces compare equal when their structure matches. + """ + # The schema name is bound by psql via --set + :'sch' rather than pasted into + # the SQL text, so there is no string interpolation to reason about even + # though the value comes from the ENV_TARGETS constant. + query = ( + "SELECT table_name, column_name, data_type, is_nullable " + "FROM information_schema.columns " + "WHERE table_schema = :'sch' " + "ORDER BY table_name, ordinal_position" + ) + r = subprocess.run( + ["psql", "-tA", "-d", db, "--set=sch=" + schema, "-c", query], + env=_pg_env(), capture_output=True, text=True, timeout=30, + ) + if r.returncode != 0: + return {} + sig: Dict[str, List[str]] = {} + for line in r.stdout.splitlines(): + parts = line.split("|") + if len(parts) != 4: + continue + table, column, dtype, nullable = (p.strip() for p in parts) + sig.setdefault(table, []).append(column + ":" + dtype + ":" + nullable) + return sig + + +def _diff_signatures(ref_env: str, ref: Dict[str, List[str]], + other_env: str, other: Dict[str, List[str]]) -> List[str]: + """Human-readable differences between two schema fingerprints.""" + diffs: List[str] = [] + missing = sorted(set(ref) - set(other)) + extra = sorted(set(other) - set(ref)) + if missing: + diffs.append(other_env + " is missing tables present in " + ref_env + + ": " + ", ".join(missing)) + if extra: + diffs.append(other_env + " has tables absent from " + ref_env + + ": " + ", ".join(extra)) + for table in sorted(set(ref) & set(other)): + if ref[table] != other[table]: + ref_cols = set(ref[table]) + other_cols = set(other[table]) + only_ref = sorted(ref_cols - other_cols) + only_other = sorted(other_cols - ref_cols) + if only_ref or only_other: + diffs.append( + "table " + table + " differs: " + ref_env + "-only [" + + ", ".join(only_ref) + "], " + other_env + "-only [" + + ", ".join(only_other) + "]" + ) + else: + # Same columns, different order — still drift worth failing on, + # because ordinal position is part of the deployed contract. + diffs.append("table " + table + " has the same columns in a " + "different ordinal order between " + ref_env + + " and " + other_env) + return diffs + + +def run_tier_e_scenario(scenario_dir: Path) -> ScenarioResult: + name = scenario_dir.name + result = ScenarioResult(tier="e", name=name) + + expected = _load_expected("e", name) + if expected is None: + result.errors.append("No expected file at expected/tier_e/" + name + ".json") + return result + result.expected = expected + + if not _can_connect_pg(): + # Same no-skip stance as Tiers I and S (FIXES_APPLIED.md F5): a missing + # prerequisite fails, so a green run always means the check really ran. + result.errors.append( + "PostgreSQL not reachable via psql " + "(install psql + start PG, or set PG* env vars)." + ) + return result + + if name == "01_envs_have_identical_structure": + return _run_envs_have_identical_structure(result, expected) + + result.errors.append("Unknown tier-E scenario: " + name) + return result + + +def _run_envs_have_identical_structure( + result: ScenarioResult, expected: Dict[str, Any] +) -> ScenarioResult: + exp = expected.get("expected", {}) + envs_under_test = expected.get("envs_under_test", + [e[0] for e in ENV_TARGETS]) + targets = [t for t in ENV_TARGETS if t[0] in envs_under_test] + + deploys = [_deploy_env(env_name) for env_name, _, _ in targets] + signatures: Dict[str, Dict[str, List[str]]] = {} + for env_name, db, schema in targets: + signatures[env_name] = _column_signature(db, schema) + + table_counts = {e: len(sig) for e, sig in signatures.items()} + result.actual = { + "deploys": deploys, + "table_counts": table_counts, + } + + errors: List[str] = [] + + if exp.get("all_envs_deploy_exit_zero") is True: + for d in deploys: + if d.get("exit_code") != 0: + errors.append( + "deploy of " + d["env"] + " exited " + + str(d.get("exit_code")) + " " + + (d.get("error") or d.get("stderr_tail") or "") + ) + + min_tables = int(exp.get("min_tables_per_env", 12)) + for env_name, count in sorted(table_counts.items()): + if count < min_tables: + errors.append( + "env " + env_name + " has " + str(count) + " tables in its " + "schema, expected >= " + str(min_tables) + + " (deploy may have partially failed)" + ) + + if exp.get("column_signature_identical_across_envs") is True: + # dev is the reference: it is the environment every other test exercises, + # so drift is defined as "differs from what the suite actually validates". + ref_env = expected.get("reference_env", "dev") + ref = signatures.get(ref_env) + if not ref: + errors.append("reference env " + ref_env + " returned no columns; " + "cannot compare the others against it") + else: + for env_name in sorted(signatures): + if env_name == ref_env: + continue + diffs = _diff_signatures(ref_env, ref, env_name, + signatures[env_name]) + errors.extend(diffs) + result.actual["signature_match"] = not errors + + result.errors = errors + result.passed = not errors + return result + + +# --------------------------------------------------------------------------- +# Tier X — Cross-engine structural coverage (VCRM BR-02) +# +# BR-02 claims six engines are supported through adapter scripts. Only +# PostgreSQL was ever executed; the other five rested on code review. This tier +# starts closing that with SQLite, the cheapest engine to verify because it needs +# no service. +# +# Deliberate scope: this executes the DDL in build/schema/sqlite/, not +# adapter_sqlite.sh. Two reasons. The DDL is the artifact that drifts — the +# adapter is a thin sed + sqlite3 wrapper around it. And using Python's stdlib +# sqlite3 module means no sqlite3 binary is required, so this scenario also runs +# on the Windows CI job, where GAP_ANALYSIS.md G2 blocks anything needing a +# database service. The adapter script itself remains unexercised; that is a +# smaller, separate gap and is recorded rather than glossed over. +# +# The comparison is table-set and column-name only. Declared types are +# deliberately NOT compared across engines: SQLite has no native UUID or +# TIMESTAMPTZ and stores both as TEXT, so a type diff would fire constantly +# while telling us nothing. Structural presence is the property BR-02 asserts. + +_ENGINE_PLACEHOLDER_TABLES = [ + "organisations", "personnel", "test_programs", "temp_documents", + "test_phases", "requirements", "test_cases", "vcrm_entries", + "test_events", "test_results", "defect_reports", "evidence_artifacts", +] + + +def _substitute_placeholders(ddl: str) -> str: + """Replace {{TBL_*}} tokens the way adapter_sqlite.sh's sed pass does. + + Uses the table's own name as its value, which is what every environment file + does for the default deployment. + """ + for tbl in _ENGINE_PLACEHOLDER_TABLES: + ddl = ddl.replace("{{TBL_" + tbl.upper() + "}}", tbl) + return ddl + + +def _sqlite_structure(ddl: str) -> Dict[str, Any]: + """Execute DDL in an in-memory SQLite database and read back its structure. + + Returns {"tables": {name: [columns]}} or {"error": "..."} if the DDL is + rejected — a syntax error in the SQLite schema is itself a finding. + """ + import sqlite3 + try: + conn = sqlite3.connect(":memory:") + except Exception as exc: # noqa: BLE001 + return {"error": "cannot open in-memory SQLite: " + str(exc)} + try: + conn.executescript(ddl) + cur = conn.cursor() + cur.execute( + "SELECT name FROM sqlite_master WHERE type='table' " + "AND name NOT LIKE 'sqlite_%' ORDER BY name" + ) + tables = [r[0] for r in cur.fetchall()] + out: Dict[str, List[str]] = {} + for t in tables: + # Table names come from sqlite_master in this same database, so there + # is no external input here. + cur.execute('PRAGMA table_info("' + t.replace('"', '""') + '")') + out[t] = sorted(r[1] for r in cur.fetchall()) + return {"tables": out} + except Exception as exc: # noqa: BLE001 — a rejected DDL is a real result + return {"error": type(exc).__name__ + ": " + str(exc)} + finally: + conn.close() + + +def run_tier_x_scenario(scenario_dir: Path) -> ScenarioResult: + name = scenario_dir.name + result = ScenarioResult(tier="x", name=name) + + expected = _load_expected("x", name) + if expected is None: + result.errors.append("No expected file at expected/tier_x/" + name + ".json") + return result + result.expected = expected + + if name == "01_sqlite_schema_structure": + return _run_sqlite_schema_structure(result, expected) + + result.errors.append("Unknown tier-X scenario: " + name) + return result + + +def _run_sqlite_schema_structure( + result: ScenarioResult, expected: Dict[str, Any] +) -> ScenarioResult: + exp = expected.get("expected", {}) + ddl_path = PROJECT_ROOT / "build" / "schema" / "sqlite" / "te_core_schema.sql" + if not ddl_path.exists(): + result.errors.append("SQLite DDL not found at " + str(ddl_path)) + return result + + raw = ddl_path.read_text(encoding="utf-8") + leftover = re.findall(r"\{\{[A-Z_]+\}\}", _substitute_placeholders(raw)) + structure = _sqlite_structure(_substitute_placeholders(raw)) + + if "error" in structure: + result.actual = structure + result.errors.append("SQLite rejected its own DDL: " + structure["error"]) + return result + + tables = structure["tables"] + result.actual = { + "table_count": len(tables), + "tables_present": sorted(tables), + "unsubstituted_placeholders": sorted(set(leftover)), + } + + errors: List[str] = [] + + # Any {{TBL_*}} the substitution missed would reach sqlite3 as a literal and + # silently create a differently-named table. + if leftover: + errors.append("DDL still contains unsubstituted placeholders: " + + ", ".join(sorted(set(leftover)))) + + # Baseline guard. expected_tables is the set SQLite actually ships today, so + # this passes on the known state and fails the moment it drifts either way: + # a table removed, or a new one added without updating this file. + expected_tables = exp.get("expected_tables") + if expected_tables is not None: + want, have = set(expected_tables), set(tables) + missing, extra = sorted(want - have), sorted(have - want) + if missing: + errors.append("expected tables absent from the SQLite DDL: " + + ", ".join(missing)) + if extra: + errors.append( + "SQLite DDL defines tables not in expected_tables: " + + ", ".join(extra) + + " — if this is intentional progress toward full parity, add " + "them to expected_tables and update known_missing_tables" + ) + + # Column presence per table, where the expectation file pins it. + for table, want_cols in (exp.get("expected_columns") or {}).items(): + have_cols = tables.get(table) + if have_cols is None: + continue # already reported by the table-set check above + absent = sorted(set(want_cols) - set(have_cols)) + if absent: + errors.append("table " + table + " is missing columns: " + + ", ".join(absent)) + + # The parity shortfall is recorded, not asserted — see NOTES.txt. Surfacing + # it in actual output keeps it visible in every report without turning the + # scenario red for work that is knowingly outstanding. + known_missing = exp.get("known_missing_tables") or [] + if known_missing: + result.actual["known_missing_tables"] = known_missing + result.actual["parity_note"] = ( + "SQLite ships " + str(len(tables)) + " of " + + str(len(_ENGINE_PLACEHOLDER_TABLES)) + + " T&E tables. Outstanding work tracked in docs/ENGINE_PARITY_TODO.md." + ) + + result.errors = errors + result.passed = not errors + return result + + # --------------------------------------------------------------------------- # Orchestration @@ -548,6 +913,8 @@ def _run_fresh_deploy_then_tests( "p": run_tier_p_scenario, "i": run_tier_i_scenario, "s": run_tier_s_scenario, + "e": run_tier_e_scenario, + "x": run_tier_x_scenario, } diff --git a/scripts/lint.sh b/scripts/lint.sh index f8c1acf..d5e6a48 100644 --- a/scripts/lint.sh +++ b/scripts/lint.sh @@ -10,6 +10,8 @@ PY_SRC=( evals/runner.py evals/gap_report.py api/main.py + api/ratelimit.py + api/services/audit.py ) PY_TESTS=( @@ -19,6 +21,8 @@ PY_TESTS=( tests/test_api_integration.py tests/test_issue_04_multi_file_upload.py tests/test_issue_05_import_summary.py + tests/test_api_hardening.py + tests/test_evals_tier_e_x.py ) # Collect scripts/*.py dynamically diff --git a/tests/test_evals_tier_e_x.py b/tests/test_evals_tier_e_x.py new file mode 100644 index 0000000..1088f84 --- /dev/null +++ b/tests/test_evals_tier_e_x.py @@ -0,0 +1,180 @@ +"""Unit tests for the Tier E (O-07) and Tier X (O-08) eval logic. + +The scenarios themselves need PostgreSQL (Tier E) or read a repo file (Tier X), +but the comparison logic inside them is pure and is where the bugs would live. A +parity check that silently returns "no differences" for two different schemas is +worse than no check, so the negative controls here matter more than the positive +ones: each asserts that a specific kind of drift is actually caught. +""" + +from __future__ import annotations + +import sys +import unittest +from pathlib import Path + +import pytest + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent / "evals")) + +import runner # noqa: E402 + + +@pytest.mark.unit +class TierEDiffSignatures(unittest.TestCase): + """O-07: cross-environment structural comparison.""" + + def test_identical_schemas_report_no_differences(self): + sig = {"organisations": ["org_id:uuid:NO", "name:text:NO"]} + self.assertEqual(runner._diff_signatures("dev", sig, "prod", dict(sig)), []) + + def test_namespace_is_not_part_of_the_comparison(self): + # The whole point of BR-01: two envs differing only in schema name are equal. + dev = {"organisations": ["org_id:uuid:NO"]} + prod = {"organisations": ["org_id:uuid:NO"]} + self.assertEqual(runner._diff_signatures("dev", dev, "prod", prod), []) + + def test_missing_table_is_caught(self): + dev = {"organisations": ["a:text:NO"], "personnel": ["b:text:NO"]} + prod = {"organisations": ["a:text:NO"]} + diffs = runner._diff_signatures("dev", dev, "prod", prod) + self.assertTrue(any("missing tables" in d and "personnel" in d for d in diffs)) + + def test_extra_table_is_caught(self): + dev = {"organisations": ["a:text:NO"]} + prod = {"organisations": ["a:text:NO"], "rogue_table": ["x:text:YES"]} + diffs = runner._diff_signatures("dev", dev, "prod", prod) + self.assertTrue(any("rogue_table" in d for d in diffs)) + + def test_added_column_is_caught(self): + # The exact scenario VCRM_GAPS.md named: someone edits env_staging.sql. + dev = {"organisations": ["org_id:uuid:NO"]} + staging = {"organisations": ["org_id:uuid:NO", "sneaky_col:text:YES"]} + diffs = runner._diff_signatures("dev", dev, "staging", staging) + self.assertTrue(any("sneaky_col" in d for d in diffs)) + + def test_changed_type_is_caught(self): + dev = {"organisations": ["org_id:uuid:NO"]} + prod = {"organisations": ["org_id:text:NO"]} + diffs = runner._diff_signatures("dev", dev, "prod", prod) + self.assertEqual(len(diffs), 1) + self.assertIn("organisations", diffs[0]) + + def test_changed_nullability_is_caught(self): + dev = {"organisations": ["name:text:NO"]} + prod = {"organisations": ["name:text:YES"]} + self.assertTrue(runner._diff_signatures("dev", dev, "prod", prod)) + + def test_reordered_columns_are_caught_as_drift(self): + # Same column set, different ordinal position. Anything doing SELECT * + # or positional COPY would behave differently between the two. + dev = {"organisations": ["a:text:NO", "b:text:NO"]} + prod = {"organisations": ["b:text:NO", "a:text:NO"]} + diffs = runner._diff_signatures("dev", dev, "prod", prod) + self.assertEqual(len(diffs), 1) + self.assertIn("ordinal order", diffs[0]) + + def test_env_targets_cover_all_four_environments(self): + names = [e[0] for e in runner.ENV_TARGETS] + self.assertEqual(names, ["dev", "test", "staging", "prod"]) + # Every env must have a distinct database AND schema, or "isolated + # environments" is not what is actually deployed. + self.assertEqual(len({e[1] for e in runner.ENV_TARGETS}), 4) + self.assertEqual(len({e[2] for e in runner.ENV_TARGETS}), 4) + + +@pytest.mark.unit +class TierXSqliteStructure(unittest.TestCase): + """O-08: SQLite DDL execution and structural readback.""" + + def test_placeholders_are_substituted(self): + out = runner._substitute_placeholders( + "CREATE TABLE {{TBL_ORGANISATIONS}} (x TEXT);" + ) + self.assertIn("organisations", out) + self.assertNotIn("{{", out) + + def test_every_known_table_has_a_substitution(self): + ddl = " ".join("{{TBL_" + t.upper() + "}}" + for t in runner._ENGINE_PLACEHOLDER_TABLES) + self.assertNotIn("{{", runner._substitute_placeholders(ddl)) + + def test_unknown_placeholder_survives_so_the_scenario_can_flag_it(self): + out = runner._substitute_placeholders("{{TBL_NOT_A_REAL_TABLE}}") + self.assertIn("{{TBL_NOT_A_REAL_TABLE}}", out) + + def test_structure_is_read_back_from_executed_ddl(self): + got = runner._sqlite_structure( + "CREATE TABLE alpha (id TEXT PRIMARY KEY, name TEXT NOT NULL);" + "CREATE TABLE beta (ref TEXT);" + ) + self.assertNotIn("error", got) + self.assertEqual(sorted(got["tables"]), ["alpha", "beta"]) + self.assertEqual(got["tables"]["alpha"], ["id", "name"]) + + def test_invalid_ddl_is_reported_rather_than_raised(self): + got = runner._sqlite_structure("CREATE TABLE (((;") + self.assertIn("error", got) + + def test_sqlite_internal_tables_are_excluded(self): + got = runner._sqlite_structure( + "CREATE TABLE t (id INTEGER PRIMARY KEY AUTOINCREMENT);" + ) + # AUTOINCREMENT creates sqlite_sequence; it is not part of the schema. + self.assertEqual(sorted(got["tables"]), ["t"]) + + def test_the_real_sqlite_ddl_executes_and_matches_the_expectation_file(self): + import json + root = Path(__file__).resolve().parent.parent + ddl = (root / "build" / "schema" / "sqlite" + / "te_core_schema.sql").read_text(encoding="utf-8") + exp = json.loads( + (root / "evals" / "expected" / "tier_x" + / "01_sqlite_schema_structure.json").read_text(encoding="utf-8") + )["expected"] + + got = runner._sqlite_structure(runner._substitute_placeholders(ddl)) + self.assertNotIn("error", got, "the shipped SQLite DDL must execute") + self.assertEqual(sorted(got["tables"]), sorted(exp["expected_tables"])) + + def test_known_missing_tables_are_genuinely_absent(self): + # Guards the honesty of the expectation file: if someone adds one of + # these tables to the DDL but forgets to move it out of + # known_missing_tables, the file would misreport the shortfall. + import json + root = Path(__file__).resolve().parent.parent + ddl = (root / "build" / "schema" / "sqlite" + / "te_core_schema.sql").read_text(encoding="utf-8") + exp = json.loads( + (root / "evals" / "expected" / "tier_x" + / "01_sqlite_schema_structure.json").read_text(encoding="utf-8") + )["expected"] + got = runner._sqlite_structure(runner._substitute_placeholders(ddl)) + for table in exp["known_missing_tables"]: + self.assertNotIn( + table, got["tables"], + f"{table} is listed as missing but the DDL now defines it — " + "move it into expected_tables", + ) + + def test_missing_plus_present_accounts_for_all_twelve_tables(self): + import json + root = Path(__file__).resolve().parent.parent + exp = json.loads( + (root / "evals" / "expected" / "tier_x" + / "01_sqlite_schema_structure.json").read_text(encoding="utf-8") + )["expected"] + total = set(exp["expected_tables"]) | set(exp["known_missing_tables"]) + self.assertEqual( + total, set(runner._ENGINE_PLACEHOLDER_TABLES), + "the expectation file must account for every T&E table, so the " + "shortfall cannot be understated by omission", + ) + + +@pytest.mark.unit +class TierRegistration(unittest.TestCase): + """Both new tiers must be reachable from the CLI.""" + + def test_all_five_tiers_are_registered(self): + self.assertEqual(sorted(runner.TIER_RUNNERS), ["e", "i", "p", "s", "x"]) From f62974c9b5b85c595ddbd92dc9b21fa585e5990c Mon Sep 17 00:00:00 2001 From: amar-python Date: Thu, 10 Sep 2026 13:39:25 +1000 Subject: [PATCH 4/8] docs: record ADR-0001 PostgreSQL-first; reconcile VCRM to measured state ADR-0001 records the decision to reach a working PostgreSQL milestone before extending to other engines, with the per-engine measurements that made it possible, the three alternatives rejected, the accepted risk, and four revisit triggers. It exists mainly to answer the question a future reader will have: why do six adapter scripts and six schema directories sit in the tree when the docs describe one engine? BR-02's wording in VCRM.md is retained unchanged and annotated as deferred -- it states the eventual target, not the current milestone. Its status moves from 'not verified' to 'deferred by decision', which is a different thing: an unverified requirement is a gap, a deferred one is a choice with a trigger to revisit. VCRM.md was independently stale: it still reported 17 of 22 (77%) with BR-01 and BR-15 listed as unverified, though BR-15 had been fixed by assertion S10 in SQL suite 05 and BR-01 is closed by Tier E. Corrected to 19 of 22 (86%), with zero actionable requirement gaps -- the three remaining are all deferred by recorded decision. RECONCILED_GAPS.md carries a correction worth keeping: an earlier register was built from a partial 7z snapshot rather than this repository and wrongly concluded the routers/services API had been superseded by a thin translator. Six of the twelve gaps it listed as open were already closed. The lesson is recorded because the failure mode -- reviewing the wrong artifact confidently -- is cheap to repeat. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01V1v7ow7NxH9y1ELUyGgACn --- docs/ENGINE_PARITY_TODO.md | 107 ++++++++++++++++++++ docs/GAP_ANALYSIS.md | 62 ++++++++++-- docs/RECONCILED_GAPS.md | 161 ++++++++++++++++++++++++++++++ docs/VCRM.md | 42 +++++--- docs/VCRM_GAPS.md | 118 +++++++++++++++++++--- docs/adr/0001-postgresql-first.md | 150 ++++++++++++++++++++++++++++ docs/reconciled_gaps.csv | 20 ++++ 7 files changed, 623 insertions(+), 37 deletions(-) create mode 100644 docs/ENGINE_PARITY_TODO.md create mode 100644 docs/RECONCILED_GAPS.md create mode 100644 docs/adr/0001-postgresql-first.md create mode 100644 docs/reconciled_gaps.csv diff --git a/docs/ENGINE_PARITY_TODO.md b/docs/ENGINE_PARITY_TODO.md new file mode 100644 index 0000000..0febbb9 --- /dev/null +++ b/docs/ENGINE_PARITY_TODO.md @@ -0,0 +1,107 @@ +# Engine parity — deferred work, measured state + +> **Status: deferred by decision, 2026-09-10.** +> Multi-engine support is deferred until the PostgreSQL milestone is reached — +> see [ADR-0001](adr/0001-postgresql-first.md) for the decision, its rationale, +> and its revisit triggers. This file is the measured state and the open +> questions, kept so the work can be picked up without re-deriving any of it. +> +> **This is not a to-do list for now.** Nothing here is scheduled. + +Raised while building the Tier X eval (O-08). The measurements below are what +made the decision possible: before Tier X, the six-engine claim rested on code +review and nobody had counted. + +**Nothing in this file is asserted by a test today.** Tier X scenario 01 pins +SQLite's *current* 7-table baseline and fails on drift; it deliberately does not +assert full parity. It is also **no longer run in CI** (ADR-0001) — run it on +demand with `make evals-engines`. See +`evals/datasets/tier_x/01_sqlite_schema_structure/NOTES.txt`. + +--- + +## Measured state + +Counted from `CREATE TABLE` statements in each engine's `te_core_schema.sql` +on 2026-09-10. + +| Engine | Tables defined | Against PostgreSQL's 12 | Artifacts present | +|---|---|---|---| +| **postgresql** | 12 | reference | `te_core_schema.sql`, `te_seed_data.sql` | +| **sqlite** | 7 | 5 missing | `te_core_schema.sql`, `te_seed_data.sql` | +| **mariadb** | 7 | 5 missing | `te_core_schema.sql`, `te_seed_data.sql` | +| **teradata** | 1 | 11 missing | `te_core_schema.sql` (skeletal) | +| **influxdb** | n/a | no DDL at all | `te_seed_data.lp` only | +| **redis** | n/a | no DDL at all | `te_seed_data.sh` only | + +## Missing from both SQLite and MariaDB + +The same five, in both engines: + +- `temp_documents` +- `test_phases` +- `test_events` +- `test_results` +- `evidence_artifacts` + +These are the execution and evidence half of the model. What is present in +SQLite and MariaDB is the planning half — organisations, personnel, programs, +requirements, test cases, VCRM entries, defects. So both engines can hold a test +*plan* and its traceability, but not a record of test *execution*. + +That matters for the BR-02 claim specifically: an engine that cannot store +`test_results` or `evidence_artifacts` cannot support the T&E workflow the +framework exists to serve, even though its adapter script runs and its DDL +deploys cleanly. + +## Why this went unnoticed + +`build/adapters/adapter_.sh` deploys whatever DDL it is given and reports +success on exit 0. With no cross-engine eval, "the adapter ran" was the only +signal available, and it was a true statement about an incomplete schema. This is +the failure mode Tier X exists to close. + +## Open questions for when this resumes + +These are the questions to answer first; ADR-0001 defers them rather than +answering them. + +1. **Target scope per engine.** Is SQLite meant to reach full 12-table parity, or + is it deliberately a planning-only subset for local/offline use? Same question + for MariaDB. The answer changes whether the 5 tables are a gap or a design + choice — and if it is a design choice, it belongs in `docs/ARCHITECTURE.md` + rather than in a TODO. +2. **Teradata.** 1 of 12 is closer to a placeholder than an implementation. + Either schedule the DDL or drop the claim to "planned". +3. **InfluxDB and Redis.** Neither is relational, so "12 tables" does not + translate. What would count as supported — seed data landing in the expected + measurements / key namespaces? That needs defining before it can be verified. +4. **BR-02 wording in `docs/VCRM.md`.** ✅ **Settled 2026-09-10.** The wording is + retained unchanged and annotated as deferred by decision, pointing at + ADR-0001. It states the eventual target, not the current milestone. BR-02's + VCRM status moved from ❌ Not verified to ⏸ Deferred by decision. + +## If you take on the SQLite tables + +Order them by dependency, since each references the one before: + +``` +test_phases -> references test_programs +test_events -> references test_phases +test_results -> references test_events, test_cases +evidence_artifacts -> references test_results +temp_documents -> references test_programs (independent of the chain above) +``` + +Then, in the same commit, move each table from `known_missing_tables` into +`expected_tables` in +`evals/expected/tier_x/01_sqlite_schema_structure.json`. Tier X will fail until +you do — `tests/test_evals_tier_e_x.py` asserts that the two lists together +account for all 12 tables, and that everything listed as missing is genuinely +absent. That is the reminder working as designed. + +## Cheapest next engine after SQLite + +MariaDB. It needs a service, but `docker run mariadb` is a ~10-minute setup and +the DDL is already 7 tables in, so a Tier X scenario 02 would follow the same +shape as 01 with a real connection instead of an in-memory one. diff --git a/docs/GAP_ANALYSIS.md b/docs/GAP_ANALYSIS.md index 60575d6..d9e862e 100644 --- a/docs/GAP_ANALYSIS.md +++ b/docs/GAP_ANALYSIS.md @@ -19,9 +19,15 @@ claim below was reproduced, not inferred from reading code. | ID | Gap | Severity | Decision needed | |---|---|---|---| | G2 | Windows CI cannot run database-backed tests | Medium | Accepted — ubuntu `integration-postgres` covers the full suite | -| G3 | Tiers X and E remain unimplemented | Medium | No — deferred by design | +| G3 | Tiers X and E — **scaffolded 2026-09-10** | Closed | No — engine parity deferred by [ADR-0001](adr/0001-postgresql-first.md) | -G1, G4, and G5 (below) are resolved — see "Closed by this pass". +G1, G4, and G5 (below) are resolved — see "Closed by this pass". G3 is now +closed: both tiers exist and run. The missing DDL in four of the six engines is +no longer an open gap but a recorded decision — multi-engine support is deferred +until the PostgreSQL milestone, per [ADR-0001](adr/0001-postgresql-first.md). +Measured state and open questions are in `docs/ENGINE_PARITY_TODO.md`. + +G2 is the only genuinely open item, and it is accepted rather than actionable. --- @@ -73,15 +79,49 @@ pass/fail in exactly one job. Windows image ships it, stopped) and provision there too. Not verified — no Windows runner was available during this audit. -### G3 — Tiers X and E unimplemented (Medium) - -`evals/PLAN.md` defines five tiers; P, I and S are implemented. **X** -(cross-engine schema equivalence) and **E** (cross-environment structural -parity) remain deferred, so cross-engine claims for MariaDB, SQLite, InfluxDB, -Redis and Teradata rest on code review rather than execution. - -Partially mitigated: `tests/test_parity.py::TestAllEnvironmentsHaveRequiredTables` -now runs against all four PostgreSQL environments. +### G3 — Tiers X and E (Medium) — SCAFFOLDED 2026-09-10 + +`evals/PLAN.md` defines five tiers. All five now have at least one executing +scenario; `runner.TIER_RUNNERS` covers `p, i, s, e, x`. + +**Tier E — cross-environment structural parity.** Scenario +`01_envs_have_identical_structure` deploys all four environments, reduces each +deployed schema to a fingerprint of table → ordered +`column:type:nullability`, and compares Test / Staging / Prod against Dev. The +schema name is excluded from the fingerprint, so the one difference BR-01 permits +(the identifier namespace) compares equal while any structural drift fails. +Reordered columns are treated as drift, because ordinal position is part of the +deployed contract. Closes VCRM BR-01. + +**Tier X — cross-engine structural coverage.** Scenario +`01_sqlite_schema_structure` executes `build/schema/sqlite/te_core_schema.sql` +in an in-memory SQLite database via Python's stdlib `sqlite3`, then reads the +structure back from `sqlite_master` / `PRAGMA table_info`. No `sqlite3` binary is +required, so unlike every other database-backed check this one also runs on the +Windows CI job (see G2). Partially addresses VCRM BR-02. + +**What Tier X revealed.** Only PostgreSQL defines all 12 T&E tables. SQLite and +MariaDB define 7, Teradata 1; InfluxDB and Redis ship seed data with no DDL. The +five tables absent from SQLite and MariaDB — `temp_documents`, `test_phases`, +`test_events`, `test_results`, `evidence_artifacts` — are the execution and +evidence half of the model, so those engines can hold a test plan but not a +record of test execution. Full detail and the decisions needed are in +`docs/ENGINE_PARITY_TODO.md`. + +**Scope limits, stated deliberately.** Tier X scenario 01 pins SQLite's current +7-table baseline rather than asserting all 12. Asserting parity would leave the +job permanently red for work that is knowingly outstanding, and a job that is +always red stops being read; the shortfall is instead carried in +`known_missing_tables`, in every generated report, and in the TODO above. The +scenario still fails on drift in either direction. It also executes the DDL +rather than `adapter_sqlite.sh`, so the adapter scripts themselves remain +unexercised — a smaller, separate gap. + +Also mitigated: `tests/test_parity.py::TestAllEnvironmentsHaveRequiredTables` +runs against all four PostgreSQL environments, and +`tests/test_evals_tier_e_x.py` unit-tests both tiers' comparison logic with +negative controls for added columns, changed types, changed nullability, +reordered columns, and missing or extra tables. ### G4 — Runtime artifacts not gitignored (Low) — RESOLVED 2026-08-13 diff --git a/docs/RECONCILED_GAPS.md b/docs/RECONCILED_GAPS.md new file mode 100644 index 0000000..9060dce --- /dev/null +++ b/docs/RECONCILED_GAPS.md @@ -0,0 +1,161 @@ +# Reconciled Gap Register — O-01 to O-12 + +**Baseline:** `C:\Users\User\OneDrive\Desktop\Migration using ai\PostgreDataMigrationAppWithCSVLoader`, branch `main` @ `269ffe2` +**Date:** 2026-09-10 +**Method:** every status below was verified by reading the code or executing it in this repo. No status is carried over from a document claim alone. + +--- + +## Correction to the previous version of this register + +An earlier draft of this document concluded that "Abacus reviewed a superseded API design." **That conclusion was wrong**, and it was wrong for an instructive reason. + +That draft was built from a package snapshot (`PostgreDataMigrationAppupdate.7z`) rather than from this repository. Inside that snapshot, `api-for-github.zip` contains a *thin-translator* API — a single `api/main.py` that shells out to `build/csv_loader.sh`, with no `config.py`, no `db.py`, no `routers/`, no `services/`. Finding that, the draft inferred the routers/services architecture Abacus described had been replaced. + +The live repository tells a different story. It has `api/config.py`, `api/db.py`, `api/routers/{csv,te,audit}_routes.py` and `api/services/{csv_parse,dynamic_loader,te_loader}.py` — the architecture Abacus actually reviewed. The thin translator in that zip is a **separate parallel artifact**, not the successor to this code. + +Two lessons worth keeping: + +1. **A snapshot is not the repository.** The 7z was a partial delta — it did not contain `build/config.env.example`, `build/setup.sh`, the six `build/csv/loader_*.sh` scripts, `tests/suites/`, or `build/adapters/`. Those absences are precisely what made the highest-priority gaps look unfixable, and what made a wrong architectural inference possible. +2. **Confirm the baseline before reviewing.** Reviewing the wrong artifact produced a register that was confidently, specifically incorrect across ten rows. + +Abacus's real limitation was different and narrower: it reviewed an **older commit**. The repository has since addressed BUG-006, 025, 027, 028, 030, 031, 035, 040 and 041, added `api/auth.py` and `api/routers/audit_routes.py`, and resolved GAP_ANALYSIS G1, G4 and G5. + +--- + +## Status of O-01 to O-12 + +| ID | Gap | Status | Evidence | +|----|-----|--------|----------| +| O-01 | `config.env.example` names mismatch loaders/`setup.sh` | ✅ **Already closed** | GAP_ANALYSIS G1, resolved 2026-08-12. Verified: `config.env.example` and `setup.sh` both use the `PG_*_` scheme. | +| O-02 | BR-15 `conn_limit` not asserted | ✅ **Already closed** | Assertion **S10** in `tests/suites/test_05_schema_and_business_rules.sql` reads `pg_roles.rolconnlimit`. | +| O-03 | No auth fail-closed, no rate limiting | ✅ **Fixed this pass** | `API_ENV` gate + `api/ratelimit.py`. 24 tests in `tests/test_api_hardening.py`. | +| O-04 | `audit_log` written on delete only | ✅ **Fixed this pass** | `api/services/audit.py`; upload path audited; delete refactored onto the shared writer. | +| O-05 | No migration-history endpoint | ✅ **Already closed** | `GET /api/csv/files` in `csv_routes.py`; pagination added in `269ffe2`. | +| O-06 | No dedup layer | ✅ **Already closed** | Three levels in `dynamic_loader.py`: filename, content SHA-256, per-row `_row_hash UNIQUE`. | +| O-07 | BR-01 cross-env parity unverified | ✅ **Fixed this pass** | Tier E scenario `01_envs_have_identical_structure` + 10 unit tests + CI wiring. | +| O-08 | BR-02 non-PG engines unverified | ⏸ **Measured, then deferred by decision** | Tier X scenario `01_sqlite_schema_structure` measured the shortfall; [ADR-0001](adr/0001-postgresql-first.md) defers multi-engine until the PostgreSQL milestone. | +| O-09 | Windows CI cannot host PostgreSQL | ✅ **Accepted + improved** | GAP_ANALYSIS G2 accepted. Tier X now runs there — the first cross-engine coverage that job has ever carried. | +| O-10 | Runtime artifacts not gitignored | ✅ **Already closed** | GAP_ANALYSIS G4, resolved 2026-08-13. Verified: all four entries present, plus `evals/reports/`. | +| O-11 | VCRM BR-20 count edited 85 → 142 | ✅ **Already closed** | GAP_ANALYSIS G5. Confirmed at 142, matching the Tier S expectation JSON. | +| O-12 | Frontend not independently audited | ✅ **Documented this pass** | "Coverage note — frontend" in `docs/VCRM_GAPS.md`, naming two specific risks and the cheapest remedy. | + +**Six of the twelve were already closed before this pass began.** They were listed as open because the earlier register was built from the snapshot, not the repo. + +--- + +## O-08 — measured, then deferred by decision + +**Settled 2026-09-10.** [ADR-0001](adr/0001-postgresql-first.md) records the +decision: reach a working PostgreSQL milestone first, then apply what that teaches +to the other engines. There is no longer an open decision here. The measurements +below are what made the decision possible. + +Building Tier X measured what each engine's `te_core_schema.sql` actually defines: + +| Engine | Tables | Against PostgreSQL's 12 | +|---|---|---| +| postgresql | 12 | reference | +| sqlite | 7 | 5 missing | +| mariadb | 7 | 5 missing | +| teradata | 1 | 11 missing | +| influxdb | no DDL (seed `.lp` only) | n/a | +| redis | no DDL (seed `.sh` only) | n/a | + +Absent from both SQLite and MariaDB: `temp_documents`, `test_phases`, `test_events`, `test_results`, `evidence_artifacts`. + +Those five are the execution and evidence half of the model. What remains is the planning half — organisations, personnel, programs, requirements, test cases, VCRM entries, defects. So both engines can hold a test *plan* and its traceability, but not a record of test *execution*. For a T&E framework that is the material half. + +**Why it went unnoticed:** `adapter_.sh` deploys whatever DDL it is handed and reports success on exit 0. With no cross-engine eval, "the adapter ran" was the only available signal, and it was a true statement about an incomplete schema. + +**How it is now recorded:** BR-02's wording in `docs/VCRM.md` is retained unchanged and annotated as deferred by decision — it states the eventual target, not the current milestone. Its status moved from ❌ Not verified to ⏸ Deferred by decision. VCRM coverage is 19 of 22 (86 %) verified with **zero actionable requirement gaps**: the three remaining are all deferred deliberately. Tier X stays in the repository but is out of CI (`make evals-engines` runs it); per-engine open questions are preserved in `docs/ENGINE_PARITY_TODO.md`. + +**Why Tier X passes rather than fails:** it pins SQLite's current 7-table baseline instead of asserting 12. Asserting parity would leave the job permanently red for work that is knowingly outstanding, and a job that is always red stops being read. The shortfall is carried instead in `known_missing_tables`, in every generated report's `actual` block, and in the TODO. The scenario still fails on drift in either direction, and `tests/test_evals_tier_e_x.py` asserts the two lists together account for all 12 tables — so the shortfall cannot be understated by omission. + +--- + +## Other findings from this pass + +| # | Finding | Action taken | +|---|---------|--------------| +| 1 | Engine parity shortfall (table above) | Measured, then deferred by [ADR-0001](adr/0001-postgresql-first.md). BR-02 annotated as deferred; `docs/ENGINE_PARITY_TODO.md` holds the measured state and open questions | +| 2 | `api/auth.py` docstring claimed "applied globally in api/main.py" — it is applied **per-router**. Enforcement was real, the documentation was not. | Docstring corrected; the per-router rationale (BUG-035, keeping `/api/health` open for probes) now stated where a reader will find it | +| 3 | `scripts/lint.sh` lints a hardcoded file list, so new modules silently escaped flake8 and bandit | Added `api/ratelimit.py`, `api/services/audit.py`, and both new test modules to the list | +| 4 | `docs/VCRM_GAPS.md` was stale: BR-15 had been fixed in code without the traceability document being updated | Updated with status headers and a "what changed" table; coverage restated 77 % → 86 % | +| 5 | `audit_log` had exactly one writer (`delete_file`), so the log recorded destructions but never creations | Fixed under O-04 | +| 6 | Stale git worktree at `.claude/worktrees/velvety-chasing-badger/` holds an older copy of `api/`, including its own `auth.py` and `routers/` | **Not touched** — flagged only. It pollutes repo-wide `grep` results and cost real time during this review. Worth pruning if it is dead. | + +--- + +## Known limitation in the O-04 fix + +The loaders own and commit their own transactions, so an upload's audit row is written *after* the load has already committed rather than inside the same transaction. A crash in the narrow window between the two leaves a load with no audit row. + +Because of that ordering, a failed audit write does **not** fail the request: the data is already in the database, and reporting failure for a load that actually succeeded would be worse than a missing log line. The failure is logged at ERROR so it is alertable. + +Closing that window means threading the caller's connection into the loaders so the audit INSERT joins their transaction. That changes the loader signatures, so it is recorded here rather than smuggled into this pass. The delete path already has the stronger guarantee — it uses `write_audit(cur, ...)` inside its own transaction. + +--- + +## Verification + +Everything below was executed in this repo, not asserted from a document. + +| Check | Result | +|---|---| +| `pytest -m "unit or regression or security or snapshot"` | **115 passed, 0 failed, 0 skipped** (was 96 before this pass) | +| `scripts/test_report.py --strict` | **RESULT: PASS** — 229 collected, 115 executed, 0 skipped, 114 deselected by marker filter | +| `bash scripts/lint.sh` (flake8 + bandit) | **All lint checks passed**, with the new modules inside the gate | +| `evals/runner.py --tiers p,x` | **24 passed, 0 failed, 0 skipped** | +| `evals/runner.py --tiers e` | Correctly **FAILS** without PostgreSQL, per the repo's no-skip policy (F5) — it does not skip | +| Tier E / Tier X comparison logic | 19 unit tests in `tests/test_evals_tier_e_x.py`, including negative controls for added column, changed type, changed nullability, reordered columns, missing table, extra table | + +Tier E's full four-environment comparison has **not** been executed end to end — this session had no PostgreSQL instance. Its comparison logic is unit-tested with negative controls, and the scenario is wired into the `integration-postgres` CI job, so its first real run will be there. That is stated plainly rather than implied as verified. + +--- + +## Files changed + +**Added (6)** + +```text +api/ratelimit.py +api/services/audit.py +tests/test_api_hardening.py +tests/test_evals_tier_e_x.py +evals/datasets/tier_e/01_envs_have_identical_structure/NOTES.txt +evals/expected/tier_e/01_envs_have_identical_structure.json +evals/datasets/tier_x/01_sqlite_schema_structure/NOTES.txt +evals/expected/tier_x/01_sqlite_schema_structure.json +docs/ENGINE_PARITY_TODO.md +docs/RECONCILED_GAPS.md +docs/adr/0001-postgresql-first.md +``` + +**Modified (10)** + +```text +api/config.py API_ENV + rate-limit budgets +api/main.py assert_auth_configured() + middleware registration +api/auth.py corrected docstring +api/routers/csv_routes.py upload audit; delete refactored onto shared writer +evals/runner.py Tier E and Tier X runners; import re; psql --set binding +tests/conftest.py rate limits disabled for the suite; limiter reset fixture +scripts/lint.sh new modules added to the lint gate +Makefile eval targets now p,i,s,e,x +.github/workflows/quality-gate.yml Tier X in free tier; Tiers E and X in integration +.github/workflows/python-validator-tests.yml Tier X on Windows +docs/GAP_ANALYSIS.md G3 closed; engine parity deferred per ADR-0001 +docs/VCRM_GAPS.md BR-01/BR-15 resolved, BR-02 deferred, frontend note, decision log +docs/VCRM.md BR-02 annotated deferred; stale 77% coverage corrected to 86% +``` + +--- + +## Suggested next steps + +1. ~~Decide BR-02's target scope per engine~~ — **done.** Deferred by [ADR-0001](adr/0001-postgresql-first.md): PostgreSQL first. +2. **Run the integration CI job** to give Tier E its first real four-environment execution. This is now the main outstanding verification. +3. **Set `API_ENV` and `API_KEY` in the Azure deployment** before the next deploy — the API now refuses to start without a key when `API_ENV` is not `dev`, which is the intended behaviour but will surface as a startup failure if the variables are not set. +4. **Consider pruning** `.claude/worktrees/velvety-chasing-badger/` if it is dead. +5. ~~Tier X scenario 02 for MariaDB~~ — deferred with the rest of multi-engine work. It stays the cheapest next engine when ADR-0001 is revisited. diff --git a/docs/VCRM.md b/docs/VCRM.md index bd741db..a799e15 100644 --- a/docs/VCRM.md +++ b/docs/VCRM.md @@ -38,7 +38,7 @@ Test-layer codes used in column headers below: | ID | Requirement | Source | |----|------------|--------| | **BR-01** | The framework shall deploy isolated **Dev / Test / Staging / Prod** environments on a single PostgreSQL instance, each with its own database, schema, app user, and connection limit. | README §"Environment Comparison" | -| **BR-02** | The framework shall support **six database engines** — PostgreSQL, MariaDB, SQLite, InfluxDB, Redis, Teradata — through adapter scripts. | README §"What This Is", `build/adapters/*` | +| **BR-02** | The framework shall support **six database engines** — PostgreSQL, MariaDB, SQLite, InfluxDB, Redis, Teradata — through adapter scripts. **⏸ Deferred by decision 2026-09-10** — PostgreSQL first; see [ADR-0001](adr/0001-postgresql-first.md). Requirement wording retained unchanged; it states the eventual target, not the current milestone. Per-engine measured state in [`ENGINE_PARITY_TODO.md`](ENGINE_PARITY_TODO.md). | README §"What This Is", `build/adapters/*` | | **BR-03** | All schema identifiers (database, schema, role, table names) shall be controlled by a single `\set` configuration block; renaming in one place updates the entire deployment. | README §"How Parameterisation Works" | | **BR-04** | The T&E data model shall persist the full lifecycle: **organisations, personnel, programs, TEMP documents, phases, requirements, test cases, VCRM entries, test events, results, defect reports, evidence artefacts** (12 tables). | README §"Schema Reference" | | **BR-05** | The framework shall enforce **100 % VCRM coverage** for every active program, with explicit gap detection for programs that intentionally lack coverage. | README §"Seed Data", section §"vcrm_entries 100% coverage" | @@ -78,8 +78,8 @@ For each requirement, the columns mark which test layer verifies it. Numbers in | ID | Requirement (short) | PU | SQL | TP | TI | TS | LV | Status | Notes | |----|--------------------|----|-----|----|----|----|----|-------|-------| -| BR-01 | Multi-env isolated (Dev/Test/Staging/Prod) | — | suite 05 (schema_name) | — | — | 01 (Dev only) | — | ⚠️ | Tier I/S only exercise Dev. Test/Staging/Prod parity is not asserted (would need Tier E). | -| BR-02 | Six DB engines via adapters | — | — | — | — | — | — | ❌ | No adapter-level tests exist; only PG is verified. Would need Tier X. | +| BR-01 | Multi-env isolated (Dev/Test/Staging/Prod) | — | suite 05 (schema_name) | — | — | 01 (Dev) | **Tier E 01** | ✅ | Tier E `01_envs_have_identical_structure` deploys all four environments and compares column signatures against Dev. Resolved 2026-09-10. | +| BR-02 | Six DB engines via adapters | — | — | — | — | — | Tier X 01 (SQLite, not in CI) | ⏸ | **Deferred by decision** (ADR-0001): PostgreSQL first. Tier X `01_sqlite_schema_structure` exists and passes but is not run in CI. Measured state: PG 12 tables, SQLite 7, MariaDB 7, Teradata 1; InfluxDB and Redis ship seed data with no DDL. | | BR-03 | `\set` parameterisation works | — | suite 05 (table existence under `:"tbl_*"` overrides) | — | — | 01 (passes `--set tbl_*=...`) | — | ✅ | Tier S proves the parameterisation contract holds end-to-end. | | BR-04 | 12-table T&E data model | — | suites 01–05 (every table referenced by at least one assertion) | — | 01 (counts rows in 11 of 12 tables; evidence_artifacts not counted) | 01 | — | ✅ | One small gap: Tier I doesn't count `evidence_artifacts`, but that table is schema-only per the README. | | BR-05 | 100 % VCRM coverage for CYB9131 + gap detection for LAND400 | — | **suite 03 (23 assertions)** | — | — | 01 | — | ✅ | Direct verification — suite 03 is the canonical VCRM check. | @@ -132,23 +132,35 @@ The `input_data/` loader has its own implicit requirements — verified end-to-e | Status | Count | Requirements | |--------|-------|-------------| -| ✅ Verified | 17 | BR-03, BR-04, BR-05, BR-06, BR-07, BR-08, BR-09, BR-10, BR-11, BR-12, BR-13, BR-14, BR-16, BR-17, BR-18, BR-19, BR-20 | -| ⚠️ Partial | 1 | BR-01 | -| ❌ Not verified | 4 | BR-02, BR-15, BR-21, BR-22 | +| ✅ Verified | 19 | BR-01, BR-03, BR-04, BR-05, BR-06, BR-07, BR-08, BR-09, BR-10, BR-11, BR-12, BR-13, BR-14, BR-15, BR-16, BR-17, BR-18, BR-19, BR-20 | +| ⚠️ Partial | 0 | — | +| ⏸ Deferred by decision | 3 | BR-02, BR-21, BR-22 | -**Headline:** **17 of 22 (77 %)** business requirements are fully verified by at least one automated test condition. Of the 5 not fully verified, 2 are deferred by design (BR-21, BR-22) and 3 are genuine gaps (BR-01 partial, BR-02 unverified, BR-15 unverified). +**Headline:** **19 of 22 (86 %)** business requirements are fully verified by at least one automated test condition, up from 17 of 22 (77 %). + +**There are no longer any unverified requirements that are not a recorded decision.** The three remaining are deferred deliberately: BR-02 by ADR-0001 (PostgreSQL first), BR-21 and BR-22 by earlier project decisions. That distinction matters in review — an unverified requirement is a gap, a deferred one is a choice with a revisit trigger. + +Changed 2026-09-10: + +- **BR-01** ⚠️ Partial → ✅ Verified. Tier E `01_envs_have_identical_structure`. +- **BR-15** ❌ Not verified → ✅ Verified. Assertion **S10** in `tests/suites/test_05_schema_and_business_rules.sql` reads `pg_roles.rolconnlimit`. +- **BR-02** ❌ Not verified → ⏸ Deferred by decision. See ADR-0001. --- ## 4. Gap analysis -### Genuine gaps (recommended to address) +### Genuine gaps + +**None outstanding as at 2026-09-10.** All three gaps previously listed here have +been closed or converted into a recorded decision. The table is retained as the +history of what was addressed and how. -| Req | Gap | Recommended verification | Effort | -|-----|-----|--------------------------|--------| -| **BR-01 partial** | Test/Staging/Prod environments are deployable but their structural equivalence to Dev is not asserted by any test. A change in `env_test.sql` that drifts from `env_dev.sql` would not be caught. | Add a Tier E scenario `01_envs_have_identical_structure` that deploys all four envs, queries `information_schema.columns` for each, and diffs the structure. | Medium (1 day) | -| **BR-02** | The framework claims to support 6 DB engines via adapters, but no test runs against MariaDB / SQLite / etc. | Add a Tier X scenario per engine. Earliest wins: SQLite (no service needed, just a file). | Medium per engine | -| **BR-15** | The per-environment `conn_limit` value lives in `env_*.sql` but no test confirms it's applied. | Add a SQL assertion in suite 05: `SELECT rolconnlimit FROM pg_roles WHERE rolname = :'app_user'` and `assert_equals(..., )`. | Small (~1 hour) | +| Req | Gap (historical) | What was done | Status | +|-----|------------------|---------------|--------| +| **BR-01 partial** | Test/Staging/Prod were deployable but their structural equivalence to Dev was not asserted. A change in `env_test.sql` drifting from `env_dev.sql` would not be caught. | Built Tier E scenario `01_envs_have_identical_structure`: deploys all four environments, reads `information_schema.columns` per environment, reduces each to a table → ordered `column:type:nullability` fingerprint excluding the schema name, and compares the other three against Dev. Reordered columns count as drift. Comparison logic unit-tested in `tests/test_evals_tier_e_x.py` with negative controls. | ✅ Resolved | +| **BR-02** | The framework claimed 6 DB engines via adapters, but no test ran against MariaDB / SQLite / etc. | Built Tier X scenario `01_sqlite_schema_structure`, which executes the SQLite DDL and pins its structure. It measured a shortfall the requirement's wording does not admit — PG 12 tables, SQLite 7, MariaDB 7, Teradata 1, InfluxDB and Redis no DDL at all. A decision was then taken to pursue PostgreSQL first. | ⏸ Deferred by decision — [ADR-0001](adr/0001-postgresql-first.md) | +| **BR-15** | The per-environment `conn_limit` lived in `env_*.sql` but no test confirmed it was applied. | Added assertion **S10** to `tests/suites/test_05_schema_and_business_rules.sql`, reading `rolconnlimit` from `pg_roles` and comparing it against the environment's `conn_limit` forwarded via `set_config('te.conn_limit')`. A typo such as `conn_limit=5` in `env_prod.sql` now fails the suite. | ✅ Resolved | ### Deliberately deferred @@ -183,7 +195,9 @@ This VCRM was derived as follows: **Recommended review actions:** 1. Stakeholder review of Section 1 — confirm the 20 in-scope requirements really are the right ones. -2. Triage the 3 genuine gaps (BR-01-partial, BR-02, BR-15) — accept the risk or schedule the verification work. +2. ~~Triage the 3 genuine gaps (BR-01-partial, BR-02, BR-15)~~ — **done 2026-09-10.** BR-01 and BR-15 are + verified; BR-02 is deferred by ADR-0001. Confirm the deferral still reflects intent at each milestone review, + using the revisit triggers in that ADR. 3. Re-verify after each release: re-run all test layers, update this matrix if any cell flips status. --- diff --git a/docs/VCRM_GAPS.md b/docs/VCRM_GAPS.md index 2cb06ab..6241ff4 100644 --- a/docs/VCRM_GAPS.md +++ b/docs/VCRM_GAPS.md @@ -8,24 +8,78 @@ Companion to `docs/VCRM.md`. Lists only the 5 business requirements that are **n ## Executive summary +**Status as at 2026-09-10.** Three of the five gaps below have been closed or +substantially narrowed since this document was first written. The per-gap sections +are kept rather than deleted, each headed with its current status and the evidence +that changed it, so the traceability history stays readable. + | Category | Count | Requirement IDs | |----------|-------|-----------------| -| ⚠️ **Partial coverage** — some aspects verified, some open | 1 | BR-01 | -| ❌ **Genuine gap** — no automated coverage, work needed | 2 | BR-02, BR-15 | -| ❌ **Deferred by design** — out of scope by project decision | 2 | BR-21, BR-22 | -| **Total gaps** | **5 of 22** (23 %) | | +| ✅ **Resolved** — automated coverage now exists | 2 | BR-01, BR-15 | +| ⏸ **Deferred by decision** — out of scope by recorded project decision | 3 | BR-02, BR-21, BR-22 | +| ❌ **Actionable gap** — no coverage and no decision | **0** | — | +| **Total not fully verified** | **3 of 22** (14 %) | all deferred | + +**Coverage from the VCRM:** 19 of 22 (86 %) requirements fully verified by ≥ 1 +automated test condition, up from 17 of 22 (77 %). + +### What changed + +| Requirement | Was | Now | Evidence | +|---|---|---|---| +| **BR-15** conn_limit | No coverage | ✅ Resolved | Assertion **S10** in `tests/suites/test_05_schema_and_business_rules.sql` reads `pg_roles.rolconnlimit` and compares it against the env's `conn_limit`, forwarded via `set_config('te.conn_limit')`. A typo such as `conn_limit=5` in `env_prod.sql` now fails the suite. | +| **BR-01** env isolation | ⚠️ Partial | ✅ Resolved | **Tier E scenario 01** `01_envs_have_identical_structure` compares all four deployed schemas column by column. Logic unit-tested in `tests/test_evals_tier_e_x.py` with negative controls. | +| **BR-02** six engines | ❌ Not verified | ⏸ Deferred by decision | **Tier X scenario 01** `01_sqlite_schema_structure` executes and pins the SQLite schema, and measured a shortfall nobody had counted before. On that evidence a decision was taken to reach a PostgreSQL milestone first — [ADR-0001](adr/0001-postgresql-first.md). Tier X remains in the repo but is out of CI. | + +### BR-02 — deferred by decision (settled 2026-09-10) + +Building Tier X measured what each engine actually defines: + +| Engine | Tables defined | Against PostgreSQL's 12 | +|---|---|---| +| postgresql | 12 | reference | +| sqlite | 7 | 5 missing | +| mariadb | 7 | 5 missing | +| teradata | 1 | 11 missing | +| influxdb | no DDL (seed `.lp` only) | n/a | +| redis | no DDL (seed `.sh` only) | n/a | + +The five absent from SQLite and MariaDB — `temp_documents`, `test_phases`, +`test_events`, `test_results`, `evidence_artifacts` — are the execution and +evidence half of the model. Those engines can hold a test plan and its +traceability but not a record of test execution. + +**Decision taken:** pursue PostgreSQL to a working milestone first, then apply +what that teaches to the other engines. Recorded in +[ADR-0001](adr/0001-postgresql-first.md) with the full rationale, the alternatives +rejected, the accepted risk, and four revisit triggers. -**Coverage from the VCRM:** 17 of 22 (77 %) requirements fully verified by ≥ 1 automated test condition. +BR-02's wording in `docs/VCRM.md` is **retained unchanged** and annotated as +deferred: it states the eventual target, not the current milestone. Its VCRM +status moved from ❌ Not verified to ⏸ Deferred by decision — which is a +different thing, and the distinction is the point. An unverified requirement is a +gap; a deferred one is a choice with a trigger to revisit. -Recommended remediation priority across the 3 actionable gaps (excluding the 2 deferred ones): +### Remaining actionable work -1. **BR-15** — 1 hour, blocks nothing, removes a documented claim with no current test. **Do first.** -2. **BR-01** — 1 day, closes the Tier E gap, raises confidence in non-Dev environments. -3. **BR-02** — 1 day per non-PG engine, but the project README still says "PostgreSQL only" in the eval scope, so timing depends on when multi-DB becomes a real goal. +**No requirement-level work outstanding.** All three not-fully-verified +requirements — BR-02, BR-21, BR-22 — are deferred by recorded decision. + +Two items sit outside the BR set: + +1. **Frontend** — see "Coverage note — frontend" at the end of this document. Not + covered by any BR. +2. **G2** in `docs/GAP_ANALYSIS.md` — Windows CI cannot host PostgreSQL. Accepted; + the ubuntu `integration-postgres` job carries the database-backed suite. --- -## Gap 1 — BR-01 (⚠️ Partial) +## Gap 1 — BR-01 (✅ RESOLVED 2026-09-10) + +> **Closed by Tier E scenario 01** `01_envs_have_identical_structure`. The +> "What's NOT verified" section below is retained as the historical record of +> why the scenario was written; it no longer describes current state. The +> recommended implementation outline further down is what was built. ### Requirement @@ -82,7 +136,16 @@ Recommended remediation priority across the 3 actionable gaps (excluding the 2 d --- -## Gap 2 — BR-02 (❌ Not verified) +## Gap 2 — BR-02 (⏸ DEFERRED BY DECISION 2026-09-10) + +> **Measured by Tier X scenario 01** `01_sqlite_schema_structure`, then +> **deferred by [ADR-0001](adr/0001-postgresql-first.md)**: PostgreSQL first, with +> the lessons applied to other engines afterwards. +> +> The recommendation below — Tier X scenarios per engine, cheapest first — remains +> the right approach for when this resumes, and scenario 01 for SQLite is already +> built. It is simply not scheduled. Tier X stays in the repository but is out of +> CI; run it with `make evals-engines`. ### Requirement @@ -133,7 +196,12 @@ Recommended remediation priority across the 3 actionable gaps (excluding the 2 d --- -## Gap 3 — BR-15 (❌ Not verified) +## Gap 3 — BR-15 (✅ RESOLVED) + +> **Closed by assertion S10** in +> `tests/suites/test_05_schema_and_business_rules.sql`, which reads +> `pg_roles.rolconnlimit` and compares it against the environment's +> `conn_limit`. Implemented as the "even cheaper alternative" sketched below. ### Requirement @@ -260,11 +328,37 @@ Closing the remaining 3 engines of BR-02 (Influx, Redis, Teradata) would take an | 2026-05-26 | Defer performance suite | `evals/HANDOFF.md` | Project team | | 2026-06-01 | BR-15 (conn_limit verification) flagged as cheap actionable gap | this file | (pending) | | 2026-06-01 | BR-01 (cross-env structural parity) flagged as medium-priority gap | this file | (pending) | +| 2026-09-10 | BR-15 resolved — assertion S10 added to SQL suite 05 | `docs/VCRM.md`, `docs/RECONCILED_GAPS.md` | Amarnath | +| 2026-09-10 | BR-01 resolved — Tier E scenario 01 built | `docs/VCRM.md`, `docs/RECONCILED_GAPS.md` | Amarnath | +| 2026-09-10 | **BR-02 deferred by decision — PostgreSQL first, multi-engine after the PG milestone** | [`docs/adr/0001-postgresql-first.md`](adr/0001-postgresql-first.md) | Amarnath | +| 2026-09-10 | Tier X retained in repo but removed from CI, per the decision above | `.github/workflows/*`, `Makefile` (`evals-engines`) | Amarnath | Append to this log whenever a gap is accepted as risk, scheduled for verification, or marked won't-fix. --- +## Coverage note — frontend (not a BR, recorded for completeness) + +`csv-table-hub-main/` has **not been independently audited** and no BR covers it. +Two specific risks follow from that, both recorded rather than measured: + +- **CSV parser parity.** The frontend parses CSV in TypeScript; the backend path + uses `build/csv/validator.py` (shell/eval-covered) and `api/services/csv_parse.py` + (API path). Nothing asserts the implementations agree on delimiter detection, + quoted fields with embedded newlines, BOM handling, or type inference. A file + the browser previews as valid could be rejected server-side, or worse, loaded + with different column typing than the preview showed. +- **CORS wiring end to end.** `settings.CORS_ORIGINS` is env-driven and the + allowed methods and headers are pinned in `api/main.py`, but no test drives a + real browser preflight against a running API. + +Cheapest useful coverage would be a shared fixture set: a directory of CSV files +with one expected-parse JSON each, asserted by both a TypeScript test and a Python +test. That turns parity from an assumption into a check without merging the two +parsers. + +--- + *Companion documents:* - `docs/VCRM.md` — full Verification Cross Reference Matrix (all 22 requirements) diff --git a/docs/adr/0001-postgresql-first.md b/docs/adr/0001-postgresql-first.md new file mode 100644 index 0000000..3742df3 --- /dev/null +++ b/docs/adr/0001-postgresql-first.md @@ -0,0 +1,150 @@ +# ADR-0001 — PostgreSQL first; multi-engine support deferred + +- **Status:** Accepted +- **Date:** 2026-09-10 +- **Decision maker:** Amarnath Rasineni +- **Affects:** BR-02 in `docs/VCRM.md`, Tier X in `evals/`, `build/adapters/*`, `build/schema/*` + +--- + +## Context + +The repository ships adapter scripts and schema directories for six database +engines — PostgreSQL, MariaDB, SQLite, InfluxDB, Redis, Teradata — and BR-02 +states the framework supports all six. Until now that claim rested on code +review: only PostgreSQL was ever executed by a test. + +Building the Tier X eval (`01_sqlite_schema_structure`) made the actual state +measurable for the first time. Counting `CREATE TABLE` statements per engine: + +| Engine | Tables defined | Against PostgreSQL's 12 | +|---|---|---| +| postgresql | 12 | reference | +| sqlite | 7 | 5 missing | +| mariadb | 7 | 5 missing | +| teradata | 1 | 11 missing | +| influxdb | no DDL (seed `.lp` only) | n/a | +| redis | no DDL (seed `.sh` only) | n/a | + +The five tables absent from SQLite and MariaDB are `temp_documents`, +`test_phases`, `test_events`, `test_results` and `evidence_artifacts`. Those are +the execution and evidence half of the T&E model. What is present is the planning +half: organisations, personnel, programs, requirements, test cases, VCRM entries, +defects. So both engines can hold a test plan and its traceability but not a +record of test execution — for a T&E framework, the material half is the one +missing. + +This went unnoticed because `adapter_.sh` deploys whatever DDL it is +handed and reports success on exit 0. With no cross-engine eval, "the adapter +ran" was the only signal available, and it was a true statement about an +incomplete schema. + +## Decision + +**Pursue PostgreSQL to a working milestone first. Defer multi-engine support +until that milestone is reached.** + +BR-02's wording in `docs/VCRM.md` is retained unchanged and annotated as deferred. +It states the eventual target, not the current milestone. + +## Rationale + +The reasoning is depth before breadth: reach a genuine milestone on one engine, +then apply what that teaches to the others. + +Three things support it. + +**The lessons transfer; the guesses don't.** Writing five more DDL variants now +would mean guessing at decisions PostgreSQL has not yet settled — how the +execution tables actually get used, which constraints earn their place, where the +indexes belong. Every one of those guesses would need revisiting, in five places, +once PostgreSQL answered the question. Deferring means porting decisions already +proven rather than propagating provisional ones. + +**Two of the six engines don't translate structurally anyway.** InfluxDB is +time-series and Redis is key-value; "12 tables" is not a meaningful target for +either. Defining what "supported" means for them is design work in its own right, +and it is cheaper once the relational model is stable. + +**Breadth was costing credibility, not buying capability.** Six adapters that +deploy incomplete schemas is weaker than one adapter that is fully verified. The +142-assertion suite, the eval tiers, and the audit trail all exist for +PostgreSQL. Spreading effort across five unverified engines would have diluted +that without making any of them usable. + +The counter-argument, recorded honestly: anyone reading BR-02 or the README today +could reasonably start a MariaDB deployment and hit an incomplete schema. That +risk is real. It is mitigated by documentation — this ADR, the BR-02 annotation, +and `docs/ENGINE_PARITY_TODO.md` — rather than by code, which is a weaker +mitigation than removing the adapters would be. Removing them was rejected +because they are a useful starting point for the later work and deleting them +would discard effort already spent. + +## Alternatives considered + +**Write the missing DDL for SQLite and MariaDB now** (~2 days). Rejected: it +propagates unsettled modelling decisions into two more places, as above. + +**Delete the five non-PostgreSQL adapters and schema directories.** Rejected: +strongest possible protection against someone starting an unsupported +deployment, but discards work that will be the basis for the later effort, and +makes the eventual resumption more expensive than it needs to be. + +**Restate BR-02 as PostgreSQL-only, dropping the six-engine target.** Rejected: +the multi-engine ambition is real and should stay tracked. A requirement annotated +as deferred keeps it visible; a deleted one quietly stops being anyone's problem. + +## Consequences + +**Immediate** + +- BR-02 moves from ❌ Not verified to ⏸ Deferred by decision. VCRM coverage is + now 19 of 22 (86 %) verified, with **no unverified requirement that is not a + recorded decision** — the remaining three are BR-02, BR-21 and BR-22, all + deferred deliberately. +- Tier X scenario `01_sqlite_schema_structure` and its unit tests **stay in the + repository** but are **removed from the CI workflows**, so the pipeline is + purely PostgreSQL. The scenario still passes when run manually + (`make evals-engines`, or `python3 evals/runner.py --tiers x`). +- Tier E is unaffected and stays in CI: cross-environment parity is PostgreSQL + work and part of the current milestone. + +**When multi-engine work resumes** + +- Re-add `x` to the eval tiers in `.github/workflows/quality-gate.yml` and + `.github/workflows/python-validator-tests.yml`. Tier X needs no database + service, so it can run in the free tier and on the Windows runner. +- Work through the per-engine questions in `docs/ENGINE_PARITY_TODO.md`, + starting with target scope per engine. +- As each SQLite table is written, move it from `known_missing_tables` into + `expected_tables` in + `evals/expected/tier_x/01_sqlite_schema_structure.json`. Tier X will fail until + you do — `tests/test_evals_tier_e_x.py` asserts the two lists together account + for all 12 tables, so the shortfall cannot be understated by omission. +- MariaDB is the cheapest next engine: already 7 tables in, and + `docker run mariadb` is roughly a ten-minute setup. + +**Accepted risk** + +Someone could follow the README, pick a non-PostgreSQL engine, and hit an +incomplete schema. Mitigated by documentation only. If this materialises — or if +a non-PostgreSQL engine becomes a delivery commitment — revisit this ADR rather +than patching around it. + +## Revisit triggers + +Any one of these should reopen the decision: + +1. The PostgreSQL milestone is reached — the trigger this decision anticipates. +2. A non-PostgreSQL engine becomes a commitment to a real user or customer. +3. Anyone reports attempting a non-PostgreSQL deployment and hitting the gap. +4. BR-02's wording is challenged in a review as claiming more than the build + delivers. + +## References + +- `docs/ENGINE_PARITY_TODO.md` — per-engine measured state and open questions +- `docs/VCRM.md` — BR-02, annotated +- `docs/VCRM_GAPS.md` — BR-02 gap section +- `evals/datasets/tier_x/01_sqlite_schema_structure/NOTES.txt` — what Tier X does and its scope limits +- `evals/PLAN.md`, `evals/FAILURE_MODES.md` — the earlier, implicit "deferred until PG is locked in" note this ADR makes explicit diff --git a/docs/reconciled_gaps.csv b/docs/reconciled_gaps.csv new file mode 100644 index 0000000..8d76185 --- /dev/null +++ b/docs/reconciled_gaps.csv @@ -0,0 +1,20 @@ +id,gap,status,fixed_by,evidence,decision_needed,notes +O-01,config.env.example names mismatch loaders and setup.sh,Already closed,Pre-existing (2026-08-12),GAP_ANALYSIS G1; verified both use PG_*_ scheme,No,Was listed open only because the earlier register used the 7z snapshot +O-02,BR-15 conn_limit not asserted against pg_roles,Already closed,Pre-existing,Assertion S10 in tests/suites/test_05_schema_and_business_rules.sql,No,Reads pg_roles.rolconnlimit per environment +O-03,No auth fail-closed and no rate limiting,Fixed this pass,This pass,api/ratelimit.py + API_ENV gate in api/main.py; 24 tests in tests/test_api_hardening.py,No,Dependency-free sliding window; API_ENV != dev with empty API_KEY now refuses to start +O-04,audit_log written on delete only,Fixed this pass,This pass,api/services/audit.py; upload path audited; delete refactored onto shared writer,No,Known limitation: upload audit is post-commit (documented in module docstring) +O-05,No migration history endpoint,Already closed,Pre-existing,GET /api/csv/files in csv_routes.py; pagination in commit 269ffe2,No, +O-06,No dedup layer,Already closed,Pre-existing,Three levels in dynamic_loader.py: filename, content SHA-256, per-row _row_hash UNIQUE,No, +O-07,BR-01 cross-environment parity unverified,Fixed this pass,This pass,Tier E scenario 01_envs_have_identical_structure; 10 unit tests; wired into integration CI,No,Full 4-env run not yet executed (no PostgreSQL this session); logic unit-tested with negative controls +O-08,BR-02 non-PG engines unverified,Deferred by decision,This pass (measured) + ADR-0001 (deferred),Tier X scenario 01_sqlite_schema_structure measured the shortfall; docs/adr/0001-postgresql-first.md defers multi-engine,No,PostgreSQL first then apply lessons to other engines. Tier X kept in repo but out of CI (make evals-engines) +O-09,Windows CI cannot host PostgreSQL,Accepted and improved,This pass,GAP_ANALYSIS G2 accepted; Tier X now runs on the Windows job,No,First cross-engine coverage that job has carried +O-10,Runtime artifacts not gitignored,Already closed,Pre-existing (2026-08-13),GAP_ANALYSIS G4; all four entries present plus evals/reports/,No, +O-11,VCRM BR-20 count edited 85 to 142,Already closed,Pre-existing,GAP_ANALYSIS G5; confirmed at 142 matching Tier S expectation JSON,No, +O-12,Frontend not independently audited,Documented this pass,This pass,Coverage note in docs/VCRM_GAPS.md naming CSV parser parity and CORS preflight risks,No,Cheapest remedy proposed: shared fixture set asserted by both TS and Python tests +F-01,Engine parity shortfall across 5 non-PG engines,Measured then deferred,This pass,Counted CREATE TABLE per engine in build/schema/*/te_core_schema.sql,No,Deferred by ADR-0001. BR-02 wording retained and annotated deferred; VCRM now 19/22 verified with zero actionable gaps +F-02,api/auth.py docstring claimed global application in main.py,Fixed this pass,This pass,Docstring corrected; enforcement is per-router via dependencies=[Depends(require_api_key)],No,Enforcement was real; only the documentation was wrong +F-03,scripts/lint.sh hardcoded file list let new modules escape flake8 and bandit,Fixed this pass,This pass,Added api/ratelimit.py api/services/audit.py and both new test modules,No, +F-04,docs/VCRM_GAPS.md stale — BR-15 fixed in code without doc update,Fixed this pass,This pass,Status headers and what-changed table added; coverage restated 77% to 86%,No, +F-05,audit_log had a single writer so log recorded destructions not creations,Fixed this pass,This pass,Same fix as O-04,No, +F-06,Stale git worktree .claude/worktrees/velvety-chasing-badger holds older copy of api/,Flagged only,Not touched,Appears in repo-wide grep results with its own auth.py and routers/,YES,Prune if dead; it pollutes searches +D-01,PostgreSQL-first decision recorded as ADR,Recorded,This pass,docs/adr/0001-postgresql-first.md,No,Context + rationale + alternatives rejected + accepted risk + 4 revisit triggers. Tier X dropped from CI and Makefile evals-engines target added From 80e55da4386447d48bea1b2ad51e41008d5fdaf2 Mon Sep 17 00:00:00 2001 From: amar-python Date: Thu, 10 Sep 2026 14:13:08 +1000 Subject: [PATCH 5/8] docs(evals): annotate PLAN/HANDOFF/FAILURE_MODES for five tiers; fix a wrong test name Three eval docs still described Tiers E and X as deferred or future after both were implemented. Annotated rather than rewritten: each gets a dated status banner and the affected lines are struck through with the current position beside them, so the record of what was planned -- including HANDOFF.md's quote of the original 'ignore them for now' instruction -- stays intact while the docs stop misleading. Tier D remains genuinely deferred and is left alone. docs/TEST_CONDITIONS.md gains Tier E and X rows in its at-a-glance table. Its counts for the pre-existing categories were not re-measured in this pass, and the intro now says so rather than implying they were checked. One row in TEST_CONDITIONS.md was actively wrong rather than merely stale: it documented test_tier_i_skips_when_postgresql_is_unavailable as returning skipped=True. That test no longer exists -- fix F5 inverted the contract and renamed it to test_tier_i_fails_when_postgresql_is_unavailable, which asserts skipped is False and passed is False. Corrected, and the correction is noted inline. Other pre-existing staleness in these files (the phase table, an 85-assertion reference, file counts, the unit-test total) was found but deliberately left, to keep this diff reviewable. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01V1v7ow7NxH9y1ELUyGgACn --- docs/TEST_CONDITIONS.md | 8 ++++++-- evals/FAILURE_MODES.md | 10 ++++++++-- evals/HANDOFF.md | 21 ++++++++++++++++++--- evals/PLAN.md | 28 +++++++++++++++++++++++----- 4 files changed, 55 insertions(+), 12 deletions(-) diff --git a/docs/TEST_CONDITIONS.md b/docs/TEST_CONDITIONS.md index d9a9e44..1eefcdc 100644 --- a/docs/TEST_CONDITIONS.md +++ b/docs/TEST_CONDITIONS.md @@ -1,6 +1,8 @@ # TEST_CONDITIONS — comprehensive catalog -Every test condition that runs against the codebase, in one place. Five categories: Python unit tests, SQL test-suite assertions, and Tier P / I / S evals. +Every test condition that runs against the codebase, in one place. Python unit tests, SQL test-suite assertions, and Tier P / I / S / E / X evals. + +> **Updated 2026-09-10.** Tiers **E** (cross-environment structural parity) and **X** (cross-engine structural coverage) were added. Tier E runs in CI; Tier X is out of CI by [ADR-0001](adr/0001-postgresql-first.md) and runs via `make evals-engines`. Counts for the pre-existing categories are as previously recorded and were not re-measured in this pass. ## At a glance @@ -11,6 +13,8 @@ Every test condition that runs against the codebase, in one place. Five categori | Tier P eval scenarios | 23 | `evals/datasets/tier_p/` | `evals/runner.py` | | Tier I eval scenarios | 1 | `evals/datasets/tier_i/` | `evals/runner.py` | | Tier S eval scenarios | 1 | `evals/datasets/tier_s/` | `evals/runner.py` | +| Tier E eval scenarios | 1 | `evals/datasets/tier_e/` | `evals/runner.py` | +| Tier X eval scenarios | 1 | `evals/datasets/tier_x/` | `evals/runner.py` (not in CI — ADR-0001) | --- @@ -35,7 +39,7 @@ Located in `tests/`, run as `python -m unittest discover -s tests -p "test*.py"` | 6 | `test_discover_scenarios_filters_only_requested_name` | `--only ` filter returns just the named scenario | | 7 | `test_tier_p_reports_unknown_runner_action_as_failure` | Unknown `runner_action` value produces a failed scenario, not a crash | | 8 | `test_tier_p_generated_invalid_utf8_fails_cleanly` | Invalid-UTF8 generated scenario exits 1 with no traceback | -| 9 | `test_tier_i_skips_when_postgresql_is_unavailable` | Tier I returns `skipped=True` when psql/PG isn't reachable | +| 9 | `test_tier_i_fails_when_postgresql_is_unavailable` | Tier I **fails** (not skips) when psql/PG isn't reachable — asserts `result.skipped` is False and `result.passed` is False. An unavailable prerequisite is a failure, so a green run always means the scenario really executed. *(Corrected 2026-09-10: this row previously named a test that no longer exists and stated the opposite contract; the behaviour was inverted by fix F5 in `FIXES_APPLIED.md`.)* | | 10 | `test_count_dev_rows_records_none_for_failed_count_query` | `_count_dev_rows` records `None` per table on query failure | | 11 | `test_have_psql_uses_path_lookup` | `_have_psql()` uses `shutil.which("psql")` | diff --git a/evals/FAILURE_MODES.md b/evals/FAILURE_MODES.md index 75e264f..c68bd05 100644 --- a/evals/FAILURE_MODES.md +++ b/evals/FAILURE_MODES.md @@ -70,9 +70,15 @@ Tier S initial scope: only S1. ## What this catalogue does NOT yet cover -- **Multi-DB equivalence** (cross-engine schema parity) — deferred until PG is locked in. +- **Multi-DB equivalence** (cross-engine schema parity) — ~~deferred until PG is locked in.~~ + *Updated 2026-09-10:* partially covered. Tier X scenario 01 executes and pins the + SQLite schema. Full parity is deferred by + [ADR-0001](../docs/adr/0001-postgresql-first.md), and four of the six engines have + incomplete or absent DDL — see `../docs/ENGINE_PARITY_TODO.md`. Tier X is out of CI. - **Performance / scale** (1M-row load timing) — separate suite if needed later. -- **Cross-environment structural equivalence** (Dev vs Test vs Staging vs Prod) — Tier E, future. +- ~~**Cross-environment structural equivalence** (Dev vs Test vs Staging vs Prod) — Tier E, future.~~ + *Now covered as of 2026-09-10:* Tier E scenario 01 compares all four deployed + schemas column by column and treats reordered columns as drift. Closes VCRM BR-01. - **Domain-rule deep dives beyond suite 05** — Tier D, future. - **Validator behaviour on >128KB single field** — beyond the current 50KB eval and Python `csv` default field-size assumptions. diff --git a/evals/HANDOFF.md b/evals/HANDOFF.md index 464fbce..0f2f5f6 100644 --- a/evals/HANDOFF.md +++ b/evals/HANDOFF.md @@ -1,5 +1,20 @@ # Hand-off summary — evals/ package +> **Status update — 2026-09-10.** Tiers **E** and **X** are no longer deferred: +> both are implemented and have executing scenarios. The text below is preserved +> as the record of what was planned at the time and is **not** a description of +> current state. Affected lines are marked inline. +> +> - **Tier E** — implemented. `evals/datasets/tier_e/01_envs_have_identical_structure`. +> Runs in the `integration-postgres` CI job. Closes VCRM BR-01. +> - **Tier X** — implemented. `evals/datasets/tier_x/01_sqlite_schema_structure`. +> Deliberately **not** in CI per [ADR-0001](../docs/adr/0001-postgresql-first.md) +> (PostgreSQL first); run it with `make evals-engines`. +> - **Tier D** — still deferred. Unchanged. +> +> Current tier registrations live in `runner.TIER_RUNNERS`: `p, i, s, e, x`. + + **Date:** 2026-05-26 **Scope this round:** PostgreSQL only (Tiers P + I + S). MariaDB / SQLite / etc. deferred. @@ -94,8 +109,8 @@ Tier I and S can be added when you have PostgreSQL in your CI runner. | Item | Why deferred | |------|--------------| -| Tier X — cross-DB schema equivalence (MariaDB / SQLite / Teradata) | You explicitly said "ignore them for now" | -| Tier E — cross-environment (Dev/Test/Staging/Prod) structural parity | Not needed until you ship beyond Dev | +| ~~Tier X — cross-DB schema equivalence (MariaDB / SQLite / Teradata)~~ **Built 2026-09-10 (SQLite)** | Originally: you explicitly said "ignore them for now". Now: SQLite scenario exists; deferred from CI by ADR-0001, which records the same intent as a formal decision | +| ~~Tier E — cross-environment (Dev/Test/Staging/Prod) structural parity~~ **Built 2026-09-10** | Originally: not needed until you ship beyond Dev. Now: implemented and running in the `integration-postgres` job | | Tier D — extended domain-rule evals beyond suite 05 | Existing 142 assertions already cover the high-value rules | | Performance / scale (1M+ rows) | Would need a fixture generator — separate round | | AI-assisted anomaly detection | Out of scope for deterministic evals | @@ -119,6 +134,6 @@ Catalogued in `FAILURE_MODES.md` so they're not lost. Nothing blocking. If you want me to take the next step, ask for: - "extend Tier P to N more scenarios" (e.g. RTL, very-long-row, latin-1) -- "add Tier X — cross-DB equivalence" once you have other DB engines installed +- ~~"add Tier X — cross-DB equivalence" once you have other DB engines installed~~ — the SQLite scenario is built; the remaining work is DDL parity per engine, tracked in `../docs/ENGINE_PARITY_TODO.md` - "wire the evals into the GitHub Actions workflow" - "build a sample-data generator that produces 100 K / 1 M row CSVs for perf" diff --git a/evals/PLAN.md b/evals/PLAN.md index 060331c..fcff512 100644 --- a/evals/PLAN.md +++ b/evals/PLAN.md @@ -1,5 +1,20 @@ # Evals package — PostgreDataMigrationApp +> **Status update — 2026-09-10.** Tiers **E** and **X** are no longer deferred: +> both are implemented and have executing scenarios. The text below is preserved +> as the record of what was planned at the time and is **not** a description of +> current state. Affected lines are marked inline. +> +> - **Tier E** — implemented. `evals/datasets/tier_e/01_envs_have_identical_structure`. +> Runs in the `integration-postgres` CI job. Closes VCRM BR-01. +> - **Tier X** — implemented. `evals/datasets/tier_x/01_sqlite_schema_structure`. +> Deliberately **not** in CI per [ADR-0001](../docs/adr/0001-postgresql-first.md) +> (PostgreSQL first); run it with `make evals-engines`. +> - **Tier D** — still deferred. Unchanged. +> +> Current tier registrations live in `runner.TIER_RUNNERS`: `p, i, s, e, x`. + + This folder holds **data-driven evals** for the T&E Database Framework. Evals complement (but don't replace) the existing SQL test suites under `tests/suites/` and the Python unit tests under `tests/test_csv_validator.py`. ## Why a separate `evals/` folder @@ -22,10 +37,12 @@ In short: `tests/` proves the **code is correct**; `evals/` proves the **framewo - **Tier P** — Python CSV validator (`build/csv/validator.py`). Pure data-in / files-out. No DB. - **Tier I** — Idempotency of `deploy_all.sh` against a clean Dev PostgreSQL. - **Tier S** — SQL test suite integration: deploy fresh + run all 5 suites and assert 142/142. -- **Tiers deferred:** - - **Tier X** — Cross-DB schema equivalence (MariaDB/SQLite). Out until Postgres is locked in. - - **Tier E** — Cross-environment (Dev/Test/Staging/Prod) structural equivalence. - - **Tier D** — Extended domain-rule evals beyond what suite 05 already covers. +- **Tiers deferred:** *(superseded 2026-09-10 — see the status update at the top)* + - ~~**Tier X** — Cross-DB schema equivalence (MariaDB/SQLite). Out until Postgres is locked in.~~ + **Implemented**, but kept out of CI by ADR-0001 rather than unbuilt. + - ~~**Tier E** — Cross-environment (Dev/Test/Staging/Prod) structural equivalence.~~ + **Implemented** and running in CI. + - **Tier D** — Extended domain-rule evals beyond what suite 05 already covers. *Still deferred.* ## Folder layout @@ -117,7 +134,8 @@ Exit code: 0 if all scenarios in selected tiers pass, 1 otherwise. CI-friendly. | 3 | Execute Tier P locally; show results | DONE / awaiting your review | | 4 | Tier I scaffolding + runner extension | next | | 5 | Tier S scaffolding + runner extension | next | -| 6 | (Future) Tier X across MariaDB/SQLite once Postgres is locked in | deferred | +| 6 | (Future) Tier X across MariaDB/SQLite once Postgres is locked in | ~~deferred~~ **SQLite scenario built 2026-09-10**; out of CI per ADR-0001 | +| 7 | Tier E cross-environment structural parity | **DONE 2026-09-10** — in the `integration-postgres` job | ## What this DOES NOT do From 639265011b2f94e8f4e8ecbe2b44e0d004c01b64 Mon Sep 17 00:00:00 2001 From: amar-python Date: Thu, 10 Sep 2026 14:13:09 +1000 Subject: [PATCH 6/8] test(api): add integration coverage for the audit_log table (O-04) The upload audit added in e0339cb had only unit coverage against a mock cursor -- nothing asserted a row actually lands in csv_uploads.audit_log. Six tests in a new AuditLogTrail class close that: a successful upload writes exactly one 'upload' row; its mode, tableName and fileId match the upload response; upload-then-delete leaves both rows; a refused duplicate adds no second row; a failed upload adds none; and the endpoint honours its total/limit contract. Deliberately a separate class rather than additions to CsvPipelineWithDatabase, because that class's 'BR-15 audit trail' tests read GET /api/csv/files -- the upload registry (csv_files), not audit_log. Two different tables with a confusingly similar name. The new class docstring spells out the distinction so the next reader does not have to rediscover it. setUpClass enables settings.allow_destructive and restores it after, matching the pattern already used in test_api_coverage.py and test_issue_05_import_summary.py. Without it the delete half of the trail could not be exercised at all, and tearDown's cleanup would silently 403 and leave rows for the next run to trip over. audit_routes.py's docstring said the feed existed so 'the deletion trail' is consultable, which stopped being accurate once uploads started writing to the table. Updated. Not yet executed: these are integration tests and no PostgreSQL was available in this session. They collect cleanly, lint clean, and the DB-free suite is unaffected at 115 passing. Their first real run will be 'pytest -m integration'. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01V1v7ow7NxH9y1ELUyGgACn --- api/routers/audit_routes.py | 7 +- tests/test_api_integration.py | 160 ++++++++++++++++++++++++++++++++++ 2 files changed, 166 insertions(+), 1 deletion(-) diff --git a/api/routers/audit_routes.py b/api/routers/audit_routes.py index 507e7ee..7f58ab1 100644 --- a/api/routers/audit_routes.py +++ b/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 diff --git a/tests/test_api_integration.py b/tests/test_api_integration.py index 5dc8d90..13eaabb 100644 --- a/tests/test_api_integration.py +++ b/tests/test_api_integration.py @@ -314,6 +314,166 @@ def test_failed_upload_leaves_no_audit_record(self): self.assertIsNone(record, "A failed upload must not leave a registry entry") +@pytest.mark.integration +class AuditLogTrail(unittest.TestCase): + """Coverage for the real ``csv_uploads.audit_log`` table (O-04). + + Distinct from the "BR-15 audit trail" tests in ``CsvPipelineWithDatabase`` + above: those read ``GET /api/csv/files``, which is the upload *registry* + (``csv_files``). This class reads ``GET /api/audit/log``, which is the + ``audit_log`` table — a different table with a different purpose. The naming + overlap is pre-existing and easy to trip over. + + Why these tests exist: until O-04, ``audit_log`` had exactly one writer + (``delete_file``), so it recorded destructions but never creations. Uploads + now write to it as well, and that behaviour had only unit coverage against a + mock cursor — nothing asserted a row actually lands. + """ + + @classmethod + def setUpClass(cls): + # DELETE is gated by API_ALLOW_DESTRUCTIVE, which defaults to false + # (BUG-040). Enable it for this class rather than requiring an env var, + # matching the pattern in test_api_coverage.py and + # test_issue_05_import_summary.py — otherwise the delete half of the + # trail could not be tested, and tearDown's cleanup would silently 403 + # and leave rows behind for the next run to trip over. + from api.config import settings as _settings + cls._settings = _settings + cls._orig_allow_destructive = _settings.allow_destructive + _settings.allow_destructive = True + + cls.ctx = TestClient(app) + try: + cls.client = cls.ctx.__enter__() + except Exception as exc: # noqa: BLE001 + _settings.allow_destructive = cls._orig_allow_destructive + raise AssertionError( + f"Could not start the API — database connection failed.\n" + f" {type(exc).__name__}: {str(exc).strip().splitlines()[0]}\n" + f"{_HELP}" + ) from None + if cls.client.get("/api/health").json().get("status") != "ok": + cls.ctx.__exit__(None, None, None) + _settings.allow_destructive = cls._orig_allow_destructive + raise AssertionError(f"API reports the database is unreachable. {_HELP}") + + @classmethod + def tearDownClass(cls): + cls.ctx.__exit__(None, None, None) + cls._settings.allow_destructive = cls._orig_allow_destructive + + def setUp(self): + # Unique per test so the content-hash dedup cannot collide with rows a + # previous run left behind. + self.name = f"audit_{os.getpid()}_{self._testMethodName}.csv" + self.content = f"col_a,col_b\n{os.getpid()},{self._testMethodName}\n" + + def tearDown(self): + for f in self.client.get("/api/csv/files").json(): + if f["file_name"] == self.name: + self.client.delete(f"/api/csv/files/{f['id']}") + + def _entries_for(self, file_name: str) -> list[dict]: + """Audit entries for one file name, newest first. + + limit is set near the endpoint's 200 cap because audit_log accumulates + across runs and the default of 50 could push a fresh row off the page. + """ + resp = self.client.get("/api/audit/log", params={"limit": 200}) + self.assertEqual(resp.status_code, 200) + body = resp.json() + self.assertIn("entries", body) + return [e for e in body["entries"] if e.get("fileName") == file_name] + + def test_successful_upload_writes_an_audit_row(self): + """O-04: the gap this closes — a load used to leave no audit trace.""" + up = self.client.post("/api/csv/upload", + json={"fileName": self.name, "content": self.content}) + self.assertEqual(up.json()["status"], "ok", up.text) + + uploads = [e for e in self._entries_for(self.name) if e["action"] == "upload"] + self.assertEqual( + len(uploads), 1, + f"expected exactly one 'upload' audit row for {self.name}, got {len(uploads)}", + ) + + def test_audit_row_fields_match_the_upload_response(self): + """The row must identify what was loaded and where, not merely that + something happened — an audit trail that cannot be tied back to the + artefact it describes is not evidence.""" + up = self.client.post("/api/csv/upload", + json={"fileName": self.name, "content": self.content}) + body = up.json() + self.assertEqual(body["status"], "ok", up.text) + + row = next(e for e in self._entries_for(self.name) if e["action"] == "upload") + self.assertEqual(row["mode"], "dynamic") + self.assertEqual(row["tableName"], body["tableName"]) + # fileId is coerced from the loader's string form into the BIGINT column. + self.assertEqual(str(row["fileId"]), str(body["fileId"])) + self.assertTrue(row["performedAt"], "performedAt must not be empty") + + def test_upload_then_delete_leaves_both_rows(self): + """The full lifecycle. Before O-04 only the second half was recorded, + so a table could be seen to vanish with no record of its arrival.""" + up = self.client.post("/api/csv/upload", + json={"fileName": self.name, "content": self.content}) + self.assertEqual(up.json()["status"], "ok", up.text) + + file_id = next(f["id"] for f in self.client.get("/api/csv/files").json() + if f["file_name"] == self.name) + delete = self.client.delete(f"/api/csv/files/{file_id}") + # setUpClass enables API_ALLOW_DESTRUCTIVE for this class, so a 403 here + # means the gate itself changed, not that the environment is misconfigured. + self.assertEqual( + delete.status_code, 200, + f"delete was refused despite the class enabling it: {delete.text}", + ) + + actions = [e["action"] for e in self._entries_for(self.name)] + self.assertIn("upload", actions) + self.assertIn("delete", actions) + + def test_duplicate_upload_writes_no_second_audit_row(self): + """Only a genuine load is audited. A refused duplicate changed nothing, + so logging it would make the trail read as if data had landed twice.""" + first = self.client.post("/api/csv/upload", + json={"fileName": self.name, "content": self.content}) + self.assertEqual(first.json()["status"], "ok", first.text) + + second = self.client.post("/api/csv/upload", + json={"fileName": self.name, "content": self.content}) + self.assertEqual(second.json()["status"], "duplicate_file", second.text) + + uploads = [e for e in self._entries_for(self.name) if e["action"] == "upload"] + self.assertEqual( + len(uploads), 1, + "a refused duplicate must not add a second 'upload' row", + ) + + def test_failed_upload_writes_no_audit_row(self): + """A header-only file is rejected before any rows land, so there is + nothing to audit.""" + bad = f"audit_fail_{os.getpid()}.csv" + resp = self.client.post("/api/csv/upload", + json={"fileName": bad, "content": "col_a\n"}) + self.assertIn(resp.json()["status"], ("invalid_structure", "error")) + self.assertEqual( + self._entries_for(bad), [], + "a failed upload must not leave an audit row", + ) + + def test_audit_endpoint_reports_a_total_and_respects_limit(self): + """Pagination contract, so a grower table stays consultable.""" + self.client.post("/api/csv/upload", + json={"fileName": self.name, "content": self.content}) + body = self.client.get("/api/audit/log", params={"limit": 1}).json() + self.assertGreaterEqual(body["total"], 1) + self.assertEqual(body["limit"], 1) + self.assertLessEqual(len(body["entries"]), 1) + + @pytest.mark.integration class PoolTimeout(unittest.TestCase): """BUG-030 regression: an exhausted pool must 503 within POOL_GETCONN_TIMEOUT From 704b97b1abd3bdba0f609f000effbbae8a5d4648 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 25 Oct 2026 21:16:15 +1100 Subject: [PATCH 7/8] fix(ci): make the integration job's Tier E and API tests query the right place (#45) Two independent causes, both reproduced locally against PostgreSQL 16 with the job's exact steps: 1. Tier E bound the schema name with `psql --set ... -c`. psql never interpolates :'var' inside -c, so every query was a syntax error, which _column_signature turned into an empty result: "0 tables". The query now goes on stdin, and a failed query is reported as a query error instead of an empty schema. Three negative controls; two of them fail against the old code. 2. The job sets PGDATABASE=postgres, and api/config.py reads PGDATABASE, so the API looked for te_dev in the wrong database. The full-suite step now sets PGDATABASE=te_mgmt_dev. With Tier E working it found one real difference: dev has test_run_results, created by the Tier S SQL test harness. It is listed in the Tier E expectation file as ignored, with the reason, rather than filtered silently. Local replay: evals P/I/S/E 26/26, test_report --strict 239/239, 0 skipped; make lint clean. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01AYM3RR8Fmw2pwqFfnJhAo6 --- .github/workflows/quality-gate.yml | 4 ++ .../01_envs_have_identical_structure.json | 2 + evals/runner.py | 31 ++++++++--- tests/test_evals_tier_e_x.py | 55 +++++++++++++++++++ 4 files changed, 85 insertions(+), 7 deletions(-) diff --git a/.github/workflows/quality-gate.yml b/.github/workflows/quality-gate.yml index 9a8cf79..821ae9a 100644 --- a/.github/workflows/quality-gate.yml +++ b/.github/workflows/quality-gate.yml @@ -158,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 diff --git a/evals/expected/tier_e/01_envs_have_identical_structure.json b/evals/expected/tier_e/01_envs_have_identical_structure.json index adb8904..0d9bc45 100644 --- a/evals/expected/tier_e/01_envs_have_identical_structure.json +++ b/evals/expected/tier_e/01_envs_have_identical_structure.json @@ -3,6 +3,8 @@ "description": "Deploy all four environments and assert their deployed schemas are structurally identical; only the database/schema namespace may differ. Closes VCRM BR-01.", "envs_under_test": ["dev", "test", "staging", "prod"], "reference_env": "dev", + "ignore_tables": ["test_run_results"], + "ignore_tables_reason": "Created in te_dev by tests/framework/test_framework.sql when Tier S runs; test-harness state, not deployed schema.", "expected": { "all_envs_deploy_exit_zero": true, "column_signature_identical_across_envs": true, diff --git a/evals/runner.py b/evals/runner.py index c191a49..cf82dc3 100644 --- a/evals/runner.py +++ b/evals/runner.py @@ -596,8 +596,9 @@ def _column_signature(db: str, schema: str) -> Dict[str, List[str]]: different namespaces compare equal when their structure matches. """ # The schema name is bound by psql via --set + :'sch' rather than pasted into - # the SQL text, so there is no string interpolation to reason about even - # though the value comes from the ENV_TARGETS constant. + # the SQL text. psql only interpolates variables in SQL it reads from a file + # or stdin, never in -c, so the query goes on stdin (issue #45: under -c it + # was a syntax error that this function used to report as "0 tables"). query = ( "SELECT table_name, column_name, data_type, is_nullable " "FROM information_schema.columns " @@ -605,11 +606,12 @@ def _column_signature(db: str, schema: str) -> Dict[str, List[str]]: "ORDER BY table_name, ordinal_position" ) r = subprocess.run( - ["psql", "-tA", "-d", db, "--set=sch=" + schema, "-c", query], - env=_pg_env(), capture_output=True, text=True, timeout=30, + ["psql", "-tA", "-v", "ON_ERROR_STOP=1", "-d", db, "--set=sch=" + schema], + input=query, env=_pg_env(), capture_output=True, text=True, timeout=30, ) if r.returncode != 0: - return {} + raise RuntimeError("schema query on " + db + "." + schema + " failed: " + + r.stderr.strip()[-300:]) sig: Dict[str, List[str]] = {} for line in r.stdout.splitlines(): parts = line.split("|") @@ -689,16 +691,31 @@ def _run_envs_have_identical_structure( deploys = [_deploy_env(env_name) for env_name, _, _ in targets] signatures: Dict[str, Dict[str, List[str]]] = {} + query_errors: List[str] = [] for env_name, db, schema in targets: - signatures[env_name] = _column_signature(db, schema) + try: + signatures[env_name] = _column_signature(db, schema) + except RuntimeError as exc: + signatures[env_name] = {} + query_errors.append("env " + env_name + ": " + str(exc)) + + # Tables the SQL test harness creates in the schema it tests (Tier S runs + # tests/framework/test_framework.sql in te_dev). Not deployed schema, so + # not drift. Listed in the expectation file, never silently. + ignored = set(expected.get("ignore_tables", [])) + for sig in signatures.values(): + for table in ignored: + sig.pop(table, None) table_counts = {e: len(sig) for e, sig in signatures.items()} result.actual = { "deploys": deploys, "table_counts": table_counts, + "ignored_tables": sorted(ignored), } - errors: List[str] = [] + # A failed query is not an empty schema; report it as what it is. + errors: List[str] = list(query_errors) if exp.get("all_envs_deploy_exit_zero") is True: for d in deploys: diff --git a/tests/test_evals_tier_e_x.py b/tests/test_evals_tier_e_x.py index 1088f84..b74d2f1 100644 --- a/tests/test_evals_tier_e_x.py +++ b/tests/test_evals_tier_e_x.py @@ -9,8 +9,10 @@ from __future__ import annotations +import subprocess import sys import unittest +from unittest.mock import patch from pathlib import Path import pytest @@ -83,6 +85,59 @@ def test_env_targets_cover_all_four_environments(self): self.assertEqual(len({e[2] for e in runner.ENV_TARGETS}), 4) +@pytest.mark.unit +class TierEColumnSignatureQuery(unittest.TestCase): + """Issue #45: the schema query itself, not just the comparison. + + psql never interpolates :'var' inside -c, so the query must go on stdin, and + a failed query must not be reported as an empty schema. + """ + + def _scenario(self, completed): + expected = {"expected": {"min_tables_per_env": 12}, + "envs_under_test": ["dev"]} + result = runner.ScenarioResult(tier="e", name="t") + with patch.object(runner, "_deploy_env", + return_value={"env": "dev", "exit_code": 0}), \ + patch.object(runner.subprocess, "run", return_value=completed) as run: + runner._run_envs_have_identical_structure(result, expected) + return result, run + + def test_query_is_sent_on_stdin_not_via_dash_c(self): + ok = subprocess.CompletedProcess([], 0, stdout="t|c|text|NO\n", stderr="") + _, run = self._scenario(ok) + args, kwargs = run.call_args + self.assertNotIn("-c", args[0]) + self.assertIn(":'sch'", kwargs["input"]) + + def test_query_failure_is_reported_as_a_query_error(self): + bad = subprocess.CompletedProcess([], 1, stdout="", + stderr='ERROR: syntax error at or near ":"') + result, _ = self._scenario(bad) + self.assertFalse(result.passed) + self.assertTrue(any("schema query" in e and "syntax error" in e + for e in result.errors), result.errors) + + def test_only_listed_harness_tables_are_ignored(self): + two = subprocess.CompletedProcess( + [], 0, stdout="test_run_results|id|bigint|NO\nstray|id|bigint|NO\n", stderr="") + expected = {"expected": {"min_tables_per_env": 1}, + "envs_under_test": ["dev"], "ignore_tables": ["test_run_results"]} + result = runner.ScenarioResult(tier="e", name="t") + with patch.object(runner, "_deploy_env", + return_value={"env": "dev", "exit_code": 0}), \ + patch.object(runner.subprocess, "run", return_value=two): + runner._run_envs_have_identical_structure(result, expected) + self.assertEqual(result.actual["table_counts"], {"dev": 1}) + + def test_genuinely_empty_schema_still_fails(self): + empty = subprocess.CompletedProcess([], 0, stdout="", stderr="") + result, _ = self._scenario(empty) + self.assertFalse(result.passed) + self.assertIn("env dev has 0 tables in its schema, expected >= 12 " + "(deploy may have partially failed)", result.errors) + + @pytest.mark.unit class TierXSqliteStructure(unittest.TestCase): """O-08: SQLite DDL execution and structural readback.""" From bc6ff27edc685d8bf252f68e19febd264080b296 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 25 Oct 2026 21:42:05 +1100 Subject: [PATCH 8/8] fix(tests): keep SQL harness objects out of the schema under test (#45) Replaces the ignore_tables exception from the previous commit, per review: Tier E should need no exceptions. run_all_tests.sql now creates schema te_test_harness and puts it first on search_path, so test_framework.sql's unqualified CREATEs (test_run_results, assert_*, report_*) land there instead of in te_dev. T&E tables still resolve from the env schema, and the suites already address it through te.schema_name. The four report calls are qualified with te_test_harness. Tier S caught the first attempt, which left the report calls qualified with the env schema. Local replay on PostgreSQL 16: te_dev holds exactly 12 tables; evals P/I/S/E 26/26, 0 skipped; test_report --strict 238/238, 0 skipped; make lint clean. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01AYM3RR8Fmw2pwqFfnJhAo6 --- .../01_envs_have_identical_structure.json | 2 -- evals/runner.py | 9 --------- tests/run_all_tests.sql | 18 ++++++++++++------ tests/test_evals_tier_e_x.py | 12 ------------ 4 files changed, 12 insertions(+), 29 deletions(-) diff --git a/evals/expected/tier_e/01_envs_have_identical_structure.json b/evals/expected/tier_e/01_envs_have_identical_structure.json index 0d9bc45..adb8904 100644 --- a/evals/expected/tier_e/01_envs_have_identical_structure.json +++ b/evals/expected/tier_e/01_envs_have_identical_structure.json @@ -3,8 +3,6 @@ "description": "Deploy all four environments and assert their deployed schemas are structurally identical; only the database/schema namespace may differ. Closes VCRM BR-01.", "envs_under_test": ["dev", "test", "staging", "prod"], "reference_env": "dev", - "ignore_tables": ["test_run_results"], - "ignore_tables_reason": "Created in te_dev by tests/framework/test_framework.sql when Tier S runs; test-harness state, not deployed schema.", "expected": { "all_envs_deploy_exit_zero": true, "column_signature_identical_across_envs": true, diff --git a/evals/runner.py b/evals/runner.py index cf82dc3..5a9c0a0 100644 --- a/evals/runner.py +++ b/evals/runner.py @@ -699,19 +699,10 @@ def _run_envs_have_identical_structure( signatures[env_name] = {} query_errors.append("env " + env_name + ": " + str(exc)) - # Tables the SQL test harness creates in the schema it tests (Tier S runs - # tests/framework/test_framework.sql in te_dev). Not deployed schema, so - # not drift. Listed in the expectation file, never silently. - ignored = set(expected.get("ignore_tables", [])) - for sig in signatures.values(): - for table in ignored: - sig.pop(table, None) - table_counts = {e: len(sig) for e, sig in signatures.items()} result.actual = { "deploys": deploys, "table_counts": table_counts, - "ignored_tables": sorted(ignored), } # A failed query is not an empty schema; report it as what it is. diff --git a/tests/run_all_tests.sql b/tests/run_all_tests.sql index 8dd4034..abec3d9 100644 --- a/tests/run_all_tests.sql +++ b/tests/run_all_tests.sql @@ -32,11 +32,17 @@ -- ── Step 1: Load the test framework (results table + assertion functions) ───── --- Set search_path so framework objects and assertions resolve without schema prefix. +-- The harness keeps its own objects (test_run_results, assert_*, report_*) in +-- te_test_harness so they never land in the schema under test, where Tier E +-- would see them as drift (issue #45). It comes first on search_path, so the +-- framework's unqualified CREATEs go there; T&E tables still resolve unqualified +-- from the env schema, and the suites address it via te.schema_name anyway. +CREATE SCHEMA IF NOT EXISTS te_test_harness; + -- Expose per-env vars via current_setting() for PL/pgSQL DO blocks (:'var' doesn't -- substitute inside $dollar-quoted$ strings in psql 18). SELECT - set_config('search_path', :'schema_name' || ',public', false), + set_config('search_path', 'te_test_harness,' || :'schema_name' || ',public', false), set_config('te.schema_name', :'schema_name', false), set_config('te.app_user', :'app_user', false), set_config('te.conn_limit', :'conn_limit', false); @@ -70,7 +76,7 @@ SELECT skipped, pass_rate, suite_status -FROM :"schema_name".report_suite_summary(); +FROM te_test_harness.report_suite_summary(); \echo '' \echo '============================================================' @@ -83,7 +89,7 @@ SELECT expected, actual, message -FROM :"schema_name".report_failures(); +FROM te_test_harness.report_failures(); \echo '' \echo '============================================================' @@ -97,7 +103,7 @@ SELECT expected, actual, message -FROM :"schema_name".report_detail(); +FROM te_test_harness.report_detail(); \echo '' \echo '============================================================' @@ -111,7 +117,7 @@ SELECT skipped, pass_rate, overall -FROM :"schema_name".report_totals(); +FROM te_test_harness.report_totals(); \echo '' \echo '============================================================' diff --git a/tests/test_evals_tier_e_x.py b/tests/test_evals_tier_e_x.py index b74d2f1..51efa99 100644 --- a/tests/test_evals_tier_e_x.py +++ b/tests/test_evals_tier_e_x.py @@ -118,18 +118,6 @@ def test_query_failure_is_reported_as_a_query_error(self): self.assertTrue(any("schema query" in e and "syntax error" in e for e in result.errors), result.errors) - def test_only_listed_harness_tables_are_ignored(self): - two = subprocess.CompletedProcess( - [], 0, stdout="test_run_results|id|bigint|NO\nstray|id|bigint|NO\n", stderr="") - expected = {"expected": {"min_tables_per_env": 1}, - "envs_under_test": ["dev"], "ignore_tables": ["test_run_results"]} - result = runner.ScenarioResult(tier="e", name="t") - with patch.object(runner, "_deploy_env", - return_value={"env": "dev", "exit_code": 0}), \ - patch.object(runner.subprocess, "run", return_value=two): - runner._run_envs_have_identical_structure(result, expected) - self.assertEqual(result.actual["table_counts"], {"dev": 1}) - def test_genuinely_empty_schema_still_fails(self): empty = subprocess.CompletedProcess([], 0, stdout="", stderr="") result, _ = self._scenario(empty)