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: