tools: export accepted findings reports as SARIF 2.1.0

[Stefano Demiliani (demiliani)]

Why

A BCQuality findings report is a project-specific JSON document. To show findings next to the code in a pull request, every CI integration currently has to write its own converter. SARIF 2.1.0 is the standard interchange format for static-analysis results. GitHub code scanning, Azure DevOps (SARIF SAST Scans Tab), and VS Code SARIF viewers consume it directly. With a supported converter, BCQuality can be used as a PR annotation tool with no custom code.

What

tools/ConvertTo-Sarif.ps1 -ReportPath <accepted report> [-OutputPath] [-SourcePathPrefix] [-KnowledgeBaseUrl]

BCQuality	SARIF
findings[].id	ruleId. Knowledge-backed rules get name = article slug, shortDescription = article title, and helpUri = article link. agent: findings get a rule without a help link.
severity	blocker/major → error, minor → warning, info → note
location (+ range)	physicalLocation relative to SRCROOT, with an optional -SourcePathPrefix for apps in a sub-folder
confidence, domain, references, from-sub-skill, suggested-code*	result.properties
outcome partial/failed (top level or any sub-result)	invocations[0].executionSuccessful = false + toolExecutionNotifications
skill, outcome, suppressed	run.properties

Design choices:

• Only top-level findings are converted. DO already copies accepted leaf findings into a super-skill's top-level findings[], so converting sub-results as well would duplicate them, and failed leaves' findings must not be published.
• An incomplete review never looks clean. A partial/failed report yields executionSuccessful: false with the outcome reasons.
• Presentation only. The converter validates the report schema but does not replace Validate-FindingsReport.ps1; the docs say to convert only accepted reports.
• No change to Entry, READ, DO, the findings-report schema, or any skill.

Tests and docs

• tools/Test-ReviewContract.ps1 gains offline assertions: rule ID, level mapping, help link, path prefix, range, preserved confidence, and the failed-outcome notification. The review-fixtures workflow already runs it through Test-ReviewFixtures.ps1, so no workflow change is needed.
• I also validated the output for a leaf report and a super-skill report with a failed sub-result against the official SARIF 2.1.0 JSON schema (schemastore draft-07 version). Both are valid.
• docs/standalone-runner.md: new section "Publish findings as SARIF"; linked from docs/README.md.

Validation

tools/Test-ReviewContract.ps1, tools/Test-ReviewFixtures.ps1, .github/scripts/Test-KnowledgeIndex.ps1: PASSED.

[Stefano Demiliani (demiliani)]

Hi Jesper Schulz-Wedde (@JesperSchulz), could you review this one, or route it to whoever owns the tooling? It only touches tools/ and docs/, which have no CODEOWNERS entry, so no reviewer was requested automatically.

In short: it adds tools/ConvertTo-Sarif.ps1, which turns an accepted findings report into SARIF 2.1.0. BCQuality findings can then show up as code scanning annotations in GitHub PRs or Azure DevOps without each integration writing its own converter.

It's presentation only: no change to Entry, READ, DO, the report schema, or any skill. Two choices worth a look:

• Only top-level findings are converted, so a super-skill's copied leaf findings aren't duplicated.
• A partial or failed outcome sets executionSuccessful: false with the reason, so an incomplete review never looks clean.

The output validates against the official SARIF 2.1.0 schema. Offline tests sit in Test-ReviewContract.ps1, which the review-fixtures workflow already runs.

Thanks!

[Jesper Schulz-Wedde (JesperSchulz)]

Reviewed 3e2f553047e1f3a51148dce867df15b2023463f6. The top-level-only export, severity mapping and incomplete-invocation handling are sound, but source locations are not losslessly converted to URIs.

tools/ConvertTo-Sarif.ps1:133-146 copies a filesystem path directly into artifactLocation.uri. A real source path such as src/Price #1.al passes Validate-FindingsReport with its authoritative SourceRoot and scope, but the converter emits that literal string. URI resolution then treats #1.al as a fragment and targets /src/Price rather than the actual file. Literal percent sequences and question marks have analogous problems on platforms where allowed. This silently breaks the core annotation/file binding even for a fully accepted report; JSON/SARIF structural validation does not catch it.

Encode each repository-relative path segment as URI data after combining SourcePathPrefix, preserving / separators and not turning percent/hash characters into escapes/fragments. Add regressions for spaces, #, literal %, non-ASCII paths and prefixed app folders, asserting decoded URI paths resolve to the original filenames with no fragment/query. Do not change the report's raw source path. Existing exact-head contract and integration tests otherwise pass.

[Stefano Demiliani (demiliani)]

Addressed Jesper Schulz-Wedde (@JesperSchulz)'s review in commit 8fa00b0.

• Encode every repository-relative source path segment as URI data after combining SourcePathPrefix, preserving / separators.
• Preserve the report's raw source paths unchanged.
• Add regressions using real files accepted by Validate-FindingsReport with authoritative SourceRoot and SourcePaths: spaces, #, literal %23/%25, non-ASCII filenames, and prefixed app folders. Also cover ? on platforms that allow it.
• Assert lossless URI decoding and resolution to the original filenames, with no fragment or query.
• Document that source paths and prefixes are raw filesystem paths, not pre-encoded URIs.

Validation: the new regression failed before the fix; pwsh -NoProfile -File .\tools\Test-ReviewContract.ps1 passes after the fix. git diff --check also passes.

[Jesper Schulz-Wedde (JesperSchulz)]

Revised — this approval is withdrawn; see my changes-requested review on the same head.

Reviewed 8fa00b0b75a6532d0abfef53b2908bfbb5e43983. The URI-binding blocker is resolved: the converter encodes each combined source/prefix path segment while preserving separators, leaves raw report paths untouched, and has end-to-end regressions for spaces, #, literal %, non-ASCII names and prefixes. Top-level-only conversion, severity mapping, and incomplete-outcome handling are sound.

However, findings without a location (allowed by the findings-report schema) produce SARIF results that GitHub code scanning rejects (details in the follow-up review).

[Jesper Schulz-Wedde (JesperSchulz)]

Requesting changes on 8fa00b0b75a6532d0abfef53b2908bfbb5e43983. This supersedes my earlier approval.

Merge-critical: one location-less finding makes the GitHub code scanning upload fail

findings[].location is optional in schemas/findings-report.schema.json, and DO describes it as optional too. For a finding without a location, ConvertTo-Sarif.ps1 emits a result that has no locations. That is valid SARIF 2.1.0, so the official schema check passes. GitHub code scanning, however, requires at least one location per result and rejects the whole upload (locationFromSarifResult: expected at least one location; see microsoft/sarif-sdk#2770 and GitHub's "SARIF support for code scanning" docs). A single repository-level or advisory finding therefore breaks the documented GitHub path, which is the main purpose of this PR.

Please handle location-less findings deterministically, without dropping them silently. Either:

• Leave them out of results and surface them elsewhere: for example, a toolExecutionNotifications entry naming each finding ID and its message, plus a count in run.properties.
• Or anchor them to a fallback artifact the caller provides, for example -FallbackLocation app.json. Never invent one.

Then document the behavior in docs/standalone-runner.md. Add a regression with a location-less finding that asserts:

• every emitted result has at least one location with a non-empty artifactLocation.uri, and
• the finding is still visible in the SARIF log.

Everything else is sound: the per-segment URI encoding fix and its regressions, top-level-only conversion, the severity mapping, and the executionSuccessful handling. The executionSuccessful handling relies on DO deriving a partial/failed super-skill outcome from failed leaves.

[Stefano Demiliani (demiliani)]

Addressed Jesper Schulz-Wedde (@JesperSchulz)'s latest review in commit 10169cf using the notification approach.

• Findings without a source location are emitted as invocations[].toolExecutionNotifications rather than results, so all emitted results have source locations.
• Each notification includes the finding ID and full message, associatedRule, the existing severity mapping, and the finding metadata in properties.
• run.properties.locationlessFindingCount records the count, including zero. No fallback file is invented and no finding is silently discarded.
• Documented the behavior, including reports containing only location-less findings and the unchanged outcome-based executionSuccessful semantics.

Regression coverage includes accepted reports with mixed located/location-less findings and location-less-only findings, all four severities, non-empty artifact URIs on every emitted result, notification visibility and metadata, and coexistence with partial-outcome notifications.

Validation: the new regression failed before the fix; pwsh -NoProfile -File .\tools\Test-ReviewContract.ps1 now passes, including the existing URI-encoding regressions. git diff --check passes. No actual GitHub code-scanning upload was performed.