diff --git a/keel/execution/executor.py b/keel/execution/executor.py index 57471d9d..2f4a8f58 100644 --- a/keel/execution/executor.py +++ b/keel/execution/executor.py @@ -766,6 +766,9 @@ def place_bracket( qty=qty, entry=stop, stop=None, + # The trigger price again, this time where rail 9 can read it. `stop` stays None so rail + # 7 does not measure a 0% entry-to-stop move and veto the bracket (issue #206). + protective_stop=stop, notional=sizing.spend(qty, stop), is_dca=False, rule_kind=rule_name, @@ -920,6 +923,9 @@ def _roll_stop( qty=qty, entry=new_stop, stop=None, + # Belt and braces: this function already refuses a widening roll above, but that check is + # local and overridable by a future caller, whereas rail 9 is not (issue #206). + protective_stop=new_stop, notional=sizing.spend(qty, new_stop), is_dca=False, rule_kind=rule_name, diff --git a/keel/execution/guards.py b/keel/execution/guards.py index e91bf9d2..7ae5c2e0 100644 --- a/keel/execution/guards.py +++ b/keel/execution/guards.py @@ -19,7 +19,11 @@ - Rail 9 (no stop-loss widening) reads an `agent_state` key `open_stop:` holding the last-known protective stop for that product; the executor (Task 4) is expected to keep it current as brackets are placed/rolled. No prior stop recorded means there is nothing to widen - against, so the rail passes (an intent's *first* stop is never "widening"). + against, so the rail passes (an intent's *first* stop is never "widening"). It compares an + ENTRY's `stop` or a PROTECTIVE order's `protective_stop` — a bracket carries its trigger price + in `entry` and leaves `stop` unset, so keying on `stop` alone silently skipped every bracket + keel placed. Rail 7 (min-move) still reads `stop` only, and is therefore inert on a bracket by + design: entry and stop are the same price there, so there is no move to measure. - Rail 11 (drawdown breaker) reads precomputed `agent_state` keys `drawdown_total_pct` / `drawdown_weekly_pct` (owned by `money_mgmt`/`pnl`, later phases) rather than recomputing an equity curve here — guards is a pure checker, not a P&L engine. @@ -159,6 +163,18 @@ class OrderIntent: # clock access of its own. `None` means "unknown" and fails the BUY closed. withdrawals_enabled: bool | None = None + # Rail 9, for a PROTECTIVE order (`place_bracket`/`_roll_stop`): the price this order + # triggers at. A bracket carries that price in `entry` and leaves `stop` unset, because it is + # not an entry protected by a stop elsewhere -- it IS the stop. Rail 9 keys on + # `stop is not None`, so before this field it skipped every bracket keel ever placed and the + # ratchet-only invariant went unenforced on the live path (issue #206). + # + # ⚠️ Deliberately NOT expressed by populating `stop`. Rail 7 (min-move/anti-scalping) measures + # `abs(entry - stop) / entry`, and a bracket's entry and stop are the SAME price by + # construction -- so reusing `stop` would read as a 0% move and veto every protective order + # on the anti-scalping floor. Two rails, two different questions, two fields. + protective_stop: Decimal | None = None + #: Rails whose inputs describe the LIVE ACCOUNT and therefore cannot be evaluated offline: #: rail 13 needs a broker-fetched quote balance, rail 17 needs the account's real withdrawal @@ -469,12 +485,17 @@ def check( ) # 9. No stop-loss widening — stops only ratchet toward profit vs. the last recorded stop. - if intent.stop is not None: + # Reads an ENTRY's `stop` or a PROTECTIVE order's `protective_stop`: a bracket carries its + # trigger price in `entry` and leaves `stop` unset, so keying on `stop` alone skipped every + # bracket. Strictly `<`, never `<=` -- re-placing at the SAME level is how a dead or + # rejected bracket is recovered, and vetoing that would strand the position naked. + proposed_stop = intent.stop if intent.stop is not None else intent.protective_stop + if proposed_stop is not None: prior_stop = repo.get_state(f"open_stop:{intent.product_id}") - if prior_stop is not None and intent.stop < prior_stop: + if prior_stop is not None and proposed_stop < prior_stop: violations.append( - f"no_stop_widening: proposed stop {intent.stop} is wider (lower) than the prior " - f"stop {prior_stop}" + f"no_stop_widening: proposed stop {proposed_stop} is wider (lower) than the " + f"prior stop {prior_stop}" ) # 10. Sell-only-on-rule — no arbitrary liquidation; every SELL must cite a defined rule. diff --git a/tests/execution/test_guards.py b/tests/execution/test_guards.py index 9a54c8e9..130cf36a 100644 --- a/tests/execution/test_guards.py +++ b/tests/execution/test_guards.py @@ -1508,3 +1508,96 @@ def test_offline_still_honours_the_kill_switch(repo: Repository) -> None: def test_a_clean_intent_passes_offline_without_live_state(repo: Repository) -> None: intent = _intent(available_quote=None, withdrawals_enabled=None) assert check(intent, repo, _config(), NOW_TS, offline=True).ok is True + + +# -- rail 9 and the protective bracket (issue #206) ------------------------------------------- +# +# A bracket's stop travels as `entry` with `stop=None` (see `place_bracket`), because the order +# TRIGGERS at that price -- it is not an entry protected by a stop somewhere else. Rail 9's +# `intent.stop is not None` guard therefore skipped every bracket ever placed, so the one rail +# that enforces ratchet-only saw only entries. `protective_stop` is the field that makes the +# bracket's own trigger visible to it. +# +# It is deliberately a SEPARATE field rather than reusing `stop`: rail 7 (min-move) computes +# `abs(entry - stop) / entry`, and a bracket has `entry == stop` by construction, so populating +# `stop` would compute a 0% move and veto EVERY protective bracket on the anti-scalping floor. + + +def test_rail9_sees_a_protective_brackets_own_stop(repo): + """The gap. A replacement bracket trying to trigger BELOW the recorded stop is widening the + position's risk, and before `protective_stop` nothing checked it -- `_roll_stop` has its own + ratchet guard, but it has no production caller, so on the live path this was unenforced.""" + repo.set_state("open_stop:BTC-USD", Decimal("49500")) + intent = _intent( + side=Side.SELL, + entry=Decimal("49000"), + stop=None, + protective_stop=Decimal("49000"), # below the recorded 49500 -- widening + rule_kind="turtle_breakout", + ) + + result = check(intent, repo, _config(), NOW_TS) + + assert result.ok is False + assert "no_stop_widening" in _keys(result) + + +def test_rail9_allows_a_bracket_that_ratchets_toward_profit(repo): + repo.set_state("open_stop:BTC-USD", Decimal("49000")) + intent = _intent( + side=Side.SELL, + entry=Decimal("49500"), + stop=None, + protective_stop=Decimal("49500"), + rule_kind="turtle_breakout", + ) + + result = check(intent, repo, _config(), NOW_TS) + + assert "no_stop_widening" not in _keys(result) + + +def test_rail9_allows_re_placing_a_bracket_at_the_SAME_stop(repo): + """The case that must not regress. Re-bracketing after a bracket dies or is rejected + re-places at the recorded level (`_rebracket_or_escalate` and + `reconcile_unbracketed_positions` both do exactly this), so an off-by-one to `<=` here would + veto every recovery and strand the position naked -- the failure #195 just closed.""" + repo.set_state("open_stop:BTC-USD", Decimal("49000")) + intent = _intent( + side=Side.SELL, + entry=Decimal("49000"), + stop=None, + protective_stop=Decimal("49000"), + rule_kind="turtle_breakout", + ) + + result = check(intent, repo, _config(), NOW_TS) + + assert "no_stop_widening" not in _keys(result) + + +def test_a_protective_bracket_is_not_vetoed_by_the_min_move_floor(repo): + """Why `protective_stop` is a separate field and not just `stop`. Rail 7 measures + entry-to-stop distance; a bracket's are the same price, so reusing `stop` would read as a 0% + move and veto every protective order keel places.""" + intent = _intent( + side=Side.SELL, + entry=Decimal("49000"), + stop=None, + protective_stop=Decimal("49000"), + rule_kind="turtle_breakout", + ) + + result = check(intent, repo, _config(), NOW_TS) + + assert "min_move_anti_scalping" not in _keys(result) + + +def test_an_entry_intent_still_uses_its_own_stop_for_rail9(repo): + """`protective_stop` must not shadow the entry path rail 9 already covered.""" + repo.set_state("open_stop:BTC-USD", Decimal("49500")) + intent = _intent(stop=Decimal("49000")) # a BUY, widening + + result = check(intent, repo, _config(), NOW_TS) + + assert _keys(result) == {"no_stop_widening"}