feat(facade): make defaults lightweight and add consumer-shaped smoke coverage#241
Conversation
Summary by CodeRabbitRelease Notes
WalkthroughThis PR introduces text-based seed constructors ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 refactors the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request refactors the uselesskey facade to be lightweight by default, changing the default features from ["rsa"] to []. This is a significant, intentional breaking change. To improve ergonomics, new helpers Seed::from_text and Factory::deterministic_from_str are introduced. The changes are supported by extensive updates to documentation and a robust testing strategy, including new governance tests that verify the dependency graph of a token-only consumer. My feedback includes a suggestion to optimize a test helper function in tests/governance.rs to reduce string allocations.
| fn reachable_package_names(metadata: &Metadata, root_id: &str) -> BTreeSet<String> { | ||
| let node_map = metadata | ||
| .resolve | ||
| .nodes | ||
| .iter() | ||
| .map(|node| (node.id.as_str(), node.dependencies.as_slice())) | ||
| .collect::<std::collections::BTreeMap<_, _>>(); | ||
| let package_names = metadata | ||
| .packages | ||
| .iter() | ||
| .map(|pkg| (pkg.id.as_str(), pkg.name.as_str())) | ||
| .collect::<std::collections::BTreeMap<_, _>>(); | ||
|
|
||
| let mut seen = BTreeSet::new(); | ||
| let mut stack = vec![root_id.to_string()]; | ||
|
|
||
| while let Some(id) = stack.pop() { | ||
| if !seen.insert(id.clone()) { | ||
| continue; | ||
| } | ||
|
|
||
| if let Some(deps) = node_map.get(id.as_str()) { | ||
| stack.extend(deps.iter().map(|dep| dep.to_string())); | ||
| } | ||
| } | ||
|
|
||
| seen.into_iter() | ||
| .filter_map(|id| { | ||
| package_names | ||
| .get(id.as_str()) | ||
| .map(|name| (*name).to_string()) | ||
| }) | ||
| .collect() | ||
| } |
There was a problem hiding this comment.
The implementation of reachable_package_names involves several string allocations within the graph traversal loop (e.g., id.clone(), dep.to_string()). While performance is not critical for this test helper, you can make it more efficient by working with string slices (&str) for the traversal, reducing unnecessary allocations.
fn reachable_package_names(metadata: &Metadata, root_id: &str) -> BTreeSet<String> {
let node_map = metadata
.resolve
.nodes
.iter()
.map(|node| (node.id.as_str(), node.dependencies.as_slice()))
.collect::<std::collections::BTreeMap<_, _>>();
let package_names = metadata
.packages
.iter()
.map(|pkg| (pkg.id.as_str(), pkg.name.as_str()))
.collect::<std::collections::BTreeMap<_, _>>();
let mut seen = BTreeSet::new();
let mut stack = vec![root_id];
while let Some(id) = stack.pop() {
if !seen.insert(id) {
continue;
}
if let Some(deps) = node_map.get(id) {
stack.extend(deps.iter().map(String::as_str));
}
}
seen.into_iter()
.filter_map(|id| package_names.get(id).map(|&name| name.to_string()))
.collect()
}There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/fixtures/token_only_facade/src/lib.rs`:
- Line 1: This crate root is missing the crate-level safety attribute; add the
directive #![forbid(unsafe_code)] at the top of src/lib.rs (before any items or
cfg attributes like #[cfg(test)]) so the crate enforces the repo-wide ban on
unsafe code; ensure the attribute is placed at the very top of the file so it
applies to tests and all modules.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 01a8e3e4-4711-4f9e-a8b5-f197d19118bc
⛔ Files ignored due to path filters (1)
tests/snapshots/api_surface__api_shape_snapshots__core_types_shape.snapis excluded by!**/*.snap
📒 Files selected for processing (17)
crates/uselesskey-core-seed/src/lib.rscrates/uselesskey-core/src/factory.rscrates/uselesskey-core/tests/factory_text_seed.rscrates/uselesskey/Cargo.tomlcrates/uselesskey/README.mdcrates/uselesskey/src/lib.rscrates/uselesskey/tests/facade.rscrates/uselesskey/tests/feature_combinations.rscrates/uselesskey/tests/feature_isolation.rstests/Cargo.tomltests/api_surface.rstests/builder_patterns.rstests/fixtures/token_only_facade/.gitignoretests/fixtures/token_only_facade/Cargo.tomltests/fixtures/token_only_facade/src/lib.rstests/governance.rstests/tests/published_facade_smoke.rs
| @@ -0,0 +1,11 @@ | |||
| #[cfg(test)] | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add crate-level #![forbid(unsafe_code)].
This new fixture crate root skips the repo-wide unsafe ban, so it does not enforce the same safety baseline as the other crates.
Proposed fix
+#![forbid(unsafe_code)]
+
#[cfg(test)]
mod tests {As per coding guidelines, "All crates must use #![forbid(unsafe_code)] at the crate root".
📝 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(test)] | |
| #![forbid(unsafe_code)] | |
| #[cfg(test)] | |
| mod tests { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/fixtures/token_only_facade/src/lib.rs` at line 1, This crate root is
missing the crate-level safety attribute; add the directive
#![forbid(unsafe_code)] at the top of src/lib.rs (before any items or cfg
attributes like #[cfg(test)]) so the crate enforces the repo-wide ban on unsafe
code; ensure the attribute is placed at the very top of the file so it applies
to tests and all modules.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 48ba95dc86
ℹ️ 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".
| ```toml | ||
| [dev-dependencies] | ||
| uselesskey = "0.2" | ||
| uselesskey = { features = ["rsa"] } |
There was a problem hiding this comment.
Add version to README dependency snippets
The new Cargo.toml examples omit version/path for uselesskey (e.g. uselesskey = { features = ["rsa"] }), which makes the snippets invalid for downstream users copying them into a normal project: Cargo rejects dependency specs that provide only feature flags. This regresses the quick-start path introduced in this commit, so the examples should include an explicit version (or another valid source such as path/workspace = true).
Useful? React with 👍 / 👎.
Summary
This changes the
uselesskeyfacade to be lightweight by default and adds narrow consumer-shaped smoke coverage around that surface.Public surface changes
crates/uselesskeydefault features from["rsa"]to[]Seed::from_text(&str)Factory::deterministic_from_str(&str)Why
The token path was already lightweight. The real issue was facade ergonomics:
uselesskeyimplied RSAThis keeps token APIs feature-gated, removes the RSA-shaped default path, and adds a cleaner deterministic text-seed helper.
Guards
default-features = falsefeatures = ["token"]uselesskey-rsarsaDogfooding
published_facade_smokeintegration testTesting
Release note
This is a consumer-visible feature-surface change and should ship as the next minor pre-1.0 release, not a patch release.