refactor: simplify path handling in build and run commands#27
Merged
Bechma merged 2 commits intocyberfabric:mainfrom Apr 7, 2026
Merged
refactor: simplify path handling in build and run commands#27Bechma merged 2 commits intocyberfabric:mainfrom
Bechma merged 2 commits intocyberfabric:mainfrom
Conversation
Signed-off-by: Bechma <19294519+Bechma@users.noreply.github.com>
Contributor
📝 WalkthroughWalkthroughCLI workspace path handling was moved to argument-parse time: an optional Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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)
Cargo.toml (1)
20-30: Usecargo addfor these dependency bumps.Please regenerate the
cargo-generate,serde-saphyr,toml, andtoml_editupdates viacargo addinstead of hand-editing the manifest. That keeps the dependency and lockfile updates in the same reproducible workflow. As per coding guidelines,Cargo.toml:Always prefer cargo add over manually editing Cargo.toml for managing Rust dependencies.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Cargo.toml` around lines 20 - 30, Revert the manual version edits in Cargo.toml and instead update the specified crates using cargo add so the manifest and Cargo.lock stay consistent: run cargo add for cargo-generate (targeting 0.23.8), serde-saphyr (0.0.23), toml (1.1.2 with serde feature), and toml_edit (0.25.10), then verify the entries for cargo-generate, serde-saphyr, toml and toml_edit in Cargo.toml are the versions added and commit the resulting Cargo.toml and Cargo.lock changes together.crates/cli/src/run/run_loop.rs (1)
33-37: Use the resolved workspace root consistently in this method.Line 33 already captures
workspace_path, but Line 34 and Line 37 immediately call helpers that resolvecurrent_dir()again. Passingworkspace_paththrough here would keep watch-mode state explicit and avoid future drift if anything mutates the process CWD.🤖 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 33 - 37, The method captures workspace_path but then calls helpers that re-resolve the process CWD; update the three calls to use the resolved workspace_path consistently: pass &workspace_path into common::get_config (so it resolves config relative to workspace_path), into common::generate_server_structure(&self.project_name, &self.config_path, &dependencies, &workspace_path) (or adjust its signature to accept workspace_path) and into common::generated_project_dir(&self.project_name, &workspace_path) (and change that function signature if needed); ensure dependencies created via create_dependencies() also resolve any paths using the same workspace_path rather than calling current_dir().
🤖 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/common.rs`:
- Around line 19-20: PathConfigArgs::path currently becomes inconsistent because
parse_and_chdir() returns a PathBuf but later code canonicalizes only
self.config (so relative config paths are resolved against the ambient CWD after
parse_and_chdir() has already changed it), and a relative -p value becomes stale
after the chdir; fix by making parse_and_chdir() return both a
canonical/absolute path (or canonicalize its return) and ensure the code that
canonicalizes config uses the path from PathConfigArgs::path as the base (i.e.,
canonicalize config relative to self.path before any chdir), or alternatively
move the chdir until after both self.path and self.config are canonicalized;
update references to parse_and_chdir, PathConfigArgs::path, and the block that
canonicalizes self.config so the struct remains self-consistent and relative
paths are resolved against the intended directory.
In `@SKILLS.md`:
- Around line 65-69: Update SKILLS.md to make the -p/--path flag documentation
consistent: change the description for the `config mod list` example that
currently says it “defaults to .” so it no longer asserts a default, and update
the quick-reference block where `-p <workspace>` is shown as required (lines
rendering `-p <workspace>`) to show it as optional (e.g., `[-p <workspace>]` or
`-p, --path <PATH>` with the same “optional” wording). Ensure all occurrences of
the `-p, --path <PATH>` flag text match the shared section's wording that it is
optional and has no default so the `config mod list` and quick-reference entries
are consistent with the top-level `-p` description.
---
Nitpick comments:
In `@Cargo.toml`:
- Around line 20-30: Revert the manual version edits in Cargo.toml and instead
update the specified crates using cargo add so the manifest and Cargo.lock stay
consistent: run cargo add for cargo-generate (targeting 0.23.8), serde-saphyr
(0.0.23), toml (1.1.2 with serde feature), and toml_edit (0.25.10), then verify
the entries for cargo-generate, serde-saphyr, toml and toml_edit in Cargo.toml
are the versions added and commit the resulting Cargo.toml and Cargo.lock
changes together.
In `@crates/cli/src/run/run_loop.rs`:
- Around line 33-37: The method captures workspace_path but then calls helpers
that re-resolve the process CWD; update the three calls to use the resolved
workspace_path consistently: pass &workspace_path into common::get_config (so it
resolves config relative to workspace_path), into
common::generate_server_structure(&self.project_name, &self.config_path,
&dependencies, &workspace_path) (or adjust its signature to accept
workspace_path) and into common::generated_project_dir(&self.project_name,
&workspace_path) (and change that function signature if needed); ensure
dependencies created via create_dependencies() also resolve any paths using the
same workspace_path rather than calling current_dir().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 83254423-c3a1-44bc-be2a-01098766bb10
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
Cargo.tomlSKILLS.mdcrates/cli/src/build/mod.rscrates/cli/src/common.rscrates/cli/src/config/modules/add.rscrates/cli/src/config/modules/list.rscrates/cli/src/config/modules/mod.rscrates/cli/src/run/mod.rscrates/cli/src/run/run_loop.rs
💤 Files with no reviewable changes (1)
- crates/cli/src/config/modules/mod.rs
Comment on lines
+19
to
+20
| #[arg(short = 'p', long, value_parser = parse_and_chdir)] | ||
| pub path: Option<PathBuf>, |
Contributor
There was a problem hiding this comment.
PathConfigArgs::path no longer affects config resolution.
Line 40 now canonicalizes only self.config, so any in-process caller that constructs PathConfigArgs { path, config } directly resolves a relative config against the ambient CWD instead of path. Also, if -p was relative, the PathBuf returned from parse_and_chdir() is stale as soon as Line 33 changes the CWD.
💡 One way to keep the struct self-consistent
fn parse_and_chdir(s: &str) -> Result<PathBuf, String> {
- let path = PathBuf::from(s);
+ let path = PathBuf::from(s)
+ .canonicalize()
+ .map_err(|e| format!("failed to access {s}: {e}"))?;
if !path.is_dir() {
return Err(format!("not a directory: {}", path.display()));
}
env::set_current_dir(&path)
.map_err(|e| format!("failed to change directory to {}: {e}", path.display()))?;
Ok(path)
}
impl PathConfigArgs {
pub fn resolve_config(&self) -> anyhow::Result<PathBuf> {
- self.config
+ let config_path = if self.config.is_absolute() {
+ self.config.clone()
+ } else if let Some(path) = &self.path {
+ path.join(&self.config)
+ } else {
+ workspace_root()?.join(&self.config)
+ };
+
+ config_path
.canonicalize()
.context("can't canonicalize config")
}
}Also applies to: 26-37, 40-44
🤖 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 19 - 20, PathConfigArgs::path
currently becomes inconsistent because parse_and_chdir() returns a PathBuf but
later code canonicalizes only self.config (so relative config paths are resolved
against the ambient CWD after parse_and_chdir() has already changed it), and a
relative -p value becomes stale after the chdir; fix by making parse_and_chdir()
return both a canonical/absolute path (or canonicalize its return) and ensure
the code that canonicalizes config uses the path from PathConfigArgs::path as
the base (i.e., canonicalize config relative to self.path before any chdir), or
alternatively move the chdir until after both self.path and self.config are
canonicalized; update references to parse_and_chdir, PathConfigArgs::path, and
the block that canonicalizes self.config so the struct remains self-consistent
and relative paths are resolved against the intended directory.
…ndency Signed-off-by: Bechma <19294519+Bechma@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/module-parser/src/metadata.rs`:
- Around line 25-29: The function get_module_name_from_crate() currently relies
on the process CWD for cargo metadata; change it to accept a workspace path
parameter (e.g., get_module_name_from_crate(workspace: &Path) ->
anyhow::Result<HashMap<String, ConfigModule>>) and call
cargo_metadata::MetadataCommand::new().manifest_path(workspace.join("Cargo.toml"))
(or set current_dir with .current_dir(workspace)) before .exec(); then update
all callers to pass the resolved workspace path from the config loader so
metadata is always queried against the intended workspace rather than the
process CWD.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9f49a435-592f-4c5f-a997-3ae328ed630f
📒 Files selected for processing (5)
SKILLS.mdcrates/cli/src/common.rscrates/cli/src/config/modules/add.rscrates/cli/src/config/modules/list.rscrates/module-parser/src/metadata.rs
✅ Files skipped from review due to trivial changes (3)
- crates/cli/src/config/modules/list.rs
- crates/cli/src/config/modules/add.rs
- SKILLS.md
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/cli/src/common.rs
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
-p/--pathis now optional and can activate a working-directory context when supplied.Documentation
-p/--pathand clarify how it affects config, build, run, and generated output resolution and examples.Chores