chore: scaffold adapter B (PGP-native)#301
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (9)
WalkthroughA new OpenPGP native adapter crate Changes
Sequence DiagramsequenceDiagram
actor User
participant FactoryExt
participant PgpKeyPair
participant PgpNativeExt
participant pgp Crate
User->>FactoryExt: fx.pgp(PgpSpec::rsa_3072())
FactoryExt-->>PgpKeyPair: returns PgpKeyPair
User->>PgpNativeExt: keypair.secret_key_armor()
PgpNativeExt->>pgp Crate: parse_armor(secret_bytes)
pgp Crate-->>PgpNativeExt: SignedSecretKey
PgpNativeExt-->>User: SignedSecretKey
User->>PgpNativeExt: keypair.public_key_armor()
PgpNativeExt->>pgp Crate: parse_armor(public_bytes)
pgp Crate-->>PgpNativeExt: SignedPublicKey
PgpNativeExt-->>User: SignedPublicKey
User->>User: verify fingerprints match
Estimated Code Review Effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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, 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 establishes a new Highlights
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. Footnotes
|
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/uselesskey-pgp-native/examples/pgp_native.rs`:
- Around line 1-15: CI failed rustfmt due to import ordering and println
formatting; reorder the use statements so external crates are sorted (e.g.,
bring use pgp::types::KeyDetails before the uselesskey_* imports or follow your
repo's import-group convention) and ensure a blank line between external and
local imports, run cargo fmt to apply exact formatting, and adjust the two
println! lines in main (keeping fingerprint() comparisons but matching rustfmt's
preferred spacing/line breaks) so the file is formatted cleanly and ends with a
newline.
In `@crates/uselesskey-pgp-native/src/lib.rs`:
- Around line 27-49: The current PgpNativeExt impl panics on parse failures via
expect(...) in secret_key, public_key, secret_key_armor, and public_key_armor;
change the trait signatures for those methods to return a fallible Result (e.g.,
Result<SignedSecretKey, YourError> / Result<SignedPublicKey, YourError>) and
update the uselesskey_pgp::PgpKeyPair implementation to propagate parsing errors
instead of calling expect — call SignedSecretKey::from_bytes(...)? /
SignedPublicKey::from_bytes(...)? and SignedSecretKey::from_armor_single(...)? /
SignedPublicKey::from_armor_single(...)? and map/convert the returned error into
the chosen error type; keep the existing panic-throwing helpers only as separate
opt-in methods if desired.
- Around line 59-67: Replace the ad-hoc randomized #[test]
parse_round_trip_binary_and_armor with a property-based proptest that generates
the Factory (replace Factory::random() with a proptest strategy for the Factory)
and use rstest to parameterize the four key variants; specifically, convert the
test function parse_round_trip_binary_and_armor to take a generated Factory and
a parameter enum/flag for which accessor to call (secret_key, secret_key_armor,
public_key, public_key_armor) and assert the invariant keypair.fingerprint() ==
<accessor>().fingerprint().to_string() for each case; keep references to the
existing symbols Factory, parse_round_trip_binary_and_armor, keypair,
fingerprint(), secret_key(), secret_key_armor(), public_key(), and
public_key_armor() so the new test uses proptest strategies for randomness and
rstest for the four parameterized variants.
- Around line 64-67: The four assert_eq! lines using keypair.fingerprint() vs
secret_key()/secret_key_armor()/public_key()/public_key_armor() are misformatted
and failing rustfmt; reformat them to match project rustfmt style (or simply run
`cargo fmt`) so each assertion conforms to the repository's formatting rules,
ensuring calls to keypair.fingerprint(),
keypair.secret_key().fingerprint().to_string(),
keypair.secret_key_armor().fingerprint().to_string(),
keypair.public_key().fingerprint().to_string(), and
keypair.public_key_armor().fingerprint().to_string() are written in the
canonical formatted style.
In `@crates/uselesskey-pgp-native/tests/integration.rs`:
- Around line 1-21: The test file integration_parses_armored_vs_binary has
formatting issues—reorder imports to satisfy rustfmt (group std/lib crates then
local crates: pgp::types::KeyDetails together with external crates, then
uselesskey_*), and run cargo fmt to fix trailing/line-length formatting; also
reflow the assert_eq! calls in function integration_parses_armored_vs_binary so
each assertion fits rustfmt line-length rules (e.g., keep left and right args on
the same line or break after comma consistently) and ensure import ordering
matches rustfmt's default rules so cargo fmt passes.
In `@README.md`:
- Around line 180-187: Add a blank line before the fenced code block that
follows the "**pgp-native adapter**" heading to satisfy Markdown linting: locate
the "**pgp-native adapter**" heading and the subsequent "```toml" fence and
insert a single empty line between them so the code block is separated from the
preceding paragraph/heading.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c7062e33-e614-423b-87ef-cebb774351e6
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
Cargo.tomlREADME.mdcrates/uselesskey-pgp-native/Cargo.tomlcrates/uselesskey-pgp-native/README.mdcrates/uselesskey-pgp-native/examples/pgp_native.rscrates/uselesskey-pgp-native/src/lib.rscrates/uselesskey-pgp-native/tests/integration.rsdocs/metadata/workspace-docs.jsonxtask/src/main.rs
| impl PgpNativeExt for uselesskey_pgp::PgpKeyPair { | ||
| fn secret_key(&self) -> SignedSecretKey { | ||
| SignedSecretKey::from_bytes(Cursor::new(self.private_key_binary())) | ||
| .expect("failed to parse uselesskey PGP private key bytes") | ||
| } | ||
|
|
||
| fn public_key(&self) -> SignedPublicKey { | ||
| SignedPublicKey::from_bytes(Cursor::new(self.public_key_binary())) | ||
| .expect("failed to parse uselesskey PGP public key bytes") | ||
| } | ||
|
|
||
| fn secret_key_armor(&self) -> SignedSecretKey { | ||
| let (key, _) = SignedSecretKey::from_armor_single(Cursor::new(self.private_key_armored())) | ||
| .expect("failed to parse armored uselesskey PGP private key"); | ||
| key | ||
| } | ||
|
|
||
| fn public_key_armor(&self) -> SignedPublicKey { | ||
| let (key, _) = SignedPublicKey::from_armor_single(Cursor::new(self.public_key_armored())) | ||
| .expect("failed to parse armored uselesskey PGP public key"); | ||
| key | ||
| } | ||
| } |
There was a problem hiding this comment.
Avoid panics in the public conversion surface.
Lines 29-30, 34-35, 39-40, and 45-46 use expect(...), which can abort consumers on parse failure. Expose fallible methods (Result) for this API and keep panic wrappers only if explicitly opt-in.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/uselesskey-pgp-native/src/lib.rs` around lines 27 - 49, The current
PgpNativeExt impl panics on parse failures via expect(...) in secret_key,
public_key, secret_key_armor, and public_key_armor; change the trait signatures
for those methods to return a fallible Result (e.g., Result<SignedSecretKey,
YourError> / Result<SignedPublicKey, YourError>) and update the
uselesskey_pgp::PgpKeyPair implementation to propagate parsing errors instead of
calling expect — call SignedSecretKey::from_bytes(...)? /
SignedPublicKey::from_bytes(...)? and SignedSecretKey::from_armor_single(...)? /
SignedPublicKey::from_armor_single(...)? and map/convert the returned error into
the chosen error type; keep the existing panic-throwing helpers only as separate
opt-in methods if desired.
| #[test] | ||
| fn parse_round_trip_binary_and_armor() { | ||
| let fx = Factory::random(); | ||
| let keypair = fx.pgp("fixture", PgpSpec::ed25519()); | ||
|
|
||
| assert_eq!(keypair.fingerprint(), keypair.secret_key().fingerprint().to_string()); | ||
| assert_eq!(keypair.fingerprint(), keypair.secret_key_armor().fingerprint().to_string()); | ||
| assert_eq!(keypair.fingerprint(), keypair.public_key().fingerprint().to_string()); | ||
| assert_eq!(keypair.fingerprint(), keypair.public_key_armor().fingerprint().to_string()); |
There was a problem hiding this comment.
Use proptest + rstest for this randomized, repeated test pattern.
Line 60 introduces random input, and Lines 64-67 duplicate the same invariant across variants. Please convert this to a property-based test (proptest) with parameterized cases (rstest) to match repository test policy.
As per coding guidelines, "Use proptest crate for property-based tests" and "Use rstest crate for parameterized tests".
🧰 Tools
🪛 GitHub Actions: CI
[error] 61-61: cargo fmt --all -- --check failed (code formatting differences detected). Diff shows assert_eq! statements reformatted across multiple lines.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/uselesskey-pgp-native/src/lib.rs` around lines 59 - 67, Replace the
ad-hoc randomized #[test] parse_round_trip_binary_and_armor with a
property-based proptest that generates the Factory (replace Factory::random()
with a proptest strategy for the Factory) and use rstest to parameterize the
four key variants; specifically, convert the test function
parse_round_trip_binary_and_armor to take a generated Factory and a parameter
enum/flag for which accessor to call (secret_key, secret_key_armor, public_key,
public_key_armor) and assert the invariant keypair.fingerprint() ==
<accessor>().fingerprint().to_string() for each case; keep references to the
existing symbols Factory, parse_round_trip_binary_and_armor, keypair,
fingerprint(), secret_key(), secret_key_armor(), public_key(), and
public_key_armor() so the new test uses proptest strategies for randomness and
rstest for the four parameterized variants.
There was a problem hiding this comment.
Code Review
This pull request introduces a new uselesskey-pgp-native crate to the workspace, providing adapters to convert uselesskey-pgp generated fixtures into native pgp crate types such as SignedSecretKey and SignedPublicKey. The new crate includes its own Cargo.toml, README.md, an example, core lib.rs with the PgpNativeExt trait, and integration tests, with all relevant workspace configuration files updated. Feedback from the review suggests replacing the use of .expect() with Result types in the PgpNativeExt implementation for more robust error handling, particularly for negative fixtures. Additionally, minor documentation improvements are recommended for the README.md files, and several unused imports (KeyDetails and Deserializable) should be removed across the new crate's source, example, and test files for better code hygiene.
| impl PgpNativeExt for uselesskey_pgp::PgpKeyPair { | ||
| fn secret_key(&self) -> SignedSecretKey { | ||
| SignedSecretKey::from_bytes(Cursor::new(self.private_key_binary())) | ||
| .expect("failed to parse uselesskey PGP private key bytes") | ||
| } | ||
|
|
||
| fn public_key(&self) -> SignedPublicKey { | ||
| SignedPublicKey::from_bytes(Cursor::new(self.public_key_binary())) | ||
| .expect("failed to parse uselesskey PGP public key bytes") | ||
| } | ||
|
|
||
| fn secret_key_armor(&self) -> SignedSecretKey { | ||
| let (key, _) = SignedSecretKey::from_armor_single(Cursor::new(self.private_key_armored())) | ||
| .expect("failed to parse armored uselesskey PGP private key"); | ||
| key | ||
| } | ||
|
|
||
| fn public_key_armor(&self) -> SignedPublicKey { | ||
| let (key, _) = SignedPublicKey::from_armor_single(Cursor::new(self.public_key_armored())) | ||
| .expect("failed to parse armored uselesskey PGP public key"); | ||
| key | ||
| } | ||
| } |
There was a problem hiding this comment.
Using .expect() in a library can lead to panics, which is generally undesirable. It's better to return a Result to allow the caller to handle potential parsing errors gracefully. This is particularly relevant for uselesskey as it supports generating negative fixtures (e.g., corrupt keys) for testing error handling.
I recommend modifying the PgpNativeExt trait and its implementation to return pgp::errors::Result<T>.
For example:
use pgp::errors::Result as PgpResult;
pub trait PgpNativeExt {
fn secret_key(&self) -> PgpResult<SignedSecretKey>;
// ... other methods
}
impl PgpNativeExt for uselesskey_pgp::PgpKeyPair {
fn secret_key(&self) -> PgpResult<SignedSecretKey> {
SignedSecretKey::from_bytes(Cursor::new(self.private_key_binary()))
}
fn secret_key_armor(&self) -> PgpResult<SignedSecretKey> {
SignedSecretKey::from_armor_single(Cursor::new(self.private_key_armored()))
.map(|(key, _)| key)
}
// ... other methods
}This change will require updating tests and examples to handle the Result, likely by using .unwrap() in those contexts.
| uselesskey-jsonwebtoken = { version = "0.4.1" } | ||
| ``` | ||
|
|
||
|
|
There was a problem hiding this comment.
This extra blank line can be removed for more concise formatting.
| - one smoke test that parses binary and armored fixture values | ||
| - one integration-style assertion test for deterministic conversion in an external path |
There was a problem hiding this comment.
This "Tests" section reads like a developer's to-do list. For a crate's README, it would be better to either remove these informal notes or rephrase them to describe the testing strategy to users. For example:
"This crate includes tests to ensure correct parsing of both binary and armored key formats, and to verify deterministic conversion."
| use uselesskey_core::Factory; | ||
| use uselesskey_pgp::{PgpFactoryExt, PgpSpec}; | ||
| use uselesskey_pgp_native::PgpNativeExt; | ||
| use pgp::types::KeyDetails; |
There was a problem hiding this comment.
This import of KeyDetails is unused and can be removed.
|
|
||
| use std::io::Cursor; | ||
|
|
||
| use pgp::composed::{Deserializable, SignedPublicKey, SignedSecretKey}; |
There was a problem hiding this comment.
The Deserializable trait is imported but not used in this file. It can be removed to clean up the imports.
| use pgp::composed::{Deserializable, SignedPublicKey, SignedSecretKey}; | |
| use pgp::composed::{SignedPublicKey, SignedSecretKey}; |
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use pgp::types::KeyDetails; |
There was a problem hiding this comment.
This import of KeyDetails is unused within the tests module and can be removed.
| @@ -0,0 +1,21 @@ | |||
| use uselesskey_core::Factory; | |||
| use uselesskey_pgp::{PgpFactoryExt, PgpSpec}; | |||
| use pgp::types::KeyDetails; | |||
There was a problem hiding this comment.
This import of KeyDetails is unused and can be removed.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a6b63b019f
ℹ️ 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".
| fn public_key_armor(&self) -> SignedPublicKey; | ||
| } | ||
|
|
||
| impl PgpNativeExt for uselesskey_pgp::PgpKeyPair { |
There was a problem hiding this comment.
Gate PgpKeyPair impl behind pgp-native feature
uselesskey-pgp is declared optional, but this impl is compiled unconditionally, so cargo check -p uselesskey-pgp-native --no-default-features fails with an unresolved uselesskey_pgp crate. That breaks consumers who disable default features (a common workspace policy) and makes the crate’s feature contract inconsistent. Either make uselesskey-pgp non-optional or add #[cfg(feature = "pgp-native")] gating so no-default builds still compile.
Useful? React with 👍 / 👎.
18005e6 to
ba3af36
Compare
ba3af36 to
b2233fc
Compare
No description provided.