diff --git a/.agents/skills/drive-pr/SKILL.md b/.agents/skills/drive-pr/SKILL.md index d9d4774db..d207bb115 100644 --- a/.agents/skills/drive-pr/SKILL.md +++ b/.agents/skills/drive-pr/SKILL.md @@ -249,5 +249,6 @@ instance's outcome is context for the new one, never a standing answer to reuse - A genuine design trade-off, a recurring finding pattern, or an architectural redesign proposal each escalate per pr-review-conduct's own list, restated there, not duplicated here. -- A review shape the digest notes under `shapes=NOTED` is read by hand per pr-review-conduct's - Merge Gate item 4, never guessed at from what the new wording probably meant. +- A part of the review the digest prints under `UNPLACED ON THE HEAD` holds the gate until its + outcomes are acknowledged per pr-review-conduct's Merge Gate item 4. Never guess what the new + wording probably meant. Only an item that cannot be told to be a finding goes to the maintainer. diff --git a/.agents/skills/pr-review-conduct/SKILL.md b/.agents/skills/pr-review-conduct/SKILL.md index e03e3d876..1c1eef5ff 100644 --- a/.agents/skills/pr-review-conduct/SKILL.md +++ b/.agents/skills/pr-review-conduct/SKILL.md @@ -101,7 +101,8 @@ visible comments, routinely still carries a finding nobody has answered. Treatin - Every review thread on the promotion is resolved, and every check passes. - `coverage=none` is the only reason `pr_review.py` does not report the promotion ready, read from the digest's lines. A `VERDICT WITH NO COUNTED FINDING` headline, exit `50`, is a second - reason until it is answered under item 3, and not after. + reason until it is answered under item 3, and not after. A `shapes=HELD` hold is another + until its acknowledgment posts under item 4. The merge then runs with `--match-head-commit` set to the head those were checked on. The evidence names each constituent pull request and how its coverage was stated. An @@ -170,12 +171,24 @@ visible comments, routinely still carries a finding nobody has answered. Treatin the review round, and the list's length where it states the block's `(N)` count. A line under a heading that outcome 4 of that section names is not on that list. It is already its finding's outcome, and owes only the pointer that outcome names. -4. Every part of the review the tooling could not read was read by hand (an unrecognized - heading, a moved section, an unfamiliar label). The digest marks one `shapes=NOTED` and names - each under `REVIEWER OUTPUT NOTED`, and decides no exit code on it, so this item is read from - the digest rather than the exit. Read the body for what that part says, give any finding in it - item 3's outcome, and file an issue on the hub naming the spelling to add to its - `scripts/reviewers/` vocabulary, rather than guessing what the new wording probably meant. +4. Every part of a round covering the head that the tooling could not place is answered. The + digest prints an excerpt of each under `UNPLACED ON THE HEAD` with a fingerprint for the set. + It reads `shapes=HELD`, and `status` and `wait` exit `50`. + - Read each part whole in the review body, since the excerpt only locates it. Give every item + in it item 3's outcome: fixed with its SHA, declined with evidence, or listed as pre-existing + in the pull request body. + - Post the outcomes with `pr_review.py comment --repo / --fingerprint + --body `. One from an owner, member, or collaborator clears the hold, + `shapes=ACKNOWLEDGED`. New unplaced text on a new head owes a new acknowledgment. + - The spelling is a data edit to the matching `scripts/reviewers/*.json` in the hub plus one + fixture. Whoever acknowledged the part makes it in a pull request of its own, never an issue. + - An item that reads as neither a finding nor not one goes to the maintainer with its raw text + quoted, per "Escalate to the maintainer when" below. + + A part on an earlier round prints under `REVIEWER OUTPUT NOTED` and decides no exit code. So + does a label nested in a list, a quote, or a placed part. Read that block rather than the + `shapes=` field, which names a head hold first. Read each such part by hand, and give any + finding in it item 3's outcome. 5. The maintainer has given **explicit** permission to merge. The agent never merges on its own. A green or CLEAN PR with one open finding is not mergeable, @@ -399,6 +412,8 @@ from a hub checkout. Do not use a provider connector or reconstruct the GitHub m change), don't keep silently re-declining it. - A finding is judged real but should not be fixed. That decision is never the agent's alone. - An architectural redesign is proposed rather than a bug fix. +- An item in a part the digest prints under `UNPLACED ON THE HEAD` cannot be told to be a finding + or not. Quote its raw text in the escalation. An agent that cannot reach the maintainer directly, a dispatched subagent being the ordinary case, escalates to whoever dispatched it and stops that unit of work there. It never substitutes its own diff --git a/.claude-plugin/fleet-skills/.source-digests/drive-pr b/.claude-plugin/fleet-skills/.source-digests/drive-pr index c716b9e40..f4bceea62 100644 --- a/.claude-plugin/fleet-skills/.source-digests/drive-pr +++ b/.claude-plugin/fleet-skills/.source-digests/drive-pr @@ -1 +1 @@ -20fbf0764feb2653 +3609c8c27e9db5e7 diff --git a/.claude-plugin/fleet-skills/.source-digests/pr-review-conduct b/.claude-plugin/fleet-skills/.source-digests/pr-review-conduct index 8c3f1d92c..5f8952b5f 100644 --- a/.claude-plugin/fleet-skills/.source-digests/pr-review-conduct +++ b/.claude-plugin/fleet-skills/.source-digests/pr-review-conduct @@ -1 +1 @@ -5d653240a1d00f2a +75b92a3bbf15ab34 diff --git a/.claude-plugin/fleet-skills/skills/drive-pr/SKILL.md b/.claude-plugin/fleet-skills/skills/drive-pr/SKILL.md index d9d4774db..d207bb115 100644 --- a/.claude-plugin/fleet-skills/skills/drive-pr/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/drive-pr/SKILL.md @@ -249,5 +249,6 @@ instance's outcome is context for the new one, never a standing answer to reuse - A genuine design trade-off, a recurring finding pattern, or an architectural redesign proposal each escalate per pr-review-conduct's own list, restated there, not duplicated here. -- A review shape the digest notes under `shapes=NOTED` is read by hand per pr-review-conduct's - Merge Gate item 4, never guessed at from what the new wording probably meant. +- A part of the review the digest prints under `UNPLACED ON THE HEAD` holds the gate until its + outcomes are acknowledged per pr-review-conduct's Merge Gate item 4. Never guess what the new + wording probably meant. Only an item that cannot be told to be a finding goes to the maintainer. diff --git a/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md b/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md index e03e3d876..1c1eef5ff 100644 --- a/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md @@ -101,7 +101,8 @@ visible comments, routinely still carries a finding nobody has answered. Treatin - Every review thread on the promotion is resolved, and every check passes. - `coverage=none` is the only reason `pr_review.py` does not report the promotion ready, read from the digest's lines. A `VERDICT WITH NO COUNTED FINDING` headline, exit `50`, is a second - reason until it is answered under item 3, and not after. + reason until it is answered under item 3, and not after. A `shapes=HELD` hold is another + until its acknowledgment posts under item 4. The merge then runs with `--match-head-commit` set to the head those were checked on. The evidence names each constituent pull request and how its coverage was stated. An @@ -170,12 +171,24 @@ visible comments, routinely still carries a finding nobody has answered. Treatin the review round, and the list's length where it states the block's `(N)` count. A line under a heading that outcome 4 of that section names is not on that list. It is already its finding's outcome, and owes only the pointer that outcome names. -4. Every part of the review the tooling could not read was read by hand (an unrecognized - heading, a moved section, an unfamiliar label). The digest marks one `shapes=NOTED` and names - each under `REVIEWER OUTPUT NOTED`, and decides no exit code on it, so this item is read from - the digest rather than the exit. Read the body for what that part says, give any finding in it - item 3's outcome, and file an issue on the hub naming the spelling to add to its - `scripts/reviewers/` vocabulary, rather than guessing what the new wording probably meant. +4. Every part of a round covering the head that the tooling could not place is answered. The + digest prints an excerpt of each under `UNPLACED ON THE HEAD` with a fingerprint for the set. + It reads `shapes=HELD`, and `status` and `wait` exit `50`. + - Read each part whole in the review body, since the excerpt only locates it. Give every item + in it item 3's outcome: fixed with its SHA, declined with evidence, or listed as pre-existing + in the pull request body. + - Post the outcomes with `pr_review.py comment --repo / --fingerprint + --body `. One from an owner, member, or collaborator clears the hold, + `shapes=ACKNOWLEDGED`. New unplaced text on a new head owes a new acknowledgment. + - The spelling is a data edit to the matching `scripts/reviewers/*.json` in the hub plus one + fixture. Whoever acknowledged the part makes it in a pull request of its own, never an issue. + - An item that reads as neither a finding nor not one goes to the maintainer with its raw text + quoted, per "Escalate to the maintainer when" below. + + A part on an earlier round prints under `REVIEWER OUTPUT NOTED` and decides no exit code. So + does a label nested in a list, a quote, or a placed part. Read that block rather than the + `shapes=` field, which names a head hold first. Read each such part by hand, and give any + finding in it item 3's outcome. 5. The maintainer has given **explicit** permission to merge. The agent never merges on its own. A green or CLEAN PR with one open finding is not mergeable, @@ -399,6 +412,8 @@ from a hub checkout. Do not use a provider connector or reconstruct the GitHub m change), don't keep silently re-declining it. - A finding is judged real but should not be fixed. That decision is never the agent's alone. - An architectural redesign is proposed rather than a bug fix. +- An item in a part the digest prints under `UNPLACED ON THE HEAD` cannot be told to be a finding + or not. Quote its raw text in the escalation. An agent that cannot reach the maintainer directly, a dispatched subagent being the ordinary case, escalates to whoever dispatched it and stops that unit of work there. It never substitutes its own diff --git a/.github/skills/drive-pr/SKILL.md b/.github/skills/drive-pr/SKILL.md index d9d4774db..d207bb115 100644 --- a/.github/skills/drive-pr/SKILL.md +++ b/.github/skills/drive-pr/SKILL.md @@ -249,5 +249,6 @@ instance's outcome is context for the new one, never a standing answer to reuse - A genuine design trade-off, a recurring finding pattern, or an architectural redesign proposal each escalate per pr-review-conduct's own list, restated there, not duplicated here. -- A review shape the digest notes under `shapes=NOTED` is read by hand per pr-review-conduct's - Merge Gate item 4, never guessed at from what the new wording probably meant. +- A part of the review the digest prints under `UNPLACED ON THE HEAD` holds the gate until its + outcomes are acknowledged per pr-review-conduct's Merge Gate item 4. Never guess what the new + wording probably meant. Only an item that cannot be told to be a finding goes to the maintainer. diff --git a/.github/skills/pr-review-conduct/SKILL.md b/.github/skills/pr-review-conduct/SKILL.md index e03e3d876..1c1eef5ff 100644 --- a/.github/skills/pr-review-conduct/SKILL.md +++ b/.github/skills/pr-review-conduct/SKILL.md @@ -101,7 +101,8 @@ visible comments, routinely still carries a finding nobody has answered. Treatin - Every review thread on the promotion is resolved, and every check passes. - `coverage=none` is the only reason `pr_review.py` does not report the promotion ready, read from the digest's lines. A `VERDICT WITH NO COUNTED FINDING` headline, exit `50`, is a second - reason until it is answered under item 3, and not after. + reason until it is answered under item 3, and not after. A `shapes=HELD` hold is another + until its acknowledgment posts under item 4. The merge then runs with `--match-head-commit` set to the head those were checked on. The evidence names each constituent pull request and how its coverage was stated. An @@ -170,12 +171,24 @@ visible comments, routinely still carries a finding nobody has answered. Treatin the review round, and the list's length where it states the block's `(N)` count. A line under a heading that outcome 4 of that section names is not on that list. It is already its finding's outcome, and owes only the pointer that outcome names. -4. Every part of the review the tooling could not read was read by hand (an unrecognized - heading, a moved section, an unfamiliar label). The digest marks one `shapes=NOTED` and names - each under `REVIEWER OUTPUT NOTED`, and decides no exit code on it, so this item is read from - the digest rather than the exit. Read the body for what that part says, give any finding in it - item 3's outcome, and file an issue on the hub naming the spelling to add to its - `scripts/reviewers/` vocabulary, rather than guessing what the new wording probably meant. +4. Every part of a round covering the head that the tooling could not place is answered. The + digest prints an excerpt of each under `UNPLACED ON THE HEAD` with a fingerprint for the set. + It reads `shapes=HELD`, and `status` and `wait` exit `50`. + - Read each part whole in the review body, since the excerpt only locates it. Give every item + in it item 3's outcome: fixed with its SHA, declined with evidence, or listed as pre-existing + in the pull request body. + - Post the outcomes with `pr_review.py comment --repo / --fingerprint + --body `. One from an owner, member, or collaborator clears the hold, + `shapes=ACKNOWLEDGED`. New unplaced text on a new head owes a new acknowledgment. + - The spelling is a data edit to the matching `scripts/reviewers/*.json` in the hub plus one + fixture. Whoever acknowledged the part makes it in a pull request of its own, never an issue. + - An item that reads as neither a finding nor not one goes to the maintainer with its raw text + quoted, per "Escalate to the maintainer when" below. + + A part on an earlier round prints under `REVIEWER OUTPUT NOTED` and decides no exit code. So + does a label nested in a list, a quote, or a placed part. Read that block rather than the + `shapes=` field, which names a head hold first. Read each such part by hand, and give any + finding in it item 3's outcome. 5. The maintainer has given **explicit** permission to merge. The agent never merges on its own. A green or CLEAN PR with one open finding is not mergeable, @@ -399,6 +412,8 @@ from a hub checkout. Do not use a provider connector or reconstruct the GitHub m change), don't keep silently re-declining it. - A finding is judged real but should not be fixed. That decision is never the agent's alone. - An architectural redesign is proposed rather than a bug fix. +- An item in a part the digest prints under `UNPLACED ON THE HEAD` cannot be told to be a finding + or not. Quote its raw text in the escalation. An agent that cannot reach the maintainer directly, a dispatched subagent being the ordinary case, escalates to whoever dispatched it and stops that unit of work there. It never substitutes its own diff --git a/docs/pr-reviewer-evaluation.md b/docs/pr-reviewer-evaluation.md index 8f6257c65..d0bb152cb 100644 --- a/docs/pr-reviewer-evaluation.md +++ b/docs/pr-reviewer-evaluation.md @@ -148,7 +148,7 @@ The expansion should preserve one compact command while adding provider adapters `status` should report every enabled reviewer on one line and then list unresolved findings grouped by provider. `wait` should finish only when each selected reviewer reaches a recognized terminal state. -The Copilot adapter is `scripts/pr_review_copilot.py`, with its spellings in `scripts/reviewers/copilot.json`, and a provider joins it as a vocabulary file read by the same code path. Provider support must not weaken its refusal, partial-coverage, or suppressed-finding readings, and a shape it cannot place stays a note in the digest rather than an exit. +The Copilot adapter is `scripts/pr_review_copilot.py`, with its spellings in `scripts/reviewers/copilot.json`, and a provider joins it as a vocabulary file read by the same code path. Provider support must not weaken its refusal, partial-coverage, or suppressed-finding readings. A part it cannot place on a round covering the head holds the merge gate until an owner, member, or collaborator acknowledges it with written outcomes. ## Next Evaluation Steps diff --git a/scripts/README.md b/scripts/README.md index 49e376c7a..65c0b087d 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -179,15 +179,17 @@ uv run --no-project --script scripts/pr_review.py claims 452 --repo ptr727/Proje uv run --no-project --script scripts/pr_review.py wait 452 --repo ptr727/ProjectTemplate --timeout 2700 uv run --no-project --script scripts/pr_review.py comment 452 --repo ptr727/ProjectTemplate \ --body "Suppressed findings (1): **Disproven** - the target is checked before the write." +uv run --no-project --script scripts/pr_review.py comment 452 --repo ptr727/ProjectTemplate \ + --fingerprint 0123456789ab --body "**Declined**: the collapsed note restates the title." uv run --no-project --script scripts/pr_review.py reply 452 --repo ptr727/ProjectTemplate \ --match "retry count is off by one" --body "Fixed in abc1234: the loop now stops at n." --resolve uv run --no-project --script scripts/pr_review.py attest 452 --repo ptr727/ProjectTemplate --checkout ../worktree uv run --no-project --script scripts/pr_review.py cleanup 452 --repo ptr727/ProjectTemplate ``` -**The gate and the reader are separate files.** This script holds the GraphQL and REST plumbing, the checks, the verdict, the wait loop, and the writes. What a reviewer's body says is read by `pr_review_copilot.py`, whose result is a small typed reading rather than a body. The reviewer's wording has changed many times and has no programmatic API, so a change in it is a data edit in `reviewers/` plus one fixture, and never reaches the gate. A shape the reader cannot place prints under `shapes=NOTED` with a one-line note naming it, and decides no exit code. +**The gate and the reader are separate files.** This script holds the GraphQL and REST plumbing, the checks, the verdict, the wait loop, and the writes. What a reviewer's body says is read by `pr_review_copilot.py`, whose result is a small typed reading rather than a body. The reviewer's wording has changed many times and has no programmatic API, so a change in it is a data edit in `reviewers/` plus one fixture, and never reaches the gate. A part of a head round that the reader cannot place holds the gate, `shapes=HELD` and exit `50`. A finding can sit in it where no count reaches. Its text prints under `UNPLACED ON THE HEAD`, each part cut to `EXCERPT_LINES` lines of `EXCERPT_WIDTH` characters. Beside it prints a fingerprint, the first `FINGERPRINT_HEX` hex characters of a SHA-256 over the set's text. An owner, member, or collaborator clears the hold, `shapes=ACKNOWLEDGED`, with written outcomes posted by `comment --fingerprint`. That comment carries `` as an HTML line of its own. A new head carrying new unplaced text has a new fingerprint and owes a new acknowledgment. A part on an earlier round prints under `REVIEWER OUTPUT NOTED` with a one-line note. So does a label nested in a list, a quote, or a placed part. It decides no exit code. `shapes=NOTED` shows only where no head hold takes the field. -**A Copilot round is requested at defined moments rather than on every push.** The one moment is the head of a pull request into the default branch, a promotion among them. A promotion re-reviews content its feature pull requests already carried, so the hosted review runs once per change, at the promotion. On a pull request into any other branch, every head is covered by the recorded local pass the push already owes. That pass lives in the checkout's git directory, where nothing on GitHub can read it, so `attest` publishes it as a comment carrying ``. It does so only after confirming the checkout is the pushed head, holds no other change, measures the pull request's own merge base, and has a covering pass in `local_review.py status`. `status` then reads that head as `review_on_head=local` with `coverage=local`, where the attestation comes from an owner, member, or collaborator, stands as an HTML line of its own outside any code, and no round on record states partial coverage with the whole history in view. `wait` requests nothing on such a head and polls its checks instead, while they can still move the merge. Where it is not attested, `wait` exits `49`, and `--request` asks for a round anyway. The comment's findings count prints beside the reading and gates nothing, since a local pass's findings are advisory. A default branch the payload does not name reads as a promotion, so an unreadable field costs a Copilot request rather than passing a head on a local pass alone. +**A Copilot round is requested at defined moments rather than on every push.** The one moment is the head of a pull request into the default branch, a promotion among them. A promotion re-reviews content its feature pull requests already carried, so the hosted review runs once per change, at the promotion. On a pull request into any other branch, every head is covered by the recorded local pass the push already owes. That pass lives in the checkout's git directory, where nothing on GitHub can read it, so `attest` publishes it as a comment carrying ``. It does so only after confirming the checkout is the pushed head, holds no other change, measures the pull request's own merge base, and has a covering pass in `local_review.py status`. `status` then reads that head as `review_on_head=local` with `coverage=local`, where the attestation comes from an owner, member, or collaborator, stands as an HTML line of its own outside any code, and no round on record states partial coverage with the whole history in view. A hidden attestation comment does not count. `wait` requests nothing on such a head and polls its checks instead, while they can still move the merge. Where it is not attested, `wait` exits `49`, and `--request` asks for a round anyway. The comment's findings count prints beside the reading and gates nothing, since a local pass's findings are advisory. A default branch the payload does not name reads as a promotion, so an unreadable field costs a Copilot request rather than passing a head on a local pass alone. `--repo` is required and carries no default. A default names one repository, and a run from anywhere else resolves its number there instead, with every field well-formed and nothing in the output disagreeing. The digest leads with `repo=OWNER/NAME` for the same reason. A value that is not `OWNER/NAME` is rejected by name. @@ -204,7 +206,7 @@ uv run --no-project --script scripts/pr_review.py cleanup 452 --repo ptr727/Proj | `42` | `status`, `wait` | The round covering the head states partial coverage, or states none while an earlier round states partial coverage. | | `44` | `wait` | The review loop closed, the merge reads `BLOCKED`, and a required check is in a shape no wait clears. | | `49` | `wait` | No Copilot round covers the head and none was requested, on a pull request into a branch other than the default with no attestation. | -| `50` | `status`, `wait` | The round covering the head opens on a verdict other than the clean one while it counts no finding. | +| `50` | `status`, `wait` | The round covering the head opens on a verdict other than the clean one while it counts no finding. Or a head round carries a part the reader cannot place, and nothing acknowledges its fingerprint. | | `60` to `63` | `reply` | No thread matched, more than one did, the reply returned no comment url, or the resolve did not report the thread resolved. | | `64` | every write, `wait` | The write scope could not be established or excludes the target. | | `65` to `68` | `comment`, `attest` | The pull request could not be read, the response did not confirm the comment, the checkout is not the head, or no current local pass covers it. | @@ -230,7 +232,7 @@ The digest reports the completed head review's effective effort as `lite`, `bala `wait` exits `44` when the review loop has closed but a required check sits in a shape no wait clears. The digest carries `checks=N/M` beside the merge word and names the shape as `stuck=`, because `BLOCKED` alone is worn by a red check, a check nothing is running, an unresolved thread, and a missing approval alike. `NOT_POSTED` is a required status nothing has posted, `NOT_PICKED_UP` a check queued with no runner past `--check-grace`, `RUNNING_LONG` a check running past `--check-stall`, and `FAILED` a verdict. A check merely still running is none of these and exits `0`, since `wait` returns the moment coverage lands and the checks are nearly always still going then. `44` also requires that the stuck check be required, read from each rollup node's `isRequired`, and that the merge read `BLOCKED`. A rollup member of an unknown type is named on `CHECKS PARTIALLY UNREAD`, a rollup past `CHECKS_WINDOW` contexts on `CHECKS TRUNCATED`, and a payload with no commit matching the head on `CHECKS UNREADABLE`, rather than any of them reading as a clean `checks=0/0`. A `CheckRun`'s `PENDING` is dispatched and not begun while a `StatusContext`'s is under way, so the two are translated rather than renamed. `--check-grace` must be less than `--check-stall`, and a negative value for either is rejected by name. -`comment` posts one answer in the pull request conversation. A suppressed finding has no thread, so this is the path that records its disposition. The command reads the pull request node ID in the same run and exposes no ID argument. It confirms the returned comment URL and exact body before it reports success. A missing target exits `65`, and an unconfirmed response exits `66`, after which the conversation is inspected before any retry. +`comment` posts one answer in the pull request conversation. A suppressed finding has no thread, so this is the path that records its disposition. The command reads the pull request node ID in the same run and exposes no ID argument. It confirms the returned comment URL and exact body before it reports success. A missing target exits `65`, and an unconfirmed response exits `66`, after which the conversation is inspected before any retry. With `--fingerprint` the body is the acknowledgment of an unplaced set, posted with its marker beneath it. `reply` posts one answer to the thread its words select and resolves it on request. The selector is the finding's own words and there is deliberately no argument a `PRRT_...` id fits in, since a node id typed into a mutation resolves globally and writes to a real thread somewhere rather than failing. The words are matched case-insensitively against the thread's opening comment rather than a line number, since a fix push moves the line. Every failure is a stop rather than a fallback: no match exits `60`, two matches exit `61` and print both, a reply with no `url` exits `62` without resolving, and a resolve not reporting `isResolved` exits `63`. `--all-identical` answers each thread of a finding posted twice on one file. Exit `64` is the scope refusal `comment`, `wait`, and `attest` raise on the same terms: a target under an owner other than the one `git remote get-url origin` names against this script's own directory, or an owner that run could not determine. That refusal takes no environment-variable escape, since a grant the caller writes for itself is not a grant. diff --git a/scripts/pr_review.py b/scripts/pr_review.py index cf1a0c613..32e9031fb 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -14,8 +14,8 @@ This file is the gate: the plumbing, the checks, the verdict, the wait loop, and the writes. What a reviewer's body says is read by `pr_review_copilot.py`, whose vocabulary lives in `reviewers/.json`, so a change in the reviewer's wording is a data edit there and never -an edit here. A shape the adapter cannot place prints as a note under `shapes=NOTED` and decides -no exit code. +an edit here. A part of a head round the adapter cannot place holds the gate. A trusted author's +written outcomes acknowledge it, and on an earlier round it is a note. Subcommands attest Publish that a recorded local pass covers the pull request's head, as a comment carrying @@ -33,6 +33,8 @@ node id is read in the same run, and the returned comment URL and body confirm the write. Exit 0 = done, 64 = write scope could not be established or excludes the target, 65 = the PR could not be read, 66 = the response did not confirm the comment. + --fingerprint posts the body as the acknowledgment of the unplaced set `status` names, + with `` beneath it. claims Check the description against the branch it describes. A body claiming a commit or quoting a `uses:` ref the head tree no longer carries is a silent failure caught by a reviewer or not at all. Read-only. Exit 0 = every reference resolves, 70 = one does @@ -50,7 +52,9 @@ named in its headline alone, printed under `VERDICT WITH NO COUNTED FINDING`. The code repeats on that head after the headline is answered, since nothing here reads an answer. A clean verdict on the same terms prints its headline under `CLEAN VERDICT - HEADLINE`, which changes no exit code. + HEADLINE`, which changes no exit code. 50 is also a head round's part the adapter + cannot place, `shapes=HELD` with an excerpt under `UNPLACED ON THE HEAD`. A trusted + comment acknowledging its fingerprint clears it, `shapes=ACKNOWLEDGED`. `coverage=` names how the head's diff is shown read: `full` or `PARTIAL` as the round states it, `UNKNOWN` for a statement this cannot believe or a body the reader failed on, `table` where a round covering the head names exactly the changed files in its own @@ -266,7 +270,7 @@ headRefOid baseRefName state mergeable mergeStateStatus baseRepository{ defaultBranchRef{ name } } reviews(last:100){ nodes{ id author{login} state commit{oid} submittedAt body } pageInfo{ hasPreviousPage } } - comments(last:100){ nodes{ author{login} authorAssociation createdAt body } pageInfo{ hasPreviousPage } } + comments(last:100){ nodes{ author{login} authorAssociation createdAt body isMinimized } pageInfo{ hasPreviousPage } } reviewRequests(first:10){ nodes{ requestedReviewer{ __typename ... on Bot{login} ... on User{login} } } } commits(last:1){ nodes{ commit{ oid statusCheckRollup{ state contexts(first:__CHECKS_WINDOW__){ pageInfo{ hasNextPage } nodes{ @@ -354,6 +358,7 @@ re.MULTILINE, ) TRUSTED_ASSOCIATIONS = frozenset({"OWNER", "MEMBER", "COLLABORATOR"}) +ACK_MARKER = "" LOCAL_REVIEW = Path(__file__).resolve().parent / "local_review.py" # The conversation-comment and thread mutations the runbook publishes. @@ -1242,6 +1247,33 @@ def opened_by(pr: dict, node: dict) -> list[dict]: ] +def shape_hold(pr: dict) -> tuple[str, list[adapter.Unplaced], bool] | None: + """The parts the head's rounds leave unplaced, their fingerprint, and whether it is answered. + + None where those rounds place every part. A trusted comment is what answers a fingerprint. + """ + parts = list(dict.fromkeys(p for node in head_reviews(pr) for p in reading(node).unplaced)) + if not parts: + return None + fp = adapter.fingerprint(parts) + marker = ACK_MARKER.format(fp) + acked = any( + marker in lines and any(ln.strip() not in ("", marker) for ln in body.splitlines()) + for body, lines in trusted_comments(pr) + ) + return fp, parts, acked + + +def trusted_comments(pr: dict) -> Iterator[tuple[str, list[str]]]: + """Each owner's, member's, or collaborator's shown comment body, with its HTML lines outside code.""" + for node in (pr.get("comments") or {}).get("nodes") or []: + if (node.get("authorAssociation") or "") in TRUSTED_ASSOCIATIONS and not node.get( + "isMinimized" + ): + body = node.get("body") or "" + yield body, adapter.html_lines(body) + + def partial_on_record(pr: dict) -> bool: """Whether any Copilot round on the pull request, on any commit, states partial coverage.""" return any( @@ -1421,12 +1453,14 @@ def digest( found = reading(newest, pr=pr) if newest is not None else None effort, effort_source = review_effort(pr) cover, cover_why = coverage(pr) + hold = shape_hold(pr) # Every round's notes, since a renamed section on an earlier round hides a finding too. notes = [ *dict.fromkeys( f"round {((node.get('commit') or {}).get('oid') or 'unknown')[:8]}: {n}" for node in revs for n in reading(node).notes + if node not in on_head or n not in {p.note for p in reading(node).unplaced} ), *reviewer_login_drift(pr), ] @@ -1494,7 +1528,7 @@ def digest( ) + f"effort={effort} effort_source={effort_source} " f"coverage={cover.upper() if cover in ('partial', 'unknown') else cover} " - f"shapes={'NOTED' if notes else 'ok'} " + f"shapes={('ACKNOWLEDGED' if hold[2] else 'HELD') if hold else 'NOTED' if notes else 'ok'} " f"refusal={refusal_field} " f"threads={len(threads)}{'+' if truncated else ''} " f"unresolved={len(unresolved)}{'+' if truncated else ''}{breakdown} " @@ -1535,6 +1569,21 @@ def digest( f"to add belongs in `scripts/reviewers/` on {HUB}, which hosts this script" ) lines += [f" {note}" for note in notes] + if hold: + lines.append( + f" UNPLACED ON THE HEAD (fingerprint={hold[0]}): a round covering the head carries " + + ( + "parts an owner, member, or collaborator acknowledged with written outcomes" + if hold[2] + else "text the reader cannot place, where a finding can sit. Read each part whole " + "in the review body, the excerpt only locating it, and give each item an outcome, " + "fixed with its SHA, declined with evidence, or listed as pre-existing in the PR " + f"body. Post them with `comment {num} --repo {owner}/{repo} --fingerprint " + f"{hold[0]} --body `" + ) + ) + for part in hold[1]: + lines += [f" {part.note}", *([] if hold[2] else adapter.excerpt(part.text))] if found is not None and found.unthreaded: lines.append( f" FINDINGS WITH NO THREAD ({found.unthreaded}): the round covering the head names " @@ -1702,7 +1751,7 @@ def report_verdict(pr: dict) -> int: """Print the blocking verdict's own status line, and return the exit code it carries. A missing head review has no verdict yet, which the wait path owns. Coverage shown nowhere is a - note rather than an exit, and so is an unrecognized shape. + note rather than an exit. A head round's unplaced part is an exit until it is acknowledged. """ if not head_reviews(pr): return 0 @@ -1723,6 +1772,13 @@ def report_verdict(pr: dict) -> int: "COUNTED FINDING. Fix what it names and push, or answer it with `comment`. This code " "repeats on this head after the answer, since nothing here reads one" ) + if (hold := shape_hold(pr)) and not hold[2]: + print( + f"status=UNPLACED_PARTS_UNACKNOWLEDGED the round covering the head carries text the " + f"reader cannot place, under UNPLACED ON THE HEAD. Acknowledging fingerprint {hold[0]} " + "with `comment --fingerprint` and each item's outcome clears this code" + ) + if flagged or (hold and not hold[2]): return 50 if state in ("none", "unknown"): print( @@ -2079,14 +2135,12 @@ def attestation(pr: dict) -> str | None: None where no attestation vouches for the head against the pull request's current base, and "unknown" where the pass recorded no count. - Read over every comment, with the author association keeping a passer-by's marker from - vouching. The marker counts only as a line of its own outside a fence. + Read over every comment not hidden, with the author association keeping a passer-by's marker + from vouching. The marker counts only as a line of its own outside a fence. """ head, base = pr.get("headRefOid") or "", pr.get("baseRefName") or "" - for node in (pr.get("comments") or {}).get("nodes") or []: - if (node.get("authorAssociation") or "") not in TRUSTED_ASSOCIATIONS: - continue - for line in adapter.html_lines(node.get("body") or ""): + for _, lines in trusted_comments(pr): + for line in lines: m = ATTESTATION.fullmatch(line) if m and m.group(1) == head and m.group(2) == base: return m.group(3) @@ -2546,6 +2600,7 @@ def main(argv: list[str] | None = None) -> int: help="comment or reply: the answer to post, carrying the fixing commit SHA or " "the evidence that disproves the finding", ) + ap.add_argument("--fingerprint", help="comment: acknowledge the unplaced set it names") ap.add_argument( "--resolve", action="store_true", @@ -2577,6 +2632,8 @@ def main(argv: list[str] | None = None) -> int: ap.error(f"--checkout belongs to `attest` or `cleanup`, not `{a.cmd}`") if a.number is None: ap.error("the following arguments are required: number") + if a.cmd != "comment" and a.fingerprint is not None: + ap.error(f"--fingerprint belongs to `comment`, not `{a.cmd}`") if a.cmd not in ("comment", "reply") and a.body is not None: ap.error(f"--body belongs to `comment` or `reply`, not `{a.cmd}`") required = ["--body"] + (["--match"] if a.cmd == "reply" else []) @@ -2614,7 +2671,8 @@ def main(argv: list[str] | None = None) -> int: return attest(owner, repo, a.number, a.checkout or ".") if a.cmd == "comment": - return comment_on_pr(owner, repo, a.number, a.body) + marker = f"\n\n{ACK_MARKER.format(a.fingerprint)}" if a.fingerprint else "" + return comment_on_pr(owner, repo, a.number, a.body.strip() + marker) if a.cmd == "status": # One payload renders the digest and decides the code, as in `wait`. @@ -2792,8 +2850,8 @@ def held_read() -> dict: if verdict == 50 and checks_settling(checks, now, a.check_grace, a.check_stall): print( "note: a check on this head has not concluded, and this wait returns 50 at once " - "on every call until the head moves, since nothing here reads an answer to the " - "headline. Once the headline is answered, watch the checks with a bounded " + "while the hold above stands, which for a headline is until the head moves. Once " + "it is answered, watch the checks with a bounded " f"`timeout 1800 gh pr checks {a.number} --repo {owner}/{repo} --watch`, then run " "status" ) diff --git a/scripts/pr_review_copilot.py b/scripts/pr_review_copilot.py index 5c0debee4..c70cbf094 100755 --- a/scripts/pr_review_copilot.py +++ b/scripts/pr_review_copilot.py @@ -7,19 +7,21 @@ The reviewer's wording and layout change without notice and with no programmatic API, so every spelling this reads lives in `reviewers/.json` rather than here. A changed spelling is a -data edit plus one fixture. A part of a body this cannot place is a one-line note, and `read` -never raises: a body it trips on reads as no refusal, `unknown` coverage and verdict, and a note, -so a new shape reaches the digest as a note rather than the gate as a crash. +data edit plus one fixture. A part of a body this cannot place is a one-line note. One that can +hold a finding also carries its own text as an unplaced part. `read` never raises. A body it +trips on reads as no refusal, `unknown` coverage and verdict, and the whole body unplaced. A new +shape reaches the gate as text, never a crash. The body is parsed with markdown-it-py, so a code span, a fence, or an indented block is a token of its own and never reads as markup. Only Copilot's current overview format, the one carrying the -`ccr-overview-v2` marker, is read. A Copilot body in any other layout reads as a note. +`ccr-overview-v2` marker, is read. A Copilot body in any other layout reads as one unplaced part. """ from __future__ import annotations import argparse import functools +import hashlib import json import re import sys @@ -73,6 +75,9 @@ ) # A count longer than this is an identifier rather than anything a review states. COUNT_DIGITS = 4 +FINGERPRINT_HEX = 12 +EXCERPT_LINES = 12 +EXCERPT_WIDTH = 160 PARSER = MarkdownIt("commonmark").enable("table") @@ -86,12 +91,20 @@ class Section(NamedTuple): text: str +class Unplaced(NamedTuple): + """One part of a body the reader cannot place: the note naming it, and the part's own text.""" + + note: str + text: str + + class Reading(NamedTuple): """What one body says, in the fields the gate reads. `open` is the count of open findings the body states, None where it states none. `unthreaded` is how many findings it names that no thread on the pull request carries. `table` is the - paths the body's file table names, None where it carries no table. + paths the body's file table names, None where it carries no table. `unplaced` is each part + the reader cannot place, with its text, the subset of `notes` that can hold a finding. """ coverage: str = "unknown" @@ -110,6 +123,7 @@ class Reading(NamedTuple): rate_limited: str | None = None marked: bool = False notes: tuple[str, ...] = () + unplaced: tuple[Unplaced, ...] = () def held(self, kind: str) -> int: """The findings this body holds in sections of `kind`, by the counts their labels state.""" @@ -281,10 +295,10 @@ def read(body: str, reviewer: str = "copilot", threads: tuple[str, ...] | None = """ try: return _read(body or "", reviewer, threads) - # A shape the reader trips on is a note in the digest, never a crash in the gate. except Exception as exc: # noqa: BLE001 failed = f"the reader failed on this body ({type(exc).__name__})" - return Reading(refusal="none", coverage_line=failed, notes=(failed,)) + part = Unplaced(failed, (body or "").replace("\r\n", "\n").strip()) + return Reading(refusal="none", coverage_line=failed, notes=(failed,), unplaced=(part,)) @functools.lru_cache(maxsize=512) @@ -294,6 +308,12 @@ def _read(body: str, reviewer: str, threads: tuple[str, ...] | None) -> Reading: lines = text.split("\n") tokens = PARSER.parse(text) notes: list[str] = [] + unplaced: list[Unplaced] = [] + + def unread(note: str, start: int, end: int) -> None: + notes.append(note) + unplaced.append(Unplaced(note, "\n".join(lines[start:end]).strip())) + html = [t.content for t in tokens if t.type == "html_block"] + [ c.content for t in tokens @@ -340,14 +360,17 @@ def _read(body: str, reviewer: str, threads: tuple[str, ...] | None) -> Reading: if not text.strip(): notes.append("the body is empty") elif not marked: - notes.append( + unread( f"the body carries no `` marker, the one format " - "this reader reads" + "this reader reads", + 0, + len(lines), ) found = details(tokens) sections: list[Section] = [] claimed: list[tuple[int, int]] = [] + opaque: list[tuple[int, int]] = [] counted: list[tuple[int, int]] = [] open_count: int | None = None entries = linked = 0 @@ -363,7 +386,12 @@ def _read(body: str, reviewer: str, threads: tuple[str, ...] | None) -> Reading: ) if match is None: if vet and node.parent is None: - notes.append(f"a collapsed section this reader has no spelling for: `{name}`") + opaque.append((node.start, end + 1)) + unread( + f"a collapsed section this reader has no spelling for: `{name}`", + node.start, + end + 1, + ) continue claimed.append((node.start, end + 1)) # A section counting its entries, resolved ones included, holds findings rather than a statement. @@ -391,20 +419,48 @@ def _read(body: str, reviewer: str, threads: tuple[str, ...] | None) -> Reading: table: list[str] | None = None listed = 0 - def inside(token: Token) -> bool: - return any(a <= (token.map or [0])[0] < b for a, b in claimed) + def inside(token: Token, ranges: list[tuple[int, int]] = claimed) -> bool: + return any(a <= (token.map or [0])[0] < b for a, b in ranges) + + def placed(n: int, t: Token) -> bool: + if t.type == "heading_open": + return True + if t.level: + return False + if t.type == "table_open": + return bool(vocab["table_header"]) and ( + table_paths(tokens, n, vocab["table_header"]) is not None + ) + if t.type == "html_block": + return bool(FLEET_REVIEW.search(t.content)) + if t.type != "paragraph_open": + return False + said = " ".join(inline_text(tokens[n + 1]).split()) + return ( + is_label(tokens[n + 1]) + or bool(vocab["effort"].fullmatch(label(said))) + or any(p.match(said) for p in vocab["coverage_prose"]) + or any(matched(ps, label(said)) for ps in vocab["sections"].values()) + ) + + stops = sorted( + {a for a, _ in claimed + opaque} + | {(t.map or [0])[0] for n, t in enumerate(tokens) if placed(n, t)} + ) i = 0 while i < len(tokens): token = tokens[i] - if token.type == "heading_open" and not inside(token): + if token.type == "heading_open" and not inside(token) and not inside(token, opaque): name = label(tokens[i + 1].content) kind = next((k for k, ps in vocab["verdicts"].items() if matched(ps, name)), "") if kind and verdict == "unknown": verdict, verdict_heading = kind, name headline = headline_after(tokens, i + 3, lines, vocab["coverage_prose"]) elif not kind and vet and not matched(vocab["headings"], name): - notes.append(f"a heading this reader has no spelling for: `{name}`") + unread( + f"a heading this reader has no spelling for: `{name}`", *span(tokens, i, stops) + ) elif token.type == "paragraph_open": inline = tokens[i + 1] name = label(inline_text(inline)) @@ -423,8 +479,17 @@ def inside(token: Token) -> bool: open_count = (open_count or 0) + stated(found_kind[1]) elif matched(vocab["finding_lists"], name): listed += items_after(tokens, i + 3) - elif vet and is_label(inline) and not matched(vocab["labels"], name): - notes.append(f"a label this reader has no spelling for: `{name}`") + elif ( + vet + and is_label(inline) + and not matched(vocab["labels"], name) + and not inside(token, opaque) + ): + note = f"a label this reader has no spelling for: `{name}`" + if token.level or inside(token): + notes.append(note) + else: + unread(note, *span(tokens, i, stops)) elif ( token.type == "table_open" and vocab["table_header"] @@ -478,9 +543,33 @@ def inside(token: Token) -> bool: rate_limited=rate_limited, marked=marked, notes=tuple(dict.fromkeys(notes)), + unplaced=tuple(dict.fromkeys(unplaced)), ) +def span(tokens: list[Token], start: int, stops: list[int]) -> tuple[int, int]: + """The lines from the block opening at `start` to the next stop, the part a note names.""" + first = (tokens[start].map or [0])[0] + return first, next((s for s in stops if s > first), sys.maxsize) + + +def fingerprint(parts: list[Unplaced]) -> str: + """A short stable name for a set of unplaced parts, over their text in sorted order.""" + digest = hashlib.sha256() + for text in sorted({p.text for p in parts}): + digest.update(text.encode("utf-8") + b"\0") + return digest.hexdigest()[:FINGERPRINT_HEX] + + +def excerpt(text: str) -> list[str]: + """The first lines of a part, indented and cut to a width, with a closing line for the rest.""" + kept = [ln.rstrip() for ln in text.splitlines() if ln.strip()] + shown = [ln if len(ln) <= EXCERPT_WIDTH else ln[:EXCERPT_WIDTH] + "..." for ln in kept] + cut = len(kept) - EXCERPT_LINES + more = [f"... {cut} more lines in the body"] if cut > 0 else [] + return [f" {ln}" for ln in shown[:EXCERPT_LINES] + more] + + def entries_in( tokens: list[Token], start: int, end: int, found: list[_Details], vocab: dict ) -> tuple[int, list[list[str]]]: diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index ce58fe3e3..65068395f 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -156,6 +156,25 @@ def comment( return {"author": {"login": login}, "createdAt": at, "body": body} +def unplaced(finding: str = "The retry bound is off by one.") -> str: + """A marked overview carrying a collapsed section the vocabulary has no spelling for.""" + return ( + f"{OVERVIEW}\n
\nConfidence notes (1)\n\n{finding}\n" + f"
\n\n{COVERED}" + ) + + +def ack(fp: str, association: str, text: str | None = None) -> dict: + """A conversation comment acknowledging the unplaced set under `fp`, by `association`.""" + written = f"Fixed in abc1234: the bound.\n\n{pr_review.ACK_MARKER.format(fp)}" + return { + "author": {"login": "maintainer"}, + "authorAssociation": association, + "createdAt": LATE, + "body": written if text is None else text, + } + + def overview_v2( open_count: str = "2 open findings", covers: str = "", @@ -1993,13 +2012,15 @@ def test_a_note_on_an_earlier_round_still_reaches_the_digest(self) -> None: self.assertIn("shapes=NOTED ", self.out.getvalue()) self.assertIn(f"round {OLD[:8]}: the body carries no", self.out.getvalue()) - def test_a_body_the_reader_trips_on_is_a_note_and_never_a_refusal(self) -> None: + def test_a_body_the_reader_trips_on_holds_and_is_never_a_refusal(self) -> None: with mock.patch.object(pr_review.adapter, "_read", side_effect=ValueError("boom")): - self.assertEqual(0, self.status(payload([review(body="odd body")], files=["a.py"]))) + self.assertEqual(50, self.status(payload([review(body="odd body")], files=["a.py"]))) out = self.out.getvalue() self.assertIn("review_on_head=yes ", out) self.assertIn("refusal=no ", out) + self.assertIn("shapes=HELD ", out) self.assertIn("the reader failed on this body (ValueError)", out) + self.assertIn(" odd body", out) self.assertIn("COVERAGE IS UNKNOWN: the reader failed on this body (ValueError)", out) def test_a_flagging_verdict_that_counts_nothing_exits_fifty(self) -> None: @@ -2083,12 +2104,89 @@ def qodo(badge: str) -> dict: self.status(payload([review()], comments=[summary], files=["a.py"])) self.assertNotIn("qodo_open", self.out.getvalue()) - def test_an_unrecognized_shape_is_a_note_and_never_an_exit(self) -> None: - odd = "## Pull request overview\n\nThe change is narrow.\n\n" + COVERED - self.assertEqual(0, self.status(payload([review(body=odd)], files=["a.py"]))) + def test_an_unrecognized_shape_on_the_head_holds_with_its_text_and_fingerprint(self) -> None: + pr = payload([review(body=unplaced())], files=["a.py"]) + self.assertEqual(50, self.status(pr)) out = self.out.getvalue() - self.assertIn("shapes=NOTED ", out) - self.assertIn("REVIEWER OUTPUT NOTED (1)", out) + hold = pr_review.shape_hold(pr) + assert hold is not None + self.assertRegex(hold[0], r"^[0-9a-f]{12}$") + self.assertIn("shapes=HELD ", out) + self.assertIn(f"UNPLACED ON THE HEAD (fingerprint={hold[0]})", out) + self.assertIn("a collapsed section this reader has no spelling for", out) + self.assertIn(" The retry bound is off by one.", out) + self.assertIn(f"--fingerprint {hold[0]} --body ", out) + self.assertIn("status=UNPLACED_PARTS_UNACKNOWLEDGED", out) + self.assertNotIn("REVIEWER OUTPUT NOTED", out) + + def test_an_acknowledgment_by_an_owner_member_or_collaborator_clears_the_hold(self) -> None: + fp = self.fingerprint(unplaced()) + for association in ("OWNER", "MEMBER", "COLLABORATOR"): + with self.subTest(association=association): + self.out.seek(0) + self.out.truncate() + pr = payload( + [review(body=unplaced())], comments=[ack(fp, association)], files=["a.py"] + ) + self.assertEqual(0, self.status(pr)) + out = self.out.getvalue() + self.assertIn("shapes=ACKNOWLEDGED ", out) + self.assertIn(f"UNPLACED ON THE HEAD (fingerprint={fp})", out) + self.assertNotIn("The retry bound is off by one.", out) + self.assertNotIn("status=UNPLACED_PARTS_UNACKNOWLEDGED", out) + + def test_an_acknowledgment_by_a_non_member_does_not_clear_the_hold(self) -> None: + fp = self.fingerprint(unplaced()) + for association in ("CONTRIBUTOR", "FIRST_TIME_CONTRIBUTOR", "NONE", ""): + with self.subTest(association=association): + pr = payload( + [review(body=unplaced())], comments=[ack(fp, association)], files=["a.py"] + ) + self.assertEqual(50, self.status(pr)) + self.assertIn("shapes=HELD ", self.out.getvalue()) + + def test_a_marker_with_no_outcome_or_inside_a_fence_does_not_clear_the_hold(self) -> None: + marker = pr_review.ACK_MARKER.format(self.fingerprint(unplaced())) + for text in (marker, f"Fixed in abc1234.\n\n```text\n{marker}\n```"): + with self.subTest(text=text): + pr = payload( + [review(body=unplaced())], + comments=[ack("", "OWNER", text=text)], + files=["a.py"], + ) + self.assertEqual(50, self.status(pr)) + + def test_a_new_head_with_new_unplaced_text_holds_again(self) -> None: + old = unplaced("The retry bound is off by one.") + new = unplaced("The timeout is read before it is set.") + rounds = [review(oid=OLD, body=old, rid="A"), review(body=new, rid="B")] + pr = payload(rounds, comments=[ack(self.fingerprint(old), "OWNER")], files=["a.py"]) + self.assertEqual(50, self.status(pr)) + out = self.out.getvalue() + self.assertIn("shapes=HELD ", out) + self.assertIn(" The timeout is read before it is set.", out) + self.assertIn(f"round {OLD[:8]}: a collapsed section", out) + self.assertNotEqual(self.fingerprint(old), self.fingerprint(new)) + + def test_the_same_unplaced_text_on_a_new_head_keeps_its_acknowledgment(self) -> None: + def trailing(effort: str) -> str: + return f"{OVERVIEW}\n**Risk level:**\n\nLow.\n\nReview effort: {effort}" + + old, new = trailing("Lite"), trailing("Balanced") + rounds = [review(oid=OLD, body=old, rid="A"), review(body=new, rid="B")] + pr = payload(rounds, comments=[ack(self.fingerprint(old), "OWNER")], files=["a.py"]) + self.status(pr) + self.assertIn("shapes=ACKNOWLEDGED ", self.out.getvalue()) + + def test_a_hidden_acknowledgment_does_not_clear_the_hold(self) -> None: + hidden = {**ack(self.fingerprint(unplaced()), "OWNER"), "isMinimized": True} + pr = payload([review(body=unplaced())], comments=[hidden], files=["a.py"]) + self.assertEqual(50, self.status(pr)) + + def fingerprint(self, text: str) -> str: + hold = pr_review.shape_hold(payload([review(body=text)])) + assert hold is not None + return hold[0] def test_a_wait_on_a_drifted_login_ends_on_forty(self) -> None: drifted = review(login="copilot-pull-request-reviewer-v2") @@ -4362,6 +4460,25 @@ def test_an_unreadable_origin_refuses_before_the_pr_read(self) -> None: self.assertIn("OUT_OF_SCOPE", self.out.getvalue()) +class TestCommentAcknowledgesAnUnplacedSet(CommentCase): + def test_the_outcomes_post_with_the_marker_the_gate_reads(self) -> None: + held = payload([review(body=unplaced())], files=["a.py"]) + hold = pr_review.shape_hold(held) + assert hold is not None + sent = f"{COMMENT_BODY}\n\n" + self.wire({"id": "PR_from_query", "url": COMMENT["url"]}, {**COMMENT, "body": sent}) + argv = ["comment", "7", "--repo", "o/r", "--fingerprint", hold[0], "--body", COMMENT_BODY] + self.assertEqual(0, pr_review.main(argv)) + mutation = next((v for q, v in self.calls if "addComment" in q)) + self.assertEqual(sent, mutation["body"]) + answered = {**held, "comments": {"nodes": [ack(hold[0], "OWNER", text=sent)]}} + self.assertEqual((hold[0], hold[1], True), pr_review.shape_hold(answered)) + + def test_the_fingerprint_belongs_to_comment_alone(self) -> None: + with contextlib.redirect_stderr(io.StringIO()), self.assertRaises(SystemExit): + pr_review.main(["status", "7", "--repo", "o/r", "--fingerprint", "0" * 12]) + + LANDED = {"id": "c1", "url": "https://github.com/o/r/pull/7#discussion_r1", "body": "Fixed in abc."} diff --git a/tests/test_pr_review_copilot.py b/tests/test_pr_review_copilot.py index 1f1c64bff..ad5d1816c 100755 --- a/tests/test_pr_review_copilot.py +++ b/tests/test_pr_review_copilot.py @@ -425,6 +425,7 @@ def test_the_reader_never_raises(self) -> None: self.assertEqual("unknown", found.coverage) self.assertEqual("none", found.refusal) self.assertEqual(("the reader failed on this body (ValueError)",), found.notes) + self.assertEqual(("anything",), tuple(p.text for p in found.unplaced)) def test_a_pathological_body_reads_in_bounded_time(self) -> None: for text in ("
" * 5000, "> " * 20000 + "x", "`" * 30000, "\u00a0" * 30000): @@ -432,6 +433,84 @@ def test_a_pathological_body_reads_in_bounded_time(self) -> None: adapter.read(text) +class TestUnplaced(unittest.TestCase): + def test_each_unplaced_part_carries_its_own_text(self) -> None: + odd = ( + "
\nConfidence notes (2)\n\nHidden finding.\n
\n\n" + "### Confidence assessment\n\nThe loop may not end.\n\n**Risk level:**\n\nLow." + ) + parts = {p.note: p.text for p in adapter.read(body(sections=odd)).unplaced} + self.assertEqual(3, len(parts)) + texts = list(parts.values()) + self.assertTrue(any("Hidden finding." in t and "" in t for t in texts)) + self.assertIn("### Confidence assessment\n\nThe loop may not end.", texts) + self.assertIn("**Risk level:**\n\nLow.", texts) + + def test_a_body_in_another_layout_is_unplaced_whole(self) -> None: + text = "## Pull request overview\n\nThe change is narrow." + self.assertEqual((text,), tuple(p.text for p in adapter.read(text).unplaced)) + + def test_an_empty_body_and_a_placed_body_leave_nothing_unplaced(self) -> None: + self.assertEqual((), adapter.read("").unplaced) + self.assertEqual((), adapter.read(body(sections=open_section())).unplaced) + + def test_the_fingerprint_is_short_stable_and_named_by_the_text(self) -> None: + a, b = adapter.Unplaced("one", "first part"), adapter.Unplaced("two", "second part") + fp = adapter.fingerprint([a, b]) + self.assertRegex(fp, r"^[0-9a-f]{12}$") + self.assertEqual(fp, adapter.fingerprint([b, a])) + self.assertNotEqual(fp, adapter.fingerprint([a, b._replace(text="second part.")])) + + def test_an_excerpt_is_bounded_in_lines_and_width(self) -> None: + text = "\n".join(["x" * 500] + [f"line {n}" for n in range(40)]) + shown = adapter.excerpt(text) + self.assertEqual(adapter.EXCERPT_LINES + 1, len(shown)) + self.assertTrue(shown[0].endswith("x...")) + self.assertLessEqual(len(shown[0]), adapter.EXCERPT_WIDTH + 10) + self.assertIn("29 more lines in the body", shown[-1]) + + def test_an_unplaced_label_stops_before_what_the_reader_places(self) -> None: + odd = "**Reviewer musings:**\n\nThe loop may not end.\n\n" + what_changed() + (part,) = adapter.read(body(sections=odd)).unplaced + self.assertIn("The loop may not end.", part.text) + self.assertNotIn("What changed", part.text) + + def test_a_label_inside_a_list_item_is_a_note_and_not_a_part(self) -> None: + nested = "**Changes:**\n\n- Test updates:\n\n - Cover the empty case.\n" + found = adapter.read(body(sections=nested)) + self.assertEqual((), found.unplaced) + self.assertTrue(any("test updates:" in n for n in found.notes)) + + def test_the_effort_tail_stays_out_of_a_trailing_part(self) -> None: + texts = [ + adapter.read(body(sections="**Risk level:**\n\nLow.", tail=tail)).unplaced[0].text + for tail in ("Review effort: Lite", "Review effort: Balanced") + ] + self.assertEqual(["**Risk level:**\n\nLow."] * 2, texts) + + def test_a_count_line_ends_a_trailing_part(self) -> None: + texts = [ + adapter.read(body(sections=f"**Risk level:**\n\nLow.\n\n{count}")).unplaced[0].text + for count in ("**0 open findings**", "**No open findings**") + ] + self.assertEqual(["**Risk level:**\n\nLow."] * 2, texts) + + def test_an_other_table_or_html_line_stays_inside_the_part(self) -> None: + for middle in ("| Area | Note |\n|---|---|\n| retry | off by one |", ""): + with self.subTest(middle=middle): + odd = f"### Risk notes\n\n{middle}\n\nThe loop may not end." + (part,) = adapter.read(body(sections=odd)).unplaced + self.assertIn("The loop may not end.", part.text) + + def test_a_heading_inside_an_unknown_section_is_not_a_second_part(self) -> None: + odd = ( + "
\nConfidence notes (1)\n\n### Inner\n\n" + "**Odd label:**\n\nText.\n
" + ) + (part,) = adapter.read(body(sections=odd)).unplaced + self.assertIn("### Inner", part.text) + + class TestOtherReviewers(unittest.TestCase): def test_coderabbit_outside_diff_counts_its_heading(self) -> None: text = (