Commit what the translation actually wrote, and refuse events with no base branch - #11
Merged
Merged
Conversation
add-paths: '.' committed the whole workspace, so anything the caller's job left dirty before us - a build, a codegen, a rewritten lockfile - shipped inside the translation PR. The paths are now derived from what the translation actually wrote: the config's output: entries, or the inline patterns. base: github.ref_name is NN/merge on a pull_request event, an internal ref rather than a branch, so the PR targeted a base that does not exist. The real base now comes from the event, with ref_name kept for push and schedule. project-dir joins the other inputs in env: instead of being interpolated. Self-test gains a check that fails if either regresses.
…se branch
The previous revision scoped the pull request by reading the config's
`output:` entries and turning `{{lang}}` into a glob. That source is
wrong: the CLI sends `output:` to the API as `output_file_path` and
unpacks the returned archive next to the SOURCE file, by basename. A
config whose `output:` points anywhere else produced a path list that
matched nothing - and `git add` dies on the first non-matching pathspec,
staging nothing at all, after which create-pull-request commits an empty
index and fails with the literal message "Unexpected error: ".
The list is now observed instead: the working tree is hashed before the
run and again afterwards, and what differs is what this run wrote. That
also covers what no config-derived list could - the `path:` entries of
`additional_translation_files:`, a monorepo where the CLI finds sources
at any depth, and a locale written for the first time.
Content, not timestamps. The obvious implementation - a marker file and
`test -nt` - drops translations silently: the CLI unpacks them from a
ZIP, ZIP keeps mtimes to two seconds, and unzip restores them from the
archive, so a translation written moments ago can be dated before the
run began. The act fixture caught it.
A run that wrote nothing now opens no pull request. It previously fell
back to `add-paths: '.'`, and an empty add-paths makes
create-pull-request run `git add -A` - the caller's whole repository.
Paths that cannot survive the trip are dropped with a message rather
than allowed to take the pull request down with them: create-pull-request
splits add-paths on /[\n,]+/ and trims each piece, so a newline, a comma
or edge whitespace in a file name becomes pathspecs that match nothing.
A name containing a glob metacharacter is passed as `:(literal)` - `git
add -- <path>` takes a pathspec, so `messages[1].json` would otherwise
stage the caller's `messages1.json` and not the translation.
`base:` returns to a plain `github.ref_name`, because a new first step
refuses the events where it is not a branch:
pull_request the run is on the merge ref, so the translations
cover strings that exist in neither branch alone;
pull_request_target write access to this repository while the
workspace can hold a fork's files;
merge_group the queue branch is deleted when the queue resolves;
a tag or release not a base branch at all.
It runs before the CLI, so a caller is told at once instead of paying
for a translation that cannot become a pull request. Reading the base
off the event instead would make these runs look supported while
proposing translations to a branch that never had the source strings,
and create-pull-request cherry-picks with `--strategy-option=theirs`, so
not even a conflict would stop it.
Tests: tests/translation-paths.test.sh covers the path list on plain
bash, in CI, with no docker; tests/act/ runs the whole action under act
against a mock API and a stub create-pull-request, covering a workspace
dirtied before the run, a config pointing elsewhere, a run that writes
nothing, the refusal, and the push path that must keep working.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two defects, both present since the first commit of this repository, plus what reviewing the first attempt at them turned up.
1. The translation pull request committed the whole workspace
add-paths: '.'told create-pull-request to stage everything dirty in the working directory — not only what we translated. Our step does not run in a clean room: whatever the caller's job did before us is sitting right there. A job that runs an install and a build before the action produced a PR titled "Update translations from PTC" that also carried the rewritten lockfile and the entire build output.The first attempt read the paths out of the config, and that source is wrong. The CLI never places files at
output:. It sends that value to the API asoutput_file_pathand unpacks the returned archive next to the source file, by basename. A config sayingfile: locales/en.jsonwithoutput: public/i18n/{{lang}}.jsonyieldedpublic/i18n/*.jsonwhile the translations sat inlocales/. Then one of two things happened:git adddied on the first non-matching pathspec and staged nothing, including the paths that were fine. create-pull-request ignores that exit code, commits an empty index, and fails with the literal messageUnexpected error:— stderr is empty, because git wrote the reason to stdout a hundred log lines earlier.The list is now observed: the working tree is hashed before the run and again after, and what differs is what this run wrote. That also picks up what no config-derived list could — the
path:entries ofadditional_translation_files:(the compiled.mocompanions), a monorepo where the CLI finds sources at any depth, and a locale written for the first time.Content, not timestamps. A marker file and
test -ntis the obvious implementation and it drops translations silently: the CLI unpacks from a ZIP, ZIP stores mtimes at two-second granularity, and unzip restores them from the archive — so a translation written moments ago can be dated before the run began. The act fixture caught this.A run that wrote nothing now opens no pull request. It used to fall back to
add-paths: '.', and an emptyadd-pathsmakes create-pull-request rungit add -A— the caller's entire repository.Paths that cannot survive the trip are dropped with a message instead of taking the pull request down with them. create-pull-request splits
add-pathson/[\n,]+/and trims each piece, so a newline, a comma or edge whitespace in a file name becomes pathspecs that match nothing. And a name containing a glob metacharacter is passed as:(literal):git add -- <path>takes a pathspec, so a translation written tomessages[1].json— an ordinary Next.js/Nuxt layout — would otherwise stage the caller'smessages1.jsonand not the translation, withgit addexiting 0.2. The pull request targeted a base branch that does not exist
base: ${{ github.ref_name }}is correct onpushandschedule. On apull_requestevent it isNN/merge, GitHub's internal merge ref, so create-pull-request'sgit fetch origin NN/merge:NN/mergecannot resolve it and the run fails — after the translation has been paid for.Reading the base off the event is not the fix. It makes the run pass while doing something worse: the translations were produced from the merge ref, so they cover strings that exist in neither the base nor the head branch on its own, and proposing them to the base branch offers translations for source strings that branch has never seen. create-pull-request cherry-picks with
--strategy-option=theirs, so not even a conflict stops it. Upstream's own guidance for this event isbase: ${{ github.head_ref }}, which needs a per-pull-requestpr-branch(an existing PR is matched on head and base) and, for forks, push access we do not have. That is a feature, not a fix.So
base:returns to a plaingithub.ref_name, and a new first step refuses the events where it is not a branch:pull_requestpull_request_targetmerge_groupreleaseIt runs before the CLI, so the caller is told at once rather than billed for a translation that cannot become a pull request.
pull_request_targetis the one configuration that worked before and is withdrawn deliberately — it is in the release note.Tests
Previously this had static assertions only — they pin the text of
action.ymland never run it.tests/translation-paths.test.sh— 19 cases on plain bash, no docker, runs in CI. Covers a workspace dirtied before the run, a file dated in the past, identical content in two files (locale files routinely share it, and it is what makes the comparison's collation matter), a glob metacharacter, a name starting with a double quote, an unusable snapshot, and the error paths.tests/act/— the whole action underact, against a mock PTC API and a stub create-pull-request. Five jobs: a workspace dirtied before the run, a config whoseoutput:points elsewhere, a run that writes nothing, the refusal onpull_request, and thepushpath that must keep working.Both were checked by reverting the fix in a scratch copy and confirming they fail.
shellchecknow covers what we wrote, not only the vendored CLI.Not covered here
Neither acceptance criterion has been exercised on a real GitHub runner — both need a caller's pipeline rather than a fixture.
actreproducesref_name=42/mergeand the whole path, but it is not GitHub. Worth checking on the demo repository once this is merged and a new tag is published.