Repository navigation
feat(platform-wallet): derive application encryption keys with the DIP-13 rotation level - #5649
Conversation
…P-13 rotation level DIP-13's application encryption path now ends with a hardened 31-bit rotation level: m/9'/coin'/5'/7'/0'/identityId'/contractId'/purpose'/rotation'. - ConnectKey::AppEncryption gains `rotation`; the pre-rotation 8-level layout is the explicit ConnectKey::AppEncryptionLegacy, kept so wallets recognise testnet/devnet keys registered before the change. derivation_path returns a Result and refuses rotations >= 2^31. - The rotation level is appended locally because the pinned rust-dashcore only builds the 8-level path; it is replaced by key-wallet's application_encryption_path(..., rotation) from dashpay/rust-dashcore#1250 once platform bumps, and the pinned vectors keep that swap a no-op. - FFI: dash_sdk_derive_connect_key_with_resolver takes `rotation`; new dash_sdk_derive_legacy_app_encryption_key_with_resolver. - Swift: the app-encryption deriveConnectKey overload requires `rotation: UInt32`; deriveLegacyAppEncryptionKey derives the old layout. Session-key calls are unchanged. - JNI/Kotlin mirror the same shape. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 15 minutes. View limit details
No actionable comments were generated in the recent review.
|
| Layer / File(s) | Summary |
|---|---|
Rotated and legacy derivation paths packages/rs-platform-wallet/src/wallet/identity/network/identity_handle.rs |
AppEncryption paths now include a validated rotation level. AppEncryptionLegacy retains the prior path. Tests cover path structure, limits, vectors, and distinct derived keys. |
FFI APIs and validation packages/rs-platform-wallet-ffi/src/derive_connect_key.rs |
The current API accepts rotation and validates sub-feature, purpose, and rotation combinations. A legacy API derives keys without rotation. Tests cover vectors and invalid inputs. |
Kotlin and JNI APIs packages/rs-unified-sdk-jni/src/identity.rs, packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/IdentityNative.kt, packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt |
The APIs pass rotation for app-encryption derivation and provide a separate legacy method. JNI result handling frees FFI output and clears private-key copies on the described paths. |
Swift API and tests packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift, packages/swift-sdk/SwiftTests/SwiftDashSDKTests/ConnectKeyDerivationTests.swift |
The wallet exposes session-authentication, rotation-aware app-encryption, and legacy derivation methods. Tests check vectors, distinct keys, and invalid parameters. |
Priority:
Estimated code review effort: 4 (Complex) | ~45 minutes
Change: Feature
Sequence Diagram(s)
sequenceDiagram
participant SDK
participant FFI as dash_sdk_derive_connect_key_with_resolver
participant KeyPath as ConnectKey.derivation_path
participant Resolver as Mnemonic resolver
participant Master as Master key derivation
SDK->>FFI: Request key derivation
FFI->>KeyPath: Validate derivation path
FFI->>Resolver: Resolve seed
Resolver-->>FFI: Return seed
FFI->>Master: Derive key from path
Master-->>SDK: Return derived key
Suggested reviewers: quantumexplorer, lklimek, romchornyi
Merge Risk: 
Minimal · previous review 42f59
-
The rotation-aware and legacy derivation APIs appear ready to merge after normal checks. The Kotlin SDK has not been compiled locally, but no broken caller was found.
Pre-merge checks: 
5
-
Full details: Title check
-
Explanation
The title names the main change: application encryption keys are now derived with the DIP-13 rotation level. The changeset matches this. ConnectKey::AppEncryption gains a rotation field, the FFI, JNI, Kotlin, and Swift APIs accept rotation, and a separate legacy derivation path is added for pre-rotation keys. The title is specific and uses the correct domain term. It is 88 characters, which is longer than ideal for a short title, but this does not affect the assessment.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 14 functions across 7 files.
-
Finishing Touches
-
📝 Generate docstrings
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
|
Waiting for bot review — thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
|
✅ Final review complete — no blockers (commit c11751f) · triage: critical |
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
The rotation derivation logic and legacy path are otherwise coherent, and the Rust workspace and Kotlin checks passed. However, the PR changes the calling convention of an existing exported C function and JNI method without preserving the old symbol, which can cause undefined behavior for previously compiled consumers; the new tuple-dispatch rules also belong in platform-wallet rather than the FFI layer. The Swift build check remains in progress.
🔴 2 blocking | 🟡 2 suggestion(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 8: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 9: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 10: gpt-6-astra (agent: phase2-reviewer, role: ffi-engineer); reviewer 11: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 12: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — This is a large, cross-language change to cryptographic key derivation and legacy-key compatibility in platform-wallet, including Rust, FFI, JNI, Swift, and Kotlin surfaces. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort xhigh); agentphase2-reviewer - Model comparison: every Phase-2 reviewer also ran on
gpt-6-astra; the verifier weighed both sets without knowing which model wrote which
🤖 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-platform-wallet-ffi/src/derive_connect_key.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/derive_connect_key.rs:126-136: Preserve the existing connect-key ABI when adding rotation
The existing exported symbol `dash_sdk_derive_connect_key_with_resolver` keeps its name but inserts `rotation` before `out_key`. The previous C ABI accepted `out_key` as the eighth parameter; an older compiled caller will therefore pass its output pointer where the new function expects `rotation`, while the new function reads an unprovided ninth argument as `out_key`. The resulting invalid write can crash or corrupt memory before an error is returned. The same incompatibility exists for the unchanged JNI symbol when an older compiled Kotlin class is paired with the new native library. The migration instructions only help callers that are recompiled and do not protect already-built native consumers. Retain the old symbol and behavior, and add a versioned or differently named rotation-aware C/JNI entry point, or make an explicit ABI-major-version change for the shipped native library.
- [BLOCKING] packages/rs-platform-wallet-ffi/src/derive_connect_key.rs:143-165: Keep Connect-key dispatch and combination validation in platform-wallet
This FFI function interprets the raw `(sub_feature, purpose, rotation)` tuple, selects `ConnectKey::SessionAuthentication` versus `ConnectKey::AppEncryption`, and enforces which purpose and rotation combinations are valid. The new legacy entry point also directly selects `ConnectKey::AppEncryptionLegacy`. Those are wallet-domain decisions, not argument marshalling. Put the conversion and validation behind a typed constructor or request conversion owned by `rs-platform-wallet`, and have the FFI only convert C values and invoke that owner-level API. Otherwise native callers and future bindings can bypass or duplicate the canonical Connect-key invariants.
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/derive_connect_key.rs:228-233: Document the safety contracts of the private unsafe helpers
`read_leaf` dereferences a raw pointer based on a precondition enforced only by each caller's preceding `check_ptr!`, and `derive_into` similarly relies on its callers to validate `out_key`. These assumptions are currently satisfied, but the private helpers have no local `# Safety` contract, making a future caller's omission an immediate null-dereference or over-read risk. Add `# Safety` documentation describing the required pointer validity and readable lengths to both unsafe helpers.
In `packages/rs-platform-wallet/src/wallet/identity/network/identity_handle.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/identity_handle.rs:182-208: Use key-wallet's canonical rotation-aware path builders when available
The current code locally appends a hardened child to key-wallet's old eight-level application-encryption path and uses that old builder for the explicit legacy variant. This is behaviorally correct for the pinned dependency, but it duplicates path-shape and hardened-index ownership in platform-wallet. The linked rust-dashcore#1250 is still open and adds the canonical rotation-aware and without-rotation builders. Once that dependency is available, replace these local arms with those key-wallet APIs so the path definition cannot drift between crates.
|
Your move: thepastaclaw requested changes on this head; dismiss the review or push a fix; thepastaclaw left review threads unresolved; resolve them. |
…and move key selection into platform-wallet - Rename dash_sdk_derive_connect_key_with_resolver to dash_sdk_derive_connect_key_v2_with_resolver (and the JNI/Kotlin deriveConnectKeyWithResolver to deriveConnectKeyV2WithResolver) so a caller compiled against the pre-rotation signature fails to link instead of passing its out pointer where `rotation` now sits. - ConnectKey::from_parts and ConnectKey::legacy_app_encryption_from_parts in platform-wallet now map raw (sub_feature, purpose, rotation) levels to a key and refuse invalid combinations; the FFI only converts C values and calls them. - Document the safety requirements of the FFI helpers read_leaf and derive_into. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Your move: thepastaclaw left review threads unresolved; resolve them. |
|
Waiting for bot review — thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The reviewed head correctly centralizes Connect-key selection and validation in platform-wallet, preserves the legacy derivation path, and uses versioned C/JNI symbols to prevent ABI mismatches. The Rust, FFI, JNI, Swift, and Kotlin implementations agree on rotation bounds, path layout, error handling, and secret cleanup; no in-scope findings remain. Rust workspace tests, Kotlin tests, and the Swift schema check were still running, with no failed code checks reported.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 8: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 9: gpt-6-astra (agent: phase2-reviewer, role: ffi-engineer); reviewer 10: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 11: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 12: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); reviewer 13: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 14: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 15: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 16: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 17: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — This large cross-language change modifies cryptographic key derivation, rotation-path handling, and FFI/JNI/Swift/Kotlin boundaries in platform-wallet. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer - Model comparison: every Phase-2 reviewer also ran on
gpt-6-astra; the verifier weighed both sets without knowing which model wrote which
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.
No unresolved findings remain from the prior review on this head.
|
Bots are done — your move: post |
Problem
The platform wallet SDK derives DashPay Connect app encryption keys at the 8-level DIP-13 path
m/9'/coin'/5'/7'/0'/identityId'/contractId'/purpose'. Once mainnet keys are registered there, the path can't change, and it has no level for rotating a pair. DIP-13 (dashpay/dips#191) and the App Connect DIP (dashpay/dips#181) now end the path with a hardened 31-bitrotation'level, with0'as the first pair:Keys already registered on testnet and devnet use the old 8-level path, so wallets still need a way to derive them to recognise those keys.
Fix
platform-wallet:ConnectKey::AppEncryptiongains arotation: u32field, and new keys are derived with this variant. The newConnectKey::AppEncryptionLegacyvariant derives the old 8-level key, so the old path is only used when a caller asks for it.ConnectKey::derivation_pathnow returns aResultand rejects rotations of2^31or more withInvalidParameterinstead of wrapping them onto another key.platform-walletalso owns the mapping from raw values to a key.ConnectKey::from_parts(sub_feature, identity_id, leaf, purpose, rotation)accepts only the valid DIP-13 combinations, andConnectKey::legacy_app_encryption_from_parts(identity_id, contract_id, purpose)builds the old layout. The FFI only converts C values and calls these.dash_sdk_derive_connect_key_with_resolveris replaced bydash_sdk_derive_connect_key_v2_with_resolver, which takes a newrotation: u32argument afterpurpose. The old symbol is removed, not kept alongside, so a caller built against the old signature fails to link instead of passing its out pointer whererotationnow sits. Rotation is required for sub-feature7and must be0for the session key (sub-feature6). The new siblingdash_sdk_derive_legacy_app_encryption_key_with_resolver(network, wallet_id, resolver, identity_id, contract_id, purpose, out_key)derives the old layout. No argument value has a special meaning such as "legacy". Both functions check the key before asking the resolver for the seed.deriveConnectKeyWithResolveris renamed toderiveConnectKeyV2WithResolver(..., purpose, rotation), so a stale Kotlin class or native library fails withUnsatisfiedLinkErrorinstead of a mismatched signature. There is also a newderiveLegacyAppEncryptionKeyWithResolver.PlatformWalletManager.deriveConnectKeyis split into a session overload and an app-encryption overload, which requirespurposeandrotation.deriveLegacyAppEncryptionKey.Temporary local append
The pinned rust-dashcore rev only builds the 8-level path, so
platform-walletaddsChildNumber::Hardened { index: rotation }itself. dashpay/rust-dashcore#1250 (branchfeat/key-wallet-encryption-rotation) adds the level to key-wallet. When platform moves to a rust-dashcore rev that includes it, two calls replace the local code:DerivationPath::application_encryption_path(network, identity_id, contract_id, purpose, rotation)replaces the append.DerivationPath::application_encryption_path_without_rotationreplaces the legacy arm.The pinned vectors below ensure that switch changes no keys. This PR doesn't bump rust-dashcore.
Migration (Swift / Kotlin / C callers)
deriveConnectKey(subFeature: .appEncryption, ..., purpose:)now requires arotation: UInt32argument, with no default. Existing app-encryption call sites therefore fail to compile until they pass a rotation. Pass0for an identity's first pair for a contract.deriveLegacyAppEncryptionKey(identityId:contractId:purpose:network:storage:).deriveConnectKey(subFeature: .sessionAuthentication, identityId:leaf:network:)calls don't change.deriveConnectKey(walletId, APP_ENCRYPTION, identityId, leaf, purpose, rotation)andderiveLegacyAppEncryptionKey(walletId, identityId, contractId, purpose).dash_sdk_derive_connect_key_with_resolvertodash_sdk_derive_connect_key_v2_with_resolverand passrotation(0for the session key). Calldash_sdk_derive_legacy_app_encryption_key_with_resolverto get the old layout. The header is generated by cbindgen.IdentityNative.deriveConnectKeyWithResolvertoderiveConnectKeyV2WithResolver.How tested
Vectors from BIP-39
abandon ×11 about, empty passphrase, mainnet.0x35…is 32 bytes of 0x35,0x6b…is 32 bytes of 0x6b,ascis bytes 00..1f anddescis bytes ff..e0. All are pinned inconnect_keypairs_match_the_pinned_vectors:0x35…'/0x6b…'/1'/0'031a882c119e2b1f8e0349b444d877afa2599efeef49a26930fd25518cb6e9d57a0x35…'/0x6b…'/1'/1'03d0f6f0f48d968b9554d8e7fe467d7135bacf29d5c4994ca593bd10ebeed696f6asc'/desc'/1'/0'02dfed57ddaaea8c0758e8da44513520038eb8d6b0b92109953c14d6ab1087259basc'/desc'/2'/0'020bcde800f41ad1f924477a9502f111445224cc9ab70114ae4e5099ba02bdd034asc'/desc'/2'/1'021b97869c21933673076a938337c28848c8bf8094bbe6883e3a4acbf33c6390720x35…'/0x6b…'/1'03e989de1b62f137231cc06659c810c17100becd1f5e23d5710faa7602769fe434asc'/desc'/1'03c4e462229f176595ec17e28e9096d3cfbc42496fb8ed4d48f8f58355d6105aa9asc'/desc'/2'031fd0a3cfc01bebb89d43c2e12a6321c37ebdacb62c09495899e5b65a24fa5c99Other tests check:
ConnectKey::from_partsandlegacy_app_encryption_from_partsbuild each valid combination and reject the invalid ones;rotation'level;2^31or more are rejected;The existing Swift
ConnectKeyDerivationTestsare updated, with no tests removed, and cover the new overloads, the legacy derive and the rejection cases.cargo fmt --allcargo clippy -p platform-wallet -p platform-wallet-ffi -p rs-unified-sdk-jni --all-features --all-targets -- -D warnings: cleancargo test -p platform-wallet --lib identity_handle::tests: 9 passedcargo test -p platform-wallet-ffi --lib derive_connect_key: 9 passedcargo test -p rs-unified-sdk-jni --lib connect: 1 passedbuild_ios.sh --target mac --profile devfollowed byswift test --filter ConnectKeyDerivationTests: 4 passed🤖 Generated with Claude Code
PR Hygiene ·
c11751f/self-reviewedkotlin-sdk(packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/IdentityNative.kt,packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt) — HashEngineeringrs-platform-wallet-ffi(packages/rs-platform-wallet-ffi/src/derive_connect_key.rs) — HashEngineering or ZocoLini or llbartekll or romchornyirs-platform-wallet(packages/rs-platform-wallet/src/wallet/identity/network/identity_handle.rs) — HashEngineering or ZocoLini or llbartekll or romchornyipackages/rs-unified-sdk-jni/src/identity.rs) — QuantumExplorer or shumkovswift-sdk(packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift,packages/swift-sdk/SwiftTests/SwiftDashSDKTests/ConnectKeyDerivationTests.swift) — llbartekll or romchornyiWhen every merge requirement is met, the
PR Hygienecheck passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.