fix(dpp): make document type index update validation name-order independent - #4291
Conversation
…endent IndexLevel::validate_update compared index trees by level_identifier equality, but identifiers are assigned by an incrementing counter that walks the document type's indices — a BTreeMap keyed by index NAME. An index change whose name sorted before existing indexes renumbered every level and was rejected with an opaque "Invalid path", while the semantically identical change under a late-sorting name passed the tree comparison and instead hard-errored later in the JSON-schema compatibility check (which has no rule for the `indices` keyword), surfacing as an InternalError tx result. Which consensus outcome a contract owner got therefore depended purely on the index name string. At protocol v14, validate_update v1 replaces the tree comparison with a name-keyed comparison of the parsed index definitions: any added, removed or modified index is rejected with a deterministic DataContractInvalidIndexDefinitionUpdateError naming the offending index, independent of name sort order. The accepted set is unchanged — under v0 no index modification could ever pass the full pipeline, so this only makes the rejection deterministic and correctly labeled. Gated per the new-version-module convention: new DPP_VALIDATION_VERSIONS_V5 (document_type.validate_update: 1) wired into v14 only; the v0 flow is byte-for-byte frozen for protocol versions <= 13 and pinned by a regression test. Generation-independent helpers (validate_config, byte-array encoding stability, validate_schema) move verbatim to a shared common module, following the try_from_schema extraction precedent (#4276). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
🔍 Review in progress — actively reviewing now (commit ae98e86) |
|
Warning Review limit reached
Next review available in: 54 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 (2)
📝 WalkthroughWalkthroughProtocol v14 now uses document-type update validation version 1. Shared validation enforces immutable configuration, byte-array encoding stability, and schema compatibility. Version 1 compares indexes by name and reports deterministic changes. ChangesDocument-type update validation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant PlatformV14
participant validate_update
participant validate_update_v1
participant DocumentTypeRef
PlatformV14->>validate_update: select validation version 1
validate_update->>validate_update_v1: dispatch document-type update
validate_update_v1->>DocumentTypeRef: validate configuration, encoding, schema, and indexes
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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4291 +/- ##
============================================
+ Coverage 87.57% 87.60% +0.02%
============================================
Files 2700 2702 +2
Lines 344254 344605 +351
============================================
+ Hits 301493 301896 +403
+ Misses 42761 42709 -52
🚀 New features to boost your workflow:
|
The ranked aggregate index grammar (#4266) only exists at protocol v14, where validate_update now dispatches to v1, so the rankedAverageable update tests added to the v0 module assert the v1 rejection shape ("changed index 'byRestaurant'") and belong in the v1 module. The ranked flags ride the same name-keyed index-definition comparison as every other index property. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/rs-dpp/src/data_contract/document_type/methods/validate_update/common/mod.rs (1)
167-199: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for the untested
validate_configguards.The
validate_configtests cover 11 of the flags. Five guards have no test:documents_keep_transfer_history,documents_keep_purchase_history,documents_keep_pricing_history,documents_summable, andrange_summable. The comment at Lines 312-318 states that a change to the sum configuration invalidates every on-disk sum contribution. A guard without a test can be reordered or removed without a failing test.Add one test per missing guard, in the same shape as
should_return_invalid_result_when_range_countable_is_changed. For the sum guards, keep the doctype-level flag equal across old and new when you testrange_summable, so the check below it is reached.Do you want me to generate these tests?
Also applies to: 319-347
🤖 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/methods/validate_update/common/mod.rs` around lines 167 - 199, Add one `validate_config` test for each missing guard: `documents_keep_transfer_history`, `documents_keep_purchase_history`, `documents_keep_pricing_history`, `documents_summable`, and `range_summable`. Follow the structure of `should_return_invalid_result_when_range_countable_is_changed`; when testing `range_summable`, keep the doctype-level sum flag unchanged so validation reaches that guard.packages/rs-dpp/src/data_contract/document_type/methods/validate_update/v0/mod.rs (1)
143-148: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert which error the late-name addition returns.
expect_erraccepts anyProtocolError. The comment states the expected cause is the schema-compatibility check for theindiceskeyword. If a future change makes this path fail for an unrelated reason, the test still passes and the frozen v0 behavior drifts without a signal. The early-name half of this test already pins an exact string at Line 140, so the two halves are pinned at different strengths.Bind the error and assert its variant or its message.
♻️ Proposed change to pin the error
// The identical addition under a late-sorting name passes the tree // comparison and instead hard-errors in the schema compatibility // check ("schema keyword 'indices' ... is not supported"). - old.as_ref() + let late_error = old + .as_ref() .validate_update(new_late_name.as_ref(), platform_version) .expect_err("late-name addition should error in schema compatibility"); + + assert!( + late_error.to_string().contains("indices"), + "expected the schema compatibility hard error for the `indices` keyword, got {late_error}" + );🤖 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/methods/validate_update/v0/mod.rs` around lines 143 - 148, Update the late-name addition assertion in the v0 validate-update test to bind the returned ProtocolError and verify it is specifically the schema-compatibility failure for the unsupported "indices" keyword, using the existing exact-message assertion pattern or an equivalent variant match. Keep the current expectation that validation returns an error while rejecting unrelated failure causes.
🤖 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/methods/validate_update/common/mod.rs`:
- Around line 167-199: Add one `validate_config` test for each missing guard:
`documents_keep_transfer_history`, `documents_keep_purchase_history`,
`documents_keep_pricing_history`, `documents_summable`, and `range_summable`.
Follow the structure of
`should_return_invalid_result_when_range_countable_is_changed`; when testing
`range_summable`, keep the doctype-level sum flag unchanged so validation
reaches that guard.
In
`@packages/rs-dpp/src/data_contract/document_type/methods/validate_update/v0/mod.rs`:
- Around line 143-148: Update the late-name addition assertion in the v0
validate-update test to bind the returned ProtocolError and verify it is
specifically the schema-compatibility failure for the unsupported "indices"
keyword, using the existing exact-message assertion pattern or an equivalent
variant match. Keep the current expectation that validation returns an error
while rejecting unrelated failure causes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d14909f8-dbd7-4ab0-8de9-625bd50bd9cc
📒 Files selected for processing (7)
packages/rs-dpp/src/data_contract/document_type/methods/validate_update/common/mod.rspackages/rs-dpp/src/data_contract/document_type/methods/validate_update/mod.rspackages/rs-dpp/src/data_contract/document_type/methods/validate_update/v0/mod.rspackages/rs-dpp/src/data_contract/document_type/methods/validate_update/v1/mod.rspackages/rs-platform-version/src/version/dpp_versions/dpp_validation_versions/mod.rspackages/rs-platform-version/src/version/dpp_versions/dpp_validation_versions/v5.rspackages/rs-platform-version/src/version/v14.rs
…e error Adds the five validate_config guard tests that were missing (keeps transfer/purchase/pricing history, documentsSummable, rangeSummable — the latter with documentsSummable held equal so the guard below it is reached), and pins the v0 late-name index addition to the exact schema-compatibility "indices" keyword error instead of accepting any ProtocolError. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Both review nitpicks addressed in 17cd413:
🤖 Addressed by Claude Code |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
The latest delta strengthens validate_config and v0 regression coverage and moves ranked-index tests into the correct v1 module; no new latest-delta defects were found. Carried forward: one test-coverage suggestion remains valid because all eight v1 dispatcher tests still use the moving PlatformVersion::latest() target instead of protocol version 14.
Source: reviewers Codex gpt-5.6-sol and Claude claude-sonnet-5; final verifier Codex gpt-5.6-sol as fallback for the Sonnet verifier; openclaw-agent coordinator is orchestration-only.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— rust-quality (completed) - Verifier:
gpt-5.6-sol— final-verifier (fallback) - Sonnet reviewers:
claude-sonnet-5— general (failed),claude-sonnet-5— security-auditor (failed),claude-sonnet-5— rust-quality (failed),claude-sonnet-5— general (completed),claude-sonnet-5— security-auditor (completed),claude-sonnet-5— rust-quality (completed)
🟡 1 suggestion(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/methods/validate_update/v1/mod.rs`:
- [SUGGESTION] packages/rs-dpp/src/data_contract/document_type/methods/validate_update/v1/mod.rs:175: Pin v1 consensus tests to protocol version 14
All eight tests in this version-specific module call the public `validate_update` dispatcher with `PlatformVersion::latest()` at lines 175, 229, 258, 284, 314, 341, 429, and 448. `LATEST_VERSION` currently equals protocol version 14, whose validation configuration selects `validate_update_v1`, but a future version selecting v2 would silently redirect these tests and leave the protocol-v14 consensus behavior without permanent regression coverage. Pin each test to version 14, matching the adjacent v0 regression test's explicit version-13 pin and the generation-pinning convention documented by the versioned schema-parser tests.
PlatformVersion::latest() is a moving target: a future protocol version selecting a v2 dispatcher would silently redirect these tests and leave the v14 consensus behavior without permanent regression coverage. Pin to version 14 explicitly, matching the v0 module's version-13 pin. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This reverts commit 5078c6a.
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
`@packages/rs-dpp/src/data_contract/document_type/methods/validate_update/v0/mod.rs`:
- Around line 151-156: Update the assertion for late_error in the
validate-update regression test to match the concrete ProtocolError variant and
compare its message exactly, rather than using contains on
late_error.to_string(). Preserve the expected schema-compatibility message for
the indices path and retain useful failure context.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: facf216f-2235-4ff1-8c29-7cfb2bd9bb80
📒 Files selected for processing (3)
packages/rs-dpp/src/data_contract/document_type/methods/validate_update/common/mod.rspackages/rs-dpp/src/data_contract/document_type/methods/validate_update/v0/mod.rspackages/rs-dpp/src/data_contract/document_type/methods/validate_update/v1/mod.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/rs-dpp/src/data_contract/document_type/methods/validate_update/v1/mod.rs
Replaces the contains() check with an assert_matches on the concrete ProtocolError -> DataContractError::JsonSchema -> SchemaCompatibilityValidationError chain and the exact inner message, so the frozen v0 late-name behavior cannot drift to a different error without a signal. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 participants
Issue being fixed or feature implemented
IndexLevel::validate_updatecompares old/new index trees bylevel_identifierequality, but identifiers are assigned by an incrementing counter that walks the document type'sindices— aBTreeMapkeyed by index name. As a result, whether a semantically identical index change was rejected with a proper consensus error or with something opaque depended purely on where the changed index's name sorted relative to the document type's other indexes:"Invalid path"message.indiceskeyword) → surfaced as anInternalError(code 1) tx result instead of a clean consensus error.unique/nullSearchableflips (which the tree comparison is blind to) hit the same lottery.Notably, no index modification was ever acceptable end-to-end under v0 — everything that survived the tree diff was killed by the
indicesschema-compatibility hard error. The name only determined which kind of failure you got. Since paid-invalid transitions record their error codes in blocks, the outcome is consensus-visible and the fix must be protocol-gated.What was done?
DocumentTypeRef::validate_update(protocol v14) that replaces theIndexLeveltree comparison with a name-keyed comparison of the parsed index definitions: any added, removed or modified index is rejected with a deterministicDataContractInvalidIndexDefinitionUpdateErrorwhose payload names the offending index (added index 'x'/removed index 'x'/changed index 'x'), independent of name sort order.validate_updateentirely.DPP_VALIDATION_VERSIONS_V5(document_type.validate_update: 1) wired into v14 only; v13 stays on V4.validate_config, byte-array encoding stability,validate_schema) verbatim to a sharedcommonmodule, following thetry_from_schemaextraction precedent (refactor(dpp): extract shared try_from_schema parsing helpers #4276). The v0 flow itself is untouched.Known residual (kept out of scope, follow-up planned): reordering the
indicesarray without changing the index set still reaches the schema-compatindiceshard error, because the parsed definitions compare equal. That belongs in the compatibility validator, not here.How Has This Been Tested?
j:[c],k:[a,b], adding[a]as"i"vs"z"); rename, removal, added property, andunique-flip all rejected cleanly; unchanged document type accepted."a -> Invalid path"vs schema-compat error) for replay safety.dppsuite (3889 tests) passes with--all-features;platform-versiontests pass;cargo check --workspace --all-targets,cargo clippyon both touched crates, and the verify-feature-gateddrivebuild are clean.Breaking Changes
None. The change is gated at protocol v14 (unreleased); shipped protocol versions keep byte-identical behavior, and the set of acceptable contract updates does not change at any version.
Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests