feat(research): publish prediction MCTS checkpoint artifact - #204
Conversation
|
Warning Review limit reached
Next review available in: 32 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)
📝 WalkthroughWalkthroughThe PR adds validated, serializable read-only artifacts for prediction-MCTS checkpoints and run state, persists them during checkpointing, adds stability and identity validation tests, and updates policy snapshot hashes in two mission templates. ChangesPrediction-MCTS artifacts
Mission policy snapshot references
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant PredictionMctsRunState
participant PredictionMctsCheckpoint
participant ArtifactFile
PredictionMctsRunState->>PredictionMctsCheckpoint: validate checkpoint identity
PredictionMctsRunState->>PredictionMctsRunState: validate candidates and training records
PredictionMctsRunState->>ArtifactFile: atomically write prediction-mcts-artifact.json
PredictionMctsRunState->>ArtifactFile: preserve existing artifact on validation failure
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 |
![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: 1b961502bd
ℹ️ 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".
| }) | ||
| .collect::<Result<Vec<_>, _>>()?; | ||
| for candidate in [&self.pending, &self.selected].into_iter().flatten() { | ||
| if candidate.identity != identity { |
There was a problem hiding this comment.
If the durable state is corrupted or tampered with so that selected retains the mission identity but contains an unobserved candidate ID or blend, this identity-only check accepts it and the new artifact publishes it as the selected result; a subsequent resume also skips recomputing selection and can evaluate that forged candidate on held-out data. Require selected to exactly match a candidate in the validated training records (and similarly cross-check pending against the checkpoint), with a same-identity counterexample test.
AGENTS.md reference: AGENTS.md:L93-L96
Useful? React with 👍 / 👎.
| } else if self.checkpoint.is_some() { | ||
| "training_ready" |
There was a problem hiding this comment.
When the run deadline reaches zero before all candidates are trained, run_or_resume_prediction_mcts returns BudgetExhausted without updating state, but this projection continues to emit training_ready. Because the stored deadline is immutable, every resume immediately returns exhausted again, leaving the published artifact permanently describing an impossible resumable phase instead of the terminal outcome. Persist or derive a budget-exhausted phase before returning.
Useful? React with 👍 / 👎.
| let artifact = state.read_only_artifact()?; | ||
| atomic_write_json(path, state)?; | ||
| atomic_write_json( | ||
| &path.with_file_name("prediction-mcts-artifact.json"), |
There was a problem hiding this comment.
Every checkpoint replaces the same prediction-mcts-artifact.json, even though its payload contains candidate, training-evaluation, and policy evidence. This module's AGENTS.md requires following the root CLAUDE.md, whose engineering rule at line 34 requires that such evidence be content-addressed or append-only; the mutable file lets later checkpoints and resumes erase prior bytes—especially paused-state evidence—and provides no digest establishing which projection was reviewed. Publish content-addressed or append-only generations, with a separate mutable latest pointer if needed.
AGENTS.md reference: rust_hft/prediction-markets/AGENTS.md:L7-L10
Useful? React with 👍 / 👎.
| || record.evaluation.held_out_settlement.is_some() | ||
| || record.evaluation.execution.is_some() |
There was a problem hiding this comment.
A PredictionMctsEvaluation explicitly permits held-out settlement and execution evidence to be returned for analysis while only training settlement affects reward, and the previous runner durably stored such responses. This new projection instead rejects either optional field: a compliant evaluator returning one now reaches engine.observe, then fails at checkpointing before the updated state is saved, so every resume repeats the same pending evaluation; existing version-1 states containing the fields also become unreadable. Omit the auxiliary fields from the training-only projection without rejecting the valid underlying evaluation.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
rust_hft/prediction-markets/crates/ploy-research/src/prediction_mcts_run.rs (1)
120-150: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRejection branches for training/candidate identity mismatch are untested.
read_only_artifact()explicitly fails closed when a training record carriesheld_out_settlement/execution, or whencandidate/training_settlementidentities mismatch, or whenpending/selectedidentity mismatches the mission (Lines 133-134, 145-149). Only the checkpoint-identity-forgery path is covered by a test; these equally important fail-closed branches have no direct test exercising theErrpath.🤖 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/prediction_mcts_run.rs` around lines 120 - 150, Add direct tests for read_only_artifact() covering each fail-closed validation branch: mismatched training candidate identity, mismatched training settlement identity or candidate/probability hash, present held_out_settlement or execution, and mismatched pending or selected candidate identity. Assert each malformed artifact returns Err while retaining the existing valid and checkpoint-identity-forgery coverage.
🤖 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/prediction_mcts_run.rs`:
- Around line 444-450: Update checkpoint so it does not call read_only_artifact
for full-tree validation and digest recomputation on every write. Cache each
node’s probability_blend_sha256 when the node is inserted, then have checkpoint
reuse the maintained artifact or cached digests while preserving atomic writes
of both the run state and prediction-mcts-artifact.json.
---
Nitpick comments:
In `@rust_hft/prediction-markets/crates/ploy-research/src/prediction_mcts_run.rs`:
- Around line 120-150: Add direct tests for read_only_artifact() covering each
fail-closed validation branch: mismatched training candidate identity,
mismatched training settlement identity or candidate/probability hash, present
held_out_settlement or execution, and mismatched pending or selected candidate
identity. Assert each malformed artifact returns Err while retaining the
existing valid and checkpoint-identity-forgery coverage.
🪄 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: d91efeed-26a3-4086-8182-81f7b3915985
📒 Files selected for processing (4)
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/prediction_mcts.rsrust_hft/prediction-markets/crates/ploy-research/src/prediction_mcts_run.rs
| fn checkpoint(path: &Path, state: &PredictionMctsRunState) -> Result<(), String> { | ||
| atomic_write_json(path, state) | ||
| let artifact = state.read_only_artifact()?; | ||
| atomic_write_json(path, state)?; | ||
| atomic_write_json( | ||
| &path.with_file_name("prediction-mcts-artifact.json"), | ||
| &artifact, | ||
| ) |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm how often checkpoint() is invoked relative to MCTS iterations
rg -n 'checkpoint\(' rust_hft/prediction-markets/crates/ploy-research/src/prediction_mcts_run.rs -B3 -A3Repository: proerror77/monday
Length of output: 3081
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE='rust_hft/prediction-markets/crates/ploy-research/src/prediction_mcts_run.rs'
echo '== checkpoint call sites =='
rg -n 'checkpoint\(' "$FILE" -B2 -A4
echo
echo '== read_only_artifact definitions =='
rg -n 'read_only_artifact' rust_hft/prediction-markets/crates/ploy-research/src -n -A4 -B4
echo
echo '== checkpoint-related structs/usages =='
rg -n 'PredictionMctsCheckpoint|probability_blend_sha256|sha256' rust_hft/prediction-markets/crates/ploy-research/src -n -A3 -B3Repository: proerror77/monday
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the main control flow around the checkpoint calls.
sed -n '200,380p' rust_hft/prediction-markets/crates/ploy-research/src/prediction_mcts_run.rs
echo
echo '---'
# Show the checkpoint helper itself.
sed -n '440,470p' rust_hft/prediction-markets/crates/ploy-research/src/prediction_mcts_run.rsRepository: proerror77/monday
Length of output: 7226
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '277,385p' rust_hft/prediction-markets/crates/ploy-research/src/prediction_mcts.rsRepository: proerror77/monday
Length of output: 5143
Avoid re-validating the full tree on every checkpoint write. checkpoint() runs after each candidate proposal and evaluation, so state.read_only_artifact() recomputes validation and every node’s probability_blend_sha256 on a hot path. That makes the cumulative cost grow quadratically with tree size; caching the digest when a node is inserted would keep writes cheap.
🤖 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/prediction_mcts_run.rs`
around lines 444 - 450, Update checkpoint so it does not call read_only_artifact
for full-tree validation and digest recomputation on every write. Cache each
node’s probability_blend_sha256 when the node is inserted, then have checkpoint
reuse the maintained artifact or cached digests while preserving atomic writes
of both the run state and prediction-mcts-artifact.json.
1 participant
Closes #202\n\n## Change contract\n\nPublish a versioned, read-only prediction-MCTS checkpoint artifact beside the official durable runner state. It exposes governed mission identity, checkpoint candidate lineage, training-only settlement observations, selection, and terminal phase without changing MCTS selection, reward, or checkpoint state.\n\n## Out of scope\n\nFormula Alpha Search migration (#203), collectors, snapshots, evaluator metrics, cloud result publication, Paper/Live, OMS, and RiskGate.\n\n## Dependency / merge order\n\nDepends on merged #187. This PR is independent of #203. #189 follows #202, #203, and parity evidence.\n\n## Focused validation\n\n- cargo test -p ploy-research prediction_mcts --no-default-features\n- cargo test -p ploy-research checked_in_btc_and_sol_templates_pin_current_brief_and_rust_policy --no-default-features\n- cargo clippy -p ploy-research --no-default-features --no-deps -- -D warnings\n- cargo fmt --check\n- git diff --check\n\nIncludes counterexamples for forged checkpoint identity and projection/state byte preservation on rejection.\n\n## Rollout / rollback impact\n\nAdditive artifact only. Rollback uses the prior image and stops publishing the new file; no collector, deployment, execution, or checkpoint-schema migration occurs.
Summary by CodeRabbit
New Features
Bug Fixes
Configuration