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

[kp992]

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]

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

[mmcky]

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.

[kp992]

Thanks for the changes, @mmcky. LGTM

[mmcky]

Thanks @kp992. #291 merged first, so I brought this branch up to date and pushed two commits on top (ed6fc8e, 62c1c43):

• Merged main (with Preserve localised _toc.yml part captions on sync #291). src/ merged cleanly; the only conflict was the generated dist-action/index.js.map, which is rebuilt.
• CHANGELOG: the entry had landed in the released [0.29.1] [YANKED] section, and git merges it there without flagging a conflict. It now sits under [Unreleased], next to the Sync overwrites localised _toc.yml part captions with English, and nothing detects it #254 entry, so it ships with the next release. Against main the file differs by that one added line.
• FAQ: the _toc.yml caption answer (Sync overwrites localised _toc.yml part captions with English, and nothing detects it #254) and the failed-files answer here each said the rest of the TOC is byte-identical, measured from different baselines. Each now mentions the other change.
• New test: a lecture-adding sync whose new lecture fails, against a target TOC with translated part captions. The caption merge from Preserve localised _toc.yml part captions on sync #291 runs first and this PR's entry filter second, and together they hand back the target TOC byte for byte. Nothing tested the two together before.

Checks on 62c1c43, all passing: npm run build (no bundle drift, no local paths in the bundle), npm test (68 suites, 1,635 tests), npm run lint, npm run format:check, npm run check-dev-refs.

This goes into v0.29.4 alongside #291 and #335.

[Copilot]

🔵 Needs a closer look

It modifies the production sync pipeline's committed _toc.yml output with subtle text-splicing logic whose byte-preservation depends on YAML layout assumptions, warranting a human's final sign-off despite strong test coverage.

0 open findings

What changed in this PR

This PR fixes issue #156: when a sync run fails to deliver a new lecture (translation refused by the structural-parity guard, or source content that could not be fetched), the run previously still committed the source _toc.yml entry for that file, producing a dangling toctree reference that broke the target build under -n -W and corrupted cross-references in every sibling lecture that cited the missing file. The change reconciles the partial state after the per-file loop: it removes the undelivered new files' entries from any _toc.yml in the changeset (byte-preservingly) and surfaces the partial state on the PR body so reviewers understand why the PR is intentionally incomplete.

Changes:

• sync-orchestrator.ts: adds exported removeTocFileEntries/collectTocFiles helpers plus a post-loop filterTocForFailedNewFiles step that drops TOC entries for new files that errored in the loop or failed to fetch, using a parse-verified line splice with a noArrayIndent re-serialization fallback.
• index.ts: threads fetchFailedNewFiles out of fetchAllFileContents (classified via a new targetFileIsMissing 404 probe) into processFiles, and passes the resulting notice into PR creation.
• pr-creator.ts: adds a FailedNewFilesNotice and a "Files Failed to Translate" PR-body section stating exactly what happened to the TOC entries.

File	Description
src/​sync-orchestrator.ts	Core TOC-filtering logic and new result fields (failedNewFiles, filteredTocPaths).
src/​pr-creator.ts	New FailedNewFilesNotice type and PR-body notice threaded through createTranslationPR/buildPrBody.
src/​index.ts	Returns fetchFailedNewFiles, adds targetFileIsMissing 404 probe, threads the notice into PR creation.
src/​__tests__/​sync-orchestrator.test.ts	Unit tests for removeTocFileEntries/collectTocFiles and orchestrator TOC filtering (flat/parts/nested, fetch-failed, caption-merge).
src/​__tests__/​pr-creator.test.ts	Tests for the "Files Failed to Translate" PR-body section (TOC vs no-TOC wording).
docs/​user/​faq.md	New FAQ entry explaining the notice and an update to the caption FAQ.
dist-action/​index.js	Rebuilt action bundle matching the source changes.
CHANGELOG.md	[Unreleased] > Fixed entry documenting the #156 fix.

🧠 Review effort: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.