Serialise translation reviews to stop duplicate report comments#134
Merged
Conversation
Ports the fix from QuantEcon/lecture-python-programming.fr#7. This repo has the identical trigger list and no concurrency guard, and the bug is already reproducing here: PR #133 carries two "Translation Quality Review" comments, scoring 9.2/10 and 8.8/10, both posted at 23:51:37. A single translation sync fires `opened` plus one `labeled` event per label applied, all within a couple of seconds. Each starts its own run of this workflow, and the review action's "update existing comment, else create" logic is a check-then-act with no lock: concurrent runs all see "no comment yet" and each create one. On PR #133 three runs fired; two logged `Posted review comment` and one logged `Updated existing review comment`, leaving two independent reports of the same diff and three full model calls where one was intended. Add a per-PR concurrency group so runs queue instead of racing — the first creates the comment, any later run updates it. cancel-in-progress is left false on purpose: the labels are applied in one API call so event order is not guaranteed, and cancelling would let a `labeled` event for 'automated' kill the in-flight review and then skip its own job, leaving no review at all. Also ignore `labeled` events for labels other than 'action-translation', which removes the redundant review triggered by the 'automated' label. The underlying unsynchronised upsert is tracked in QuantEcon/action-translation#96. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
✅ Deploy Preview for majestic-griffin-10b166 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the Review Translations GitHub Actions workflow to prevent duplicate “Translation Quality Review” comments (and redundant model runs) by serializing workflow runs per PR and filtering out irrelevant labeled triggers.
Changes:
- Add a per-PR
concurrencygroup to ensure only one review run executes at a time for a given PR. - Refine the job-level
ifso only theaction-translationlabel’slabeledevent can trigger the review job (ignoring other labels likeautomated).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
What
Ports QuantEcon/lecture-python-programming.fr#7 (merged) to the Farsi edition: adds a per-PR
concurrencygroup toReview Translations, and stopslabeledevents for labels other thanaction-translationfrom triggering a review.Why
The bug is already reproducing in this repo. #133 — the open
numba.mdsync from source PR #550 — carries two "Translation Quality Review" comments, scoring 9.2/10 and 8.8/10, both posted at 23:51:37. Both are genuine model output: the same diff reviewed twice.The sync action creates the PR and then applies its labels in a separate call, so one sync fires
openedplus alabeledevent per label within a couple of seconds. Each starts its own run. The action's "update the existing comment, else create one" logic is a check-then-act with no lock, so concurrent runs all observe "no comment yet" and each create one. The run logs for #133 show exactly that:Posted review comment on PR #133Posted review comment on PR #133Updated existing review comment on PR #133Two runs won the create race; the third found a comment and updated it. Because each comment was then overwritten by a different run, neither necessarily reflects the run that created it.
Besides the duplicate report, this is a 3x model-spend multiplier on that PR, and it scales with the number of labels applied.
Why
cancel-in-progress: falseWorth reviewing carefully, because
trueis the reflex and would be wrong here. Both labels are applied in a singleaddLabelscall, so the order of the resultinglabeledevents is not guaranteed. Withtrue, anautomatedevent arriving last would cancel the in-flight review started byaction-translation, then skip its own job via the new label filter — leaving no review at all. Cancellation happens when the run is queued, before the job-levelifis evaluated, so the filter cannot protect against it.Queuing is safe in every ordering: the first run to reach the comment step creates the comment, any later run updates it.
Scope
This fixes the symptom at the workflow layer. The unsynchronised upsert is a latent bug in the action itself, tracked as QuantEcon/action-translation#96 — any two concurrent review runs will duplicate again until that lands.
lecture-python-programming.zh-cntriggers only on[opened, synchronize]and is much less exposed, so I have left it alone.Note this does not retroactively clean up #133, which still has both comments on it.
Verification
YAML parses, and the action inputs are unchanged (
source-language: en,target-language: fa, same action pin, same secrets) — the diff is the concurrency block plus theiffilter and comments, nothing else. The behaviour itself can only be confirmed on the next Farsi sync PR; worth watching that it produces exactly one review comment.🤖 Generated with Claude Code