Add self-contained AI translation engine (GlotPress replacement) - #15962
Add self-contained AI translation engine (GlotPress replacement)#15962JorgeMucientes wants to merge 38 commits into
Conversation
Generated by 🚫 Danger |
2e9d14f to
d148979
Compare
28a28e2 to
8e50e39
Compare
|
|
8e50e39 to
cb88042
Compare
d148979 to
3e3c5e9
Compare
b97e222 to
4045656
Compare
toupper
left a comment
There was a problem hiding this comment.
Reviewed the 39 changed files (~4.6k LOC added) on top of issue/woomob-translations-p1-locales. The engine is impressively well-scoped — stdlib-only Ruby with 1.4k LOC of test coverage, a deliberate source-only invalidation contract, hard validator gates, and a layered set of rescue paths so a single LLM hiccup never blocks a PR. The "preserved-XML for glotpress-import keys" approach is a smart way to avoid re-serializing 16 locales worth of human work just to introduce the engine.
I'm not blocking on anything; the items below are suggestions and observations for follow-up.
Suggestions
-
Engine#translate_locale(fastlane/ai_translation/lib/woo_ai_translation/engine.rb:84)raises onValidators.xml_well_formedfailure of the just-written file. That's the one place the engine can still die mid-sweep (after writing this locale's file, before reaching the next locale'smanifest.save). The PR description andb6ef4feb25framing both lean on "never crash mid-locale"; this raise undercuts that guarantee for an admittedly rare failure mode (Writer escaping is supposed to prevent it, but adversarial model output could still produce stray controls / lone surrogates that break REXML on the round-trip). Consider turning it into a per-localegate_errorso the rest of the sweep — and the manifest persistence — survive. -
.buildkite/commands/ai-translate-pr.sh:86sanitizes only--in LLM-generated XML comments.'---'.gsub('--', '- -')evaluates to'- --', which still contains--and would produce an invalid XML comment when interpolated into<!-- … -->on line 107. Likewise a description ending in-becomes<!-- foo- -->, which is invalid (XML 1.0 forbids--and--->). Either reject any description that still matches/--/after the first pass, loop until stable, or fall through to no-comment-added (the rest of the pipeline already handles a missing comment gracefully). The same script also doesn't escape&,<,>in the description — these are technically legal inside an XML comment, but the resulting English context would be confusing for a reviewer. -
MetadataEngine#translate_field(fastlane/ai_translation/lib/woo_ai_translation/metadata_engine.rb:91-104) only re-prompts once with the "Be more concise" hint when the field is over cap. If the second attempt is also over, it silently falls back to the unmodified English source and writes that to disk — but the English source itself may exceed the cap (the testtest_metadata_engine_caps_release_notes_gating_and_reuseexercises exactly this: a 60-char title is written as English fallback to atitle.txtwhose cap is 50). The fallback is documented as "English fallback, never over-cap garbage" but in this case it ships an over-cap English file. Consider either keeping the previous on-disk value when fallback would overflow, hard-truncating with a logged warning, or making this a hard gate on the metadata sweep. -
Translator#system_blocksflags only the last system block for prompt caching. The current ordering puts the per-locale style block last, so the cache breakpoint only covers up to glossary/rules. A future maintainer might reorder these or add another block after the per-locale style and quietly tank the cache hit rate. Worth a comment nearcacheable_systeminanthropic_client.rbpinning the contract ("last block is the cache breakpoint; everything before it is shared prefix"), and ideally an assertion in the test that asserts the cache_control is on a constant block. -
bin/merge-manifestsdiscardsdata['metadata']['locales']on key collision (merged['metadata'][k] = voverwrites the whole bucket). For a sharded backfill where each shard touches a disjoint set of locales but the metadata bucket key (e.g."title"or"10.3:release_notes") is shared, the last shard wins and earlier shards' per-locale records are lost. Mirror the per-key locales-deep-merge you already do above for the metadata buckets. -
AnthropicClient#client_error_no_retry?(fastlane/ai_translation/lib/woo_ai_translation/anthropic_client.rb:140-146) parses the status fromerror.message[/HTTP (\d+)/, 1]. The error string is"HTTP #{res.code}: #{res.body}", and a 5xx whose body happens to start withHTTP 4xx ...would be classified as a non-retryable client error. Vanishingly unlikely, but the regex isn't anchored. A one-line guard or a more explicit^HTTP (\d{3})(?=:)would remove the ambiguity. -
AnthropicClient#post_messages(line 106) callsJSON.parse(res.body)outside any rescue. A 2xx with malformed JSON would propagateJSON::ParserErrorpastwith_retries(which only rescuesErrorandTRANSPORT_ERRORS) and crash the entire run, bypassing the Translator'ssplit_retry. The Anthropic API doesn't do this in practice, but the assumption is worth either documenting or hardening. -
engine.rb:283-286runsValidators.glossary_preservationunconditionally on every entry but only runsplaceholder_paritywhenformatted != 'false'. The asymmetry is correct intent (brand-name preservation still applies to literal-percent strings) but worth a one-line code comment so the next reader doesn't "fix" it. -
docs/translation-rollout.md:60-65describessoft_fail: trueas the rollout starting point, but.buildkite/pipeline.yml:53-62ships the job withoutsoft_fail, and the PR description calls this out as intentional ("AI translation Buildkite step is no longer soft-failed"). If this is the deliberate cutover state, consider trimming/updating the Phase 5 wording so the runbook doesn't read as "we are about to start" when we're already there.
Positives
-
Source-only invalidation contract is explicit, documented in three places (
Manifestclass header,docs/localization.mdtable, dedicated teststest_model_bump_does_not_invalidate…,test_attribute_only_change_does_not_invalidate,test_manifest_loss_trusts_existing_localized_files). The "manifest loss does not clobber human work" guarantee is exactly the right escape hatch. -
Hard gates are blocking-by-design and the failure mode is "omit the key, let Android fall back to default" rather than "ship garbage" — both the placeholder gate and glossary gate are tested for this (
test_placeholder_failure_is_dropped_not_shipped,test_glossary_preservation_failure_is_dropped_not_shipped). -
CLDR plural reshaping (
dup_shell_for_locale) genuinely solves the Polish/Thai Lint warnings the old GlotPress path always had, with a CLDR-47-sourced data file and tested per-locale (test_polish_plurals_get_all_four_cldr_quantities_synthesized,test_thai_plurals_drop_irrelevant_one_quantity). -
Anthropic prompt-caching is wired correctly via
cache_control: ephemeralon the last system block, and the temperature-rejection self-learning (@no_temperature_models) is a pragmatic workaround for the Opus 4.7 quirk with a clean test (test_drops_temperature_and_retries_on_deprecation_400,test_remembers_model_and_skips_temperature_on_subsequent_calls). -
The Translator's recoverable-error rescue list deliberately excludes
NoMethodError/SystemCallErrorso genuine engine bugs still surface; that boundary is captured intest_unknown_error_class_still_propagates. Same intent in theAnthropicClient#client_error_no_retry?4xx-except-429 gate. This is exactly the right "fail loud on bugs, fail soft on flakes" line. -
PartialWriterpreserves unrelated existing lines byte-for-byte and only edits selected blocks;test_only_names_preserves_existing_file_order_and_headerpins the contract on a fixture that includes the original GlotPress header. That keeps PR diffs scoped and makes the engine adoption non-destructive to the 16 existing locales. -
BaselineReader+ thepreserved_xmlpath on the Writer means existing human translations can be re-emitted byte-identically as long as their manifest origin staysglotpress-import—EnginePreservedLineIntegrationTestproves a deliberately idiosyncratic baseline spacing survives a full engine run, andtest_ai_origin_keys_still_route_through_render_unitproves AI-origin keys go through the canonical writer. -
Tooling-namespace attributes (
tools:override) round-trip intact viaexpanded_name, with a test that pins the regression (test_tools_namespace_prefix_survives_parse_and_write). Easy thing to miss and would silently break Android Lint suppressions in localized files. -
bin/woo-ai-translateCliShimTestcovers the very specific failure mode of the blanketbin/gitignore rule eating the shim — small but easy to regress. -
The fork-PR / no-secret path in
ai-translate-pr.shcorrectly short-circuits before doing any work, and the bot-commit self-skip (BOT_SKIP_MARKER) prevents the per-PR concurrency-limited job from looping on its own commit. Skip-label bypass is a sensible operator escape hatch. -
Operational concerns are addressed: per-PR concurrency in
.buildkite/pipeline.yml, manifest is saved after each locale to survive Ctrl-C,mac-metalagent restricts where the API key is needed, source encoding test catches stray NUL bytes that previously caused breakage. -
Engine version (
VERSION/PROMPT_VERSION) is recorded in every manifest entry as audit metadata even though it doesn't trigger re-translation — keeps the audit trail without coupling to invalidation, exactly as docs/localization.md describes.
Reviewed on behalf of @toupper — drafted by Claude Code, please confirm before merging.
Stdlib-only Ruby engine under fastlane/ai_translation: order-preserving strings.xml reader/writer, manifest delta cache (sha(source+context+locale+model+prompt_version)), pluggable AINFRA-1707 context seam, Anthropic client with prompt caching + retry/backoff, batched Sonnet/Opus translator, and blocking validation gates (placeholder parity, XML well-formedness, key parity, plural-pair output integrity). Adds an additive `ai_translate` Fastlane lane (GlotPress still present, so shadow-capable) and a minitest suite. Verified offline against the real 3,966-key strings.xml.
Restructures SUPPORTED_LOCALES (drops glotpress codes, adds the 15 new locales with Play codes), repurposes download_translations and download_release_translations onto the AI engine, adds ai_translate / ai_translate_metadata lanes, removes the GlotPress constants, check_translation_progress_* lanes, download_promo_strings and the dead PlayStoreStrings.pot, and guts update_play_store_strings (no more .pot). Adds a metadata translation engine (workstream 3c, char-cap gate + release-notes gating) with tests, and rewrites docs/localization.md (AI pipeline, retained manual plural convention + deferred CLDR pointer, open questions incl. WPCOM cron retirement handoff).
Adds a PR-time AI Translations check in the Linters group: translates the PR's string delta for all locales and a bot commits values-*/strings + manifest back to the PR branch (no-op + [skip ai-translate] marker so it never loops; fork/no-secret builds skip cleanly; non-blocking spot-check comment via comment_on_pr; soft_fail while eased in). Repoints the code-freeze sweep pipeline onto the AI sweep + metadata and adds an AI translation step to the hotfix finalize flow so a hotfix never waits for a PR run.
d2fdc80 to
7aa3f96
Compare
|
Thanks @toupper. I pushed fixes for the critical engine issues you called out. Changed:
Not changed:
|
|
Version |
|
Version |
|
Version |
|
Version |
|
Version |

Summary
Adds the GlotPress-independent translation engine and wires it into WooCommerce Android release/PR automation.
This PR now contains the previously stacked engine/CI/Fastlane hardening work that was manually folded together:
fastlane/ai_translation/.formatted="false", locale plural quantities, and preserved baseline lines.@string/...reference validation, and text normalization.bin/woo-ai-translate/bin/merge-manifestsshims.i18n-context-generator, and an intentionalskip-ai-translationlabel bypass.No generated locale backfill files are added in this PR. The sidecar baseline is in #15974, Play metadata content is in #15983, the 15-locale Android string backfill is in #16004, and the draft PR-time pipeline fixture is #16011.
Blocker and Review Fixes Included
(key, locale), so updating one stale locale no longer makes the other locales for that key look fresh.prtime --strictonly fails on hard gate errors; recoverable per-key translation gaps remain non-blocking and fall back to default resources.<string>,<string-array>, and<plurals>entries; failures are logged and skipped so translation can proceed.Manifest::SEPnow uses an escaped NUL sentinel instead of a literal NUL byte in Ruby source, with a regression test that prevents NUL bytes in the AI translation Ruby files.pt-rPT) CLDR plural coverage now includesmany, matching CLDR 47 cardinal categories.@string/...references that no longer exist in the source resource table.merge-manifestsnow deep-merges metadata locale shards instead of replacing whole metadata buckets.Stack
Base: #15961.
Open stack above this PR:
Merged/folded stack PR:
Full stack map: #15961.
Test Plan
ruby fastlane/ai_translation/spec/woo_ai_translation_test.rb-98 runs / 362 assertions / 0 failures.bash -n .buildkite/commands/ai-translate-pr.shruby -c fastlane/ai_translation/lib/woo_ai_translation/manifest.rbruby -c fastlane/ai_translation/lib/woo_ai_translation/validators.rbruby -c fastlane/ai_translation/lib/woo_ai_translation/engine.rbruby -c fastlane/ai_translation/lib/woo_ai_translation/metadata_engine.rbruby -c fastlane/ai_translation/lib/woo_ai_translation/anthropic_client.rbbundle exec rubocop fastlane/ai_translation --force-exclusion- current repo config excludes this self-contained tool, so RuboCop inspects 0 files.bundle exec i18n-context-generator version-i18n-context-generator 0.4.0.bundle exec i18n-context-generator extract --translations WooCommerce/src/main/res/values/strings.xml --source WooCommerce/src/main --keys settings_usage_tracking --dry-runruby -Ifastlane/ai_translation/lib fastlane/ai_translation/spec/woo_ai_translation_test.rb --name '/comment|context/'git diff --checkruby -c fastlane/Fastfile