feat(backtest): require evidence-bound executable L2 replay - #65
Conversation
|
Warning Review limit reached
Next review available in: 58 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (19)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb1b763027
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if self.strategy.volume_factor > 0.0 || self.strategy.cvd_threshold != 0.0 { | ||
| bail!("strategy requires trade flow but dataset manifest is LOB-only"); |
There was a problem hiding this comment.
When using the checked-in config/backtest/default.yaml with --grid config/backtest/param_grid.yaml, every planned run sets volume_factor to 0.8/1.0/1.2, and sweep::run reaches this validation in both dry-run and execution, so the stock sweep aborts with strategy requires trade flow before any backtest runs. Either update the grid to sweep only LOB-compatible values or provide a trade-flow manifest for those combinations.
Useful? React with 👍 / 👎.
| assert!(Command::new("zstd") | ||
| .args(["-q", "-f"]) | ||
| .arg(&raw_path) | ||
| .arg("-o") | ||
| .arg(&compressed_path) | ||
| .status() | ||
| .unwrap() |
There was a problem hiding this comment.
In environments where the zstd binary is not installed, the newly added compression tests panic at status().unwrap() before exercising the backtest validation logic, so cargo test -p hft-backtest fails for a missing optional system tool. Guard these tests the same way the existing collector zstd tests do, or use an in-process encoder for the fixture.
Useful? React with 👍 / 👎.
| let artifact_rows = parse_ndjson_values(artifact_bytes, "backtest event tape")?; | ||
| let mut materialized_rows = Vec::with_capacity(artifact_rows.len()); |
There was a problem hiding this comment.
For realistic L2 backtest artifacts, this builds both artifact_rows and materialized_rows as full Vec<serde_json::Value> collections before replay starts, on top of the earlier fs::read of the entire artifact. A multi-GB tape can therefore OOM in --dry-run or normal backtests even though the previous runner streamed events; compare rows incrementally while hashing/materializing instead of retaining both tapes.
Useful? React with 👍 / 👎.
Change contract
Make a backtest result executable only when it is deterministically materialized from verified, sequence-safe Binance L2 collector evidence. Bind source segments, manifests, success markers, configuration, and result artifacts by SHA-256. The collector materializer and backtest use one shared replay validator in
data-pipelines/core.Out of scope
No live execution, account reconciliation, risk policy, readiness endpoint, alpha promotion, or market-data adapter rollout change.
Dependencies and merge order
Independent; may merge directly to
main. It deliberately does not depend on #59 because it validates governed Binance L2 evidence rather than enabling the Prediction venue adapter.Focused validation
cargo test -p hft-datacargo test -p hft-backtestcargo test -p hft-collector --libcargo test -p hft-collector --bin lob-pit-materializercargo clippy -p hft-data -p hft-backtest --all-targets --no-deps -- -D warningscargo tree -p hft-backtest -i hft-collectoris absent (verified)Rollout / rollback
Backtest-only, fail closed: malformed, stale, unbound, or non-deterministic input is rejected before execution. Revert this PR to restore the prior non-executable replay path.
Scope exception
20 files / 2,321 added lines exceed the normal split-assessment threshold, but this is one atomic trust contract: the raw collector evidence, PIT materializer, shared replay validator, and executable backtest must agree on the same schema and sequence semantics. Merging a subset would either publish unconsumed evidence or create a backtest path that cannot verify the evidence it executes. Named reviewer approval required before ready-for-review:
proerror77.