docs(spec): design for SP27 remittances architecture refactor

Tier 1: split scheduler.py into handlers/ subpackage, dedup helpers, loosen INBOUND_RE, add SFTP operation timeouts, surface SFTP errors in Scheduler.status(). Tier 2: unify 835 ingest + reconciliation into one critical section, add GET /api/claims/{id}/chain, guard matched_remittance_id ↔ Remittance.claim_id invariant, emit claim.rejected_after_remit audit when a 277CA rejection hits a matched claim. Status: Draft, awaiting user sign-off.
2026-06-29 09:54:33 -06:00
parent f57f1d2875
commit 454c3598b1
1 changed files with 391 additions and 0 deletions
@@ -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/<type>.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.
+- `<NAME>_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).