Skip to content

🐛 Fix translation review never triggering on auto-labelled PRs#28465

Merged
9larsons merged 1 commit into
mainfrom
fix/i18n-review-trigger
Jun 9, 2026
Merged

🐛 Fix translation review never triggering on auto-labelled PRs#28465
9larsons merged 1 commit into
mainfrom
fix/i18n-review-trigger

Conversation

@9larsons

@9larsons 9larsons commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

no ref

Problem

The Translation Review workflow (translation-review.yml) triggered only on pull_request_target labeled / synchronize, gated on the affects:i18n label.

That label is applied automatically by the label-actions workflow using the default GITHUB_TOKEN. GitHub deliberately does not start new workflow runs from events triggered by GITHUB_TOKEN, so the labeled path has produced zero runs since the workflow shipped.

Reviews still appeared because i18n PRs almost always get a follow-up push (a translator iterating, or a "Merge branch 'main'" commit) which raises a synchronize event — and that's what actually ran the job. The bug stayed hidden until a PR was opened, auto-labelled, and never pushed to again (e.g. #28462), which got no review at all.

Evidence (PR #28434)

  • 10:23:49 affects:i18n added by github-actions[bot]no run
  • 13:32:07 push "Merge branch 'main'" → synchronize → run 13:32:14 → review 13:32:37
  • 14:24:38 push "Merge branch 'main'" → synchronize → run 14:24:45 → review 14:25:05

Every run that produced a review was synchronize; none fired at label-time. Same pattern across every i18n PR sampled.

Fix

Trigger on real PR-author events (opened / reopened / synchronize) gated by a paths filter on ghost/i18n/locales/**, instead of relying on the bot-applied label. These events come from the PR author (not GITHUB_TOKEN), so they aren't suppressed. labeled is kept only as a manual maintainer re-trigger (remove + re-add the label).

The review script remains the precise final gate — it only reviews ghost/i18n/locales/<locale>/<namespace>.json (excluding en/) and exits cheaply otherwise, so the broader paths glob costs at most a no-op checkout.

No change to the security posture: still pull_request_target checking out main (never the PR head), reading the diff via API only, with the existing file/line/timeout abuse guards intact.

SETUP.md updated to document the new triggers and the GITHUB_TOKEN rationale.

The Translation Review workflow triggered only on `pull_request_target`
`labeled`/`synchronize`. The `affects:i18n` label is applied automatically
by the label-actions workflow using the default `GITHUB_TOKEN`, and GitHub
does not start new workflow runs from events triggered by `GITHUB_TOKEN` —
so the `labeled` path produced zero runs. Reviews only ever fired when a
follow-up push happened to raise a `synchronize` event, which masked the
bug until a PR was opened, auto-labelled, and never pushed to again.

Trigger on real PR-author events (`opened`/`reopened`/`synchronize`) gated
by a `paths` filter on `ghost/i18n/locales/**` instead of relying on the
bot-applied label. `labeled` is kept only as a manual maintainer
re-trigger. The review script remains the precise final gate.
@coderabbitai

coderabbitai Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

The PR updates the Translation Review workflow to shift from label-driven triggering (affects:i18n label) to path-based triggering on changes to ghost/i18n/locales/** files. The workflow now triggers on PR author events (opened, reopened, synchronize) and manual re-runs via labeled, with a refined job condition that allows labeled events only when the label is exactly affects:i18n. Supporting documentation in SETUP.md explains the rationale for avoiding label-only triggering, clarifies the script's file-level filtering behavior, and updates guidance for retroactively reviewing existing labeled PRs.

Possibly related PRs

  • TryGhost/Ghost#27966: Added the translation review workflow and scripts that this PR now refines by switching from label-driven to path-based triggering.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed Title clearly identifies the bug being fixed and is directly related to the changeset, which updates the translation review workflow trigger mechanism.
Description check ✅ Passed Description comprehensively explains the problem, the root cause involving GITHUB_TOKEN suppression, the evidence of the bug, and the proposed fix with security considerations.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/i18n-review-trigger

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
.github/scripts/i18n-review/SETUP.md (1)

4-5: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Update stale label-driven wording in setup docs.

Line 4 and Line 144 still describe label-based behavior, but the workflow now primarily keys off paths + PR-author events. Renaming affects:i18n no longer disables normal runs, and the opening description should reflect path-based triggering.

Suggested doc patch
- that posts an advisory (non-approving) GitHub review on PRs labelled `affects:i18n`.
+ that posts an advisory (non-approving) GitHub review on PRs that touch locale files under `ghost/i18n/locales/**`.

- - Or simply rename the `affects:i18n` label — the conditional won't match
+ - Or comment out/disable the workflow trigger block in `translation-review.yml` (label renaming only affects the manual `labeled` re-trigger path)

Also applies to: 144-145

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/scripts/i18n-review/SETUP.md around lines 4 - 5, Update the stale
label-driven wording that claims runs are gated by the `affects:i18n` label and
that renaming it disables normal runs; instead state that the workflow is
triggered primarily by changed paths and PR-author events (e.g., path-based
triggers) and no longer relies on label presence to allow runs. Search for
occurrences of the literal label `affects:i18n` and the opening
description/top-level summary and the later paragraph that mentions disabling
runs, then replace those sentences to explain path-based triggering and that
renaming the label does not stop workflows from running. Ensure the new wording
briefly describes "paths" + PR-author events as the trigger mechanism and
removes any implication that removing/renaming the label disables workflow
execution.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In @.github/scripts/i18n-review/SETUP.md:
- Around line 4-5: Update the stale label-driven wording that claims runs are
gated by the `affects:i18n` label and that renaming it disables normal runs;
instead state that the workflow is triggered primarily by changed paths and
PR-author events (e.g., path-based triggers) and no longer relies on label
presence to allow runs. Search for occurrences of the literal label
`affects:i18n` and the opening description/top-level summary and the later
paragraph that mentions disabling runs, then replace those sentences to explain
path-based triggering and that renaming the label does not stop workflows from
running. Ensure the new wording briefly describes "paths" + PR-author events as
the trigger mechanism and removes any implication that removing/renaming the
label disables workflow execution.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 31b03982-61e7-474a-b851-3ea01f4d607b

📥 Commits

Reviewing files that changed from the base of the PR and between 31c1252 and 8bd25f2.

📒 Files selected for processing (2)
  • .github/scripts/i18n-review/SETUP.md
  • .github/workflows/translation-review.yml

@9larsons 9larsons enabled auto-merge (squash) June 9, 2026 22:58
@codecov

codecov Bot commented Jun 9, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.74%. Comparing base (31c1252) to head (8bd25f2).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #28465   +/-   ##
=======================================
  Coverage   73.74%   73.74%           
=======================================
  Files        1541     1541           
  Lines      132184   132184           
  Branches    15784    15784           
=======================================
  Hits        97484    97484           
  Misses      33733    33733           
  Partials      967      967           
Flag Coverage Δ
admin-tests 54.67% <ø> (ø)
e2e-tests 73.74% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@9larsons 9larsons merged commit c454a50 into main Jun 9, 2026
55 checks passed
@9larsons 9larsons deleted the fix/i18n-review-trigger branch June 9, 2026 23:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant