export PrincipalBucketHash - #1069
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
General PR Review: export PrincipalBucketHashBlocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0 Review SummaryThe new commit ( Risk triage (incremental commit): Silence no — a divergence now fails the build rather than escaping. Durability no — no stored key, leaf framing, or manifest change; Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agents |
c499711 to
e7c4bb5
Compare
Review feedback on the export: - Add PrincipalDigestBucket(rt, id, level) so callers get the bucket index from a signature that enforces level <= DigestBucketHashBits, instead of hand-deriving it from a doc-comment formula. - GetEntitlementGrantDigestNodes / ScanEntitlementGrantBucket now error on a level past digestMaxWidthBits instead of clamping: a clamped Level paired with a caller's unclamped Index silently scanned the wrong bucket. - Compile-time assertions couple digestBucketHashLen*8 to digestMaxWidthBits, which the exported constant's width promise depends on. - Golden-vector test pins PrincipalBucketHash's literal output (ABI, tied to GrantDigestABIVersion); the existing tests only proved it consistent with the primitives it calls. - The served-buckets test now covers the stored-leaf fold path (levels 1-3) as well as the index-scan fallback, and asserts the level-17 error on every entry point. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| if level > digestMaxWidthBits { | ||
| return nil, false, fmt.Errorf("pebble: grant-digest level %d exceeds bucket-hash resolution %d", level, digestMaxWidthBits) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: this is a deliberate default-behavior break on an exported interface method — a caller that previously passed level > 16 got a clamped result set, and now gets an error (same for ScanEntitlementGrantBucket). The rationale is well argued in the doc comments, but the PR body only describes the PrincipalBucketHash export, and pkg/sdk/version.go stays at v0.24.1. Per this repo's SDK criteria, a default-behavior change should carry a 0.x minor bump and a line in the PR description so downstreams have a signal beyond reading the diff. (confidence: medium — the surface looks new enough that no external caller is likely relying on the clamp) (confidence: medium)
There was a problem hiding this comment.
PR description now calls out the default-behavior change (error instead of clamp past the bucket-hash resolution). Left pkg/sdk/version.go alone: version bumps in this repo land as standalone "Update SDK version to vX" release commits, not inside feature PRs — flagging that the next release should be a minor (0.25.0) rather than a patch. Leaving this thread open for a maintainer call on the bump.
- PrincipalBucketHash's recipe now says which levels are cheap (native, folded leaves) and that finer levels cost an O(grants) index scan. - connectorstore's error contract points at the native Level as the always-in-range bound and at pebble.DigestBucketHashBits for the full width, so interface-only consumers can discover the limit. - Fix the served-buckets test comment: level 4 exceeds the native width (3) and takes the index-scan path, not the stored-leaf fold. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
squire has apparently been impersonating me in this PR... those comments are not mine 😨 |
ScanEntitlementGrantBucket bounded Level but passed Index through, and
bucketBounds shifts an oversized index's high bits away — so
{Level: 4, Index: 20} silently scanned bucket 4. Reject
Index >= 2^Level (Level 0 keeps its Index-ignored contract), the same
treatment the Level axis got.
The served-buckets test now also runs level 16 — DigestBucketHashBits
itself, the finest permitted level, where the shifts on both sides
consume the full stored width — pinning the accept side of the bound
whose reject side the level-17 subtest pins.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The ">> (16 - bits)" shifts in bucketOfHash, foldedLeafBuckets and computeBucketsAtWidth assume the leaf-prefix width and the resolution bound agree; growing digestMaxWidthBits (which DigestBucketHashBits's doc invites) without digestLeafPrefixLen would compile and then panic on a negative shift at read time. Same assertion pattern as the bucket-hash pair. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 participants
PrincipalBucketHash is now public as we will want to calculate this hash on the c1 side to store an index of grants by their bucket hash.
This is the bucket hash, not the content hash which is already exported.
Review hardening (cb358eb, a7ce6b8, 18e8dac):
PrincipalDigestBucket(rt, id, level)so callers get the bucket index from a signature that enforceslevel <= DigestBucketHashBitsinstead of hand-deriving it.GetEntitlementGrantDigestNodes/ScanEntitlementGrantBucketnow return an error for a level past the bucket-hash resolution (16 bits) instead of clamping, andScanEntitlementGrantBucketalso rejectsIndex >= 2^Levelinstead of wrapping to the low bits. Previously such calls silently returned the wrong bucket's data; buckets built viaPrincipalDigestBucketare always in range.