diff --git a/.github/actions/pr-review/prompts/base-pr-review.md b/.github/actions/pr-review/prompts/base-pr-review.md index 17a8de0..73c0ad0 100644 --- a/.github/actions/pr-review/prompts/base-pr-review.md +++ b/.github/actions/pr-review/prompts/base-pr-review.md @@ -127,10 +127,15 @@ PR head tree, not the previous reviewed tree. ### Step 3 — Audit prior findings (mandatory) Read `.github/prior-findings.json` — it lists every finding this reviewer has -previously posted on this PR (path, line, severity, excerpt, and the thread's -`thread_resolved` / `thread_outdated` state). Also read -`.github/resolved-threads.json` and use its `resolved_count` when reporting -"Threads Resolved" in the summary. +previously posted on this PR: `path`, `line`, `severity`, `excerpt`, the +thread's `thread_resolved` / `thread_outdated` state, and `human_engaged`, +which is true when a person has dealt with the thread at all — wrote in it, or +resolved it themselves. +Also read `.github/resolved-threads.json`: use its `resolved_count` when +reporting "Threads Resolved", and note `resolution_blocked` — when it is true +this run could not resolve its own outdated threads (the token is denied that +mutation in some repos), so expect stale `thread_outdated` entries that nobody +will ever clear. Thread state is not evidence of code state. A resolved or outdated thread does NOT mean the issue was fixed — anyone can resolve a thread without changing @@ -140,11 +145,48 @@ have been fixed since. Only the current code decides. For EACH entry in `prior_findings`, read the current code at (and around) the flagged location and assign exactly one verdict: -- `still present` — the issue exists in the current code. If the existing - thread is outdated (its line no longer matches the code), post a fresh inline - comment at the current location; if the thread is still open and accurate, do - not post a duplicate — the open thread already covers it. Either way, count - it in the summary's Blocking Issues or Suggestions at its severity. +- `still present` — the issue exists in the current code. Count it in the + summary's Blocking Issues or Suggestions at its severity in EVERY case + below; what changes is only whether you open a NEW inline thread for it. + Work down these in order and stop at the first that matches: + - **`human_engaged` is true** — a person replied in that thread or resolved + it. Do not open a new one, whether the thread is open, outdated or + resolved. Report it as "Prior — still present" in the summary. Re-raising + something a person already dealt with belongs in the summary, where it is + said once. + - **Another thread for this same issue is open and accurate, OR has + `human_engaged` true, OR you already posted a fresh comment for this issue + earlier in this audit** — do not post a duplicate. Prior findings are one + entry per thread, not per issue, so one issue appears here once per thread + it has ever had, and the bot's own superseded threads stay in that list + forever. Each of the three clauses closes a different way the same issue + gets raised twice: a live thread already covers it, a person already dealt + with it, or you covered it yourself a moment ago. The last one matters + because the audit walks entries, not issues: without it, an issue with two + stale threads gets two fresh comments in a single run, four after the next + change, and so on. That is also the shape to expect when + `resolution_blocked` is true — nothing is ever auto-resolved, so stale + threads only accumulate, and this clause is the only thing holding the + line. + - **The thread is outdated and nothing above covers the issue** — post a + fresh inline comment at the current location. Nothing on the PR points at + it otherwise, and this is also the path that recovers a finding whose + thread was resolved automatically rather than by a person. + - **Otherwise** (thread open and accurate) — do not post a duplicate. + + This branches on `human_engaged` rather than on `thread_resolved` because + resolution alone does not say who did it: this action resolves its own + outdated, reply-free threads on every run, so most resolved threads are its + doing rather than a decision. `human_engaged` covers both ways a person + signals they have dealt with a finding — replying, and simply clicking + Resolve, which is the more common of the two. An outdated thread also stays + outdated forever, so keying on that alone re-raises the same finding on + every future run, each new thread becoming another prior finding that + re-posts in turn. + + Scope: all of this is about the flagged location. A distinct occurrence of + the same kind of issue somewhere else is a New finding and these rules do + not cover it. - `fixed` — the current code resolves it. Cite the file:line that fixes it. - `obsolete` — the code it applied to was removed or rewritten so the issue no longer applies. Say what replaced it. @@ -252,10 +294,11 @@ confident about is a validated finding at `suggestion` severity with its confide noted, not a dropped finding and not an unvalidated guess. The downstream verdict logic, not pre-filtering, decides what blocks merge. -Handle prior findings per the Step 3 audit — never silently skip them. Do not -post a duplicate inline comment for a still-present issue whose thread is open -and accurate, but DO count it in the summary counts, and DO post a fresh inline -comment when the old thread is outdated and no longer points at the code. +Handle prior findings per the Step 3 audit — never silently skip them, and +apply its four thread cases exactly as written there rather than re-deriving +them. The invariant those cases sit under, and the one thing to carry out of +Step 3 if you carry nothing else: EVERY still-present issue is counted in the +summary at its severity, whatever you decide about its thread. Finally, take stock of your own coverage. If part of the change could not be fully reviewed — truncated or dropped diff paths, unreadable generated content, areas you diff --git a/.github/actions/pr-review/scripts/resolve-outdated-threads.py b/.github/actions/pr-review/scripts/resolve-outdated-threads.py index 09f4a47..aeae22f 100644 --- a/.github/actions/pr-review/scripts/resolve-outdated-threads.py +++ b/.github/actions/pr-review/scripts/resolve-outdated-threads.py @@ -20,7 +20,11 @@ REVIEW_PREFIXES = ("🔴 Security:", "🟠 Bug:", "🟡 Suggestion:") DEFAULT_API_ATTEMPTS = 3 HTTP_STATUS_PATTERN = re.compile(r"HTTP\s+(\d{3})") -BOT_LOGINS = {"github-actions[bot]", "github-actions"} +# Shared with publish-review-report.py and fetch-pr-context.py so every side +# of the contract classifies comments identically. Thread classification is one +# of those sides: an unlisted bot login would read as a human reply here and +# suppress re-raising a finding, so a private copy drifting is fail-open. +from _review_state import BOT_LOGINS # noqa: E402 LIST_THREADS_QUERY = """ query($owner: String!, $repo: String!, $number: Int!, $after: String) { @@ -31,13 +35,14 @@ id isResolved isOutdated + resolvedBy { login __typename } path line comments(first: 20) { totalCount nodes { body - author { login } + author { login __typename } } } } @@ -173,6 +178,38 @@ def severity_of(body: str) -> str: return "unknown" +def _author_is_person(actor: Optional[dict]) -> bool: + """Whether a comment author is a human. + + `PullRequestReviewComment.author` is the `Actor` INTERFACE, so every + GitHub App arrives as a `Bot` node and the type alone settles it. Matching + on the login instead would need a list of every app anyone ever installs, + and would miss them by default — the direction that silently retires a + finding from inline reporting. + """ + return bool(actor) and actor.get("__typename") == "User" + + +def _resolver_is_person(resolver: Optional[dict]) -> bool: + """Whether a thread was resolved by a human. + + This cannot use the same test. `PullRequestReviewThread.resolvedBy` is the + `User` OBJECT, not `Actor`, so an app that resolves a thread is coerced + into a User and its `__typename` is ALWAYS "User" — checking the type here + would read every bot resolution as a person and permanently retire the + finding. Observed on a real PR: `c1-squire-dev[bot]` resolved 37 threads, + each reported as `User`. + + Under that coercion the login keeps the `[bot]` suffix (`c1-squire-dev[bot]`), + which it does NOT carry as a `Bot` node — which is also why BOT_LOGINS has + to list `github-actions` both ways. + """ + if not resolver: + return False + login = resolver.get("login", "") + return bool(login) and login not in BOT_LOGINS and not login.endswith("[bot]") + + def collect_prior_findings(threads: list[dict]) -> list[dict]: """Build the prior-findings list the review prompt audits against. @@ -181,6 +218,26 @@ def collect_prior_findings(threads: list[dict]) -> list[dict]: not mean the issue was fixed; an open one does not mean it is still present), so the reviewer re-verifies each entry against the current code. Unresolved entries sort first, then by path. + + `human_engaged` records whether a person has dealt with the thread at all: + wrote in it, or resolved it. It is not evidence about the code either — + it says the thread is not just this bot talking to itself. Step 3 of the + review prompt branches on it to decide whether to open a NEW inline thread + for a still-present finding, never to skip verifying one. + + It is the axis rather than `thread_resolved`, which does not separate the + populations: should_resolve above resolves this reviewer's own outdated, + reply-free threads on every run, so most resolved threads are the action's + doing. `resolvedBy` is what tells those apart from a person clicking + Resolve, which people do far more often than they reply. + + "A person" is decided differently for the two halves, because GraphQL + types them differently — see _author_is_person and _resolver_is_person. + Either way it is not "a login outside BOT_LOGINS": plenty of other apps + write on these PRs, and counting one as engagement retires the finding + from inline reporting for good. BOT_LOGINS stays narrow on purpose — + _review_state uses it as a trust allowlist for summary authorship, which + is a different question from "is this commenter a human". """ findings = [] for thread in threads: @@ -200,6 +257,26 @@ def collect_prior_findings(threads: list[dict]) -> list[dict]: "excerpt": body.splitlines()[0][:200], "thread_resolved": thread["isResolved"], "thread_outdated": thread["isOutdated"], + # A truncated thread counts as engaged-with. The query takes the + # first 20 comments, so on a longer thread a human reply can sit + # past the cut and read as "nobody ever answered". + # + # Note which way this errs, because it is not the usual one: the + # audit suppresses a new inline thread when this is true, so True + # buys silence, and for a reviewer silence is the unsafe + # direction. It is chosen anyway because a thread past twenty + # comments has demonstrably been engaged with, and re-raising into + # a conversation that long is the worse failure. should_resolve + # above reads the same truncation as a reason not to auto-resolve: + # opposite literal value, same instinct — do not act on what was + # not read. + "human_engaged": ( + thread["comments"]["totalCount"] > len(comments) + or any(_author_is_person(c.get("author")) for c in comments[1:]) + # Resolved by a person rather than by should_resolve or any + # other automation, which keeps the re-anchor path for those. + or _resolver_is_person(thread.get("resolvedBy")) + ), }) findings.sort(key=lambda f: (f["thread_resolved"], f["path"] or "", f["line"] or 0)) return findings @@ -286,6 +363,13 @@ def main(): } write_summary(summary) + # DELIBERATELY the pre-mutation snapshot. `resolve_thread` changes GitHub, + # not these dicts, so a thread resolved seconds ago is still reported + # unresolved and outdated — which is the one path that lets the review + # re-anchor that finding at its new location before the resolution takes + # effect for good. "Reporting the state we just set" looks like an obvious + # cleanup and would silently turn every outdated finding into a summary + # line forever. test_prior_findings_report_pre_resolution_state pins it. write_prior_findings(collect_prior_findings(threads)) print(f"\nDone: resolved {len(resolved)}/{len(to_resolve)} threads") diff --git a/.github/actions/pr-review/scripts/test_verdict_scaffolding.py b/.github/actions/pr-review/scripts/test_verdict_scaffolding.py index 82c327d..21db83e 100755 --- a/.github/actions/pr-review/scripts/test_verdict_scaffolding.py +++ b/.github/actions/pr-review/scripts/test_verdict_scaffolding.py @@ -1755,17 +1755,31 @@ def _thread( outdated: bool = False, path: str = "pkg/foo.go", line: int | None = 42, + replies: list[tuple[str, str]] | None = None, + resolved_by: str | None = None, ) -> dict: + def actor(login: str) -> dict: + # GraphQL reports every GitHub App as a Bot, and the Actions bot's + # login comes back without the "[bot]" suffix. + kind = "Bot" if login in ("github-actions", "github-actions[bot]") \ + or login.endswith("[bot]") else "User" + return {"login": login, "__typename": kind} + + nodes = [{"body": body, "author": actor(author)}] + for reply_author, reply_body in replies or []: + nodes.append({"body": reply_body, "author": actor(reply_author)}) return { "id": "PRRT_x", "isResolved": resolved, "isOutdated": outdated, + # resolvedBy is the User OBJECT, so even an app comes back typed + # "User" — with the "[bot]" suffix the Bot node does not carry. + "resolvedBy": ( + {"login": resolved_by, "__typename": "User"} if resolved_by else None + ), "path": path, "line": line, - "comments": { - "totalCount": 1, - "nodes": [{"body": body, "author": {"login": author}}], - }, + "comments": {"totalCount": len(nodes), "nodes": nodes}, } def test_collects_bot_findings_only(self): @@ -1796,6 +1810,158 @@ def test_outdated_state_preserved(self): findings = rot.collect_prior_findings([self._thread("🟠 Bug: x", outdated=True)]) self.assertTrue(findings[0]["thread_outdated"]) + def test_reply_counts_as_engagement(self): + # The signal the audit needs to tell "a human engaged with this" from + # "the bot has been talking to itself". + findings = rot.collect_prior_findings([ + self._thread("🟠 Bug: x", replies=[("octocat", "fixed in abc1234")]), + ]) + self.assertTrue(findings[0]["human_engaged"]) + + def test_bot_only_thread_is_not_engagement(self): + for replies in (None, [("github-actions[bot]", "re-posting, prior thread outdated")]): + with self.subTest(replies=replies): + findings = rot.collect_prior_findings([ + self._thread("🟠 Bug: x", replies=replies), + ]) + self.assertFalse(findings[0]["human_engaged"]) + + def test_prior_findings_report_pre_resolution_state(self): + # main() resolves outdated bot-only threads and THEN collects prior + # findings from the same, un-refreshed list. That ordering is what + # lets the review re-anchor the finding once before the resolution + # takes effect; "report the state we just set" would look like a + # cleanup and would silently turn every outdated finding into a + # summary line forever. If this fails, read the comment in main(). + thread = self._thread("🟠 Bug: x", outdated=True) + self.assertTrue(rot.should_resolve(thread)) # main() would resolve it + finding = rot.collect_prior_findings([thread])[0] # ...and still reports it + self.assertFalse(finding["thread_resolved"]) # as unresolved + outdated, + self.assertTrue(finding["thread_outdated"]) # which is the re-anchor path + + def test_reply_survives_a_later_bot_comment(self): + # Ordering must not matter: the bot replying last does not erase the + # fact that a human wrote in the thread. + findings = rot.collect_prior_findings([ + self._thread("🟠 Bug: x", replies=[ + ("octocat", "accepted trade-off, see the docs"), + ("github-actions[bot]", "acknowledged"), + ]), + ]) + self.assertTrue(findings[0]["human_engaged"]) + + def test_resolved_and_outdated_thread_still_reported(self): + # The entry is still produced — the audit must re-verify it against + # the code. What changed is the prompt's instruction about opening a + # NEW thread for it, not whether the finding is surfaced at all. + findings = rot.collect_prior_findings([ + self._thread("🟠 Bug: x", resolved=True, outdated=True, + replies=[("octocat", "accepted")]), + ]) + self.assertEqual(len(findings), 1) + self.assertTrue(findings[0]["thread_resolved"]) + self.assertTrue(findings[0]["thread_outdated"]) + self.assertTrue(findings[0]["human_engaged"]) + + def _bot_thread_of_length(self, n: int, total: int | None = None) -> dict: + """A bot-only thread of n FETCHED comments, totalCount defaulting to n. + + The query is comments(first: 20), so a truncated thread really looks + like 20 fetched nodes with a higher totalCount — not one node and a + big number, which the API cannot produce. + """ + thread = self._thread( + "\U0001f7e0 Bug: x", + replies=[("github-actions[bot]", f"bot {i}") for i in range(n - 1)], + ) + thread["comments"]["totalCount"] = n if total is None else total + return thread + + def test_other_apps_are_not_engagement(self): + # Copilot, CodeRabbit, linear-code and friends write on these PRs, and + # c1-squire-dev[bot] has resolved 37 threads on a real connector PR. + # Counting any of them as a person would retire the finding from + # inline reporting permanently. + for app in ("copilot-pull-request-reviewer", "linear-code", "c1-squire-dev"): + with self.subTest(app=app): + replied = self._thread("🟠 Bug: x", replies=[(app + "[bot]", "noted")]) + self.assertFalse( + rot.collect_prior_findings([replied])[0]["human_engaged"] + ) + resolved = self._thread( + "🟠 Bug: x", resolved=True, outdated=True, + resolved_by=app + "[bot]", + ) + self.assertFalse( + rot.collect_prior_findings([resolved])[0]["human_engaged"] + ) + + def test_author_engagement_is_decided_by_node_type(self): + # `author` is the Actor interface, so an app is a Bot node and a human + # login that looks bot-ish is still a User. + human = self._thread("🟠 Bug: x") + human["comments"]["nodes"].append( + {"body": "mine", "author": {"login": "robot-dev", "__typename": "User"}} + ) + human["comments"]["totalCount"] = 2 + self.assertTrue(rot.collect_prior_findings([human])[0]["human_engaged"]) + + def test_resolver_engagement_cannot_use_node_type(self): + # `resolvedBy` is the User OBJECT, so an app that resolves a thread is + # reported as "User" too — keying on the type here would read every + # bot resolution as a person and kill the re-anchor path. Shapes taken + # from the live API. + for login, engaged in ( + ("c1-squire-dev[bot]", False), # observed on baton-datadog#40 + ("github-actions[bot]", False), + ("github-actions", False), # BOT_LOGINS carries both spellings + ("octocat", True), + ): + with self.subTest(login=login): + t = self._thread("🟠 Bug: x", resolved=True, outdated=True) + t["resolvedBy"] = {"login": login, "__typename": "User"} + self.assertEqual( + rot.collect_prior_findings([t])[0]["human_engaged"], engaged + ) + + def test_silent_human_resolution_counts_as_engagement(self): + # Resolving without replying is the common case, and the earlier rule + # read it as "nobody engaged" and re-posted into it — the #147 loop by + # another route. + thread = self._thread( + "🟠 Bug: x", resolved=True, outdated=True, resolved_by="octocat", + ) + self.assertTrue(rot.collect_prior_findings([thread])[0]["human_engaged"]) + + def test_bot_resolution_is_not_engagement(self): + # should_resolve closes this reviewer's own reply-free threads every + # run. Those must stay false, or the re-anchor path disappears. + for resolver in ("github-actions[bot]", "github-actions"): + with self.subTest(resolver=resolver): + thread = self._thread( + "🟠 Bug: x", resolved=True, outdated=True, + resolved_by=resolver, + ) + self.assertFalse( + rot.collect_prior_findings([thread])[0]["human_engaged"] + ) + + def test_unresolved_thread_has_no_resolver(self): + # resolvedBy is null on an open thread; that must not read as a human. + thread = self._thread("🟠 Bug: x", outdated=True) + self.assertIsNone(thread["resolvedBy"]) + self.assertFalse(rot.collect_prior_findings([thread])[0]["human_engaged"]) + + def test_truncation_boundary(self): + # 20 fetched of 20 is complete and must stay False; 20 of 21 is the + # first truncated shape the query can actually return. + complete = self._bot_thread_of_length(20) + self.assertEqual(len(complete["comments"]["nodes"]), 20) + self.assertFalse(rot.collect_prior_findings([complete])[0]["human_engaged"]) + + truncated = self._bot_thread_of_length(20, total=21) + self.assertTrue(rot.collect_prior_findings([truncated])[0]["human_engaged"]) + def test_severity_mapping(self): self.assertEqual(rot.severity_of("🔴 Security: s"), "security") self.assertEqual(rot.severity_of("🟠 Bug: b"), "bug")