From 46747de25e4d4fcedfb08d397b09bd152193b4e8 Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Wed, 26 Aug 2026 18:44:39 -0400 Subject: [PATCH] fix(executor): refuse a stop roll that reaches the target, and record why #502 stage 2 is blocked MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Started on #502 stage 2 -- "the executor still builds its raw trigger_bracket_gtc dict against the pre-port CoinbaseClient, and moving it is stage 2's job" -- and found it cannot be done as scoped. What CAN be delivered now is the guarantee the migration would have bought, so this ships that. ── WHY STAGE 2 IS BLOCKED ───────────────────────────────────────────────────── The port's signature is `place_order(spec: OrderSpec, *, idempotency_key=None)`. The executor calls `broker.place_order(product_id, side, order_configuration)` -- three positional arguments and a raw venue dict, which is the PRE-PORT CoinbaseClient's shape. The executor is not on the port at all; it imports only `keel_broker_api.results`, and every order kind it places (`_order_configuration` for market and limit, `_bracket_order_configuration` for the bracket) is a hand-built Coinbase dict. So moving the bracket alone has two options and both are wrong: * translate in the executor -- requires importing `keel_broker_coinbase` into `keel/execution/`, which is the layering regression the port exists to prevent; or * migrate the executor/broker boundary to specs -- which is #524, and touches every live order path, not the bracket. #502 stage 2 is therefore gated on #524. Recorded in the issue rather than worked around. ── THE GAP THAT DID NOT NEED THE MIGRATION ──────────────────────────────────── `BracketGTC.__post_init__` refuses a stop at or above the take-profit -- "a coin flip wearing a protective order's name", since the two exits then race at the same level and whichever the venue evaluates first decides profit or loss. `_roll_stop` has never checked it. It guards against WIDENING (`new_stop < prior_stop`) and against a missing target, and then places whatever it was given. Reachable rather than theoretical: `trail_stop_atr` computes `price - atr * multiplier`, and the live agent cycles ONCE A DAY, so a gap through the target that reconciliation has not caught up with leaves a recorded target below the newly computed stop. Refusing is the conservative half: the roll is abandoned and the EXISTING bracket stays in force, so the position keeps the protection it already has. The alternative is cancelling a working bracket to install an inverted one -- and if the venue refuses that, the position is naked until the next sweep. `>=`, not `>`. Equal is the subtler half: two equal prices read as an ordinary pair of numbers and describe a stop and a target racing at the same price. `BracketGTC` refuses equal legs as firmly as inverted ones; so does this. Both tests mutation-checked -- removing the guard fails two, weakening `>=` to `>` fails the equal-legs one. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01T6yA5khYnJ2qzheArRToQ2 --- keel/execution/executor.py | 30 ++++++++++++ tests/execution/test_executor.py | 81 ++++++++++++++++++++++++++++++++ 2 files changed, 111 insertions(+) diff --git a/keel/execution/executor.py b/keel/execution/executor.py index 35f88ad..18fb9bf 100644 --- a/keel/execution/executor.py +++ b/keel/execution/executor.py @@ -1689,6 +1689,36 @@ def _roll_stop( ) return None + # A stop AT OR ABOVE the target is not a tighter stop; it is a coin flip. + # + # The replacement is a single native bracket carrying both prices, so a stop that has caught + # up with the target describes two exits racing at the same level, where whichever side the + # venue evaluates first decides whether this position took a profit or a loss. + # `keel_broker_api.orders.BracketGTC` refuses exactly this shape at construction -- "a coin + # flip wearing a protective order's name" -- but the live path does not build one of those + # yet (#502 stage 2 is blocked on #524's port migration), so nothing between the ratchet and + # Coinbase has been checking it. + # + # Reachable rather than theoretical: `trail_stop_atr` computes `price - atr * multiplier` and + # the live agent cycles ONCE A DAY, so a gap through the target that has not yet been + # reconciled leaves a position whose recorded target sits below the newly computed stop. + # + # Refusing here is the conservative half: the roll is abandoned and the EXISTING bracket stays + # in force, so the position keeps the protection it already had. Placing the inverted pair + # instead would cancel a working bracket to install a coin flip -- and if the venue refused it, + # leave the position naked until the next sweep. + if new_stop >= target: + log_event( + logger, + logging.WARNING, + "executor.stop_roll_refused", + product=product_id, + new_stop=new_stop, + target=target, + reason="new_stop is at or above the target -- the existing bracket stays in force", + ) + return None + # THE CRASH LEDGER, written BEFORE the venue is touched (#519). # # Everything below this line can die mid-flight, and until this record existed one of those diff --git a/tests/execution/test_executor.py b/tests/execution/test_executor.py index 4584344..f17d09c 100644 --- a/tests/execution/test_executor.py +++ b/tests/execution/test_executor.py @@ -1024,6 +1024,87 @@ def test_roll_to_break_even_never_widens_the_stop(repo): assert repo.get_state("open_stop:BTC-USD") == Decimal("49000") +def test_a_roll_that_reaches_the_target_is_refused_and_the_bracket_stays(repo): + """**A stop at or above the target is not a tighter stop; it is a coin flip.** + + The replacement is a single native bracket carrying both prices, so a stop that has caught up + with the target describes two exits racing at the same level -- whichever side the venue + evaluates first decides whether this position took a profit or a loss. + `keel_broker_api.orders.BracketGTC` refuses exactly this at construction, but the live path + does not build one yet (#502 stage 2 is blocked on #524), so nothing between the ratchet and + Coinbase was checking it. + + Refusing is the conservative half, and this asserts that half: the roll is abandoned, the + EXISTING bracket is untouched (`pending`, not `canceled`), and the recorded stop is unchanged, + so the position keeps the protection it already had. The alternative -- cancel a working + bracket to install an inverted one -- risks a venue refusal that leaves the position naked. + """ + broker = FakeBroker() + stop_id = place_bracket( + broker, + repo, + _config(), + product_id="BTC-USD", + qty=Decimal("0.01"), + stop=Decimal("49000"), + target=Decimal("53000"), + rule_name="pullback_continuation", + now_ts=NOW_TS, + ) + + # A break-even roll to a price ABOVE the recorded target. It tightens (53500 > 49000, so the + # ratchet is satisfied) and is still nonsense. + result = roll_to_break_even( + broker, + repo, + _config(), + product_id="BTC-USD", + old_stop_order_id=stop_id, + entry_price=Decimal("53500"), + qty=Decimal("0.01"), + rule_name="pullback_continuation", + now_ts=NOW_TS + 100, + ) + + assert result is None + assert repo.get_order(stop_id)["status"] == "pending", "a working bracket was cancelled" + assert repo.get_state("open_stop:BTC-USD") == Decimal("49000") + assert repo.get_state("open_target:BTC-USD") == Decimal("53000") + + +def test_a_roll_exactly_onto_the_target_is_refused_too(repo): + """`>=`, not `>`. Equal is the subtler half: two equal prices read as an ordinary pair of + numbers, and what they describe is a stop and a target racing at the SAME price. `BracketGTC` + refuses equal legs as firmly as inverted ones, for the same reason.""" + broker = FakeBroker() + stop_id = place_bracket( + broker, + repo, + _config(), + product_id="BTC-USD", + qty=Decimal("0.01"), + stop=Decimal("49000"), + target=Decimal("53000"), + rule_name="pullback_continuation", + now_ts=NOW_TS, + ) + + result = roll_to_break_even( + broker, + repo, + _config(), + product_id="BTC-USD", + old_stop_order_id=stop_id, + entry_price=Decimal("53000"), + qty=Decimal("0.01"), + rule_name="pullback_continuation", + now_ts=NOW_TS + 100, + ) + + assert result is None + assert repo.get_state("open_stop:BTC-USD") == Decimal("49000") + + # -- ATR trailing stop -----------------------------------------------------------------------