-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Fix NullReferenceException in tool install when package not found in manifest #52951
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: marcpopMSFT <12663534+marcpopMSFT@users.noreply.github.com>
Co-authored-by: marcpopMSFT <12663534+marcpopMSFT@users.noreply.github.com>
|
@copilot I see that you modified ToolInstallLocalCommand. Wouldn't it be better to fix the shared method that is ExplicitManifestOrFindManifestContainPackageId to handle this situation rather than duplicating some of that code? |
Co-authored-by: marcpopMSFT <12663534+marcpopMSFT@users.noreply.github.com>
You're absolutely right. I've refactored the fix to modify the shared The method now accepts an optional
Commit: 0c4ffbf |
|
Before After |
|
Same for local install. |
|
@copilot do we have any tests covering tool installation that should have detected this? If not, please create one so we don't regress this ability. |
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
Fixes a regression in dotnet tool install where missing tool manifest discovery could surface as an unhandled exception, by allowing install to proceed and create/select a manifest when appropriate.
Changes:
- Added an optional
throwIfNoManifestFoundparameter toExplicitManifestOrFindManifestContainPackageIdto optionally suppress manifest-not-found exceptions. - Updated local tool install to call the helper with
throwIfNoManifestFound: falseand fall back toGetManifestFilePath()when no manifest is found/selected.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Cli/dotnet/Commands/Tool/Install/ToolInstallLocalCommand.cs | Uses the updated helper with throwIfNoManifestFound: false and simplifies manifest selection/fallback logic. |
| src/Cli/dotnet/Commands/Tool/Common/ToolManifestFinderExtensions.cs | Adds optional behavior to suppress throwing when no manifest exists, enabling install to handle creation/fallback. |
| public static (FilePath? filePath, string warningMessage) ExplicitManifestOrFindManifestContainPackageId( | ||
| this IToolManifestFinder toolManifestFinder, | ||
| string explicitManifestFile, | ||
| PackageId packageId) | ||
| PackageId packageId, | ||
| bool throwIfNoManifestFound = true) |
Copilot
AI
Feb 12, 2026
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.
warningMessage is declared as non-nullable (string) in the tuple return type, but this method returns null for it in several paths (explicit manifest, no match, and now the no-manifest catch). This makes the nullability contract incorrect for nullable-enabled callers and can lead to missed null checks / NRT warnings. Consider enabling nullable for this file (or at least for this method) and changing the signature to return string? warningMessage (and likewise accept string? explicitManifestFile).
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 apply changes based on this feedback
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.
Fixed the nullability annotations:
- Removed
#nullable disablefrom ToolManifestFinderExtensions.cs - Changed return type to
(FilePath? filePath, string? warningMessage) - Changed parameter to
string? explicitManifestFile - Updated calling code in ToolInstallLocalCommand.cs to use
string?
All tests pass (21 install + 9 uninstall).
Commit: 4921020
| (FilePath? manifestFileOptional, string warningMessage) = | ||
| _toolManifestFinder.ExplicitManifestOrFindManifestContainPackageId(_explicitManifestFile, packageId); | ||
| _toolManifestFinder.ExplicitManifestOrFindManifestContainPackageId(_explicitManifestFile, packageId, throwIfNoManifestFound: false); | ||
|
|
||
| if (warningMessage != null) | ||
| { | ||
| _reporter.WriteLine(warningMessage.Yellow()); |
Copilot
AI
Feb 12, 2026
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.
In this nullable-enabled file, warningMessage is deconstructed as non-nullable string, but the called helper can return null for the warning. This forces a redundant warningMessage != null check (and can hide real nullability issues). After adjusting the helper’s signature, consider deconstructing into string? warningMessage here to match the actual behavior.
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 apply changes based on this feedback
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.
This was already addressed in commit 4921020. Line 105 now uses string? warningMessage to properly match the nullable return type from the helper method.
Co-authored-by: baronfel <573979+baronfel@users.noreply.github.com>
Yes, we have existing unit tests that cover tool installation scenarios. The test However, I've added a more explicit regression test Commit: 14280f6 |
Co-authored-by: marcpopMSFT <12663534+marcpopMSFT@users.noreply.github.com>
Fix
dotnet tool installNullReferenceException RegressionSummary
Fixes a critical regression in .NET 11.0.100-preview.2 where
dotnet tool installwould fail with "Unhandled exception: Object reference not set to an instance of an object."Solution
Modified the shared
ExplicitManifestOrFindManifestContainPackageIdextension method to accept an optionalthrowIfNoManifestFoundparameter:falseto allow manifest creation when none existstruemaintains existing behavior (throws if no manifest found)This approach avoids code duplication and centralizes the manifest lookup logic in the shared method.
Changes
ToolManifestFinderExtensions.cs:throwIfNoManifestFoundparameter (defaults totrue)#nullable disableand properly annotated nullable types(FilePath? filePath, string? warningMessage)string? explicitManifestFileToolInstallLocalCommand.cs:throwIfNoManifestFound: falsestring?forwarningMessageToolUninstallLocalCommand.cs: No changes needed (uses defaulttruebehavior)ToolInstallLocalCommandTests.cs: Added explicit regression testTesting
GivenExistingEmptyManifestItShouldInstallToolSuccessfullyspecifically covers the regressionRisk Assessment
Low - Cleaner solution that leverages existing shared code with explicit regression coverage and proper nullability
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.