docs(spec): design for CycloneStore split (Step 4)
Step 4 of the file-split refactor series. Targets backend/src/cyclone/store.py (2,412 lines) which sits on the hot path of parse-999, parse-277ca, reconciliation, and inbox match/unmatch. Decisions locked: - Public surface: cyclone/store/ subpackage + thin facade (__init__.py re-exports every currently-importable name). - Class shape: module functions for bodies, CycloneStore keeps its current method signatures as 1-line delegations. - Helper distribution: dedicated utility modules (records, orm_builders, ui, exceptions). 13 target modules total; largest is write.py at ~210 lines. Zero public API changes, zero test changes. Single atomic commit migration; rollback is a single git revert.
This commit is contained in:
Tyler
@@ -0,0 +1,264 @@
|
||||
# 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.
|
||||
|
||||
## 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.
|
||||
|
||||
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.
|
||||
|
||||
## Decisions (locked during brainstorming)
|
||||
|
||||
1. **Public surface:** Subpackage + thin facade. `cyclone/store.py` is deleted; replaced by `cyclone/store/__init__.py` re-exporting every currently-importable name.
|
||||
2. **Class structure:** Module functions for the bodies, `CycloneStore` keeps its current method signatures as 1-line delegations. (Mixin classes were considered and rejected for added MRO surface area; composition was rejected for breaking `isinstance` and adding 41 delegating boilerplate methods in a different shape.)
|
||||
3. **Helper distribution:** Dedicated utility modules for non-method helpers — `records.py`, `orm_builders.py`, `ui.py`, `exceptions.py`. Co-location was rejected for circular-import risk.
|
||||
|
||||
## Target module layout
|
||||
|
||||
```
|
||||
backend/src/cyclone/
|
||||
├── store.py ← DELETED
|
||||
└── store/
|
||||
├── __init__.py ← thin facade: re-exports + CycloneStore + `store` singleton + utcnow
|
||||
├── exceptions.py ← AlreadyMatchedError, NotMatchedError, InvalidStateError
|
||||
├── records.py ← BatchKind, BatchRecord, BatchRecord837, BatchRecord835 (Pydantic)
|
||||
├── orm_builders.py ← _service_dates_from_claim, _claim_837_row, _remittance_835_row,
|
||||
│ _cas_adjustment_row, _persist_835_remit, _claim_status_from_validation
|
||||
├── ui.py ← all to_ui_* / _iso_z / _address_to_ui / _validation_issues_to_ui /
|
||||
│ _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)
|
||||
├── 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
|
||||
├── 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
|
||||
├── backups.py ← add_backup_pending
|
||||
├── inbox.py ← list_unmatched, manual_match, manual_unmatch
|
||||
└── providers.py ← list_providers, get_provider, upsert_provider, list_payers,
|
||||
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).
|
||||
|
||||
## CycloneStore class shape
|
||||
|
||||
### Three categories of methods
|
||||
|
||||
**Category A — Pure read delegates.** Open session, run query, return. No shared state, no private-method calls, no events.
|
||||
|
||||
Batches reads: `get_batch`, `get`, `list`, `all`, `load_two_for_diff`, `get_remittance`.
|
||||
Claim detail reads: `get_claim_detail`, `iter_claims`, `iter_remittances`, `distinct_providers`, `recent_activity`.
|
||||
ACK reads: `list_acks`, `get_ack`, `list_ta1_acks`, `get_ta1_ack`, `list_277ca_acks`, `get_277ca_ack`.
|
||||
Provider/payer reads: `list_providers`, `get_provider`, `list_payers`, `get_payer_config`, `get_clearhouse`, `ensure_clearhouse_seeded`.
|
||||
|
||||
**Category B — Write-with-side-effects.** Module functions take an explicit `event_bus` kwarg where applicable; the `CycloneStore` methods delegate. The 3 private methods (`_publish_events_sync`, `_run_reconcile`, `_sync_publish`) stay on the class as 1-line delegations so any third-party calling them by private API doesn't break.
|
||||
|
||||
Methods: `add`, `add_ack`, `add_ta1_ack`, `add_277ca_ack`, `add_backup_pending`. Only `add` has a non-trivial body (insert + idempotency + reconcile + publish); the ACK add methods and `add_backup_pending` are simple single-row inserts that move to `acks.py` and `backups.py` respectively.
|
||||
|
||||
**Category C — Manual match/unmatch (3 methods).** Use `reconcile.apply_payment` / `apply_reversal`; translate `skipped=True` into `InvalidStateError`. Module functions in `inbox.py`; class methods delegate.
|
||||
|
||||
Methods: `list_unmatched`, `manual_match`, `manual_unmatch`.
|
||||
|
||||
### Iterator methods
|
||||
|
||||
`iter_claims`, `iter_remittances` are generators. Module functions in `claim_detail.py` use `with db.SessionLocal()() as s:` and `yield` inside; session lives for generator lifetime. Class methods delegate.
|
||||
|
||||
### Session management
|
||||
|
||||
Each module function inlines `with db.SessionLocal()() as s:` at the top — matches the existing pattern verbatim. No extracted `session_scope()` helper (YAGNI; ~30 call sites would change with no behaviour gain).
|
||||
|
||||
### 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`.
|
||||
|
||||
### Naming collision table
|
||||
|
||||
| Class method | Module function | Reason |
|
||||
|---|---|---|
|
||||
| `store.list` | `batches.list_batches` | shadows builtin |
|
||||
| `store.all` | `batches.all_batches` | shadows builtin |
|
||||
| `store.add` | `write.add_record` | more descriptive |
|
||||
| `store.get` | `batches.get_record` | matches existing test usage |
|
||||
| `store.utcnow` | top-level `utcnow()` in `__init__.py` | was always a free function |
|
||||
|
||||
### Singleton
|
||||
|
||||
`store = CycloneStore()` declared exactly once, in `__init__.py`. All sibling modules reference `cyclone.db` directly for session access, never the singleton.
|
||||
|
||||
## Public API preservation
|
||||
|
||||
### Source callers — zero changes
|
||||
|
||||
| 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_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/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).
|
||||
|
||||
### 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).
|
||||
|
||||
## Migration plan
|
||||
|
||||
**Single atomic commit.** `cyclone/store.py` cannot coexist with `cyclone/store/` (Python prefers packages), so deletion and creation must happen together:
|
||||
|
||||
```bash
|
||||
rm backend/src/cyclone/store.py
|
||||
mkdir -p backend/src/cyclone/store
|
||||
# write all 13 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"
|
||||
```
|
||||
|
||||
**Follow-up commit (only if needed):** Docs update — add "CycloneStore subpackage" section to README following the precedent of commits `81aebf5`, `2718114`, `ea64e6e`, `804e557`.
|
||||
|
||||
**Rollback:** `git revert <sha>` — single-commit revert, no half-state.
|
||||
|
||||
## Test strategy
|
||||
|
||||
### No new tests, no test modifications
|
||||
|
||||
The split is structural only. Every test that passes today must pass after.
|
||||
|
||||
### Verification tiers
|
||||
|
||||
**Tier 1 — Full backend suite, baseline + after.**
|
||||
|
||||
```bash
|
||||
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.
|
||||
|
||||
**Tier 2 — Hot-path focused (14 files).** Exercises every method being split.
|
||||
|
||||
```bash
|
||||
cd backend && python -m pytest tests/test_parse_999.py tests/test_parse_277ca.py \
|
||||
tests/test_api_999.py tests/test_api_277ca.py \
|
||||
tests/test_reconcile.py tests/test_reconcile_line_level.py \
|
||||
tests/test_store_reconcile.py tests/test_api_line_reconciliation.py \
|
||||
tests/test_inbox_state.py tests/test_inbox_lanes.py \
|
||||
tests/test_inbox_endpoints.py tests/test_inbox_endpoints_sp7.py \
|
||||
tests/test_payer_rejected_acknowledge.py tests/test_store.py \
|
||||
--tb=short -q
|
||||
```
|
||||
|
||||
**Tier 3 — Store-specific (3 files).** Covers the class surface directly.
|
||||
|
||||
```bash
|
||||
cd backend && python -m pytest tests/test_store.py tests/test_store_claim_detail.py \
|
||||
tests/test_store_reconcile.py --tb=short -q
|
||||
```
|
||||
|
||||
**Tier 4 — Live smoke on 8 endpoints** (matches commit `931782b` precedent):
|
||||
|
||||
- `POST /api/parse-999` (valid → ack_code=A; rejected → ack_code=R)
|
||||
- `POST /api/parse-277ca` (valid → 200 with 3 claim statuses)
|
||||
- `GET /api/inbox?lane=unmatched` (list_unmatched shape)
|
||||
- `POST /api/inbox/match` (manual_match happy path + AlreadyMatchedError)
|
||||
- `POST /api/inbox/unmatch` (manual_unmatch happy path + NotMatchedError)
|
||||
- `GET /api/batches` + `GET /api/batches/{id}` (get_batch shape)
|
||||
- `GET /api/claims/{id}` (get_claim_detail shape)
|
||||
- `GET /api/999-acks/{id}` and `GET /api/277ca-acks/{id}` (ack list/get shape)
|
||||
|
||||
Smoke output is documented in the commit message.
|
||||
|
||||
## Risk register
|
||||
|
||||
| # | Risk | Likelihood | Impact | Mitigation |
|
||||
|---|---|---|---|---|
|
||||
| R1 | Circular import between `__init__.py` and a sibling | Low | High | Siblings never import from `__init__.py`. Module docstrings state this rule. |
|
||||
| R2 | Module function loses `self.` context | Low | Medium | `CycloneStore.__init__` keeps `_lock` and `_batches`. Module functions don't touch them. |
|
||||
| R3 | `_BatchesShim.clear()` semantics change | Low | High | `_BatchesShim` moves verbatim; line-by-line verification during impl. |
|
||||
| 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. |
|
||||
| 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. |
|
||||
|
||||
## Implementation discipline
|
||||
|
||||
1. **Don't refactor logic.** The split is structural. Code smells stay for separate tasks.
|
||||
2. **Don't add type hints.** Match existing annotations exactly.
|
||||
3. **Don't add docstrings.** Preserve existing; only add a short module-level docstring per new file.
|
||||
4. **Preserve order of operations.** Identical to current `add()` body.
|
||||
5. **Don't optimize imports in passing.** Move them as-is.
|
||||
|
||||
## Pre-flight checklist
|
||||
|
||||
```bash
|
||||
# 1. Capture baseline
|
||||
cd backend && python -m pytest tests/ --tb=line -q 2>&1 | tee /tmp/cyclone-baseline.txt | tail -5
|
||||
|
||||
# 2. Find every private helper imported from cyclone.store in tests
|
||||
grep -rn "from cyclone.store import _" backend/tests/
|
||||
|
||||
# 3. Find every private helper imported from cyclone.store in source
|
||||
grep -rn "from cyclone.store import _" backend/src/
|
||||
|
||||
# 4. Verify package discovery
|
||||
cd backend && pip install -e . --quiet
|
||||
python -c "import cyclone.store; print(cyclone.store.__file__)"
|
||||
|
||||
# 5. Verify no other cyclone/store* conflicts
|
||||
find backend/src/cyclone -maxdepth 1 -name "store*"
|
||||
```
|
||||
|
||||
## Post-flight checklist
|
||||
|
||||
```bash
|
||||
# 1. Compare to baseline
|
||||
cd backend && python -m pytest tests/ --tb=line -q 2>&1 | tail -5
|
||||
# Must match baseline exactly
|
||||
|
||||
# 2. Hot-path focused (14 files) — must be 100% green
|
||||
|
||||
# 3. Import sanity check
|
||||
python -c "
|
||||
from cyclone.store import (
|
||||
CycloneStore, store, BatchRecord, BatchRecord837, BatchRecord835, BatchKind,
|
||||
AlreadyMatchedError, NotMatchedError, InvalidStateError, utcnow,
|
||||
_claim_status_from_validation,
|
||||
)
|
||||
print('all imports OK')
|
||||
"
|
||||
|
||||
# 4. Live smoke on 8 endpoints
|
||||
```
|
||||
|
||||
## Out of scope (explicit)
|
||||
|
||||
- Renaming any public symbol
|
||||
- Changing any method signature
|
||||
- Refactoring `manual_match` / `manual_unmatch` business logic
|
||||
- Refactoring `add()` body or its idempotency strategy
|
||||
- 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`
|
||||
|
||||
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.
|
||||
Reference in New Issue
Block a user