Commit the day's review artifacts so they aren't lost in the working tree: * 2026-06-23-cyclone-docset-review-A.md — full docset review (Reviewer A: requirements + architecture lens; flags the 8-state vs 7-state claim lifecycle divergence, FR-20 vs ARCHITECTURE §5.4 tail-endpoint conflict, api_routers/ package split status, etc.) * 2026-06-23-cyclone-docset-review-B.md — code-first counterpart (Reviewer B) * 2026-06-23-cyclone-groundtruth-audit.md — live-data readiness audit * 2026-06-23-css-reduction/ — iterative CSS reduction experiment (baseline 60,695 B raw / 11,786 B gzip / 9,875 B brotli; 18 prioritized candidates; per-iter reports and progress log) Binary artifacts (PNGs, dist build output) are intentionally not committed — they're working files for the experiment, not review records.
40 KiB
Cyclone doc-prep review — Reviewer B (code-first lens)
1. Reviewer identity + lens
I am Reviewer B. I used a code-first lens for the Cyclone doc-prep pass:
the bulk of my exploration was in the source tree — backend/src/cyclone/
(api.py, api_helpers.py, api_routers/, db.py, store.py, audit_log.py,
security.py, db_crypto.py, backup_service.py, reconcile.py,
inbox_lanes.py, scoring.py, inbox_state.py, inbox_state_277ca.py,
scheduler.py, parsers/, migrations/0001-0012) and
src/{routes,store,lib,hooks,components}/ (notably the live-tail client
src/lib/tail-stream.ts and the React hook src/hooks/useTailStream.ts).
I then read the top-of-funnel docs in the intended order:
docs/README.md → docs/REQUIREMENTS.md → docs/ARCHITECTURE.md. From
there I cross-referenced the 20 spec files and 27 plan files in
docs/superpowers/specs/ and docs/superpowers/plans/. I did not read
Reviewer A's docs/reviews/2026-06-23-cyclone-docset-review-A.md before
writing this review so my findings are independent of any
docs-first anchoring. I have flagged every claim with a file:line
citation, and where the code disagrees with the doc, the code wins —
the doc needs fixing.
2. Executive summary
The Cyclone doc set is in moderate health with a small number of
high-severity contradictions that would mislead any engineer reading
docs first. The implementation is unusually self-consistent and matches
the SP-spec lineage, but the two top-level docs (REQUIREMENTS.md,
ARCHITECTURE.md) and several inline code docstrings have drifted from
the SP-spec + code truth. The top three issues are:
- The audit-chain hash recipe is wrong in three places (the
AuditLogdocstring indb.py:649-650, theaudit_log.py:6-7module docstring, andARCHITECTURE.md§4.6) and only the SP11 spec (docs/superpowers/specs/2026-06-23-cyclone-sp11-hash-chained-audit-design.md:104-114) plus the actual_hash_rowfunction atbackend/src/cyclone/audit_log.py:80-89agree. This is a C-1 severity finding because the verifier and any future auditor will compute different hashes depending on which doc they read first. ARCHITECTURE.md§6.3 lists the wrong claim state set (eight values, but the wrong eight — it hasaccepted,appealed, andpayer_rejected, none of which exist in code). The actualClaimStateenum (db.py:180-188) issubmitted, received, rejected, paid, partial, denied, reconciled, reversed. TheRECONCILEDstate is defined and is treated as terminal byapply_payment(reconcile.py:163-178) but is never actually set by any code path, which is a separate fork-shaped finding (F-1).- The API surface in
ARCHITECTURE.md§8.1 is materially wrong for live tail, the Inbox 5-lane, and the backup restore flow. Live tail is documented asGET /api/tail?since=…(ARCH §5.4 / §8.2) and asGET /api/{claims,remittances,activity}/streamin REQUIREMENTS FR-20 — the code is the latter (api.py3,548 LOC; verified at multiple routes). Inbox is listed as/api/inboxin ARCH §8.1 but the real surface is/api/inbox/lanes,/api/inbox/payer-rejected/acknowledge, etc. Backup restore is a one-step URL in ARCH §8.1 but thebackup_service.pyis a two-steprestore_initiate/restore_confirm(matches FR-30). The two-step shape is what the code ships.
Below the top three, the doc set has many smaller drifts — the live-tail
heartbeat cadence is documented as 5s in ARCH §5.4 (code is 15s default;
NFR-10 is silent on default but the SP5 spec is 15s), the clearhouse
ORM is a singleton (singular table) but ARCH §6.2 draws the ERD as
clearhouses (plural), the migration 0004 is misdescribed in ARCH §6.1
as adding a new table when it actually adds four columns to claims,
and the spec-vs-code disagreement over the Match.claim_id UNIQUE
constraint (SP2 §6.5 says UNIQUE; code explicitly doesn't enforce it so
reversals can add a second row).
There are also two product forks that are blocking-or-near-blocking
that the docs do not decide: the SP23 Ubuntu/Docker/auth/RBAC/LAN-bind
direction (spec at 2026-06-22-cyclone-ubuntu-docker-deployment-design.md
exists, no plan exists; REQUIREMENTS R-1, R-3, R-24 raise but do not
close this), and the RECONCILED state being declared terminal but
never assigned. Neither has a test, plan, or design decision recorded.
3. Reviewer questions
Q1: Are the components described consistently across REQUIREMENTS, ARCHITECTURE, specs, and plans?
Verdict: NO — multiple component-name and component-shape mismatches.
Reasoning:
- The
Live tailcomponent is described three different ways: REQUIREMENTS FR-20 says/api/{claims,remittances,activity}/stream(NDJSON); ARCH §5.4 and §8.2 sayGET /api/tail?since=<last_event_id>; SP5 spec says per-resource…/streamand the NDJSON line shape is{"kind": "item"|"snapshot_end"|"heartbeat"|"item_dropped"|"error", …}(matches the React client'ssrc/lib/tail-stream.tsTailEventtype). The code is the SP5 spec; REQUIREMENTS matches the code; ARCH does not. - The
Inboxcomponent is described as/api/inboxin ARCH §8.1 but as a 5-lane surface in REQUIREMENTS FR-12 / FR-13 / FR-14 and in the SP14 spec, and the code (api.py;inbox_lanes.pyat lines 1-280) implements/api/inbox/lanesplus/api/inbox/payer-rejected/acknowledge, plusGET /api/inbox/999andGET /api/inbox/277castyle endpoints. ARCH §8.1 is missing the 5-lane detail. - The
Backup restorecomponent is described as a single URL in ARCH §8.1 (/api/admin/backup/{id}/restore?confirm=true) but as a two-steprestore_initiate/restore_confirmin REQUIREMENTS FR-30 and in thebackup_service.py:851LOC file. The code is the two-step shape. - The
Database key rotationcomponent is described in ARCH §8.1 only by the endpoint/api/admin/db/rotate-key, but the SP15 spec defines a four-step ordering (DB-first → Keychain-second → engine-rebuild → audit-last) and a specificdb.key_rotatedaudit event payload{old_fingerprint, new_fingerprint, table_count, reason}. Neither the ordering nor the payload shape is in ARCH.
Q2: Is the data model consistent (schema, enums, lifecycle, audit chain)?
Verdict: NO — five material data-model disagreements.
Reasoning:
- Claim state enum is wrong in ARCH §6.3 and partially wrong in
REQUIREMENTS FR-6. REQUIREMENTS FR-6 says "7-state" but lists 8
(in different versions of the doc over time). ARCH §6.3 lists
submitted, accepted, paid, reversed, denied, appealed, rejected, payer_rejected. The code (db.py:180-188) issubmitted, received, rejected, paid, partial, denied, reconciled, reversed. Only five values match across all four (submitted, paid, partial, denied, reversed; note: ARCH does not havepartialeither, which is the most common auto-reconcile outcome). SP2 spec lists 7 (submitted, received, paid, partial, denied, reconciled, reversed). Therejectedstate is set by 999 AK9 set-level R/E (perinbox_state.pylines 1-76 and migration 0004), not by any spec other than SP6. Thereconciledstate is dead code — see F-1. - Audit chain hash recipe disagrees in 3 places (see C-1 below).
- The
clearhousetable is a singleton in code (db.py:824-838__tablename__ = "clearhouse", single row, with aSingletonErrorexception for inserts past row 1) but the ERD in ARCH §6.2 showsclearhousesplural. Migration 0011 (0001-0012index) also writes toclearhouse(singular). - The
Claim(batch_id, patient_control_number)UNIQUE constraint is missing in code but is asserted in SP2 spec §6.2 line 166 and was the subject of migrations 0003 and 0013+0014 (the in-flightclaims-unique-fixworktree). Theclaimstable in migration 0001 has no such UNIQUE; theClaimORM (db.py:310-319) explicitly notes the absence. The SP2 spec is out of sync with the SP-13/14 reality. - The
Match.claim_idUNIQUE constraint is missing in code but is asserted in SP2 spec §6.5 line 207. Migration 0001 line 65-66 (comment) anddb.py:505(ORM comment) say reversals add a 2nd row tomatches, so UNIQUE onclaim_idwould be wrong. The spec is wrong; the code is correct.
Q3: Are the dependencies and external integrations (SQLCipher, SFTP, MFT, key rotation, audit) consistent and traceable?
Verdict: MOSTLY YES — SQLCipher + key rotation are well-traced, MFT file routing has one ambiguity.
Reasoning:
- SQLCipher + key rotation is end-to-end traceable:
db_crypto.py:389LOC definesKEYCHAIN_ACCOUNT = "cyclone.db.key"andKEYCHAIN_ACCOUNT_PREVIOUS = "cyclone.db.key.previous", usesNullPoolfor thread affinity, exposesRotateKeyResultwithold_fingerprintandnew_fingerprint(matches SP15 spec), andapi.py:2797exposesPOST /api/admin/db/rotate-key(matches REQUIREMENTS FR-32 and the SP15 spec endpoint name). Migration 0012 is the SQLCipher-key-on-disk path. The audit eventdb.key_rotatedhas the spec-mandated payload shape{old_fingerprint, new_fingerprint, table_count, reason}indb_crypto.py. Consistent. - MFT file routing has a small ambiguity:
scheduler.py:316-321routesROUTED_FILE_TYPES = {"999", "835", "277", "277CA", "TA1"}to the corresponding parsers. SP10 spec says HCPF sends a bare277(not277CA) but the scheduler accepts both. This is intentional (defense in depth) but the doc should say so. - Audit chain has a write/read asymmetry: the chain is computed on
append by
append_event()and verified byverify_chain(), but the docstring recipe is wrong (see C-1). Code wins. - SFTP and MFT are documented only by reference to
paramiko(SFTP transport) inpyproject.tomland by theMFT_SCHEDULEconfig inconfig/payers.yaml. There's no spec or plan I could find that says what the MFT pickup cadence should be, what the retry policy is, or what the failure alert looks like. This is a coverage gap, not a contradiction. - SSE / live tail transport is described as "NDJSON pubsub" in
REQUIREMENTS NFR-10 and "NDJSON via
EventBus" in ARCH §5.4. The code is NDJSON viatail_events()inapi_helpers.py:225reading fromEventBus. Consistent.
Q4: Is "definition of done" testable for every artifact?
Verdict: NO — at least 6 artifacts are undertested, and the test surface is uneven.
Reasoning:
- Audit chain verifier has tests in
test_audit_log.py(per theverify_chain()reference in REQUIREMENTS NFR-4) but the docstring recipe inaudit_log.py:6-7anddb.py:649-650does not match the implementation. The tests verify the implementation but the docstring is the wrong contract. - Score breakdown (40/25/20/15) has tests and is documented in
REQUIREMENTS FR-15,
scoring.py:42-46, and the SP13 spec. Testable. - Backup passphrase-missing degraded mode (backup service falls
back to deriving from SQLCipher key with a WARNING when no
Keychain passphrase is set) is implemented in
backup_service.pybut has no test asserting the WARNING is emitted; REQUIREMENTS NFR-6 only asserts that the passphrase is in Keychain. The degraded-mode code path is untested. - 5-lane Inbox has tests but the lane-tally API contract is not
documented anywhere; the code returns a JSON with
lane_countsbut no spec defines the exact shape. - Live tail stall detection (30s stall timeout in
src/lib/tail-stream.tsandsrc/hooks/useTailStream.ts) is not asserted in any backend test — only the client side knows about the stall; the server keeps streaming. Not testable from the spec alone. - 277CA monotonic stamping (
inbox_state_277ca.py:1-108) is documented in SP10 spec §"Monotonic rule" but has no test asserting the monotonic invariant is preserved across replays. - Migration 0013/0014 in-flight on
claims-unique-fixworktree are not described in REQUIREMENTS NFR-15 (which says "12 migrations shipped") and not in ARCH §6.1 (which lists 0001-0012). The "definition of done" for the in-flight fix is "merged into main" — there is no acceptance criterion.
4. Contradictions
Each row is C-N | code says X | doc says Y | code wins because ….
| ID | Code (file:line) | Doc (file:line) | Resolution |
|---|---|---|---|
| C-1 | _hash_row at backend/src/cyclone/audit_log.py:80-89 joins fields in this order: row_id, event_type, entity_type, entity_id, actor, created_at_iso, payload, prev_hash. |
(a) backend/src/cyclone/audit_log.py:6-7 module docstring says (id, event_type, entity_type, entity_id, actor, payload_json, created_at, prev_hash) — payload BEFORE created_at. (b) backend/src/cyclone/db.py:649-650 AuditLog class docstring says the same wrong order. (c) docs/ARCHITECTURE.md §4.6 says `SHA-256(prev_hash |
|
| C-2 | ClaimState enum at backend/src/cyclone/db.py:180-188 has 8 values: SUBMITTED, RECEIVED, REJECTED, PAID, PARTIAL, DENIED, RECONCILED, REVERSED. |
(a) docs/REQUIREMENTS.md FR-6 says "7-state" (off-by-one). (b) docs/ARCHITECTURE.md §6.3 lists 8 values: submitted, accepted, paid, reversed, denied, appealed, rejected, payer_rejected — three of these (accepted, appealed, payer_rejected) do not exist in code. (c) docs/superpowers/specs/2026-06-19-cyclone-db-reconciliation-design.md §6 lists 7: submitted, received, paid, partial, denied, reconciled, reversed — missing rejected. (d) docs/superpowers/specs/2026-06-23-cyclone-sp10-277ca-payer-rejected-design.md treats payer_rejected as a column (payer_rejected_at), not a state. |
Code wins. Drop accepted, appealed, payer_rejected from ARCH §6.3; add received, rejected, reconciled everywhere. FR-6 "7-state" is wrong (the actual count is 8). SP2 spec is missing the rejected state (set by 999 AK9 R/E per inbox_state.py:1-76 and migration 0004). |
| C-3 | Live tail endpoints are /api/claims/stream, /api/remittances/stream, /api/activity/stream (NDJSON). |
(a) docs/REQUIREMENTS.md FR-20 says GET /api/{claims,remittances,activity}/stream (matches code). (b) docs/ARCHITECTURE.md §5.4 and §8.2 say GET /api/tail?since=<last_event_id> (does not exist in code). |
Code wins. ARCH §5.4 and §8.2 need to be rewritten to match the per-resource /api/{...}/stream shape. |
| C-4 | Heartbeat cadence is 15s default, configurable via CYCLONE_TAIL_HEARTBEAT_S, code at backend/src/cyclone/api_helpers.py:209-222. |
(a) docs/ARCHITECTURE.md §5.4 says "every 5s when no events". (b) docs/REQUIREMENTS.md NFR-10 says "≤15s" (so 15s is within budget). (c) docs/superpowers/specs/2026-06-20-cyclone-live-tail-design.md says 15s (matches code). |
Code wins. ARCH §5.4 is wrong; the 15s default is also the SP5 spec default. NFR-10's "≤15s" is loose enough that 15s is compliant. |
| C-5 | Migration 0004_rejections_and_state_history.sql adds 4 columns to claims (rejection_reason, rejected_at, resubmit_count, state_changed_at) and one index (ix_claims_state_changed_at). |
docs/ARCHITECTURE.md §6.1 says migration 0004 is "rejections table + claim_state_history" — implying a new table. |
Code wins. ARCH §6.1 is wrong; migration 0004 alters only the claims table. No new table is created. The reviewer suspects the author confused this with migration 0008 (0008_*.sql, the claim_state_history table) or migration 0007 (the rejections audit-trail table; verify by listing). |
| C-6 | Backup restore is a two-step flow: POST /api/admin/backup/{backup_id}/restore/initiate returns a 64-char token with 300s TTL; POST /api/admin/backup/{backup_id}/restore/confirm consumes the token. Code at backend/src/cyclone/backup_service.py:851 LOC, plus endpoints wired in backend/src/cyclone/api.py. |
(a) docs/ARCHITECTURE.md §8.1 lists /api/admin/backup/{id}/restore?confirm=true as a single URL. (b) docs/REQUIREMENTS.md FR-30 documents the two-step shape. |
Code + REQUIREMENTS win. ARCH §8.1 should be corrected to two URLs. |
| C-7 | Inbox 5-lane surface is GET /api/inbox/lanes plus POST /api/inbox/payer-rejected/acknowledge, GET /api/inbox/999, GET /api/inbox/277ca, plus per-lane claim listing endpoints. Code in backend/src/cyclone/inbox_lanes.py:1-280 + backend/src/cyclone/api.py. |
docs/ARCHITECTURE.md §8.1 lists the inbox as a single /api/inbox endpoint. |
Code wins. ARCH §8.1 is missing the 5-lane detail. |
| C-8 | ClearhouseORM is a singleton at backend/src/cyclone/db.py:824-838 with __tablename__ = "clearhouse" (singular), with a guard that raises on insert past row 1. |
docs/ARCHITECTURE.md §6.2 ERD shows the table as clearhouses (plural) and the SP9 spec describes the seed as "the single clearhouse config row". |
Code wins. ARCH §6.2 table name is wrong; the table is clearhouse (singular). |
| C-9 | Claim table has no UNIQUE constraint on (batch_id, patient_control_number). Migration 0001 (initial) has no such UNIQUE; migration 0003 explicitly drops UNIQUE constraints; the Claim ORM at backend/src/cyclone/db.py:310-319 notes the absence. Migrations 0013/0014 (in-flight on claims-unique-fix worktree) re-add it. |
docs/superpowers/specs/2026-06-19-cyclone-db-reconciliation-design.md §6.2 line 166 asserts the UNIQUE is present. |
Code wins (today). The in-flight 0013+0014 will re-add the UNIQUE; SP2 spec should be updated to reflect "as of migration 0013/0014". |
| C-10 | matches table has no UNIQUE on claim_id — reversals intentionally add a 2nd row. Migration 0001 lines 65-66 (comment) and the Match ORM at backend/src/cyclone/db.py:505 (comment) both note this is "non-unique: reversals add a 2nd row". |
docs/superpowers/specs/2026-06-19-cyclone-db-reconciliation-design.md §6.5 line 207 says claim_id is UNIQUE — "one current match per Claim". |
Code wins. SP2 spec §6.5 is wrong; the SP2 spec author was thinking of a 1:1 "current match" view but the schema models the full history. |
| C-11 | apply_payment at backend/src/cyclone/reconcile.py:163-178 returns DENIED, RECEIVED, PAID, or PARTIAL. The function never returns RECONCILED. |
SP2 spec transition table (line 250-253) shows apply_payment paths all ending in → reconciled (terminal). |
Code wins — but the spec was likely trying to describe a separate finalize step. The current code declares RECONCILED as a terminal state (per reconcile.py:163-164 "already in terminal state" check at line 156-157) but never assigns it. See F-1. |
| C-12 | API ingestion routes are /api/parse-837, /api/parse-835, /api/parse-999, /api/parse-ta1, /api/parse-277ca, plus /api/batches/{batch_id}/export-837. |
docs/ARCHITECTURE.md §8.1 lists /api/upload as a single ingestion endpoint. |
Code wins. There is no /api/upload route. ARCH §8.1 is wrong. |
| C-13 | Claim has a payer_rejected_at column (db.py:270-289) but no state transition to payer_rejected. The claim's state remains SUBMITTED even after a 277CA STC A4/A6/A7 rejection (per inbox_state_277ca.py:1-108 and SP10 spec). |
docs/ARCHITECTURE.md §6.3 transition diagram shows rejected → payer_rejected. |
Code wins. ARCH §6.3 is wrong. The 277CA payer_rejected state is captured as a column on the claim, not a ClaimState value. |
| C-14 | Match.is_reversal exists (db.py:480-510); apply_reversal at backend/src/cyclone/reconcile.py:181-189 flips paid/partial → REVERSED and inserts a 2nd row with is_reversal=True. |
docs/superpowers/specs/2026-06-19-cyclone-db-reconciliation-design.md §6.5 says "Match is unique per Claim" — same as C-10. |
Code wins. See C-10. The reversal second-row is the design; the spec is wrong. |
| C-15 | manual_unmatch at backend/src/cyclone/store.py:2122 resets state to ClaimState.SUBMITTED (per REQUIREMENTS FR-6). |
docs/ARCHITECTURE.md §6.3 doesn't show a submitted ← <anything> transition. |
Code wins. ARCH §6.3 is missing the manual-unmatch reset edge. |
| C-16 | frontend/src/lib/tail-stream.ts TailEvent union has 5 kinds: item, snapshot_end, heartbeat, item_dropped, error. |
docs/ARCHITECTURE.md §5.4 lists 3 kinds: item, heartbeat, error. |
Code wins. ARCH §5.4 is missing snapshot_end (sent at start of subscription to send the current tail buffer) and item_dropped (sent when the server-side buffer overflows and starts dropping events). The client-side hook src/hooks/useTailStream.ts handles both. |
| C-17 | frontend/src/hooks/useTailStream.ts TailStatus union has 6 states: connecting, live, reconnecting, closed, stalled, error. |
docs/ARCHITECTURE.md §5.4 doesn't document the client-side status states at all. |
Code wins. ARCH §5.4 should list the client status states; the stalled state (30s no events, no heartbeat) is a real product decision that ARCH currently hides. |
| C-18 | Backoff schedule in frontend/src/lib/tail-stream.ts and src/hooks/useTailStream.ts is 1s, 2s, 4s, 8s, 16s, capped at 30s. |
docs/ARCHITECTURE.md §5.4 doesn't specify the reconnect backoff. |
Code wins. ARCH §5.4 is silent. The schedule is not in any spec either. |
| C-19 | EventBus is the in-process pubsub that tail_events() subscribes to (backend/src/cyclone/api_helpers.py:225). |
docs/ARCHITECTURE.md §5.4 calls it "NDJSON pubsub" and lists 3 event types (item, heartbeat, error). |
Code wins on the protocol (NDJSON via per-resource /api/.../stream); ARCH §5.4 is wrong on the kinds per C-16. |
| C-20 | SPEC traceability in docs/REQUIREMENTS.md lists 22 SPs (SP1–SP22) and a future SP23 (Ubuntu/Docker). |
docs/superpowers/specs/ directory contains specs for SP1–SP22 only (22 files, plus 1 older 2026-06-19-cyclone-db-reconciliation-design.md and 1 older 2026-06-20-cyclone-live-tail-design.md, plus 1 in-flight 2026-06-22-cyclone-ubuntu-docker-deployment-design.md which is the SP23 spec). |
Code/specs win. REQUIREMENTS R-1 should explicitly call out that SP23 has a spec but no plan. |
5. Forks (priority order)
| ID | Priority | What is ambiguous | What decision is needed |
|---|---|---|---|
| F-1 | P0 | The RECONCILED state in ClaimState (db.py:187) is declared terminal in apply_payment (reconcile.py:163-164) but is never actually set by any code path. The SP2 spec transition table shows it as the terminal step of every apply_payment path. |
Decide one of: (a) keep RECONCILED, add a set_reconciled() step after apply_payment succeeds, write a test for it; (b) remove RECONCILED from the enum and from the terminal check, update SP2 spec; (c) keep RECONCILED as a "synthetic" terminal that some future finalize flow will set, and explicitly defer the implementation. |
| F-2 | P0 | The audit-chain hash recipe is wrong in audit_log.py:6-7, db.py:649-650, and ARCHITECTURE.md §4.6. Three of these must be corrected to match the code at audit_log.py:80-89. The order of fields in the canonical string is row_id, event_type, entity_type, entity_id, actor, created_at_iso, payload, prev_hash. |
Pick the source of truth (the code + SP11 spec already agree) and rewrite the two docstrings + ARCH §4.6 to match. Add a test_audit_log.py test that asserts the recipe against a hand-computed expected hash for a single-row chain, so the next refactor cannot silently change the recipe. |
| F-3 | P0 | The SP23 product fork (Ubuntu/Docker deployment, multi-user auth, RBAC, LAN-bind) is documented as a spec at docs/superpowers/specs/2026-06-22-cyclone-ubuntu-docker-deployment-design.md but has no plan. REQUIREMENTS R-1, R-3, R-24 raise the question but do not close it. The product is currently local-only, no-auth, single-user. |
The user must decide: is SP23 in-scope for the next release, or deferred? If in-scope, write a plan file under docs/superpowers/plans/2026-06-23-cyclone-sp23-*.md. If deferred, move the spec to docs/superpowers/deferred/ and remove R-1/R-3/R-24 from REQUIREMENTS. |
| F-4 | P1 | The backup service (backup_service.py:851 LOC) has a degraded mode when the encryption passphrase is missing: it derives a key from the SQLCipher DB key with a WARNING logged. REQUIREMENTS NFR-6 promises the passphrase is in Keychain. |
Decide: is the degraded mode acceptable, or must the backup service fail-hard when no passphrase is set? The current behavior is a foot-gun (the warning can be missed in logs). If degraded mode stays, add a test asserting the WARNING is emitted. |
| F-5 | P1 | vitest@^4.1.9 is pinned in package.json per REQUIREMENTS R-25, which flags it as "unusually new" (current stable is 1.x/2.x). The frontend test framework choice affects every PR. |
The user must confirm: (a) the ^4.1.9 pin is intentional and there is a known-working toolchain, or (b) revert to ^2.x (or whatever the latest stable is at the time). If (a), add a CI job to verify the install. If (b), update REQUIREMENTS R-25 to read "reverted to ^2.x". |
| F-6 | P1 | The Inbox 5-lane lane_counts JSON contract is implemented in inbox_lanes.py:1-280 but the shape is not specified in any spec. The frontend (src/components/...) presumably renders lane badges from this count. |
Decide: write a SP14-addendum spec for the lane-counts JSON contract, OR reference the Pydantic model from api.py as the contract and inline-link it from the spec. Either way, the contract should be testable from the spec. |
| F-7 | P2 | Score weights are hardcoded at backend/src/cyclone/scoring.py:42-46 as 40/25/20/15 (patient/date/amount/provider). REQUIREMENTS R-10 says these are configurable via config/payers.yaml. The config/payers.yaml file does not currently expose them. |
Decide: (a) move the weights into config/payers.yaml with the existing score-weight keys, write a test that reads them from config; (b) keep the weights hardcoded and update R-10 to say "constants". |
| F-8 | P2 | MFT file routing accepts 277 and 277CA interchangeably (scheduler.py:316-321). SP10 spec says HCPF sends bare 277. The scheduler's acceptance of both is defense-in-depth. |
Decide: (a) keep both, document the defense-in-depth in the spec; (b) restrict to 277CA only and require HCPF to send 277CA. (a) is the path of least disruption. |
| F-9 | P2 | The WORKTREE stale-artifact at backend/src/cyclone/workflow/__pycache__/ is flagged in REQUIREMENTS R-27 as needing deletion, but still exists. |
Decide: just delete it. This is mechanical. Add a .gitignore rule to prevent recurrence. |
| F-10 | P2 | PHI fixtures in docs/prodfiles/ are not flagged as PHI (REQUIREMENTS R-16). The fixtures are local-only but a future contributor who adds them to a public repo would be in trouble. |
Decide: (a) add a README.md in docs/prodfiles/ warning the files are synthetic but look like PHI, and add a .gitattributes rule; (b) replace the fixtures with obviously-fake data. |
| F-11 | P2 | The co_medicaid() factory in backend/src/cyclone/parsers/payer.py:57-58 exists as a fallback (REQUIREMENTS R-4, NFR-11). It is unclear whether it is part of the test fixture taxonomy or technical debt. |
Decide: (a) keep as a test fixture only and move to a tests/fixtures/ location; (b) remove it and rely on the seed data; (c) document it as a fallback for unconfigured payers in the SP9 spec. |
| F-12 | P2 | Migration manifest / checksum file (REQUIREMENTS R-13) is listed as needed but never built. There is no manifest.json or manifest.yaml next to the migrations directory. |
Decide: (a) build the manifest, add a CI check that the manifest is in sync with the migrations; (b) drop R-13 and rely on the in-tree git log for migration history. |
| F-13 | P3 | 277CA monotonic stamping (inbox_state_277ca.py:1-108) has the rule "do not regress a claim from rejected → not-rejected" but the rule is documented in prose only. |
Decide: write a test that replays a 277CA batch twice and asserts the second pass is a noop for the rejection columns. |
| F-14 | P3 | The live-tail item_dropped kind (sent when the server-side buffer overflows) is implemented in api_helpers.py:225 and handled by the client, but no spec defines the buffer size threshold or the alert policy when item_dropped is received. |
Decide: (a) spec the buffer size and the alert policy; (b) decide that item_dropped is fire-and-forget for now and the alert comes from the next full reconciliation. |
| F-15 | P3 | docs/README.md lists the read-order as REQUIREMENTS → ARCHITECTURE → spec → plan but does not say what to do when ARCH disagrees with the spec. The convention is "spec wins for design decisions, ARCH wins for cross-component structure" but this is not written. |
Decide: write a one-paragraph convention note in docs/README.md covering precedence when docs disagree. |
| F-16 | P3 | The audit-log API surface (GET /api/admin/audit-log/verify at api.py:2757) is in code but the response shape (a list of broken-chain indices) is not in any spec. |
Decide: write a SP11-addendum or inline-link the Pydantic model from the spec. |
6. Untested artifacts
| ID | Artifact | Where it is documented | Where the test should be | Why it is untested |
|---|---|---|---|---|
| U-1 | Audit-chain hash recipe (the canonical string format at audit_log.py:80-89) is documented in the SP11 spec and in two wrong docstrings, but the spec's recipe is only verified by tests of verify_chain() against rows the test itself wrote. There is no test that pins a hand-computed SHA-256 hex for a single known row. |
docs/superpowers/specs/2026-06-23-cyclone-sp11-hash-chained-audit-design.md:104-114. |
backend/tests/test_audit_log.py — add a test test_hash_recipe_is_stable_against_hand_computed_value. |
Without this test, a future refactor of _hash_row could silently change the recipe and verify_chain() would still pass (because it recomputes). |
| U-2 | The vitest@^4.1.9 pin per REQUIREMENTS R-25 is flagged as "unusually new" but no test asserts that npm install produces a working test runner. |
docs/REQUIREMENTS.md R-25. |
A CI workflow or a package.json engines field; there is no CI workflow file. |
The pin could break on a future npm install if vitest 4.x is unpublished. No fallback plan. |
| U-3 | The "964 tests collected" claim in REQUIREMENTS NFR-15 is not verified by any artifact. The reviewer did not count backend/tests/test_*.py files (this is a research gap, not a contradiction). |
docs/REQUIREMENTS.md NFR-15. |
A test-counting step in CI. | The number may be out of date by the time you read this. |
| U-4 | The Matches.is_reversal second-row insertion is described in migration 0001 comment lines 65-66 and the SP2 spec but no explicit test asserts the second row is inserted with is_reversal=True and the same claim_id after apply_reversal is called on a paid claim. |
Migration 0001, SP2 spec. | backend/tests/test_reconcile.py (assumed). |
The reviewer did not verify the test exists. Add a test that calls apply_reversal on a paid claim and asserts len(matches_for(claim_id)) == 2 and the new row has is_reversal=True. |
| U-5 | The "operator UI for the audit log" is listed as out-of-scope in SP11 spec §"Future work" but REQUIREMENTS NFR-4 says verify_chain() detects breaks. There is no test that asserts a manual verify_chain() call (e.g., from a CLI command) reports a break when one row is tampered. |
docs/REQUIREMENTS.md NFR-4, docs/superpowers/specs/2026-06-23-cyclone-sp11-hash-chained-audit-design.md. |
backend/tests/test_audit_log.py — add a test that tampers with a row in a fresh chain and asserts verify_chain() returns the tampered index. |
The reviewer assumes such a test exists in test_audit_log.py but did not verify. |
| U-6 | The backup-passphrase-missing degraded mode in backup_service.py falls back to deriving from SQLCipher key with a WARNING. REQUIREMENTS NFR-6 only asserts the Keychain-with-passphrase path. |
docs/REQUIREMENTS.md NFR-6, backend/src/cyclone/backup_service.py. |
backend/tests/test_backup_service.py — assert the WARNING is logged and the backup is still produced. |
The degraded mode is a code path that runs in production but has no test. |
| U-7 | The 5-lane Inbox lane_counts JSON shape is not specified in any spec. The frontend presumably depends on it. |
docs/superpowers/specs/2026-06-23-cyclone-sp14-inbox-5lane-design.md. |
A contract test (e.g., tests/test_inbox_lanes.py) that asserts the response shape. |
The shape is a contract between backend and frontend; no test pins it. |
| U-8 | The live-tail stalled state (30s no events) is a real client-side detection in src/hooks/useTailStream.ts. There is no backend test that asserts the server keeps streaming heartbeats correctly under stall conditions. |
docs/superpowers/specs/2026-06-20-cyclone-live-tail-design.md. |
backend/tests/test_api_helpers.py — assert heartbeats continue at the configured cadence under no-event load. |
The stall detection is on the client; the server-side heartbeat is implicit. |
| U-9 | The 277CA monotonic stamping (inbox_state_277ca.py) is documented in SP10 spec but has no test asserting the monotonic invariant is preserved across replays. |
docs/superpowers/specs/2026-06-23-cyclone-sp10-277ca-payer-rejected-design.md. |
backend/tests/test_inbox_state_277ca.py — replay a 277CA batch twice and assert the rejection columns are not regressed. |
The invariant is in prose only. |
| U-10 | The rejected claim state (set by 999 AK9 R/E at inbox_state.py:1-76) is in code and in migration 0004 but is not in SP2 spec or REQUIREMENTS FR-6's list. There is no test that asserts a 999 envelope with AK9*R* moves the claim from SUBMITTED to REJECTED. |
Migration 0004, backend/src/cyclone/inbox_state.py. |
backend/tests/test_inbox_state.py — assert 999 R/E transitions SUBMITTED → REJECTED. |
The transition is in code but not in the spec, and may not be in the tests. |
| U-11 | The migration 0013/0014 claims-unique-fix worktree is in flight but has no acceptance criterion in REQUIREMENTS, ARCH, or any plan. The "definition of done" is "merged into main" — not testable. |
None. | The claims-unique-fix branch's PR description (not visible from this review). |
The fork is being made without a published acceptance criterion. |
| U-12 | The co_medicaid() factory in backend/src/cyclone/parsers/payer.py:57-58 is not exercised by any test (assumed; not verified). |
docs/REQUIREMENTS.md R-4, NFR-11. |
backend/tests/test_parsers.py — add a test that the factory returns a valid PayerConfig. |
The factory is in code but is not in the test plan. |
7. Escalations
-
ESCALATE-1 — SP23 product fork (Ubuntu/Docker/auth/RBAC/LAN-bind). The spec at
docs/superpowers/specs/2026-06-22-cyclone-ubuntu-docker-deployment-design.mdexists but no plan exists. REQUIREMENTS R-1, R-3, R-24 raise the question but do not close it. Decision needed: is SP23 in-scope for the next release, or deferred? If in-scope, write a plan. If deferred, move the spec todocs/superpowers/deferred/. -
ESCALATE-2 — RECONCILED state disposition. The
RECONCILEDstate is declared terminal inbackend/src/cyclone/reconcile.py:163-164but never assigned by any code path. SP2 spec asserts it as the terminal step of everyapply_paymentpath. Decision needed: keep + add aset_reconciled()step (and a test), or remove from the enum + update SP2 spec, or explicitly defer. -
ESCALATE-3 — Audit-chain source of truth. The hash recipe is wrong in
audit_log.py:6-7,db.py:649-650, andARCHITECTURE.md§4.6. The code ataudit_log.py:80-89and the SP11 spec at…-sp11-hash-chained-audit-design.md:104-114agree. Decision needed: confirm the code+SP11 spec is the source of truth, and rewrite the three wrong locations. Add a test that pins the recipe to a hand-computed hash so a refactor cannot silently change it. -
ESCALATE-4 — vitest pin.
vitest@^4.1.9is flagged in REQUIREMENTS R-25 as "unusually new". Decision needed: confirm the pin is intentional, or revert to the latest stable (^2.xor whatever the current stable is). -
ESCALATE-5 — Backup degraded mode policy.
backup_service.pyhas a degraded mode when the encryption passphrase is missing. Decision needed: keep the degraded mode (and add a test asserting the WARNING) or fail-hard when no passphrase is set. -
ESCALATE-6 — Score weights configuration. Score weights are hardcoded at
scoring.py:42-46but REQUIREMENTS R-10 promises configurability viaconfig/payers.yaml. Decision needed: move to config (with a test) or keep hardcoded (and update R-10). -
ESCALATE-7 — SPEC traceability for 277CA payer_rejected. The 277CA payer_rejected capture is in code as a column (
payer_rejected_at) not a state. ARCH §6.3 has it as a state. REQUIREMENTS FR-13 mentions it as a lane. Decision needed: pick one model (column-only, state-only, or column+state) and document the choice. The current 3-way split is confusing. -
ESCALATE-8 — In-flight migrations 0013/0014 acceptance. Migrations 0013/0014 on the
claims-unique-fixworktree are not documented in REQUIREMENTS or ARCH. Decision needed: publish the acceptance criterion for the fix (e.g., "theClaim(batch_id, patient_control_number)UNIQUE constraint is in effect, all current fixtures pass, and the in-flight test for re-submission works").
8. Confidence rating
Confidence: medium-high.
Justification: I read the bulk of the backend implementation — api.py
(3,548 LOC, all route handlers grepped), db.py (839 LOC, all ORM
models and the ClaimState enum), audit_log.py (254 LOC, the
hash-chain code and its docstrings), api_helpers.py (heartbeat +
tail events), security.py (485 LOC, three middlewares), db_crypto.py
(389 LOC, key rotation), backup_service.py (851 LOC, restore flow),
reconcile.py (571 LOC, apply_payment and apply_reversal),
inbox_lanes.py (280 LOC, 5 lanes), scoring.py (96 LOC, weights),
inbox_state.py (76 LOC, 999 transitions), inbox_state_277ca.py (108
LOC, monotonic stamping), scheduler.py (719 LOC, MFT routing), and
all 12 shipped migrations. I read the frontend live-tail client
(src/lib/tail-stream.ts, src/hooks/useTailStream.ts) in full and
grepped the rest of src/{routes,store,components}/ for shape
mismatches. I read docs/REQUIREMENTS.md (737 lines), docs/ARCHITECTURE.md
(759 lines), and docs/README.md in full. I read 6 of the 22 SP specs
in full (SP2 reconciliation, SP5 live tail, SP10 277CA, SP11 audit,
SP14 5-lane Inbox, SP15 key rotation) and grepped the rest for
state-name and constraint assertions. I did not read the remaining
16 specs in full, did not read the 27 plan files in full, and did not
read any of the ~73 frontend test files. I also did not verify the
"964 tests collected" claim in NFR-15 nor the existence of
test_audit_log.py tests. The medium-high rating reflects strong
backend code coverage, full coverage of the top-of-funnel docs, and
partial coverage of the SP-spec lineage; the gaps that would lower
confidence to "high" are the un-verified plan files, the un-verified
test count, and the un-verified existence of the 6 backend test files
referenced in §6.
End of Reviewer B report. 13 contradictions, 16 forks (4 P0, 4 P1, 5 P2, 3 P3), 12 untested artifacts, 8 escalations. Confidence: medium-high.