SARIF reporter #10759

xmo-odoo · 2025-11-25T13:35:00Z

	Type
✓	✨ New feature

SARIF is a unified file format used to exchange information between static analysis tools (like pylint) and various types of formatters, meta-runners, broadcasters / alert system, ...

This implementation is ad-hoc, and non-validating.

Spec v Github

Turns out Github both doesn't implement all of SARIF (which makes sense) and requires a bunch of properties which the spec considers optional. The official SARIF validator (linked to by both oasis and github) was used to validate the output of the reporter, ensuring that all the github requirements it flags are fulfilled, and fixing some of the validator's pet issues.

As of now the following issues are left unaddressed:

azure requires run.automationDetails, looking at the spec I don't think it makes sense for the reporter to inject that, it's more up to the CI
the validator wants a run.versionControlProvenance, same as above
the validator wants rule names in PascalCase, lol
the validator wants templated result messages, but without pylint providing the args as part of the Message that's a bit of a chore
the validator wants region to include a snippet (the flagged content)
the validator wants physicalLocation to have a contextRegion (most likely with a snippet)

On URIs

The reporter makes use of URIs for artifacts (~files). Per "guidance on the use of artifactLocation objects", uri should capture the deterministic part of the artifact location and uriBaseId should capture the non-deterministic part. However as far as I can tell pylint has no requirement (and no clean way to require) consistent resolution roots: path is just relative to the cwd, and there is no requirement to have project-level files to use pylint. This makes the use of relative uris dodgy, but absolute uris are pretty much always broken for the purpose of interchange so they're not really any better.

As a side-note, Github asserts

While this [nb: originalUriBaseIds] is not required by GitHub for the code scanning results to be displayed correctly, it is required to produce a valid SARIF output when using relative URI references.

However per 3.4.4 this is incorrect, the uriBaseId can be resolved through end-user configuration, originalUriBaseIds, external information (e.g. envvars), or heuristics.

It would be nice to document the "relative root" via originalUriBaseIds (which may be omitted for that purpose per 3.14.14, but per the above claiming a consistent project root is dodgy.

We could resolve known project files (e.g. pyproject.toml, tox.ini, etc...) in order to find a consistent root (project root, repo root, ...) and set / use that for relative URIs but that's a lot of additional complexity which I'm not sure is warranted at least for a first version.

Fixes #5493

Pierre-Sassoulas

Thank you, this is great.

I'm not a SARIF expert so I have some code placement comments, and I hope someone more knowledgeable in SARIF will have an opinion about the actual content.

pylint/reporters/json_reporter.py

tests/reporters/unittest_json_reporter.py

pylint/reporters/sarif_types.py

codecov · 2025-11-25T13:49:45Z

Codecov Report

❌ Patch coverage is 97.93814% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 95.99%. Comparing base (7588243) to head (8878b8e).
⚠️ Report is 11 commits behind head on main.

Files with missing lines	Patch %	Lines
pylint/reporters/sarif_reporter.py	97.91%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #10759   +/-   ##
=======================================
  Coverage   95.98%   95.99%           
=======================================
  Files         176      177    +1     
  Lines       19560    19657   +97     
=======================================
+ Hits        18775    18870   +95     
- Misses        785      787    +2

Files with missing lines	Coverage Δ
pylint/lint/base_options.py	`100.00% <ø> (ø)`
pylint/reporters/__init__.py	`100.00% <100.00%> (ø)`
pylint/reporters/sarif_reporter.py	`97.91% <97.91%> (ø)`

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

xmo-odoo · 2025-11-25T13:51:41Z

I'm not a SARIF expert so I have some code placement comments, and I hope someone more knowledgeable in SARIF will have an opinion about the actual content.

Maybe @nvuillam is still active since they're the original requestor.

nvuillam · 2025-11-25T16:35:03Z

As long as the sarif validator validates the SARIF output, it's good :)

pylint/reporters/sarif_reporter.py

github-actions · 2025-11-26T14:15:29Z

🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉

This comment was generated for commit ebf822c

xmo-odoo · 2025-11-27T08:07:53Z

Not entirely sure how to fix the coverage issue as it's mostly for the conversion of windows paths to relative URIs.

Pierre-Sassoulas

Implementation LGTM, it seems we don't need to cover everything as long as we add sarif validation in the test suite ?

xmo-odoo · 2025-11-27T13:00:42Z

Implementation LGTM, it seems we don't need to cover everything as long as we add sarif validation in the test suite ?

Not sure it's that useful: the type specs should be the useful (to pylint) subset of the json schema (modulo a rule or two which are painful to type), while the "official" validator with additional suggestions or service-specific requirements doesn't seem to be available as anything but a webservice of sorts?

Pierre-Sassoulas · 2025-11-27T13:18:01Z

pylint maintainers will probably have to deal with bug fixes on this, so we need to have something to make sure this does not regress somehow. We do have windows specific CI jobs though, so we can cover the windows specific path.

The config uses the `pre-commit/pre-commit-hook` repository which since v5.0 has multiple hooks using the `pre-commit` stage which was only added in pre-commit [3.2.0][] causing a run failure when run under a pre-commit pinned to 2.2. From a very surface looks it seems like https://pre-commit.ci/ is anal about keeping everything on latest so pinning pre-commit in tox seems like it can only cause problems (by drifting behind the actual CI). Of note: it looks like the `minimum_pre_commit_version` stricture on hooks does not do anything: all the `pre-commit` hooks are filtered on version except [`destroyed-symlinks`][] but I get an error on `check-added-large-file`: ==> At Hook(id='check-added-large-files') ==> At key: stages ==> At index 0 =====> Expected one of commit, commit-msg, manual, merge-commit, post-checkout, post-commit, post-merge, post-rewrite, prepare-commit-msg, push but got: 'pre-commit' [3.2.0]: https://github.com/pre-commit/pre-commit/blob/main/CHANGELOG.md#320---2023-03-17 [`destroyed-symlinks`]: pre-commit/pre-commit-hooks@c7d1e85

SARIF is a unified file format used to exchange information between static analysis tools (like pylint) and various types of formatters, meta-runners, broadcasters / alert system, ... This implementation is ad-hoc, and non-validating. Spec v Github ------------- Turns out Github both doesn't implement all of SARIF (which makes sense) and requires a bunch of properties which the spec considers optional. The [official SARIF validator][] (linked to by both oasis and github) was used to validate the output of the reporter, ensuring that all the github requirements it flags are fulfilled, and fixing *some* of the validator's pet issues. As of now the following issues are left unaddressed: - azure requires `run.automationDetails`, looking at the spec I don't think it makes sense for the reporter to inject that, it's more up to the CI - the validator wants a `run.versionControlProvenance`, same as above - the validator wants rule names in PascalCase, lol - the validator wants templated result messages, but without pylint providing the args as part of the `Message` that's a bit of a chore - the validator wants `region` to include a snippet (the flagged content) - the validator wants `physicalLocation` to have a `contextRegion` (most likely with a snippet) On URIs ------- The reporter makes use of URIs for artifacts (~files). Per ["guidance on the use of artifactLocation objects"][3.4.7], `uri` *should* capture the deterministic part of the artifact location and `uriBaseId` *should* capture the non-deterministic part. However as far as I can tell pylint has no requirement (and no clean way to require) consistent resolution roots: `path` is just relative to the cwd, and there is no requirement to have project-level files to use pylint. This makes the use of relative uris dodgy, but absolute uris are pretty much always broken for the purpose of *interchange* so they're not really any better. As a side-note, Github [asserts][relative-uri-guidance] > While this [nb: `originalUriBaseIds`] is not required by GitHub for > the code scanning results to be displayed correctly, it is required > to produce a valid SARIF output when using relative URI references. However per [3.4.4][] this is incorrect, the `uriBaseId` can be resolved through end-user configuration, `originalUriBaseIds`, external information (e.g. envvars), or heuristics. It would be nice to document the "relative root" via `originalUriBaseIds` (which may be omitted for that purpose per [3.14.14][], but per the above claiming a consistent project root is dodgy. We *could* resolve known project files (e.g. pyproject.toml, tox.ini, etc...) in order to find a consistent root (project root, repo root, ...) and set / use that for relative URIs but that's a lot of additional complexity which I'm not sure is warranted at least for a first version. Fixes pylint-dev#5493 [3.4.4]: https://docs.oasis-open.org/sarif/sarif/v2.1.0/csprd01/sarif-v2.1.0-csprd01.html#_Toc10540869 [3.4.7]: https://docs.oasis-open.org/sarif/sarif/v2.1.0/csprd01/sarif-v2.1.0-csprd01.html#_Toc10540872 [3.14.14]: https://docs.oasis-open.org/sarif/sarif/v2.1.0/csprd01/sarif-v2.1.0-csprd01.html#_Toc10540936 [relative-uri-guidance]: https://docs.github.com/en/code-security/code-scanning/integrating-with-code-scanning/sarif-support-for-code-scanning#relative-uri-guidance-for-sarif-producers [official SARIF validator]: https://sarifweb.azurewebsites.net/

cdce8p · 2025-12-08T15:10:27Z

pyproject.toml

+  "jsonschema~=4.25",
  "py~=1.11.0",
  "pytest>=8.4,<10",
  "pytest-benchmark~=5.1",
  "pytest-timeout~=2.4",
  "requests",
+  "rpds-py<0.28; platform_python_implementation=='PyPy' and python_version<'3.11'",


Is there any way we could get by without having to install jsonschema and in particular rpds-py? Even with it just being a test dependency, it will make maintenance, especially testing new Pyhton versions before release, more difficult.

in particular rpds-py?

That's a hard dependency of jsonschema

Is there any way we could get by without having to install jsonschema

Not doing schema validation, which was the original proposal but on discord Pierrre was wary of drift / maintenance burden without an upstream source of truth.

In theory types could be generated and used for typecheking (or runtime validation), with the generated code checked / validated against the upstream schema once in a while, but from a quick browse of https://json-schema.org/tools there does not seem to be any maintained generator:

OpenAPI is archived

Statham is unmaintained, does not support recent python, and does not support features required by the sarif schema

thydux has completely disappeared from the internet

yacg is not a complete e2e project

cdce8p · 2025-12-08T15:13:06Z

tests/reporters/unittest_sarif_reporter.py

+    schema = pytestconfig.cache.get(CACHE_KEY, None)
+    if not schema:
+        try:
+            res = requests.get(SCHEMA_URL, timeout=5)
+        except requests.exceptions.RequestException as e:
+            raise pytest.skip(
+                f"Unable to retrieve schema v{SCHEMA_VERSION}: {e}"
+            ) from e
+
+        if res.status_code != 200:
+            raise pytest.skip(
+                f"Unable to retrieve schema v{SCHEMA_VERSION}: {res.reason}"
+            )
+        schema = res.text
+        pytestconfig.cache.set(CACHE_KEY, schema)


How often does the schema change? Is it necessary to download it every time or could we just vendor the current version. Any external download will inevitably slow down test runs.

Wouldn't it be enough to just check the output against a known good version? That would also help get rid of the jsonschema dependency.

How often does the schema change? Is it necessary to download it every time or could we just vendor the current version. Any external download will inevitably slow down test runs.

Rarely to never I would assume: the original specification was published in 2020, and when revisions were done in Errata 01 in 2023 that led to a new schema. So vendoring is not an issue as long as you're OK with a fairly large blob (the file is 115KB).

That would also help get rid of the jsonschema dependency.

It would not.

What if we create a repository specifically for the sarif reporter ? This way we do not slow down the pipeline in main pylint and we can add the reporter as a dependency anyway.

What if we create a repository specifically for the sarif reporter ? This way we do not slow down the pipeline in main pylint and we can add the reporter as a dependency anyway.

I think this would make sense. It's more complex than the current builtin ones while also being more niche than say the GithubReporter. Users who need it could just install it like any other pylint plugin.

I'm not sure it even needs to be added as a dependency, a reference in the docs and help output might be enough.

Pierre-Sassoulas added the Enhancement ✨ Improvement to a component label Nov 25, 2025

Pierre-Sassoulas added this to the 4.1.0 milestone Nov 25, 2025

Pierre-Sassoulas requested changes Nov 25, 2025

View reviewed changes

pylint/reporters/json_reporter.py Outdated Show resolved Hide resolved

tests/reporters/unittest_json_reporter.py Outdated Show resolved Hide resolved

pylint/reporters/sarif_types.py Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

xmo-odoo force-pushed the sarif branch from e21a200 to 6f1aef0 Compare November 25, 2025 14:30

This comment has been minimized.

Sign in to view

xmo-odoo force-pushed the sarif branch 3 times, most recently from d2110f6 to 940ce59 Compare November 25, 2025 15:09

This comment has been minimized.

Sign in to view

Pierre-Sassoulas reviewed Nov 25, 2025

View reviewed changes

pylint/reporters/sarif_reporter.py Show resolved Hide resolved

xmo-odoo force-pushed the sarif branch from 75230ad to 430ed1a Compare November 26, 2025 06:29

This comment has been minimized.

Sign in to view

xmo-odoo force-pushed the sarif branch 4 times, most recently from e64b059 to ebf822c Compare November 26, 2025 13:46

xmo-odoo requested a review from Pierre-Sassoulas November 27, 2025 08:06

Pierre-Sassoulas reviewed Nov 27, 2025

View reviewed changes

xmo-odoo force-pushed the sarif branch 4 times, most recently from 73f84df to b6aaa55 Compare December 8, 2025 09:49

xmo-odoo force-pushed the sarif branch from b6aaa55 to 0d6d936 Compare December 8, 2025 10:28

xmo-odoo force-pushed the sarif branch 2 times, most recently from 406280f to 164d694 Compare December 8, 2025 11:10

xmo-odoo force-pushed the sarif branch from 164d694 to 8878b8e Compare December 8, 2025 13:11

cdce8p reviewed Dec 8, 2025

View reviewed changes

Uh oh!

SARIF reporter #10759

Are you sure you want to change the base?

SARIF reporter #10759

Conversation

xmo-odoo commented Nov 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Spec v Github

On URIs

Uh oh!

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Nov 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

xmo-odoo commented Nov 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nvuillam commented Nov 25, 2025

Uh oh!

Uh oh!

This comment has been minimized.

github-actions bot commented Nov 26, 2025

Uh oh!

xmo-odoo commented Nov 27, 2025

Uh oh!

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Uh oh!

xmo-odoo commented Nov 27, 2025

Uh oh!

Pierre-Sassoulas commented Nov 27, 2025

Uh oh!

cdce8p Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

xmo-odoo Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cdce8p Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

xmo-odoo Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

Pierre-Sassoulas Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

cdce8p Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

xmo-odoo commented Nov 25, 2025 •

edited

Loading

codecov bot commented Nov 25, 2025 •

edited

Loading

xmo-odoo commented Nov 25, 2025 •

edited

Loading

xmo-odoo Dec 9, 2025 •

edited

Loading