feat(alpha): harden factor evaluation evidence - #12
Conversation
|
Warning Review limit reached
Next review available in: 51 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 (1)
📝 WalkthroughWalkthroughFormula evaluation now uses separate proposal and evaluation contexts, predictive metrics and gates, post-cost trading checks, and versioned Formula v3 evidence. Governance and storage require canonical walk-forward and sealed-holdout evidence before promotion. ChangesFormula v3 evaluation hardening
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ProposalEngine
participant FormulaEvaluator
participant AlphaStore
participant Governance
ProposalEngine->>FormulaEvaluator: submit candidate with proposal metadata
FormulaEvaluator->>AlphaStore: persist predictive and trading evidence
AlphaStore->>Governance: provide canonical walk-forward and sealed evidence
Governance->>Governance: validate evaluator version and holdout status
🚥 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 00b42e0153
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rust_hft/alpha-harness/app/src/governance.rs (1)
71-116: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAllow
OnnxModelcandidates through the walk-forward gate
validated_walk_forward_candidates_in_lineagerejects every non-Formulaartifact before the later match that acceptsCandidateArtifact::OnnxModel(_), so the ONNX branch is unreachable and ONNX candidates can never pass this check.Proposed fix
- if !matches!(candidate.artifact, CandidateArtifact::Formula(_)) { + if !matches!( + candidate.artifact, + CandidateArtifact::Formula(_) | CandidateArtifact::OnnxModel(_) + ) { continue; }🤖 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/governance.rs` around lines 71 - 116, The walk-forward candidate filter in validated_walk_forward_candidates_in_lineage rejects OnnxModel artifacts before the evaluator-version match; remove or broaden the earlier Formula-only check so both Formula and OnnxModel candidates reach the existing artifact/version validation, preserving the matching evaluator-version requirements.
🤖 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.
Inline comments:
In `@rust_hft/alpha-harness/engine/src/formula_evaluator.rs`:
- Around line 73-130: Update predictive_gates to accept evaluator_version and
gate ICIR/RankICIR based on the same matches! evaluator-version check used by
CandidateEvaluation::validate(), not ranges.len(). Ensure single-fold
walk-forward evaluations still compute and record both ICIR failures so
validation returns a normal failed evaluation rather than
InvalidEvaluationEvidence.
In `@rust_hft/alpha-harness/store/src/lib.rs`:
- Around line 822-860: Update has_canonical_walk_forward_evidence so
deserialization failures from serde_json::from_value and validation failures
from evaluation.validate are converted to Ok(false), matching the function’s
graceful “not canonical evidence” behavior. Preserve propagation for unrelated
storage errors and continue allowing callers such as
sealed_passed_candidate_for_mission to inspect other candidates.
---
Outside diff comments:
In `@rust_hft/alpha-harness/app/src/governance.rs`:
- Around line 71-116: The walk-forward candidate filter in
validated_walk_forward_candidates_in_lineage rejects OnnxModel artifacts before
the evaluator-version match; remove or broaden the earlier Formula-only check so
both Formula and OnnxModel candidates reach the existing artifact/version
validation, preserving the matching evaluator-version requirements.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a10795c6-f5bd-4a57-9f14-afcb51327334
📒 Files selected for processing (18)
docs/superpowers/specs/2026-07-11-loop-engineer-production-hardening-design.mdrust_hft/ARCHITECTURE.mdrust_hft/alpha-harness/README.mdrust_hft/alpha-harness/app/src/governance.rsrust_hft/alpha-harness/app/src/loop_control.rsrust_hft/alpha-harness/domain/src/lib.rsrust_hft/alpha-harness/engine/src/engines/bayesian.rsrust_hft/alpha-harness/engine/src/engines/gp.rsrust_hft/alpha-harness/engine/src/engines/mcts.rsrust_hft/alpha-harness/engine/src/engines/offline_rl.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/engine/src/llm.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
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
monday/rust_hft/alpha-harness/store/src/lib.rs
Lines 978 to 982 in 6ba1a73
Require canonical sealed revision IDs
When callers use AlphaStore::promote_candidate directly, a valid v3 sealed_evaluation stored under a legacy or arbitrary revision_id still passes this binding check because only the kind, asset id, and payload fields are compared. That bypasses the new version-bound sealed_evaluation_revision_id contract and can create promotions whose sealed_evaluation_id the governance CLI would never produce, so the store should reject ids other than sealed-evaluation:<evaluator_version>:<candidate_id>.
ℹ️ 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".
|
Fixed the latest sealed-revision finding in 9e573d9: direct promotion now requires @codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
rust_hft/alpha-harness/engine/src/lib.rs (1)
559-560: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueAvoid cloning the JSON payload.
serde_json::from_valuerequires taking ownership of theValue, which necessitatespayload.clone(). Since&serde_json::ValueimplementsDeserializer, you can deserialize directly from the reference without cloning the entire JSON DOM.♻️ Proposed refactor
- let evaluation: CandidateEvaluation = - serde_json::from_value(payload.clone()).map_err(|error| error.to_string())?; + use serde::Deserialize; + let evaluation = CandidateEvaluation::deserialize(payload) + .map_err(|error| error.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/lib.rs` around lines 559 - 560, Update the CandidateEvaluation deserialization in the surrounding evaluation flow to deserialize directly from the borrowed payload reference instead of cloning the serde_json::Value. Preserve the existing error-to-string mapping and propagation behavior while removing the unnecessary payload.clone() allocation.
🤖 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/lib.rs`:
- Around line 559-560: Update the CandidateEvaluation deserialization in the
surrounding evaluation flow to deserialize directly from the borrowed payload
reference instead of cloning the serde_json::Value. Preserve the existing
error-to-string mapping and propagation behavior while removing the unnecessary
payload.clone() allocation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d53d6c49-4000-4242-9578-51cb698678bb
📒 Files selected for processing (6)
docs/superpowers/specs/2026-07-11-loop-engineer-production-hardening-design.mdrust_hft/alpha-harness/app/src/governance.rsrust_hft/alpha-harness/domain/src/lib.rsrust_hft/alpha-harness/engine/src/formula_evaluator.rsrust_hft/alpha-harness/engine/src/lib.rsrust_hft/alpha-harness/store/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- docs/superpowers/specs/2026-07-11-loop-engineer-production-hardening-design.md
- rust_hft/alpha-harness/store/src/lib.rs
- rust_hft/alpha-harness/domain/src/lib.rs
- rust_hft/alpha-harness/engine/src/formula_evaluator.rs
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Summary
Validation
Safety
Summary by CodeRabbit