ACE-040: make the DB-required sentinel actually fail, and close five corpus gaps - #161
ACE-040: make the DB-required sentinel actually fail, and close five corpus gaps#161sandeep-agami wants to merge 1 commit into
Conversation
…pus gaps The sentinel that is supposed to stop the Postgres job passing while proving nothing did not work. `pytest.importorskip` runs before the sentinel is ever consulted, so a missing driver or transport dependency skipped every DB-backed test and the job exited 0 -- green, having executed neither the Postgres-served safety corpus nor the read-only role floor. Reproduced by blocking the import: exit 0, 7 skipped, with the sentinel set. `itdeps.importorfail` is the drop-in that respects it: identical to importorskip when the sentinel is unset, so the DB-free job keeps skipping cleanly, and a hard failure when it is set. Both call sites now use it -- the driver import in the `pg_admin` fixture and the four module-level imports the corpus module needs, either of which could silently take the whole corpus out of the run. The new test drives the job's own invocation in a subprocess with a dependency made unimportable and asserts the exit code both ways. It fails on the previous code for both dependencies and passes now. It also caught a defect in the first version of this fix: a bare `pytest.skip` at module scope is a collection ERROR, which would have turned the DB-free job red for a dependency it is allowed to be missing -- hence the explicit `module_level` flag. The corpus gains nine vectors it did not carry: server-side file functions, remote SQL execution, a TCL statement and a session SET, a statement a comment hides from a splitter that trusts semicolons, and writes smuggled inside a CTE. All nine were already refused, on both the SQLite and Postgres paths -- these close a coverage gap rather than report a hole, so they pass immediately. A cross-join runaway is added at the shared executor for the bounded outcome. One coupling is written down while adding these: a case label containing "role" or "db_path" changes which tests the DB-backed job selects, which is how a file-path case leaked into it here before the label was renamed. Verified against Postgres 16: the job goes from 88 to 108 passed, the whole e2e directory to 230, and the DB-free suite from 2280 to 2306 passed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR hardens the AGAMI_IT_PG_REQUIRED sentinel so the Postgres integration job cannot pass “green” when prerequisites (DB driver or transport deps) are missing, and expands the safety SQL corpus with additional refusal/availability coverage cases.
Changes:
- Added
tests/e2e/itdeps.py(importorfail+unavailable) to unify “skip vs fail” behavior underAGAMI_IT_PG_REQUIRED, and wired it into the DB-backed e2e tests/fixtures. - Added
tests/e2e/test_it_sentinel.pyto prove the sentinel behavior via a subprocess that blocks imports and asserts the job’s exit code both with and without the sentinel. - Expanded
tests/safety/corpus.pywith additional refusal cases (filesystem/network/session/CTE-write) and a cross-join availability case; documented a CI-kselection footgun inCase.id.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/safety/corpus.py | Adds corpus cases + documents the -k "db_path or role" selection coupling via the param id. |
| tests/e2e/test_safety_corpus.py | Replaces module-level importorskip with sentinel-aware importorfail for required CI runs. |
| tests/e2e/test_it_sentinel.py | New subprocess-based tests that assert the DB-backed job fails when prerequisites are missing under the sentinel. |
| tests/e2e/itdeps.py | New helper module implementing the sentinel-aware “skip vs fail” dependency gate. |
| tests/e2e/conftest.py | Routes the Postgres fixtures’ dependency and “DB unavailable” gating through itdeps. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # NOTE: this string becomes the pytest test id, and the DB-backed CI job selects its tests | ||
| # with `-k "db_path or role"` — so a `cls`/`note` containing "role" or "db_path" silently | ||
| # changes which tests that job runs. Keep those two words out of new labels. |
| def importorfail(name: str): | ||
| """`pytest.importorskip`, except a missing module FAILS when the sentinel is set. | ||
|
|
||
| Called at module scope in the DB-backed test modules, so the failure surfaces as a collection | ||
| error and the job exits non-zero instead of silently reporting a skip.""" |
| assert blocked in proc.stdout, ( | ||
| f"the job failed, but not because {blocked} was missing — that half is still skipping " | ||
| f"silently:\n{proc.stdout}\n{proc.stderr}" | ||
| ) |
Converting to draft — there is a fourth defeat path, and it is the one that mattersThe fix wraps five dependency sites. But the job's real precondition is "the DB-backed tests ran", and nothing asserts that. Reviewer demonstrated it: rename Exit 0. Green. The entire Postgres-served corpus — the file/db parity proof, the whole reason the job exists — is gone, and the sentinel says nothing, because no import failed. An empty So the change fixes the defeat path that was reported and leaves a wider one open. Wrapping call sites one at a time is the wrong shape — the floor has to be asserted once, per session, derived from the corpus so it cannot drift: def pytest_sessionfinish(session, exitstatus):
if not itdeps.db_required():
return
expected = 2 * len([c for c in CASES if c.runs_on("PostgreSQL")]) + 6 # 2 surfaces + role floor
if _DB_PROOF_PASSED < expected:
session.exitstatus = 1That closes the rename gap, the empty-parametrize gap, a
|
Spec: ACE-040
Test and CI infrastructure only — no file under
packages/agami-core/src/changes.The sentinel was defeated by two independent paths
AGAMI_IT_PG_REQUIREDexists so the Postgres integration job hard-fails rather than skips when a database is expected. It did not work. With the sentinel set and drivers absent:Two causes, not one: the driver import in
pg_admin, and four module-levelimportorskipcalls that take the whole corpus module out before the sentinel is ever consulted. The DB-unreachable path already behaved correctly (exit 1); only the dependency path was open.tests/e2e/itdeps.pyaddsimportorfail—importorskipwhen the sentinel is unset, hard failure when set — wired into both.tests/e2e/test_it_sentinel.pyruns the job's own invocation in a subprocess with a dependency blocked and asserts the exit code both ways. Verified failing on base by reverting only the two call-site edits: both sentinel arms failed, both "still skips" arms passed.Two things worth recording from building it:
ImportErroris not caught by this pytest'simportorskip, which defaultsexc_typetoModuleNotFoundError— an early version made base look stricter than it is.pytest.skipis a collection error, so a first cut would have turned the DB-free job red whenevermcp/starlette/sqlglot/pydanticwere absent. Hence the explicitmodule_levelflag.Corpus gaps
Nine cases added —
pg_read_file,lo_import,dblink,dblink_exec,SELECT …; COMMIT,SET ROLE postgres, a line comment hiding a statement separator, and CTE-hiddenDELETE/INSERT— plus a cross-join runaway asserting"bounded".These are pure coverage gaps and passed immediately. They caught nothing. Every one was probed first on both paths and already refused with
permission, identically on SQLite and Postgres.A trap for future authors
The CI job selects with
-k "db_path or role", so a case label containingrolesilently pulls file-path tests into the DB job — an early label did exactly that, 108 → 110. Renamed, and the coupling is now documented onCase.idwhere an author will see it.Verification
Postgres job: 88 → 108 passed. Full
tests/e2e/: 230 passed. DB-free suite: 2280 → 2306.dev.py check: 2306 passed, 115 skipped, 2 xfailed; ruff + gitleaks clean; no vendored-lib drift.One unexplained transient: in six job runs, one reported
107 passed, 1 error; four consecutive re-runs were clean at 108. It followed a working-tree rewrite mid-session, so most likely stale bytecode from local tooling — but the traceback was not captured, so flakiness in the role fixtures (which drop and recreate a global role per test, already marked serial-only) is not ruled out.