diff --git a/AGENTS.md b/AGENTS.md index a7b3e2d..6fd260b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -32,7 +32,7 @@ docker compose up # Postgres + migrate + app on :8000 - **`change_type` on Bitbucket events only.** Don't denormalise onto pipeline / Argo rows; join at read time. - **Automation detection is config-last.** Order: configured `automation` authors (matched against login *and* display name, case-insensitive) → acting user's `type == "SERVICE"` from the payload → `*-bot` name shape. Senders also declare themselves (`reviewer_handle` / `actor_handle` + account kind, read-time filter). Only accounts nobody reports get a config entry. `automation` is org-wide, at the config root. - **CI events are source-tagged, not source-routed.** Every CI lands in `pipeline_events` via `POST /webhooks/pipeline`, told apart by `source`. No per-CI tables or endpoints. Dedup key `source#pipeline_name#run_id#phase`. -- **Noergler carries finops + reviewer-precision only.** `event_type` ∈ `pr_completed` | `feedback`; the pre-rollup `completed` is rejected. Never re-emit PR lifecycle — `bitbucket_events` covers open / merged / declined. Dedup keys `pr_completed##`, `feedback##`. `pr_completed` is also the source for PR diff size (Bitbucket webhooks carry none) and for the reviewer's own account. +- **Noergler carries finops + reviewer-precision only.** `event_type` ∈ `pr_completed` | `feedback`; the pre-rollup `completed` is rejected. Never re-emit PR lifecycle — `bitbucket_events` covers open / merged / declined. Dedup keys `pr_completed##`, `feedback##`. A rollup is cumulative and a declined PR reopened and merged emits again (not a lifecycle replay): read spend via the `noergler_pr_rollups` view (newest row per `pr_key`), never a SUM over `noergler_events`. Known gap: declined (or deleted) again after a reopen emits nothing. `pr_completed` is also the source for PR diff size (Bitbucket webhooks carry none) and for the reviewer's own account. - **Senders verify at startup via `GET /auth/ping`** — authenticated, returns the caller's team, so a wrong token fails fast. Never reuse `/health` (unauth liveness) or `/ready` (unauth readiness). - **`modified_at` has a Postgres trigger** (`riptide_set_modified_at`), so any `UPDATE`, raw SQL included, bumps it. Keep the trigger when changing migrations. - **Database is external.** riptide ships no Postgres. diff --git a/README.md b/README.md index 3132517..73d63fc 100644 --- a/README.md +++ b/README.md @@ -172,7 +172,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` 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. | +| **PR size** | `lines_added`, `lines_removed`, `files_changed` on `noergler_pr_rollups` (one row per PR; a reopened PR has two `pr_completed` rows in `noergler_events`), 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, over `pr:opened` / `pr:merged` rows only — a push to `master` has no prefix to classify and would swamp the distribution with `other`. | @@ -180,7 +180,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`; an account the git host itself marks as a service account — Bitbucket DC's built-in system user, which files the default PR tasks — is tagged `service-account` with no config entry), plus the `non_human_identities` filter for accounts a sender declared. Keep `bot` and `service` apart when reading: a **bot** authors work of its own and its velocity is worth its own view, while a **service** account (a CI user pushing merges) authors nothing and should simply not appear in human activity. Default dashboards exclude both. | -| **AI reviewer precision** *(noergler)* | `1 - count(noergler_events WHERE event_type='feedback' AND verdict='disagreed') / sum(findings_count) FILTER (WHERE event_type='pr_completed')` per repo × week — findings on both sides, since one PR can collect several disagreements. Higher = the AI review is more useful. Filter on `outcome='merged'` to score precision only on PRs that shipped. | +| **AI reviewer precision** *(noergler)* | `1 - count(noergler_events WHERE event_type='feedback' AND verdict='disagreed') / sum(noergler_pr_rollups.findings_count)` 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 @@ -191,9 +191,9 @@ events arrive pre-priced in USD. | Signal | How it's computed | |---|---| -| **LLM review spend per PR / team** *(noergler)* | `SUM(cost_usd), SUM(prompt_tokens + completion_tokens) GROUP BY team` over `event_type = 'pr_completed'`. Each row is a per-PR rollup (one event per merged / declined / deleted PR). Filter on `outcome='merged'` for "spend that actually shipped"; keep all outcomes for total LLM-review spend including abandoned PRs. Pre-priced — no multiplier needed. | -| **LLM review cost per KLOC** *(noergler)* | `SUM(cost_usd) / NULLIF(SUM(lines_added + lines_removed), 0) * 1000 GROUP BY team, outcome` over `noergler_events WHERE event_type='pr_completed'`. Diff-size normalised cost — fair comparison across small fixes and large refactors. | -| **Wasted LLM review** *(noergler)* | `SUM(cost_usd) FROM noergler_events WHERE event_type='pr_completed' AND outcome IN ('declined','deleted')`. Review effort spent on code that never shipped. | +| **LLM review spend per PR / team** *(noergler)* | `SUM(cost_usd), SUM(prompt_tokens + completion_tokens) GROUP BY team` over the `noergler_pr_rollups` view, the newest cumulative rollup per PR (a declined PR reopened and merged emits twice; summing `noergler_events` double-counts it). Filter on `outcome='merged'` for "spend that actually shipped"; keep all outcomes for total LLM-review spend including abandoned PRs. Pre-priced — no multiplier needed. | +| **LLM review cost per KLOC** *(noergler)* | `SUM(cost_usd) / NULLIF(SUM(lines_added + lines_removed), 0) * 1000 GROUP BY team, outcome` over `noergler_pr_rollups`. Diff-size normalised cost — fair comparison across small fixes and large refactors. | +| **Wasted LLM review** *(noergler)* | `SUM(cost_usd) FROM noergler_pr_rollups WHERE outcome IN ('declined','deleted')`. Review effort spent on code that never shipped. | | **CI compute time per pipeline / team** | `SUM(pipeline_events.duration_seconds) GROUP BY pipeline_name, team`. The unit metric for CI cost attribution. | | **Wasted CI** | `SUM(duration_seconds) WHERE status IN ('FAILURE','Failed')` — failed builds × time. Quantifies the cost of flakes / broken tests. | | **Bot-driven pipeline churn** | `pipeline_events` joined to `bitbucket_events` via `commit_sha` filtered on `is_automated = true`. Renovate / Dependabot can drive 40–70% of pipeline runs in many orgs; useful input for batching policies. | diff --git a/backend/api/openapi.yaml b/backend/api/openapi.yaml index 8b6f871..0e41dd8 100644 --- a/backend/api/openapi.yaml +++ b/backend/api/openapi.yaml @@ -154,7 +154,8 @@ paths: description: | Authenticated with the team's `noergler` key. Unknown fields are a `422`. Delivery ids: `pr_completed##` and - `feedback##`. + `feedback##`. A declined PR that is reopened and + merged sends a second, cumulative `pr_completed`. security: [{ teamKey: [] }] requestBody: required: true diff --git a/backend/internal/parse/owned.go b/backend/internal/parse/owned.go index c421d20..aed680d 100644 --- a/backend/internal/parse/owned.go +++ b/backend/internal/parse/owned.go @@ -262,9 +262,11 @@ func parsePRCompleted(o *object, raw []byte) (*NoerglerDraft, error) { d.MergeCommitSHA = ptr(strings.ToLower(*merge)) } d.OccurredAt = closed - // One terminal outcome per PR, redelivered on retries. The key includes - // the outcome so deleted-after-declined lands two rows; pr_key is - // lowercased so a casing-flipped redelivery still dedupes. + // One rollup per terminal outcome, redelivered on retries. The key + // includes the outcome so a PR reopened after a decline and merged lands + // its merged rollup as a second row; noergler_pr_rollups reads the + // newest. pr_key is lowercased so a casing-flipped redelivery still + // dedupes. d.DeliveryID = NoerglerPRCompleted + "#" + d.PRKey + "#" + outcome return d, nil } diff --git a/backend/internal/store/migrations/0002_noergler_pr_rollups.sql b/backend/internal/store/migrations/0002_noergler_pr_rollups.sql new file mode 100644 index 0000000..efe2c91 --- /dev/null +++ b/backend/internal/store/migrations/0002_noergler_pr_rollups.sql @@ -0,0 +1,31 @@ +-- noergler_pr_rollups: the newest pr_completed row per pr_key. A rollup is +-- cumulative over the PR's life, and a declined PR that is reopened and +-- merged emits again (two rows: declined, then merged). Summed over +-- noergler_events, the earlier row's spend counted twice. Plain view: the +-- raw rows stay append-only and nothing needs refreshing. +-- +-- Newest by occurred_at (the sender's emit time), then by id for a tie, so a +-- delayed older rollup cannot win by arriving last. A deleted rollup ranks +-- below any other: a declined PR deleted later (older senders emitted both) +-- carries 0 lines and 0 files, and would wipe the declined row's diff size. +-- Per pr_key, not per team: the newest rollup already carries the PR's whole +-- spend. It also moves a reopened PR's whole spend into its merge week. +-- Known gap: a PR declined (or deleted) again after a reopen sends no second +-- rollup, so the spend after the first decline is missing here. +-- +-- Columns are listed, not *: a column added to noergler_events later is a +-- deliberate CREATE OR REPLACE here, not a silent omission. +CREATE INDEX ix_noergler_events_pr_completed_newest + ON noergler_events (pr_key, (outcome = 'deleted'), occurred_at DESC, id DESC) + WHERE event_type = 'pr_completed'; + +CREATE VIEW noergler_pr_rollups AS +SELECT DISTINCT ON (pr_key) + id, delivery_id, event_type, pr_key, repo, commit_sha, + prompt_tokens, completion_tokens, elapsed_ms, findings_count, cost_usd, + occurred_at, created_at, team, payload, outcome, merge_commit_sha, + lines_added, lines_removed, files_changed, total_runs, models_used, + first_review_at, reviewer_handle, reviewer_account_kind + FROM noergler_events + WHERE event_type = 'pr_completed' + ORDER BY pr_key, (outcome = 'deleted'), occurred_at DESC, id DESC; diff --git a/backend/internal/store/store_test.go b/backend/internal/store/store_test.go index da1d47f..fb513e4 100644 --- a/backend/internal/store/store_test.go +++ b/backend/internal/store/store_test.go @@ -5,6 +5,7 @@ import ( "encoding/json" "io" "log/slog" + "reflect" "strings" "testing" "time" @@ -55,8 +56,12 @@ func TestMigrateIsIdempotentAndSchemaCurrent(t *testing.T) { t.Fatal(err) } var n int - if err := s.pool.QueryRow(ctx, `SELECT count(*) FROM schema_migrations`).Scan(&n); err != nil || n != 1 { - t.Fatalf("schema_migrations rows = %d, %v", n, err) + files, err := migrationFS.ReadDir("migrations") + if err != nil { + t.Fatal(err) + } + if err := s.pool.QueryRow(ctx, `SELECT count(*) FROM schema_migrations`).Scan(&n); err != nil || n != len(files) { + t.Fatalf("schema_migrations rows = %d, want %d, %v", n, len(files), err) } if _, err := s.pool.Exec(ctx, `DELETE FROM schema_migrations`); err != nil { t.Fatal(err) @@ -97,13 +102,74 @@ func TestCountersAreBigint(t *testing.T) { t.Errorf("%s is %s", col, typ) } } - if n != 12 { - t.Errorf("found %d counter columns, want 12", n) + // 12 on the tables, 8 passed through noergler_pr_rollups. + if n != 20 { + t.Errorf("found %d counter columns, want 20", n) } } func ptr[T any](v T) *T { return &v } +// A declined PR reopened and merged emits again, and each rollup is +// cumulative. Summed over noergler_events, declined-then-merged counted the +// declined spend twice; the view keeps only the newest row per PR. +func TestNoerglerPRRollupsKeepTheNewestPerPR(t *testing.T) { + s, pool := testStore(t) + ctx := context.Background() + t0 := time.Date(2026, 4, 28, 10, 0, 0, 0, time.UTC) + rollup := func(key, outcome, cost string, at time.Time) { + t.Helper() + lines := int64(100) + if outcome == "deleted" { + lines = 0 + } + d := &parse.NoerglerDraft{ + DeliveryID: "pr_completed#" + key + "#" + outcome, EventType: "pr_completed", PRKey: key, + Outcome: ptr(outcome), CostUSD: ptr(cost), LinesAdded: ptr(lines), OccurredAt: at, Payload: []byte(`{}`), + } + if _, err := s.InsertNoergler(ctx, d, "checkout"); err != nil { + t.Fatal(err) + } + } + rollup("a#1", "declined", "1.000000", t0) + rollup("a#1", "merged", "1.500000", t0.Add(time.Hour)) + // c#3's older, costlier rollup arrives last: newest is by emit time, + // not arrival order, cost or outcome name. + rollup("c#3", "merged", "0.500000", t0.Add(time.Hour)) + rollup("c#3", "declined", "0.900000", t0) + rollup("b#2", "merged", "2.000000", t0) + // Same emit time: the later arrival wins. + rollup("d#4", "declined", "0.100000", t0) + rollup("d#4", "merged", "0.200000", t0) + // Deleted after declined carries 0 lines: the declined row keeps its size. + rollup("e#5", "declined", "0.300000", t0) + rollup("e#5", "deleted", "0.300000", t0.Add(time.Hour)) + + got := map[string]string{} + rs, err := pool.Query(ctx, `SELECT pr_key, outcome || ' ' || cost_usd::text || ' ' || lines_added + FROM noergler_pr_rollups`) + if err != nil { + t.Fatal(err) + } + for rs.Next() { + var k, v string + if err := rs.Scan(&k, &v); err != nil { + t.Fatal(err) + } + got[k] = v + } + if err := rs.Err(); err != nil { + t.Fatal(err) + } + want := map[string]string{ + "a#1": "merged 1.500000 100", "b#2": "merged 2.000000 100", "c#3": "merged 0.500000 100", + "d#4": "merged 0.200000 100", "e#5": "declined 0.300000 100", + } + if !reflect.DeepEqual(got, want) { + t.Errorf("rollups = %v, want %v", got, want) + } +} + func TestInsertsDedupeOnDeliveryID(t *testing.T) { s, pool := testStore(t) ctx := context.Background() diff --git a/docs/setup-noergler-notification.md b/docs/setup-noergler-notification.md index 12504c4..74929eb 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 | |---|---|---| -| `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) | +| `pr_completed` | When a PR reaches a terminal outcome (merged / declined / deleted); a declined PR that is reopened and merged emits again | 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 @@ -74,8 +74,12 @@ 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. `total_cost_usd` may be omitted when the sender cannot price the +`(pr_key, outcome)` is the idempotency key, so noergler may safely retry. A +rollup is cumulative over the PR's life, and a declined PR that is reopened +and merged emits again, so one PR can have two rows (declined, then merged). Read spend through the `noergler_pr_rollups` view, the newest row per +PR; summing `noergler_events` counts the earlier row twice. A PR declined again +(or deleted) after a reopen sends no second rollup, so the spend after the +first decline is missing. `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 @@ -127,8 +131,8 @@ Idempotency key is `(finding_id, verdict)`. -- 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' +FROM noergler_pr_rollups, unnest(models_used) AS m +WHERE created_at > now() - interval '7 days' GROUP BY 1 ORDER BY prs_involved DESC; @@ -137,24 +141,26 @@ 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'; +FROM noergler_pr_rollups +WHERE 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 noergler_events -WHERE event_type = 'pr_completed' AND created_at > now() - interval '7 days' +FROM noergler_pr_rollups +WHERE created_at > now() - interval '7 days' GROUP BY 1; -- 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. +-- per-PR denominator can drive the estimate below zero. A reopened PR's +-- findings count in the window of its newest rollup, so short windows wobble. SELECT 1.0 - ( - COUNT(*) FILTER (WHERE event_type = 'feedback' AND verdict = 'disagreed')::numeric - / NULLIF(SUM(findings_count) FILTER (WHERE event_type = 'pr_completed'), 0) -) AS precision_estimate -FROM noergler_events -WHERE created_at > now() - interval '7 days'; + (SELECT COUNT(*) FROM noergler_events + WHERE event_type = 'feedback' AND verdict = 'disagreed' + AND created_at > now() - interval '7 days')::numeric + / NULLIF((SELECT SUM(findings_count) FROM noergler_pr_rollups + WHERE created_at > now() - interval '7 days'), 0) +) AS precision_estimate; ``` ## Troubleshooting