cyclone/docs/superpowers/specs/2026-06-21-cyclone-store-split-design.md

# CycloneStore split (Step 4) — Design Spec

**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,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.

## 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 — ~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,
    │                     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
    ├── 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
```

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

### 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 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

| 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` (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`) |

**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

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

**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 14 new files
cd backend && python -m pytest tests/ --tb=line -q  # must be green
git add -A
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`.

**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 (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.

```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 → **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. |

## 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,
    dashboard_kpis, check_matched_pair_drift,
    _claim_status_from_validation, _persist_835_remit, _remittance_835_row,
)
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 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

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).