feat(research): persist verified catalog partitions - #367
Conversation
|
Warning Review limit reached
Next review available in: 46 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 (6)
📝 WalkthroughWalkthroughThe change adds strict persisted Ready receipt reconstruction, bounded catalog partition artifact serialization, content-addressed readback verification, partition membership checks, public artifact APIs, and updated policy snapshot references. ChangesCatalog partition artifact lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant CatalogPartitionWriter
participant ContentAddressedStorage
participant CatalogPartitionReader
CatalogPartitionWriter->>ContentAddressedStorage: write canonical content-addressed artifact
ContentAddressedStorage-->>CatalogPartitionWriter: return artifact reference and digests
CatalogPartitionReader->>ContentAddressedStorage: bounded verified read
ContentAddressedStorage-->>CatalogPartitionReader: return artifact bytes
CatalogPartitionReader->>CatalogPartitionReader: validate canonical payload, policy, digests, and receipt membership
🚥 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: a60d4f65cd
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
rust_hft/prediction-markets/crates/ploy-market-data/src/polymarket_evidence/catalog.rs (3)
357-357: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a message to this bare
ensure!.Every other check in
into_receiptcarries a diagnostic; this one degrades to anyhow's generic "Condition failed" text, which is the least helpful place to lose context (it also silently conflates "empty" with "untrimmed").♻️ Suggested change
- ensure!(!self.market_id.trim().is_empty() && self.market_id.trim() == self.market_id); + ensure!( + !self.market_id.trim().is_empty() && self.market_id.trim() == self.market_id, + "persisted Ready catalog receipt {} has an empty or untrimmed market_id", + self.receipt_sha256 + );🤖 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-market-data/src/polymarket_evidence/catalog.rs` at line 357, Update the ensure! validation in into_receipt to include a diagnostic message that identifies the market_id requirement and distinguishes empty values from values with surrounding whitespace; preserve the existing validation condition and add context comparable to the other checks in the method.
1142-1215: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering the duplicate-receipt guards.
The uniqueness branches at Lines 234-245 (duplicate
receipt_sha256, duplicatemarket_id) are the only new invariants infrom_persisted_ready_receiptswith no test. A second copy ofreceiptin the input vec exercises both cheaply.🤖 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-market-data/src/polymarket_evidence/catalog.rs` around lines 1142 - 1215, Extend persisted ready receipt coverage in persisted_ready_catalog_rejects_non_ready_extra_and_rehashed_receipts by passing two copies of the same receipt to from_persisted_ready_receipts and asserting it rejects the duplicate receipt_sha256 or market_id invariant. Ensure the test verifies the duplicate-receipt error rather than only successful parsing.
225-249: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueDuplicate
market_idscan is O(n²).With
MAX_READY_CATALOG_ENTRIES = 512this is harmless today, but aBTreeSet<String>of seen market IDs keeps the intent explicit and linear. Also note this constructor itself enforces no entry-count cap; on the read path the only bound is the 8 MiB artifact size (seeread_catalog_partition_artifact).🤖 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-market-data/src/polymarket_evidence/catalog.rs` around lines 225 - 249, Update from_persisted_ready_receipts to track market IDs in a BTreeSet<String> and perform duplicate checks through set insertion, keeping duplicate market_id errors intact while making validation linear. Ensure the constructor enforces MAX_READY_CATALOG_ENTRIES, rather than relying only on the 8 MiB bound in read_catalog_partition_artifact.rust_hft/prediction-markets/crates/ploy-research/src/prediction_loop_fs.rs (1)
344-357: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDigest and content-addressed-filename checks are copied verbatim from
verify_artifact.Lines 344-357 duplicate Lines 306-319. Extracting a small
verify_artifact_bytes(path, artifact, body)helper that both call keeps the two integrity gates from drifting — andverify_artifactitself could then just delegate to the bounded reader with its own limit.🤖 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_loop_fs.rs` around lines 344 - 357, Extract the duplicated digest and content-addressed filename validation from verify_artifact and the surrounding evidence-reading flow into a shared verify_artifact_bytes helper accepting path, artifact, and body. Have both callers delegate to this helper while preserving the existing hash-mismatch and filename-validation errors; keep verify_artifact’s bounded reader behavior intact.rust_hft/prediction-markets/crates/ploy-research/src/event_cohort_partition.rs (2)
207-238: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftCohort assignment and cutoff rules are now duplicated.
Lines 207-238 re-implement the exact train/crossing/held-out classification, label cutoff derivation, and training-label check that
EventCohortPartition::from_ready_entriesalready owns. Any future change to the boundary rule has to be applied in both places or readback silently diverges from construction.Extracting a shared helper that both call (returning the three ID vectors plus the cutoff) keeps this one rule in one place.
🤖 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/event_cohort_partition.rs` around lines 207 - 238, Extract the train/crossing/held-out classification, label cutoff derivation, and training-label validation from the current readback block into a shared helper used by both EventCohortPartition::from_ready_entries and the persisted-assignment validation path. Have the helper return the three market-ID vectors and computed cutoff, then compare or validate those results without duplicating boundary logic.
277-301: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winHash the persisted payload instead of the write-time payload.
write_catalog_partition_artifact()hashesCatalogPartitionArtifactPayload<'_>(receipts as&PolymarketCatalogReceipt), but verification reproaches the stored envelope’sPersistedCatalogPartitionArtifactPayload(catalog_receipts: Vec<Value>). Keep the current change if the receipt payload must match the provenance format, but avoid the implicit write/read JSON round-trip invariant by hashing after building the persisted payload.🤖 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/event_cohort_partition.rs` around lines 277 - 301, Update write_catalog_partition_artifact() to construct the persisted PersistedCatalogPartitionArtifactPayload first, then compute payload_sha256 from its canonical JSON bytes rather than hashing CatalogPartitionArtifactPayload. Use that same persisted payload in the envelope so the stored hash matches verification’s representation.
🤖 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/config/research_missions/polymarket-btc-5m.example.json`:
- Line 18: Regenerate the policy snapshot identity using
current_prediction_policy_snapshot_id() on the final tree, then update
search_policy_snapshot_id in both
rust_hft/prediction-markets/config/research_missions/polymarket-btc-5m.example.json:18-18
and
rust_hft/prediction-markets/config/research_missions/polymarket-sol-5m.example.json:18-18
to the resulting value.
In
`@rust_hft/prediction-markets/crates/ploy-research/src/event_cohort_partition.rs`:
- Around line 373-375: After reconstructing the catalog and partition in the
read path, invoke the existing validate_catalog_partition_write_bounds check
before returning or using the partition. Ensure the reconstructed Ready catalog
is rejected when its entry count exceeds MAX_READY_CATALOG_ENTRIES, preserving
the same fail-closed bounds enforced during writes.
---
Nitpick comments:
In
`@rust_hft/prediction-markets/crates/ploy-market-data/src/polymarket_evidence/catalog.rs`:
- Line 357: Update the ensure! validation in into_receipt to include a
diagnostic message that identifies the market_id requirement and distinguishes
empty values from values with surrounding whitespace; preserve the existing
validation condition and add context comparable to the other checks in the
method.
- Around line 1142-1215: Extend persisted ready receipt coverage in
persisted_ready_catalog_rejects_non_ready_extra_and_rehashed_receipts by passing
two copies of the same receipt to from_persisted_ready_receipts and asserting it
rejects the duplicate receipt_sha256 or market_id invariant. Ensure the test
verifies the duplicate-receipt error rather than only successful parsing.
- Around line 225-249: Update from_persisted_ready_receipts to track market IDs
in a BTreeSet<String> and perform duplicate checks through set insertion,
keeping duplicate market_id errors intact while making validation linear. Ensure
the constructor enforces MAX_READY_CATALOG_ENTRIES, rather than relying only on
the 8 MiB bound in read_catalog_partition_artifact.
In
`@rust_hft/prediction-markets/crates/ploy-research/src/event_cohort_partition.rs`:
- Around line 207-238: Extract the train/crossing/held-out classification, label
cutoff derivation, and training-label validation from the current readback block
into a shared helper used by both EventCohortPartition::from_ready_entries and
the persisted-assignment validation path. Have the helper return the three
market-ID vectors and computed cutoff, then compare or validate those results
without duplicating boundary logic.
- Around line 277-301: Update write_catalog_partition_artifact() to construct
the persisted PersistedCatalogPartitionArtifactPayload first, then compute
payload_sha256 from its canonical JSON bytes rather than hashing
CatalogPartitionArtifactPayload. Use that same persisted payload in the envelope
so the stored hash matches verification’s representation.
In `@rust_hft/prediction-markets/crates/ploy-research/src/prediction_loop_fs.rs`:
- Around line 344-357: Extract the duplicated digest and content-addressed
filename validation from verify_artifact and the surrounding evidence-reading
flow into a shared verify_artifact_bytes helper accepting path, artifact, and
body. Have both callers delegate to this helper while preserving the existing
hash-mismatch and filename-validation errors; keep verify_artifact’s bounded
reader behavior intact.
🪄 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: 9181f983-fb6e-4d1e-9ea3-4618ba57e324
📒 Files selected for processing (6)
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-market-data/src/polymarket_evidence/catalog.rsrust_hft/prediction-markets/crates/ploy-research/src/event_cohort_partition.rsrust_hft/prediction-markets/crates/ploy-research/src/lib.rsrust_hft/prediction-markets/crates/ploy-research/src/prediction_loop_fs.rs
Change contract
Persist and independently read back a bounded, content-addressed #319 Ready catalog plus #322 EventCohortPartition artifact without reconstructing or resplitting either input.
Acceptance evidence
cargo test -p ploy-research: 318 lib + 2 bin tests passed.cargo test -p ploy-market-data: 71 tests passed.ploy-market-contractsderivable_implslints.Out of scope
Admission CLI protocol (#364), alpha-harness dispatcher (#334), Kubernetes, evaluator/MCTS, collector deployment, Paper, Shadow, Live, and promotion.
Dependency / merge order
Base:
mainat15f266aa. Depends on merged #319/#322; unblocks #364, then #334 and #323.Atomic exception
971 lines exceed the 750-line review threshold. Approved by
/root: strict catalog recovery, partition artifact/readback, shared bounded verified reader, and required policy-template pins are one inseparable trust-boundary contract. Splitting would publish an artifact that cannot be safely admitted or independently rolled back.Rollout / rollback
No runtime rollout. The artifact is opt-in for a later #364 CLI. Reverting this PR removes the readback seam without changing collector, dispatcher, or execution behavior.
Summary by CodeRabbit
New Features
Configuration
Tests