Repository navigation
ci: hold a PR while automatic-review findings are unanswered (#4299 option B) - #4575
Conversation
…ption B) New workflow `Review answered` publishes the check `Automatic review answered` on every pull request and merge-queue entry. It is RED while the automatic review has not landed (a quota refusal or unrecognised body is not a review), while any thread the reviewer opened has no reply from a person, or while the check cannot read its input completely. It never skips. A maintainer waiver (`review-waived`, attributed via REST issue events, admin/maintain only) releases only the missing-review condition. Measured: the reviewer posts reviews as copilot-pull-request-reviewer[bot] and comments as Copilot, one account id (175728472); its July quota refusal was posted as a review. Controls: RED on #4310/#4366/#4343 at merge, GREEN on #4556 at merge and #4343 now; 12 of 12 predicate mutations caught by the self-test. Lands non-required. Part of #4299. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
New documentation/comments assert per-PR concurrency semantics that don’t match the merge-queue concurrency key as implemented, which could mislead rollout/ops expectations.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds an “observed” (non-required) GitHub Actions check that turns red until the repository’s automatic code review has landed and every inline finding thread from that reviewer has a human reply, plus documentation and guardrails to explain and self-test the behavior.
Changes:
- Introduces the
Review answeredworkflow publishing theAutomatic review answeredcheck for PR and merge-queue events. - Adds the predicate + self-test script (
.github/scripts/check-review-answered.py) and wires its self-test intodotnet-test.yml. - Documents the design/rollout and updates the PR workflow skill + What’s New entry.
File summaries
| File | Description |
|---|---|
.github/workflows/review-answered.yml |
New workflow that runs the check across PR + merge queue events and publishes the Automatic review answered context. |
.github/scripts/check-review-answered.py |
Predicate implementation and fixture-based self-test for “review landed + all threads answered + reads complete”. |
.github/workflows/dotnet-test.yml |
Adds a CI-shell self-test invocation to ensure predicate breakage fails on the PR that introduced it. |
src/MeshWeaver.Documentation/Data/Architecture/ReviewFindingsAnswered.md |
Design doc describing the motivation, rule, waiver model, controls, and rollout steps. |
src/MeshWeaver.Documentation/Data/Architecture.md |
Adds the new design doc to the Architecture topic map. |
src/MeshWeaver.Documentation/Data/WhatsNew/2026-09-17-a-pull-request-now-shows-when-a-review-finding-is-unanswered.md |
What’s New entry describing the new check and its observed-only rollout. |
.claude/skills/pullrequest/SKILL.md |
Updates the PR flow instructions to explicitly reply to all automatic-review threads and avoid self-applying the waiver label. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| # One evaluation per pull request at a time, NEVER cancelled mid-read: a review lands as one | ||
| # `submitted` event plus one `created` event per inline comment, and the evaluation that runs LAST | ||
| # must be the one that read LAST. GitHub keeps one pending run per group, so a burst of fourteen | ||
| # comments collapses to the running evaluation plus the newest pending one. | ||
| concurrency: |
There was a problem hiding this comment.
Correct, and it was the comment that was wrong rather than the key. Fixed in bf37e92.
The group is per pull request for the pull-request events and per queue entry for merge_group — the queue head ref carries the entry's sha — and that is the granularity that matters: what must never race is two evaluations publishing the same check-run name on the same commit, and two queue entries of one pull request are two different commits with two separate check-runs. So a re-queue evaluating alongside an older entry is by design, and the comment now says so. It also now says why nothing is cancelled: cancelling a merge_group run ejects the entry rather than retrying it.
| Evaluations of one pull request share a concurrency group and are never cancelled mid-read. A review | ||
| arrives as one `submitted` event and one `created` event per inline comment; GitHub keeps one running | ||
| and one pending run per group, so a burst of fourteen collapses to two evaluations, and the one that | ||
| runs last read last. |
There was a problem hiding this comment.
Fixed in bf37e92 — the paragraph now states the real behaviour: serialised per pull request for pull-request events, per queue entry for merge_group (the queue head ref carries the entry's sha), with the reason that two entries of one pull request are two commits carrying two separate check-runs, so there is nothing to serialise between them.
The same commit adds the measurement your review made visible: the reviewer's own pull_request_review event created run 35197843933 with conclusion action_required and zero jobs (triggering actor Copilot, repository policy first_time_contributors), so that event cannot evaluate the check at all — hence the bounded --wait-for-review 15, and the approval policy as Rollout step 1.
…ait for it Measured on this pull request (#4575, run 35197843933): the reviewer's `pull_request_review` event arrives and GitHub creates a run for it with conclusion `action_required` and ZERO jobs — the triggering actor is `Copilot` and this repository requires approval for a first-time contributor's runs (`actions/permissions/fork-pr-contributor-approval` = first_time_contributors). Its two `pull_request_review_comment` runs were `action_required` too. So the event that says "the review has landed" cannot evaluate anything, and a pull request whose review raises no findings would keep the red from its `opened` evaluation until somebody pushed again. `--wait-for-review 15` closes that: while the ONLY thing missing is the review, the step re-reads every 30 s for up to 15 minutes, then answers RED (job cap 20 min). Nothing else is ever waited for — an unanswered thread needs a person, an incomplete listing needs another read. A person's reply is not gated and re-runs the check in seconds. The cheaper fix is the approval policy, and it is now Rollout step 1. Also addresses Copilot's review of this PR: the concurrency comment and the doc paragraph claimed per-pull-request serialisation for `merge_group` too, which the key (the queue head ref carries the entry sha) does not do. Both now state what it is — per pull request for pull-request events, per queue entry for `merge_group` — and why that is the right granularity. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The order that matters, with the run that did the work: the opened evaluation red for the right reason; the reviewer's own event action_required with zero jobs; the push and the two replies both evaluating and reading 2 of 2 answered; three sibling runs cancelled by the concurrency group. Plus the two facts a reader would otherwise have to rediscover — the review that counts was submitted on the previous head, and the 15-minute wait was exercised against #645 (three polls, then RED naming the quota refusal). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Part of #4299 — builds option B (the maintainer's decision of 2026-09-17). It lands non-required; making it required is the maintainer's step after observing it.
What it does
A new workflow,
Review answered, publishes the checkAutomatic review answeredon every pull request and every merge-queue entry. It is RED when any of these hold, and it never skips:Pull request overview). A refusal (Copilot was unable to review …,**Files reviewed:** 0/…) or an unrecognised body is not a review.user.type == "User", followingin_reply_to_idto the root).review_commentscount read before it.Events:
pull_request(opened, synchronize, reopened, ready_for_review, labeled, unlabeled),pull_request_review(submitted, dismissed),pull_request_review_comment(created, deleted),merge_group(the PR number comes fromgh-readonly-queue/main/pr-<N>-<sha>). REST only. No job-levelif:, nocontinue-on-error, no path filter;timeout-minutes: 5; one evaluation per PR at a time, never cancelled..github/scripts/check-review-answered.py. The self-test also runs indotnet-test.yml's CI-shell lane.Doc/Architecture/ReviewFindingsAnswered, plus a line in the topic map.Category: Feature)./pullrequestskill step 4: reply to every thread; never apply the waiver label yourself.The waiver (design)
review-waived(created on the repo for this PR). Whether it is present comes from the PR. Who applied it comes from RESTissues/{n}/events, using the latestlabeled/unlabeledevent for that label.role_namefromcollaborators/{login}/permissionisadminormaintain. A label applied by a writer or a bot, or one no event can be tied to, is refused, and the refusal names the account.Measured before writing
pulls/{n}/reviewsit iscopilot-pull-request-reviewer[bot]; inpulls/{n}/commentsit isCopilot. Both are type Bot with the same account id, 175728472. Exactly one review per PR, alwaysCOMMENTED. The ruleset hasreview_on_push: falseandreview_draft_pull_requests: true.review_commentscount == comment listing length on 60 of 60 recent PRs.Controls (the real REST adapter,
--as-of)Backtest over 60 merges (2026-09-16T08:48Z → 09-17T07:41Z), each as of its
merged_at: 32 of 60 would have been RED, all because of unanswered threads (126 findings, 86 unanswered at merge).Negative controls, watched failing. Each self-test case names the exact reasons it must be red for. I broke the predicate twelve ways, one at a time: a bot reply counted as an answer; a refusal counted as a review; an unrecognised body counted as a review; any bot treated as the reviewer; the completeness check removed; a waiver releasing threads; a writer allowed to waive; a bot allowed to waive;
--as-ofignored for comments; only direct replies followed; a PENDING review counted; a short sha accepted as a queue ref. Each mutation turned the self-test red (13 of 13, including "wait on any red" after the wait was added).Local gates:
check-workflow-timeouts(0 violations),check-workflow-yaml-keys,check-pr-secret-preflight,check-workflow-permission-pairing,check-workflow-shell(0 live findings).MeshWeaver.Documentation.Testwas built-c Release -warnaserror(0 warnings) and run: 508 passed, 0 failed, including link integrity, the topic map and What's New integrity.Measured on this PR — and it changed the design
pull_requestrun publishes its check-run on the PR head sha (c496a0bffb), besideConsolidate test results; the first evaluation was RED for the right reason ("the automatic review has not landed — no review by the automatic reviewer"), andGITHUB_TOKENreadcollaborators/rbuergi/permission→admin, so the waiver path is readable with the job's own token.pull_request_reviewevent arrived and GitHub created run35197843933— conclusionaction_required, zero jobs, triggering actorCopilot, becauseactions/permissions/fork-pr-contributor-approvalisfirst_time_contributors. Bothpull_request_review_commentruns its comments raised wereaction_requiredtoo. So a PR whose review raises no findings would have kept the red from itsopenedevaluation until somebody pushed again.bf37e92a09:--wait-for-review 15— while the only thing missing is the review, the step re-reads every 30 s for up to 15 minutes, then answers RED (job cap 20 min). Nothing else is waited for; an unanswered thread needs a person and an incomplete listing needs another read. The reviewer took 4m38s on this PR. A person's reply is not gated, so the reply path re-runs in seconds.Copilot-triggered runs), which is now Rollout step 1 in the doc — with it,--wait-for-reviewgoes to 0.bf37e92a09: the concurrency comment and the doc paragraph claimed per-PR serialisation formerge_group, which the key does not do (the queue head ref carries the entry sha). Both now state the real granularity — per PR for pull-request events, per queue entry formerge_group— and why that is the right one.To make it required (not done here)
Add the context
Automatic review answeredto ruleset2128472next toConsolidate test results. First, per the doc's Rollout section:merge-queue-steward.pyabout this check. Today it reads only failedMeshWeaver Build and Testmerge_group runs, so an ejection caused by this check would be rejected as unclassifiable, or matched to an older failed build of the same PR.Pairs-with: none — adds a workflow, a script and docs; removes no public surface.
🤖 Generated with Claude Code