fix(dpp): stop hard-erroring on index-order-only contract updates - #4295
Conversation
The JSON-schema compatibility validator has no keyword rule for `indices`, so any contract-update schema diff under /indices that survived the index checks returned Err(UnsupportedSchemaKeywordError -> SchemaCompatibilityValidationError) instead of a clean consensus validation result — surfaced by drive-abci as StateTransitionExecutionResult::InternalError for user-triggerable input. At protocol v14 the reachable case is reordering the indices array without changing the definition set: validate_update v1's name-keyed comparison passes (a reorder is a semantic no-op — indices are keyed by name), but the JSON diff under /indices still hard-errored. Add validate_schema_compatibility v1, which strips the top-level `indices` key from both schemas before diffing: index definitions are validated in exactly one place (validate_update v1's name-keyed comparison, which rejects any real index change with a clean DataContractInvalidIndexDefinitionUpdateError before schema compatibility runs). A property named "indices" lives under /properties/indices and is still validated. Gated at protocol v14 via CONTRACT_VERSIONS_V6 (used only by v14); v0 stays byte-identical and protocol v13 behavior is pinned by tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 55 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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change adds a v1 schema compatibility validator that ignores top-level index ordering, routes protocol v6 to it, documents the protocol v14 behavior, and adds regression tests for schema and update validation. ChangesSchema compatibility and index reordering
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SchemaValidation
participant V1Validator
participant JsonSchemaCompatibility
SchemaValidation->>V1Validator: dispatch compatibility version 1
V1Validator->>V1Validator: remove top-level indices
V1Validator->>JsonSchemaCompatibility: compare filtered schemas
JsonSchemaCompatibility-->>V1Validator: changes or validation error
V1Validator-->>SchemaValidation: SimpleValidationResult or ProtocolError
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
📝 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 |
|
🔍 Review in progress — actively reviewing now (commit 3a3f4dc) |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4295 +/- ##
============================================
- Coverage 87.60% 87.60% -0.01%
============================================
Files 2703 2704 +1
Lines 344990 345181 +191
============================================
+ Hits 302242 302379 +137
- Misses 42748 42802 +54
🚀 New features to boost your workflow:
|
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
This PR adds a well-scoped, protocol-v14-gated validate_schema_compatibility v1 that strips the top-level indices key before diffing document-type schemas, closing a real gap where an index-order-only contract update hard-errored into an internal error. I verified the version wiring end-to-end (v13 = validate_update v0 + schema-compat v0, v14 = validate_update v1 + schema-compat v1), confirmed the $defs call site is unaffected (no top-level indices key there) and that a genuine /properties/indices property is still validated, and ran the new tests locally — all pass. No blocking issues; remaining findings are minor documentation/duplication/test-robustness nitpicks.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— rust-quality (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
claude-sonnet-5— final-verifier - Sonnet reviewers:
claude-sonnet-5— general (failed),claude-sonnet-5— security-auditor (failed),claude-sonnet-5— rust-quality (completed),claude-sonnet-5— ffi-engineer (completed),claude-sonnet-5— general (completed),claude-sonnet-5— security-auditor (completed)
🟡 1 suggestion(s) | 💬 3 nitpick(s)
🤖 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/document_type/schema/validate_schema_compatibility/v1/mod.rs`:
- [SUGGESTION] packages/rs-dpp/src/data_contract/document_type/schema/validate_schema_compatibility/v1/mod.rs:1-87: v1's correctness relies on an unenforced cross-version pairing invariant
`validate_schema_compatibility_v1`'s safety (unconditionally stripping `indices` is fine) depends on `DocumentTypeRef::validate_update` — gated by the *separately versioned* `platform_version.dpp.validation.document_type.validate_update` field — always rejecting any real index change before this check runs, for whichever `validate_update` generation is paired with schema-compat v1 in a given `PlatformVersion`. I confirmed this holds today: v13 pairs `validate_update` v0 with schema-compat v0, and v14 pairs `validate_update` v1 with schema-compat v1 (`DPP_VALIDATION_VERSIONS_V5` + `CONTRACT_VERSIONS_V6`). But nothing in the code ties these two independent version numbers together — a future protocol version that bumps schema-compat without also bumping (or with a looser) `validate_update` would silently let real index changes bypass compatibility checking, since `indices` is unconditionally stripped. This isn't a defect in this PR (the current wiring is correct and tested), but a short code comment on `validate_schema_compatibility_v1` calling out the dependency on `validate_update` v1's guarantee would help future maintainers avoid breaking the pairing.
… note The v14 header claimed the shared-prefix fix was the only pre-ranked behavior change; the index-reorder schema-compatibility fix is a second, orthogonal one, so name both. Also document on validate_schema_compatibility_v1 that unconditionally stripping `indices` is safe only while every version table pairing schema-compat v1 also selects validate_update >= 1, which enforces index immutability before this check runs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…alds-8d4c55 # Conflicts: # packages/rs-platform-version/src/version/v14.rs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/rs-dpp/src/data_contract/document_type/schema/validate_schema_compatibility/v1/mod.rs (1)
106-107: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin the v1 tests to protocol version 14.
PlatformVersion::latest()dispatches through the current version table. If a future protocol selects another schema-compatibility generation, these tests can stop exercisingvalidate_schema_compatibility_v1. UsePlatformVersion::get(14)in these tests to keep version-1 coverage stable.Proposed test pin
- let platform_version = PlatformVersion::latest(); + let platform_version = + PlatformVersion::get(14).expect("protocol version 14 must exist");Also applies to: 149-150, 187-188
🤖 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 `@packages/rs-dpp/src/data_contract/document_type/schema/validate_schema_compatibility/v1/mod.rs` around lines 106 - 107, Update the v1 compatibility tests, including should_ignore_indices_reordering and the analogous cases around the other referenced locations, to obtain the platform version with PlatformVersion::get(14) instead of PlatformVersion::latest(). Keep the tests otherwise unchanged so they consistently exercise validate_schema_compatibility_v1.
🤖 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
`@packages/rs-dpp/src/data_contract/document_type/schema/validate_schema_compatibility/v1/mod.rs`:
- Around line 106-107: Update the v1 compatibility tests, including
should_ignore_indices_reordering and the analogous cases around the other
referenced locations, to obtain the platform version with
PlatformVersion::get(14) instead of PlatformVersion::latest(). Keep the tests
otherwise unchanged so they consistently exercise
validate_schema_compatibility_v1.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 26ce473b-9695-4b85-93a7-23853daedea1
📒 Files selected for processing (5)
packages/rs-dpp/src/data_contract/document_type/methods/validate_update/v1/mod.rspackages/rs-dpp/src/data_contract/document_type/schema/validate_schema_compatibility/mod.rspackages/rs-dpp/src/data_contract/document_type/schema/validate_schema_compatibility/v1/mod.rspackages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v6.rspackages/rs-platform-version/src/version/v14.rs
|
Re @coderabbitai's suggestion to pin the 🤖 Addressed by Claude Code |
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
You are interacting with an AI system. |
Issue being fixed or feature implemented
The JSON-schema compatibility validator has no keyword rule for
indices, so any contract-update schema diff under/indicesthat survived the dedicated index checks returnedErr(UnsupportedSchemaKeywordError → SchemaCompatibilityValidationError)instead of a clean consensus validation result. drive-abci surfaces that asStateTransitionExecutionResult::InternalError(code 1) for user-triggerable input.At protocol v14 (after #4291's name-keyed index comparison) the reachable case is reordering the
indicesarray without changing the definition set: the name-keyed comparison passes — a reorder is a semantic no-op, since indices are keyed by name — but the JSON diff under/indicesstill hard-errored.What was done?
validate_schema_compatibilityv1 (packages/rs-dpp/src/data_contract/document_type/schema/validate_schema_compatibility/v1/mod.rs): a new generation that strips the top-levelindiceskey from both schemas before diffing. Index definitions are validated in exactly one place —validate_updatev1's name-keyed comparison, which rejects any real index change with a cleanDataContractInvalidIndexDefinitionUpdateErrorbefore schema compatibility runs. A property namedindices(under/properties/indices) is still validated.validate_schema_compatibility/mod.rs) and bumpedvalidate_schema_compatibility: 1inCONTRACT_VERSIONS_V6, which only protocol v14 uses — the shipped v0 generation stays byte-identical and protocol ≤13 behavior is unchanged.CONTRACT_VERSIONS_V6andPLATFORM_V14changelog comments.Stripping was chosen over an "any
/indiceschange is incompatible" rule because the only diff that can reach this check at v14 is a reorder-only update, which is harmless and should be accepted rather than cleanly rejected.How Has This Been Tested?
validate_update/v1/mod.rs:should_pass_when_indices_are_reordered_without_changes— reorder-only contract update through the publicvalidate_updatedispatcher at the latest protocol version now returns a valid result instead of anErr.validate_schema_compatibility/v1/mod.rs: reorder-only diff is ignored; an incompatible property change alongside an/indicesdiff is still reported; a property literally namedindicesis still validated; replay-safety pin that protocol v13 (v0) still hard-errors on an/indicesdiff with the exact unsupported-keyword message.cargo test -p dpp(full suite),cargo test -p platform-version,cargo check -p drive-abci,cargo clippy -p dpp -p platform-version --all-targets,cargo check -p dpp --all-features --testsand--no-default-features --tests— all green.Breaking Changes
None. Gated at protocol v14 (unreleased); v0 stays byte-identical and pre-v14 outcomes are pinned by tests. Note this makes a reorder-only update the first index-touching contract update ever accepted end-to-end — previously every such update was rejected one way or another, so no historical accepted-set changes.
Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
indiceswithin document schemas remain fully validated.Documentation