Fix/windows path separator#27
Merged
Merged
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughIntroduces cross-platform path normalization for rm-related safety checks, updates tests and test helpers for Windows paths, makes explain cwd resolution absolute, automates dist synchronization in the pre-push hook, and adds a Windows GitHub Actions workflow to run the test suite on windows-latest. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #27 +/- ##
==========================================
- Coverage 99.51% 99.44% -0.07%
==========================================
Files 50 50
Lines 4509 4533 +24
==========================================
+ Hits 4487 4508 +21
- Misses 22 25 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Greptile OverviewGreptile SummaryThis PR improves Windows compatibility for the Key Changes
Technical ReviewThe implementation correctly handles the main Windows compatibility issues:
The Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant CLI as cc-safety-net CLI
participant Config as buildAnalyzeOptions
participant Analyze as analyzeCommand
participant RmRules as analyzeRm
participant PathNorm as normalizePathForComparison
User->>CLI: execute command with --cwd
CLI->>Config: buildAnalyzeOptions(explainOptions)
Config->>Config: resolve(cwd) to absolute path
Note over Config: Prevents relative path issues<br/>in comparison logic
Config-->>CLI: AnalyzeOptions with absolute cwd
CLI->>Analyze: analyzeCommand(command, options)
Analyze->>RmRules: analyzeRm(tokens, options)
alt Target is absolute path
RmRules->>PathNorm: normalizePathForComparison(target)
Note over PathNorm: On Windows:<br/>1. normalize()<br/>2. Replace / with \<br/>3. toLowerCase()
PathNorm-->>RmRules: normalized target
RmRules->>PathNorm: normalizePathForComparison(cwd)
PathNorm-->>RmRules: normalized cwd
RmRules->>RmRules: Check if target starts with cwd+sep
Note over RmRules: Uses platform-specific<br/>separator from path.sep
else Target is relative
RmRules->>RmRules: resolve(cwd, target)
RmRules->>PathNorm: normalizePathForComparison(resolved)
PathNorm-->>RmRules: normalized resolved path
RmRules->>PathNorm: normalizePathForComparison(cwd)
PathNorm-->>RmRules: normalized cwd
RmRules->>RmRules: Compare normalized paths
end
RmRules-->>Analyze: null (allowed) or reason (blocked)
Analyze-->>CLI: analysis result
CLI-->>User: ALLOWED or BLOCKED with reason
|
![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
🤖 Fix all issues with AI agents
In @.husky/pre-push:
- Around line 9-14: The pre-push hook uses "git add dist/*" which won't stage
deletions or nested files; replace that call with a command that stages all
changes under the dist tree (e.g., "git add --all dist/" or "git add -A dist/")
so deletions and nested paths are included before the "git commit -m 'chore:
sync dist' --no-verify" step; update the script to use that new add command in
place of "git add dist/*".
In `@src/core/rules-rm.ts`:
- Around line 7-23: normalizePathForComparison currently normalizes slashes and
case on Windows but leaves trailing path separators, which can cause false
negatives when other code appends `${sep}` to prefixes (e.g., comparisons
involving cwd/tmpdir). Update normalizePathForComparison to strip trailing
separators after normalization (for both platforms) while preserving root/drive
roots (e.g., "/" or "C:\") so you don't remove the root separator; keep
referencing IS_WINDOWS and normalize(p) and ensure the function returns the
trimmed path for downstream prefix checks.
🧹 Nitpick comments (1)
tests/helpers.ts (1)
83-90: Add@internaltag for the test-only export.This helper is exported solely for tests; please tag it to satisfy knip conventions.
As per coding guidelines, add the `@internal` tag for test-only exports.♻️ Proposed tweak
/** + * `@internal` Exported for testing * Convert Windows backslashes to forward slashes for shell command embedding. * shell-quote interprets backslashes as escape characters, which corrupts * Windows paths like C:\Users\... into C:Users... */ export function toShellPath(p: string): string { return p.replace(/\\/g, '/'); }
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
This PR improves Windows compatibility by handling Windows-specific path separators and drive letters in the
rm -rfsafety rules. It also ensures that the current working directory is always resolved to an absolute path for reliable comparison and adds a dedicated Windows CI workflow.rm -rfrule to support Windows backslashes and case-insensitive path comparisons.cwdin theexplaincommand configuration.Changes
normalizePathForComparisoninsrc/core/rules-rm.tsto handle platform differences correctly (backslashes vs. forward slashes, case-sensitivity).node:path'ssep.C:\).src/bin/explain/config.tsto resolvecwdto an absolute path, preventing mismatches when relative paths are provided..github/workflows/test-windows.ymlto run the test suite on Windows runners..husky/pre-pushto automatically sync and commitdist/changes before pushing.analyzeRm Windows path handlingintests/core/rules-rm.test.ts.tests/bin/explain/cli.test.tsto use platform-agnostic temporary directories.toShellPathhelper intests/helpers.tsto handle path escaping in tests.Testing
Manual verification was performed by running the
explaincommand with Windows-style paths and ensuring correct blocking/allowing logic on both mock and real path scenarios.Related Issue
Fixes #26
PR Checklist
bun run checkpasses (lint, types, dead code, rules, tests)package.jsonSummary by CodeRabbit
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.