Skip to content

check-translation-ownership.mjs accepts a missing --actor silently and then judges the PR anyway #116

Description

@os-elon

Found while fixing #113 (usage message for a missing --files), deliberately not fixed there: that card is scoped to how a misinvocation is reported, and this one changes a verdict.

The script's header documents the invocation as --actor LOGIN --files PATH, and .github/workflows/translations.yml passes both. The implementation, though, defaults the actor to the empty string:

constactor=(value('actor')??'').trim();

so omitting --actor is not an error. Measured on main (2f6494c), changed-list naming two locale artifacts:

$ TRANSLATION_BOT_LOGIN=transbot node .github/scripts/check-translation-ownership.mjs --files changed.txt
✗ translations are generated, not hand-written.
This PR edits 2 translation artifact(s):
content/docs/index.zh-Hans.mdx
content/docs/meta.ja.json
...
[exit 1]

An empty actor can never equal a non-empty TRANSLATION_BOT_LOGIN, so isBot is false and the run is judged as a human PR. A translation-account PR invoked without --actor is therefore rejected with a confident and wrong reason: the discriminator the entire check is built on was absent, and nothing said so. With the variable unset (main today) the actor is never read, so the omission is completely invisible — exit 0, same inert report as a correct invocation.

Why it was not folded into #113. Requiring --actor changes the exit status of node .github/scripts/check-translation-ownership.mjs --files changed.txt from 0 to 1, and today that invocation returns 0 through the inert path that #68 owns. That is a behaviour change rather than a diagnostics fix, and it wants a decision instead of a drive-by.

Options if it is taken up:

  • require --actor exactly the way --files is now required (usage message, exit 1) — matches the documented contract, and unreachable for the one real caller, which always passes it;
  • require it only when TRANSLATION_BOT_LOGIN is set — today's inert path is untouched, loud precisely when the value is load-bearing;
  • keep it optional and drop it from the documented usage line, i.e. declare "treat as human" the intended default.

Related, same surface, same species as #113 and also left alone: a --files path that does not exist is still reported as an uncaught ENOENT with a stack, because the read is unguarded:

$ node .github/scripts/check-translation-ownership.mjs --actor octocat --files nope.txt
node:fs:440
return binding.readFileUtf8(path, stringToFlags(options.flag));
^
Error: ENOENT: no such file or directory, open '/.../nope.txt'

That one is pure diagnostics and is a two-line follow-up to the usage-error class #113 introduces.

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions