Surface issue in apps intents strings localization tooling by mokagio · Pull Request #25805 · wordpress-mobile/WordPress-iOS

mokagio · 2026-07-20T01:37:58Z

I finally got to have a look at #25766. Or rather, I first handed it to Opus 4.8 for a look. It found two possible issues and I asked it to add tests to guard against them. They are currently red.

test_no_two_placeholders_resolve_to_the_same_argument — the counter advances on explicitly-positioned placeholders too, so %2$arg and %arg renumbers both onto argument 2 and one interpolation silently disappears from the translated string.
test_an_escaped_percent_before_arg_is_not_a_placeholder — 100%%arg is prose containing a literal percent, but the regex matches the second % and swallows the word, producing 100%%1$@ and inventing a format argument that doesn't exist.

The shared consequence is the same: both corrupt the English originals before they reach GlotPress, so translators work from a broken string and the damage lands in every locale at once, with no compile error and no runtime crash to catch it.

AI-generated details

Description

Stacked on #25766 — review that one first; this PR's diff is only the commits on top.

positionalize_untyped_arguments is the fiddliest logic in #25766: a regex that renumbers the extraction's untyped %arg placeholders into positional printf specifiers before they reach GlotPress. It sat inside platform :ios do in localization_catalog.rb, which the repo's pure-Ruby minitest convention cannot load, so it shipped untested. This moves it to an AppIntentsStrings module — where .buildkite/commands/test-localization-tooling.sh already globs for a suite — and specifies its behaviour.

The extraction itself is verbatim, so it reviews as behaviour-preserving.

This suite is intentionally red. Two cases describe bugs the rewrite has today and fail against it; they go green when it is fixed:

Mixing explicit and untyped positions renumbers onto a duplicate index — "%2$arg and %arg" collapses two placeholders onto argument 2. Asserted as an invariant (no two placeholders resolve to the same argument) rather than an exact string, because which index the untyped one should take is a design call for whoever fixes it.
An escaped percent immediately before the word "arg" is consumed as a placeholder — "100%%arg" becomes 100%%1$@.

Neither is reachable today, since no App Intents defaultValue interpolates anything yet. That is the argument for specifying them now: the first time one is written, the failure mode is mangled originals sent to translators at a code freeze, with nothing in the loop to catch it.

I left the fixes out so the extraction stays reviewable on its own — happy to add them here instead.

Testing instructions

.buildkite/commands/test-localization-tooling.sh

Runs on CI as the Localization tooling unit tests step; needs no bundle, Xcode, or network. Expect exactly two failures, both in app_intents_strings_helper_test.rb; every other suite passes.

iOS 18 and later resolve the widget configuration UI's App Intents strings against the app bundle, whose GlotPress-managed Localizable.strings already carries the ios-widget. keys in every locale, so the static copies in the extension bundle only served iOS 17 (verified on 17.5/18.6/26.5 simulators, including that iOS 17 has no fallback to the app bundle). The iOS 17 user base no longer justifies maintaining 34 hand-refreshed translation snapshots, so its widget configuration UI now shows the English defaults.

genstrings cannot parse LocalizedStringResource, so the App Intents strings reached GlotPress through a hand-maintained mirror of the defaultValues in code, which could silently drift and ship a new string English-only. The code freeze now extracts the call sites with xcstringstool (which understands LocalizedStringResource, including comments) and merges them into the upload like the plural originals, making code the single source of truth. Translator comments move into the code via the comment: parameter, and the mirror file is deleted.

app_intents_entries duplicated current_english_values' fresh-catalog sync inline, which also pushed its ABC size past the RuboCop threshold flagged on the PR. Both now delegate to a shared synced_throwaway_catalog helper, and the empty-stringsdata guard now applies to both callers.

`positionalize_untyped_arguments` sat inside `platform :ios do` in `localization_catalog.rb`, so the repo's pure-Ruby minitest convention could not load it. Moving it to an `AppIntentsStrings` module makes it testable and puts it where `.buildkite/commands/test-localization-tooling.sh` already globs for a suite. The logic is unchanged, so the extraction can be reviewed as behaviour-preserving and the tests that follow pin what it does today. --- Generated with the help of Claude Code, https://claude.ai/code Co-Authored-By: Claude Code Opus 4.8 <noreply@anthropic.com>

Two cases are asserted as they behave today rather than as they should, so a fix flips them deliberately instead of silently: - A value mixing explicit and untyped positions renumbers onto a duplicate index (`"%2$arg and %arg"` yields `%2$@` twice), collapsing two placeholders onto one argument. - An escaped percent immediately before the word "arg" is consumed as a placeholder (`"100%%arg"` yields `100%%1$@`). Neither is reachable from a current call site — no App Intents `defaultValue` interpolates anything yet — which is precisely why they would otherwise surface as mangled translator originals at some future code freeze. --- Generated with the help of Claude Code, https://claude.ai/code Co-Authored-By: Claude Code Opus 4.8 <noreply@anthropic.com>

Co-authored-by: Gio Lodi <giovanni.lodi42@gmail.com>

The two bug cases asserted what the code does today, which makes the defects permanent: a fix has to edit the tests to land. Inverted, they now fail and go green when the rewrite is fixed. The mixed-form case asserts that no two placeholders resolve to the same argument rather than an exact string, because which index the untyped one should take is a design call for whoever fixes it. --- Generated with the help of Claude Code, https://claude.ai/code Co-Authored-By: Claude Code Opus 4.8 <noreply@anthropic.com>

--- Generated with the help of Claude Code, https://claude.ai/code Co-Authored-By: Claude Code Opus 4.8 <noreply@anthropic.com>

A test class's name and its cases say what it covers, so the cop only ever buys a comment written to satisfy it — which is what the line this removes was. --- Generated with the help of Claude Code, https://claude.ai/code Co-Authored-By: Claude Code Opus 4.8 <noreply@anthropic.com>

mokagio · 2026-07-20T09:40:12Z

+Style/Documentation:
+  Exclude:
+    - '**/*_test.rb'
+


Could be out of scope for this PR, but Danger -> RuboCop barked at me for not having documentation in the new test file. But, IMHO, test files should not need a documentation header. The file name and the test methods names and implementations should be short and self-explanatory.

wpmobilebot · 2026-07-20T09:52:35Z

📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.

	App Name	WordPress
	Configuration	Release-Alpha
	Build Number	`33265`
	Version	`PR #25805`
	Bundle ID	`org.wordpress.alpha`
	Commit	`71305e8`
	Installation URL	208lgenij4nt8

Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

wpmobilebot · 2026-07-20T09:52:45Z

📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.

	App Name	Jetpack
	Configuration	Release-Alpha
	Build Number	`33265`
	Version	`PR #25805`
	Bundle ID	`com.jetpack.alpha`
	Commit	`71305e8`
	Installation URL	06opvngidk7ao

Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

Copilot

Pull request overview

This PR extracts the App Intents string placeholder-rewriting logic into a plain-Ruby helper module so it can be unit-tested, and adds a minitest suite that codifies (and currently exposes) edge cases that can corrupt English source strings before they reach GlotPress.

Changes:

Move positionalize_untyped_arguments out of localization_catalog.rb into AppIntentsStrings for direct unit testing.
Add a new minitest suite covering placeholder rewriting behavior, including two edge cases that currently fail.
Adjust RuboCop configuration related to documentation requirements for test files.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File	Description
fastlane/lanes/localization_catalog.rb	Switch to calling the extracted helper module for App Intents placeholder rewriting.
fastlane/lanes/app_intents_strings_helper.rb	Introduce a unit-testable module containing `positionalize_untyped_arguments`.
fastlane/lanes/app_intents_strings_helper_test.rb	Add minitest coverage for placeholder rewriting, including regression cases.
.rubocop.yml	Exclude test files from `Style/Documentation` (broad configuration change).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+  #
+  # Two bugs, red in the adjacent suite: the counter advances on placeholders that already carry an explicit
+  # position, and an escaped percent before "arg" is consumed as a placeholder.
+  def positionalize_untyped_arguments(value)
+    index = 0
+    value.gsub(/%(\d+\$)?arg/) do
+      index += 1
+      "%#{Regexp.last_match(1) || "#{index}$"}@"
+    end
+  end


+require 'minitest/autorun'
+require_relative 'app_intents_strings_helper'
+
+class AppIntentsStringsHelperTest < Minitest::Test


+  # FAILING. Asserted as an invariant, not an exact string: which index the untyped placeholder should take
+  # is the fixer's call, that it must not duplicate an explicit one is not.
+  def test_no_two_placeholders_resolve_to_the_same_argument
+    indices = positionalize('%2$arg and %arg').scan(/%(\d+)\$@/).flatten
+
+    assert_equal indices.uniq, indices
+  end
+
+  # FAILING. `%%` is a literal percent, so "100%arg" is prose, not an interpolation.
+  def test_an_escaped_percent_before_arg_is_not_a_placeholder
+    assert_equal '100%%arg', positionalize('100%%arg')
+  end


+Style/Documentation:
+  Exclude:
+    - '**/*_test.rb'
+


wpmobilebot · 2026-07-21T23:30:59Z

Version 27.1 has now entered code-freeze, so the milestone of this PR has been updated to 27.2.

crazytonyli and others added 5 commits July 9, 2026 13:30

mokagio self-assigned this Jul 20, 2026

mokagio added the Siri label Jul 20, 2026

mokagio added this to the 27.1 milestone Jul 20, 2026

mokagio commented Jul 20, 2026

View reviewed changes

Comment thread fastlane/lanes/app_intents_strings_helper_test.rb Outdated

mokagio commented Jul 20, 2026

View reviewed changes

Comment thread fastlane/lanes/app_intents_strings_helper_test.rb Outdated

mokagio and others added 2 commits July 20, 2026 11:42

Remove verbose AI comments

232ed44

Co-authored-by: Gio Lodi <giovanni.lodi42@gmail.com>

mokagio added the Testing Unit and UI Tests and Tooling label Jul 20, 2026

mokagio and others added 2 commits July 20, 2026 19:34

Satisfy Style/Documentation on the test class

732b924

--- Generated with the help of Claude Code, https://claude.ai/code Co-Authored-By: Claude Code Opus 4.8 <noreply@anthropic.com>

mokagio commented Jul 20, 2026

View reviewed changes

mokagio requested a review from crazytonyli July 20, 2026 09:54

mokagio changed the title ~~App Intents: extract the placeholder rewrite into a tested helper~~ Surface issue in apps intents strings localization tooling Jul 20, 2026

mokagio marked this pull request as ready for review July 20, 2026 09:56

mokagio requested a review from a team as a code owner July 20, 2026 09:56

Copilot AI review requested due to automatic review settings July 20, 2026 09:56

Copilot started reviewing on behalf of mokagio July 20, 2026 09:56 View session

mokagio mentioned this pull request Jul 20, 2026

App Intents: extract localizable strings from code at code freeze #25766

Open

Copilot AI reviewed Jul 20, 2026

View reviewed changes

wpmobilebot modified the milestones: 27.1, 27.2 Jul 21, 2026

crazytonyli force-pushed the task/new-siri-integration-simplify-localization branch from d523a8f to 5778339 Compare July 22, 2026 05:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Surface issue in apps intents strings localization tooling#25805

Surface issue in apps intents strings localization tooling#25805
mokagio wants to merge 9 commits into
task/new-siri-integration-simplify-localizationfrom
mokagio/appintents-strings-helper

mokagio commented Jul 20, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

mokagio Jul 20, 2026

Uh oh!

wpmobilebot commented Jul 20, 2026

Uh oh!

wpmobilebot commented Jul 20, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

wpmobilebot commented Jul 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

mokagio commented Jul 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing instructions

Uh oh!

Uh oh!

Uh oh!

mokagio Jul 20, 2026

Choose a reason for hiding this comment

Uh oh!

wpmobilebot commented Jul 20, 2026

Uh oh!

wpmobilebot commented Jul 20, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

wpmobilebot commented Jul 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mokagio commented Jul 20, 2026 •

edited

Loading