Fix bot detection and deploy-to-commit correlation

AGENTS.md

@@ -27,10 +27,10 @@ If `docker ps` fails, ask the user to start OrbStack.
 - **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.
 - **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 needs `payload->'images'`.** `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. The App-repo SHA rides in image tags from `.app.status.summary.images`, stored as `payload->'images'`; a future correlator extracts it to bridge Argo CD to pipeline events. The image-tag SHA is the contract — 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 the image-tag SHA in `payload->'images'` at read time.
+- **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.
 - **CI events are source-tagged, not source-routed.** All pipeline events from any CI (Jenkins, Tekton, …) land in the single `pipeline_events` table via `POST /webhooks/pipeline`, distinguished by the `source` column. Do not add per-CI tables or endpoints. The dedup key is `source#pipeline_name#run_id#phase`.
-- **Noergler events carry finops + reviewer-precision only.** The `noergler_events` table is `event_type`-discriminated (`completed` | `feedback`) and is fed by `POST /webhooks/noergler` from optional noergler instances. Do not re-emit PR lifecycle from noergler — `bitbucket_events` already covers open / merged / declined. Dedup keys: `completed#<run_id>` and `feedback#<finding_id>#<verdict>`.
+- **Noergler events carry finops + reviewer-precision only.** The `noergler_events` table is `event_type`-discriminated (`pr_completed` | `feedback`; historical rows may carry the pre-0002 `completed`) and is fed by `POST /webhooks/noergler` from optional noergler instances. Do not re-emit PR lifecycle from noergler — `bitbucket_events` already covers open / merged / declined. Dedup keys: `pr_completed#<pr_key>#<outcome>` and `feedback#<finding_id>#<verdict>`. `pr_completed` is also the source for PR diff size and for `reviewer_handle`, the bot's own account.
 - **Senders verify reachability + bearer at startup via `GET /auth/ping`** — authenticated, returns `{"status":"ok","team":"<caller_team>"}`, so a wrong token fails fast. Never reuse `/health` (unauth liveness) or `/ready` (unauth readiness) for it; those answer different questions.
 - **`modified_at` has a Postgres trigger** (`riptide_set_modified_at`), not just SQLAlchemy `onupdate`. Raw-SQL updates also bump it. Keep the trigger when changing migrations.
 - **Database is external.** `riptide-collector` does NOT manage Postgres. Do not add a Postgres Deployment to `openshift/`.

README.md

@@ -60,10 +60,10 @@ the data captured in v1.
 
 | Metric | How it's computed |
 |---|---|
-| **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. |
+| **Deployment frequency** | `COUNT(DISTINCT revision)` of `argocd_events` per `team` / time window where `operation_phase = 'Succeeded' AND environment = 'prod'`. Count revisions, not rows: one release reconciles every Argo App that shares the GitOps revision, so `COUNT(*)` per `app_name` overcounts releases. Group by `app_name` for the per-App drill-down. |
+| **Lead time for changes** | Deploy → build → commit, joined on the image reference: `argocd_events.payload->'images'` contains the full image refs Argo rendered, `pipeline_events.image_ref` is what the build published. From the pipeline row, `commit_sha` gives the App-repo commit; its first sighting in `bitbucket_events` starts the clock, the `argocd_events.occurred_at` of the prod deploy ends it. Stratify by `bitbucket_events.change_type` (feature / hotfix / bugfix / …) to separate hotfix from feature lead time. `argocd_events.revision` is the GitOps-repo SHA and does **not** join to `commit_sha` — see [Correlating deploys back to commits](docs/correlating-deploys-to-commits.md), which also documents the read-time fallback for events collected before senders reported `image_ref`. |
 | **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")* | Two-part computation per PR — see the SQL block below the table. Clock-start = `COALESCE(pr:ready_for_review, pr:opened)`: PRs opened ready start at `pr:opened`; PRs opened as drafts start at the synthetic `pr:ready_for_review` (emitted by the parser when a `pr:modified` payload carries `previousDraft=true, draft=false`). Engagement = first reviewer touch (`pr:comment:added`, `pr:reviewer:approved`, `pr:reviewer:unapproved`, `pr:reviewer:needs_work`, `pr:reviewer:updated`) where `author != pr_opener AND NOT is_automated AND occurred_at >= clock-start`. The five-event reviewer union covers every touch Bitbucket DC emits (silent approvals, retracted approvals, "needs work" flips, bare reviewer-status changes); the `occurred_at >= clock-start` guard drops early-feedback comments solicited during the draft phase, which would otherwise produce negative pickup times. `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. |
+| **Time to first review** *(DX Core 4 "code review pickup time")* | Two-part computation per PR — see the SQL block below the table. Clock-start = `COALESCE(pr:ready_for_review, pr:opened)`: PRs opened ready start at `pr:opened`; PRs opened as drafts start at the synthetic `pr:ready_for_review` (emitted by the parser when a `pr:modified` payload carries `previousDraft=true, draft=false`). Engagement = first reviewer touch (`pr:comment:added`, `pr:reviewer:approved`, `pr:reviewer:unapproved`, `pr:reviewer:needs_work`, `pr:reviewer:updated`) where `author != pr_opener AND NOT is_automated AND occurred_at >= clock-start`. The five-event reviewer union covers every touch Bitbucket DC emits (silent approvals, retracted approvals, "needs work" flips, bare reviewer-status changes); the `occurred_at >= clock-start` guard drops early-feedback comments solicited during the draft phase, which would otherwise produce negative pickup times. `NOT is_automated` strips bot comments (noergler / Renovate / etc.) — every review-time bot must be recognisable, otherwise its instant comment drives the metric toward zero. Detection matches the `automation` config block against **both** the login handle (`author`) and the display name (`author_display_name`), because a bot is often provisioned as an ordinary user account whose login says nothing. `is_automated` is decided at ingest, so rows written before a bot was recognised stay marked human: the `non_human_identities` CTE in the query below filters those at read time from the accounts senders declare (`reviewer_handle` + `reviewer_account_kind` on noergler rollups, `actor_handle` + `actor_account_kind` on pipeline events) — riptide stores the declaration, it carries no account names of its own. |
 | **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. |
@@ -78,7 +78,20 @@ dropped at parse time. The raw `eventKey` survives on `payload.eventKey` for
 traceability. With that in place, the metric is one CTE:
 
 ```sql
-WITH pickup_start AS (
+WITH non_human_identities AS (
+  -- Accounts senders declared as non-human, learned from the stream rather
+  -- than from config: each sender reports the git-host account it acts
+  -- through and what that account is. `is_automated` is decided at ingest,
+  -- so rows written before an identity was declared are filtered here.
+  SELECT DISTINCT lower(reviewer_handle) AS handle
+  FROM noergler_events
+  WHERE reviewer_handle IS NOT NULL AND reviewer_account_kind IN ('bot', 'service')
+  UNION
+  SELECT DISTINCT lower(actor_handle)
+  FROM pipeline_events
+  WHERE actor_handle IS NOT NULL AND actor_account_kind IN ('bot', 'service')
+),
+pickup_start AS (
   SELECT
     repo_full_name,
     pr_id,
@@ -109,6 +122,9 @@ WHERE ps.clock_start IS NOT NULL
       )
   AND e.author IS DISTINCT FROM ps.pr_opener
   AND NOT e.is_automated
+  AND NOT EXISTS (
+        SELECT 1 FROM non_human_identities n WHERE n.handle = lower(e.author)
+      )
   AND e.occurred_at >= ps.clock_start
 GROUP BY e.repo_full_name, e.pr_id, ps.clock_start;
 ```
@@ -130,15 +146,15 @@ because there's no clock-start to subtract from in the first place.
 
 | Metric | How it's computed |
 |---|---|
-| **PR size** | `lines_added`, `lines_removed`, `files_changed` columns on `bitbucket_events` (extracted from the PR payload). |
+| **PR size** | `lines_added`, `lines_removed`, `files_changed` on `noergler_events` (`event_type = 'pr_completed'`), joined to Bitbucket PRs on `pr_key` = `'<repo_full_name>#<pr_id>'`. The same columns exist on `bitbucket_events` but are always NULL: Bitbucket DC webhooks carry no diff stats, and fetching them would put an outbound REST call in the ingest path. Covers the repos noergler reviews. |
 | **Revert rate** | `COUNT(*) WHERE is_revert = true` over total commits — a free, weak Change-Failure-Rate proxy. |
 | **Hotfix rate** | `COUNT(*) WHERE change_type = 'hotfix'` over total deploys per window — operational-pain signal. |
 | **Change mix** | Distribution of `change_type` (feature / bugfix / hotfix / chore / refactor / docs / other) per team per week. |
 | **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`). Default dashboards exclude bots; bot velocity is a separate CI-health view. |
-| **AI reviewer precision** *(noergler)* | `1 - count(noergler_events WHERE event_type='feedback' AND verdict='disagreed') / count(noergler_events WHERE event_type='pr_completed')` per repo × week. Higher = the AI review is more useful. Filter on `outcome='merged'` to score precision only on PRs that shipped. |
+| **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. |
+| **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
 
@@ -178,9 +194,12 @@ What riptide does **not** provide today, and the natural seam for it:
 - **Pre-aggregated metric tables.** Compute on read; only materialize when
   query volume justifies it.
 
-The universal join key across all three sources is the **commit SHA**
-(`bitbucket_events.commit_sha`, `pipeline_events.commit_sha`,
-`argocd_events.revision`).
+Bitbucket and pipeline events join on the **commit SHA**
+(`bitbucket_events.commit_sha` = `pipeline_events.commit_sha`, App-repo SHA on
+both sides). Argo CD does not: `argocd_events.revision` is the GitOps-repo SHA.
+Deploys reach the commit through the image reference — `pipeline_events.image_ref`
+against `argocd_events.payload->'images'` — as described in
+[Correlating deploys back to commits](docs/correlating-deploys-to-commits.md).
 
 ## Quickstart (local)
 

docs/correlating-deploys-to-commits.md

@@ -0,0 +1,82 @@
+# Correlating deploys back to commits
+
+Lead time for changes, tickets-per-deploy and per-ticket flow all need the same
+thing: given an Argo CD deploy, which App-repo commits did it ship?
+
+`argocd_events.revision` does **not** answer that. It is the GitOps-repo SHA —
+several Apps of one service share it, and it matches neither
+`pipeline_events.commit_sha` nor `bitbucket_events.commit_sha`. Image tags do
+not answer it either: they are usually versions (`registry/app:2.0.41`), not
+commit SHAs.
+
+## The contract: image reference
+
+CI senders report the full image reference they published as
+`pipeline_events.image_ref`. Argo CD stores the rendered references of the
+synced manifests in `argocd_events.payload->'images'`. The two are the same
+strings, so the join is exact and the pipeline row carries the App-repo
+`commit_sha`:
+
+```sql
+select a.app_name, a.environment, a.occurred_at as deployed_at,
+       p.commit_sha, p.pipeline_name
+from argocd_events a
+cross join lateral jsonb_array_elements_text(a.payload->'images') as img(ref)
+join pipeline_events p on p.image_ref = img.ref
+-- A sender that reports image_ref on every phase writes STARTED and COMPLETED
+-- rows carrying the same reference; without this filter each deploy fans out
+-- across them and every count or average built on the join is inflated.
+where a.operation_phase = 'Succeeded'
+  and p.phase = 'COMPLETED'
+  and p.status = 'SUCCESS';
+```
+
+Lead time then measures from the first sighting of `p.commit_sha` in
+`bitbucket_events` to `a.occurred_at`.
+
+Senders that publish no image simply omit `image_ref`; those runs stay
+uncorrelated, which is honest.
+
+## Fallback for rows collected before `image_ref`
+
+Where the GitOps repo is itself onboarded to the Bitbucket webhook, its commits
+are in `bitbucket_events` and `argocd_events.revision` matches
+`bitbucket_events.commit_sha` for those rows. If the release commit's message
+lists the component bumps it carries — many release-note generators emit compare
+links containing both the old and the new App-repo SHA — those SHAs can be
+extracted at read time:
+
+```sql
+with release as (
+  select distinct a.revision, a.app_name, a.environment,
+         a.occurred_at as deployed_at, b.payload::text as body
+  from argocd_events a
+  join bitbucket_events b on b.commit_sha = a.revision
+  where a.operation_phase = 'Succeeded'
+    -- prefilter: keeps the regex off every push payload in the table
+    and b.payload::text like '%compare/diff?targetBranch=%'
+),
+bumped as (
+  select revision, app_name, environment, deployed_at,
+         regexp_matches(
+           body,
+           'repos/([a-z0-9-]+)/compare/diff\?targetBranch=([0-9a-f]{40})&sourceBranch=([0-9a-f]{40})',
+           'g') as m
+  from release
+)
+select app_name, environment, deployed_at,
+       m[1] as app_repo, m[3] as commit_sha
+from bumped;
+```
+
+This depends on a site-specific message format and only sees releases that go
+through the release-PR path — a direct version-bump commit carries no links.
+Treat it as a way to make history queryable, never as the contract: prefer
+`image_ref` for everything collected from now on.
+
+## What not to do
+
+Do not introduce a `service_id`, a service column, or a hand-maintained mapping
+of app names to repositories to paper over a missing join. Correlation stays
+identifier-based: commit SHA between Bitbucket and CI, image reference between
+CI and Argo CD.

docs/setup-argocd-notification.md

@@ -95,8 +95,9 @@ This adds:
   `revision`, `sync_status`, `operation_phase`, `started_at`, `finished_at`,
   `destination_namespace`, and `images` (a JSON array rendered from
   `.app.status.summary.images`). `images` is the bridge for joining Argo CD
-  events to pipeline events: `revision` is the GitOps-repo SHA, but image
-  tags typically embed the App-repo commit SHA that the pipeline reports.
+  events to pipeline events: `revision` is the GitOps-repo SHA, but the image
+  references match `pipeline_events.image_ref` verbatim, and the pipeline row
+  carries the App-repo `commit_sha`.
 - `trigger.on-deployed` and `trigger.on-sync-failed` (riptide-flavored).
   We do **not** ship `on-sync-succeeded`: every ArgoCD reconciliation
   enters a brief `Succeeded` operationState, which would flood the
@@ -237,6 +238,8 @@ ORDER BY created_at DESC LIMIT 5;
 
 `revision` is the **GitOps-repo SHA**, not the App-repo SHA — direct joins
 to `pipeline_events.commit_sha` or `bitbucket_events.commit_sha` will not
-match. The App-repo SHA is typically embedded in the image tag (e.g.
-`registry/app:abc1234`); a future reader/correlator pulls SHAs out of
-`payload->'images'` to bridge to pipeline events.
+match. Nor is the image tag a commit SHA: in practice it is a version
+(`registry/app:2.0.41`). The reliable bridge is the full image reference —
+`payload->'images'` against `pipeline_events.image_ref`, which the CI sender
+reports — and the pipeline row then carries the App-repo `commit_sha`. See
+[Correlating deploys back to commits](correlating-deploys-to-commits.md).