Thales — design review against first principles

Second review at commit cd571b2. The first (thales-design-review-2026-09-05-cd571b2.md, same day, same commit) could not spawn an advocate, so none of its findings had ever been contested. This review's purpose was that exchange. It changed the report materially: the top finding here came from the advocate, and the previous review's headline number was wrong by roughly 10× — in the repository's favour.

{
  "review": {
    "repo": "/Users/dan/Documents/GitHub/thales",
    "commit": "cd571b2837a0e7baa0aad81ecfd8ec29ae65cfd8",
    "reviewed_at": "2026-09-05",
    "database_principles": 250,
    "canon_commit": "a81d17627af7281611568525cf2508a94bb97f9c"
  },
  "scan": {
    "code_files": 249,
    "loc": 58344,
    "primary_language": "python",
    "excluded": ["web/.next (~1.8 GB) and web/node_modules (~634 MB): gitignored build trees present on disk. The step-3e copy holds tracked files only (729 files, 189 MB). The unfiltered match was ALSO run against the full tree and reconciled — see tool_warnings."],
    "scan_looks_sound": true,
    "note": "Scanner reports 249 code files; `git ls-files | grep -cE '\\.(py|ts|tsx|js|sh)$'` gives 246. The 3-file gap is scanner-counted extensions outside that set. Reconciled. Filtered and unfiltered rankings differ by exactly one principle (reproducible-builds, present only unfiltered because the copy has no .git) — explainable, so no subset was silently reviewed."
  },
  "detection_rank": [
    "prefer-pure-functions",
    "bound-retry-load",
    "separate-logic-from-io",
    "weigh-dependency-cost-before-adding",
    "match-rigour-to-lifespan",
    "eliminate-unknown-unknowns",
    "narrow-try-scope",
    "prefer-fakes-over-mocks",
    "prevent-backsliding",
    "delete-obsolete-code",
    "test-failure-paths",
    "make-intent-explicit",
    "enforce-invariants-in-one-place",
    "design-by-contract",
    "clarity-over-cleverness"
  ],
  "priority_rank": [
    "handle-all-error-outcomes",
    "mutate-state-only-after-success",
    "prefer-fakes-over-mocks",
    "enforce-invariants-in-one-place",
    "protect-shared-mutable-state",
    "retire-stale-docs",
    "least-privilege"
  ],
  "priority_rank_note": "Detection and priority share only two ids. The top four detections carry prior_disproof against this exact commit and died again; the two highest-priority findings were found by reading and by the advocate and have no score at all. Seven ids are prioritised rather than five because the last two (retire-stale-docs, least-privilege) are single-file, certain, sub-hour fixes that cost the reader nothing to carry.",
  "findings": [
    {
      "rank": null,
      "priority": 1,
      "principle": "handle-all-error-outcomes",
      "gap": null,
      "score": null,
      "source": "read",
      "coordinates": "reviewer",
      "verification": "consequence-verified",
      "p_manifests": 0.5,
      "sink": "src/thales/execution/daily.py:1252; src/thales/execution/daily.py:1190; src/thales/cli.py:3696; src/thales/cli.py:1137",
      "evidence": "src/thales/execution/daily.py:1236; src/thales/execution/daily.py:1190; src/thales/cli.py:3641; src/thales/cli.py:3696; src/thales/cli.py:4405; src/thales/execution/alpaca_broker.py:421-427; src/thales/utils/notifications.py:74-76; src/thales/execution/simulated.py:195-200; src/thales/execution/broker.py:125-127; src/thales/cli.py:1130-1138",
      "damage_reach": "system",
      "apply_risk": "local",
      "mechanical": true,
      "prerequisite": null,
      "confidence": "high",
      "priority_basis": "P~0.5 (the precondition fired 546 times in 33 meanrev sessions and the guard was inert throughout; the residual uncertainty is the broker's cancel-refusal rate, which I cannot measure without broker logs) x cost=up-to-2x intended quantity at the ONE submit site outside the safety gate, plus a mid-run halt that does not actually halt x diff-size=one `if` per site, five sites. Highest product in the report by a wide margin.",
      "contested": "reinstated",
      "challenge": "The review never found this at all; the repository's advocate produced it, and my own draft in this area (idempotent-operations) was aimed at the wrong mechanism. In round 2 the advocate pressed that it should outrank finding 2, and produced the frequency evidence that decided it.",
      "held_on": "Reproduced by execution in the copy with a real SafetyLimits: cancel RAISES -> no replacement; cancel returns False (what AlpacaBroker actually does) -> submit_market_order called with ('VAL', 5.0, 'buy', client_order_id='cid-9-mr'), the FULL original quantity. Round 2 added four more sites and the exposure count, which I re-derived: 546 cancel-replacements on 15 of 33 meanrev trading days, including 222/222 orders on 2026-08-28 and 251/252 on 2026-08-31."
    },
    {
      "rank": null,
      "priority": 2,
      "principle": "mutate-state-only-after-success",
      "gap": null,
      "score": null,
      "source": "read",
      "coordinates": "reviewer",
      "verification": "consequence-verified",
      "p_manifests": 0.25,
      "sink": "src/thales/portfolio/kelly.py:81-88; src/thales/execution/daily.py:720-721",
      "evidence": "src/thales/execution/daily.py:1130; src/thales/execution/daily.py:1133; src/thales/execution/daily.py:1421-1423; src/thales/execution/daily.py:929; src/thales/execution/daily.py:1024-1026; src/thales/execution/daily.py:665-667; src/thales/backtest/engine.py:711; src/thales/backtest/engine.py:936",
      "damage_reach": "system",
      "apply_risk": "module",
      "mechanical": false,
      "prerequisite": null,
      "confidence": "high",
      "priority_basis": "P~0.25 (kill switch has never fired; equity -8.1% into a -20% trigger) x cost=187-848% over-sizing on re-entry, plus a vol estimator that drops the highest-vol days x diff-size=reorder two writes and add one booking call. Ranked below finding 1 on frequency, not on severity.",
      "contested": "held",
      "challenge": "All four Kelly figures were computed on the raw 5000-row ledger file; production truncates to max_pool = kelly.rolling_window_months(24) x construction.momentum_sleeve_size(50) = 1200 (construct.py:129-141, applied at kelly.py:91-92).",
      "held_on": "Re-priced through the production entry point build_target_weights with config/settings.yaml: as-is 0.10890; booking -20% -> 0.03796 (187% larger); -30% -> 0.01148 (848% larger); -40% -> 0.0, the fail-closed zero-deployment branch at kelly.py:117-124. The finding survives ~10x STRONGER than drafted. The advocate additionally showed a second sink I verified: daily.py:1024-1026 persists kill_switch active BEFORE the liquidation is attempted, daily.py:665-667 stamps it on every later snapshot, and daily.py:720-721 drops those days from the realized-vol sample while the book may still be fully invested."
    },
    {
      "rank": 8,
      "priority": 3,
      "principle": "prefer-fakes-over-mocks",
      "gap": null,
      "score": 0.87,
      "source": "matched",
      "coordinates": "reviewer",
      "verification": "consequence-verified",
      "p_manifests": 0.3,
      "sink": "tests/test_execution/test_daily.py:576",
      "evidence": "tests/test_execution/test_daily.py:569; src/thales/execution/simulated.py:195-200; src/thales/execution/alpaca_broker.py:421-427",
      "damage_reach": "module",
      "apply_risk": "local",
      "mechanical": false,
      "prerequisite": "handle-all-error-outcomes",
      "confidence": "high",
      "priority_basis": "P~0.3 (this is the mechanism by which finding 1 reached production and stayed) x cost=a guard that reads as tested and is inert x diff-size=swap MagicMock for the SimulatedBroker the repo already owns. Ranked here because it is finding 1's cause, not a separate defect.",
      "contested": "reinstated",
      "challenge": "The signal fired at rank 8 with score 0.87 and the previous review never examined the ranking below rank 5; I would not have opened this file either had the advocate not produced finding 1.",
      "held_on": "SimulatedBroker.cancel_order('unknown-id') returns False, as does AlpacaBroker and the base contract; the test at test_daily.py:569 configures side_effect=RuntimeError. No Broker in this repository raises, so the assert_not_called() at :576 passes for a reason unrelated to production."
    },
    {
      "rank": 13,
      "priority": 4,
      "principle": "enforce-invariants-in-one-place",
      "gap": null,
      "score": 0.855,
      "source": "matched",
      "coordinates": "reviewer",
      "verification": "consequence-verified",
      "p_manifests": 0.4,
      "sink": "src/thales/data/options_skew.py:316-344",
      "evidence": "data/options_skew.jsonl: 2068 rows dated 2026-06-19, 2026-07-03, 2026-08-29; src/thales/cli.py:4592-4598",
      "damage_reach": "module",
      "apply_risk": "local",
      "mechanical": true,
      "prerequisite": null,
      "confidence": "high",
      "priority_basis": "P~0.4 (the bad rows already exist; it manifests whenever a study groups the stream by date) x cost=phantom sessions carrying drifted stale quotes in the moat dataset x diff-size=one filter on the read path plus a quarantine pass. Certain premise, moderate consequence.",
      "contested": "reinstated",
      "challenge": "The review swept data integrity via the writer and the append path and concluded 'handled well'; it never checked whether rows already in the store satisfy the invariant the writer now enforces.",
      "held_on": "Re-derived independently against thales.utils.calendar.get_trading_days: [('2026-06-19', 892), ('2026-07-03', 283), ('2026-08-29', 893)] = 2068 of 50438 rows, and 2026-08-29 is a Saturday. The writer was fixed at cli.py:4592-4598; no reader filters."
    },
    {
      "rank": null,
      "priority": 5,
      "principle": "protect-shared-mutable-state",
      "gap": null,
      "score": null,
      "source": "read",
      "coordinates": "reviewer",
      "verification": "consequence-verified",
      "p_manifests": 0.15,
      "sink": ".github/workflows/skew-snapshot.yml:88-93",
      "evidence": ".github/workflows/skew-snapshot.yml (no concurrency block); ops/launchd/com.thales.dispatch-skew.plist:27-30; src/thales/data/options_skew.py:401-412",
      "damage_reach": "module",
      "apply_risk": "local",
      "mechanical": true,
      "prerequisite": null,
      "confidence": "high",
      "priority_basis": "P~0.15 (dual-fire is real under PDT but the rebase conflicts loudly) x cost=one permanently lost capture day plus a red build x diff-size=four lines already written three times in this repo.",
      "contested": "narrowed",
      "challenge": "The drafted consequence was interleaved JSONL corruption; the advocate reproduced the exact persist tail with two clones and got 'CONFLICT (both add at EOF)' then 'fatal: You are not currently on a branch' x3, final exit 128.",
      "held_on": "The surviving claim: skew-snapshot.yml is the only SCHEDULED state-writing workflow without a concurrency group, and under PDT its launchd dispatch and its cron target the same minute, so the loser silently loses a permanent capture day and reds the build — not data corruption, and only ~8 months of the year."
    },
    {
      "rank": null,
      "priority": 6,
      "principle": "retire-stale-docs",
      "gap": null,
      "score": null,
      "source": "read",
      "coordinates": "reviewer",
      "verification": "consequence-verified",
      "p_manifests": 1.0,
      "sink": "research/queue/open/skw-1-skew-client-no-deadline.md",
      "evidence": "src/thales/data/options_skew.py:388; src/thales/data/options_skew.py:398; src/thales/execution/alpaca_broker.py:102; src/thales/execution/alpaca_broker.py:352; src/thales/execution/vrp_daily.py:165",
      "damage_reach": "local",
      "apply_risk": "local",
      "mechanical": true,
      "prerequisite": null,
      "confidence": "high",
      "priority_basis": "P=1.0 (the document is wrong right now) x cost=a reader plans work that already shipped x diff-size=one `git mv`. Cheapest item in the report; carried because certainty is 1.",
      "contested": "unchallenged",
      "challenge": null,
      "held_on": null
    },
    {
      "rank": null,
      "priority": 7,
      "principle": "least-privilege",
      "gap": null,
      "score": null,
      "source": "read",
      "coordinates": "reviewer",
      "verification": "consequence-verified",
      "p_manifests": 0.05,
      "sink": "web/app/api/subscribe/route.ts:59-74",
      "evidence": "web/app/api/subscribe/route.ts:16-23; web/app/api/subscribe/route.ts:49-50; .github/workflows/_alert.yml:53-54; .github/workflows/routine-outbox.yml:62-63",
      "damage_reach": "module",
      "apply_risk": "local",
      "mechanical": true,
      "prerequisite": null,
      "confidence": "medium",
      "priority_basis": "P~0.05 (needs deliberate abuse of a low-traffic form) x cost=degraded alerting for a PAPER trading system x diff-size=a separate credential. Demoted from the previous review's priority 3 on the advocate's evidence.",
      "contested": "narrowed",
      "challenge": "'alerting for a live trading system degrades' overstates it: all three sleeves are paper: true (config/settings.yaml:243, config/meanrev.yaml:141, config/vrp.yaml:110) and RUNBOOK.md:60-61 says there are no live credentials anywhere.",
      "held_on": "The surviving claim: one Gmail app password is shared between seven workflows and a public unauthenticated POST, and the public endpoint is the only holder anonymous users can reach. Blast radius is a paper account's alerting, not a live book."
    },
    {
      "rank": null,
      "priority": null,
      "principle": "idempotent-operations",
      "gap": null,
      "score": null,
      "source": "read",
      "coordinates": "reviewer",
      "verification": "consequence-disproved",
      "p_manifests": 0.02,
      "sink": null,
      "evidence": "src/thales/execution/pipeline.py:362-375; src/thales/execution/pipeline.py:202-217; src/thales/execution/pipeline.py:181-186",
      "damage_reach": "local",
      "apply_risk": "local",
      "mechanical": false,
      "prerequisite": null,
      "confidence": "high",
      "priority_basis": "Chain traced and dead-ended. Kept at priority null so the next review does not re-derive it from the same reading.",
      "contested": "conceded",
      "challenge": "Quantity is recomputed from live broker truth every run (daily.py:1409-1412 -> pipeline.py:114-150), so a filled sell yields diff=0 and no order at all; a different quantity SHOULD get a different key, which is what pipeline.py:363-371 says the qty component is for. Within one retry envelope the key is stable — nothing in _submit_with_retry mutates order['shares'].",
      "held_on": null
    },
    {
      "rank": null,
      "priority": null,
      "principle": null,
      "gap": "No principle covers BACKTEST/LIVE PARITY: the invariant that a live execution path must reproduce the decisions of the validated simulation it was signed off against. Finding 2 is filed under mutate-state-only-after-success, which names the mechanism (state published before the fallible call) but not the invariant that makes it severe here.",
      "score": null,
      "source": "read",
      "coordinates": "reviewer",
      "verification": "consequence-verified",
      "p_manifests": 0.25,
      "sink": "src/thales/portfolio/kelly.py:81-88",
      "evidence": "src/thales/backtest/engine.py:711; src/thales/backtest/engine.py:936; src/thales/execution/daily.py:929",
      "damage_reach": "system",
      "apply_risk": "module",
      "mechanical": false,
      "prerequisite": null,
      "confidence": "high",
      "priority_basis": "Not separately prioritised: the actionable work is finding 2. Recorded so the gap is rankable by `canon gaps` rather than lost in prose.",
      "contested": "unchallenged",
      "challenge": null,
      "held_on": null
    },
    {
      "rank": null,
      "priority": null,
      "principle": null,
      "gap": "No principle covers PREFERRING OBSERVED STATE OVER RECORDED INTENT: when a fact is both persisted as intent and observable directly in the same scope, deriving behaviour from the intent flag silently diverges whenever the intended action did not happen. `canon show prefer-observed-state-over-recorded-intent` -> no such principle; nothing in the top 6 of a targeted search fits. The closest ids (make-intent-explicit, design-for-transparency) are about different concerns.",
      "score": null,
      "source": "read",
      "coordinates": "reviewer",
      "verification": "consequence-verified",
      "p_manifests": 0.2,
      "sink": "src/thales/execution/daily.py:720-721",
      "evidence": "src/thales/execution/daily.py:665-667; src/thales/execution/daily.py:648-649; src/thales/execution/daily.py:720-721",
      "damage_reach": "system",
      "apply_risk": "local",
      "mechanical": true,
      "prerequisite": null,
      "confidence": "medium",
      "priority_basis": "Would rank about 3rd if it had a record: modelled x1.77 vol-target scalar inflation applied on the re-entry run, one-line fix. Absent from priority_rank because the contract admits ids only.",
      "contested": "reinstated",
      "challenge": "I had folded this into finding 2 as a 'second sink'; the advocate showed in round 2 that it is a distinct mechanism with a distinct fix, and that the leg I HAD credited to it (the snapshot wipe) is already owned by the repo's RWG-1 row.",
      "held_on": "daily.py:665-667 sets snap['kill_switch'] from persisted intent (read_kill_switch_state().active) when the truth is in the same dict eleven lines earlier: snap['long_mkt_value'] at daily.py:648-649. Verified the exclusion at daily.py:720-721 consumes it. Distinct from SAC-1's 'member four', which is the vol_scalar divisor at daily.py:714-719."
    },
    {
      "rank": null,
      "priority": null,
      "principle": null,
      "gap": "No principle covers AUTHORIZATION: which identity may read or write what, and what a public entry point returns to a caller supplying no credentials. Swept by hand here and found clean, but nothing in the database would have prompted the sweep.",
      "score": null,
      "source": "read",
      "coordinates": "reviewer",
      "verification": "premise-verified",
      "p_manifests": 0.0,
      "sink": null,
      "evidence": "web/app/api/subscribe/route.ts:25; web/app/api/[transport]/route.ts (GET/POST/DELETE, all read-only tools)",
      "damage_reach": "local",
      "apply_risk": "local",
      "mechanical": false,
      "prerequisite": null,
      "confidence": "high",
      "priority_basis": "No finding — the dimension is clean here. Recorded only so the database gap is rankable.",
      "contested": "unchallenged",
      "challenge": null,
      "held_on": null
    },
    {
      "rank": null,
      "priority": null,
      "principle": null,
      "gap": "No principle covers COST PER REQUEST: what a single unauthenticated request can make the system spend. least-privilege fits the shared credential in finding 7 but not the unauthenticated-spend shape, which is the part that is actually reachable by an anonymous caller.",
      "score": null,
      "source": "read",
      "coordinates": "reviewer",
      "verification": "consequence-verified",
      "p_manifests": 0.05,
      "sink": "web/app/api/subscribe/route.ts:59-74",
      "evidence": "web/app/api/subscribe/route.ts:16-23",
      "damage_reach": "module",
      "apply_risk": "local",
      "mechanical": true,
      "prerequisite": null,
      "confidence": "medium",
      "priority_basis": "Not separately prioritised: the actionable work is finding 7. Recorded so the gap is rankable.",
      "contested": "unchallenged",
      "challenge": null,
      "held_on": null
    }
  ],
  "exchange": {
    "rounds": 2,
    "advocate_findings_adopted": [
      {"what": "The cancel-failure guard at the one submit site outside the safety gate is inert: AlpacaBroker.cancel_order returns False rather than raising, the return is discarded, and an unconfirmed cancel is followed by a full-size market replacement", "where": "src/thales/execution/daily.py:1236", "became": "handle-all-error-outcomes"},
      {"what": "A run in which every de-risking sell failed prints 'Orders FAILED: N' and then exits 0 green", "where": "src/thales/cli.py:1130-1138", "became": "handle-all-error-outcomes"},
      {"what": "Finding 1's magnitude was computed on the raw ledger instead of the production 1200-row window, understating it by ~10x", "where": "src/thales/portfolio/construct.py:129-141", "became": "mutate-state-only-after-success"},
      {"what": "Kill-switch state is persisted before the liquidation is attempted, and those days are then dropped from the realized-vol sample while the book may still be fully invested", "where": "src/thales/execution/daily.py:1024-1026; src/thales/execution/daily.py:720-721", "became": "mutate-state-only-after-success"},
      {"what": "2068 rows in the moat dataset are stamped on non-sessions (including a Saturday); the writer was fixed but the rows were never quarantined and no reader filters", "where": "data/options_skew.jsonl; src/thales/data/options_skew.py:316-344", "became": "enforce-invariants-in-one-place"},
      {"what": "The RUNBOOK's post-halt recovery instruction is a silent green no-op, because _record_safety_event writes a run_summary that already_ran_today then honours", "where": "src/thales/execution/pipeline.py:274; src/thales/execution/state.py:180-187; RUNBOOK.md:253-254", "became": "prose — see Near misses"},
      {"what": "TCA books a partially-filled order's entire quantity at the market-replacement price, feeding a pre-registered decision", "where": "src/thales/execution/daily.py:1748-1751", "became": "prose — see Near misses"},
      {"what": "An existing test is not evidence a guard works: the cancel-failure guard is pinned by a mock that raises, and no Broker in the repository raises — grep the concrete implementations for the failure signal before crediting the guard", "where": "tests/test_execution/test_daily.py:569", "became": "prefer-fakes-over-mocks"},
      {"what": "A SECOND site of the top finding: the mid-run halt cancels every unfilled limit with the same discarded bool, so a halt engaged mid-run does not stop the resting DAY limits", "where": "src/thales/execution/daily.py:1190", "became": "handle-all-error-outcomes"},
      {"what": "Three send_email call sites discard both error signals, including the NO-RUN escalation whose own comment says the alert channel must never eat the exit code", "where": "src/thales/cli.py:3641; src/thales/cli.py:3696; src/thales/cli.py:4405", "became": "handle-all-error-outcomes"},
      {"what": "The binding go-live TCA criterion is at 47.3 bps against a 50 bps threshold — a 5.4% margin — and go_live.py pools every fill since inception with no rolling window, so measurement error there is permanent and cumulative", "where": "src/thales/execution/go_live.py:309-314; config/settings.yaml:363", "became": "prose — see Near misses"},
      {"what": "Per-name capture coverage is unmeasured: GHC and NVR have contributed zero rows in 62 capture days and are outside SKW-2's dash-class scope; MASI 1/62, ASGN and HOLX 18/62, PSTG 19/62. capture_qa only tests total row count vs trailing median, and one absent name is 0.1% of that", "where": "src/thales/data/captures.py:610-629", "became": "prose — see Near misses"},
      {"what": "The vol-sample exclusion is driven by recorded intent when observed truth is in the same dict — a distinct mechanism from finding 2, with a modelled x1.77 scalar inflation", "where": "src/thales/execution/daily.py:665-667", "became": "gap finding 9 (no principle exists)"}
    ],
    "conceded": [
      {"principle": "idempotent-operations", "why": "Quantity is recomputed from broker truth each run, so a divergent key accompanies a genuinely divergent intent; and within one retry envelope the key is provably stable."},
      {"principle": "protect-shared-mutable-state", "why": "The advocate ran the actual persist tail: concurrent appends conflict in git rebase and exit 128 rather than interleaving. The zero-duplicate count was the tell and the draft recorded it as counter-evidence instead of letting it overturn the mechanism."},
      {"principle": "least-privilege", "why": "Every sleeve is paper: true and the RUNBOOK states there are no live credentials, so 'live trading system' was vocabulary inflation."}
    ],
    "held": [
      {"principle": "mutate-state-only-after-success", "challenge": "All four Kelly magnitudes were wrong (computed on 5000 rows, production uses 1200)", "held_on": "Re-priced through build_target_weights with production config: 187% / 848% / fail-closed-zero, i.e. the finding is ~10x stronger than drafted, and the advocate conceded mechanism, chain, principle mapping and priority."},
      {"principle": "handle-all-error-outcomes", "challenge": "Whether the 848% magnitude of finding 2 should outrank this finding's higher frequency", "held_on": "Held at priority 1 on frequency: marketable_limit is live daily on 2 of 3 sleeves (config/settings.yaml:267, config/meanrev.yaml:153) whereas the kill switch has never fired (kill_switch_state.json active=false, -8.1% into a -20% trigger)."},
      {"principle": "protect-shared-mutable-state", "challenge": "The drafted consequence was interleaved JSONL corruption; the advocate reproduced the persist tail at skew-snapshot.yml:88-93 with two clones and got CONFLICT (both add at EOF) then 'fatal: You are not currently on a branch' x3, final exit 128 — so the merge conflicts loudly rather than interleaving.", "held_on": "Narrowed, and the narrowed claim survives: skew-snapshot.yml is still the only SCHEDULED state-writing workflow with no concurrency group, and its launchd dispatch (plist:27-30, no TimeZone key) collides with its 19:00 UTC cron under PDT — so the loser permanently loses a capture day and reds the build. Seasonal, ~8 months a year, not 12."},
      {"principle": "least-privilege", "challenge": "'the alerting layer of a live trading system' is vocabulary inflation: every sleeve is paper: true (config/settings.yaml:243, config/meanrev.yaml:141, config/vrp.yaml:110) and RUNBOOK.md:60-61 states there are no live credentials anywhere.", "held_on": "Narrowed, and the narrowed claim survives: one Gmail app password is still shared between seven workflows and an unauthenticated public POST, and that endpoint is the only holder anonymous users can reach. The advocate additionally strengthened the backstop argument — _alert.yml:97-100 wraps login/send_message in no try, so an SMTP quota rejection reds the job rather than passing silently."}
    ],
    "unresolved": [
      {"claim": "The dual of the inert-guard sweep: for every MagicMock attribute given an exception side_effect in the test suite, check whether any concrete implementation of that method can raise. Every hit is a guard whose test is testing the mock.", "why_open": "Proposed and agreed in round 2, not built. On the evidence of tests/test_execution/test_daily.py:569 it is worth about an hour. Route: canon maintainers, or this repo's own test suite."},
      {"claim": "Whether the 47.3 bps TCA figure is itself biased by the finding-1 replacement path, and by how much", "why_open": "The near-miss mechanism over-states |slippage_bps| (the replacement is by construction the worse fill), so the true median is below 47.3 and the gate is less fragile than it looks — but neither the advocate nor I quantified the correction. Route: the maintainer's TCA gate review before the December go-live decision."}
    ]
  },
  "dimensions_swept": [
    {"dimension": "concurrency", "verdict": "checked - finding",
     "basis": "read all 7 state-writing workflows for `concurrency:` groups vs their git commit/push counts; read ops/launchd/com.thales.dispatch-skew.plist; grep for threading/multiprocessing across src (only backtest/cpcv.py:329). Finding 5."},
    {"dimension": "data integrity", "verdict": "checked - finding",
     "basis": "read state.py _append_jsonl (fsync + PIPE_BUF reasoning) and write_kelly_ledger (atomic_write_lines); then cross-checked STORED rows against get_trading_days -> 2068 non-session rows. Finding 4."},
    {"dimension": "numeric representation", "verdict": "checked - clean",
     "basis": "`grep -rl Decimal src/thales` -> 0 files; all money is float. Judged acceptable because the broker is the source of truth for equity and positions (safety.py:1-20) and no arithmetic settles cash; the reconciler compares order-ID sets and dates, not monetary values."},
    {"dimension": "security posture", "verdict": "checked - handled well",
     "basis": "grepped every tracked file for hardcoded credential patterns -> one hit, a FAKE key inside a test of the redaction scrubber (tests/test_execution/test_public_export.py:281). `git ls-files --error-unmatch web/.env.local` -> not tracked; `git check-ignore -v` -> web/.gitignore:34. A committed-beacon-URL test exists and I reproduced its assertion against the real repo by hand: zero offenders."},
    {"dimension": "external side effects", "verdict": "checked - finding",
     "basis": "traced every submission site: `grep -rn execute_orders src/thales` -> 3 callers, all via the chokepoint, plus the documented cancel-replace at daily.py:1174-1179. Findings 1 and 5."},
    {"dimension": "failure across boundaries", "verdict": "detector lied",
     "basis": "scan reports remote_calls_without_timeout=5; I opened all five and all five are false positives (see Tool defects). Hand-swept instead: all 9 requests/urlopen sites carry timeout=, and all 4 Alpaca SDK clients carry _apply_default_timeout (alpaca_broker.py:102,:352, vrp_daily.py:165, options_skew.py:398). My own first grep missed the SDK-client shape entirely and I only caught it by reading the repo's own queue."},
    {"dimension": "operability", "verdict": "checked - finding",
     "basis": "read cli.py:1105-1138 exit paths; `grep -rn failed_order_streak src/thales` -> 0 hits, confirming the repo's own open FOS-1 row. Finding 1's second site."},
    {"dimension": "authorization", "verdict": "checked - clean",
     "basis": "enumerated every route under web/app/api -> exactly 2. Read both: subscribe/route.ts is an unauthenticated POST that only mails the operator (finding 7); [transport]/route.ts exports GET/POST/DELETE over mcp-handler but every tool is read-only over already-public JSON with no trading or advisory tool. No endpoint mutates system state. Canon has no authorization principle — swept by hand."},
    {"dimension": "identity & equality", "verdict": "checked - handled well",
     "basis": "read pipeline.py:362-375 _client_order_id and executed it on boundary inputs; re-derived (date,symbol) uniqueness over data/options_skew.jsonl -> 50438 rows, 50438 distinct keys, 0 duplicates. See the demoted idempotent-operations finding for the one instability I found and then disproved."},
    {"dimension": "determinism", "verdict": "checked - clean",
     "basis": "read backtest/cpcv.py:320-360: combos come from itertools.combinations and mp.Pool.map preserves order; `grep -n 'seed\\|np.random' src/thales/backtest/cpcv.py` -> no hits, so no unseeded RNG. Silent path-dropping measured across 172 stored results — see Verified but demoted."},
    {"dimension": "published artifacts", "verdict": "checked - handled well",
     "basis": "traced web/public/data/*.json and llms.txt to a single writer, execution/public_export.py:367-485; nothing restates them by hand. The one misstatement found is in a research artifact, not the site — see Verified but demoted."},
    {"dimension": "cost per request", "verdict": "checked - finding",
     "basis": "read the only unauthenticated write-ish endpoint, web/app/api/subscribe/route.ts:16-23: an in-process Map limiter, 5/60s, keyed on spoofable x-forwarded-for, per serverless instance. Finding 7. Canon has no cost-per-request principle — swept by hand."},
    {"dimension": "backtest/live parity", "verdict": "checked - finding", "added": true,
     "basis": "compared the live kill-switch path (daily.py:1421-1485) against the validated engine (engine.py:711, :936, :650-670). Finding 2. This is the invariant this repository spends the most effort defending and canon has no principle naming it."},
    {"dimension": "defect-tracker accuracy", "verdict": "checked - finding", "added": true,
     "basis": "swept all 11 rows in research/queue/open/ against HEAD. SKW-1's fix shipped in 29c6f23; MEM-1 (memory/ absent), SKW-2 (no dash-class symbol ever captured), FOS-1 and RPL-1 confirmed genuinely open. Finding 6."},
    {"dimension": "statistical validity", "verdict": "not examined", "added": true,
     "basis": "Routed, not skipped: the repo's own C2/C4 memos, null-calibration suite (25 stored runs) and monthly-revalidation.yml verify PBO/CPCV/DSR far better than a design review can. See not_checked."}
  ],
  "dropped": [
    {"principle": "prefer-pure-functions", "was_score": 0.942, "why": "Carries prior_disproof against thales@cd571b28 and dies again for the same reason: it fires on size at ~58k LOC, not on evidence. Confirmed the fetch-vs-pure split still exists rather than re-deriving the disproof. Third consecutive review to drop it."},
    {"principle": "bound-retry-load", "was_score": 0.94, "why": "Carries prior_disproof against thales@cd571b28. retry_sites_without_backoff=14 is a metric artefact: the retry sites implement bounded exponential backoff (pipeline.py:377-451, downloader.py:36-49). Third consecutive review to drop it."},
    {"principle": "separate-logic-from-io", "was_score": 0.939, "why": "Carries prior_disproof; adjudicated 2026-08-01 and accepted as an on-touch rule at TECH_DEBT.md:727-731. Re-reporting a dispositioned finding unchanged is worse than missing it, and I found no new evidence to reopen it."},
    {"principle": "reproducible-builds", "was_score": 0.941, "why": "Appears only in the UNFILTERED ranking (the copy has no .git). This is the repo's own lck-1 row: requirements-ci.lock carries no --hash. Raised 2026-08-29 and triaged as OVERFLOW at a pre-registered 12-item cap. Dispositioned; not reopened."},
    {"principle": "narrow-try-scope", "was_score": 0.88, "why": "broad_catch_count=181 is real but the swallows are concentrated in data-parsing loops where `continue` is correct. safety.py — the module where it would matter — has exactly ONE except, and it fails closed (safety.py:144). The genuine defect in this area is not scope but an unhandled RETURN value, which is finding 1 under a different principle."}
  ],
  "not_checked": [
    {"domain": "statistical-validity (PBO, deflated Sharpe, CPCV purge/embargo calibration)", "route_to": "the repo's own C2/C4 memos, results/diagnostics/null_calibration (25 stored runs) and .github/workflows/monthly-revalidation.yml — better-verified there than a design review could make it"},
    {"domain": "live broker behaviour under a real stall, partial fill, or cancel rejection", "route_to": "the maintainer's paper-account replay. Deliberately not attempted: it needs live credentials and network traffic to a real endpoint, both barred."},
    {"domain": "the dual inert-guard sweep — mocks given an exception side_effect for a method no concrete implementation can raise", "route_to": "canon maintainers as a candidate scanner metric, or this repo's own test suite; proposed and agreed with the advocate in round 2 but not built. See exchange.unresolved[0]."},
    {"domain": "the modelled x1.77 vol-scalar inflation in gap finding 9", "route_to": "the maintainer, by replaying a halted liquidation in the paper account; the figure is modelled on a synthetic 63-day window because the scenario has never occurred, so it is the one magnitude in this report not measured against live data"},
    {"domain": "web/ front-end correctness beyond the two API routes", "route_to": "web/tests plus a front-end reviewer; the Next.js app was swept for authorization and cost-per-request only"}
  ],
  "tool_warnings": [
    "remote_calls_without_timeout reported 5 on this Python repository and ALL FIVE are false positives: one is a bash regex in .claude/hooks/pretool_guard.sh:130 (not code), one is a browser fetch to a relative URL in web/components/subscribe-box.tsx:19, and three (scripts/detect_runner_loss.py:239,254,261) are call sites of an injected `fetch=` parameter whose default _gh_json carries timeout=60 at scripts/detect_runner_loss.py:180. The metric matches call sites by name without resolving the callee, and it scans non-source files.",
    "retry_sites_without_backoff reported 14, driving bound-retry-load to 0.940 for the third consecutive review of this repository, on a codebase whose retry sites all implement bounded exponential backoff.",
    "prefer-pure-functions scored 0.942 and led the detection ranking for the third consecutive review; it fires on repository size at ~58k LOC rather than on evidence.",
    "canon match's walk does not honour .gitignore: the unfiltered run traversed web/.next (~1.8 GB) and web/node_modules (~634 MB). The tracked-files-only copy matched far faster and the two rankings differ by exactly one principle.",
    "canon search --format json emits a bare JSON array while match/scan/show emit objects; a caller doing d.get(...) crashes on search only.",
    "canon has no principle covering AUTHORIZATION or COST PER REQUEST; finding 7 is filed under least-privilege, which fits the shared credential but not the unauthenticated-spend shape.",
    "canon has no principle covering BACKTEST/LIVE PARITY, the invariant this repository cares about most; finding 2 is filed under mutate-state-only-after-success, which names the mechanism but not the invariant."
  ],
  "claims": [
    {"figure": "5401 lines in src/thales/cli.py", "command": "wc -l src/thales/cli.py"},
    {"figure": "1356 passed, 1 failed, 1 skipped of 1358 tests", "command": "PYTHONPATH=<copy>/src /Users/dan/Documents/GitHub/thales/.venv/bin/python -m pytest tests -q -p no:cacheprovider"},
    {"figure": "the single failure needs a .git the copy lacks, and its assertion passes on the real repo", "command": "git ls-files | while read f; do grep -oE 'hc-ping\\.com/[0-9a-fA-F]{8}-...' \"$f\"; done  # zero offenders"},
    {"figure": "Kelly gross 0.10890 as-is vs 0.03796 / 0.01148 / 0.0 when a -20/-30/-40% period on 66 names is booked", "command": "build_target_weights(sel, ..., config=yaml.safe_load(open('config/settings.yaml')), pooled_returns_chrono=pool, return_details=True)[1]['kelly_gross']"},
    {"figure": "max_pool = 24 x 50 = 1200, not the ledger's 5000 rows", "command": "grep -n rolling_window_months config/settings.yaml; grep -n momentum_sleeve_size config/settings.yaml; sed -n '125,145p' src/thales/portfolio/construct.py"},
    {"figure": "cancel returning False still submits a full-size replacement", "command": "b.cancel_order.return_value=False; w._resolve_unfilled_limits([{...'shares':5.0...}], date(2026,9,3)) -> call('VAL', 5.0, 'buy', client_order_id='cid-9-mr')"},
    {"figure": "50438 rows, 50438 distinct (date,symbol) keys, 0 duplicates", "command": "python - <<'EOF' ... collections.Counter((r['date'],r['symbol']) for r in open('data/options_skew.jsonl')) EOF"},
    {"figure": "2068 rows on 3 non-session dates, one of them a Saturday", "command": "from thales.utils.calendar import get_trading_days; [(d,c[d]) for d in sorted(c) if d not in td] -> [('2026-06-19',892),('2026-07-03',283),('2026-08-29',893)]"},
    {"figure": "12:00 PT == 19:00 UTC under PDT, 20:00 UTC under PST", "command": "datetime(2026,9,4,12,0,tzinfo=ZoneInfo('America/Los_Angeles')).astimezone(ZoneInfo('UTC'))"},
    {"figure": "171 of 172 stored CPCV results have n_paths == C(n_groups,k_test)", "command": "walk results/**/*.json + research/**/*.json comparing math.comb(n_groups,k_test) to n_paths"},
    {"figure": "3 callers of the execution chokepoint, all gated", "command": "grep -rn execute_orders src/thales | grep -v Binary"},
    {"figure": "all 4 Alpaca clients carry _apply_default_timeout", "command": "grep -rn _apply_default_timeout src/thales | grep -v Binary"},
    {"figure": "246 tracked code files vs the scanner's 249", "command": "git ls-files | grep -cE '\\.(py|ts|tsx|js|sh)$'"},
    {"figure": "546 cancel-replacements on 15 of 33 meanrev trading days; 222/222 on 2026-08-28, 251/252 on 2026-08-31", "command": "collections.Counter(r['date'] for r in order_log if r.get('market_replace_of')) over data/state/meanrev/order_log.jsonl"},
    {"figure": "TCA median |slippage_bps| = 47.3 over 162 fills since 2026-06-11, vs a binding threshold of 50", "command": "np.median([abs(r.slippage_bps) for r in TCATracker(log_path='data/processed/tca_log.jsonl').load_records() if str(r.date)>=str(resolve_since(None,cfg))])"},
    {"figure": "tca_ok is a binding criterion, not advisory", "command": "sed -n '309,315p;389,392p' src/thales/execution/go_live.py  # tca_ok = med <= g['tca_median_slippage_bps_max']; overall_pass = all(c.ok ...)"},
    {"figure": "the inert-guard sweep returns exactly 5 sites with 0 false positives on ~17k lines", "command": "two-stage AST sweep described under Tool defects; stage 1 -> 26 candidates, stage 2 -> 5"}
  ]
}

How this review was scoped

The detection ranking was spent before it began. Three of its top four principles carry prior_disproof annotations naming this exact repository and commit, filed by the previous review's canon learn. I confirmed each annotation still describes the tree and moved on rather than re-deriving the disproof — which is what those annotations are for, and it bought the budget that produced findings 1, 3 and 4.

What remained went to three places: the paths where being wrong is expensive here (order submission, the safety chokepoint, kill-switch state, the moat capture stream), executing the code rather than reading it, and the advocate exchange the previous review could not run.

Of the eleven items below, two came from the detection ranking (and only after reading led me to them independently), three from reading, and six from the advocate across two rounds — including the priority-1 finding, a 10× correction to priority 2, and the outright death of my only original draft finding.

1 · handle-all-error-outcomes — priority 1 (rank —) · error-handling

If a call can report three outcomes, handling two of them is a bug no tool will catch.

Verified by execution. This is the most dangerous thing in the report and the review did not find it — the repository's advocate did.

daily.py:1174-1179 documents that the cancel-replace is the one order-submission site outside pipeline.execute_orders, and therefore outside the fail-closed safety gate. Its guard reads:

try:
    broker.cancel_order(r["order_id"])
except Exception as e:
    # An un-cancellable order must not abort the run tail ... and replacing
    # WITHOUT a confirmed cancel risks a double fill. Skip.
    logger.error(...)
    continue

The comment states the stake exactly right. The code cannot deliver it, because cancel_order does not raise — it returns a bool, and the return is discarded:

So except Exception is unreachable against every implementation in the repository, and False falls through to daily.py:1252, which submits a full-size market replacement on top of a resting limit that may still be live.

Executed in the copy (the only difference between the two rows is the cancel's behaviour):

cancel RAISES  (only the repo's own test does this)      -> NO replacement (guard held)
cancel returns False (what AlpacaBroker actually does)   -> REPLACED: call('VAL', 5.0, 'buy',
                                                            client_order_id='cid-9-mr')

Note 5.0 — the full original quantity, not a remainder.

The precondition is not hypothetical — it has fired 546 times. order_type: "marketable_limit" is live for momentum (config/settings.yaml:267) and meanrev (config/meanrev.yaml:153). Counting cancel-replacements in the meanrev order log:

546 replacements on 15 of 33 trading days
heavy days (originals, replacements):
  2026-08-27 (56, 55)   2026-08-28 (222, 222)   2026-08-31 (252, 251)

On 2026-08-28 and 2026-08-31, effectively every order of the day went through cancel-replace — 528 exposures to this inert guard in three sessions. Those are the displaced-cron days the repo already knows about (PIT-1), which is also when a cancel is most likely to be refused: an order resting post-close in ACCEPTED/PENDING_NEW. A cancel legitimately returns False when the order is PENDING_CANCEL, is filling at that moment, or the API errors — precisely the moments a double fill is possible. On the sell side the excess opens an unintended short, and because this site is outside the chokepoint the safety gate never sees it.

Four more sites of the identical shape, three of which the advocate found in round 2:

• daily.py:1190 — the mid-run halt does not halt. When a manual or fleet halt engages during the fill window, the code cancels every unfilled limit with the same discarded bool and the same unreachable except Exception: pass. So the resting DAY limits stay live at the broker and can fill for the rest of the session. A fail-closed control that does not close.
• cli.py:3696 — the NO-RUN escalation. send_email(...) is called as a bare statement inside try: ... except Exception as e: # the alert channel must never eat the exit code. But notifications.send_email returns False on unset credentials and swallows every SMTP failure (notifications.py:74-76), so that handler is unreachable and the only signal is the discarded False. The loudest alert in the CLI fails silently and prints nothing. cli.py:3641 and cli.py:4405 are the same shape for the digest-FAILED alerts. (raise typer.Exit(2) still reds the workflow, so GitHub's own mail is the backstop — but this is exactly finding 7's failure domain.)
• cli.py:1130-1138 — a failed run exits 0. Prints Orders FAILED: N, then falls through to typer.echo("Daily run complete."). Only critical_skips = {"No signals", "Stale price panel"} exits non-zero. A run in which every de-risking sell was rejected is green, with a green heartbeat beacon. The repo's own open FOS-1 row records that per-symbol failed-order streaks are already silent; this is the process-level half.

Scope against the repository's own queue. RPL-1 (open) covers this same function and names two facets — the gate bypass, and a check-then-act race where fills landing between the get_order_status read and the cancel over-buy the remainder. This is a third facet at that site, and it is the deterministic version of RPL-1's race: RPL-1 needs a fill to land in a millisecond window; this needs only a cancel to be refused. Filed separately for that reason — if it is folded into RPL-1 at triage, the distinction is what gets lost.

Remedy: Enumerate the call's documented outcomes and give each an explicit branch (or an exhaustive switch whose default raises); make "unrecognised result" an error rather than an implicit success.

Applied here, five one-line changes: if not broker.cancel_order(r["order_id"]): continue at daily.py:1236 and the equivalent at daily.py:1190 (keeping the existing except for genuine transport errors); if not send_email(...): console.print(...) at cli.py:3641, :3696 and :4405; and a non-zero exit in cli.py when result.orders_failed is non-empty.

How to find the rest of this class mechanically — the advocate built and ran the sweep, and it is the most transferable thing in this review, so it is written up in Tool defects below rather than buried here. Two AST stages over src/thales returned exactly these five sites with zero false positives.

2 · mutate-state-only-after-success — priority 2 (rank —) · error-handling

Build the result in a local and publish it last, so a failure leaves state untouched.

Verified, and about ten times more severe than the previous review reported.

On a kill-switch day daily.py:1421-1423 returns early into _liquidate_to_cash, upstream of daily.py:1485 — which grep -n '_update_kelly_ledger_returns' confirms is the only call site of the booking method. _liquidate_to_cash then clears the snapshot at daily.py:1130 and only afterwards calls execute_orders at daily.py:1133. Next selection day, daily.py:929 (if not prev_prices: return) eats the entire holding period — which is, by construction, the drawdown that tripped the switch.

The magnitude was wrong in the repository's favour. The previous review priced this from the raw 5000-row ledger file. Production truncates first: construct.py:129-141 computes max_pool = kelly.rolling_window_months (24) × construction.momentum_sleeve_size (50) = 1200, applied at kelly.py:91-92. Re-priced through the production entry point:

The previous review reported an 18–38% band. The real figure at a plausible −30% leg is 848%, and at −40% the correct behaviour is to deploy nothing while today's code sizes normally.

What is already owned, and what is new. The repository's open RWG-1 row, leg 3, states the snapshot half verbatim: "the kill-switch path wipes the Kelly holding-return snapshot at daily.py:1101-1103 before execute_orders — a halted liquidation clears the snapshot while the book remains fully held", and SAC-1's scope boundary explicitly defers it to RWG-1. That leg is dispositioned; I am not re-reporting it. What is new here is a different mechanism at the same site: even a fully successful liquidation discards the crash holding period, because _update_kelly_ledger_returns is never reached on a kill-switch day at all — RWG-1 is about the halted case, this is about every case. The 187%/848% magnitude and the parity comparison against engine.py are also new. The advocate retracted its own round-1 claim on this leg after reading the queue, which is the correct outcome and worth recording.

A second sink, same root cause. _evaluate_kill_switch persists {"active": True, ...} at daily.py:1024-1026 before the liquidation is attempted. daily.py:665-667 then stamps kill_switch: True on every later snapshot, and daily.py:720-721 drops those days from the realized-vol sample —

if ks:
    continue  # risk-off day: not informative about the book's vol

— which is true only if the book actually went flat. If execute_orders HALTs (manual or fleet halt file, account blocked), nothing is sold, the book stays fully invested, and the vol-target estimator silently excludes the highest-vol days it owns. On re-entry daily.py:1047-1048 also resets hwm = equity to the crashed level.

Parity. The validated engine books it: engine.py:711 gates on not kill_switch_active, but last_rebal_idx only advances inside that block (engine.py:936), so the first rebalance after re-entry spans pre-crash to present. Live throws it away. Backtest/live parity is the invariant this repository defends hardest, and canon has no principle naming it.

Why priority 2 and not 1. The kill switch has never fired: data/processed/kill_switch_state.json is active: false, days_in_cash: 0, and equity 13699.33 against a 14913.66 HWM is −8.1% into a −20% trigger. p_manifests 0.25.

Remedy: Accumulate into a local variable or temporary file, perform every validation and fallible call first, and finish with a single swap, assignment, or atomic rename that publishes the result.

Applied here: book the in-flight period before the kill-switch branch returns; move the snapshot clear and the active: True write to after execute_orders returns without a halt, in the same write_kelly_ledger call — the shape the 2026-08-29 returns_booked_for fix already established.

3 · prefer-fakes-over-mocks — priority 3 (detection rank 8, score 0.87) · testing

One in-memory implementation beats every test re-teaching the dependency how to behave.

Verified. This is the mechanism by which finding 1 reached production and stayed there.

Finding 1's guard is tested — tests/test_execution/test_daily.py:560-576 exists specifically to pin it, and ends broker.submit_market_order.assert_not_called(). It passes. It passes because line 569 says:

broker.cancel_order.side_effect = RuntimeError("cancel rejected")

Raising is a behaviour no Broker in this repository exhibits. I checked all three:

SimulatedBroker.cancel_order('unknown-id') -> False
AlpacaBroker.cancel_order                  -> catches Exception, returns False
Broker base contract                       -> returns False

The repository already owns the right tool: SimulatedBroker is a real in-memory fake of the broker interface. Had the test used it instead of a MagicMock re-teaching the dependency an invented behaviour, the assertion would have failed on the day it was written.

This is the finding I would most want a maintainer to internalise, because it generalises: a guard whose only test violates the interface it guards is not a tested guard.

Remedy: Write (or request from the owner) one in-memory implementation of the interface at the boundary that is hard to test, use it everywhere, and delete the repeated stub setup; keep a stub only where a test needs a specific error or edge value the fake cannot easily produce.

Applied here: rewrite test_daily.py:560-576 against SimulatedBroker with a resting unfilled limit, and audit the other MagicMock-based broker tests for invented failure modes. Land finding 1's fix first so the rewritten test goes green for the right reason.

4 · enforce-invariants-in-one-place — priority 4 (detection rank 13, score 0.855) · correctness

A rule checked in five methods is a rule that is wrong in at least one of them.

Verified. The invariant is enforced at the writer only, and 2068 rows that predate it are still being served.

cli.py:4592-4598 now refuses to capture on a non-trading day. But the rows written before that fix were never quarantined, and nothing on the read path re-checks:

[('2026-06-19', 892), ('2026-07-03', 283), ('2026-08-29', 893)]   = 2068 of 50438 rows

2026-06-19 and 2026-07-03 are NYSE holidays. 2026-08-29 is a Saturday.

load_skew (options_skew.py:316-344) returns them verbatim; there is no trading-day filter anywhere in the read path. So any study that groups this stream by date sees three phantom sessions carrying drifted stale quotes — in the dataset this project treats as its moat, and which it publishes.

This is a fix-sweep failure of the classic kind: the fix landed where the bug was reported (the writer), not everywhere the bug lives (the stored rows, and every reader that trusts them).

Remedy: Choose the single layer that owns the rule, enforce it there, and delete the partial checks elsewhere; where the rule must hold on data already stored, re-assert it on read until the store is migrated.

Applied here: filter load_skew on get_trading_days, and quarantine or delete the 2068 rows so the writer and the store agree.

5 · protect-shared-mutable-state — priority 5 (rank —) · state

If two threads can touch the same mutable value and one writes, lock it or stop sharing it.

Verified, narrower than the previous review implies.

Seven workflows commit and push state. Four carry a concurrency: group; three do not, and of those only skew-snapshot.yml is scheduled (routine-outbox.yml and monthly-revalidation.yml are dispatch-only).

It is dual-triggered: ops/launchd/com.thales.dispatch-skew.plist:27-30 fires at 12:00 local, the workflow cron is '0 19 * * 1-5', and the plist carries no TimeZone key — so under PDT (in force now) both target the same minute. Under PST they are an hour apart. The collision is real about eight months a year, not twelve.

What I withdrew. The drafted consequence was interleaved JSONL corruption. The advocate reproduced the actual persist tail (skew-snapshot.yml:88-93) with two clones and got CONFLICT (both add at EOF), then fatal: You are not currently on a branch three times, final exit 128. The merge is git rebase, and on an append-only JSONL it conflicts loudly. The real outcome is that the loser's capture day is permanently lost and the step reds — bad, but not silent corruption. The zero-duplicate count (50438 rows, 50438 distinct keys) was the tell, and the previous review recorded it as counter-evidence instead of letting it overturn the mechanism.

Remedy: Make the state immutable or confine it to a single thread; if it must be shared, funnel every read and write through one synchronised accessor and delete the unguarded paths.

Applied here: give skew-snapshot.yml the four-line concurrency: {group: skew-snapshot, cancel-in-progress: false} block the other three already carry.

6 · retire-stale-docs — priority 6 (rank —) · documentation

A confidently wrong document costs more than a missing one.

Verified. The fix shipped; the row still says open.

research/queue/open/skw-1-skew-client-no-deadline.md states that a fourth Alpaca client "has no request deadline" and that the capture cron "can still hang forever". At HEAD, all four clients carry the guard:

alpaca_broker.py:102    _apply_default_timeout(self.client._session)
alpaca_broker.py:352    _apply_default_timeout(self._data_client._session)
vrp_daily.py:165        _apply_default_timeout(client._session)
options_skew.py:398     _apply_default_timeout(client._session)   <- the row's subject

git log -S"_apply_default_timeout" -- src/thales/data/options_skew.py gives 29c6f23 ("SKW-1 deadline"). The row was simply never moved out of open/.

I swept the other ten rows rather than report a single instance: MEM-1 (memory/ is absent), SKW-2 (no dash-class symbol has ever captured — I checked, the only BRK-ish symbols in the stream are BRKR, UMBF and PBF), FOS-1 (grep failed_order_streak → 0 hits) and RPL-1 are all genuinely open. SKW-1 is the only stale one.

This matters more than a tidy-up: this queue is how the project decides what to work on, and a reader planning from it would redo shipped work.

Remedy: Delete or update the document in the same change as the code; where a doc must outlive a change, give it a review date and an owner.

Applied here: git mv research/queue/open/skw-1-skew-client-no-deadline.md research/queue/built/, noting 29c6f23.

7 · least-privilege — priority 7 (rank —) · security

Every capability you grant is one a bug or an attacker can use.

Verified, and deliberately discounted further than the previous review discounted it.

web/app/api/subscribe/route.ts is an unauthenticated public POST that reads EMAIL_ADDRESS / EMAIL_APP_PASSWORD at :49-50 and opens an SMTP session at :59-74. That credential is shared with seven workflows including _alert.yml and all three trading workflows. The only limiter is route.ts:16-23, an in-process Map keyed on the spoofable x-forwarded-for, 5 hits per 60s, per serverless instance.

What I withdrew. The previous review wrote "the alerting layer of a live trading system". Every sleeve is paper: true (config/settings.yaml:243, config/meanrev.yaml:141, config/vrp.yaml:110) and RUNBOOK.md:60-61 states there are no live credentials anywhere. The blast radius is a paper account's alerting. p_manifests 0.05.

Remedy: Grant the narrowest capability that works, scope it to the shortest lifetime and smallest resource set, and separate credentials so one compromise does not imply another.

Applied here: a dedicated sending credential for the web form, so abuse of a public endpoint cannot touch the operational alerting path.

8 · gap: backtest/live parity has no principle — priority — (no record) · backtest/live parity

There is nothing in the 250-principle database naming the invariant this repository spends the most effort defending: the live execution path must reproduce the decisions of the validated simulation it was signed off against. Finding 2 is filed under mutate-state-only-after-success, which correctly names the mechanism (state published before the fallible call) but not the reason it is severe here — that engine.py:711 and engine.py:936 book the crash period and daily.py:929 does not, so the two diverge silently and only under the conditions nobody has yet observed.

The actionable work is finding 2; this entry exists so the gap is rankable rather than buried in prose. Recorded, not invented — I did not force it into an approximating id.

9 · gap: observed state vs recorded intent has no principle — priority — (no record) · state

The highest-value gap in this report: a real finding with a measured magnitude and no principle to hang it on. canon show prefer-observed-state-over-recorded-intent returns "no principle"; a targeted search returns nothing closer than make-intent-explicit, which is about a different concern. I did not force it into an approximating id.

_record_portfolio_snapshot sets snap["kill_switch"] from persisted intent — read_kill_switch_state().active at daily.py:665-667 — when the ground truth is sitting in the same dict eleven lines earlier, as snap["long_mkt_value"] (daily.py:648-649). _gross_returns then consumes that flag:

if ks:
    continue  # risk-off day: not informative about the book's vol

The comment is right when the liquidation happened. When it HALTed, intent says cash, the book is fully held, and every subsequent day is dropped from the realized-vol sample — the highest-vol days the book actually owns. Modelled over a 63-day window with a 21-day crash regime: realized vol 24.5% (truth, n=63) versus 13.9% (as computed, n=43), a ×1.77 inflation of the vol-target scalar, which max_leverage: 3.0 does not clip. It is applied on the re-entry run, sizing the rebuild straight after a crash.

This is not finding 2 and not the repo's SAC-1 (whose "member four" is the vol_scalar divisor at daily.py:714-719, a different line and mechanism). The fix is one line in a different function: derive the flag from long_mkt_value, not from the intent record. The magnitude is modelled rather than observed, because the scenario has never occurred — hence confidence: medium.

10 · gap: authorization has no principle — priority — (no record) · authorization

The database has no principle covering who may read or write what, so no ranking would ever prompt the sweep. I ran it by hand: web/app/api holds exactly two routes; subscribe/route.ts:25 is an unauthenticated POST that only mails the operator, and [transport]/route.ts exports GET/POST/DELETE over mcp-handler where every tool is read-only over already-public JSON, with no tool that trades or advises. The dimension is clean here — this entry records the gap, not a defect.

11 · gap: cost per request has no principle — priority — (no record) · cost per request

Nothing covers what one unauthenticated request can make the system spend. Finding 7 is filed under least-privilege, which fits the shared credential but not the unauthenticated-spend shape — and the spend shape is the half an anonymous caller can actually reach, via the per-instance Map limiter at route.ts:16-23. The actionable work is finding 7.

Conflicts

canon tensions returns nothing for handle-all-error-outcomes, prefer-fakes-over-mocks, retire-stale-docs, mutate-state-only-after-success, protect-shared-mutable-state and least-privilege. An empty result is not an error and is worth stating: the selected set carries no recorded conflicts.

The one tension touching this report is idempotent-operations ↔ prefer-simplicity, and both a_stance and b_stance are null — the database supplies no position, so the resolution below is the reviewer's judgement, not a database ruling:

• Resolution for this repo: prefer simplicity. The idempotency machinery here is already three layers deep (already_ran_today, the open-order skip, _client_order_id) and the layer I proposed adding turned out to be unnecessary — see below. Do not add a fourth. Fix finding 1 instead, which removes a branch rather than adding one.

Dropped after verification

A note on method: I stopped sweeping the detection ranking after the top four died for the same reason, per the procedure's systematic-failure rule. Ranks 5–7 and 9–12 (weigh-dependency-cost-before-adding, match-rigour-to-lifespan, eliminate-unknown-unknowns, prevent-backsliding, delete-obsolete-code, test-failure-paths, make-intent-explicit) are unexamined, not clean. Ranks 8 and 13 were pulled back in only because reading and the advocate led me to them independently — which is itself evidence that the tail was worth more than the head here.

Verified but demoted

idempotent-operations — premise verified, consequence disproved. p_manifests 0.02.

_client_order_id (pipeline.py:362-375) hashes order['shares'], and execute_orders mutates that field at pipeline.py:202-217 via a sell-clip skipped whenever broker.get_positions() throws (pipeline.py:204-206). I confirmed by execution that the same intent yields two different keys, and that a transient failure does not halt the run:

both broker reads OK                                  halted=False  key=...-s-AAPL-356a19
get_positions fails ONLY at the clip site (call 1)    halted=False  key=...-s-AAPL-8140e0
get_positions fails durably (calls 1 and 2)           halted=True   nothing submitted

All true — and it does not matter. The advocate's refutation, which I verified: the quantity is recomputed from live broker truth every run (daily.py:1409-1412 → pipeline.py:114-150). A filled sell disappears from current_positions, diff is 0, and no order is generated at all; a partial fill produces an order that is the remainder. A different quantity therefore accompanies a genuinely different intent, which is exactly what pipeline.py:363-371 says the qty component is for. And within one retry envelope the key is provably stable — nothing in _submit_with_retry touches order['shares'].

Recorded rather than deleted so the next review does not re-derive it. What survives is one narrow observation: get_positions() is fail-closed HALT at pipeline.py:251 and silently swallowed at pipeline.py:203, forty-eight lines apart. The advocate agrees that split is worth fixing; it just produces a rejected over-sell (and, per finding 1, a green run), not a double fill.

CPCV silent path-dropping — premise verified, consequence measured to ~0.01.

cpcv.py drops a failed path with logger.warning + continue at four sites, and format_cpcv (cpcv.py:779-781) then prints C({n_groups},{k_test})={n_paths} paths — asserting an identity that only holds if nothing was dropped. I checked every stored result: 171 of 172 have n_paths exactly equal to C(n,k). The single exception has n_paths=0, which the formatter handles explicitly ("No valid paths"). The partial case has essentially never occurred. Real, cosmetic, and not worth a programme of work.

The exchange

Two rounds with canon:repo-advocate, which re-derived every figure and read the expensive paths independently. It moved this report more than any other step.

What it found that I missed — seven items, three of which became findings 1, 2's second sink and 4. The other four:

• The post-halt recovery instruction is a green no-op. _record_safety_event (pipeline.py:274) writes a run_summary on any HALT, and already_ran_today (state.py:180-187) honours any run_summary for that date. So RUNBOOK.md:253-254's "thales run --skip-market-check locally" returns skip_reason="Already ran today", which is not in critical_skips, and exits 0. The operator's screen says success. The doc and the code disagree about what recovery does.
• TCA prices a partial fill at the replacement price. daily.py:1244-1258 replaces only remaining, but daily.py:1748-1751 records the fill with the original shares, so the already-filled portion is booked at the market-replacement price — feeding a pre-registered TCA decision (settings.yaml:393).
• The Kelly magnitude correction (finding 2), which made my own finding ten times stronger.
• The paper-vs-live correction (finding 7), which made one weaker.

How the contested findings ended:

I verified every refutation before conceding it; none of them failed to reproduce, which is itself notable — this advocate was unusually well-evidenced.

Round 2 changed the report again, which is why it was worth running rather than stopping at one exchange:

• It found a second site of finding 1 (daily.py:1190) that I had literally read on screen and failed to connect, plus three more (cli.py:3641, :3696, :4405). Finding 1 went from one site to five.
• It produced the frequency evidence that settled the finding-1-vs-finding-2 ranking dispute: 546 cancel-replacements on 15 of 33 meanrev sessions, against a kill switch whose worst live drawdown in 5½ months is 10.4% versus a 20% trigger. I re-derived both.
• It retracted one of its own round-1 claims after reading the repo's queue — the snapshot-wipe leg is already owned by RWG-1 leg 3 — and it corrected its own M6 mechanism, which I had been about to file wrongly.
• It verified my queue sweep independently rather than accepting it, confirmed all four rows I kept, checked the six I had not, and found no second silently-shipped row — closing an item I had listed as unresolved.

Still open when I stopped: the dual sweep (mocks given an exception side_effect for a method nothing can raise), and how much the finding-1 replacement path biases the 47.3 bps TCA figure. Both are routed in not_checked.

The advocate's recurrence lesson, which I am adopting: every finding in my draft was a missing guard; every one of its best findings was a guard that exists and does not fire. On a codebase this dense with guards, a sweep that scans for absence is structurally blind to the actual defect class. It then built the sweep that finds the other class — see Tool defects.

Near misses

• The RUNBOOK's post-halt recovery no-op (above) — bounded to one lost day because the month stays open via selection_completed=False (daily.py:502-511), which is why it is here and not a finding.
• The binding go-live TCA criterion is at 94.6% of its threshold. Measured: median |slippage_bps| = 47.3 over 162 fills since 2026-06-11, against tca_median_slippage_bps_max: 50 (config/settings.yaml:363), which go_live.py:313-314 makes binding via overall_pass = all(c.ok ...). A 2.7 bps margin. go_live.py:309-311 pools every fill since inception with no rolling window and no re-weighting, so any measurement error at that site is permanent and cumulative. Of the seven go-live criteria this is the only one near its boundary, and nothing monitors it between now and the December decision. Not a finding because it is a number, not a defect — but it is the number that makes the item below matter.
• TCA prices a partially-filled order at the replacement price. daily.py:1258 sets r["order_id"] = res.order_id, so _backfill_fill_prices (daily.py:1264-1281) fills the record from the replacement order and the whole quantity is priced at the market fill rather than a qty-weighted blend. The advocate's round-1 mechanism for this was wrong and it corrected itself in round 2: shares cannot bias the gate, because TCARecord.__post_init__ (tca.py:43-53) derives slippage_bps from prices only. The real vector is fill_price, and it fails in the safe direction — the replacement is by construction the worse fill, so this over-states |slippage_bps| and biases the gate toward FAIL. Recorded with the corrected mechanism so it survives the next reviewer.
• Per-name capture coverage is unmeasured. GHC and NVR have contributed zero rows in 62 capture days and are outside SKW-2's dash-class scope; MASI appears on 1 day, ASGN and HOLX on 18, PSTG on 19. capture_qa's only per-day skew test is total row count against a trailing median (captures.py:610-629), and one permanently-absent name is ~0.1% of that count. Worth re-scoping SKW-2 from "the two dash-class names" to "per-name coverage is unmeasured", with a coverage census as the deliverable.
• RPL-1 remains live at daily.py:1252: the market-replacement never re-enters execute_orders. Already open in the repo's queue; finding 1 is the sharper statement of why that bypass is dangerous today.
• _read_jsonl (state.py:191) has no try around json.loads. The docstring's PIPE_BUF atomicity argument is correct for lines under ~4 KiB, but a run_summary carrying many rejected orders can exceed that. Guarded in practice by the workflow serialisation that finding 5 is about.
• max_cyclomatic_complexity = 213, max_function_loc = 956 — real, and concentrated in cli.py (5401 lines). Not raised as a finding because it is dispositioned tech debt and nothing I traced routed a correctness defect through it.

What's healthy

Held to the same evidence standard as the findings — I opened each file rather than trusting its docstring, and checked I was not praising something I criticised elsewhere.

• The execution chokepoint is real, and enforced against its own authors. grep -rn execute_orders src/thales gives exactly three callers (daily.py:1133, daily.py:1724, vrp_daily.py:677), all gated. More convincingly, broker.py:137-152 makes close_position and close_all_positions raise NotImplementedError with the message "bypasses the safety gate — construct sell orders and route through pipeline.execute_orders (fleet law)". A latent footgun deleted rather than documented. (The one exception, the cancel-replace, is finding 1 — and its existence is documented at daily.py:1174-1179; the defect is in its guard, not in the decision to have it.)
• Two of the three fail-closed branches around that same code are correct, which is why finding 1 is a defect and not a pattern: daily.py:1219-1231 refuses to act on an unknown order state ("no cancel, no replacement"), and the halt-mid-run branch cancels without replacing. Only the cancel-failure branch reads the wrong signal.
• Durable state writes. _append_jsonl (state.py) flushes and fsyncs, with a correct PIPE_BUF atomicity argument; write_kelly_ledger uses atomic_write_lines.
• Deadlines are complete once you look at the right shape. All 9 requests/urlopen sites carry timeout=, and all four Alpaca SDK clients carry _apply_default_timeout. The scanner's contrary metric is wrong (see below), and so was my own first grep.
• Secrets. No credential is committed; the one pattern hit is a fake key inside a test of the redaction scrubber (test_public_export.py:281). A test enforces that no live healthchecks.io beacon URL is ever committed — I reproduced its assertion against the real repository by hand and it holds. web/.env.local is untracked and gitignored.
• 1356 of 1358 tests pass in a clean copy in 17.9s. The one failure needs a .git the copy lacks; I verified its assertion by hand against the original. The one skip is conditional and benign. Finding 3 is about a specific test's fidelity, not the suite's health.
• The project audits itself, in public. An open defect queue with pre-registered kill criteria, a published kill-list of rejected ideas, and research notes exported verbatim. Four of the five items I checked in that queue were accurate — finding 6 is one stale row against a process that is, on this evidence, working.

Tool defects (about canon, not about this repository)

• remote_calls_without_timeout reported 5; all 5 are false positives. One is a bash regex in .claude/hooks/pretool_guard.sh:130 — not code in any language. One is a browser fetch() to a relative URL (web/components/subscribe-box.tsx:19). Three (scripts/detect_runner_loss.py:239,254,261) are call sites of an injected fetch= parameter whose default _gh_json carries timeout=60 at detect_runner_loss.py:180. The metric matches call sites by name without resolving the callee, and it scans non-source files. This is the dangerous kind of defect: a reviewer who trusts it reports five missing deadlines that do not exist, and one who dismisses it may skip the dimension.
• retry_sites_without_backoff reported 14, driving bound-retry-load to 0.940 for the third consecutive review of this repository, on a codebase whose retry sites all implement bounded exponential backoff.
• prefer-pure-functions led the detection ranking for the third consecutive review at 0.942; it fires on repository size at ~58k LOC.
• canon match's walk does not honour .gitignore — it traversed web/.next (~1.8 GB) and web/node_modules (~634 MB).
• canon search --format json emits a bare array where match/scan/show emit objects.
• Database gaps. No principle covers authorization, cost per request, backtest/live parity, or preferring observed state over recorded intent. All four are filed as principle: null findings with a gap so canon gaps can rank them — not only as warnings, which canon learn discards as unclassified. (This review's first canon learn run reported "3 tool warnings not classified"; that is exactly the failure mode, and it is why the gaps were moved into findings.)

A metric canon should have: the inert-guard sweep

The most transferable output of this review. An inert guard is a call site where the caught signal and the returned signal differ, and only one is handled. Two AST stages:

1. Recall. For every def whose body contains except …: return <value> with no reachable raise, record it as a swallower. Flag every try: block that calls one — its except is unreachable for that call.
2. Precision. Keep only sites where the call is a bare expression statement, so the return value is discarded too. Both signals dropped.

Run against src/thales (~17k lines), stage 1 gives 26 candidates; stage 2 gives exactly five, with zero false positives — and all five are the bugs in finding 1:

BOTH-SIGNALS-DROPPED  cli.py:3641    send_email()    (swallows at utils/notifications.py)
BOTH-SIGNALS-DROPPED  cli.py:3696    send_email()
BOTH-SIGNALS-DROPPED  cli.py:4405    send_email()
BOTH-SIGNALS-DROPPED  daily.py:1190  cancel_order()  (swallows at alpaca_broker.py:421)
BOTH-SIGNALS-DROPPED  daily.py:1236  cancel_order()

Stage 1's output is the better artifact for a reviewer (26 places a reader would reasonably believe an error is handled); stage 2's for a maintainer (5 places where nothing is). Two known caveats: it self-matches when a swallower calls itself (alpaca_broker.py:429-431), and it misses guards that swallow via a decorator or context manager rather than a literal try.

This would have found the highest-severity item in this report in seconds, on a repository where the existing metrics produced three false-positive principles and nothing else.

Reviewer's notes

The single most important thing in this report is that the previous review's headline number was wrong by 10×, and that no amount of re-reading would have caught it. It was caught by calling build_target_weights with the production config instead of re-implementing the estimator from the ledger file. On a codebase where sizing is a chain of config-gated overlays, pricing a finding by re-deriving the arithmetic is not verification — it is a second, unreviewed implementation. I would make that a standing rule.

The ranking was worth almost nothing here and the advocate was worth almost everything. Two of seven findings came from the ranking, and both only after reading led me to them independently. The top four detections have now died three times on this repository. By contrast the advocate produced the priority-1 finding, corrected the priority-2 magnitude upward by an order of magnitude, corrected priority-7 downward, and killed my only original draft finding outright. A review of this repository without an advocate — which is exactly what shipped this morning — is substantially weaker, and the previous review said so itself in its not_checked.

What I got wrong, recorded so it is not repeated. My hand-sweep for missing deadlines grepped requests.get|urlopen and concluded "all clean". That grep was structurally blind to the Alpaca SDK client shape, which is where the repository's own open row said the gap was. I only caught it by reading the defect queue. This is the same failure the skill warns about for scanner zeros — and I made it with my own grep, which is worse, because I trusted it more.

A second round of the exchange paid for itself and I nearly skipped it. After round 1 I had a clean, check-passing report and every incentive to ship. Round 2 quadrupled finding 1's site count, produced the number that settled the priority dispute, retracted one advocate claim, corrected another, and closed an open item. The stopping rule in the procedure is "a round that only restates the previous one" — round 2 restated nothing.

Where I would look next, in order. (1) Build the dual sweep — mocks given an exception side_effect for a method no implementation can raise. Finding 3 is one instance and the sweep is an hour. (2) vrp_daily.py, which shares the chokepoint but has its own 843-line run path I read only where it touched the chokepoint. (3) Quantify how much the finding-1 replacement path inflates the 47.3 bps TCA median, since that number is 2.7 bps from blocking a go-live decision.

On confidence. Findings 1, 3, 4 and 6 are verified by execution or by direct file-and-line inspection and I would act on them today. Finding 2's mechanism is certain and its magnitude is now measured, but its trigger has never occurred. Findings 5 and 7 are correct and small. The demoted idempotent-operations entry is the one I would most expect a third reviewer to re-derive and waste a day on, which is why it is written up in full rather than deleted.