# IGD-1 — five guards that exist and cannot fire: a bool-returning call wrapped in an unreachable `except` with the return discarded — at the ONE submit site outside the safety gate, the mid-run halt, and three alert emails; the test that "pins" the first one teaches a mock a failure no broker has; a run whose orders all failed exits 0 status: open · raised 2026-09-05 (external design review r2, findings 1 + 3 — advocate-found; maintainer-verified by reading every site and by a read-only broker-truth pull) · class: execution-safety + loudness (live path; a fail-closed control that does not close) · judgement: **YES** · effort ~0.25 pd (five one-liners, one test rewrite, one exit code) · horizon: the precondition fires daily on two sleeves — 546 cancel-replacements in 35 meanrev sessions, 39 on momentum — the consequence has been observed **0 times in 544 checked** --- **Plain-language summary for an owner reading one paragraph.** When a touch-priced limit order sits unfilled past the fill window, the code cancels it and re-sends the remainder as a market order. The comment around the cancel says exactly the right thing: if the cancel is not confirmed, do not replace, because the resting order might still fill and you would buy twice. But the guard is written to catch an *exception*, and none of the three broker implementations ever raises from `cancel_order` — they all return `False` — so the guard is dead code and a refused cancel would be followed by a full market replacement anyway. The same shape appears four more times: the mid-run halt "cancels" resting limits with the same discarded bool (so a halt does not actually stop them), and three alert emails — including the NO-RUN escalation whose own comment says the alert channel must never eat the exit code — discard `send_email`'s `False`. The unit test that exists to pin the cancel guard passes only because it tells a mock to raise, which no real broker does. Finally, a run in which every de-risking sell was rejected prints `Orders FAILED: N` and exits 0, green. Broker truth (below) says the cancel has succeeded every one of the 544 times it was tried — so this has not cost money — and the fix is five `if not` lines. ## Mechanism — verified at HEAD `cd571b2` (source unchanged through `3779020`) | site | call | on failure it returns | what the code does with that | |---|---|---|---| | `daily.py:1236` | `broker.cancel_order(r["order_id"])` | `False` — `alpaca_broker.py:421-427` catches `Exception`, logs, returns `False`; `simulated.py:195-200` returns `False`; `broker.py:125-127` base contract returns `False` ("True if the cancel was accepted") | bare statement; `except Exception` at `:1237` unreachable; falls through to `submit_market_order(sym, remaining, ...)` at `:1252` — `remaining` is the pre-cancel read, so a resting order that then fills double-fills | | `daily.py:1190` | `broker.cancel_order(...)` in the mid-run-halt branch | `False` | bare statement, `except Exception: pass`; resting DAY limits stay live at the broker for the rest of the session after a fleet/manual halt engaged mid-run | | `cli.py:3641`, `:3696`, `:4405` | `send_email(...)` | `False` — `notifications.py:74-76` catches every SMTP failure and unset credentials | bare statement; at `:3696` inside `try/except Exception as e: # the alert channel must never eat the exit code` — unreachable; the loudest alert in the CLI fails silently (`raise typer.Exit(2)` still reds the workflow, so GitHub's mail is the backstop) | | `cli.py:1130-1138` | `result.orders_failed` | — | prints the list, then `"Daily run complete."`; only `critical_skips = {"No signals", "Stale price panel"}` (`:1107`) exits non-zero | The two neighbouring branches at the same site are *correct*, which is why this 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 branch cancels without chasing. Only the cancel-failure branch reads the wrong signal. ## What broker truth says (read-only pull, 2026-09-05; the residual the reviewer could not measure) meanrev, 45-day window 07-23 → 09-04, 3,255 broker orders: - 546 local replacements (`market_replace_of` set), 544 inside the window; - **544 / 544 original limit orders: status `canceled`, `filled_qty` 0**; - **0** cases where original + replacement fills exceed the intended quantity; - observed cancel-refusal rate **0 / 544**. The review's `p_manifests 0.5` is the probability that a replacement *occurs*, not that a refused cancel is followed by one; the consequence probability is far lower. The fix is unchanged — the guard's comment promises something the code cannot deliver. - Side observation, filed as **OSR-1**: 130 of the 544 *replacements* were `rejected` by the broker at the next pre-market while the local log records `OrderStatus.ACCEPTED` for all 130. ## Why the existing test passes for the wrong reason (finding 3) `tests/test_execution/test_daily.py:560-576` (`test_unfilled_limit_cancel_failure_skips_replacement`) configures `broker.cancel_order.side_effect = RuntimeError("cancel rejected")` on a `MagicMock`, then asserts `submit_market_order.assert_not_called()`. Raising is a behaviour no `Broker` in the repository exhibits, so the assertion passes for a reason unrelated to production. The repo already owns the right tool — `SimulatedBroker` is a real in-memory fake whose `cancel_order` of an unknown id returns `False` — and had the test used it, the assertion would have failed the day it was written. Generalisable: *a guard whose only test violates the interface it guards is not a tested guard.* ## Relationship to open rows - **RPL-1** owns the same function: the gate bypass and the check-then-act race (a fill landing between the status read and the cancel). This is the deterministic third facet — no race needed, only a refused cancel. Filed separately because its fix is five one-liners that can land in an afternoon, while RPL-1 is a plumbing rewire the row itself schedules for an unhurried week; if folded at triage, the `cli.py` sites and the test rewrite are what must not be lost. - **FOS-1** owns per-symbol failed-order streaks in the digest; the exit-0 leg here is the process-level half of the same blindness (and **OSR-1** the broker-side half). ## Fix shape (propose-only) 1. `if not broker.cancel_order(r["order_id"]): logger.error("cancel not confirmed for %s — skipping replacement (fail closed)"); continue` at `daily.py:1236`, keeping the existing `except` for genuine transport errors; the equivalent at `:1190` (log the un-cancelled ids; never `pass`). 2. `if not send_email(...): console.print("[yellow]... alert email failed — see workflow log[/yellow]")` at the three `cli.py` sites; the NO-RUN escalation still exits 2. 3. Exit non-zero (a distinct code) when `result.orders_failed` is non-empty and the run is not a dry run. Safety REJECTs already persist via `_record_safety_event`; this makes the run's own exit agree with them. 4. Rewrite the test against `SimulatedBroker`: a resting unfilled limit whose cancel returns `False` → no replacement. Land fix 1 first so the rewritten test goes green for the right reason. 5. The dual sweep as a test-suite lint, proposed by the review and agreed by its advocate but not built: for every `MagicMock` attribute given an exception `side_effect` under `tests/`, check whether any concrete implementation of that method can raise. Every hit is a guard whose test is testing the mock. ~1 h. ## Test design + negative control (CQA-1 doctrine) - cancel returns `False` → `submit_market_order` not called (**red today** — this is the reviewer's executed reproduction: `call('VAL', 5.0, 'buy', client_order_id='cid-9-mr')` on unfixed code). - cancel returns `True` → replacement for `remaining` submitted (unchanged). - halt branch with a `False` cancel → the un-cancelled order id is logged at ERROR; revert → silent. - `send_email` returns `False` → the console carries the failure line and the exit code is unchanged; revert → nothing printed. - `orders_failed` non-empty → exit non-zero; revert → exit 0. ## Kill criterion — pre-registered Sites 1–2 and the test cannot be wrong-diagnosed by reading (three implementations, one base contract, one bare statement). The only kill is a ruling: if the owner rules exit-0-on-failed-orders deliberate (e.g. because the digest and the safety event already carry it), leg 3 is dropped with the ruling recorded and legs 1, 2, 4 stand. ## Cap note for triage Fold partner **RPL-1** (same function, same fix-sitting for the `daily.py` sites). The `cli.py` sites, the exit code and the test rewrite must survive any fold. --- ## Built — recorded 2026-09-05 (the night the row was raised); ONE LEG HELD **Shipped (code PR #151, follows the records PR #150):** - **Leg 1 — the two cancel sites.** `daily.py` `_resolve_unfilled_limits`: the cancel-replace branch reads the bool — `if not cancelled: log ERROR "cancel NOT confirmed … skipping replacement (fail closed)"; continue` — and keeps the `except` for genuine transport errors; the mid-run-halt branch logs at ERROR (with the order id) instead of `pass` when the cancel is refused or raises. - **Leg 2 — the three alert emails.** `cli.py` fleet-digest FAILED, NO-RUN escalation, and per-sleeve digest FAILED: `if not send_email(...):` prints a yellow "could not be sent / escalation email failed (send_email returned False)" line. Exit codes unchanged (1 / 2 / 1). - **Leg 4 — the test.** `test_unfilled_limit_cancel_failure_skips_replacement` is parametrized over `returns_false` (what production produces) and `raises` (the transport branch), and two new tests run the pass against the repo's own fake: a `SimulatedBroker` whose resting limits are queryable — the positive control replaces the remainder at market; the refusing variant (`cancel_order → False`) leaves no replacement, no position, the limit still resting. The mid-run-halt test now asserts the refused cancel is logged. Two `observability` tests pin the False-return reporting on the alert sites. With the source reverted, the refusing-fake test buys 5 VAL (the double fill) and the parametrized `returns_false` case submits the replacement. **HELD for the owner's ruling — leg 3, exit non-zero on `orders_failed`.** Consequence the owner must weigh before it ships: meanrev has failed to sell AVB for nine-plus consecutive sessions (FOS-1), so "red the run on any failed order" would red the meanrev workflow every day — and fire its failure alert — until that name is resolved. Options: (a) red on any failed order; (b) red only when a *de-risking sell* failed (the case that matters for the safety story; buys that fail leave the book under-invested, not exposed); (c) keep exit 0 and rely on the digest's `!! N FAILED` line plus FOS-1's streak detector. Recommendation: (b). Not decided by the maintainer. **Not built — leg 5,** the dual sweep as a test-suite lint (mocks given an exception `side_effect` for a method no implementation raises). ~1 h; a follow-up, not part of this fix. **Broker truth at build time (2026-09-05):** 0 refused cancels in 544 — the guard had never been needed. It is real now. **Route — recorded honestly:** built on the owner's direct instruction in the 2026-09-05 interactive session, NOT via `queue/approved/`; the guard denies the open→built rename, so a human merges. RPL-1 (gate bypass + race at the same site) stays open and untouched. --- ## Leg 3 — ruled and built 2026-09-06 (code PR #154) The owner's ruling on the held leg: **escalate NEW de-risking failures, report standing streaks.** `thales run` exits 4 when a sell (or an order-intent close) fails for a symbol that was not already failing in the previous session — exposure left on for the first time; the workflow's failure alert fires. A symbol still failing from the previous session is printed as a standing failure and does not red the run: that is the digest's streak line (FOS-1, built in the same PR). Unreadable failure history is treated as "everything is new" — classification must never hide a failure. Pinned at the source by `test_run_command_routes_failed_orders_through_the_rule_and_exits_4`; the rule itself by `test_escalation_rule_only_fresh_de_risking_failures`. Leg 5 (the dual-sweep lint) remains not built.