From 60273911ede7c2466cb6591d7eb18bde009933c3 Mon Sep 17 00:00:00 2001 From: Sandeep Date: Tue, 28 Jul 2026 16:18:37 -0700 Subject: [PATCH 1/4] test: run the safety corpus against real MySQL, SQL Server and DuckDB MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ACE-079 proved the guard's verdict on all eleven engines, but with a spy executor and no database — the vetted statement was re-parsed by sqlglot, never accepted by an engine. That leaves the fidelity half of REQ-014 proven only on SQLite and Postgres, both `"`-quoting, i.e. the family where the defect class cannot manifest. Adds a per-engine physical-fixture seam (`harness.EngineFixture` + `seed_schema`) and refactors BOTH existing seeders onto it — SQLite's `seed_sqlite` and the Postgres seeder that was inline in `conftest.db_safety_env` — so a DDL-spelling difference lives in one table rather than a second hand-written seeder that can drift. Adding an engine is now a row in `ENGINE_FIXTURES`. Executes the corpus against MySQL 8 (backtick — its native quote), SQL Server 2022 (the only bracket-quoting engine) and DuckDB in-process. Together with Postgres and SQLite that covers all three identifier-quoting families. New cases pin the behaviour the dialect fix exists for: - a backtick-quoted governed query is allowed AND executes on MySQL; - `SELECT TOP n [col] FROM [tbl]` is scoped correctly on SQL Server — the exact shape a generic parse reduces to no tables with `TOP` mistaken for a column; - on MySQL a double-quoted statement is `unscopable_sql`, because there `"` is a string literal. The generic reading of that same text is a scopable query over `orders`, so this is the positive proof that reading in the engine's own grammar changes the verdict and not merely the parse. The two `"`-quoting cases that ran everywhere are pinned to the engines where `"` actually quotes an identifier, with MySQL's counterpart asserted separately. Co-Authored-By: Claude Opus 5 (1M context) --- tests/e2e/conftest.py | 203 ++++++++++++++++++++++++++++++-- tests/e2e/harness.py | 80 ++++++++++++- tests/e2e/test_safety_corpus.py | 62 ++++++++++ tests/safety/corpus.py | 47 ++++++-- 4 files changed, 368 insertions(+), 24 deletions(-) diff --git a/tests/e2e/conftest.py b/tests/e2e/conftest.py index 2843ce8..8bbed28 100644 --- a/tests/e2e/conftest.py +++ b/tests/e2e/conftest.py @@ -18,6 +18,8 @@ import harness # noqa: E402 (tests/e2e is on sys.path during collection) +from safety.corpus import SCHEMA # noqa: E402 + BASE = "https://demo.example.com" @@ -138,17 +140,12 @@ def db_safety_env(pg_admin, tmp_path, monkeypatch): psycopg2, conn, sc = pg_admin cur = conn.cursor() - # 1) demo datasource tables in public (owner-seeded), from the single-sourced SCHEMA. - from safety.corpus import SCHEMA - - for name, spec in SCHEMA.items(): - cur.execute(f"DROP TABLE IF EXISTS {name} CASCADE") - pg_types = {"INTEGER": "integer", "REAL": "double precision", "TEXT": "text"} - ddl = ", ".join(f"{c} {pg_types[t]}" for c, t in spec["columns"]) - cur.execute(f"CREATE TABLE {name} ({ddl})") - placeholders = ", ".join("%s" for _ in spec["columns"]) - for row in spec["rows"]: - cur.execute(f"INSERT INTO {name} VALUES ({placeholders})", row) + # 1) demo datasource tables in public (owner-seeded), from the single-sourced SCHEMA — through + # the shared per-engine seeder (ACE-082), so Postgres and every other engine build the + # physical fixture the same way and a DDL-spelling difference lives in ONE table. + harness.seed_schema( + lambda sql, params=None: cur.execute(sql, params), "PostgreSQL", drop_first=True + ) # 2) the SELECT-only role the app connects to the datasource as. create_ro_role(cur, sc["dbname"]) @@ -181,6 +178,190 @@ def db_safety_env(pg_admin, tmp_path, monkeypatch): _reset_ro_role(cur) +# ── ACE-082: the per-engine EXECUTION fixtures (real engines, containerized) ─────────────────── +# ACE-079 proved the guard's VERDICT on all eleven engines, but with a spy executor and no +# database — the vetted statement was re-parsed by sqlglot, never accepted by an engine. These +# fixtures close that half: the same corpus, through the same tool, against a real server. +# +# Each engine follows `pg_admin`'s contract exactly: absent connection settings SKIP, unless +# AGAMI_IT__REQUIRED is set — in the CI job that owns an engine's evidence, an +# unreachable server FAILS, because pytest exits 0 when everything skips and a silently-skipped +# job would claim per-engine coverage while proving nothing. + + +def _engine_creds(engine: str, default_port: str) -> dict[str, str]: + """AGAMI_IT__* connection settings. Defaults target the local containers; CI overrides.""" + e = engine.upper() + return { + "host": os.environ.get(f"AGAMI_IT_{e}_HOST", "127.0.0.1"), + "port": os.environ.get(f"AGAMI_IT_{e}_PORT", default_port), + "user": os.environ.get(f"AGAMI_IT_{e}_USER", ""), + "password": os.environ.get(f"AGAMI_IT_{e}_PASSWORD", ""), + "database": os.environ.get(f"AGAMI_IT_{e}_DB", "shop"), + } + + +def _unavailable(engine: str): + """`pytest.fail` in the job that owns this engine's evidence, else `pytest.skip`.""" + return pytest.fail if os.environ.get(f"AGAMI_IT_{engine.upper()}_REQUIRED") else pytest.skip + + +def _write_credentials(art: Path, profile: str, fields: dict[str, str], monkeypatch) -> None: + """Write /local/credentials for engines with no DSN scheme (SQLServer, DuckDB). + + Both surfaces must read the SAME file: the stdio subprocess re-resolves the path from + AGAMI_ARTIFACTS_DIR in `execute_sql.main()`, but the in-process HTTP surface captured + CREDENTIALS_PATH at import — so the module constant is repointed here too. That is a + test-ordering artifact, not a product gap: a deployed server has its credentials file + before it boots. + """ + import execute_sql + + local = art / "local" + local.mkdir(parents=True, exist_ok=True) + path = local / "credentials" + body = f"[{profile}]\n" + "".join(f"{k} = {v}\n" for k, v in fields.items()) + path.write_text(body) + path.chmod(0o600) # the executor refuses a more permissive credentials file + monkeypatch.setattr(execute_sql, "CREDENTIALS_PATH", path) + + +def _engine_model_env(art: Path, storage_type: str, monkeypatch) -> None: + """File-served model declaring `storage_type` — the engine the guard parses every statement in, + and the engine the executor is then reconciled against.""" + (art / "acme").mkdir(parents=True, exist_ok=True) + harness.write_disk_model(art / "acme", storage_type=storage_type) + monkeypatch.delenv("AGAMI_DB_URL", raising=False) + monkeypatch.delenv("APP_DATABASE_URL", raising=False) + monkeypatch.setenv("AGAMI_ARTIFACTS_DIR", str(art)) + monkeypatch.setenv("AGAMI_PROFILE", "acme") + monkeypatch.delenv("AGAMI_SQL_UNSCOPABLE_POSTURE", raising=False) + + +@pytest.fixture +def mysql_safety_env(tmp_path, monkeypatch): + """MySQL 8 — the engine that matters most here: backticks are MySQL's NATIVE identifier quote, + so this is where a wrongly-dialected parse and a real execution can genuinely disagree.""" + pymysql = pytest.importorskip("pymysql") + unavailable = _unavailable("MySQL") + c = _engine_creds("MySQL", "53307") + if not c["password"]: + unavailable("set AGAMI_IT_MYSQL_PASSWORD to run the MySQL safety corpus") + try: + conn = pymysql.connect( + host=c["host"], + port=int(c["port"]), + user=c["user"], + password=c["password"], + database=c["database"], + autocommit=True, + connect_timeout=10, + ) + except Exception as exc: + unavailable(f"no reachable MySQL ({exc})") + cur = conn.cursor() + harness.seed_schema( + lambda sql, params=None: cur.execute(sql, params), "MySQL", drop_first=True + ) + art = tmp_path / "art" + _engine_model_env(art, "MySQL", monkeypatch) + monkeypatch.setenv( + "DATASOURCE_URL__ACME", + f"mysql://{c['user']}:{c['password']}@{c['host']}:{c['port']}/{c['database']}", + ) + try: + yield + finally: + for name in SCHEMA: + try: + cur.execute(f"DROP TABLE IF EXISTS {name}") + except Exception: + pass + conn.close() + + +@pytest.fixture +def sqlserver_safety_env(tmp_path, monkeypatch): + """SQL Server 2022 — the only bracket-quoting engine, and the worst measured divergence: + under a generic parse `SELECT TOP n [col] FROM [tbl]` resolves to no tables and `TOP` as a + column, so every scope gate finds nothing to object to.""" + pymssql = pytest.importorskip("pymssql") + unavailable = _unavailable("SQLServer") + c = _engine_creds("SQLServer", "11433") + if not c["password"]: + unavailable("set AGAMI_IT_SQLSERVER_PASSWORD to run the SQL Server safety corpus") + try: + boot = pymssql.connect( + server=c["host"], + port=int(c["port"]), + user=c["user"], + password=c["password"], + autocommit=True, + login_timeout=15, + ) + except Exception as exc: + unavailable(f"no reachable SQL Server ({exc})") + bcur = boot.cursor() + bcur.execute(f"IF DB_ID('{c['database']}') IS NULL CREATE DATABASE [{c['database']}]") + boot.close() + + conn = pymssql.connect( + server=c["host"], + port=int(c["port"]), + user=c["user"], + password=c["password"], + database=c["database"], + autocommit=True, + login_timeout=15, + ) + cur = conn.cursor() + harness.seed_schema( + lambda sql, params=None: cur.execute(sql, params), "SQLServer", drop_first=True + ) + art = tmp_path / "art" + _engine_model_env(art, "SQLServer", monkeypatch) + # SQL Server has no DSN scheme (`_env_datasource_dsn` handles postgres/mysql/sqlite/…), so the + # per-field credentials file is the supported channel. Clear the env DSN or it would win. + monkeypatch.delenv("DATASOURCE_URL__ACME", raising=False) + _write_credentials( + art, + "acme", + { + "type": "sqlserver", + "host": c["host"], + "port": c["port"], + "user": c["user"], + "password": c["password"], + "database": c["database"], + }, + monkeypatch, + ) + try: + yield + finally: + for name in SCHEMA: + try: + cur.execute(f"DROP TABLE IF EXISTS {name}") + except Exception: + pass + conn.close() + + +@pytest.fixture +def duckdb_safety_env(tmp_path, monkeypatch): + """DuckDB in-process — no server, no container. Included because it is cheap and because the + corpus previously claimed DuckDB coverage it did not have.""" + pytest.importorskip("duckdb") + db = tmp_path / "shop.duckdb" + harness.seed_duckdb(db) + art = tmp_path / "art" + _engine_model_env(art, "DuckDB", monkeypatch) + # DuckDB has no DSN scheme either — same per-field credentials channel as SQL Server. + monkeypatch.delenv("DATASOURCE_URL__ACME", raising=False) + _write_credentials(art, "acme", {"type": "duckdb", "path": str(db)}, monkeypatch) + yield + + @pytest.fixture def pg_ro_conn(pg_admin): """A RAW connection AS the read-only role, plus one seeded table — for the role-floor test. This diff --git a/tests/e2e/harness.py b/tests/e2e/harness.py index 8c62440..dc59110 100644 --- a/tests/e2e/harness.py +++ b/tests/e2e/harness.py @@ -14,6 +14,7 @@ import sqlite3 import subprocess import sys +from dataclasses import dataclass, field from pathlib import Path REPO_ROOT = Path(__file__).resolve().parents[2] @@ -28,6 +29,66 @@ _TYPE_MAP = {"INTEGER": "integer", "REAL": "float", "TEXT": "string"} +# ── the physical-fixture seam (ACE-082) ──────────────────────────────────────────────────────── +@dataclass(frozen=True) +class EngineFixture: + """How the single-sourced `SCHEMA` is materialized physically on ONE engine. + + The corpus never forks: the cases, the schema and the expected verdicts are shared. The only + thing that legitimately varies per engine is the physical fixture — how each column type is + spelled in DDL, which parameter placeholder the driver takes, and whether DROP needs a + CASCADE. Naming exactly those three here keeps every engine on one seeding code path, so + adding an engine is a row in `ENGINE_FIXTURES` rather than a second seeder that can drift. + """ + + storage_type: str # what the semantic model declares (m.StorageConnection.storage_type) + credential_type: str # what the executor dispatches on (`type` in the DSN / credentials file) + ddl_types: dict[str, str] = field(default_factory=dict) + placeholder: str = "?" + drop_suffix: str = "" # Postgres needs CASCADE; the others reject the keyword + + +ENGINE_FIXTURES: dict[str, EngineFixture] = { + "SQLite": EngineFixture( + "SQLite", "sqlite", {"INTEGER": "INTEGER", "REAL": "REAL", "TEXT": "TEXT"}, "?" + ), + "PostgreSQL": EngineFixture( + "PostgreSQL", + "postgres", + {"INTEGER": "integer", "REAL": "double precision", "TEXT": "text"}, + "%s", + " CASCADE", + ), + "MySQL": EngineFixture( + "MySQL", "mysql", {"INTEGER": "int", "REAL": "double", "TEXT": "varchar(255)"}, "%s" + ), + "SQLServer": EngineFixture( + "SQLServer", "sqlserver", {"INTEGER": "int", "REAL": "float", "TEXT": "nvarchar(255)"}, "%s" + ), + "DuckDB": EngineFixture( + "DuckDB", "duckdb", {"INTEGER": "INTEGER", "REAL": "DOUBLE", "TEXT": "VARCHAR"}, "?" + ), +} + + +def seed_schema(execute, engine: str, *, drop_first: bool = False) -> None: + """Materialize every `SCHEMA` table on `engine`, through the caller's `execute(sql, params=None)`. + + The adapter is passed in rather than a connection, so this function imports no driver and is + the single place that turns `SCHEMA` into physical tables — on every engine, including the two + that were previously seeded by hand (SQLite here, Postgres inline in `conftest.db_safety_env`). + """ + fx = ENGINE_FIXTURES[engine] + for name, spec in SCHEMA.items(): + if drop_first: + execute(f"DROP TABLE IF EXISTS {name}{fx.drop_suffix}") + ddl = ", ".join(f"{c} {fx.ddl_types[t]}" for c, t in spec["columns"]) + execute(f"CREATE TABLE {name} ({ddl})") + placeholders = ", ".join(fx.placeholder for _ in spec["columns"]) + for row in spec["rows"]: + execute(f"INSERT INTO {name} VALUES ({placeholders})", row) + + # ── model + datasource builders (single-sourced from SCHEMA) ─────────────────────────────────── def build_org(storage_type: str = "SQLite"): """The semantic model the guards scope against — built from SCHEMA (names + sensitive flags).""" @@ -113,16 +174,25 @@ def seed_sqlite(path: Path) -> None: """Create + seed the physical SQLite datasource governed queries execute against.""" con = sqlite3.connect(str(path)) try: - for name, spec in SCHEMA.items(): - ddl = ", ".join(f"{c} {t}" for c, t in spec["columns"]) - con.execute(f"CREATE TABLE {name} ({ddl})") - placeholders = ", ".join("?" for _ in spec["columns"]) - con.executemany(f"INSERT INTO {name} VALUES ({placeholders})", spec["rows"]) + seed_schema(lambda sql, params=None: con.execute(sql, params or ()), "SQLite") con.commit() finally: con.close() +def seed_duckdb(path: Path) -> None: + """Create + seed the physical DuckDB datasource. Seeded through a WRITE connection here because + the executor deliberately opens DuckDB `read_only=True` — the fixture must not need the executor + to be writable to exist.""" + import duckdb # type: ignore + + con = duckdb.connect(str(path)) + try: + seed_schema(lambda sql, params=None: con.execute(sql, params or []), "DuckDB") + finally: + con.close() + + def seed_db_model(url: str, ds: str = "acme", storage_type: str = "SQLite") -> None: """Write the DB-served model into an app DB at `url` (the hosted path's model source).""" import model_store diff --git a/tests/e2e/test_safety_corpus.py b/tests/e2e/test_safety_corpus.py index 2c3e107..4477d4e 100644 --- a/tests/e2e/test_safety_corpus.py +++ b/tests/e2e/test_safety_corpus.py @@ -6,6 +6,43 @@ scopability, or recon fails here on whichever surface it regressed; availability is asserted via the row-cap arm (the statement-timeout arm is proven end-to-end in `tests/test_resource_limits.py`). The read-only-role floor lives in `test_role_floor_pg.py` (Postgres, env-gated). + +ACE-082 — per-engine EXECUTION coverage +======================================= +ACE-079 made the guard read every statement in the datasource's own dialect, and proved the VERDICT +on all eleven supported engines (`tests/test_guard_dialect_parsing.py`). That proof uses a spy +executor and no database: the vetted statement is re-parsed by sqlglot, never accepted by an engine. +This module adds the other half — the statement the guard vetted is a statement a REAL engine +accepts — and the table below is the single place that says how far that goes per engine. + +| Engine | Identifier quote | Coverage here | What is NOT proven | +|------------|------------------|-----------------------------------|---------------------------------------| +| SQLite | `"` `` ` `` `[]` | real, in-process (file path) | — | +| PostgreSQL | `"` | real server (`integration-pg`) | — | +| MySQL | `` ` `` | real server (`mysql:8`) | — | +| SQLServer | `[]` | real server (`mssql/server:2022`) | — | +| DuckDB | `"` | real, in-process | — | +| Redshift | `"` | verdict only (ACE-079) | execution parity | +| Snowflake | `"` | verdict only (ACE-079) | execution parity | +| BigQuery | `` ` `` | verdict only (ACE-079) | execution parity | +| Databricks | `` ` `` | verdict only (ACE-079) | execution parity | +| Oracle | `"` | verdict only (ACE-079) | execution parity | +| Trino | `"` | verdict only (ACE-079) | execution parity | + +The five executing engines cover all three identifier-quoting families — backtick (MySQL), bracket +(SQLServer) and double-quote (PostgreSQL/SQLite/DuckDB) — which is the axis ACE-079's defect class +turns on. The six verdict-only engines each share a quoting family with one that executes. + +Two engines named in ACE-082 are deliberately absent, because reaching them would require changing +runtime source, which that spec forbids: + - **BigQuery emulator** — `execute_sql._run_bigquery` builds `bigquery.Client(project=…)` with no + `client_options` / `api_endpoint`, so it cannot be pointed at an emulator without a source + change. (The emulator is also SQLite-backed, so it would prove parse/accept, not BigQuery + semantics.) + - **Databricks via local Spark** — `execute_sql._run_databricks` connects through the Databricks + SQL connector to a workspace; a local `SparkSession` is not reachable through it. Testing it + would mean asserting against a stub, which is what ACE-079 already does. +Both remain verdict-only above, and are recorded as residual risk in the spec rather than implied. """ from __future__ import annotations @@ -64,6 +101,31 @@ def test_safety_corpus_file_path(case, surface, file_safety_env): assert_outcome(env, case.expect, case.sql) +# ── ACE-082: the same corpus, against real engines beyond Postgres/SQLite ───────────────────── +# One env fixture per executing engine. Each SKIPS without connection settings and FAILS instead +# when AGAMI_IT__REQUIRED is set, so the job that owns an engine's evidence cannot pass +# green while proving nothing. Pairing engine×case here (rather than skipping inside the test) +# keeps the ids exact, so a CI job selects its engine with `-k "engine and MySQL"`. +ENGINE_ENV_FIXTURES = { + "MySQL": "mysql_safety_env", + "SQLServer": "sqlserver_safety_env", + "DuckDB": "duckdb_safety_env", +} +ENGINE_CASES = [(e, c) for e in ENGINE_ENV_FIXTURES for c in CASES if c.runs_on(e)] + + +@pytest.mark.parametrize( + "engine,case", ENGINE_CASES, ids=[f"{e}:{c.id}" for e, c in ENGINE_CASES] +) +def test_safety_corpus_engine(engine, case, surface, request): + # The verdict must be identical to the SQLite/Postgres paths AND the vetted statement must be + # one this engine actually accepts — an "ok" case here executes for real, so a statement the + # guard approved but the engine rejects fails as an executor error rather than passing. + request.getfixturevalue(ENGINE_ENV_FIXTURES[engine]) + env = surface(case.sql, datasource="acme", max_rows=case.max_rows) + assert_outcome(env, case.expect, case.sql) + + @pytest.mark.parametrize("case", DB_PATH_CASES, ids=[c.id for c in DB_PATH_CASES]) def test_safety_corpus_db_path(case, surface, db_safety_env): # DB-served model (Postgres app DB) + Postgres datasource read as the read-only role. IDENTICAL diff --git a/tests/safety/corpus.py b/tests/safety/corpus.py index b0331d1..1c55810 100644 --- a/tests/safety/corpus.py +++ b/tests/safety/corpus.py @@ -157,17 +157,45 @@ def id(self) -> str: # (The demo datasource is SQLite, which accepts both quoting styles.) # SQLite accepts backticks and brackets as identifier quoting; Postgres does not, where the # same text is not valid SQL at all — so these are pinned to the engine they mean something on. + # MySQL is where these matter most: the backtick is MySQL's NATIVE identifier quote, so this is + # the engine where a wrongly-dialected parse and a real execution can genuinely disagree. SQLite + # accepts backticks too, so both run the same expectation. Case("quoting", "SELECT `id` FROM `undeclared`", "table_out_of_scope", - "backtick-undeclared-table", engines=("SQLite",)), + "backtick-undeclared-table", engines=("SQLite", "MySQL")), Case("quoting", "SELECT `nosuchcol` FROM `orders`", "column_out_of_scope", - "backtick-undeclared-col", engines=("SQLite",)), + "backtick-undeclared-col", engines=("SQLite", "MySQL")), Case("quoting", "SELECT `email` FROM `customers`", "ok", - "backtick-sensitive-is-seen", engines=("SQLite",)), - Case("quoting", "SELECT * FROM `orders`", "select_star", "backtick-star", engines=("SQLite",)), + "backtick-sensitive-is-seen", engines=("SQLite", "MySQL")), + Case("quoting", "SELECT * FROM `orders`", "select_star", "backtick-star", + engines=("SQLite", "MySQL")), + # The other half of the guarantee: a backtick-quoted GOVERNED query is not merely allowed, it + # executes on the engine whose native quote that is. A vetted statement the engine then rejects + # would fail here as an executor error rather than passing. + Case("quoting", "SELECT `id`, `amount` FROM `orders`", "ok", + "backtick-governed-executes", engines=("SQLite", "MySQL")), + # SQL Server is the only bracket-quoting engine, and `TOP n` is where a generic parse fails + # worst: it resolves to no tables at all with `TOP` mistaken for a column, so every scope gate + # finds nothing to object to and allows the statement. Case("quoting", "SELECT [id] FROM [undeclared]", "table_out_of_scope", - "bracket-undeclared-table", engines=("SQLite",)), - # Valid identifier quoting on both engines, so it runs everywhere. - Case("quoting", 'SELECT "id" FROM "undeclared"', "table_out_of_scope", "quoted-undeclared-table"), + "bracket-undeclared-table", engines=("SQLite", "SQLServer")), + Case("quoting", "SELECT TOP 5 [id] FROM [undeclared]", "table_out_of_scope", + "bracket-top-undeclared-table", engines=("SQLServer",)), + Case("quoting", "SELECT TOP 5 [email] FROM [customers]", "ok", + "bracket-top-sensitive-is-seen", engines=("SQLServer",)), + Case("quoting", "SELECT TOP 5 [id], [amount] FROM [orders]", "ok", + "bracket-top-governed-executes", engines=("SQLServer",)), + # `"` is an identifier quote on SQLite/Postgres/SQL Server/DuckDB — but on MySQL it is a STRING + # literal, so the same text is a different statement there and gets its own expectation below. + Case("quoting", 'SELECT "id" FROM "undeclared"', "table_out_of_scope", "quoted-undeclared-table", + engines=("SQLite", "PostgreSQL", "SQLServer", "DuckDB")), + # On MySQL a double-quoted statement cannot be scoped at all (the "identifiers" are strings), so + # the guard must fail CLOSED rather than mis-read it as a query over declared objects. This is + # the positive proof that reading in the engine's own grammar changes the verdict, not just the + # parse — the generic reading of this statement is a scopable query over `orders`. + Case("quoting", 'SELECT "id" FROM "undeclared"', "unscopable_sql", + "double-quote-is-a-string-on-mysql", engines=("MySQL",)), + Case("quoting", 'SELECT "id", "amount" FROM "orders"', "unscopable_sql", + "double-quote-governed-unscopable-on-mysql", engines=("MySQL",)), # The read-only lexer is a separate code path from the dialect-aware parse; a write stays # refused whatever the quoting, on every engine. Case("quoting", "DELETE FROM `orders`", "permission", "backtick-delete"), @@ -175,7 +203,10 @@ def id(self) -> str: # change which tightens parsing has to prove it did not start refusing valid SQL. Without # them the "no false refusal" side of the corpus rests on four statements, and a tightening # that broke ordinary queries could still pass. - Case("governed", 'SELECT "id", "amount" FROM "orders"', "ok", "quoted-identifiers"), + # Pinned to the engines where `"` quotes an identifier; MySQL's counterpart is the + # `unscopable_sql` case above, where the same text is a string literal. + Case("governed", 'SELECT "id", "amount" FROM "orders"', "ok", "quoted-identifiers", + engines=("SQLite", "PostgreSQL", "SQLServer", "DuckDB")), Case("governed", "SELECT o.id, o.amount FROM orders o", "ok", "aliased-table"), Case("governed", "SELECT id FROM orders WHERE status = 'paid'", "ok", "where-literal"), Case("governed", "SELECT id, amount FROM orders ORDER BY amount DESC", "ok", "order-by"), From 80f0060bc9e02434046d53472ae1a1c285f75ea8 Mon Sep 17 00:00:00 2001 From: Sandeep Date: Tue, 28 Jul 2026 17:09:58 -0700 Subject: [PATCH 2/4] ci: run the per-engine safety corpus on MySQL, SQL Server and DuckDB MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three separate jobs with no `needs:` on integration-pg and none between themselves, so one flaky engine container cannot turn the Postgres role-floor result — the F9 done-bar — green or red. integration-pg's selector is unchanged and matches none of the new tests. Each job sets AGAMI_IT__REQUIRED so an unreachable server FAILS instead of skipping: pytest exits 0 when everything skips, and a silently-skipped job would claim per-engine coverage while proving nothing. Verified both directions — with no connection settings the engine tests skip cleanly, and with REQUIRED set against a dead port they fail loudly. No secrets are involved, so these run on fork PRs too. SQL Server accepts TCP before it can serve queries, so readiness is an explicit query poll rather than a port check. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/ci.yml | 130 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 130 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 02db4e4..1f7dec6 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -100,6 +100,136 @@ jobs: pytest tests/e2e/test_safety_corpus.py tests/e2e/test_role_floor_pg.py -q -k "db_path or role" + # ── ACE-082: per-engine EXECUTION evidence ─────────────────────────────────────────────────── + # Deliberately SEPARATE jobs, with no `needs:` on integration-pg and none between themselves, so + # one flaky engine container cannot turn the Postgres role-floor result (the F9 done-bar) green + # or red. Each job selects only its own engine, and sets AGAMI_IT__REQUIRED so an + # unreachable server FAILS rather than skipping — pytest exits 0 when everything skips, and a + # silently-skipped job would claim per-engine coverage while proving nothing. + # These need no secrets, so they run on fork PRs too. + + integration-mysql: + # The highest-value engine here: backticks are MySQL's NATIVE identifier quote, so this is + # where a wrongly-dialected parse and a real execution can genuinely disagree. + name: integration (MySQL — dialect execution matrix) + runs-on: ubuntu-latest + services: + mysql: + image: mysql:8 + env: + MYSQL_DATABASE: shop + MYSQL_USER: agami_test + # Ephemeral localhost CI service on a throwaway container. MySQL has no `trust` auth + # equivalent, so a literal is unavoidable; this is a non-secret sentinel, not a credential. + MYSQL_PASSWORD: ci_local_only + MYSQL_ROOT_PASSWORD: ci_local_only_root + ports: + - 3306:3306 + # `mysqladmin ping` exits 0 once the server answers, even for an unauthenticated ping — + # which is exactly the readiness signal we want. + options: >- + --health-cmd "mysqladmin ping" + --health-interval 5s + --health-timeout 5s + --health-retries 20 + env: + AGAMI_IT_MYSQL_HOST: 127.0.0.1 + AGAMI_IT_MYSQL_PORT: "3306" + AGAMI_IT_MYSQL_USER: agami_test + AGAMI_IT_MYSQL_PASSWORD: ci_local_only + AGAMI_IT_MYSQL_DB: shop + AGAMI_IT_MYSQL_REQUIRED: "1" + steps: + - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 + - uses: astral-sh/setup-uv@fac544c07dec837d0ccb6301d7b5580bf5edae39 # v8.2.0 + with: + python-version: "3.12" + - name: pytest (MySQL safety corpus) + run: >- + uvx --python 3.12 + --with pytest --with pymysql + --with-editable "packages/agami-core[model,server]" + pytest tests/e2e/test_safety_corpus.py -q -k "engine and MySQL" + + integration-sqlserver: + # The only bracket-quoting engine, and the worst measured divergence: under a generic parse + # `SELECT TOP n [col] FROM [tbl]` resolves to no tables with `TOP` mistaken for a column. + name: integration (SQL Server — dialect execution matrix) + runs-on: ubuntu-latest + services: + sqlserver: + image: mcr.microsoft.com/mssql/server:2022-latest + env: + ACCEPT_EULA: "Y" + MSSQL_PID: Developer # free edition, no licence + # Non-secret sentinel for an ephemeral localhost service. Must still satisfy SQL Server's + # complexity rule (upper + lower + digit + non-alphanumeric) or the server refuses to start. + MSSQL_SA_PASSWORD: Ci_local_only_1 + ports: + - 1433:1433 + env: + AGAMI_IT_SQLSERVER_HOST: 127.0.0.1 + AGAMI_IT_SQLSERVER_PORT: "1433" + AGAMI_IT_SQLSERVER_USER: sa + AGAMI_IT_SQLSERVER_PASSWORD: Ci_local_only_1 + AGAMI_IT_SQLSERVER_DB: shop + AGAMI_IT_SQLSERVER_REQUIRED: "1" + steps: + - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 + - uses: astral-sh/setup-uv@fac544c07dec837d0ccb6301d7b5580bf5edae39 # v8.2.0 + with: + python-version: "3.12" + # SQL Server accepts TCP connections before it can serve queries, so a port check is not a + # readiness signal — poll an actual query. Explicit here rather than a `--health-cmd`, whose + # nested shell quoting cannot be verified without running it on a x86 runner. + - name: wait for SQL Server + run: | + uvx --python 3.12 --with pymssql python - <<'PY' + import os, sys, time, pymssql + + deadline, last = time.time() + 180, None + while time.time() < deadline: + try: + conn = pymssql.connect( + server=os.environ["AGAMI_IT_SQLSERVER_HOST"], + port=int(os.environ["AGAMI_IT_SQLSERVER_PORT"]), + user=os.environ["AGAMI_IT_SQLSERVER_USER"], + password=os.environ["AGAMI_IT_SQLSERVER_PASSWORD"], + login_timeout=5, + ) + conn.cursor().execute("SELECT 1") + conn.close() + print("SQL Server ready") + sys.exit(0) + except Exception as exc: + last = exc + time.sleep(5) + sys.exit(f"SQL Server never became ready: {last}") + PY + - name: pytest (SQL Server safety corpus) + run: >- + uvx --python 3.12 + --with pytest --with pymssql + --with-editable "packages/agami-core[model,server]" + pytest tests/e2e/test_safety_corpus.py -q -k "engine and SQLServer" + + integration-duckdb: + # In-process, no service. Cheap, and it closes a coverage claim the corpus previously made + # without having: DuckDB appeared only in a plugin-script test and an always-skipped one. + name: integration (DuckDB — dialect execution matrix) + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 + - uses: astral-sh/setup-uv@fac544c07dec837d0ccb6301d7b5580bf5edae39 # v8.2.0 + with: + python-version: "3.12" + - name: pytest (DuckDB safety corpus) + run: >- + uvx --python 3.12 + --with pytest --with duckdb + --with-editable "packages/agami-core[model,server]" + pytest tests/e2e/test_safety_corpus.py -q -k "engine and DuckDB" + gitleaks: name: gitleaks (secret scan) runs-on: ubuntu-latest From 963fe33b24d2c31cd9b65743ea2030b9fb79b9a7 Mon Sep 17 00:00:00 2001 From: Sandeep Date: Tue, 28 Jul 2026 17:16:02 -0700 Subject: [PATCH 3/4] test: parse each valid corpus statement under an engine it is declared on MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two latent problems the new engines surfaced. test_no_false_refusal_on_valid_sql parsed EVERY valid case under a hardcoded sqlite dialect, though its own docstring says each statement is parsed "under its declared engine". That held only while every case was SQLite-valid. `SELECT TOP n [col] FROM [tbl]` is valid T-SQL and is not SQLite at all, so the corpus could not carry a statement for another engine without a false failure. It now measures (case, engine) pairs, checking a pinned case under each engine it is declared on and only an unpinned case under the default — which is what the docstring already claimed and what keeps the measurement honest as engines are added. test_resolve_guard_model loaded tests/e2e/harness.py via spec_from_file_location without registering it in sys.modules. A module executed detached that way cannot resolve its own module globals, so adding any class that reads them at creation time — @dataclass here — broke an unrelated test at import. Registering before exec_module is the documented idiom and removes the trap rather than avoiding it. Neither test is weakened: both now check strictly more than before. Co-Authored-By: Claude Opus 5 (1M context) --- tests/test_no_false_refusal_on_valid_sql.py | 34 +++++++++++++++------ tests/test_resolve_guard_model.py | 4 +++ 2 files changed, 28 insertions(+), 10 deletions(-) diff --git a/tests/test_no_false_refusal_on_valid_sql.py b/tests/test_no_false_refusal_on_valid_sql.py index 4751644..a328bf2 100644 --- a/tests/test_no_false_refusal_on_valid_sql.py +++ b/tests/test_no_false_refusal_on_valid_sql.py @@ -33,16 +33,26 @@ sys.path.insert(0, str(REPO_ROOT / "tests")) from semantic_model import runtime as rt # noqa: E402 +from semantic_model.sql_dialect import sqlglot_dialect # noqa: E402 from safety import corpus # noqa: E402 -# The corpus executes against SQLite; the seeded sample model declares SQLite too. Parsing -# each statement under the engine it is written for is the whole point — a statement is only -# valid with respect to some engine. -CORPUS_DIALECT = "sqlite" +# Parsing each statement under the engine it is written for is the whole point — a statement is +# only valid with respect to some engine. The corpus now carries statements written for engines +# other than SQLite (`SELECT TOP n [col]` is valid T-SQL and is not SQLite at all), so a case +# pinned to engines is checked under EACH of them, and only an unpinned case — one claimed to be +# valid everywhere — falls back to the default. Checking those under one fixed dialect would +# either fail on valid SQL or, worse, quietly stop covering the engines the corpus added. +DEFAULT_CORPUS_ENGINE = "SQLite" +CORPUS_DIALECT = sqlglot_dialect(DEFAULT_CORPUS_ENGINE) # the seeded sample model declares SQLite VALID_CORPUS_SQL = [c for c in corpus.CASES if c.expect == "ok"] +# (case, engine) pairs — the unit actually being measured, since one case can be valid on several. +VALID_CORPUS_PAIRS = [ + (c, e) for c in VALID_CORPUS_SQL for e in (c.engines or (DEFAULT_CORPUS_ENGINE,)) +] + SAMPLE_EXAMPLES = sorted( (REPO_ROOT / "plugins" / "agami" / "samples").rglob("prompt_examples/*/examples.yaml") ) @@ -78,10 +88,12 @@ def test_the_seeded_examples_were_found(): assert SEEDED_SQL, "no seeded example SQL found to check" -@pytest.mark.parametrize("case", VALID_CORPUS_SQL, ids=lambda c: c.id) -def test_valid_corpus_sql_still_parses(case): - tree, why = rt._parse_reporting(case.sql, CORPUS_DIALECT) - assert tree is not None, f"valid corpus SQL now refuses ({why}): {case.sql}" +@pytest.mark.parametrize( + "case,engine", VALID_CORPUS_PAIRS, ids=[f"{c.id}@{e}" for c, e in VALID_CORPUS_PAIRS] +) +def test_valid_corpus_sql_still_parses(case, engine): + tree, why = rt._parse_reporting(case.sql, sqlglot_dialect(engine)) + assert tree is not None, f"valid corpus SQL now refuses on {engine} ({why}): {case.sql}" @pytest.mark.parametrize( @@ -94,6 +106,8 @@ def test_seeded_example_sql_still_parses(path, sql): def test_the_measured_refusal_delta_is_zero(): """The number reported in the pull request, computed rather than asserted by hand.""" - checked = [c.sql for c in VALID_CORPUS_SQL] + [s for _, s in SEEDED_SQL] - refused = [s for s in checked if rt._parse_reporting(s, CORPUS_DIALECT)[0] is None] + checked = [(c.sql, sqlglot_dialect(e)) for c, e in VALID_CORPUS_PAIRS] + [ + (s, CORPUS_DIALECT) for _, s in SEEDED_SQL + ] + refused = [s for s, dialect in checked if rt._parse_reporting(s, dialect)[0] is None] assert not refused, f"{len(refused)}/{len(checked)} valid statements refused: {refused}" diff --git a/tests/test_resolve_guard_model.py b/tests/test_resolve_guard_model.py index 483d3d4..40251e5 100644 --- a/tests/test_resolve_guard_model.py +++ b/tests/test_resolve_guard_model.py @@ -31,6 +31,10 @@ def _write_disk_model(root: Path) -> None: "e2e_harness", REPO_ROOT / "tests" / "e2e" / "harness.py" ) h = importlib.util.module_from_spec(spec) + # Register BEFORE exec_module — the documented importlib idiom. A module executed while absent + # from sys.modules cannot resolve its own module globals, which anything reading them at class + # -creation time needs (`@dataclass` is one), so loading it detached fails at import. + sys.modules[spec.name] = h spec.loader.exec_module(h) h.write_disk_model(root) From 5adf1b59ca7f0692490c1d7d71d6b159d915d280 Mon Sep 17 00:00:00 2001 From: Sandeep Date: Tue, 28 Jul 2026 17:39:15 -0700 Subject: [PATCH 4/4] test: close the fail-loud hole a missing driver left open MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review found the AGAMI_IT__REQUIRED contract only half-closed, and reproduced it: `pytest.importorskip("pymysql")` ran BEFORE the REQUIRED check, so a missing driver skipped every test and pytest exited 0 — in the very job that owns that engine's evidence. `integration-duckdb` was worse: no REQUIRED flag existed at all, and these drivers are supplied only by `--with` flags on the CI uvx line, so dropping one is the likeliest way to empty a job. Three comments in the diff asserted this could not happen. `_require_driver` now consults `_unavailable` first, and the DuckDB job sets REQUIRED. Verified: missing driver + REQUIRED now exits 1; without REQUIRED it still skips cleanly at exit 0. Also from review: - MySQL's `--health-cmd "mysqladmin ping"` could pass against the entrypoint's temporary init server, which accepts on the unix socket before the real server binds 3306 — a real flake, the same reason SQL Server already got an explicit poll. Probing `-h 127.0.0.1` stays refused until the port is really listening. - Engine connections were opened outside the `try` that closes them, so a failure while seeding leaked one connection per test and the tail reported "too many connections" instead of the real DDL error. - `assert_outcome`'s "ok" arm never checked that a result set came back, so a fixture seeded into the wrong database would still pass — which would have hollowed out exactly the cases named `*-executes`. - The claim that integration-pg's `-k "db_path or role"` selects no engine test is now asserted rather than assumed; a future case note containing "role" would otherwise silently couple the F9 done-bar to an engine container. - `EngineFixture.storage_type` / `.credential_type` were declared but never read, so the seam permitted the drift its docstring said it prevented. Both are now read at the call sites. - SERIAL-ONLY note extended to the new fixtures; type hints on the new helpers; the coverage table no longer implies an executing engine is proven beyond the corpus's own cases. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/ci.yml | 14 ++- tests/e2e/conftest.py | 176 +++++++++++++++++++------------- tests/e2e/harness.py | 5 +- tests/e2e/test_safety_corpus.py | 53 +++++++--- 4 files changed, 159 insertions(+), 89 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 1f7dec6..f690d57 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -125,10 +125,12 @@ jobs: MYSQL_ROOT_PASSWORD: ci_local_only_root ports: - 3306:3306 - # `mysqladmin ping` exits 0 once the server answers, even for an unauthenticated ping — - # which is exactly the readiness signal we want. + # `mysqladmin ping` exits 0 once the server answers, even for an unauthenticated ping. + # `-h 127.0.0.1` is load-bearing: the entrypoint's temporary init server accepts on the + # unix socket while the real server has not yet bound 3306, so a socket ping reports ready + # during that window. Over TCP the probe stays refused until the port is really listening. options: >- - --health-cmd "mysqladmin ping" + --health-cmd "mysqladmin ping -h 127.0.0.1" --health-interval 5s --health-timeout 5s --health-retries 20 @@ -136,6 +138,7 @@ jobs: AGAMI_IT_MYSQL_HOST: 127.0.0.1 AGAMI_IT_MYSQL_PORT: "3306" AGAMI_IT_MYSQL_USER: agami_test + # Same non-secret sentinel as the service block above — not a credential. AGAMI_IT_MYSQL_PASSWORD: ci_local_only AGAMI_IT_MYSQL_DB: shop AGAMI_IT_MYSQL_REQUIRED: "1" @@ -171,6 +174,7 @@ jobs: AGAMI_IT_SQLSERVER_HOST: 127.0.0.1 AGAMI_IT_SQLSERVER_PORT: "1433" AGAMI_IT_SQLSERVER_USER: sa + # Same non-secret sentinel as the service block above — not a credential. AGAMI_IT_SQLSERVER_PASSWORD: Ci_local_only_1 AGAMI_IT_SQLSERVER_DB: shop AGAMI_IT_SQLSERVER_REQUIRED: "1" @@ -218,6 +222,10 @@ jobs: # without having: DuckDB appeared only in a plugin-script test and an always-skipped one. name: integration (DuckDB — dialect execution matrix) runs-on: ubuntu-latest + env: + # This job is DuckDB's only evidence. Its sole availability risk is the driver below, so + # REQUIRED makes a missing one fail rather than skip the whole corpus green. + AGAMI_IT_DUCKDB_REQUIRED: "1" steps: - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 - uses: astral-sh/setup-uv@fac544c07dec837d0ccb6301d7b5580bf5edae39 # v8.2.0 diff --git a/tests/e2e/conftest.py b/tests/e2e/conftest.py index 8bbed28..15c4f9b 100644 --- a/tests/e2e/conftest.py +++ b/tests/e2e/conftest.py @@ -3,9 +3,12 @@ from __future__ import annotations +import importlib import os import sys +from collections.abc import Callable from pathlib import Path +from typing import NoReturn import pytest @@ -183,10 +186,17 @@ def db_safety_env(pg_admin, tmp_path, monkeypatch): # database — the vetted statement was re-parsed by sqlglot, never accepted by an engine. These # fixtures close that half: the same corpus, through the same tool, against a real server. # -# Each engine follows `pg_admin`'s contract exactly: absent connection settings SKIP, unless +# Each engine follows `pg_admin`'s contract: absent connection settings SKIP, unless # AGAMI_IT__REQUIRED is set — in the CI job that owns an engine's evidence, an -# unreachable server FAILS, because pytest exits 0 when everything skips and a silently-skipped -# job would claim per-engine coverage while proving nothing. +# unavailable engine FAILS, because pytest exits 0 when everything skips and a silently-skipped +# job would claim per-engine coverage while proving nothing. "Unavailable" deliberately includes a +# MISSING DRIVER, not just an unreachable server: these drivers are supplied only by `--with` flags +# on the CI `uvx` line, so dropping one in a future edit is the likeliest way to empty a job, and +# `pytest.importorskip` would turn exactly that into a green run. +# +# Like the Postgres fixtures above, these DROP/CREATE shared table names in a shared database, so +# they are SERIAL-ONLY — do not run this dir under pytest-xdist (`-n`) without per-worker table +# names, or workers would race. The current invocation is serial. def _engine_creds(engine: str, default_port: str) -> dict[str, str]: @@ -201,11 +211,23 @@ def _engine_creds(engine: str, default_port: str) -> dict[str, str]: } -def _unavailable(engine: str): +def _unavailable(engine: str) -> Callable[[str], NoReturn]: """`pytest.fail` in the job that owns this engine's evidence, else `pytest.skip`.""" return pytest.fail if os.environ.get(f"AGAMI_IT_{engine.upper()}_REQUIRED") else pytest.skip +def _require_driver(engine: str, module: str): + """Import an engine's driver, honouring the REQUIRED contract. + + Deliberately not `pytest.importorskip`: that skips unconditionally, which in the job that owns + this engine's evidence would report success for a run that executed nothing. + """ + try: + return importlib.import_module(module) + except ImportError as exc: + _unavailable(engine)(f"{module} not installed, so {engine} proves nothing ({exc})") + + def _write_credentials(art: Path, profile: str, fields: dict[str, str], monkeypatch) -> None: """Write /local/credentials for engines with no DSN scheme (SQLServer, DuckDB). @@ -226,11 +248,12 @@ def _write_credentials(art: Path, profile: str, fields: dict[str, str], monkeypa monkeypatch.setattr(execute_sql, "CREDENTIALS_PATH", path) -def _engine_model_env(art: Path, storage_type: str, monkeypatch) -> None: - """File-served model declaring `storage_type` — the engine the guard parses every statement in, - and the engine the executor is then reconciled against.""" +def _engine_model_env(art: Path, engine: str, monkeypatch) -> None: + """File-served model declaring the engine's storage type — what the guard parses every statement + in, and what the executor is then reconciled against. The storage type is read from the engine's + `EngineFixture` rather than restated, so the model and the physical fixture cannot disagree.""" (art / "acme").mkdir(parents=True, exist_ok=True) - harness.write_disk_model(art / "acme", storage_type=storage_type) + harness.write_disk_model(art / "acme", storage_type=harness.ENGINE_FIXTURES[engine].storage_type) monkeypatch.delenv("AGAMI_DB_URL", raising=False) monkeypatch.delenv("APP_DATABASE_URL", raising=False) monkeypatch.setenv("AGAMI_ARTIFACTS_DIR", str(art)) @@ -238,12 +261,22 @@ def _engine_model_env(art: Path, storage_type: str, monkeypatch) -> None: monkeypatch.delenv("AGAMI_SQL_UNSCOPABLE_POSTURE", raising=False) +def _drop_corpus_tables(cur) -> None: + """Best-effort teardown. Tolerates a table that was never created — the next seed re-runs + DROP ... IF EXISTS first, so a failure here cannot leave stale rows behind.""" + for name in SCHEMA: + try: + cur.execute(f"DROP TABLE IF EXISTS {name}") + except Exception: + pass + + @pytest.fixture def mysql_safety_env(tmp_path, monkeypatch): """MySQL 8 — the engine that matters most here: backticks are MySQL's NATIVE identifier quote, so this is where a wrongly-dialected parse and a real execution can genuinely disagree.""" - pymysql = pytest.importorskip("pymysql") unavailable = _unavailable("MySQL") + pymysql = _require_driver("MySQL", "pymysql") c = _engine_creds("MySQL", "53307") if not c["password"]: unavailable("set AGAMI_IT_MYSQL_PASSWORD to run the MySQL safety corpus") @@ -259,24 +292,22 @@ def mysql_safety_env(tmp_path, monkeypatch): ) except Exception as exc: unavailable(f"no reachable MySQL ({exc})") - cur = conn.cursor() - harness.seed_schema( - lambda sql, params=None: cur.execute(sql, params), "MySQL", drop_first=True - ) - art = tmp_path / "art" - _engine_model_env(art, "MySQL", monkeypatch) - monkeypatch.setenv( - "DATASOURCE_URL__ACME", - f"mysql://{c['user']}:{c['password']}@{c['host']}:{c['port']}/{c['database']}", - ) + # Everything after the connect lives in the try, so a failure while seeding closes the + # connection instead of leaking one per test. try: + cur = conn.cursor() + harness.seed_schema( + lambda sql, params=None: cur.execute(sql, params), "MySQL", drop_first=True + ) + art = tmp_path / "art" + _engine_model_env(art, "MySQL", monkeypatch) + monkeypatch.setenv( + "DATASOURCE_URL__ACME", + f"mysql://{c['user']}:{c['password']}@{c['host']}:{c['port']}/{c['database']}", + ) yield finally: - for name in SCHEMA: - try: - cur.execute(f"DROP TABLE IF EXISTS {name}") - except Exception: - pass + _drop_corpus_tables(conn.cursor()) conn.close() @@ -285,80 +316,81 @@ def sqlserver_safety_env(tmp_path, monkeypatch): """SQL Server 2022 — the only bracket-quoting engine, and the worst measured divergence: under a generic parse `SELECT TOP n [col] FROM [tbl]` resolves to no tables and `TOP` as a column, so every scope gate finds nothing to object to.""" - pymssql = pytest.importorskip("pymssql") unavailable = _unavailable("SQLServer") + pymssql = _require_driver("SQLServer", "pymssql") c = _engine_creds("SQLServer", "11433") if not c["password"]: unavailable("set AGAMI_IT_SQLSERVER_PASSWORD to run the SQL Server safety corpus") - try: - boot = pymssql.connect( - server=c["host"], - port=int(c["port"]), - user=c["user"], - password=c["password"], - autocommit=True, - login_timeout=15, - ) - except Exception as exc: - unavailable(f"no reachable SQL Server ({exc})") - bcur = boot.cursor() - bcur.execute(f"IF DB_ID('{c['database']}') IS NULL CREATE DATABASE [{c['database']}]") - boot.close() - - conn = pymssql.connect( + connect = dict( server=c["host"], port=int(c["port"]), user=c["user"], password=c["password"], - database=c["database"], autocommit=True, login_timeout=15, ) - cur = conn.cursor() - harness.seed_schema( - lambda sql, params=None: cur.execute(sql, params), "SQLServer", drop_first=True - ) - art = tmp_path / "art" - _engine_model_env(art, "SQLServer", monkeypatch) - # SQL Server has no DSN scheme (`_env_datasource_dsn` handles postgres/mysql/sqlite/…), so the - # per-field credentials file is the supported channel. Clear the env DSN or it would win. - monkeypatch.delenv("DATASOURCE_URL__ACME", raising=False) - _write_credentials( - art, - "acme", - { - "type": "sqlserver", - "host": c["host"], - "port": c["port"], - "user": c["user"], - "password": c["password"], - "database": c["database"], - }, - monkeypatch, - ) try: + boot = pymssql.connect(**connect) + except Exception as exc: + unavailable(f"no reachable SQL Server ({exc})") + try: # the fixture database may not exist yet; create it before connecting into it + boot.cursor().execute( + f"IF DB_ID('{c['database']}') IS NULL CREATE DATABASE [{c['database']}]" + ) + finally: + boot.close() + + conn = pymssql.connect(database=c["database"], **connect) + try: + cur = conn.cursor() + harness.seed_schema( + lambda sql, params=None: cur.execute(sql, params), "SQLServer", drop_first=True + ) + art = tmp_path / "art" + _engine_model_env(art, "SQLServer", monkeypatch) + # SQL Server has no DSN scheme (`_env_datasource_dsn` handles postgres/mysql/sqlite/…), so + # the per-field credentials file is the supported channel. Clear the env DSN or it would win. + monkeypatch.delenv("DATASOURCE_URL__ACME", raising=False) + _write_credentials( + art, + "acme", + { + "type": harness.ENGINE_FIXTURES["SQLServer"].credential_type, + "host": c["host"], + "port": c["port"], + "user": c["user"], + "password": c["password"], + "database": c["database"], + }, + monkeypatch, + ) yield finally: - for name in SCHEMA: - try: - cur.execute(f"DROP TABLE IF EXISTS {name}") - except Exception: - pass + _drop_corpus_tables(conn.cursor()) conn.close() @pytest.fixture def duckdb_safety_env(tmp_path, monkeypatch): """DuckDB in-process — no server, no container. Included because it is cheap and because the - corpus previously claimed DuckDB coverage it did not have.""" - pytest.importorskip("duckdb") + corpus previously claimed DuckDB coverage it did not have. + + Its only availability risk is the driver, so it goes through the same REQUIRED contract: the + `integration-duckdb` job is this engine's ONLY evidence, and a skipped run there would be a + green job that proved nothing.""" + _require_driver("DuckDB", "duckdb") db = tmp_path / "shop.duckdb" harness.seed_duckdb(db) art = tmp_path / "art" _engine_model_env(art, "DuckDB", monkeypatch) # DuckDB has no DSN scheme either — same per-field credentials channel as SQL Server. monkeypatch.delenv("DATASOURCE_URL__ACME", raising=False) - _write_credentials(art, "acme", {"type": "duckdb", "path": str(db)}, monkeypatch) + _write_credentials( + art, + "acme", + {"type": harness.ENGINE_FIXTURES["DuckDB"].credential_type, "path": str(db)}, + monkeypatch, + ) yield diff --git a/tests/e2e/harness.py b/tests/e2e/harness.py index dc59110..e334eeb 100644 --- a/tests/e2e/harness.py +++ b/tests/e2e/harness.py @@ -14,6 +14,7 @@ import sqlite3 import subprocess import sys +from collections.abc import Callable from dataclasses import dataclass, field from pathlib import Path @@ -71,7 +72,9 @@ class EngineFixture: } -def seed_schema(execute, engine: str, *, drop_first: bool = False) -> None: +def seed_schema( + execute: Callable[..., object], engine: str, *, drop_first: bool = False +) -> None: """Materialize every `SCHEMA` table on `engine`, through the caller's `execute(sql, params=None)`. The adapter is passed in rather than a connection, so this function imports no driver and is diff --git a/tests/e2e/test_safety_corpus.py b/tests/e2e/test_safety_corpus.py index 4477d4e..f0291aa 100644 --- a/tests/e2e/test_safety_corpus.py +++ b/tests/e2e/test_safety_corpus.py @@ -15,19 +15,22 @@ This module adds the other half — the statement the guard vetted is a statement a REAL engine accepts — and the table below is the single place that says how far that goes per engine. -| Engine | Identifier quote | Coverage here | What is NOT proven | -|------------|------------------|-----------------------------------|---------------------------------------| -| SQLite | `"` `` ` `` `[]` | real, in-process (file path) | — | -| PostgreSQL | `"` | real server (`integration-pg`) | — | -| MySQL | `` ` `` | real server (`mysql:8`) | — | -| SQLServer | `[]` | real server (`mssql/server:2022`) | — | -| DuckDB | `"` | real, in-process | — | -| Redshift | `"` | verdict only (ACE-079) | execution parity | -| Snowflake | `"` | verdict only (ACE-079) | execution parity | -| BigQuery | `` ` `` | verdict only (ACE-079) | execution parity | -| Databricks | `` ` `` | verdict only (ACE-079) | execution parity | -| Oracle | `"` | verdict only (ACE-079) | execution parity | -| Trino | `"` | verdict only (ACE-079) | execution parity | +| Engine | Identifier quote | Coverage here | What is NOT proven | +|------------|------------------|-----------------------------------|-------------------------------| +| SQLite | `"` `` ` `` `[]` | real, in-process (file path) | anything outside these cases | +| PostgreSQL | `"` | real server (`integration-pg`) | anything outside these cases | +| MySQL | `` ` `` | real server (`mysql:8`) | anything outside these cases | +| SQLServer | `[]` | real server (`mssql/server:2022`) | anything outside these cases | +| DuckDB | `"` | real, in-process | anything outside these cases | +| Redshift | `"` | verdict only (ACE-079) | execution parity | +| Snowflake | `"` | verdict only (ACE-079) | execution parity | +| BigQuery | `` ` `` | verdict only (ACE-079) | execution parity | +| Databricks | `` ` `` | verdict only (ACE-079) | execution parity | +| Oracle | `"` | verdict only (ACE-079) | execution parity | +| Trino | `"` | verdict only (ACE-079) | execution parity | + +An executing engine is proven for the corpus's own cases — that is a regression harness, not a +claim about the engine's whole grammar. The five executing engines cover all three identifier-quoting families — backtick (MySQL), bracket (SQLServer) and double-quote (PostgreSQL/SQLite/DuckDB) — which is the axis ACE-079's defect class @@ -71,6 +74,12 @@ def assert_outcome(env: dict, expect: str, sql: str) -> None: assert env["status"] == "ok", (sql, env) assert "refusal" not in env assert env["audit_id"] + # A result set actually came back. Without this, "ok" only proves the guard ALLOWED the + # statement — a fixture that seeded the wrong database, or an engine that accepted the + # statement and matched nothing, would still pass, and the per-engine cases whose whole + # point is that the vetted statement EXECUTES would prove only half of it. Every governed + # case in the corpus selects from seeded rows, so an empty result is a broken fixture. + assert env["data"]["rows"], (sql, env) elif expect == "bounded": # Availability: a runaway result is bounded — EITHER a resource_limit refusal, OR an ok # Envelope flagged truncated with a row_cap in `applied` (capped + flagged, never silent). @@ -126,6 +135,24 @@ def test_safety_corpus_engine(engine, case, surface, request): assert_outcome(env, case.expect, case.sql) +def test_engine_cases_stay_out_of_the_postgres_gate(): + """The `integration-pg` job selects with `-k "db_path or role"` and carries the F9 done-bar — + the read-only role floor and file/db parity. Its result must stay independent of the engine + jobs, which is true today only because no engine node id contains `db_path` or `role`. + + That is an accident waiting to be broken by a case note, so it is asserted rather than assumed: + a new case called e.g. "role-column" would silently join the done-bar job and let a container + hiccup there turn the F9 gate red. `-k` matches case-insensitively, so this does too. + """ + for engine, case in ENGINE_CASES: + node_id = f"{engine}:{case.id}" + for selector in ("db_path", "role"): + assert selector not in node_id.lower(), ( + f"engine case {node_id!r} would be selected by integration-pg's " + f'-k "db_path or role", coupling the F9 done-bar to an engine container' + ) + + @pytest.mark.parametrize("case", DB_PATH_CASES, ids=[c.id for c in DB_PATH_CASES]) def test_safety_corpus_db_path(case, surface, db_safety_env): # DB-served model (Postgres app DB) + Postgres datasource read as the read-only role. IDENTICAL