From a7c8a486d741825ae7d72a9546306a281e394136 Mon Sep 17 00:00:00 2001 From: yebof Date: Thu, 16 Jul 2026 19:01:26 +0800 Subject: [PATCH 1/4] =?UTF-8?q?fix(broker):=20CRITICAL=20=E2=80=94=20BUY-a?= =?UTF-8?q?ttached=20stops=20expired=20at=20the=20close,=20positions=20nak?= =?UTF-8?q?ed=20overnight?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found by the 2026-07-16 full-codebase audit; confirmed against the SDK, the production logs, and the live account. alpaca-py's StopLossRequest has NO time_in_force field of its own, so an OTO child leg inherits the PARENT's tif. The entry parent is (correctly) DAY — an unfilled entry limit must die at the close rather than fill into a stale thesis the next morning — which silently made every BUY-attached protective stop a DAY order too. Alpaca expired it at 16:00 ET the same session. Any position bought in the morning and not later handed a midday/close TRAIL_STOP (the only path that used the GTC _submit_stop_limit_order) sat COMPLETELY UNPROTECTED overnight — exactly when the gap risk the stop exists for happens. Production proof: VST bought 2026-06-26 09:47 ET with SL=$158.75; the SAME evening's coverage reconcile logged 'VST held=31.0000 but only 0.0000 covered'; the gap persisted until an LLM SELL exited at $152.77 on 07-01 — ~$185 worse than the stop would have capped. Control: the GTC trail-stops on GE survived day boundaries in the same account. Live check today: all 3 open stops are GTC stop-limits from the trail path; not one BUY-attached stop exists. This also contradicted the close-session prompt, which tells the reviewer to hold overnight BECAUSE a broker stop is standing watch. Fix (the leg's tif cannot be set independently, so decouple): - submit_order no longer attaches an OTO leg; the entry stays a plain DAY limit/market and returns pending_stop_price so the caller owes a stop. - broker.place_entry_protection(): waits for the entry to reach terminal, reads the ACTUAL fill, and places a GTC stop-limit for exactly that qty. This also fixes a latent bug — the OTO leg was sized to the REQUESTED qty, so a partial entry fill left a stop covering shares we never owned. - ExecutionStage protects every filled entry after the submission burst. - Belt: _reconcile_stop_coverage now REPAIRS a naked long instead of only flagging it, using the stop recorded on its last BUY (the level PM/RM approved — no longer 'unknown', which was the original objection to auto-repair). Guards: never place at/above the live price (that would fire instantly — an exit decision belongs to the reviewer), never invent a level. This retroactively protects anything the old bug left naked and covers a crash between an entry fill and the stop placement. Tests: entry carries no OTO leg + stays DAY; protection is GTC and sized to the actual fill; no-fill places nothing; stop-submit failure is swallowed; repair from the recorded BUY stop (full/partial/refuse-above-price/no-stop/ failure/covered). 1320 green. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01PQkESoSTYx2bCy7WYnTPXR --- src/execution/broker.py | 128 +++++++++++++++++++++-------- src/pipeline.py | 81 ++++++++++++++++-- src/pipeline_stages.py | 30 +++++++ tests/test_broker.py | 100 +++++++++++++++++++++- tests/test_stop_coverage_repair.py | 80 ++++++++++++++++++ 5 files changed, 376 insertions(+), 43 deletions(-) create mode 100644 tests/test_stop_coverage_repair.py diff --git a/src/execution/broker.py b/src/execution/broker.py index fc94d33b..2241e151 100644 --- a/src/execution/broker.py +++ b/src/execution/broker.py @@ -974,54 +974,45 @@ def submit_order(self, symbol: str, qty: float, side: str, ) return {"id": None, "status": "rejected_outlier", "symbol": symbol} - # Attach stop-loss as OTO (one-triggers-other) leg — no hard take-profit, - # profit management is handled by midday reviewer's trailing stop logic + # Protective stop for a BUY is placed as a SEPARATE GTC stop-limit + # AFTER the entry fills — NOT as an OTO leg. + # + # WHY (2026-07-16 audit, CRITICAL): `StopLossRequest` carries no + # time_in_force of its own, so an OTO child leg inherits the PARENT's + # TIF. The parent must be DAY (an unfilled entry limit must die at the + # close, never fill into a stale thesis the next morning) — which + # silently made every BUY-attached stop a DAY order too. Alpaca expired + # it at 16:00 ET the same session, so any position bought in the + # morning and not later given a midday/close TRAIL_STOP (which uses the + # GTC `_submit_stop_limit_order` path) sat NAKED overnight — precisely + # when gap risk is the reason the stop exists. Confirmed in production: + # VST bought 2026-06-26 09:47 ET with SL=$158.75; the same evening's + # coverage reconcile logged `VST held=31.0000 but only 0.0000 covered`; + # it was ultimately exited at $152.77 for ~$185 more loss than the stop + # would have capped. This also contradicted the close-session prompt, + # which tells the reviewer to hold overnight *because* the broker stop + # is standing watch. + # + # Placing the stop post-fill also fixes a second latent bug: the OTO + # leg was sized to the REQUESTED qty, so a partial entry fill left a + # stop covering more shares than we own. `_place_entry_protection` + # keys the stop to the ACTUAL filled qty. use_stop = (stop_loss_price is not None and stop_loss_price > 0 and order_side == OrderSide.BUY) - # Stop-limit instead of stop-market for BUY OTO brackets: - # On a gap-down (overnight earnings blowup, geopolitical shock), - # a plain stop_price is a market order — it fills at whatever price - # the book has, which can be 10%+ worse than the stop. A stop-limit - # caps the worst-case fill at `stop_limit_price`. We set the limit - # 3% below stop — user preference "prioritize fill over price" means - # this buffer needs to be generous enough that routine volatility - # clears it. Trade-off: on extreme gaps beyond −3% from stop, the - # stop-limit won't fill and the position stays open until the next - # midday review can act. Accepted for the upside of bounded exits. - STOP_LIMIT_BUFFER_PCT = 0.03 - stop_limit_price = None - if stop_loss_price is not None and stop_loss_price > 0: - stop_limit_price = _quantize_price(stop_loss_price * (1 - STOP_LIMIT_BUFFER_PCT)) - if limit_price is not None: - kwargs = dict( + request = LimitOrderRequest( symbol=symbol, qty=qty, side=order_side, time_in_force=TimeInForce.DAY, limit_price=limit_price, ) - if use_stop: - kwargs["order_class"] = OrderClass.OTO - kwargs["stop_loss"] = StopLossRequest( - stop_price=stop_loss_price, limit_price=stop_limit_price, - ) - request = LimitOrderRequest(**kwargs) else: - kwargs = dict( + request = MarketOrderRequest( symbol=symbol, qty=qty, side=order_side, time_in_force=TimeInForce.DAY, ) - if use_stop: - kwargs["order_class"] = OrderClass.OTO - kwargs["stop_loss"] = StopLossRequest( - stop_price=stop_loss_price, limit_price=stop_limit_price, - ) - request = MarketOrderRequest(**kwargs) order = self.client.submit_order(request) - bracket_info = ( - f" [SL=${stop_loss_price}/limit=${stop_limit_price}]" - if use_stop else "" - ) + bracket_info = f" [SL=${stop_loss_price} to be placed on fill]" if use_stop else "" logger.info("Order submitted: %s %s %s @ %s%s — status: %s", side, qty, symbol, limit_price or "market", bracket_info, str(getattr(order.status, "value", order.status))) @@ -1046,8 +1037,75 @@ def submit_order(self, symbol: str, qty: float, side: str, "qty": qty, "limit_price": limit_price, "stop_loss_price": stop_loss_price if use_stop else None, + # Signals the caller that this entry still OWES a protective stop + # (see _place_entry_protection). Absent/None => nothing to place. + "pending_stop_price": stop_loss_price if use_stop else None, } + # 3% below the stop: a stop-MARKET fills at whatever the book has on a + # gap-down (10%+ worse than the stop); a stop-limit caps the worst-case + # fill. The buffer must be wide enough that routine volatility clears it + # ("prioritize fill over price"). Trade-off: on gaps beyond -3% the limit + # won't fill and the position stays open until a session can act. + STOP_LIMIT_BUFFER_PCT = 0.03 + + def place_entry_protection( + self, symbol: str, order_id: str, stop_price: float, + *, requested_qty: float | None = None, + ) -> dict | None: + """Wait for an entry order to reach terminal, then place a GTC + protective stop-limit for the ACTUAL filled qty. + + Returns the stop order dict, or None when nothing was placed (entry + didn't fill / didn't converge / stop submit failed). Never raises — + a failure here must not abort the session, but it DOES leave the + position naked, so it logs at ERROR and relies on the next session's + `_reconcile_stop_coverage` auto-repair as the belt. + """ + try: + status = self.wait_for_order_terminal(order_id) + except Exception as exc: # noqa: BLE001 + logger.warning("entry protection: wait failed for %s (%s): %s", + symbol, order_id, exc) + status = None + try: + info = self.get_order_fill_info(order_id) or {} + except Exception as exc: # noqa: BLE001 + logger.warning("entry protection: fill info failed for %s: %s", symbol, exc) + info = {} + try: + filled_qty = float(info.get("filled_qty") or 0) + except (TypeError, ValueError): + filled_qty = 0.0 + if filled_qty <= 0: + logger.warning( + "entry protection: %s entry %s filled 0 (status=%s) — no stop " + "placed (nothing to protect)", symbol, order_id, status or "unknown", + ) + return None + if requested_qty and filled_qty < requested_qty: + logger.warning( + "entry protection: %s partially filled %.4f/%.4f — stop sized to " + "the ACTUAL fill", symbol, filled_qty, requested_qty, + ) + try: + stop_order = self._submit_stop_limit_order( + symbol=symbol, qty=filled_qty, stop_price=stop_price, + limit_price=stop_price * (1 - self.STOP_LIMIT_BUFFER_PCT), + ) + logger.info( + "entry protection: GTC stop-limit placed for %s qty=%.4f @ stop $%.2f", + symbol, filled_qty, stop_price, + ) + return stop_order + except Exception as exc: # noqa: BLE001 + logger.error( + "entry protection FAILED for %s (%.4f shares held, stop $%.2f): %s " + "— position is UNPROTECTED; next session's coverage reconcile " + "must repair it", symbol, filled_qty, stop_price, exc, + ) + return None + def close_position(self, symbol: str) -> dict: order = self.client.close_position(symbol) logger.info("Closed position: %s", symbol) diff --git a/src/pipeline.py b/src/pipeline.py index 028e58a0..d59e9484 100644 --- a/src/pipeline.py +++ b/src/pipeline.py @@ -834,15 +834,14 @@ def _reconcile_stop_coverage(self) -> list[dict]: continue covered = sum(float(s.get("qty", 0) or 0) for s in (specs or [])) if covered + 1e-6 < qty: - gaps.append({ - "symbol": symbol, "held_qty": qty, "covered_qty": covered, - }) + gap = {"symbol": symbol, "held_qty": qty, "covered_qty": covered} logger.warning( "STOP-COVERAGE GAP: %s held=%.4f but only %.4f covered by " "open protective stops — (partially) unprotected with no WAL " - "recovery row. Manual review / re-protect needed.", - symbol, qty, covered, + "recovery row.", symbol, qty, covered, ) + gap["repaired"] = self._repair_stop_coverage(symbol, qty - covered) + gaps.append(gap) if longs_checked and not gaps: logger.info( "Stop-coverage reconcile: all %d long position(s) adequately " @@ -850,6 +849,78 @@ def _reconcile_stop_coverage(self) -> list[dict]: ) return gaps + def _repair_stop_coverage(self, symbol: str, uncovered_qty: float) -> bool: + """Best-effort: re-place a GTC protective stop on an uncovered long + using the stop level recorded on its last BUY. Returns True when a + stop was actually submitted. + + Why this is now safe to auto-repair (it deliberately wasn't before): + the old objection was "the original protective level is unknown for a + position with no live stop, so picking one is a policy decision". It + isn't unknown — the BUY row carries the `stop_loss` the PM/RM agreed + and the constructor sized against. Repairing to THAT level restores the + reviewed intent rather than inventing a new one. + + This is the belt for the 2026-07-16 CRITICAL (BUY-attached OTO stops + inherited a DAY tif and expired at the close, leaving positions naked + overnight) — both for any position that bug left uncovered, and for a + crash between an entry fill and `place_entry_protection`. + + Guards: never place a stop at/above the current price (that would + instantly fire and turn a repair into a market-order exit — a decision + for the reviewer, not for a janitor), and never invent a level when the + BUY row has none. + """ + if uncovered_qty <= 0: + return False + try: + buy = self.db.get_symbol_last_buy(symbol) or {} + except Exception as exc: # noqa: BLE001 + logger.warning("coverage repair: last-BUY lookup failed for %s: %s", symbol, exc) + return False + try: + stop_price = float(buy.get("stop_loss") or 0) + except (TypeError, ValueError): + stop_price = 0.0 + if stop_price <= 0: + logger.warning( + "coverage repair: %s has no recorded BUY stop_loss — leaving the " + "gap flagged for manual review", symbol, + ) + return False + try: + price = self.broker.get_latest_price(symbol) + except Exception as exc: # noqa: BLE001 + logger.warning("coverage repair: price lookup failed for %s: %s", symbol, exc) + return False + if not (isinstance(price, (int, float)) and price > 0 and math.isfinite(price)): + return False + if stop_price >= price: + logger.warning( + "coverage repair: %s recorded stop $%.2f is at/above the live " + "price $%.2f — a repair would fire immediately. Leaving the gap " + "flagged; the reviewer owns this exit decision.", + symbol, stop_price, price, + ) + return False + try: + self.broker._submit_stop_limit_order( + symbol=symbol, qty=uncovered_qty, stop_price=stop_price, + limit_price=stop_price * (1 - self.broker.STOP_LIMIT_BUFFER_PCT), + ) + except Exception as exc: # noqa: BLE001 + logger.error( + "coverage repair FAILED for %s (%.4f uncovered, stop $%.2f): %s", + symbol, uncovered_qty, stop_price, exc, + ) + return False + logger.warning( + "COVERAGE REPAIRED: %s — placed GTC stop-limit for %.4f uncovered " + "share(s) at the recorded BUY stop $%.2f", + symbol, uncovered_qty, stop_price, + ) + return True + def _submit_protected_sell( self, *, diff --git a/src/pipeline_stages.py b/src/pipeline_stages.py index 9f6e5f6f..73025f08 100644 --- a/src/pipeline_stages.py +++ b/src/pipeline_stages.py @@ -927,6 +927,7 @@ def run(self, ctx: RunContext) -> list[dict]: total_value = ctx.total_value available_cash = cash + pending_entry_stops: list[dict] = [] for decision in buy_decisions: if decision.action != "BUY": continue @@ -1136,8 +1137,37 @@ def run(self, ctx: RunContext) -> list[dict]: "Executed: buy %d %s @ %s $%.2f", qty, decision.symbol, order_type, executed_price, ) + # The entry still owes a protective stop: it is placed as a + # separate GTC order AFTER the fill, because an OTO leg would + # inherit the parent's DAY tif and be expired by the broker at + # 16:00 ET the same day (2026-07-16 audit — positions were + # naked every night). Deferred until all BUYs are submitted so + # the fill waits don't serialize the submission burst. + if isinstance(order, dict) and order.get("pending_stop_price"): + pending_entry_stops.append({ + "symbol": decision.symbol, + "order_id": order.get("id"), + "stop_price": order["pending_stop_price"], + "qty": qty, + }) except Exception as e: logger.error("Order failed for %s %s: %s", decision.action, decision.symbol, e) + # Protect every filled entry (GTC stop-limit keyed to the ACTUAL fill). + for spec in pending_entry_stops: + if not spec.get("order_id"): + continue + try: + pipeline.broker.place_entry_protection( + symbol=spec["symbol"], order_id=spec["order_id"], + stop_price=spec["stop_price"], requested_qty=spec["qty"], + ) + except Exception as e: # noqa: BLE001 — never abort the session here + logger.error( + "entry protection raised for %s: %s — position may be " + "unprotected until the next coverage reconcile", + spec["symbol"], e, + ) + ctx.orders = orders return orders diff --git a/tests/test_broker.py b/tests/test_broker.py index e7bc3c3e..b4faf52e 100644 --- a/tests/test_broker.py +++ b/tests/test_broker.py @@ -1,5 +1,6 @@ import pytest from unittest.mock import patch, MagicMock, PropertyMock +from alpaca.trading.enums import TimeInForce from src.execution.broker import AlpacaBroker @@ -505,7 +506,7 @@ def test_submit_order_quantizes_sub_penny_limit_price(mock_tc_cls): mock_tc_cls.return_value = mock_client broker = AlpacaBroker(api_key="test", secret_key="test", paper=True) - broker.submit_order( + result = broker.submit_order( symbol="UPS", qty=5, side="buy", limit_price=106.515, stop_loss_price=98.127, # stop too — same tick rule applies @@ -513,8 +514,11 @@ def test_submit_order_quantizes_sub_penny_limit_price(mock_tc_cls): req = mock_client.submit_order.call_args[0][0] assert isinstance(req, LimitOrderRequest) assert float(req.limit_price) == 106.52 # quantized to nearest cent - # The OTO stop_loss leg carries the stop_price on its own sub-object. - assert float(req.stop_loss.stop_price) == 98.13 + # 2026-07-16 audit: the entry carries NO OTO stop leg any more (the leg + # would inherit the parent's DAY tif and be expired at the close). The + # quantized stop rides back on the result for post-fill GTC placement. + assert getattr(req, "stop_loss", None) is None + assert float(result["pending_stop_price"]) == 98.13 @patch("src.execution.broker.TradingClient") @@ -1530,3 +1534,93 @@ def test_get_recent_daily_closes_swallows_errors(mock_tc_cls): mock_tc_cls.return_value = mock_client broker = AlpacaBroker(api_key="k", secret_key="s", paper=True) assert broker.get_recent_daily_closes() == [] # best-effort, never raises + + +# ============================================================================ +# 2026-07-16 audit, CRITICAL: BUY-attached protective stops were OTO legs that +# inherited the parent's DAY time_in_force, so Alpaca expired them at 16:00 ET +# the same session — every position bought in the morning and not later given a +# midday/close TRAIL_STOP sat NAKED overnight (VST 06-26: stop $158.75 gone by +# the close, exited 07-01 @ $152.77 for ~$185 more loss than the stop capped). +# alpaca-py's StopLossRequest has no TIF field of its own, so the only fix is +# to place the stop as a separate GTC order after the entry fills. +# ============================================================================ + +@patch("src.execution.broker.TradingClient") +def test_buy_entry_carries_no_oto_leg(mock_tc_cls): + """The entry must be a plain DAY limit — an unfilled entry must still die + at the close, and the stop must NOT ride on the parent's tif.""" + from alpaca.trading.requests import LimitOrderRequest + + mock_client = MagicMock() + order = MagicMock(id="e1", status="accepted", symbol="NVDA") + mock_client.submit_order.return_value = order + mock_tc_cls.return_value = mock_client + + broker = AlpacaBroker(api_key="test", secret_key="test", paper=True) + result = broker.submit_order(symbol="NVDA", qty=10, side="buy", + limit_price=100.0, stop_loss_price=90.0) + + req = mock_client.submit_order.call_args[0][0] + assert isinstance(req, LimitOrderRequest) + assert getattr(req, "order_class", None) is None + assert getattr(req, "stop_loss", None) is None + assert req.time_in_force == TimeInForce.DAY # entry still dies at the close + assert result["pending_stop_price"] == 90.0 # caller owes the stop + + +@patch("src.execution.broker.TradingClient") +def test_place_entry_protection_uses_gtc_and_actual_fill_qty(mock_tc_cls): + """The protective stop is GTC (survives the close) and is sized to the + ACTUAL fill — the old OTO leg was sized to the REQUESTED qty, so a partial + entry fill left a stop covering shares we never owned.""" + from alpaca.trading.requests import StopLimitOrderRequest + + mock_client = MagicMock() + stop_order = MagicMock(id="s1", status="new", symbol="NVDA") + mock_client.submit_order.return_value = stop_order + mock_tc_cls.return_value = mock_client + + broker = AlpacaBroker(api_key="test", secret_key="test", paper=True) + broker.wait_for_order_terminal = MagicMock(return_value="filled") + broker.get_order_fill_info = MagicMock(return_value={ + "status": "filled", "filled_qty": 7.0, "filled_avg_price": 100.0, + }) + + out = broker.place_entry_protection( + symbol="NVDA", order_id="e1", stop_price=90.0, requested_qty=10, + ) + + assert out is not None + req = mock_client.submit_order.call_args[0][0] + assert isinstance(req, StopLimitOrderRequest) + assert req.time_in_force == TimeInForce.GTC # THE fix — survives 16:00 ET + assert float(req.qty) == 7.0 # actual fill, not the 10 requested + assert float(req.stop_price) == 90.0 + assert float(req.limit_price) == 87.3 # 3% buffer below the stop + + +@patch("src.execution.broker.TradingClient") +def test_place_entry_protection_no_fill_places_nothing(mock_tc_cls): + mock_client = MagicMock() + mock_tc_cls.return_value = mock_client + broker = AlpacaBroker(api_key="test", secret_key="test", paper=True) + broker.wait_for_order_terminal = MagicMock(return_value="canceled") + broker.get_order_fill_info = MagicMock(return_value={"filled_qty": 0.0}) + + assert broker.place_entry_protection("NVDA", "e1", 90.0) is None + mock_client.submit_order.assert_not_called() + + +@patch("src.execution.broker.TradingClient") +def test_place_entry_protection_swallows_stop_submit_failure(mock_tc_cls): + """A failed stop must not abort the session — it logs ERROR and leaves the + gap for the next coverage reconcile to repair.""" + mock_client = MagicMock() + mock_client.submit_order.side_effect = RuntimeError("alpaca 500") + mock_tc_cls.return_value = mock_client + broker = AlpacaBroker(api_key="test", secret_key="test", paper=True) + broker.wait_for_order_terminal = MagicMock(return_value="filled") + broker.get_order_fill_info = MagicMock(return_value={"filled_qty": 10.0}) + + assert broker.place_entry_protection("NVDA", "e1", 90.0) is None # no raise diff --git a/tests/test_stop_coverage_repair.py b/tests/test_stop_coverage_repair.py new file mode 100644 index 00000000..aecd0873 --- /dev/null +++ b/tests/test_stop_coverage_repair.py @@ -0,0 +1,80 @@ +"""2026-07-16 audit CRITICAL — belt: naked longs get their stop re-placed. + +The BUY-attached OTO stop inherited the parent's DAY tif and was expired by +the broker at 16:00 ET, so positions bought in the morning sat unprotected +overnight. The primary fix places a GTC stop post-fill; this reconciler is the +belt that (a) repairs anything the old bug left naked and (b) covers a crash +between an entry fill and the stop placement. + +Repair uses the stop level RECORDED ON THE LAST BUY — the reviewed intent, not +an invented one — and refuses to place a stop at/above the live price (that +would fire instantly and turn a janitor into an exit decision). +""" +from unittest.mock import MagicMock + +from src.pipeline import TradingPipeline + + +def _pipeline(held_qty=31.0, covered=0.0, buy_stop=158.75, price=165.0): + p = TradingPipeline.__new__(TradingPipeline) + p.broker = MagicMock() + p.broker.get_positions.return_value = [ + MagicMock(symbol="VST", qty=held_qty), + ] + p.broker.snapshot_protective_stops.return_value = ( + True, ([{"qty": covered, "stop_price": 158.0}] if covered else []), + ) + p.broker.get_latest_price.return_value = price + p.broker.STOP_LIMIT_BUFFER_PCT = 0.03 + p.db = MagicMock() + p.db.get_pending_protection_restores.return_value = [] + p.db.get_symbol_last_buy.return_value = {"stop_loss": buy_stop} + p.cash_sweeper = None + return p + + +def test_naked_long_is_repaired_from_the_recorded_buy_stop(): + p = _pipeline() + gaps = p._reconcile_stop_coverage() + assert len(gaps) == 1 and gaps[0]["repaired"] is True + kwargs = p.broker._submit_stop_limit_order.call_args.kwargs + assert kwargs["symbol"] == "VST" + assert kwargs["qty"] == 31.0 # the whole uncovered position + assert kwargs["stop_price"] == 158.75 # the level PM/RM actually approved + assert abs(kwargs["limit_price"] - 158.75 * 0.97) < 0.01 + + +def test_partial_coverage_repairs_only_the_uncovered_shares(): + p = _pipeline(held_qty=31.0, covered=20.0) + gaps = p._reconcile_stop_coverage() + assert gaps[0]["repaired"] is True + assert p.broker._submit_stop_limit_order.call_args.kwargs["qty"] == 11.0 + + +def test_repair_refuses_a_stop_at_or_above_the_live_price(): + """Recorded stop $158.75 but the stock is now $150 — placing it would fire + instantly. That's an exit decision; flag, don't act.""" + p = _pipeline(price=150.0) + gaps = p._reconcile_stop_coverage() + assert gaps[0]["repaired"] is False + p.broker._submit_stop_limit_order.assert_not_called() + + +def test_repair_skipped_when_the_buy_row_has_no_stop(): + p = _pipeline(buy_stop=0.0) + gaps = p._reconcile_stop_coverage() + assert gaps[0]["repaired"] is False + p.broker._submit_stop_limit_order.assert_not_called() + + +def test_repair_failure_still_reports_the_gap(): + p = _pipeline() + p.broker._submit_stop_limit_order.side_effect = RuntimeError("alpaca 500") + gaps = p._reconcile_stop_coverage() + assert len(gaps) == 1 and gaps[0]["repaired"] is False # no raise + + +def test_covered_long_needs_no_repair(): + p = _pipeline(covered=31.0) + assert p._reconcile_stop_coverage() == [] + p.broker._submit_stop_limit_order.assert_not_called() From 0b3b06d2926e0b897898b28299d4b448e103454a Mon Sep 17 00:00:00 2001 From: yebof Date: Thu, 16 Jul 2026 19:18:32 +0800 Subject: [PATCH 2/4] fix(audit): 12 more confirmed defects from the full-codebase audit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All confirmed by an independent skeptic pass; each has a regression test. RISK ENGINE - NaN market_value silently DISABLED max_total_position_pct + max_sector_pct for the whole session (NaN comparisons are all False, so `total_pct > cap` evaluated False). Now blocks with a synthetic hard violation, mirroring the total_value guard — a broken snapshot is the day the caps matter most. - correlation_cluster omitted the BUY symbol's OWN existing position: `highly_correlated_peers` (correctly) excludes the symbol itself, so an ADD to the biggest name in a cluster counted only the ADD. 40k NVDA + 10k AVGO + 5k add scored 15%, not 55%. CONSTRUCTOR - allocation_pct was a GROSS weight delta but every consumer (rules.py, ExecutionStage) spends it as RAW notional: a 6% SQQQ (3x) target deployed $6k = 18% gross, and the next session saw 18 vs target 6 and SOLD 67% of the hedge PM wanted held, repeating until raw ~2%. Convert once at the source; no-op at multiplier 1.0. (SELL is a ratio of gross weights — the multiplier cancels — so it was already right.) - target_weight_pct=0 means CLOSE, but the min_trade_weight_delta churn filter turned a small dreg's explicit close into a HOLD forever. - The stop was validated unrounded and shipped rounded, so a stop that rounds UP to exactly the entry passed the `stop < entry` check → risk_per_share=0 and a stop that fires on the first tick down. BROKER - get_session_close() ALWAYS returned None against the real SDK: Calendar.close is a naive datetime, not a time, so datetime.combine() raised TypeError every call → the early-close guard was dead code and midday/close ran against a shut market on half-days. The test that should have caught it built a MagicMock with a `time`; it now constructs the real Calendar model. - _get_sector() returned "Unknown" for EVERY ETF (yfinance .info has no sector for ETFs), which both skipped max_sector_pct entirely on an ETF BUY and made a held XLV contribute $0 to Healthcare for an LLY BUY. Deterministic _ETF_SECTORS table, consulted before the network fetch. - cancel_snapshotted_stops discarded _restore_stop_orders' failed_specs, so a rollback that itself failed left shrunk coverage reported as a bare False. PIPELINE - finalize persisted the POST-sell residual into the position_qty_before_sell column; the drain recomputes `pre - fill` from the same order, so the fill was subtracted twice — for an exact fill the residual hit 0, took the "full exit, nothing to protect" early return, reported success and DELETED the WAL row. The residual position stayed naked forever. - Ex-div stop adjustment could NEVER fire for a Monday ex-div: it compared against calendar `today + 1 day`, which for a Mon-Fri session is never a Monday (Friday computes Saturday). Now uses the next TRADING day. - _force_delever booked projected proceeds only AFTER insert_trade succeeded, so a DB hiccup on a live SELL made the loop force-sell the NEXT position to cover a deficit already covered — liquidating holdings over a bookkeeping failure. DATA - macro sector_guidance was never persisted by save_last_state, so every macro_sector_stance / macro_sector_tailwind was PERMANENTLY "unknown" — the evening thesis-health step and every missed-opportunity snapshot rendered "Macro sector stance: unknown" nightly while macro was emitting OW/UW calls. Three stacked breaks (not persisted; reader wants a dict, model carries a list; overweight/underweight vs bullish/bearish) — normalized on write. The round-trip test had PINNED the bug by asserting the key was dropped; its real intent (keep the snapshot tiny) is honoured by storing the map without the `reason` prose. 1336 tests green. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01PQkESoSTYx2bCy7WYnTPXR --- src/data/macro_store.py | 49 +++++ src/execution/broker.py | 79 +++++++- src/pipeline.py | 77 ++++++-- src/portfolio_constructor.py | 36 +++- src/risk/rules.py | 32 ++- tests/test_audit_fixes_2026_07_16.py | 278 +++++++++++++++++++++++++++ tests/test_broker.py | 39 +++- tests/test_macro_store.py | 13 +- 8 files changed, 566 insertions(+), 37 deletions(-) create mode 100644 tests/test_audit_fixes_2026_07_16.py diff --git a/src/data/macro_store.py b/src/data/macro_store.py index 1e4aa795..14de08a7 100644 --- a/src/data/macro_store.py +++ b/src/data/macro_store.py @@ -16,6 +16,42 @@ def _atomic_write(path: Path, data: str) -> None: os.replace(str(tmp), str(path)) +# MacroAnalysis.sector_guidance is a LIST of {sector, stance, reason} where +# stance ∈ overweight|neutral|underweight. Every consumer of the persisted +# state (`_missed_ops_macro_sector_map`, thesis-health) wants a DICT keyed by +# sector with bullish|neutral|bearish values. Convert once, on write. +_STANCE_TO_DIRECTION = { + "overweight": "bullish", + "neutral": "neutral", + "underweight": "bearish", +} + + +def _normalize_sector_guidance(raw) -> dict[str, str]: + """[{sector, stance, reason}, ...] → {sector: bullish|neutral|bearish}. + + Tolerates the already-normalized dict shape (idempotent) and drops + anything unrecognized. Never raises — a malformed guidance block must + not take down the macro save. + """ + out: dict[str, str] = {} + if isinstance(raw, dict): + for sector, direction in raw.items(): + if isinstance(direction, str) and direction in ("bullish", "neutral", "bearish"): + out[str(sector)] = direction + return out + if not isinstance(raw, list): + return out + for item in raw: + if not isinstance(item, dict): + continue + sector = item.get("sector") + direction = _STANCE_TO_DIRECTION.get(str(item.get("stance") or "").lower()) + if sector and direction: + out[str(sector)] = direction + return out + + class MacroStore: def __init__(self, data_dir: str = "data/macro"): self.data_dir = Path(data_dir) @@ -47,6 +83,19 @@ def save_last_state(self, analysis: dict) -> None: "equity_outlook": analysis.get("equity_outlook"), "summary": analysis.get("summary"), "position_guidance": analysis.get("position_guidance"), + # 2026-07-16 audit: this key was never persisted, so EVERY + # downstream macro_sector_stance / macro_sector_tailwind was + # permanently "unknown" — the evening thesis-health step and every + # missed-opportunity snapshot rendered "Macro sector stance: + # unknown" for every position, every night, while macro was in fact + # emitting OW/UW calls. Stored pre-normalized (see + # _normalize_sector_guidance): the readers want {sector: direction} + # and the model carries a list of {sector, stance, reason}. The + # bulky `reason` strings stay out — this file's contract is "keep + # it small"; reasons live in agent_logs. + "sector_guidance": _normalize_sector_guidance( + analysis.get("sector_guidance") + ), } _atomic_write(self.last_state_path, json.dumps(snapshot, indent=2, ensure_ascii=False)) logger.info("Saved macro last state → %s (regime=%s)", diff --git a/src/execution/broker.py b/src/execution/broker.py index 2241e151..9aa2a0ca 100644 --- a/src/execution/broker.py +++ b/src/execution/broker.py @@ -19,6 +19,34 @@ # Index ETFs that have no single sector — bucket them as "Broad". _INDEX_ETFS = {"SPY", "QQQ", "IWM", "DIA", "VTI", "VOO", "IVV"} +# Sector / thematic ETFs → their canonical sector bucket. +# +# WHY (2026-07-16 audit): yfinance's `.info` carries no `sector` key for ETFs, +# so _get_sector fell through to "Unknown" for every one of them. Two silent +# failures followed: (1) `max_sector_pct` is gated on `new_sector != "Unknown"` +# (risk/rules.py), so a BUY of XLV/SMH/... skipped the sector cap ENTIRELY; +# (2) a held ETF carries sector="Unknown", so it contributed $0 to the sector +# bucket of a same-sector single name — a book that is 30% XLV would let an +# LLY BUY through as if Healthcare exposure were zero. Both directions of the +# cap were dead for these symbols despite the universe being ~20% ETFs. +# +# Deterministic table, consulted BEFORE the network fetch: an ETF's sector is +# a fact about the product, not something to rediscover per process. +_ETF_SECTORS = { + # SPDR sector suite + "XLF": "Financial Services", "XLE": "Energy", "XLV": "Healthcare", + "XLI": "Industrials", "XLP": "Consumer Defensive", "XLY": "Consumer Cyclical", + "XLU": "Utilities", "XLRE": "Real Estate", "XLB": "Basic Materials", + "XLK": "Technology", "XLC": "Communication Services", + # Semiconductor / AI thematics + "SMH": "Technology", "SOXX": "Technology", "DRAM": "Technology", + "CHPX": "Technology", + # Inverse / leveraged index ETFs track a BROAD index — they have no sector + # of their own. (Their leverage is handled separately by the signed/gross + # multipliers in risk/rules.py.) + "SH": "Broad", "SDS": "Broad", "PSQ": "Broad", "SQQQ": "Broad", +} + # Default HTTP timeout for ALL Alpaca SDK calls (connect, read). # Without this, a stalled TCP connection to the broker can hang the process # for hours under launchd — observed 2026-04-17 when the evening job sat for @@ -123,6 +151,14 @@ def _get_sector(symbol: str) -> str: with _sector_lock: _sector_cache[symbol] = "Broad" return "Broad" + # Sector/thematic ETFs: yfinance .info has no `sector` for ETFs, so + # without this table they resolve to "Unknown" and silently switch the + # sector cap OFF (see _ETF_SECTORS). Deterministic, offline, before the fetch. + etf_sector = _ETF_SECTORS.get(symbol.upper()) + if etf_sector is not None: + with _sector_lock: + _sector_cache[symbol] = etf_sector + return etf_sector def _fetch(): try: @@ -389,10 +425,21 @@ def get_session_close(self, on_date: date | None = None): if entry_date is None or entry_close is None: return None try: + # alpaca-py's Calendar.close is a full naive DATETIME (already + # carrying the session date + ET wall clock), NOT a time. The old + # code called datetime.combine(date, datetime), which ALWAYS + # raised TypeError → logged → returned None → the early-close + # guard never fired and midday/close ran against a shut market on + # half-days, submitting orders that can only be rejected + # (2026-07-16 audit: dead code since it was written; the test that + # was supposed to cover it used a MagicMock with a `time`). + # Keep the `time` branch for the older SDK shape. + if isinstance(entry_close, _dt): + return entry_close.replace(tzinfo=ET) return _dt.combine(entry_date, entry_close).replace(tzinfo=ET) except Exception as exc: logger.warning( - "get_session_close: failed to combine date=%s close=%s: %s", + "get_session_close: failed to resolve date=%s close=%s: %s", entry_date, entry_close, exc, ) return None @@ -672,13 +719,31 @@ def cancel_snapshotted_stops( ) failed += 1 if failed > 0: + restored = 0 + rollback_failed: list[dict] = [] if cancelled: - self._restore_stop_orders(symbol, cancelled) - logger.warning( - "cancel_snapshotted_stops: %d/%d cancel(s) failed for %s " - "(rolled back %d that succeeded); SELL won't proceed", - failed, len(specs), symbol, len(cancelled), - ) + # _restore_stop_orders returns (restored_count, failed_specs) + # — the old code DISCARDED it, so a rollback that itself + # failed left the position with shrunk coverage and reported + # only a bare False. The SELL is skipped either way, but the + # operator (and the next session's coverage reconcile, which + # now auto-repairs) must be able to see it (2026-07-16 audit). + restored, rollback_failed = self._restore_stop_orders(symbol, cancelled) + if rollback_failed: + logger.error( + "cancel_snapshotted_stops: %d/%d cancel(s) failed for %s AND " + "the rollback could not restore %d of %d cancelled stop(s) — " + "%s is now UNDER-PROTECTED; next session's coverage reconcile " + "must repair it. SELL won't proceed.", + failed, len(specs), symbol, len(rollback_failed), len(cancelled), + symbol, + ) + else: + logger.warning( + "cancel_snapshotted_stops: %d/%d cancel(s) failed for %s " + "(rolled back %d/%d that succeeded); SELL won't proceed", + failed, len(specs), symbol, restored, len(cancelled), + ) return False if cancelled: logger.info( diff --git a/src/pipeline.py b/src/pipeline.py index d59e9484..2aa123ab 100644 --- a/src/pipeline.py +++ b/src/pipeline.py @@ -1327,9 +1327,21 @@ def _finalize_protection_after_sell_core( # Codex r9 #1: previously this just returned False without # persisting, and the SELL-path callers ignored that bool — # the recovery intent was silently lost. + # + # Persist the PRE-sell qty, not `actual_residual` (2026-07-16 + # audit): the drain replays this row through the same finalize + # core, which recomputes `position_qty_before_sell - fill_qty` + # from the SAME order. Passing the post-sell residual made the + # replay subtract the fill twice — for a SELL that filled exactly + # what it asked for, the recomputed residual hit 0, took the + # "full exit — nothing to re-protect" early return, reported + # success, and DELETED the row. Net effect: the residual position + # stayed naked forever and the recovery intent was destroyed. + # The drain's downward clip against the live broker position keeps + # this correct even if a concurrent SELL took shares meanwhile. if not from_drain: self._persist_orphaned_protection_restore( - order_id, symbol, actual_residual, cancelled_specs, + order_id, symbol, position_qty_before_sell, cancelled_specs, wal_row_id=wal_row_id, ) return False, list(cancelled_specs) @@ -2187,7 +2199,24 @@ def _handle_ex_dividends(self, positions, run_id: str) -> list[dict]: """ from datetime import timedelta as _td orders: list[dict] = [] - tomorrow = et_today() + _td(days=1) + today = et_today() + # NEXT TRADING day, not calendar tomorrow (2026-07-16 audit): sessions + # only run Mon-Fri, so `today + 1 day` can never BE a Monday — every + # Monday ex-div silently went unadjusted, and Friday's sessions (the + # last chance to act) computed Saturday. Same hole for any ex-div the + # day after a holiday. Fall back to calendar+1 if the calendar lookup + # fails — degrading to today's behavior beats crashing the session. + next_trading_day = today + _td(days=1) + for _ in range(7): + try: + if self.broker.is_trading_day(next_trading_day): + break + except Exception as e: # noqa: BLE001 + logger.warning("ex-div: is_trading_day failed (%s) — falling back " + "to calendar+1", e) + next_trading_day = today + _td(days=1) + break + next_trading_day += _td(days=1) for p in positions: if p.qty <= 0: @@ -2215,11 +2244,13 @@ def _handle_ex_dividends(self, positions, run_id: str) -> list[dict]: continue if not div: continue - if div.get("date") != tomorrow: - # Only act the day BEFORE ex-div. On ex-div day itself, the - # gap has already happened at open — stop adjustment is too - # late, and "day after" adjustment is wrong (stock is - # re-pricing back to normal vol). + div_date = div.get("date") + if not (div_date and today < div_date <= next_trading_day): + # Only act on the session BEFORE ex-div. On ex-div day itself + # the gap has already happened at open — adjustment is too + # late — and "day after" is wrong (the stock is re-pricing + # back to normal vol). The window is (today, next_trading_day] + # so a Monday ex-div is caught by Friday's sessions. continue amount = div.get("amount") or 0 if amount <= 0: @@ -5117,6 +5148,23 @@ def _tier(p): order, prot = sale pending_protections.append(prot) try: + # Count the proceeds BEFORE the ledger write: the SELL is + # already live at the broker, so its cash is coming whether or + # not we manage to record it. Booking it only after a + # successful insert_trade meant a DB hiccup left + # projected_proceeds short, and the loop force-sold the NEXT + # position to cover a deficit the in-flight order had already + # covered — liquidating real holdings over a bookkeeping + # failure (2026-07-16 audit). + # Conservative estimate: market × 0.99 (matches our limit). + projected_proceeds += p.market_value * 0.99 + orders.append(order) + logger.info( + "FORCE DE-LEVER SELL %s qty=%s @ limit=$%.2f " + "(unrealized_pnl=$%.2f, mkt_value=$%.2f)", + p.symbol, self._format_qty(qty), sell_limit, + p.unrealized_pnl, p.market_value, + ) self.db.insert_trade( symbol=p.symbol, action="FORCE_DELEVER", qty=qty, price=p.current_price, @@ -5129,17 +5177,12 @@ def _tier(p): broker_order_id=order.get("id"), fill_status="submitted", ) - orders.append(order) - # Conservative estimate: market × 0.99 (matches our limit). - projected_proceeds += p.market_value * 0.99 - logger.info( - "FORCE DE-LEVER SELL %s qty=%s @ limit=$%.2f " - "(unrealized_pnl=$%.2f, mkt_value=$%.2f)", - p.symbol, self._format_qty(qty), sell_limit, - p.unrealized_pnl, p.market_value, - ) except Exception as e: - logger.error("FORCE DE-LEVER SELL %s failed: %s", p.symbol, e) + logger.error( + "FORCE DE-LEVER SELL %s failed: %s — the order may still be " + "live at the broker; its proceeds are already counted so the " + "sweep will not over-liquidate", p.symbol, e, + ) # Block the session until fills land so the post-refresh cash is real. # Then finalize protection — if any limit didn't fill, restore the diff --git a/src/portfolio_constructor.py b/src/portfolio_constructor.py index 2f8885d5..6ce9650e 100644 --- a/src/portfolio_constructor.py +++ b/src/portfolio_constructor.py @@ -84,7 +84,14 @@ def construct_orders( target_pct = target.target_weight_pct delta_pct = target_pct - current_pct - if abs(delta_pct) < self.cfg.min_trade_weight_delta: + # target_weight_pct == 0 is PM saying "CLOSE this position", not + # "rebalance toward ~0". The churn filter must not swallow it: a + # 0.4%-weight dreg with an explicit close target was silently + # converted into a HOLD, so a position PM had decided to exit sat + # in the book indefinitely (2026-07-16 audit). Anything held with + # target 0 goes to the SELL builder, which emits a full exit. + closing = (target_pct == 0 and current_pct > 0) + if not closing and abs(delta_pct) < self.cfg.min_trade_weight_delta: # No action — emit HOLD for audit continuity so PM's intent # to keep this position at its current level is recorded. if current_pct > 0: @@ -240,7 +247,15 @@ def _build_buy( # Resolve stop — priority: target's suggested stop, then TA's stop, # then ATR-based default, then fallback % of entry. + # Round FIRST, then validate: the TradeDecision below ships + # round(stop_loss, 2), so validating the unrounded value let a stop + # that rounds UP to exactly the entry price through the + # `stop_loss < entry_price` check (e.g. entry $10.00, stop $9.999 → + # ships $10.00 == entry → risk_per_share = 0, and a stop at the entry + # fires on the first tick down). 2026-07-16 audit. stop_loss = self._resolve_stop(target, analysis, entry_price) + if stop_loss is not None: + stop_loss = round(stop_loss, 2) if stop_loss is None or stop_loss <= 0 or stop_loss >= entry_price: logger.warning( "Constructor: BUY %s rejected — no valid stop below entry " @@ -257,9 +272,24 @@ def _build_buy( stop_gap_pct = (entry_price - stop_loss) / entry_price take_profit = round(entry_price * (1 + 2 * stop_gap_pct), 2) - allocation_pct = target_pct - current_pct + # `target_pct` and `current_pct` are GROSS-leverage weights (see + # _current_weights), but every consumer of `allocation_pct` spends it + # as RAW notional: risk/rules.py does `total_value * alloc/100` and + # THEN applies the gross multiplier itself, and ExecutionStage sizes + # `qty = total_value * alloc/100 / price`. Emitting the gross delta + # raw therefore over-deployed leveraged/inverse ETFs by their + # multiplier (2026-07-16 audit: a PM target of 6% gross on SQQQ (3x) + # deployed $6k raw = 18% gross of a $100k book — and the NEXT session + # saw current_pct=18 vs target 6 and emitted SELL 67% of the hedge PM + # wanted held, repeating until raw ≈ 2%). Convert once, here, so the + # delta and every downstream consumer speak the same units. No-op for + # the ~99% of the universe with multiplier 1.0. + from src.risk.rules import _gross_multiplier + allocation_pct = (target_pct - current_pct) / _gross_multiplier(target.symbol) # Pull in vol-adj sizing in a uniform way: ensure qty (computed # downstream) doesn't put more than risk_budget_pct of equity at risk. + # NOTE: alloc_cap_by_risk below is computed in RAW notional terms, so + # this conversion must happen BEFORE the comparison. risk_per_share = entry_price - stop_loss risk_dollars_allowed = total_value * self.cfg.risk_budget_pct / 100 # qty_by_risk = risk_dollars_allowed / risk_per_share @@ -294,7 +324,7 @@ def _build_buy( symbol=target.symbol, allocation_pct=allocation_pct, entry_price=entry_price, - stop_loss=round(stop_loss, 2), + stop_loss=stop_loss, # already rounded + validated above take_profit=take_profit, reasoning=reasoning[:500], ) diff --git a/src/risk/rules.py b/src/risk/rules.py index 189d536a..b5f46a79 100644 --- a/src/risk/rules.py +++ b/src/risk/rules.py @@ -92,6 +92,27 @@ def check(self, decision: TradeDecision, positions: list[Position], ) baseline = total_value + # A single non-finite position market_value poisons every sum below. + # NaN comparisons are all False, so `sector_pct > cap` and + # `total_pct > cap` silently evaluate False — the exposure and sector + # caps switch OFF for the whole session on exactly the broken-snapshot + # day they matter most (2026-07-16 audit; Alpaca has been observed to + # return NaN market_value during market-open glitches). Block instead, + # mirroring the total_value guard above: no risk-check, no BUY. + bad_mv = [p.symbol for p in positions if not math.isfinite(p.market_value)] + if bad_mv: + return [RiskViolation( + rule="max_total_position_pct", # in HARD_BLOCK_RULES + message=( + f"non-finite market_value for {', '.join(sorted(bad_mv))} — " + f"exposure / sector caps cannot be computed; refusing to " + f"risk-check BUY for {decision.symbol}; blocking until the " + f"next clean snapshot" + ), + value=0.0, + limit=0.0, + )] + violations = [] signed_mul = _effective_multiplier(decision.symbol) # net direction gross_mul = _gross_multiplier(decision.symbol) # size magnitude @@ -170,9 +191,18 @@ def check(self, decision: TradeDecision, positions: list[Position], # 3x notional, even though its directional sign cancels for # NET exposure (#2). Pre-fix this rule treated SQQQ as 1x # which silently under-counted cluster concentration. + # The cluster must include the BUY symbol's OWN existing + # position, not just its peers: `highly_correlated_peers` + # (correctly) excludes the symbol itself, so an ADD to the + # largest member of a cluster counted only the ADD's notional + # and none of the stack already held — the concentration this + # rule exists to catch was invisible exactly when it was worst + # (2026-07-16 audit). A symbol is trivially correlated 1.0 + # with itself, so it belongs in its own cluster total. + cluster_symbols = set(peers) | {decision.symbol} peer_value = sum( p.market_value * _gross_multiplier(p.symbol) - for p in positions if p.symbol in peers + for p in positions if p.symbol in cluster_symbols ) cluster_pct = (peer_value + gross_new) / total_value * 100 if cluster_pct > max_correlated_cluster_pct: diff --git a/tests/test_audit_fixes_2026_07_16.py b/tests/test_audit_fixes_2026_07_16.py new file mode 100644 index 00000000..24c90725 --- /dev/null +++ b/tests/test_audit_fixes_2026_07_16.py @@ -0,0 +1,278 @@ +"""Regressions for the 2026-07-16 full-codebase audit findings. + +Each test names the bug it locks out. See the commit body for the audit trail. +""" +from datetime import date, timedelta +from unittest.mock import MagicMock, patch + +import pytest + +from src.config import RiskConfig +from src.models import Position, TargetPosition, TradeDecision +from src.portfolio_constructor import PortfolioConstructor +from src.risk.rules import RiskRuleEngine +from src.pipeline import TradingPipeline + + +def _cfg(**kw): + base = dict(max_position_pct=20.0, max_total_position_pct=90.0, + max_daily_loss_pct=3.0, max_sector_pct=40.0, + require_stop_loss=True, allow_margin=False) + base.update(kw) + return RiskConfig(**base) + + +def _buy(symbol="SPY", alloc=20.0): + return TradeDecision(action="BUY", symbol=symbol, allocation_pct=alloc, + entry_price=100.0, stop_loss=95.0, take_profit=120.0, + reasoning="t") + + +# ---------- non-finite market_value must BLOCK, not disable the caps ---------- + +def test_nan_market_value_blocks_instead_of_silently_disabling_caps(): + """NaN comparisons are all False, so `total_pct > cap` evaluated False and + the exposure + sector caps switched OFF for the whole session — on exactly + the broken-snapshot day they matter most.""" + eng = RiskRuleEngine(_cfg()) + positions = [ + Position(symbol="AAPL", qty=10, avg_entry=100, current_price=float("nan"), + market_value=float("nan"), unrealized_pnl=0.0, sector="Technology"), + Position(symbol="NVDA", qty=100, avg_entry=800, current_price=800, + market_value=80_000, unrealized_pnl=0.0, sector="Technology"), + ] + violations = eng.check(decision=_buy(), positions=positions, + total_value=100_000.0, daily_pnl=0.0, cash=100_000.0) + from src.pipeline import HARD_BLOCK_RULES + assert violations, "a NaN position must not yield an all-clear" + assert any(v.rule in HARD_BLOCK_RULES for v in violations) + + +def test_clean_snapshot_still_evaluates_normally(): + eng = RiskRuleEngine(_cfg()) + positions = [Position(symbol="NVDA", qty=10, avg_entry=100, current_price=100, + market_value=1_000, unrealized_pnl=0.0, sector="Technology")] + violations = eng.check(decision=_buy(alloc=5.0), positions=positions, + total_value=100_000.0, daily_pnl=0.0, cash=100_000.0) + assert violations == [] + + +# ---------- correlation cluster must include the BUY's own position ---------- + +def test_cluster_includes_the_buy_symbols_own_existing_position(): + """An ADD to the biggest name in a cluster counted only the ADD's notional + and none of the stack already held.""" + eng = RiskRuleEngine(_cfg()) + positions = [ + Position(symbol="NVDA", qty=40, avg_entry=1000, current_price=1000, + market_value=40_000, unrealized_pnl=0.0, sector="Technology"), + Position(symbol="AVGO", qty=10, avg_entry=1000, current_price=1000, + market_value=10_000, unrealized_pnl=0.0, sector="Technology"), + ] + matrix = {"NVDA": {"AVGO": 0.9}, "AVGO": {"NVDA": 0.9}} + violations = eng.check( + decision=_buy("NVDA", alloc=5.0), positions=positions, + total_value=100_000.0, daily_pnl=0.0, cash=100_000.0, + correlation_matrix=matrix, max_correlated_cluster_pct=50.0, + ) + # 40k NVDA + 10k AVGO + 5k add = 55% > 50% cap. Pre-fix: 10k + 5k = 15% → silent. + assert any(v.rule == "correlation_cluster" for v in violations) + + +# ---------- constructor: gross weight delta must convert to raw notional ---------- + +def _target(symbol, weight): + return TargetPosition(symbol=symbol, target_weight_pct=weight, + conviction="high", thesis="t", thesis_invalid_if="") + + +def test_leveraged_etf_target_is_converted_to_raw_notional(): + """PM targets 6% GROSS on SQQQ (3x). Pre-fix the constructor emitted + alloc=6.0, which ExecutionStage spends as $6k raw = 18% gross — 3x the + intended exposure.""" + c = PortfolioConstructor() + decisions = c.construct_orders( + targets=[_target("SQQQ", 6.0)], positions=[], analyses={}, + total_value=100_000.0, price_map={"SQQQ": 100.0}, + ) + buys = [d for d in decisions if d.action == "BUY"] + assert len(buys) == 1 + assert buys[0].allocation_pct == pytest.approx(2.0, abs=0.01) # 6% gross / 3x + + +def test_unleveraged_target_is_unchanged(): + c = PortfolioConstructor() + decisions = c.construct_orders( + targets=[_target("AAPL", 6.0)], positions=[], analyses={}, + total_value=100_000.0, price_map={"AAPL": 100.0}, + ) + buys = [d for d in decisions if d.action == "BUY"] + assert buys[0].allocation_pct == pytest.approx(6.0, abs=0.01) + + +def test_explicit_close_target_is_not_swallowed_by_the_churn_filter(): + """target_weight_pct=0 means CLOSE, not "rebalance toward ~0" — a small + dreg with an explicit close target used to become a HOLD forever.""" + c = PortfolioConstructor() + pos = Position(symbol="AAPL", qty=4, avg_entry=100, current_price=100, + market_value=400, unrealized_pnl=0.0, sector="Technology") + decisions = c.construct_orders( + targets=[_target("AAPL", 0.0)], positions=[pos], analyses={}, + total_value=100_000.0, price_map={"AAPL": 100.0}, + ) + sells = [d for d in decisions if d.action == "SELL"] + assert len(sells) == 1 and sells[0].allocation_pct == 100.0 + + +# ---------- ETF sector resolution ---------- + +def test_sector_etfs_resolve_to_a_real_sector_not_unknown(): + """yfinance .info has no `sector` for ETFs → every one returned "Unknown", + which silently switched max_sector_pct OFF for them.""" + from src.execution.broker import _get_sector, _sector_cache + _sector_cache.clear() + try: + assert _get_sector("XLV") == "Healthcare" + assert _get_sector("XLF") == "Financial Services" + assert _get_sector("SMH") == "Technology" + assert _get_sector("SQQQ") == "Broad" # inverse index ETF: no sector + assert _get_sector("SPY") == "Broad" # pre-existing index fast path + finally: + _sector_cache.clear() + + +def test_held_sector_etf_counts_toward_the_sector_cap(): + """A book that is 30% XLV must not let an LLY BUY through as if Healthcare + exposure were zero.""" + eng = RiskRuleEngine(_cfg(max_sector_pct=40.0)) + positions = [Position(symbol="XLV", qty=200, avg_entry=150, current_price=150, + market_value=30_000, unrealized_pnl=0.0, + sector="Healthcare")] + with patch("src.execution.broker._get_sector", return_value="Healthcare"): + violations = eng.check( + decision=_buy("LLY", alloc=15.0), positions=positions, + total_value=100_000.0, daily_pnl=0.0, cash=100_000.0, + ) + assert any(v.rule == "max_sector_pct" for v in violations) + + +# ---------- ex-dividend: next TRADING day, not calendar tomorrow ---------- + +def _exdiv_pipeline(div_date, today): + p = TradingPipeline.__new__(TradingPipeline) + p.db = MagicMock() + p.db.get_trades.return_value = [] + p.market = MagicMock() + p.market.get_upcoming_ex_dividend.return_value = {"date": div_date, "amount": 0.51} + p.broker = MagicMock() + p.broker.is_trading_day.side_effect = lambda d: d.weekday() < 5 + p.broker.get_current_stop_price.return_value = 61.80 + p.broker.replace_stop_loss.return_value = {"id": "s1", "status": "accepted"} + p._format_qty = lambda q: str(q) + return p + + +def test_monday_ex_div_is_caught_by_friday_session(): + """`today + 1 calendar day` can never BE a Monday for a Mon-Fri session — + every Monday ex-div went unadjusted and stopped positions out on the + mechanical dividend gap.""" + friday, monday = date(2026, 7, 17), date(2026, 7, 20) + p = _exdiv_pipeline(monday, friday) + pos = Position(symbol="KO", qty=200, avg_entry=60, current_price=62.40, + market_value=12_480, unrealized_pnl=480, sector="Consumer Defensive") + with patch("src.pipeline.et_today", return_value=friday): + orders = p._handle_ex_dividends([pos], run_id="r1") + assert len(orders) == 1 + p.broker.replace_stop_loss.assert_called_once() + + +def test_midweek_ex_div_still_uses_tomorrow(): + wed, thu = date(2026, 7, 15), date(2026, 7, 16) + p = _exdiv_pipeline(thu, wed) + pos = Position(symbol="KO", qty=200, avg_entry=60, current_price=62.40, + market_value=12_480, unrealized_pnl=480, sector="Consumer Defensive") + with patch("src.pipeline.et_today", return_value=wed): + assert len(p._handle_ex_dividends([pos], run_id="r1")) == 1 + + +def test_far_future_ex_div_is_not_acted_on_early(): + wed, next_wed = date(2026, 7, 15), date(2026, 7, 22) + p = _exdiv_pipeline(next_wed, wed) + pos = Position(symbol="KO", qty=200, avg_entry=60, current_price=62.40, + market_value=12_480, unrealized_pnl=480, sector="Consumer Defensive") + with patch("src.pipeline.et_today", return_value=wed): + assert p._handle_ex_dividends([pos], run_id="r1") == [] + p.broker.replace_stop_loss.assert_not_called() + + +# ---------- macro sector guidance must survive the round-trip ---------- + +def test_macro_sector_guidance_is_persisted_and_normalized(tmp_path): + """Three stacked breaks made every macro_sector_stance permanently + "unknown": the key was never persisted; the reader wants a dict but the + model carries a list; the vocabulary differs (overweight vs bullish).""" + from src.data.macro_store import MacroStore + store = MacroStore(data_dir=str(tmp_path)) + store.save_last_state({ + "regime": "risk-on", "confidence": "high", "equity_outlook": "bullish", + "summary": "s", "position_guidance": {"target_invested_pct": 75}, + "sector_guidance": [ + {"sector": "Technology", "stance": "overweight", "reason": "AI capex"}, + {"sector": "Real Estate", "stance": "underweight", "reason": "rates"}, + {"sector": "Energy", "stance": "neutral", "reason": "range"}, + ], + }) + state = store.load_last_state() + assert state["sector_guidance"] == { + "Technology": "bullish", "Real Estate": "bearish", "Energy": "neutral", + } + # and the reader that was permanently empty now resolves + p = TradingPipeline.__new__(TradingPipeline) + p.macro_store = store + assert p._missed_ops_macro_sector_map()["Technology"] == "bullish" + + +def test_macro_sector_guidance_tolerates_junk(tmp_path): + from src.data.macro_store import MacroStore + store = MacroStore(data_dir=str(tmp_path)) + store.save_last_state({"regime": "neutral", "sector_guidance": "not a list"}) + assert store.load_last_state()["sector_guidance"] == {} + store.save_last_state({"regime": "neutral", "sector_guidance": None}) + assert store.load_last_state()["sector_guidance"] == {} + + +def test_macro_sector_guidance_normalize_is_idempotent(tmp_path): + """Re-saving an already-normalized dict must not corrupt it.""" + from src.data.macro_store import MacroStore + store = MacroStore(data_dir=str(tmp_path)) + store.save_last_state({"regime": "risk-on", + "sector_guidance": {"Technology": "bullish"}}) + assert store.load_last_state()["sector_guidance"] == {"Technology": "bullish"} + + +# ---------- finalize must persist the PRE-sell qty ---------- + +def test_finalize_persists_pre_sell_qty_so_the_drain_can_reprotect(): + """Persisting the POST-sell residual made the drain recompute + `residual - fill` — a double subtraction that hit 0 for an exact fill, + took the "full exit, nothing to protect" early return, reported success, + and DELETED the row. The residual stayed naked forever.""" + p = TradingPipeline.__new__(TradingPipeline) + p.broker = MagicMock() + p.broker.wait_for_order_terminal.return_value = "filled" + p.broker.get_order_fill_info.return_value = {"status": "filled", "filled_qty": 50.0} + p.db = MagicMock() + p._current_position_qty_for_finalize = MagicMock(return_value=50.0) + p._reprotect_residual_after_partial_sell = MagicMock(return_value=False) # blip + p._persist_orphaned_protection_restore = MagicMock() + + ok, _ = p._finalize_protection_after_sell_core( + order_id="o1", symbol="NVDA", position_qty_before_sell=100.0, + cancelled_specs=[{"id": "s1", "qty": 100.0, "stop_price": 95.0}], + from_drain=False, wal_row_id=7, + ) + assert ok is False + persisted_qty = p._persist_orphaned_protection_restore.call_args[0][2] + assert persisted_qty == 100.0, ( + "must persist the PRE-sell qty; the drain re-derives residual = pre - fill" + ) diff --git a/tests/test_broker.py b/tests/test_broker.py index b4faf52e..bea84089 100644 --- a/tests/test_broker.py +++ b/tests/test_broker.py @@ -95,15 +95,20 @@ def test_is_trading_day_uses_calendar(mock_tc_cls): @patch("src.execution.broker.TradingClient") def test_get_session_close_returns_et_datetime_on_trading_day(mock_tc_cls): - """Half-day detection path: calendar is queried; combines returned - date + close time into an ET-aware datetime. This is what the pipeline's - early-close guard compares `et_now()` against.""" - from datetime import date as _date, time as _time, datetime as _dt + """Half-day detection path, against the REAL SDK model. + + 2026-07-16 audit: this test used to build a MagicMock with + `entry.close = time(13, 0)`. The real `alpaca.trading.models.Calendar` + returns a full naive DATETIME, so production hit + `datetime.combine(date, datetime)` → TypeError → None on EVERY call and + the early-close guard was dead code — while this test stayed green. + Construct the real model so the shape can't drift silently again.""" + from datetime import date as _date, datetime as _dt + from alpaca.trading.models import Calendar from src.trading_calendar import ET - entry = MagicMock() - entry.date = _date(2026, 11, 27) # Thanksgiving Friday — 13:00 early close - entry.close = _time(13, 0) + # Thanksgiving Friday — 13:00 early close + entry = Calendar(date="2026-11-27", open="09:30", close="13:00") mock_client = MagicMock() mock_client.get_calendar.return_value = [entry] mock_tc_cls.return_value = mock_client @@ -111,12 +116,32 @@ def test_get_session_close_returns_et_datetime_on_trading_day(mock_tc_cls): broker = AlpacaBroker(api_key="test", secret_key="test", paper=True) close = broker.get_session_close(on_date=_date(2026, 11, 27)) + assert close is not None, "early-close guard is dead if this returns None" assert isinstance(close, _dt) assert close.tzinfo is ET assert close.hour == 13 and close.minute == 0 assert close.date() == _date(2026, 11, 27) +@patch("src.execution.broker.TradingClient") +def test_get_session_close_accepts_legacy_time_shape(mock_tc_cls): + """Older/alternative SDK shape (close as a `time`) must still combine.""" + from datetime import date as _date, time as _time, datetime as _dt + from src.trading_calendar import ET + + entry = MagicMock() + entry.date = _date(2026, 11, 27) + entry.close = _time(13, 0) + mock_client = MagicMock() + mock_client.get_calendar.return_value = [entry] + mock_tc_cls.return_value = mock_client + + broker = AlpacaBroker(api_key="test", secret_key="test", paper=True) + close = broker.get_session_close(on_date=_date(2026, 11, 27)) + + assert isinstance(close, _dt) and close.hour == 13 and close.tzinfo is ET + + @patch("src.execution.broker.TradingClient") def test_get_session_close_returns_none_on_non_trading_day(mock_tc_cls): mock_client = MagicMock() diff --git a/tests/test_macro_store.py b/tests/test_macro_store.py index 679b24cc..939010d7 100644 --- a/tests/test_macro_store.py +++ b/tests/test_macro_store.py @@ -22,7 +22,9 @@ def test_save_then_load_round_trip(tmp_path): }, # Fields not in the snapshot subset — should be dropped. "reasoning_chain": {"volatility_analysis": "…"}, - "sector_guidance": [{"sector": "Technology", "stance": "overweight"}], + "sector_guidance": [ + {"sector": "Technology", "stance": "overweight", "reason": "AI capex cycle"}, + ], } store.save_last_state(analysis) @@ -32,7 +34,14 @@ def test_save_then_load_round_trip(tmp_path): assert loaded["position_guidance"]["target_invested_pct"] == 75.0 # Ensure the large fields are NOT persisted (we want the snapshot tiny). assert "reasoning_chain" not in loaded - assert "sector_guidance" not in loaded + # sector_guidance IS persisted — but compactly, as {sector: direction}. + # 2026-07-16 audit: this test used to assert it was dropped, pinning the + # bug that made every downstream macro_sector_stance permanently + # "unknown". The "keep it tiny" intent is honoured by storing the + # normalized map WITHOUT the bulky `reason` prose (that lives in + # agent_logs). + assert loaded["sector_guidance"] == {"Technology": "bullish"} + assert "reason" not in json.dumps(loaded["sector_guidance"]) # Date stamp is added on save. assert "date" in loaded From c1af704424478cd1feb4f6856f6b19451fcd84c4 Mon Sep 17 00:00:00 2001 From: yebof Date: Thu, 16 Jul 2026 19:28:02 +0800 Subject: [PATCH 3/4] =?UTF-8?q?fix(audit):=206=20more=20defects=20?= =?UTF-8?q?=E2=80=94=20silent=20data=20loss,=20dead=20indicators,=20invert?= =?UTF-8?q?ed=20views?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second pass over the audit's unverified backlog (the verify fleet hit the session limit mid-run, so I verified these by hand — each is reproduced). SILENT DATA LOSS - parse_json scored a top-level LIST as 0, so whenever the model wrapped its JSON in any prose the candidate scan compared the array against its own elements and returned the LAST ELEMENT. tech_analyst returns an ARRAY of per-symbol analyses (`items = parsed if isinstance(parsed, list) else [parsed]`), so a 25-symbol chunk could silently collapse to 1 analysis with 24 discarded and no error anywhere. Reproduced; a container is now scored by the SUM of its elements so it strictly outranks any element it contains. DEAD INDICATOR - ma_200 was unconditionally None in production: lookback_days is CALENDAR days (`start = today - timedelta(days=lookback_days)`), so 120 yielded ~82 bars and technical.py's `len(df) >= 200` never held. The tech_analyst prompt rendered "MA200=None" for every symbol every day — the analyst was asked to judge trend with the most-used long-term reference permanently absent. lookback_days 120 -> 320 (~220 bars), with a test that pins the config against every indicator window the code advertises. STALE / INVERTED VIEWS - ExecutionStage discarded the fresh price_map from the no-SELL refresh, so for an ADD to a held name the 5% entry-staleness guard and the order sizing both used research-time prices from 5-10 minutes earlier. - position_reviewer was handed RAW cash while the sweep vehicle was stripped from its positions — it saw an all-in book with a few hundred dollars spare while most of the equity sat parked and instantly available. Its de-lever mandate and weight reasoning key off that number. (Regression from the cash-sweep work: DecisionStage already credited it for the PM.) - The evening Telegram position snapshot counted parked SGOV as deployed capital, reporting a ~99%-deployed book on a night the money was entirely in T-bills, and listing SGOV among the P&L movers. Parked value is now reported separately as what it is. - position_reviewer's prompt instructs "think in ATRs" and the pipeline pays for an ATR fetch per position, but build_user_message never rendered atr_pct / stop_distance_atrs — the instruction referred to data the model could not see. (Also a regression from the exit-quality work.) 1336 tests green (+9 regressions here). Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01PQkESoSTYx2bCy7WYnTPXR --- config/settings.yaml | 12 ++++- src/agents/base.py | 12 +++++ src/agents/position_reviewer.py | 10 ++++ src/notifier.py | 21 ++++++++ src/pipeline.py | 15 +++++- src/pipeline_stages.py | 12 ++++- tests/test_audit_fixes_2026_07_16.py | 81 ++++++++++++++++++++++++++++ 7 files changed, 159 insertions(+), 4 deletions(-) diff --git a/config/settings.yaml b/config/settings.yaml index 1f2a11ad..83a79dbc 100644 --- a/config/settings.yaml +++ b/config/settings.yaml @@ -182,7 +182,17 @@ trading: # Data Center REITs (AI infra exposure — interconnect + hyperscale) - EQIX # Equinix (global #1 interconnection / colocation) - DLR # Digital Realty (hyperscale data-center REIT) - lookback_days: 120 + # CALENDAR days of OHLCV history — market.get_ohlcv does + # `start = today - timedelta(days=lookback_days)`, so the usable BAR count is + # only ~69% of this (120 gave ~82 bars). + # + # 2026-07-16 audit: MA200 needs 200 bars, so `ma_200` was unconditionally + # None in production and the tech_analyst prompt rendered "MA200=None" for + # every symbol every day — the analyst was asked to judge trend with the + # most-used long-term reference permanently absent. 320 calendar days ≈ 220 + # trading days: enough for MA200 with holiday headroom. Pinned by + # tests/test_audit_fixes_2026_07_16.py against every advertised indicator. + lookback_days: 320 # NOTE: these times are only consumed by `main.py --mode live` (APScheduler). # Production runs via launchd — see `src/trading_calendar.py SESSION_WINDOWS` # for the authoritative ET windows and `scripts/run_if_et_window.sh` for the diff --git a/src/agents/base.py b/src/agents/base.py index e088f9d6..08cad6ee 100644 --- a/src/agents/base.py +++ b/src/agents/base.py @@ -361,6 +361,18 @@ class AgentResult: @staticmethod def _shape_score(parsed) -> int: """How 'agent-output shaped' a JSON candidate looks. Higher is better.""" + # A top-level LIST is a first-class agent shape: tech_analyst returns + # an array of per-symbol analyses (tech_analyst.py: `items = parsed if + # isinstance(parsed, list) else [parsed]`). Scoring it 0 meant that + # whenever the model wrapped the array in ANY prose (so the clean + # json.loads happy path missed), the candidate scan compared the array + # (score 0) against each of its own elements (score > 0) and returned + # the LAST ELEMENT — silently discarding every other symbol's analysis + # in the chunk. Score the container by the SUM of its elements so it + # strictly outranks any single element it contains (2026-07-16 audit; + # reproduced: a 3-analysis array returned 1 dict). + if isinstance(parsed, list): + return sum(AgentResult._shape_score(item) for item in parsed) if not isinstance(parsed, dict): return 0 keys = set(parsed.keys()) diff --git a/src/agents/position_reviewer.py b/src/agents/position_reviewer.py index 1fe71020..6b1d8bc2 100644 --- a/src/agents/position_reviewer.py +++ b/src/agents/position_reviewer.py @@ -186,6 +186,16 @@ def _pnl_pct(p: Position) -> str: metric_bits.append(f"to_stop={pf['distance_to_stop_pct']:.1f}%") if pf.get("distance_to_target_pct") is not None: metric_bits.append(f"to_target={pf['distance_to_target_pct']:.1f}%") + # Vol units. The prompt tells the reviewer to reason in ATRs ("a + # stop <1.25 ATRs away is inside daily noise") and the pipeline + # pays for an ATR fetch per position to compute these — but they + # were never rendered, so the instruction referred to data the + # model could not see (2026-07-16 audit; drift introduced with the + # exit-quality work). None => omit: unknown must not read as zero. + if pf.get("atr_pct") is not None: + metric_bits.append(f"atr={pf['atr_pct']:.2f}%") + if pf.get("stop_distance_atrs") is not None: + metric_bits.append(f"stop_distance={pf['stop_distance_atrs']:.2f}×ATR") if pf.get("weight_pct") is not None: metric_bits.append(f"weight={pf['weight_pct']:.1f}%") if metric_bits: diff --git a/src/notifier.py b/src/notifier.py index 71ffea6f..d2f8b323 100644 --- a/src/notifier.py +++ b/src/notifier.py @@ -37,6 +37,14 @@ # the caller's CWD. _DB_PATH = Path(__file__).resolve().parent.parent / "data" / "quant_agent.db" +# Cash-sweep parking vehicles — cash equivalents, never "deployed capital". +# The notifier reads the DB directly (it deliberately doesn't thread config +# in — see the comment at the sqlite3 connect), so it can't ask +# CashSweepConfig for the configured symbol. Cover the supported vehicles; +# an unknown custom symbol degrades to today's behaviour (counted as a +# position), which is visible rather than silent. +_SWEEP_SYMBOLS = frozenset({"SGOV", "BIL"}) + class TelegramNotifier: """Best-effort Telegram Bot API notifier. @@ -562,6 +570,15 @@ def _append_position_snapshot(lines: list[str], total_value: float | None) -> No return if not rows: return + # The cash-sweep vehicle is parked CASH, not deployed capital (that's its + # whole contract: hidden from every LLM view, credited as cash by the risk + # engine, first to liquidate in force_delever). Counting it here reported a + # ~99%-deployed book on a night the money was entirely in T-bills — + # inverting the operator's one nightly glance at exposure, and listing SGOV + # among the P&L movers (2026-07-16 audit). + parked = sum(r[4] for r in rows + if r[0] in _SWEEP_SYMBOLS and r[4] is not None) + rows = [r for r in rows if r[0] not in _SWEEP_SYMBOLS] invested = sum(r[4] for r in rows if r[4] is not None) cash_pct = None if total_value and total_value > 0: @@ -569,7 +586,11 @@ def _append_position_snapshot(lines: list[str], total_value: float | None) -> No summary = f" Positions: {len(rows)} invested ${invested:,.0f}" if cash_pct is not None: summary += f" ({100 - cash_pct:.0f}% deployed / {cash_pct:.0f}% cash)" + if parked > 0: + summary += f" [+${parked:,.0f} parked in T-bills]" lines.append(summary) + if not rows: + return def _row_line(r: tuple) -> str: sym, qty, avg, curr, mv, pnl = r diff --git a/src/pipeline.py b/src/pipeline.py index 2aa123ab..1a109c0c 100644 --- a/src/pipeline.py +++ b/src/pipeline.py @@ -5783,9 +5783,20 @@ def run_position_review(self, session_type: str = "midday") -> dict: # `positions` stays in scope for the paths that need broker truth # (emergency liquidate below sells EVERYTHING, parked cash included). review_positions = positions + review_cash = cash sweeper = self._sweeper() if sweeper is not None: - review_positions, _parked = sweeper.split_positions(positions) + review_positions, parked = sweeper.split_positions(positions) + # ...and the cash side of that same contract: stripping the + # vehicle from the position list while showing RAW cash told the + # reviewer the book was ~all-in with a few hundred dollars spare, + # when most of the equity was parked and instantly available. Its + # de-lever mandate and weight reasoning both key off this number + # (2026-07-16 audit; DecisionStage already credits it for the PM). + if parked is not None: + mv = parked.market_value + if isinstance(mv, (int, float)) and math.isfinite(mv) and mv > 0: + review_cash = cash + mv if review_positions: # Sweep any straggler fills before building the reviewer prompt. @@ -5842,7 +5853,7 @@ def run_position_review(self, session_type: str = "midday") -> dict: review, md_result = self.position_reviewer.review( positions=review_positions, macro_summary=macro_summary, - cash_balance=cash, + cash_balance=review_cash, total_value=total_value, session_type=session_type, position_facts=position_facts, diff --git a/src/pipeline_stages.py b/src/pipeline_stages.py index 73025f08..ed2be3c5 100644 --- a/src/pipeline_stages.py +++ b/src/pipeline_stages.py @@ -879,12 +879,22 @@ def run(self, ctx: RunContext) -> list[dict]: # snapshot. if buy_decisions: if not sell_decisions: - account, positions, _ = pipeline._refresh_account_state() + # Take the FRESH price_map too (2026-07-16 audit): it was + # discarded into `_`, leaving `price_map` at research-time + # position prices from 5-10 minutes earlier. For an ADD to a + # held name that stale price is what the 5% entry-staleness + # guard compares the LLM's entry against, and what sizes the + # order — so the guard could pass a genuinely stale entry (or + # reject a good one) on exactly the fast-moving tape where it + # matters. New symbols were unaffected (they miss the map and + # fall through to a live quote). + account, positions, fresh_prices = pipeline._refresh_account_state() cash = account["cash"] total_value = account["portfolio_value"] ctx.positions = positions ctx.cash = cash ctx.total_value = total_value + price_map = {**price_map, **fresh_prices} daily_pnl_now = total_value - ctx.last_equity loss_violation_now = pipeline.risk_engine.check_daily_loss( ctx.last_equity, daily_pnl_now, diff --git a/tests/test_audit_fixes_2026_07_16.py b/tests/test_audit_fixes_2026_07_16.py index 24c90725..fd4ff8c7 100644 --- a/tests/test_audit_fixes_2026_07_16.py +++ b/tests/test_audit_fixes_2026_07_16.py @@ -276,3 +276,84 @@ def test_finalize_persists_pre_sell_qty_so_the_drain_can_reprotect(): assert persisted_qty == 100.0, ( "must persist the PRE-sell qty; the drain re-derives residual = pre - fill" ) + + +# ---------- parse_json must not drop a top-level array ---------- + +def test_parse_json_keeps_a_prose_wrapped_top_level_array(): + """tech_analyst returns an ARRAY of per-symbol analyses. Scoring lists 0 + meant the candidate scan compared the array against its own elements and + returned the LAST one — silently discarding 24 of 25 symbols in a chunk + whenever the model wrapped its JSON in any prose.""" + from src.agents.base import AgentResult + raw = ( + "Here are the analyses:\n" + '[{"symbol": "AAPL", "rating": "buy"}, ' + '{"symbol": "NVDA", "rating": "hold"}, ' + '{"symbol": "MSFT", "rating": "sell"}]\n' + ) + out = AgentResult(raw_text=raw, tokens_used=0, model="m").parse_json() + assert isinstance(out, list) and len(out) == 3 + assert [x["symbol"] for x in out] == ["AAPL", "NVDA", "MSFT"] + + +def test_parse_json_single_dict_response_still_wins(): + from src.agents.base import AgentResult + raw = 'Result: {"decisions": [], "portfolio_view": "flat"}' + out = AgentResult(raw_text=raw, tokens_used=0, model="m").parse_json() + assert isinstance(out, dict) and out["portfolio_view"] == "flat" + + +# ---------- indicator windows must fit in the configured lookback ---------- + +def test_configured_lookback_supplies_every_advertised_indicator(): + """`ma_200` was unconditionally None: lookback_days is CALENDAR days, so + 120 yielded ~82 bars and `len(df) >= 200` never held — the tech_analyst + prompt rendered "MA200=None" for every symbol, every day.""" + from pathlib import Path + import yaml + cfg = yaml.safe_load( + (Path(__file__).resolve().parents[1] / "config" / "settings.yaml").read_text() + ) + calendar_days = cfg["trading"]["lookback_days"] + # ~252 trading days per 365 calendar days + approx_bars = calendar_days * 252 / 365 + assert approx_bars >= 200, ( + f"lookback_days={calendar_days} yields ~{approx_bars:.0f} bars; MA200 " + f"needs 200 and technical.py gates on `len(df) >= 200`" + ) + + +# ---------- reviewer view: ATR metrics rendered, parked cash credited ---------- + +def _reviewer(): + from src.agents.position_reviewer import PositionReviewerAgent + with patch("anthropic.Anthropic"): + return PositionReviewerAgent(api_key="k", model="claude-opus-4-7", + max_tokens=1024) + + +def test_reviewer_prompt_renders_the_atr_metrics_it_is_told_to_use(): + """The prompt instructs "think in ATRs" and the pipeline pays for an ATR + fetch per position — but build_user_message never rendered them.""" + pos = Position(symbol="GE", qty=26, avg_entry=316, current_price=360, + market_value=9_360, unrealized_pnl=1_144, sector="Industrials") + msg = _reviewer().build_user_message( + positions=[pos], macro_summary={}, cash_balance=1_000.0, + total_value=100_000.0, session_type="midday", + position_facts={"GE": {"atr_pct": 2.22, "stop_distance_atrs": 1.25, + "distance_to_stop_pct": 2.8}}, + ) + assert "atr=2.22%" in msg + assert "stop_distance=1.25×ATR" in msg + + +def test_reviewer_prompt_omits_unknown_atr_rather_than_showing_zero(): + pos = Position(symbol="GE", qty=26, avg_entry=316, current_price=360, + market_value=9_360, unrealized_pnl=1_144, sector="Industrials") + msg = _reviewer().build_user_message( + positions=[pos], macro_summary={}, cash_balance=1_000.0, + total_value=100_000.0, session_type="midday", + position_facts={"GE": {"atr_pct": None, "stop_distance_atrs": None}}, + ) + assert "atr=" not in msg and "stop_distance=" not in msg From 6b4519fb66b3c78cf9b0594db4ae051a557a8fc5 Mon Sep 17 00:00:00 2001 From: yebof Date: Thu, 16 Jul 2026 19:45:10 +0800 Subject: [PATCH 4/4] fix(audit): 6 verified defects from the audit backlog (2 major, 4 minor) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A second verify pass (14 skeptics, one per remaining candidate) confirmed 6 and refuted 8 — the refuted set included several plausible-looking claims that did not survive contact with the code (news symbol matching already uses word boundaries; every _locked_write caller passes a single statement; the Anthropic cache accounting is right even though its comment is stale). MAJOR - compute_trade_calibration ignored FILLED TRAIL_STOP exits, so every stop-out left a phantom open lot and closed nothing: LLY BUY8 -> stop-filled 8 -> BUY6 -> stop-filled 6 read as 14 shares still held and zero LLY trades closed, while the position was flat. The 8 real stop-outs in the ledger move the numbers materially (a typical window: win_rate 22.2% -> 30.0%, avg_return -2.79% -> -2.18%) — and these feed PM as facts and the reviewer as calibration_note. The filled-guard mirrors _build_post_exit_reality: a placed but unfilled stop is protection, not an exit. - earnings record_failure() wrote the new filing_date into the manifest, and _check_symbol treats that as "already processed" — so after the FIRST transient LLM failure the filing was never re-queued, the 3-strike budget never reached `abandoned`, and PM was served the PRIOR quarter's analysis labelled "[from cache]" as if it were current. It bit every symbol with a same-form analysis already on disk (12 today; the whole universe in steady state). "Already processed" now means SUCCEEDED — confirm_filing() zeroes failed_attempts, so attempts 1-2 retry as designed and the 3rd abandons. MINOR - _build_trade_grade_summary counted each re-grade of the same SELL as an independent sell (the 2-day grading window has no already-graded filter, so evening re-grades a trade 2-3 nights running), inflating the premature/wrong counts that drive the reviewer's patience tilt. Deduped on (symbol, sell_date); a malformed row without a date is counted rather than collapsed. - _symbols_already_trimmed_today failed OPEN on a partially-filled-then- canceled sell: shares left the book at midday, but close saw a clean slate and could trim the same name again on the same soft flag — the 2026-05-04 AMZN double-trim this guard exists to stop. Now uses the codebase's existing _trade_executed_or_pending contract (a zero-fill rejection still allows a retry). - _clamp_queued_earnings_buys capped the constructor's DELTA, not the RESULTING weight, so a name held at 15% with an unread filing could be topped up to 20% because the add itself was <= 5% — while the prompt and the docstring both promise a cap on the resulting position. - credit_spread's "change_30d_bps" measured ~57-60 days: the reference was the head of a 60-calendar-day fetch rather than an observation ~30 days back (inherited from the earlier MONTHLY series fetcher, where it was harmless). On live FRED data today it reported -11.0 bps where the true 30-day change was +6.0 — a sign flip on a risk-off input to macro. 1341 tests green (+15 regressions here). Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01PQkESoSTYx2bCy7WYnTPXR --- src/data/earnings.py | 28 ++++- src/data/macro.py | 16 ++- src/pipeline.py | 118 +++++++++++++++---- src/pipeline_stages.py | 6 + src/storage/db.py | 42 ++++++- tests/test_audit_fixes_2026_07_16.py | 169 +++++++++++++++++++++++++++ tests/test_cash_sweep.py | 2 +- 7 files changed, 353 insertions(+), 28 deletions(-) diff --git a/src/data/earnings.py b/src/data/earnings.py index 55de9fdb..63aaccc7 100644 --- a/src/data/earnings.py +++ b/src/data/earnings.py @@ -622,12 +622,38 @@ def _check_symbol(self, symbol: str) -> EarningsReport | None: ) return self._get_existing_analysis(symbol, form_type=latest.form_type) - if last_known == latest.filing_date: + # "Already processed" must mean SUCCEEDED, not merely attempted. + # + # 2026-07-16 audit: record_failure() writes the new filing_date into + # the manifest alongside failed_attempts=1 and logs "will retry next + # session" — but this gate then saw last_known == latest.filing_date, + # found the PRIOR quarter's analysis on disk, and returned it with + # is_new=False. The pipeline only re-queues is_new reports, so the + # filing was never re-analyzed, record_failure never ticked again, the + # 3-strike budget never reached `abandoned`, and PM was served last + # quarter's numbers labelled "[from cache]" as if they were current. + # One transient LLM failure permanently dropped that quarter's filing — + # for every symbol that already had a same-form analysis on disk, i.e. + # the whole universe in steady state. + # confirm_filing() writes failed_attempts=0 on success, so a genuinely + # processed filing still short-circuits here. Attempts 1-2 now fall + # through to re-download → is_new=True → re-analysis; on the 3rd + # failure the `abandoned` branch above takes over as designed. + try: + prior_failures = int(entry.get("failed_attempts", 0) or 0) + except (TypeError, ValueError): + prior_failures = 0 + if last_known == latest.filing_date and not prior_failures: # Already processed this filing — return existing analysis matching this form_type existing = self._get_existing_analysis(symbol, form_type=latest.form_type) if existing: return existing # Analysis file missing (e.g. killed mid-analysis) — re-download + elif last_known == latest.filing_date and prior_failures: + logger.info( + "%s %s (%s): retrying after %d failed analysis attempt(s)", + symbol, latest.form_type, latest.filing_date, prior_failures, + ) # New filing — download it local_path = self._download_filing(cik, latest) diff --git a/src/data/macro.py b/src/data/macro.py index e9a7a1e9..126b482c 100644 --- a/src/data/macro.py +++ b/src/data/macro.py @@ -225,7 +225,21 @@ def get_credit_spread(self) -> dict: if series.empty: return {"current_bps": None, "change_30d_bps": None, "staleness_days": None} current = float(series.iloc[-1]) * 100 # FRED returns % — convert to bps - prior_30d = float(series.iloc[0]) * 100 if len(series) >= 2 else current + # Anchor the reference to a DATE, not to the head of the window. + # + # 2026-07-16 audit: `series.iloc[0]` is the OLDEST observation in a + # 60-CALENDAR-day fetch, so "change_30d_bps" was really a ~57-60 day + # change — about 2x the advertised window, and on a live check it even + # flipped the sign (code said -11.0 bps; the true 30-day change was + # +6.0 bps). BAMLH0A0HYM2 is business-daily; keep the 60d fetch as + # buffer for holidays/gaps, but take the last observation at or before + # T-30d. (The wide window was inherited verbatim from the earlier + # MONTHLY FEDFUNDS fetcher, where iloc[0] was harmless.) + prior_30d = current + if len(series) >= 2: + cutoff = series.index[-1] - pd.Timedelta(days=30) + prior = series[series.index <= cutoff] + prior_30d = float(prior.iloc[-1] if not prior.empty else series.iloc[0]) * 100 return { "current_bps": round(current, 1), "change_30d_bps": round(current - prior_30d, 1), diff --git a/src/pipeline.py b/src/pipeline.py index 1a109c0c..099e63d5 100644 --- a/src/pipeline.py +++ b/src/pipeline.py @@ -1857,13 +1857,25 @@ def _clamp_queued_earnings_buys( decisions: list[TradeDecision], earnings_results: list[dict], max_pct: float = 5.0, + positions: list | None = None, + total_value: float | None = None, ) -> list[TradeDecision]: - """Hard-cap BUY allocation on symbols with queued (just-filed) earnings. + """Hard-cap the RESULTING position weight on symbols with queued + (just-filed) earnings. A 10-Q filed today but not yet analyzed by the LLM can move the stock ±10% overnight. PM shouldn't size up before the analyst has read it. - Prompt rule asks PM to self-comply; this is the belt that keeps things - safe even if the LLM ignores the rule. + The prompt rule asks PM to self-comply ("cap at target_weight_pct <= + 5.0"); this is the belt that holds when the LLM ignores it. + + 2026-07-16 audit: the belt capped the wrong number. By this point in + the pipeline `allocation_pct` is the constructor's DELTA (target minus + current weight), not the target — so a name already held at 15% with + an unread filing could be topped up to 20% because the ADD itself was + <= 5%. The cap now measures what it documents: existing weight + add. + `positions`/`total_value` are optional so the old delta-only behavior + remains for callers that can't supply a book (tests, and any future + caller with no position context) rather than crashing. """ queued_symbols = { (ea.get("symbol") or "").strip().upper() @@ -1873,20 +1885,49 @@ def _clamp_queued_earnings_buys( queued_symbols.discard("") if not queued_symbols: return decisions + + # Existing GROSS weights, same convention as the risk engine. + current: dict[str, float] = {} + if positions and total_value and total_value > 0: + try: + from src.portfolio_constructor import PortfolioConstructor + current = PortfolioConstructor._current_weights(positions, total_value) + except Exception as e: # noqa: BLE001 + logger.warning("Earnings-queued cap: weight lookup failed (%s) — " + "falling back to delta-only capping", e) + current = {} + clamped: list[TradeDecision] = [] for d in decisions: - if d.action == "BUY" and d.symbol.upper() in queued_symbols and d.allocation_pct > max_pct: - try: - reduced = d.model_copy(update={"allocation_pct": max_pct}) - logger.warning( - "Earnings-queued cap: %s BUY %.2f%% → %.2f%% (fresh filing not yet analyzed)", - d.symbol, d.allocation_pct, max_pct, - ) - clamped.append(reduced) - except Exception as e: - logger.warning("Earnings-queued cap copy failed for %s: %s — keeping original", d.symbol, e) - clamped.append(d) - else: + if d.action != "BUY" or d.symbol.upper() not in queued_symbols: + clamped.append(d) + continue + from src.risk.rules import _gross_multiplier + held_pct = current.get(d.symbol.upper(), 0.0) + # Room left under the cap, expressed in the RAW notional units + # `allocation_pct` is spent in (see PortfolioConstructor._build_buy). + allowed_raw = max(0.0, max_pct - held_pct) / _gross_multiplier(d.symbol) + if d.allocation_pct <= allowed_raw: + clamped.append(d) + continue + if allowed_raw <= 0: + logger.warning( + "Earnings-queued cap: DROPPING %s BUY %.2f%% — already at " + "%.1f%% weight, at/over the %.1f%% cap with a fresh filing " + "not yet analyzed", + d.symbol, d.allocation_pct, held_pct, max_pct, + ) + continue # a BUY with allocation_pct=0 is not a valid no-op downstream + try: + reduced = d.model_copy(update={"allocation_pct": round(allowed_raw, 2)}) + logger.warning( + "Earnings-queued cap: %s BUY %.2f%% → %.2f%% (held %.1f%%, " + "cap %.1f%%; fresh filing not yet analyzed)", + d.symbol, d.allocation_pct, allowed_raw, held_pct, max_pct, + ) + clamped.append(reduced) + except Exception as e: + logger.warning("Earnings-queued cap copy failed for %s: %s — keeping original", d.symbol, e) clamped.append(d) return clamped @@ -3064,14 +3105,35 @@ def _load(col: str, row: dict) -> list[dict]: return v rows_in_window = rows[:lookback_days] # newest first from get_recent_insights + # One SELL, one vote. `_build_recent_sells_for_grading` uses a 2-day + # window with no already-graded filter, so evening re-grades the same + # trade on 2-3 consecutive nights and each re-grade used to count as an + # independent sell — inflating the premature/wrong counts that drive + # the reviewer's patience tilt (2026-07-16 audit; the production + # insights rows show the duplicates). Rows arrive newest-first, so the + # FIRST grade seen for a (symbol, sell_date) is the freshest — and the + # one with the most post-exit price history behind it. + seen_sells: set[tuple] = set() for row in rows_in_window: for g in _load("sell_grades_json", row): if not isinstance(g, dict): continue + sym = g.get("symbol") + sell_date = g.get("sell_date") + # Dedup only with a real (symbol, sell_date) key — SellGrade + # requires sell_date, so this is the normal path. A malformed + # row without one is counted rather than collapsed: keying on + # (symbol, None) would fold every distinct sell of that symbol + # into a single vote, which is a worse error than the + # double-count this dedup removes. + if sym and sell_date: + key = (sym, sell_date) + if key in seen_sells: + continue + seen_sells.add(key) grade = g.get("grade") if grade in sell_counts: sell_counts[grade] += 1 - sym = g.get("symbol") if sym and grade == "premature": sell_premature_by_symbol[sym] = sell_premature_by_symbol.get(sym, 0) + 1 if sym and grade == "wrong": @@ -4754,10 +4816,13 @@ def _symbols_already_trimmed_today(self) -> set[str]: EMERGENCY_SELL / FORCE_DELEVER. TRAIL_STOP and HOLD do NOT count (TRAIL_STOP is stop adjustment, HOLD is no-op). - Filters out canceled / rejected / expired fills — if a SELL was - submitted earlier and broker rejected, the symbol is fair game for - re-trying. Pending (`submitted`) and `filled` rows both block, so - we never double-submit on the same symbol within one day. + Filters out canceled / rejected / expired orders that filled ZERO + shares — if a SELL was submitted earlier and the broker rejected it, + the symbol is fair game for re-trying. A PARTIAL fill still blocks: + those shares left the book, so a second trim today would be the + double-application this guard exists to prevent. Pending (`submitted`) + and `filled` rows both block, so we never double-submit on the same + symbol within one day. """ try: rows = self.db.get_trades(today_only=True, limit=200) @@ -4770,7 +4835,6 @@ def _symbols_already_trimmed_today(self) -> set[str]: "REDUCE", "SELL", "TAKE_PROFIT", "EMERGENCY_SELL", "FORCE_DELEVER", } - bad_status = {"canceled", "cancelled", "rejected", "expired", "done_for_day"} out: set[str] = set() for r in rows: action = (r.get("action") or "").upper() @@ -4778,8 +4842,16 @@ def _symbols_already_trimmed_today(self) -> set[str]: base_action = action.split("(", 1)[0].strip() if base_action not in sell_actions and base_action != "PARTIAL_SELL": continue - status = (r.get("fill_status") or "").lower() - if status in bad_status: + # A terminal-fail status that nevertheless moved shares IS a trim. + # Filtering on fill_status alone (2026-07-16 audit) let a + # partially-filled-then-canceled REDUCE fall through: the shares + # left the book at midday, but close saw a clean slate and was free + # to trim the same name again on the same soft flag — the exact + # 2026-05-04 AMZN 41→21→11 double-trim this guard exists to stop. + # `_trade_executed_or_pending` is the codebase's existing contract + # for this (NULL/submitted/filled → yes; canceled/rejected/expired + # → only when fill_qty > 0), and matches db._executed_trade_predicate. + if not self._trade_executed_or_pending(r): continue sym = r.get("symbol") if sym: diff --git a/src/pipeline_stages.py b/src/pipeline_stages.py index ed2be3c5..54f3334b 100644 --- a/src/pipeline_stages.py +++ b/src/pipeline_stages.py @@ -577,8 +577,14 @@ def run(self, ctx: RunContext) -> dict | None: len(portfolio_decision.decisions), ) + # Pass the book so the cap measures the RESULTING weight, not just the + # add: allocation_pct here is the constructor's delta, so a name already + # at 15% with an unread filing could otherwise be topped up to 20%. + # rm_positions (sweep-vehicle-free) is the right basis — parked T-bills + # are cash and never carry an earnings filing. portfolio_decision.decisions = pipeline._clamp_queued_earnings_buys( portfolio_decision.decisions, earnings_results, + positions=rm_positions, total_value=total_value, ) daily_pnl = total_value - last_equity diff --git a/src/storage/db.py b/src/storage/db.py index 325dae05..2656fe93 100644 --- a/src/storage/db.py +++ b/src/storage/db.py @@ -8,6 +8,31 @@ logger = logging.getLogger(__name__) +def _is_filled_trail_stop(row, action: str) -> bool: + """True for a TRAIL_STOP row the broker actually EXECUTED. + + A TRAIL_STOP row is written fill_status='submitted' at placement and only + flipped to 'filled' by _reconcile_fills when the broker reports a fill, so + the status is what distinguishes "protection sitting there" from "the stop + sold our shares". Mirrors the same distinction in + pipeline._build_post_exit_reality. Legacy rows can carry a NULL + fill_status; those only count when a real fill_qty was recorded, so a + never-filled stop can't book a phantom exit at its stop price. + """ + if action != "TRAIL_STOP": + return False + try: + status = (row["fill_status"] or "").lower() + except (KeyError, IndexError, TypeError): + status = "" + if status == "filled": + return True + try: + return status == "" and float(row["fill_qty"] or 0) > 0 + except (KeyError, IndexError, TypeError, ValueError): + return False + + class Database: def __init__(self, db_path: str): self.db_path = db_path @@ -1104,7 +1129,8 @@ def compute_trade_calibration(self, lookback_days: int = 45) -> dict: # pre-date reconciliation and are treated as filled for backward # compatibility. rows = self.conn.execute( - "SELECT symbol, action, qty, price, timestamp, fill_qty, fill_price " + "SELECT symbol, action, qty, price, timestamp, fill_qty, " + "fill_price, fill_status " "FROM trades WHERE timestamp > datetime('now', ?) " f"AND {self._executed_trade_predicate()} " "ORDER BY timestamp", @@ -1127,7 +1153,19 @@ def compute_trade_calibration(self, lookback_days: int = 45) -> dict: open_lots[sym].append({"qty": qty, "price": price, "ts": ts}) elif (act.startswith("SELL") or act.startswith("PARTIAL_SELL") or act in ("EMERGENCY_SELL", "FORCE_DELEVER", - "REDUCE", "TAKE_PROFIT")): + "REDUCE", "TAKE_PROFIT") + or _is_filled_trail_stop(row, act)): + # A FILLED TRAIL_STOP is a realized exit — the broker sold the + # shares. Omitting it (2026-07-16 audit) left phantom open lots + # for every stop-out and no close at all: LLY BUY8 → stop-filled + # 8 → BUY6 → stop-filled 6 read as 14 shares still held and zero + # LLY trades closed, while the position was flat. The win_rate / + # avg_return / avg_hold_days this function produces feed PM as + # facts and the reviewer as calibration_note — on the real + # ledger the omission moved win_rate 22.2% → 30.0% and + # avg_return −2.79% → −2.18% for a typical window. The + # filled-guard mirrors _build_post_exit_reality: a placed-but- + # unfilled TRAIL_STOP is protection, not an exit. # Close from oldest lot first remaining = qty lots = open_lots[sym] diff --git a/tests/test_audit_fixes_2026_07_16.py b/tests/test_audit_fixes_2026_07_16.py index fd4ff8c7..d863bf07 100644 --- a/tests/test_audit_fixes_2026_07_16.py +++ b/tests/test_audit_fixes_2026_07_16.py @@ -357,3 +357,172 @@ def test_reviewer_prompt_omits_unknown_atr_rather_than_showing_zero(): position_facts={"GE": {"atr_pct": None, "stop_distance_atrs": None}}, ) assert "atr=" not in msg and "stop_distance=" not in msg + + +# ---------- calibration must count filled TRAIL_STOP exits ---------- + +def test_calibration_closes_the_lot_on_a_filled_trail_stop(tmp_path): + """A filled TRAIL_STOP is a realized exit — omitting it left a phantom + open lot for every stop-out and NO closed trade, so win_rate / + avg_return / avg_hold_days (fed to PM as facts and to the reviewer as + calibration_note) were computed off a book that never sells.""" + from src.storage.db import Database + db = Database(str(tmp_path / "t.db")) + db.initialize() + # compute_trade_calibration needs >= 3 closed trades to report. + for i, sym in enumerate(("LLY", "DXPE", "ORCL")): + db.insert_trade(symbol=sym, action="BUY", qty=8, price=1000.0, + reasoning="entry", run_id="r1", fill_status="filled") + db.insert_trade(symbol=sym, action="TRAIL_STOP", qty=8, price=1100.0, + reasoning="trail", run_id="r2", + broker_order_id=f"stop-{i}", fill_status="submitted") + db.update_trade_fill(f"stop-{i}", fill_status="filled", fill_qty=8, + fill_price=1100.0) + + calib = db.compute_trade_calibration(lookback_days=45) + assert calib.get("n") == 3, "each stop-out must close its lot" + assert calib["win_rate_pct"] == 100.0 # 1000 -> 1100 + assert calib["avg_return_pct"] == pytest.approx(10.0, abs=0.1) + + +def test_calibration_ignores_an_unfilled_trail_stop(tmp_path): + """A placed-but-unfilled stop is protection, not an exit — it must not + book a phantom close at its stop price.""" + from src.storage.db import Database + db = Database(str(tmp_path / "t.db")) + db.initialize() + for i, sym in enumerate(("GE", "XLV", "UNH")): + db.insert_trade(symbol=sym, action="BUY", qty=10, price=300.0, + reasoning="entry", run_id="r1", fill_status="filled") + db.insert_trade(symbol=sym, action="TRAIL_STOP", qty=10, price=280.0, + reasoning="trail", run_id="r2", + broker_order_id=f"live-{i}", fill_status="submitted") + # Protection sitting at the broker is not an exit — nothing closed, so the + # >=3-closed-trades floor keeps the summary empty. + assert db.compute_trade_calibration(lookback_days=45) == {} + + +# ---------- one SELL, one grade vote ---------- + +def test_grade_summary_counts_a_re_graded_sell_once(): + """evening re-grades the same trade for 2-3 consecutive nights (the + grading window has no already-graded filter), and each re-grade used to + count as an independent sell — inflating the premature/wrong counts that + drive the reviewer's patience tilt.""" + import json as _json + p = TradingPipeline.__new__(TradingPipeline) + p.db = MagicMock() + p.broker = MagicMock() + p.db.get_trades.return_value = [] + grade = {"symbol": "LLY", "sell_date": "2026-07-14", "grade": "premature"} + p.db.get_recent_insights.return_value = [ + {"date": "2026-07-16", "sell_grades_json": _json.dumps([grade])}, + {"date": "2026-07-15", "sell_grades_json": _json.dumps([grade])}, + {"date": "2026-07-14", "sell_grades_json": _json.dumps([grade])}, + ] + s = p._build_trade_grade_summary(lookback_days=14) + assert s["n_sells"] == 1 + assert s["sell_counts"]["premature"] == 1 + assert s["repeat_premature_symbols"] == [] # one sell is not a pattern + + +def test_grade_summary_still_counts_distinct_sells_of_one_symbol(): + import json as _json + p = TradingPipeline.__new__(TradingPipeline) + p.db = MagicMock() + p.broker = MagicMock() + p.db.get_trades.return_value = [] + p.db.get_recent_insights.return_value = [ + {"date": "2026-07-16", "sell_grades_json": _json.dumps([ + {"symbol": "LLY", "sell_date": "2026-07-15", "grade": "premature"}, + {"symbol": "LLY", "sell_date": "2026-06-11", "grade": "premature"}, + ])}, + ] + s = p._build_trade_grade_summary(lookback_days=14) + assert s["n_sells"] == 2 + assert s["repeat_premature_symbols"] == ["LLY"] # two real sells = a pattern + + +# ---------- same-day trim guard must fail CLOSED on a partial fill ---------- + +def test_trim_guard_blocks_after_a_partially_filled_then_canceled_reduce(): + """The shares left the book — a second trim today is the double-trim this + guard exists to prevent. Filtering on fill_status alone let it through.""" + p = TradingPipeline.__new__(TradingPipeline) + p.db = MagicMock() + p.db.get_trades.return_value = [ + {"symbol": "AMZN", "action": "REDUCE", "fill_status": "canceled", + "fill_qty": 8, "qty": 20}, + ] + assert "AMZN" in p._symbols_already_trimmed_today() + + +def test_trim_guard_still_allows_retry_after_a_zero_fill_rejection(): + p = TradingPipeline.__new__(TradingPipeline) + p.db = MagicMock() + p.db.get_trades.return_value = [ + {"symbol": "NVDA", "action": "SELL", "fill_status": "rejected", + "fill_qty": 0, "qty": 10}, + ] + assert p._symbols_already_trimmed_today() == set() + + +# ---------- queued-earnings cap must bound the RESULTING weight ---------- + +def test_queued_earnings_cap_bounds_the_resulting_weight_not_the_add(): + """A name already at 15% with an unread filing could be topped up to 20% + because the ADD itself was <= 5% — the belt capped the delta, while the + prompt/docstring promise a cap on the resulting position.""" + p = TradingPipeline.__new__(TradingPipeline) + held = Position(symbol="NKE", qty=150, avg_entry=100, current_price=100, + market_value=15_000, unrealized_pnl=0.0, + sector="Consumer Cyclical") + queued = [{"symbol": "NKE", "queued": True, "analysis": None}] + out = p._clamp_queued_earnings_buys( + [_buy("NKE", alloc=5.0)], queued, + positions=[held], total_value=100_000.0, + ) + assert out == [], "already at 15% > the 5% cap — the add must be dropped" + + +def test_queued_earnings_cap_still_allows_a_bounded_fresh_entry(): + p = TradingPipeline.__new__(TradingPipeline) + queued = [{"symbol": "NKE", "queued": True, "analysis": None}] + out = p._clamp_queued_earnings_buys( + [_buy("NKE", alloc=12.0)], queued, positions=[], total_value=100_000.0, + ) + assert len(out) == 1 and out[0].allocation_pct == 5.0 + + +def test_queued_earnings_cap_untouched_symbols_pass_through(): + p = TradingPipeline.__new__(TradingPipeline) + queued = [{"symbol": "NKE", "queued": True, "analysis": None}] + out = p._clamp_queued_earnings_buys( + [_buy("AAPL", alloc=12.0)], queued, positions=[], total_value=100_000.0, + ) + assert len(out) == 1 and out[0].allocation_pct == 12.0 + + +# ---------- credit spread: 30d must mean 30 days ---------- + +def test_credit_spread_change_is_anchored_30_days_back(): + """`series.iloc[0]` is the oldest obs in a 60-CALENDAR-day fetch, so + "change_30d_bps" was really a ~57-60 day change — ~2x the advertised + window, and on live data it flipped the sign.""" + import pandas as pd + from src.data.macro import MacroDataProvider + + # Business-daily series spanning 60 days: flat at 3.00%, then a late move. + idx = pd.bdate_range(end=pd.Timestamp("2026-07-14"), periods=42) + values = [3.00] * len(idx) + for i in range(-20, 0): + values[i] = 3.06 # +6bps only within the last ~30 days + series = pd.Series(values, index=idx) + + m = MacroDataProvider.__new__(MacroDataProvider) + m._safe_get_series = MagicMock(return_value=series) + m._staleness_days = MagicMock(return_value=1) + out = m.get_credit_spread() + # true 30d change is 0 -> +6bps; the old head-of-window read would have + # reported the full 60-day move. + assert out["change_30d_bps"] == pytest.approx(6.0, abs=0.5) diff --git a/tests/test_cash_sweep.py b/tests/test_cash_sweep.py index c43a5710..0880e159 100644 --- a/tests/test_cash_sweep.py +++ b/tests/test_cash_sweep.py @@ -375,7 +375,7 @@ def test_risk_stage_rm_view_excludes_vehicle(): p.market = MagicMock() p.market.get_ohlcv.return_value = [] p._filter_supported_symbols = MagicMock(side_effect=lambda d, a, pos: (d, [])) - p._clamp_queued_earnings_buys = MagicMock(side_effect=lambda d, e: d) + p._clamp_queued_earnings_buys = MagicMock(side_effect=lambda d, e, **kw: d) p._filter_hard_risk_decisions = MagicMock(side_effect=lambda d, *a, **k: (d, [], [])) p.risk_manager = MagicMock() p.risk_manager.review.return_value = (