Skip to content

fix(cli): resolve the glossary package-relative and report the outcome (#149)#150

Merged
mmcky merged 1 commit into
mainfrom
fix/149-cli-glossary-resolution
Jul 23, 2026
Merged

fix(cli): resolve the glossary package-relative and report the outcome (#149)#150
mmcky merged 1 commit into
mainfrom
fix/149-cli-glossary-resolution

Conversation

@mmcky

@mmcky mmcky commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Fixes #149.

What was wrong

forward and init resolved their glossary candidates only against process.cwd(). The built-in glossaries ship inside this package and no edition repository carries one — I checked lecture-python.zh-cn, lecture-intro.zh-cn and lecture-python-programming.zh-cn, none has a glossary/ directory. So a resync launched from the target repo, a bench root, or anywhere a globally installed CLI is naturally invoked translated with no glossary at all. It logged nothing either way, and a bare catch {} swallowed parse errors, so a run that dropped terminology enforcement was byte-for-byte indistinguishable in its output from one that applied it. That unobservability is the worst of it — it made the difference unauditable after the fact, which is why #149 could only state the defect by construction rather than from the logs.

forward also had no --glossary escape hatch at all, so there was no way to override it even knowingly.

The fix

Resolution moves into one testable module, src/cli/glossary-loader.ts, shared by forward and init (both had the identical CWD-relative lookup, so both are fixed and the duplicate is gone). Precedence, reported on every run:

Order Candidate Behaviour
1 --glossary <path> The only candidate when given — missing or malformed is a hard error, never a silent fallback to different terminology
2 <cwd>/glossary/<lang>.json, <cwd>/glossary-<lang>.json Unchanged, so a project carrying its own glossary still overrides the estate defaults
3 <package-root>/glossary/<lang>.json New — resolved relative to the installed CLI, not to the working directory

Every outcome is logged: ✓ Loaded built-in glossary for zh-cn — 357 terms (…) on success, or a warning that names every path tried when a language has no glossary anywhere. A candidate that exists but does not parse is now an error rather than a fall-through to a different glossary — silently translating against terminology you did not ask for is the same class of invisibility.

Bulk resolves once before the first file and threads the result through, so the load is logged once rather than per-file, and a bad glossary stops the wave rather than surfacing after N files have already been resynced against nothing.

On the suggestion to make a glossary-less run an error

#149's point 4 asks whether a glossary-less run should be an error for a language that has a built-in glossary. With resolution now package-relative, that case is unreachable by constructionzh-cn, fa and fr always resolve. The remaining warning path is exactly the legitimate case: a language with no glossary anywhere. So the warning stays a warning, and the two genuinely-wrong cases (explicit path unusable, existing file malformed) are hard errors. Happy to tighten further if you would rather it refuse outright.

Where import.meta.url lives, and why

The package directory is resolved in src/cli/index.ts and threaded into the commands as an option, matching what src/index.ts already does for the Action. The Jest CJS module registry cannot load import.meta.url, so putting it in forward.ts would have made the whole command untestable — the same trap src/rebase-siblings.ts documents. The entry point resolves it; the resolution logic stays in a unit-testable module.

Verification

Built and run from /tmp, a directory with no glossary, against the harness repos:

$ cd /tmp && translate forward -s …/test-translation-sync -t …/test-translation-sync.zh-cn -f lecture.md --test
  Content changes detected — resyncing (whole-file)…
✓ Loaded built-in glossary for zh-cn — 357 terms (…/action-translation/glossary/zh-cn.json)

Before this change that run silently used no glossary. Also confirmed live: -l ja warns and lists all three paths tried; --glossary /no/such.json prints ❌ Glossary not found at /no/such.json (--glossary) and exits 1.

21 new tests across two files — CWD-independence (the regression itself), precedence including explicit-path exclusivity, all three throw paths, the reporting on both success and miss, and an integration test asserting what the translator actually receives on the resync path from a working directory with no glossary, including that the glossary handed over contains Marginal distribution, the term whose absence surfaced this in QuantEcon/lecture-python.zh-cn#198. Full suite: 54 suites, 1270 passing (6 pre-existing skips); lint and format clean.

Not in scope

🤖 Generated with Claude Code

#149)

`forward` and `init` resolved glossary candidates only against
`process.cwd()`. The built-in glossaries ship inside this package and no
edition repository carries one, so a resync launched from the target repo
— or a bench root, or anywhere a globally installed CLI is naturally
invoked — translated with no glossary at all. Nothing was logged either
way and a bare `catch {}` swallowed parse errors, so a run that dropped
terminology enforcement was indistinguishable from one that applied it.
That unobservability is the worst of it: it made the difference
unauditable after the fact.

The production signature matches exactly. In lecture-python.zh-cn the
`init`-seeded lectures all use the glossary's 边缘分布 for "Marginal
distribution", while the 2026-07-19 `forward` wave took prob_matrix.md
from 12 wrong / 28 correct to 25 wrong / 35 correct — newly generated
text ignoring a glossary entry that exists. See
QuantEcon/lecture-python.zh-cn#198 for the review finding.

Resolution now lives in one testable module shared by both commands, with
the packaged directory resolved relative to the installed CLI and threaded
in as an option — `import.meta.url` cannot be loaded by the Jest CJS
module registry, so the entry point resolves it and the logic stays unit
testable.

- precedence: --glossary → repo-local glossary/<lang>.json → built-in
- every outcome reported: the origin, term count and path on success; a
  warning naming every path tried when a language has no glossary anywhere
- loud on failure: a missing or malformed --glossary is a hard error, not
  a silent fallback to different terminology, and any candidate that
  exists but does not parse is an error rather than a fall-through
- bulk resolves once before the first file, so a bad glossary stops the
  wave instead of surfacing after N files have been resynced against
  nothing
- `forward --glossary <path>` added, matching `init` — the resync path
  previously had no override at all

21 new tests: CWD-independence, precedence, the throw paths, and what the
translator actually receives on the resync path from a working directory
with no glossary.

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

Copilot AI left a comment

Copy link
Copy Markdown

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 CLI glossary resolution so translate forward and translate init reliably load the packaged (built-in) glossary regardless of the current working directory, and always report what happened (including loud failures for missing/malformed explicit glossaries), addressing #149.

Changes:

  • Added a shared, testable CLI glossary resolver/loader (src/cli/glossary-loader.ts) with ordered precedence and explicit reporting.
  • Wired forward and init to use package-relative built-in glossaries and to log success/miss consistently; forward gains a --glossary <path> override.
  • Added focused unit + integration-style tests for resolution/precedence/reporting, and updated docs + changelog accordingly.

Reviewed changes

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

Show a summary per file
File Description
src/cli/types.ts Extends ForwardOptions to carry --glossary, built-in glossary dir, and an optional pre-resolved glossary for bulk runs.
src/cli/index.ts Resolves <package-root>/glossary via import.meta.url and threads it into forward/init; adds forward --glossary.
src/cli/glossary-loader.ts New module implementing ordered glossary resolution, strict parsing/validation, and consistent reporting.
src/cli/commands/init.ts Switches to shared loader and ensures the load outcome is reported (and failures are loud).
src/cli/commands/forward.ts Uses shared loader, adds a single-wave (bulk) pre-resolve path, and supports --glossary.
src/cli/tests/glossary-loader.test.ts New tests for CWD-independence, precedence, loud failure, and reporting behavior.
src/cli/tests/forward-glossary.test.ts New tests asserting the resync path actually passes the resolved glossary to the translator.
src/cli/tests/cli-smoke.test.ts Updates smoke expectations to include --glossary on forward.
docs/user/glossary.md Documents CLI resolution order, exclusivity of --glossary, and reporting behavior.
docs/user/cli-reference.md Updates CLI reference for forward --glossary and revised init glossary description.
CHANGELOG.md Records the fix and the new forward --glossary option under Unreleased.

@mmcky
mmcky merged commit f213598 into main Jul 23, 2026
2 checks passed
@mmcky
mmcky deleted the fix/149-cli-glossary-resolution branch July 23, 2026 06:03
@mmcky mmcky mentioned this pull request Jul 23, 2026
mmcky added a commit that referenced this pull request Jul 23, 2026
* chore: release v0.23.0

Bundles the three "fix before shadow mode" blockers, all harness-validated:

- #150 (#149) — forward/init resolved the glossary CWD-relative and no
  edition repo carries one, so a resync from the target repo translated
  with no glossary and logged nothing either way. Now package-relative,
  reported, loud on failure; `forward --glossary` added.

- #151 (#146) — review mode never read `glossary-path`, so a repo with a
  custom glossary was translated against one terminology and judged
  against another. Fixing it exposed that the shared loader tried the
  built-in glossary FIRST, so `glossary-path` was silently dead in all
  three modes for every language that ships one. Custom is now an
  override that fails the run when unreadable.

- #152 (#148) — the four diffChecks gate absolutely but were all model
  output. structurePreserved and headingMapCorrect are now computed;
  scopeCorrect and positionCorrect stay model-asserted, are tagged in a
  new optional `diffCheckSources` field, and gate through a `diff-check`
  finding. Routing is unchanged; shadow calibration can now separate
  measured fact from model opinion.

No behavioural change on upgrade for the fleet: no edition sets
`glossary-path`, `auto-merge-mode` still defaults to `off`, and the
deterministic checks were verified against every production file before
shipping (248 of 249 with sections carry a heading map; the one gap was
real and is fixed in lecture-python-programming.fa#142).

schemaVersion stays 1 — `diffCheckSources` is a pure optional addition.
Pre-flight: rebuilt dist-action matches the committed bundle.

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

* chore: re-emit the pull_request event (empty; squashed away on merge)

CI produced no run for 6439ad0 on either `opened` or `reopened`, and no
skipped-run record either. Ruled out first: paths-ignore (PR #145 fired
with an identical five-file set including .dev/STATE.md, on its own
release commit), Actions disabled, workflow inactive, draft PR, a
push/create race, skip-ci tokens, and workflow-file drift. This empty
commit tests whether a fresh head SHA is evaluated at all.

---------

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.

translate forward loads its glossary CWD-relative and fails silently — resyncs can run with no glossary and nothing says so

2 participants