fix(collector): report deduped tape rows in the canonical quality field

[proerror77]

Change contract

Follow-up to #746 (auto-merged during review resolution). The original change reported the dedup count in a new top-level duplicate_trade_rows manifest key while the existing quality.duplicate_record_ids field stayed hardcoded at 0 — a stale counter and two parallel fields for one number (raised in the #746 review thread, now resolved).

Report the dedup count only in the canonical quality.duplicate_record_ids field and drop the redundant top-level key. Scan behavior is unchanged.

Issue relationship

None

Out of scope

None beyond the field placement.

Dependencies and merge order

Builds on #746 (already merged).

Focused validation

• Updated test asserts quality.duplicate_record_ids == 1 on a duplicate tape; cargo test -p hft-collector --locked polymarket_upload — 87 passed.

Rollout and rollback

No production impact at merge. Rollback is revert.

Scope exception

None

Summary by CodeRabbit

• Bug Fixes
  • Duplicate trade records are now accepted during tape validation instead of causing failures.
  • Duplicate counts are recorded in validation quality metrics.
  • Event totals continue to include all submitted records.
  • Updated validation coverage confirms duplicate-record handling.

[coderabbitai]

📝 Walkthrough

Walkthrough

The tape scanner now accepts duplicate polymarket_trade rows. It records the duplicate count in quality.duplicate_record_ids while preserving trade and total event counts. The related test verifies successful scanning and manifest evidence.

Changes

Polymarket duplicate trade handling

Layer / File(s)	Summary
Duplicate trade scanning and manifest reporting rust_hft/tools/collector/src/polymarket_upload.rs	The scanner counts duplicate trade rows, accepts repeated record IDs, and reports the count in the manifest quality data.
Duplicate trade validation test rust_hft/tools/collector/src/polymarket_upload.rs	The test verifies successful scanning, one recorded duplicate, and preserved trade and total event counts.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

• proerror77/monday#746: Modifies duplicate polymarket_trade handling in the same module.
• proerror77/monday#152: Extends trade-row validation to record duplicate IDs in the manifest.
• proerror77/monday#34: Introduces related tape validation and duplicate-trade handling.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the main change: reporting deduplicated tape rows in the canonical quality field.
Description check	✅ Passed	The description includes every required template section and provides the behavior, rationale, validation, dependencies, and rollback details.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 💡 2

⚔️ Resolve merge conflicts 💡

• Resolve merge conflict in branch codex/polymarket-upload-dedup-scan

🛠️ Fix failing CI checks 💡

• Create stacked PR
• Commit on current branch

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch codex/polymarket-upload-dedup-scan

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

rust_hft/tools/collector/src/polymarket_upload.rs (1)

4514-4517: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

Assert that the obsolete top-level field is absent.

The test verifies quality.duplicate_record_ids, but it does not verify removal of duplicate_trade_rows. Add the absence assertion so the manifest cannot emit both fields again.

The PR objective removes the top-level duplicate_trade_rows field.

Proposed regression assertion

         let manifest = scan_tape(&tape, "crypto_expiry_reference", 0, 0).unwrap();
+        assert!(manifest.get("duplicate_trade_rows").is_none());
         assert_eq!(manifest["quality"]["duplicate_record_ids"], json!(1));

🤖 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_upload.rs` around lines 4514 - 4517,
Update the manifest assertions in the scan_tape test to verify that the
top-level duplicate_trade_rows field is absent, while preserving the existing
quality.duplicate_record_ids and event assertions.

🤖 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_upload.rs`:
- Around line 4514-4517: Update the manifest assertions in the scan_tape test to
verify that the top-level duplicate_trade_rows field is absent, while preserving
the existing quality.duplicate_record_ids and event assertions.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: bf3b9e93-f007-473b-9817-fdfceccddde7

📥 Commits

Reviewing files that changed from the base of the PR and between 1f7e740 and bffa63b.

📒 Files selected for processing (1)

• rust_hft/tools/collector/src/polymarket_upload.rs

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bffa63bede

ℹ️ 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".

[proerror77]

Review follow-ups:

P1 (remove duplicate rows before archiving): Deliberately out of scope for this fix, by design. A duplicate record id is a SHA-256 over the complete trade identity, so the repeated row is content-identical to the one already archived — keeping it in the artifact is lossless for downstream consumers (they deduplicate by record id) and preserves the crash-recovery audit trail that produced the re-emission (the incident showed these rows are exactly how we diagnose recovery bugs). This mirrors the collector-side #743 semantics: dedupe for processing, keep the evidence. Physically rewriting artifact bytes would also invalidate the sha256 chain between collector state, tape, and manifest. If a future change wants byte-level dedup at archive time, it should be its own contract with downstream verification.

P2 (stale manifests from #746-era uploads): Not applicable operationally. No binary carrying #746 was deployed before this follow-up merged: all uploads during the incident window ran the pre-#746 artifact (5e34169a / 7b8a61a), whose scanner hard-failed on duplicates and emitted no manifests for duplicate-bearing tapes. The first manifests with a non-zero duplicate count will come from builds that include both changes.