refactor(research): unify Formula MCTS state - #206
Conversation
Move Formula Alpha Search state, selection, backpropagation, budget, and resume onto the shared UCT kernel adapter. Publish deterministic read-only projections and reject legacy or forged checkpoints fail closed.\n\nCloses #203
|
Warning Review limit reached
Next review available in: 45 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 (4)
📝 WalkthroughWalkthroughFormula Alpha Search now uses a versioned ChangesFormula MCTS checkpoint migration
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/prediction-markets/crates/ploy-research/src/bin/monday-prediction-evaluator.rs`:
- Around line 2016-2021: Move the `formula_mcts_checkpoint_json` loading and
validation block using `read_formula_mcts_checkpoint` to immediately after CLI
argument parsing, before report artifacts or prediction feedback are written.
Preserve the existing panic-on-error behavior and retain the validated
`mcts_state` for later evaluation, while keeping the load-status message tied to
the checkpoint path.
- Line 118: Update the reprice-pilot artifact schema identifier from
monday.polymarket.reprice_pilot.v1 to the next version wherever it is declared
or emitted, including the artifact path around the evaluator’s schema metadata.
Keep formula_mcts_checkpoint_sha256 required in the serialized shape so new
readers can distinguish and validate the updated payload.
In `@rust_hft/prediction-markets/docs/ALPHA_FACTOR_SEARCH_CICD.md`:
- Around line 769-772: Update the documentation entry for
--alpha-search-state-json to describe it as a rejected legacy state input, not a
legacy projection. Keep the documented projection names mcts-state.json and
mcts-expansion-plan.json unchanged, and retain the explicit migration diagnostic
behavior.
🪄 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: e515d137-2d6d-41ce-9cf3-0a183a0fc803
📒 Files selected for processing (6)
rust_hft/prediction-markets/config/research_missions/polymarket-btc-5m.example.jsonrust_hft/prediction-markets/config/research_missions/polymarket-sol-5m.example.jsonrust_hft/prediction-markets/crates/ploy-research/src/alpha_search.rsrust_hft/prediction-markets/crates/ploy-research/src/bin/monday-prediction-evaluator.rsrust_hft/prediction-markets/crates/ploy-research/src/lib.rsrust_hft/prediction-markets/docs/ALPHA_FACTOR_SEARCH_CICD.md
| #[derive(serde::Serialize)] | ||
| struct RepricePilotSearchArtifact { | ||
| summary: AlphaSearchArtifactSummary, | ||
| formula_mcts_checkpoint_sha256: String, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Bump the reprice-pilot artifact schema version.
formula_mcts_checkpoint_sha256 is a required serialized field, but the enclosing artifact still advertises monday.polymarket.reprice_pilot.v1 at Line 571. New readers cannot distinguish old v1 payloads from the new shape and may fail on the missing field.
Suggested contract update
- schema_version: "monday.polymarket.reprice_pilot.v1",
+ schema_version: "monday.polymarket.reprice_pilot.v2",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| formula_mcts_checkpoint_sha256: String, | |
| schema_version: "monday.polymarket.reprice_pilot.v2", |
🤖 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/prediction-markets/crates/ploy-research/src/bin/monday-prediction-evaluator.rs`
at line 118, Update the reprice-pilot artifact schema identifier from
monday.polymarket.reprice_pilot.v1 to the next version wherever it is declared
or emitted, including the artifact path around the evaluator’s schema metadata.
Keep formula_mcts_checkpoint_sha256 required in the serialized shape so new
readers can distinguish and validate the updated payload.
![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: 49edaef14d
ℹ️ 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".
| ) -> Result<FormulaMctsCheckpoint, AlphaSearchArtifactError> { | ||
| if let Some(prior) = prior_state { | ||
| validate_formula_mcts_checkpoint(prior)?; | ||
| if prior.target != target || prior.side != side { |
There was a problem hiding this comment.
When the evaluator receives --formula-mcts-checkpoint-json, it iterates over both non-side settlement targets and passes the same checkpoint to each writer (monday-prediction-evaluator.rs lines 2049-2055 and 2090). This identity check therefore rejects the unrelated target; the caller only logs that failure and still exits successfully, leaving that target's artifact bundle stale or missing on every legitimate resume. Previously, mismatched prior state was ignored for the other target, so the caller should pass the checkpoint only to its matching target while initializing unrelated targets without prior state.
Useful? React with 👍 / 👎.
| checkpoint | ||
| .nodes | ||
| .iter() | ||
| .map(|node| (node.factor_name.clone(), node.clone())) | ||
| .collect(), |
There was a problem hiding this comment.
When a resumed run no longer generates a historical candidate—for example after changing the typed LLM prior—this constructs the selectable tree from every checkpoint node rather than restricting it to the current run's metrics. A stale high-scoring leaf can consequently consume one of the 12 selections, but domain_candidates_for_target_with_guidance only expands selected names found in the current candidate set (autofactor.rs lines 2050-2056), so enough stale leaves can yield a nonempty plan that produces no guided mutations. Preserve cumulative statistics, but select only nodes represented by the current evaluation.
Useful? React with 👍 / 👎.
| use hft_search_kernel::{ | ||
| backpropagate as kernel_backpropagate, select_expandable as kernel_select_expandable, | ||
| validate_tree as validate_kernel_tree, UctError, UctNode, UctStats, | ||
| }; |
There was a problem hiding this comment.
This refactor makes Formula selection and backpropagation depend on research-core/search-kernel/src/lib.rs, but current_prediction_policy_snapshot_id() hashes alpha_search.rs and a dependency graph where the local hft-search-kernel has checksum=none; it never hashes the kernel source itself. A later source-only kernel change can therefore alter governed MCTS results without changing the policy snapshot ID, allowing missions pinned to the previous behavior to pass validation. Add the kernel source to prediction_policy_sources() and cover a changed-kernel counterexample.
AGENTS.md reference: AGENTS.md:L93-L96
Useful? React with 👍 / 👎.
| "legacy alpha search state `{path}` is not resumable; use --formula-mcts-checkpoint-json" | ||
| ); | ||
| } | ||
| let formula_mcts_checkpoint_json = flag_value(&args, "--formula-mcts-checkpoint-json"); |
There was a problem hiding this comment.
When --formula-mcts-checkpoint-json is the final argument without a value, flag_value() returns None, so the evaluator silently starts a fresh search instead of rejecting the malformed resume request. If the same output directory is reused, this can overwrite the intended cumulative checkpoint and projections with newly initialized state. Check flag_present() and require a nonempty following value before treating the checkpoint as absent.
Useful? React with 👍 / 👎.
| "Formula MCTS checkpoint contains duplicate candidate identities".to_string(), | ||
| )); | ||
| } | ||
| formula_mcts_kernel_nodes(records)?; |
There was a problem hiding this comment.
The checkpoint validator checks individual UctStats values and topology but not the ancestor statistics guaranteed by kernel_backpropagate. For example, a checkpoint with a synthetic root at zero visits and a child at one visit passes this call, even though the writer can never produce it; projection then reports total_visits = 0, and UCT selection calculates exploration using the forged parent count. Reject checkpoints whose root/ancestor visit counts are inconsistent with their descendants, with a targeted corrupted-state counterexample.
AGENTS.md reference: AGENTS.md:L93-L96
Useful? React with 👍 / 👎.
1 participant
Change contract
Move Formula Alpha Search control state onto the existing shared
hft-search-kernel:formula-mcts-checkpoint.jsonis the sole resumable state, whilemcts-state.jsonandmcts-expansion-plan.jsonare deterministic read-only projections.Closes #203.
Out of scope
Prediction MCTS artifact publication, collector or snapshot changes, evaluator metric redesign, cloud result publication, Paper/Live, OMS, RiskGate, and execution adapters.
Dependency / merge order
Depends on merged #185 and merged #204. No stacked PR; merge this PR directly after CI and review.
Focused validation
cargo test -p ploy-research --no-default-features(280 library tests + 2 CLI tests)cargo clippy -p ploy-research --no-default-features --no-deps -- -D warningscargo fmt --checkgit diff --checksha256:e98b091da59ca2878c79d817b5d774cc8f3591384730eea93b76499f0c0eea79, matching both checked-in BTC/SOL mission templates.Rollout / rollback impact
The new Formula image writes one versioned checkpoint plus read-only projections. Mixed checkpoint writers are forbidden; legacy
--alpha-search-state-jsoninput fails with an explicit migration diagnostic. Rollback selects the previous Formula image and its prior checkpoint schema. No runtime, collector, or trading deployment changes.Scope assessment
This is 6 files and 857 changed non-generated lines, above the 750-line assessment threshold. It remains one atomic schema rollout because the checkpoint writer, reader flag, projection contract, policy pins, and migration documentation must move together; splitting would create a new-writer/old-reader or old-writer/new-reader intermediate that cannot safely resume. The rollback unit is likewise the prior Formula image plus prior checkpoint schema. Read-only reviewers
standards_review_203andspec_review_203approved this atomic boundary after the root-order, budget-resume, and documentation findings were fixed.Summary by CodeRabbit
New Features
Bug Fixes
Documentation