tooling: doc_comment_lint keys the baseline on OS-native paths, so it fails on a clean tree on Windows by Zebsi235 · Pull Request #1691 · ryanbr/noop

Zebsi235 · 2026-08-27T12:39:16Z

What this PR does

One line, found by trying to follow the "run doc_comment_lint locally" guidance before opening #1685.

Tools/doc_comment_lint.py builds the key it looks up in the baseline with

rel = str(path.relative_to(ROOT))

str() on a path gives OS-native separators. Tools/doc_comment_lint_baseline.txt is written with forward slashes, so on Windows every lookup misses. Two consequences, both on a completely clean checkout:

the gate exits 1 with FAIL 25 detached doc comment(s) beyond the baseline — every grandfathered site counted as a new regression;
simultaneously, all 13 baselined files print as IMPROVED n -> 0. Lower it in doc_comment_lint_baseline.txt — advice to shrink counts that have not changed.

The two messages point opposite directions, and the natural conclusion is that the baseline is stale and should be regenerated, which the script's own comment forbids. CI runs the gate on ubuntu, so it stays green there; the failure is only visible to a Windows contributor running it locally, who then goes hunting for 25 regressions they did not cause.

The fix:

rel = path.relative_to(ROOT).as_posix()

as_posix() forces forward slashes and is a no-op on POSIX hosts, where relative_to already yields them.

Type of change

How it was tested

Not on the BLE path; no app code touched.

On Windows 11 / Python 3.11, against a clean checkout of main at 35182be:

before: exit 1, 25 false regressions listed, 13 false IMPROVED lines — on an unmodified tree;
after: OK no NEW detached doc comments (1819 files, 25 baselined site(s) remaining), exit 0.

The baselined count is unchanged at 25, which is the check that the fix restores the intended lookup rather than hiding anything.

Not re-run on Linux or macOS: as_posix() is a no-op there since relative_to already yields forward slashes, and source-hygiene.yml runs the gate on ubuntu on this PR anyway — but that is reasoning plus CI, not a local run, so saying it rather than claiming it.

Checklist

Swift package tests pass for any package I touched — n/a, no packages touched
Android unit tests pass if I touched android/ — n/a
No new build warnings introduced
UI changes use only StrandDesign tokens — n/a
No hardcoded hex frame bytes — n/a
Follows the conventions in docs/CONTRIBUTING.md
I did not commit generated output or any secrets/keystores

Related issues

None found for this. Surfaced while preparing #1685, whose "doc_comment_lint passes" claim was only locally checkable on Windows because of this fix.

…ses a clean tree on Windows doc_comment_lint builds its baseline lookup key with str(path.relative_to(ROOT)), which yields OS-native separators. The baseline file is written with forward slashes, so on Windows every lookup misses and, on a completely unmodified checkout, the gate exits 1 with all 25 grandfathered sites counted as new regressions - while simultaneously reporting every baselined file as "IMPROVED n -> 0", advice that points at regenerating the baseline, which the script's own comment forbids. CI runs on Linux, so it stays green there and the failure is only visible to a Windows contributor trying to follow the "run it locally" guidance. as_posix() forces forward slashes and is a no-op on POSIX hosts. Verified on Windows 11 / Python 3.11 against a clean checkout: before, exit 1 with 25 false regressions and 13 false IMPROVED lines; after, "OK no NEW detached doc comments (1819 files, 25 baselined site(s) remaining)", exit 0. The baselined count is unchanged, which is the check that the fix restores the intended lookup rather than hiding anything.

Zebsi235 · 2026-08-27T12:47:11Z

The linux-capture failure is pre-existing on main, not from this change. Chased it down:

The failing test is test_home_i18n.test_apple_home_semantic_display_contracts, which pins the literal String(localized: "Skin temperature +\(temperature) °C") in AppModel (Tools/test_home_i18n.py:508).
240c48a (ui: show skin temperature in the reader's unit on the alert banner and the fusion row #1671) changed that line yesterday to String(localized: "Skin temperature \(temperature)") and the test kept the old literal.
It fails identically on a clean checkout of main at 35182be with this PR's change stashed. This PR touches only doc_comment_lint.py, which no test imports, and the doc-comments job itself passed.

Why it surfaced here rather than on #1671: tools-python.yml is path-filtered to Tools/**, and #1671 touched only Swift, so the suite never ran on the commit that broke it. This is the first PR to touch Tools/ since, so the workflow fired and caught a two-day-old break. Which is the failure mode the workflow's own header describes, one level up: this contract test's subject lives in files that do not trigger the workflow that runs it.

Happy to update the assertion in this PR if you want it here, but it read as a separate concern: the new literal presumably wants to pin the reader's-unit form from #1671, and what that contract should say seems like #1671's call rather than mine to guess.

240c48a changed AppModel's illness-signal label from "Skin temperature +\(temperature) °C" to "Skin temperature \(temperature)" (the sign and unit now come from UnitFormatter.skinTempSignalPhrase in the reader's unit) and renamed the catalog key to match, but the home-i18n contract test kept both old literals. tools-python.yml is path-filtered to Tools/**, so the suite never ran on that commit and the break sat invisible until the next PR to touch Tools/ surfaced it. Both pins updated to the current contract: the assertIn literal and the catalog-key list. No behaviour change anywhere outside the test.

Zebsi235 · 2026-08-27T12:53:07Z

Went ahead and pushed the assertion update here after all, as its own commit (b4a04f0) so it is trivial to drop if you would rather take it with #1671's concern.

What it pins: the assertIn literal becomes String(localized: "Skin temperature \(temperature)"), matching AppModel after 240c48a, and the catalog-key list in the same test moves from "Skin temperature +%@ °C" to "Skin temperature %@", which #1671 already translated in the xcstrings for all languages. Both were pinning the pre-#1671 contract; nothing outside the test changes.

Verified: test_home_i18n runs 29 tests, OK. Full Tools/ discover on this branch runs 108 with one remaining error that is Windows-local only and pre-existing: test_appchangelog_gen.test_that_key_really_is_in_strings_xml calls read_text() with no encoding=, which decodes as cp1252 on Windows and dies on the UTF-8 strings.xml. Your ubuntu run does not hit it (the CI log shows 108 ran, one failure, and it was only the home-i18n one). Same class of bug as the path fix this PR is about, but a third concern, so leaving it alone unless you want it.

ryanbr · 2026-08-28T06:05:22Z

Thanks @Zebsi235 — both changes are right, and the second one is repairing a break of mine.

The path bug is real and the fix is minimal. main does use str(path.relative_to(ROOT)) at line
133, the baseline is forward-slashed, and as_posix() is the correct one-liner. I verified it is a
genuine no-op on POSIX rather than something that hides findings: on this branch the gate still reports
25 baselined site(s) remaining, the same count as before.

Your description of the failure mode is worth keeping in the comment, and it is: two messages pointing
opposite ways, with the natural conclusion being "regenerate the baseline", which the script forbids. A
contributor who follows that instinct makes it worse.

The second file is my fault, not scope creep

Tools/test_home_i18n.py fails on current main:

main:      Ran 108 tests — FAILED (failures=1)
this PR:   Ran 108 tests — OK

The assertion it updates was broken by #1671, which moved the sign and the hardcoded °C out of that
localized literal and into UnitFormatter.skinTempSignalPhrase. That was my change, and I did not notice
because tools-python.yml is path-filtered to Tools/**.py and Tools/linux-capture/** — my commit
touched Swift, so the workflow never ran.

So normally I would ask for one concern per PR. Not here: touching Tools/ is what made the workflow run,
which surfaced my latent breakage and would have shown up as a red check on your PR through no fault of
yours. Fixing it to get green is the right call, and your commit message attributes it correctly.

The follow-up this exposes

test_home_i18n.py asserts on the contents of files it does not live near:

Strand/App/AppModel.swift
Strand/Liquid/LiquidTodayView.swift
Strand/Resources/Localizable.xcstrings
Packages/StrandAnalytics/Sources/StrandAnalytics/ReadinessEngine.swift
Packages/StrandDesign/Sources/StrandDesign/DayNavBar.swift
…

None of those paths trigger the workflow. So any Swift change to a file this test reads can break it
silently and stay broken until someone happens to touch Tools/ — which is exactly the sequence that
just played out. That is a gap in the trigger, not in your PR, and I will take it separately rather than
grow this one.

Merging as is. Good find, and thanks for repairing the collateral rather than working around it.

@Zebsi235

…s on a clean tree on Windows (#1747) Every baseline in Tools/ stores forward slashes, and six sites keyed their lookups on str(path.relative_to(ROOT)) - OS-native, so on Windows every lookup missed. On a clean checkout that reported 236 + 63 NEW literals and eleven failing locale allowances, while the same run printed STALE for the very entries it was failing against. Two messages pointing opposite ways, and the natural conclusion - the baselines are stale, regenerate them - would have wiped the ratchet. CI runs on ubuntu, so only a contributor running the repo's own gate locally on Windows ever saw it, and then went hunting 299 regressions they did not cause. Verified rather than taken on description. The baseline really is forward-slashed (sampled: android/app/src/main/java/com/noop/ui/AppRoot.kt). The change really is a no-op on POSIX: the audit run on main and on this branch in place produces byte-identical output, exit 0 both, and the tool's own suite passes. The six converted sites are exactly the behaviour-affecting ones. All thirteen relative_to(ROOT) sites were classified: the six changed build lookup or dict keys, and the seven untouched are pure print() - including 1227's `line`, which only feeds the human-readable summary. A principled split, not a partial job. Those seven will still render backslashes on Windows, which is cosmetic and worth folding in later. Worth doing next, and it is the more valuable half: this is the SECOND tool with this exact bug (doc_comment_lint was #1691), and the fix is six inlined as_posix() calls. Six sites is six chances for a seventh to be added wrong, which is how it happened twice. A one-line helper - rel(p) returning p.relative_to(ROOT).as_posix() - collapses them to one, makes the rule impossible for the next reader to miss, and makes it TESTABLE, which it is not today because the conversion is inlined at every call site. Neither this PR nor #1691 carries a regression test, and there is currently nothing to assert against. Merged as-is because the fix is correct and a Windows contributor is blocked by it today; the helper is a follow-up rather than a condition. Thanks @Zebsi235 - the before/after on a clean tree, and stating plainly that Linux and macOS were reasoned about rather than run, are both exactly right.

…d filter only the leg that does not `tools-python.yml` carried a `Tools/**` path filter, so a change touching no Python never ran it. Its own header already argued against that, in detail: the core Tools/ suites assert on the CONTENTS of app and package files, `test_home_i18n.py` reads TodayScreen.kt, AppModel.swift and the xcstrings catalogues, and a Swift-only commit had already broken it once while the filter kept the job from running, leaving main red until an unrelated contributor touching Tools/ tripped over it in their own PR (#1691). The filter was reinstated underneath that paragraph anyway, for a reason that was sound on its own terms: a Windows runner billed at twice a Linux minute should not run on the many pull requests that touch no Python. It then cost the same thing a second time. On 2026-09-24 an Android-only commit changed the hero label padding that `test_home_i18n.py` asserts on, this job did not run, and main carried a red suite through nine merges until a commit that happened to touch Tools/ finally tripped it. One of those nine was an outside contributor's. So the two legs are split rather than the argument had a third time: tools-python.yml UNFILTERED. Runs the Tools/linux-capture decoders AND the core Tools/ suites, and the core suites read product source, so no path list can describe its inputs. Ubuntu, about 25 seconds, every PR and every push. tools-python-windows.yml FILTERED on Tools/linux-capture/**. Runs ONLY the linux-capture tests, a second time under legacy Windows console encodings, and those read nothing outside their own package. The cost argument is true here and is answered here. Job names are unchanged, `linux-capture` and `windows-capture`, so the check names are too. `test_core_tools_filter_covers_every_governance_tool_path` pinned the old filter exactly, so it is repinned to the new shape rather than dropped: the core workflow must carry NO path filter and must still run the core Tools/ step, and the Windows workflow must carry exactly the linux-capture filter and must NOT run the core step. Reinstating `Tools/**` on the core leg now reds it, which is what the last two rounds of this lacked. Parity governance is left alone. Its filter is deliberate, there is a test asserting it excludes product globs, and it already carries a daily schedule on main for exactly the staleness a filter cannot see. Three documents described this and two of them were already describing the state this restores, not the state that shipped. CONTRIBUTING.md's check table said `linux-capture` runs on "every PR", and docs/CONTRIBUTING.md said `source-hygiene`, `tools-python` and `i18n-coverage` "have no path filter, so all three run on everything". Both were false under the filter: contributors were told a gate covered them while it was silent. They are true again now. AGENTS.md was the one that was accurate, "active, path-filtered", and it is updated, along with a row for the new Windows workflow that its table had never carried. CONTRIBUTING.md's table gains the `windows-capture` check it had never listed either. Core Tools suite 130 tests, no failures. Governance acceptance 49 tests, no failures.

…d filter only the leg that does not (#2451) `tools-python.yml` carried a `Tools/**` path filter, so a change touching no Python never ran it. The core Tools/ suites assert on the CONTENTS of app and package files: `test_home_i18n.py` reads TodayScreen.kt, AppModel.swift and the xcstrings catalogues. A path list naming Tools/ therefore described the job's location rather than its inputs. Four separate statements of intent said otherwise, and none of them was enforced: * the workflow's own header, at length, including the incident that prompted it (#1691) * CONTRIBUTING.md's check table, "linux-capture ... every PR" * docs/CONTRIBUTING.md, "source-hygiene, tools-python and i18n-coverage have no path filter, so all three run on everything" * the sibling parity workflow's daily schedule, which exists because a filter cannot see staleness that product changes cause The filter was reinstated under the first of those for a reason sound on its own terms: a Windows runner billed at twice a Linux minute should not run on the many pull requests that touch no Python. The remedy was not, because it silenced the Linux leg too, and the Linux leg is the one that reads product source. It then cost the same thing a second time. On 2026-09-24 an Android-only commit changed the hero label padding `test_home_i18n.py` asserts on, this job did not run, and main carried a red suite through nine merges until a commit that happened to touch Tools/ finally tripped it. One of those nine was an outside contributor's. `paths` is workflow-level, not job-level, so one filter was serving two jobs with different inputs. They are now two workflows: tools-python.yml UNFILTERED. The linux-capture decoders AND the core Tools/ suites. Ubuntu, about 25 seconds, every PR and every push to main. tools-python-windows.yml FILTERED to Tools/linux-capture/**. Only those tests, a second time under legacy Windows console encodings; they read nothing outside their own package, so the cost argument is true here and answered here. Job names are unchanged, `linux-capture` and `windows-capture`, so the check names are too. `test_core_tools_filter_covers_every_governance_tool_path` pinned the old filter exactly, so it is repinned rather than dropped: the core workflow must carry NO path filter and must still run the core Tools/ step, and the Windows workflow must carry exactly the linux-capture filter and must NOT run the core step. Reinstating `Tools/**` on the core leg now reds it, verified by doing so. That is what the previous two rounds lacked: the intent was written down four times and enforced nowhere. The two contributor docs are true again with no edit. AGENTS.md was the accurate one, "active, path-filtered", and is updated, with a row for the new workflow its table never carried; CONTRIBUTING.md's table gains the `windows-capture` check it never listed. Parity governance is left alone. Its filter is deliberate, a test asserts it excludes product globs, and its daily schedule already covers the staleness a filter cannot see. Core Tools suite 130 tests, governance acceptance 49 tests, no failures. On this PR both changed workflows self-tested, and the wider claim is not demonstrated here: this branch touches Tools/, so the old filter would have run the job anyway. The next PR that touches only android/ or Strand/ is where it shows.

`Tools/tests/test_parity_governance_acceptance.py` does not test the scanner. It asserts that the CHECKED-IN authority and baseline still reproduce from the CURRENT product source, so a product change is what invalidates it. It ran only in parity-governance.yml, whose filter names `Tools/parity_*`. So a change could move the declaration and call-site graph the ledger measures without the gate that measures it ever running. That is not hypothetical. Earlier today #2557 took the only production call to `primarySessionRestingHR` on both platforms, turning it test-only, and the two resulting findings went unrecorded. Seventeen green checks, none of them this one. `main` carried a red ratchet until #2581, and it surfaced on #2577, an outside contributor's PR that had caused none of it. parity-governance.yml's own header notes that shape had already happened four times, three of them on an outside contributor. Here, and not behind a filter naming product paths, for the reason the guard test below already gives about a different suite: the list would be whatever the assertions happen to read today, so a filter only moves the trap. #1691 is the precedent, where a `Tools/**` filter silenced a suite that reads `TodayScreen.kt` and `AppModel.swift`, and main carried it red through nine merges. Its own step and its own floor, so the core floor of 114 stays independent, which is the reasoning already written beside that floor. Cost, measured on this runner rather than estimated: 51 s for 49 tests, against roughly 25 s for the rest of the leg. I first assumed this suite cost 4 to 6 minutes and designed a separate parallel workflow around that; the 4 to 6 minutes is `parity_ratchet.py`, which checks out the base tree and derives twice. This suite derives once. The guard is extended to assert the unfiltered leg actually invokes the module. The failure mode is not someone arguing against this, it is someone dropping the step or hanging a filter off it later, which is exactly what #1691 was. Mutation-verified: pointing the step at a different module makes the guard fail. The assertion reads the dotted module path, which appears only in the invocation and not in the prose above it, so a comment cannot satisfy it. Verified: `tests.test_parity_governance_acceptance` plus `tests.test_parity_disposition_kinds`, 51 tests, pass. The workflow parses. Refs #2567, #2557, #1691

AGENTS.md carried the gate-trigger gap as a still-open problem. Half of it is shut now: the repository-acceptance suite, the only test in parity-governance that reads product source rather than the scanner, runs on the unfiltered tools-python leg, with a guard test pinning it there. Past-tense the old description, record what the remedy deliberately was not, and keep the #1691 lesson attached to it. docs/CONTRIBUTING.md had nothing on the ledger at all: no scanned roots, no --refresh-derived, no disposition types. A contributor met it first as a red check with no way to read what it wanted. Add a section under cross-platform discipline covering the exact roots, the refresh command, --repair-stale-base versus the maintainer-only --migrate-authority, and the three typed dispositions with their exact key sets. Name the two scope gaps plainly rather than letting people discover them: Strand/ and StrandiOS/ are outside Swift scope while com/noop/data/** is inside Kotlin scope (#2567), and out_of_scope_twin is Kotlin-only with a twin_path pattern that cannot name a file under StrandiOS/. Also point "Add a new metric" at the new section, since its step 1 lands in a scanned root and its catalog twin is the exact asymmetry case.

ryanbr merged commit 7a8d0b6 into ryanbr:main Aug 28, 2026
3 checks passed

ryanbr mentioned this pull request Aug 28, 2026

ci: gates that never ran on the change they were written to catch #1699

Merged

Zebsi235 mentioned this pull request Aug 30, 2026

tooling: i18n_audit keys its baselines on OS-native paths, so it fails on a clean tree on Windows #1747

Merged

8 of 12 tasks

Zebsi235 deleted the fix/doclint-windows-paths branch September 6, 2026 13:20

ryanbr mentioned this pull request Sep 25, 2026

ci: run the Tools suites that read product source on every change, and filter only the leg that does not #2451

Merged

ryanbr mentioned this pull request Sep 29, 2026

ci: run the parity repository-acceptance suite on the unfiltered leg #2582

Merged

ryanbr mentioned this pull request Sep 29, 2026

docs: record the parity authority workflow and its scope gaps #2584

Merged

Navigation Menu

Repository navigation

tooling: doc_comment_lint keys the baseline on OS-native paths, so it fails on a clean tree on Windows - #1691

tooling: doc_comment_lint keys the baseline on OS-native paths, so it fails on a clean tree on Windows#1691
ryanbr merged 2 commits into
ryanbr:mainfrom
Zebsi235:fix/doclint-windows-paths

Zebsi235 commented Aug 27, 2026

Uh oh!

Zebsi235 commented Aug 27, 2026

Uh oh!

Zebsi235 commented Aug 27, 2026

Uh oh!

ryanbr commented Aug 28, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

Zebsi235 commented Aug 27, 2026

What this PR does

Type of change

How it was tested

Checklist

Related issues

Uh oh!

Zebsi235 commented Aug 27, 2026

Uh oh!

Zebsi235 commented Aug 27, 2026

Uh oh!

ryanbr commented Aug 28, 2026

The second file is my fault, not scope creep

The follow-up this exposes

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants