Failing tests: placeholder gate rejects valid reordered human translations

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 -------------------------------------------------------------------------

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

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

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
     (?:(?<position>\d+)\$)?              # optional positional index: 1$, 2$, …
     [\#0\-+']*                           # flags (NOT space — see note above)
-    (?:\d+|\*)?                          # field width
-    (?:\.(?:\d+|\*))?                    # precision
+    (?<width>\d+|\*)?                    # field width (`*` = a dynamic-width int argument)
+    (?:\.(?<precision>\d+|\*))?          # precision (`*` = a dynamic-precision int argument)
     (?<length>hh|h|ll|l|L|q|z|t|j)?      # length modifier
     (?<conv>[@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)

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