PR 3 — Core data model

[batbrainy]

Pull Request

Linked issue

Closes #13 (PR 3 — Core data model; IMPLEMENTATION_PLAN.md §13 "PR 3"). Parent story: #6. Related tracks: #7 (Story 3 — Enrich Push Events), #10 (Extension B — Idempotency and Restart Safety).

Problem

The app booted, reported health, and logged JSON — but had zero business tables. db/schema.rb was at version 0 and db/migrate/ did not exist. Plan §8 makes PostgreSQL the sole source of business truth (an event is accepted only once its push_events row commits), so every capability from PR 4 onward needs this schema first.

Issue #13 also carries a delivery gate: CI must run the real suite from this PR onward. It previously ran only test -f file-existence checks — no Ruby, no PostgreSQL, no rspec — and bin/ci ran no tests either.

Scope

Included:

• Seven migrations, one per table, dependency-ordered — github_actors, github_repositories, event_sources, github_api_budget, ingestion_runs, push_events, quarantined_events (§7)
• event_sources scheduling components as four separate columns (cadence_due_at, poll_floor_until, retry_not_before_at, next_poll_at) — §9 forbids collapsing them, or --force could not tell which constraint it may bypass
• github_api_budget as a schema-enforced singleton (id integer PRIMARY KEY DEFAULT 1 CHECK (id = 1)) with window fields and per-class counters
• push_events with every structured column NOT NULL, SHAs as varchar(64), raw payload as jsonb, and FKs onto the entity tables' unique github_id
• quarantined_events with payload_fingerprint as the sole unique identity and github_event_id indexed but not unique
• Partial index on both entity tables matching the reconciler's exact predicate
• Three idempotent write paths as tested primitives: PushEvent.insert_if_new, upsert_stub! on both entities, QuarantinedEvent.record!, plus Enrichable.touch_activity!
• 109 new specs (118 total); CI runs the real suite; one ADR

Deliberately not included (later PRs per §13):

Deferred	Owner
RequestExecutor, SourceLock, RequestGate, UrlPolicy, transports, fixture corpus, BudgetLedger reservation + allowance formula + bootstrap, budget seed row, counter arithmetic invariants, webmock/faraday	PR 4
Tolerant PushEvent parsing, the SHA-256 fingerprint algorithm, quarantine taxonomy routing, ingest-transaction orchestration (gating touch_activity! on insert_if_new), ingestion_runs.status vocabulary, one-shot command	PR 5
Pagination, 304 debit, effective_poll_time, global-vs-class blocking, event-source seeding, event_sources.status vocabulary	PR 6
Enrichment execution, state transitions, skipped_budget reactivation, fairness shares, refresh TTLs	PR 7
solid_queue, worker, jobs, reconciler	PR 8
Event inspection API, /status	PR 10
GIN index on raw_payload	when a query demonstrates the need (§7)
Remaining ADRs, design brief, full data-model reference	PR 12

No Gemfile change. §2A pins the test stack to RSpec + WebMock + hand-authored fixtures; factory_bot and shoulda-matchers are not in it, so test data is built by explicit helpers in spec/support/. WebMock is pinned but there is no HTTP in PR 3.

Technical decisions

• Monotonic activity timestamps. touch_activity! and record! use GREATEST/LEAST rather than plain assignment. Delayed events carry an older created_at (documented 30s–6h latency) and sources commit independently, so assignment could move last_seen_at or last_received_at backwards and distort newest-first enrichment ordering. For quarantined_events.last_received_at this is a deliberate hardening of §7's snippet: it implements the column's stated meaning ("most recent observation") more faithfully than the literal EXCLUDED assignment.
• insert_if_new validates explicitly. insert bypasses Active Record validations, so without an explicit validate! the 40-or-64-hex SHA rule would never run on the real write path — a decorative validation. No DB regex CHECK: §7's enumerated constraint list has none, and a StatementInvalid mid-batch is exactly what quarantine exists to avoid. PR 5's parser remains the routing point.
• Merge rules omit rather than overwrite. upsert_stub!'s SET list leaves out raw_payload, name/description/language, and every enrichment_* column, so an envelope replay cannot clear enrichment or reactivate a skipped_budget entity (§7 rules 1 and 4). COALESCE stops a sparse envelope from blanking a known value. Verified by spec: an entity with skipped_at, enrichment_attempts, next_retry_at, and last_error set survives a changed-login replay with all four intact.
• Non-negativity CHECKs only. The write paths (insert, upsert, update_all) bypass validations, so counter floors are enforced in the schema. Reservation arithmetic (used <= allowance, reserve headroom) is deliberately not constrained — that is PR 4's to define, and encoding it now would turn a benign transient into a batch-aborting error.
• enrichment_status is text + CHECK, not a native enum. A CHECK round-trips cleanly through schema.rb (which db:test:prepare reloads) and widening it later is a constraint swap rather than ALTER TYPE. text rather than varchar also keeps PostgreSQL from injecting a ::text cast into the partial-index predicate.
• No status vocabulary for event_sources / ingestion_runs. Neither is defined anywhere in the plan, and the owning PR (6 and 5) owns the state machine. Both are text NOT NULL with no default and no CHECK. Note the trap avoided: an obvious-looking rate_limited source status would re-introduce per-source rate-limit state, which V2 explicitly moved to the global ledger (§7).
• Constraint specs use insert!, not insert. insert_all hard-codes on_duplicate: :skip, so a uniqueness test written with insert would silently never raise. Every expected violation names its specific error class — ActiveRecord::StatementInvalid is the superclass of all of them, so a broad assertion could pass on the wrong failure.
• Violations run inside a savepoint. Under use_transactional_fixtures a failed statement aborts the example's transaction and every later query raises PG::InFailedSqlTransaction. spec/support/constraint_helpers.rb wraps each violation in transaction(requires_new: true).
• Partial-index specs assert semantics, not planner behaviour. PostgreSQL normalises IN (...) to = ANY (ARRAY[...]), so a predicate string comparison is brittle; and Relation#explain returns an ExplainProxy, while on a seed-sized table the planner prefers a sequential scan anyway. The specs check index shape and predicate membership (driven off the Enrichable constants) plus scope behaviour across all five statuses.
• Rejected: factory_bot/shoulda-matchers (outside §2A's pinned stack); a native PostgreSQL enum type; a GIN index on raw_payload (§7 gates it on a demonstrated query); DB-level SHA and timestamp-ordering CHECKs; deferring touch_activity! wholesale to PR 5 — its effect is testable in isolation now, and only its gate depends on PR 5.

Testing performed

• docker compose run --rm test — 130 examples, 0 failures (121 new; the 9 PR 2 health/formatter examples still pass)
• Coverage maps to §12's "Persistence tests": unique event ID with DO NOTHING RETURNING id; raw JSON retention asserted as semantic equivalence, with one spec asserting the byte difference explicitly so the tradeoff is visible in the suite; required fields/types/NOT NULL; stub merge rules including the skipped_budget non-reactivation case; fingerprint as sole unique key (same event ID + two payloads coexist) and occurrence counting; pending-status queries against the partial indexes
• Also covered: the singleton at the DB level, negative-counter rejection on every counter, monotonicity in both directions, 40- and 64-char SHA acceptance with 39/41/non-hex rejection, and spec/db/schema_spec.rb guarding the ledger's exact column set and the absence of any *_class_blocked_until column
• bundle exec rubocop: 51 files, no offenses. bin/brakeman: 0 security warnings
• GitHub Actions: both jobs green. The RSpec suite job resolved Ruby 3.4.10 from .ruby-version, prepared both test databases, and ran 130 examples / 0 failures. A libpq-dev install step was added defensively, then removed once the first run proved the runner already ships it.

Docker verification

• Clean apply: dropped the development databases and ran db:prepare — all seven migrations applied in order, both FKs bound to github_id
• schema.rb round-trip confirmed — the dump preserves id: :integer, default: 1 and t.check_constraint "id = 1". This was the gate to watch: db:test:prepare loads from schema.rb, so an un-dumpable construct would vanish in test. The budget singleton spec passing under docker compose run --rm test proves it survived
• Singleton proven directly in psql: INSERT ... VALUES (2) → violates check constraint "github_api_budget_singleton"; id = 1 accepted
• Partial index in psql: ... WHERE (enrichment_status = ANY (ARRAY['pending'::text, 'retryable_failure'::text])) — no cast on the column, because the type is text
• Rollback safety: db:rollback:primary STEP=7 unwinds all seven in reverse FK order to a clean database, then db:migrate re-applies all seven. (Note for reviewers: this app is multi-database, so bare db:rollback aborts and directs you to the namespaced task.)

Documentation updates

docs/adr/0001-jsonb-semantic-retention.md — the one architectural decision this PR makes, recording that jsonb discards whitespace, key order, and duplicate keys; that array order is preserved; and that a byte-exact text column is deliberately not built. README: status, reviewer-commands note, planned-contents table, and a pointer to docs/adr/.

db/queue_schema.rb is deliberately unchanged — db:prepare re-dumps it and clobbers PR 2's explanatory comment, which is unrelated noise in a data-model PR.

Review round 1

Three findings, all fixed in 44df326:

• quarantined_events.raw_payload was NOT NULL. The justification conflated two separate rows of §7's taxonomy: a wholly unparseable response body is an ingestion failure, but a valid events array can contain null, {}, or [] as an element — each a parsed value with no usable envelope that §7 says to quarantine. NOT NULL turned exactly those into a NotNullViolation aborting the ingest transaction, destroying the malformed event quarantine exists to preserve. Now nullable, and off the presence validation too (presence treats {}/[] as blank). payload_fingerprint stays NOT NULL — the canonical fingerprint of null is as well defined as any other.
• upsert_stub! reached PostgreSQL unguarded — the same defect already fixed in insert_if_new, missed here. A nil github_id/login/full_name aborted the transaction before the caller could quarantine one envelope. Both stubs now validate! first, backed by real validations (github_id in the Enrichable concern, login on the actor, full_name on the repository). No DB round trip is added, since there is no uniqueness validation.
• updated_at regressed on a late replay. GREATEST protected last_received_at but not updated_at, and both entity IDENTITY_MERGEs had the same latent bug. All three merges are now monotonic, matching touch_activity!, which already did this — the merges were the inconsistent ones.

Review round 2

One finding, fixed in c651c3f: the identity fields could still move backward. Making updated_at monotonic in round 1 without applying the same rule to the identity assignments left the two inconsistent — writing new-login at t + 300 then old-login at t stored old-login while updated_at stayed at t + 300, so the timestamp claimed the newer observation while the row held the older one. Reachable because sources commit independently and events arrive late.

Both IDENTITY_MERGEs now use one expression per assignment, which collapses the freshness guard and the existing sparse-envelope guard into a single mechanism rather than adding a second:

COALESCE(CASE WHEN EXCLUDED.updated_at >= <table>.updated_at
              THEN EXCLUDED.col END,
         <table>.col)

A stale envelope makes the CASE yield NULL so COALESCE keeps the stored value; a fresh envelope omitting an optional field also yields NULL, preserving the original behaviour. >= lets a same-instant write win rather than being discarded. QuarantinedEvent::OCCURRENCE_MERGE needs no equivalent — it overwrites no identity fields.

Regression tests on both entities use values that actually differ and assert updated_at, plus a same-instant boundary case. Verified non-vacuous: with the guard removed the actor spec fails with got: "old-login"; with it restored it passes.

Known limitations

• No behaviour reads or writes these tables yet. The primitives are exercised only by specs; PR 5 wires them into the ingest transaction.
• touch_activity! is ungated by design — nothing yet enforces that it is called only for a newly inserted event. That gate, and the spec proving it, belong to PR 5.
• Counter relationships are unconstrained: the schema permits poll_used > poll_allowance. PR 4's ledger owns reservation arithmetic.
• event_sources.status and ingestion_runs.status accept any text until PRs 6 and 5 define their vocabularies.
• The queue database remains schema-empty until PR 8.

[batbrainy]

A parsed payload is not necessarily non-null. For example, a valid events array could contain a null element, which should fall into the invalid-envelope quarantine path. Rails serializes that Ruby value as SQL NULL, so this constraint makes record!(raw_payload: nil) raise ActiveRecord::NotNullViolation instead of preserving the malformed event. The model presence validation also rejects empty objects and arrays, which are valid parsed values for an invalid envelope. Could we allow these values here and add coverage for null and empty envelopes?

[batbrainy]

This upsert reaches PostgreSQL without validating the required stub fields. A nil login currently raises ActiveRecord::NotNullViolation, which aborts the current transaction before the caller can route the malformed envelope to quarantine. Could we validate or guard the required fields before issuing the upsert, similar to PushEvent.insert_if_new? The repository stub has the same issue with github_id and full_name.

[batbrainy]

The GREATEST above keeps last_received_at monotonic, but this assignment still moves updated_at backward for the same late-replay case. Recording t + 300 and then t leaves last_received_at at t + 300 while setting updated_at back to t, even though occurrence_count was just updated. Should this use GREATEST(quarantined_events.updated_at, EXCLUDED.updated_at) as well?

[batbrainy]

The timestamp now stays monotonic, but the identity fields can still move backward. I tried writing new-login at t + 300 followed by old-login at t; the row ended up with old-login while updated_at remained t + 300. The repository merge behaves the same way. That leaves the timestamp claiming the newer observation while storing identity from the older one, which can happen when source transactions commit out of order. Could we guard these assignments using EXCLUDED.updated_at >= the existing updated_at, apply the same rule to the optional identity fields and repository merge, and add a regression test where the values actually differ?