fix(research): bind sealed snapshot identity to Mission v3 - #381
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 (10)
📝 WalkthroughWalkthroughMission v4 admission now binds pipeline-smoke execution to authenticated cohort, partition, policy, contract, source, and snapshot identities. CLI, dispatch, runner validation, snapshot admission, tests, fixtures, configuration, and architecture documentation were updated accordingly. ChangesMission v4 authenticated admission
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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: bd2f69e7a7
ℹ️ 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".
bd2f69e to
8fd2d50
Compare
7fc77a2 to
e70bd0b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rust_hft/prediction-markets/crates/ploy-research/src/bin/monday-prediction-snapshot.rs (1)
223-233: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winCompare
snapshot_contract_idagainst the admitted snapshot.The protocol emits
snapshot.snapshot_contract_id()and admits ifmission.snapshot_hashmatches the admitted hash, but still accepts the mission whenmission.snapshot_contract_idmatchesrequest.snapshot_contract_idinstead. A caller-supplied contract id can drift from the admitted snapshot; pass the admitted contract id tovalidate_mission_admission_identityand compare it against the mission.🤖 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-snapshot.rs` around lines 223 - 233, The mission admission check currently trusts the caller-supplied request contract ID instead of the admitted snapshot’s contract ID. Update the call to validate_mission_admission_identity to pass snapshot.snapshot_contract_id(), then ensure the snapshot_contract_id comparison inside the validation condition uses that admitted value against mission.snapshot_contract_id.
🧹 Nitpick comments (2)
rust_hft/prediction-markets/crates/ploy-research/src/research_snapshot.rs (1)
5144-5146: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the exported schema constant instead of the literal.
PREDICTION_MISSION_V3_SCHEMA_VERSIONis public; hard-coding"prediction_research_mission.v4"here means a future bump silently fails this test on the version check rather than on the identity assertions it is meant to exercise.🤖 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/research_snapshot.rs` around lines 5144 - 5146, Update the PredictionResearchMissionV3 construction to use the exported PREDICTION_MISSION_V3_SCHEMA_VERSION constant for schema_version instead of the hard-coded string, preserving the test’s intended version behavior.rust_hft/prediction-markets/crates/ploy-research/src/prediction_mission_v3.rs (1)
316-328: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse a shared lowercase-hex predicate.
This byte check duplicates
validate_sha256_id's hex validation. Extracting oneis_lowercase_hex(&str, len)helper keeps both digest formats from drifting.🤖 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_mission_v3.rs` around lines 316 - 328, Extract the shared lowercase-hex validation from validate_sha256_id into an is_lowercase_hex(&str, len) helper, then update both validate_sha256_id and validate_snapshot_hash to reuse it with their required lengths. Preserve each validator’s existing error messages and validation behavior.
🤖 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/alpha-harness/app/src/prediction_runner.rs`:
- Around line 943-970: Strengthen
execute_rejects_forged_pipeline_partition_before_starting_the_runner by
asserting the returned error contains the expected forged partition
identity-rejection text, rather than only checking is_err(). Match the sibling
tests’ verification that the runner executable was never started, using the
existing must-not-run path, so the test specifically validates the admission
gate.
---
Outside diff comments:
In
`@rust_hft/prediction-markets/crates/ploy-research/src/bin/monday-prediction-snapshot.rs`:
- Around line 223-233: The mission admission check currently trusts the
caller-supplied request contract ID instead of the admitted snapshot’s contract
ID. Update the call to validate_mission_admission_identity to pass
snapshot.snapshot_contract_id(), then ensure the snapshot_contract_id comparison
inside the validation condition uses that admitted value against
mission.snapshot_contract_id.
---
Nitpick comments:
In
`@rust_hft/prediction-markets/crates/ploy-research/src/prediction_mission_v3.rs`:
- Around line 316-328: Extract the shared lowercase-hex validation from
validate_sha256_id into an is_lowercase_hex(&str, len) helper, then update both
validate_sha256_id and validate_snapshot_hash to reuse it with their required
lengths. Preserve each validator’s existing error messages and validation
behavior.
In `@rust_hft/prediction-markets/crates/ploy-research/src/research_snapshot.rs`:
- Around line 5144-5146: Update the PredictionResearchMissionV3 construction to
use the exported PREDICTION_MISSION_V3_SCHEMA_VERSION constant for
schema_version instead of the hard-coded string, preserving the test’s intended
version 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 Plus
Run ID: fcca190c-c15e-469a-8d07-60f3d798d75a
📒 Files selected for processing (10)
docs/architecture/PREDICTION_MARKETS.mdrust_hft/alpha-harness/app/src/cli.rsrust_hft/alpha-harness/app/src/prediction_dispatch.rsrust_hft/alpha-harness/app/src/prediction_runner.rsrust_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-research.rsrust_hft/prediction-markets/crates/ploy-research/src/bin/monday-prediction-snapshot.rsrust_hft/prediction-markets/crates/ploy-research/src/prediction_mission_v3.rsrust_hft/prediction-markets/crates/ploy-research/src/research_snapshot.rs
e70bd0b to
436de41
Compare
436de41 to
7c85cac
Compare
Change contract
Bind Mission v3 admission and checkpoint identity to the opaque, independently verified sealed ResearchSnapshot identity: cohort, partition, causal policy, snapshot contract/hash, and baseline source authority.
Acceptance evidence
cargo test -p ploy-research --lib(328 passed)cargo fmt --package ploy-research -- --checkandgit diff --checkOut of scope
MCTS mechanics, evaluator metrics, collector/runtime deployment, dispatch, Paper, Shadow, Live, and result publication.
Dependency / merge order
Follow-up to merged #320 and #378. Merge before #324; #324 consumes this sealed handoff.
Rollout / rollback impact
Research-only admission boundary. It fails closed on old checkpoint schema or any identity/source drift; reverting restores the prior absence of the handoff.
Closes #379.
Summary by CodeRabbit