diff --git a/README.md b/README.md index 2d26683..6c55e7f 100644 --- a/README.md +++ b/README.md @@ -63,7 +63,7 @@ the data captured in v1. | **Deployment frequency** | `COUNT(*)` of `argocd_events` per `app_name` / `team` / time window where `operation_phase = 'Succeeded' AND environment = 'prod'`. Drop the `environment` filter (or slice by it) for staging visibility. | | **Lead time for changes** | For each merged PR, `MIN(bitbucket_events.occurred_at)` for the PR (first commit) → `argocd_events.occurred_at` of the prod deploy that carries the same `commit_sha` and `environment = 'prod'`. Joined via the SHA. Stratify by `bitbucket_events.change_type` (feature / hotfix / bugfix / …) to see hotfix lead time vs. feature lead time separately. | | **PR cycle time** | `pullrequest:fulfilled.occurred_at − pullrequest:created.occurred_at` per PR id. | -| **Time to first review** *(DX Core 4 "code review pickup time")* | First reviewer event timestamp − PR-created timestamp on `bitbucket_events`. | +| **Time to first review** *(DX Core 4 "code review pickup time")* | `MIN(occurred_at) WHERE event_type IN ('pr:comment:added', 'pr:reviewer:approved', 'pr:reviewer:unapproved', 'pr:reviewer:needs_work', 'pr:reviewer:updated') AND author != AND NOT is_automated` per PR, minus the matching `pr:opened` row. The five-event union covers every reviewer touch Bitbucket DC emits — silent approvals, retracted approvals, "needs work" flips, and bare reviewer-status changes; `NOT is_automated` strips bot comments (noergler / Renovate / etc.) — every review-time bot must have its handle in the `automation` config block, otherwise its instant comment drives the metric toward zero. | | **Build success rate** | `pipeline_events` with `phase = 'COMPLETED'` grouped by `status`. Slice by `source` to compare Jenkins vs Tekton, by `pipeline_name` / `team` for ownership. | | **Build duration** | `pipeline_events.duration_seconds` (a Postgres `GENERATED ALWAYS AS (finished_at − started_at)` column). | | **Deploy success rate** | `argocd_events` with `operation_phase IN ('Succeeded', 'Failed')` aggregated, filtered to `environment = 'prod'` for the prod-only view. | diff --git a/openshift/collector/riptide.json b/openshift/collector/riptide.json index 75bffa8..4ce1066 100644 --- a/openshift/collector/riptide.json +++ b/openshift/collector/riptide.json @@ -29,6 +29,10 @@ "mend": { "authors": ["mend-bot", "mend[bot]"], "branch_prefixes": ["whitesource-"] + }, + "noergler": { + "authors": ["noergler"], + "branch_prefixes": [] } } } diff --git a/scripts/bitbucket_onboarding.py b/scripts/bitbucket_onboarding.py index 807a5c0..da8c924 100644 --- a/scripts/bitbucket_onboarding.py +++ b/scripts/bitbucket_onboarding.py @@ -60,12 +60,24 @@ DEFAULT_WEBHOOK_NAME = "riptide" # Events riptide's bitbucket router (src/riptide_collector/routers/bitbucket.py) -# can extract data from: PR lifecycle + push (for revert detection in +# can extract data from: PR lifecycle, reviewer activity (for DX Core 4 +# "code review pickup time"), and push (for revert detection in # `push.changes[]`). +# +# Pickup time per DX Core 4 = MIN(timestamp) across the first reviewer +# action of any kind. A single signal (e.g. only `pr:comment:added`) is +# not enough — reviewers often approve silently without typing anything, +# and a comment-only signal both misses those and inflates the metric +# with bot comments. The four reviewer-activity events here together +# cover the workflows BBS DC reviewers actually use. REQUIRED_WEBHOOK_EVENTS: tuple[str, ...] = ( "pr:opened", "pr:from_ref_updated", "pr:comment:added", + "pr:reviewer:approved", + "pr:reviewer:unapproved", + "pr:reviewer:needs_work", + "pr:reviewer:updated", "pr:merged", "pr:deleted", "repo:refs_changed", diff --git a/src/riptide_collector/parsers_bitbucket.py b/src/riptide_collector/parsers_bitbucket.py index 08d8206..331c83b 100644 --- a/src/riptide_collector/parsers_bitbucket.py +++ b/src/riptide_collector/parsers_bitbucket.py @@ -16,6 +16,20 @@ parse_change_type, ) +# Events where the meaningful actor is the reviewer/commenter, not the +# PR author. Used to power the DX Core 4 "code review pickup time" metric: +# MIN(occurred_at WHERE event_type IN these AND author != pr_opener) +# answers "when did someone other than the PR author engage with the PR". +_REVIEWER_ACTIVITY_EVENTS = frozenset( + { + "pr:comment:added", + "pr:reviewer:approved", + "pr:reviewer:needs_work", + "pr:reviewer:updated", + "pr:reviewer:unapproved", + } +) + @dataclass(frozen=True) class BitbucketEventDraft: @@ -151,6 +165,13 @@ def extract_event( author: str | None = None is_revert = False + # Reviewer-activity events carry the actor (the reviewer / commenter) + # as the meaningful "who did this" — different from pr.author who + # opened the PR. We need that to attribute the "first review pickup" + # signal (DX Core 4) to the right user and to filter out the + # PR-author-self-comment and bot-comment noise. + is_reviewer_activity = event_type in _REVIEWER_ACTIVITY_EVENTS + if pr: from_ref = _as_dict(pr.get("fromRef")) display_id = from_ref.get("displayId") @@ -159,8 +180,9 @@ def extract_event( branch_name = display_id if isinstance(latest_commit, str): commit_sha = latest_commit - author_user = _as_dict(_as_dict(pr.get("author")).get("user")) - author = _user_handle(author_user) + if not is_reviewer_activity: + author_user = _as_dict(_as_dict(pr.get("author")).get("user")) + author = _user_handle(author_user) # PR-side revert detection: the title is the only signal we have # without a REST round-trip. Push-side detection would need the # commit messages between fromHash..toHash. diff --git a/tests/fixtures/bitbucket_pr_comment_added.json b/tests/fixtures/bitbucket_pr_comment_added.json new file mode 100644 index 0000000..071578e --- /dev/null +++ b/tests/fixtures/bitbucket_pr_comment_added.json @@ -0,0 +1,48 @@ +{ + "eventKey": "pr:comment:added", + "date": "2026-04-28T10:05:00+0000", + "actor": { + "name": "bob", + "displayName": "Bob Reviewer", + "slug": "bob" + }, + "pullRequest": { + "id": 42, + "title": "ABC-123: Add payment retry", + "description": "Fixes ABC-123 and PROJ-9", + "state": "OPEN", + "fromRef": { + "id": "refs/heads/feature/ABC-123-retries", + "displayId": "feature/ABC-123-retries", + "latestCommit": "abc1234567890abc1234567890abc1234567890a", + "repository": { + "slug": "payments-api", + "project": {"key": "ACME"} + } + }, + "toRef": { + "id": "refs/heads/master", + "displayId": "master", + "repository": { + "slug": "payments-api", + "project": {"key": "ACME"} + } + }, + "author": { + "user": { + "name": "alice", + "displayName": "Alice Example", + "slug": "alice" + } + } + }, + "comment": { + "id": 1001, + "text": "Could you split the retry policy into its own helper?", + "author": { + "name": "bob", + "displayName": "Bob Reviewer", + "slug": "bob" + } + } +} diff --git a/tests/fixtures/bitbucket_pr_reviewer_approved.json b/tests/fixtures/bitbucket_pr_reviewer_approved.json new file mode 100644 index 0000000..52c47bf --- /dev/null +++ b/tests/fixtures/bitbucket_pr_reviewer_approved.json @@ -0,0 +1,50 @@ +{ + "eventKey": "pr:reviewer:approved", + "date": "2026-04-28T10:05:00+0000", + "actor": { + "name": "bob", + "displayName": "Bob Reviewer", + "slug": "bob" + }, + "pullRequest": { + "id": 42, + "title": "ABC-123: Add payment retry", + "description": "Fixes ABC-123 and PROJ-9", + "state": "OPEN", + "fromRef": { + "id": "refs/heads/feature/ABC-123-retries", + "displayId": "feature/ABC-123-retries", + "latestCommit": "abc1234567890abc1234567890abc1234567890a", + "repository": { + "slug": "payments-api", + "project": {"key": "ACME"} + } + }, + "toRef": { + "id": "refs/heads/master", + "displayId": "master", + "repository": { + "slug": "payments-api", + "project": {"key": "ACME"} + } + }, + "author": { + "user": { + "name": "alice", + "displayName": "Alice Example", + "slug": "alice" + } + } + }, + "participant": { + "user": { + "name": "bob", + "displayName": "Bob Reviewer", + "slug": "bob" + }, + "role": "REVIEWER", + "approved": true, + "status": "APPROVED" + }, + "previousStatus": "UNAPPROVED" +} diff --git a/tests/test_config.py b/tests/test_config.py index c6e755c..b301450 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -127,6 +127,18 @@ def test_human_returns_none(self, tmp_path: Path) -> None: store = RiptideConfigStore(path) assert store.detect_automation_source("alice", "feature/x") is None + def test_review_bot_author_match(self, tmp_path: Path) -> None: + # Review-time bots (e.g. noergler) need a config entry to be + # recognised because their handle ("noergler") doesn't match the + # `*-bot` / `*[bot]` heuristic. With the entry in place, comment + # rows authored by noergler land with is_automated=true and the + # DX Core 4 pickup-time query filters them out. + data = json.loads(json.dumps(VALID)) + data["automation"]["noergler"] = {"authors": ["noergler"], "branch_prefixes": []} + path = _write(tmp_path / "c.json", data) + store = RiptideConfigStore(path) + assert store.detect_automation_source("noergler", None) == "noergler" + class TestEnvironments: def test_defaults_when_block_absent(self, tmp_path: Path) -> None: diff --git a/tests/test_parsers_bitbucket.py b/tests/test_parsers_bitbucket.py index 35e6cc1..8d504a2 100644 --- a/tests/test_parsers_bitbucket.py +++ b/tests/test_parsers_bitbucket.py @@ -192,6 +192,92 @@ def test_falls_back_to_actor_when_pr_author_missing(self) -> None: assert isinstance(result, BitbucketEventDraft) assert result.author == "alice" + def test_reviewer_event_uses_actor_not_pr_author(self) -> None: + # Given — pr:reviewer:approved fixture has pullRequest.author=alice + # and top-level actor=bob (the reviewer doing the approval). + body = _load("bitbucket_pr_reviewer_approved.json") + + # When + result = extract_event( + body, + x_event_key="pr:reviewer:approved", + x_request_uuid="r", + x_hook_uuid=None, + ) + + # Then — for review-pickup analytics we want the reviewer's handle, + # not the PR opener's; the parser must prefer actor for these events. + assert isinstance(result, BitbucketEventDraft) + assert result.author == "bob" + # pr_id still extracted from the payload so feedback joins back + # to the opener via bitbucket_events rows for the same pr_id. + assert result.pr_id == 42 + + def test_comment_added_uses_actor_not_pr_author(self) -> None: + # Same rule applies to pr:comment:added — the commenter is the + # signal, not the PR opener. Dedicated fixture (not the reviewer + # fixture with a swapped eventKey) so the shape stays honest. + body = _load("bitbucket_pr_comment_added.json") + + result = extract_event( + body, + x_event_key="pr:comment:added", + x_request_uuid="r", + x_hook_uuid=None, + ) + + assert isinstance(result, BitbucketEventDraft) + assert result.author == "bob" + + def test_reviewer_unapproved_uses_actor_not_pr_author(self) -> None: + # 'pr:reviewer:unapproved' (retracted approval) is also reviewer + # engagement and feeds the pickup-time metric. Same actor-wins rule. + body = _load("bitbucket_pr_reviewer_approved.json") + + result = extract_event( + body, + x_event_key="pr:reviewer:unapproved", + x_request_uuid="r", + x_hook_uuid=None, + ) + + assert isinstance(result, BitbucketEventDraft) + assert result.author == "bob" + assert result.event_type == "pr:reviewer:unapproved" + + def test_reviewer_event_falls_back_to_actor_when_pr_author_missing(self) -> None: + # Edge case: a Bitbucket DC payload without pullRequest.author (or + # with a broken/empty author block). The reviewer-activity path + # already prefers actor; this confirms the regular actor fallback + # at the end of extract_event still works as a backstop and we + # don't end up with author=None. + body = _load("bitbucket_pr_reviewer_approved.json") + body["pullRequest"]["author"] = {} + + result = extract_event( + body, + x_event_key="pr:reviewer:approved", + x_request_uuid="r", + x_hook_uuid=None, + ) + + assert isinstance(result, BitbucketEventDraft) + assert result.author == "bob" + + def test_pr_opened_keeps_pr_author_not_actor(self) -> None: + # Regression guard: for PR-lifecycle events (opened / merged / + # from_ref_updated / deleted) we still want the PR author, even + # if a maintainer (different actor) triggered the event. + body = _load("bitbucket_pr_merged.json") + body["actor"] = {"name": "carol", "slug": "carol"} + + result = extract_event(body, x_event_key="pr:merged", x_request_uuid="r", x_hook_uuid=None) + + assert isinstance(result, BitbucketEventDraft) + # alice opened the PR; carol merged it. The historical row should + # still attribute the PR to alice. + assert result.author == "alice" + class TestEventTypeAndOccurredAt: def test_event_type_defaults_to_unknown_when_header_missing(self) -> None: