fix(alpha): bind evaluation protocol evidence - #46
Conversation
|
Warning Review limit reached
Next review available in: 54 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 selected for processing (17)
📝 WalkthroughWalkthroughThe change introduces a versioned evaluation protocol, derives it from CLI validation settings, propagates it through dataset and evaluator flows, and binds its hash into candidate evidence, sealed evaluations, strategy bundles, and promotion records. ChangesEvaluation protocol binding
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as ValidationArgs
participant Dataset as PreparedDataset
participant Evaluator as FormulaEvaluator
participant Evidence as CandidateEvaluation
participant Store
participant Bundle as StrategyBundle
CLI->>Dataset: create protocol and prepare dataset
Dataset->>Evaluator: provide protocol and rows
Evaluator->>Evidence: emit evaluation and protocol hash
Store->>Evidence: validate canonical walk-forward evidence
Store->>Bundle: require matching evaluation_protocol_hash
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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.
🧹 Nitpick comments (1)
rust_hft/alpha-harness/engine/src/evaluation.rs (1)
184-194: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDead re-check + misleading error message after
protocol.validate().
protocol.validate()(domain/src/lib.rs) already rejects zeroinitial_train_rows/validation_rows/fold_count/sealed_holdout_rowsas part of its own||chain, so by the time control reaches Line 188 these four conditions can never be true — the explicit re-check is dead code. Additionally,map_err(|_| EvaluationError::InvalidConfiguration)discards the actualDomainErrorreason, so a protocol rejected for a negative cost, zero label horizon, or version mismatch is reported with the walk-forward-specific message "walk-forward configuration must use positive train, validation, holdout, and fold sizes", which is misleading for debugging.♻️ Proposed cleanup
protocol .validate() .map_err(|_| EvaluationError::InvalidConfiguration)?; let config = &protocol.walk_forward; - if config.initial_train_rows == 0 - || config.validation_rows == 0 - || config.fold_count == 0 - || config.sealed_holdout_rows == 0 - { - return Err(EvaluationError::InvalidConfiguration); - } if rows.iter().any(|row| {Consider also giving
EvaluationError::InvalidConfigurationa payload (or adding a dedicated variant) so the underlyingDomainErrormessage isn't discarded, e.g.InvalidConfiguration(String)populated fromerror.to_string().🤖 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/engine/src/evaluation.rs` around lines 184 - 194, Remove the redundant zero-value check after protocol.validate() in the evaluation flow, including the walk-forward-specific error return. Preserve the validation error’s underlying DomainError details by updating the protocol.validate() map_err handling and, if needed, the EvaluationError::InvalidConfiguration definition and its consumers to carry or otherwise expose the original message.
🤖 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/engine/src/evaluation.rs`:
- Around line 184-194: Remove the redundant zero-value check after
protocol.validate() in the evaluation flow, including the walk-forward-specific
error return. Preserve the validation error’s underlying DomainError details by
updating the protocol.validate() map_err handling and, if needed, the
EvaluationError::InvalidConfiguration definition and its consumers to carry or
otherwise expose the original message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b4ec5698-68e6-4449-9bc0-48ad6e5a30f3
📒 Files selected for processing (17)
rust_hft/alpha-harness/README.mdrust_hft/alpha-harness/app/src/cli.rsrust_hft/alpha-harness/app/src/governance.rsrust_hft/alpha-harness/app/src/loop_control.rsrust_hft/alpha-harness/app/src/mission.rsrust_hft/alpha-harness/app/src/mission_runner.rsrust_hft/alpha-harness/domain/src/lib.rsrust_hft/alpha-harness/engine/src/engines/bayesian.rsrust_hft/alpha-harness/engine/src/engines/mcts.rsrust_hft/alpha-harness/engine/src/engines/mod.rsrust_hft/alpha-harness/engine/src/evaluation.rsrust_hft/alpha-harness/engine/src/formula_evaluator.rsrust_hft/alpha-harness/engine/src/lib.rsrust_hft/alpha-harness/onnx-evaluator/src/lib.rsrust_hft/alpha-harness/store/src/lib.rsrust_hft/apps/live/tests/deployment_artifacts.rsrust_hft/apps/live/tests/deployment_envelope.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b45e92ede
ℹ️ 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".
| .sum::<f64>() | ||
| / self.metrics.folds.len().max(1) as f64; | ||
| if self.evaluator_version.trim().is_empty() | ||
| if self.protocol_binding().is_err() |
There was a problem hiding this comment.
When resuming a mission that already has purged-walk-forward-v3 evaluations written before this change, serde defaults both new protocol fields to None, and AutoResearchKernel::historical_evaluation still calls validate() for non-v2 observations. This new unconditional protocol check therefore makes mission run/resume fail before proposing any new candidates, even though legacy records only need to be replayed for search state and should fail closed for validation/promotion; please tolerate or skip protocol-less legacy observations in the historical replay path.
Useful? React with 👍 / 👎.
| protocol | ||
| .validate() | ||
| .map_err(|_| EvaluationError::InvalidConfiguration)?; |
There was a problem hiding this comment.
For standalone mission run/evaluate on OHLCV or imported feature manifests, the loaded ResearchRows do not carry any horizon or bucket-frequency metadata (the OHLCV loader derives a one-bar-ahead label), so accepting any protocol after only protocol.validate() lets evidence be minted with arbitrary label_horizon_buckets and observation_frequency_millis values that do not describe the actual labels. This undermines the new protocol hash for label definitions outside the durable materialization path; please verify the label spec against dataset metadata or reject datasets whose label spec cannot be proven.
Useful? React with 👍 / 👎.
| if protocol.content_hash()? != protocol_hash { | ||
| return Err(DomainError::InvalidEvaluationEvidence); | ||
| } |
There was a problem hiding this comment.
If a malformed or hand-registered evaluation payload reaches the store, validate() now checks that the protocol hash matches the embedded protocol but never checks that the metrics were actually produced with that protocol’s split sizes. A walk-forward payload with two 30-row folds can therefore claim a protocol with a different fold_count or validation_rows and still be accepted by promotion as long as the sealed payload uses the same hash; please reject protocol/metric dimension mismatches in CandidateEvaluation::validate().
Useful? React with 👍 / 👎.
| #[test] | ||
| fn bundle_staging_cleans_up_on_persistence_failure_without_partial_output() { | ||
| let (promotion, bundle) = promotion_fixture(); | ||
| let directory = std::env::temp_dir().join(format!( |
| #[test] | ||
| fn conflicting_bundle_output_is_rejected_before_any_overwrite() { | ||
| let (_, bundle) = promotion_fixture(); | ||
| let directory = std::env::temp_dir().join(format!( |
| let mut second = first.clone(); | ||
| second.candidate_content_hash = "f".repeat(64); | ||
| second.bundle_hash = second.calculated_hash().unwrap(); | ||
| let directory = std::env::temp_dir().join(format!( |
| let model_bytes = b"shared-onnx-model"; | ||
| let (first, second) = conflicting_onnx_bundles_for_shared_model(model_bytes); | ||
|
|
||
| let directory = std::env::temp_dir().join(format!( |
| fn bundle_conflict_keeps_a_shared_model_referenced_by_the_winner() { | ||
| let model_bytes = b"shared-onnx-model"; | ||
| let (first, second) = conflicting_onnx_bundles_for_shared_model(model_bytes); | ||
| let directory = std::env::temp_dir().join(format!( |
20ad1cd to
dcf002f
Compare
dcf002f to
e86d4b1
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e86d4b1aa7
ℹ️ 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".
| &candidate_iteration, | ||
| &candidate_artifact, | ||
| )? | ||
| || walk_forward_protocol_hash.as_deref() != Some(sealed_protocol_hash) |
There was a problem hiding this comment.
When a store has matching walk-forward and sealed evaluations but no mission_evaluation_protocol registry row (for example, evidence imported or written before the new binding was frozen), this helper still returns the candidate because it only compares the two evidence hashes to each other. progress_completed_mission uses this result to mark HoldoutPassed, so a LoopRun can advance on evidence that promote_candidate now correctly rejects as missing the immutable mission protocol binding. Please also require the mission binding hash to exist and equal sealed_protocol_hash here.
Useful? React with 👍 / 👎.
| if materialization.bucket_ms != validation.observation_frequency_millis | ||
| || materialization.label_horizon_buckets != validation.label_horizon_buckets | ||
| { | ||
| bail!("evaluation label horizon or frequency does not match the materialization"); |
There was a problem hiding this comment.
For mission execute with a materialization horizon larger than the default purge (e.g. the LOB materializer default label_horizon_buckets = 5 while --purge-rows defaults to 1), this check passes because it only compares the supplied label facts, but ValidationArgs::evaluation_protocol later rejects purge_rows < horizon_buckets. By then execute has already imported the feature manifest and created the research mission in the result DB, so a bad protocol leaves partial artifacts instead of failing up front; construct/validate the EvaluationProtocolV1 here before any store writes.
Useful? React with 👍 / 👎.
Outcome
Binds every walk-forward, sealed-holdout, promotion, and strategy-bundle artifact to one versioned evaluation protocol hash so IC/RankIC/ICIR/RankICIR and post-cost Sharpe evidence cannot be reused under different splits, costs, label horizons, or metric definitions.
What changed
EvaluationProtocolV1covering walk-forward split/purge/embargo/holdout, fee/funding/latency costs, label horizon/frequency, and ICIR/Sharpe definitionsVerification
cargo test -p alpha-domain --locked— 32 passedcargo test -p alpha-engine --no-default-features --locked— 41 passed, 1 ignored live testgit diff --check— passedFull store/app/ONNX/live verification is intentionally delegated to GitHub CI / the Aliyun research plane to avoid a local bundled-DuckDB rebuild.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation