ci(i18n): fix the two guards that keep the crowdin sync failing

Two independent guards were failing the workflow on healthy input. Retention read "exported value equals the English source" as a lost translation, but exports are built with skipUntranslatedStrings, so Crowdin omits a key entirely once it stops carrying a translation. Anything present in the export is by definition translated, and the equality check only ever fired on correct work: Spanish "Error", "Feb" and "Cookie", French "Description", Portuguese "Readwise". Retention now compares presence alone, which is what absence already meant. The same inference lived in a legacyComplete branch that would have silently stopped protecting the 2,318 keys whose translation legitimately equals the source once any locale reached full translation. Scheduled runs had no way out of that. The only escape hatch is allowed_translation_losses, a workflow_dispatch input that is never persisted, so every schedule rediscovered the same losses and failed until someone dispatched the run by hand. The slot count rule then only runs for the keys it is given, and the sync passed none, so its rejection filter was weaker than the locale validation running on its output one step later. An invalid ja message survived the filter, reached the catalog, and failed the next step instead of being dropped and reported. Move the source scan into locale-source-keys.mjs so the sync and validate-locales.mjs derive the same keys.
2026-11-03 00:11:37 -06:00 · 2026-08-21 15:36:12 +00:00
parent 93e7c7b6b7
commit 3b9bae7525
5 changed files with 201 additions and 64 deletions
@@ -0,0 +1,42 @@
+import { readdir, readFile } from 'node:fs/promises'
+import path from 'node:path'
+
+const scriptDirectory = import.meta.dirname ?? path.join(process.cwd(), 'scripts')
+const clientRoot = path.resolve(scriptDirectory, '..')
+
+async function sourceFiles(directory) {
+  const entries = await readdir(directory, { withFileTypes: true })
+  const files = await Promise.all(
+    entries.map(async (entry) => {
+      const entryPath = path.join(directory, entry.name)
+      if (entry.isDirectory()) {
+        if (entry.name === '__tests__') return []
+        return sourceFiles(entryPath)
+      }
+      return entry.isFile() && /\.(ts|vue)$/.test(entry.name) && !/\.(spec|test)\.ts$/.test(entry.name) ? [entryPath] : []
+    }),
+  )
+  return files.flat()
+}
+
+export async function collectSourceMessageKeys(sourceDirectory = path.join(clientRoot, 'src')) {
+  const files = await sourceFiles(sourceDirectory)
+  const keys = new Set()
+  const slotCountKeys = new Set()
+
+  for (const file of files) {
+    const source = await readFile(file, 'utf8')
+    for (const match of source.matchAll(/(?<![\w$])(?:\$?t|\$?tc|\$?te)\(\s*(['"])([A-Za-z0-9_.-]+)\1/g)) {
+      keys.add(match[2])
+    }
+    for (const match of source.matchAll(/<i18n-t\b[^>]*\bkeypath\s*=\s*(['"])([A-Za-z0-9_.-]+)\1/g)) {
+      keys.add(match[2])
+    }
+    for (const match of source.matchAll(/<IcuCountText\b[^>]*\bkeypath\s*=\s*(['"])([A-Za-z0-9_.-]+)\1/g)) {
+      keys.add(match[2])
+      slotCountKeys.add(match[2])
+    }
+  }
+
+  return { keys, slotCountKeys }
+}
@@ -5,6 +5,7 @@ import { pathToFileURL } from 'node:url'
 import path from 'node:path'
 import { TARGET_CATALOGS, assertCrowdinTargetConfiguration } from './locale-configuration.mjs'
 import { findInvalidTargetMessages, flattenCatalog, validateCatalogs } from './locale-catalog-validation.mjs'
+import { collectSourceMessageKeys } from './locale-source-keys.mjs'

 const API = 'https://api.crowdin.com/api/v2'
 const SOURCE_PATH_SUFFIX = '/client/src/locales/en.json'
@@ -260,21 +261,17 @@ export function parseAllowedTranslationLosses(value = '') {
  return allowed
 }

+// Exports are built with skipUntranslatedStrings, so Crowdin drops a key entirely once it stops
+// carrying a translation. Retention therefore compares presence, never message content: a translation
+// that equals the English source, such as Spanish "Error" or a product name, is a real translation.
 export function findTranslationLosses({ locale, reference, current, exported, rejected = new Map() }) {
-  const legacyComplete = current.size === reference.size && [...reference.keys()].every((key) => current.has(key))
  const losses = []

-  for (const [key, currentMessage] of current) {
-    const referenceMessage = reference.get(key)
-    if (referenceMessage === undefined || (legacyComplete && currentMessage === referenceMessage)) continue
+  for (const key of current.keys()) {
+    if (!reference.has(key) || exported.has(key)) continue

-    const exportedMessage = exported.get(key)
-    if (exportedMessage === undefined) {
-      const rejection = rejected.get(key)
-      losses.push({ locale, key, reason: rejection ? `rejected by catalog validation - ${rejection[0]}` : 'missing from Crowdin export' })
-    } else if (currentMessage !== referenceMessage && exportedMessage === referenceMessage) {
-      losses.push({ locale, key, reason: 'replaced by English source text' })
-    }
+    const rejection = rejected.get(key)
+    losses.push({ locale, key, reason: rejection ? `rejected by catalog validation - ${rejection[0]}` : 'missing from Crowdin export' })
  }

  return losses
@@ -354,6 +351,7 @@ export async function syncCrowdinTranslations({
  allowedLosses = new Set(),
  targetCatalogs = TARGET_CATALOGS,
  assertTargetConfiguration = assertCrowdinTargetConfiguration,
+  collectMessageKeys = collectSourceMessageKeys,
  reportPath = process.env.CROWDIN_REJECTION_REPORT || '',
 }) {
  if (!token) throw new Error('CROWDIN_TOKEN is required')
@@ -388,14 +386,15 @@ export async function syncCrowdinTranslations({
  const catalogs = new Map([['en', referenceMessages]])
  for (const { locale, catalog } of downloaded) catalogs.set(locale, flattenCatalog(catalog))

-  const rejections = findInvalidTargetMessages({ catalogs })
+  const { slotCountKeys } = await collectMessageKeys()
+  const rejections = findInvalidTargetMessages({ catalogs, slotCountKeys })
  const rejectedLocales = new Set(rejections.map(({ locale }) => locale))
  for (const { locale, key } of rejections) catalogs.get(locale).delete(key)
  for (const entry of downloaded) {
    if (rejectedLocales.has(entry.locale)) entry.catalog = orderedSparseCatalog(reference, catalogs.get(entry.locale))
  }

-  const errors = validateCatalogs({ catalogs })
+  const errors = validateCatalogs({ catalogs, slotCountKeys })
  if (errors.length > 0) throw new Error(`Crowdin export validation failed:\n${errors.join('\n')}`)
  assertTranslationRetention({
    reference: referenceMessages,
@@ -3,6 +3,8 @@ import { tmpdir } from 'node:os'
 import path from 'node:path'
 import { afterEach, describe, expect, it, vi } from 'vitest'
 import { assertCrowdinTargetConfiguration, validateCrowdinTargetConfiguration } from './locale-configuration.mjs'
+import { flattenCatalog, validateCatalogs } from './locale-catalog-validation.mjs'
+import { collectSourceMessageKeys } from './locale-source-keys.mjs'
 import {
  TARGET_CATALOGS,
  assertSafeDownloadUrl,
@@ -26,11 +28,11 @@ afterEach(async () => {
  await Promise.all(temporaryDirectories.splice(0).map((directory) => rm(directory, { recursive: true, force: true })))
 })

-async function createCatalogFixture(targetCatalogs, currentCatalogs = new Map()) {
+async function createCatalogFixture(targetCatalogs, currentCatalogs = new Map(), referenceCatalog = { common: { save: 'Save' } }) {
  const directory = await mkdtemp(path.join(tmpdir(), 'bookorbit-crowdin-sync-'))
  temporaryDirectories.push(directory)
  await mkdir(directory, { recursive: true })
-  await writeFile(path.join(directory, 'en.json'), `${JSON.stringify({ common: { save: 'Save' } }, null, 2)}\n`)
+  await writeFile(path.join(directory, 'en.json'), `${JSON.stringify(referenceCatalog, null, 2)}\n`)
  await Promise.all(
    targetCatalogs.map(({ locale }) =>
      writeFile(path.join(directory, `${locale}.json`), `${JSON.stringify(currentCatalogs.get(locale) ?? {}, null, 2)}\n`),
@@ -84,7 +86,7 @@ describe('Crowdin translation synchronization', () => {
    })
  })

-  it('protects real translations in complete legacy catalogs without treating English fallbacks as translations', () => {
+  it('protects a complete catalog as strictly as a sparse one', () => {
    const referenceMessages = new Map([
      ['common.save', 'Save'],
      ['common.cancel', 'Cancel'],
@@ -97,6 +99,7 @@ describe('Crowdin translation synchronization', () => {

    expect(findTranslationLosses({ locale: 'cs', reference: referenceMessages, current, exported })).toEqual([
      { locale: 'cs', key: 'common.save', reason: 'missing from Crowdin export' },
+      { locale: 'cs', key: 'common.cancel', reason: 'missing from Crowdin export' },
    ])
  })

@@ -112,16 +115,69 @@ describe('Crowdin translation synchronization', () => {
    ])
  })

-  it('rejects translations that Crowdin replaces with English source text', () => {
-    const referenceMessages = new Map([['common.save', 'Save']])
-    const current = new Map([['common.save', 'Uložit']])
-    const exported = new Map([['common.save', 'Save']])
+  it.each([
+    ['es', 'bookDock.tab.error', 'Error', 'error'],
+    ['es', 'book.detail.readingLog.months.feb', 'Feb', 'febrero'],
+    ['es', 'settings.metadata.providers.fields.cookie', 'Cookie', 'galleta'],
+    ['fr', 'settings.metadata.fields.description', 'Description', 'Descriptif'],
+    ['pt', 'settings.integrations.tabs.readwise', 'Readwise', 'Leitura'],
+  ])('keeps %s:%s when Crowdin translates it as the English source text', (locale, key, source, currentMessage) => {
+    expect(
+      findTranslationLosses({
+        locale,
+        reference: new Map([[key, source]]),
+        current: new Map([[key, currentMessage]]),
+        exported: new Map([[key, source]]),
+      }),
+    ).toEqual([])
+  })

-    expect(findTranslationLosses({ locale: 'cs', reference: referenceMessages, current, exported })).toEqual([
-      { locale: 'cs', key: 'common.save', reason: 'replaced by English source text' },
+  it('reports why catalog validation dropped a key instead of a bare absence', () => {
+    expect(
+      findTranslationLosses({
+        locale: 'cs',
+        reference: new Map([['common.save', 'Save']]),
+        current: new Map([['common.save', 'Uložit']]),
+        exported: new Map(),
+        rejected: new Map([['common.save', ['cs: Unicode em dash is not allowed in common.save']]]),
+      }),
+    ).toEqual([
+      {
+        locale: 'cs',
+        key: 'common.save',
+        reason: 'rejected by catalog validation - cs: Unicode em dash is not allowed in common.save',
+      },
    ])
  })

+  it('ignores catalog keys that English no longer defines', () => {
+    expect(
+      findTranslationLosses({
+        locale: 'cs',
+        reference: new Map([['common.save', 'Save']]),
+        current: new Map([['common.removed', 'Odstraněno']]),
+        exported: new Map(),
+      }),
+    ).toEqual([])
+  })
+
+  it('needs no acknowledgement when Crowdin corrects a translation to the English source', () => {
+    const referenceMessages = new Map([['bookDock.tab.error', 'Error']])
+    const currentCatalogs = new Map(TARGET_CATALOGS.map(({ locale }) => [locale, new Map()]))
+    const exportedCatalogs = new Map(TARGET_CATALOGS.map(({ locale }) => [locale, new Map([['bookDock.tab.error', 'Error']])]))
+    currentCatalogs.set('es', new Map([['bookDock.tab.error', 'error']]))
+
+    expect(() => assertTranslationRetention({ reference: referenceMessages, currentCatalogs, exportedCatalogs })).not.toThrow()
+    expect(() =>
+      assertTranslationRetention({
+        reference: referenceMessages,
+        currentCatalogs,
+        exportedCatalogs,
+        allowedLosses: parseAllowedTranslationLosses('es:bookDock.tab.error'),
+      }),
+    ).toThrow('es:bookDock.tab.error - acknowledgement does not match an exported loss')
+  })
+
  it('requires exact acknowledgements for intentional translation losses', () => {
    const referenceMessages = new Map([['common.save', 'Save']])
    const currentCatalogs = new Map(TARGET_CATALOGS.map(({ locale }) => [locale, new Map()]))
@@ -154,7 +210,7 @@ describe('Crowdin translation synchronization', () => {
    expect(() => parseAllowedTranslationLosses('cs:common.save,cs:common.save')).toThrow('Duplicate translation loss acknowledgement')
  })

-  it('requests untranslated-string omission and downloads the bounded export', async () => {
+  it('omits untranslated strings from the export so retention can read absence as loss', async () => {
    const fetchImpl = vi
      .fn()
      .mockResolvedValueOnce(
@@ -354,4 +410,81 @@ describe('Crowdin translation synchronization', () => {
    ).rejects.toThrow('missing from Crowdin export')
    await expect(access(outputDirectory)).rejects.toThrow()
  })
+
+  it('writes a Crowdin correction that replaces a translation with the English source text', async () => {
+    const targetCatalogs = TARGET_CATALOGS.slice(0, 2)
+    const currentCatalogs = new Map([[targetCatalogs[0].locale, { common: { save: 'Ulozit' } }]])
+    const catalogDirectory = await createCatalogFixture(targetCatalogs, currentCatalogs)
+    const outputDirectory = path.join(catalogDirectory, 'output')
+    const catalogs = new Map([[targetCatalogs[0].languageId, { common: { save: 'Save' } }]])
+
+    await expect(
+      syncCrowdinTranslations({
+        token: 'secret',
+        fetchImpl: createSynchronizationFetch({ catalogs }),
+        catalogDirectory,
+        outputDirectory,
+        targetCatalogs,
+        assertTargetConfiguration: async () => {},
+      }),
+    ).resolves.toEqual({ rejections: [] })
+    expect(JSON.parse(await readFile(path.join(outputDirectory, `${targetCatalogs[0].locale}.json`), 'utf8'))).toEqual({
+      common: { save: 'Save' },
+    })
+  })
+
+  it('drops a slot count violation so the catalog it writes passes locale validation', async () => {
+    const key = 'tools.bulkRename.confirmDialog.body'
+    const referenceCatalog = { tools: { bulkRename: { confirmDialog: { body: '{count, plural, one {Rename # file} other {Rename # files}}' } } } }
+    const targetCatalogs = TARGET_CATALOGS.slice(0, 2)
+    const catalogDirectory = await createCatalogFixture(targetCatalogs, new Map(), referenceCatalog)
+    const outputDirectory = path.join(catalogDirectory, 'output')
+    const catalogs = new Map([
+      [
+        targetCatalogs[0].languageId,
+        { tools: { bulkRename: { confirmDialog: { body: '{count, plural, one {Rename # file} few {Rename # files} other {Rename files}}' } } } },
+      ],
+      [targetCatalogs[1].languageId, {}],
+    ])
+
+    const { rejections } = await syncCrowdinTranslations({
+      token: 'secret',
+      fetchImpl: createSynchronizationFetch({ identifiers: [key], catalogs }),
+      catalogDirectory,
+      outputDirectory,
+      targetCatalogs,
+      assertTargetConfiguration: async () => {},
+      collectMessageKeys: async () => ({ keys: new Set([key]), slotCountKeys: new Set([key]) }),
+    })
+
+    expect(rejections).toEqual([
+      {
+        locale: targetCatalogs[0].locale,
+        key,
+        errors: [`${targetCatalogs[0].locale}: slot count message ${key} must render exactly one # in every branch`],
+      },
+    ])
+    expect(JSON.parse(await readFile(path.join(outputDirectory, `${targetCatalogs[0].locale}.json`), 'utf8'))).toEqual({})
+
+    const written = new Map([['en', flattenCatalog(referenceCatalog)]])
+    for (const { locale } of targetCatalogs) {
+      written.set(locale, flattenCatalog(JSON.parse(await readFile(path.join(outputDirectory, `${locale}.json`), 'utf8'))))
+    }
+    expect(validateCatalogs({ catalogs: written, slotCountKeys: new Set([key]) })).toEqual([])
+  })
+
+  it('scans the same sources locale validation scans', async () => {
+    const directory = await mkdtemp(path.join(tmpdir(), 'bookorbit-locale-source-'))
+    temporaryDirectories.push(directory)
+    await mkdir(path.join(directory, '__tests__'), { recursive: true })
+    await writeFile(path.join(directory, 'Widget.vue'), '<template><IcuCountText keypath="tools.bulkRename.confirmDialog.body" /></template>')
+    await writeFile(path.join(directory, 'helper.ts'), "export const label = t('common.save')\n")
+    await writeFile(path.join(directory, 'helper.spec.ts'), "t('ignored.spec')\n")
+    await writeFile(path.join(directory, '__tests__', 'helper.ts'), "t('ignored.tests')\n")
+
+    await expect(collectSourceMessageKeys(directory)).resolves.toEqual({
+      keys: new Set(['tools.bulkRename.confirmDialog.body', 'common.save']),
+      slotCountKeys: new Set(['tools.bulkRename.confirmDialog.body']),
+    })
+  })
 })
@@ -3,48 +3,11 @@ import { fileURLToPath } from 'node:url'
 import path from 'node:path'
 import { SUPPORTED_LOCALES } from './locale-configuration.mjs'
 import { flattenCatalog, validateCatalogs } from './locale-catalog-validation.mjs'
+import { collectSourceMessageKeys } from './locale-source-keys.mjs'

 const clientRoot = fileURLToPath(new URL('..', import.meta.url))
 const localesDirectory = path.join(clientRoot, 'src/locales')

-async function sourceFiles(directory) {
-  const entries = await readdir(directory, { withFileTypes: true })
-  const files = await Promise.all(
-    entries.map(async (entry) => {
-      const entryPath = path.join(directory, entry.name)
-      if (entry.isDirectory()) {
-        if (entry.name === '__tests__') return []
-        return sourceFiles(entryPath)
-      }
-      return entry.isFile() && /\.(ts|vue)$/.test(entry.name) && !/\.(spec|test)\.ts$/.test(entry.name) ? [entryPath] : []
-    }),
-  )
-  return files.flat()
-}
-
-async function sourceMessageKeys() {
-  const sourceDirectory = path.join(clientRoot, 'src')
-  const files = await sourceFiles(sourceDirectory)
-  const keys = new Set()
-  const slotCountKeys = new Set()
-
-  for (const file of files) {
-    const source = await readFile(file, 'utf8')
-    for (const match of source.matchAll(/(?<![\w$])(?:\$?t|\$?tc|\$?te)\(\s*(['"])([A-Za-z0-9_.-]+)\1/g)) {
-      keys.add(match[2])
-    }
-    for (const match of source.matchAll(/<i18n-t\b[^>]*\bkeypath\s*=\s*(['"])([A-Za-z0-9_.-]+)\1/g)) {
-      keys.add(match[2])
-    }
-    for (const match of source.matchAll(/<IcuCountText\b[^>]*\bkeypath\s*=\s*(['"])([A-Za-z0-9_.-]+)\1/g)) {
-      keys.add(match[2])
-      slotCountKeys.add(match[2])
-    }
-  }
-
-  return { keys, slotCountKeys }
-}
-
 const localeFiles = (await readdir(localesDirectory)).filter((file) => file.endsWith('.json')).sort()
 const expectedFiles = SUPPORTED_LOCALES.map((locale) => `${locale}.json`).sort()
 if (JSON.stringify(localeFiles) !== JSON.stringify(expectedFiles)) {
@@ -60,7 +23,7 @@ for (const locale of SUPPORTED_LOCALES) {
 const reference = catalogs.get('en')
 if (!reference) throw new Error('English reference catalog is required')

-const { keys: referencedKeys, slotCountKeys } = await sourceMessageKeys()
+const { keys: referencedKeys, slotCountKeys } = await collectSourceMessageKeys()
 const errors = validateCatalogs({ catalogs, referencedKeys, slotCountKeys })

 if (errors.length > 0) {
@@ -99,7 +99,7 @@ After an English source change reaches `main`:
 2. Translators update target strings in Crowdin.
 3. The scheduled or manually dispatched `Crowdin Translation Sync` workflow verifies that Crowdin's source keys match `en.json`.
 4. The workflow requests only translated strings, removes the empty values Crowdin emits for untranslated nested JSON entries, and validates the resulting sparse catalogs.
-5. Before writing, the workflow rejects exports that omit an existing translation or replace one with English source text.
+5. Before writing, the workflow rejects exports that omit an existing translation.
 6. Only after validation and retention checks pass, the workflow updates `l10n_main` and opens a pull request to `main`.
 7. CI verifies that the pull request changes only the twenty-four target catalogs and runs the normal client checks.
 8. A maintainer reviews and squash-merges the pull request.
@@ -109,7 +109,7 @@ Deleting that branch is required, not tidiness. It guarantees that every later e

 Translation pull requests retain the configured `i18n(client)` title and commit format. The `i18n` commit type produces a patch release and an Internationalization release-note section. `CROWDIN_PR_TOKEN` must contain a fine-grained GitHub token with repository contents and pull-request write access. A separate token is required because pull requests created with the workflow's default `GITHUB_TOKEN` do not trigger normal pull-request workflows.

-The retention check distinguishes the initial complete legacy catalogs from later sparse catalogs. In a complete legacy catalog, it protects values that differ from English and ignores copied English fallbacks. Once a catalog is sparse, every existing key is treated as an intentional translation, including technical terms that legitimately match English.
+The retention check compares presence, never message content. Exports request only translated strings, so Crowdin omits a key as soon as it stops carrying a translation, and every key already present in a target catalog must come back in the export. A translation that legitimately matches the English source, such as `Error` in Spanish or a product name left untranslated, is a real translation and never counts as a loss. Reading it as one would reject the routine corrections that translators make to short labels, abbreviations, and loanwords.

 An intentional removal requires a manual workflow dispatch with its exact `locale:message.key` value in `allowed_translation_losses`. Separate multiple acknowledgements with commas. Unknown, misspelled, duplicate, and unused acknowledgements fail the run, so this input cannot act as a broad bypass. Scheduled runs never acknowledge translation loss automatically.