Address PR review feedback: security, CLI ergonomics, git2 add_submodule, and correctness fixes#19
Conversation
…ity, utilities fixes, options Unspecified skip, CLI fallback improvements, git2 add_submodule implementation Co-authored-by: bashandbone <89049923+bashandbone@users.noreply.github.com>
…update after cloning Co-authored-by: bashandbone <89049923+bashandbone@users.noreply.github.com>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
There was a problem hiding this comment.
Pull request overview
This PR addresses prior review feedback by tightening MCP filesystem allowlists, improving CLI ergonomics around internal sentinel values, fixing name parsing correctness, and implementing a real git2-backed add_submodule (with CLI fallback improvements).
Changes:
- Harden MCP config by removing
~/.cargofrom the filesystem allowlist. - Improve CLI UX by hiding internal
Unspecifiedsentinel variants and makingadd --ignore/--fetch/--updatetruly optional (Option<...>). - Replace the
git2add_submodulestub with a functional implementation and enhance the CLI fallback to forward--branchand shallow depth.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/utilities.rs |
Fixes name_from_osstring validation ordering and updates anyhow imports. |
src/options.rs |
Hides internal Unspecified sentinel variants from clap help/completions. |
src/commands.rs |
Makes add command’s --ignore/--fetch/--update optional (Option<...>). |
src/main.rs |
Propagates optional ignore/fetch/update values into GitManager::add_submodule. |
src/git_ops/mod.rs |
Improves CLI fallback git submodule add invocation by forwarding --branch and --depth 1. |
src/git_ops/git2_ops.rs |
Implements git2-based add_submodule with clone/index/finalize and config writes. |
.roo/mcp.json |
Removes ~/.cargo from MCP filesystem server args to avoid credential exposure. |
Cargo.toml |
Removes duplicate license metadata and reduces keywords count. |
README.md |
Updates example usage to include submodule names for delete/disable. |
sample_config/submod.toml |
Fixes typo (“fetchest” → “fetches”). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // 1. Create submodule entry in .gitmodules and index | ||
| let mut sub = self | ||
| .repo | ||
| .submodule(&opts.url, opts.path.as_path(), true) | ||
| .with_context(|| { | ||
| format!( | ||
| "Failed to create submodule entry for '{}' from '{}'", | ||
| opts.name, opts.url | ||
| ) | ||
| })?; |
There was a problem hiding this comment.
repo.submodule(&opts.url, opts.path.as_path(), true) ignores opts.name (it only appears in error messages). This makes backend behavior diverge from the CLI fallback (which passes --name) and can lead to the submodule being registered under its path instead of the user-supplied name. If opts.name is intended to be authoritative, adjust the git2 path to ensure .gitmodules/submodule naming matches it (or explicitly document/enforce that name == path for the git2 backend).
| // 5. Apply optional configuration via git config. | ||
| // git2's submodule() keys the submodule by path; use the path as the config key. | ||
| let path_str = opts.path.to_string_lossy(); | ||
| let mut config = self | ||
| .repo | ||
| .config() | ||
| .with_context(|| "Failed to open git config")?; | ||
|
|
||
| // Set branch if specified | ||
| if let Some(branch) = &opts.branch { | ||
| let branch_key = format!("submodule.{}.branch", path_str); | ||
| config | ||
| .set_str(&branch_key, &branch.to_string()) | ||
| .with_context(|| format!("Failed to set branch for submodule '{}'", opts.name))?; | ||
| } | ||
|
|
||
| // Set ignore rule if specified and not the sentinel Unspecified value | ||
| if let Some(ignore) = &opts.ignore { | ||
| if !matches!(ignore, SerializableIgnore::Unspecified) { | ||
| let ignore_key = format!("submodule.{}.ignore", path_str); | ||
| config | ||
| .set_str(&ignore_key, &ignore.to_string()) | ||
| .with_context(|| { | ||
| format!("Failed to set ignore for submodule '{}'", opts.name) | ||
| })?; | ||
| } | ||
| } | ||
|
|
||
| // Set fetch recurse if specified and not the sentinel Unspecified value | ||
| if let Some(fetch_recurse) = &opts.fetch_recurse { | ||
| if !matches!(fetch_recurse, SerializableFetchRecurse::Unspecified) { | ||
| let fetch_key = format!("submodule.{}.fetchRecurseSubmodules", path_str); | ||
| config | ||
| .set_str(&fetch_key, &fetch_recurse.to_string()) | ||
| .with_context(|| { | ||
| format!("Failed to set fetchRecurse for submodule '{}'", opts.name) | ||
| })?; | ||
| } | ||
| } | ||
|
|
||
| // Set update strategy if specified and not the sentinel Unspecified value | ||
| if let Some(update) = &opts.update { | ||
| if !matches!(update, SerializableUpdate::Unspecified) { | ||
| let update_key = format!("submodule.{}.update", path_str); | ||
| config |
There was a problem hiding this comment.
Config writes in add_submodule() use path_str in keys like submodule.<...>.branch/ignore/update/fetchRecurseSubmodules, but the rest of this module reads these settings using the submodule name (e.g. get_submodule_branch() builds submodule.{name}.branch). This mismatch means the settings written here may never be read back (especially when name != path). Use a consistent identifier for both reading and writing (ideally the actual git submodule name returned by git2 after creation).
| // 1. Create submodule entry in .gitmodules and index | ||
| let mut sub = self | ||
| .repo | ||
| .submodule(&opts.url, opts.path.as_path(), true) | ||
| .with_context(|| { | ||
| format!( | ||
| "Failed to create submodule entry for '{}' from '{}'", | ||
| opts.name, opts.url | ||
| ) | ||
| })?; | ||
|
|
||
| // 2. Configure clone options | ||
| let mut update_opts = git2::SubmoduleUpdateOptions::new(); | ||
| let mut fetch_opts = git2::FetchOptions::new(); | ||
| if opts.shallow { | ||
| fetch_opts.depth(1); | ||
| } | ||
| update_opts.fetch(fetch_opts); | ||
|
|
||
| // 3. Clone the submodule repository | ||
| sub.clone(Some(&mut update_opts)).with_context(|| { | ||
| format!( | ||
| "Failed to clone submodule '{}' from '{}'", | ||
| opts.name, opts.url | ||
| ) | ||
| })?; | ||
|
|
||
| // 4. Add to index and finalize | ||
| sub.add_to_index(true) | ||
| .with_context(|| format!("Failed to add submodule '{}' to index", opts.name))?; | ||
| sub.add_finalize() | ||
| .with_context(|| format!("Failed to finalize submodule '{}'", opts.name))?; | ||
|
|
There was a problem hiding this comment.
This implementation performs side-effecting steps (creating the submodule entry / writing .gitmodules, cloning, staging) before returning an error. Since the higher-level fallback chain will try the CLI path on failure, a mid-way failure here can leave a partially-added submodule that makes the CLI fallback fail or produce a broken repo state. Consider making this operation transactional (best-effort cleanup of .gitmodules/index/workdir on error) or preventing CLI fallback if git2 has already mutated repo state.
| if s.trim().is_empty() { | ||
| return Err(anyhow::anyhow!("Name cannot be empty or whitespace-only")); | ||
| } | ||
| let sep = std::path::MAIN_SEPARATOR.to_string(); | ||
| s.trim() | ||
| .split(&sep) | ||
| .last() | ||
| .map(|name| name.to_string()) | ||
| .ok_or_else(|| anyhow::anyhow!("Failed to extract name from OsString")) |
There was a problem hiding this comment.
name_from_osstring() can still return an empty name when the input ends with a path separator (e.g. "foo/bar/") because split(MAIN_SEPARATOR).last() becomes "", which bypasses the earlier trim().is_empty() check. Consider trimming trailing separators before splitting, or (more robustly) using Path::new(..).file_name() and validating the extracted component is non-empty/non-whitespace.
| if s.trim().is_empty() { | |
| return Err(anyhow::anyhow!("Name cannot be empty or whitespace-only")); | |
| } | |
| let sep = std::path::MAIN_SEPARATOR.to_string(); | |
| s.trim() | |
| .split(&sep) | |
| .last() | |
| .map(|name| name.to_string()) | |
| .ok_or_else(|| anyhow::anyhow!("Failed to extract name from OsString")) | |
| let trimmed = s.trim(); | |
| if trimmed.is_empty() { | |
| return Err(anyhow::anyhow!("Name cannot be empty or whitespace-only")); | |
| } | |
| let path = std::path::Path::new(trimmed); | |
| let component = path | |
| .file_name() | |
| .ok_or_else(|| anyhow::anyhow!("Failed to extract name from OsString"))?; | |
| let component_str = component | |
| .to_str() | |
| .ok_or_else(|| anyhow::anyhow!("Failed to convert OsString component to UTF-8"))?; | |
| let name = component_str.trim(); | |
| if name.is_empty() { | |
| return Err(anyhow::anyhow!("Name cannot be empty or whitespace-only")); | |
| } | |
| Ok(name.to_string()) |
Addresses all suggestions from the review of #12. Fixes a security concern in MCP config, corrects Cargo.toml metadata, improves CLI UX by hiding internal sentinel values, and replaces the always-failing git2
add_submodulestub with a real implementation.Cargo.toml
license/license-filefields — keep onlylicense-file = "LICENSE.md"keywordsto 5 (crates.io hard limit)Security
~/.cargofrom.roo/mcp.jsonfilesystem MCP allowlist to prevent exposure of registry credentialssrc/utilities.rsuse anyhow::Okand redundantuse std::result::Result; useuse anyhow::Resultname_from_osstring(): null-byte check was unreachable for non-empty strings; now validates null bytes first, then rejects empty/whitespace-only namesCLI —
Unspecifiedsentinel hiding#[value(skip)]to theUnspecifiedvariant onSerializableIgnore,SerializableFetchRecurse, andSerializableUpdateso it never surfaces in--helpor tab completionCLI —
addcommand option types--ignore,--fetch,--updateinAddfromdefault_value = "unspecified"non-optional fields toOption<...>— users only see semantically meaningful choices:git2— realadd_submoduleimplementationReplace the stub that always returned an error (forcing CLI fallback as the primary path) with a proper git2 implementation:
repo.submodule(url, path, use_gitlink)— registers in.gitmodules+ indexsub.clone(update_opts)— clones with optionaldepth(1)for shallowsub.add_to_index()+sub.add_finalize()branch,ignore,fetchRecurseSubmodules, andupdateCLI fallback (
git_ops/mod.rs)git submodule addinvocation now forwards--branchand--depth 1(shallow) fromSubmoduleAddOptionsDocs / samples
submod deleteandsubmod disableREADME examplessample_config/submod.toml💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.