From 2b47c626654a172ba388116b510f5882e80a97cd Mon Sep 17 00:00:00 2001 From: Steven Syrek Date: Wed, 29 Jul 2026 13:47:44 -0400 Subject: [PATCH 01/27] docs: fix high-severity errors from the pre-release documentation audit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - README Development prerequisites said Node >= 20.19.0; engines.node is >=24.0.0 - SYNC.md GitLab CI recipe pinned node:20, which cannot install or run @deepl/cli - README claimed 429 is not retried; the client retries with Retry-After/jittered backoff - API.md advertised versions 1.2.0/1.1.0 and stale dates in header and footer - API.md cited nonexistent 'deepl auth test' and 'deepl config unset' as exit-code sources - write supported-language lists omitted ja, ko, zh, zh-Hans in API.md and README - sync init alias-deprecation prose rewritten for the 2.0 cut (aliases removed) - CHANGELOG exit-codes cross-link pointed at API.md instead of docs/API.md 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- CHANGELOG.md | 2 +- README.md | 11 ++++++++--- docs/API.md | 18 +++++++++++------- docs/SYNC.md | 6 +++--- 4 files changed, 23 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 903b416..29cb70a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -159,7 +159,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `deepl tm list` subcommand — lists all translation memories on the account, mirroring `deepl glossary list`. Text output filters control chars and zero-width codepoints from TM names so a malicious API-returned name cannot corrupt the terminal; `--format json` emits the raw `TranslationMemory[]` as returned by `GET /v3/translation_memories`. Help text on `deepl translate --translation-memory` now cross-references the new command - `src/utils/uuid.ts` — shared strict UUID regex (`UUID_RE`) + `validateUuid` / `validateTranslationMemoryId` helpers. `validateTranslationMemoryId` is dormant today (TM IDs only appear in `/v2/translate` POST bodies, which are JSON-escaped) but guards the path-injection surface the moment any future per-TM endpoint interpolates a user-supplied UUID into a URL segment - **sync**: `deepl sync resolve` now prints a per-entry decision report (`kept ours` / `kept theirs` / `length-heuristic` / `unresolved`) plus a summary, and accepts `--dry-run` to preview decisions without writing the lockfile. -- **sync docs**: `docs/SYNC.md` Exit Codes table and `docs/API.md` sync Behavior bullet now cross-link to the canonical [Exit Codes](API.md#exit-codes) appendix. +- **sync docs**: `docs/SYNC.md` Exit Codes table and `docs/API.md` sync Behavior bullet now cross-link to the canonical [Exit Codes](docs/API.md#exit-codes) appendix. - **sync**: New `sync.max_scan_files` config key (default 50,000). - **errors**: `SyncConflictError` class in `src/utils/errors.ts` mirroring `SyncDriftError` — `ExitCode.SyncConflict` (11) is now throwable as a typed error so library consumers can `instanceof`-match the conflict case. - **SECURITY.md**: `1.1.x` row added to the Supported Versions table. diff --git a/README.md b/README.md index 5928420..830302b 100644 --- a/README.md +++ b/README.md @@ -650,9 +650,13 @@ deepl write "Fresh improvement please." --lang en-US --no-cache - Spanish (`es`) - French (`fr`) - Italian (`it`) +- Japanese (`ja`) +- Korean (`ko`) - Portuguese (`pt`) - generic, defaults to Brazilian Portuguese - Portuguese - Brazilian (`pt-BR`) - Portuguese - European (`pt-PT`) +- Chinese (`zh`) - generic, defaults to Simplified Chinese +- Chinese - Simplified (`zh-Hans`) **Writing Styles:** @@ -1072,8 +1076,9 @@ deepl translate "Hello" --to es deepl translate "Hello" --to es # Fails immediately without retries on auth errors -# Rate limiting (429) - does not retry -# You may want to wait before retrying manually +# Rate limiting (429) - retried automatically +# Honors the Retry-After header when the server sends one, +# otherwise falls back to jittered exponential backoff ``` **Note:** Retry and timeout settings use sensible defaults optimized for the DeepL API. These are internal features that work automatically - no configuration required. @@ -1331,7 +1336,7 @@ Cache location: `~/.cache/deepl-cli/cache.db` (or `~/.deepl-cli/cache.db` for le ### Prerequisites -- Node.js >= 20.19.0 +- Node.js >= 24.0.0 - npm >= 9.0.0 - DeepL API key diff --git a/docs/API.md b/docs/API.md index f7a6aea..71076a2 100644 --- a/docs/API.md +++ b/docs/API.md @@ -1,7 +1,7 @@ # DeepL CLI - API Reference -**Version**: 1.2.0 -**Last Updated**: April 25, 2026 +**Version**: 2.0.0 +**Last Updated**: July 29, 2026 Complete reference for all DeepL CLI commands, options, and configuration. @@ -730,9 +730,13 @@ When `--style` or `--tone` is set for a target language that does not support it - `es` - Spanish - `fr` - French - `it` - Italian +- `ja` - Japanese +- `ko` - Korean - `pt` - Portuguese (generic, defaults to Brazilian Portuguese) - `pt-BR` - Brazilian Portuguese - `pt-PT` - European Portuguese +- `zh` - Chinese (generic, defaults to Simplified Chinese) +- `zh-Hans` - Simplified Chinese #### Examples @@ -1134,7 +1138,7 @@ Interactive setup wizard that creates `.deepl-sync.yaml` by scanning the project - `--path GLOB` - Source file path or glob pattern - `--sync-config PATH` - Path to `.deepl-sync.yaml` -`--source-lang` and `--target-langs` are accepted as deprecated aliases for one minor release and emit a stderr warning; they will be removed in the next major release. `deepl translate --target-lang` is unchanged — it operates on strings and stays aligned with the DeepL API's wire name. +`--source-lang` and `--target-langs` were accepted as deprecated aliases during `1.x` and were removed in `2.0.0`; use `--source-locale` / `--target-locales`. `deepl translate --target-lang` is unchanged — it operates on strings and stays aligned with the DeepL API's wire name. **Examples:** @@ -3071,7 +3075,7 @@ Unclassified failure: emitted when an error escapes every typed handler and matc Authentication failed or no API key is available. Emitted by: -- `deepl auth set-key`, `deepl auth test` when the key cannot be validated +- `deepl auth set-key` when the key cannot be validated - Every command that touches the API (`translate`, `write`, `voice`, `glossary`, `usage`, `sync`, `tm list`, `admin`, etc.) when `DEEPL_API_KEY` is unset and no key is in the config file - HTTP 401/403 responses from the DeepL API @@ -3113,7 +3117,7 @@ Remediation: re-read the command's `--help` and the relevant section of this API The configuration file or a configuration value is invalid. Emitted by: - `deepl config set` with a key that is not in the schema, or a value that fails validation (invalid language code, invalid formality, invalid output format, non-positive cache size, non-HTTPS `baseUrl`, path-traversal attempts) -- `deepl config get/unset` with a malformed key +- `deepl config get` with a malformed key - Any command that loads the config file when the file fails to parse, is missing a required field, or specifies an unsupported version - `sync` when `.deepl-sync.yaml` is missing required fields, has invalid locales, or declares an unsupported version - `sync push` / `sync pull` when the remote TMS returns 401/403 (surfaced as `ConfigError` with a hint to check `TMS_API_KEY` / `TMS_TOKEN` and the relevant YAML fields) @@ -3230,5 +3234,5 @@ deepl write --check README.md --- -**Last Updated**: April 20, 2026 -**DeepL CLI Version**: 1.1.0 +**Last Updated**: July 29, 2026 +**DeepL CLI Version**: 2.0.0 diff --git a/docs/SYNC.md b/docs/SYNC.md index 066c9ee..1e1f94e 100644 --- a/docs/SYNC.md +++ b/docs/SYNC.md @@ -476,7 +476,7 @@ deepl sync init [OPTIONS] | `--path ` | Source file path or glob pattern | | `--sync-config ` | Path to `.deepl-sync.yaml` (default: auto-detect) | -`--source-lang` and `--target-langs` remain accepted as deprecated aliases for one minor release and emit a stderr deprecation warning; they will be removed in the next major release. The `--locale` filter on `sync push` / `pull` / `status` / `export` is unchanged. `deepl translate --target-lang` is unchanged — it operates on strings and stays aligned with the DeepL API's wire name. +`--source-lang` and `--target-langs` were accepted as deprecated aliases during `1.x` and were removed in `2.0.0`; use `--source-locale` and `--target-locales`. The `--locale` filter on `sync push` / `pull` / `status` / `export` is unchanged. `deepl translate --target-lang` is unchanged — it operates on strings and stays aligned with the DeepL API's wire name. **Examples:** @@ -793,7 +793,7 @@ Each target locale's approved dictionary is fetched exactly once per `sync pull` ### Worked example -`deepl sync init` currently accepts `--source-lang` and `--target-langs` as deprecated aliases for `--source-locale` and `--target-locales` (shipped via commit `d65cbb3`). Both aliases emit a stderr deprecation warning pointing at the replacement flag. Under the policy above, these aliases will be removed at the `2.0` cut — this is confirmed, and the removal will not slip into any `1.x` release. See the [`deepl sync init`](#deepl-sync-init) section for the current accepted synopsis. +`deepl sync init` accepted `--source-lang` and `--target-langs` as deprecated aliases for `--source-locale` and `--target-locales` throughout `1.x`; both emitted a stderr deprecation warning pointing at the replacement flag. Per the policy above, the aliases were removed at the `2.0` cut — since `2.0.0` they fail as unknown options. See the [`deepl sync init`](#deepl-sync-init) section for the current accepted synopsis. ## CI/CD Integration @@ -861,7 +861,7 @@ The `.deepl-sync.lock` file is only staged when this sync run actually wrote an ```yaml i18n-check: stage: test - image: node:20 + image: node:24 script: - npm install -g @deepl/cli - deepl sync --frozen From 1a100d71f9aa1f90d5c3987b7a2c3417fc2ba4be Mon Sep 17 00:00:00 2001 From: Steven Syrek Date: Wed, 29 Jul 2026 13:49:13 -0400 Subject: [PATCH 02/27] fix(cache): seed the in-memory enabled flag from config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CacheService always started enabled, so `cache stats` reported the process-local default rather than the persisted `cache.enabled` value and could never print "disabled" — not even after `config set cache.enabled false`, which the translation path does honour. CacheServiceOptions gains `enabled`, and the CLI's option factory (now `resolveCacheOptions`) reads `cache.enabled` alongside `cache.ttl` and `cache.maxSize`. Deriving it there rather than inside CacheService keeps `--config ` overrides effective, since that flag replaces the ConfigService the factory closes over. --- src/cli/cache-loader.ts | 17 +++++++++++++++ src/cli/index.ts | 15 ++++--------- src/storage/cache.ts | 6 +++++- tests/unit/cache-loader.test.ts | 32 ++++++++++++++++++++++++++- tests/unit/cache-service.test.ts | 37 ++++++++++++++++++++++++++++++++ 5 files changed, 94 insertions(+), 13 deletions(-) diff --git a/src/cli/cache-loader.ts b/src/cli/cache-loader.ts index 0d58030..3dbbf7c 100644 --- a/src/cli/cache-loader.ts +++ b/src/cli/cache-loader.ts @@ -7,12 +7,29 @@ */ import type { CacheService, CacheServiceOptions } from '../storage/cache.js'; +import type { ConfigService } from '../storage/config.js'; import { Logger } from '../utils/logger.js'; import { errorMessage } from '../utils/error-message.js'; import { isNativeModuleLoadError } from '../utils/native-module-error.js'; type CacheModule = Pick; +/** + * Derive cache-service options from persisted config. Reading `cache.enabled` + * here is what keeps the service's in-memory flag from diverging from config: + * `cache stats` and the translation path then agree on the same value. + */ +export function resolveCacheOptions(config: ConfigService, dbPath: string): CacheServiceOptions { + const ttlSeconds = config.getValue('cache.ttl'); + return { + dbPath, + // Config TTL is in seconds, CacheService expects milliseconds + ttl: ttlSeconds !== undefined ? ttlSeconds * 1000 : undefined, + maxSize: config.getValue('cache.maxSize'), + enabled: config.getValue('cache.enabled'), + }; +} + export function createCacheServiceGetter( getOptions: () => CacheServiceOptions, importCacheModule: () => Promise = () => import('../storage/cache.js'), diff --git a/src/cli/index.ts b/src/cli/index.ts index b7670ad..8f208db 100644 --- a/src/cli/index.ts +++ b/src/cli/index.ts @@ -11,7 +11,7 @@ import { readFileSync } from 'fs'; import { fileURLToPath } from 'url'; import { dirname, join, resolve, isAbsolute, extname } from 'path'; import { ConfigService } from '../storage/config.js'; -import { createCacheServiceGetter } from './cache-loader.js'; +import { createCacheServiceGetter, resolveCacheOptions } from './cache-loader.js'; import { resolvePaths } from '../utils/paths.js'; import type { DeepLClient } from '../api/deepl-client.js'; import { Logger } from '../utils/logger.js'; @@ -59,16 +59,9 @@ const paths = resolvePaths(); // Create config service - can be overridden by --config flag let configService = new ConfigService(paths.configFile); -const getCacheService = createCacheServiceGetter(() => { - const configTtl = configService.getValue('cache.ttl'); - const configMaxSize = configService.getValue('cache.maxSize'); - return { - dbPath: paths.cacheFile, - // Config TTL is in seconds, CacheService expects milliseconds - ttl: configTtl !== undefined ? configTtl * 1000 : undefined, - maxSize: configMaxSize, - }; -}); +const getCacheService = createCacheServiceGetter(() => + resolveCacheOptions(configService, paths.cacheFile), +); /** * Handle error and exit with appropriate exit code diff --git a/src/storage/cache.ts b/src/storage/cache.ts index cfa85b8..dd6ef87 100644 --- a/src/storage/cache.ts +++ b/src/storage/cache.ts @@ -16,6 +16,7 @@ export interface CacheServiceOptions { maxSize?: number; // in bytes ttl?: number; // in milliseconds, 0 = disabled busyTimeoutMs?: number; // how long SQLite waits on a locked DB before erroring + enabled?: boolean; // starting state of the cache; defaults to true } export interface CacheStats { @@ -78,7 +79,9 @@ export class CacheService { private maxSize: number; private ttl: number; private busyTimeoutMs: number; - private enabled: boolean = true; + // Seeded from options so the in-memory flag matches the persisted + // `cache.enabled` config; a bare `new CacheService()` stays enabled. + private enabled: boolean; private isClosed: boolean = false; // Seeded to 0 so the first operation of every process sweeps expired rows; // seeding to Date.now() would keep the sweep from ever running in a process @@ -95,6 +98,7 @@ export class CacheService { this.maxSize = options.maxSize ?? DEFAULT_MAX_SIZE; this.ttl = options.ttl ?? DEFAULT_TTL; this.busyTimeoutMs = options.busyTimeoutMs ?? DEFAULT_BUSY_TIMEOUT_MS; + this.enabled = options.enabled ?? true; try { this.openDatabase(dbPath); diff --git a/tests/unit/cache-loader.test.ts b/tests/unit/cache-loader.test.ts index 9c15a57..0914dc3 100644 --- a/tests/unit/cache-loader.test.ts +++ b/tests/unit/cache-loader.test.ts @@ -14,9 +14,14 @@ jest.mock('../../src/utils/logger', () => ({ }, })); -import { createCacheServiceGetter } from '../../src/cli/cache-loader'; +import { createCacheServiceGetter, resolveCacheOptions } from '../../src/cli/cache-loader'; import { Logger } from '../../src/utils/logger'; import type { CacheService } from '../../src/storage/cache'; +import type { ConfigService } from '../../src/storage/config'; + +function fakeConfig(values: Record): ConfigService { + return { getValue: (key: string) => values[key] } as unknown as ConfigService; +} function makeError(message: string, code?: string): Error { const error = new Error(message); @@ -26,6 +31,31 @@ function makeError(message: string, code?: string): Error { return error; } +describe('resolveCacheOptions', () => { + it('carries cache.enabled through to the cache service', () => { + expect(resolveCacheOptions(fakeConfig({ 'cache.enabled': false }), '/tmp/cache.db').enabled).toBe(false); + expect(resolveCacheOptions(fakeConfig({ 'cache.enabled': true }), '/tmp/cache.db').enabled).toBe(true); + }); + + it('converts the configured TTL from seconds to milliseconds', () => { + const options = resolveCacheOptions(fakeConfig({ 'cache.ttl': 90 }), '/tmp/cache.db'); + expect(options.ttl).toBe(90_000); + }); + + it('passes the db path and max size through unchanged', () => { + const options = resolveCacheOptions(fakeConfig({ 'cache.maxSize': 4096 }), '/tmp/cache.db'); + expect(options.dbPath).toBe('/tmp/cache.db'); + expect(options.maxSize).toBe(4096); + }); + + it('leaves unset keys undefined so the service defaults apply', () => { + const options = resolveCacheOptions(fakeConfig({}), '/tmp/cache.db'); + expect(options.ttl).toBeUndefined(); + expect(options.maxSize).toBeUndefined(); + expect(options.enabled).toBeUndefined(); + }); +}); + describe('createCacheServiceGetter', () => { beforeEach(() => { jest.clearAllMocks(); diff --git a/tests/unit/cache-service.test.ts b/tests/unit/cache-service.test.ts index 5dfb3f0..cd63d4f 100644 --- a/tests/unit/cache-service.test.ts +++ b/tests/unit/cache-service.test.ts @@ -294,6 +294,43 @@ describe('CacheService', () => { }); }); + describe('enabled option', () => { + it('should default to enabled when the option is omitted', () => { + expect(cacheService.stats().enabled).toBe(true); + }); + + it('should start disabled when constructed with enabled: false', () => { + const disabled = new CacheService({ dbPath: path.join(testCacheDir, 'disabled.db'), enabled: false }); + try { + expect(disabled.stats().enabled).toBe(false); + } finally { + disabled.close(); + } + }); + + it('should not read or write entries when constructed disabled', () => { + const disabled = new CacheService({ dbPath: path.join(testCacheDir, 'disabled-io.db'), enabled: false }); + try { + disabled.set('test', { text: 'Hello' }); + expect(disabled.get('test')).toBeNull(); + expect(disabled.stats().entries).toBe(0); + } finally { + disabled.close(); + } + }); + + it('should allow enable() to override a disabled start', () => { + const disabled = new CacheService({ dbPath: path.join(testCacheDir, 'reenable.db'), enabled: false }); + try { + disabled.enable(); + disabled.set('test', { text: 'Hello' }); + expect(disabled.get('test')).toEqual({ text: 'Hello' }); + } finally { + disabled.close(); + } + }); + }); + describe('enable() / disable()', () => { it('should enable cache', () => { cacheService.disable(); From d003dc406774869f06f73bb4493a591b4d2e44b4 Mon Sep 17 00:00:00 2001 From: Steven Syrek Date: Wed, 29 Jul 2026 13:49:22 -0400 Subject: [PATCH 03/27] fix(hooks): resolve the effective hooks directory and stop clobbering backups MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolve the hooks directory with `git rev-parse --git-path hooks` so `deepl hooks install` honours core.hooksPath (husky) and works when `.git` is a gitdir pointer file (linked worktrees, submodules) instead of writing to a `/hooks` path git never reads. A repeat install no longer overwrites an existing `.backup`: the second backup lands in a free numbered slot, and install now returns the hook and backup paths so the CLI can print where the previous hook went. Also resolve findGitRoot's start path to an absolute path — a relative path made the parent-directory walk loop forever. --- src/cli/commands/hooks.ts | 12 +- src/services/git-hooks.ts | 76 ++++++- .../git-hooks-resolution.integration.test.ts | 196 ++++++++++++++++++ tests/unit/hooks-command.test.ts | 23 +- 4 files changed, 299 insertions(+), 8 deletions(-) create mode 100644 tests/integration/git-hooks-resolution.integration.test.ts diff --git a/src/cli/commands/hooks.ts b/src/cli/commands/hooks.ts index 40f9b8f..bdf9134 100644 --- a/src/cli/commands/hooks.ts +++ b/src/cli/commands/hooks.ts @@ -29,9 +29,17 @@ export class HooksCommand { throw new ValidationError('Not in a git repository. Run this command from within a git repository.'); } - this.gitHooksService.install(hookType); + const result = this.gitHooksService.install(hookType); - return chalk.green(`✓ Installed ${hookType} hook`); + const lines = [chalk.green(`✓ Installed ${hookType} hook`)]; + if (result?.hookPath) { + lines.push(chalk.gray(` Path: ${result.hookPath}`)); + } + if (result?.backupPath) { + lines.push(chalk.gray(` Previous hook backed up to: ${result.backupPath}`)); + } + + return lines.join('\n'); } /** diff --git a/src/services/git-hooks.ts b/src/services/git-hooks.ts index 7e85072..2c78588 100644 --- a/src/services/git-hooks.ts +++ b/src/services/git-hooks.ts @@ -6,6 +6,7 @@ import * as fs from 'fs'; import * as path from 'path'; import * as crypto from 'crypto'; +import { execFileSync } from 'child_process'; import { ValidationError } from '../utils/errors.js'; export type HookType = 'pre-commit' | 'pre-push' | 'commit-msg' | 'post-commit'; @@ -14,6 +15,14 @@ export interface HookStatus { [key: string]: boolean; } +export interface InstallResult { + hookPath: string; + /** Path the pre-existing non-DeepL hook was copied to, or null if nothing was backed up. */ + backupPath: string | null; +} + +const MAX_BACKUP_SLOTS = 100; + export interface HookIntegrity { installed: boolean; markerVersion: null | 'legacy' | 1; @@ -34,13 +43,46 @@ export class GitHooksService { throw new ValidationError('Git directory not found: ' + gitDir); } - this.hooksDir = path.join(gitDir, 'hooks'); + this.hooksDir = GitHooksService.resolveHooksDir(gitDir); + } + + /** + * Resolve the directory git actually reads hooks from. + * + * `git rev-parse --git-path hooks` honours core.hooksPath (husky and friends) + * and resolves the `gitdir:` pointer used by linked worktrees and submodules, + * where `/.git` is a file rather than a directory. + */ + private static resolveHooksDir(gitDir: string): string { + const workDir = path.dirname(gitDir); + + try { + const resolved = execFileSync('git', ['rev-parse', '--git-path', 'hooks'], { + cwd: workDir, + encoding: 'utf-8', + stdio: ['ignore', 'pipe', 'ignore'], + }).trim(); + if (resolved) { + return path.resolve(workDir, resolved); + } + } catch { + // git unavailable, or workDir is not a working tree — fall back below. + } + + if (!fs.statSync(gitDir).isDirectory()) { + throw new ValidationError( + `Cannot resolve the hooks directory for "${gitDir}": it is a gitdir pointer file and git is not available to resolve it.`, + 'Install git, or run this command from the repository that owns the worktree.', + ); + } + + return path.join(gitDir, 'hooks'); } /** * Install a git hook */ - install(hookType: HookType): void { + install(hookType: HookType): InstallResult { this.validateHookType(hookType); const hookPath = this.getHookPath(hookType); @@ -52,10 +94,11 @@ export class GitHooksService { } // Backup existing hook if it exists and is not a DeepL hook + let backupPath: string | null = null; if (fs.existsSync(hookPath)) { const existingContent = fs.readFileSync(hookPath, 'utf-8'); if (!this.isDeepLHook(existingContent)) { - const backupPath = hookPath + '.backup'; + backupPath = GitHooksService.nextBackupPath(hookPath); fs.copyFileSync(hookPath, backupPath); } } @@ -65,6 +108,29 @@ export class GitHooksService { // Make it executable fs.chmodSync(hookPath, 0o755); + + return { hookPath, backupPath }; + } + + /** + * Pick a backup path that does not already exist, so a repeat install after + * another tool rewrote the hook cannot destroy the first backup. + */ + private static nextBackupPath(hookPath: string): string { + const primary = hookPath + '.backup'; + if (!fs.existsSync(primary)) { + return primary; + } + for (let slot = 1; slot < MAX_BACKUP_SLOTS; slot++) { + const candidate = `${primary}.${slot}`; + if (!fs.existsSync(candidate)) { + return candidate; + } + } + throw new ValidationError( + `Refusing to install: ${MAX_BACKUP_SLOTS} backups of "${path.basename(hookPath)}" already exist.`, + `Remove the unneeded ${path.basename(primary)}* files and retry.`, + ); } /** @@ -137,7 +203,9 @@ export class GitHooksService { * Find git root directory from current path */ static findGitRoot(startPath?: string): string | null { - let currentPath = startPath ?? process.cwd(); + // Resolve first: path.dirname() of a relative path bottoms out at '.', + // which never equals path.parse().root and loops forever. + let currentPath = path.resolve(startPath ?? process.cwd()); // Traverse up the directory tree while (currentPath !== path.parse(currentPath).root) { diff --git a/tests/integration/git-hooks-resolution.integration.test.ts b/tests/integration/git-hooks-resolution.integration.test.ts new file mode 100644 index 0000000..7e770d2 --- /dev/null +++ b/tests/integration/git-hooks-resolution.integration.test.ts @@ -0,0 +1,196 @@ +/** + * Integration Tests for git hooks directory resolution + * Covers core.hooksPath repos, linked worktrees, submodules and backup safety + * against real git repositories created in temp directories. + */ + +import * as fs from 'fs'; +import * as path from 'path'; +import * as os from 'os'; +import { execFileSync } from 'child_process'; +import { GitHooksService } from '../../src/services/git-hooks.js'; + +function git(cwd: string, ...args: string[]): string { + return execFileSync('git', args, { + cwd, + encoding: 'utf-8', + env: { + ...process.env, + GIT_CONFIG_GLOBAL: '/dev/null', + GIT_CONFIG_SYSTEM: '/dev/null', + GIT_AUTHOR_NAME: 'Test', + GIT_AUTHOR_EMAIL: 'test@example.com', + GIT_COMMITTER_NAME: 'Test', + GIT_COMMITTER_EMAIL: 'test@example.com', + }, + }); +} + +function initRepo(dir: string): void { + fs.mkdirSync(dir, { recursive: true }); + git(dir, 'init', '-q', '.'); +} + +describe('GitHooksService hooks directory resolution', () => { + let tmpRoot: string; + + beforeEach(() => { + tmpRoot = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'deepl-hooks-res-'))); + }); + + afterEach(() => { + fs.rmSync(tmpRoot, { recursive: true, force: true }); + }); + + describe('core.hooksPath', () => { + it('should install into the effective hooks path when core.hooksPath is set', () => { + const repo = path.join(tmpRoot, 'husky-repo'); + initRepo(repo); + fs.mkdirSync(path.join(repo, '.husky', '_'), { recursive: true }); + git(repo, 'config', 'core.hooksPath', '.husky/_'); + + const service = new GitHooksService(path.join(repo, '.git')); + service.install('pre-commit'); + + expect(fs.existsSync(path.join(repo, '.husky', '_', 'pre-commit'))).toBe(true); + expect(fs.existsSync(path.join(repo, '.git', 'hooks', 'pre-commit'))).toBe(false); + }); + + it('should report the effective hooks path from getHookPath', () => { + const repo = path.join(tmpRoot, 'husky-path'); + initRepo(repo); + git(repo, 'config', 'core.hooksPath', '.husky/_'); + + const service = new GitHooksService(path.join(repo, '.git')); + + expect(service.getHookPath('pre-push')).toBe(path.join(repo, '.husky', '_', 'pre-push')); + }); + + it('should have list() reflect hooks installed at the effective path', () => { + const repo = path.join(tmpRoot, 'husky-list'); + initRepo(repo); + git(repo, 'config', 'core.hooksPath', 'hooks-dir'); + + const service = new GitHooksService(path.join(repo, '.git')); + service.install('commit-msg'); + + expect(fs.existsSync(path.join(repo, 'hooks-dir', 'commit-msg'))).toBe(true); + expect(service.list()['commit-msg']).toBe(true); + expect(service.isInstalled('commit-msg')).toBe(true); + }); + + it('should uninstall from the effective hooks path', () => { + const repo = path.join(tmpRoot, 'husky-uninstall'); + initRepo(repo); + git(repo, 'config', 'core.hooksPath', '.husky/_'); + + const service = new GitHooksService(path.join(repo, '.git')); + service.install('pre-commit'); + service.uninstall('pre-commit'); + + expect(fs.existsSync(path.join(repo, '.husky', '_', 'pre-commit'))).toBe(false); + }); + }); + + describe('.git as a file', () => { + it('should install into the shared hooks directory from a linked worktree', () => { + const repo = path.join(tmpRoot, 'main-repo'); + initRepo(repo); + git(repo, 'commit', '-q', '--allow-empty', '-m', 'init'); + const linked = path.join(tmpRoot, 'linked-wt'); + git(repo, 'worktree', 'add', '-q', linked, '-b', 'feature'); + + const gitFile = path.join(linked, '.git'); + expect(fs.statSync(gitFile).isFile()).toBe(true); + + const service = new GitHooksService(gitFile); + expect(() => service.install('pre-commit')).not.toThrow(); + + expect(fs.existsSync(path.join(repo, '.git', 'hooks', 'pre-commit'))).toBe(true); + expect(service.isInstalled('pre-commit')).toBe(true); + }); + + it('should install into the submodule git directory when .git is a pointer file', () => { + const upstream = path.join(tmpRoot, 'upstream'); + initRepo(upstream); + fs.writeFileSync(path.join(upstream, 'README.md'), '# upstream\n'); + git(upstream, 'add', '.'); + git(upstream, 'commit', '-q', '-m', 'init'); + + const parent = path.join(tmpRoot, 'parent'); + initRepo(parent); + git(parent, 'commit', '-q', '--allow-empty', '-m', 'init'); + git(parent, '-c', 'protocol.file.allow=always', 'submodule', 'add', '-q', upstream, 'sub'); + + const subGitFile = path.join(parent, 'sub', '.git'); + expect(fs.statSync(subGitFile).isFile()).toBe(true); + + const service = new GitHooksService(subGitFile); + expect(() => service.install('pre-commit')).not.toThrow(); + + const expected = path.join(parent, '.git', 'modules', 'sub', 'hooks', 'pre-commit'); + expect(fs.existsSync(expected)).toBe(true); + expect(service.isInstalled('pre-commit')).toBe(true); + }); + }); + + describe('backup safety', () => { + it('should not clobber an existing .backup on repeat install', () => { + const repo = path.join(tmpRoot, 'backup-repo'); + initRepo(repo); + const hookPath = path.join(repo, '.git', 'hooks', 'pre-commit'); + fs.writeFileSync(hookPath, '#!/bin/sh\n# original user hook\nexit 0\n'); + + const service = new GitHooksService(path.join(repo, '.git')); + const first = service.install('pre-commit'); + expect(first.backupPath).toBe(hookPath + '.backup'); + expect(fs.readFileSync(hookPath + '.backup', 'utf-8')).toContain('original user hook'); + + // A third-party tool rewrites the hook, then deepl installs again. + fs.writeFileSync(hookPath, '#!/bin/sh\n# husky wrapper\nexit 0\n'); + const second = service.install('pre-commit'); + + expect(fs.readFileSync(hookPath + '.backup', 'utf-8')).toContain('original user hook'); + expect(second.backupPath).not.toBe(hookPath + '.backup'); + expect(second.backupPath).toBeTruthy(); + expect(fs.readFileSync(second.backupPath!, 'utf-8')).toContain('husky wrapper'); + }); + + it('should report no backup path when no pre-existing hook is present', () => { + const repo = path.join(tmpRoot, 'no-backup-repo'); + initRepo(repo); + + const service = new GitHooksService(path.join(repo, '.git')); + const result = service.install('pre-push'); + + expect(result.backupPath).toBeNull(); + expect(result.hookPath).toBe(path.join(repo, '.git', 'hooks', 'pre-push')); + }); + + it('should not create a backup when replacing an existing DeepL hook', () => { + const repo = path.join(tmpRoot, 'reinstall-repo'); + initRepo(repo); + + const service = new GitHooksService(path.join(repo, '.git')); + service.install('pre-commit'); + const second = service.install('pre-commit'); + + expect(second.backupPath).toBeNull(); + expect(fs.existsSync(second.hookPath + '.backup')).toBe(false); + }); + }); + + describe('findGitRoot', () => { + it('should resolve a relative start path without hanging', () => { + const repo = path.join(tmpRoot, 'relative-repo'); + initRepo(repo); + const originalCwd = process.cwd(); + process.chdir(repo); + try { + expect(GitHooksService.findGitRoot('.')).toBe(path.join(fs.realpathSync(repo), '.git')); + } finally { + process.chdir(originalCwd); + } + }); + }); +}); diff --git a/tests/unit/hooks-command.test.ts b/tests/unit/hooks-command.test.ts index be18cd0..86875e1 100644 --- a/tests/unit/hooks-command.test.ts +++ b/tests/unit/hooks-command.test.ts @@ -73,17 +73,36 @@ describe('HooksCommand', () => { describe('install()', () => { it('should install pre-commit hook', () => { - mockGitHooksService.install.mockReturnValue(undefined); + mockGitHooksService.install.mockReturnValue({ + hookPath: '/path/to/.git/hooks/pre-commit', + backupPath: null, + }); const command = new HooksCommand('/path/to/.git'); const result = command.install('pre-commit'); expect(mockGitHooksService.install).toHaveBeenCalledWith('pre-commit'); expect(result).toContain('Installed pre-commit hook'); + expect(result).toContain('/path/to/.git/hooks/pre-commit'); + }); + + it('should report the backup path when an existing hook was backed up', () => { + mockGitHooksService.install.mockReturnValue({ + hookPath: '/path/to/.git/hooks/pre-commit', + backupPath: '/path/to/.git/hooks/pre-commit.backup', + }); + const command = new HooksCommand('/path/to/.git'); + + const result = command.install('pre-commit'); + + expect(result).toContain('backed up to: /path/to/.git/hooks/pre-commit.backup'); }); it('should install pre-push hook', () => { - mockGitHooksService.install.mockReturnValue(undefined); + mockGitHooksService.install.mockReturnValue({ + hookPath: '/path/to/.git/hooks/pre-push', + backupPath: null, + }); const command = new HooksCommand('/path/to/.git'); const result = command.install('pre-push'); From aceb9991fd67dd5d5f679839c7ca7df4b2aec140 Mon Sep 17 00:00:00 2001 From: Steven Syrek Date: Wed, 29 Jul 2026 13:49:22 -0400 Subject: [PATCH 04/27] fix(cache): persist cache enable/disable across processes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `cache disable` printed "✓ Cache disabled" and exited 0, but only flipped an in-memory field in a process that exited immediately. Nothing reached config, so the next invocation still had the cache enabled and translations kept writing entries. `cache enable --max-size` appeared to work only because that path already persisted `cache.maxSize`. Both handlers now write `cache.enabled` to config, which is what TranslationService reads. The in-memory enable()/disable() calls remain for the rest of the current process. The e2e regression test asserts the toggle from a separate CLI invocation: an in-process test would have passed while the bug was live. --- src/cli/commands/cache.ts | 9 ++++---- tests/e2e/cli-cache.e2e.test.ts | 39 ++++++++++++++++++++++++++++++++ tests/unit/cache-command.test.ts | 18 ++++++++++++++- 3 files changed, 61 insertions(+), 5 deletions(-) diff --git a/src/cli/commands/cache.ts b/src/cli/commands/cache.ts index 22d6e67..d8567e8 100644 --- a/src/cli/commands/cache.ts +++ b/src/cli/commands/cache.ts @@ -39,23 +39,24 @@ export class CacheCommand { } /** - * Enable cache with optional max size + * Enable cache with optional max size. + * Persisted to config: the in-memory flag alone would die with this process. */ async enable(maxSize?: number): Promise { - // Set max size if provided if (maxSize !== undefined) { this.config.set('cache.maxSize', maxSize); this.cache.setMaxSize(maxSize); } - // Enable cache + this.config.set('cache.enabled', true); this.cache.enable(); } /** - * Disable cache + * Disable cache. Persisted to config, as with enable(). */ async disable(): Promise { + this.config.set('cache.enabled', false); this.cache.disable(); } diff --git a/tests/e2e/cli-cache.e2e.test.ts b/tests/e2e/cli-cache.e2e.test.ts index fbca295..117a21a 100644 --- a/tests/e2e/cli-cache.e2e.test.ts +++ b/tests/e2e/cli-cache.e2e.test.ts @@ -13,6 +13,45 @@ describe('Cache Command E2E', () => { testConfig.cleanup(); }); + // Each assertion below runs in a process separate from the one that + // toggled the cache, so an in-memory-only toggle cannot satisfy them. + describe('enable/disable persistence across processes', () => { + const toggleConfig = createTestConfigDir('e2e-cache-toggle'); + const toggle = makeNodeRunCLI(toggleConfig.path); + + afterAll(() => { + toggleConfig.cleanup(); + }); + + it('should report disabled from a later invocation after cache disable', () => { + expect(toggle.runCLIAll('cache disable')).toContain('Cache disabled'); + + expect(toggle.runCLI('cache stats')).toContain('Cache Status: disabled'); + expect(toggle.runCLI('config get cache.enabled').trim()).toBe('false'); + }); + + it('should report enabled from a later invocation after cache enable', () => { + toggle.runCLIAll('cache disable'); + expect(toggle.runCLIAll('cache enable')).toContain('Cache enabled'); + + expect(toggle.runCLI('cache stats')).toContain('Cache Status: enabled'); + expect(toggle.runCLI('config get cache.enabled').trim()).toBe('true'); + }); + + it('should report disabled when cache.enabled is set through config directly', () => { + toggle.runCLIAll('config set cache.enabled false'); + + expect(toggle.runCLI('cache stats')).toContain('Cache Status: disabled'); + }); + + it('should preserve --max-size while persisting the enabled flag', () => { + toggle.runCLIAll('cache enable --max-size 100M'); + + expect(toggle.runCLI('config get cache.enabled').trim()).toBe('true'); + expect(toggle.runCLI('config get cache.maxSize').trim()).toBe(String(100 * 1024 * 1024)); + }); + }); + describe('cache --help', () => { it('should display help text', () => { const output = runCLI('cache --help'); diff --git a/tests/unit/cache-command.test.ts b/tests/unit/cache-command.test.ts index 6b77737..dcc9398 100644 --- a/tests/unit/cache-command.test.ts +++ b/tests/unit/cache-command.test.ts @@ -100,6 +100,14 @@ describe('CacheCommand', () => { expect(mockCacheService.enable).toHaveBeenCalledTimes(1); }); + it('should persist cache.enabled so the next process sees it', async () => { + mockCacheService.enable.mockReturnValue(undefined); + + await cacheCommand.enable(); + + expect(mockConfigService.set).toHaveBeenCalledWith('cache.enabled', true); + }); + it('should not throw error if cache is already enabled', async () => { mockCacheService.enable.mockReturnValue(undefined); @@ -124,7 +132,7 @@ describe('CacheCommand', () => { await cacheCommand.enable(); - expect(mockConfigService.set).not.toHaveBeenCalled(); + expect(mockConfigService.set).not.toHaveBeenCalledWith('cache.maxSize', expect.anything()); expect(mockCacheService.setMaxSize).not.toHaveBeenCalled(); expect(mockCacheService.enable).toHaveBeenCalledTimes(1); }); @@ -155,6 +163,14 @@ describe('CacheCommand', () => { await expect(cacheCommand.disable()).resolves.not.toThrow(); }); + + it('should persist cache.enabled so the next process sees it', async () => { + mockCacheService.disable.mockReturnValue(undefined); + + await cacheCommand.disable(); + + expect(mockConfigService.set).toHaveBeenCalledWith('cache.enabled', false); + }); }); describe('formatStats()', () => { From cbc8c1f7cd0c7e69c61bfe4211abe6f0c96a59a5 Mon Sep 17 00:00:00 2001 From: Steven Syrek Date: Wed, 29 Jul 2026 13:50:35 -0400 Subject: [PATCH 05/27] fix(formats): refuse Android CDATA values that close the section A translation containing "]]>" ended the CDATA section it was written into, so its remainder became XML markup in the generated resource file. The adjacent-section split that stood in for escaping could not survive the round-trip either, since the deletion pass then rewrote the result. Refuse such values with a ValidationError instead, matching the XLIFF parser's stance on CDATA in translatable content. --- src/formats/android-xml.ts | 29 +++-- tests/unit/formats-android-cdata.test.ts | 139 ++++++++++++++++++----- 2 files changed, 129 insertions(+), 39 deletions(-) diff --git a/src/formats/android-xml.ts b/src/formats/android-xml.ts index a7bdadd..12bcb19 100644 --- a/src/formats/android-xml.ts +++ b/src/formats/android-xml.ts @@ -1,4 +1,5 @@ import type { ExtractedEntry, FormatParser, TranslatedEntry } from './format.js'; +import { ValidationError } from '../utils/errors.js'; interface PluralItem { quantity: string; @@ -76,6 +77,22 @@ function escapeAndroid(value: string): string { .replace(/>/g, '>'); } +/** + * Refuse a value that would close the CDATA section it is written into. + * Nothing inside a CDATA body is entity-escaped, so the text after a "]]>" + * would be parsed as XML — an injected element in a generated resource file. + * Fail fast rather than rewrite the value into something the round-trip + * cannot reproduce, matching the XLIFF parser's stance on CDATA. + */ +function assertNoCdataBreakout(value: string): void { + if (value.includes(']]>')) { + throw new ValidationError( + 'Android CDATA values containing "]]>" are not supported.', + 'Remove the "]]>" sequence from the text, or drop the wrapper in the source file so the value is entity-escaped instead.', + ); + } +} + export class AndroidXmlFormatParser implements FormatParser { readonly name = 'Android XML'; readonly configKey = 'android_xml'; @@ -260,8 +277,7 @@ export class AndroidXmlFormatParser implements FormatParser { private decodeValue(raw: string): string { if (raw.startsWith('" - // into `]]]]>`, so a literal "]]>" spans two sections. + // Adjacent sections concatenate, so `` is "ab". const sectionRe = //g; let joined = ''; let consumedTo = 0; @@ -277,12 +293,9 @@ export class AndroidXmlFormatParser implements FormatParser { } private escapeForReconstruct(originalInner: string, translation: string): string { - if (/^" would close the section early and the - // remainder would be parsed as XML. Splitting into adjacent CDATA - // sections keeps the text literal without escaping it. - const safe = translation.replace(/]]>/g, ']]]]>'); - return ``; + if (originalInner.startsWith('`; } return escapeAndroid(translation); } diff --git a/tests/unit/formats-android-cdata.test.ts b/tests/unit/formats-android-cdata.test.ts index c944a0a..1174b5c 100644 --- a/tests/unit/formats-android-cdata.test.ts +++ b/tests/unit/formats-android-cdata.test.ts @@ -1,16 +1,14 @@ /** - * Tests that a translation cannot break out of an Android CDATA section. + * A translation cannot break out of an Android CDATA section. * - * escapeForReconstruct wrapped the translation in `` with no - * escaping, so a value containing `]]>` closed the section early and the - * remainder was parsed as XML — allowing extra elements to be - * injected into a generated resource file. This is reachable without a - * malicious API, because on translation failure the source string is written - * through verbatim and the source file is used as the template when the - * target locale file does not exist yet. + * The regex round-trip is asymmetric inside CDATA — nothing in the body is + * entity-escaped — so a value carrying `]]>` would close the section and have + * its remainder parsed as XML. Such values are refused, matching the XLIFF + * parser's stance on CDATA in translatable content. */ import { AndroidXmlFormatParser } from '../../src/formats/android-xml'; +import { ValidationError } from '../../src/utils/errors'; const WITH_CDATA = ` @@ -19,6 +17,9 @@ const WITH_CDATA = ` `; +const BREAKOUT = + ']]>https://evil.example.com { it('should extract CDATA content without the wrapper', () => { const entries = new AndroidXmlFormatParser().extract(WITH_CDATA); @@ -27,38 +28,91 @@ describe('Android XML CDATA safety', () => { expect(byKey.get('body')).toBe('Bold text'); }); - it('should keep injected markup inside the CDATA section', () => { + it('should refuse a translation that closes the CDATA section', () => { const parser = new AndroidXmlFormatParser(); const entries = parser.extract(WITH_CDATA); - const attack = - ']]>https://evil.example.com ({ ...e, translation: e.key === 'body' ? attack : e.value })), - ); + const translated = entries.map((e) => ({ + ...e, + translation: e.key === 'body' ? BREAKOUT : e.value, + })); - // Strip every CDATA section: whatever remains is real markup, and the - // attacker's element must not be part of it. - const markupOnly = out.replace(//g, ''); - expect(markupOnly).not.toContain('name="injected"'); - // Known limitation: extract() is regex-based, so literal `` - // text inside a CDATA body counts as an element. A real Android build - // reads one element here. + expect(() => parser.reconstruct(WITH_CDATA, translated)).toThrow(ValidationError); + expect(() => parser.reconstruct(WITH_CDATA, translated)).toThrow(/CDATA/); }); - it('should preserve "]]>" as literal text through a round-trip', () => { + it('should refuse a bare "]]>" sequence rather than splitting the section', () => { const parser = new AndroidXmlFormatParser(); const entries = parser.extract(WITH_CDATA); - const literal = 'array]]> end'; - const out = parser.reconstruct( - WITH_CDATA, - entries.map((e) => ({ ...e, translation: e.key === 'body' ? literal : e.value })), - ); + expect(() => + parser.reconstruct( + WITH_CDATA, + entries.map((e) => ({ ...e, translation: e.key === 'body' ? 'array]]> end' : e.value })), + ), + ).toThrow(/"\]\]>"/); + }); + + it('should refuse a breakout inside a plural item', () => { + const xml = ` + + + 1 item]]> + %d items]]> + + +`; + const parser = new AndroidXmlFormatParser(); + + expect(() => + parser.reconstruct(xml, [ + { + key: 'items', + value: '%d items', + translation: '%d Elemente', + metadata: { + plurals: [ + { quantity: 'one', value: '1 Element' }, + { quantity: 'other', value: `]]>x { + const xml = ` + + + + + +`; + const parser = new AndroidXmlFormatParser(); + + expect(() => + parser.reconstruct(xml, [ + { key: 'labels.0', value: 'Less < More', translation: ']]>injected [e.key, e.value])); - expect(roundTripped.get('body')).toBe(literal); + it('should escape "]]>" as entities outside a CDATA section', () => { + const xml = ` + + Plain + +`; + const parser = new AndroidXmlFormatParser(); + + const out = parser.reconstruct(xml, [ + { key: 'plain', value: 'Plain', translation: 'array]]> end' }, + ]); + + expect(out).toContain('array]]> end'); + expect(new Map(parser.extract(out).map((e) => [e.key, e.value])).get('plain')).toBe( + 'array]]> end', + ); }); it('should keep CDATA output well-formed for ordinary translations', () => { @@ -75,4 +129,27 @@ describe('Android XML CDATA safety', () => { 'Fett Text', ); }); + + it('should never re-extract more entries than the template declares', () => { + const parser = new AndroidXmlFormatParser(); + const entries = parser.extract(WITH_CDATA); + + const out = parser.reconstruct( + WITH_CDATA, + entries.map((e) => ({ ...e, translation: e.key === 'body' ? 'x' : e.value })), + ); + + expect(parser.extract(out)).toHaveLength(entries.length); + }); + + it('should concatenate adjacent CDATA sections when extracting', () => { + const xml = ` + + + +`; + const entries = new AndroidXmlFormatParser().extract(xml); + + expect(entries).toEqual([{ key: 'split', value: 'firstsecond' }]); + }); }); From 48dc090da69a345ce112081830cffb3d12ec8a70 Mon Sep 17 00:00:00 2001 From: Steven Syrek Date: Wed, 29 Jul 2026 13:52:17 -0400 Subject: [PATCH 06/27] fix(sync): validate auto-glossary terms and make repeat syncs idempotent Terms whose source or translation is empty, whitespace-only, or contains a tab, carriage return or newline are now skipped with a warning naming the key. Uploading them either split the term across TSV columns or fabricated a different entry pair that DeepL then applied to live translations. Dictionary comparison now normalizes both sides the way the glossary TSV round trip does, so a clean glossary is created once and never re-uploaded. Previously local terms were compared against trimmed API entries, which could never be equal, so every sync issued an update. Glossary failures are also isolated per locale and reported with the locale, glossary name and term count instead of silently ending glossary sync for every remaining locale. --- src/sync/sync-glossary.ts | 114 ++++++++++----- src/sync/sync-service.ts | 5 +- tests/unit/sync/sync-glossary.test.ts | 203 ++++++++++++++++++++++++++ 3 files changed, 284 insertions(+), 38 deletions(-) diff --git a/src/sync/sync-glossary.ts b/src/sync/sync-glossary.ts index 466853b..cadb2a6 100644 --- a/src/sync/sync-glossary.ts +++ b/src/sync/sync-glossary.ts @@ -1,6 +1,7 @@ import type { GlossaryService } from '../services/glossary.js'; import type { Language } from '../types/index.js'; import { Logger } from '../utils/logger.js'; +import { errorMessage } from '../utils/error-message.js'; export interface SyncGlossaryManagerOptions { sourceLocale: string; @@ -10,13 +11,36 @@ export interface SyncGlossaryManagerOptions { const MAX_TERM_LENGTH = 50; const MIN_KEY_COUNT = 3; +const TERM_FORBIDDEN_CHARS = /[\t\r\n]/; + +/** + * A term the glossary TSV format cannot carry: tabs and newlines are the + * column and row separators, so uploading one either splits the term or + * fabricates a different entry pair. + */ +function isUnusableTerm(text: string): boolean { + return text.trim() === '' || TERM_FORBIDDEN_CHARS.test(text); +} + +/** + * Normalize the way the glossary TSV round trip does — the API returns entries + * parsed back out of TSV, which trims each field. Comparing raw local terms + * against that could never be equal, so every sync re-uploaded the dictionary. + */ +function normalizeForComparison(entries: Record): Map { + const normalized = new Map(); + for (const [source, target] of Object.entries(entries)) { + normalized.set(source.trim(), target.trim()); + } + return normalized; +} function entriesEqual(a: Record, b: Record): boolean { - const aKeys = Object.keys(a); - const bKeys = Object.keys(b); - if (aKeys.length !== bKeys.length) return false; - for (const key of aKeys) { - if (a[key] !== b[key]) return false; + const normalizedA = normalizeForComparison(a); + const normalizedB = normalizeForComparison(b); + if (normalizedA.size !== normalizedB.size) return false; + for (const [key, value] of normalizedA) { + if (normalizedB.get(key) !== value) return false; } return true; } @@ -74,9 +98,17 @@ export class SyncGlossaryManager { } } - if (isConsistent && consistentTranslation !== undefined) { - terms[sourceText] = consistentTranslation; + if (!isConsistent || consistentTranslation === undefined) continue; + + if (isUnusableTerm(sourceText) || isUnusableTerm(consistentTranslation)) { + const sampleKey = keys.values().next().value; + Logger.warn( + `Skipping glossary term from key "${sampleKey}" (${locale}): source or translation is empty or contains a tab, carriage return or newline.`, + ); + continue; } + + terms[sourceText] = consistentTranslation; } if (Object.keys(terms).length > 0) { @@ -105,40 +137,48 @@ export class SyncGlossaryManager { } const name = this.getGlossaryName(targetLocale); - const existing = await this.options.glossaryService.getGlossaryByName(name); const sourceLang = this.options.sourceLocale as Language; const targetLang = targetLocale as Language; - - if (existing) { - const currentEntries = await this.options.glossaryService.getGlossaryEntries( - existing.glossary_id, - sourceLang, - targetLang, - ); - - const localePair = `${this.options.sourceLocale}-${targetLocale}`; - glossaryIds[localePair] = existing.glossary_id; - - if (!entriesEqual(currentEntries, localeTerms)) { - await this.options.glossaryService.updateGlossary(existing.glossary_id, { - dictionaries: [{ - sourceLang, - targetLang, - entries: localeTerms, - }], - }); - Logger.info(`Updated glossary "${name}" (${existing.glossary_id})`); + const localePair = `${this.options.sourceLocale}-${targetLocale}`; + + // One rejected dictionary must not abandon the remaining locales. + try { + const existing = await this.options.glossaryService.getGlossaryByName(name); + + if (existing) { + const currentEntries = await this.options.glossaryService.getGlossaryEntries( + existing.glossary_id, + sourceLang, + targetLang, + ); + + glossaryIds[localePair] = existing.glossary_id; + + if (!entriesEqual(currentEntries, localeTerms)) { + await this.options.glossaryService.updateGlossary(existing.glossary_id, { + dictionaries: [{ + sourceLang, + targetLang, + entries: localeTerms, + }], + }); + Logger.info(`Updated glossary "${name}" (${existing.glossary_id})`); + } + } else { + const created = await this.options.glossaryService.createGlossary( + name, + sourceLang, + [targetLang], + localeTerms, + ); + glossaryIds[localePair] = created.glossary_id; + Logger.info(`Created glossary "${name}" (${created.glossary_id})`); } - } else { - const created = await this.options.glossaryService.createGlossary( - name, - sourceLang, - [targetLang], - localeTerms, + } catch (error) { + delete glossaryIds[localePair]; + Logger.warn( + `Glossary sync failed for ${localePair} (glossary "${name}", ${Object.keys(localeTerms).length} terms): ${errorMessage(error)}`, ); - const localePair = `${this.options.sourceLocale}-${targetLocale}`; - glossaryIds[localePair] = created.glossary_id; - Logger.info(`Created glossary "${name}" (${created.glossary_id})`); } } diff --git a/src/sync/sync-service.ts b/src/sync/sync-service.ts index f432e89..edfe3d6 100644 --- a/src/sync/sync-service.ts +++ b/src/sync/sync-service.ts @@ -24,6 +24,7 @@ import { SyncGlossaryManager } from './sync-glossary.js'; import { resolveTranslationMemoryId } from '../services/translation-memory.js'; import { TmCache } from './tm-cache.js'; import { Logger } from '../utils/logger.js'; +import { errorMessage } from '../utils/error-message.js'; export type SyncProgressEvent = | { type: 'locale-complete'; locale: string; file: string; translated: number; failed: number; totalKeys: number; charactersBilled: number } @@ -321,7 +322,9 @@ export class SyncService { lockFile.glossary_ids = { ...lockFile.glossary_ids, ...glossaryIds }; lockDirty = true; } catch (error) { - Logger.warn('Auto-glossary sync failed:', error); + Logger.warn( + `Auto-glossary sync failed for locales ${effectiveLocales.join(', ')}: ${errorMessage(error)}. Translations were written; the project glossaries were not updated.`, + ); } } diff --git a/tests/unit/sync/sync-glossary.test.ts b/tests/unit/sync/sync-glossary.test.ts index faf38e4..b0bf1be 100644 --- a/tests/unit/sync/sync-glossary.test.ts +++ b/tests/unit/sync/sync-glossary.test.ts @@ -1,7 +1,28 @@ import { SyncGlossaryManager } from '../../../src/sync/sync-glossary'; import type { SyncGlossaryManagerOptions } from '../../../src/sync/sync-glossary'; +import { GlossaryService } from '../../../src/services/glossary'; +import type { GlossaryInfo } from '../../../src/types/index'; +import { Logger } from '../../../src/utils/logger'; import { createMockGlossaryService } from '../../helpers/mock-factories'; +/** Mirror what a DeepL glossary returns after entries survive a TSV round trip. */ +function roundTrip(entries: Record): Record { + return GlossaryService.tsvToEntries(GlossaryService.entriesToTSV(entries)); +} + +function repeated(source: string, target: string, prefix: string): { + sourceEntries: Map; + localeEntries: Map; +} { + const sourceEntries = new Map(); + const localeEntries = new Map(); + for (let i = 0; i < 3; i++) { + sourceEntries.set(`${prefix}${i}`, source); + localeEntries.set(`${prefix}${i}`, target); + } + return { sourceEntries, localeEntries }; +} + describe('SyncGlossaryManager', () => { function createManager( overrides: Partial = {}, @@ -121,6 +142,95 @@ describe('SyncGlossaryManager', () => { }); }); + describe('extractTerms term validation', () => { + let warnSpy: jest.SpyInstance; + + beforeEach(() => { + warnSpy = jest.spyOn(Logger, 'warn').mockImplementation(() => undefined); + }); + + afterEach(() => { + warnSpy.mockRestore(); + }); + + it('should skip a term whose target translation is empty', () => { + const { manager } = createManager(); + const { sourceEntries, localeEntries } = repeated('OK', '', 'k'); + + const result = manager.extractTerms(sourceEntries, new Map([['de', localeEntries]])); + + expect(result.has('de')).toBe(false); + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining('k0')); + }); + + it('should skip a term whose target translation is whitespace-only', () => { + const { manager } = createManager(); + const { sourceEntries, localeEntries } = repeated('OK', ' ', 'k'); + + const result = manager.extractTerms(sourceEntries, new Map([['de', localeEntries]])); + + expect(result.has('de')).toBe(false); + }); + + it('should skip a term whose source is empty', () => { + const { manager } = createManager(); + const { sourceEntries, localeEntries } = repeated('', 'Leer', 'k'); + + const result = manager.extractTerms(sourceEntries, new Map([['de', localeEntries]])); + + expect(result.has('de')).toBe(false); + }); + + it('should skip a term whose source contains a tab', () => { + const { manager } = createManager(); + const { sourceEntries, localeEntries } = repeated('Col\tumn', 'Spalte', 'k'); + + const result = manager.extractTerms(sourceEntries, new Map([['de', localeEntries]])); + + expect(result.has('de')).toBe(false); + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining('k0')); + }); + + it('should skip a multi-line source string instead of uploading a wrong mapping', () => { + const { manager } = createManager(); + const { sourceEntries, localeEntries } = repeated( + 'Are you sure?\nThis cannot be undone.', + 'Sicher?', + 'k', + ); + + const result = manager.extractTerms(sourceEntries, new Map([['de', localeEntries]])); + + expect(result.has('de')).toBe(false); + }); + + it('should skip a term whose target contains a newline or carriage return', () => { + const { manager } = createManager(); + const withNewline = repeated('Save', 'Spei\nchern', 'a'); + const withCr = repeated('Open', 'Öff\rnen', 'b'); + + const sourceEntries = new Map([...withNewline.sourceEntries, ...withCr.sourceEntries]); + const localeEntries = new Map([...withNewline.localeEntries, ...withCr.localeEntries]); + + const result = manager.extractTerms(sourceEntries, new Map([['de', localeEntries]])); + + expect(result.has('de')).toBe(false); + }); + + it('should keep clean terms alongside skipped ones', () => { + const { manager } = createManager(); + const clean = repeated('Save', 'Speichern', 'a'); + const dirty = repeated('Bad\tterm', 'Schlecht', 'b'); + + const sourceEntries = new Map([...clean.sourceEntries, ...dirty.sourceEntries]); + const localeEntries = new Map([...clean.localeEntries, ...dirty.localeEntries]); + + const result = manager.extractTerms(sourceEntries, new Map([['de', localeEntries]])); + + expect(result.get('de')).toEqual({ Save: 'Speichern' }); + }); + }); + describe('syncGlossaries', () => { it('should create new glossary when none exists', async () => { const { manager, glossaryService } = createManager({ targetLocales: ['de'] }); @@ -305,6 +415,99 @@ describe('SyncGlossaryManager', () => { expect(glossaryService.getGlossaryByName).not.toHaveBeenCalled(); expect(result).toEqual({}); }); + + describe('idempotency', () => { + /** + * Run syncGlossaries twice against a service that stores what was + * uploaded and returns it the way the API does — through a TSV round trip. + */ + async function syncTwice( + sourceEntries: Map, + localeEntries: Map, + ): Promise<{ glossaryService: ReturnType }> { + const { manager, glossaryService } = createManager({ targetLocales: ['de'] }); + const info: GlossaryInfo = { + glossary_id: 'gid-1', + name: 'deepl-sync-en-de', + source_lang: 'en', + target_langs: ['de'], + dictionaries: [{ source_lang: 'en', target_lang: 'de', entry_count: 1 }], + creation_time: '2026-01-01T00:00:00Z', + }; + + let stored: Record | null = null; + glossaryService.getGlossaryByName.mockImplementation(async () => (stored ? info : null)); + glossaryService.createGlossary.mockImplementation(async (_n, _s, _t, entries) => { + stored = roundTrip(entries); + return info; + }); + glossaryService.getGlossaryEntries.mockImplementation(async () => stored ?? {}); + glossaryService.updateGlossary.mockResolvedValue(undefined); + + const targetEntries = new Map([['de', localeEntries]]); + await manager.syncGlossaries(sourceEntries, targetEntries); + await manager.syncGlossaries(sourceEntries, targetEntries); + + return { glossaryService }; + } + + it('should create once and never update when the source terms are unchanged', async () => { + const { sourceEntries, localeEntries } = repeated('Save', 'Speichern', 'k'); + + const { glossaryService } = await syncTwice(sourceEntries, localeEntries); + + expect(glossaryService.createGlossary).toHaveBeenCalledTimes(1); + expect(glossaryService.updateGlossary).toHaveBeenCalledTimes(0); + }); + + it('should not re-upload when a term differs only by surrounding whitespace', async () => { + const { sourceEntries, localeEntries } = repeated('Save ', 'Speichern', 'k'); + + const { glossaryService } = await syncTwice(sourceEntries, localeEntries); + + expect(glossaryService.createGlossary).toHaveBeenCalledTimes(1); + expect(glossaryService.updateGlossary).toHaveBeenCalledTimes(0); + }); + }); + + describe('per-locale failure isolation', () => { + it('should keep syncing other locales and name the failing locale in the warning', async () => { + const warnSpy = jest.spyOn(Logger, 'warn').mockImplementation(() => undefined); + try { + const { manager, glossaryService } = createManager({ targetLocales: ['de', 'fr'] }); + + glossaryService.getGlossaryByName.mockResolvedValue(null); + glossaryService.createGlossary.mockImplementation(async (name) => { + if (name === 'deepl-sync-en-de') { + throw new Error('glossary rejected by API'); + } + const info: GlossaryInfo = { + glossary_id: 'fr-id', + name, + source_lang: 'en', + target_langs: ['fr'], + dictionaries: [{ source_lang: 'en', target_lang: 'fr', entry_count: 1 }], + creation_time: '2026-01-01T00:00:00Z', + }; + return info; + }); + + const { sourceEntries, localeEntries } = repeated('Yes', 'Ja', 'k'); + const targetEntries = new Map([ + ['de', localeEntries], + ['fr', new Map([...localeEntries].map(([k]) => [k, 'Oui'] as [string, string]))], + ]); + + const result = await manager.syncGlossaries(sourceEntries, targetEntries); + + expect(result).toEqual({ 'en-fr': 'fr-id' }); + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining('de')); + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining('glossary rejected by API')); + } finally { + warnSpy.mockRestore(); + } + }); + }); }); describe('getProjectGlossary', () => { From 775626cadefac252e3a9aaa4c89cf0e449d27674 Mon Sep 17 00:00:00 2001 From: Steven Syrek Date: Wed, 29 Jul 2026 13:53:29 -0400 Subject: [PATCH 07/27] fix(formats): bound YAML alias expansion in the parser walkers The YAML parser resolves aliases while walking the document AST, a path the yaml package's maxAliasCount guard does not cover. Nested alias fan-out expanded exponentially and a self-referential anchor recursed until the stack overflowed. Both extract and reconstruct now carry an alias budget that caps the number of nodes reached through alias expansion and the alias nesting depth, throwing the parser's usual "YAML parse error" for documents that exceed either limit. --- src/formats/yaml.ts | 93 +++++++++++++++++++++++++++------ tests/unit/formats/yaml.test.ts | 79 ++++++++++++++++++++++++++++ 2 files changed, 155 insertions(+), 17 deletions(-) diff --git a/src/formats/yaml.ts b/src/formats/yaml.ts index f735ee2..377872e 100644 --- a/src/formats/yaml.ts +++ b/src/formats/yaml.ts @@ -1,6 +1,16 @@ import * as YAML from 'yaml'; import type { FormatParser, ExtractedEntry, TranslatedEntry } from './format.js'; +// Aliases are resolved while walking the AST, where the yaml package's own +// maxAliasCount guard does not apply, so expansion is bounded here instead. +const MAX_ALIAS_EXPANDED_NODES = 200000; +const MAX_ALIAS_DEPTH = 20; + +interface AliasBudget { + expandedNodes: number; + depth: number; +} + export class YamlFormatParser implements FormatParser { readonly name = 'YAML'; readonly configKey = 'yaml'; @@ -23,7 +33,10 @@ export class YamlFormatParser implements FormatParser { } const entries: ExtractedEntry[] = []; - this.walkNode(doc.contents, [], entries, doc); + this.walkNode(doc.contents, [], entries, doc, { + expandedNodes: 0, + depth: 0, + }); return entries; } @@ -43,37 +56,44 @@ export class YamlFormatParser implements FormatParser { } const existingPaths: string[][] = []; + const budget: AliasBudget = { expandedNodes: 0, depth: 0 }; + const walkAlias = (alias: YAML.Alias, path: string[]): void => { + const resolved = this.resolveAlias(alias, doc, budget); + if (YAML.isScalar(resolved) && typeof resolved.value === 'string') { + existingPaths.push(path); + } else if (YAML.isMap(resolved) || YAML.isSeq(resolved)) { + budget.depth++; + walkDoc(resolved, path); + budget.depth--; + } + }; const walkDoc = (node: unknown, path: string[]): void => { if (YAML.isMap(node)) { for (const item of node.items) { + if (budget.depth > 0) { + this.chargeAliasBudget(budget); + } const key = String(YAML.isScalar(item.key) ? item.key.value : item.key); if (YAML.isScalar(item.value) && typeof item.value.value === 'string') { existingPaths.push([...path, key]); } else if (YAML.isMap(item.value) || YAML.isSeq(item.value)) { walkDoc(item.value, [...path, key]); } else if (YAML.isAlias(item.value)) { - const resolved = item.value.resolve(doc); - if (YAML.isScalar(resolved) && typeof resolved.value === 'string') { - existingPaths.push([...path, key]); - } else if (YAML.isMap(resolved) || YAML.isSeq(resolved)) { - walkDoc(resolved, [...path, key]); - } + walkAlias(item.value, [...path, key]); } } } else if (YAML.isSeq(node)) { for (let i = 0; i < node.items.length; i++) { + if (budget.depth > 0) { + this.chargeAliasBudget(budget); + } const item = node.items[i]; if (YAML.isScalar(item) && typeof item.value === 'string') { existingPaths.push([...path, String(i)]); } else if (YAML.isMap(item) || YAML.isSeq(item)) { walkDoc(item, [...path, String(i)]); } else if (YAML.isAlias(item)) { - const resolved = item.resolve(doc); - if (YAML.isScalar(resolved) && typeof resolved.value === 'string') { - existingPaths.push([...path, String(i)]); - } else if (YAML.isMap(resolved) || YAML.isSeq(resolved)) { - walkDoc(resolved, [...path, String(i)]); - } + walkAlias(item, [...path, String(i)]); } } } @@ -106,25 +126,64 @@ export class YamlFormatParser implements FormatParser { pathParts: string[], entries: ExtractedEntry[], doc: YAML.Document, + budget: AliasBudget, ): void { + if (budget.depth > 0) { + this.chargeAliasBudget(budget); + } + if (YAML.isMap(node)) { for (const item of node.items) { const key = String(YAML.isScalar(item.key) ? item.key.value : item.key); - this.walkNode(item.value, [...pathParts, key], entries, doc); + this.walkNode(item.value, [...pathParts, key], entries, doc, budget); } } else if (YAML.isSeq(node)) { for (let i = 0; i < node.items.length; i++) { - this.walkNode(node.items[i], [...pathParts, String(i)], entries, doc); + this.walkNode( + node.items[i], + [...pathParts, String(i)], + entries, + doc, + budget, + ); } } else if (YAML.isScalar(node) && typeof node.value === 'string') { entries.push({ key: pathParts.join('\0'), value: node.value }); } else if (YAML.isAlias(node)) { - const resolved = node.resolve(doc); + const resolved = this.resolveAlias(node, doc, budget); if (YAML.isScalar(resolved) && typeof resolved.value === 'string') { entries.push({ key: pathParts.join('\0'), value: resolved.value }); } else if (YAML.isMap(resolved) || YAML.isSeq(resolved)) { - this.walkNode(resolved, pathParts, entries, doc); + budget.depth++; + this.walkNode(resolved, pathParts, entries, doc, budget); + budget.depth--; } } } + + private resolveAlias( + alias: YAML.Alias, + doc: YAML.Document, + budget: AliasBudget, + ): unknown { + if (budget.depth >= MAX_ALIAS_DEPTH) { + throw new Error( + `YAML parse error: alias nesting is too deep (limit ${MAX_ALIAS_DEPTH}); ` + + 'an anchor in this document may reference itself', + ); + } + this.chargeAliasBudget(budget); + return alias.resolve(doc); + } + + private chargeAliasBudget(budget: AliasBudget): void { + budget.expandedNodes++; + if (budget.expandedNodes > MAX_ALIAS_EXPANDED_NODES) { + throw new Error( + `YAML parse error: aliases expand to more than ${MAX_ALIAS_EXPANDED_NODES} ` + + 'nodes; the anchors and aliases in this document expand to more content ' + + 'than can be processed', + ); + } + } } diff --git a/tests/unit/formats/yaml.test.ts b/tests/unit/formats/yaml.test.ts index f52693d..b4cc1cf 100644 --- a/tests/unit/formats/yaml.test.ts +++ b/tests/unit/formats/yaml.test.ts @@ -337,6 +337,85 @@ describe('yaml parser', () => { }); }); + describe('alias expansion limits', () => { + const aliasBomb = (): string => { + const lines = ['level0: &level0 [a, b, c, d]']; + for (let i = 1; i <= 12; i++) { + const refs = Array.from({ length: 9 }, () => `*level${i - 1}`).join(', '); + lines.push(`level${i}: &level${i} [${refs}]`); + } + return `${lines.join('\n')}\n`; + }; + + const anchorReusedManyTimes = (): string => { + const lines = ['base: &base shared']; + for (let i = 0; i < 200; i++) { + lines.push(`key${i}: *base`); + } + return `${lines.join('\n')}\n`; + }; + + it('should reject nested alias fan-out during extract without expanding it', () => { + const start = Date.now(); + expect(() => parser.extract(aliasBomb())).toThrow(/YAML parse error/); + expect(Date.now() - start).toBeLessThan(2000); + }); + + it('should reject nested alias fan-out during reconstruct without expanding it', () => { + const start = Date.now(); + expect(() => parser.reconstruct(aliasBomb(), [])).toThrow(/YAML parse error/); + expect(Date.now() - start).toBeLessThan(2000); + }); + + it('should reject repeated expansion of a large anchor', () => { + const lines = ['big: &big']; + for (let i = 0; i < 500; i++) { + lines.push(` entry${i}: value${i}`); + } + for (let i = 0; i < 500; i++) { + lines.push(`copy${i}: *big`); + } + const yaml = `${lines.join('\n')}\n`; + + const start = Date.now(); + expect(() => parser.extract(yaml)).toThrow(/YAML parse error/); + expect(Date.now() - start).toBeLessThan(2000); + }); + + it('should reject a self-referential anchor during extract', () => { + const yaml = 'root: &r\n child: *r\n'; + const start = Date.now(); + expect(() => parser.extract(yaml)).toThrow(/YAML parse error/); + expect(Date.now() - start).toBeLessThan(2000); + }); + + it('should reject a self-referential anchor during reconstruct', () => { + const yaml = 'root: &r\n child: *r\n'; + expect(() => parser.reconstruct(yaml, [])).toThrow(/YAML parse error/); + }); + + it('should report which construct exceeded the limit', () => { + expect(() => parser.extract(aliasBomb())).toThrow(/alias/i); + expect(() => parser.extract('root: &r\n child: *r\n')).toThrow(/alias/i); + }); + + it('should still extract a document that reuses one anchor many times', () => { + const entries = parser.extract(anchorReusedManyTimes()); + expect(entries).toHaveLength(201); + expect(entries.every(e => e.value === 'shared')).toBe(true); + }); + + it('should still round-trip a document that reuses one anchor many times', () => { + const yaml = anchorReusedManyTimes(); + const entries: TranslatedEntry[] = parser + .extract(yaml) + .map(e => ({ key: e.key, value: e.value, translation: 'compartido' })); + const result = parser.reconstruct(yaml, entries); + expect(result).toContain('compartido'); + expect(result).toContain('key199'); + }); + }); + describe('reconstruct trailing newline handling', () => { it('should strip trailing newline if original does not end with one', () => { const yaml = 'greeting: Hello'; From 429f60d76288b06878aced8fc8cd5107602661b4 Mon Sep 17 00:00:00 2001 From: Steven Syrek Date: Wed, 29 Jul 2026 13:54:56 -0400 Subject: [PATCH 08/27] fix(glossary): reject separator characters in terms and sniff the dialect per file addEntry/updateEntry now reject a source or target containing a tab, carriage return or newline, naming the offending character. Passing argv straight through let a single term shift every following column of the uploaded dictionary. tsvToEntries picks TSV or CSV once for the whole file instead of per line, so a quoted CSV field containing a tab is no longer tab-split into garbage, and rows whose parsed fields carry a separator character are skipped. --- src/services/glossary.ts | 47 ++++++++++++++++++- tests/unit/glossary-service.test.ts | 73 ++++++++++++++++++++++++++++- 2 files changed, 117 insertions(+), 3 deletions(-) diff --git a/src/services/glossary.ts b/src/services/glossary.ts index 300d81b..1150d00 100644 --- a/src/services/glossary.ts +++ b/src/services/glossary.ts @@ -20,6 +20,34 @@ function hasSuspiciousChars(name: string): boolean { const LIST_CACHE_TTL_MS = 60_000; +/** + * Characters the glossary TSV wire format reserves as column and row + * separators. A term containing one of them cannot survive the round trip: + * it either shifts columns or fabricates extra entries. + */ +const TERM_SEPARATOR_NAMES: Array<[string, string]> = [ + ['\t', 'tab'], + ['\r', 'carriage return'], + ['\n', 'newline'], +]; + +function findSeparatorChar(text: string): string | null { + for (const [char, name] of TERM_SEPARATOR_NAMES) { + if (text.includes(char)) return name; + } + return null; +} + +function assertNoSeparatorChars(label: string, text: string): void { + const found = findSeparatorChar(text); + if (found) { + throw new ValidationError( + `${label} cannot contain a ${found} character`, + 'Glossary entries are stored as tab-separated rows; split the term into separate entries instead.', + ); + } +} + export class GlossaryService { private client: DeepLClient; private resolutionCache = new Map(); @@ -197,6 +225,8 @@ export class GlossaryService { if (!targetText || targetText.trim() === '') { throw new ValidationError('Target text cannot be empty'); } + assertNoSeparatorChars('Source text', sourceText); + assertNoSeparatorChars('Target text', targetText); await this.mutateEntries(glossaryId, sourceLang, targetLang, (entries) => { if (entries[sourceText] !== undefined) { throw new ValidationError(`Entry "${sourceText}" already exists in glossary`); @@ -221,6 +251,8 @@ export class GlossaryService { if (!newTargetText || newTargetText.trim() === '') { throw new ValidationError('Target text cannot be empty'); } + assertNoSeparatorChars('Source text', sourceText); + assertNoSeparatorChars('Target text', newTargetText); await this.mutateEntries(glossaryId, sourceLang, targetLang, (entries) => { if (entries[sourceText] === undefined) { throw new ConfigError(`Entry "${sourceText}" not found in glossary`); @@ -415,6 +447,10 @@ export class GlossaryService { } const lines = content.split('\n'); + // Pick the dialect once for the whole file. Sniffing per line tab-splits a + // quoted CSV field that happens to contain a tab into garbage columns. + const firstDataLine = lines.map(line => line.trim()).find(line => line !== '') ?? ''; + const isTabSeparated = firstDataLine.includes('\t'); let lineNumber = 0; for (const line of lines) { @@ -426,10 +462,9 @@ export class GlossaryService { continue; } - // Try tab-separated first (TSV is preferred), then comma-separated (CSV) let parts: string[]; - if (trimmed.includes('\t')) { + if (isTabSeparated) { parts = trimmed.split('\t'); } else if (trimmed.includes(',')) { // Use proper CSV parsing for comma-separated values (handles quoted fields) @@ -460,6 +495,14 @@ export class GlossaryService { continue; } + // A quoted CSV field may carry a tab or newline that the TSV wire format + // cannot represent; keeping it would corrupt every following entry. + const separator = findSeparatorChar(source) ?? findSeparatorChar(target); + if (separator) { + Logger.warn(`Line ${lineNumber}: Source or target contains a ${separator} character, skipping`); + continue; + } + // Add to entries (duplicates will overwrite earlier entries) if (Object.hasOwn(entries, source)) { Logger.warn(`Line ${lineNumber}: Duplicate source "${source}", overwriting previous entry`); diff --git a/tests/unit/glossary-service.test.ts b/tests/unit/glossary-service.test.ts index 5aad1b4..a4e5978 100644 --- a/tests/unit/glossary-service.test.ts +++ b/tests/unit/glossary-service.test.ts @@ -4,7 +4,7 @@ import { GlossaryService } from '../../src/services/glossary'; import { DeepLClient } from '../../src/api/deepl-client'; -import { ConfigError } from '../../src/utils/errors'; +import { ConfigError, ValidationError } from '../../src/utils/errors'; import { createMockDeepLClient } from '../helpers/mock-factories'; // Mock DeepLClient @@ -702,6 +702,37 @@ describe('GlossaryService', () => { 'hello, world': 'hola mundo', }); }); + + describe('dialect selection', () => { + it('should not tab-split a quoted CSV field in a CSV file', () => { + const csv = 'API,API\n"tab\there,inside",valor'; + + const entries = GlossaryService.tsvToEntries(csv); + + // Tab-splitting the second line would have produced the garbage key + // '"tab' with value 'here,inside",valor'. + expect(Object.keys(entries)).toEqual(['API']); + }); + + it('should skip an entry whose parsed field contains a tab or newline', () => { + const csv = 'API,API\n"bad\tterm",valor\nHello,Hola'; + + const entries = GlossaryService.tsvToEntries(csv); + + expect(entries).toEqual({ API: 'API', Hello: 'Hola' }); + }); + + it('should keep parsing a TSV file as TSV when a later value contains commas', () => { + const tsv = 'API\tAPI\nHello\tHola, mundo'; + + const entries = GlossaryService.tsvToEntries(tsv); + + expect(entries).toEqual({ + API: 'API', + Hello: 'Hola, mundo', + }); + }); + }); }); describe('addEntry()', () => { @@ -742,6 +773,29 @@ describe('GlossaryService', () => { glossaryService.addEntry('test-123', 'en', 'es', 'source', '') ).rejects.toThrow('Target text cannot be empty'); }); + + it('should reject source text containing a tab', async () => { + await expect( + glossaryService.addEntry('test-123', 'en', 'es', 'Col\tumn', 'Columna') + ).rejects.toThrow(ValidationError); + await expect( + glossaryService.addEntry('test-123', 'en', 'es', 'Col\tumn', 'Columna') + ).rejects.toThrow(/tab/i); + expect(mockDeepLClient.updateGlossaryEntries).not.toHaveBeenCalled(); + }); + + it('should reject target text containing a newline', async () => { + await expect( + glossaryService.addEntry('test-123', 'en', 'es', 'Line', 'Lí\nnea') + ).rejects.toThrow(/newline/i); + expect(mockDeepLClient.updateGlossaryEntries).not.toHaveBeenCalled(); + }); + + it('should reject source text containing a carriage return', async () => { + await expect( + glossaryService.addEntry('test-123', 'en', 'es', 'Line\r', 'Línea') + ).rejects.toThrow(/carriage return/i); + }); }); describe('updateEntry()', () => { @@ -779,6 +833,23 @@ describe('GlossaryService', () => { glossaryService.updateEntry('test-123', 'en', 'es', 'source', '') ).rejects.toThrow('Target text cannot be empty'); }); + + it('should reject a new target text containing a tab', async () => { + mockDeepLClient.getGlossaryEntries.mockResolvedValue('API\tAPI'); + + await expect( + glossaryService.updateEntry('test-123', 'en', 'es', 'API', 'Inter\tface') + ).rejects.toThrow(/tab/i); + expect(mockDeepLClient.updateGlossaryEntries).not.toHaveBeenCalled(); + }); + + it('should reject a new target text containing a newline', async () => { + mockDeepLClient.getGlossaryEntries.mockResolvedValue('API\tAPI'); + + await expect( + glossaryService.updateEntry('test-123', 'en', 'es', 'API', 'Inter\nface') + ).rejects.toThrow(/newline/i); + }); }); describe('removeEntry()', () => { From 865c3e759df1237776b1e7d2e89675c19a5a54a5 Mon Sep 17 00:00:00 2001 From: Steven Syrek Date: Wed, 29 Jul 2026 13:56:53 -0400 Subject: [PATCH 09/27] fix(sync): route --locale and --sync-config to subcommand handlers Commander keeps matching the parent command's options after a subcommand name, so `deepl sync status --locale de` bound `de` to the parent `sync` command and left the subcommand's own option store undefined. Both flags are declared on the parent and on the subcommands, so the filter logic in status/validate/export never received a value and --sync-config silently fell back to the auto-detected config. Add resolveSyncConfig() alongside the existing resolveLocale(), plus a shared parseLocaleFilter(), and route status, validate, export, audit, resolve, push, and pull through them. --- src/cli/commands/sync/register-sync-audit.ts | 6 +- src/cli/commands/sync/register-sync-export.ts | 15 +- src/cli/commands/sync/register-sync-pull.ts | 16 +- src/cli/commands/sync/register-sync-push.ts | 16 +- .../commands/sync/register-sync-resolve.ts | 6 +- src/cli/commands/sync/register-sync-status.ts | 27 +-- .../commands/sync/register-sync-validate.ts | 23 ++- src/cli/commands/sync/sync-options.ts | 30 +++- tests/e2e/cli-sync-option-routing.e2e.test.ts | 157 ++++++++++++++++++ tests/unit/cli/sync-options.test.ts | 56 +++++++ 10 files changed, 315 insertions(+), 37 deletions(-) create mode 100644 tests/e2e/cli-sync-option-routing.e2e.test.ts create mode 100644 tests/unit/cli/sync-options.test.ts diff --git a/src/cli/commands/sync/register-sync-audit.ts b/src/cli/commands/sync/register-sync-audit.ts index 5f9c39c..61b92a3 100644 --- a/src/cli/commands/sync/register-sync-audit.ts +++ b/src/cli/commands/sync/register-sync-audit.ts @@ -4,7 +4,7 @@ import { ValidationError } from '../../../utils/errors.js'; import type { ServiceDeps } from '../service-factory.js'; import type { TargetTranslationIndex } from '../../../sync/sync-glossary-report.js'; import { extractTranslatable } from '../../../sync/sync-bucket-walker.js'; -import { emitJsonErrorAndExit, resolveFormat } from './sync-options.js'; +import { emitJsonErrorAndExit, resolveFormat, resolveSyncConfig } from './sync-options.js'; interface AuditOptions { format?: string; @@ -65,7 +65,9 @@ async function handleSyncAudit( const pathMod = await import('path'); const fsMod = await import('fs'); - const config = await loadSyncConfig(process.cwd(), { configPath: options.syncConfig }); + const config = await loadSyncConfig(process.cwd(), { + configPath: resolveSyncConfig(options, command), + }); const lockPath = pathMod.join(config.projectRoot, LOCK_FILE_NAME); const lockManager = new SyncLockManager(lockPath); const lockFile = await lockManager.read(); diff --git a/src/cli/commands/sync/register-sync-export.ts b/src/cli/commands/sync/register-sync-export.ts index 8cea483..7579760 100644 --- a/src/cli/commands/sync/register-sync-export.ts +++ b/src/cli/commands/sync/register-sync-export.ts @@ -2,7 +2,13 @@ import { Command, Option } from 'commander'; import { Logger } from '../../../utils/logger.js'; import { ValidationError } from '../../../utils/errors.js'; import type { ServiceDeps } from '../service-factory.js'; -import { emitJsonErrorAndExit, resolveFormat } from './sync-options.js'; +import { + emitJsonErrorAndExit, + parseLocaleFilter, + resolveFormat, + resolveLocale, + resolveSyncConfig, +} from './sync-options.js'; interface ExportOptions { locale?: string; @@ -47,9 +53,12 @@ async function handleSyncExport( const pathMod = await import('path'); const fsMod = await import('fs'); - const config = await loadSyncConfig(process.cwd(), { configPath: options.syncConfig }); + const localeFilter = parseLocaleFilter(resolveLocale(options, command)); + const config = await loadSyncConfig(process.cwd(), { + configPath: resolveSyncConfig(options, command), + localeFilter, + }); const registry = await createDefaultRegistry(); - const localeFilter = options.locale?.split(',').map((l: string) => l.trim()); const result = await exportTranslations(config, registry, { localeFilter }); if (options.output) { diff --git a/src/cli/commands/sync/register-sync-pull.ts b/src/cli/commands/sync/register-sync-pull.ts index 920a326..49a3844 100644 --- a/src/cli/commands/sync/register-sync-pull.ts +++ b/src/cli/commands/sync/register-sync-pull.ts @@ -2,7 +2,13 @@ import { Command, Option } from 'commander'; import { Logger } from '../../../utils/logger.js'; import { ConfigError } from '../../../utils/errors.js'; import type { ServiceDeps } from '../service-factory.js'; -import { emitJsonErrorAndExit, resolveFormat, resolveLocale } from './sync-options.js'; +import { + emitJsonErrorAndExit, + parseLocaleFilter, + resolveFormat, + resolveLocale, + resolveSyncConfig, +} from './sync-options.js'; interface PullOptions { locale?: string; @@ -40,6 +46,7 @@ full field list and REST contract. .action((options: PullOptions, command: Command) => { options.format = resolveFormat(options, command); options.locale = resolveLocale(options, command); + options.syncConfig = resolveSyncConfig(options, command); return handleSyncPull(options, deps.handleError); }); } @@ -55,7 +62,11 @@ export async function handleSyncPull( const { pullTranslations, formatSkippedSummary } = await import('../../../sync/sync-tms.js'); const { acquireSyncProcessLock } = await import('../../../sync/sync-process-lock.js'); - const config = await loadSyncConfig(process.cwd(), { configPath: options.syncConfig }); + const localeFilter = parseLocaleFilter(options.locale); + const config = await loadSyncConfig(process.cwd(), { + configPath: options.syncConfig, + localeFilter, + }); if (!config.tms?.enabled) { throw new ConfigError( 'TMS integration not configured', @@ -67,7 +78,6 @@ export async function handleSyncPull( try { const client = createTmsClient(config.tms); const registry = await createDefaultRegistry(); - const localeFilter = options.locale?.split(',').map((l: string) => l.trim()); const result = await pullTranslations(config, client, registry, { localeFilter }); if (options.format === 'json') { process.stdout.write(JSON.stringify({ ok: true, pulled: result.pulled, skipped: result.skipped }) + '\n'); diff --git a/src/cli/commands/sync/register-sync-push.ts b/src/cli/commands/sync/register-sync-push.ts index eae5d49..a4dfe27 100644 --- a/src/cli/commands/sync/register-sync-push.ts +++ b/src/cli/commands/sync/register-sync-push.ts @@ -2,7 +2,13 @@ import { Command, Option } from 'commander'; import { Logger } from '../../../utils/logger.js'; import { ConfigError } from '../../../utils/errors.js'; import type { ServiceDeps } from '../service-factory.js'; -import { emitJsonErrorAndExit, resolveFormat, resolveLocale } from './sync-options.js'; +import { + emitJsonErrorAndExit, + parseLocaleFilter, + resolveFormat, + resolveLocale, + resolveSyncConfig, +} from './sync-options.js'; interface PushOptions { locale?: string; @@ -52,7 +58,11 @@ async function handleSyncPush( const { createDefaultRegistry } = await import('../../../formats/index.js'); const { pushTranslations, formatSkippedSummary } = await import('../../../sync/sync-tms.js'); - const config = await loadSyncConfig(process.cwd(), { configPath: options.syncConfig }); + const localeFilter = parseLocaleFilter(resolveLocale(options, command)); + const config = await loadSyncConfig(process.cwd(), { + configPath: resolveSyncConfig(options, command), + localeFilter, + }); if (!config.tms?.enabled) { throw new ConfigError( 'TMS integration not configured', @@ -63,8 +73,6 @@ async function handleSyncPush( const client = createTmsClient(config.tms); const registry = await createDefaultRegistry(); - const locale = resolveLocale(options, command); - const localeFilter = locale?.split(',').map((l: string) => l.trim()); const result = await pushTranslations(config, client, registry, { localeFilter }); if (options.format === 'json') { process.stdout.write(JSON.stringify({ ok: true, pushed: result.pushed, skipped: result.skipped }) + '\n'); diff --git a/src/cli/commands/sync/register-sync-resolve.ts b/src/cli/commands/sync/register-sync-resolve.ts index b4c6b37..e2a3657 100644 --- a/src/cli/commands/sync/register-sync-resolve.ts +++ b/src/cli/commands/sync/register-sync-resolve.ts @@ -2,7 +2,7 @@ import { Command, Option } from 'commander'; import { Logger } from '../../../utils/logger.js'; import { SyncConflictError } from '../../../utils/errors.js'; import type { ServiceDeps } from '../service-factory.js'; -import { emitJsonErrorAndExit, resolveFormat } from './sync-options.js'; +import { emitJsonErrorAndExit, resolveFormat, resolveSyncConfig } from './sync-options.js'; interface ResolveOptions { syncConfig?: string; @@ -43,7 +43,9 @@ async function handleSyncResolve( const { resolveLockFile } = await import('../../../sync/sync-resolve.js'); const pathMod = await import('path'); - const config = await loadSyncConfig(process.cwd(), { configPath: options.syncConfig }); + const config = await loadSyncConfig(process.cwd(), { + configPath: resolveSyncConfig(options, command), + }); const lockPath = pathMod.join(config.projectRoot, LOCK_FILE_NAME); const result = await resolveLockFile(lockPath, { dryRun }); diff --git a/src/cli/commands/sync/register-sync-status.ts b/src/cli/commands/sync/register-sync-status.ts index 6895d0e..6eeadf3 100644 --- a/src/cli/commands/sync/register-sync-status.ts +++ b/src/cli/commands/sync/register-sync-status.ts @@ -1,7 +1,13 @@ import { Command, Option } from 'commander'; import { Logger } from '../../../utils/logger.js'; import type { ServiceDeps } from '../service-factory.js'; -import { emitJsonErrorAndExit, resolveFormat } from './sync-options.js'; +import { + emitJsonErrorAndExit, + parseLocaleFilter, + resolveFormat, + resolveLocale, + resolveSyncConfig, +} from './sync-options.js'; interface StatusOptions { locale?: string; @@ -37,20 +43,17 @@ async function handleSyncStatus( const { createDefaultRegistry } = await import('../../../formats/index.js'); const { computeSyncStatus } = await import('../../../sync/sync-status.js'); - const config = await loadSyncConfig(process.cwd(), { configPath: options.syncConfig }); + const localeFilter = parseLocaleFilter(resolveLocale(options, command)); + const config = await loadSyncConfig(process.cwd(), { + configPath: resolveSyncConfig(options, command), + localeFilter, + }); const registry = await createDefaultRegistry(); const status = await computeSyncStatus(config, registry); - let locales = status.locales; - if (options.locale) { - const filterLocales = options.locale.split(',').map((l) => l.trim()); - locales = locales.filter((l) => filterLocales.includes(l.locale)); - if (locales.length === 0) { - Logger.warn( - `No matching locales for filter. Available: ${config.target_locales.join(', ')}`, - ); - } - } + const locales = localeFilter + ? status.locales.filter((l) => localeFilter.includes(l.locale)) + : status.locales; if (options.format === 'json') { process.stdout.write(JSON.stringify({ ...status, locales }, null, 2) + '\n'); diff --git a/src/cli/commands/sync/register-sync-validate.ts b/src/cli/commands/sync/register-sync-validate.ts index 729c54b..fda92af 100644 --- a/src/cli/commands/sync/register-sync-validate.ts +++ b/src/cli/commands/sync/register-sync-validate.ts @@ -3,7 +3,13 @@ import chalk from 'chalk'; import { Logger } from '../../../utils/logger.js'; import { ExitCode } from '../../../utils/exit-codes.js'; import type { ServiceDeps } from '../service-factory.js'; -import { emitJsonErrorAndExit, resolveFormat } from './sync-options.js'; +import { + emitJsonErrorAndExit, + parseLocaleFilter, + resolveFormat, + resolveLocale, + resolveSyncConfig, +} from './sync-options.js'; interface ValidateOptions { locale?: string; @@ -39,17 +45,16 @@ async function handleSyncValidate( const { createDefaultRegistry } = await import('../../../formats/index.js'); const { validateTranslations } = await import('../../../sync/sync-validate.js'); - const config = await loadSyncConfig(process.cwd(), { configPath: options.syncConfig }); + const localeFilter = parseLocaleFilter(resolveLocale(options, command)); + const config = await loadSyncConfig(process.cwd(), { + configPath: resolveSyncConfig(options, command), + localeFilter, + }); - const allLocales = [...config.target_locales]; - if (options.locale) { - const filterLocales = options.locale.split(',').map((l) => l.trim()); + if (localeFilter) { config.target_locales = config.target_locales.filter((l: string) => - filterLocales.includes(l), + localeFilter.includes(l), ); - if (config.target_locales.length === 0) { - Logger.warn(`No matching locales for filter. Available: ${allLocales.join(', ')}`); - } } const registry = await createDefaultRegistry(); diff --git a/src/cli/commands/sync/sync-options.ts b/src/cli/commands/sync/sync-options.ts index 4590b0f..91965d6 100644 --- a/src/cli/commands/sync/sync-options.ts +++ b/src/cli/commands/sync/sync-options.ts @@ -21,9 +21,13 @@ export function resolveFormat( } /** - * Resolve --locale across parent (`sync`) and subcommand scopes so push/pull - * narrow the fan-out regardless of where the flag sits on the invocation + * Resolve --locale across parent (`sync`) and subcommand scopes so a + * subcommand narrows regardless of where the flag sits on the invocation * line. Subcommand value wins; otherwise fall back to the parent's. + * + * Without positional options, commander keeps matching the parent's flags + * after the subcommand name, so `deepl sync status --locale de` binds `de` to + * the parent `sync` command and leaves the subcommand's own store undefined. */ export function resolveLocale( opts: { locale?: string }, @@ -33,6 +37,28 @@ export function resolveLocale( return command.parent?.opts()['locale'] as string | undefined; } +/** + * Resolve --sync-config across parent (`sync`) and subcommand scopes. Same + * parent/child binding rule as {@link resolveLocale}: a subcommand that reads + * only its own store silently falls back to the auto-detected config. + */ +export function resolveSyncConfig( + opts: { syncConfig?: string }, + command: Command, +): string | undefined { + if (opts.syncConfig !== undefined) return opts.syncConfig; + return command.parent?.opts()['syncConfig'] as string | undefined; +} + +/** + * Split a resolved --locale value into the comma-separated filter list that + * `loadSyncConfig` validates against `target_locales`. + */ +export function parseLocaleFilter(locale: string | undefined): string[] | undefined { + if (!locale) return undefined; + return locale.split(',').map((l) => l.trim()); +} + /** * JSON error envelope shape emitted on stderr when --format json is set and * a command fails. Shared across every `deepl sync` subcommand so script diff --git a/tests/e2e/cli-sync-option-routing.e2e.test.ts b/tests/e2e/cli-sync-option-routing.e2e.test.ts new file mode 100644 index 0000000..6a52643 --- /dev/null +++ b/tests/e2e/cli-sync-option-routing.e2e.test.ts @@ -0,0 +1,157 @@ +/** + * E2E tests for --locale / --sync-config routing across `deepl sync` and its + * subcommands. + * + * Commander binds an invocation-line flag to the nearest command that declares + * it, and the parent `sync` command is reached first even when the flag trails + * the subcommand name. Both flags are declared on the parent and on the + * subcommands, so a subcommand that reads only its own option store sees + * `undefined`. These tests drive the real CLI so the parent/child binding is + * exercised — asserting on the handlers directly would pass either way. + */ + +import { spawnSync, SpawnSyncReturns } from 'child_process'; +import * as fs from 'fs'; +import * as path from 'path'; +import { createTestConfigDir, createTestDir } from '../helpers'; + +const CLI_PATH = path.join(process.cwd(), 'dist/cli/index.js'); + +describe('CLI sync option routing E2E', () => { + const testConfig = createTestConfigDir('e2e-sync-routing'); + const testFiles = createTestDir('e2e-sync-routing-files'); + + function writeConfig(configDir: string): void { + const config = { + auth: { apiKey: 'mock-api-key-for-testing:fx' }, + api: { baseUrl: 'http://127.0.0.1:1/', usePro: false }, + defaults: { targetLangs: [], formality: 'default', preserveFormatting: true }, + cache: { enabled: false, maxSize: 1048576, ttl: 2592000 }, + output: { format: 'text', verbose: false, color: false }, + watch: { debounceMs: 500, autoCommit: false, pattern: '*.md' }, + }; + fs.writeFileSync(path.join(configDir, 'config.json'), JSON.stringify(config, null, 2)); + } + + function writeSyncConfig(projectDir: string, locales: string[] = ['de', 'fr']): string { + const yaml = [ + 'version: 1', + 'source_locale: en', + 'target_locales:', + ...locales.map((l) => ` - ${l}`), + 'buckets:', + ' json:', + ' include:', + ' - "locales/en.json"', + ].join('\n') + '\n'; + const configPath = path.join(projectDir, '.deepl-sync.yaml'); + fs.writeFileSync(configPath, yaml); + return configPath; + } + + function writeSourceFile(projectDir: string): void { + const dir = path.join(projectDir, 'locales'); + fs.mkdirSync(dir, { recursive: true }); + fs.writeFileSync( + path.join(dir, 'en.json'), + JSON.stringify({ greeting: 'Hello', farewell: 'Goodbye' }, null, 2) + '\n', + ); + } + + interface Run { + status: number; + stdout: string; + stderr: string; + output: string; + } + + function runCli(args: string[], cwd: string = testFiles.path): Run { + const result: SpawnSyncReturns = spawnSync('node', [CLI_PATH, ...args], { + encoding: 'utf-8', + cwd, + env: { + ...process.env, + DEEPL_CONFIG_DIR: testConfig.path, + DEEPL_API_KEY: 'mock-api-key-for-testing:fx', + NO_COLOR: '1', + CI: undefined, + }, + stdio: ['ignore', 'pipe', 'pipe'], + timeout: 15000, + }); + const stdout = result.stdout ?? ''; + const stderr = result.stderr ?? ''; + return { status: result.status ?? 1, stdout, stderr, output: stdout + stderr }; + } + + beforeAll(() => { + writeConfig(testConfig.path); + }); + + beforeEach(() => { + for (const entry of fs.readdirSync(testFiles.path)) { + fs.rmSync(path.join(testFiles.path, entry), { recursive: true, force: true }); + } + }); + + afterAll(() => { + testConfig.cleanup(); + testFiles.cleanup(); + }); + + describe('--locale reaches the subcommand handler', () => { + it('narrows `sync status` output to the requested locale', () => { + writeSyncConfig(testFiles.path, ['de', 'fr']); + writeSourceFile(testFiles.path); + + const run = runCli(['sync', 'status', '--locale', 'de']); + + expect(run.status).toBe(0); + expect(run.output).toMatch(/\bde\b/); + expect(run.output).not.toMatch(/^\s+fr\s/m); + }); + + it('narrows `sync export` XLIFF output to the requested locale', () => { + writeSyncConfig(testFiles.path, ['de', 'fr']); + writeSourceFile(testFiles.path); + + const run = runCli(['sync', 'export', '--locale', 'de']); + + expect(run.status).toBe(0); + expect(run.stdout).toContain('target-language="de"'); + expect(run.stdout).not.toContain('target-language="fr"'); + }); + }); + + describe('--sync-config reaches the subcommand handler', () => { + for (const sub of ['status', 'validate', 'export', 'audit', 'resolve', 'push', 'pull']) { + it(`\`sync ${sub} --sync-config \` fails with ConfigError (exit 7)`, () => { + writeSyncConfig(testFiles.path, ['de', 'fr']); + writeSourceFile(testFiles.path); + + const missing = path.join(testFiles.path, 'nope', 'absent.yaml'); + const run = runCli(['sync', sub, '--sync-config', missing]); + + expect(run.status).toBe(7); + expect(run.output).toContain(missing); + }); + } + + it('honors --sync-config pointing at a config outside the auto-detected one', () => { + writeSyncConfig(testFiles.path, ['de', 'fr']); + writeSourceFile(testFiles.path); + + const altRoot = path.join(testFiles.path, 'alt'); + fs.mkdirSync(altRoot, { recursive: true }); + writeSyncConfig(altRoot, ['it']); + writeSourceFile(altRoot); + const altConfig = path.join(altRoot, '.deepl-sync.yaml'); + + const run = runCli(['sync', 'status', '--sync-config', altConfig]); + + expect(run.status).toBe(0); + expect(run.output).toMatch(/\bit\b/); + expect(run.output).not.toMatch(/^\s+de\s/m); + }); + }); +}); diff --git a/tests/unit/cli/sync-options.test.ts b/tests/unit/cli/sync-options.test.ts new file mode 100644 index 0000000..96c7858 --- /dev/null +++ b/tests/unit/cli/sync-options.test.ts @@ -0,0 +1,56 @@ +import { Command } from 'commander'; +import { + parseLocaleFilter, + resolveLocale, + resolveSyncConfig, +} from '../../../src/cli/commands/sync/sync-options'; + +describe('sync-options', () => { + function childOf(parentOpts: Record): Command { + const parent = new Command('sync'); + const child = parent.command('status'); + Object.assign(parent.opts(), parentOpts); + return child; + } + + describe('resolveLocale', () => { + it('prefers the subcommand value', () => { + expect(resolveLocale({ locale: 'de' }, childOf({ locale: 'fr' }))).toBe('de'); + }); + + it('falls back to the parent value', () => { + expect(resolveLocale({}, childOf({ locale: 'fr' }))).toBe('fr'); + }); + + it('returns undefined when neither scope has a value', () => { + expect(resolveLocale({}, childOf({}))).toBeUndefined(); + }); + }); + + describe('resolveSyncConfig', () => { + it('prefers the subcommand value', () => { + expect(resolveSyncConfig({ syncConfig: 'a.yaml' }, childOf({ syncConfig: 'b.yaml' }))).toBe( + 'a.yaml', + ); + }); + + it('falls back to the parent value', () => { + expect(resolveSyncConfig({}, childOf({ syncConfig: 'b.yaml' }))).toBe('b.yaml'); + }); + + it('returns undefined when neither scope has a value', () => { + expect(resolveSyncConfig({}, childOf({}))).toBeUndefined(); + }); + }); + + describe('parseLocaleFilter', () => { + it('splits and trims a comma-separated list', () => { + expect(parseLocaleFilter('de, fr ,ja')).toEqual(['de', 'fr', 'ja']); + }); + + it('returns undefined for an absent or empty value', () => { + expect(parseLocaleFilter(undefined)).toBeUndefined(); + expect(parseLocaleFilter('')).toBeUndefined(); + }); + }); +}); From 2299bfdfaf22648fa6508ba09adaeb5e831de79b Mon Sep 17 00:00:00 2001 From: Steven Syrek Date: Wed, 29 Jul 2026 13:57:30 -0400 Subject: [PATCH 10/27] fix(api): retry only idempotent requests and classify transport failures by code A client-side abort says nothing about whether the server already accepted the request, yet every failure other than a 4xx was replayed for every HTTP method. A slow batch translate or a large document upload that exceeded the 30s timeout was re-sent three more times, each attempt already accepted and billed server-side; worst case, admin key creation duplicated keys whose secret is returned once. Automatic retries are now limited to idempotent methods (GET/HEAD/PUT/ DELETE/OPTIONS/TRACE). A POST is replayed only on transport errors that prove it never reached the server (ECONNREFUSED/ENOTFOUND/EAI_AGAIN); 429 handling is unchanged for all methods because a rate-limited request was rejected without being processed. Classification now branches on the axios error code and the absence of a response instead of matching message substrings: a client-side timeout reads "timeout of 30000ms exceeded", matched no substring, and surfaced as ValidationError (exit 6) where exit-codes.ts documents a network error (exit 5). ECONNABORTED and ERR_CANCELED were absent entirely. 401 now maps to AuthError (exit 2) instead of falling through to exit 6. Also adds a wall-clock budget shared across attempts (default: twice the per-request timeout, backoff sleeps excluded) so a never-responding server bounds total wait instead of running the full attempt count, scopes the Trace ID quoted in an error to the request that failed rather than the client's last-seen header, and stops doubling the "Network error:" prefix. Tests updated deliberately, all of which assumed retry-everything: - deepl-client.test.ts retry-logic and maxRetries cases now exercise an idempotent GET; a new case asserts translate is not replayed. - deepl-client.integration.test.ts 500-retry case likewise, plus a new no-replay assertion. - sync.integration.test.ts 503 recovery now asserts the POST is not replayed and the locale is recorded as failed. --- src/api/http-client.ts | 166 ++++++++-- .../deepl-client.integration.test.ts | 34 +- .../file-translation.integration.test.ts | 1 - tests/integration/sync.integration.test.ts | 23 +- tests/unit/deepl-client.test.ts | 55 ++-- tests/unit/http-retry-policy.test.ts | 300 ++++++++++++++++++ 6 files changed, 496 insertions(+), 83 deletions(-) create mode 100644 tests/unit/http-retry-policy.test.ts diff --git a/src/api/http-client.ts b/src/api/http-client.ts index b323ed5..b3c53d7 100644 --- a/src/api/http-client.ts +++ b/src/api/http-client.ts @@ -31,6 +31,9 @@ export interface DeepLClientOptions { usePro?: boolean; timeout?: number; maxRetries?: number; + /** Wall-clock budget for all attempts of one request, excluding backoff + * sleeps. Defaults to `timeout * 2`. */ + totalTimeout?: number; baseUrl?: string; proxy?: ProxyConfig; } @@ -58,6 +61,44 @@ const KEEP_ALIVE_MSECS = 1000; const RETRY_INITIAL_DELAY_MS = 1000; const RETRY_MAX_DELAY_MS = 10000; const RETRY_AFTER_MAX_SECONDS = 60; +const TOTAL_TIMEOUT_FACTOR = 2; + +/** + * Methods a failed attempt may be replayed on. A POST is excluded because a + * client-side abort (timeout, mid-flight reset) says nothing about whether + * the server already accepted — and billed — the request; replaying it + * duplicates the work (a second translation, a second uploaded document, a + * second API key whose secret is only returned once). + */ +const IDEMPOTENT_METHODS = new Set([ + 'GET', + 'HEAD', + 'PUT', + 'DELETE', + 'OPTIONS', + 'TRACE', +]); + +/** Transport errors that prove the request never reached the server, so even + * a non-idempotent request is safe to replay. */ +const UNSENT_REQUEST_CODES = new Set([ + 'ECONNREFUSED', + 'ENOTFOUND', + 'EAI_AGAIN', +]); + +/** Axios codes for a request the client itself gave up on. */ +const CLIENT_ABORT_CODES = new Set([ + 'ECONNABORTED', + 'ETIMEDOUT', + 'ERR_CANCELED', +]); + +/** Per-request overrides for the retry policy and timeouts. */ +export interface RequestPolicy { + maxRetries?: number; + timeout?: number; +} /** * Compute a retry delay for attempt `n` with full jitter: a uniform @@ -68,6 +109,14 @@ const RETRY_AFTER_MAX_SECONDS = 60; * Exported for unit testing; the caller pulls the randomized value * and passes it straight to `sleep()`. */ +/** Prefixes a transport failure without stuttering when the underlying + * message already carries the label. */ +function prefixNetwork(detail: string, label: string): string { + return /^network (error|timeout)\b/i.test(detail) + ? detail + : `${label}: ${detail}`; +} + export function computeBackoffWithJitter(attempt: number): number { const cap = Math.min(RETRY_INITIAL_DELAY_MS * 2 ** attempt, RETRY_MAX_DELAY_MS); return Math.floor(Math.random() * cap); @@ -76,6 +125,8 @@ export function computeBackoffWithJitter(attempt: number): number { export class HttpClient { protected client: AxiosInstance; protected maxRetries: number; + protected requestTimeout: number; + protected totalTimeout: number; protected _lastTraceId?: string; private static parseProxyFromEnv(): ProxyConfig | undefined { @@ -133,6 +184,9 @@ export class HttpClient { options.baseUrl ?? (options.usePro ? PRO_API_URL : FREE_API_URL); this.maxRetries = options.maxRetries ?? DEFAULT_MAX_RETRIES; + this.requestTimeout = options.timeout ?? DEFAULT_TIMEOUT; + this.totalTimeout = + options.totalTimeout ?? this.requestTimeout * TOTAL_TIMEOUT_FACTOR; const axiosConfig: Record = { baseURL, @@ -266,54 +320,66 @@ export class HttpClient { protected async makeRawRequest( method: 'GET' | 'POST' | 'PUT' | 'PATCH' | 'DELETE', path: string, - buildConfig: () => Record + buildConfig: () => Record, + policy?: RequestPolicy ): Promise { - return this.executeWithRetry(method, path, buildConfig); + return this.executeWithRetry(method, path, buildConfig, policy); } protected async executeWithRetry( method: string, path: string, - buildConfig: () => Record + buildConfig: () => Record, + policy?: RequestPolicy ): Promise { + const maxRetries = policy?.maxRetries ?? this.maxRetries; + const requestTimeout = policy?.timeout ?? this.requestTimeout; + // Backoff sleeps are deliberately excluded from the budget: a + // server-directed Retry-After wait is not time spent waiting on a dead + // connection, and collapsing the two would abandon honest rate limits. + let remainingBudget = Math.max(this.totalTimeout, requestTimeout); let lastError: Error | undefined; + let traceId: string | undefined; - for (let attempt = 0; attempt <= this.maxRetries; attempt++) { + for (let attempt = 0; attempt <= maxRetries; attempt++) { + const requestStart = Date.now(); try { const config = buildConfig(); - const requestStart = Date.now(); const response = await this.client.request({ method, url: path, ...config, + timeout: Math.min(requestTimeout, remainingBudget), }); const requestElapsed = Date.now() - requestStart; Logger.verbose( `[verbose] HTTP ${method} ${path} completed in ${requestElapsed}ms (status ${response.status})` ); - const traceId = response.headers?.['x-trace-id'] as string | undefined; - if (traceId) { - this._lastTraceId = traceId; + const responseTraceId = response.headers?.['x-trace-id'] as + | string + | undefined; + if (responseTraceId) { + this._lastTraceId = responseTraceId; } return response.data; } catch (error) { + remainingBudget -= Date.now() - requestStart; lastError = error as Error; if (this.isAxiosError(error)) { - const traceId = error.response?.headers?.['x-trace-id'] as + const responseTraceId = error.response?.headers?.['x-trace-id'] as | string | undefined; - if (traceId) { - this._lastTraceId = traceId; + if (responseTraceId) { + traceId = responseTraceId; + this._lastTraceId = responseTraceId; } - } - if (this.isAxiosError(error)) { const status = error.response?.status; - if (status === 429 && attempt < this.maxRetries) { + if (status === 429 && attempt < maxRetries) { const retryAfterDelay = this.parseRetryAfter( error.response?.headers?.['retry-after'] as string | undefined ); @@ -324,43 +390,70 @@ export class HttpClient { const delay = retryAfterDelay ?? computeBackoffWithJitter(attempt); Logger.verbose( - `[verbose] HTTP ${method} ${path} retry ${attempt + 1}/${this.maxRetries} in ${delay}ms (status 429${retryAfterDelay !== null && retryAfterDelay !== undefined ? ', Retry-After' : ', jitter backoff'})` + `[verbose] HTTP ${method} ${path} retry ${attempt + 1}/${maxRetries} in ${delay}ms (status 429${retryAfterDelay !== null && retryAfterDelay !== undefined ? ', Retry-After' : ', jitter backoff'})` ); await this.sleep(delay); continue; } if (status && status >= 400 && status < 500) { - throw this.handleError(error); + throw this.handleError(error, undefined, traceId); } } - if (attempt < this.maxRetries) { + if ( + attempt < maxRetries && + remainingBudget > 0 && + this.isReplayable(method, error) + ) { const delay = computeBackoffWithJitter(attempt); const status = this.isAxiosError(error) ? error.response?.status : undefined; Logger.verbose( - `[verbose] HTTP ${method} ${path} retry ${attempt + 1}/${this.maxRetries} in ${delay}ms (${status ? `status ${status}` : 'network error'}, jitter backoff)` + `[verbose] HTTP ${method} ${path} retry ${attempt + 1}/${maxRetries} in ${delay}ms (${status ? `status ${status}` : 'network error'}, jitter backoff)` ); await this.sleep(delay); + continue; } + + break; } } throw lastError - ? this.handleError(lastError) + ? this.handleError(lastError, undefined, traceId) : new NetworkError('Request failed after retries'); } - protected handleError(error: unknown, context?: string): Error { - const result = this.classifyError(error); + /** + * Whether a failed attempt may be sent again. 4xx responses never reach + * here (they throw immediately) and 429 is handled by the caller, so the + * cases left are 5xx responses and transport failures. + */ + private isReplayable(method: string, error: unknown): boolean { + if (!this.isAxiosError(error)) { + return false; + } + if (!error.response && error.code && UNSENT_REQUEST_CODES.has(error.code)) { + return true; + } + return IDEMPOTENT_METHODS.has(method.toUpperCase()); + } + + protected handleError( + error: unknown, + context?: string, + traceId?: string + ): Error { + const result = this.classifyError(error, traceId); if (context) { result.message = `${result.message} [${context}]`; } return result; } - private classifyError(error: unknown): Error { - const traceIdSuffix = this._lastTraceId - ? ` (Trace ID: ${this._lastTraceId})` + private classifyError(error: unknown, traceId?: string): Error { + const requestTraceId = traceId ?? this._lastTraceId; + const traceIdSuffix = requestTraceId + ? ` (Trace ID: ${requestTraceId})` : ''; if (this.isAxiosError(error)) { @@ -377,6 +470,10 @@ export class HttpClient { const message = sanitizeForTerminal(responseData?.message ?? error.message ?? ''); switch (status) { + case 401: + return new AuthError( + `Authentication failed: Invalid or missing API key${traceIdSuffix}` + ); case 403: return new AuthError( `Authentication failed: Invalid API key${traceIdSuffix}` @@ -399,8 +496,14 @@ export class HttpClient { `Server error (${status}): ${message}${traceIdSuffix}` ); } - if (!error.response && this.isNetworkLevelError(error)) { - return new NetworkError(`Network error: ${error.message}`); + // No response at all means the request never completed a round + // trip: a refused connection, a DNS failure, a reset socket, or a + // client-side abort. All of those are network conditions, and the + // axios `code` is the reliable signal — the message is not (a + // timeout reads "timeout of 30000ms exceeded" and matches no + // substring list). + if (!error.response) { + return this.transportError(error); } return new ValidationError(`API error: ${message}${traceIdSuffix}`); } @@ -408,7 +511,7 @@ export class HttpClient { if (error instanceof Error) { if (this.isNetworkLevelError(error)) { - return new NetworkError(`Network error: ${error.message}`); + return new NetworkError(prefixNetwork(error.message, 'Network error')); } return error; } @@ -416,6 +519,15 @@ export class HttpClient { return new NetworkError('Unknown error occurred'); } + private transportError(error: AxiosError): NetworkError { + const detail = sanitizeForTerminal(error.message ?? ''); + const label = + error.code && CLIENT_ABORT_CODES.has(error.code) + ? 'Network timeout' + : 'Network error'; + return new NetworkError(prefixNetwork(detail, label)); + } + private isNetworkLevelError(error: Error): boolean { const msg = error.message.toLowerCase(); return ( diff --git a/tests/integration/deepl-client.integration.test.ts b/tests/integration/deepl-client.integration.test.ts index e602658..786585e 100644 --- a/tests/integration/deepl-client.integration.test.ts +++ b/tests/integration/deepl-client.integration.test.ts @@ -309,9 +309,7 @@ describe('DeepLClient Integration', () => { const client = new DeepLClient(API_KEY, { maxRetries: 2 }); clients.push(client); - // Mock all retry attempts (initial + 2 retries = 3 total) - nock(FREE_API_URL).post('/v2/translate').reply(503, { message: 'Service temporarily unavailable' }); - nock(FREE_API_URL).post('/v2/translate').reply(503, { message: 'Service temporarily unavailable' }); + // A translate POST is not replayed, so one attempt is all the server sees. nock(FREE_API_URL).post('/v2/translate').reply(503, { message: 'Service temporarily unavailable' }); await expect(client.translate('Hello', { targetLang: 'es' })).rejects.toThrow( @@ -341,21 +339,35 @@ describe('DeepLClient Integration', () => { ); }); - it('should retry on 500 errors', async () => { + it('should retry an idempotent request on 500 errors', async () => { const client = new DeepLClient(API_KEY, { maxRetries: 2 }); clients.push(client); // First two attempts fail with 500, third succeeds - nock(FREE_API_URL).post('/v2/translate').reply(500, 'Internal Server Error'); - nock(FREE_API_URL).post('/v2/translate').reply(500, 'Internal Server Error'); + nock(FREE_API_URL).get('/v2/usage').reply(500, 'Internal Server Error'); + nock(FREE_API_URL).get('/v2/usage').reply(500, 'Internal Server Error'); nock(FREE_API_URL) + .get('/v2/usage') + .reply(200, { character_count: 10, character_limit: 100 }); + + const usage = await client.getUsage(); + expect(usage.characterCount).toBe(10); + }); + + it('should not replay a translate request on 500 errors', async () => { + const client = new DeepLClient(API_KEY, { maxRetries: 2 }); + clients.push(client); + + const scope = nock(FREE_API_URL) .post('/v2/translate') - .reply(200, { - translations: [{ text: 'Hola' }], - }); + .reply(500, { message: 'Internal Server Error' }); - const result = await client.translate('Hello', { targetLang: 'es' }); - expect(result.text).toBe('Hola'); + await expect( + client.translate('Hello', { targetLang: 'es' }) + ).rejects.toThrow(/Server error \(500\)/); + + expect(scope.isDone()).toBe(true); + expect(nock.pendingMocks()).toHaveLength(0); }); it('should NOT retry on 4xx errors', async () => { diff --git a/tests/integration/file-translation.integration.test.ts b/tests/integration/file-translation.integration.test.ts index 454a402..c1eadbc 100644 --- a/tests/integration/file-translation.integration.test.ts +++ b/tests/integration/file-translation.integration.test.ts @@ -119,7 +119,6 @@ describe('FileTranslation Integration', () => { nock(DEEPL_FREE_API_URL) .post('/v2/translate') - .times(4) .reply(503, { message: 'Service unavailable' }); await expect( diff --git a/tests/integration/sync.integration.test.ts b/tests/integration/sync.integration.test.ts index 30bc49d..cc6f8b5 100644 --- a/tests/integration/sync.integration.test.ts +++ b/tests/integration/sync.integration.test.ts @@ -1347,7 +1347,7 @@ buckets: retryClient.destroy(); }); - it('should retry on 503 and complete successfully', async () => { + it('should not replay a translate POST on 503', async () => { const retryClient = new DeepLClient(TEST_API_KEY, { maxRetries: 1 }); const mockConfig = createMockConfigService({ get: jest.fn(() => ({ @@ -1373,28 +1373,19 @@ buckets: const scope503 = nock(DEEPL_FREE_API_URL) .post('/v2/translate') .reply(503, { message: 'Service unavailable' }, { 'Retry-After': '0' }); - const scope200 = nock(DEEPL_FREE_API_URL) - .post('/v2/translate') - .reply(200, { - translations: [ - { text: 'Hallo', detected_source_language: 'EN', billed_characters: 5 }, - ], - }); - const start = Date.now(); const config = await loadSyncConfig(tmpDir); const result = await retrySyncService.sync(config); - const elapsed = Date.now() - start; - expect(result.success).toBe(true); - expect(result.totalKeys).toBe(1); expect(scope503.isDone()).toBe(true); - expect(scope200.isDone()).toBe(true); - // Guard against runaway backoff — with Retry-After: 0 the whole retry should finish fast. - expect(elapsed).toBeLessThan(3000); + expect(nock.pendingMocks()).toHaveLength(0); + + const deResult = result.fileResults.find((r) => r.locale === 'de'); + expect(deResult?.written).toBe(false); + expect(deResult?.failed).toBeGreaterThan(0); const targetFile = path.join(tmpDir, 'locales', 'de.json'); - expect(fs.existsSync(targetFile)).toBe(true); + expect(fs.existsSync(targetFile)).toBe(false); retryClient.destroy(); }); diff --git a/tests/unit/deepl-client.test.ts b/tests/unit/deepl-client.test.ts index 4f55a6f..d613d9d 100644 --- a/tests/unit/deepl-client.test.ts +++ b/tests/unit/deepl-client.test.ts @@ -618,7 +618,6 @@ describe('DeepLClient', () => { it('should handle 503 service unavailable', async () => { nock(baseUrl) .post(path) - .times(4) .reply(503, { message: 'Service temporarily unavailable' }); await expect(call(client)).rejects.toThrow('Service temporarily unavailable'); @@ -627,7 +626,6 @@ describe('DeepLClient', () => { it('should handle network errors', async () => { nock(baseUrl) .post(path) - .times(4) .replyWithError('Network error'); await expect(call(client)).rejects.toThrow(); @@ -667,7 +665,7 @@ describe('DeepLClient', () => { it('should strip ANSI escape sequences from a 5xx server message before interpolating into the thrown error', async () => { const maliciousBody = { message: '\x1b[31mInternal failure\x1b[0m' }; - nock(baseUrl).post('/v2/translate').times(4).reply(500, maliciousBody); + nock(baseUrl).post('/v2/translate').reply(500, maliciousBody); await expect(client.translate('Hello', { targetLang: 'es' })).rejects.toThrow( expect.objectContaining({ message: expect.not.stringContaining('\x1b') }), @@ -856,31 +854,38 @@ describe('DeepLClient', () => { }); describe('retry logic', () => { - it('should retry on transient failures', async () => { + it('should retry an idempotent request on transient failures', async () => { nock(baseUrl) - .post('/v2/translate') + .get('/v2/usage') .reply(503) - .post('/v2/translate') + .get('/v2/usage') .reply(503) - .post('/v2/translate') - .reply(200, { - translations: [{ text: 'Hola' }], - }); + .get('/v2/usage') + .reply(200, { character_count: 1, character_limit: 2 }); - const result = await client.translate('Hello', { targetLang: 'es' }); + const usage = await client.getUsage(); - expect(result.text).toBe('Hola'); + expect(usage.characterCount).toBe(1); + }); + + it('should not replay a translate request on transient failures', async () => { + const scope = nock(baseUrl).post('/v2/translate').reply(503); + + await expect( + client.translate('Hello', { targetLang: 'es' }) + ).rejects.toThrow('Service temporarily unavailable'); + + expect(scope.isDone()).toBe(true); + expect(nock.pendingMocks()).toHaveLength(0); }); it('should give up after max retries', async () => { nock(baseUrl) - .post('/v2/translate') + .get('/v2/usage') .times(4) .reply(503); - await expect( - client.translate('Hello', { targetLang: 'es' }) - ).rejects.toThrow(); + await expect(client.getUsage()).rejects.toThrow(); }); it('should not retry on 4xx errors', async () => { @@ -993,13 +998,11 @@ describe('DeepLClient', () => { const customClient = new DeepLClient(apiKey, { maxRetries: 1 }); nock(baseUrl) - .post('/v2/translate') + .get('/v2/usage') .times(2) // Should only retry 1 time + initial = 2 requests total .reply(503); - await expect( - customClient.translate('Hello', { targetLang: 'es' }) - ).rejects.toThrow(); + await expect(customClient.getUsage()).rejects.toThrow(); expect(nock.isDone()).toBe(true); }); @@ -1008,13 +1011,11 @@ describe('DeepLClient', () => { const customClient = new DeepLClient(apiKey, { maxRetries: 0 }); nock(baseUrl) - .post('/v2/translate') + .get('/v2/usage') .times(1) // Should not retry, only 1 request .reply(503); - await expect( - customClient.translate('Hello', { targetLang: 'es' }) - ).rejects.toThrow(); + await expect(customClient.getUsage()).rejects.toThrow(); expect(nock.isDone()).toBe(true); }); @@ -1023,13 +1024,11 @@ describe('DeepLClient', () => { const customClient = new DeepLClient(apiKey, { maxRetries: 2 }); nock(baseUrl) - .post('/v2/translate') + .get('/v2/usage') .times(3) // Should retry 2 times + initial = 3 requests total .reply(503); - await expect( - customClient.translate('Hello', { targetLang: 'es' }) - ).rejects.toThrow(); + await expect(customClient.getUsage()).rejects.toThrow(); expect(nock.isDone()).toBe(true); }); diff --git a/tests/unit/http-retry-policy.test.ts b/tests/unit/http-retry-policy.test.ts new file mode 100644 index 0000000..9f12624 --- /dev/null +++ b/tests/unit/http-retry-policy.test.ts @@ -0,0 +1,300 @@ +/** + * Retry-policy and transport-error classification tests for HttpClient. + * + * Every assertion here counts SERVER-SIDE requests: a replay of a + * non-idempotent request is only observable from the server's side, so + * `.rejects.toThrow()` alone cannot catch it. + */ + +import nock from 'nock'; +import { HttpClient } from '../../src/api/http-client'; +import { + AuthError, + NetworkError, + RateLimitError, +} from '../../src/utils/errors'; +import { ExitCode, exitCodeForError } from '../../src/utils/exit-codes'; + +class TestHttpClient extends HttpClient { + get(path: string): Promise { + return this.makeRequest('GET', path); + } + + post(path: string, data?: Record): Promise { + return this.makeRequest('POST', path, data); + } + + put(path: string, data?: Record): Promise { + return this.makeRequest('PUT', path, data); + } + + delete(path: string): Promise { + return this.makeRequest('DELETE', path); + } +} + +const BASE_URL = 'https://api-free.deepl.com'; + +describe('HttpClient retry policy', () => { + const apiKey = 'test-api-key'; + let clients: TestHttpClient[]; + + function makeClient(options: Record = {}): TestHttpClient { + const client = new TestHttpClient(apiKey, { + timeout: 200, + maxRetries: 3, + totalTimeout: 10_000, + ...options, + }); + jest + .spyOn(client as unknown as { sleep: (ms: number) => Promise }, 'sleep') + .mockResolvedValue(); + clients.push(client); + return client; + } + + /** nock only produces a coded transport error from a real Error instance. */ + function transportError(code: string, message: string): Error { + return Object.assign(new Error(message), { code }); + } + + /** Counts requests the server actually received for `path`. */ + function countRequests(scope: nock.Scope): () => number { + let count = 0; + scope.on('request', () => { + count++; + }); + return () => count; + } + + beforeEach(() => { + clients = []; + }); + + // Interceptors left unconsumed are the point of several tests here (a + // request that must NOT be replayed leaves its spare interceptors + // pending), so clean up before the global pending-mock assertion runs. + afterEach(() => { + for (const client of clients) { + client.destroy(); + } + nock.cleanAll(); + }); + + describe('client-side timeout', () => { + it('sends a POST exactly once when the client aborts on timeout', async () => { + const scope = nock(BASE_URL) + .post('/v2/translate') + .times(4) + .delayConnection(2000) + .reply(200, { translations: [{ text: 'Hola' }] }); + const requests = countRequests(scope); + + await expect(makeClient().post('/v2/translate', { text: 'Hello' })).rejects.toThrow( + NetworkError + ); + + expect(requests()).toBe(1); + }); + + it('classifies a client-side timeout as a network error (exit 5), not invalid input', async () => { + nock(BASE_URL) + .post('/v2/translate') + .times(4) + .delayConnection(2000) + .reply(200, { translations: [{ text: 'Hola' }] }); + + const error = await makeClient() + .post('/v2/translate', { text: 'Hello' }) + .catch((e: unknown) => e); + + expect(error).toBeInstanceOf(NetworkError); + expect(exitCodeForError(error)).toBe(ExitCode.NetworkError); + }); + + it('retries an idempotent GET that times out', async () => { + const scope = nock(BASE_URL) + .get('/v2/usage') + .times(4) + .delayConnection(2000) + .reply(200, {}); + const requests = countRequests(scope); + + await expect(makeClient().get('/v2/usage')).rejects.toThrow(NetworkError); + + expect(requests()).toBe(4); + }); + + it('retries an idempotent PUT that times out', async () => { + const scope = nock(BASE_URL) + .put('/v3/glossaries/g-1') + .times(4) + .delayConnection(2000) + .reply(200, {}); + const requests = countRequests(scope); + + await expect(makeClient().put('/v3/glossaries/g-1', { name: 'x' })).rejects.toThrow( + NetworkError + ); + + expect(requests()).toBe(4); + }); + }); + + describe('transport failures', () => { + it('retries a POST when the connection was refused', async () => { + const scope = nock(BASE_URL) + .post('/v2/translate') + .times(4) + .replyWithError(transportError('ECONNREFUSED', 'connect ECONNREFUSED 127.0.0.1:443')); + const requests = countRequests(scope); + + await expect(makeClient().post('/v2/translate', { text: 'Hello' })).rejects.toThrow( + NetworkError + ); + + expect(requests()).toBe(4); + }); + + it('retries a POST when DNS resolution failed', async () => { + const scope = nock(BASE_URL) + .post('/v2/translate') + .times(4) + .replyWithError(transportError('ENOTFOUND', 'getaddrinfo ENOTFOUND api-free.deepl.com')); + const requests = countRequests(scope); + + await expect(makeClient().post('/v2/translate', { text: 'Hello' })).rejects.toThrow( + NetworkError + ); + + expect(requests()).toBe(4); + }); + + it('does not retry a POST reset mid-flight — the request may have been accepted', async () => { + const scope = nock(BASE_URL) + .post('/v2/translate') + .times(4) + .replyWithError(transportError('ECONNRESET', 'socket hang up')); + const requests = countRequests(scope); + + await expect(makeClient().post('/v2/translate', { text: 'Hello' })).rejects.toThrow( + NetworkError + ); + + expect(requests()).toBe(1); + }); + }); + + describe('server errors', () => { + it('retries a 500 on an idempotent GET', async () => { + const scope = nock(BASE_URL).get('/v2/usage').times(4).reply(500, { message: 'boom' }); + const requests = countRequests(scope); + + await expect(makeClient().get('/v2/usage')).rejects.toThrow(NetworkError); + + expect(requests()).toBe(4); + }); + + it('does not retry a 500 on a POST', async () => { + const scope = nock(BASE_URL) + .post('/v2/translate') + .times(4) + .reply(500, { message: 'boom' }); + const requests = countRequests(scope); + + await expect(makeClient().post('/v2/translate', { text: 'Hello' })).rejects.toThrow( + /Server error \(500\)/ + ); + + expect(requests()).toBe(1); + }); + + it('does not retry a 503 on a POST', async () => { + const scope = nock(BASE_URL) + .post('/v2/translate') + .times(4) + .reply(503, { message: 'Service Unavailable' }); + const requests = countRequests(scope); + + await expect(makeClient().post('/v2/translate', { text: 'Hello' })).rejects.toThrow( + /Service temporarily unavailable/ + ); + + expect(requests()).toBe(1); + }); + + it('still retries a POST on 429 — the server rejected it without processing', async () => { + const scope = nock(BASE_URL) + .post('/v2/translate') + .times(4) + .reply(429, { message: 'Too many requests' }); + const requests = countRequests(scope); + + await expect(makeClient().post('/v2/translate', { text: 'Hello' })).rejects.toThrow( + RateLimitError + ); + + expect(requests()).toBe(4); + }); + }); + + describe('401 handling', () => { + it('maps 401 to AuthError without retrying', async () => { + const scope = nock(BASE_URL).get('/v2/usage').times(4).reply(401, { message: 'Unauthorized' }); + const requests = countRequests(scope); + + const error = await makeClient() + .get('/v2/usage') + .catch((e: unknown) => e); + + expect(error).toBeInstanceOf(AuthError); + expect(exitCodeForError(error)).toBe(ExitCode.AuthError); + expect(requests()).toBe(1); + }); + }); + + describe('total deadline', () => { + it('stops retrying an idempotent request once the overall budget is spent', async () => { + const scope = nock(BASE_URL) + .get('/v2/usage') + .times(6) + .delayConnection(2000) + .reply(200, {}); + const requests = countRequests(scope); + + const client = makeClient({ timeout: 200, maxRetries: 5, totalTimeout: 500 }); + const start = Date.now(); + await expect(client.get('/v2/usage')).rejects.toThrow(NetworkError); + const elapsed = Date.now() - start; + + expect(requests()).toBeLessThanOrEqual(3); + expect(elapsed).toBeLessThan(1500); + }); + + it('derives a default deadline from the request timeout', async () => { + const scope = nock(BASE_URL) + .get('/v2/usage') + .times(6) + .delayConnection(2000) + .reply(200, {}); + const requests = countRequests(scope); + + const client = makeClient({ timeout: 200, maxRetries: 5, totalTimeout: undefined }); + await expect(client.get('/v2/usage')).rejects.toThrow(NetworkError); + + expect(requests()).toBeLessThanOrEqual(3); + }); + }); + + describe('error messages', () => { + it('does not double the "Network error" prefix', async () => { + nock(BASE_URL).post('/v2/translate').replyWithError('Network error'); + + const error = await makeClient() + .post('/v2/translate', { text: 'Hello' }) + .catch((e: unknown) => e); + + expect((error as Error).message).not.toMatch(/Network error: Network error/); + }); + }); +}); From fff3e1651ba83d5bf9aabcf5376b18f491b704ba Mon Sep 17 00:00:00 2001 From: Steven Syrek Date: Wed, 29 Jul 2026 13:57:35 -0400 Subject: [PATCH 11/27] fix(sync): reject a --locale value that is not in target_locales MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit docs/API.md has documented that a locale outside target_locales passed to `sync --locale` exits with a ConfigError, but the run reported success while translating nothing — a typo'd locale made CI report green. Validate the filter in applyCliOverrides, which every config load already funnels through, so the root command and every subcommand that resolves a --locale filter enforce it from one place. The error names the offending locales and the configured ones. --- src/sync/sync-config.ts | 17 +++- tests/e2e/cli-sync-option-routing.e2e.test.ts | 86 +++++++++++++++++++ tests/unit/sync/sync-config.test.ts | 22 +++++ 3 files changed, 123 insertions(+), 2 deletions(-) diff --git a/src/sync/sync-config.ts b/src/sync/sync-config.ts index 3d0cd24..f09f81d 100644 --- a/src/sync/sync-config.ts +++ b/src/sync/sync-config.ts @@ -521,14 +521,27 @@ export function validateSyncConfig(raw: unknown): SyncConfig { }; } -// Single source of truth for merging CLI overrides (--formality, --glossary, -// --model-type, --scan-context, --batch/--no-batch) into a loaded +// Single source of truth for merging CLI overrides (--locale, --formality, +// --glossary, --model-type, --scan-context, --batch/--no-batch) into a loaded // SyncConfig. All guards that cross the YAML-vs-CLI layer boundary live here // so they cannot be bypassed by callers that assemble the config elsewhere. +// --locale is a filter over target_locales, not a specifier, so a value +// outside target_locales is a ConfigError rather than a silent no-op. export function applyCliOverrides( config: SyncConfig, overrides: SyncConfigOverrides, ): SyncConfig { + if (overrides.localeFilter !== undefined) { + const unconfigured = overrides.localeFilter.filter( + (locale) => !config.target_locales.includes(locale), + ); + if (unconfigured.length > 0) { + throw new ConfigError( + `--locale ${unconfigured.join(', ')} not in target_locales (configured: ${config.target_locales.join(', ')})`, + 'Pass only locales listed in target_locales, or add the locale to target_locales in .deepl-sync.yaml.', + ); + } + } if (overrides.formality !== undefined) { config.translation = config.translation ?? {}; config.translation.formality = overrides.formality as Formality; diff --git a/tests/e2e/cli-sync-option-routing.e2e.test.ts b/tests/e2e/cli-sync-option-routing.e2e.test.ts index 6a52643..9f390fe 100644 --- a/tests/e2e/cli-sync-option-routing.e2e.test.ts +++ b/tests/e2e/cli-sync-option-routing.e2e.test.ts @@ -8,6 +8,9 @@ * subcommands, so a subcommand that reads only its own option store sees * `undefined`. These tests drive the real CLI so the parent/child binding is * exercised — asserting on the handlers directly would pass either way. + * + * Also covers the ConfigError documented at docs/API.md for a --locale value + * that is not in `target_locales`. */ import { spawnSync, SpawnSyncReturns } from 'child_process'; @@ -154,4 +157,87 @@ describe('CLI sync option routing E2E', () => { expect(run.output).not.toMatch(/^\s+de\s/m); }); }); + + describe('--locale must name a configured target locale', () => { + it('exits 7 on `sync --locale ` naming the offending and configured locales', () => { + writeSyncConfig(testFiles.path, ['de', 'fr']); + writeSourceFile(testFiles.path); + + const run = runCli(['sync', '--locale', 'es', '--dry-run']); + + expect(run.status).toBe(7); + expect(run.output).toContain('es'); + expect(run.output).toContain('de, fr'); + expect(run.output).not.toContain('Sync complete'); + }); + + it('exits 7 on `sync status --locale `', () => { + writeSyncConfig(testFiles.path, ['de', 'fr']); + writeSourceFile(testFiles.path); + + const run = runCli(['sync', 'status', '--locale', 'es']); + + expect(run.status).toBe(7); + expect(run.output).toContain('es'); + expect(run.output).toContain('de, fr'); + }); + + it('exits 7 on `sync validate --locale ` instead of reporting all-passed', () => { + writeSyncConfig(testFiles.path, ['de', 'fr']); + writeSourceFile(testFiles.path); + + const run = runCli(['sync', 'validate', '--locale', 'zz']); + + expect(run.status).toBe(7); + expect(run.output.toLowerCase()).not.toContain('passed validation'); + }); + + it('exits 7 on `sync export --locale ` instead of exporting every locale', () => { + writeSyncConfig(testFiles.path, ['de', 'fr']); + writeSourceFile(testFiles.path); + + const run = runCli(['sync', 'export', '--locale', 'zz']); + + expect(run.status).toBe(7); + expect(run.stdout).not.toContain(' { + writeSyncConfig(testFiles.path, ['de', 'fr']); + writeSourceFile(testFiles.path); + + const run = runCli(['sync', 'status', '--locale', 'es', '--format', 'json']); + + expect(run.status).toBe(7); + const envelope = JSON.parse(run.stderr.trim()) as { + ok: boolean; + error: { code: string }; + exitCode: number; + }; + expect(envelope.ok).toBe(false); + expect(envelope.error.code).toBe('ConfigError'); + expect(envelope.exitCode).toBe(7); + }); + + it('still accepts a configured locale on the root command', () => { + writeSyncConfig(testFiles.path, ['de', 'fr']); + writeSourceFile(testFiles.path); + + const run = runCli(['sync', '--locale', 'de', '--dry-run']); + + expect(run.status).toBe(0); + expect(run.output).toContain('dry-run'); + }); + + it('still accepts a configured locale on a subcommand', () => { + writeSyncConfig(testFiles.path, ['de', 'fr']); + writeSourceFile(testFiles.path); + + const run = runCli(['sync', 'status', '--locale', 'de,fr']); + + expect(run.status).toBe(0); + expect(run.output).toMatch(/\bde\b/); + expect(run.output).toMatch(/\bfr\b/); + }); + }); }); diff --git a/tests/unit/sync/sync-config.test.ts b/tests/unit/sync/sync-config.test.ts index a726851..2cad2cc 100644 --- a/tests/unit/sync/sync-config.test.ts +++ b/tests/unit/sync/sync-config.test.ts @@ -1198,6 +1198,28 @@ translation: buckets: { json: { include: ['locales/en.json'] } }, }); + it('should accept a localeFilter whose entries are all in target_locales', () => { + const config = baseConfig(); + config.target_locales = ['de', 'fr']; + expect(() => applyCliOverrides(config, { localeFilter: ['de', 'fr'] })).not.toThrow(); + }); + + it('should throw ConfigError naming the offending and configured locales', () => { + const config = baseConfig(); + config.target_locales = ['de', 'fr']; + expect(() => applyCliOverrides(config, { localeFilter: ['es'] })).toThrow(ConfigError); + expect(() => applyCliOverrides(config, { localeFilter: ['es'] })).toThrow(/es/); + expect(() => applyCliOverrides(config, { localeFilter: ['es'] })).toThrow(/de, fr/); + }); + + it('should report every unconfigured locale in a mixed filter', () => { + const config = baseConfig(); + config.target_locales = ['de', 'fr']; + expect(() => applyCliOverrides(config, { localeFilter: ['de', 'es', 'zz'] })).toThrow( + /es, zz/, + ); + }); + it('should merge formality into existing translation block', () => { const config = baseConfig(); config.translation = { glossary: 'g' }; From 6b50a88071a67638a426a41ea59817a7c51a9654 Mon Sep 17 00:00:00 2001 From: Steven Syrek Date: Wed, 29 Jul 2026 13:58:03 -0400 Subject: [PATCH 12/27] fix(sync): stop using the source hash as a translation identity in sync audit `deepl sync audit` fell back to the lock file's per-locale hash when a target file could not be read. That hash is a hash of the SOURCE text, so it is identical across a term group by construction: divergent translations were reported as consistent, and identical ones as an inconsistency displaying a hex hash where a translation should be. Unreadable targets are now excluded from the comparison and listed in a new `missingTargets` field, which the text output prints as well. --- src/cli/commands/sync/register-sync-audit.ts | 11 +- src/sync/sync-glossary-report.ts | Bin 2712 -> 3095 bytes tests/e2e/cli-sync.e2e.test.ts | 70 ++++ tests/unit/sync/sync-glossary-report.test.ts | 325 ++++++++++--------- 4 files changed, 257 insertions(+), 149 deletions(-) diff --git a/src/cli/commands/sync/register-sync-audit.ts b/src/cli/commands/sync/register-sync-audit.ts index 5f9c39c..95c796d 100644 --- a/src/cli/commands/sync/register-sync-audit.ts +++ b/src/cli/commands/sync/register-sync-audit.ts @@ -93,7 +93,7 @@ async function handleSyncAudit( for (const entry of entries) keyMap.set(entry.key, entry.value); fileLocaleMap.set(locale, keyMap); } catch { - // Unreadable / unparseable target file — fall through to hash identity. + // Unreadable / unparseable target file — reported as a missing target. } } if (fileLocaleMap.size > 0) targetTranslations.set(relPath, fileLocaleMap); @@ -117,6 +117,15 @@ async function handleSyncAudit( Logger.info(` Files: ${inc.files.join(', ')}`); } } + + if (report.missingTargets.length > 0) { + Logger.info( + `\n${report.missingTargets.length} target(s) could not be read and were excluded from the comparison:`, + ); + for (const target of report.missingTargets) { + Logger.info(` ${target.filePath} [${target.locale}]`); + } + } } } catch (error) { if (options.format === 'json') { diff --git a/src/sync/sync-glossary-report.ts b/src/sync/sync-glossary-report.ts index aab5d515914922d831b4f83a72c213f4d4afefdb..c23742ac32eee9e6139d7fc8682be12893a9e35c 100644 GIT binary patch delta 988 zcmZWo!D`e{6m?1|C4#OBb>&$UnhayQZylkuE=xt*l?ZbuxoMtFUxxQyW?H7h_zB7g z`U`FZ>BqS5*0p=@OD4r?U|@3JednHg&dJw}FPk5~&v&|A?8A{cV~0!?j7eTvhBKsX zL5@>6zH~^+sz{+rhatl-r9IdZ%3(ql1!oRrCBF_l7-Gao*%L;#MnG*cElhjn-4q;!T)WQd~eH|_?jzdMWDwbQxE5K`hQDo%Xt`u#pCopPo$r%|00ET5&qv1Jzywx+Fdvj5)^{x9b7*u@}cAIC%N+gOyTM63`S`qVOt7?gt^)Fd^` zCa9|%CmG>%ncZn3HpA1H@VWnoQkt@6waR&h6U>~5xuUX11ZlW1jRHOv$hfNfXbj@Aullig#}Z-K)Ky-O2rv<>4XTC9XIb%m3jiU+58XNKB|Wheq_V zE$EtzCCm_p`1_eu-pQKC21grcPR?ih5#AbCwoN6*j48PAe%i-0?VQm_Z>!}}O=96R zH?m< { // Clean up the nested dirs created above so the next test's beforeEach // doesn't inherit them (beforeEach only wipes locales/ at the top). }); + + it('reports a not-yet-created locale file as missing instead of an inconsistency', () => { + const sourceDir = path.join(testFiles.path, 'locales', 'en'); + fs.mkdirSync(sourceDir, { recursive: true }); + fs.writeFileSync(path.join(sourceDir, 'common.json'), JSON.stringify({ greeting: 'Dashboard' }, null, 2)); + fs.writeFileSync(path.join(sourceDir, 'admin.json'), JSON.stringify({ header: 'Dashboard' }, null, 2)); + + // Only one of the two German files exists yet. + const targetDir = path.join(testFiles.path, 'locales', 'de'); + fs.mkdirSync(targetDir, { recursive: true }); + fs.writeFileSync(path.join(targetDir, 'common.json'), JSON.stringify({ greeting: 'Armaturenbrett' }, null, 2)); + + fs.writeFileSync( + path.join(testFiles.path, '.deepl-sync.yaml'), + [ + 'version: 1', + 'source_locale: en', + 'target_locales:', + ' - de', + 'buckets:', + ' json:', + ' include:', + ' - "locales/en/*.json"', + '', + ].join('\n'), + ); + + const lockContent = { + _comment: 'test', + version: 1, + generated_at: '2026-04-19T00:00:00Z', + source_locale: 'en', + entries: { + 'locales/en/common.json': { + greeting: { + source_hash: 'sh', + source_text: 'Dashboard', + translations: { + de: { hash: 'de-hash-a', translated_at: '2026-04-19T00:00:00Z', status: 'translated' }, + }, + }, + }, + 'locales/en/admin.json': { + header: { + source_hash: 'sh', + source_text: 'Dashboard', + translations: { + de: { hash: 'de-hash-a', translated_at: '2026-04-19T00:00:00Z', status: 'translated' }, + }, + }, + }, + }, + stats: { total_keys: 2, total_translations: 2, last_sync: '2026-04-19T00:00:00Z' }, + }; + fs.writeFileSync(path.join(testFiles.path, '.deepl-sync.lock'), JSON.stringify(lockContent, null, 2)); + + const output = runSyncAll('audit --format json'); + const jsonMatch = output.match(/\{[\s\S]*\}/); + expect(jsonMatch).not.toBeNull(); + const parsed = JSON.parse(jsonMatch![0]) as { + inconsistencies: Array<{ translations: string[] }>; + missingTargets: Array<{ filePath: string; locale: string }>; + }; + + expect(parsed.inconsistencies).toHaveLength(0); + expect(parsed.missingTargets).toEqual([ + { filePath: 'locales/en/admin.json', locale: 'de' }, + ]); + expect(output).not.toContain('de-hash-a'); + }); }); describe('sync export output safety', () => { diff --git a/tests/unit/sync/sync-glossary-report.test.ts b/tests/unit/sync/sync-glossary-report.test.ts index 81e10ac..a6b0765 100644 --- a/tests/unit/sync/sync-glossary-report.test.ts +++ b/tests/unit/sync/sync-glossary-report.test.ts @@ -1,4 +1,5 @@ import { generateGlossaryReport } from '../../../src/sync/sync-glossary-report'; +import type { TargetTranslationIndex } from '../../../src/sync/sync-glossary-report'; import type { SyncLockFile } from '../../../src/sync/types'; function makeLockFile(entries: SyncLockFile['entries'] = {}): SyncLockFile { @@ -12,166 +13,130 @@ function makeLockFile(entries: SyncLockFile['entries'] = {}): SyncLockFile { }; } +function translated(hash: string) { + return { hash, translated_at: '2026-01-01T00:00:00Z', status: 'translated' as const }; +} + +function index( + spec: Record>>, +): TargetTranslationIndex { + const result: TargetTranslationIndex = new Map(); + for (const [file, locales] of Object.entries(spec)) { + const localeMap = new Map>(); + for (const [locale, keys] of Object.entries(locales)) { + localeMap.set(locale, new Map(Object.entries(keys))); + } + result.set(file, localeMap); + } + return result; +} + describe('generateGlossaryReport()', () => { - it('should report no inconsistencies for consistent translations', () => { + it('should report no inconsistencies when the same source text is translated identically', () => { const lock = makeLockFile({ 'en/common.json': { - greeting: { - source_hash: 'abc123', - source_text: 'Hello', - translations: { - de: { hash: 'hash_de_1', translated_at: '2026-01-01T00:00:00Z', status: 'translated' }, - fr: { hash: 'hash_fr_1', translated_at: '2026-01-01T00:00:00Z', status: 'translated' }, - }, - }, + greeting: { source_hash: 'abc123', source_text: 'Hello', translations: { de: translated('h1') } }, }, 'en/other.json': { - greeting2: { - source_hash: 'abc123', - source_text: 'Hello', - translations: { - de: { hash: 'hash_de_1', translated_at: '2026-01-01T00:00:00Z', status: 'translated' }, - fr: { hash: 'hash_fr_1', translated_at: '2026-01-01T00:00:00Z', status: 'translated' }, - }, - }, + greeting2: { source_hash: 'abc123', source_text: 'Hello', translations: { de: translated('h1') } }, }, }); - const report = generateGlossaryReport(lock); + const report = generateGlossaryReport( + lock, + index({ + 'en/common.json': { de: { greeting: 'Hallo' } }, + 'en/other.json': { de: { greeting2: 'Hallo' } }, + }), + ); expect(report.totalTerms).toBe(1); expect(report.inconsistencies).toHaveLength(0); + expect(report.missingTargets).toHaveLength(0); }); - it('should detect inconsistency when same source_text has different hashes for same locale', () => { + it('should detect an inconsistency when the same source text has different translations', () => { const lock = makeLockFile({ 'en/common.json': { - greeting: { - source_hash: 'abc123', - source_text: 'Hello', - translations: { - de: { hash: 'hash_de_v1', translated_at: '2026-01-01T00:00:00Z', status: 'translated' }, - }, - }, + greeting: { source_hash: 'abc123', source_text: 'Hello', translations: { de: translated('h1') } }, }, 'en/other.json': { - welcome: { - source_hash: 'abc123', - source_text: 'Hello', - translations: { - de: { hash: 'hash_de_v2', translated_at: '2026-01-02T00:00:00Z', status: 'translated' }, - }, - }, + welcome: { source_hash: 'abc123', source_text: 'Hello', translations: { de: translated('h1') } }, }, }); - const report = generateGlossaryReport(lock); + const report = generateGlossaryReport( + lock, + index({ + 'en/common.json': { de: { greeting: 'Hallo' } }, + 'en/other.json': { de: { welcome: 'Guten Tag' } }, + }), + ); expect(report.inconsistencies).toHaveLength(1); expect(report.inconsistencies[0]).toEqual({ sourceText: 'Hello', locale: 'de', - translations: expect.arrayContaining(['hash_de_v1', 'hash_de_v2']), + translations: expect.arrayContaining(['Hallo', 'Guten Tag']), files: expect.arrayContaining(['en/common.json', 'en/other.json']), }); }); it('should return zero terms and no inconsistencies for empty lock file', () => { - const lock = makeLockFile({}); - - const report = generateGlossaryReport(lock); + const report = generateGlossaryReport(makeLockFile({})); expect(report.totalTerms).toBe(0); expect(report.inconsistencies).toHaveLength(0); + expect(report.missingTargets).toHaveLength(0); }); it('should populate files array correctly for multiple files with same source text', () => { const lock = makeLockFile({ 'file-a.json': { - key1: { - source_hash: 'h1', - source_text: 'Save', - translations: { - de: { hash: 'same_hash', translated_at: '2026-01-01T00:00:00Z', status: 'translated' }, - }, - }, + key1: { source_hash: 'h1', source_text: 'Save', translations: { de: translated('h') } }, }, 'file-b.json': { - key2: { - source_hash: 'h1', - source_text: 'Save', - translations: { - de: { hash: 'same_hash', translated_at: '2026-01-01T00:00:00Z', status: 'translated' }, - }, - }, + key2: { source_hash: 'h1', source_text: 'Save', translations: { de: translated('h') } }, }, 'file-c.json': { - key3: { - source_hash: 'h1', - source_text: 'Save', - translations: { - de: { hash: 'same_hash', translated_at: '2026-01-01T00:00:00Z', status: 'translated' }, - }, - }, + key3: { source_hash: 'h1', source_text: 'Save', translations: { de: translated('h') } }, }, }); - const report = generateGlossaryReport(lock); + const report = generateGlossaryReport( + lock, + index({ + 'file-a.json': { de: { key1: 'Speichern' } }, + 'file-b.json': { de: { key2: 'Speichern' } }, + 'file-c.json': { de: { key3: 'Sichern' } }, + }), + ); expect(report.totalTerms).toBe(1); - expect(report.inconsistencies).toHaveLength(0); - - // Even with no inconsistency, verify the internal grouping worked by - // introducing a divergent hash and checking the files list - const lockWithDivergence = makeLockFile({ - ...lock.entries, - 'file-c.json': { - key3: { - source_hash: 'h1', - source_text: 'Save', - translations: { - de: { hash: 'different_hash', translated_at: '2026-01-01T00:00:00Z', status: 'translated' }, - }, - }, - }, - }); - - const report2 = generateGlossaryReport(lockWithDivergence); - expect(report2.inconsistencies).toHaveLength(1); - expect(report2.inconsistencies[0]!.files).toEqual( + expect(report.inconsistencies).toHaveLength(1); + expect(report.inconsistencies[0]!.files).toEqual( expect.arrayContaining(['file-a.json', 'file-b.json', 'file-c.json']), ); - expect(report2.inconsistencies[0]!.files).toHaveLength(3); + expect(report.inconsistencies[0]!.files).toHaveLength(3); }); it('surfaces actual translated text (not hashes) when a target-translation index is provided', () => { const lock = makeLockFile({ 'en/common.json': { - greeting: { - source_hash: 'h', - source_text: 'Dashboard', - translations: { - de: { hash: 'de-v1', translated_at: '2026-01-01T00:00:00Z', status: 'translated' }, - }, - }, + greeting: { source_hash: 'h', source_text: 'Dashboard', translations: { de: translated('de-v1') } }, }, 'en/admin.json': { - header: { - source_hash: 'h', - source_text: 'Dashboard', - translations: { - de: { hash: 'de-v2', translated_at: '2026-01-01T00:00:00Z', status: 'translated' }, - }, - }, + header: { source_hash: 'h', source_text: 'Dashboard', translations: { de: translated('de-v2') } }, }, }); - const targets = new Map([ - ['en/common.json', new Map([['de', new Map([['greeting', 'Armaturenbrett']])]])], - ['en/admin.json', new Map([['de', new Map([['header', 'Dashboard']])]])], - ]); - - const report = generateGlossaryReport(lock, targets); + const report = generateGlossaryReport( + lock, + index({ + 'en/common.json': { de: { greeting: 'Armaturenbrett' } }, + 'en/admin.json': { de: { header: 'Dashboard' } }, + }), + ); expect(report.inconsistencies).toHaveLength(1); const inc = report.inconsistencies[0]!; @@ -182,69 +147,133 @@ describe('generateGlossaryReport()', () => { expect(inc.translations).not.toContain('de-v2'); }); - it('falls back to the hash when the target-translation index is missing an entry', () => { - const lock = makeLockFile({ - 'en/common.json': { - greeting: { - source_hash: 'h', - source_text: 'Hi', - translations: { - de: { hash: 'de-hashA', translated_at: '2026-01-01T00:00:00Z', status: 'translated' }, - }, + describe('unreadable targets', () => { + it('reports a missing locale file instead of inventing an inconsistency', () => { + const lock = makeLockFile({ + 'en/common.json': { + greeting: { source_hash: 'h', source_text: 'Hi', translations: { de: translated('de-hashA') } }, }, - }, - 'en/other.json': { - greeting2: { - source_hash: 'h', - source_text: 'Hi', - translations: { - de: { hash: 'de-hashB', translated_at: '2026-01-01T00:00:00Z', status: 'translated' }, - }, + 'en/other.json': { + greeting2: { source_hash: 'h', source_text: 'Hi', translations: { de: translated('de-hashB') } }, }, - }, + }); + + // en/other.json has no de file yet. + const report = generateGlossaryReport( + lock, + index({ 'en/common.json': { de: { greeting: 'Hallo' } } }), + ); + + expect(report.inconsistencies).toHaveLength(0); + expect(report.missingTargets).toEqual([{ filePath: 'en/other.json', locale: 'de' }]); }); - // Only en/common.json has a target; en/other.json is missing from the index. - const partialTargets = new Map([ - ['en/common.json', new Map([['de', new Map([['greeting', 'Hallo']])]])], - ]); + it('never emits a lock hash as a translation string', () => { + const lock = makeLockFile({ + 'en/common.json': { + greeting: { source_hash: 'h', source_text: 'Hi', translations: { de: translated('de-hashA') } }, + }, + 'en/other.json': { + greeting2: { source_hash: 'h', source_text: 'Hi', translations: { de: translated('de-hashB') } }, + }, + }); - const report = generateGlossaryReport(lock, partialTargets); + const report = generateGlossaryReport( + lock, + index({ 'en/common.json': { de: { greeting: 'Hallo' } } }), + ); - expect(report.inconsistencies).toHaveLength(1); - expect(report.inconsistencies[0]!.translations).toEqual(expect.arrayContaining(['Hallo', 'de-hashB'])); + const allTranslations = report.inconsistencies.flatMap(inc => inc.translations); + expect(allTranslations).not.toContain('de-hashA'); + expect(allTranslations).not.toContain('de-hashB'); + }); + + it('still detects divergence among the readable targets of a group', () => { + const lock = makeLockFile({ + 'a.json': { k1: { source_hash: 'h', source_text: 'Hi', translations: { de: translated('x') } } }, + 'b.json': { k2: { source_hash: 'h', source_text: 'Hi', translations: { de: translated('x') } } }, + 'c.json': { k3: { source_hash: 'h', source_text: 'Hi', translations: { de: translated('x') } } }, + }); + + const report = generateGlossaryReport( + lock, + index({ + 'a.json': { de: { k1: 'Hallo' } }, + 'b.json': { de: { k2: 'Servus' } }, + }), + ); + + expect(report.inconsistencies).toHaveLength(1); + expect(report.inconsistencies[0]!.translations).toEqual( + expect.arrayContaining(['Hallo', 'Servus']), + ); + expect(report.inconsistencies[0]!.files).toEqual(['a.json', 'b.json']); + expect(report.missingTargets).toEqual([{ filePath: 'c.json', locale: 'de' }]); + }); + + it('reports every unreadable target once per file and locale', () => { + const lock = makeLockFile({ + 'a.json': { + k1: { source_hash: 'h', source_text: 'Hi', translations: { de: translated('x'), fr: translated('x') } }, + k2: { source_hash: 'h2', source_text: 'Bye', translations: { de: translated('y') } }, + }, + }); + + const report = generateGlossaryReport(lock, index({})); + + expect(report.missingTargets).toEqual([ + { filePath: 'a.json', locale: 'de' }, + { filePath: 'a.json', locale: 'fr' }, + ]); + }); + + it('treats an omitted index as every target being unreadable', () => { + const lock = makeLockFile({ + 'a.json': { k1: { source_hash: 'h', source_text: 'Hi', translations: { de: translated('x') } } }, + 'b.json': { k2: { source_hash: 'h', source_text: 'Hi', translations: { de: translated('y') } } }, + }); + + const report = generateGlossaryReport(lock); + + expect(report.inconsistencies).toHaveLength(0); + expect(report.missingTargets).toHaveLength(2); + }); }); it('should count different source texts separately in totalTerms', () => { const lock = makeLockFile({ 'en/common.json': { - greeting: { - source_hash: 'h1', - source_text: 'Hello', - translations: { - de: { hash: 'hash1', translated_at: '2026-01-01T00:00:00Z', status: 'translated' }, - }, - }, - farewell: { - source_hash: 'h2', - source_text: 'Goodbye', - translations: { - de: { hash: 'hash2', translated_at: '2026-01-01T00:00:00Z', status: 'translated' }, - }, - }, - save: { - source_hash: 'h3', - source_text: 'Save', - translations: { - de: { hash: 'hash3', translated_at: '2026-01-01T00:00:00Z', status: 'translated' }, - }, - }, + greeting: { source_hash: 'h1', source_text: 'Hello', translations: { de: translated('hash1') } }, + farewell: { source_hash: 'h2', source_text: 'Goodbye', translations: { de: translated('hash2') } }, + save: { source_hash: 'h3', source_text: 'Save', translations: { de: translated('hash3') } }, }, }); - const report = generateGlossaryReport(lock); + const report = generateGlossaryReport( + lock, + index({ + 'en/common.json': { de: { greeting: 'Hallo', farewell: 'Auf Wiedersehen', save: 'Speichern' } }, + }), + ); expect(report.totalTerms).toBe(3); expect(report.inconsistencies).toHaveLength(0); }); + + it('should ignore translations that have not reached the translated status', () => { + const lock = makeLockFile({ + 'a.json': { + k1: { + source_hash: 'h', + source_text: 'Hi', + translations: { de: { hash: 'x', translated_at: '2026-01-01T00:00:00Z', status: 'pending' } }, + }, + }, + }); + + const report = generateGlossaryReport(lock, index({})); + + expect(report.inconsistencies).toHaveLength(0); + expect(report.missingTargets).toHaveLength(0); + }); }); From 3ab502d82057d1a5490e77770762a032423150a5 Mon Sep 17 00:00:00 2001 From: Steven Syrek Date: Wed, 29 Jul 2026 13:58:08 -0400 Subject: [PATCH 13/27] fix(sync): honor --sync-config in sync init `sync init` registered --sync-config but always resolved the config against process.cwd(), so the flag could neither redirect where the file was written nor which existing file triggered the already-exists guard. Resolve the flag to the config file path (any basename, matching what loadSyncConfig accepts) and use its directory as the root that detection and the generated globs are relative to. configExists() and writeSyncConfig() now take that path instead of a root directory. --- src/cli/commands/sync/register-sync-init.ts | 23 ++++--- src/sync/sync-init.ts | 18 +++-- tests/e2e/cli-sync-option-routing.e2e.test.ts | 66 +++++++++++++++++++ tests/unit/sync/sync-init.test.ts | 35 +++++++++- 4 files changed, 127 insertions(+), 15 deletions(-) diff --git a/src/cli/commands/sync/register-sync-init.ts b/src/cli/commands/sync/register-sync-init.ts index 56ca5cc..d0c6a6d 100644 --- a/src/cli/commands/sync/register-sync-init.ts +++ b/src/cli/commands/sync/register-sync-init.ts @@ -8,6 +8,7 @@ import { emitJsonErrorAndExit, emitJsonInitSuccessAndExit, resolveFormat, + resolveSyncConfig, } from './sync-options.js'; import { ExitCode } from '../../../utils/exit-codes.js'; @@ -88,12 +89,18 @@ async function handleSyncInit( try { applyDeprecationAliases(options); - const { configExists, detectI18nFiles, generateSyncConfig, writeSyncConfig } = + const { configExists, detectI18nFiles, generateSyncConfig, resolveInitConfigPath, writeSyncConfig } = await import('../../../sync/sync-init.js'); + const pathMod = await import('path'); - const cwd = process.cwd(); + const targetConfigPath = resolveInitConfigPath( + process.cwd(), + resolveSyncConfig(options, command), + ); + const cwd = pathMod.dirname(targetConfigPath); + const displayPath = pathMod.relative(process.cwd(), targetConfigPath) || targetConfigPath; - if (configExists(cwd)) { + if (configExists(targetConfigPath)) { if (options.format === 'json') { // Already-present config is not an error per se, but scripted // bootstrap flows need a non-ok envelope to branch on. @@ -101,16 +108,16 @@ async function handleSyncInit( ok: false, error: { code: 'ConfigError', - message: 'Config file .deepl-sync.yaml already exists.', + message: `Config file ${displayPath} already exists.`, suggestion: - 'Remove or rename the existing .deepl-sync.yaml, or edit it directly.', + 'Remove or rename the existing config file, or edit it directly.', }, exitCode: ExitCode.ConfigError, }; process.stderr.write(JSON.stringify(envelope) + '\n'); process.exit(ExitCode.ConfigError); } - Logger.warn(chalk.yellow('Config file .deepl-sync.yaml already exists.')); + Logger.warn(chalk.yellow(`Config file ${displayPath} already exists.`)); return; } @@ -133,7 +140,7 @@ async function handleSyncInit( format: options.fileFormat, pattern: options.path, }); - const configPath = await writeSyncConfig(cwd, content); + const configPath = await writeSyncConfig(targetConfigPath, content); emitInitSuccess(options.format, { configPath, sourceLocale: validated.sourceLocale, @@ -203,7 +210,7 @@ async function handleSyncInit( pattern: options.path ?? project.pattern, targetPathPattern: options.path ? undefined : project.targetPathPattern, }); - const configPath = await writeSyncConfig(cwd, content); + const configPath = await writeSyncConfig(targetConfigPath, content); emitInitSuccess(options.format, { configPath, sourceLocale, diff --git a/src/sync/sync-init.ts b/src/sync/sync-init.ts index 05ed50d..9fcd763 100644 --- a/src/sync/sync-init.ts +++ b/src/sync/sync-init.ts @@ -343,12 +343,22 @@ export function generateSyncConfig(opts: { return YAML.stringify(config); } -export async function writeSyncConfig(rootDir: string, content: string): Promise { - const configPath = path.join(rootDir, SYNC_CONFIG_FILENAME); +/** + * Resolve the config file `deepl sync init` should create. `--sync-config` + * names the file itself (any basename, matching what `loadSyncConfig` accepts), + * so its directory becomes the project root that detection and the generated + * globs are relative to. + */ +export function resolveInitConfigPath(cwd: string, configPath?: string): string { + return configPath ? path.resolve(cwd, configPath) : path.join(cwd, SYNC_CONFIG_FILENAME); +} + +export async function writeSyncConfig(configPath: string, content: string): Promise { + await fs.promises.mkdir(path.dirname(configPath), { recursive: true }); await fs.promises.writeFile(configPath, content, 'utf-8'); return configPath; } -export function configExists(rootDir: string): boolean { - return fs.existsSync(path.join(rootDir, SYNC_CONFIG_FILENAME)); +export function configExists(configPath: string): boolean { + return fs.existsSync(configPath); } diff --git a/tests/e2e/cli-sync-option-routing.e2e.test.ts b/tests/e2e/cli-sync-option-routing.e2e.test.ts index 9f390fe..2d6927d 100644 --- a/tests/e2e/cli-sync-option-routing.e2e.test.ts +++ b/tests/e2e/cli-sync-option-routing.e2e.test.ts @@ -240,4 +240,70 @@ describe('CLI sync option routing E2E', () => { expect(run.output).toMatch(/\bfr\b/); }); }); + + describe('sync init --sync-config', () => { + it('writes the config at the requested path', () => { + const altRoot = path.join(testFiles.path, 'alt'); + fs.mkdirSync(altRoot, { recursive: true }); + writeSourceFile(altRoot); + + const target = path.join(altRoot, 'custom-sync.yaml'); + const run = runCli([ + 'sync', 'init', + '--sync-config', target, + '--source-locale', 'en', + '--target-locales', 'de', + '--file-format', 'json', + '--path', 'locales/en.json', + ]); + + expect(run.status).toBe(0); + expect(fs.existsSync(target)).toBe(true); + expect(fs.existsSync(path.join(testFiles.path, '.deepl-sync.yaml'))).toBe(false); + expect(fs.readFileSync(target, 'utf-8')).toContain('source_locale: en'); + }); + + it('does not treat an unrelated cwd config as the already-exists case', () => { + writeSyncConfig(testFiles.path, ['de', 'fr']); + writeSourceFile(testFiles.path); + + const altRoot = path.join(testFiles.path, 'alt'); + fs.mkdirSync(altRoot, { recursive: true }); + writeSourceFile(altRoot); + + const target = path.join(altRoot, '.deepl-sync.yaml'); + const run = runCli([ + 'sync', 'init', + '--sync-config', target, + '--source-locale', 'en', + '--target-locales', 'it', + '--file-format', 'json', + '--path', 'locales/en.json', + ]); + + expect(run.status).toBe(0); + expect(run.output).not.toContain('already exists'); + expect(fs.existsSync(target)).toBe(true); + expect(fs.readFileSync(target, 'utf-8')).toContain('it'); + }); + + it('reports the already-exists case against the --sync-config path', () => { + const altRoot = path.join(testFiles.path, 'alt'); + fs.mkdirSync(altRoot, { recursive: true }); + writeSourceFile(altRoot); + const target = writeSyncConfig(altRoot, ['de']); + + const run = runCli([ + 'sync', 'init', + '--sync-config', target, + '--source-locale', 'en', + '--target-locales', 'it', + '--file-format', 'json', + '--path', 'locales/en.json', + ]); + + expect(run.output).toContain('already exists'); + expect(fs.readFileSync(target, 'utf-8')).not.toContain('it'); + }); + }); }); diff --git a/tests/unit/sync/sync-init.test.ts b/tests/unit/sync/sync-init.test.ts index 82c116f..cb6fbd9 100644 --- a/tests/unit/sync/sync-init.test.ts +++ b/tests/unit/sync/sync-init.test.ts @@ -17,6 +17,7 @@ import { generateSyncConfig, writeSyncConfig, configExists, + resolveInitConfigPath, } from '../../../src/sync/sync-init'; const mockFg = fg as jest.MockedFunction; @@ -653,22 +654,50 @@ describe('sync-init', () => { it('should return true when config file exists', () => { fs.writeFileSync(path.join(testDir, '.deepl-sync.yaml'), 'version: 1\n'); - expect(configExists(testDir)).toBe(true); + expect(configExists(path.join(testDir, '.deepl-sync.yaml'))).toBe(true); }); it('should return false when config file does not exist', () => { - expect(configExists(testDir)).toBe(false); + expect(configExists(path.join(testDir, '.deepl-sync.yaml'))).toBe(false); + }); + }); + + describe('resolveInitConfigPath', () => { + it('should default to .deepl-sync.yaml in the cwd', () => { + expect(resolveInitConfigPath(testDir)).toBe(path.join(testDir, '.deepl-sync.yaml')); + }); + + it('should honor an explicit path with a custom basename', () => { + expect(resolveInitConfigPath(testDir, 'nested/custom.yaml')).toBe( + path.join(testDir, 'nested', 'custom.yaml'), + ); + }); + + it('should leave an absolute explicit path untouched', () => { + const abs = path.join(testDir, 'abs', 'custom.yaml'); + expect(resolveInitConfigPath(testDir, abs)).toBe(abs); }); }); describe('writeSyncConfig', () => { it('should write config file and return path', async () => { const content = 'version: 1\nsource_locale: en\n'; - const configPath = await writeSyncConfig(testDir, content); + const configPath = await writeSyncConfig( + path.join(testDir, '.deepl-sync.yaml'), + content, + ); expect(configPath).toBe(path.join(testDir, '.deepl-sync.yaml')); expect(fs.readFileSync(configPath, 'utf-8')).toBe(content); }); + + it('should create missing parent directories', async () => { + const target = path.join(testDir, 'deep', 'nested', 'custom.yaml'); + const configPath = await writeSyncConfig(target, 'version: 1\n'); + + expect(configPath).toBe(target); + expect(fs.existsSync(target)).toBe(true); + }); }); describe('non-interactive generation', () => { From f886bb549a5f7e9af8b9b342f83d847b4c8680f8 Mon Sep 17 00:00:00 2001 From: Steven Syrek Date: Wed, 29 Jul 2026 13:59:26 -0400 Subject: [PATCH 14/27] fix(api): never retry the document result endpoint and widen transfer timeouts The result endpoint is effectively single-use, yet a client-side timeout replayed the download three more times; a replay against an exhausted result can permanently lose an already-billed translation. Downloads now run with retries disabled. Uploads and downloads also move whole files, so both now get a 300s transfer timeout instead of the 30s interactive request timeout (an explicitly configured longer timeout still wins). Status polling keeps the default timeout. --- src/api/document-client.ts | 19 ++++++- tests/unit/document-client.test.ts | 81 ++++++++++++++++++++++++++++++ 2 files changed, 98 insertions(+), 2 deletions(-) diff --git a/src/api/document-client.ts b/src/api/document-client.ts index f50e240..6849094 100644 --- a/src/api/document-client.ts +++ b/src/api/document-client.ts @@ -16,11 +16,19 @@ interface DeepLDocumentStatusResponse { error_message?: string; } +/** Uploads and downloads move whole files, so they need far more headroom + * than the interactive request timeout. */ +const TRANSFER_TIMEOUT_MS = 300_000; + export class DocumentClient extends HttpClient { constructor(apiKey: string, options: DeepLClientOptions = {}) { super(apiKey, options); } + private get transferTimeout(): number { + return Math.max(this.requestTimeout, TRANSFER_TIMEOUT_MS); + } + async uploadDocument( file: Buffer, options: DocumentTranslationOptions @@ -69,7 +77,8 @@ export class DocumentClient extends HttpClient { ...formData.getHeaders(), }, }; - } + }, + { timeout: this.transferTimeout } ); return { @@ -101,6 +110,11 @@ export class DocumentClient extends HttpClient { } } + /** + * The result endpoint is effectively single-use: a replayed download can + * consume the result of an already-billed translation and lose it + * permanently, so this request is never retried. + */ async downloadDocument(handle: DocumentHandle): Promise { try { const response = await this.makeRawRequest( @@ -116,7 +130,8 @@ export class DocumentClient extends HttpClient { }, responseType: 'arraybuffer', }; - } + }, + { maxRetries: 0, timeout: this.transferTimeout } ); return Buffer.from(response); diff --git a/tests/unit/document-client.test.ts b/tests/unit/document-client.test.ts index d39d17b..d3b252f 100644 --- a/tests/unit/document-client.test.ts +++ b/tests/unit/document-client.test.ts @@ -256,4 +256,85 @@ describe('DocumentClient', () => { ).rejects.toThrow(); }); }); + + describe('transfer policy', () => { + // The result endpoint is effectively single-use: a replay can consume the + // download of an already-billed translation and lose it permanently. + const unsentError = { + isAxiosError: true, + code: 'ECONNREFUSED', + message: 'connect ECONNREFUSED 127.0.0.1:443', + }; + + beforeEach(() => { + jest.spyOn(axios, 'isAxiosError').mockReturnValue(true); + }); + + it('should never retry the download endpoint, even on a refused connection', async () => { + mockAxiosInstance.request.mockRejectedValue(unsentError); + + await expect( + client.downloadDocument({ documentId: 'doc-1', documentKey: 'key-1' }) + ).rejects.toThrow(); + + expect(mockAxiosInstance.request).toHaveBeenCalledTimes(1); + }); + + it('should still retry an upload whose connection was refused', async () => { + mockAxiosInstance.request.mockRejectedValue(unsentError); + + await expect( + client.uploadDocument(Buffer.from('content'), { + targetLang: 'de', + filename: 'test.txt', + }) + ).rejects.toThrow(); + + expect(mockAxiosInstance.request).toHaveBeenCalledTimes(4); + }); + + it('should give document transfers a larger timeout than the default request timeout', async () => { + mockAxiosInstance.request.mockResolvedValue({ + data: Buffer.from('content'), + status: 200, + headers: {}, + }); + + await client.downloadDocument({ documentId: 'doc-1', documentKey: 'key-1' }); + + expect(mockAxiosInstance.request).toHaveBeenCalledWith( + expect.objectContaining({ timeout: 300000 }), + ); + }); + + it('should honour an explicitly configured timeout larger than the transfer default', async () => { + const slowClient = new DocumentClient('test-api-key', { timeout: 600000 }); + mockAxiosInstance.request.mockResolvedValue({ + data: Buffer.from('content'), + status: 200, + headers: {}, + }); + + await slowClient.downloadDocument({ documentId: 'doc-1', documentKey: 'key-1' }); + + expect(mockAxiosInstance.request).toHaveBeenCalledWith( + expect.objectContaining({ timeout: 600000 }), + ); + slowClient.destroy(); + }); + + it('should leave status polling on the default request timeout', async () => { + mockAxiosInstance.request.mockResolvedValue({ + data: { document_id: 'doc-1', status: 'done' }, + status: 200, + headers: {}, + }); + + await client.getDocumentStatus({ documentId: 'doc-1', documentKey: 'key-1' }); + + expect(mockAxiosInstance.request).toHaveBeenCalledWith( + expect.objectContaining({ timeout: 30000 }), + ); + }); + }); }); From a644a07fccf625a7b4dcb5552ff533b0b92c5abd Mon Sep 17 00:00:00 2001 From: Steven Syrek Date: Wed, 29 Jul 2026 14:01:12 -0400 Subject: [PATCH 15/27] fix(sync): harden TMS request URLs, timeout classification and pull body size MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Build request URLs through the URL API so a trailing slash on server: no longer yields "//", a base path survives, and a query string or fragment is rejected up front instead of silently truncating the API path. Timeouts now throw TmsTimeoutError, a NetworkError subclass, so a hung TMS exits 5 instead of falling through the untyped-message classifier to 1. Error messages route the server URL through sanitizeUrl, which redacts embedded credentials; the helper moved to src/utils/sanitize-url.ts and is still re-exported from http-client. pullKeys reads the body under a 32MiB cap — the existing key and value limits only applied after the whole response had already been parsed. --- src/api/http-client.ts | 14 +-- src/sync/tms-client.ts | 88 +++++++++++++-- src/utils/sanitize-url.ts | 16 +++ tests/unit/sync/tms-client.test.ts | 176 ++++++++++++++++++++++++++++- 4 files changed, 270 insertions(+), 24 deletions(-) create mode 100644 src/utils/sanitize-url.ts diff --git a/src/api/http-client.ts b/src/api/http-client.ts index b323ed5..9a583d4 100644 --- a/src/api/http-client.ts +++ b/src/api/http-client.ts @@ -13,6 +13,7 @@ import { import { Logger } from '../utils/logger.js'; import { errorMessage } from '../utils/error-message.js'; import { sanitizeForTerminal } from '../utils/control-chars.js'; +import { sanitizeUrl } from '../utils/sanitize-url.js'; import { VERSION } from '../version.js'; export const USER_AGENT = `deepl-cli/${VERSION} node/${process.versions.node}`; @@ -35,18 +36,7 @@ export interface DeepLClientOptions { proxy?: ProxyConfig; } -export function sanitizeUrl(url: string): string { - try { - const parsed = new URL(url); - if (parsed.username || parsed.password) { - parsed.username = '***'; - parsed.password = '***'; - } - return parsed.toString(); - } catch { - return '[invalid URL]'; - } -} +export { sanitizeUrl }; export const FREE_API_URL = 'https://api-free.deepl.com'; export const PRO_API_URL = 'https://api.deepl.com'; diff --git a/src/sync/tms-client.ts b/src/sync/tms-client.ts index 2cf1d1e..ccb2745 100644 --- a/src/sync/tms-client.ts +++ b/src/sync/tms-client.ts @@ -1,11 +1,13 @@ -import { ConfigError, ValidationError } from '../utils/errors.js'; +import { ConfigError, NetworkError, ValidationError } from '../utils/errors.js'; import { sanitizeForTerminal } from '../utils/control-chars.js'; +import { sanitizeUrl } from '../utils/sanitize-url.js'; import { Logger } from '../utils/logger.js'; import type { ExtractedEntry } from '../formats/format.js'; import type { SyncTmsConfig } from './types.js'; const MAX_PULL_VALUE_BYTES = 64 * 1024; export const MAX_PULL_KEY_COUNT = 50000; +export const MAX_PULL_BODY_BYTES = 32 * 1024 * 1024; // eslint-disable-next-line no-control-regex -- intentional: checking for control chars in untrusted TMS-returned keys const KEY_FORBIDDEN_CHARS = /[\x00-\x1f\x7f/\\]/; // eslint-disable-next-line no-control-regex -- intentional: strip control chars from untrusted TMS-returned values before they reach the filesystem @@ -54,6 +56,14 @@ export function sanitizePullKeysResponse(raw: unknown): Record { return result; } +/** A TMS request that hit the client-side timeout; classified as a network error (exit 5). */ +export class TmsTimeoutError extends NetworkError { + constructor(message: string) { + super(message, 'Raise timeout_ms in the tms: block of .deepl-sync.yaml, or check that the TMS server is reachable.'); + this.name = 'TmsTimeoutError'; + } +} + export interface TmsClientRetryOptions { maxAttempts?: number; baseDelayMs?: number; @@ -124,6 +134,47 @@ async function readErrorBody(response: Response): Promise { } } +function oversizedBodyError(): ValidationError { + return new ValidationError( + `TMS pull response exceeds the ${MAX_PULL_BODY_BYTES / (1024 * 1024)}MiB response size limit`, + 'Partition the TMS export by locale, or paginate the pull.', + ); +} + +/** + * Read and parse a JSON body without buffering more than the cap. The key and + * value limits in sanitizePullKeysResponse only apply after a parse, so an + * unbounded body would already have been materialized by then. + */ +async function readJsonBounded(response: Response): Promise { + const declared = Number(response.headers?.get('content-length')); + if (Number.isFinite(declared) && declared > MAX_PULL_BODY_BYTES) { + throw oversizedBodyError(); + } + + const body = response.body; + if (!body || typeof body.getReader !== 'function') { + return await response.json(); + } + + const reader = body.getReader(); + const chunks: Uint8Array[] = []; + let total = 0; + for (;;) { + const { done, value } = await reader.read(); + if (done) break; + if (!value) continue; + total += value.byteLength; + if (total > MAX_PULL_BODY_BYTES) { + await reader.cancel(); + throw oversizedBodyError(); + } + chunks.push(value); + } + + return JSON.parse(Buffer.concat(chunks).toString('utf-8')); +} + export class TmsClient { private readonly timeoutMs: number; private readonly retry: Required; @@ -157,11 +208,9 @@ export class TmsClient { }); } catch (err) { if (isAbortError(err)) { - const timeoutErr = new Error( - `TMS request timed out after ${this.timeoutMs}ms: ${method} ${url}`, + throw new TmsTimeoutError( + `TMS request timed out after ${this.timeoutMs}ms: ${method} ${sanitizeUrl(url)}`, ); - timeoutErr.name = 'TmsTimeoutError'; - throw timeoutErr; } throw err; } finally { @@ -169,18 +218,36 @@ export class TmsClient { } } - private async request(method: string, path: string, body?: unknown): Promise { + /** + * Join the configured server URL with an API path through the URL API, so a + * trailing slash cannot produce "//" and a base path is preserved. + */ + private buildUrl(path: string): string { let parsedUrl: URL; try { parsedUrl = new URL(this.options.serverUrl); } catch { - throw new ConfigError(`Invalid TMS server URL: ${this.options.serverUrl}`); + throw new ConfigError(`Invalid TMS server URL: ${sanitizeUrl(this.options.serverUrl)}`); } + const isLocalhost = parsedUrl.hostname === 'localhost' || parsedUrl.hostname === '127.0.0.1'; if (parsedUrl.protocol !== 'https:' && !isLocalhost) { throw new ConfigError('TMS server URL must use HTTPS'); } - const url = `${this.options.serverUrl}/api/projects/${encodeURIComponent(this.options.projectId)}${path}`; + if (parsedUrl.search || parsedUrl.hash) { + throw new ConfigError( + `TMS server URL must not contain a query string or fragment: ${sanitizeUrl(this.options.serverUrl)}`, + 'Set server: to the bare origin (plus a base path if your TMS is mounted under one).', + ); + } + + const basePath = parsedUrl.pathname.replace(/\/+$/, ''); + const projectPath = `${basePath}/api/projects/${encodeURIComponent(this.options.projectId)}`; + return new URL(`${projectPath}${path}`, parsedUrl).toString(); + } + + private async request(method: string, path: string, body?: unknown): Promise { + const url = this.buildUrl(path); let lastError: unknown; for (let attempt = 0; attempt < this.retry.maxAttempts; attempt++) { @@ -189,8 +256,7 @@ export class TmsClient { response = await this.fetchOnce(url, method, body); } catch (err) { lastError = err; - const isTimeout = err instanceof Error && err.name === 'TmsTimeoutError'; - const retriable = isTimeout || isRetriableNetworkError(err); + const retriable = err instanceof TmsTimeoutError || isRetriableNetworkError(err); if (!retriable || attempt === this.retry.maxAttempts - 1) throw err; const delay = computeBackoffDelay(attempt, this.retry.baseDelayMs, this.retry.maxDelayMs, this.retry.jitter); await sleep(delay); @@ -240,7 +306,7 @@ export class TmsClient { async pullKeys(locale: string): Promise> { const resp = await this.request('GET', `/keys/export?format=json&locale=${encodeURIComponent(locale)}`); - return sanitizePullKeysResponse(await resp.json()); + return sanitizePullKeysResponse(await readJsonBounded(resp)); } async getProjectStatus(): Promise { diff --git a/src/utils/sanitize-url.ts b/src/utils/sanitize-url.ts new file mode 100644 index 0000000..3f43df8 --- /dev/null +++ b/src/utils/sanitize-url.ts @@ -0,0 +1,16 @@ +/** + * Redact userinfo before a URL reaches a log line or an error message. + * Unparseable input is reported as such rather than echoed back. + */ +export function sanitizeUrl(url: string): string { + try { + const parsed = new URL(url); + if (parsed.username || parsed.password) { + parsed.username = '***'; + parsed.password = '***'; + } + return parsed.toString(); + } catch { + return '[invalid URL]'; + } +} diff --git a/tests/unit/sync/tms-client.test.ts b/tests/unit/sync/tms-client.test.ts index c271d01..ef10b97 100644 --- a/tests/unit/sync/tms-client.test.ts +++ b/tests/unit/sync/tms-client.test.ts @@ -1,6 +1,13 @@ -import { TmsClient, resolveTmsCredentials, createTmsClient } from '../../../src/sync/tms-client'; +import { + TmsClient, + TmsTimeoutError, + MAX_PULL_BODY_BYTES, + resolveTmsCredentials, + createTmsClient, +} from '../../../src/sync/tms-client'; import type { SyncTmsConfig } from '../../../src/sync/types'; import { ConfigError, ValidationError } from '../../../src/utils/errors'; +import { ExitCode, exitCodeForError } from '../../../src/utils/exit-codes'; const mockFetch = jest.fn(); (global as unknown as Record)['fetch'] = mockFetch; @@ -113,6 +120,173 @@ describe('TmsClient HTTPS validation', () => { }); }); +describe('TmsClient URL construction', () => { + beforeEach(() => mockFetch.mockReset()); + + function clientFor(serverUrl: string): TmsClient { + return new TmsClient({ serverUrl, projectId: 'proj-1', apiKey: 'key', fetch: mockFetch }); + } + + it('should not double the separator when the server URL has a trailing slash', async () => { + mockFetch.mockResolvedValue({ ok: true, json: async () => ({}) }); + await clientFor('https://tms.example.com/').pushKey('greeting', 'de', 'Hallo'); + + expect(mockFetch).toHaveBeenCalledWith( + 'https://tms.example.com/api/projects/proj-1/keys/greeting', + expect.anything(), + ); + }); + + it('should preserve a base path on the server URL', async () => { + mockFetch.mockResolvedValue({ ok: true, json: async () => ({}) }); + await clientFor('https://tms.example.com/tms/').pushKey('greeting', 'de', 'Hallo'); + + expect(mockFetch).toHaveBeenCalledWith( + 'https://tms.example.com/tms/api/projects/proj-1/keys/greeting', + expect.anything(), + ); + }); + + it('should reject a server URL carrying a query string instead of truncating the path', async () => { + await expect(clientFor('https://tms.example.com/?token=abc').pushKey('k', 'de', 'v')).rejects.toThrow( + ConfigError, + ); + expect(mockFetch).not.toHaveBeenCalled(); + }); + + it('should reject a server URL carrying a fragment', async () => { + await expect(clientFor('https://tms.example.com/#frag').pushKey('k', 'de', 'v')).rejects.toThrow( + ConfigError, + ); + expect(mockFetch).not.toHaveBeenCalled(); + }); + + it('should keep the export query string intact for pullKeys', async () => { + mockFetch.mockResolvedValue({ ok: true, json: async () => ({}) }); + await clientFor('https://tms.example.com/').pullKeys('de'); + + expect(mockFetch).toHaveBeenCalledWith( + 'https://tms.example.com/api/projects/proj-1/keys/export?format=json&locale=de', + expect.anything(), + ); + }); +}); + +describe('TmsClient error message redaction', () => { + beforeEach(() => mockFetch.mockReset()); + + it('should redact credentials embedded in the server URL from timeout messages', async () => { + jest.useFakeTimers(); + try { + const stubFetch = jest.fn().mockImplementation((_url: string, init?: RequestInit) => { + return new Promise((_resolve, reject) => { + init?.signal?.addEventListener('abort', () => { + const err = new Error('aborted'); + err.name = 'AbortError'; + reject(err); + }); + }); + }); + const client = new TmsClient({ + serverUrl: 'https://alice:s3cret@tms.example.com', + projectId: 'p', + apiKey: 'k', + fetch: stubFetch, + timeoutMs: 1000, + retry: { maxAttempts: 1 }, + }); + + const promise = client.pushKey('k', 'de', 'v'); + promise.catch(() => undefined); + await jest.advanceTimersByTimeAsync(1001); + + await expect(promise).rejects.toThrow( + expect.objectContaining({ message: expect.not.stringContaining('s3cret') }), + ); + } finally { + jest.useRealTimers(); + } + }); + + it('should redact credentials from the invalid-server-URL error', async () => { + const client = new TmsClient({ + serverUrl: 'not a url with s3cret inside', + projectId: 'p', + apiKey: 'k', + fetch: mockFetch, + }); + + await expect(client.pushKey('k', 'de', 'v')).rejects.toThrow( + expect.objectContaining({ message: expect.not.stringContaining('s3cret') }), + ); + }); +}); + +describe('TmsTimeoutError classification', () => { + it('should exit with the network error code', () => { + expect(exitCodeForError(new TmsTimeoutError('TMS request timed out'))).toBe(ExitCode.NetworkError); + expect(exitCodeForError(new TmsTimeoutError('TMS request timed out'))).toBe(5); + }); +}); + +describe('TmsClient.pullKeys response size cap', () => { + beforeEach(() => mockFetch.mockReset()); + + const client = new TmsClient({ + serverUrl: 'https://tms.example.com', + projectId: 'proj-1', + apiKey: 'key', + }); + + it('should reject a response whose declared content-length exceeds the cap', async () => { + mockFetch.mockResolvedValue({ + ok: true, + headers: new Headers({ 'content-length': String(MAX_PULL_BODY_BYTES + 1) }), + json: async () => ({}), + }); + + await expect(client.pullKeys('de')).rejects.toThrow(ValidationError); + await expect(client.pullKeys('de')).rejects.toThrow(/size|large|bytes/i); + }); + + it('should reject a streamed response that exceeds the cap without declaring a length', async () => { + const chunk = new Uint8Array(1024 * 1024); + chunk.fill(0x61); + let emitted = 0; + mockFetch.mockResolvedValue({ + ok: true, + headers: new Headers(), + body: new ReadableStream({ + pull(controller) { + if (emitted > MAX_PULL_BODY_BYTES) { + controller.close(); + return; + } + emitted += chunk.byteLength; + controller.enqueue(chunk); + }, + }), + }); + + await expect(client.pullKeys('de')).rejects.toThrow(ValidationError); + }); + + it('should parse a streamed response that stays under the cap', async () => { + mockFetch.mockResolvedValue({ + ok: true, + headers: new Headers(), + body: new ReadableStream({ + start(controller) { + controller.enqueue(new TextEncoder().encode('{"greeting":"Hallo"}')); + controller.close(); + }, + }), + }); + + await expect(client.pullKeys('de')).resolves.toEqual({ greeting: 'Hallo' }); + }); +}); + describe('TmsClient fetch injection', () => { beforeEach(() => mockFetch.mockReset()); From 99ad341cb01c31dae54c70c9e67b089ccd43cd6a Mon Sep 17 00:00:00 2001 From: Steven Syrek Date: Wed, 29 Jul 2026 14:02:25 -0400 Subject: [PATCH 16/27] fix(sync): write .deepl-sync.yaml atomically writeSyncConfig used a plain writeFile, so an interrupted `deepl sync init` could leave a truncated config in place of a working one. Use the same atomic write-then-rename helper as the rest of the sync writers. --- src/sync/sync-init.ts | 3 ++- tests/unit/sync/sync-init.test.ts | 19 +++++++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/src/sync/sync-init.ts b/src/sync/sync-init.ts index 05ed50d..e177776 100644 --- a/src/sync/sync-init.ts +++ b/src/sync/sync-init.ts @@ -4,6 +4,7 @@ import * as YAML from 'yaml'; import fg from 'fast-glob'; import { SYNC_CONFIG_FILENAME } from './sync-config.js'; import { safeReadFileSync } from '../utils/safe-read-file.js'; +import { atomicWriteFile } from '../utils/atomic-write.js'; import { createDefaultRegistry, type FormatRegistry } from '../formats/index.js'; export interface DetectedProject { @@ -345,7 +346,7 @@ export function generateSyncConfig(opts: { export async function writeSyncConfig(rootDir: string, content: string): Promise { const configPath = path.join(rootDir, SYNC_CONFIG_FILENAME); - await fs.promises.writeFile(configPath, content, 'utf-8'); + await atomicWriteFile(configPath, content, 'utf-8'); return configPath; } diff --git a/tests/unit/sync/sync-init.test.ts b/tests/unit/sync/sync-init.test.ts index 82c116f..f84ca88 100644 --- a/tests/unit/sync/sync-init.test.ts +++ b/tests/unit/sync/sync-init.test.ts @@ -669,6 +669,25 @@ describe('sync-init', () => { expect(configPath).toBe(path.join(testDir, '.deepl-sync.yaml')); expect(fs.readFileSync(configPath, 'utf-8')).toBe(content); }); + + it('should leave no temp sibling behind and keep the previous config on write failure', async () => { + const configPath = path.join(testDir, '.deepl-sync.yaml'); + fs.writeFileSync(configPath, 'version: 1\nsource_locale: en\n', 'utf-8'); + + const renameSpy = jest + .spyOn(fs.promises, 'rename') + .mockRejectedValue(new Error('rename failed')); + try { + await expect(writeSyncConfig(testDir, 'version: 1\nsource_locale: de\n')).rejects.toThrow( + 'rename failed', + ); + } finally { + renameSpy.mockRestore(); + } + + expect(fs.readFileSync(configPath, 'utf-8')).toBe('version: 1\nsource_locale: en\n'); + expect(fs.readdirSync(testDir).filter(name => name.includes('.tmp.'))).toEqual([]); + }); }); describe('non-interactive generation', () => { From 6fd0519d714f17b8b72d557fef5eaf02d543fc3f Mon Sep 17 00:00:00 2001 From: Steven Syrek Date: Wed, 29 Jul 2026 14:03:22 -0400 Subject: [PATCH 17/27] fix(voice): reconnect on transport failures and close the input on give-up MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit WebSocket reconnect was unreachable for real transport failures. The client bridged the socket's 'error' event into callbacks.onError, which marked the stream ended and rejected, so the close event that always follows an error returned early and never reconnected — despite --reconnect defaulting on with three attempts. Every socket also carried two 'error' listeners, so the run promise was rejected twice. The socket error is now recorded and handleClose decides whether to reconnect, reporting the recorded transport error when no attempt is left. VoiceClient no longer routes transport errors into the server-message callbacks, leaving exactly one 'error' listener per socket and keeping protocol errors distinguishable from dropped connections. A terminal failure now also wakes the chunk pump and closes the input generator. Previously a failed reconnect left a library consumer's audio generator suspended at a yield forever, holding its fd. Test updates: the two existing "WebSocket emits error" cases now emit the close event that ws always follows an error with, and the voice-client case asserts the socket owner sees transport errors instead of onError. --- src/api/voice-client.ts | 13 +-- src/services/voice-stream-session.ts | 68 ++++++++--- tests/unit/voice-client.test.ts | 16 ++- tests/unit/voice-service.test.ts | 5 + tests/unit/voice-stream-session.test.ts | 149 ++++++++++++++++++++++-- 5 files changed, 213 insertions(+), 38 deletions(-) diff --git a/src/api/voice-client.ts b/src/api/voice-client.ts index 6530b6f..2ca28b5 100644 --- a/src/api/voice-client.ts +++ b/src/api/voice-client.ts @@ -99,15 +99,10 @@ export class VoiceClient extends HttpClient { this.dispatchMessage(message, callbacks); }); - ws.on('error', (error: Error) => { - callbacks.onError?.({ - request_type: 'unknown', - error_code: 0, - reason_code: 0, - error_message: error.message, - }); - }); - + // Transport errors are deliberately not routed into `callbacks.onError`: + // that made a dropped connection indistinguishable from a server error + // message and gave every socket a second 'error' listener. The caller + // owns the socket lifecycle and handles 'error'/'close' itself. return ws; } diff --git a/src/services/voice-stream-session.ts b/src/services/voice-stream-session.ts index 8150c02..abc9aff 100644 --- a/src/services/voice-stream-session.ts +++ b/src/services/voice-stream-session.ts @@ -39,6 +39,8 @@ export class VoiceStreamSession { private streamEnded = false; private ws!: WebSocket; private chunkStreamingResolve: (() => void) | null = null; + private transportError: Error | undefined; + private chunks: AsyncGenerator | undefined; constructor( client: VoiceClient, @@ -71,6 +73,7 @@ export class VoiceStreamSession { } run(chunks: AsyncGenerator): Promise { + this.chunks = chunks; return new Promise((resolve, reject) => { const internalCallbacks = this.createInternalCallbacks(resolve, reject); @@ -84,11 +87,23 @@ export class VoiceStreamSession { this.streamChunks(chunks, reject); }); - this.ws.on('close', () => { this.handleClose(internalCallbacks, reject); }); - this.ws.on('error', (error: Error) => { this.handleError(error, reject); }); + this.attachSocketHandlers(internalCallbacks, reject); }); } + /** + * A socket error is recorded rather than acted on: `ws` always follows an + * error with a close event, and only `handleClose` knows whether a + * reconnect is still available. + */ + private attachSocketHandlers( + internalCallbacks: VoiceStreamCallbacks, + reject: (reason: unknown) => void, + ): void { + this.ws.on('close', () => { this.handleClose(internalCallbacks, reject); }); + this.ws.on('error', (error: Error) => { this.transportError = error; }); + } + private createInternalCallbacks( resolve: (value: VoiceSessionResult) => void, reject: (reason: unknown) => void, @@ -119,6 +134,7 @@ export class VoiceStreamSession { this.streamEnded = true; this.callbacks?.onEndOfStream?.(); this.ws.close(); + this.closeInput(); this.finalizeTranscripts(); resolve({ sessionId: this.session.session_id, @@ -127,10 +143,11 @@ export class VoiceStreamSession { }); }, onError: (error) => { - this.streamEnded = true; this.callbacks?.onError?.(error); - this.ws.close(); - reject(new VoiceError(`Voice streaming error: ${error.error_message} (${error.error_code})`)); + this.fail( + reject, + new VoiceError(`Voice streaming error: ${error.error_message} (${error.error_code})`), + ); }, }; } @@ -148,9 +165,15 @@ export class VoiceStreamSession { this.callbacks?.onReconnecting?.(this.reconnectAttempts); void this.reconnect(internalCallbacks, reject); - } else if (!this.streamEnded) { - reject(new VoiceError('WebSocket closed unexpectedly')); + return; } + + this.fail( + reject, + this.transportError + ? new VoiceError(`WebSocket connection failed: ${this.transportError.message}`) + : new VoiceError('WebSocket closed unexpectedly'), + ); } private async reconnect( @@ -160,6 +183,7 @@ export class VoiceStreamSession { try { const reconnectResponse = await this.client.reconnectSession(this.currentToken); this.currentToken = reconnectResponse.token; + this.transportError = undefined; this.ws = this.client.createWebSocket( reconnectResponse.streaming_url, @@ -167,8 +191,7 @@ export class VoiceStreamSession { internalCallbacks, ); - this.ws.on('close', () => { this.handleClose(internalCallbacks, reject); }); - this.ws.on('error', (error: Error) => { this.handleError(error, reject); }); + this.attachSocketHandlers(internalCallbacks, reject); this.ws.on('open', () => { if (this.chunkStreamingResolve) { @@ -177,12 +200,30 @@ export class VoiceStreamSession { } }); } catch (error) { - reject(error instanceof Error ? error : new VoiceError(String(error))); + this.fail(reject, error instanceof Error ? error : new VoiceError(String(error))); + } + } + + /** + * Settle the session as failed. Waking the chunk pump and closing the + * input generator are both required: without them a library consumer's + * audio source stays suspended at a `yield` forever, holding its fd. + */ + private fail(reject: (reason: unknown) => void, error: Error): void { + this.streamEnded = true; + this.ws.close(); + if (this.chunkStreamingResolve) { + this.chunkStreamingResolve(); + this.chunkStreamingResolve = null; } + this.closeInput(); + reject(error); } - private handleError(error: Error, reject: (reason: unknown) => void): void { - reject(new VoiceError(`WebSocket connection failed: ${error.message}`)); + private closeInput(): void { + const chunks = this.chunks; + this.chunks = undefined; + void chunks?.return(undefined).catch(() => undefined); } private streamChunks( @@ -202,8 +243,7 @@ export class VoiceStreamSession { } this.client.sendEndOfSource(this.ws); } catch (error) { - this.ws.close(); - reject(error instanceof Error ? error : new VoiceError(String(error))); + this.fail(reject, error instanceof Error ? error : new VoiceError(String(error))); } })(); } diff --git a/tests/unit/voice-client.test.ts b/tests/unit/voice-client.test.ts index 302b8e5..b6e470d 100644 --- a/tests/unit/voice-client.test.ts +++ b/tests/unit/voice-client.test.ts @@ -455,18 +455,22 @@ describe('VoiceClient', () => { ); }); - it('should handle WebSocket errors via callback', () => { + // Transport errors belong to whoever owns the socket lifecycle: routing + // them into onError made them indistinguishable from server error + // messages, and left every socket carrying two 'error' listeners. + it('should leave transport errors to the socket owner', () => { const onError = jest.fn(); const ws = client.createWebSocket('wss://voice.deepl.com/ws', 'token', { onError, }); + const transportErrors: Error[] = []; + ws.on('error', (error: Error) => transportErrors.push(error)); ws.emit('error', new Error('Connection failed')); - expect(onError).toHaveBeenCalledWith( - expect.objectContaining({ - error_message: 'Connection failed', - }) - ); + + expect(ws.listenerCount('error')).toBe(1); + expect(transportErrors).toHaveLength(1); + expect(onError).not.toHaveBeenCalled(); }); it('should ignore unparseable messages', () => { diff --git a/tests/unit/voice-service.test.ts b/tests/unit/voice-service.test.ts index 4f4d470..a004ed2 100644 --- a/tests/unit/voice-service.test.ts +++ b/tests/unit/voice-service.test.ts @@ -480,7 +480,10 @@ describe('VoiceService', () => { }); mockClient.createWebSocket.mockImplementation((_url: string, _token: string, _callbacks: any) => { process.nextTick(() => { + // ws always emits close after an error; the session reports the + // transport failure from there. mockWs.emit('error', new Error('Connection refused')); + mockWs.emit('close'); }); return mockWs as any; }); @@ -489,12 +492,14 @@ describe('VoiceService', () => { service.translateFile(testFile, { targetLangs: ['de'], chunkInterval: 0, + reconnect: false, }), ).rejects.toThrow(VoiceError); await expect( service.translateFile(testFile, { targetLangs: ['de'], chunkInterval: 0, + reconnect: false, }), ).rejects.toThrow(/WebSocket connection failed: Connection refused/); }); diff --git a/tests/unit/voice-stream-session.test.ts b/tests/unit/voice-stream-session.test.ts index ceb81e0..cb8e783 100644 --- a/tests/unit/voice-stream-session.test.ts +++ b/tests/unit/voice-stream-session.test.ts @@ -252,25 +252,31 @@ describe('VoiceStreamSession', () => { }); describe('error handling', () => { - it('should reject with VoiceError on WebSocket error', async () => { + // A socket error is always followed by a close event; the transport + // failure is reported from there so reconnect stays reachable. + it('should reject with VoiceError on WebSocket error when reconnect is disabled', async () => { const EventEmitter = require('events'); - const mockWs = new EventEmitter(); - mockWs.readyState = 1; - mockWs.send = jest.fn(); - mockWs.close = jest.fn(); mockClient.createWebSocket.mockImplementation(() => { + const mockWs = new EventEmitter(); + mockWs.readyState = 1; + mockWs.send = jest.fn(); + mockWs.close = jest.fn(); + process.nextTick(() => { mockWs.emit('error', new Error('Connection refused')); + mockWs.readyState = 3; + mockWs.emit('close'); }); return mockWs; }); - const streamSession = new VoiceStreamSession(mockClient, session, options); - - await expect(streamSession.run(emptyChunks())).rejects.toThrow(VoiceError); + const noReconnect = { ...options, reconnect: false }; + await expect( + new VoiceStreamSession(mockClient, session, noReconnect).run(emptyChunks()), + ).rejects.toThrow(VoiceError); await expect( - new VoiceStreamSession(mockClient, session, options).run(emptyChunks()), + new VoiceStreamSession(mockClient, session, noReconnect).run(emptyChunks()), ).rejects.toThrow(/WebSocket connection failed: Connection refused/); }); @@ -319,6 +325,117 @@ describe('VoiceStreamSession', () => { }); describe('reconnection', () => { + it('should reconnect after a transport error closes the socket', async () => { + const EventEmitter = require('events'); + const onReconnecting = jest.fn(); + + mockClient.reconnectSession.mockResolvedValue({ + streaming_url: 'wss://test-new.deepl.com/stream', + token: 'token-2', + }); + + let wsCallCount = 0; + mockClient.createWebSocket.mockImplementation((_url, _token, callbacks) => { + wsCallCount++; + const mockWs = new EventEmitter(); + mockWs.readyState = 1; + mockWs.send = jest.fn(); + mockWs.close = jest.fn(); + + if (wsCallCount === 1) { + process.nextTick(() => { + mockWs.emit('open'); + process.nextTick(() => { + mockWs.emit('error', new Error('read ECONNRESET')); + mockWs.readyState = 3; + mockWs.emit('close'); + }); + }); + } else { + process.nextTick(() => { + mockWs.emit('open'); + process.nextTick(() => callbacks.onEndOfStream?.()); + }); + } + return mockWs; + }); + + const streamSession = new VoiceStreamSession( + mockClient, session, options, { onReconnecting }, + ); + const result = await streamSession.run(emptyChunks()); + + expect(result.sessionId).toBe('session-1'); + expect(mockClient.reconnectSession).toHaveBeenCalledTimes(1); + expect(mockClient.reconnectSession).toHaveBeenCalledWith('token-1'); + expect(onReconnecting).toHaveBeenCalledWith(1); + }); + + it('should reject with the transport error once reconnect attempts are exhausted', async () => { + const EventEmitter = require('events'); + + mockClient.reconnectSession.mockResolvedValue({ + streaming_url: 'wss://test-new.deepl.com/stream', + token: 'token-2', + }); + + mockClient.createWebSocket.mockImplementation(() => { + const mockWs = new EventEmitter(); + mockWs.readyState = 1; + mockWs.send = jest.fn(); + mockWs.close = jest.fn(); + + process.nextTick(() => { + mockWs.emit('open'); + process.nextTick(() => { + mockWs.emit('error', new Error('read ECONNRESET')); + mockWs.readyState = 3; + mockWs.emit('close'); + }); + }); + return mockWs; + }); + + await expect( + new VoiceStreamSession(mockClient, session, { + ...options, + maxReconnectAttempts: 1, + }).run(emptyChunks()), + ).rejects.toThrow(/WebSocket connection failed: read ECONNRESET/); + expect(mockClient.reconnectSession).toHaveBeenCalledTimes(1); + }); + + it('should close the audio input when reconnect fails', async () => { + const EventEmitter = require('events'); + const input = trackedChunks(); + + mockClient.reconnectSession.mockRejectedValue( + new VoiceError('Voice API access denied.'), + ); + + mockClient.createWebSocket.mockImplementation(() => { + const mockWs = new EventEmitter(); + // Never open for writing, so the chunk pump parks waiting for a + // usable socket — the state that used to leak the generator. + mockWs.readyState = 3; + mockWs.send = jest.fn(); + mockWs.close = jest.fn(); + + process.nextTick(() => { + mockWs.emit('open'); + process.nextTick(() => { + process.nextTick(() => mockWs.emit('close')); + }); + }); + return mockWs; + }); + + const streamSession = new VoiceStreamSession(mockClient, session, options); + await expect(streamSession.run(input.chunks)).rejects.toThrow(VoiceError); + + expect(input.closed()).toBe(true); + }); + it('should reconnect on unexpected WebSocket close', async () => { const EventEmitter = require('events'); const onReconnecting = jest.fn(); @@ -659,3 +776,17 @@ async function* singleChunk(data: Buffer): AsyncGenerator { async function* throwingChunks(): AsyncGenerator { throw new Error('chunk error'); } + +/** A chunk source that reports whether the session closed it. */ +function trackedChunks(): { chunks: AsyncGenerator; closed: () => boolean } { + let closed = false; + async function* generate(): AsyncGenerator { + try { + yield Buffer.from('audio-1'); + yield Buffer.from('audio-2'); + } finally { + closed = true; + } + } + return { chunks: generate(), closed: () => closed }; +} From b8b67abbacfe7da80546f4e3ae74f10c0c7a96ed Mon Sep 17 00:00:00 2001 From: Steven Syrek Date: Wed, 29 Jul 2026 14:03:22 -0400 Subject: [PATCH 18/27] fix(formats): scan Android XML and XLIFF elements in linear time MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both parsers matched elements with `([\s\S]*?)`, so every opening tag with no matching close scanned to end of input — quadratic in file size, and reachable with a file well under the sync size cap. A 4 MiB input took minutes; Android reconstruct additionally rescanned the whole file once per dotted key to classify string-array members. Replace the lazy patterns with a single forward scan: sticky opening-tag patterns that cannot match past the following tag, and one cursor for the closing-tag search. A missing closing tag now ends the scan instead of being retried per opening tag. The scan also skips CDATA sections when looking for a closing tag, so markup quoted inside one no longer truncates the element, and a self-closing element is left alone rather than swallowing its successor. --- src/formats/android-xml.ts | 198 ++++++++---------- src/formats/xliff.ts | 267 +++++++++++-------------- src/formats/xml-scan.ts | 172 ++++++++++++++++ tests/unit/formats/android-xml.test.ts | 75 +++++++ tests/unit/formats/xliff.test.ts | 82 ++++++++ tests/unit/formats/xml-scan.test.ts | 89 +++++++++ 6 files changed, 620 insertions(+), 263 deletions(-) create mode 100644 src/formats/xml-scan.ts create mode 100644 tests/unit/formats/xml-scan.test.ts diff --git a/src/formats/android-xml.ts b/src/formats/android-xml.ts index 12bcb19..d59b431 100644 --- a/src/formats/android-xml.ts +++ b/src/formats/android-xml.ts @@ -1,26 +1,45 @@ import type { ExtractedEntry, FormatParser, TranslatedEntry } from './format.js'; import { ValidationError } from '../utils/errors.js'; +import { + replaceElements, + scanElements, + type ElementPattern, + type ScannedElement, +} from './xml-scan.js'; interface PluralItem { quantity: string; value: string; } -const STRING_RE = - /([\s\S]*?)<\/string>/g; +const ATTRS = String.raw`((?:\s+[a-zA-Z_:][a-zA-Z0-9_:.-]*=(?:"[^"<]*"|'[^'<]*'))*)`; -const TRANSLATABLE_FALSE_RE = /\btranslatable\s*=\s*"false"/; +const STRING_EL: ElementPattern = { + open: new RegExp(String.raw``, 'y'), + close: /<\/string>/y, +}; + +const PLURALS_EL: ElementPattern = { + open: new RegExp(String.raw``, 'y'), + close: /<\/plurals>/y, +}; -const PLURALS_RE = - /]*)>([\s\S]*?)<\/plurals>/g; +const PLURAL_ITEM_EL: ElementPattern = { + open: new RegExp(String.raw``, 'y'), + close: /<\/item>/y, +}; -const PLURAL_ITEM_RE = - /]*>([\s\S]*?)<\/item>/g; +const STRING_ARRAY_EL: ElementPattern = { + open: new RegExp(String.raw``, 'y'), + close: /<\/string-array>/y, +}; -const STRING_ARRAY_RE = - /]*)>([\s\S]*?)<\/string-array>/g; +const ARRAY_ITEM_EL: ElementPattern = { + open: //y, + close: /<\/item>/y, +}; -const ARRAY_ITEM_RE = /([\s\S]*?)<\/item>/g; +const TRANSLATABLE_FALSE_RE = /\btranslatable\s*=\s*"false"/; const XML_ENTITY_RE = /&(?:#x([0-9a-fA-F]+)|#(\d+)|(amp|lt|gt|quot|apos));/g; @@ -112,6 +131,7 @@ export class AndroidXmlFormatParser implements FormatParser { const translations = new Map(); const pluralTranslations = new Map>(); const arrayTranslations = new Map>(); + let arrayNames: Set | undefined; for (const entry of entries) { if (entry.metadata?.['plurals']) { @@ -124,10 +144,12 @@ export class AndroidXmlFormatParser implements FormatParser { } else if (entry.key.includes('.')) { const lastDot = entry.key.lastIndexOf('.'); const arrayName = entry.key.substring(0, lastDot); - const indexStr = entry.key.substring(lastDot + 1); - const index = parseInt(indexStr, 10); + const index = parseInt(entry.key.substring(lastDot + 1), 10); + arrayNames ??= new Set( + scanElements(originalContent, STRING_ARRAY_EL).map((el) => el.groups[0]!), + ); - if (!isNaN(index) && this.isStringArrayKey(originalContent, arrayName)) { + if (!isNaN(index) && arrayNames.has(arrayName)) { if (!arrayTranslations.has(arrayName)) { arrayTranslations.set(arrayName, new Map()); } @@ -140,115 +162,76 @@ export class AndroidXmlFormatParser implements FormatParser { } } - let result = originalContent; - - result = result.replace(STRING_RE, (match, name: string, attrs: string, innerText: string) => { + let result = replaceElements(originalContent, STRING_EL, (el) => { + const attrs = el.groups[1] ?? ''; if (TRANSLATABLE_FALSE_RE.test(attrs)) { - return match; + return el.text; } - const translation = translations.get(name); - if (translation !== undefined) { - const escapedTranslation = this.escapeForReconstruct(innerText, translation); - return `${escapedTranslation}`; + const translation = translations.get(el.groups[0]!); + if (translation === undefined) { + return null; } - return match; + return this.rewriteInner(el, this.escapeForReconstruct(el.inner, translation)); }); - result = result.replace(PLURALS_RE, (match, name: string, extraAttrs: string, innerContent: string) => { - const quantityMap = pluralTranslations.get(name); + result = replaceElements(result, PLURALS_EL, (el) => { + const quantityMap = pluralTranslations.get(el.groups[0]!); if (!quantityMap) { - return match; + return null; } - const updatedInner = innerContent.replace( - PLURAL_ITEM_RE, - (itemMatch, quantity: string, value: string) => { - const translation = quantityMap.get(quantity); - if (translation !== undefined) { - return `${this.escapeForReconstruct(value, translation)}`; - } - return itemMatch; - }, - ); - return `${updatedInner}`; + const inner = replaceElements(el.inner, PLURAL_ITEM_EL, (item) => { + const translation = quantityMap.get(item.groups[0]!); + if (translation === undefined) { + return item.text; + } + return this.rewriteInner(item, this.escapeForReconstruct(item.inner, translation)); + }); + return this.rewriteInner(el, inner); }); - result = result.replace(STRING_ARRAY_RE, (match, name: string, extraAttrs: string, innerContent: string) => { - const indexMap = arrayTranslations.get(name); + result = replaceElements(result, STRING_ARRAY_EL, (el) => { + const indexMap = arrayTranslations.get(el.groups[0]!); if (!indexMap) { - return match; + return null; } - let idx = 0; - const updatedInner = innerContent.replace( - ARRAY_ITEM_RE, - (itemMatch, value: string) => { - const translation = indexMap.get(idx); - idx++; - if (translation !== undefined) { - return `${this.escapeForReconstruct(value, translation)}`; - } - return itemMatch; - }, - ); - return `${updatedInner}`; + let index = 0; + const inner = replaceElements(el.inner, ARRAY_ITEM_EL, (item) => { + const translation = indexMap.get(index); + index++; + if (translation === undefined) { + return item.text; + } + return this.rewriteInner(item, this.escapeForReconstruct(item.inner, translation)); + }); + return this.rewriteInner(el, inner); }); - const stringKeys = new Set([...translations.keys()]); - const pluralKeys = new Set([...pluralTranslations.keys()]); - const arrayKeys = new Set([...arrayTranslations.keys()]); - - result = result.replace( - /[ \t]*]*>[\s\S]*?<\/string>\s*\n?/g, - (match, name: string) => stringKeys.has(name) || TRANSLATABLE_FALSE_RE.test(match) ? match : '', - ); - result = result.replace( - /[ \t]*]*>[\s\S]*?<\/plurals>\s*\n?/g, - (match, name: string) => pluralKeys.has(name) ? match : '', - ); - result = result.replace( - /[ \t]*]*>[\s\S]*?<\/string-array>\s*\n?/g, - (match, name: string) => arrayKeys.has(name) ? match : '', - ); - return result; } - private extractStrings(content: string, entries: ExtractedEntry[]): void { - const regex = new RegExp(STRING_RE.source, STRING_RE.flags); - let match: RegExpExecArray | null; - while ((match = regex.exec(content)) !== null) { - const name = match[1]!; - const attrs = match[2] ?? ''; - const rawValue = match[3]!; + private rewriteInner(element: ScannedElement, inner: string): string { + return `${element.openTag}${inner}${element.closeTag}`; + } - if (TRANSLATABLE_FALSE_RE.test(attrs)) { + private extractStrings(content: string, entries: ExtractedEntry[]): void { + for (const el of scanElements(content, STRING_EL)) { + if (TRANSLATABLE_FALSE_RE.test(el.groups[1] ?? '')) { continue; } - - const value = this.decodeValue(rawValue); - entries.push({ key: name, value }); + entries.push({ key: el.groups[0]!, value: this.decodeValue(el.inner) }); } } private extractPlurals(content: string, entries: ExtractedEntry[]): void { - const pluralsRegex = new RegExp(PLURALS_RE.source, PLURALS_RE.flags); - let match: RegExpExecArray | null; - while ((match = pluralsRegex.exec(content)) !== null) { - const name = match[1]!; - const innerContent = match[3]!; - - const plurals: PluralItem[] = []; - const itemRegex = new RegExp(PLURAL_ITEM_RE.source, PLURAL_ITEM_RE.flags); - let itemMatch: RegExpExecArray | null; - while ((itemMatch = itemRegex.exec(innerContent)) !== null) { - plurals.push({ - quantity: itemMatch[1]!, - value: this.decodeValue(itemMatch[2]!), - }); - } + for (const el of scanElements(content, PLURALS_EL)) { + const plurals: PluralItem[] = scanElements(el.inner, PLURAL_ITEM_EL).map((item) => ({ + quantity: item.groups[0]!, + value: this.decodeValue(item.inner), + })); const defaultItem = plurals.find(p => p.quantity === 'other') ?? plurals[0]; entries.push({ - key: name, + key: el.groups[0]!, value: defaultItem?.value ?? '', metadata: { plurals }, }); @@ -256,20 +239,11 @@ export class AndroidXmlFormatParser implements FormatParser { } private extractStringArrays(content: string, entries: ExtractedEntry[]): void { - const arrayRegex = new RegExp(STRING_ARRAY_RE.source, STRING_ARRAY_RE.flags); - let match: RegExpExecArray | null; - while ((match = arrayRegex.exec(content)) !== null) { - const name = match[1]!; - const innerContent = match[3]!; - - const itemRegex = new RegExp(ARRAY_ITEM_RE.source, ARRAY_ITEM_RE.flags); - let itemMatch: RegExpExecArray | null; + for (const el of scanElements(content, STRING_ARRAY_EL)) { + const name = el.groups[0]!; let index = 0; - while ((itemMatch = itemRegex.exec(innerContent)) !== null) { - entries.push({ - key: `${name}.${index}`, - value: this.decodeValue(itemMatch[1]!), - }); + for (const item of scanElements(el.inner, ARRAY_ITEM_EL)) { + entries.push({ key: `${name}.${index}`, value: this.decodeValue(item.inner) }); index++; } } @@ -299,10 +273,4 @@ export class AndroidXmlFormatParser implements FormatParser { } return escapeAndroid(translation); } - - private isStringArrayKey(content: string, name: string): boolean { - const escaped = name.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); - const regex = new RegExp(`]*version=["'](\d+\.\d+)["']/i; -const TRANS_UNIT_RE = - /<(?:\w+:)?trans-unit\s+id=["']([^"']+)["'][^>]*>([\s\S]*?)<\/(?:\w+:)?trans-unit>/gi; +const TRANS_UNIT_EL: ElementPattern = { + open: /<(?:\w+:)?trans-unit\s+id=["']([^"'<]+)["'][^><]*>/iy, + close: /<\/(?:\w+:)?trans-unit>/iy, +}; -const UNIT_RE = - /<(?:\w+:)?unit\s+id=["']([^"']+)["'][^>]*>([\s\S]*?)<\/(?:\w+:)?unit>/gi; +const UNIT_EL: ElementPattern = { + open: /<(?:\w+:)?unit\s+id=["']([^"'<]+)["'][^><]*>/iy, + close: /<\/(?:\w+:)?unit>/iy, +}; + +const SOURCE_EL: ElementPattern = { + open: /<(\w+:)?source>/iy, + close: /<\/(?:\w+:)?source>/iy, +}; -const SOURCE_RE = /<(?:\w+:)?source>([\s\S]*?)<\/(?:\w+:)?source>/i; // Attributes are optional but must be preserved: `state` is a standard XLIFF // attribute that every CAT tool writes, so requiring a bare tag would make -// those elements invisible to this regex. -const TARGET_RE = /<(\w+:)?target((?:\s[^>]*)?)>([\s\S]*?)<\/(?:\w+:)?target>/i; -const NOTE_RE = /<(?:\w+:)?note(?:\s[^>]*)?>([\s\S]*?)<\/(?:\w+:)?note>/i; -const SEGMENT_RE = /<(?:\w+:)?segment(?:\s[^>]*)?>([\s\S]*?)<\/(?:\w+:)?segment>/i; +// those elements invisible to this scan. +const TARGET_EL: ElementPattern = { + open: /<(\w+:)?target((?:\s[^><]*)?)>/iy, + close: /<\/(?:\w+:)?target>/iy, +}; + +const NOTE_EL: ElementPattern = { + open: /<(?:\w+:)?note(?:\s[^><]*)?>/iy, + close: /<\/(?:\w+:)?note>/iy, +}; + +const SEGMENT_EL: ElementPattern = { + open: /<(?:\w+:)?segment(?:\s[^><]*)?>/iy, + close: /<\/(?:\w+:)?segment>/iy, +}; + +const TRANSLATABLE_EL: ElementPattern = { + open: /<(?:\w+:)?(?:source|target)(?:\s[^><]*)?>/iy, + close: /<\/(?:\w+:)?(?:source|target)>/iy, +}; const NAMED_ENTITIES: Record = { amp: '&', @@ -50,9 +81,6 @@ function unescapeXml(value: string): string { }); } -const CDATA_IN_TRANSLATABLE_RE = - /<(?:\w+:)?(?:source|target)[^>]*>[\s\S]*?/i; - /** * Refuse XLIFF input that contains a CDATA section inside a `` or * `` element. The regex-based extract/reconstruct pair cannot @@ -64,11 +92,13 @@ const CDATA_IN_TRANSLATABLE_RE = */ function assertNoCdataInTranslatable(content: string): void { if (!content.includes(' / elements containing CDATA sections are not supported.', - 'Inline the literal text without the wrapper, or preprocess the file to entity-escape CDATA content before syncing.', - ); + for (const element of scanElements(content, TRANSLATABLE_EL)) { + if (element.inner.includes(' / elements containing CDATA sections are not supported.', + 'Inline the literal text without the wrapper, or preprocess the file to entity-escape CDATA content before syncing.', + ); + } } } @@ -84,6 +114,36 @@ function detectVersion(content: string): string { return match?.[1] ?? '1.2'; } +function rewriteInner(element: ScannedElement, inner: string): string { + return `${element.openTag}${inner}${element.closeTag}`; +} + +/** + * Replace the first `` in a block, or insert one after `` + * when the block has none. + */ +function applyTarget(block: string, escaped: string): string { + const target = findElement(block, TARGET_EL); + if (target) { + const ns = target.groups[0] ?? ''; + const attrs = target.groups[1] ?? ''; + return ( + block.slice(0, target.start) + + `<${ns}target${attrs}>${escaped}` + + block.slice(target.end) + ); + } + + const source = findElement(block, SOURCE_EL); + if (!source) return block; + const ns = source.groups[0] ?? ''; + return ( + block.slice(0, source.end) + + `\n <${ns}target>${escaped}` + + block.slice(source.end) + ); +} + export class XliffFormatParser implements FormatParser { readonly name = 'XLIFF'; readonly configKey = 'xliff'; @@ -118,156 +178,67 @@ export class XliffFormatParser implements FormatParser { } extractContext(content: string, key: string): string | undefined { - const version = detectVersion(content); + const unit = detectVersion(content) === '2.0' ? UNIT_EL : TRANS_UNIT_EL; - if (version === '2.0') { - return this.extractContextV2(content, key); + for (const element of scanElements(content, unit)) { + if (element.groups[0] !== key) continue; + const note = findElement(element.inner, NOTE_EL); + return note ? unescapeXml(note.inner) : undefined; } - return this.extractContextV12(content, key); + return undefined; } private extractV12(content: string, entries: ExtractedEntry[]): void { - const regex = new RegExp(TRANS_UNIT_RE.source, TRANS_UNIT_RE.flags); - let match: RegExpExecArray | null; - while ((match = regex.exec(content)) !== null) { - const id = match[1]!; - const block = match[2]!; - - const sourceMatch = SOURCE_RE.exec(block); - if (!sourceMatch) continue; - - const value = unescapeXml(sourceMatch[1]!); - const noteMatch = NOTE_RE.exec(block); - const context = noteMatch ? unescapeXml(noteMatch[1]!) : undefined; - - const entry: ExtractedEntry = { key: id, value }; - if (context !== undefined) { - entry.context = context; - } - entries.push(entry); + for (const element of scanElements(content, TRANS_UNIT_EL)) { + const source = findElement(element.inner, SOURCE_EL); + if (!source) continue; + entries.push(this.toEntry(element.groups[0]!, source.inner, element.inner)); } } private extractV2(content: string, entries: ExtractedEntry[]): void { - const regex = new RegExp(UNIT_RE.source, UNIT_RE.flags); - let match: RegExpExecArray | null; - while ((match = regex.exec(content)) !== null) { - const id = match[1]!; - const block = match[2]!; - - const segmentMatch = SEGMENT_RE.exec(block); - if (!segmentMatch) continue; - - const segment = segmentMatch[1]!; - const sourceMatch = SOURCE_RE.exec(segment); - if (!sourceMatch) continue; - - const value = unescapeXml(sourceMatch[1]!); - const noteMatch = NOTE_RE.exec(block); - const context = noteMatch ? unescapeXml(noteMatch[1]!) : undefined; - - const entry: ExtractedEntry = { key: id, value }; - if (context !== undefined) { - entry.context = context; - } - entries.push(entry); + for (const element of scanElements(content, UNIT_EL)) { + const segment = findElement(element.inner, SEGMENT_EL); + if (!segment) continue; + + const source = findElement(segment.inner, SOURCE_EL); + if (!source) continue; + entries.push(this.toEntry(element.groups[0]!, source.inner, element.inner)); } } - private reconstructV12( - content: string, - translations: Map, - ): string { - const regex = new RegExp(TRANS_UNIT_RE.source, TRANS_UNIT_RE.flags); - let result = content.replace(regex, (fullMatch, id: string, block: string) => { - const translation = translations.get(id); - if (translation === undefined) return ''; - - const escaped = escapeXml(translation); - const targetMatch = TARGET_RE.exec(block); - - let newBlock: string; - if (targetMatch) { - const ns = targetMatch[1] ?? ''; - const attrs = targetMatch[2] ?? ''; - newBlock = block.replace(TARGET_RE, () => `<${ns}target${attrs}>${escaped}`); - } else { - const sourceNsMatch = /<(\w+:)?source>/i.exec(block); - const ns = sourceNsMatch?.[1] ?? ''; - newBlock = block.replace( - /(<\/(?:\w+:)?source>)/i, - (_match, src: string) => `${src}\n <${ns}target>${escaped}`, - ); - } - - return fullMatch.replace(block, () => newBlock); - }); - result = result.replace(/\n{3,}/g, '\n\n'); - return result; + private toEntry(id: string, rawSource: string, block: string): ExtractedEntry { + const entry: ExtractedEntry = { key: id, value: unescapeXml(rawSource) }; + const note = findElement(block, NOTE_EL); + if (note) { + entry.context = unescapeXml(note.inner); + } + return entry; } - private reconstructV2( - content: string, - translations: Map, - ): string { - const regex = new RegExp(UNIT_RE.source, UNIT_RE.flags); - let result = content.replace(regex, (fullMatch, id: string, block: string) => { - const translation = translations.get(id); + private reconstructV12(content: string, translations: Map): string { + const result = replaceElements(content, TRANS_UNIT_EL, (element) => { + const translation = translations.get(element.groups[0]!); if (translation === undefined) return ''; - - const escaped = escapeXml(translation); - const segmentMatch = SEGMENT_RE.exec(block); - if (!segmentMatch) return fullMatch; - - const segment = segmentMatch[1]!; - const targetMatch = TARGET_RE.exec(segment); - - let newSegment: string; - if (targetMatch) { - const ns = targetMatch[1] ?? ''; - const attrs = targetMatch[2] ?? ''; - newSegment = segment.replace(TARGET_RE, () => `<${ns}target${attrs}>${escaped}`); - } else { - const sourceNsMatch = /<(\w+:)?source>/i.exec(segment); - const ns = sourceNsMatch?.[1] ?? ''; - newSegment = segment.replace( - /(<\/(?:\w+:)?source>)/i, - (_match, src: string) => `${src}\n <${ns}target>${escaped}`, - ); - } - - const newBlock = block.replace(segment, () => newSegment); - return fullMatch.replace(block, () => newBlock); + return rewriteInner(element, applyTarget(element.inner, escapeXml(translation))); }); - result = result.replace(/\n{3,}/g, '\n\n'); - return result; + return result.replace(/\n{3,}/g, '\n\n'); } - private extractContextV12(content: string, key: string): string | undefined { - const escaped = key.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); - const regex = new RegExp( - `<(?:\\w+:)?trans-unit\\s+id=["']${escaped}["'][^>]*>([\\s\\S]*?)<\\/(?:\\w+:)?trans-unit>`, - 'i', - ); - const match = regex.exec(content); - if (!match) return undefined; - - const block = match[1]!; - const noteMatch = NOTE_RE.exec(block); - return noteMatch ? unescapeXml(noteMatch[1]!) : undefined; - } + private reconstructV2(content: string, translations: Map): string { + const result = replaceElements(content, UNIT_EL, (element) => { + const translation = translations.get(element.groups[0]!); + if (translation === undefined) return ''; - private extractContextV2(content: string, key: string): string | undefined { - const escaped = key.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); - const regex = new RegExp( - `<(?:\\w+:)?unit\\s+id=["']${escaped}["'][^>]*>([\\s\\S]*?)<\\/(?:\\w+:)?unit>`, - 'i', - ); - const match = regex.exec(content); - if (!match) return undefined; + const segment = findElement(element.inner, SEGMENT_EL); + if (!segment) return element.text; - const block = match[1]!; - const noteMatch = NOTE_RE.exec(block); - return noteMatch ? unescapeXml(noteMatch[1]!) : undefined; + const inner = + element.inner.slice(0, segment.start) + + rewriteInner(segment, applyTarget(segment.inner, escapeXml(translation))) + + element.inner.slice(segment.end); + return rewriteInner(element, inner); + }); + return result.replace(/\n{3,}/g, '\n\n'); } } diff --git a/src/formats/xml-scan.ts b/src/formats/xml-scan.ts new file mode 100644 index 0000000..b5cc881 --- /dev/null +++ b/src/formats/xml-scan.ts @@ -0,0 +1,172 @@ +/** + * Bounded element scanning for the XML-shaped format parsers. + * + * Matching an element as one regex (`([\s\S]*?)`) costs a scan + * of the remaining input for every opening tag that never closes, which is + * quadratic in file size. The scanners here walk the input once: opening tags + * are matched sticky (anchored, so no scan), and the search for a closing tag + * moves a single cursor forward. Once no closing tag remains, no later opening + * tag can have one either, so scanning stops. + * + * Every quantifier in a pattern passed here must exclude `<`, so a match + * attempt cannot run past the following tag. XML forbids `<` in attribute + * values, so this only rejects input that is already malformed. + */ + +const CDATA_OPEN = ''; +const WHITESPACE_RE = /\s/; + +export interface ElementPattern { + /** Sticky pattern for an opening tag: `<` followed by an element name. */ + open: RegExp; + /** Sticky pattern for a closing tag: ` string | null, +): string { + const elements = scanElements(content, pattern); + if (elements.length === 0) return content; + + const parts: string[] = []; + let copied = 0; + + for (const element of elements) { + const replacement = replace(element); + if (replacement === null) { + let from = element.start; + while (from > copied && (content[from - 1] === ' ' || content[from - 1] === '\t')) from--; + let to = element.end; + while (to < content.length && WHITESPACE_RE.test(content[to]!)) to++; + parts.push(content.slice(copied, from)); + copied = to; + } else { + parts.push(content.slice(copied, element.start), replacement); + copied = element.end; + } + } + + parts.push(content.slice(copied)); + return parts.join(''); +} diff --git a/tests/unit/formats/android-xml.test.ts b/tests/unit/formats/android-xml.test.ts index 538db4b..e865eb1 100644 --- a/tests/unit/formats/android-xml.test.ts +++ b/tests/unit/formats/android-xml.test.ts @@ -273,6 +273,22 @@ describe('android-xml parser', () => { expect(result).not.toContain('name="sizes"'); }); + it('should keep a self-closing string element untouched', () => { + const xml = ` + + + Hello +`; + const entries = parser.extract(xml); + expect(entries.map((e) => e.key)).toEqual(['greeting']); + + const result = parser.reconstruct(xml, [ + { key: 'greeting', value: 'Hello', translation: 'Hallo' }, + ]); + expect(result).toContain(''); + expect(result).toContain('Hallo'); + }); + it('should preserve translatable="false" strings even when not in entries', () => { const xml = ` @@ -287,4 +303,63 @@ describe('android-xml parser', () => { expect(result).toContain('name="greeting"'); }); }); + + describe('elements with no closing tag', () => { + const unclosed = ` + + Hello + Never closed +`; + + it('should still extract the well-formed entries before the dangling tag', () => { + expect(parser.extract(unclosed).map((e) => e.key)).toEqual(['greeting']); + }); + + it('should leave the dangling element untouched on reconstruct', () => { + const result = parser.reconstruct(unclosed, [ + { key: 'greeting', value: 'Hello', translation: 'Hallo' }, + ]); + expect(result).toContain('Hallo'); + expect(result).toContain('Never closed'); + }); + + it('should not let a CDATA body swallow the closing tag', () => { + const xml = ` + + y here]]> + Plain +`; + expect(parser.extract(xml)).toEqual([ + { key: 'snippet', value: 'use y here' }, + { key: 'plain', value: 'Plain' }, + ]); + }); + }); + + describe('adversarial input scaling', () => { + // A file of opening tags that never close: the parser must not rescan the + // remainder of the input once per opening tag. + function unclosedOpeners(bytes: number): string { + const head = '\n\n'; + const opener = + ' v\n \n \n'; + return head + opener.repeat(Math.ceil((bytes - head.length) / opener.length)); + } + + const content = unclosedOpeners(4 * 1024 * 1024); + + it('should extract a 4 MiB file of unclosed openers in linear time', () => { + const start = process.hrtime.bigint(); + parser.extract(content); + const ms = Number(process.hrtime.bigint() - start) / 1e6; + expect(ms).toBeLessThan(2000); + }); + + it('should reconstruct a 4 MiB file of unclosed openers in linear time', () => { + const start = process.hrtime.bigint(); + parser.reconstruct(content, [{ key: 'k', value: 'v', translation: 'w' }]); + const ms = Number(process.hrtime.bigint() - start) / 1e6; + expect(ms).toBeLessThan(2000); + }); + }); }); diff --git a/tests/unit/formats/xliff.test.ts b/tests/unit/formats/xliff.test.ts index ec76eb0..1f334b7 100644 --- a/tests/unit/formats/xliff.test.ts +++ b/tests/unit/formats/xliff.test.ts @@ -381,5 +381,87 @@ describe('xliff parser', () => { `; expect(() => parser.extract(xliff)).not.toThrow(); }); + + it('accepts a CDATA that sits between and ', () => { + const xliff = ` + +Hello]]>Hallo +`; + expect(() => parser.extract(xliff)).not.toThrow(); + expect(parser.extract(xliff).map((e) => e.key)).toEqual(['a']); + }); + }); + + describe('elements with no closing tag', () => { + const unclosed = ` + + + + + Hello + Hallo + + + Never closed + + +`; + + it('should still extract the well-formed units before the dangling tag', () => { + expect(parser.extract(unclosed).map((e) => e.key)).toEqual(['greeting']); + }); + + it('should leave the dangling unit untouched on reconstruct', () => { + const result = parser.reconstruct(unclosed, [ + { key: 'greeting', value: 'Hello', translation: 'Guten Tag' }, + ]); + expect(result).toContain('Guten Tag'); + expect(result).toContain(''); + }); + }); + + describe('adversarial input scaling', () => { + // A file of opening tags that never close: the parser must not rescan the + // remainder of the input once per opening tag. + function unclosedOpeners(bytes: number, version: '1.2' | '2.0'): string { + const head = `\n\n\n`; + const opener = + version === '2.0' + ? ' v\n' + : ' v\n'; + return head + opener.repeat(Math.ceil((bytes - head.length) / opener.length)); + } + + const v12 = unclosedOpeners(4 * 1024 * 1024, '1.2'); + const v20 = unclosedOpeners(4 * 1024 * 1024, '2.0'); + + it('should extract a 4 MiB v1.2 file of unclosed openers in linear time', () => { + const start = process.hrtime.bigint(); + parser.extract(v12); + const ms = Number(process.hrtime.bigint() - start) / 1e6; + expect(ms).toBeLessThan(2000); + }); + + it('should reconstruct a 4 MiB v1.2 file of unclosed openers in linear time', () => { + const start = process.hrtime.bigint(); + parser.reconstruct(v12, [{ key: 'k', value: 'v', translation: 'w' }]); + const ms = Number(process.hrtime.bigint() - start) / 1e6; + expect(ms).toBeLessThan(2000); + }); + + it('should extract a 4 MiB v2.0 file of unclosed openers in linear time', () => { + const start = process.hrtime.bigint(); + parser.extract(v20); + const ms = Number(process.hrtime.bigint() - start) / 1e6; + expect(ms).toBeLessThan(2000); + }); + + it('should scan a 4 MiB file for CDATA in translatable content in linear time', () => { + const withTrailingCdata = v12 + '\n'; + const start = process.hrtime.bigint(); + parser.extract(withTrailingCdata); + const ms = Number(process.hrtime.bigint() - start) / 1e6; + expect(ms).toBeLessThan(2000); + }); }); }); diff --git a/tests/unit/formats/xml-scan.test.ts b/tests/unit/formats/xml-scan.test.ts new file mode 100644 index 0000000..086df49 --- /dev/null +++ b/tests/unit/formats/xml-scan.test.ts @@ -0,0 +1,89 @@ +import { + findElement, + replaceElements, + scanElements, + type ElementPattern, +} from '../../../src/formats/xml-scan'; + +const TAG: ElementPattern = { + open: //y, + close: /<\/tag>/y, +}; + +describe('xml-scan', () => { + describe('scanElements', () => { + it('should return elements in document order with their parts split out', () => { + const elements = scanElements('aonebtwo', TAG); + + expect(elements.map((e) => [e.groups[0], e.inner, e.openTag, e.closeTag])).toEqual([ + ['x', 'one', '', ''], + [undefined, 'two', '', ''], + ]); + expect(elements[0]!.text).toBe('one'); + }); + + it('should stop at an opening tag that never closes', () => { + const elements = scanElements('onetwothree', TAG); + + expect(elements.map((e) => e.inner)).toEqual(['one']); + }); + + it('should skip a closing tag quoted inside a CDATA section', () => { + const elements = scanElements(']]>rest', TAG); + + expect(elements.map((e) => e.inner)).toEqual([']]>rest']); + }); + + it('should stop at an unterminated CDATA section', () => { + expect(scanElements('', TAG)).toEqual([]); + }); + + it('should ignore a self-closing tag', () => { + const elements = scanElements('one', TAG); + + expect(elements.map((e) => e.inner)).toEqual(['one']); + }); + + it('should not let an unterminated attribute value match across the next tag', () => { + const elements = scanElements('