Serialise translation reviews to stop duplicate report comments#7
Merged
Conversation
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 #6 that produced five concurrent runs and two independent review comments with different scores (9.2 and 9.0), plus five 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>
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 review runs caused by multiple near-simultaneous pull_request events during automated sync PR creation.
Changes:
- Adds a per-PR
concurrencygroup to serialize translation review runs for the same PR. - Tightens the job-level
ifcondition so only theaction-translationlabel triggers a run forlabeledevents (ignoring other labels likeautomated).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mmcky
added a commit
to QuantEcon/lecture-python-programming.fa
that referenced
this pull request
Jul 16, 2026
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>
mmcky
added a commit
to QuantEcon/lecture-python-programming.fa
that referenced
this pull request
Jul 16, 2026
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>
mmcky
added a commit
to QuantEcon/action-translation
that referenced
this pull request
Jul 16, 2026
* fix(review): keep exactly one review comment under concurrent runs postReviewComment listed comments, looked for its own, then created one if absent — a check-then-act with nothing making it atomic. Concurrent runs all observed "no comment yet" and each created one. Concurrency here is routine, not exceptional: one sync fires `opened` plus a `labeled` event per label, so lecture-python-programming.fr#6 got five runs in four seconds, two surviving comments, and scores that matched no single review (each had been overwritten by a different run than the one that created it). Issue comments have no conditional-write primitive, so the fix reconciles instead of locking. Every comment carries a hidden marker (`<!-- action-translation-review -->`); after writing, a run deletes every marked comment with a lower id. Ids increase with creation time and each run lists after it writes, so the run holding the highest id always sees and removes the rest — one comment survives any interleaving. Note this is the mirror of the rule sketched in #96: deleting *newer* ids leaves duplicates whenever the winner lists before a slower run creates. Matching is anchored at the start of the body. The old prose predicate matched "Translation Quality Review" and "action-translation" anywhere in a comment, so it could adopt — and now would delete — a human comment quoting a review. Pre-marker comments still match by their generated heading, so the duplicates already on .fr#6 self-heal on its next review. Converging the comment does not converge the spend: each racing run still pays for a full review. That is workflow-side, and the trigger list #96 blames on the target repo is shipped by our own docs — connect-existing.md, which .fa also follows. All four review templates gain a per-PR `concurrency` group; the one triggering on `labeled` now ignores labels other than `action-translation` (`labeled` itself is load-bearing — labels land after the PR opens); and all declare `permissions` explicitly, since removing a duplicate needs `pull-requests: write`. Tests drive concurrent reviewers against a shared fake comments API; the natural await interleaving reproduces the race. Verified they fail against the pre-fix implementation (5 comments for 5 runs). Refs #96 Workflow-side fix in QuantEcon/lecture-python-programming.fr#7 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * chore(dev): track PR #98 in STATE; note the #96 sighting against #92 Leaves `verified:` at 2026-07-15 and the Health line describing main as-is — this only records the in-flight PR and the #92-family lead #96 turned up, it is not a full re-verification of the document. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mmcky
added a commit
that referenced
this pull request
Jul 16, 2026
* Pin to @v0 and adopt the upstream review-workflow template Two changes, both aligning this repo with action-translation's own documented conventions. Pin to @v0 on both workflows. The action's docs use @v0 throughout (19 occurrences against a single exact pin) and v0.17.0's notes recommend it directly. Exact pins were the deviation, and they cost us: v0.17.0's typography and review fixes sat unreachable behind nine stale pins while every sync kept stripping French non-breaking spaces. Development tempo is fast enough that manual bumps lag the releases that matter. The safety net is not the pin — sync output lands as a PR a human reviews before merge — and the floating tags are well maintained (v0 tracks v0.17.0; every vN.N tag tracks its latest patch). Adopt the upstream review template verbatim (docs/user/tutorials/ connect-existing.md), replacing the local variant from #7: - Add permissions: contents: read + pull-requests: write. v0.17.0's review dedupe deletes superseded comments, which needs pull-requests: write. Posting works today only because the default token happens to be permissive. - Move concurrency from workflow level to job level and flip cancel-in-progress to true. #7 argued true was unsafe, because an 'automated' labeled event would cancel the in-flight review then skip its own job, leaving none. That reasoning holds at workflow level, which is where #7 put it. At job level the group is entered only after the `if` passes, so a skipped job cancels nothing — and true is then better than false, superseding a stale in-flight review instead of queueing a redundant one. It also matches rebase-translations.yml, which has used job-level permissions + concurrency all along. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Correct the label-event comment: one addLabels call, one event per label Copilot caught this on #10. The comment claimed the sync applies labels 'one call at a time', but pr-creator.ts makes a single addLabels call with an array — it is GitHub that emits one `labeled` event per label, which is what multiplies the runs. Comment only; no behaviour change. The wording came verbatim from the upstream template (connect-existing.md), so the same inaccuracy is there and worth reporting back rather than silently diverging. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
Adds a per-PR
concurrencygroup toReview Translations, and stopslabeledevents for labels other thanaction-translationfrom triggering a review.Why
#6 — the first auto-generated French sync PR — carries two "Translation Quality Review" comments, posted one second apart, with different scores (9.2/10 and 9.0/10). Both are genuine model output: the same diff was reviewed twice.
The sync action creates the PR and then applies its labels in a separate call, so one sync produced five qualifying events within four seconds (
opened, plus alabeledevent per label). Each started its own run, and all five ran a full review. The action's "update the existing comment, else create one" logic is a check-then-act with no lock, so two runs saw "no comment yet" and both created one, while the other three found a comment and updated it:Posted review comment(23:50:42.96)Posted review comment(23:50:43.35)Updated existing review commentBesides the duplicate, this is a 5x cost multiplier — five full Sonnet reviews of one diff — which matters as the program fans out across editions.
Why
cancel-in-progress: falseThis is the part worth reviewing carefully, because
trueis the reflex and it would be wrong here.Both labels are applied in a single
addLabelscall, so the order of the resultinglabeledevents is not guaranteed. Withcancel-in-progress: true, 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 this.Queuing instead is safe in every ordering: the first run to reach the comment step creates it, and any later run updates it. One comment, always. The trade-off is that a rapid
synchronizewill review the intermediate commit before the latest one, which is acceptable for sync PRs that are pushed once.Scope and follow-ups
This fixes the symptom at the workflow layer for this repo only. The unsynchronised upsert is a latent bug in the action itself and is filed as QuantEcon/action-translation#96 — any two concurrent review runs will duplicate again until that lands.
lecture-python-programming.fahas the identical trigger list and no concurrency guard, so it has the same exposure and wants the same patch.lecture-python-programming.zh-cntriggers only on[opened, synchronize], so it is much less exposed.Verification
YAML parses and the job structure is unchanged (2 steps, same action pin, same inputs). The behaviour itself can only be confirmed on the next real sync PR — worth watching that it produces exactly one review comment.
🤖 Generated with Claude Code