Add uninstall scripts, tests, and PowerShell utilities#541
Open
fredrik-hellmangroup wants to merge 7 commits intoaffaan-m:mainfrom
Open
Add uninstall scripts, tests, and PowerShell utilities#541fredrik-hellmangroup wants to merge 7 commits intoaffaan-m:mainfrom
fredrik-hellmangroup wants to merge 7 commits intoaffaan-m:mainfrom
Conversation
Add shell and PowerShell uninstall wrappers (uninstall.sh, uninstall.ps1) that resolve symlinked bin paths and delegate to the Node-based uninstall runtime (scripts/uninstall.js). Add cross-platform tests (tests/scripts/uninstall-sh.test.js, tests/scripts/uninstall-ps1.test.js) and extend tests/scripts/uninstall.test.js to cover permission error behavior during removal. Update README with usage examples for the uninstall entrypoints and include the new wrapper scripts in package.json files list. These changes provide native entrypoints for uninstalling ECC-managed files and validate dry-run, delegation, and error handling behavior.
Extract symlink resolution into a Resolve-LinkedScriptPath function and use it in install.ps1 and uninstall.ps1. The helper resolves chained symlinks (including array targets and relative targets), returns absolute paths, detects cycles, enforces a 32-depth limit, and throws clearer errors on failure. This makes determining $scriptPath more robust when the scripts are symlinked.
Refactor toBashPath to accept platform and env parameters so behavior can be tested deterministically. Add detection for WSL, Cygwin and MSYS-style environments and apply appropriate drive prefixes (/mnt/, /cygdrive/, or /) when converting Windows paths to bash-style paths. Add unit tests covering non-Windows, WSL, MSYS and Cygwin conversion cases and a small console log to show path conversion helpers during test runs.
Add preflight checks to install.ps1 to ensure Node.js is in PATH and the installer script exists, failing with clear errors. Use path.resolve for resolving install config paths to always return absolute paths, and update tests to match this behavior. Extract PowerShell detection logic into tests/scripts/powershell-test-utils.js with unit tests, and refactor install/uninstall PowerShell tests to use the new helper, add tests for missing Node and missing installer script, and report skipped tests when PowerShell is not available.
Contributor
📝 WalkthroughWalkthroughAdds cross-platform uninstall entrypoints ( Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Wrapper as "uninstall.sh / uninstall.ps1"
participant Resolver as "Link Resolver"
participant Node as "node scripts/uninstall.js"
participant FS as "File system (.cursor/ecc-install-state.json)"
User->>Wrapper: invoke uninstall with args
Wrapper->>Resolver: resolve real script path (follow symlinks)
Resolver-->>Wrapper: resolved script dir
Wrapper->>Node: exec node with forwarded args
Node->>FS: read/install-state, plan removals
FS-->>Node: state or errors
Node-->>Wrapper: exit code + JSON summary
Wrapper-->>User: propagate exit code and output
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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)
📝 Coding Plan
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 |
Contributor
Greptile SummaryThis PR adds cross-platform uninstall entrypoints ( Key changes:
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A([User invokes uninstall.sh / uninstall.ps1]) --> B[Resolve symlinks\nwith depth limit 32\nand cycle detection]
B --> C{Symlink valid?}
C -- No / depth exceeded --> ERR1([exit 1: error message])
C -- Yes --> D{node in PATH?}
D -- No --> ERR2([exit 1: Node.js not found])
D -- Yes --> E{scripts/uninstall.js\nexists?}
E -- No --> ERR3([exit 1: script not found])
E -- Yes --> F[exec node scripts/uninstall.js args]
F --> G{--dry-run flag?}
G -- Yes --> H[Print planned removals\nECC state preserved]
G -- No --> I[Remove ECC-managed files\nfrom install-state]
I --> J{Permission errors?}
J -- Yes --> K([exit 1: error summary\nFiles NOT removed])
J -- No --> L([exit 0: removal complete])
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Adds cross-platform uninstall entrypoints and expands test coverage around uninstall wrappers, PowerShell resolution, and path/config handling to improve reliability of ECC install/uninstall flows.
Changes:
- Added
uninstall.shanduninstall.ps1wrapper entrypoints delegating to the Node uninstaller runtime. - Improved PowerShell wrapper robustness for
install.ps1(symlink resolution + friendlier preflight errors) and added shared PowerShell resolution utilities for tests. - Updated config path resolution to always return absolute paths and expanded tests (including Windows-to-bash path conversion cases).
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| uninstall.sh | New bash wrapper that resolves symlinks and delegates to scripts/uninstall.js. |
| uninstall.ps1 | New PowerShell wrapper that resolves symlinks and delegates to scripts/uninstall.js. |
| install.ps1 | Refactor to robust symlink resolution + Node/script preflight checks with friendly errors. |
| scripts/lib/install/config.js | Switch to path.resolve to ensure absolute config paths. |
| package.json | Publish uninstall wrapper scripts in the npm package file list. |
| README.md | Document uninstall usage across macOS/Linux, Windows, and npx ecc uninstall. |
| tests/scripts/uninstall.test.js | Add regression test ensuring permission errors are reported and managed files aren’t silently removed. |
| tests/scripts/uninstall-sh.test.js | New test ensuring uninstall.sh delegates correctly and preserves dry-run output. |
| tests/scripts/uninstall-ps1.test.js | New test ensuring uninstall.ps1 is published and delegates correctly (with dry-run). |
| tests/scripts/powershell-test-utils.js | New helper for locating an available PowerShell executable for tests. |
| tests/scripts/powershell-test-utils.test.js | Unit tests for the new PowerShell resolution helper. |
| tests/scripts/install-ps1.test.js | Refactor to use shared PowerShell resolver + add preflight error scenario tests. |
| tests/lib/install-config.test.js | Update tests to validate absolute path resolution behavior. |
| tests/hooks/hooks.test.js | Enhance and test Windows-to-bash path conversion across WSL/Cygwin/MSYS cases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| $scriptPath = Resolve-LinkedScriptPath -InitialPath $PSCommandPath | ||
| $scriptDir = Split-Path -Parent $scriptPath | ||
| $uninstallerScript = Join-Path -Path (Join-Path -Path $scriptDir -ChildPath 'scripts') -ChildPath 'uninstall.js' | ||
|
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
5 issues found across 14 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/scripts/uninstall.test.js">
<violation number="1" location="tests/scripts/uninstall.test.js:285">
P2: The new permission-error test is counted as passed when skipped, producing false-green coverage on Windows/root environments.</violation>
</file>
<file name="tests/scripts/powershell-test-utils.js">
<violation number="1" location="tests/scripts/powershell-test-utils.js:19">
P2: PowerShell availability probe uses a hardcoded 5s startup timeout, which can falsely mark PowerShell as unavailable and skip CI tests.</violation>
</file>
<file name="uninstall.ps1">
<violation number="1" location="uninstall.ps1:29">
P2: Hard links are misclassified as symlinks, which can make wrapper path resolution throw and prevent uninstall execution.</violation>
<violation number="2" location="uninstall.ps1:56">
P2: Add the same Node.js and uninstaller-script preflight checks here so users get a clear error when prerequisites are missing instead of a cryptic OS/Node failure.</violation>
</file>
<file name="README.md">
<violation number="1" location="README.md:186">
P2: Generic uninstall docs hardcode Cursor target, conflicting with default-install examples that target Claude by default.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if (test('reports permission errors without silently removing managed files', () => { | ||
| if (process.platform === 'win32' || process.getuid?.() === 0) { | ||
| console.log(' (skipped — chmod ineffective on Windows/root)'); | ||
| return; |
Contributor
There was a problem hiding this comment.
P2: The new permission-error test is counted as passed when skipped, producing false-green coverage on Windows/root environments.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/scripts/uninstall.test.js, line 285:
<comment>The new permission-error test is counted as passed when skipped, producing false-green coverage on Windows/root environments.</comment>
<file context>
@@ -279,6 +279,79 @@ function runTests() {
+ if (test('reports permission errors without silently removing managed files', () => {
+ if (process.platform === 'win32' || process.getuid?.() === 0) {
+ console.log(' (skipped — chmod ineffective on Windows/root)');
+ return;
+ }
+
</file context>
| { | ||
| encoding: 'utf8', | ||
| stdio: ['pipe', 'pipe', 'pipe'], | ||
| timeout: 5000, |
Contributor
There was a problem hiding this comment.
P2: PowerShell availability probe uses a hardcoded 5s startup timeout, which can falsely mark PowerShell as unavailable and skip CI tests.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/scripts/powershell-test-utils.js, line 19:
<comment>PowerShell availability probe uses a hardcoded 5s startup timeout, which can falsely mark PowerShell as unavailable and skip CI tests.</comment>
<file context>
@@ -0,0 +1,34 @@
+ {
+ encoding: 'utf8',
+ stdio: ['pipe', 'pipe', 'pipe'],
+ timeout: 5000,
+ }
+ );
</file context>
| $visitedPaths[$currentPath] = $true | ||
| $item = Get-Item -LiteralPath $currentPath -Force | ||
|
|
||
| if (-not $item.LinkType) { |
Contributor
There was a problem hiding this comment.
P2: Hard links are misclassified as symlinks, which can make wrapper path resolution throw and prevent uninstall execution.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At uninstall.ps1, line 29:
<comment>Hard links are misclassified as symlinks, which can make wrapper path resolution throw and prevent uninstall execution.</comment>
<file context>
@@ -0,0 +1,57 @@
+ $visitedPaths[$currentPath] = $true
+ $item = Get-Item -LiteralPath $currentPath -Force
+
+ if (-not $item.LinkType) {
+ return $currentPath
+ }
</file context>
|
|
||
| ```bash | ||
| # macOS/Linux | ||
| ./uninstall.sh --target cursor --dry-run |
Contributor
There was a problem hiding this comment.
P2: Generic uninstall docs hardcode Cursor target, conflicting with default-install examples that target Claude by default.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At README.md, line 186:
<comment>Generic uninstall docs hardcode Cursor target, conflicting with default-install examples that target Claude by default.</comment>
<file context>
@@ -176,6 +176,34 @@ npm install # or: pnpm install | yarn install | bun install
+
+```bash
+# macOS/Linux
+./uninstall.sh --target cursor --dry-run
+./uninstall.sh --target cursor
+
</file context>
Introduce resolveExecutablePath helper in tests/scripts/powershell-test-utils.js and export it for reuse. Replace duplicated locator logic in install-ps1.test.js with the new helper and add unit tests for path resolution and error reporting. Enhance uninstall-ps1.test.js to use the resolver, accept injected env and scriptPath, and add tests that verify friendly preflight errors when Node is missing or the runtime uninstaller script is absent. Update uninstall.ps1 to perform the same preflight checks (Node in PATH and existence of scripts/uninstall.js) and emit clear error messages with non-zero exit codes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
install.ps1 (1)
1-67:⚠️ Potential issue | 🟡 MinorAdd UTF-8 BOM to
install.ps1or replace the em dash with ASCII characters.The file contains a non-ASCII em dash character (—) on line 2 in the comment but lacks UTF-8 BOM, which can cause encoding issues in older PowerShell environments. Either save the file as UTF-8 with BOM or replace the em dash with a regular hyphen or double hyphen (–).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@install.ps1` around lines 1 - 67, The comment header in install.ps1 contains a non-ASCII em dash (—) which can break older PowerShell parsers; either re-save install.ps1 as UTF-8 with BOM or replace the em dash with an ASCII hyphen (e.g., "-") (the offending text is the second comment line in the file near the shebang and the file header); after changing encoding or character, re-run to ensure Resolve-LinkedScriptPath and the rest of the script execute without encoding-related errors.
🧹 Nitpick comments (2)
tests/hooks/hooks.test.js (1)
266-271: Add a focused test for theWSL_DISTRO_NAMEdetection branch.You already cover
WSLENV; adding the second WSL signal improves branch-level regression protection.Suggested test addition
+ if ( + test('toBashPath uses WSL /mnt drive mounts when WSL_DISTRO_NAME is set', () => { + assert.strictEqual( + toBashPath('E:\\workspace\\hooks\\observe.sh', 'win32', { WSL_DISTRO_NAME: 'Ubuntu' }), + '/mnt/e/workspace/hooks/observe.sh' + ); + }) + ) + passed++; + else failed++;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hooks/hooks.test.js` around lines 266 - 271, Add a new unit test for toBashPath that mirrors the existing WSLENV test but sets WSL_DISTRO_NAME (instead of WSLENV) to simulate WSL detection; create a test named like "toBashPath uses WSL /mnt drive mounts when WSL_DISTRO_NAME is set" and call toBashPath('E:\\workspace\\hooks\\observe.sh', 'win32', { WSL_DISTRO_NAME: 'Ubuntu' }) asserting the expected '/mnt/e/workspace/hooks/observe.sh' result so the WSL detection branch that checks WSL_DISTRO_NAME is covered.uninstall.sh (1)
9-14: Consider adding symlink cycle detection for consistency with the PowerShell implementation.The PowerShell version (
uninstall.ps1) includes cycle detection and a depth limit (MaxLinkDepth = 32), but this shell script could loop indefinitely if a symlink cycle exists. While rare in practice, adding a depth counter would provide parity:🔧 Optional: Add depth limit for symlink resolution
SCRIPT_PATH="$0" +max_depth=32 +depth=0 while [ -L "$SCRIPT_PATH" ]; do + depth=$((depth + 1)) + if [ "$depth" -gt "$max_depth" ]; then + echo "Error: Exceeded symlink resolution depth limit" >&2 + exit 1 + fi link_dir="$(cd "$(dirname "$SCRIPT_PATH")" && pwd)" SCRIPT_PATH="$(readlink "$SCRIPT_PATH")" [[ "$SCRIPT_PATH" != /* ]] && SCRIPT_PATH="$link_dir/$SCRIPT_PATH" done🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@uninstall.sh` around lines 9 - 14, The symlink resolution loop using SCRIPT_PATH, link_dir and readlink can loop forever on a cyclic symlink; add a depth counter (e.g., link_depth) and a MAX_LINK_DEPTH (e.g., 32) and increment link_depth each iteration of the while [ -L "$SCRIPT_PATH" ] loop, breaking and printing a clear error and nonzero exit code if link_depth exceeds MAX_LINK_DEPTH to mirror uninstall.ps1's cycle detection; ensure you still resolve relative links using the existing link_dir logic when not exiting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@install.ps1`:
- Around line 1-67: The comment header in install.ps1 contains a non-ASCII em
dash (—) which can break older PowerShell parsers; either re-save install.ps1 as
UTF-8 with BOM or replace the em dash with an ASCII hyphen (e.g., "-") (the
offending text is the second comment line in the file near the shebang and the
file header); after changing encoding or character, re-run to ensure
Resolve-LinkedScriptPath and the rest of the script execute without
encoding-related errors.
---
Nitpick comments:
In `@tests/hooks/hooks.test.js`:
- Around line 266-271: Add a new unit test for toBashPath that mirrors the
existing WSLENV test but sets WSL_DISTRO_NAME (instead of WSLENV) to simulate
WSL detection; create a test named like "toBashPath uses WSL /mnt drive mounts
when WSL_DISTRO_NAME is set" and call
toBashPath('E:\\workspace\\hooks\\observe.sh', 'win32', { WSL_DISTRO_NAME:
'Ubuntu' }) asserting the expected '/mnt/e/workspace/hooks/observe.sh' result so
the WSL detection branch that checks WSL_DISTRO_NAME is covered.
In `@uninstall.sh`:
- Around line 9-14: The symlink resolution loop using SCRIPT_PATH, link_dir and
readlink can loop forever on a cyclic symlink; add a depth counter (e.g.,
link_depth) and a MAX_LINK_DEPTH (e.g., 32) and increment link_depth each
iteration of the while [ -L "$SCRIPT_PATH" ] loop, breaking and printing a clear
error and nonzero exit code if link_depth exceeds MAX_LINK_DEPTH to mirror
uninstall.ps1's cycle detection; ensure you still resolve relative links using
the existing link_dir logic when not exiting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b136d9e3-306e-49fa-9f22-9bac860ee303
📒 Files selected for processing (14)
README.mdinstall.ps1package.jsonscripts/lib/install/config.jstests/hooks/hooks.test.jstests/lib/install-config.test.jstests/scripts/install-ps1.test.jstests/scripts/powershell-test-utils.jstests/scripts/powershell-test-utils.test.jstests/scripts/uninstall-ps1.test.jstests/scripts/uninstall-sh.test.jstests/scripts/uninstall.test.jsuninstall.ps1uninstall.sh
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/scripts/install-ps1.test.js (1)
16-51: Consider extracting shared test utilities to reduce duplication.The
createTempDir,cleanup,run, andtestfunctions are duplicated between this file andtests/scripts/uninstall-ps1.test.js. While acceptable for now, extracting these to a shared test helper module would improve maintainability.♻️ Example extraction to shared module
Create a new file
tests/scripts/ps1-test-helpers.js:const fs = require('fs'); const os = require('os'); const path = require('path'); const { execFileSync } = require('child_process'); function createTempDir(prefix) { return fs.mkdtempSync(path.join(os.tmpdir(), prefix)); } function cleanup(dirPath) { fs.rmSync(dirPath, { recursive: true, force: true }); } function runPowerShell(powerShellCommand, defaultScript, args = [], options = {}) { // ... existing run() logic } function test(name, fn) { // ... existing test() logic } module.exports = { createTempDir, cleanup, runPowerShell, test };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/scripts/install-ps1.test.js` around lines 16 - 51, Extract the duplicated test helpers (createTempDir, cleanup, run, and test) into a shared module and replace the in-file copies with imports; specifically, create a helpers module that exports createTempDir, cleanup, and a runPowerShell wrapper (or keep the name run) which accepts powerShellCommand, scriptPath, args and options and reproduces the execFileSync behavior from this diff, then update the tests to import those helpers and call them instead of the local functions (look for createTempDir, cleanup, run, test in both install-ps1.test.js and uninstall-ps1.test.js to locate where to remove duplication).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/scripts/install-ps1.test.js`:
- Around line 16-51: Extract the duplicated test helpers (createTempDir,
cleanup, run, and test) into a shared module and replace the in-file copies with
imports; specifically, create a helpers module that exports createTempDir,
cleanup, and a runPowerShell wrapper (or keep the name run) which accepts
powerShellCommand, scriptPath, args and options and reproduces the execFileSync
behavior from this diff, then update the tests to import those helpers and call
them instead of the local functions (look for createTempDir, cleanup, run, test
in both install-ps1.test.js and uninstall-ps1.test.js to locate where to remove
duplication).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7ebc6b68-7c53-464b-a788-4a6d36b261d0
📒 Files selected for processing (5)
tests/scripts/install-ps1.test.jstests/scripts/powershell-test-utils.jstests/scripts/powershell-test-utils.test.jstests/scripts/uninstall-ps1.test.jsuninstall.ps1
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/scripts/powershell-test-utils.test.js
Contributor
There was a problem hiding this comment.
3 issues found across 14 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="uninstall.sh">
<violation number="1" location="uninstall.sh:10">
P2: Symlink resolution loop has no cycle/depth guard, so cyclic links can make uninstall.sh hang indefinitely.</violation>
</file>
<file name="README.md">
<violation number="1" location="README.md:186">
P2: Quick Start uninstall commands are pinned to `--target cursor`, but the install flow above uses the default target (`claude`), causing mismatched install/uninstall targeting in docs.</violation>
</file>
<file name="tests/scripts/uninstall.test.js">
<violation number="1" location="tests/scripts/uninstall.test.js:285">
P2: The new permission test is silently counted as passed when skipped on root/Windows, which can mask missing coverage for uninstall permission-error handling in CI.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Comment on lines
+186
to
+187
| ./uninstall.sh --target cursor --dry-run | ||
| ./uninstall.sh --target cursor |
Contributor
There was a problem hiding this comment.
P2: Quick Start uninstall commands are pinned to --target cursor, but the install flow above uses the default target (claude), causing mismatched install/uninstall targeting in docs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At README.md, line 186:
<comment>Quick Start uninstall commands are pinned to `--target cursor`, but the install flow above uses the default target (`claude`), causing mismatched install/uninstall targeting in docs.</comment>
<file context>
@@ -176,6 +176,34 @@ npm install # or: pnpm install | yarn install | bun install
+
+```bash
+# macOS/Linux
+./uninstall.sh --target cursor --dry-run
+./uninstall.sh --target cursor
+
</file context>
Suggested change
| ./uninstall.sh --target cursor --dry-run | |
| ./uninstall.sh --target cursor | |
| ./uninstall.sh --dry-run | |
| ./uninstall.sh |
| if (test('reports permission errors without silently removing managed files', () => { | ||
| if (process.platform === 'win32' || process.getuid?.() === 0) { | ||
| console.log(' (skipped — chmod ineffective on Windows/root)'); | ||
| return; |
Contributor
There was a problem hiding this comment.
P2: The new permission test is silently counted as passed when skipped on root/Windows, which can mask missing coverage for uninstall permission-error handling in CI.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/scripts/uninstall.test.js, line 285:
<comment>The new permission test is silently counted as passed when skipped on root/Windows, which can mask missing coverage for uninstall permission-error handling in CI.</comment>
<file context>
@@ -279,6 +279,79 @@ function runTests() {
+ if (test('reports permission errors without silently removing managed files', () => {
+ if (process.platform === 'win32' || process.getuid?.() === 0) {
+ console.log(' (skipped — chmod ineffective on Windows/root)');
+ return;
+ }
+
</file context>
Introduce a set of Bash/Windows path normalization and test utility helpers, improve install config helpers, and harden the shell uninstaller wrapper. Key changes: - tests: add many reusable test helpers (createTempDir, cleanup, test/asyncTest runners, shell/PowerShell helpers, toBashPath/toNativePath, toBashEnv, buildBashExports, shellQuote, runShellScript improvements, readCommandLog, withPrependedPath, assert helpers, etc.) and update tests to use them. Several new test cases added (WSL path behavior, missing Node.js, missing runtime script, symlink depth limit, detect-project output parsing). - scripts/lib/install/config.js: add small helper docs and utility functions (readJson, getValidator caching, dedupeStrings, formatValidationErrors, resolveInstallConfigPath) and retain loadInstallConfig usage. - uninstall.sh: add symlink resolution depth limit, check for Node.js on PATH, verify uninstaller runtime script exists, and centralize the uninstall script path variable. - install.ps1: minor comment formatting tweak. - powershell utils: expose/getPowerShellCandidates and doc improvements. These changes improve cross-platform path handling in tests, add clearer error reporting for the uninstaller, and consolidate common test helpers for future coverage.
Comment on lines
+11
to
+50
| param( | ||
| [Parameter(Mandatory = $true)] | ||
| [string] $InitialPath, | ||
|
|
||
| [int] $MaxLinkDepth = 32 | ||
| ) | ||
|
|
||
| $currentPath = [System.IO.Path]::GetFullPath($InitialPath) | ||
| $visitedPaths = @{} | ||
|
|
||
| for ($depth = 0; $depth -le $MaxLinkDepth; $depth += 1) { | ||
| if ($visitedPaths.ContainsKey($currentPath)) { | ||
| throw "Detected symlink cycle while resolving script path: $currentPath" | ||
| } | ||
|
|
||
| $visitedPaths[$currentPath] = $true | ||
| $item = Get-Item -LiteralPath $currentPath -Force | ||
|
|
||
| if (-not $item.LinkType) { | ||
| return $currentPath | ||
| } | ||
|
|
||
| $targetPath = $item.Target | ||
| if ($targetPath -is [array]) { | ||
| $targetPath = $targetPath[0] | ||
| } | ||
|
|
||
| if (-not $targetPath) { | ||
| throw "Unable to resolve symlink target for script path: $currentPath" | ||
| } | ||
|
|
||
| if (-not [System.IO.Path]::IsPathRooted($targetPath)) { | ||
| $targetPath = Join-Path -Path $item.DirectoryName -ChildPath $targetPath | ||
| } | ||
|
|
||
| $currentPath = [System.IO.Path]::GetFullPath($targetPath) | ||
| } | ||
|
|
||
| throw "Exceeded symlink resolution depth limit ($MaxLinkDepth) while resolving script path: $InitialPath" | ||
| } |
Contributor
There was a problem hiding this comment.
Resolve-LinkedScriptPath duplicated from install.ps1
The Resolve-LinkedScriptPath function (lines 11–50) is copied verbatim from the refactored install.ps1. Any future fix to the symlink-resolution logic (e.g., adjusting MaxLinkDepth, changing error messages) would need to be applied in two places. Consider extracting it to a shared helper (e.g., scripts/resolve-script-path.ps1) and dot-sourcing it from both wrappers, or accept the duplication with an explicit comment noting the copy relationship so a future maintainer knows to sync changes.
Contributor
There was a problem hiding this comment.
2 issues found across 11 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/scripts/uninstall-sh.test.js">
<violation number="1" location="tests/scripts/uninstall-sh.test.js:133">
P1: The new Node-missing test blanks PATH so aggressively that `bash` (and required shell utilities) may be unavailable, causing ENOENT or unrelated shell errors before the wrapper reaches the intended Node.js preflight check.</violation>
</file>
<file name="tests/hooks/hooks.test.js">
<violation number="1" location="tests/hooks/hooks.test.js:67">
P2: Non-WSL Windows env normalization omits `CLV2_HOMUNCULUS_DIR`, causing inconsistent Bash path handling versus the WSL path.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| cwd: projectDir, | ||
| homeDir, | ||
| env: { | ||
| PATH: emptyPathDir, |
Contributor
There was a problem hiding this comment.
P1: The new Node-missing test blanks PATH so aggressively that bash (and required shell utilities) may be unavailable, causing ENOENT or unrelated shell errors before the wrapper reaches the intended Node.js preflight check.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/scripts/uninstall-sh.test.js, line 133:
<comment>The new Node-missing test blanks PATH so aggressively that `bash` (and required shell utilities) may be unavailable, causing ENOENT or unrelated shell errors before the wrapper reaches the intended Node.js preflight check.</comment>
<file context>
@@ -103,6 +120,85 @@ function runTests() {
+ cwd: projectDir,
+ homeDir,
+ env: {
+ PATH: emptyPathDir,
+ },
+ });
</file context>
| return nextEnv; | ||
| } | ||
|
|
||
| const pathKeys = ['HOME', 'USERPROFILE', 'TMP', 'TEMP', 'TMPDIR', 'CLAUDE_PROJECT_DIR']; |
Contributor
There was a problem hiding this comment.
P2: Non-WSL Windows env normalization omits CLV2_HOMUNCULUS_DIR, causing inconsistent Bash path handling versus the WSL path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/hooks/hooks.test.js, line 67:
<comment>Non-WSL Windows env normalization omits `CLV2_HOMUNCULUS_DIR`, causing inconsistent Bash path handling versus the WSL path.</comment>
<file context>
@@ -24,11 +27,104 @@ function toBashPath(filePath, platform = process.platform, env = process.env) {
+ return nextEnv;
+ }
+
+ const pathKeys = ['HOME', 'USERPROFILE', 'TMP', 'TEMP', 'TMPDIR', 'CLAUDE_PROJECT_DIR'];
+ const isWsl = Boolean(env.WSLENV || env.WSL_DISTRO_NAME);
+
</file context>
Suggested change
| const pathKeys = ['HOME', 'USERPROFILE', 'TMP', 'TEMP', 'TMPDIR', 'CLAUDE_PROJECT_DIR']; | |
| const pathKeys = ['HOME', 'USERPROFILE', 'TMP', 'TEMP', 'TMPDIR', 'CLAUDE_PROJECT_DIR', 'CLV2_HOMUNCULUS_DIR']; |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/hooks/hooks.test.js (1)
281-286:⚠️ Potential issue | 🟡 MinorDon’t silently drop malformed shim-log entries.
Returning
nullon JSON parse failure can mask harness bugs and produce false-green assertions. Fail fast with a line-specific parse error instead.💡 Suggested patch
- .map(line => { + .map((line, index) => { try { return JSON.parse(line); - } catch { - return null; + } catch (error) { + throw new Error(`Invalid JSON in command log at line ${index + 1}: ${error.message}`); } }) - .filter(Boolean); + .filter(Boolean);As per coding guidelines: "Always handle errors explicitly at every level and never silently swallow errors".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hooks/hooks.test.js` around lines 281 - 286, The map callback that currently does try { return JSON.parse(line); } catch { return null; } silently swallows parse failures; instead rethrow a descriptive, line-specific error so tests fail fast—replace the catch branch to throw a new Error (including the offending line content or its index) when JSON.parse(line) fails, referencing the same .map callback/JSON.parse(line) so malformed shim-log entries surface immediately.scripts/lib/install/config.js (1)
15-20:⚠️ Potential issue | 🟡 Minor
readJsoncurrently mislabels non-parse failures as JSON syntax errors.Line 19 reports
"Invalid JSON"even for file-read issues (e.g., EISDIR/EPERM), which can mislead debugging.Proposed fix
function readJson(filePath, label) { + let content; try { - return JSON.parse(fs.readFileSync(filePath, 'utf8')); + content = fs.readFileSync(filePath, 'utf8'); } catch (error) { - throw new Error(`Invalid JSON in ${label}: ${error.message}`); + throw new Error(`Unable to read ${label}: ${error.message}`); + } + + try { + return JSON.parse(content); + } catch (error) { + throw new Error(`Invalid JSON in ${label}: ${error.message}`); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/lib/install/config.js` around lines 15 - 20, The readJson function currently wraps both file-read and JSON.parse errors into a generic "Invalid JSON" message; change it to separate the file read and parse steps so file system errors (from fs.readFileSync using filePath) throw a clear filesystem error (including error.code and message) and JSON.parse errors throw an "Invalid JSON in {label}" error with the parse error message; update error handling in readJson to first read into a variable, catch/read errors and rethrow with filesystem context, then try JSON.parse and catch/throw parse-specific errors referencing label.
🧹 Nitpick comments (3)
tests/hooks/hooks.test.js (1)
381-5276: Consider splittingrunTestsinto suite-level helpers.
runTestshas grown very large, which makes failures harder to localize and increases review/maintenance cost. Breaking this into grouped suite runners (path helpers, hook wrappers, each hook script family) would materially improve maintainability.As per coding guidelines: "Keep functions small (less than 50 lines)" and "Keep files focused (less than 800 lines)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hooks/hooks.test.js` around lines 381 - 5276, runTests has become monolithic and exceeds the suggested function/line limits; split it into smaller suite-level helper functions to improve readability and maintainability. Refactor by extracting logical groups (e.g., path conversion helpers, session-start suite, session-end suite, post-edit-* suites, suggest-compact/evaluate-session suites, hooks/plugin validations) into separate functions like runPathHelpers(), runSessionStartSuite(), runSessionEndSuite(), runFormatSuite(), runTypecheckSuite(), runSuggestCompactSuite(), then have runTests simply call these helpers and aggregate passed/failed counts; update any helper utilities (createEvalWrapper, createTestDir, runScript) visibility if needed and keep each extracted function <= ~50 lines and confined to the same file or split into multiple test modules to stay under file size guideline.tests/scripts/uninstall.test.js (1)
317-349: Consider extracting the repeated install-state fixture payload.This block duplicates a large state object used in other tests, which increases drift risk when schema fields evolve.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/scripts/uninstall.test.js` around lines 317 - 349, The large install-state object passed to writeState is duplicated across tests; extract it into a shared fixture or factory (e.g., createInstallState or INSTALL_STATE_FIXTURE) and import/use it in uninstall.test.js instead of inlining the object; ensure the factory accepts overrides for fields like adapter.id, targetRoot, operations, source.repoCommit so tests can tweak values without copying the whole payload, and update the call sites to pass statePath and any necessary overrides into writeState(createInstallState({...})).tests/scripts/powershell-test-utils.js (1)
40-68: Add a fail-fast guard for invalidcommandvalues.Line 40 currently assumes a valid executable name; a non-string/empty input should throw a clearer validation error before invoking
which/where.exe.Proposed fix
function resolveExecutablePath(command, platform = process.platform, execFile = execFileSync) { + if (typeof command !== 'string' || command.trim() === '') { + throw new Error('Executable command must be a non-empty string.'); + } + const locator = platform === 'win32' ? 'where.exe' : 'which';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/scripts/powershell-test-utils.js` around lines 40 - 68, The function resolveExecutablePath lacks input validation for the command parameter; add a fail-fast guard at the start of resolveExecutablePath that verifies command is a non-empty string (e.g., typeof command === 'string' && command.trim().length > 0) and throw a clear TypeError (or Error) if invalid, before calling execFile/which/where.exe; keep existing behavior and error wrapping intact so only validated inputs reach the locator invocation and the catch block still wraps runtime failures with the detailed message and cause.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/hooks/hooks.test.js`:
- Around line 67-68: The test's toBashEnv path normalization misses
CLV2_HOMUNCULUS_DIR leading to raw Windows paths in non-WSL Bash; update the
pathKeys array used by toBashEnv to include 'CLV2_HOMUNCULUS_DIR' (alongside
existing keys like 'HOME','USERPROFILE','TMP', etc.), and ensure the logic that
checks isWsl (Boolean(env.WSLENV || env.WSL_DISTRO_NAME)) still bypasses
normalization for WSL but converts backslashes/drive letters to POSIX form for
non-WSL Windows when building Bash exports; locate the variables pathKeys, isWsl
and the toBashEnv helper in the test and add the key so CLV2_HOMUNCULUS_DIR is
treated as path-like consistently with the export handling.
---
Outside diff comments:
In `@scripts/lib/install/config.js`:
- Around line 15-20: The readJson function currently wraps both file-read and
JSON.parse errors into a generic "Invalid JSON" message; change it to separate
the file read and parse steps so file system errors (from fs.readFileSync using
filePath) throw a clear filesystem error (including error.code and message) and
JSON.parse errors throw an "Invalid JSON in {label}" error with the parse error
message; update error handling in readJson to first read into a variable,
catch/read errors and rethrow with filesystem context, then try JSON.parse and
catch/throw parse-specific errors referencing label.
In `@tests/hooks/hooks.test.js`:
- Around line 281-286: The map callback that currently does try { return
JSON.parse(line); } catch { return null; } silently swallows parse failures;
instead rethrow a descriptive, line-specific error so tests fail fast—replace
the catch branch to throw a new Error (including the offending line content or
its index) when JSON.parse(line) fails, referencing the same .map
callback/JSON.parse(line) so malformed shim-log entries surface immediately.
---
Nitpick comments:
In `@tests/hooks/hooks.test.js`:
- Around line 381-5276: runTests has become monolithic and exceeds the suggested
function/line limits; split it into smaller suite-level helper functions to
improve readability and maintainability. Refactor by extracting logical groups
(e.g., path conversion helpers, session-start suite, session-end suite,
post-edit-* suites, suggest-compact/evaluate-session suites, hooks/plugin
validations) into separate functions like runPathHelpers(),
runSessionStartSuite(), runSessionEndSuite(), runFormatSuite(),
runTypecheckSuite(), runSuggestCompactSuite(), then have runTests simply call
these helpers and aggregate passed/failed counts; update any helper utilities
(createEvalWrapper, createTestDir, runScript) visibility if needed and keep each
extracted function <= ~50 lines and confined to the same file or split into
multiple test modules to stay under file size guideline.
In `@tests/scripts/powershell-test-utils.js`:
- Around line 40-68: The function resolveExecutablePath lacks input validation
for the command parameter; add a fail-fast guard at the start of
resolveExecutablePath that verifies command is a non-empty string (e.g., typeof
command === 'string' && command.trim().length > 0) and throw a clear TypeError
(or Error) if invalid, before calling execFile/which/where.exe; keep existing
behavior and error wrapping intact so only validated inputs reach the locator
invocation and the catch block still wraps runtime failures with the detailed
message and cause.
In `@tests/scripts/uninstall.test.js`:
- Around line 317-349: The large install-state object passed to writeState is
duplicated across tests; extract it into a shared fixture or factory (e.g.,
createInstallState or INSTALL_STATE_FIXTURE) and import/use it in
uninstall.test.js instead of inlining the object; ensure the factory accepts
overrides for fields like adapter.id, targetRoot, operations, source.repoCommit
so tests can tweak values without copying the whole payload, and update the call
sites to pass statePath and any necessary overrides into
writeState(createInstallState({...})).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 94ad1ad0-6f18-4d4b-b9f7-0219fdfccdc4
📒 Files selected for processing (11)
install.ps1scripts/lib/install/config.jstests/hooks/hooks.test.jstests/lib/install-config.test.jstests/scripts/install-ps1.test.jstests/scripts/powershell-test-utils.jstests/scripts/powershell-test-utils.test.jstests/scripts/uninstall-ps1.test.jstests/scripts/uninstall-sh.test.jstests/scripts/uninstall.test.jsuninstall.sh
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/lib/install-config.test.js
- uninstall.sh
- tests/scripts/uninstall-sh.test.js
- tests/scripts/powershell-test-utils.test.js
Comment on lines
+67
to
+68
| const pathKeys = ['HOME', 'USERPROFILE', 'TMP', 'TEMP', 'TMPDIR', 'CLAUDE_PROJECT_DIR']; | ||
| const isWsl = Boolean(env.WSLENV || env.WSL_DISTRO_NAME); |
Contributor
There was a problem hiding this comment.
Normalize CLV2_HOMUNCULUS_DIR in toBashEnv for non-WSL Windows runs.
Line 67 path keys omit CLV2_HOMUNCULUS_DIR, but Line 105 treats it as path-like. This creates inconsistent behavior between non-WSL (toBashEnv) and WSL/export paths, and can leave a raw C:\... value in Bash.
💡 Suggested patch
- const pathKeys = ['HOME', 'USERPROFILE', 'TMP', 'TEMP', 'TMPDIR', 'CLAUDE_PROJECT_DIR'];
+ const pathKeys = ['HOME', 'USERPROFILE', 'TMP', 'TEMP', 'TMPDIR', 'CLAUDE_PROJECT_DIR', 'CLV2_HOMUNCULUS_DIR'];Also applies to: 105-105, 201-201
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/hooks/hooks.test.js` around lines 67 - 68, The test's toBashEnv path
normalization misses CLV2_HOMUNCULUS_DIR leading to raw Windows paths in non-WSL
Bash; update the pathKeys array used by toBashEnv to include
'CLV2_HOMUNCULUS_DIR' (alongside existing keys like 'HOME','USERPROFILE','TMP',
etc.), and ensure the logic that checks isWsl (Boolean(env.WSLENV ||
env.WSL_DISTRO_NAME)) still bypasses normalization for WSL but converts
backslashes/drive letters to POSIX form for non-WSL Windows when building Bash
exports; locate the variables pathKeys, isWsl and the toBashEnv helper in the
test and add the key so CLV2_HOMUNCULUS_DIR is treated as path-like consistently
with the export handling.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What Changed
This pull request introduces comprehensive uninstall support for ECC-managed files across platforms, improves PowerShell script resolution and error handling, and enhances path conversion and config resolution logic. It also adds robust testing for new and existing functionality, ensuring reliability and cross-platform compatibility.
Uninstall support and documentation
uninstall.shanduninstall.ps1scripts, CLI commands, and updatedREADME.mdwith uninstall instructions for macOS/Linux, Windows, and cross-platform usage. The uninstaller safely removes only ECC-managed files and provides dry-run and error handling guidance. (README.md,uninstall.sh,uninstall.ps1)PowerShell script resolution and error handling
install.ps1to robustly resolve symlinks, check for Node.js and installer script presence, and surface user-friendly errors when prerequisites are missing. (install.ps1)Path conversion and config resolution improvements
toBashPathto handle WSL, Cygwin, and MSYS environments, improving Windows-to-Unix path conversion for hooks. Added tests for all cases. (tests/hooks/hooks.test.js)Testing and validation
uninstall.ps1wrapper, validating package file publishing and delegation to Node uninstaller, including dry-run output and state preservation. (tests/scripts/uninstall-ps1.test.js)install.ps1tests to cover missing Node.js and installer script scenarios, ensuring user-friendly errors and robust preflight checks. (tests/scripts/install-ps1.test.js)These changes collectively improve ECC's installation and uninstallation experience, reliability, and cross-platform support.
Testing Done
node tests/run-all.js)Type of Change
fix:Bug fixfeat:New featurerefactor:Code refactoringdocs:Documentationtest:Testschore:Maintenance/toolingci:CI/CD changesSecurity & Quality Checklist
Documentation
Summary by cubic
Adds native uninstall entrypoints with safe, previewable removal and clear errors, plus hardened symlink and path handling with new cross‑platform test utilities.
New Features
uninstall.shanduninstall.ps1that resolve symlinks, delegate to the Node uninstaller, support--dry-run/--json, and only remove ECC‑managed files.package.jsonand updated README with macOS/Linux, Windows, andnpx ecc uninstallexamples.Refactors
Resolve-LinkedScriptPathhelper with cycle detection and a 32‑link depth limit; applied toinstall.ps1anduninstall.ps1. Switched install config path resolution topath.resolve.toBashPathfor WSL/Cygwin/MSYS and env exports; new PowerShell utils (resolvePowerShellCommand,resolveExecutablePath); added cases for missing Node/runtime script, symlink depth limit, and permission‑error reporting.Written for commit c7c7a3a. Summary will update on new commits.
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Tests