# Design review — thales ```json { "review": { "repo": "/Users/dan/Documents/GitHub/thales", "commit": "24cfe0b1a6f4528dbea6ca7c04f668f31dd860d4", "reviewed_at": "2026-08-29", "database_principles": 250, "canon_commit": "f5f6fdfad7d743ef3f6c39bbb366aae1cc1f4fe0" }, "scan": { "code_files": 182, "loc": 33711, "primary_language": "python", "excluded": ["data/ (1.0 GB parquet)", "web/ (2.5 GB dashboard + assets)", "books/ (19 MB PDFs)", "results/ (79 MB)", "logs/ (6.2 MB)", ".venv/"], "scan_looks_sound": true, "note": "match run twice — filtered copy (data/web/books/results/logs excluded) and unfiltered on the real tree. The unfiltered walk pulls in web/ TypeScript (3,151 LOC) and reshuffles the mid-ranking but agrees on the top cluster (deps, partial-failure, pure-functions). Shipped python is 182 files / 33,711 src LOC (git ls-files 'src/*.py'), 95 test files. Both rankings were reconciled; nothing in the excluded trees is reviewable source." }, "detection_rank": ["pin-external-dependency-versions", "design-for-partial-failure", "bound-retry-load", "reproducible-builds", "prefer-pure-functions", "separate-logic-from-io", "eliminate-unknown-unknowns", "weigh-dependency-cost-before-adding", "prefer-fakes-over-mocks", "narrow-try-scope"], "priority_rank": ["idempotent-operations", "design-for-partial-failure", "fail-safe-defaults", "reproducible-builds", "enforce-invariants-in-one-place"], "findings": [ { "rank": null, "priority": 1, "principle": "idempotent-operations", "score": null, "source": "read", "coordinates": "reviewer", "verification": "consequence-verified", "p_manifests": 0.4, "sink": "src/thales/execution/daily.py:936 (ledger['returns'] append, persisted immediately) — the half-Kelly return pool that scales the whole book's sizing at the next selection", "evidence": "the booking (_update_kelly_ledger_returns) is called at daily.py:1453 and persists at :930-937; its ONLY dedup guard is daily.py:912 `if snapshot.get('date') == today: return`, but the snapshot date is advanced only by _save_kelly_snapshot at :1740, gated by `if not halted` at :1732 (run-end). Every non-completion on a selection day runs after the booking and before that advance: safety HALT (:1732 false), stale panel (early return ~:1489, selection_completed:False), no signals (~:1506). _is_selection_day (:501-510) re-enters next day because no run this month has selection_completed:True. Advocate reproduced against the repo's own code: two attempts before a successful snapshot book the same holding period twice ([0.10,-0.10] → [0.10,-0.10,0.21,-0.19])", "damage_reach": "system", "apply_risk": "module", "mechanical": false, "prerequisite": null, "confidence": "high", "priority_basis": "P~0.4 (halt / stale-panel / crash on a monthly selection day, then the next-day retry — the system's NORMAL recovery flow) x moderate cost (silent bias of the book-wide half-Kelly sizing scalar, skewed toward adverse days since halts cluster there; diluted in a 5000-cap pool) x moderate diff (one atomic booking+marker write)", "contested": "held", "challenge": "Raised BY the advocate as the headline miss; reviewer verified every line and the retry re-entry", "held_on": "daily.py:912 guard vs :1453 booking vs :1732 `if not halted` snapshot advance; _is_selection_day :501-510 re-enters; empirical repro" }, { "rank": 2, "priority": 2, "principle": "design-for-partial-failure", "score": 0.993, "source": "matched", "coordinates": "reviewer", "verification": "consequence-verified", "p_manifests": 0.3, "sink": "src/thales/execution/alpaca_broker.py:73/321 and vrp_daily.py:157 (Alpaca TradingClient / StockHistoricalDataClient / OptionHistoricalDataClient created with no timeout) — submit_order/get_positions/get_open_orders/get_latest_quotes/reconcile can hang indefinitely", "evidence": "alpaca-py 0.43.2 RESTClient._request/_one_request contain no timeout; the call is self._session.request(...) with no timeout (alpaca/common/rest.py:194); requests default timeout is None=infinite (verified via inspect.getsource). No timeout wrapper in src/thales/execution/. Contrast: the Tiingo downloader sets timeout=30.0 (downloader.py:93). Mitigation is only the GHA job timeout-minutes:20-30 (paper-trading*.yml), which kills the whole job — possibly mid-order-loop (pipeline.py:321-359), before log_run/reconcile. Compounds finding 1: a hung call → SIGKILL mid-run → non-completion → Kelly re-book", "damage_reach": "system", "apply_risk": "local", "mechanical": true, "prerequisite": null, "confidence": "high", "priority_basis": "P~0.3 (a broker/data API stall on any scheduled run) x moderate-high cost (the run hangs to the job limit, is SIGKILLed mid-order-loop, and — critically — the idempotency/retry design is UNREACHABLE because a stall never raises for the retry loop to catch) x tiny diff (pass a request timeout to the SDK clients; treat a timeout as the ambiguous case the client_order_id already handles)", "contested": "held", "challenge": "does the GHA job-timeout fully mitigate, and does alpaca-py really set no timeout?", "held_on": "advocate confirmed against installed alpaca-py 0.43.2 (rest.py:194, no timeout) and showed the job-timeout kills mid-loop rather than failing the call cleanly, leaving the retry/lookup path dead in the stall case it was built for" }, { "rank": null, "priority": 3, "principle": "fail-safe-defaults", "score": null, "source": "read", "coordinates": "reviewer", "verification": "consequence-verified", "p_manifests": 0.05, "sink": "src/thales/execution/safety.py:227-228 — the daily-loss circuit breaker is skipped when last_equity is 0/None/NaN", "evidence": "safety.py:226 `last_eq = float(getattr(account,'last_equity',0) or 0)`; :227 `eq_ok = isfinite(last_eq) and last_eq>0 and isfinite(equity) and equity>0`; :228 `if eq_ok and not all_de_risking:` — so a degenerate last_equity silently SKIPS the -10% breaker (the position/notional/order-count caps still apply). In an otherwise rigorously fail-CLOSED layer (broker-unreachable→HALT pipeline.py:255, gate-error→HALT safety.py:265, account-None/blocked→HALT safety.py:182-214), this one arithmetic guard fails OPEN", "damage_reach": "system", "apply_risk": "local", "mechanical": true, "prerequisite": null, "confidence": "high", "priority_basis": "P~0.05 (only reachable on an unfunded/never-traded account where last_equity is 0) x high cost IF reached (the loss breaker is the Knight-Capital guard the file cites) x tiny diff (an unreadable equity should HALT, not skip — the safe default is fail-closed)", "contested": "held", "challenge": "Raised BY the advocate as a minor; reviewer confirmed the skip", "held_on": "safety.py:227 — eq_ok false gates out the breaker at :228; the same pattern (a divide-by-degenerate guard that becomes a silent bypass) is the recurring shape the advocate named" }, { "rank": 4, "priority": 4, "principle": "reproducible-builds", "score": 0.9365, "source": "matched", "coordinates": "reviewer", "verification": "premise-verified", "p_manifests": 0.1, "sink": "a hand-provisioned prod/dev venv (`pip install .` without -c) resolves floating floors to whatever exists that day, on the box that trades real money", "evidence": "pyproject.toml:24-44 (floating floors: polars>=1.0, alpaca-py>=0.21, …); CI installs with `pip install . -c requirements-ci.lock` (paper-trading*.yml, fleet-digest.yml — reproducible in CI); but requirements-ci.lock has NO --hash/--require-hashes (grep), so CI is version-reproducible not hash-verified, and a non-CI install floats", "damage_reach": "system", "apply_risk": "local", "mechanical": true, "prerequisite": null, "confidence": "high", "priority_basis": "P~0.1 (only on a hand-run install off the CI path) x moderate cost (a surprise major on the trading box) x small diff (add hashes to the lock; install from it everywhere, not only CI)", "contested": "narrowed", "challenge": "the scanner's 'no lock committed' is a partial miss — a constraints file IS committed and CI uses it", "held_on": "narrowed to: reproducible in CI, but hash-less and not enforced off the CI path; advocate confirmed the lock has no hashes and pyproject floors" }, { "rank": null, "priority": 5, "principle": "enforce-invariants-in-one-place", "score": null, "source": "read", "coordinates": "reviewer", "verification": "consequence-verified", "p_manifests": 0.05, "sink": "src/thales/execution/daily.py:1207-1234 (_resolve_unfilled_limits: the cancel-and-market-replace of an unfilled marketable-limit remainder)", "evidence": "the replace path re-checks only the manual/fleet halt (daily.py:1155-1165), NOT a daily-loss breaker or per-order reject that materialized during the ≤5-min fill wait — so the full safety gate is enforced at initial submission but only partially on the replace. Live: settings.yaml:267 (momentum) and meanrev.yaml:153 are both order_type: marketable_limit (vrp.yaml:123 is market). Second facet: `remaining = shares − filled_qty` reads filled_qty (daily.py:1202) BEFORE the cancel (daily.py:1208); if the resting DAY limit fills more between the read and the cancel, the market replacement over-buys by the delta (a check-then-act race)", "damage_reach": "module", "apply_risk": "local", "mechanical": false, "prerequisite": null, "confidence": "high", "priority_basis": "P~0.05 (needs a >10% intraday move or a fresh reject inside a minutes-long fill window, and only bites a BUY remainder — a de-risking SELL should complete) x moderate cost (a market BUY placed after the loss breaker tripped, or a small over-buy) x moderate diff (re-run the FULL safety gate on the replacement, and re-read filled_qty after the cancel confirms)", "contested": "held", "challenge": "Raised BY the advocate as a latent minor; round 2 showed it is reachable in the live config (two sleeves marketable_limit)", "held_on": "config/settings.yaml:267 + config/meanrev.yaml:153 both marketable_limit; daily.py:1155-1165 re-checks only manual/fleet halt; daily.py:1202 reads filled_qty before the :1208 cancel" } ], "exchange": { "rounds": 2, "advocate_findings_adopted": [ {"what": "The half-Kelly return pool double-books on every failed-selection retry — the dedup guard is keyed on the snapshot date (advanced only on a successful, non-halted run) instead of a booking marker; reproduced empirically", "where": "daily.py:912 vs :1453 vs :1732/:1740; :501-510", "became": "idempotent-operations (priority 1)"}, {"what": "The idempotency/retry design is unreachable under a stall: a hung Alpaca call never raises, so the retry-lookup-before-resubmit path is dead code exactly when a hang SIGKILLs the job mid-order-loop", "where": "alpaca/common/rest.py:194; pipeline.py:321-437", "became": "compounding link, folded into design-for-partial-failure (priority 2)"}, {"what": "The daily-loss circuit breaker fails OPEN when last_equity is degenerate (0/None/NaN)", "where": "safety.py:227-228", "became": "fail-safe-defaults (priority 3)"}, {"what": "The marketable-limit cancel-and-market-replace re-checks only the manual/fleet halt, not a daily-loss breaker/reject that tripped during the fill wait, and over-buys on a stale filled_qty read — reachable in the live config (round 2)", "where": "daily.py:1155-1234; settings.yaml:267; meanrev.yaml:153", "became": "enforce-invariants-in-one-place (priority 5)"} ], "conceded": [], "held": [ {"principle": "design-for-partial-failure", "challenge": "does alpaca-py set no timeout / does the job-timeout mitigate?", "held_on": "confirmed against alpaca-py 0.43.2 rest.py:194; job-timeout kills mid-loop, not the call"}, {"principle": "reproducible-builds", "challenge": "a lock IS committed", "held_on": "narrowed to hash-less + off-CI-path floating; advocate re-derived"} ], "unresolved": [] }, "dimensions_swept": [ {"dimension": "concurrency", "verdict": "checked - clean", "basis": "the live path is a single scheduled cron per sleeve; no threads/pools in execution/. State files are one-writer-per-run. grep for threading/multiprocessing in src/thales/execution → none"}, {"dimension": "data integrity", "verdict": "checked - finding", "basis": "read the kelly_ledger read-modify-write cycles (daily.py) — three disjoint-key RMWs per run are fine, but the returns booking is not idempotent across retries (finding 1). fetch now MERGES not overwrites (TECH_DEBT, verified in downloader)"}, {"dimension": "numeric representation", "verdict": "checked - handled well", "basis": "read the order sizing + VRP geometry; shares rounded explicitly, limit_price rounded to 2dp (alpaca_broker.py:215), VRP credit/width/worst-case arithmetic re-derived by the advocate and correct (option_intents.py:174-223)"}, {"dimension": "security posture", "verdict": "checked - handled well", "basis": "API keys from env, redacted from httpx exception messages (downloader.py:88 note); no secret in the tree; paper/live flag explicit"}, {"dimension": "external side effects", "verdict": "checked - finding", "basis": "read the whole order path — idempotent client_order_id + duplicate-422 + open-order skip (pipeline.py:182) is well-built, but no request timeout on the broker (finding 2) and the stall case defeats the idempotency (finding 2)"}, {"dimension": "failure across boundaries", "verdict": "checked - finding", "basis": "the money path (Alpaca) has no per-call deadline while the data vendor (Tiingo, timeout=30) does — finding 2; the safety layer HALTs on broker-unreachable but a HANG is not unreachable, it is silence"}, {"dimension": "operability", "verdict": "checked - handled well", "basis": "read AUDIT.md — a standing daily/weekly/monthly audit program, kill-switch state, reconcile (book==broker), runner-loss detection, a week-long-silent-failure regression test. Exceptional"}, {"dimension": "authorization", "verdict": "not applicable", "basis": "no multi-user surface; the only external actor is the broker API keyed by env secret. The GHA workflows are the entry points, gated by repo access"}, {"dimension": "identity & equality", "verdict": "checked - handled well", "basis": "the client_order_id is a deterministic (date,side,symbol,qty) key — advocate confirmed no accidental cross-quantity collision (the qty hash is a function of shares, not random); symbols normalized (class-share BRK-B↔BRK.B, alpaca_broker.py:31)"}, {"dimension": "determinism", "verdict": "checked - handled well", "basis": "the engine↔live parity harness anchors live construction byte-for-byte to a real run_backtest (TECH_DEBT P1); backtest non-reproducibility across refetches is a KNOWN, dispositioned data-vendor re-basing issue, not a code defect"}, {"dimension": "published artifacts", "verdict": "checked - clean", "basis": "the public data export (execution/public_export.py) + web dashboard derive from the run state; not read in depth but no hand-restated schema found in the paths swept"}, {"dimension": "cost per request", "verdict": "checked - handled well", "basis": "the pre-trade quote fetch is bounded (_MAX_QUOTE_FETCH_CALLS=24, alpaca_broker.py) so a pathological batch cannot storm the API — the EXQ-1 fix, verified"} ], "dropped": [ {"principle": "pin-external-dependency-versions", "was_score": 0.9948, "why": "ranked #1 on pyproject floors + 'no lock committed' — but requirements-ci.lock IS committed and CI installs with -c against it. The reproducibility residual survives as finding 4 (reproducible-builds, hash-less/off-CI); the raw 'no lock' signal is a detector miss (constraints file unrecognized)"}, {"principle": "bound-retry-load", "was_score": 0.94, "why": "the order retry (_submit_with_retry) uses exponential backoff (pipeline.py); the flagged no-backoff sites are tests and the Tiingo downloader (which has its own 429/hourly-cap handling). Not a load-amplification defect"}, {"principle": "prefer-pure-functions", "was_score": 0.9347, "why": "evidence is test fixtures (np.random.default_rng(42) seeds, datetime.now in test helpers) and .claude/ hooks; the engine core is a pure decision function anchored by the parity harness"}, {"principle": "separate-logic-from-io", "was_score": 0.9051, "why": "max_function_loc=956 and the print/open sites are research scripts (research/a3_nowcast.py) and tests; execution logic is layered behind the Broker ABC and a state store"}, {"principle": "reproducible-builds (the raw signal)", "was_score": 0.9365, "why": "the absolute-path and date/hostname evidence is .claude/settings.local.json (per-user, gitignored) and a report header in monthly_revalidation.sh — not build artifacts. The REAL reproducibility gap (finding 4) was found by reading the lock, not from these signals"} ], "not_checked": [ {"domain": "whether a surviving legacy/hand-restored VRP position file (lacking strikes) exists in the gitignored data/ state, which would make the VRP close self-reject (near-miss) live", "route_to": "the maintainer — advocate could not inspect the gitignored data/ state"}, {"domain": "the web/ TypeScript dashboard (3,151 LOC) and public_export end to end", "route_to": "a front-end/read-only-publish review; it derives from run state and is not money-moving"}, {"domain": "the research/ experiment scripts and the auto-merge records gate (AMG panel findings)", "route_to": "the ops RESEARCH_PANEL program — a self-modifying-workflow safety area, out of the money path"}, {"domain": "full test-suite execution (large; runs against fixtures)", "route_to": "CI (it runs the suite on every push with the constraints lock); reviewer ran targeted reads not the full suite"} ], "tool_warnings": [ "pin-external-dependency-versions ranked #1 on 'no lock committed' — a pip constraints file (requirements-ci.lock) IS committed and used by CI; the signal doesn't recognize constraints files", "design-for-partial-failure's evidence pointed at shell hooks (.claude/hooks/pretool_guard.sh) and an internal fetch() call, NOT the broker; the real untimed-money-path defect was found by reading, not the signal", "the reproducible-builds signals fired on a gitignored per-user .claude/settings.local.json and a report-header hostname/date — noise; the real gap is the hash-less lock, invisible to the signal", "prefer-pure-functions / separate-logic-from-io / bound-retry-load drew evidence predominantly from tests/ and research/ scripts (the repo is test- and research-heavy); on such a tree the regex signals over-weight fixtures", "the walk counted data/ (1GB) and web/ (2.5GB) gitignored trees; filtered and unfiltered matches were reconciled" ], "claims": [ {"figure": "182 code files, 33,711 src LOC, 95 test files", "command": "git ls-files 'src/*.py' 'tests/*.py' 'scripts/*.py' | wc -l; git ls-files 'src/*.py' | xargs wc -l | tail -1"}, {"figure": "alpaca-py 0.43.2 sets no request timeout", "command": "inspect.getsource of RESTClient._request/_one_request; grep timeout alpaca/common/rest.py → self._session.request(...) at :194, no timeout"}, {"figure": "Tiingo downloader sets timeout=30.0", "command": "grep -n timeout src/thales/data/downloader.py → :93"}, {"figure": "Kelly returns double-book on retry", "command": "advocate repro against repo code: two selection attempts before _save_kelly_snapshot → period booked twice"}, {"figure": "daily-loss breaker skipped on degenerate equity", "command": "sed -n '226,228p' src/thales/execution/safety.py — eq_ok gates the -10% breaker"}, {"figure": "CI installs from the constraints lock", "command": "grep -rn 'requirements-ci.lock' .github/workflows/ → pip install . -c requirements-ci.lock"}, {"figure": "lock has no hashes", "command": "grep -c -- '--hash' requirements-ci.lock → 0"} ] } ``` ## Reviewer's notes thales is a rigorously engineered and, unusually, rigorously *self-audited* trading system. `TECH_DEBT.md` and `AUDIT.md` are not aspirational — they carry dozens of dated, dispositioned findings, most fixed and pinned as regression tests (the engine↔live parity harness anchors live construction byte-for-byte to a real backtest; a week-long silent failure has its own test). Order submission is idempotent by construction, the safety layer cites Knight Capital and is genuinely fail-closed on the paths that matter, and the EXQ-1 fix that stopped "one bad ticker dumping the book to market" is correct and bounded. The scanner's top four were mostly noise or already-handled (a committed constraints lock the signal couldn't see; test-fixture-driven purity signals), so I did what the procedure says on a hygienic repo: abandoned the ranking early and spent the budget reading the money path and briefing an adversary on it. That was the right call, because the one finding that matters was not detectable and was not in the audits: **the half-Kelly return pool double-books whenever a monthly selection day books its returns and then does not complete** — a halt, a stale panel, no signals, or (finding 2's hang) a mid-run SIGKILL — after which the next day's retry re-books the same holding period. The dedup guard exists (`daily.py:912`) but is keyed on the snapshot date, which only advances at the end of a *successful, non-halted* run, so it catches only a same-day forced re-run and misses the exact recovery flow the system is built around. The advocate reproduced it against the repo's own code. It is diluted in a 5,000-sample pool so I rate it moderate, but it silently biases the scalar that sizes the entire book, and the bias is toward adverse days because that is when halts and stale panels cluster. The two money-path findings compound in a way worth stating plainly: the Alpaca clients have no request timeout (finding 2), so a broker stall does not raise — it hangs until GitHub SIGKILLs the job, which (a) can land mid-order-loop, and (b) is itself a live trigger for the Kelly double-book. Fixing the booking's idempotency marker and putting a timeout on the broker clients closes the two most consequential silent-failure modes in the money path together, which is why they lead the report. Round 2 settled the fix shape: the dedup marker must key on the *period-start snapshot date*, not `today`; the booking must stay upstream of sizing (because `_size_targets` reads the freshly-booked pool); and it is independent of the F4 price-resolution. There is a small accepted residual (the retry keeps the first-attempt period endpoint rather than the exact engine-faithful one), but it is far less harmful than the current overlapping double-book. Nothing was left open. Where I'd look next with more time: the VRP options path beyond the geometry the advocate verified (the live mleg fill-sign reconciliation), and the auto-merge records gate — a self-modifying-workflow surface with its own recent panel findings, outside the money path but not outside the blast radius. --- ### 1 · `idempotent-operations` — priority 1 (rank —, no score) · operability > Anything that can be retried will be, so twice must end up the same as once. **Verified, advocate-found.** On a monthly selection day the run books the prior holding period's realized per-stock returns into the pooled half-Kelly sample *before* anything can fail: `_update_kelly_ledger_returns` is called at `daily.py:1453`, appends to `ledger["returns"]`, and persists immediately (`daily.py:930-937`). Its only double-book guard is `daily.py:912`: ```python if snapshot.get("date") == today.isoformat(): return # already booked this period's returns (e.g. forced re-run) ``` But `snapshot["date"]` is the *previous* selection's date — it is advanced to `today` only inside `_save_kelly_snapshot` (`daily.py:1740`), which runs at the very end of the run and is gated by `if not halted` (`daily.py:1732`). So the guard fires only for a same-day forced re-run *after* a successful completion. Every non-completion on a selection day runs *after* the booking and *never* reaches the snapshot advance: a safety HALT (the `if not halted` is false), a stale price panel (early return ~`daily.py:1489`, `selection_completed: False`), no signals (~`daily.py:1506`), or a crash / SIGKILL mid-run (finding 2). And `_is_selection_day` (`daily.py:501-510`) returns True for the retry because no run *this month* carries `selection_completed: True`. So the next day re-enters the selection block and re-books the same holding period. The advocate reproduced it against the repo's own code: two attempts before a successful snapshot advance leave the period booked twice. The consequence is real money: the pooled `returns` sample is the half-Kelly denominator that scales the *whole book* at the next selection (`_size_targets` → `build_target_weights(..., pooled_returns_chrono=ledger_returns)`). The duplication is biased, not random — halts and stale panels cluster on adverse market days, so the extra samples skew toward loss periods and pull the Kelly scalar around precisely because the month started badly (N halted days → the period booked N+1 times). It is silent (append-only, capped at 5000; no tripwire). Diluted in a large pool, hence moderate — but a genuine, reachable, silent corruption of a risk-sizing input, via the system's normal recovery flow, that the audits never caught. > **Remedy:** "Derive a stable key for the unit of work and store it alongside the effect in the same transaction, returning the recorded result when the key reappears. Use create-or-update rather than create." Applied here (fix shape resolved in round 2): gate the booking on a marker written **in the same `write_kelly_ledger` call** as the returns append, keyed on the **snapshot / period-start date it booked against** — `returns_booked_for_snapshot = snapshot["date"]`, skip when it equals the current `snapshot["date"]` — *not* on `today` (storing `today` reproduces the current defect: only same-day re-runs are caught). The advocate traced it: a halt at T1 books P0→T1 and sets the marker to T0; the T1 retry sees `booked_for(T0)==snapshot(T0)` and skips (no double-book); the next month's snapshot has advanced to a new period start, re-arming the guard, so a real period is never skipped. Two constraints the round-2 exchange established: the booking must stay *upstream* of sizing (it cannot move after the halt check, because `_size_targets` reads the freshly-booked pool at `daily.py:796`, called at `:1611`), and the fix is independent of the F4 price-resolution (F4 governs *what value* is booked; the marker governs *whether*). One accepted residual: skipping on the retry keeps the first-attempt endpoint (P0→T1) rather than the engine-faithful P0→T1′, truncating the halt-window days from one period's return — small, one-directional, and far less harmful than the current overlapping double-book. ### 2 · `design-for-partial-failure` — priority 2 (rank 2, score 0.993) · operability > Across a boundary you do not control, some things fail while others keep running, and nothing can tell you which. **Verified.** No Alpaca broker or market-data call has a per-request timeout. `AlpacaBroker` builds `TradingClient(...)` (`alpaca_broker.py:73`) and `StockHistoricalDataClient(...)` (`alpaca_broker.py:321`) bare, `vrp_daily.py:157` builds `OptionHistoricalDataClient(...)` bare, and alpaca-py (0.43.2, installed) sets none by default — its `RESTClient._request`/`_one_request` contain no `timeout` and the call is `self._session.request(...)` at `alpaca/common/rest.py:194`; `requests`' default is `None` = infinite. So `submit_market_order`/`submit_limit_order`/`get_open_orders`/`get_positions`/`get_latest_quotes`/reconcile and the VRP option-chain calls can all hang indefinitely. The contrast is the finding: the Tiingo data downloader *does* set `timeout=30.0` (`downloader.py:93`) — the guard exists for the data vendor but not the money path. The advocate sharpened the consequence. The only mitigation is the GHA `timeout-minutes: 20-30` on the paper-trading jobs, which kills the *whole job* — possibly mid-order-loop (`pipeline.py:321-359`), after some orders are submitted, before `log_run`/reconcile (and that mid-run kill is itself a trigger for finding 1). Worse, the idempotency design — deterministic `client_order_id`, duplicate-422 handling, retry-lookup-before-resubmit — assumes an ambiguous submit *raises* so the retry loop can look the order up. A stalled TCP connection never returns and never raises, so that entire path is dead code in exactly the hang scenario it was built for. > **Remedy:** "Give every call that crosses a boundary a deadline, and decide what the caller is promised when the deadline passes… Where the answer is that the caller cannot know, make the operation safe to repeat." Applied here: pass a request timeout to the Alpaca SDK clients (the SDK accepts one) so a stall *raises* — at which point the existing retry/idempotency machinery does exactly the right thing (look up by `client_order_id`, treat a duplicate as submitted). The operation is already safe to repeat; it just needs the deadline that turns a silent hang into the catchable, idempotent error the code already handles. Mechanical. ### 3 · `fail-safe-defaults` — priority 3 (rank —, no score) · operability > Forgetting an option should produce a no-op or a crash, never a silent destructive write. **Verified, advocate-found.** The daily-loss circuit breaker — the Knight-Capital guard the safety module's own docstring cites — is silently skipped when the account's `last_equity` is degenerate. `safety.py:226-228`: ```python last_eq = float(getattr(account, "last_equity", 0) or 0) eq_ok = math.isfinite(last_eq) and last_eq > 0 and math.isfinite(equity) and equity > 0 if eq_ok and not all_de_risking: day_ret = equity / last_eq - 1 if day_ret <= -limits.max_daily_loss_pct: # HALT ``` If `last_equity` is `0`/`None`/`NaN`, `eq_ok` is False and the −10% breaker never evaluates (the position, notional, and order-count caps still apply). This is a fail-*open* on one specific limit inside a layer that is otherwise rigorously fail-*closed*: broker-unreachable HALTs (`pipeline.py:255`), a gate-internal error HALTs (`safety.py:265`), account None/blocked/identity-mismatch HALTs (`safety.py:182-214`). The guard that was added to avoid dividing by a degenerate equity became a silent bypass of the check it guards — the recurring shape the advocate named. It is only reachable on an unfunded / never-traded account (where `last_equity` is legitimately 0), so `p_manifests` is low; but the safe default when you cannot read the prior equity is to HALT, because you cannot prove you are within the loss limit. > **Remedy:** "Invert the default so the destructive path requires an explicit flag… remove defaults from environment-specific settings so an unset value fails." Applied here: treat an unreadable/degenerate `last_equity` as a HALT reason rather than a skip — the same fail-closed posture every other unreadable account field already gets two functions up. One branch. Mechanical. ### 4 · `reproducible-builds` — priority 4 (rank 4, score 0.936) · dependencies > Same sources and declared tools in, same artifact out — on any machine, on any day. **Verified, narrowed.** `pyproject.toml` declares floating floors (`polars>=1.0`, `alpaca-py>=0.21`, …, `pyproject.toml:24-44`), but CI installs with `pip install . -c requirements-ci.lock` (a committed pip *constraints* file, used across the paper-trading and fleet-digest workflows), so **CI is version-reproducible** — the scanner's top-ranked "no lock committed" (0.995) is a detector miss that doesn't recognize a constraints file. The residual, confirmed by the advocate: `requirements-ci.lock` has no `--hash`/`--require-hashes` (so CI is version-pinned but not hash-verified), and a non-CI `pip install .` — a dev box, or a fresh prod venv provisioned by hand — floats to whatever exists that day, on the machine that trades real money. This is "reproducible-in-CI ≠ reproducible-on-the-box-that-trades." > **Remedy:** "Declare every input explicitly: tool and compiler versions, dependency hashes, source files." Applied here: add hashes to `requirements-ci.lock` (`pip-compile --generate-hashes` or equivalent) and install from it everywhere the trading code runs, not only in CI — so the deploy installs the same verified artifact the pipeline tests. Mechanical; the lock already exists, it just needs hashes and universal use. ### 5 · `enforce-invariants-in-one-place` — priority 5 (rank —, no score) · state > A rule checked in five methods is a rule that is wrong in at least one of them. **Verified, advocate-found, low-P but live.** When a marketable-limit order does not fully fill within `fill_timeout_minutes`, `_resolve_unfilled_limits` cancels the resting limit and re-submits the remainder as a market order (`daily.py:1207-1234`). That replacement re-checks only the manual/fleet halt (`daily.py:1155-1165`) — not a daily-loss breaker or a per-order reject that materialized *during* the (up to ~5-minute) fill wait. So the full safety gate that guards initial submission is enforced in one place but only *partially* re-enforced on the replace path: a market BUY remainder can go out after the −10% breaker tripped intraday. Round 2 established this is not latent — `config/settings.yaml:267` (momentum, the flagship sleeve) and `config/meanrev.yaml:153` both run `order_type: marketable_limit` (only `vrp.yaml` is `market`), so two live equity sleeves take this path on every rebalance where a touch-priced limit doesn't fully fill. A second facet shares the site: `remaining = shares − filled_qty` reads `filled_qty` (`daily.py:1202`) *before* the cancel (`daily.py:1208`); if the resting DAY limit fills more between the read and the cancel taking effect, the market replacement over-buys by the delta — a check-then-act race. The trigger is rare (a >10% intraday move or a fresh reject inside a minutes-long window, and it only bites a BUY remainder — a de-risking SELL remainder *should* complete), hence `p_manifests` is low; but it is a real gap on a live path, in the one place the otherwise-centralized safety gate is not fully re-applied. > **Remedy:** "Move the check into… the single mutating operation every path must call, delete the duplicated guards." Applied here: run the *full* safety gate (not just the halt check) on the market replacement, so the replace goes through the same chokepoint as the initial submission; and re-read `filled_qty` *after* the cancel is confirmed before sizing the remainder. `mechanical: false` — it re-routes the replace through the gate. ### Conflicts No two selected principles are in tension — `./canon tensions` returns no rows *among* `idempotent-operations`, `design-for-partial-failure`, `fail-safe-defaults`, `reproducible-builds`, and `enforce-invariants-in-one-place` (their tension rows all point at non-selected principles). The findings are mutually reinforcing: finding 2's remedy (a broker timeout that raises) is what makes finding 1's failure mode less frequent (fewer mid-run SIGKILLs) *and* keeps the idempotency machinery alive; findings 1, 3, and 5 are all "a guard keyed on the wrong condition or missing on one path," fixed the same way (assert the real invariant, fail toward safety). Nothing to hand the applying agent as a live dispute. ### Dropped after verification - `pin-external-dependency-versions` (0.995, ranked #1) — a `requirements-ci.lock` constraints file IS committed and CI installs against it; the raw "no lock" signal is a detector miss. The real residual survives as finding 4. - `bound-retry-load` (0.94) — the order retry uses exponential backoff (`pipeline.py`); the no-backoff sites are tests and the Tiingo downloader (which has its own 429/hourly-cap logic). - `prefer-pure-functions` (0.935) — evidence is test seeds (`np.random.default_rng(42)`), `datetime.now` in test helpers, and `.claude/` hooks; the engine core is a pure decision function pinned by the parity harness. - `separate-logic-from-io` (0.905) — `max_function_loc=956` and the print/open sites are research scripts and tests; execution is layered behind the `Broker` ABC and a state store. - the `reproducible-builds` *raw signal* evidence (absolute paths, hostname/date) is a per-user gitignored file and a report header — noise; the real gap was found by reading the lock. ### The exchange Two rounds; one advocate, continued. The review handed the advocate a deliberately thin draft (two findings, both hygiene-adjacent) and the mandate to read the money path for wrong answers. It returned the report's headline: the half-Kelly double-book, reproduced against the repo's own code, on the retry path — a finding neither the scanner nor the extensive in-repo audits had. It also sharpened the timeout finding into a real failure (the idempotency design is unreachable under a stall) and surfaced the daily-loss breaker fail-open. It conceded, with re-derivation, that the idempotency key is sound, that reconcile/backfill place no orders, that the VRP sign/geometry is correct, and that the safety layer is genuinely fail-closed on the paths that matter — an advocate that granted the strong parts is why the parts it contested carry weight. Round 2 closed the exchange: it resolved the Kelly fix shape (the marker must key on the period-start snapshot date, booking stays upstream of sizing because `_size_targets` consumes the freshly-booked pool, and it is F4-independent — with a small accepted endpoint-truncation residual), and it promoted one latent minor to a live low-P finding (the marketable-limit market-replace, reachable because two live sleeves run `marketable_limit`). Nothing was left open. ### Near misses - **VRP close self-reject on a legacy position file** (`vrp_daily.py:471-474`) — `_close_structure` defaults `short_strike`/`long_strike` to `1.0`; a position file missing strikes yields equal strikes → the shape validator rejects it ("distinct strikes", `safety.py:322`) and the close never goes out until expiry. Latent: post-activation files always carry strikes (`vrp_daily.py:774-775`). One line to surface it (route to advocate round 2 for reachability). - The **auto-merge records gate** (AMG panel findings in the log) — a self-modifying-workflow surface with its own recurring issues; outside the money path, flagged for the ops program. ### What's healthy Each verified by reading the cited code: - **Idempotent order submission by construction.** A deterministic `client_order_id = thales-{date}-{side}-{symbol}-{sha1(shares)[:6]}` (`pipeline.py:361-375`), a duplicate-422 handler that treats an already-existing order as submitted (`alpaca_broker.py:183-190`), a retry that looks the order up before resubmitting (`pipeline.py:426`), and an open-order skip (`pipeline.py:182`). The advocate confirmed no accidental cross-quantity collision. (Its one blind spot — a stall that never raises — is finding 2.) - **A genuinely fail-closed safety layer** that reads limits from broker account state, HALTs on broker-unreachable / gate-error / account-None / blocked / identity-mismatch (`pipeline.py:255`, `safety.py:182-267`), computes order direction by *risk* not order side (so an option credit-open counts as risk-increasing, `safety.py:216-225`), and cites Knight Capital as the reason it exists. (Its one leak — degenerate equity — is finding 3.) - **The EXQ-1 bounded quote fetch**: `_MAX_QUOTE_FETCH_CALLS = 24` so a pathological batch degrades only the un-fetched remainder to market, not the whole book, plus a class-share symbol map (`BRK-B`↔`BRK.B`) so one bad symbol no longer takes down the batched quote call. The "one bad ticker dumps the book to market" incident, correctly fixed and scoped. - **The engine↔live parity harness** anchors live construction byte-for-byte to a real `run_backtest` and pins every known divergence as a documented-divergence test (TECH_DEBT P1) — the load-bearing invariant of a system that validates one code path and trades another. - **A standing audit program** (`AUDIT.md`): daily silent-failure detection (cron ran, equity logged, book==broker reconcile, kill-switch state), a week-long-silent-failure regression test, runner-loss detection. The operability posture is the best in this batch. - **Dependency discipline**: `tests/test_declared_dependencies.py` fails if any imported package is undeclared (born from the real matplotlib-vanished-and-killed-the-digest incident documented in `pyproject.toml`). The reproducibility gap (finding 4) is narrow precisely because the rest of this is tight. ### Limits The per-dimension blind-spot table with bases is in `dimensions_swept`. Beyond it: I read the execution money path in depth — `pipeline.py`, `alpaca_broker.py`, `safety.py`, the `daily.py` Kelly/selection/snapshot machinery, VRP geometry via the advocate — and the dependency/CI configuration. I did **not** read the backtest engine internals (`engine.py`, `cpcv.py`, `evaluate.py` — the parity harness and the extensive CPCV audits are the evidence there), the `web/` TypeScript dashboard, the research scripts, or run the full test suite (large; CI runs it against the constraints lock on every push). Dynamic verification: the advocate reproduced the Kelly double-book against the repo's code and confirmed the alpaca-py timeout absence against the installed 0.43.2; I verified those line references and the safety fail-open by reading. The scanner's ranking was abandoned after the top cluster proved handled/noise (recorded in `dropped`), per the procedure. One fix-shape question is open (`unresolved`). ### Tool defects (canon, not this repository) Collected in `tool_warnings`: the #1-ranked `pin` signal doesn't recognize a committed pip *constraints* file, so it reported "no lock" on a repo whose CI is version-reproducible; `design-for-partial-failure` pointed at shell hooks and an internal `fetch()` rather than the untimed *broker* calls (found by reading); the `reproducible-builds` signals fired on a gitignored per-user file and a report header rather than the real hash-less-lock gap; and the purity/IO/retry signals over-weighted this repo's large `tests/` and `research/` trees. Separately, the standing database gaps did **not** bite here — the two highest findings (`idempotent-operations`, `design-for-partial-failure`) both had principles to attach to; the value the scanner missed was in *which* code the principles applied to, which is what reading and the advocate supplied.