Skip to content

Silent data loss in the sync merge path — five ways translations vanish while the run reports success #90

Description

@mmcky

Five defects found in the 2026-07-15 deep review (REVIEW-FABLE5-2026-07-15.md §6.1) that are not covered by PLAN Phase 2. They share one shape: sync loses or corrupts target content and reports success. Nothing fails, nothing warns, and the loss is only visible by diffing the output against what should have happened.

That shape is why they matter more than their individual severity suggests — and it is the same class as the truncation bug fixed in #83 (a cut-off translation committed as complete) and the harness bugs fixed in #86. Line references verified against main @ 9f317e5.

1. Heading-map lookups fail for every role-bearing heading — [M]

Map values are written role-stripped (src/heading-map.ts:94, via stripMystRoles), but Strategy-1 lookup compares them against the raw target heading (src/file-processor.ts:738):

const cleanTargetHeading = targetSection.heading.replace(/^#+\s+/, '').trim();

So the stored value 熊猫 is compared against {index}`熊猫 <single: 熊猫>` and never matches. QuantEcon lectures use {index} roles in headings routinely (v0.12.5 exists because of it), and translations preserve them.

Failure: with equal section counts the positional fallback masks it. With unequal counts the section is either skipped (silently dropped from the PR) or treated as new — re-translated from scratch, discarding accumulated human refinements, which is the one thing the pipeline is supposed to never do.

The rebase added-section path at file-processor.ts:226-231 strips both sides correctly. The asymmetry is the bug. Fix: one canonicalizeHeading() used at every write and lookup site (REVIEW §7.6 R12).

2. Target-only sections are silently deleted — [M]

Reconstruction iterates only newSource.sections (src/file-processor.ts:176-217, :436-441). Any section existing only in the target — human-added content — is omitted from the output with no warning. skippedSections/onSkippedSection covers only the source-side analog.

The CLI's forward path at least warns (TARGET_HAS_ADDITIONS); the action destroys silently.

3. A per-file fetch failure drops the file from the sync — [M]

src/index.ts:945 (also :1005, :436):

catch (error) { core.error(`Error fetching content for ${file.filename}: ${error}`); }

core.error is a log annotation — it neither records an error nor fails the run. A transient 5xx on one file yields a PR missing that file, a success comment listing the rest, no failure issue, and a green check.

Fix: have fetchAllFileContents return {files, errors} and treat fetch errors like processing errors (failure issue + setFailed). No happy-path change.

4. Heading level promotion/demotion loses translations or leaks English — [M]

src/diff-detector.ts:94-97 + src/file-processor.ts:697-752, :377-397.

  • Promote ### Setup## Setup: findSectionById matches the nested old section recursively, producing a level-crossing "modified"; target lookup searches only top-level sections and the map key is path-based (Overview::Setup), so the leaf lookup of Setup misses → "treating as new" → existing translation discarded and re-translated.
  • Demote ## Setup### Setup: the merge helper looks up Overview::Setup, which doesn't exist (the old key was top-level Setup), falls through to "keep source as-is" → an untranslated English subsection embedded in the zh-cn/fa document, run reports success.

Not covered by the tracked rewording (ARCHITECTURE R5) or duplicate-slug items.

5. injectHeadingMap re-types frontmatter scalars — [M→L]

src/heading-map.ts:193 round-trips the whole target frontmatter through yaml.load/yaml.dump. Verified with the repo's own js-yaml: date: 2026-07-15date: 2026-07-15T00:00:00.000Z; 1.101.1. Silent frontmatter rewriting on every processed file that carries an implicitly-typed scalar.

Why together

1 and 4 are the same root cause (heading identity is compared inconsistently across sites) and both discard human translations. 2 and 3 are both "output is quietly missing content". They want one fix each but a single verification: the Phase 2 round-trip invariant test would catch 1, 2 and 4 as a class, which is the argument for doing Phase 2 first rather than patching these individually.

Refs: REVIEW-FABLE5-2026-07-15.md §6.1 and §7.6 (R12), PLAN Phase 2, #65

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't workinghigh-priorityAddress soon

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions