feat(research): gate MCTS on authenticated cohorts - #387
Conversation
|
Warning Review limit reached
Next review available in: 29 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 (5)
📝 WalkthroughWalkthroughThe change adds authenticated Mission V3 prediction-MCTS execution, validates sealed mission and snapshot identities, partitions authenticated snapshots into training and held-out views, and publishes content-addressed receipt evidence. It also updates policy fingerprints and example mission snapshot identifiers. ChangesAuthenticated MCTS
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant MissionV3
participant AuthenticatedTrial
participant PredictionMctsEngine
participant AuthenticatedRunner
participant ReceiptStore
MissionV3->>AuthenticatedTrial: validate admitted mission and snapshot identity
AuthenticatedTrial->>AuthenticatedTrial: build training and held-out partitions
AuthenticatedTrial->>PredictionMctsEngine: construct admitted MCTS engine
AuthenticatedTrial->>AuthenticatedRunner: run or resume authenticated trial
AuthenticatedRunner-->>AuthenticatedTrial: return selection and evaluator evidence
AuthenticatedTrial->>ReceiptStore: publish content-addressed receipt reference
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc572108f0
ℹ️ 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".
fc57210 to
5a4c6ae
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
rust_hft/prediction-markets/crates/ploy-research/src/prediction_mcts_authenticated.rs (2)
449-482: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider grouping rows once instead of rescanning per market.
snapshot_viewscans allobservationsand allpm_book_snapshotsonce per market id, andauthenticated_snapshot_viewscalls it twice (train + held-out), so cost grows asmarkets x rows. Fine for the three-event fixture, but it becomes the dominant cost on a real cohort. A single grouping pass keyed byevent_id, then indexed lookup in catalog order, preserves the existing ordering and error semantics.♻️ Sketch: group once, then index in catalog order
fn snapshot_view( snapshot: &ResearchSnapshot, ordered_market_ids: &[String], ) -> Result<AuthenticatedTrainingSnapshot, String> { + let mut observations_by_market: BTreeMap<&str, Vec<&FactorObservation>> = BTreeMap::new(); + for row in &snapshot.observations { + observations_by_market + .entry(row.event_id.as_str()) + .or_default() + .push(row); + } + let mut books_by_market: BTreeMap<&str, Vec<&ResearchPmBookSnapshot>> = BTreeMap::new(); + for row in &snapshot.pm_book_snapshots { + books_by_market + .entry(row.event_id.as_str()) + .or_default() + .push(row); + } let mut observations = Vec::new(); let mut pm_book_snapshots = Vec::new(); for market_id in ordered_market_ids { - let before = observations.len(); - observations.extend( - snapshot - .observations - .iter() - .filter(|row| row.event_id == *market_id) - .cloned(), - ); - if observations.len() == before { - return Err(format!( - "authenticated snapshot has no observations for {market_id}" - )); - } - pm_book_snapshots.extend( - snapshot - .pm_book_snapshots - .iter() - .filter(|row| row.event_id == *market_id) - .cloned(), - ); + let rows = observations_by_market + .get(market_id.as_str()) + .ok_or_else(|| { + format!("authenticated snapshot has no observations for {market_id}") + })?; + observations.extend(rows.iter().copied().cloned()); + if let Some(books) = books_by_market.get(market_id.as_str()) { + pm_book_snapshots.extend(books.iter().copied().cloned()); + } }🤖 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_authenticated.rs` around lines 449 - 482, Optimize snapshot_view by grouping observations and pm_book_snapshots once by event_id, then assembling both output collections by looking up each market_id in ordered_market_ids. Preserve catalog-order grouping, cloned rows, and the existing error when a market has no observations, while avoiding per-market rescans of the full snapshot collections.
508-533: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a counterexample for the receipt namespace guard.
The two partition counterexamples are here, but the entrypoint's two other fail-closed branches — "authenticated receipt exists before held-out completion" (Line 296) and "authenticated receipt escaped its task namespace" (Line 303) — have no test. A small tmpdir fixture that writes an
authenticated-receipt-ref.jsonpointing outsidetask_dir/receiptswould pin the namespace guard without needing a full evaluator.🤖 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_authenticated.rs` around lines 508 - 533, Add a focused test in the existing tests module for the authenticated receipt namespace guard in the entrypoint logic, covering a receipt reference that resolves outside task_dir/receipts and asserting the operation fails closed. Use a temporary-directory fixture to create the task directory and authenticated-receipt-ref.json pointing to an external receipt, without constructing a full evaluator; also preserve coverage for the existing pre-held-out-completion rejection branch if needed by the surrounding test setup.
🤖 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.
Nitpick comments:
In
`@rust_hft/prediction-markets/crates/ploy-research/src/prediction_mcts_authenticated.rs`:
- Around line 449-482: Optimize snapshot_view by grouping observations and
pm_book_snapshots once by event_id, then assembling both output collections by
looking up each market_id in ordered_market_ids. Preserve catalog-order
grouping, cloned rows, and the existing error when a market has no observations,
while avoiding per-market rescans of the full snapshot collections.
- Around line 508-533: Add a focused test in the existing tests module for the
authenticated receipt namespace guard in the entrypoint logic, covering a
receipt reference that resolves outside task_dir/receipts and asserting the
operation fails closed. Use a temporary-directory fixture to create the task
directory and authenticated-receipt-ref.json pointing to an external receipt,
without constructing a full evaluator; also preserve coverage for the existing
pre-held-out-completion rejection branch if needed by the surrounding test
setup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9f82570b-7350-423d-8a7d-e9a748d57079
📒 Files selected for processing (9)
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/lib.rsrust_hft/prediction-markets/crates/ploy-research/src/prediction_loop.rsrust_hft/prediction-markets/crates/ploy-research/src/prediction_mcts.rsrust_hft/prediction-markets/crates/ploy-research/src/prediction_mcts_authenticated.rsrust_hft/prediction-markets/crates/ploy-research/src/prediction_mcts_run.rsrust_hft/prediction-markets/crates/ploy-research/src/prediction_mission_v3.rsrust_hft/prediction-markets/crates/ploy-research/src/research_snapshot.rs
Change contract
Allow the shared prediction MCTS runner to consume only a freshly verified authenticated snapshot partition, preserve catalog order, exclude crossing events, release held-out rows only after a durable selected checkpoint, bind the immutable evaluator image across resumes, and durably stage task-isolated held-out evaluator evidence.
Out of scope
Typed receipts are intentionally a dependent #324 layer: execution and publication remain separate merge and rollback units. A separate PRD is unnecessary because issue #324 already defines the approved parent behavior contract.
Dependency / merge order
Focused validation
Rollout / rollback impact
No runtime, result-publication, or collector rollout. The new evaluator trait is sealed, so this layer cannot be invoked by an external evaluator before the trusted built-in implementation lands. Rollback is this PR only; legacy MCTS behavior remains unchanged.