add configuration module management with add, remove, and list commands#7
Merged
Bechma merged 2 commits intocyberfabric:mainfrom Mar 5, 2026
Merged
Conversation
Signed-off-by: Bechma <19294519+Bechma@users.noreply.github.com>
Contributor
📝 WalkthroughWalkthroughAdds a CLI config subsystem with module management (add/list/remove), richer module parsing (deps, features, capabilities), refactors path/config argument handling, introduces AppConfig types, updates workspace dependencies (async HTTP, tar/gzip, serde_json), and tightens lint rules. Changes
Sequence Diagram(s)sequenceDiagram
participant User as CLI User
participant CLI as CLI (config modules list)
participant Context as ModulesContext
participant Config as AppConfig
participant CratesIO as crates.io
participant Tarball as Tarball Extraction
User->>CLI: run "config modules list --verbose"
CLI->>Context: resolve_modules_context(path_config)
Context-->>CLI: workspace & config paths
CLI->>Config: load_config(path)
Config-->>CLI: AppConfig
CLI->>CratesIO: fetch_all_crates_io_metadata(system_crates)
loop per crate
CratesIO->>CratesIO: fetch crate metadata & tarball URL
CratesIO->>Tarball: download tarball
Tarball->>Tarball: extract src/module.rs
Tarball-->>CratesIO: module.rs content
CratesIO->>CLI: parsed RegistryMetadata (version, features, deps, capabilities)
end
CLI-->>User: display system/workspace/configured modules and metadata
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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.
Actionable comments posted: 2
🧹 Nitpick comments (2)
crates/cli/src/config/modules/remove.rs (1)
29-37: Consider extracting shared validation logic.The
validate_module_namefunction may be duplicated inadd.rs. If both modules need the same validation, consider extracting it to a shared location (e.g., the parentmod.rs) to maintain consistency and reduce duplication.#!/bin/bash # Check if similar validation exists in add.rs rg -n "validate_module_name|is_empty|is_ascii_alphanumeric" crates/cli/src/config/modules/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/config/modules/remove.rs` around lines 29 - 37, The validation logic in validate_module_name (used in remove.rs) is likely duplicated in add.rs; extract the shared check into a common function (e.g., pub(crate) fn validate_module_name in the parent module file mod.rs or a shared validators module) and have both add.rs and remove.rs call that single implementation to avoid duplication and keep behavior consistent; update the imports/usages in add.rs and remove.rs to call the centralized validate_module_name and keep the same signature and error type (anyhow::Result<()>) so callers need no other changes.crates/cli/src/mod/add.rs (1)
153-200: Consider deduplicating table vs inline-table dependency extraction.Both branches parse the same fields with near-identical logic; extracting a shared helper would reduce drift and simplify future metadata additions.
🤖 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 153 - 200, The code duplicates parsing logic for value.as_table() and value.as_inline_table(); create a single helper like extract_dep_fields<T: ?>(table: &T) -> (Option<&str>, Option<&str>, Option<&str>, Vec<String>, Option<bool>) that uses table.get(...) to read "package", "version", "path", "features" (map array->Vec<String>) and "default-features"/"default_features" (as_bool), then call that helper for both value.as_table() and value.as_inline_table() instead of repeating the blocks so package, version, pkg/path, features and default_features are populated once.
🤖 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 181-214: print_config_metadata currently omits
metadata.capabilities and metadata.default_features from verbose output; update
the function to also display default_features (print a line like
"default_features: (none)" or list items) and capabilities (similarly handling
empty vs non-empty) alongside the existing package/version/path/features/deps
logic, using the same indentation and iteration pattern as for features and deps
so fields in crate::config::app_config::ModuleConfig::metadata are fully shown.
In `@crates/module-parser/src/module_rs.rs`:
- Around line 100-113: The code currently parses meta.value()? into an syn::Expr
and silently ignores any non-string or malformed entries when matching
syn::Expr::Array -> array.elems and then syn::Expr::Lit(Lit::Str) before
deps.push; change this to validate shapes and return a parse error for invalid
forms: when meta.value() parses to a non-Array, or when any element in
array.elems is not an Expr::Lit with Lit::Str, produce and return a syn::Error
(e.g. via syn::Error::new_spanned) referencing the offending meta/value/element
span so malformed deps are rejected instead of dropped. Ensure you update the
branch that currently handles syn::Expr::Array and the inner element match to
propagate an error on unexpected kinds rather than skipping them.
---
Nitpick comments:
In `@crates/cli/src/config/modules/remove.rs`:
- Around line 29-37: The validation logic in validate_module_name (used in
remove.rs) is likely duplicated in add.rs; extract the shared check into a
common function (e.g., pub(crate) fn validate_module_name in the parent module
file mod.rs or a shared validators module) and have both add.rs and remove.rs
call that single implementation to avoid duplication and keep behavior
consistent; update the imports/usages in add.rs and remove.rs to call the
centralized validate_module_name and keep the same signature and error type
(anyhow::Result<()>) so callers need no other changes.
In `@crates/cli/src/mod/add.rs`:
- Around line 153-200: The code duplicates parsing logic for value.as_table()
and value.as_inline_table(); create a single helper like extract_dep_fields<T:
?>(table: &T) -> (Option<&str>, Option<&str>, Option<&str>, Vec<String>,
Option<bool>) that uses table.get(...) to read "package", "version", "path",
"features" (map array->Vec<String>) and "default-features"/"default_features"
(as_bool), then call that helper for both value.as_table() and
value.as_inline_table() instead of repeating the blocks so package, version,
pkg/path, features and default_features are populated once.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4ec9f889-b723-4187-835b-1640ad4ffcd5
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
Cargo.tomlcrates/cli/Cargo.tomlcrates/cli/src/build/mod.rscrates/cli/src/common.rscrates/cli/src/config/app_config.rscrates/cli/src/config/mod.rscrates/cli/src/config/modules/add.rscrates/cli/src/config/modules/list.rscrates/cli/src/config/modules/mod.rscrates/cli/src/config/modules/remove.rscrates/cli/src/main.rscrates/cli/src/mod/add.rscrates/cli/src/run/mod.rscrates/cli/src/run/run_loop.rscrates/module-parser/src/config.rscrates/module-parser/src/lib.rscrates/module-parser/src/module_rs.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
.clippy.toml (1)
1-6: Clarify scope: this config applies workspace-wide, not just to Modkit.Line 1's comment implies a local exception, but
.clippy.tomlat the workspace root applies globally to all crates. This weakens lint pressure across the entire codebase. For functions that legitimately require higher complexity (e.g., parsers), use targeted#[allow(clippy::cognitive_complexity, clippy::too_many_lines)]or#[expect(...)]instead of relaxing global thresholds. This keeps strictness for the rest of the codebase and documents the reason for each exception.Suggested comment update
-# Modkit allowed to have more complex logic than modules +# Workspace-wide thresholds. Prefer `#[allow(clippy::...)]` on specific functions +# rather than raising these limits globally.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.clippy.toml around lines 1 - 6, Update the top comment in .clippy.toml to state that the thresholds (cognitive-complexity-threshold, type-complexity-threshold, too-many-lines-threshold) apply workspace-wide rather than only to "Modkit"; then revert the global threshold relaxation and instead add guidance in the comment indicating contributors should use targeted attributes (e.g., #[allow(clippy::cognitive_complexity)], #[allow(clippy::too_many_lines)] or #[expect(...)]) on specific functions/crates (such as parsers) when higher complexity is justified, so that the file documents both the global scope and the recommended per-item exception mechanism.
🤖 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 134-136: print_local_metadata currently calls
print_metadata(&module.metadata, false) which hides features and
default_features under --verbose; change the second argument to true so
print_local_metadata(module: &ConfigModule) calls
print_metadata(&module.metadata, true) to expose those fields, and make the
identical change in the other local-module metadata printing block that uses
print_metadata(..., false) (the similar block flagged in the review) so all
local modules honor --verbose.
- Around line 224-233: The code spawns a task per entry in
SYSTEM_REGISTRY_MODULES which lets all fetch_crates_io_metadata calls run
concurrently and each task calls .bytes().await (buffering full archives),
causing large memory spikes; limit in-flight archive fetches by adding
concurrency control (e.g., a tokio::sync::Semaphore or use
futures::stream::StreamExt::buffer_unordered) and only allow a small number
(e.g., 4–8) of fetch_crates_io_metadata calls to proceed at once. Concretely,
replace the plain for-loop + tokio::task::JoinSet that spawns for each module
with either (a) acquire a permit from a Semaphore inside each spawned task
before calling fetch_crates_io_metadata/.bytes().await and release it after, or
(b) build a stream from SYSTEM_REGISTRY_MODULES and drive async closures that
call fetch_crates_io_metadata through buffer_unordered(CONCURRENCY) so
.bytes().await runs for at most CONCURRENCY modules concurrently; keep the rest
of the result handling (wrapping with with_context and returning
(module.crate_name, metadata)) unchanged.
In `@crates/cli/src/config/modules/mod.rs`:
- Around line 138-145: The current save_config function writes directly to the
target path which can leave a truncated config if interrupted; change
save_config to write the serialized string to a temporary file created in the
same directory (e.g., using path.with_extension(".tmp") or a uniquely named temp
file), fs::write that temp file, then atomically replace the target by renaming
the temp into place (std::fs::rename), and propagate context on both the write
and rename operations; keep references to the existing symbols save_config,
path, config, serialized and ensure the temp file is created in the same
directory so rename is atomic.
---
Nitpick comments:
In @.clippy.toml:
- Around line 1-6: Update the top comment in .clippy.toml to state that the
thresholds (cognitive-complexity-threshold, type-complexity-threshold,
too-many-lines-threshold) apply workspace-wide rather than only to "Modkit";
then revert the global threshold relaxation and instead add guidance in the
comment indicating contributors should use targeted attributes (e.g.,
#[allow(clippy::cognitive_complexity)], #[allow(clippy::too_many_lines)] or
#[expect(...)]) on specific functions/crates (such as parsers) when higher
complexity is justified, so that the file documents both the global scope and
the recommended per-item exception mechanism.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 73897849-a1f9-45ce-9c0d-c8ca97f96011
📒 Files selected for processing (6)
.clippy.tomlcrates/cli/src/config/app_config.rscrates/cli/src/config/modules/add.rscrates/cli/src/config/modules/list.rscrates/cli/src/config/modules/mod.rscrates/cli/src/config/modules/remove.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/cli/src/config/modules/add.rs
Signed-off-by: Bechma <19294519+Bechma@users.noreply.github.com>
Bechma
force-pushed
the
config-modification-added
branch
from
1708f70 to
63878ce
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.clippy.toml (1)
1-6: Scope mismatch: these relaxed thresholds apply workspace-wide, not just Modkit.The comment states a Modkit-specific intent, but placing these values in the root
.clippy.tomlweakens linting for every crate. Consider moving these thresholds to a crate-local configuration (via#[allow(...)]attributes on specific items) to avoid reducing signal globally.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.clippy.toml around lines 1 - 6, The relaxed thresholds (cognitive-complexity-threshold, type-complexity-threshold, too-many-lines-threshold) are currently in the root .clippy.toml and therefore apply workspace-wide; remove these keys from the root file and instead either (A) add a crate-local .clippy.toml inside the Modkit crate directory with those same threshold settings so only Modkit is affected, or (B) avoid changing thresholds and apply targeted attributes like #[allow(clippy::cognitive_complexity, clippy::type_complexity, clippy::too_many_lines)] on the specific functions or modules in Modkit that need exemptions; pick one approach and update the repository accordingly so other crates keep the default linting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.clippy.toml:
- Around line 1-6: The relaxed thresholds (cognitive-complexity-threshold,
type-complexity-threshold, too-many-lines-threshold) are currently in the root
.clippy.toml and therefore apply workspace-wide; remove these keys from the root
file and instead either (A) add a crate-local .clippy.toml inside the Modkit
crate directory with those same threshold settings so only Modkit is affected,
or (B) avoid changing thresholds and apply targeted attributes like
#[allow(clippy::cognitive_complexity, clippy::type_complexity,
clippy::too_many_lines)] on the specific functions or modules in Modkit that
need exemptions; pick one approach and update the repository accordingly so
other crates keep the default linting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3e51edf8-2149-405c-a318-a41f3c880689
📒 Files selected for processing (6)
.clippy.tomlcrates/cli/src/config/app_config.rscrates/cli/src/config/modules/add.rscrates/cli/src/config/modules/list.rscrates/cli/src/config/modules/mod.rscrates/cli/src/config/modules/remove.rs
This was referenced Mar 5, 2026
This was referenced Mar 13, 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
New Features
Chores