feat: docker template command resolves #17#25
feat: docker template command resolves #17#25devjow wants to merge 4 commits intocyberfabric:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI Deploy<br/>Handler
participant WS as Workspace<br/>Resolver
participant FS as File System<br/>Copy & Validation
participant GEN as Server<br/>Generator
participant TPL as Template<br/>Fetcher/Renderer
participant OUT as Bundle<br/>Output
CLI->>WS: Resolve workspace root, config path, project name
WS-->>CLI: workspace_root, config_path, project_name
CLI->>FS: Compute & canonicalize output dir, validate reserved paths
FS-->>CLI: Safe output path or error
CLI->>FS: Collect workspace members & path deps
FS->>FS: Recursively copy inputs, filter junk, reject symlinks
FS-->>CLI: Local paths copied into bundle
CLI->>GEN: Generate server project at bundle output root
GEN-->>CLI: Server project created inside bundle
CLI->>TPL: Fetch template source (local or git) and prepare values
TPL-->>CLI: Template files available
CLI->>TPL: Render templates with context (name, paths, otel)
TPL-->>CLI: Rendered artifacts (e.g., Dockerfile)
CLI->>OUT: Write rendered files and finalize bundle
OUT-->>CLI: Print bundle location
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/cli/src/deploy/mod.rs (1)
15-17: Consider adding more filtered entries mentioned in README.The README mentions filtering
.env*files, swap files, and Finder metadata. The constantFILTERED_ENTRY_NAMEScovers some but the additional patterns are handled inshould_skip_bundle_entry. This split is fine, but consider documenting the full filter logic in a comment here for maintainability.📝 Suggested documentation comment
+// Additional patterns are handled in `should_skip_bundle_entry`: +// - `.env*` files (via starts_with check) +// - `*.swp` and `*~` swap/backup files (via ends_with checks) const FILTERED_ENTRY_NAMES: &[&str] = &[".DS_Store", ".git", ".github", ".idea", ".vscode", "target"];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/deploy/mod.rs` around lines 15 - 17, Add a short documentation comment above the FILTERED_ENTRY_NAMES constant that summarizes the complete bundle filtering logic and points readers to should_skip_bundle_entry for additional patterns; explicitly mention that README-listed filters like ".env*" files, editor swap/temporary files (e.g. trailing ~ and .swp/.swpx), and macOS Finder metadata are handled by should_skip_bundle_entry so maintainers know the full set of exclusions without hunting through the code.
🤖 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 122-124: The code is referencing a non-existent
config.opentelemetry field; update references to use the actual top-level
tracing field: replace config.opentelemetry.tracing.enabled with
config.tracing.enabled and pass &config.tracing (not &config.opentelemetry) to
the telemetry init function (modkit::telemetry::init::init_tracing) when
constructing otel_layer and in the other occurrence around the block at lines
138-140 so callers expect the TracingConfig type.
In `@crates/cli/src/deploy/mod.rs`:
- Around line 460-467: The template context is hardcoding
config.opentelemetry.tracing.enabled: true which can drift from the real runtime
config; either populate that key from the actual runtime configuration value
(read the existing runtime config object instead of using a literal) or, if it
is intentionally fixed for template rendering only, add an inline comment near
the template context insertion explaining why this override is necessary and
referencing the related common.rs discrepancy; update the code that builds the
template context (the block creating the "config" -> "opentelemetry" ->
"tracing" -> "enabled" entry) to use the runtime value or document the intent.
---
Nitpick comments:
In `@crates/cli/src/deploy/mod.rs`:
- Around line 15-17: Add a short documentation comment above the
FILTERED_ENTRY_NAMES constant that summarizes the complete bundle filtering
logic and points readers to should_skip_bundle_entry for additional patterns;
explicitly mention that README-listed filters like ".env*" files, editor
swap/temporary files (e.g. trailing ~ and .swp/.swpx), and macOS Finder metadata
are handled by should_skip_bundle_entry so maintainers know the full set of
exclusions without hunting through the code.
🪄 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: 66b26242-0165-4ab9-8ed3-f5bd47148f05
📒 Files selected for processing (5)
README.mdSKILLS.mdcrates/cli/src/common.rscrates/cli/src/deploy/mod.rscrates/cli/src/lib.rs
Bechma
left a comment
Bechma
left a comment
There was a problem hiding this comment.
What about of including an automated build of the image in case docker command is available? (Otherwise the user can be informed that the Dockerfile is created but docker is not available)
crates/cli/src/deploy/mod.rs
Outdated
| use crate::common::{self, PathConfigArgs}; | ||
| use anyhow::{Context, bail}; | ||
| use clap::{Args, ValueEnum}; | ||
| use liquid::ParserBuilder; |
There was a problem hiding this comment.
Instead of using liquid, use cargo_generate as in mod init and mod add
You don't need to deal with git cloning and all that stuff.
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/deploy/mod.rs`:
- Around line 277-296: copy_file and copy_optional_file currently allow
symlinked sources; add the same symlink guard used in copy_path_recursively() so
symlinks are rejected. In both functions, call source.symlink_metadata() and
check file_type().is_symlink() and return an error (or mirror the exact error
handling used by copy_path_recursively()) when a symlink is detected; keep
copy_optional_file's existing Ok(false) for non-files but ensure it errors on
symlinks, and have copy_file fail early on symlinks before creating parent dirs
or copying.
- Around line 363-389: read_workspace_members currently parses Cargo.toml and
treats workspace.members as literal paths, which fails for glob entries like
"crates/*"; replace the TOML parsing logic in read_workspace_members with
cargo_metadata::MetadataCommand to load the workspace metadata for the
workspace_root manifest, iterate metadata.workspace_members (or
metadata.packages filtered by workspace_root) to collect each package's
manifest_path parent or package root, convert those to workspace-relative
PathBufs (using resolve_workspace_relative_path or similar) and insert them into
the BTreeSet; ensure you handle errors from MetadataCommand and preserve the
function signature and return type.
🪄 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: fe44375f-3f78-4e46-b917-be824f08896c
📒 Files selected for processing (4)
crates/cli/src/build/mod.rscrates/cli/src/common.rscrates/cli/src/deploy/mod.rscrates/cli/src/run/run_loop.rs
| fn read_workspace_members(workspace_root: &Path) -> anyhow::Result<BTreeSet<PathBuf>> { | ||
| let cargo_toml_path = workspace_root.join("Cargo.toml"); | ||
| let raw = fs::read_to_string(&cargo_toml_path) | ||
| .with_context(|| format!("can't read {}", cargo_toml_path.display()))?; | ||
| let doc = raw | ||
| .parse::<toml_edit::DocumentMut>() | ||
| .with_context(|| format!("can't parse {}", cargo_toml_path.display()))?; | ||
|
|
||
| let members = doc["workspace"]["members"] | ||
| .as_array() | ||
| .map(|array| { | ||
| array | ||
| .iter() | ||
| .filter_map(toml_edit::Value::as_str) | ||
| .map(PathBuf::from) | ||
| .collect::<Vec<_>>() | ||
| }) | ||
| .unwrap_or_default(); | ||
|
|
||
| let mut resolved = BTreeSet::new(); | ||
| for member in members { | ||
| let relative = resolve_workspace_relative_path(workspace_root, &member)?; | ||
| if !relative.as_os_str().is_empty() { | ||
| resolved.insert(relative); | ||
| } | ||
| } | ||
| Ok(resolved) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n crates/cli/src/deploy/mod.rs | sed -n '363,389p'Repository: cyberfabric/cf-cli
Length of output: 1241
🏁 Script executed:
# Check the full function and surrounding context
cat -n crates/cli/src/deploy/mod.rs | sed -n '350,400p'Repository: cyberfabric/cf-cli
Length of output: 2036
🏁 Script executed:
# Look for the resolve_workspace_relative_path function
rg "fn resolve_workspace_relative_path" -A 15Repository: cyberfabric/cf-cli
Length of output: 1081
🏁 Script executed:
# Check what crate dependencies are available, especially Cargo-related
cat crates/cli/Cargo.tomlRepository: cyberfabric/cf-cli
Length of output: 1123
🏁 Script executed:
# Search for any glob-related imports or usage in the deploy module
rg "glob" crates/cli/src/deploy/ -iRepository: cyberfabric/cf-cli
Length of output: 44
🏁 Script executed:
# Check for any tests of this function
rg "read_workspace_members" -B 3 -A 3Repository: cyberfabric/cf-cli
Length of output: 1089
🏁 Script executed:
# Look for cargo metadata usage elsewhere in the codebase
rg "cargo.*metadata\|workspace.*metadata" -iRepository: cyberfabric/cf-cli
Length of output: 44
🏁 Script executed:
# Check Cargo.toml for workspace config - see if this repo uses globs
cat Cargo.toml | head -50Repository: cyberfabric/cf-cli
Length of output: 1437
🏁 Script executed:
# Check if cargo_metadata is already used in the codebase
rg "cargo_metadata\|MetadataCommand" -B 2 -A 5Repository: cyberfabric/cf-cli
Length of output: 44
🏁 Script executed:
# Check the resolve_workspace_relative_path full implementation
cat -n crates/cli/src/deploy/mod.rs | sed -n '392,420p'Repository: cyberfabric/cf-cli
Length of output: 1202
🏁 Script executed:
# Search for any glob expansion or pattern matching in the deploy module
rg "glob|glob_expansion|\*\*" crates/cli/src/deploy/mod.rsRepository: cyberfabric/cf-cli
Length of output: 44
🏁 Script executed:
# Check the actual error behavior - see what happens if we trace back the call path
rg "read_workspace_members|read_deploy_dir_members" -B 3 -A 1 crates/cli/src/deploy/mod.rsRepository: cyberfabric/cf-cli
Length of output: 447
🏁 Script executed:
# Look for any integration tests or examples that might show glob usage
find . -name "*.rs" -path "*/test*" -o -name "*test*.rs" | head -20Repository: cyberfabric/cf-cli
Length of output: 131
🏁 Script executed:
# Verify Cargo documentation claim about glob patterns in workspace.members
web_search
Cargo workspace members glob patterns support documentationRepository: cyberfabric/cf-cli
Length of output: 156
🌐 Web query:
Cargo workspace members glob patterns support
💡 Result:
Cargo does support glob patterns in a workspace’s member list.
- In the workspace root
Cargo.toml,[workspace].members(and[workspace].exclude) accept glob patterns; the Cargo Book explicitly mentions “typical filename glob patterns like*and?”. [1] - Cargo’s workspace globbing is based on the
globcrate’s syntax, which supports:?(single character)*(any sequence within a path component)**(recursive: “current directory and arbitrary subdirectories”, as its own path component)- character classes like
[0-9]/[!a][2]
Example:
[workspace]
members = ["crates/*", "tools/**"]
exclude = ["crates/experimental/*"]Notes: The strings are paths (relative to the workspace root) that must ultimately resolve to directories containing a Cargo.toml for them to be valid workspace members. [1]
Sources: [1] [2]
🏁 Script executed:
# Check if there are any tests for the deploy module
cat crates/cli/src/test/mod.rs | head -50Repository: cyberfabric/cf-cli
Length of output: 349
🏁 Script executed:
# Search for how this function is called and in what context
rg "read_deploy_dir_members|get_workspace_member_paths" -B 5 -A 5Repository: cyberfabric/cf-cli
Length of output: 44
🏁 Script executed:
# Check if there are any workspaces in the test data or fixtures that use glob patterns
find . -name "Cargo.toml" -exec grep -l "members.*\*" {} \;Repository: cyberfabric/cf-cli
Length of output: 44
🏁 Script executed:
# Get more context on how read_deploy_dir_members is called
rg "read_deploy_dir_members|get_workspace_member_paths" -B 10 -A 2 crates/cli/src/Repository: cyberfabric/cf-cli
Length of output: 44
🏁 Script executed:
# Check the cargo_metadata version and see if it's being used anywhere
rg "cargo_metadata\|MetadataCommand\|Metadata::" crates/Repository: cyberfabric/cf-cli
Length of output: 44
🏁 Script executed:
# Look at the broader deploy context - where is read_workspace_members used?
cat -n crates/cli/src/deploy/mod.rs | sed -n '340,365p'Repository: cyberfabric/cf-cli
Length of output: 1106
🏁 Script executed:
# Let's check what error message would be produced
cat -n crates/cli/src/deploy/mod.rs | sed -n '400,405p'Repository: cyberfabric/cf-cli
Length of output: 322
🏁 Script executed:
# Check if there's any actual glob expansion happening anywhere in the codebase
rg "glob::|GlobBuilder|Pattern" crates/Repository: cyberfabric/cf-cli
Length of output: 44
🏁 Script executed:
# Check the structure of how deploy is called to understand the impact
rg "collect_required_local_paths" -B 5 -A 10Repository: cyberfabric/cf-cli
Length of output: 4241
🏁 Script executed:
# Check what the test looks like to confirm it only tests literal paths
cat -n crates/cli/src/deploy/mod.rs | sed -n '430,480p'Repository: cyberfabric/cf-cli
Length of output: 1897
🏁 Script executed:
# Get the actual test that validates workspace members
cat -n crates/cli/src/deploy/mod.rs | sed -n '495,545p'Repository: cyberfabric/cf-cli
Length of output: 2224
Use Cargo's metadata API to read workspace members instead of raw TOML parsing.
The function treats all workspace.members entries as literal directory paths and passes them directly to canonicalize(). However, Cargo supports glob patterns like "crates/*" in the members list. When a glob pattern string is passed to canonicalize(), it fails with "can't canonicalize" because the pattern doesn't exist as a literal directory. This breaks cf deploy on any valid workspace using glob patterns.
Since cargo_metadata (0.23.1) is already a dependency, use cargo_metadata::MetadataCommand to read the workspace—it automatically resolves globs and handles Cargo's manifest semantics correctly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/cli/src/deploy/mod.rs` around lines 363 - 389, read_workspace_members
currently parses Cargo.toml and treats workspace.members as literal paths, which
fails for glob entries like "crates/*"; replace the TOML parsing logic in
read_workspace_members with cargo_metadata::MetadataCommand to load the
workspace metadata for the workspace_root manifest, iterate
metadata.workspace_members (or metadata.packages filtered by workspace_root) to
collect each package's manifest_path parent or package root, convert those to
workspace-relative PathBufs (using resolve_workspace_relative_path or similar)
and insert them into the BTreeSet; ensure you handle errors from MetadataCommand
and preserve the function signature and return type.
devjow
force-pushed
the
feat/cmd-docker-template
branch
from
3ac5ae2 to
4c750b4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
crates/cli/src/deploy/mod.rs (2)
362-389:⚠️ Potential issue | 🟠 Major
workspace.membersneeds Cargo’s resolver.
members = ["crates/*"]is valid Cargo, but this helper treats each entry as a literal directory and immediatelycanonicalize()s it.cf deploywill fail on a supported workspace layout. Please switch this tocargo_metadata::MetadataCommandand add a regression test with a globbed member.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/deploy/mod.rs` around lines 362 - 389, The current read_workspace_members function treats workspace.members entries as literal paths and fails on globbed entries like "crates/*"; replace its implementation to call cargo_metadata::MetadataCommand to obtain the resolved workspace member package paths (use MetadataCommand::new().manifest_path(workspace_root.join("Cargo.toml")) and read the workspace_members/package.manifest_path results), convert those manifest paths into workspace-relative directories (using resolve_workspace_relative_path as needed) instead of parsing Cargo.toml directly, and update or add a regression test that creates a workspace with a globbed member (e.g., members = ["crates/*"]) to verify cf deploy succeeds.
276-295:⚠️ Potential issue | 🔴 CriticalReject symlinked top-level inputs before copying.
copy_path_recursively()blocks symlinks, butcopy_optional_file()/copy_file()still accept a symlinkedCargo.toml,Cargo.lock, orconfig.yml:Path::is_file()follows the link, andfs::copy()copies the target. That bypasses the deploy bundle’s current safety model.Suggested fix
fn copy_optional_file(source: &Path, destination: &Path) -> anyhow::Result<bool> { - if !source.is_file() { + let metadata = match fs::symlink_metadata(source) { + Ok(metadata) => metadata, + Err(err) if err.kind() == std::io::ErrorKind::NotFound => return Ok(false), + Err(err) => return Err(err).with_context(|| format!("can't inspect {}", source.display())), + }; + if metadata.file_type().is_symlink() { + bail!( + "symlinked paths are not supported in deploy bundles: {}", + source.display() + ); + } + if !metadata.is_file() { return Ok(false); } copy_file(source, destination)?; Ok(true) } fn copy_file(source: &Path, destination: &Path) -> anyhow::Result<()> { + let metadata = fs::symlink_metadata(source) + .with_context(|| format!("can't inspect {}", source.display()))?; + if metadata.file_type().is_symlink() { + bail!( + "symlinked paths are not supported in deploy bundles: {}", + source.display() + ); + } + if let Some(parent) = destination.parent() { fs::create_dir_all(parent).with_context(|| format!("can't create {}", parent.display()))?; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/deploy/mod.rs` around lines 276 - 295, The code currently follows symlinks because Path::is_file and fs::copy follow links; update copy_optional_file and copy_file to first call std::fs::symlink_metadata (or Path::symlink_metadata) and reject any source whose metadata.file_type().is_symlink() (return an error with a clear message rather than copying or treating it as a normal file); also use the metadata.is_file() from symlink_metadata to decide existence instead of Path::is_file so you don't follow links, and keep creating parent dirs and copying only after the symlink check passes.
🧹 Nitpick comments (1)
crates/cli/src/deploy/mod.rs (1)
455-458: Reusecommon::BASE_PATHforgenerated_project_dir.This is the same path the rest of the module derives from
common::BASE_PATH. Keeping a string literal here creates an easy drift point if the generated project root ever changes.Suggested cleanup
- "generated_project_dir": format!(".cyberfabric/{project_name}"), + "generated_project_dir": format!("{}/{project_name}", common::BASE_PATH),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/deploy/mod.rs` around lines 455 - 458, The generated_project_dir field in the liquid::object! block currently hardcodes ".cyberfabric/{project_name}"; change it to build the path using common::BASE_PATH and project_name (e.g., join or format with common::BASE_PATH and project_name) so it reuses the module-wide constant (replace the format!(".cyberfabric/{project_name}") expression for generated_project_dir in the liquid::object! block with a value derived from common::BASE_PATH and project_name).
🤖 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/deploy/mod.rs`:
- Around line 708-709: The test sets the process CWD via
std::env::set_current_dir(workspace_root) but never restores it, causing CWD
leakage; wrap the change in an RAII guard (e.g., implement or reuse a
CurrentDirGuard) that captures std::env::current_dir() before calling
set_current_dir(workspace_root) and restores the original directory in Drop so
the CWD is always reset when the test exits; update the test to create this
guard immediately after calling set_current_dir(workspace_root) (referencing the
existing set_current_dir(workspace_root) call and workspace_root variable) so
parallel tests won’t inherit a deleted or changed CWD.
---
Duplicate comments:
In `@crates/cli/src/deploy/mod.rs`:
- Around line 362-389: The current read_workspace_members function treats
workspace.members entries as literal paths and fails on globbed entries like
"crates/*"; replace its implementation to call cargo_metadata::MetadataCommand
to obtain the resolved workspace member package paths (use
MetadataCommand::new().manifest_path(workspace_root.join("Cargo.toml")) and read
the workspace_members/package.manifest_path results), convert those manifest
paths into workspace-relative directories (using resolve_workspace_relative_path
as needed) instead of parsing Cargo.toml directly, and update or add a
regression test that creates a workspace with a globbed member (e.g., members =
["crates/*"]) to verify cf deploy succeeds.
- Around line 276-295: The code currently follows symlinks because Path::is_file
and fs::copy follow links; update copy_optional_file and copy_file to first call
std::fs::symlink_metadata (or Path::symlink_metadata) and reject any source
whose metadata.file_type().is_symlink() (return an error with a clear message
rather than copying or treating it as a normal file); also use the
metadata.is_file() from symlink_metadata to decide existence instead of
Path::is_file so you don't follow links, and keep creating parent dirs and
copying only after the symlink check passes.
---
Nitpick comments:
In `@crates/cli/src/deploy/mod.rs`:
- Around line 455-458: The generated_project_dir field in the liquid::object!
block currently hardcodes ".cyberfabric/{project_name}"; change it to build the
path using common::BASE_PATH and project_name (e.g., join or format with
common::BASE_PATH and project_name) so it reuses the module-wide constant
(replace the format!(".cyberfabric/{project_name}") expression for
generated_project_dir in the liquid::object! block with a value derived from
common::BASE_PATH and project_name).
🪄 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: 5204815f-92a0-406a-b30e-f6402d7096b3
📒 Files selected for processing (5)
README.mdSKILLS.mdcrates/cli/src/common.rscrates/cli/src/deploy/mod.rscrates/cli/src/lib.rs
✅ Files skipped from review due to trivial changes (2)
- README.md
- SKILLS.md
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/cli/src/lib.rs
- crates/cli/src/common.rs
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
crates/cli/src/deploy/mod.rs (2)
370-396:⚠️ Potential issue | 🟠 MajorUse Cargo’s workspace view here instead of re-parsing
workspace.members.Line 378 only handles literal string entries, so valid workspaces that use member globs are missed, and Line 392 silently drops the workspace root if it is a member. That can make
cf deployfail or produce incomplete bundles for perfectly normal Cargo workspaces. Please resolve members through Cargo metadata instead of interpretingCargo.tomlby hand.Cargo workspace.members glob patterns support and whether a workspace root package is automatically a workspace member🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/deploy/mod.rs` around lines 370 - 396, The function read_workspace_members currently re-parses Cargo.toml and only handles literal strings, missing globbed members and silently dropping the workspace root; replace its logic with a Cargo metadata-based resolution: call cargo_metadata::MetadataCommand (or use cargo metadata / workspace view) to obtain the workspace members and resolve each package's manifest_path into a PathBuf, then convert those to workspace-relative paths using resolve_workspace_relative_path; ensure globbed members and the workspace-root package (if present in metadata) are included and return the collected BTreeSet<PathBuf> accordingly.
276-281:⚠️ Potential issue | 🟡 MinorReject optional symlinks before treating them as “missing”.
Line 277 uses
Path::is_file(), which follows symlinks. A broken or directory symlink atCargo.lockwill returnfalsehere and be silently skipped, even though the rest of the bundle code rejects symlinks. Inspect the path withsymlink_metadata()first so every symlink still fails fast.🛡️ Suggested fix
fn copy_optional_file(source: &Path, destination: &Path) -> anyhow::Result<bool> { - if !source.is_file() { - return Ok(false); - } + let metadata = match fs::symlink_metadata(source) { + Ok(metadata) => metadata, + Err(err) if err.kind() == std::io::ErrorKind::NotFound => return Ok(false), + Err(err) => return Err(err).with_context(|| format!("can't inspect {}", source.display())), + }; + if metadata.file_type().is_symlink() { + bail!( + "symlinked paths are not supported in deploy bundles: {}", + source.display() + ); + } + if !metadata.is_file() { + return Ok(false); + } copy_file(source, destination)?; Ok(true) }Rust std::path::Path::is_file symlink behavior and the difference between metadata() and symlink_metadata()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/deploy/mod.rs` around lines 276 - 281, The copy_optional_file function currently calls Path::is_file() which follows symlinks and silently skips broken or directory symlinks; change it to use source.symlink_metadata() to inspect the file_type(), explicitly error if file_type().is_symlink() (or otherwise reject symlinks), return Ok(false) only when the path truly does not exist or is not a regular file, and proceed to call copy_file(source, destination)? when metadata indicates a regular file; reference the function copy_optional_file and ensure any symlink-related errors propagate as an anyhow::Result::Err instead of being treated as “missing.”
🤖 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/deploy/mod.rs`:
- Around line 198-207: The template_dir function currently joins subfolder
directly and can be escaped via absolute paths or ../ segments; fix it by
canonicalizing the root and the resolved dir (use std::fs::canonicalize) and
validate that the canonicalized dir starts_with the canonicalized root,
returning an error if not; additionally ensure reject_symlink() is invoked on
the final template directory (and in render_templates() before reading any
template files) to prevent symlink escapes when loading templates from
template.as_str() and subfolder.
---
Duplicate comments:
In `@crates/cli/src/deploy/mod.rs`:
- Around line 370-396: The function read_workspace_members currently re-parses
Cargo.toml and only handles literal strings, missing globbed members and
silently dropping the workspace root; replace its logic with a Cargo
metadata-based resolution: call cargo_metadata::MetadataCommand (or use cargo
metadata / workspace view) to obtain the workspace members and resolve each
package's manifest_path into a PathBuf, then convert those to workspace-relative
paths using resolve_workspace_relative_path; ensure globbed members and the
workspace-root package (if present in metadata) are included and return the
collected BTreeSet<PathBuf> accordingly.
- Around line 276-281: The copy_optional_file function currently calls
Path::is_file() which follows symlinks and silently skips broken or directory
symlinks; change it to use source.symlink_metadata() to inspect the file_type(),
explicitly error if file_type().is_symlink() (or otherwise reject symlinks),
return Ok(false) only when the path truly does not exist or is not a regular
file, and proceed to call copy_file(source, destination)? when metadata
indicates a regular file; reference the function copy_optional_file and ensure
any symlink-related errors propagate as an anyhow::Result::Err instead of being
treated as “missing.”
🪄 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: 7ad3daa4-6b0a-49f9-a40a-6a3fcd70496d
📒 Files selected for processing (1)
crates/cli/src/deploy/mod.rs
crates/cli/src/deploy/mod.rs
Outdated
| fn template_dir( | ||
| &self, | ||
| subfolder: &str, | ||
| template: DeployTemplateKind, | ||
| ) -> anyhow::Result<PathBuf> { | ||
| let dir = self.root.join(subfolder).join(template.as_str()); | ||
| if !dir.is_dir() { | ||
| bail!("deploy template directory not found at {}", dir.display()); | ||
| } | ||
| Ok(dir) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Rust PathBuf::join behavior with absolute paths and recommended way to ensure a resolved path stays under a base directory
💡 Result:
PathBuf::join with absolute paths
Path::join (and PathBuf::push, which join uses) does not always “append”. If the argument is absolute, it replaces the base path (with some Windows-specific nuances). Example from the standard docs: Path::new("/etc").join("/bin/sh") == "/bin/sh". [1]
Recommended pattern to keep a resolved path under a base directory (and avoid traversal)
If you need “this user-supplied path must stay inside base/”, do not rely on join alone. Common approach:
- Reject absolute input early (and often also reject any
..components, depending on your needs). - Resolve the final path, then verify it’s still under the base using
strip_prefix/starts_with.
use std::{fs, io, path::{Path, PathBuf, Component}};
fn resolve_under_base(base: &Path, user: &Path) -> io::Result<PathBuf> {
// 1) Reject obvious escapes
if user.is_absolute() || user.components().any(|c| matches!(c, Component::ParentDir)) {
return Err(io::Error::new(io::ErrorKind::InvalidInput, "invalid path"));
}
// 2) Resolve symlinks + normalize, then enforce containment
let base_canon = fs::canonicalize(base)?; // resolves symlinks [2]
let cand_canon = fs::canonicalize(base_canon.join(user))?; // join + resolve [1][2]
cand_canon
.strip_prefix(&base_canon) // fails if not contained [3]
.map_err(|_| io::Error::new(io::ErrorKind::PermissionDenied, "path escapes base"))?;
Ok(cand_canon)
}Notes / pitfalls
fs::canonicalizeresolves symlinks and requires the path to exist. [2]- If you need to handle non-existent paths (e.g., creating a new file),
std::path::absolutecan make a path absolute without filesystem access, but it doesn’t resolve symlinks, so it’s not sufficient by itself to prevent symlink-based escapes. [4] - For strongest protection (and to reduce TOCTOU issues), consider capability / “openat”-style APIs that resolve paths relative to an already-open directory handle (rather than checking strings and then opening). Rust ecosystems like WASI-style
Dircapability objects and “open_at” patterns exist for this. [5]
Sources:
[1] Rust Path::join docs (absolute path replaces base; examples) (doc.rust-lang.org)
[2] Rust std::fs::canonicalize docs (canonical absolute form; resolves symlinks) (doc.rust-lang.org)
[3] Rust Path::strip_prefix docs (fails if base not a prefix) (doc.rust-lang.org)
[4] Rust std::path::absolute docs (no FS access; no symlink resolution) (doc.rust-lang.org)
[5] Capability / open-at style directory APIs (example: WASI Dir-style capability model) (docs.wasmtime.dev)
Citations:
- 1: https://doc.rust-lang.org/stable/std/path/struct.Path.html
- 2: https://doc.rust-lang.org/stable/std/fs/fn.canonicalize.html
- 3: https://doc.rust-lang.org/nightly/std/path/struct.Path.html
- 4: https://doc.rust-lang.org/stable/std/path/fn.absolute.html
- 5: https://docs.wasmtime.dev/api/wasi_common/sync/struct.Dir.html?utm_source=openai
🏁 Script executed:
# First, let's view the template_dir function and surrounding context
head -n 210 crates/cli/src/deploy/mod.rs | tail -n 50Repository: cyberfabric/cf-cli
Length of output: 1786
🏁 Script executed:
# Now let's see the template file reading code around line 491-493
sed -n '485,510p' crates/cli/src/deploy/mod.rsRepository: cyberfabric/cf-cli
Length of output: 1188
🏁 Script executed:
# Search for any existing symlink handling or validation in the file
rg -n "symlink|canonicalize|strip_prefix|starts_with" crates/cli/src/deploy/mod.rsRepository: cyberfabric/cf-cli
Length of output: 1919
🏁 Script executed:
# Check if there's a reject_symlink function mentioned in the suggested fix
rg -n "reject_symlink" crates/cli/src/Repository: cyberfabric/cf-cli
Length of output: 273
🏁 Script executed:
# Find DeployTemplateKind definition
rg -n "enum DeployTemplateKind|DeployTemplateKind" crates/cli/src/deploy/mod.rs | head -20Repository: cyberfabric/cf-cli
Length of output: 356
🏁 Script executed:
# Get more context around where template_dir is called and how templates are processed
sed -n '470,520p' crates/cli/src/deploy/mod.rsRepository: cyberfabric/cf-cli
Length of output: 1966
Prevent path traversal and symlink escapes in template directory resolution.
The template_dir() function joins subfolder without validating containment. An absolute or ../-relative subfolder bypasses the root, and symlinked template files in render_templates() (line 492) read arbitrary host files into the bundle.
Canonicalize the resolved template directory and verify it starts with the root. Use the existing reject_symlink() function (already defined in this file) before reading template files.
Suggested hardening
fn template_dir(
&self,
subfolder: &str,
template: DeployTemplateKind,
) -> anyhow::Result<PathBuf> {
- let dir = self.root.join(subfolder).join(template.as_str());
- if !dir.is_dir() {
+ let root = self
+ .root
+ .canonicalize()
+ .with_context(|| format!("can't canonicalize {}", self.root.display()))?;
+ let dir = root.join(subfolder).join(template.as_str());
+ let dir = dir
+ .canonicalize()
+ .with_context(|| format!("can't canonicalize {}", dir.display()))?;
+ if !dir.starts_with(&root) {
+ bail!(
+ "deploy template directory escapes the template checkout root: {}",
+ dir.display()
+ );
+ }
+ if !dir.is_dir() {
bail!("deploy template directory not found at {}", dir.display());
}
Ok(dir)
}
@@
for (template_name, output_name) in TEMPLATE_FILE_PAIRS {
let template_path = template_dir.join(template_name);
+ reject_symlink(&template_path)?;
let template_source = fs::read_to_string(&template_path)Also applies to: lines 489–493
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/cli/src/deploy/mod.rs` around lines 198 - 207, The template_dir
function currently joins subfolder directly and can be escaped via absolute
paths or ../ segments; fix it by canonicalizing the root and the resolved dir
(use std::fs::canonicalize) and validate that the canonicalized dir starts_with
the canonicalized root, returning an error if not; additionally ensure
reject_symlink() is invoked on the final template directory (and in
render_templates() before reading any template files) to prevent symlink escapes
when loading templates from template.as_str() and subfolder.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
crates/cli/src/deploy/mod.rs (2)
185-191:⚠️ Potential issue | 🟡 MinorInspect optional files with
symlink_metadata()before the early return.
source.is_file()follows symlinks, so a broken or non-fileCargo.locksymlink currently returnsOk(false)instead of tripping the symlink rejection. Keep the "missing file => false" behavior, but switch this branch tosymlink_metadata()so every symlink is rejected before the function exits.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/deploy/mod.rs` around lines 185 - 191, In copy_optional_file, replace the early return that uses source.is_file() with a symlink-aware check: call source.symlink_metadata() first, if it returns a NotFound error keep the current behavior and return Ok(false), if symlink_metadata indicates a symlink (metadata.file_type().is_symlink()) return an error (e.g., anyhow::bail or Err) to reject symlinks, otherwise proceed using the metadata (metadata.is_file() or source.is_file()) to decide whether to call copy_file(source, destination) and return Ok(true).
279-305:⚠️ Potential issue | 🟠 MajorResolve workspace members through Cargo, not raw TOML strings.
members = ["crates/*"]is valid Cargo, but this code feeds"crates/*"straight intocanonicalize(), so deploy fails on globbed workspaces. It also turnsmembers = ["."]into an empty relative path and silently drops the root package. Use Cargo's resolved member set (cargo_metadata::MetadataCommandor equivalent) and then handle the workspace-root member explicitly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/deploy/mod.rs` around lines 279 - 305, The current read_workspace_members reads Cargo.toml raw members (so globs like "crates/*" and "." are handled incorrectly); replace the TOML parsing with Cargo metadata resolution via cargo_metadata::MetadataCommand to get the actual resolved workspace member set (use MetadataCommand::new().exec() / metadata.workspace_members and map each PackageId to its package manifest path / package root), then convert those manifest paths into relative paths with resolve_workspace_relative_path(workspace_root, ...) and ensure the workspace-root package (".") is included rather than dropped (handle manifest_path.parent() for each package and deduplicate into the BTreeSet).
🤖 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/deploy/mod.rs`:
- Around line 72-88: The code currently calls prepare_output_dir(...) before
determining the full set of source paths, which allows output_dir to overlap
sources and cause recursion or accidental deletion; to fix, move the
collection/canonicalization step before calling prepare_output_dir: call
collect_required_local_paths(&workspace_root, &dependencies) and canonicalize
workspace_root, config_path, Cargo.toml/Cargo.lock and each local path
(resolving symlinks/relative segments) to absolute paths, then check for any
ancestor/descendant relationship between output_dir (canonicalized via
resolve_output_dir) and any source path and return an error if any overlap is
found; only after this safety check call prepare_output_dir(...) and proceed
with copy_file/copy_optional_file/copy_relative_workspace_path so functions like
copy_path_recursively cannot recurse into the output bundle.
---
Duplicate comments:
In `@crates/cli/src/deploy/mod.rs`:
- Around line 185-191: In copy_optional_file, replace the early return that uses
source.is_file() with a symlink-aware check: call source.symlink_metadata()
first, if it returns a NotFound error keep the current behavior and return
Ok(false), if symlink_metadata indicates a symlink
(metadata.file_type().is_symlink()) return an error (e.g., anyhow::bail or Err)
to reject symlinks, otherwise proceed using the metadata (metadata.is_file() or
source.is_file()) to decide whether to call copy_file(source, destination) and
return Ok(true).
- Around line 279-305: The current read_workspace_members reads Cargo.toml raw
members (so globs like "crates/*" and "." are handled incorrectly); replace the
TOML parsing with Cargo metadata resolution via cargo_metadata::MetadataCommand
to get the actual resolved workspace member set (use
MetadataCommand::new().exec() / metadata.workspace_members and map each
PackageId to its package manifest path / package root), then convert those
manifest paths into relative paths with
resolve_workspace_relative_path(workspace_root, ...) and ensure the
workspace-root package (".") is included rather than dropped (handle
manifest_path.parent() for each package and deduplicate into the BTreeSet).
🪄 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: bd08a2f2-abc1-463c-8575-512c4dbb4ee1
📒 Files selected for processing (3)
crates/cli/src/deploy/mod.rscrates/module-parser/src/metadata.rscrates/module-parser/src/source.rs
✅ Files skipped from review due to trivial changes (2)
- crates/module-parser/src/source.rs
- crates/module-parser/src/metadata.rs
| let output_dir = self.resolve_output_dir(&workspace_root, &project_name); | ||
| prepare_output_dir(&output_dir, &workspace_root, self.force)?; | ||
|
|
||
| copy_file( | ||
| &workspace_root.join("Cargo.toml"), | ||
| &output_dir.join("Cargo.toml"), | ||
| )?; | ||
| let has_cargo_lock = copy_optional_file( | ||
| &workspace_root.join("Cargo.lock"), | ||
| &output_dir.join("Cargo.lock"), | ||
| )?; | ||
| copy_file(&config_path, &output_dir.join("config.yml"))?; | ||
|
|
||
| let local_paths = collect_required_local_paths(&workspace_root, &dependencies)?; | ||
| for relative_path in &local_paths { | ||
| copy_relative_workspace_path(&workspace_root, &output_dir, relative_path)?; | ||
| } |
There was a problem hiding this comment.
Reject output directories that overlap the copied inputs.
prepare_output_dir() runs before you've established that output_dir is disjoint from the workspace/config/local paths you're about to copy. If --output-dir sits inside a copied crate, or a local path resolves to . / the workspace root, copy_path_recursively() will recurse into the bundle it just created. If --force targets an ancestor of one of those inputs, the cleanup step can delete source files before the copy starts. Collect/canonicalize the source set first and fail on any ancestor/descendant overlap with output_dir.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/cli/src/deploy/mod.rs` around lines 72 - 88, The code currently calls
prepare_output_dir(...) before determining the full set of source paths, which
allows output_dir to overlap sources and cause recursion or accidental deletion;
to fix, move the collection/canonicalization step before calling
prepare_output_dir: call collect_required_local_paths(&workspace_root,
&dependencies) and canonicalize workspace_root, config_path,
Cargo.toml/Cargo.lock and each local path (resolving symlinks/relative segments)
to absolute paths, then check for any ancestor/descendant relationship between
output_dir (canonicalized via resolve_output_dir) and any source path and return
an error if any overlap is found; only after this safety check call
prepare_output_dir(...) and proceed with
copy_file/copy_optional_file/copy_relative_workspace_path so functions like
copy_path_recursively cannot recurse into the output bundle.
Signed-off-by: devjow <me@devjow.com>
… address PR review comment
Signed-off-by: devjow <me@devjow.com>
Signed-off-by: devjow <me@devjow.com>
devjow
force-pushed
the
feat/cmd-docker-template
branch
from
4bfbdea to
c676482
Compare
2 participants
Summary by CodeRabbit
New Features
deployCLI command to generate Docker deployment bundles.--template docker,--local-pathor--gittemplate sources,--name,--output-dir, and--force.Documentation