VSCode should surface BestPractice or ManualReview severity rules in problems window

[JaneX8]

Describe the bug
Only hits from custom rules with severity: moderate and severity: critical show up in VS code.

To Reproduce
All are selected:

The problems tab shows low/note problems from other linters but not from DevSkim:

Expected behavior
severities: critical (red), moderate (yellow), low (blue). And a fallback to blue for unsupported keys because otherwise they won't show at all in VScode.

Versions(please complete the following information):

• OS: Windows 11
• devskim 1.0.44+10b85ce690

Additional context
I tried the others I found, none show a blue I icon at note/low level:

"low",
"medium",
"high",
"critical",
"moderate",
"important",
"manualreview",
"bestpractice",
"unspecified",
"Critical",
"ManualReview",
"BestPractice"

[gfs]

You can configure this in the DevSkim VS Code extension settings.

[JaneX8]

Ok, I'm sorry but I don't get a blue icon in VScode like in my first screenshot. Only confidence has a low option. The confidence has nothing to do with the icon?

Severity has critical and important which are both a red icon. And moderate which is yellow. No low severity option. Also ManualReview and BestPractice (apparently a severity) enabled don't turn into a blue icon either.

[gfs]

Low is not a valid value for the Severity enum:

https://github.com/microsoft/ApplicationInspector/blob/c84796ae8120f835165bc98d182fa9fb9540e87d/AppInspector.RulesEngine/Severity.cs#L13

The matches do show up with intellisense when enabled but do not show up in the problems window:

I think this is because we map ManualReview and BestPractice to Hint:

DevSkim/DevSkim-DotNet/Microsoft.DevSkim.LanguageServer/TextDocumentSyncHandler.cs

Lines 122 to 134 in 210b5d1

	private static DiagnosticSeverity DevSkimSeverityToDiagnositicSeverity(Severity ruleSeverity)
	{
	return ruleSeverity switch
	{
	Severity.Unspecified => DiagnosticSeverity.Hint,
	Severity.Critical => DiagnosticSeverity.Error,
	Severity.Important => DiagnosticSeverity.Error,
	Severity.Moderate => DiagnosticSeverity.Warning,
	Severity.BestPractice => DiagnosticSeverity.Hint,
	Severity.ManualReview => DiagnosticSeverity.Hint,
	_ => DiagnosticSeverity.Information
	};
	}

According to the VSCode documentation for the enum values, Hint seems to be the most appropriate for this class of detections:
https://vscode-api.js.org/enums/vscode.DiagnosticSeverity.html

Something to hint to a better way of doing it, like proposing a refactoring.

Once populated by the LSP its up to VS Code to decide how to surface the results, and it seems that for hints they do not populate them in the problems window.

Perhaps one of these severities would work better as Information to give a way for messages to appear in the problems section without moving severity for the rule up to Warning. Making such a change would require that we review all the default rules with that severity to make sure they are actionable enough to surface in the problems section.

[JaneX8]

I see, thank you for further investigating and reopening. Having these exact and consistent possible values defined in the proposed JSON Schema would really help here too. As well as documenting the exact mapping.

I propose two solutions, which either way I think should be documented in this mapping table that documents mapping between DevSkim and Sarif, it should add a column with the VSCode mapping. Which should be consistent with the Sarif standard '3.27.10 level property 'too. Meaning sarif severity none should be mapped too. Which fits DevSkim Unspecified.

I'd almost ask why not just use only the four 'Enumeration DiagnosticSeverity' levels Error/Warning/Information/Hint. Beyond VSCode I don't use DevSkim, so I don't exactly know in what context ManualReview and Best-Practise (btw yet another writing style in VSCode docs) are used, but I can imagine they are other use cases outside VSCode as well the importance of maintaining backwards combability.

Proposed solution 1 (change ManualReview/BestPractise to Information instead of Hint) and add Sarif None:
I think without re-evaluating all rules it is safe to map/assign ManualReview = Information and BestPractice = Information, instead of mapping them to Hint since this won't require changes to existing rules for now, while enabling it's use in VSCode, which currently can't use information at all, so doesn't work as expected. The difference between Hint and Information seems very minor while Information is broad enough to cover Hints too. Optionally even the severity 'low' could be added for completeness.

DevSkim severity	VScode DiagnosticSeverity	Sarif Level
Important, Critical	error	error
Moderate	warning	warning
ManualReview, BestPractice(, Low)	information	note
Unspecified	hint	none

Proposed solution 2 (introduce Low and add Sarif None):
I like the simplicity of simple low/medium/high but again, I don't know of the use-cases outside of VSCode so I guess that's not an option. Another option while keeping backwards compatibility is to add a severity "informational" or "low" for this particular use case in VSCode, or even name it 'information' and 'hint' as VSCode does. But again, it needs to be properly documented and I think it will only add to confusion.

DevSkim severity	VScode DiagnosticSeverity	Sarif Level
Important, Critical	error	error
Moderate	warning	warning
Low	information	note
ManualReview, BestPractise	hint	note
Unspecified	hint	none

Either solution would only affect the VSCode extension and won't affect existing rules. However the second solution won't show existing rules without re-evaluating in VSCode. I would very much prefer solution 1.

[gfs]

I propose two solutions, which either way I think should be documented in this mapping table

I think its reasonable to update that table with the diagnostic severity mappings as well.

I think without re-evaluating all rules it is safe to map/assign ManualReview = Information and BestPractice = Information

While the intention of Best Practice can roughly map to low severity, ManualReview does not cleanly map to the concept of low severity. Manual review is for cases where it requires a manual review to determine the importance of the issue. So, I think either the second solution of adding a low severity or treating best practice as low but leaving Manual Review alone is best.
 
That said, any change to how the rules present in VS Code will affect behavior of existing rules. Since those rules do not present findings in the problem window now, if the mapping is changing between severity and diagnostic level those findings will begin to appear in the problems window. This is mitigated by that best practice and manual review rules are disabled by default, so the majority of users will never see a difference, but I think is another point in favor of simply adding a low severity.

[JaneX8]

I'd make a PR but I never wrote CS and won't know how to test it, so probably a bad idea.

If you want I can work on a documentation PR but I would like to finish the JSON schema because that makes things a lot easier.