Apply localisation rules on the sync path for newly-created files - #225
Apply localisation rules on the sync path for newly-created files#225mmcky wants to merge 3 commits into
Conversation
`init` has always applied the localisation rules — code comments, figure
labels, i18n font config. The sync path applied none of them, so every
lecture arriving through the automated production path landed with English
figure labels and no CJK font block, and the divergence from the seeded
corpus grew with each upstream merge.
The plumbing was already there: `customInstructions` exists on
FullDocumentTranslationRequest and is interpolated into the prompt. Only
the sync path never populated it — file-processor.ts called
translateFullDocument with four fields and no instructions.
Measured on lecture-python.zh-cn (2026-07-27), sync-created lectures
against init-seeded ones in the same edition:
sync init
font config 0/7 37/37
figure labels 0/64 449/562
code comments 0/21 469/470
Only NEW files are affected. An existing translation already carries its
localisation and the translator prompts preserve it (#107), so nothing
changes for the update path — pinned by a test.
The rebase path gets the same wiring, since it can re-create a file from
scratch and that output needs localising for the same reason.
Adds a `localize` action input mirroring the CLI's `--localize`, so an
edition can opt out or select a subset. An unrecognised rule name throws
rather than falling back: a typo would otherwise ship a whole lecture
unlocalised, and nothing downstream fails on that — which is exactly how
this defect stayed invisible for so long.
Refs #178
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
| Configuration | Structural pass rate | 95% CI (Wilson) |
|---|---|---|
--localize default (all three rules) |
2/7 = 28.6% | [8%, 64%] |
--localize none |
6/7 = 85.7% | [49%, 97%] |
Fisher exact p = 0.103 — not significant, and I want to be explicit that this does not establish the effect. But the direction was consistent across every run, and the relevant asymmetry is that the sync path is 7/7 structurally clean in production today precisely because it passes no localisation rules. This PR removes that property.
Every failure carried the identical signature: directive #1 name changed: source line 15 has {raw}, output line 45 has {index}, plus the document anchor missing.
Why the guard does not make this safe
checkStructuralParity catches the corruption and refuses the write, so nothing malformed reaches a repo. But the failure mode on refusal is a silently missing file — the run reports success, the PR omits the lecture, and nothing says so unless the same run also wrote a _toc.yml entry (#156, #222). Trading "lands unlocalised" for "sometimes does not land at all, silently" is not obviously the better trade, and it is not a trade this PR's description acknowledges.
What would settle it
Per the estate's harness-first policy, validate on test-translation-sync before this reaches a production edition. Two things worth knowing about that:
- The harness fixtures
lecture.mdandlecture-minimal.mdcontain zero{raw} jupyterblocks and zero(label)=anchors — so as they stand they cannot exercise this failure mode at all. A fixture carrying a QuantEcon-style head block is a prerequisite for the harness to be informative here. Head-block prevalence in production is 97/138 inlecture-python.myst. - A powered replicate run would settle it directly. At the observed effect size, ~15 runs per cell gives 80% power; the design here (n=7) had roughly 13%. At the ~$0.21/file measured in Phase 1 run-book (Mon 2026-07-28): ml benchmark — Sonnet 5 vs Opus 4.8 vs native reference #194 that is on the order of $10.
Options
- Hold until the above is answered. Safest; the localisation gap has existed for months, so a few more days costs little.
- Merge with
localize: noneas the action default, flipping to rules-on per edition once validated. Ships the mechanism without changing behaviour — but then the PR fixes nothing until someone flips it. - Merge as-is and accept a possible increase in silent file drops, mitigated by sync silently first-translates lectures missing from the target — unreachable when it succeeds, invisible when it fails #222's proposed metadata-vs-delivered comparison landing first so the drops stop being silent.
My recommendation is to hold, and to treat the fixture gap in item 1 as the blocking prerequisite — without it the harness would return a green result that means nothing.
There was a problem hiding this comment.
Pull request overview
This PR closes #178 by wiring the existing localization-rule prompt plumbing into the GitHub Action sync (and rebase) path so that newly created translated files receive the same code-cell localization instructions (code-comments, figure-labels, i18n-font-config) that translate init has long applied.
Changes:
- Add a new
localizeaction input, parsed/validated into alocalizationRulesarray (supportingnoneto opt out and failing fast on unknown rule names). - Pass a localization prompt (
customInstructions) into full-document translation when syncing a new file (and for rebase runs via the same input wiring). - Add Jest coverage pinning that new files receive localization prompts while existing files continue to use the section-based update path.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/types.ts | Adds localizationRules to ActionInputs and RebaseInputs. |
| src/sync-orchestrator.ts | Builds/passes localization prompt to processFull for new markdown files during sync. |
| src/inputs.ts | Parses localize input via parseLocalizationRules with defaults and strict validation. |
| src/index.ts | Threads inputs.localizationRules into SyncOrchestrator config for sync and rebase. |
| src/file-processor.ts | Extends processFull to accept customInstructions and forwards it to translateFullDocument. |
| src/tests/sync-localisation.test.ts | Adds tests asserting localization prompt wiring for new files and opt-out behavior. |
| docs/user/action-reference.md | Documents the new localize input and its behavior/constraints. |
| action.yml | Adds the localize input with default rule set and description. |
| dist-action/index.js | Updates the committed bundle to reflect the new runtime behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // A new file has no existing localisation to preserve, so the rules have | ||
| // to be applied here or the lecture lands with English figure labels and | ||
| // no font config (#178). `init` has always done this; sync never did. | ||
| const rules = this.config.localizationRules ?? DEFAULT_RULES; | ||
| const localizationPrompt = | ||
| rules.length > 0 ? buildLocalizationPrompt(rules, this.config.targetLanguage) : ''; | ||
| if (localizationPrompt) { | ||
| this.logger.info( | ||
| `${file.filename}: new file — applying localisation rules (${rules.join(', ')})` |
There was a problem hiding this comment.
Fixed in 5f2b1c5 — correct, and a genuine miss. I wired the rules into the isNewFile branch and did not notice processRenamedFile carries its own full-translation branch for the no-existing-translation case. A file reaching it is a first-time translation in every respect, so the defect this PR fixes was surviving in the path next door.
Your parenthetical is the important part: this is reachable more often than "someone ran git mv" suggests, because GitHub's rename detection is a similarity heuristic — a heavily-edited file gets reported as a rename, and the old path may simply never have been translated in the target.
Both call sites now share one private localizationPromptForNewFile helper so they cannot drift apart again, and the log line says "first-time translation" rather than "new file", since that is what both cases actually are. Three tests added: rules applied on the renamed-without-translation path, a renamed file that does have a translation staying on the section-based path, and the opt-out honoured there too.
From Copilot's review of #225. The first cut wired the rules into the isNewFile branch only, but processRenamedFile has its own full-translation branch for the case where the target has no translation at the old path. A file reaching that branch is a first-time translation in every respect and would still have landed unlocalised — the defect this PR fixes, surviving in the path next door. It is reachable more often than "someone ran git mv" suggests: GitHub's rename detection is a similarity heuristic, so a heavily-edited file can be reported as a rename, and the old path may simply never have been translated in the target. Both call sites now share one private helper, so the two paths cannot drift apart again, and the log line reads "first-time translation" rather than "new file" since that is what both cases are. Three tests added: rules applied on the renamed-without-translation path, a renamed file that does have a translation staying on the section-based path, and the opt-out honoured on the renamed path too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#226 (bibliography backfill) landed while this branch was open, and the two PRs added adjacent things in the same eight files: an action input, a SyncConfig field, an ActionInputs/RebaseInputs field, an orchestrator construction argument, and a docs table row each. Every conflict is additive and both sides are kept. One needed hand repair rather than keep-both: the SyncConfig conflict opened mid-JSDoc, so a mechanical resolution left the second field's comment without its `/**`. Verified after the merge: both fields on SyncConfig, both passed at both orchestrator construction sites, both inputs declared in action.yml, both parsed in getInputs and getRebaseInputs. 1485 tests across 65 suites — the union of both PRs' suites — with lint, format and the rebuilt bundle clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Validation complete — the risk is confirmed, and this should not merge as it standsMy earlier comment on this PR said the localisation-rule hypothesis was "not significant, and I want to be explicit that this does not establish the effect", on 7 runs per arm. I have now run it to the sample size that question needs. It is established. Correcting the record. ResultSame lecture (
Fisher exact, two-sided: p = 0.002. All nine recorded failures carry a byte-identical signature:
— the Why this blocks the PRThe sync path is 7/7 structurally clean on the new lectures it has created, precisely because it passes no localisation rules. This PR turns them on. On this evidence that moves sync from ~100% toward ~33%, and the failure mode on the parity guard's refusal is a silently missing file (#156, #222) — not a visibly broken one. That is the wrong trade: "every new lecture ships with English figure labels" is a visible, fixable defect; "two in three new lectures do not ship at all, and nothing says so" is not. Method and its limits15 runs per arm, chosen from a power calculation on the earlier effect size (~80% power; the original 7-per-arm design had ~13%). Stated honestly: this is measured through I did not run the E2E harness for this. Its fixtures ( Options
The code in this PR is not what is wrong — the wiring is correct and does what #178 asks for. What is wrong is the content of the rule prompt it now delivers on a path that was previously clean. |
Closes #178.
inithas always applied the localisation rules — code comments, figure labels, i18n font config. The sync path applied none of them, so every lecture arriving through the automated production path landed with English figure labels and no CJK font block, and the divergence from the seeded corpus grew with each upstream merge.The gap
The plumbing already existed.
customInstructionsis a field onFullDocumentTranslationRequestand is interpolated into the full-document prompt attranslator.ts:534. The sync path simply never populated it —file-processor.tscalledtranslateFullDocumentwith four fields and no instructions, so the rules thatinitpasses ascustomInstructions(cli/commands/init.ts:257) had no equivalent on the automated path.Evidence
Measured on
lecture-python.zh-cnon 2026-07-27, comparing the 7 lectures created by sync against the 37 seeded byinitin the same edition, against the same English sources:Every one of those 7 lectures uses matplotlib in its source, so every one renders tofu in its figures for a Chinese reader.
A controlled run confirms this is a path property, not a model one: driving the same 7 lectures through
initat v0.24.0 with bothclaude-sonnet-5andclaude-opus-5produced font config on 100% of delivered files and localised figure labels at 69% under both models. The rules work whenever they are passed; sync never passed them.What changes
Only files the run creates for the first time. An existing translation already carries its localisation and the translator prompts preserve it (#107), so the update path is untouched — there is a test pinning that
processFullis never reached for an existing file.The rebase path gets the same wiring, since it can re-create a file from scratch and that output needs localising for the same reason.
A new
localizeaction input mirrors the CLI's--localize: any comma-separated subset of the three rules, ornoneto opt out. An unrecognised rule name throws rather than falling back to defaults — a typo would otherwise ship a whole lecture unlocalised, and nothing downstream fails on that, which is exactly how this defect stayed invisible for months.Testing
src/__tests__/sync-localisation.test.tscovers: rules applied to a NEW file by default; target language named in the prompt; empty rule set passes no instructions; a partial rule set omits the unrequested rule; existing files never route through the full-document path.Full suite: 1449 passed, 64 suites.
npm run lintandnpm run format:checkclean.dist-action/rebuilt vianpm run buildas the committed bundle requires.Deliberately not in scope
_fonts/…vs an edition keeping fonts elsewhere) — that is init: i18n-font-config injection is nondeterministic (missed 5 of 37 lectures) and the font path is hard-coded #141, and it bitesinittoday just as much.localize: nonethat is a deliberate choice rather than a silent gap.🤖 Generated with Claude Code