CEX: admit immutable Binance replay snapshots - #406
Conversation
📝 WalkthroughWalkthroughChangesCEX replay snapshot admission
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Collector
participant MaterializationReport
participant MissionRunner
participant AlphaHarness
participant AlphaStore
Collector->>MaterializationReport: publish validated snapshot and digest
MissionRunner->>MaterializationReport: validate snapshot lineage and hashes
MissionRunner->>AlphaHarness: admit CEX replay dataset
AlphaHarness->>AlphaStore: register dataset revision linked to features
AlphaHarness-->>MissionRunner: return dataset manifest
MissionRunner->>AlphaHarness: execute mission with dataset manifest
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 8dbe45bbc0
ℹ️ 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".
06da643 to
4846f6f
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
rust_hft/alpha-harness/app/src/mission_runner.rs (1)
659-670: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRe-sign the snapshot so this test isolates the modality gate.
Setting
snapshot_sha256to an all-zero digest makes the manifest invalid in two independent ways. The assertion currently passes only becausesnapshot.validate()runs before the digest comparison invalidate_materialization. Usingresign_materialization(as the sibling tests do) proves the stronger property: a correctly signed snapshot missingaggregate_tradeis still rejected.🧪 Proposed change
- materialization["snapshot_sha256"] = serde_json::json!("0".repeat(64)); - std::fs::write( - &fixture.materialization_path, - serde_json::to_vec_pretty(&materialization).unwrap(), - ) - .unwrap(); - let mut args = fixture.args; - args.materialization_sha256 = sha256_file(&fixture.materialization_path).unwrap(); - - let error = execute(args).unwrap_err(); + let mut fixture = fixture; + fixture.materialization = materialization; + resign_materialization(&mut fixture); + + let error = execute(fixture.args.clone()).unwrap_err();Note
resign_materializationdeserializes the snapshot intoCexReplaySnapshotV1to hash it, which works here because the replacement JSON is still structurally valid for that type.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust_hft/alpha-harness/app/src/mission_runner.rs` around lines 659 - 670, Update the test around execute to replace the snapshot_sha256 zeroing and manual file write with the existing resign_materialization helper, preserving the removal of aggregate_trade and recomputing args.materialization_sha256 afterward. Keep the assertion that execute rejects the correctly signed snapshot with the “required modalities” error.rust_hft/research-core/manifest/src/lib.rs (1)
430-472: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a direct counterexample for the range-containment branch.
Digest determinism, segment ordering, and dataset identity are covered, but
"event range is outside source segments"— the branch that blocks a snapshot claiming events outside its authenticated tape — has no unit-level counterexample here. It's cheap to pin down at this layer rather than relying on the mission-runner integration tests.As per coding guidelines, "Safety boundaries require targeted counterexample tests, not only workspace compilation."
🧪 Proposed test
+ #[test] + fn cex_replay_snapshot_rejects_events_outside_source_segments() { + let mut snapshot = cex_snapshot(); + snapshot.last_event_time = DateTime::parse_from_rfc3339("2026-07-14T00:00:20Z") + .unwrap() + .with_timezone(&Utc); + + assert_eq!( + snapshot.validate().unwrap_err(), + ManifestError::InvalidCexReplaySnapshot("event range is outside source segments") + ); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust_hft/research-core/manifest/src/lib.rs` around lines 430 - 472, Add a focused unit test alongside the existing CEX replay snapshot validation tests that modifies the snapshot’s event range to extend beyond its authenticated source segment bounds, then asserts validate() returns ManifestError::InvalidCexReplaySnapshot("event range is outside source segments"). Keep the test limited to exercising this range-containment branch.Source: Coding guidelines
rust_hft/tools/collector/Cargo.toml (1)
38-38: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDeclare
hft-research-manifestthrough the workspace dependency table.
rust_hft/Cargo.tomldefines[workspace.dependencies], while this crate and several adjacent crates use a direct{ path = ... }forhft-research-manifest. Use{ workspace = true }from the workspace manifest so dependency graph changes remain centralized. Includecargo metadata --locked --no-depsas validation evidence for this workspace-graph change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust_hft/tools/collector/Cargo.toml` at line 38, Update the hft-research-manifest dependency declaration in the collector crate to use the workspace dependency table via workspace = true instead of a direct path. Ensure the dependency is defined centrally in rust_hft/Cargo.toml’s [workspace.dependencies], then validate the workspace graph with cargo metadata --locked --no-deps.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@rust_hft/alpha-harness/app/src/mission_runner.rs`:
- Around line 659-670: Update the test around execute to replace the
snapshot_sha256 zeroing and manual file write with the existing
resign_materialization helper, preserving the removal of aggregate_trade and
recomputing args.materialization_sha256 afterward. Keep the assertion that
execute rejects the correctly signed snapshot with the “required modalities”
error.
In `@rust_hft/research-core/manifest/src/lib.rs`:
- Around line 430-472: Add a focused unit test alongside the existing CEX replay
snapshot validation tests that modifies the snapshot’s event range to extend
beyond its authenticated source segment bounds, then asserts validate() returns
ManifestError::InvalidCexReplaySnapshot("event range is outside source
segments"). Keep the test limited to exercising this range-containment branch.
In `@rust_hft/tools/collector/Cargo.toml`:
- Line 38: Update the hft-research-manifest dependency declaration in the
collector crate to use the workspace dependency table via workspace = true
instead of a direct path. Ensure the dependency is defined centrally in
rust_hft/Cargo.toml’s [workspace.dependencies], then validate the workspace
graph with cargo metadata --locked --no-deps.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 743e6f62-e33a-4bf1-a193-be8f8b3a30fd
⛔ Files ignored due to path filters (1)
rust_hft/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
rust_hft/alpha-harness/app/src/data_mission.rsrust_hft/alpha-harness/app/src/mission_runner.rsrust_hft/research-core/manifest/Cargo.tomlrust_hft/research-core/manifest/src/lib.rsrust_hft/tools/collector/Cargo.tomlrust_hft/tools/collector/src/bin/lob-pit-materializer.rs
4846f6f to
5832fdc
Compare
Closes #400
Change contract
Admit one existing authenticated Binance combined market-tape replay as a versioned immutable CEX snapshot whose digest becomes the alpha-harness mission dataset identity, and fail closed before evaluation when modality, lineage, digest, sequence, range, or point-in-time availability evidence is invalid.
Out of scope
Dependency or merge order
None. This branch is based on
0cf2cdadb0ebcb65209e11b595059fcd8a16a18eand can merge independently. Polymarket #324 merged separately through PR #405 and is now in the base. #401 depends on this issue; #402 depends on #401.Focused validation
cargo test -p hft-research-manifest— 9 passedcargo test -p hft-collector --bin lob-pit-materializer— 7 passedcargo test -p alpha-harness— 118 passed5832fdc355147f75f820cb289afda800b4a526a8.rustfmt --checkandgit diff --checkpassed.Codex standards reviewer /root/standards_review_400: these files form one fail-closed authenticated-tape → immutable-snapshot → CEX-admission → mission-dataset contract; splitting would create an unsafe producer/consumer schema gap and coupled rollback order.Rollout or rollback impact
Research-only. No deployment, collector cutover, publication, promotion, or live execution is performed. The producer and consumer move together to
binance-lob-pit-v2; rollback is a single PR revert. Existing unrelated research paths remain unchanged.