ci: gate on doc comments that bind to nothing - #849
Merged
Conversation
Kotlin and Swift attach a doc block to the declaration that FOLLOWS it, and keep only the LAST of several stacked ones. So inserting a block above an existing one — or inserting a declaration into the gap between a comment and the thing it documents — silently orphans the text. Nothing looks wrong on review: the prose is still there, still above the right function, still correct. It just no longer reaches Dokka or IDE hover. #846 fixed three sites, one of which #842 had introduced the same morning, and the block that came adrift was "Byte-parity twin of Swift classifyCoverage" — a parity-contract statement, and parity is the first rule in CLAUDE.md. That is the case for gating rather than just fixing. Tools/doc_comment_lint.py catches both detaching patterns (stacked blocks, and a blank line between a doc block and its declaration) across Kotlin and Swift, skipping file headers where a blank line before `package`/`import` is conventional. #846 also claimed the tree was clean afterwards. It was not: that sweep only matched SINGLE-LINE blocks. A scanner that handles multi-line blocks finds 25 more pre-existing sites across 13 files — 8 in WhoopBleClient.kt alone, and AppViewModel.kt:68 is a class doc detached by an inserted data class, the same shape as the #842 slip. Corrected on that PR. Those 25 are grandfathered per-file in a baseline so the gate starts green and blocks only NEW ones. Baselined by COUNT, not line number: line numbers churn on every edit above them, which would keep the file stale and train people to regenerate it without looking. A count moves only when someone adds or removes a site — the event worth gating on. It ratchets DOWN; fixing a site prints an IMPROVED line asking for the allowance to be lowered. Verified three ways: passes on clean main; catches all three pre-#846 sites when they are reconstructed; and fails both on a new site in an unbaselined file AND on an EXTRA site in a file that is already baselined, so the count is genuinely enforced rather than mere presence. Pure Python, ubuntu, seconds — no Xcode or Gradle, so it works despite app-build.yml and android.yml being disabled.
This was referenced Jul 27, 2026
ryanbr
added a commit
that referenced
this pull request
Jul 27, 2026
Tools/i18n_audit.py:34 was LANGS = ["de","es","fr","pt-PT"]. The Apple catalogs ship EIGHT non-English localizations, so the gate covered four of them — and the four it did not cover drifted exactly as you would expect: catalog de/es/fr/pt-PT it ru zh-Hans zh-Hant Strand (3255 keys) 100% 3086 3110 3136 3086 StrandDesign (95) 100% 14 41 20 14 NOOPWatch (52) 100% 49 50 49 49 NOOPWatchComplications (30) 100% 5 30 5 5 The gated four are at 100% everywhere. The ungated four are missing up to 85% — StrandDesign ships 14 of 95 Italian, the watch complications 5 of 30. That is the gate working where it is pointed and nothing happening where it is not. Android is the same: ANDROID_LOCALE_DIRS hardcoded the same four, values-zh existed on disk, was never compared, and sat 43 keys behind. That is how #834 passed CI while adding a key to four of five locale files (#844). Both lists are now DISCOVERED — Apple languages from the union of each catalog own localizations, Android dirs from values-* on disk — so a locale is covered the day it appears rather than the day someone remembers a constant. LANGS keeps ZERO tolerance and is untouched: those four are perfect precisely because they are hard-gated. The newly covered locales carry 1019 real pre-existing gaps and are gated against a ratcheting allowance instead; switching them on hard would red-check every open PR on lines nobody touched, the exact #514 failure this workflow own comments warn about. Counts rather than key lists, same reasoning as doc_comment_lint_baseline (#849) — a key list goes stale on every edit and trains people to regenerate it unread. Ratchets DOWN, printing IMPROVED when a count drops and STALE for an allowance whose target no longer exists. catalog_summary() had the same blind spot and is fixed too: it reported four of eight languages, so the human-readable coverage view read 0 missing across the board. That is very likely WHY this went unnoticed — anyone checking coverage was shown a clean sheet. It now doubles as the burn-down view for the 1019. Verified: green on main; fires when the values-zh gap grows 43->44; fires when one StrandDesign Italian string is removed; de still hard-fails on a single missing key, so zero tolerance is intact; a fabricated allowance target is reported STALE; and the locale section states its own verdict even when an earlier check failed (it keyed on the global failed flag, so a broken German string silenced it entirely). Translates nothing. It stops the hole growing and makes the debt countable. Whether it/ru/zh-* should ship at 14/95 or be withheld until filled is a product call, not a linter one.
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.
Follow-up to #846.
Why gate rather than just fix
Kotlin and Swift attach a doc block to the declaration that follows it, and keep only the last of several stacked ones. Insert a block above an existing one — or a declaration into the gap between a comment and the thing it documents — and the text is silently orphaned.
Nothing looks wrong on review. The prose is still there, still above the right function, still correct. It just no longer reaches Dokka or IDE hover. That is exactly why it survives.
#846 fixed three sites in one day, one of which #842 had introduced that same morning, and the block that came adrift was "Byte-parity twin of Swift
classifyCoverage" — a parity-contract statement, with parity being the first rule inCLAUDE.md. Not a cosmetic loss.Correcting #846
That PR claimed the tree was clean afterwards. It wasn't. My sweep only matched single-line blocks. A scanner handling multi-line blocks finds 25 more pre-existing sites across 13 files — 8 in
WhoopBleClient.ktalone.AppViewModel.kt:68is a class doc detached by an inserteddata class: the same shape as my #842 slip. Corrected on that PR too.The baseline, and why it's counts
Those 25 are grandfathered per-file so the gate starts green and blocks only new ones.
Keyed by count, not line number. Line numbers churn on every edit above them, so a line-keyed baseline goes stale constantly and trains people to regenerate it without looking. A count moves only when someone adds or removes a site — precisely the event worth gating on. It ratchets down: fixing a site prints an
IMPROVEDline asking for the allowance to be lowered.Verification
Three ways, because a gate that never fires is worthless:
3b is the one that matters — a presence-only baseline would let
WhoopBleClient.ktgrow from 8 to 80 unnoticed.Cost
Pure Python + git on ubuntu, seconds. No Xcode, no Gradle — so unlike the app-target and Android checks it actually runs, given
app-build.ymlandandroid.ymlare disabled. Same shape and triggers asi18n-coverage.yml, including auditing pushes tomainso a release landing without a PR is still covered.New workflow rather than a step in
i18n-coverage.yml: different concern, and that job is currently the only thing gating PRs — I'd rather not have a docs regression report itself as an i18n failure.Not included
Fixing the 25. Each needs a judgement call about how to merge the two blocks, and doing 25 of those at volume is how the mistake gets made in the first place. The baseline makes them visible and burnable-down without blocking anyone.