fix(backtest): freeze cost book, rank on train, refuse live-go - #179
Conversation
Close holdout ranking leaks and silent cost mutation without a new engine. WFO validation of a fixed config stays on experiment_autopilot. Co-authored-by: Yderf <Trujillofa@users.noreply.github.com>
Retest after mergeDo not retest frozen autopilot / A locked Do re-run Leave |
Disjoint calendar WFO windows, persist trades_fingerprint without changing run_id, reject unknown timeframes before data fetch. Do not merge.
Translate half-open WFO window ends to inclusive reader bounds so a bar at train_end/test_start is fetched only in the test dataset.
Trujillofa
left a comment
There was a problem hiding this comment.
Review: approve with nits
Verified locally on 446e9ae (fresh worktree): 1342 passed in 33s, ruff check / ruff format --check clean, CI green on all three jobs, mergeable_state: clean.
The core holds up. I traced every cost call site in the engine — _calculate_entry_qty, _cap_fixed_notional, _apply_quantity_step, _open_long / _open_short, the exit path, and funding all read self._cost_book, with no self._config.fee_rate / slippage_pct reads left behind. Frozen BacktestConfig has no remaining mutation site anywhere in src/, scripts/, or tests/ except the adversarial one at tests/test_backtest_quality_bar.py:211. The shared timeframes.py genuinely kills the Sharpe-inflation bug — _TIMEFRAME_MINUTES.get(config.timeframe, 1) was annualizing an unknown label at 525,600 periods/year, and run() now validates the label (plus both MTF legs) before any bars are read. fee_rate=None correctly falls through to REALISTIC_FEE_RATE in the factory, so dropping the 0.001 default in run_backtest.py routes through the single source rather than a second one.
The diagnosis in the description matches what's in the diff — the leaks were around the simulator, not in it.
Findings
- The live-go refusal is unreachable on 5 of the 6 wired CLIs (
scripts/run_backtest.py:94).refuse_live_goruns afterparse_args(), so argparse exits 2 first; I reproduced it. Only the env-var arm can actually fire. The safety outcome is unchanged, but the guard and its message are dead code as wired, and this doc's "Passing one is refused" is true only by accident. Inline comment has the fix. - The ranking key isn't printed (
scripts/run_config_search.py:903,scripts/run_mtf_search.py:570). Both printers sort onselection_sharpeand show every column except that one. passes_gatesdropped out of the sort order — same thread. Probably correct (gates read OOS), but it's a triage regression worth making deliberate.- One-window selector (
docs/BACKTEST_AND_WFO.md:52). Ranking onwindows[0]train only trades the leak for a variance problem; averaging train metrics across windows is equally leak-free. scripts/run_wfo_sweep.pyshould be deleted, not stubbed — nothing importswfo_sweepor this module'sparse_backtest_output. Step 2 of the framework.- Two source-text-grep tests are maintenance traps (
tests/test_backtest_quality_bar.py:422) — banningtimedelta(days=across two large scripts, and asserting exact call-site strings thatruff formatcould rewrap. - Nit: the two search scripts build the same ranking two different ways, and one silently dedupes by name.
Only #1 is worth fixing before merge; the rest are follow-ups.
Cost/comparability note
Backing your own "Retest after merge" comment: spot run_backtest.py moves 0.001 → 0.0004 and both search scripts drop their hard-coded 0.001. Any spot number produced before this lands is not comparable to one produced after — your re-run guidance for run_config_search / run_mtf_search is the right scope.
Also worth knowing before the first search run: _evaluate_candidate now runs an extra backtest per candidate for the selection window, so a sweep of N candidates goes from N×(1+W) runs to N×(2+W). Correct and worth paying — just not free on wall-clock.
Generated by Claude Code
refuse_live_go ran after parse_args(), so argparse rejected an unknown --live and exited 2 before the guard could fire. LiveGoRefused never raised on any of the five wired CLIs; the flags=vars(args) arm was dead too, since no script defines a live/live_go/promote dest. Only the CRYPTO_AGENT_LIVE_GO env var could trigger it. Guard raw argv before parsing (the ordering run_full_backtest.py already used), then re-check the parsed flags. run_wfo.py additionally could not start: this branch added the first `from src...` import to it, but unlike its siblings it never appended the repo root to sys.path, so it died with ModuleNotFoundError before reaching main. Append cwd the way run_backtest.py and the search scripts do. Cover it with a test that invokes each CLI in a real process and asserts on the refusal message. The existing unit test passed the whole time the guard was dead, so only execution proves it is reachable. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H6UGe3bi1PjPgz2MqYrLwu
Both search printers order rows by selection_sharpe but showed every column except that one, so the visible return/wfo_sharpe fields no longer explained the order and the list read as unsorted. The value was only recoverable from the CSV. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H6UGe3bi1PjPgz2MqYrLwu
|
Pushed two commits addressing findings 1 and 2 from my review (
|
Summary
Surgical quality-bar pass on this repo's existing backtest / WFO / sweep stack. No shared engine, no new strategies, no retunes, no live-go.
Hypothesis was mostly right:
src/backtest/engine.pyis already the structured simulator. The leaks were around it — mutable costs, search ranking on holdout, inconsistent fees, a broken param sweep, and no adversarial tests for peek / mutation / live flags.Gaps found
CostProfilewas frozen, butBacktestConfigwas mutable.test_backtest_atr.pyeven swappedstrategy_classesafter engine construction. Search scripts hard-codedfee_rate=0.001(legacy) while the factory default is 0.0004.run_backtest.pysilently used 0.001 on spot when--feewas omitted.run_config_search.pyandrun_mtf_search.pysorted by WFO OOS + full-period return.run_wfo_sweep.pynever appliedparam_gridand ranked on test Sharpe.--live/live_go/promotewas smuggled in._TIMEFRAME_MINUTES.get(..., 1)), which would inflate Sharpe. Cost funding used a second map.What changed
BacktestConfig+ engineCostBooksnapshot (fee, slip, funding, size). Forced mutation after init does not change fills. Units documented (fractions of notional/price; slip is spread+slip).src/backtest/timeframes.pyclock contract: bartimeis open; unknown TF raises.rank_by_selection_score: ranking uses first-train-window metrics only. Test asserts swapping holdout scores does not change order. Wired into config/MTF search printers.refuse_live_goon canonical research CLIs.run_wfo_sweep.pynow errors: it is not a selection tool.tradeslist (keep counts/metrics).run_backtest.pyuses factory cost defaults unless--feeis explicit.docs/BACKTEST_AND_WFO.md(how to run; not a live-go). Pointers inEXPERIMENT_AUTOPILOT.mdandRESEARCH_FRAMEWORK.md.tests/test_backtest_quality_bar.py.What was left alone (and why)
execution_parity_v2causality — already closed-bar / next-open; existing tests are solid. Did not changerun_backtest.py's defaultlegacy_v1(reproducibility of old CLI runs). Autopilot already defaults to v2.experiment_autopilotgates — single pre-specified config; OOS Sharpe/return is the point of validation. Full-period drawdown/bootstrap left as-is to avoid retuning gate outcomes.run_wfo.py— OOS eval of a frozen config, not a ranker. Only added live-go refuse.autoresearch.py,mtf_*.py,run_full_backtest.pyfee default) — not the canonical path; not rewritten.Type of Change
Testing
ruff check+ruff format --checkon touched files