Repository navigation
Conversation
Two people mentioning the bot on one PR bought two full engine runs over byte-identical code, because jobs were deduplicated by trigger comment id. Under THEMIS_CONCURRENCY they would also run at once and double-post. Reviews now share one queue id per PR, so at most one is ever in flight, and a `revision` decides which of the colliding triggers is redundant: same head sha (or the same re-delivered comment) is dropped, anything else is kept as a follow-up and runs when the PR's slot frees. Steered mentions key on their own comment - the request text is part of what gets reviewed, so it is never the review already running. An unknown revision never counts as a duplicate; guessing "same" would drop real work. The head sha comes free on pull_request payloads; mentions and /api/review resolve it with the installation token the ack already mints. Resolution failure degrades to per-comment dedup rather than blocking the trigger. Closes #77 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![themis-lezgo[bot]](https://avatars.githubusercontent.com/u/22979684?s=60&v=4){kind=small}
⚖️ Themis review: 🟠 Fix before mergeThe SHA-based queue key is a solid direction, but it currently describes the trigger rather than the checkout the worker actually reviews. A request captured at commit A can begin after commit B is pushed, review B, and still leave a queued request for B to run a second full review. CI is still running; local tests could not start because this environment cannot download a required dependency.
🟠 Majors
📝 Walkthrough
🧪 How to verify
Product take: This prevents duplicated agent runs and duplicate PR output, directly addressing a costly review-loop failure. 🧭 Assumptions & unverified claimsNo unverified assumptions or claims. A neat queue key needs a checkout to match it · reviewed at 98d0a3b |
The revision captured at trigger time names the head as it looked then. The worker resolves the real head when it starts, so a review requested at A can begin after B is pushed, review B, and still leave the follow-up stored for B to run a second full review of code already reviewed. A finished run now reports the commit it posted a review of, and the queue corrects the in-flight revision to it: a follow-up waiting for that commit is superseded, and later triggers naming it are duplicates. A run that posted nothing (closed PR, auto-review off, title skip, no delta base) reports nothing, so a skip can never masquerade as coverage. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Round 1 finding fixed — you were right, and the hole was the one the PR claims to close.
Closed by making the queue learn what actually happened rather than pinning the checkout:
I chose correction over pinning the clone to the captured SHA: pinning would change what gets reviewed (a review requested before a push would deliberately ignore that push) and would need the workspace to fetch a specific commit rather than Regression tests, all failing before this commit:
@themis-lezgo review |
|
No findings. |
|
@themis-lezgo review |
⚖️ Themis review: ✅ Ship itTL;DR: Review requests for the same PR head now collapse while a newer head is retained for one follow-up run. The completed test and lockfile checks passed; the focused queue and router coverage exercises the new revision and head-reconciliation paths.
⚖️ Acknowledged
📝 Walkthrough
🧪 How to verify
Product take: A solid operational improvement: duplicate agent runs and duplicate review posts are avoided without dropping a later push. 🧭 Assumptions & unverified claimsNo unverified assumptions or claims. One PR, one head, one review — much less déjà vu · reviewed at aedbc15 |
The revision named the commit but not the kind of review. A delta job and a mention resolving the same head produced the same string, so the mention was dropped as a duplicate - and a delta re-review covers only the commits pushed since the last review, and may decline to run at all (delta disabled, no prior review, no checkpoint key). The user who asked for a review got a narrower one, or silence. Revisions are now scope-qualified (`delta:<sha>` vs `sha:<sha>`), reported back in the same form when a run reports what it covered. Two triggers on one commit still collapse when they are the same kind of work, which is the case the issue is about. A delta follow-up left alive behind a full review re-checks the checkpoint marker and exits cheaply, which is the documented shape. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Post-ship-it correction — I found the round-1 finding had a sibling I had not closed, so I would rather land it than merge around it. The class, not just the instance: round 1 was "the revision names the trigger, not the work". I fixed the commit half (report what was actually reviewed) and left the scope half open.
Someone asked for a review and got silence. Even when the delta does post, it is explicitly scoped ("Do not re-review unchanged code the earlier review already covered") — a full-review request answered by a narrower review. Same root cause, other instances of the class, all closed by the same change: an auto job that early-exits on Fix: revisions are scope-qualified — New regression tests:
|
|
@themis-lezgo review |
⚖️ Themis review: 🔴 Hold the mergeTL;DR: Collapsing equivalent heads is the right direction, but a single follow-up slot now overwrites distinct focused review requests while a review is active. That silently drops an explicit request, and the new pre-enqueue GitHub calls can make webhook delivery fail before any job exists. CI is still running.
🔴 Blockers
🟠 Majors
⚖️ Acknowledged
📝 Walkthrough
🧪 How to verify
Product take: This removes wasteful duplicate reviews and double-posting, a solid operational improvement. 🧭 Assumptions & unverified claimsNo unverified assumptions or claims. One follow-up slot is neat until two humans ask for two things · reviewed at 96c9c3d 1 finding(s) anchored outside the diff (not posted inline)
Do not coalesce distinct explicit review requests. Observed: every |
|
Round 3 lands at the review budget I was given for this PR (3 rounds), so I am stopping here rather than pushing an unreviewed fourth fix. Recording both findings so they are not lost, with my read of each. 🔴 The fix looks small and local — give a steered request its own job id again ( 🟠 The direction I would take: enqueue first with the head the payload carried (possibly Neither is written, neither is reviewed. Over to you on whether to take this PR with them fixed in a follow-up, or to have me continue. |
A single follow-up slot per PR meant the newest held trigger replaced whatever was already waiting. That is right for pushes and plain mentions - each asks for "review the current state", so the newest subsumes the rest - and wrong for a steered request, whose instruction no later review answers. Two owners each asking for a different focus area during one running review left only the second queued. The queue now takes an `on_conflict` policy instead of a followup flag: "drop" (unchanged default), "coalesce" (supersedable: replaces the first coalescing job held, in place, so a push stream cannot reorder itself ahead of an explicit ask), and "queue" (held in arrival order, never replaced). Held jobs are a list: the revision check reads all of them, completion drops only the ones the finished run covered, and requeueing the first keeps the rest. `_prepare_trigger` also runs before the enqueue on every ingress path, and resolving a head talks to GitHub. It swallows its own failures, but a cancelled handler raises through it - CancelledError is a BaseException - and GitHub does not redeliver a delivery whose connection it dropped. The enqueue moves into a `finally` on all three paths, so the trigger is queued either way and degrades to the head it arrived with.
|
Round 4. Both round-3 findings are fixed — and in fixing them I rejected both of the fix directions I published last round, because working them through showed each one trading the blocker for a different one. 🔴 What I said I would do: give a steered request its own job id again ( Your suggestion was the right one and I took it: FIFO for what cannot be superseded, newest-wins only for what can. Concretely,
Three details that are where this would go wrong if I got them wrong, each with its own test:
Your I did not add a cap on the held list. It is unbounded, and I considered bounding it, but a cap can only be enforced by dropping a request — which is the blocker again, just rarer and with a log line. 🟠 What I said I would do: enqueue first with the payload's head and correct the revision afterwards through The enqueue does not need to move — it needs to be unskippable. It is now in a try:
job = await _prepare_trigger(settings, job, ack)
finally:
enqueued = _enqueue(settings, queue, slug, job)
Applied to all three ingress paths, not just the webhook. The webhook is the severe one (GitHub does not redeliver a delivery whose connection it dropped) but Last round I flagged the One thing I looked at and deliberately left alone. The revision a run reports back is scope-qualified with the trigger's scope (
|
|
@themis-lezgo review |
⚖️ Themis review: 🟠 Fix before mergeTL;DR: The revision-aware queue collapses same-head review requests while retaining changed heads, but webhook handling still waits on GitHub API work before returning. GitHub requires a webhook 2XX within 10 seconds and records slower deliveries as failures; the external calls here have 30-second timeouts. The completed test and lockfile-freshness checks passed.
🟠 Majors
⚖️ Acknowledged
📝 Walkthrough
🧪 How to verify
Product take: This removes wasted duplicate review runs for active PRs, a solid operational improvement. The webhook response path still needs a deadline-safe handoff so explicit review requests are not lost during GitHub API slowness. 🧭 Assumptions & unverified claimsNo unverified assumptions or claims. The queue is finally learning the difference between deja vu and a new commit · reviewed at f925231 |
Trigger preparation is the only GitHub work on the webhook response path, and GitHub records a delivery as failed when the endpoint has not answered within 10 seconds. Three calls at a 30-second client timeout could sit well past that, and a delivery marked failed is one GitHub does not retry. Preparation now runs under a 5-second deadline that also governs its HTTP clients, and assigns as it goes so a deadline landing mid-ack keeps a head resolved a moment earlier. What it produces is enrichment - an eyes reaction, a sharper dedup key - never the trigger itself, so passing the deadline costs those and nothing else: the job enqueues with the head it arrived with, which degrades to per-comment dedup.
|
Round 4 finding fixed. 🟠 Preparation now runs under a deadline: TRIGGER_PREPARE_TIMEOUT = 5.0
...
try:
await asyncio.wait_for(prepare(), TRIGGER_PREPARE_TIMEOUT)
except TimeoutError:
logger.warning("themis_trigger_prepare_timeout repo=%s pr=%s seconds=%s", ...)Five seconds against GitHub's ten, and the same number is the Two details that make the degradation clean rather than merely fast:
Your This composes with the
|
|
@themis-lezgo review |
| being replaced. Two owners asking for two different focus areas are two | ||
| reviews, and always were: before issue #77 each mention had its own job id. | ||
| """ | ||
| return "queue" if job.extra_context else "coalesce" |
There was a problem hiding this comment.
🔴 Blocker · ⚡ Quick win
Keep unresolved-head review requests instead of coalescing them.
Observed: _revision assigns a failed head lookup a distinct comment:<id> revision (or None), but this function still selects coalesce; _hold then replaces an existing coalescing follow-up. Predicted: if head resolution is unavailable while a review runs, a later explicit request could replace an earlier request and the earlier review would never run. Return queue whenever head_sha is absent, so only requests with a known head are coalesced, and add a regression for two distinct unresolved-head mentions.
| return "queue" if job.extra_context else "coalesce" | |
| return "queue" if job.extra_context or job.head_sha is None else "coalesce" |
⚖️ Themis review: 🔴 Hold the mergeThe known-head path correctly serializes a PR and collapses duplicate work, but the fallback for a failed head lookup can discard a distinct explicit review request. The test check is still running.
🔴 Blockers
⚖️ Acknowledged
📝 Walkthrough
🧪 How to verify
Product take: This is a solid reduction in duplicate agent work, especially for busy PRs. 🧭 Assumptions & unverified claimsNo unverified assumptions or claims. The queue is learning that “unknown” is not a synonym for “latest” · reviewed at 6e9e579 |
…redundant A round of review read `_on_conflict` as depending on the head commit, and the docstring invited that: it explained itself in terms of what a review covers. It does not depend on the head. `_revision` answers "is this provably the same work?" and needs the head; `_on_conflict` answers "does a later ask of this kind answer this one?" and the trigger states that itself. Behavior is unchanged; a test pins the pair of outcomes the two keys produce for the same two mentions, resolved head and unresolved.
|
🔴 The decisive fact is that the unresolved-head path is strictly more conservative than the resolved-head path it falls back from. Same two plain mentions during one running review:
The resolved row is issue #77's actual case and this reviewer accepted it in rounds 1–4. The unresolved row keeps more work than that, not less. A degraded path that preserves less than the happy path would be a bug; one that preserves more is the fallback doing its job. That is now pinned as one test with both halves side by side — Why the head does not enter this decision at all. The two keys answer different questions, and I had not said so clearly enough:
So an unresolved head changes the revision and not the conflict policy. The alternative — treating an unknown head as "not supersedable" — is consistent with the letter of the unknown-never-matches rule and costs the outcome that rule exists to protect: during a GitHub outage, five mentions on one PR would become five sequential full engine runs over identical code, which is issue #77 reappearing in exactly the degraded path where the engine budget matters most. The finding did surface a real defect, and I fixed that. If a careful reader read One correction to my own round-4 comment while I am here: I said the latency exposure predates this PR. What I verified is the call ordering on
|
|
@themis-lezgo review |
⚖️ Themis review: 🔴 Hold the mergeTL;DR: Same-head requests now share one review slot and the queue records the commit actually reviewed, which addresses the duplicate-work goal. An open blocker remains when GitHub cannot resolve a PR head: successive plain review requests can still replace one another. CI is still running; the completed
🔴 Blockers
⚖️ Acknowledged
📝 Walkthrough
🧪 How to verify
Product take: Avoiding duplicate full reviews is a solid operational improvement for busy PRs. 🧭 Assumptions & unverified claims
The queue has learned the difference between déjà vu and a new commit; it just needs to keep every note when GitHub goes quiet. · reviewed at c3ed2e2 |
|
Round 6 is the last of the budget for this PR, and it returns the round-5 finding unchanged. Recording where that leaves things rather than spending another round. Rounds 4 and 5 fixed the things I agreed with, both of them regressions this PR introduced:
The remaining 🔴 I decline, and round 6 did not add anything to change that. The verify step asks to confirm each distinct unresolved-head request is "retained rather than replaced". The reason I am not doing that is the comparison in the previous comment, which this round did not engage:
The unresolved path already keeps strictly more work than the resolved path it falls back from, and the resolved path is issue #77's actual case — accepted in rounds 1 through 4 of this same review. "Retaining each request" would mean N sequential full engine runs over identical code during a GitHub outage, which is the waste this PR exists to remove, reappearing in the degraded path where the engine budget matters most.
Note for whoever picks this up: the assumptions block on this round says the suite could not be run in the review sandbox. Locally, Over to a human on the last point. Everything else in this PR is reviewed and closed. |
Coalescing replaced any held coalescing job with any other, and both a plain mention and a push-delta are coalescing. So: a review is running, someone mentions the bot asking for a review of newer code, the author then pushes - and the delta job takes the mention's place in the held list. The delta may decline outright (delta_review off, no prior review, no checkpoint key) and post nothing, and it skips the ack, so the request that was acknowledged with an eyes reaction is never answered. On main the mention had its own job id and always ran. 96c9c3d closed the same pairing in the other arrival order, where the scope-qualified revision keeps a mention from being read as the queued delta's duplicate. This is that fix's sibling on the coalescing side: jobs now coalesce within a group, and `_scope` is the group, so a delta only ever supersedes another delta. A burst of pushes still collapses to one re-review; a mention and a push are two. Found by an independent review pass, not by the review loop on this PR.
|
An independent review pass over the whole branch found a lost-work regression that six rounds here did not, and I have fixed it. Recording it, plus its verdicts on the things this review and I disagreed about. 🔴 A push takes the place of a review someone asked for.
This is a regression from Fixed in
On the finding I declined twice: the independent pass reached the same conclusion I did and could not construct a case where a plain mention goes unanswered — the surviving follow-up re-resolves the PR and reviews its current state, which answers both requests. It also made the point that the instinct behind the finding was sound and pointed at something real; it just lived in the delta-versus-mention pairing above, not the plain-versus-plain one. So: the specific finding stays declined, and the thing it was reaching for is now fixed. It also checked the three things I said I was leaving alone and agreed with all three, having verified the delta-fallback claim against Two smaller things it raised that I am not changing here, so they are on the record rather than in a commit:
This is past the review budget I set for this PR, and I am asking for one more round anyway: the change is to the queue's core replacement rule, and shipping a blocker fix with no review at all is the wrong trade. |
|
@themis-lezgo review |
⚖️ Themis review: 🔴 Hold the mergeThe SHA-based deduplication handles known heads and reconciles the commit actually reviewed, but an unavailable head lookup can still replace an earlier explicit request with a later one. That means a transient GitHub failure can discard review work rather than retaining it. CI is still running; the completed
🔴 Blockers
⚖️ Acknowledged
📝 Walkthrough
🧪 How to verify
Product take: Avoiding duplicate agent runs is a solid operational improvement. 🧭 Assumptions & unverified claims
The duplicate-run net is nearly there; it just needs to hold onto requests when GitHub blinks · reviewed at b7bab17 |
|
Round 7: the coalescing-group fix is reviewed and uncontested, and the two earlier findings are marked resolved. The only remaining objection is the unresolved-head one, now raised a third time in the same terms, again from a sandbox whose assumptions block says it could not run the suite — so it has not been able to execute I am not spending another round re-arguing it. The branch is at |
Closes #77.
Problem
Review jobs were deduplicated by trigger comment id, so two people mentioning the bot on the same PR enqueued two full reviews of byte-identical code (observed on Flagsmith/flagsmith#8052: ~10 min of wasted engine time, cancelled by restarting the service). With
THEMIS_CONCURRENCY > 1the duplicates would also run simultaneously and double-post.Change
Two keys instead of one, because the id was doing two jobs badly:
review:{repo}#{pr}, so at most one review of a PR is in flight regardless of concurrency.revisiononqueue.enqueuenames what the job would process. A revision already in flight under that id (the running job's, or the follow-up already stored behind it) is dropped instead of queued.Review revisions:
sha:<head>sha:<new head>@themis review <context>)context:<comment id>comment:<id>/api/review)NoneNonenever counts as a duplicate: guessing "same" would drop real work, and the existing rule is that an event arriving mid-run must not vanish.The head sha is free on
pull_requestpayloads. Mentions and/api/reviewresolve it viaget_pr, reusing the installation token the eyes-ack already mints (_ackbecame_prepare_trigger, one token for both). Resolution failure is best-effort: the job keepshead_sha=Noneand degrades to per-comment dedup rather than blocking the trigger.Notes for review
tests/test_queue.py::test_new_revision_still_runs_after_the_active_jobpins it.Verify
uv run pytest -q(876 passed),uv run ruff check src tests.Regression tests fail on
main:Manually: mention the bot twice on one PR without pushing in between — the second webhook answers
duplicateand the logs showthemis_job_duplicate id=review:… revision=sha:…with a single engine run. Push a commit while a review runs, then confirm the follow-up review starts on the new head once the first finishes.🤖 Generated with Claude Code