feat(bitbucket): wire DX Core 4 'code review pickup time' signals (#57)

* feat(bitbucket): wire DX Core 4 'code review pickup time' signals

The README cites DX Core 4 'code review pickup time' as a derivable metric
but the inputs weren't all present:

- Only `pr:comment:added` was subscribed. DX defines pickup as the first
  ANY reviewer action — silent approvals and `needs_work` flips never
  showed up, and the bot comment noergler posts seconds after pr:opened
  would have driven every PR's pickup-time to ~0.
- For reviewer-activity events the parser was filling `author` from
  `pullRequest.author` (the PR opener) instead of `actor` (the reviewer).
  That meant the `author != pr_opener` filter couldn't ever fire.

This PR:
- Adds `pr:reviewer:approved`, `pr:reviewer:needs_work`, and
  `pr:reviewer:updated` to the Bitbucket onboarding event list. Together
  with the existing `pr:comment:added` they cover every reviewer action
  Bitbucket DC emits.
- Teaches the parser that comment / reviewer events are reviewer-activity:
  it now picks `actor` (the human or bot performing the action) over
  `pullRequest.author` for those event types only. PR-lifecycle events
  (opened / merged / from_ref_updated / deleted) keep the existing PR
  author semantics; a regression test pins this.
- Adds a `noergler` entry to the example automation config so
  `detect_automation_source` flags noergler's instant review comment as
  `is_automated=true`. The existing `NOT is_automated` filter in
  pickup-time queries then strips it. The README explicitly calls out
  that every review-time bot needs a matching automation entry.

Tests cover: actor-as-author for `pr:reviewer:approved` and
`pr:comment:added`, regression guard that `pr:merged` still attributes
to the PR opener (not the merger), and that the config detection picks
up the noergler entry.

* review(#57): subscribe pr:reviewer:unapproved, dedicated comment fixture

- Subscribe pr:reviewer:unapproved and add it to the pickup-time query.
  The parser already treated it as reviewer activity but the event was
  never delivered (not in REQUIRED_WEBHOOK_EVENTS) and never queried, so
  the parser branch was dead. A retracted approval is reviewer engagement
  too — same actor-wins rule applies.
- New tests/fixtures/bitbucket_pr_comment_added.json so the comment-route
  test doesn't reuse the reviewer:approved fixture with a swapped eventKey
  (which left a stale REVIEWER 'participant' block in the payload).
- New test for pr:reviewer:unapproved (actor-as-author).
- New test for the edge case where pullRequest.author is an empty block:
  the actor-wins path covers it cleanly, no None author.

branch_prefixes:[] vs missing key concern (raised in review) confirmed
to be semantically identical in config.py: tuple(cfg.get("branch_prefixes") or []).

Author: trick77

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 != <pr_opener_handle> 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. |

openshift/collector/riptide.json

@@ -29,6 +29,10 @@
     "mend": {
       "authors": ["mend-bot", "mend[bot]"],
       "branch_prefixes": ["whitesource-"]
+    },
+    "noergler": {
+      "authors": ["noergler"],
+      "branch_prefixes": []
     }
   }
 }

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",

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.

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"
+    }
+  }
+}

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"
+}

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:

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: