fix: restore green CI (stale world-map tests, xmldom advisory, service_role grant)#9
Merged
Conversation
…ctations Two pre-existing failing tests in world-map-styles.test.ts asserted behaviour the source had deliberately, intentionally changed (and documented) — the tests were stale, not the code: - zoneCategoryColor: source maps industrial -> epochInfluence (purple) and liminal -> textMuted, NOT industrial -> warning. The comment in ZONE_CATEGORY_STYLE explains why: warning resolves to the same amber as primary in the default theme, so authority and industrial zones would be visually identical. Updated the two expectations to match. - computeBounds: source fits the bbox to geometry (zones/streets/buildings) and intentionally EXCLUDES city centres when geometry is present, to avoid a far-flung empty-city anchor dragging the fitted box across dead canvas (the out-of-bounds label is dropped via pointInBounds). The sibling test "skips cities whose centre is missing" already encodes geometry-only fitting, so the failing test was the inconsistent outlier. Reworked it into a complete spec: geometry-only bounds when geometry exists, plus a fallback assertion that the city centre anchors the bounds when there is no geometry at all. 39/39 world-map-styles tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`npm audit --audit-level=high` (CI Frontend Dependency Audit gate) failed on @xmldom/xmldom <=0.8.12 (4 HIGH advisories: recursion DoS + XML injection via unvalidated DocumentType/PI/comment serialization). It is a transitive dep of the build-time @lit/localize-tools@^0.8.2 (not in the runtime bundle, so real risk is low, but the gate is strict). `npm audit fix` applies the non-breaking patch 0.8.12 -> 0.8.13, which satisfies the ^0.8.2 range — no override, no @lit/localize-tools change. `npm audit --audit-level=high` now reports 0 vulnerabilities. package-lock.json only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…h prod) CI Test Backend failed with 111 "permission denied for table ..." errors (code 42501) for service_role across 30 tables — every integration test that sets up data via the service_role client. Root cause: the CI uses supabase/setup-cli@latest, and newer `supabase start` no longer grants anon/authenticated/service_role the table + sequence access that the project's prod and local databases were provisioned with (verified: local pg_default_acl grants all three; service_role has SELECT on public tables locally). So migration-created tables on CI lack the grants. Add a psql step after migrations that re-grants table + sequence access to match prod (RLS still gates anon/authenticated; service_role bypasses RLS as designed). Routines are deliberately NOT granted — per-function EXECUTE is governed by the migrations (ADR-006) and the SECDEF anon-RPC guard, so this does not weaken the SECURITY DEFINER lockdown. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…clock, seed-skip) The remaining Test Backend reds after the service_role grant fix — all pre-existing test-rot, unrelated to any source behaviour: - test_routes.py (4 tests): `[route.path for route in app.routes]` raised `AttributeError: '_IncludedRouter' object has no attribute 'path'`. Newer FastAPI/Starlette puts include_router wrappers without a `.path` into app.routes. Guard the comprehension with `hasattr(route, "path")` (the API routes we assert on all still carry `.path`). - test_orphan_sweeper_scheduler.py::test_not_due_is_a_noop: a stale-clock time bomb. `_NOW` is a fixed 2026-04-21 constant, but `_process_tick` reads the real `datetime.now()`, so once wall-time drifted past `_NOW` + the 7-day interval the "last run 1 day ago" config read as overdue — the sweep ran (not a noop) and hit `_report_result`'s `result.error_count > 0` with the test's bare-AsyncMock sweep result (AsyncMock > int). Pin the scheduler clock to `_NOW` for this test (MagicMock(wraps=datetime), now → _NOW). The source line is correct. - test_zone_adjacencies.py::test_backfill_velgarien_geometry: the `_zones_present` skip-guard only checks the three zone IDs exist, but backfill derives adjacencies from zone *polygons* under SIM_VELGARIEN. CI seeds the bare zone rows (for the insert-based tests) without geojson, so the guard passed and the backfill then found 0 zones → assertion failure. Added `_zones_have_geometry` (zones under SIM_VELGARIEN carrying geojson) and switched this test to it, so it skips where geometry is unseeded (CI) and runs where present (local — passes). Verified locally: test_routes 4/4, orphan TestProcessTick + full file, zone backfill (runs locally) + full file — 40+ pass, ruff clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…introspection The hasattr(route, "path") guard from the previous commit stopped the AttributeError but dropped the actual API routes: CI's newer FastAPI wraps included routers in `_IncludedRouter` objects (no `.path`; the real routes nest inside), so filtering them out left "0 API routes" and the prefix/count assertions failed. (Local FastAPI 0.136.3 has no such wrapper, which is why this only reproduced in CI, where `pip install` pulls a newer FastAPI within the `>=0.136.0` range.) Switch all four TestRouteRegistration checks to `app.openapi()["paths"]` — the public, version-stable schema. It yields the full path set regardless of how FastAPI represents routes internally. Verified locally: 467 /api/ paths, all EXPECTED_PREFIXES + EXPECTED_SIMULATION_SCOPED entities + chat endpoints present; 9/9 test_routes pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mleihs
added a commit
that referenced
this pull request
Jun 15, 2026
…migration 258 fallout) PR #8's Test Backend job hung 54+ min (normal ~5 min) and never failed: there is no per-test or job timeout, so one wedged test runs until GitHub's 6h default. Root-caused to a deterministic cross-event-loop deadlock, reproduced locally with pytest-timeout (thread method) — the worker thread's loop blocks forever in asyncio base_events `_run_once -> selector.select(None)`. Why now: migration 258 (ADR-006 part 2) switched CycleResolutionService's RP-grant chokepoints (`_grant_rp_batch`, `spend_rp`, `grant_rp_single`) to obtain their RPC client from the process-global `get_admin_supabase_client()` cache instead of the caller-supplied client. `test_race_conditions` deliberately drives `resolve_cycle` from two threads, each with its own `asyncio.run` loop, to exercise the DB optimistic lock under real concurrency. The first thread populates the global cache with a client bound to its loop; the second reuses that cached client on a different loop, so its httpx socket is registered on a now-dead loop and the live loop's selector waits forever — exactly the "NOT thread-safe / event-loop affinity" hazard documented in `supabase_admin_cache.py`. (PR #8's earlier run failed fast at 5.6 min rather than hanging because the table-grant step was absent; tests short-circuited on 42501 before reaching this path. With #9's grant step merged in, the test now runs to the hang.) Test-only fix — production is unaffected: uvicorn runs one event loop for the process lifetime, and a repo-wide grep confirms zero `run_in_executor` / `ThreadPoolExecutor` / `threading.Thread` / `asyncio.run(` in non-test backend code, so the global cache is only touched from a single loop in prod. The cache's docstring already mandates that a thread with its own loop "must construct its own client inside the thread's loop." The fix honours that: monkeypatch `cycle_resolution_service.get_admin_supabase_client` to a per-call factory that builds a fresh service-role client on whatever loop is running, so each worker thread gets a loop-local client. Real concurrency and the optimistic-lock assertion are preserved; mirrors the existing `route_secdef_admin` conftest seam. Verification: - test_concurrent_resolve_does_not_double_increment: 0.28s (was 54+ min hang) - Full backend suite: 3430 passed, 11 skipped, 0 failed (55.6s), thread-timeout armed - ruff check: clean Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the three pre-existing CI failures on
main(red since the #7 merge run). All unrelated to the SECDEF security work in #8 — split out so that PR stays pure.1. Test Backend (111 failures) —
service_role"permission denied for table …"Every integration test that seeds via the service_role client failed (42501, 30 tables). Root cause: CI uses
supabase/setup-cli@latest, and newersupabase startno longer grantsanon/authenticated/service_rolethe table+sequence access the project's prod/local DBs were provisioned with (verified: localpg_default_aclgrants all three). Added a psql grant step after migrations to match prod — RLS still gates anon/authenticated; service_role bypasses RLS as designed. Routines are deliberately not granted (per-function EXECUTE stays governed by migrations + the ADR-006 SECDEF guard).2. Test Frontend (2 failures) — stale
world-map-stylestestsBoth assertions tested behaviour the source had deliberately changed and documented; the tests were stale, not the code:
zoneCategoryColor:industrial → epochInfluence(purple),liminal → textMuted—warningcollides withprimary's amber (documented inZONE_CATEGORY_STYLE).computeBounds: fits to geometry and excludes city centres when geometry is present (avoids dead canvas; the sibling "skips cities whose centre is missing" test already encodes geometry-only fitting). Reworked into a complete spec incl. the no-geometry fallback.3. Frontend Dependency Audit —
@xmldom/xmldomHIGH advisorynpm audit fixapplies the non-breaking patch0.8.12 → 0.8.13(transitive dep of the build-time@lit/localize-tools).npm audit --audit-level=highnow reports 0 vulnerabilities.Verification
npx vitest run tests/world-map-styles.test.ts→ 39/39 pass.npm audit --audit-level=high→ 0 vulnerabilities.Note on merge order vs #8
Both this PR and #8 add a step to the
test-backendjob after migrations. Merging this first turnsmaingreen; #8 then rebases cleanly (its SECDEF-guard step and this grant step are distinct steps / distinct objects — tables vs functions).🤖 Generated with Claude Code