Full-stack audit remediation — batch 1 (4 P1 findings)#10
Merged
Conversation
…checklist The 2026-06-15 fresh-eyes app-wide audit (workflow ws54p55ss, 13 agents: 6 area finders x 4 semantic dimensions + adversarial verify per area) produced 31 confirmed findings (15 P1 / 12 P2 / 4 P3, zero P0). Persist the rendered checklist into the repo so the remediation has a durable, checkable source independent of the ephemeral workflow output. Each finding carries a [ ] checkbox ticked as it is remediated, one focused commit per finding (or tight cluster), per the Task-5 contract. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…g (P1 postgres-first)
WHY — _post_event_mutation's crisis/sabotage degradation block read
buildings.building_condition, did `float(cond) - degradation` in Python, and
wrote the numeric result back. Two defects:
1. DATA CORRUPTION. building_condition is a TEXT enum (good/moderate/poor/
ruined — supabase/migrations/...entities.sql:50; the chain is canonical in
fn_degrade_building, migration 148). Every other consumer treats it as that
string: building_service filters `.eq('building_condition', <str>)`,
codex_export renders it verbatim, forge prompts vary the text values,
instagram defaults to "operational". This one path wrote "0.9" into the
column, which then permanently fails fn_degrade_building's CASE (falls to
ELSE -> no further change), renders as literal "0.9" in the codex/Forge, and
breaks condition filtering.
2. ADR-007 RACE. The read-modify-write had no compare-and-swap, so two
concurrent crisis events both read the same old condition and the second
UPDATE clobbered the first (lost degradation).
FIX — delete the Python read-modify-write and call the existing atomic RPC
fn_degrade_building(p_building_id) per affected building. It walks the enum
chain one step (good->moderate->poor->ruined) under a
`WHERE building_condition = v_old` compare-and-swap and returns
{changed, old_condition, new_condition}, so concurrent events can no longer
lose-update and the column stays the TEXT enum. A crisis now degrades by one
condition step (the fractional `heartbeat_building_crisis_degradation` config —
read only here — is retired; its migration-132 seed row is now inert).
fn_degrade_building is service_role-only since migration 258, so the call runs
on the admin client (get_admin_supabase_client, the canonical pattern from the
258 call-site switches); the surrounding zone/building reads stay on the
RLS-scoped user client. This mirrors operative_mission_service._apply_saboteur_
effect, which already calls fn_degrade_building (verified its resolution path
uses the admin client, so no regression there).
VERIFICATION — ruff check clean; backend/tests/unit/test_event_service.py 6/6
green; module loads. No test asserted the old float behavior.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… postgres-first) WHY — _post_event_mutation attached newly-created events to matching narrative arcs with a Python read-append-write on narrative_arcs.source_event_ids: it read the arc's array snapshot once, appended one id in Python, and wrote the whole array back. Called from 4 user-facing endpoints + the resonance pipeline, two near-simultaneous mutations both read the same snapshot, each appended only its own event id, and the second full-array UPDATE clobbered the first -> the first event silently dropped from the arc (ADR-007 lost-update, no compare-and-swap). Two corrections to the audit while verifying against ground truth: - The column is source_event_ids UUID[] (migration 129:95), NOT a JSON array. The audit's proposed jsonb_agg/jsonb_array_elements SQL would have failed; the correct atomic dedup-append on a Postgres array is array_append + a `NOT (p_event_id = ANY(source_event_ids))` guard. - narrative_arcs grants authenticated/anon only SELECT (migration 129:272); the FOR-ALL write policy is service_role-only (129:254). So the previous user-client UPDATE silently failed under RLS for every non-admin event mutation (swallowed by the `except -> "arc attachment unavailable"` debug log) — the attachment only ever ran on admin-path mutations. FIX — new migration 259 adds fn_arc_attach_event(p_arc_id, p_event_id): a single-statement dedup-append (the `NOT (... = ANY(...))` WHERE clause is the race-free dedup; the Python `not in existing_ids` check is now just a cheap pre-filter). It is SECURITY INVOKER and service_role-only (REVOKE from PUBLIC/anon/authenticated, GRANT to service_role per 257/258 hygiene), invoked via the admin client. Routing through the admin client both serializes the append and un-breaks the RLS-silent-failure, so arc attachment now works for all event mutations. _post_event_mutation fetches the admin client once and reuses it for this and the building-degradation block. VERIFICATION — ruff check clean; migration parses + grants apply + array dedup-logic verified in a rolled-back local txn (is_secdef=f, existing blocked / new allowed); test_event_service.py 6/6 green. CI applies migrations on a fresh DB as the final net. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…tive_atomic (P1 postgres-first)
WHY — OperativeMissionService.deploy spent RP early (EpochService.spend_rp) and
inserted the mission later in two separate statements, with two round-trips of
reads/computation (resonance eligibility, success-probability) in between. If
anything between them raised, the RP was already debited and never refunded —
the player lost RP with no mission (ADR-007: non-atomic mutate of
concurrent-access data). fn_deploy_operative_atomic was built in migration 214
to collapse this into one transaction, but had ZERO callers — dead code while
the race it fixes was live.
The audit's literal proposal ("call the existing RPC") was WRONG: the RPC was
STALE. It predates the resonance_op column (migration 216), never set
deployed_cycle (migration 090), and hardcoded status='deploying' — but the
service sets status='active' for 0-deploy-cycle operatives (spy/guardian, see
OPERATIVE_DEPLOY_CYCLES) and writes resonance_op + deployed_cycle. Wiring it up
as-is would have silently NULLed resonance_op/deployed_cycle and mis-set status.
FIX
- Migration 260: DROP the old 12-arg signature (zero callers) and recreate a
15-arg version that also inserts status/deployed_cycle/resonance_op. Because a
changed signature is a new function identity, the new signature is explicitly
REVOKEd from PUBLIC/anon/authenticated and GRANTed to service_role — the
257/258 anon-revokes only covered the old signature, and a fresh CREATE would
otherwise inherit a PUBLIC EXECUTE grant (the SECDEF-exposure class 257/258
closed).
- deploy() reordered: all reads/computation (resonance eligibility,
_calculate_success_probability, resolve-time) run FIRST; then a single
fn_deploy_operative_atomic call (spend + insert in one txn) on the admin
client (router always passes one; fall back to get_admin_supabase_client()).
insufficient_rp -> 400; then re-fetch the row for the battle-log/response
context. RP can no longer be debited without a mission (and a unique-mission
constraint trip now rolls back the debit too).
VERIFICATION — ruff clean; migration parses + DROP/CREATE/REVOKE/GRANT apply
(SECURITY DEFINER, 15 args) in a rolled-back local txn; 41/41 operative tests
green incl. 2 NEW tests covering the previously-untested success path and the
insufficient_rp path. CI applies migrations on a fresh DB + the SECDEF lint gate
re-checks the anon surface as the final net.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…-cycle schedulers (P1 DRY)
WHY — backend/services/social/scheduler_base.py:BaseSchedulerMixin encodes the
canonical resilient run-loop (the 5-clause try/except: CancelledError re-raise,
transient httpx connectivity warn-and-retry, PostgrestAPIError/HTTPError/Key/
Type/Value -> logged + tagged Sentry capture, and a last-resort `except
Exception` guard with push_scope so a per-tick poison pill re-alerts each
interval instead of silently killing the task while /health stays 200). Six
schedulers already inherit it. But ResonanceScheduler._run_loop and
EpochCycleScheduler._run_sweep_loop hand-rolled the exact same scaffold —
including the literal silent-tick-death comment copy-pasted into both — so every
loop-policy change had to be hand-patched in two more copies, and both copies had
already DRIFTED: their middle except clause used a bare, untagged
`sentry_sdk.capture_exception(exc)` (no push_scope, no service/phase tags), worse
observability than the mixin they should share.
FIX — both classes now inherit BaseSchedulerMixin:
- ResonanceScheduler: _scheduler_name='resonance', _load_config now returns a
dict {enabled, interval} (was a tuple), _process_tick delegates to the existing
_check_and_process. The hand-rolled _run_loop/start/_task are deleted.
- EpochCycleScheduler: _scheduler_name='epoch_cycle', _load_config returns the
fixed {enabled:True, interval:30} (no platform_settings gate), _process_tick
delegates to _sweep_expired_cycles. start() overrides to call super().start()
then seed the eager-timer subsystem (unchanged). _run_sweep_loop is deleted.
Both inherit the silent-tick-death guard + tagged Sentry capture automatically;
~70 lines of duplicated control flow removed. Net behavior is identical except
the loop's unexpected-error path now reports with proper tags.
VERIFICATION — ruff clean (incl. now-unused asyncio/sentry_sdk/get_admin_supabase
imports dropped from resonance_scheduler); test_resonance_scheduler.py +
test_epoch_cycle_scheduler.py 22/22 green (5 _load_config tests updated tuple->
dict); both still registered + .start() called in app.py lifespan; no dangling
refs to the removed loop methods.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… dup) Task-5 audit finding routers+models #2 (P1 duplication). WHY: every AI-generation endpoint (generate_agent/building/portrait_description/ event/relationships/lore_image/image) ended in two near-identical ~17-line except blocks — OpenRouterError -> 503 and Exception -> 500 — each opening its own sentry push_scope, setting the generation_endpoint tag + generation context, and capturing. ~180 lines of copy-pasted observability scaffolding in the HTTP layer; any change to the AI-failure contract had to be edited in 7 places (and had already drifted: lore_image's 500 detail omits "Please try again.", and only relationships carried `except HTTPException: raise`). WHAT: extracted one router-local `@asynccontextmanager _ai_generation_guard( endpoint, *, simulation_id, fail_detail, context)`. Exception->HTTP mapping is legitimately the controller's job, so the guard stays in the router (not a service). Each endpoint body now runs inside `async with _ai_generation_guard(...)`. Behaviour preserved exactly (verified line-by-line against the originals): - OpenRouterError -> 503 "AI service temporarily unavailable." with `from None` (cause suppressed); any other exception -> 500 `fail_detail` with `from e` (cause chained). - lore_image keeps its detail without "Please try again." - HTTPException raised inside the block (e.g. AgentService.get 404 in generate_relationships) propagates unchanged with NO sentry capture — this is now uniform across all endpoints (harmless for the six that never raise one). - sentry scope tagged + generation context set exactly once per failure; context always carries simulation_id plus the per-endpoint keys. IMPACT: generation.py −219 lines net (101 ins / 320 del). One AI-failure contract instead of seven. No public behaviour change (same status codes, details, tags, context, log levels). VERIFICATION: ruff check + format clean; `import backend.routers.generation` OK (guard present, all endpoints resolve). New backend/tests/unit/test_generation_guard.py (6 tests, all green) pins: success pass-through, OpenRouterError->503 + capture + tag/context, RateLimitError subclass->503, generic->500 + fail_detail + capture, HTTPException passthrough with capture_exception NOT called, and default-context (simulation_id only). Path had zero prior router-level coverage. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… (P1 dup) Task-5 audit finding routers+models #1 (P1 duplication). WHY: instagram.py and bluesky.py each carried a ~50-line force_publish endpoint that was the same publish-with-compensation workflow copy-pasted — get_post, status guard (400), build platform client, publish, on-error open a sentry push_scope + reset_post_status + 502, audit log, re-fetch. The credential-loader helpers (_get_instagram_service / _get_bluesky_service) were parallel copies too. This is business logic living in the HTTP layer, duplicated; any change to the publish-failure contract had to be edited in both routers and would drift. WHAT: hoisted the workflow onto BaseSchedulerMixin (both schedulers already inherit it) as `force_publish_post(admin, post_id, *, actor_id) -> dict`, raising typed domain errors defined in scheduler_base.py: - PostNotPublishableError (wrong status) -> router maps to 400 - SocialCredentialsError (not configured) -> router maps to 400 - PublishFailedError (publish raised) -> router maps to 502 Per-platform variation collapses to three hooks on each scheduler: - _content_service (get_post + reset_post_status) - _force_publish_statuses (("draft","scheduled") / ("pending","failed")) - _build_publish_client (absorbs the old router credential loaders, which were force-publish-only — verified no other callers) Each router force_publish is now: admin-log -> one service call -> two except arms -> AuditService.safe_log -> return. Removed the now-unused `sentry_sdk` import and the _get_*_service helpers from both routers. BEHAVIOUR PRESERVED EXACTLY (verified line-by-line): - 400 detail "Cannot publish post with status '<status>'." (PostNotPublishableError message is verbatim); 400 "Instagram/Bluesky credentials not configured."; 502 "Force-publish failed: <exc>[:200]"; reset reason "Force-publish failed: <exc>[:300]"; sentry tag "<platform>_phase"="force_publish" + context key "user_id"; a missing post still surfaces the content service's 404 unchanged. IMPACT: ~90 lines of duplicated orchestration removed from the HTTP layer; one publish-with-compensation contract instead of two. No API behaviour change. VERIFICATION: ruff check + format clean across all 5 files; all modules import (hooks wired: IG=InstagramContentService/("draft","scheduled"), BS=BlueskyContentService/("pending","failed"); force_publish_post inherited). New backend/tests/unit/test_force_publish_workflow.py (6 tests, green): happy path (publish + re-fetch, no reset), wrong-status -> PostNotPublishableError (no publish), missing-creds -> SocialCredentialsError before publish, publish-failure -> reset_post_status + sentry capture + PublishFailedError carrying the message, and concrete IG/BS wiring. Full instagram/bluesky/scheduler/social suite: 197 passed. Path had zero prior router-level coverage. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Task-5 audit finding routers+models #3 (P1 architecture-smell). WHY: CLAUDE.md mandates "Audit logging required for all mutations", yet journal.py (6 state-changing constellation endpoints) and social_media.py (4 POST mutations) had ZERO AuditService references — in the router OR the underlying service (verified: grep AuditService = 0 in both routers + constellation_service + social_media_service). Sibling user-facing routers all audit every mutation (resonances 7, bonds 3, campaigns 4), so these two were a genuine compliance gap. WHAT: added AuditService.safe_log after each successful mutation, router-level, matching the resonances/bonds convention. journal.py (user-global, simulation_id=None): create -> journal_constellations / "create" rename / archive / place_fragment / remove_fragment / crystallize -> same table, constellation_id as entity_id, operation-named actions. social_media.py (simulation-scoped, real simulation_id): sync (entity_id None) / transform / analyze_sentiment / generate_reactions -> social_media_posts, with small useful detail dicts. VERIFY-DON'T-TRUST: the audit proposal named the constellation table `resonance_constellations` — WRONG. The actual table is `journal_constellations` (ConstellationService._TABLE). Used the correct name. social-media table confirmed `social_media_posts`. safe_log is best-effort (swallows PostgrestAPIError/httpx.HTTPError), so it cannot break the endpoint if the audit insert is RLS-constrained. VERIFICATION: ruff check + format clean; both routers import AuditService; 6 + 4 safe_log calls confirmed. 140 journal/social/resonance/insight/attunement tests pass (the added best-effort log introduces no call-count regressions). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ildings routers (P1) Task-5 audit finding cross-cutting-DRY #3 (P1 architecture-smell). WHY: agents.py and buildings.py each carried a verbatim translation-orchestration block on create AND update — fetch SimulationService.get_simulation_context, gate on its existence, and call the fire-and-forget schedule_auto_translation with name/theme — plus the null_de_fields_for_update + update_data.update(de_nulls) two-liner on update. That is orchestration (sim-context resolution, existence gating, knowing schedule_auto_translation's arg shape) living in the HTTP layer, duplicated across 4 sites. CLAUDE.md: "Routers = HTTP only". WHAT: two helpers in translation_service.py (where schedule_auto_translation + null_de_fields_for_update already live): - `async schedule_entity_translation(supabase, table, entity, simulation_id, *, entity_type)` — resolves the sim context once, no-ops when missing, forwards to schedule_auto_translation. The routers now say only "this entity changed, translate it". - `merge_stale_de_nulls(table, update_data) -> dict` — folds the null_de_fields_for_update + in-place merge; returns the nulls so the update path still gates re-translation on `if de_nulls`. Router create-tail and update-body collapse to single-line calls. Dropped the now -unused SimulationService + null_de_fields_for_update/schedule_auto_translation imports from both routers. BEHAVIOUR PRESERVED: create always attempts translation (no-op if sim missing, as before); update re-translates only when an EN source field changed (de_nulls truthy), identical to the old `if de_nulls:` gate; same table/entity_type args. CIRCULAR-IMPORT SAFE: translation_service now imports SimulationService; simulation_service imports only models + utils (no path back). `import backend.services.translation_service` verified clean. VERIFICATION: ruff check + format clean; all three modules import (helpers present; both routers no longer reference SimulationService). No test patched the removed router symbols. 222 agent/building/translation tests pass (1 skipped). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…P2 reuse) Task-5 audit finding backend-services #1 (P2 shared-reuse-gap). WHY: backend/utils/settings.py:decrypt_setting(raw) already encapsulates the gAAAAA-prefix check + Fernet decrypt + empty-on-failure + warning, yet several services hand-rolled the identical block, each with its own late `from backend.utils.encryption import decrypt` import (violating the module-level-import rule) and a redundant `except (ValueError, Exception)`. VERIFY-DON'T-TRUST: the audit said "replace all 6". Inspecting each site's failure semantics showed only 4 are the same operation, and only 2 of those are byte-clean: - instagram_content_service / bluesky_content_service: failure -> "" (the result field is pre-initialised ""), non-encrypted -> raw. EXACTLY decrypt_setting -> `result[...] = decrypt_setting(raw)`. - platform_api_keys / external_service_resolver: cache/return is `str | None` with failure/empty -> None, so used `decrypt_setting(raw) or None` to keep the "credential missing" semantics callers depend on. - settings_service / platform_settings_service: NOT changed. These decrypt to MASK for display, falling back to the ciphertext / "***" on failure, then mask(...). decrypt_setting returns "" on failure, which would change that fallback — a different operation the audit wrongly grouped. Left as-is. WHAT: 4 sites now call decrypt_setting; removed 3 inline encryption late-imports + the redundant except tuples; external_service_resolver swaps its module-level `decrypt` import for `decrypt_setting`. Credential decryption now has one canonical failure contract. VERIFICATION: ruff check --fix + format clean; all 4 modules import cleanly (decrypt_setting reuse is circular-safe — utils.settings imports only utils.responses). 191 settings/resolver/credential/instagram/bluesky tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Task-5 audit finding backend-services #2 (P2 duplication). WHY: InstagramContentService.get_pipeline_settings and BlueskyContentService.get_pipeline_settings were byte-for-byte identical (verified) — same select of setting_key/setting_value/description, same jsonb->string coercion loop (dict/list -> json.dumps, bool -> "true"/"false", other non-null -> str, null -> ""), differing only in the docstring and cls.PIPELINE_SETTINGS_KEYS. The description-carrying variant (used by the admin Platform-settings UI) was not factored anywhere, so it was copy-pasted into both content services. WHAT: added load_settings_with_description(admin, keys) -> dict[str, dict] to backend/utils/settings.py (sibling of load_platform_settings, which only returns raw values). Both get_pipeline_settings methods delegate to it with their own PIPELINE_SETTINGS_KEYS. Removed bluesky_content_service's now-unused `import json` (its sole json use was this loop; instagram keeps json for its other 6 uses). The coercion has no try/except (matching the originals — load_platform_settings wraps in try/except, but get_pipeline_settings deliberately let errors propagate; preserved that). VERIFICATION: ruff check --fix + format clean (no leftover unused imports); all three modules import; load_settings_with_description present + both methods delegate. 165 instagram/bluesky/settings tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Task-5 audit finding routers+models #4 (P2 architecture-smell). WHY: the generate_image endpoint did service-shaped payload work in the HTTP layer — popped description_override, branched on entity_type, and hand-assembled an 11-key building_data dict literal before calling generate_building_image. The router was supposed to be HTTP-only; this leaked the building-image pipeline's exact field set into the controller. WHAT: added ForgeImageService.generate_entity_image(entity_type, entity_id, entity_name, extra) that owns the dispatch (agent/banner/building), the description_override pop, and the building_data assembly. The router endpoint is now a single delegating call + audit + return. BEHAVIOUR PRESERVED: same three branches, same building_data keys/defaults, same description_override handling. The service copies `dict(extra or {})` before popping, so it no longer mutates the request body (the old inline code popped body.extra in place) — output is identical. VERIFICATION: ruff check + format clean; both modules import; generate_entity_image present on the service. 259 forge/generation/image tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
WHY: test_weekday_heavy_history_changes_projection_vs_linear used
datetime.now(UTC) to build its history and compute days_remaining, then asserted
the seasonal projection differs from the linear one. But seasonal == linear
EXACTLY when days_remaining is a multiple of 7: a whole-week tail covers every
weekday once, so its mean DOW multiplier is 1.0 and the seasonal sum collapses to
overall_mean × days_remaining. The test therefore failed on ~1-in-7 calendar
days — e.g. today (2026-06-16): June has 30 days, days_remaining = 30-15-1 = 14,
a multiple of 7, so the weekday-$10/weekend-$0 history projects identically both
ways and `linear != seasonal` is False. The SERVICE is correct; the test's
premise ("all remaining-day-windows of length >=1 produce a delta") was wrong.
WHAT: pin a fixed reference date (2026-01-15 → days_remaining = 16, 16 % 7 = 2,
a partial-week tail) and build the history relative to the same reference, then
pass it via the existing `_build_snapshot(rows, now=ref)` param. Added a
precondition assert (days_remaining % 7 != 0) documenting why the delta is
guaranteed: with dow multipliers in {1.4 weekday, 0 weekend}, no partial week of
1–6 days can average to 1.0. Test is now deterministic on every calendar day.
Pre-existing flaky, unrelated to the Task-5 audit work; fixed on request.
VERIFICATION: ruff clean; backend/tests/test_ops_forecast_service.py 5 passed
(was 4 passed / 1 failed). Deterministic regardless of run date.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…race) Task-5 audit finding SQL/migrations #5 (P2 postgres-first). WHY: AllianceService.create_proposal's solo-team auto-accept counted members then directly `update({"team_id": ...})` on the user client — the exact count-then-join race fn_join_team_checked (migration 214/220/233) was built to close. Two concurrent solo-joins (or a join racing a size-boundary acceptance) could both pass the line-90 size check and over-fill the team. epoch_participation _service:274 already uses the RPC correctly; this path bypassed it. WHAT: replaced the count+raw-update with fn_join_team_checked(p_epoch_id, p_team_id, p_simulation_id, p_max_size), branching on its return: None -> not_found ("Team not found or has been dissolved.") False -> bad_request("Team is full (max N members).") True -> proceed to insert the accepted proposal record (unchanged). The RPC locks epoch_teams FOR UPDATE then size-checks, so the size enforcement + join are one atomic operation. No new migration (RPC already deployed). VERIFY-DON'T-TRUST: confirmed the RPC's actual signature + return contract, and that it is SECDEF / service_role-only (migration 258) — so it must be called on the admin client (get_admin_supabase_client), mirroring the existing correct caller. Authorization is already validated upstream in create_proposal (proposer is an unaligned participant; team exists + is active), so the admin-client RPC is safe (router-validated-then-privileged-RPC, per ADR-006). Surrounding RLS table ops stay on the user client. VERIFICATION: ruff check + format clean; alliance_service imports; updated test_auto_accepts_solo_team to stub the admin RPC (returns True) and assert it is called with "fn_join_team_checked". 42 alliance + epoch-concurrency + hardening tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…und race)
WHY: OperativeMissionService.recall granted the 50% RP refund
(EpochService.grant_rp) BEFORE flipping the mission status, and the status
UPDATE filtered only on id -- no compare-and-swap. Two concurrent recall()
calls (double-click / retry) both read status='active', both pass the
in-('deploying','active') guard, and both grant the refund: the player is
refunded twice. grant_rp is itself atomic, but the eligibility decision was
ungated, so issuing it twice defeated the atomicity (ADR-007 lost-update /
double-spend on the refund path).
WHAT: New migration 261 fn_recall_operative -- CAS-flip + conditional refund
in one transaction:
- UPDATE operative_missions SET status='returning', resolved_at=now()
WHERE id=? AND status IN ('deploying','active') RETURNING * (the CAS gate;
the row lock serialises N concurrent recalls, only one transitions).
- NOT FOUND -> {"error":"already_recalled"} (no refund).
- On the winning transition only: PERFORM fn_grant_rp_single (cap-aware) in
the SAME transaction -> refund issued exactly once, and never lost between
a successful flip and a separate grant statement.
Mirrors fn_deploy_operative_atomic (migration 260), completing the
deploy/recall pair as atomic service_role-only RPCs. SECDEF hygiene:
REVOKE FROM PUBLIC/anon/authenticated, GRANT TO service_role (257/258).
recall() reordered: reads/validation first (ownership, epoch-status,
mission-status pre-flight 400s for the common case), compute refund +
rp_cap off the already-loaded epoch (EpochService.get returns config -> no
extra round-trip), then ONE RPC on the admin client. {"error":"already_recalled"}
-> 409 conflict, no refund. The router threads its injected admin_supabase
into recall (mirroring deploy).
DESIGN: chose the full RPC over the contained CAS-gate option -- true
atomicity (ADR-007), deploy/recall symmetry, and it closes the partial-failure
window the CAS-gate leaves open (flip succeeds, process dies before the
separate grant -> refund silently lost).
VERIFY-DON'T-TRUST: confirmed the refund-before-flip ordering + id-only UPDATE
filter (audit correct here), fn_grant_rp_single's real (epoch,sim,amount,rp_cap)
signature + LEAST cap, that EpochService.get selects '*' (config present), and
that the router already injects admin_supabase. Migration validated against the
real local schema with body-check ON (CREATE type-checks the body vs
operative_missions + fn_grant_rp_single), rolled back.
VERIFICATION: ruff clean; +1 new conflict test (the double-refund proof: the
CAS loser gets 409 and zero refund) and rewired test_recall_active_mission onto
the admin RPC; 5 recall/operative tests green. NOTE: migration 261 needs
prod-apply WITH the deploy (recall calls the RPC).
[Task 5 audit remediation -- SQL/migrations #14, P2]
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…W race) WHY: DossierEvolutionService.evolve_section re-read evolution_log, appended a log entry in Python, and wrote the whole array back with `evolution_count + 1` -- where evolution_count was captured at the top-of-function section read (a stale snapshot). It ALSO rebuilt body/body_de by concatenating onto section["body"], the SAME stale snapshot. So three writes (body, evolution_log, evolution_count) all derived from stale reads: two concurrent evolutions of one lore section would lose an addendum, drop a log entry, and under-count (ADR-007 read-modify-write). WHAT: New migration 262 fn_record_dossier_evolution does the entire mutation server-side on the LIVE row in one statement -- no read first, no stale snapshot: body = body || separator || addendum body_de = coalesce(body_de,'') || de_separator || addendum_de evolution_count = coalesce(evolution_count,0) + 1 evolution_log = coalesce(evolution_log,'[]'::jsonb) || log_entry evolved_at = now() jsonb `array || object` appends the object as one element (verified). Python still owns the addendum text and the body_de separator/fallback (translation success vs English fallback) and passes the pieces in; SQL owns the atomic concatenation. Service-role-only SECDEF (admin client, heartbeat pipeline); REVOKE FROM PUBLIC/anon/authenticated, GRANT TO service_role (257/258 hygiene). Dropped the now-unused `import json` (its only use was the deleted str-decode). VERIFY-DON'T-TRUST: confirmed evolution_log IS jsonb (audit correct this time, unlike narrative_arcs) and evolution_count is the stale snapshot. Found the audit UNDER-stated it -- body/body_de share the same stale base, so the RPC closes all three races, not just counter+log. Migration validated against the real local schema (body type-checks vs simulation_lore), rolled back. VERIFICATION: ruff clean; filled a zero-coverage gap with test_dossier_evolution_service.py (2 tests: RPC-wiring with correct params + body_de English fallback on translation failure); both green. NOTE: migration 262 needs prod-apply WITH the deploy (evolve_section calls the RPC). [Task 5 audit remediation -- SQL/migrations DossierEvolution, P3] Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ts (P2)
WHY: heartbeat.py + news_scanner.py annotated their endpoints as bare
`-> SuccessResponse:` / `-> PaginatedResponse:` while backend/models/heartbeat.py
and backend/models/news_scanner.py defined matching *Response models that were
never used (dead). CLAUDE.md: "Return type annotation is the single source of
truth ... Response models live in backend/models/<domain>.py."
RISK handled: a return annotation makes FastAPI FILTER the response through the
model -- dropping any FE-read field the model lacks, and 500-ing on any Literal
value / required-field-null it can't accept. So each endpoint was VERIFIED, and
the audit's "just wire them" would have shipped TWO latent 500s:
- entry_type Literal was MISSING 'resonance_mood' (present in the live CHECK
constraint) -> typing list_heartbeat_entries as-is would 500 the user-facing
chronicle feed. Added it to HeartbeatEntryType.
- get_heartbeat_overview's no-heartbeat early return {"last_tick": 0} omitted
the required simulation_id -> 500. Fixed the service to return a complete
overview ({simulation_id, last_tick: 0}; other fields default).
VERIFICATION per endpoint (verify-don't-trust): all Literals checked vs live
CHECK constraints (severity / arc_type / arc-status / bureau response_type+status
/ anchor-status all matched; only entry_type drifted); all list SELECTs confirmed
select("*"); all required model fields confirmed NOT NULL in the live schema; the
two admin dashboards confirmed built key-for-key to their models; all mutation
services confirmed to return full rows (response.data[0]). Frontend consumption
mapped to ensure no read-field is dropped.
TYPED (18 endpoints): heartbeat overview, entries (+public), arcs, bureau
responses (list/create/cancel), attunements (list/set/remove), anchors
(list/create/join/leave), admin dashboard, cascade-rules; news dashboard +
candidates. Candidates keeps its FE-locked {items, meta, recommended_threshold}
shape via a NEW ScanCandidateListResponse envelope -- NOT switched to the
standard paginated() {data, meta}, which would break the admin UI's
.items/.recommended_threshold reads (the audit's proposal there was unsafe).
LEFT UNTYPED + documented: get_daily_briefing (no model), force_tick
(process-summary), news list_adapters (team models adapters as list[dict]),
toggle_adapter/reject_candidate (status dicts), trigger_scan (cycle metrics),
approve_candidate (returns a resonance), update_candidate (dead), scan_log
(no model). paginated()-generic suggestion skipped (static-only; the per-endpoint
annotation already carries the element type for FastAPI).
VERIFICATION: ruff clean; app.openapi() builds (471 paths); 528
route/heartbeat/scanner tests green; representative payloads (incl. resonance_mood
entry, fixed early-return, candidates envelope, arbitrary adapter dicts) validate
through the typed wrappers. No SQL, no migration.
[Task 5 audit remediation -- routers+models #8, P2]
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…(P3)
WHY: instagram_image_service's five compose_story_* methods forward to
StoryComposer, and SocialStoryService._compose_* wrap those again — flagged as
a pure pass-through middle layer.
VERIFY-DON'T-TRUST: the audit's proposed fix ("have SocialStoryService hold a
StoryComposer directly because it constructs InstagramImageService only to reach
_story") is FALSE. SocialStoryService.compose_story_image constructs
InstagramImageService(admin) and uses the SAME instance for upload_to_staging on
the composed bytes (social_story_service.py:760) — the image service is genuinely
needed, not just a StoryComposer wrapper. Inlining the forwarders away would
force a parallel StoryComposer handle alongside the image service per story.
WHAT: the forwarders are a deliberate façade keeping the whole story-image
pipeline on ONE image entry-point (StoryComposer = pure Pillow template
rendering; InstagramImageService = image I/O: staging upload + compositing).
Added an explicit "intentional façade, NOT dead indirection — do not inline
these away" comment at the forwarder site (the class docstring already noted the
composition pattern). This is exactly the finding's stated goal: "flagging only
so the pass-through isn't mistaken for meaningful behavior." Comment-only, no
behaviour change.
[Task 5 audit remediation -- backend-services #3, P3]
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…e RPC (P1)
WHY: resolve_cycle granted RP (fn_batch_grant_rp), reset the cycle_ready/
has_acted_this_cycle flags, and advanced mission timers BEFORE the compare-and-
swap that guarded the cycle increment. The migration-167 fn_advance_epoch_cycle
only locked the increment+phase transition, NOT the grant. So two concurrent
resolve_cycle calls both granted rp_per_cycle to every participant (and both
advanced mission timers) before either reached the CAS; the loser got a 409, but
the RP was already double-credited and never rolled back (ADR-007). The
use_atomic_cycle_advance flag (default false) merely chose between two variants
of the SAME pre-CAS-grant bug.
VERIFY-DON'T-TRUST: the finding claimed "the atomic path is safe because
grant+increment+phase are one transaction" — FALSE. The grant was OUTSIDE the
RPC in BOTH branches (line 455, before the line-481 RPC), so even with the flag
ON, concurrent resolves double-granted.
WHAT: migration 263 CREATE OR REPLACE fn_advance_epoch_cycle to move all three
once-per-cycle mutations INSIDE the RPC, AFTER the SELECT FOR UPDATE + CAS:
RP grant (+50% foundation bonus, cap-enforced) + cycle_ready/has_acted reset +
mission-timer advance + cycle increment + phase transition all commit in one
transaction. A loser of the CAS returns concurrent_resolution having mutated
nothing — no partial double-grant. RP amount/cap/cycle_hours read from config
(COALESCE defaults mirror EpochConfig 12/40/8); foundation bonus (rp*3)/2 ==
Python int(rp*1.5); grant+timers reuse fn_batch_grant_rp / fn_advance_mission_
timers via PERFORM (both SECDEF, same owner). CREATE OR REPLACE preserves the
service_role-only ACL (257/258); re-asserted explicitly. The
use_atomic_cycle_advance platform_settings row is deleted.
resolve_cycle collapses to one unconditional RPC call + post-transition side
effects; the legacy branch (~70 lines), the flag read, and 3 now-unused imports
(PlatformConfigService, extract_one, server_error) are deleted.
BONUS — defuses the CI-hang root cause: the 54-min selector hang was
_grant_rp_batch reaching the process-global admin cache from threaded
resolve_cycle. The grant now runs server-side inside the RPC, so resolve_cycle
no longer touches that cache; the race test's monkeypatch is now harmless
insurance. _grant_rp_batch itself stays (epoch_lifecycle_service uses it for the
foundation grant).
VERIFICATION: functionally tested against the REAL local schema (rolled back) on
a live competition epoch: RPC returned {new_cycle:5, rp_granted:10}, DB showed
cycle 4->5, RP 20->30, ready/acted->false; a stale retry (expected=4, actual=5)
returned {error_code:concurrent_resolution} having granted nothing (the
double-grant proof). No test changes needed (unit tests hit the status guard /
mock resolve_cycle; integration tests assert observable behavior, preserved).
ruff clean; 90 epoch/cycle/scheduler/router unit tests green (thread-timeout,
no hang). NOTE: migration 263 needs prod-apply WITH the deploy.
[Task 5 audit remediation -- SQL/migrations resolve_cycle, P1]
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.
First batch of the 2026-06-15 full-stack architecture audit remediation (Task 5).
4 of 31 findings, all P1, each verified against ground truth before fixing.
Backend-only — zero file overlap with the parallel
feat/graphical-dungeonfrontend work, so the two branches merge into
mainindependently without conflict.Findings remediated
event_servicebuilding_condition data corruption (P1 postgres-first) — thecrisis-degradation block did
float(building_condition) - degradationand wrote anumeric value back into a TEXT enum column (good/moderate/poor/ruined), which
permanently broke
fn_degrade_building's CASE, condition filtering, and codex/Forgerendering — plus a lost-update race. Now calls the existing atomic
fn_degrade_buildingRPC (one enum step, compare-and-swap) on the admin client. Prod verified clean (no
corrupted rows existed). No migration.
event_servicenarrative-arc attachment race (P1 postgres-first) — read-append-writeon
narrative_arcs.source_event_idslost concurrent attachments. New migration 259fn_arc_attach_eventdoes an atomic dedup-append. (Audit proposed jsonb SQL; the columnis actually
UUID[]→ usedarray_append+NOT (… = ANY(…)). Also un-broke asilent RLS failure: authenticated has SELECT-only on narrative_arcs, so the old
user-client UPDATE never ran for non-admins — now routed via the admin client.)
OperativeMissionService.deployRP-loss race (P1 postgres-first) — RP was spent thenthe mission inserted in two statements with reads between; any failure debited RP with no
mission. Now one atomic
fn_deploy_operative_atomiccall. (Audit said "use the existingRPC as-is", but it was stale — predated the
resonance_opcolumn, never setdeployed_cycle, hardcodedstatus='deploying'. Migration 260 drops the old 12-argsignature and recreates a 15-arg version inserting all three, re-locked to service_role.)
Added 2 deploy tests — the success path had zero prior coverage.
Scheduler run-loop duplication (P1 DRY) —
resonance_schedulerandepoch_cycle_schedulerhand-rolled the same resilient run-loop asBaseSchedulerMixin,with weaker (untagged) Sentry capture. Both now inherit the mixin; ~70 lines of
duplicated control flow removed; both gain the tagged silent-tick-death guard.
Migrations (prod-apply needed)
259+260— both are called by the new code, so they must be applied with/beforethe code deploy. Both are SECURITY-correct (service_role-only, REVOKE from PUBLIC/anon/
authenticated per the 257/258 hygiene). Validated locally (parse + grants + array logic in
a rolled-back txn); CI re-applies on a fresh DB.
Verification
ruffclean; targeted suites green (test_event_service6/6,test_operative_service41/41 incl. 2 new,
test_resonance_scheduler+test_epoch_cycle_scheduler22/22). Fullsuite run pending before the remaining batches.
Scope
Remaining 27 findings (11 P1, 12 P2, 4 P3) tracked with checkboxes in
docs/analysis/full-stack-audit-findings-2026-06-15.md. The riskiest P1 (resolve_cyclelegacy path) is deliberately sequenced for dedicated focus.
🤖 Generated with Claude Code