Fix: absolute glob-paths silently match no files in publish command - #83
Conversation
Refactored FindMatchingFiles to detect absolute path patterns via Path.IsPathRooted() and split them into a root directory plus a relative pattern (SplitAbsolutePattern). The Matcher now executes against the correct root directory instead of silently finding nothing. Also added test Publishing_Run_WithAbsoluteGlobPattern_ReadsMatchingFiles which verifies that absolute patterns work independently of the cwd. Agent-Logs-Url: https://github.com/demaconsulting/VersionMark/sessions/75c92412-4558-4f71-b78b-611acc0f2b57 Co-authored-by: Malcolmnixon <1863707+Malcolmnixon@users.noreply.github.com>
Agent-Logs-Url: https://github.com/demaconsulting/VersionMark/sessions/75c92412-4558-4f71-b78b-611acc0f2b57 Co-authored-by: Malcolmnixon <1863707+Malcolmnixon@users.noreply.github.com>
|
@copilot That pattern-matching is a lot to bolt into Program.cs - I think it warrants being another software-unit possibly under a Utilities subsystem. Please move the glob-matching logic there and remember to:
|
- Add Utilities subsystem with GlobMatcher unit (src/DemaConsulting.VersionMark/Utilities/GlobMatcher.cs) - Update Program.cs to delegate to GlobMatcher.FindMatchingFiles - Add 8 unit tests in test/.../Utilities/GlobMatcherTests.cs - Add requirements: docs/reqstream/version-mark/utilities.yaml and docs/reqstream/version-mark/utilities/glob-matcher.yaml - Add design docs: docs/design/version-mark/utilities.md and docs/design/version-mark/utilities/glob-matcher.md - Add verification docs: docs/verification/version-mark/utilities.md and docs/verification/version-mark/utilities/glob-matcher.md - Update requirements.yaml with new includes - Update docs/design/introduction.md software structure tree - Update docs/design/version-mark.md publish mode interaction - Update docs/verification/version-mark.md subsystems list - Add VersionMark-Utilities review sets to .reviewmark.yaml Agent-Logs-Url: https://github.com/demaconsulting/VersionMark/sessions/201cbd9d-683b-48b4-a394-9ce761661f2c Co-authored-by: Malcolmnixon <1863707+Malcolmnixon@users.noreply.github.com>
Done in commit d20c0f3. The glob-matching logic has been moved to a new Added alongside:
|
|
@copilot now that there's a Utilities subsystem, should PathHelpers be moved to it? |
- Move src/.../SelfTest/PathHelpers.cs → src/.../Utilities/PathHelpers.cs (namespace: DemaConsulting.VersionMark.Utilities) - Move test/.../SelfTest/PathHelpersTests.cs → test/.../Utilities/PathHelpersTests.cs - Update Validation.cs and 5 test files to use new namespace - Move docs/reqstream/version-mark/self-test/path-helpers.yaml → docs/reqstream/version-mark/utilities/path-helpers.yaml - Add VersionMark-Utilities-SafePath requirement to utilities.yaml - Update requirements.yaml include path - Update design/self-test.md, design/utilities.md, design/path-helpers.md - Update verification/self-test.md, verification/utilities.md, verification/path-helpers.md - Update introduction.md (tree, folder layout, scope) - Update .reviewmark.yaml (new VersionMark-Utilities-PathHelpers set) Agent-Logs-Url: https://github.com/demaconsulting/VersionMark/sessions/136dcc6b-0dbe-4bbd-a8bd-9ce44abfa395 Co-authored-by: Malcolmnixon <1863707+Malcolmnixon@users.noreply.github.com>
Yes — done in commit ede720a. |
There was a problem hiding this comment.
Pull request overview
This PR fixes publish glob resolution so absolute glob patterns (common in CI) correctly match files by introducing a Utilities subsystem and routing publish-time matching through it.
Changes:
- Added
Utilities.GlobMatcherto support matching both relative and absolute glob patterns (with dedup + stable sort). - Moved
PathHelpersfromSelfTestintoUtilitiesand updated references across source/tests/docs. - Added/updated tests and requirements/design/verification documentation to cover the new Utilities subsystem.
Reviewed changes
Copilot reviewed 26 out of 27 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| test/DemaConsulting.VersionMark.Tests/Utilities/PathHelpersTests.cs | Updates PathHelpers tests to the new Utilities namespace. |
| test/DemaConsulting.VersionMark.Tests/Utilities/GlobMatcherTests.cs | Adds unit tests for absolute/relative glob matching and pattern splitting. |
| test/DemaConsulting.VersionMark.Tests/SelfTest/SelfTestTests.cs | Updates imports to use Utilities.PathHelpers. |
| test/DemaConsulting.VersionMark.Tests/Publishing/PublishingTests.cs | Adds integration test ensuring publish works with an absolute glob pattern independent of cwd. |
| test/DemaConsulting.VersionMark.Tests/ProgramTests.cs | Updates import to use Utilities.PathHelpers. |
| test/DemaConsulting.VersionMark.Tests/IntegrationTests.cs | Updates import to use Utilities.PathHelpers. |
| test/DemaConsulting.VersionMark.Tests/Capture/CaptureTests.cs | Updates import to use Utilities.PathHelpers. |
| src/DemaConsulting.VersionMark/Utilities/PathHelpers.cs | Moves PathHelpers into Utilities namespace (no logic change). |
| src/DemaConsulting.VersionMark/Utilities/GlobMatcher.cs | Implements absolute+relative glob matching with SplitAbsolutePattern helper. |
| src/DemaConsulting.VersionMark/SelfTest/Validation.cs | Updates to use Utilities.PathHelpers. |
| src/DemaConsulting.VersionMark/Program.cs | Replaces inline matcher logic with GlobMatcher.FindMatchingFiles. |
| requirements.yaml | Adds Utilities requirements includes and removes old SelfTest PathHelpers include. |
| docs/verification/version-mark/utilities/path-helpers.md | Updates test location reference after moving tests. |
| docs/verification/version-mark/utilities/glob-matcher.md | Adds Utilities GlobMatcher verification chapter. |
| docs/verification/version-mark/utilities.md | Adds Utilities subsystem verification chapter. |
| docs/verification/version-mark/self-test.md | Updates SelfTest verification scope now that PathHelpers moved out. |
| docs/verification/version-mark.md | Updates subsystem list to include Utilities. |
| docs/reqstream/version-mark/utilities/path-helpers.yaml | Adds PathHelpers unit requirements under Utilities. |
| docs/reqstream/version-mark/utilities/glob-matcher.yaml | Adds GlobMatcher unit requirements. |
| docs/reqstream/version-mark/utilities.yaml | Adds Utilities subsystem requirements linking to GlobMatcher/PathHelpers. |
| docs/design/version-mark/utilities/path-helpers.md | Adds Utilities PathHelpers design chapter. |
| docs/design/version-mark/utilities/glob-matcher.md | Adds Utilities GlobMatcher design chapter. |
| docs/design/version-mark/utilities.md | Adds Utilities subsystem design chapter. |
| docs/design/version-mark/self-test.md | Updates SelfTest design scope now that PathHelpers moved out. |
| docs/design/version-mark.md | Updates publish-mode flow to include Utilities glob resolution step. |
| docs/design/introduction.md | Updates system overview to include Utilities subsystem. |
| .reviewmark.yaml | Replaces SelfTest PathHelpers review with Utilities subsystem/unit reviews. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@copilot apply changes based on the comments in this thread |
…s, doc fixes - Update docs/design/definition.yaml: remove stale self-test/path-helpers.md, add Utilities chapters (utilities.md, glob-matcher.md, path-helpers.md) - Update docs/verification/definition.yaml: same - Add GlobMatcher_SplitAbsolutePattern_UnixRootPattern test (non-Windows) - Add GlobMatcher_SplitAbsolutePattern_WindowsDriveRootPattern test (Windows-only) - Update docs/verification/version-mark.md Utilities bullet (GlobMatcher + PathHelpers) - Fix SelfTestTests.cs class summary (remove stale PathHelpers mention) - Update verification docs with new test entries Agent-Logs-Url: https://github.com/demaconsulting/VersionMark/sessions/0780fb23-d543-4557-8c6a-9f63992255a0 Co-authored-by: Malcolmnixon <1863707+Malcolmnixon@users.noreply.github.com>
Done in commit 1227063. Applied all review comments:
|
|
@copilot Is it appropriate to skip the unix pattern on windows? Is there still a test that uses the absolute (via slash or backslash but without drive letter) pattern on windows? |
… equivalent /*.json is rooted on both Unix and Windows (.NET returns "/" as the path root on both platforms), so no skip is needed. Rename the test method from UnixRootPattern to ForwardSlashRootPattern to make the intent clear. Update verification docs to match. Agent-Logs-Url: https://github.com/demaconsulting/VersionMark/sessions/53701e75-7c7d-4d89-a994-c535fecdc7fc Co-authored-by: Malcolmnixon <1863707+Malcolmnixon@users.noreply.github.com>
The skip was not appropriate — it was unnecessary. In .NET, Fixed in commit d894e84: removed the |
|
@copilot It fails on windows with: |
…t; use fs-aware comparer
On Windows, Path.GetPathRoot("/*.json") returns "\" (backslash), not "/".
Fix the ForwardSlashRootPattern test to assert the platform-specific expected
root so it passes on both Unix and Windows.
Also fix FindMatchingFiles to use a file-system-appropriate comparer for
the dedup HashSet and sort: OrdinalIgnoreCase on Windows (case-insensitive
FS), Ordinal on Linux/macOS (case-sensitive FS). This prevents incorrectly
deduplicating distinct files like a.json and A.json on Linux.
Update verification docs to document the platform-specific root behaviour.
Agent-Logs-Url: https://github.com/demaconsulting/VersionMark/sessions/c33a127e-f75e-4364-93c4-034a38823555
Co-authored-by: Malcolmnixon <1863707+Malcolmnixon@users.noreply.github.com>
... Fixed in commit 1b51ebc. Two changes:
|
Microsoft.Extensions.FileSystemGlobbing.Matcher.AddInclude()only accepts relative patterns — passing absolute paths (common in CI where env vars expand to absolute paths) caused silent zero-match results.Description
The fix introduces a new Utilities subsystem with two units —
GlobMatcherandPathHelpers— that encapsulate general-purpose file-system helpers.Program.csdelegates toGlobMatcher.FindMatchingFiles()for the publish command.PathHelpershas been moved from theSelfTestsubsystem intoUtilitiesas it is a general-purpose utility.GlobMatcher unit (
src/DemaConsulting.VersionMark/Utilities/GlobMatcher.cs)FindMatchingFiles— detects absolute patterns viaPath.IsPathRooted(); routes them through a per-pattern matcher rooted at the correct directory, while all relative patterns continue to be matched against cwd as before. Results are deduplicated via aHashSet(using a file-system-appropriate comparer) before the final sorted return.SplitAbsolutePattern(internal helper) — locates the last directory separator before the first wildcard (*,?,[) to split an absolute pattern into a root directory and a relative glob pattern; handles edge cases:Path.GetDirectoryName/Path.GetFileName/*.json→ root is the platform path root (/on Unix,\on Windows)C:\*.json→ root normalised toC:\(notC:)PathHelpers unit (
src/DemaConsulting.VersionMark/Utilities/PathHelpers.cs)Moved from
SelfTesttoUtilities. No logic changes — only the namespace (DemaConsulting.VersionMark.Utilities) and documentation updated to reflect its new general-purpose home.Tests
test/.../Utilities/GlobMatcherTests.cs— 10 unit tests covering: relative pattern matching, absolute path pattern matching, single-file absolute path (no wildcard), mixed absolute and relative patterns, empty pattern array, no-match patterns, forward-slash root pattern (/*.json, all platforms asserting the platform-specific root), and Windows drive-root pattern (C:\*.json, Windows only).test/.../Utilities/PathHelpersTests.cs— moved fromSelfTest/; all 10 existing tests unchanged.Publishing_Run_WithAbsoluteGlobPattern_ReadsMatchingFiles— sets cwd to a different directory to confirm matching is independent of cwd when an absolute pattern is supplied.Documentation and traceability
docs/reqstream/version-mark/utilities.yaml(+VersionMark-Utilities-SafePath) +docs/reqstream/version-mark/utilities/glob-matcher.yaml+docs/reqstream/version-mark/utilities/path-helpers.yamldocs/design/version-mark/utilities.md+docs/design/version-mark/utilities/glob-matcher.md+docs/design/version-mark/utilities/path-helpers.md;docs/design/definition.yamlupdated to include Utilities chapters and remove staleself-test/path-helpers.mdentrydocs/verification/version-mark/utilities.md+docs/verification/version-mark/utilities/glob-matcher.md+docs/verification/version-mark/utilities/path-helpers.md;docs/verification/definition.yamlupdated similarlyrequirements.yaml,docs/design/introduction.md,docs/design/version-mark.md,docs/design/version-mark/self-test.md,docs/verification/version-mark.md,docs/verification/version-mark/self-test.md, and.reviewmark.yamlType of Change
Related Issues
Pre-Submission Checklist
Before submitting this pull request, ensure you have completed the following:
Build and Test
pwsh ./build.ps1Code Quality
Quality Checks
Please run the following checks before submitting:
pwsh ./lint.ps1Testing
Documentation
Additional Notes
Relative patterns are unchanged in behaviour. Mixed invocations (some absolute, some relative) are handled correctly. The
GlobMatcherandPathHelpersunits are self-contained within theUtilitiessubsystem and have no dependency onProgramorContext, making them independently testable.The
/*.jsonforward-slash root pattern test (GlobMatcher_SplitAbsolutePattern_ForwardSlashRootPattern_SplitsToRootAndRelative) runs on all platforms without a skip guard. On Unix,Path.GetPathRoot("/*.json")returns"/"and on Windows it returns"\"(rooted to the current drive's root); the test asserts the platform-specific expected value, covering the empty-rootDirfallback branch on both. The only platform-conditional test is the Windows drive-letter case (C:\*.json), which is genuinely not applicable on Unix.FindMatchingFilesselects the deduplicationHashSetcomparer based on file-system case-sensitivity:OrdinalIgnoreCaseon Windows (case-insensitive file system) andOrdinalon Linux/macOS (case-sensitive file system), ensuring distinct files likea.jsonandA.jsonare never incorrectly merged on case-sensitive platforms.