fix relative path imports for config path#10
Merged
Bechma merged 1 commit intocyberfabric:mainfrom Mar 9, 2026
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughRefactors CLI config resolution: Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 (3)
crates/cli/src/common.rs (2)
29-33: Consider improving the error message for workspace path resolution.Similar to the config path, the error message could be more descriptive to help users understand what went wrong.
Suggested improvement
pub fn resolve_path(&self) -> anyhow::Result<PathBuf> { self.path .canonicalize() - .context("can't canonicalize workspace path") + .with_context(|| format!( + "workspace path '{}' not found or inaccessible", + self.path.display() + )) }🤖 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 29 - 33, The current resolve_path method returns a generic "can't canonicalize workspace path" error; update resolve_path to include the failing path value and more context by wrapping the canonicalize error with a descriptive message (e.g., include self.path.display() or similar) so the process that calls resolve_path sees which path failed and why; locate the resolve_path function and modify the .context(...) call around self.path.canonicalize() to include the path and a short human-readable message referencing workspace path.
23-27:canonicalize()fails if the config file doesn't exist.The
canonicalize()call will return an error if the file path doesn't exist on disk. Whileload_config()also requires the file to exist, this creates an unclear error message ("can't canonicalize config") when users provide a path to a non-existent config file.Consider providing a more helpful error message that guides users:
Suggested improvement for error clarity
pub fn resolve_config(&self) -> anyhow::Result<PathBuf> { self.config .canonicalize() - .context("can't canonicalize config") + .with_context(|| format!( + "config file '{}' not found or inaccessible", + self.config.display() + )) }🤖 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 23 - 27, The resolve_config method currently calls self.config.canonicalize() which fails with an unclear "can't canonicalize config" when the path doesn't exist; change resolve_config to first check existence (e.g., self.config.exists() or std::fs::metadata) and return a clear error like "config file not found: <path>" if missing, otherwise call canonicalize and keep a contextual error for canonicalization failures; reference resolve_config and the canonicalize() call so the check and improved error message are applied there (and keep behavior consistent with load_config which also expects the file to exist).crates/cli/src/config/modules/mod.rs (1)
117-120: Inconsistent path resolution:workspace_pathis not canonicalized.The
config_pathis resolved viaresolve_config()which canonicalizes it, butworkspace_pathuses a direct clone without canonicalization. This could lead to inconsistent behavior when relative paths are provided.Consider using the new
resolve_path()helper for consistency:Suggested fix
pub(super) fn resolve_modules_context( path_config: &PathConfigArgs, ) -> anyhow::Result<ModulesContext> { Ok(ModulesContext { - workspace_path: path_config.path.clone(), + workspace_path: path_config.resolve_path()?, config_path: path_config.resolve_config()?, }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/config/modules/mod.rs` around lines 117 - 120, ModulesContext is constructed with a non-canonicalized workspace_path (path_config.path.clone()) while config_path uses path_config.resolve_config(); change workspace_path to use the same canonicalization helper by calling path_config.resolve_path() (or the new resolve_path() helper) so both paths are consistently resolved; update the ModulesContext construction to assign workspace_path from the resolved result and handle the returned Result/err as resolve_config() does.
🤖 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/common.rs`:
- Around line 29-33: The current resolve_path method returns a generic "can't
canonicalize workspace path" error; update resolve_path to include the failing
path value and more context by wrapping the canonicalize error with a
descriptive message (e.g., include self.path.display() or similar) so the
process that calls resolve_path sees which path failed and why; locate the
resolve_path function and modify the .context(...) call around
self.path.canonicalize() to include the path and a short human-readable message
referencing workspace path.
- Around line 23-27: The resolve_config method currently calls
self.config.canonicalize() which fails with an unclear "can't canonicalize
config" when the path doesn't exist; change resolve_config to first check
existence (e.g., self.config.exists() or std::fs::metadata) and return a clear
error like "config file not found: <path>" if missing, otherwise call
canonicalize and keep a contextual error for canonicalization failures;
reference resolve_config and the canonicalize() call so the check and improved
error message are applied there (and keep behavior consistent with load_config
which also expects the file to exist).
In `@crates/cli/src/config/modules/mod.rs`:
- Around line 117-120: ModulesContext is constructed with a non-canonicalized
workspace_path (path_config.path.clone()) while config_path uses
path_config.resolve_config(); change workspace_path to use the same
canonicalization helper by calling path_config.resolve_path() (or the new
resolve_path() helper) so both paths are consistently resolved; update the
ModulesContext construction to assign workspace_path from the resolved result
and handle the returned Result/err as resolve_config() does.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c984176e-4f7c-47c4-ad01-0deb0c87469a
📒 Files selected for processing (6)
crates/cli/src/build/mod.rscrates/cli/src/common.rscrates/cli/src/config/db/mod.rscrates/cli/src/config/modules/add.rscrates/cli/src/config/modules/mod.rscrates/cli/src/run/mod.rs
Signed-off-by: Bechma <19294519+Bechma@users.noreply.github.com>
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 17-19: The CLI struct change made the config field mandatory (pub
config: PathBuf), breaking existing invocations; revert config back to
Option<PathBuf> and restore the previous fallback logic in
resolve_workspace_and_config() so that when config is None it uses
DEFAULT_CONFIG_FILE (or the same default path previously used), or alternatively
clearly document the breaking change—specifically, change the CLI struct's
config field back to Option<PathBuf> and update the call sites and
resolve_workspace_and_config() to accept Option<PathBuf> and apply the default
when None.
- Around line 23-26: The current resolve_config() uses
self.config.canonicalize(), which produces Windows verbatim paths (\\?\...) that
break when later embedded and normalized in prepare_cargo_server_main(); change
resolve_config to stop canonicalizing and instead return the original path
(e.g., return self.config.clone()/to_path_buf()) so the path string remains in
the same platform-native form the template serialization expects; reference
resolve_config and prepare_cargo_server_main and ensure any code that embeds the
path (the generated main.rs that calls PathBuf::from(...)) receives the
unmodified path string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f0cc232c-59ae-4569-9f8c-ff5365551969
📒 Files selected for processing (6)
crates/cli/src/build/mod.rscrates/cli/src/common.rscrates/cli/src/config/db/mod.rscrates/cli/src/config/modules/add.rscrates/cli/src/config/modules/mod.rscrates/cli/src/run/mod.rs
✅ Files skipped from review due to trivial changes (1)
- crates/cli/src/config/modules/add.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/cli/src/config/modules/mod.rs
- crates/cli/src/config/db/mod.rs
- crates/cli/src/build/mod.rs
- crates/cli/src/run/mod.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