From c02dcc5add5e6224a62087a8b8cc771ab4772609 Mon Sep 17 00:00:00 2001 From: trick77 Date: Tue, 8 Sep 2026 14:13:41 +0200 Subject: [PATCH 1/5] Fix bot detection and deploy-to-commit correlation Both found by measuring 3.5 months of production events against the documented metrics. Bot detection matched only the login handle, so a review bot provisioned as an ordinary user account counted as a human reviewer and pulled the DX Core 4 pickup time down to seconds. detect_automation_source now matches the configured authors against the display name too, the parser carries it, and it is stored as bitbucket_events.author_display_name. Noergler can also self-report the account it posts under (reviewer_handle), so an installation does not have to configure the handle at all. Argo CD correlation never worked as documented: image tags are versions, not commit SHAs (0 of 4936 image refs in production carried a SHA), and argocd_events.revision is the GitOps-repo SHA, matching no pipeline row. The contract is now the full image reference: senders report pipeline_events.image_ref, Argo CD stores the same strings in payload->'images', and the join is exact. docs/correlating-deploys-to-commits.md documents it plus a read-time fallback for events collected before senders reported it. Also: PR size comes from noergler rollups, since Bitbucket DC webhooks carry no diff stats and the bitbucket_events columns are always NULL; the noergler setup doc still described the pre-0002 per-run payload. --- AGENTS.md | 4 +- README.md | 17 ++-- docs/correlating-deploys-to-commits.md | 77 +++++++++++++++++++ docs/setup-argocd-notification.md | 13 ++-- docs/setup-jenkins-notification.md | 15 +++- docs/setup-noergler-notification.md | 72 +++++++++++------ docs/setup-tekton-pipeline.md | 12 ++- .../versions/0003_metric_join_columns.py | 68 ++++++++++++++++ src/riptide_collector/config.py | 22 +++++- src/riptide_collector/models.py | 13 ++++ src/riptide_collector/parsers_bitbucket.py | 22 +++++- src/riptide_collector/routers/bitbucket.py | 7 +- src/riptide_collector/routers/noergler.py | 3 + src/riptide_collector/routers/pipeline.py | 1 + src/riptide_collector/schemas/argocd.py | 8 +- src/riptide_collector/schemas/noergler.py | 10 +++ src/riptide_collector/schemas/pipeline.py | 10 +++ tests/test_config.py | 32 ++++++++ tests/test_parsers_bitbucket.py | 50 ++++++++++++ tests/test_webhooks.py | 25 ++++++ tests/test_webhooks_noergler.py | 25 ++++++ 21 files changed, 456 insertions(+), 50 deletions(-) create mode 100644 docs/correlating-deploys-to-commits.md create mode 100644 migrations/versions/0003_metric_join_columns.py diff --git a/AGENTS.md b/AGENTS.md index 17fe5db..01a621b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -27,8 +27,8 @@ 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#` and `feedback##`. - **Senders verify reachability + bearer at startup via `GET /auth/ping`** — authenticated, returns `{"status":"ok","team":""}`, so a wrong token fails fast. Never reuse `/health` (unauth liveness) or `/ready` (unauth readiness) for it; those answer different questions. diff --git a/README.md b/README.md index 06f6440..c2f42c5 100644 --- a/README.md +++ b/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; noergler additionally self-reports its account as `noergler_events.reviewer_handle`. | | **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. | @@ -130,7 +130,7 @@ 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` = `'#'`. 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. | @@ -178,9 +178,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) diff --git a/docs/correlating-deploys-to-commits.md b/docs/correlating-deploys-to-commits.md new file mode 100644 index 0000000..b0768e4 --- /dev/null +++ b/docs/correlating-deploys-to-commits.md @@ -0,0 +1,77 @@ +# 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 +where a.operation_phase = 'Succeeded'; +``` + +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. diff --git a/docs/setup-argocd-notification.md b/docs/setup-argocd-notification.md index d347538..dd2dcc4 100644 --- a/docs/setup-argocd-notification.md +++ b/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). diff --git a/docs/setup-jenkins-notification.md b/docs/setup-jenkins-notification.md index 3b9585a..e15c94b 100644 --- a/docs/setup-jenkins-notification.md +++ b/docs/setup-jenkins-notification.md @@ -17,14 +17,22 @@ mandatory** — without them the metrics break. "phase": "COMPLETED", "status": "", "commit_sha": "", + "image_ref": "", "started_at": "", "finished_at": "" } ``` The `team` column is populated from the bearer token, not the payload. -Cross-source joins (to Bitbucket / ArgoCD / Noergler) use `commit_sha`; -per-pipeline aggregations use `pipeline_name`. +Cross-source joins (to Bitbucket / Noergler) use `commit_sha`; per-pipeline +aggregations use `pipeline_name`. + +`image_ref` is optional but send it whenever the run publishes an image: it is +the only reliable link from an Argo CD deploy back to the commit, because +`argocd_events.revision` is the GitOps-repo SHA and image tags are versions +rather than SHAs. Send the **full** reference, exactly as pushed +(`registry.example.com/team/app:2.0.41`), so it matches the strings Argo CD +reports. See [Correlating deploys back to commits](correlating-deploys-to-commits.md). ## Per-team token @@ -82,6 +90,9 @@ def riptideNotify(String phase) { phase: phase, status: currentBuild.currentResult ?: 'SUCCESS', commit_sha: env.GIT_COMMIT, + // Full reference of the image this run pushed, or null. Links the + // deploy back to this build — Argo CD reports the same string. + image_ref: env.RIPTIDE_IMAGE_REF ?: null, started_at: new Date(started).format("yyyy-MM-dd'T'HH:mm:ss'Z'", TimeZone.getTimeZone('UTC')), finished_at: phase == 'COMPLETED' ? new Date(finished).format("yyyy-MM-dd'T'HH:mm:ss'Z'", TimeZone.getTimeZone('UTC')) diff --git a/docs/setup-noergler-notification.md b/docs/setup-noergler-notification.md index 0982104..723d69d 100644 --- a/docs/setup-noergler-notification.md +++ b/docs/setup-noergler-notification.md @@ -8,7 +8,7 @@ lifecycle — PR open/merge/close already comes in via Bitbucket): | Event | When | Carries | |---|---|---| -| `completed` | After an LLM review run finishes | model, token counts, elapsed time, cost (finops) | +| `pr_completed` | Once per PR, when it reaches a terminal outcome (merged / declined / deleted) | outcome, final diff size, aggregated token counts, elapsed time, cost, models used (finops) | | `feedback` | When a reviewer disagrees with or acknowledges a finding | finding id, verdict, actor (reviewer-precision) | Lead-time, activity, and other PR-lifecycle metrics are **not** emitted by @@ -47,28 +47,46 @@ noergler verifies reachability and bearer validity at startup via ## Payloads -### `completed` +### `pr_completed` ```json { - "event_type": "completed", + "event_type": "pr_completed", + "outcome": "merged", "pr_key": "PROJ/payments-api#42", "repo": "acme/payments-api", - "commit_sha": "", - "run_id": "", - "model": "gpt-4o-2024-08-06", - "prompt_tokens": 12345, - "completion_tokens": 678, - "elapsed_ms": 8200, - "findings_count": 3, - "cost_usd": "0.124500", - "finished_at": "2026-04-29T18:01:00Z" + "reviewer_handle": "riptide-reviewer", + "source_commit_sha": "", + "merge_commit_sha": "", + "lines_added": 320, + "lines_removed": 75, + "files_changed": 12, + "total_runs": 3, + "total_prompt_tokens": 38420, + "total_completion_tokens": 2110, + "total_elapsed_ms": 24800, + "total_findings_count": 7, + "total_cost_usd": "0.382100", + "models_used": ["gpt-4o-2024-08-06"], + "first_review_at": "2026-04-29T17:30:00Z", + "closed_at": "2026-04-29T18:42:00Z" } ``` -`run_id` alone is the idempotency key — noergler may safely retry. The -`commit_sha` enables joins to `bitbucket_events`, `pipeline_events`, and -`argocd_events` for cost-vs-deployment analysis. +One rollup per PR: `(pr_key, outcome)` is the idempotency key, so noergler may +safely retry. `outcome` is `merged`, `declined` or `deleted` — only merged PRs +shipped, so throughput and DORA queries filter on it, while FinOps keeps all +three to see review spend on code that never landed. `source_commit_sha` and +`merge_commit_sha` join to `bitbucket_events` and `pipeline_events` for +cost-vs-deployment analysis. `pr_key` (`#`) joins to +`bitbucket_events (repo_full_name, pr_id)` — that is also where riptide gets PR +diff sizes from, since Bitbucket's webhooks carry none. + +`reviewer_handle` is optional but recommended: it is the account noergler posts +its review comments under on the git host. Reporting it lets riptide recognise +those comments as automation without every installation adding the handle to its +`automation` config; unrecognised, the bot counts as a human reviewer and drives +the code-review pickup-time metric toward zero. ### `feedback` @@ -91,20 +109,26 @@ Idempotency key is `(finding_id, verdict)`. ## Verify ```sql --- finops: cost-by-model, last 7 days -SELECT model, +-- finops: cost by model, last 7 days +SELECT m AS model, SUM(prompt_tokens + completion_tokens) AS tokens, SUM(cost_usd) AS spend, - COUNT(*) AS runs -FROM noergler_events -WHERE event_type = 'completed' AND created_at > now() - interval '7 days' -GROUP BY model + COUNT(*) AS prs +FROM noergler_events, unnest(models_used) AS m +WHERE event_type = 'pr_completed' AND created_at > now() - interval '7 days' +GROUP BY 1 ORDER BY spend DESC; --- reviewer precision: 1 - disagreed/total, last 7 days +-- review spend that never shipped, last 7 days +SELECT outcome, COUNT(*) AS prs, SUM(cost_usd) AS spend +FROM noergler_events +WHERE event_type = 'pr_completed' AND created_at > now() - interval '7 days' +GROUP BY 1; + +-- reviewer precision: 1 - disagreed PRs / reviewed PRs, last 7 days SELECT 1.0 - ( - SUM(CASE WHEN event_type='feedback' AND verdict='disagreed' THEN 1 ELSE 0 END) - / NULLIF(SUM(CASE WHEN event_type='completed' THEN findings_count ELSE 0 END), 0) + COUNT(*) FILTER (WHERE event_type = 'feedback' AND verdict = 'disagreed')::numeric + / NULLIF(COUNT(*) FILTER (WHERE event_type = 'pr_completed'), 0) ) AS precision_estimate FROM noergler_events WHERE created_at > now() - interval '7 days'; diff --git a/docs/setup-tekton-pipeline.md b/docs/setup-tekton-pipeline.md index 364ce53..0d1fbe9 100644 --- a/docs/setup-tekton-pipeline.md +++ b/docs/setup-tekton-pipeline.md @@ -31,6 +31,12 @@ spec: description: $(tasks.status) from the calling Pipeline (Succeeded/Failed/Completed) - name: commit-sha description: git SHA built / deployed + - name: image-ref + description: >- + full reference of the image this run published + (registry/path/app:tag), or "" if it published none. Links the Argo CD + deploy back to this build; see docs/correlating-deploys-to-commits.md + default: "" - name: started-at description: PipelineRun status.startTime (ISO 8601 UTC) - name: finished-at @@ -63,10 +69,12 @@ spec: --arg phase "COMPLETED" \ --arg st "$STATUS" \ --arg sha "$(params.commit-sha)" \ + --arg img "$(params.image-ref)" \ --arg sa "$(params.started-at)" \ --arg fa "$(params.finished-at)" \ '{source:$src, pipeline_name:$pn, run_id:$rid, phase:$phase, - status:$st, commit_sha:$sha, started_at:$sa, finished_at:$fa}') + status:$st, commit_sha:$sha, started_at:$sa, finished_at:$fa} + + (if $img == "" then {} else {image_ref:$img} end)') # --connect-timeout caps TCP/TLS handshake; --max-time caps the full # request. One quick retry handles transient blips. Any non-2xx is # logged and ignored — the PipelineRun result is unaffected. @@ -138,6 +146,8 @@ spec: value: $(tasks.status) - name: commit-sha value: $(params.commit-sha) + - name: image-ref + value: $(tasks.build.results.image-ref) - name: started-at value: $(context.pipelineRun.startTime) - name: finished-at diff --git a/migrations/versions/0003_metric_join_columns.py b/migrations/versions/0003_metric_join_columns.py new file mode 100644 index 0000000..e7a807d --- /dev/null +++ b/migrations/versions/0003_metric_join_columns.py @@ -0,0 +1,68 @@ +"""columns that make bot detection and deploy→commit correlation work + +Three additive nullable columns, all driven by what production data showed: + +- `bitbucket_events.author_display_name`: a review bot posting under an + ordinary-looking login is only recognisable by its display name; without + it the bot counts as a human reviewer and collapses pickup time. +- `pipeline_events.image_ref`: the full image reference the run published. + Argo CD stores exactly these strings in `payload->'images'`, so this is + the exact join from a deploy back to the build and its commit — image + tags are versions, not commit SHAs. +- `noergler_events.reviewer_handle`: the reviewer's own account handle, + self-reported, so automation identity comes from the stream rather than + per-installation configuration. + +Revision ID: 0003 +Revises: 0002 +Create Date: 2026-09-08 + +""" + +from collections.abc import Sequence + +import sqlalchemy as sa +from alembic import op + +revision: str = "0003" +down_revision: str | None = "0002" +branch_labels: str | Sequence[str] | None = None +depends_on: str | Sequence[str] | None = None + + +def upgrade() -> None: + op.add_column( + "bitbucket_events", + sa.Column( + "author_display_name", + sa.String, + nullable=True, + comment="actor.displayName; bot accounts often identify themselves only here", + ), + ) + op.add_column( + "pipeline_events", + sa.Column( + "image_ref", + sa.String, + nullable=True, + comment="full image reference published by the run; joins to argocd payload->'images'", + ), + ) + op.add_column( + "noergler_events", + sa.Column( + "reviewer_handle", + sa.String, + nullable=True, + comment="git-host account the reviewer posts under; self-reported automation identity", + ), + ) + op.create_index("ix_pipeline_events_image_ref", "pipeline_events", ["image_ref"]) + + +def downgrade() -> None: + op.drop_index("ix_pipeline_events_image_ref", table_name="pipeline_events") + op.drop_column("noergler_events", "reviewer_handle") + op.drop_column("pipeline_events", "image_ref") + op.drop_column("bitbucket_events", "author_display_name") diff --git a/src/riptide_collector/config.py b/src/riptide_collector/config.py index 7ec0d11..e595c1d 100644 --- a/src/riptide_collector/config.py +++ b/src/riptide_collector/config.py @@ -207,17 +207,31 @@ def team(self, name: str | None) -> Team | None: return None return self._config.teams_by_name.get(name) - def detect_automation_source(self, author: str | None, branch_name: str | None) -> str | None: + def detect_automation_source( + self, + author: str | None, + branch_name: str | None, + author_display_name: str | None = None, + ) -> str | None: + """Match an event's author against the configured automation sources. + + `authors` entries are matched against both the login handle and the + display name: a bot provisioned as an ordinary user account (nondescript + 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. + """ config = self._config - if author: + handles = [name for name in (author, author_display_name) if name] + for handle in handles: for source in config.automation: - if author in source.authors: + if handle in source.authors: return source.name if branch_name: for source in config.automation: for prefix in source.branch_prefixes: if branch_name.startswith(prefix): return source.name - if looks_bot_shaped(author): + if any(looks_bot_shaped(handle) for handle in handles): return "other-bot" return None diff --git a/src/riptide_collector/models.py b/src/riptide_collector/models.py index dc58fe2..7e2c7ec 100644 --- a/src/riptide_collector/models.py +++ b/src/riptide_collector/models.py @@ -43,6 +43,10 @@ class BitbucketEvent(Base): pr_id: Mapped[int | None] = mapped_column(Integer, nullable=True) commit_sha: Mapped[str | None] = mapped_column(String, nullable=True) author: Mapped[str | None] = mapped_column(String, nullable=True) + # `actor.displayName` next to the login handle: bot accounts are often + # provisioned with a nondescript login and only identify themselves here, + # so read-time queries filtering bots need both. + author_display_name: Mapped[str | None] = mapped_column(String, nullable=True) branch_name: Mapped[str | None] = mapped_column(String, nullable=True) change_type: Mapped[str | None] = mapped_column(String, nullable=True) jira_keys: Mapped[list[str]] = mapped_column(ARRAY(String), nullable=False, server_default="{}") @@ -87,6 +91,11 @@ class PipelineEvent(Base): phase: Mapped[str] = mapped_column(String, nullable=False) status: Mapped[str | None] = mapped_column(String, nullable=True) commit_sha: Mapped[str | None] = mapped_column(String, nullable=True) + # Full image reference the run published, e.g. `registry/path/app:2.0.41`. + # Argo CD stores exactly these strings in `payload->'images'`, which makes + # `argocd_events → pipeline_events → commit_sha` an exact join even when the + # image tag is a version rather than a commit SHA. + image_ref: Mapped[str | None] = mapped_column(String, nullable=True) started_at: Mapped[datetime | None] = mapped_column(DateTime(timezone=True), nullable=True) finished_at: Mapped[datetime | None] = mapped_column(DateTime(timezone=True), nullable=True) duration_seconds: Mapped[int | None] = mapped_column( @@ -129,6 +138,10 @@ class NoerglerEvent(Base): commit_sha: Mapped[str | None] = mapped_column(String, nullable=True) # pr_completed-only: outcome: Mapped[str | None] = mapped_column(String, nullable=True) + # The reviewer's own account handle on the git host. Self-reported so + # riptide learns the bot's identity from the stream instead of every + # installation configuring it by hand. + reviewer_handle: Mapped[str | None] = mapped_column(String, nullable=True) merge_commit_sha: Mapped[str | None] = mapped_column(String, nullable=True) lines_added: Mapped[int | None] = mapped_column(Integer, nullable=True) lines_removed: Mapped[int | None] = mapped_column(Integer, nullable=True) diff --git a/src/riptide_collector/parsers_bitbucket.py b/src/riptide_collector/parsers_bitbucket.py index 040c42e..0b456af 100644 --- a/src/riptide_collector/parsers_bitbucket.py +++ b/src/riptide_collector/parsers_bitbucket.py @@ -61,6 +61,10 @@ class BitbucketEventDraft: pr_id: int | None commit_sha: str | None author: str | None + # The same user's `displayName`, kept alongside the login so bot + # 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 branch_name: str | None change_type: str | None jira_keys: list[str] @@ -136,6 +140,17 @@ def _user_handle(user: dict[str, Any]) -> str | None: return None +def _user_display_name(user: dict[str, Any]) -> str | None: + """The human-readable name of a BBS DC user, if it differs from the handle. + + A bot account is often provisioned with a nondescript login and only + identifies itself through `displayName`, so this travels with the + author for automation detection. + """ + value = user.get("displayName") + return value if isinstance(value, str) and value else None + + def _synth_delivery_id(event_key: str | None, body: dict[str, Any]) -> str: pr = _as_dict(body.get("pullRequest")) pr_id = pr.get("id") @@ -193,6 +208,7 @@ def extract_event( branch_name: str | None = None commit_sha: str | None = None author: str | None = None + author_display_name: str | None = None is_revert = False # Reviewer-activity events carry the actor (the reviewer / commenter) @@ -218,6 +234,7 @@ def extract_event( if not is_actor_authored: author_user = _as_dict(_as_dict(pr.get("author")).get("user")) author = _user_handle(author_user) + author_display_name = _user_display_name(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. @@ -259,7 +276,9 @@ def extract_event( ) if not author: - author = _user_handle(_as_dict(body.get("actor"))) + actor = _as_dict(body.get("actor")) + author = _user_handle(actor) + author_display_name = _user_display_name(actor) repo_full_name = lower(raw_repo_full_name) branch_name = lower(branch_name) @@ -272,6 +291,7 @@ def extract_event( pr_id=pr_id, commit_sha=commit_sha, author=author, + author_display_name=author_display_name, 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 6495720..0f9e684 100644 --- a/src/riptide_collector/routers/bitbucket.py +++ b/src/riptide_collector/routers/bitbucket.py @@ -71,7 +71,11 @@ async def bitbucket_webhook( # pyright: ignore[reportUnusedFunction] # this assert turns into a typing error rather than a silent bug. assert isinstance(parsed, BitbucketEventDraft) draft = parsed - automation_source = config.detect_automation_source(draft.author, draft.branch_name) + automation_source = config.detect_automation_source( + draft.author, + draft.branch_name, + draft.author_display_name, + ) try: async with session_factory() as session: @@ -84,6 +88,7 @@ async def bitbucket_webhook( # pyright: ignore[reportUnusedFunction] pr_id=draft.pr_id, commit_sha=draft.commit_sha, author=draft.author, + author_display_name=draft.author_display_name, branch_name=draft.branch_name, change_type=draft.change_type, jira_keys=draft.jira_keys, diff --git a/src/riptide_collector/routers/noergler.py b/src/riptide_collector/routers/noergler.py index 84428c0..d3c608c 100644 --- a/src/riptide_collector/routers/noergler.py +++ b/src/riptide_collector/routers/noergler.py @@ -86,6 +86,9 @@ def _values_pr_completed( "outcome": event.outcome, "pr_key": lower(event.pr_key), "repo": lower(event.repo), + # Kept case-preserved: it is matched against the git host's own + # user handles, which riptide stores as delivered. + "reviewer_handle": event.reviewer_handle, "commit_sha": lower(event.source_commit_sha), "merge_commit_sha": lower(event.merge_commit_sha) if event.merge_commit_sha else None, "lines_added": event.lines_added, diff --git a/src/riptide_collector/routers/pipeline.py b/src/riptide_collector/routers/pipeline.py index c3e1ec6..abbae22 100644 --- a/src/riptide_collector/routers/pipeline.py +++ b/src/riptide_collector/routers/pipeline.py @@ -49,6 +49,7 @@ async def pipeline_webhook( # pyright: ignore[reportUnusedFunction] phase=event.phase, status=event.status, commit_sha=commit_sha, + image_ref=event.image_ref, started_at=event.started_at, finished_at=event.finished_at, occurred_at=event.finished_at or event.started_at or datetime.now(UTC), diff --git a/src/riptide_collector/schemas/argocd.py b/src/riptide_collector/schemas/argocd.py index 7db53ce..2ccaaef 100644 --- a/src/riptide_collector/schemas/argocd.py +++ b/src/riptide_collector/schemas/argocd.py @@ -23,9 +23,11 @@ class ArgoCDWebhook(BaseModel): description=( "rendered images from the synced manifests (Argo CD field path " "`.app.status.summary.images`). may be empty for apps without pods. " - "the App-Repo commit SHA is typically embedded in the image tag and " - "bridges argocd.revision (GitOps-repo SHA) to pipeline.commit_sha " - "(App-repo SHA) — correlation logic is built on top of this field." + "these are full image references (`registry/path:tag`) and they are " + "the bridge from a deploy to the build that produced it: they match " + "pipeline_events.image_ref verbatim. do not parse a commit SHA out of " + "the tag — tags are usually versions. argocd.revision is the " + "GitOps-repo SHA and never matches pipeline.commit_sha." ), ) diff --git a/src/riptide_collector/schemas/noergler.py b/src/riptide_collector/schemas/noergler.py index f1d5055..c3fbd6e 100644 --- a/src/riptide_collector/schemas/noergler.py +++ b/src/riptide_collector/schemas/noergler.py @@ -42,6 +42,16 @@ class NoerglerPrCompleted(_Common): ) pr_key: str = Field(..., min_length=1, description="Bitbucket PR key, e.g. 'PROJ/repo#42'") repo: str = Field(..., min_length=1) + reviewer_handle: str | None = Field( + default=None, + min_length=1, + description=( + "The account the reviewer posts its comments under on the git host. " + "Self-reporting it lets riptide recognise the reviewer's own comments as " + "automation without every installation configuring the handle by hand — " + "otherwise the bot counts as a human reviewer and collapses review-pickup time." + ), + ) source_commit_sha: str = Field( ..., min_length=7, diff --git a/src/riptide_collector/schemas/pipeline.py b/src/riptide_collector/schemas/pipeline.py index 457fc6f..3a62423 100644 --- a/src/riptide_collector/schemas/pipeline.py +++ b/src/riptide_collector/schemas/pipeline.py @@ -36,6 +36,16 @@ class PipelineWebhook(BaseModel): phase: str = Field(..., min_length=1, description="STARTED / COMPLETED / FINALIZED") status: str | None = Field(default=None, description="SUCCESS / FAILURE / etc.") commit_sha: str = Field(..., min_length=7, description="git commit SHA being built") + image_ref: str | None = Field( + default=None, + min_length=1, + description=( + "full image reference the run published, e.g. 'registry/path/app:2.0.41'. " + "Argo CD reports the same strings in its rendered image list, so sending it " + "makes deploy → build → commit an exact join even when the tag is a version " + "rather than a commit SHA. Omit it for runs that publish no image." + ), + ) started_at: datetime finished_at: datetime | None = None diff --git a/tests/test_config.py b/tests/test_config.py index b301450..96b9f36 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -199,3 +199,35 @@ def test_rejects_production_stage_in_ignored_list(self, tmp_path: Path) -> None: path = _write(tmp_path / "c.json", data) with pytest.raises(RiptideConfigError, match="production_stage"): load_config_from_path(path) + + +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 + # short handle like any human's, and only `displayName` says what it + # is. Matching the login alone lets it count as a human reviewer, + # which drives the DX Core 4 pickup-time metric toward zero. + 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("rop", None, "noergler") == "noergler" + + def test_bot_shaped_display_name_falls_back_to_other_bot(self, tmp_path: Path) -> None: + path = _write(tmp_path / "c.json", VALID) + store = RiptideConfigStore(path) + + assert store.detect_automation_source("svc01", None, "release-bot") == "other-bot" + + def test_human_display_name_stays_human(self, tmp_path: Path) -> None: + path = _write(tmp_path / "c.json", VALID) + store = RiptideConfigStore(path) + + assert store.detect_automation_source("alice", "feature/x", "Alice Example") is None + + def test_login_match_still_wins_without_display_name(self, tmp_path: Path) -> None: + path = _write(tmp_path / "c.json", VALID) + store = RiptideConfigStore(path) + + assert store.detect_automation_source("renovate-bot", None) == "renovate" diff --git a/tests/test_parsers_bitbucket.py b/tests/test_parsers_bitbucket.py index 33e932b..90a3fe8 100644 --- a/tests/test_parsers_bitbucket.py +++ b/tests/test_parsers_bitbucket.py @@ -450,3 +450,53 @@ def test_occurred_at_parsed_from_iso_string(self) -> None: assert isinstance(result, BitbucketEventDraft) assert result.occurred_at.year == 2026 assert result.occurred_at.tzinfo is not None + + +class TestAuthorDisplayName: + def test_actor_display_name_travels_with_reviewer_events(self) -> None: + # Given a reviewer-activity event, where the actor is the author + body = _load("bitbucket_pr_comment_added.json") + + # When + result = extract_event( + body, + x_event_key="pr:comment:added", + x_request_uuid="r", + x_hook_uuid=None, + ) + + # Then the display name is carried alongside the login handle, so a + # bot posting under an ordinary-looking login can still be detected. + assert isinstance(result, BitbucketEventDraft) + assert result.author == "bob" + assert result.author_display_name == "Bob Reviewer" + + def test_pr_author_display_name_used_for_non_reviewer_events(self) -> None: + # Given a PR lifecycle event, where the PR opener is the author + body = _load("bitbucket_pr_merged.json") + body["pullRequest"]["author"]["user"]["displayName"] = "Alice Example" + + # When + result = extract_event(body, x_event_key="pr:merged", x_request_uuid="r", x_hook_uuid=None) + + # Then + assert isinstance(result, BitbucketEventDraft) + assert result.author == "alice" + assert result.author_display_name == "Alice Example" + + def test_missing_display_name_is_none(self) -> None: + # Given + body = _load("bitbucket_pr_comment_added.json") + del body["actor"]["displayName"] + + # When + result = extract_event( + body, + x_event_key="pr:comment:added", + x_request_uuid="r", + x_hook_uuid=None, + ) + + # Then + assert isinstance(result, BitbucketEventDraft) + assert result.author_display_name is None diff --git a/tests/test_webhooks.py b/tests/test_webhooks.py index 8afdb3f..e36a686 100644 --- a/tests/test_webhooks.py +++ b/tests/test_webhooks.py @@ -224,6 +224,31 @@ async def test_tekton_payload_inserted(self, client: AsyncClient) -> None: assert row.run_id == "payments-api-deploy-7gx2k" assert row.pipeline_name == "payments-api-deploy" + async def test_image_ref_persisted(self, client: AsyncClient) -> None: + # The full image reference is what Argo CD reports in its rendered + # image list, so storing it verbatim makes deploy → build → commit an + # exact join even when the tag is a version instead of a commit SHA. + payload = _load("pipeline_jenkins_completed.json") + payload["image_ref"] = "registry.example.com/acme/payments-api:2.0.41" + response = await client.post("/webhooks/pipeline", json=payload, headers=PIPELINE_AUTH) + assert response.status_code == 202 + + factory = TestBitbucketWebhook._fresh_session_factory(client) + async with factory() as session: + row = (await session.execute(select(PipelineEvent))).scalar_one() + assert row.image_ref == "registry.example.com/acme/payments-api:2.0.41" + + async def test_image_ref_optional(self, client: AsyncClient) -> None: + # Senders that publish no image (test-only runs) stay valid. + payload = _load("pipeline_jenkins_completed.json") + response = await client.post("/webhooks/pipeline", json=payload, headers=PIPELINE_AUTH) + assert response.status_code == 202 + + factory = TestBitbucketWebhook._fresh_session_factory(client) + async with factory() as session: + row = (await session.execute(select(PipelineEvent))).scalar_one() + assert row.image_ref is None + async def test_uppercase_commit_sha_normalised_to_lowercase(self, client: AsyncClient) -> None: payload = _load("pipeline_jenkins_completed.json") payload["commit_sha"] = payload["commit_sha"].upper() diff --git a/tests/test_webhooks_noergler.py b/tests/test_webhooks_noergler.py index d08526e..d90e9f7 100644 --- a/tests/test_webhooks_noergler.py +++ b/tests/test_webhooks_noergler.py @@ -196,6 +196,31 @@ async def test_delivery_id_dedupes_across_pr_key_casing(self, client: AsyncClien assert len(rows) == 1 +class TestReviewerHandle: + async def test_reviewer_handle_persisted_case_preserved(self, client: AsyncClient) -> None: + # Self-reported so riptide can recognise the reviewer's own PR + # comments as automation without every installation configuring the + # handle by hand. Stored as delivered — it is matched against the git + # host's user handles, which riptide does not lowercase either. + payload = _load("noergler_pr_completed_merged.json") + payload["reviewer_handle"] = "Rop" + r = await client.post("/webhooks/noergler", json=payload, headers=AUTH) + assert r.status_code == 202 + + async with _fresh_session_factory(client)() as session: + row = (await session.execute(select(NoerglerEvent))).scalar_one() + assert row.reviewer_handle == "Rop" + + async def test_reviewer_handle_optional(self, client: AsyncClient) -> None: + payload = _load("noergler_pr_completed_merged.json") + r = await client.post("/webhooks/noergler", json=payload, headers=AUTH) + assert r.status_code == 202 + + async with _fresh_session_factory(client)() as session: + row = (await session.execute(select(NoerglerEvent))).scalar_one() + assert row.reviewer_handle is None + + class TestPrCompletedNonMerged: async def test_declined_recorded_with_outcome(self, client: AsyncClient) -> None: payload = _load("noergler_pr_completed_declined.json") From e46a51b4dc949f7fd69f3df9abf45f32e74f9daa Mon Sep 17 00:00:00 2001 From: trick77 Date: Tue, 8 Sep 2026 14:32:40 +0200 Subject: [PATCH 2/5] Address review findings on the metric-join changes - Tekton: pass image-ref as a Pipeline param. A finally task consuming an uninitialized task result is skipped, so a failed build would have stopped notifying riptide entirely, losing the rows the build success rate needs. - reviewer_handle is now actually read: the pickup-time query in the README derives a bot_identities set from it, which is what fixes historical rows that were tagged human at ingest. - Declare ix_pipeline_events_image_ref on the model too, so metadata-created schemas match the migration. - Filter the deploy-to-commit join to COMPLETED/SUCCESS runs; a sender that reports the image on every phase would otherwise fan each deploy out. - Reviewer precision: findings on both sides of the ratio. Counting disagreements against PRs can go negative. - Accept an empty image_ref / reviewer_handle as null instead of 422: an unset template param arrives as "", and rejecting it dropped the whole event over an optional field. - Match automation handles case-insensitively; a display name is human-formatted and a case-only miss reproduces the bug being fixed. - total_cost_usd is optional: an unpriced model must not cost the whole PR rollup. Senders omit the cost instead of dropping the event or sending 0. - AGENTS.md: noergler dedup keys were still the pre-0002 per-run ones. --- AGENTS.md | 2 +- README.md | 16 ++++++++--- docs/correlating-deploys-to-commits.md | 7 ++++- docs/setup-noergler-notification.md | 23 +++++++++++----- docs/setup-tekton-pipeline.md | 9 ++++++- src/riptide_collector/config.py | 8 ++++-- src/riptide_collector/models.py | 1 + src/riptide_collector/schemas/noergler.py | 20 ++++++++++++-- src/riptide_collector/schemas/pipeline.py | 12 +++++++-- tests/test_config.py | 10 +++++++ tests/test_webhooks.py | 13 +++++++++ tests/test_webhooks_noergler.py | 33 +++++++++++++++++++++++ 12 files changed, 135 insertions(+), 19 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 01a621b..914aea3 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -30,7 +30,7 @@ If `docker ps` fails, ask the user to start OrbStack. - **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#` and `feedback##`. +- **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##` and `feedback##`. `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":""}`, 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/`. diff --git a/README.md b/README.md index c2f42c5..2a80d57 100644 --- a/README.md +++ b/README.md @@ -63,7 +63,7 @@ the data captured in v1. | **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 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; noergler additionally self-reports its account as `noergler_events.reviewer_handle`. | +| **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 `bot_identities` CTE in the query below filters those at read time from the handles noergler self-reports (`noergler_events.reviewer_handle`). | | **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,16 @@ 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 bot_identities AS ( + -- Identities riptide learned from the stream rather than from config: + -- noergler reports the account it comments under. `is_automated` was + -- decided at ingest, so rows written before an identity was known are + -- filtered here instead. + SELECT DISTINCT lower(reviewer_handle) AS handle + FROM noergler_events + WHERE reviewer_handle IS NOT NULL +), +pickup_start AS ( SELECT repo_full_name, pr_id, @@ -109,6 +118,7 @@ WHERE ps.clock_start IS NOT NULL ) AND e.author IS DISTINCT FROM ps.pr_opener AND NOT e.is_automated + AND lower(e.author) NOT IN (SELECT handle FROM bot_identities) AND e.occurred_at >= ps.clock_start GROUP BY e.repo_full_name, e.pr_id, ps.clock_start; ``` @@ -138,7 +148,7 @@ because there's no clock-start to subtract from in the first place. | **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. | +| **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/docs/correlating-deploys-to-commits.md b/docs/correlating-deploys-to-commits.md index b0768e4..39ca54f 100644 --- a/docs/correlating-deploys-to-commits.md +++ b/docs/correlating-deploys-to-commits.md @@ -23,7 +23,12 @@ select a.app_name, a.environment, a.occurred_at as deployed_at, 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 -where a.operation_phase = 'Succeeded'; +-- 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 diff --git a/docs/setup-noergler-notification.md b/docs/setup-noergler-notification.md index 723d69d..ac3bea0 100644 --- a/docs/setup-noergler-notification.md +++ b/docs/setup-noergler-notification.md @@ -74,7 +74,11 @@ noergler verifies reachability and bearer validity at startup via ``` One rollup per PR: `(pr_key, outcome)` is the idempotency key, so noergler may -safely retry. `outcome` is `merged`, `declined` or `deleted` — only merged PRs +safely retry. `total_cost_usd` may be omitted when the sender cannot price the +run (unpriced model, gateway not reporting a cost header) — send no cost rather +than a `0`, and never drop the whole rollup: outcome, diff size, tokens and runs +still feed the delivery metrics, and a NULL cost makes the pricing gap visible +(`count(*) FILTER (WHERE cost_usd IS NULL)`). `outcome` is `merged`, `declined` or `deleted` — only merged PRs shipped, so throughput and DORA queries filter on it, while FinOps keeps all three to see review spend on code that never landed. `source_commit_sha` and `merge_commit_sha` join to `bitbucket_events` and `pipeline_events` for @@ -83,10 +87,13 @@ cost-vs-deployment analysis. `pr_key` (`#`) joins to diff sizes from, since Bitbucket's webhooks carry none. `reviewer_handle` is optional but recommended: it is the account noergler posts -its review comments under on the git host. Reporting it lets riptide recognise -those comments as automation without every installation adding the handle to its -`automation` config; unrecognised, the bot counts as a human reviewer and drives -the code-review pickup-time metric toward zero. +its review comments under on the git host. Reporting it lets riptide's read-time +queries recognise those comments as automation — see the `bot_identities` CTE in +the pickup-time query in the README — without every installation adding the +handle to its `automation` config. Unrecognised, the bot counts as a human +reviewer and drives the code-review pickup-time metric toward zero. Adding the +handle to `automation` as well is still worthwhile: that also tags new rows +`is_automated` at ingest. ### `feedback` @@ -125,10 +132,12 @@ FROM noergler_events WHERE event_type = 'pr_completed' AND created_at > now() - interval '7 days' GROUP BY 1; --- reviewer precision: 1 - disagreed PRs / reviewed PRs, last 7 days +-- reviewer precision: 1 - disagreed findings / reported findings, last 7 days. +-- Both sides count findings: a PR can collect several disagreements, so a +-- per-PR denominator can drive the estimate below zero. SELECT 1.0 - ( COUNT(*) FILTER (WHERE event_type = 'feedback' AND verdict = 'disagreed')::numeric - / NULLIF(COUNT(*) FILTER (WHERE event_type = 'pr_completed'), 0) + / NULLIF(SUM(findings_count) FILTER (WHERE event_type = 'pr_completed'), 0) ) AS precision_estimate FROM noergler_events WHERE created_at > now() - interval '7 days'; diff --git a/docs/setup-tekton-pipeline.md b/docs/setup-tekton-pipeline.md index 0d1fbe9..57b3bc8 100644 --- a/docs/setup-tekton-pipeline.md +++ b/docs/setup-tekton-pipeline.md @@ -121,6 +121,13 @@ metadata: spec: params: - name: commit-sha + # The image reference this run publishes. A Pipeline param, deliberately + # not `$(tasks.build.results.image-ref)`: Tekton *skips* a finally task + # that consumes an uninitialized result, so a failed build would stop + # notifying riptide altogether — losing exactly the rows the build + # success-rate metric needs. + - name: image-ref + default: "" tasks: - name: build taskRef: { name: build } @@ -147,7 +154,7 @@ spec: - name: commit-sha value: $(params.commit-sha) - name: image-ref - value: $(tasks.build.results.image-ref) + value: $(params.image-ref) - name: started-at value: $(context.pipelineRun.startTime) - name: finished-at diff --git a/src/riptide_collector/config.py b/src/riptide_collector/config.py index e595c1d..4d5ee55 100644 --- a/src/riptide_collector/config.py +++ b/src/riptide_collector/config.py @@ -223,9 +223,13 @@ def detect_automation_source( """ config = self._config handles = [name for name in (author, author_display_name) if name] - for handle in handles: + # Case-insensitive: login handles are usually lowercase, but display + # names are human-formatted ("Noergler" vs the configured "noergler"), + # and a silent near-miss reintroduces the bug this matching exists for. + folded = [handle.casefold() for handle in handles] + for handle in folded: for source in config.automation: - if handle in source.authors: + if any(handle == known.casefold() for known in source.authors): return source.name if branch_name: for source in config.automation: diff --git a/src/riptide_collector/models.py b/src/riptide_collector/models.py index 7e2c7ec..5503f6a 100644 --- a/src/riptide_collector/models.py +++ b/src/riptide_collector/models.py @@ -81,6 +81,7 @@ class PipelineEvent(Base): Index("ix_pipeline_events_source", "source"), Index("ix_pipeline_events_pipeline_name", "pipeline_name"), Index("ix_pipeline_events_commit_sha", "commit_sha"), + Index("ix_pipeline_events_image_ref", "image_ref"), ) id: Mapped[int] = mapped_column(BigInteger, primary_key=True, autoincrement=True) diff --git a/src/riptide_collector/schemas/noergler.py b/src/riptide_collector/schemas/noergler.py index c3fbd6e..e508132 100644 --- a/src/riptide_collector/schemas/noergler.py +++ b/src/riptide_collector/schemas/noergler.py @@ -44,7 +44,6 @@ class NoerglerPrCompleted(_Common): repo: str = Field(..., min_length=1) reviewer_handle: str | None = Field( default=None, - min_length=1, description=( "The account the reviewer posts its comments under on the git host. " "Self-reporting it lets riptide recognise the reviewer's own comments as " @@ -70,7 +69,17 @@ class NoerglerPrCompleted(_Common): total_completion_tokens: int = Field(..., ge=0) total_elapsed_ms: int = Field(..., ge=0) total_findings_count: int = Field(..., ge=0) - total_cost_usd: Decimal = Field(..., ge=0) + total_cost_usd: Decimal | None = Field( + default=None, + ge=0, + description=( + "Aggregated review cost. Omit it when the sender cannot price the run " + "(unpriced model, gateway not reporting cost) — never send 0, which " + "would silently understate FinOps. A rollup without cost still carries " + "the outcome, diff size, tokens and runs, so dropping the whole event " + "over a missing price would cost delivery metrics as well." + ), + ) models_used: list[str] = Field( ..., min_length=1, @@ -94,6 +103,13 @@ def _models_non_empty(cls, v: list[str]) -> list[str]: raise ValueError("models_used entries must be non-empty strings") return v + @field_validator("reviewer_handle") + @classmethod + def _empty_reviewer_handle_is_none(cls, v: str | None) -> str | None: + # An unset handle arrives as "" as often as it is omitted; rejecting + # it would cost the whole rollup over an optional field. + return v.strip() or None if v else None + @model_validator(mode="after") def _check_merge_commit_consistency(self) -> NoerglerPrCompleted: # Catch sender bugs: only merged PRs produce a merge commit, and a diff --git a/src/riptide_collector/schemas/pipeline.py b/src/riptide_collector/schemas/pipeline.py index 3a62423..759e4ca 100644 --- a/src/riptide_collector/schemas/pipeline.py +++ b/src/riptide_collector/schemas/pipeline.py @@ -38,12 +38,12 @@ class PipelineWebhook(BaseModel): commit_sha: str = Field(..., min_length=7, description="git commit SHA being built") image_ref: str | None = Field( default=None, - min_length=1, description=( "full image reference the run published, e.g. 'registry/path/app:2.0.41'. " "Argo CD reports the same strings in its rendered image list, so sending it " "makes deploy → build → commit an exact join even when the tag is a version " - "rather than a commit SHA. Omit it for runs that publish no image." + "rather than a commit SHA. Omit it (or send an empty string) for runs " + "that publish no image." ), ) started_at: datetime @@ -53,3 +53,11 @@ class PipelineWebhook(BaseModel): @classmethod def _normalise_tz(cls, v: datetime | None) -> datetime | None: return _to_utc(v) + + @field_validator("image_ref") + @classmethod + def _empty_image_ref_is_none(cls, v: str | None) -> str | None: + # Templating a param that wasn't set yields "" far more often than it + # yields an absent key. Rejecting that would drop the whole run — + # build duration and success/failure with it — over an optional field. + return v.strip() or None if v else None diff --git a/tests/test_config.py b/tests/test_config.py index 96b9f36..dd92576 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -214,6 +214,16 @@ def test_display_name_match_when_login_is_ordinary(self, tmp_path: Path) -> None assert store.detect_automation_source("rop", None, "noergler") == "noergler" + def test_display_name_match_is_case_insensitive(self, tmp_path: Path) -> None: + # Display names are human-formatted; a case-only mismatch against the + # configured handle would silently reproduce the zero-pickup-time bug. + 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("rop", None, "Noergler") == "noergler" + def test_bot_shaped_display_name_falls_back_to_other_bot(self, tmp_path: Path) -> None: path = _write(tmp_path / "c.json", VALID) store = RiptideConfigStore(path) diff --git a/tests/test_webhooks.py b/tests/test_webhooks.py index e36a686..f77b4a8 100644 --- a/tests/test_webhooks.py +++ b/tests/test_webhooks.py @@ -238,6 +238,19 @@ async def test_image_ref_persisted(self, client: AsyncClient) -> None: row = (await session.execute(select(PipelineEvent))).scalar_one() assert row.image_ref == "registry.example.com/acme/payments-api:2.0.41" + async def test_empty_image_ref_accepted_as_null(self, client: AsyncClient) -> None: + # Templating an unset param yields "" more often than an absent key; + # rejecting it would drop the run's duration and status too. + payload = _load("pipeline_jenkins_completed.json") + payload["image_ref"] = "" + response = await client.post("/webhooks/pipeline", json=payload, headers=PIPELINE_AUTH) + assert response.status_code == 202 + + factory = TestBitbucketWebhook._fresh_session_factory(client) + async with factory() as session: + row = (await session.execute(select(PipelineEvent))).scalar_one() + assert row.image_ref is None + async def test_image_ref_optional(self, client: AsyncClient) -> None: # Senders that publish no image (test-only runs) stay valid. payload = _load("pipeline_jenkins_completed.json") diff --git a/tests/test_webhooks_noergler.py b/tests/test_webhooks_noergler.py index d90e9f7..e2a691c 100644 --- a/tests/test_webhooks_noergler.py +++ b/tests/test_webhooks_noergler.py @@ -196,6 +196,29 @@ async def test_delivery_id_dedupes_across_pr_key_casing(self, client: AsyncClien assert len(rows) == 1 +class TestUnpricedRollup: + async def test_rollup_without_cost_accepted(self, client: AsyncClient) -> None: + # An unpriced model must not cost us the whole PR record: outcome, + # diff size, tokens and runs still feed the delivery metrics, and the + # NULL cost makes the FinOps gap visible instead of hiding it in a 0. + payload = _load("noergler_pr_completed_merged.json") + del payload["total_cost_usd"] + r = await client.post("/webhooks/noergler", json=payload, headers=AUTH) + assert r.status_code == 202 + + async with _fresh_session_factory(client)() as session: + row = (await session.execute(select(NoerglerEvent))).scalar_one() + assert row.cost_usd is None + assert row.outcome == "merged" + assert row.total_runs == 3 + + async def test_negative_cost_still_rejected(self, client: AsyncClient) -> None: + payload = _load("noergler_pr_completed_merged.json") + payload["total_cost_usd"] = "-1.0" + r = await client.post("/webhooks/noergler", json=payload, headers=AUTH) + assert r.status_code == 422 + + class TestReviewerHandle: async def test_reviewer_handle_persisted_case_preserved(self, client: AsyncClient) -> None: # Self-reported so riptide can recognise the reviewer's own PR @@ -211,6 +234,16 @@ async def test_reviewer_handle_persisted_case_preserved(self, client: AsyncClien row = (await session.execute(select(NoerglerEvent))).scalar_one() assert row.reviewer_handle == "Rop" + async def test_empty_reviewer_handle_accepted_as_null(self, client: AsyncClient) -> None: + payload = _load("noergler_pr_completed_merged.json") + payload["reviewer_handle"] = "" + r = await client.post("/webhooks/noergler", json=payload, headers=AUTH) + assert r.status_code == 202 + + async with _fresh_session_factory(client)() as session: + row = (await session.execute(select(NoerglerEvent))).scalar_one() + assert row.reviewer_handle is None + async def test_reviewer_handle_optional(self, client: AsyncClient) -> None: payload = _load("noergler_pr_completed_merged.json") r = await client.post("/webhooks/noergler", json=payload, headers=AUTH) From 892b3a6f6f6bbe6895d7699e647e88fe12a7bd1f Mon Sep 17 00:00:00 2001 From: trick77 Date: Tue, 8 Sep 2026 14:43:11 +0200 Subject: [PATCH 3/5] Fix two query defects found in review The cost-by-model query unnested models_used but summed per-PR totals, so a two-model PR credited its full spend to both. The rollup carries no per-model split, so the query now reports model involvement and spend separately. The bot-identity filter used NOT IN, which is NULL for a row with no author and would drop those reviewer touches once any reviewer_handle exists. --- README.md | 4 +++- docs/setup-noergler-notification.md | 19 +++++++++++++------ 2 files changed, 16 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index 2a80d57..0375fe4 100644 --- a/README.md +++ b/README.md @@ -118,7 +118,9 @@ WHERE ps.clock_start IS NOT NULL ) AND e.author IS DISTINCT FROM ps.pr_opener AND NOT e.is_automated - AND lower(e.author) NOT IN (SELECT handle FROM bot_identities) + AND NOT EXISTS ( + SELECT 1 FROM bot_identities b WHERE b.handle = lower(e.author) + ) AND e.occurred_at >= ps.clock_start GROUP BY e.repo_full_name, e.pr_id, ps.clock_start; ``` diff --git a/docs/setup-noergler-notification.md b/docs/setup-noergler-notification.md index ac3bea0..36fdeaa 100644 --- a/docs/setup-noergler-notification.md +++ b/docs/setup-noergler-notification.md @@ -116,15 +116,22 @@ Idempotency key is `(finding_id, verdict)`. ## Verify ```sql --- finops: cost by model, last 7 days -SELECT m AS model, - SUM(prompt_tokens + completion_tokens) AS tokens, - SUM(cost_usd) AS spend, - COUNT(*) AS prs +-- finops: which models were in play, last 7 days. The rollup is per PR, not +-- per model, so cost cannot be split across a multi-model PR — this counts +-- PRs a model took part in, not spend attributable to it. +SELECT m AS model, COUNT(*) AS prs_involved FROM noergler_events, unnest(models_used) AS m WHERE event_type = 'pr_completed' AND created_at > now() - interval '7 days' GROUP BY 1 -ORDER BY spend DESC; +ORDER BY prs_involved DESC; + +-- finops: spend, last 7 days (per PR, the level the data actually supports) +SELECT SUM(cost_usd) AS spend, + SUM(prompt_tokens + completion_tokens) AS tokens, + COUNT(*) AS prs, + COUNT(*) FILTER (WHERE cost_usd IS NULL) AS unpriced_prs +FROM noergler_events +WHERE event_type = 'pr_completed' AND created_at > now() - interval '7 days'; -- review spend that never shipped, last 7 days SELECT outcome, COUNT(*) AS prs, SUM(cost_usd) AS spend From 0922ab84338243bd6913cef6ba8c55f05a6297ae Mon Sep 17 00:00:00 2001 From: trick77 Date: Tue, 8 Sep 2026 15:30:26 +0200 Subject: [PATCH 4/5] Store the sender's automation declaration, not an assumption A reported reviewer account only tells riptide who acted, not what that account is. Senders now declare both: reviewer_handle plus reviewer_is_bot. Read-time bot filtering keys off the declaration, so riptide holds no bot names of its own and a reviewer that posts as a person is not miscounted. The handle stays necessary as the join key: review comments arrive from Bitbucket, where the reviewer is an ordinary user. --- README.md | 12 ++++---- docs/setup-noergler-notification.md | 22 +++++++++------ .../versions/0003_metric_join_columns.py | 18 ++++++++++-- src/riptide_collector/models.py | 8 ++++-- src/riptide_collector/routers/noergler.py | 4 ++- src/riptide_collector/schemas/noergler.py | 15 ++++++++-- tests/test_webhooks_noergler.py | 28 +++++++++++++++++++ 7 files changed, 83 insertions(+), 24 deletions(-) diff --git a/README.md b/README.md index 0375fe4..1609167 100644 --- a/README.md +++ b/README.md @@ -63,7 +63,7 @@ the data captured in v1. | **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 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 `bot_identities` CTE in the query below filters those at read time from the handles noergler self-reports (`noergler_events.reviewer_handle`). | +| **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 `bot_identities` CTE in the query below filters those at read time from the accounts senders declare as automation (`noergler_events.reviewer_handle` + `reviewer_is_bot`) — riptide stores the declaration, it does not carry bot 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. | @@ -79,13 +79,13 @@ traceability. With that in place, the metric is one CTE: ```sql WITH bot_identities AS ( - -- Identities riptide learned from the stream rather than from config: - -- noergler reports the account it comments under. `is_automated` was - -- decided at ingest, so rows written before an identity was known are - -- filtered here instead. + -- Accounts a sender declared as automation, learned from the stream rather + -- than from config: noergler reports the account it comments under and that + -- it is a bot. `is_automated` is decided at ingest, so rows written before + -- an identity was declared are filtered here instead. SELECT DISTINCT lower(reviewer_handle) AS handle FROM noergler_events - WHERE reviewer_handle IS NOT NULL + WHERE reviewer_handle IS NOT NULL AND reviewer_is_bot ), pickup_start AS ( SELECT diff --git a/docs/setup-noergler-notification.md b/docs/setup-noergler-notification.md index 36fdeaa..c55ca55 100644 --- a/docs/setup-noergler-notification.md +++ b/docs/setup-noergler-notification.md @@ -56,6 +56,7 @@ noergler verifies reachability and bearer validity at startup via "pr_key": "PROJ/payments-api#42", "repo": "acme/payments-api", "reviewer_handle": "riptide-reviewer", + "reviewer_is_bot": true, "source_commit_sha": "", "merge_commit_sha": "", "lines_added": 320, @@ -86,14 +87,19 @@ cost-vs-deployment analysis. `pr_key` (`#`) joins to `bitbucket_events (repo_full_name, pr_id)` — that is also where riptide gets PR diff sizes from, since Bitbucket's webhooks carry none. -`reviewer_handle` is optional but recommended: it is the account noergler posts -its review comments under on the git host. Reporting it lets riptide's read-time -queries recognise those comments as automation — see the `bot_identities` CTE in -the pickup-time query in the README — without every installation adding the -handle to its `automation` config. Unrecognised, the bot counts as a human -reviewer and drives the code-review pickup-time metric toward zero. Adding the -handle to `automation` as well is still worthwhile: that also tags new rows -`is_automated` at ingest. +`reviewer_handle` and `reviewer_is_bot` are optional but recommended: together +they are the sender declaring **which account it acts as and that the account is +automation**. riptide stores that declaration rather than keeping bot names of +its own — but it does need the handle, because the review comments arrive from +Bitbucket, where the reviewer is just another user, and the handle is the only +key back to those rows. `reviewer_is_bot` defaults to `true`; set it `false` if +the reviewer posts as a person. + +Read-time queries exclude declared-automation accounts — see the +`bot_identities` CTE in the pickup-time query in the README. Undeclared, the bot +counts as a human reviewer and drives code-review pickup time toward zero. +Adding the handle to the `automation` config as well is still worthwhile: that +tags new rows `is_automated` at ingest. ### `feedback` diff --git a/migrations/versions/0003_metric_join_columns.py b/migrations/versions/0003_metric_join_columns.py index e7a807d..df6d22f 100644 --- a/migrations/versions/0003_metric_join_columns.py +++ b/migrations/versions/0003_metric_join_columns.py @@ -9,9 +9,11 @@ Argo CD stores exactly these strings in `payload->'images'`, so this is the exact join from a deploy back to the build and its commit — image tags are versions, not commit SHAs. -- `noergler_events.reviewer_handle`: the reviewer's own account handle, - self-reported, so automation identity comes from the stream rather than - per-installation configuration. +- `noergler_events.reviewer_handle` / `reviewer_is_bot`: the account the + reviewer acts as and whether it is automation, both self-reported, so + automation identity comes from the stream rather than per-installation + configuration. The handle is the join key back to the Bitbucket rows the + reviewer's comments produced. Revision ID: 0003 Revises: 0002 @@ -58,11 +60,21 @@ def upgrade() -> None: comment="git-host account the reviewer posts under; self-reported automation identity", ), ) + op.add_column( + "noergler_events", + sa.Column( + "reviewer_is_bot", + sa.Boolean, + nullable=True, + comment="whether reviewer_handle is automation, as declared by the sender", + ), + ) op.create_index("ix_pipeline_events_image_ref", "pipeline_events", ["image_ref"]) def downgrade() -> None: op.drop_index("ix_pipeline_events_image_ref", table_name="pipeline_events") + op.drop_column("noergler_events", "reviewer_is_bot") op.drop_column("noergler_events", "reviewer_handle") op.drop_column("pipeline_events", "image_ref") op.drop_column("bitbucket_events", "author_display_name") diff --git a/src/riptide_collector/models.py b/src/riptide_collector/models.py index 5503f6a..951a1ea 100644 --- a/src/riptide_collector/models.py +++ b/src/riptide_collector/models.py @@ -139,10 +139,12 @@ class NoerglerEvent(Base): commit_sha: Mapped[str | None] = mapped_column(String, nullable=True) # pr_completed-only: outcome: Mapped[str | None] = mapped_column(String, nullable=True) - # The reviewer's own account handle on the git host. Self-reported so - # riptide learns the bot's identity from the stream instead of every - # installation configuring it by hand. + # The reviewer's own account on the git host, and whether that account is + # automation. Self-reported: riptide stores the sender's declaration rather + # than carrying bot names in its config. The handle is the join key back to + # the Bitbucket rows those review comments produced. reviewer_handle: Mapped[str | None] = mapped_column(String, nullable=True) + reviewer_is_bot: Mapped[bool | None] = mapped_column(Boolean, nullable=True) merge_commit_sha: Mapped[str | None] = mapped_column(String, nullable=True) lines_added: Mapped[int | None] = mapped_column(Integer, nullable=True) lines_removed: Mapped[int | None] = mapped_column(Integer, nullable=True) diff --git a/src/riptide_collector/routers/noergler.py b/src/riptide_collector/routers/noergler.py index d3c608c..223c817 100644 --- a/src/riptide_collector/routers/noergler.py +++ b/src/riptide_collector/routers/noergler.py @@ -87,8 +87,10 @@ def _values_pr_completed( "pr_key": lower(event.pr_key), "repo": lower(event.repo), # Kept case-preserved: it is matched against the git host's own - # user handles, which riptide stores as delivered. + # user handles, which riptide stores as delivered. The flag is the + # sender's declaration about that account, not our inference. "reviewer_handle": event.reviewer_handle, + "reviewer_is_bot": event.reviewer_is_bot if event.reviewer_handle else None, "commit_sha": lower(event.source_commit_sha), "merge_commit_sha": lower(event.merge_commit_sha) if event.merge_commit_sha else None, "lines_added": event.lines_added, diff --git a/src/riptide_collector/schemas/noergler.py b/src/riptide_collector/schemas/noergler.py index e508132..23b1ad3 100644 --- a/src/riptide_collector/schemas/noergler.py +++ b/src/riptide_collector/schemas/noergler.py @@ -46,9 +46,18 @@ class NoerglerPrCompleted(_Common): default=None, description=( "The account the reviewer posts its comments under on the git host. " - "Self-reporting it lets riptide recognise the reviewer's own comments as " - "automation without every installation configuring the handle by hand — " - "otherwise the bot counts as a human reviewer and collapses review-pickup time." + "It is the join key back to the Bitbucket events those comments produced — " + "riptide cannot recognise them any other way, since Bitbucket reports an " + "ordinary user. Self-reporting it keeps the handle out of riptide's config." + ), + ) + reviewer_is_bot: bool = Field( + default=True, + description=( + "Whether that account is automation. Sent alongside the handle so riptide " + "does not have to assume what a reported identity means: it stores the " + "sender's declaration and excludes declared-automation accounts from " + "human-review metrics. Set false if the reviewer posts as a person." ), ) source_commit_sha: str = Field( diff --git a/tests/test_webhooks_noergler.py b/tests/test_webhooks_noergler.py index e2a691c..1487ba0 100644 --- a/tests/test_webhooks_noergler.py +++ b/tests/test_webhooks_noergler.py @@ -220,6 +220,32 @@ async def test_negative_cost_still_rejected(self, client: AsyncClient) -> None: class TestReviewerHandle: + async def test_declared_human_reviewer_not_flagged_as_bot(self, client: AsyncClient) -> None: + # The flag is the sender's declaration about its own account; riptide + # stores it rather than assuming every reported handle is a bot. + payload = _load("noergler_pr_completed_merged.json") + payload["reviewer_handle"] = "alice" + payload["reviewer_is_bot"] = False + r = await client.post("/webhooks/noergler", json=payload, headers=AUTH) + assert r.status_code == 202 + + async with _fresh_session_factory(client)() as session: + row = (await session.execute(select(NoerglerEvent))).scalar_one() + assert row.reviewer_handle == "alice" + assert row.reviewer_is_bot is False + + async def test_is_bot_null_without_a_handle(self, client: AsyncClient) -> None: + # A flag with no account to attach it to is meaningless, so it is not + # stored as a standalone truth. + payload = _load("noergler_pr_completed_merged.json") + r = await client.post("/webhooks/noergler", json=payload, headers=AUTH) + assert r.status_code == 202 + + async with _fresh_session_factory(client)() as session: + row = (await session.execute(select(NoerglerEvent))).scalar_one() + assert row.reviewer_handle is None + assert row.reviewer_is_bot is None + async def test_reviewer_handle_persisted_case_preserved(self, client: AsyncClient) -> None: # Self-reported so riptide can recognise the reviewer's own PR # comments as automation without every installation configuring the @@ -233,6 +259,8 @@ async def test_reviewer_handle_persisted_case_preserved(self, client: AsyncClien async with _fresh_session_factory(client)() as session: row = (await session.execute(select(NoerglerEvent))).scalar_one() assert row.reviewer_handle == "Rop" + # Automation unless the sender says otherwise. + assert row.reviewer_is_bot is True async def test_empty_reviewer_handle_accepted_as_null(self, client: AsyncClient) -> None: payload = _load("noergler_pr_completed_merged.json") From 28147edbd8e3d6810e2605617fc814e973f7a41a Mon Sep 17 00:00:00 2001 From: trick77 Date: Tue, 8 Sep 2026 15:36:44 +0200 Subject: [PATCH 5/5] Generalise the identity declaration to bot and service accounts A boolean only answered 'not human'. Senders now declare an account kind - bot, service or human - because the two non-human kinds read differently: a bot authors work of its own and its velocity is worth charting, while a service account authors nothing and should simply not appear in human activity. 'agent' was avoided as a name, it already means a build agent here. Pipeline senders can declare their git account the same way, which is what finally removes a CI service account from human metrics. Read-time filtering unions every sender's declarations. --- README.md | 22 ++++++----- docs/setup-jenkins-notification.md | 12 ++++++ docs/setup-noergler-notification.md | 17 ++++---- .../versions/0003_metric_join_columns.py | 39 ++++++++++++++----- src/riptide_collector/models.py | 9 ++++- src/riptide_collector/routers/noergler.py | 2 +- src/riptide_collector/routers/pipeline.py | 7 ++++ src/riptide_collector/schemas/noergler.py | 14 ++++--- src/riptide_collector/schemas/pipeline.py | 23 +++++++++++ tests/test_webhooks.py | 27 +++++++++++++ tests/test_webhooks_noergler.py | 25 +++++++----- 11 files changed, 154 insertions(+), 43 deletions(-) diff --git a/README.md b/README.md index 1609167..3670e74 100644 --- a/README.md +++ b/README.md @@ -63,7 +63,7 @@ the data captured in v1. | **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 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 `bot_identities` CTE in the query below filters those at read time from the accounts senders declare as automation (`noergler_events.reviewer_handle` + `reviewer_is_bot`) — riptide stores the declaration, it does not carry bot names of its own. | +| **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,14 +78,18 @@ dropped at parse time. The raw `eventKey` survives on `payload.eventKey` for traceability. With that in place, the metric is one CTE: ```sql -WITH bot_identities AS ( - -- Accounts a sender declared as automation, learned from the stream rather - -- than from config: noergler reports the account it comments under and that - -- it is a bot. `is_automated` is decided at ingest, so rows written before - -- an identity was declared are filtered here instead. +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_is_bot + 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 @@ -119,7 +123,7 @@ 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 bot_identities b WHERE b.handle = lower(e.author) + 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; @@ -149,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`). Default dashboards exclude bots; bot velocity is a separate CI-health view. | +| **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 diff --git a/docs/setup-jenkins-notification.md b/docs/setup-jenkins-notification.md index e15c94b..6cb3dc9 100644 --- a/docs/setup-jenkins-notification.md +++ b/docs/setup-jenkins-notification.md @@ -18,6 +18,8 @@ mandatory** — without them the metrics break. "status": "", "commit_sha": "", "image_ref": "", + "actor_handle": "", + "actor_account_kind": "service", "started_at": "", "finished_at": "" } @@ -27,6 +29,12 @@ The `team` column is populated from the bearer token, not the payload. Cross-source joins (to Bitbucket / Noergler) use `commit_sha`; per-pipeline aggregations use `pipeline_name`. +`actor_handle` is the git-host account Jenkins itself commits and pushes as. +Declaring it (with `actor_account_kind: "service"`) keeps those events out of +human-activity metrics — a CI account can easily author a third of all +repository events, and counted as a person it distorts every per-author view. +riptide stores the declaration instead of guessing from the name. + `image_ref` is optional but send it whenever the run publishes an image: it is the only reliable link from an Argo CD deploy back to the commit, because `argocd_events.revision` is the GitOps-repo SHA and image tags are versions @@ -93,6 +101,10 @@ def riptideNotify(String phase) { // Full reference of the image this run pushed, or null. Links the // deploy back to this build — Argo CD reports the same string. image_ref: env.RIPTIDE_IMAGE_REF ?: null, + // The git account Jenkins acts through, so its pushes and merges + // don't count as human activity. + actor_handle: env.RIPTIDE_ACTOR_HANDLE ?: null, + actor_account_kind: 'service', started_at: new Date(started).format("yyyy-MM-dd'T'HH:mm:ss'Z'", TimeZone.getTimeZone('UTC')), finished_at: phase == 'COMPLETED' ? new Date(finished).format("yyyy-MM-dd'T'HH:mm:ss'Z'", TimeZone.getTimeZone('UTC')) diff --git a/docs/setup-noergler-notification.md b/docs/setup-noergler-notification.md index c55ca55..12504c4 100644 --- a/docs/setup-noergler-notification.md +++ b/docs/setup-noergler-notification.md @@ -56,7 +56,7 @@ noergler verifies reachability and bearer validity at startup via "pr_key": "PROJ/payments-api#42", "repo": "acme/payments-api", "reviewer_handle": "riptide-reviewer", - "reviewer_is_bot": true, + "reviewer_account_kind": "bot", "source_commit_sha": "", "merge_commit_sha": "", "lines_added": 320, @@ -87,13 +87,14 @@ cost-vs-deployment analysis. `pr_key` (`#`) joins to `bitbucket_events (repo_full_name, pr_id)` — that is also where riptide gets PR diff sizes from, since Bitbucket's webhooks carry none. -`reviewer_handle` and `reviewer_is_bot` are optional but recommended: together -they are the sender declaring **which account it acts as and that the account is -automation**. riptide stores that declaration rather than keeping bot names of -its own — but it does need the handle, because the review comments arrive from -Bitbucket, where the reviewer is just another user, and the handle is the only -key back to those rows. `reviewer_is_bot` defaults to `true`; set it `false` if -the reviewer posts as a person. +`reviewer_handle` and `reviewer_account_kind` are optional but recommended: +together they are the sender declaring **which account it acts as, and what that +account is**. riptide stores that declaration rather than keeping account names +of its own — but it does need the handle, because the review comments arrive +from Bitbucket, where the reviewer is just another user, and the handle is the +only key back to those rows. `reviewer_account_kind` is `bot` (the default, an +actor working on its own), `service` (a technical account a system acts through) +or `human` (a person, counted as one). Read-time queries exclude declared-automation accounts — see the `bot_identities` CTE in the pickup-time query in the README. Undeclared, the bot diff --git a/migrations/versions/0003_metric_join_columns.py b/migrations/versions/0003_metric_join_columns.py index df6d22f..7e01fbb 100644 --- a/migrations/versions/0003_metric_join_columns.py +++ b/migrations/versions/0003_metric_join_columns.py @@ -9,11 +9,12 @@ Argo CD stores exactly these strings in `payload->'images'`, so this is the exact join from a deploy back to the build and its commit — image tags are versions, not commit SHAs. -- `noergler_events.reviewer_handle` / `reviewer_is_bot`: the account the - reviewer acts as and whether it is automation, both self-reported, so - automation identity comes from the stream rather than per-installation - configuration. The handle is the join key back to the Bitbucket rows the - reviewer's comments produced. +- `noergler_events.reviewer_handle` / `reviewer_account_kind` and + `pipeline_events.actor_handle` / `actor_account_kind`: the git-host account a + sender acts through and what that account is (bot / service / human), both + self-declared, so automation identity comes from the stream rather than + per-installation configuration. The handle is the join key back to the + Bitbucket rows that account produced. Revision ID: 0003 Revises: 0002 @@ -63,10 +64,28 @@ def upgrade() -> None: op.add_column( "noergler_events", sa.Column( - "reviewer_is_bot", - sa.Boolean, + "reviewer_account_kind", + sa.String, + nullable=True, + comment="bot | service | human — what reviewer_handle is, per the sender", + ), + ) + op.add_column( + "pipeline_events", + sa.Column( + "actor_handle", + sa.String, + nullable=True, + comment="git-host account this CI acts through; self-declared", + ), + ) + op.add_column( + "pipeline_events", + sa.Column( + "actor_account_kind", + sa.String, nullable=True, - comment="whether reviewer_handle is automation, as declared by the sender", + comment="bot | service | human — what actor_handle is, per the sender", ), ) op.create_index("ix_pipeline_events_image_ref", "pipeline_events", ["image_ref"]) @@ -74,7 +93,9 @@ def upgrade() -> None: def downgrade() -> None: op.drop_index("ix_pipeline_events_image_ref", table_name="pipeline_events") - op.drop_column("noergler_events", "reviewer_is_bot") + op.drop_column("pipeline_events", "actor_account_kind") + op.drop_column("pipeline_events", "actor_handle") + op.drop_column("noergler_events", "reviewer_account_kind") op.drop_column("noergler_events", "reviewer_handle") op.drop_column("pipeline_events", "image_ref") op.drop_column("bitbucket_events", "author_display_name") diff --git a/src/riptide_collector/models.py b/src/riptide_collector/models.py index 951a1ea..771c254 100644 --- a/src/riptide_collector/models.py +++ b/src/riptide_collector/models.py @@ -97,6 +97,12 @@ class PipelineEvent(Base): # `argocd_events → pipeline_events → commit_sha` an exact join even when the # image tag is a version rather than a commit SHA. image_ref: Mapped[str | None] = mapped_column(String, nullable=True) + # The git-host account this CI acts through, and what that account is + # ('bot' | 'service' | 'human'). Senders declare their own identity; + # riptide stores the declaration rather than guessing from the name, and + # read-time queries drop non-human accounts from human-activity metrics. + actor_handle: Mapped[str | None] = mapped_column(String, nullable=True) + actor_account_kind: Mapped[str | None] = mapped_column(String, nullable=True) started_at: Mapped[datetime | None] = mapped_column(DateTime(timezone=True), nullable=True) finished_at: Mapped[datetime | None] = mapped_column(DateTime(timezone=True), nullable=True) duration_seconds: Mapped[int | None] = mapped_column( @@ -144,7 +150,8 @@ class NoerglerEvent(Base): # than carrying bot names in its config. The handle is the join key back to # the Bitbucket rows those review comments produced. reviewer_handle: Mapped[str | None] = mapped_column(String, nullable=True) - reviewer_is_bot: Mapped[bool | None] = mapped_column(Boolean, nullable=True) + # 'bot' | 'service' | 'human' — see PipelineEvent.actor_account_kind. + reviewer_account_kind: Mapped[str | None] = mapped_column(String, nullable=True) merge_commit_sha: Mapped[str | None] = mapped_column(String, nullable=True) lines_added: Mapped[int | None] = mapped_column(Integer, nullable=True) lines_removed: Mapped[int | None] = mapped_column(Integer, nullable=True) diff --git a/src/riptide_collector/routers/noergler.py b/src/riptide_collector/routers/noergler.py index 223c817..26a6c12 100644 --- a/src/riptide_collector/routers/noergler.py +++ b/src/riptide_collector/routers/noergler.py @@ -90,7 +90,7 @@ def _values_pr_completed( # user handles, which riptide stores as delivered. The flag is the # sender's declaration about that account, not our inference. "reviewer_handle": event.reviewer_handle, - "reviewer_is_bot": event.reviewer_is_bot if event.reviewer_handle else None, + "reviewer_account_kind": event.reviewer_account_kind if event.reviewer_handle else None, "commit_sha": lower(event.source_commit_sha), "merge_commit_sha": lower(event.merge_commit_sha) if event.merge_commit_sha else None, "lines_added": event.lines_added, diff --git a/src/riptide_collector/routers/pipeline.py b/src/riptide_collector/routers/pipeline.py index abbae22..9961700 100644 --- a/src/riptide_collector/routers/pipeline.py +++ b/src/riptide_collector/routers/pipeline.py @@ -50,6 +50,13 @@ async def pipeline_webhook( # pyright: ignore[reportUnusedFunction] status=event.status, commit_sha=commit_sha, image_ref=event.image_ref, + # The kind is the sender's statement about its own + # account; without a handle there is nothing for it + # to describe. + actor_handle=event.actor_handle, + actor_account_kind=( + event.actor_account_kind if event.actor_handle else None + ), started_at=event.started_at, finished_at=event.finished_at, occurred_at=event.finished_at or event.started_at or datetime.now(UTC), diff --git a/src/riptide_collector/schemas/noergler.py b/src/riptide_collector/schemas/noergler.py index 23b1ad3..d71329e 100644 --- a/src/riptide_collector/schemas/noergler.py +++ b/src/riptide_collector/schemas/noergler.py @@ -51,13 +51,15 @@ class NoerglerPrCompleted(_Common): "ordinary user. Self-reporting it keeps the handle out of riptide's config." ), ) - reviewer_is_bot: bool = Field( - default=True, + reviewer_account_kind: Literal["bot", "service", "human"] = Field( + default="bot", description=( - "Whether that account is automation. Sent alongside the handle so riptide " - "does not have to assume what a reported identity means: it stores the " - "sender's declaration and excludes declared-automation accounts from " - "human-review metrics. Set false if the reviewer posts as a person." + "What that account is. Sent alongside the handle so riptide does not have " + "to assume what a reported identity means: it stores the sender's " + "declaration and excludes 'bot' and 'service' accounts from human metrics. " + "'bot' acts on its own (a review bot, Renovate); 'service' is a technical " + "account a system acts through (a CI user pushing merges); 'human' is a " + "person, and is counted as one." ), ) source_commit_sha: str = Field( diff --git a/src/riptide_collector/schemas/pipeline.py b/src/riptide_collector/schemas/pipeline.py index 759e4ca..d4891e3 100644 --- a/src/riptide_collector/schemas/pipeline.py +++ b/src/riptide_collector/schemas/pipeline.py @@ -6,6 +6,7 @@ """ from datetime import UTC, datetime +from typing import Literal from pydantic import BaseModel, ConfigDict, Field, field_validator @@ -46,9 +47,31 @@ class PipelineWebhook(BaseModel): "that publish no image." ), ) + actor_handle: str | None = Field( + default=None, + description=( + "the git-host account this CI system acts through, e.g. the user whose " + "name appears on merges and pushes it makes. Declaring it (with " + "actor_account_kind) keeps those events out of human-activity metrics — " + "a CI service account can easily be a third of all repository events." + ), + ) + actor_account_kind: Literal["bot", "service", "human"] = Field( + default="service", + description=( + "what actor_handle is: 'service' for a technical account a system acts " + "through, 'bot' for automation acting on its own, 'human' for a person. " + "riptide stores this declaration instead of guessing from the name." + ), + ) started_at: datetime finished_at: datetime | None = None + @field_validator("actor_handle") + @classmethod + def _empty_actor_handle_is_none(cls, v: str | None) -> str | None: + return v.strip() or None if v else None + @field_validator("started_at", "finished_at") @classmethod def _normalise_tz(cls, v: datetime | None) -> datetime | None: diff --git a/tests/test_webhooks.py b/tests/test_webhooks.py index f77b4a8..1ab1067 100644 --- a/tests/test_webhooks.py +++ b/tests/test_webhooks.py @@ -238,6 +238,33 @@ async def test_image_ref_persisted(self, client: AsyncClient) -> None: row = (await session.execute(select(PipelineEvent))).scalar_one() assert row.image_ref == "registry.example.com/acme/payments-api:2.0.41" + async def test_ci_service_account_declaration_persisted(self, client: AsyncClient) -> None: + # A CI account can author a third of all repository events; declaring + # it keeps those out of human-activity metrics without riptide + # knowing any account names itself. + payload = _load("pipeline_jenkins_completed.json") + payload["actor_handle"] = "ci-service" + response = await client.post("/webhooks/pipeline", json=payload, headers=PIPELINE_AUTH) + assert response.status_code == 202 + + factory = TestBitbucketWebhook._fresh_session_factory(client) + async with factory() as session: + row = (await session.execute(select(PipelineEvent))).scalar_one() + assert row.actor_handle == "ci-service" + # A technical account unless the sender says otherwise. + assert row.actor_account_kind == "service" + + async def test_actor_kind_null_without_a_handle(self, client: AsyncClient) -> None: + payload = _load("pipeline_jenkins_completed.json") + response = await client.post("/webhooks/pipeline", json=payload, headers=PIPELINE_AUTH) + assert response.status_code == 202 + + factory = TestBitbucketWebhook._fresh_session_factory(client) + async with factory() as session: + row = (await session.execute(select(PipelineEvent))).scalar_one() + assert row.actor_handle is None + assert row.actor_account_kind is None + async def test_empty_image_ref_accepted_as_null(self, client: AsyncClient) -> None: # Templating an unset param yields "" more often than an absent key; # rejecting it would drop the run's duration and status too. diff --git a/tests/test_webhooks_noergler.py b/tests/test_webhooks_noergler.py index 1487ba0..ed79f98 100644 --- a/tests/test_webhooks_noergler.py +++ b/tests/test_webhooks_noergler.py @@ -220,22 +220,29 @@ async def test_negative_cost_still_rejected(self, client: AsyncClient) -> None: class TestReviewerHandle: - async def test_declared_human_reviewer_not_flagged_as_bot(self, client: AsyncClient) -> None: - # The flag is the sender's declaration about its own account; riptide + async def test_declared_human_reviewer_kept_human(self, client: AsyncClient) -> None: + # The kind is the sender's declaration about its own account; riptide # stores it rather than assuming every reported handle is a bot. payload = _load("noergler_pr_completed_merged.json") payload["reviewer_handle"] = "alice" - payload["reviewer_is_bot"] = False + payload["reviewer_account_kind"] = "human" r = await client.post("/webhooks/noergler", json=payload, headers=AUTH) assert r.status_code == 202 async with _fresh_session_factory(client)() as session: row = (await session.execute(select(NoerglerEvent))).scalar_one() assert row.reviewer_handle == "alice" - assert row.reviewer_is_bot is False + assert row.reviewer_account_kind == "human" - async def test_is_bot_null_without_a_handle(self, client: AsyncClient) -> None: - # A flag with no account to attach it to is meaningless, so it is not + async def test_unknown_account_kind_rejected(self, client: AsyncClient) -> None: + payload = _load("noergler_pr_completed_merged.json") + payload["reviewer_handle"] = "svc" + payload["reviewer_account_kind"] = "robot" + r = await client.post("/webhooks/noergler", json=payload, headers=AUTH) + assert r.status_code == 422 + + async def test_account_kind_null_without_a_handle(self, client: AsyncClient) -> None: + # A kind with no account to describe is meaningless, so it is not # stored as a standalone truth. payload = _load("noergler_pr_completed_merged.json") r = await client.post("/webhooks/noergler", json=payload, headers=AUTH) @@ -244,7 +251,7 @@ async def test_is_bot_null_without_a_handle(self, client: AsyncClient) -> None: async with _fresh_session_factory(client)() as session: row = (await session.execute(select(NoerglerEvent))).scalar_one() assert row.reviewer_handle is None - assert row.reviewer_is_bot is None + assert row.reviewer_account_kind is None async def test_reviewer_handle_persisted_case_preserved(self, client: AsyncClient) -> None: # Self-reported so riptide can recognise the reviewer's own PR @@ -259,8 +266,8 @@ async def test_reviewer_handle_persisted_case_preserved(self, client: AsyncClien async with _fresh_session_factory(client)() as session: row = (await session.execute(select(NoerglerEvent))).scalar_one() assert row.reviewer_handle == "Rop" - # Automation unless the sender says otherwise. - assert row.reviewer_is_bot is True + # A review bot unless the sender says otherwise. + assert row.reviewer_account_kind == "bot" async def test_empty_reviewer_handle_accepted_as_null(self, client: AsyncClient) -> None: payload = _load("noergler_pr_completed_merged.json")