🐛 Validate custom rules file types in profile file dialog - #1477
🐛 Validate custom rules file types in profile file dialog#1477ibolton336 wants to merge 1 commit into
Conversation
Fixes konveyor#580 The Select Custom Rules dialog accepted any file type. Filter the dialog to YAML rule files and validate the selection, skipping non-YAML files with a warning (dialog filters don't apply to folders and are not enforced on all platforms). Folders are still accepted as rule directories, matching the webview upload path which already validates .yaml/.yml. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughCustom rule selection now uses a YAML-specific file filter and validates each selected URI. Directories remain accepted, while non-YAML files are skipped and listed in a warning. The flow exits when no valid inputs remain, and the unreleased changelog documents this behavior. Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 install timed out. The project may have too many dependencies for the sandbox. 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 |
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 `@vscode/core/src/utilities/profiles/profileActions.ts`:
- Around line 21-34: Update the selection loop in profileActions to wrap each
workspace.fs.stat call in try/catch, recording failures separately from skipped
non-YAML filenames and continuing with subsequent URIs. Preserve the existing
directory/YAML validation for successful stats and ensure both failure
categories remain available for later handling.
🪄 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: 9d9e90cc-55ae-46a6-88ce-f3b6e69f6ae2
📒 Files selected for processing (2)
changes/unreleased/580-validate-custom-rules-file-types.yamlvscode/core/src/utilities/profiles/profileActions.ts
| // Dialog filters don't apply to folders and are not enforced on all | ||
| // platforms, so validate the selection: directories are accepted as rule | ||
| // directories, files must be YAML. | ||
| const validUris: Uri[] = []; | ||
| const skippedNames: string[] = []; | ||
| for (const uri of fileUris) { | ||
| const stat = await workspace.fs.stat(uri); | ||
| const isDirectory = (stat.type & FileType.Directory) !== 0; | ||
| const isYamlFile = uri.fsPath.endsWith(".yaml") || uri.fsPath.endsWith(".yml"); | ||
| if (isDirectory || isYamlFile) { | ||
| validUris.push(uri); | ||
| } else { | ||
| skippedNames.push(basename(uri.fsPath)); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate file =="
git ls-files | rg '(^|/)profileActions\.ts$|profiles/profileActions\.ts$' || true
echo "== File outline/size =="
wc -l vscode/core/src/utilities/profiles/profileActions.ts
ast-grep outline vscode/core/src/utilities/profiles/profileActions.ts || true
echo "== Relevant file section =="
sed -n '1,120p' vscode/core/src/utilities/profiles/profileActions.ts
echo "== Search stat/usages of validUris/skippedNames =="
rg -n "workspace\.fs\.stat|validUris|skippedNames|getProfiles|save|yaml|yml" vscode/core/src/utilities/profiles/profileActions.tsRepository: konveyor/editor-extensions
Length of output: 4702
🌐 Web query:
Microsoft VSCode API workspace.fs.stat Error FileSystemError stat does not find file throws documentation
💡 Result:
In the Visual Studio Code extension API, the vscode.workspace.fs.stat method is designed to throw a vscode.FileSystemError when the target file or folder cannot be found [1][2]. When implementing or consuming this API, you should handle this scenario using a try-catch block [1][2]. Specifically, when a file is missing, the API throws an error that satisfies e instanceof vscode.FileSystemError and whose name or code corresponds to FileNotFound [1][2][3]. Key technical details: - Error Handling: To verify if a file does not exist, wrap the stat call in a try-catch and check for the FileNotFound error [1][2]. - Implementation Requirements: If you are implementing a custom FileSystemProvider, you are expected to throw vscode.FileSystemError.FileNotFound() when a requested path does not exist [4]. This allows VS Code to correctly propagate the state and identify the missing file [4]. - Identification: You can identify the specific error type by checking the error object's name or code property, which will be set to FileNotFound [3][5]. Note that some custom FileSystemProvider implementations (such as specific Git provider versions) may behave inconsistently if not properly configured to throw these errors; however, the standard expectation for the vscode.workspace.fs.stat API is that it must signal a missing entry by throwing the appropriate FileSystemError [6][4].
Citations:
- 1: https://github.com/microsoft/vscode/blob/ae7dce46/extensions/vscode-api-tests/src/singlefolder-tests/workspace.fs.test.ts
- 2: https://github.com/microsoft/vscode/blob/main/extensions/vscode-api-tests/src/singlefolder-tests/workspace.fs.test.ts
- 3: https://vshaxe.github.io/vscode-extern/vscode/FileSystemError.html
- 4: regression in insiders: VSCode calls stat() on FileSystemProvider before it creates a directory microsoft/vscode#71118
- 5: Add error code getter to FileSystemError #90517 microsoft/vscode#90586
- 6: Git - fileSystemProvider does not return an error when a file does not exist microsoft/vscode#204231
🌐 Web query:
VSCode FileSystemError.name FileNotFound stat throws error code name
💡 Result:
In the Visual Studio Code extension API, when implementing a FileSystemProvider—such as the stat method—you should throw a vscode.FileSystemError.FileNotFound error to indicate that a requested file or directory does not exist [1][2]. For your implementation, use the built-in factory method: throw vscode.FileSystemError.FileNotFound(uri); Key technical details regarding FileSystemError codes: 1. Identifying Errors: The FileSystemError class includes a code property that identifies the type of error [3][4]. This code corresponds to the class name of the factory method (e.g., 'FileNotFound') [3][5]. 2. Internal Mappings: Internally, VS Code maps these errors to specific provider error codes (like EntryNotFound) [6]. When you throw using the provided factory methods, the VS Code file service correctly handles and identifies these errors [1][7]. 3. Best Practice: Always use the static factory methods provided by the vscode.FileSystemError class (e.g., FileExists, FileNotADirectory, NoPermissions) rather than manually constructing an error object [1][5][8]. This ensures the error is correctly recognized by the host side of the VS Code API [1].
Citations:
- 1: regression in insiders: VSCode calls stat() on FileSystemProvider before it creates a directory microsoft/vscode#71118
- 2: https://github.com/microsoft/vscode-extension-samples/blob/aa42c2ec7248059ff5903ff11aa6eeabc04329bf/fsprovider-sample/src/fileSystemProvider.ts
- 3: Add error code getter to FileSystemError #90517 microsoft/vscode#90586
- 4: Add a default value to FileSystemError#code #90517 microsoft/vscode#92427
- 5: https://vshaxe.github.io/vscode-extern/vscode/FileSystemError.html
- 6: https://github.com/Microsoft/vscode/blob/master/src/vs/platform/files/common/files.ts
- 7: https://github.com/microsoft/vscode/blob/ae7dce46/src/vs/workbench/api/common/extHostFileSystemConsumer.ts
- 8: https://pub.dev/documentation/typings_fork/latest/typings.vscode/FileSystemError-class.html
Handle workspace.fs.stat failures per selection.
window.showOpenDialog can return stale or unsupported URIs, and workspace.fs.stat(uri) can throw before later selections are evaluated. Wrap each stat call in try/catch, track stat failures separately from non-YAML files, and continue evaluating the remaining selections.
🤖 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 `@vscode/core/src/utilities/profiles/profileActions.ts` around lines 21 - 34,
Update the selection loop in profileActions to wrap each workspace.fs.stat call
in try/catch, recording failures separately from skipped non-YAML filenames and
continuing with subsequent URIs. Preserve the existing directory/YAML validation
for successful stats and ensure both failure categories remain available for
later handling.
## Summary `Test (macos)` has been failing on every PR since VS Code 1.131.0 shipped, and `Verify changelog fragment` fails on every fork PR. Both are broken on main, not by any particular change. Confirmed by running `npx vscode-test` on a clean `ed42f5a` checkout. - 1.131.0 dropped `Contents/MacOS/Electron` for `Contents/MacOS/Code`, and `@vscode/test-electron` 2.x still spawns the old name. `.vscode-test.mjs` pins no version so it picks up stable at run time, which is why this started on its own. #1477 passed on 1.130.0. macOS failing then cancels linux and windows via fail-fast, so all three go red. - Bump `@vscode/test-electron` to 3.x. That gets it launching and exposes a second macOS failure underneath: VS Code opens a unix socket in its user data dir, and the default `.vscode-test/user-data` path is over the 104 byte `sun_path` limit. CI's macOS path is exactly 104, so the bump on its own would have swapped one failure for another. Point `--user-data-dir` at a short temp path. - `@vscode/test-cli` stops at 0.0.14 on purpose. 0.0.15 pulls `yargs@18`, which wants node `^22.12.0`, and `.nvmrc` is v22.9.0, so install dies with EBADENGINE before anything runs. Bumping `.nvmrc` would also work but that is a bigger call than this PR should make. - The changelog job checks out the PR head, which `actions/checkout` now refuses under `pull_request_target` since that runs fork code with a privileged token. It only needs the changed filenames, so ask the API for them instead. Avoids opting into `allow-unsafe-pr-checkout`. `test:integration` reports 0 passing before and after, because the webpack build never emits `out/test/`. That step exists to launch VS Code and exit clean, and it does again. The 150 tests CI reports come from `test:unit-tests`. ## Test plan - [x] `npx vscode-test` on main reproduces the macOS failure, and passes with these changes - [x] `Test (macos)`, `Test (linux)` and `Test (windows)` green on this PR - [ ] `Verify changelog fragment` green on the next bugfix or feature PR. It is skipped here since this is `:seedling:` Signed-off-by: Fabian von Feilitzsch <fabian@fabianism.us> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…) (#1481) ## Summary `Test (macos)` has been failing on every PR since VS Code 1.131.0 shipped, and `Verify changelog fragment` fails on every fork PR. Both are broken on main, not by any particular change. Confirmed by running `npx vscode-test` on a clean `ed42f5a` checkout. - 1.131.0 dropped `Contents/MacOS/Electron` for `Contents/MacOS/Code`, and `@vscode/test-electron` 2.x still spawns the old name. `.vscode-test.mjs` pins no version so it picks up stable at run time, which is why this started on its own. #1477 passed on 1.130.0. macOS failing then cancels linux and windows via fail-fast, so all three go red. - Bump `@vscode/test-electron` to 3.x. That gets it launching and exposes a second macOS failure underneath: VS Code opens a unix socket in its user data dir, and the default `.vscode-test/user-data` path is over the 104 byte `sun_path` limit. CI's macOS path is exactly 104, so the bump on its own would have swapped one failure for another. Point `--user-data-dir` at a short temp path. - `@vscode/test-cli` stops at 0.0.14 on purpose. 0.0.15 pulls `yargs@18`, which wants node `^22.12.0`, and `.nvmrc` is v22.9.0, so install dies with EBADENGINE before anything runs. Bumping `.nvmrc` would also work but that is a bigger call than this PR should make. - The changelog job checks out the PR head, which `actions/checkout` now refuses under `pull_request_target` since that runs fork code with a privileged token. It only needs the changed filenames, so ask the API for them instead. Avoids opting into `allow-unsafe-pr-checkout`. `test:integration` reports 0 passing before and after, because the webpack build never emits `out/test/`. That step exists to launch VS Code and exit clean, and it does again. The 150 tests CI reports come from `test:unit-tests`. ## Test plan - [x] `npx vscode-test` on main reproduces the macOS failure, and passes with these changes - [x] `Test (macos)`, `Test (linux)` and `Test (windows)` green on this PR - [ ] `Verify changelog fragment` green on the next bugfix or feature PR. It is skipped here since this is `:seedling:` Signed-off-by: Fabian von Feilitzsch <fabian@fabianism.us> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Cherry Picker <noreply@github.com> Signed-off-by: Fabian von Feilitzsch <fabian@fabianism.us> Signed-off-by: Cherry Picker <noreply@github.com> Co-authored-by: Fabian von Feilitzsch <fabian@fabianism.us> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Fixes #580
Summary
The "Select Custom Rules" dialog (Manage Profile → Configure Custom Rules) accepted any file type —
.java,.png, etc. were silently added to the profile as custom rules.Changes
.yaml/.yml), matching the webview upload path which already enforces YAML-onlyVerification
tsc --noEmitclean onvscode/coreeslintclean on the changed filenpm run changelog:validatepasses🤖 Generated with Claude Code
Summary by CodeRabbit