Skip to content

fix(sync): apply typography in the sync path, typeset heading map with body - #99

Merged
mmcky merged 2 commits into
mainfrom
fix/sync-typography
Jul 16, 2026
Merged

fix(sync): apply typography in the sync path, typeset heading map with body#99
mmcky merged 2 commits into
mainfrom
fix/sync-typography

Conversation

@mmcky

@mmcky mmcky commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Closes #97.

applyTypography() had exactly one production caller — init.ts:296 — so the action's sync path silently stripped the non-breaking spaces the seed inserted, falling back on the prompt instruction #79 was opened because the model ignores it. v0.16.0 shipped the transform with this scope limit noted in its changelog entry; this closes it.

What changed

Both sync entry points in file-processor.ts are now typeset, at the point where init does it — before anything derives from the text:

Entry point Where the transform runs
processFull on the raw model output, before the heading map is built from it (one line, mirrors init.ts)
processSectionBased on the reconstructed document, before the map is injected

The part worth reviewing

The obvious implementation is wrong in a way that is worse than the bug, so it is worth being explicit about why the code looks like it does.

applyTypography skips frontmatter by design. Both processSectionBased and processFull return content with the heading map already injected into frontmatter. So wrapping the returned string — the natural reading of "call it in the sync path" — typesets body headings while leaving map values with the plain spacing the model emitted.

That matters because heading lookup is an exact === compare. The only normalisation is cleanHeading, which strips the # prefix, strips MyST roles, and trims — there is no interior-whitespace or U+00A0 handling anywhere in the matching path. The ID fallback compares an English id against a French one and fails; the positional fallback is skipped whenever the section count changed. What remains is that an unchanged section is silently dropped from the output, and a modified section is retranslated from English, discarding human edits.

So the body, the map, and the title are typeset together, which is what scripts/typography/apply.mjs already does. This relies on typeset(cleanHeading(h)) === cleanHeading(typeset(h)), which holds because the transform is whitespace-only and masks inline roles.

Only headings the model produces fresh can drift — modified sections reuse the existing target heading, so they cannot desync. That makes an added section the case that actually reproduces the trap, and it is what the tests use.

Verification

Rather than trust that the new tests pass, I ran them against both broken variants:

Variant Result
unfixed main 5 of 9 fail
body-only typography (the trap) 3 of 9 fail, including the added-section case, which reports a skipped section — the data loss described above
this PR 9 of 9 pass

Full suite: 1090 passing, lint and format clean, dist-action/ rebuilt.

Decisions worth a second opinion

Document-scoped, not section-scoped. Typesetting the whole reconstructed document also repairs drift in sections the source PR did not touch, so drift self-corrects on each file's next sync. The cost is that sync PRs may carry whitespace-only changes on lines the source change did not touch. Section-scoped would keep diffs tracking the source exactly but leave every existing edition dependent on a manual backfill. Happy to switch if you would rather diffs stay minimal.

Not included, per the issue discussion: removing the now-redundant nbsp rule from additionalRules. That is a prompt-behaviour change and separable from this bug fix. Note the guillemets rule on the adjacent line should not get the same treatment — the transform is spacing-only and has no guillemets handling, so dropping that rule would lose the behaviour outright rather than make it deterministic.

Backfill still wanted. Editions carry drift from syncs merged before this lands. A one-off scripts/typography/apply.mjs --lang fr per edition resets the baseline; numba.md needs it regardless. Referenced sync PR for context: QuantEcon/lecture-python-programming.fr#6.

🤖 Generated with Claude Code

applyTypography() ran only in the `translate init` seed path, so every sync
stripped the non-breaking spaces the seed inserted and fell back on the prompt
instruction #79 was opened because the model ignores. One merged sync took
numba.md from 27 to 14 non-breaking spaces, invisible in review because the
diff renders as unchanged text.

Both sync entry points now typeset:

- processFull: on the raw model output, before the heading map is derived from
  it — the same ordering as init.ts.
- processSectionBased: on the reconstructed document, before the map is
  injected. Document-scoped, so drift in sections the source PR did not touch
  is repaired on the next sync of that file.

The map is typeset alongside the body rather than after it. applyTypography
skips frontmatter by design, so typesetting already-injected content would
leave map values plain-spaced while body headings gained non-breaking spaces.
Heading lookup is an exact string compare with no whitespace normalisation, and
the positional fallback is unavailable whenever the section count changes, so
that mismatch silently drops unchanged sections and retranslates modified ones
from English, discarding human edits. Body, map, and title are typeset
together, matching what scripts/typography/apply.mjs already did.

Tests were verified to fail against both the unfixed code and the body-only
variant; against the latter the added-section case reports a skipped section,
which is the data loss above.

See #97

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 16, 2026 04:27

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a long-standing gap where deterministic typography (applyTypography) was only applied during translate init, but not during the action’s sync path—causing French NBSP spacing to be stripped on subsequent syncs and potentially breaking exact heading-map matching.

Changes:

  • Apply applyTypography() in FileProcessor.processFull() before deriving the heading map, so the map is built from the same typeset strings that land in the body.
  • Apply typography in FileProcessor.processSectionBased() in a way that keeps body + heading-map values + translation title typeset together (avoiding the “body-only” trap).
  • Add targeted unit tests that reproduce the destructive mismatch scenario (especially added-section cases) and pin the invariant.

Reviewed changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.

File Description
src/file-processor.ts Applies deterministic typography in both sync entry points while keeping heading-map/title consistent with typeset body output.
src/tests/file-processor-typography.test.ts Adds regression tests covering missing typography, body-only typography trap, added-section matching, drift repair, and title typesetting.
dist-action/index.js Rebuilt bundle reflecting the new typography application in the sync path.
CHANGELOG.md Documents the fix and the rationale (including the “body-only” mismatch failure mode).

Comment thread src/__tests__/file-processor-typography.test.ts Outdated
Comment thread CHANGELOG.md Outdated
mmcky added a commit that referenced this pull request Jul 16, 2026
Addresses Copilot feedback on #99.

The test file defined NBSP as a raw literal, the only such definition in the
repo — src/typography.ts and src/__tests__/typography.test.ts both use the
' ' escape. An invisible character is a poor definition for the constant
every assertion in the suite depends on, since an editor could mangle it
silently and the tests would then pass vacuously. Line 16 was the only literal;
every fixture already interpolates ${NBSP}, so the escape covers the file.

Verified the change is a behaviour no-op by re-running the tests against both
broken variants: 7 of 9 still fail against main's file-processor.ts, and 3 of 9
against the body-only variant.

The CHANGELOG clause tried to extract the object of "ignores" out of a
because-clause, which no single missing word repairs. Rewritten to match the
wording in the issue.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Addresses Copilot feedback on #99.

The test file defined NBSP as a raw literal non-breaking space, the only such
definition in the repo -- src/typography.ts and src/__tests__/typography.test.ts
both spell it as an escape sequence. An invisible character is a poor definition
for the constant every assertion in the suite depends on: an editor could mangle
it silently, and the tests would then compare plain spaces to plain spaces and
pass vacuously. Line 16 held the only literal; every fixture already interpolates
${NBSP}, so fixing the definition covers the file.

Verified the change is a behaviour no-op by re-running the tests against both
broken variants: 7 of 9 still fail against main's file-processor.ts, and 3 of 9
against the body-only variant.

The CHANGELOG clause tried to extract the object of "ignores" out of a
because-clause, which no single missing word repairs. Rewritten to match the
wording used in the issue.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@mmcky
mmcky force-pushed the fix/sync-typography branch from 13ca264 to 56ec6c4 Compare July 16, 2026 04:33
@mmcky
mmcky merged commit c906d91 into main Jul 16, 2026
1 check passed
@mmcky
mmcky deleted the fix/sync-typography branch July 16, 2026 04:55
@mmcky

mmcky commented Jul 16, 2026

Copy link
Copy Markdown
Contributor Author

Post-merge correction, from an independent fact-check of this PR's claims against the code.

The verification table above is wrong in one cell: "unfixed main | 5 of 9 fail" should read 7 of 9. Re-running the merged test suite against the pre-merge file-processor.ts fails 7 of 9 — as this PR's own reply comment in the NBSP review thread correctly states. The 5-of-9 figure came from an earlier revision of the tests, before two guard tests were rebuilt around an added-section fixture, and the table was not updated to match. The stronger number favours the suite, but the two public statements contradicted each other and the table is the one that was wrong.

Also worth qualifying: the "3 of 9 fail against the body-only variant" row is not reproducible as stated, because the exact variant tested was never recorded. The variant behind that number wrapped only processSectionBased's return in applyTypography; a reconstruction that wraps both entry points fails 4 of 9. The substantive claim — that the added-section test catches the body-only trap by reporting a dropped section — holds for both variants.

One further correction, to the analysis comment on #97 that preceded this PR: it said heading matching's "only normalisation is cleanHeading, which strips the # prefix, strips MyST roles, and trims". That overstates the body side of the compare, which stripped only the # prefix and never stripped MyST roles — meaning role-wrapped body headings could never match by heading map at all and survived on the positional fallback alone. That stricter-than-stated compare is a pre-existing hole, and it — plus a real map/body divergence found in the fr corpus (python_oop.md's title carries an NBSP its role-masked body heading structurally cannot) — is addressed in #100.

mmcky added a commit that referenced this pull request Jul 16, 2026
…phy gaps (#100)

* fix(matching): normalize heading comparisons; close remaining typography gaps

Matching a frontmatter heading-map value to its body heading was byte
equality, and typography makes the two sides legitimately diverge:
applyTypography masks role spans while stripMystRoles exposes their display
text (a map value can carry an NBSP its body heading structurally cannot),
apply.mjs typesets map values independently of the body, and human edits
touch one side only. On a mismatch, sync drops unchanged sections or
retranslates modified ones from English, discarding human edits.

normalizeHeadingForMatch canonicalizes both sides of every comparison:
strip # markers and MyST roles, fold NBSP/narrow-NBSP, collapse whitespace
runs, drop spaces before ; : ! ? — covering the zero-gap shape where
applyTypography inserts an NBSP the model never emitted. Exact compares run
first and a normalized match is accepted only when unique, so raw-distinct
headings that canonicalize identically keep their positional pairing
instead of first-match landing on the wrong section. Written values stay
exactly as typeset; only comparisons normalize.

Also closed, from the audit that produced this change:

- The body side of Strategy 1 never stripped MyST roles while map values
  always did, so role-wrapped headings could never match by heading map and
  survived on the positional fallback alone (pre-existing).
- The CLI validator and subsection merging built lookup keys from raw
  source headings while keys are stored role-stripped (pre-existing).
- translate forward's whole-file resync wrote raw model output — the one
  write path without deterministic typography.
- translate headingmap's byte-equality change detection ping-ponged with
  apply.mjs, each rewriting the other's output forever.
- CHANGELOG: restored the [0.16.1] release header PR #98 accidentally
  deleted, and corrected the now-false "exact string compare" sentence in
  the unreleased #99 entry.

Every behavior change is pinned by a test verified to fail against main;
the ambiguity guard is additionally verified to fail against the naive
always-normalized variant it protects against. All 401 headings of the
French edition match under every typography variant (one diverged before).

See #97

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* fix(matching): make the subsection-merge fallback substitute headings consistently

Addresses the Copilot comment on #100, whose underlying defect is real but
differently shaped than stated: the map-hit substitution never reached the
document at all. serializeSection writes section.content, whose first line
is the heading, so replacing only the .heading field left the body English
while updateHeadingMap recorded the translated heading — a map/body
divergence the next sync cannot match (pre-existing for plain headings; the
role-stripped key fix in this PR extended it to role-wrapped ones).

The fallback now writes the map heading into both .heading and .content for
role-free headings, and keeps role-wrapped headings as source — map values
are role-stripped display text, so substituting one would drop the {role}
wrapper. Pinned by a test verified to fail against the pre-repair code.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mmcky added a commit to QuantEcon/lecture-python-programming.fr that referenced this pull request Jul 16, 2026
The v0.16.1 sync path did not apply deterministic typography, so PR #6
regressed 14 lines of numba.md from U+00A0 to plain spaces before high
punctuation (documented in QuantEcon/action-translation#97). This re-runs
scripts/typography/apply.mjs --lang fr, whose postcondition verifies the
only difference is spacing before ; : ! ?. One net-new non-breaking space
relative to the seed comes from a line #6 added.

The action-side fix so syncs stop stripping these landed in
QuantEcon/action-translation#99, with matching hardened in
QuantEcon/action-translation#100.

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@mmcky mmcky mentioned this pull request Jul 16, 2026
mmcky added a commit that referenced this pull request Jul 16, 2026
* chore: release v0.17.0

Promote [Unreleased] to [0.17.0]: the typography-erosion fixes (#99
closes #97: sync path now applies deterministic typography; #100:
typography-insensitive heading matching with exact-first + ambiguity
guard, forward-resync typesetting, headingmap/apply.mjs ping-pong,
role-stripped key lookups, subsection-merge consistency) and the
concurrent-review dedup (#98). Bump package.json/package-lock,
copilot-instructions version + test counts (43 suites); record the
release and the estate-pin follow-up in .dev/STATE; refresh the
release-notes scratch file.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* chore: STATE.md — don't claim v0.17.0 released before the tag exists

Addresses Copilot review on #101: the release entry now says the tag and
GitHub release land on merge of this PR, so the note is accurate whether
read before or after.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Sync path never applies applyTypography() — every sync strips the French non-breaking spaces the seed inserted

2 participants