-
Notifications
You must be signed in to change notification settings - Fork 1
Expand file tree
/
Copy pathcheck-pr-fully-clean.py
More file actions
executable file
·3123 lines (2812 loc) · 152 KB
/
Copy pathcheck-pr-fully-clean.py
File metadata and controls
executable file
·3123 lines (2812 loc) · 152 KB
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
205
206
207
208
209
210
211
212
213
214
215
216
217
218
219
220
221
222
223
224
225
226
227
228
229
230
231
232
233
234
235
236
237
238
239
240
241
242
243
244
245
246
247
248
249
250
251
252
253
254
255
256
257
258
259
260
261
262
263
264
265
266
267
268
269
270
271
272
273
274
275
276
277
278
279
280
281
282
283
284
285
286
287
288
289
290
291
292
293
294
295
296
297
298
299
300
301
302
303
304
305
306
307
308
309
310
311
312
313
314
315
316
317
318
319
320
321
322
323
324
325
326
327
328
329
330
331
332
333
334
335
336
337
338
339
340
341
342
343
344
345
346
347
348
349
350
351
352
353
354
355
356
357
358
359
360
361
362
363
364
365
366
367
368
369
370
371
372
373
374
375
376
377
378
379
380
381
382
383
384
385
386
387
388
389
390
391
392
393
394
395
396
397
398
399
400
401
402
403
404
405
406
407
408
409
410
411
412
413
414
415
416
417
418
419
420
421
422
423
424
425
426
427
428
429
430
431
432
433
434
435
436
437
438
439
440
441
442
443
444
445
446
447
448
449
450
451
452
453
454
455
456
457
458
459
460
461
462
463
464
465
466
467
468
469
470
471
472
473
474
475
476
477
478
479
480
481
482
483
484
485
486
487
488
489
490
491
492
493
494
495
496
497
498
499
500
501
502
503
504
505
506
507
508
509
510
511
512
513
514
515
516
517
518
519
520
521
522
523
524
525
526
527
528
529
530
531
532
533
534
535
536
537
538
539
540
541
542
543
544
545
546
547
548
549
550
551
552
553
554
555
556
557
558
559
560
561
562
563
564
565
566
567
568
569
570
571
572
573
574
575
576
577
578
579
580
581
582
583
584
585
586
587
588
589
590
591
592
593
594
595
596
597
598
599
600
601
602
603
604
605
606
607
608
609
610
611
612
613
614
615
616
617
618
619
620
621
622
623
624
625
626
627
628
629
630
631
632
633
634
635
636
637
638
639
640
641
642
643
644
645
646
647
648
649
650
651
652
653
654
655
656
657
658
659
660
661
662
663
664
665
666
667
668
669
670
671
672
673
674
675
676
677
678
679
680
681
682
683
684
685
686
687
688
689
690
691
692
693
694
695
696
697
698
699
700
701
702
703
704
705
706
707
708
709
710
711
712
713
714
715
716
717
718
719
720
721
722
723
724
725
726
727
728
729
730
731
732
733
734
735
736
737
738
739
740
741
742
743
744
745
746
747
748
749
750
751
752
753
754
755
756
757
758
759
760
761
762
763
764
765
766
767
768
769
770
771
772
773
774
775
776
777
778
779
780
781
782
783
784
785
786
787
788
789
790
791
792
793
794
795
796
797
798
799
800
801
802
803
804
805
806
807
808
809
810
811
812
813
814
815
816
817
818
819
820
821
822
823
824
825
826
827
828
829
830
831
832
833
834
835
836
837
838
839
840
841
842
843
844
845
846
847
848
849
850
851
852
853
854
855
856
857
858
859
860
861
862
863
864
865
866
867
868
869
870
871
872
873
874
875
876
877
878
879
880
881
882
883
884
885
886
887
888
889
890
891
892
893
894
895
896
897
898
899
900
901
902
903
904
905
906
907
908
909
910
911
912
913
914
915
916
917
918
919
920
921
922
923
924
925
926
927
928
929
930
931
932
933
934
935
936
937
938
939
940
941
942
943
944
945
946
947
948
949
950
951
952
953
954
955
956
957
958
959
960
961
962
963
964
965
966
967
968
969
970
971
972
973
974
975
976
977
978
979
980
981
982
983
984
985
986
987
988
989
990
991
992
993
994
995
996
997
998
999
1000
#!/usr/bin/env python3
"""Automated verification tool for ARDI / fully-clean status.
Verifies that:
1. All GitHub Actions check runs for the PR's HEAD commit SHA are completed and passing.
2. An automated review comment evaluating the exact HEAD commit SHA has been posted.
3. All review comments evaluating the HEAD commit SHA contain zero findings, and no active CHANGES_REQUESTED or REJECTED state exists on the PR.
4. Every reviewer's latest verdict-bearing statement is clean.
Criterion 4 is deliberately scoped wider than criteria 2 and 3, which look only
at items evaluating the current HEAD SHA. An explicit "Needs more work" posted
against an EARLIER commit falls outside them entirely, and a later comment that
states no verdict raises no finding either -- so the PR reads clean while its
last actual verdict was "Needs more work". Absence of a verdict is not a
clearing: only a later CLEAN verdict from the SAME reviewer supersedes that
reviewer's earlier not-clean (the ordinary ARDI iterate path, #1275).
A later CLEAN from a different reviewer does not: any reviewer's standing
not-clean vetoes, including under mwc (ai-config#2274).
See shared/workflow/fully-clean.md.
NOT COVERED. A `FULLY CLEAN` line here is not the whole of that fragment's
"Findings hide on several surfaces" check, and the difference is mechanical
rather than a matter of thoroughness. Both halves of the mechanism say so.
scripts/lib/payload_fetcher.py, which governs the `--from-json` path, maps
`gh pr view`, `gh repo view`, and two `gh api` reads; the default path's own
call sites are `gh pr view --json` and the `/check-runs` read in
scripts/lib/pull_request.py, `gh repo view` for repo resolution, and the two
`/actions/runs/` reads below. `pulls/<N>/comments` is in neither, so inline
review comments are invisible here, resolved or not (ai-config#3079). No
`<summary>`-scoped match on `suppressed` exists in this file either, so a
Copilot finding inside a collapsed `<details>` block is invisible too
(ai-config#3170): it creates no inline comment and states no verdict, so no
count performed here can see it. Measured on ai-config#3167, where this
script printed FULLY CLEAN twice over a standing finding -- an inline comment
at head 16544c50, and a suppressed "previously missed" item at head 7e1294b0.
Both are pre-squash heads, reachable from no branch: fetch them from
`refs/pull/3167/head`, or read the squash commit d29d33c71 on `main`.
A caller reporting a PR ready runs the fragment's three queries alongside
this script --- `pulls/<N>/comments`, `pulls/<N>/reviews` and
`issues/<N>/comments` --- unfiltered by head SHA and printing each body. Two
of them cover the blind spots named above; the third is there because a
verdict-bearing review body can land as an issue comment rather than as a
review, which this script's own scan reads but a caller checking only the
`pulls` endpoints would miss.
Which repository is being asked about is resolved once, at startup, and threaded
through every `gh` call. It is NOT hardcoded: the same value reaches the PR
lookup and the check-runs query, so the two halves cannot describe different
repositories. Pass `-R/--repo OWNER/REPO` to target a repo other than the
current checkout's. See Morrison-Lab/ai-config#1391.
Exit codes:
0: Fully clean (safe to end ARDI loop)
1: Not clean (in-progress checks, failing checks, missing review, findings present,
or a standing not-clean verdict that nothing later superseded)
2: Called wrong, or the repository could not be resolved. Deliberately distinct
from 1, so "you invoked this incorrectly" is never read as "the PR is not
clean" (shared/principles/fail-fast.md).
"""
from __future__ import annotations
import argparse
import json
import re
import bisect
import shlex
import subprocess
import sys
import unicodedata
from datetime import datetime, timedelta, timezone
from pathlib import Path
sys.path.insert(0, str(Path(__file__).resolve().parent / "lib"))
from fences import ( # noqa: E402
CODE_SPAN_RE,
find_fence_spans,
strip_code_spans,
)
from payload_fetcher import PayloadError, PayloadFetcher # noqa: E402
from review_payload import ( # noqa: E402
extract_structured_review,
payload_findings,
payload_findings_malformed,
payload_is_blocking,
payload_is_clean,
normalize_verdict,
)
from typing import Any, Dict, List, Optional, Set, Tuple
# The status glyphs below are non-ASCII, and a Windows console defaults to
# cp1252, which cannot encode them -- so every run raised UnicodeEncodeError
# before reaching its verdict, including the test suite. Degrade the glyph
# rather than the run; on a UTF-8 console this changes nothing.
if hasattr(sys.stdout, "reconfigure"):
sys.stdout.reconfigure(errors="replace")
# `raise SystemExit("message")` prints the message but exits **1**, which is
# this script's "not clean" code -- so a usage or environment error would have
# been read as a verdict about the PR. The exit code is set explicitly for that
# reason.
USAGE_EXIT = 2
def die(message: str) -> None:
print(message, file=sys.stderr)
raise SystemExit(USAGE_EXIT)
def run_cmd(cmd: List[str]) -> str:
try:
# `encoding` is load-bearing on Windows, not tidiness. Without it the
# locale codec decodes, which is cp1252 there, and cp1252 mis-handles
# UTF-8 in TWO ways -- the quieter one being the commoner:
#
# * Silent mojibake, for most non-ASCII. Only five bytes are
# undefined in cp1252 (0x81, 0x8D, 0x8F, 0x90, 0x9D), so a smart
# quote (e2 80 9c) or U+1F600 (f0 9f 98 80) decodes WITHOUT error
# into wrong characters. The JSON still parses, so callers matching
# verdict phrases against a review body were silently matching
# against corrupted text.
# * A hard failure, only when one of those five bytes appears --
# U+1F44D is f0 9f 91 8d, which carries 0x8D. The decode then
# raises inside subprocess's reader THREAD, so the thread dies,
# `returncode` stays 0, and `stdout` is left as None. The
# returncode guard below passes and the caller sees an
# AttributeError rather than anything about decoding.
#
# Strict UTF-8 fixes both: GitHub serves UTF-8, so strict is correct,
# and errors="replace" would preserve the silent-corruption case this
# is meant to end. `text=True` is omitted deliberately -- per
# subprocess's own docs, "Text mode is triggered by setting any of
# text, encoding, errors or universal_newlines", so `encoding` already
# selects it and naming both invites the reader to think one of them is
# doing separate work.
res = subprocess.run(cmd, capture_output=True, encoding="utf-8", check=False)
except FileNotFoundError:
# A missing binary is an ENVIRONMENT failure, and it must not surface as
# exit 1 -- that is this script's "not clean" code, so an uninstalled
# `gh` would be reported as a verdict about the PR. Handled here rather
# than at one call site: `resolve_repo` guarded its own call while every
# other `gh` call still raised a raw traceback, which is the partial
# guard fail-fast.md describes -- the guard's presence reads as the
# hazard being handled everywhere. See Morrison-Lab/ai-config#1330 for
# the standing dependency on `gh` itself, which this does not remove.
message = f"`{cmd[0]}` is not installed or not on PATH."
if cmd[0] == "gh":
# fail-fast.md asks a failure to name its own remedy, and the line
# above names only the dependency. What this used to say next --
# "This script requires the GitHub CLI; -R cannot substitute for
# it." -- read as a closed door: it ruled out the one alternative
# it mentioned and stopped. It was also false, because the remedy
# ships in this same directory (`build-pr-payload.py`,
# ai-config#2908) and needs no CLI. That remedy was reachable only
# from `fully-clean.md` or that script's `--help`, both of which
# require already suspecting it exists -- so a stranded session
# hand-built the payload instead (ai-config#2938) or skipped the
# check. The error message is the one surface such a session is
# guaranteed to read (ai-config#3113).
#
# Two details the recipe cannot omit and stay executable. The
# paths are derived from `__file__` rather than written relative
# to the repo root, because this script is routinely invoked by
# absolute path from an unrelated cwd, where
# `scripts/build-pr-payload.py` resolves to nothing. And the token
# is named because `build-pr-payload.py`'s `_token()` dies without
# GITHUB_TOKEN or GH_TOKEN, so a recipe that omitted it would send
# the reader into a second dead end.
#
# Gated on `gh` because this `die` serves every command run_cmd is
# handed, and neither `--from-json` nor anything about the GitHub
# CLI answers a missing `git`.
here = Path(__file__).resolve()
message += (
"\n`-R` alone cannot substitute for it, but the GitHub CLI is"
" not required: score a JSON payload instead."
" `build-pr-payload.py` assembles one from plain REST, and"
" needs GITHUB_TOKEN or GH_TOKEN set:\n"
f" python3 {shlex.quote(str(here.parent / 'build-pr-payload.py'))}"
" OWNER/REPO N /tmp/pr.json\n"
f" python3 {shlex.quote(str(here))}"
" N -R OWNER/REPO --from-json /tmp/pr.json"
)
else:
message += "\nInstall it, or put it on PATH, and re-run."
die(message)
if res.returncode != 0:
# `stderr` is exposed to the same reader-thread decode failure as
# `stdout`, so it can be None here even though the exit code arrived.
#
# Splitting the two cases rather than substituting a placeholder for
# both. A non-zero exit WITH readable stderr is a fact about the command
# -- a 404 on a deleted run, say -- and `_resolve_run_head_sha` is
# entitled to catch that RuntimeError and degrade to "cannot resolve the
# SHA". A non-zero exit with an UNREADABLE stderr is an environment
# failure, and routing it through RuntimeError would let that same catch
# launder it into "No review comment has been posted evaluating HEAD
# SHA ..." -- exit 1 with a finding bullet, which is the laundering this
# whole change exists to close. `die` exits 2 and raises SystemExit,
# which derives from BaseException and so escapes both that catch and
# the broad `except Exception` wrappers elsewhere.
if res.stderr is None:
die(
f"Command failed ({' '.join(cmd)}) and its stderr could not be "
"read or decoded, so the reason is unavailable. This is an "
"environment failure, not a verdict about the PR."
)
raise RuntimeError(f"Command failed ({' '.join(cmd)}): {res.stderr.strip()}")
if res.stdout is None:
# Defence in depth for the reader-thread failure described above, and
# for any future cause of it.
#
# `die` rather than `raise RuntimeError`, for two independent reasons,
# both of which a RuntimeError gets wrong:
# * `_resolve_run_head_sha` wraps its `run_cmd` call in
# `except RuntimeError: return None`. A RuntimeError here would be
# swallowed there, and the caller would go on to report "No review
# comment has been posted evaluating HEAD SHA ..." -- exit 1 WITH a
# finding bullet, which is the one shape fully-clean.md's crash test
# (rc==1 plus no ` - ` bullets) cannot distinguish from a verdict.
# * SystemExit(USAGE_EXIT) exits 2, so an environment failure stays
# out of the "not clean" code, which is why USAGE_EXIT exists.
die(
f"Command produced no capturable stdout ({' '.join(cmd)}); "
"its output could not be read or decoded. This is an environment "
"failure, not a verdict about the PR."
)
return res.stdout.strip()
# `gh pr view --repo` accepts a URL as well as OWNER/REPO, while
# `gh api repos/{repo}/...` accepts only the bare OWNER/REPO. Interpolating one
# spelling into both call sites is how the two halves came apart in the first
# place, so a value that cannot serve both is refused rather than passed on.
REPO_PATTERN = re.compile(r"^[A-Za-z0-9._-]+/[A-Za-z0-9._-]+$")
def resolve_repo(explicit: str = "") -> str:
"""The OWNER/REPO every `gh` call in this run will name.
Defaults to the current checkout's repository rather than to a literal.
Hardcoding a literal is the defect this function exists to remove: the PR
lookup resolved the repo from the working directory while the check-runs
query named `Morrison-Lab/ai-config`, so outside this repo the script read
the PR from one repository and its checks from another -- loudly when the
SHA did not exist there, and silently wrong when it did.
Exits 2 rather than falling back when the repository cannot be resolved.
A fallback is what made the wrong answer quiet.
"""
if explicit:
repo, source = explicit.strip(), "--repo"
else:
try:
repo = fetch(
["gh", "repo", "view", "--json", "nameWithOwner", "-q", ".nameWithOwner"]
)
except RuntimeError as exc:
# `gh` itself missing is handled in run_cmd, because -R cannot
# substitute for it. This branch is the narrower case where `gh`
# ran and could not name a repo -- not a checkout, no remote, not
# authenticated -- and there -R IS the right hint.
die(
f"Cannot resolve the repository from the current directory: {exc}\n"
"Run this from inside a git checkout, or pass -R OWNER/REPO."
)
source = "the current checkout"
if not REPO_PATTERN.match(repo):
die(
f"Repository {repo!r} (from {source}) is not in OWNER/REPO form.\n"
"Pass -R OWNER/REPO -- a URL is accepted by `gh pr view` but not by "
"the check-runs API path, and a value that cannot serve both is what "
"lets the two halves disagree."
)
return repo
# Set by --from-json. When present it replaces every `gh` invocation, so the
# script runs where the CLI does not exist (ai-config#2441). None means "use
# run_cmd", i.e. the unchanged local behaviour.
_FETCHER = None
def fetch(cmd):
"""Run *cmd* via `gh`, or answer it from a --from-json payload."""
if _FETCHER is not None:
return _FETCHER(cmd)
return run_cmd(cmd)
def get_pr_info(pr_num: str, repo: str):
if str(Path(__file__).resolve().parent.parent) not in sys.path:
sys.path.insert(0, str(Path(__file__).resolve().parent.parent))
from scripts.lib.pull_request import PullRequest
pr = PullRequest(pr_num, repo, fetcher=fetch)
return pr
def _is_bot_author(login: Optional[str]) -> bool:
"""Return True if *login* belongs to an automated review bot."""
login_str = str(login or "")
if not login_str:
return False
return (
login_str in ("github-actions", "github-actions[bot]", "claude[bot]", "claude", "cursor")
or login_str.endswith("[bot]")
)
# Known review agent opening markers -- used to detect a review whose format the
# classifier cannot read (Morrison-Lab/ai-config#1524). The key is a lowercase
# substring to match against the body; the value is a human-readable agent name.
REVIEW_AGENT_MARKERS: Dict[str, str] = {
"**claude finished": "Claude",
"### \U0001f916 antigravity agent report": "Antigravity",
"verdict: block": "Jules",
"_posted by codex (ai agent)": "Codex",
"_posted by opencode (ai agent)": "OpenCode",
# Deliberately NOT here: the Claude Code disclosure footer
# (`_Posted by Claude Code (AI agent) ...`). CLAUDE.md's
# "Every comment you post to a forge says an agent posted it" mandates that
# footer on EVERY agent-posted comment -- claims, status updates, replies,
# and the in-chat-feedback paraphrases -- not on reviews only. Admitting it
# here made each of those a quorum-eligible automated review with identity
# "Claude": measured, an owner comment ending in the footer superseded a
# real bot "Needs more work" and satisfied quorum on a PR carrying no
# automated review at all. That reopens #2308's invariant, quoted below:
# approval authority comes from author identity and never from body text.
# A Claude review identifies itself by "**Claude finished" instead.
}
# Logins that are one reviewer, never shared. Claude and Antigravity both post
# as github-actions, so they cannot live here; Jules posts as jules[bot], and
# its body marker (`verdict: block`) is only present on the not-clean form.
EXCLUSIVE_BOT_IDENTITY: Dict[str, str] = {
"jules": "Jules",
"jules[bot]": "Jules",
"cursor": "Cursor",
}
# Workflow STATUS notices, which are not reviews. Every one of these is posted
# by `github-actions[bot]`, so the comment-admission test below -- bot author OR
# a review-header marker -- admits them all on author alone, and a notice that
# happens to carry no finding vocabulary then reads as a clean review.
#
# Measured on ai-config#1841, #1845 and #1853 (2026-08-21): of the six distinct
# bot comment shapes those PRs carry, exactly ONE is a review. A PR whose review
# quota-skipped four times reported FULLY CLEAN, because the skip notice was
# admitted, matched HEAD through its own `View run` link, and contained no
# findings (ai-config#1719).
#
# Matched against a PREFIX WINDOW rather than the whole body, because this
# corpus quotes these strings constantly -- a real review discussing a dispatch
# notice must stay a review. A notice always leads with its marker; a review
# always leads with `**Claude finished`, which is deliberately absent here.
NON_REVIEW_NOTICE_MARKERS = (
"claude review dispatched",
"claude review skipped",
"claude review did not finish",
# The AGENT workflow's quota shape, distinct from the review workflow's
# wording above. self-review-fallback.md documents both and says "Both mean
# no bot will respond on this run", so covering one and not the other left
# the identical false clean reachable through the other notice
# (review finding on ai-config#1862). scripts/pr-sweep.py's REFUSAL_MARKERS
# already carried "spend limit" for the same reason.
"spend limit",
"[pr preview action]",
"**cost:**",
)
NOTICE_PREFIX_WINDOW = 200
# The body markers that make a comment look like a review regardless of author.
# Shared by the admission test and by is_non_review_notice()'s precedence guard,
# because those two must agree: anything wide enough to be ADMITTED as a review
# has to be wide enough to be PROTECTED from notice exclusion. They disagreed
# once, and a self-review opening by quoting the skip notice it was standing in
# for -- which self-review-fallback.md tells you to write -- was dropped
# entirely, verdict and all (review finding on ai-config#1862).
REVIEW_BODY_MARKERS = (
"\U0001f916",
"### \U0001f916",
"code review",
"**claude finished",
"### verdict",
"_posted by codex (ai agent)",
"_posted by opencode (ai agent)",
"verdict:",
"review-data:",
)
def has_review_body_marker(body: str) -> bool:
"""True when *body* carries a marker that makes it read as a review."""
body_lower = body.lower()
return any(marker in body_lower for marker in REVIEW_BODY_MARKERS)
def _reviewer_identity(body: str, author: str = "") -> str:
"""Stable identity for per-reviewer latest-verdict grouping (#2274).
GitHub Actions posts Claude and Antigravity under the same bot login, so
author alone cannot tell two reviewers apart. Exclusive bots (Jules) are
keyed on login first, because their body marker is not stable across
verdicts. Shared-login reviewers are keyed on a known agent marker from
the FIRST non-empty line; fall back to the login; then to "unknown".
The first line, not the first paragraph: semantic line breaks often put
the header and the next sentence in one paragraph, and a quote of
``**Claude finished**`` on line 2 must not inherit Claude's identity.
Code spans and cited finding vocabulary are blanked first so a code span does not
match.
Residual: a shared-login review whose first or last non-empty line has no known
agent marker falls back to the login, so two unmarked ``github-actions``
bodies share one identity.
Real Claude and Antigravity reviews carry the marker on that first line.
CLI agents like Codex and OpenCode append the marker on the last line.
Scanning the whole body would re-open the quote-inheritance hole this
first-and-last-line rule exists to minimize.
"""
login = str(author or "").strip()
exclusive = EXCLUSIVE_BOT_IDENTITY.get(login.lower())
if exclusive:
return exclusive
lines = [ln.strip() for ln in (body or "").splitlines() if ln.strip()]
first_line = strip_cited_finding_vocab(strip_code_spans(lines[0])) if lines else ""
last_line = strip_cited_finding_vocab(strip_code_spans(lines[-1])) if lines else ""
agent = _detect_review_agent(first_line) or _detect_review_agent(last_line)
if agent:
return agent
# The payload's own `reviewer` field deliberately does NOT feed identity,
# even from a bot login. It is body text a reviewer writes about itself,
# so trusting it is the inverse of #2308's invariant restated in
# REVIEW_AGENT_MARKERS above -- and it is load-bearing for quorum, not
# merely stylistic: measured, two clean comments from the single login
# `github-actions[bot]` whose payloads named different reviewers satisfied
# `--quorum 2`. A quorum above 1 is the ordinary case rather than a corner
# -- `shared/workflow/fully-clean.md` prescribes
# `--quorum <number-of-reachable-providers>` and pins a three-provider
# quorum for this repo. Identity comes from the marker on the first or
# last line, or from the login.
if login:
return login
return "unknown"
def _approval_clears(
identity: str, author: str, approved_authors: set
) -> bool:
"""True when this reviewer's own later GitHub APPROVED supersedes them.
`approved_authors` is logins. Skip only when this identity is that login
(Copilot) or the exclusive-bot mapping of that login (Jules). A shared
login such as github-actions must not clear Claude because a sibling
bot later APPROVED.
"""
if author not in approved_authors:
return False
if identity == author:
return True
return EXCLUSIVE_BOT_IDENTITY.get(author.lower()) == identity
def _detect_review_agent(body: str) -> Optional[str]:
"""Return the agent name if *body* contains a known review agent marker.
Returns ``None`` when no marker matches -- which does NOT mean the comment
is not a review; it means the comment is not one of the agents whose format
we recognise. A new agent or a format change lands here until its marker is
added to ``REVIEW_AGENT_MARKERS``.
The earliest marker in the text wins, not dict order. Claude's marker is
first in the table, so a first-line Antigravity header that later quotes
``**Claude finished**`` would otherwise inherit Claude (#2274).
"""
body_lower = body.lower()
best_pos = None
best_name = None
for marker, name in REVIEW_AGENT_MARKERS.items():
pos = body_lower.find(marker)
if pos < 0:
continue
if best_pos is None or pos < best_pos:
best_pos = pos
best_name = name
return best_name
ARD_DISPOSITION_PHRASE = "ard review disposition summary"
def is_ard_disposition_summary(body: str) -> bool:
"""Is this an ARD round's own disposition summary?
An ARD round posts a disposition summary (`skills/ard/SKILL.md` requires
the summary; the heading matched here is this checker's own convention,
written nowhere else in the corpus). A driving session's round-up quotes
the verdict it is disposing of, so without this skip it reads as a
standing verdict on the session's own PR. `check_review_comments` applies
it before it reaches `is_non_review_notice`.
A bare substring test over the whole lowercased body, deliberately
unlike `is_non_review_notice`: there is no heading, position, or
review-agent precedence guard, so a review that merely QUOTES the phrase
is skipped too.
Extracted from `check_review_comments`, where it was inlined, so that
`check-review-body.py` can call it instead of duplicating the phrase. A
duplicated literal drifts silently the moment either side changes, and
the AST guard written to detect that drift was narrowed across three
review rounds and still had escapes.
What the extraction closes is the literal-drift problem, and only that.
Whether the CALL still sits ahead of admission is a property of
`check_review_comments`, not of this function, and is covered by a
behavioural test rather than by anything here.
"""
return ARD_DISPOSITION_PHRASE in body.lower()
def is_non_review_notice(body: str) -> bool:
"""True when *body* is a workflow status notice rather than a review.
A known review-agent marker takes PRECEDENCE and settles it immediately: a
real review that DISCUSSES a dispatch or skip notice -- which any review of
this corpus routinely does, since the notices are what these checks are
about -- must stay a review. Without that precedence a review quoting
`Claude Review Dispatched` in its opening paragraph was excluded outright,
turning a false clean into a false "no review at this HEAD".
Only then is the prefix window consulted. A notice leads with its marker,
so a window bounds the match rather than letting a mention anywhere in a
long body decide.
"""
# The agent check is redundant TODAY -- every REVIEW_AGENT_MARKERS entry
# happens to contain a REVIEW_BODY_MARKERS entry, so the second call decides
# every case. It stays because the redundancy is a coincidence of the current
# marker values, not an invariant: a new agent marker that is not a superset
# of some body marker would recreate the precedence gap this round existed to
# close. A test pins the property rather than leaving it to whoever edits the
# marker tables next.
if _detect_review_agent(body) or has_review_body_marker(body):
return False
head = body[:NOTICE_PREFIX_WINDOW].lower()
return any(marker in head for marker in NON_REVIEW_NOTICE_MARKERS)
def _resolve_run_head_sha(body: str, repo: str, branch: str = "") -> Optional[str]:
"""Extract a workflow run ID from a review comment body and return its head_sha.
Review comments from the ``@claude`` workflow contain a "View run" link
like ``https://github.com/{owner}/{repo}/actions/runs/{run_id}``.
Fetching that run's ``head_sha`` proves which commit the reviewer was
dispatched against, which is the authoritative source per #1520.
A ``workflow_dispatch`` run's ``head_sha`` names the dispatch ref, not
the reviewed commit (see ``fully-clean.rationale.md``), so this only
trusts the field when the run's ``head_branch`` matches the PR's own
branch -- confirming the dispatcher passed an explicit ``--ref``.
Falls back to ``None`` (body-SHA scan) when the check cannot be made.
"""
m = re.search(r"/actions/runs/(\d+)", body)
if not m:
return None
run_id = m.group(1)
try:
out = fetch(["gh", "api", f"repos/{repo}/actions/runs/{run_id}"])
run = json.loads(out)
event = run.get("event", "")
head_branch = run.get("head_branch", "")
head_sha = run.get("head_sha")
if event == "workflow_dispatch" and branch and head_branch != branch:
return None
return head_sha
except RuntimeError:
return None
def _workflow_path_for_run(run_id: str, repo: str, cache: dict) -> Optional[str]:
"""Resolve a workflow file path from an Actions run id, with per-call caching."""
if run_id in cache:
return cache[run_id]
try:
out = fetch(["gh", "api", f"repos/{repo}/actions/runs/{run_id}"])
path = json.loads(out).get("path") or ""
except RuntimeError:
path = ""
cache[run_id] = path
return path or None
def _workflow_path_from_check_run(cr: dict, repo: str, cache: dict) -> Optional[str]:
url = cr.get("html_url") or ""
m = re.search(r"/actions/runs/(\d+)/", url)
if not m:
return None
return _workflow_path_for_run(m.group(1), repo, cache)
def check_ci_runs(pr) -> Tuple[bool, List[str]]:
sha = pr.head_sha
repo = pr.repo
check_runs = [{"name": cr.name, "status": cr.status, "conclusion": cr.conclusion, "html_url": cr.html_url} for cr in pr.get_check_runs()]
issues = []
if not check_runs:
issues.append(f"No check runs found for SHA {sha[:8]}")
return False, issues
# A job name is not unique across workflows: two workflows in one repo can
# each define a job called `ubuntu-latest (release)`. Naming one alone is
# therefore ambiguous exactly when it matters, and the ambiguity is
# invisible in the rendered line, so nothing prompts the reader to check.
# Disambiguate the duplicated names with the run's own URL, which the
# payload already carries -- no extra API call.
seen = {}
for cr in check_runs:
seen[cr["name"]] = seen.get(cr["name"], 0) + 1
duplicated = {n for n, count in seen.items() if count > 1}
# Concurrency `cancel-in-progress` leaves a superseded run `cancelled` beside
# a later success with the same job name on the same SHA (ai-config#2277).
# Scope by workflow file path, not name alone: two workflows can share a job
# name (#1869) without one run superseding the other.
workflow_cache: dict = {}
success_keys = set()
for cr in check_runs:
if cr.get("status") != "completed" or cr.get("conclusion") != "success":
continue
wp = _workflow_path_from_check_run(cr, repo, workflow_cache)
if wp:
success_keys.add((cr["name"], wp))
for cr in check_runs:
name = cr["name"]
status = cr["status"]
conclusion = cr.get("conclusion")
where = ""
if name in duplicated:
# `html_url` only. A check-suite id was tried as a fallback and
# dropped: it is a different numeric namespace from the workflow-run
# id that `gh run view` takes, so rendering it in the same slot
# points the reader at nothing. And `check_suite` is documented as
# `object or null`, so `.get("check_suite", {})` returns None on a
# real payload -- `.get()` substitutes only for an absent KEY, not a
# null VALUE -- and the AttributeError would exit 1, the status this
# repo reserves for "not clean". A payload quirk would then read as
# a PR regression. No annotation beats a wrong or fatal one.
url = cr.get("html_url")
where = f" ({url})" if url else ""
if status != "completed":
issues.append(
f"Check run '{name}'{where} is still in status '{status}'")
elif conclusion not in ("success", "neutral", "skipped"):
if conclusion == "cancelled":
wp = _workflow_path_from_check_run(cr, repo, workflow_cache)
if wp and (name, wp) in success_keys:
continue
issues.append(
f"Check run '{name}'{where} completed with conclusion "
f"'{conclusion}'")
return len(issues) == 0, issues
# origin/main's own inline-span pattern, reused verbatim. The scan text this
# module produces is byte-identical to origin/main's; see
# strip_cited_finding_vocab_with_mask.
_BASE_INLINE_SPAN = re.compile(r"`[^`\n]*`")
_STRAIGHT_QUOTE_SPAN = re.compile(r'"[^"\n]*"')
_CURLY_QUOTE_SPAN = re.compile("\u201c[^\u201d\\n]*\u201d")
# Longest line this will scan for code spans. CODE_SPAN_RE restarts a lazy
# scan at every unpairable backtick run, so cost is quadratic in line length on
# a line of many unclosed runs: measured 674 ms at 34 KB against 1 ms for the
# base pass, versus 8 ms at GitHub's 65,536-character comment cap for a
# realistic multi-line body. Over the cap, a line is left UNMASKED, so nothing
# is suppressed on it and the checker behaves exactly as origin/main does --
# the over-flagging direction, which is the safe one.
_MAX_MASKED_LINE = 4096
def _citation_mask(text: str, min_backticks: int = 2) -> bytearray:
"""Mark every offset lying inside a closed code span of min_backticks+ backticks.
The INTERSECTION of a per-line scan and a whole-body scan, which is
strictly safer than either alone because each over-reaches where the other
does not.
A whole-body scan over-reaches downward: ``CODE_SPAN_RE`` bounds a span by a
blank line rather than by a line ending, so a stray backtick on one line
pairs with a stray backtick on the next and marks the finding between them.
A per-line scan over-reaches upward, which is less obvious. It can
MANUFACTURE a span CommonMark does not have, by pairing two runs on one line
that CommonMark has already consumed into a span opened on the line above:
A stray `` opener sits on this line.
``Needs more work`` on scripts/a.py is my actual verdict.
CommonMark pairs line 3's run with the FIRST run on line 4, so
``Needs more work`` is literal prose and not a citation at all. A per-line
scan sees a tidy span on line 4 and marks it, and the finding is suppressed.
Measured: ``origin/main`` not-clean, per-line-only HEAD clean.
Taking only offsets both scans agree on suppresses a match solely when it
sits in a span under both readings. Everything either scan claims alone is
left unmarked, which merely over-flags -- the safe direction
(``shared/workflow/fully-clean.md``).
"""
per_line = bytearray(len(text))
offset = 0
oversized = []
for line in text.split("\n"):
if len(line) > _MAX_MASKED_LINE:
oversized.append((offset, offset + len(line)))
else:
for match in CODE_SPAN_RE.finditer(line):
if len(match.group(1)) >= min_backticks:
begin, finish = match.span()
per_line[offset + begin:offset + finish] = (
b"\x01" * (finish - begin)
)
offset += len(line) + 1
# Blank oversized lines to same-length filler before the whole-body scan
# too, or the quadratic cost simply moves there. Offsets are preserved, and
# losing a whole-body span that crosses such a line only masks LESS.
scannable = text
if oversized:
chars = list(text)
for begin, finish in oversized:
chars[begin:finish] = " " * (finish - begin)
scannable = "".join(chars)
whole = bytearray(len(text))
for match in CODE_SPAN_RE.finditer(scannable):
if len(match.group(1)) >= min_backticks:
begin, finish = match.span()
whole[begin:finish] = b"\x01" * (finish - begin)
mask = bytearray(
(int.from_bytes(per_line, "big") & int.from_bytes(whole, "big")).to_bytes(
len(text), "big"
)
)
for begin, finish in oversized:
mask[begin:finish] = b"\x00" * (finish - begin)
return mask
def _sub_with_mask(
pattern, repl, text: str, mask: bytearray
) -> Tuple[str, bytearray]:
"""Like ``re.sub``, but carrying the mask along so offsets stay aligned.
A string ``repl`` is inserted LITERALLY, not expanded as a template, so
``r"[\\1]"`` stays those four characters rather than becoming the first
group. Today's callers pass ``" "`` or a callable, and no expansion is
wanted; the difference is named so a later caller does not assume it.
A replaced region takes mask 1 when it replaces a span; a ``repl``
callable that returns the match unchanged keeps that region's original mask,
which is what lets ``_blank_quote``'s preserve path survive.
"""
out: List[str] = []
out_mask = bytearray()
prev = 0
for match in pattern.finditer(text):
begin, finish = match.span()
out.append(text[prev:begin])
out_mask.extend(mask[prev:begin])
replacement = repl(match) if callable(repl) else repl
if replacement == match.group(0):
out_mask.extend(mask[begin:finish])
else:
out_mask.extend(b"\x01" * len(replacement))
out.append(replacement)
prev = finish
out.append(text[prev:])
out_mask.extend(mask[prev:])
return "".join(out), out_mask
def _strip_fences_with_mask(
text: str, mask: bytearray, to_strip: Optional[set[int]] = None
) -> Tuple[str, bytearray]:
"""``strip_fences(text, replacement=" ")``, carrying the mask along."""
lines = text.split("\n")
if to_strip is None:
fenced, _, orphans = find_fence_spans(text)
to_strip = fenced | orphans
out: List[str] = []
out_mask = bytearray()
offset = 0
for index, line in enumerate(lines):
if index in to_strip:
out.append(" ")
out_mask.append(1)
else:
out.append(line)
out_mask.extend(mask[offset:offset + len(line)])
if index != len(lines) - 1:
out.append("\n")
out_mask.append(0)
offset += len(line) + 1
return "".join(out), out_mask
def match_is_cited(mask: bytearray, start: int, end: int) -> bool:
"""True when a match lies WHOLLY inside cited text.
Containment is the whole discriminator. A phrase that straddles a span
boundary -- ``Needs ``more`` work``, whose verdict words are the author's
own and only whose emphasis is quoted -- is not a citation, and every
earlier attempt at this fix lost exactly that case by blanking text instead
of filtering matches.
"""
return end > start and (0 not in mask[start:end])
def strip_cited_finding_vocab_with_mask(text: str) -> Tuple[str, bytearray]:
"""Blank out spans where finding-indicator vocabulary appears as a *citation*
rather than as a raised finding, so the finding and verdict scans key on genuine
findings.
A clean verdict body routinely quotes finding vocabulary -- especially on PRs
*about* the review tooling -- inside code spans (`**Location:**`), fenced
blocks, or double quotes ("Needs more work"). A real verdict or findings
heading is never expressed that way, and the structural findings-heading and
formal CHANGES_REQUESTED/REJECTED checks remain as independent backstops.
See Morrison-Lab/ai-config#1202.
Code spans and fenced blocks are unambiguous citation and are always blanked.
A double-quoted span is blanked only when it does NOT itself carry a bold
``**...**`` finding label, so a genuine finding that happens to fall inside
quotes on the same line (e.g. ``"... **Location:** foo.py:1 ..."``) is
preserved and still detected. Blanking less can only add safe-direction
re-flags of a clean verdict; it never hides a real finding.
A THIRD citation shape, found on Morrison-Lab/ai-config#1752 (tracked as
#1760): a review narrating what changed since a prior round cites that
round's verdict as bold text inside a plain parenthetical, with no quotes
at all -- ``(**Needs more work**, reviewed at `abc1234`)``. Neither the
code-span nor the quote handling above touches this, because there ARE no
quotes.
A first version of this gated on citation-shaped WORDING anywhere in the
same parenthetical (``reviewed at``, or a ``previous``/``prior`` round)
and blanked the WHOLE parenthetical. Review on #1762 (finding 1)
confirmed that regresses: a genuine, still-unaddressed finding very
plausibly mentions "the previous round" in its OWN text while re-raising
it, and blanking the entire span erased that live finding along with the
citation --
(**Needs more work:** src/a.py:10 was flagged in the previous round
and is still unfixed)
-- which is exactly the unsafe direction line 269's ``_blank_quote``
comment and fully-clean.md both warn against: missing a not-clean signal,
not over-flagging, is the dangerous failure. Bold text plus citation
wording CO-OCCURRING anywhere in the parenthetical cannot distinguish
"citing a past verdict" from "a live finding that happens to reference
the past" -- the two are lexically identical under that gate.
A second version tightened the gate to SYNTACTIC adjacency -- the bold
span immediately followed by ``reviewed at `sha` `` -- reasoning that "a
live finding does not describe itself that way." Review on #1762 (round
2) refuted that claim by execution: a reviewer re-raising a
still-unresolved finding across rounds naturally cites the commit it was
FIRST flagged at, using the identical syntax --
(**Needs more work**, reviewed at `53f9acbf`) is still present
and unaddressed in this diff.
-- which the adjacency-only gate also silently erased. The syntax alone
can never disambiguate "citing a resolved past finding" from "citing
when a still-live finding was first raised", because both write the
identical ``**bold**, reviewed at `sha` `` fragment; only what comes
AFTER the citation says which one this is.
So the gate now also requires explicit RESOLUTION wording following the
citation within the same sentence -- ``is now Addressed`` (this corpus's
own ARD disposition vocabulary; #1752's actual comment reads "... is now
Addressed"), ``is now fixed/resolved``, ``has (since) been
fixed/addressed/resolved``, or ``no longer applies``. Only "still
present and unaddressed" (no resolution wording) fails this and is
correctly left alone; "is now Addressed" passes and blanks the citation.
This is deliberately grounded in the one wording actually observed
(#1752) rather than invented -- the safe direction, when the true
discriminator (was this specific finding actually resolved?) cannot be
determined from text alone, is to require the narrowest signal that
still covers the real case, not the broadest one that covers every
hypothetical phrasing.
Only the matched bold-plus-citation-suffix span is blanked, never the
resolution wording or anything else in the surrounding text, so an
unrelated live finding nearby always survives. Must run BEFORE the
code-span stripping below, since the SHA citation is itself
backtick-quoted and would otherwise already be blanked by the time this
runs.
Spans are replaced with a space (not deleted) so surrounding text and the
``changes requested`` negation-prefix lookbehind stay separated.
Returns ``(scan, mask)``. The scan is byte-identical to what
``origin/main`` produces -- this function deliberately blanks NOTHING extra.
The mask marks which offsets came from inside a code span delimited by a run
of two or more backticks, and ``match_is_cited`` lets the finding scans
ignore a match lying wholly inside one.
That split is the entire design, and it was arrived at the hard way. A code
span of 2+ backticks is a citation just as a single-backtick span is -- the
longer run being the only way CommonMark lets a span quote text that itself
contains a backtick, which is what a review of this corpus does constantly.
The obvious fix is to blank those spans too. Four successive attempts to do
that each broke a different downstream pass, because much of this module
measures the scan in characters and offsets rather than reading it:
- collapsing a span to one space moved ``classify_verdict``'s anchored
negation windows, so a negator reached a finding it was never next to;
- filling it to width with a non-word character unmarked a bare rejection
that ``_is_marked_or_in_verdict_section`` had accepted, swallowed the
sentence boundary ``RESOLVED_BLOCKING_SUFFIX`` stops at, destroyed the
item tag ``_findings_section_resolves_empty`` vetoes on, and split
``Needs `` `` `` `` more `` `` `` `` work`` so the phrase stopped matching;
- blanking the span at all removed a ``"`` the quote pass paired on and a
``**`` that ``_blank_quote`` preserves a span for, and could change which
identity ``_reviewer_identity`` reads.
Every one of those was fail-open on a fail-closed instrument. They are not a
list of bugs to patch individually: they are what happens when the text a
dozen character-sensitive checks consume is edited underneath them. Leaving
the text alone retires the whole class, and the mask expresses the actual
intent, which was never "blank more" but "do not count a quoted phrase as a
stated one".
Containment is the discriminator, and it falls out of the mask for free.
``Needs `` `` `` `` more `` `` `` `` work`` is the author's own verdict with
one word emphasized, so the match straddles the span and still counts; a
phrase wholly inside the span is a citation and does not.
The line this was measured on is from the ``claude-review`` verdict comment
on #2431 (ai-config#2449, 2026-08-27), verbatim:
- The exact quoted string `` Addressed GitHub Claude of `9508454e`
(Needs more work) `` matches the real comment (id 5430978306)
verbatim, confirmed via direct API fetch.
``origin/main``'s pattern finds THREE matches on that line, not two: the
inner pair around the SHA, plus each outer double-backtick delimiter
consumed as an empty span. Consuming the delimiters is the whole mechanism
-- it is what leaves ``(Needs more work)`` exposed between them, so a
Ready-for-merge review reads NOT clean. The outer spaces are optional
padding: the quoted content neither starts nor ends with a backtick, so the
no-spaces form is fixed identically.
That body stays not-clean after this fix, for a different reason: the bullet
list under its ``## Findings`` heading vetoes
``_findings_section_resolves_empty``, so ``_unresolved_finding_pattern``
still returns the findings-heading pattern.
ai-config#2452 is a SEPARATE comment on the same PR, not a second signal on
this body. Measured on ``origin/main``, the two sentences behave
differently, which is worth recording because they read alike:
No blocking findings. -> matched, then
exempted by
NOT_CLEAN_NEGATION_PREFIX
No other findings, blocking or otherwise, remain
open. -> matched, NOT
exempted
Only the second is #2452. The negation prefix looks back 25 characters, so
it clears the ``No`` immediately before ``blocking`` and not the one five
words away. Whether the second then counts still depends on whether its
position is marked, which is why it needs the surrounding comment to
reproduce and does not fire from the bare sentence alone.
"""
def _blank_quote(m: "re.Match") -> str:
# Preserve a quoted span carrying a bold finding label; blanking it could
# hide an incidentally-quoted genuine finding -- the unsafe direction.
return m.group(0) if "**" in m.group(0) else " "
# A bold-labeled citation of a PAST verdict's SHA, gated on BOTH tight
# syntactic adjacency (the bold span immediately followed by "reviewed
# at `sha`") AND explicit resolution wording within the same sentence
# afterward (#1752/#1760/#1762 rounds 1-2). Neither signal alone is
# sufficient: adjacency alone still matches a live finding re-raised
# across rounds (round 2's finding), and wording alone still matches a
# live finding that happens to mention a prior round (round 1's
# finding). The lookahead scans past an optional closing paren and up to