Add governed GP and immutable CEX Factor Bank core - #645
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe domain adds governed GP policies and factor-bank revisions. The GP engine applies policy-driven candidate generation and retention. Mission lineage persists duplicate GP candidate artifacts and records duplicate proposals. ChangesGoverned GP and Factor Bank
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant GeneticProgrammingEngine
participant CexGpPolicyV1
participant MissionLineage
GeneticProgrammingEngine->>CexGpPolicyV1: Validate generated candidate
CexGpPolicyV1-->>GeneticProgrammingEngine: Return admission result
GeneticProgrammingEngine->>MissionLineage: Submit governed candidate
MissionLineage-->>GeneticProgrammingEngine: Record candidate or duplicate result
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
rust_hft/alpha-harness/domain/src/lib.rs (3)
1387-1397: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider deduplicating
admitted_fieldsincontrolled_v1.
sort()does not remove duplicates. If a caller passes the same field twice,validate()fails with "governed v1 fields, grammar, limits, or budget drifted". The failure is safe, but the message does not identify the real cause. Adedup()call makes the constructor accept normalized input.♻️ Proposed change
admitted_fields.sort(); + admitted_fields.dedup();🤖 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 1387 - 1397, Update controlled_v1 to call dedup on admitted_fields immediately after sorting it, so duplicate field names are normalized before constructing and validating the policy.
1700-1707: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
expected_revision_iddeep-clones the whole revision on every call.The clone copies all attempts, ASTs, and evaluations to blank one
Stringfield.validate()calls this method on every invocation, andnew()calls it twice in total. For a bank with many attempts the copy dominates the hashing cost.A serializable borrowed view, or serializing to a
serde_json::Valueand removingrevision_id, avoids the deep clone. The current behavior is correct, so treat this as an optimization.🤖 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 1700 - 1707, Optimize expected_revision_id by avoiding the deep clone of the entire revision solely to clear revision_id. Build a borrowed serializable view or remove revision_id from a serialized representation before calling canonical_json_hash, while preserving the current hash input and resulting revision ID behavior.
3962-3991: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering the remaining
validate_candidaterejection paths.The test covers unadmitted fields and unadmitted operators. Two governed limits stay untested: the constant rejection path, and the
max_ast_depth/max_ast_nodeslimit. Governed v1 setsconstantsto empty, so anyFactorTerminal::Constantmust be rejected. Add both assertions to lock the governed grammar.As per coding guidelines: "Run the focused package tests with
cargo test -p alpha-domain --locked".🤖 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 3962 - 3991, Extend governed_gp_policy_rejects_unadmitted_fields_and_operators to assert that a FactorTerminal::Constant candidate is rejected, and add a candidate exceeding the policy’s max_ast_depth or max_ast_nodes limit that validate_candidate also rejects. Keep the existing field and operator assertions, then run cargo test -p alpha-domain --locked.Source: Coding guidelines
rust_hft/alpha-harness/engine/src/lib.rs (1)
1545-1583: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the negative case for the new persistence branch.
The test proves that a GP duplicate is persisted. It does not prove that a non-GP duplicate is still discarded. The new condition at lines 457-461 has two halves, and only one is covered.
Add a second run that uses
FixedFormulaEnginewithkind: EngineKind::ManualSeedand assertslineage.candidates.len() == 1withlineage.iterations[1].candidate_artifact_id.is_none().As per coding guidelines: "Run the focused package tests with ...
cargo test -p alpha-engine --locked".🤖 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 1545 - 1583, Add a companion test for the non-GP duplicate path using FixedFormulaEngine configured with EngineKind::ManualSeed, then assert the resulting mission lineage contains one candidate and iterations[1].candidate_artifact_id is None. Keep the existing GP persistence test unchanged, and run the focused package tests with cargo test -p alpha-engine --locked.Source: Coding guidelines
rust_hft/alpha-harness/engine/src/engines/gp.rs (1)
239-262: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare the
evaluationtest fixture instead of copying it.This helper is identical to the
evaluationhelpers inrust_hft/alpha-harness/engine/src/engines/mcts.rsandrust_hft/alpha-harness/engine/src/engines/bayesian.rs.rust_hft/alpha-harness/engine/src/engines/mod.rsalready hosts the sharedtest_datasethelper. Moveevaluationthere and import it in all three test modules. A future change toCandidateEvaluationthen needs one edit, not three.🤖 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/engines/gp.rs` around lines 239 - 262, Move the duplicated evaluation test fixture from gp.rs, mcts.rs, and bayesian.rs into the shared test helpers in engines/mod.rs, alongside test_dataset. Remove the local evaluation definitions and import the shared evaluation helper in all three engine test modules, preserving its current behavior and signature.
🤖 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/domain/src/lib.rs`:
- Around line 1581-1592: Update CexFactorBankRevisionV1::validate to track
entry.factor_id values with a set and reject duplicate factor identities before
the final entry-binding check, returning DomainError::InvalidCexFactorBank with
an appropriate message. Preserve the existing candidate-id and entry-binding
validation.
In `@rust_hft/alpha-harness/engine/src/lib.rs`:
- Around line 457-461: The candidate-artifact uniqueness assumption must no
longer apply universally to GeneticProgramming engines. Update the branch around
persist_candidate to document that uniqueness is guaranteed only by the generic
GeneticProgrammingEngine’s seen-set filtering, or adjust
gp_resume_restores_history_and_keeps_candidate_artifacts_unique so its
uniqueness assertion applies only to generic GP and not governed GP.
---
Nitpick comments:
In `@rust_hft/alpha-harness/domain/src/lib.rs`:
- Around line 1387-1397: Update controlled_v1 to call dedup on admitted_fields
immediately after sorting it, so duplicate field names are normalized before
constructing and validating the policy.
- Around line 1700-1707: Optimize expected_revision_id by avoiding the deep
clone of the entire revision solely to clear revision_id. Build a borrowed
serializable view or remove revision_id from a serialized representation before
calling canonical_json_hash, while preserving the current hash input and
resulting revision ID behavior.
- Around line 3962-3991: Extend
governed_gp_policy_rejects_unadmitted_fields_and_operators to assert that a
FactorTerminal::Constant candidate is rejected, and add a candidate exceeding
the policy’s max_ast_depth or max_ast_nodes limit that validate_candidate also
rejects. Keep the existing field and operator assertions, then run cargo test -p
alpha-domain --locked.
In `@rust_hft/alpha-harness/engine/src/engines/gp.rs`:
- Around line 239-262: Move the duplicated evaluation test fixture from gp.rs,
mcts.rs, and bayesian.rs into the shared test helpers in engines/mod.rs,
alongside test_dataset. Remove the local evaluation definitions and import the
shared evaluation helper in all three engine test modules, preserving its
current behavior and signature.
In `@rust_hft/alpha-harness/engine/src/lib.rs`:
- Around line 1545-1583: Add a companion test for the non-GP duplicate path
using FixedFormulaEngine configured with EngineKind::ManualSeed, then assert the
resulting mission lineage contains one candidate and
iterations[1].candidate_artifact_id is None. Keep the existing GP persistence
test unchanged, and run the focused package tests with cargo test -p
alpha-engine --locked.
🪄 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: 2acbc0ea-11aa-4fd3-bd20-6522a04506f3
📒 Files selected for processing (3)
rust_hft/alpha-harness/domain/src/lib.rsrust_hft/alpha-harness/engine/src/engines/gp.rsrust_hft/alpha-harness/engine/src/lib.rs
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be73d6461b
ℹ️ 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".
be73d64 to
f1c5d6c
Compare
1 participant
Change contract
Add the typed governed-GP policy, immutable CEX Factor Bank invariants, and auditable GP-attempt persistence required by the Agent-led research pipeline, while preserving fitness-guided behavior for generic GP.
Issue relationship
Refs #599
Out of scope
Mission-execute wiring and Factor Bank publication; MCTS subset search; continuous tuning; sealed-holdout opening; Paper, Shadow, LiveSmall, deployment, or runtime activation; new databases or services; prediction-market evaluation; RL or multi-agent runtime behavior.
Dependencies and merge order
Depends on closed Issue #598. Merge in order: this Core PR #645, Determinism PR #647, Evidence PR #648, then Mission PR #646.
Focused validation
cargo +1.91 test -p alpha-domain -p alpha-engine --locked— 46 domain and 80 engine tests passed; one credential-dependent LLM test ignored as designed.generic_gp_remains_fitness_guided— red before the fix, green after it.governed_gp_does_not_observe_evaluation_scores— passed.cargo +1.91 clippy -p alpha-domain -p alpha-engine --all-targets --locked -- -D warnings— passed.git diff --check, and agent-worktree preflight — passed.Rollout and rollback
Research-domain and engine code only; no runtime mutation. Roll back by reverting this PR before its dependent Mission PR.
Scope exception
None. This layer changes 3 files with 744 additions and is below the repository split threshold.
Summary by CodeRabbit
New Features
Bug Fixes