Recovered: fix(test): restore full test suite on Windows (#4 by @SahilRakhaiya05)#243
Recovered: fix(test): restore full test suite on Windows (#4 by @SahilRakhaiya05)#243jangjos-128 wants to merge 3 commits into
Conversation
- Strip both slash and backslash trailing separators in resolveBundleDir - Add cross-platform npm helper for subprocess/snapshot builds - Set USERPROFILE alongside HOME in subprocess tests - Replace Unix-only rm with unlinkSync for credential cleanup - Skip symlink and chmod assertions when platform cannot honor them - Normalize CRLF in frontmatter description parsing - Enforce LF line endings via .gitattributes
Rebased onto v0.2.0 main introduced skill-nudge tests that assumed Unix-style path prefixes. Use path.join/resolve and TARGETS landing paths so probes match on Windows.
WalkthroughChangesCross-platform compatibility
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/lib/skill-nudge.test.ts`:
- Line 89: Replace the prefix-only containment checks in both assertions of the
skill-nudge tests with boundary-safe path.relative checks: resolve each observed
path against the resolved root, and accept it only when the relative result is
neither absolute nor equal to or nested under .. . Preserve the existing
every-based assertions and test intent.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3d31c606-a737-4562-b2bf-d8bec80a7295
📒 Files selected for processing (10)
.gitattributessrc/commands/agent.test.tssrc/lib/agent-targets.test.tssrc/lib/bundle.test.tssrc/lib/bundle.tssrc/lib/credentials.test.tssrc/lib/skill-nudge.test.tstest/cli.subprocess.test.tstest/help.snapshot.test.tstest/helpers/execNpm.ts
| }); | ||
| expect(seen.every(p => p.startsWith('/some/proj'))).toBe(true); | ||
| const resolvedRoot = path.resolve(root); | ||
| expect(seen.every(p => path.resolve(p).startsWith(resolvedRoot))).toBe(true); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use a boundary-safe path containment check.
startsWith(resolvedRoot) also accepts sibling paths such as /work/proj2 for /work/proj, so these tests may miss an escaped probe. Use path.relative and reject .. and absolute results in both assertions.
As per path instructions, the src/** review must focus on correctness; these prefix-only path assertions are not containment-safe.
Suggested fix
+const isWithin = (root: string, candidate: string): boolean => {
+ const relative = path.relative(root, path.resolve(candidate));
+ return (
+ relative === '' ||
+ (!path.isAbsolute(relative) &&
+ relative !== '..' &&
+ !relative.startsWith(`..${path.sep}`))
+ );
+};
+
-expect(seen.every(p => path.resolve(p).startsWith(resolvedRoot))).toBe(true);
+expect(seen.every(p => isWithin(resolvedRoot, p))).toBe(true);
-expect(probed.every(p => path.resolve(p).startsWith(resolvedCwd))).toBe(true);
+expect(probed.every(p => isWithin(resolvedCwd, p))).toBe(true);Also applies to: 216-216
🤖 Prompt for 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.
In `@src/lib/skill-nudge.test.ts` at line 89, Replace the prefix-only containment
checks in both assertions of the skill-nudge tests with boundary-safe
path.relative checks: resolve each observed path against the resolved root, and
accept it only when the relative result is neither absolute nor equal to or
nested under .. . Preserve the existing every-based assertions and test intent.
Source: Path instructions
|
Hi @SahilRakhaiya05 👋 — this is the recovery of your PR #4, which was auto-closed by an error in our 2026-07-09 release process. Your commits are intact — thank you for the contribution! It currently shows merge conflicts because Option A — merge (no force-push): git checkout BRANCH
git fetch upstream # your remote for TestSprite/testsprite-cli (may be "origin")
git merge upstream/main # resolve conflicts, commit
git pushOption B — rebase (cleaner history, needs force): git checkout BRANCH
git fetch upstream
git rebase upstream/main # resolve conflicts
git push --force-with-leaseOnce you push, the conflicts here resolve automatically and we'll take it into review. Sorry again for the extra step! |
2 participants
Fixes 10 Windows-only test failures so the Vitest suite runs cleanly on Windows dev machines. Linux/macOS CI behavior is unchanged.
Windows result:
What changed
resolveBundleDirto strip both/and\trailing separators while preserving roots likeC:\and/.HOMEandUSERPROFILEso credentials stay inside the test temp directory on Windows.rmusage withunlinkSync().test/helpers/execNpm.tssonpmsubprocess calls work on Windows.cmdshims.\r..gitattributesto normalize line endings to LF.0600chmod assertions on Windows because NTFS does not enforce POSIX file modes.path.resolve(),path.join(), andhomedir()based checks.Test Plan
npm test— 1416 passed, 3 skipped on Windows onlynpm run typechecknpm run lintSkipped on Windows by design:
0600test due to NTFS behaviorSummary by CodeRabbit
Bug Fixes
Tests
Solving Issue #108