From 28b3273150ba228d495c9917aeb2eae75b655576 Mon Sep 17 00:00:00 2001 From: Dennis Thiessen Date: Tue, 11 Aug 2026 23:24:58 +0200 Subject: [PATCH] fix(backtest): quote one book, not two MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The recommendation's "Book vs SPY" line read from portfolio_sim — the hold/target policy book — while the tiles directly above read from portfolio_monitor, the production ATR-trail book. Same SPY figure, different portfolio return, on one screen. It now reads the same production row the tiles do. Also removed, for the same reason: the "legacy exit diagnostic" comparing hold against the S/R target. Both are exits the production book replaced, so a recommendation between them could not lead to an action. And the fallback headline, which advised the fixed-hold exit whenever a report had no production row — a report that cannot describe the production baseline now states none. portfolio_sim stays in the report payload: scripts/run_backtest_snapshot.py and reports/compare_reports.py read it, and it is no longer surfaced in the UI. The test fixture now carries a production monitor whose numbers differ from its policy sim, so re-sourcing that line from the old place fails rather than passes unnoticed. Co-Authored-By: Claude Opus 5 --- app/services/backtest_service.py | 63 ++++++++--------------------- tests/unit/test_backtest_service.py | 41 ++++++++++++++++++- 2 files changed, 56 insertions(+), 48 deletions(-) diff --git a/app/services/backtest_service.py b/app/services/backtest_service.py index 43c7818..5cc6689 100644 --- a/app/services/backtest_service.py +++ b/app/services/backtest_service.py @@ -3946,38 +3946,12 @@ def _build_recommendation(report: dict) -> dict: q = report.get("overall_qualified") or {} target_net = q.get("net_avg_r") - # Legacy diagnostic: target/stop race vs the best fixed hold. + # The best fixed hold, kept ONLY to pick the basis for the robustness check + # below. The hold-vs-target comparison itself is deliberately not reported: + # both are legacy exits the production book replaced with the ATR trail, so + # a recommendation between them could not lead to an action. time_rows = [r for r in report.get("time_exit_sweep") or [] if r.get("net_avg_r") is not None] best_hold = max(time_rows, key=lambda r: r["net_avg_r"], default=None) - sim_rows = { - p.get("policy"): p - for p in (report.get("portfolio_sim") or {}).get("policies", []) - } - hold_sim = sim_rows.get("hold") - if best_hold is not None and target_net is not None: - if best_hold["net_avg_r"] > target_net + _EXIT_SWITCH_THRESHOLD: - text = ( - f"Legacy exit diagnostic: hold {best_hold['hold_days']} trading days with the initial stop " - f"({best_hold['net_avg_r']:+.2f}R net/trade vs {target_net:+.2f}R for the S/R target exit)." - ) - target_sim = sim_rows.get("target") - if ( - hold_sim is not None and target_sim is not None - and hold_sim.get("cagr_pct") is not None and target_sim.get("cagr_pct") is not None - ): - text += ( - f" The simulated book agrees: {hold_sim['cagr_pct']:+.1f}% vs " - f"{target_sim['cagr_pct']:+.1f}% CAGR at similar drawdown." - ) - items.append({"topic": "exit", "text": text}) - else: - items.append({ - "topic": "exit", - "text": ( - f"Legacy exit diagnostic: keep the S/R target exit ({target_net:+.2f}R net/trade) — " - "no fixed hold beats it by a meaningful margin." - ), - }) # Gate floors, judged under the hold exit (the ablation's Hold column). ablation = {r["variant"]: r for r in report.get("gate_ablation") or []} @@ -4025,17 +3999,20 @@ def _build_recommendation(report: dict) -> dict: ), }) - # Book vs benchmark. - book = hold_sim or sim_rows.get("target") - if book is not None and book.get("spy_return_pct") is not None: - edge = book["total_return_pct"] - book["spy_return_pct"] + # Book vs benchmark — read from the SAME production monitor row the page + # shows in its tiles. It used to read the hold/target policy sim, so the + # recommendation quoted a different portfolio return than the tile directly + # above it, against an identical SPY figure. Those policies are legacy + # diagnostics; the production book is the ATR trail. + if production_row is not None and production_row.get("spy_return_pct") is not None: + edge = production_row["total_return_pct"] - production_row["spy_return_pct"] verdict = "beats" if edge > 0 else "LAGS" items.append({ "topic": "benchmark", "text": ( f"Book vs SPY: {verdict} buy-and-hold by {edge:+.1f} points " - f"({book['total_return_pct']:+.1f}% vs {book['spy_return_pct']:+.1f}%), " - f"max drawdown −{book['max_drawdown_pct']:.1f}%." + f"({production_row['total_return_pct']:+.1f}% vs " + f"{production_row['spy_return_pct']:+.1f}%)." ), }) @@ -4072,16 +4049,10 @@ def _build_recommendation(report: dict) -> dict: ), }) - if headline is None and hold_recommended: - cagr_note = ( - f" (~{hold_sim['cagr_pct']:.0f}% CAGR simulated)" - if hold_sim is not None and hold_sim.get("cagr_pct") is not None - else "" - ) - headline = ( - f"Trade the qualified list long-only; hold {best_hold['hold_days']} trading days " - f"with the initial ATR stop{cagr_note}." - ) + # No fallback headline. It used to recommend the fixed-hold exit whenever the + # portfolio monitor was missing, which meant a report without a production + # row advised an exit the production book had already replaced. A report that + # cannot describe the production baseline states no baseline. return { "headline": headline, diff --git a/tests/unit/test_backtest_service.py b/tests/unit/test_backtest_service.py index 43377a4..cae841b 100644 --- a/tests/unit/test_backtest_service.py +++ b/tests/unit/test_backtest_service.py @@ -1272,20 +1272,36 @@ def test_build_recommendation_reads_the_report(): {"min_momentum_percentile": 60.0, "net_avg_r": 0.05, "total": 300}, {"min_momentum_percentile": 0.0, "net_avg_r": -0.12, "total": 1000}, ], + # Legacy policy book. Its numbers are deliberately DIFFERENT from the + # production monitor's below, so sourcing the benchmark line from here + # again would fail the assertion rather than pass unnoticed. "portfolio_sim": {"policies": [ {"policy": "target", "cagr_pct": 23.7, "total_return_pct": 134.8, "spy_return_pct": 95.9, "max_drawdown_pct": 20.7}, {"policy": "hold", "cagr_pct": 31.9, "total_return_pct": 203.6, "spy_return_pct": 95.9, "max_drawdown_pct": 21.2}, ]}, + "portfolio_monitor": { + "production_strategy": "prod", + "runs": [{ + "strategy": "prod", "lookback": "all", "lookback_label": "All history", + "cagr_pct": 40.0, "sharpe": 1.72, "max_drawdown_pct": 17.7, + "total_return_pct": 297.8, "spy_return_pct": 101.9, + }], + }, } rec = bt._build_recommendation(report) by_topic: dict[str, list[str]] = {} for item in rec["items"]: by_topic.setdefault(item["topic"], []).append(item["text"]) - assert rec["headline"] is not None and "hold 30" in rec["headline"] - assert any("hold 30 trading days" in t for t in by_topic["exit"]) + assert rec["headline"] is not None and "Production baseline" in rec["headline"] + # The hold-vs-target comparison is gone: both are exits the production book + # replaced, so a recommendation between them cannot lead to an action. + assert "exit" not in by_topic + # Benchmark must quote the SAME row the page's tiles show, not the policy sim. + assert "+297.8%" in by_topic["benchmark"][0] + assert "203.6" not in by_topic["benchmark"][0] gate_texts = " | ".join(by_topic["gate"]) assert "confidence floor adds nothing" in gate_texts assert "keep the R:R floor" in gate_texts @@ -1810,3 +1826,24 @@ class TestPortfolioQualityMetrics: for key in ("sortino", "gain_to_pain", "profit_factor"): assert key in sim assert sim["sortino"] is None + +def test_build_recommendation_states_no_baseline_without_a_production_row(): + """A report with no portfolio monitor cannot describe the production book. + It used to fall back to recommending the fixed-hold exit — advice for a model + the production book had already replaced.""" + report = { + "overall_qualified": {"net_avg_r": 0.13, "net_avg_r_ex_top5": 0.05}, + "time_exit_sweep": [{"hold_days": 30, "net_avg_r": 0.50, "net_avg_r_ex_top5": 0.21}], + "portfolio_sim": {"policies": [ + {"policy": "hold", "cagr_pct": 31.9, "total_return_pct": 203.6, + "spy_return_pct": 95.9, "max_drawdown_pct": 21.2}, + ]}, + } + rec = bt._build_recommendation(report) + topics = {item["topic"] for item in rec["items"]} + + assert rec["headline"] is None + # Nothing may be sourced from the legacy policy book. + assert "benchmark" not in topics + assert "exit" not in topics +