Conversation
File paths containing shell metacharacters (like $) get backslash-escaped by git/CI tools, but normalizeFilePaths() didn't unescape them, causing validateFilesExist() to fail with "File not found" errors. This is common with React Router/Remix dynamic route files like users.$id.tsx which become users.\$id.tsx when passed through shell variables in CI. Added filePath.replace(/\\(.)/g, '$1') in normalizeFilePaths() to strip backslash escapes before checking if files exist. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
![doistbot-app[bot]](https://avatars.githubusercontent.com/in/2345791?s=60&v=4){kind=small}
There was a problem hiding this comment.
The PR correctly addresses the issue of shell-escaped paths for POSIX-style inputs, allowing tools to find files with special characters like $. However, the implementation introduces a critical regression for Windows environments by treating directory separators as escape characters. This needs to be addressed before merging.
|
|
||
| return filePaths.map((filePath) => { | ||
| // Unescape shell-escaped characters (e.g., \$ → $, \! → !) | ||
| const unescaped = filePath.replace(/\\(.)/g, '$1') |
There was a problem hiding this comment.
[P1] Windows Path Corruption
The global regex replacement /\\(.)/g will eagerly match backslashes used as path separators on Windows systems (e.g., src\utils\file.ts becomes srcutilsfile.ts). This breaks path resolution for users providing native Windows paths.
You should likely guard this logic to only run if the path does not appear to be a native Windows path (e.g., check path.sep or if the path uses backslashes as separators), or restrict the unescaping to specific target characters like \$.
There was a problem hiding this comment.
Added a check so we only handle paths with forward-slashes. This is because tools like git bash on Windows would still provide paths with backslashes, but path.sep or platform checks would indicate it's Windows. This does have the caveat that paths without any slashes would fail this check, but something like ./$id.tsx. would work.
Only apply shell escape unescaping to paths containing forward slashes. Paths using only backslashes (Windows native paths) are preserved to avoid corrupting path separators. This handles: - Git Bash on Windows (uses forward slashes with backslash escaping) - Windows CI (uses forward slashes from git) - Native Windows cmd/PowerShell (uses backslashes, no shell escaping) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
@doistbot /review |
There was a problem hiding this comment.
The PR effectively solves the issue of shell-escaped paths in CI environments. However, the unescaping logic is too broad and causes a regression for Windows paths that use mixed separators (forward and backslashes) by incorrectly stripping backslashes that are intended as directory separators.
Restrict the unescape regex to /\\([^a-zA-Z0-9])/g to preserve mixed paths like src/utils\file.ts which are valid on Windows. Shells typically escape special characters ($, space, !), not letters or digits. Windows separators are usually followed by alphanumeric directory names, so this change prevents corruption while still unescaping shell metacharacters. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
1 participant
Fixes an issue where
--check-filesand--stage-record-filefail on files with$in their names (common in React Router/Remix dynamic route files likeusers.$id.tsx).When CI tools pass file paths through shell variables, special characters get backslash-escaped. So
draft.$id.tsxbecomesdraft.\$id.tsx. The tool was looking for the escaped path literally instead of unescaping it first.Added
filePath.replace(/\\(.)/g, '$1')innormalizeFilePaths()to strip the backslash escapes before checking if files exist.Test plan
npm run buildnode dist/index.mjs --check-files 'src/__fixtures__/sample-project/src/route.\$param.tsx'- verify it finds the file🤖 Generated with Claude Code