docs(spec): resume SP21 store split — 14 modules, 3 SP27 symbols, 3 private-helper re-exports
Update the 2026-06-21 approved spec with the post-SP27 reality: store.py grew 2,412 → 2,995 LOC. Three new top-level symbols (dashboard_kpis, _claim_state_str, check_matched_pair_drift) get slotted into the new kpis.py and claim_detail.py. Three private helpers (the original _claim_status_from_validation + the newly- discovered _persist_835_remit and _remittance_835_row) are re- exported from the facade to preserve the 4 test files that import them. Branch bumped to sp21-store-split per SP-N convention. All previously-locked Decisions are preserved verbatim.
This commit is contained in:
Nora
@@ -1,13 +1,26 @@
|
||||
# CycloneStore split (Step 4) — Design Spec
|
||||
|
||||
**Date:** 2026-06-21
|
||||
**Status:** Draft, pending user review
|
||||
**Branch:** `main` (split will land as a single atomic commit)
|
||||
**Scope:** Behaviour-preserving structural split of `backend/src/cyclone/store.py` (2,412 lines) into a `cyclone/store/` subpackage. Zero public API changes, zero test changes.
|
||||
**Date:** 2026-06-21 (original); 2026-06-29 (resumed)
|
||||
**Status:** Approved 2026-06-21; resumed 2026-06-29 after SP25-SP27 grew the file
|
||||
**Branch:** `sp21-store-split` (per SP-N convention; lands as a single atomic merge commit into `main`)
|
||||
**Scope:** Behaviour-preserving structural split of `backend/src/cyclone/store.py` (2,995 lines as of 2026-06-29) into a `cyclone/store/` subpackage. Zero public API changes, zero test changes.
|
||||
|
||||
## Delta vs. 2026-06-21 baseline
|
||||
|
||||
Resumed on 2026-06-29 because SP25-SP27 added 583 lines to `store.py` and three new top-level symbols (`dashboard_kpis`, `_claim_state_str`, `check_matched_pair_drift`) that the original 13-module layout didn't anticipate. Everything in **Decisions** below is preserved verbatim from the 2026-06-21 spec; only the implementation details (module count, baseline numbers, import surface) are updated.
|
||||
|
||||
| | 2026-06-21 | 2026-06-29 |
|
||||
|---|---|---|
|
||||
| `store.py` size | 2,412 LOC | 2,995 LOC |
|
||||
| Target modules | 13 | **14** (new `kpis.py`) |
|
||||
| Test baseline | ~712 passed / ~31 failed / ~16 skipped | **1,176 passed / 1 failed (isolation flake) / 10 skipped** |
|
||||
| Test files using `_lock`/`_batches` idiom | 17 | **7** |
|
||||
| Private-helper leaks via `from cyclone.store import _` | 1 (`_claim_status_from_validation`) | **3** (`_persist_835_remit`, `_remittance_835_row` were imported by tests since SP7 but not audited at spec time) |
|
||||
| New top-level symbols to slot | — | `dashboard_kpis`, `_claim_state_str`, `check_matched_pair_drift` |
|
||||
|
||||
## Context
|
||||
|
||||
`store.py` is the SQLAlchemy-backed facade over the parsed-X12 store. It sits on the hot path of `parse-999`, `parse-277ca`, reconciliation, and inbox match/unmatch. At 2,412 lines it has outgrown a single file: 41 methods on `CycloneStore`, ~17 module-level helpers, Pydantic models, ORM row builders, UI serializers, and exception types all live in one module.
|
||||
`store.py` is the SQLAlchemy-backed facade over the parsed-X12 store. It sits on the hot path of `parse-999`, `parse-277ca`, reconciliation, and inbox match/unmatch. At 2,995 lines it has outgrown a single file: 41 methods on `CycloneStore`, ~20 module-level helpers, Pydantic models, ORM row builders, UI serializers, dashboard aggregators, and exception types all live in one module.
|
||||
|
||||
This is "Step 4" of the ongoing refactor series. Steps 1–3 (the api.py splits — commits `931782b`, `fc73075`, `3b5e2af`, `eb674f8`, `ff43f90`, `6ce6385`, `e63be87`, `a63ba5e`) established the pattern: turn a monolithic file into a subpackage with a thin facade, preserving every public import.
|
||||
|
||||
@@ -32,10 +45,12 @@ backend/src/cyclone/
|
||||
│ _svc_to_wire_dict / _date_in_bounds / _provider_orm_to_dict /
|
||||
│ _payer_orm_to_dict
|
||||
├── write.py ← add, _publish_events_sync, _sync_publish, _run_reconcile
|
||||
│ (biggest single module — ~210 lines)
|
||||
│ (biggest single module — ~280 lines after SP27 growth)
|
||||
├── batches.py ← get_batch, get, list, all, load_two_for_diff, _BatchesShim, _row_to_record
|
||||
├── claim_detail.py ← get_remittance, get_claim_detail, iter_claims, iter_remittances,
|
||||
│ distinct_providers, recent_activity
|
||||
│ distinct_providers, recent_activity,
|
||||
│ check_matched_pair_drift ← NEW from SP27 (cross-table invariant)
|
||||
├── kpis.py ← dashboard_kpis, _claim_state_str ← NEW MODULE from SP27
|
||||
├── acks.py ← add_ack, list_acks, get_ack (999),
|
||||
│ add_ta1_ack, list_ta1_acks, get_ta1_ack,
|
||||
│ add_277ca_ack, list_277ca_acks, get_277ca_ack
|
||||
@@ -45,7 +60,11 @@ backend/src/cyclone/
|
||||
get_payer_config, get_clearhouse, ensure_clearhouse_seeded
|
||||
```
|
||||
|
||||
13 modules total. Largest is `write.py` at ~210 lines; all others are ≤150 lines. Facade `__init__.py` ≈ 80 lines (re-exports + class + singleton).
|
||||
14 modules total. Largest is `write.py` at ~280 lines; second-largest is `ui.py` at ~250 lines; all others are ≤180 lines. Facade `__init__.py` ≈ 100 lines (re-exports + class + singleton + the 3 new private-helper re-exports).
|
||||
|
||||
**Why a separate `kpis.py`:** `dashboard_kpis` and `_claim_state_str` together are a coherent unit — read-only aggregations across claims + remittances + batches that the UI consumes as a single dashboard payload. They don't fit any existing module's responsibility (`ui.py` is row-formatters, `claim_detail.py` is single-claim reads). A 14th module is one file with two functions; co-locating them in `ui.py` would push `ui.py` past 300 LOC and mix row-shaping with aggregate aggregation.
|
||||
|
||||
**Why `check_matched_pair_drift` in `claim_detail.py`:** It's a read-only invariant check that joins `claims` + `remittances` — the same pair-of-tables `get_claim_detail`'s matched-remittance summary reads from. No other module touches both tables for a single read; creating a 15th file for one function would be over-decomposition.
|
||||
|
||||
## CycloneStore class shape
|
||||
|
||||
@@ -76,7 +95,7 @@ Each module function inlines `with db.SessionLocal()() as s:` at the top — mat
|
||||
|
||||
### Lock + shim stay on the class
|
||||
|
||||
`CycloneStore.__init__` keeps `self._lock = threading.RLock()` and `self._batches = _BatchesShim()` because 17 test files use `with store._lock: store._batches.clear()` as the cleanup idiom. The `_BatchesShim` class itself moves to `batches.py` alongside `_row_to_record`.
|
||||
`CycloneStore.__init__` keeps `self._lock = threading.RLock()` and `self._batches = _BatchesShim()` because 7 test files still use `with store._lock: store._batches.clear()` as the cleanup idiom (down from 17 at spec time — others moved to `db._reset_for_tests()` over SP23-SP27). The `_BatchesShim` class itself moves to `batches.py` alongside `_row_to_record`.
|
||||
|
||||
### Naming collision table
|
||||
|
||||
@@ -98,21 +117,34 @@ Each module function inlines `with db.SessionLocal()() as s:` at the top — mat
|
||||
|
||||
| Caller | Import | After split |
|
||||
|---|---|---|
|
||||
| `cyclone/api.py` | `from cyclone.store import (CycloneStore, store, BatchRecord, AlreadyMatchedError, ...)` | unchanged |
|
||||
| `cyclone/api.py` (deferred) | `from cyclone.store import NotMatchedError` | unchanged |
|
||||
| `cyclone/batch_diff.py` | `from cyclone.store import BatchRecord, BatchRecord835, BatchRecord837, _claim_status_from_validation` | facade re-exports `_claim_status_from_validation` |
|
||||
| `cyclone/backup_service.py` (deferred) | `from cyclone.store import store as cycl_store` | unchanged |
|
||||
| `cyclone/api.py` (top of file, line 87) | `from cyclone.store import (AlreadyMatchedError, BatchRecord, InvalidStateError, dashboard_kpis, store, utcnow)` | unchanged |
|
||||
| `cyclone/api.py` (line 155, in startup hook) | `from cyclone.store import check_matched_pair_drift` | unchanged |
|
||||
| `cyclone/api.py` (line 2182, deferred) | `from cyclone.store import NotMatchedError` | unchanged |
|
||||
| `cyclone/batch_diff.py` (line 32) | `from cyclone.store import BatchRecord, BatchRecord835, BatchRecord837, _claim_status_from_validation` | facade re-exports `_claim_status_from_validation` |
|
||||
| `cyclone/backup_service.py` (line 231, deferred) | `from cyclone.store import store as cycl_store` | unchanged |
|
||||
| `cyclone/api_routers/acks.py` | `from cyclone.store import store` | unchanged |
|
||||
| `cyclone/api_routers/ta1_acks.py` | `from cyclone.store import store` | unchanged |
|
||||
| `cyclone/scheduler.py` | `from cyclone.store import store as cycl_store` (also `BatchRecord` deferred) | unchanged |
|
||||
| `cyclone/handlers/handle_999.py` | `from cyclone.store import store as cycl_store` | unchanged (NEW from SP27) |
|
||||
| `cyclone/handlers/handle_277ca.py` | `from cyclone.store import store as cycl_store` | unchanged (NEW from SP27) |
|
||||
| `cyclone/handlers/handle_ta1.py` | `from cyclone.store import store as cycl_store` | unchanged (NEW from SP27) |
|
||||
| `cyclone/handlers/handle_835.py` | `from cyclone.store import BatchRecord, store as cycl_store` | unchanged (NEW from SP27) |
|
||||
| `cyclone/parsers/validator.py` (3 deferred) | `from cyclone import store as store_mod` | unchanged |
|
||||
| `cyclone/api_helpers.py` | docstring references `cyclone.store.utcnow` | unchanged (facade re-exports `utcnow`) |
|
||||
|
||||
**Open decision:** `_claim_status_from_validation` is currently a module-level helper imported by `batch_diff.py` (line 32). Recommended: re-export it from `__init__.py` to preserve the import. This is a small facade pollution but matches the precedent of the api.py split (which re-exports routers).
|
||||
**Resolved open item:** `_claim_status_from_validation` is re-exported from `__init__.py` (preserves `batch_diff.py:32`). Two further private-helper leaks surfaced during the SP25-SP27 audit and are re-exported the same way:
|
||||
|
||||
| Private helper | Imported by | Lives in |
|
||||
|---|---|---|
|
||||
| `_claim_status_from_validation` | `batch_diff.py:32` | `orm_builders.py` |
|
||||
| `_persist_835_remit` | `test_service_line_payments.py`, `test_api_line_reconciliation.py`, `test_reconcile_line_level.py`, `test_inbox_endpoints_sp7.py` | `orm_builders.py` |
|
||||
| `_remittance_835_row` | same 4 test files | `orm_builders.py` |
|
||||
|
||||
All three are leading-underscore "internal but reachable" names; re-exporting them from the facade is the same precedent the api.py split set when it re-exported routers. The `_lock` + `_batches.clear()` idiom check is still 7 files (not the original 17); the test files that no longer use it moved to `db._reset_for_tests()` over SP23-SP27.
|
||||
|
||||
### Tests — zero changes
|
||||
|
||||
17 test files use `with store._lock: store._batches.clear()` — all continue to work because `CycloneStore.__init__` still sets those attributes and `_BatchesShim` continues to function identically. The 3 dedicated store tests (`test_store.py`, `test_store_claim_detail.py`, `test_store_reconcile.py`) import from `cyclone.store` (still works) or `cyclone` (still works).
|
||||
7 test files still use `with store._lock: store._batches.clear()` — all continue to work because `CycloneStore.__init__` still sets those attributes and `_BatchesShim` continues to function identically. The 3 dedicated store tests (`test_store.py`, `test_store_claim_detail.py`, `test_store_reconcile.py`) import from `cyclone.store` (still works) or `cyclone` (still works). The 4 test files that import `_persist_835_remit` / `_remittance_835_row` continue to work because the facade re-exports them.
|
||||
|
||||
## Migration plan
|
||||
|
||||
@@ -121,10 +153,10 @@ Each module function inlines `with db.SessionLocal()() as s:` at the top — mat
|
||||
```bash
|
||||
rm backend/src/cyclone/store.py
|
||||
mkdir -p backend/src/cyclone/store
|
||||
# write all 13 new files
|
||||
# write all 14 new files
|
||||
cd backend && python -m pytest tests/ --tb=line -q # must be green
|
||||
git add -A
|
||||
git commit -m "refactor(store): split store.py into cyclone/store/ subpackage"
|
||||
git commit -m "refactor(sp21): split store.py into cyclone/store/ subpackage (14 modules)"
|
||||
```
|
||||
|
||||
**Follow-up commit (only if needed):** Docs update — add "CycloneStore subpackage" section to README following the precedent of commits `81aebf5`, `2718114`, `ea64e6e`, `804e557`.
|
||||
@@ -145,7 +177,7 @@ The split is structural only. Every test that passes today must pass after.
|
||||
cd backend && python -m pytest tests/ --tb=line -q 2>&1 | tee /tmp/cyclone-baseline.txt | tail -5
|
||||
```
|
||||
|
||||
Expected baseline: ~712 passed, ~31 failed (pre-existing), ~16 skipped. The 31 failures are documented cross-test pollution (11 `test_serialize_837`, 3 `test_secrets`, 2 `test_sftp_paramiko`, etc., verified in commit `931782b`). Post-split count must match exactly.
|
||||
Expected baseline (captured 2026-06-29 on `main`): **1,176 passed, 1 failed, 10 skipped**. The single failure is `tests/test_provider_extended_response.py::test_provider_detail_includes_orphan_remit_received`, which is a known test-isolation flake (passes when run solo, fails after other tests in the suite due to shared state) — verified pre-SP21 and not caused by this refactor. Post-split count must match exactly: 1,176 / 1 / 10.
|
||||
|
||||
**Tier 2 — Hot-path focused (14 files).** Exercises every method being split.
|
||||
|
||||
@@ -190,7 +222,7 @@ Smoke output is documented in the commit message.
|
||||
| R4 | `add_record()` body diverges from current `add()` | Medium | High | Mechanical copy of lines 898-1107, strip `self.` prefix, change 3 private-method calls to module-function calls. No logic refactoring. |
|
||||
| R5 | Singleton duplicated | Low | High | Declared exactly once, in `__init__.py`. |
|
||||
| R6 | TYPE_CHECKING imports break runtime | Low | Low | `EventBus` already used in current `add()` signature; same pattern. |
|
||||
| R7 | Test imports of private helpers break | Low | Medium | `grep -rn "from cyclone.store import _" backend/tests/` before impl; re-export anything found. |
|
||||
| R7 | Test imports of private helpers break | Low → **Materialized** | Medium | Audited 2026-06-29: 3 leaks found (`_claim_status_from_validation`, `_persist_835_remit`, `_remittance_835_row`). All three are re-exported from the facade. Re-audit before merge. |
|
||||
| R8 | Package discovery misses new subpackage | Low | Medium | Verify with `pip install -e .` before commit; if needed, add explicit `packages = [...]`. |
|
||||
| R9 | Generator methods leak sessions | Low | Medium | Preserve `with` inside generator body. |
|
||||
| R10 | Docstring drift | Low | Low | Preserve existing docstrings verbatim on re-exports. |
|
||||
@@ -237,7 +269,8 @@ python -c "
|
||||
from cyclone.store import (
|
||||
CycloneStore, store, BatchRecord, BatchRecord837, BatchRecord835, BatchKind,
|
||||
AlreadyMatchedError, NotMatchedError, InvalidStateError, utcnow,
|
||||
_claim_status_from_validation,
|
||||
dashboard_kpis, check_matched_pair_drift,
|
||||
_claim_status_from_validation, _persist_835_remit, _remittance_835_row,
|
||||
)
|
||||
print('all imports OK')
|
||||
"
|
||||
@@ -254,11 +287,17 @@ print('all imports OK')
|
||||
- Adding new tests
|
||||
- Modifying DB models in `cyclone.db`
|
||||
- Updating SQLCipher key rotation flow
|
||||
- Touching any of the 17 test files that use `_lock` / `_batches.clear()`
|
||||
- Touching any importer in `cyclone.api`, `cyclone.api_routers.*`, `cyclone.batch_diff`, `cyclone.backup_service`, `cyclone.scheduler`, `cyclone.parsers.validator`, `cyclone.api_helpers`
|
||||
- Touching any of the 7 test files that use `_lock` / `_batches.clear()`
|
||||
- Touching any of the 4 test files that import `_persist_835_remit` / `_remittance_835_row`
|
||||
- Touching any importer in `cyclone.api`, `cyclone.api_routers.*`, `cyclone.batch_diff`, `cyclone.backup_service`, `cyclone.scheduler`, `cyclone.handlers.*`, `cyclone.parsers.validator`, `cyclone.api_helpers`
|
||||
|
||||
If any of these surface during implementation, they get deferred to follow-up tasks — never silently included.
|
||||
|
||||
## Open items
|
||||
|
||||
1. **`_claim_status_from_validation` re-export** — recommended re-export from `__init__.py`. User to confirm during spec review.
|
||||
All open items from the 2026-06-21 draft are resolved:
|
||||
|
||||
1. **`_claim_status_from_validation` re-export** — **resolved**: re-exported from `__init__.py` (confirmed by brainstorming 2026-06-29).
|
||||
2. **`_persist_835_remit` / `_remittance_835_row` re-export** — **resolved** (NEW during resume): both re-exported from `__init__.py` to preserve the 4 test files that import them.
|
||||
3. **`_claim_state_str` placement** — **resolved** (NEW during resume): co-located with `dashboard_kpis` in the new `kpis.py` module.
|
||||
4. **`check_matched_pair_drift` placement** — **resolved** (NEW during resume): in `claim_detail.py` (cross-table read with the same pair-of-tables territory as `get_claim_detail`'s matched-remit summary).
|
||||
Reference in New Issue
Block a user