This method signature is very limiting. It only allows us to report true or false for this interface that tells if there are vulnerabilities in the solution or not. The variable is even called has vulnerabilities "in solution" which is very specific to where the vulnerabilities are found. Overall this interface has a lot of implementation details in its signature.

Instead, we could have a method ReportVulnerabilitiesAsync that accepts a list of vulnerabilities from the caller and the implementation decides what to do with it. In the implementation you need for the InfoBar, it would just count the list and if it's greater than 0 show the message that there are vulnerabilities in the solution.

We should either change the interface to be less implementation-specific or remove the interface altogether and reference the concrete class directly.

Instead, we could have a method ReportVulnerabilitiesAsync that accepts a list of vulnerabilities from the caller and the implementation decides what to do with it. In the implementation you need for the InfoBar, it would just count the list and if it's greater than 0 show the message that there are vulnerabilities in the solution.

I'm not sure I agree. I see that as over optimization.
No need to allocate extra collections if no one is gonna make use of that.
This method is easy to change if there's a better use for it in the future.

Looking more closely, I see you don't actually have the list of vulnerabilities from the restore, but rather a list restore summaries and their errors. So then what we're really reporting is on specific restore errors that are occurring which in this usage happens to be ones that occur when the restore audit finds a vulnerability. The name of this interface and its method should reflect its usage then. We are providing a way for restore to report on restore errors, not report vulnerabilities.

We are providing a way for restore to report on restore errors, not report vulnerabilities

No, restore is reporting that vulnerabilities have been found in any project within the solution. (Probably how Martin arrived at the interface name: IVulnerabilitiesFoundService)
Vulnerabilities are not necessarily errors.

The purpose of this interface is to "notify" the InfoBar that there are vulnerabilities, which as Nikolche said, are not necessarily errors.

I think that changing this Interface to a more generic one, like report all restore errors, is going to involve a lot of changes. We probably already have "components" that react to restore errors and are not centralized in one Interface.

No, restore is reporting that vulnerabilities have been found in any project within the solution.

This is an implementation detail, not an interface specification. The SolutionRestoreJob should not be responsible for deciding what should be surfaced to the user through the UI. We're overloading the responsibilities of a restore job to also decide what is important to show to users. If this ever changes, and we want to report other errors / warnings / results, even if to a different place, we then have to change the SolutionRestoreJob again which violates the open-closed principle.

@martinrrm feel free to merge the PR on the basis of others' approvals. Please do not consider this a blocker, although I disagree with this design and cannot in good conscience approve the PR, I do not believe it should block the PR either. The rest LGTM.

The RestoreSummary.Errors is a poor name, but it's documented that it's warnings and errors,

. Unfortunately we can't fix that due to that library being a part of https://github.com/NuGet/NuGet.Client/blob/dev/docs/nuget-sdk.md.
Fortunately for us, NuGet.SolutionRestoreManager isn't part of that SDK

This is an implementation detail, not an interface specification. The SolutionRestoreJob should not be responsible for deciding what should be surfaced to the user through the UI. We're overloading the responsibilities of a restore job to also decide what is important to show to users

I don't think that's true. SolutionRestoreJob is not deciding what's being surfaced. It's notifying via an interface that there are restore time vulnerability warnings or errors. The implementer is deciding to surface an info bar.

Software is going to change. Trying to predict that change is impossible.

I don't think that's true. SolutionRestoreJob is not deciding what's being surfaced.

Of all the errors and warnings (let's call them Codes) the SolutionRestoreJob can encounter, it is deciding that the presence of vulnerabilities is the only thing that matters to surface. While it does not make any claims on how that should be presented, it does decide that no other information from the restore is going to be surfaced. Any changes to this in the future require a change to the SolutionRestoreJob and potentially the addition of yet another ISomethingNotificationService or a change to the IVulnerabilitiesNotificationService. If instead the interface method accepted the Codes, the implementations could decide what is of importance and the SolutionRestoreJob doesn't have to care what is determined as being important, it just does its job and allows the implementation of the notification service to decide what it will present. Of course this design also allows for composite implementations that each handle different Codes and we get that flexibility for free.

Happy to chat about design principles more offline. For the purposes of this PR, I think we are in agreement that it can be merged without this blocking, provided it gets sufficient approvals.

Of all the errors and warnings (let's call them Codes) the SolutionRestoreJob can encounter, it is deciding that the presence of vulnerabilities is the only thing that matters to surface

I think this perspective is one where reasonable people would easily disagree :)

Why does the restore job have to alert anyone of any error codes? After all the assets file is the final output for the operation. Why not read the assets file as it's the final source of truth anyways?