From 3f672d5db31fb985d76b3863059d10759401b9d3 Mon Sep 17 00:00:00 2001 From: Tyler Date: Sun, 21 Jun 2026 16:54:07 -0600 Subject: [PATCH] spec: drop claims UNIQUE constraint + 409 UX --- ...ims-unique-constraint-and-409-ux-design.md | 343 ++++++++++++++++++ 1 file changed, 343 insertions(+) create mode 100644 docs/superpowers/specs/2026-06-21-cyclone-claims-unique-constraint-and-409-ux-design.md diff --git a/docs/superpowers/specs/2026-06-21-cyclone-claims-unique-constraint-and-409-ux-design.md b/docs/superpowers/specs/2026-06-21-cyclone-claims-unique-constraint-and-409-ux-design.md new file mode 100644 index 0000000..c7811a5 --- /dev/null +++ b/docs/superpowers/specs/2026-06-21-cyclone-claims-unique-constraint-and-409-ux-design.md @@ -0,0 +1,343 @@ +# Drop `UNIQUE(batch_id, patient_control_number)` on Claims + Robust 409 UX + +**Date:** 2026-06-21 +**Branch:** `main` +**Status:** Draft (brainstorming approved, awaiting writing-plans) +**Scope:** Backend schema + minimal frontend error handling for collision 409. + +--- + +## 1. Why this exists + +Today, every multi-claim 837P file in `docs/prodfiles/837p-from-axiscare/` and +`docs/prodfiles/FromHPE/` returns **HTTP 409** on upload. The cause: + +* The `claims` table has `UNIQUE(batch_id, patient_control_number)` declared + inline in `CREATE TABLE` (auto-index `sqlite_autoindex_claims_2`). +* `store._claim_837_row` populates `patient_control_number` from + `claim.subscriber.member_id` (the subscriber's insurance member id), + not from the CLM01 segment value. +* A real 837P submission routinely contains many `CLM*` segments per + subscriber (a member seeing multiple providers on the same day). All + those rows share the same `member_id` → same `patient_control_number`. + Within one batch, the constraint fires on claim #2. + +Migration `0003_drop_claims_remits_unique_constraints.sql` already exists +with the right intent but is **buggy**: it does +`DROP INDEX IF EXISTS uq_claims_batch_pcn`, but the constraint is inline, +not a named index — so the `DROP INDEX` is a no-op and the constraint +survives. + +The 409 error message ("duplicate CLM01 control numbers") is also +misleading because the column stores `member_id`, not CLM01. + +This SP fixes both: drops the constraint properly via SQLite table +recreation, and gives the upload page a structured error panel for the +collision case so the operator can find the existing batch in one click. + +--- + +## 2. Operator surface + +| Surface | Change | +|---|---| +| DB | New migration `0013_drop_claims_unique_constraint.sql`; idempotent. | +| Python | `store.find_existing_batch_for_claim(claim_id)` new helper. | +| API | `POST /api/parse-837` and `/api/parse-835` 409 responses gain `existing_batch_id`. | +| UI | `/upload` page renders an inline error panel for 409 with a link to the existing batch. | +| Tests | New migration test, store helper test, API test, component test. | + +No CLI / settings changes. + +--- + +## 3. Schema migration + +`backend/src/cyclone/migrations/0013_drop_claims_unique_constraint.sql`: + +```sql +-- version: 13 +-- Drop the inline UNIQUE(batch_id, patient_control_number) on claims. +-- Migration 0003 attempted DROP INDEX IF EXISTS uq_claims_batch_pcn but +-- the constraint is inline in CREATE TABLE, so the drop was a no-op. +-- The only way to remove an inline UNIQUE in SQLite is table recreation. +-- +-- X12 837P allows any number of CLM segments per 2000B subscriber loop; +-- claim identity is provided by the primary key (claims.id = CLM01). +-- The remittances table had a parallel constraint already removed in 0003 +-- (because that one WAS a named index), so this migration only touches +-- claims. + +PRAGMA foreign_keys=OFF; +BEGIN; + +CREATE TABLE claims_new ( + id TEXT PRIMARY KEY, + batch_id TEXT NOT NULL REFERENCES batches(id) ON DELETE CASCADE, + patient_control_number TEXT NOT NULL, + service_date_from DATE, + service_date_to DATE, + charge_amount NUMERIC(12, 2) NOT NULL DEFAULT 0, + provider_npi TEXT, + payer_id TEXT, + state TEXT NOT NULL DEFAULT 'submitted', + state_before_reversal TEXT, + matched_remittance_id TEXT REFERENCES remittances(id), + raw_json TEXT, + rejection_reason TEXT, + rejected_at TIMESTAMP, + resubmit_count INTEGER NOT NULL DEFAULT 0, + state_changed_at TIMESTAMP, + payer_rejected_at TEXT, + payer_rejected_reason TEXT, + payer_rejected_status_code TEXT, + payer_rejected_by_277ca_id TEXT, + payer_rejected_acknowledged_at TEXT, + payer_rejected_acknowledged_actor TEXT + -- NO UNIQUE (batch_id, patient_control_number) — removed. +); + +INSERT INTO claims_new SELECT * FROM claims; +DROP TABLE claims; +ALTER TABLE claims_new RENAME TO claims; + +-- Recreate secondary indexes (same names, same columns as initial schema +-- plus later migrations). +CREATE INDEX ix_claims_state ON claims(state); +CREATE INDEX ix_claims_patient_control_number ON claims(patient_control_number); +CREATE INDEX ix_claims_service_date_from ON claims(service_date_from); +CREATE INDEX ix_claims_state_changed_at ON claims(state, state_changed_at); +CREATE INDEX idx_claims_payer_rejected_at ON claims(payer_rejected_at); +CREATE INDEX idx_claims_payer_rejected_unack + ON claims(payer_rejected_at) + WHERE payer_rejected_acknowledged_at IS NULL; + +COMMIT; +PRAGMA foreign_keys=ON; +``` + +**Note on the `db.py` ORM model:** `backend/src/cyclone/db.py` declares +the `Claim` ORM model with `UNIQUE(batch_id, patient_control_number)` in +`__table_args__`. After this migration the DB no longer enforces that +constraint, so the ORM declaration becomes aspirational. We leave it in +place as documentation (and as a guard if the DB ever gets rebuilt from +ORM on a fresh schema). No code reads through it; the dedup happens via +PK on `claims.id` in `store.add`. + +--- + +## 4. Store helper + +New function in `backend/src/cyclone/store.py`: + +```python +def find_existing_batch_for_claim(claim_id: str) -> str | None: + """Return the batch_id of the first batch containing this CLM01, or None.""" + from cyclone import db + from cyclone.db import Claim + with db.SessionLocal()() as s: + row = s.execute( + select(Claim.batch_id).where(Claim.id == claim_id).limit(1) + ).first() + return row[0] if row else None + + +def find_existing_batch_for_remit(payer_claim_control_number: str) -> str | None: + """Return the batch_id of the first batch containing this CLP01, or None.""" + from cyclone import db + from cyclone.db import Remittance + with db.SessionLocal()() as s: + row = s.execute( + select(Remittance.batch_id) + .where(Remittance.id == payer_claim_control_number) + .limit(1) + ).first() + return row[0] if row else None +``` + +Both pure reads; no transaction management needed. + +--- + +## 5. API change + +`backend/src/cyclone/api.py`: + +### 837 path (line 394-415) + +```python +except IntegrityError as exc: + existing_batch_id = store.find_existing_batch_for_claim(...) + body = { + "error": "Duplicate claim", + "detail": "...", + "batch_id": rec.id, + } + if existing_batch_id: + body["existing_batch_id"] = existing_batch_id + log.warning("Duplicate claim while persisting batch %s: %s", rec.id, exc) + return JSONResponse(status_code=409, content=body) +``` + +The `...` argument is the first `claim.claim_id` from `result.claims` +where the collision occurred. We pick the first one because the DB raises +on the second insert; iterating through the parser's claim list to find +which CLM01 already existed is cheap. + +### 835 path (line 588-602) + +Same pattern with `find_existing_batch_for_remit` and the existing remit's +`payer_claim_control_number`. + +--- + +## 6. Frontend change + +### `src/lib/api.ts` + +`ApiError` carries an optional `existingBatchId`: + +```typescript +export class ApiError extends Error { + constructor( + public status: number, + message: string, + public existingBatchId: string | null = null, + ) { + super(message); + } +} +``` + +`readErrorBody()` parses JSON and, when status ≥ 400, attempts to extract +`existing_batch_id` and pass it through. `parse837`/`parse835` throw +`new ApiError(res.status, detail, existingBatchId)`. + +### `src/pages/Upload.tsx` + +New local state: + +```typescript +type UploadError = { + kind: "duplicate"; + existingBatchId: string | null; + filename: string; +}; +const [uploadError, setUploadError] = useState(null); +``` + +On mutation error: + +```typescript +} catch (err) { + if (err instanceof ApiError && err.status === 409) { + setUploadError({ + kind: "duplicate", + existingBatchId: err.existingBatchId, + filename: file.name, + }); + toast.error("Duplicate claim — file not ingested"); + } else { + toast.error(err instanceof Error ? err.message : "Failed to parse file"); + } +} +``` + +Inline error panel JSX (renders above streaming results when +`uploadError` is set): + +```tsx +{uploadError ? ( +
+ 409 +
Duplicate claim — file not ingested
+
+ {filename} collides with an existing record. + {existingBatchId + ? " Open the existing batch to compare, or pick a different file." + : " Pick a different file."} +
+
+ {existingBatchId ? ( + + ) : null} + +
+
+) : null} +``` + +Styling matches the existing paper-toned surface (slate/parchment) per +the hybrid dark/paper treatment used elsewhere in the app. + +--- + +## 7. Files changed + +* `backend/src/cyclone/migrations/0013_drop_claims_unique_constraint.sql` — new +* `backend/src/cyclone/store.py` — `find_existing_batch_for_claim`, + `find_existing_batch_for_remit` +* `backend/src/cyclone/api.py` — both 409 handlers add + `existing_batch_id` lookup +* `src/lib/api.ts` — `ApiError.existingBatchId` +* `src/pages/Upload.tsx` — error state + inline panel +* `src/pages/Upload.test.tsx` — new test file +* `backend/tests/test_api_parse_persists.py` — new test cases +* `backend/tests/test_db_migrate.py` — new migration test +* `backend/tests/test_store.py` — new helper tests + +No new dependencies. No config changes. + +--- + +## 8. Out of scope + +* A "Replace existing batch" destructive action — requires + `DELETE /api/batches/{id}` and reconciliation cascade handling; deferred. +* General 4xx error UI for other statuses (empty file, parse error, + validation errors). Those already surface in toasts and the streaming + view; a future SP can generalize the inline panel pattern. +* Renaming `patient_control_number` to `subscriber_member_id` to match + what it actually stores. Out of scope for this fix; tracked separately + if it becomes a source of confusion. + +--- + +## 9. Risk + +* **Migration irreversibility**: SQLite has no `DROP CONSTRAINT`; the + recreation is destructive to schema (but not to data — `INSERT INTO + claims_new SELECT * FROM claims` preserves every row). If the + migration fails mid-way, the transaction rolls back. The + `PRAGMA foreign_keys=OFF` block is required because SQLite otherwise + can't drop and rename a table that other tables reference. +* **Loss of uniqueness**: after the migration, two claims in one batch + *can* share a `patient_control_number`. This is the intended behavior. + Claim identity is still unique via `claims.id` (CLM01, PK) and the + existing dedup check in `store.add` (`s.get(Claim, claim.claim_id)`). +* **`existing_batch_id` race**: the helper runs after the failed + `store.add` transaction has rolled back. Between rollback and helper + call, another writer could delete the colliding claim. Result: 409 + fires with no `existing_batch_id`; UI shows panel without link. + Acceptable — the panel still explains the collision and offers + "Pick a different file." + +--- + +## 10. Test plan + +| Test | Asserts | +|---|---| +| `test_db_migrate.py::test_drop_claims_unique_constraint` | After running migrations from v12, `user_version=13`, no `*claims*unique*` index exists, two rows with same `(batch_id, patient_control_number)` insert cleanly. | +| `test_db_migrate.py::test_migration_idempotent` | Running migrations on a v13 DB is a no-op. | +| `test_store.py::test_find_existing_batch_for_claim` | Helper returns None for unknown, returns batch_id for known, deterministic on duplicates. | +| `test_api_parse_persists.py::test_409_response_includes_existing_batch_id` | Upload duplicate; assert body has `existing_batch_id` pointing to the right batch. | +| `test_api_parse_persists.py::test_multi_claim_batch_with_duplicate_member_id_succeeds` | Upload a file with duplicate `member_id` claims; assert 200, all claims persisted. | +| `Upload.test.tsx::test_error_panel_renders_on_409_with_link` | Mock `useParse` to throw `ApiError(409, ..., existingBatchId)`; assert panel visible, link present. | +| `Upload.test.tsx::test_error_panel_renders_on_409_without_link` | Mock 409 with `existingBatchId: null`; assert panel visible, no link. | +| `Upload.test.tsx::test_no_panel_on_non_409` | Mock 400; assert panel absent, only toast. | +| `Upload.test.tsx::test_pick_different_clears_error` | Click button; assert `pickFile(null)` called and `errorState` cleared. | \ No newline at end of file