Repository navigation
Conversation
The fold reused a machine cell from the previous run without checking it still matched the English. When the source's placeholders change (e.g. `%1$ld` to `%1$d`), the old cell translates a string that no longer exists, yet it stayed in the catalog and anchored the next AI request. Human cells already pass the same placeholder check. --- Generated with the help of Claude Code, https://claude.com/claude-code Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Generated by 🚫 Danger |
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues were identified for the change that prevents reuse of machine translations with mismatched placeholders.
No open issues 🎉
What changed in this PR
This PR prevents the plural localization fold from reusing machine translations whose placeholders no longer match English, supporting source changes such as the word-count update in #26261.
Changes:
- Validate retained machine translations against English; warn and discard mismatches so they cannot anchor AI requests.
- Add regression tests covering replacement by fresh AI translations and fallback to English when AI is unavailable.
| File | Description |
|---|---|
fastlane/lanes/plural_strings_helper.rb |
Checks retained machine cells for placeholder compatibility before reuse. |
fastlane/lanes/plural_strings_helper_test.rb |
Tests stale-cell rejection, AI request anchors, warnings, and English fallback. |
Review effort: Balanced
AliSoftware
left a comment
AliSoftware
left a comment
There was a problem hiding this comment.
As a side note, if I understand correctly this change, this change only drops a previously-machine-translated copy if the placeholders of the machine translation don't match the placeholders of the English source anymore… but I wonder if at some point we shouldn't go even further and try to detect if the English copy was changed in any way (even if the placeholders are still the same but the copy around was tweaked), not just placeholders, to enforce English sources to be immutable 🤔 Reflection for a future PR only though, and only if we find it necessary.
| (slot.cats - human_forms.keys).each_with_object({}) do |cat, acc| | ||
| value = kept_ai_value(prior[cat], english_forms[cat]) | ||
| acc[cat] = value if value && !human_forms.key?(cat) | ||
| acc[cat] = value if value && still_matches_english?(value, english_forms[cat], "#{slot.locale} machine translation for #{key.inspect} (#{cat})") |
There was a problem hiding this comment.
iinm technically speaking a value can be dropped if the placeholders differ and it was not a human-provided translation. "Machine translation" is the main case of "not a human-provided translation" indeed, but it can also be a case of an English fallback (e.g. if the last AI pass refused to translate because missing Anthropic key or outage or whatnot). So maybe the description for the error message should be "machine or fallback translation" rather than just "machine translation" to be more accurate?
| acc[cat] = value if value && still_matches_english?(value, english_forms[cat], "#{slot.locale} machine translation for #{key.inspect} (#{cat})") | |
| acc[cat] = value if value && still_matches_english?(value, english_forms[cat], "#{slot.locale} machine (or fallback) translation for #{key.inspect} (#{cat})") |
This is quite pedantic though, so not really that important, but still mentioning it also to validate that my understanding of the logic behind that code and PR (and it also dropping English fallback cases) is correct (I'm still trying to wrap my head about how the overall AI translation process was implemented in WPiOS as I never had the occasion to look at this code and all its helpers and how pieces fit together until now)
Description
The plural fold reuses machine (
needs_review) cells from its previous run without checking they still match the English.When the English source changes its placeholders, as #26261 does (
%1$ld→%1$d), the old machine text stays inPlurals.xcstringsand anchors the next AI request, even though human translations of the old English are already rejected by the same placeholder check.Kept machine cells now go through that check too: a mismatched one is dropped with a warning and re-filled by the AI tier, or English.
Found while validating #26261; independent of it, so it's based on
trunk.Testing instructions
ruby fastlane/lanes/plural_strings_helper_test.rb(or the Localization Tooling Unit Tests job). The two new tests fail without the fix.download_localized_plurals, so it ran on a throwaway stacked PR ([DO NOT MERGE] Validate plural fold for #26263 #26342, closed) — build 35795. On Use%1$din the plural word count #26261's catalog, withoutANTHROPIC_API_KEY, trunk's helper kept all 32 stale%1$ldword-count cells. This PR's helper dropped all 32 with a warning each, touched no other key, and the folded catalog compiles.