External design review (2026-08-01) — archived with ground-truth corrections
An external principles-based design review of this repo at commit 3591271,
archived verbatim below, followed by the corrections we verified against the
code on 2026-08-02. Archived because its verified findings drove real changes
(versioned state records; the exact-money December pin; the on-touch
decomposition rules in TECH_DEBT.md) — and because the reject ledger applies
to inbound findings the same as to our own ideas.
How it was adjudicated: every checkable claim was ground-truthed before acting. Two of five findings had factual errors in their evidence or consequence chains; both corrections are recorded in the appendix, and the process feedback we sent the reviewer is summarized at the end.
The review, verbatim
{
"review": {
"repo": "/Users/dan/Documents/GitHub/thales",
"commit": "3591271",
"reviewed_at": "2026-08-01",
"database_principles": 250,
"canon_commit": "6bd42da"
},
"scan": {
"code_files": 235, "loc": 53549, "primary_language": "python",
"excluded": [".claude/worktrees (7507 files, ~557k loc — agent session copies of the repo)"],
"scan_looks_sound": true
},
"findings": [
{"rank": 1, "principle": "decompose-by-likely-change", "score": 0.97},
{"rank": 2, "principle": "separate-logic-from-io", "score": 0.94},
{"rank": 3, "principle": "avoid-primitive-obsession", "score": null, "source": "read"},
{"rank": 4, "principle": "versioned-data-formats", "score": 0.91},
{"rank": 5, "principle": "design-for-transparency", "score": 0.90}
],
"dropped": [
{"principle": "bound-retry-load", "was_score": 0.94, "why": "known-bad metric"},
{"principle": "prefer-pure-functions", "was_score": 0.95, "why": "fires broadly at this size"}
],
"not_checked": ["concurrency", "statistical-validity"]
}
- decompose-by-likely-change — cli.py is 5,247 lines; "longest function 352 lines". Remedy: give each decision a module.
- separate-logic-from-io — daily.py (1,784 loc) and diagnostics.py interleave computation with fetching/writing.
- avoid-primitive-obsession (found by reading) — no
Decimalanywhere; "for order sizing, spend against a cap, and P&L reconciliation against broker truth [float] is the wrong representation — reconciliation is exactly where a cent of drift surfaces as a spurious divergence and trips your fail-closed HALT." Reviewer's own priority pick. - versioned-data-formats — JSONL state and backups lack version fields; readers not audited.
- design-for-transparency — observability is command-shaped, not emission-shaped.
Reviewer's notes (their words, condensed): ranking is detection strength, not priority; finding 3 is the act-first item; whether float drift is real "logs/ would show it"; cli.py may be mostly declarative ("I did not measure the split"); statistical validity is outside a design review's competence — "don't read a clean review as reassurance there."
Appendix — ground-truth corrections (2026-08-02)
- Finding 3's consequence chain is structurally absent today. reconcile diffs order-ID sets and equity-DATE presence, never monetary values (the standing M3 limitation) — a cent of float drift is invisible to it. And a reconcile divergence reds the run and alerts; the fail-closed HALT is a separate subsystem (execution/safety.py) it does not touch. The reviewer's own suggested empirical check, run: zero spurious-divergence incidents in the logs, ever. The PROSPECTIVE kernel is accepted and pinned as December package item (d): exact-money boundary + value-level reconcile, before the first real dollar.
- Finding 1's numbers: longest function is 199 lines (ic_triage), not 352; the file is ~18% declarative (203 Typer lines + 745 docstring lines of 5,247), so the reviewer's bikebot-style doubt does not apply. The finding survives on evidence the reviewer lacked: three parallel branches merge-conflicted in cli.py on 2026-08-01 — decompose-by-likely-change manifesting operationally. Accepted as an on-touch rule.
- Finding 4 accepted and DONE (SCHEMA_V stamping + tolerant readers, 2026-08-02) — and the rollout immediately caught TCARecord(**row) crashing on the first unknown key: the reader-fragility the finding predicted, proven and fixed in the same commit.
- Finding 5 substantially stale vs the week of 2026-07-27..08-01: run-summary records with state lineage, per-run decision telemetry (A2), verdict ledgers and the beacon/alert layer are emission-shaped observability. Remainder folds into NORTHSTAR gauge G2.
- Finding 2 accepted directionally — the null-close crash (2026-07-30) lived exactly in daily.py's interleaved zone — as an on-touch rule; the highest-value extraction (construct.py + the parity harness) predates the review.
- The statistical-validity disclaimer, inverted here: the area the review names as its blind spot is where this repo has the freshest receipts — PBO null-calibrated against 24 no-skill books and the purge leak measured and fixed, both on 2026-08-01 (C2/C4 memos).
Process feedback sent to the reviewer (summary): verify consequence chains to their sinks, not just premises; run the two-minute checks you name; read critical money/state paths regardless of scorer output; add change-history signals (churn, merge-conflict frequency); calibrate recall with planted-defect repos, not only precision via dropped findings; emit priority as a first-class field with its arithmetic.