From 44ac58dcf805e8fde16fff93bee3f11718a98d57 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Thu, 1 Oct 2026 20:44:40 -0700 Subject: [PATCH 1/8] Add pr_review.py attest and Narrow the Copilot Ruleset Triggers Co-Authored-By: Claude Opus 5.5 --- repo-config/README.md | 5 +- repo-config/develop.json | 4 +- repo-config/main.json | 4 +- scripts/pr_review.py | 138 ++++++++++++++++++++++++++++++++++++++- tests/test_pr_review.py | 108 ++++++++++++++++++++++++++++++ 5 files changed, 252 insertions(+), 7 deletions(-) diff --git a/repo-config/README.md b/repo-config/README.md index 06fe9c09d..0cbb9617a 100644 --- a/repo-config/README.md +++ b/repo-config/README.md @@ -12,9 +12,9 @@ Hub-only repository and branch configuration held as committed files, kept out o Two workflow models share `main.json` but differ on `develop` (registry `workflowModel`, default `release`): - **`release`** (`develop.json`): `develop` requires squash merges with linear history and a PR, the feature-branch pipeline. -- **`operational`** (`operational/develop.json`): `develop` takes **direct signed pushes**, carrying only `deletion`, `non_fast_forward`, and `required_signatures`; no PR, no status-check, no Copilot-on-push. CI runs on the push as advisory feedback. Read the dropped rules as an allowance rather than a prohibition, since a PR into `develop` remains legal and the lint workflow triggers on it, with its result reported and not required (a required check here would gate the direct push as well). This is for live-service config repos that edit `develop` directly and promote a known-good snapshot to `main` via an occasional PR (see [GOVERNANCE.md "Branching Model"][governance-branching-model]). +- **`operational`** (`operational/develop.json`): `develop` takes **direct signed pushes**, carrying only `deletion`, `non_fast_forward`, and `required_signatures`; no PR, no status-check, no Copilot review rule. CI runs on the push as advisory feedback. Read the dropped rules as an allowance rather than a prohibition, since a PR into `develop` remains legal and the lint workflow triggers on it, with its result reported and not required (a required check here would gate the direct push as well). This is for live-service config repos that edit `develop` directly and promote a known-good snapshot to `main` via an occasional PR (see [GOVERNANCE.md "Branching Model"][governance-branching-model]). -`main` (both models) requires merge-commit merges (no linear-history rule), signed commits, a passing `Check pull request workflow status job`, resolved review threads, and Copilot review, and blocks force-pushes and deletion, so a `develop -> main` promotion is always gated even when `develop` takes direct commits. Every ruleset intentionally leaves "Require branches to be up to date before merging" **off**, per [GOVERNANCE.md "Branching Model"][governance-branching-model]. +`main` (both models) requires merge-commit merges (no linear-history rule), signed commits, a passing `Check pull request workflow status job`, resolved review threads, and Copilot review, and blocks force-pushes and deletion, so a `develop -> main` promotion is always gated even when `develop` takes direct commits. The Copilot review rule in `develop.json` and `main.json` reviews a pull request when it opens and not on each push or while it is a draft, since a later round is requested deliberately per [GOVERNANCE.md "PR Review Etiquette"][governance-pr-review-etiquette] rather than spent on every push. Every ruleset intentionally leaves "Require branches to be up to date before merging" **off**, per [GOVERNANCE.md "Branching Model"][governance-branching-model]. The result is **exactly two rulesets named `develop` and `main`**, and the names are load-bearing (`GOVERNANCE.md` and the workflows reference them). Only the `develop` *content* varies by model. The required check binds by name and only turns green after the repo's PR workflow runs once. @@ -79,6 +79,7 @@ A repository linked to a project of its own is left alone, like a label the payl [governance-communicating-with-the-user]: ../GOVERNANCE.md#communicating-with-the-user [governance-durable-knowledge]: ../GOVERNANCE.md#durable-knowledge-and-self-improvement [governance-hub-hosted-tooling]: ../GOVERNANCE.md#hub-hosted-tooling +[governance-pr-review-etiquette]: ../GOVERNANCE.md#pr-review-etiquette [governance-verification-discipline]: ../GOVERNANCE.md#verification-discipline [project-json]: ./project.json [repo-config-doc]: ../docs/repo-config.md diff --git a/repo-config/develop.json b/repo-config/develop.json index 89cac0614..fcd488c4c 100644 --- a/repo-config/develop.json +++ b/repo-config/develop.json @@ -52,8 +52,8 @@ }, { "parameters": { - "review_draft_pull_requests": true, - "review_on_push": true + "review_draft_pull_requests": false, + "review_on_push": false }, "type": "copilot_code_review" } diff --git a/repo-config/main.json b/repo-config/main.json index 6c7b76b6e..d51074df1 100644 --- a/repo-config/main.json +++ b/repo-config/main.json @@ -49,8 +49,8 @@ }, { "parameters": { - "review_draft_pull_requests": true, - "review_on_push": true + "review_draft_pull_requests": false, + "review_on_push": false }, "type": "copilot_code_review" } diff --git a/scripts/pr_review.py b/scripts/pr_review.py index 8d676bb68..80753eb5e 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -814,6 +814,14 @@ def strip_fences( repository(owner:$o,name:$r){ pullRequest(number:$n){ id url } }} """ +Q_ATTEST_TARGET = """ +query($o:String!,$r:String!,$n:Int!){ + repository(owner:$o,name:$r){ pullRequest(number:$n){ headRefOid baseRefName } }} +""" +ATTESTATION = re.compile(r"") +TRUSTED_ASSOCIATIONS = frozenset({"OWNER", "MEMBER", "COLLABORATOR"}) +LOCAL_REVIEW = Path(__file__).resolve().parent / "local_review.py" + # The conversation-comment and thread mutations the runbook publishes. # `url` is fetched because it is the one field that confirms a comment or reply carried a body. # A reply that posted empty still returns a comment, and three did, each then resolved. @@ -3552,6 +3560,123 @@ def comment_on_pr(owner: str, repo: str, num: int, body: str) -> int: return 0 +def attest(owner: str, repo: str, num: int, checkout: str) -> int: + """Publish that a recorded local pass covers this pull request's head. Returns an exit code. + + The receipt `local_review.py` records lives in the checkout's git directory, so nothing on + GitHub can read it, and the review gate for a fix push needs to. This reads the receipt + where it lives and posts a comment the gate can read, and only after three checks, each of + which would otherwise vouch for content the pull request does not carry: the checkout's + HEAD is the pull request's head, the checkout holds no change beyond that commit, and + `local_review.py check` finds a current pass against the pull request's base. + """ + ok, why = in_scope(owner) + if not ok: + print(f"status=OUT_OF_SCOPE nothing was written: {why}") + return 64 + target = gql(Q_ATTEST_TARGET, owner, repo, num) or {} + head, base = target.get("headRefOid") or "", target.get("baseRefName") or "" + if not head or not base: + print( + f"status=TARGET_NOT_READ nothing was written: {owner}/{repo} #{num} did not return " + "its head commit and base branch" + ) + return 65 + local = _git(checkout, "rev-parse", "HEAD") + dirty = _git(checkout, "status", "--porcelain") + if local.returncode != 0 or dirty.returncode != 0: + print( + f"status=CHECKOUT_NOT_READ nothing was written: {checkout} is not a readable checkout" + ) + return 67 + if local.stdout.strip() != head: + print( + f"status=CHECKOUT_NOT_THE_HEAD nothing was written: the checkout is at " + f"{local.stdout.strip()[:8]} and the pull request's head is {head[:8]}, so push or " + "fetch until they agree" + ) + return 67 + if dirty.stdout.strip(): + print( + "status=CHECKOUT_NOT_THE_HEAD nothing was written: the checkout holds changes the " + "head commit does not, so a pass over it does not describe what was pushed" + ) + return 67 + try: + check = subprocess.run( + [sys.executable, str(LOCAL_REVIEW), "check", "--target", base], + cwd=checkout, + capture_output=True, + text=True, + encoding="utf-8", + timeout=120, + check=False, + ) + except (OSError, subprocess.SubprocessError): + check = subprocess.CompletedProcess([], 2, "", "local_review.py could not be run") + if check.returncode != 0: + print( + f"status=NO_LOCAL_PASS nothing was written: `local_review.py check --target {base}` " + f"exited {check.returncode}, so no recorded pass covers this content. Run the " + "local strict review, record it, and attest again" + ) + print(f" {(check.stdout or check.stderr).strip()[:400]}") + return 68 + body = ( + f"A recorded local strict-review pass covers head `{head}`, the content this pull " + f"request carries at that commit against `{base}`.\n\n" + ) + return comment_on_pr(owner, repo, num, body) + + +def _git(checkout: str, *args: str) -> subprocess.CompletedProcess: + """One read-only git command in `checkout`, returned whole rather than raised.""" + try: + return subprocess.run( + ["git", "-C", checkout, *args], + capture_output=True, + text=True, + encoding="utf-8", + timeout=30, + check=False, + ) + except (OSError, subprocess.SubprocessError): + return subprocess.CompletedProcess([], 1, "", "git could not be run") + + +def attested(pr: dict) -> bool: + """Whether a comment from someone who can write here vouches for this pull request's head. + + Read over every comment rather than the reviewer's own, since an attestation is the + maintainer's account speaking. The association is what keeps a passer-by's comment carrying + the same marker from vouching for anything. + """ + head = pr.get("headRefOid") or "" + for node in (pr.get("comments") or {}).get("nodes") or []: + if (node.get("authorAssociation") or "") not in TRUSTED_ASSOCIATIONS: + continue + if any(m.group(1) == head for m in ATTESTATION.finditer(node.get("body") or "")): + return True + return False + + +def promotion(pr: dict) -> bool: + """Whether the pull request merges into the repository's default branch. + + That is the promotion, whose own Copilot round is the backstop reading the whole diff, so + the local pass never stands in for it. A default branch the payload does not name reads as + a promotion too, since the failure that way is one more Copilot request rather than a gate + passed on a local pass alone. + """ + default = ((pr.get("baseRepository") or {}).get("defaultBranchRef") or {}).get("name") or "" + return not default or pr.get("baseRefName") == default + + +def first_round_done(pr: dict) -> bool: + """Whether Copilot has reviewed this pull request at all, on any head, a refusal not counting.""" + return any(not refusal_of(n) for n in reviewer_nodes(pr, "reviews")) + + def reply_to_thread( owner: str, repo: str, num: int, match: str, body: str, path: str | None, resolve: bool ) -> int: @@ -3811,7 +3936,7 @@ def utf8_console() -> None: def main(argv: list[str] | None = None) -> int: utf8_console() ap = argparse.ArgumentParser() - ap.add_argument("cmd", choices=["claims", "comment", "status", "reply", "wait"]) + ap.add_argument("cmd", choices=["attest", "claims", "comment", "status", "reply", "wait"]) ap.add_argument("number", type=int) # No default, because the wrong repository is the failure this argument has actually had. # A default names one repository, and every run from elsewhere silently reads that one. @@ -3888,6 +4013,12 @@ def main(argv: list[str] | None = None) -> int: action="store_true", help="reply: resolve the thread once the reply is confirmed", ) + ap.add_argument( + "--checkout", + metavar="DIR", + help="attest: the checkout holding the pull request's head and its recorded local pass " + "(default the current directory)", + ) a = ap.parse_args(argv) # Named for the command they belong to, since one silently ignored reads as one that took effect. # A `status` given --body reports a clean digest and writes nothing. @@ -3901,6 +4032,8 @@ def main(argv: list[str] | None = None) -> int: for flag, value in reply_only.items(): if value is not None: ap.error(f"{flag} belongs to `reply`, not `{a.cmd}`") + if a.cmd != "attest" and a.checkout is not None: + ap.error(f"--checkout belongs to `attest`, 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 []) @@ -3937,6 +4070,9 @@ def main(argv: list[str] | None = None) -> int: if a.cmd == "claims": return check_claims(owner, repo, a.number) + if a.cmd == "attest": + return attest(owner, repo, a.number, a.checkout or ".") + if a.cmd == "comment": return comment_on_pr(owner, repo, a.number, a.body) diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index cc261e857..55d047f24 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -7011,6 +7011,7 @@ def test_a_cross_owner_target_is_refused_before_the_wait_reads_or_writes_anythin "Fixed.", ], "wait": ["wait", "7", "--repo", "someone-else/r"], + "attest": ["attest", "7", "--repo", "someone-else/r"], } # The parser's remaining `cmd` choices, none of which write. @@ -7047,6 +7048,113 @@ def test_each_write_command_refuses_before_reaching_either_transport(self) -> No gh_graphql.assert_not_called() +class TestAttest(unittest.TestCase): + """A local pass is published only for the content the pull request's head carries.""" + + def setUp(self) -> None: + self.dir = self.enterContext(tempfile.TemporaryDirectory()) + env = {**os.environ, "GIT_CONFIG_GLOBAL": os.devnull, "GIT_CONFIG_SYSTEM": os.devnull} + for args in ( + ["init", "-q", "-b", "feature"], + [ + "-c", + "user.name=t", + "-c", + "user.email=t@example.test", + "commit", + "-q", + "--allow-empty", + "-m", + "base", + ], + ): + subprocess.run(["git", "-C", self.dir, *args], check=True, env=env) + self.head = subprocess.run( + ["git", "-C", self.dir, "rev-parse", "HEAD"], + capture_output=True, + text=True, + check=True, + env=env, + ).stdout.strip() + self.enterContext(mock.patch.object(pr_review, "in_scope", return_value=(True, ""))) + self.posted: list[str] = [] + + def post(_o: str, _r: str, _n: int, body: str) -> int: + self.posted.append(body) + return 0 + + self.enterContext(mock.patch.object(pr_review, "comment_on_pr", side_effect=post)) + self.enterContext(contextlib.redirect_stdout(io.StringIO())) + + def run_attest(self, head: str, check_exit: int = 0) -> int: + stub = Path(self.dir).parent / f"stub-{check_exit}.py" + stub.write_text(f"import sys\nsys.exit({check_exit})\n") + self.addCleanup(stub.unlink) + target = {"headRefOid": head, "baseRefName": "develop"} + with ( + mock.patch.object(pr_review, "gql", return_value=target), + mock.patch.object(pr_review, "LOCAL_REVIEW", stub), + ): + return pr_review.attest("o", "r", 7, self.dir) + + def test_a_covered_head_is_attested_by_its_full_commit(self) -> None: + self.assertEqual(0, self.run_attest(self.head)) + self.assertEqual(1, len(self.posted)) + self.assertIn(f"", self.posted[0]) + + def test_a_checkout_at_another_commit_is_refused(self) -> None: + self.assertEqual(67, self.run_attest("f" * 40)) + self.assertEqual([], self.posted) + + def test_a_checkout_holding_changes_is_refused(self) -> None: + (Path(self.dir) / "extra.txt").write_text("not in the head\n") + self.assertEqual(67, self.run_attest(self.head)) + self.assertEqual([], self.posted) + + def test_no_current_local_pass_is_refused(self) -> None: + self.assertEqual(68, self.run_attest(self.head, check_exit=1)) + self.assertEqual([], self.posted) + + +class TestAttestationReadings(unittest.TestCase): + """What the gate reads an attestation, a promotion, and a first round from.""" + + def comment(self, body: str, association: str = "OWNER") -> dict: + return {"body": body, "authorAssociation": association, "author": {"login": "someone"}} + + def test_only_a_writer_s_marker_for_the_current_head_attests(self) -> None: + marker = f"" + for nodes, expected in ( + ([self.comment(marker)], True), + ([self.comment(marker, "COLLABORATOR")], True), + ([self.comment(marker, "NONE")], False), + ([self.comment(marker, "CONTRIBUTOR")], False), + ([self.comment(f"")], False), + ([], False), + ): + with self.subTest(nodes=nodes): + pr = {"headRefOid": HEAD, "comments": {"nodes": nodes}} + self.assertIs(expected, pr_review.attested(pr)) + + def test_a_promotion_is_a_pull_request_into_the_default_branch(self) -> None: + for base, default, expected in ( + ("main", "main", True), + ("develop", "main", False), + ("develop", None, True), + ): + with self.subTest(base=base, default=default): + pr = { + "baseRefName": base, + "baseRepository": {"defaultBranchRef": {"name": default} if default else None}, + } + self.assertIs(expected, pr_review.promotion(pr)) + + def test_a_refusal_is_not_a_first_round(self) -> None: + self.assertFalse(pr_review.first_round_done(payload([review(body=REFUSED)]))) + self.assertTrue(pr_review.first_round_done(payload([review(oid=OLD)]))) + self.assertFalse(pr_review.first_round_done(payload([]))) + + class TestWriteCommandsPartitionParserChoices(unittest.TestCase): """WRITE_COMMANDS and READ_ONLY_COMMANDS must together account for every parser choice. From 7482f18e1606dead28c7d1d00a2c2e3519f4bd34 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Thu, 1 Oct 2026 20:57:14 -0700 Subject: [PATCH 2/8] Request Copilot at Defined Moments and Cover Fix Pushes With an Attested Local Pass Co-Authored-By: Claude Opus 5.5 --- .agents/skills/pr-review-conduct/SKILL.md | 30 +++-- .../.source-digests/pr-review-conduct | 2 +- .../skills/pr-review-conduct/SKILL.md | 30 +++-- .github/copilot-instructions.md | 2 +- .github/skills/pr-review-conduct/SKILL.md | 30 +++-- AUDIT.md | 2 +- GOVERNANCE.md | 2 +- README.md | 2 +- RESYNC.md | 2 +- scripts/README.md | 3 + scripts/pr_review.py | 126 +++++++++++++++--- tests/test_pr_review.py | 84 ++++++++++++ 12 files changed, 260 insertions(+), 55 deletions(-) diff --git a/.agents/skills/pr-review-conduct/SKILL.md b/.agents/skills/pr-review-conduct/SKILL.md index e14a8e6b3..1ee7e9e2e 100644 --- a/.agents/skills/pr-review-conduct/SKILL.md +++ b/.agents/skills/pr-review-conduct/SKILL.md @@ -37,8 +37,11 @@ visible comments, routinely still carries a finding nobody has answered. Treatin 1. Required status checks are green, and where they are not, the reason is **read**, never inferred. `BLOCKED` covers a failed check, a required check nothing is running, an unresolved thread, and a missing approval alike, and the response differs by cause. -2. A review is confirmed on the **current head SHA**, matched by commit SHA rather than assumed - from a green merge-state. A push makes checks go green *before* the re-review lands, and the +2. A review is confirmed on the **current head SHA**. On a pull request into a branch other than + the default, a head after Copilot's first round can carry an attested local pass instead, + `review_on_head=local` in the digest, where no round on record states or appears to state + partial coverage, and a promotion's head always carries a Copilot round of its own. A review + is matched by commit SHA rather than assumed from a green merge-state. A push makes checks go green *before* the re-review lands, and the matched review is **read**, not just counted. A review can carry the head SHA and still decline the PR outright, or say it read only part of the changed files. Where the round covering the head states no coverage at all, the newest round that does state some stands in for it, and @@ -143,7 +146,8 @@ must have done. automatically, which is not the same as not reviewing at all. Comment the reviewer's documented review command, such as `@coderabbitai review`, and wait for the result as with any other requested review. The agent driving the loop posts that comment itself, on the same standing as - requesting a review after a push. + requesting a Copilot round, and at most once per pull request, on the head the drive judges + final, since the reviewer caps its reviews per pull request and its absence blocks nothing. - **A notice naming when the reviewer can next run is a rate limit, and asking does not clear it.** It reads like the skip notice above and is the opposite case: the trigger returns the same notice rather than a review, so a loop that keeps asking waits on something no amount of asking @@ -185,11 +189,18 @@ Run `local-strict-review` against the branch's current diff before every push th 1. Push changes to the PR branch and open the pull request when it does not exist. 2. Run `scripts/pr_review.py status --repo /` once in the foreground and read its output. -3. Re-request a review for the **current head SHA**. Auto-trigger is unreliable, so request it - explicitly, which step 4's `wait` is what does, though it skips the request where a review - already covers the head, where the answer came outside a formal review, where it detects - drift, and where something is already in the request set, which is the condition the recovery - below clears. Requesting in the pull request UI is the maintainer's route rather than this loop's. +3. Request a Copilot round at the defined moments only: the pull request's first round, and + the head of a promotion into the default branch. Auto-trigger is unreliable, so request it + explicitly, which step 4's `wait` is what does. On a fix push into any other branch after the + first round, `wait` requests nothing, since that push is covered by the pass the paragraph + above records. Publish it once the push lands by running `scripts/pr_review.py attest + --repo / --checkout `, which refuses unless the checkout is the + pushed head and a recorded pass covers it, and pass `wait --request` where a fix deserves a + Copilot round anyway. `wait` also skips the request where a review already covers the head, + where the answer came outside a formal review, where it detects drift, under a quota or error + refusal, and where something is already in the request set, which is the condition the + recovery below clears. Requesting in the pull request UI is the maintainer's route rather than + this loop's. 4. Run a bounded `scripts/pr_review.py wait --repo /` in a background process and read its terminal output. A completed review raising **no findings** is a valid terminal outcome, so do not re-trigger it or read silence as a missing review. A review whose body says it declined to review is the one @@ -199,7 +210,8 @@ Run `local-strict-review` against the branch's current diff before every push th 6. Apply fixes or write a rationale for declines. 7. Reply to each thread, and resolve what was addressed and what was declined on evidence the reviewer could check for itself, per outcome 2 below. -8. Re-run the loop after every fix push until the checks are green and no finding remains open. +8. Re-run the loop after every fix push until the checks are green, the current head is covered, + and no finding remains open. The review effort setting is user-controlled. The workflow never selects or changes it. `status` reports `effort=lite`, `effort=balanced`, or `effort=max` when the completed review exposes that metadata, lowercased, and names an inherited setting apart from a chosen one in a separate `effort_source=default|explicit` field, both reading `unknown` when no effort line parses. Missing effort metadata reports `unknown` and does not change coverage or completion. A pending effort-labeled request can complete without a `copilot_work_started` timeline event, so absence of that event never proves the request is abandoned. The bounded timeout reports `PENDING` when no review or terminal answer arrives. `requested=yes` reports that the request was accepted rather than that a round is coming. An accepted request can sit unpicked, printing the same digest as one about to be served, so a driver reading that field as progress is waiting on evidence it does not hold. After a timeout carrying it, rerun `wait` for another bounded interval by default, because the request may still be active. Where a second bounded wait times out as well, read the pending set, and clear it only where no human or team reviewer is requested alongside the bot, because the clear replaces that set rather than adding to it and nothing restores a request it drops. A stall on a pull request that has a human or team reviewer requested goes to the maintainer instead, and so does one still pending after the wait that follows a clear. The clear leaves the next `wait` nothing outstanding to defer to, so that run requests afresh, and its own auto-request line is what says so, since `wait` reads the reviewer's node id out of the repository's recent reviews and polls without requesting where it finds none. The hub's `docs/pr-reviewer-reference.md` carries the mutation, and an agent seat can run it, where removing and re-adding the reviewer in the pull request UI is a step only the maintainer can take. This recovery replaces only the review request and never changes the effort setting. A `wait` ending `REQUEST_NOT_RECORDED`, exit 48, is a different state and takes none of this recovery: the request returned success and left neither a pending reviewer nor a review-request event, which is how an exhausted Copilot allowance has shown itself, so clearing and requesting again does not clear it and it goes to the maintainer. diff --git a/.claude-plugin/fleet-skills/.source-digests/pr-review-conduct b/.claude-plugin/fleet-skills/.source-digests/pr-review-conduct index c52b2a67f..0accc60cf 100644 --- a/.claude-plugin/fleet-skills/.source-digests/pr-review-conduct +++ b/.claude-plugin/fleet-skills/.source-digests/pr-review-conduct @@ -1 +1 @@ -e611a6183eb65c72 +9bee557e230ab52f 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 e14a8e6b3..1ee7e9e2e 100644 --- a/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md @@ -37,8 +37,11 @@ visible comments, routinely still carries a finding nobody has answered. Treatin 1. Required status checks are green, and where they are not, the reason is **read**, never inferred. `BLOCKED` covers a failed check, a required check nothing is running, an unresolved thread, and a missing approval alike, and the response differs by cause. -2. A review is confirmed on the **current head SHA**, matched by commit SHA rather than assumed - from a green merge-state. A push makes checks go green *before* the re-review lands, and the +2. A review is confirmed on the **current head SHA**. On a pull request into a branch other than + the default, a head after Copilot's first round can carry an attested local pass instead, + `review_on_head=local` in the digest, where no round on record states or appears to state + partial coverage, and a promotion's head always carries a Copilot round of its own. A review + is matched by commit SHA rather than assumed from a green merge-state. A push makes checks go green *before* the re-review lands, and the matched review is **read**, not just counted. A review can carry the head SHA and still decline the PR outright, or say it read only part of the changed files. Where the round covering the head states no coverage at all, the newest round that does state some stands in for it, and @@ -143,7 +146,8 @@ must have done. automatically, which is not the same as not reviewing at all. Comment the reviewer's documented review command, such as `@coderabbitai review`, and wait for the result as with any other requested review. The agent driving the loop posts that comment itself, on the same standing as - requesting a review after a push. + requesting a Copilot round, and at most once per pull request, on the head the drive judges + final, since the reviewer caps its reviews per pull request and its absence blocks nothing. - **A notice naming when the reviewer can next run is a rate limit, and asking does not clear it.** It reads like the skip notice above and is the opposite case: the trigger returns the same notice rather than a review, so a loop that keeps asking waits on something no amount of asking @@ -185,11 +189,18 @@ Run `local-strict-review` against the branch's current diff before every push th 1. Push changes to the PR branch and open the pull request when it does not exist. 2. Run `scripts/pr_review.py status --repo /` once in the foreground and read its output. -3. Re-request a review for the **current head SHA**. Auto-trigger is unreliable, so request it - explicitly, which step 4's `wait` is what does, though it skips the request where a review - already covers the head, where the answer came outside a formal review, where it detects - drift, and where something is already in the request set, which is the condition the recovery - below clears. Requesting in the pull request UI is the maintainer's route rather than this loop's. +3. Request a Copilot round at the defined moments only: the pull request's first round, and + the head of a promotion into the default branch. Auto-trigger is unreliable, so request it + explicitly, which step 4's `wait` is what does. On a fix push into any other branch after the + first round, `wait` requests nothing, since that push is covered by the pass the paragraph + above records. Publish it once the push lands by running `scripts/pr_review.py attest + --repo / --checkout `, which refuses unless the checkout is the + pushed head and a recorded pass covers it, and pass `wait --request` where a fix deserves a + Copilot round anyway. `wait` also skips the request where a review already covers the head, + where the answer came outside a formal review, where it detects drift, under a quota or error + refusal, and where something is already in the request set, which is the condition the + recovery below clears. Requesting in the pull request UI is the maintainer's route rather than + this loop's. 4. Run a bounded `scripts/pr_review.py wait --repo /` in a background process and read its terminal output. A completed review raising **no findings** is a valid terminal outcome, so do not re-trigger it or read silence as a missing review. A review whose body says it declined to review is the one @@ -199,7 +210,8 @@ Run `local-strict-review` against the branch's current diff before every push th 6. Apply fixes or write a rationale for declines. 7. Reply to each thread, and resolve what was addressed and what was declined on evidence the reviewer could check for itself, per outcome 2 below. -8. Re-run the loop after every fix push until the checks are green and no finding remains open. +8. Re-run the loop after every fix push until the checks are green, the current head is covered, + and no finding remains open. The review effort setting is user-controlled. The workflow never selects or changes it. `status` reports `effort=lite`, `effort=balanced`, or `effort=max` when the completed review exposes that metadata, lowercased, and names an inherited setting apart from a chosen one in a separate `effort_source=default|explicit` field, both reading `unknown` when no effort line parses. Missing effort metadata reports `unknown` and does not change coverage or completion. A pending effort-labeled request can complete without a `copilot_work_started` timeline event, so absence of that event never proves the request is abandoned. The bounded timeout reports `PENDING` when no review or terminal answer arrives. `requested=yes` reports that the request was accepted rather than that a round is coming. An accepted request can sit unpicked, printing the same digest as one about to be served, so a driver reading that field as progress is waiting on evidence it does not hold. After a timeout carrying it, rerun `wait` for another bounded interval by default, because the request may still be active. Where a second bounded wait times out as well, read the pending set, and clear it only where no human or team reviewer is requested alongside the bot, because the clear replaces that set rather than adding to it and nothing restores a request it drops. A stall on a pull request that has a human or team reviewer requested goes to the maintainer instead, and so does one still pending after the wait that follows a clear. The clear leaves the next `wait` nothing outstanding to defer to, so that run requests afresh, and its own auto-request line is what says so, since `wait` reads the reviewer's node id out of the repository's recent reviews and polls without requesting where it finds none. The hub's `docs/pr-reviewer-reference.md` carries the mutation, and an agent seat can run it, where removing and re-adding the reviewer in the pull request UI is a step only the maintainer can take. This recovery replaces only the review request and never changes the effort setting. A `wait` ending `REQUEST_NOT_RECORDED`, exit 48, is a different state and takes none of this recovery: the request returned success and left neither a pending reviewer nor a review-request event, which is how an exhausted Copilot allowance has shown itself, so clearing and requesting again does not clear it and it goes to the maintainer. diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 4fbb79f20..a32801db2 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -28,7 +28,7 @@ For every review: The review automation is `scripts/pr_review.py`, run from a hub checkout. Use its `status`, `wait`, `comment`, and `reply --resolve` commands instead of reconstructing GraphQL queries or copying review identifiers by hand. Use `comment` for a suppressed-finding answer in the pull request conversation. Its status gate verifies the current head, diff coverage, output shape, inline threads, body-only findings, and required checks. -A formal review with no findings is complete only when it covers the current head and full diff coverage of the change set that head has is stated, or read from a file table as below. The round covering the head states it, or the newest round that states it at all does and the pull request changes the same set of files at both commits, which is the only condition under which a statement carries forward. Only that newest round is consulted, so an older round whose change set does match carries nothing. Where no statement reaches the head either way, a round covering it whose own file table names exactly the changed files covers it, which is the reading a round at Balanced review effort gives, and a statement that reaches the head, stated on it or carried to it, wins over that table. Where no round covering the head carries a table of its own, the newest round that does, its table naming exactly the changed files, stands in under the bound a statement carries under, the pull request changing the same set of files at both commits. The table stands in only where no Copilot round on the pull request, on any commit, states or appears to state partial coverage, so a pull request that ever had a partial round goes to the maintainer. A round reporting partial coverage of the diff blocks the merge, and so does a refusal, a coverage statement that does not reach this head, meaning absent from every round or carried by none because the change set moved or could not be compared, with no file table standing in for it, an unrecognized output shape, an unresolved thread, or a body-only finding. Re-run the loop after every fix push. Never infer review completion from `mergeStateStatus: CLEAN`. +A formal review with no findings is complete only when it covers the current head and full diff coverage of the change set that head has is stated, or read from a file table as below. The round covering the head states it, or the newest round that states it at all does and the pull request changes the same set of files at both commits, which is the only condition under which a statement carries forward. Only that newest round is consulted, so an older round whose change set does match carries nothing. Where no statement reaches the head either way, a round covering it whose own file table names exactly the changed files covers it, which is the reading a round at Balanced review effort gives, and a statement that reaches the head, stated on it or carried to it, wins over that table. Where no round covering the head carries a table of its own, the newest round that does, its table naming exactly the changed files, stands in under the bound a statement carries under, the pull request changing the same set of files at both commits. The table stands in only where no Copilot round on the pull request, on any commit, states or appears to state partial coverage, so a pull request that ever had a partial round goes to the maintainer. A round reporting partial coverage of the diff blocks the merge, and so does a refusal, a coverage statement that does not reach this head, meaning absent from every round or carried by none because the change set moved or could not be compared, with no file table standing in for it, an unrecognized output shape, an unresolved thread, or a body-only finding. On a pull request into a branch other than the default, a fix push after Copilot's first round is covered by an attested local pass rather than another Copilot round, `review_on_head=local` in the digest, while a promotion's head always carries a Copilot round of its own. Re-run the loop after every fix push. Never infer review completion from `mergeStateStatus: CLEAN`. Review effort is user-controlled. The automation observes `Lite`, `Balanced`, or `Max`, including an inherited `Default ()`, and never selects or changes the setting. Effort does not determine coverage or completion. A request can complete without a `copilot_work_started` event, so absence of that event is not a stalled-review verdict. When `wait` returns `PENDING` with `requested=yes`, report the state and rerun `wait` for another bounded interval by default, reading that field as acceptance of the request rather than as delivery of a round. Do not clear the request on that first timeout, because it may still be active. Where a second bounded wait times out as well, read the pending set, clear it only where no human or team reviewer is requested alongside the bot, and rerun `wait`, which then has nothing outstanding to defer to and requests afresh, or polls and says so on its own auto-request line where it finds no reviewer node id to request with. The clear replaces that set rather than adding to it and nothing restores a request it drops, so a stall on a pull request that has a human or team reviewer requested goes to the maintainer, and so does one still pending after the wait that follows a clear. That clear is a recovery step the script does not implement, and `docs/pr-reviewer-reference.md`, in the hub checkout the script is run from, carries it. This recovery replaces only the review request and never changes the effort setting. A `wait` ending `REQUEST_NOT_RECORDED`, exit 48, is a different state and takes none of this recovery: the request returned success and left neither a pending reviewer nor a review-request event, which is how an exhausted Copilot allowance has shown itself, so clearing and requesting again does not clear it and it goes to the maintainer. diff --git a/.github/skills/pr-review-conduct/SKILL.md b/.github/skills/pr-review-conduct/SKILL.md index e14a8e6b3..1ee7e9e2e 100644 --- a/.github/skills/pr-review-conduct/SKILL.md +++ b/.github/skills/pr-review-conduct/SKILL.md @@ -37,8 +37,11 @@ visible comments, routinely still carries a finding nobody has answered. Treatin 1. Required status checks are green, and where they are not, the reason is **read**, never inferred. `BLOCKED` covers a failed check, a required check nothing is running, an unresolved thread, and a missing approval alike, and the response differs by cause. -2. A review is confirmed on the **current head SHA**, matched by commit SHA rather than assumed - from a green merge-state. A push makes checks go green *before* the re-review lands, and the +2. A review is confirmed on the **current head SHA**. On a pull request into a branch other than + the default, a head after Copilot's first round can carry an attested local pass instead, + `review_on_head=local` in the digest, where no round on record states or appears to state + partial coverage, and a promotion's head always carries a Copilot round of its own. A review + is matched by commit SHA rather than assumed from a green merge-state. A push makes checks go green *before* the re-review lands, and the matched review is **read**, not just counted. A review can carry the head SHA and still decline the PR outright, or say it read only part of the changed files. Where the round covering the head states no coverage at all, the newest round that does state some stands in for it, and @@ -143,7 +146,8 @@ must have done. automatically, which is not the same as not reviewing at all. Comment the reviewer's documented review command, such as `@coderabbitai review`, and wait for the result as with any other requested review. The agent driving the loop posts that comment itself, on the same standing as - requesting a review after a push. + requesting a Copilot round, and at most once per pull request, on the head the drive judges + final, since the reviewer caps its reviews per pull request and its absence blocks nothing. - **A notice naming when the reviewer can next run is a rate limit, and asking does not clear it.** It reads like the skip notice above and is the opposite case: the trigger returns the same notice rather than a review, so a loop that keeps asking waits on something no amount of asking @@ -185,11 +189,18 @@ Run `local-strict-review` against the branch's current diff before every push th 1. Push changes to the PR branch and open the pull request when it does not exist. 2. Run `scripts/pr_review.py status --repo /` once in the foreground and read its output. -3. Re-request a review for the **current head SHA**. Auto-trigger is unreliable, so request it - explicitly, which step 4's `wait` is what does, though it skips the request where a review - already covers the head, where the answer came outside a formal review, where it detects - drift, and where something is already in the request set, which is the condition the recovery - below clears. Requesting in the pull request UI is the maintainer's route rather than this loop's. +3. Request a Copilot round at the defined moments only: the pull request's first round, and + the head of a promotion into the default branch. Auto-trigger is unreliable, so request it + explicitly, which step 4's `wait` is what does. On a fix push into any other branch after the + first round, `wait` requests nothing, since that push is covered by the pass the paragraph + above records. Publish it once the push lands by running `scripts/pr_review.py attest + --repo / --checkout `, which refuses unless the checkout is the + pushed head and a recorded pass covers it, and pass `wait --request` where a fix deserves a + Copilot round anyway. `wait` also skips the request where a review already covers the head, + where the answer came outside a formal review, where it detects drift, under a quota or error + refusal, and where something is already in the request set, which is the condition the + recovery below clears. Requesting in the pull request UI is the maintainer's route rather than + this loop's. 4. Run a bounded `scripts/pr_review.py wait --repo /` in a background process and read its terminal output. A completed review raising **no findings** is a valid terminal outcome, so do not re-trigger it or read silence as a missing review. A review whose body says it declined to review is the one @@ -199,7 +210,8 @@ Run `local-strict-review` against the branch's current diff before every push th 6. Apply fixes or write a rationale for declines. 7. Reply to each thread, and resolve what was addressed and what was declined on evidence the reviewer could check for itself, per outcome 2 below. -8. Re-run the loop after every fix push until the checks are green and no finding remains open. +8. Re-run the loop after every fix push until the checks are green, the current head is covered, + and no finding remains open. The review effort setting is user-controlled. The workflow never selects or changes it. `status` reports `effort=lite`, `effort=balanced`, or `effort=max` when the completed review exposes that metadata, lowercased, and names an inherited setting apart from a chosen one in a separate `effort_source=default|explicit` field, both reading `unknown` when no effort line parses. Missing effort metadata reports `unknown` and does not change coverage or completion. A pending effort-labeled request can complete without a `copilot_work_started` timeline event, so absence of that event never proves the request is abandoned. The bounded timeout reports `PENDING` when no review or terminal answer arrives. `requested=yes` reports that the request was accepted rather than that a round is coming. An accepted request can sit unpicked, printing the same digest as one about to be served, so a driver reading that field as progress is waiting on evidence it does not hold. After a timeout carrying it, rerun `wait` for another bounded interval by default, because the request may still be active. Where a second bounded wait times out as well, read the pending set, and clear it only where no human or team reviewer is requested alongside the bot, because the clear replaces that set rather than adding to it and nothing restores a request it drops. A stall on a pull request that has a human or team reviewer requested goes to the maintainer instead, and so does one still pending after the wait that follows a clear. The clear leaves the next `wait` nothing outstanding to defer to, so that run requests afresh, and its own auto-request line is what says so, since `wait` reads the reviewer's node id out of the repository's recent reviews and polls without requesting where it finds none. The hub's `docs/pr-reviewer-reference.md` carries the mutation, and an agent seat can run it, where removing and re-adding the reviewer in the pull request UI is a step only the maintainer can take. This recovery replaces only the review request and never changes the effort setting. A `wait` ending `REQUEST_NOT_RECORDED`, exit 48, is a different state and takes none of this recovery: the request returned success and left neither a pending reviewer nor a review-request event, which is how an exhausted Copilot allowance has shown itself, so clearing and requesting again does not clear it and it goes to the maintainer. diff --git a/AUDIT.md b/AUDIT.md index bf4fc5301..26e65f551 100644 --- a/AUDIT.md +++ b/AUDIT.md @@ -223,7 +223,7 @@ flowchart LR Sections 0-9 (the audit and its report) are **read-only** and never touch the target. **Converging** is the separate follow-on phase: the drift the report found is **resolved by applying fixes to the target repo**, not left as a report. The convergence loop: - **Apply via a pull request on the target repo.** Branch from the target's `develop` (or `main` for a `main`-only repo), make the fix, and open a PR. Never push a fix directly to a protected branch, and never hand-edit a target outside a PR. -- **Drive the PR's review to green** - the same loop the hub runs (see [GOVERNANCE.md "PR Review Etiquette"][governance-pr-review-etiquette], and the [Copilot review runbook][copilot-runbook] in `.github/copilot-instructions.md` for that one reviewer's mechanics): request review on every push, address and resolve every thread from every reviewer the repo has configured, and confirm the review covers the head SHA. +- **Drive the PR's review to green** - the same loop the hub runs (see [GOVERNANCE.md "PR Review Etiquette"][governance-pr-review-etiquette], and the [Copilot review runbook][copilot-runbook] in `.github/copilot-instructions.md` for that one reviewer's mechanics): request review at the defined moments, address and resolve every thread from every reviewer the repo has configured, and confirm the review covers the head SHA. - **Merge only with explicit maintainer approval.** The agent drives to green and stops. The maintainer merges. - **One focused PR per drift class**, cross-referencing the audit finding. A sprawling all-drifts PR draws many review rounds and never feels done. - **A `hub-only:` finding converges by deleting the file, not by updating it.** That prefix is how `spec/audit.py` reports the **carried-scope** dimension section 4 names. It is the one class where the fix removes content, so it is easy to convert into a re-vendor by reflex and end up refreshing a copy that should not exist. Delete the repo's copy and reach the hub's per [GOVERNANCE.md "Hub-Hosted Tooling"][governance-hub-hosted-tooling]. Confirm the disposition is `retire` before deleting anything: an untriaged hit may be the repo's own content at a shared path, and deleting that destroys work the hub never owned. diff --git a/GOVERNANCE.md b/GOVERNANCE.md index 51e8ab1bc..824cb99d0 100644 --- a/GOVERNANCE.md +++ b/GOVERNANCE.md @@ -212,7 +212,7 @@ The checks that separate work actually done from work that merely reports succes ## PR Review Etiquette -The provider-agnostic review-loop contract every fleet repo follows starts when a pull request opens. Open every fleet-owned pull request ready for review. Draft state is reserved for the separately documented upstream contribution workflow while a third-party contribution is still being prepared. Creating the pull request is not a terminal handoff. Run the review status once in the foreground. Then start the bounded review wait in a background process. Request a review on every push. Confirm it covers the current head SHA and the full diff rather than only part of it. Where the round covering the head states no coverage at all, read the newest round that does state some as covering this head only where the pull request changes the same set of files at both commits, a head round's own statement always winning over a carried one. Where no statement reaches the head either way, a round covering it whose own file table names exactly the changed files covers it, which is the reading a Copilot round at Balanced review effort gives, and a statement that reaches the head, stated on it or carried to it, wins over that table. Where no round covering the head carries a table of its own, the newest round that does, its table naming exactly the changed files, stands in under the bound a statement carries under, the pull request changing the same set of files at both commits. The table stands in only where no Copilot round on the pull request, on any commit, states or appears to state partial coverage, so a pull request that ever had a partial round goes to the maintainer. Triage every finding, including low-confidence findings collapsed into the review body rather than threads. Reply to and resolve every addressed finding. Repeat after every fix until the checks are green and the current-head review leaves no finding open. Only an explicit maintainer instruction may stop, defer, or alter this default. Silence or a request that says only "open a PR" is not such an instruction. Never merge on a green or CLEAN merge state alone. That state does not prove the review covered the current head SHA and full diff. It also does not expose unanswered low-confidence findings that opened no thread. +The provider-agnostic review-loop contract every fleet repo follows starts when a pull request opens. Open every fleet-owned pull request ready for review. Draft state is reserved for the separately documented upstream contribution workflow while a third-party contribution is still being prepared. Creating the pull request is not a terminal handoff. Run the review status once in the foreground. Then start the bounded review wait in a background process. Request a review at defined moments rather than on every push: when the pull request opens, and on the head of a promotion into the default branch. On a pull request into any other branch, a fix push after the first round is covered instead by the recorded local pass the push already owes, published to the pull request so the review status can read it. Confirm that a review, or for such a fix push that published pass, covers the current head SHA, and that a review covers the full diff rather than only part of it. Where the round covering the head states no coverage at all, read the newest round that does state some as covering this head only where the pull request changes the same set of files at both commits, a head round's own statement always winning over a carried one. Where no statement reaches the head either way, a round covering it whose own file table names exactly the changed files covers it, which is the reading a Copilot round at Balanced review effort gives, and a statement that reaches the head, stated on it or carried to it, wins over that table. Where no round covering the head carries a table of its own, the newest round that does, its table naming exactly the changed files, stands in under the bound a statement carries under, the pull request changing the same set of files at both commits. The table stands in only where no Copilot round on the pull request, on any commit, states or appears to state partial coverage, so a pull request that ever had a partial round goes to the maintainer. Triage every finding, including low-confidence findings collapsed into the review body rather than threads. Reply to and resolve every addressed finding. Repeat after every fix until the checks are green, the current head is covered, and no finding is left open. Only an explicit maintainer instruction may stop, defer, or alter this default. Silence or a request that says only "open a PR" is not such an instruction. Never merge on a green or CLEAN merge state alone. That state does not prove the review covered the current head SHA and full diff. It also does not expose unanswered low-confidence findings that opened no thread. This is packaged as the `pr-review-conduct` Skill at `.agents/skills/pr-review-conduct/SKILL.md` in the hub, not a repo-relative link since that path is hub-local and not carried into every fleet repo. The summary above sketches the contract. Read the skill for the merge gate, the expected loop, and how a finding is closed. diff --git a/README.md b/README.md index 164d6f22c..ca0f101f7 100644 --- a/README.md +++ b/README.md @@ -103,7 +103,7 @@ This repo is the single home for those rules, a machine-readable spec they are c - **Stand a repository up** - carry the baseline a project is owed for its declared types and workflow model, per [STANDUP.md][standup]. An absent file is a baseline that never arrived rather than drift. - **Audit a repository** - read a live project against the spec and report its drift, per [AUDIT.md][audit]. The audit never edits what it measures, so a fix is a separate change. - **Resync a repository** - bring an already-stood-up project up to the current hub, per [RESYNC.md][resync]. It audits for the findings and then applies them in order, which includes deleting what the hub hosts rather than carries. -- **Close the review loop** - request a review on every push, confirm it covered the head commit, triage every finding, reply and resolve, and escalate when stuck, per [GOVERNANCE.md "PR Review Etiquette"][governance-pr-review-etiquette]. +- **Close the review loop** - request a review at the defined moments, confirm the head commit is covered, triage every finding, reply and resolve, and escalate when stuck, per [GOVERNANCE.md "PR Review Etiquette"][governance-pr-review-etiquette]. - **Carried against reached** - a project carries the content it is audited against and reaches the machinery that is identical everywhere, per [GOVERNANCE.md "Hub-Hosted Tooling"][governance-hub-hosted-tooling]. ## What It Achieves diff --git a/RESYNC.md b/RESYNC.md index ecc78f83d..fbe5f7333 100644 --- a/RESYNC.md +++ b/RESYNC.md @@ -118,7 +118,7 @@ The other half is section 4 of [`AUDIT.md`][audit]: no check belonging to a proj - **One focused pull request per drift class**, branched from the target's `develop`, cross-referencing the finding it closes. A sprawling all-drifts pull request draws many review rounds and never feels done. - **Never push a fix directly to a protected branch**, and never hand-edit a target outside a pull request. An operational repository commits to `develop` directly by design, and a conformance change is still a reviewable change. -- **Close the review loop.** Request a review on every push, confirm it covered the head commit, and answer and resolve every thread, per [GOVERNANCE.md "PR Review Etiquette"][governance-pr-review-etiquette] and the [Copilot review runbook][copilot-runbook]. +- **Close the review loop.** Request a review at the defined moments, confirm the head commit is covered, and answer and resolve every thread, per [GOVERNANCE.md "PR Review Etiquette"][governance-pr-review-etiquette] and the [Copilot review runbook][copilot-runbook]. - **The maintainer merges.** The agent drives to green and stops. - **Fix systemic drift in the hub instead.** Where many repositories share a drift, fix the rule or add a check here and let a re-audit re-flag it, rather than hand-patching each repository for a shared cause. diff --git a/scripts/README.md b/scripts/README.md index d3bd8bde5..77a63dd2d 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -172,8 +172,11 @@ python3 scripts/pr_review.py comment 452 --repo ptr727/ProjectTemplate \ --body "Suppressed findings (1): **Disproven** - the target is checked before the write." python3 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 +python3 scripts/pr_review.py attest 452 --repo ptr727/ProjectTemplate --checkout ../worktree ``` +**A Copilot round is requested at defined moments rather than on every push.** The two moments are a pull request's first round and the head of a promotion into the default branch. On a pull request into any other branch, a fix push after the first round 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 ``, and only after confirming the checkout is the pushed head, holds no other change, and passes `local_review.py check`. `status` then reads that head as `review_on_head=local` with `coverage=local`, where the attestation comes from someone who can write to the repository and no round on record states or appears to state partial coverage. `wait` requests nothing on such a head, ending as covered where it is attested and exiting `49` where it is not, and `--request` asks for a round anyway. The rulesets review a pull request when it opens and not on each push, since a trigger on push would spend a round however the tooling chose. 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: the digest renders, every field is well-formed, and nothing in the output disagrees. Two runs read this repository's pull requests while their own was the subject, each caught by the maintainer rather than by the run. The digest leads with `repo=OWNER/NAME` for the same reason, since a number alone reads as correct in any repository. A value that is not `OWNER/NAME` is rejected by name rather than raised as an unpacking traceback, that being the near-miss a required argument still admits. **Do not infer one field's spelling from another's.** Several states print upper-case, `merge=` reproduces GitHub's own enum verbatim, and a field can arrive behind a prefix, so a pattern built by analogy with a neighboring field silently never fires. `review_on_head` is the one that catches a watcher, rendering `yes` against `NO`, so a pattern written for `YES` waits out its whole bound over a review that landed. A watcher that cannot fire reads exactly like a review that has not landed, so run the command once, read a real line, and write the pattern against that. Anchor the pattern on what is unique to the field being read, meaning that field's own name with its value, and for a pair nested in a parenthesized group the group's name ahead of it, since values repeat across fields and the groups carry the same inner names as each other. A watcher testing for a bare negative matches a line a covered review wrote, so it reports no review over a review that landed and waits out its whole bound saying so. That one matches on the wrong evidence rather than failing to match, which is worse than the casing trap above, since its answer is the digest's own inverted rather than absent. Give the watcher a branch that exits non-zero where the digest cannot be read at all, or a pattern that never fires and a command that never ran are equally silent. diff --git a/scripts/pr_review.py b/scripts/pr_review.py index ab697107c..8b949abc6 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -8,6 +8,15 @@ Discipline" for the rule this implements. Subcommands + attest Publish that a recorded local pass covers the pull request's head, as a comment carrying + `` that `status` and `wait` read. It reads the + receipt `local_review.py` keeps in the checkout's git directory, which nothing on GitHub + can, and refuses unless the checkout (--checkout, default the current directory) is at + the pull request's head with no change beyond it and `local_review.py check` passes + against the base. Exit 0 = posted, 64 = the write scope could not be established or + excludes the target, 65 = the pull request could not be read, 66 = the response did not + confirm the comment, 67 = the checkout is not the head or holds changes, 68 = no current + local pass covers the content. comment Post one PR-conversation answer, including a suppressed-finding disposition. The PR 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 @@ -31,6 +40,10 @@ commit while Copilot's own `review_on_head` still reads `NO`, and an empty body from that other reviewer on that head is its own ordinary "reviewed, nothing to flag" shape, the same reading an empty-bodied Copilot round already gets, not a gap. + `review_on_head=local` reads a head no Copilot round covers on a pull request into a + branch other than the default, after Copilot's first round, where an attestation vouches + for it and no round on record states or appears to state partial coverage, with + `coverage=local` beside it. Use `wait` when review presence is the condition, since `status` reports an absent review without treating it as a failure. 42 = a round read fewer files than the pull request changed, so part of the diff @@ -168,20 +181,24 @@ done, 60 = no thread matched, 61 = more than one did, 62 = the reply returned no comment url so nothing was resolved, 63 = the resolve did not report the thread resolved, 64 = the write scope could not be established or excludes the target. - wait Request a review where none is outstanding, then poll until Copilot's review lands - on the current head, then print the digest. The auto-request is skipped once a - review already covers the head, once Copilot has already answered outside a formal - review, or once one is already in the pending request set, so calling `wait` again on the - same PR never double-requests. It is also skipped under 46's and 47's quota readings - below, since a request into a reached limit spends quota and returns the same refusal. It - reads the Copilot reviewer's bot id from the repository's own most recently updated PRs - rather than a fixed id: the last HISTORY_PRS, widened once to HISTORY_PRS_WIDE where that - narrow window carries no Copilot activity at all, since an outage that outlasts - HISTORY_PRS PRs would otherwise empty it on every call for as long as the outage runs. - Requests nothing (falling back to polling only) where both windows come up empty, since a - repository with no Copilot review in either has nothing to read the id from and a - fabricated one is never an option. The loop runs in-process, so a 45-minute wait costs - one agent turn, not 90. + wait Request a review where none is outstanding, then poll until Copilot's review lands on the + current head, then print the digest. The auto-request is skipped once a review already + covers the head, once Copilot has already answered outside a formal review, or once one + is already in the pending request set, so calling `wait` again on the same PR never + double-requests. It is also skipped under 46's and 47's quota readings below, since a + request into a reached limit spends quota and returns the same refusal. It is skipped too + on a pull request into a branch other than the default once Copilot has reviewed it at + all, since a fix push there is covered by an attested local pass: an attested head ends + the wait as covered, and one with no attestation exits 49 naming the `attest` step. + --request asks for a round anyway. A promotion into the default branch, and a pull + request Copilot has not reviewed yet, are requested as before. It reads the Copilot + reviewer's bot id from the repository's own most recently updated PRs rather than a fixed + id: the last HISTORY_PRS, widened once to HISTORY_PRS_WIDE where that narrow window + carries no Copilot activity at all, since an outage that outlasts HISTORY_PRS PRs would + otherwise empty it on every call for as long as the outage runs. Requests nothing + (falling back to polling only) where both windows come up empty, since a repository with + no Copilot review in either has nothing to read the id from and a fabricated one is never + an option. The loop runs in-process, so a 45-minute wait costs one agent turn, not 90. Exit 0 = review present, 30 = still pending at timeout (pending is not failure), 40 = Copilot answered outside a formal review, so read the printed body. 40 reports the shape of that answer and reads nothing of its cause: an answer @@ -230,6 +247,10 @@ request exists to answer. It cannot meet 46 or 47, since neither sends a request, and ranks under 0/40/41/42/43/44/45. `status` cannot report it, since a request that recorded nothing leaves nothing for a later read to find. + 49 = no Copilot round covers this head and none was requested, the pull request merging + into a branch other than the default after Copilot's first round, and no attestation + vouches for the head. Run the local strict review, record it, push, and run `attest`, or + pass --request. 64 = the write scope could not be established or excludes the target, checked before the auto-request or any poll, so a cross-owner target reads and writes nothing here. @@ -503,6 +524,7 @@ def strip_fences( # Not a state a round reports, but the reading that carries an earlier round's forward. CARRIED = "carried" TABLE = "table" +LOCAL = "local" NO_HEAD_TABLE = "no round covering the head carries a file table of its own" SEVERITY = (UNVETTED, PARTIAL, FULL, UNSTATED) # Upper-case for the two that block a merge, for the reason `review_on_head=NO` is upper-case. @@ -515,6 +537,7 @@ def strip_fences( FULL: "full", UNSTATED: "unstated", TABLE: "table", + LOCAL: "local", } # Every structural marker the reviewer's own bodies carry, measured over the same 333. @@ -763,10 +786,11 @@ def strip_fences( query($o:String!,$r:String!,$n:Int!){ repository(owner:$o,name:$r){ pullRequest(number:$n){ headRefOid baseRefName mergeable mergeStateStatus + baseRepository{ defaultBranchRef{ name } } reviews(last:100){ nodes{ id author{login} state commit{oid} submittedAt body } pageInfo{ hasPreviousPage } } reviewThreads(first:100){ nodes{ id isResolved comments(first:1){ nodes{ author{login} path line body pullRequestReview{ id } } } } pageInfo{ hasNextPage } } - comments(last:100){ nodes{ author{login} createdAt body } pageInfo{ hasPreviousPage } } + comments(last:100){ nodes{ author{login} authorAssociation createdAt body } pageInfo{ hasPreviousPage } } reviewRequests(first:10){ nodes{ requestedReviewer{ __typename ... on Bot{login} ... on User{login} } } } files(first:__FILES_WINDOW__){ pageInfo{ hasNextPage } nodes{ path } } commits(last:1){ nodes{ commit{ oid statusCheckRollup{ state @@ -3105,6 +3129,9 @@ def digest( ) if cover == UNSTATED and on_head and not table_short: cover = TABLE + local = not on_head and local_cover(pr) + if local: + cover = LOCAL unknown = unrecognized_shapes(pr) threads = pr["reviewThreads"]["nodes"] # True where the connection cut off before this pull request's actual thread count. @@ -3159,7 +3186,7 @@ def digest( # Spent where coverage of the same head landed, the precedence the exit codes already hold. # Reported regardless, it prints `review_on_head=yes refusal=YES` over a reviewed head. # That tells a reader to split a pull request the reviewer has just reviewed. - refusal = None if on_head else (refusing_review(pr) or stopping_refusal(pr)) + refusal = stopping_refusal(pr) or (None if on_head else refusing_review(pr)) # Read once and handed to the line below, since `quota_refusal` re-walks `refusal_of`. refusal_field = ( "no" @@ -3212,7 +3239,7 @@ def digest( # The repository leads the line, since a number alone reads as correct anywhere. # A digest of the wrong pull request is well-formed, so naming it is what shows the miss. f"repo={owner}/{repo} pr={num} head={head[:8]} rounds={len(revs)} " - f"review_on_head={'yes' if on_head else 'NO'} " + f"review_on_head={'yes' if on_head else 'local' if local else 'NO'} " # Present only where at least one other tracked reviewer has posted on this exact head. # No verdict rides on it, unlike `review_on_head`, since nothing here reads what a CodeRabbit or qodo round said, only that one landed. + (f"other_reviewed={','.join(other_on_head)} " if other_on_head else "") @@ -3371,6 +3398,12 @@ def digest( ) elif cover == UNSTATED and on_head: lines.append(f" NO FILE TABLE STANDS IN: {table_short}") + elif cover == LOCAL: + lines.append( + " COVERAGE IS READ FROM THE LOCAL PASS: no Copilot round covers this head, which " + "follows the pull request's first round, and a comment from someone who can write " + "here attests a recorded local strict-review pass over exactly this head's content" + ) if cover == PARTIAL: # The line prints under the marker for the reason a suppressed block does. # The counts say how much of the diff went unread, and no thread carries them. @@ -3856,6 +3889,24 @@ def first_round_done(pr: dict) -> bool: return any(not refusal_of(n) for n in reviewer_nodes(pr, "reviews")) +def local_cover(pr: dict) -> bool: + """Whether a recorded local pass stands in for a Copilot round on this head. + + Only on a pull request into a branch other than the default, after Copilot's first round, on + a head its writer attests, and only where nothing on record says any round read part of a + diff and the whole review history is in view, the bound a file table stands in under. The + maintainer's answer on the hub's issue 2261 set the moments: a fix push after the first + round is covered by the local pass, and a promotion keeps its own Copilot round. + """ + return ( + not promotion(pr) + and first_round_done(pr) + and attested(pr) + and not reviews_truncated(pr) + and not partial_shaped(pr) + ) + + def reply_to_thread( owner: str, repo: str, num: int, match: str, body: str, path: str | None, resolve: bool ) -> int: @@ -4175,6 +4226,12 @@ def main(argv: list[str] | None = None) -> int: "answering it since, or this pull request's newest Copilot review is a quota or " "error refusal, pass this once the quota is believed to have reset", ) + ap.add_argument( + "--request", + action="store_true", + help="wait: request a Copilot round even on a fix push into a branch other than the " + "default, which an attested local pass otherwise covers", + ) ap.add_argument( "--min-rounds", type=int, @@ -4315,10 +4372,14 @@ def main(argv: list[str] | None = None) -> int: # Read from the same, unfiltered history rather than one that drops this pull request's own entries: a genuine review on an earlier head of this same pull request, superseded since by a push, is real evidence about the account and not a self-reference to discard. # A refusal on this pull request's own current head still never reaches this signal, since it is caught directly and at higher priority first. signal = None if a.ignore_quota_signal else quota_signal(history) - stopped = ( - None - if a.ignore_quota_signal or done or answer or drift - else stopping_refusal(gql(Q_FULL, owner, repo, a.number)) + snapshot = None if done or answer or drift else gql(Q_FULL, owner, repo, a.number) + stopped = None if a.ignore_quota_signal or snapshot is None else stopping_refusal(snapshot) + held = ( + snapshot is not None + and not a.request + and not promotion(snapshot) + and first_round_done(snapshot) + and not reviewer_requested(pr) ) # Request before the first poll, not just at the call site: a caller expects `wait` to make a review happen, not merely to watch for one. # Two prior gaps this closed, a push superseding an already-answered request and an auto-seed that never fired, both left nothing outstanding for the loop below to ever see land. @@ -4331,6 +4392,7 @@ def main(argv: list[str] | None = None) -> int: and not drift and not signal and not stopped + and not held and not reviewer_requested(pr) ): line, recorded = request_copilot_review( @@ -4349,6 +4411,14 @@ def main(argv: list[str] | None = None) -> int: "request, no pending reviewer and no review-request event, so this wait stops here " "rather than polling --timeout out against a request that does not exist." ) + elif held: + final = snapshot + print( + "note: this pull request merges into a branch other than the default and Copilot " + "has reviewed it already, so a fix push is covered by an attested local pass rather " + "than another Copilot round, and this wait requests nothing. Pass --request to ask " + "for a round anyway." + ) elif stopped and not reviewer_requested(pr): print( "note: this pull request's newest Copilot review is a refusal naming the account " @@ -4412,7 +4482,11 @@ def main(argv: list[str] | None = None) -> int: # Gating the verdict behind it left the login check unable to reach an exit code. # The digest above printed `shapes=UNRECOGNIZED` the whole time it did so. # Coverage of the head is the other half, returning 0 only once the diff is covered too. - if unrecognized_shapes(final) or head_review_done(final, a.min_rounds): + covered = held and local_cover(final) + halted = None if a.ignore_quota_signal else stopping_refusal(final) + if unrecognized_shapes(final) or ( + not halted and (head_review_done(final, a.min_rounds) or covered) + ): verdict = report_verdict(final, owner, repo) # The check reading ranks under both of those, and never replaces either. # An unreadable shape means no field here can be believed, this one included. @@ -4480,6 +4554,14 @@ def main(argv: list[str] | None = None) -> int: "--ignore-quota-signal once the limit is believed to have reset" ) return 46 + if held and not refusal: + print( + "status=AWAITING_LOCAL_PASS no Copilot round covers this head and none was requested, " + "since a fix push after the first round is covered by a local pass. Run the local " + "strict review, record it, push, and run `pr_review.py attest` from the checkout, " + "or pass --request to ask Copilot for a round instead" + ) + return 49 if refusal: print( "status=REVIEW_IS_A_REFUSAL the review carrying the head says it did not review, " diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index e4eb9d647..9ff776cce 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -5733,6 +5733,90 @@ def test_ignore_quota_signal_requests_past_a_current_head_refusal(self) -> None: self.assertEqual(0, self.cli(["wait", "7", "--ignore-quota-signal"])) self.assertEqual(1, len([c for c in calls if "requestReviews" in c[0]])) + def into(self, pr: dict, base: str = "develop", attest: bool = False) -> dict: + """A pull request into `base` on a repository whose default branch is main.""" + pr = {**pr, "baseRefName": base, "baseRepository": {"defaultBranchRef": {"name": "main"}}} + if attest: + marker = { + "author": {"login": "maintainer"}, + "authorAssociation": "OWNER", + "createdAt": LATE, + "body": f"Attested.\n\n", + } + pr["comments"] = {"nodes": [marker], "pageInfo": {"hasPreviousPage": False}} + return pr + + def test_a_newer_quota_refusal_outranks_older_coverage_of_the_head(self) -> None: + """A re-request answered by the limit is the newest word, whatever the head already had.""" + pr = payload( + [ + review(at=EARLY, rid="PRR_a"), + review(body=QUOTA_REFUSED, at=LATE, rid="PRR_b"), + ] + ) + self.answer(pr) + out, _ = pr_review.digest("o", "r", 7) + self.assertIn("refusal=QUOTA", out) + with mock.patch.object(pr_review.time, "sleep"): + self.assertEqual(46, self.cli(["wait", "7", "--timeout", "0"])) + + def test_an_older_file_count_refusal_still_yields_to_coverage(self) -> None: + pr = payload([review(body=REFUSED, at=EARLY, rid="PRR_a"), review(at=LATE, rid="PRR_b")]) + self.answer(pr) + out, _ = pr_review.digest("o", "r", 7) + self.assertIn("refusal=no", out) + + def test_a_fix_push_after_the_first_round_is_not_requested(self) -> None: + self.answer(self.into(payload([review(oid=OLD)]))) + calls = self.wire_history([hist_review(7, OVERVIEW + "\n" + COVERED)]) + with mock.patch.object(pr_review.time, "sleep") as slept: + self.assertEqual(49, self.cli(["wait", "7", "--timeout", "0"])) + slept.assert_not_called() + self.assertEqual(0, len([c for c in calls if "requestReviews" in c[0]])) + self.assertIn("status=AWAITING_LOCAL_PASS", self.out.getvalue()) + + def test_an_attested_fix_push_closes_the_wait(self) -> None: + self.answer(self.into(payload([review(oid=OLD)]), attest=True)) + calls = self.wire_history([hist_review(7, OVERVIEW + "\n" + COVERED)]) + with mock.patch.object(pr_review.time, "sleep"): + self.assertEqual(0, self.cli(["wait", "7", "--timeout", "0"])) + self.assertEqual(0, len([c for c in calls if "requestReviews" in c[0]])) + out = self.out.getvalue() + self.assertIn("review_on_head=local", out) + self.assertIn("coverage=local", out) + + def test_request_asks_for_a_round_on_a_fix_push(self) -> None: + self.answer(self.into(payload([review(oid=OLD)]))) + calls = self.wire_history([hist_review(7, OVERVIEW + "\n" + COVERED)]) + with mock.patch.object(pr_review.time, "sleep"): + self.cli(["wait", "7", "--timeout", "0", "--request"]) + self.assertEqual(1, len([c for c in calls if "requestReviews" in c[0]])) + + def test_a_promotion_and_a_first_round_are_still_requested(self) -> None: + for pr in ( + self.into(payload([review(oid=OLD)]), base="main", attest=True), + self.into(payload([])), + ): + with self.subTest(base=pr["baseRefName"]): + self.out.seek(0) + self.out.truncate() + self.answer(pr) + calls = self.wire_history([hist_review(7, OVERVIEW + "\n" + COVERED)]) + with mock.patch.object(pr_review.time, "sleep"): + self.cli(["wait", "7", "--timeout", "0"]) + self.assertEqual(1, len([c for c in calls if "requestReviews" in c[0]])) + + def test_status_reads_an_attested_head_unless_a_partial_is_on_record(self) -> None: + part = OVERVIEW + "\n" + for rounds, field in ( + ([review(oid=OLD)], "review_on_head=local"), + ([review(oid=OLD, body=part)], "review_on_head=NO"), + ): + with self.subTest(field=field): + self.answer(self.into(payload(rounds), attest=True)) + out, _ = pr_review.digest("o", "r", 7) + self.assertIn(field, out) + def test_ignore_quota_signal_requests_past_the_repo_wide_signal(self) -> None: self.answer(payload([])) calls = self.wire_history([hist_review(962, QUOTA_REFUSED)]) From 963ff963d0f6aad1c94527e458eeb2fe9c87919a Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Thu, 1 Oct 2026 21:10:19 -0700 Subject: [PATCH 3/8] Close the Defined-Moments Review Gaps An attested head is covered whatever a stale refusal or an old plain answer says, a partial on record or a truncated history requests a round instead of holding, the marker counts only as a line of its own outside a fence, and attest checks the merge base and an unconfigured clean tree. The docs name a pull request into the default branch rather than a promotion alone, and drop the per-push wording left beside the new rule. Co-Authored-By: Claude Opus 5.5 --- .agents/skills/pr-review-conduct/SKILL.md | 64 +++++++-------- .../.source-digests/pr-review-conduct | 2 +- .../skills/pr-review-conduct/SKILL.md | 64 +++++++-------- .github/copilot-instructions.md | 4 +- .github/skills/pr-review-conduct/SKILL.md | 64 +++++++-------- GOVERNANCE.md | 2 +- docs/pr-reviewer-reference.md | 2 +- repo-config/README.md | 2 +- scripts/README.md | 2 +- scripts/pr_review.py | 59 +++++++++----- tests/test_pr_review.py | 78 ++++++++++++++++++- 11 files changed, 222 insertions(+), 121 deletions(-) diff --git a/.agents/skills/pr-review-conduct/SKILL.md b/.agents/skills/pr-review-conduct/SKILL.md index 1ee7e9e2e..5ebb0e617 100644 --- a/.agents/skills/pr-review-conduct/SKILL.md +++ b/.agents/skills/pr-review-conduct/SKILL.md @@ -1,9 +1,9 @@ --- name: pr-review-conduct description: >- - Governs opening, driving, and merging a pull request review loop in a ptr727/ProjectTemplate - fleet repo: requesting a review after a push, triaging findings, replying and resolving threads, - and deciding whether a PR is actually mergeable. Use this whenever about to open a PR, + Governs opening, driving, and merging a pull request review loop in a ptr727/ProjectTemplate fleet + repo: requesting a review at its defined moments, triaging findings, replying and resolving + threads, and deciding whether a PR is actually mergeable. Use this whenever about to open a PR, immediately after creating one, about to merge a PR, enable auto-merge, ask the maintainer for merge permission, push a fix and move on without re-checking review state, or judge a PR "green" or "clean" from CI or mergeStateStatus alone. Triggers even when the request sounds routine, @@ -37,13 +37,15 @@ visible comments, routinely still carries a finding nobody has answered. Treatin 1. Required status checks are green, and where they are not, the reason is **read**, never inferred. `BLOCKED` covers a failed check, a required check nothing is running, an unresolved thread, and a missing approval alike, and the response differs by cause. -2. A review is confirmed on the **current head SHA**. On a pull request into a branch other than - the default, a head after Copilot's first round can carry an attested local pass instead, - `review_on_head=local` in the digest, where no round on record states or appears to state - partial coverage, and a promotion's head always carries a Copilot round of its own. A review - is matched by commit SHA rather than assumed from a green merge-state. A push makes checks go green *before* the re-review lands, and the - matched review is **read**, not just counted. A review can carry the head SHA and still decline - the PR outright, or say it read only part of the changed files. Where the round covering the +2. A review is confirmed on the **current head SHA**. On a pull request into a branch other than the + default, a head after Copilot's first round can carry an attested local pass instead, + `review_on_head=local` in the digest, where the whole review history is in view and no round on + record states or appears to state partial coverage, and a pull request into the default branch, a + promotion among them, always carries a Copilot round on its head. A review is matched by commit + SHA rather than assumed from a green merge-state. A push makes checks go green *before* the + re-review lands, and the matched review is **read**, not just counted. A review can carry the + head SHA and still decline the PR outright, or say it read only part of the changed files. Where + the round covering the head states no coverage at all, the newest round that does state some stands in for it, and only where the pull request changes the same set of files at both commits, since a statement about a diff this head no longer has says nothing about this one. A head round's own @@ -55,12 +57,11 @@ visible comments, routinely still carries a finding nobody has answered. Treatin carries a table of its own, the newest round that does, its table naming exactly the changed files, stands in under the bound a statement carries under, the pull request changing the same set of files at both commits, and that reading shows as `coverage=carried:table`. The table - stands in only where no - Copilot round on the pull request, on any commit, states or appears to state partial - coverage, so a pull request that ever had a partial round goes to the maintainer. The - coverage this item - requires is Copilot's, and CodeRabbit and Qodo are advisory, since the hub's - `docs/pr-reviewer-evaluation.md` "Status" names Copilot the incumbent and says no candidate is + stands in only where no Copilot round on the pull request, on any commit, states or appears to + state partial coverage, so a pull request that ever had a partial round goes to the maintainer. + The coverage this item requires is Copilot's, or the attested local pass above, and CodeRabbit + and Qodo are advisory, since the hub's `docs/pr-reviewer-evaluation.md` "Status" names Copilot + the incumbent and says no candidate is a required reviewer: an advisory reviewer's absence blocks nothing, while its findings owe item 3 exactly as Copilot's do. `pr_review.py`'s `review_on_head` names Copilot's own coverage specifically, not "no review of any kind covers this head": an advisory reviewer carrying the @@ -147,20 +148,22 @@ must have done. review command, such as `@coderabbitai review`, and wait for the result as with any other requested review. The agent driving the loop posts that comment itself, on the same standing as requesting a Copilot round, and at most once per pull request, on the head the drive judges - final, since the reviewer caps its reviews per pull request and its absence blocks nothing. + final, and never after a rate-limit notice, since the reviewer caps its reviews per pull request + and its absence blocks nothing. - **A notice naming when the reviewer can next run is a rate limit, and asking does not clear it.** It reads like the skip notice above and is the opposite case: the trigger returns the same notice rather than a review, so a loop that keeps asking waits on something no amount of asking - produces. Wait for the time it names, or proceed on the reviewers that did run, since an advisory - reviewer blocks nothing. + produces. Do not ask again on that pull request, and proceed on the reviewers that did run, + since an advisory reviewer blocks nothing. - **Silence is not evidence, and is never read as one on its own.** A reviewer that has posted nothing may not have started yet, may not cover this repository at all, or may have reviewed and had nothing to say, which Merge Gate item 2 describes as its own ordinary shape and which posts no comment to read. Read the reviews themselves rather than the comments alone, since the third case appears only there. - **Copilot's absence blocks, and is answered elsewhere.** Merge Gate item 2 requires Copilot's own - coverage of the current head, and the loop's own re-request step below is where a missing one is - answered, on the terms stated there. A refusal naming the account quota is its own case rather + coverage of the current head, or on a fix push into a branch other than the default an attested + local pass, and the loop's own request step below is where a missing one is answered, on the + terms stated there. A refusal naming the account quota is its own case rather than a review, and so is one saying only that Copilot encountered an error, which is what the weekly rate limit posts. Either covers no head, so the gate stays unsatisfied, and nothing the loop does clears it, since re-requesting returns it again and spends quota doing so. Where the @@ -189,18 +192,17 @@ Run `local-strict-review` against the branch's current diff before every push th 1. Push changes to the PR branch and open the pull request when it does not exist. 2. Run `scripts/pr_review.py status --repo /` once in the foreground and read its output. -3. Request a Copilot round at the defined moments only: the pull request's first round, and - the head of a promotion into the default branch. Auto-trigger is unreliable, so request it - explicitly, which step 4's `wait` is what does. On a fix push into any other branch after the - first round, `wait` requests nothing, since that push is covered by the pass the paragraph - above records. Publish it once the push lands by running `scripts/pr_review.py attest +3. Request a Copilot round at the defined moments only: the pull request's first round, and the head + of a pull request into the default branch, a promotion among them. Auto-trigger is unreliable, so + request it explicitly, which step 4's `wait` is what does. On a fix push into any other branch + after the first round, `wait` requests nothing, since that push is covered by the pass the + paragraph above records. Publish it once the push lands by running `scripts/pr_review.py attest --repo / --checkout `, which refuses unless the checkout is the pushed head and a recorded pass covers it, and pass `wait --request` where a fix deserves a - Copilot round anyway. `wait` also skips the request where a review already covers the head, - where the answer came outside a formal review, where it detects drift, under a quota or error - refusal, and where something is already in the request set, which is the condition the - recovery below clears. Requesting in the pull request UI is the maintainer's route rather than - this loop's. + Copilot round anyway. `wait` also skips the request where a review already covers the head, where + the answer came outside a formal review, where it detects drift, under a quota or error refusal, + and where something is already in the request set, which is the condition the recovery below + clears. Requesting in the pull request UI is the maintainer's route rather than this loop's. 4. Run a bounded `scripts/pr_review.py wait --repo /` in a background process and read its terminal output. A completed review raising **no findings** is a valid terminal outcome, so do not re-trigger it or read silence as a missing review. A review whose body says it declined to review is the one diff --git a/.claude-plugin/fleet-skills/.source-digests/pr-review-conduct b/.claude-plugin/fleet-skills/.source-digests/pr-review-conduct index 0accc60cf..aed1424f4 100644 --- a/.claude-plugin/fleet-skills/.source-digests/pr-review-conduct +++ b/.claude-plugin/fleet-skills/.source-digests/pr-review-conduct @@ -1 +1 @@ -9bee557e230ab52f +115e04e054ba1096 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 1ee7e9e2e..5ebb0e617 100644 --- a/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md @@ -1,9 +1,9 @@ --- name: pr-review-conduct description: >- - Governs opening, driving, and merging a pull request review loop in a ptr727/ProjectTemplate - fleet repo: requesting a review after a push, triaging findings, replying and resolving threads, - and deciding whether a PR is actually mergeable. Use this whenever about to open a PR, + Governs opening, driving, and merging a pull request review loop in a ptr727/ProjectTemplate fleet + repo: requesting a review at its defined moments, triaging findings, replying and resolving + threads, and deciding whether a PR is actually mergeable. Use this whenever about to open a PR, immediately after creating one, about to merge a PR, enable auto-merge, ask the maintainer for merge permission, push a fix and move on without re-checking review state, or judge a PR "green" or "clean" from CI or mergeStateStatus alone. Triggers even when the request sounds routine, @@ -37,13 +37,15 @@ visible comments, routinely still carries a finding nobody has answered. Treatin 1. Required status checks are green, and where they are not, the reason is **read**, never inferred. `BLOCKED` covers a failed check, a required check nothing is running, an unresolved thread, and a missing approval alike, and the response differs by cause. -2. A review is confirmed on the **current head SHA**. On a pull request into a branch other than - the default, a head after Copilot's first round can carry an attested local pass instead, - `review_on_head=local` in the digest, where no round on record states or appears to state - partial coverage, and a promotion's head always carries a Copilot round of its own. A review - is matched by commit SHA rather than assumed from a green merge-state. A push makes checks go green *before* the re-review lands, and the - matched review is **read**, not just counted. A review can carry the head SHA and still decline - the PR outright, or say it read only part of the changed files. Where the round covering the +2. A review is confirmed on the **current head SHA**. On a pull request into a branch other than the + default, a head after Copilot's first round can carry an attested local pass instead, + `review_on_head=local` in the digest, where the whole review history is in view and no round on + record states or appears to state partial coverage, and a pull request into the default branch, a + promotion among them, always carries a Copilot round on its head. A review is matched by commit + SHA rather than assumed from a green merge-state. A push makes checks go green *before* the + re-review lands, and the matched review is **read**, not just counted. A review can carry the + head SHA and still decline the PR outright, or say it read only part of the changed files. Where + the round covering the head states no coverage at all, the newest round that does state some stands in for it, and only where the pull request changes the same set of files at both commits, since a statement about a diff this head no longer has says nothing about this one. A head round's own @@ -55,12 +57,11 @@ visible comments, routinely still carries a finding nobody has answered. Treatin carries a table of its own, the newest round that does, its table naming exactly the changed files, stands in under the bound a statement carries under, the pull request changing the same set of files at both commits, and that reading shows as `coverage=carried:table`. The table - stands in only where no - Copilot round on the pull request, on any commit, states or appears to state partial - coverage, so a pull request that ever had a partial round goes to the maintainer. The - coverage this item - requires is Copilot's, and CodeRabbit and Qodo are advisory, since the hub's - `docs/pr-reviewer-evaluation.md` "Status" names Copilot the incumbent and says no candidate is + stands in only where no Copilot round on the pull request, on any commit, states or appears to + state partial coverage, so a pull request that ever had a partial round goes to the maintainer. + The coverage this item requires is Copilot's, or the attested local pass above, and CodeRabbit + and Qodo are advisory, since the hub's `docs/pr-reviewer-evaluation.md` "Status" names Copilot + the incumbent and says no candidate is a required reviewer: an advisory reviewer's absence blocks nothing, while its findings owe item 3 exactly as Copilot's do. `pr_review.py`'s `review_on_head` names Copilot's own coverage specifically, not "no review of any kind covers this head": an advisory reviewer carrying the @@ -147,20 +148,22 @@ must have done. review command, such as `@coderabbitai review`, and wait for the result as with any other requested review. The agent driving the loop posts that comment itself, on the same standing as requesting a Copilot round, and at most once per pull request, on the head the drive judges - final, since the reviewer caps its reviews per pull request and its absence blocks nothing. + final, and never after a rate-limit notice, since the reviewer caps its reviews per pull request + and its absence blocks nothing. - **A notice naming when the reviewer can next run is a rate limit, and asking does not clear it.** It reads like the skip notice above and is the opposite case: the trigger returns the same notice rather than a review, so a loop that keeps asking waits on something no amount of asking - produces. Wait for the time it names, or proceed on the reviewers that did run, since an advisory - reviewer blocks nothing. + produces. Do not ask again on that pull request, and proceed on the reviewers that did run, + since an advisory reviewer blocks nothing. - **Silence is not evidence, and is never read as one on its own.** A reviewer that has posted nothing may not have started yet, may not cover this repository at all, or may have reviewed and had nothing to say, which Merge Gate item 2 describes as its own ordinary shape and which posts no comment to read. Read the reviews themselves rather than the comments alone, since the third case appears only there. - **Copilot's absence blocks, and is answered elsewhere.** Merge Gate item 2 requires Copilot's own - coverage of the current head, and the loop's own re-request step below is where a missing one is - answered, on the terms stated there. A refusal naming the account quota is its own case rather + coverage of the current head, or on a fix push into a branch other than the default an attested + local pass, and the loop's own request step below is where a missing one is answered, on the + terms stated there. A refusal naming the account quota is its own case rather than a review, and so is one saying only that Copilot encountered an error, which is what the weekly rate limit posts. Either covers no head, so the gate stays unsatisfied, and nothing the loop does clears it, since re-requesting returns it again and spends quota doing so. Where the @@ -189,18 +192,17 @@ Run `local-strict-review` against the branch's current diff before every push th 1. Push changes to the PR branch and open the pull request when it does not exist. 2. Run `scripts/pr_review.py status --repo /` once in the foreground and read its output. -3. Request a Copilot round at the defined moments only: the pull request's first round, and - the head of a promotion into the default branch. Auto-trigger is unreliable, so request it - explicitly, which step 4's `wait` is what does. On a fix push into any other branch after the - first round, `wait` requests nothing, since that push is covered by the pass the paragraph - above records. Publish it once the push lands by running `scripts/pr_review.py attest +3. Request a Copilot round at the defined moments only: the pull request's first round, and the head + of a pull request into the default branch, a promotion among them. Auto-trigger is unreliable, so + request it explicitly, which step 4's `wait` is what does. On a fix push into any other branch + after the first round, `wait` requests nothing, since that push is covered by the pass the + paragraph above records. Publish it once the push lands by running `scripts/pr_review.py attest --repo / --checkout `, which refuses unless the checkout is the pushed head and a recorded pass covers it, and pass `wait --request` where a fix deserves a - Copilot round anyway. `wait` also skips the request where a review already covers the head, - where the answer came outside a formal review, where it detects drift, under a quota or error - refusal, and where something is already in the request set, which is the condition the - recovery below clears. Requesting in the pull request UI is the maintainer's route rather than - this loop's. + Copilot round anyway. `wait` also skips the request where a review already covers the head, where + the answer came outside a formal review, where it detects drift, under a quota or error refusal, + and where something is already in the request set, which is the condition the recovery below + clears. Requesting in the pull request UI is the maintainer's route rather than this loop's. 4. Run a bounded `scripts/pr_review.py wait --repo /` in a background process and read its terminal output. A completed review raising **no findings** is a valid terminal outcome, so do not re-trigger it or read silence as a missing review. A review whose body says it declined to review is the one diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index a32801db2..fb2402f9f 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -26,9 +26,9 @@ For every review: 4. Use an inline comment when a changed line can anchor the finding. Use the review body only when no valid inline anchor exists. 5. End the review body with the exact machine-readable marker required by the `fleet-code-review` skill. -The review automation is `scripts/pr_review.py`, run from a hub checkout. Use its `status`, `wait`, `comment`, and `reply --resolve` commands instead of reconstructing GraphQL queries or copying review identifiers by hand. Use `comment` for a suppressed-finding answer in the pull request conversation. Its status gate verifies the current head, diff coverage, output shape, inline threads, body-only findings, and required checks. +The review automation is `scripts/pr_review.py`, run from a hub checkout. Use its `status`, `wait`, `attest`, `comment`, and `reply --resolve` commands instead of reconstructing GraphQL queries or copying review identifiers by hand. Use `comment` for a suppressed-finding answer in the pull request conversation. Its status gate verifies the current head, diff coverage, output shape, inline threads, body-only findings, and required checks. -A formal review with no findings is complete only when it covers the current head and full diff coverage of the change set that head has is stated, or read from a file table as below. The round covering the head states it, or the newest round that states it at all does and the pull request changes the same set of files at both commits, which is the only condition under which a statement carries forward. Only that newest round is consulted, so an older round whose change set does match carries nothing. Where no statement reaches the head either way, a round covering it whose own file table names exactly the changed files covers it, which is the reading a round at Balanced review effort gives, and a statement that reaches the head, stated on it or carried to it, wins over that table. Where no round covering the head carries a table of its own, the newest round that does, its table naming exactly the changed files, stands in under the bound a statement carries under, the pull request changing the same set of files at both commits. The table stands in only where no Copilot round on the pull request, on any commit, states or appears to state partial coverage, so a pull request that ever had a partial round goes to the maintainer. A round reporting partial coverage of the diff blocks the merge, and so does a refusal, a coverage statement that does not reach this head, meaning absent from every round or carried by none because the change set moved or could not be compared, with no file table standing in for it, an unrecognized output shape, an unresolved thread, or a body-only finding. On a pull request into a branch other than the default, a fix push after Copilot's first round is covered by an attested local pass rather than another Copilot round, `review_on_head=local` in the digest, while a promotion's head always carries a Copilot round of its own. Re-run the loop after every fix push. Never infer review completion from `mergeStateStatus: CLEAN`. +A formal review with no findings is complete only when it covers the current head and full diff coverage of the change set that head has is stated, or read from a file table as below. The round covering the head states it, or the newest round that states it at all does and the pull request changes the same set of files at both commits, which is the only condition under which a statement carries forward. Only that newest round is consulted, so an older round whose change set does match carries nothing. Where no statement reaches the head either way, a round covering it whose own file table names exactly the changed files covers it, which is the reading a round at Balanced review effort gives, and a statement that reaches the head, stated on it or carried to it, wins over that table. Where no round covering the head carries a table of its own, the newest round that does, its table naming exactly the changed files, stands in under the bound a statement carries under, the pull request changing the same set of files at both commits. The table stands in only where no Copilot round on the pull request, on any commit, states or appears to state partial coverage, so a pull request that ever had a partial round goes to the maintainer. A round reporting partial coverage of the diff blocks the merge, and so does a refusal, a coverage statement that does not reach this head, meaning absent from every round or carried by none because the change set moved or could not be compared, with no file table standing in for it, an unrecognized output shape, an unresolved thread, or a body-only finding. On a pull request into a branch other than the default, a fix push after Copilot's first round is covered by an attested local pass rather than another Copilot round, `review_on_head=local` in the digest, while a pull request into the default branch, a promotion among them, always carries a Copilot round on its head. Re-run the loop after every fix push. Never infer review completion from `mergeStateStatus: CLEAN`. Review effort is user-controlled. The automation observes `Lite`, `Balanced`, or `Max`, including an inherited `Default ()`, and never selects or changes the setting. Effort does not determine coverage or completion. A request can complete without a `copilot_work_started` event, so absence of that event is not a stalled-review verdict. When `wait` returns `PENDING` with `requested=yes`, report the state and rerun `wait` for another bounded interval by default, reading that field as acceptance of the request rather than as delivery of a round. Do not clear the request on that first timeout, because it may still be active. Where a second bounded wait times out as well, read the pending set, clear it only where no human or team reviewer is requested alongside the bot, and rerun `wait`, which then has nothing outstanding to defer to and requests afresh, or polls and says so on its own auto-request line where it finds no reviewer node id to request with. The clear replaces that set rather than adding to it and nothing restores a request it drops, so a stall on a pull request that has a human or team reviewer requested goes to the maintainer, and so does one still pending after the wait that follows a clear. That clear is a recovery step the script does not implement, and `docs/pr-reviewer-reference.md`, in the hub checkout the script is run from, carries it. This recovery replaces only the review request and never changes the effort setting. A `wait` ending `REQUEST_NOT_RECORDED`, exit 48, is a different state and takes none of this recovery: the request returned success and left neither a pending reviewer nor a review-request event, which is how an exhausted Copilot allowance has shown itself, so clearing and requesting again does not clear it and it goes to the maintainer. diff --git a/.github/skills/pr-review-conduct/SKILL.md b/.github/skills/pr-review-conduct/SKILL.md index 1ee7e9e2e..5ebb0e617 100644 --- a/.github/skills/pr-review-conduct/SKILL.md +++ b/.github/skills/pr-review-conduct/SKILL.md @@ -1,9 +1,9 @@ --- name: pr-review-conduct description: >- - Governs opening, driving, and merging a pull request review loop in a ptr727/ProjectTemplate - fleet repo: requesting a review after a push, triaging findings, replying and resolving threads, - and deciding whether a PR is actually mergeable. Use this whenever about to open a PR, + Governs opening, driving, and merging a pull request review loop in a ptr727/ProjectTemplate fleet + repo: requesting a review at its defined moments, triaging findings, replying and resolving + threads, and deciding whether a PR is actually mergeable. Use this whenever about to open a PR, immediately after creating one, about to merge a PR, enable auto-merge, ask the maintainer for merge permission, push a fix and move on without re-checking review state, or judge a PR "green" or "clean" from CI or mergeStateStatus alone. Triggers even when the request sounds routine, @@ -37,13 +37,15 @@ visible comments, routinely still carries a finding nobody has answered. Treatin 1. Required status checks are green, and where they are not, the reason is **read**, never inferred. `BLOCKED` covers a failed check, a required check nothing is running, an unresolved thread, and a missing approval alike, and the response differs by cause. -2. A review is confirmed on the **current head SHA**. On a pull request into a branch other than - the default, a head after Copilot's first round can carry an attested local pass instead, - `review_on_head=local` in the digest, where no round on record states or appears to state - partial coverage, and a promotion's head always carries a Copilot round of its own. A review - is matched by commit SHA rather than assumed from a green merge-state. A push makes checks go green *before* the re-review lands, and the - matched review is **read**, not just counted. A review can carry the head SHA and still decline - the PR outright, or say it read only part of the changed files. Where the round covering the +2. A review is confirmed on the **current head SHA**. On a pull request into a branch other than the + default, a head after Copilot's first round can carry an attested local pass instead, + `review_on_head=local` in the digest, where the whole review history is in view and no round on + record states or appears to state partial coverage, and a pull request into the default branch, a + promotion among them, always carries a Copilot round on its head. A review is matched by commit + SHA rather than assumed from a green merge-state. A push makes checks go green *before* the + re-review lands, and the matched review is **read**, not just counted. A review can carry the + head SHA and still decline the PR outright, or say it read only part of the changed files. Where + the round covering the head states no coverage at all, the newest round that does state some stands in for it, and only where the pull request changes the same set of files at both commits, since a statement about a diff this head no longer has says nothing about this one. A head round's own @@ -55,12 +57,11 @@ visible comments, routinely still carries a finding nobody has answered. Treatin carries a table of its own, the newest round that does, its table naming exactly the changed files, stands in under the bound a statement carries under, the pull request changing the same set of files at both commits, and that reading shows as `coverage=carried:table`. The table - stands in only where no - Copilot round on the pull request, on any commit, states or appears to state partial - coverage, so a pull request that ever had a partial round goes to the maintainer. The - coverage this item - requires is Copilot's, and CodeRabbit and Qodo are advisory, since the hub's - `docs/pr-reviewer-evaluation.md` "Status" names Copilot the incumbent and says no candidate is + stands in only where no Copilot round on the pull request, on any commit, states or appears to + state partial coverage, so a pull request that ever had a partial round goes to the maintainer. + The coverage this item requires is Copilot's, or the attested local pass above, and CodeRabbit + and Qodo are advisory, since the hub's `docs/pr-reviewer-evaluation.md` "Status" names Copilot + the incumbent and says no candidate is a required reviewer: an advisory reviewer's absence blocks nothing, while its findings owe item 3 exactly as Copilot's do. `pr_review.py`'s `review_on_head` names Copilot's own coverage specifically, not "no review of any kind covers this head": an advisory reviewer carrying the @@ -147,20 +148,22 @@ must have done. review command, such as `@coderabbitai review`, and wait for the result as with any other requested review. The agent driving the loop posts that comment itself, on the same standing as requesting a Copilot round, and at most once per pull request, on the head the drive judges - final, since the reviewer caps its reviews per pull request and its absence blocks nothing. + final, and never after a rate-limit notice, since the reviewer caps its reviews per pull request + and its absence blocks nothing. - **A notice naming when the reviewer can next run is a rate limit, and asking does not clear it.** It reads like the skip notice above and is the opposite case: the trigger returns the same notice rather than a review, so a loop that keeps asking waits on something no amount of asking - produces. Wait for the time it names, or proceed on the reviewers that did run, since an advisory - reviewer blocks nothing. + produces. Do not ask again on that pull request, and proceed on the reviewers that did run, + since an advisory reviewer blocks nothing. - **Silence is not evidence, and is never read as one on its own.** A reviewer that has posted nothing may not have started yet, may not cover this repository at all, or may have reviewed and had nothing to say, which Merge Gate item 2 describes as its own ordinary shape and which posts no comment to read. Read the reviews themselves rather than the comments alone, since the third case appears only there. - **Copilot's absence blocks, and is answered elsewhere.** Merge Gate item 2 requires Copilot's own - coverage of the current head, and the loop's own re-request step below is where a missing one is - answered, on the terms stated there. A refusal naming the account quota is its own case rather + coverage of the current head, or on a fix push into a branch other than the default an attested + local pass, and the loop's own request step below is where a missing one is answered, on the + terms stated there. A refusal naming the account quota is its own case rather than a review, and so is one saying only that Copilot encountered an error, which is what the weekly rate limit posts. Either covers no head, so the gate stays unsatisfied, and nothing the loop does clears it, since re-requesting returns it again and spends quota doing so. Where the @@ -189,18 +192,17 @@ Run `local-strict-review` against the branch's current diff before every push th 1. Push changes to the PR branch and open the pull request when it does not exist. 2. Run `scripts/pr_review.py status --repo /` once in the foreground and read its output. -3. Request a Copilot round at the defined moments only: the pull request's first round, and - the head of a promotion into the default branch. Auto-trigger is unreliable, so request it - explicitly, which step 4's `wait` is what does. On a fix push into any other branch after the - first round, `wait` requests nothing, since that push is covered by the pass the paragraph - above records. Publish it once the push lands by running `scripts/pr_review.py attest +3. Request a Copilot round at the defined moments only: the pull request's first round, and the head + of a pull request into the default branch, a promotion among them. Auto-trigger is unreliable, so + request it explicitly, which step 4's `wait` is what does. On a fix push into any other branch + after the first round, `wait` requests nothing, since that push is covered by the pass the + paragraph above records. Publish it once the push lands by running `scripts/pr_review.py attest --repo / --checkout `, which refuses unless the checkout is the pushed head and a recorded pass covers it, and pass `wait --request` where a fix deserves a - Copilot round anyway. `wait` also skips the request where a review already covers the head, - where the answer came outside a formal review, where it detects drift, under a quota or error - refusal, and where something is already in the request set, which is the condition the - recovery below clears. Requesting in the pull request UI is the maintainer's route rather than - this loop's. + Copilot round anyway. `wait` also skips the request where a review already covers the head, where + the answer came outside a formal review, where it detects drift, under a quota or error refusal, + and where something is already in the request set, which is the condition the recovery below + clears. Requesting in the pull request UI is the maintainer's route rather than this loop's. 4. Run a bounded `scripts/pr_review.py wait --repo /` in a background process and read its terminal output. A completed review raising **no findings** is a valid terminal outcome, so do not re-trigger it or read silence as a missing review. A review whose body says it declined to review is the one diff --git a/GOVERNANCE.md b/GOVERNANCE.md index 824cb99d0..50714e9d7 100644 --- a/GOVERNANCE.md +++ b/GOVERNANCE.md @@ -212,7 +212,7 @@ The checks that separate work actually done from work that merely reports succes ## PR Review Etiquette -The provider-agnostic review-loop contract every fleet repo follows starts when a pull request opens. Open every fleet-owned pull request ready for review. Draft state is reserved for the separately documented upstream contribution workflow while a third-party contribution is still being prepared. Creating the pull request is not a terminal handoff. Run the review status once in the foreground. Then start the bounded review wait in a background process. Request a review at defined moments rather than on every push: when the pull request opens, and on the head of a promotion into the default branch. On a pull request into any other branch, a fix push after the first round is covered instead by the recorded local pass the push already owes, published to the pull request so the review status can read it. Confirm that a review, or for such a fix push that published pass, covers the current head SHA, and that a review covers the full diff rather than only part of it. Where the round covering the head states no coverage at all, read the newest round that does state some as covering this head only where the pull request changes the same set of files at both commits, a head round's own statement always winning over a carried one. Where no statement reaches the head either way, a round covering it whose own file table names exactly the changed files covers it, which is the reading a Copilot round at Balanced review effort gives, and a statement that reaches the head, stated on it or carried to it, wins over that table. Where no round covering the head carries a table of its own, the newest round that does, its table naming exactly the changed files, stands in under the bound a statement carries under, the pull request changing the same set of files at both commits. The table stands in only where no Copilot round on the pull request, on any commit, states or appears to state partial coverage, so a pull request that ever had a partial round goes to the maintainer. Triage every finding, including low-confidence findings collapsed into the review body rather than threads. Reply to and resolve every addressed finding. Repeat after every fix until the checks are green, the current head is covered, and no finding is left open. Only an explicit maintainer instruction may stop, defer, or alter this default. Silence or a request that says only "open a PR" is not such an instruction. Never merge on a green or CLEAN merge state alone. That state does not prove the review covered the current head SHA and full diff. It also does not expose unanswered low-confidence findings that opened no thread. +The provider-agnostic review-loop contract every fleet repo follows starts when a pull request opens. Open every fleet-owned pull request ready for review. Draft state is reserved for the separately documented upstream contribution workflow while a third-party contribution is still being prepared. Creating the pull request is not a terminal handoff. Run the review status once in the foreground. Then start the bounded review wait in a background process. Request a review at defined moments rather than on every push: when the pull request opens, and on the head of a pull request into the default branch, a promotion among them. On a pull request into any other branch, a fix push after the first round is covered instead by the recorded local pass the push already owes, published to the pull request so the review status can read it. Confirm that a review, or for such a fix push that published pass, covers the current head SHA, and that a review covers the full diff rather than only part of it. Where the round covering the head states no coverage at all, read the newest round that does state some as covering this head only where the pull request changes the same set of files at both commits, a head round's own statement always winning over a carried one. Where no statement reaches the head either way, a round covering it whose own file table names exactly the changed files covers it, which is the reading a Copilot round at Balanced review effort gives, and a statement that reaches the head, stated on it or carried to it, wins over that table. Where no round covering the head carries a table of its own, the newest round that does, its table naming exactly the changed files, stands in under the bound a statement carries under, the pull request changing the same set of files at both commits. The table stands in only where no Copilot round on the pull request, on any commit, states or appears to state partial coverage, so a pull request that ever had a partial round goes to the maintainer. Triage every finding, including low-confidence findings collapsed into the review body rather than threads. Reply to and resolve every addressed finding. Repeat after every fix until the checks are green, the current head is covered, and no finding is left open. Only an explicit maintainer instruction may stop, defer, or alter this default. Silence or a request that says only "open a PR" is not such an instruction. Never merge on a green or CLEAN merge state alone. That state does not prove the review covered the current head SHA and full diff. It also does not expose unanswered low-confidence findings that opened no thread. This is packaged as the `pr-review-conduct` Skill at `.agents/skills/pr-review-conduct/SKILL.md` in the hub, not a repo-relative link since that path is hub-local and not carried into every fleet repo. The summary above sketches the contract. Read the skill for the merge gate, the expected loop, and how a finding is closed. diff --git a/docs/pr-reviewer-reference.md b/docs/pr-reviewer-reference.md index 57afc9ba7..f07e2d19a 100644 --- a/docs/pr-reviewer-reference.md +++ b/docs/pr-reviewer-reference.md @@ -24,7 +24,7 @@ The consequence a review loop actually needs: **a private repository has Copilot The repository already has first-class status, wait, comment, reply, resolution, coverage, and output-shape handling in `scripts/pr_review.py`. -A review is requested by that script rather than by hand. `wait` requests one on the current head where nothing it reads already settles the round and nothing is outstanding, so an accepted request that is never picked up is a state it cannot clear for itself. Clearing the request set is what leaves the next `wait` nothing to defer to. A `wait` ending `REQUEST_NOT_RECORDED`, exit 48, is a different state and takes none of this recovery: the request returned success and left neither a pending reviewer nor a review-request event, which is how an exhausted Copilot allowance has shown itself, so clearing and requesting again does not clear it and it goes to the maintainer. Nothing in the commands below is the script, so none of its scope refusals reaches them, and the owner in the target is checked by whoever runs them: +A review is requested by that script rather than by hand. `wait` requests one on the current head where nothing it reads already settles the round, nothing is outstanding, and the head is not a fix push an attested local pass covers, so an accepted request that is never picked up is a state it cannot clear for itself. Clearing the request set is what leaves the next `wait` nothing to defer to. A `wait` ending `REQUEST_NOT_RECORDED`, exit 48, is a different state and takes none of this recovery: the request returned success and left neither a pending reviewer nor a review-request event, which is how an exhausted Copilot allowance has shown itself, so clearing and requesting again does not clear it and it goes to the maintainer. Nothing in the commands below is the script, so none of its scope refusals reaches them, and the owner in the target is checked by whoever runs them: ```sh PR_NODE=$(gh pr view "" --repo "/" --json id --jq '.id') diff --git a/repo-config/README.md b/repo-config/README.md index 0cbb9617a..54253ca5d 100644 --- a/repo-config/README.md +++ b/repo-config/README.md @@ -12,7 +12,7 @@ Hub-only repository and branch configuration held as committed files, kept out o Two workflow models share `main.json` but differ on `develop` (registry `workflowModel`, default `release`): - **`release`** (`develop.json`): `develop` requires squash merges with linear history and a PR, the feature-branch pipeline. -- **`operational`** (`operational/develop.json`): `develop` takes **direct signed pushes**, carrying only `deletion`, `non_fast_forward`, and `required_signatures`; no PR, no status-check, no Copilot review rule. CI runs on the push as advisory feedback. Read the dropped rules as an allowance rather than a prohibition, since a PR into `develop` remains legal and the lint workflow triggers on it, with its result reported and not required (a required check here would gate the direct push as well). This is for live-service config repos that edit `develop` directly and promote a known-good snapshot to `main` via an occasional PR (see [GOVERNANCE.md "Branching Model"][governance-branching-model]). +- **`operational`** (`operational/develop.json`): `develop` takes **direct signed pushes**, carrying only `deletion`, `non_fast_forward`, and `required_signatures`, with no PR, no status-check, and no Copilot review rule. CI runs on the push as advisory feedback. Read the dropped rules as an allowance rather than a prohibition, since a PR into `develop` remains legal and the lint workflow triggers on it, with its result reported and not required (a required check here would gate the direct push as well). This is for live-service config repos that edit `develop` directly and promote a known-good snapshot to `main` via an occasional PR (see [GOVERNANCE.md "Branching Model"][governance-branching-model]). `main` (both models) requires merge-commit merges (no linear-history rule), signed commits, a passing `Check pull request workflow status job`, resolved review threads, and Copilot review, and blocks force-pushes and deletion, so a `develop -> main` promotion is always gated even when `develop` takes direct commits. The Copilot review rule in `develop.json` and `main.json` reviews a pull request when it opens and not on each push or while it is a draft, since a later round is requested deliberately per [GOVERNANCE.md "PR Review Etiquette"][governance-pr-review-etiquette] rather than spent on every push. Every ruleset intentionally leaves "Require branches to be up to date before merging" **off**, per [GOVERNANCE.md "Branching Model"][governance-branching-model]. diff --git a/scripts/README.md b/scripts/README.md index 77a63dd2d..1b3f1ccc9 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -175,7 +175,7 @@ python3 scripts/pr_review.py reply 452 --repo ptr727/ProjectTemplate \ python3 scripts/pr_review.py attest 452 --repo ptr727/ProjectTemplate --checkout ../worktree ``` -**A Copilot round is requested at defined moments rather than on every push.** The two moments are a pull request's first round and the head of a promotion into the default branch. On a pull request into any other branch, a fix push after the first round 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 ``, and only after confirming the checkout is the pushed head, holds no other change, and passes `local_review.py check`. `status` then reads that head as `review_on_head=local` with `coverage=local`, where the attestation comes from someone who can write to the repository and no round on record states or appears to state partial coverage. `wait` requests nothing on such a head, ending as covered where it is attested and exiting `49` where it is not, and `--request` asks for a round anyway. The rulesets review a pull request when it opens and not on each push, since a trigger on push would spend a round however the tooling chose. 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 two moments are a pull request's first round and the head of a pull request into the default branch, a promotion among them. On a pull request into any other branch, a fix push after the first round 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 ``, and only after confirming the checkout is the pushed head, holds no other change, and passes `local_review.py check`. `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 a line of its own outside a fence, and no round on record states or appears to state partial coverage with the whole history in view. `wait` requests nothing on such a head, ending as covered where it is attested and exiting `49` where it is not, and `--request` asks for a round anyway. A partial on record or a history past the window requests a round instead, since no attestation can clear either, and a request already pending is polled for as before. `attest` also refuses where the checkout's merge base with the base branch is not the pull request's own. The rulesets review a pull request when it opens and not on each push, since a trigger on push would spend a round however the tooling chose. 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: the digest renders, every field is well-formed, and nothing in the output disagrees. Two runs read this repository's pull requests while their own was the subject, each caught by the maintainer rather than by the run. The digest leads with `repo=OWNER/NAME` for the same reason, since a number alone reads as correct in any repository. A value that is not `OWNER/NAME` is rejected by name rather than raised as an unpacking traceback, that being the near-miss a required argument still admits. diff --git a/scripts/pr_review.py b/scripts/pr_review.py index 8b949abc6..1ae38ae81 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -78,9 +78,10 @@ A refusal naming the account quota still reads as absent here, exit 0, since a refusal covers no head either. Its printed digest line carries `refusal=QUOTA` regardless, and `refusal=ERROR` for an error refusal whose run log names no rate limit or could not be - read. Either field reads a refusal on an earlier head where it is the pull request's - newest Copilot review and nothing covers the head. `wait` is where that state gets its - own exit codes, 46 and 47 below, because only `wait` is the command a caller might + read. Either field reads the pull request's newest Copilot review where it is a quota or + error refusal, on an earlier head or over a head a genuine round covered before it, and a + file-count refusal is spent by coverage of the same head. `wait` is where that state gets + its own exit codes, 46 and 47 below, because only `wait` is the command a caller might otherwise poll out a timeout on. `unresolved` counts every tracked reviewer's own open thread, not only Copilot's: CodeRabbit (`coderabbitai`) and qodo (`qodo-free-for-open-source-projects`) are @@ -199,7 +200,8 @@ (falling back to polling only) where both windows come up empty, since a repository with no Copilot review in either has nothing to read the id from and a fabricated one is never an option. 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, or on a held head an attested local pass, 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. @@ -864,7 +866,7 @@ def strip_fences( query($o:String!,$r:String!,$n:Int!){ repository(owner:$o,name:$r){ pullRequest(number:$n){ headRefOid baseRefName } }} """ -ATTESTATION = re.compile(r"") +ATTESTATION = re.compile(r"^$", re.MULTILINE) TRUSTED_ASSOCIATIONS = frozenset({"OWNER", "MEMBER", "COLLABORATOR"}) LOCAL_REVIEW = Path(__file__).resolve().parent / "local_review.py" @@ -3183,9 +3185,6 @@ def digest( unlisted = unlisted_findings(manifest) answer = answered_outside_review(pr) - # Spent where coverage of the same head landed, the precedence the exit codes already hold. - # Reported regardless, it prints `review_on_head=yes refusal=YES` over a reviewed head. - # That tells a reader to split a pull request the reviewer has just reviewed. refusal = stopping_refusal(pr) or (None if on_head else refusing_review(pr)) # Read once and handed to the line below, since `quota_refusal` re-walks `refusal_of`. refusal_field = ( @@ -3401,8 +3400,9 @@ def digest( elif cover == LOCAL: lines.append( " COVERAGE IS READ FROM THE LOCAL PASS: no Copilot round covers this head, which " - "follows the pull request's first round, and a comment from someone who can write " - "here attests a recorded local strict-review pass over exactly this head's content" + "follows the pull request's first round, and a comment from an owner, member, or " + "collaborator attests a recorded local strict-review pass over exactly this head's " + "content" ) if cover == PARTIAL: # The line prints under the marker for the reason a suppressed block does. @@ -3795,7 +3795,9 @@ def attest(owner: str, repo: str, num: int, checkout: str) -> int: ) return 65 local = _git(checkout, "rev-parse", "HEAD") - dirty = _git(checkout, "status", "--porcelain") + dirty = _git( + checkout, "status", "--porcelain", "--untracked-files=all", "--ignore-submodules=none" + ) if local.returncode != 0 or dirty.returncode != 0: print( f"status=CHECKOUT_NOT_READ nothing was written: {checkout} is not a readable checkout" @@ -3814,6 +3816,18 @@ def attest(owner: str, repo: str, num: int, checkout: str) -> int: "head commit does not, so a pass over it does not describe what was pushed" ) return 67 + ours = _git(checkout, "merge-base", f"origin/{base}", "HEAD") + theirs = gh_rest( + f"repos/{owner}/{repo}/compare/{base}...{head}", ".merge_base_commit.sha // empty" + ) + if ours.returncode != 0 or ours.stdout.strip() != theirs.stdout.strip(): + print( + f"status=CHECKOUT_NOT_THE_HEAD nothing was written: the checkout's merge base with " + f"origin/{base} is not the pull request's own, so a pass against it measured a " + "different change set. Fetch, or attest from a checkout of the pull request's own " + "repository" + ) + return 67 try: check = subprocess.run( [sys.executable, str(LOCAL_REVIEW), "check", "--target", base], @@ -3857,17 +3871,19 @@ def _git(checkout: str, *args: str) -> subprocess.CompletedProcess: def attested(pr: dict) -> bool: - """Whether a comment from someone who can write here vouches for this pull request's head. + """Whether a comment from an owner, member, or collaborator vouches for this pull request's head. Read over every comment rather than the reviewer's own, since an attestation is the maintainer's account speaking. The association is what keeps a passer-by's comment carrying - the same marker from vouching for anything. + the same marker from vouching for anything. The marker counts only as a line of its own + outside a fence, so a comment quoting it in a span or a code block vouches for nothing. """ head = pr.get("headRefOid") or "" for node in (pr.get("comments") or {}).get("nodes") or []: if (node.get("authorAssociation") or "") not in TRUSTED_ASSOCIATIONS: continue - if any(m.group(1) == head for m in ATTESTATION.finditer(node.get("body") or "")): + body = strip_fences(node.get("body") or "", to_end=True) + if any(m.group(1) == head for m in ATTESTATION.finditer(body)): return True return False @@ -4372,14 +4388,19 @@ def main(argv: list[str] | None = None) -> int: # Read from the same, unfiltered history rather than one that drops this pull request's own entries: a genuine review on an earlier head of this same pull request, superseded since by a push, is real evidence about the account and not a self-reference to discard. # A refusal on this pull request's own current head still never reaches this signal, since it is caught directly and at higher priority first. signal = None if a.ignore_quota_signal else quota_signal(history) - snapshot = None if done or answer or drift else gql(Q_FULL, owner, repo, a.number) - stopped = None if a.ignore_quota_signal or snapshot is None else stopping_refusal(snapshot) + snapshot = None if done or drift else gql(Q_FULL, owner, repo, a.number) + stopped = ( + None if a.ignore_quota_signal or answer or snapshot is None else stopping_refusal(snapshot) + ) held = ( snapshot is not None and not a.request and not promotion(snapshot) and first_round_done(snapshot) + and not reviews_truncated(snapshot) + and not partial_shaped(snapshot) and not reviewer_requested(pr) + and (not answer or attested(snapshot)) ) # Request before the first poll, not just at the call site: a caller expects `wait` to make a review happen, not merely to watch for one. # Two prior gaps this closed, a push superseding an already-answered request and an auto-seed that never fired, both left nothing outstanding for the loop below to ever see land. @@ -4484,8 +4505,10 @@ def main(argv: list[str] | None = None) -> int: # Coverage of the head is the other half, returning 0 only once the diff is covered too. covered = held and local_cover(final) halted = None if a.ignore_quota_signal else stopping_refusal(final) - if unrecognized_shapes(final) or ( - not halted and (head_review_done(final, a.min_rounds) or covered) + if ( + unrecognized_shapes(final) + or covered + or (not halted and head_review_done(final, a.min_rounds)) ): verdict = report_verdict(final, owner, repo) # The check reading ranks under both of those, and never replaces either. diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index 9ff776cce..96846436c 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -5806,6 +5806,49 @@ def test_a_promotion_and_a_first_round_are_still_requested(self) -> None: self.cli(["wait", "7", "--timeout", "0"]) self.assertEqual(1, len([c for c in calls if "requestReviews" in c[0]])) + def test_an_attested_head_behind_a_stale_quota_refusal_is_covered(self) -> None: + rounds = [ + review(oid="a" * 39 + "1", at=EARLY, rid="PRR_a"), + review(oid=OLD, body=QUOTA_REFUSED, at=LATE, rid="PRR_b"), + ] + self.answer(self.into(payload(rounds), attest=True)) + self.wire_history([hist_review(7, QUOTA_REFUSED)]) + with mock.patch.object(pr_review.time, "sleep"): + self.assertEqual(0, self.cli(["wait", "7", "--timeout", "0"])) + + def test_a_partial_on_record_requests_a_round_rather_than_holding(self) -> None: + part = OVERVIEW + "\n" + self.answer(self.into(payload([review(oid=OLD, body=part)]), attest=True)) + calls = self.wire_history([hist_review(7, OVERVIEW + "\n" + COVERED)]) + with mock.patch.object(pr_review.time, "sleep"): + self.cli(["wait", "7", "--timeout", "0"]) + self.assertEqual(1, len([c for c in calls if "requestReviews" in c[0]])) + self.assertNotIn("AWAITING_LOCAL_PASS", self.out.getvalue()) + + def test_an_attested_head_past_an_old_plain_answer_is_covered(self) -> None: + pr = self.into(payload([review(oid=OLD, at=EARLY)], comments=[comment(at=LATE)])) + attested = self.into(pr, attest=True) + attested["comments"]["nodes"].append(comment(at=LATE)) + self.answer(attested) + self.wire_history([hist_review(7, OVERVIEW + "\n" + COVERED)]) + with mock.patch.object(pr_review.time, "sleep"): + self.assertEqual(0, self.cli(["wait", "7", "--timeout", "0"])) + + def test_a_pending_request_on_a_fix_push_is_polled_for(self) -> None: + self.answer(self.into(payload([review(oid=OLD)], pending=True), attest=True)) + calls = self.wire_history([hist_review(7, OVERVIEW + "\n" + COVERED)]) + with mock.patch.object(pr_review.time, "sleep"): + self.assertEqual(30, self.cli(["wait", "7", "--timeout", "0"])) + self.assertEqual(0, len([c for c in calls if "requestReviews" in c[0]])) + + def test_a_truncated_review_history_is_no_local_cover(self) -> None: + pr = self.into(payload([review(oid=OLD)], older_reviews=True), attest=True) + self.assertFalse(pr_review.local_cover(pr)) + + def test_checkout_belongs_to_attest_alone(self) -> None: + with contextlib.redirect_stderr(io.StringIO()), self.assertRaises(SystemExit): + pr_review.main(["status", "7", "--repo", "o/r", "--checkout", "."]) + def test_status_reads_an_attested_head_unless_a_partial_is_on_record(self) -> None: part = OVERVIEW + "\n" for rounds, field in ( @@ -7458,6 +7501,11 @@ def setUp(self) -> None: ], ): subprocess.run(["git", "-C", self.dir, *args], check=True, env=env) + subprocess.run( + ["git", "-C", self.dir, "update-ref", "refs/remotes/origin/develop", "HEAD"], + check=True, + env=env, + ) self.head = subprocess.run( ["git", "-C", self.dir, "rev-parse", "HEAD"], capture_output=True, @@ -7475,13 +7523,23 @@ def post(_o: str, _r: str, _n: int, body: str) -> int: self.enterContext(mock.patch.object(pr_review, "comment_on_pr", side_effect=post)) self.enterContext(contextlib.redirect_stdout(io.StringIO())) - def run_attest(self, head: str, check_exit: int = 0) -> int: + def run_attest(self, head: str, check_exit: int = 0, base: str | None = None) -> int: + """Run `attest` with a stub `local_review.py` that records how it was called.""" + record = Path(self.dir).parent / f"calls-{check_exit}.txt" stub = Path(self.dir).parent / f"stub-{check_exit}.py" - stub.write_text(f"import sys\nsys.exit({check_exit})\n") + stub.write_text( + "import os, sys\n" + f"open({str(record)!r}, 'w').write(os.getcwd() + '\\n' + ' '.join(sys.argv[1:]))\n" + f"sys.exit({check_exit})\n" + ) self.addCleanup(stub.unlink) + self.addCleanup(lambda: record.unlink(missing_ok=True)) + self.record = record target = {"headRefOid": head, "baseRefName": "develop"} + merge_base = subprocess.CompletedProcess([], 0, (base or self.head) + "\n", "") with ( mock.patch.object(pr_review, "gql", return_value=target), + mock.patch.object(pr_review, "gh_rest", return_value=merge_base), mock.patch.object(pr_review, "LOCAL_REVIEW", stub), ): return pr_review.attest("o", "r", 7, self.dir) @@ -7489,7 +7547,19 @@ def run_attest(self, head: str, check_exit: int = 0) -> int: def test_a_covered_head_is_attested_by_its_full_commit(self) -> None: self.assertEqual(0, self.run_attest(self.head)) self.assertEqual(1, len(self.posted)) - self.assertIn(f"", self.posted[0]) + self.assertIn(f"\n", self.posted[0]) + cwd, argv = self.record.read_text().split("\n") + self.assertEqual(os.path.realpath(self.dir), os.path.realpath(cwd)) + self.assertEqual("check --target develop", argv) + + def test_a_checkout_measuring_another_merge_base_is_refused(self) -> None: + self.assertEqual(67, self.run_attest(self.head, base="e" * 40)) + self.assertEqual([], self.posted) + + def test_an_unread_pull_request_is_refused(self) -> None: + with mock.patch.object(pr_review, "gql", return_value={}): + self.assertEqual(65, pr_review.attest("o", "r", 7, self.dir)) + self.assertEqual([], self.posted) def test_a_checkout_at_another_commit_is_refused(self) -> None: self.assertEqual(67, self.run_attest("f" * 40)) @@ -7519,6 +7589,8 @@ def test_only_a_writer_s_marker_for_the_current_head_attests(self) -> None: ([self.comment(marker, "NONE")], False), ([self.comment(marker, "CONTRIBUTOR")], False), ([self.comment(f"")], False), + ([self.comment(f"Attest posts `{marker}` as its last line.")], False), + ([self.comment(f"Quoted:\n\n```\n{marker}\n```\n")], False), ([], False), ): with self.subTest(nodes=nodes): From 56ab0901deaaf68f111fc2a7e58c6a949d8b7cc8 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Fri, 2 Oct 2026 06:21:11 -0700 Subject: [PATCH 4/8] Carry the Local Pass Findings Count in an Attestation Per the maintainer's answer, attest reads the covering passes' recorded findings from local_review.py status and carries the total in its marker, which status prints beside the local-pass reading and nothing gates on. Unread merge-base cases report apart, the remote ref is named in full, and the header states the new refusal and the requested cases. Co-Authored-By: Claude Opus 5.5 --- scripts/README.md | 2 +- scripts/local_review.py | 1 + scripts/pr_review.py | 94 ++++++++++++++++++++++++++++---------- tests/test_local_review.py | 1 + tests/test_pr_review.py | 34 +++++++++++--- 5 files changed, 102 insertions(+), 30 deletions(-) diff --git a/scripts/README.md b/scripts/README.md index 1b3f1ccc9..14cec994f 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -175,7 +175,7 @@ python3 scripts/pr_review.py reply 452 --repo ptr727/ProjectTemplate \ python3 scripts/pr_review.py attest 452 --repo ptr727/ProjectTemplate --checkout ../worktree ``` -**A Copilot round is requested at defined moments rather than on every push.** The two moments are a pull request's first round and the head of a pull request into the default branch, a promotion among them. On a pull request into any other branch, a fix push after the first round 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 ``, and only after confirming the checkout is the pushed head, holds no other change, and passes `local_review.py check`. `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 a line of its own outside a fence, and no round on record states or appears to state partial coverage with the whole history in view. `wait` requests nothing on such a head, ending as covered where it is attested and exiting `49` where it is not, and `--request` asks for a round anyway. A partial on record or a history past the window requests a round instead, since no attestation can clear either, and a request already pending is polled for as before. `attest` also refuses where the checkout's merge base with the base branch is not the pull request's own. The rulesets review a pull request when it opens and not on each push, since a trigger on push would spend a round however the tooling chose. 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 two moments are a pull request's first round and the head of a pull request into the default branch, a promotion among them. On a pull request into any other branch, a fix push after the first round 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 ``, and only after confirming the checkout is the pushed head, holds no other change, and passes `local_review.py check`. `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 a line of its own outside a fence, and no round on record states or appears to state partial coverage with the whole history in view. `wait` requests nothing on such a head, ending as covered where it is attested and exiting `49` where it is not, and `--request` asks for a round anyway. A partial on record or a history past the window requests a round instead, since no attestation can clear either, and a request already pending is polled for as before. `attest` also refuses where the checkout's merge base with the base branch is not the pull request's own. The comment carries the findings count the covering pass recorded, which `status` prints beside the reading and nothing gates on, since a local pass's findings are advisory. The rulesets review a pull request when it opens and not on each push, since a trigger on push would spend a round however the tooling chose. 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: the digest renders, every field is well-formed, and nothing in the output disagrees. Two runs read this repository's pull requests while their own was the subject, each caught by the maintainer rather than by the run. The digest leads with `repo=OWNER/NAME` for the same reason, since a number alone reads as correct in any repository. A value that is not `OWNER/NAME` is rejected by name rather than raised as an unpacking traceback, that being the near-miss a required argument still admits. diff --git a/scripts/local_review.py b/scripts/local_review.py index 135ac404b..0a6c96a49 100755 --- a/scripts/local_review.py +++ b/scripts/local_review.py @@ -919,6 +919,7 @@ def cmd_status(args: argparse.Namespace) -> int: "changedPaths": changed, "covered": bool(passes), "reviewers": [p["reviewer"] for p in passes], + "findings": {p["reviewer"]: p.get("findings") for p in passes}, "receiptProblems": problems, }, indent=2, diff --git a/scripts/pr_review.py b/scripts/pr_review.py index 1ae38ae81..6f13e4779 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -12,11 +12,12 @@ `` that `status` and `wait` read. It reads the receipt `local_review.py` keeps in the checkout's git directory, which nothing on GitHub can, and refuses unless the checkout (--checkout, default the current directory) is at - the pull request's head with no change beyond it and `local_review.py check` passes - against the base. Exit 0 = posted, 64 = the write scope could not be established or - excludes the target, 65 = the pull request could not be read, 66 = the response did not - confirm the comment, 67 = the checkout is not the head or holds changes, 68 = no current - local pass covers the content. + the pull request's head with no change beyond it, its merge base with the base branch is + the pull request's own, and `local_review.py check` passes against the base. Exit 0 = + posted, 64 = the write scope could not be established or excludes the target, 65 = the + pull request could not be read, 66 = the response did not confirm the comment, 67 = the + checkout is not the head, holds changes, or measures another merge base, or that merge + base could not be read, 68 = no current local pass covers the content. comment Post one PR-conversation answer, including a suppressed-finding disposition. The PR 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 @@ -191,15 +192,17 @@ on a pull request into a branch other than the default once Copilot has reviewed it at all, since a fix push there is covered by an attested local pass: an attested head ends the wait as covered, and one with no attestation exits 49 naming the `attest` step. - --request asks for a round anyway. A promotion into the default branch, and a pull - request Copilot has not reviewed yet, are requested as before. It reads the Copilot - reviewer's bot id from the repository's own most recently updated PRs rather than a fixed - id: the last HISTORY_PRS, widened once to HISTORY_PRS_WIDE where that narrow window - carries no Copilot activity at all, since an outage that outlasts HISTORY_PRS PRs would - otherwise empty it on every call for as long as the outage runs. Requests nothing - (falling back to polling only) where both windows come up empty, since a repository with - no Copilot review in either has nothing to read the id from and a fabricated one is never - an option. The loop runs in-process, so a 45-minute wait costs one agent turn, not 90. + --request asks for a round anyway. A pull request into the default branch, a promotion + among them, a pull request Copilot has not reviewed yet, and one with a partial on record + or a review history past the window are requested as before. The comment also carries the + findings count the pass recorded, shown and not gated. It reads the Copilot reviewer's + bot id from the repository's own most recently updated PRs rather than a fixed id: the + last HISTORY_PRS, widened once to HISTORY_PRS_WIDE where that narrow window carries no + Copilot activity at all, since an outage that outlasts HISTORY_PRS PRs would otherwise + empty it on every call for as long as the outage runs. Requests nothing (falling back to + polling only) where both windows come up empty, since a repository with no Copilot review + in either has nothing to read the id from and a fabricated one is never an option. The + loop runs in-process, so a 45-minute wait costs one agent turn, not 90. Exit 0 = review present, or on a held head an attested local pass, 30 = still pending at timeout (pending is not failure), 40 = Copilot answered outside a formal review, so read the printed body. @@ -866,7 +869,9 @@ def strip_fences( query($o:String!,$r:String!,$n:Int!){ repository(owner:$o,name:$r){ pullRequest(number:$n){ headRefOid baseRefName } }} """ -ATTESTATION = re.compile(r"^$", re.MULTILINE) +ATTESTATION = re.compile( + r"^$", re.MULTILINE +) TRUSTED_ASSOCIATIONS = frozenset({"OWNER", "MEMBER", "COLLABORATOR"}) LOCAL_REVIEW = Path(__file__).resolve().parent / "local_review.py" @@ -3402,7 +3407,7 @@ def digest( " COVERAGE IS READ FROM THE LOCAL PASS: no Copilot round covers this head, which " "follows the pull request's first round, and a comment from an owner, member, or " "collaborator attests a recorded local strict-review pass over exactly this head's " - "content" + f"content, which recorded {attestation(pr)} finding(s)" ) if cover == PARTIAL: # The line prints under the marker for the reason a suppressed block does. @@ -3816,11 +3821,19 @@ def attest(owner: str, repo: str, num: int, checkout: str) -> int: "head commit does not, so a pass over it does not describe what was pushed" ) return 67 - ours = _git(checkout, "merge-base", f"origin/{base}", "HEAD") + ours = _git(checkout, "merge-base", f"refs/remotes/origin/{base}", "HEAD") theirs = gh_rest( f"repos/{owner}/{repo}/compare/{base}...{head}", ".merge_base_commit.sha // empty" ) - if ours.returncode != 0 or ours.stdout.strip() != theirs.stdout.strip(): + if ours.returncode != 0 or theirs.returncode != 0 or not theirs.stdout.strip(): + print( + f"status=MERGE_BASE_NOT_READ nothing was written: the merge base with {base} could " + "not be read in the checkout or from GitHub, so the pass's scope cannot be compared " + "with the pull request's" + ) + print(f" {(ours.stderr or theirs.stderr).strip()[:400]}") + return 67 + if ours.stdout.strip() != theirs.stdout.strip(): print( f"status=CHECKOUT_NOT_THE_HEAD nothing was written: the checkout's merge base with " f"origin/{base} is not the pull request's own, so a pass against it measured a " @@ -3848,13 +3861,40 @@ def attest(owner: str, repo: str, num: int, checkout: str) -> int: ) print(f" {(check.stdout or check.stderr).strip()[:400]}") return 68 + findings = pass_findings(checkout, base) body = ( f"A recorded local strict-review pass covers head `{head}`, the content this pull " - f"request carries at that commit against `{base}`.\n\n" + f"request carries at that commit against `{base}`, and it recorded {findings} " + f"finding{'' if findings == '1' else 's'}.\n\n" + f"" ) return comment_on_pr(owner, repo, num, body) +def pass_findings(checkout: str, base: str) -> str: + """The findings the covering passes recorded, summed, or "unknown" where any recorded none. + + Shown rather than gated, since a local pass's findings are advisory, and a pass that raised + some is otherwise invisible on the pull request it now covers. + """ + try: + proc = subprocess.run( + [sys.executable, str(LOCAL_REVIEW), "status", "--target", base], + cwd=checkout, + capture_output=True, + text=True, + encoding="utf-8", + timeout=120, + check=False, + ) + counts = json.loads(proc.stdout).get("findings") or {} + except (OSError, subprocess.SubprocessError, ValueError, AttributeError): + return "unknown" + if not counts or not all(isinstance(n, int) and n >= 0 for n in counts.values()): + return "unknown" + return str(sum(counts.values())) + + def _git(checkout: str, *args: str) -> subprocess.CompletedProcess: """One read-only git command in `checkout`, returned whole rather than raised.""" try: @@ -3871,7 +3911,14 @@ def _git(checkout: str, *args: str) -> subprocess.CompletedProcess: def attested(pr: dict) -> bool: - """Whether a comment from an owner, member, or collaborator vouches for this pull request's head. + """Whether an attestation vouches for this pull request's head, per `attestation`.""" + return attestation(pr) is not None + + +def attestation(pr: dict) -> str | None: + """The findings count an owner's, member's, or collaborator's attestation of this head carries. + + None where no attestation vouches for the head, and "unknown" where one carries no count. Read over every comment rather than the reviewer's own, since an attestation is the maintainer's account speaking. The association is what keeps a passer-by's comment carrying @@ -3883,9 +3930,10 @@ def attested(pr: dict) -> bool: if (node.get("authorAssociation") or "") not in TRUSTED_ASSOCIATIONS: continue body = strip_fences(node.get("body") or "", to_end=True) - if any(m.group(1) == head for m in ATTESTATION.finditer(body)): - return True - return False + for m in ATTESTATION.finditer(body): + if m.group(1) == head: + return m.group(2) or "unknown" + return None def promotion(pr: dict) -> bool: diff --git a/tests/test_local_review.py b/tests/test_local_review.py index ab702ab0c..56214f0b7 100755 --- a/tests/test_local_review.py +++ b/tests/test_local_review.py @@ -1420,6 +1420,7 @@ def test_status_emits_json_carrying_the_reviewers(self) -> None: self.assertEqual(code, 0) data = json.loads(buf.getvalue()) self.assertEqual(data["reviewers"], ["agent-skill"]) + self.assertEqual(data["findings"], {"agent-skill": 2}) self.assertTrue(data["covered"]) def test_record_reports_what_it_recorded(self) -> None: diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index 96846436c..1b6ab7e3f 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -5841,6 +5841,13 @@ def test_a_pending_request_on_a_fix_push_is_polled_for(self) -> None: self.assertEqual(30, self.cli(["wait", "7", "--timeout", "0"])) self.assertEqual(0, len([c for c in calls if "requestReviews" in c[0]])) + def test_a_truncated_review_history_requests_a_round_rather_than_holding(self) -> None: + self.answer(self.into(payload([review(oid=OLD)], older_reviews=True), attest=True)) + calls = self.wire_history([hist_review(7, OVERVIEW + "\n" + COVERED)]) + with mock.patch.object(pr_review.time, "sleep"): + self.cli(["wait", "7", "--timeout", "0"]) + self.assertEqual(1, len([c for c in calls if "requestReviews" in c[0]])) + def test_a_truncated_review_history_is_no_local_cover(self) -> None: pr = self.into(payload([review(oid=OLD)], older_reviews=True), attest=True) self.assertFalse(pr_review.local_cover(pr)) @@ -7528,8 +7535,10 @@ def run_attest(self, head: str, check_exit: int = 0, base: str | None = None) -> record = Path(self.dir).parent / f"calls-{check_exit}.txt" stub = Path(self.dir).parent / f"stub-{check_exit}.py" stub.write_text( - "import os, sys\n" - f"open({str(record)!r}, 'w').write(os.getcwd() + '\\n' + ' '.join(sys.argv[1:]))\n" + "import json, os, sys\n" + f"open({str(record)!r}, 'a').write(os.getcwd() + '|' + ' '.join(sys.argv[1:]) + '\\n')\n" + "if sys.argv[1] == 'status':\n" + " print(json.dumps({'findings': {'agent-skill': 2, 'coderabbit-cli': 1}}))\n" f"sys.exit({check_exit})\n" ) self.addCleanup(stub.unlink) @@ -7547,10 +7556,13 @@ def run_attest(self, head: str, check_exit: int = 0, base: str | None = None) -> def test_a_covered_head_is_attested_by_its_full_commit(self) -> None: self.assertEqual(0, self.run_attest(self.head)) self.assertEqual(1, len(self.posted)) - self.assertIn(f"\n", self.posted[0]) - cwd, argv = self.record.read_text().split("\n") - self.assertEqual(os.path.realpath(self.dir), os.path.realpath(cwd)) - self.assertEqual("check --target develop", argv) + self.assertIn(f"\n", self.posted[0]) + calls = [line.split("|") for line in self.record.read_text().splitlines()] + self.assertEqual( + ["check --target develop", "status --target develop"], [c[1] for c in calls] + ) + for cwd, _ in calls: + self.assertEqual(os.path.realpath(self.dir), os.path.realpath(cwd)) def test_a_checkout_measuring_another_merge_base_is_refused(self) -> None: self.assertEqual(67, self.run_attest(self.head, base="e" * 40)) @@ -7581,6 +7593,16 @@ class TestAttestationReadings(unittest.TestCase): def comment(self, body: str, association: str = "OWNER") -> dict: return {"body": body, "authorAssociation": association, "author": {"login": "someone"}} + def test_the_attestation_carries_the_findings_count(self) -> None: + for line, expected in ( + (f"", "4"), + (f"", "unknown"), + (f"", "unknown"), + ): + with self.subTest(line=line): + pr = {"headRefOid": HEAD, "comments": {"nodes": [self.comment(line)]}} + self.assertEqual(expected, pr_review.attestation(pr)) + def test_only_a_writer_s_marker_for_the_current_head_attests(self) -> None: marker = f"" for nodes, expected in ( From 26831ab5ddd948b37e4bee02e6dac85e998b0970 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Fri, 2 Oct 2026 06:31:08 -0700 Subject: [PATCH 5/8] Recheck the Checkout After the Pass and Keep a Head Refusal Over an Attestation Co-Authored-By: Claude Opus 5.5 --- scripts/pr_review.py | 21 +++++++++++++++++---- tests/test_pr_review.py | 29 +++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 4 deletions(-) diff --git a/scripts/pr_review.py b/scripts/pr_review.py index 6f13e4779..8feeafe2d 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -3862,6 +3862,16 @@ def attest(owner: str, repo: str, num: int, checkout: str) -> int: print(f" {(check.stdout or check.stderr).strip()[:400]}") return 68 findings = pass_findings(checkout, base) + after = _git(checkout, "rev-parse", "HEAD") + still = _git( + checkout, "status", "--porcelain", "--untracked-files=all", "--ignore-submodules=none" + ) + if after.stdout.strip() != head or still.returncode != 0 or still.stdout.strip(): + print( + "status=CHECKOUT_NOT_THE_HEAD nothing was written: the checkout moved or changed " + "while the pass was being checked, so the check no longer describes the head" + ) + return 67 body = ( f"A recorded local strict-review pass covers head `{head}`, the content this pull " f"request carries at that commit against `{base}`, and it recorded {findings} " @@ -3958,12 +3968,13 @@ def local_cover(pr: dict) -> bool: Only on a pull request into a branch other than the default, after Copilot's first round, on a head its writer attests, and only where nothing on record says any round read part of a - diff and the whole review history is in view, the bound a file table stands in under. The - maintainer's answer on the hub's issue 2261 set the moments: a fix push after the first - round is covered by the local pass, and a promotion keeps its own Copilot round. + diff and the whole review history is in view, the bound a file table stands in under. A fix + push after the first round is covered by the local pass, and a promotion keeps its own + Copilot round. A refusal on this head is Copilot's own word on it and is never overruled. """ return ( - not promotion(pr) + not refusing_review(pr) + and not promotion(pr) and first_round_done(pr) and attested(pr) and not reviews_truncated(pr) @@ -4351,6 +4362,8 @@ def main(argv: list[str] | None = None) -> int: for flag, value in reply_only.items(): if value is not None: ap.error(f"{flag} belongs to `reply`, not `{a.cmd}`") + if a.cmd != "wait" and a.request: + ap.error(f"--request belongs to `wait`, not `{a.cmd}`") if a.cmd != "attest" and a.checkout is not None: ap.error(f"--checkout belongs to `attest`, not `{a.cmd}`") if a.cmd not in ("comment", "reply") and a.body is not None: diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index 1b6ab7e3f..44fede8c3 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -5856,6 +5856,18 @@ def test_checkout_belongs_to_attest_alone(self) -> None: with contextlib.redirect_stderr(io.StringIO()), self.assertRaises(SystemExit): pr_review.main(["status", "7", "--repo", "o/r", "--checkout", "."]) + def test_request_belongs_to_wait_alone(self) -> None: + with contextlib.redirect_stderr(io.StringIO()), self.assertRaises(SystemExit): + pr_review.main(["status", "7", "--repo", "o/r", "--request"]) + + def test_a_refusal_on_the_head_is_not_overruled_by_an_attestation(self) -> None: + rounds = [ + review(oid=OLD, at=EARLY, rid="PRR_a"), + review(body=REFUSED, at=LATE, rid="PRR_b"), + ] + pr = self.into(payload(rounds), attest=True) + self.assertFalse(pr_review.local_cover(pr)) + def test_status_reads_an_attested_head_unless_a_partial_is_on_record(self) -> None: part = OVERVIEW + "\n" for rounds, field in ( @@ -7517,6 +7529,7 @@ def setUp(self) -> None: ["git", "-C", self.dir, "rev-parse", "HEAD"], capture_output=True, text=True, + encoding="utf-8", check=True, env=env, ).stdout.strip() @@ -7568,6 +7581,22 @@ def test_a_checkout_measuring_another_merge_base_is_refused(self) -> None: self.assertEqual(67, self.run_attest(self.head, base="e" * 40)) self.assertEqual([], self.posted) + def test_a_checkout_that_moves_during_the_check_is_refused(self) -> None: + original = pr_review._git + reads: list[str] = [] + + def moved(checkout: str, *args: str) -> subprocess.CompletedProcess: + proc = original(checkout, *args) + if args == ("rev-parse", "HEAD"): + reads.append("HEAD") + if len(reads) > 1: + return subprocess.CompletedProcess([], 0, "f" * 40 + "\n", "") + return proc + + with mock.patch.object(pr_review, "_git", side_effect=moved): + self.assertEqual(67, self.run_attest(self.head)) + self.assertEqual([], self.posted) + def test_an_unread_pull_request_is_refused(self) -> None: with mock.patch.object(pr_review, "gql", return_value={}): self.assertEqual(65, pr_review.attest("o", "r", 7, self.dir)) From 60a624f2749d0827677a92baf1573e7b5d15628b Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Fri, 2 Oct 2026 06:41:42 -0700 Subject: [PATCH 6/8] Grade a Held Wait on a Fresh Read and Document All Four Attest Checks Co-Authored-By: Claude Opus 5.5 --- scripts/pr_review.py | 30 ++++++++++++++++++++++-------- tests/test_pr_review.py | 11 +++++++++++ 2 files changed, 33 insertions(+), 8 deletions(-) diff --git a/scripts/pr_review.py b/scripts/pr_review.py index 8feeafe2d..203160f71 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -3782,10 +3782,12 @@ def attest(owner: str, repo: str, num: int, checkout: str) -> int: The receipt `local_review.py` records lives in the checkout's git directory, so nothing on GitHub can read it, and the review gate for a fix push needs to. This reads the receipt - where it lives and posts a comment the gate can read, and only after three checks, each of + where it lives and posts a comment the gate can read, and only after four checks, each of which would otherwise vouch for content the pull request does not carry: the checkout's - HEAD is the pull request's head, the checkout holds no change beyond that commit, and - `local_review.py check` finds a current pass against the pull request's base. + HEAD is the pull request's head, the checkout holds no change beyond that commit, its merge + base with the base branch is the pull request's own, and `local_review.py check` finds a + current pass against the pull request's base. The first two are read again once the check + returns, since the checkout can move while it runs. """ ok, why = in_scope(owner) if not ok: @@ -3963,6 +3965,21 @@ def first_round_done(pr: dict) -> bool: return any(not refusal_of(n) for n in reviewer_nodes(pr, "reviews")) +def holds(pr: dict) -> bool: + """Whether this head is a fix push the local pass covers, so `wait` requests no round for it. + + Read again on the payload the verdict is graded on, since a push between two reads moves the + head the hold was decided for. + """ + return ( + not promotion(pr) + and first_round_done(pr) + and not reviews_truncated(pr) + and not partial_shaped(pr) + and not reviewer_requested(pr) + ) + + def local_cover(pr: dict) -> bool: """Whether a recorded local pass stands in for a Copilot round on this head. @@ -4456,11 +4473,8 @@ def main(argv: list[str] | None = None) -> int: held = ( snapshot is not None and not a.request - and not promotion(snapshot) - and first_round_done(snapshot) - and not reviews_truncated(snapshot) - and not partial_shaped(snapshot) and not reviewer_requested(pr) + and holds(snapshot) and (not answer or attested(snapshot)) ) # Request before the first poll, not just at the call site: a caller expects `wait` to make a review happen, not merely to watch for one. @@ -4494,7 +4508,6 @@ def main(argv: list[str] | None = None) -> int: "rather than polling --timeout out against a request that does not exist." ) elif held: - final = snapshot print( "note: this pull request merges into a branch other than the default and Copilot " "has reviewed it already, so a fix push is covered by an attested local pass rather " @@ -4564,6 +4577,7 @@ def main(argv: list[str] | None = None) -> int: # Gating the verdict behind it left the login check unable to reach an exit code. # The digest above printed `shapes=UNRECOGNIZED` the whole time it did so. # Coverage of the head is the other half, returning 0 only once the diff is covered too. + held = held and holds(final) covered = held and local_cover(final) halted = None if a.ignore_quota_signal else stopping_refusal(final) if ( diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index 44fede8c3..d91ee883e 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -5841,6 +5841,17 @@ def test_a_pending_request_on_a_fix_push_is_polled_for(self) -> None: self.assertEqual(30, self.cli(["wait", "7", "--timeout", "0"])) self.assertEqual(0, len([c for c in calls if "requestReviews" in c[0]])) + def test_a_push_during_a_held_wait_grades_the_new_head(self) -> None: + """An attestation of the head read first does not cover the head the verdict reads.""" + first = self.into(payload([review(oid=OLD)]), attest=True) + moved = self.into(payload([review(oid=OLD)]), attest=True) + moved["headRefOid"] = "c" * 40 + self.answer(first, first, moved) + calls = self.wire_history([hist_review(7, OVERVIEW + "\n" + COVERED)]) + with mock.patch.object(pr_review.time, "sleep"): + self.assertEqual(49, self.cli(["wait", "7", "--timeout", "0"])) + self.assertEqual(0, len([c for c in calls if "requestReviews" in c[0]])) + def test_a_truncated_review_history_requests_a_round_rather_than_holding(self) -> None: self.answer(self.into(payload([review(oid=OLD)], older_reviews=True), attest=True)) calls = self.wire_history([hist_review(7, OVERVIEW + "\n" + COVERED)]) From 719472e0f3cf899a15e2c69866bca0fc4b194688 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Fri, 2 Oct 2026 06:52:09 -0700 Subject: [PATCH 7/8] Bind an Attestation to Its Base and Guard the Compare Path Co-Authored-By: Claude Opus 5.5 --- scripts/README.md | 2 +- scripts/pr_review.py | 39 ++++++++++++++++++++++++--------------- tests/test_pr_review.py | 39 ++++++++++++++++++++++++++++++--------- 3 files changed, 55 insertions(+), 25 deletions(-) diff --git a/scripts/README.md b/scripts/README.md index 14cec994f..373108a9a 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -175,7 +175,7 @@ python3 scripts/pr_review.py reply 452 --repo ptr727/ProjectTemplate \ python3 scripts/pr_review.py attest 452 --repo ptr727/ProjectTemplate --checkout ../worktree ``` -**A Copilot round is requested at defined moments rather than on every push.** The two moments are a pull request's first round and the head of a pull request into the default branch, a promotion among them. On a pull request into any other branch, a fix push after the first round 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 ``, and only after confirming the checkout is the pushed head, holds no other change, and passes `local_review.py check`. `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 a line of its own outside a fence, and no round on record states or appears to state partial coverage with the whole history in view. `wait` requests nothing on such a head, ending as covered where it is attested and exiting `49` where it is not, and `--request` asks for a round anyway. A partial on record or a history past the window requests a round instead, since no attestation can clear either, and a request already pending is polled for as before. `attest` also refuses where the checkout's merge base with the base branch is not the pull request's own. The comment carries the findings count the covering pass recorded, which `status` prints beside the reading and nothing gates on, since a local pass's findings are advisory. The rulesets review a pull request when it opens and not on each push, since a trigger on push would spend a round however the tooling chose. 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 two moments are a pull request's first round and the head of a pull request into the default branch, a promotion among them. On a pull request into any other branch, a fix push after the first round 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 ``, and only after confirming the checkout is the pushed head, holds no other change, and passes `local_review.py check`. `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 a line of its own outside a fence, and no round on record states or appears to state partial coverage with the whole history in view. `wait` requests nothing on such a head, ending as covered where it is attested and exiting `49` where it is not, and `--request` asks for a round anyway. A partial on record or a history past the window requests a round instead, since no attestation can clear either, and a request already pending is polled for as before. `attest` also refuses where the checkout's merge base with the base branch is not the pull request's own. The comment carries the findings count the covering pass recorded, which `status` prints beside the reading and nothing gates on, since a local pass's findings are advisory. The rulesets review a pull request when it opens and not on each push, since a trigger on push would spend a round however the tooling chose. 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: the digest renders, every field is well-formed, and nothing in the output disagrees. Two runs read this repository's pull requests while their own was the subject, each caught by the maintainer rather than by the run. The digest leads with `repo=OWNER/NAME` for the same reason, since a number alone reads as correct in any repository. A value that is not `OWNER/NAME` is rejected by name rather than raised as an unpacking traceback, that being the near-miss a required argument still admits. diff --git a/scripts/pr_review.py b/scripts/pr_review.py index 203160f71..f17ab74a8 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -9,15 +9,16 @@ Subcommands attest Publish that a recorded local pass covers the pull request's head, as a comment carrying - `` that `status` and `wait` read. It reads the - receipt `local_review.py` keeps in the checkout's git directory, which nothing on GitHub - can, and refuses unless the checkout (--checkout, default the current directory) is at - the pull request's head with no change beyond it, its merge base with the base branch is - the pull request's own, and `local_review.py check` passes against the base. Exit 0 = - posted, 64 = the write scope could not be established or excludes the target, 65 = the - pull request could not be read, 66 = the response did not confirm the comment, 67 = the - checkout is not the head, holds changes, or measures another merge base, or that merge - base could not be read, 68 = no current local pass covers the content. + `` that `status` and + `wait` read, an attestation counting only against the base it names. It reads the receipt + `local_review.py` keeps in the checkout's git directory, which nothing on GitHub can, and + refuses unless the checkout (--checkout, default the current directory) is at the pull + request's head with no change beyond it, its merge base with the base branch is the pull + request's own, and `local_review.py check` passes against the base. Exit 0 = posted, 64 = + the write scope could not be established or excludes the target, 65 = the pull request + could not be read, 66 = the response did not confirm the comment, 67 = the checkout is + not the head, holds changes, or measures another merge base, or that merge base could not + be read, 68 = no current local pass covers the content. comment Post one PR-conversation answer, including a suppressed-finding disposition. The PR 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 @@ -870,7 +871,8 @@ def strip_fences( repository(owner:$o,name:$r){ pullRequest(number:$n){ headRefOid baseRefName } }} """ ATTESTATION = re.compile( - r"^$", re.MULTILINE + r"^$", + re.MULTILINE, ) TRUSTED_ASSOCIATIONS = frozenset({"OWNER", "MEMBER", "COLLABORATOR"}) LOCAL_REVIEW = Path(__file__).resolve().parent / "local_review.py" @@ -3823,6 +3825,12 @@ def attest(owner: str, repo: str, num: int, checkout: str) -> int: "head commit does not, so a pass over it does not describe what was pushed" ) return 67 + if UNSAFE_REF.search(base) or DOT_SEGMENT.search(base): + print( + f"status=MERGE_BASE_NOT_READ nothing was written: the base branch {base!r} carries a " + "character that would change the compare path rather than travel along it" + ) + return 67 ours = _git(checkout, "merge-base", f"refs/remotes/origin/{base}", "HEAD") theirs = gh_rest( f"repos/{owner}/{repo}/compare/{base}...{head}", ".merge_base_commit.sha // empty" @@ -3878,7 +3886,7 @@ def attest(owner: str, repo: str, num: int, checkout: str) -> int: f"A recorded local strict-review pass covers head `{head}`, the content this pull " f"request carries at that commit against `{base}`, and it recorded {findings} " f"finding{'' if findings == '1' else 's'}.\n\n" - f"" + f"" ) return comment_on_pr(owner, repo, num, body) @@ -3930,21 +3938,22 @@ def attested(pr: dict) -> bool: def attestation(pr: dict) -> str | None: """The findings count an owner's, member's, or collaborator's attestation of this head carries. - None where no attestation vouches for the head, and "unknown" where one carries no count. + None where no attestation vouches for the head against the pull request's current base, so + a retargeted pull request needs a new one, and "unknown" where the pass recorded no count. Read over every comment rather than the reviewer's own, since an attestation is the maintainer's account speaking. The association is what keeps a passer-by's comment carrying the same marker from vouching for anything. The marker counts only as a line of its own outside a fence, so a comment quoting it in a span or a code block vouches for nothing. """ - head = pr.get("headRefOid") or "" + 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 body = strip_fences(node.get("body") or "", to_end=True) for m in ATTESTATION.finditer(body): - if m.group(1) == head: - return m.group(2) or "unknown" + if m.group(1) == head and m.group(2) == base: + return m.group(3) return None diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index d91ee883e..dc474c68b 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -5741,7 +5741,7 @@ def into(self, pr: dict, base: str = "develop", attest: bool = False) -> dict: "author": {"login": "maintainer"}, "authorAssociation": "OWNER", "createdAt": LATE, - "body": f"Attested.\n\n", + "body": f"Attested.\n\n", } pr["comments"] = {"nodes": [marker], "pageInfo": {"hasPreviousPage": False}} return pr @@ -7580,7 +7580,10 @@ def run_attest(self, head: str, check_exit: int = 0, base: str | None = None) -> def test_a_covered_head_is_attested_by_its_full_commit(self) -> None: self.assertEqual(0, self.run_attest(self.head)) self.assertEqual(1, len(self.posted)) - self.assertIn(f"\n", self.posted[0]) + self.assertIn( + f"\n", + self.posted[0], + ) calls = [line.split("|") for line in self.record.read_text().splitlines()] self.assertEqual( ["check --target develop", "status --target develop"], [c[1] for c in calls] @@ -7608,6 +7611,16 @@ def moved(checkout: str, *args: str) -> subprocess.CompletedProcess: self.assertEqual(67, self.run_attest(self.head)) self.assertEqual([], self.posted) + def test_a_base_that_would_change_the_compare_path_is_refused(self) -> None: + target = {"headRefOid": self.head, "baseRefName": "develop#frag"} + with ( + mock.patch.object(pr_review, "gql", return_value=target), + mock.patch.object(pr_review, "gh_rest") as rest, + ): + self.assertEqual(67, pr_review.attest("o", "r", 7, self.dir)) + rest.assert_not_called() + self.assertEqual([], self.posted) + def test_an_unread_pull_request_is_refused(self) -> None: with mock.patch.object(pr_review, "gql", return_value={}): self.assertEqual(65, pr_review.attest("o", "r", 7, self.dir)) @@ -7635,28 +7648,36 @@ def comment(self, body: str, association: str = "OWNER") -> dict: def test_the_attestation_carries_the_findings_count(self) -> None: for line, expected in ( - (f"", "4"), - (f"", "unknown"), - (f"", "unknown"), + (f"", "4"), + (f"", "unknown"), + (f"", None), + (f"", None), ): with self.subTest(line=line): - pr = {"headRefOid": HEAD, "comments": {"nodes": [self.comment(line)]}} + pr = { + "headRefOid": HEAD, + "baseRefName": "develop", + "comments": {"nodes": [self.comment(line)]}, + } self.assertEqual(expected, pr_review.attestation(pr)) def test_only_a_writer_s_marker_for_the_current_head_attests(self) -> None: - marker = f"" + marker = f"" for nodes, expected in ( ([self.comment(marker)], True), ([self.comment(marker, "COLLABORATOR")], True), ([self.comment(marker, "NONE")], False), ([self.comment(marker, "CONTRIBUTOR")], False), - ([self.comment(f"")], False), + ( + [self.comment(f"")], + False, + ), ([self.comment(f"Attest posts `{marker}` as its last line.")], False), ([self.comment(f"Quoted:\n\n```\n{marker}\n```\n")], False), ([], False), ): with self.subTest(nodes=nodes): - pr = {"headRefOid": HEAD, "comments": {"nodes": nodes}} + pr = {"headRefOid": HEAD, "baseRefName": "develop", "comments": {"nodes": nodes}} self.assertIs(expected, pr_review.attested(pr)) def test_a_promotion_is_a_pull_request_into_the_default_branch(self) -> None: From 793f50fb7605b131787f4a429c0e5a22b20562c9 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Fri, 2 Oct 2026 07:02:05 -0700 Subject: [PATCH 8/8] Read the Receipt Once for Coverage and Count, and Mask Code Spans Around the Marker Co-Authored-By: Claude Opus 5.5 --- scripts/README.md | 2 +- scripts/pr_review.py | 85 +++++++++++++++++++++-------------------- tests/test_pr_review.py | 8 ++-- 3 files changed, 49 insertions(+), 46 deletions(-) diff --git a/scripts/README.md b/scripts/README.md index 373108a9a..13e4ae6c4 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -175,7 +175,7 @@ python3 scripts/pr_review.py reply 452 --repo ptr727/ProjectTemplate \ python3 scripts/pr_review.py attest 452 --repo ptr727/ProjectTemplate --checkout ../worktree ``` -**A Copilot round is requested at defined moments rather than on every push.** The two moments are a pull request's first round and the head of a pull request into the default branch, a promotion among them. On a pull request into any other branch, a fix push after the first round 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 ``, and only after confirming the checkout is the pushed head, holds no other change, and passes `local_review.py check`. `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 a line of its own outside a fence, and no round on record states or appears to state partial coverage with the whole history in view. `wait` requests nothing on such a head, ending as covered where it is attested and exiting `49` where it is not, and `--request` asks for a round anyway. A partial on record or a history past the window requests a round instead, since no attestation can clear either, and a request already pending is polled for as before. `attest` also refuses where the checkout's merge base with the base branch is not the pull request's own. The comment carries the findings count the covering pass recorded, which `status` prints beside the reading and nothing gates on, since a local pass's findings are advisory. The rulesets review a pull request when it opens and not on each push, since a trigger on push would spend a round however the tooling chose. 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 two moments are a pull request's first round and the head of a pull request into the default branch, a promotion among them. On a pull request into any other branch, a fix push after the first round 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 ``, and only after confirming the checkout is the pushed head, holds no other change, 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 a line of its own outside a fence, and no round on record states or appears to state partial coverage with the whole history in view. `wait` requests nothing on such a head, ending as covered where it is attested and exiting `49` where it is not, and `--request` asks for a round anyway. A partial on record or a history past the window requests a round instead, since no attestation can clear either, and a request already pending is polled for as before. `attest` also refuses where the checkout's merge base with the base branch is not the pull request's own. The comment carries the findings count the covering pass recorded, which `status` prints beside the reading and nothing gates on, since a local pass's findings are advisory. The rulesets review a pull request when it opens and not on each push, since a trigger on push would spend a round however the tooling chose. 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: the digest renders, every field is well-formed, and nothing in the output disagrees. Two runs read this repository's pull requests while their own was the subject, each caught by the maintainer rather than by the run. The digest leads with `repo=OWNER/NAME` for the same reason, since a number alone reads as correct in any repository. A value that is not `OWNER/NAME` is rejected by name rather than raised as an unpacking traceback, that being the near-miss a required argument still admits. diff --git a/scripts/pr_review.py b/scripts/pr_review.py index f17ab74a8..ee520ed36 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -14,11 +14,12 @@ `local_review.py` keeps in the checkout's git directory, which nothing on GitHub can, and refuses unless the checkout (--checkout, default the current directory) is at the pull request's head with no change beyond it, its merge base with the base branch is the pull - request's own, and `local_review.py check` passes against the base. Exit 0 = posted, 64 = - the write scope could not be established or excludes the target, 65 = the pull request - could not be read, 66 = the response did not confirm the comment, 67 = the checkout is - not the head, holds changes, or measures another merge base, or that merge base could not - be read, 68 = no current local pass covers the content. + request's own, and `local_review.py status` reports a covering pass against the base, + read once for the coverage and the findings count alike. Exit 0 = posted, 64 = the write + scope could not be established or excludes the target, 65 = the pull request could not be + read, 66 = the response did not confirm the comment, 67 = the checkout is not the head, + holds changes, or measures another merge base, or that merge base could not be read, 68 = + no current local pass covers the content. comment Post one PR-conversation answer, including a suppressed-finding disposition. The PR 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 @@ -3783,13 +3784,13 @@ def attest(owner: str, repo: str, num: int, checkout: str) -> int: """Publish that a recorded local pass covers this pull request's head. Returns an exit code. The receipt `local_review.py` records lives in the checkout's git directory, so nothing on - GitHub can read it, and the review gate for a fix push needs to. This reads the receipt - where it lives and posts a comment the gate can read, and only after four checks, each of - which would otherwise vouch for content the pull request does not carry: the checkout's - HEAD is the pull request's head, the checkout holds no change beyond that commit, its merge - base with the base branch is the pull request's own, and `local_review.py check` finds a - current pass against the pull request's base. The first two are read again once the check - returns, since the checkout can move while it runs. + GitHub can read it, and the review gate for a fix push needs to. This reads the receipt where it + lives and posts a comment the gate can read, and only after four checks, each of which would + otherwise vouch for content the pull request does not carry: the checkout's HEAD is the pull + request's head, the checkout holds no change beyond that commit, its merge base with the base + branch is the pull request's own, and `local_review.py status` reports a current pass against + the pull request's base, one read answering the coverage and the findings count alike. The first + two are read again once the check returns, since the checkout can move while it runs. """ ok, why = in_scope(owner) if not ok: @@ -3851,27 +3852,15 @@ def attest(owner: str, repo: str, num: int, checkout: str) -> int: "repository" ) return 67 - try: - check = subprocess.run( - [sys.executable, str(LOCAL_REVIEW), "check", "--target", base], - cwd=checkout, - capture_output=True, - text=True, - encoding="utf-8", - timeout=120, - check=False, - ) - except (OSError, subprocess.SubprocessError): - check = subprocess.CompletedProcess([], 2, "", "local_review.py could not be run") - if check.returncode != 0: + covered, findings, said = receipt_reading(checkout, base) + if not covered: print( - f"status=NO_LOCAL_PASS nothing was written: `local_review.py check --target {base}` " - f"exited {check.returncode}, so no recorded pass covers this content. Run the " - "local strict review, record it, and attest again" + f"status=NO_LOCAL_PASS nothing was written: `local_review.py status --target {base}` " + "reports no recorded pass covering this content. Run the local strict review, " + "record it, and attest again" ) - print(f" {(check.stdout or check.stderr).strip()[:400]}") + print(f" {said.strip()[:400]}") return 68 - findings = pass_findings(checkout, base) after = _git(checkout, "rev-parse", "HEAD") still = _git( checkout, "status", "--porcelain", "--untracked-files=all", "--ignore-submodules=none" @@ -3891,11 +3880,13 @@ def attest(owner: str, repo: str, num: int, checkout: str) -> int: return comment_on_pr(owner, repo, num, body) -def pass_findings(checkout: str, base: str) -> str: - """The findings the covering passes recorded, summed, or "unknown" where any recorded none. +def receipt_reading(checkout: str, base: str) -> tuple[bool, str, str]: + """Whether a recorded pass covers the checkout's content, its findings, and what was read. - Shown rather than gated, since a local pass's findings are advisory, and a pass that raised - some is otherwise invisible on the pull request it now covers. + One `local_review.py status` read answers both, so the coverage and the count describe the + same receipt, where a check followed by a second read could straddle a record replacing it. + The findings are the covering passes' counts summed, or "unknown" where any recorded none, + and they are shown rather than gated, since a local pass's findings are advisory. """ try: proc = subprocess.run( @@ -3907,12 +3898,24 @@ def pass_findings(checkout: str, base: str) -> str: timeout=120, check=False, ) - counts = json.loads(proc.stdout).get("findings") or {} - except (OSError, subprocess.SubprocessError, ValueError, AttributeError): - return "unknown" - if not counts or not all(isinstance(n, int) and n >= 0 for n in counts.values()): - return "unknown" - return str(sum(counts.values())) + except (OSError, subprocess.SubprocessError): + return False, "unknown", "local_review.py could not be run" + try: + data = json.loads(proc.stdout) + except ValueError: + return False, "unknown", proc.stdout or proc.stderr + if proc.returncode != 0 or not isinstance(data, dict) or data.get("covered") is not True: + return False, "unknown", proc.stdout or proc.stderr + if data.get("receiptProblems"): + return False, "unknown", proc.stdout + counts = data.get("findings") + if ( + not isinstance(counts, dict) + or not counts + or not all(isinstance(n, int) and n >= 0 for n in counts.values()) + ): + return True, "unknown", proc.stdout + return True, str(sum(counts.values())), proc.stdout def _git(checkout: str, *args: str) -> subprocess.CompletedProcess: @@ -3950,7 +3953,7 @@ def attestation(pr: dict) -> str | None: for node in (pr.get("comments") or {}).get("nodes") or []: if (node.get("authorAssociation") or "") not in TRUSTED_ASSOCIATIONS: continue - body = strip_fences(node.get("body") or "", to_end=True) + body = CODE_SPAN.sub(" ", strip_fences(node.get("body") or "", to_end=True)) for m in ATTESTATION.finditer(body): if m.group(1) == head and m.group(2) == base: return m.group(3) diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index dc474c68b..e25a08709 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -7562,7 +7562,8 @@ def run_attest(self, head: str, check_exit: int = 0, base: str | None = None) -> "import json, os, sys\n" f"open({str(record)!r}, 'a').write(os.getcwd() + '|' + ' '.join(sys.argv[1:]) + '\\n')\n" "if sys.argv[1] == 'status':\n" - " print(json.dumps({'findings': {'agent-skill': 2, 'coderabbit-cli': 1}}))\n" + f" print(json.dumps({{'covered': {check_exit == 0}, 'receiptProblems': []," + " 'findings': {'agent-skill': 2, 'coderabbit-cli': 1}}))\n" f"sys.exit({check_exit})\n" ) self.addCleanup(stub.unlink) @@ -7585,9 +7586,7 @@ def test_a_covered_head_is_attested_by_its_full_commit(self) -> None: self.posted[0], ) calls = [line.split("|") for line in self.record.read_text().splitlines()] - self.assertEqual( - ["check --target develop", "status --target develop"], [c[1] for c in calls] - ) + self.assertEqual(["status --target develop"], [c[1] for c in calls]) for cwd, _ in calls: self.assertEqual(os.path.realpath(self.dir), os.path.realpath(cwd)) @@ -7674,6 +7673,7 @@ def test_only_a_writer_s_marker_for_the_current_head_attests(self) -> None: ), ([self.comment(f"Attest posts `{marker}` as its last line.")], False), ([self.comment(f"Quoted:\n\n```\n{marker}\n```\n")], False), + ([self.comment(f"A span `\n{marker}\n` across lines.")], False), ([], False), ): with self.subTest(nodes=nodes):