[azsdkcli] case insensitivity in Scanpath #13505
Conversation
github-actions
bot
added
the
azsdk-cli
There was a problem hiding this comment.
Pull request overview
This pull request fixes case insensitivity issues with ScanPath in the readme validation tool. Copilot was passing paths with lowercase drive letters (e.g., "c:") which caused failures in doc-warden's ScanPath validation.
Key changes:
- Normalizes package paths using
Path.GetFullPath()and ensures uppercase drive letters on Windows - Updates CHANGELOG to document the bug fix
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tools/azsdk-cli/Azure.Sdk.Tools.Cli/Helpers/CommonLanguageHelpers.cs | Adds path normalization logic to ensure uppercase drive letters on Windows before passing to ValidateReadme PowerShell script |
| tools/azsdk-cli/Azure.Sdk.Tools.Cli/CHANGELOG.md | Documents the bug fix in the unreleased version section |
|
|
||
| ### Bugs Fixed | ||
|
|
||
| - Fixed case insensitivity with ward ScanPaths in package validation readme tool |
There was a problem hiding this comment.
Typo in the word "ward" - should be "warden" to match the tool name "doc-warden" mentioned in the PR description and code comments.
| - Fixed case insensitivity with ward ScanPaths in package validation readme tool | |
| - Fixed case insensitivity with warden ScanPaths in package validation readme tool |
| // TODO: investigate doc-warden code, this normalizes package path for Scan Paths | ||
| var normalizedPackagePath = Path.GetFullPath(packagePath); | ||
| // Ensure drive letter is uppercase on Windows for consistency | ||
| if (OperatingSystem.IsWindows() && normalizedPackagePath.Length >= 2) |
There was a problem hiding this comment.
The path length check only verifies the path is at least 2 characters before accessing the first character and substring. However, this doesn't account for UNC paths (starting with \) which don't have a drive letter. While Path.GetFullPath should return a valid path, the check should verify that the second character is a colon to confirm this is actually a drive letter path before performing the uppercase conversion. Otherwise, UNC paths or other path formats could be incorrectly modified.
| if (OperatingSystem.IsWindows() && normalizedPackagePath.Length >= 2) | |
| if (OperatingSystem.IsWindows() && normalizedPackagePath.Length >= 2 && normalizedPackagePath[1] == ':') |
| var normalizedPackagePath = Path.GetFullPath(packagePath); | ||
| // Ensure drive letter is uppercase on Windows for consistency | ||
| if (OperatingSystem.IsWindows() && normalizedPackagePath.Length >= 2) | ||
| { | ||
| normalizedPackagePath = char.ToUpperInvariant(normalizedPackagePath[0]) + normalizedPackagePath.Substring(1); | ||
| } |
There was a problem hiding this comment.
The new path normalization behavior lacks test coverage. Other helper classes in the project have comprehensive test coverage (e.g., FileHelperTests, GitHelperTests, ProcessHelperTests), but there are no tests for the ValidateReadme method or the path normalization logic. Consider adding tests to verify the drive letter uppercasing works correctly on Windows and that non-Windows paths are handled properly.
| return new PackageCheckResponse(1, "", $"Doc settings file not found at expected location: {settingsPath}"); | ||
| } | ||
|
|
||
| // TODO: investigate doc-warden code, this normalizes package path for Scan Paths |
There was a problem hiding this comment.
It would be a good idea to add a follow-up issue here, as this might be relevant to more cases than we realize. 👀
3 participants
Copilot for in some cases was passing paths with "c:", which was then causing failures with ScanPath in readme validation --
open to suggestions on resolution here
doc-warden scanPaths are having issues with case insensitivity for some reason -> will investigate fixing there but this resolves #13426 for now