fix(polymarket): disclose exact rescan mismatch values - #291
Conversation
📝 WalkthroughWalkthrough
ChangesRescan mismatch diagnostics
Estimated code review effort: 2 (Simple) | ~10 minutes 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.
Actionable comments posted: 1
🤖 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/tools/collector/src/polymarket_research_import.rs`:
- Around line 549-564: Update bounded_rescan_value so truncated output,
including the "...<truncated>" marker, never exceeds
MAX_RESCAN_DIAGNOSTIC_CHARS: reserve the marker length when collecting the
prefix. Add an assertion that the final truncated diagnostic length equals
MAX_RESCAN_DIAGNOSTIC_CHARS, while preserving unchanged output for values within
the limit.
🪄 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: 86192d26-f8f2-4045-be89-a68ea997780f
📒 Files selected for processing (1)
rust_hft/tools/collector/src/polymarket_research_import.rs
| const MAX_RESCAN_DIAGNOSTIC_CHARS: usize = 512; | ||
|
|
||
| fn bounded_rescan_value(value: Option<&Value>) -> String { | ||
| let raw = value.map_or_else(|| "<missing>".to_owned(), Value::to_string); | ||
| let mut chars = raw.chars(); | ||
| let prefix = chars | ||
| .by_ref() | ||
| .take(MAX_RESCAN_DIAGNOSTIC_CHARS) | ||
| .collect::<String>(); | ||
| if chars.next().is_some() { | ||
| format!("{prefix}...<truncated>") | ||
| } else { | ||
| raw | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep the truncation marker inside the 512-character limit.
The helper currently emits 512 value characters plus ...<truncated>, so truncated diagnostics exceed the stated maximum. Reserve the marker’s length from the prefix budget, and assert that the final diagnostic length equals MAX_RESCAN_DIAGNOSTIC_CHARS.
Proposed fix
const MAX_RESCAN_DIAGNOSTIC_CHARS: usize = 512;
fn bounded_rescan_value(value: Option<&Value>) -> String {
let raw = value.map_or_else(|| "<missing>".to_owned(), Value::to_string);
+ let marker = "...<truncated>";
+ let value_limit = MAX_RESCAN_DIAGNOSTIC_CHARS.saturating_sub(marker.chars().count());
let mut chars = raw.chars();
let prefix = chars
.by_ref()
- .take(MAX_RESCAN_DIAGNOSTIC_CHARS)
+ .take(value_limit)
.collect::<String>();
if chars.next().is_some() {
- format!("{prefix}...<truncated>")
+ format!("{prefix}{marker}")
} else {
raw
}
}- MAX_RESCAN_DIAGNOSTIC_CHARS + "...<truncated>".chars().count()
+ MAX_RESCAN_DIAGNOSTIC_CHARSAlso applies to: 1767-1775
🤖 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_import.rs` around lines 549
- 564, Update bounded_rescan_value so truncated output, including the
"...<truncated>" marker, never exceeds MAX_RESCAN_DIAGNOSTIC_CHARS: reserve the
marker length when collecting the prefix. Add an assertion that the final
truncated diagnostic length equals MAX_RESCAN_DIAGNOSTIC_CHARS, while preserving
unchanged output for values within the limit.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c5185fa6a
ℹ️ 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".
| const MAX_RESCAN_DIAGNOSTIC_CHARS: usize = 512; | ||
|
|
||
| fn bounded_rescan_value(value: Option<&Value>) -> String { | ||
| let raw = value.map_or_else(|| "<missing>".to_owned(), Value::to_string); |
There was a problem hiding this comment.
When a mismatch involves a large scan-derived collection such as trade_completions, Value::to_string materializes the entire JSON value before the iterator keeps 512 characters. Because rescans can derive these collections from multi-gigabyte tapes, the error path can allocate a very large temporary buffer or be OOM-killed instead of returning the intended bounded diagnostic; serialize into a size-limited writer rather than truncating only after full serialization.
Useful? React with 👍 / 👎.
| bail!("manifest field {field} does not match producer rescan"); | ||
| bail!( | ||
| "manifest {} field {field} does not match producer rescan: producer={}; rescan={}", | ||
| artifact.identity.file, |
There was a problem hiding this comment.
When validation receives multiple supported reference segments, including consecutive hours or same-hour fragments, the producer convention gives every segment the same basename such as market-updates.crypto_expiry_reference.ndjson.zst. Reporting only artifact.identity.file therefore produces the same label for every reference and does not identify which content-addressed artifact mismatched; include authenticated partition and digest information so an operator can locate the offending input.
Useful? React with 👍 / 👎.
Change contract
When exact-main evidence import rejects a producer manifest field after bound-FD rescan, report the authenticated artifact file plus bounded producer and rescan JSON values without weakening validation.
A PRD is unnecessary because issue #290 specifies one small error-message-only behavior and the change was developed with a focused failing test.
Acceptance evidence
bail!path; valid-input behavior is unchanged.cargo test -p hft-collectorpassed.cargo clippy -p hft-collector --all-targets --no-deps -- -D warningspassed.git diff --checkpassed.Out of scope
Accepting a new manifest shape; changing quality calculations; collector deployment; data repacking; evidence, snapshot, or evaluator behavior.
Dependency / merge order
Independent diagnostic prerequisite for the next #270 cloud compile attempt. Merge this PR, publish one exact-main compiler image, then rerun #270.
Focused validation
TDD on the existing producer-rescan mismatch counterexample, bounded-value test, full hft-collector tests, strict Clippy, formatting, and two-axis code review.
Rollout / rollback impact
Error-message-only change in a fail-closed one-shot compiler. Rollback removes diagnostics but does not alter accepted data.
Closes #290
Summary by CodeRabbit