feat(drive): boolean HAVING range queries on ranked index axes - #4384
Merged
QuantumExplorer merged 13 commits intoAug 13, 2026
Conversation
Serve a grouped aggregate carrying exactly one HAVING clause on the selected aggregate (GROUP BY p HAVING <agg> <op> <value> LIMIT n) as a value-bounded range read of the covering ranked index's axis secondary — the same grovedb trees the PV14 ranked top-k surface walks — with a completeness-attesting proof. - rs-drive: drive_document_having_query (versioned grammar, bounds translation, executors) + document_having verifier; prover and verifier share one bounds-to-Merk-query translation and path builder - rs-drive-abci: compute_aggregate_mode_and_check_limit v2 routes the shape to dispatch_having_v1; response reuses RankedEntries with skipped unset, so zero proto changes - rs-platform-version: PV14 selects DRIVE_ABCI_QUERY_VERSIONS_V3; detect_having_mode / verify_having_range_proof slots dormant at 0 in all tables; v13 and earlier keep rejecting every non-empty HAVING - rs-drive-proof-verifier / rs-sdk: DocumentHavingEntries with FromProof/Fetch, binding the proof to the quorum-signed app hash Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change adds protocol-versioned ChangesHAVING range query support
Estimated code review effort: 5 (Critical) | ~90 minutes Mergeability Score: 🔵 Low · up to The new HAVING query behavior is mergeable, but the protocol documentation should clarify the valid ORDER BY keys for aggregate queries to avoid client integration errors; this is a bounded follow-up for the owner. Sequence Diagram(s)sequenceDiagram
participant SDK
participant ABCI
participant Drive
participant GroveDB
participant Verifier
SDK->>ABCI: Submit grouped aggregate with HAVING range
ABCI->>Drive: Dispatch HAVING request
Drive->>Drive: Validate mode and resolve ranked index
Drive->>GroveDB: Execute range or generate proof
GroveDB-->>Drive: Return entries or range proof
Drive-->>ABCI: Return HAVING response
ABCI-->>SDK: Return entries and metadata
SDK->>Verifier: Verify HAVING proof
Verifier-->>SDK: Return verified entries
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Collaborator
|
🕓 Ready for review — 2 ahead in queue (commit d94aa01) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
packages/rs-drive-abci/src/query/document_query/v1/mod.rs (1)
1463-1488: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated contract and document-type resolution.
These 26 lines are the fifth copy of the same block.
dispatch_count_v1,dispatch_sum_v1,dispatch_average_v1, anddispatch_ranked_v1contain identical logic, including the two error strings. A future change to the "document type not found" or "contract not found" wording must now be applied in five places.Extract one private helper that returns
(Arc<DataContractFetchInfo>, Identifier)and call it from each dispatcher.🤖 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-drive-abci/src/query/document_query/v1/mod.rs` around lines 1463 - 1488, The repeated contract and document-type resolution should be centralized in one private helper returning (Arc<DataContractFetchInfo>, Identifier). Extract the shared validation and lookup logic from the current block, including both existing error messages, then update dispatch_count_v1, dispatch_sum_v1, dispatch_average_v1, dispatch_ranked_v1, and the current dispatcher to call that helper and use its returned contract information and identifier.packages/rs-drive/src/query/drive_document_having_query/tests.rs (1)
296-366: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd grammar tests for the remaining rejection branches.
The suite covers most of the v0 grammar. Three rejection branches in
detect_having_mode_v0have no test: a non-emptywhere_clauses, more than oneorder_byclause, and an empty select field forSUM/AVG. The unknown-method-version arm ofdetect_having_modeis also untested. Each branch is a one-call test, and each guards a documented request-shape contract.🤖 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-drive/src/query/drive_document_having_query/tests.rs` around lines 296 - 366, Add focused one-call rejection tests alongside limit_is_required_and_capped and offset_and_start_at_are_rejected for non-empty where_clauses, multiple order_by clauses, and empty select fields with SUM or AVG, asserting detect_having_mode_v0 returns an error. Also add a test for detect_having_mode with an unsupported method version, asserting the unknown-version error path.
🤖 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-drive-abci/src/query/document_query/v1/mod.rs`:
- Around line 1513-1524: Update the error-handling block in the having-query
proof path to remove the “should be unreachable” empty-tree comment and replace
the ranking-specific empty_ranking_proof_rejection handling with
having-query-appropriate empty-secondary rejection wording. Preserve the
existing conversion of unrelated errors through Err(e.into()) and align the
message with an_empty_match_set_reads_empty_and_proves_empty.
In `@packages/rs-drive-proof-verifier/src/proof/document_having.rs`:
- Around line 126-136: Update the `other` arm in the response decoding logic to
report only the received result variant, not the full `other` payload. Replace
`{other:?}` with a variant-name representation that avoids serializing document
contents while preserving the existing routing guidance and error type.
Apply the same fix in
`@packages/rs-drive/src/query/drive_document_ranked_query/path.rs` around lines 32
- 40: The shared helper emits ranked-only wording when called by having-range
queries.
In `@packages/rs-drive/src/query/drive_document_having_query/mode_detection.rs`:
- Around line 497-527: Update avg_operand to reject finite float operands whose
value multiplied by AVG_FIXED_POINT_SCALE is not an exact integer, instead of
silently truncating through the f64-to-i128 conversion. Return the existing
InvalidParameter error for inexact scaled values, while preserving accepted
exact values and the current range validation.
In
`@packages/rs-drive/src/verify/document_having/verify_having_range_proof/mod.rs`:
- Around line 37-50: Update the platform-version verification definitions to add
a dedicated verify.document_having group and move verify_having_range_proof out
of document_ranked into it. In the dispatcher using platform_version, update the
match to read document_having.verify_having_range_proof while preserving the
existing version handling and error metadata.
---
Nitpick comments:
In `@packages/rs-drive-abci/src/query/document_query/v1/mod.rs`:
- Around line 1463-1488: The repeated contract and document-type resolution
should be centralized in one private helper returning
(Arc<DataContractFetchInfo>, Identifier). Extract the shared validation and
lookup logic from the current block, including both existing error messages,
then update dispatch_count_v1, dispatch_sum_v1, dispatch_average_v1,
dispatch_ranked_v1, and the current dispatcher to call that helper and use its
returned contract information and identifier.
In `@packages/rs-drive/src/query/drive_document_having_query/tests.rs`:
- Around line 296-366: Add focused one-call rejection tests alongside
limit_is_required_and_capped and offset_and_start_at_are_rejected for non-empty
where_clauses, multiple order_by clauses, and empty select fields with SUM or
AVG, asserting detect_having_mode_v0 returns an error. Also add a test for
detect_having_mode with an unsupported method version, asserting the
unknown-version error path.
🪄 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: 4d767716-5f83-4a98-9a0f-454a3e7a6e92
📒 Files selected for processing (35)
packages/rs-drive-abci/src/query/document_query/v1/compute_aggregate_mode_and_check_limit/mod.rspackages/rs-drive-abci/src/query/document_query/v1/compute_aggregate_mode_and_check_limit/v2/mod.rspackages/rs-drive-abci/src/query/document_query/v1/mod.rspackages/rs-drive-abci/src/query/document_query/v1/tests.rspackages/rs-drive-proof-verifier/src/lib.rspackages/rs-drive-proof-verifier/src/proof.rspackages/rs-drive-proof-verifier/src/proof/document_having.rspackages/rs-drive-proof-verifier/src/proof/document_ranked.rspackages/rs-drive/src/query/drive_document_having_query/drive_dispatcher.rspackages/rs-drive/src/query/drive_document_having_query/execute_range.rspackages/rs-drive/src/query/drive_document_having_query/executors.rspackages/rs-drive/src/query/drive_document_having_query/mod.rspackages/rs-drive/src/query/drive_document_having_query/mode_detection.rspackages/rs-drive/src/query/drive_document_having_query/tests.rspackages/rs-drive/src/query/drive_document_ranked_query/path.rspackages/rs-drive/src/query/mod.rspackages/rs-drive/src/verify/document_having/mod.rspackages/rs-drive/src/verify/document_having/verify_having_range_proof/mod.rspackages/rs-drive/src/verify/document_having/verify_having_range_proof/v0/mod.rspackages/rs-drive/src/verify/mod.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_query_versions/mod.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_query_versions/v3.rspackages/rs-platform-version/src/version/drive_versions/drive_document_method_versions/mod.rspackages/rs-platform-version/src/version/drive_versions/drive_document_method_versions/v1.rspackages/rs-platform-version/src/version/drive_versions/drive_document_method_versions/v2.rspackages/rs-platform-version/src/version/drive_versions/drive_document_method_versions/v3.rspackages/rs-platform-version/src/version/drive_versions/drive_document_method_versions/v4.rspackages/rs-platform-version/src/version/drive_versions/drive_verify_method_versions/mod.rspackages/rs-platform-version/src/version/drive_versions/drive_verify_method_versions/v1.rspackages/rs-platform-version/src/version/v14.rspackages/rs-sdk/src/mock/requests.rspackages/rs-sdk/src/platform/documents/document_having_entries.rspackages/rs-sdk/src/platform/documents/document_query.rspackages/rs-sdk/src/platform/documents/having_proof_helpers.rspackages/rs-sdk/src/platform/documents/mod.rs
Pins the worked example — SELECT AVG(grade) GROUP BY identityId HAVING AVG(grade) > 80 — against a contract whose group key is a 32-byte identifier rather than a string: strict-bound exclusion of an exactly-at-threshold average, inclusion of a fractional average just above it, byte-exact identifier keys in both walk directions, and proof verification against the live root hash. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
GROUP BY identityId, class HAVING AVG(grade) > 80 must be rejected, not misserved: ranked axes live on single-property indexes (a ranked flag on a compound index is already rejected at contract-parse time, covered by dpp's test_index_try_from_ranked_on_compound_index_rejected). Pins the drive grammar rejection and that it surfaces through the abci wire path as InvalidArgument naming the single-property rule. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4384 +/- ##
============================================
- Coverage 87.68% 87.00% -0.69%
============================================
Files 2686 2712 +26
Lines 342538 346704 +4166
============================================
+ Hits 300369 301636 +1267
- Misses 42169 45068 +2899
🚀 New features to boost your workflow:
|
thepastaclaw
suggested changes
Aug 12, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Preliminary review — Codex only
The HAVING range implementation follows the indexed-axis proof architecture, but two core API semantics are incorrect: floating-point AVG predicates can resolve to the wrong fixed-point range, and the documented bound-only pagination cannot continue through aggregate ties. The canonical protobuf documentation and several new error paths also need smaller corrections.
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)
🔴 2 blocking | 🟡 3 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-drive/src/query/drive_document_having_query/mode_detection.rs`:
- [BLOCKING] packages/rs-drive/src/query/drive_document_having_query/mode_detection.rs:517-526: Translate floating AVG bounds without truncating
`avg_operand` casts the scaled floating threshold to `i128` before `bounds_for_operator` applies inclusive or exclusive semantics. The cast truncates toward zero, so it discards information needed to choose the correct integer boundary. If the scaled threshold is `0.5`, `AVG >= threshold` must start at `1`, but this code starts at `0`; if it is `-0.5`, `AVG > threshold` must start at `0`, but this code starts at `1`. A non-integral equality threshold also cannot be converted into an equality lookup on the truncated integer. The conversion must compute operator-specific floor or ceiling bounds from the exact IEEE-754 value, or reject floating operands that cannot be represented with defined fixed-point semantics. A simple `scaled.fract()` check is insufficient at the current 10^19 scale because large `f64` products have already lost sub-integer precision.
In `packages/rs-drive/src/query/drive_document_having_query/mod.rs`:
- [BLOCKING] packages/rs-drive/src/query/drive_document_having_query/mod.rs:254-256: Bound-only pagination loses groups tied at the page boundary
The secondary is ordered by `(sort_key || group_key)`, but callers can adjust only the aggregate-value bound. If a page ends inside a tie, moving the threshold past the last aggregate excludes every remaining group with that value, while keeping the threshold returns the same first page. For example, with 101 groups at the same count and `LIMIT 100`, the final group cannot be retrieved; if more than `MAX_HAVING_LIMIT` groups tie, increasing the limit cannot help either. This contradicts the PR and SDK contract that callers can paginate by tightening the bound. Add a continuation cursor containing the composite secondary key/group key, or remove the pagination claim and explicitly document that result sets cut inside a tie cannot be fully enumerated.
In `packages/dapi-grpc/protos/platform/v0/platform.proto`:
- [SUGGESTION] packages/dapi-grpc/protos/platform/v0/platform.proto:706-711: Update the canonical wire documentation for the new HAVING grammar
The protobuf documentation still says HAVING cannot be combined with aggregate `ORDER BY`, even though PV14 accepts that clause to select the range-walk direction. The request documentation at lines 852-857, 886-888, and 1087-1092 also says every non-empty HAVING is rejected at every protocol version. These comments feed generated API documentation and now contradict the server behavior introduced by this PR. Document the supported PV14 single-clause range shape, its index and pagination restrictions, and the continued rejection on PV13 and earlier.
In `packages/rs-drive-abci/src/query/document_query/v1/mod.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/query/document_query/v1/mod.rs:1513-1524: Use HAVING-specific empty-axis proof diagnostics
The comment says the empty-secondary failure should be unreachable, but `an_empty_match_set_reads_empty_and_proves_empty` explicitly verifies that the HAVING range prover still returns `Cannot create proof for empty tree` for a completely empty secondary. The error is then mapped through `empty_ranking_proof_rejection`, whose user-facing text calls the request a ranking. Keep the typed `InvalidArgument` mapping and retry guidance, but describe this as an empty HAVING axis/range and remove the inaccurate unreachable-path explanation.
In `packages/rs-drive-proof-verifier/src/proof/document_having.rs`:
- [SUGGESTION] packages/rs-drive-proof-verifier/src/proof/document_having.rs:126-136: Keep unexpected HAVING response diagnostics payload-independent
Formatting `{other:?}` serializes the entire unexpected response into the error, including every document byte array when a node returns `ResultData.documents`. This makes diagnostic allocation and logging proportional to an untrusted response payload and can expose returned document contents in logs. Report only whether the result was absent, proof, or the received `ResultData` variant. The shared path helper in `drive_document_ranked_query/path.rs` should likewise use surface-neutral wording because it is now called by both ranked and HAVING queries rather than saying only that “ranked queries” require a single-property index.
6 tasks
…nuation contract Review fixes for the having-range surface: - Float AVG operands now translate through the exact IEEE-754 value with operator-aware floor/ceiling (scaled_avg_operand + avg_bounds_for_operator) instead of f64-multiply-and-truncate, which lost sub-tick precision at the 10^19 scale and could move an inclusive bound by one tick — including the sign-dependent cases around zero. An equality bound between ticks is rejected loudly instead of silently becoming a point lookup on the truncated tick. - The continuation-by-bound story is stated honestly everywhere: a page cut at the limit continues past distinct aggregate values only; a cut inside a tie cannot be continued without a composite-key cursor (future capability), so callers size the limit above the widest expected tie. - The abci empty-axis mapping keeps its typed InvalidArgument but now describes both ranking and HAVING-range shapes, and the having dispatcher's comment no longer claims the path is unreachable (the empty-secondary prove failure is pinned by test). - Unexpected getDocuments result variants are reported by variant name only (shared result_variant_name helper) so error strings and logs cannot grow with — or leak — an untrusted response payload; the shared single-property path error now names both query surfaces. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The exact-head revalidation confirms that the floating-point AVG translation, tie-boundary contract, empty-axis diagnostics, and payload-independent response diagnostics have been corrected. One prior documentation suggestion remains valid, and the HAVING-specific offset rejection still directs callers to unsupported cursor pagination; no blocking issue remains.
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)
🟡 2 suggestion(s)
2 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/dapi-grpc/protos/platform/v0/platform.proto`:
- [SUGGESTION] packages/dapi-grpc/protos/platform/v0/platform.proto:706-711: Update the canonical wire documentation for the new HAVING grammar
The canonical protobuf comments still contradict the PV14 behavior added by this PR. This section says HAVING cannot be combined with aggregate `ORDER BY`, although `detect_having_mode_v0` accepts that combination to choose the range-walk direction. The request documentation at lines 852-857, 886-888, and 1087-1092 also says every non-empty HAVING is rejected at every protocol version. Update the generated-API comments to document the supported PV14 single-clause COUNT/SUM/AVG range shape, the required ranked-axis index and limit, the lack of offset or start cursors, the inability to continue a page cut inside an aggregate tie, and the continued rejection on PV13 and earlier.
In `packages/rs-drive-abci/src/query/document_query/v1/mod.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/query/document_query/v1/mod.rs:419-423: Do not recommend an unsupported cursor for HAVING offsets
A PV14 HAVING request carrying `offset` is first routed to `RoutingDecision::HavingRange` and then rejected by this shared gate. The returned message directs the caller to `start_after` or `start_at`, but `detect_having_mode_v0` rejects both cursor forms because document-ID cursors do not address the aggregate-sorted secondary. Preserve the load-bearing legacy message for other non-ranked routes, but return a HAVING-specific message for `RoutingDecision::HavingRange` explaining that this surface has neither offset nor cursor pagination and can continue only by tightening past a distinct aggregate value, subject to the documented tie limitation.
The canonical platform.proto comments still described the pre-PV14 behavior (every non-empty having rejected at every protocol version, having cannot combine with an aggregate ORDER BY). They now document the served single-clause COUNT/SUM/AVG range shape, the required ranked-axis index and limit, the absence of offset and cursor pagination with the distinct-value continuation and its tie limitation, and the unchanged rejection on v13 and earlier. Clients regenerated (only the Objective-C header embeds comments). The having-range route also gets its own OFFSET rejection message: the legacy one recommends `start_after` / `start_at`, which that surface rejects too, so it now explains continuation-by-bound instead. The legacy message stays byte-identical on every other route. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Member
Author
|
Both final-validation suggestions addressed in 0c80416:
🤖 Addressed by Claude Code |
Contributor
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/dapi-grpc/protos/platform/v0/platform.proto`:
- Around line 706-717: The having-range documentation in GetDocumentsRequestV1
must use accepted aggregate order-key spellings: document ORDER BY f for SUM(f)
and AVG(f), and ORDER BY $count for COUNT(*), instead of ORDER BY <agg>. Update
the corresponding generated documentation in
packages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbobjc.h at lines
2651-2662 after changing packages/dapi-grpc/protos/platform/v0/platform.proto at
lines 706-717; no other sites require changes.
🪄 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: 7557446b-7838-478f-a47a-dc8197cb35ba
📒 Files selected for processing (4)
packages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbobjc.hpackages/dapi-grpc/protos/platform/v0/platform.protopackages/rs-drive-abci/src/query/document_query/v1/mod.rspackages/rs-drive-abci/src/query/document_query/v1/tests.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/rs-drive-abci/src/query/document_query/v1/tests.rs
…ling `ORDER BY <agg>` read as an explicit OrderClause.aggregate target, which the wire rejects; the accepted spelling is the field name for SUM/AVG and the $count sentinel for COUNT(*), same as ranked mode. Objective-C client regenerated. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
approved these changes
Aug 13, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
At exact head 39e14e5, the HAVING-range implementation has no remaining in-scope blocking issues, suggestions, or nitpicks from the supplied Codex and CodeRabbit evidence. Both prior verified findings are fixed: the canonical wire documentation now describes the PV14 grammar and limitations accurately, and HAVING offsets receive surface-specific continuation guidance.
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)
…luation-feasibility-5b3db9 # Conflicts: # packages/rs-drive-abci/src/query/document_query/v1/tests.rs # packages/rs-platform-version/src/version/drive_versions/drive_document_method_versions/mod.rs # packages/rs-platform-version/src/version/drive_versions/drive_document_method_versions/v1.rs # packages/rs-platform-version/src/version/drive_versions/drive_document_method_versions/v2.rs # packages/rs-platform-version/src/version/drive_versions/drive_document_method_versions/v3.rs # packages/rs-platform-version/src/version/drive_versions/drive_document_method_versions/v4.rs
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The HAVING-range implementation correctly preserves versioned dispatch, reconstructs proof bounds consistently, and binds verified GroveDB roots to Tenderdash signatures; no blocking correctness issue remains. Two nonblocking gaps remain: the public Drive HAVING documentation still describes the feature as entirely unsupported, and the new client trust-boundary wrapper lacks direct end-to-end proof-composition coverage.
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)
🟡 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-drive-proof-verifier/src/proof/document_having.rs`:
- [SUGGESTION] packages/rs-drive-proof-verifier/src/proof/document_having.rs:166-179: Exercise the HAVING proof through the Tenderdash trust boundary
This wrapper is the security-critical composition that turns a valid GroveDB proof into an authenticated platform result by binding its reconstructed root to the quorum-signed app hash. The current tests cover GroveDB proof verification, server-side proof emission, request-shape resolution, and unproved response decoding, but no test invokes this function or the SDK `DocumentHavingEntries::fetch` proof path. Add an integration test using a real HAVING proof, response metadata, and context provider that accepts the correctly signed root and rejects a mismatched signed app hash or tampered metadata. That directly protects the SDK trust boundary against accidentally omitting or miswiring `verify_tenderdash_proof`.
In `packages/rs-drive/src/query/having.rs`:
- [SUGGESTION] packages/rs-drive/src/query/having.rs:30-37: Update the public Drive HAVING documentation
This public module documentation still says that no HAVING grammar executes and every non-empty clause is rejected. That is now false: protocol version 14 accepts the single-clause COUNT/SUM/AVG contiguous-range surface implemented by `detect_having_mode_v0`. Update the canonical Drive type documentation to describe the supported v14 shape while retaining the rejection contract for earlier protocol versions and unsupported forms such as multiple clauses, `NOT_EQUAL`, and `IN`.
…oundary Four tests on the proof-verifier's verify_having_range_proof wrapper — the composition that turns a grovedb-valid proof into an authenticated result by binding its reconstructed root to the quorum-signed app hash. A real Drive generates a real AVG having proof; a test quorum key signs the canonical precommit. The correctly signed root verifies and returns exactly the matching groups; a commit over a different app hash, tampered response metadata, and a wrong quorum key are each rejected at the signature binding — so omitting or miswiring verify_tenderdash_proof turns a test red. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The ranked-index examples chapter still said every non-empty having is Unsupported at every protocol version. Rewrite the HAVING paragraph and the rejection-table row to the shipped contract: a grouped single-clause bound on the selected aggregate is served as a value-bounded range read since protocol v14 (direction via ORDER BY on the aggregate, no rank pagination, contiguous operators only), while multi-clause, cross-aggregate, non-contiguous, ungrouped, and offset shapes stay rejected, and v13 rejects everything unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
|
📖 Book Preview built successfully. Download the preview from the workflow artifacts. Updated at 2026-08-13T12:51:19.337Z |
thepastaclaw
suggested changes
Aug 13, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Preliminary review — Codex only
The HAVING implementation remains correctly version-gated and now directly tests the Tenderdash proof-composition boundary, but that test introduces a blocking client/server dependency-boundary violation by enabling Drive's full feature inside the client proof-verifier crate. Three public documentation surfaces also remain inconsistent or incomplete: Drive and SDK rustdocs still describe HAVING as wholly unsupported, while the ranked-index book omits the documented aggregate-tie pagination limitation.
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 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 | 🟡 3 suggestion(s)
2 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-drive-proof-verifier/Cargo.toml`:
- [BLOCKING] packages/rs-drive-proof-verifier/Cargo.toml:55-62: Do not enable Drive's server feature from the client verifier crate
`drive-proof-verifier` is a client-side proof-verification crate, but this new dev-dependency enables Drive's `full` feature, which includes `server`. During this crate's test build, Cargo unifies the normal `verify` dependency with the dev-dependency and compiles Drive with both client verification and node-side storage/proving functionality. That violates the repository's client/server dependency boundary even though the dependency is test-only. Move proof generation and state setup into a server-side integration harness, or use a checked-in/generated proof fixture here so the client verifier continues to build Drive with `verify` only.
In `packages/rs-drive/src/query/having.rs`:
- [SUGGESTION] packages/rs-drive/src/query/having.rs:30-37: Update the public Drive HAVING documentation
This public module documentation still says that no HAVING grammar executes and every non-empty clause is rejected. That is now false: protocol version 14 accepts the single-clause COUNT/SUM/AVG contiguous-range surface implemented by `detect_having_mode_v0`. Update the canonical Drive type documentation to describe the supported v14 shape while retaining the rejection contract for earlier protocol versions and unsupported forms such as multiple clauses, `NOT_EQUAL`, and `IN`.
In `packages/rs-sdk/src/platform/documents/document_query.rs`:
- [SUGGESTION] packages/rs-sdk/src/platform/documents/document_query.rs:293-299: Update the public with_having builder documentation
The public `with_having` builder still states that every non-empty HAVING value is rejected and that the method exists only for future support. This contradicts the updated `DocumentQuery::having` field documentation and the PV14 route implemented by this PR. Document the supported single-clause COUNT/SUM/AVG range shape here, along with continued rejection on PV13 and earlier and for unsupported clause forms.
In `book/src/drive/ranked-index-examples.md`:
- [SUGGESTION] book/src/drive/ranked-index-examples.md:36: State the tie limitation in the ranked-book HAVING continuation guidance
The new book text says a HAVING page continues by tightening the bound past the last value seen, but this does not work when `LIMIT` cuts inside an aggregate tie. Retaining the boundary value repeats the same page, while moving past it permanently excludes the remaining tied groups. The Drive query and proof-verifier documentation already state this limitation; add the same caveat here so the book does not present bound tightening as complete pagination for every result set.
… into files Pure motion, no behavior change: mod.rs was 1748 lines holding the shape-routing table, six per-route dispatchers, and their wire-shape helpers. Now mod.rs keeps the handler entry (query_documents_v1), RoutingDecision, and the shared not_yet_implemented helper; routing.rs holds validate_and_route + the offset gate + the test probe; and dispatch/ holds one file per RoutingDecision arm (documents, count, sum, average, ranked, having), each with its own impl block and its route-local helpers, with the entry-shape helpers shared by the ranked + having pair in dispatch/mod.rs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ection Both mode_detection.rs files held the version-table dispatcher and the frozen v0 grammar in one file, unlike the verify_having_range_proof module next door. Conform them to the file-based versioning scheme: mode_detection/mod.rs keeps the dispatcher (and ranked_order_key on the ranked side, which is version-independent and externally referenced), mode_detection/v0/mod.rs holds the frozen grammar and its private helpers. The _v0 entry points are re-exported so drive_dispatcher and the test suites keep their import paths. Pure motion, no behavior change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The tenderdash trust-boundary tests dev-depended on drive's full feature from inside the client proof-verifier crate; feature unification then compiled node-side storage/proving into the client crate's test build, violating the client/server dependency boundary. Move the suite to rs-drive-abci (which already builds drive server and already dev-depends on drive-proof-verifier) as having_trust_boundary: same four tests, now generating proofs on the server side and running the client wrapper against them. drive-proof-verifier's drive dependency is back to verify-only in both sections. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Three public surfaces still described HAVING as wholly unsupported or over-promised its pagination: the rs-drive having-grammar module docs and the rs-sdk with_having builder docs now state the served PV14 shape (one clause bounding the selected aggregate, contiguous operators) alongside what stays rejected and the PV13 behavior; the ranked book chapter's continuation guidance now carries the aggregate-tie limitation the Drive and proof-verifier rustdocs already state (a limit cut inside a tie cannot be continued by bound tightening). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
QuantumExplorer
deleted the
claude/having-evaluation-feasibility-5b3db9
branch
August 13, 2026 12:52
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
Grouped aggregate queries could rank groups by their aggregate (
GROUP BY p ORDER BY <agg> LIMIT n, the PV14 ranked surface) but could not filter groups by an aggregate bound — e.g. "hashtags with more than 100 posts". Any non-emptyhavingon the wire was rejected at every protocol version.What was done?
Implements a boolean-
HAVINGrange query surface end to end, served as a value-bounded range read of the covering ranked index's axis secondary (the same grovedb trees the ranked top-k surface walks), so a having read isO(log n + k)with a completeness-attesting proof.Query shape (v1 grammar,
detect_having_mode_v0in drive):SELECT COUNT(*)/SUM(f)/AVG(f) ... GROUP BY p HAVING <the selected aggregate> <op> <value> LIMIT n=,>,>=,<,<=and the fourBETWEENvariants translate to inclusive per-axis bounds (!=/INrejected — non-contiguous)order_byoptional (may name the selected aggregate to flip walk direction); limit required,1..=100; no offset/start_at — pagination is "tighten the bound past the last value seen"ANDand cross-aggregate predicates keep thenot_yet_implementedcontractLayer by layer:
query/drive_document_having_query/module (versioned grammar, bounds translation, executors over grovedb'sindexed_*_range/prove_indexed_*_query) andverify/document_having/verifier. Prover and verifier share one bounds→Merk-query translation (AxisRangeBounds::merk_query) and one path builder, so they cannot drift.compute_aggregate_mode_and_check_limitv2 routes a grouped single-clausehavingto the newdispatch_having_v1. The response reuses the existingRankedEntrieswire message withskippedunset — zero proto changes, zero client regeneration.DRIVE_ABCI_QUERY_VERSIONS_V3; drive slotsdetect_having_mode/verify_having_range_proofexist at 0 in all tables (same dormancy pattern asdetect_ranked_mode). v13 and earlier keep rejecting the shape, so mixed-version networks agree across the upgrade.DocumentHavingEntrieswithFromProof/Fetch— client-side verification binds the proof's reconstructed root hash to the quorum-signed app hash, and Merk range boundaries attest completeness (an in-range group the node omitted fails verification).No grovedb changes — the pinned revision already ships
indexed_{count,sum,avg}_rangeandprove/verify_indexed_*_query.How Has This Been Tested?
103 tests across the stack, all passing:
cargo check --workspace --all-targetsclean, clippy clean on all touched crates,cargo fmtapplied. rs-drive additionally checked--no-default-featuresand--all-features --all-targets.Known limitation: proving against a completely empty axis secondary (fresh contract) fails at the grovedb layer ("Cannot create proof for empty tree"); drive-abci maps this to a clear InvalidArgument suggesting
prove=false, the same class as the existing ranked-surface limitation. Populated-tree absence proofs work fine.Breaking Changes
None for deployed networks: the routing gate lives in PV14, which is unreleased. PV13 and earlier nodes keep rejecting every non-empty
havingexactly as before.Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
HAVINGrange forCOUNT,SUM, orAVG.HAVINGresults, including empty responses.Documentation
Bug Fixes