Uh oh!
There was an error while loading. Please reload this page.
fix(drive): enforce aggregate query limits - #4149
Merged
Merged
Conversation
Contributor
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR introduces explicit aggregate and bounded-distinct range-sum walk modes, applies configured no-proof limits, optionally prunes and stages Swift SDK Cargo artifacts, and hardens Kotlin emulator and macOS Rust CI workflows. ChangesRange-sum walk modes
Swift SDK target pruning
Kotlin emulator CI
Rust workspace CI
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant DocumentSumDispatcher
participant execute_range_sum_no_proof
participant distinct_sum_path_query
DocumentSumDispatcher->>DocumentSumDispatcher: derive effective distinct limit
DocumentSumDispatcher->>execute_range_sum_no_proof: pass RangeSumWalkMode
execute_range_sum_no_proof->>distinct_sum_path_query: execute bounded distinct query
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
QuantumExplorer
marked this pull request as ready for review
July 20, 2026 13:26
Collaborator
🕓 Ready for review — 34 ahead in queue (commit 8aaad75) |
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Final validation — Codex + Sonnet
The fix correctly threads an effective, clamped limit from the dispatcher into RangeSumOptions.distinct_limit, which flows into distinct_sum_path_query's SizedQuery::limit — verified in code at execute_range_sum.rs:152-161 and path_query.rs:663 — and fails closed if a future caller omits the bound. No correctness defects found in the production changes; the two agents converge on a real test-coverage gap (the new test only exercises the pure normalization helper, not the storage boundary), plus two independent minor architecture nitpicks from Claude.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— general (superseded_coverage),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— rust-quality (superseded_coverage),gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (completed) - Verifier:
claude-sonnet-5— final-verifier - Sonnet reviewers:
claude-sonnet-5— general (failed),claude-sonnet-5— general (superseded_coverage),claude-sonnet-5— rust-quality (failed),claude-sonnet-5— rust-quality (superseded_coverage),claude-sonnet-5— general (completed),claude-sonnet-5— rust-quality (completed)
🟡 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_sum_query/drive_dispatcher.rs`:
- [SUGGESTION] packages/rs-drive/src/query/drive_document_sum_query/drive_dispatcher.rs:284-316: New regression test only covers the normalization helper, not the storage-boundary fix it's meant to protect
`no_proof_distinct_limit_uses_the_default_and_clamps_to_the_maximum` calls `effective_no_proof_distinct_limit` directly and never exercises `execute_document_sum_request` with `SumMode::GroupByRange`/`GroupByCompound` and `prove: false`. I confirmed there is no existing no-proof integration test for this path either — the two `SumMode::GroupByRange` tests in `tests.rs` (lines ~444 and ~540) both set `prove: true`. That means the exact regression this PR fixes — the effective limit getting lost before reaching `SizedQuery::limit` in `distinct_sum_path_query` (path_query.rs:663) — could recur while every test in this diff continues to pass. The analogous count module already has this style of end-to-end no-proof test (`test_compound_range_in_summed_no_proof_uses_per_in_aggregate_fanout`, drive_document_count_query/tests.rs:1182, using `execute_document_count_request` against a live `Drive` with `prove: false`), so there's a direct template to follow: insert more matching documents than the limit, call the dispatcher with `prove: false` and `limit: None`/explicit/over-max, and assert the returned `Entries` are capped at the default/explicit/clamped count respectively. Also add a direct executor-level case asserting `distinct_limit: None` reaching `execute_range_sum_no_proof` in distinct mode returns the `InvalidLimit` error rather than reaching storage unbounded.
- [SUGGESTION] packages/rs-drive/src/query/drive_document_sum_query/drive_dispatcher.rs:25-46: u16::try_from error branch in effective_no_proof_distinct_limit is dead code
`default_query_limit` and `max_query_limit` are both declared `u16` on `DriveConfig` (config.rs:52,72, confirmed). `effective_limit` is `requested_limit.unwrap_or(default_query_limit as u32).min(max_query_limit as u32)`, so after the `.min()` it can never exceed `u16::MAX` — `u16::try_from(effective_limit)` at line 41 can never return `Err`, and no test can exercise the "does not fit u16" error message because it's unreachable given the current types. A maintainer reading that error text would reasonably assume it's a real, testable failure path. Replace it with a direct `effective_limit as u16` and a short comment noting it's infallible because both `DriveConfig` fields feeding it are `u16`.
In `packages/rs-drive/src/query/drive_document_sum_query/mod.rs`:
- [SUGGESTION] packages/rs-drive/src/query/drive_document_sum_query/mod.rs:266-282: RangeSumOptions can still represent the invalid state this PR fails closed on at runtime
`return_distinct_sums_in_range: bool` and `distinct_limit: Option<u16>` are independently settable, but `distinct_limit` is only meaningful when `return_distinct_sums_in_range` is true. `RangeSumOptions::default()` itself is safe (both fields default to false/None, a valid non-distinct combination — confirmed via `#[derive(Clone, Debug, Default)]` at mod.rs:267), but nothing stops a future caller from constructing the invalid pair via `RangeSumOptions { return_distinct_sums_in_range: true, ..Default::default() }`, which is only caught by the runtime `ok_or_else` check added in execute_range_sum.rs:152-156. Modeling this as `enum SumWalkMode { Aggregate, Distinct(u16) }` in place of the bool + Option<u16> pair would make the invalid state unrepresentable at compile time — exactly the kind of footgun this PR is otherwise trying to close for future callers — and would let you drop the runtime check entirely.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## v4.1-dev #4149 +/- ##
============================================
+ Coverage 87.47% 87.49% +0.01%
============================================
Files 2661 2663 +2 Lines 336035 336414 +379 ============================================
+ Hits 293957 294354 +397 + Misses 42078 42060 -18
🚀 New features to boost your workflow:
|
QuantumExplorer
requested review from
ZocoLini, llbartekll and shumkov
as code ownersJuly 21, 2026 07:24
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/swift-sdk/build_ios.sh (1)
162-165: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSafeguard directory removal against an unset
TARGET_DIR.To prevent accidental deletion in the root filesystem if
TARGET_DIRis empty or unset, require it to be set.🛡️ Proposed fix
rm -rf \ - "$TARGET_DIR/aarch64-apple-ios" \- "$TARGET_DIR/aarch64-apple-ios-sim" \- "$TARGET_DIR/aarch64-apple-darwin"+ "${TARGET_DIR:?}/aarch64-apple-ios" \+ "${TARGET_DIR:?}/aarch64-apple-ios-sim" \+ "${TARGET_DIR:?}/aarch64-apple-darwin"🤖 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/swift-sdk/build_ios.sh` around lines 162 - 165, Require TARGET_DIR to be set before the rm -rf cleanup command in build_ios.sh, using a shell-safe guard that exits or fails when it is unset or empty; preserve the existing removal of the three target-specific directories once the variable is validated.
🤖 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/swift-sdk/build_ios.sh`:
- Line 73: Safeguard all destructive cleanup paths in build_ios.sh against an
empty TARGET_DIR or target. At packages/swift-sdk/build_ios.sh lines 73-73,
update the rm command to require both variables via ${TARGET_DIR:?} and
${target:?}; at lines 162-165, replace each $TARGET_DIR reference with
${TARGET_DIR:?} while preserving the existing architecture-specific paths.
---
Nitpick comments:
In `@packages/swift-sdk/build_ios.sh`:
- Around line 162-165: Require TARGET_DIR to be set before the rm -rf cleanup
command in build_ios.sh, using a shell-safe guard that exits or fails when it is
unset or empty; preserve the existing removal of the three target-specific
directories once the variable is validated.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f0435fe1-1e06-4d7e-a11e-617eaadafc4f
📒 Files selected for processing (9)
.github/workflows/swift-sdk-build.ymlpackages/rs-drive/src/query/drive_document_average_query/drive_dispatcher.rspackages/rs-drive/src/query/drive_document_sum_query/drive_dispatcher.rspackages/rs-drive/src/query/drive_document_sum_query/execute_range_sum.rspackages/rs-drive/src/query/drive_document_sum_query/executors/range_no_proof.rspackages/rs-drive/src/query/drive_document_sum_query/mod.rspackages/rs-drive/src/query/drive_document_sum_query/tests.rspackages/rs-drive/src/query/mod.rspackages/swift-sdk/build_ios.sh
Uh oh!
There was an error while loading. Please reload this page.
…t-sum-query-limit # Conflicts: # packages/swift-sdk/build_ios.sh
QuantumExplorer
commented
Jul 21, 2026
MemberAuthor
Addressed the full review summary. ee3c9af adds storage-boundary no-proof regression coverage and removes the dead conversion branch through RangeSumWalkMode. b7a3bd5 guards all Swift build cleanup paths with non-empty TARGET_DIR and target assertions; shell syntax and explicit empty-variable abort checks pass. |
QuantumExplorer
commented
Jul 22, 2026
MemberAuthor
reviewed |
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 freeto 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.
What
Why
One aggregate query family did not preserve its request bound at the storage boundary. This change centralizes the effective limit and fails closed if a future caller omits it.
This covers PLATFORM-DS-CAND-386.
Checks
Summary by CodeRabbit
New Features
Bug Fixes