Refactor cloneTemplate to use execFile and improve error handling#17645
Refactor cloneTemplate to use execFile and improve error handling#17645DawitMengistu wants to merge 2 commits into
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
|
Looking for one thing? Review this PR in Change Stack to search files, summaries, diffs, and code without losing your place. Walkthrough
ChangesTemplate cloning refactor to execFile
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
PR triageLinked issue check passed (#17646). Mastra uses CodeRabbit for automated code reviews. Please address all feedback from CodeRabbit by either making changes to your PR or leaving a comment explaining why you disagree with the feedback. Since CodeRabbit is an AI, it may occasionally provide incorrect feedback. PR complexity score
Applied label: Changed test gateChanged Test Gate is pending. The |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/src/utils/clone-template.ts`:
- Around line 35-37: The clone flow leaves targetPath behind on failure causing
the directoryExists guard in the projectPath/projectName check to fail on
retries; update the cloning logic in clone-template.ts to either (a) perform the
clone into a temporary directory (e.g., tmpTarget) and rename/move it to
targetPath only after the clone succeeds, or (b) ensure any created targetPath
is removed in all failure paths (catch blocks and both degit and git fallback
code paths) before rethrowing; reference the projectPath/projectName checks,
targetPath creation, and the degit/git fallback branches so cleanup is executed
consistently on errors and spinner.error is still called.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8c41d630-f107-46af-adc0-1f1e47d4491f
📒 Files selected for processing (1)
packages/cli/src/utils/clone-template.ts
| if (await directoryExists(projectPath)) { | ||
| spinner.error(`Directory ${projectName} already exists`); | ||
| throw new Error(`Directory ${projectName} already exists`); |
There was a problem hiding this comment.
Clean up targetPath when cloning fails.
Because Line 92 creates targetPath before either clone command runs, any failure path leaves that directory behind. The next retry then immediately trips the existing-directory guard at Lines 35-37, and the git fallback is also fragile if degit wrote anything before throwing. Remove targetPath on failure, or clone into a temp directory and rename only after success.
Also applies to: 92-119
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/cli/src/utils/clone-template.ts` around lines 35 - 37, The clone
flow leaves targetPath behind on failure causing the directoryExists guard in
the projectPath/projectName check to fail on retries; update the cloning logic
in clone-template.ts to either (a) perform the clone into a temporary directory
(e.g., tmpTarget) and rename/move it to targetPath only after the clone
succeeds, or (b) ensure any created targetPath is removed in all failure paths
(catch blocks and both degit and git fallback code paths) before rethrowing;
reference the projectPath/projectName checks, targetPath creation, and the
degit/git fallback branches so cleanup is executed consistently on errors and
spinner.error is still called.
There was a problem hiding this comment.
@DawitMengistu can you address this comment? Thanks!
dane-ai-mastra
Bot
added
complexity: low
|
Tests seem to fail as well |
3 participants
Fixes #17646
This PR fixes a Windows-specific failure when cloning template repositories in create-mastra.
The issue occurs because shell-quote is used together with child_process.exec, which causes Git URLs to be incorrectly escaped (e.g. https:// becomes https://). This results in git clone failing with:
fatal: Too many arguments
Root cause
Using shell-quote.quote() to build a command string for exec() introduces unsafe shell escaping behavior for Git URLs, which is not cross-platform reliable.
Fix
Replaced exec + shellQuote with child_process.execFile, passing arguments as an array instead of a shell string. This removes shell parsing entirely and ensures consistent behavior across Windows, macOS, and Linux.
Changes
Removed shell-quote dependency for command execution
Replaced exec with execFile
Updated:
git clone
npx degit
package manager install
Ensured all commands use argument arrays instead of shell strings
Verification
Reproduced issue on Windows by logging the generated command, confirming:
git clone https://github.com/...
After fix, commands execute correctly with no escaping issues.
Related issue(s)
Fixes # (you can open one first, then paste the number here)
ELI5
This PR fixes a bug where cloning templates on Windows would fail because the code was trying to use shell escaping on command URLs, which broke them (turning
https://intohttps\://). Instead of escaping commands for a shell, the fix passes commands directly as a list of arguments, which works correctly on all operating systems.Summary
This PR refactors the template cloning functionality to replace unsafe shell command construction with direct file execution. The core issue was that
child_process.exec()combined withshell-quotecaused platform-specific escaping problems on Windows when handling Git URLs. By switching tochild_process.execFile(), commands are passed as argument arrays, eliminating shell parsing entirely and providing cross-platform reliability.Key Changes
Command Execution Refactor:
execFileimport usingutil.promisify(child_process.execFile)instead of building shell command stringscloneRepositoryWithoutGit()to use array-based arguments:npx degitinvocation:execFile('npx', ['degit', repoWithBranch, targetPath])execFile('git', ['clone', '--branch', branch, repoUrl, targetPath])(branch conditional)installDependencies()to use:execFile(pm, ['install'], { cwd: projectPath })shell-quotefor command constructionDirectory and Environment Handling:
fs.mkdir()before attempting clones.envfile logic withMODEL=replacement.gitdirectory after successful git cloneError Handling and UX:
Testing
Comprehensive test suite included covering:
.envfile updates with LLM provider configurationNo changes to exported/public API signatures.