diff --git a/AGENTS.md b/AGENTS.md index 914aea3..307b85f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -25,7 +25,7 @@ If `docker ps` fails, ask the user to start OrbStack. - **`riptide.json` is config, not data.** `openshift/collector/riptide.json` (in-repo sample) declares teams (name + `group_email`) and org-wide automation rules; edits go through PRs and the pod hot-reloads via mtime in `RiptideConfigStore.maybe_reload()`. Never propose moving it into Postgres. - **Per-team bearer keys live in a separate file**, mounted in production from the `riptide-collector-team-keys` Secret (never committed); `openshift/collector/team-keys.json` is a dev sample with deterministic test hashes (raw dev bearers in `compose.yaml`). Stored as sha256, hot-reloaded by `TeamKeysStore` like the config. The bearer **is** the team identity — every webhook is tagged `team = caller_team`. - **No `service` column, no `service_id` on the wire.** Per-source aggregations group by `repo_full_name` / `pipeline_name` / `app_name` / `repo`; org-wide rollups by `team`. Identifiers are lowercased at ingest (`commit_sha`, `revision`, `repo_full_name`, `branch_name`, `repo`) so joins are case-stable. Never propose a unified `service` column or `service_id` — it served only single-pane labelling and was dropped. -- **`automation` is org-wide.** Bot definitions live at the config root, not per team. +- **`automation` is org-wide.** Bot definitions live at the config root, not per team. Config is the last resort, not the first: prefer what the upstream payload already states (the acting user's `type == "SERVICE"` — `actor` on push and reviewer events, `pullRequest.author.user` on PR lifecycle ones → `automation_source = "service-account"`, ranked above the `*-bot` name guess) and what senders declare about themselves (`reviewer_handle` / `actor_handle` + kind). Only accounts nobody reports get a config entry. - **Metrics are computed on read, not at ingest.** No aggregation tables or scheduled rollup jobs in v1. Schema additions preserve raw events; new metrics are SQL against existing rows or future materialized views. - **Commit SHA joins Bitbucket↔Pipeline; Argo CD joins on the image reference.** `bitbucket_events.commit_sha = pipeline_events.commit_sha` is deterministic (App-repo SHA both sides). `argocd_events.revision` is the **GitOps-repo SHA** — proven empirically, four Apps for one service share one revision — so it does NOT match the other two. Image **tags are not commit SHAs** either (measured in production: 0 of 4 936 image refs, all semver) — never build correlation on parsing a SHA out of a tag. The contract is the **full image reference**: senders report `pipeline_events.image_ref` (`registry/path:tag`), Argo CD stores the same strings in `payload->'images'` from `.app.status.summary.images`, and the join is exact. Historical rows predating `image_ref` fall back to the release-note correlator in `docs/correlating-deploys-to-commits.md`. Never propose `service_id` or hand-coded name mappings to fix correlation. - **`change_type` lives on Bitbucket events only.** Don't denormalise it onto pipeline / Argo rows; join Pipeline rows via `commit_sha` and Argo rows via `payload->'images'` ↔ `pipeline_events.image_ref` at read time. diff --git a/README.md b/README.md index 3670e74..5502d71 100644 --- a/README.md +++ b/README.md @@ -153,7 +153,7 @@ because there's no clock-start to subtract from in the first place. | **Tickets per deploy** | `COUNT(DISTINCT unnest(jira_keys))` per deploy — small-batch indicator. Jira keys are extracted at write time from PR title, description, branch name, and commit messages via regex `[A-Z][A-Z0-9]+-\d+`, deduplicated, GIN-indexed. | | **Untracked-work rate** | `COUNT(*) WHERE jira_keys = '{}'` over merged PRs — process-compliance signal. | | **Per-ticket flow** | `WHERE 'ABC-1234' = ANY(jira_keys)` returns every event for a ticket across Bitbucket / pipeline / Argo (joined via commit_sha). | -| **Human vs automated split** | `WHERE NOT is_automated` (Renovate / Dependabot / Snyk / Mend / generic-bot detection runs at write time and tags `automation_source`), plus the `non_human_identities` filter for accounts a sender declared. Keep `bot` and `service` apart when reading: a **bot** authors work of its own and its velocity is worth its own view, while a **service** account (a CI user pushing merges) authors nothing and should simply not appear in human activity. Default dashboards exclude both. | +| **Human vs automated split** | `WHERE NOT is_automated` (Renovate / Dependabot / Snyk / Mend / generic-bot detection runs at write time and tags `automation_source`; an account the git host itself marks as a service account — Bitbucket DC's built-in system user, which files the default PR tasks — is tagged `service-account` with no config entry), plus the `non_human_identities` filter for accounts a sender declared. Keep `bot` and `service` apart when reading: a **bot** authors work of its own and its velocity is worth its own view, while a **service** account (a CI user pushing merges) authors nothing and should simply not appear in human activity. Default dashboards exclude both. | | **AI reviewer precision** *(noergler)* | `1 - count(noergler_events WHERE event_type='feedback' AND verdict='disagreed') / sum(findings_count) FILTER (WHERE event_type='pr_completed')` per repo × week — findings on both sides, since one PR can collect several disagreements. Higher = the AI review is more useful. Filter on `outcome='merged'` to score precision only on PRs that shipped. | ### FinOps signals diff --git a/src/riptide_collector/config.py b/src/riptide_collector/config.py index 4d5ee55..49c865d 100644 --- a/src/riptide_collector/config.py +++ b/src/riptide_collector/config.py @@ -212,6 +212,7 @@ def detect_automation_source( author: str | None, branch_name: str | None, author_display_name: str | None = None, + author_is_service_account: bool = False, ) -> str | None: """Match an event's author against the configured automation sources. @@ -220,6 +221,12 @@ def detect_automation_source( login, bot name only in `displayName`) is otherwise indistinguishable from a human, and its instant review comments drive the DX Core 4 pickup-time metric toward zero. + + `author_is_service_account` is the git host's own verdict about the + account (Bitbucket DC marks its built-in system user `type: SERVICE`). + It ranks below the configured sources — those name the tool, which is + more specific — but above the `*-bot` name heuristic, since a stated + fact beats a guess from the handle. """ config = self._config handles = [name for name in (author, author_display_name) if name] @@ -236,6 +243,12 @@ def detect_automation_source( for prefix in source.branch_prefixes: if branch_name.startswith(prefix): return source.name + # The host's verdict beats guessing from the name: an account called + # `ci-bot` that Bitbucket reports as a service account is a service + # account, and mislabelling it `other-bot` would put a technical user + # into bot-velocity views, which are meant to show work bots author. + if author_is_service_account: + return "service-account" if any(looks_bot_shaped(handle) for handle in handles): return "other-bot" return None diff --git a/src/riptide_collector/parsers_bitbucket.py b/src/riptide_collector/parsers_bitbucket.py index 0b456af..e0f9222 100644 --- a/src/riptide_collector/parsers_bitbucket.py +++ b/src/riptide_collector/parsers_bitbucket.py @@ -65,6 +65,10 @@ class BitbucketEventDraft: # detection can match on either. Some review bots post under an # ordinary-looking login and are only recognisable by display name. author_display_name: str | None + # Bitbucket's own verdict on the account: DC marks its built-in system + # user (default PR tasks, stale-PR notices) `type: SERVICE`. Trusting the + # host beats asking every installation to name a server-side account. + author_is_service_account: bool branch_name: str | None change_type: str | None jira_keys: list[str] @@ -151,6 +155,17 @@ def _user_display_name(user: dict[str, Any]) -> str | None: return value if isinstance(value, str) and value else None +def _is_service_account(user: dict[str, Any]) -> bool: + """Whether Bitbucket itself classifies this user as a service account. + + BBS DC sets `type` to NORMAL for people and SERVICE for accounts the + server acts as — the built-in system user that files default PR tasks and + posts stale-PR notices. Those comments land within a second of a PR being + opened, so counted as human they make review pickup time look instant. + """ + return str(user.get("type", "")).upper() == "SERVICE" + + def _synth_delivery_id(event_key: str | None, body: dict[str, Any]) -> str: pr = _as_dict(body.get("pullRequest")) pr_id = pr.get("id") @@ -209,6 +224,7 @@ def extract_event( commit_sha: str | None = None author: str | None = None author_display_name: str | None = None + author_is_service_account = False is_revert = False # Reviewer-activity events carry the actor (the reviewer / commenter) @@ -235,6 +251,7 @@ def extract_event( author_user = _as_dict(_as_dict(pr.get("author")).get("user")) author = _user_handle(author_user) author_display_name = _user_display_name(author_user) + author_is_service_account = _is_service_account(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. @@ -279,6 +296,7 @@ def extract_event( actor = _as_dict(body.get("actor")) author = _user_handle(actor) author_display_name = _user_display_name(actor) + author_is_service_account = _is_service_account(actor) repo_full_name = lower(raw_repo_full_name) branch_name = lower(branch_name) @@ -292,6 +310,7 @@ def extract_event( commit_sha=commit_sha, author=author, author_display_name=author_display_name, + author_is_service_account=author_is_service_account, branch_name=branch_name, change_type=parse_change_type(branch_name), jira_keys=extract_jira_keys(title, description, branch_name), diff --git a/src/riptide_collector/routers/bitbucket.py b/src/riptide_collector/routers/bitbucket.py index 0f9e684..c986a02 100644 --- a/src/riptide_collector/routers/bitbucket.py +++ b/src/riptide_collector/routers/bitbucket.py @@ -75,6 +75,7 @@ async def bitbucket_webhook( # pyright: ignore[reportUnusedFunction] draft.author, draft.branch_name, draft.author_display_name, + draft.author_is_service_account, ) try: diff --git a/tests/test_config.py b/tests/test_config.py index dd92576..eb4a43b 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -201,6 +201,42 @@ def test_rejects_production_stage_in_ignored_list(self, tmp_path: Path) -> None: load_config_from_path(path) +class TestServiceAccountDetection: + def test_host_declared_service_account_is_automation(self, tmp_path: Path) -> None: + # Bitbucket marks its own system user as a service account, so no + # installation has to list it by name. + path = _write(tmp_path / "c.json", VALID) + store = RiptideConfigStore(path) + + assert ( + store.detect_automation_source("bitbucket.system-user", None, "Bitbucket", True) + == "service-account" + ) + + def test_configured_source_keeps_its_own_name(self, tmp_path: Path) -> None: + # The host's verdict is a fallback: a configured bot stays attributed + # to the tool it is, so per-source views keep working. + path = _write(tmp_path / "c.json", VALID) + store = RiptideConfigStore(path) + + assert store.detect_automation_source("renovate-bot", None, None, True) == "renovate" + + def test_human_unaffected(self, tmp_path: Path) -> None: + path = _write(tmp_path / "c.json", VALID) + store = RiptideConfigStore(path) + + assert store.detect_automation_source("alice", "feature/x", "Alice", False) is None + + def test_bot_shaped_name_does_not_mask_a_service_account(self, tmp_path: Path) -> None: + # A stated fact beats a guess from the handle: labelling a technical + # account `other-bot` would drop it into bot-velocity views, which + # exist to show work that bots actually author. + path = _write(tmp_path / "c.json", VALID) + store = RiptideConfigStore(path) + + assert store.detect_automation_source("ci-bot", None, None, True) == "service-account" + + class TestAutomationByDisplayName: def test_display_name_match_when_login_is_ordinary(self, tmp_path: Path) -> None: # A review bot provisioned as a normal user account: the login is a diff --git a/tests/test_parsers_bitbucket.py b/tests/test_parsers_bitbucket.py index 90a3fe8..73e2a1b 100644 --- a/tests/test_parsers_bitbucket.py +++ b/tests/test_parsers_bitbucket.py @@ -452,6 +452,65 @@ def test_occurred_at_parsed_from_iso_string(self) -> None: assert result.occurred_at.tzinfo is not None +class TestServiceAccounts: + def test_service_type_actor_flagged(self) -> None: + # Bitbucket's built-in system user (default PR tasks, stale-PR + # notices) comments within a second of a PR opening. + body = _load("bitbucket_pr_comment_added.json") + body["actor"]["type"] = "SERVICE" + + 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_is_service_account is True + + def test_normal_type_actor_not_flagged(self) -> None: + body = _load("bitbucket_pr_comment_added.json") + body["actor"]["type"] = "NORMAL" + + 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_is_service_account is False + + def test_missing_type_defaults_to_not_a_service_account(self) -> None: + # Older payloads and other hosts may not send `type` at all; absence + # must not promote a person to a service account. + body = _load("bitbucket_pr_comment_added.json") + body["actor"].pop("type", None) + + 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_is_service_account is False + + def test_pr_author_service_type_flagged(self) -> None: + # PR lifecycle events attribute to the PR opener, so the check has to + # follow the same user the author came from. + body = _load("bitbucket_pr_merged.json") + body["pullRequest"]["author"]["user"]["type"] = "SERVICE" + + result = extract_event(body, x_event_key="pr:merged", x_request_uuid="r", x_hook_uuid=None) + + assert isinstance(result, BitbucketEventDraft) + assert result.author_is_service_account is True + + class TestAuthorDisplayName: def test_actor_display_name_travels_with_reviewer_events(self) -> None: # Given a reviewer-activity event, where the actor is the author diff --git a/tests/test_webhooks.py b/tests/test_webhooks.py index 1ab1067..71bb554 100644 --- a/tests/test_webhooks.py +++ b/tests/test_webhooks.py @@ -127,6 +127,35 @@ async def test_renovate_pr_tagged_as_automated( assert row.automation_source == "renovate" assert row.is_automated is True + async def test_host_service_account_tagged_as_automated( + self, + client: AsyncClient, + session_factory: async_sessionmaker[AsyncSession], + ) -> None: + # End to end: Bitbucket's own system user posts the default PR tasks + # seconds after a PR opens. Tagged from the host's `type: SERVICE`, + # with nothing in the automation config naming the account. + del session_factory + payload = _load("bitbucket_pr_comment_added.json") + payload["actor"] = { + "name": "bitbucket.system-user", + "slug": "bitbucket.system-user", + "displayName": "Bitbucket", + "type": "SERVICE", + } + response = await post_bitbucket( + client, + payload, + extra_headers={"X-Request-UUID": "uuid-s", "X-Event-Key": "pr:comment:added"}, + ) + assert response.status_code == 202 + + async with self._fresh_session_factory(client)() as session: + row = (await session.execute(select(BitbucketEvent))).scalar_one() + assert row.author == "bitbucket.system-user" + assert row.automation_source == "service-account" + assert row.is_automated is True + async def test_idempotency_same_uuid_inserts_once( self, client: AsyncClient,