Sprint 4 — Discovery, Notification Pipeline, and the CI Gate That Almost Was Not

Operator reported that the discovery worker was healthy and on the current image, but only one cycle had ever run (the 18:57 boot run) and no subsequent interval fire across 71 minutes. /api/v1/inventory was missing every VM entity it should have materialised.

Two defects in one. The interval issue was the discovery_main entrypoint passing next_run_time=None to scheduler.add_job. In APScheduler 3.x that does NOT mean fire-immediately — it puts the job in the PAUSED state, because _real_add_job only computes the trigger first-fire time if not hasattr(job, "next_run_time"), and passing None explicitly satisfies the hasattr check while leaving the attribute None forever. The one cycle that ran was the explicit _enqueue_discovery() kick at boot, not the trigger. Verified empirically against APScheduler 3.11.2. The operator hypothesis — that the AsyncIO scheduler was blocked by the dramatiq consumer — was refuted by reading the actual worker: it is a BackgroundScheduler in its own thread, the worker thread is non-blocking, the seal worker uses the same pattern and works.

Delete next_run_time=None and the misleading “Fire once shortly after boot” comment. Extract the add_job call into a _make_scheduler() helper so the regression test exercises the production code path, not a re-rolled copy. Pin via AST (no next_run_time kwarg, no positional past the canonical two — blocks the index-10 positional escape). Pin via behaviour (build the scheduler, start it, assert next_run_time is set). Also instrument the adapter so the next live cycle is self-diagnosing: per-node qemu HTTP 200 vm_count=N plus per-node tally plus a warning when qemu returns non-200. Commit 1c29bee.

Operator framed this as a code bug: the proxmox adapter must not be reading the per-VM /config endpoint, so it never persists VM entities. /api/v1/inventory had hardware (pve1/2/3) plus 12 orphan docker_container rows but zero VM-type entities.

The framing was wrong — the adapter ALREADY materialises a vm/proxmox_vm InventoryEntity from any qemu-LIST row that carries a vmid (proxmox.py:148-198 in the qemu-list+gather block and the entity-append loop), independent of the /config call. The /config call only adds metadata.ip_addresses. An adversarial root-cause workflow refuted the framing and pointed upstream: zero VMs plus qemu HTTP 200 plus zero observed /config calls fingerprints an empty qemu LIST, which means the deployed PVEAuditor token cannot enumerate the homelab VMs (privilege-separation or VM.Audit scoped wrong). The proxmox.py docstring asserted “Config is readable with the read-only PVEAuditor grant (VM.Audit)” as a hard fact — that assertion was unverified and now contradicted by the live behaviour. The workflow also flagged the original operator math — “first new-code cycle should report ~15-20 VMs touched, only 2 touched” — as mathematically wrong: touched counts only discovered+updated (not unchanged), and 2 touched on an already-populated DB means VMs have been absent on EVERY prior cycle, not just this boot.

Adapter code untouched (it is correct). Instrumented proxmox.discover so the next live cycle reveals which branch fires (empty list vs token-rejected vs vmid-shape) without a manual curl. Rewrote the docstring to mark the VM.Audit grant as an architectural ASSUMPTION (not verified deployment state) and to point at the new instrumentation log line as the source of truth, with the decisive operator-side curl spelled out inline. Added a project memory note (proxmox_token_audit_state.md) so future agents inherit the truthful state instead of re-reading the old confident docstring. The decisive curl is for the operator to run; once the token is fixed (or replaced) the memory + docstring get updated to record the empirical state.

Adversarial review of the Piece 2 diff (the new E2E smoke) flagged that the catch-all 404 was registered last in setupApiStubs — and in Playwright the last-registered route wins. So every /api/v1/me, every /tenants list, every audit fetch was returning 404, every specific stub was dead, and the tests would either fail (good outcome) or pass for the wrong reasons (worse).

Playwright matches route handlers in REVERSE registration order — documented behaviour in playwright-core types.d.ts. Earlier handlers only run if the later one calls route.fallback(). Also separate but related: single-asterisk glob in page.route compiles to [^/]* which does not span slash, so **/api/v1/audit* does not match /api/v1/audit/integrity. Trailing double-asterisk is only treated as a deep glob when both neighbours are slash or end-of-string, so **/api/v1/tasks** does NOT match /api/v1/tasks/<id>/cancel. Same for approvals approve/reject. Four blocking stub-shape bugs in one review.

Restructured setupApiStubs: catch-all registered FIRST so it runs LAST in LIFO, every specific route registered after so they win. Multi-segment routes switched to RegExp matchers — /\/api\/v1\/tasks\/[^/]+\/cancel$/ etc. — because RegExp matchers are unambiguous and avoid the glob single-asterisk gotcha entirely. Audit assertion strengthened from /valid|integrity/i (which matched the always-rendered “Chain integrity” label even when the integrity stub was broken) to /^valid$/i (which only renders after chain.data resolves with status===‘valid’). PlatformHealth.components renamed from bare postgres/redis to the real health.patroni/health.redis keys the COMPONENT_ICON map expects.

First draft of 14-test-auth confidently asserted “backend containers MUST NOT be reachable from any network path that bypasses the edge” and “the edge MUST strip any client-supplied X-Auth-Request-* before forwarding.” Adversarial review (3 lenses, 14 verifier agents, 21 findings) refused to let either claim stand.

The base UFW role in aros-infrastructure allows :8000 from from_ip: 192.168.1.0/24 — the entire LAN, not just the edge VM and the worker host. The aros-backend Dockerfile runs uvicorn --host 0.0.0.0 and CI binds -p 8000:8000 on the host. So any host on the LAN can header-inject and impersonate any identity today, including the seeded platform admin. Separately, ADR-0013 does NOT enumerate header stripping/overwriting as a Consequence — Traefik authResponseHeaders only OVERWRITES X-Auth-Request-* on forward-authed routes (the per-env -health routers in aros-edge.yml bypass forward-auth, so client-supplied headers on /health/ reach the backend untouched — but auth.py exempts /health/ from the auth path so the header is read-and-discarded). The “edge strips” mental model was too strong.

§7 rewritten honestly. Three subsections: §7.1 documents what the edge actually does (overwrite, not strip; only on forward-authed routes); §7.2 admits the UFW :8000 LAN trust is not enforced isolation; §7.3 lists the three hardening tasks (tighten UFW to edge-plus-localhost, add a trusted-proxy or shared-secret guard in auth.py, amend ADR-0013 with a Consequences bullet). The doc tracks these as known follow-ups, not as something the recipes already assume. Also: stopped hardcoding akadmin@example.com in test recipes (the seed actually uses Hi@ryantaylor.uk, the email match is case-sensitive, and leaving the literal in the doc would have functioned as a header-injection cheatsheet); recipes now source via importlib at runtime from migration 0011 PLATFORM_ADMIN_*.

Piece 3 wired pytest -m integration into a CI step against postgres:16-alpine. The first run failed every PG-touching test with psycopg.OperationalError: fe_sendauth: no password supplied. After fixing that, eight tests failed with UniqueViolation: duplicate key value violates unique constraint user_username_key / Key (username)=(akadmin) already exists. After fixing that, one test (test_tenant_selection_isolates_inventory_and_rejects_non_member) failed with AssertionError: assert sel-client-entity not in {link-test-container, link-test-vm, sel-client-entity, sel-platform-entity} — a cross-tenant RLS leak in a force-enabled policy.

Round 1: conftest._stable_env pinned PG_PASSWORD="" for every test as a defensive measure (so accidental DB connections fail loudly in the unit lane); the clobber survived into the integration lane. Round 2: tests hardcoded X-Auth-Request-User=akadmin and X-Auth-Request-Email=akadmin@example.com; migration 0011 seeds akadmin/Hi@ryantaylor.uk; upsert_user_from_oidc searches by email, did not find the wrong email, tried to INSERT a new identity.user with username=‘akadmin’, collided with the seed. Round 3: ALTER ROLE NOSUPERUSER alone — STILL leaked. Round 4: added NOBYPASSRLS — PG refused with permission denied to alter role / The bootstrap user must have the SUPERUSER attribute. PostgreSQL structurally protects POSTGRES_USER from being demoted by itself. The whole strategy of degrading the bootstrap role cannot work.

Round 5: create a SEPARATE non-superuser role aros_backend_test with the same password as the existing PG_PASSWORD, grant it CONNECT plus USAGE plus ALL on every schema the app touches (public, identity, tenant, audit, inventory, tasks, policy, approval, notification), and run alembic plus pytest as that role with PG_USER override. Migrations already finished as the bootstrap superuser; the test role only needs to read alembic_version (no-op at head) and CRUD the data tables. Diagnostic dump in the step shows role attributes plus inventory.entity RLS state so a future regression is loud. Comment in ci.yml explains WHY aros_backend_test exists so nobody re-derives this in three months. Then the verify-and-close pass adversarially audited every tenant-scoped table — t/t clean — and extended the diagnostic dump to cover all six tenant-scoped schemas, not just inventory.

Round 1 of the CI integration iteration above. Worth its own card because it is the kind of cross-lane fixture bug that only surfaces when a new lane runs for the first time.

The autouse _stable_env fixture in conftest.py wrote monkeypatch.setenv("PG_PASSWORD", "") for every test. The intent was defensive: if a unit test accidentally tries to connect to PG, fail loudly with a missing-password error rather than reach the local DB on a development workstation. Reasonable for the unit lane. The integration lane is the first lane that actually needs PG_PASSWORD to be whatever the CI step set, and the fixture ran first and clobbered it before the FastAPI app loaded Settings.

Make the override conditional on the test marker. if not request.node.get_closest_marker("integration"): monkeypatch.setenv("PG_PASSWORD", ""). Integration tests inherit the CI env value. All four other stable-output overrides (ENVIRONMENT, COMMIT_SHA, DEPLOY_TIMESTAMP, DEPLOY_HOST) stay unconditional so integration assertions on those values stay deterministic. Lesson archived in the close-out commit message so the pattern (test fixture for the unit lane silently breaks the new lane) is not re-discovered.