fix: stabilize Windows config and install regressions#2392
Closed
dlr-1337 wants to merge 1 commit intogsd-build:mainfrom
Closed
fix: stabilize Windows config and install regressions#2392dlr-1337 wants to merge 1 commit intogsd-build:mainfrom
dlr-1337 wants to merge 1 commit intogsd-build:mainfrom
Conversation
📝 WalkthroughWalkthroughIntroduces transient filesystem read retry logic with exponential backoff, centralizes home directory resolution across configuration modules, improves cross-platform path normalization, and ensures consistent test cleanup and working directory restoration. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bin/install.js (1)
5848-5868:⚠️ Potential issue | 🟠 MajorRetry the destination writes as well.
The new wrapper only protects reading/copying the source file. Both the
.jsand.shbranches still do a singlefs.writeFileSync(destFile, content), so a transient lock on an existing hook can still abort the install on Windows.Proposed fix
+function writeFileUtf8WithRetry(filePath, content) { + return withFsRetry(() => fs.writeFileSync(filePath, content, 'utf8')); +} + // ... - fs.writeFileSync(destFile, content); + writeFileUtf8WithRetry(destFile, content); // Ensure hook files are executable (fixes `#1162` — missing +x permission) try { fs.chmodSync(destFile, 0o755); } catch (e) { /* Windows doesn't support chmod */ } } else { // .sh hooks carry a gsd-hook-version header so gsd-check-update.js can // detect staleness after updates — stamp the version just like .js hooks. if (entry.endsWith('.sh')) { let content = readFileUtf8WithRetry(srcFile); content = content.replace(/\{\{GSD_VERSION\}\}/g, pkg.version); - fs.writeFileSync(destFile, content); + writeFileUtf8WithRetry(destFile, content); try { fs.chmodSync(destFile, 0o755); } catch (e) { /* Windows doesn't support chmod */ } } else { copyFileWithRetry(srcFile, destFile); } }Also applies to: 5973-5986
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/install.js` around lines 5848 - 5868, The writes to destination files use a single fs.writeFileSync(destFile, content) and can fail on transient Windows locks; add a retry wrapper (or reuse the existing copyFileWithRetry pattern) and replace the direct fs.writeFileSync calls in both the .js branch (where content is modified for .claude/.qwen and GSD_VERSION) and the .sh branch (entry.endsWith('.sh')) with a writeFileWithRetry(destFile, content) that retries on failure a few times before throwing; keep the existing chmodSync try/catch behavior and apply the same change to the other occurrence noted around the 5973-5986 range so all destination writes are retried.
🧹 Nitpick comments (1)
get-shit-done/bin/lib/config.cjs (1)
214-220: Consider accepting string booleans inparallelizationnormalization.Line 214-220 currently treats non-boolean/non-object as invalid and falls back to defaults. Legacy/manual configs with
"true"/"false"get ignored.♻️ Suggested patch
if (typeof config.parallelization !== 'boolean') { if (config.parallelization && typeof config.parallelization === 'object' && 'enabled' in config.parallelization) { config.parallelization = !!config.parallelization.enabled; + } else if (typeof config.parallelization === 'string') { + const v = config.parallelization.trim().toLowerCase(); + if (v === 'true') config.parallelization = true; + else if (v === 'false') config.parallelization = false; + else config.parallelization = hardcoded.parallelization; } else { config.parallelization = hardcoded.parallelization; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@get-shit-done/bin/lib/config.cjs` around lines 214 - 220, The normalization for config.parallelization currently only accepts booleans or objects; update the branch that handles non-boolean values to also accept string booleans by checking if typeof config.parallelization === 'string' and converting "true"/"false" (case-insensitive) to the corresponding boolean before falling back to the existing object check or hardcoded.parallelization. Keep the existing object-path that looks for a .enabled property and ensure the new string parsing runs before defaulting to hardcoded.parallelization so legacy configs like "true"/"false" are honored.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@get-shit-done/bin/lib/core.cjs`:
- Line 100: The comparison parent === homedir is brittle because env-provided
home paths can differ by separators, trailing slashes or case; normalize both
sides before comparing by using path utilities: call path.resolve() and
path.normalize() on resolveHomeDir() and on the parent value, strip any trailing
path.sep, and on Windows compare using .toLowerCase() to be case-insensitive;
then replace the raw equality check in the code that references homedir with the
normalized/resolved comparison (use resolveHomeDir(), homedir variable, and the
parent variable names to locate and update the logic).
In `@get-shit-done/bin/lib/init.cjs`:
- Around line 1772-1773: The dedupe uses the raw skill `name`, so variants like
"MySkill" and " myskill " bypass it; normalize `name` before checking/adding to
`seenSkillNames` (e.g., compute a `normalized` value via trim() and
toLowerCase(), guard against null/undefined names), then use
`seenSkillNames.has(normalized)` and `seenSkillNames.add(normalized)` instead of
using the raw `name` so case/whitespace variants are treated as duplicates.
In `@tests/prompt-injection-scan.test.cjs`:
- Around line 63-65: The allowlist helper toAllowlistPath currently returns a
normalized relative path but the scan bucket logic still checks raw absolute
paths (e.g., conditions like f.includes('/agents/')), so on Windows those checks
never match; update the scan logic to use the normalized relative path returned
by toAllowlistPath for both allowlist membership and bucket selection instead of
the raw file path — locate uses of f.includes('/agents/') (and similar bucket
checks) and replace them to operate on the normalized path produced by
toAllowlistPath.
In `@tests/prune-orphaned-worktrees.test.cjs`:
- Line 152: The assertions normalize path separators but not case, causing
Windows flakes; update the normalization to also normalize case by applying a
consistent case conversion (e.g., toLowerCase()) after replacing backslashes
with slashes for the variables like normalizedWorktreeDir (and the analogous
normalized... variables used around lines where you replace /\\/g with '/'), so
that comparisons use the same lowercase path form on Windows; ensure every place
that does worktreeDir.replace(/\\/g, '/') also chains .toLowerCase() (or a
single helper normalizePath that does both) so all assertions are stable across
drive-letter/path case differences.
---
Outside diff comments:
In `@bin/install.js`:
- Around line 5848-5868: The writes to destination files use a single
fs.writeFileSync(destFile, content) and can fail on transient Windows locks; add
a retry wrapper (or reuse the existing copyFileWithRetry pattern) and replace
the direct fs.writeFileSync calls in both the .js branch (where content is
modified for .claude/.qwen and GSD_VERSION) and the .sh branch
(entry.endsWith('.sh')) with a writeFileWithRetry(destFile, content) that
retries on failure a few times before throwing; keep the existing chmodSync
try/catch behavior and apply the same change to the other occurrence noted
around the 5973-5986 range so all destination writes are retried.
---
Nitpick comments:
In `@get-shit-done/bin/lib/config.cjs`:
- Around line 214-220: The normalization for config.parallelization currently
only accepts booleans or objects; update the branch that handles non-boolean
values to also accept string booleans by checking if typeof
config.parallelization === 'string' and converting "true"/"false"
(case-insensitive) to the corresponding boolean before falling back to the
existing object check or hardcoded.parallelization. Keep the existing
object-path that looks for a .enabled property and ensure the new string parsing
runs before defaulting to hardcoded.parallelization so legacy configs like
"true"/"false" are honored.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 51c93ee8-4d67-49f6-8760-9062fad0b69b
📒 Files selected for processing (10)
bin/install.jsget-shit-done/bin/lib/config.cjsget-shit-done/bin/lib/core.cjsget-shit-done/bin/lib/init.cjstests/bug-1736-local-install-commands.test.cjstests/bug-2136-sh-hook-version.test.cjstests/bug-2248-local-install-statusline.test.cjstests/few-shot-calibration.test.cjstests/prompt-injection-scan.test.cjstests/prune-orphaned-worktrees.test.cjs
| const resolved = path.resolve(startDir); | ||
| const root = path.parse(resolved).root; | ||
| const homedir = require('os').homedir(); | ||
| const homedir = resolveHomeDir(); |
There was a problem hiding this comment.
Normalize home path before ancestor-stop comparison.
Line 127 compares parent === homedir as raw strings. With env-provided home paths (different separators/casing/trailing slash), this can miss the home boundary check.
🔧 Suggested patch
- const homedir = resolveHomeDir();
+ const homedir = normalizeComparablePath(resolveHomeDir());
@@
- if (parent === homedir) break; // never go above home
+ if (normalizeComparablePath(parent) === homedir) break; // never go above homeAlso applies to: 127-127
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@get-shit-done/bin/lib/core.cjs` at line 100, The comparison parent ===
homedir is brittle because env-provided home paths can differ by separators,
trailing slashes or case; normalize both sides before comparing by using path
utilities: call path.resolve() and path.normalize() on resolveHomeDir() and on
the parent value, strip any trailing path.sep, and on Windows compare using
.toLowerCase() to be case-insensitive; then replace the raw equality check in
the code that references homedir with the normalized/resolved comparison (use
resolveHomeDir(), homedir variable, and the parent variable names to locate and
update the logic).
Comment on lines
+1772
to
+1773
| if (seenSkillNames.has(name)) continue; | ||
| seenSkillNames.add(name); |
There was a problem hiding this comment.
Make skill-name dedupe robust to case/whitespace variants.
Line 1772/Line 1773 dedupe on raw name, so "MySkill" and " myskill " are treated as different entries and can still duplicate in the manifest.
🔧 Suggested patch
- const name = frontmatter.name || entry.name;
+ const name = String(frontmatter.name || entry.name).trim();
const description = frontmatter.description || '';
- if (seenSkillNames.has(name)) continue;
- seenSkillNames.add(name);
+ const dedupeKey = name.toLowerCase();
+ if (seenSkillNames.has(dedupeKey)) continue;
+ seenSkillNames.add(dedupeKey);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@get-shit-done/bin/lib/init.cjs` around lines 1772 - 1773, The dedupe uses the
raw skill `name`, so variants like "MySkill" and " myskill " bypass it;
normalize `name` before checking/adding to `seenSkillNames` (e.g., compute a
`normalized` value via trim() and toLowerCase(), guard against null/undefined
names), then use `seenSkillNames.has(normalized)` and
`seenSkillNames.add(normalized)` instead of using the raw `name` so
case/whitespace variants are treated as duplicates.
Comment on lines
+63
to
+65
| function toAllowlistPath(file) { | ||
| return path.relative(PROJECT_ROOT, file).split(path.sep).join('/'); | ||
| } |
There was a problem hiding this comment.
Normalize the scan bucket paths too.
This only fixes allowlist membership. The actual scan buckets below still use raw absolute paths like f.includes('/agents/'), which never match Windows paths such as C:\repo\agents\..., so those tests pass without scanning anything. Normalize once here and use the normalized relative path for both allowlist checks and bucket selection.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/prompt-injection-scan.test.cjs` around lines 63 - 65, The allowlist
helper toAllowlistPath currently returns a normalized relative path but the scan
bucket logic still checks raw absolute paths (e.g., conditions like
f.includes('/agents/')), so on Windows those checks never match; update the scan
logic to use the normalized relative path returned by toAllowlistPath for both
allowlist membership and bucket selection instead of the raw file path — locate
uses of f.includes('/agents/') (and similar bucket checks) and replace them to
operate on the normalized path produced by toAllowlistPath.
| test('runs git worktree prune to clear stale references', () => { | ||
| const repoDir = path.join(tmpBase, 'repo4'); | ||
| const worktreeDir = path.join(tmpBase, 'wt-stale'); | ||
| const normalizedWorktreeDir = worktreeDir.replace(/\\/g, '/'); |
There was a problem hiding this comment.
Normalize case too, not only separators, for Windows-stable assertions.
Line 152/Line 161/Line 172 normalize \ vs /, but not casing. Git output can differ in drive-letter/path case on Windows, which can still make these assertions flaky.
🔧 Suggested patch
- const normalizedWorktreeDir = worktreeDir.replace(/\\/g, '/');
+ const normalizeForCompare = (p) => {
+ const normalized = p.replace(/\\/g, '/');
+ return process.platform === 'win32' ? normalized.toLowerCase() : normalized;
+ };
+ const normalizedWorktreeDir = normalizeForCompare(worktreeDir);
@@
- const beforeList = execSync('git worktree list --porcelain', { cwd: repoDir, encoding: 'utf8' }).replace(/\\/g, '/');
+ const beforeList = normalizeForCompare(
+ execSync('git worktree list --porcelain', { cwd: repoDir, encoding: 'utf8' })
+ );
@@
- const afterList = execSync('git worktree list --porcelain', { cwd: repoDir, encoding: 'utf8' }).replace(/\\/g, '/');
+ const afterList = normalizeForCompare(
+ execSync('git worktree list --porcelain', { cwd: repoDir, encoding: 'utf8' })
+ );Also applies to: 161-163, 172-175
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/prune-orphaned-worktrees.test.cjs` at line 152, The assertions
normalize path separators but not case, causing Windows flakes; update the
normalization to also normalize case by applying a consistent case conversion
(e.g., toLowerCase()) after replacing backslashes with slashes for the variables
like normalizedWorktreeDir (and the analogous normalized... variables used
around lines where you replace /\\/g with '/'), so that comparisons use the same
lowercase path form on Windows; ensure every place that does
worktreeDir.replace(/\\/g, '/') also chains .toLowerCase() (or a single helper
normalizePath that does both) so all assertions are stable across
drive-letter/path case differences.
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
1 participant
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.
Summary
Verification
node --test tests/config.test.cjsnode --test tests/skill-manifest.test.cjsnode --test tests/prompt-injection-scan.test.cjsnode --test tests/prune-orphaned-worktrees.test.cjsnode --test tests/few-shot-calibration.test.cjsnode --test tests/bug-1736-local-install-commands.test.cjsnode --test tests/bug-2248-local-install-statusline.test.cjsnode --test tests/bug-2136-sh-hook-version.test.cjsnpm testSummary by CodeRabbit