Skip to content

Backfill bibliography entries for citations a sync introduces - #226

Merged
mmcky merged 2 commits into
mainfrom
fix/117-bibliography-backfill
Jul 27, 2026
Merged

mmcky merged 2 commits into
mainfrom
fix/117-bibliography-backfill

Conversation

@mmcky

@mmcky mmcky commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Closes #117.

sync and forward translate {cite} roles, but the bibliography those keys resolve against is a shared file that never crossed the boundary. Under a strict build (-n -W) Sphinx promotes "could not find bibtex key" into a hard failure, so the target repo stops building on a file nobody edited.

The gap

src/sync-orchestrator.ts filters changed files with .endsWith('.md'), so a non-markdown file never enters the sync surface at all. More fundamentally, the engine had zero occurrences of .bib, bibtex or quant-econ anywhere in src/ — it had no bibliography concept. init only ever obtained one as a side effect of copying every non-markdown file wholesale ("Copied 108 non-markdown file(s)"), which is why seeded editions look fine and synced ones drift.

Field record: four lectures in one resync wave, then a 70-key backfill (see QuantEcon/lecture-python.zh-cn#203), then 21 more keys on 2026-07-27. Three manual repairs of a mechanism that did not exist.

The trigger is demand-driven, not diff-driven

The obvious design — "when the source PR touches the .bib, carry it" — misses the larger class. lecture-python-intro/lectures/msy_fishery.md cites five keys that lecture-intro.zh-cn's bibliography lacks, and no source PR touches the bib: the target simply has not translated that lecture yet. The moment it does, five citations dangle. So the trigger here is the set of keys a run introduces into the target, whatever the source diff happened to contain.

Safety model

A key that already resolves in the target is never a candidate, and appends never rewrite existing bytes. An edition that has localised a bibliography entry therefore cannot have it clobbered — that is structural, not a check that could be bypassed. The observation that the estate's bibliographies are currently strict subsets of their sources (480 vs 501 keys, 0 target-only) is not relied on anywhere in the design.

Ambiguity is reported, never guessed. A key declared twice in the source, or one differing from an existing target key only in case, fails the run rather than appending something that would produce an ambiguous reference. Keys that were already dangling before the run are surfaced as warnings and left alone — the run did not introduce them, and silently repairing pre-existing drift would hide it.

Outcomes

Situation Result
Introduced key resolves in target nothing happens
Introduced key resolves only in source entry appended, listed in the run log
Introduced key resolves nowhere run fails with file, line and nearest-key hint
Key declared twice in source, or case-collides in target run fails, nothing appended
Key already dangling before this run warning, left alone
Target configures no bibtex_bibfiles check does not apply
Target configures a bibliography that cannot be read run fails (fails closed)

Two measured details that drive the implementation

Indented entries. The entry parser tolerates leading whitespace because eight of the 501 entries in quant-econ.bib are indented. An ^@-anchored parser finds 493, would report those eight keys as missing, and would re-append them — producing exactly the duplicate-key -W failure this PR prevents. I hit this myself while measuring the estate before writing the fix, which is why there is a test named after it.

Citations inside admonitions. The citation walker is stack-based and selectively transparent: opaque inside {code-cell}, {raw}, {math} and bare fences, transparent inside {note}, {exercise}, {warning}, {prf:*}. Measured on lecture-python.myst: 71 of 922 citations (7.7%) live inside those containers — 65 in {note} alone. A flat skip-all-fences walker, like the one in structural-parity.ts, misses every one of them. The module header explains why all three fence walkers in this codebase differ deliberately.

Behaviour changes that turn previously-green runs red

Both are intentional, and both mean the target could not have built:

  • A sync introducing a citation key that resolves in neither bibliography now fails.
  • A target whose _config.yml names a bibtex_bibfiles entry that cannot be read now fails, rather than skipping the check silently.

The bibliography input selects backfill (default), lint (report without copying — the opt-out for editions that hand-author entries) or off. An unrecognised value fails the run, because a disabled guard looks exactly like a passing one.

Testing

src/__tests__/bibliography.test.ts — 33 cases covering mode parsing, the fence walker (including a code cell nested inside an admonition), indented entries, unterminated entries, duplicate keys, brace-nested field values, bibtex_bibfiles block and flow sequences, and every row of the outcomes table above.

Parser validated against the live estate before wiring: 501 source keys, 480 target keys, 21 source-only, 0 target-only, 0 duplicates, 0 unterminated — matching an independent measurement exactly. Citation extraction over all 139 source lectures finds 922 citations resolving to 356 distinct keys, with 0 unresolved against the source bibliography and 19 against the target.

Full suite: 1477 passed, 64 suites. npm run lint and npm run format:check clean. dist-action/ rebuilt on Node 24.

Deliberately not in scope

🤖 Generated with Claude Code

sync and forward translate {cite} roles, but the bibliography those keys
resolve against is a shared file that never crossed the boundary:
sync-orchestrator filters changed files to .endsWith('.md'), and the
engine had zero occurrences of .bib or bibtex anywhere in src/. It had no
bibliography concept at all — init only ever got one as a side effect of
copying every non-markdown file wholesale.

Under a strict build (-n -W) Sphinx promotes "could not find bibtex key"
to a hard failure, so the target stopped building on a file nobody
edited. Three manual repairs so far: four lectures in one resync wave,
then 70 keys (see QuantEcon/lecture-python.zh-cn#203), then 21 more on
2026-07-27.

The trigger is demand-driven, not diff-driven. lecture-python-intro's
msy_fishery.md cites five keys lecture-intro.zh-cn lacks and no source PR
touches the bib — the target simply has not translated that lecture yet.
A "did the source diff touch the .bib" design misses that class entirely.

Safety model: a key that already resolves in the target is never a
candidate, and appends never rewrite existing bytes, so an edition that
has localised an entry cannot have it clobbered. The observation that the
estate's bibs are currently strict subsets of their sources is not relied
on anywhere. Ambiguity is reported rather than guessed — a key declared
twice in the source, or differing from an existing target key only in
case, fails the run. Keys already dangling before the run are warnings,
not errors: the run did not introduce them.

Two measured details drive the implementation, both pinned by tests:

- The entry parser tolerates leading whitespace. Eight of the 501 entries
  in quant-econ.bib are indented; an ^@-anchored parser finds 493 and
  would re-append those eight, producing exactly the duplicate-key -W
  failure this prevents.
- The citation walker is stack-based and selectively transparent,
  descending into {note}/{exercise}/{warning}/{prf:*} while staying
  opaque inside code cells. 71 of 922 citations in lecture-python.myst
  (7.7%) live inside admonitions; a flat skip-all-fences walker misses
  every one.

New `bibliography` input selects backfill (default), lint or off. An
unrecognised value fails the run — a disabled guard looks exactly like a
passing one.

Refs #117

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

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

Adds a bibliography-aware post-translation step to prevent sync/rebase runs from introducing {cite} keys that don’t resolve in the target repo’s configured BibTeX files, which would otherwise break strict Sphinx/JupyterBook builds.

Changes:

  • Introduces src/bibliography.ts to extract {cite} keys, parse BibTeX entries, and plan/perform append-only backfills from source → target bibliographies.
  • Threads a new bibliography action input through inputs/types into SyncOrchestrator, and fetches bib sources/targets based on the target repo’s _config.yml bibtex_bibfiles.
  • Adds comprehensive Jest coverage plus documentation/architecture/changelog updates, and rebuilds dist-action/.

Reviewed changes

Copilot reviewed 11 out of 12 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/types.ts Adds bibliographyMode to action/rebase input types.
src/sync-orchestrator.ts Runs citation backfill planning after translation and records the plan in results.
src/inputs.ts Parses new bibliography input and normalizes it into bibliographyMode.
src/index.ts Fetches bibliography sources/targets during sync/rebase and adds fetchBibliographies helper.
src/bibliography.ts New module: citation extraction, BibTeX parsing, backfill planning, and reporting helpers.
src/tests/bibliography.test.ts Adds tests covering mode parsing, citation walker, BibTeX parsing edge cases, and planner outcomes.
docs/user/action-reference.md Documents the new bibliography input behavior and modes.
docs/developer/architecture.md Adds bibliography.ts to the module map.
dist-action/index.js Updates bundled action output for the new bibliography functionality.
CHANGELOG.md Documents the user-visible fix for missing bib entries across sync.
action.yml Adds the bibliography input with default backfill.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/index.ts
Comment thread src/bibliography.ts Outdated
Both from Copilot's review of #226.

1. `fetchBibliographies` swallowed every error from reading the target's
   `_config.yml` as "this edition has no bibliography". A rate limit, a
   5xx or a permissions failure therefore disabled the guard silently and
   the run went green having skipped the check — the exact failure shape
   this module exists to remove, and a contradiction of the fail-closed
   discipline applied to the bibliography read one function lower. Only a
   genuine 404 now means "not applicable"; every other status fails the
   run with the status in the message.

   The same bare catch guarded the source-side read. Copilot flagged one
   instance; the principle covers both. A transient failure there would
   have surfaced as a misleading "resolves in neither bibliography" error
   attributed to the wrong cause.

2. The module header quoted 45 of 872 citations (5.2%) inside non-code
   containers, from the design note. The figure measured against the
   current corpus is 71 of 922 (7.7%), which is what the tests and the PR
   body state. Corrected, anchored to the commit it was measured at, and
   marked indicative — nothing depends on its exact value; it is quoted
   to justify why the walker is stack-based rather than flat.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@mmcky
mmcky merged commit 2c3d624 into main Jul 27, 2026
1 check passed
@mmcky
mmcky deleted the fix/117-bibliography-backfill branch July 27, 2026 09:03
mmcky added a commit that referenced this pull request Jul 27, 2026
#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>
@mmcky mmcky mentioned this pull request Aug 4, 2026
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.

forward/sync don't carry shared assets referenced by lectures — quant-econ.bib and _admonition/* dangle and fail strict builds

2 participants