Add fast-check property suite; fix two escaping bugs it found

CHANGELOG.md

@@ -21,6 +21,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Changed
 
+- **tests**: A fast-check property suite (`tests/property/`) now enforces round-trip laws across all 11 format parsers — translated values survive reconstruct/extract intact, re-applying the same translations never changes the file, and an identity sync is a fixed point — plus preservation laws for the placeholder and ICU utilities. Runs are seeded-random with 200 cases per law by default (`FC_NUM_RUNS` overrides; `FC_SEED`/`FC_PATH` replay a recorded counterexample). The suite found the U+2028 TOML corruption and the `.properties` leading-space loss fixed in this release; example tests document decisions, properties enforce laws.
 - **tests**: Tests that assert only inside a `catch` block now declare their assertion count, so they fail instead of passing with zero assertions when the command under test unexpectedly succeeds — the failure they exist to detect was the one they could not see. Sites that already assert on the success path are unchanged. `npm test` also refuses to run against a **stale** `dist/`, not just a missing one: the suites execute the built CLI, so a stale build reported results that did not describe the current source. A new suite checks every documented `deepl …` invocation in the README and docs against the CLI's real surface, so a command or flag that drifts out of existence fails CI rather than waiting for a reader to find it.
 - **docs**: The README leads with the npm install path and marks Homebrew as pending until the tap exists. Install instructions target `@deepl/cli` (10 occurrences across `docs/SYNC.md`, four example scripts, `examples/README.md`, and the git-hook template in `src/services/git-hooks.ts`). The README installation section documents three install paths — npm (`npm install -g @deepl/cli`), from source, and Homebrew (`brew install deepl/tap/deepl`, marked pending until the tap ships) — with an explicit Node.js 24 prerequisite, replacing the `better-sqlite3` native-compilation caveat (Xcode CLT / python3-make-gcc), which no longer applies. The `TROUBLESHOOTING.md` `NODE_MODULE_VERSION` / `npm rebuild better-sqlite3` entry is replaced by accurate guidance for the one remaining cache-degradation cause (running on Node < 24). Six stale `DeepLcom` GitHub URLs now point at the `DeepL` org directly. `CONTRIBUTING.md` states the Node 24 development prerequisite, and `SECURITY.md`'s supported-versions table reflects that only 2.x is a published, supported line.
 - **tests**: Removed the global manual mocks for `p-limit` and `fast-glob` (`tests/__mocks__/`). Because a manual mock for a node module is auto-applied to every suite, and `resetMocks: true` strips its implementation, `pLimit(n)` resolved to `undefined` and `fg(...)` resolved to `undefined` in all 236 suites — so no test exercised a real concurrency limit or a real glob walk, and two concurrency defects reached a release candidate undetected. The suites that need these mocked declare them explicitly with their own implementations, so nothing depended on the global versions. Added `tests/unit/concurrency-limiting.test.ts`, which asserts that `p-limit` rejects a non-positive concurrency, that peak overlap never exceeds the limit, and that `fast-glob` returns real paths — it fails if either global mock is reintroduced.
@@ -52,6 +53,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- **formats**: TOML reconstruction escapes U+2028/U+2029 (Unicode line/paragraph separators) in double-quoted values, and literal-string values gaining one fall back to double quotes. Written raw, these characters broke the entry-line scan on the *next* sync (JavaScript's `.` excludes line terminators), which re-appended the key as a duplicate and made the third sync fail to parse the file at all — first sync fine, second silently corrupting, third crashing. Found by the property-based round-trip suite.
+- **formats**: `.properties` reconstruction escapes leading spaces in values (`\ `), which the value parser otherwise strips on the next read — a translation beginning with a space silently lost it on every subsequent sync. Leading tabs, trailing spaces, and newlines were already escaped correctly. Found by the property-based round-trip suite.
 - **sync**: `--auto-commit` now recognises its own translation output rather than only the files a given run wrote, which fixes two related problems. A translation left on disk by an earlier refused run was classed as an unrelated modification, so auto-commit refused forever and the user had to commit it by hand; it is now committed once the genuinely unrelated changes are dealt with. And in `--watch` mode, where the same path runs once per trigger, a trigger that translated nothing skipped the checks entirely and reported success while a commit was still owed. Staging is also driven by what is actually dirty, so a rewrite that produced identical bytes no longer attempts an empty commit. Ownership is derived from the lockfile's tracked source files, with each file matched to its own bucket so one bucket's `target_path_pattern` cannot claim another's output.
 - **sync**: Re-running `deepl sync --auto-commit` after a refusal no longer reports success without committing. A refused run still writes its translations to disk, so the retry found nothing to translate; the preflight was skipped in that case and the command exited 0 with no commit made, the translation still uncommitted and the tree still dirty. The repo-state checks now run whenever `--auto-commit` is requested, so the refusal is reported identically on every attempt. Staging and committing are still skipped when there is nothing to commit. Two tests covered this path but could not see the defect: their assertions ran only inside a `catch` the retry never entered, which also hid an assertion that could never have matched (it looked for "detached HEAD" against a message reading "HEAD is detached").
 - **write**: `deepl write` now runs from a published install. `diff` is imported at the top of the write command but was declared only under `devDependencies`, which consumers do not install, so every command that loaded the module failed with `Cannot find package 'diff'` before producing output. `--help` did not surface it because the module loads lazily.

package.json

@@ -92,6 +92,7 @@
     "eslint": "^10.8.0",
     "eslint-config-prettier": "^10.1.8",
     "eslint-plugin-jest": "^29.16.0",
+    "fast-check": "^4.9.0",
     "globals": "^17.8.0",
     "jest": "^30.4.2",
     "memfs": "^4.64.0",

src/formats/properties.ts

@@ -155,8 +155,12 @@ export class PropertiesFormatParser implements FormatParser {
   }
 
   private escapeValue(s: string): string {
-    let result = '';
-    for (const ch of s) {
+    // Leading spaces must be escaped: the value parser strips unescaped
+    // whitespace after the separator, so a raw leading space is lost on
+    // the next read. (Leading tabs are covered by the \t escape below.)
+    const leading = /^ +/.exec(s);
+    let result = leading ? '\\ '.repeat(leading[0].length) : '';
+    for (const ch of s.slice(leading ? leading[0].length : 0)) {
       switch (ch) {
         case '\n': result += '\\n'; break;
         case '\t': result += '\\t'; break;

src/formats/toml.ts

@@ -241,20 +241,25 @@ export class TomlFormatParser implements FormatParser {
 function encodeTomlString(value: string, useDoubleQuote: boolean): string {
   if (!useDoubleQuote) {
     // Literal strings (single-quoted) are raw — no escapes, cannot contain `'`
-    // or a newline. If the translation has either, fall back to double-quoted.
-    if (value.includes("'") || /[\n\r]/.test(value)) {
+    // or a newline. U+2028/U+2029 also force the double-quoted fallback: left
+    // raw they break ENTRY_LINE_RE on the next pass (JS `.` excludes line
+    // separators), so the key is re-appended as a duplicate.
+    if (value.includes("'") || /[\n\r\u2028\u2029]/.test(value)) {
       return encodeTomlString(value, true);
     }
     return `'${value}'`;
   }
 
   // Double-quoted: escape backslash first, then `"`, then common control chars.
   // Order matters so later escapes don't double-escape the backslashes we add.
+  // U+2028/U+2029 must never be written raw (see the literal-string comment).
   const escaped = value
     .replace(/\\/g, '\\\\')
     .replace(/"/g, '\\"')
     .replace(/\n/g, '\\n')
     .replace(/\r/g, '\\r')
-    .replace(/\t/g, '\\t');
+    .replace(/\t/g, '\\t')
+    .replace(/\u2028/g, '\\u2028')
+    .replace(/\u2029/g, '\\u2029');
   return `"${escaped}"`;
 }

tests/property/arbitraries.ts

@@ -0,0 +1,86 @@
+/**
+ * Shared fast-check arbitraries for the property suite.
+ *
+ * Translation values are grapheme-based (no lone surrogates) with a pool of
+ * adversarial constants mixed in: format syntax, entities, quotes, RTL,
+ * astral characters, combining marks, placeholder patterns.
+ *
+ * Control characters other than \n and \t are excluded: they are invalid in
+ * XML 1.0 and questionable input for every format, so they would only add
+ * invalid-input noise to the run.
+ */
+import fc from 'fast-check';
+
+export const NASTY_STRINGS: readonly string[] = [
+  'Hello 😀 world', // astral / surrogate pair
+  'café', // combining acute (NFD-style)
+  'café', // precomposed (NFC)
+  'שלום עולם', // RTL Hebrew
+  'ثم شرب القهوة', // RTL Arabic
+  '日本語のテキスト',
+  'Türkçe İstanbul ı',
+  'Terms & Conditions',
+  '5 < 10 && 10 > 5',
+  '<b>bold</b> <i>it</i>',
+  ']]>',
+  '"double" and \'single\' quotes',
+  'back\\slash \\n literal',
+  '{name} has {{count}} items',
+  '${var} and %1$s and %s and %@',
+  'line one\nline two',
+  'tab\there',
+  '  leading and trailing  ',
+  "it''s '{'quoted'}' ICU",
+  'key=value # hash ; semi : colon',
+  'msgid "injected"',
+  '<?php echo "hi"; ?>',
+  '[section] toml = "like"',
+  '- yaml: like',
+  '=0 one other',
+];
+
+// eslint-disable-next-line no-control-regex -- excluding control chars is the point
+const CONTROL_CHARS_RE = /[\u0000-\u0008\u000B-\u001F\u007F-\u009F]/;
+
+/** A single translation value: never empty, no control chars beyond \n \t. */
+export const translationArb: fc.Arbitrary<string> = fc
+  .oneof(
+    {
+      weight: 3,
+      arbitrary: fc.string({ unit: 'grapheme', minLength: 1, maxLength: 60 }),
+    },
+    { weight: 2, arbitrary: fc.constantFrom(...NASTY_STRINGS) },
+    {
+      weight: 1,
+      arbitrary: fc
+        .tuple(
+          fc.constantFrom(...NASTY_STRINGS),
+          fc.string({ unit: 'grapheme', maxLength: 20 })
+        )
+        .map(([nasty, tail]) => `${nasty}${tail}`),
+    }
+  )
+  .filter((s) => s.length > 0 && !CONTROL_CHARS_RE.test(s));
+
+/** Exactly `n` translation values. */
+export function translationsArb(n: number): fc.Arbitrary<string[]> {
+  return fc.array(translationArb, { minLength: n, maxLength: n });
+}
+
+export const FC_NUM_RUNS = Number(process.env['FC_NUM_RUNS'] ?? 200);
+
+/**
+ * Common fc.assert parameters. Replay a recorded failure with:
+ *   FC_SEED=<seed> FC_PATH=<path> npx jest tests/property -t '<test name>'
+ */
+export function fcParams(): fc.Parameters<unknown> {
+  const params: fc.Parameters<unknown> = { numRuns: FC_NUM_RUNS };
+  if (process.env['FC_SEED']) {
+    params.seed = Number(process.env['FC_SEED']);
+  }
+  if (process.env['FC_PATH']) {
+    params.path = process.env['FC_PATH'];
+    params.endOnFailure = true;
+  }
+  return params;
+}

tests/property/batch-splitting.property.test.ts

@@ -0,0 +1,114 @@
+/**
+ * Property tests for TranslationService.translateBatch chunking and
+ * index mapping (src/services/translation.ts).
+ *
+ * Laws, with a fake client and the cache disabled:
+ *   B1  no client call carries more than TRANSLATE_BATCH_SIZE texts
+ *   B2  the texts sent across all calls are exactly the unique non-empty
+ *       inputs, each sent once (dedup)
+ *   B3  every non-empty input position gets its own text's translation,
+ *       duplicates included (index mapping)
+ *   B4  the number of client calls is ceil(unique / TRANSLATE_BATCH_SIZE)
+ */
+import fc from 'fast-check';
+import {
+  TranslationService,
+  TRANSLATE_BATCH_SIZE,
+} from '../../src/services/translation';
+import type { DeepLClient } from '../../src/api/deepl-client';
+import type { ConfigService } from '../../src/storage/config';
+import { fcParams } from './arbitraries';
+
+interface RecordedCall {
+  texts: string[];
+}
+
+function makeFakes(): {
+  client: DeepLClient;
+  config: ConfigService;
+  calls: RecordedCall[];
+} {
+  const calls: RecordedCall[] = [];
+  const client = {
+    translateBatch: (texts: string[]) => {
+      calls.push({ texts: [...texts] });
+      return Promise.resolve(texts.map((t) => ({ text: `X:${t}` })));
+    },
+  } as unknown as DeepLClient;
+  const config = {
+    get: () => ({ defaults: {} }),
+    getValue: () => false, // cache.enabled -> false
+  } as unknown as ConfigService;
+  return { client, config, calls };
+}
+
+/**
+ * Input positions index into a small pool so duplicates are common — the
+ * index-mapping law is only interesting when the same text appears at
+ * several positions.
+ */
+const textsArb = fc
+  .tuple(
+    fc.array(fc.string({ unit: 'grapheme', minLength: 1, maxLength: 20 }), {
+      minLength: 1,
+      maxLength: 130,
+    }),
+    fc.array(fc.nat(129), { minLength: 0, maxLength: 200 })
+  )
+  .map(([pool, picks]) => {
+    // Suffix with the pool index so pool entries are pairwise distinct.
+    const distinct = pool.map((t, i) => `${t}#${i}`);
+    return picks.map((p) => distinct[p % distinct.length]!);
+  });
+
+describe('translateBatch chunking and index mapping', () => {
+  it('B1-B4: chunk size, dedup, index mapping, call count', () => {
+    return fc.assert(
+      fc.asyncProperty(textsArb, async (texts) => {
+        const { client, config, calls } = makeFakes();
+        const service = new TranslationService(client, config);
+
+        const results = await service.translateBatch(texts, {
+          targetLang: 'de',
+        });
+
+        // B1
+        for (const call of calls) {
+          expect(call.texts.length).toBeLessThanOrEqual(TRANSLATE_BATCH_SIZE);
+        }
+
+        // B2
+        const sent = calls.flatMap((c) => c.texts);
+        const unique = [...new Set(texts.filter((t) => t))];
+        expect(sent.length).toBe(unique.length);
+        expect(new Set(sent)).toEqual(new Set(unique));
+
+        // B3
+        expect(results.length).toBe(texts.length);
+        for (let i = 0; i < texts.length; i++) {
+          if (texts[i]) {
+            expect(results[i]?.text).toBe(`X:${texts[i]!}`);
+          }
+        }
+
+        // B4
+        const expectedCalls =
+          unique.length === 0
+            ? 0
+            : Math.ceil(unique.length / TRANSLATE_BATCH_SIZE);
+        expect(calls.length).toBe(expectedCalls);
+      }),
+      fcParams()
+    );
+  });
+
+  it('empty input makes no client calls and returns an empty array', async () => {
+    const { client, config, calls } = makeFakes();
+    const service = new TranslationService(client, config);
+
+    const results = await service.translateBatch([], { targetLang: 'de' });
+
+    expect(results).toEqual([]);
+    expect(calls).toEqual([]);
+  });
+});

tests/property/cache-key-normalization.test.ts

@@ -0,0 +1,33 @@
+/**
+ * Directed demonstration (not a property).
+ *
+ * TranslationService.generateCacheKey hashes the raw text without Unicode
+ * normalization, so NFC and NFD encodings of the same visible string produce
+ * two cache entries — and therefore two billable API calls.
+ *
+ * generateCacheKey reads nothing from `this`, so it is invoked via the
+ * prototype without constructing the service.
+ */
+import { TranslationService } from '../../src/services/translation';
+
+type KeyFn = (text: string, options: { targetLang: string }) => string;
+
+const generateCacheKey = (
+  TranslationService.prototype as unknown as { generateCacheKey: KeyFn }
+).generateCacheKey;
+
+describe('translation cache key normalization', () => {
+  it('documents that NFC and NFD of the same visible string produce distinct keys', () => {
+    const nfc = 'café'.normalize('NFC');
+    const nfd = 'café'.normalize('NFD');
+    expect(nfc).not.toBe(nfd); // different code points
+    expect(nfc.normalize('NFC')).toBe(nfd.normalize('NFC')); // same visible string
+
+    const keyNfc = generateCacheKey.call(null, nfc, { targetLang: 'de' });
+    const keyNfd = generateCacheKey.call(null, nfd, { targetLang: 'de' });
+
+    // Current behavior: two entries for one visible string. Whether this is
+    // a bug or intentional exact-input keying is a policy decision.
+    expect(keyNfc).not.toBe(keyNfd);
+  });
+});