Fix/windows installer exceptions - #1229
Conversation
📝 WalkthroughWalkthroughThe installer now uses a dedicated commit-privacy setup CLI. The crusher shim normalizes PATH results before filtering changed entries during installation and uninstallation. ChangesInstaller hardening
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant InstallScript as scripts/install.ps1
participant PrivacyCLI as scripts/lib/apply-commit-privacy.js
participant PrivacyHelper as applyCommitPrivacyFilterCli
participant TargetScript as scripts/check-state-leak.js
InstallScript->>PrivacyCLI: pass project root
PrivacyCLI->>PrivacyHelper: configure project and target script
PrivacyHelper->>TargetScript: apply commit-privacy filtering
PrivacyCLI-->>InstallScript: prefixed logs and status
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
|
DCO sign-off missing One or more commits in this PR are missing a How to fix: # Single commit
git commit --amend -s
git push --force-with-lease
# Multiple commits -- replace N with the number of commits in your PR
git rebase --signoff HEAD~N
git push --force-with-lease
|
a52d3a1 to
903f3c7
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@scripts/crusher-shim.js`:
- Around line 17-21: Update the result normalization and output handling in
scripts/crusher-shim.js around rawPathResult, pathResultArray, and changed to
support the Windows object shape from shim-install.js: preserve a displayable
Windows location when changed is true, and surface rawPathResult.error before
falling back to the generic “already configured” message when no changes
occurred.
In `@scripts/install.ps1`:
- Line 221: Update the commit-privacy setup invocation near
apply-commit-privacy.js so it executes with the user's repository,
$InvokedFromDir, as the working directory while retaining $RootDir for locating
the script and package resources. Ensure process.cwd() resolves to the user's
repository during setup.
- Line 221: Update the install script’s Node executable setup to resolve the
binary once using the established fixed-path-with-PATH-fallback approach, then
reuse that resolved variable when invoking apply-commit-privacy.js. Ensure the
version check and CLI call use the same Node binary, and remove the bare node
invocation.
In `@scripts/lib/apply-commit-privacy.js`:
- Around line 11-14: Replace both console.log calls in the commit-privacy filter
setup and catch path with the repository’s logging abstraction, keeping the
setup message at an informational level and routing the non-fatal failure
message through warning or error logging. Preserve the existing message content
and behavior.
- Around line 13-15: Update the catch block handling applyCommitPrivacyFilterCli
failures to set process.exitCode to 1 after logging the non-fatal error.
Preserve continuation of the installer while ensuring scripts/install.ps1 can
detect the helper failure through the process exit status.
🪄 Autofix
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: ASSERTIVE
Plan: Pro Plus
Run ID: 67314f54-26da-486e-b6df-07df09033d26
📒 Files selected for processing (3)
scripts/crusher-shim.jsscripts/install.ps1scripts/lib/apply-commit-privacy.js
… function Signed-off-by: Akisolu <morillomiguel2707@gmail.com>
Signed-off-by: Akisolu <morillomiguel2707@gmail.com>
903f3c7 to
266e845
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…exit codes Signed-off-by: Akisolu <morillomiguel2707@gmail.com>
Fmarzochi
left a comment
Fmarzochi
left a comment
There was a problem hiding this comment.
First code PR and you killed both crashes for real: end-to-end tested on the exact PowerShell that bit us, rebased cleanly on top of the stdio fix, tight scope, green CI and a passed quality gate. This is exactly the shape a fix PR should have. 👏
Four things to make it land solid, three of them Windows-only edge cases surfaced by the same investigation:
-
printUninstallResultstill has the original crash (scripts/crusher-shim.js:30): it callsresult.pathResult.filter(...)just like the install path did, and on WindowsuninstallWindowsPath()returns the same single object, soegc crusher-shim uninstallwould still throw the exact TypeError you just fixed for install. Suggestion: extract your normalization into a tiny helper (e.g.toPathResultArray(raw)) and use it in both printers, so the contract lives in one place. -
The new script call only resolves from the repo root (
scripts/install.ps1:221):node scripts/lib/apply-commit-privacy.js $RootDiris relative to the caller's working directory. Running the installer from anywhere else (powershell -File C:\path\to\EGC\scripts\install.ps1from your home directory, for example) dies withCannot find module. The original-eform was immune because it received$RootDirand required an absolute path. Fix:node "$RootDir/scripts/lib/apply-commit-privacy.js" $RootDir(the quotes also keep paths with spaces safe). -
On Windows the "changed" message prints an empty path:
installWindowsPath()returns{ changed, error }with nopathfield, so when the PATH really does get updated,changed.map(r => r.path).join(', ')rendersPATH updated in:with nothing after it. Cheapest fix inside your normalization: when wrapping the single object, give it a label, e.g.[{ path: 'Windows user PATH (registry)', ...rawPathResult }]. -
The try/catch in
apply-commit-privacy.jsmakes the ps1 error check unreachable: catching everything and exiting 0 meansif ($LASTEXITCODE -ne 0)on the next line can never fire, and the "non-fatal" note now lives in two places. Simplest is to drop the try/catch entirely: if the module throws, node exits non-zero andinstall.ps1already prints the note, which is exactly how the old inline version behaved.
Nit: the new file is missing a trailing newline.
None of this needs re-architecting: all four fit inside the two files you already touched. Ping me when it's pushed and I'll run the full cycle again. 🦾
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
Quick sync, since your 691261c and my review crossed mid-air: that commit already resolves my points 3 (path label) and 4 (the exit code now reaches the ps1 check) plus the newline nit. Nice reflexes. 😄 Two corrections and one leftover:
After that this is ready to land. 🦾 |
… shim printer Signed-off-by: Akisolu <morillomiguel2707@gmail.com>
|
All 4 points addressed, @Fmarzochi!
Trailing newline added as well. Pushed and ready for the cycle! 🚀 |
|
So close! The
As written, the script dies with One mentor note, because it is a classic: if an editor AI suggested those paths, this is the textbook hallucinated import. A quick |
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Signed-off-by: Akisolu <morillomiguel2707@gmail.com>
|
Good catch, Lesson learned on double-checking AI-suggested import paths against the file system. Restored the correct references in scripts/lib/apply-commit-privacy.js: Required ./memory-filters for applyCommitPrivacyFilterCli. Pointed scriptPath to scripts/check-state-leak.js. Tested locally with node scripts/lib/apply-commit-privacy.js $PWD and confirmed zero MODULE_NOT_FOUND issues. Pushed and good to go! |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@scripts/crusher-shim.js`:
- Around line 30-31: Update the reporting logic in scripts/crusher-shim.js
around pathResultArray and changed to inspect each PATH operation result’s error
before evaluating changed or using the normal fallback. Ensure
installWindowsPath errors are reported instead of the generic already-configured
message, and uninstallWindowsPath errors are explicitly reported rather than
producing no output.
- Around line 30-31: Update the changed-result messaging in the flow using
pathResultArray and changed so Windows registry updates do not instruct users to
source the fallback label. Detect the Windows-specific registry result and print
guidance to open a new terminal; retain source instructions only for file-backed
shell updates.
🪄 Autofix
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: ASSERTIVE
Plan: Pro Plus
Run ID: 60ed5285-35bf-4358-a888-8b6b6093bf26
📒 Files selected for processing (3)
scripts/crusher-shim.jsscripts/install.ps1scripts/lib/apply-commit-privacy.js
|
Fmarzochi
left a comment
Fmarzochi
left a comment
There was a problem hiding this comment.
All review findings addressed and verified against the repo: shared toPathResultArray helper covering both printers, absolute script path, English-only comments, and the module references restored and locally tested. Green CI, quality gate passed. Approving.
|
Thank you so much for the thorough review, guidance, and patience throughout the entire process. I'm glad to contribute to EGC and see that both Windows issues have been officially resolved :D |
|
The pleasure was ours, Akisolu. This PR raised the bar for what a first code contribution looks like here: report, diagnosis, fix and verification, all from the same person. The review rounds were fast because you made them fast. See you in the next one. 🦾 |
2 participants
What Changed
scripts/install.ps1): Extracted inlinenode -escript execution into a dedicated script (scripts/lib/apply-commit-privacy.js) to avoid PowerShell 5.1 inner quote-stripping issues.crusher-shim.jsTypeError (scripts/crusher-shim.js): Added type normalization inprintInstallResult()to handleinstallWindowsPath()single-object returns ({ changed, error }) on Windows and safely wrap them in an array before calling.filter().Why This Change
node -einline scripts ininstall.ps1, causing path strings to unquote and throwingSyntaxError: Invalid regular expression flags.installWindowsPath()returns a single result object rather than an array, causingresult.pathResult.filter(...)incrusher-shim.jsto crash with aTypeError: result.pathResult.filter is not a functionduring installation summary printing.Testing Done
node tests/run-all.js)~/.egc/), a concurrent-access test was added or updatedType of Change
fix:Bug fixfeat:New featurerefactor:Code refactoringdocs:Documentationtest:Testschore:Maintenance/toolingci:CI/CD changesSecurity & Quality Checklist
Documentation
Summary by cubic
Fixes Windows installer errors on PowerShell 5.1 and resolves PATH summary crashes during install and uninstall on Windows. Fixes #1217.
node -ewithscripts/lib/apply-commit-privacy.js; restores correct./memory-filtersimport and resolvesscripts/check-state-leak.js; exit codes respected, failures are non-fatal and logged.scripts/crusher-shim.js: normalizepathResultto an array for both flows, add a default PATH label ("Windows user PATH (registry)"), and guard filtering before printing.Written for commit 0dc1b8c. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Improvements