Repository navigation
Run CodeQL's security queries, not security-and-quality - #8
Merged
Merged
Conversation
Code scanning posts its findings as pull request review comments, and the ruleset on main sets required_review_thread_resolution, so every PR arrived with the merge button already blocked by whatever the quality suite had to say. #7 carried eleven such threads. Three were real and fixed; the other eight were deliberate code or false positives and had to be dismissed by hand: - three "call to obsolete method" on calls inside a #pragma warning disable CS0618 whose entire purpose is to exercise the obsolete forwarder - two "useless upcast" where the cast is load-bearing, because Task.FromResult infers the wrong tuple type without it - one "missed opportunity to use Where" - one "generic catch clause" on the observer catch that is documented as logged-and-swallowed by design - one "string concatenation in loop" in a test helper That is eight dismissals per PR as the steady state, which trains everyone to click through the one list that is supposed to be read. This narrows what runs, not what is reported, so it is a real loss and worth naming: the bare-catch bug fixed in 0f2ef53 — a write replacing an undecryptable secret with its default — was found by a quality query, not a security one. Against that, the quality rules are the analyser's weakest signal here, the compiler already enforces more than they do (warnings-as-errors, AnalysisLevel latest-recommended, and CA rules that rejected a return type during this very change), and the friction landed on every PR rather than occasionally. If the quality queries are wanted back, the better shape is to keep them and stop them blocking: a codeql-config with query-filters excluding the specific noisy rules, or paths-ignore for tests/, which is where seven of the eight sat. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
190abec passed `queries: security`, which fails init outright: A fatal error occurred: Query pack security cannot be found. Check the spelling of the pack. There is no pack by that name. The named suites are security-extended, security-and-quality and security-experimental; the security-only suite is CodeQL's default and is selected by passing no `queries` input at all. A wrong name is not ignored and does not fall back — unlike the minimumCoverage input two commits earlier, which GitHub dropped with a warning. Here the whole job failed in 22 seconds, which is the better behaviour and is how this was caught. security-extended would also have excluded the quality rules, but it adds lower-precision security queries, which is new noise in place of the noise being removed. The default suite is the one that matches the intent. Co-Authored-By: Claude Opus 5 <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
1 participant
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.
Code scanning posts findings as PR review comments, and the ruleset on
mainsetsrequired_review_thread_resolution: true. The result is that every pull request arrives with themerge button already blocked by whatever CodeQL's quality suite has to say about the diff.
#7 carried 11 such threads. Three were real and got fixed. The other eight
were deliberate code or false positives and had to be dismissed by hand:
#pragma warning disable CS0618whose whole purpose is to exercise the obsolete forwarderTask.FromResultinfers the wrong tuple type without itWhereEight dismissals per PR as the steady state trains everyone to click through the one list that is
meant to be read.
This is a real loss, not a free win
Narrowing the suite changes what runs, not what is reported. The bare-catch bug fixed in
0f2ef53— a write silently replacing an undecryptable secret with its compile-time default — was found
by a quality query, not a security one. That is worth stating plainly rather than burying.
The trade accepted here:
TreatWarningsAsErrors,AnalysisLevel latest-recommended, and CA rules that rejected a return type during the work on Close the API gaps: write observers, discovery endpoints, projectors #7.If the quality queries should come back
The better shape is to keep them and stop them blocking, rather than stop running them:
codeql-config.ymlwithquery-filtersexcluding the specific noisy rules, orpaths-ignorefortests/, which is where seven of the eight sat.Happy to do either instead of this — say so and I'll swap it.
Verification
This PR is its own test: with
queries: security, it should produce no blocking quality threads,so the merge button should be available on a green build without any manual dismissals.
🤖 Generated with Claude Code