feat(research): materialize authenticated ready snapshots - #363
Conversation
|
Warning Review limit reached
Next review available in: 43 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 (4)
📝 WalkthroughWalkthroughAdds authenticated Polymarket cohort validation and sealed research-snapshot caching, exposes the new APIs, adds coverage for rejection and cache reuse, and refreshes BTC/SOL mission search-policy snapshot identifiers. ChangesAuthenticated research snapshots
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ReadyEventCatalog
participant authenticate_ready_event_cohort
participant materialize_authenticated_research_snapshot
participant FilesystemCache
ReadyEventCatalog->>authenticate_ready_event_cohort: catalog and partition
authenticate_ready_event_cohort->>materialize_authenticated_research_snapshot: authenticated cohort
materialize_authenticated_research_snapshot->>FilesystemCache: load and verify sealed snapshot
FilesystemCache-->>materialize_authenticated_research_snapshot: cached snapshot or cache miss
materialize_authenticated_research_snapshot->>FilesystemCache: build, write, and seal snapshot
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: f175f23286
ℹ️ 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".
63f3cbd to
d3b423e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
rust_hft/prediction-markets/crates/ploy-research/src/research_snapshot.rs (3)
586-591: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
require_identityistrueat every call site.All three callers (Lines 504, 515, 526) pass
true, so the flag is unexercised flexibility. Dropping the parameter keeps the admission path unconditional and removes a knob that could later be flipped to skip identity binding checks.🤖 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 586 - 591, Remove the require_identity parameter from validate_authenticated_snapshot and update all three callers to stop passing true. Keep identity binding validation unconditional within the function, preserving the existing authenticated snapshot admission behavior.
4640-4656: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider asserting the seal positively, not just its absence.
unsealed_cache_is_rejected_without_rebuildingonly covers marker deletion. A companion assertion that every path fromsnapshot_cache_pathsis read-only after a successful materialize would pin the read-only half of the seal contract, which is currently untested.🤖 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 4640 - 4656, Extend unsealed_cache_is_rejected_without_rebuilding with a companion assertion after successful materialize_authenticated_research_snapshot: enumerate every path returned by snapshot_cache_paths and verify each is read-only, while preserving the existing marker-removal and no-rebuild rejection checks.
260-271: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider implementing
Display/std::error::Errorfor the exported rejection enum.
AuthenticatedResearchSnapshotRejectionis re-exported fromlib.rs, but downstream callers can only{:?}it or hand-match every variant to surface a reason. ADisplay(plusError) impl keeps rejection reasons usable inanyhowchains and logs without each caller re-deriving the mapping.🤖 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 260 - 271, Implement std::fmt::Display and std::error::Error for the exported AuthenticatedResearchSnapshotRejection enum. In Display, map every variant to a clear message that includes its reason field, preserving the existing variant distinctions so downstream callers can use the rejection directly in logs and anyhow error chains.
🤖 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/research_snapshot.rs`:
- Around line 492-531: Change the materialization flow around
write_research_snapshot and seal_snapshot_cache to build and seal the snapshot
in a unique staging directory under request.cache_root, then atomically rename
it to snapshot_dir only after sealing succeeds. Do not treat an existing but
incomplete final directory as a valid cache hit; handle rename races by
discarding the staging directory and loading the now-published snapshot through
the existing cache-hit path.
- Around line 720-729: Update the side matching in the snapshot book validation
around the `books` entry and
`AuthenticatedResearchSnapshotRejection::TokenMismatch` branch to compare sides
case-insensitively, matching `polymarket_chainlink_baseline_ticks`. Preserve the
existing token-ID checks and mismatch rejection while accepting uppercase,
lowercase, and mixed-case representations of UP and DOWN.
---
Nitpick comments:
In `@rust_hft/prediction-markets/crates/ploy-research/src/research_snapshot.rs`:
- Around line 586-591: Remove the require_identity parameter from
validate_authenticated_snapshot and update all three callers to stop passing
true. Keep identity binding validation unconditional within the function,
preserving the existing authenticated snapshot admission behavior.
- Around line 4640-4656: Extend unsealed_cache_is_rejected_without_rebuilding
with a companion assertion after successful
materialize_authenticated_research_snapshot: enumerate every path returned by
snapshot_cache_paths and verify each is read-only, while preserving the existing
marker-removal and no-rebuild rejection checks.
- Around line 260-271: Implement std::fmt::Display and std::error::Error for the
exported AuthenticatedResearchSnapshotRejection enum. In Display, map every
variant to a clear message that includes its reason field, preserving the
existing variant distinctions so downstream callers can use the rejection
directly in logs and anyhow error chains.
🪄 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: 7a35c3a4-ed1c-43d4-aa56-eeb4ff414219
📒 Files selected for processing (4)
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/research_snapshot.rs
Change contract: Materialize authenticated catalog plus immutable partition into a content-addressed ResearchSnapshot with fresh verified readback and an opaque admitted/rejected handle.
Out of scope: Alpha-harness dispatcher, Job creation, Mission semantics, MCTS, collector deployment, schedulers, databases, Paper, Shadow, and Live.
Dependency / merge order: Depends on #319 and #322/#362, both merged; merge before #334/#323.
Focused validation: focused cache reuse/corrupt cache/template tests;
cargo test -p ploy-research(308 library + 2 binary tests passed).Rollout / rollback impact: Research-only snapshot materialization. Revert this PR to remove the admission/cache seam; no runtime collector or execution behavior changes.
Atomic exception: 4 files, +792/-10. Reviewer
/rootapproved the inseparable authenticated cohort -> identity -> fresh readback -> opaque handle contract; splitting would create a fail-open read/write seam.Summary by CodeRabbit
New Features
Configuration
Tests