fix: Windows compatibility for delete, size, and path separators#7
fix: Windows compatibility for delete, size, and path separators#7vindiarputra wants to merge 3 commits intocodingstark-dev:mainfrom
Conversation
📝 WalkthroughWalkthroughAdds a GitHub Actions CI workflow for multi-OS testing, documents Bun and platform requirements in README, and implements cross-platform path normalization and safer filesystem operations in scanner with related CLI and benchmark updates for Windows/WSL support. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant GH as GitHub Actions
participant Runner as Runner (ubuntu/macos/windows)
participant Repo as Repository
participant Bun as Bun runtime
participant CLI as Project CLI
GH->>Runner: start job (matrix OS)
Runner->>Repo: checkout
Runner->>Bun: install Bun (bun-version: latest)
Runner->>Repo: run `bun install`
Runner->>Repo: run `bun run check`
Runner->>Repo: run `bun run build`
Runner->>CLI: run `bun run src/cli.ts --help`
Runner->>CLI: run `bun run src/cli.ts --dir $GITHUB_WORKSPACE --dry-run --hide-errors`
alt Windows
Runner->>CLI: run dry-run using PowerShell (pwsh)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 114-118: Update the Linux status row in the README table that
currently reads "Linux | ⚠️ Not tested yet" to reflect the new CI coverage
introduced by this PR; replace that cell with a concise phrase such as "✅ Tested
(CI)" or "✅ Tested (CI runs)" so the Linux entry matches the CI matrix and
avoids contradiction with the added CI runs.
In `@src/scanner.ts`:
- Around line 58-60: normalizeProjectPath currently builds a dynamic RegExp from
the user-controlled target which can throw for malformed patterns and cause
semaphore leaks; replace the RegExp with safe string operations: inside
normalizeProjectPath (and using normalizeSep), normalize the target (trim any
trailing slashes), construct the suffix `"/" + normalizedTarget`, then if the
normalized path endsWith that suffix remove it with slice; this avoids RegExp
construction and eliminates the crash risk that occurs after
metaSemaphore.acquire().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ee18e38a-d5b6-4646-9f85-f5f835a5fa8e
📒 Files selected for processing (4)
.github/workflows/ci.ymlREADME.mdsrc/cli.tssrc/scanner.ts
| | Platform | Status | | ||
| |---|---| | ||
| | macOS | ✅ Tested | | ||
| | Linux | ⚠️ Not tested yet | | ||
| | Windows 10/11 | ✅ Tested (Windows Terminal recommended) | |
There was a problem hiding this comment.
Align Linux platform status with the new CI matrix.
Line 117 currently says Linux is “Not tested yet”, but this PR adds Linux CI runs. Please update wording to avoid contradictory platform claims.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 114 - 118, Update the Linux status row in the README
table that currently reads "Linux | ⚠️ Not tested yet" to reflect the new CI
coverage introduced by this PR; replace that cell with a concise phrase such as
"✅ Tested (CI)" or "✅ Tested (CI runs)" so the Linux entry matches the CI matrix
and avoids contradiction with the added CI runs.
|
@coderabbitai full review |
|
✅ Actions performedReview triggered.
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Pull request overview
This PR improves cross-platform (especially Windows) behavior in BunKill’s scanning, sizing, and deletion logic, and adds CI coverage to validate the tool on multiple OSes.
Changes:
- Normalize path separators and project-path derivation to work on Windows (scanner + CLI).
- Replace shell
rm -rfdeletion withfs.promises.rm()for cross-platform deletes; adjust sizing fallback to include dotfiles. - Add GitHub Actions CI matrix for Ubuntu/macOS/Windows and update README with Windows usage notes.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/scanner.ts |
Adds Windows-aware path normalization, improves nesting/hidden checks, adjusts sizing and deletion implementations for cross-platform support. |
src/cli.ts |
Reuses shared normalizeProjectPath to avoid hardcoded /node_modules path assumptions. |
README.md |
Documents Bun requirement, Windows install/usage, and platform support notes. |
.github/workflows/ci.yml |
Adds OS-matrix CI with typecheck/build and CLI smoke tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| | Platform | Status | | ||
| |---|---| | ||
| | macOS | ✅ Tested | | ||
| | Linux | ⚠️ Not tested yet | | ||
| | Windows 10/11 | ✅ Tested (Windows Terminal recommended) | |
There was a problem hiding this comment.
README now claims Linux is "
|
|
||
| - uses: oven-sh/setup-bun@v2 | ||
| with: | ||
| bun-version: latest |
There was a problem hiding this comment.
CI uses bun-version: latest, which can make builds non-deterministic and introduce sudden breakages when Bun releases. Consider pinning to a specific Bun version (or at least the minimum supported in package.json engines) and optionally adding a separate scheduled job for latest if you want early warning.
| bun-version: latest | |
| bun-version: '1.0.0' |
|
@vindiarputra Thanks for the contribution. Can you please run the test or run the benchmark and paste the result here? I just want to see if this is properly working. I don't have window to test :( |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
benchmark/three-way-benchmark.ts (1)
113-117: Package.json check may cause result mismatch with scanner.Per context snippet 2, the scanner does NOT validate
package.jsonexistence at discovery time—it only filters later inprocessModuleMeta. This meansbunKillScanmay count directories thatbunGlobScannow excludes, leading to differentfoundcounts in benchmark output.If the intent is benchmark fairness (comparing equivalent logic), either remove this check from
bunGlobScan, or document that the glob baseline uses stricter filtering.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmark/three-way-benchmark.ts` around lines 113 - 117, The package.json existence check in bunGlobScan (the parentPath / pkgJson logic) causes bunGlobScan to exclude modules that bunKillScan/processModuleMeta would include later; to make the benchmark fair, remove the pkgJson.exists() early check (the const parentPath / const pkgJson and its await pkgJson.exists() conditional) so discovery behavior matches the scanner, or alternatively move that filtering into the same later step (processModuleMeta) used by bunKillScan; update references in bunGlobScan accordingly so counts align with processModuleMeta.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@benchmark/three-way-benchmark.ts`:
- Around line 48-50: The WSL branch of scriptCmd currently appends "> /dev/null"
which discards stdout so proc.stdout (where results are read) is empty; update
the WSL command in scriptCmd (the array built when useWsl is true, which uses
toWslPath(dir) and runs npkill) to stop redirecting stdout — either remove the
trailing "> /dev/null" entirely or replace it with "2>/dev/null" to only silence
stderr — so that proc.stdout receives the npkill output for parsing.
---
Nitpick comments:
In `@benchmark/three-way-benchmark.ts`:
- Around line 113-117: The package.json existence check in bunGlobScan (the
parentPath / pkgJson logic) causes bunGlobScan to exclude modules that
bunKillScan/processModuleMeta would include later; to make the benchmark fair,
remove the pkgJson.exists() early check (the const parentPath / const pkgJson
and its await pkgJson.exists() conditional) so discovery behavior matches the
scanner, or alternatively move that filtering into the same later step
(processModuleMeta) used by bunKillScan; update references in bunGlobScan
accordingly so counts align with processModuleMeta.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1310069d-3c63-4134-a77d-a32591b5d603
📒 Files selected for processing (1)
benchmark/three-way-benchmark.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
benchmark/three-way-benchmark.ts (1)
102-117: Reuse the scanner’s filtering helpers here instead of re-encoding them.This block has already diverged from
src/scanner.ts: separator normalization is unconditional here, andpackage.jsonexistence is treated as sufficient, whilereadPackageMetadata()/processModuleMeta()exclude unreadable or invalid manifests. That meansbunGlobScan()can count a different project set thanscan(), which weakens the benchmark comparison.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmark/three-way-benchmark.ts` around lines 102 - 117, The benchmark is re-implementing scanner logic and diverging from scan(); update three-way-benchmark.ts to reuse the scanner helpers instead of custom checks: import and call the same normalization/filtering utilities and shouldSkip logic used by scan(), and replace the manual package.json existence check with readPackageMetadata() (and/or processModuleMeta()) to ensure manifests are readable and valid before counting a project; this ensures bunGlobScan() uses identical filtering/normalization as scan() and prevents the benchmark from counting different project sets.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@benchmark/three-way-benchmark.ts`:
- Around line 45-55: npkillScan currently assumes npkill/script exist inside WSL
and treats any no-output run as success; fix by probing the WSL environment for
required binaries before running and by checking the process exit code after the
Promise.race. Specifically, before constructing or invoking the wsl command in
npkillScan, run a quick probe like `wsl bash -lc "command -v script >/dev/null
&& command -v npkill >/dev/null"` (or equivalent spawn) and bail out (return -1)
if the probe fails; after awaiting Promise.race([collectPromise, proc.exited])
inspect proc.exitCode (or proc.success) and return -1 / throw if non-zero before
parsing stdout. Apply the same pre-check + exit-code inspection pattern to the
other similar call sites noted (the sections around lines 79–87, 218–224,
246–254) so missing tools in WSL are detected rather than silently producing
zero results.
- Around line 37-42: The toWslPath function must resolve relative paths to
absolute ones, handle both back- and forward-slash Windows drive prefixes, and
shell-escape single quotes before returning; fix it by first calling
path.resolve(winPath) to normalize and resolve relative inputs, update the
drive-regex in toWslPath to accept either backslash or forward slash after the
drive letter (e.g., /^([A-Za-z]):[\\/]/), convert Windows separators to POSIX
with replaceAll("\\", "/") (or equivalent) so C:/... becomes /mnt/c/..., and
finally escape single quotes in the produced WSL path (replace ' with '\'' or
the equivalent safe shell-escaping) so the path is safe to interpolate into the
bash -lc command. Ensure changes are made inside the toWslPath function and that
callers (where the command is built with bash -lc) receive the escaped WSL path.
---
Nitpick comments:
In `@benchmark/three-way-benchmark.ts`:
- Around line 102-117: The benchmark is re-implementing scanner logic and
diverging from scan(); update three-way-benchmark.ts to reuse the scanner
helpers instead of custom checks: import and call the same
normalization/filtering utilities and shouldSkip logic used by scan(), and
replace the manual package.json existence check with readPackageMetadata()
(and/or processModuleMeta()) to ensure manifests are readable and valid before
counting a project; this ensures bunGlobScan() uses identical
filtering/normalization as scan() and prevents the benchmark from counting
different project sets.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eb95b884-64dd-4307-b62f-99f8cb8647b9
📒 Files selected for processing (1)
benchmark/three-way-benchmark.ts
| /** Convert a Windows absolute path to a WSL /mnt/... path. */ | ||
| function toWslPath(winPath: string): string { | ||
| // e.g. C:\Users\foo → /mnt/c/Users/foo | ||
| return winPath | ||
| .replace(/^([A-Za-z]):\\/, (_, drive) => `/mnt/${drive.toLowerCase()}/`) | ||
| .replaceAll("\\", "/"); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's see the actual code around lines 37-50 in the file
cat -n benchmark/three-way-benchmark.ts | head -60 | tail -35Repository: codingstark-dev/bunkill
Length of output: 1591
🏁 Script executed:
# Search for where toWslPath is called and how dir parameter is passed
rg "toWslPath|npkillScan" benchmark/three-way-benchmark.ts -A 2 -B 2Repository: codingstark-dev/bunkill
Length of output: 992
🏁 Script executed:
# Find where SCAN_DIR is defined
rg "SCAN_DIR|--dir" benchmark/three-way-benchmark.ts -B 2 -A 2Repository: codingstark-dev/bunkill
Length of output: 862
🏁 Script executed:
# Verify the path resolution behavior by testing the proposed fix logic
python3 << 'EOF'
import re
# Current implementation
def toWslPath_current(winPath: str) -> str:
result = winPath
# Regex only matches backslashes
result = re.sub(r'^([A-Za-z]):\\', lambda m: f"/mnt/{m.group(1).lower()}/", result)
result = result.replace("\\", "/")
return result
# Proposed implementation (simulated on Unix)
def toWslPath_proposed(winPath: str) -> str:
# Simulate resolve() by converting mixed slashes to forward slashes
normalized = winPath.replace("\\", "/")
# Now match forward slash pattern
result = re.sub(r'^([A-Za-z]):/', lambda m: f"/mnt/{m.group(1).lower()}/", normalized)
return result
# Test cases
test_cases = [
".",
"./repo",
"C:\\Users\\foo",
"C:/Users/foo",
"C:\\Users\\foo's\\bar",
]
print("Testing path conversions:")
print("-" * 60)
for case in test_cases:
current = toWslPath_current(case)
proposed = toWslPath_proposed(case)
print(f"Input: {case!r}")
print(f" Current: {current!r}")
print(f" Proposed: {proposed!r}")
print()
EOFRepository: codingstark-dev/bunkill
Length of output: 530
🏁 Script executed:
# Also verify the shell escaping issue with a simple test
python3 << 'EOF'
def escapeForSingleQuotedShell(value: str) -> str:
return value.replace("'", "'\"'\"'")
# Test with a problematic path
path_with_quote = "/mnt/c/User's Files"
escaped = escapeForSingleQuotedShell(path_with_quote)
print("Shell escaping test:")
print(f"Original path: {path_with_quote!r}")
print(f"Escaped path: {escaped!r}")
print()
print("Bash command construction:")
print(f"Unescaped: bash -lc 'npkill -d \"{path_with_quote}\" -nu'")
print(f"Escaped: bash -lc 'npkill -d \"{escaped}\" -nu'")
EOFRepository: codingstark-dev/bunkill
Length of output: 316
Resolve relative paths and escape shell metacharacters in WSL path conversion.
The toWslPath() function has three bugs:
- Relative paths (e.g.,
--dir .) are not resolved to absolute paths, so WSL scans the wrong directory - Forward-slash Windows paths (e.g.,
--dir C:/repo) are not converted to/mnt/form due to the backslash-only regex - Paths containing single quotes break the
bash -lcstring interpolation
Use resolve() to normalize the input, update the regex to handle forward slashes, and shell-escape the final WSL path before building the command.
Fix
-import { join } from "node:path";
+import { join, resolve } from "node:path";
+
+function escapeForSingleQuotedShell(value: string): string {
+ return value.replaceAll("'", `'"'"'`);
+}
/** Convert a Windows absolute path to a WSL /mnt/... path. */
function toWslPath(winPath: string): string {
- // e.g. C:\Users\foo → /mnt/c/Users/foo
- return winPath
- .replace(/^([A-Za-z]):\\/, (_, drive) => `/mnt/${drive.toLowerCase()}/`)
- .replaceAll("\\", "/");
+ const normalized = resolve(winPath).replaceAll("\\", "/");
+ return normalized.replace(/^([A-Za-z]):\//, (_, drive) => `/mnt/${drive.toLowerCase()}/`);
}
async function npkillScan(dir: string, useWsl = false): Promise<number> {
const scriptCmd = useWsl
- ? ["wsl", "bash", "-lc", `script -q /dev/null -c 'npkill -d "${toWslPath(dir)}" -nu --hide-errors' 2>/dev/null`]
+ ? ["wsl", "bash", "-lc", `script -q /dev/null -c 'npkill -d "${escapeForSingleQuotedShell(toWslPath(dir))}" -nu --hide-errors' 2>/dev/null`]
: ["script", "-q", "/dev/null", "npkill", "-d", dir, "-nu", "--hide-errors"];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@benchmark/three-way-benchmark.ts` around lines 37 - 42, The toWslPath
function must resolve relative paths to absolute ones, handle both back- and
forward-slash Windows drive prefixes, and shell-escape single quotes before
returning; fix it by first calling path.resolve(winPath) to normalize and
resolve relative inputs, update the drive-regex in toWslPath to accept either
backslash or forward slash after the drive letter (e.g., /^([A-Za-z]):[\\/]/),
convert Windows separators to POSIX with replaceAll("\\", "/") (or equivalent)
so C:/... becomes /mnt/c/..., and finally escape single quotes in the produced
WSL path (replace ' with '\'' or the equivalent safe shell-escaping) so the path
is safe to interpolate into the bash -lc command. Ensure changes are made inside
the toWslPath function and that callers (where the command is built with bash
-lc) receive the escaped WSL path.
| async function npkillScan(dir: string, useWsl = false): Promise<number> { | ||
| // Use login shell so node/npkill are in PATH inside WSL. | ||
| // Only silence stderr (2>/dev/null) so stdout flows to proc.stdout for parsing. | ||
| // Terminal corruption from npkill's \r sequences is handled by printRow's \r\x1b[2K. | ||
| const scriptCmd = useWsl | ||
| ? ["wsl", "bash", "-lc", `script -q /dev/null -c 'npkill -d "${toWslPath(dir)}" -nu --hide-errors' 2>/dev/null`] | ||
| : ["script", "-q", "/dev/null", "npkill", "-d", dir, "-nu", "--hide-errors"]; | ||
|
|
||
| try { | ||
| const proc = Bun.spawn({ | ||
| cmd: ["script", "-q", "/dev/null", "npkill", "-d", dir, "-nu", "--hide-errors"], | ||
| cmd: scriptCmd, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd benchmark && wc -l three-way-benchmark.tsRepository: codingstark-dev/bunkill
Length of output: 93
🏁 Script executed:
cd benchmark && sed -n '40,90p' three-way-benchmark.tsRepository: codingstark-dev/bunkill
Length of output: 1697
🏁 Script executed:
cd benchmark && sed -n '210,260p' three-way-benchmark.tsRepository: codingstark-dev/bunkill
Length of output: 2058
🏁 Script executed:
cd benchmark && sed -n '75,95p' three-way-benchmark.tsRepository: codingstark-dev/bunkill
Length of output: 682
🏁 Script executed:
cd benchmark && grep -n "exitCode\|exitStatus\|exit\|proc\." three-way-benchmark.ts | head -20Repository: codingstark-dev/bunkill
Length of output: 381
🏁 Script executed:
web_search
Bun.spawn exit code status checkingRepository: codingstark-dev/bunkill
Length of output: 165
🌐 Web query:
Bun.spawn exit code checking proc.exited
💡 Result:
To check the exit code of a process spawned with Bun.spawn, use the proc.exited Promise and the proc.exitCode property on the returned Subprocess object. Key methods: 1. Await proc.exited to wait for the process to exit, which resolves to the exit code: const proc = Bun.spawn(["bun", "--version"]); const exitCode = await proc.exited; // e.g. 0 for success console.log(exitCode); 2. After awaiting exited (or if already exited), read proc.exitCode synchronously (null if not exited): await proc.exited; console.log(proc.exitCode); // number | null 3. Use the onExit option in spawn options for a callback: const proc = Bun.spawn(["echo", "hello"], { onExit(subprocess, exitCode, signalCode, error) { console.log(Exited with code ${exitCode}); }, }); proc.exited is a Promise that resolves to the exit code when the process exits. Bun waits for all child processes to exit by default unless unref is called. This matches Node.js child_process behavior but uses ReadableStreams for stdio.
Citations:
- 1: https://bun.com/reference/bun/Spawn/SpawnSyncOptions/onExit
- 2: https://bun.com/docs/api/spawn
- 3: https://bun.com/docs/guides/process/spawn
- 4: https://bun.com/docs/runtime/child-process
- 5: https://bun.com/reference/bun/SpawnOptions/OptionsObject/onExit
🏁 Script executed:
cd benchmark && rg -A 5 -B 5 "proc\.exited" three-way-benchmark.tsRepository: codingstark-dev/bunkill
Length of output: 420
🏁 Script executed:
cd benchmark && grep -n "available" three-way-benchmark.ts | tail -10Repository: codingstark-dev/bunkill
Length of output: 706
Check WSL tool availability before running benchmarks and inspect process exit codes.
where npkill and where wsl only confirm the Windows shim and WSL binary exist—they do not prove npkill or script are available inside the WSL environment. The code runs wsl bash -lc "script... npkill..." at line 49, but if either tool is missing in WSL, the process fails silently.
Additionally, Promise.race([collectPromise, proc.exited]) at line 75 awaits the exit but never inspects proc.exitCode. A failed command with no output returns found: 0 indistinguishably from a successful scan with zero packages, allowing the benchmark to appear available and successful when the underlying tools are unavailable.
Probe inside WSL before enabling npkill (e.g., wsl bash -lc "command -v script && command -v npkill"), or check proc.exitCode after line 75 and return -1 if non-zero before parsing.
Also applies to: 218–224, 246–254, 79–87
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@benchmark/three-way-benchmark.ts` around lines 45 - 55, npkillScan currently
assumes npkill/script exist inside WSL and treats any no-output run as success;
fix by probing the WSL environment for required binaries before running and by
checking the process exit code after the Promise.race. Specifically, before
constructing or invoking the wsl command in npkillScan, run a quick probe like
`wsl bash -lc "command -v script >/dev/null && command -v npkill >/dev/null"`
(or equivalent spawn) and bail out (return -1) if the probe fails; after
awaiting Promise.race([collectPromise, proc.exited]) inspect proc.exitCode (or
proc.success) and return -1 / throw if non-zero before parsing stdout. Apply the
same pre-check + exit-code inspection pattern to the other similar call sites
noted (the sections around lines 79–87, 218–224, 246–254) so missing tools in
WSL are detected rather than silently producing zero results.
I made some improvements to benchmark/three-way-benchmark.ts before running:
Here are the benchmark results on Windows 11 with WSL: Runs : 1 (+ 1 warm-up) npkill : C:\nvm4w\nodejs\npkill (via WSL) Running benchmarks... Results: npkill avg= 10.71s min= 10.71s max= 10.71s found= 0 baseline Installed vs local benchmark (bun run bench:installed-vs-local --dir --runs 1): Runs : 1 (+ 2 warm-ups) Warming up... installed bunkill 6 found, reported 11.97s Running timed benchmark... run 1 installed bunkill wall= 8.77s reported= 8.52s found= 6 Summary: installed bunkill avg wall= 8.77s avg reported= 8.52s found= 6 Fastest: local bundled dist |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Require a package.json in the parent directory (same as bunkill) | ||
| const parentPath = fullPath.slice(0, fullPath.length - "/node_modules".length); | ||
| const pkgJson = Bun.file(join(parentPath, "package.json")); | ||
| if (!(await pkgJson.exists())) continue; |
There was a problem hiding this comment.
parentPath is derived by slicing with the hard-coded string length of "/node_modules". On Windows fullPath uses backslashes (e.g. ...\node_modules), so this slice removes the wrong number of characters and can produce an invalid parent directory (breaking the package.json existence check). Prefer using path.dirname(fullPath) (or compute based on the actual separator/normalized string) instead of string-length slicing.
|
|
||
| this.pendingUiMeta.add(module.path); | ||
| const projectPath = module.path.replace(/\/node_modules$/, ""); | ||
| const projectPath = normalizeProjectPath(module.path); |
There was a problem hiding this comment.
normalizeProjectPath(module.path) uses the default target (node_modules). If the user runs the CLI with --target <name>, module.path will end with that custom target, and this call won’t strip it—so git -C will run inside the target directory instead of the project root (often causing git status to fail / mark as non-repo). Consider persisting the scan target on the BunKill instance and passing it here (or storing the derived project root on NodeModule).
| const projectPath = normalizeProjectPath(module.path); | |
| const projectPath = | |
| (module as any).projectRoot ?? | |
| normalizeProjectPath(module.path); |
| // Only silence stderr (2>/dev/null) so stdout flows to proc.stdout for parsing. | ||
| // Terminal corruption from npkill's \r sequences is handled by printRow's \r\x1b[2K. | ||
| const scriptCmd = useWsl | ||
| ? ["wsl", "bash", "-lc", `script -q /dev/null -c 'npkill -d "${toWslPath(dir)}" -nu --hide-errors' 2>/dev/null`] |
There was a problem hiding this comment.
When useWsl is true, the bash -lc command string interpolates dir into a shell script (... "${toWslPath(dir)}" ...). If --dir contains characters like ", $, backticks, or ', this can break quoting and potentially lead to shell injection or unexpected behavior. Prefer passing the path as a positional parameter to bash -lc (e.g. bash -lc '... "$1" ...' -- <path>) or otherwise escaping the value safely.
| ? ["wsl", "bash", "-lc", `script -q /dev/null -c 'npkill -d "${toWslPath(dir)}" -nu --hide-errors' 2>/dev/null`] | |
| ? [ | |
| "wsl", | |
| "bash", | |
| "-lc", | |
| 'script -q /dev/null -c "npkill -d \\"$1\\" -nu --hide-errors" 2>/dev/null', | |
| "--", | |
| toWslPath(dir), | |
| ] |
Summary by CodeRabbit
Chores
Documentation
Bug Fixes
Refactor