From f4d04b7f8e9ca8292e940a0f3f909d3067e7df13 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sun, 2 Aug 2026 20:23:32 -0700 Subject: [PATCH 1/8] Make a wait report its own failure instead of reading as pending A backgrounded wait emits nothing when its command is broken, which is exactly what a wait whose condition has not happened yet emits, so a watcher that stopped working reads as a watcher still working. One session polled `gh pr checks --json` against an installed gh that has no such flag, took the `|| echo '[]'` fallback on every error, and reported "still waiting" for 25 minutes while CI had been green for most of them. The rule text gains the failure clause it was missing, in AGENTS.md for the wait mechanics and in GOVERNANCE.md for the reporting: a launched process is not a result, and a cause nobody observed is not a diagnosis. Both sections are verbatim fleet content, so TODO.md records the re-vendor they owe. The Copilot runbook gains the terminal case. A response naming a quota or a rate limit ends the wait rather than extending it, since no formal review follows one and re-requesting does not change it, and the newest Copilot activity of any kind is read before another wait opens, because a wait still reporting "pending" against a landed review is a broken wait rather than a slow reviewer. `pr_review.py wait` implements that structurally rather than by matching a phrase: a reviewer comment postdating its newest review is an answer that satisfies no coverage check by design, so it exits 40 and prints the body whole for the maintainer to read. The timeout path prints the full digest, since a bare PENDING line reports a broken poll and a slow reviewer identically. Co-Authored-By: Claude Opus 5 (1M context) --- .github/copilot-instructions.md | 2 + AGENTS.md | 1 + GOVERNANCE.md | 1 + TODO.md | 3 +- scripts/README.md | 2 + scripts/pr_review.py | 87 ++++++++++++++++++++++-------- scripts/test_pr_review.py | 94 ++++++++++++++++++++++++++++++--- 7 files changed, 162 insertions(+), 28 deletions(-) diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 4c4a8e68..c0aace38 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -148,6 +148,8 @@ This path is only for a **genuinely missing** review, meaning no Copilot review **A slow review is pending, not missing, so poll with backoff and never escalate on a timeout alone.** Copilot can lag far beyond the usual one-to-three minutes when it has been re-requested many times in quick succession, because it throttles under load, and a re-review landing tens of minutes after the request is normal. A poll that times out is therefore evidence only that the review has not landed *yet*, not that Copilot is done or unresponsive. Report the status as "review still pending" and keep polling on a widening interval (for example 20s steps, then a few minutes) rather than stopping. Enter the escalation step below only when the `requestReviews` mutation itself no-ops or errors, or after a genuinely long wait with the request confirmed accepted, never merely because one fixed poll window elapsed. +**Bound each wait, and read what Copilot actually posted before opening another one.** A poll that widens forever is indistinguishable from a poll that has stopped, and "still pending" is the honest report for exactly as long as evidence supports it. Two readings decide whether waiting again is warranted. Compare the request's timestamp against the newest Copilot activity of **any** kind on the pull request, since a reviewer that has already answered on a later head, or that posted an issue comment instead of a formal review, is not a reviewer running late, and a wait that keeps reporting "pending" against a landed review is a broken wait rather than a slow reviewer. Then read that newest response, because a Copilot answer naming a quota or a rate limit is a **terminal** outcome rather than a pending one: no formal review will land, so path (1) never matches the head and path (2) is correctly never confirmed, both paths behave exactly as specified, and the agent waits for something that is not coming. The fix is account-side and re-requesting does not change it, so report it to the maintainer and stop waiting. Where the newest response is neither a review nor a refusal you recognize, that too goes to the maintainer with its text, rather than being waited through. + If a review did not run on the current head, retry: 1. Wait briefly and check head-SHA coverage (see above). diff --git a/AGENTS.md b/AGENTS.md index 13d20700..05fb1a18 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -45,6 +45,7 @@ If a rule you were given does not cover what you find, stop and report it. Do no ``` - **Wait in a background process, not in a poll loop.** A review or CI wait is a sequence of near-identical requests, each billed for whatever context it happens to carry. Run the wait as one backgrounded command that returns when the condition is met. +- **A wait separates three outcomes, and says which one it reached.** The condition was met, it has not been met yet, and the wait cannot reach it at all are three different results, and a backgrounded wait that emits nothing renders all three identically. Run the command once in the foreground and read its output before backgrounding it, because a wait is only as good as the command inside it, and an unsupported flag on the installed tool version exits non-zero with an empty stdout that every naive test reads as "nothing yet". Never let a fallback stand in for a failed command, since `|| echo '[]'`, `|| true`, and `2>/dev/null` convert an error into that same reading, which is the suppression the write-safety rules already forbid on a mutation. Make the wait emit on failure as loudly as on success, so silence means "still running" and nothing else, and bound it, so a condition that is never coming ends in a report rather than in another wait. ## Where the Rules Live diff --git a/GOVERNANCE.md b/GOVERNANCE.md index 16cb53e1..2b3cf77a 100644 --- a/GOVERNANCE.md +++ b/GOVERNANCE.md @@ -221,6 +221,7 @@ The checks that separate work actually done from work that merely reports succes - **Never edit source through a shell heredoc when the text carries backslash escapes.** The shell consumes the escape and writes an invisible control character in its place, so a `\b` inside a regex becomes a backspace and the pattern silently matches nothing while every test still passes. Use a file-editing tool for such text. When a check inspects text for control characters, use `str.isprintable()` rather than a codepoint floor, since DEL and the Unicode format characters sit above 32 and are equally invisible in a diff. - **Never edit an active `.code-workspace` file.** A workspace file rewritten on disk can make VS Code reload the window, and a reload destroys the running agent session's context, so the work in flight is lost with nothing to catch it, and the trigger is not fully characterized (an agent's edit has caused the reload where a human's identical edit did not). Surface the needed change for the maintainer to apply by hand. - **A green check is not evidence the work happened.** A skipped job and a passing job are indistinguishable in the aggregated required check. When a job exists to exercise something, confirm from its log that it ran and produced the output it promises. +- **A launched process is not a result, and a cause nobody observed is not a diagnosis.** "The watcher is armed" names a process rather than a finding, so what gets reported is the output that process produced, and where it produced none, that absence is the report. The failure it prevents is an agent standing still on a condition that was met half an hour earlier, having announced the wait and never read it. Naming an external cause for such a stall afterwards, a throttle or a quota that appears nowhere in the record, turns a local defect into a story about someone else and closes the investigation on the wrong party, so read the record for the cause before naming one, and where the record does not carry it, report the cause as unknown. - **A workflow change is only fully exercised by CI.** Extracting a `run:` block and executing it locally validates the script and nothing else, because `secrets: inherit`, `permissions:`, `needs:` wiring, and reusable-workflow inputs resolve only in a real run. - **A review flags an instance, so fix the class.** When a reviewer cites one stale claim, one silent-narrowing pattern, or one mis-worded contract, sweep for its siblings before replying. Reviewers sample rather than enumerate. diff --git a/TODO.md b/TODO.md index b6e6a252..adf8df5c 100644 --- a/TODO.md +++ b/TODO.md @@ -17,7 +17,8 @@ Running backlog for this repo, kept in a committed file so the guidance survives - Rework [`spec/readme-structure.md`][readme-structure] to match the hand-crafted PlexCleaner README, which is the shape the maintainer wants, and make the result auditable rather than advisory. Four concrete divergences are already identified, measured against PlexCleaner `README.md`, this repo's `README.md`, and the current spec. First, the distribution bullet is labeled by deliverable: PlexCleaner ships executables and calls the channel **Binary Releases**, while the spec fixes the label as **Versioned Releases** for every repo, so the label belongs in a per-channel table rather than as one string. Second, the license shield sits in the top **Build Status** block here and at the very bottom of PlexCleaner, inside a closing `## License` section that reads `Licensed under the [MIT License]` followed by the shield, immediately before the link definitions. Third, the Release Notes section closes with `See Release History for complete release notes and older versions.` in PlexCleaner against `See Release History for the full history.` here, and the PlexCleaner form is the wanted one. Note that PlexCleaner writes that link inline, which the reference-style rule forbids, so adopt the wording and keep the reference form. Fourth, the channel bullets and their shields vary by deliverable, meaning GitHub binaries, Docker Hub, NuGet, and PyPI each carry a different bullet label and a different shield set, which is what a per-type table has to encode for the `readme-structure` audit dimension to check a repo against its own declared types. - Decide whether the canonical README section order follows PlexCleaner, which is a separate question from the four divergences above and affects every repo plus the `readme-structure` audit. PlexCleaner places **Questions or Issues** immediately after the Table of Contents, where the spec orders it ninth, and it carries sections the spec names nowhere, including Performance Considerations, Runtime Metrics, Custom Plugins, Testing, Development Tooling, Feature Ideas, and Sample Media Files. Under the recurrence rule in [`spec/section-model.md`][section-model] those last ones are correctly repo-specific and stay undeclared, so the open question is only the position of the sections the spec already names. - Declare locally-required secrets the way GitHub-stored ones are already declared, and make a gitignored `secrets/` directory the fleet standard that holds them. [`spec/secrets.json`][secrets] covers only the Actions and Dependabot stores, so a repo that deploys somewhere has no declared way to say what it needs at runtime, and the required set is discoverable only by reading the deploy. The pattern already runs in the fleet in two shapes: HomeAutomation-Config keeps a gitignored secrets directory of env files and Docker secret files, and ESPHome-Config keeps a gitignored `secrets.yaml` beside a committed `_secrets.yaml`. The committed file carries the required names with dummy values, so the shape of the requirement is in git while the values never are, which is the same split the GitHub side already gets from `requiredSecrets[]`. Blog needs it immediately, since it deploys on the proxmox host through HomeAutomation-Config's Docker Compose stack and carries the copy destinations and the internal URI. The hub carries neither the directory nor a `.gitignore` entry for one today, so adopting it here comes first. -- Re-vendor `repo-config/configure.sh` across the fleet. The hub swept it to one sentence per line, and it is carried `verbatim` with `appliesTo: "*"`, so every repo already holding a copy is byte-mismatched against the hub until it takes the new one. +- Re-vendor the changed `verbatim` content across the fleet, which is one sweep covering three files. `repo-config/configure.sh` is carried `verbatim` with `appliesTo: "*"` and the hub swept it to one sentence per line. `AGENTS.md` "Context and Delegation Discipline" carries the wait rule's failure clause, and `GOVERNANCE.md` "Verification Discipline" carries the rule that a launched process is not a result. Every repo already holding a copy is byte-mismatched against the hub until it takes the new one, which the audit reports as stale rather than modified. +- Decide where a carried file may name hub-only machinery, since `GOVERNANCE.md` "PR Review Etiquette" points at `scripts/pr_review.py` and the fleet carries the section but not the script. A downstream reader follows that pointer to a path their repo does not have. Either the script joins the carried set, or the rule states the behavior and drops the tool name the way the coordination-reference rule already requires for the template repo itself. - Make [`prose_lint.py`][prose-lint] assert a floor on its own scope, applying to itself the rule [`GOVERNANCE.md`][governance] already states: a gate that finds nothing is indistinguishable from a gate with nothing to find. A `--diff` run that resolves a non-empty diff and then matches **zero** files has almost certainly failed to scope rather than found a clean change, so it should say so instead of exiting 0. One session produced four separate routes to that same false clean: an unresolvable base widening to a whole-tree scan, a multi-line `paths` input read only to its first newline, a diff taken in one repository while scanning another, and a path under no repository at all. Each was fixed with its own guard, which is the wrong shape, because the fifth route will need a fifth guard and will be found the same way the first four were, by a reviewer rather than by the gate. A floor assertion covers the family. Note the honest limit before building it: a change touching only files the gate does not read (an image, a lock file) legitimately scopes to zero, so the assertion compares against the diff's own file list rather than against zero alone. - Teach the `sha-pin` check in [`repo_gate.py`][repo-gate] to verify a pin **resolves**, not merely that it is shaped like a SHA. Forty hex characters is a format any fabricated string satisfies, and an agent hand-writing a plausible SHA into a workflow is a real failure mode rather than a hypothetical one. A resolvability check also catches the neighboring case, a pin whose commit was reachable only from a branch that has since been squashed and deleted, which breaks a downstream gate long after the change that caused it. Scope the network call to same-owner repositories, where the fleet's own actions live, and skip rather than fail when the host is offline so the local gate stays usable. Note that the existing `gh-write-guard` hook cannot cover this, since it watches Bash and an editor tool writing the same string into a file never reaches it. - Add a check that a pull request's **description** does not contradict its own branch. Three stale descriptions in one session generated six review findings between them, each one a reviewer noticing that the body named a commit, a branch, or a behavior the branch no longer carried. The cheap and precise form is to extract SHAs and `uses:` refs quoted in the body and confirm each still appears in the head tree, since those are the claims that go stale silently and the ones a reviewer actually catches. Prose claims are out of scope, and deliberately so: judging those needs a similarity heuristic, which [`spec/section-model.md`][section-model] already rejects for exactly the reason it would fail here. diff --git a/scripts/README.md b/scripts/README.md index 1ee2f87c..84b074db 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -93,6 +93,8 @@ python3 scripts/pr_review.py wait 452 --timeout 2700 `wait` exits `30` when the review is still pending at the timeout, which is pending rather than failed. Its failure mode is a wrong answer rather than a crash, so the cases feed crafted GraphQL payloads: a review attributed to the wrong login, a review counted against a stale head, a maintainer's own thread read as a finding, and a wait that returns success while nothing landed. One case reads the reviewer login out of the runbook rather than restating it, since GraphQL drops the `[bot]` suffix REST carries, and another asserts no mutation has crept into a read-only script. +`wait` exits `40` when Copilot answers the request with a plain comment rather than a review, meaning a comment of its own that postdates its newest review on the pull request. That shape is the one a refusal takes, a quota or rate-limit message among them, and it satisfies no coverage check by design, so a wait that reads formal reviews alone reads a refusal as an unmet condition and then polls out its whole timeout against an answer that already arrived. The comment prints whole, since its wording is the only thing separating a refusal from an ordinary remark, and a refusal is terminal: no review follows it and re-requesting does not clear it, so `40` ends the wait and hands the text to the maintainer. A comment **older** than the newest review is spent rather than terminal, because the review it preceded did land. The timeout path prints the full digest for the same reason, as a bare `PENDING` line reports a slow reviewer and a broken poll identically, which is the reading that turns a stalled watcher into a watcher nobody notices is stalled. + The digest also reports the **suppressed findings** a review body collapses into a `
` block. Those reach no review thread, so a loop that polls threads alone reports a clean pass while they stand, and the [merge gate][governance] counts them as outstanding findings either way. `suppressed=N` counts findings rather than blocks, reading the `(N)` the heading carries, since one body holds one block per round and counting blocks reports two findings as one. It covers **every** round rather than the current head, because a suppressed finding has no resolved state for a push to retire: head-scoping read "superseded by a push" as "answered", and a finding nobody replied to left the digest the moment the branch moved, so the run reported zero. That is how four rounds went unanswered across three pull requests in one day, each found by the maintainer rather than by this script. The summary line splits the count as `suppressed=N (on_head=N earlier=N)` and each block is marked with the round that raised it, since a finding on an older round may since be moot and deciding that is the reader's call rather than one the count should make for them. Each block prints whole where a thread body truncates, because a thread can be re-read at its id and a suppressed finding cannot, and it prints under a marker naming what closing it takes: no thread exists to reply on or resolve, so the answer goes in the PR conversation. The match is on the block's heading rather than anywhere in the body, and on the runbook's alternation rather than on one phrasing, since the wording has already appeared two ways. A case asserts the script's pattern is the one the runbook publishes rather than a copy of it that can drift. Reading the whole body was the first implementation and its own review caught it: a review whose overview prose discusses suppressed findings carries none, and reporting that as a finding trains the reader to skim the field. A heading outside any `
` wrapper is still read, because reporting zero when the markup moves is the same false clean one level up, and that fallback takes a count so ordinary prose does not become one. diff --git a/scripts/pr_review.py b/scripts/pr_review.py index 067d4afa..58604d0e 100644 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -11,7 +11,10 @@ status One digest line, any unresolved threads, and any suppressed findings. Read-only. wait Poll until Copilot's review lands on the current head, then print the digest. The loop runs in-process, so a 45-minute wait costs one agent turn, not 90. - Exit 0 = review present, 30 = still pending at timeout (pending is not failure). + Exit 0 = review present, 30 = still pending at timeout (pending is not failure), + 40 = Copilot answered outside a formal review, so read the printed body. + A quota or rate-limit refusal is terminal: no review lands and waiting on + makes no difference, so 40 ends the wait rather than extending it. Read-only by design. Mutations (re-request review, reply, resolve thread) are deliberately NOT implemented here - they are state-changing calls that must stay @@ -24,22 +27,25 @@ REVIEWER = 'copilot-pull-request-reviewer' # A review body can carry a collapsed block of findings withheld from the inline threads. -# Those appear nowhere in `reviewThreads`, so polling threads alone reports a clean pass while -# they stand. The alternation is the runbook's, because the heading wording has changed once -# already, and matching one phrasing alone reports zero on a review that has them. +# Those appear nowhere in `reviewThreads`, so polling threads alone reports a clean pass. +# The alternation is the runbook's, since the heading wording has changed once already. +# Matching one phrasing alone reports zero on a review that has them. SUPPRESSED = re.compile(r'Suppressed comments|low confidence', re.IGNORECASE) DETAILS = re.compile(r'
(.*?)
', re.DOTALL | re.IGNORECASE) SUMMARY = re.compile(r'(.*?)', re.DOTALL | re.IGNORECASE) TAGS = re.compile(r'', re.IGNORECASE) COUNT = re.compile(r'\((\d+)\)') -# Liveness query: two scalars only, no comment bodies. +# Liveness query: timestamps and ids only, no comment or review bodies. # A liveness check does not need the finding text, and re-fetching bodies was 76% of polls. +# It does need the reviewer's non-review answers. +# A wait reading formal reviews alone treats a refusal as an unmet condition. Q_LIVE = """ query($o:String!,$r:String!,$n:Int!){ repository(owner:$o,name:$r){ pullRequest(number:$n){ headRefOid - reviews(last:20){ nodes{ author{login} state commit{oid} } } + reviews(last:20){ nodes{ author{login} state commit{oid} submittedAt } } + comments(last:5){ nodes{ author{login} createdAt } } }}} """ @@ -51,6 +57,7 @@ reviews(last:20){ nodes{ author{login} state commit{oid} submittedAt body } } reviewThreads(first:100){ nodes{ id isResolved comments(first:1){ nodes{ author{login} path line body } } }} + comments(last:5){ nodes{ author{login} createdAt body } } }}} """ @@ -66,14 +73,36 @@ def gql(query: str, owner: str, repo: str, num: int) -> dict: return json.loads(r.stdout)['data']['repository']['pullRequest'] -def live_state(owner: str, repo: str, num: int) -> tuple[str, bool]: - """Return (head_sha, copilot_reviewed_current_head).""" +def reviewer_nodes(pr: dict, field: str) -> list[dict]: + """The reviewer's own nodes under `field`, oldest first as the API returns them.""" + return [n for n in ((pr.get(field) or {}).get('nodes') or []) + if (n.get('author') or {}).get('login') == REVIEWER] + + +def answered_outside_review(pr: dict) -> dict | None: + """The reviewer's newest plain comment, where it postdates its newest formal review. + + Copilot answers a request with a comment rather than a review when it will not review at all, + a quota refusal among them, and that answer satisfies no coverage check by design. + Reading it as an unmet condition is what turns a refusal into a wait with nothing at its end. + A comment older than the newest review is spent, since the review it preceded did land. + """ + comments = reviewer_nodes(pr, 'comments') + if not comments: + return None + newest = max(comments, key=lambda n: n.get('createdAt') or '') + reviews = reviewer_nodes(pr, 'reviews') + latest_review = max((n.get('submittedAt') or '' for n in reviews), default='') + return newest if (newest.get('createdAt') or '') > latest_review else None + + +def live_state(owner: str, repo: str, num: int) -> tuple[str, bool, dict | None]: + """Return (head_sha, copilot_reviewed_current_head, copilot_answer_outside_a_review).""" pr = gql(Q_LIVE, owner, repo, num) head = pr['headRefOid'] - done = any((n.get('author') or {}).get('login') == REVIEWER - and (n.get('commit') or {}).get('oid') == head - for n in pr['reviews']['nodes']) - return head, done + done = any((n.get('commit') or {}).get('oid') == head + for n in reviewer_nodes(pr, 'reviews')) + return head, done, answered_outside_review(pr) def heading_of(block: str) -> str: @@ -110,8 +139,7 @@ def finding_count(block: str) -> int: def digest(owner: str, repo: str, num: int, seen: set[str] | None = None) -> tuple[str, int]: pr = gql(Q_FULL, owner, repo, num) head = pr['headRefOid'] - revs = [n for n in pr['reviews']['nodes'] - if (n.get('author') or {}).get('login') == REVIEWER] + revs = reviewer_nodes(pr, 'reviews') on_head = [n for n in revs if (n.get('commit') or {}).get('oid') == head] threads = pr['reviewThreads']['nodes'] unresolved = [t for t in threads @@ -130,14 +158,24 @@ def digest(owner: str, repo: str, num: int, seen: set[str] | None = None) -> tup stale = sum(finding_count(b) for n, b in blocks) - sum( finding_count(b) for b in on_head_blocks) + answer = answered_outside_review(pr) lines = [ f'pr={num} head={head[:8]} rounds={len(revs)} ' f'review_on_head={"yes" if on_head else "NO"} ' f'threads={len(threads)} unresolved={len(unresolved)} ' f'suppressed={sum(finding_count(b) for n, b in blocks)} ' f'(on_head={sum(finding_count(b) for b in on_head_blocks)} earlier={stale}) ' + f'answered_outside_review={"yes" if answer else "no"} ' f'merge={pr.get("mergeStateStatus")}' ] + if answer: + # Printed whole for the same reason a suppressed finding is, since it reaches no thread. + # Its wording is the only thing separating a refusal from an ordinary remark. + lines.append(f' COPILOT COMMENT ({answer.get("createdAt")}, newer than any review): ' + 'read it before waiting again, since a quota or rate-limit refusal is ' + 'terminal and no review follows it') + lines += [f' {ln.rstrip()}' for ln in (answer.get('body') or '').splitlines() + if ln.strip()] new = 0 for t in unresolved: c = (t.get('comments') or {}).get('nodes', [{}])[0] @@ -152,8 +190,8 @@ def digest(owner: str, repo: str, num: int, seen: set[str] | None = None) -> tup body = ' '.join((c.get('body') or '').split()) lines.append(f' {mark}{tid} {c.get("path")}:{c.get("line")} {body[:160]}') for n, b in blocks: - # Printed whole where a thread body is truncated: a thread can be re-read at its id, - # while a suppressed finding has no thread, so this digest is the only place it appears. + # Printed whole where a thread body is truncated, since a thread can be re-read at its id. + # A suppressed finding has none, so this digest is the only place it appears. # GraphQL returns a null commit for a pending or partial review. # An empty sha rendered as "raised on , earlier round", losing what traces the finding. sha = ((n.get('commit') or {}).get('oid') or '')[:8] @@ -189,20 +227,27 @@ def main(argv: list[str] | None = None) -> int: # In-process backoff, so the whole wait costs one agent turn. delays = [15, 20, 30, 45, 60, 120] start = time.monotonic() - head0, done = live_state(owner, repo, a.number) + _, done, answer = live_state(owner, repo, a.number) i = 0 - while not done: + while not done and not answer: if time.monotonic() - start > a.timeout: - print(f'pr={a.number} head={head0[:8]} review_on_head=NO ' - f'status=PENDING waited={int(time.monotonic()-start)}s') + # The timeout is where the digest's one extra call is worth most. + # A bare PENDING line reports a broken wait and a slow reviewer identically. + out, _ = digest(owner, repo, a.number) + print(out) + print(f'status=PENDING waited={int(time.monotonic()-start)}s') return 30 time.sleep(delays[min(i, len(delays) - 1)]) i += 1 # Re-read head each iteration: a push during the wait moves it. - head0, done = live_state(owner, repo, a.number) + _, done, answer = live_state(owner, repo, a.number) out, _ = digest(owner, repo, a.number) print(out) print(f'waited={int(time.monotonic()-start)}s') + if not done: + print('status=ANSWERED_OUTSIDE_REVIEW read the comment above, ' + 'a quota or rate-limit refusal is terminal and re-requesting does not clear it') + return 40 return 0 diff --git a/scripts/test_pr_review.py b/scripts/test_pr_review.py index 5f722356..fbee6f77 100644 --- a/scripts/test_pr_review.py +++ b/scripts/test_pr_review.py @@ -22,9 +22,19 @@ OLD = 'b' * 40 -def review(login: str = pr_review.REVIEWER, oid: str = HEAD, body: str = '') -> dict: +EARLY = '2026-08-02T10:00:00Z' +LATE = '2026-08-02T11:00:00Z' + + +def review(login: str = pr_review.REVIEWER, oid: str = HEAD, body: str = '', + at: str = EARLY) -> dict: return {'author': {'login': login}, 'state': 'COMMENTED', 'commit': {'oid': oid}, - 'body': body} + 'body': body, 'submittedAt': at} + + +def comment(login: str = pr_review.REVIEWER, at: str = LATE, + body: str = 'I have reached my quota limit and cannot review this now.') -> dict: + return {'author': {'login': login}, 'createdAt': at, 'body': body} def collapsed(heading: str = 'Comments suppressed due to low confidence (1)', @@ -41,9 +51,10 @@ def thread(tid: str, resolved: bool = False, login: str = pr_review.REVIEWER, def payload(reviews: list[dict], threads: list[dict] | None = None, - merge: str = 'CLEAN') -> dict: + merge: str = 'CLEAN', comments: list[dict] | None = None) -> dict: return {'headRefOid': HEAD, 'mergeable': 'MERGEABLE', 'mergeStateStatus': merge, - 'reviews': {'nodes': reviews}, 'reviewThreads': {'nodes': threads or []}} + 'reviews': {'nodes': reviews}, 'reviewThreads': {'nodes': threads or []}, + 'comments': {'nodes': comments or []}} class GqlCase(unittest.TestCase): @@ -71,12 +82,37 @@ def test_the_review_must_be_the_reviewer_and_on_the_current_head(self) -> None: ): with self.subTest(case=label): self.answer(payload(reviews)) - self.assertEqual((HEAD, want), pr_review.live_state('o', 'r', 1)) + self.assertEqual((HEAD, want, None), pr_review.live_state('o', 'r', 1)) def test_a_null_author_or_commit_does_not_raise(self) -> None: """GraphQL returns null for a deleted account, and a crash there stalls the whole wait.""" self.answer(payload([{'author': None, 'state': 'COMMENTED', 'commit': None}])) - self.assertEqual((HEAD, False), pr_review.live_state('o', 'r', 1)) + self.assertEqual((HEAD, False, None), pr_review.live_state('o', 'r', 1)) + + +class TestAnsweredOutsideReview(unittest.TestCase): + """A refusal answers the request without covering the head, so a wait cannot read it as pending.""" + + def test_a_reviewer_comment_newer_than_every_review_is_the_answer(self) -> None: + answer = pr_review.answered_outside_review( + payload([review(oid=OLD, at=EARLY)], comments=[comment(at=LATE)])) + self.assertIsNotNone(answer) + self.assertEqual(LATE, answer['createdAt']) + + def test_an_answer_the_reviewer_then_superseded_is_spent(self) -> None: + """The review it preceded did land, so the comment is history rather than a stop signal.""" + self.assertIsNone(pr_review.answered_outside_review( + payload([review(at=LATE)], comments=[comment(at=EARLY)]))) + + def test_another_account_s_comment_is_not_the_reviewer_answering(self) -> None: + """A maintainer note and a codecov post both postdate the review and mean nothing here.""" + for login in ('ptr727', 'codecov[bot]', 'copilot-swe-agent'): + with self.subTest(login=login): + self.assertIsNone(pr_review.answered_outside_review( + payload([review(oid=OLD)], comments=[comment(login=login)]))) + + def test_no_comments_at_all_reads_as_no_answer(self) -> None: + self.assertIsNone(pr_review.answered_outside_review(payload([review(oid=OLD)]))) class TestDigest(GqlCase): @@ -229,6 +265,23 @@ def test_a_human_review_carrying_the_phrase_is_not_a_copilot_finding(self) -> No self.assertIn('suppressed=0', out) +class TestDigestReportsTheAnswer(GqlCase): + def test_the_comment_prints_whole_under_a_marker_naming_it_terminal(self) -> None: + """Its wording is what separates a refusal from a remark, so it is not truncated.""" + text = 'Copilot has reached its quota limit.\nTry again after the window resets.' + self.answer(payload([review(oid=OLD)], comments=[comment(body=text)])) + out, _ = pr_review.digest('o', 'r', 7) + self.assertIn('answered_outside_review=yes', out) + self.assertIn('COPILOT COMMENT', out) + for line in text.splitlines(): + self.assertIn(line, out) + + def test_a_pull_request_with_no_such_answer_says_so_rather_than_staying_silent(self) -> None: + self.answer(payload([review()])) + out, _ = pr_review.digest('o', 'r', 7) + self.assertIn('answered_outside_review=no', out) + + class TestGqlTransport(unittest.TestCase): def test_a_failed_call_raises_rather_than_returning_an_empty_reading(self) -> None: """Returning nothing on failure would read as a PR with no reviews and no threads.""" @@ -279,6 +332,35 @@ def test_wait_exits_thirty_at_the_timeout_rather_than_reporting_success(self) -> self.assertEqual(30, pr_review.main(['wait', '7', '--timeout', '0'])) self.assertIn('status=PENDING', self.out.getvalue()) + def test_the_timeout_carries_the_digest_rather_than_a_bare_pending_line(self) -> None: + """A wait that ends with no evidence reports a slow reviewer and a broken poll alike.""" + self.answer(payload([review(oid=OLD)], [thread('T1')])) + with mock.patch.object(pr_review.time, 'sleep'): + self.assertEqual(30, pr_review.main(['wait', '7', '--timeout', '0'])) + out = self.out.getvalue() + self.assertIn('review_on_head=NO', out) + self.assertIn('unresolved=1', out) + + def test_wait_ends_on_an_answer_outside_a_review_instead_of_waiting_it_out(self) -> None: + """A refusal covers no head, so polling on for the timeout waits for nothing. + + The zero timeout is what this case fails on rather than hangs on: an answer read as + pending spins the loop for the whole default wait, and a case that hangs gates nothing. + """ + self.answer(payload([review(oid=OLD)], comments=[comment()])) + with mock.patch.object(pr_review.time, 'sleep') as slept: + self.assertEqual(40, pr_review.main(['wait', '7', '--timeout', '0'])) + slept.assert_not_called() + out = self.out.getvalue() + self.assertIn('status=ANSWERED_OUTSIDE_REVIEW', out) + self.assertIn('quota', out) + + def test_a_landed_review_wins_over_an_older_answer(self) -> None: + """Coverage is the success case, and a spent comment does not downgrade it to 40.""" + self.answer(payload([review(at=LATE)], comments=[comment(at=EARLY)])) + with mock.patch.object(pr_review.time, 'sleep'): + self.assertEqual(0, pr_review.main(['wait', '7'])) + def test_the_repo_argument_splits_into_owner_and_name(self) -> None: self.answer(payload([review()])) with mock.patch.object(pr_review, 'digest', return_value=('x', 0)) as dig: From b404164e5e34635180bd680a9d2c76a4c228c9fd Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sun, 2 Aug 2026 20:28:49 -0700 Subject: [PATCH 2/8] Widen the comment window, and report a full one as unknown The queries read the newest comments rather than the reviewer's, since GraphQL offers no author filter, so ordinary discussion is what pushes an answer out of reach. At five comments a codecov post and two replies were enough, and the digest then reported no answer on a pull request carrying one, which is the false clean the field exists to prevent. A full window is reported as unknown rather than as no, because that is the one case where finding nothing and having nothing to find are the same reading, and a case holds the guard's number equal to the one the queries carry so a drift between them cannot read clean. Co-Authored-By: Claude Opus 5 (1M context) --- scripts/README.md | 2 +- scripts/pr_review.py | 25 ++++++++++++++++++++++--- scripts/test_pr_review.py | 31 ++++++++++++++++++++++++++++++- 3 files changed, 53 insertions(+), 5 deletions(-) diff --git a/scripts/README.md b/scripts/README.md index 84b074db..024dcb98 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -93,7 +93,7 @@ python3 scripts/pr_review.py wait 452 --timeout 2700 `wait` exits `30` when the review is still pending at the timeout, which is pending rather than failed. Its failure mode is a wrong answer rather than a crash, so the cases feed crafted GraphQL payloads: a review attributed to the wrong login, a review counted against a stale head, a maintainer's own thread read as a finding, and a wait that returns success while nothing landed. One case reads the reviewer login out of the runbook rather than restating it, since GraphQL drops the `[bot]` suffix REST carries, and another asserts no mutation has crept into a read-only script. -`wait` exits `40` when Copilot answers the request with a plain comment rather than a review, meaning a comment of its own that postdates its newest review on the pull request. That shape is the one a refusal takes, a quota or rate-limit message among them, and it satisfies no coverage check by design, so a wait that reads formal reviews alone reads a refusal as an unmet condition and then polls out its whole timeout against an answer that already arrived. The comment prints whole, since its wording is the only thing separating a refusal from an ordinary remark, and a refusal is terminal: no review follows it and re-requesting does not clear it, so `40` ends the wait and hands the text to the maintainer. A comment **older** than the newest review is spent rather than terminal, because the review it preceded did land. The timeout path prints the full digest for the same reason, as a bare `PENDING` line reports a slow reviewer and a broken poll identically, which is the reading that turns a stalled watcher into a watcher nobody notices is stalled. +`wait` exits `40` when Copilot answers the request with a plain comment rather than a review, meaning a comment of its own that postdates its newest review on the pull request. That shape is the one a refusal takes, a quota or rate-limit message among them, and it satisfies no coverage check by design, so a wait that reads formal reviews alone reads a refusal as an unmet condition and then polls out its whole timeout against an answer that already arrived. The comment prints whole, since its wording is the only thing separating a refusal from an ordinary remark, and a refusal is terminal: no review follows it and re-requesting does not clear it, so `40` ends the wait and hands the text to the maintainer. A comment **older** than the newest review is spent rather than terminal, because the review it preceded did land. Both queries read the newest `COMMENT_WINDOW` comments rather than the reviewer's own, since GraphQL offers no author filter, so ordinary discussion is what pushes an answer out of reach and the window is wide enough that it takes a hundred comments to do it. A **full** window reports `answered_outside_review=unknown` rather than `no`, because that is the one case where finding nothing and having nothing to find are the same reading, and a case holds the guard's number equal to the one the queries carry so a drift between them cannot read clean. The timeout path prints the full digest for the same reason, as a bare `PENDING` line reports a slow reviewer and a broken poll identically, which is the reading that turns a stalled watcher into a watcher nobody notices is stalled. The digest also reports the **suppressed findings** a review body collapses into a `
` block. Those reach no review thread, so a loop that polls threads alone reports a clean pass while they stand, and the [merge gate][governance] counts them as outstanding findings either way. `suppressed=N` counts findings rather than blocks, reading the `(N)` the heading carries, since one body holds one block per round and counting blocks reports two findings as one. It covers **every** round rather than the current head, because a suppressed finding has no resolved state for a push to retire: head-scoping read "superseded by a push" as "answered", and a finding nobody replied to left the digest the moment the branch moved, so the run reported zero. That is how four rounds went unanswered across three pull requests in one day, each found by the maintainer rather than by this script. The summary line splits the count as `suppressed=N (on_head=N earlier=N)` and each block is marked with the round that raised it, since a finding on an older round may since be moot and deciding that is the reader's call rather than one the count should make for them. Each block prints whole where a thread body truncates, because a thread can be re-read at its id and a suppressed finding cannot, and it prints under a marker naming what closing it takes: no thread exists to reply on or resolve, so the answer goes in the PR conversation. diff --git a/scripts/pr_review.py b/scripts/pr_review.py index 58604d0e..5e89ced5 100644 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -36,6 +36,11 @@ TAGS = re.compile(r'', re.IGNORECASE) COUNT = re.compile(r'\((\d+)\)') +# How many of the newest comments both queries read, and the number the saturation guard tests. +# A narrow window drops the reviewer's answer behind ordinary discussion, reporting no answer. +# A test holds this equal to the number the queries carry, since a drift between them reads clean. +COMMENT_WINDOW = 100 + # Liveness query: timestamps and ids only, no comment or review bodies. # A liveness check does not need the finding text, and re-fetching bodies was 76% of polls. # It does need the reviewer's non-review answers. @@ -45,7 +50,7 @@ repository(owner:$o,name:$r){ pullRequest(number:$n){ headRefOid reviews(last:20){ nodes{ author{login} state commit{oid} submittedAt } } - comments(last:5){ nodes{ author{login} createdAt } } + comments(last:100){ nodes{ author{login} createdAt } } }}} """ @@ -57,7 +62,7 @@ reviews(last:20){ nodes{ author{login} state commit{oid} submittedAt body } } reviewThreads(first:100){ nodes{ id isResolved comments(first:1){ nodes{ author{login} path line body } } }} - comments(last:5){ nodes{ author{login} createdAt body } } + comments(last:100){ nodes{ author{login} createdAt body } } }}} """ @@ -96,6 +101,16 @@ def answered_outside_review(pr: dict) -> dict | None: return newest if (newest.get('createdAt') or '') > latest_review else None +def answer_window_saturated(pr: dict) -> bool: + """True where the window is full, so an answer sitting behind it cannot be ruled out. + + The query reads the newest comments rather than the reviewer's, so ordinary discussion is + what pushes an answer out of reach, and a full window is the one case where finding nothing + and having nothing to find are the same reading. + """ + return len(((pr.get('comments') or {}).get('nodes') or [])) >= COMMENT_WINDOW + + def live_state(owner: str, repo: str, num: int) -> tuple[str, bool, dict | None]: """Return (head_sha, copilot_reviewed_current_head, copilot_answer_outside_a_review).""" pr = gql(Q_LIVE, owner, repo, num) @@ -159,15 +174,19 @@ def digest(owner: str, repo: str, num: int, seen: set[str] | None = None) -> tup finding_count(b) for b in on_head_blocks) answer = answered_outside_review(pr) + answered = 'yes' if answer else ('unknown' if answer_window_saturated(pr) else 'no') lines = [ f'pr={num} head={head[:8]} rounds={len(revs)} ' f'review_on_head={"yes" if on_head else "NO"} ' f'threads={len(threads)} unresolved={len(unresolved)} ' f'suppressed={sum(finding_count(b) for n, b in blocks)} ' f'(on_head={sum(finding_count(b) for b in on_head_blocks)} earlier={stale}) ' - f'answered_outside_review={"yes" if answer else "no"} ' + f'answered_outside_review={answered} ' f'merge={pr.get("mergeStateStatus")}' ] + if answered == 'unknown': + lines.append(f' COMMENT WINDOW FULL: the newest {COMMENT_WINDOW} comments carry none ' + 'from the reviewer, so an older answer cannot be ruled out from here') if answer: # Printed whole for the same reason a suppressed finding is, since it reaches no thread. # Its wording is the only thing separating a refusal from an ordinary remark. diff --git a/scripts/test_pr_review.py b/scripts/test_pr_review.py index fbee6f77..75327df6 100644 --- a/scripts/test_pr_review.py +++ b/scripts/test_pr_review.py @@ -9,7 +9,7 @@ Run as `python3 scripts/test_pr_review.py`, or under `python3 -m unittest discover -s scripts`. """ from __future__ import annotations -import contextlib, io, json, subprocess, sys, unittest +import contextlib, io, json, re, subprocess, sys, unittest from pathlib import Path from unittest import mock @@ -114,6 +114,19 @@ def test_another_account_s_comment_is_not_the_reviewer_answering(self) -> None: def test_no_comments_at_all_reads_as_no_answer(self) -> None: self.assertIsNone(pr_review.answered_outside_review(payload([review(oid=OLD)]))) + def test_ordinary_discussion_does_not_push_the_answer_out_of_the_window(self) -> None: + """The window reads the newest comments, not the reviewer's, so others crowd it.""" + chatter = [comment(login='ptr727', at=LATE) for _ in range(pr_review.COMMENT_WINDOW - 1)] + found = pr_review.answered_outside_review( + payload([review(oid=OLD, at=EARLY)], comments=[comment(at=LATE)] + chatter)) + self.assertIsNotNone(found) + + def test_a_full_window_is_unknown_rather_than_no_answer(self) -> None: + """Finding nothing and having nothing to find are one reading once the window fills.""" + full = [comment(login='ptr727') for _ in range(pr_review.COMMENT_WINDOW)] + self.assertTrue(pr_review.answer_window_saturated(payload([review()], comments=full))) + self.assertFalse(pr_review.answer_window_saturated(payload([review()], comments=full[:-1]))) + class TestDigest(GqlCase): def test_the_summary_line_counts_what_it_names(self) -> None: @@ -281,6 +294,15 @@ def test_a_pull_request_with_no_such_answer_says_so_rather_than_staying_silent(s out, _ = pr_review.digest('o', 'r', 7) self.assertIn('answered_outside_review=no', out) + def test_a_full_window_reports_unknown_and_names_why(self) -> None: + """Reporting `no` off a saturated window is the false clean this field exists to avoid.""" + self.answer(payload([review()], + comments=[comment(login='ptr727') + for _ in range(pr_review.COMMENT_WINDOW)])) + out, _ = pr_review.digest('o', 'r', 7) + self.assertIn('answered_outside_review=unknown', out) + self.assertIn('COMMENT WINDOW FULL', out) + class TestGqlTransport(unittest.TestCase): def test_a_failed_call_raises_rather_than_returning_an_empty_reading(self) -> None: @@ -392,6 +414,13 @@ def test_no_mutation_reaches_this_script(self) -> None: with self.subTest(verb=verb): self.assertFalse(verb in source, f'{verb!r} is a state-changing call in a read-only script') + def test_the_guard_tests_the_window_the_queries_actually_read(self) -> None: + """A guard measuring one number while the query fetches another reads clean on drift.""" + source = (REPO / 'scripts' / 'pr_review.py').read_text(encoding='utf-8') + windows = set(re.findall(r'comments\(last:(\d+)\)', source)) + self.assertEqual({str(pr_review.COMMENT_WINDOW)}, windows) + self.assertEqual(2, source.count(f'comments(last:{pr_review.COMMENT_WINDOW})')) + def test_the_backoff_is_bounded_and_non_decreasing(self) -> None: """A wait that sleeps zero seconds is a busy loop, and one that shrinks polls harder later.""" source = (REPO / 'scripts' / 'pr_review.py').read_text(encoding='utf-8') From c24c2fc242904c81cc868596d71413ea9d764f44 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sun, 2 Aug 2026 20:35:37 -0700 Subject: [PATCH 3/8] Report the shape of an answer, not a cause nothing reads Three of the messages around exit 40 named a quota or rate limit as the reason the reviewer answered without reviewing, while the code compares timestamps and reads no word of the body. That is the rule this same branch adds to GOVERNANCE.md, that a cause nobody observed is not a diagnosis, written into the tool that reports it. The marker, the status line, the docstring and the module header now state what is known: a comment carries no commit, so it covers no head whatever it says, and the reader decides whether it is a refusal, which is terminal, or a remark, which is not. The class is swept rather than the three instances, so `scripts/README.md` gains the same distinction. A deleted account also crashed the digest. GraphQL sends `author` present and null, which `.get('author', {})` returns as None rather than as the default, so the chained lookup raised and took the whole wait with it. Co-Authored-By: Claude Opus 5 (1M context) --- scripts/README.md | 2 +- scripts/pr_review.py | 25 ++++++++++++++----------- scripts/test_pr_review.py | 11 ++++++++++- 3 files changed, 25 insertions(+), 13 deletions(-) diff --git a/scripts/README.md b/scripts/README.md index 024dcb98..35958eab 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -93,7 +93,7 @@ python3 scripts/pr_review.py wait 452 --timeout 2700 `wait` exits `30` when the review is still pending at the timeout, which is pending rather than failed. Its failure mode is a wrong answer rather than a crash, so the cases feed crafted GraphQL payloads: a review attributed to the wrong login, a review counted against a stale head, a maintainer's own thread read as a finding, and a wait that returns success while nothing landed. One case reads the reviewer login out of the runbook rather than restating it, since GraphQL drops the `[bot]` suffix REST carries, and another asserts no mutation has crept into a read-only script. -`wait` exits `40` when Copilot answers the request with a plain comment rather than a review, meaning a comment of its own that postdates its newest review on the pull request. That shape is the one a refusal takes, a quota or rate-limit message among them, and it satisfies no coverage check by design, so a wait that reads formal reviews alone reads a refusal as an unmet condition and then polls out its whole timeout against an answer that already arrived. The comment prints whole, since its wording is the only thing separating a refusal from an ordinary remark, and a refusal is terminal: no review follows it and re-requesting does not clear it, so `40` ends the wait and hands the text to the maintainer. A comment **older** than the newest review is spent rather than terminal, because the review it preceded did land. Both queries read the newest `COMMENT_WINDOW` comments rather than the reviewer's own, since GraphQL offers no author filter, so ordinary discussion is what pushes an answer out of reach and the window is wide enough that it takes a hundred comments to do it. A **full** window reports `answered_outside_review=unknown` rather than `no`, because that is the one case where finding nothing and having nothing to find are the same reading, and a case holds the guard's number equal to the one the queries carry so a drift between them cannot read clean. The timeout path prints the full digest for the same reason, as a bare `PENDING` line reports a slow reviewer and a broken poll identically, which is the reading that turns a stalled watcher into a watcher nobody notices is stalled. +`wait` exits `40` when Copilot answers the request with a plain comment rather than a review, meaning a comment of its own that postdates its newest review on the pull request. The test is the **shape** of that answer and not its cause, which the script reads nothing of: a comment carries no commit, so it satisfies no coverage check whatever it says, and a wait reading formal reviews alone treats it as an unmet condition and then polls out its whole timeout against an answer that already arrived. A refusal is the case that makes this worth catching, a quota or rate-limit message among them, and `40` neither asserts nor detects one. The comment prints whole because its wording is the only thing separating a refusal, which is terminal since no review follows it and re-requesting does not clear it, from an ordinary remark that is not, so `40` ends the wait and hands the text to the reader who can tell them apart. A comment **older** than the newest review is spent rather than terminal, because the review it preceded did land. Both queries read the newest `COMMENT_WINDOW` comments rather than the reviewer's own, since GraphQL offers no author filter, so ordinary discussion is what pushes an answer out of reach and the window is wide enough that it takes a hundred comments to do it. A **full** window reports `answered_outside_review=unknown` rather than `no`, because that is the one case where finding nothing and having nothing to find are the same reading, and a case holds the guard's number equal to the one the queries carry so a drift between them cannot read clean. The timeout path prints the full digest for the same reason, as a bare `PENDING` line reports a slow reviewer and a broken poll identically, which is the reading that turns a stalled watcher into a watcher nobody notices is stalled. The digest also reports the **suppressed findings** a review body collapses into a `
` block. Those reach no review thread, so a loop that polls threads alone reports a clean pass while they stand, and the [merge gate][governance] counts them as outstanding findings either way. `suppressed=N` counts findings rather than blocks, reading the `(N)` the heading carries, since one body holds one block per round and counting blocks reports two findings as one. It covers **every** round rather than the current head, because a suppressed finding has no resolved state for a push to retire: head-scoping read "superseded by a push" as "answered", and a finding nobody replied to left the digest the moment the branch moved, so the run reported zero. That is how four rounds went unanswered across three pull requests in one day, each found by the maintainer rather than by this script. The summary line splits the count as `suppressed=N (on_head=N earlier=N)` and each block is marked with the round that raised it, since a finding on an older round may since be moot and deciding that is the reader's call rather than one the count should make for them. Each block prints whole where a thread body truncates, because a thread can be re-read at its id and a suppressed finding cannot, and it prints under a marker naming what closing it takes: no thread exists to reply on or resolve, so the answer goes in the PR conversation. diff --git a/scripts/pr_review.py b/scripts/pr_review.py index 5e89ced5..de69711a 100644 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -13,8 +13,8 @@ The loop runs in-process, so a 45-minute wait costs one agent turn, not 90. Exit 0 = review present, 30 = still pending at timeout (pending is not failure), 40 = Copilot answered outside a formal review, so read the printed body. - A quota or rate-limit refusal is terminal: no review lands and waiting on - makes no difference, so 40 ends the wait rather than extending it. + 40 reports the shape of that answer and reads nothing of its cause: an answer + carrying no commit covers no head, so the wait ends and the reader decides. Read-only by design. Mutations (re-request review, reply, resolve thread) are deliberately NOT implemented here - they are state-changing calls that must stay @@ -87,9 +87,9 @@ def reviewer_nodes(pr: dict, field: str) -> list[dict]: def answered_outside_review(pr: dict) -> dict | None: """The reviewer's newest plain comment, where it postdates its newest formal review. - Copilot answers a request with a comment rather than a review when it will not review at all, - a quota refusal among them, and that answer satisfies no coverage check by design. - Reading it as an unmet condition is what turns a refusal into a wait with nothing at its end. + The test is the shape of the answer rather than its cause, which this reads nothing of: + a comment carries no commit, so it satisfies no coverage check whatever it says. + Treating that shape as an unmet condition is what leaves a wait with nothing at its end. A comment older than the newest review is spent, since the review it preceded did land. """ comments = reviewer_nodes(pr, 'comments') @@ -157,10 +157,12 @@ def digest(owner: str, repo: str, num: int, seen: set[str] | None = None) -> tup revs = reviewer_nodes(pr, 'reviews') on_head = [n for n in revs if (n.get('commit') or {}).get('oid') == head] threads = pr['reviewThreads']['nodes'] + # A deleted account leaves `author` present and null, which `.get('author', {})` returns as + # None rather than as the default, so the chained lookup crashes the whole digest. unresolved = [t for t in threads if not t['isResolved'] - and ((t.get('comments') or {}).get('nodes') or [{}])[0] - .get('author', {}).get('login') == REVIEWER] + and ((((t.get('comments') or {}).get('nodes') or [{}])[0] + .get('author') or {}).get('login') == REVIEWER)] # Every round, not just the head, because a suppressed finding has no resolved state to read. # Head-scoping treated "superseded by a push" as "answered", and the two are not the same. @@ -191,8 +193,8 @@ def digest(owner: str, repo: str, num: int, seen: set[str] | None = None) -> tup # Printed whole for the same reason a suppressed finding is, since it reaches no thread. # Its wording is the only thing separating a refusal from an ordinary remark. lines.append(f' COPILOT COMMENT ({answer.get("createdAt")}, newer than any review): ' - 'read it before waiting again, since a quota or rate-limit refusal is ' - 'terminal and no review follows it') + 'the reviewer answered without reviewing, so read the body below and ' + 'decide, since a refusal is terminal and a remark is not') lines += [f' {ln.rstrip()}' for ln in (answer.get('body') or '').splitlines() if ln.strip()] new = 0 @@ -264,8 +266,9 @@ def main(argv: list[str] | None = None) -> int: print(out) print(f'waited={int(time.monotonic()-start)}s') if not done: - print('status=ANSWERED_OUTSIDE_REVIEW read the comment above, ' - 'a quota or rate-limit refusal is terminal and re-requesting does not clear it') + print('status=ANSWERED_OUTSIDE_REVIEW the reviewer answered without reviewing, ' + 'so read the comment above and decide, since where it declines or names a limit ' + 'no review follows and re-requesting does not clear it') return 40 return 0 diff --git a/scripts/test_pr_review.py b/scripts/test_pr_review.py index 75327df6..ca9484a1 100644 --- a/scripts/test_pr_review.py +++ b/scripts/test_pr_review.py @@ -97,7 +97,7 @@ def test_a_reviewer_comment_newer_than_every_review_is_the_answer(self) -> None: answer = pr_review.answered_outside_review( payload([review(oid=OLD, at=EARLY)], comments=[comment(at=LATE)])) self.assertIsNotNone(answer) - self.assertEqual(LATE, answer['createdAt']) + self.assertEqual(LATE, (answer or {}).get('createdAt')) def test_an_answer_the_reviewer_then_superseded_is_spent(self) -> None: """The review it preceded did land, so the comment is history rather than a stop signal.""" @@ -148,6 +148,15 @@ def test_review_on_head_reports_no_when_every_round_is_stale(self) -> None: out, _ = pr_review.digest('o', 'r', 7) self.assertIn('review_on_head=NO', out) + def test_a_thread_from_a_deleted_account_does_not_crash_the_digest(self) -> None: + """GraphQL sends `author` present and null, which a defaulted lookup returns as None.""" + orphan = thread('T1') + orphan['comments']['nodes'][0]['author'] = None + self.answer(payload([review()], [orphan, thread('T2')])) + out, unresolved = pr_review.digest('o', 'r', 7) + self.assertEqual(1, unresolved) + self.assertIn('T2', out) + def test_only_the_reviewer_s_own_unresolved_threads_are_listed(self) -> None: """A maintainer's own open thread is not a review finding to answer.""" self.answer(payload([review()], [thread('T1', login='ptr727'), thread('T2')])) From 4b6bcdbd69091c7c7f19785aa6507b1847532a21 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sun, 2 Aug 2026 20:40:45 -0700 Subject: [PATCH 4/8] Report unknown only where the window is genuinely unreadable The saturation guard fired on any full window, where only a full window carrying none of the reviewer's own comments is actually unreadable. Comments arrive in creation order, so anything behind the window is older than everything inside it, and one spent reviewer comment in view proves every hidden one is spent too. That case is a provable no. The guard was broader than the line it prints, which already said the newest comments carry none from the reviewer. An unknown a reader can disprove from the digest in front of them is what teaches them to skim the field, so the field says unknown only where the question is open. Co-Authored-By: Claude Opus 5 (1M context) --- scripts/README.md | 2 +- scripts/pr_review.py | 11 +++++++---- scripts/test_pr_review.py | 8 ++++++++ 3 files changed, 16 insertions(+), 5 deletions(-) diff --git a/scripts/README.md b/scripts/README.md index 35958eab..9e02d49d 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -93,7 +93,7 @@ python3 scripts/pr_review.py wait 452 --timeout 2700 `wait` exits `30` when the review is still pending at the timeout, which is pending rather than failed. Its failure mode is a wrong answer rather than a crash, so the cases feed crafted GraphQL payloads: a review attributed to the wrong login, a review counted against a stale head, a maintainer's own thread read as a finding, and a wait that returns success while nothing landed. One case reads the reviewer login out of the runbook rather than restating it, since GraphQL drops the `[bot]` suffix REST carries, and another asserts no mutation has crept into a read-only script. -`wait` exits `40` when Copilot answers the request with a plain comment rather than a review, meaning a comment of its own that postdates its newest review on the pull request. The test is the **shape** of that answer and not its cause, which the script reads nothing of: a comment carries no commit, so it satisfies no coverage check whatever it says, and a wait reading formal reviews alone treats it as an unmet condition and then polls out its whole timeout against an answer that already arrived. A refusal is the case that makes this worth catching, a quota or rate-limit message among them, and `40` neither asserts nor detects one. The comment prints whole because its wording is the only thing separating a refusal, which is terminal since no review follows it and re-requesting does not clear it, from an ordinary remark that is not, so `40` ends the wait and hands the text to the reader who can tell them apart. A comment **older** than the newest review is spent rather than terminal, because the review it preceded did land. Both queries read the newest `COMMENT_WINDOW` comments rather than the reviewer's own, since GraphQL offers no author filter, so ordinary discussion is what pushes an answer out of reach and the window is wide enough that it takes a hundred comments to do it. A **full** window reports `answered_outside_review=unknown` rather than `no`, because that is the one case where finding nothing and having nothing to find are the same reading, and a case holds the guard's number equal to the one the queries carry so a drift between them cannot read clean. The timeout path prints the full digest for the same reason, as a bare `PENDING` line reports a slow reviewer and a broken poll identically, which is the reading that turns a stalled watcher into a watcher nobody notices is stalled. +`wait` exits `40` when Copilot answers the request with a plain comment rather than a review, meaning a comment of its own that postdates its newest review on the pull request. The test is the **shape** of that answer and not its cause, which the script reads nothing of: a comment carries no commit, so it satisfies no coverage check whatever it says, and a wait reading formal reviews alone treats it as an unmet condition and then polls out its whole timeout against an answer that already arrived. A refusal is the case that makes this worth catching, a quota or rate-limit message among them, and `40` neither asserts nor detects one. The comment prints whole because its wording is the only thing separating a refusal, which is terminal since no review follows it and re-requesting does not clear it, from an ordinary remark that is not, so `40` ends the wait and hands the text to the reader who can tell them apart. A comment **older** than the newest review is spent rather than terminal, because the review it preceded did land. Both queries read the newest `COMMENT_WINDOW` comments rather than the reviewer's own, since GraphQL offers no author filter, so ordinary discussion is what pushes an answer out of reach and the window is wide enough that it takes a hundred comments to do it. A full window carrying **none** of the reviewer's own comments reports `answered_outside_review=unknown` rather than `no`, because that is the one case where finding nothing and having nothing to find are the same reading. A full window carrying even one spent reviewer comment reports `no` and is not a gap: comments arrive in creation order, so anything behind the window is older than everything inside it, and one spent comment in view therefore proves every hidden one is spent too. A case holds the guard's number equal to the one the queries carry, so a drift between them cannot read clean. The timeout path prints the full digest for the same reason, as a bare `PENDING` line reports a slow reviewer and a broken poll identically, which is the reading that turns a stalled watcher into a watcher nobody notices is stalled. The digest also reports the **suppressed findings** a review body collapses into a `
` block. Those reach no review thread, so a loop that polls threads alone reports a clean pass while they stand, and the [merge gate][governance] counts them as outstanding findings either way. `suppressed=N` counts findings rather than blocks, reading the `(N)` the heading carries, since one body holds one block per round and counting blocks reports two findings as one. It covers **every** round rather than the current head, because a suppressed finding has no resolved state for a push to retire: head-scoping read "superseded by a push" as "answered", and a finding nobody replied to left the digest the moment the branch moved, so the run reported zero. That is how four rounds went unanswered across three pull requests in one day, each found by the maintainer rather than by this script. The summary line splits the count as `suppressed=N (on_head=N earlier=N)` and each block is marked with the round that raised it, since a finding on an older round may since be moot and deciding that is the reader's call rather than one the count should make for them. Each block prints whole where a thread body truncates, because a thread can be re-read at its id and a suppressed finding cannot, and it prints under a marker naming what closing it takes: no thread exists to reply on or resolve, so the answer goes in the PR conversation. diff --git a/scripts/pr_review.py b/scripts/pr_review.py index de69711a..85a253b8 100644 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -102,13 +102,16 @@ def answered_outside_review(pr: dict) -> dict | None: def answer_window_saturated(pr: dict) -> bool: - """True where the window is full, so an answer sitting behind it cannot be ruled out. + """True where the window is full and carries no reviewer comment, the one unreadable case. The query reads the newest comments rather than the reviewer's, so ordinary discussion is - what pushes an answer out of reach, and a full window is the one case where finding nothing - and having nothing to find are the same reading. + what pushes an answer out of reach. Comments arrive in creation order, so anything behind + the window is older than everything inside it: one reviewer comment in view, spent against + a later review, proves every hidden one is spent too, and that reads as no rather than as + unknown. Only a full window with none of the reviewer's own leaves the question open. """ - return len(((pr.get('comments') or {}).get('nodes') or [])) >= COMMENT_WINDOW + comments = (pr.get('comments') or {}).get('nodes') or [] + return len(comments) >= COMMENT_WINDOW and not reviewer_nodes(pr, 'comments') def live_state(owner: str, repo: str, num: int) -> tuple[str, bool, dict | None]: diff --git a/scripts/test_pr_review.py b/scripts/test_pr_review.py index ca9484a1..9b83503e 100644 --- a/scripts/test_pr_review.py +++ b/scripts/test_pr_review.py @@ -127,6 +127,14 @@ def test_a_full_window_is_unknown_rather_than_no_answer(self) -> None: self.assertTrue(pr_review.answer_window_saturated(payload([review()], comments=full))) self.assertFalse(pr_review.answer_window_saturated(payload([review()], comments=full[:-1]))) + def test_one_spent_reviewer_comment_in_view_settles_a_full_window(self) -> None: + """Comments arrive in creation order, so a hidden one is older than the spent one in view.""" + full = ([comment(at=EARLY)] + + [comment(login='ptr727') for _ in range(pr_review.COMMENT_WINDOW - 1)]) + pr = payload([review(at=LATE)], comments=full) + self.assertIsNone(pr_review.answered_outside_review(pr)) + self.assertFalse(pr_review.answer_window_saturated(pr)) + class TestDigest(GqlCase): def test_the_summary_line_counts_what_it_names(self) -> None: From bced438bb154996f2a3d793c5a28d18e61da3510 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sun, 2 Aug 2026 20:47:32 -0700 Subject: [PATCH 5/8] Decide the window from what is behind it, not from its length A full window and a window holding every comment the pull request has are the same length, so the node count cannot tell a gap from a complete reading, and a pull request with exactly the window's worth of comments reported unknown while nothing was hidden. `pageInfo.hasPreviousPage` answers precisely the question the count was standing in for. A case holds both queries to asking for it, since a query that stops asking reports no rather than unknown, which is the silent narrowing one level up from the one the field exists to prevent. Co-Authored-By: Claude Opus 5 (1M context) --- scripts/README.md | 2 +- scripts/pr_review.py | 19 ++++++++++--------- scripts/test_pr_review.py | 34 ++++++++++++++++++++++------------ 3 files changed, 33 insertions(+), 22 deletions(-) diff --git a/scripts/README.md b/scripts/README.md index 9e02d49d..01e597bd 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -93,7 +93,7 @@ python3 scripts/pr_review.py wait 452 --timeout 2700 `wait` exits `30` when the review is still pending at the timeout, which is pending rather than failed. Its failure mode is a wrong answer rather than a crash, so the cases feed crafted GraphQL payloads: a review attributed to the wrong login, a review counted against a stale head, a maintainer's own thread read as a finding, and a wait that returns success while nothing landed. One case reads the reviewer login out of the runbook rather than restating it, since GraphQL drops the `[bot]` suffix REST carries, and another asserts no mutation has crept into a read-only script. -`wait` exits `40` when Copilot answers the request with a plain comment rather than a review, meaning a comment of its own that postdates its newest review on the pull request. The test is the **shape** of that answer and not its cause, which the script reads nothing of: a comment carries no commit, so it satisfies no coverage check whatever it says, and a wait reading formal reviews alone treats it as an unmet condition and then polls out its whole timeout against an answer that already arrived. A refusal is the case that makes this worth catching, a quota or rate-limit message among them, and `40` neither asserts nor detects one. The comment prints whole because its wording is the only thing separating a refusal, which is terminal since no review follows it and re-requesting does not clear it, from an ordinary remark that is not, so `40` ends the wait and hands the text to the reader who can tell them apart. A comment **older** than the newest review is spent rather than terminal, because the review it preceded did land. Both queries read the newest `COMMENT_WINDOW` comments rather than the reviewer's own, since GraphQL offers no author filter, so ordinary discussion is what pushes an answer out of reach and the window is wide enough that it takes a hundred comments to do it. A full window carrying **none** of the reviewer's own comments reports `answered_outside_review=unknown` rather than `no`, because that is the one case where finding nothing and having nothing to find are the same reading. A full window carrying even one spent reviewer comment reports `no` and is not a gap: comments arrive in creation order, so anything behind the window is older than everything inside it, and one spent comment in view therefore proves every hidden one is spent too. A case holds the guard's number equal to the one the queries carry, so a drift between them cannot read clean. The timeout path prints the full digest for the same reason, as a bare `PENDING` line reports a slow reviewer and a broken poll identically, which is the reading that turns a stalled watcher into a watcher nobody notices is stalled. +`wait` exits `40` when Copilot answers the request with a plain comment rather than a review, meaning a comment of its own that postdates its newest review on the pull request. The test is the **shape** of that answer and not its cause, which the script reads nothing of: a comment carries no commit, so it satisfies no coverage check whatever it says, and a wait reading formal reviews alone treats it as an unmet condition and then polls out its whole timeout against an answer that already arrived. A refusal is the case that makes this worth catching, a quota or rate-limit message among them, and `40` neither asserts nor detects one. The comment prints whole because its wording is the only thing separating a refusal, which is terminal since no review follows it and re-requesting does not clear it, from an ordinary remark that is not, so `40` ends the wait and hands the text to the reader who can tell them apart. A comment **older** than the newest review is spent rather than terminal, because the review it preceded did land. Both queries read the newest `COMMENT_WINDOW` comments rather than the reviewer's own, since GraphQL offers no author filter, so ordinary discussion is what pushes an answer out of reach and the window is wide enough that it takes a hundred comments to do it. `answered_outside_review=unknown` reports the one unreadable case: comments sit behind the window and none in view are the reviewer's. Everything else is decidable and says so. A window carrying even one **spent** reviewer comment reports `no`, because comments arrive in creation order, so anything behind the window is older than everything inside it and one spent comment in view proves every hidden one is spent too. A window holding every comment the pull request has reports `no` as well, which is why the guard reads `pageInfo.hasPreviousPage` rather than the node count: a full window and a complete one are the same length, so length alone would report a gap where none exists. Cases hold the window number equal in both queries and hold both queries to asking for `hasPreviousPage`, since a query that stops asking reports `no` instead of `unknown`, the silent narrowing one level up. The timeout path prints the full digest for the same reason, as a bare `PENDING` line reports a slow reviewer and a broken poll identically, which is the reading that turns a stalled watcher into a watcher nobody notices is stalled. The digest also reports the **suppressed findings** a review body collapses into a `
` block. Those reach no review thread, so a loop that polls threads alone reports a clean pass while they stand, and the [merge gate][governance] counts them as outstanding findings either way. `suppressed=N` counts findings rather than blocks, reading the `(N)` the heading carries, since one body holds one block per round and counting blocks reports two findings as one. It covers **every** round rather than the current head, because a suppressed finding has no resolved state for a push to retire: head-scoping read "superseded by a push" as "answered", and a finding nobody replied to left the digest the moment the branch moved, so the run reported zero. That is how four rounds went unanswered across three pull requests in one day, each found by the maintainer rather than by this script. The summary line splits the count as `suppressed=N (on_head=N earlier=N)` and each block is marked with the round that raised it, since a finding on an older round may since be moot and deciding that is the reader's call rather than one the count should make for them. Each block prints whole where a thread body truncates, because a thread can be re-read at its id and a suppressed finding cannot, and it prints under a marker naming what closing it takes: no thread exists to reply on or resolve, so the answer goes in the PR conversation. diff --git a/scripts/pr_review.py b/scripts/pr_review.py index 85a253b8..e08dea54 100644 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -36,7 +36,7 @@ TAGS = re.compile(r'', re.IGNORECASE) COUNT = re.compile(r'\((\d+)\)') -# How many of the newest comments both queries read, and the number the saturation guard tests. +# How many of the newest comments both queries read. # A narrow window drops the reviewer's answer behind ordinary discussion, reporting no answer. # A test holds this equal to the number the queries carry, since a drift between them reads clean. COMMENT_WINDOW = 100 @@ -50,7 +50,7 @@ repository(owner:$o,name:$r){ pullRequest(number:$n){ headRefOid reviews(last:20){ nodes{ author{login} state commit{oid} submittedAt } } - comments(last:100){ nodes{ author{login} createdAt } } + comments(last:100){ nodes{ author{login} createdAt } pageInfo{ hasPreviousPage } } }}} """ @@ -62,7 +62,7 @@ reviews(last:20){ nodes{ author{login} state commit{oid} submittedAt body } } reviewThreads(first:100){ nodes{ id isResolved comments(first:1){ nodes{ author{login} path line body } } }} - comments(last:100){ nodes{ author{login} createdAt body } } + comments(last:100){ nodes{ author{login} createdAt body } pageInfo{ hasPreviousPage } } }}} """ @@ -102,16 +102,17 @@ def answered_outside_review(pr: dict) -> dict | None: def answer_window_saturated(pr: dict) -> bool: - """True where the window is full and carries no reviewer comment, the one unreadable case. + """True where comments sit behind the window and none in view are the reviewer's. The query reads the newest comments rather than the reviewer's, so ordinary discussion is what pushes an answer out of reach. Comments arrive in creation order, so anything behind the window is older than everything inside it: one reviewer comment in view, spent against a later review, proves every hidden one is spent too, and that reads as no rather than as - unknown. Only a full window with none of the reviewer's own leaves the question open. + unknown. `hasPreviousPage` is what says anything is back there at all, since a full window + and a window holding every comment the pull request has are the same length. """ - comments = (pr.get('comments') or {}).get('nodes') or [] - return len(comments) >= COMMENT_WINDOW and not reviewer_nodes(pr, 'comments') + older = ((pr.get('comments') or {}).get('pageInfo') or {}).get('hasPreviousPage') + return bool(older) and not reviewer_nodes(pr, 'comments') def live_state(owner: str, repo: str, num: int) -> tuple[str, bool, dict | None]: @@ -190,8 +191,8 @@ def digest(owner: str, repo: str, num: int, seen: set[str] | None = None) -> tup f'merge={pr.get("mergeStateStatus")}' ] if answered == 'unknown': - lines.append(f' COMMENT WINDOW FULL: the newest {COMMENT_WINDOW} comments carry none ' - 'from the reviewer, so an older answer cannot be ruled out from here') + lines.append(f' COMMENTS BEHIND THE WINDOW: the newest {COMMENT_WINDOW} carry none from ' + 'the reviewer and older ones exist, so an answer cannot be ruled out here') if answer: # Printed whole for the same reason a suppressed finding is, since it reaches no thread. # Its wording is the only thing separating a refusal from an ordinary remark. diff --git a/scripts/test_pr_review.py b/scripts/test_pr_review.py index 9b83503e..e6ceaec0 100644 --- a/scripts/test_pr_review.py +++ b/scripts/test_pr_review.py @@ -51,10 +51,11 @@ def thread(tid: str, resolved: bool = False, login: str = pr_review.REVIEWER, def payload(reviews: list[dict], threads: list[dict] | None = None, - merge: str = 'CLEAN', comments: list[dict] | None = None) -> dict: + merge: str = 'CLEAN', comments: list[dict] | None = None, + older: bool = False) -> dict: return {'headRefOid': HEAD, 'mergeable': 'MERGEABLE', 'mergeStateStatus': merge, 'reviews': {'nodes': reviews}, 'reviewThreads': {'nodes': threads or []}, - 'comments': {'nodes': comments or []}} + 'comments': {'nodes': comments or [], 'pageInfo': {'hasPreviousPage': older}}} class GqlCase(unittest.TestCase): @@ -121,17 +122,23 @@ def test_ordinary_discussion_does_not_push_the_answer_out_of_the_window(self) -> payload([review(oid=OLD, at=EARLY)], comments=[comment(at=LATE)] + chatter)) self.assertIsNotNone(found) - def test_a_full_window_is_unknown_rather_than_no_answer(self) -> None: - """Finding nothing and having nothing to find are one reading once the window fills.""" + def test_comments_behind_the_window_are_unknown_rather_than_no_answer(self) -> None: + """Finding nothing and having nothing to find are one reading once an answer can hide.""" full = [comment(login='ptr727') for _ in range(pr_review.COMMENT_WINDOW)] - self.assertTrue(pr_review.answer_window_saturated(payload([review()], comments=full))) - self.assertFalse(pr_review.answer_window_saturated(payload([review()], comments=full[:-1]))) + self.assertTrue( + pr_review.answer_window_saturated(payload([review()], comments=full, older=True))) - def test_one_spent_reviewer_comment_in_view_settles_a_full_window(self) -> None: + def test_a_window_holding_every_comment_is_not_a_gap(self) -> None: + """A full window and a window holding the lot are the same length, so length cannot say.""" + full = [comment(login='ptr727') for _ in range(pr_review.COMMENT_WINDOW)] + self.assertFalse( + pr_review.answer_window_saturated(payload([review()], comments=full, older=False))) + + def test_one_spent_reviewer_comment_in_view_settles_the_question(self) -> None: """Comments arrive in creation order, so a hidden one is older than the spent one in view.""" full = ([comment(at=EARLY)] + [comment(login='ptr727') for _ in range(pr_review.COMMENT_WINDOW - 1)]) - pr = payload([review(at=LATE)], comments=full) + pr = payload([review(at=LATE)], comments=full, older=True) self.assertIsNone(pr_review.answered_outside_review(pr)) self.assertFalse(pr_review.answer_window_saturated(pr)) @@ -311,14 +318,14 @@ def test_a_pull_request_with_no_such_answer_says_so_rather_than_staying_silent(s out, _ = pr_review.digest('o', 'r', 7) self.assertIn('answered_outside_review=no', out) - def test_a_full_window_reports_unknown_and_names_why(self) -> None: - """Reporting `no` off a saturated window is the false clean this field exists to avoid.""" - self.answer(payload([review()], + def test_an_unreadable_window_reports_unknown_and_names_why(self) -> None: + """Reporting `no` off a window an answer can hide behind is the false clean to avoid.""" + self.answer(payload([review()], older=True, comments=[comment(login='ptr727') for _ in range(pr_review.COMMENT_WINDOW)])) out, _ = pr_review.digest('o', 'r', 7) self.assertIn('answered_outside_review=unknown', out) - self.assertIn('COMMENT WINDOW FULL', out) + self.assertIn('COMMENTS BEHIND THE WINDOW', out) class TestGqlTransport(unittest.TestCase): @@ -437,6 +444,9 @@ def test_the_guard_tests_the_window_the_queries_actually_read(self) -> None: windows = set(re.findall(r'comments\(last:(\d+)\)', source)) self.assertEqual({str(pr_review.COMMENT_WINDOW)}, windows) self.assertEqual(2, source.count(f'comments(last:{pr_review.COMMENT_WINDOW})')) + # The guard reads `hasPreviousPage`, so a query that stops asking reports no, not unknown. + # That is the silent narrowing this holds both queries against. + self.assertEqual(2, source.count('pageInfo{ hasPreviousPage }')) def test_the_backoff_is_bounded_and_non_decreasing(self) -> None: """A wait that sleeps zero seconds is a busy loop, and one that shrinks polls harder later.""" From bf7749940d485e83baf909a44a0a8a16cfeed87b Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sun, 2 Aug 2026 20:47:52 -0700 Subject: [PATCH 6/8] Close a finding with an action, never with a round count The session rule ended a session on a third review round, which reads as license to leave a loop open while it is still producing real defects, and it fires on nearly every pull request now that rounds routinely run past ten. A count measures how much was found rather than whether the work is done. The style heuristic counted rounds the same way, licensing a rule change only at three. Both are replaced by the four outcomes a finding can actually end in: fix it, disprove it in the thread with proof the reviewer can read so it stops raising the same thing, get the maintainer's explicit answer where it is real and deliberately unfixed, or fix the class where the code keeps earning the same finding. That last one is why a comment carrying the non-obvious why is a legitimate remedy rather than an appeasement, under the same comment rules as any other. Co-Authored-By: Claude Opus 5 (1M context) --- AGENTS.md | 2 +- GOVERNANCE.md | 14 ++++++++++++-- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 05fb1a18..3afe6f43 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -13,7 +13,7 @@ An agent session is billed on the context it carries, not the work it does. Ever ### Session Scope - **One deliverable, one session.** A session covers one branch and one deliverable, and ends when that work merges. A multi-step task is one deliverable and stays in one session. Two unrelated tasks are two sessions even when they run back to back. -- **End a session at any of these, without being asked:** the branch changes, the pull request merges, the next task is unrelated to the last, or a third review round opens on the same pull request. +- **End a session at any of these, without being asked:** the branch changes, the pull request merges, or the next task is unrelated to the last. A review round is none of them. A loop still producing findings is the deliverable in progress, and a round count is not a reason to leave one open. - **Hand off in a file, never in context.** Close a session by writing at most 2 KB to a scratch file: branch, pull request link, what is done, the next command. A summary held in context is re-billed until the session ends, and a summary on disk is read once by whoever needs it. - **Re-derive state, do not carry it.** "This session already has the context" is the signal to split, not to continue. Context that has gone stale is worse than absent, because a file read hundreds of requests ago no longer describes the file. - **Compaction is a fallback, not the strategy.** It restarts context from a floor and climbs again, where a fresh session starts from zero. diff --git a/GOVERNANCE.md b/GOVERNANCE.md index 2b3cf77a..e068fa7a 100644 --- a/GOVERNANCE.md +++ b/GOVERNANCE.md @@ -258,6 +258,15 @@ Drive the loop to green, meaning a review confirmed on the latest head SHA and e For provider-specific mechanics (how to request review, query review state, post replies, resolve threads), see the **GitHub Copilot Review Runbook** in [.github/copilot-instructions.md](./.github/copilot-instructions.md). This file owns the contract, and that file owns the mechanics. +### Every Finding Ends in an Action + +**A finding is closed by one of four outcomes, and a round count is never one of them.** The loop runs until no finding stands, however many rounds that takes, because the number of rounds measures how much was found rather than whether the work is done. A finding parked, waited out, or superseded by a push is still open. + +1. **It is real, so fix it.** Reply with the fixing commit SHA. +2. **It is not real, so disprove it in the thread**, with the command and its output, the code path that makes it impossible, or the rule that governs it. The proof is addressed to the reviewer as much as to the maintainer, since a decline it can read is what stops it raising the same thing next round. An assertion is not a proof and does not close a finding. +3. **It is real and deliberately not being fixed, which is the maintainer's call and not the agent's.** Say what the finding is, why the fix is unwanted, and get an explicit answer. Never suppress one by silence, by resolving the thread, or by an answer that reads as a decline while conceding the point. +4. **It keeps coming back, so fix the class rather than the instance.** A finding raised repeatedly against correct code is a defect in what the code communicates, not in the reviewer. Give it what it lacks: the non-obvious *why* as a comment where the code cannot state it, a clearer name, a narrower interface, or the rule change where the rule is what is wrong. A comment written for this earns its place under the comment rules like any other, so it states the why, stays short, and never cites a rule or addresses the reviewer. Making the noise stop is worth doing well, because a reviewer that repeats itself trains the reader to skim it, and skimming is how a real finding gets missed. + ### Triaging Review Comments **A low-confidence finding is not a low-value one.** Copilot collapses the findings it is least sure of into the review body instead of raising a thread, and in this fleet's experience those are right the large majority of the time. Judge each one against the code, never against its confidence label. They are also the easiest to lose, because they appear in no thread, so a loop that polls threads alone reports a clean pass while they stand (see the Merge Gate, condition 3). @@ -267,7 +276,7 @@ For each comment, classify before responding: - **Bug** - wrong behavior, missing test coverage, or a real divergence between code and docs. Fix it. Reply with the fixing commit SHA when done. - **Style/convention** - the comment cites a rule from this file or a language-specific style guide. Two cases: - The cited rule matches what the existing codebase already does -> fix the offending code. - - The cited rule contradicts what's in the tree, or industry norm -> **update the rule instead of the code**. The rule is wrong, not the code. Bouncing the same code across rounds is the symptom of a wrong rule. Heuristic: three rounds on the same style category means the rule needs adjusting and the user should authorize the rule change. + - The cited rule contradicts what's in the tree, or industry norm -> **update the rule instead of the code**. The rule is wrong, not the code. Bouncing the same code across rounds is the symptom of a wrong rule, so treat the recurrence itself as the finding and take it to the user for the rule change (outcome 4 above), rather than counting rounds until some threshold licenses it. - **Architectural opinion** - the comment proposes a different design ("constrain this to disabled-by-default", "move it elsewhere", "add a runtime guardrail"). This is judgment, not a bug. Surface it to the user with a recommendation, and don't apply it unilaterally. ### Responding and Resolution Expectations @@ -287,7 +296,8 @@ After the final push on a PR, sweep older threads from earlier rounds whose code Bring the user in when: - **Genuine design trade-off** surfaces (fail-open vs fail-closed, narrow vs broad refactor scope, "should we add a guardrail or trust the docstring"). Triage, recommend, ask. -- **Repeated friction** across rounds without convergence, which is the rule-needs-updating signal. Stop, summarize the pattern, and let the user authorize the rule change. +- **A recurring finding** the code keeps attracting, which is the fix-the-class signal. Summarize the pattern and bring the remedy, whether that is the rule change or what the code has to say differently to stop earning it. +- **A finding you judge real but do not want fixed**, which is outcome 3 above and is never the agent's call to make quietly. - **Architectural redesign** is requested rather than a bug fix. Surface with a recommendation, and never apply it unilaterally. Anti-pattern: don't keep flipping the code on the same style point. Flip the rule once and stick to the rule. From 884154215e04b9ea7013341ba6223b71cd3c9306 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sun, 2 Aug 2026 20:52:19 -0700 Subject: [PATCH 7/8] Let a finding be deferred, but only against a filed issue A finding worth doing later had no honest close. The four outcomes forced it into a decline it does not deserve or into silence, and a deferral recorded in a thread is lost the moment the pull request merges. Filing the issue first and replying with its link is what makes the deferral exist rather than be intended. The boundary matters more than the outcome. It covers work the change did not create, an adjacent defect noticed in passing or a fix too large to ride along, and never a defect in the change under review, since filing an issue about a bug about to merge is the deliberate-decline outcome wearing other clothes and that one is the maintainer's to decide. TODO.md queues the size question this loop raised. Five rounds each found something new, which reads as a large change earning a different finding every round where a small one converges, and the review history already carries the data to test it. Co-Authored-By: Claude Opus 5 (1M context) --- GOVERNANCE.md | 7 ++++--- TODO.md | 1 + 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/GOVERNANCE.md b/GOVERNANCE.md index e068fa7a..5c35b9bb 100644 --- a/GOVERNANCE.md +++ b/GOVERNANCE.md @@ -260,12 +260,13 @@ For provider-specific mechanics (how to request review, query review state, post ### Every Finding Ends in an Action -**A finding is closed by one of four outcomes, and a round count is never one of them.** The loop runs until no finding stands, however many rounds that takes, because the number of rounds measures how much was found rather than whether the work is done. A finding parked, waited out, or superseded by a push is still open. +**A finding is closed by one of five outcomes, and a round count is never one of them.** The loop runs until no finding stands, however many rounds that takes, because the number of rounds measures how much was found rather than whether the work is done. A finding parked, waited out, or superseded by a push is still open. 1. **It is real, so fix it.** Reply with the fixing commit SHA. 2. **It is not real, so disprove it in the thread**, with the command and its output, the code path that makes it impossible, or the rule that governs it. The proof is addressed to the reviewer as much as to the maintainer, since a decline it can read is what stops it raising the same thing next round. An assertion is not a proof and does not close a finding. 3. **It is real and deliberately not being fixed, which is the maintainer's call and not the agent's.** Say what the finding is, why the fix is unwanted, and get an explicit answer. Never suppress one by silence, by resolving the thread, or by an answer that reads as a decline while conceding the point. -4. **It keeps coming back, so fix the class rather than the instance.** A finding raised repeatedly against correct code is a defect in what the code communicates, not in the reviewer. Give it what it lacks: the non-obvious *why* as a comment where the code cannot state it, a clearer name, a narrower interface, or the rule change where the rule is what is wrong. A comment written for this earns its place under the comment rules like any other, so it states the why, stays short, and never cites a rule or addresses the reviewer. Making the noise stop is worth doing well, because a reviewer that repeats itself trains the reader to skim it, and skimming is how a real finding gets missed. +4. **It is real and worth doing later, so file the issue first and reply with its link.** A deferral recorded only in a thread is lost the moment the pull request merges, so the issue is what carries it and the link is what proves it exists rather than being intended. This is for work the change did not create: an adjacent defect the reviewer noticed in passing, or a fix too large to ride along. It does not cover a defect in the change under review, because filing an issue about a bug you are about to merge is outcome 3 in other clothes, and that one is the maintainer's to decide. +5. **It keeps coming back, so fix the class rather than the instance.** A finding raised repeatedly against correct code is a defect in what the code communicates, not in the reviewer. Give it what it lacks: the non-obvious *why* as a comment where the code cannot state it, a clearer name, a narrower interface, or the rule change where the rule is what is wrong. A comment written for this earns its place under the comment rules like any other, so it states the why, stays short, and never cites a rule or addresses the reviewer. Making the noise stop is worth doing well, because a reviewer that repeats itself trains the reader to skim it, and skimming is how a real finding gets missed. ### Triaging Review Comments @@ -276,7 +277,7 @@ For each comment, classify before responding: - **Bug** - wrong behavior, missing test coverage, or a real divergence between code and docs. Fix it. Reply with the fixing commit SHA when done. - **Style/convention** - the comment cites a rule from this file or a language-specific style guide. Two cases: - The cited rule matches what the existing codebase already does -> fix the offending code. - - The cited rule contradicts what's in the tree, or industry norm -> **update the rule instead of the code**. The rule is wrong, not the code. Bouncing the same code across rounds is the symptom of a wrong rule, so treat the recurrence itself as the finding and take it to the user for the rule change (outcome 4 above), rather than counting rounds until some threshold licenses it. + - The cited rule contradicts what's in the tree, or industry norm -> **update the rule instead of the code**. The rule is wrong, not the code. Bouncing the same code across rounds is the symptom of a wrong rule, so treat the recurrence itself as the finding and take it to the user for the rule change (outcome 5 above), rather than counting rounds until some threshold licenses it. - **Architectural opinion** - the comment proposes a different design ("constrain this to disabled-by-default", "move it elsewhere", "add a runtime guardrail"). This is judgment, not a bug. Surface it to the user with a recommendation, and don't apply it unilaterally. ### Responding and Resolution Expectations diff --git a/TODO.md b/TODO.md index adf8df5c..90396687 100644 --- a/TODO.md +++ b/TODO.md @@ -18,6 +18,7 @@ Running backlog for this repo, kept in a committed file so the guidance survives - Decide whether the canonical README section order follows PlexCleaner, which is a separate question from the four divergences above and affects every repo plus the `readme-structure` audit. PlexCleaner places **Questions or Issues** immediately after the Table of Contents, where the spec orders it ninth, and it carries sections the spec names nowhere, including Performance Considerations, Runtime Metrics, Custom Plugins, Testing, Development Tooling, Feature Ideas, and Sample Media Files. Under the recurrence rule in [`spec/section-model.md`][section-model] those last ones are correctly repo-specific and stay undeclared, so the open question is only the position of the sections the spec already names. - Declare locally-required secrets the way GitHub-stored ones are already declared, and make a gitignored `secrets/` directory the fleet standard that holds them. [`spec/secrets.json`][secrets] covers only the Actions and Dependabot stores, so a repo that deploys somewhere has no declared way to say what it needs at runtime, and the required set is discoverable only by reading the deploy. The pattern already runs in the fleet in two shapes: HomeAutomation-Config keeps a gitignored secrets directory of env files and Docker secret files, and ESPHome-Config keeps a gitignored `secrets.yaml` beside a committed `_secrets.yaml`. The committed file carries the required names with dummy values, so the shape of the requirement is in git while the values never are, which is the same split the GitHub side already gets from `requiredSecrets[]`. Blog needs it immediately, since it deploys on the proxmox host through HomeAutomation-Config's Docker Compose stack and carries the copy destinations and the internal URI. The hub carries neither the directory nor a `.gitignore` entry for one today, so adopting it here comes first. - Re-vendor the changed `verbatim` content across the fleet, which is one sweep covering three files. `repo-config/configure.sh` is carried `verbatim` with `appliesTo: "*"` and the hub swept it to one sentence per line. `AGENTS.md` "Context and Delegation Discipline" carries the wait rule's failure clause, and `GOVERNANCE.md` "Verification Discipline" carries the rule that a launched process is not a result. Every repo already holding a copy is byte-mismatched against the hub until it takes the new one, which the audit reports as stale rather than modified. +- Measure review rounds against pull request size, and decide what the number licenses. The recent loops suggest a large change earns a different finding every round while a small one converges in one or two, which would make change size the lever on review cost rather than the reviewer's thoroughness, and would argue for splitting a change before review rather than discovering it through five rounds of findings. The data needs no new instrumentation, since the review history already carries it: for each recent pull request, record the diff size in files and lines, the number of rounds, and the findings per round, counting suppressed findings alongside threaded ones because they are the majority of what these loops produce. The outcome worth having is a threshold [`GOVERNANCE.md`][governance] can state in the branching or review guidance, expressed as the size at which a change is split rather than as advice to keep changes small. Note two confounds before drawing a line from the numbers. A large change is usually also a novel one, so size and unfamiliarity move together and the record should note what kind of change each was. And a round that finds something new is not evidence of a problem by itself, since a round that finds something new is the reviewer working, so the metric to watch is findings that a smaller first cut would have surfaced earlier rather than findings per round on its own. - Decide where a carried file may name hub-only machinery, since `GOVERNANCE.md` "PR Review Etiquette" points at `scripts/pr_review.py` and the fleet carries the section but not the script. A downstream reader follows that pointer to a path their repo does not have. Either the script joins the carried set, or the rule states the behavior and drops the tool name the way the coordination-reference rule already requires for the template repo itself. - Make [`prose_lint.py`][prose-lint] assert a floor on its own scope, applying to itself the rule [`GOVERNANCE.md`][governance] already states: a gate that finds nothing is indistinguishable from a gate with nothing to find. A `--diff` run that resolves a non-empty diff and then matches **zero** files has almost certainly failed to scope rather than found a clean change, so it should say so instead of exiting 0. One session produced four separate routes to that same false clean: an unresolvable base widening to a whole-tree scan, a multi-line `paths` input read only to its first newline, a diff taken in one repository while scanning another, and a path under no repository at all. Each was fixed with its own guard, which is the wrong shape, because the fifth route will need a fifth guard and will be found the same way the first four were, by a reviewer rather than by the gate. A floor assertion covers the family. Note the honest limit before building it: a change touching only files the gate does not read (an image, a lock file) legitimately scopes to zero, so the assertion compares against the diff's own file list rather than against zero alone. - Teach the `sha-pin` check in [`repo_gate.py`][repo-gate] to verify a pin **resolves**, not merely that it is shaped like a SHA. Forty hex characters is a format any fabricated string satisfies, and an agent hand-writing a plausible SHA into a workflow is a real failure mode rather than a hypothetical one. A resolvability check also catches the neighboring case, a pin whose commit was reachable only from a branch that has since been squashed and deleted, which breaks a downstream gate long after the change that caused it. Scope the network call to same-owner repositories, where the fleet's own actions live, and skip rather than fail when the host is offline so the local gate stays usable. Note that the existing `gh-write-guard` hook cannot cover this, since it watches Bash and an editor tool writing the same string into a file never reaches it. From 17e65e8a09ee302403e204ec36c9bb41a5d9d958 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sun, 2 Aug 2026 21:00:51 -0700 Subject: [PATCH 8/8] Guard the review window the same way, where the cost is a false stop The review window carried the same truncation the comment window had, and consuming it for the answer baseline made it load-bearing. Twenty reviews is inside reach of a long pull request, and once the reviewer's own fall out of view the baseline is empty, which dates every comment as newer so each one reads as an answer. That is a wrong terminal rather than a missed one: it stops a loop on a pull request whose review landed. Both windows widen to a shared WINDOW and both connections carry `hasPreviousPage`, so one guard covers all four. Blind on comments leaves the question open and reports unknown. Blind on reviews leaves it unaskable, so nothing is reported and the wait keeps polling, since a wait that runs on is visible where a wrong terminal is not. Co-Authored-By: Claude Opus 5 (1M context) --- scripts/README.md | 4 ++- scripts/pr_review.py | 42 ++++++++++++++++-------------- scripts/test_pr_review.py | 55 +++++++++++++++++++++++++++------------ 3 files changed, 64 insertions(+), 37 deletions(-) diff --git a/scripts/README.md b/scripts/README.md index 01e597bd..4e27b4ba 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -93,7 +93,9 @@ python3 scripts/pr_review.py wait 452 --timeout 2700 `wait` exits `30` when the review is still pending at the timeout, which is pending rather than failed. Its failure mode is a wrong answer rather than a crash, so the cases feed crafted GraphQL payloads: a review attributed to the wrong login, a review counted against a stale head, a maintainer's own thread read as a finding, and a wait that returns success while nothing landed. One case reads the reviewer login out of the runbook rather than restating it, since GraphQL drops the `[bot]` suffix REST carries, and another asserts no mutation has crept into a read-only script. -`wait` exits `40` when Copilot answers the request with a plain comment rather than a review, meaning a comment of its own that postdates its newest review on the pull request. The test is the **shape** of that answer and not its cause, which the script reads nothing of: a comment carries no commit, so it satisfies no coverage check whatever it says, and a wait reading formal reviews alone treats it as an unmet condition and then polls out its whole timeout against an answer that already arrived. A refusal is the case that makes this worth catching, a quota or rate-limit message among them, and `40` neither asserts nor detects one. The comment prints whole because its wording is the only thing separating a refusal, which is terminal since no review follows it and re-requesting does not clear it, from an ordinary remark that is not, so `40` ends the wait and hands the text to the reader who can tell them apart. A comment **older** than the newest review is spent rather than terminal, because the review it preceded did land. Both queries read the newest `COMMENT_WINDOW` comments rather than the reviewer's own, since GraphQL offers no author filter, so ordinary discussion is what pushes an answer out of reach and the window is wide enough that it takes a hundred comments to do it. `answered_outside_review=unknown` reports the one unreadable case: comments sit behind the window and none in view are the reviewer's. Everything else is decidable and says so. A window carrying even one **spent** reviewer comment reports `no`, because comments arrive in creation order, so anything behind the window is older than everything inside it and one spent comment in view proves every hidden one is spent too. A window holding every comment the pull request has reports `no` as well, which is why the guard reads `pageInfo.hasPreviousPage` rather than the node count: a full window and a complete one are the same length, so length alone would report a gap where none exists. Cases hold the window number equal in both queries and hold both queries to asking for `hasPreviousPage`, since a query that stops asking reports `no` instead of `unknown`, the silent narrowing one level up. The timeout path prints the full digest for the same reason, as a bare `PENDING` line reports a slow reviewer and a broken poll identically, which is the reading that turns a stalled watcher into a watcher nobody notices is stalled. +`wait` exits `40` when Copilot answers the request with a plain comment rather than a review, meaning a comment of its own that postdates its newest review on the pull request. The test is the **shape** of that answer and not its cause, which the script reads nothing of: a comment carries no commit, so it satisfies no coverage check whatever it says, and a wait reading formal reviews alone treats it as an unmet condition and then polls out its whole timeout against an answer that already arrived. A refusal is the case that makes this worth catching, a quota or rate-limit message among them, and `40` neither asserts nor detects one. The comment prints whole because its wording is the only thing separating a refusal, which is terminal since no review follows it and re-requesting does not clear it, from an ordinary remark that is not, so `40` ends the wait and hands the text to the reader who can tell them apart. A comment **older** than the newest review is spent rather than terminal, because the review it preceded did land. Every connection reads the newest `WINDOW` nodes rather than the reviewer's own, since GraphQL offers no author filter, so ordinary traffic is what pushes theirs out of reach. `window_blind` is the one guard over both sides, and each side fails differently. Blind on **comments** means an answer could be back there unseen, which reads as `answered_outside_review=unknown` rather than `no`. Blind on **reviews** is worse, because the newest review in view is then not the newest there is, and an empty baseline dates every comment as newer so each one reads as an answer: a false `40` that stops the loop on a pull request whose review actually landed. That case reports nothing and lets the wait keep polling, since a wait that runs on is visible where a wrong terminal is not. + +Everything else is decidable and says so. One of the reviewer's own nodes in view, even a **spent** one, settles the question, because nodes arrive in creation order, so anything behind the window is older than everything inside it. A window holding every node the pull request has is settled too, which is why the guard reads `pageInfo.hasPreviousPage` rather than the node count: a full window and a complete one are the same length, so length alone would report a gap where none exists. Cases hold `WINDOW` equal across all four windows and hold all four to asking for `hasPreviousPage`, since a connection that stops asking reports `no` instead of `unknown`, the silent narrowing one level up. The timeout path prints the full digest for the same reason, as a bare `PENDING` line reports a slow reviewer and a broken poll identically, which is the reading that turns a stalled watcher into a watcher nobody notices is stalled. The digest also reports the **suppressed findings** a review body collapses into a `
` block. Those reach no review thread, so a loop that polls threads alone reports a clean pass while they stand, and the [merge gate][governance] counts them as outstanding findings either way. `suppressed=N` counts findings rather than blocks, reading the `(N)` the heading carries, since one body holds one block per round and counting blocks reports two findings as one. It covers **every** round rather than the current head, because a suppressed finding has no resolved state for a push to retire: head-scoping read "superseded by a push" as "answered", and a finding nobody replied to left the digest the moment the branch moved, so the run reported zero. That is how four rounds went unanswered across three pull requests in one day, each found by the maintainer rather than by this script. The summary line splits the count as `suppressed=N (on_head=N earlier=N)` and each block is marked with the round that raised it, since a finding on an older round may since be moot and deciding that is the reader's call rather than one the count should make for them. Each block prints whole where a thread body truncates, because a thread can be re-read at its id and a suppressed finding cannot, and it prints under a marker naming what closing it takes: no thread exists to reply on or resolve, so the answer goes in the PR conversation. diff --git a/scripts/pr_review.py b/scripts/pr_review.py index e08dea54..2e86796d 100644 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -36,10 +36,10 @@ TAGS = re.compile(r'', re.IGNORECASE) COUNT = re.compile(r'\((\d+)\)') -# How many of the newest comments both queries read. +# How many of the newest reviews and comments both queries read. # A narrow window drops the reviewer's answer behind ordinary discussion, reporting no answer. # A test holds this equal to the number the queries carry, since a drift between them reads clean. -COMMENT_WINDOW = 100 +WINDOW = 100 # Liveness query: timestamps and ids only, no comment or review bodies. # A liveness check does not need the finding text, and re-fetching bodies was 76% of polls. @@ -49,7 +49,7 @@ query($o:String!,$r:String!,$n:Int!){ repository(owner:$o,name:$r){ pullRequest(number:$n){ headRefOid - reviews(last:20){ nodes{ author{login} state commit{oid} submittedAt } } + reviews(last:100){ nodes{ author{login} state commit{oid} submittedAt } pageInfo{ hasPreviousPage } } comments(last:100){ nodes{ author{login} createdAt } pageInfo{ hasPreviousPage } } }}} """ @@ -59,7 +59,7 @@ query($o:String!,$r:String!,$n:Int!){ repository(owner:$o,name:$r){ pullRequest(number:$n){ headRefOid mergeable mergeStateStatus - reviews(last:20){ nodes{ author{login} state commit{oid} submittedAt body } } + reviews(last:100){ nodes{ author{login} state commit{oid} submittedAt body } pageInfo{ hasPreviousPage } } reviewThreads(first:100){ nodes{ id isResolved comments(first:1){ nodes{ author{login} path line body } } }} comments(last:100){ nodes{ author{login} createdAt body } pageInfo{ hasPreviousPage } } @@ -93,7 +93,10 @@ def answered_outside_review(pr: dict) -> dict | None: A comment older than the newest review is spent, since the review it preceded did land. """ comments = reviewer_nodes(pr, 'comments') - if not comments: + # A blind review window leaves no honest baseline to date a comment against. + # An empty one dates every comment as newer, so each reads as an answer. + # Reporting nothing keeps the wait polling, where a wrong answer ends it outright. + if not comments or window_blind(pr, 'reviews'): return None newest = max(comments, key=lambda n: n.get('createdAt') or '') reviews = reviewer_nodes(pr, 'reviews') @@ -101,18 +104,18 @@ def answered_outside_review(pr: dict) -> dict | None: return newest if (newest.get('createdAt') or '') > latest_review else None -def answer_window_saturated(pr: dict) -> bool: - """True where comments sit behind the window and none in view are the reviewer's. +def window_blind(pr: dict, field: str) -> bool: + """True where the reviewer's own nodes can sit behind the window, so the view cannot decide. - The query reads the newest comments rather than the reviewer's, so ordinary discussion is - what pushes an answer out of reach. Comments arrive in creation order, so anything behind - the window is older than everything inside it: one reviewer comment in view, spent against - a later review, proves every hidden one is spent too, and that reads as no rather than as - unknown. `hasPreviousPage` is what says anything is back there at all, since a full window - and a window holding every comment the pull request has are the same length. + Each query reads the newest nodes rather than the reviewer's, so ordinary traffic is what + pushes theirs out of reach. Nodes arrive in creation order, so anything behind the window is + older than everything inside it: one of the reviewer's in view bounds every hidden one as + older still, which settles the question rather than leaving it open. `hasPreviousPage` is + what says anything is back there at all, since a full window and a window holding the lot + are the same length. """ - older = ((pr.get('comments') or {}).get('pageInfo') or {}).get('hasPreviousPage') - return bool(older) and not reviewer_nodes(pr, 'comments') + older = ((pr.get(field) or {}).get('pageInfo') or {}).get('hasPreviousPage') + return bool(older) and not reviewer_nodes(pr, field) def live_state(owner: str, repo: str, num: int) -> tuple[str, bool, dict | None]: @@ -180,7 +183,8 @@ def digest(owner: str, repo: str, num: int, seen: set[str] | None = None) -> tup finding_count(b) for b in on_head_blocks) answer = answered_outside_review(pr) - answered = 'yes' if answer else ('unknown' if answer_window_saturated(pr) else 'no') + blind = [f for f in ('reviews', 'comments') if window_blind(pr, f)] + answered = 'yes' if answer else ('unknown' if blind else 'no') lines = [ f'pr={num} head={head[:8]} rounds={len(revs)} ' f'review_on_head={"yes" if on_head else "NO"} ' @@ -190,9 +194,9 @@ def digest(owner: str, repo: str, num: int, seen: set[str] | None = None) -> tup f'answered_outside_review={answered} ' f'merge={pr.get("mergeStateStatus")}' ] - if answered == 'unknown': - lines.append(f' COMMENTS BEHIND THE WINDOW: the newest {COMMENT_WINDOW} carry none from ' - 'the reviewer and older ones exist, so an answer cannot be ruled out here') + if blind: + lines.append(f' BEHIND THE WINDOW ({" and ".join(blind)}): the newest {WINDOW} carry ' + 'none from the reviewer and older ones exist, so this cannot decide') if answer: # Printed whole for the same reason a suppressed finding is, since it reaches no thread. # Its wording is the only thing separating a refusal from an ordinary remark. diff --git a/scripts/test_pr_review.py b/scripts/test_pr_review.py index e6ceaec0..b032bfbf 100644 --- a/scripts/test_pr_review.py +++ b/scripts/test_pr_review.py @@ -52,9 +52,10 @@ def thread(tid: str, resolved: bool = False, login: str = pr_review.REVIEWER, def payload(reviews: list[dict], threads: list[dict] | None = None, merge: str = 'CLEAN', comments: list[dict] | None = None, - older: bool = False) -> dict: + older: bool = False, older_reviews: bool = False) -> dict: return {'headRefOid': HEAD, 'mergeable': 'MERGEABLE', 'mergeStateStatus': merge, - 'reviews': {'nodes': reviews}, 'reviewThreads': {'nodes': threads or []}, + 'reviews': {'nodes': reviews, 'pageInfo': {'hasPreviousPage': older_reviews}}, + 'reviewThreads': {'nodes': threads or []}, 'comments': {'nodes': comments or [], 'pageInfo': {'hasPreviousPage': older}}} @@ -117,30 +118,49 @@ def test_no_comments_at_all_reads_as_no_answer(self) -> None: def test_ordinary_discussion_does_not_push_the_answer_out_of_the_window(self) -> None: """The window reads the newest comments, not the reviewer's, so others crowd it.""" - chatter = [comment(login='ptr727', at=LATE) for _ in range(pr_review.COMMENT_WINDOW - 1)] + chatter = [comment(login='ptr727', at=LATE) for _ in range(pr_review.WINDOW - 1)] found = pr_review.answered_outside_review( payload([review(oid=OLD, at=EARLY)], comments=[comment(at=LATE)] + chatter)) self.assertIsNotNone(found) def test_comments_behind_the_window_are_unknown_rather_than_no_answer(self) -> None: """Finding nothing and having nothing to find are one reading once an answer can hide.""" - full = [comment(login='ptr727') for _ in range(pr_review.COMMENT_WINDOW)] + full = [comment(login='ptr727') for _ in range(pr_review.WINDOW)] self.assertTrue( - pr_review.answer_window_saturated(payload([review()], comments=full, older=True))) + pr_review.window_blind(payload([review()], comments=full, older=True), 'comments')) def test_a_window_holding_every_comment_is_not_a_gap(self) -> None: """A full window and a window holding the lot are the same length, so length cannot say.""" - full = [comment(login='ptr727') for _ in range(pr_review.COMMENT_WINDOW)] + full = [comment(login='ptr727') for _ in range(pr_review.WINDOW)] self.assertFalse( - pr_review.answer_window_saturated(payload([review()], comments=full, older=False))) + pr_review.window_blind(payload([review()], comments=full, older=False), 'comments')) + + def test_reviews_behind_the_window_report_nothing_rather_than_a_false_answer(self) -> None: + """No reviewer review in view dates every comment as newer, so each reads as an answer. + + Reporting nothing keeps the wait polling, where a wrong answer ends it outright on a + pull request whose review landed and simply sits behind a busier review history. + """ + pr = payload([review(login='ptr727') for _ in range(pr_review.WINDOW)], + comments=[comment(at=LATE)], older_reviews=True) + self.assertTrue(pr_review.window_blind(pr, 'reviews')) + self.assertIsNone(pr_review.answered_outside_review(pr)) + + def test_one_reviewer_review_in_view_is_a_baseline_the_answer_can_be_dated_against(self) -> None: + """Reviews arrive in creation order too, so a hidden one is older than the one in view.""" + pr = payload([review(at=EARLY, oid=OLD)] + + [review(login='ptr727') for _ in range(pr_review.WINDOW - 1)], + comments=[comment(at=LATE)], older_reviews=True) + self.assertFalse(pr_review.window_blind(pr, 'reviews')) + self.assertIsNotNone(pr_review.answered_outside_review(pr)) def test_one_spent_reviewer_comment_in_view_settles_the_question(self) -> None: """Comments arrive in creation order, so a hidden one is older than the spent one in view.""" full = ([comment(at=EARLY)] - + [comment(login='ptr727') for _ in range(pr_review.COMMENT_WINDOW - 1)]) + + [comment(login='ptr727') for _ in range(pr_review.WINDOW - 1)]) pr = payload([review(at=LATE)], comments=full, older=True) self.assertIsNone(pr_review.answered_outside_review(pr)) - self.assertFalse(pr_review.answer_window_saturated(pr)) + self.assertFalse(pr_review.window_blind(pr, 'comments')) class TestDigest(GqlCase): @@ -322,10 +342,10 @@ def test_an_unreadable_window_reports_unknown_and_names_why(self) -> None: """Reporting `no` off a window an answer can hide behind is the false clean to avoid.""" self.answer(payload([review()], older=True, comments=[comment(login='ptr727') - for _ in range(pr_review.COMMENT_WINDOW)])) + for _ in range(pr_review.WINDOW)])) out, _ = pr_review.digest('o', 'r', 7) self.assertIn('answered_outside_review=unknown', out) - self.assertIn('COMMENTS BEHIND THE WINDOW', out) + self.assertIn('BEHIND THE WINDOW (comments)', out) class TestGqlTransport(unittest.TestCase): @@ -441,12 +461,13 @@ def test_no_mutation_reaches_this_script(self) -> None: def test_the_guard_tests_the_window_the_queries_actually_read(self) -> None: """A guard measuring one number while the query fetches another reads clean on drift.""" source = (REPO / 'scripts' / 'pr_review.py').read_text(encoding='utf-8') - windows = set(re.findall(r'comments\(last:(\d+)\)', source)) - self.assertEqual({str(pr_review.COMMENT_WINDOW)}, windows) - self.assertEqual(2, source.count(f'comments(last:{pr_review.COMMENT_WINDOW})')) - # The guard reads `hasPreviousPage`, so a query that stops asking reports no, not unknown. - # That is the silent narrowing this holds both queries against. - self.assertEqual(2, source.count('pageInfo{ hasPreviousPage }')) + windows = set(re.findall(r'(?:comments|reviews)\(last:(\d+)\)', source)) + self.assertEqual({str(pr_review.WINDOW)}, windows) + # The guard reads `hasPreviousPage`, so a connection that stops asking reports no. + # That is the silent narrowing this holds every window against. + # Four: reviews and comments, in each of the two queries. + self.assertEqual(4, source.count('pageInfo{ hasPreviousPage }')) + self.assertEqual(4, len(re.findall(r'(?:comments|reviews)\(last:\d+\)', source))) def test_the_backoff_is_bounded_and_non_decreasing(self) -> None: """A wait that sleeps zero seconds is a busy loop, and one that shrinks polls harder later."""