feat: add comprehensive test coverage, per-crate READMEs, and X.509 SAN support#3
Conversation
…s, and roadmap updates
- Updated CLAUDE.md to clarify the problem with secret scanners and added context on ecosystem gaps. - Modified Cargo.lock to include `serde_json` and `uselesskey-core` dependencies. - Revised README.md to improve clarity on outputs and added context on secret incidents. - Expanded BDD tests in rsa.feature to cover additional scenarios for deterministic and random RSA key generation. - Updated bdd.rs to support new scenarios and added handling for corrupted PEM and truncated DER. - Introduced new features for JWK generation and negative fixtures with corresponding feature files. - Added migration guide to assist users in transitioning from committed key fixtures to runtime generation.
- Implemented Ed25519 keypair generation with PKCS#8 support in the `uselesskey-ed25519` crate. - Added specifications for Ed25519 keys, including stable encoding for deterministic derivation. - Introduced X.509 certificate generation in the `uselesskey-x509` crate, supporting self-signed certificates and various key usages. - Implemented negative testing fixtures for X.509 certificates, allowing for the generation of intentionally invalid certificates for error handling. - Updated documentation for usage examples and negative fixture generation.
- Introduced new commands: `FeatureMatrix`, `NoBlob`, and `PublishCheck` in the xtask CLI. - Implemented `ci` function to run common CI tasks including feature matrix checks. - Added `run_feature_matrix` function to validate various feature combinations. - Created `publish_check` function to perform dry-run publishes for crates in dependency order. - Added HMAC secret fixture generation with tests for JWK and JWKS. - Implemented new crates: `uselesskey-hmac` and `uselesskey-jwk` for HMAC and JWK handling. - Updated documentation for release and requirements to reflect new features and commands.
- Introduced a new crate `uselesskey-jsonwebtoken` for integrating JWT functionality with uselesskey test fixtures. - Implemented extension traits for encoding and decoding JWTs using RSA, ECDSA, Ed25519, and HMAC keys. - Added examples demonstrating signing and verifying JWTs with different key types. - Updated README to reflect the new crate and its usage. - Enhanced existing examples to showcase JWK/JWKS generation and tempfile handling for key fixtures. - Added `uselesskey-jsonwebtoken` as a member in the workspace and included necessary dependencies.
The no-blob check was flagging .rs test files that contain PEM markers as strings (e.g., "-----BEGIN TEST KEY-----" in PEM corruption tests). These are source code, not committed fixtures. Also adds mutants.out/ to .gitignore.
…odebase - Created `timeout.txt` to track timeout-related mutants. - Created `unviable.txt` to document unviable mutants with specific code replacements using `Default::default()` for various structures across multiple crates.
…btoken in publish check
- Bump Rust edition from 2021 to 2024 in Cargo.toml and fuzz/Cargo.toml. - Update various dependencies to their latest versions, including: - thiserror to 2.0.18 - blake3 to 1.8.3 - dashmap to 6.1.0 - tempfile to 3.24.0 - proptest to 1.10.0 - rstest to 0.26.1 - anyhow to 1.0.101 - clap to 4.5.57 - cucumber to 0.22.1 - tokio to 1.49.0 - rsa to 0.9.10 - ed25519-dalek to 2.2.0 - serde to 1.0.228 - serde_json to 1.0.149 - base64 to 0.22.1 - Update Clippy MSRV to 1.92. - Adjust versions of dependencies in the `uselesskey-bdd` crate. - Refactor imports in various files for clarity and consistency. - Ensure safety in environment variable manipulation in doctests.
…d clean up test imports
…tures - Added `X509Chain` struct for generating a three-level X.509 certificate chain (root CA, intermediate CA, leaf). - Introduced `ChainSpec` struct to define specifications for certificate generation, including common names, validity periods, and RSA key sizes. - Created utility functions for deterministic base time and serial number generation. - Implemented negative testing scenarios for certificate chains, including hostname mismatches, unknown CAs, and expired certificates. - Added comprehensive tests for chain generation, PEM formatting, determinism, and negative cases.
…25519 dead_code) - x509: default validity 365d -> 3650d for self-signed + chain leaves - x509: reuse RSA keys across chain/self-signed variants (no variant in key labels) - x509: add tests for >=5y validity and key reuse across negative variants - ed25519: cfg_attr allow(dead_code) for public field when jwk is disabled
…y reuse in negative variants
…mpacted crates in plan
- Introduced a new crate `uselesskey-ring` to provide integration with the `ring` library for RSA, ECDSA, and Ed25519 key types. - Updated `Cargo.toml` files to include `uselesskey-ring` as a member and added necessary dependencies. - Enhanced README.md to document the new crate and its usage examples. - Implemented extension traits in `uselesskey-ring` for converting uselesskey fixtures into `ring` native signing key types. - Added convenience builders for `rustls::ServerConfig` and `rustls::ClientConfig` in `uselesskey-rustls`. - Implemented support for CRLs in X.509 chains, including tests for revoked certificates. - Updated xtask scripts to include `uselesskey-ring` in publish checks and dependency tracking.
…ryptographic libraries - Implement tests for deterministic factory creation and environment variable handling in `uselesskey-core`. - Add comprehensive tests for ECDSA keypair generation and validation in `uselesskey-ecdsa`. - Introduce tests for Ed25519 keypair functionality in `uselesskey-ed25519`. - Create README files for `uselesskey-jsonwebtoken`, `uselesskey-ring`, `uselesskey-rustcrypto`, and `uselesskey-rustls` with usage examples and feature descriptions. - Implement RustCrypto integration for keypair conversion in `uselesskey-rustcrypto`. - Add tests for HMAC functionality in `uselesskey-hmac`. - Ensure facade tests validate core items and re-export functionality across different key types.
- Moved downcast logic to a separate function `downcast_or_panic` for better readability and reuse. - Added tests for `downcast_or_panic` to ensure it correctly handles type mismatches and returns expected values. - Created a new test module `factory_tests` to encapsulate tests related to the `Factory` struct. - Improved the `random_seed` function to ensure it generates a seed of the expected length. - Updated existing tests to enhance clarity and maintainability, including assertions for cache behavior and debug output. - Added additional tests for X.509 certificate specifications and negative scenarios to ensure comprehensive coverage.
…add max iterations for TLS handshake
- Implement JWT integration tests covering signing/verification flows for RSA, ECDSA, Ed25519, and HMAC algorithms. - Include tests for JWT with JWKS for key lookup and cross-crate compatibility. - Add test utilities for deterministic key generation. - Create TLS integration tests for server/client configuration, mTLS scenarios, and cross-key type compatibility. - Ensure tests cover various key sizes and certificate chain structures. - Validate determinism in key and chain generation across different labels.
…ltiple test files - Added missing newlines at the end of files in `uselesskey-ring` and `uselesskey-rustcrypto` test utilities. - Improved readability by formatting long lines and aligning method calls in `crypto_backend_integration.rs`, `e2e_workflows.rs`, `jwt_integration.rs`, and `tls_integration.rs`. - Consolidated import statements in `e2e_workflows.rs` and `jwt_integration.rs` for better organization. - Enhanced consistency in JWT encoding and decoding error handling across various test modules. - Updated chain specifications in `chain_tests` for better clarity and maintainability. - Cleaned up unnecessary whitespace and improved overall code style in `xtask/src/main.rs`.
- Fix Ed25519 sign API calls in aws-lc-rs tests (returns Signature, not Result) - Fix typo in rustls adapter: private_key_derustls -> private_key_der_rustls - Fix integration tests: correct cfg feature names, remove nonexistent API calls - Remove accidentally committed bdd_output file - Add #![allow(unused)] to adapter test utility modules - Remove aws-lc-rs dependency from integration tests (requires NASM)
- Fix broken aws-lc-rs example in root README (wrong trait/method names) - Fix `native (default)` claim in aws-lc-rs README and lib.rs doc comments - Add [package.metadata.docs.rs] to 10 crates for docs.rs feature coverage - Create dedicated READMEs for core, rsa, ecdsa, ed25519, x509 crates - Update Cargo.toml readme paths from root to per-crate READMEs - Merge CHANGELOG [Unreleased] into [0.2.0] with today's date - Add NASM install step to CI for aws-lc-rs coverage - Include in-progress aws-lc-rs build.rs and test updates
…ion, SAN step) P0: Fix plan.rs feature-matrix trigger — the condition required both crates/uselesskey/ AND .feature files, but .feature files live under crates/uselesskey-bdd/. Changed to OR logic so facade changes or BDD feature file changes each independently trigger feature-matrix. P0: Fix BDD default compilation — bdd.rs imports ECDSA/Ed25519/HMAC/ X509/JWK unconditionally, but uselesskey-bdd defaults only enable uselesskey/default (rsa). Make xtask bdd pass --features uk-all. P1: Fix SAN step in BDD — add_san_to_cert collected SANs in world state but never applied them to X509Spec. Added sans field to X509Spec with with_sans() builder, stable_bytes() encoding (bumped version to 3), and SAN injection in cert.rs generation. BDD step now passes accumulated SANs to the spec when regenerating. Also: remove native feature from aws-lc-rs docs.rs metadata (docs.rs won't install NASM), use rustdoc-args ["--cfg", "docsrs"] instead.
- Introduced `testutil.rs` for shared test utilities in `uselesskey-hmac` and `uselesskey-x509`. - Added comprehensive unit tests for JWK serialization, builder edge cases, and display implementations in `uselesskey-jwk`. - Implemented external unit tests for X.509 specifications, including SAN handling, key usage, and certificate ordering. - Created cross-adapter compatibility tests for ECDSA, Ed25519, and RSA to ensure interoperability between different cryptographic backends. - Developed key rotation tests covering JWT key rotation, HMAC secret rotation, cross-algorithm rotation, JWKS incremental builds, and TLS certificate rotation.
Summary by CodeRabbitRelease Notes
WalkthroughThis pull request introduces NASM build detection infrastructure, adds Subject Alternative Names (SANs) support to X.509 certificate generation, expands test coverage across multiple cryptographic adapters with integration and cross-adapter tests, updates documentation and feature metadata across all crates, and refreshes project planning status reflecting completed test coverage phases. Changes
Sequence Diagram(s)sequenceDiagram
participant Build as Build System
participant Script as build.rs
participant NASM as NASM Check
participant Compiler as Rust Compiler
participant Output as Compilation Output
Build->>Script: Execute build script
Script->>NASM: Run: nasm -v
alt NASM Available
NASM-->>Script: Version output
Script->>Compiler: Emit: rustc-cfg=has_nasm
Script->>Output: Warn: Compiling from source
Compiler->>Compiler: Compile with has_nasm=true
else NASM Not Found
NASM-->>Script: Command not found
Script->>Output: Warn: Tests will be skipped
Compiler->>Compiler: Compile with has_nasm=false
end
Compiler-->>Build: Build complete
sequenceDiagram
participant Test as Test/Builder
participant X509Spec as X509Spec
participant CertBuilder as CertificateParams
participant Generator as rcgen
participant Cert as Generated Cert
Test->>X509Spec: X509Spec::self_signed(cn)
X509Spec->>X509Spec: Store CN, initialize sans=[]
Test->>X509Spec: with_sans([dns1, dns2])
X509Spec->>X509Spec: Sort & deduplicate SANs
X509Spec->>CertBuilder: Create CertificateParams
CertBuilder->>CertBuilder: Add CN to params
CertBuilder->>CertBuilder: For each SAN: append DnsName
CertBuilder->>Generator: self_signed(key_pair)
Generator->>Cert: Generate cert with all SANs
Cert-->>Test: X.509 certificate + SANs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @EffortlessSteven, 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 significantly advances the project's stability and usability by completing the 0.2.0 roadmap. It introduces extensive new test suites to validate cryptographic operations, ensures interoperability between different crypto adapters, and implements robust key rotation mechanisms. Additionally, it enhances X.509 certificate generation with Subject Alternative Name support and improves documentation with dedicated READMEs for core crates, making the project more reliable and easier to understand for developers. Highlights
Changelog
Ignored Files
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 is a significant and high-quality pull request that completes the 0.2.0 roadmap. The introduction of comprehensive test suites, including external unit tests for multiple crates and new integration tests for key rotation and cross-adapter compatibility, greatly enhances the project's robustness. The addition of X.509 Subject Alternative Name (SAN) support is a valuable feature, and the implementation is solid, with careful attention to deterministic derivation. The per-crate READMEs and CI improvements, especially the NASM detection for aws-lc-rs, are also excellent additions. I have found one critical issue in the new build script that needs to be addressed.
|
|
||
| fn main() { | ||
| // Tell cargo about our custom cfg to avoid warnings | ||
| println!("cargo::rustc-check-cfg=cfg(has_nasm)"); |
There was a problem hiding this comment.
There is a typo in the println! macro for this build script instruction. The separator should be a single colon (:) instead of a double colon (::). The correct instruction is cargo:rustc-check-cfg=cfg(has_nasm).
Using :: will cause Cargo to ignore this instruction. While the build may still succeed, this can lead to "unknown cfg" warnings and makes the conditional compilation less robust across different toolchain versions.
| println!("cargo::rustc-check-cfg=cfg(has_nasm)"); | |
| println!("cargo:rustc-check-cfg=cfg(has_nasm)"); |
There was a problem hiding this comment.
💡 Codex Review
After default was changed to [], enabling only features = ["all"] no longer enables native, but every exported adapter trait is gated on feature = "native" in src/lib.rs; this makes the crate effectively a no-op for the common all feature path (including the new docs.rs config that requests only all). This is a regression from previous behavior where all worked via the default native feature.
ℹ️ 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".
| let mut sorted_sans = self.sans.clone(); | ||
| sorted_sans.sort(); | ||
| out.extend_from_slice(&(sorted_sans.len() as u32).to_be_bytes()); |
There was a problem hiding this comment.
Deduplicate SANs before serializing stable bytes
X509Spec::stable_bytes sorts SANs but does not deduplicate them, while certificate generation in load_inner_with_spec sorts and deduplicates before emitting SAN extensions; as a result, two specs that produce the same SAN set (e.g. ['a'] vs ['a','a']) still derive different seeds/cache keys, causing avoidable cache misses and inconsistent deterministic identity for equivalent cert parameters.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/uselesskey-x509/src/spec.rs (1)
244-331: 🧹 Nitpick | 🔵 TrivialInline tests don't cover the new
with_sansbuilder or SANs instable_bytes.The existing inline tests validate the default spec, builder methods, and stable_bytes determinism, but none exercise the
with_sansmethod or verify that SANs affect stable_bytes output. While the AI summary mentions external tests inx509_unit.rscover this, consider adding at least one inline test forwith_sansto maintain parity with the other builder method tests (test_builder_methods_applyon line 277).✅ Proposed inline test addition
#[test] fn test_with_sans_builder() { let spec = X509Spec::self_signed("san.example.com") .with_sans(vec!["a.example.com".into(), "b.example.com".into()]); assert_eq!(spec.sans.len(), 2); } #[test] fn test_sans_affect_stable_bytes() { let spec_no_sans = X509Spec::self_signed("test"); let spec_with_sans = X509Spec::self_signed("test") .with_sans(vec!["alt.example.com".into()]); assert_ne!(spec_no_sans.stable_bytes(), spec_with_sans.stable_bytes()); } #[test] fn test_sans_order_independent_stable_bytes() { let spec1 = X509Spec::self_signed("test") .with_sans(vec!["b.example.com".into(), "a.example.com".into()]); let spec2 = X509Spec::self_signed("test") .with_sans(vec!["a.example.com".into(), "b.example.com".into()]); assert_eq!(spec1.stable_bytes(), spec2.stable_bytes()); }crates/uselesskey-aws-lc-rs/src/lib.rs (1)
10-17: 🧹 Nitpick | 🔵 TrivialDoc comment doesn't mention
has_nasmno-op behavior.Line 10 mentions NASM is required "on Windows," and lines 16-17 describe the no-op behavior only when
nativeis disabled. Sincehas_nasmis now also required (on all platforms), the crate is also a no-op when NASM is absent even withnativeenabled. Consider updating the docs to reflect this.📝 Suggested doc update
-//! - `native` - Enable `aws-lc-rs` dependency (requires NASM on Windows). Disable for wasm-safe builds. +//! - `native` - Enable `aws-lc-rs` dependency. Disable for wasm-safe builds. //! - `rsa` - RSA keypairs -> `aws_lc_rs::rsa::KeyPair` //! - `ecdsa` - ECDSA keypairs -> `aws_lc_rs::signature::EcdsaKeyPair` //! - `ed25519` - Ed25519 keypairs -> `aws_lc_rs::signature::Ed25519KeyPair` //! - `all` - All key types above //! -//! When the `native` feature is disabled, this crate compiles as a no-op -//! with no traits or implementations available. +//! When the `native` feature is disabled or NASM is not detected at build time, +//! this crate compiles as a no-op with no traits or implementations available.
🤖 Fix all issues with AI agents
In `@crates/uselesskey-aws-lc-rs/build.rs`:
- Around line 8-23: The build script's cargo directive syntax is inconsistent:
replace the old single-colon directives in main (the println calls that emit
"cargo:rustc-cfg=has_nasm" and "cargo:warning=...") with the new double-colon
form ("cargo::rustc-cfg=has_nasm" and "cargo::warning=...") so all prints use
the stabilized cargo:: syntax, and add a rerun guard by printing
"cargo::rerun-if-env-changed=PATH" (e.g., at the top of main alongside the
existing println for rustc-check-cfg) so the build script only reruns when PATH
changes; update the println invocations in main accordingly.
In `@crates/uselesskey-aws-lc-rs/Cargo.toml`:
- Around line 22-24: docs.rs is configured to build with features = ["all"]
which omits the "native" feature so the public extension traits
AwsLcRsRsaKeyPairExt, AwsLcRsEcdsaKeyPairExt, and AwsLcRsEd25519KeyPairExt
(gated by #[cfg(feature = "native")]) won't appear; update the
package.metadata.docs.rs features array to include "native" (e.g. features =
["all", "native"]) so docs.rs enables the native/aws-lc-rs gated APIs when
generating documentation, and verify aws-lc-rs builds on docs.rs/Linux with NASM
or fallback available.
In `@crates/uselesskey-aws-lc-rs/tests/aws_lc_rs_comprehensive.rs`:
- Around line 13-27: The imports for testutil::fx and uselesskey_core::{Factory,
Seed} are only guarded with #[cfg(has_nasm)] which is less restrictive than the
module-level guards and may cause unused-import warnings; change their cfg
attributes to match the broader gate used elsewhere (e.g., use #[cfg(all(feature
= "native", has_nasm))] or the exact #[cfg(all(feature = "native", has_nasm,
...))] used by the modules that reference them) so that testutil::fx, Factory
and Seed are only compiled when the same full feature set (including feature =
"native") is active.
- Line 628: The cfg attribute on the test module uses a redundant nested all():
change #[cfg(all(feature = "native", has_nasm, all(feature = "rsa", feature =
"ecdsa")))] to a flattened form like #[cfg(all(feature = "native", has_nasm,
feature = "rsa", feature = "ecdsa"))] so the condition is equivalent but without
the nested all(); update the attribute where it appears (the cfg on the
test/module) accordingly.
In `@crates/uselesskey-hmac/Cargo.toml`:
- Around line 26-28: Remove the redundant dev-dependency entry for
uselesskey-core in the [dev-dependencies] section: since uselesskey-core is
already declared as a regular dependency (non-optional) it is available to tests
and the duplicate in [dev-dependencies] should be deleted; leave the base64 = {
workspace = true } line intact.
In `@crates/uselesskey-hmac/tests/hmac_unit.rs`:
- Around line 133-175: Replace the loop-based parameterization in
test_jwk_alg_matches_all_specs and test_jwk_k_decodes_to_secret_bytes with
rstest-based cases: annotate each test with #[rstest] and add #[case] parameters
for the different HmacSpec values (HmacSpec::hs256(), HmacSpec::hs384(),
HmacSpec::hs512()) and the expected_alg strings ("HS256","HS384","HS512") for
the first test; update the test signatures to accept the case parameters, call
fx().hmac(...) and perform the same assertions inside each case so each spec is
reported as a separate rstest case. Ensure you import rstest::rstest and adjust
the test function names (test_jwk_alg_matches_all_specs,
test_jwk_k_decodes_to_secret_bytes) to take the #[case] args and keep existing
uses of secret.jwk().to_value(), jwk["k"], and secret.secret_bytes().
In `@crates/uselesskey-jwk/tests/jwk_unit.rs`:
- Around line 213-232: Add assertions to the existing
test_rsa_private_debug_omits_all_private_fields (or create a new similar test)
to ensure Debug output for EcPrivateJwk, OkpPrivateJwk, and OctJwk also omits
private key material: construct instances via helpers (e.g., ec_private(...),
okp_private(...), oct_jwk(...)) or the same factory used for
rsa_private("kid-rsa"), format!("{:?}", instance) and assert the formatted debug
string contains the struct name and kid but does NOT contain sensitive fields
"d" for EcPrivateJwk and OkpPrivateJwk and "k" for OctJwk (use the same pattern
of assert!(!dbg.contains(...), "...")). Ensure you reference the concrete types
EcPrivateJwk, OkpPrivateJwk, and OctJwk in the assertions so all private key
fields are covered.
In `@crates/uselesskey-x509/README.md`:
- Around line 15-24: The README example uses wrong APIs; replace the incorrect
calls: instead of Factory::deterministic(b"test-seed") construct a Seed (e.g.
Seed::new([u8;32]) or call Factory::deterministic_from_env()) and pass that to
Factory::deterministic, call X509Spec::self_signed with a CN string (e.g.
"server.example.com"), use the extension method x509_self_signed (from the
X509FactoryExt trait) on the Factory instance instead of fx.x509, and call
private_key_pkcs8_pem() on the returned X509Cert for the key (keep
cert.cert_pem() for the cert); update the README snippet accordingly.
In `@crates/uselesskey-x509/src/spec.rs`:
- Around line 213-221: In stable_bytes (in spec.rs) the SAN list is sorted but
not deduplicated, causing different stable bytes for logically identical SAN
sets; update the sorted_sans handling used in stable_bytes so that after sorting
you call .dedup() on sorted_sans (mirror the behavior in the cert generation
path) before serializing lengths and bytes, ensuring X509Spec::stable_bytes and
the certificate generation produce the same canonical SAN representation.
In `@crates/uselesskey-x509/tests/x509_unit.rs`:
- Around line 310-327: The test test_x509_negative_apply_preserves_cn uses a
for-loop over X509Negative variants which short-circuits on failure; convert it
to parameterized rstest cases so each variant runs independently: replace the
loop with an rstest parameterized test that supplies each X509Negative variant
(Expired, NotYetValid, WrongKeyUsage, SelfSignedButClaimsCA) and asserts that
neg.apply_to_spec(&base).subject_cn == "preserve.example.com" using
X509Spec::self_signed("preserve.example.com") as the base; ensure the new test
function is annotated with rstest and each case is a separate test invocation.
In `@plans/summary.md`:
- Around line 15-21: The markdown headings in this file (e.g., the "### Current
State (updated 2026-02-15)" heading and other flagged headings at lines noted by
the linter) are missing required blank lines above and/or below them; fix by
inserting a single blank line before and after each heading (apply the pattern
demonstrated for "### Current State (updated 2026-02-15)") so headings at the
other flagged locations (the headings referenced by the linter around lines 43,
49, 67, 77, 86) each have exactly one blank line separating them from
surrounding paragraphs or lists.
In `@tests/e2e_workflows.rs`:
- Around line 594-596: The two JwksBuilder chains (creating jwks1 and jwks2) are
written as single-line builders; change them to the multi-line builder style
used elsewhere for consistency and readability by breaking the chain into
multiple lines (one call per line) for JwksBuilder::new(),
.add_public(rsa1.public_jwk()) / .add_public(rsa2.public_jwk()), and .build(),
keeping the same method calls and variable names (jwks1, jwks2, JwksBuilder,
rsa1.public_jwk, rsa2.public_jwk).
In `@tests/key_rotation.rs`:
- Around line 15-30: The literal 9999999999 overflows usize on 32-bit targets;
update the Claims struct and its test() to use a fixed-width integer (e.g.,
change the exp (and optionally iat) field type from usize to u64) and then use
9999999999u64 in Claims::test(), or alternatively replace the literal with a
32-bit-safe value (e.g., 999_999_999) in Claims::test(); adjust the Claims
definition and test() accordingly so the types and literal widths match.
In `@xtask/src/plan.rs`:
- Around line 337-342: Add a negative unit test alongside
bdd_feature_file_triggers_feature_matrix that calls build_plan with a
non-.feature path such as "crates/uselesskey-bdd/src/lib.rs" and asserts that
plan.run_feature_matrix is false; locate the existing test function
bdd_feature_file_triggers_feature_matrix and add a new #[test] function (e.g.,
bdd_non_feature_file_does_not_trigger_feature_matrix) which uses build_plan(...)
and asserts !plan.run_feature_matrix to prevent broadening the trigger
condition.
| fn main() { | ||
| // Tell cargo about our custom cfg to avoid warnings | ||
| println!("cargo::rustc-check-cfg=cfg(has_nasm)"); | ||
| // Check if NASM is available | ||
| let nasm_available = Command::new("nasm") | ||
| .arg("-v") | ||
| .output() | ||
| .map(|output| output.status.success()) | ||
| .unwrap_or(false); | ||
|
|
||
| if nasm_available { | ||
| println!("cargo:rustc-cfg=has_nasm"); | ||
| println!("cargo:warning=NASM found - aws-lc-rs will be compiled from source"); | ||
| } else { | ||
| println!("cargo:warning=NASM not found - aws-lc-rs tests will be skipped"); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Inconsistent cargo directive syntax: mixing cargo:: (new) and cargo: (old).
Line 10 uses the newer cargo::rustc-check-cfg syntax (double colon, stabilized in Rust 1.80), while lines 19–22 use the older cargo:rustc-cfg / cargo:warning syntax (single colon). Since MSRV is 1.88, prefer the new syntax consistently.
Also consider adding println!("cargo::rerun-if-env-changed=PATH"); so the build script only re-runs when PATH changes, rather than on every build.
♻️ Proposed fix for consistency and rebuild efficiency
fn main() {
// Tell cargo about our custom cfg to avoid warnings
println!("cargo::rustc-check-cfg=cfg(has_nasm)");
+ println!("cargo::rerun-if-env-changed=PATH");
+
// Check if NASM is available
let nasm_available = Command::new("nasm")
.arg("-v")
.output()
.map(|output| output.status.success())
.unwrap_or(false);
if nasm_available {
- println!("cargo:rustc-cfg=has_nasm");
- println!("cargo:warning=NASM found - aws-lc-rs will be compiled from source");
+ println!("cargo::rustc-cfg=has_nasm");
+ println!("cargo::warning=NASM found - aws-lc-rs will be compiled from source");
} else {
- println!("cargo:warning=NASM not found - aws-lc-rs tests will be skipped");
+ println!("cargo::warning=NASM not found - aws-lc-rs tests will be skipped");
}
}📝 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.
| fn main() { | |
| // Tell cargo about our custom cfg to avoid warnings | |
| println!("cargo::rustc-check-cfg=cfg(has_nasm)"); | |
| // Check if NASM is available | |
| let nasm_available = Command::new("nasm") | |
| .arg("-v") | |
| .output() | |
| .map(|output| output.status.success()) | |
| .unwrap_or(false); | |
| if nasm_available { | |
| println!("cargo:rustc-cfg=has_nasm"); | |
| println!("cargo:warning=NASM found - aws-lc-rs will be compiled from source"); | |
| } else { | |
| println!("cargo:warning=NASM not found - aws-lc-rs tests will be skipped"); | |
| } | |
| fn main() { | |
| // Tell cargo about our custom cfg to avoid warnings | |
| println!("cargo::rustc-check-cfg=cfg(has_nasm)"); | |
| println!("cargo::rerun-if-env-changed=PATH"); | |
| // Check if NASM is available | |
| let nasm_available = Command::new("nasm") | |
| .arg("-v") | |
| .output() | |
| .map(|output| output.status.success()) | |
| .unwrap_or(false); | |
| if nasm_available { | |
| println!("cargo::rustc-cfg=has_nasm"); | |
| println!("cargo::warning=NASM found - aws-lc-rs will be compiled from source"); | |
| } else { | |
| println!("cargo::warning=NASM not found - aws-lc-rs tests will be skipped"); | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@crates/uselesskey-aws-lc-rs/build.rs` around lines 8 - 23, The build script's
cargo directive syntax is inconsistent: replace the old single-colon directives
in main (the println calls that emit "cargo:rustc-cfg=has_nasm" and
"cargo:warning=...") with the new double-colon form ("cargo::rustc-cfg=has_nasm"
and "cargo::warning=...") so all prints use the stabilized cargo:: syntax, and
add a rerun guard by printing "cargo::rerun-if-env-changed=PATH" (e.g., at the
top of main alongside the existing println for rustc-check-cfg) so the build
script only reruns when PATH changes; update the println invocations in main
accordingly.
| [package.metadata.docs.rs] | ||
| features = ["all"] | ||
| rustdoc-args = ["--cfg", "docsrs"] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find and read the Cargo.toml file
find . -path "*uselesskey-aws-lc-rs/Cargo.toml" -type fRepository: EffortlessMetrics/uselesskey
Length of output: 112
🏁 Script executed:
#!/bin/bash
# Read the Cargo.toml file to see the actual feature definitions
cat -n crates/uselesskey-aws-lc-rs/Cargo.tomlRepository: EffortlessMetrics/uselesskey
Length of output: 1685
🏁 Script executed:
#!/bin/bash
# Search for public items gated on "native" or "has_nasm" features
rg -n '#\[cfg\(.*native.*\)\]|#\[cfg\(.*has_nasm.*\)\]' --type=rust -C2Repository: EffortlessMetrics/uselesskey
Length of output: 8142
docs.rs build won't include the native feature — the main extension trait API will be missing from generated docs.
The all feature (line 35) enables only the key-type crate dependencies (rsa, ecdsa, ed25519) but not native (which gates dep:aws-lc-rs). Since docs.rs is configured with features = ["all"] (line 23), the public extension traits—AwsLcRsRsaKeyPairExt, AwsLcRsEcdsaKeyPairExt, and AwsLcRsEd25519KeyPairExt—are all gated on #[cfg(all(feature = "native", has_nasm, ...))] and will not appear in the published documentation.
Proposed fix
[package.metadata.docs.rs]
-features = ["all"]
+features = ["native", "all"]
rustdoc-args = ["--cfg", "docsrs"]This fix depends on aws-lc-rs successfully building with NASM support on the docs.rs Linux environment. If NASM is unavailable, the traits still won't appear. Verify that aws-lc-rs can build on docs.rs (it typically can on Linux with a C compiler fallback).
📝 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.
| [package.metadata.docs.rs] | |
| features = ["all"] | |
| rustdoc-args = ["--cfg", "docsrs"] | |
| [package.metadata.docs.rs] | |
| features = ["native", "all"] | |
| rustdoc-args = ["--cfg", "docsrs"] |
🤖 Prompt for AI Agents
In `@crates/uselesskey-aws-lc-rs/Cargo.toml` around lines 22 - 24, docs.rs is
configured to build with features = ["all"] which omits the "native" feature so
the public extension traits AwsLcRsRsaKeyPairExt, AwsLcRsEcdsaKeyPairExt, and
AwsLcRsEd25519KeyPairExt (gated by #[cfg(feature = "native")]) won't appear;
update the package.metadata.docs.rs features array to include "native" (e.g.
features = ["all", "native"]) so docs.rs enables the native/aws-lc-rs gated APIs
when generating documentation, and verify aws-lc-rs builds on docs.rs/Linux with
NASM or fallback available.
| #[cfg(all(feature = "native", has_nasm))] | ||
| use aws_lc_rs::{ | ||
| digest, | ||
| hmac::{self, Key as HmacKey}, | ||
| rand::SystemRandom, | ||
| signature::{self, KeyPair}, | ||
| }; | ||
| #[cfg(has_nasm)] | ||
| use testutil::fx; | ||
| #[cfg(feature = "native")] | ||
| #[cfg(all(feature = "native", has_nasm))] | ||
| use uselesskey_aws_lc_rs::{ | ||
| AwsLcRsEcdsaKeyPairExt, AwsLcRsEd25519KeyPairExt, AwsLcRsRsaKeyPairExt, | ||
| }; | ||
| #[cfg(has_nasm)] | ||
| use uselesskey_core::{Factory, Seed}; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
NASM gating on imports is appropriately applied.
The #[cfg(has_nasm)] on testutil::fx and uselesskey_core imports (lines 20, 26) is less restrictive than the module-level #[cfg(all(feature = "native", has_nasm, ...))] guards. This won't cause issues since the items are only used within the more restrictive modules, but unused-import warnings could surface when has_nasm is set but native or specific key-type features are not. If clippy or CI warns, consider tightening these to match the broadest module gate.
🤖 Prompt for AI Agents
In `@crates/uselesskey-aws-lc-rs/tests/aws_lc_rs_comprehensive.rs` around lines 13
- 27, The imports for testutil::fx and uselesskey_core::{Factory, Seed} are only
guarded with #[cfg(has_nasm)] which is less restrictive than the module-level
guards and may cause unused-import warnings; change their cfg attributes to
match the broader gate used elsewhere (e.g., use #[cfg(all(feature = "native",
has_nasm))] or the exact #[cfg(all(feature = "native", has_nasm, ...))] used by
the modules that reference them) so that testutil::fx, Factory and Seed are only
compiled when the same full feature set (including feature = "native") is
active.
| } | ||
|
|
||
| #[cfg(all(feature = "native", all(feature = "rsa", feature = "ecdsa")))] | ||
| #[cfg(all(feature = "native", has_nasm, all(feature = "rsa", feature = "ecdsa")))] |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Redundant nested all() in cfg attribute.
#[cfg(all(feature = "native", has_nasm, all(feature = "rsa", feature = "ecdsa")))] can be flattened.
♻️ Suggested simplification
-#[cfg(all(feature = "native", has_nasm, all(feature = "rsa", feature = "ecdsa")))]
+#[cfg(all(feature = "native", has_nasm, feature = "rsa", feature = "ecdsa"))]📝 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.
| #[cfg(all(feature = "native", has_nasm, all(feature = "rsa", feature = "ecdsa")))] | |
| #[cfg(all(feature = "native", has_nasm, feature = "rsa", feature = "ecdsa"))] |
🤖 Prompt for AI Agents
In `@crates/uselesskey-aws-lc-rs/tests/aws_lc_rs_comprehensive.rs` at line 628,
The cfg attribute on the test module uses a redundant nested all(): change
#[cfg(all(feature = "native", has_nasm, all(feature = "rsa", feature =
"ecdsa")))] to a flattened form like #[cfg(all(feature = "native", has_nasm,
feature = "rsa", feature = "ecdsa"))] so the condition is equivalent but without
the nested all(); update the attribute where it appears (the cfg on the
test/module) accordingly.
| [dev-dependencies] | ||
| uselesskey-core = { path = "../uselesskey-core", version = "0.2.0" } | ||
| base64 = { workspace = true } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Redundant uselesskey-core dev-dependency.
uselesskey-core is already declared as a non-optional dependency on line 15, which makes it unconditionally available in tests. The duplicate entry in [dev-dependencies] is unnecessary.
The base64 entry is fine — it ensures test code can use base64 without requiring the jwk feature flag.
♻️ Suggested cleanup
[dev-dependencies]
-uselesskey-core = { path = "../uselesskey-core", version = "0.2.0" }
base64 = { workspace = true }📝 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.
| [dev-dependencies] | |
| uselesskey-core = { path = "../uselesskey-core", version = "0.2.0" } | |
| base64 = { workspace = true } | |
| [dev-dependencies] | |
| base64 = { workspace = true } |
🤖 Prompt for AI Agents
In `@crates/uselesskey-hmac/Cargo.toml` around lines 26 - 28, Remove the redundant
dev-dependency entry for uselesskey-core in the [dev-dependencies] section:
since uselesskey-core is already declared as a regular dependency (non-optional)
it is available to tests and the duplicate in [dev-dependencies] should be
deleted; leave the base64 = { workspace = true } line intact.
| #[test] | ||
| fn test_x509_negative_apply_preserves_cn() { | ||
| let base = X509Spec::self_signed("preserve.example.com"); | ||
|
|
||
| for neg in [ | ||
| X509Negative::Expired, | ||
| X509Negative::NotYetValid, | ||
| X509Negative::WrongKeyUsage, | ||
| X509Negative::SelfSignedButClaimsCA, | ||
| ] { | ||
| let modified = neg.apply_to_spec(&base); | ||
| assert_eq!( | ||
| modified.subject_cn, "preserve.example.com", | ||
| "{:?} should preserve subject_cn", | ||
| neg | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using rstest for parameterized variant testing.
The for-loop approach works but means a failure on one variant short-circuits the remaining assertions. With rstest, each variant runs as an independent test case, giving full visibility on failures.
♻️ Suggested refactor
+use rstest::rstest;
+
+#[rstest]
+#[case(X509Negative::Expired)]
+#[case(X509Negative::NotYetValid)]
+#[case(X509Negative::WrongKeyUsage)]
+#[case(X509Negative::SelfSignedButClaimsCA)]
+fn test_x509_negative_apply_preserves_cn(#[case] neg: X509Negative) {
let base = X509Spec::self_signed("preserve.example.com");
-
- for neg in [
- X509Negative::Expired,
- X509Negative::NotYetValid,
- X509Negative::WrongKeyUsage,
- X509Negative::SelfSignedButClaimsCA,
- ] {
- let modified = neg.apply_to_spec(&base);
- assert_eq!(
- modified.subject_cn, "preserve.example.com",
- "{:?} should preserve subject_cn",
- neg
- );
- }
+ let modified = neg.apply_to_spec(&base);
+ assert_eq!(
+ modified.subject_cn, "preserve.example.com",
+ "{:?} should preserve subject_cn",
+ neg
+ );
}As per coding guidelines, crates/**/*.rs: "Use rstest for parameterized tests".
📝 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.
| #[test] | |
| fn test_x509_negative_apply_preserves_cn() { | |
| let base = X509Spec::self_signed("preserve.example.com"); | |
| for neg in [ | |
| X509Negative::Expired, | |
| X509Negative::NotYetValid, | |
| X509Negative::WrongKeyUsage, | |
| X509Negative::SelfSignedButClaimsCA, | |
| ] { | |
| let modified = neg.apply_to_spec(&base); | |
| assert_eq!( | |
| modified.subject_cn, "preserve.example.com", | |
| "{:?} should preserve subject_cn", | |
| neg | |
| ); | |
| } | |
| } | |
| #[rstest] | |
| #[case(X509Negative::Expired)] | |
| #[case(X509Negative::NotYetValid)] | |
| #[case(X509Negative::WrongKeyUsage)] | |
| #[case(X509Negative::SelfSignedButClaimsCA)] | |
| fn test_x509_negative_apply_preserves_cn(#[case] neg: X509Negative) { | |
| let base = X509Spec::self_signed("preserve.example.com"); | |
| let modified = neg.apply_to_spec(&base); | |
| assert_eq!( | |
| modified.subject_cn, "preserve.example.com", | |
| "{:?} should preserve subject_cn", | |
| neg | |
| ); | |
| } |
🤖 Prompt for AI Agents
In `@crates/uselesskey-x509/tests/x509_unit.rs` around lines 310 - 327, The test
test_x509_negative_apply_preserves_cn uses a for-loop over X509Negative variants
which short-circuits on failure; convert it to parameterized rstest cases so
each variant runs independently: replace the loop with an rstest parameterized
test that supplies each X509Negative variant (Expired, NotYetValid,
WrongKeyUsage, SelfSignedButClaimsCA) and asserts that
neg.apply_to_spec(&base).subject_cn == "preserve.example.com" using
X509Spec::self_signed("preserve.example.com") as the base; ensure the new test
function is annotated with rstest and each case is a separate test invocation.
| ### Current State (updated 2026-02-15) | ||
| - **15 BDD feature files** with 250+ scenarios | ||
| - **7 crates** have unit/integration tests (uselesskey-core, uselesskey-rsa, uselesskey-ecdsa, uselesskey-ed25519, uselesskey-jsonwebtoken, uselesskey-ring, uselesskey-aws-lc-rs) | ||
| - **5 crates** still need unit tests (uselesskey-x509, uselesskey-hmac, uselesskey-jwk, uselesskey-rustls, uselesskey-rustcrypto) | ||
| - **10 crates** have unit/integration tests (uselesskey-core, uselesskey-rsa, uselesskey-ecdsa, uselesskey-ed25519, uselesskey-jsonwebtoken, uselesskey-ring, uselesskey-aws-lc-rs, uselesskey-x509, uselesskey-hmac, uselesskey-jwk) | ||
| - **2 crates** still need unit tests (uselesskey-rustls, uselesskey-rustcrypto) | ||
| - **BDD features cover** JWT, TLS, and edge case integration scenarios | ||
| - **Cross-adapter compatibility tests** verify ring/RustCrypto interoperability | ||
| - **Key rotation workflow tests** cover JWT, JWKS, and TLS rotation scenarios |
There was a problem hiding this comment.
Fix missing blank lines around headings (markdownlint MD022).
Static analysis flags multiple headings that lack the required surrounding blank lines. This affects lines 15, 43, 49, 67, 77, and 86.
🔧 Example fix for the pattern (line 15)
-### Current State (updated 2026-02-15)
+
+### Current State (updated 2026-02-15)
+
- **15 BDD feature files** with 250+ scenariosApply the same blank-line insertion around all flagged headings (lines 43, 49, 67, 77, 86).
🧰 Tools
🪛 markdownlint-cli2 (0.20.0)
[warning] 15-15: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
In `@plans/summary.md` around lines 15 - 21, The markdown headings in this file
(e.g., the "### Current State (updated 2026-02-15)" heading and other flagged
headings at lines noted by the linter) are missing required blank lines above
and/or below them; fix by inserting a single blank line before and after each
heading (apply the pattern demonstrated for "### Current State (updated
2026-02-15)") so headings at the other flagged locations (the headings
referenced by the linter around lines 43, 49, 67, 77, 86) each have exactly one
blank line separating them from surrounding paragraphs or lists.
| let jwks1 = JwksBuilder::new().add_public(rsa1.public_jwk()).build(); | ||
|
|
||
| let jwks2 = JwksBuilder::new() | ||
| .add_public(rsa2.public_jwk()) | ||
| .build(); | ||
| let jwks2 = JwksBuilder::new().add_public(rsa2.public_jwk()).build(); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider multi-line format for consistency with other builder patterns.
These builder chains are now single-line, while similar builder patterns elsewhere in the file use multi-line formatting (e.g., lines 104-108, 157-160, 308-312, 369-372). For consistency and readability, consider using the multi-line format:
♻️ Suggested formatting for consistency
- let jwks1 = JwksBuilder::new().add_public(rsa1.public_jwk()).build();
+ let jwks1 = JwksBuilder::new()
+ .add_public(rsa1.public_jwk())
+ .build();
- let jwks2 = JwksBuilder::new().add_public(rsa2.public_jwk()).build();
+ let jwks2 = JwksBuilder::new()
+ .add_public(rsa2.public_jwk())
+ .build();📝 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.
| let jwks1 = JwksBuilder::new().add_public(rsa1.public_jwk()).build(); | |
| let jwks2 = JwksBuilder::new() | |
| .add_public(rsa2.public_jwk()) | |
| .build(); | |
| let jwks2 = JwksBuilder::new().add_public(rsa2.public_jwk()).build(); | |
| let jwks1 = JwksBuilder::new() | |
| .add_public(rsa1.public_jwk()) | |
| .build(); | |
| let jwks2 = JwksBuilder::new() | |
| .add_public(rsa2.public_jwk()) | |
| .build(); |
🤖 Prompt for AI Agents
In `@tests/e2e_workflows.rs` around lines 594 - 596, The two JwksBuilder chains
(creating jwks1 and jwks2) are written as single-line builders; change them to
the multi-line builder style used elsewhere for consistency and readability by
breaking the chain into multiple lines (one call per line) for
JwksBuilder::new(), .add_public(rsa1.public_jwk()) /
.add_public(rsa2.public_jwk()), and .build(), keeping the same method calls and
variable names (jwks1, jwks2, JwksBuilder, rsa1.public_jwk, rsa2.public_jwk).
| #[derive(Debug, Serialize, Deserialize, PartialEq)] | ||
| struct Claims { | ||
| sub: String, | ||
| exp: usize, | ||
| iat: usize, | ||
| } | ||
|
|
||
| impl Claims { | ||
| fn test() -> Self { | ||
| Self { | ||
| sub: "user123".to_string(), | ||
| exp: 9999999999, | ||
| iat: 1234567890, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
exp value overflows usize on 32-bit targets.
9999999999 exceeds the maximum u32 value (4_294_967_295), so this literal won't compile on 32-bit platforms where usize is 32 bits. If only 64-bit targets are supported for tests, this is fine, but it's fragile.
🛡️ Suggested fix: use a value within 32-bit range or annotate the type
impl Claims {
fn test() -> Self {
Self {
sub: "user123".to_string(),
- exp: 9999999999,
+ exp: 4_102_444_800, // 2100-01-01 00:00:00 UTC — fits in u32
iat: 1234567890,
}
}
}🤖 Prompt for AI Agents
In `@tests/key_rotation.rs` around lines 15 - 30, The literal 9999999999 overflows
usize on 32-bit targets; update the Claims struct and its test() to use a
fixed-width integer (e.g., change the exp (and optionally iat) field type from
usize to u64) and then use 9999999999u64 in Claims::test(), or alternatively
replace the literal with a 32-bit-safe value (e.g., 999_999_999) in
Claims::test(); adjust the Claims definition and test() accordingly so the types
and literal widths match.
| #[test] | ||
| fn bdd_feature_file_triggers_feature_matrix() { | ||
| let paths = vec!["crates/uselesskey-bdd/features/rsa.feature".to_string()]; | ||
| let plan = build_plan(&paths); | ||
| assert!(plan.run_feature_matrix); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding a negative test for non-.feature files in uselesskey-bdd.
The positive case is well-covered. A complementary test asserting that e.g. crates/uselesskey-bdd/src/lib.rs does not trigger run_feature_matrix would guard against accidentally broadening the condition later.
📝 Suggested negative test
+ #[test]
+ fn bdd_rust_file_does_not_trigger_feature_matrix() {
+ let paths = vec!["crates/uselesskey-bdd/src/lib.rs".to_string()];
+ let plan = build_plan(&paths);
+ assert!(!plan.run_feature_matrix);
+ }📝 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.
| #[test] | |
| fn bdd_feature_file_triggers_feature_matrix() { | |
| let paths = vec!["crates/uselesskey-bdd/features/rsa.feature".to_string()]; | |
| let plan = build_plan(&paths); | |
| assert!(plan.run_feature_matrix); | |
| } | |
| #[test] | |
| fn bdd_feature_file_triggers_feature_matrix() { | |
| let paths = vec!["crates/uselesskey-bdd/features/rsa.feature".to_string()]; | |
| let plan = build_plan(&paths); | |
| assert!(plan.run_feature_matrix); | |
| } | |
| #[test] | |
| fn bdd_rust_file_does_not_trigger_feature_matrix() { | |
| let paths = vec!["crates/uselesskey-bdd/src/lib.rs".to_string()]; | |
| let plan = build_plan(&paths); | |
| assert!(!plan.run_feature_matrix); | |
| } |
🤖 Prompt for AI Agents
In `@xtask/src/plan.rs` around lines 337 - 342, Add a negative unit test alongside
bdd_feature_file_triggers_feature_matrix that calls build_plan with a
non-.feature path such as "crates/uselesskey-bdd/src/lib.rs" and asserts that
plan.run_feature_matrix is false; locate the existing test function
bdd_feature_file_triggers_feature_matrix and add a new #[test] function (e.g.,
bdd_non_feature_file_does_not_trigger_feature_matrix) which uses build_plan(...)
and asserts !plan.run_feature_matrix to prevent broadening the trigger
condition.
There was a problem hiding this comment.
Pull request overview
This PR completes the 0.2.0 testing/documentation roadmap by adding new external test suites (per-crate + cross-crate workflows), introducing X.509 Subject Alternative Name (SAN) support with deterministic derivation updates, and tightening CI/tooling to reliably run the expanded coverage.
Changes:
- Add large external unit + integration test suites (key rotation workflows, cross-adapter interoperability, and new per-crate external tests).
- Add X.509 SAN support (
X509Spec::with_sans) and bump deterministic derivation versioning to include SANs. - Improve docs/packaging (per-crate READMEs, docs.rs metadata) and CI/tooling (feature-matrix triggers, BDD features, NASM handling).
Reviewed changes
Copilot reviewed 38 out of 39 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
xtask/src/plan.rs |
Updates feature-matrix trigger logic and adds regression tests for path/BDD triggers. |
xtask/src/main.rs |
Runs BDD tests with --features uk-all for full coverage. |
tests/key_rotation.rs |
Adds integration tests covering JWT/JWKS/HMAC/TLS rotation workflows. |
tests/e2e_workflows.rs |
Formatting cleanup in integration workflow tests. |
tests/cross_adapter.rs |
Adds ring ↔ RustCrypto interoperability tests (ECDSA/Ed25519/RSA) plus public-key identity checks. |
tests/Cargo.toml |
Adds new feature-gated integration tests and optional deps (rsa/p256/sha2/etc). |
plans/summary.md |
Updates the testing coverage plan/status summary. |
crates/uselesskey-x509/tests/x509_unit.rs |
Adds external unit tests for X.509 specs, SAN determinism, PEM ordering, chain negatives, etc. |
crates/uselesskey-x509/tests/testutil.rs |
Adds deterministic test factory helper for external x509 tests. |
crates/uselesskey-x509/src/spec.rs |
Adds sans field + with_sans, bumps stable-bytes version and encodes SANs for deterministic derivation. |
crates/uselesskey-x509/src/cert.rs |
Encodes SANs into generated certificates (sorted/deduped). |
crates/uselesskey-x509/README.md |
New per-crate README for x509. |
crates/uselesskey-x509/Cargo.toml |
Points readme to per-crate file and configures docs.rs features. |
crates/uselesskey-rustls/Cargo.toml |
Adds docs.rs metadata features for rustls adapter crate. |
crates/uselesskey-rustcrypto/Cargo.toml |
Adds docs.rs metadata features for rustcrypto adapter crate. |
crates/uselesskey-rsa/README.md |
New per-crate README for RSA fixtures. |
crates/uselesskey-rsa/Cargo.toml |
Points readme to per-crate file and sets docs.rs features. |
crates/uselesskey-ring/Cargo.toml |
Adds docs.rs metadata features for ring adapter crate. |
crates/uselesskey-jwk/tests/jwk_unit.rs |
Adds external unit tests for serde renames, Display/Debug, builder edge cases, and kid delegation. |
crates/uselesskey-jsonwebtoken/Cargo.toml |
Adds docs.rs metadata features for jsonwebtoken adapter crate. |
crates/uselesskey-hmac/tests/testutil.rs |
Adds deterministic test factory helper for external hmac tests. |
crates/uselesskey-hmac/tests/hmac_unit.rs |
Adds external unit tests for secret lengths, determinism, debug redaction, stable bytes, and JWK output. |
crates/uselesskey-hmac/Cargo.toml |
Adds docs.rs metadata + dev-deps needed by external tests. |
crates/uselesskey-ed25519/README.md |
New per-crate README for Ed25519 fixtures. |
crates/uselesskey-ed25519/Cargo.toml |
Points readme to per-crate file and sets docs.rs features. |
crates/uselesskey-ecdsa/README.md |
New per-crate README for ECDSA fixtures. |
crates/uselesskey-ecdsa/Cargo.toml |
Points readme to per-crate file and sets docs.rs features. |
crates/uselesskey-core/README.md |
New per-crate README for the core factory/derivation engine. |
crates/uselesskey-core/Cargo.toml |
Points readme to per-crate file. |
crates/uselesskey-bdd/tests/bdd.rs |
Updates SAN-related BDD step to accumulate/apply SANs via with_sans(). |
crates/uselesskey-aws-lc-rs/tests/aws_lc_rs_comprehensive.rs |
Gates aws-lc-rs tests on has_nasm for graceful skipping. |
crates/uselesskey-aws-lc-rs/src/lib.rs |
Gates aws-lc-rs adapter traits/impls behind has_nasm + updates docs. |
crates/uselesskey-aws-lc-rs/build.rs |
Adds NASM detection and sets cfg(has_nasm). |
crates/uselesskey-aws-lc-rs/README.md |
Updates aws-lc-rs README for explicit native feature enablement. |
crates/uselesskey-aws-lc-rs/Cargo.toml |
Changes default features + adds docs.rs metadata. |
README.md |
Fixes aws-lc-rs example to use correct RSA API names/features. |
Cargo.lock |
Adds new dependency entries needed for new test suites/features. |
CHANGELOG.md |
Restructures 0.2.0 entry and updates release date. |
.github/workflows/ci.yml |
Installs NASM in CI jobs to support aws-lc-rs builds/tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| uselesskey-ed25519 = { path = "../uselesskey-ed25519", version = "0.2.0", optional = true } | ||
|
|
||
| [package.metadata.docs.rs] | ||
| features = ["all"] |
There was a problem hiding this comment.
docs.rs metadata enables only features = ["all"], but this crate’s public extension traits are additionally gated behind feature = "native" and cfg(has_nasm). As a result, docs.rs will build without documenting any of the main API surface. Consider either enabling native for docs.rs builds, or adjusting the cfg gates to expose the traits under cfg(docsrs) (even if implementations stay gated) so the docs aren’t effectively empty.
| features = ["all"] | |
| features = ["all", "native"] |
| // SANs (sorted for stability) | ||
| let mut sorted_sans = self.sans.clone(); | ||
| sorted_sans.sort(); | ||
| out.extend_from_slice(&(sorted_sans.len() as u32).to_be_bytes()); | ||
| for san in &sorted_sans { | ||
| let san_bytes = san.as_bytes(); | ||
| out.extend_from_slice(&(san_bytes.len() as u32).to_be_bytes()); | ||
| out.extend_from_slice(san_bytes); | ||
| } |
There was a problem hiding this comment.
stable_bytes() sorts SANs but does not deduplicate them, while certificate generation in cert.rs sorts + dedup()s before encoding. This means semantically equivalent SAN inputs (e.g. duplicates) can produce different derivation bytes and therefore different deterministic cert outputs/cache keys. Consider deduping (and ideally normalizing) SANs in stable_bytes() (or in with_sans() before storing) so duplicates don’t affect determinism.
| use uselesskey_core::Factory; | ||
| use uselesskey_x509::{X509FactoryExt, X509Spec}; | ||
|
|
||
| let fx = Factory::deterministic(b"test-seed"); | ||
| let cert = fx.x509("server", X509Spec::self_signed()); | ||
|
|
||
| let cert_pem = cert.cert_pem(); | ||
| let key_pem = cert.key_pem(); | ||
| ``` |
There was a problem hiding this comment.
The example code doesn’t match the crate API: Factory::deterministic expects a Seed (not raw bytes), X509Spec::self_signed requires a CN argument, and the factory method is x509_self_signed (not x509). As written, the README example won’t compile.
| use uselesskey_core::Factory; | ||
|
|
||
| // Random mode -- fresh key material every run | ||
| let fx = Factory::random(); | ||
|
|
||
| // Deterministic mode -- reproducible from seed | ||
| let fx = Factory::deterministic(b"my-test-seed"); |
There was a problem hiding this comment.
Factory::deterministic takes a Seed, but the README example passes a byte string (b"my-test-seed"). This example won’t compile unless converted to a Seed first (e.g. via Seed::from_env_value(...) or Seed::new([u8; 32])).
| use uselesskey_core::Factory; | |
| // Random mode -- fresh key material every run | |
| let fx = Factory::random(); | |
| // Deterministic mode -- reproducible from seed | |
| let fx = Factory::deterministic(b"my-test-seed"); | |
| use uselesskey_core::{Factory, Seed}; | |
| // Random mode -- fresh key material every run | |
| let fx = Factory::random(); | |
| // Deterministic mode -- reproducible from seed | |
| let fx = Factory::deterministic(Seed::from_env_value(b"my-test-seed")); |
| #[derive(Debug, Serialize, Deserialize, PartialEq)] | ||
| struct Claims { | ||
| sub: String, | ||
| exp: usize, | ||
| iat: usize, | ||
| } | ||
|
|
||
| impl Claims { | ||
| fn test() -> Self { | ||
| Self { | ||
| sub: "user123".to_string(), | ||
| exp: 9999999999, | ||
| iat: 1234567890, | ||
| } |
There was a problem hiding this comment.
exp is set to 9999999999 as a usize. This literal overflows usize on 32-bit targets and will fail to compile there. Use a 64-bit type for JWT timestamps (e.g. u64/i64) or choose values that fit u32/usize if 32-bit support matters.
Summary
Completes the 0.2.0 roadmap with comprehensive external test suites, per-crate documentation, X.509 Subject Alternative Name support, cross-adapter interoperability tests, and CI fixes.
Test Coverage (5 new test suites, ~1300 lines)
crates/uselesskey-hmac/tests/hmac_unit.rs— Secret length validation (HS256/HS384/HS512), label/spec isolation, debug safety (no secret leakage), determinism survival through cache clear,stable_bytesexact values, JWK field validationcrates/uselesskey-jwk/tests/jwk_unit.rs— Serde rename validation (use_→"use"), empty builder edge case, Display impls for all enum variants, debug safety (RSA private fields omitted), AnyJwk kid delegation across 6 pathscrates/uselesskey-x509/tests/x509_unit.rs—X509Spec::with_sans()builder,stable_bytesversion prefix (X509Spec=3, ChainSpec=1), SAN order-independence for determinism, KeyUsage exact values, tempfile content matching, PEM ordering guarantees (identity/chain/full_chain), ChainNegative variant coverage, chain issuer validation via DER parsingtests/cross_adapter.rs— Sign with ring / verify with RustCrypto (and reverse) for ECDSA P-256, Ed25519, and RSA; public key byte identity across adapterstests/key_rotation.rs— JWT rotation (old/new key isolation, grace period JWKS), HMAC secret rotation, cross-algorithm rotation (Ed25519 → ECDSA), JWKS incremental build/removal, kid uniqueness, deterministic kid stability, TLS cert rotation (chain and self-signed)X.509 SAN Support
X509Spec::with_sans(Vec<String>)builder methodstable_bytesversion bumped from v2 → v3 to include SAN fieldrcgen::SanType::DnsNamewith_sans()properlyPer-Crate READMEs (5 new files)
uselesskey-core,uselesskey-rsa,uselesskey-ecdsa,uselesskey-ed25519,uselesskey-x509— each with purpose, quick example, and license notice.aws-lc-rs NASM Handling
build.rsdetects NASM availability and setscfg(has_nasm)cfg(has_nasm)— graceful skip without NASMdefaultfeatures changed from["native"]to[](NASM required explicitly)apt-getin both CI jobsCHANGELOG & Documentation
RsaSpec/AwsLcRsRsaKeyPairExt(was referencing non-existent Ed25519 API names)CI & Tooling
--features uk-allfor full coveragextask/src/plan.rs: fixed BDD feature-file detection (was&&instead of||), added 3 tests for facade/BDD/Windows path triggersuselesskey-bdd/features/*.featurepathsIntegration Test Infrastructure
tests/Cargo.toml: addedkey-rotationandcross-adapterfeature gates withuselesskey-rustcrypto,rsa,p256,ed25519-dalek,sha2deps[[test]]entries forkey_rotationandcross_adaptertests/e2e_workflows.rsTest plan
cargo xtask test— all workspace tests passcargo xtask clippy— no warningscargo xtask fmt— no formatting issuescargo xtask bdd— BDD tests pass withuk-allfeaturescargo xtask feature-matrix— all feature combinations compilecargo test -p uselesskey-integration-tests --features key-rotation— key rotation testscargo test -p uselesskey-integration-tests --features cross-adapter— cross-adapter testscargo test -p uselesskey-x509 --test x509_unit— X.509 unit tests with SAN coverage