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)

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.