Fix backtest rigor and safety gaps (lookback guard, metrics inf, breakout look-ahead) - #18
Merged
Merged
Conversation
- lookback_guard: enforce_lookback now raises InsufficientWarmupError instead of only logging, and is actually applied to RSI/ATR feature functions (previously dead code, applied nowhere) - runner: warmup check failures now block the affected asset's backtest instead of being caught and logged as a warning - runner: fix docstring claiming square-root market impact when the implementation is flat linear bps - strategies: BreakoutStrategy now excludes the current bar from its rolling high/low so breakout levels reflect genuine prior extremes - metrics: Calmar/Sortino return a finite MAX_RATIO sentinel instead of inf on near-zero drawdown/downside, so they're safe to sort/rank/persist - metrics: bootstrap_sharpe uses a moving-block bootstrap instead of an IID resample, preserving autocorrelation for trend/momentum strategies - base_planner: max_retries is now honored by the raw Gemini and OpenAI planner paths (previously only the LangChain path used it), and API key validation rejects malformed/whitespace keys, not just known placeholders - proposal_generator/agent_graph: reflect/hypothesize loop now feeds prior-iteration results back into the LLM prompt so retries reason about what already underperformed within the same run All 63 existing tests pass unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes found during a code review of the backtest engine, feature guards, and agent loop:
lookback_guardwas advisory-only, despite the README's anti-look-ahead-bias claim.enforce_lookbackonly logged a warning and was applied nowhere;WarmupEnforcer's raisedInsufficientWarmupErrorwas caught and logged inrunner.pyrather than blocking the backtest. Now the decorator raises and is applied to RSI/ATR, and a warmup failure excludes that asset from the run instead of silently proceeding.BreakoutStrategycompared today's close against a rolling high/low that included today's own bar. Now usesshift(1)so the breakout level reflects genuinely prior extremes.Calmar/Sortinoreturnedfloat("inf")on near-zero drawdown/downside, which is a landmine for any future ranking/sorting by these metrics. Now clipped to a finiteMAX_RATIOsentinel.bootstrap_sharpeused a naive IID resample, destroying autocorrelation — understates uncertainty for the momentum/trend strategies this repo mostly trades. Switched to a moving-block bootstrap.max_retrieswas only honored by the LangChain planner path; raw Gemini/OpenAI planners ignored it. Added a shared retry helper used by both, plus tighter API-key validation (rejects malformed/whitespace keys, not just known placeholder strings).agent_graphnow accumulatesall_resultsacross iterations and passes them into the proposal prompt.Test plan
pytest -q— all 63 existing tests pass unchangedruff checkon all touched files — clean