From a16d5bba85d78ec780a58522298a1b4401277111 Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Thu, 27 Aug 2026 20:28:46 -0400 Subject: [PATCH 1/6] feat(executor): a public single-roll entry point, and rolls keep the tranche link (#502) `roll_stop_to` is the general form of the two named roll primitives: one explicit level, one cancel-and-replace. The agent's live stop-management step needs exactly that shape -- a policy carrying both exit knobs can win on both in one cycle, and calling `roll_to_break_even` and `trail_stop_atr` back-to-back would walk #519's cancel-before-place window twice against the same position for no benefit. `_roll_stop` now also repoints the owning tranche at the replacement bracket. `get_position_for_bracket` is reconciliation's one linkage direction; until rolls were reachable the link could not go stale here, but a roll that cancels the bracket a tranche names and leaves the name behind orphans the replacement -- its eventual fill resolves to no tranche, the trade_outcomes row is dropped, and rail 16 miscounts a managed winner. Also brings the module docstring and #560's guard comment up to the post-#569 reality they still described falsely (the live path DOES build a BracketGTC now; the 'no live caller' claim about stop management is what the next commit changes). --- keel/execution/executor.py | 102 ++++++++++++++++++++---------- tests/execution/test_executor.py | 103 ++++++++++++++++++++++++++++++- 2 files changed, 171 insertions(+), 34 deletions(-) diff --git a/keel/execution/executor.py b/keel/execution/executor.py index 7b44eed..17b307a 100644 --- a/keel/execution/executor.py +++ b/keel/execution/executor.py @@ -33,34 +33,29 @@ both problems by construction. It still runs through `guards.check` like any other order (un-overridable); a vetoed bracket is simply never placed and `place_bracket` returns `None`. -**Stop management -- IMPLEMENTED, TESTED, AND NOT CALLED ON THE LIVE PATH (#442; be not -misled).** `roll_to_break_even` / `trail_stop_atr` cancel the existing protective stop leg -and replace it at a new price, never widening it: both delegate to `_roll_stop`, which -refuses (returns `None`, leaving the existing stop in force) if the proposed new stop is -below the last-recorded `open_stop:` -- the same "only ratchet toward profit" -invariant rail 9 enforces on entries, applied directly here since the replacement order's -own `guards.check` call (still mandatory) can't reuse rail 9 as-is: rail 9 is paired with -the min-move/anti-scalping rail, which has no meaning for a stop-replacement order that has -no separate entry price of its own. `scale_out` closes part of a position (a rule-driven -partial profit-take) through the same guard+preview+place+log pipeline as a plain SELL leg. -NONE of the three has a live caller: nothing in the agent cycle manages a stop after -placement (`agent._handle_exits` executes rule EXIT signals only), so reading this module -alone OVERSTATES what keel does -- live, a position's exits are exactly the entry-time -bracket above and a rule EXIT signal, nothing that ratchets. Why unwired, established in -#442: (1) ratchet-only is rail-9-safe BY CONSTRUCTION (`_roll_stop` refuses a widening -proposal before `guards.check` ever runs; pinned by `tests/execution/ -test_executor.py::test_a_ratchet_only_trail_can_never_trip_rail_9`); (2) live wiring needs -a cancel-and-replace of the native bracket, and the broker port has NO bracket/OCO -`OrderSpec` kind -- the bracket reaches the venue only because this module bypasses the -port with a raw configuration dict -- so live stop management is split out to issue #502 -rather than solved here; (3) the exit POLICY those primitives encode (the same -ratchet-only ATR trail and break-even roll) IS wired where exits are driven per bar: the -sim/backtest engines, via `strategy/exit_policy.py` and the per-family `trail_atr_mult` / -`be_roll_rr` params on `pullback_continuation` and `rsi_meanrev`. `turtle_breakout` -deliberately carries neither knob -- its real exit is the Donchian channel, and a trail -would cut the long winners the system exists to let run. `scale_out` stays unwired until -#502 also covers its two pinned prerequisites (bracket resize for a partial SELL, and a -`trade_outcomes` row so rail 16 does not count a scaled-out winner as a loss). +**Stop management -- wired on the live cycle, DEFAULT OFF per rule (#442 wired the policy; +#502 stage 2 wired the live step).** `roll_to_break_even` / `trail_stop_atr` cancel the +existing protective bracket and replace it at a new price, never widening it: both delegate +to `_roll_stop`, which refuses (returns `None`, leaving the existing stop in force) if the +proposed new stop is below the last-recorded `open_stop:` -- the same "only +ratchet toward profit" invariant rail 9 enforces on entries, applied directly here since the +replacement order's own `guards.check` call (still mandatory) can't reuse rail 9 as-is: +rail 9 is paired with the min-move/anti-scalping rail, which has no meaning for a +stop-replacement order that has no separate entry price of its own. `roll_stop_to` is the +general single-roll form the agent's per-cycle step drives (see its docstring for why one +roll, not one per arm). The live caller is `agent.run_once`'s stop-management step (#502): +per held tranche whose owning rule carries `trail_atr_mult` / `be_roll_rr` (the +`pullback_continuation` / `rsi_meanrev` knobs), it applies the same `strategy/exit_policy` +the sim/backtest engines apply and rolls the resting bracket to the ratcheted level. A rule +whose params carry NEITHER knob is never managed -- the #442 experiment +(docs/experiments/2026-08-22-trailing-vs-static-exits.md) measured trailing WORSE and the +break-even roll no better than the static exit at the 120 bp fee, so the capability ships +and the operator opts in per rule; `turtle_breakout` deliberately offers neither knob (its +real exit is the Donchian channel, and a trail would cut the long winners the system exists +to let run). `scale_out` is the one primitive still WITHOUT a live caller: it stays unwired +(pinned by `tests/execution/test_executor.py::test_scale_out_has_no_production_caller`) +until its two prerequisites are built -- bracket resize for a partial SELL, and a +`trade_outcomes` row so rail 16 does not count a scaled-out winner as a loss. **USDC-funding balance (rail 13, Issue #59).** For a BUY `_build_intent` fetches the live available balance of the PRODUCT's quote leg from `broker.get_balances()` and hands @@ -1724,9 +1719,11 @@ def _roll_stop( # 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. + # flip wearing a protective order's name" -- and the spec built below IS one of those + # (#524/#569: every order this module places is a port value now), so the construction check + # itself is one line behind this guard. #560 added this earlier, explicit refusal for the + # same hazard because it predates the spec-shaped live path; it stays because it refuses + # BEFORE the cancel, leaving the existing bracket in force. # # 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 @@ -1842,6 +1839,18 @@ def _roll_stop( # `place_bracket`'s own success path; leaving it would have the sweep re-place a bracket that # already exists on the next cycle. repo.set_state(f"{UNBRACKETED_PREFIX}{product_id}", None) + # Repoint the owning tranche at the replacement (#502). `get_position_for_bracket` is the + # ONE linkage direction reconciliation has: it resolves a bracket FILL back to the trade it + # closed. Until rolls were reachable this link could not go stale here -- `place_bracket` + # and the sweep's re-place both set it -- but a roll that cancels the bracket a tranche + # names and leaves the name behind orphans the replacement: when it eventually fills, the + # fill resolves to no tranche, its `trade_outcomes` row is dropped, and rail 16 miscounts a + # managed winner as nothing at all. `None` (no tranche names the old order -- e.g. a tranche + # predating the ledger) is skipped silently: the roll is still correct, only the attribution + # is absent, exactly as it was before the ledger existed. + position = repo.get_position_for_bracket(old_stop_order_id) + if position is not None: + repo.set_position_bracket(position["id"], result.order_id) log_event( logger, logging.INFO, @@ -1854,6 +1863,37 @@ def _roll_stop( return result.order_id +def roll_stop_to( + broker: Any, + repo: Repository, + config: Config, + product_id: str, + old_stop_order_id: int, + new_stop: Decimal, + qty: Decimal, + rule_name: str, + now_ts: int, +) -> int | None: + """Roll the protective bracket's stop to `new_stop` -- the GENERAL, single-roll entry point. + + `roll_to_break_even` and `trail_stop_atr` are the two named special cases (a level derived + from the entry, a level derived from an ATR multiple), and each performs its own full + cancel-and-replace. A rule carrying BOTH exit knobs can win on both in one cycle, and + calling the two named primitives back-to-back would walk #519's cancel-before-place window + TWICE against the same position for no benefit. The caller that faces that case -- + `agent.run_once`'s live stop-management step (#502) -- therefore computes ONE ratcheted + level (`strategy.exit_policy.next_stop`, the max over the arms, the same function the + sim/backtest engines apply) and hands it here, so exactly one replacement is placed. + + Every `_roll_stop` guarantee applies unchanged: refusal on widening, refusal at/above the + target, the crash ledger before the venue is touched, cancel before place, and the + tranche-bracket repoint on success. `None` means the existing bracket stays in force. + """ + return _roll_stop( + broker, repo, config, product_id, old_stop_order_id, new_stop, qty, rule_name, now_ts + ) + + def roll_to_break_even( broker: Any, repo: Repository, diff --git a/tests/execution/test_executor.py b/tests/execution/test_executor.py index 863bb0d..b70081a 100644 --- a/tests/execution/test_executor.py +++ b/tests/execution/test_executor.py @@ -39,6 +39,7 @@ ExecutionResult, execute, place_bracket, + roll_stop_to, roll_to_break_even, scale_out, trail_stop_atr, @@ -1048,9 +1049,10 @@ def test_a_roll_that_reaches_the_target_is_refused_and_the_bracket_stays(repo): 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. + `keel_broker_api.orders.BracketGTC` refuses exactly this at construction, and every roll + IS one of those since #524/#569 -- but #560 added this earlier, explicit refusal for the + same hazard, and it stays because it refuses BEFORE the cancel, leaving the existing + bracket resting rather than relying on the construction error after the cancel landed. 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, @@ -1751,6 +1753,101 @@ def test_rolling_the_stop_carries_the_original_target_forward(repo): assert replacement.stop_trigger_price == Decimal("50000") # stop moved to break-even +def test_roll_stop_to_rolls_to_an_explicit_policy_computed_level(repo): + """`roll_stop_to` is the single-roll entry point the agent's live management step drives + (#502 stage 2). `roll_to_break_even` and `trail_stop_atr` each perform their OWN + cancel-and-replace; a policy carrying both arms can win on both in one cycle, and calling + the two named primitives back-to-back would walk the naked-position window (#519's + cancel-before-place) TWICE for no benefit. The step therefore computes ONE ratcheted level + (`strategy.exit_policy.next_stop` -- max over the arms, the same function the sim and + backtester apply) and hands it here. + """ + broker = FakeBroker() + old_id = place_bracket( + broker, + repo, + _config(), + product_id="BTC-USD", + qty=Decimal("0.01"), + stop=Decimal("49000"), + target=Decimal("54000"), + rule_name="pullback_continuation", + now_ts=NOW_TS, + ) + + new_id = roll_stop_to( + broker, + repo, + _config(), + product_id="BTC-USD", + old_stop_order_id=old_id, + new_stop=Decimal("50750"), + qty=Decimal("0.01"), + rule_name="pullback_continuation", + now_ts=NOW_TS + 100, + ) + + assert new_id is not None and new_id != old_id + assert repo.get_order(old_id)["status"] == "canceled" + assert repo.get_state("open_stop:BTC-USD") == Decimal("50750") + # The #519 protocol, unchanged by the new entry point: cancel BEFORE place. + assert broker.events == ["place", "cancel", "place"], broker.events + replacement = broker.place_calls[-1]["spec"] + assert isinstance(replacement, BracketGTC) + assert replacement.stop_trigger_price == Decimal("50750") + assert replacement.take_profit_price == Decimal("54000") + + +def test_a_roll_repoints_the_owning_tranche_at_the_replacement_bracket(repo): + """The tranche<->bracket link is how a bracket FILL resolves back to the trade it closed + (`Repository.get_position_for_bracket`, reconciliation's one lookup direction). Until the + live management step (#502) rolls were unreachable, so `_roll_stop` never had to maintain + it; with rolls live, a roll that cancels the bracket a tranche names and does not repoint + it leaves every LATER fill of the replacement bracket resolving to no tranche -- its + `trade_outcomes` row is dropped and rail 16 miscounts a managed winner. + """ + broker = FakeBroker() + old_id = place_bracket( + broker, + repo, + _config(), + product_id="BTC-USD", + qty=Decimal("0.01"), + stop=Decimal("49000"), + target=Decimal("54000"), + rule_name="pullback_continuation", + now_ts=NOW_TS, + ) + position_id = repo.open_position( + product_id="BTC-USD", + rule_name="pullback_continuation", + opened_at=NOW_TS, + qty=Decimal("0.01"), + entry_fill=Decimal("50000"), + entry_fee=Decimal("0"), + initial_stop=Decimal("49000"), + bracket_order_id=old_id, + ) + + new_id = roll_stop_to( + broker, + repo, + _config(), + product_id="BTC-USD", + old_stop_order_id=old_id, + new_stop=Decimal("50750"), + qty=Decimal("0.01"), + rule_name="pullback_continuation", + now_ts=NOW_TS + 100, + ) + + assert new_id is not None + # The replacement is the tranche's bracket now, and the cancelled order no longer answers. + assert repo.get_position_for_bracket(new_id) is not None + assert repo.get_position_for_bracket(new_id)["id"] == position_id + assert repo.get_position_for_bracket(old_id) is None + + def test_a_roll_that_cannot_replace_the_bracket_screams_that_the_position_is_naked(repo, caplog): """The cost of cancel-first. If the cancel succeeds and the replacement is then rejected, the position is left with NO protective stop. That must never pass quietly.""" From 0a56727a7b042e194276c7612edaa45a22111be1 Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Thu, 27 Aug 2026 20:44:03 -0400 Subject: [PATCH 2/6] feat(agent): the per-cycle live stop-management step, off by default (#502) `run_once` now manages held positions' exchange brackets: after the trading decisions, before cycle end, `_manage_stops` walks the open tranches and, for each whose owning rule carries `trail_atr_mult` / `be_roll_rr`, computes the ratcheted level with the SAME policy the sim/backtest engines apply (`policy_for` + `next_stop` on the latest COMPLETED bar -- poll stores closed candles only, so the no-lookahead contract holds) and rolls the resting bracket ONCE via `executor.roll_stop_to` (#519's cancel-before-place protocol, reused, not re-invented). DEFAULT OFF, exactly like the sim wiring: a rule whose params carry neither knob is never touched, and every rule row in existence carries neither -- pinned by the identity test. The #442 experiment measured trailing WORSE and the break-even roll no better than the static exit at the 120 bp fee, so the capability ships dark and the operator opts in per rule; turtle stays Donchian by choice. Paper cycles never run the step (no exchange brackets to roll), and a tranche without a recorded initial_stop runs with the break-even arm disabled -- the ledger's own contract. Tests: the defaults-off identity pin, a trailing roll through the fake broker (level pinned to the policy's own arithmetic, cancel-before-place asserted, tranche repointed), a falling cycle that never widens, the break-even roll and its NULL-initial_stop disable, turtle never managed, paper never managed. Docstrings that claimed the knobs were sim-only (`exit_policy`, both rule families' PARAM_DOCS, paper, portfolio_sim) and #560-era claims that the port has no bracket kind (executor, reconcile) are brought up to date. --- keel/agent.py | 130 ++++++- keel/execution/executor.py | 11 +- keel/execution/reconcile.py | 11 +- keel/sim/portfolio_sim.py | 4 +- keel/strategy/exit_policy.py | 50 +-- keel/strategy/paper.py | 9 +- keel/strategy/rules/pullback_continuation.py | 10 +- keel/strategy/rules/rsi_meanrev.py | 13 +- tests/test_agent.py | 341 +++++++++++++++++++ 9 files changed, 532 insertions(+), 47 deletions(-) diff --git a/keel/agent.py b/keel/agent.py index 79d954c..9f26297 100644 --- a/keel/agent.py +++ b/keel/agent.py @@ -61,7 +61,7 @@ import logging import time from collections.abc import Callable -from dataclasses import dataclass, field +from dataclasses import dataclass, field, replace from decimal import Decimal from typing import Any, Literal @@ -77,6 +77,7 @@ from keel.execution.executor import ExecutionResult, _fetch_available_quote from keel.execution.guards import FEED_STALENESS_CYCLES from keel.strategy import engine +from keel.strategy.exit_policy import EXIT_POLICY_OFF, next_stop, policy_for, trailing_atr from keel.strategy.paper import PaperTrader from keel.strategy.rules.base import Action, Rule, Setup, Signal from keel.strategy.rules.dca import Dca @@ -819,6 +820,122 @@ def _close_tranches( repo.close_position(position["id"], closed_at=now_ts) +def _manage_stops( + broker: Any, + repo: Repository, + config: Config, + rules: list[Rule], + candles_by_tf_by_product: dict[str, dict[Granularity, list[Any]]], + now_ts: int, +) -> None: + """The per-cycle live stop-management step (#502 stage 2): ratchet each held tranche's + resting bracket according to its OWNING rule's exit policy. + + DEFAULT OFF, exactly like the sim wiring. The knobs are per-rule-family params + (`trail_atr_mult` / `be_roll_rr` on `pullback_continuation` and `rsi_meanrev`), a rule + whose params carry neither gets `EXIT_POLICY_OFF` and its position is not touched -- so + every rule row that has not opted in trades exactly as before this step existed. The + #442 experiment (docs/experiments/2026-08-22-trailing-vs-static-exits.md) measured + trailing WORSE and the break-even roll no better than the static exit at the 120 bp fee, + so the capability ships dark and the operator opts in per rule; `turtle_breakout` offers + the knobs nowhere at all (its exit is the Donchian channel by choice). + + The decision is DELEGATED, not re-derived: `policy_for` + `next_stop` are the same + functions the sim/backtest engines apply, so the live level and the simulated level come + from one place. The bar is the latest COMPLETED one on the rule's trading timeframe + (`market_feed.poll_once` stores closed candles only), so the no-lookahead contract holds: + during the bar the venue's bracket rested at the old level, and the new level binds from + the next bar. A punctual cycle sees exactly one new bar per run; a MISSED cycle reads only + the newest bar, which is conservative in the one arm where it differs -- a break-even + trigger on an older bar's high goes unobserved and leaves the stop lower, never looser. + + The ROLL itself is `executor.roll_stop_to`: ONE cancel-and-replace per tranche per cycle + (#519's cancel-before-place protocol, the crash ledger, the ratchet and at/above-target + refusals -- all in the executor, none re-invented here). Paper cycles never call this: + paper entries place no exchange-side brackets, so there is nothing to roll. + """ + rules_by_name = {rule.name: rule for rule in rules} + for tranche in repo.get_open_positions(): + rule = rules_by_name.get(tranche["rule_name"]) + if rule is None: + continue # the owning rule is not on this cycle's set (demoted/retired): no policy + policy = policy_for(rule) + if policy is EXIT_POLICY_OFF: + continue # the default: the rule never asked for stop management + + bracket_id = tranche["bracket_order_id"] + if bracket_id is None: + continue # no bracket recorded: the unbracketed sweep owns this position (#195) + old_order = repo.get_order(bracket_id) + if old_order is None or old_order["status"] not in executor.RESTING_STATUSES: + continue # filled or dead: reconciliation owns that bracket, not this step + current_stop = repo.get_state(f"open_stop:{tranche['product_id']}") + if current_stop is None: + continue # no recorded level to ratchet from -- nothing this step may act on + + # `initial_stop` is the tranche's ORIGINAL setup stop (#520) -- the denominator of the + # break-even threshold. NULL means "nobody recorded it" (pre-ledger tranche, DCA), and + # the ledger's own contract is that the BE arm switches OFF rather than substitutes the + # current stop, which is a different (and on a ratcheted position, stale) policy. + initial_stop = tranche["initial_stop"] + if initial_stop is None: + policy = replace(policy, be_roll_rr=None) + + candles_by_tf = candles_by_tf_by_product.get(tranche["product_id"]) + if not candles_by_tf: + continue # stale product this cycle: no completed bar to manage on + series = candles_by_tf.get(_management_timeframe(rule, candles_by_tf)) or [] + if not series: + log_event( + logger, + logging.INFO, + "agent.stop_management_skipped", + product=tranche["product_id"], + rule=rule.name, + reason="no candles on the rule's trading timeframe", + ) + continue + + atr = trailing_atr(series, policy.atr_period) + level = next_stop( + policy, + tranche["entry_fill"], + # `initial_stop` is only read by the BE arm, which is OFF above when it is NULL; + # the entry is the neutral stand-in for the unreachable denominator. + initial_stop if initial_stop is not None else tranche["entry_fill"], + current_stop, + series[-1], + atr, + ) + if level <= current_stop: + continue # the ratchet: nothing proposed toward profit this cycle + + executor.roll_stop_to( + broker, + repo, + config, + product_id=tranche["product_id"], + old_stop_order_id=bracket_id, + new_stop=level, + qty=tranche["qty"], + rule_name=tranche["rule_name"], + now_ts=now_ts, + ) + + +def _management_timeframe(rule: Rule, candles_by_tf: dict[Granularity, list[Any]]) -> Granularity: + """The series stop management reads: the rule's own trading timeframe when it declares one + (`granularity` on `TurtleBreakout`/`PullbackContinuation`, `timeframe` on `RsiMeanReversion` + -- the same attribute order `engine._trading_granularity` and `backtest._rule_trading_tf` + use), else the finest series this cycle actually has. Every knob-carrying family declares + its timeframe, so the fallback is a bound, not a path anyone trades on.""" + for attr in ("granularity", "timeframe"): + value = getattr(rule, attr, None) + if isinstance(value, Granularity): + return value + return min(candles_by_tf, key=lambda g: _GRANULARITY_ORDER.get(g, 0)) + + # -- venue session (FR-9: a closed market is not a stale feed) -------------------------------- #: `agent_state` key PREFIXES the cycle writes each run when (and only when) the broker is a @@ -1662,6 +1779,17 @@ def run_once( initial_stop=signal.setup.stop if signal.setup is not None else None, ) + # == STOP MANAGEMENT: ratchet held positions' brackets per the owning rule's policy == + # + # AFTER the trading decisions (a rule exit this cycle already closed its tranches, so + # they are no longer `open` and cannot be managed; a fresh entry's bracket rests at its + # own setup stop and may trail from this same completed bar, mirroring the sim's + # bar-end sequencing) and BEFORE cycle end. LIVE cycles only: paper entries place no + # exchange-side brackets, so there is nothing to roll and the paper path must not touch + # the real order book. Default-off per rule -- see `_manage_stops`. + if paper_trader is None: + _manage_stops(broker, repo, config, rules, candles_by_tf_by_product, now_ts) + cycle_result = LoopResult( ts=now_ts, skipped=False, diff --git a/keel/execution/executor.py b/keel/execution/executor.py index 17b307a..c198dc0 100644 --- a/keel/execution/executor.py +++ b/keel/execution/executor.py @@ -825,11 +825,12 @@ def _record_observed_fill_quantity( the exit-side over-booking is #502's to flag. The observation itself is recorded for BOTH sides: `filled_quantity` is what actually executed, whatever the order's direction. - DELIBERATELY detect-and-surface only. Resizing the bracket means either amending a live - native trigger-bracket or cancel-and-replace, and the broker port carries no bracket/OCO - kind at all (#502) -- the live bracket already bypasses the port as a raw dict. Auto-cancelling - a protective order on the strength of a snapshot that may still be settling is a wrong - auto-action on live money; a loud warning is the safe half, and it is what this does. + DELIBERATELY detect-and-surface only. The port can EXPRESS the replacement since #502 + stage 1 (`BracketGTC` -- every order is a spec now), and `_roll_stop` cancels-and-replaces + through it; what does not exist yet is the RESIZE policy (a roll re-places the SAME size). + Auto-cancelling a protective order on the strength of a snapshot that may still be + settling is a wrong auto-action on live money; a loud warning is the safe half, and the + automated resize policy remains #502's open half (with `scale_out`). """ filled = observed.get("filled_size") if not filled or filled <= 0: diff --git a/keel/execution/reconcile.py b/keel/execution/reconcile.py index 284ca9c..ad3fb65 100644 --- a/keel/execution/reconcile.py +++ b/keel/execution/reconcile.py @@ -30,10 +30,11 @@ auto-remediation, not the validity of recognizing the state. What is deliberately NOT done here: resizing or amending the bracket when a partially-filled -entry leaves it oversized for what is held. The broker port has no bracket/OCO kind (#502); the -live bracket bypasses it with a raw dict, and auto-cancelling live protective orders on the -strength of a possibly-still-settling partial snapshot is strictly worse than a loud warning. -This module records and surfaces; the amend-vs-cancel-and-replace policy is #502's. +entry leaves it oversized for what is held. The port can express the cancel-and-replace +(`BracketGTC`, #502 stage 1) and `executor._roll_stop` performs one, but a RESIZE is a +re-place at a different size, and auto-cancelling live protective orders on the strength of a +possibly-still-settling partial snapshot is strictly worse than a loud warning. This module +records and surfaces; the resize policy rides with `scale_out`'s remaining #502 scope. """ from __future__ import annotations @@ -588,6 +589,6 @@ def _native_order_id(order_row: dict[str, Any]) -> str | None: return None try: data = json.loads(raw) - except (TypeError, ValueError): + except TypeError, ValueError: return None return data.get("order_id") diff --git a/keel/sim/portfolio_sim.py b/keel/sim/portfolio_sim.py index 0f08094..a663d13 100644 --- a/keel/sim/portfolio_sim.py +++ b/keel/sim/portfolio_sim.py @@ -54,8 +54,8 @@ through the stop exits at its OPEN (`strategy.backtest._stop_exit_price`, the shared convention). A rule without the knobs trades exactly as before the wiring existed -- identity pinned by the unit-identity tests plus the unchanged pre-existing suite with the - wiring live. This is the sim-side expression of the live `executor` stop-management - primitives, which themselves stay uncalled on the live path (port-blocked, issue #502). + wiring live. This is the sim-side expression of the `executor` stop-management primitives, + which the live cycle drives too since #502 stage 2 (`agent._manage_stops`, default off). **No lookahead:** the per-bar `candles_by_tf` window handed to `Rule.detect`/`exit_signal` and to `engine.evaluate` only ever contains candles with `ts <= t` (the current bar). The one deliberate diff --git a/keel/strategy/exit_policy.py b/keel/strategy/exit_policy.py index 88c7a74..1d3fdac 100644 --- a/keel/strategy/exit_policy.py +++ b/keel/strategy/exit_policy.py @@ -1,34 +1,41 @@ -"""The per-family, ratchet-only stop-management policy for held positions (#442). - -`execution/executor.py` implements three exit primitives -- `trail_stop_atr`, -`roll_to_break_even`, `scale_out` -- that have ZERO callers on the live path: live exits -ride ONE native Coinbase trigger-bracket placed at entry (the exchange owns the -stop-vs-target race), the broker port has no bracket/OCO order kind to express a -cancel-and-replace through, and the agent cycle has no per-bar stop-management step at -all. That live wiring is split out to issue #502; it is NOT this module's concern. - -What this module IS: the exit POLICY those live primitives encode -- an ATR-multiple -trailing stop and a break-even roll, both strictly ratchet-only (a stop may move toward -profit, never away from it) -- expressed as pure functions the engines that DO drive -exits per bar can apply: - -- `strategy.backtest.backtest` (the single-rule, production-faithful backtester), and -- `sim.portfolio_sim._process_held` (the account simulator's exit resolution). - -Both engines apply it with the SAME sequencing, which is the no-lookahead contract: +"""The per-family, ratchet-only stop-management policy for held positions (#442; live wiring +#502). + +`execution/executor.py` implements the exit primitives -- `trail_stop_atr`, +`roll_to_break_even`, `roll_stop_to`, `scale_out`. The first three have a live caller since +#502 stage 2: `agent.run_once`'s per-cycle stop-management step applies the policy computed +HERE (`policy_for` + `next_stop`) to each held tranche whose owning rule carries the knobs, +then rolls the resting exchange-side bracket through the executor's single-roll protocol. +Live management is DEFAULT OFF exactly like the sim wiring -- a rule whose params carry +neither knob gets `EXIT_POLICY_OFF` and nothing ever moves its stop (#442 measured trailing +WORSE and the break-even roll no better than the static exit at the 120 bp fee, so the +operator opts in per rule). `scale_out` remains without a live caller, pinned by its tripwire +test until its two prerequisites (bracket resize, `trade_outcomes` for a partial exit) exist. + +What this module IS: the exit POLICY itself -- an ATR-multiple trailing stop and a +break-even roll, both strictly ratchet-only (a stop may move toward profit, never away from +it) -- expressed as pure functions every engine that drives exits applies the same way: + +- `strategy.backtest.backtest` (the single-rule, production-faithful backtester), +- `sim.portfolio_sim._process_held` (the account simulator's exit resolution), and +- `agent._manage_stops` (the live per-cycle step, #502 stage 2). + +All three apply it with the SAME sequencing, which is the no-lookahead contract: 1. touch checks for a bar run against the stop level carried INTO that bar (the level management produced from COMPLETED prior bars -- live, the bracket rests at the old level until a management cycle replaces it, and this mirrors that exactly); 2. only if the position survives the bar does `next_stop` run, on that bar's own - high/close and an ATR over a window ending at that bar. + high/close and an ATR over a window ending at that bar. The live step reads the latest + COMPLETED bar on the rule's trading timeframe (`market_feed.poll_once` stores closed + candles only), which is this same rule at cycle cadence. **Where the policy lives (design decision).** The knobs are per-rule-family constructor params (`trail_atr_mult`, `be_roll_rr`, mirroring the existing `atr_mult`-style naming), NOT a global config block: exit behavior is a property of a rule family in the same way `stop_method` is, and a global knob could not express "turtle does not trail". A rule whose `params` carry neither knob gets `EXIT_POLICY_OFF` and trades exactly as before -the wiring existed -- pinned by tests on both engines. +the wiring existed -- pinned by tests on both engines and on the live step. **Why turtle is deliberately not offered the knobs** (#442 hypothesis 3, confirmed): `turtle_breakout`'s real exit is the asymmetric Donchian channel (`exit_signal`), with @@ -41,7 +48,8 @@ compares a proposed protective stop against the last recorded one and vetoes any strictly lower proposal. `next_stop` starts from the current stop and only ever takes maxima, so it can never emit a wider stop -- the sim-side twin of -`tests/execution/test_executor.py::test_a_ratchet_only_trail_can_never_trip_rail_9`. +`tests/execution/test_executor.py::test_a_ratchet_only_trail_can_never_trip_rail_9`, +and the property the live step inherits by computing its level HERE. """ from __future__ import annotations diff --git a/keel/strategy/paper.py b/keel/strategy/paper.py index b73f14c..72115e3 100644 --- a/keel/strategy/paper.py +++ b/keel/strategy/paper.py @@ -22,10 +22,11 @@ aggregates them via the shared `strategy.stats.summarize` helper into the same `BacktestResult` shape `backtest.py` produces, so paper and historical stats are directly comparable. -**Exit-policy knobs are engine-only (#442/#502):** the `trail_atr_mult`/`be_roll_rr` knobs run in -the sim/backtest engines (`strategy.exit_policy`); this trader does not manage stops -- its exits -are the signal's own static stop/target touches (via `backtest.py`'s shared touch helpers) and -signal-driven closes. The knobs' PARAM_DOCS say so outright. +**Exit-policy knobs are not applied here (#442/#502):** the `trail_atr_mult`/`be_roll_rr` knobs +run in the sim/backtest engines and, since #502 stage 2, in the agent's LIVE stop-management +step; this trader does not manage stops -- a paper-forward's exits are the signal's own static +stop/target touches (via `backtest.py`'s shared touch helpers) and signal-driven closes, so its +results stay comparable with the static-exit baseline the knobs were measured against. """ from __future__ import annotations diff --git a/keel/strategy/rules/pullback_continuation.py b/keel/strategy/rules/pullback_continuation.py index 1d412e4..29ffe6e 100644 --- a/keel/strategy/rules/pullback_continuation.py +++ b/keel/strategy/rules/pullback_continuation.py @@ -376,14 +376,16 @@ class PullbackContinuation(Rule): "trail_atr_mult": ( "ratchet-only trailing stop this many ATRs below each bar's close, once the " "trade is open. Default off: measured WORSE than the static exit at the 120 bp " - "fee (docs/experiments/2026-08-22-trailing-vs-static-exits.md). Sim/backtest " - "engines only -- live stop management is issue #502." + "fee (docs/experiments/2026-08-22-trailing-vs-static-exits.md). Applied by the " + "sim/backtest engines and, since #502, by the agent's live per-cycle stop " + "management -- off until a rule opts in." ), "be_roll_rr": ( "roll the stop to the entry once the trade has been up this many R. Default " "off: measured no-better than the static exit at the 120 bp fee " - "(docs/experiments/2026-08-22-trailing-vs-static-exits.md). Sim/backtest " - "engines only -- live stop management is issue #502." + "(docs/experiments/2026-08-22-trailing-vs-static-exits.md). Applied by the " + "sim/backtest engines and, since #502, by the agent's live per-cycle stop " + "management -- off until a rule opts in." ), } diff --git a/keel/strategy/rules/rsi_meanrev.py b/keel/strategy/rules/rsi_meanrev.py index cd928ce..96f048e 100644 --- a/keel/strategy/rules/rsi_meanrev.py +++ b/keel/strategy/rules/rsi_meanrev.py @@ -62,14 +62,16 @@ class RsiMeanReversion(Rule): "trail_atr_mult": ( "ratchet-only trailing stop this many ATRs below each bar's close, once the " "trade is open. Default off: measured WORSE than the static exit at the 120 bp " - "fee (docs/experiments/2026-08-22-trailing-vs-static-exits.md). Sim/backtest " - "engines only -- live stop management is issue #502." + "fee (docs/experiments/2026-08-22-trailing-vs-static-exits.md). Applied by the " + "sim/backtest engines and, since #502, by the agent's live per-cycle stop " + "management -- off until a rule opts in." ), "be_roll_rr": ( "roll the stop to the entry once the trade has been up this many R. Default " "off: measured no-better than the static exit at the 120 bp fee " - "(docs/experiments/2026-08-22-trailing-vs-static-exits.md). Sim/backtest " - "engines only -- live stop management is issue #502." + "(docs/experiments/2026-08-22-trailing-vs-static-exits.md). Applied by the " + "sim/backtest engines and, since #502, by the agent's live per-cycle stop " + "management -- off until a rule opts in." ), "level_tolerance": ( "how close two prices must be to count as one support/resistance level." @@ -101,7 +103,8 @@ class RsiMeanReversion(Rule): fixed_rr: Decimal = Decimal("2") # #442: the per-family exit-policy knobs `strategy.exit_policy` reads. Default OFF # (None) -- the wiring changes no rule's behavior until an operator/research run turns - # a knob on; the live executor has no management cycle to honor them (issue #502). + # a knob on. Honored by the sim/backtest engines and by the live cycle's management + # step since #502 stage 2. trail_atr_mult: Decimal | None = None be_roll_rr: Decimal | None = None level_tolerance: Decimal = Decimal("0.002") diff --git a/tests/test_agent.py b/tests/test_agent.py index afbab80..87892fc 100644 --- a/tests/test_agent.py +++ b/tests/test_agent.py @@ -70,6 +70,10 @@ def __init__(self, series: dict[tuple[str, Granularity], list[Candle]] | None = self.get_candles_calls: list[tuple[str, Granularity, int, int]] = [] self.preview_calls: list[dict[str, Any]] = [] self.place_calls: list[dict[str, Any]] = [] + self.cancel_calls: list[str] = [] + # Ordered exchange interactions (place/cancel), so a test can assert SEQUENCE -- the + # stop-management step (#502) must cancel a bracket before placing its replacement. + self.events: list[str] = [] self._order_seq = 0 # The previewed fee, overridable by a subclass that means to test fee splitting. self.commission = Decimal("0") @@ -107,9 +111,12 @@ def preview_order(self, spec: OrderSpec) -> Preview: def place_order(self, spec: OrderSpec, *, idempotency_key: str | None = None) -> PlaceResult: self._order_seq += 1 self.place_calls.append({"product_id": spec.product_id, "side": spec.side}) + self.events.append("place") return PlaceResult(success=True, broker_order_id=f"broker-order-{self._order_seq}") def cancel_order(self, order_id: str) -> bool: + self.cancel_calls.append(order_id) + self.events.append("cancel") return True # a CONFIRMED cancel -- see `_cancel_at_exchange` @@ -1629,6 +1636,340 @@ def test_run_once_brackets_a_tranche_whose_bracket_was_never_placed(repo: Reposi assert repo.get_state(f"unbracketed:{PRODUCT}") is None +# -- live stop management (#502 stage 2): the per-cycle step, default off -------------------- +# +# The state these tests seed is what a real entry leaves behind: held inventory in the orders +# audit log, ONE resting exchange-side bracket, the `open_stop:`/`open_target:` pair +# `place_bracket` records, and a `positions` tranche naming the bracket and carrying the +# ORIGINAL setup stop (`initial_stop`, #520) the break-even threshold is measured from. + + +def _seed_bracketed_tranche( + repo: Repository, + *, + rule_name: str = "pullback_continuation", + entry: Decimal = Decimal("50000"), + initial_stop: Decimal | None = Decimal("49000"), + qty: Decimal = Decimal("0.01"), + target: Decimal = Decimal("55000"), + ts: int = 1_000, +) -> int: + """Held inventory + a resting bracket + the tranche row that owns it. Returns the bracket's + order id. `initial_stop=None` seeds the pre-#520 TRANCHE shape the ledger documents as "BE + arm disabled", not "zero" -- while the resting bracket's own stop (`open_stop:` state and + the order row's `expected_fill`) stays a real number, because it always is: the bracket + exists, so it rests at some level.""" + stop = initial_stop if initial_stop is not None else Decimal("49000") + _seed_open_position(repo, PRODUCT, qty, entry, ts=ts) + bracket_id = repo.insert_order( + dict( + mode="live", + product_id=PRODUCT, + side=Side.SELL.value, + order_type="market", + qty=qty, + limit_price=None, + status="pending", + fee=None, + expected_fill=stop, + actual_fill=None, + raw_response='{"order_id": "cb-bracket-1"}', + created_at=ts, + updated_at=ts, + ) + ) + repo.set_state(f"position_rule:{PRODUCT}", {"rule_name": rule_name, "opened_at": ts}) + repo.open_position( + product_id=PRODUCT, + rule_name=rule_name, + opened_at=ts, + qty=qty, + entry_fill=entry, + entry_fee=Decimal("0"), + initial_stop=initial_stop, + bracket_order_id=bracket_id, + ) + repo.set_state(f"open_stop:{PRODUCT}", stop) + repo.set_state(f"open_target:{PRODUCT}", target) + return bracket_id + + +def _ranged_candle(ts: int, low: Decimal, high: Decimal, close: Decimal, open_: Decimal) -> Candle: + return Candle(ts=ts, open=open_, high=high, low=low, close=close, volume=Decimal("1")) + + +#: Bar timestamps are exact multiples of ONE_DAY and `now` sits one full period past the last +#: bar, so the whole series is CLOSED and STORED by `market_feed.poll_once` and the product is +#: FRESH by `is_fresh`'s own arithmetic (age = one bar + 1,000s < 3 cycles at interval 50,000). +#: The pre-existing `1_000 + i * 86_400` pattern leaves bars OFF the day grid, the last stored +#: bar a full period older, and the product STALE -- which skips the step for a reason these +#: tests are not about. +def _bar_ts(i: int) -> int: + return (i + 1) * 86_400 + + +def _management_now_ts(n_bars: int = 30) -> int: + return (n_bars + 1) * 86_400 + 1_000 + + +def _rising_series( + n: int = 30, base: Decimal = Decimal("50000"), step: Decimal = Decimal("100") +) -> list[Candle]: + """A steady climb with a real high/low range, so Wilder ATR is non-zero and the trail arm + has something to trail. Bar i: closes at `base + i*step`, +/-60 around it.""" + return [ + _ranged_candle( + _bar_ts(i), + low=base + i * step - Decimal("60"), + high=base + i * step + Decimal("60"), + close=base + i * step, + open_=base + (i - 1) * step if i else base, + ) + for i in range(n) + ] + + +def _seed_history(repo: Repository, series: list[Candle]) -> None: + """Pre-store the series in the candles table. `market_feed.poll_once` cold-starts an EMPTY + table with only the newest closed bar (its catch-up start is `latest_closed`), so a test + that means "a deployment with ATR history" must seed the table itself -- the cycle's poll + then finds the tail current and stores nothing new.""" + repo.upsert_candles(PRODUCT, Granularity.ONE_DAY, series) + + +def test_run_once_leaves_stops_alone_when_the_rule_carries_no_exit_knobs(repo: Repository) -> None: + """THE DEFAULTS-OFF PIN. A rule whose params carry neither `trail_atr_mult` nor `be_roll_rr` + gets `EXIT_POLICY_OFF` (see `strategy.exit_policy.policy_for`), and the management step + must leave its position byte-for-byte as the pre-#502 cycle did: no roll attempt (no + cancel at the broker), the resting bracket untouched, the recorded levels unchanged. This + is every rule row in existence today unless an operator opts a rule in.""" + repo.insert_rule( + "pullback_continuation", + {"product_id": PRODUCT, "granularity": "ONE_DAY"}, + status="live", + ) + bracket_id = _seed_bracketed_tranche(repo) + series = _rising_series() + _seed_history(repo, series) + _seed_history(repo, series) + broker = FakeBroker(series={(PRODUCT, Granularity.ONE_DAY): series}) + + run_once(broker, repo, _config(), now_ts=_management_now_ts()) + + assert "cancel" not in broker.events, "a knob-less rule had its bracket rolled" + assert repo.get_state(f"open_stop:{PRODUCT}") == Decimal("49000") + assert repo.get_state(f"open_target:{PRODUCT}") == Decimal("55000") + assert repo.get_order(bracket_id)["status"] == "pending" + tranche = repo.get_open_positions(PRODUCT)[0] + assert tranche["bracket_order_id"] == bracket_id + + +def test_run_once_trails_a_ratcheting_stop_through_the_broker(repo: Repository) -> None: + """OPTED IN (`trail_atr_mult`), the cycle manages the held position's bracket with the SAME + policy the sim/backtest engines apply: `next_stop` on the latest COMPLETED bar (poll stores + closed candles only), then ONE roll -- #519's cancel-before-place protocol -- so the venue's + bracket ratchets with the climb. The expected level is computed here with the policy's own + functions: the live step's contract is fidelity to `strategy.exit_policy`, not a re-derived + trail of its own.""" + from keel.strategy.exit_policy import next_stop, policy_for, trailing_atr + + repo.insert_rule( + "pullback_continuation", + {"product_id": PRODUCT, "granularity": "ONE_DAY", "trail_atr_mult": "1.5"}, + status="live", + ) + bracket_id = _seed_bracketed_tranche(repo) + series = _rising_series() + _seed_history(repo, series) + broker = FakeBroker(series={(PRODUCT, Granularity.ONE_DAY): series}) + + run_once(broker, repo, _config(), now_ts=_management_now_ts()) + + policy = policy_for(_build_rule(repo.get_rules("live")[0])) + expected = next_stop( + policy, + Decimal("50000"), + Decimal("49000"), + Decimal("49000"), + series[-1], + trailing_atr(series, policy.atr_period), + ) + assert expected > Decimal("49000"), "the fixture must climb far enough to trail" + assert repo.get_state(f"open_stop:{PRODUCT}") == expected + # The #519 protocol through the cycle: the roll cancels the old bracket, then places. + assert broker.events[-2:] == ["cancel", "place"], broker.events + assert repo.get_order(bracket_id)["status"] == "canceled" + replacement_id = repo.get_open_positions(PRODUCT)[0]["bracket_order_id"] + assert replacement_id is not None and replacement_id != bracket_id + assert repo.get_order(replacement_id)["status"] == "pending" + assert repo.get_state(f"open_target:{PRODUCT}") == Decimal("55000") + + +def test_the_live_trail_never_widens_the_stop(repo: Repository) -> None: + """Rail 9's invariant on the live path: a falling cycle proposes a trail BELOW the recorded + stop, and the step must not roll at all -- the existing bracket stays resting, the recorded + stop unmoved. `next_stop` is ratchet-only by construction; this pins that the LIVE step + inherits it (it only rolls when the level strictly improves).""" + repo.insert_rule( + "pullback_continuation", + {"product_id": PRODUCT, "granularity": "ONE_DAY", "trail_atr_mult": "1.5"}, + status="live", + ) + bracket_id = _seed_bracketed_tranche(repo, initial_stop=Decimal("52000")) + falling = [ + _ranged_candle( + _bar_ts(i), + low=Decimal("53000") - Decimal("120") * i - Decimal("60"), + high=Decimal("53000") - Decimal("120") * i + Decimal("60"), + close=Decimal("53000") - Decimal("120") * i, + open_=Decimal("53000") - Decimal("120") * (i - 1) if i else Decimal("53000"), + ) + for i in range(30) + ] + _seed_history(repo, falling) + broker = FakeBroker(series={(PRODUCT, Granularity.ONE_DAY): falling}) + + run_once(broker, repo, _config(), now_ts=_management_now_ts()) + + assert "cancel" not in broker.events + assert repo.get_state(f"open_stop:{PRODUCT}") == Decimal("52000") + assert repo.get_order(bracket_id)["status"] == "pending" + + +def test_run_once_rolls_to_break_even_once_the_trade_reaches_be_roll_rr(repo: Repository) -> None: + """The other arm, opted in alone: a bar whose HIGH clears `entry + be_roll_rr x` the + ORIGINAL per-unit risk (the tranche's `initial_stop`, #520 -- never the already-raised + current stop) rolls the stop to the entry.""" + repo.insert_rule( + "pullback_continuation", + {"product_id": PRODUCT, "granularity": "ONE_DAY", "be_roll_rr": "1"}, + status="live", + ) + bracket_id = _seed_bracketed_tranche(repo) + series = [ + _ranged_candle( + _bar_ts(i), + low=Decimal("49950"), + high=Decimal("50050"), + close=Decimal("50000"), + open_=Decimal("50000"), + ) + for i in range(29) + ] + [ + # high 51200 clears entry + 1R (50000 + 1 * (50000 - 49000) = 51000) + _ranged_candle( + _bar_ts(29), + low=Decimal("50300"), + high=Decimal("51200"), + close=Decimal("50800"), + open_=Decimal("50000"), + ) + ] + _seed_history(repo, series) + broker = FakeBroker(series={(PRODUCT, Granularity.ONE_DAY): series}) + + run_once(broker, repo, _config(), now_ts=_management_now_ts()) + + assert repo.get_state(f"open_stop:{PRODUCT}") == Decimal("50000") + assert repo.get_order(bracket_id)["status"] == "canceled" + assert broker.events[-2:] == ["cancel", "place"], broker.events + + +def test_a_tranche_without_initial_stop_disables_the_break_even_arm(repo: Repository) -> None: + """The ledger's own contract (`Repository.open_position` / #520's migration): `initial_stop + IS NULL` means "nobody recorded it" -- pre-ledger tranches and DCA -- and the BE arm must + switch OFF rather than substitute the current stop, which is a different policy (and, on a + ratcheted position, a guaranteed profit-stealing roll to a stale level).""" + repo.insert_rule( + "pullback_continuation", + {"product_id": PRODUCT, "granularity": "ONE_DAY", "be_roll_rr": "1"}, + status="live", + ) + bracket_id = _seed_bracketed_tranche(repo, initial_stop=None) + series = [ + _ranged_candle( + _bar_ts(i), + low=Decimal("49950"), + high=Decimal("50050"), + close=Decimal("50000"), + open_=Decimal("50000"), + ) + for i in range(29) + ] + [ + _ranged_candle( + _bar_ts(29), + low=Decimal("50300"), + high=Decimal("51200"), + close=Decimal("50800"), + open_=Decimal("50000"), + ) + ] + _seed_history(repo, series) + broker = FakeBroker(series={(PRODUCT, Granularity.ONE_DAY): series}) + + run_once(broker, repo, _config(), now_ts=_management_now_ts()) + + assert "cancel" not in broker.events + assert repo.get_state(f"open_stop:{PRODUCT}") == Decimal("49000") + assert repo.get_order(bracket_id)["status"] == "pending" + + +def test_turtle_positions_are_never_managed(repo: Repository) -> None: + """`policy_for` reads turtle as OFF by DESIGN (#442 hypothesis 3): its real exit is the + asymmetric Donchian channel, and a trail would cut the rare long winners a low-win-rate + trend-follower exists to let run. The family carries neither knob and cannot express + them.""" + repo.insert_rule("turtle_breakout", {"product_id": PRODUCT}, status="live") + bracket_id = _seed_bracketed_tranche(repo, rule_name="turtle_breakout") + flat = [ + _ranged_candle( + _bar_ts(i), + low=Decimal("49950"), + high=Decimal("50050"), + close=Decimal("50000"), + open_=Decimal("50000"), + ) + for i in range(30) + ] + _seed_history(repo, flat) + broker = FakeBroker(series={(PRODUCT, Granularity.ONE_DAY): flat}) + + run_once(broker, repo, _config(), now_ts=_management_now_ts()) + + assert "cancel" not in broker.events + assert repo.get_state(f"open_stop:{PRODUCT}") == Decimal("49000") + assert repo.get_order(bracket_id)["status"] == "pending" + + +def test_paper_cycles_do_not_manage_exchange_brackets(repo: Repository) -> None: + """Paper mode never places exchange-side brackets (its entries resolve on the synthetic + account), so there is nothing for a live step to roll -- and touching the broker's real + order book from the paper path would be a category error. The step is live-mode only.""" + repo.insert_rule( + "pullback_continuation", + {"product_id": PRODUCT, "granularity": "ONE_DAY", "trail_atr_mult": "1.5"}, + status="paper", + ) + bracket_id = _seed_bracketed_tranche(repo) + # The SAME data the trail test rolls on -- so "no roll" here proves the PAPER gate blocked + # the step, not an unlucky series. + series = _rising_series() + _seed_history(repo, series) + broker = FakeBroker(series={(PRODUCT, Granularity.ONE_DAY): series}) + + run_once( + broker, + repo, + _config(auto_trade=AutoTradeConfig(mode="paper", interval_sec=50_000)), + now_ts=_management_now_ts(), + ) + + assert "cancel" not in broker.events + assert broker.place_calls == [] + assert repo.get_state(f"open_stop:{PRODUCT}") == Decimal("49000") + assert repo.get_order(bracket_id)["status"] == "pending" + + def test_a_rule_exit_records_one_outcome_per_tranche(repo: Repository) -> None: """The other half of the per-tranche ledger, and the half the plan originally left behind. From b7ca9907df9450e5378a5ecfebdd1bf1e5863c66 Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Thu, 27 Aug 2026 21:57:26 -0400 Subject: [PATCH 3/6] fix(agent): stops manage under the OWNING row's policy, per product (#502) The rules table holds one row per (kind, product) and Rule.name is the FAMILY name, so the step's name-keyed dict governed a multi-product family's every tranche under whichever same-family row loaded last: an opted-in BTC row managing a knob-less ETH tranche's bracket, or the reverse. Key by (product_id, name) and look up (tranche.product_id, tranche.rule_name) -- the same product scoping _handle_exits uses. Pinned by test_a_family_row_only_manages_the_tranches_of_its_own_product: two same-family rows with different knobs on different products; the knob-less product's tranche sees no cancel/place, the opted-in product's does. --- keel/agent.py | 13 ++++++--- tests/test_agent.py | 70 ++++++++++++++++++++++++++++++++++++++------- 2 files changed, 69 insertions(+), 14 deletions(-) diff --git a/keel/agent.py b/keel/agent.py index b89ccc5..f0f15e4 100644 --- a/keel/agent.py +++ b/keel/agent.py @@ -829,7 +829,12 @@ def _manage_stops( now_ts: int, ) -> None: """The per-cycle live stop-management step (#502 stage 2): ratchet each held tranche's - resting bracket according to its OWNING rule's exit policy. + resting bracket according to its OWNING rule's exit policy -- the rule row that owns the + tranche's PRODUCT, never merely a row sharing its family name (the rules table holds one + row per (kind, product), so a name-only key would govern a multi-product family's every + tranche under whichever same-family row loaded last: an opted-in BTC row managing a + knob-less ETH tranche's bracket, or the reverse; `_handle_exits` scopes ownership by + product for the same reason). DEFAULT OFF, exactly like the sim wiring. The knobs are per-rule-family params (`trail_atr_mult` / `be_roll_rr` on `pullback_continuation` and `rsi_meanrev`), a rule @@ -854,11 +859,11 @@ def _manage_stops( refusals -- all in the executor, none re-invented here). Paper cycles never call this: paper entries place no exchange-side brackets, so there is nothing to roll. """ - rules_by_name = {rule.name: rule for rule in rules} + rules_by_owner = {(getattr(rule, "product_id", None), rule.name): rule for rule in rules} for tranche in repo.get_open_positions(): - rule = rules_by_name.get(tranche["rule_name"]) + rule = rules_by_owner.get((tranche["product_id"], tranche["rule_name"])) if rule is None: - continue # the owning rule is not on this cycle's set (demoted/retired): no policy + continue # the owning row is not on this cycle's set (demoted/retired): no policy policy = policy_for(rule) if policy is EXIT_POLICY_OFF: continue # the default: the rule never asked for stop management diff --git a/tests/test_agent.py b/tests/test_agent.py index 94d2cc9..f1641df 100644 --- a/tests/test_agent.py +++ b/tests/test_agent.py @@ -1649,6 +1649,8 @@ def _seed_bracketed_tranche( repo: Repository, *, rule_name: str = "pullback_continuation", + product: str = PRODUCT, + bracket_ref: str = "cb-bracket-1", entry: Decimal = Decimal("50000"), initial_stop: Decimal | None = Decimal("49000"), qty: Decimal = Decimal("0.01"), @@ -1659,13 +1661,15 @@ def _seed_bracketed_tranche( order id. `initial_stop=None` seeds the pre-#520 TRANCHE shape the ledger documents as "BE arm disabled", not "zero" -- while the resting bracket's own stop (`open_stop:` state and the order row's `expected_fill`) stays a real number, because it always is: the bracket - exists, so it rests at some level.""" + exists, so it rests at some level. `bracket_ref` is the broker-side order id the cancel + path reads out of `raw_response` -- distinct per product so a multi-product test can tell + whose cancel was whose.""" stop = initial_stop if initial_stop is not None else Decimal("49000") - _seed_open_position(repo, PRODUCT, qty, entry, ts=ts) + _seed_open_position(repo, product, qty, entry, ts=ts) bracket_id = repo.insert_order( dict( mode="live", - product_id=PRODUCT, + product_id=product, side=Side.SELL.value, order_type="market", qty=qty, @@ -1674,14 +1678,14 @@ def _seed_bracketed_tranche( fee=None, expected_fill=stop, actual_fill=None, - raw_response='{"order_id": "cb-bracket-1"}', + raw_response=f'{{"order_id": "{bracket_ref}"}}', created_at=ts, updated_at=ts, ) ) - repo.set_state(f"position_rule:{PRODUCT}", {"rule_name": rule_name, "opened_at": ts}) + repo.set_state(f"position_rule:{product}", {"rule_name": rule_name, "opened_at": ts}) repo.open_position( - product_id=PRODUCT, + product_id=product, rule_name=rule_name, opened_at=ts, qty=qty, @@ -1690,8 +1694,8 @@ def _seed_bracketed_tranche( initial_stop=initial_stop, bracket_order_id=bracket_id, ) - repo.set_state(f"open_stop:{PRODUCT}", stop) - repo.set_state(f"open_target:{PRODUCT}", target) + repo.set_state(f"open_stop:{product}", stop) + repo.set_state(f"open_target:{product}", target) return bracket_id @@ -1730,12 +1734,12 @@ def _rising_series( ] -def _seed_history(repo: Repository, series: list[Candle]) -> None: +def _seed_history(repo: Repository, series: list[Candle], product: str = PRODUCT) -> None: """Pre-store the series in the candles table. `market_feed.poll_once` cold-starts an EMPTY table with only the newest closed bar (its catch-up start is `latest_closed`), so a test that means "a deployment with ATR history" must seed the table itself -- the cycle's poll then finds the tail current and stores nothing new.""" - repo.upsert_candles(PRODUCT, Granularity.ONE_DAY, series) + repo.upsert_candles(product, Granularity.ONE_DAY, series) def test_run_once_leaves_stops_alone_when_the_rule_carries_no_exit_knobs(repo: Repository) -> None: @@ -1971,6 +1975,52 @@ def test_paper_cycles_do_not_manage_exchange_brackets(repo: Repository) -> None: assert repo.get_order(bracket_id)["status"] == "pending" +def test_a_family_row_only_manages_the_tranches_of_its_own_product(repo: Repository) -> None: + """PRODUCT-SCOPED ownership (#502 review): the rules table holds ONE row per + (kind, product) and `Rule.name` is the FAMILY name, so resolving a tranche's policy by + name alone governed a multi-product family's every tranche under whichever same-family + row loaded last -- here the ETH row (id 1, knob-less) loses a name-only dict to the BTC + row (id 2, opted in), and BTC's trail policy would have rolled ETH's bracket on a climb + ETH never asked for. The step keys rows by `(product_id, name)` -- the same product + scoping `_handle_exits` uses -- so each product's tranche answers to its OWN row.""" + eth = "ETH-USD" + repo.insert_rule( + "pullback_continuation", + {"product_id": eth, "granularity": "ONE_DAY"}, + status="live", + ) + repo.insert_rule( + "pullback_continuation", + {"product_id": PRODUCT, "granularity": "ONE_DAY", "trail_atr_mult": "1.5"}, + status="live", + ) + btc_bracket = _seed_bracketed_tranche( + repo, bracket_ref="btc-bracket-1", target=Decimal("60000") + ) + eth_bracket = _seed_bracketed_tranche(repo, product=eth, bracket_ref="eth-bracket-1", ts=2_000) + # The SAME climb on both products: under a name-only key this series rolls ETH too. + series = _rising_series(57) + _seed_history(repo, series) + _seed_history(repo, series, product=eth) + broker = FakeBroker( + series={ + (PRODUCT, Granularity.ONE_DAY): series, + (eth, Granularity.ONE_DAY): series, + } + ) + + run_once(broker, repo, _config(), now_ts=_management_now_ts(57)) + + assert "eth-bracket-1" not in broker.cancel_calls, ( + "the knob-less product's tranche was managed under the opted-in row's policy" + ) + assert repo.get_order(eth_bracket)["status"] == "pending" + assert repo.get_state(f"open_stop:{eth}") == Decimal("49000") + assert "btc-bracket-1" in broker.cancel_calls, "the opted-in product's tranche was not managed" + assert repo.get_order(btc_bracket)["status"] == "canceled" + assert repo.get_state(f"open_stop:{PRODUCT}") > Decimal("49000") + + def test_a_rule_exit_records_one_outcome_per_tranche(repo: Repository) -> None: """The other half of the per-tranche ledger, and the half the plan originally left behind. From 5d964a90714dfe8e0a65051f5c7760a990c9708a Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Thu, 27 Aug 2026 21:57:50 -0400 Subject: [PATCH 4/6] fix(agent): a failed roll is loud and isolated, never a dead cycle (#502) The per-cycle step called executor.roll_stop_to bare, so a reachable cancel failure (CancelPending/CancelUnavailable -- ordinary Coinbase batch-cancel outcomes when a fill lands during the roll) aborted run_once AFTER entries were placed: no LoopResult, no post-cycle notify, a nonzero CLI exit, and the live-run wrapper declining to stamp the UTC day -- the next trigger re-runs the cycle into the duplicate-entry window. Wrap each tranche's roll in a deliberately broad except: log CRITICAL with the traceback attached (keel-core's log_exception grows a level kwarg so the severity is not bought by dropping the stack) and continue to the next tranche. Pinned by test_a_failed_roll_is_loud_and_isolated_never_a_dead_cycle: a broker whose cancel_order raises still yields a LoopResult, the later tranche is still managed, the crash ledger stands for the sweep, and the CRITICAL agent.stop_management_roll_failed is on record. --- keel/agent.py | 58 +++++++++++++++++----- packages/keel-core/keel_core/telemetry.py | 18 ++++--- tests/test_agent.py | 60 +++++++++++++++++++++++ 3 files changed, 118 insertions(+), 18 deletions(-) diff --git a/keel/agent.py b/keel/agent.py index f0f15e4..34aa599 100644 --- a/keel/agent.py +++ b/keel/agent.py @@ -67,7 +67,13 @@ from keel_broker_api.results import MarketSchedule, SessionState from keel_core.products import quote_currency_of -from keel_core.telemetry import bind_cycle, log_event, new_cycle_id, unbind_cycle +from keel_core.telemetry import ( + bind_cycle, + log_event, + log_exception, + new_cycle_id, + unbind_cycle, +) from keel.config import Config from keel.data import freshness, market_feed @@ -858,6 +864,15 @@ def _manage_stops( (#519's cancel-before-place protocol, the crash ledger, the ratchet and at/above-target refusals -- all in the executor, none re-invented here). Paper cycles never call this: paper entries place no exchange-side brackets, so there is nothing to roll. + + A failed roll is LOUD and ISOLATED, never a dead cycle. The roll's cancel half is a + live-money action with ordinary failure modes (`CancelPending` / `CancelUnavailable` are + what Coinbase's batch-cancel answers when a fill lands during the roll), so each + tranche's roll is wrapped: a raise is logged at CRITICAL -- a possibly-half-completed + action on live money must be loud -- and the step CONTINUES to the next tranche. Letting + it propagate would abort `run_once` AFTER entries were placed: no `LoopResult`, no + post-cycle notify, a nonzero CLI exit, and the live-run wrapper declining to stamp the + UTC day -- so the next trigger re-runs the whole cycle into the duplicate-entry window. """ rules_by_owner = {(getattr(rule, "product_id", None), rule.name): rule for rule in rules} for tranche in repo.get_open_positions(): @@ -915,17 +930,36 @@ def _manage_stops( if level <= current_stop: continue # the ratchet: nothing proposed toward profit this cycle - executor.roll_stop_to( - broker, - repo, - config, - product_id=tranche["product_id"], - old_stop_order_id=bracket_id, - new_stop=level, - qty=tranche["qty"], - rule_name=tranche["rule_name"], - now_ts=now_ts, - ) + try: + executor.roll_stop_to( + broker, + repo, + config, + product_id=tranche["product_id"], + old_stop_order_id=bracket_id, + new_stop=level, + qty=tranche["qty"], + rule_name=tranche["rule_name"], + now_ts=now_ts, + ) + except Exception: + # Per-tranche isolation, deliberately BROAD: `CancelPending`/`CancelUnavailable` + # from the fill-during-roll race is the expected raise, but a possibly- + # half-completed action on live money must not be graded by exception type + # here. The position may be unprotected RIGHT NOW (the cancel may have landed + # before the raise), so this is CRITICAL with the traceback attached -- and the + # cycle SURVIVES, per the docstring: a dead `run_once` after entries were + # placed re-runs the cycle into the duplicate-entry window. + log_exception( + logger, + "agent.stop_management_roll_failed", + level=logging.CRITICAL, + product=tranche["product_id"], + rule=rule.name, + old_stop_order_id=bracket_id, + attempted_stop=level, + ) + continue def _management_timeframe(rule: Rule, candles_by_tf: dict[Granularity, list[Any]]) -> Granularity: diff --git a/packages/keel-core/keel_core/telemetry.py b/packages/keel-core/keel_core/telemetry.py index 888699a..d402b8e 100644 --- a/packages/keel-core/keel_core/telemetry.py +++ b/packages/keel-core/keel_core/telemetry.py @@ -134,13 +134,19 @@ def log_event(logger: logging.Logger, level: int, event: str, /, **fields: Any) logger.log(level, event, extra={_FIELDS_ATTR: fields}) -def log_exception(logger: logging.Logger, event: str, /, **fields: Any) -> None: - """Emit a structured `event` at ERROR with the active exception's traceback attached. - - Use inside an `except` block. Equivalent to `log_event` at ERROR level plus `exc_info`, - which `JsonFormatter` renders into the payload's `exc` key. +def log_exception( + logger: logging.Logger, event: str, /, *, level: int = logging.ERROR, **fields: Any +) -> None: + """Emit a structured `event` at ERROR (or `level`) with the active exception's traceback. + + Use inside an `except` block. Equivalent to `log_event` at the given level plus + `exc_info`, which `JsonFormatter` renders into the payload's `exc` key. `level` exists + for the one severity a traceback must not downgrade from: a caller reporting a + possibly-half-completed action on live money (#502's stop-management roll) logs at + CRITICAL and still keeps the stack. Like `log_event`'s positional-only guard, `level` + is keyword-only; no existing caller passes a field of that name. """ - logger.log(logging.ERROR, event, exc_info=True, extra={_FIELDS_ATTR: fields}) + logger.log(level, event, exc_info=True, extra={_FIELDS_ATTR: fields}) def is_venue_unreachable(exc: BaseException | None) -> bool: diff --git a/tests/test_agent.py b/tests/test_agent.py index f1641df..94c96f9 100644 --- a/tests/test_agent.py +++ b/tests/test_agent.py @@ -2021,6 +2021,66 @@ def test_a_family_row_only_manages_the_tranches_of_its_own_product(repo: Reposit assert repo.get_state(f"open_stop:{PRODUCT}") > Decimal("49000") +def test_a_failed_roll_is_loud_and_isolated_never_a_dead_cycle(repo: Repository, caplog) -> None: + """PER-TRANCHE ISOLATION (#502 review): a cancel that raises mid-roll -- `CancelPending`/ + `CancelUnavailable` are ordinary Coinbase batch-cancel outcomes when a fill lands during + the roll -- must not abort `run_once`. A dead cycle AFTER entries were placed costs the + `LoopResult`, the post-cycle notify, and the live-run wrapper's UTC day-stamp, so the + next trigger re-runs the whole cycle into the duplicate-entry window. The failure is + CRITICAL (the roll may be half-completed on live money), the crash-ledger record stands + for the sweep to heal from, and the LATER tranche is still managed.""" + eth = "ETH-USD" + for product in (PRODUCT, eth): + repo.insert_rule( + "pullback_continuation", + {"product_id": product, "granularity": "ONE_DAY", "trail_atr_mult": "1.5"}, + status="live", + ) + btc_bracket = _seed_bracketed_tranche( + repo, bracket_ref="btc-bracket-1", target=Decimal("60000") + ) + eth_bracket = _seed_bracketed_tranche( + repo, product=eth, bracket_ref="eth-bracket-2", ts=2_000, target=Decimal("60000") + ) + series = _rising_series(57) + _seed_history(repo, series) + _seed_history(repo, series, product=eth) + + class _CancelDeniedBroker(FakeBroker): + def cancel_order(self, order_id: str) -> bool: + if order_id == "btc-bracket-1": + raise RuntimeError("batch cancel unavailable (fill in flight)") + return super().cancel_order(order_id) + + broker = _CancelDeniedBroker( + series={ + (PRODUCT, Granularity.ONE_DAY): series, + (eth, Granularity.ONE_DAY): series, + } + ) + + with caplog.at_level(logging.CRITICAL): + result = run_once(broker, repo, _config(), now_ts=_management_now_ts(57)) + + assert result.skipped is False, "a failed roll killed the whole cycle" + failures = [ + (record.getMessage(), getattr(record, _FIELDS_ATTR, {})) + for record in caplog.records + if record.getMessage() == "agent.stop_management_roll_failed" + ] + assert len(failures) == 1 + assert failures[0][1]["product"] == PRODUCT + assert failures[0][1]["old_stop_order_id"] == btc_bracket + # The raise lands inside the executor's cancel, BEFORE the local `canceled` mark, so + # the old bracket still rests and the crash ledger stands for the next cycle's sweep. + assert repo.get_order(btc_bracket)["status"] == "pending" + assert repo.get_state(f"unbracketed:{PRODUCT}") is not None + # The LATER tranche was still managed -- isolation, not abandonment. + assert "eth-bracket-2" in broker.cancel_calls + assert repo.get_order(eth_bracket)["status"] == "canceled" + assert repo.get_state(f"open_stop:{eth}") > Decimal("49000") + + def test_a_rule_exit_records_one_outcome_per_tranche(repo: Repository) -> None: """The other half of the per-tranche ledger, and the half the plan originally left behind. From 3a328699b394958940ade7ac0e9c12494f3f9d05 Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Thu, 27 Aug 2026 22:00:44 -0400 Subject: [PATCH 5/6] fix(agent): warmup, visibility, and the quiet corners of stop management (#502) Minors from review, plus the unpinned paths: - Young-table warmup: the trail level only matches the #442-measured sim behavior once the table holds 4*atr_period+1 closed bars (poll_once cold-starts at ONE bar; backfill has no production caller), so the trail arm is off short of that; the BE arm is NOT equally affected (it reads only the latest bar's high) and stays live. The waiting notice is INFO, once per product, not per cycle. - Stale-product skip: the comment claimed 'no completed bar', which is wrong (the table may hold bars; the cycle fetched none) and the skip was silent; it now logs at the empty-TF case's INFO volume. - Kill-switch window documented on _roll_stop/_manage_stops: a cancel is not rail-gated, so a switch engaging mid-roll fails only the replacement (rail 12) and leaves the position unbracketed until it clears; the CRITICAL is loud, reconcile heals when cycles resume. - The open_stop read inside the per-tranche loop now says out loud that it assumes one tranche per asset; per-tranche keying is a pyramiding prerequisite. _management_timeframe's docstring names its bounded divergence from backtest's ONE_HOUR fallback. - Pins: the dict-miss skip (no fallback to a row the tranche does not own) and the empty-timeframe skip (with its INFO event); a both-knobs test proving exactly ONE roll per cycle; the duplicated _seed_history call dropped. The 57-bar fixtures sit at the warmup threshold, past the at/above-target refusal's reach. --- keel/agent.py | 64 ++++++++++++- keel/execution/executor.py | 7 ++ tests/test_agent.py | 185 +++++++++++++++++++++++++++++++++++-- 3 files changed, 243 insertions(+), 13 deletions(-) diff --git a/keel/agent.py b/keel/agent.py index 34aa599..b4d0c82 100644 --- a/keel/agent.py +++ b/keel/agent.py @@ -826,6 +826,14 @@ def _close_tranches( repo.close_position(position["id"], closed_at=now_ts) +#: Products whose young-table warmup notice has already been logged THIS PROCESS (#502). The +#: notice is per product, not per cycle -- a daily deployment a few bars short of the +#: `4 x atr_period + 1` threshold would otherwise repeat the same line every cycle until +#: warmed. Process scope is the deliberate bound: an `agent_state` key would outlive the +#: condition it describes and suppress the notice forever after a restart. +_WARMUP_LOGGED_PRODUCTS: set[str] = set() + + def _manage_stops( broker: Any, repo: Repository, @@ -873,6 +881,13 @@ def _manage_stops( it propagate would abort `run_once` AFTER entries were placed: no `LoopResult`, no post-cycle notify, a nonzero CLI exit, and the live-run wrapper declining to stamp the UTC day -- so the next trigger re-runs the whole cycle into the duplicate-entry window. + + The kill-switch window inside a roll (see `executor._roll_stop`): a cancel is not + rail-gated -- it REMOVES risk, so there is nothing for a guard to veto -- and a switch + that engages mid-roll, after the cancel, fails only the REPLACEMENT closed (rail 12 + fails every order), leaving the position unbracketed until the switch clears. That is + the correct failure direction, and it is not silent: the CRITICAL is loud, and + `reconcile` heals from the crash ledger when cycles resume. """ rules_by_owner = {(getattr(rule, "product_id", None), rule.name): rule for rule in rules} for tranche in repo.get_open_positions(): @@ -889,6 +904,10 @@ def _manage_stops( old_order = repo.get_order(bracket_id) if old_order is None or old_order["status"] not in executor.RESTING_STATUSES: continue # filled or dead: reconciliation owns that bracket, not this step + # Per-PRODUCT ratchet state read inside the per-TRANCHE loop: this assumes the + # ledger's one-tranche-per-asset shape (the same assumption that lets the defers + # treat a product's slots as exclusive). Keying the ratchet per TRANCHE is a + # prerequisite for pyramiding, and is deliberately not invented here. current_stop = repo.get_state(f"open_stop:{tranche['product_id']}") if current_stop is None: continue # no recorded level to ratchet from -- nothing this step may act on @@ -903,7 +922,20 @@ def _manage_stops( candles_by_tf = candles_by_tf_by_product.get(tranche["product_id"]) if not candles_by_tf: - continue # stale product this cycle: no completed bar to manage on + # Stale product this cycle: the freshness pre-pass skipped it, so THIS cycle + # fetched no bars for it -- which is not "the table holds nothing" (it usually + # holds older closed bars; staleness is a claim about the newest one). The + # tranche waits a cycle, and says so at the same INFO volume as the + # empty-timeframe skip below rather than skipping silently. + log_event( + logger, + logging.INFO, + "agent.stop_management_skipped", + product=tranche["product_id"], + rule=rule.name, + reason="product skipped this cycle (stale feed) -- no bars fetched", + ) + continue series = candles_by_tf.get(_management_timeframe(rule, candles_by_tf)) or [] if not series: log_event( @@ -916,6 +948,29 @@ def _manage_stops( ) continue + # Young-table warmup (#442 fidelity). `trailing_atr` prices the trail off a + # Wilder average that only matches the #442-measured sim behavior once the table + # holds `4 x atr_period + 1` closed bars, and the live table does not start there: + # `market_feed.poll_once` cold-starts an EMPTY table with one bar, and backfill has + # no production caller. Short of the threshold the trail arm is OFF for the cycle. + # The BE arm is NOT equally affected -- it reads only the latest bar's HIGH against + # thresholds fixed at entry time, which is exactly what the sim does from bar one -- + # so it stays live. The notice fires once per product, not per cycle. + warmup_bars = 4 * policy.atr_period + 1 + if policy.trail_atr_mult is not None and len(series) < warmup_bars: + policy = replace(policy, trail_atr_mult=None) + if tranche["product_id"] not in _WARMUP_LOGGED_PRODUCTS: + _WARMUP_LOGGED_PRODUCTS.add(tranche["product_id"]) + log_event( + logger, + logging.INFO, + "agent.stop_management_waiting_for_warmup", + product=tranche["product_id"], + rule=rule.name, + bars=len(series), + needed=warmup_bars, + ) + atr = trailing_atr(series, policy.atr_period) level = next_stop( policy, @@ -966,8 +1021,11 @@ def _management_timeframe(rule: Rule, candles_by_tf: dict[Granularity, list[Any] """The series stop management reads: the rule's own trading timeframe when it declares one (`granularity` on `TurtleBreakout`/`PullbackContinuation`, `timeframe` on `RsiMeanReversion` -- the same attribute order `engine._trading_granularity` and `backtest._rule_trading_tf` - use), else the finest series this cycle actually has. Every knob-carrying family declares - its timeframe, so the fallback is a bound, not a path anyone trades on.""" + use), else the finest series this cycle actually has. One deliberate divergence from the + backtester, bounded: `backtest._rule_trading_tf` falls back to a fixed ONE_HOUR, this falls + back to whatever the cycle polled finest -- but every knob-carrying family declares its + timeframe, so a rule that is ever actually managed (`policy_for` non-OFF) never takes a + fallback series at all, and the divergence is a bound, not a path anyone trades on.""" for attr in ("granularity", "timeframe"): value = getattr(rule, attr, None) if isinstance(value, Granularity): diff --git a/keel/execution/executor.py b/keel/execution/executor.py index 63c54e6..f17f492 100644 --- a/keel/execution/executor.py +++ b/keel/execution/executor.py @@ -1691,6 +1691,13 @@ def _roll_stop( entries. The replacement order still runs through `guards.check` (allowlist/caps/kill-switch/ etc. -- every order, no exceptions) before it is placed. + **The kill-switch window inside a roll.** A cancel is not rail-gated -- it REMOVES risk, and + there is nothing for a guard to veto -- so a kill switch that engages mid-roll, AFTER the + cancel, fails only the REPLACEMENT: rail 12 fails every order closed and the position is left + unbracketed until the switch clears. Never trading against a thrown switch is the correct + failure direction, but the window must be understood, not discovered: the CRITICAL below is + loud, and `reconcile_unbracketed_positions` heals from the crash ledger when cycles resume. + **The cancel-then-place window is not atomic and cannot be made so** (see the comment at the cancel below). What it CAN be is recoverable: an `unbracketed:` record is written before the venue is touched and cleared only once the replacement rests, so a process that diff --git a/tests/test_agent.py b/tests/test_agent.py index 94c96f9..29b2059 100644 --- a/tests/test_agent.py +++ b/tests/test_agent.py @@ -159,6 +159,16 @@ def _register_fake_rules(): del agent.RULE_REGISTRY["fake_no_exit"] +@pytest.fixture(autouse=True) +def _reset_warmup_notice(): + """Start every test with a clean per-product warmup-notice set: the notice is once per + product PER PROCESS (`agent._WARMUP_LOGGED_PRODUCTS`), so without a reset an earlier + test's notice would suppress the one a later test asserts on.""" + agent._WARMUP_LOGGED_PRODUCTS.clear() + yield + agent._WARMUP_LOGGED_PRODUCTS.clear() + + # -- fixtures / builders -------------------------------------------------------------------- @@ -1756,7 +1766,6 @@ def test_run_once_leaves_stops_alone_when_the_rule_carries_no_exit_knobs(repo: R bracket_id = _seed_bracketed_tranche(repo) series = _rising_series() _seed_history(repo, series) - _seed_history(repo, series) broker = FakeBroker(series={(PRODUCT, Granularity.ONE_DAY): series}) run_once(broker, repo, _config(), now_ts=_management_now_ts()) @@ -1775,7 +1784,11 @@ def test_run_once_trails_a_ratcheting_stop_through_the_broker(repo: Repository) closed candles only), then ONE roll -- #519's cancel-before-place protocol -- so the venue's bracket ratchets with the climb. The expected level is computed here with the policy's own functions: the live step's contract is fidelity to `strategy.exit_policy`, not a re-derived - trail of its own.""" + trail of its own. + + 57 bars is the young-table warmup threshold (`4 x 14 + 1`, the default ATR period) -- the + threshold side of the boundary `test_the_trail_arm_waits_for_a_warm_candle_table` pins + from below.""" from keel.strategy.exit_policy import next_stop, policy_for, trailing_atr repo.insert_rule( @@ -1783,12 +1796,15 @@ def test_run_once_trails_a_ratcheting_stop_through_the_broker(repo: Repository) {"product_id": PRODUCT, "granularity": "ONE_DAY", "trail_atr_mult": "1.5"}, status="live", ) - bracket_id = _seed_bracketed_tranche(repo) - series = _rising_series() + # A 57-bar climb trails to ~55.4k, past the default 55k target -- the at/above-target + # refusal (a stop that has caught the target is a coin flip) would veto the roll these + # tests exist to observe, so the fixture's target sits above the trail's reach. + bracket_id = _seed_bracketed_tranche(repo, target=Decimal("60000")) + series = _rising_series(57) _seed_history(repo, series) broker = FakeBroker(series={(PRODUCT, Granularity.ONE_DAY): series}) - run_once(broker, repo, _config(), now_ts=_management_now_ts()) + run_once(broker, repo, _config(), now_ts=_management_now_ts(57)) policy = policy_for(_build_rule(repo.get_rules("live")[0])) expected = next_stop( @@ -1807,14 +1823,16 @@ def test_run_once_trails_a_ratcheting_stop_through_the_broker(repo: Repository) replacement_id = repo.get_open_positions(PRODUCT)[0]["bracket_order_id"] assert replacement_id is not None and replacement_id != bracket_id assert repo.get_order(replacement_id)["status"] == "pending" - assert repo.get_state(f"open_target:{PRODUCT}") == Decimal("55000") + assert repo.get_state(f"open_target:{PRODUCT}") == Decimal("60000") def test_the_live_trail_never_widens_the_stop(repo: Repository) -> None: """Rail 9's invariant on the live path: a falling cycle proposes a trail BELOW the recorded stop, and the step must not roll at all -- the existing bracket stays resting, the recorded stop unmoved. `next_stop` is ratchet-only by construction; this pins that the LIVE step - inherits it (it only rolls when the level strictly improves).""" + inherits it (it only rolls when the level strictly improves). 57 bars so the trail arm is + PAST warmup and genuinely proposing -- a no-roll here is the ratchet refusing, not the + warmup gate muting the arm.""" repo.insert_rule( "pullback_continuation", {"product_id": PRODUCT, "granularity": "ONE_DAY", "trail_atr_mult": "1.5"}, @@ -1829,12 +1847,12 @@ def test_the_live_trail_never_widens_the_stop(repo: Repository) -> None: close=Decimal("53000") - Decimal("120") * i, open_=Decimal("53000") - Decimal("120") * (i - 1) if i else Decimal("53000"), ) - for i in range(30) + for i in range(57) ] _seed_history(repo, falling) broker = FakeBroker(series={(PRODUCT, Granularity.ONE_DAY): falling}) - run_once(broker, repo, _config(), now_ts=_management_now_ts()) + run_once(broker, repo, _config(), now_ts=_management_now_ts(57)) assert "cancel" not in broker.events assert repo.get_state(f"open_stop:{PRODUCT}") == Decimal("52000") @@ -1844,7 +1862,9 @@ def test_the_live_trail_never_widens_the_stop(repo: Repository) -> None: def test_run_once_rolls_to_break_even_once_the_trade_reaches_be_roll_rr(repo: Repository) -> None: """The other arm, opted in alone: a bar whose HIGH clears `entry + be_roll_rr x` the ORIGINAL per-unit risk (the tranche's `initial_stop`, #520 -- never the already-raised - current stop) rolls the stop to the entry.""" + current stop) rolls the stop to the entry. 30 bars also pins that the BE arm is NOT + young-table-gated (see `_manage_stops`'s warmup note): it reads only the latest bar's + high, exactly what the sim does from bar one.""" repo.insert_rule( "pullback_continuation", {"product_id": PRODUCT, "granularity": "ONE_DAY", "be_roll_rr": "1"}, @@ -2021,6 +2041,106 @@ def test_a_family_row_only_manages_the_tranches_of_its_own_product(repo: Reposit assert repo.get_state(f"open_stop:{PRODUCT}") > Decimal("49000") +def test_a_tranche_whose_owning_row_is_absent_this_cycle_is_skipped(repo: Repository) -> None: + """The dict-miss path, pinned: the tranche names a family whose row is not on this + cycle's (product, name) set -- demoted, retired, or simply never existed -- and the step + skips it rather than falling back to any OTHER row's policy, not even the same + product's different family's row (the opted-in `pullback_continuation` here must not + adopt a tranche opened by `rsi_meanrev`).""" + repo.insert_rule( + "pullback_continuation", + {"product_id": PRODUCT, "granularity": "ONE_DAY", "trail_atr_mult": "1.5"}, + status="live", + ) + bracket_id = _seed_bracketed_tranche(repo, rule_name="rsi_meanrev") + series = _rising_series(57) + _seed_history(repo, series) + broker = FakeBroker(series={(PRODUCT, Granularity.ONE_DAY): series}) + + run_once(broker, repo, _config(), now_ts=_management_now_ts(57)) + + assert "cancel" not in broker.events, "a tranche was managed under a row it does not own" + assert repo.get_state(f"open_stop:{PRODUCT}") == Decimal("49000") + assert repo.get_order(bracket_id)["status"] == "pending" + + +def test_management_skips_and_logs_when_the_rules_timeframe_has_no_candles( + repo: Repository, caplog +) -> None: + """The empty-timeframe path, pinned: the rule declares ONE_HOUR, this deployment polls + ONE_DAY only, so the management series is empty and the tranche waits a cycle WITH an + INFO skip -- never rolling on a bar that does not exist.""" + repo.insert_rule( + "pullback_continuation", + {"product_id": PRODUCT, "granularity": "ONE_HOUR", "trail_atr_mult": "1.5"}, + status="live", + ) + bracket_id = _seed_bracketed_tranche(repo) + series = _rising_series(57) + _seed_history(repo, series) + broker = FakeBroker(series={(PRODUCT, Granularity.ONE_DAY): series}) + + with caplog.at_level(logging.INFO): + run_once(broker, repo, _config(), now_ts=_management_now_ts(57)) + + skips = [ + (record.getMessage(), getattr(record, _FIELDS_ATTR, {})) + for record in caplog.records + if record.getMessage() == "agent.stop_management_skipped" + ] + assert skips == [ + ( + "agent.stop_management_skipped", + { + "product": PRODUCT, + "rule": "pullback_continuation", + "reason": "no candles on the rule's trading timeframe", + }, + ) + ] + assert "cancel" not in broker.events + assert repo.get_order(bracket_id)["status"] == "pending" + + +def test_the_trail_arm_waits_for_a_warm_candle_table(repo: Repository, caplog) -> None: + """Young-table warmup (#502 review): the trail level only matches the #442-measured sim + behavior once the table holds `4 x atr_period + 1` closed bars, and the live table does + not start there (`poll_once` cold-starts at ONE bar; backfill has no production caller). + 56 bars -- one short of 4 x 14 + 1 -- carries the trail knob but must NOT roll, and must + say so at INFO once per product, not once per cycle (the second cycle stays quiet). The + threshold side of the boundary is + `test_run_once_trails_a_ratcheting_stop_through_the_broker` (57 bars, rolls).""" + repo.insert_rule( + "pullback_continuation", + {"product_id": PRODUCT, "granularity": "ONE_DAY", "trail_atr_mult": "1.5"}, + status="live", + ) + bracket_id = _seed_bracketed_tranche(repo) + series = _rising_series(56) + _seed_history(repo, series) + broker = FakeBroker(series={(PRODUCT, Granularity.ONE_DAY): series}) + + with caplog.at_level(logging.INFO): + run_once(broker, repo, _config(), now_ts=_management_now_ts(56)) + run_once(broker, repo, _config(), now_ts=_management_now_ts(56) + 500) + + notices = [ + record + for record in caplog.records + if record.getMessage() == "agent.stop_management_waiting_for_warmup" + ] + assert len(notices) == 1, "the warmup notice is per product, not per cycle" + assert getattr(notices[0], _FIELDS_ATTR, {}) == { + "product": PRODUCT, + "rule": "pullback_continuation", + "bars": 56, + "needed": 57, + } + assert "cancel" not in broker.events, "a young table rolled a trail anyway" + assert repo.get_state(f"open_stop:{PRODUCT}") == Decimal("49000") + assert repo.get_order(bracket_id)["status"] == "pending" + + def test_a_failed_roll_is_loud_and_isolated_never_a_dead_cycle(repo: Repository, caplog) -> None: """PER-TRANCHE ISOLATION (#502 review): a cancel that raises mid-roll -- `CancelPending`/ `CancelUnavailable` are ordinary Coinbase batch-cancel outcomes when a fill lands during @@ -2081,6 +2201,51 @@ def cancel_order(self, order_id: str) -> bool: assert repo.get_state(f"open_stop:{eth}") > Decimal("49000") +def test_both_knobs_resolve_to_one_roll_per_cycle(repo: Repository) -> None: + """ONE roll, not one per arm (#502 review): a rule carrying BOTH knobs can win on both + in the same cycle, and the step must walk #519's cancel-before-place window exactly + once -- `next_stop` takes the max over the arms and `roll_stop_to` places exactly one + replacement, never `trail_stop_atr` and `roll_to_break_even` back to back.""" + from keel.strategy.exit_policy import next_stop, policy_for, trailing_atr + + repo.insert_rule( + "pullback_continuation", + { + "product_id": PRODUCT, + "granularity": "ONE_DAY", + "trail_atr_mult": "1.5", + "be_roll_rr": "1", + }, + status="live", + ) + # A 57-bar climb trails to ~55.4k, past the default 55k target -- the at/above-target + # refusal (a stop that has caught the target is a coin flip) would veto the roll these + # tests exist to observe, so the fixture's target sits above the trail's reach. + bracket_id = _seed_bracketed_tranche(repo, target=Decimal("60000")) + series = _rising_series(57) + _seed_history(repo, series) + broker = FakeBroker(series={(PRODUCT, Granularity.ONE_DAY): series}) + + run_once(broker, repo, _config(), now_ts=_management_now_ts(57)) + + policy = policy_for(_build_rule(repo.get_rules("live")[0])) + expected = next_stop( + policy, + Decimal("50000"), + Decimal("49000"), + Decimal("49000"), + series[-1], + trailing_atr(series, policy.atr_period), + ) + # Both arms genuinely contended: the climb clears the BE threshold (the last bar's high + # is ~55,660 >> 51,000) and the trail proposes higher still. + assert expected > Decimal("50000") + assert broker.events == ["cancel", "place"], broker.events + assert repo.get_state(f"open_stop:{PRODUCT}") == expected + assert repo.get_order(bracket_id)["status"] == "canceled" + assert repo.get_open_positions(PRODUCT)[0]["bracket_order_id"] is not None + + def test_a_rule_exit_records_one_outcome_per_tranche(repo: Repository) -> None: """The other half of the per-tranche ledger, and the half the plan originally left behind. From 8f54d7f7a2dae5a7276967b93d9fed72c92b9c1a Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Thu, 27 Aug 2026 22:58:35 -0400 Subject: [PATCH 6/6] test(agent): the ownership pin clears the target guard on BOTH products; nits from review --- tests/test_agent.py | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/tests/test_agent.py b/tests/test_agent.py index 29b2059..ac502e8 100644 --- a/tests/test_agent.py +++ b/tests/test_agent.py @@ -2017,7 +2017,13 @@ def test_a_family_row_only_manages_the_tranches_of_its_own_product(repo: Reposit btc_bracket = _seed_bracketed_tranche( repo, bracket_ref="btc-bracket-1", target=Decimal("60000") ) - eth_bracket = _seed_bracketed_tranche(repo, product=eth, bracket_ref="eth-bracket-1", ts=2_000) + # target=60000, NOT the 55000 default: the 57-bar climb trails to ~55.4k, and under a + # name-only key the leaked roll would be vetoed by the at/above-target refusal before any + # cancel -- leaving the test green against the very bug it exists to pin (the at/above-target + # guard fires before the cancel, so the fixture must stay clear of it on BOTH products). + eth_bracket = _seed_bracketed_tranche( + repo, product=eth, bracket_ref="eth-bracket-1", ts=2_000, target=Decimal("60000") + ) # The SAME climb on both products: under a name-only key this series rolls ETH too. series = _rising_series(57) _seed_history(repo, series) @@ -2110,15 +2116,16 @@ def test_the_trail_arm_waits_for_a_warm_candle_table(repo: Repository, caplog) - say so at INFO once per product, not once per cycle (the second cycle stays quiet). The threshold side of the boundary is `test_run_once_trails_a_ratcheting_stop_through_the_broker` (57 bars, rolls).""" + warmup_product = "WARM-USD" # unique: the once-per-product notice set is module state repo.insert_rule( "pullback_continuation", - {"product_id": PRODUCT, "granularity": "ONE_DAY", "trail_atr_mult": "1.5"}, + {"product_id": warmup_product, "granularity": "ONE_DAY", "trail_atr_mult": "1.5"}, status="live", ) - bracket_id = _seed_bracketed_tranche(repo) + bracket_id = _seed_bracketed_tranche(repo, product=warmup_product) series = _rising_series(56) - _seed_history(repo, series) - broker = FakeBroker(series={(PRODUCT, Granularity.ONE_DAY): series}) + _seed_history(repo, series, product=warmup_product) + broker = FakeBroker(series={(warmup_product, Granularity.ONE_DAY): series}) with caplog.at_level(logging.INFO): run_once(broker, repo, _config(), now_ts=_management_now_ts(56)) @@ -2131,13 +2138,13 @@ def test_the_trail_arm_waits_for_a_warm_candle_table(repo: Repository, caplog) - ] assert len(notices) == 1, "the warmup notice is per product, not per cycle" assert getattr(notices[0], _FIELDS_ATTR, {}) == { - "product": PRODUCT, + "product": warmup_product, "rule": "pullback_continuation", "bars": 56, "needed": 57, } assert "cancel" not in broker.events, "a young table rolled a trail anyway" - assert repo.get_state(f"open_stop:{PRODUCT}") == Decimal("49000") + assert repo.get_state(f"open_stop:{warmup_product}") == Decimal("49000") assert repo.get_order(bracket_id)["status"] == "pending" @@ -2184,13 +2191,14 @@ def cancel_order(self, order_id: str) -> bool: assert result.skipped is False, "a failed roll killed the whole cycle" failures = [ - (record.getMessage(), getattr(record, _FIELDS_ATTR, {})) + record for record in caplog.records if record.getMessage() == "agent.stop_management_roll_failed" ] assert len(failures) == 1 - assert failures[0][1]["product"] == PRODUCT - assert failures[0][1]["old_stop_order_id"] == btc_bracket + assert getattr(failures[0], _FIELDS_ATTR, {})["product"] == PRODUCT + assert getattr(failures[0], _FIELDS_ATTR, {})["old_stop_order_id"] == btc_bracket + assert failures[0].exc_info, "a CRITICAL half-completed live-money action carries the traceback" # The raise lands inside the executor's cancel, BEFORE the local `canceled` mark, so # the old bracket still rests and the crash ledger stands for the next cycle's sweep. assert repo.get_order(btc_bracket)["status"] == "pending"