Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mmcky merged 2 commits into from
Jul 23, 2026
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d6f8783
fix(review): honour glossary-path, and make a custom glossary an over…
mmcky Jul 23, 2026
7e39bfb
fix(review): unwrap error messages and validate the built-in glossary…
mmcky Jul 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
## [Unreleased]

### Fixed
- **Review mode honours `glossary-path`, and a custom glossary is now an override rather than a dead input** (#146): `runReview` read the built-in glossary file directly and never looked at `inputs.glossaryPath`, so on a repo with a custom glossary the translation was produced against one terminology and judged against another — silently, since the run logged `✓ Loaded glossary for <lang>` either way. That mattered more after v0.22.0 made `terminology` a **gating** category: a reviewer judging against the wrong glossary suppresses the auto-merge gate on correct work and biases the Stage 4 shadow calibration the floors will be chosen from. Review now goes through the same `loadGlossary` sync and rebase use. **Fixing it surfaced a deeper defect in that shared loader**, which #146 did not describe: the custom path was tried *after* the built-in one, so for every language that ships a glossary — `zh-cn`, `fa`, `fr`, i.e. the entire estate — `glossary-path` was silently ignored in **all three** modes, not just review; and an unreadable custom file quietly degraded to built-in terminology. A custom glossary is now an **override** (the only candidate when set), and a path that cannot be read or parsed **fails the run** instead of falling back to terminology the operator did not ask for. No production edition sets `glossary-path` today, so nothing in the fleet changes behaviour. Also fixed while in there: a repository whose name carries no language suffix reviewed with no glossary and said nothing — it now warns. The reviewer's glossary-to-prompt formatting moved out of `src/index.ts` (unreachable by the Jest CJS registry, so untestable) into `formatGlossaryTerms`, with tests.
- **`forward` and `init` find the built-in glossary regardless of working directory, and say which one they loaded** (#149): both commands 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 parse errors were swallowed by a bare `catch {}`, so a run that dropped terminology enforcement was indistinguishable from one that applied it; that unobservability was the worst of it, because it made the difference unauditable after the fact. Production signature matching the defect exactly: in `lecture-python.zh-cn`, the `init`-seeded lectures (QuantEcon/lecture-python.zh-cn#196) 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. Resolution now lives in one testable module (`src/cli/glossary-loader.ts`) 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 registry, so the entry point resolves it and the logic stays unit-testable). Precedence is `--glossary` → repo-local `glossary/<lang>.json` → built-in, and every outcome is reported: `✓ Loaded built-in glossary for zh-cn — 357 terms (…)` on success, a warning naming every path tried when a language has no glossary anywhere. **Loud on failure**: an explicit `--glossary` that is missing or malformed is a hard error rather than 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.

### Added
Expand Down
88 changes: 49 additions & 39 deletions dist-action/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,18 +205,18 @@ var require_file_command = __commonJS({
exports2.issueFileCommand = issueFileCommand;
exports2.prepareKeyValueMessage = prepareKeyValueMessage;
var crypto = __importStar(require("crypto"));
var fs5 = __importStar(require("fs"));
var fs4 = __importStar(require("fs"));
var os = __importStar(require("os"));
var utils_1 = require_utils();
function issueFileCommand(command, message) {
const filePath = process.env[`GITHUB_${command}`];
if (!filePath) {
throw new Error(`Unable to find environment variable for file command ${command}`);
}
if (!fs5.existsSync(filePath)) {
if (!fs4.existsSync(filePath)) {
throw new Error(`Missing file at path: ${filePath}`);
}
fs5.appendFileSync(filePath, `${(0, utils_1.toCommandValue)(message)}${os.EOL}`, {
fs4.appendFileSync(filePath, `${(0, utils_1.toCommandValue)(message)}${os.EOL}`, {
encoding: "utf8"
});
}
Expand Down Expand Up @@ -20353,21 +20353,21 @@ var require_io_util = __commonJS({
exports2.isRooted = isRooted;
exports2.tryGetExecutablePath = tryGetExecutablePath;
exports2.getCmdPath = getCmdPath;
var fs5 = __importStar(require("fs"));
var fs4 = __importStar(require("fs"));
var path6 = __importStar(require("path"));
_a2 = fs5.promises, exports2.chmod = _a2.chmod, exports2.copyFile = _a2.copyFile, exports2.lstat = _a2.lstat, exports2.mkdir = _a2.mkdir, exports2.open = _a2.open, exports2.readdir = _a2.readdir, exports2.rename = _a2.rename, exports2.rm = _a2.rm, exports2.rmdir = _a2.rmdir, exports2.stat = _a2.stat, exports2.symlink = _a2.symlink, exports2.unlink = _a2.unlink;
_a2 = fs4.promises, exports2.chmod = _a2.chmod, exports2.copyFile = _a2.copyFile, exports2.lstat = _a2.lstat, exports2.mkdir = _a2.mkdir, exports2.open = _a2.open, exports2.readdir = _a2.readdir, exports2.rename = _a2.rename, exports2.rm = _a2.rm, exports2.rmdir = _a2.rmdir, exports2.stat = _a2.stat, exports2.symlink = _a2.symlink, exports2.unlink = _a2.unlink;
exports2.IS_WINDOWS = process.platform === "win32";
function readlink(fsPath) {
return __awaiter(this, void 0, void 0, function* () {
const result = yield fs5.promises.readlink(fsPath);
const result = yield fs4.promises.readlink(fsPath);
if (exports2.IS_WINDOWS && !result.endsWith("\\")) {
return `${result}\\`;
}
return result;
});
}
exports2.UV_FS_O_EXLOCK = 268435456;
exports2.READONLY = fs5.constants.O_RDONLY;
exports2.READONLY = fs4.constants.O_RDONLY;
function exists(fsPath) {
return __awaiter(this, void 0, void 0, function* () {
try {
Expand Down Expand Up @@ -38104,31 +38104,46 @@ function stateFileRelativePath(filename) {

// dist/sync-orchestrator.js
async function loadGlossary(targetLanguage, builtInGlossaryDir, customGlossaryPath, logger) {
const builtInPath = path4.join(builtInGlossaryDir, `${targetLanguage}.json`);
try {
const content = await import_fs.promises.readFile(builtInPath, "utf-8");
const glossary = JSON.parse(content);
if (glossary) {
logger?.info(`\u2713 Loaded built-in glossary for ${targetLanguage} with ${glossary.terms.length} terms`);
return glossary;
}
} catch (error3) {
logger?.warning(`Could not load built-in glossary for ${targetLanguage}: ${error3}`);
}
if (customGlossaryPath) {
let glossary;
try {
const content = await import_fs.promises.readFile(customGlossaryPath, "utf-8");
const glossary = JSON.parse(content);
if (glossary) {
logger?.info(`\u2713 Loaded custom glossary from ${customGlossaryPath} with ${glossary.terms.length} terms`);
return glossary;
}
glossary = await readGlossaryFile(customGlossaryPath);
} catch (error3) {
logger?.warning(`Could not load custom glossary from ${customGlossaryPath}: ${error3}`);
throw new Error(`Could not load custom glossary from ${customGlossaryPath}: ${errorMessage(error3)}`);
}
logger?.info(`\u2713 Loaded custom glossary from ${customGlossaryPath} with ${glossary.terms.length} terms`);
return glossary;
}
const builtInPath = path4.join(builtInGlossaryDir, `${targetLanguage}.json`);
try {
const glossary = await readGlossaryFile(builtInPath);
logger?.info(`\u2713 Loaded built-in glossary for ${targetLanguage} with ${glossary.terms.length} terms`);
return glossary;
} catch (error3) {
logger?.warning(`Could not load built-in glossary for ${targetLanguage}: ${errorMessage(error3)}`);
}
return void 0;
}
function errorMessage(error3) {
const message = error3?.message;
return typeof message === "string" ? message : String(error3);
}
async function readGlossaryFile(file) {
const content = await import_fs.promises.readFile(file, "utf-8");
let glossary;
try {
glossary = JSON.parse(content);
} catch (error3) {
throw new Error(`${file} is not valid JSON: ${errorMessage(error3)}`);
}
if (!glossary || !Array.isArray(glossary.terms)) {
throw new Error(`${file} has no "terms" array`);
}
return glossary;
}
function formatGlossaryTerms(glossary, targetLanguage) {
return glossary.terms.map((t) => `- "${t.en}" \u2192 "${t[targetLanguage] || ""}"${t.context ? ` (${t.context})` : ""}`).join("\n");
}
function classifyChangedFiles(files, docsFolder) {
const prefix = docsFolder;
const changedMarkdownFiles = files.filter(
Expand Down Expand Up @@ -38211,9 +38226,9 @@ var SyncOrchestrator = class {
break;
}
} catch (error3) {
const errorMessage = `Error processing ${file.filename}: ${error3}`;
this.logger.error(errorMessage);
result.errors.push(errorMessage);
const errorMessage2 = `Error processing ${file.filename}: ${error3}`;
this.logger.error(errorMessage2);
result.errors.push(errorMessage2);
}
}
return result;
Expand Down Expand Up @@ -38428,7 +38443,6 @@ async function refreshStaleBranch(octokit, owner, repo, prNumber) {
}

// dist/index.js
var import_fs2 = require("fs");
var path5 = __toESM(require("path"), 1);
var import_url = require("url");
var __filename = (0, import_url.fileURLToPath)(__importMetaUrl);
Expand Down Expand Up @@ -38458,17 +38472,13 @@ async function runReview() {
let glossaryTerms;
const targetLanguage = detectTargetLanguage();
if (targetLanguage) {
const builtInGlossaryPath = path5.join(__dirname2, "..", "glossary", `${targetLanguage}.json`);
try {
const glossaryContent = await import_fs2.promises.readFile(builtInGlossaryPath, "utf-8");
const glossary = JSON.parse(glossaryContent);
if (glossary && glossary.terms) {
glossaryTerms = glossary.terms.map((t) => `- "${t.en}" \u2192 "${t[targetLanguage] || ""}"${t.context ? ` (${t.context})` : ""}`).join("\n");
core7.info(`\u2713 Loaded glossary for ${targetLanguage} with ${glossary.terms.length} terms`);
}
} catch (error3) {
core7.warning(`Could not load glossary for ${targetLanguage}: ${error3}`);
const builtInGlossaryDir = path5.join(__dirname2, "..", "glossary");
const glossary = await loadGlossary(targetLanguage, builtInGlossaryDir, inputs.glossaryPath || void 0, coreLogger);
if (glossary) {
glossaryTerms = formatGlossaryTerms(glossary, targetLanguage);
}
} else {
core7.warning(`Could not detect a target language from repository name '${github2.context.repo.repo}' \u2014 reviewing WITHOUT a glossary, so terminology findings are unreliable.`);
}
const result = await reviewer.reviewPR(prNumber, inputs.sourceRepo, github2.context.repo.owner, github2.context.repo.repo, inputs.docsFolder, glossaryTerms, targetLanguage, inputs.autoMergeMode);
core7.setOutput("review-verdict", result.verdict);
Expand Down
Loading
Loading