refactor: replace string registry with enum and update related logic#13
Merged
Bechma merged 1 commit intocyberfabric:mainfrom Mar 17, 2026
Merged
refactor: replace string registry with enum and update related logic#13Bechma merged 1 commit intocyberfabric:mainfrom
Bechma merged 1 commit intocyberfabric:mainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a typed Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as "cf-cli (List/Docs)"
participant Resolver
participant Cache
participant Remote as "Registry HTTP"
User->>CLI: run list/docs --registry <registry>
CLI->>Resolver: resolve with Registry
Resolver->>Cache: check registry cache (registry-specific path)
alt cache hit
Cache-->>Resolver: return cached crate/source
else cache miss
Resolver->>Remote: fetch metadata & module.rs from https://{registry}/...
Remote-->>Resolver: metadata + module.rs
Resolver->>Cache: write registry-scoped cache
end
Resolver-->>CLI: resolved module(s)/docs
CLI-->>User: display results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
README.md (1)
72-72: Clarify tool name:cargofmtvsrustfmt.The documentation mentions
cargofmt, but the actual tool being installed/checked isrustfmt(a rustup component). Consider usingrustfmthere to match what users will see installed, or clarify thatcargofmtis the CLI argument name that installsrustfmt.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 72, Update the README wording so the tool name matches what's actually installed: replace or clarify the `cargofmt` mention with `rustfmt` (or explicitly state that the CLI flag `cargofmt` installs the `rustfmt` rustup component). Ensure the sentence referencing `tools` and the install/upgrade list mentions `rustfmt` so users see the same tool name that will be installed, or add a parenthetical note linking `cargofmt` -> `rustfmt`.crates/cli/src/docs/mod.rs (1)
304-310: Consider updating the hardcoded error message.The error message on line 307 specifically mentions "crates.io registry" while the function now accepts a generic
Registryparameter. Consider using the registry's display representation for consistency:Proposed fix
} else { fetch_exact_crates_io_candidate(client, registry, crate_name) .await? .with_context(|| { - format!("could not resolve package '{crate_name}' from the crates.io registry") + format!("could not resolve package '{crate_name}' from the {registry} registry") })? .max_version };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/docs/mod.rs` around lines 304 - 310, The error message is hardcoded to "crates.io registry" but the call to fetch_exact_crates_io_candidate(client, registry, crate_name) accepts a generic Registry; update the with_context closure to include the registry's display instead of the fixed string by formatting the message with crate_name and the registry (e.g., use registry's Display/ToString) so the context reads like "could not resolve package '{crate_name}' from registry '{registry}'"; locate the call to fetch_exact_crates_io_candidate and modify its with_context formatting accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 6-9: The CI workflow's paths filter currently only includes
Rust/TOML/lockfile patterns under the YAML key paths, so changes to workflow
files under .github/workflows can skip running this pipeline; update the paths
list in the ci.yml workflow (the YAML key "paths" and its current glob entries)
to also include workflow file globs (e.g., add a pattern matching
.github/workflows/**/*.yml or .github/**) or remove the "paths" restriction
entirely so edits to workflow files will trigger the pipeline.
In `@crates/cli/src/config/modules/list.rs`:
- Around line 31-32: The registry field on the command struct (registry:
Registry) is unused in run(); either remove the field or wire it up consistently
with docs/mod.rs: update run() to accept and pass the selected Registry into the
metadata fetcher/resolver instead of calling fetch_all_crates_io_metadata() with
no args — e.g., replace the direct call to fetch_all_crates_io_metadata() with a
call that accepts the Registry or call the resolver used in docs (passing
registry) and also pass registry into clean_registry_cache()/fetch helper;
alternatively remove the unused registry field and its arg attribute if
multi-registry support is not intended.
---
Nitpick comments:
In `@crates/cli/src/docs/mod.rs`:
- Around line 304-310: The error message is hardcoded to "crates.io registry"
but the call to fetch_exact_crates_io_candidate(client, registry, crate_name)
accepts a generic Registry; update the with_context closure to include the
registry's display instead of the fixed string by formatting the message with
crate_name and the registry (e.g., use registry's Display/ToString) so the
context reads like "could not resolve package '{crate_name}' from registry
'{registry}'"; locate the call to fetch_exact_crates_io_candidate and modify its
with_context formatting accordingly.
In `@README.md`:
- Line 72: Update the README wording so the tool name matches what's actually
installed: replace or clarify the `cargofmt` mention with `rustfmt` (or
explicitly state that the CLI flag `cargofmt` installs the `rustfmt` rustup
component). Ensure the sentence referencing `tools` and the install/upgrade list
mentions `rustfmt` so users see the same tool name that will be installed, or
add a parenthetical note linking `cargofmt` -> `rustfmt`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c33f79bf-e5ae-430a-bf57-9c56ffc104c0
📒 Files selected for processing (7)
.github/workflows/ci.ymlAGENTS.mdREADME.mdcrates/cli/src/common.rscrates/cli/src/config/modules/list.rscrates/cli/src/docs/mod.rscrates/cli/src/tools/mod.rs
Bechma
force-pushed
the
add-documentation
branch
from
9f24787 to
6693bd6
Compare
Contributor
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 `@crates/cli/src/tools/mod.rs`:
- Around line 111-123: The prereq check currently uses
is_installed(tool.check_binary()) and then immediately bails for
InstallMethod::Prerequisite, which misses the ensure_rustup fallback
(~/.cargo/bin/rustup) used elsewhere; update the logic so that before bailing
for InstallMethod::Prerequisite you call or leverage ensure_rustup() (or extend
is_installed to check the same fallback path) to verify rustup via the fallback
location and only bail if both PATH and the ensure_rustup fallback fail—refer to
is_installed, tool.check_binary(), ensure_rustup, and
InstallMethod::Prerequisite to locate and change the check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d3e6b9b8-3730-4221-af31-77d9bfae6997
📒 Files selected for processing (7)
.github/workflows/ci.ymlAGENTS.mdREADME.mdcrates/cli/src/common.rscrates/cli/src/config/modules/list.rscrates/cli/src/docs/mod.rscrates/cli/src/tools/mod.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- README.md
- .github/workflows/ci.yml
Bechma
force-pushed
the
add-documentation
branch
from
6693bd6 to
b0c0cfc
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/cli/src/tools/mod.rs (1)
191-195:⚠️ Potential issue | 🟠 MajorPATH-only recheck breaks fresh rustup installs.
install_rustup()runs in a child process, so a successful install cannot update this process's PATH. On hosts where~/.cargo/binwas not already exported, this branch bails immediately after install and the laterrustupinvocations never run. Reuse a resolved rustup path here instead of re-checking PATH only.#!/bin/bash set -euo pipefail # Verify that rustup is only validated/invoked via PATH in the current flow. rg -n -C2 'fn ensure_rustup|install_rustup\(|is_installed\("rustup"\)|Command::new\("rustup"\)|cargo_bin_path|\.cargo/bin' crates/cli/src/tools/mod.rs # Expected: the output shows PATH-only checks and direct "rustup" invocations, # with no fallback path reused after auto-install.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/tools/mod.rs` around lines 191 - 195, The current logic bails if is_installed("rustup") fails, which breaks when install_rustup() ran in a child process and updated ~/.cargo/bin only for new shells; instead, after calling install_rustup() or when detecting a successful installation, resolve the rustup binary path (e.g., by checking the concrete path under cargo_bin_path() or using a which/lookup helper) and reuse that resolved path for subsequent rustup invocations rather than re-checking PATH; change the branch that currently calls bail! when is_installed("rustup") is false to accept a resolved rustup_path (from install_rustup or cargo_bin_path()) and only bail if neither a PATH lookup nor a concrete resolved path is available so subsequent calls use the known rustup_path.
🧹 Nitpick comments (1)
crates/cli/src/docs/mod.rs (1)
713-717: Rename the helper to match the new abstraction.
fetch_exact_crates_io_candidatenow accepts a genericRegistry, so the current name reads like a leftover special-case. A registry-neutral name would make the new flow easier to follow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/docs/mod.rs` around lines 713 - 717, The helper function fetch_exact_crates_io_candidate is misleading because it now accepts a generic Registry; rename it (and all its call sites) to a registry-neutral identifier such as fetch_exact_registry_candidate or fetch_exact_candidate to reflect the new abstraction, updating the function declaration, any imports/usages, and documentation/comments that reference fetch_exact_crates_io_candidate so compilation and call semantics remain consistent.
🤖 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/cli/src/config/modules/list.rs`:
- Around line 30-32: Update the field doc comment for the registry argument (the
registry: Registry field with #[arg(long, value_enum, default_value_t =
Registry::CratesIo)]) to clearly state that the --registry flag is only
consulted when both --system and --verbose are enabled (i.e., the --verbose path
alone does not query the registry); reference the Registry enum in the help text
so users know the allowed values and keep the existing default value behavior.
---
Duplicate comments:
In `@crates/cli/src/tools/mod.rs`:
- Around line 191-195: The current logic bails if is_installed("rustup") fails,
which breaks when install_rustup() ran in a child process and updated
~/.cargo/bin only for new shells; instead, after calling install_rustup() or
when detecting a successful installation, resolve the rustup binary path (e.g.,
by checking the concrete path under cargo_bin_path() or using a which/lookup
helper) and reuse that resolved path for subsequent rustup invocations rather
than re-checking PATH; change the branch that currently calls bail! when
is_installed("rustup") is false to accept a resolved rustup_path (from
install_rustup or cargo_bin_path()) and only bail if neither a PATH lookup nor a
concrete resolved path is available so subsequent calls use the known
rustup_path.
---
Nitpick comments:
In `@crates/cli/src/docs/mod.rs`:
- Around line 713-717: The helper function fetch_exact_crates_io_candidate is
misleading because it now accepts a generic Registry; rename it (and all its
call sites) to a registry-neutral identifier such as
fetch_exact_registry_candidate or fetch_exact_candidate to reflect the new
abstraction, updating the function declaration, any imports/usages, and
documentation/comments that reference fetch_exact_crates_io_candidate so
compilation and call semantics remain consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fcd67e83-8202-4008-85fd-3f2bc9594e92
📒 Files selected for processing (7)
.github/workflows/ci.ymlAGENTS.mdREADME.mdcrates/cli/src/common.rscrates/cli/src/config/modules/list.rscrates/cli/src/docs/mod.rscrates/cli/src/tools/mod.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/ci.yml
- README.md
Signed-off-by: Bechma <19294519+Bechma@users.noreply.github.com>
Bechma
force-pushed
the
add-documentation
branch
from
b0c0cfc to
7fc9447
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/cli/src/tools/mod.rs (1)
107-123:⚠️ Potential issue | 🟠 MajorRustup availability check can still fail right after auto-install.
Line 111 only checks PATH. If
ensure_rustupinstalls rustup under cargo’s bin path but PATH is not refreshed in-process, Line 119 can still bail incorrectly and downstream rustup component commands may fail.🔧 Suggested fix
fn ensure_rustup(yolo: bool) -> anyhow::Result<()> { - if is_installed("rustup") { + if is_rustup_available() { return Ok(()); } @@ println!("Installing rustup..."); install_rustup().context("failed to install rustup")?; + if !is_rustup_available() { + bail!("rustup was installed but is not discoverable in this shell; ensure cargo bin is on PATH"); + } println!("✓ rustup installed"); Ok(()) } +fn is_rustup_available() -> bool { + if is_installed("rustup") { + return true; + } + let exe = if cfg!(windows) { "rustup.exe" } else { "rustup" }; + std::env::var_os("HOME") + .map(std::path::PathBuf::from) + .map(|home| home.join(".cargo").join("bin").join(exe)) + .map(|path| path.to_string_lossy().into_owned()) + .as_deref() + .is_some_and(is_installed) +} @@ - let installed = is_installed(tool.check_binary()); + let installed = match tool { + ToolName::Rustup => is_rustup_available(), + _ => is_installed(tool.check_binary()), + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/tools/mod.rs` around lines 107 - 123, install_tools currently calls ensure_rustup(self.yolo) but then only checks PATH once via is_installed(tool.check_binary()), which can cause a false-negative if rustup was just installed and the process PATH wasn't refreshed; update install_tools to re-check rustup availability after ensure_rustup (e.g., call is_installed("rustup") or re-run the binary check used by is_installed) before bailing in the InstallMethod::Prerequisite branch and before attempting RustupComponent installs (referencing install_tools, ensure_rustup, is_installed, tool.install_method, InstallMethod::Prerequisite, InstallMethod::RustupComponent) so that a freshly-installed rustup is detected and the code only bails when rustup truly cannot be found.
🧹 Nitpick comments (1)
AGENTS.md (1)
3-10: Optional: reduce repeated “Always …” phrasing for readability.Consider converting these lines into a short bullet list with parallel phrasing; it will scan faster.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AGENTS.md` around lines 3 - 10, Refactor the repeated "Always ..." sentences into a concise bullet list with parallel phrasing for readability: replace the four sentences with bullets like "Use cargo clippy", "Format code with cargo fmt and run cargo test when touching Rust code", "Use cargo add instead of editing Cargo.toml", and "Prefer enums over strings when values are fixed"; keep the same guidance but remove the repeated "Always" preface and ensure punctuation and capitalization are consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/cli/src/tools/mod.rs`:
- Around line 107-123: install_tools currently calls ensure_rustup(self.yolo)
but then only checks PATH once via is_installed(tool.check_binary()), which can
cause a false-negative if rustup was just installed and the process PATH wasn't
refreshed; update install_tools to re-check rustup availability after
ensure_rustup (e.g., call is_installed("rustup") or re-run the binary check used
by is_installed) before bailing in the InstallMethod::Prerequisite branch and
before attempting RustupComponent installs (referencing install_tools,
ensure_rustup, is_installed, tool.install_method, InstallMethod::Prerequisite,
InstallMethod::RustupComponent) so that a freshly-installed rustup is detected
and the code only bails when rustup truly cannot be found.
---
Nitpick comments:
In `@AGENTS.md`:
- Around line 3-10: Refactor the repeated "Always ..." sentences into a concise
bullet list with parallel phrasing for readability: replace the four sentences
with bullets like "Use cargo clippy", "Format code with cargo fmt and run cargo
test when touching Rust code", "Use cargo add instead of editing Cargo.toml",
and "Prefer enums over strings when values are fixed"; keep the same guidance
but remove the repeated "Always" preface and ensure punctuation and
capitalization are consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6d1018c7-a99c-4fda-bbb6-8480222360d3
📒 Files selected for processing (7)
.github/workflows/ci.ymlAGENTS.mdREADME.mdcrates/cli/src/common.rscrates/cli/src/config/modules/list.rscrates/cli/src/docs/mod.rscrates/cli/src/tools/mod.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/ci.yml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Documentation
New Features
Chores