diff --git a/docs/superpowers/specs/2026-06-29-cyclone-remittances-architecture-refactor-design.md b/docs/superpowers/specs/2026-06-29-cyclone-remittances-architecture-refactor-design.md new file mode 100644 index 0000000..2697118 --- /dev/null +++ b/docs/superpowers/specs/2026-06-29-cyclone-remittances-architecture-refactor-design.md @@ -0,0 +1,391 @@ +# Sub-project 27 — Remittances Architecture Refactor: Design Spec + + **Date:** 2026-06-29 + **Status:** Draft, awaiting user sign-off + **Branch:** `sp27-remittances-architecture-refactor` + **Aesthetic direction:** No new UI (chain view rendered into existing ClaimDrawer; no new page) + + ## 1. Scope + +Cyclone today ships the full claim/ack/remit cycle end-to-end: outbound +837 submitted via SFTP, inbound 999 / 277CA / 835 / TA1 polled and parsed, +remits matched against claims. Live data is now flowing on the production +system (~60K remits, ~542K 999 acks since 06/19). The architecture serving +that cycle has accumulated rough edges that an afternoon of operational +pressure surfaced: + +- **Scheduler is fragile.** A 06/25 silent hang (no error logged) took + the MFT poll down for ~3 days; recovered only because the operator + restarted the backend. Root cause: `sftp.listdir_attr()` runs without + a socket timeout, so when the MFT TCP-acks but stops responding the + thread blocks indefinitely. The scheduler's status surface is too + shallow to detect this in real time. +- **Filename classification is brittle.** `edi/filenames.py:INBOUND_RE` + requires a `_file_type.x12` suffix on inbound files. Gainwell's + actual 835s ship without that suffix (see the 5×835 batch from + 6/15–6/19), so the only thing the scheduler could do was mark them + `skipped`. Operators had to ingest those by hand. +- **Helpers duplicate API logic.** `cyclone.scheduler._ack_count_summary` + and `_ack_synthetic_source_batch_id` copy logic from + `cyclone.api` with a comment saying so. Two places to keep in sync. +- **Reconciliation is decoupled from ingest.** 835 ingest writes a + `Remittance` with `adjustment_amount=0`, and a separate + `reconcile.run()` pass later sums `CasAdjustment.amount` rows and + overwrites `adjustment_amount`. The UI sees stale values until the + pass runs, and the two phases can drift if reconcile is skipped or + crashes mid-way. +- **No unified claim-chain view.** A claim's 837 submission, 999 ack, + 277CA status, and 835 remit are reconstructed by joining across four + pages and three queries. The "did this remit's claim ever get + matched?" question requires manual SQL. + +### In scope + +Tier 1 (scheduler/ingest split, SFTP hardening, filename classifier): + +1. Move the four per-file-type handlers (`999`, `835`, `277CA`, `TA1`) + out of `backend/src/cyclone/scheduler.py` into + `backend/src/cyclone/handlers/.py`. Each handler exposes one + `handle(text, source_file) -> (parser_used, claim_count)` function. +2. Pull `_ack_count_summary`, `_ack_synthetic_source_batch_id`, and + `_277ca_synthetic_source_batch_id` out of `scheduler.py` and out of + `api.py` into a new `backend/src/cyclone/handlers/_ack_id.py`. Both + callers import from one module. +3. Loosen `edi/filenames.py:parse_inbound_filename` to accept filenames + lacking `_file_type.x12`; fall back to `orig_tx` as the + provisional `file_type`, and accept the file as either `835` (if + `orig_tx` ends in `835`) or `999`/`277CA` (if `orig_tx` does). +4. Wrap `clearhouse.SftpClient._list_inbound_paramiko` / + `_list_inbound_names_paramiko` / `_download_inbound_paramiko` / + `_read_file_paramiko` calls in `asyncio.wait_for(... timeout=N)` + via `asyncio.to_thread`, with `N` configurable via + `CYCLONE_SFTP_OP_TIMEOUT_SECONDS` (default 30s). +5. Surface SFTP errors in `Scheduler.status()` — `last_error_at`, + `last_error`, `consecutive_failures`, `last_sftp_attempt_at`. + When `consecutive_failures >= 3`, the operator pill flips to a + destructive state until a successful tick clears it. +6. Stop swallowing `IOError` from `sftp.listdir_attr()` — return the + error in `result.errors` and let the scheduler record `last_error`. + +Tier 2 (reconciliation atomicity, chain view, invariant guards): + +7. Unify 835 ingest + `reconcile.run()` into a single critical + section inside `cyclone.handlers.handle_835`. The whole flow + (parse → validate → persist batch + remits + CasAdjustments → match + claims → write back `adjustment_amount` and `matched_remittance_id`) + happens in one `db.SessionLocal()()`. The `Remittance.adjustment_amount` + is computed in the same transaction as the `CasAdjustment` rows. +8. Add `GET /api/claims/{id}/chain` returning the claim's full chain + in one response: `submission` (837 → `Claim` row), + `ack_999` (matching `Ack` rows), `ack_277ca` (matching + `Two77caAck` rows), `remittance` (`Remittance` row including + adjustments and matched claim FK). Empty slots when a piece is + missing — never 404 the whole endpoint just because one piece is + absent. +9. The 999 ack handler and 277CA ack handler — when they reject a + claim, and the claim is already matched to a remit — emit an + `ActivityEvent` `claim.rejected_after_remit` so the operator + surfaces the conflict in the Activity page rather than silently + breaking the manual-match workflow. +10. Verify the denormalized pair `Claim.matched_remittance_id` ↔ + `Remittance.claim_id` is written in the same transaction on every + write-path (`store.manual_match`, `store.manual_unmatch`, the new + atomic 835 handler). Add a startup invariant check: if any Claim + has `matched_remittance_id` but the matching Remittance's + `claim_id` is unset (or vice versa), log a structured error so an + operator sees the drift in the logs. + +### Out of scope (deferred to future increments) + +- **Frontend scheduler operator UI** (start/stop/tick/status page, + reconnect button, live log tail). The `/api/admin/scheduler/*` + JSON endpoints remain sufficient; the operator uses curl. +- **Multi-tenant SFTP blocks** (per-payer or per-tenant). Single + dzinesco block remains. +- **Docker-secret `_FILE` conventions** beyond what SP26 already added. +- **Outbound file pickup verification** (polling HPE's MFT to + confirm the operator-dropped 837 was received). +- **Frontend chain drawer component.** The `GET /api/claims/{id}/chain` + endpoint is exposed; rendering it inside the existing + `ClaimDrawer` is a separate UI increment. + +## 2. Decisions (locked during brainstorming) + +1. **Branch base is `Version-1.0.0`, not `main`.** `main` is at + `74aa64f` (SP26 only) and is missing the SP25+26 SFTP work, the + `[sftp]` docker extra, and the permission-matrix fixes. The + production system runs `Version-1.0.0`, so the refactor lands + there. A subsequent forward-merge from `Version-1.0.0` → `main` + can pull SP27 in if the operator wants it on main. + +2. **One module per handler, not a class.** Each handler exposes one + pure function `handle(text: str, source_file: str) -> HandleResult`, + where `HandleResult` is a small dataclass with `parser_used`, + `claim_count`, `batch_id?`, and per-handler extras (e.g. 835 + carries `matched_count`). No class hierarchy, no plugin registry — + the scheduler keeps its `HANDLERS` dict as the registry. + +3. **The `_FILE` env-var convention from SP26 stays.** This spec + doesn't add a new secret tier; the existing `secrets.get_secret()` + three-tier lookup (env var → file → Keychain) is reused. + +4. **Time-bounded SFTP operations use `asyncio.to_thread` + + `asyncio.wait_for`.** `paramiko` is synchronous; the scheduler + already wraps calls in `asyncio.to_thread` for the stub path. + Real-mode calls get the same treatment so a hanging `listdir_attr` + on the worker thread can be cancelled by the event loop and + surfaced as a `Scheduler._last_error`. + +5. **Default SFTP operation timeout is 30s.** `CYCLONE_SFTP_OP_TIMEOUT_SECONDS` + lets operators tune it down (e.g. 10s for a flaky VPN uplink) or + up for slow connections. The poll-interval itself remains + `CYCLONE_SCHEDULER_POLL_SECONDS` (default 60s). + +6. **Reconciliation runs inside the 835 ingest session.** No + background reconciler process. If a future need emerges (catch-up + reconcile on old batches) it lands in a future SP with a + separate `cyclone reconcile` CLI subcommand. + +7. **Chain endpoint renders even when slots are empty.** The contract + is "give me this claim's chain; if any piece is missing, return + `null` for that field with `missing: ["277ca_ack"]` so the UI can + show a placeholder." This matches the operator mental model: + "I submitted 837 week-1, the 999 came back accepted, the 277CA + came back accepted, but I haven't seen the 835 yet — show me that + intermediate state." + +8. **`Scheduler.status()` gains `consecutive_failures`, + `last_error_at`, `last_error`, `last_sftp_attempt_at`.** + The existing fields (`running`, `poll_interval_seconds`, + `sftp_block_name`, `last_poll_at`, `poll_count`, totals, last_tick) + are unchanged for backward-compat. + +9. **The `claim.rejected_after_remit` audit event is informational, + not blocking.** The 999/277CA handlers still write the rejection + even when a matched remit exists; the audit event is the operator's + signal to manually unmatch the pair. We do NOT auto-unmatch (would + violate the manual-match semantics from T15). + +10. **The startup invariant check is log-only.** It does not block + boot. A drift between `Claim.matched_remittance_id` and + `Remittance.claim_id` indicates a historical bug; we surface it + in the logs and continue. Operators who want to reconcile the + drift have a one-shot CLI subcommand (`cyclone reconcile + reindex-matches`) deferred to a follow-up SP. + +## 3. Architecture (after SP27) + +``` +backend/src/cyclone/ + scheduler.py — slim: Scheduler class + HANDLERS registry + lifecycle + handlers/ NEW subpackage + __init__.py + _ack_id.py — ack_count_summary, ack_synthetic_source_batch_id, *_277ca_* + ack.py — top-level ack dispatch (re-exported helpers) + handle_999.py — text → parsed 999 → ack row → claim rejections + handle_835.py — text → parsed 835 → batch + remits + CasAdjustments + + match claims (atomic one-section) + handle_277ca.py — text → parsed 277CA → ack row + claim rejections + + emit claim.rejected_after_remit + handle_ta1.py — text → parsed TA1 → ack row + clearhouse/__init__.py — _paramiko ops wrapped in asyncio.wait_for via to_thread + edi/filenames.py — parse_inbound_filename accepts suffix-less names + store.py — manual_match / manual_unmatch keep matched_remittance_id + and Remittance.claim_id in sync (same transaction); + startup invariant check + api.py — /api/parse-835 deduped vs scheduler handlers via + handlers.handle_835; new /api/claims/{id}/chain endpoint + reconcile.py — run() folded into handle_835; top-level `match()` + kept for the CLI subcommand (deferred) + api_routers/chain.py NEW — GET /api/claims/{id}/chain (auth, matrix_gate) + +src/ + hooks/useClaimChain.ts NEW — TanStack Query wrapper around /api/claims/{id}/chain + pages/ClaimDrawer.tsx — one new section: "Chain" with submission / 999 / 277CA / remit + placeholders; renders adjustments inline +``` + +The scheduler shrinks from 860 LOC to ~250 LOC (the Scheduler class + +singleton plumbing). Each `handle_*.py` is ~80–150 LOC and can be +tested in isolation against a real prodfiles fixture. + +## 4. Files + +**New:** + +- `backend/src/cyclone/handlers/__init__.py` +- `backend/src/cyclone/handlers/_ack_id.py` +- `backend/src/cyclone/handlers/handle_999.py` +- `backend/src/cyclone/handlers/handle_835.py` +- `backend/src/cyclone/handlers/handle_277ca.py` +- `backend/src/cyclone/handlers/handle_ta1.py` +- `backend/src/cyclone/api_routers/chain.py` +- `src/hooks/useClaimChain.ts` +- `backend/tests/test_handlers_999.py` +- `backend/tests/test_handlers_835.py` +- `backend/tests/test_handlers_277ca.py` +- `backend/tests/test_handlers_ta1.py` +- `backend/tests/test_inbound_filename_loose.py` +- `backend/tests/test_sftp_op_timeout.py` +- `backend/tests/test_scheduler_status_errors.py` +- `backend/tests/test_handler_835_atomic_reconcile.py` +- `backend/tests/test_api_claim_chain.py` +- `backend/tests/test_handler_277ca_rejected_after_remit.py` +- `backend/tests/test_store_match_invariant.py` +- `src/hooks/useClaimChain.test.ts` +- `src/pages/ClaimDrawer.test.tsx` (extension — the chain drawer section) + +**Modified:** + +- `backend/src/cyclone/scheduler.py` — slim to Scheduler class + lifecycle +- `backend/src/cyclone/edi/filenames.py` — `parse_inbound_filename` + accepts suffix-less inbound filenames; `is_inbound_filename` likewise +- `backend/src/cyclone/clearhouse/__init__.py` — wrap paramiko + operations in `asyncio.wait_for(asyncio.to_thread(...), timeout=N)` +- `backend/src/cyclone/store.py` — `manual_match` / + `manual_unmatch` write the pair in one transaction; startup + invariant log; `add` (835 branch) calls `handlers.handle_835` + instead of inlining +- `backend/src/cyclone/reconcile.py` — `run` exposed for the + follow-up CLI subcommand; `match` kept; the scheduler / 835 + handler stop calling `run` directly +- `backend/src/cyclone/api.py` — `/api/parse-835` reuses + `handlers.handle_835`; `/api/parse-999`, `/api/parse-277ca`, + `/api/parse-ta1` likewise; add `/api/claims/{id}/chain` route +- `backend/src/cyclone/api_helpers.py` (or new file) — wire + `/api/claims/{id}/chain` if the route doesn't fit the existing + pattern +- `src/pages/ClaimDrawer.tsx` — one new "Chain" section +- `src/lib/api.ts` — expose `api.fetchClaimChain(id)` +- `src/types/index.ts` — add `ClaimChain` type + +**New migrations:** none. The DB schema is unchanged. + +## 5. API surface + +| Method | Path | Returns | Auth | +|---|---|---|---| +| GET | `/api/claims/{id}/chain` | `ClaimChain` JSON | `matrix_gate` (any logged-in user) | + +No new endpoints beyond this one. No new env-var-driven behavior +beyond `CYCLONE_SFTP_OP_TIMEOUT_SECONDS` (default 30s). + +```jsonc +// GET /api/claims/{id}/chain +{ + "claim_id": "...", + "submission": { + "batch_id": "...", + "patient_control_number": "...", + "service_date_from": "2026-06-15", + "service_date_to": "2026-06-15", + "charge_amount": "125.00", + "state": "submitted", + "submitted_at": "2026-06-16T..." + }, + "ack_999": { + "source_batch_id": "999-PCN-12345678", + "ack_code": "A", + "received_count": 1, + "accepted_count": 1, + "rejected_count": 0, + "received_at": "2026-06-17T..." + } | null, + "ack_277ca": { + "source_batch_id": "277CA-000000001", + "classification": "accepted", + "received_at": "2026-06-18T..." + } | null, + "remittance": { + "id": "...", + "payer_claim_control_number": "...", + "total_paid": "85.00", + "patient_responsibility": "15.00", + "adjustment_amount": "25.00", + "status_label": "Paid", + "received_at": "2026-06-19T...", + "matched": true, + "adjustments": [ + { "group_code": "CO", "reason_code": "97", "amount": "25.00", "label": "..." } + ] + } | null, + "missing": ["277ca_ack"] // list of any of the above that came back null +} +``` + +## 6. Env vars (operator-facing) + +| Variable | Default | Purpose | +|---|---|---| +| `CYCLONE_SFTP_OP_TIMEOUT_SECONDS` | `30` | Per-operation SFTP timeout in `clearhouse`. Lower for flaky links, higher for slow ones. The existing `CYCLONE_SCHEDULER_POLL_SECONDS` and `CYCLONE_SFTP_PASSWORD[_FILE]` from SP25+26 are unchanged. | + +No new env vars beyond this one. + +## 7. Validation rules + +No new R-code rules. The 835 validator (`parser_835.validator_835`) +is called unchanged by `handlers.handle_835` inside the same +transaction; the new chain endpoint doesn't introduce new parser +or validation logic. + +## 8. Testing plan + +Target backend test count after SP27: current + 17 new tests. + +1. **Per-handler unit tests** (4 files × ~3 tests each): + `test_handlers_999.py`, `test_handlers_835.py`, + `test_handlers_277ca.py`, `test_handlers_ta1.py` — call each + handler against a prodfiles fixture and assert the persisted + rows. +2. **`test_inbound_filename_loose.py`** (4 cases): + - filename with `_835.x12` suffix → `file_type="835"` (existing behavior). + - filename without `_file_type.x12` and `orig_tx=835` → `file_type="835"` (new). + - filename without suffix and `orig_tx=999` → `file_type="999"` (new). + - filename without suffix and `orig_tx=837P` → `file_type="999"` rejected by `ALLOWED_FILE_TYPES` — no false positive for 837s. +3. **`test_sftp_op_timeout.py`** (3 cases): stub SFTP returns a slow + `sftp.listdir_attr` (raises after 60s); assert the scheduler + times out within `CYCLONE_SFTP_OP_TIMEOUT_SECONDS` + slack; + stub returns fast → assert happy path still works. +4. **`test_scheduler_status_errors.py`** (4 cases): `consecutive_failures` + increments on tick error; `last_error` populated; + `status_sftp_health` flips to `failing` after 3 fails; flips back + on next success. +5. **`test_handler_835_atomic_reconcile.py`** (3 cases): 835 ingest + persists `Remittance` with `adjustment_amount` correct from the + start; reconcile crash mid-transaction leaves no orphans; an + existing claim gets `matched_remittance_id` set in the same + transaction. +6. **`test_api_claim_chain.py`** (5 cases): happy path with all four + slots populated; missing ack_999 → `null + missing=["ack_999"]`; + missing remittance → `null + missing=["remittance"]`; missing + claim → 404; auth → 401. +7. **`test_handler_277ca_rejected_after_remit.py`** (2 cases): + 277CA rejects a claim that is already matched → emits + `claim.rejected_after_remit` audit event with both IDs; + 277CA rejects an unmatched claim → no audit event. +8. **`test_store_match_invariant.py`** (3 cases): + `manual_match` writes `Claim.matched_remittance_id` and + `Remittance.claim_id` in one transaction; + `manual_unmatch` clears both; pre-existing drift is logged at + startup without blocking boot. +9. **Frontend useClaimChain.test.ts** (3 cases): hook fetches the + chain and returns typed data; loading/error states; caches + for 30s. +10. **Frontend ClaimDrawer.test.tsx extension** (2 cases): renders + the chain section; renders placeholders for missing pieces. + +The full backend suite (`cd backend && .venv/bin/pytest`) and +frontend suite (`npm test`) remain the merge gate. + +## 9. Out of scope (future SPs) + +- Frontend scheduler operator UI (start/stop/tick/status page). +- Multi-tenant / per-payer SFTP blocks. +- `_FILE` Docker-secret convention beyond what SP26 added. +- Outbound file pickup verification. +- Auto-unmatch on rejection (would violate T15 manual-match contract). +- `cyclone reconcile reindex-matches` one-shot CLI for historical drift. +- Per-payer chain-fanout (different payer-specific parsing of the same + 277CA segment). +- Persisting the chain response server-side (it is a join, not a stored + view; keeping it computed avoids drift).