Review comments duplicate under concurrent runs — postReviewComment upsert is an unsynchronised check-then-act

[mmcky]

Observed on the first auto-generated French sync PR: QuantEcon/lecture-python-programming.fr#6 carries two independent "Translation Quality Review" comments, posted one second apart, with different scores (9.2/10 and 9.0/10) and different suggestion text. Both are real model output — the same diff reviewed twice.

Root cause

postReviewComment in src/reviewer.ts#L1062-L1089 is a check-then-act with no lock: it lists comments, looks for an existing review comment, then updates it or creates one. Nothing makes that sequence atomic, so concurrent runs all observe "no comment yet" and each create one.

That is exactly what the run logs show. Five Review Translations runs fired for this PR, and all five ran a full review:

Run	Outcome logged
29459720253	Posted review comment on PR #6 (23:50:42.96)
29459720018	Posted review comment on PR #6 (23:50:43.35)
29459719852	Updated existing review comment on PR #6
29459720427	Updated existing review comment on PR #6
29459719885	Updated existing review comment on PR #6

Two runs won the create race; the other three found a comment and updated it. Because each of the two comments was later overwritten by a different updating run, neither comment now reflects the run that created it — the visible scores are an arbitrary interleaving.

Why five runs

The target repo's review-translations.yml triggers on [opened, synchronize, labeled, reopened]. A sync creates the PR and then applies its labels in a separate addLabels call, so one sync produces opened plus a labeled event per label — five qualifying events within four seconds, five concurrent runs.

The workflow side is being fixed in QuantEcon/lecture-python-programming.fr#7 (per-PR concurrency group, plus ignoring labeled events for labels other than action-translation). That removes the race in practice for this repo, but the unsynchronised upsert is still a latent bug in the action: any two concurrent review runs — a synchronize landing alongside a labeled, or two editions' workflows misconfigured to share a PR — will duplicate again. .fa has the identical trigger list and no concurrency guard today.

Cost

Five full Sonnet reviews of the same diff where one was intended. This is a 5x per-PR multiplier on review spend, and it scales with the number of labels applied, right as the program fans out across editions.

Shape of the fix

The reliable primitive is a stable marker plus a conditional write, not a read-then-write. Options, roughly in order of robustness:

1. Search by hidden marker and tolerate the race. Embed an HTML comment marker (e.g. <!-- action-translation-review -->) and, after creating, re-list and delete any comment with the same marker whose id is greater than yours. Last-writer-wins converges to one comment.
2. Use a deterministic single-comment channel. Post the review as a PR review body rather than an issue comment, or key off a marker the action can update unconditionally.
3. At minimum, narrow the matching predicate. The current body.includes('Translation Quality Review') && body.includes('action-translation') is prose matching — a marker is cheaper and cannot be broken by rewording the template.

Note that a retry-on-conflict does not help here; there is no conflict to detect. The create succeeds for both runs.

Secondary observation (possibly #92 family)

GitHub recorded four labeled events for this PR — action-translation and automated each applied twice, four distinct event ids (28042533909, 28042533927, 28042534096, 28042534170) — while the sync run logged only a single successful Added labels: action-translation, automated at 23:50:11.87 and no retry attempt (Label attempt N/3 failed never appears). So labels were applied twice while the action believed it called once.

I could not establish the mechanism from the logs, and github.getOctokit is constructed without a retry plugin, so this is not a transparent client retry. Flagging it because it smells like the same "the API did the thing, then reported something else" family as #92, and because each spurious labeled event costs a full review run. Worth confirming before the label retry loop in src/pr-creator.ts#L196-L219 — which is precisely the naive, non-idempotent retry #92 warns about — ever does fire.