From 9a988c5db11115965d695906cfb285a0acd33c53 Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Wed, 30 Sep 2026 06:14:26 -0400 Subject: [PATCH] fix(execution): a permissions refusal on a bracket leg is definite -- rejected and retried, not state-unknown (#945) R5 booked every post-row raise as state-unknown: the pending row stayed, the unbracketed: retry was cleared, and the product's exits waited on a human to reconcile a row no venue order backed. A TradeScopeDenied is not ambiguity -- the venue refused the order outright, so nothing rests at the exchange. The row now takes rejected (the status a broker-rejected placement already takes), the retry record is armed, and a CRITICAL executor.bracket_refused names the position; the next cycle's sweep re-places, and heals the moment the credential trades again. Timeouts and network raises keep R5's state-unknown booking. _run_order's refutation write (#233) is untouched. --- .../2026-09-28-dca-sleeve-sell-side-build.md | 2 + keel/execution/executor.py | 73 +++++++++++++++-- tests/execution/test_bracket_downgrade.py | 82 +++++++++++++++++-- tests/execution/test_reconcile.py | 40 +++++++++ 4 files changed, 184 insertions(+), 13 deletions(-) diff --git a/docs/superpowers/plans/2026-09-28-dca-sleeve-sell-side-build.md b/docs/superpowers/plans/2026-09-28-dca-sleeve-sell-side-build.md index 51ecbb38..0c89fbca 100644 --- a/docs/superpowers/plans/2026-09-28-dca-sleeve-sell-side-build.md +++ b/docs/superpowers/plans/2026-09-28-dca-sleeve-sell-side-build.md @@ -407,6 +407,8 @@ Each ruling is stated as `what — why — cost if wrong`. - Cost if wrong: a breach during a webhook outage is seen at the next `keel doctor` or preview, not on the phone. - **R77** (added in P16). P16's PR REFERENCES #857 and does not close it. #857 is the design issue. It was already closed when the spec merged (#859), and P17 and P18 still carry `(#857)` in their titles. The operator stopped the build after P16 (R28), so the gated placement path stays unbuilt and #857's scope is not complete. - Cost if wrong: none. The issue's state does not change either way. +- **R80** (added in the post-review fix PR, #945; the review's S-6, narrowing R5's unknown-state bucket). A `TradeScopeDenied` on a PROTECTED bracket leg is a DEFINITE refusal, not an ambiguous raise: the venue did not take the order, so nothing rests at the exchange. The written row takes `rejected` -- the status a broker-rejected placement already takes in `_run_order` -- the `unbracketed:` retry record is ARMED instead of cleared, and a CRITICAL `executor.bracket_refused` names the position. The next cycle's `reconcile_unbracketed_positions` retries instead of the product's exits waiting on a human to reconcile a phantom pending row. Every OTHER raise after the row exists (a timeout, a network error) keeps R5's state-unknown booking unchanged -- there the venue really may be holding the order. `_run_order`'s refutation write (#233) is untouched: a placement-path refusal is a credential fact; only its row bookkeeping changes. + - Cost if wrong: none found. The definite/ambiguous split follows the exception type itself; the one behavior given up -- a human being forced to look at a permissions-refused stop -- is replaced by a CRITICAL that says the same thing plus a retry that succeeds the moment the credential is fixed. --- diff --git a/keel/execution/executor.py b/keel/execution/executor.py index 6cef87a7..4625eab7 100644 --- a/keel/execution/executor.py +++ b/keel/execution/executor.py @@ -1305,9 +1305,10 @@ def _run_order( # # Written BEFORE the re-raise, and that ordering is the whole mechanism. NOTHING upstream # catches this except `place_bracket` for the bracket leg specifically (#799, R5): its - # broad `except Exception` around this same call swallows the re-raise on purpose, - # already having read this write via `_try_record_trade_scope_refuted` before doing so, - # so nothing is lost by the catch. Every OTHER caller is unguarded -- `agent.run_once` + # dedicated `TradeScopeDenied` handler (#945) and broad `Exception` handler around this + # same call swallow the re-raise on purpose, already having read this write via + # `_try_record_trade_scope_refuted` before doing so, so nothing is lost by the catch. + # Every OTHER caller is unguarded -- `agent.run_once` # does not wrap `executor.execute`, and neither does the `run_loop` above it, so the # exception leaves the process. (#233's design says a "cycle-survival handler" catches # it -- that handler is `_manage_stops`' per-tranche one, which this path does not pass @@ -2470,6 +2471,11 @@ def _floor_or_original(qty: Decimal, increment: Decimal) -> Decimal: #: may or may not be holding it, so nothing re-places it and the row stays `pending` for a human. BRACKET_STATE_UNKNOWN_EVENT = "executor.bracket_state_unknown" +#: #945: the bracket leg was refused OUTRIGHT on permissions -- a DEFINITE refusal, no order at +#: the venue. Unlike `BRACKET_STATE_UNKNOWN_EVENT` there is nothing for a human to reconcile: the +#: row is `rejected`, the `unbracketed:` retry is armed, and the sweep retries next cycle. +BRACKET_REFUSED_EVENT = "executor.bracket_refused" + def place_bracket( broker: Any, @@ -2507,8 +2513,14 @@ def place_bracket( - before the bracket's `orders` row exists (the spec cannot be built, or `_run_order` raised before writing one): the `unbracketed:` retry record, exactly as for a veto or a venue refusal; - - after it exists (`place_order` raised): NO retry record -- one already standing from an - earlier attempt is CLEARED -- the `pending` row stays, and a CRITICAL + - after it exists, with `TradeScopeDenied` (#945): a permissions refusal is DEFINITE -- the + venue did not take the order. The row is marked `rejected` (the status a broker-rejected + placement already takes in `_run_order`), the `unbracketed:` retry record is ARMED, and a + CRITICAL `executor.bracket_refused` names it: the position is unprotected, but nothing is + unknown, and the sweep retries next cycle instead of parking the product's exits behind a + pending row no venue order backs; + - after it exists (any other `place_order` raise): NO retry record -- one already standing + from an earlier attempt is CLEARED -- the `pending` row stays, and a CRITICAL `executor.bracket_state_unknown` names it. The venue may be holding that bracket, so a retry could double-commit the base; exits on the product fail closed on the row until a human reconciles it. @@ -2595,6 +2607,57 @@ def place_bracket( now_ts, spec=spec, ) + except TradeScopeDenied as exc: + # #945: a permissions refusal is DEFINITE -- the venue did not take the order, so there + # is nothing at the exchange and no unknown to reconcile. The generic handler below books + # ambiguity (a raise after the row was written MIGHT mean the venue is holding it); this + # exception says it is not. `_run_order` has already written the trade-scope refutation + # before re-raising, so nothing is lost by catching it here. + written = [ + o + for o in repo.get_orders(mode="live", product_id=product_id) + if o["id"] not in before and o["side"] == Side.SELL.value + ] + if not written: + # Refused at the PREVIEW, before any row existed: the pre-row refusal of the + # docstring's first bullet, whatever the exception -- nothing to reject. + repo.set_state( + f"{UNBRACKETED_PREFIX}{product_id}", + {"stop": stop, "target": target, "qty": qty}, + ) + log_event( + logger, + logging.WARNING, + "executor.bracket_not_placed", + product=product_id, + reason=f"the venue refused the bracket leg on permissions before it was " + f"placed: {exc!r}", + vetoed_by=[], + ) + else: + # The row this wrote is `pending`; the venue refused it outright, so `rejected` is + # the fact (the same status a broker-rejected placement takes). ARM the retry -- the + # sweep is driven from the ledger and `unbracketed:`, and a `rejected` row is not a + # resting one, so nothing blocks it from re-placing next cycle. + repo.update_order(written[-1]["id"], status="rejected", updated_at=now_ts) + repo.set_state( + f"{UNBRACKETED_PREFIX}{product_id}", + {"stop": stop, "target": target, "qty": qty}, + ) + log_event( + logger, + logging.CRITICAL, + BRACKET_REFUSED_EVENT, + product=product_id, + order_id=written[-1]["id"], + reason=repr(exc), + detail=( + "the venue refused the bracket leg outright on permissions: no order " + "rests at the exchange. The row is rejected and the sweep retries next " + "cycle; until one is accepted, the position has no stop" + ), + ) + return None except Exception as exc: # noqa: BLE001 -- see the comment above written = [ o diff --git a/tests/execution/test_bracket_downgrade.py b/tests/execution/test_bracket_downgrade.py index aee64f59..daa1cb3d 100644 --- a/tests/execution/test_bracket_downgrade.py +++ b/tests/execution/test_bracket_downgrade.py @@ -4,7 +4,8 @@ escape `executor.execute`. The stage decides the recovery: a throw BEFORE the bracket's `orders` row exists (the preview) writes the `unbracketed:` retry record; a throw AFTER it exists (`place_order`) leaves the `pending` row for a human and writes no retry, because the venue may be -holding the bracket. +holding the bracket -- EXCEPT a permissions refusal (#945), which is definite: the venue did not +take the order, so the row is `rejected`, the retry is armed, and the sweep re-places next cycle. """ from __future__ import annotations @@ -91,17 +92,15 @@ def test_a_bracket_preview_that_throws_after_a_filled_entry_downgrades( assert _events(caplog, executor.BRACKET_STATE_UNKNOWN_EVENT) == [] -@pytest.mark.parametrize( - "exc", - [TimeoutError("read timed out"), TradeScopeDenied("trade scope denied")], - ids=["timeout", "trade_scope_denied"], -) def test_a_bracket_place_that_throws_downgrades_without_a_retry( repo, # noqa: F811 caplog: pytest.LogCaptureFixture, - exc: Exception, ) -> None: - broker = _SellPlaceRaises(exc) + """An AMBIGUOUS raise (a timeout: the request may have landed) keeps the state-unknown + booking -- the venue may hold a resting bracket, so a retry could double-commit the base and + the pending row waits for a human. `TradeScopeDenied` is split out below (#945): it is not + ambiguous.""" + broker = _SellPlaceRaises(TimeoutError("read timed out")) with caplog.at_level(logging.WARNING, logger="keel.execution.executor"): result = executor.execute( @@ -125,6 +124,73 @@ def test_a_bracket_place_that_throws_downgrades_without_a_retry( assert repo.get_state("open_stop:BTC-USD") is None, "no stop is asserted for an unknown row" +def test_a_permissions_refusal_on_the_bracket_leg_is_definite_rejected_and_retried( + repo, # noqa: F811 + caplog: pytest.LogCaptureFixture, +) -> None: + """#945: `TradeScopeDenied` is a DEFINITE answer -- the venue did not take the order, so + nothing rests at the exchange and there is no unknown to reconcile. The row takes the status + a broker-rejected placement already takes (`rejected`), the `unbracketed:` retry is ARMED + rather than cleared, and the next cycle's sweep heals with a working credential instead of + parking the product's exits behind a phantom pending row.""" + broker = _SellPlaceRaises(TradeScopeDenied("trade scope denied")) + + with caplog.at_level(logging.WARNING, logger="keel.execution.executor"): + result = executor.execute( + _enter_signal(), broker, repo, _config(), "autonomous", now_ts=NOW_TS + ) + + assert broker.sell_places == 1, "the bracket leg must actually have reached place_order" + assert result.placed is True + assert result.bracket_order_id is None + [refused_sell] = [o for o in repo.get_orders(mode="live") if o["side"] == "SELL"] + assert refused_sell["status"] == "rejected", "the refusal is definite, not unknown" + retry = repo.get_state(f"{executor.UNBRACKETED_PREFIX}BTC-USD") + assert retry is not None, "the sweep must be able to retry next cycle" + assert _events(caplog, executor.BRACKET_STATE_UNKNOWN_EVENT) == [] + [critical] = _events(caplog, executor.BRACKET_REFUSED_EVENT) + assert critical.levelno == logging.CRITICAL + fields = getattr(critical, _FIELDS_ATTR) + assert (fields["product"], fields["order_id"]) == ("BTC-USD", refused_sell["id"]) + assert repo.get_state("open_stop:BTC-USD") is None, "no stop rests anywhere yet" + + +def test_a_permissions_refusal_before_the_row_exists_arms_the_retry( + repo, # noqa: F811 + caplog: pytest.LogCaptureFixture, +) -> None: + """The other stage: refused at the PREVIEW, before any row was written. That case was + already correct (the retry record, like any pre-row refusal); it stays correct under the + dedicated handler, and no row is written to reject.""" + + class _SellPreviewDenied(FakeBroker): + def __init__(self) -> None: + super().__init__() + self.sell_previews = 0 + + def preview_order(self, spec: OrderSpec) -> Preview: + if spec.side is Side.SELL: + self.sell_previews += 1 + raise TradeScopeDenied("trade scope denied") + return super().preview_order(spec) + + broker = _SellPreviewDenied() + + with caplog.at_level(logging.WARNING, logger="keel.execution.executor"): + result = executor.execute( + _enter_signal(), broker, repo, _config(), "autonomous", now_ts=NOW_TS + ) + + assert broker.sell_previews == 1 + assert result.placed is True + assert [(o["side"], o["status"]) for o in repo.get_orders(mode="live")] == [ + ("BUY", "filled") + ], "a preview-stage refusal writes no bracket row" + assert repo.get_state(f"{executor.UNBRACKETED_PREFIX}BTC-USD") is not None + assert _events(caplog, executor.BRACKET_STATE_UNKNOWN_EVENT) == [] + assert _events(caplog, executor.BRACKET_REFUSED_EVENT) == [] + + def test_a_bracket_that_places_still_returns_its_order_id(repo) -> None: # noqa: F811 """The control: the downgrade must not swallow the success path.""" broker = FakeBroker() diff --git a/tests/execution/test_reconcile.py b/tests/execution/test_reconcile.py index fdb89506..66f8132d 100644 --- a/tests/execution/test_reconcile.py +++ b/tests/execution/test_reconcile.py @@ -16,6 +16,7 @@ import pytest from keel_broker_api.orders import OrderSpec +from keel_broker_api.port import TradeScopeDenied from keel_broker_api.results import Balance, OrderStatus, PlaceResult, Preview from keel_core.telemetry import _FIELDS_ATTR @@ -1373,6 +1374,45 @@ def test_a_retry_whose_placement_state_is_unknown_is_not_retried_again(repo, cap assert getattr(escalation, _FIELDS_ATTR)["retry_scheduled"] is False +class _TradeScopeDeniedRebracketBroker(_RebracketingBroker): + """The venue refuses the bracket's PLACEMENT outright on permissions (#945) -- definite: + no order rests at the exchange, whatever a raise after the row would usually imply.""" + + def place_order(self, spec: OrderSpec, *, idempotency_key: str | None = None) -> PlaceResult: + self.placed.append({"spec": spec}) + raise TradeScopeDenied("trade scope denied") + + +def test_a_permissions_refusal_is_definite_so_the_sweep_retries_and_heals(repo, caplog): + """#945, the mirror of the state-unknown case above: a `TradeScopeDenied` is not ambiguity + to park on a human. The row is `rejected` (not `pending`), the `unbracketed:` record + SURVIVES, the refusal gets its own CRITICAL -- and once the credential trades again, the + next sweep heals, because a rejected row blocks nothing.""" + _seed_unbracketed_tranche(repo) + _allow_orders(repo) + denied = _TradeScopeDeniedRebracketBroker() + + with caplog.at_level(logging.CRITICAL): + reconcile.reconcile_unbracketed_positions(denied, repo, _config(), now_ts=NOW) + + assert len(denied.placed) == 1 + [refused] = repo.get_orders(mode="live", product_id=PRODUCT, status="rejected") + assert refused["side"] == Side.SELL.value + assert repo.get_state(f"unbracketed:{PRODUCT}") is not None, "the retry must survive" + events = [r.getMessage() for r in caplog.records if r.levelno == logging.CRITICAL] + assert events.count("executor.bracket_refused") == 1 + assert events.count("executor.bracket_state_unknown") == 0 + [escalation] = [r for r in caplog.records if r.getMessage() == "reconcile.position_unprotected"] + assert getattr(escalation, _FIELDS_ATTR)["retry_scheduled"] is True + + healed = reconcile.reconcile_unbracketed_positions( + _RebracketingBroker(), repo, _config(), now_ts=NOW + ) + + assert healed == [repo.get_open_positions(PRODUCT)[0]["id"]] + assert repo.get_state(f"unbracketed:{PRODUCT}") is None, "healed" + + def test_a_tranche_with_a_resting_bracket_is_left_alone(repo): """Already protected. Re-placing would commit inventory the resting bracket already holds and be refused for insufficient funds -- turning a healthy position into a naked one."""