# 2026-08-29 — External design review: maintainer-side verification

**Provenance.** An external design review of this repo at `24cfe0b` arrived
2026-08-29 and is committed verbatim as
`research/2026-08-29_external_design_review.md`. This memo is the
maintainer-side verification: every load-bearing claim re-checked against the
code and — where the reviewer explicitly could not look — against committed
state. Verdict up front: **all five findings are real**; the headline one had
**already fired in production**, which the reviewer could not see and
correctly routed to the maintainer.

## The live confirmation the reviewer couldn't reach (finding 1)

The review reproduced the Kelly double-book in a harness and flagged
`p_manifests ≈ 0.4`. The gitignored-state caveat cut the other way: meanrev
selects **daily**, safety-HALTed on 2026-08-19 and 08-20 (the RWG-1 runaway
guard), and its ledger is committed. Per-commit history of
`data/state/meanrev/kelly_ledger.json`:

| run | returns count | Δ booked | snapshot date after |
|---|---|---|---|
| 08-18 (ok) | 2,865 | +186 | advances → **08-18** (195 names) |
| 08-19 (**HALT**) | 3,060 | **+195** — P(08-18→19) | frozen at 08-18 |
| 08-20 (**HALT**) | 3,255 | **+195 again** — P(08-18→20), overlapping | frozen at 08-18 |
| 08-21 (ok) | 3,450 | **+195 a third time** — P(08-18→21), overlapping both | advances → 08-21 |

One period start, booked three times: **390 excess overlapping samples ≈
10.6% of meanrev's 3,668-sample pool**, skewed toward the adverse stretch —
exactly the bias direction the review predicted, because halts cluster there.

Two mitigations, both verified:

- **meanrev has `sizing: "none"`** (`config/meanrev.yaml:88`) — the control
  sleeve's ledger is advisory; the contamination sized nothing.
- **momentum's ledger is clean** — exactly one booking commit per selection
  day (2026-06-11, 07-01, 08-03), no retried selection day has ever occurred
  on the flagship.

Which is also why the review's priority-1 call stands: momentum *does* size
from this pool, its next selection day is **Tue 2026-09-01**, and the week of
08-25 demonstrated the exact triggers (a ~9-hour GHA cron slide, a run
crossing UTC midnight, a SIGKILL-class capture failure).

**Contamination disposition: recorded, not rewritten.** The 390 excess
samples stay in meanrev's ledger — it sizes nothing (`sizing: "none"`), and
hand-editing committed state to make history prettier is what this shop does
not do. Caveat for any future reader: the pool caps at 5,000, so
contamination also **evicts** real samples; if meanrev's ledger ever came to
matter it needs a rebuild from order history, not a trim.

## Per-finding verification

| # | review's claim | verification | disposition |
|---|---|---|---|
| 1 | Kelly booking double-books on failed-selection retry (`daily.py:912` guard keyed on `today`; snapshot advance gated `if not halted`) | **Confirmed by reading** every cited line, then **confirmed live** (table above). `log_run` writes `selection_completed: not halted`, so a halted selection re-enters — exactly as claimed | **Fixed this PR** — `returns_booked_for` marker, same atomic write |
| 2 | No request timeout on any Alpaca client; a stall hangs to the GHA job kill; the idempotency path never fires because a hang never raises | **Confirmed**: `TradingClient.__init__`/`RESTClient.__init__` signatures (installed 0.43.2) accept no timeout; zero `timeout` occurrences in `alpaca/common/rest.py`; Tiingo path has `timeout=30.0`, money path had none | **Fixed this PR** — with a corrected shape (below) |
| 3 | Daily-loss breaker silently skipped when `last_equity` is 0/None/NaN — fail-open in a fail-closed layer | **Confirmed by reading** `safety.py:226-231` | **Fixed this PR** — degenerate equity now HALTs risk-increasing batches; de-risking exemption preserved |
| 4 | `requirements-ci.lock` has no hashes; non-CI installs float | **Confirmed**: `grep -c -- '--hash' requirements-ci.lock` → 0 | **Queued** (`queue/open/lck-1`) — deliberately after the trip; see the row for why |
| 5 | The marketable-limit market-replace re-checks only the manual/fleet halt; `filled_qty` read pre-cancel (over-buy race); live on two sleeves | **Confirmed by reading** `daily.py` (`_resolve_unfilled_limits`); both sleeve configs are `marketable_limit` | **Queued** (`queue/open/rpl-1`) — sharper framing below |
| — | Near-miss: VRP close self-rejects on a legacy position file missing strikes (defaults 1.0/1.0 → equal-strike shape reject); routed to maintainer | **Checked**: the live `data/state/vrp/vrp_position.json` carries real strikes (748/747) | Latent-only on current state; noted, no action |

## Corrections to the review (it holds up; these sharpen it)

1. **Finding 2's remedy line is factually wrong in its easiest claim** —
   "pass a request timeout to the SDK clients (the SDK accepts one)". It does
   not: alpaca-py 0.43.2 exposes no timeout parameter anywhere. The finding
   is untouched; the fix is a deadline injected at each client's
   `requests.Session` (`_apply_default_timeout`, this PR) — still small, but
   it needed a fake-session test, not a kwarg.
2. **Finding 5 is stronger than its principle framing.** The replace path
   calling `broker.submit_market_order` directly is not merely an
   enforce-invariants miss — it violates the repo's own standing covenant
   (2026-06-06, `memory/execution-safety-layer`): `pipeline.execute_orders`
   is THE fail-closed chokepoint and *"never add an order path that bypasses
   it."* The queue row carries the covenant citation.
3. **Finding 3's reachability is broader than "unfunded accounts"** —
   `float(getattr(account, "last_equity", 0) or 0)` also swallows an SDK
   field rename or a partial response. Still low-P; same fix.
4. **Finding 4's "the box that trades real money" overstates** — trading
   runs on GHA, which installs from the lock. The true residual is
   hash-verification plus local-parity. Correctly ranked last.
5. **"A finding the audits never caught" needs precision — and the precise
   version is more instructive.** An earlier execution audit hit this *site*
   twice ("Kelly phantom returns"; an "idempotency gap" — the origin of the
   `:912` guard). The guard fixed the same-day-re-run case and missed the
   retry case. Guards written for the narrow reproduction of an incident,
   rather than for the invariant, miss the sibling path.

## The family observation (for the panel)

Three findings in two weeks are one class: **state advanced, or keyed,
without regard to whether the run completed.**

- RWG-1 leg 2 — the turnover-blend base advances on a HALT;
- STAMP-1 (fired live 2026-08-27/28) — the runner's UTC date is both the
  trading-day stamp and the idempotency key, so a cron slide across midnight
  burned a capture day;
- the Kelly booking (this review) — returns persist immediately, the
  period marker only on successful completion.

The class predicts its next member: any write that pairs "effect now" with
"marker later," or keys either on wall-clock rather than the unit of work.
Worth a deliberate sweep with that lens rather than waiting for instance
four to fire.

## What shipped vs. queued (all owner-directed, recorded as such)

- **Shipped this PR** (each negative-controlled — the guard reverted, only
  its own tests red): finding 1 (booking marker, both guards kept — the
  `:912` today-guard still owns the same-day-after-success case), finding 2
  (session deadline on all three Alpaca clients: trading, stock data, VRP
  option data), finding 3 (fail-closed breaker).
- **Queued**: RPL-1 (finding 5 — order-path rewiring deserves an unhurried
  pass; exposure is a minutes-wide, BUY-only window), LCK-1 (finding 4 —
  after the trip, because regenerating the lock re-resolves versions and
  perturbing CI's install inputs before a selection day is its own risk).
- **Routing note**: built on the owner's direct instruction in an
  interactive session, not via `queue/approved/` — same route and same
  honesty note as EXQ-1 (2026-08-26).