feat: add --exclude-publish-false option#443
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new CLI flag/property ExcludePublishFalse and threads it from Program through LicenseValidationOrchestrator into ReferencedPackageReader. ReferencedPackageReader gains an excludePublishFalse parameter and implements per-target logic to detect and prune packages with Publish="false" using MSBuild PackageReference metadata and assets lock data, including transitive-only exclusion via a reachability graph. Adds INuGetFrameworkUtility/NuGetFrameworkUtility, AssetsPackageDependencyReader/IAssetsPackageDependencyReader, PackageReferenceMetadata and new project wrapper APIs, and updates tests to exercise per-target publish semantics. Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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. Comment Tip CodeRabbit can generate a title for your PR based on the changes.Add |
|
@GK-302 thank you very much for your contribution. I will take a look at the changes as I can. Since I'm not familiar with the Publish property inside PackageReference, would you mind pointing me to the documentation? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/NuGetUtility/Wrapper/MsBuildWrapper/ProjectWrapper.cs`:
- Around line 53-65: GetPackageReferencesForTarget creates a new Project
instance that gets added to the shared _project.ProjectCollection and is never
unloaded; to fix this, materialize the package-reference results and then unload
(or dispose) the created Project/ProjectCollection before returning: evaluate
targetProject.GetItems(PackageReferenceItemType).Select(... ) into a concrete
list (e.g., ToList()) and then call
targetProject.ProjectCollection.UnloadProject(targetProject) or create a new
ProjectCollection, load the Project into it, read and materialize the
PackageReferenceMetadata using CreateMetadata, and finally dispose the
ProjectCollection to avoid leaking entries and InvalidOperationException when
reloading the same path with different globals.
🧹 Nitpick comments (2)
tests/NuGetUtility.Test/ReferencedPackagesReader/ReferencedPackageReaderTest.cs (1)
307-339: Consider adding complementary test cases for stronger coverage.The test correctly verifies that packages with
Publish=falseare excluded whenexcludePublishFalse=true. Two additional cases would strengthen coverage:
- Verify that with
excludePublishFalse=false, the package withPublish=falsemetadata is not excluded.- Verify the
targetFramework-specific path (whereGetPackageReferencesForTargetis called instead ofGetPackageReferences).src/NuGetUtility/ReferencedPackagesReader/ReferencedPackageReader.cs (1)
144-162: Redundant null-coalescing onpackageReferences.On line 152,
packageReferencesis the return value ofGetPackageReferences()orGetPackageReferencesForTarget(), both of which returnIEnumerable<PackageReferenceMetadata>(non-nullable per the interface). The?? Array.Empty<PackageReferenceMetadata>()is unnecessary. Not a bug, just a nit.Proposed simplification
- foreach (PackageReferenceMetadata packageReference in packageReferences ?? Array.Empty<PackageReferenceMetadata>()) + foreach (PackageReferenceMetadata packageReference in packageReferences)
|
@GK-302 something else I noticed: nuget-license allows to analyze transitive dependencies. Transitive dependencies are dependencies of dependencies. Obviously those will not be put in the project file. How does the exclusion logic you added play with this feature? Support for transitive dependencies is the main reason nuget-license makes use of the lock file generated when building the project. I'm wondering whether the exclude you are looking for would be acheived much easier (and more correctly) when requiring a publish operation before analyzing the packages. If above is not possible I would like to ask you to research whether the exclusion logic can be calulated from the lock file in order to properly support transitive dependencies. I assume that packages marked to not publish will carry this same property to it's dependencies.... |
|
@sensslen You are absolutely right—my current implementation only handles direct dependencies and fails to exclude transitive ones. I agree that basing the exclusion logic on the result of a publish operation is a much more robust approach. It would allow nuget-license to accurately filter the dependency list (from the lock file) by checking if the corresponding files actually exist in the output directory. If you are okay with this direction, I'll go ahead with this plan: using the lock file to get the full dependency tree and then filtering it based on the actual contents of the publish folder. Does this align with what you had in mind? |
If I'm not mistaken the lock file should only contain whatever got published. If that is not the case: is there a way to extract the publish information from the lock file? I would avoid reading the output folder as that seems like another source of truth... You may also have a look how the dotnet commandline tool does this filtering. That's what I did for the original implementation |
|
Thank you for the feedback. I have investigated this further and would like to refine the implementation strategy as follows: About the Publish property: This is an MSBuild Item Metadata processed by the .NET SDK. While it is not extensively covered in the primary NuGet documentation, it is a recognized method within the community for recursively excluding PackageReference assets from the publish output. Analysis via Lock Files: Instead of simply parsing the project file, I will change the logic to trace the dependency tree using lock files (such as packages.lock.json or project.assets.json). This will allow for a more accurate identification of transitive dependencies. Exclusion Logic : I will implement logic that considers multiple dependency paths. A package will be excluded from the analysis only if every path leading to it originates from a reference marked with Publish="false". If a package is reachable via any reference where Publish is true (the default), it will be retained in the analysis to ensure no required licenses are missed. Alignment with Official Behavior: I aim to make this implementation consistent with how the dotnet publish command internally filters assets. If you have any additional technical insights or preferred documentation regarding these internal SDK flows, it would be greatly appreciated. Does this direction look good to you? |
Sounds good with me but also fairly complicated. Unfortunately I have not found any good documentation with regards to the dotnet command other than it's source code. |
|
Sorry for the delay! I've updated the PR with the revised implementation. Key changes:
Looking forward to your feedback! |
|
@GK-302 Thank you for your effort. I will require some time to go through the changes to validate them. For now, can you respond to the issues raised by coderabbit? |
|
Thanks for the review! I've seen the feedback and CodeRabbit's comments. I'll address them as soon as I can. |
|
I've addressed CodeRabbitAI's feedback. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/NuGetUtility/ReferencedPackagesReader/ReferencedPackageReader.cs (2)
199-254: Dual file parsing introduces subtle coupling.The assets file is read twice: once via
ILockFileFactory(for package enumeration) and again directly viaSystem.Text.Json(for dependency graph). This works but has trade-offs:
- Duplicates I/O for the same file
- Couples the code to the raw JSON structure of
project.assets.json- Potential inconsistency if the file changes between reads (unlikely but possible)
This is a pragmatic solution given
ILockFiledoesn't expose the dependency graph conveniently. Consider documenting this design decision or extracting the JSON parsing into a dedicated abstraction if the dependency graph logic grows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NuGetUtility/ReferencedPackagesReader/ReferencedPackageReader.cs` around lines 199 - 254, BuildDependencyGraphFromAssetsFile reads project.assets.json directly causing duplicate I/O and coupling to JSON; either document this trade-off near the method or extract the JSON parsing into a small abstraction (e.g., an IAssetsDependencyGraphParser with a BuildDependencyGraphFromAssetsFile-like method) and have ReferencedPackageReader depend on that instead of calling System.Text.Json directly; update usages to accept the new abstraction (or add an explanatory comment on why ILockFile/ILockFileFactory can't be used) and move the JSON-specific parsing logic into the new class to isolate structure coupling and make future changes/testability simpler.
164-175: Silent exception handling may mask issues.
IOExceptionandJsonExceptionare silently caught, returning only direct exclusions. While this provides graceful degradation, users won't know that transitive exclusion failed.Consider logging a warning when these exceptions occur so users are aware that the full exclusion analysis couldn't be performed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NuGetUtility/ReferencedPackagesReader/ReferencedPackageReader.cs` around lines 164 - 175, The catch blocks around the call to BuildDependencyGraphFromAssetsFile are currently swallowing IOException and JsonException and returning excludedPackages silently; update those catch handlers to log a warning (including the exception message/stack and context like resolvedAssetsPath and targetFrameworks) before returning so callers know transitive exclusion analysis failed—use the existing logging facility (e.g. a class ILogger/_logger or System.Diagnostics.Trace if none exists) and keep behavior of returning excludedPackages after logging; modify the catch for both IOException and JsonException in ReferencedPackageReader.cs where dependencyGraph is built.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/NuGetUtility/ReferencedPackagesReader/ReferencedPackageReader.cs`:
- Around line 199-254: BuildDependencyGraphFromAssetsFile reads
project.assets.json directly causing duplicate I/O and coupling to JSON; either
document this trade-off near the method or extract the JSON parsing into a small
abstraction (e.g., an IAssetsDependencyGraphParser with a
BuildDependencyGraphFromAssetsFile-like method) and have ReferencedPackageReader
depend on that instead of calling System.Text.Json directly; update usages to
accept the new abstraction (or add an explanatory comment on why
ILockFile/ILockFileFactory can't be used) and move the JSON-specific parsing
logic into the new class to isolate structure coupling and make future
changes/testability simpler.
- Around line 164-175: The catch blocks around the call to
BuildDependencyGraphFromAssetsFile are currently swallowing IOException and
JsonException and returning excludedPackages silently; update those catch
handlers to log a warning (including the exception message/stack and context
like resolvedAssetsPath and targetFrameworks) before returning so callers know
transitive exclusion analysis failed—use the existing logging facility (e.g. a
class ILogger/_logger or System.Diagnostics.Trace if none exists) and keep
behavior of returning excludedPackages after logging; modify the catch for both
IOException and JsonException in ReferencedPackageReader.cs where
dependencyGraph is built.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/NuGetUtility/ReferencedPackagesReader/ReferencedPackageReader.cssrc/NuGetUtility/Wrapper/MsBuildWrapper/ProjectWrapper.cstests/NuGetUtility.Test/ReferencedPackagesReader/ReferencedPackageReaderTest.cs
…ssets parsing trade-off
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/NuGetUtility/ReferencedPackagesReader/ReferencedPackageReader.cs (1)
31-31: Consider adding XML documentation for the new parameter.The public
GetInstalledPackagesmethod now has a newexcludePublishFalseparameter. As per coding guidelines, public APIs should have XML documentation comments. Consider documenting the parameter's behavior (excludes packages marked withPublish="false"metadata and their transitive dependencies when no other path exists).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NuGetUtility/ReferencedPackagesReader/ReferencedPackageReader.cs` at line 31, Add XML documentation to the public method GetInstalledPackages to document the new excludePublishFalse parameter: add a <summary> describing what the method does, and a <param name="excludePublishFalse"> that explains it excludes packages that have metadata Publish="false" and their transitive dependencies when no other package path to them exists; also ensure other parameters (projectPath, includeTransitive, targetFramework) have <param> tags and update any remarks if needed to reflect this behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/NuGetUtility/ReferencedPackagesReader/ReferencedPackageReader.cs`:
- Line 31: Add XML documentation to the public method GetInstalledPackages to
document the new excludePublishFalse parameter: add a <summary> describing what
the method does, and a <param name="excludePublishFalse"> that explains it
excludes packages that have metadata Publish="false" and their transitive
dependencies when no other package path to them exists; also ensure other
parameters (projectPath, includeTransitive, targetFramework) have <param> tags
and update any remarks if needed to reflect this behavior.
…clusion diagnostics
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/NuGetUtility/ReferencedPackagesReader/ReferencedPackageReader.cs`:
- Around line 94-113: When targetFramework is null the code calls
GetPackagesExcludedFromPublish(project, targetFramework) once and applies that
global exclusion to all selected targets; instead, when targetFramework == null
aggregate exclusions per target framework (or evaluate per-target) by calling
GetPackagesExcludedFromPublish for each selected target framework and unioning
the results before applying them to referencedLibraries, and when
includeTransitive call GetPackagesExcludedFromPublishDependencyPaths with the
per-target excluded sets or the aggregated set and matching target framework
identifiers so target-conditional Publish=false is only applied to the
frameworks that actually specify it; update the block around
GetPackagesExcludedFromPublish, the creation of excludedPackages, and the
subsequent call to referencedLibraries.RemoveWhere accordingly.
- Around line 78-79: The assets file target matching fails for RID-qualified
keys; update the logic around selectedTargets and
BuildDependencyGraphFromAssetsFile to also consider RID-qualified target keys:
detect the project's RuntimeIdentifier (or active RID) and construct candidate
keys like $"{targetFramework.ToString()}/{runtimeIdentifier}" in addition to the
TFM-only string, then match assetsFile.Targets against both variants
(case-insensitive). Specifically, modify the code paths that build the target
key(s) — referenced by selectedTargets, the call site passing
TargetFramework.ToString() into BuildDependencyGraphFromAssetsFile, and the
BuildDependencyGraphFromAssetsFile implementation — to try TFM+RID before
falling back to TFM-only so RID-qualified keys such as "net8.0/win-x64" are
found.
…ng transitive dependencies
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@sensslen Ready for your review! Thanks. |
|
@GK-302 thank you again for working on this. Don't worry too much about your ability to follow any "required" flow. You're responding to comments in reasonable time which is the most important asset. While reviewing the code I will comment on things I notice. Don't feel like you need to change things immediately. These are mostly ment as conversation starters to that we can make sure the final state of this is good. |
|
Update: PR synced and verified. I have merged the latest main into this branch and confirmed that my changes work correctly with the new implementation. Manual Test: Confirmed that --exclude-publish-false correctly filters packages in the latest version of the tool. CI Status: The current test failures are unrelated to this PR, as they also seem to occur on the main branch due to the project's infrastructure configuration. Everything is ready for review. Thank you! |
|
Update: I've updated the PR description |
|
Thank you for the follow-up! I'll check the content later. |
Some improvements to your PR
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
tests/NuGetUtility.Test/Extensions/StringExtensionsTest.cs (1)
44-64: LGTM – explicit test names improve diagnostics.The added
TestNameattributes provide clearer test output and make failures easier to identify. The naming convention (PathLike_Windows_{Context}_{Description}) is consistent and descriptive.Minor consistency note: Linux/Unix path test cases (lines 51-57) don't have explicit
TestNameattributes while their Windows counterparts do. Consider adding similar names for parity if you want uniform test reporting across platforms.,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/NuGetUtility.Test/Extensions/StringExtensionsTest.cs` around lines 44 - 64, The Linux/Unix path test cases (e.g. TestCase entries with source "/home/user/projects/MyProject/MyProject.csproj" and patterns like "*.csproj", "MyProject.csproj", "/home/user/*/MyProject/*.csproj", etc.) lack explicit TestName attributes; add TestName to each of those TestCase attributes to match the Windows naming convention (use a consistent convention such as "PathLike_Unix_{Context}_{Description}" analogous to the existing "PathLike_Windows_..." names) so failures report with parity; update the TestCase declarations in StringExtensionsTest to include those TestName strings.src/NuGetUtility/Wrapper/NuGetWrapper/ProjectModel/ILockFileTargetLibrary.cs (1)
9-12: Consider adding XML documentation for public interface.Public interfaces should have XML documentation comments per coding guidelines. This helps consumers understand the purpose and contract.
📝 Suggested documentation
+ /// <summary> + /// Represents a library entry within a specific target framework in the lock file. + /// </summary> public interface ILockFileTargetLibrary : ILockFileLibrary { + /// <summary> + /// Gets the collection of package dependencies for this library. + /// </summary> IReadOnlyList<IPackageDependency> Dependencies { get; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NuGetUtility/Wrapper/NuGetWrapper/ProjectModel/ILockFileTargetLibrary.cs` around lines 9 - 12, The public interface ILockFileTargetLibrary (which extends ILockFileLibrary) and its Dependencies property lack XML documentation; add summary XML comments to the interface and to the IReadOnlyList<IPackageDependency> Dependencies property describing the purpose of the interface, what it represents (a lock-file target library), and what the Dependencies list contains (package dependencies for that target) so consumers understand the contract and relationship to ILockFileLibrary and IPackageDependency.src/NuGetUtility/Wrapper/NuGetWrapper/Packaging/Core/WrappedPackageDependency.cs (1)
10-14: Add null guard for constructor parameter and fix formatting.The constructor should validate that
dependencyis not null to fail fast. Also, add a blank line between the constructor and property for consistency with Allman style.♻️ Proposed fix
- public WrappedPackageDependency(PackageDependency dependency) + public WrappedPackageDependency(PackageDependency dependency!!) { - Id = dependency.Id; + Id = dependency.Id ?? throw new ArgumentNullException(nameof(dependency), $"The field {nameof(dependency.Id)} must not be null"); } + public string Id { get; }Or using traditional null check:
public WrappedPackageDependency(PackageDependency dependency) { + ArgumentNullException.ThrowIfNull(dependency); - Id = dependency.Id; + Id = dependency.Id ?? throw new ArgumentNullException(nameof(dependency), $"The field {nameof(dependency.Id)} must not be null"); } + public string Id { get; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NuGetUtility/Wrapper/NuGetWrapper/Packaging/Core/WrappedPackageDependency.cs` around lines 10 - 14, The WrappedPackageDependency constructor should validate its parameter: add a null guard at the start of WrappedPackageDependency(PackageDependency dependency) that throws ArgumentNullException(nameof(dependency)) if dependency is null, then assign Id = dependency.Id; also insert a blank line between the constructor and the public string Id { get; } property to match the Allman-style formatting.src/NuGetUtility/Wrapper/NuGetWrapper/ProjectModel/WrappedLockFileTargetLibrary.cs (1)
17-17: Add null guard for Dependencies to maintain consistency.Other properties (
Type,Name,Version) validate for null and throwArgumentNullException. TheDependenciesproperty should follow the same defensive pattern for consistency, even thoughLockFileTargetLibrary.Dependenciesis typically non-null.🛡️ Proposed fix
- Dependencies = library.Dependencies.Select(d => new WrappedPackageDependency(d)).ToArray(); + Dependencies = (library.Dependencies ?? throw new ArgumentNullException(nameof(library), $"The field {nameof(library.Dependencies)} on {nameof(library)} must not be null")) + .Select(d => new WrappedPackageDependency(d)).ToArray();Or extract to local for readability:
+ IList<NuGet.Packaging.Core.PackageDependency> dependencies = library.Dependencies ?? throw new ArgumentNullException(nameof(library), $"The field {nameof(library.Dependencies)} on {nameof(library)} must not be null"); - Dependencies = library.Dependencies.Select(d => new WrappedPackageDependency(d)).ToArray(); + Dependencies = dependencies.Select(d => new WrappedPackageDependency(d)).ToArray();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NuGetUtility/Wrapper/NuGetWrapper/ProjectModel/WrappedLockFileTargetLibrary.cs` at line 17, The constructor for WrappedLockFileTargetLibrary must validate library.Dependencies like the other properties; add a null guard for library.Dependencies (throw ArgumentNullException) before mapping, or use a null-coalescing guard when assigning Dependencies in WrappedLockFileTargetLibrary (e.g., extract library.Dependencies to a local variable and ensure it's not null before calling Select). Reference the WrappedLockFileTargetLibrary constructor and the Dependencies assignment to locate where to add the check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/NuGetUtility/Wrapper/NuGetWrapper/ProjectModel/AssetsPackageDependencyReader.cs`:
- Around line 31-49: The XML docs and null checks for
GetPackageDependenciesForTargetFramework are incorrect: update the <param> and
<exception> XML to reference the actual parameter lockFile (remove or rename the
incorrect assetsPath entry) and add a null guard at the start of
GetPackageDependenciesForTargetFramework that throws
ArgumentNullException(nameof(lockFile)) before any use of lockFile (e.g., before
accessing lockFile.Path), ensuring the documentation and runtime validation
match the method signature.
In `@src/NuGetUtility/Wrapper/NuGetWrapper/ProjectModel/ILockFile.cs`:
- Around line 10-11: The ILockFile.Targets property is now non-nullable; update
the test
GetInstalledPackages_Should_ThrowReferencedPackageReaderException_If_TargetsArrayIsNull
in ReferencedPackageReaderTest.cs to mock Targets as an empty
IEnumerable<ILockFileTarget> (e.g., Enumerable.Empty<ILockFileTarget>()) instead
of null so it exercises the valid-empty case, and in ReferencedPackageReader.cs
replace the null-coalescing check `!(assetsFile.Targets?.Any() ?? false)` with
the simpler `!assetsFile.Targets.Any()` since Targets is guaranteed non-null;
adjust any test assertions expecting null behavior to expect an empty collection
behavior instead.
In `@src/NuGetUtility/Wrapper/NuGetWrapper/ProjectModel/WrappedLockFile.cs`:
- Around line 20-21: The Targets property currently calls
_file.Targets.Select(...) and can throw NullReferenceException if _file.Targets
is null; update the Targets getter in WrappedLockFile (the Targets property that
projects to WrappedLockFileTarget) to handle null defensively—either return an
empty IEnumerable<ILockFileTarget> when _file.Targets is null (use
null-coalescing to Enumerable.Empty and then Select) or throw a descriptive
ArgumentException/InvalidOperationException indicating a malformed lock file;
ensure the change references _file.Targets, Targets, and WrappedLockFileTarget
so callers like AssetsPackageDependencyReader.BuildDependencyMapFromAssetsFile
and ReferencedPackageReader.GetSelectedTargets receive a safe/clear behavior.
- Line 21: The Path property currently returns _file.Path directly which can be
null; update WrappedLockFile to validate and expose a non-null Path like other
wrappers: in the WrappedLockFile constructor capture and validate _file.Path
(e.g., assign a private readonly string _path = _file.Path ?? throw new
InvalidOperationException("LockFile.Path is null")) and change the public string
Path => _path; reference the ILockFile contract, the Path property, the backing
field _file.Path, and mirror the null-check pattern used for Type/Name/Version
and WrappedLockFileTargetLibrary.
In `@tests/targets/PackagesConfigProject/packages.config`:
- Line 3: The test fixture references TinyCsvParser version 3.0.1 which does not
support net472 (the .nuspec only targets net8.0/net10.0); update the
packages.config entry for the TinyCsvParser package (the package
id="TinyCsvParser" element) to a version that supports net472 (e.g., revert to
2.7.2) or remove this net472 test fixture entirely if it’s no longer required so
the project’s targetFramework remains valid.
---
Nitpick comments:
In
`@src/NuGetUtility/Wrapper/NuGetWrapper/Packaging/Core/WrappedPackageDependency.cs`:
- Around line 10-14: The WrappedPackageDependency constructor should validate
its parameter: add a null guard at the start of
WrappedPackageDependency(PackageDependency dependency) that throws
ArgumentNullException(nameof(dependency)) if dependency is null, then assign Id
= dependency.Id; also insert a blank line between the constructor and the public
string Id { get; } property to match the Allman-style formatting.
In
`@src/NuGetUtility/Wrapper/NuGetWrapper/ProjectModel/ILockFileTargetLibrary.cs`:
- Around line 9-12: The public interface ILockFileTargetLibrary (which extends
ILockFileLibrary) and its Dependencies property lack XML documentation; add
summary XML comments to the interface and to the
IReadOnlyList<IPackageDependency> Dependencies property describing the purpose
of the interface, what it represents (a lock-file target library), and what the
Dependencies list contains (package dependencies for that target) so consumers
understand the contract and relationship to ILockFileLibrary and
IPackageDependency.
In
`@src/NuGetUtility/Wrapper/NuGetWrapper/ProjectModel/WrappedLockFileTargetLibrary.cs`:
- Line 17: The constructor for WrappedLockFileTargetLibrary must validate
library.Dependencies like the other properties; add a null guard for
library.Dependencies (throw ArgumentNullException) before mapping, or use a
null-coalescing guard when assigning Dependencies in
WrappedLockFileTargetLibrary (e.g., extract library.Dependencies to a local
variable and ensure it's not null before calling Select). Reference the
WrappedLockFileTargetLibrary constructor and the Dependencies assignment to
locate where to add the check.
In `@tests/NuGetUtility.Test/Extensions/StringExtensionsTest.cs`:
- Around line 44-64: The Linux/Unix path test cases (e.g. TestCase entries with
source "/home/user/projects/MyProject/MyProject.csproj" and patterns like
"*.csproj", "MyProject.csproj", "/home/user/*/MyProject/*.csproj", etc.) lack
explicit TestName attributes; add TestName to each of those TestCase attributes
to match the Windows naming convention (use a consistent convention such as
"PathLike_Unix_{Context}_{Description}" analogous to the existing
"PathLike_Windows_..." names) so failures report with parity; update the
TestCase declarations in StringExtensionsTest to include those TestName strings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0126ec3e-31dd-4462-8095-fe9bc7fe46a0
📒 Files selected for processing (19)
src/NuGetUtility/NuGetUtility.csprojsrc/NuGetUtility/ReferencedPackagesReader/ReferencedPackageReader.cssrc/NuGetUtility/Wrapper/NuGetWrapper/Packaging/Core/IPackageDependency.cssrc/NuGetUtility/Wrapper/NuGetWrapper/Packaging/Core/WrappedPackageDependency.cssrc/NuGetUtility/Wrapper/NuGetWrapper/ProjectModel/AssetsPackageDependencyReader.cssrc/NuGetUtility/Wrapper/NuGetWrapper/ProjectModel/IAssetsPackageDependencyReader.cssrc/NuGetUtility/Wrapper/NuGetWrapper/ProjectModel/ILockFile.cssrc/NuGetUtility/Wrapper/NuGetWrapper/ProjectModel/ILockFileTarget.cssrc/NuGetUtility/Wrapper/NuGetWrapper/ProjectModel/ILockFileTargetLibrary.cssrc/NuGetUtility/Wrapper/NuGetWrapper/ProjectModel/WrappedLockFile.cssrc/NuGetUtility/Wrapper/NuGetWrapper/ProjectModel/WrappedLockFileTarget.cssrc/NuGetUtility/Wrapper/NuGetWrapper/ProjectModel/WrappedLockFileTargetLibrary.cstests/FileLicenseMatcher.Test/FileLicenseMatcher.Test.csprojtests/NuGetUtility.Test/Extensions/StringExtensionsTest.cstests/NuGetUtility.Test/NuGetUtility.Test.csprojtests/NuGetUtility.Test/ReferencedPackagesReader/ReferencedPackageReaderTest.cstests/NuGetUtility.UrlToLicenseMapping.Test/NuGetUtility.UrlToLicenseMapping.Test.csprojtests/targets/PackagesConfigProject/PackagesConfigProject.csprojtests/targets/PackagesConfigProject/packages.config
✅ Files skipped from review due to trivial changes (2)
- tests/targets/PackagesConfigProject/PackagesConfigProject.csproj
- src/NuGetUtility/Wrapper/NuGetWrapper/Packaging/Core/IPackageDependency.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/NuGetUtility/Wrapper/NuGetWrapper/ProjectModel/IAssetsPackageDependencyReader.cs
| /// <summary> | ||
| /// Gets package-to-dependency mappings for libraries in the target framework section of the specified assets file. | ||
| /// </summary> | ||
| /// <param name="assetsPath">Path to the <c>project.assets.json</c> file to inspect.</param> | ||
| /// <param name="normalizedTargetFramework">Normalized target framework moniker used to select matching targets.</param> | ||
| /// <returns> | ||
| /// A dictionary that maps package IDs to a case-insensitive set of dependency package IDs | ||
| /// (<c>Dictionary<string, HashSet<string>></c>). | ||
| /// Returns an empty dictionary when the assets file does not exist or cannot be read. | ||
| /// </returns> | ||
| /// <exception cref="ArgumentNullException"> | ||
| /// Thrown when <paramref name="assetsPath"/> or <paramref name="normalizedTargetFramework"/> is <see langword="null"/>. | ||
| /// </exception> | ||
| public Dictionary<string, HashSet<string>> GetPackageDependenciesForTargetFramework(ILockFile lockFile, string normalizedTargetFramework) | ||
| { | ||
| if (normalizedTargetFramework is null) | ||
| { | ||
| throw new ArgumentNullException(nameof(normalizedTargetFramework)); | ||
| } |
There was a problem hiding this comment.
XML documentation parameter mismatch and missing null validation.
- The
<param name="assetsPath">at line 34 doesn't match the actual parameterlockFileat line 44. - The
<exception>tag at lines 41-42 referencesassetsPathbut parameter islockFile. - Missing null validation for
lockFile- if null is passed,lockFile.Pathat line 59 will throwNullReferenceException.
📝 Proposed fix
/// <summary>
/// Gets package-to-dependency mappings for libraries in the target framework section of the specified assets file.
/// </summary>
- /// <param name="assetsPath">Path to the <c>project.assets.json</c> file to inspect.</param>
+ /// <param name="lockFile">The lock file representation of the <c>project.assets.json</c> to inspect.</param>
/// <param name="normalizedTargetFramework">Normalized target framework moniker used to select matching targets.</param>
/// <returns>
/// A dictionary that maps package IDs to a case-insensitive set of dependency package IDs
/// (<c>Dictionary<string, HashSet<string>></c>).
/// Returns an empty dictionary when the assets file does not exist or cannot be read.
/// </returns>
/// <exception cref="ArgumentNullException">
- /// Thrown when <paramref name="assetsPath"/> or <paramref name="normalizedTargetFramework"/> is <see langword="null"/>.
+ /// Thrown when <paramref name="lockFile"/> or <paramref name="normalizedTargetFramework"/> is <see langword="null"/>.
/// </exception>
public Dictionary<string, HashSet<string>> GetPackageDependenciesForTargetFramework(ILockFile lockFile, string normalizedTargetFramework)
{
+ if (lockFile is null)
+ {
+ throw new ArgumentNullException(nameof(lockFile));
+ }
+
if (normalizedTargetFramework is null)
{
throw new ArgumentNullException(nameof(normalizedTargetFramework));
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/NuGetUtility/Wrapper/NuGetWrapper/ProjectModel/AssetsPackageDependencyReader.cs`
around lines 31 - 49, The XML docs and null checks for
GetPackageDependenciesForTargetFramework are incorrect: update the <param> and
<exception> XML to reference the actual parameter lockFile (remove or rename the
incorrect assetsPath entry) and add a null guard at the start of
GetPackageDependenciesForTargetFramework that throws
ArgumentNullException(nameof(lockFile)) before any use of lockFile (e.g., before
accessing lockFile.Path), ensuring the documentation and runtime validation
match the method signature.
| IEnumerable<ILockFileTarget> Targets { get; } | ||
| string Path { get; } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify all usages of ILockFile.Targets and related test mocks
rg -n "\.Targets" --type=cs -C2 | head -100Repository: sensslen/nuget-license
Length of output: 10344
🏁 Script executed:
cat -n src/NuGetUtility/Wrapper/NuGetWrapper/ProjectModel/ILockFile.csRepository: sensslen/nuget-license
Length of output: 564
🏁 Script executed:
# Check the git diff to see what changed
git diff HEAD src/NuGetUtility/Wrapper/NuGetWrapper/ProjectModel/ILockFile.csRepository: sensslen/nuget-license
Length of output: 48
🏁 Script executed:
# Look at the test method name and context
sed -n '160,175p' tests/NuGetUtility.Test/ReferencedPackagesReader/ReferencedPackageReaderTest.csRepository: sensslen/nuget-license
Length of output: 803
🏁 Script executed:
# Check line 327 context more carefully to understand null handling
sed -n '320,335p' src/NuGetUtility/ReferencedPackagesReader/ReferencedPackageReader.csRepository: sensslen/nuget-license
Length of output: 733
🏁 Script executed:
cat -n src/NuGetUtility/Wrapper/NuGetWrapper/ProjectModel/WrappedLockFile.csRepository: sensslen/nuget-license
Length of output: 1367
🏁 Script executed:
# Also check what ILockFile is wrapping - look at the import
head -30 src/NuGetUtility/Wrapper/NuGetWrapper/ProjectModel/WrappedLockFile.csRepository: sensslen/nuget-license
Length of output: 1157
🏁 Script executed:
# Check NuGet SDK documentation or package info
# First, let's see if there's any documentation or comments about the underlying LockFile
rg -n "LockFile" --type=cs src/NuGetUtility/Wrapper/NuGetWrapper/ProjectModel/ | grep -i "nuget\|sdk\|comment"Repository: sensslen/nuget-license
Length of output: 4597
🏁 Script executed:
# Search for any null checks on _file.Targets in WrappedLockFile or related code
rg -n "_file\.Targets" --type=cs -A2 -B2Repository: sensslen/nuget-license
Length of output: 672
🏁 Script executed:
# Check if NuGet.ProjectModel LockFile.Targets is documented as nullable
web_search
NuGet.ProjectModel.LockFile.Targets property nullable documentationRepository: sensslen/nuget-license
Length of output: 190
🌐 Web query:
NuGet.ProjectModel.LockFile.Targets property nullable
💡 Result:
In NuGet.ProjectModel.LockFile, Targets is not declared nullable (it’s IList<LockFileTarget>) and it is initialized to an empty list by default (= new List<LockFileTarget>()). So, under normal construction/deserialization paths, you should treat it as non-null (but possibly empty). [1]
That said, because it has a public setter, it could be set to null by consumer code, but that’s not the intended/assumed state (methods like GetTarget(...) call Targets.FirstOrDefault(...), which would throw if Targets were null). [1]
Sources:
[1] (raw.githubusercontent.com)
Citations:
Update test mock for non-nullable Targets property.
The test GetInstalledPackages_Should_ThrowReferencedPackageReaderException_If_TargetsArrayIsNull (line 165 in ReferencedPackageReaderTest.cs) will fail because it attempts to mock Targets as null. Since NuGet SDK's LockFile.Targets is initialized to an empty list and designed to never be null, the interface change to non-nullable is correct. Remove or refactor this test to use an empty collection instead, which is the valid edge case that triggers the exception.
Additionally, the null-coalescing check at line 327 in ReferencedPackageReader.cs (!(assetsFile.Targets?.Any() ?? false)) can be simplified to !assetsFile.Targets.Any() since Targets is now guaranteed non-null.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/NuGetUtility/Wrapper/NuGetWrapper/ProjectModel/ILockFile.cs` around lines
10 - 11, The ILockFile.Targets property is now non-nullable; update the test
GetInstalledPackages_Should_ThrowReferencedPackageReaderException_If_TargetsArrayIsNull
in ReferencedPackageReaderTest.cs to mock Targets as an empty
IEnumerable<ILockFileTarget> (e.g., Enumerable.Empty<ILockFileTarget>()) instead
of null so it exercises the valid-empty case, and in ReferencedPackageReader.cs
replace the null-coalescing check `!(assetsFile.Targets?.Any() ?? false)` with
the simpler `!assetsFile.Targets.Any()` since Targets is guaranteed non-null;
adjust any test assertions expecting null behavior to expect an empty collection
behavior instead.
| public IEnumerable<ILockFileTarget> Targets => _file.Targets.Select(t => new WrappedLockFileTarget(t)); | ||
| public string Path => _file.Path; |
There was a problem hiding this comment.
Add defensive null handling for _file.Targets to prevent NullReferenceException.
The Targets property directly chains .Select() on _file.Targets without null-coalescing. While LockFile.Targets is typically non-null in valid lock files, edge cases (corrupted files, incomplete parsing) could result in null, causing a NullReferenceException that propagates up as an unhandled crash rather than a meaningful error.
Per context snippets, AssetsPackageDependencyReader.BuildDependencyMapFromAssetsFile and ReferencedPackageReader.GetSelectedTargets iterate over lockFile.Targets without guards, relying on this property being safe.
🛡️ Proposed defensive handling
Option 1 - Return empty on null (silent fallback):
- public IEnumerable<ILockFileTarget> Targets => _file.Targets.Select(t => new WrappedLockFileTarget(t));
+ public IEnumerable<ILockFileTarget> Targets => _file.Targets?.Select(t => new WrappedLockFileTarget(t)) ?? Enumerable.Empty<ILockFileTarget>();Option 2 - Throw descriptive exception (fail-fast):
- public IEnumerable<ILockFileTarget> Targets => _file.Targets.Select(t => new WrappedLockFileTarget(t));
+ public IEnumerable<ILockFileTarget> Targets => (_file.Targets ?? throw new InvalidOperationException($"Lock file '{_file.Path}' has null Targets"))
+ .Select(t => new WrappedLockFileTarget(t));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/NuGetUtility/Wrapper/NuGetWrapper/ProjectModel/WrappedLockFile.cs` around
lines 20 - 21, The Targets property currently calls _file.Targets.Select(...)
and can throw NullReferenceException if _file.Targets is null; update the
Targets getter in WrappedLockFile (the Targets property that projects to
WrappedLockFileTarget) to handle null defensively—either return an empty
IEnumerable<ILockFileTarget> when _file.Targets is null (use null-coalescing to
Enumerable.Empty and then Select) or throw a descriptive
ArgumentException/InvalidOperationException indicating a malformed lock file;
ensure the change references _file.Targets, Targets, and WrappedLockFileTarget
so callers like AssetsPackageDependencyReader.BuildDependencyMapFromAssetsFile
and ReferencedPackageReader.GetSelectedTargets receive a safe/clear behavior.
| public IPackageSpec PackageSpec => new WrappedPackageSpec(_file.PackageSpec); | ||
| public IEnumerable<ILockFileTarget>? Targets => _file.Targets?.Select(t => new WrappedLockFileTarget(t)); | ||
| public IEnumerable<ILockFileTarget> Targets => _file.Targets.Select(t => new WrappedLockFileTarget(t)); | ||
| public string Path => _file.Path; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how LockFile instances are created and if Path is always set
rg -n "LockFile" --type=cs -A3 | grep -E "(new LockFile|\.Path)" | head -30Repository: sensslen/nuget-license
Length of output: 406
🏁 Script executed:
# Find the ILockFile interface definition
rg -n "interface ILockFile" --type=cs -A 20Repository: sensslen/nuget-license
Length of output: 3093
🏁 Script executed:
# Check WrappedLockFile initialization and _file field
rg -n "class WrappedLockFile" --type=cs -A 30Repository: sensslen/nuget-license
Length of output: 8437
🏁 Script executed:
# Find LockFileFactory implementation
rg -n "class LockFileFactory" --type=cs -A 20Repository: sensslen/nuget-license
Length of output: 1730
🏁 Script executed:
# Check if there are any null checks for Path in the codebase
rg -n "Path.*null|null.*Path" --type=cs | head -20Repository: sensslen/nuget-license
Length of output: 1777
🏁 Script executed:
# Check if PackageSpec, Libraries, and Targets are also validated elsewhere
rg -n "PackageSpec|_file.Libraries|_file.Targets" --type=cs -B 2 -A 2 | grep -E "(new Wrapped|??|throw|null)" | head -20Repository: sensslen/nuget-license
Length of output: 1978
🏁 Script executed:
# Check complete WrappedLockFile file to see all properties
cat -n src/NuGetUtility/Wrapper/NuGetWrapper/ProjectModel/WrappedLockFile.csRepository: sensslen/nuget-license
Length of output: 1367
Add null validation to Path property to satisfy non-nullable contract.
The Path property is declared as non-nullable string in ILockFile, but directly returns _file.Path from the NuGet SDK's LockFile without validation. This is inconsistent with the null-checking pattern used elsewhere in the codebase (e.g., WrappedLockFileTargetLibrary). The calling code also defensively null-checks Targets with ?., indicating the concern is legitimate. Add validation using null-coalescing or throw in the constructor, similar to the approach used for Type, Name, and Version in other wrapped types.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/NuGetUtility/Wrapper/NuGetWrapper/ProjectModel/WrappedLockFile.cs` at
line 21, The Path property currently returns _file.Path directly which can be
null; update WrappedLockFile to validate and expose a non-null Path like other
wrappers: in the WrappedLockFile constructor capture and validate _file.Path
(e.g., assign a private readonly string _path = _file.Path ?? throw new
InvalidOperationException("LockFile.Path is null")) and change the public string
Path => _path; reference the ILockFile contract, the Path property, the backing
field _file.Path, and mirror the null-check pattern used for Type/Name/Version
and WrappedLockFileTargetLibrary.
| <?xml version="1.0" encoding="utf-8"?> | ||
| <packages> | ||
| <package id="TinyCsvParser" version="2.7.2" targetFramework="net472" /> | ||
| <package id="TinyCsvParser" version="3.0.1" targetFramework="net472" /> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify TinyCsvParser 3.0.1 exists on NuGet and is compatible with net472
# Check if version 3.0.1 exists
curl -s "https://api.nuget.org/v3-flatcontainer/tinycsvparser/index.json" | jq '.versions | map(select(. == "3.0.1"))'
# Get package metadata for 3.0.1 to check target frameworks
curl -s "https://api.nuget.org/v3/registration5-semver1/tinycsvparser/3.0.1.json" | jq '.catalogEntry.dependencyGroups[].targetFramework' 2>/dev/null || echo "Checking alternate endpoint..."
curl -s "https://api.nuget.org/v3-flatcontainer/tinycsvparser/3.0.1/tinycsvparser.nuspec" | grep -i "targetframework\|dependencies" | head -20Repository: sensslen/nuget-license
Length of output: 232
TinyCsvParser 3.0.1 does not support net472.
The version bump from 2.7.2 to 3.0.1 introduces a critical incompatibility. The .nuspec for version 3.0.1 lists only net8.0 and net10.0 as supported target frameworks—not net472. Since this test fixture is specifically for a net472 project, using version 3.0.1 will break the test. Either revert to a version that supports net472, or remove this test fixture if it's no longer needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/targets/PackagesConfigProject/packages.config` at line 3, The test
fixture references TinyCsvParser version 3.0.1 which does not support net472
(the .nuspec only targets net8.0/net10.0); update the packages.config entry for
the TinyCsvParser package (the package id="TinyCsvParser" element) to a version
that supports net472 (e.g., revert to 2.7.2) or remove this net472 test fixture
entirely if it’s no longer required so the project’s targetFramework remains
valid.
There was a problem hiding this comment.
I suspect that upgrading TinyCsvParser to 3.0.1 might be related to the net472 test failures I'm seeing locally.
CodeRabbit suggests a potential incompatibility, and I am getting the following fatal error during test discovery:
A fatal error was encountered. The library 'hostpolicy.dll' required to execute the application was not found.
According to NuGet, version 3.0.1 lists only net8.0 and net10.0 as supported frameworks:
If we need to maintain net472 support, do you think we should consider reverting this package to 2.7.2, or do you have other plans for the test environment?
There was a problem hiding this comment.
@GK-302, yes — reverting to 2.7.2 is the right call here. The 3.0.1 upgrade dropped net472 support entirely (it targets only net8.0 and net10.0), so the hostpolicy.dll fatal error you're seeing is a direct consequence of the net472 runtime being unable to load a net8.0-only assembly.
The packages.config fixture is purely for testing packages.config-style resolution against a net472 project; there's no benefit in upgrading it to a version that doesn't even support net472.
Note that the companion file tests/targets/PackagesConfigProject/PackagesConfigProject.csproj was also updated in this PR to reference the 3.0.1.0 assembly DLL hint path — both files need to be reverted together:
tests/targets/PackagesConfigProject/packages.config
- <package id="TinyCsvParser" version="3.0.1" targetFramework="net472" />
+ <package id="TinyCsvParser" version="2.7.2" targetFramework="net472" />tests/targets/PackagesConfigProject/PackagesConfigProject.csproj
- <HintPath>..\..\packages\TinyCsvParser.3.0.1.0\lib\netstandard2.0\TinyCsvParser.dll</HintPath>
+ <HintPath>..\..\packages\TinyCsvParser.2.7.2.0\lib\netstandard2.0\TinyCsvParser.dll</HintPath>(The exact hint path format should match what was there before the change in this PR.)
🧠 Learnings used
Learnt from: CR
Repo: sensslen/nuget-license PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-27T19:49:51.688Z
Learning: Applies to **/src/**/*.csproj : Target multiple .NET versions including .NET Framework 4.7.2, net8.0, net9.0, and net10.0, and test code changes on all target frameworks when modifying core functionality
Learnt from: CR
Repo: sensslen/nuget-license PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-27T19:49:51.688Z
Learning: Avoid adding new NuGet dependencies unless absolutely necessary, and when adding dependencies, ensure they work with all target frameworks (net8.0, net9.0, net10.0, net472)
|
@GK-302 would you mind reverting the update for TinyCsvParser so that the build/tests succeed? The remaining feedback I can address in a follow up.... |
|
|
I reverted the TinyCsvParser update as requested to unblock this PR and align with the previous working package versions.
I’ll take you up on your offer and leave the remaining feedback for you to handle in a follow-up. Thanks for the help! |
|
@sensslen Thank you for merging my PR! I'm so happy that I could work on this with your support over the last month. I hope this feature will be helpful for all nuget-license users. I really like this project and look forward to contributing again! |
2 participants
Summary
Adds a new
--exclude-publish-falseoption to excludePackageReferenceitems marked with<Publish>false</Publish>from license analysis. The implementation excludes transitive dependencies only when they are not reachable from publishable direct dependencies for the evaluated target framework.New Features
--exclude-publish-false.Publish="false"direct references.NuGet.ProjectModel(LockFile) for assets parsing andNuGet.Frameworksfor framework normalization/equivalence handling.Technical Details
Publish-false exclusion semantics
Publish=falseare identified via evaluatedPackageReferencemetadata from MSBuild items.Target framework handling
Refactoring and maintainability
NuGetFrameworkUtility.AssetsPackageDependencyReader.Testing
Expanded
ReferencedPackageReaderTestcoverage forpublish-falsebehavior, including:publish-falseroots.targetFrameworkis omitted.Documentation
--exclude-publish-falseoption and behavior.References
NuGet.ProjectModel(project.assets.json)Summary by CodeRabbit
New Features
Behavior
Tests
Documentation