feat(collector): publish per-modality PIT clocks in USD-M reference manifest v2 - #803
Conversation
|
Warning Review limit reached
Next review available in: 33 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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 (4)
📝 WalkthroughWalkthroughThe Binance reference artifact changes to manifest schema v2 with separate mark/index/funding and open-interest pit clocks. Publication and verification validate modality-specific evidence, while historical v1 manifests remain readable. Shadow-gate policies and fixtures now enforce the new contract. ChangesBinance reference manifest contract
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant BinanceReferenceArtifact
participant ArtifactVerifier
participant ShadowGatePolicy
participant ShadowGateFixtures
BinanceReferenceArtifact->>ArtifactVerifier: publish v2 manifest with modality clocks
ArtifactVerifier->>ArtifactVerifier: validate v1 or v2 schema
ArtifactVerifier->>ShadowGatePolicy: provide artifact evidence
ShadowGatePolicy->>ShadowGateFixtures: validate modality clocks and counts
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c7085df14d
ℹ️ 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".
…anifest v2 The reference artifact manifest merged funding/mark-index and open-interest row clocks into one cross-modality time_bounds, which cannot feed the V2 CexPitSeriesEvidenceV2 contract that requires independent first/last-available, observations, and max_gap_ns per modality. Manifest schema binance.usdm_reference_manifest.v2 now publishes a separate ModalityPitClock per modality (received_at_ns availability bounds, max_gap_ns, and source_time_ms event range), drops the merged time_bounds, and fails closed when a modality is missing or its availability gap exceeds the 90s CEX_DERIVATIVES_MAX_GAP_NS bound. V1 manifests remain decodable read-only for already-published evidence; the publisher never writes them. The shadow gate runner, policy, and fixtures assert the per-modality clocks.
…ests The verifier bin historical fixture sealed historical v2 data with a fresh V2 manifest, a combination that never existed: historical v2 triplets were published with V1 manifests carrying one merged cross-modality time_bounds. Downgrade the resealed manifest to that exact historical shape so the read-only decode path is exercised against real V1 identity while V2 manifests stay strict.
3d7750e to
ddd39ea
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
deployment/aliyun/binance-usdm-reference-shadow-gate-policy.jq (1)
31-39: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueConsider binding
max_gap_nsto the availability window.
pit_clockaccepts anymax_gap_nsin[0, 90000000000], even when it exceeds the whole availability span. Adding.max_gap_ns <= (.last_available_at_ns - .first_available_at_ns)makes the clock internally consistent and rejects fabricated gap values.🤖 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 `@deployment/aliyun/binance-usdm-reference-shadow-gate-policy.jq` around lines 31 - 39, Update the pit_clock predicate to require max_gap_ns not exceed the availability span, by adding a comparison with last_available_at_ns minus first_available_at_ns while preserving the existing numeric, integer, nonnegative, and upper-bound validations.rust_hft/tools/collector/src/binance_usdm_reference_artifact.rs (3)
578-596: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueThe V2 read-only path reads and hashes the artifact twice.
verify_reference_artifact_read_onlycallsread_artifact_trust_anchor, then callsverify_reference_artifact, which repeats the same file reads and SHA-256 computations.MAX_DATA_BYTESbounds the cost, but the duplicate work is avoidable by callingverify_current_manifest_v2directly with the already-readdataandmanifest_bytes.♻️ Proposed change
let schema = peek_manifest_schema(&manifest_bytes)?; if schema == MANIFEST_SCHEMA_V2 { - let batch = - verify_reference_artifact(published, expected_data_sha256, expected_manifest_sha256)?; + let batch = verify_current_manifest_v2( + published, + &data, + &manifest_bytes, + expected_data_sha256, + )?; return Ok(VerifiedReferenceCounts {🤖 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/binance_usdm_reference_artifact.rs` around lines 578 - 596, Update verify_reference_artifact_read_only’s MANIFEST_SCHEMA_V2 branch to call verify_current_manifest_v2 directly with the already-read data and manifest_bytes from read_artifact_trust_anchor, instead of invoking verify_reference_artifact. Preserve the existing VerifiedReferenceCounts construction and validation behavior while avoiding the duplicate artifact reads and hashing.
1546-1599: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the test to match its assertion.
historical_v1_manifest_remains_readable_read_onlyassertshistorical_read_only: false. The V1 manifest in this fixture carriesdata_schema == REFERENCE_SCHEMA, so the verifier takes the full-verification branch, not the historical read-only branch. The name states the opposite of the asserted outcome. A name such ashistorical_v1_manifest_over_current_rows_verifies_fullydescribes the covered path.🤖 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/binance_usdm_reference_artifact.rs` around lines 1546 - 1599, Rename the test function historical_v1_manifest_remains_readable_read_only to reflect that the V1 manifest with REFERENCE_SCHEMA verifies through the full-verification path and produces historical_read_only: false; use a name such as historical_v1_manifest_over_current_rows_verifies_fully.
505-571: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the shared verification body.
verify_current_manifest_v2andverify_current_manifest_v1differ only in the manifest type, the expected schema constant, and the clock comparison. The identity check, partition check, batch parse, coverage recompute, and row-count check are identical. A shared helper that returns the parsed batch and recomputed coverage would remove the duplication and keep the two paths in sync.🤖 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/binance_usdm_reference_artifact.rs` around lines 505 - 571, Extract the duplicated identity validation, partition validation, batch parsing, coverage recomputation, and row-count checks from verify_current_manifest_v2 and verify_current_manifest_v1 into a shared helper that returns the parsed batch and recomputed coverage. Keep each version-specific manifest parsing, schema constant, and clock/time-bounds comparison in its respective verifier, and reuse the helper in both paths.
🤖 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 `@agent-worktree.yml`:
- Around line 7-11: Update the allowed_files list in agent-worktree.yml to
include every path changed by this PR: the verifier binary, shadow-gate policy,
shadow-gate script, and both host-gate and shadow-gate test scripts. Replace the
unrelated entries so the worktree permission contract exactly matches the PR’s
changed files.
---
Nitpick comments:
In `@deployment/aliyun/binance-usdm-reference-shadow-gate-policy.jq`:
- Around line 31-39: Update the pit_clock predicate to require max_gap_ns not
exceed the availability span, by adding a comparison with last_available_at_ns
minus first_available_at_ns while preserving the existing numeric, integer,
nonnegative, and upper-bound validations.
In `@rust_hft/tools/collector/src/binance_usdm_reference_artifact.rs`:
- Around line 578-596: Update verify_reference_artifact_read_only’s
MANIFEST_SCHEMA_V2 branch to call verify_current_manifest_v2 directly with the
already-read data and manifest_bytes from read_artifact_trust_anchor, instead of
invoking verify_reference_artifact. Preserve the existing
VerifiedReferenceCounts construction and validation behavior while avoiding the
duplicate artifact reads and hashing.
- Around line 1546-1599: Rename the test function
historical_v1_manifest_remains_readable_read_only to reflect that the V1
manifest with REFERENCE_SCHEMA verifies through the full-verification path and
produces historical_read_only: false; use a name such as
historical_v1_manifest_over_current_rows_verifies_fully.
- Around line 505-571: Extract the duplicated identity validation, partition
validation, batch parsing, coverage recomputation, and row-count checks from
verify_current_manifest_v2 and verify_current_manifest_v1 into a shared helper
that returns the parsed batch and recomputed coverage. Keep each
version-specific manifest parsing, schema constant, and clock/time-bounds
comparison in its respective verifier, and reuse the helper in both paths.
🪄 Autofix
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: fecdfc9f-c0ae-4829-b4a2-eb1894b37d75
📒 Files selected for processing (7)
agent-worktree.ymldeployment/aliyun/binance-usdm-reference-shadow-gate-policy.jqdeployment/aliyun/binance-usdm-reference-shadow-gate.shdeployment/aliyun/test-binance-usdm-reference-host-gate.shdeployment/aliyun/test-binance-usdm-reference-shadow-gate.shrust_hft/tools/collector/src/bin/binance-usdm-reference-artifact-verifier.rsrust_hft/tools/collector/src/binance_usdm_reference_artifact.rs
… clock assertions Review follow-ups for the per-modality PIT clock manifest: - verify_reference_artifact (the writable path used by the collector self-check and the OSS uploader) now rejects historical v1 manifests; their decoder is reserved for verify_reference_artifact_read_only, so superseded evidence cannot re-enter the publish/upload pipeline. - The shadow gate policy requires every modality clock bound to be an unsigned integer before ordering it; jq's null <= number comparison no longer admits clocks missing first/last event or availability bounds. - agent-worktree.yml allowed_files now records the exact changed-file scope of this contract.
Change contract
Reference artifact 发布层把 funding/mark-index 与 open-interest 的行级时钟合并成单一跨 modality
time_bounds,违反 V2 契约(CexPitSeriesEvidenceV2要求每 modality 独立first_available_at/last_available_at/observations/max_gap_ns,且max_gap_ns ≤ 90s)。本 PR 把 manifest 升级为binance.usdm_reference_manifest.v2:每 modality 独立发布ModalityPitClock(available 语义 = 行级received_at_ns的 first/last 与max_gap_ns;event-time 范围 =source_time_ms的 first/last),不再发布合并 time_bounds;modality 缺失或max_gap_ns超过CEX_DERIVATIVES_MAX_GAP_NS(90s)时 fail-closed 不发布,readback 重算时钟并逐字段比对。这是 V2 补齐批次的 PR-1,为 PR #786 之后的 V3 materialization 提供独立 funding/OI 时钟数据源。
Issue relationship
Refs #794
Out of scope
lob-pit-materializer与 research-corepit_series_covers当前不读 reference manifest 的合并 time_bounds(已全仓取证),无需编译对齐;PR feat(research): publish verified CEX ResearchSnapshot V2 #786(head 9806c34)正在重写 materializer 证据装配,本 PR 不触碰lob-pit-materializer.rs/lib.rs/Cargo.toml,V3 materialization 对本数据源的装配留给 PR feat(research): publish verified CEX ResearchSnapshot V2 #786 后续。binance_usdm_reference.rs的MarkIndexFundingObservation/OpenInterestObservation)本就已独立,未改动。Dependencies and merge order
None(base:origin/main 161b761)。与 PR #786 无文件交叠,可独立合并。
Focused validation
cargo test -p hft-collector --locked --lib binance_usdm_reference:24/24 通过。新增覆盖:交错到达时两 modality 时钟互不污染(available 与 event-time 范围各自独立)、mark modality availability gap 100s > 90s fail-closed 且不落盘、modality 缺失(空迭代 + manifest 删键)fail-closed、manifest 字段形状(schema=v2、无 time_bounds、两组各 6 字段)、V1 manifest 只读解码回归。cargo clippy -p hft-collector --locked --target x86_64-unknown-linux-gnu --all-targets:零告警。test-binance-usdm-reference-shadow-gate.sh、test-binance-usdm-reference-host-gate.sh、test-binance-usdm-reference-release-contract.sh全部通过;shadow-gate policy 新增 reject 用例(legacy manifest schema、缺失任一模态时钟、OI gap 超 90s、时钟 observations 与 coverage 不符、last_available 晚于 observed_at_ns)。CEX_DERIVATIVES_MAX_GAP_NS;发布侧 gap 超界时 staging 目录不产生任何批次(测试断言 hour 目录为空)。libc::RUSAGE_THREAD(Linux-only,fix(collector): reuse sealed market tape validation #772)无法编译,本地用临时 shim(RUSAGE_SELF)跑测试,shim 未提交;linux target clippy 为零告警基线。binance_fee_upload4 个测试在 macOS 宿主因 tempdir/var→/private/var符号链接触发 canonicalization 拒绝而失败,为预存在的宿主平台问题,本 PR 未触碰该代码路径。agent-worktree-preflight.sh在本机对任意 managed worktree 均因 SIGPIPE(git worktree list | awk 提前退出,pipefail)exit 141,预存在环境问题;ownership 记录的语义校验已手工逐条核对通过。Rollout and rollback
部署后 collector 只写 v2 manifest;uploader/verifier 对 spool 中存量 v1 manifest 批次保持只读验证可上传。shadow gate policy 只接受 v2,绑定 candidate 产物,不影响在跑生产。回滚 = 还原本 PR,collector 重新发布 v1 manifest。
Scope exception
None(6 个文件,+588/-120 行)。
Summary by CodeRabbit
New Features
Bug Fixes
Tests