mirror of
https://github.com/vitorpamplona/amethyst.git
synced 2026-10-05 19:28:25 +00:00
feature: document the orphaned-translation trap and gate it pre-push
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 <string> to <plurals>). 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.
This commit is contained in:
Executable
+61
@@ -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())
|
||||
Executable
+87
@@ -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-<locale>/strings.xml` entry that still declares it. In an Android res
|
||||
tree that is an `[ExtraTranslation]` lint ERROR, which aborts
|
||||
`:amethyst:lint<Variant>` 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())
|
||||
Executable
+37
@@ -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-<locale>/*.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<Variant>` 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"
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
}
|
||||
]
|
||||
}
|
||||
|
||||
@@ -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 `<string>` to `<plurals>` 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 "<key>" --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 `(?<!\\)` guard** — `\%2$d` is an escaped literal to lint, but a naive `%\d+\$[sd]` regex matches the placeholder inside it and reports the string clean. A parity sweep missing this guard will certify a broken translation. Also treat a *repeated* index (`%1$s` twice where the base has it once) as legitimate — German does this where English says "They".
|
||||
|
||||
@@ -21,6 +21,9 @@ jobs:
|
||||
- name: Checkout code
|
||||
uses: actions/checkout@v7
|
||||
|
||||
- name: Orphaned translations (no locale string may outlive its default key)
|
||||
run: .claude/hooks/orphan_strings_check.py
|
||||
|
||||
- name: Set up JDK 21
|
||||
uses: actions/setup-java@v5.7.0
|
||||
with:
|
||||
|
||||
@@ -1,9 +1,69 @@
|
||||
# String resources — plural handling
|
||||
# String resources
|
||||
|
||||
Two traps live here: retiring a key without cleaning up its translations, and
|
||||
getting plural categories wrong.
|
||||
|
||||
## Renaming or removing a string key
|
||||
|
||||
Deleting or renaming a key in the default `values/strings.xml` **orphans every
|
||||
`values-*/strings.xml` entry that still declares it**. Android lint reports each
|
||||
orphan as an `[ExtraTranslation]` **error**, and lint errors abort
|
||||
`:amethyst:lint<Variant>` — 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 `<plurals>` resource, not a `<string>`.** 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 `<item quantity="zero">` 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:
|
||||
- `<item quantity="one">1 reply</item>` → 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`:
|
||||
|
||||
|
||||
Reference in New Issue
Block a user