Persist governed GP screening into an immutable CEX Factor Bank - #646
Conversation
|
Warning Review limit reached
Next review available in: 44 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 Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughChangesGoverned GP mission and factor-bank construction
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 9941631b5c
ℹ️ 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".
9941631 to
079a1b8
Compare
56dafbe to
cb4db0a
Compare
079a1b8 to
e95ff28
Compare
8a4c78c to
3f0d08e
Compare
e95ff28 to
6cf7ca6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
rust_hft/alpha-harness/domain/src/lib.rs (1)
1940-1959: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
ScreeningFailedis now unreachable for new revisions.
rejected_codes_are_boundaccepts only derived gate codes when evidence exists, and onlyDuplicateCandidate,EvaluationFailed, orEngineFailurewhen evidence is absent. No path can now produceCexFactorRejectionCodeV1::ScreeningFailed. The test at Line 4398 confirms it is rejected.If the variant is kept only for reading legacy v1 payloads, add a short comment that states this. Otherwise remove it.
🤖 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/domain/src/lib.rs` around lines 1940 - 1959, Document that CexFactorRejectionCodeV1::ScreeningFailed is retained solely for reading legacy v1 payloads, since rejected_codes_are_bound no longer accepts it for new revisions. Add the comment next to the variant or its relevant compatibility handling; if legacy deserialization is not required, remove the unused variant instead.rust_hft/alpha-harness/app/src/mission_runner.rs (2)
291-295: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winMove the screening-policy check before the first store write.
The GP policy check at Line 167 runs before
AlphaStore::open, so a drifted GP policy leaves no database. The screening-policy check runs afterstore.put_registry_revisionfor the mission and after the feature and dataset registration. A drifted screening policy therefore aborts the run with a partially populatedalpha.duckdband results directory.The inputs to
FormulaEvaluatorConfig::for_missionthat matter here aresearch_budget.max_candidatesandvalidator_spec, both taken fromcontrol_mission. Compute the same hash next to the GP policy check withFormulaEvaluatorConfig::for_trials(control_mission.spec.search.budget.max_candidates)and fail there.♻️ Proposed fail-fast ordering
gp_policy.validate_binding(&control_mission.spec.policies.gp)?; + if canonical_json_hash(&FormulaEvaluatorConfig::for_trials( + control_mission.spec.search.budget.max_candidates, + )?)? != control_mission.spec.policies.screening.content_sha256 + { + bail!("Mission screening policy does not match the evaluator configuration"); + } data_mission::write_json_atomic(&results_dir.join("gp-policy.json"), &gp_policy)?;Then remove the later check at Lines 291-295.
🤖 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 291 - 295, Move the screening-policy validation from the post-registration section into the pre-store validation block near the existing GP policy check in the mission runner. Compute the expected hash using FormulaEvaluatorConfig::for_trials(control_mission.spec.search.budget.max_candidates), compare it with control_mission.spec.policies.screening.content_sha256, and fail before AlphaStore::open or any writes; then remove the later FormulaEvaluatorConfig::for_mission check.
474-484: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueRestrict the
failure_explanationfallback to rejected attempts.The fallback applies to every verdict. If a kept iteration carries a
failure_explanation,rejection_detailsbecomes non-empty.CexFactorBankRevisionV2::validaterequiresrejection_details.is_empty()for an accepted attempt, so the run then fails at bank construction.Gate the fallback on the verdict.
♻️ Proposed guard
.filter(|reasons| !reasons.is_empty()) .or_else(|| { + if verdict == CexFactorScreeningVerdictV1::Accepted { + return None; + } iteration .failure_explanation .clone() .map(|reason| vec![reason]) })🤖 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 474 - 484, Update the rejection_details construction to use iteration.failure_explanation only when the verdict indicates a rejected attempt; accepted or kept iterations must retain an empty fallback so CexFactorBankRevisionV2::validate can succeed. Preserve the existing evaluation evidence handling and default-empty behavior.
🤖 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/app/src/mission_runner.rs`:
- Around line 1258-1262: Update the factor-bank assertion near the orientation
check to first require that the entries array contains at least one element,
then retain the existing assertion that every entry has positive orientation.
Use the parsed entries collection so both assertions validate the
accepted-candidate path.
---
Nitpick comments:
In `@rust_hft/alpha-harness/app/src/mission_runner.rs`:
- Around line 291-295: Move the screening-policy validation from the
post-registration section into the pre-store validation block near the existing
GP policy check in the mission runner. Compute the expected hash using
FormulaEvaluatorConfig::for_trials(control_mission.spec.search.budget.max_candidates),
compare it with control_mission.spec.policies.screening.content_sha256, and fail
before AlphaStore::open or any writes; then remove the later
FormulaEvaluatorConfig::for_mission check.
- Around line 474-484: Update the rejection_details construction to use
iteration.failure_explanation only when the verdict indicates a rejected
attempt; accepted or kept iterations must retain an empty fallback so
CexFactorBankRevisionV2::validate can succeed. Preserve the existing evaluation
evidence handling and default-empty behavior.
In `@rust_hft/alpha-harness/domain/src/lib.rs`:
- Around line 1940-1959: Document that CexFactorRejectionCodeV1::ScreeningFailed
is retained solely for reading legacy v1 payloads, since
rejected_codes_are_bound no longer accepts it for new revisions. Add the comment
next to the variant or its relevant compatibility handling; if legacy
deserialization is not required, remove the unused variant instead.
🪄 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 Plus
Run ID: 2595aef6-9420-41ca-800d-e042670e46f6
📒 Files selected for processing (3)
rust_hft/alpha-harness/app/src/mission.rsrust_hft/alpha-harness/app/src/mission_runner.rsrust_hft/alpha-harness/domain/src/lib.rs
The seed-7 fixture proposed an always-positive spread signal against unrelated labels, so its orientation assertion passed vacuously with an empty Factor Bank. Align the test-only imbalance signal and label so the mission seam proves an accepted entry without weakening screening gates. Refs #599
Change contract
Extend the sole acceptance seam
alpha-harness mission executeto derive and enforce the frozen governed GP policy, run GP only against label-free research context, audit every candidate attempt, and persist one typed create-oncecex-factor-bank-v2registry revision with evidence-bound orientation and structured rejection codes.Issue relationship
Closes #599
Out of scope
MCTS subset search or formula generation; Ridge/CART baselines; continuous tuning; sealed-holdout opening; Paper, Shadow, LiveSmall, deployment, runtime activation, new databases or services, prediction-market evaluation, RL, or multi-agent runtime behavior.
Dependencies and merge order
Core PR #645, Determinism PR #647, Evidence PR #648, and canonical-hash PR #649 are merged. This PR is rebuilt directly on
main@99589288cb3b79fd63891c4c99d3e52b87b95ad5and is the final code contract for Issue #599.Focused validation
execute_screens_gp_candidates_into_an_immutable_factor_bank— proves GP-only execution, Factor Bank v2 publication, immutable registry conflict rejection, and no MCTS receipt.execute_records_duplicate_gp_attempts_with_a_typed_rejection— proves structured evaluated/duplicate rejection codes and that rejected candidates cannot enterentries.execute_is_deterministic_and_ignores_holdout_only_values— proves chunked/resumed and uninterrupted execution produce identical Factor Bank semantic content, including after a holdout-only mutation.execute_rejects_gp_policy_drift_before_screening— proves frozen-policy drift fails before persistence.cargo +1.91 test -p alpha-domain -p alpha-engine -p alpha-harness --locked— 49 domain, 81 engine, and 129 harness tests passed; one credential-dependent LLM test ignored as designed.-D warnings, exact-package Rustfmt,git diff --check, and agent-worktree preflight — passed.Rollout and rollback
Research-only mission behavior; sealed holdout remains closed and no runtime authority is added. Roll back by reverting this PR; the already-merged domain/hash prerequisites remain inert without this producer path.
Scope exception
None. This layer changes 3 files with 560 insertions and remains below the repository split threshold.
Summary by CodeRabbit
New Features
Improvements