26 KiB

Raw Blame History

ADR 0016 — Tests run against a real PostgreSQL service container, never SQLite, and constraint tests assert that forbidden writes raise


Status	Accepted — 2026-07-30
Scope	The database used by the test suite, the one required CI pipeline and its gates, the test layering, and the specific class of test that asserts a database invariant is enforced. Does not choose the hosting platform (ADR 0012) or the migration authority (ADR 0017)
Owner	Talha Ahmed owns the workflow definition, the service-container configuration, branch protection and the migration/grant fixtures. Ahmed Mujtaba owns the constraint-test suite (the "forbidden writes raise" tests), the module-facade test layer, the fixture generator and the Playwright journeys — each independently demonstrable, each with a Talha review checkpoint
Consistent with	`_decisions.md` Part 1 → Testing, CI and delivery process; Part 2 → Database engine (the extension and feature set the tests must exercise), Audit table strategy, Soft delete, PII classification and retention
Related ADRs	0002 (the engine features the suite must not be able to avoid), 0017 (plain-SQL migrations, which only a real Postgres can apply), 0007 (`UPDATE` revoked on `*_version` tables — untestable without real grants), 0015 (`lint-imports` runs in the same workflow), 0012 (the same pipeline deploys), 0014 (the XSS grep gates live here)

Context

There is no test of any kind in the repository, no test runner, and no CI configuration — .github/ is absent (_repo-findings.md §B, §H). So this is entirely additive and can be shaped correctly from the start, which is a rare position and worth not wasting.

Why the test database is a real architectural decision, not a preference

Because in this design the invariants are the schema. The brief's non-negotiable constraints are implemented almost entirely as database objects, not as Python:

Constraint	Mechanism (ADR 0002, 0007, 0008)	Exists in SQLite?
Raw intake must exist before candidate creation	`job_application.raw_intake_id` / `candidate.created_from_raw_intake_id` `NOT NULL` against non-deferrable FKs	FKs yes, but the enforcement semantics differ and are off by default in older builds
A malformed email cannot create a candidate	`CHECK` with a regex on `candidate_email.address`, plus a `DEFERRABLE INITIALLY DEFERRED` constraint trigger requiring at least one contact channel at `COMMIT`	No. No deferrable constraint triggers
One email, one identity	Partial unique index on the normalised address `WHERE deleted_at IS NULL`	No. No partial indexes on expressions in the form needed
Jobs, requirements and scoring configs are immutable	`UPDATE` revoked at column level on six `*_version` tables, plus an immutability trigger	No. No `GRANT` system at all
ATS scores are append-only	`UPDATE`/`DELETE` revoked on `ats_result`	No
No interview double-booking	`tstzrange` + GiST `EXCLUDE` constraint	No. No exclusion constraints, no range types
Candidate search	`tsvector` / `ts_rank_cd` + `pg_trgm` similarity over a trigger-maintained index table	No
Audit growth	Declarative `RANGE` partitioning, hash-chained rows	No
Durable job queue	`procrastinate` on `LISTEN`/`NOTIFY` with `SELECT … FOR UPDATE SKIP LOCKED`	No
Raw payload storage and parser output	`JSONB` + GIN	Partial (JSON1, no GIN)

Read that table as a single sentence: on SQLite, roughly nine of the design's load-bearing guarantees are not merely untested — they are unexpressible. A green suite on SQLite would be a green suite against a different, weaker system.

The second-order effect, which is the real argument

The failure mode is not "a bug slips through". It is subtler and worse: the team starts avoiding the features the design depends on. If the suite runs on SQLite, then a partial unique index cannot be tested, so the developer under time pressure moves the uniqueness check into Python where it can be tested. That check is then racy, and the invariant that was a database fact becomes a code path — which is precisely the failure the prototype already demonstrates, at js/data.js:117-127, and precisely what _decisions.md Part 2 was written to prevent.

Once that happens a few times, the test suite is still green and no longer describes the system. _decisions.md states this as a deliberate call, and this ADR is where it is argued.

The other constraint shaping the pipeline

Two developers, one of them junior, and one reviewer. That points the same direction twice: the senior needs the boundary and schema rules enforced mechanically because he is the only reviewer, and the junior needs varied, independently demonstrable work — for which testing is close to ideal, because every module facade is a self-contained target with a visible pass/fail.

Options considered

Option A — Real PostgreSQL as a CI service container, matching the production major version (chosen)

For: the suite exercises the actual engine, the actual extensions, the actual grants and the actual migrations. Every invariant in the table above becomes assertable. GitHub Actions services: makes it roughly four lines of YAML and a health check.
Against: slower than in-memory SQLite — container start plus migration apply per job. Requires migrations to be fast and correct, and requires the CI role/grant setup to mirror production.
Verdict: chosen. The cost is measured in seconds per run; the alternative is measured in invariants.

Option B — SQLite in CI for speed, PostgreSQL only in staging

For: fast, zero infrastructure, in-memory, trivially parallel. The standard Django default.
Against: everything in the Context table. Nine load-bearing guarantees unexpressible; the suite silently redefines "passing" as "passing on a system we do not run"; and the drift is toward moving invariants into application code where they become racy.
Verdict: rejected. This is the option this ADR exists to reject on the record.

Option C — A shared long-lived PostgreSQL instance for CI

For: no per-job container start; slightly faster.
Against: cross-job interference, ordering dependencies, "works on the second run" flakiness, and no clean way to test migrations from empty. Also a shared mutable resource owned by nobody.
Verdict: rejected. Ephemeral per-job is the point.

Option D — Mock the database; test the service layer against fakes

For: fastest possible suite; forces clean interfaces.
Against: the guarantees under test are database behaviour. A fake repository asserting "duplicate email rejected" tests the fake. _decisions.md says it flatly: no mocking of the database.
Verdict: rejected. Fakes are used for the AI provider (ADR 0011), where the external system is genuinely non-deterministic and expensive, and nowhere else.

Option E — Testcontainers instead of the CI runner's native service block

For: identical database setup locally and in CI, programmatic lifecycle, easy multi-version matrices.
Against: requires a Docker daemon inside the test process, slower startup per session, and an extra dependency. The services: block plus a local docker compose (ADR 0012) already gives parity with less machinery.
Verdict: rejected for now, on simplicity. Revisit at T4 if a version matrix becomes necessary.

Option F — Coverage percentage as the primary quality gate

For: one number, easy to enforce, easy to report.
Against: it rewards testing getters. A suite at 85% coverage that cannot express a deferrable constraint trigger is worse than a suite at 45% that asserts every forbidden write raises.
Verdict: rejected as a gate. Coverage is reported for information, never enforced as a threshold.

Decision

1. One required CI workflow, on GitHub Actions

Greenfield — .github/ is absent (§B). One required pipeline, not several, so there is exactly one answer to "is this mergeable".

#	Gate	Tool	Why it is required
1	Lint and format	`ruff`	Cheap, first, fails fast
2	Types	`mypy`	The facade signature plus `mypy` is the inter-module contract (ADR 0015) — so this gate is load-bearing, not hygiene
3	Module boundaries	`lint-imports` (ADR 0015)	A boundary violation is a failed build, not a review argument
4	Tests against real PostgreSQL	`pytest` + `services: postgres`	The subject of this ADR
5a	Migration drift — models vs state	`makemigrations --check --dry-run`	ADR 0017's ORM gate: it compares models against declared migration state, never the live database, so a model edited without its migration fails the build
5b	Migration drift — database objects	`pg_dump --schema-only` plus a `pg_trigger` / `column_privileges` catalogue diff against a committed expected dump	ADR 0017's SQL gate, and the security-relevant half: it is what catches a re-`GRANT`ed `UPDATE` on `audit_event` or a dropped immutability trigger, neither of which any ORM check would notice. Only possible because §2 builds the schema from the migrations, from empty
6	Frontend lint	ESLint with `react/no-danger` as an error	Makes the XSS guarantee structural (ADR 0013)
7	Design tokens	`stylelint` token allowlist	Keeps the frozen design system frozen (ADR 0013)
8	Frontend types	`tsc --noEmit`	Catches the candidate/application entity churn
9	Component tests	`Vitest`	The ported `js/ui.js` primitives
10	Smoke journeys	5 `Playwright` tests	Below
11	Prototype XSS gates	grep gates on unescaped interpolation and inline handlers (ADR 0014)	Keeps the Phase 0 patch from decaying
12	Secrets	`gitleaks`	No `.env` exists today (§B); this is what keeps that true

Branch protection: no direct pushes to main; every PR requires Talha's review. That review requirement is the mandated review checkpoint for the junior's work — it is enforced by the platform rather than remembered.

2. The database configuration

services:
  postgres:
    image: postgres:17
    env:
      POSTGRES_PASSWORD: postgres
    options: >-
      --health-cmd pg_isready --health-interval 5s
      --health-timeout 5s --health-retries 10      
    ports: ["5432:5432"]

Rules that make this parity rather than theatre:

Rule	Reason
The major version matches production (target 17, per ADR 0002)	A minor drift is acceptable; a major one is not — partitioning and planner behaviour change
The full Phase 1 extension set is installed: `pg_trgm`, `unaccent`, `btree_gist`, `pgcrypto`	A missing extension must fail loudly at migration time, not silently skip an index
Schemas `app`/`ref`/`audit`/`ai`/`staging` are created by migration 0001, not by a test fixture	The schema layout is production behaviour and must be exercised as such
*The schema is built by applying `db/migrations/.sql` in order from empty** — never by `--create-db` from models, never from a dumped snapshot	This is the only way the migration path itself is tested. It is also what makes ADR 0017 verifiable
Both roles exist: a migration role with DDL, and an application role without DDL and without `UPDATE`/`DELETE` on append-only tables	Without this, the append-only guarantee cannot be tested at all — and it is the guarantee most likely to be quietly broken
`timezone = 'UTC'` on the server and the application role	The timestamp discipline in `_decisions.md` Part 2 is only real if the test environment shares it
Database per test session, transaction rollback per test, `TRUNCATE` only where a trigger or `COMMIT`-time constraint must fire	Deferrable constraint triggers fire at `COMMIT`, so some tests genuinely need to commit

3. The test layering

Layer	Target	Owner	Notes
Unit	`scoring` component functions, `document_parsing` extractors, dedupe signal scoring	Talha writes, Ahmed extends	Pure functions, no database, genuinely fast
Constraint	Database invariants — §4 below	Ahmed	The layer that makes this ADR worth its cost
Service / facade	Each module's `service.py`	Ahmed, per module	The facade is the test seam, which is what makes ADR 0015's rule 1 pay off twice
Integration	Intake chain end to end: `raw_intake → attachment → scan → parse → candidate → application → score`	Talha	Real queue (`procrastinate` on the same database), fake AI provider (ADR 0011)
Smoke (Playwright)	5 journeys: login; ingest a CV and see it parsed; promote a submission to candidate; create and publish a requisition version; move an application through two stages	Ahmed	Deliberately five. They are a deploy gate, not a regression suite

No mocking of the database, at any layer. The only fake in the suite is FakeAiProvider (ADR 0011), including its deliberately ugly cases.

4. Constraint tests — the class of test that asserts forbidden writes raise

This is the distinguishing practice, and it is the half that a SQLite suite cannot contain at all. Each test performs a write that must fail and asserts the specific exception:

#	Attempted write	Expected	Guards
1	`UPDATE app.job_version SET …` as the application role	`InsufficientPrivilege`	ADR 0007; `08` AC1.7
2	`UPDATE app.ats_result SET score = …`	`InsufficientPrivilege`	Append-only scores
3	`UPDATE`/`DELETE` on `audit.audit_event`	`InsufficientPrivilege`	Audit immutability
4	`INSERT INTO app.candidate` with `created_from_raw_intake_id = NULL`	`NotNullViolation`	Raw intake before candidate
5	`INSERT INTO app.job_application` with `raw_intake_id = NULL`	`NotNullViolation`	Same, application side
6	`INSERT INTO app.candidate_email` with `'not-an-email'`	`CheckViolation`	Malformed email cannot create a candidate
7	`COMMIT` a candidate with zero contact channels	`IntegrityError` at commit, not at insert	The deferrable constraint trigger — and the assertion that it is deferred is itself part of the test
8	Second active `candidate_email` with the same normalised address	`UniqueViolation`	Partial unique index; and the same insert with `deleted_at` set must succeed, which is the half that proves the index is partial
9	Two overlapping interviews for one interviewer	`ExclusionViolation`	GiST `EXCLUDE` on `tstzrange`
10	`scoring_config_version` whose criterion weights do not sum to 1.0	`IntegrityError` from the constraint trigger	ADR 0007
11	`DDL` as the application role	`InsufficientPrivilege`	Role separation is real, not documentation
12	`undo_merge()` out of `seq` order	Application-level error, with the merge log unchanged afterwards	ADR 0008 stack discipline
13	`application.transition()` into a terminal-negative stage with `actor_kind` of `system`, `integration` and `ai_agent` (three parametrised cases), plus the positive case proving `'user'` is permitted	`TerminalTransitionRequiresHumanActor`, and the message must match the literal string `actor_kind = 'user'`	"AI must never auto-reject" — `_decisions.md` RULING-01, `adr/0011` §"Guard literal"
14	`INSERT INTO app.pipeline_transition_rule` with `is_terminal_negative = true` and `allowed_actor_kinds <> ARRAY['user']`, and separately with a member outside the actor enum	`CheckViolation` from `ck_terminal_negative_user_only` and `ck_allowed_actor_kinds` respectively	Same rule, rules-layer expression

Three properties of this list are deliberate. First, each test asserts the specific exception type, not merely "something raised" — a NotNullViolation where a CheckViolation was expected means the invariant moved. Second, every test has a positive twin: the same operation that must fail in one form must succeed in its legitimate form (test 8 is the clearest case). A suite of only-negative tests passes when the table is missing.

Third, test 13 additionally pins the literal comparison value, not just the exception type, and it is the only test in the suite that does. This is not belt-and-braces: the guard's whole content is the string it compares against, and that string was written four incompatible ways across this package before _decisions.md RULING-01 settled it (actor_type != human, actor_kind = 'human', actor_kind = 'user', allowed_actor_kind = 'user_or_system'). Two of those spellings compare against a value the CHECK constraint cannot hold, so they either refuse every legitimate recruiter rejection or throw. A test asserting only pytest.raises(TerminalTransitionRequiresHumanActor) passes identically against all four, which is precisely how the defect survived. The assertion is therefore pytest.raises(TerminalTransitionRequiresHumanActor, match=r"actor_kind = 'user'"). Task A-26 in 07-implementation-plan.md owns it.

5. What is deliberately not a gate

Not a gate	Why
Coverage percentage	Rewards testing getters. Reported, never enforced
A separate QA phase before release	Two developers. Quality is in the pipeline or it is nowhere
Contract tests between modules	Unnecessary in-process; the facade signature plus `mypy` is the contract (ADR 0015)
Performance/load tests in the required workflow	At 66 seats there is nothing to defend yet. Search latency instrumentation (ADR 0006) is production telemetry, not a CI gate
Mutation testing	Real value, wrong phase for two developers

6. Delivery

One deploy path: merge to main → the required workflow passes → auto-deploy to staging → manual promote to production (ADR 0012). The same workflow that gates the merge builds the image that is promoted, so the artefact deployed is the artefact tested.

Justification

The one-line version: the invariants in this design live in the database, so a test suite that cannot see the database cannot see the design.

Everything else follows. The extension set, the two roles, the migrate-from-empty rule and the twelve constraint tests are not thoroughness for its own sake — each one corresponds to a specific non-negotiable constraint from the brief, and each one is unverifiable on any other engine. 08-requirements-traceability.md cites AC1.7 ("UPDATE against job_version raises an exception, asserted by tests") as the acceptance evidence for the versioning requirement; without this decision, that acceptance criterion has no home.

On cost, honestly. A service container plus applying the full migration set from empty costs perhaps 20–60 seconds per CI run versus in-memory SQLite. Over a year at two developers that is real time, and it is worth naming rather than hand-waving. It buys the difference between a suite that describes the system and a suite that describes a weaker system that happens to share some Python.

On the two-role setup, which is the most-skipped part. Most projects run tests as the owner because it is easier. Doing that here would make tests 1, 2, 3 and 11 impossible — and those four are the append-only guarantee, which is the one that protects the score-reproducibility requirement (BRD §6.2) and the audit trail. If the tests run as a superuser, "append-only" is a documented intention with nothing behind it.

On giving the constraint suite to the junior. It is the best-shaped work in the package for him: each test is tiny, independently demonstrable, and either red or green with no interpretation. It also teaches the data model from the outside in — he learns what the schema forbids, which is the fastest route to understanding why it is shaped that way. And it directly mitigates the bus-factor risk, because writing these tests requires reading Talha's schema closely.

The tradeoff, stated plainly

We are paying 20–60 seconds per CI run and the setup cost of two database roles and an extension-complete test database, in exchange for a suite in which every load-bearing invariant is assertable and — more importantly — in which nobody is ever incentivised to move an invariant out of the database to make it testable. Given that the invariants are the design, that is not a close call.

Consequences

Positive

Every invariant in _decisions.md Part 2 is assertable, and twelve of them are asserted explicitly.
The migration path is tested on every run, because the schema is built from db/migrations/*.sql from empty — which is what makes ADR 0017's "plain SQL is the authority" verifiable rather than aspirational.
The append-only guarantee is real, because the tests run as a role that genuinely lacks the privilege.
Nobody is pushed toward moving a constraint into racy Python to make it testable — the drift the prototype demonstrates cannot start here.
The junior gets a large, varied, visible workstream (constraint tests, facade tests, fixtures, Playwright) that is not CRUD.
Branch protection makes the required Talha review a platform property rather than a remembered practice.
One required workflow means one answer to "is this mergeable", and the tested artefact is the promoted artefact.

Negative — the costs being accepted

Slower CI: container start plus a full migration apply, 20–60 s per run, growing as the migration count grows.
Migrations must stay fast. A slow migration is now a slow test suite on every push — which is a useful pressure, but a real one.
Two-role setup complexity in test fixtures, and a genuine footgun: a fixture that accidentally uses the migration role makes tests 1–3 and 11 pass vacuously. The positive twins in §4 are partly there to catch this.
Commit-requiring tests are slower and harder to isolate than transaction-rollback tests, and the deferrable-constraint tests must commit by construction.
The extension set is a coupling to a specific managed-Postgres capability. If a future host lacks btree_gist the suite fails, loudly, which is correct but is a constraint on hosting choices (ADR 0012).
No coverage gate means no single number to show a stakeholder who asks "how well tested is it". Accepted; the answer is the constraint list, which is a better answer and a harder one to communicate.

Risks

#	Risk	Likelihood	Impact	Mitigation
R1	CI time grows and someone proposes SQLite "just for the fast unit job"	Medium — this is the realistic decay path	High	Rejected on the record here. If speed is the problem, the fix is a fast lint/type/unit job plus the Postgres job, parallel, both required — never a substituted engine
R2	Tests silently run as the migration/owner role, making the privilege tests vacuous	Medium	High — the guarantee looks tested and is not	Test 11 (DDL must fail) is the canary: if it passes, the role is right. Positive twins throughout. Fixture role assignment reviewed by Talha
R3	The CI Postgres drifts from production in version, extensions, or role grants	Medium	Medium	Version pinned in the workflow and asserted against ADR 0002; the extension list is created by migration 0001, so a mismatch fails at apply time; grants are part of the migration set, not the fixture
R4	Flaky commit-requiring tests get marked `xfail` to unblock a merge	Medium	Medium–High	`xfail` on any test in §4 requires a Talha review and a linked issue. A skipped constraint test is a disabled invariant
R5	Migration-from-empty becomes slow enough that someone caches a schema dump	Medium	Medium	If it happens, it must be a documented cache with a hash of the migration directory as the key, and one job per day still building from empty. Never the default path
R6	The suite grows negative-only, so a dropped table passes	Low–Medium	Medium	Every constraint test has a positive twin, stated as a rule in §4
R7	Playwright journeys become a slow, flaky regression suite instead of five smoke tests	Medium	Low–Medium	Capped at five by decision. New end-to-end coverage goes to facade or integration tests, which are faster and less brittle

Revisit conditions

#	Condition	Expected move
T1	The required workflow exceeds ~15 minutes	Parallelise into jobs (lint/type, backend, frontend, e2e) and shard `pytest`. Do not change the engine
T2	Migration-from-empty exceeds ~60 seconds	Squash early migrations into a reviewed baseline that is itself applied from empty in a daily job, keeping the property that the migration path is tested
T3	A major Postgres upgrade is planned for production	Add a version matrix (old + new) to the Postgres job for one release cycle, then drop the old
T4	Local/CI database setup diverges enough to cause "works on my machine"	Adopt Testcontainers (Option E) for one definition shared by both
T5	A third developer joins, or merge queue contention appears	Add a merge queue; revisit whether the five Playwright journeys should run pre-merge or post-merge
T6	Any constraint in §4 moves out of the database into application code	Reopen this ADR and ADR 0002 — that change invalidates the premise of both, and it is the single change most likely to happen quietly

ADR 0002 — PostgreSQL as the single primary database. The feature list that makes SQLite non-substitutable is that ADR's decision, and this one is what keeps it honest.
ADR 0017 — plain-SQL migrations as the schema authority. Gates 5a and 5b are its two drift gates, and the migrate-from-empty rule in §2 is what makes 5b possible at all: without a database built by running every migration, there is nothing to dump and nothing to diff.
ADR 0007 — versioning and the UPDATE-revoked *_version tables. Constraint tests 1 and 10 are its acceptance evidence (08 AC1.7).
ADR 0008 — duplicate merge and reversal; constraint test 12 asserts stack discipline.
ADR 0015 — module boundaries; lint-imports and the deliberately-violating fixture run as gates 3 and its companion step in this same workflow.
ADR 0011 — the AI provider port; FakeAiProvider is the only fake in the suite, and the reason it is allowed is stated there.
ADR 0013 / 0014 — gates 6, 7, 8, 9 and 11 (react/no-danger, stylelint tokens, tsc, Vitest, the XSS grep gates).
ADR 0012 — deployment: the same workflow auto-deploys staging and gates the manual production promote, and the local docker compose stack is the parity target.
_decisions.md Part 1 → Testing, CI and delivery process.
_repo-findings.md §B (no tests, no runner, no CI), §H.

26 KiB Raw Blame History Unescape Escape