Quiet known-noise CodeQL alerts without losing coverage - #807
Conversation
Three sources produced 58 of the repo's 66 open CodeQL alerts, none of them defects: * py/unused-import fired in every generated Python service. Take the path the config file itself prescribed for this case: strip generated Python in the SARIF filter, with a lookahead sparing the hand-written services/_base.py and _async_base.py that paths-ignore cannot express. * python/tests/ produced empty-except/unreachable/unused-local lint and two false-positive URL-sanitization highs. Ignore it alongside the other test infrastructure (conformance/). * py/ineffectual-statement flags the `...` bodies of Protocol and overload stubs in auth.py and async_auth.py — idiomatic typing code, and every future Protocol method would add another. Exclude the query.
github-actions
Bot
added
the
github-actions
Sensitive Change Detection (shadow mode)This PR modifies control-plane files:
|
There was a problem hiding this comment.
Pull request overview
Reduces recurring CodeQL noise from generated Python, tests, and typing stubs.
Changes:
- Extends SARIF filtering to generated Python.
- Ignores Python tests.
- Excludes
py/ineffectual-statement.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
.github/workflows/codeql.yml |
Filters generated Python SARIF findings. |
.github/codeql/codeql-config.yml |
Adds Python test and query exclusions. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| - exclude: | ||
| id: py/ineffectual-statement |
There was a problem hiding this comment.
Not doing this — the repo-wide reach is real, but the false-positive axis is Protocol/overload stubs, not these two files. A path-scoped SARIF clause reopens this thread the first time a Protocol lands in a new file, and inline suppressions need the alert-suppression query pack plus eight annotations on idiomatic lines. What the query protects after this change is the small hand-written Python surface (auth plumbing and services/_*), fully typed, where its output to date is 8/8 false positives — same trade the config already documents for java/class-name-matches-super-class. Leaving unresolved for a human call.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc05b2e538
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # that this query flags as a defect. auth.py and async_auth.py alone produced | ||
| # eight such alerts, and every future Protocol method adds another. | ||
| - exclude: | ||
| id: py/ineffectual-statement |
There was a problem hiding this comment.
When an ordinary hand-written Python module contains a genuinely ineffectual expression, this rule will no longer report it because query-filters excludes the query by ID from the entire suite, not merely from the eight Protocol declarations. GitHub's query-filters documentation describes these filters as selecting queries by metadata and provides no file or line scope. Keep py/ineffectual-statement enabled and suppress or post-filter only the known stub locations so future runtime defects remain detectable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not doing this — you are right that query-filters has no file scope, and that is the point of contention: the false-positive axis is Protocol/overload stubs anywhere, not the two files currently affected, so a location-scoped SARIF post-filter goes stale on the next Protocol and inline suppressions cost a query-pack change plus annotations on idiomatic code. On the remaining hand-written Python surface this query has produced only false positives, so the repo-wide exclusion follows the config's existing precedent (java/class-name-matches-super-class). Leaving unresolved for a human call.
Review caught that test-sarif-filter.sh embedded its own copy of the filter regex — still the old one — so it kept passing without exercising the new generated-Python exception. Updating the copy would fix this round and leave the next edit to drift the same way. Extract the program to .github/codeql/sarif-filter.jq and point both the workflow (jq -f) and the test at it, so the test always runs exactly what the workflow runs. Add fixture cases for the Python paths: generated services and models are dropped, hand-written services/_base.py and _async_base.py are kept. Verified the test fails when the lookahead is removed.
The security-alert cleanup sweep found 58 of this repo's 66 open CodeQL alerts came from three noise sources, none of them defects. This mutes each at the source rather than by perpetual dismissal:
py/unused-import, all ingenerated/services/): handled exactly as the config file's own comment prescribes — the SARIF filter now stripspython/src/basecamp/generated/with a(?!services/_)lookahead, so the hand-writtenservices/_base.pyandservices/_async_base.pystay scanned. Regex verified against representative paths (generated files stripped, the two_files andauth.pyuntouched).python/tests/(26 alerts: empty excepts, unreachable statements, unused locals, plus two false-positivepy/incomplete-url-substring-sanitizationhighs on\"https://3.basecamp.com\" in urlassertions): added topaths-ignoreunder the existing test-infrastructure entries.py/ineffectual-statement(8 alerts): every hit is a...Protocol/overload stub body inauth.py/async_auth.py— idiomatic typing code the query mistakes for a defect, growing with every future Protocol method. Excluded viaquery-filters, mirroring the existingjava/class-name-matches-super-classprecedent.The existing open alerts in these categories were already dismissed with matching reasons; this change keeps them from coming back. The remaining 8 open alerts (Scorecard process findings) are untouched.
Summary by cubic
Quiets 58 known-noise CodeQL alerts without losing coverage of the hand-written Python service bases. Previously, generated Python, tests, and the
py/ineffectual-statementquery reopened false positives; now they are filtered or excluded..github/codeql/sarif-filter.jqand used by the workflow; the test now runs the same program. Dropspython/src/basecamp/generated/results exceptgenerated/services/_via a lookahead so_base.pyand_async_base.pystay scanned; fixture asserts both are kept.python/tests/topaths-ignore.py/ineffectual-statementtoquery-filtersto avoid Protocol/overload...stub false positives.Written for commit 2a41aab. Summary will update on new commits.