fix(cli): sanitize init target directory#152
Conversation
* fix(cli): sanitize init target directory to avoid shell-breaking names Names with spaces (e.g. "future of open slide") used to produce a "Next steps" `cd ...` line the shell rejected. Sanitize the entered name (spaces → hyphens, shell-unsafe chars stripped) and re-prompt with the safe suggestion; in non-TTY mode, error out with the suggestion. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(cli): show suggested safe name in init warning The warning previously only described the problem and relied on the re-prompt's prefilled default to communicate the fix. Print the suggested name (e.g. `future-of-open-slide`) on the warning line itself so the user sees a concrete example before answering the prompt. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@itskylechung is attempting to deploy a commit to the Yiwei Ho Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThis PR adds directory-name sanitization to the CLI ChangesDirectory name sanitization in init
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 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. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 01be4495b1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (trimmed === '.' || trimmed === '..') return trimmed; | ||
| const cleaned = trimmed | ||
| .replace(/\s+/g, '-') | ||
| .replace(/[^A-Za-z0-9_./-]/g, '-') |
There was a problem hiding this comment.
sanitizeDirName treats \ as an invalid character and rewrites it to -, so on Windows an input like slides\q2 is transformed into slides-q2 (or rejected in non-TTY mode) instead of keeping the intended nested directory. This is a regression from prior behavior where Windows-native path separators worked, and it breaks valid init usage for Windows users passing normal paths.
Useful? React with 👍 / 👎.
| .replace(/(^-|-$)/g, '') | ||
| .replace(/-*\/-*/g, '/'); |
There was a problem hiding this comment.
The current cleanup can convert relative, invalid input into absolute-looking paths (for example !!!/!!! becomes /) because hyphen trimming and slash normalization remove segment content. In TTY mode this is suggested as the default and can lead users to scaffold into / (or another absolute path) after confirming overwrite, which is a dangerous change in target scope introduced by this sanitizer.
Useful? React with 👍 / 👎.
![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
🧹 Nitpick comments (1)
packages/cli/src/init.ts (1)
45-45: ⚡ Quick winAllow Unicode letters/numbers in
sanitizeDirNameinstead of forcing ASCII-onlyThe current allowlist on
packages/cli/src/init.ts(line 45) replaces all non-ASCII characters with-, flattening localized names (e.g.,my-slides). Since the repo targets Node>=18, Unicode property escapes are available for a safer UX-focused allowlist.Suggested update
- .replace(/[^A-Za-z0-9_./-]/g, '-') + .replace(/[^\p{L}\p{N}_./-]/gu, '-')🤖 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 `@packages/cli/src/init.ts` at line 45, Replace the ASCII-only character class in sanitizeDirName that currently uses /[^A-Za-z0-9_./-]/g with a Unicode-aware regex like /[^\p{L}\p{N}_./-]/gu so letters and numbers from all scripts are preserved; update the call site in sanitizeDirName to use the 'u' flag (and keep 'g') when calling replace. Ensure the regex is the one used inside sanitizeDirName and run tests to confirm localized names (e.g., accented or non-Latin) are no longer replaced with hyphens.
🤖 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 @.changeset/sanitize-init-target-dir.md:
- Line 5: Replace the verbose explanatory changeset line in the
sanitize-init-target-dir changeset with a single present-tense, user-facing
one-liner; e.g. "Sanitize target directory names during init so suggested cd
commands are shell-safe." Ensure it's short, direct, present-tense, and contains
no rationale or implementation details.
---
Nitpick comments:
In `@packages/cli/src/init.ts`:
- Line 45: Replace the ASCII-only character class in sanitizeDirName that
currently uses /[^A-Za-z0-9_./-]/g with a Unicode-aware regex like
/[^\p{L}\p{N}_./-]/gu so letters and numbers from all scripts are preserved;
update the call site in sanitizeDirName to use the 'u' flag (and keep 'g') when
calling replace. Ensure the regex is the one used inside sanitizeDirName and run
tests to confirm localized names (e.g., accented or non-Latin) are no longer
replaced with hyphens.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9311287c-b8ea-4d80-bb95-846eea8e4a69
📒 Files selected for processing (4)
.changeset/sanitize-init-target-dir.mdpackages/cli/src/index.tspackages/cli/src/init.test.tspackages/cli/src/init.ts
- Preserve non-ASCII letters and digits by switching to Unicode property
escapes (\p{L}\p{N}) — `投影片`, `スライド`, `café` now survive
sanitization instead of being flattened to hyphens.
- Allow Windows backslash separators so `slides\q2` stays `slides\q2`
instead of becoming `slides-q2`. The hyphen-around-separator collapse
now applies symmetrically to `/` and `\`.
- Fall back to `my-slides` when sanitization would yield a root-like
path (e.g. `!!!/!!!` previously suggested `/`, which could scaffold
into filesystem root if confirmed).
- Tighten changeset wording to one direct user-facing line.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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 `@packages/cli/src/init.ts`:
- Around line 45-49: The current sanitization treats backslash as a safe
separator on all OSes, leaving shell-fragile names like "slides\q2"; update the
sanitization in init.ts so backslash is only preserved on Windows. Concretely,
when building `cleaned` adjust the character-class and the separator-normalizing
replace (the two regexes that include `\\` and the final replace that matches
`([/\\])`) to conditionally allow backslash only when `process.platform ===
'win32'` (or use `path.sep`), otherwise treat `\` as an unsafe character to be
replaced—ensure `cleaned` normalization and the early-empty check still work the
same.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fcec4be5-9746-4812-8217-ed30b43d2f4f
📒 Files selected for processing (3)
.changeset/sanitize-init-target-dir.mdpackages/cli/src/init.test.tspackages/cli/src/init.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/sanitize-init-target-dir.md
| .replace(/[^\\\p{L}\p{N}_./-]/gu, '-') | ||
| .replace(/-+/g, '-') | ||
| .replace(/(^-|-$)/g, '') | ||
| .replace(/-*([/\\])-*/g, '$1'); | ||
| if (cleaned === '' || /^[/\\]+$/.test(cleaned)) return 'my-slides'; |
There was a problem hiding this comment.
Backslash should not be preserved on non-Windows platforms.
On Line 45 and Line 48, \ is treated as a safe path separator for every OS. In POSIX shells, backslash is an escape character, so names like slides\q2 remain shell-fragile and can still break unquoted cd guidance.
Suggested fix
export function sanitizeDirName(value: string): string {
const trimmed = value.trim();
if (trimmed === '.' || trimmed === '..') return trimmed;
+ const unsafeChars = IS_WINDOWS ? /[^\\\p{L}\p{N}_./-]/gu : /[^\p{L}\p{N}_./-]/gu;
+ const separatorWithHyphens = IS_WINDOWS ? /-*([/\\])-*/g : /-*(\/)-*/g;
+ const rootLikeOnly = IS_WINDOWS ? /^[/\\]+$/ : /^\/+$/;
const cleaned = trimmed
.replace(/\s+/g, '-')
- .replace(/[^\\\p{L}\p{N}_./-]/gu, '-')
+ .replace(unsafeChars, '-')
.replace(/-+/g, '-')
.replace(/(^-|-$)/g, '')
- .replace(/-*([/\\])-*/g, '$1');
- if (cleaned === '' || /^[/\\]+$/.test(cleaned)) return 'my-slides';
+ .replace(separatorWithHyphens, '$1');
+ if (cleaned === '' || rootLikeOnly.test(cleaned)) return 'my-slides';
return cleaned;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .replace(/[^\\\p{L}\p{N}_./-]/gu, '-') | |
| .replace(/-+/g, '-') | |
| .replace(/(^-|-$)/g, '') | |
| .replace(/-*([/\\])-*/g, '$1'); | |
| if (cleaned === '' || /^[/\\]+$/.test(cleaned)) return 'my-slides'; | |
| export function sanitizeDirName(value: string): string { | |
| const trimmed = value.trim(); | |
| if (trimmed === '.' || trimmed === '..') return trimmed; | |
| const unsafeChars = IS_WINDOWS ? /[^\\\p{L}\p{N}_./-]/gu : /[^\p{L}\p{N}_./-]/gu; | |
| const separatorWithHyphens = IS_WINDOWS ? /-*([/\\])-*/g : /-*(\/)-*/g; | |
| const rootLikeOnly = IS_WINDOWS ? /^[/\\]+$/ : /^\/+$/; | |
| const cleaned = trimmed | |
| .replace(/\s+/g, '-') | |
| .replace(unsafeChars, '-') | |
| .replace(/-+/g, '-') | |
| .replace(/(^-|-$)/g, '') | |
| .replace(separatorWithHyphens, '$1'); | |
| if (cleaned === '' || rootLikeOnly.test(cleaned)) return 'my-slides'; | |
| return cleaned; | |
| } |
🤖 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 `@packages/cli/src/init.ts` around lines 45 - 49, The current sanitization
treats backslash as a safe separator on all OSes, leaving shell-fragile names
like "slides\q2"; update the sanitization in init.ts so backslash is only
preserved on Windows. Concretely, when building `cleaned` adjust the
character-class and the separator-normalizing replace (the two regexes that
include `\\` and the final replace that matches `([/\\])`) to conditionally
allow backslash only when `process.platform === 'win32'` (or use `path.sep`),
otherwise treat `\` as an unsafe character to be replaced—ensure `cleaned`
normalization and the early-empty check still work the same.
|
@itskylechung hey Kyle! |
2 participants
Summary
npx @open-slide/cli initpreviously accepted directory names with spaces (e.g.future of open slide), then printed acd ...line in "Next steps" that the shell rejected (cd: too many arguments).init— spaces → hyphens, shell-unsafe characters stripped, leading/trailing hyphens trimmed, hyphens around path separators collapsed.Test plan
pnpm test— 218/218 pass, includes 11 new tests inpackages/cli/src/init.test.tscovering safe passthrough,./.., whitespace, shell metacharacters, fallback when nothing usable remains, path separators, idempotence, trailing slash, and hyphens collapsing around separators.pnpm exec biome check packages/cli/src/...— clean.init "future of open slide..."→ errors with suggested safe name and exits 1.initand enter a name with spaces → warning showsSuggested: "..."and re-prompts with the safe default.🤖 Generated with Claude Code
Summary by CodeRabbit