diff --git a/docs/superpowers/specs/2026-06-21-cyclone-store-split-design.md b/docs/superpowers/specs/2026-06-21-cyclone-store-split-design.md new file mode 100644 index 0000000..bf5dc95 --- /dev/null +++ b/docs/superpowers/specs/2026-06-21-cyclone-store-split-design.md @@ -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 ` — 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. \ No newline at end of file