refactor(rng): hide rand ABI behind seed boundaries#243
Conversation
Summary by CodeRabbit
WalkthroughAPIs and tests across many crates were converted from mutable RNGs to a deterministic Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Factory
participant Seed
participant ChaChaRng as ChaCha20Rng
participant Generator
Caller->>Factory: get_or_init(domain,label,spec,variant, init: FnOnce(Seed)->T)
Factory->>Seed: derive Seed (deterministic from inputs)
Factory->>init: call init(Seed)
init->>ChaChaRng: ChaCha20Rng::from_seed(Seed.bytes()) (optional)
alt init uses ChaChaRng
ChaChaRng->>Generator: generate bytes / keys / tokens
else init uses Seed.fill_bytes
Seed->>Generator: fill_bytes(dest)
end
Generator-->>Factory: constructed artifact (T)
Factory-->>Caller: Arc<T>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on refactoring the random number generator (RNG) usage within the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request is a significant and well-executed refactoring to hide the rand ABI behind a Seed boundary, which is a great step towards easier RNG library upgrades in the future. The changes are applied consistently across the workspace, with public APIs now correctly using Seed instead of Rng traits. I've identified a minor case of code duplication in the X.509 certificate generation logic that could be centralized for better maintainability. Overall, this is a solid architectural improvement.
| fn next_serial_number(rng: &mut impl RngCore) -> rcgen::SerialNumber { | ||
| let mut bytes = [0u8; 16]; | ||
| rng.fill_bytes(&mut bytes); | ||
| bytes[0] &= 0x7F; | ||
| rcgen::SerialNumber::from_slice(&bytes) | ||
| } |
There was a problem hiding this comment.
This next_serial_number helper function appears to be a duplicate of logic in uselesskey-core-x509-derive and is also present in chain.rs.
To improve maintainability and reduce code duplication, you could centralize this logic. One option is to make deterministic_serial_number_with_rng in uselesskey-core-x509-derive public and re-export it for use here.
For this specific file (cert.rs), since only one serial number is generated within the get_or_init closure, you could simplify further by removing this helper and the local rng instance, and instead call uselesskey_core_x509::deterministic_serial_number(seed) directly.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 96f263dad7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| use rand_chacha::ChaCha20Rng; | ||
| #[cfg(feature = "uk-core-token-shape")] | ||
| use rand_core::SeedableRng; | ||
| use uselesskey_core_seed::Seed; |
There was a problem hiding this comment.
Gate Seed usage with a feature that enables its crate
Using uselesskey_core_seed::Seed here makes uk-core-token-shape no longer build by itself: this module is compiled under uk-core-token-shape, but in crates/uselesskey-bdd-steps/Cargo.toml that feature only enables dep:uselesskey-core-token-shape, while uselesskey-core-seed remains behind uk-core-seed. Building with --no-default-features --features uk-core-token-shape will therefore fail with an unresolved uselesskey_core_seed import. Please either add dep:uselesskey-core-seed to uk-core-token-shape or gate this file on both features.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
crates/uselesskey-core-factory/tests/factory_concurrency.rs (1)
3-4:⚠️ Potential issue | 🟠 MajorAdd crate-level unsafe prohibition in this integration test crate.
Line 3 defines the crate attributes for this test crate, but
#![forbid(unsafe_code)]is missing.🔧 Proposed patch
+#![forbid(unsafe_code)] #![cfg(feature = "std")]As per coding guidelines, "Forbid all unsafe code; all crates must use #![forbid(unsafe_code)]".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/uselesskey-core-factory/tests/factory_concurrency.rs` around lines 3 - 4, Add the crate-level attribute to forbid unsafe code in this integration test crate by inserting #![forbid(unsafe_code)] alongside the existing crate attributes in tests/factory_concurrency.rs (the top of the file where #![cfg(feature = "std")] is declared) so the crate enforces the coding guideline prohibiting unsafe usage.crates/uselesskey-core-x509/tests/x509_facade_tests.rs (1)
1-7:⚠️ Potential issue | 🟠 MajorAdd crate-level unsafe prohibition for this test crate.
This integration test file is a crate root but does not declare
#![forbid(unsafe_code)].🔧 Proposed patch
+#![forbid(unsafe_code)] use uselesskey_core_seed::Seed;As per coding guidelines, "Forbid all unsafe code; all crates must use #![forbid(unsafe_code)]".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/uselesskey-core-x509/tests/x509_facade_tests.rs` around lines 1 - 7, This test crate's root (the integration test module in x509_facade_tests.rs) is missing the crate-level attribute forbidding unsafe code; add the directive #![forbid(unsafe_code)] at the top of the file (before any uses or module-level doc comments) so the test crate enforces the project's "forbid unsafe code" rule.crates/uselesskey-core/tests/concurrency_stress.rs (1)
6-7:⚠️ Potential issue | 🟠 MajorMissing
#![forbid(unsafe_code)]in this integration test crate.Line 6 currently only gates on feature flags; add unsafe-forbid at crate level.
🔧 Proposed patch
+#![forbid(unsafe_code)] #![cfg(feature = "std")]As per coding guidelines, "Forbid all unsafe code; all crates must use #![forbid(unsafe_code)]".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/uselesskey-core/tests/concurrency_stress.rs` around lines 6 - 7, Add the crate-level attribute to forbid unsafe code by inserting #![forbid(unsafe_code)] at the top of the integration test source (before the existing #![cfg(feature = "std")]), ensuring the forbid attribute is applied at crate scope so the test crate disallows any unsafe code.crates/uselesskey-core-factory/src/lib.rs (1)
84-116:⚠️ Potential issue | 🟠 MajorDocument that
initcan run more than once per key.
init(seed)executes beforeinsert_if_absent_typed, so concurrent misses can still evaluate the initializer multiple times for the same artifact ID. The companion edge-case test already permits that behavior, but the public API docs do not. That is risky for callers that do anything non-idempotent insideinit.📌 Minimal contract fix
/// Return a cached value by `(domain, label, spec, variant)` or generate one. /// /// The initializer receives the derived seed for this artifact identity. /// Callers that need an RNG should instantiate it privately from that seed. + /// + /// Note: on concurrent cache misses, `init` may run more than once for the + /// same artifact identity before one value wins the insert race. `init` + /// must therefore be side-effect-free / idempotent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/uselesskey-core-factory/src/lib.rs` around lines 84 - 116, The doc comment for get_or_init must state that the provided init(seed) may be executed more than once for the same ArtifactId under concurrent misses: currently get_or_init calls seed_for(&id) and then init(seed) before calling insert_if_absent_typed, so concurrent callers can race and each run init; update the public API doc on get_or_init to explicitly document this behavior and require callers to make init idempotent or side-effect-free (or to perform their own synchronization) and reference the involved symbols (get_or_init, init, seed_for, insert_if_absent_typed, ArtifactId) so readers can locate the code path.crates/uselesskey-core-token-shape/tests/shape_tests.rs (1)
282-292:⚠️ Potential issue | 🟡 MinorUse
#[rstest]for parameterized test with multiple length values.This test loops over multiple length values
[0, 1, 10, 100, 256]to test different cases. Per coding guidelines, parameterized tests should use therstestcrate instead of a manual loop:#[rstest] #[case(0)] #[case(1)] #[case(10)] #[case(100)] #[case(256)] fn random_base62_only_alphanumeric(#[case] len: usize) { let rng = Seed::new([82u8; 32]); let s = random_base62(rng, len); assert_eq!(s.len(), len); assert!( s.chars().all(|c| c.is_ascii_alphanumeric()), "random_base62({len}) produced non-alphanumeric chars" ); }(Note:
SeedisCopyvia#[derive(Clone, Copy, ...)], so the current code compiles; however, usingrstestprovides clearer test case organization and better reporting per guidelines.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/uselesskey-core-token-shape/tests/shape_tests.rs` around lines 282 - 292, Replace the manual loop in the test function random_base62_only_alphanumeric with an rstest parameterized test: annotate the function with #[rstest] and add #[case(...)] for each length (0,1,10,100,256), change the signature to accept len: usize (#[case] len: usize), keep creating the Seed value (Seed is Copy) and call random_base62(rng, len), and retain the existing assertions; this moves per-length cases into rstest for clearer reporting and follows the test guideline.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Cargo.toml`:
- Around line 106-107: The tests in crates/uselesskey-rustcrypto use
rand_core::OsRng but rand_core is pulled with default-features = false, so
enable the required feature for tests by either adding a crate feature (e.g.,
add a "std" feature in the uselesskey-rustcrypto Cargo.toml that depends on
rand_core/std and rand_chacha/std, matching the pattern in
uselesskey-core-factory) and gate OsRng usage in tests with #[cfg(feature =
"std")], or simply modify the dev-dependency for rand_core in Cargo.toml to
include features = ["std"] (and similarly for rand_chacha if needed) so
rustcrypto_cross_verify.rs can return OsRng in tests.
In `@crates/uselesskey-core-base62/src/lib.rs`:
- Around line 24-26: Replace direct ChaCha20Rng construction in random_base62 by
using Seed::fill_bytes as the single RNG boundary: remove
ChaCha20Rng::from_seed(*seed.bytes()) from random_base62 and instead call
seed.fill_bytes(&mut buf) to obtain deterministic bytes, then adapt
random_base62_with_rng to consume that byte buffer (or add a companion function
that takes a &[u8]) so all rand-specific construction stays in the seed crate;
update references to random_base62_with_rng to use the byte-buffer based API and
remove any direct ChaCha20Rng usage in this crate.
In `@crates/uselesskey-core-base62/tests/edge_cases.rs`:
- Around line 1-5: Add the crate-level attribute to forbid unsafe code by
inserting #![forbid(unsafe_code)] at the top of the integration test file
(before any use or doc comments) so the test crate enforces the project policy;
ensure this is placed in the same file that references BASE62_ALPHABET,
random_base62, and Seed (tests/edge_cases.rs) and that the attribute precedes
the existing module-level doc comment and imports.
In `@crates/uselesskey-core-seed/src/lib.rs`:
- Around line 218-228: The test fill_bytes_is_seed_stable currently only checks
self-consistency; change it to assert the output against a hard-coded, pinned
expected byte array so changes to the derivation algorithm fail the test.
Specifically, in the test using Seed::new([7u8; 32]) and seed.fill_bytes(&mut a)
assert that a == <expected 16-byte literal> (and you can optionally also assert
a == b after filling b) so the test verifies the deterministic boundary output
of Seed::fill_bytes rather than only internal consistency.
- Around line 39-46: Update the fill_bytes documentation to explicitly state
that each call reconstructs the RNG from the stored seed and therefore produces
the same byte sequence on repeated calls (i.e., it does not advance a persistent
stream). Mention ChaCha20Rng::from_seed(self.0) and clarify that the RNG is
newly created on every invocation of fill_bytes, so callers should not expect
subsequent calls to yield continued/streamed output.
In `@crates/uselesskey-core-token/tests/token_integration.rs`:
- Around line 1-5: Add the crate-level attribute to forbid unsafe code by
inserting #![forbid(unsafe_code)] at the top of this integration test file
(before any use or doc comments); ensure the attribute is the very first
non-comment item so it applies to the test crate that contains the tests (the
file containing the tests that import uselesskey_core_seed::Seed and define the
token integration tests).
In `@crates/uselesskey-core-x509-derive/src/lib.rs`:
- Around line 86-88: The helper function deterministic_serial_number currently
constructs a ChaCha20Rng via ChaCha20Rng::from_seed(*seed.bytes()), leaking
rand_chacha dependency; instead call the public Seed::fill_bytes to obtain RNG
bytes and pass them to deterministic_serial_number_with_rng (or construct the
RNG using those bytes within the Seed boundary), e.g., use seed.fill_bytes(...)
to produce the seed material and then call
deterministic_serial_number_with_rng(&mut rng) so all ChaCha20Rng construction
is isolated to the seed crate and deterministic_serial_number no longer directly
references ChaCha20Rng::from_seed or seed.bytes().
In `@crates/uselesskey-core-x509-derive/tests/integration.rs`:
- Line 3: The integration test file is missing the crate-level prohibition of
unsafe code; add the crate attribute #![forbid(unsafe_code)] as the very first
line of the file (above the existing use uselesskey_core_seed::Seed; statement)
so the test crate enforces the repository policy forbidding unsafe code; ensure
the attribute appears before any items or imports in tests/integration.rs (the
file containing the use of Seed) to take effect.
In `@crates/uselesskey-core-x509-derive/tests/prop_x509_derive.rs`:
- Around line 1-2: Add the crate-level attribute to forbid unsafe code in this
test crate by inserting #![forbid(unsafe_code)] at the top of
tests/prop_x509_derive.rs (above the existing use statements such as use
proptest::prelude::* and use uselesskey_core_seed::Seed) so the test file itself
enforces the workspace rule that no unsafe code is allowed.
In `@crates/uselesskey-x509/src/cert.rs`:
- Around line 582-586: The next_serial_number function can return a zero serial
if the 16 random bytes are all zero; update next_serial_number (the rng: &mut
impl RngCore -> rcgen::SerialNumber logic) to guarantee a non-zero result before
calling rcgen::SerialNumber::from_slice: after filling bytes and masking
bytes[0] &= 0x7F, detect the all-zero case and mutate at least one byte (e.g.,
set bytes[15] = 1) or loop/regenerate until bytes contains any non-zero byte so
the produced serial is never zero.
---
Outside diff comments:
In `@crates/uselesskey-core-factory/src/lib.rs`:
- Around line 84-116: The doc comment for get_or_init must state that the
provided init(seed) may be executed more than once for the same ArtifactId under
concurrent misses: currently get_or_init calls seed_for(&id) and then init(seed)
before calling insert_if_absent_typed, so concurrent callers can race and each
run init; update the public API doc on get_or_init to explicitly document this
behavior and require callers to make init idempotent or side-effect-free (or to
perform their own synchronization) and reference the involved symbols
(get_or_init, init, seed_for, insert_if_absent_typed, ArtifactId) so readers can
locate the code path.
In `@crates/uselesskey-core-factory/tests/factory_concurrency.rs`:
- Around line 3-4: Add the crate-level attribute to forbid unsafe code in this
integration test crate by inserting #![forbid(unsafe_code)] alongside the
existing crate attributes in tests/factory_concurrency.rs (the top of the file
where #![cfg(feature = "std")] is declared) so the crate enforces the coding
guideline prohibiting unsafe usage.
In `@crates/uselesskey-core-token-shape/tests/shape_tests.rs`:
- Around line 282-292: Replace the manual loop in the test function
random_base62_only_alphanumeric with an rstest parameterized test: annotate the
function with #[rstest] and add #[case(...)] for each length (0,1,10,100,256),
change the signature to accept len: usize (#[case] len: usize), keep creating
the Seed value (Seed is Copy) and call random_base62(rng, len), and retain the
existing assertions; this moves per-length cases into rstest for clearer
reporting and follows the test guideline.
In `@crates/uselesskey-core-x509/tests/x509_facade_tests.rs`:
- Around line 1-7: This test crate's root (the integration test module in
x509_facade_tests.rs) is missing the crate-level attribute forbidding unsafe
code; add the directive #![forbid(unsafe_code)] at the top of the file (before
any uses or module-level doc comments) so the test crate enforces the project's
"forbid unsafe code" rule.
In `@crates/uselesskey-core/tests/concurrency_stress.rs`:
- Around line 6-7: Add the crate-level attribute to forbid unsafe code by
inserting #![forbid(unsafe_code)] at the top of the integration test source
(before the existing #![cfg(feature = "std")]), ensuring the forbid attribute is
applied at crate scope so the test crate disallows any unsafe code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0fe9afaa-81b6-46a0-91d0-972cfd1b4ea6
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (67)
Cargo.tomlcrates/uselesskey-bdd-steps/src/steps/core_token_shape_steps.rscrates/uselesskey-core-base62/Cargo.tomlcrates/uselesskey-core-base62/src/lib.rscrates/uselesskey-core-base62/tests/edge_cases.rscrates/uselesskey-core-base62/tests/integration.rscrates/uselesskey-core-base62/tests/mutant_killers.rscrates/uselesskey-core-base62/tests/prop_base62.rscrates/uselesskey-core-base62/tests/snapshots_base62.rscrates/uselesskey-core-factory/Cargo.tomlcrates/uselesskey-core-factory/src/lib.rscrates/uselesskey-core-factory/tests/edge_cases.rscrates/uselesskey-core-factory/tests/error_paths.rscrates/uselesskey-core-factory/tests/factory_concurrency.rscrates/uselesskey-core-factory/tests/factory_prop.rscrates/uselesskey-core-factory/tests/mutant_killers.rscrates/uselesskey-core-factory/tests/snapshots_factory.rscrates/uselesskey-core-seed/Cargo.tomlcrates/uselesskey-core-seed/src/lib.rscrates/uselesskey-core-token-shape/Cargo.tomlcrates/uselesskey-core-token-shape/src/lib.rscrates/uselesskey-core-token-shape/tests/comprehensive_shape.rscrates/uselesskey-core-token-shape/tests/edge_cases.rscrates/uselesskey-core-token-shape/tests/integration.rscrates/uselesskey-core-token-shape/tests/mutant_killers.rscrates/uselesskey-core-token-shape/tests/prop_token_shape.rscrates/uselesskey-core-token-shape/tests/shape_tests.rscrates/uselesskey-core-token-shape/tests/snapshots_token_shape.rscrates/uselesskey-core-token/Cargo.tomlcrates/uselesskey-core-token/tests/integration.rscrates/uselesskey-core-token/tests/snapshots_token_core.rscrates/uselesskey-core-token/tests/token_integration.rscrates/uselesskey-core-token/tests/token_tests.rscrates/uselesskey-core-x509-derive/Cargo.tomlcrates/uselesskey-core-x509-derive/src/lib.rscrates/uselesskey-core-x509-derive/tests/comprehensive.rscrates/uselesskey-core-x509-derive/tests/derivation_paths.rscrates/uselesskey-core-x509-derive/tests/edge_cases.rscrates/uselesskey-core-x509-derive/tests/integration.rscrates/uselesskey-core-x509-derive/tests/mutant_killers.rscrates/uselesskey-core-x509-derive/tests/prop_x509_derive.rscrates/uselesskey-core-x509-derive/tests/snapshots_derive.rscrates/uselesskey-core-x509-derive/tests/trait_impls.rscrates/uselesskey-core-x509/Cargo.tomlcrates/uselesskey-core-x509/tests/negative_policy_tests.rscrates/uselesskey-core-x509/tests/x509_facade_tests.rscrates/uselesskey-core/Cargo.tomlcrates/uselesskey-core/src/factory.rscrates/uselesskey-core/tests/concurrency_stress.rscrates/uselesskey-core/tests/core_prop.rscrates/uselesskey-core/tests/determinism_regression.rscrates/uselesskey-core/tests/edge_cases.rscrates/uselesskey-core/tests/snapshots_core.rscrates/uselesskey-ecdsa/Cargo.tomlcrates/uselesskey-ecdsa/src/keypair.rscrates/uselesskey-ed25519/Cargo.tomlcrates/uselesskey-ed25519/src/keypair.rscrates/uselesskey-hmac/Cargo.tomlcrates/uselesskey-hmac/src/secret.rscrates/uselesskey-pgp/Cargo.tomlcrates/uselesskey-pgp/src/keypair.rscrates/uselesskey-rsa/Cargo.tomlcrates/uselesskey-rsa/src/keypair.rscrates/uselesskey-token/src/token.rscrates/uselesskey-x509/Cargo.tomlcrates/uselesskey-x509/src/cert.rscrates/uselesskey-x509/src/chain.rs
💤 Files with no reviewable changes (3)
- crates/uselesskey-ed25519/Cargo.toml
- crates/uselesskey-core/Cargo.toml
- crates/uselesskey-core-factory/Cargo.toml
| rand_core = { version = "0.6.4", default-features = false } | ||
| rand_chacha = { version = "0.3.1", default-features = false } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Search for std/getrandom-dependent rand APIs:"
rg -n -C2 --type rust '\bOsRng\b|::from_entropy\s*\(' crates tests || true
echo
echo "List crates inheriting the workspace rand deps:"
rg -n -C1 --glob 'Cargo.toml' 'rand_(chacha|core)\.workspace\s*=\s*true' crates || trueRepository: EffortlessMetrics/uselesskey
Length of output: 5201
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check uselesskey-core-factory Cargo.toml ==="
cat -n crates/uselesskey-core-factory/Cargo.toml | head -40
echo
echo "=== Check uselesskey-rustcrypto Cargo.toml ==="
cat -n crates/uselesskey-rustcrypto/Cargo.toml | head -60Repository: EffortlessMetrics/uselesskey
Length of output: 3994
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check if uselesskey-rustcrypto has any std feature ==="
grep -A5 '\[features\]' crates/uselesskey-rustcrypto/Cargo.toml
echo
echo "=== Show the exact OsRng usage in test ==="
cat -n crates/uselesskey-rustcrypto/tests/rustcrypto_cross_verify.rs | sed -n '24,35p'Repository: EffortlessMetrics/uselesskey
Length of output: 866
uselesskey-rustcrypto test code uses OsRng without re-enabling required rand features.
With default-features = false at the workspace level, rand_core::OsRng is unavailable unless the std or getrandom feature is explicitly re-enabled. The test in crates/uselesskey-rustcrypto/tests/rustcrypto_cross_verify.rs returns rand_core::OsRng (line 28–29) but the crate has no feature that propagates rand_core/std to its tests.
Compare this to uselesskey-core-factory which correctly defines a std feature including rand_core/std (lines 29–30) and gates OsRng usage behind #[cfg(feature = "std")]. Apply the same pattern to uselesskey-rustcrypto: either add a test feature that includes rand_core/std, or add features = ["std"] to the rand_core dev-dependency entry.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Cargo.toml` around lines 106 - 107, The tests in crates/uselesskey-rustcrypto
use rand_core::OsRng but rand_core is pulled with default-features = false, so
enable the required feature for tests by either adding a crate feature (e.g.,
add a "std" feature in the uselesskey-rustcrypto Cargo.toml that depends on
rand_core/std and rand_chacha/std, matching the pattern in
uselesskey-core-factory) and gate OsRng usage in tests with #[cfg(feature =
"std")], or simply modify the dev-dependency for rand_core in Cargo.toml to
include features = ["std"] (and similarly for rand_chacha if needed) so
rustcrypto_cross_verify.rs can return OsRng in tests.
| pub fn random_base62(seed: Seed, len: usize) -> String { | ||
| let mut rng = ChaCha20Rng::from_seed(*seed.bytes()); | ||
| random_base62_with_rng(&mut rng, len) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Expected: Seed exposes a fill_bytes-style boundary, and this file still
# constructs ChaCha20Rng in the production path.
rg -n -C2 'struct Seed|impl Seed|fn fill_bytes|pub fn fill_bytes' crates
rg -n -C2 'ChaCha20Rng::from_seed|random_base62_with_rng' crates/uselesskey-core-base62/src/lib.rsRepository: EffortlessMetrics/uselesskey
Length of output: 4338
Consolidate RNG construction to the Seed boundary in uselesskey-core-base62.
The Seed::fill_bytes() method is already the public deterministic boundary and internally constructs ChaCha20Rng. However, random_base62 still rebuilds ChaCha20Rng::from_seed(*seed.bytes()) independently, duplicating RNG construction logic across crates. This means a future rand-line migration must modify both uselesskey-core-seed AND uselesskey-core-base62.
Refactor random_base62 and random_base62_with_rng to use Seed::fill_bytes directly as the only RNG boundary, keeping the rand shim exclusively in the seed crate.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/uselesskey-core-base62/src/lib.rs` around lines 24 - 26, Replace
direct ChaCha20Rng construction in random_base62 by using Seed::fill_bytes as
the single RNG boundary: remove ChaCha20Rng::from_seed(*seed.bytes()) from
random_base62 and instead call seed.fill_bytes(&mut buf) to obtain deterministic
bytes, then adapt random_base62_with_rng to consume that byte buffer (or add a
companion function that takes a &[u8]) so all rand-specific construction stays
in the seed crate; update references to random_base62_with_rng to use the
byte-buffer based API and remove any direct ChaCha20Rng usage in this crate.
| //! Edge-case and boundary tests for base62 generation. | ||
|
|
||
| use rand_chacha::ChaCha20Rng; | ||
| use rand_core::SeedableRng; | ||
| use uselesskey_core_base62::{BASE62_ALPHABET, random_base62}; | ||
| use uselesskey_core_seed::Seed; | ||
|
|
There was a problem hiding this comment.
Add #![forbid(unsafe_code)] to this integration test crate.
This test crate root is missing the required unsafe prohibition.
🔧 Proposed patch
+#![forbid(unsafe_code)]
use uselesskey_core_base62::{BASE62_ALPHABET, random_base62};
use uselesskey_core_seed::Seed;As per coding guidelines, "Forbid all unsafe code; all crates must use #![forbid(unsafe_code)]".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| //! Edge-case and boundary tests for base62 generation. | |
| use rand_chacha::ChaCha20Rng; | |
| use rand_core::SeedableRng; | |
| use uselesskey_core_base62::{BASE62_ALPHABET, random_base62}; | |
| use uselesskey_core_seed::Seed; | |
| #![forbid(unsafe_code)] | |
| //! Edge-case and boundary tests for base62 generation. | |
| use uselesskey_core_base62::{BASE62_ALPHABET, random_base62}; | |
| use uselesskey_core_seed::Seed; | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/uselesskey-core-base62/tests/edge_cases.rs` around lines 1 - 5, Add
the crate-level attribute to forbid unsafe code by inserting
#![forbid(unsafe_code)] at the top of the integration test file (before any use
or doc comments) so the test crate enforces the project policy; ensure this is
placed in the same file that references BASE62_ALPHABET, random_base62, and Seed
(tests/edge_cases.rs) and that the attribute precedes the existing module-level
doc comment and imports.
| /// Fill the destination buffer with deterministic bytes derived from this seed. | ||
| /// | ||
| /// This keeps RNG implementation details private while allowing callers to | ||
| /// derive stable byte sequences from seed material. | ||
| pub fn fill_bytes(&self, dest: &mut [u8]) { | ||
| let mut rng = ChaCha20Rng::from_seed(self.0); | ||
| rng.fill_bytes(dest); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What are the repeated-call semantics of rand_core::RngCore::fill_bytes on the same RNG instance?
💡 Result:
rand_core::RngCore::fill_bytes(&mut self, dest) is specified to fill the whole buffer with “new” random data on each call (and may panic if it cannot). (docs.rs)
For repeated calls on the same RNG instance, the only general semantic guarantee is that each call draws fresh output from the RNG as it advances; however, there is no required relationship between the streams produced by fill_bytes, next_u32, and next_u64, and implementations are allowed to discard some generated bits/bytes (e.g. to avoid buffering a partially-used word/block). Consequently, calling fill_bytes multiple times is not guaranteed to be equivalent to one larger fill_bytes call and splitting the result. (docs.rs)
A concrete example is BlockRng64: if a prior next_u32 left a half-consumed u64, then a subsequent fill_bytes call discards the remaining half; also, if the requested length isn’t a multiple of 8, it may discard trailing bytes. (docs.tockos.org)
Citations:
- 1: https://docs.rs/rand_core/latest/src/rand_core/lib.rs.html
- 2: https://docs.rs/rand_core/latest/src/rand_core/lib.rs.html
- 3: https://docs.tockos.org/src/rand_core/block.rs
🏁 Script executed:
sed -n '35,46p' crates/uselesskey-core-seed/src/lib.rsRepository: EffortlessMetrics/uselesskey
Length of output: 545
Clarify in fill_bytes documentation that each call regenerates from the seed rather than advancing a stream.
The method rebuilds ChaCha20Rng::from_seed(self.0) on each invocation, so repeated calls produce identical output. The current docstring says "deterministic" and "stable" but does not explicitly state this restart behavior. Add a sentence clarifying that each call independently derives bytes from the seed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/uselesskey-core-seed/src/lib.rs` around lines 39 - 46, Update the
fill_bytes documentation to explicitly state that each call reconstructs the RNG
from the stored seed and therefore produces the same byte sequence on repeated
calls (i.e., it does not advance a persistent stream). Mention
ChaCha20Rng::from_seed(self.0) and clarify that the RNG is newly created on
every invocation of fill_bytes, so callers should not expect subsequent calls to
yield continued/streamed output.
| #[test] | ||
| fn fill_bytes_is_seed_stable() { | ||
| let seed = Seed::new([7u8; 32]); | ||
| let mut a = [0u8; 16]; | ||
| let mut b = [0u8; 16]; | ||
|
|
||
| seed.fill_bytes(&mut a); | ||
| seed.fill_bytes(&mut b); | ||
|
|
||
| assert_eq!(a, b); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Pin one known output for Seed::fill_bytes.
fill_bytes_is_seed_stable only proves self-consistency. It still passes if the derivation algorithm changes, so it will not catch deterministic output drift at the boundary this PR is introducing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/uselesskey-core-seed/src/lib.rs` around lines 218 - 228, The test
fill_bytes_is_seed_stable currently only checks self-consistency; change it to
assert the output against a hard-coded, pinned expected byte array so changes to
the derivation algorithm fail the test. Specifically, in the test using
Seed::new([7u8; 32]) and seed.fill_bytes(&mut a) assert that a == <expected
16-byte literal> (and you can optionally also assert a == b after filling b) so
the test verifies the deterministic boundary output of Seed::fill_bytes rather
than only internal consistency.
| //! Integration tests for token generation facade — covers all token kinds, | ||
| //! determinism, and structural invariants. | ||
|
|
||
| use rand_chacha::ChaCha20Rng; | ||
| use rand_chacha::rand_core::SeedableRng; | ||
| use uselesskey_core_seed::Seed; | ||
|
|
There was a problem hiding this comment.
Missing #![forbid(unsafe_code)] at test crate root.
Please add the crate-level unsafe prohibition in this integration test file.
🔧 Proposed patch
+#![forbid(unsafe_code)]
use uselesskey_core_seed::Seed;As per coding guidelines, "Forbid all unsafe code; all crates must use #![forbid(unsafe_code)]".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| //! Integration tests for token generation facade — covers all token kinds, | |
| //! determinism, and structural invariants. | |
| use rand_chacha::ChaCha20Rng; | |
| use rand_chacha::rand_core::SeedableRng; | |
| use uselesskey_core_seed::Seed; | |
| //! Integration tests for token generation facade — covers all token kinds, | |
| //! determinism, and structural invariants. | |
| #![forbid(unsafe_code)] | |
| use uselesskey_core_seed::Seed; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/uselesskey-core-token/tests/token_integration.rs` around lines 1 - 5,
Add the crate-level attribute to forbid unsafe code by inserting
#![forbid(unsafe_code)] at the top of this integration test file (before any use
or doc comments); ensure the attribute is the very first non-comment item so it
applies to the test crate that contains the tests (the file containing the tests
that import uselesskey_core_seed::Seed and define the token integration tests).
| pub fn deterministic_serial_number(seed: Seed) -> SerialNumber { | ||
| let mut rng = ChaCha20Rng::from_seed(*seed.bytes()); | ||
| deterministic_serial_number_with_rng(&mut rng) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Expected: Seed exposes a fill_bytes-style boundary, and this file still
# constructs ChaCha20Rng in the production path.
rg -n -C2 'struct Seed|impl Seed|fn fill_bytes|pub fn fill_bytes' crates
rg -n -C2 'ChaCha20Rng::from_seed|deterministic_serial_number_with_rng' crates/uselesskey-core-x509-derive/src/lib.rsRepository: EffortlessMetrics/uselesskey
Length of output: 3413
Use Seed::fill_bytes to keep RNG construction isolated to the seed crate.
deterministic_serial_number reconstructs ChaCha20Rng::from_seed() directly, maintaining a direct dependency on rand_chacha in this helper crate. The Seed type already provides a public fill_bytes() method that encapsulates RNG construction, keeping the crypto implementation details isolated. Refactor to use seed.fill_bytes() instead to align with the PR's boundary separation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/uselesskey-core-x509-derive/src/lib.rs` around lines 86 - 88, The
helper function deterministic_serial_number currently constructs a ChaCha20Rng
via ChaCha20Rng::from_seed(*seed.bytes()), leaking rand_chacha dependency;
instead call the public Seed::fill_bytes to obtain RNG bytes and pass them to
deterministic_serial_number_with_rng (or construct the RNG using those bytes
within the Seed boundary), e.g., use seed.fill_bytes(...) to produce the seed
material and then call deterministic_serial_number_with_rng(&mut rng) so all
ChaCha20Rng construction is isolated to the seed crate and
deterministic_serial_number no longer directly references ChaCha20Rng::from_seed
or seed.bytes().
|
|
||
| use rand_chacha::ChaCha20Rng; | ||
| use rand_core::SeedableRng; | ||
| use uselesskey_core_seed::Seed; |
There was a problem hiding this comment.
Add crate-level unsafe prohibition in this test crate.
Please add #![forbid(unsafe_code)] at the top of this integration test file to match repository policy.
Suggested patch
+#![forbid(unsafe_code)]
//! Integration tests for `uselesskey-core-x509-derive`.As per coding guidelines, "Forbid all unsafe code; all crates must use #![forbid(unsafe_code)]".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| use uselesskey_core_seed::Seed; | |
| #![forbid(unsafe_code)] | |
| //! Integration tests for `uselesskey-core-x509-derive`. | |
| use uselesskey_core_seed::Seed; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/uselesskey-core-x509-derive/tests/integration.rs` at line 3, The
integration test file is missing the crate-level prohibition of unsafe code; add
the crate attribute #![forbid(unsafe_code)] as the very first line of the file
(above the existing use uselesskey_core_seed::Seed; statement) so the test crate
enforces the repository policy forbidding unsafe code; ensure the attribute
appears before any items or imports in tests/integration.rs (the file containing
the use of Seed) to take effect.
| use proptest::prelude::*; | ||
| use rand_chacha::ChaCha20Rng; | ||
| use rand_core::SeedableRng; | ||
| use uselesskey_core_seed::Seed; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add #![forbid(unsafe_code)] at this crate root.
tests/*.rs compiles as its own crate, so the workspace unsafe ban needs to live in this file too.
Suggested patch
+#![forbid(unsafe_code)]
+
use proptest::prelude::*;
use uselesskey_core_seed::Seed;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| use proptest::prelude::*; | |
| use rand_chacha::ChaCha20Rng; | |
| use rand_core::SeedableRng; | |
| use uselesskey_core_seed::Seed; | |
| #![forbid(unsafe_code)] | |
| use proptest::prelude::*; | |
| use uselesskey_core_seed::Seed; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/uselesskey-core-x509-derive/tests/prop_x509_derive.rs` around lines 1
- 2, Add the crate-level attribute to forbid unsafe code in this test crate by
inserting #![forbid(unsafe_code)] at the top of tests/prop_x509_derive.rs (above
the existing use statements such as use proptest::prelude::* and use
uselesskey_core_seed::Seed) so the test file itself enforces the workspace rule
that no unsafe code is allowed.
| fn next_serial_number(rng: &mut impl RngCore) -> rcgen::SerialNumber { | ||
| let mut bytes = [0u8; 16]; | ||
| rng.fill_bytes(&mut bytes); | ||
| bytes[0] &= 0x7F; | ||
| rcgen::SerialNumber::from_slice(&bytes) |
There was a problem hiding this comment.
Ensure the generated serial can never be zero.
Masking the sign bit only makes the serial non-negative. If these 16 bytes happen to be all zero, this emits serial 0, which violates the X.509 positive-serial requirement.
Suggested patch
fn next_serial_number(rng: &mut impl RngCore) -> rcgen::SerialNumber {
let mut bytes = [0u8; 16];
rng.fill_bytes(&mut bytes);
bytes[0] &= 0x7F;
+ if bytes.iter().all(|&b| b == 0) {
+ bytes[bytes.len() - 1] = 1;
+ }
rcgen::SerialNumber::from_slice(&bytes)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/uselesskey-x509/src/cert.rs` around lines 582 - 586, The
next_serial_number function can return a zero serial if the 16 random bytes are
all zero; update next_serial_number (the rng: &mut impl RngCore ->
rcgen::SerialNumber logic) to guarantee a non-zero result before calling
rcgen::SerialNumber::from_slice: after filling bytes and masking bytes[0] &=
0x7F, detect the all-zero case and mutate at least one byte (e.g., set bytes[15]
= 1) or loop/regenerate until bytes contains any non-zero byte so the produced
serial is never zero.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
crates/uselesskey-core-seed/src/lib.rs (2)
39-43:⚠️ Potential issue | 🟡 MinorClarify repeated-call behavior in
fill_bytesdocs.Line 39 docs are still ambiguous about repeated calls. Since Line 44 recreates RNG state each call, document that calls do not continue a stream.
Suggested doc update
- /// This keeps RNG implementation details private while allowing callers to - /// derive stable byte sequences from seed material. + /// This keeps RNG implementation details private while allowing callers to + /// derive stable byte sequences from seed material. + /// + /// Each call reinitializes from the same seed state, so repeated calls with + /// the same output length return the same bytes (i.e., no stream continuation).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/uselesskey-core-seed/src/lib.rs` around lines 39 - 43, Update the docstring for the method fill_bytes to explicitly state that each call recreates RNG state from the seed and therefore does not continue a previous byte stream; mention that repeated calls produce the same deterministic bytes for the same seed and dest length rather than advancing an internal stream. Reference the fill_bytes method and the fact that RNG state is reinitialized on each call so callers know this behavior.
218-238:⚠️ Potential issue | 🟠 MajorPin a fixed output vector for the deterministic boundary.
Line 219 and Line 231 tests only check self-consistency / mutation. They won’t catch boundary drift if derivation behavior changes.
Suggested strengthening
#[test] fn fill_bytes_is_seed_stable() { let seed = Seed::new([7u8; 32]); let mut a = [0u8; 16]; let mut b = [0u8; 16]; seed.fill_bytes(&mut a); seed.fill_bytes(&mut b); - assert_eq!(a, b); + const EXPECTED: [u8; 16] = [ + /* pin current 16-byte output for Seed::new([7u8; 32]) */ + ]; + assert_eq!(a, EXPECTED); + assert_eq!(b, EXPECTED); } #[test] fn fill_bytes_overwrites_destination_buffer() { let seed = Seed::new([7u8; 32]); let mut out = [0xAA; 16]; seed.fill_bytes(&mut out); - assert_ne!(out, [0xAA; 16]); + const EXPECTED: [u8; 16] = [ + /* same pinned vector as above */ + ]; + assert_eq!(out, EXPECTED); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/uselesskey-core-seed/src/lib.rs` around lines 218 - 238, The tests seed stability currently only compare outputs to themselves and initial buffers, which won't catch deterministic boundary drift; update the tests for Seed::new and its method fill_bytes to assert against a pinned, hard-coded expected output vector (e.g., compute the canonical 16-byte output once from Seed::new([7u8; 32]) and replace the self-comparison in fill_bytes_is_seed_stable with assert_eq!(a, EXPECTED) and in fill_bytes_overwrites_destination_buffer assert_ne!(out, [0xAA;16]) plus assert_eq!(out, EXPECTED) (or a slice comparison) so any change in derivation will fail; keep the test names (fill_bytes_is_seed_stable, fill_bytes_overwrites_destination_buffer) and reference Seed::new and fill_bytes when adding the EXPECTED constant.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/uselesskey-core-seed/src/lib.rs`:
- Around line 39-43: Update the docstring for the method fill_bytes to
explicitly state that each call recreates RNG state from the seed and therefore
does not continue a previous byte stream; mention that repeated calls produce
the same deterministic bytes for the same seed and dest length rather than
advancing an internal stream. Reference the fill_bytes method and the fact that
RNG state is reinitialized on each call so callers know this behavior.
- Around line 218-238: The tests seed stability currently only compare outputs
to themselves and initial buffers, which won't catch deterministic boundary
drift; update the tests for Seed::new and its method fill_bytes to assert
against a pinned, hard-coded expected output vector (e.g., compute the canonical
16-byte output once from Seed::new([7u8; 32]) and replace the self-comparison in
fill_bytes_is_seed_stable with assert_eq!(a, EXPECTED) and in
fill_bytes_overwrites_destination_buffer assert_ne!(out, [0xAA;16]) plus
assert_eq!(out, EXPECTED) (or a slice comparison) so any change in derivation
will fail; keep the test names (fill_bytes_is_seed_stable,
fill_bytes_overwrites_destination_buffer) and reference Seed::new and fill_bytes
when adding the EXPECTED constant.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8fff13aa-8bd6-43e0-81fe-b2e3a7218a85
📒 Files selected for processing (1)
crates/uselesskey-core-seed/src/lib.rs
Summary
This is the boundary refactor for the RNG migration lane. It does not try to converge the workspace on a single rand line yet.
What changed
Factory::get_or_initto pass a derivedSeedinstead of a publicChaCha20RngArtifactRng-shaped usage fromuselesskey-core/uselesskey-core-factoryrsa,ecdsa,pgp,hmac,x509)Seedinstead of rand trait types:uselesskey-core-base62uselesskey-core-token-shapeuselesskey-core-x509-deriveSeed::fill_bytesas the byte-oriented deterministic boundarySeed::next_u32/Seed::next_u64helpersWhy
The goal here is to stop leaking a specific
rand/rand_coreABI through public APIs before attempting a staged RNG upgrade.That gives us a cleaner migration shape:
Topology note
This PR does not claim full RNG convergence yet.
Current graph shape after this refactor:
rand_core 0.6/rand_chacha 0.3remain in the workspace for the stable RSA / Ed25519 path and the seed boundaryrand_core 0.9remains viaproptestdev-dependenciesThe important change is that no published uselesskey API surface now exposes rand-specific traits or concrete RNG types.
Verification
cargo test -p uselesskey-core-seed -p uselesskey-core-factory -p uselesskey-core -p uselesskey-ed25519cargo xtask gaterg -n "pub .*Rng|pub .*RngCore|pub .*CryptoRng|pub .*TryRng|pub use .*ArtifactRng|\bArtifactRng\b" cratescargo tree -d | rg "rand(_core|_chacha)? v"Follow-up
A later pass can decide whether to: