implement add deps to cargo workspace#9
Merged
Bechma merged 1 commit intocyberfabric:mainfrom Mar 9, 2026
Merged
Conversation
Bechma
force-pushed
the
fix-add-deps-after-mod-add
branch
from
b0318d6 to
f76f726
Compare
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughRefactors the CLI module-add flow to be workspace-aware: module generation now returns generated paths; modules are validated and staged, manifests rewritten to workspace inheritance; dependencies are collected, semver-merged and applied to the root Cargo.toml. Adds semver workspace dependency and many TOML/semver helpers. Changes
Sequence DiagramsequenceDiagram
rect rgba(200,230,255,0.5)
actor CLI
end
participant Generator as ModuleGenerator
participant Preparer as ModulePreparer
participant Resolver as DepResolver
participant Updater as WorkspaceUpdater
participant FS as FileSystem
CLI->>Generator: generate_module()
Generator->>FS: create module files
Generator-->>CLI: return generated_paths
CLI->>Preparer: ensure_modules_directory(workspace_root)
Preparer->>FS: check/create modules/ directory
Preparer-->>CLI: ok
CLI->>Preparer: prepare_generated_modules(workspace_root, generated_paths)
Preparer->>FS: read each module Cargo.toml
Preparer->>Resolver: get_dependencies(doc, source_dir, workspace_root)
Resolver->>Resolver: normalize names, resolve workspace-relative paths, parse versions
Resolver-->>Preparer: collected dependencies
Preparer->>FS: rewrite module manifests to workspace inheritance
Preparer-->>CLI: staged_module_writes + deps
CLI->>Updater: update_workspace_cargo_toml(workspace_root, generated_paths, deps)
Updater->>FS: read root Cargo.toml
Updater->>Updater: add members, merge/upsert deps (semver logic)
Updater->>FS: write updated root Cargo.toml
Updater-->>CLI: success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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)
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 (1)
crates/cli/src/mod/add.rs (1)
332-336: Consider warning when falling back to wildcard version.Using
"*"as the fallback version (line 335) enables the workflow to proceed when a module template doesn't specify a version, but wildcard versions can lead to non-reproducible builds. Consider logging a warning when this fallback is used to alert users that they should pin the version.💡 Optional: Add a warning for wildcard fallback
if let Some(version) = metadata.version { dep_table.insert("version", version.into()); } else { + eprintln!("warning: no version specified for dependency, using wildcard '*'"); dep_table.insert("version", "*".into()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/mod/add.rs` around lines 332 - 336, When falling back to the wildcard version ("*") in the metadata.version None branch, emit a warning to alert users that using "*" can lead to non-reproducible builds and they should pin versions; locate the block that checks metadata.version and inserts into dep_table (the dep_table.insert("version", "*".into()) branch) and add a warning via the project's logging facility (e.g., warn! / tracing::warn! / log::warn!) immediately after inserting the wildcard, including the module name or identifier if available to give context.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/cli/src/mod/add.rs`:
- Around line 332-336: When falling back to the wildcard version ("*") in the
metadata.version None branch, emit a warning to alert users that using "*" can
lead to non-reproducible builds and they should pin versions; locate the block
that checks metadata.version and inserts into dep_table (the
dep_table.insert("version", "*".into()) branch) and add a warning via the
project's logging facility (e.g., warn! / tracing::warn! / log::warn!)
immediately after inserting the wildcard, including the module name or
identifier if available to give context.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6236be12-c07a-4e6a-aeff-3c53f8775cf2
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
Cargo.tomlcrates/cli/Cargo.tomlcrates/cli/src/mod/add.rs
Bechma
force-pushed
the
fix-add-deps-after-mod-add
branch
from
f76f726 to
9ae0bed
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
crates/cli/src/mod/add.rs (5)
336-343: Consider logging the dependency name in the wildcard warning.When falling back to
*, the warning doesn't identify which dependency triggered it, making debugging harder.Note: This function doesn't have access to the dependency name. You'd need to pass it as a parameter or restructure the warning.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/mod/add.rs` around lines 336 - 343, The warning lacks the dependency name; modify the function that builds dep_table (the one using variables metadata, dep_table and inserting path/version) to accept the dependency name (e.g., dependency_name: &str) or otherwise obtain it from the caller, then change the fallback branch to eprintln! including that dependency_name (e.g., "warning: no version specified for dependency '{}', using wildcard '*'" ), and update all call sites to pass the dependency name so the log pinpoints which dependency fell back to '*'.
604-611: Minor: Prefer.first()over.get(0)for idiomatic Rust.💡 Suggested change
- assert_eq!( - features.get(0).and_then(toml_edit::Value::as_str), - Some("json") - ); + assert_eq!( + features.first().and_then(toml_edit::Value::as_str), + Some("json") + );Same applies to lines 718.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/mod/add.rs` around lines 604 - 611, The code uses features.get(0).and_then(toml_edit::Value::as_str) which is unidiomatic; change to features.first().and_then(toml_edit::Value::as_str) in the assertion that checks the first feature (the features variable and its assertion block), and make the same replacement at the other occurrence around the second assertion near line 718 so both checks use .first() instead of .get(0).
206-209: Silent skip of unknown dependency formats could hide issues.If a dependency has an unexpected TOML structure (neither string, table, nor inline table), it's silently skipped. While this is defensive, a warning would help debug template issues.
💡 Optional: add trace/debug logging
} else { + eprintln!("warning: skipping dependency '{name}' with unsupported format"); continue; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/mod/add.rs` around lines 206 - 209, The code currently silently continues when a dependency value is neither a string, table, nor inline table; change that silent skip to emit a warning so template issues are visible. In the branch that handles dependency value types (the else { continue; } branch that follows the string/table/inline_table checks), replace the silent continue with a warning log (e.g., tracing::warn! or log::warn!) that includes the dependency key and the unexpected value (or its type) and then continue; keep the rest of the control flow the same so non-conforming TOML still skips but is logged for debugging.
743-763: Tests use absolute Unix-style paths that may fail on Windows CI.The test at line 754-755 uses
/repo/modules/rest-gatewayand/repowhich won't work correctly on Windows where paths start with drive letters.💡 Consider using platform-agnostic test paths
If Windows CI support is needed, consider using
tempdiror constructing paths relative tostd::env::current_dir():use std::env; let repo = env::current_dir().unwrap(); let module_path = repo.join("modules").join("rest-gateway");Or skip these tests on Windows with
#[cfg(unix)]if Windows support isn't a priority.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/mod/add.rs` around lines 743 - 763, The test collects_dependencies_with_workspace_relative_paths uses hard-coded absolute Unix-style paths ("/repo/...") which will fail on Windows; update the test to build platform-agnostic paths (e.g., use std::env::current_dir() or a tempdir and PathBuf::join) and pass those PathBufs into get_dependencies instead of literal strings so Path operations work on Windows and Unix; locate the test function collects_dependencies_with_workspace_relative_paths and replace the Path::new("/repo/modules/rest-gateway") and Path::new("/repo") inputs with constructed PathBuf values derived from current_dir() or a temp directory before calling get_dependencies, keeping the same assertions on sdk_dep.path.
377-380: Fallback to wildcard version in edge case may mask issues.Line 378 falls back to
"*"ifexisting_dep.as_str()returnsNone. By the time this branch is reached,existing_depshould always be a string (since table/inline branches returned early). If it'sNone, something unexpected happened.Consider using
unwrap_or_elsewith a warning, or an explicit assertion.💡 Optional: Add defensive logging
let mut dep_table = toml_edit::InlineTable::new(); - dep_table.insert("version", existing_dep.as_str().unwrap_or("*").into()); + let version = existing_dep.as_str().unwrap_or_else(|| { + eprintln!("warning: unexpected dependency format, using wildcard version"); + "*" + }); + dep_table.insert("version", version.into());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/mod/add.rs` around lines 377 - 380, The code currently falls back to "*" via existing_dep.as_str().unwrap_or("*"), which masks an unexpected non-string existing_dep; update the handling so you explicitly check existing_dep.as_str() (referencing existing_dep and dep_table and the assignment to toml_edit::Item::Value) and if it is None either emit a defensive warning with context (e.g., using the repository logger/tracing) and return an Err, or assert/fail fast (debug_assert or panic) instead of silently using "*"; implement this explicit branch (log/error + early return or assert) rather than unwrap_or to make the unexpected state visible.
🤖 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/mod/add.rs`:
- Around line 128-133: Currently CargoTomlDependencies is merged via
HashMap::extend in the loop (see generated_modules loop, get_dependencies, and
CargoTomlDependencies usage), which silently overwrites duplicate dependency
entries; change the merge to check for existing keys and when a conflict is
found call should_replace_with_newer_semver(old_version, new_version) to decide
whether to keep/replace, and log a warning when versions are incompatible or
when an overwrite occurs; specifically replace the direct
dependencies.extend(...) with an iterative merge that inspects existing entries
from CargoTomlDependencies before inserting, uses
should_replace_with_newer_semver to pick the newer semver, and emits a clear
warning mentioning the dependency name and both versions when a change happens.
---
Nitpick comments:
In `@crates/cli/src/mod/add.rs`:
- Around line 336-343: The warning lacks the dependency name; modify the
function that builds dep_table (the one using variables metadata, dep_table and
inserting path/version) to accept the dependency name (e.g., dependency_name:
&str) or otherwise obtain it from the caller, then change the fallback branch to
eprintln! including that dependency_name (e.g., "warning: no version specified
for dependency '{}', using wildcard '*'" ), and update all call sites to pass
the dependency name so the log pinpoints which dependency fell back to '*'.
- Around line 604-611: The code uses
features.get(0).and_then(toml_edit::Value::as_str) which is unidiomatic; change
to features.first().and_then(toml_edit::Value::as_str) in the assertion that
checks the first feature (the features variable and its assertion block), and
make the same replacement at the other occurrence around the second assertion
near line 718 so both checks use .first() instead of .get(0).
- Around line 206-209: The code currently silently continues when a dependency
value is neither a string, table, nor inline table; change that silent skip to
emit a warning so template issues are visible. In the branch that handles
dependency value types (the else { continue; } branch that follows the
string/table/inline_table checks), replace the silent continue with a warning
log (e.g., tracing::warn! or log::warn!) that includes the dependency key and
the unexpected value (or its type) and then continue; keep the rest of the
control flow the same so non-conforming TOML still skips but is logged for
debugging.
- Around line 743-763: The test
collects_dependencies_with_workspace_relative_paths uses hard-coded absolute
Unix-style paths ("/repo/...") which will fail on Windows; update the test to
build platform-agnostic paths (e.g., use std::env::current_dir() or a tempdir
and PathBuf::join) and pass those PathBufs into get_dependencies instead of
literal strings so Path operations work on Windows and Unix; locate the test
function collects_dependencies_with_workspace_relative_paths and replace the
Path::new("/repo/modules/rest-gateway") and Path::new("/repo") inputs with
constructed PathBuf values derived from current_dir() or a temp directory before
calling get_dependencies, keeping the same assertions on sdk_dep.path.
- Around line 377-380: The code currently falls back to "*" via
existing_dep.as_str().unwrap_or("*"), which masks an unexpected non-string
existing_dep; update the handling so you explicitly check existing_dep.as_str()
(referencing existing_dep and dep_table and the assignment to
toml_edit::Item::Value) and if it is None either emit a defensive warning with
context (e.g., using the repository logger/tracing) and return an Err, or
assert/fail fast (debug_assert or panic) instead of silently using "*";
implement this explicit branch (log/error + early return or assert) rather than
unwrap_or to make the unexpected state visible.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4051903b-1618-4b07-88f8-bf836f6ba1bc
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
Cargo.tomlcrates/cli/Cargo.tomlcrates/cli/src/mod/add.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/cli/Cargo.toml
- Cargo.toml
Bechma
force-pushed
the
fix-add-deps-after-mod-add
branch
from
9ae0bed to
018f48e
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/mod/add.rs`:
- Around line 64-65: prepare_generated_modules currently persists each generated
module Cargo.toml (setting workspace = true) before update_workspace_cargo_toml
runs, which can leave modules pointing to non-existent workspace members if the
root update fails; change the flow so prepare_generated_modules returns the
in-memory representations (e.g., a Vec of (module_path, rewritten_toml_string)
or a GeneratedModule struct) without writing files, then call
update_workspace_cargo_toml(&self.path, &generated_modules, dependencies) and
only if that call succeeds iterate the staged data and write the rewritten
manifests to disk; update references to generated_modules and any callers to use
the staged data return type and ensure error handling aborts without writing
module files when the root update fails.
- Around line 133-153: The conflict handler in get_dependencies currently only
compares versions (using should_replace_with_newer_semver) and inserts the
chosen incoming dependency, dropping other metadata (default_features, path,
package, features); update the logic that processes (name, incoming) vs existing
so that whenever you insert/replace you also merge metadata: reconcile
default_features by preferring false over true (i.e., if either is false, set
false), union features lists, and preserve non-empty path and package values
(prefer the explicit/local value over empty/workspace when merging); apply this
merge both when should_replace_with_newer_semver decides to replace and when
versions are equal (old_ver == new_ver) so you never silently drop
default_features/path/package from existing or incoming entries.
- Around line 333-336: In add_dependencies_to_workspace, existing workspace
entries (workspace_deps.get_mut(&name)) currently only call
maybe_upgrade_workspace_dep_version and
maybe_apply_workspace_dep_default_features and then continue, which silently
drops incoming source-defining keys; change this logic to detect incoming
metadata.package and metadata.path: if the existing_dep lacks a package or path,
merge the incoming package/path into existing_dep; if existing_dep has a
conflicting package or path value (different from incoming), return/fail with a
clear error mentioning the dependency name and the conflicting key; keep the
current behavior for version and default-features via
maybe_upgrade_workspace_dep_version and
maybe_apply_workspace_dep_default_features but ensure source-defining keys are
never silently discarded.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 89412f0e-c1d3-4f32-ac6e-fe259e662b45
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
Cargo.tomlcrates/cli/Cargo.tomlcrates/cli/src/mod/add.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/cli/Cargo.toml
Signed-off-by: Bechma <19294519+Bechma@users.noreply.github.com>
Bechma
force-pushed
the
fix-add-deps-after-mod-add
branch
from
018f48e to
8002fab
Compare
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
New Features
Documentation
Chores