CLAUDE.md (2)

110-115: ⚠️ Potential issue | 🟡 Minor

PgpFactoryExt is absent from the Extension Pattern section.

TokenFactoryExt was correctly added at Line 114, but PgpFactoryExt — the parallel trait introduced by uselesskey-pgp in this same PR — is not listed.

📝 Proposed addition

 - `TokenFactoryExt` → `fx.token(label, spec)`
 - `X509FactoryExt` → `fx.x509(label, spec)`
+- `PgpFactoryExt` → `fx.pgp(label, spec)`

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@CLAUDE.md` around lines 110 - 115, The Extension Pattern list in CLAUDE.md is
missing the PgpFactoryExt entry; add a line mapping the new trait to the factory
helper (e.g., `PgpFactoryExt` → `fx.pgp(label, spec)`) so the documentation
shows the parallel API introduced by the uselesskey-pgp change; update the list
near the existing `TokenFactoryExt` entry and use the exact symbol names
`PgpFactoryExt` and `fx.pgp` to keep consistency.

---

80-96: ⚠️ Potential issue | 🟡 Minor

uselesskey-pgp and uselesskey-tonic are missing from the workspace structure listing.

Both crates are introduced in this PR (confirmed by the PR objectives and AI summary), but neither appears in the Architecture section. This leaves the guidance file incomplete for anyone — or any AI agent — working in this repo.

📝 Proposed additions

- **`crates/uselesskey-token`** - Token fixtures (API key, bearer, OAuth/JWT-shape) via `TokenFactoryExt` trait
  **`crates/uselesskey-x509`** - X.509 certificate fixtures via `X509FactoryExt` trait
+ **`crates/uselesskey-pgp`** - OpenPGP key fixtures via `PgpFactoryExt` trait
  **`crates/uselesskey-jsonwebtoken`** - Adapter: `jsonwebtoken` integration
  **`crates/uselesskey-rustls`** - Adapter: `rustls` / `rustls-pki-types` integration
  **`crates/uselesskey-ring`** - Adapter: `ring` integration
  **`crates/uselesskey-rustcrypto`** - Adapter: RustCrypto integration
  **`crates/uselesskey-aws-lc-rs`** - Adapter: `aws-lc-rs` integration
+ **`crates/uselesskey-tonic`** - Adapter: `tonic` TLS transport integration
  **`crates/uselesskey-bdd`** - Cucumber BDD tests

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@CLAUDE.md` around lines 80 - 96, The Architecture section of CLAUDE.md
omitted the two new crates introduced by the PR: `crates/uselesskey-pgp` and
`crates/uselesskey-tonic`; update the listing block that enumerates workspace
crates to include lines for **`crates/uselesskey-pgp`** (PGP-related
fixtures/adapters) and **`crates/uselesskey-tonic`** (gRPC/tonic integration),
matching the style of the other entries so the README accurately reflects the
workspace; edit the Architecture section where the other `crates/uselesskey-*`
entries are listed to add those two entries.

crates/uselesskey-rustcrypto/src/lib.rs (1)

196-221: 🧹 Nitpick | 🔵 Trivial

Migrate remaining Factory::random() calls in ecdsa_tests, ed25519_tests, and hmac_tests to super::fx().

fx() was added to the parent tests module and is already reachable via super::fx() from all three sub-modules, but lines 265, 281, 296, 304, 320, 342, 357, and 371 still call Factory::random(). This leaves the majority of the tests non-deterministic and inconsistent with the test_rsa_sign_verify migration.

♻️ Proposed migration for `ecdsa_tests`, `ed25519_tests`, and `hmac_tests`

 mod ecdsa_tests {
     use crate::RustCryptoEcdsaExt;
-    use uselesskey_core::Factory;
     use uselesskey_ecdsa::{EcdsaFactoryExt, EcdsaSpec};

     #[test]
     fn test_p256_sign_verify() {
         use p256::ecdsa::signature::{Signer, Verifier};
-        let fx = Factory::random();
+        let fx = super::super::fx();
         ...
     }

     #[test]
     fn test_p384_sign_verify() {
         use p384::ecdsa::signature::{Signer, Verifier};
-        let fx = Factory::random();
+        let fx = super::super::fx();
         ...
     }

     #[test]
     #[should_panic(expected = "expected P-384")]
     fn test_p384_on_p256_key_panics() {
-        let fx = Factory::random();
+        let fx = super::super::fx();
         ...
     }

     #[test]
     #[should_panic(expected = "expected P-256")]
     fn test_p256_on_p384_key_panics() {
-        let fx = Factory::random();
+        let fx = super::super::fx();
         ...
     }
 }

 mod ed25519_tests {
     use crate::RustCryptoEd25519Ext;
     use ed25519_dalek::{Signer, Verifier};
-    use uselesskey_core::Factory;
     use uselesskey_ed25519::{Ed25519FactoryExt, Ed25519Spec};

     #[test]
     fn test_ed25519_sign_verify() {
-        let fx = Factory::random();
+        let fx = super::super::fx();
         ...
     }
 }

 mod hmac_tests {
     use crate::RustCryptoHmacExt;
     use hmac::Mac;
-    use uselesskey_core::Factory;
     use uselesskey_hmac::{HmacFactoryExt, HmacSpec};

     #[test]
     fn test_hmac_sha256() {
-        let fx = Factory::random();
+        let fx = super::super::fx();
         ...
     }

     #[test]
     fn test_hmac_sha384() {
-        let fx = Factory::random();
+        let fx = super::super::fx();
         ...
     }

     #[test]
     fn test_hmac_sha512() {
-        let fx = Factory::random();
+        let fx = super::super::fx();
         ...
     }
 }

Note: the path is super::super::fx() because these are nested one level deeper (tests::ecdsa_tests, etc.) than where fx() is defined (tests). Alternatively, move all sub-module tests up one level or re-export fx inside each sub-module.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/uselesskey-rustcrypto/src/lib.rs` around lines 196 - 221, Replace
non-deterministic Factory::random() calls in the nested test modules
ecdsa_tests, ed25519_tests, and hmac_tests with the deterministic fx() helper
added to the parent tests module; since these modules are nested one level
deeper, call super::super::fx() wherever Factory::random() is used (e.g., inside
the test functions in ecdsa_tests, ed25519_tests, and hmac_tests) so all tests
use the fixed seed via fx() and become deterministic.

xtask/src/plan.rs (1)

236-254: 🧹 Nitpick | 🔵 Trivial

Optional: assert "uselesskey-tonic" in core_change_expands_dependents.

A core change transitively reaches uselesskey-tonic via core → x509 → tonic. The existing style of this test is not exhaustive on transitive dependents, but now that tonic is a first-class leaf in the graph, asserting it here would close the loop alongside the new token/pgp assertions.

💡 Suggested addition

     assert!(impacted.contains("uselesskey-x509"));
     assert!(impacted.contains("uselesskey"));
     assert!(impacted.contains("uselesskey-bdd"));
+    assert!(impacted.contains("uselesskey-tonic"));
     assert!(plan.run_bdd);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@xtask/src/plan.rs` around lines 236 - 254, The test
core_change_expands_dependents in the xtask module doesn't assert that
transitive dependent "uselesskey-tonic" is included; update the test (function
core_change_expands_dependents) to add an assertion that plan.impacted_crates
contains "uselesskey-tonic" so the transitive path core → x509 → tonic is
covered; locate the test using build_plan(...) and impacted_crates and add an
assert!(impacted.contains("uselesskey-tonic")) alongside the other impacted
assertions.

crates/uselesskey-rustls/src/config.rs (2)

376-414: 🧹 Nitpick | 🔵 Trivial

Consider extracting the handshake loop into a shared helper.

The handshake-driving loop (client↔server byte shuffling, progress tracking, iteration cap) is duplicated verbatim across tls_handshake_roundtrip, mtls_with_provider_roundtrip, and mtls_roundtrip. A small helper would eliminate ~60 lines of repetition:

♻️ Suggested helper

fn drive_handshake(
    client: &mut rustls::ClientConnection,
    server: &mut rustls::ServerConnection,
) {
    let mut buf = Vec::new();
    for iteration in 0..MAX_HANDSHAKE_ITERATIONS {
        let mut progress = false;

        buf.clear();
        if client.wants_write() {
            client.write_tls(&mut buf).unwrap();
            if !buf.is_empty() {
                server.read_tls(&mut &buf[..]).unwrap();
                server.process_new_packets().unwrap();
                progress = true;
            }
        }

        buf.clear();
        if server.wants_write() {
            server.write_tls(&mut buf).unwrap();
            if !buf.is_empty() {
                client.read_tls(&mut &buf[..]).unwrap();
                client.process_new_packets().unwrap();
                progress = true;
            }
        }

        if !progress {
            break;
        }

        assert!(
            iteration < MAX_HANDSHAKE_ITERATIONS - 1,
            "handshake did not complete within {} iterations",
            MAX_HANDSHAKE_ITERATIONS
        );
    }

    assert!(!client.is_handshaking());
    assert!(!server.is_handshaking());
}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/uselesskey-rustls/src/config.rs` around lines 376 - 414, The handshake
loop is duplicated in tls_handshake_roundtrip, mtls_with_provider_roundtrip, and
mtls_roundtrip; extract it into a helper function (e.g., drive_handshake) that
accepts &mut rustls::ClientConnection and &mut rustls::ServerConnection,
performs the client↔server write/read/progress/iteration logic, and preserves
the existing safety/assert checks (including the MAX_HANDSHAKE_ITERATIONS
assertion and final !is_handshaking() asserts); then replace the duplicated loop
in each of tls_handshake_roundtrip, mtls_with_provider_roundtrip, and
mtls_roundtrip with a call to this helper to eliminate repetition.

---

205-227: ⚠️ Potential issue | 🟠 Major

Use WebPkiClientVerifier::builder_with_provider() in server_config_mtls_rustls_with_provider.

The provider parameter passed to server_config_mtls_rustls_with_provider is threaded through ServerConfig::builder_with_provider(provider) correctly, but WebPkiClientVerifier::builder(root_store.into()).build() at line 214 does not use the explicit provider.

rustls 0.23 exposes WebPkiClientVerifier::builder_with_provider(roots, provider: Arc<CryptoProvider>), which should be used here to ensure the client verifier respects the explicitly passed provider:

Current code (line 214)

let client_verifier = rustls::server::WebPkiClientVerifier::builder(root_store.into())
    .build()
    .expect("valid client verifier");

Change to pass the provider parameter:

let client_verifier = rustls::server::WebPkiClientVerifier::builder_with_provider(root_store.into(), provider.clone())
    .build()
    .expect("valid client verifier");

Without this fix, the client verifier silently falls back to the process-default CryptoProvider, defeating the purpose of the explicit provider parameter.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/uselesskey-rustls/src/config.rs` around lines 205 - 227, The client
verifier in server_config_mtls_rustls_with_provider is built with
WebPkiClientVerifier::builder(...) and therefore ignores the explicit provider
parameter; change the construction to call
WebPkiClientVerifier::builder_with_provider(root_store.into(), provider.clone())
so the verifier uses the same Arc<CryptoProvider> passed into
server_config_mtls_rustls_with_provider, then continue to .build().expect("valid
client verifier") as before.

crates/uselesskey-bdd/tests/bdd.rs (2)

2311-2335: 🛠️ Refactor suggestion | 🟠 Major

build_jwks_both and build_jwks_containing_both have identical bodies — DRY violation.

Unlike build_jwks_all_three which correctly delegates to build_jwks_all, these two share no code despite having identical logic. build_jwks_containing_both should simply call build_jwks_both.

♻️ Proposed fix

 #[when(regex = r#"^I build a JWKS containing both keys with kids "([^"]+)" and "([^"]+)"$"#)]
 fn build_jwks_containing_both(world: &mut UselessWorld, kid1: String, kid2: String) {
-    assert_eq!(world.rsa_keys.len(), 2, "need 2 RSA keys");
-    let mut jwk1 = world.rsa_keys[0].public_jwk();
-    let mut jwk2 = world.rsa_keys[1].public_jwk();
-    set_public_kid(&mut jwk1, &kid1);
-    set_public_kid(&mut jwk2, &kid2);
-    let mut builder = JwksBuilder::new();
-    builder.push_public(jwk1);
-    builder.push_public(jwk2);
-    world.jwks_output_1 = Some(builder.build().to_value());
+    build_jwks_both(world, kid1, kid2);
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/uselesskey-bdd/tests/bdd.rs` around lines 2311 - 2335, Replace the
duplicated body of build_jwks_containing_both by delegating to build_jwks_both:
change build_jwks_containing_both to simply call build_jwks_both(world, kid1,
kid2) (matching the same signature world: &mut UselessWorld, kid1: String, kid2:
String) and return; remove the duplicate JWK construction code so only
build_jwks_both contains the implementation.

---

30-30: ⚠️ Potential issue | 🟡 Minor

#[derive(Debug)] on UselessWorld exposes raw private-key material.

Fields pkcs8_der_original, hmac_secret_1, hmac_secret_2, ed25519_pkcs8_der_original, ecdsa_pkcs8_der_original (and their _2 siblings) are Option<Vec<u8>> containing raw PKCS#8 DER or HMAC secret bytes. When any cucumber step panics or a test runner prints world state, the full byte arrays would appear in logs.

Implement a manual Debug for these fields, or introduce a Redacted newtype wrapper:

🛡️ Proposed fix – redact sensitive fields from Debug

-#[derive(Default, Debug, World)]
+#[derive(Default, World)]
 struct UselessWorld {
     ...
 }
+
+impl std::fmt::Debug for UselessWorld {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        f.debug_struct("UselessWorld")
+            .field("label", &self.label)
+            .field("pkcs8_der_original", &"<redacted>")
+            .field("hmac_secret_1", &"<redacted>")
+            .field("hmac_secret_2", &"<redacted>")
+            .field("ed25519_pkcs8_der_original", &"<redacted>")
+            .field("ecdsa_pkcs8_der_original", &"<redacted>")
+            // add non-sensitive fields as needed
+            .finish_non_exhaustive()
+    }
+}

As per coding guidelines, "Never print key material in Debug output to prevent key leakage" applies to **/*.rs.

Also applies to: 46-48, 66-67, 74-75, 88-89

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/uselesskey-bdd/tests/bdd.rs` at line 30, The current derive(Debug) on
UselessWorld leaks raw key material (fields like pkcs8_der_original,
hmac_secret_1/2, ed25519_pkcs8_der_original, ecdsa_pkcs8_der_original and their
_2 variants) when the world is printed; remove the automatic #[derive(Debug)]
from UselessWorld and either (a) implement a manual impl Debug for UselessWorld
that redacts those sensitive Option<Vec<u8>> fields (showing e.g. "<redacted>"
or lengths only) or (b) introduce a Redacted newtype wrapper used on those
fields with its own Debug that does not reveal contents, then update the
UselessWorld definition to use Redacted for the sensitive fields and keep
non-sensitive fields printable; ensure the impl Debug or Redacted covers all
listed sensitive names so test failures or panics won’t leak key bytes.

xtask/src/main.rs (1)

158-168: ⚠️ Potential issue | 🟡 Minor

--release for BDD suppresses debug_assert! checks; confirm this is intentional.

Release mode disables debug_assert! and overflow checks. If BDD tests or their fixtures rely on these for correctness verification, failures may be silently swallowed. Additionally, release builds substantially increase CI compilation time. If the motivation is purely performance (e.g., crypto operations are too slow in debug mode), consider whether a profile with opt-level=1 and debug-assertions=true would satisfy both needs.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@xtask/src/main.rs` around lines 158 - 168, The bdd() function currently
passes "--release" to cargo (see Command::new("cargo") args in bdd()), which
disables debug_assert! and overflow checks and increases CI compile time; either
remove the "--release" flag so tests run with debug assertions enabled, or
replace it by using a custom cargo profile (e.g., add "--profile" and create a
profile in Cargo.toml with opt-level=1 and debug-assertions=true) so BDD tests
keep debug checks while improving performance.

crates/uselesskey-core/tests/negative_prop.rs (1)

54-73: 🧹 Nitpick | 🔵 Trivial

fraction.max(1) is redundant; consider using 2usize..10 to avoid wasting proptest budget.

Since fraction in 1usize..10 already guarantees fraction >= 1, the .max(1) call is dead code. More importantly, when fraction == 1, target_bytes == pem.len() causes prop_assume!(target_bytes < pem.len()) to always discard the case, silently burning ~11 % of the proptest budget.

✂️ Proposed fix

-        fraction in 1usize..10
+        fraction in 2usize..10
     ) {
-        let target_bytes = pem.len() / fraction.max(1);
+        let target_bytes = pem.len() / fraction;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/uselesskey-core/tests/negative_prop.rs` around lines 54 - 73, The test
corrupt_pem_truncate_produces_correct_length wastes proptest budget by allowing
fraction == 1 and then masking it with .max(1); change the fraction generator
from 1usize..10 to 2usize..10 and remove the unnecessary .max(1) call so
target_bytes = pem.len() / fraction; keep the prop_assume!(target_bytes <
pem.len()) and prop_assume!(target_bytes > 0) and the rest of the assertion
using CorruptPem::Truncate unchanged.

crates/uselesskey-x509/src/chain_spec.rs (1)

165-226: 🧹 Nitpick | 🔵 Trivial

Consider adding a deduplication test for ChainSpec::stable_bytes, parallel to the one added in spec.rs.

spec.rs adds test_stable_bytes_deduplicates_sans; chain_spec.rs only has test_stable_bytes_san_order_independent. A sibling dedup test would close the coverage gap for the same change in this file.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/uselesskey-x509/src/chain_spec.rs` around lines 165 - 226, Add a new
unit test in the tests module that mirrors spec.rs: create two ChainSpec
instances (using ChainSpec::new and with_sans) where one has duplicate SAN
entries (e.g., ["a.example.com", "a.example.com", "b.example.com"]) and the
other has the duplicates removed (e.g., ["a.example.com", "b.example.com"]),
then assert that their stable_bytes() values are equal; name the test
test_stable_bytes_deduplicates_sans so it parallels the existing
test_stable_bytes_san_order_independent and validates ChainSpec::stable_bytes
deduplication behavior.

crates/uselesskey-core/tests/factory_env.rs (2)

69-72: ⚠️ Potential issue | 🟡 Minor

message.contains("invalid hex char") is brittle — it couples to internal library error text.

If the underlying hex parser changes its error message, this assertion breaks the test without indicating a real regression. Consider asserting only on the error variant and var field, and optionally logging message for diagnostics.

🔧 Proposed fix

     match err {
         Error::InvalidSeed { var: got, message } => {
             assert_eq!(got, var);
-            assert!(message.contains("invalid hex char"));
+            // Verify the message is non-empty (content is library-internal).
+            assert!(!message.is_empty(), "expected a non-empty error message, got: {message}");
         }
         other => panic!("unexpected error: {other:?}"),
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/uselesskey-core/tests/factory_env.rs` around lines 69 - 72, The test
currently asserts on the internal error text with Error::InvalidSeed { var: got,
message } and message.contains("invalid hex char"); remove the brittle string
assertion and instead only assert the error variant and that the captured var
equals the expected var (e.g., assert_eq!(got, var)); if you still want
diagnostics, optionally print or debug-log the message rather than asserting its
contents so the test no longer couples to the hex parser's error wording.

---

25-29: 🧹 Nitpick | 🔵 Trivial

Add a // SAFETY: comment to the unsafe remove_var call in Drop.

Drop can be invoked during thread unwind while other threads are iterating the environment. Even though the variable name is unique, the call still satisfies the unsafe contract only if no concurrent mutation of the environment occurs. Document the invariant being upheld.

✏️ Suggested safety comment

 impl Drop for EnvGuard {
     fn drop(&mut self) {
-        unsafe { std::env::remove_var(&self.key) };
+        // SAFETY: the variable name is unique to this test invocation
+        // (timestamp-derived), so no other thread observes or mutates it.
+        unsafe { std::env::remove_var(&self.key) };
     }
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/uselesskey-core/tests/factory_env.rs` around lines 25 - 29, Add a
SAFETY comment above the unsafe std::env::remove_var call in the EnvGuard Drop
impl that documents the invariant ensuring no concurrent mutation of the process
environment occurs when Drop runs; reference EnvGuard and its Drop
implementation and state that the safety relies on tests creating unique
variable names and ensuring EnvGuard is dropped only when no other threads may
access or mutate std::env (e.g., tests run single-threaded or callers
synchronize), so remove_var is safe to call during Drop.

tests/tls_integration.rs (1)

215-228: ⚠️ Potential issue | 🟡 Minor

test_deterministic_chains_produce_same_configs doesn't actually verify determinism.

The test name promises that the same seed + label produces identical chains, but the only assertion is that both server_config.alpn_protocols.len() == 0 — a trivially-zero property that proves nothing about cert content equality. The comment "cache hit on cloned Factory" describes the intended behavior but no assertion validates it.

✅ Suggested stronger assertions

 assert_eq!(server_config1.alpn_protocols.len(), 0);
 assert_eq!(server_config2.alpn_protocols.len(), 0);
+// Actually assert determinism — same seed + label must produce byte-identical certs
+assert_eq!(chain1.leaf_cert_pem(), chain2.leaf_cert_pem());
+assert_eq!(chain1.root_cert_pem(), chain2.root_cert_pem());

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/tls_integration.rs` around lines 215 - 228, The test
test_deterministic_chains_produce_same_configs currently only checks
alpn_protocols length and doesn't assert determinism; update it to actually
compare the outputs from fx().x509_chain("deterministic", ChainSpec::new(...))
by asserting equality between chain1 and chain2 results: for example, compare
the certificate bytes/DERs or the certificate list and private-key identifier
produced by chain1 and chain2, and/or assert that server_config_rustls()
produces equivalent configs (compare the certs presented by
server_config_rustls(), or compute and compare fingerprints of the certs/keys)
instead of the trivial alpn length checks so the test guarantees identical
chains/configs.

crates/uselesskey-core/tests/core_prop.rs (2)

30-53: ⚠️ Potential issue | 🟡 Minor

deterministic_is_order_independent_for_cache_keys doesn't actually exercise deterministic derivation.

Both init closures return hardcoded values (123u32, 456u32) and ignore _rng. After clear_cache() the closures again return the same hardcoded values, so *a == *a2 is trivially guaranteed regardless of seed, factory mode, or insertion order. The test would pass with Factory::random() too.

To validate that BLAKE3-keyed derivation produces the same RNG state irrespective of initialization order, the closures need to consume the provided RNG:

🐛 Suggested fix — use the RNG in the closures

-    let a = fx.get_or_init("domain:a", "label", &spec_bytes(1, 2), "good", |_rng| {
-        123u32
-    });
-    let b = fx.get_or_init("domain:b", "label", &spec_bytes(3, 4), "good", |_rng| {
-        456u32
-    });
+    let a = fx.get_or_init("domain:a", "label", &spec_bytes(1, 2), "good", |rng| {
+        use rand_core::RngCore;
+        rng.next_u32()
+    });
+    let b = fx.get_or_init("domain:b", "label", &spec_bytes(3, 4), "good", |rng| {
+        use rand_core::RngCore;
+        rng.next_u32()
+    });

     fx.clear_cache();
-    let b2 = fx.get_or_init("domain:b", "label", &spec_bytes(3, 4), "good", |_rng| {
-        456u32
-    });
-    let a2 = fx.get_or_init("domain:a", "label", &spec_bytes(1, 2), "good", |_rng| {
-        123u32
-    });
+    let b2 = fx.get_or_init("domain:b", "label", &spec_bytes(3, 4), "good", |rng| {
+        use rand_core::RngCore;
+        rng.next_u32()
+    });
+    let a2 = fx.get_or_init("domain:a", "label", &spec_bytes(1, 2), "good", |rng| {
+        use rand_core::RngCore;
+        rng.next_u32()
+    });

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/uselesskey-core/tests/core_prop.rs` around lines 30 - 53, The test
deterministic_is_order_independent_for_cache_keys is currently using hardcoded
return values in the get_or_init closures so it doesn't exercise the provided
RNG; update the closures passed to Factory::deterministic().get_or_init (and the
subsequent calls after fx.clear_cache()) to consume the supplied RNG parameter
(the |_rng| argument) to produce the returned u32s (e.g., call a RNG method like
gen::<u32>() / fill / next_u32 on the rng) so that the produced values depend on
the deterministic RNG state and therefore legitimately verify order-independence
of the BLAKE3-keyed derivation.

---

17-17: 🧹 Nitpick | 🔵 Trivial

Remove the unused _id binding in derive_seed_for_test.

_id is created but never read — get_or_init constructs its own ArtifactId internally from the same arguments. The _ prefix silences the warning but leaves dead code and a misleading comment.

♻️ Proposed cleanup

 fn derive_seed_for_test(master: &Seed, domain: &'static str, label: &str, variant: &str) -> Seed {
-    let _id = ArtifactId::new(domain, label, &[0u8; 8], variant, DerivationVersion::V1);
-    // Use the factory to observe the derived seed indirectly via determinism.
     let fx = Factory::deterministic(*master);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/uselesskey-core/tests/core_prop.rs` at line 17, The binding _id in
derive_seed_for_test is unused and should be removed to eliminate dead code;
delete the line that calls ArtifactId::new and the unused _id variable so the
test relies solely on the get_or_init call (which constructs its own ArtifactId
from the same domain, label, &[0u8; 8], variant, DerivationVersion::V1), keeping
derive_seed_for_test clean and warning-free.

crates/uselesskey-aws-lc-rs/src/lib.rs (1)

162-227: 🧹 Nitpick | 🔵 Trivial

ECDSA and Ed25519 test modules still use Factory::random() instead of fx().

The fx() deterministic helper is defined (lines 126–133) and used by rsa_tests, but ecdsa_tests (lines 171, 189) and ed25519_tests (line 215) still use Factory::random(). This is inconsistent with the PR's goal of consistent factory initialization and makes those tests non-reproducible. It also explains the #[allow(dead_code)] annotations on lines 122/125.

♻️ Proposed fix: use fx() in all test modules

     mod ecdsa_tests {
         use crate::AwsLcRsEcdsaKeyPairExt;
         use aws_lc_rs::signature::{self, KeyPair};
-        use uselesskey_core::Factory;
         use uselesskey_ecdsa::{EcdsaFactoryExt, EcdsaSpec};
 
         #[test]
         fn test_ecdsa_p256_sign_verify() {
-            let fx = Factory::random();
+            let fx = super::fx();
             let ecdsa = fx.ecdsa("test", EcdsaSpec::es256());

         #[test]
         fn test_ecdsa_p384_sign_verify() {
-            let fx = Factory::random();
+            let fx = super::fx();
             let ecdsa = fx.ecdsa("test", EcdsaSpec::es384());

     mod ed25519_tests {
         use crate::AwsLcRsEd25519KeyPairExt;
         use aws_lc_rs::signature::{self, KeyPair};
-        use uselesskey_core::Factory;
         use uselesskey_ed25519::{Ed25519FactoryExt, Ed25519Spec};
 
         #[test]
         fn test_ed25519_sign_verify() {
-            let fx = Factory::random();
+            let fx = super::fx();
             let ed = fx.ed25519("test", Ed25519Spec::new());

After this, the #[allow(dead_code)] annotations on lines 122 and 125 can be removed.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/uselesskey-aws-lc-rs/src/lib.rs` around lines 162 - 227, Tests in
ecdsa_tests (test_ecdsa_p256_sign_verify, test_ecdsa_p384_sign_verify) and
ed25519_tests (test_ed25519_sign_verify) use Factory::random() instead of the
deterministic helper fx(); replace each Factory::random() call with fx() (the
fx() helper defined earlier) so the tests use the deterministic factory, then
remove the now-unnecessary #[allow(dead_code)] annotations referenced near fx()
if desired.

crates/uselesskey-core/src/factory.rs (1)

261-267: 🧹 Nitpick | 🔵 Trivial

Redundant .to_string() on label and variant

ArtifactId::new accepts impl Into<String>, and &str already implements Into<String>. The .to_string() calls produce an unnecessary intermediate allocation.

♻️ Proposed simplification

         let id = ArtifactId::new(
             domain,
-            label.to_string(),
+            label,
             spec_bytes,
-            variant.to_string(),
+            variant,
             DerivationVersion::V1,
         );

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/uselesskey-core/src/factory.rs` around lines 261 - 267, The calls to
.to_string() when constructing the ArtifactId are redundant because
ArtifactId::new accepts impl Into<String> and &str already implements
Into<String>; update the ArtifactId::new invocation (the id creation) to pass
label and variant directly (keep domain and spec_bytes as before) and remove the
intermediate .to_string() allocations so the signature remains
ArtifactId::new(domain, label, spec_bytes, variant, DerivationVersion::V1).