fix: parallel-test race on process-global platform version in contract to_value - #4395
Conversation
…t to_value Every test platform build calls PlatformVersion::set_current (a process-wide global), and DataContract's serde Serialize impl reads that global to pick the serialization format. Tests that round-trip a contract through platform_value::to_value while another test concurrently builds a platform at an old protocol version (e.g. with_initial_protocol_version(8)) observe the old version mid-test: the contract serializes in the V0 format (config v0), which latest-version validation then rejects with DataContractConfigUpdateError instead of the expected error. This reproducibly failed test_data_contract_creation_fails_with_more_than_fifty_keywords in full `cargo test -p drive-abci --lib` runs while passing in isolation. Add DataContractValueConversionMethodsV0::to_value(&self, platform_version) which serializes through DataContractInSerializationFormat at an explicit platform version, and switch the data_contract_create / data_contract_update tests to it so their round-trip path no longer reads the global at all. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 17 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe PR adds explicit platform-version contract serialization through ChangesContract serialization
Estimated code review effort: 3 (Moderate) | ~20 minutes Mergeability Score: ⚪ Minimal · up to The change makes contract test round-trips use an explicit platform version instead of shared process-wide state, without changing production serialization behavior; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
✅ Final review complete — no blockers (commit 2e0c6dc) |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4395 +/- ##
============================================
- Coverage 86.81% 85.86% -0.95%
============================================
Files 2647 2658 +11
Lines 340850 347979 +7129
============================================
+ Hits 295913 298804 +2891
- Misses 44937 49175 +4238
🚀 New features to boost your workflow:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The explicit conversion path removes the affected test round-trips from process-global platform-version selection, but adding a required method to the existing public, unsealed conversion trait is a source-breaking API change and must be addressed or declared as such. The new stateless API also needs focused version-selection coverage, and the central DataContract guidance should no longer recommend the global-dependent path. Source: reviewer backend gpt-5.6-sol; final verifier backend gpt-5.6-sol. Orchestration-only: openclaw-agent/cliproxy/gpt-5.6-sol (not reviewer evidence).
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking | 🟡 2 suggestion(s)
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-dpp/src/data_contract/conversion/value/v0/mod.rs`:
- [BLOCKING] packages/rs-dpp/src/data_contract/conversion/value/v0/mod.rs:30-32: Adding a required trait method is a breaking API change
`DataContractValueConversionMethodsV0` is a publicly reachable, unsealed trait, and it was already shipped in the v4.1.0 history. Downstream crates may implement it for their own local types, so adding the required `to_value` method makes every such implementation fail to compile. This contradicts the PR's declaration that the API change is additive and has no breaking changes. Preserve source compatibility by putting `to_value` on a separate output extension trait implemented for `DataContract`, `DataContractV0`, and `DataContractV1`; otherwise mark and version this as a breaking change.
In `packages/rs-dpp/src/data_contract/conversion/value/mod.rs`:
- [SUGGESTION] packages/rs-dpp/src/data_contract/conversion/value/mod.rs:38-41: Add deterministic coverage for explicit version selection
The changed drive-abci tests exercise the new method through `DataContract`, generally using the latest platform version, while the new `DataContractV0` and `DataContractV1` implementations have no direct coverage. Add focused rs-dpp tests that invoke the explicit API with platform versions selecting serialization formats 0 and 1, assert the resulting top-level `$formatVersion` and expected shape, and cover all three concrete implementations. That directly pins the guarantee introduced by this fix instead of relying on parallel test scheduling to expose an incorrect global-version read.
In `packages/rs-dpp/src/data_contract/mod.rs`:
- [SUGGESTION] packages/rs-dpp/src/data_contract/mod.rs:127-130: Update the primary DataContract documentation to recommend the stateless API
This conversion guidance still directs callers to `platform_value::to_value(&dc)`, even though that path reads the mutable process-global platform version and is the source of the race fixed by this PR. The new explicit conversion API makes the comment stale. Recommend `DataContractValueConversionMethodsV0::to_value` whenever a platform version is available and explain that the serde-based alternatives retain global-dependent behavior.
Review follow-up: DataContractValueConversionMethodsV0 is public and unsealed, so a required to_value method would break downstream implementations. Make it a provided method instead, defaulted (with a where-clause) for any implementor convertible to DataContractInSerializationFormat — existing DataContract/V0/V1 callers are unchanged and foreign implementors keep compiling without providing it. Also add rs-dpp tests pinning that to_value selects the serialization format ($formatVersion 0 vs 1) from the passed platform version for DataContract, DataContractV0, and DataContractV1, and round-trips through from_value at the same version. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The explicit conversion API correctly selects serialization from the supplied PlatformVersion, and the provided trait method plus focused tests resolve the previous compatibility and coverage findings. The central DataContract conversion guidance still recommends the process-global serialization path, leaving one documentation suggestion. Source: reviewer backend model gpt-5.6-sol; final verifier backend model gpt-5.6-sol; orchestration-only: openclaw-agent/cliproxy/gpt-5.6-sol (not reviewer evidence).
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
🟡 1 suggestion(s)
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-dpp/src/data_contract/mod.rs`:
- [SUGGESTION] packages/rs-dpp/src/data_contract/mod.rs:127-128: Update the primary DataContract documentation to recommend the stateless API
The primary DataContract conversion guidance still directs callers to `platform_value::to_value(&dc)`, even though that path selects its format through the mutable process-global platform version and caused the race fixed by this PR. The new explicit API is documented only in lower-level conversion modules, so callers following this central guidance can continue introducing nondeterministic conversions. Recommend `DataContractValueConversionMethodsV0::to_value` whenever a `PlatformVersion` is available and identify the serde-based alternatives as global-dependent.
…dance Review follow-up: the DataContract conversion note still pointed callers at the serde serialization path, which selects its format from the process-global current platform version. Recommend DataContractValueConversionMethodsV0::to_value(&dc, pv) whenever a PlatformVersion is in hand and flag the serde alternatives as global-dependent. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The explicit conversion path uses the supplied PlatformVersion, preserves source compatibility through a default trait method, and has focused coverage for format selection and round trips. The central DataContract documentation now recommends the explicit-version API and clearly identifies the process-global behavior of serde alternatives; no in-scope defects remain.
Source: reviewer backend gpt-5.6-sol; final verifier backend gpt-5.6-sol. Orchestration-only: openclaw-agent/cliproxy/gpt-5.6-sol (not reviewer evidence).
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
|
Reviewed |
2 participants
Issue being fixed or feature implemented
cargo test -p drive-abci --lib(full suite) reproducibly faileddata_contract_create::tests::keywords::test_data_contract_creation_fails_with_more_than_fifty_keywords, while the same test passed in isolation and at module scope. The assertion expectedTooManyKeywordsErrorbut gotDataContractConfigUpdateError("config version 0 is not supported, minimum version is 1").This is a parallel-test race on the process-wide current platform version:
Platform::open_with_client_no_saved_state/open_with_client_saved_state) callsPlatformVersion::set_current, a process-global.DataContract's serdeSerializeimpl picks the serialization format from that global (get_version_or_current_or_latest(None)).platform_value::to_value(&contract)while another test concurrently builds a platform at an old protocol version (e.g.with_initial_protocol_version(8)) serializes the contract in the old V0 format with a v0 config. Deserializing and validating at the latest version then rejects the v0 config before the code under test is ever reached.What was done?
Threaded an explicit platform version through the round-trip path instead of serializing via the global:
DataContractValueConversionMethodsV0::to_value(&self, platform_version)(implemented forDataContract,DataContractV0,DataContractV1), which serializes through the existingDataContractInSerializationFormatconversion at the explicit version — same wire shape as the serde path, no global read. The serde impl itself is intentionally left untouched (its behavior is pinned by thedata_contract_serde_pins_critical_4tests).to_valuecontract round-trip sites in the drive-abci tests to the new method: 7 indata_contract_create(keywords + descriptions) and 4 indata_contract_update, which had the identical latent race.Note: the
set_currentglobal itself remains (it is load-bearing for production paths); this removes the dependence on it from these test code paths.How Has This Been Tested?
cargo test -p drive-abci --libruns: 2671 passed / 0 failed each (previously reproducibly 1 failure per run).dpp,drive,wasm-dpp,wasm-dpp2compile against the trait change; clippy ondppshows only pre-existing warnings.Breaking Changes
None (test-only behavior plus an additive API; no consensus or serialization changes at any given platform version).
Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes