Route review threads by who spoke last instead of one model call each

[trask]

Review threads cost one model call each today, which is the bulk of the dashboard's model spend. This routes them by who spoke last instead: an unresolved thread whose last comment is not the author's is the author's to answer, decided without a model. Only the author's own last word goes to a model, to tell a promise of future work from a finished reply.

A short reviewer comment is checked by a praise binary. Praise is dropped rather than treated as settling the thread, and routing falls back to the comment before it: if that was another reviewer's request, the thread stays with the author; if it was the author's own reply, the deferral binary decides. Only the last comment is checked, so a thread needs nobody exactly when its single comment is praise. Of the four threads judged praise in the corpus, two are lone comments where the two readings agree, and two have earlier comments where they differ.

Fixes #152, which is the case that motivated this: four comments, none of them the author's, where the per-thread model answered none and the pull request dropped off the dashboard. The last-speaker rule routes it to the author with no model involved. #151 is top-level review feedback and is not touched here; it needs the top-level classifier swapped, which is separate work with its own evaluation.

On a 441 pull request corpus this replaces 341 per-thread calls with about 87 batched ones, and 134 of the 144 pull requests with a live thread keep the same thread-level ownership. Of the ten that move, six now ask the author, including #152. The other four hand off to reviewers because the deferral binary reads the author's last word as complete: an offer waiting on a reviewer to answer, a pushback, a handoff to a third party, and a discussion moved to Slack.

Whole-pull-request route agreement is not measured here. The replay corpus was collected without commits, so compute_facts never sees pushes or CI and route_pr cannot be evaluated; #192 tracks closing that gap.

[opentelemetry-pr-dashboard]

Pull request dashboard status

Merged · refreshed 2026-07-30 13:08 UTC

Status above doesn't look right?

• Anything look wrong? Report it with what you expected; it helps us improve the dashboard.

[Copilot]

Pull request overview

Reworks unresolved review-thread routing to reduce model calls through deterministic last-speaker ownership.

Changes:

• Routes reviewer-last threads directly to authors.
• Batches author-reply and short-praise classification.
• Adds regression tests for routing and praise handling.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
classification.py	Implements deterministic routing and binary classifiers.
dashboard.py	Removes the obsolete classifier constant import.
test_top_level_actions.py	Tests last-speaker and praise routing.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

.github/scripts/pull-request-dashboard/classification.py:1037

• This assumes comments[-1] is who spoke last, but review-thread comments are sorted by updatedAt first (dashboard.py:644,652). Editing an older comment therefore moves it to the end: for example, editing the reviewer's original request after an author reply routes the thread back to the author, even though the author actually spoke last. Preserve creation order (or carry an explicit last-created-comment role) before applying this deterministic rule.

        elif comments and comments[-1].get("actor_role") == "author":

.github/scripts/pull-request-dashboard/classification.py:1031

• Only one trailing praise comment is removed. A thread such as reviewer: "Nice" followed by reviewer: "LGTM" is entirely praise, but after this single pop() the earlier reviewer comment remains and the thread is routed to the author, contradicting the stated “thread that is nothing but praise needs nobody” behavior. Strip/classify trailing praise repeatedly (with coverage for multiple praise comments) before choosing the fallback speaker.

This issue also appears on line 1037 of the same file.

        dropped = discussion["discussion_id"] in ignored
        if dropped:
            comments.pop()
        if dropped and not comments:

[trask]

Both low-confidence findings are mechanically right; neither fires on the 441 pull request corpus. 2b57543.

Only one trailing praise comment is removed. Correct, and my wording was the thing at fault. _could_be_praise inspects only the last comment, so two consecutive praise comments leave one behind and the thread goes to the author — which contradicted "a thread that is nothing but praise needs nobody". I have not made the strip iterative: each round is another model round-trip per pull request, and the corpus has exactly one thread with two trailing short non-author comments, whose text is 'Please link to a permanent link in...' — a request, not praise. So the case being defended against does not occur, and the failure is toward the author, which one /dashboard route:reviewers undoes. Instead the limitation is now stated in the code comment and the description, and pinned by test_only_the_last_comment_is_checked_for_praise so it cannot drift silently.

Comments are ordered by updatedAt. Also correct: group_review_threads builds each timestamp as updatedAt or createdAt and sorts on it, so editing an older comment does move it to the end. Measured across 341 live threads, sorting by createdAt instead changes who spoke last on 0 of them. I have left it alone here for two reasons: createdAt is not carried through discussion_comment, so fixing it properly means changing what the whole pipeline stores, including the since timestamps the status comment shows; and the failure direction is a thread returning to the author who may not owe anything, rather than one going missing. Worth its own change rather than folding an ordering fix into a routing one.

320 tests pass.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

.github/scripts/pull-request-dashboard/classification.py:1028

• comments[-1] is not necessarily the participant who posted last. group_review_threads stores updatedAt as each comment's timestamp and sorts on it (dashboard.py:643-652), so editing an older reviewer comment after an author reply moves that reviewer comment to the end. This new last-speaker routing then sends the thread back to the author (and may run praise detection on the edited old comment) even though the author's reply is still the final comment in thread order. Preserve creation/thread order for this decision; keep update time separately if it is needed for activity tracking.

        comments = list(discussion.get("comments") or [])

.github/scripts/pull-request-dashboard/classification.py:1299

• When the CLI exits nonzero but still returns a valid, non-duplicate verdict, this else also reports that no valid verdict was returned. That makes failure diagnostics contradictory; only append this reason when valid_response is false, matching the existing top-level batch parser.

            else:
                reasons.append(
                    "Copilot CLI did not return a valid verdict for this discussion_id"
                )

[trask]

c517e8a.

Contradictory failure diagnostics. Correct, and it was a divergence from the parser this was modelled on. run_llm_for_top_level_batch guards the same message with elif not valid_response or not valid_action:, while run_llm_for_verdict_batch used a bare else bound to the duplicate check. So a nonzero exit that still returned a perfectly good verdict reported Copilot CLI exited with status 1; Copilot CLI did not return a valid verdict for this discussion_id, the second half of which is false — and that string is what someone would grep for when the classifier misbehaves. Now an elif not valid_response, with two tests: a nonzero exit carrying a usable verdict no longer claims the answer was unreadable, and an unreadable answer still does.

Comment ordering by updatedAt. This is the same finding as the previous review, and my answer is unchanged: the mechanism is real, and across 341 live threads sorting by createdAt instead changes who spoke last on 0 of them. Worth adding since it is now raised twice — the praise path has the same exposure, because _could_be_praise reads comments[-1], so an edited old comment could in principle be the one classified. Still 0 occurrences, still recoverable in direction, and still not fixable without changing what discussion_comment stores and therefore the since timestamps the status comment renders. I would rather do that as its own change than bundle a storage change into a routing one.

322 tests pass.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

[trask]

Reverting 339be6a in 58e1d06, and correcting what I wrote about it.

I claimed the cap "turns a slow refresh into no refresh at all". That is wrong. save_classification_cache runs inside classify_discussion_domains, before build_pr_result checks for failed classifications, so the cache persists even when the refresh is rejected. Each run therefore classifies up to 200 uncached items and keeps them, and the next run gets them free. A pull request with 205 threads publishes on the second cycle; one with 1000 takes five. It is a delay, not an outage, and I asserted the severe reading without checking whether the cache survived the failure.

That undermines the change. The remaining harm is a cycle or two of staleness on a pull request nobody has: the largest live-thread count in the 441 pull request corpus is 12. Against that, overflow_is_safe did exactly what the existing policy refuses to do — invent an answer. For an author-last thread the deterministic rule says reviewer, and overflow was routing it to author, which is a decision nobody computed rather than a neutral default. The original comment on that branch is right that an unread item should not be given an action.

So the cap behaves as it did before this pull request for every path, and overflow_is_safe is gone rather than left unused. The finding was still correct that the thread binaries inherited a cap the old per-thread path did not have; I just picked the wrong remedy, and the right one is that the cap converges on its own.

325 tests pass.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

.github/scripts/pull-request-dashboard/classification.py:1046

• Dropping praise only from this local list leaves the original thread unchanged. build_review_thread_pending_actions later reads discussion["comments"][-1] (dashboard.py:1024), so it records the praise comment's timestamp as waiting_since even though ownership fell back to the preceding comment. A later “LGTM” therefore resets the displayed wait age for an older pending request/reply. Carry the effective fallback timestamp into pending-action construction (and cover both reviewer→praise and author→praise cases).

        dropped = discussion["discussion_id"] in ignored
        if dropped:
            comments.pop()

[trask]

Correct, and it is live rather than theoretical. 5032f06.

Dropping the praise comment only from the local list meant build_review_thread_pending_actions still dated the pending action from discussion["comments"][-1], which is the praise. So ownership fell back to the earlier comment while the clock restarted at the praise. On the 441 pull request corpus:

opentelemetry-java#8164  praise=2026-05-20  deciding comment=2026-03-12
opentelemetry-js#6293    praise=2026-06-15  deciding comment=2026-06-15

The first understates that wait by 69 days. That is the worst direction for this field, because wait age is what makes a stale request visible — a reviewer dropping an LGTM on an old unanswered thread would have made it look freshly raised.

classify_review_threads now carries the effective comment's timestamp on the record, and build_review_thread_pending_actions prefers it over the raw last comment. It is set for every thread rather than only the ignored ones, so the deciding comment and the date it is shown with cannot drift apart again.

Both cases you named are covered: test_praise_after_a_reviewer_request_keeps_the_request_date and test_praise_after_an_author_reply_keeps_the_reply_date. Reverting the since preference makes both fail. Routing across the corpus is unchanged at 262 author / 77 reviewer / 2 none, since this only moves the date.

327 tests pass.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

.github/scripts/pull-request-dashboard/dashboard.py:1215

• This timestamp comparison no longer reflects thread order. group_review_threads now intentionally orders comments by createdAt, but each comment's timestamp is still updatedAt; if an earlier request is edited after the deciding reply, it is skipped here, while a later praise comment with an older timestamp can be counted as an open reviewer. That defeats the new goal of excluding only the ignored final praise and can assign open_thread to the praising reviewer or omit the actual requester. Carry an explicit “ignored final comment”/deciding-position marker through the classification and pending-action records, then filter by position rather than comparing timestamps.

            if decided_at and (comment.get("timestamp") or "") > decided_at:
                continue

[trask]

Correct, and it is a regression I introduced two commits ago. a8d9314.

The premise checks out: group_review_threads orders by createdAt while each comment's timestamp is updatedAt, so my > decided_at comparison mixed the two orderings.

It is also worse than described, because it does not need praise to trigger. since is set for every thread, so on any thread an earlier comment edited after the last one exceeds decided_at and gets skipped — dropping the real requester from open_thread and from reviewer notifications. Measured on the corpus:

live threads: 355
threads with an edited comment: 261
threads where updatedAt order differs from createdAt order: 2

Editing is normal rather than exotic, and 2 threads already have the ordering mismatch that makes this bite. Before this pull request reviewers_with_open_threads had no filter at all, so I created the hole while fixing the notification bug.

Fixed positionally as you suggested, and the position was already known: ignored is exactly the set of threads whose last comment was dropped. That flag now rides on the classification record and the pending action, and reviewers_with_open_threads skips comments[-1] when it is set. No timestamp comparison remains, so edits cannot affect it. The flag is only written when true, to leave the pending-action shape unchanged everywhere else.

test_an_edited_request_still_counts_its_reviewer pins it: a request created first but edited last, an author reply, then praise — the requester must still be counted. Restoring the timestamp comparison makes it fail.

328 tests pass, and corpus routing is unchanged at 262 author / 77 reviewer / 2 none.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.