[DiskAccessPaths] Fix illegal characters in executionName #235
Conversation
WalkthroughSanitizes ExecutionName generation in DiskAccessPaths to replace illegal characters with underscores and add a DiskAccessPath_ prefix. Updates CHANGELOG to document the DiskAccessPaths ExecutionName sanitization. No changes to exported/public declarations. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Assessment against linked issues
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
-
CHANGELOG.md(1 hunks) -
source/DSCResources/DiskAccessPaths/DiskAccessPaths.schema.psm1(1 hunks)
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.md
[grammar] ~33-~33: There might be a mistake here.
Context: ... Fixed - Formatting. - DiskAccessPaths: - Replace illegal characters in executionN...
(QB_NEW_EN)
[grammar] ~34-~34: Make sure to use plural and singular nouns correctly
Context: ...llegal characters in executionName with underscore ## [0.10.0] - 2025-07-26 ### Added - Adds...
(QB_NEW_EN_OTHER_ERROR_IDS_10)
🔇 Additional comments (2)
source/DSCResources/DiskAccessPaths/DiskAccessPaths.schema.psm1 (2)
13-13: Good fix: sanitize ExecutionName and add a stable prefixPrefixing with
DiskAccessPath_and sanitizing problematic characters via-replaceaddresses the illegal-character issue and makes the instance name stable.
13-15: ExecutionName sanitization matches FilesAndFolders — verifiedBoth resources use the same sanitization regex (-replace '[\s(){}/\:-]', '_'); only the resource-specific prefix differs.
- source/DSCResources/DiskAccessPaths/DiskAccessPaths.schema.psm1:13 —
$executionName = "DiskAccessPath_$ ($item.AccessPath)" -replace '[\s(){}/\:-]', '_'- source/DSCResources/FilesAndFolders/FilesAndFolders.schema.psm1:60 —
$executionName = "file_$ ($item.DestinationPath)" -replace '[\s(){}/\:-]', '_'
| - DiskAccessPaths: | ||
| - Replace illegal characters in executionName with underscore |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Clarify and fix grammar; mention the added DiskAccessPath_ prefix
The changelog should reflect the added prefix and use precise wording. Also prefer code formatting for identifiers for consistency with the rest of the file.
Apply this diff:
- - DiskAccessPaths:
- - Replace illegal characters in executionName with underscore
+ - DiskAccessPaths:
+ - Prefix `ExecutionName` with `DiskAccessPath_` and replace illegal characters with `_`📝 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.
| - DiskAccessPaths: | |
| - Replace illegal characters in executionName with underscore | |
| - DiskAccessPaths: | |
| - Prefix `ExecutionName` with `DiskAccessPath_` and replace illegal characters with `_` |
🧰 Tools
🪛 LanguageTool
[grammar] ~33-~33: There might be a mistake here.
Context: ... Fixed - Formatting. - DiskAccessPaths: - Replace illegal characters in executionN...
(QB_NEW_EN)
[grammar] ~34-~34: Make sure to use plural and singular nouns correctly
Context: ...llegal characters in executionName with underscore ## [0.10.0] - 2025-07-26 ### Added - Adds...
(QB_NEW_EN_OTHER_ERROR_IDS_10)
🤖 Prompt for AI Agents
In CHANGELOG.md around lines 33-34, update the entry to clearly state the added
prefix and use code formatting: change the line to say that a `DiskAccessPath_`
prefix is added and that illegal characters in `executionName` are replaced with
underscores (use backticks for `DiskAccessPath_` and `executionName`), and fix
grammar so it reads as a complete action (e.g., "Add `DiskAccessPath_` prefix
and replace illegal characters in `executionName` with underscores").
| $executionName = "DiskAccessPath_$($item.AccessPath)" -replace '[\s(){}/\\:-]', '_' | ||
| (Get-DscSplattedResource -ResourceName DiskAccessPath -ExecutionName $executionName -Properties $item -NoInvoke).Invoke($item) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add basic tests to lock the behavior
Given this fixes #234 and alters naming, a couple of Pester tests that assert the produced ExecutionName for representative inputs (e.g., E:\Logs, E:\\Logs, paths with spaces, dots, parentheses) would prevent regressions.
I can draft a Pester test that validates the computed ExecutionName values without invoking the resource. Want me to open an issue and provide the test scaffold?
| foreach ($item in $Items) | ||
| { | ||
| $executionName = $item.AccessPath | ||
| $executionName = "DiskAccessPath_$($item.AccessPath)" -replace '[\s(){}/\\:-]', '_' |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Make sanitization more robust to future inputs (e.g., dots, brackets)
The current character class does not cover some characters that can still break DSC instance labels (e.g., '.', '[]'). Consider normalizing by replacing any non-word character, which leaves only [A-Za-z0-9_]. Optionally, collapse multiple underscores.
Apply this diff:
- $executionName = "DiskAccessPath_$($item.AccessPath)" -replace '[\s(){}/\\:-]', '_'
+ $executionName = ("DiskAccessPath_$($item.AccessPath)" -replace '[^\w]', '_') -replace '_{2,}', '_'📝 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.
| $executionName = "DiskAccessPath_$($item.AccessPath)" -replace '[\s(){}/\\:-]', '_' | |
| $executionName = ("DiskAccessPath_$($item.AccessPath)" -replace '[^\w]', '_') -replace '_{2,}', '_' |
🤖 Prompt for AI Agents
In source/DSCResources/DiskAccessPaths/DiskAccessPaths.schema.psm1 around line
13, the current sanitization for $executionName only replaces a limited set of
characters and misses dots and brackets; update the replacement to normalize by
replacing any non-word character (i.e., anything not A-Za-z0-9_) with an
underscore, then collapse consecutive underscores into a single underscore and
trim leading/trailing underscores so the resulting name contains only letters,
digits and single underscores.
2 participants
Pull Request
Pull Request (PR) description
The executionName of this ressource had illegal characters because of paths like \ or .
With this change these illegal characters will be replaced with an underscore "_"
Fixed
Task list
build.ps1 -ResolveDependency).and comment-based help.
This change is