feat(polymarket): publish immutable normalized evidence - #98
Conversation
📝 WalkthroughWalkthroughThe collector adds a ChangesPolymarket evidence publishing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as polymarket-raw-ops
participant Publisher as publish_polymarket_evidence
participant Validator as validate_dataset
participant Filesystem as bound artifact directory
CLI->>Publisher: submit evidence configuration and output root
Publisher->>Validator: validate evidence and generate report
Validator-->>Publisher: return validated evidence
Publisher->>Filesystem: install data, manifest, and _SUCCESS
Filesystem-->>Publisher: return published artifact paths
Publisher-->>CLI: print publication report as JSON
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 |
| let descriptor = unsafe { | ||
| libc::openat( | ||
| directory.as_raw_fd(), | ||
| name.as_ptr(), | ||
| libc::O_RDONLY | libc::O_CLOEXEC | libc::O_DIRECTORY | libc::O_NOFOLLOW, | ||
| ) | ||
| }; |
| return Err(std::io::Error::last_os_error()) | ||
| .with_context(|| format!("bind artifact directory {}", path.display())); | ||
| } | ||
| directory = unsafe { File::from_raw_fd(descriptor) }; |
| let result = unsafe { | ||
| libc::fstatat( | ||
| directory.as_raw_fd(), | ||
| name.as_ptr(), | ||
| stat.as_mut_ptr(), | ||
| libc::AT_SYMLINK_NOFOLLOW, | ||
| ) | ||
| }; |
| } | ||
| return Err(error).with_context(|| format!("inspect artifact target {}", path.display())); | ||
| } | ||
| let stat = unsafe { stat.assume_init() }; |
| let descriptor = unsafe { | ||
| libc::openat( | ||
| directory.as_raw_fd(), | ||
| name.as_ptr(), | ||
| libc::O_RDONLY | libc::O_CLOEXEC | libc::O_NOFOLLOW, | ||
| ) | ||
| }; |
| return Err(std::io::Error::last_os_error()) | ||
| .with_context(|| format!("open immutable artifact {}", path.display())); | ||
| } | ||
| let mut file = unsafe { File::from_raw_fd(descriptor) }; |
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_evidence_artifact.rs`:
- Around line 361-370: Make published artifact and manifest files read-only by
removing all write permission bits after creation, and re-read and validate
their bytes immediately before publishing _SUCCESS, including newly written data
and manifest files. Update the relevant completion test to assert zero write
bits while preserving a deliberate permission-change scenario that detects
conflicting bytes.
🪄 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
Run ID: b2869bf3-d634-4f47-81b2-df873b14fd82
📒 Files selected for processing (3)
rust_hft/tools/collector/src/bin/polymarket-raw-ops.rsrust_hft/tools/collector/src/lib.rsrust_hft/tools/collector/src/polymarket_evidence_artifact.rs
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
b011983 to
068983f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 068983f6b3
ℹ️ 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".
| drop(output); | ||
| verify_bound_directory(directory_path, directory)?; | ||
| let linked = unsafe { |
There was a problem hiding this comment.
When the selected output_root is writable by another process/user, closing the temp file before the path-based linkat leaves the temporary pathname replaceable: a racer can unlink .tmp, create different bytes at the same name, and have this code link that replacement as the final artifact. Because the successful link path is not read back before publishing _SUCCESS, the marker can attest bytes that this command did not write; keep the fd-bound inode through publication or verify the linked target before success can be installed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9281195: the temp FD now remains open through linkat, and the linked entry identity plus bytes/mode are verified before publication can continue.
| | libc::O_EXCL | ||
| | libc::O_CLOEXEC | ||
| | libc::O_NOFOLLOW, | ||
| 0o600, |
There was a problem hiding this comment.
When the publisher runs under a collector/service account and downstream evidence importers run as a different user or group, the final data, manifest, and _SUCCESS files inherit this temp file's 0600 mode because they are hard-linked from the temp inode. The command can report a successful content-addressed artifact that downstream consumers cannot open; create the temp with the normal shared artifact mode, or otherwise set intended readable permissions before linking.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9281195: data, manifest, and _SUCCESS are published as exact 0444 files, so downstream users can read them while no write bits remain.
068983f to
9281195
Compare
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_evidence_artifact.rs`:
- Around line 351-360: Change the artifact finalization flow around
temporary_identity and linkat so linking uses a non-replaceable staging location
or the already-open descriptor, rather than resolving the replaceable temporary
pathname. On identity mismatch, safely remove the mismatched final target and
ensure cleanup cannot unlink an attacker-replaced entry. Add a targeted
counterexample test that swaps the temporary directory entry immediately before
linking and verifies the immutable artifact is not poisoned.
🪄 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
Run ID: 125b2400-396c-46d5-8c1a-6fd67b2a81a7
📒 Files selected for processing (3)
rust_hft/tools/collector/src/bin/polymarket-raw-ops.rsrust_hft/tools/collector/src/lib.rsrust_hft/tools/collector/src/polymarket_evidence_artifact.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- rust_hft/tools/collector/src/bin/polymarket-raw-ops.rs
- rust_hft/tools/collector/src/lib.rs
Constraint: Downstream importers need content-addressed collector evidence without inheriting research snapshot authority semantics before PR #96 merges Rejected: reuse research artifact naming | it leaked snapshot-oriented identifiers and banned settlement surface semantics Directive: Keep content_sha256/content_bytes distinct from snapshot contracts and preserve the no-clobber triplet publisher as one rollout unit with the CLI Confidence: high Scope-risk: narrow Tested: cargo fmt --all; cargo test -p hft-collector manifest_semantics_disclose_content_digest_and_recording_limits --locked; cargo test -p hft-collector publish_polymarket_evidence_cli_requires_a_bounded_window_and_output_root --locked; cargo test -p hft-collector --locked; cargo clippy -p hft-collector --lib --locked -- -D warnings; git diff --check; git diff --cached --check
9281195 to
b3fb50c
Compare
| let descriptor = unsafe { | ||
| libc::openat( | ||
| directory.as_raw_fd(), | ||
| c".".as_ptr(), | ||
| libc::O_WRONLY | libc::O_TMPFILE | libc::O_CLOEXEC, | ||
| 0o600, | ||
| ) | ||
| }; |
| if descriptor < 0 { | ||
| return Err(std::io::Error::last_os_error()).context("create anonymous artifact file"); | ||
| } | ||
| Ok(unsafe { File::from_raw_fd(descriptor) }) |
| Ok(unsafe { | ||
| libc::linkat( | ||
| libc::AT_FDCWD, | ||
| source.as_ptr(), | ||
| directory.as_raw_fd(), | ||
| name.as_ptr(), | ||
| libc::AT_SYMLINK_FOLLOW, | ||
| ) | ||
| }) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b3fb50c398
ℹ️ 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".
| bail!("artifact payload disappeared before success publication"); | ||
| } | ||
| install_no_clobber(directory_path, &directory, success_path, bytes.success)?; | ||
| if !exact_or_missing(directory_path, &directory, success_path, bytes.success)? { |
There was a problem hiding this comment.
In a shared output directory, data and manifest are validated before _SUCCESS is published, but after install_no_clobber returns this only re-reads the marker. If another publisher or cleanup job unlinks/replaces the data or manifest between the data_ready/manifest_ready checks and this final check, publish_triplet can return Ok while leaving the completion marker for an incomplete or non-matching triplet; revalidate the payload files after the marker is installed before reporting success.
Useful? React with 👍 / 👎.
Change contract
Publish an already normalized
NormalizedPolymarketEvidencepayload as one immutable, content-addressed, no-clobber data/manifest/_SUCCESStriplet, exposed throughpolymarket-raw-ops publish-polymarket-evidence.Out of scope
snapshot_contract_hash, evaluator labels, or execution signals.Dependency / merge order
Depends on the neutral normalization contract from #96, which is already merged into
main. No outstanding stacked dependency remains; this PR is independently mergeable and rollbackable.Focused validation
cargo fmt --package hft-collector -- --checkcargo test -p hft-collector --locked(177 passed,2 ignored; all package binaries passed on macOS)cargo clippy -p hft-collector --locked --all-targets -- -D warningscargo check -p hft-collector --locked --target x86_64-unknown-linux-gnucargo clippy -p hft-collector --locked --all-targets --no-deps --target x86_64-unknown-linux-gnu -- -D warningsgit diff --check origin/main_SUCCESSrefusal, parent-directory swap refusal, exact0444publication, writable-mode drift, conflicting bytes, and payload reread before_SUCCESS.O_TMPFILEcounterexamples is required from current-head CI before merge.Rollout / rollback impact
Rollout is opt-in through the new CLI command and writes only beneath an explicitly supplied output root. It does not alter running collectors, databases, snapshots, or execution. Rollback is removal of the CLI/module; already published content-addressed artifacts remain immutable evidence and require no migration.
Scope evidence