#324 layer 1: type shared MCTS task identities - #384
Conversation
📝 WalkthroughWalkthroughPrediction MCTS now uses task-typed identities and training evidence, validates sealed mission admissions, persists typed run state, isolates task outputs, and rejects incompatible or forged evidence. Mission examples update their policy snapshot identifiers. ChangesTyped prediction MCTS
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Mission as Admitted mission
participant Run as prediction_mcts_run
participant Engine as PredictionMctsEngine
participant Evaluator as monday-prediction-evaluator
participant State as PredictionMctsRunState
Mission->>Run: provide mission and typed identity
Run->>State: load and validate versioned state
Run->>Engine: construct with validated identity
Evaluator->>Engine: submit typed training evidence
Engine->>Engine: validate task and evidence bindings
Engine->>State: persist training reward and checkpoint state
🚥 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: b99c2639f4
ℹ️ 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: 1
🧹 Nitpick comments (4)
rust_hft/prediction-markets/crates/ploy-research/src/prediction_mcts_run.rs (2)
512-542: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAsymmetric match arms make
task_output_dirharder to follow than it needs to be.Two arms early-return a fully built path while the third falls through to the shared
joinat Line 541. Producing aStringin all three arms keeps the path construction in one place.♻️ Proposed simplification
- let task = match identity.task { - crate::prediction_mcts::PredictionMctsTask::SettlementProbability => "settlement", - crate::prediction_mcts::PredictionMctsTask::UpExecution { - prediction_horizon_secs, - } => { - return Ok(output_dir - .join("mcts-v4") - .join(mission_sha256) - .join(format!("up-execution-{prediction_horizon_secs}s"))) - } - crate::prediction_mcts::PredictionMctsTask::DownExecution { - prediction_horizon_secs, - } => { - return Ok(output_dir - .join("mcts-v4") - .join(mission_sha256) - .join(format!("down-execution-{prediction_horizon_secs}s"))) - } - }; + use crate::prediction_mcts::PredictionMctsTask; + let task = match identity.task { + PredictionMctsTask::SettlementProbability => "settlement".to_string(), + PredictionMctsTask::UpExecution { + prediction_horizon_secs, + } => format!("up-execution-{prediction_horizon_secs}s"), + PredictionMctsTask::DownExecution { + prediction_horizon_secs, + } => format!("down-execution-{prediction_horizon_secs}s"), + }; Ok(output_dir.join("mcts-v4").join(mission_sha256).join(task))🤖 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 512 - 542, Update task_output_dir so all PredictionMctsTask match arms, including UpExecution and DownExecution, produce only their task-name String values; then construct the shared output_dir.join("mcts-v4").join(mission_sha256).join(task) path once after the match, preserving the existing task names and horizon formatting.
935-957: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis test can no longer fail.
With
include_auxiliary_evidencegone, the setup at Lines 939-941 is justFakeEvaluator::default(), andheld_out_settlement/executionare no longer fields on any type in the evidence enum — so the twoget(...).is_none()assertions hold unconditionally. Same for the equivalent assertions at Lines 915-916. Either delete this test as superseded byrunner_rejects_task_mismatched_persisted_training_evidence, or re-point it at something the new schema can actually violate (e.g. asserting the serializedtrainingobject has exactly thesettlement_probabilitykey).🤖 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 935 - 957, Remove the now-vacuous test runner_omits_auxiliary_evidence_without_rejecting_the_training_result and its equivalent assertions near the earlier test, since the removed include_auxiliary_evidence behavior makes them unconditionally pass. Retain runner_rejects_task_mismatched_persisted_training_evidence as the relevant schema validation coverage.rust_hft/prediction-markets/crates/ploy-research/src/prediction_mcts.rs (2)
1277-1295: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest no longer exercises what its name claims.
right_evaluationis now a clone ofleft_evaluation, so the assertion reduces to "two identical engines observing identical input agree" — it no longer proves thattrainingis the only surface influencing state. Consider asserting instead that a divergent non-trainingmutation is impossible by construction (e.g. thatPredictionMctsEvaluationround-trips with only thetrainingkey), or rename to reflect the determinism check.🤖 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.rs` around lines 1277 - 1295, The test training_evidence_is_the_only_observation_surface currently compares identical evaluations and only verifies determinism. Update it to exercise the claimed contract by proving PredictionMctsEvaluation round-trips with only the training key and that non-training fields cannot mutate observed state; otherwise rename the test to describe identical-engine determinism.
37-40: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider dropping
#[serde(default)]ontasknow that the schema version is bumped.
CHECKPOINT_VERSION/RUN_STATE_VERSIONmoved to 3, so no v2 payload can reach this deserializer. Keepingdefaultmeans a v3 payload that omitstasksilently decodes asSettlementProbability— including whensealed_missionis present (that combination passesvalidate). Equality checks against the caller-supplied identity currently contain the blast radius, but makingtaskmandatory (and addingdeny_unknown_fieldsto matchPredictionMctsSealedMissionIdentity) would make the typed identity fail closed on its own.♻️ Proposed tightening
+#[serde(deny_unknown_fields)] pub struct PredictionMctsIdentity { pub mission_id: String, pub data_snapshot_id: String, pub symbol: String, pub horizon: String, - #[serde(default)] pub task: PredictionMctsTask, #[serde(default, skip_serializing_if = "Option::is_none")] sealed_mission: Option<PredictionMctsSealedMissionIdentity>, }🤖 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.rs` around lines 37 - 40, Make the prediction MCTS typed identity/configuration struct require task by removing #[serde(default)] from its task field, so omitted v3 values fail deserialization instead of defaulting to SettlementProbability. Add #[serde(deny_unknown_fields)] to the same struct, matching PredictionMctsSealedMissionIdentity, so sealed mission payloads reject unsupported fields and malformed combinations fail closed.
🤖 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/config/research_missions/polymarket-btc-5m.example.json`:
- Line 18: Regenerate the policy snapshot IDs for both Polymarket research
example missions using the current digest produced by
current_prediction_policy_snapshot_id(), including prediction_mcts_run.rs, and
update each search_policy_snapshot_id so validate_prediction_mission_v3 accepts
them.
---
Nitpick comments:
In `@rust_hft/prediction-markets/crates/ploy-research/src/prediction_mcts_run.rs`:
- Around line 512-542: Update task_output_dir so all PredictionMctsTask match
arms, including UpExecution and DownExecution, produce only their task-name
String values; then construct the shared
output_dir.join("mcts-v4").join(mission_sha256).join(task) path once after the
match, preserving the existing task names and horizon formatting.
- Around line 935-957: Remove the now-vacuous test
runner_omits_auxiliary_evidence_without_rejecting_the_training_result and its
equivalent assertions near the earlier test, since the removed
include_auxiliary_evidence behavior makes them unconditionally pass. Retain
runner_rejects_task_mismatched_persisted_training_evidence as the relevant
schema validation coverage.
In `@rust_hft/prediction-markets/crates/ploy-research/src/prediction_mcts.rs`:
- Around line 1277-1295: The test
training_evidence_is_the_only_observation_surface currently compares identical
evaluations and only verifies determinism. Update it to exercise the claimed
contract by proving PredictionMctsEvaluation round-trips with only the training
key and that non-training fields cannot mutate observed state; otherwise rename
the test to describe identical-engine determinism.
- Around line 37-40: Make the prediction MCTS typed identity/configuration
struct require task by removing #[serde(default)] from its task field, so
omitted v3 values fail deserialization instead of defaulting to
SettlementProbability. Add #[serde(deny_unknown_fields)] to the same struct,
matching PredictionMctsSealedMissionIdentity, so sealed mission payloads reject
unsupported fields and malformed combinations fail closed.
🪄 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: 86d9f5e3-3f8c-4473-9e65-f578dd8d0c78
📒 Files selected for processing (5)
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/bin/monday-prediction-evaluator.rsrust_hft/prediction-markets/crates/ploy-research/src/prediction_mcts.rsrust_hft/prediction-markets/crates/ploy-research/src/prediction_mcts_run.rs
Change contract: Add typed shared-MCTS training evidence, sealed Mission v4 task identity binding, task-isolated checkpoint namespaces, and fail-closed state upgrades for prediction-market research runs. Legacy bridge execution for sealed Mission v4 identities is rejected until the authenticated runner layer lands.
Out of scope: Authenticated partition-backed evaluator inputs, held-out gating for Mission v4 tasks, typed receipts/experiment manifest generation, dispatch or alpha-harness admission wiring, and collector/snapshot changes.
Dependency or merge order: Base main at d80b2bf after #382. This is the first stacked layer for #324; authenticated evaluator/receipt work follows on top.
Focused validation: cargo +1.91 test --locked -p ploy-research --lib (334 passed); cargo +1.91 test --locked -p ploy-research --features ml --lib (358 passed before final fail-closed guard); cargo +1.91 clippy --locked -p ploy-research --features ml --all-targets --no-deps -- -D warnings; cargo +1.91 check --locked -p ploy-research --bin monday-prediction-research; cargo +1.91 check --locked -p ploy-research --features db --bin monday-prediction-evaluator; targeted sealed-bridge and persisted-evidence counterexamples; git diff --check. Dual-axis read-only review: CLEAN.
Rollout/rollback impact: Research-only. Rollback is a single revert of this PR; no collector/runtime cutover or live trading impact.
Why no PRD split note: #324 already has an explicit issue contract; this PR is the minimum mergeable layer that keeps the typed-kernel/state contract fail-closed before evaluator/receipt follow-ups.
Summary by CodeRabbit
New Features
Bug Fixes