fix: validate --path flag is a directory, not a file#7335
Open
byrichardpowell wants to merge 2 commits intomainfrom
Open
fix: validate --path flag is a directory, not a file#7335byrichardpowell wants to merge 2 commits intomainfrom
byrichardpowell wants to merge 2 commits intomainfrom
Conversation
The --path flag in theme commands only checked that the path existed using fileExistsSync(), which returns true for both files and directories. When users passed a file path (e.g. sections/foo.liquid), the downstream theme-check-node code would call readdir() on it and crash with ENOTDIR. This was causing ~490 errors across 220+ users in the last 7 days. The fix adds an isDirectorySync() check after the existence check, with a clear error message that includes a "Did you mean:" suggestion pointing to the parent directory. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Improves validation for the --path flag in theme commands to prevent passing file paths (which can lead to ENOTDIR errors downstream) and adds/updates tests and release notes.
Changes:
- Validate that
--pathexists and is a directory; otherwise render a clearer error (with a parent-directory suggestion) and exit. - Add a test covering the “file path provided” case and fix minor test wording typos.
- Add a changeset documenting the patch release.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/theme/src/cli/flags.ts | Adds directory validation and improved error output for invalid --path values. |
| packages/theme/src/cli/flags.test.ts | Adds coverage for passing a file path; fixes test description typos. |
| .changeset/fix-theme-check-path-file-validation.md | Documents the patch change in release notes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address review feedback — the error body now says "not a directory" instead of "is a file", which is more accurate for non-directory path types. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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
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
isDirectorySync()validation to the--pathflag in theme commandsreturnbeforeprocess.exit(1)calls for safety whenprocess.exitis mocked in testsProblem
The
--pathflag in all theme commands (theme check,theme dev,theme push, etc.) validated that the provided path existed usingfileExistsSync(), but did not check whether it was a directory. SincefileExistsSync()returnstruefor both files and directories, users could pass a file path likesections/foo.liquidand the validation would accept it.Downstream,
@shopify/theme-check-node'sloadConfig()callsfs.readdir(root)on the path, which throwsENOTDIR: not a directory, scandirwhen the path is a file.Impact: ~490 error events across 220+ affected users in the last 7 days (first seen in CLI 3.93.0).
The errors appear across all platforms (macOS, Windows, Linux) and affect many different theme files (
sections/*.liquid,snippets/*.liquid,templates/*.json,blocks/*.liquid,locales/*.json). The likely cause is IDE integrations or editor tasks that pass the currently open file as the--pathargument.Related issue: https://github.com/shop/issues/issues/33774
Observe dashboard: https://observe.shopify.io/a/observe/errors/13708469716602397840
Test plan
--pathcheck.test.tstests pass (these mockprocess.exitand use fake paths)🤖 Generated with Claude Code