refactor(preflight): simplify swap handling#1122
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRefactored swap provisioning in Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
bin/lib/preflight.js (1)
228-239: Preferfs.mkdirSyncover shellmkdir -p.Using a shell command for directory creation is unnecessary when Node.js provides
fs.mkdirSyncwithrecursive: true. This also avoids a potential command injection risk ifos.homedir()contains shell metacharacters (unlikely but possible).♻️ Suggested refactor
function writeManagedSwapMarker() { const nemoclawDir = path.join(os.homedir(), ".nemoclaw"); - if (!fs.existsSync(nemoclawDir)) { - runCapture(`mkdir -p ${nemoclawDir}`, { ignoreError: true }); - } + fs.mkdirSync(nemoclawDir, { recursive: true }); try { fs.writeFileSync(path.join(nemoclawDir, "managed_swap"), "/swapfile");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/preflight.js` around lines 228 - 239, The writeManagedSwapMarker function currently uses runCapture("mkdir -p ...") to create the .nemoclaw directory; replace that shell call with Node's fs.mkdirSync(nemoclawDir, { recursive: true }) to avoid spawning a shell and potential injection risks and to use native filesystem behavior; keep the existing existence check or simply call mkdirSync in a try/catch and preserve the current best-effort semantics (the try/catch around fs.writeFileSync for the managed_swap marker should remain unchanged).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@bin/lib/preflight.js`:
- Around line 228-239: The writeManagedSwapMarker function currently uses
runCapture("mkdir -p ...") to create the .nemoclaw directory; replace that shell
call with Node's fs.mkdirSync(nemoclawDir, { recursive: true }) to avoid
spawning a shell and potential injection risks and to use native filesystem
behavior; keep the existing existence check or simply call mkdirSync in a
try/catch and preserve the current best-effort semantics (the try/catch around
fs.writeFileSync for the managed_swap marker should remain unchanged).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b732a5d4-367d-4f97-a595-c13710db1c1b
📒 Files selected for processing (1)
bin/lib/preflight.js
ericksoa
left a comment
There was a problem hiding this comment.
Clean refactoring — the extracted helpers make ensureSwap() much easier to follow. Behavior-preserving with good early-return structure for the dry-run path. LGTM.
Summary
Refactor
bin/lib/preflight.jsswap handling to satisfy the current ESLint rules. This keeps the existing behavior but reducesensureSwap()complexity and replaces the empty catch block with an explicit best-effort path.Changes
ensureSwap()control flow so it stays under the configured complexity limitType of Change
Testing
npx prek run --all-filespasses (or equivalentlymake check).npm testpasses.make docsbuilds without warnings. (for doc-only changes)Ran locally:
npm run lintnpm test -- test/preflight.test.jsnpm test(currently fails locally in existingtest/onboard.test.jscases unrelated to this change)Checklist
General
Code Changes
npx prek run --all-filesauto-fixes formatting (ormake formatfor targeted runs).Summary by CodeRabbit