fix(review): honour glossary-path, and make a custom glossary an override (#146)

[mmcky]

Fixes #146.

What was wrong

runReview built its glossary by reading the built-in file directly and never looked at inputs.glossaryPath — which getReviewInputs() had been parsing all along and handing over unused. So on a repo with a custom glossary, the translation was produced against one terminology and judged against another, silently: the run logged ✓ Loaded glossary for <lang> with N terms either way, so the only symptom was puzzling terminology findings.

Since v0.22.0 that isn't just noise. terminology is a gating category — one minor finding blocks auto-merge — so a reviewer on the wrong glossary suppresses the gate on correct work, and biases the Stage 4 shadow calibration that the per-criterion floors will be chosen from.

The deeper defect underneath it

Fixing this meant routing review through the shared loadGlossary in sync-orchestrator.ts — the loader #146 describes as already doing it properly. It isn't. It tried the built-in glossary first and treated the custom path as a fallback:

// Try built-in glossary first  → returns here for zh-cn/fa/fr, always
// Fallback: try custom glossary path  → unreachable for any language that ships one

So glossary-path was silently dead in all three modes, not just review — for every language with a built-in glossary, which is the entire estate. And an unreadable custom file quietly degraded to built-in terminology rather than complaining. Both are the same silent-wrong-glossary class as #149.

That is a behaviour change beyond the issue's scope, so stating it plainly: a custom glossary is now an override (the only candidate when set), and a path that cannot be read or parsed fails the run. I checked every edition's workflows — no repo in the fleet sets glossary-path, and the issue says the same, so nothing changes behaviour in production today. The alternative — leaving custom-as-fallback and just pointing review at it — would have made review consistent with sync while leaving custom glossaries just as ignored, which fixes the symptom the issue names and not the thing that would bite the first edition to adopt one.

Also in this change

• A repository whose name carries no language suffix reviewed with no glossary at all and said nothing. detectTargetLanguage() returns undefined and the whole block was skipped in silence. It now warns that terminology findings are unreliable.
• The reviewer's glossary-to-prompt formatting moved out of src/index.ts into formatGlossaryTerms. src/index.ts uses import.meta.url, which the Jest CJS registry cannot load, so nothing in that file can be unit tested — the same trap src/rebase-siblings.ts documents, and the reason this formatting had no tests.

Verification

Full suite green: 54 suites, 1277 passing (6 pre-existing skips); lint and format clean. Nine tests over the loader and formatter, including the two that pin the new contract — custom beats built-in, and an unreadable custom path throws instead of falling back.

Not yet exercised on the harness. This and #149 will be tested together there before v0.23.0 is cut, which is the point of holding the tag.

Sequencing

Both halves of the glossary defect are now fixed — #149 (resync path resolved the glossary CWD-relative) and #146 (review path ignored it). NEXT-STEPS records both as needing to land before shadow mode, since each biases the calibration data Stage 4 depends on. #148 (diffChecks are model output but gate absolutely) is the remaining one on that list and is untouched here.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

This PR fixes review-mode glossary handling so review verdicts are computed against the same terminology configuration used during translation. It routes review glossary loading through the shared loadGlossary helper, changes glossary-path to behave as a strict override (and fail fast when unreadable), and extracts reviewer glossary prompt formatting into a testable helper.

Changes:

• Make glossary-path an override in the shared glossary loader and fail the run when a configured custom glossary can’t be read/parsed.
• Update review mode to use the shared glossary loader and shared prompt formatting, and warn when target language can’t be inferred from the repo name.
• Add unit tests for the updated loader contract and for glossary prompt formatting; update docs and changelog accordingly.

Reviewed changes

Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/sync-orchestrator.ts	Updates loadGlossary semantics (custom override + fail-fast) and adds formatGlossaryTerms helper for reviewer prompt formatting.
src/index.ts	Updates runReview to use loadGlossary(inputs.glossaryPath) and formatGlossaryTerms; adds warning when language suffix can’t be detected.
src/__tests__/sync-orchestrator.test.ts	Adds tests pinning the new glossary precedence/fail-fast behavior and tests for formatGlossaryTerms.
docs/user/glossary.md	Documents glossary-path as an override and highlights the need for consistent configuration across modes.
docs/user/action-reference.md	Updates glossary-path description to reflect override + fail-fast behavior across sync/rebase/review.
dist-action/index.js	Updates the built action bundle to include the new glossary loading/formatting behavior.
CHANGELOG.md	Adds an unreleased entry describing the glossary-path/review-mode fixes and related behavior change.

Comments suppressed due to low confidence (1)

src/sync-orchestrator.ts:153

• The built-in glossary branch doesn’t validate that glossary.terms is an array before logging glossary.terms.length and returning it. If the built-in JSON is malformed (or the schema changes), this can either throw later (e.g., in formatGlossaryTerms) or return an invalid Glossary object. Validate the schema here the same way the custom glossary path is validated, so failures are caught early with a clear message.

  try {
    const content = await fs.readFile(builtInPath, 'utf-8');
    const glossary: Glossary = JSON.parse(content);
    if (glossary) {
      logger?.info(
        `✓ Loaded built-in glossary for ${targetLanguage} with ${glossary.terms.length} terms`
      );
      return glossary;
    }

[mmcky]

Addressed Copilot's review in 7e39bfb — the one inline comment, plus the suppressed low-confidence note, which was the better of the two findings.

Inline (error interpolation) — valid, fixed; replied on the thread. The stated symptom was inaccurate ([object Object] can't arise from an Error), but the real one is worse: a doubled Error: prefix once run() re-wraps the message.

Suppressed note (built-in branch skipped the terms validation) — valid, and an asymmetry this PR introduced by hardening only the custom branch. The bad case is {"terms": {}}: it logged with undefined terms and returned the invalid object, which then failed downstream in formatGlossaryTerms or the translator, far from the cause. Both branches now share readGlossaryFile, so validation is identical while the consequence stays deliberately different — a bad custom path is operator misconfiguration and fails the run; a corrupt built-in glossary is a broken install and warns.

Worth recording from the fix: the error helper duck-types on .message rather than using this repo's error instanceof Error idiom, because fs.promises rejections cross a realm boundary under Jest's vm context and instanceof Error is false for them. The idiom would have silently fallen back to String(error) and re-added the prefix — the regression test caught precisely that against the first version of the fix.

54 suites, 1279 passing.