feat: dedicated Semgrep findings view - #336
Open
semgrep-chris wants to merge 3 commits into
Open
Conversation
Findings currently only reach the user through the Problems panel, via the LSP diagnostics the language client publishes. That works, but a diagnostic carries only range/severity/source/message/code — there is no `data` payload — so a rule's metadata never reaches the extension, and CRITICAL has already been flattened into Error by the time we see it. This adds a Findings view backed by `semgrep scan --json` instead, which carries the whole rule: `metadata.vulnerability_class`, CWE, the registry shortlink, and the full four-level severity. Findings are grouped by vulnerability class by default, each row naming the rule that caught it and where. Rule, File and Severity groupings are also available. The scan is on demand — the view runs on live diagnostics until the user asks for one — so opening a window never triggers an unexpected workspace scan. Two things worth calling out for review: - The CLI replaces rather than merges `--config`, so passing the user's `semgrep.scan.configuration` alone silently drops the deployment policy, which the language server does include. `--config auto` is added back alongside local configs to match. Falls back to local configs only when resolving `auto` fails, which happens when Semgrep's metrics are disabled. - Rules are authored against two severity vocabularies (ERROR/WARNING/INFO and CRITICAL/HIGH/MEDIUM/LOW) and both appear in scan output, so HIGH and MEDIUM are mapped rather than falling through to info. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Also corrects a comment that referred to a `sourceLabel` which does not exist; the header text is built in `updateHeader`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a dedicated Findings view to the Semgrep sidebar, so findings have a home of their own rather than only appearing mixed into the Problems panel.
Grouped by vulnerability class by default; each row names the rule that caught the finding and where it is, and clicking opens the file at that range. Group By in the view toolbar also offers rule, file and severity. Right-click a finding for Open Rule Documentation / Copy Message.
Why the CLI and not diagnostics
The obvious implementation is to read the LSP diagnostics the language client already holds — that is what fills the Problems panel. I started there, but a Semgrep diagnostic carries only
range/severity/source/message/code, with nodatapayload, so none of the rule's metadata reaches the extension. I verified this directly againstsemgrep lspwithdataSupport: true; a rule withvulnerability_class,cweandowaspset produces a diagnostic with none of it.semgrep/workspaceRulesis declared inlspExtensions.tsbut never answers.semgrep scan --jsoncarries the whole rule:metadata.vulnerability_class,cwe,shortlink, and the full four-level severity — LSP diagnostics have three levels, so CRITICAL is already flattened into Error by the time the client sees it.The trade-off is that this is a second scan, separate from the language server's. It therefore runs only on demand: the view shows live diagnostics until someone presses scan, so opening a window never triggers an unexpected workspace scan. The header states which source is in use.
Two things I'd like a careful look at
1.
--configmerge semantics. The CLI replaces configs where the language server merges. Passing the user'ssemgrep.scan.configurationalone silently drops the deployment policy:--config my-rules.yaml--config auto --config my-rules.yamlSo
--config autois added back alongside local configs to match the language server. This divergence is arguably a bug in its own right, independent of this view — anyone withscan.configurationset gets a narrower CLI scan than their LSP results suggest.Resolving
autorequires Semgrep's metrics to be enabled, so there is a fallback to local configs only when it fails; without that, users who have turned metrics off get no findings at all.2. Severity vocabularies. Rules are authored against both
ERROR/WARNING/INFOandCRITICAL/HIGH/MEDIUM/LOW, and both reach the scan output. Handling only the first set silently demotes aHIGHfinding to an info icon at the bottom of the tree.HIGH→error andMEDIUM→warning are now mapped.Test plan
The repo suite passes in full, exit 0:
npm testis currently broken on VS Code stable, unrelated to this PR. VS Code 1.131 renamed its main binary fromElectrontoCode; the pinned@vscode/test-electron@^2.3.8still looks forElectron, so the runner exits withENOENTbefore running a single test. I worked around it locally with a symlink inside.vscode-test/. This will hit CI and every contributor and probably deserves its own issue plus a dependency bump.Manual verification, in the extension development host against a workspace scanned with the deployment policy:
semgrep scan --jsonresults.No unit tests are included for the new modules. The tree-building and rule-id logic are pure functions over parsed scan output and would test well; I did not add them, and that is a fair thing to ask for before merge.
Performance
Checked against synthetic result sets at the scale of a large monorepo, because the first version froze the extension host:
The rule-id disambiguation was quadratic in unique rule count; it now counts suffix occurrences once per depth. Findings are also memoised per refresh, groups start collapsed above 40 findings, and diagnostic events no longer trigger rebuilds once scan results own the tree.
Notes for the reviewer
src/extension.tsgains a registration;package.jsongains the view, five commands and a submenu.semgrepBinaryPathwas extracted intosrc/semgrepBinary.tsrather than exported fromlsp.ts, so the scanner does not pull in the language client just to resolve a path. It mirrorsfindSemgrep's precedence, which is duplication worth flagging — the two could drift.🤖 Generated with Claude Code