diff --git a/plugins/magpie-security-tools/tools/pr-management/src/pr_management/stats/reference.py b/plugins/magpie-security-tools/tools/pr-management/src/pr_management/stats/reference.py index 3fe3bb820..5f5e90662 100644 --- a/plugins/magpie-security-tools/tools/pr-management/src/pr_management/stats/reference.py +++ b/plugins/magpie-security-tools/tools/pr-management/src/pr_management/stats/reference.py @@ -387,10 +387,10 @@ def classify(pr, ctx, *, partial=False): """Annotate a PR node in place with `_`-prefixed classification fields. `partial=True` declares that the PR came from a reduced schema (the - closed-PR query, which omits the heavy engagement collections — - commits / latestReviews / reviewThreads / timelineItems). Those signals are - read defensively below, so an absent collection contributes False to - `_is_engaged` rather than raising. `isDraft` IS required from both queries + closed-PR query, which omits commits / latestReviews / reviewThreads but + includes the last ClosedEvent actor in timelineItems). Missing collections + are read defensively below and contribute False to `_is_engaged` rather + than raising. `isDraft` IS required from both queries (CLOSED_PRS_QUERY now selects it); it falls back to False only as a guard. """ author = pr["author"]["login"] if pr["author"] else None @@ -434,12 +434,22 @@ def classify(pr, ctx, *, partial=False): break if has_review_thread_collab: break - # Timeline events (LabeledEvent / draft conversion by maintainer) + # Timeline events: open-PR label/draft activity and closed-PR closure actors. has_maintainer_event = False label_added_at = None for ev in pr.get("timelineItems", {}).get("nodes") or []: actor = (ev.get("actor") or {}).get("login") - if actor and not is_bot(actor): + # Exclude only contributors' own label/draft-state activity; + # collaborator PRs and closure actors retain their existing signal. + if ( + actor + and not is_bot(actor) + and not ( + pr["_is_contrib"] + and ev.get("__typename") in ("LabeledEvent", "ReadyForReviewEvent", "ConvertToDraftEvent") + and actor.lower() == (author or "").lower() + ) + ): has_maintainer_event = True if ev.get("label", {}).get("name") == ctx["ready_label"]: at = parse_iso(ev.get("createdAt")) diff --git a/tools/pr-management/src/pr_management/stats/reference.py b/tools/pr-management/src/pr_management/stats/reference.py index 3fe3bb820..5f5e90662 100644 --- a/tools/pr-management/src/pr_management/stats/reference.py +++ b/tools/pr-management/src/pr_management/stats/reference.py @@ -387,10 +387,10 @@ def classify(pr, ctx, *, partial=False): """Annotate a PR node in place with `_`-prefixed classification fields. `partial=True` declares that the PR came from a reduced schema (the - closed-PR query, which omits the heavy engagement collections — - commits / latestReviews / reviewThreads / timelineItems). Those signals are - read defensively below, so an absent collection contributes False to - `_is_engaged` rather than raising. `isDraft` IS required from both queries + closed-PR query, which omits commits / latestReviews / reviewThreads but + includes the last ClosedEvent actor in timelineItems). Missing collections + are read defensively below and contribute False to `_is_engaged` rather + than raising. `isDraft` IS required from both queries (CLOSED_PRS_QUERY now selects it); it falls back to False only as a guard. """ author = pr["author"]["login"] if pr["author"] else None @@ -434,12 +434,22 @@ def classify(pr, ctx, *, partial=False): break if has_review_thread_collab: break - # Timeline events (LabeledEvent / draft conversion by maintainer) + # Timeline events: open-PR label/draft activity and closed-PR closure actors. has_maintainer_event = False label_added_at = None for ev in pr.get("timelineItems", {}).get("nodes") or []: actor = (ev.get("actor") or {}).get("login") - if actor and not is_bot(actor): + # Exclude only contributors' own label/draft-state activity; + # collaborator PRs and closure actors retain their existing signal. + if ( + actor + and not is_bot(actor) + and not ( + pr["_is_contrib"] + and ev.get("__typename") in ("LabeledEvent", "ReadyForReviewEvent", "ConvertToDraftEvent") + and actor.lower() == (author or "").lower() + ) + ): has_maintainer_event = True if ev.get("label", {}).get("name") == ctx["ready_label"]: at = parse_iso(ev.get("createdAt")) diff --git a/tools/pr-management/tests/stats/helpers.py b/tools/pr-management/tests/stats/helpers.py index 5f7843568..6d59a060a 100644 --- a/tools/pr-management/tests/stats/helpers.py +++ b/tools/pr-management/tests/stats/helpers.py @@ -81,8 +81,9 @@ def make_pr( ): """Build one PR node. - ``include_engagement=False`` simulates the reduced CLOSED_PRS_QUERY schema - (no commits / latestReviews / reviewThreads / timelineItems). + ``include_engagement=False`` omits commits / latestReviews / reviewThreads. + An explicit ``timeline`` is still included, for the closed query's last + ClosedEvent actor. Leaving it unset also exercises missing collections. """ node = { "number": number, @@ -97,8 +98,9 @@ def make_pr( if include_engagement: node["latestReviews"] = {"nodes": reviews or []} node["reviewThreads"] = {"nodes": review_threads or []} - node["timelineItems"] = {"nodes": timeline or []} node["commits"] = {"nodes": commits or []} + if include_engagement or timeline is not None: + node["timelineItems"] = {"nodes": timeline or []} if closed_days_ago is not None: node["closedAt"] = _iso(closed_days_ago) node["mergedAt"] = _iso(closed_days_ago) if merged else None diff --git a/tools/pr-management/tests/stats/test_engagement.py b/tools/pr-management/tests/stats/test_engagement.py new file mode 100644 index 000000000..a3c42e40a --- /dev/null +++ b/tools/pr-management/tests/stats/test_engagement.py @@ -0,0 +1,218 @@ +# SPDX-License-Identifier: Apache-2.0 +# https://www.apache.org/licenses/LICENSE-2.0 +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Author timeline activity must not hide contributor PRs awaiting triage.""" + +from __future__ import annotations + +import pytest + +from pr_management.stats import dashboard, reference + +from .helpers import _iso, comment, make_ctx, make_pr + +EVENT_TYPES = ("LabeledEvent", "ReadyForReviewEvent", "ConvertToDraftEvent") + + +def _event(kind, actor="contributor", *, label="area:docs", days_ago=1): + event = { + "__typename": kind, + "createdAt": _iso(days_ago), + "actor": {"login": actor} if actor else None, + } + if kind == "LabeledEvent": + event["label"] = {"name": label} + return event + + +@pytest.mark.parametrize("kind", EVENT_TYPES) +# The case variant is defensive coverage, not a claim about live API casing. +@pytest.mark.parametrize("actor", ["contributor", "CONTRIBUTOR"]) +def test_author_timeline_activity_stays_in_untriaged_backlog(kind, actor): + ctx = make_ctx() + pr = make_pr(1, created_days_ago=40, timeline=[_event(kind, actor)]) + + reference.classify(pr, ctx) + + assert pr["_is_engaged"] is False + assert pr["_is_untriaged"] is True + hero = dashboard.compute_hero_counts([pr], ctx["now"]) + assert hero["untriaged"] == hero["untriaged_4w"] == 1 + assert hero["defacto"] == 0 + assert dashboard.compute_health_rating(hero)[0] == "⚠️ Needs attention" + recommendations = dashboard.compute_recommendations([pr], [], [], hero, 0, ctx["now"]) + triage = next( + r for r in recommendations if r["action"] == "/magpie-pr-management:pr-triage all PR issues" + ) + assert triage["priority"] == "high" + assert triage["count"] == 1 + + +@pytest.mark.parametrize("kind", EVENT_TYPES) +def test_other_human_timeline_activity_still_counts_as_engagement(kind): + pr = make_pr(1, timeline=[_event(kind, "maintainer")]) + + reference.classify(pr, make_ctx()) + + assert pr["_is_engaged"] is True + assert pr["_is_untriaged"] is False + + +@pytest.mark.parametrize("actor", ["github-actions[bot]", "Copilot", None]) +def test_bot_or_missing_actor_does_not_count_as_engagement(actor): + pr = make_pr(1, timeline=[_event(kind, actor) for kind in EVENT_TYPES]) + + reference.classify(pr, make_ctx()) + + assert pr["_is_engaged"] is False + assert pr["_is_untriaged"] is True + + +def test_author_event_does_not_hide_other_human_event(): + pr = make_pr( + 1, + timeline=[_event("ReadyForReviewEvent"), _event("LabeledEvent", "maintainer")], + ) + + reference.classify(pr, make_ctx()) + + assert pr["_is_engaged"] is True + + +def test_author_event_does_not_suppress_collaborator_comment(): + pr = make_pr(1, timeline=[_event("ReadyForReviewEvent")], comments=[comment("Looking into this.")]) + + reference.classify(pr, make_ctx()) + + assert pr["_is_engaged"] is True + + +def test_ready_label_and_latest_label_timestamp_are_independent_of_actor(): + ctx = make_ctx() + ready = ctx["ready_label"] + pr = make_pr( + 1, + labels=[ready], + timeline=[ + _event("LabeledEvent", label=ready, days_ago=1), + _event("LabeledEvent", "maintainer", label=ready, days_ago=3), + ], + ) + + reference.classify(pr, ctx) + + assert pr["_has_ready"] is True + assert pr["_is_engaged"] is True + assert pr["_is_untriaged"] is False + assert pr["_label_added_at"] == reference.parse_iso(_iso(1)) + + +def test_ready_label_is_engagement_even_when_only_author_has_events(): + ctx = make_ctx() + pr = make_pr( + 1, + labels=[ctx["ready_label"]], + timeline=[_event("LabeledEvent", label=ctx["ready_label"])], + ) + + reference.classify(pr, ctx) + + assert pr["_is_engaged"] is True + assert pr["_is_untriaged"] is False + assert pr["_label_added_at"] == reference.parse_iso(_iso(1)) + + +def test_other_human_event_with_deleted_pr_author(): + pr = make_pr(1, author=None, timeline=[_event("LabeledEvent", "maintainer")]) + + reference.classify(pr, make_ctx()) + + assert pr["_is_engaged"] is True + + +@pytest.mark.parametrize("kind", EVENT_TYPES) +@pytest.mark.parametrize("association", ["OWNER", "MEMBER", "COLLABORATOR"]) +def test_collaborator_own_timeline_activity_remains_engagement(kind, association): + ctx = make_ctx() + pr = make_pr(1, author="maintainer", assoc=association, timeline=[_event(kind, "maintainer")]) + + reference.classify(pr, ctx) + + assert pr["_is_engaged"] is True + assert pr["_is_untriaged"] is False + assert dashboard.compute_hero_counts([pr], ctx["now"])["defacto"] == 1 + + +@pytest.mark.parametrize("association", ["CONTRIBUTOR", "MEMBER"]) +@pytest.mark.parametrize("merged", [False, True]) +@pytest.mark.parametrize("typed", [False, True]) +def test_closed_pr_own_actor_remains_engagement(association, merged, typed): + # Preserve legacy self-closure behavior for this limited change. + # A separate closed-PR policy fix may revise this compatibility expectation. + # The real closed queries select only actor.login, without __typename. + event: dict[str, object] = {"actor": {"login": "author"}} + if typed: + event["__typename"] = "ClosedEvent" + pr = make_pr( + 1, + author="author", + assoc=association, + closed_days_ago=2, + merged=merged, + include_engagement=False, + timeline=[event], + ) + assert pr["timelineItems"]["nodes"] == [event] + assert not {"commits", "latestReviews", "reviewThreads"}.intersection(pr) + + reference.classify(pr, make_ctx(), partial=True) + + assert pr["_is_engaged"] is True + + +def test_closed_pr_coverage_preserves_all_closure_actors(): + # Keep the legacy closed-PR metric here, including contributor self-closes. + # This compatibility assertion does not endorse self-closure as maintainer engagement. + ctx = make_ctx() + closed_prs = [] + for n, (author, association, actor, merged) in enumerate( + [ + ("member", "MEMBER", "member", True), + ("collaborator", "COLLABORATOR", "collaborator", True), + ("contributor", "CONTRIBUTOR", "contributor", False), + ("other", "CONTRIBUTOR", "maintainer", False), + ], + start=1, + ): + pr = make_pr( + n, + author=author, + assoc=association, + created_days_ago=20, + closed_days_ago=2, + merged=merged, + include_engagement=False, + timeline=[{"actor": {"login": actor}}], + ) + reference.classify(pr, ctx, partial=True) + closed_prs.append(pr) + + coverage = dashboard.compute_triage_coverage_rate(closed_prs, ctx["weeks"]) + bucket = next(row for row in coverage if row["opened"]) + assert bucket["opened"] == bucket["engaged"] == 4 + assert bucket["rate"] == 100.0 diff --git a/tools/pr-management/tests/stats/test_json_parity.py b/tools/pr-management/tests/stats/test_json_parity.py index e3a1bbf04..e1900c696 100644 --- a/tools/pr-management/tests/stats/test_json_parity.py +++ b/tools/pr-management/tests/stats/test_json_parity.py @@ -142,3 +142,36 @@ def test_known_counts_for_fixture(tmp_path, monkeypatch): assert dash["ready_count"] == 1 assert dash["untriaged_count"] == 1 assert dash["partial"] is False + + +def test_collaborator_self_activity_counts_in_written_sidecars(tmp_path, monkeypatch): + monkeypatch.setattr( + f"{__name__}._open_fixture", + lambda: [ + make_pr( + 1, + author="maintainer", + assoc="MEMBER", + timeline=[ + { + "__typename": "LabeledEvent", + "actor": {"login": "maintainer"}, + "label": {"name": "area:docs"}, + } + ], + ), + make_pr( + 2, + author="maintainer", + assoc="MEMBER", + comments=[comment("Looking into this.", author="maintainer", assoc="MEMBER")], + ), + ], + ) + + for module, stem in [(reference, "reference"), (dashboard, "dashboard")]: + with monkeypatch.context() as m: + sidecar = _run(m, module, tmp_path / f"{stem}.html") + assert sidecar["engaged_count"] == 2 + if module is dashboard: + assert sidecar["hero"]["defacto"] == 2