fix(research): bind typed Mission snapshot admission - #374
Conversation
📝 WalkthroughWalkthroughThe snapshot admission protocol is upgraded to v2 with typed task and immutable cohort identity bindings. Mission JSON is fetched and verified before admission, sibling I/O gains bounded deadline handling, and admitted identities flow into validation and Kubernetes job annotations. ChangesSnapshot admission v2
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
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.
🧹 Nitpick comments (3)
rust_hft/prediction-markets/crates/ploy-research/src/bin/monday-prediction-snapshot.rs (2)
1061-1063: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWire test emits the new v2 fields but never asserts them.
task,cohort_partition_id, andcohort_manifest_idare added to the fixture, yet the sha256-shape loop at Lines 1078-1083 skips both cohort ids and nothing checksresponse["task"].♻️ Extend the assertions to the v2 identity fields
+ assert_eq!(response["task"]["kind"], "settlement_probability"); for field in [ "snapshot_contract_id", "partition_digest", "policy_identity", + "cohort_partition_id", + "cohort_manifest_id", "immutable_image_identity", ] {🤖 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 1061 - 1063, Extend the wire test assertions near the existing sha256-shape loop to validate both cohort_partition_id and cohort_manifest_id, and add an assertion that response["task"] matches the settlement_probability fixture value. Keep the existing assertions unchanged.
956-1017: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the
invalid_missionbranch and the remaining bound fields.The test exercises
Ok,cohort_manifest_idmismatch, andtaskmismatch, but notinvalid_mission(unparseable JSON, a V2 mission, or a mission failingvalidate_prediction_mission_v3) nor mismatches onmission_id,snapshot_contract_id, andsearch_policy_snapshot_id. Those are the identity bindings this PR exists to enforce, so a regression that drops one of them would pass today.🤖 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 956 - 1017, Extend mission_admission_identity_binds_distinct_partition_and_cohort_manifest_ids to assert Err("invalid_mission") for unparseable mission JSON, a V2 mission, and a V3 mission rejected by validate_prediction_mission_v3. Add mismatch cases for mission_id, snapshot_contract_id, and search_policy_snapshot_id, each asserting Err("mission_mismatch") through validate_mission_admission_identity while preserving the existing coverage.rust_hft/alpha-harness/app/src/prediction_dispatch.rs (1)
1539-1606: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRenamed test drops rejected-admission coverage and leaves a now-unreachable sibling fixture.
mission_urlis invalid, sovalidate_submissionfails before the sibling is ever spawned. The v2 rejection stub written at Line 1574 is never executed, and theexpect_errmessage at Line 1600 still describes the old rejected-admission scenario. The previous guarantee — a sibling-rejected admission must not reach kubectl — is no longer covered anywhere.Consider keeping this test as-is (with an accurate message and without the dead sibling stub) and adding a second test that uses a valid submission plus the v2 rejection stub to restore the rejection-path assertion.
♻️ Message and fixture cleanup for the early-validation test
- let _error = submit_with_binaries( + let error = submit_with_binaries( PredictionDispatchSubmitArgs { submission, context: "ack".to_owned(), namespace: "monday-research".to_owned(), }, &sibling, &kubectl, ) - .expect_err("rejected admission must fail dispatch after reporting a typed result"); + .expect_err("invalid submission must fail dispatch before admission"); + assert!(error.to_string().contains("mission"), "{error:#}"); assert!( !kubectl_log.exists(), "invalid submission must fail before any Kubernetes read or write" );🤖 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/alpha-harness/app/src/prediction_dispatch.rs` around lines 1539 - 1606, Update invalid_submission_never_reaches_kubectl to reflect early validate_submission failure: retain the invalid mission_url case, remove the unused executable sibling rejection fixture, and change the expect_err message to describe validation failure rather than rejected admission. Add a separate test covering sibling-rejected admission using a valid submission and the existing v2 rejection stub, asserting that submit_with_binaries fails before invoking kubectl.
🤖 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/alpha-harness/app/src/prediction_dispatch.rs`:
- Around line 1539-1606: Update invalid_submission_never_reaches_kubectl to
reflect early validate_submission failure: retain the invalid mission_url case,
remove the unused executable sibling rejection fixture, and change the
expect_err message to describe validation failure rather than rejected
admission. Add a separate test covering sibling-rejected admission using a valid
submission and the existing v2 rejection stub, asserting that
submit_with_binaries fails before invoking kubectl.
In
`@rust_hft/prediction-markets/crates/ploy-research/src/bin/monday-prediction-snapshot.rs`:
- Around line 1061-1063: Extend the wire test assertions near the existing
sha256-shape loop to validate both cohort_partition_id and cohort_manifest_id,
and add an assertion that response["task"] matches the settlement_probability
fixture value. Keep the existing assertions unchanged.
- Around line 956-1017: Extend
mission_admission_identity_binds_distinct_partition_and_cohort_manifest_ids to
assert Err("invalid_mission") for unparseable mission JSON, a V2 mission, and a
V3 mission rejected by validate_prediction_mission_v3. Add mismatch cases for
mission_id, snapshot_contract_id, and search_policy_snapshot_id, each asserting
Err("mission_mismatch") through validate_mission_admission_identity while
preserving the existing coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3235b0c8-5ce6-41d7-8fb9-4db8d1d11b82
📒 Files selected for processing (2)
rust_hft/alpha-harness/app/src/prediction_dispatch.rsrust_hft/prediction-markets/crates/ploy-research/src/bin/monday-prediction-snapshot.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 36d8d7b628
ℹ️ 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".
| let ParsedPredictionMission::V3(mission) = | ||
| parse_prediction_mission_json(request.mission_json.as_bytes()) | ||
| .map_err(|_| "invalid_mission")? | ||
| else { | ||
| return Err("invalid_mission"); | ||
| }; |
There was a problem hiding this comment.
Every request accepted here must now be Mission V3, but the resulting Job sends those same bytes through prediction_runner.rs, where PredictionMissionIdentity still requires the legacy lane and data_snapshot_id fields (lines 27-33), and then monday-prediction-research.rs deserializes them as the V2 PredictionResearchMission (lines 753-757). V3 has neither legacy field and denies compatibility extras, so every admitted submission fails before the evaluator starts; update both execution consumers in this rollout or keep V3 admission closed.
AGENTS.md reference: AGENTS.md:L86-L88
Useful? React with 👍 / 👎.
| validate_prediction_mission_v3(&mission).map_err(|_| "invalid_mission")?; | ||
| if mission.mission_id != request.mission_id | ||
| || mission.task != request.task | ||
| || mission.cohort_manifest_id != cohort_manifest_id | ||
| || mission.snapshot_contract_id != request.snapshot_contract_id | ||
| || mission.search_policy_snapshot_id != policy_identity |
There was a problem hiding this comment.
When a Mission declares the internally consistent polymarket_chainlink_binance profile and Binance capability but points at a cached baseline snapshot, this validation still succeeds because it compares only the task and digest fields and never derives an authenticated authority profile or capability set from the snapshot. The canonical admit_prediction_mission_v3 path explicitly rejects authenticated authority and capability mismatches in prediction_mission_v3.rs lines 235-239; bypassing those checks here lets the sibling report admission for a snapshot that lacks the Mission's required Binance inputs. Bind the cached snapshot's authenticated source/capabilities before returning admitted.
AGENTS.md reference: AGENTS.md:L53-L54
Useful? React with 👍 / 👎.
| let write_result = match write_receiver.recv_timeout(remaining_admission_time( | ||
| deadline, | ||
| "snapshot admission sibling request", | ||
| )?) { |
There was a problem hiding this comment.
When the Mission fetch consumes nearly all of the 30-second admission budget, the sibling can be spawned and then remaining_admission_time(...)? can return an error here before recv_timeout starts. That early return never calls terminate_admission_child, so a non-reading sibling can remain alive with the detached writer blocked on its stdin; the equivalent ? in the response phase has the same leak. Compute the remaining duration before spawning or explicitly kill and reap the child whenever the deadline check fails.
Useful? React with 👍 / 👎.
Change contract
Bind a SHA-verified, typed Mission V3 task and distinct cohort/partition identities to snapshot admission. Only the supported BTC 5m settlement-probability task is admitted.
Out of scope
Snapshot construction, evaluator/MCTS execution, collector deployment or cutover, pipeline smoke execution, and result publication.
Dependency / merge order
Builds on merged #364/#334 and rebases onto current main. This is the prerequisite for completing #323; it does not promote #331 or start #326.
Focused validation
Rollout / rollback impact
The dispatcher and snapshot sibling move together to admission schema v2; no runtime cutover occurs in this PR. Revert this PR to roll back the protocol change.
Summary by CodeRabbit
New Features
Bug Fixes