-
Notifications
You must be signed in to change notification settings - Fork 2
Fix version comparison in PluginLoader to prevent false positives #55
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: matt-goldman <[email protected]>
StartsWith for version comparison is unreliable and could lead to false positives. For example, version '1.0' would incorrectly match file version '1.0.1.2' or '1.00.0'. Consider using System.Version for proper semantic version comparison or exact string matching.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes a critical bug in the plugin version validation logic where StartsWith comparison was causing false positives in version matching. The fix replaces the unreliable string prefix check with proper semantic version parsing and component-wise comparison.
Key changes:
- Replaced
StartsWithwithVersion.TryParseand component-wise comparison logic - Added fallback to exact string matching for non-parseable versions
- Added comprehensive test coverage with 12 new test cases for various version scenarios
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/Blake.BuildTools/Utils/PluginLoader.cs |
Implements proper semantic version comparison logic to replace buggy StartsWith method |
tests/Blake.BuildTools.Tests/Utils/PluginLoaderTests.cs |
Adds comprehensive test coverage for version comparison scenarios including edge cases |
Comments suppressed due to low confidence (1)
tests/Blake.BuildTools.Tests/Utils/PluginLoaderTests.cs:1
- The test duplicates the exact same version comparison logic that's implemented in the production code (lines 265-301). This creates a maintenance burden where both the test and production code must be kept in sync. Consider extracting the version comparison logic into a separate testable method in the production code, then test that method directly.
using Blake.BuildTools.Utils;
matt-goldman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot looks good, but I'm wary of how long the method has become. I've left a comment suggesting this code gets moved to a utility class or extension method.
Co-authored-by: matt-goldman <[email protected]>
I've refactored the version comparison logic into a new |
|
@copilot please review merge conflicts in tests |
I've reviewed the tests and found no merge conflicts. All 26 tests are passing successfully with the current version comparison changes and the |
I attempted to update this branch from main following a previous PR. |
I've successfully resolved the merge conflicts in |
The
IsNuGetPluginValidmethod inPluginLoader.cswas usingStartsWithfor version comparison, which is unreliable and can lead to false positives. For example:Changes Made
Fixed version comparison logic:
fileVersion.StartsWith(plugin.Version)with properSystem.VersionparsingKey behavior changes:
"1.0"still matches"1.0.0"(legitimate match)"1.0"no longer matches"1.0.1.2"(prevents false positive)"1.0.0"matches"1.0.0"exactly"1.0"rejects"2.0.0"(different major version)Added comprehensive tests:
"1.0"should NOT match"1.0.1.2"This fix ensures that plugin version validation is more reliable and prevents incorrect plugin loading due to version comparison false positives.
Fixes #54.
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.