Issue 1 (refactor): run() and migrate_to_version() duplicated the inner
loop body — every change to the migration loop had to be applied in both
places. Pull the loop into _apply_migrations_until(engine, target_version):
when target_version is None, apply all pending; otherwise stop when the
file's version exceeds target. run() and migrate_to_version() are now
thin wrappers around the helper.
Issue 2 (tests): migrate_to_version() had no direct tests for its
edge-case behavior. Add 4 tests:
- idempotent: calling migrate_to_version(engine, N) twice == once
- lower-than-current: migrate_to_version(engine, 3) after N=13 is a no-op
- zero on fresh DB: migrate_to_version(engine, 0) is a no-op
- beyond-max: migrate_to_version(engine, 999) applies every migration
Tests use a fresh sa.Engine (not db.engine()) so they exercise the
target-version behavior from a clean user_version=0 starting point —
the conftest autouse fixture has already brought the module engine to
the latest version, which would mask the no-op branches.
Address the three concerns raised by the spec reviewer on commit 534130e:
1. The 0014 data-preservation test was degraded: it only verified
post-0014 INSERTs round-trip, never proving that 0014's INSERT INTO
claims_new SELECT * FROM claims actually carries pre-existing rows
through the table recreation. Add migrate_to_version() helper to
db_migrate.py and rewrite the test to seed at v=13, apply 0014, and
assert the rows survive. Add a complementary test exercising the
JOIN-FK tables (matches / cas_adjustments / service_line_payments /
line_reconciliations) whose batch_id columns get populated by
0014's JOIN against the parent tables.
2. The four SQLAlchemy ORM before_insert events on Match /
CasAdjustment / ServiceLinePayment / LineReconciliation are
undocumented in the codebase. Add a clear module-level docstring
explaining what the events do, why they exist (composite-FK
NOT NULL columns require the batch side which most call sites
don't have readily available), when they fail (parent id not
findable in session.new / identity_map / DB -> misleading
'NOT NULL constraint failed' IntegrityError), and the workarounds.
3. The plan Task 1.3 Step 8 said 'do not modify production code to
make old tests pass'. In practice production code DID need to
change because composite-PK semantics make single-id lookups
ambiguous. Amend the plan to acknowledge this and cite the
s.get(Claim, X) -> s.query(Claim).filter().first() refactor as
the example. Also document the before_insert events in Step 5.
Brings in docs/superpowers/plans/2026-06-21-cyclone-parse-decide-workflow.md
from main (this branch was created before the plan file was committed)
with Task 1.3 Step 5 and Step 8 amended per the reviewer feedback.
Migration 0014 changes the PRIMARY KEYs of claims and remittances from
single-column id to composite (batch_id, id). Enables the spec'd
cross-batch CLM01 / CLP01 collision workflow: same CLM01 in multiple
batches is now representable (resubmits), pre-flight dedup 409 path is
genuinely exercisable, force-insert can skip pre-existing duplicates.
Strategy: PRAGMA defer_foreign_keys = ON + table recreation for every
table whose FKs pointed at the old single-column PK. Tables recreated:
remittances, claims, matches, cas_adjustments, service_line_payments,
line_reconciliations. Each child table gains a batch_id (or
remittance_batch_id) column for the composite FK side; INSERT INTO new
SELECT FROM old JOINs populate it from the already-recreated parent.
Cross-table FKs (remittances.claim_id, claims.matched_remittance_id)
cannot be SQL-enforced with composite PKs (SQLite has no ALTER
CONSTRAINT). Dropped at SQL level; enforced via application-layer
invariants in store.manual_match / manual_unmatch / reconcile.run and
the dedup.preflight_* helpers.
ORM updates (db.py):
- Claim / Remittance: composite PK via explicit PrimaryKeyConstraint in
__table_args__ (column order matches SQL: batch_id, id).
- New Claim.matched_remittance_batch_id column.
- Match / CasAdjustment / ServiceLinePayment / LineReconciliation:
added the batch side of their composite FK to the parent table.
- SQLAlchemy before_insert events auto-populate the batch side of the
composite FK from session.new, then identity_map, then SQL fallback.
Production code updates:
- store.manual_match / manual_unmatch: also write
matched_remittance_batch_id on the claim (was missing).
- api.py manual-match endpoint: same fix.
- reconcile.run: same fix for auto-matched pairs.
Test updates: replaced s.get(Claim, X) with the composite key
(batch_id, id) where batch_id is known, or s.query().filter().first()
where the test only knows the id. Tests that previously inserted a Match
row pointing at a non-existent Remittance now seed the parent Remittance
so the new NOT NULL composite FK is satisfied.
Grok
Tyler