feat(research): evaluate authenticated MCTS tasks - #408
Conversation
📝 WalkthroughWalkthroughThe PR updates BTC and SOL mission snapshot identifiers and substantially revises authenticated Prediction MCTS evaluation with typed settlement/execution metrics, schema-versioned artifact validation, deadline checks, persistence validation, and expanded tests. ChangesAuthenticated Prediction MCTS evaluation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Mission
participant Evaluator
participant Metrics
participant Artifact
Mission->>Evaluator: provide task and candidate
Evaluator->>Metrics: compute settlement or execution metrics
Metrics-->>Evaluator: return typed metrics
Evaluator->>Artifact: construct and validate held-out artifact
Artifact-->>Evaluator: persist validated artifact
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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af2c53b3c5
ℹ️ 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".
71bf456 to
a444f10
Compare
af2c53b to
62db49e
Compare
62db49e to
5c081ca
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
rust_hft/prediction-markets/crates/ploy-research/src/prediction_mcts_authenticated.rs (2)
265-283: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHardcoded
stakes_usd: vec![15.0]/min_bucket_observations: 1override the module defaults without an explanatory comment. Consider extracting a named constant documenting why $15 / per-decision granularity is the right choice for authenticated evaluation (vs. the bucketed defaults of[1.0, 3.0, 5.0, 10.0, 15.0]/20).🤖 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 265 - 283, The execution_task_metrics options use unexplained authenticated-evaluation overrides. Extract the $15 stake and per-decision min-bucket settings into clearly named constants near the module defaults, add a concise comment documenting why authenticated evaluation requires them instead of the bucketed defaults, and use those constants in FullDepthExecutionMatrixOptions.
854-1139: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSolid coverage for label-isolation and missing-quote/book fail-closed behavior. Consider adding one more counterexample for an out-of-range
prediction_horizon_secs(e.g.21) assertingbuiltin_task_metrics/execution_task_metricsreturnsErrrather than zeroed metrics, tying into the fail-closed gap flagged on Line 172-200/265-405. Happy to draft this test if useful.🤖 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 854 - 1139, The existing tests lack coverage for invalid execution horizons. Extend the authenticated metrics tests around `builtin_task_metrics` and the `mission` helper with an UpExecution mission using `prediction_horizon_secs` set to 21, and assert the metrics call returns `Err` rather than zeroed metrics; ensure the underlying `execution_task_metrics` path remains 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/crates/ploy-research/src/prediction_mcts_authenticated.rs`:
- Around line 202-263: Update settlement_task_metrics to ensure the
mean_log_loss calculation remains finite when q is exactly 0.0 or 1.0. Adjust
the log-loss computation around the existing q.ln() and (1.0 - q).ln() calls by
clamping probabilities or handling boundary outcomes explicitly, while
preserving the current binary-label scoring behavior and ensuring the resulting
artifact can be serialized and hashed consistently.
---
Nitpick comments:
In
`@rust_hft/prediction-markets/crates/ploy-research/src/prediction_mcts_authenticated.rs`:
- Around line 265-283: The execution_task_metrics options use unexplained
authenticated-evaluation overrides. Extract the $15 stake and per-decision
min-bucket settings into clearly named constants near the module defaults, add a
concise comment documenting why authenticated evaluation requires them instead
of the bucketed defaults, and use those constants in
FullDepthExecutionMatrixOptions.
- Around line 854-1139: The existing tests lack coverage for invalid execution
horizons. Extend the authenticated metrics tests around `builtin_task_metrics`
and the `mission` helper with an UpExecution mission using
`prediction_horizon_secs` set to 21, and assert the metrics call returns `Err`
rather than zeroed metrics; ensure the underlying `execution_task_metrics` path
remains 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: 043dfc0e-803c-42e2-b910-f0877dcc65d4
📒 Files selected for processing (3)
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_authenticated.rs
Bind snapshot execution settings, reject incomplete horizon and joint evidence plus empty settlement cohorts, and recheck the metric deadline.
Change contract
Evaluate the selected canonical shared-MCTS candidate against authenticated settlement, Up-execution, or Down-execution snapshot views; emit a typed, self-digested evaluator artifact and fail closed when any admitted decision, joint binary book, task identity, candidate identity, requested horizon evidence, snapshot-bound execution setting, or settlement metric is missing, mismatched, or non-finite.
This is the already-specified evaluator layer of #324; a separate PRD is unnecessary because #324 and its OWNER clarification define the behavior contract and authority boundary.
Out of scope
Dependency / merge order
#405 and #407 are merged. This PR is rebased directly onto main at 5f7ca6c; no stacked dependency remains.
Atomic scope exception
The complete diff is 1007 additions and 13 deletions across one substantive evaluator file plus two required policy-digest template syncs. Codex reviewers /root/review_324_joint_spec and /root/review_324_joint_standards explicitly approved the atomic exception. Snapshot-bound settings, exact decision and horizon coverage, joint-depth validity, finite typed metrics, artifact self-validation, and post-compute deadlines are one fail-closed evaluator trust boundary. Splitting would permit an evaluator with known incomplete authenticated guarantees to merge or roll back independently.
Focused validation
Targeted counterexamples cover snapshot-bound stake and quote age, missing requested-horizon observation or book, hashed but invalid joint depth, empty settlement held-out evidence, post-compute deadline expiry, exact-boundary finite log loss for either outcome, missing settlement joint quotes, missing sibling execution books, task metric substitution, candidate or task identity drift, label isolation, and artifact digest tampering.
Rollout / rollback impact
Research-only artifact behavior. No collector, runtime, execution, deployment, or live-trading state changes. Roll back by reverting this PR; existing authenticated snapshots and shared-kernel state remain immutable and fail closed.
Refs #324