From 91dfa47b6f06995ff37a9a9a851eefd32f2731c6 Mon Sep 17 00:00:00 2001 From: Builder Date: Sun, 13 Sep 2026 14:04:55 +0800 Subject: [PATCH] fix(risk): stop the position limit being bypassed by a key mismatch (BUG-20) The limit checks read `order["ticker"]`: ticker = order.get("ticker", "") current_value = self.positions.get(ticker, {}).get("value", 0) `trading/live_runner.py` passed `{"code": ...}` -- the name `Order`, the positions table and the rest of the codebase use. That resolved to an empty ticker, found no existing position, and re-checked the limit from zero. Demonstrated with a 49% holding against a 5% single-name limit and a 10k order: {"ticker": "600519", ...} -> approved=False (weight 50.0% exceeds 5.0%) {"code": "600519", ...} -> approved=True <- limit bypassed The tests never caught it because they pass `ticker`, which is the key the function documents and the one key that worked. Two callers were wrong: trading/live_runner.py passed `code` strategy/portfolio_orchestrator called check_pre_trade(ticker=..., side=...) with keyword arguments against a method that takes a single dict, and then read `check.get('approved')` from a return value that is an (approved, breaches) tuple. Both wrong, so that path raised instead of checking anything. Fix, in three parts: 1. `check_pre_trade` accepts `ticker`, `symbol` or `code`. All three are names this codebase actually uses for the same thing, so accepting them is the honest contract; `risk/realtime_engine.py` already accepted ticker/symbol. 2. It **fails closed** on an order with no identifier at all. Position and sector limits cannot be evaluated without one, and the old behaviour -- silently checking from zero -- is precisely the bypass. Risk checks must not approve what they cannot evaluate. 3. Both callers pass the documented shape. Tests (5 new, parametrized over the three key names): - every alias hits the limit check (all three failed before for code/symbol) - an order with no identifier is blocked - an identifier with no position yet is still checked, not rejected, so the fail-closed rule does not over-block Verified by reverting the guard: 3 of the new tests fail, the `ticker` variant passes both ways -- it was the one path that worked. 1339 passed, 1 skipped. Co-Authored-By: Claude Opus 4.8 (1M context) --- risk/circuit_breaker.py | 24 ++++++++++++ strategy/portfolio_orchestrator.py | 27 ++++++++----- tests/test_risk/test_circuit_breaker.py | 52 +++++++++++++++++++++++++ trading/live_runner.py | 2 +- 4 files changed, 94 insertions(+), 11 deletions(-) diff --git a/risk/circuit_breaker.py b/risk/circuit_breaker.py index 0aae9ff..27f9ff8 100644 --- a/risk/circuit_breaker.py +++ b/risk/circuit_breaker.py @@ -111,6 +111,30 @@ def check_pre_trade(self, order: dict) -> tuple[bool, list[RiskBreach]]: """ breaches = [] + # A missing instrument key used to degrade silently: the limit checks + # below read `order.get("ticker", "")`, so an order dict keyed on `code` + # -- the name `Order` and the positions table use -- resolved to an + # empty ticker, found no existing position, and re-checked the limit + # from zero. A held position of 49% against a 5% limit passed. Risk + # checks fail closed, so an order we cannot identify is blocked rather + # than waved through, and the aliases the codebase actually uses are + # accepted instead of only the one this function documents. + instrument = order.get("ticker") or order.get("symbol") or order.get("code") + if not instrument: + breaches.append(RiskBreach( + breach_type=BreachType.POSITION_LIMIT, + severity=RiskLevel.KILL, + message=( + "Order has no instrument identifier (ticker/symbol/code) -- " + "blocked, because position and sector limits cannot be " + "evaluated without one" + ), + auto_action="block", + )) + return False, breaches + if order.get("ticker") != instrument: + order = {**order, "ticker": instrument} + if self.kill_switch_active: breaches.append(RiskBreach( breach_type=BreachType.POSITION_LIMIT, diff --git a/strategy/portfolio_orchestrator.py b/strategy/portfolio_orchestrator.py index 407f5a0..e6b20c3 100644 --- a/strategy/portfolio_orchestrator.py +++ b/strategy/portfolio_orchestrator.py @@ -184,17 +184,24 @@ def rebalance(self) -> list[dict]: quantity=self._round_lot(buy_qty), strategy=strategy_id, ) - # Pre-trade risk check + # Pre-trade risk check. + # + # This called `check_pre_trade(ticker=..., side=...)` with + # keyword arguments, but the method takes a single dict, and + # it then read `check.get('approved')` from a return value + # that is a (approved, breaches) tuple. Both were wrong, so + # this path raised rather than checking anything. if self.risk_monitor is not None: - check = self.risk_monitor.check_pre_trade( - ticker=ticker, - quantity=order.quantity, - price=price, - side=OrderSide.BUY, - ) - if not check.get('approved', True): - logger.warning('Order blocked by risk: %s', check.get('reason', '')) - order.notes = f'Risk blocked: {check.get("reason", "")}' + approved, breaches = self.risk_monitor.check_pre_trade({ + "ticker": ticker, + "quantity": order.quantity, + "price": price, + "side": "buy", + }) + if not approved: + reason = "; ".join(b.message for b in breaches) + logger.warning("Order blocked by risk: %s", reason) + order.notes = f"Risk blocked: {reason}" self.exec_engine.reject_order(order) continue self.exec_engine.submit_order(order) diff --git a/tests/test_risk/test_circuit_breaker.py b/tests/test_risk/test_circuit_breaker.py index 1c5fbeb..87265eb 100644 --- a/tests/test_risk/test_circuit_breaker.py +++ b/tests/test_risk/test_circuit_breaker.py @@ -1,5 +1,6 @@ """Tests for risk.circuit_breaker — RiskMonitor and circuit breakers.""" +import pytest from quant_platform.risk.circuit_breaker import ( BreachType, RiskLevel, @@ -107,3 +108,54 @@ def test_breach_history(self): "ticker": "600519", "side": "buy", "quantity": 100, "price": 100, }) assert len(self.monitor.breaches) > 0 + + +class TestInstrumentKeyAliases: + """Regression for BUG-20. + + Position and sector limits read `order["ticker"]`. `trading/live_runner.py` + passed `{"code": ...}` -- the name `Order`, the positions table and the rest + of the codebase use -- so the lookup resolved to an empty ticker, found no + existing position, and re-checked the limit from zero. A held position of + 49% against a 5% single-name limit was approved. + """ + + def _monitor_holding(self): + monitor = RiskMonitor() + monitor.update_portfolio_state( + portfolio_value=1_000_000, + daily_pnl=0, + positions={"600519": {"value": 490_000, "sector": "Tech"}}, + sector_weights={"Tech": 0.49}, + ) + return monitor + + @pytest.mark.parametrize("key", ["ticker", "code", "symbol"]) + def test_every_instrument_key_alias_hits_the_limit_check(self, key): + """A new order of 10k on top of a 49% holding must breach.""" + monitor = self._monitor_holding() + approved, breaches = monitor.check_pre_trade({ + key: "600519", "side": "buy", "quantity": 100, "price": 100, + }) + assert approved is False, f"{key!r} bypassed the position limit" + assert any(b.breach_type == BreachType.POSITION_LIMIT for b in breaches) + + def test_order_without_an_identifier_is_blocked(self): + """Risk checks fail closed: with no identifier the limits cannot be + evaluated, so approving would be a silent bypass.""" + monitor = self._monitor_holding() + approved, breaches = monitor.check_pre_trade({ + "side": "buy", "quantity": 100, "price": 100, + }) + assert approved is False + assert breaches + + def test_unknown_identifier_is_still_checked_not_short_circuited(self): + """An identifier that simply has no position yet is legitimate -- it + must be checked against the limit, not rejected.""" + monitor = self._monitor_holding() + approved, breaches = monitor.check_pre_trade({ + "code": "000001", "side": "buy", "quantity": 100, "price": 100, + }) + assert approved is True + assert breaches == [] diff --git a/trading/live_runner.py b/trading/live_runner.py index 7af2995..0a162ed 100644 --- a/trading/live_runner.py +++ b/trading/live_runner.py @@ -329,7 +329,7 @@ def _execute_signal(self, sig: dict) -> dict | None: # Risk pre-trade check side = OrderSide.BUY if sig["side"] == "buy" else OrderSide.SELL approved, breaches = self._risk.check_pre_trade({ - "code": code, "side": sig["side"], + "ticker": code, "side": sig["side"], "quantity": qty, "price": price, }) if not approved: