fix(review): keep exactly one review comment under concurrent runs (#98)

* 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>

Author: mmcky

.dev/STATE.md

@@ -7,6 +7,10 @@ Roadmap detail lives in [PLAN.md](PLAN.md), not here.
 
 ## In flight
 
+- **PR #98** — #96: duplicate review comments under concurrent runs (unsynchronised upsert).
+  Marker + delete-older-ids reconciliation; also adds the `concurrency` group missing from
+  all four review workflow templates, which is where `.fr`/`.fa` inherited the 5x review
+  spend. Decision: `decisions/D-2026-07-16-single-review-comment.md`.
 - **PR #78** — fr programming-domain glossary terms; awaiting native review.
 - **PR #71** — Malayalam (`ml`) draft; awaiting native-reviewer calibration batch.
   Glossary PR **#69** (ja) open, awaiting native review + a `LANGUAGE_CONFIGS` entry.
@@ -53,7 +57,9 @@ Roadmap detail lives in [PLAN.md](PLAN.md), not here.
   Phase 2 round-trip test would catch three of them as a class, so Phase 2 first.
 - Smaller review-round follow-ups: **#91** (heading-maps.md documents a key format the action
   has never written — nearly caused a bad rubric fix), **#92** (PR creation reports failure
-  when the API times out *after* succeeding; naive retry would duplicate).
+  when the API times out *after* succeeding; naive retry would duplicate — #96 flags a
+  possible second sighting: four `labeled` events recorded for two `addLabels` calls,
+  unreproduced, mechanism unknown).
 - Earlier review-round issues: **#81** (typography on sync path), **#82** (model-swap
   eval — see REVIEW §7.4 for a concrete deterministic design).
 

.dev/decisions/D-2026-07-16-single-review-comment.md

@@ -0,0 +1,41 @@
+# One review comment per PR: hidden marker + delete-older reconciliation
+
+**Context**: `postReviewComment` was a check-then-act (list → find → update-or-create) with no
+lock, so concurrent review runs each observed "no comment yet" and each created a comment
+(#96). Concurrency is routine, not exceptional: one sync fires `opened` plus a `labeled` event
+per label, which is five qualifying events in four seconds on `lecture-python-programming.fr#6`.
+Issue comments have no conditional-write primitive (no ETag/If-Match, no unique key), so the
+race cannot be closed by retrying — both creates genuinely succeed.
+
+**Decision**: Every review comment carries a hidden `<!-- action-translation-review -->` marker
+on its first line. After writing (create *or* update), a run lists again and deletes every
+marked comment with an id **lower** than the one it wrote — newest wins.
+
+Delete-older converges; delete-newer does not. Ids increase with creation time, and each run
+lists strictly after it writes. So for any two of our comments `ci < cj`, the run owning `cj`
+listed after `cj` was created, hence after `ci` was created, hence it sees and deletes `ci`.
+Only the highest id survives, whatever the interleaving. The mirror rule #96 suggested
+("delete ids greater than yours", keeping the oldest) leaves duplicates whenever the winner
+lists before a slower run creates — the exact interleaving that produced the bug.
+
+Matching is anchored at the *start* of the body (marker, or the generated `## … Translation
+Quality Review` heading for pre-marker comments). These comments get deleted, so a loose
+predicate destroys data: the old prose match — "Translation Quality Review" and
+"action-translation" appearing anywhere — would match a human comment quoting a review.
+
+**Consequences**:
+
+- Exactly one review comment per PR under any concurrency. The common path (re-review after a
+  push) still updates in place, so the comment keeps its position in the thread.
+- Duplicates from v0.16.1 and earlier are marker-less; the legacy heading match adopts the
+  newest as ours and deletes the rest, so affected PRs self-heal on their next review.
+- Costs one extra `listComments` per review, and needs `pull-requests: write` to delete.
+  Deletes are best-effort — a failed delete leaves a duplicate, which is not worth failing a
+  review that posted successfully.
+- Converging the comment does **not** converge the spend: every racing run still pays for a
+  full review. That is a workflow-side problem, fixed by the `concurrency` group and the
+  `labeled` guard now in the review templates.
+
+**Refs**: QuantEcon/action-translation#96 · workflow-side fix
+QuantEcon/lecture-python-programming.fr#7 · tests in `src/__tests__/reviewer-comment.test.ts`
+drive concurrent reviewers against a shared fake comments API.

.dev/log/2026-07-16-dupcomment.md

@@ -0,0 +1,30 @@
+# 2026-07-16 — #96: duplicate review comments under concurrent runs
+
+Fixed the unsynchronised check-then-act in `postReviewComment` (#96, observed on
+`lecture-python-programming.fr#6`: two review comments a second apart, each overwritten by a
+different run). Hidden `<!-- action-translation-review -->` marker + delete-older-ids
+reconciliation after every write; decision and the convergence argument in
+`decisions/D-2026-07-16-single-review-comment.md`. Note the rule is the *mirror* of the one
+#96 sketched — deleting **newer** ids doesn't converge, deleting older ones provably does.
+
+Tests (`src/__tests__/reviewer-comment.test.ts`) drive N reviewers against a shared in-memory
+comments API; the natural `await` interleaving reproduces the all-list-then-all-create race
+without any scheduling tricks. Confirmed the five-run test fails on the pre-fix code (5
+comments) before keeping it — 5 of 16 fail against the old implementation.
+
+Found while there: the `[opened, synchronize, labeled, reopened]` trigger list #96 blames on
+the target repo is **shipped by our own docs** (`connect-existing.md`), and no review template
+had a `concurrency` group — so every edition set up from the tutorials inherits the 5x review
+spend, `.fa` included. Added the per-PR `concurrency` group to all four review templates, the
+`github.event.label.name` guard to the one that triggers on `labeled`, and explicit
+`permissions` (the delete needs `pull-requests: write`). `labeled` itself is load-bearing —
+labels are applied after the PR opens, so dropping it would skip reviews entirely.
+
+#promote: docs that ship a workflow template make workflow bugs a *fleet* problem — the
+target-repo fix (lecture-python-programming.fr#7) doesn't stop the next edition inheriting it.
+Worth checking other QuantEcon action repos whose READMEs carry copy-paste workflows.
+
+Not addressed: #96's secondary observation (four `labeled` events recorded for two
+`addLabels` calls, #92 family). Unreproduced, mechanism unknown from the logs; the
+`concurrency` group makes it harmless for now, but the naive label retry loop in
+`pr-creator.ts` is still the #92-shaped hazard if it ever fires.

CHANGELOG.md

@@ -7,7 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
-## [0.16.1] - 2026-07-15
+### Fixed
+- **Concurrent review runs no longer post duplicate review comments**: `postReviewComment` listed comments, looked for its own, then created one if absent — a check-then-act with nothing making it atomic, so every concurrent run saw "no comment yet" and created one. Observed on `lecture-python-programming.fr#6`, which carried two "Translation Quality Review" comments a second apart, each subsequently overwritten by a *different* run, leaving scores that matched no single review. Review comments now carry a hidden `<!-- action-translation-review -->` marker, and each run deletes any older marked comment after writing its own. Because ids increase with creation time and every run lists after it writes, the run holding the highest id always sees and removes the rest: exactly one comment survives any interleaving. Pre-existing duplicates (from v0.16.1 and earlier, which have no marker) are cleaned up on the next review of that PR.
+
+### Changed
+- **Review comments are identified by marker, not prose**: the old predicate matched any comment containing both "Translation Quality Review" and "action-translation" anywhere in its body — it could match, and overwrite, a human comment quoting a review. Matching is now anchored at the start of the body, so quoted or reposted reviews are never touched.
+- **Review workflow templates gained a `concurrency` group** (docs): the action-side fix converges the *comment*, but each racing run still pays for a full review. A per-PR `concurrency` group with `cancel-in-progress` collapses the `opened` and `labeled` events a single sync produces into one review. The `connect-existing` template additionally ignores `labeled` events for labels other than `action-translation` — it triggers on `labeled` (necessary, since labels are applied after the PR is opened), and a sync applying two labels was starting a full review per label. Templates now also declare `permissions` explicitly (`pull-requests: write`, required to remove duplicate comments). Existing target repos should copy these guards; without them a sync bills several reviews of the same diff.
 
 ### Fixed
 - **French typography no longer corrupts footnote/link-reference definitions**: the NBSP pass rewrote `[^id]: text` as `[^id] : text`, which stops the line parsing as a definition — it rendered as literal text and broke every reference (shipped in the fr seed, e.g. `pandas.md`). Definition labels are now masked, and the exact corruption is repaired on contact, so running `scripts/typography/apply.mjs` over an affected repo heals it. The definition text after the colon is still typeset.

dist-action/index.js

@@ -29038,6 +29038,19 @@ var REVIEW_RETRY_CONFIG = {
   baseDelayMs: 1e3
   // 1s, 2s, 4s with exponential backoff
 };
+var REVIEW_COMMENT_MARKER = "<!-- action-translation-review -->";
+var LEGACY_REVIEW_HEADING = /^#{2} .*Translation Quality Review/;
+var REVIEW_COMMENT_UPSERT_ATTEMPTS = 3;
+function isActionReviewComment(body) {
+  if (!body)
+    return false;
+  if (body.startsWith(REVIEW_COMMENT_MARKER))
+    return true;
+  return LEGACY_REVIEW_HEADING.test(body) && body.includes("action-translation");
+}
+function isNotFoundError(error3) {
+  return typeof error3 === "object" && error3 !== null && error3.status === 404;
+}
 function parseJsonResponse(text) {
   let parsed;
   try {
@@ -29771,32 +29784,90 @@ ${diffResult.issues.map((i) => `- ${i}`).join("\n")}`;
     return comment;
   }
   /**
-   * Post review comment on PR
+   * Our review comments on the PR, oldest first.
+   *
+   * Paginated: beyond 30 comments the existing one is missed and duplicates accumulate.
+   */
+  async listOwnReviewComments(prNumber, owner, repo) {
+    const comments = await this.octokit.paginate(this.octokit.rest.issues.listComments, {
+      owner,
+      repo,
+      issue_number: prNumber,
+      per_page: 100
+    });
+    return comments.filter((c) => isActionReviewComment(c.body)).sort((a, b) => a.id - b.id);
+  }
+  /**
+   * Delete our review comments older than `keepId` — duplicates left by concurrent runs.
+   *
+   * Best effort: a duplicate comment is not worth failing a review that posted successfully.
+   */
+  async deleteOlderReviewComments(prNumber, owner, repo, keepId) {
+    let duplicates;
+    try {
+      duplicates = (await this.listOwnReviewComments(prNumber, owner, repo)).filter((c) => c.id < keepId);
+    } catch (error3) {
+      core2.warning(`Could not check PR #${prNumber} for duplicate review comments: ${error3}`);
+      return;
+    }
+    for (const duplicate of duplicates) {
+      try {
+        await this.octokit.rest.issues.deleteComment({ owner, repo, comment_id: duplicate.id });
+        core2.info(`Removed duplicate review comment ${duplicate.id} on PR #${prNumber}`);
+      } catch (error3) {
+        if (isNotFoundError(error3))
+          continue;
+        core2.warning(`Could not remove duplicate review comment ${duplicate.id} on PR #${prNumber}: ${error3}`);
+      }
+    }
+  }
+  /**
+   * Post the review, leaving exactly one review comment on the PR.
+   *
+   * Concurrent review runs are routine — one sync fires `opened` plus a `labeled` event per
+   * label — and list-then-create is a check-then-act race: every run sees "no comment yet"
+   * and creates one (issue #96). Issue comments have no conditional-write primitive, so each
+   * run instead reconciles after writing, deleting every *older* review comment of ours.
+   * Ids increase with creation time and each run lists after it writes, so the run holding the
+   * highest id necessarily sees the others and removes them: one comment survives any
+   * interleaving. (Deleting *newer* ids instead would not converge — a run that lists before
+   * a slower run creates would leave both.)
    */
   async postReviewComment(prNumber, owner, repo, comment) {
+    const body = `${REVIEW_COMMENT_MARKER}
+${comment}`;
     try {
-      const comments = await this.octokit.paginate(this.octokit.rest.issues.listComments, {
-        owner,
-        repo,
-        issue_number: prNumber
-      });
-      const existingComment = comments.find((c) => c.body?.includes("Translation Quality Review") && c.body?.includes("action-translation"));
-      if (existingComment) {
-        await this.octokit.rest.issues.updateComment({
-          owner,
-          repo,
-          comment_id: existingComment.id,
-          body: comment
-        });
+      for (let attempt = 1; attempt <= REVIEW_COMMENT_UPSERT_ATTEMPTS; attempt++) {
+        const existing = await this.listOwnReviewComments(prNumber, owner, repo);
+        if (existing.length === 0) {
+          const { data: created } = await this.octokit.rest.issues.createComment({
+            owner,
+            repo,
+            issue_number: prNumber,
+            body
+          });
+          core2.info(`Posted review comment on PR #${prNumber}`);
+          await this.deleteOlderReviewComments(prNumber, owner, repo, created.id);
+          return;
+        }
+        const target = existing[existing.length - 1];
+        try {
+          await this.octokit.rest.issues.updateComment({
+            owner,
+            repo,
+            comment_id: target.id,
+            body
+          });
+        } catch (error3) {
+          if (isNotFoundError(error3) && attempt < REVIEW_COMMENT_UPSERT_ATTEMPTS) {
+            core2.info(`Review comment ${target.id} was removed by a concurrent run, retrying (${attempt})`);
+            continue;
+          }
+          throw error3;
+        }
         core2.info(`Updated existing review comment on PR #${prNumber}`);
-      } else {
-        await this.octokit.rest.issues.createComment({
-          owner,
-          repo,
-          issue_number: prNumber,
-          body: comment
-        });
-        core2.info(`Posted review comment on PR #${prNumber}`);
+        await this.deleteOlderReviewComments(prNumber, owner, repo, target.id);
+        return;
       }
     } catch (error3) {
       core2.error(`Failed to post review comment: ${error3}`);