add hard clippy rules and fix init#4
Merged
Bechma merged 5 commits intocyberfabric:mainfrom Feb 25, 2026
Merged
Conversation
Signed-off-by: Bechma <19294519+Bechma@users.noreply.github.com>
Contributor
📝 WalkthroughWalkthroughUpdate workspace dependencies and Clippy lint config; refactor CLI internals (arg rename, const ctor, cargo spawn/current_dir, error formatting, restart handling); add tools management (rustup-aware install/upgrade) and module generation (cargo-generate + toml_edit workspace updates). Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant CLI as CLI
participant OS as OS/Runtime
participant Rustup as rustup
participant CargoGen as cargo-generate
User->>CLI: run `cli tools install/upgrade [names]` (flags: yolo, verbose)
CLI->>CLI: resolve requested tools vs TOOLS catalog
alt unknown tool
CLI->>User: error (list known tools)
else known tools
CLI->>OS: check `is_installed(binary)`
OS-->>CLI: installed? (yes/no)
alt needs rustup/component
CLI->>Rustup: ensure_rustup (may call install_rustup)
Rustup-->>CLI: installed/available
CLI->>Rustup: add components / run prerequisites
Rustup-->>CLI: result
else prerequisite binary
CLI->>OS: run installer/command (with verbose option)
OS-->>CLI: result
end
CLI->>User: report per-tool result
end
User->>CLI: run `cli module create ...` (local/git, subfolder, branch)
CLI->>CLI: compute template path & destination
alt git template
CLI->>CargoGen: generate with `TemplatePath::Git`
else local template
CLI->>CargoGen: generate with local `TemplatePath`
end
CargoGen-->>CLI: generation result
CLI->>CLI: update workspace `Cargo.toml` via `toml_edit` (add members, merge deps)
CLI->>User: print created module path(s)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
crates/cli/src/run/mod.rs (1)
14-14: Optional: alignbr_argsnaming withbuild/mod.rs.
crates/cli/src/build/mod.rsusesbuild_run_argsfor the same embeddedBuildRunArgsfield. Usingbr_argshere is inconsistent — either rename the build module's field to match or restore the full name here for uniformity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/run/mod.rs` at line 14, The field name br_args in the Run module is inconsistent with the build module which uses build_run_args; rename the field to build_run_args (type BuildRunArgs) in the struct that currently declares br_args to align naming across modules, and update all references/usages of br_args within functions/methods in this file (and any callers) to use build_run_args so the symbol BuildRunArgs is consistently exposed as build_run_args like in build/mod.rs.crates/cli/src/run/run_loop.rs (1)
52-55:cargo_dir_cloneis now a move, not a clone — misleading name.After line 52,
cargo_diris consumed; naming the destinationcargo_dir_cloneimplies a.clone()occurred. A simple rename tocargo_dir(move-shadowing the original) avoids the confusion.♻️ Rename for clarity
- let cargo_dir_clone = cargo_dir; - let runner_handle = std::thread::spawn(move || { - cargo_run_loop(&cargo_dir_clone, &signal_rx); - }); + let runner_handle = std::thread::spawn(move || { + cargo_run_loop(&cargo_dir, &signal_rx); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/run/run_loop.rs` around lines 52 - 55, The variable cargo_dir_clone is misleading because it's moved, not cloned; change the moved variable name to cargo_dir (shadowing the outer one) inside the thread spawn to reflect a move instead of implying a clone and update the closure call to cargo_run_loop(&cargo_dir, &signal_rx) so the move uses the new cargo_dir identifier; reference occurrences: the spawned thread block that calls cargo_run_loop, the cargo_dir_clone identifier, and signal_rx/runner_handle to locate and rename.Cargo.toml (1)
36-39: LGTM on the lint config — one advisory onnursery.
pedanticwithpriority = -1is the idiomatic way to enable it below specific overrides (e.g.struct_excessive_bools = "allow"). Thenurserygroup is explicitly marked unstable and its lint set can change betweenrustcreleases, occasionally introducing new warnings on untouched code. Since both are set to"warn", CI won't break — but a future toolchain bump could surface a wave of new warnings.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Cargo.toml` around lines 36 - 39, The nursery lint group is unstable and may introduce unexpected warnings on toolchain bumps; update the workspace.lints.clippy block so nursery uses a lower priority like pedantic (e.g., nursery = { level = "warn", priority = -1 }) or explicitly change its level to "allow" to prevent future waves of warnings—modify the entries for nursery and/or pedantic in the workspace.lints.clippy section (referencing the pedantic, nursery, and struct_excessive_bools keys) to keep CI stable.crates/cli/src/common.rs (1)
213-217: Non-deterministic ordering in generateduse … as _;lines.
HashMap::keys()has no guaranteed iteration order, so each run ofprepare_cargo_server_mainmay produce a differently-ordered block. While the watcher doesn't observe.cyberfabric/src/main.rs, the output is non-reproducible (e.g. diffs will be noisy, hermetic builds differ).♻️ Sort keys before folding
- let dependencies = dependencies.keys().fold(String::new(), |mut acc, name| { - _ = writeln!(acc, "use {name} as _;"); - acc - }); + let mut keys: Vec<&String> = dependencies.keys().collect(); + keys.sort(); + let dependencies = keys.iter().fold(String::new(), |mut acc, name| { + _ = writeln!(acc, "use {name} as _;"); + acc + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/common.rs` around lines 213 - 217, The generated `use {name} as _;` lines are non-deterministic because dependencies.keys() iterates HashMap keys in arbitrary order; in prepare_cargo_server_main, collect the keys into a Vec, sort that Vec (e.g., ascending), then fold/iterate over the sorted keys to build the dependencies string so output is deterministic (refer to the dependencies variable construction where keys().fold(...) is used).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Cargo.toml`:
- Around line 36-39: The nursery lint group is unstable and may introduce
unexpected warnings on toolchain bumps; update the workspace.lints.clippy block
so nursery uses a lower priority like pedantic (e.g., nursery = { level =
"warn", priority = -1 }) or explicitly change its level to "allow" to prevent
future waves of warnings—modify the entries for nursery and/or pedantic in the
workspace.lints.clippy section (referencing the pedantic, nursery, and
struct_excessive_bools keys) to keep CI stable.
In `@crates/cli/src/common.rs`:
- Around line 213-217: The generated `use {name} as _;` lines are
non-deterministic because dependencies.keys() iterates HashMap keys in arbitrary
order; in prepare_cargo_server_main, collect the keys into a Vec, sort that Vec
(e.g., ascending), then fold/iterate over the sorted keys to build the
dependencies string so output is deterministic (refer to the dependencies
variable construction where keys().fold(...) is used).
In `@crates/cli/src/run/mod.rs`:
- Line 14: The field name br_args in the Run module is inconsistent with the
build module which uses build_run_args; rename the field to build_run_args (type
BuildRunArgs) in the struct that currently declares br_args to align naming
across modules, and update all references/usages of br_args within
functions/methods in this file (and any callers) to use build_run_args so the
symbol BuildRunArgs is consistently exposed as build_run_args like in
build/mod.rs.
In `@crates/cli/src/run/run_loop.rs`:
- Around line 52-55: The variable cargo_dir_clone is misleading because it's
moved, not cloned; change the moved variable name to cargo_dir (shadowing the
outer one) inside the thread spawn to reflect a move instead of implying a clone
and update the closure call to cargo_run_loop(&cargo_dir, &signal_rx) so the
move uses the new cargo_dir identifier; reference occurrences: the spawned
thread block that calls cargo_run_loop, the cargo_dir_clone identifier, and
signal_rx/runner_handle to locate and rename.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
Cargo.tomlcrates/cli/src/common.rscrates/cli/src/run/mod.rscrates/cli/src/run/run_loop.rscrates/cli/src/setup/init.rscrates/cli/src/setup/mod.rscrates/cli/src/setup/tools.rs
Signed-off-by: Bechma <19294519+Bechma@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/cli/src/setup/tools.rs (1)
175-187: Use thehomecrate for getting the home directory.
std::env::home_dir()is deprecated (though Rust 1.86+ may remove the deprecation warning). The canonical replacement is thehomecrate, maintained by the Cargo team and used by Cargo and Rustup themselves. Addhometo dependencies and usehome::home_dir(), or alternatively access$HOME/$USERPROFILEdirectly viastd::env::var_os().♻️ Suggested replacement
+ let home = home::home_dir().unwrap_or_default();Requires adding to
Cargo.toml:home = { workspace = true } # Add to [workspace.dependencies] in root Cargo.toml🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/setup/tools.rs` around lines 175 - 187, The function cargo_bin_path uses deprecated std::env::home_dir(); replace it with the `home` crate: add home = { workspace = true } to workspace dependencies and change the call to home::home_dir().unwrap_or_default() in cargo_bin_path, leaving the existing logic that builds bin (using cfg!(target_family = "windows") to append ".exe") and the subsequent .join(".cargo").join("bin").join(bin).to_string_lossy().into_owned() intact; this preserves behavior while removing the deprecation.
🤖 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/setup/tools.rs`:
- Around line 65-85: resolve_tools currently returns all entries from TOOLS
whenever self.install is None, which ignores the --all flag; update
resolve_tools to explicitly check the --all flag (e.g. a field like self.all)
and only return TOOLS.iter().collect() when that flag is true, otherwise return
an empty Vec or a user-facing error instructing to pass --install or --all;
ensure the code path that handles names (self.install) remains unchanged and
keep the with_context logic for unknown names, or alternatively remove/adjust
the --all flag/help text to reflect the new behavior if you prefer to default to
installing everything.
- Around line 156-173: ensure_rustup currently auto-installs rustup
unconditionally; update it to respect the confirmation flow by checking the
existing confirmation flag (e.g., yolo) or accepting a confirmation parameter.
If yolo is false, do not call install_rustup(); instead prompt the user for
consent (via the existing prompt/confirm helper) or return an error instructing
them to re-run with --yolo. Change the signature of ensure_rustup (or read the
shared confirmation state) so the function can access yolo, and only call
install_rustup() when consent is granted or yolo == true; otherwise bail with a
clear message.
---
Nitpick comments:
In `@crates/cli/src/setup/tools.rs`:
- Around line 175-187: The function cargo_bin_path uses deprecated
std::env::home_dir(); replace it with the `home` crate: add home = { workspace =
true } to workspace dependencies and change the call to
home::home_dir().unwrap_or_default() in cargo_bin_path, leaving the existing
logic that builds bin (using cfg!(target_family = "windows") to append ".exe")
and the subsequent
.join(".cargo").join("bin").join(bin).to_string_lossy().into_owned() intact;
this preserves behavior while removing the deprecation.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/cli/src/setup/module.rs (2)
36-52: Consider checking if the module already exists before generation.If a user runs
module create footwice,cargo_generatewill fail (sinceoverwritedefaults tofalse), but the error message may be cryptic. A pre-check would give a friendlier message:Proposed pre-existence check
if !modules_dir.exists() { bail!( "modules directory does not exist at {}. Make sure you are in a workspace initialized with 'init'.", modules_dir.display() ); } + let target = modules_dir.join(&self.name); + if target.exists() { + bail!("module '{}' already exists at {}", self.name, target.display()); + } + // Generate the main module self.generate_module("Modules", &self.name)?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/setup/module.rs` around lines 36 - 52, Before calling self.generate_module("Modules", &self.name) add a pre-existence check for the target module directory (e.g., compute let module_path = modules_dir.join(&self.name) and check module_path.exists()). If it exists, return an error/bail with a clear, user-friendly message that the module already exists (mention the module name) and suggest removing it or using an overwrite/force option; otherwise proceed to call generate_module as before. This change should be made in the run() function near the modules_dir and generate_module calls.
16-18:no_sdkfield is unused; commented-out code is already stale.The
--no-sdkflag is accepted by the CLI but has no effect since the SDK generation code is entirely commented out. The commented-out code on line 58 also references a 3-parametergenerate_modulesignature that no longer matches the current 2-parameter version, so it can't simply be uncommented.Consider removing both the
no_sdkfield and the commented-out block until the SDK feature is ready, to avoid exposing a no-op flag to users.Also applies to: 54-65
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/setup/module.rs` around lines 16 - 18, The no_sdk flag is defined but unused; remove the unused struct field `no_sdk` from the CLI args in module.rs and delete the stale commented-out SDK generation block that references the old 3-parameter `generate_module` signature so we don't expose a no-op flag; also search for and remove any help text, tests, or code paths that reference `--no-sdk` so the CLI help and parsing no longer advertise the flag, and if/when SDK generation is reintroduced, re-add a properly wired `no_sdk` check against the current `generate_module` (2-parameter) API.
🤖 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/setup/module.rs`:
- Around line 27-29: The CLI `--subfolder` value (self.subfolder) is defined but
never used because generate_module is called with a hardcoded "Modules" string
and that value is used for auto_path and local path construction; update
generate_module usage to use self.subfolder (or remove the redundant parameter
and reference self.subfolder inside generate_module) so the CLI input is
honored, and reconcile the template name casing/plurality (choose either
"Module" or "Modules" and apply it consistently to auto_path and local path
logic); ensure references to auto_path and any local path construction in
generate_module or its callers use the chosen self.subfolder value.
---
Nitpick comments:
In `@crates/cli/src/setup/module.rs`:
- Around line 36-52: Before calling self.generate_module("Modules", &self.name)
add a pre-existence check for the target module directory (e.g., compute let
module_path = modules_dir.join(&self.name) and check module_path.exists()). If
it exists, return an error/bail with a clear, user-friendly message that the
module already exists (mention the module name) and suggest removing it or using
an overwrite/force option; otherwise proceed to call generate_module as before.
This change should be made in the run() function near the modules_dir and
generate_module calls.
- Around line 16-18: The no_sdk flag is defined but unused; remove the unused
struct field `no_sdk` from the CLI args in module.rs and delete the stale
commented-out SDK generation block that references the old 3-parameter
`generate_module` signature so we don't expose a no-op flag; also search for and
remove any help text, tests, or code paths that reference `--no-sdk` so the CLI
help and parsing no longer advertise the flag, and if/when SDK generation is
reintroduced, re-add a properly wired `no_sdk` check against the current
`generate_module` (2-parameter) API.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/setup/module.rs`:
- Around line 16-17: The struct field local_path in ModuleArgs is missing a doc
comment so the --local-path CLI flag is undocumented; add a /// doc comment
above the local_path field describing its purpose (e.g., "Path to a local
module" or similar) so it appears in --help, keeping the existing attribute
#[arg(long = "local-path", conflicts_with_all = ["git", "branch"])] and the
Option<String> type unchanged.
- Around line 54-88: generate_module currently uses self.path as the generate
destination so cargo_generate creates self.path/self.name (workspace root)
instead of placing the new crate inside modules/ computed in run(); thread the
validated modules_dir into generate_module (either by adding a modules_dir
parameter to generate_module or storing it on the struct) and change the
GenerateArgs destination to Some(modules_dir.clone()) (not
modules_dir.join(name)); also update the success message in run() to display
modules_dir.display() as the creation path; reference generate_module, run,
GenerateArgs, destination, and modules_dir when making the change.
- Around line 72-85: The GenerateArgs struct construction in module.rs omits
no_workspace, causing cargo_generate to try updating the workspace manifest;
update the generate(...) call to set no_workspace: true in the GenerateArgs
literal (alongside existing fields like template_path, destination, name, quiet,
verbose) so it matches init.rs behavior and prevents unintended workspace
Cargo.toml modifications.
- Around line 82-85: The GenerateArgs construction in module.rs sets quiet and
verbose but falls back to GenerateArgs::default(), which leaves
no_workspace=false and allows cargo_generate to modify the workspace Cargo.toml;
update the builder for GenerateArgs used in module.rs to include no_workspace:
true alongside quiet: !self.verbose and verbose: self.verbose so the final args
explicitly disable workspace registration (match the init.rs usage), ensuring
the new module crate is not auto-registered in the workspace.
Signed-off-by: Bechma <19294519+Bechma@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
crates/cli/src/setup/module.rs (1)
47-51:⚠️ Potential issue | 🟡 MinorSuccess message shows the parent directory, not the generated module path.
modules_dir.display()resolves to e.g./workspace/modules, while the module is actually created at/workspace/modules/<name>. The message is therefore misleading.📝 Proposed fix
- println!( - "Module '{}' created at {}", - self.name, - modules_dir.display() - ); + println!( + "Module '{}' created at {}", + self.name, + modules_dir.join(&self.name).display() + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/setup/module.rs` around lines 47 - 51, The println! currently prints modules_dir (parent) instead of the actual created module path, so update the success message to show the real path by joining the module name to modules_dir; e.g., compute the module path (modules_dir.join(&self.name) or reuse an existing module_dir variable) and use module_dir.display() in the println! while still including self.name in the message.
🧹 Nitpick comments (2)
Cargo.toml (1)
38-41:clippy::nurseryis explicitly flagged as unstable — consider cherry-picking instead of enabling the whole group.The
clippy::nurserygroup contains lints which are buggy or need more work. It is not recommended to enable the whole group, but rather cherry-pick lints that are useful for your code base. Enabling it workspace-wide means any Rust toolchain upgrade can introduce new nursery lints that silently start emitting warnings (or break CI ifdeny(warnings)is in effect).clippy::pedanticcarries the same risk at a lower degree since it's stable but contains some very aggressive lints prone to false positives.If the intent is to enforce a strict style, prefer listing individual lints explicitly rather than opting into the whole
nurserycategory.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Cargo.toml` around lines 38 - 41, The Cargo.toml currently enables the entire clippy nursery group under [workspace.lints.clippy] (nursery = { level = "warn", priority = -1 }), which is unstable and may introduce new/broken lints on toolchain updates; instead, remove or disable the nursery group and replace it by listing only the specific clippy lints you want (or keep pedantic if desired) — update the [workspace.lints.clippy] block to drop nursery and add explicit lint keys (e.g., add individual entries for any needed nursery lints) while keeping or adjusting pedantic and struct_excessive_bools as appropriate so CI won't break on future toolchain changes.crates/cli/src/setup/module.rs (1)
36-46:modules_dirandmodules_pathcompute the same path independently.
run()buildsmodules_dir(line 36) for the existence check, thengenerate_module()re-derives the identical path asmodules_path(line 74). Consider passing it as a parameter to avoid the redundantjoin.♻️ Proposed refactor
- self.generate_module()?; + self.generate_module(modules_dir.clone())?;- fn generate_module(&self) -> anyhow::Result<()> { + fn generate_module(&self, modules_path: PathBuf) -> anyhow::Result<()> { // ... - let modules_path = self.path.join("modules"); let module_path = modules_path.join(&self.name);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/setup/module.rs` around lines 36 - 46, In run() the modules_dir is computed and checked for existence, then generate_module() recomputes the same path as modules_path; change generate_module() to accept the precomputed modules_dir (or PathBuf) as an argument (update its signature and all callers) and pass modules_dir from run() so the join isn't performed twice; update references to modules_path inside generate_module() to use the passed parameter (modules_dir) to remove the redundant join.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Cargo.toml`:
- Line 28: The workspace declares toml = "1.0.3" which conflicts with
cargo-generate v0.23.7 that requires toml 0.9.x; resolve by either pinning toml
to a 0.9.x range or updating cargo-generate to a version that supports toml 1.x:
open Cargo.toml and change the toml dependency to ~0.9 (e.g., "0.9") to match
cargo-generate, or update the cargo-generate entry (or
toolchain/dev-dependencies referencing cargo-generate) to a newer release that
accepts toml 1.0; ensure you run cargo update and verify the lockfile no longer
contains both toml versions.
In `@crates/cli/src/setup/module.rs`:
- Around line 93-116: The workspace registration is delayed until after optional
SDK generation, risking a partial-failure that leaves the main crate on disk but
not listed in Cargo.toml; after the main module generate(...) call succeeds,
immediately call self.add_modules_to_workspace with the main module path (the
generated vec containing format!("modules/{}", self.name)) to register it before
attempting the SDK-specific logic (the sdk_template branch that calls
generate(GenerateArgs) and fs::remove_dir_all); keep the SDK generation and
removal as-is but ensure failures there do not prevent the workspace update
already performed for the main module.
---
Duplicate comments:
In `@crates/cli/src/setup/module.rs`:
- Around line 47-51: The println! currently prints modules_dir (parent) instead
of the actual created module path, so update the success message to show the
real path by joining the module name to modules_dir; e.g., compute the module
path (modules_dir.join(&self.name) or reuse an existing module_dir variable) and
use module_dir.display() in the println! while still including self.name in the
message.
---
Nitpick comments:
In `@Cargo.toml`:
- Around line 38-41: The Cargo.toml currently enables the entire clippy nursery
group under [workspace.lints.clippy] (nursery = { level = "warn", priority = -1
}), which is unstable and may introduce new/broken lints on toolchain updates;
instead, remove or disable the nursery group and replace it by listing only the
specific clippy lints you want (or keep pedantic if desired) — update the
[workspace.lints.clippy] block to drop nursery and add explicit lint keys (e.g.,
add individual entries for any needed nursery lints) while keeping or adjusting
pedantic and struct_excessive_bools as appropriate so CI won't break on future
toolchain changes.
In `@crates/cli/src/setup/module.rs`:
- Around line 36-46: In run() the modules_dir is computed and checked for
existence, then generate_module() recomputes the same path as modules_path;
change generate_module() to accept the precomputed modules_dir (or PathBuf) as
an argument (update its signature and all callers) and pass modules_dir from
run() so the join isn't performed twice; update references to modules_path
inside generate_module() to use the passed parameter (modules_dir) to remove the
redundant join.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
Cargo.tomlcrates/cli/Cargo.tomlcrates/cli/src/setup/init.rscrates/cli/src/setup/module.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/cli/src/setup/init.rs
Signed-off-by: Bechma <19294519+Bechma@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/module-parser/src/config.rs (1)
17-17:⚠️ Potential issue | 🟠 Major
clippy::single_char_pattern– use char literals instead of single-char strings.
"-"and"_"should be'-'and'_'to satisfy the hard Clippy rules this PR is adding.♻️ Proposed fix
- let package = package.replace("-", "_"); + let package = package.replace('-', '_');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/module-parser/src/config.rs` at line 17, The code uses single-character string literals in package.replace("-", "_") which triggers clippy::single_char_pattern; change the call to use the char overload by replacing the string literals with char literals so the package variable assignment becomes package.replace('-', '_') (locate the line that assigns to the package variable and update the replace call accordingly).
♻️ Duplicate comments (2)
crates/cli/src/setup/tools.rs (2)
65-85:⚠️ Potential issue | 🟡 Minor
--allis effectively a no-op in tool selection.Line 84 returns all tools whenever
--installis absent, so--alldoes not alter behavior. Either require explicit selection (--all/--install) or update the flag/help semantics to “default: all”.Suggested fix (explicit selection)
fn resolve_tools(&self) -> anyhow::Result<Vec<&'static Tool>> { + if self.all { + return Ok(TOOLS.iter().collect()); + } if let Some(names) = &self.install { let mut tools = Vec::with_capacity(names.len()); for name in names { let tool = TOOLS .iter() .find(|t| t.name == name.as_str()) .with_context(|| { format!( "unknown tool '{}'. known tools: {}", name, TOOLS.iter().map(|t| t.name).collect::<Vec<_>>().join(", ") ) })?; tools.push(tool); } return Ok(tools); } - Ok(TOOLS.iter().collect()) + bail!("no tools selected; use --all or --install") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/setup/tools.rs` around lines 65 - 85, resolve_tools currently ignores an explicit --all flag because it returns all TOOLS when self.install is None; update resolve_tools to honor an explicit selection by first checking for self.install (Vec names) then for a boolean self.all (or require one of them) and only return Ok(TOOLS.iter().collect()) when self.all is true; if neither self.install nor self.all is set, return an error explaining that the caller must pass --install or --all. Ensure you reference resolve_tools, self.install, self.all (or equivalent flag), and TOOLS when making the change so the logic enforces explicit selection.
88-89:⚠️ Potential issue | 🟡 MinorRustup bootstrap bypasses confirmation controls.
Line 161 auto-installs rustup even when
yolois false. This is inconsistent with the rest of the prompt/consent flow and can surprise users.Suggested fix (thread `yolo` into rustup bootstrap)
- ensure_rustup()?; + ensure_rustup(self.yolo)?; ... - ensure_rustup()?; + ensure_rustup(self.yolo)?; ... -fn ensure_rustup() -> anyhow::Result<()> { +fn ensure_rustup(yolo: bool) -> anyhow::Result<()> { if is_installed("rustup") { return Ok(()); } + if !yolo && !confirm("rustup is required. Install it now?")? { + bail!("rustup is required to continue"); + } println!("rustup is not installed. Attempting to install it..."); install_rustup().context("failed to install rustup")?;Also applies to: 119-120, 156-163
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/setup/tools.rs` around lines 88 - 89, The ensure_rustup function currently auto-installs rustup without checking the user confirmation flag; change ensure_rustup to accept a yolo: bool parameter (e.g., fn ensure_rustup(yolo: bool) -> Result<...>) and thread that flag into the internal rustup bootstrap/installer call so it only auto-installs when yolo is true; update every call site that invoked ensure_rustup() (the callers around the other mentioned blocks) to pass the local yolo variable through (e.g., ensure_rustup(yolo)?), and ensure the internal prompt/confirmation code inside ensure_rustup uses yolo to skip or enforce prompting accordingly.
🧹 Nitpick comments (1)
crates/module-parser/src/config.rs (1)
11-11: Return type should use the newCargoTomlDependenciesalias.
create_dependenciesreturnsHashMap<String, ConfigModuleMetadata>, which is the exact type thatCargoTomlDependenciesaliases. Using the raw type here is inconsistent with the alias introduced below and inmodule.rs.♻️ Proposed fix
- pub fn create_dependencies(self) -> anyhow::Result<HashMap<String, ConfigModuleMetadata>> { + pub fn create_dependencies(self) -> anyhow::Result<CargoTomlDependencies> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/module-parser/src/config.rs` at line 11, The function create_dependencies currently returns anyhow::Result<HashMap<String, ConfigModuleMetadata>>; change its signature to return anyhow::Result<CargoTomlDependencies> (the alias introduced for HashMap<String, ConfigModuleMetadata>) and update any local type annotations or casts within create_dependencies to use CargoTomlDependencies instead of the raw HashMap type so the function and its callers (e.g., places referencing create_dependencies in module.rs) consistently use the alias.
🤖 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/setup/module.rs`:
- Around line 204-230: In the dependency loop handling ConfigModuleMetadata,
path and deps are never written into dep_table; update the block that builds
dep_table (the code around dep_table, the handling of metadata.version/features,
and before workspace_deps.insert) to also: if let Some(path) = metadata.path {
dep_table.insert("path", path.into()); } and if !metadata.deps.is_empty() { let
deps_array: toml_edit::Array =
metadata.deps.into_iter().map(toml_edit::Value::from).collect();
dep_table.insert("deps", toml_edit::Value::Array(deps_array)); } so the path and
deps from ConfigModuleMetadata are preserved when inserting into workspace_deps.
- Around line 132-138: Update the error messages in function get_cargo_toml so
they don't always say "workspace Cargo.toml": use the actual cargo_toml_path (or
path) in the context strings—e.g., change the fs::read_to_string context and the
parse context to include cargo_toml_path.display() (or format!("Cargo.toml at
{}", cargo_toml_path.display())) so callers of get_cargo_toml for module paths
show the correct file location; keep the logic in get_cargo_toml, only change
the context messages.
- Around line 140-145: The get_dependencies function currently bails with
context("dependencies section not found or not a table") when a Cargo.toml
legitimately has no [dependencies], causing valid crates to error; change the
logic so that when doc.get("dependencies") is None you treat it as an empty
table and return an empty CargoTomlDependencies instead of failing. In practice,
remove the .context(...) check and branch on the presence of
doc.get("dependencies").and_then(|d| d.as_table()) — if Some(table) parse as
before, if None return Ok(CargoTomlDependencies::default() or an empty map) so
templates with no dependencies succeed; update references to dependencies
parsing in get_dependencies to handle an empty table case.
- Around line 123-124: The code reads Cargo.toml from the un-rendered template
(sdk_template) instead of the freshly generated crate; update the call that
builds dependencies so it loads Cargo.toml from the generated output directory
produced by generate() (use the generated crate path/variable returned by
generate() or the module's sdk output path) rather than sdk_template: replace
the use of sdk_template in the call to get_cargo_toml/get_dependencies with the
generated crate path variable (keeping fs::remove_dir_all(sdk_template) to clean
up the template afterwards) so get_cargo_toml(...) reads the rendered
Cargo.toml.
---
Outside diff comments:
In `@crates/module-parser/src/config.rs`:
- Line 17: The code uses single-character string literals in
package.replace("-", "_") which triggers clippy::single_char_pattern; change the
call to use the char overload by replacing the string literals with char
literals so the package variable assignment becomes package.replace('-', '_')
(locate the line that assigns to the package variable and update the replace
call accordingly).
---
Duplicate comments:
In `@crates/cli/src/setup/tools.rs`:
- Around line 65-85: resolve_tools currently ignores an explicit --all flag
because it returns all TOOLS when self.install is None; update resolve_tools to
honor an explicit selection by first checking for self.install (Vec names) then
for a boolean self.all (or require one of them) and only return
Ok(TOOLS.iter().collect()) when self.all is true; if neither self.install nor
self.all is set, return an error explaining that the caller must pass --install
or --all. Ensure you reference resolve_tools, self.install, self.all (or
equivalent flag), and TOOLS when making the change so the logic enforces
explicit selection.
- Around line 88-89: The ensure_rustup function currently auto-installs rustup
without checking the user confirmation flag; change ensure_rustup to accept a
yolo: bool parameter (e.g., fn ensure_rustup(yolo: bool) -> Result<...>) and
thread that flag into the internal rustup bootstrap/installer call so it only
auto-installs when yolo is true; update every call site that invoked
ensure_rustup() (the callers around the other mentioned blocks) to pass the
local yolo variable through (e.g., ensure_rustup(yolo)?), and ensure the
internal prompt/confirmation code inside ensure_rustup uses yolo to skip or
enforce prompting accordingly.
---
Nitpick comments:
In `@crates/module-parser/src/config.rs`:
- Line 11: The function create_dependencies currently returns
anyhow::Result<HashMap<String, ConfigModuleMetadata>>; change its signature to
return anyhow::Result<CargoTomlDependencies> (the alias introduced for
HashMap<String, ConfigModuleMetadata>) and update any local type annotations or
casts within create_dependencies to use CargoTomlDependencies instead of the raw
HashMap type so the function and its callers (e.g., places referencing
create_dependencies in module.rs) consistently use the alias.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
Cargo.tomlcrates/cli/src/setup/module.rscrates/cli/src/setup/tools.rscrates/module-parser/src/config.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- Cargo.toml
Signed-off-by: Bechma <19294519+Bechma@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
crates/cli/src/setup/module.rs (3)
150-156:⚠️ Potential issue | 🟡 MinorUse the concrete
Cargo.tomlpath in parse/read error context.Lines 153 and 155 currently report the directory path (
path.display()), which makes debugging harder than reporting the actualCargo.tomlfile path.💡 Proposed fix
fn get_cargo_toml(path: &Path) -> anyhow::Result<toml_edit::DocumentMut> { let cargo_toml_path = path.join("Cargo.toml"); fs::read_to_string(&cargo_toml_path) - .with_context(|| format!("can't read {}", path.display()))? + .with_context(|| format!("can't read {}", cargo_toml_path.display()))? .parse::<toml_edit::DocumentMut>() - .with_context(|| format!("can't parse {}", path.display())) + .with_context(|| format!("can't parse {}", cargo_toml_path.display())) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/setup/module.rs` around lines 150 - 156, The error contexts in get_cargo_toml report the directory path instead of the actual Cargo.toml file; change the with_context closures to reference the concrete cargo_toml_path (use cargo_toml_path.display()) so both the read_to_string and parse error messages include the full Cargo.toml file path for easier debugging.
57-62:⚠️ Potential issue | 🟠 MajorRegister the main module before SDK steps to prevent orphaned workspace state.
Line 57 waits for all generation work to finish before mutating
Cargo.tomlat Lines 60-61. If SDK generation/cleanup fails later (Lines 115-133), the main crate is already created on disk but never added toworkspace.members, and reruns fail on “already exists”.Also applies to: 93-136
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/setup/module.rs` around lines 57 - 62, The main crate is being created on disk before updating the workspace, which can leave an orphaned crate if subsequent SDK generation/cleanup (the later SDK steps) fails; move the workspace registration so add_modules_to_workspace(&mut doc, modules)? is called immediately after let (modules, dependencies) = self.generate_module()? (and before any SDK generation/cleanup), then proceed with add_dependencies_to_workspace(&mut doc, dependencies)?. Ensure the change is applied both in the earlier block and the duplicate block around the SDK generation/cleanup so the main module is always added to workspace.members before any further operations.
116-133:⚠️ Potential issue | 🟠 MajorRead SDK dependencies from the generated SDK crate, not the SDK template directory.
Line 131 currently reads
Cargo.tomlfromsdk_template(the template undermodules/<name>/sdk) instead of the generated output atmodules/<name>-sdk, so parsed dependencies can be stale/unrendered.💡 Proposed fix
if sdk_template.exists() { let name = format!("{}-sdk", self.name); generated.push(format!("modules/{name}")); generate(GenerateArgs { template_path: TemplatePath { path: Some(sdk_template.to_string_lossy().to_string()), ..TemplatePath::default() }, destination: Some(modules_path), - name: Some(name), + name: Some(name.clone()), quiet: !self.verbose, verbose: self.verbose, no_workspace: true, ..GenerateArgs::default() }) .with_context(|| format!("can't generate sdk module '{}-sdk'", self.name))?; - dependencies.extend(get_cargo_toml(&sdk_template).map(|x| get_dependencies(&x))?); + let sdk_output_path = self.path.join("modules").join(&name); + dependencies.extend(get_cargo_toml(&sdk_output_path).map(|x| get_dependencies(&x))?); fs::remove_dir_all(sdk_template) .with_context(|| format!("can't remove sdk template for module '{}'", self.name))?; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/setup/module.rs` around lines 116 - 133, The code reads Cargo.toml from the template directory (sdk_template) instead of the generated crate, causing stale/unrendered dependencies; after calling generate(...) use the generated crate path (the destination modules_path joined with name, where name = format!("{}-sdk", self.name)) when invoking get_cargo_toml and then get_dependencies, and only afterward call fs::remove_dir_all(sdk_template) so you parse the rendered Cargo.toml from the generated module rather than the template.
🤖 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/setup/module.rs`:
- Around line 150-156: The error contexts in get_cargo_toml report the directory
path instead of the actual Cargo.toml file; change the with_context closures to
reference the concrete cargo_toml_path (use cargo_toml_path.display()) so both
the read_to_string and parse error messages include the full Cargo.toml file
path for easier debugging.
- Around line 57-62: The main crate is being created on disk before updating the
workspace, which can leave an orphaned crate if subsequent SDK
generation/cleanup (the later SDK steps) fails; move the workspace registration
so add_modules_to_workspace(&mut doc, modules)? is called immediately after let
(modules, dependencies) = self.generate_module()? (and before any SDK
generation/cleanup), then proceed with add_dependencies_to_workspace(&mut doc,
dependencies)?. Ensure the change is applied both in the earlier block and the
duplicate block around the SDK generation/cleanup so the main module is always
added to workspace.members before any further operations.
- Around line 116-133: The code reads Cargo.toml from the template directory
(sdk_template) instead of the generated crate, causing stale/unrendered
dependencies; after calling generate(...) use the generated crate path (the
destination modules_path joined with name, where name = format!("{}-sdk",
self.name)) when invoking get_cargo_toml and then get_dependencies, and only
afterward call fs::remove_dir_all(sdk_template) so you parse the rendered
Cargo.toml from the generated module rather than the template.
This was referenced Mar 2, 2026
This was referenced Mar 11, 2026
Merged
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