Settle the Ash codegen vs hand-written migration convention #3269

New issue

Open

opened 2026-05-12 04:19:42 +00:00 by mfreeman451 · 3 comments

mfreeman451 commented

2026-05-12 04:19:42 +00:00

Owner

Background

The add-platform-security-hardening change in branch
feature/add-platform-security-hardening shipped the Phoenix-layer
work (cluster-aware rate limiter, RateLimit / SecurityHeaders /
UploadGuard plugs, CSP report endpoint, hardened session cookies,
RBAC permission keys) and deferred four sections that depend on
Ash resources:

Section 5 — ServiceRadar.Security.WebhookSecret resource +
WebhookSignature plug
Section 6 — ServiceRadar.Security.SecurityEvent resource +
Events recorder GenServer
Section 7 — ServiceRadar.Security.AuthLockout resource +
Lockouts helper + LockoutCheck plug
Section 9 — Settings → Audit operator LiveViews
(Events / Lockouts / Webhook Secrets)

The plug code and audit LiveViews were already written on the
branch before being reverted; the proposal under
openspec/changes/add-platform-security-hardening/ documents the
intended shape.

Why this is its own issue

Bringing the resources back is blocked on a project-wide question:
how do we actually generate Ash migrations today?

elixir/serviceradar_core/CLAUDE.md says:

NEVER use mix ecto.migrate / mix ecto.gen.migration. Use
mix ash.codegen <name> instead.

But:

elixir/serviceradar_core/priv/resource_snapshots/ is gitignored
(added by the January 2026 "tenant cleanup" PR — commit
db15a4d81). Without committed snapshots, every developer's
first mix ash.codegen run produces a create-everything
migration that collides with existing schema on demo / prod /
any already-migrated environment.
Every existing migration in priv/repo/migrations/ (including
the consolidating 20260117090000_rebuild_schema.exs from the
same January cleanup) is hand-written use Ecto.Migration. The
codebase's actual practice does not match the documented one.

The PR author ran into this directly: codegen against the local
docker-compose CNPG generated a 2 878-line "create every table"
migration. Hand-writing the migration would match the codebase's
real pattern but contradict CLAUDE.md and would not update the
(gitignored) snapshots that codegen relies on.

Acceptance criteria

Pick one of these (or document a third):

Option A — re-track snapshots. Drop
elixir/serviceradar_core/priv/resource_snapshots/ from
.gitignore, commit current snapshots from a fully-migrated
demo DB, and update CLAUDE.md to spell out the workflow.
Future schema work lands via mix ash.codegen <name>.
Option B — adopt hand-written migrations officially.
Update CLAUDE.md to match what the codebase actually does
(hand-written use Ecto.Migration with idempotency markers
CREATE IF NOT EXISTS / drop_if_exists), remove the "never
use ecto.migrate" rule, and document the convention.

Once the convention is settled, restore the deferred sections.
The plug + LiveView code from this branch can be reused; only the
resources, the recorder GenServer, and the migration generation
mechanism need to follow the chosen path.

References

Branch: feature/add-platform-security-hardening
Proposal: openspec/changes/add-platform-security-hardening/
Operator runbook (defers called out under §7): docs/PLATFORM_SECURITY_HARDENING.md
January cleanup that removed snapshots: db15a4d81 tenant cleanup (#2320)

## Background The `add-platform-security-hardening` change in branch `feature/add-platform-security-hardening` shipped the Phoenix-layer work (cluster-aware rate limiter, RateLimit / SecurityHeaders / UploadGuard plugs, CSP report endpoint, hardened session cookies, RBAC permission keys) and **deferred** four sections that depend on Ash resources: - **Section 5** — `ServiceRadar.Security.WebhookSecret` resource + `WebhookSignature` plug - **Section 6** — `ServiceRadar.Security.SecurityEvent` resource + `Events` recorder GenServer - **Section 7** — `ServiceRadar.Security.AuthLockout` resource + `Lockouts` helper + `LockoutCheck` plug - **Section 9** — Settings → Audit operator LiveViews (Events / Lockouts / Webhook Secrets) The plug code and audit LiveViews were already written on the branch before being reverted; the proposal under `openspec/changes/add-platform-security-hardening/` documents the intended shape. ## Why this is its own issue Bringing the resources back is blocked on a project-wide question: how do we actually generate Ash migrations today? `elixir/serviceradar_core/CLAUDE.md` says: > NEVER use `mix ecto.migrate` / `mix ecto.gen.migration`. Use > `mix ash.codegen <name>` instead. But: 1. `elixir/serviceradar_core/priv/resource_snapshots/` is gitignored (added by the January 2026 "tenant cleanup" PR — commit `db15a4d81`). Without committed snapshots, every developer's first `mix ash.codegen` run produces a *create-everything* migration that collides with existing schema on demo / prod / any already-migrated environment. 2. Every existing migration in `priv/repo/migrations/` (including the consolidating `20260117090000_rebuild_schema.exs` from the same January cleanup) is hand-written `use Ecto.Migration`. The codebase's actual practice does not match the documented one. The PR author ran into this directly: codegen against the local docker-compose CNPG generated a 2 878-line "create every table" migration. Hand-writing the migration would match the codebase's real pattern but contradict CLAUDE.md and would not update the (gitignored) snapshots that codegen relies on. ## Acceptance criteria Pick one of these (or document a third): - [ ] **Option A — re-track snapshots.** Drop `elixir/serviceradar_core/priv/resource_snapshots/` from `.gitignore`, commit current snapshots from a fully-migrated demo DB, and update CLAUDE.md to spell out the workflow. Future schema work lands via `mix ash.codegen <name>`. - [ ] **Option B — adopt hand-written migrations officially.** Update CLAUDE.md to match what the codebase actually does (hand-written `use Ecto.Migration` with idempotency markers `CREATE IF NOT EXISTS` / `drop_if_exists`), remove the "never use ecto.migrate" rule, and document the convention. Once the convention is settled, restore the deferred sections. The plug + LiveView code from this branch can be reused; only the resources, the recorder GenServer, and the migration generation mechanism need to follow the chosen path. ## References - Branch: `feature/add-platform-security-hardening` - Proposal: `openspec/changes/add-platform-security-hardening/` - Operator runbook (defers called out under §7): `docs/PLATFORM_SECURITY_HARDENING.md` - January cleanup that removed snapshots: `db15a4d81 tenant cleanup (#2320)`

mfreeman451 commented

2026-05-12 05:02:15 +00:00

Author

Owner

Update: The WebhookSecret resource + WebhookSignature plug
sections that were originally listed here as "deferred" have been
dropped from scope entirely — serviceradar has no inbound HTTP
webhook surface (Falco alerts and datasource pushes flow over NATS,
not HTTP). They were leftover from porting the inkit security
survey wholesale; this codebase doesn't have the underlying need.

The remaining concern this issue tracks is the codegen workflow
itself, which is real and independent: priv/resource_snapshots/
was gitignored in the January 2026 "tenant cleanup" PR (db15a4d81),
which makes mix ash.codegen produce a "create everything"
migration on first run for every fresh checkout. Every existing
migration in priv/repo/migrations/ is hand-written
use Ecto.Migration — including the migration this branch landed
for SecurityEvent + AuthLockout. Pick one of the two options
in the original description (re-track snapshots OR adopt the
hand-written convention officially) and update CLAUDE.md to match.

**Update:** The `WebhookSecret` resource + `WebhookSignature` plug sections that were originally listed here as "deferred" have been **dropped from scope entirely** — serviceradar has no inbound HTTP webhook surface (Falco alerts and datasource pushes flow over NATS, not HTTP). They were leftover from porting the inkit security survey wholesale; this codebase doesn't have the underlying need. The remaining concern this issue tracks is the codegen workflow itself, which is real and independent: `priv/resource_snapshots/` was gitignored in the January 2026 "tenant cleanup" PR (`db15a4d81`), which makes `mix ash.codegen` produce a "create everything" migration on first run for every fresh checkout. Every existing migration in `priv/repo/migrations/` is hand-written `use Ecto.Migration` — including the migration this branch landed for `SecurityEvent` + `AuthLockout`. Pick one of the two options in the original description (re-track snapshots OR adopt the hand-written convention officially) and update CLAUDE.md to match.

mfreeman451 commented

2026-05-12 05:02:20 +00:00

Author

Owner

mfreeman451 commented

2026-05-12 05:02:41 +00:00

Author

Owner

mfreeman451 changed title from ~~Restore deferred security resources (Sections 5/6/7/9) — settle the Ash codegen vs hand-written migration convention~~ to Settle the Ash codegen vs hand-written migration convention

2026-05-12 05:02:50 +00:00

mfreeman451 referenced this issue from a commit

2026-05-12 05:03:17 +00:00

Drop webhook scope; land hand-written migration for the rest