Skip to content

Remove failed-new-file TOC entries before opening sync PR - #289

Open
kp992 wants to merge 5 commits into
QuantEcon:mainfrom
kp992:issue-156
Open

Remove failed-new-file TOC entries before opening sync PR#289
kp992 wants to merge 5 commits into
QuantEcon:mainfrom
kp992:issue-156

Conversation

@kp992

@kp992 kp992 commented Aug 22, 2026

Copy link
Copy Markdown

When a sync run fails to translate a new lecture, the _toc.yml was committed with an entry for the absent file, producing a dangling reference that broke the target build and corrupted cross-references in every sibling lecture.

Changes: sync-orchestrator.ts (filtering logic + exported removeTocFileEntries), pr-creator.ts (PR body notice), index.ts (thread new field through). New unit tests cover flat/parts TOC layouts, the existing-file exemption, and the PR body section.

Fixes #156

@kp992

kp992 commented Aug 23, 2026

Copy link
Copy Markdown
Author

@mmcky Please review the files under src/ directory. dist-action/ was auto generated with npm run build to make the CI pass.

mmcky and others added 2 commits September 1, 2026 16:32
…notice (QuantEcon#156)

Review follow-up on the failed-new-file TOC filter:

- removeTocFileEntries no longer round-trips the TOC through yaml.dump.
  QuantEcon TOCs use the zero-indent block style js-yaml re-indents, so a
  one-entry removal became a whole-file rewrite (401 changed lines on the
  204-line lecture-python TOC). Entries are now spliced out of the original
  text — the `- file:` line plus any deeper-indented `title:`/`sections:`
  children — and the result is verified by parse against the filtered
  document before a noArrayIndent re-serialisation fallback is considered.
  Real-TOC check: exactly the removed lines differ, nothing else.
- Recursion covers `sections:` as well as `chapters:` at any depth.
- A new markdown file whose *source* fetch fails never reached the filter
  (it is in fetchErrors, not filesToSync). fetchAllFileContents now probes
  the target on that path — 404 means nothing will be delivered under the
  name, any other answer keeps the entry — and passes the list into
  processFiles so those entries leave the TOC too.
- The PR-body notice says what actually happened: names the TOC(s) the
  entries were removed from, or states that no TOC was in the changeset and
  a pre-existing target entry may still dangle. The unconditional "failure
  issue has been filed" is reworded; result.filteredTocPaths carries the
  distinction.
- js-yaml import moved beside the other local imports so the bundle diff is
  the change itself (+207/-9) rather than an esbuild module reshuffle.
- Tests: byte-identity on a QuantEcon-style fixture with comments, quoting
  and trailing whitespace; title/sections removal; nested sections; url/glob
  siblings; the fallback path; unparseable input; fetch-failed inputs;
  no-TOC-in-changeset; both PR-body wordings. Duplicate empty-set test
  removed. CHANGELOG and FAQ entries added.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@mmcky

mmcky commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Thanks @kp992 — the mechanism here is right and this closes the #156 field case: the new-vs-existing distinction is derived correctly (processedFiles is only pushed on success, isNewFile comes from the target 404), the filter sits at the one point a TOC can be emitted, and runRebase throws before touching the branch so only runSync needs the plumbing. I did a detailed review, and since maintainerCanModify was on I pushed the follow-ups straight to this branch rather than round-tripping them as comments — three commits on top of yours: f464a85 and 4d1f02d merge current main — the v0.27.0 release landed while I was working, so the only conflicts were the committed bundle and the CHANGELOG section header — and ac1bf6f carries the changes below. Please look them over; nothing you wrote was thrown away, and every original test still passes in its new form.

What the review found

# Finding Severity Status
1 yaml.dump rewrites the whole TOC. QuantEcon TOCs use the zero-indent block style (parts: / - caption:), which js-yaml re-indents on a round trip. Running the original filter over the live lecture-python.myst/lectures/_toc.yml (204 lines) to drop one entry changed 401 lines — the one-line removal a reviewer needs to see is buried in a full-file rewrite, and the next verbatim sync flips the formatting back. should-fix fixed in ac1bf6f
2 A new markdown file whose source fetch fails goes to fetchErrors and never enters filesToSync, so the filter cannot see it and the dangling entry still lands. (Translation failures — the actual field case — were covered; fetch failures were not.) should-fix fixed
3 The PR-body notice asserted "their _toc.yml entries have been removed" unconditionally, but failedNewFiles is set before checking whether any TOC is in translatedFiles. On a run that adds no TOC (the silent lecture-python.zh-cn#208 shape) the body claimed a removal that did not happen. should-fix fixed
4 Recursion descended chapters: but Jupyter Book's nested key under an entry is sections:. Latent — no QuantEcon TOC nests today. nit fixed
5 The +3865/−3772 dist-action diff was not a toolchain mismatch (same esbuild 0.27.7, lockfile unchanged): the new js-yaml import sat above ./translator.js, which moves the module in esbuild's emission order and reflows the bundle. nit fixed — bundle diff vs main is now +207/−9
6 "A failure issue has been filed on the source PR" was asserted unconditionally; createFailureIssue is skipped in test mode and swallows API errors. Two tests were duplicates (empty drop set / keeps all entries when drop set is empty). nit reworded; deduplicated

Confirmed correct and left as-is: the stem mapping for flat, parts and subdirectory TOC layouts; the url:/glob: sibling handling via the typeof e.file === 'string' guard; the optional trailing parameter so the other buildPrBody / createTranslationPR call sites are untouched; and translation-sync-metadata still declaring the failed file — that is desirable, a later rebase should retry it, and W1's declared-vs-delivered assertion (#259) is what makes a nine-declared/eight-delivered PR visible when no TOC is involved.

What changed in ac1bf6f

Byte-preserving removal. removeTocFileEntries no longer round-trips through yaml.dump. It splices the dropped entries' lines out of the original text — the - file: line plus any deeper-indented title: / sections: children — and then verifies the splice by parsing the result: the surviving file: list must equal what filtering the parsed document yields. Only if that check fails (a layout the splice does not recognise, e.g. a dash on its own line) does it fall back to filtering the parsed document and re-serialising with noArrayIndent. Same reference is returned on a no-op, as before. Validated against the real TOCs: on lecture-python.myst (202 lines, 141 files) dropping phillips_lost_conquest — the #156 case — removes exactly - file: phillips_lost_conquest, adds nothing, and parses to 140 files; dropping three entries at once removes exactly those three lines; on lecture-python.zh-cn it is a correct no-op because that lecture was never delivered there.

Fetch-failure coverage. fetchAllFileContents now probes the target when a markdown source fetch fails: a 404 means nothing will be delivered under that name, so the file is returned as fetchFailedNewFiles and passed into processFiles (new optional fifth argument), where it joins the drop set. Any other answer — rate limit, 5xx, network — keeps the entry, so an outage never removes a live one; the run still fails on the fetch error as before. Renamed files are deliberately not classified: dropping the new stem would orphan the old file from the toctree, which fails the strict build just the same, so that case stays a plain run failure (comment in the code says so).

Accurate PR-body notice. The result gains filteredTocPaths alongside failedNewFiles, and createTranslationPR / buildPrBody take a FailedNewFilesNotice { files, filteredTocPaths } object instead of a bare string array. The body now says which TOC(s) the entries were removed from, or — when no TOC was in the changeset — that nothing was removed and a pre-existing target entry may still dangle until the file arrives. The failure-issue sentence is reworded to not assert something the run may not have done.

Tests (suite 1,538 → 1,566, all green; lint and format:check clean): byte-identity on a QuantEcon-style fixture that carries a comment, a quoted stem and the trailing spaces the real files have; title + nested sections removed with their parent; a nested sections: entry removed under a chapter that stays; url: / glob: siblings untouched; the fallback path; unparseable input returned unchanged; collectTocFiles ordering; the orchestrator with fetch-failed inputs; filteredTocPaths empty when no TOC is in the run; and both PR-body wordings. The orchestrator tests now assert the TOC bytes, not just the parsed shape.

Docs. CHANGELOG entry under Unreleased → Fixed, and a FAQ entry explaining the "Files Failed to Translate" section and how to read its second sentence.

Two things worth knowing

  • Relationship to Preserve localised _toc.yml part captions on sync #291. Both PRs insert a new section after classifyChangedFiles and both import js-yaml; git auto-merges the two import lines into a duplicate identifier that tsc rejects, and the two section headers conflict trivially (keep both). Functionally they compose — Preserve localised _toc.yml part captions on sync #291 rewrites the TOC in processTocFile, this filter runs afterwards over translatedFiles — so whichever lands second is a two-hunk rebase plus one deleted import. This one should go first: it fixes a build-breaking production defect and needs no design choice. Preserve localised _toc.yml part captions on sync #291 has the same yaml.dump reformat problem, and collectTocFiles / the splice approach here are reusable there.
  • Scope. This is the "detection before repair" half for the loud case only. It stops the build break; it does not by itself make the drop visible when no TOC is in the changeset — that is W1's declared-vs-delivered box.

From my side this is ready to merge once you have had a look. If anything in the follow-up commit reads wrong to you, say so and I will adjust rather than you needing to rework it.

mmcky added a commit to kp992/action-translation that referenced this pull request Sep 1, 2026
…only target fetch (QuantEcon#254)

Review follow-up on the _toc.yml caption preservation:

- Matching by identical file set lost the caption of exactly the part a
  lecture-adding sync touches (measured on the real python/zh-cn pair: append
  one lecture to the first part and 基础工具 reverted to English). Parts are
  now paired by largest shared membership, each target part used once, ties
  to the nearest position. A part with no shared files keeps the source
  caption and is named in a warning; a matched part whose caption is already
  byte-identical to the source is logged as not localised. No positional
  guessing.
- The merge no longer round-trips through yaml.dump (201 of 204 lines
  changed on lecture-python's TOC, comments dropped, re-dumped even when
  nothing was preserved). Caption values are substituted into the source
  text and the result is verified by parse against the intended document;
  noArrayIndent re-serialisation is the fallback only.
- The rebase replay's `type: 'toc'` site in index.ts now fetches
  targetContent too, so a rebase no longer re-copies English captions.
- Both target-TOC fetches swallow only a 404; any other status fails the
  fetch instead of silently dropping the captions on a transient error.
- Code moved to src/toc-captions.ts (new module, new test file) so this PR
  and QuantEcon#289 — which inserts at the same point in sync-orchestrator.ts and
  adds the same js-yaml import — merge cleanly in either order.
- Tests: byte-identity on a QuantEcon-style fixture with a comment, quoted
  caption and trailing whitespace; gain/lose/reorder a lecture; parts moved;
  new part; no-overlap part not guessed; split part; target-only parts;
  never-localised no-op by reference; no parts; malformed target; quoting;
  commented-out caption line; the block-scalar fallback; matcher unit tests.
  Orchestrator test asserts bytes. CHANGELOG, FAQ and architecture map
  updated. Merged current main.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@kp992 kp992 left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks for the changes, @mmcky. LGTM

mmcky added a commit that referenced this pull request Sep 3, 2026
… v0.27.0 shipped), resume pointer

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
mmcky added a commit that referenced this pull request Sep 3, 2026
… v0.27.0 shipped), resume pointer (#302)

Co-authored-by: Claude Fable 5 <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 opens a PR whose _toc.yml references a lecture the same run failed to translate

2 participants