Skip to content

i18n CI: make the coverage gate diff-scoped again, not a full-tree invariant - #524

Merged
ryanbr merged 4 commits into
ryanbr:mainfrom
digitalerdude:fix/452-i18n-diff-scoped-gate
Sep 19, 2026
Merged

ryanbr merged 4 commits into
ryanbr:mainfrom
digitalerdude:fix/452-i18n-diff-scoped-gate

Conversation

@digitalerdude

@digitalerdude digitalerdude commented Jul 16, 2026 •

Copy link
Copy Markdown

Why

ci_check() in Tools/i18n_audit.py (#452) originally audited the whole merged tree unconditionally once the #453 backlog closed — base_ref was accepted on the CLI but never actually used. That means a transient regression on main red-flags every open PR, including ones whose diff never touched the offending file.

That's exactly what happened: the 9.0.1 release generated a raw literal into AppChangelog.kt (fixed by #514), and it broke the gate on #507, #512, #513 — then #514 fixed main, but #508 and #506 kept showing the stale failure for hours afterward, because a pull_request-triggered workflow doesn't re-run just because the base branch changed.

(#509 independently re-implemented the same AppChangelog.kt fix as #514, which had already landed — it didn't touch this underlying issue.)

Since this PR was opened, #558/#581 landed a much-improved literal scanner (catches literals hidden behind if/else/when/.let, and contentDescription inside a semantics{} lambda) plus a baseline (Tools/i18n_audit_baseline.json) to exempt the ~250-entry backlog that scanner surfaced. The baseline solves a different problem than this PR, though: it's a fixed snapshot, so a new regression on main after the snapshot (the same class of incident as #514) would still red-flag every unrelated open PR until someone manually updates the baseline.

What

ci_check now exempts a violation on either of two independent grounds:

  1. It's in the committed baseline (the known, incrementally-closable backlog).
  2. It already exists at base_ref — so this PR didn't cause it, regardless of whether it's made it into the baseline yet.

Locale-gap and format-mismatch checks (missing translations, printf-argument mismatches) aren't covered by the baseline at all — the base_ref diff is their only exemption. On a healthy main this is a no-op: the gate behaves identically to before. It only changes what happens when main itself regresses — that regression now stays scoped to whoever caused it, instead of spreading to every other open PR, independent of whether the baseline has caught up yet.

Verification

  • python3 Tools/i18n_audit.py --ci HEAD on the current tree → exit 0 (192 Android + 64 iOS pre-existing findings correctly reported as exempt, not failures).
  • Simulated the original incident: pinned both tree and base_ref to the pre-i18n: localize the 9.0.1 What's New title (unblock the coverage gate) #514 commit (still carrying the broken AppChangelog.kt literal) → reported as pre-existing, exit 0. Under the gate as it stood before this PR, that same state hard-fails (confirmed against Clarify phone heart-rate broadcast copy #508/Restore recognizable Android dashboard metric icons #506's actual CI logs at the time).
  • Removed one real entry from the committed baseline and reran --ci HEAD → still exit 0, proving the base_ref exemption works independently of the baseline (this is the actual value of the PR: a future regression not yet in any baseline still won't spread to unrelated PRs).
  • Added a genuine new hardcoded literal to a Text(...) call not present in the baseline or on base_ref → correctly caught, exit 1; reverted, exit 0 again.
  • Android <plurals> (i18n: real <plurals> instead of hand-rolled singular/plural pairs #541) and Apple variations.plural (i18n: teach the Apple audit about xcstrings plural variations #542) regressions (deleted a real translated plural / broke placeholder parity on a synthetic multi-form plural) → both still correctly caught after merging with this PR's diff-scoping.
  • python3 Tools/i18n_audit.py (plain report mode) and --platform ios/android --full and --update-baseline all still run correctly and match the committed baseline exactly (no drift).

Scope

Python-only change to Tools/i18n_audit.py, no workflow YAML changes needed. No app code touched.

…variant

ci_check() audited the whole merged tree unconditionally once the ryanbr#453
backlog closed, ignoring base_ref entirely. That means a transient
regression on main (the 9.0.1 release generating a raw AppChangelog.kt
literal, fixed by ryanbr#514) red-flagged every open PR whose diff never
touched the offending file — and those PRs stayed red, since a
pull_request-triggered workflow doesn't re-run just because main
changed. ryanbr#508 and ryanbr#506 are still showing that stale failure right now,
over 5 hours after ryanbr#514 fixed main.

Restores the base_ref diff (Android side already had the git_show/
literals_at_ref scaffolding for this, just unused) across every check:
hardcoded literals, missing-translation keys, and format-string
mismatches, on both platforms. A violation already present on base_ref
isn't this PR's fault and no longer fails it; existing debt is still
printed for visibility. Today that's zero everywhere, so the practical
behavior on a healthy main is unchanged — this only changes what
happens when main itself regresses.
@digitalerdude
digitalerdude force-pushed the fix/452-i18n-diff-scoped-gate branch from 77de488 to 301a39a Compare July 27, 2026 13:59
@ryanbr
ryanbr merged commit e6bbceb into ryanbr:main Sep 19, 2026
4 checks passed
ryanbr added a commit that referenced this pull request Sep 19, 2026
#524 made `--ci` exempt any violation already present at the base ref, which is what
stops a regression on main red-flagging every open PR. The comment beside the
invocation still said the opposite:

    ... `--ci` is a whole-tree gate, so the value it resolves to does not
        change the verdict.

Comment only, no behaviour change. It now records the two things a future editor of
this file needs and cannot see from the YAML.

`fetch-depth: 0` is load-bearing: the audit reads the base through `git show`, so a
shallow clone misses every base lookup. That direction fails safe, since nothing
gets exempted and the gate fails loudly, but it fails for a reason nobody would
guess from the error.

And on a PUSH to main the fallback resolves to origin/main, which IS the pushed
commit, so that run compares the tree against itself and cannot fail. Verified by
planting a hardcoded literal and running both ways: base=HEAD passes, base=HEAD~1
catches it. The pull_request run is the one that bites; this job on main is a no-op
today rather than the main-side safety net the push trigger was added as. Recorded
here rather than changed, because the fallback is a behaviour question for a
separate change.

Sharpened after reading this file's own header: the push trigger was added for a
named, recurring case. A release pushes straight to main with no PR, and
appchangelog-gen.py emits the What's New title as a raw Kotlin literal that has to
be localized by hand. Auditing the push is how that gets caught the moment it lands.
The comment now says so, rather than calling it a safety net in the abstract.
ryanbr added a commit that referenced this pull request Sep 19, 2026
#524 made `--ci` exempt any violation already present at the base ref, which is what
stops a regression on main red-flagging every open PR. It also made the base ref
decide the verdict, and the push path was still resolving it the old way.

On a push, `github.base_ref` is empty and the fallback resolved to `origin/main`,
which IS the pushed commit. That run compared the tree against itself and could
never fail.

This is not abstract. The push trigger exists, per this file's own header, because a
RELEASE pushes straight to main with no PR and `appchangelog-gen.py` emits the
What's New title as a raw Kotlin literal that has to be localized by hand. Auditing
the push is how that gets caught the moment it lands. Simulated exactly that, using
the real 11.8.0 title as the planted literal:

    base = origin/main (the pushed commit)   exit 0   slips through
    base = github.event.before               exit 1   FAIL 1 new hardcoded literal
      android/.../SleepScreen.kt: 'A gym log book on your wrist, a Coach you can
      switch off'

So a push now diffs against `github.event.before`, the tip before the push, which
spans every commit the push carried rather than only the last one. A pull request is
unchanged: it still diffs against the branch it targets.

`event.before` is all zeros on a branch's first push and absent on a manual run.
Neither can be diffed, so both fall back to `HEAD~1`: narrower, but still a real
comparison rather than a self-comparison. All three paths exercised locally.

The comment now describes the behaviour as fixed rather than as broken, so main
never carries a note saying this run is inert.
ryanbr added a commit that referenced this pull request Sep 19, 2026
…2329)

ci(i18n): diff a push against the tip before it, not against itself

#524 made `--ci` exempt any violation already present at the base ref, which is what
stops a regression on main red-flagging every open PR. It also made the base ref
decide the verdict, and the push path was still resolving it the old way.

On a push, `github.base_ref` is empty and the fallback resolved to `origin/main`,
which IS the pushed commit. That run compared the tree against itself and could
never fail.

This is not abstract. The push trigger exists, per this file's own header, because a
RELEASE pushes straight to main with no PR and `appchangelog-gen.py` emits the
What's New title as a raw Kotlin literal that has to be localized by hand. Auditing
the push is how that gets caught the moment it lands. Simulated exactly that, using
the real 11.8.0 title as the planted literal:

    base = origin/main (the pushed commit)   exit 0   slips through
    base = github.event.before               exit 1   FAIL 1 new hardcoded literal
      android/.../SleepScreen.kt: 'A gym log book on your wrist, a Coach you can
      switch off'

So a push now diffs against `github.event.before`, the tip before the push, which
spans every commit the push carried rather than only the last one. A pull request is
unchanged: it still diffs against the branch it targets.

`event.before` is all zeros on a branch's first push and absent on a manual run.
Neither can be diffed, so both fall back to `HEAD~1`: narrower, but still a real
comparison rather than a self-comparison. All three paths exercised locally.

The comment now describes the behaviour as fixed rather than as broken, so main
never carries a note saying this run is inert.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants