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..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. @@ -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..5c35b9bb 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. @@ -257,6 +258,16 @@ 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 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 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 **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). @@ -266,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. 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 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 @@ -286,7 +297,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. diff --git a/TODO.md b/TODO.md index b6e6a252..90396687 100644 --- a/TODO.md +++ b/TODO.md @@ -17,7 +17,9 @@ 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. +- 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. - 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..4e27b4ba 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -93,6 +93,10 @@ 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. 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. 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..2e86796d 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. + 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 @@ -24,22 +27,30 @@ 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. +# 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. +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. +# 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:100){ nodes{ author{login} state commit{oid} submittedAt } pageInfo{ hasPreviousPage } } + comments(last:100){ nodes{ author{login} createdAt } pageInfo{ hasPreviousPage } } }}} """ @@ -48,9 +59,10 @@ 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 } } }}} """ @@ -66,14 +78,53 @@ 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. + + 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') + # 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') + latest_review = max((n.get('submittedAt') or '' for n in reviews), default='') + return newest if (newest.get('createdAt') or '') > latest_review else None + + +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. + + 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(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]: + """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,14 +161,15 @@ 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'] + # 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. @@ -130,14 +182,29 @@ 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) + 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"} ' 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={answered} ' f'merge={pr.get("mergeStateStatus")}' ] + 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. + lines.append(f' COPILOT COMMENT ({answer.get("createdAt")}, newer than any review): ' + '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 for t in unresolved: c = (t.get('comments') or {}).get('nodes', [{}])[0] @@ -152,8 +219,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 +256,28 @@ 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 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 5f722356..b032bfbf 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 @@ -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,12 @@ 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, + 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}}} class GqlCase(unittest.TestCase): @@ -71,12 +84,83 @@ 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 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.""" + 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)]))) + + 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.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.WINDOW)] + self.assertTrue( + 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.WINDOW)] + self.assertFalse( + 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.WINDOW - 1)]) + pr = payload([review(at=LATE)], comments=full, older=True) + self.assertIsNone(pr_review.answered_outside_review(pr)) + self.assertFalse(pr_review.window_blind(pr, 'comments')) class TestDigest(GqlCase): @@ -99,6 +183,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')])) @@ -229,6 +322,32 @@ 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) + + 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.WINDOW)])) + out, _ = pr_review.digest('o', 'r', 7) + self.assertIn('answered_outside_review=unknown', out) + self.assertIn('BEHIND THE WINDOW (comments)', 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 +398,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: @@ -310,6 +458,17 @@ 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|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.""" source = (REPO / 'scripts' / 'pr_review.py').read_text(encoding='utf-8')