fix: improve git onboarding and file handling#44
Conversation
Zhang-Henry
left a comment
There was a problem hiding this comment.
Review Comments
PR scope is too large
This PR is 12,859 additions mixing several unrelated changes:
- Git onboarding UX improvements (good, small)
- Safe argv-based spawning for file paths with special characters (good, small)
- Entire
drclaw+vibelabPython CLI packages (~5,000+ lines) - Extensive OpenClaw integration docs in README
Please split this into separate PRs. The git fixes are reviewable; the CLI infrastructure needs its own dedicated review.
Shell injection still present in git.js
The PR migrates some exec() calls to spawn() with argv arrays (good), but several file-path-sensitive calls still use template strings with execAsync():
await execAsync(`git status --porcelain "${file}"`, { cwd: projectPath });
await execAsync(`git show HEAD:"${file}"`, { cwd: projectPath });Quoting doesn't protect against backticks or $(...) in filenames. These should also use argv-based spawning for consistency with the rest of your changes.
Code duplication: drclaw vs vibelab
The two Python packages are byte-for-byte identical except for package names. This creates a maintenance burden — any bug fix must be applied twice. Consider making vibelab a thin wrapper that imports from drclaw, or use a single package with an entry-point alias.
1dc51e7 to
89722ed
Compare
|
Hi @Zhang-Henry Thank you for the detailed review and for catching those issues! PR Scope & Cleanup You were absolutely right about the scope. I realized I had accidentally branched off an unmerged openclaw branch, which caused several unrelated packages and docs to be mixed in. I have now re-created this branch from scratch to keep it clean. This PR now strictly focuses on the Git initialization and onboarding logic. The CLI infrastructure and other changes have been removed and will be submitted in separate, dedicated PRs later.
The PR should now be much smaller and easier to review. Please let me know if there are any other concerns! |
|
Added a follow-up fix in 5f1e52a to address the review findings:
Validation: npm run typecheck |
Summary
Testing