feat(collector): gate USD-M reference shadow - #259
Conversation
📝 WalkthroughWalkthroughThe PR adds Binance USD-M reference control-asset packaging, a templated shadow collector service, a jq-based evidence gate policy, and Bash tests covering valid, invalid, and deployment-configuration cases. ChangesBinance USD-M reference shadow
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1d998e979b
ℹ️ 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".
| and ([.artifacts[].observed_at_ns] as $times | ||
| | $times == ($times | sort) | ||
| and ($times | unique | length) == ($times | length) | ||
| and ([range(1; $times | length) as $index | ||
| | $times[$index] - $times[$index - 1]] as $gaps | ||
| | all($gaps[]; . > 0 and . <= 90000000000) | ||
| and .max_artifact_gap_ns == ($gaps | max))) |
There was a problem hiding this comment.
The policy validates duration_seconds >= 3600 independently from the artifact timestamps, so evidence claiming a one-hour run can pass with only three readbacks collected within seconds. The checked-in positive fixture demonstrates this by claiming 3,600 seconds while its artifacts span only 60 seconds, meaning the production-eligibility gate does not establish continuous canonical output for the required hour. Require the first-to-last artifact span to cover the claimed/minimum duration and add a rejection test for this counterexample.
AGENTS.md reference: AGENTS.md:L93-L96
Useful? React with 👍 / 👎.
| (.active_contracts | type) == "number" | ||
| and .active_contracts == (.active_contracts | floor) | ||
| and .active_contracts > 0 | ||
| and .metadata_observations == .active_contracts | ||
| and .mark_index_funding_observations == .active_contracts |
There was a problem hiding this comment.
Requiring only one active contract lets a partial but syntactically valid exchangeInfo response qualify for production: the collector treats the returned contracts as the expected set, filters premiumIndex to that set, and requests open interest only for those symbols, so even a one-symbol response can produce three artifacts with “complete” coverage and pass this gate. The existing USD-M shadow policy requires at least 400 symbols (rust-lob-shadow-gate-policy.jq lines 70-73); enforce a comparable production floor or a trusted catalog baseline here and add a low-universe rejection test so an incomplete reference dataset cannot be promoted.
AGENTS.md reference: AGENTS.md:L93-L96
Useful? React with 👍 / 👎.
| and .service.unit == ("binance-usdm-reference-collector-shadow@" + $candidate_sha256 + ".service") | ||
| and .service.active == true | ||
| and .service.restart_count == 0 | ||
| and .service.binary_sha256 == $candidate_sha256 |
There was a problem hiding this comment.
The service evidence records only the unit name, current active state, restart counter, and binary digest, so a process replacement during the observation window can still pass using the persistent spool: after a quick replacement, the new candidate process is active with the same digest, while artifacts from before and after it can remain ordered with gaps below 90 seconds. The established collector control plane explicitly treats PID and NRestarts as insufficient and pins InvocationID as well (deployment/aliyun/README.md lines 325-330); capture the initial PID/invocation and require the same identity at completion, with a replacement counterexample test.
AGENTS.md reference: AGENTS.md:L93-L96
Useful? React with 👍 / 👎.
| and .observed_at_ns == (.observed_at_ns | floor) | ||
| and .observed_at_ns > 0 | ||
| and (.coverage | complete_coverage) | ||
| and .time_bounds.min_source_time_ms <= .time_bounds.max_source_time_ms |
There was a problem hiding this comment.
The artifact manifest carries max_staleness_ms, but this policy omits it and therefore interprets stale_* == 0 using whatever threshold produced the artifact. The candidate CLI permits values up to 300,000 ms, and the runtime evidence does not validate its arguments, so a modified unit or drop-in can run the expected binary with a five-minute threshold and have five-minute-old rows pass as non-stale even though the shipped service pins 30,000 ms. Include the read-back threshold in each artifact and require exactly 30,000 ms.
AGENTS.md reference: AGENTS.md:L50-L54
Useful? React with 👍 / 👎.
|
Addressed all four Codex review P1s in
Added a focused counterexample for each condition. Both independent read-only reviewers report no blocker; the local gate test and shellcheck pass. |
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 `@deployment/aliyun/binance-usdm-reference-shadow-gate-policy.jq`:
- Around line 27-46: Update canonical_artifact to require type == "number" for
all four time_bounds fields—min_source_time_ms, max_source_time_ms,
min_received_at_ns, and max_received_at_ns—before the existing ordering
comparisons. Preserve the current bounds checks and fail-closed behavior for
missing or null time_bounds values.
🪄 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: 3ba396cb-8579-4e5f-8e79-39715656e676
📒 Files selected for processing (4)
.github/workflows/acr-publish.ymldeployment/aliyun/binance-usdm-reference-collector-shadow@.servicedeployment/aliyun/binance-usdm-reference-shadow-gate-policy.jqdeployment/aliyun/test-binance-usdm-reference-shadow-gate.sh
| def canonical_artifact: | ||
| .canonical_readback == true | ||
| and .venue == "binance_usdm" | ||
| and .dataset == "reference" | ||
| and .manifest_schema == "binance.usdm_reference_manifest.v1" | ||
| and .data_schema == "binance.usdm_reference.v2" | ||
| and .source_origin == "https://fapi.binance.com" | ||
| and (.source_endpoints | endpoints) | ||
| and .max_staleness_ms == 30000 | ||
| and (.data_sha256 | sha256) | ||
| and (.manifest_sha256 | sha256) | ||
| and .success_sha256 == .data_sha256 | ||
| and .content_rows_verified == true | ||
| and (.observed_at_ns | type) == "number" | ||
| and .observed_at_ns == (.observed_at_ns | floor) | ||
| and .observed_at_ns > 0 | ||
| and (.coverage | complete_coverage) | ||
| and .time_bounds.min_source_time_ms <= .time_bounds.max_source_time_ms | ||
| and .time_bounds.min_received_at_ns <= .time_bounds.max_received_at_ns | ||
| and .time_bounds.max_received_at_ns <= .observed_at_ns; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Missing/null time_bounds silently passes the ordering checks.
None of the four time_bounds fields (min_source_time_ms, max_source_time_ms, min_received_at_ns, max_received_at_ns) have a type == "number" guard before the <= comparisons at Line 44-46, unlike observed_at_ns, active_contracts, duration_seconds, and artifact_count, which all get explicit numeric-type checks elsewhere in this file.
In jq, comparison operators use a total ordering where null sorts below every other type, and <=/>= never raise a type error (unlike arithmetic -). So if an artifact's time_bounds object (or any of its subfields) is missing, each becomes null, and null <= null and null <= <number> both evaluate to true. An artifact with time_bounds deleted entirely (e.g. del(.artifacts[1].time_bounds)) would therefore still satisfy all three ordering checks and pass canonical_artifact, defeating the fail-closed intent of this gate. No existing reject counterexample in the test script covers this case.
🛡️ Proposed fix: add explicit numeric-type guards
and (.coverage | complete_coverage)
- and .time_bounds.min_source_time_ms <= .time_bounds.max_source_time_ms
- and .time_bounds.min_received_at_ns <= .time_bounds.max_received_at_ns
- and .time_bounds.max_received_at_ns <= .observed_at_ns;
+ and (.time_bounds.min_source_time_ms | type) == "number"
+ and (.time_bounds.max_source_time_ms | type) == "number"
+ and (.time_bounds.min_received_at_ns | type) == "number"
+ and (.time_bounds.max_received_at_ns | type) == "number"
+ and .time_bounds.min_source_time_ms <= .time_bounds.max_source_time_ms
+ and .time_bounds.min_received_at_ns <= .time_bounds.max_received_at_ns
+ and .time_bounds.max_received_at_ns <= .observed_at_ns;Would you like me to also draft a reject counterexample (e.g. del(.artifacts[1].time_bounds)) for the test script to cover this case going forward?
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def canonical_artifact: | |
| .canonical_readback == true | |
| and .venue == "binance_usdm" | |
| and .dataset == "reference" | |
| and .manifest_schema == "binance.usdm_reference_manifest.v1" | |
| and .data_schema == "binance.usdm_reference.v2" | |
| and .source_origin == "https://fapi.binance.com" | |
| and (.source_endpoints | endpoints) | |
| and .max_staleness_ms == 30000 | |
| and (.data_sha256 | sha256) | |
| and (.manifest_sha256 | sha256) | |
| and .success_sha256 == .data_sha256 | |
| and .content_rows_verified == true | |
| and (.observed_at_ns | type) == "number" | |
| and .observed_at_ns == (.observed_at_ns | floor) | |
| and .observed_at_ns > 0 | |
| and (.coverage | complete_coverage) | |
| and .time_bounds.min_source_time_ms <= .time_bounds.max_source_time_ms | |
| and .time_bounds.min_received_at_ns <= .time_bounds.max_received_at_ns | |
| and .time_bounds.max_received_at_ns <= .observed_at_ns; | |
| def canonical_artifact: | |
| .canonical_readback == true | |
| and .venue == "binance_usdm" | |
| and .dataset == "reference" | |
| and .manifest_schema == "binance.usdm_reference_manifest.v1" | |
| and .data_schema == "binance.usdm_reference.v2" | |
| and .source_origin == "https://fapi.binance.com" | |
| and (.source_endpoints | endpoints) | |
| and .max_staleness_ms == 30000 | |
| and (.data_sha256 | sha256) | |
| and (.manifest_sha256 | sha256) | |
| and .success_sha256 == .data_sha256 | |
| and .content_rows_verified == true | |
| and (.observed_at_ns | type) == "number" | |
| and .observed_at_ns == (.observed_at_ns | floor) | |
| and .observed_at_ns > 0 | |
| and (.coverage | complete_coverage) | |
| and (.time_bounds.min_source_time_ms | type) == "number" | |
| and (.time_bounds.max_source_time_ms | type) == "number" | |
| and (.time_bounds.min_received_at_ns | type) == "number" | |
| and (.time_bounds.max_received_at_ns | type) == "number" | |
| and .time_bounds.min_source_time_ms <= .time_bounds.max_source_time_ms | |
| and .time_bounds.min_received_at_ns <= .time_bounds.max_received_at_ns | |
| and .time_bounds.max_received_at_ns <= .observed_at_ns; |
🤖 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
27 - 46, Update canonical_artifact to require type == "number" for all four
time_bounds fields—min_source_time_ms, max_source_time_ms, min_received_at_ns,
and max_received_at_ns—before the existing ordering comparisons. Preserve the
current bounds checks and fail-closed behavior for missing or null time_bounds
values.
Part of #222
Change contract: publish an isolated USD-M reference shadow service and a fail-closed canonical-readback gate bound to an exact candidate, source revision, and dedicated control bundle.
Out of scope: ECS installation or execution, production collector replacement/cutover, collector source/artifact/runtime logic, issues #223/#224, snapshot construction, evaluator/MCTS, and research logic.
Dependency or merge order: depends on merged #257 release packaging. This is issue #222 layer 3B; host evidence generation follows in a separate PR.
Focused validation:
bash -n deployment/aliyun/test-binance-usdm-reference-shadow-gate.shshellcheck deployment/aliyun/test-binance-usdm-reference-shadow-gate.shbash deployment/aliyun/test-binance-usdm-reference-shadow-gate.shbash deployment/aliyun/test-binance-usdm-reference-release-contract.shgit diff --checkRollout/rollback impact: no host mutation. The release artifact gains a dedicated reference control bundle. Rollback is reverting this PR; the existing Polymarket control bundle identity is unchanged.
Summary by CodeRabbit
New Features
Tests