Repository navigation
CONTRIBUTING.md's T-SQL conventions are unenforced: a COUNT() violation passed every gate, caught only by a review bot #3081
Description
CONTRIBUTING.md states nine T-SQL conventions as "All T-SQL code must follow", and a violation of one of them was merged-ready and green through every gate the repo has. The rule was caught by a review bot reading the diff, which is the only thing standing between the convention block and irrelevance.
The demonstration, which is experimental rather than inferred
PR #3078 introduced COUNT(*) and COUNT(DISTINCT owt.wait_type) in a new collector query. CONTRIBUTING.md:355 says:
Functions: Use
COUNT_BIG()notCOUNT(),ROWCOUNT_BIG()not@@ROWCOUNT
Both violations passed:
Darling Linux buildDarling PostgreSQL tests- all four whole-tree guards
- the 145-test command-deadline family
reviewandverify
Nothing objected. The lane fixed it on the bot's finding and added a local assertion for that one query, and said in its reply that it is a local guard rather than the repo-wide scanner the rule arguably wants. That local guard is the right scope for that PR and the wrong scope for the rule.
What is actually unenforced
Measured on dev:
ROWCOUNT_BIG: appears in zero test files across both test projects.COUNT_BIG: appears in fiveLite.Testsfiles, zero inDarling.Tests— and whatever those five do, they demonstrably do not scanPerformanceMonitor.Collectors, since the violation above lived there and passed.- No test file is named for a T-SQL style scan (nothing matching
Tsql/SqlStyle/SqlConvention/SqlLint/QueryStyle).
So the convention block is documentation with no mechanism, and the failure direction is the usual one: it fails toward not noticing, silently, on exactly the change that introduces a violation.
Why this is a design decision and not a chore
The nine conventions are not equally checkable against SQL held in C# string literals, and a scanner that tries to enforce all nine would be noisy enough to stop being read — which is the failure mode #3076 is currently weighing for a different guard.
Mechanically checkable with low false-positive risk:
COUNT(whereCOUNT_BIG(is meant, and@@ROWCOUNTwhereROWCOUNT_BIG()is meant--line comments, where the rule is block comments only- abbreviated data types —
intforinteger,nvarchar(MAX)fornvarchar(max) - tabs, where the rule is 4 spaces
Genuinely hard, and probably out of scope:
- UPPERCASE keywords and
ASon every table alias — both need enough parsing to avoid firing on prose inside a comment, on a column literally namedas_of, or on identifiers that contain a keyword column_name = expressionaliasing and trailing-comma placement — style shapes rather than tokenssysnamefor identifiers, which depends on what the column means
So the scope decision this issue carries is which subset gets a guard, and the honest answer may be four rules rather than nine. A guard covering the checkable subset and saying so is worth more than one that claims the block and quietly under-covers it — the latter is the shape that produced #3063, #3067 and #3076 in one night.
Requirements if it is built
- Floor the population. A scanner that opens no files, or extracts no SQL, must fail rather than report zero violations. This guard's entire failure direction is silent non-detection, so
files scanned > 0andliterals extracted > 0need to be assertions distinct fromviolations == 0. - State the covered subset in the failure message and in the doc comment, so the next reader knows which rules are guarded and which are on trust. Record the unguarded ones explicitly — the Fixes #3067 #3075 shape, where a known-answer fixture carries the uncovered case deliberately absent from the expected set, rather than leaving the limitation implied.
- Existing violations must be inventoried before the guard goes red. There are 103 files in
PerformanceMonitor.Collectorsalone; if the checkable subset has pre-existing violations, they need either fixing in the same change or an allow-list at set equality with a stated bound (theCommentFilterAdoptionTests/ Fixes #3067 #3075 shape), not a lowered bar. - Both SKUs. The violation landed in
PerformanceMonitor.Collectors, which both Darling and Lite build on, so a guard living in only one test project inherits the sibling gap CrossAppGuardCiGateTests anchors on Lite/, so the two Darling.Tests reads of Lite.Tests/ are invisible to it #3067 was filed for.
Provenance
Found by the #2880 lane after a review bot flagged the COUNT(*). The lane fixed the violation, added a local assertion, and flagged that nothing repo-wide enforces the rule — explicitly declining to overclaim its local guard as enforcement. Measurements above are independent of that lane's report.
Activity
erikdarlingdata commentedon Sep 6, 2026
Claude posting for Erik Darling
Correcting two counts in this issue's body — the conclusion stands, the numbers did not
The body says:
Both figures are wrong. Re-derived by reading all 757 test files on
devdirectly:Darling.TestsfilesLite.TestsfilesCOUNT_BIGROWCOUNT_BIGThe
ROWCOUNT_BIGzero holds.Why they were wrong: both came from GitHub's code-search API, which returned a false zero for
COUNT_BIGinDarling.Testsand undercountedLite.Testsby one. A code-search zero is not evidence of absence — its index does not cover this repo completely, demonstrated independently in the same session where it reported 0 hits forlast_error_aton a symbol that is present ondevin four files.The conclusion is unaffected, and here is why rather than an assertion that it is
The single
Darling.Testsoccurrence isDarlingCollectorRunnerTests.cs:397:That is a test fixture query that happens to follow the convention, not a test that enforces it. So nothing in either test project scans for the rule, and the load-bearing evidence for this issue was never the search anyway — it is experimental: PR #3078's
COUNT(*)andCOUNT(DISTINCT …)violations passed the Linux build, the PostgreSQL tests, all four whole-tree guards, the 145-test command-deadline family,reviewandverify, and were caught only by a review bot reading the diff.The part worth carrying into the fix
This is the same defect class the issue is about, committed by the issue itself: a count published from an instrument that fails silently toward zero. Two operational consequences for whoever builds the guard:
files scanned > 0andliterals extracted > 0must be their own assertions, because "zero violations" and "zero files examined" are indistinguishable from the outside — which is precisely what happened here.Fixes #3081 (#3085)
erikdarlingdata commentedon Sep 6, 2026
Claude posting for Erik Darling
Fixed by #3085, merged to
devasc05d029e40b. Closing by hand — a closing keyword does not fire on adev-targeted PR here.Four of the nine bullets are enforced —
COUNT_BIG()/ROWCOUNT_BIG(),--comments, tabs, and data-type case. The remaining five, plus the abbreviation half of the data-type bullet, are recorded with reasons and each carries a fixture whose expected finding set is empty with a positive control spliced through the same detector. The disposition map is compared for equality against the bullets parsed out ofCONTRIBUTING.md, so a tenth bullet cannot appear without the map disagreeing.Nine pre-existing violations were fixed, so there is no waiver list at all — three
COUNT(*), one--, five type-name casings. That is better than an allow-list: an empty waiver cannot go stale, cannot be mistaken for a bound, and cannot become the thing someone adds to. The deferred abbreviation half re-derives its 33-site cost every run, with failure text telling the next reader to enable the rule when it reaches zero — a countdown rather than an exemption.The finding that mattered most came from testing a sentence rather than writing it. The guard's own source asserted "nothing T-SQL could fall outside the population"; testing that instead of stating it found that
DarlingXeSessions' Extended Events provisioning DDL —CREATE/ALTER/DROP EVENT SESSION, sent to production servers on every session repair — was entirely unjudged.EVENT SESSIONis now a marker, so it is judged. That file needed no edit and is correctly absent from the diff: it carries zero bareCOUNT(, so there was nothing to fix. An unjudged population that happens to be compliant is still an unjudged population.Two markers added alongside it were then deleted.
sqlserver.andpackage0.came out on the measurement that every statement in the corpus carrying either also carriesEVENT SESSION— neither could be shown to do work. An unprovable marker is indistinguishable from a load-bearing one until measured, and it makes the population look better-justified than it is.The residue is pinned at set equality over the population's complement (500+ literals) with a vacuity floor, because an enumeration over an empty complement would pass while asserting nothing.
Not verified, and stated at the top of the PR's verification section rather than trailing it: no query in this diff was executed against a SQL Server, of any version, anywhere. Nine hand-edits to T-SQL that runs against monitored production, verified by inspection only. Worth adding for a future reader that the risk is narrower than that sentence sounds — every type change is a T-SQL synonym or a case change on a case-insensitive keyword, so identical by the language definition; the comment conversion is single-line to single-line and cannot swallow following content; and the three
COUNT_BIGconversions are identical in result for any count under 2³¹, against availability-group and log-file counts.Two figures in this issue's body were wrong and are corrected in a comment above:
COUNT_BIGappears in 1Darling.Testsfile and 6Lite.Testsfiles, not 0 and 5. They came from GitHub's code-search API, which returns false zeros on this repo.A side finding — the
int-for-integerremediation across 33 sites / 7 files, with ansp_executesqlparameter-list caveat needing a live server — is classified hygiene and raised as a task chip rather than an issue.