fix(polymarket): admit bounded quote recovery - #286
Conversation
📝 WalkthroughWalkthroughEvent-local Polymarket validation now permits bounded quote recovery, adds selected-token quote coverage checks, and preserves strict validation for reference segments and non-event-local imports. ChangesPolymarket quote recovery
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 (1)
rust_hft/tools/collector/src/polymarket_research_select.rs (1)
133-165: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the earlier fallback when validating quote recovery gaps.
The recovery message is only reachable after a failure and before any successful quote has set
last_recorded_at. For the interrupted-stream case—where an earlier quote exists—the code rejects as a generic availability gap before reaching the recovery branch, so callers may not see the intended recovery-specific validation. Either surface the same recovery failure for recovered-outstanding failures, or fold it back into a clearer recovery-specific check.🤖 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/tools/collector/src/polymarket_research_select.rs` around lines 133 - 165, Update the quote handling branch in the collector so pending_failure_at is validated before the last_recorded_at availability-gap check. Ensure interrupted streams with an earlier quote use the recovery-specific 30-second validation and error, while retaining the existing gap checks for quotes without a pending failure.
🤖 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/tools/collector/src/polymarket_research_select.rs`:
- Around line 133-165: Update the quote handling branch in the collector so
pending_failure_at is validated before the last_recorded_at availability-gap
check. Ensure interrupted streams with an earlier quote use the
recovery-specific 30-second validation and error, while retaining the existing
gap checks for quotes without a pending failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 97303ac7-e991-4bd1-907b-d22f1659f625
📒 Files selected for processing (2)
rust_hft/tools/collector/src/polymarket_research_import.rsrust_hft/tools/collector/src/polymarket_research_select.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 06dbcb55c0
ℹ️ 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".
| if state.pending_failure_at.is_some() { | ||
| bail!("selected token {token} has an unresolved quote collection failure"); | ||
| } | ||
| let coverage_end = segment_end.min(contract.event_end); |
There was a problem hiding this comment.
When a tape with a recovered quote failure ends before the selected contract does—for example, collection stops at 05:01 for a 05:00–05:05 market—using the observed segment_end as the boundary lets the last quote at shutdown pass even though the remaining market interval is absent. This artifact can reach the relaxed path because scan_tape always emits source_session_closed: true and does not prove that the UTC partition completed (polymarket_upload.rs:1389-1412); before this change its disclosed failure made it noncanonical and fail closed. Measure the trailing gap against the earlier of event_end and the segment's UTC-hour boundary rather than the last observed record.
Useful? React with 👍 / 👎.
Change contract
For explicit market evidence only, accept a source-closed, content-addressed market tape whose global canonical/complete flags are false because of unrelated market coverage or disclosed transient quote failures, provided every selected Up/Down token has full-depth 1-second quotes and each selected failure/gap recovers within 30 seconds.
Out of scope
Collector runtime or deployment changes; reference acquisition; verifier semantics owned by #284/#285; snapshot, evaluator/MCTS, and publication logic; fabricated or rewritten source evidence.
Dependency / merge order
#285 is merged at dc0331d. This PR must merge before evidence compilation in #270. No stacked PR remains.
Focused validation
Rollout / rollback impact
The relaxation is reachable only through explicit-market normalization; the strict importer remains fail-closed. Rollback is a single commit and restores rejection of globally noncanonical tapes. No runtime collector rollout occurs in this PR.
A PRD is unnecessary because this is the focused defect contract already scoped in #283 and implemented test-first.
Closes #283
Summary by CodeRabbit
New Features
Bug Fixes