Skip to content

ci: strip generated-code results from CodeQL SARIF before upload - #457

Merged
twcclegg merged 3 commits into
mainfrom
ci/codeql-silence-generated-code-noise
Sep 8, 2026
Merged

twcclegg merged 3 commits into
mainfrom
ci/codeql-silence-generated-code-noise

Conversation

@twcclegg

@twcclegg twcclegg commented Sep 5, 2026

Copy link
Copy Markdown
Owner

Problem

157 of 178 open code-scanning alerts were CodeQL hits inside source-generator output (RegexGenerator.g.cs, JSON-serialization contexts) under obj/**/generated/** — not hand-written code. These files never exist on disk (build-mode: manual extracts them from the Roslyn compilation), so the config-file paths-ignore filter doesn't reach them, and two rules were being suppressed repo-wide just to silence the noise.

Change

analyze now writes SARIF without uploading; .github/codeql/filter-sarif.sh drops any result whose primary location is under an obj/ path segment; upload-sarif publishes the filtered file under the same category. A result that only references generated code via relatedLocations/codeFlows is kept, since that's a real finding in hand-written code. The two repo-wide rule suppressions in codeql-config.yml are removed now that generated noise is filtered instead of rules being disabled.

Outcome

Verification run: 303 results in → 289 dropped → 14 uploaded. Open alerts: 11, none under obj/. cs/coupled-types still correctly flags the one hand-written hit, on PhoneNumberMatcher.cs:43, that the old rule suppression would have hidden.

The csharp job runs CodeQL with build-mode: manual, so CodeQL extracts
everything the real `dotnet build` compiles. That includes
source-generator output - RegexGenerator.g.cs from
System.Text.RegularExpressions.Generator and the
System.Text.Json.SourceGeneration *.g.cs files - which accounted for 156
of the repository's 167 open CodeQL alerts, all of them style findings
in code nobody wrote or can edit. Those files cannot be dropped from the
build, since the partial classes they emit back real [GeneratedRegex]
and JsonSerializerContext declarations the library depends on.

A config-file paths-ignore of "**/obj/**" does not suppress them. That
was tested rather than assumed: actions run 33946304824 carried the
paths-ignore entry and CodeQL still emitted all 170 results, the same
count as a run without it, matching GitHub's documented behaviour that
the filter applies to interpreted languages and build-mode: none but not
to manual/autobuild.

So the analyze step now writes SARIF instead of uploading it, a filter
step drops every result whose primary location is under obj/, and
upload-sarif publishes the result. Filtering by path rather than by rule
means no query is disabled: cs/useless-cast-to-self,
cs/nested-if-statements and cs/coupled-types keep reporting normally
against hand-written code, and the one real cs/coupled-types finding in
PhoneNumberMatcher.cs is still surfaced.

A result is dropped only when its own primary location is generated
code. Findings in hand-written code that merely reference generated code
through relatedLocations or codeFlows are kept, and the run's tool,
automationDetails, versionControlProvenance, artifacts and properties
are passed through untouched so fingerprints and analysis matching keep
working.

Because the noise is now handled by path, the two query-filters this
file used to carry are no longer needed and are removed:
cs/unused-label and cs/useless-assignment-to-local are enabled again.
Verified as a free win - re-enabling them added 133 results (119 and 14
respectively), every one of them under obj/ and filtered out, leaving
the published alert set unchanged.

Verified end to end against the branch: 11 hand-written CodeQL alerts
reported, 0 under obj/, down from 156.

The filter is a small stdlib-only script rather than
advanced-security/filter-sarif, which would add a third-party action and
whose released tags lag its fixes; it also drops a result when any
location matches instead of the primary one, mutates the surviving
locations array, and has an open TODO about not resolving
artifactLocation.index.
@codecov

codecov Bot commented Sep 5, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.69%. Comparing base (8ad7fa2) to head (d0d2dce).
⚠️ Report is 25 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #457      +/-   ##
==========================================
+ Coverage   87.51%   87.69%   +0.17%     
==========================================
  Files          43       43              
  Lines        3886     3893       +7     
  Branches      991      993       +2     
==========================================
+ Hits         3401     3414      +13     
+ Misses        280      277       -3     
+ Partials      205      202       -3     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

AGENTS.md is explicit that new CI/build tooling belongs in bash (simple
text/JSON-via-jq logic) or a small C# console tool, not a third scripting
language - a rule written after this repo had to walk back a batch of
incidental lib/*.js CI helpers once already. filter-sarif.py's actual job
(drop SARIF results whose primary location has an "obj" path segment,
including index-based artifact references, and report drop counts) fits
comfortably in jq once it isn't trying to support arbitrary glob syntax
for a caller that only ever passes '**/obj/**'.

filter-sarif.sh matches an exact "<dir>/" path segment via --exclude-dir
instead of implementing a glob engine, since that's the one thing this has
ever needed to express. It keeps the primary-location-only semantics
(locations[0]; relatedLocations/codeFlows references to generated code are
not treated as generated), the index-based artifactLocation resolution
fallback, and the drop-count-by-rule summary. It also adds a guard the
Python version didn't have: if a filter run would drop every remaining
result, it fails loudly instead of silently uploading an empty SARIF -
the scenario where a future --exclude-dir ends up matching too broadly
should show up as a red CI check, not as a clean security scan.

Verified against a hand-built SARIF fixture covering: a direct-URI
generated-code result (dropped), a hand-written result (kept), an
index-based artifactLocation pointing at generated code (dropped), a
result whose primary location is real code but whose relatedLocations
references generated code (kept - correctly not treated as generated),
a path with an "objects" directory that isn't an exact "obj" segment
(kept), an all-dropped run (exits 1), and a zero-result run (exits 0).
@twcclegg
twcclegg force-pushed the ci/codeql-silence-generated-code-noise branch from e05f23a to 0073ff3 Compare September 5, 2026 19:38
codeql.yml passes the same path as --input and --output, and the shell
truncates a redirection target before exec'ing the command on the right of
it. jq therefore read an empty file and wrote an empty one, and the upload
step failed with "Invalid SARIF. JSON syntax error: Unexpected end of JSON
input" while the script itself exited 0 reporting 289 dropped and 13 kept.

Write to a mktemp scratch file and move it into place, so an in-place run is
supported rather than silently destructive.

Also replace the empty-result guard. It was computed from the stats pass, so
it could not have caught the truncation it was there to prevent, and its
"every result was dropped" condition is the goal state rather than a fault:
once the last hand-written alert is fixed, every surviving result lives in
generated code and the build would have gone red. Verify the file that was
actually written instead - that it parses, and that it holds the number of
results the stats pass predicted.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants