From 42c96bf8fcb1dd4913dcf177c0bcf6eb647f0f8b Mon Sep 17 00:00:00 2001 From: davotoula Date: Tue, 1 Sep 2026 10:28:30 +0200 Subject: [PATCH] feature: document the orphaned-translation trap and gate it pre-push MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The [ExtraTranslation] failure that took main red in 1ce583ec92 was not documented anywhere. amethyst/src/main/res/CLAUDE.md and the find-missing-translations skill both mention the lint rule, but only inside one narrow case (converting a to ). Neither stated the general rule: removing or renaming a key in the default values/strings.xml orphans every locale entry that still declares it. Nor would running lint have caught it in practice. The only pre-push gate is pre-push-spotless.sh, which runs spotlessApply and nothing else, and :amethyst:lintFdroidBenchmark takes ~19 minutes on a warm daemon, so it is not a per-commit check. Add both halves: - amethyst/src/main/res/CLAUDE.md gains a "Renaming or removing a string key" section stating the same-commit rule and why Crowdin is not a cleanup step CI waits for. The trap is that Crowdin is *partly* reliable — it cleaned 32 of 47 locales — so the tree looks correct in whichever files you open. Retitled the file (it is no longer plural-only) and marked the existing sections as the plural-specific ones they always were. - orphan_strings_check.py scans every locale's resource names against its tree's default values/, across both Crowdin-managed resource systems: the Android res trees and the commons Compose-Multiplatform catalog. 0.17s against the full repo. pre-push-orphan-strings.sh wraps it as a PreToolUse gate on git push / create_pull_request, reusing the shell-tokenizing push-detection from pre-push-spotless.sh so "push" inside a commit message is not mistaken for the subcommand. - find-missing-translations gains a Common Mistakes entry pointing at both. Verified: clean on the current tree; exit 2 listing the orphans when route_video is reintroduced in two locales and a retired key is seeded in the Compose catalog; gate fires on git push and create_pull_request, stays quiet on a commit whose message contains "push" and on non-Bash tools. --- .claude/hooks/lib/git_push_gate.py | 61 +++++++++++++ .claude/hooks/orphan_strings_check.py | 87 +++++++++++++++++++ .claude/hooks/pre-push-orphan-strings.sh | 37 ++++++++ .claude/hooks/pre-push-spotless.sh | 51 +++-------- .claude/settings.json | 5 ++ .../skills/find-missing-translations/SKILL.md | 1 + .github/workflows/build.yml | 3 + amethyst/src/main/res/CLAUDE.md | 70 +++++++++++++-- 8 files changed, 272 insertions(+), 43 deletions(-) create mode 100755 .claude/hooks/lib/git_push_gate.py create mode 100755 .claude/hooks/orphan_strings_check.py create mode 100755 .claude/hooks/pre-push-orphan-strings.sh diff --git a/.claude/hooks/lib/git_push_gate.py b/.claude/hooks/lib/git_push_gate.py new file mode 100755 index 0000000000..8e83cf690c --- /dev/null +++ b/.claude/hooks/lib/git_push_gate.py @@ -0,0 +1,61 @@ +#!/usr/bin/env python3 +"""Decide whether a PreToolUse payload on stdin is a push/PR boundary. + +Shared by every pre-push hook in this directory (pre-push-spotless.sh, +pre-push-orphan-strings.sh) so the gate condition is defined once. Each hook is +a separate process with its own stdin, so this is exec'd per hook rather than +run once and shared. + +Exit 0 = this call publishes code (gate it). Exit 1 = let it through. +""" + +import json +import shlex +import sys + +# Reaching the push subcommand means stepping over git's global options first. +GLOBAL_WITH_ARG = {"-c", "-C", "--namespace", "--git-dir", "--work-tree", "--exec-path"} + + +def is_boundary(data): + tool = data.get("tool_name", "") + if tool.endswith("create_pull_request"): + return True + if tool != "Bash": + return False + + cmd = (data.get("tool_input") or {}).get("command", "") + # Tokenize like a shell so `push` inside a quoted commit message or heredoc + # stays one token and is NOT mistaken for the push subcommand. + try: + tokens = shlex.split(cmd, comments=True) + except ValueError: + tokens = cmd.split() + + for i, token in enumerate(tokens): + if token != "git" and not token.endswith("/git"): + continue + j = i + 1 + while j < len(tokens): + tok = tokens[j] + if tok in GLOBAL_WITH_ARG: + j += 2 + elif tok.startswith("-"): + j += 1 + else: + break + if j < len(tokens) and tokens[j] == "push": + return True + return False + + +def main(): + try: + data = json.load(sys.stdin) + except Exception: + return 1 + return 0 if is_boundary(data) else 1 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/.claude/hooks/orphan_strings_check.py b/.claude/hooks/orphan_strings_check.py new file mode 100755 index 0000000000..a18b73de59 --- /dev/null +++ b/.claude/hooks/orphan_strings_check.py @@ -0,0 +1,87 @@ +#!/usr/bin/env python3 +"""Fail if any locale declares a string resource its default values/ no longer has. + +A key removed or renamed in a default `values/strings.xml` orphans every +`values-/strings.xml` entry that still declares it. In an Android res +tree that is an `[ExtraTranslation]` lint ERROR, which aborts +`:amethyst:lint` and with it the whole `test-and-build-android` CI job. + +Run directly, or via the pre-push-orphan-strings.sh hook that wraps it. +Exits 0 when clean, 2 with a report when not. +""" + +import glob +import os +import re +import sys +from collections import defaultdict + +# values-night, values-v29, values-sw600dp, ... are configuration qualifiers, +# not locales; only locale-qualified dirs can hold a translation. +LOCALE = re.compile(r"^values-(?:b\+[A-Za-z0-9+]+|[a-z]{2,3}(?:-r[A-Z]{2,3})?)$") +NAMED = re.compile(r'<(?:string|plurals|string-array)\s+[^>]*name="([^"]+)"') + +# Both Crowdin-managed resource systems (see the find-missing-translations +# skill, "Resource trees — scan BOTH"), each with what an orphan costs there. +# Android res is the tree lint policies; the Compose-Multiplatform catalog is +# not lint-checked, but an orphan there is the same authoring mistake and +# leaves a dead translation behind. +ROOTS = ( + ("*/src/*/res/values", "Android lint [ExtraTranslation] error — aborts the build"), + ("*/src/*/composeResources/values", "dead translation — key no longer exists in the default catalog"), +) + + +def names(paths): + found = set() + for path in paths: + with open(path, encoding="utf-8") as handle: + found |= set(NAMED.findall(handle.read())) + return found + + +def find_orphans(): + orphans = defaultdict(list) # (res_root, key, consequence) -> [locale, ...] + for pattern, consequence in ROOTS: + for default_dir in sorted(glob.glob(pattern)): + res_root = os.path.dirname(default_dir) + base = names(glob.glob(os.path.join(default_dir, "*.xml"))) + for locale_dir in sorted(glob.glob(os.path.join(res_root, "values-*"))): + locale = os.path.basename(locale_dir) + if not LOCALE.match(locale): + continue + extra = names(glob.glob(os.path.join(locale_dir, "*.xml"))) - base + for key in extra: + orphans[(res_root, key, consequence)].append(locale[len("values-"):]) + return orphans + + +def main(): + orphans = find_orphans() + if not orphans: + return 0 + + total = sum(len(v) for v in orphans.values()) + out = sys.stderr + print( + f"BLOCKED: {total} orphaned translation(s) across {len(orphans)} key(s) — " + "translated in a locale, absent from that tree's default values/.", + file=out, + ) + print(file=out) + for (res_root, key, consequence), locales in sorted(orphans.items()): + print(f" {res_root}: {key!r} in {len(locales)} locale(s) — {consequence}", file=out) + print(f" {' '.join(sorted(locales))}", file=out) + print(file=out) + print( + "A key removed or renamed in a default values/strings.xml must be deleted\n" + "from every values-*/strings.xml in the SAME commit. Crowdin's next sync is\n" + "not a cleanup step CI waits for — lint runs on the tree you push.\n" + "See amethyst/src/main/res/CLAUDE.md, 'Renaming or removing a string key'.", + file=out, + ) + return 2 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/.claude/hooks/pre-push-orphan-strings.sh b/.claude/hooks/pre-push-orphan-strings.sh new file mode 100755 index 0000000000..54983456c7 --- /dev/null +++ b/.claude/hooks/pre-push-orphan-strings.sh @@ -0,0 +1,37 @@ +#!/bin/bash +# PreToolUse gate: no locale string may outlive its default-locale key. +# +# Fires on `git push` (Bash tool) and on the create_pull_request MCP tool. +# Delegates to orphan_strings_check.py, which compares every +# `values-/*.xml` resource name against the union of names declared in +# that tree's default `values/*.xml`. Anything present in a locale but absent +# from the default is an orphan: in an Android res tree Android lint reports it +# as an [ExtraTranslation] ERROR, which aborts `:amethyst:lint` and +# therefore the whole `test-and-build-android` CI job. +# +# Why a dedicated hook instead of "just run lint": `:amethyst:lintFdroidBenchmark` +# takes ~19 minutes on a warm daemon, so nobody runs it per-commit. This check is +# a directory scan and finishes in well under a second. +# +# Run the scan by hand any time with: .claude/hooks/orphan_strings_check.py +set -uo pipefail + +hook_dir="$(cd "$(dirname "$0")" && pwd)" +cd "${CLAUDE_PROJECT_DIR:-.}" || exit 0 + +# --- Is this call a push/PR boundary? --- +payload="$(cat)" + +# Cheap pure-bash pre-filter before paying for a python spawn. The gate below +# can only answer "yes" for a payload containing "push" (a git push command) or +# "pull_request" (the create_pull_request MCP tool), so anything else is a +# guaranteed no. This hook runs on EVERY Bash tool call, and the spawn it skips +# costs ~35ms each time. +case "$payload" in + *push*|*pull_request*) ;; + *) exit 0 ;; +esac + +printf '%s' "$payload" | python3 "$hook_dir/lib/git_push_gate.py" || exit 0 + +exec python3 "$hook_dir/orphan_strings_check.py" diff --git a/.claude/hooks/pre-push-spotless.sh b/.claude/hooks/pre-push-spotless.sh index 7acf98eb73..a4d4932f9a 100755 --- a/.claude/hooks/pre-push-spotless.sh +++ b/.claude/hooks/pre-push-spotless.sh @@ -9,48 +9,23 @@ # so a clean apply means a green check. set -uo pipefail +hook_dir="$(cd "$(dirname "$0")" && pwd)" cd "${CLAUDE_PROJECT_DIR:-.}" || exit 0 -# --- Parse the tool call off stdin; decide whether this call is a boundary. --- +# --- Is this call a push/PR boundary? --- payload="$(cat)" -should_gate="$( - printf '%s' "$payload" | python3 -c ' -import json, shlex, sys -try: - data = json.load(sys.stdin) -except Exception: - print("no"); sys.exit(0) -tool = data.get("tool_name", "") -if tool.endswith("create_pull_request"): - print("yes"); sys.exit(0) -if tool != "Bash": - print("no"); sys.exit(0) -cmd = (data.get("tool_input") or {}).get("command", "") -# Tokenize like a shell so `push` inside a quoted commit message or heredoc -# stays one token and is NOT mistaken for the push subcommand. -try: - tokens = shlex.split(cmd, comments=True) -except ValueError: - tokens = cmd.split() -GLOBAL_WITH_ARG = {"-c", "-C", "--namespace", "--git-dir", "--work-tree", "--exec-path"} -for i, t in enumerate(tokens): - if t != "git" and not t.endswith("/git"): - continue - j = i + 1 - while j < len(tokens): # skip git global options to reach the subcommand - tok = tokens[j] - if tok in GLOBAL_WITH_ARG: - j += 2; continue - if tok.startswith("-"): - j += 1; continue - break - if j < len(tokens) and tokens[j] == "push": - print("yes"); sys.exit(0) -print("no") -' 2>/dev/null -)" -[ "$should_gate" = "yes" ] || exit 0 +# Cheap pure-bash pre-filter before paying for a python spawn. The gate below +# can only answer "yes" for a payload containing "push" (a git push command) or +# "pull_request" (the create_pull_request MCP tool), so anything else is a +# guaranteed no. This hook runs on EVERY Bash tool call, and the spawn it skips +# costs ~35ms each time. +case "$payload" in + *push*|*pull_request*) ;; + *) exit 0 ;; +esac + +printf '%s' "$payload" | python3 "$hook_dir/lib/git_push_gate.py" || exit 0 # Nothing to format if no Kotlin is tracked/changed at all — cheap early out. if ! git ls-files --error-unmatch '*.kt' '*.kts' >/dev/null 2>&1; then diff --git a/.claude/settings.json b/.claude/settings.json index 53540bdb4d..929a186506 100644 --- a/.claude/settings.json +++ b/.claude/settings.json @@ -8,6 +8,11 @@ "type": "command", "command": "$CLAUDE_PROJECT_DIR/.claude/hooks/pre-push-spotless.sh", "timeout": 180 + }, + { + "type": "command", + "command": "$CLAUDE_PROJECT_DIR/.claude/hooks/pre-push-orphan-strings.sh", + "timeout": 30 } ] } diff --git a/.claude/skills/find-missing-translations/SKILL.md b/.claude/skills/find-missing-translations/SKILL.md index 491b2478ef..50699978e5 100644 --- a/.claude/skills/find-missing-translations/SKILL.md +++ b/.claude/skills/find-missing-translations/SKILL.md @@ -540,6 +540,7 @@ When adding translated strings to locale files: - **Pasting the union set of missing keys into every locale → duplicate keys** — the union is the right set to *translate*, but the wrong set to *insert*. A key missing in only some locales, inserted into all of them, duplicates in the ones that already had it. Drive each file's insertion off its own per-locale diff (see Step 6). In `commons`, a duplicate key is build-breaking: `convertXmlValueResourcesForCommonMain` fails with `Duplicated key '…'`. **Always run the post-insertion duplicate + XML-wellformedness gate in Step 6 before declaring done.** (Happened 2026-07-21 with `ps1_save_block` / `podcast_value_for_value` / `chats_history_relays`.) - **Declaring the pass done without running `:amethyst:lintPlayBenchmark`** — the duplicate-key + XML + `convertXmlValueResourcesForCommonMain` gate is necessary but nowhere near sufficient. `MissingQuantity` and `ImpliedQuantity` are errors, there is no lint baseline, and `abortOnError` is on, so a change that compiles and passes every check in Step 6's first half can still take CI red. Compiling is not evidence. (Happened 2026-08-13: 3 lint errors after a clean duplicate/XML gate and a green `compileFdroidDebugKotlin`.) - **Converting a `` to `` with `other` only** — "Crowdin fills the rest" is false; `MissingQuantity` errors immediately and CI fails before any sync. Supply every category the locale uses at conversion time, and re-check the declension rather than reusing the old text for `one`. +- **Renaming or removing a key in `values/strings.xml` without deleting it from every locale in the same commit** — the surviving locale entries become orphans, and `ExtraTranslation` is an error. "Crowdin drops retired keys on its next sync" is the same false belief as the `other`-only shortcut above: lint runs on the tree you push. Worse, it's *partly* true — the sync cleans some locales and silently leaves others, so the files you happen to open look fine. Scan with the sub-second `.claude/hooks/orphan_strings_check.py` instead of the ~19-minute lint; see `amethyst/src/main/res/CLAUDE.md`, "Renaming or removing a string key". (Happened 2026-08-31: `route_video`/`new_short` left in 15 of 47 locales, 30 errors, red `main`.) - **Putting `tools:ignore` on a locale file** — Crowdin strips it on the next export. Suppressions belong on the source entry in `values/strings.xml`, which propagates. The `tools:ignore="Typos"` copies visible in cs/de/ar/eo/bn are the *result* of that propagation, not proof that locale-file attributes survive. (Happened 2026-08-13; it broke `main`.) - **Suppressing a lint rule on a key nothing references** — check `grep -rn "" --include='*.kt'` first. `poll_results_voters` was a bare noun with no count, zero call sites, and an unlocalizable shape; deleting it retired the problem outright where a suppression would only have muted it. - **Comparing placeholders without a `(?` — which fails the whole `test-and-build-android` CI job. + +**Rule: retire the key in every locale in the same commit that changes the +default locale.** One `git grep -l 'name="old_key"' amethyst/src/main/res` and a +delete pass; there is no follow-up commit that makes it right. + +**Do not wait for Crowdin.** Crowdin does eventually drop retired keys, but that +is irrelevant: lint runs against the tree you push, not against Crowdin's next +export. It is also only *mostly* reliable, which is what makes it a trap — the +sync may clean most locales and silently leave the rest, so the tree looks +correct in the files you happen to open. + +> 2026-08-31: `d6d5a72e49` renamed `route_video` → `route_media` and +> `new_short` → `new_media`, with the commit message reasoning "Crowdin drops the +> retired keys on its next sync". The sync that merged right after cleaned 32 of +> the 47 locales and left both keys in 15 — 30 `[ExtraTranslation]` errors, red +> main. Fixed in `1ce583ec92`. + +Renaming rather than editing in place is still correct when the **meaning** +changes (an in-place edit silently keeps 47 translations of the old meaning). +The key is new; the obligation is to delete the old one everywhere at once. + +### Checking before you push + +`:amethyst:lintFdroidBenchmark` catches this, but takes ~19 minutes on a warm +daemon, so it is not a per-commit gate. Use the sub-second scan instead: + +```bash +.claude/hooks/orphan_strings_check.py +``` + +It diffs every locale's resource names against its tree's default `values/` and +exits non-zero listing any orphan. It covers **both** Crowdin-managed resource +systems — the Android res trees (`amethyst/src/main/res`, +`commons/src/androidMain/res`) and the Compose-Multiplatform catalog +(`commons/src/commonMain/composeResources`) — and says which consequence applies: +lint only polices the Android trees, but an orphan in the Compose catalog is the +same mistake and leaves a dead translation behind. + +The same script runs at two other layers, so the rule is enforced rather than +merely described: + +| Layer | Where | Covers | +|---|---|---| +| CI | the fast `lint` job in `.github/workflows/build.yml` | every PR and every push to `main`, **including the Crowdin sync bot's** | +| Agent session | `.claude/hooks/pre-push-orphan-strings.sh`, wired as a `PreToolUse` hook in `.claude/settings.json` | a `git push` or PR creation from a Claude Code session | + +The CI layer is the one that matters most: the 2026-08-31 desync arrived through +a bot-authored PR (`.github/workflows/crowdin.yml` → `create-pull-request`), with +no local session anywhere in the path. It sits in the 15-minute `lint` job rather +than the 60-minute `test-and-build-android` job that originally caught it. + +## Plural rules Always consider Slavic / Baltic / Semitic / Celtic languages when a string contains a count. The CLDR plural categories `one` / `other` that English uses are **not enough** — these language families decline the noun on `few`, `many`, `two`, `zero`, etc. -## Rules - 1. **Any string whose noun changes form with the count must be a `` resource, not a ``.** If the English reads naturally as "1 X" vs "N X" with a different noun form, it's a plural. 2. **The same applies to thresholds** ("more than %1$d hashtags"). The count IS the threshold, and the noun form depends on it in some languages. 3. **Never hardcode `"1"` in the English text** of a `quantity="one"` item — always use the `%1$d` placeholder. Hardcoding breaks every language whose `one` category covers numbers other than 1 (e.g. some Slavic languages). @@ -16,7 +76,7 @@ Always consider Slavic / Baltic / Semitic / Celtic languages when a string conta Latvian's `zero` does **not** mean "no items" — it covers 0, 10, 11–19, 20, 30, … (`n % 10 = 0` or `n % 100 = 11..19`), so it fires on most counts and must read as a normal plural form. Never strip `` from `values-lv-rLV`; among the locales we ship, only Arabic and Latvian have an integer-bearing `zero`. -## Anti-patterns to flag +## Plural anti-patterns to flag When adding or reviewing strings, flag these: @@ -24,7 +84,7 @@ When adding or reviewing strings, flag these: - `1 reply` → hardcoded `1`, should be `%1$d reply`. - A locale `strings.xml` providing only `one`/`other` for Polish / Czech / Russian → missing `few`/`many`, will silently fall through to `other` for counts 2–4, 22–24, etc. -## Call-site patterns +## Plural call-site patterns In a `@Composable`: