diff --git a/fastlane/lanes/catalog_helper.rb b/fastlane/lanes/catalog_helper.rb index 449fa2ab1915..32b2a63db2ae 100644 --- a/fastlane/lanes/catalog_helper.rb +++ b/fastlane/lanes/catalog_helper.rb @@ -24,9 +24,13 @@ def coverage_gap(reference, catalog_keys) reference.reject { |key| catalog_canonical.include?(canonical(key)) } end - # Collapse format specifiers to a single token so source-form (%li) and normalized (%1$li) compare equal. + # Strip the positional index from each format specifier so a source-form specifier (%li) and its normalized form + # (%1$li) compare equal, while specifiers of a DIFFERENT argument type stay distinct. The positional `N$` prefix + # is the only thing that differs between the two extraction paths (genstrings vs xcstringstool) for the same + # source literal; collapsing every specifier to one token — as this did before — conflated `%d days` with + # `%@ days` and masked a genuinely-dropped key behind a same-prose sibling of a different type. def canonical(key) - key.gsub(FORMAT_SPECIFIER, "\u0001") + key.gsub(FORMAT_SPECIFIER) { |specifier| specifier.sub(/\A%\d+\$/, '%') } end # --- immutable-key enforcement ------------------------------------------------------------------------- diff --git a/fastlane/lanes/catalog_helper_test.rb b/fastlane/lanes/catalog_helper_test.rb index 02e4470777ad..10b0bedcea1e 100644 --- a/fastlane/lanes/catalog_helper_test.rb +++ b/fastlane/lanes/catalog_helper_test.rb @@ -6,9 +6,9 @@ require 'minitest/autorun' require_relative 'catalog_helper' -# Exercises the reword detector: an explicit-key string whose stored English differs from a fresh extraction is -# flagged; key-as-source, variations/plural, unchanged, and removed keys are not. -class CatalogHelperRewordedKeysTest < Minitest::Test +# Exercises the reword detector (an explicit-key string whose stored English differs from a fresh extraction is +# flagged; key-as-source, variations/plural, unchanged, and removed keys are not) and the coverage compare. +class CatalogHelperTest < Minitest::Test def en(value) { 'localizations' => { 'en' => { 'stringUnit' => { 'state' => 'translated', 'value' => value } } } } end @@ -45,4 +45,12 @@ def test_reports_every_reworded_key strings = { 'a' => en('One'), 'b' => en('Two'), 'c' => en('Three') } assert_equal %w[b c], reworded(strings, { 'a' => 'One', 'b' => 'TWO', 'c' => 'THREE' }).sort end + + # coverage_gap must not conflate keys that differ only in argument TYPE. `canonical` used to replace every format + # specifier with one sentinel regardless of type/index, so if the build-free extraction loses "%d days" (e.g. a + # same-basename .stringsdata overwrite — the exact regression this gate exists to catch), the surviving "%@ days" + # masked it. "%d days" (int) and "%@ days" (object) are real, distinct en.lproj keys; dropping either is a hole. + def test_coverage_gap_does_not_conflate_distinct_type_sibling_keys + assert_equal ['%d days'], CatalogHelper.coverage_gap(['%d days', '%@ days'], ['%@ days']) + end end diff --git a/fastlane/lanes/catalog_strings_helper_test.rb b/fastlane/lanes/catalog_strings_helper_test.rb index 8f093402c1f4..b8fb8ec3a203 100644 --- a/fastlane/lanes/catalog_strings_helper_test.rb +++ b/fastlane/lanes/catalog_strings_helper_test.rb @@ -73,6 +73,21 @@ def test_human_translation_is_used_and_marked_translated assert_equal({ 'state' => 'translated', 'value' => 'Enregistrer' }, cell(cat, 'a', 'fr')) end + # A human translation that reorders a non-positional source (%@ … %@) via positional specifiers (%2$@ … %1$@) + # is valid at runtime — String(format:) honors positional specifiers regardless of the source's shape — and is + # the standard iOS reordering pattern for RTL / word-order-differing locales. It must ship as `translated`, NOT + # be rejected by the placeholder gate and downgraded to machine/English. This is the user-facing half of the + # TranslationValidator regression (see translation_validator_test.rb): 23 key-as-source strings across 34 + # locales (513 currently-shipping cells) do exactly this, e.g. ar "%@ of %@ used on your site" folds to + # "%1$@ من %2$@ على موقعك". With no AI tier here, the rejected human currently degrades to English (needs_review). + def test_reordered_human_translation_of_a_non_positional_source_is_kept_as_translated + cat = catalog('a' => entry('%@ of %@ used')) + capture_stderr { fold(cat, translations: { 'fr' => { 'a' => '%2$@ sur %1$@ utilisés' } }) } + + assert_equal({ 'state' => 'translated', 'value' => '%2$@ sur %1$@ utilisés' }, cell(cat, 'a', 'fr'), + 'a reordered human translation must ship as translated, not be downgraded') + end + # A whitespace-only GlotPress value is not a real translation: it must not ship as `translated` — the key # should still reach the model and land as `needs_review`, same as if GlotPress had no value at all. def test_whitespace_only_human_value_is_treated_as_untranslated diff --git a/fastlane/lanes/translation_validator.rb b/fastlane/lanes/translation_validator.rb index 29b9e9c887fa..853359d4ab71 100644 --- a/fastlane/lanes/translation_validator.rb +++ b/fastlane/lanes/translation_validator.rb @@ -3,10 +3,11 @@ # Format-specifier safety gate for machine-translated strings. # # The one correctness invariant for a translated `.strings` / `.xcstrings` value: it must preserve the -# source's printf / NSString format ARGUMENTS exactly — same count, same types, and (for positional -# `%1$@` specifiers) the same index→type mapping. The surrounding prose is free to change; the argument -# contract is not. Break it and the app reads the wrong vararg off the stack — a crash or garbage at -# runtime, in a locale the author can't read and CI can't catch. +# source's printf / NSString format ARGUMENTS exactly — same count, same types, and the same index→type +# mapping (bare specifiers are numbered by appearance order, so a translation may reorder them with +# explicit `%2$@ … %1$@` specifiers — the standard iOS mechanism). The surrounding prose is free to +# change; the argument contract is not. Break it and the app reads the wrong vararg off the stack — a +# crash or garbage at runtime, in a locale the author can't read and CI can't catch. # # This is deliberately plain Ruby with no dependencies, so it can gate EVERY machine translation before it # is written and be unit-tested directly. It's the floor under the `human ?? AI ?? English` resolution in @@ -23,8 +24,8 @@ module TranslationValidator % # leading percent (?:(?\d+)\$)? # optional positional index: 1$, 2$, … [\#0\-+']* # flags (NOT space — see note above) - (?:\d+|\*)? # field width - (?:\.(?:\d+|\*))? # precision + (?\d+|\*)? # field width (`*` = a dynamic-width int argument) + (?:\.(?\d+|\*))? # precision (`*` = a dynamic-precision int argument) (?hh|h|ll|l|L|q|z|t|j)? # length modifier (?[@dDiuUxXoOfFeEgGaAcCsSpn%]) # conversion /x @@ -60,8 +61,8 @@ def placeholders_match?(source, candidate) # nil when the contract is preserved; otherwise a short human-readable reason (for logging which AI cells # were rejected and why). def mismatch_reason(source, candidate) - src = signature(source) - cand = signature(candidate) + src = implied_positional(signature(source)) + cand = implied_positional(signature(candidate)) if src.positional != cand.positional "positional placeholders differ (source: #{describe_positional(src.positional)}; " \ @@ -75,19 +76,58 @@ def mismatch_reason(source, candidate) def signature(str) positional = {} sequential = [] - each_specifier(str.to_s) do |match| - next if match[:conv] == '%' # literal %% — not an argument - - token = "#{match[:length]}:#{TYPE_CLASS.fetch(match[:conv], match[:conv])}" - if match[:position] - positional[match[:position].to_i] = token - else - sequential << token - end - end + each_specifier(str.to_s) { |match| record_specifier(match, positional, sequential) } Signature.new(positional, sequential) end + # Fold one matched specifier into the running positional/sequential views (mutated in place). `%%` is a literal + # percent, consuming no argument. Dynamic field width/precision each consume their OWN int argument, bound + # sequentially before the value (`%*d` = width int then value; `%.*f` = precision int then value), so counting + # them keeps `%d` and `%*d` — which are NOT interchangeable — from reducing to the same signature. + def record_specifier(match, positional, sequential) + return if match[:conv] == '%' + + sequential << ':int' if match[:width] == '*' + sequential << ':int' if match[:precision] == '*' + + token = "#{match[:length]}:#{TYPE_CLASS.fetch(match[:conv], match[:conv])}" + if match[:position] + add_positional(positional, match[:position].to_i, token) + else + sequential << token + end + end + private_class_method :record_specifier + + # Record a positional specifier's type at its index. A positional index MAY be referenced more than once, but only + # with a CONSISTENT type (`%1$d … %1$d` is legal — it reads argument 1 twice). Two different types at one index is + # malformed — it passes an int to a `%@` conversion (or vice versa) at runtime — so store the conflict as a + # distinct token that can never equal a well-formed source's single type, rather than overwriting last-wins. + def add_positional(positional, index, token) + existing = positional[index] + positional[index] = existing && existing != token ? "conflict(#{[existing, token].sort.join(', ')})" : token + end + private_class_method :add_positional + + # A purely-sequential signature rewritten with the implicit 1..N indices its bare specifiers already + # carry (`%@ %d` binds argument 1 then 2, so it becomes `{1 => object, 2 => int}`). This is what lets a + # translation reorder a NON-positional source via explicit positional specifiers: once both sides are + # expressed as index→type maps, `%@ - %@` and `%2$@ - %1$@` compare equal — the reorder is honored at + # runtime by `String(format:)` regardless of the source's shape. A genuine sequential flip still fails, + # because `%@: %d` → `{1 => object, 2 => int}` and `%d : %@` → `{1 => int, 2 => object}` disagree on + # which index owns which type. A signature that already uses positional specifiers is returned + # unchanged; so is one that MIXES bare and positional specifiers (malformed per Apple's docs), so a + # mixed string still requires an exact structural match and can't accidentally validate against a clean + # map. + def implied_positional(sig) + return sig unless sig.positional.empty? && !sig.sequential.empty? + + positional = {} + sig.sequential.each_with_index { |token, i| positional[i + 1] = token } + Signature.new(positional, []) + end + private_class_method :implied_positional + # Yields each format-specifier MatchData in appearance order. Scans forward from the end of each match, so # adjacent specifiers (`%d%@`) and specifiers embedded in text are all found. def each_specifier(str) diff --git a/fastlane/lanes/translation_validator_test.rb b/fastlane/lanes/translation_validator_test.rb index 8cbd91a2f38f..bbc9c3a955e3 100644 --- a/fastlane/lanes/translation_validator_test.rb +++ b/fastlane/lanes/translation_validator_test.rb @@ -23,6 +23,24 @@ def test_positional_type_change_is_rejected refute V.placeholders_match?('%1$@ posts', '%1$d posts') end + # A NON-positional source (%@ … %@) whose translation reorders via positional specifiers (%2$@ … %1$@) is the + # standard, Apple-documented iOS way to reorder arguments for target grammar: String(format:) honors positional + # specifiers regardless of the source's shape, and each specifier still reads an argument of the same type. So + # this must be ACCEPTED, exactly like the already-positional reorder above. The gate currently rejects it, + # because it compares the source's positional/sequential views independently and a bare-%@ source has an empty + # positional map. This is not academic: real GlotPress data has 23 key-as-source strings across 34 locales + # (513 shipping cells) that positionalize a bare-%@ English source this way — e.g. the Arabic + # "%@ of %@ used on your site" => "%1$@ من %2$@ على موقعك". The human-translation gate this catalog work adds + # (CatalogStrings.trusted_human, PluralStrings.human_forms_for) rejects every one of them and downgrades a + # valid, currently-shipping human translation to machine/English. + # + # (Genuine breakage stays rejected by the existing tests: a sequential flip `%@: %d` => `%d : %@` and a type + # change are still caught, so a correct fix cannot simply ignore the positional/sequential distinction.) + def test_positionalizing_a_non_positional_source_to_reorder_is_allowed + assert V.placeholders_match?('%@ - %@', '%2$@ - %1$@') # two objects, reordered + assert V.placeholders_match?('%@. %d posts.', '%1$@. %2$d posts.') # object + int, positionalized in place + end + def test_sequential_order_must_be_preserved refute V.placeholders_match?('%@: %d', '%d : %@') # flipped non-positional args assert V.placeholders_match?('%@: %d', 'Total %@: %d') # same order, prose changed @@ -56,4 +74,29 @@ def test_mismatch_reason_is_descriptive assert_nil V.mismatch_reason('%1$@ invited %2$@', '%2$@ a invité %1$@') end + + # --- Failing tests: gaps surfaced by the pipeline-breaker audit (currently RED; each documents a real defect + # where the gate violates its own "same count, same types, same index→type mapping" invariant) ----------- + + # A candidate that binds the SAME positional index to two different types (%1$@ then %1$d) changes the argument + # contract: at runtime String(format:) reads argument 1 as an OBJECT for %1$@, but the source only ever supplies + # an int there — a wrong-vararg read (crash / garbage). The gate must reject it. It currently does NOT, because + # signature() stores specifiers in a Hash keyed by index with plain assignment, so %1$d silently OVERWRITES + # %1$@ at index 1 (last-wins), collapsing the type conflict to {1 => int}. This PR's implied_positional widened + # the blast radius: a bare-specifier source like `%d` now normalizes to {1 => int} and matches the collapsed + # candidate, so the first case below regressed from reject (pre-PR) to accept. + def test_duplicate_positional_index_with_conflicting_type_is_rejected + refute V.placeholders_match?('%d', '%1$@ %1$d') # regressed by implied_positional (rejected pre-PR) + refute V.placeholders_match?('%1$d words', '%1$@ %1$d words') # pre-existing last-wins collapse at index 1 + end + + # Dynamic field width / precision (`*`) consumes an EXTRA int vararg before the value: `%*d` reads a width int + # then the int value (two args); `%.*f` reads a precision int then the double (two args). The gate matches the + # `*` in the FORMAT_SPECIFIER regex but never emits an argument token for it, so `%d` and `%*d` (and `%.*f` vs + # `%.2f`) reduce to the identical signature — an arg-count mismatch it must catch per its own "same count" + # invariant. Both currently accepted. No shipping source uses `%*` today, so this is latent, not live. + def test_dynamic_width_precision_star_is_counted + refute V.placeholders_match?('%d posts', '%*d posts') # candidate reads one int past what the source supplies + refute V.placeholders_match?('Value: %.*f', 'Value: %.2f') # source consumes precision-int + double; candidate one + end end