From 55da026bfe10dce6d2780af35989aa02f9305a1e Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 19 Aug 2026 00:08:43 +0000 Subject: [PATCH 1/2] fix(scripts): require --actor when TRANSLATION_BOT_LOGIN is set check-translation-ownership.mjs documented `--actor ` but defaulted it to the empty string and judged the PR anyway. An empty actor can never equal a non-empty bot login, so a translation-account PR invoked without the flag was rejected as hand-written: a confident verdict reached with the discriminator the check rests on absent. The argument is now required exactly while TRANSLATION_BOT_LOGIN is set, which is the only condition under which it is read. With the variable unset the check stays inert and a missing --actor still exits 0, unchanged. Declared second change, same species: a --files path that does not exist is answered with the usage message instead of a raw ENOENT stack. Every other read failure still keeps its stack. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Yaqu7kmKZM3tRPd9Y4xivo --- .../scripts/check-translation-ownership.mjs | 50 ++++++++++++++++--- 1 file changed, 44 insertions(+), 6 deletions(-) diff --git a/.github/scripts/check-translation-ownership.mjs b/.github/scripts/check-translation-ownership.mjs index 22b5fc3..6eba556 100644 --- a/.github/scripts/check-translation-ownership.mjs +++ b/.github/scripts/check-translation-ownership.mjs @@ -27,6 +27,7 @@ * node .github/scripts/check-translation-ownership.mjs --actor --files * * is a file containing one changed path per line (`git diff --name-only`). + * is required whenever `TRANSLATION_BOT_LOGIN` is set — see `main()`. */ import { readFileSync } from 'node:fs'; import { join, dirname, resolve, relative } from 'node:path'; @@ -64,10 +65,10 @@ function isTranslationArtifact(path) { * * So argument validation throws this class and the entry point below prints the * message plus the usage line, no stack, exit 1. Everything else — an - * unparseable i18n.ts, an unreadable list file, a bug in here — is a genuine - * internal failure and keeps its stack, which is what a real fault needs. - * The exit status is unchanged in both cases: a misinvocation is still a - * failure, never a silent pass. + * unparseable i18n.ts, a list file that exists but cannot be read, a bug in + * here — is a genuine internal failure and keeps its stack, which is what a + * real fault needs. The exit status is unchanged in both cases: a + * misinvocation is still a failure, never a silent pass. */ class UsageError extends Error {} @@ -75,9 +76,29 @@ const USAGE = [ 'usage: node .github/scripts/check-translation-ownership.mjs --actor --files ', '', ' --actor PR author login (workflows pass github.event.pull_request.user.login)', + ' required whenever TRANSLATION_BOT_LOGIN is set', ' --files file listing one changed path per line (git diff --name-only)', ].join('\n'); +/** + * The changed-path list, with a path that is simply not there answered as the + * misinvocation it is. Naming a file that does not exist is the same class of + * mistake as omitting `--files` altogether, and it used to surface as a raw + * seven-line ENOENT stack — which reads as "this script is broken". Every + * other way the read can fail (a directory, a permission error, a bad mount) + * is a real fault and keeps its stack. + */ +function readChangedList(listFile) { + const path = resolve(ROOT, listFile); + try { + return readFileSync(path, 'utf8'); + } catch (error) { + if (error?.code !== 'ENOENT') throw error; + const where = path === listFile ? '' : ` (resolved to ${path})`; + throw new UsageError(`--files ${listFile} does not exist${where}`); + } +} + function main() { const argv = process.argv.slice(2); const value = (name) => { @@ -89,12 +110,29 @@ function main() { const listFile = value('files'); if (!listFile) throw new UsageError('--files is required'); - const changed = readFileSync(resolve(ROOT, listFile), 'utf8') + const botLogin = (process.env.TRANSLATION_BOT_LOGIN ?? '').trim(); + + // `--actor` is documented, but the implementation defaulted it to '' and + // then judged the PR anyway. An empty actor can never equal a non-empty + // bot login, so `isBot` is false and a translation-account PR invoked + // without the flag is rejected as a hand-written one: a confident verdict + // reached with the discriminator the whole check rests on absent, and + // nothing saying so. + // + // Required exactly while the bot login is set, which is the only condition + // under which the actor is read at all. With it unset the check is inert by + // design — it reports and passes so this could land before the account + // existed — the actor is never consulted, and demanding an argument nothing + // reads would be noise on an invocation whose meaning is settled elsewhere. + if (botLogin && !actor) { + throw new UsageError('--actor is required when TRANSLATION_BOT_LOGIN is set'); + } + + const changed = readChangedList(listFile) .split('\n') .map((s) => s.trim()) .filter(Boolean); - const botLogin = (process.env.TRANSLATION_BOT_LOGIN ?? '').trim(); const isBot = botLogin !== '' && actor.toLowerCase() === botLogin.toLowerCase(); const artifacts = changed.filter(isTranslationArtifact); From 1778d4a2d92f3c781c6bfcfed5179abae3b5de5b Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 19 Aug 2026 00:18:26 +0000 Subject: [PATCH 2/2] fix(scripts): require --actor unconditionally, per the triage adjudication MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rework of the first commit on this branch, which implemented the conditional form. The governing ruling on the card is the triage adjudication: `--actor` is required exactly the way `--files` is, one usage-error convention for the file. The actor is the discriminator the check is built on and the header documents it as part of the invocation, yet it defaulted to the empty string and the PR was judged anyway — an empty actor can never equal a non-empty bot login, so a translation-account PR invoked without the flag came back rejected as hand-written. Declared, therefore enforced. This answers a malformed invocation, not the meaning of the inert path: a well-formed call with TRANSLATION_BOT_LOGIN unset still reports and passes. Declared second change, unchanged from the first round: a --files path that does not exist is answered with the usage message instead of a raw ENOENT stack, narrowed to ENOENT so every other read failure keeps its stack. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Yaqu7kmKZM3tRPd9Y4xivo --- .../scripts/check-translation-ownership.mjs | 31 ++++++++----------- 1 file changed, 13 insertions(+), 18 deletions(-) diff --git a/.github/scripts/check-translation-ownership.mjs b/.github/scripts/check-translation-ownership.mjs index 6eba556..bb35176 100644 --- a/.github/scripts/check-translation-ownership.mjs +++ b/.github/scripts/check-translation-ownership.mjs @@ -27,7 +27,7 @@ * node .github/scripts/check-translation-ownership.mjs --actor --files * * is a file containing one changed path per line (`git diff --name-only`). - * is required whenever `TRANSLATION_BOT_LOGIN` is set — see `main()`. + * Both arguments are required; see `main()` for why the actor is not optional. */ import { readFileSync } from 'node:fs'; import { join, dirname, resolve, relative } from 'node:path'; @@ -76,7 +76,6 @@ const USAGE = [ 'usage: node .github/scripts/check-translation-ownership.mjs --actor --files ', '', ' --actor PR author login (workflows pass github.event.pull_request.user.login)', - ' required whenever TRANSLATION_BOT_LOGIN is set', ' --files file listing one changed path per line (git diff --name-only)', ].join('\n'); @@ -110,29 +109,25 @@ function main() { const listFile = value('files'); if (!listFile) throw new UsageError('--files is required'); - const botLogin = (process.env.TRANSLATION_BOT_LOGIN ?? '').trim(); - - // `--actor` is documented, but the implementation defaulted it to '' and - // then judged the PR anyway. An empty actor can never equal a non-empty - // bot login, so `isBot` is false and a translation-account PR invoked - // without the flag is rejected as a hand-written one: a confident verdict - // reached with the discriminator the whole check rests on absent, and - // nothing saying so. + // The actor is the discriminator this whole check is built on, and the + // header documents it as part of the invocation — so it is enforced the way + // `--files` is. The implementation used to default it to '' and judge the PR + // anyway: an empty actor can never equal a non-empty bot login, so `isBot` + // was false and a translation-account PR invoked without the flag came back + // rejected as hand-written — a confident verdict reached with the + // discriminator absent, and nothing saying so. Declared, therefore enforced. // - // Required exactly while the bot login is set, which is the only condition - // under which the actor is read at all. With it unset the check is inert by - // design — it reports and passes so this could land before the account - // existed — the actor is never consulted, and demanding an argument nothing - // reads would be noise on an invocation whose meaning is settled elsewhere. - if (botLogin && !actor) { - throw new UsageError('--actor is required when TRANSLATION_BOT_LOGIN is set'); - } + // This answers a malformed invocation, not the question of what the check + // means once it is well formed: a call that passes both arguments with + // `TRANSLATION_BOT_LOGIN` unset still reports and passes, untouched. + if (!actor) throw new UsageError('--actor is required'); const changed = readChangedList(listFile) .split('\n') .map((s) => s.trim()) .filter(Boolean); + const botLogin = (process.env.TRANSLATION_BOT_LOGIN ?? '').trim(); const isBot = botLogin !== '' && actor.toLowerCase() === botLogin.toLowerCase(); const artifacts = changed.filter(isTranslationArtifact);