From c654cf72b0ed5ca0baa38d73928ff76a7c42781c Mon Sep 17 00:00:00 2001 From: Tamir Dresher Date: Sun, 15 Mar 2026 16:05:44 +0200 Subject: [PATCH] =?UTF-8?q?feat(metrics):=20Add=20rework=20rate=20OTEL=20m?= =?UTF-8?q?etrics=20(#265)=20=E2=80=94=205th=20DORA=20metric?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add PR rework rate as OTEL metrics in squad-sdk, following the exact pattern of existing metrics (#261-#264) in otel-metrics.ts. New modules: - runtime/rework.ts: Pure calculation module with typed interfaces (PrInfo, PrReview, PrCommit, PrReworkResult, ReworkSummary) and functions (calculatePrRework, calculateReworkSummary) - otel-metrics.ts #265 section: Four OTEL instruments - squad.rework.rate (Gauge) — rework rate percentage - squad.rework.cycles (Histogram) — review cycles per PR - squad.rework.rejection_rate (Gauge) — rejection percentage - squad.rework.time_ms (Histogram) — time spent in rework - Export functions: recordReworkMetrics(), recordReworkSummary() Includes 19 Vitest tests (9 for calculatePrRework, 4 for calculateReworkSummary, 6 for OTEL metric recording). Replaces #381 (CLI command approach) with proper SDK-level OTEL metrics. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .changeset/add-rework-rate-metric.md | 5 + packages/squad-sdk/src/index.ts | 1 + .../squad-sdk/src/runtime/otel-metrics.ts | 70 +++ packages/squad-sdk/src/runtime/rework.ts | 180 ++++++++ test/rework.test.ts | 417 ++++++++++++++++++ 5 files changed, 673 insertions(+) create mode 100644 .changeset/add-rework-rate-metric.md create mode 100644 packages/squad-sdk/src/runtime/rework.ts create mode 100644 test/rework.test.ts diff --git a/.changeset/add-rework-rate-metric.md b/.changeset/add-rework-rate-metric.md new file mode 100644 index 000000000..20d83e64c --- /dev/null +++ b/.changeset/add-rework-rate-metric.md @@ -0,0 +1,5 @@ +--- +"@bradygaster/squad-sdk": minor +--- + +Add rework rate OTEL metrics (#265) — tracks PR revision patterns as the emerging 5th DORA metric diff --git a/packages/squad-sdk/src/index.ts b/packages/squad-sdk/src/index.ts index 83d16833d..c42244c8c 100644 --- a/packages/squad-sdk/src/index.ts +++ b/packages/squad-sdk/src/index.ts @@ -30,6 +30,7 @@ export * from './runtime/i18n.js'; export * from './runtime/benchmarks.js'; export * from './runtime/otel-init.js'; export * from './runtime/otel-metrics.js'; +export * from './runtime/rework.js'; export { getMeter, getTracer } from './runtime/otel.js'; export { safeTimestamp } from './utils/safe-timestamp.js'; export { EventBus as RuntimeEventBus } from './runtime/event-bus.js'; diff --git a/packages/squad-sdk/src/runtime/otel-metrics.ts b/packages/squad-sdk/src/runtime/otel-metrics.ts index 3c21f874a..c8e6cb144 100644 --- a/packages/squad-sdk/src/runtime/otel-metrics.ts +++ b/packages/squad-sdk/src/runtime/otel-metrics.ts @@ -10,6 +10,7 @@ import { getMeter } from './otel.js'; import type { UsageEvent } from './streaming.js'; +import type { PrReworkResult, ReworkSummary } from './rework.js'; // ============================================================================ // #261 — Token Usage Metrics @@ -251,6 +252,74 @@ export function recordTokensPerSecond(tokensPerSec: number): void { m.tokensPerSecGauge.record(tokensPerSec); } +// ============================================================================ +// #265 — Rework Rate Metrics +// ============================================================================ + +interface ReworkMetrics { + rateGauge: ReturnType['createGauge']>; + cyclesHistogram: ReturnType['createHistogram']>; + rejectionRateGauge: ReturnType['createGauge']>; + timeHistogram: ReturnType['createHistogram']>; +} + +let _reworkMetrics: ReworkMetrics | undefined; + +function ensureReworkMetrics(): ReworkMetrics { + if (!_reworkMetrics) { + const meter = getMeter('squad-sdk'); + _reworkMetrics = { + rateGauge: meter.createGauge('squad.rework.rate', { + description: 'Current rework rate percentage', + unit: '%', + }), + cyclesHistogram: meter.createHistogram('squad.rework.cycles', { + description: 'Review cycles per PR', + }), + rejectionRateGauge: meter.createGauge('squad.rework.rejection_rate', { + description: 'Percentage of PRs with changes requested', + unit: '%', + }), + timeHistogram: meter.createHistogram('squad.rework.time_ms', { + description: 'Time spent in rework in milliseconds', + unit: 'ms', + }), + }; + } + return _reworkMetrics; +} + +/** Record rework metrics for a single PR analysis result. */ +export function recordReworkMetrics(result: PrReworkResult): void { + const m = ensureReworkMetrics(); + const attrs = { + 'pr.number': result.number, + 'pr.author': result.author, + }; + m.rateGauge.record(result.reworkRate, attrs); + m.cyclesHistogram.record(result.reviewCycles, attrs); + if (result.reworkTimeMs !== null) { + m.timeHistogram.record(result.reworkTimeMs, attrs); + } +} + +/** Record aggregate rework summary metrics across multiple PRs. */ +export function recordReworkSummary(summary: ReworkSummary): void { + const m = ensureReworkMetrics(); + if (summary.avgReworkRate != null) { + m.rateGauge.record(summary.avgReworkRate, { scope: 'summary' }); + } + if (summary.rejectionRate != null) { + m.rejectionRateGauge.record(summary.rejectionRate, { scope: 'summary' }); + } + if (summary.avgReviewCycles != null) { + m.cyclesHistogram.record(summary.avgReviewCycles, { scope: 'summary' }); + } + if (summary.avgReworkTimeHours != null) { + m.timeHistogram.record(summary.avgReworkTimeHours * 3_600_000, { scope: 'summary' }); + } +} + // ============================================================================ // Reset (for testing) // ============================================================================ @@ -261,4 +330,5 @@ export function _resetMetrics(): void { _agentMetrics = undefined; _sessionPoolMetrics = undefined; _latencyMetrics = undefined; + _reworkMetrics = undefined; } diff --git a/packages/squad-sdk/src/runtime/rework.ts b/packages/squad-sdk/src/runtime/rework.ts new file mode 100644 index 000000000..57c2a5056 --- /dev/null +++ b/packages/squad-sdk/src/runtime/rework.ts @@ -0,0 +1,180 @@ +/** + * Rework Rate — Pure calculation helpers. + * + * Computes PR rework metrics from review and commit data. + * No I/O, no side effects — safe to import in tests. + * + * @module runtime/rework + */ + +/** Minimal PR shape from `gh pr list --json ...` */ +export interface PrInfo { + number: number; + title: string; + author?: { login: string }; + mergedAt?: string; + additions?: number; + deletions?: number; + changedFiles?: number; +} + +/** Review data from `gh pr view --json reviews` */ +export interface PrReview { + submittedAt?: string; + state?: string; +} + +/** Commit data from `gh pr view --json commits` */ +export interface PrCommit { + committedDate?: string; + commit?: { committedDate?: string }; + oid?: string; +} + +/** Per-PR rework analysis result */ +export interface PrReworkResult { + number: number; + title: string; + author: string; + mergedAt: string | undefined; + totalCommits: number; + reworkCommits: number; + reworkRate: number; + reviewCycles: number; + hadChangesRequested: boolean; + reworkTimeMs: number | null; + totalReviews: number; + additions: number; + deletions: number; +} + +/** Aggregate summary across analyzed PRs */ +export interface ReworkSummary { + totalPrs: number; + avgReworkRate?: number; + prsWithRework?: number; + prsWithChangesRequested?: number; + avgReviewCycles?: number; + totalReworkCommits?: number; + totalCommits?: number; + avgReworkTimeHours?: number | null; + rejectionRate?: number; +} + +function getCommitDate(c: PrCommit): string | undefined { + return c.committedDate ?? c.commit?.committedDate; +} + +/** + * Calculate rework metrics for a single PR. + * Rework = commits pushed after the first review. + */ +export function calculatePrRework( + pr: PrInfo, + reviews: PrReview[], + commits: PrCommit[], +): PrReworkResult { + const sortedReviews = reviews + .filter((r): r is PrReview & { submittedAt: string } => !!r.submittedAt) + .sort((a, b) => new Date(a.submittedAt).getTime() - new Date(b.submittedAt).getTime()); + + const sortedCommits = commits + .filter((c) => !!getCommitDate(c)) + .sort((a, b) => new Date(getCommitDate(a)!).getTime() - new Date(getCommitDate(b)!).getTime()); + + const firstReview = sortedReviews[0]; + const firstReviewTime = firstReview ? new Date(firstReview.submittedAt).getTime() : null; + + // Count review cycles: changes-requested → approved transitions + let reviewCycles = 0; + let hadChangesRequested = false; + let firstChangesRequested: string | null = null; + let lastApproval: string | null = null; + let pendingChangeRequest = false; + + for (const review of sortedReviews) { + const state = (review.state ?? '').toUpperCase(); + if (state === 'CHANGES_REQUESTED') { + hadChangesRequested = true; + pendingChangeRequest = true; + if (!firstChangesRequested) firstChangesRequested = review.submittedAt; + } else if (state === 'APPROVED' && pendingChangeRequest) { + reviewCycles++; + pendingChangeRequest = false; + lastApproval = review.submittedAt; + } else if (state === 'APPROVED') { + lastApproval = review.submittedAt; + } + } + + // Count post-review commits (rework) + const postReviewCommits = firstReviewTime + ? sortedCommits.filter((c) => new Date(getCommitDate(c)!).getTime() > firstReviewTime) + : []; + + const totalCommits = sortedCommits.length; + const reworkCommits = postReviewCommits.length; + const reworkRate = totalCommits > 0 ? reworkCommits / totalCommits : 0; + + // Rework time: first changes-requested to last approval + const reworkTimeMs = + firstChangesRequested && lastApproval + ? new Date(lastApproval).getTime() - new Date(firstChangesRequested).getTime() + : null; + + return { + number: pr.number, + title: pr.title, + author: pr.author?.login ?? 'unknown', + mergedAt: pr.mergedAt, + totalCommits, + reworkCommits, + reworkRate: Math.round(reworkRate * 100), + reviewCycles, + hadChangesRequested, + reworkTimeMs, + totalReviews: sortedReviews.length, + additions: pr.additions ?? 0, + deletions: pr.deletions ?? 0, + }; +} + +/** + * Calculate aggregate rework summary across all analyzed PRs. + */ +export function calculateReworkSummary(results: PrReworkResult[]): ReworkSummary { + if (results.length === 0) return { totalPrs: 0 }; + + const totalPrs = results.length; + const avgReworkRate = Math.round( + results.reduce((s, r) => s + r.reworkRate, 0) / totalPrs, + ); + const prsWithRework = results.filter((r) => r.reworkRate > 0).length; + const prsWithChangesRequested = results.filter((r) => r.hadChangesRequested).length; + const avgReviewCycles = +( + results.reduce((s, r) => s + r.reviewCycles, 0) / totalPrs + ).toFixed(1); + const totalReworkCommits = results.reduce((s, r) => s + r.reworkCommits, 0); + const totalCommits = results.reduce((s, r) => s + r.totalCommits, 0); + + // Average rework time (only for PRs that had rework time) + const reworkTimes = results + .filter((r): r is PrReworkResult & { reworkTimeMs: number } => r.reworkTimeMs !== null) + .map((r) => r.reworkTimeMs); + const avgReworkTimeHours = + reworkTimes.length > 0 + ? +(reworkTimes.reduce((s, t) => s + t, 0) / reworkTimes.length / 3_600_000).toFixed(1) + : null; + + return { + totalPrs, + avgReworkRate, + prsWithRework, + prsWithChangesRequested, + avgReviewCycles, + totalReworkCommits, + totalCommits, + avgReworkTimeHours, + rejectionRate: Math.round((prsWithChangesRequested / totalPrs) * 100), + }; +} diff --git a/test/rework.test.ts b/test/rework.test.ts new file mode 100644 index 000000000..7e91ce502 --- /dev/null +++ b/test/rework.test.ts @@ -0,0 +1,417 @@ +/** + * Rework Rate — Unit tests for calculation logic and OTEL metrics. + * + * Tests the pure calculation functions in runtime/rework.ts and the + * OTEL metric recording functions in otel-metrics.ts. + * No gh CLI calls needed — all data is synthetic. + */ + +import { describe, it, expect, beforeEach, vi, type Mock } from 'vitest'; +import { + calculatePrRework, + calculateReworkSummary, + recordReworkMetrics, + recordReworkSummary, + _resetMetrics, + type PrInfo, + type PrReview, + type PrCommit, + type PrReworkResult, +} from '@bradygaster/squad-sdk'; + +// --------------------------------------------------------------------------- +// calculatePrRework +// --------------------------------------------------------------------------- + +describe('calculatePrRework', () => { + it('returns zero rework for PR with no reviews', () => { + const pr: PrInfo = { number: 1, title: 'test', author: { login: 'alice' }, additions: 10, deletions: 5 }; + const reviews: PrReview[] = []; + const commits: PrCommit[] = [ + { committedDate: '2025-01-01T10:00:00Z', oid: 'abc' }, + { committedDate: '2025-01-01T11:00:00Z', oid: 'def' }, + ]; + + const result = calculatePrRework(pr, reviews, commits); + + expect(result.reworkRate).toBe(0); + expect(result.reworkCommits).toBe(0); + expect(result.totalCommits).toBe(2); + expect(result.reviewCycles).toBe(0); + expect(result.hadChangesRequested).toBe(false); + expect(result.reworkTimeMs).toBeNull(); + }); + + it('detects rework when commits are pushed after first review', () => { + const pr: PrInfo = { number: 2, title: 'feat', author: { login: 'bob' }, additions: 20, deletions: 3 }; + const reviews: PrReview[] = [ + { submittedAt: '2025-01-01T12:00:00Z', state: 'CHANGES_REQUESTED' }, + { submittedAt: '2025-01-01T15:00:00Z', state: 'APPROVED' }, + ]; + const commits: PrCommit[] = [ + { committedDate: '2025-01-01T10:00:00Z', oid: 'a1' }, + { committedDate: '2025-01-01T11:00:00Z', oid: 'a2' }, + { committedDate: '2025-01-01T13:00:00Z', oid: 'a3' }, // post-review + { committedDate: '2025-01-01T14:00:00Z', oid: 'a4' }, // post-review + ]; + + const result = calculatePrRework(pr, reviews, commits); + + expect(result.reworkRate).toBe(50); // 2 of 4 commits + expect(result.reworkCommits).toBe(2); + expect(result.totalCommits).toBe(4); + expect(result.reviewCycles).toBe(1); + expect(result.hadChangesRequested).toBe(true); + expect(result.reworkTimeMs).toBeGreaterThan(0); + }); + + it('counts multiple review cycles', () => { + const pr: PrInfo = { number: 3, title: 'fix', author: { login: 'carol' }, additions: 5, deletions: 1 }; + const reviews: PrReview[] = [ + { submittedAt: '2025-01-01T10:00:00Z', state: 'CHANGES_REQUESTED' }, + { submittedAt: '2025-01-01T12:00:00Z', state: 'APPROVED' }, + { submittedAt: '2025-01-01T14:00:00Z', state: 'CHANGES_REQUESTED' }, + { submittedAt: '2025-01-01T16:00:00Z', state: 'APPROVED' }, + ]; + const commits: PrCommit[] = [ + { committedDate: '2025-01-01T09:00:00Z', oid: 'c1' }, + ]; + + const result = calculatePrRework(pr, reviews, commits); + + expect(result.reviewCycles).toBe(2); + expect(result.hadChangesRequested).toBe(true); + }); + + it('handles PR approved without changes requested', () => { + const pr: PrInfo = { number: 4, title: 'docs', author: { login: 'dave' }, additions: 2, deletions: 0 }; + const reviews: PrReview[] = [ + { submittedAt: '2025-01-01T10:00:00Z', state: 'APPROVED' }, + ]; + const commits: PrCommit[] = [ + { committedDate: '2025-01-01T09:00:00Z', oid: 'd1' }, + ]; + + const result = calculatePrRework(pr, reviews, commits); + + expect(result.reworkRate).toBe(0); + expect(result.reviewCycles).toBe(0); + expect(result.hadChangesRequested).toBe(false); + expect(result.reworkTimeMs).toBeNull(); + }); + + it('handles commits with nested commit object', () => { + const pr: PrInfo = { number: 5, title: 'test nested', author: { login: 'eve' }, additions: 1, deletions: 1 }; + const reviews: PrReview[] = [ + { submittedAt: '2025-01-01T12:00:00Z', state: 'COMMENTED' }, + ]; + const commits: PrCommit[] = [ + { commit: { committedDate: '2025-01-01T10:00:00Z' }, oid: 'e1' }, + { commit: { committedDate: '2025-01-01T14:00:00Z' }, oid: 'e2' }, // post-review + ]; + + const result = calculatePrRework(pr, reviews, commits); + + expect(result.reworkCommits).toBe(1); + expect(result.totalCommits).toBe(2); + expect(result.reworkRate).toBe(50); + }); + + it('handles empty commits', () => { + const pr: PrInfo = { number: 6, title: 'empty', author: { login: 'frank' }, additions: 0, deletions: 0 }; + + const result = calculatePrRework(pr, [], []); + + expect(result.reworkRate).toBe(0); + expect(result.totalCommits).toBe(0); + expect(result.reworkCommits).toBe(0); + }); + + it('handles missing author', () => { + const pr: PrInfo = { number: 7, title: 'no author' }; + + const result = calculatePrRework(pr, [], []); + + expect(result.author).toBe('unknown'); + expect(result.additions).toBe(0); + expect(result.deletions).toBe(0); + }); + + it('filters reviews without submittedAt', () => { + const pr: PrInfo = { number: 8, title: 'partial reviews', author: { login: 'gina' } }; + const reviews: PrReview[] = [ + { state: 'APPROVED' }, // no submittedAt — should be ignored + { submittedAt: '2025-01-01T12:00:00Z', state: 'APPROVED' }, + ]; + const commits: PrCommit[] = [ + { committedDate: '2025-01-01T10:00:00Z', oid: 'g1' }, + ]; + + const result = calculatePrRework(pr, reviews, commits); + + expect(result.totalReviews).toBe(1); + expect(result.reworkRate).toBe(0); + }); + + it('correctly computes rework time in ms', () => { + const pr: PrInfo = { number: 9, title: 'rework time', author: { login: 'hank' } }; + const reviews: PrReview[] = [ + { submittedAt: '2025-01-01T10:00:00Z', state: 'CHANGES_REQUESTED' }, + { submittedAt: '2025-01-01T13:00:00Z', state: 'APPROVED' }, // 3 hours later + ]; + const commits: PrCommit[] = [ + { committedDate: '2025-01-01T09:00:00Z', oid: 'h1' }, + ]; + + const result = calculatePrRework(pr, reviews, commits); + + expect(result.reworkTimeMs).toBe(3 * 3_600_000); // 3 hours + }); +}); + +// --------------------------------------------------------------------------- +// calculateReworkSummary +// --------------------------------------------------------------------------- + +describe('calculateReworkSummary', () => { + it('returns empty summary for no results', () => { + const summary = calculateReworkSummary([]); + + expect(summary.totalPrs).toBe(0); + }); + + it('calculates correct aggregate metrics', () => { + const results: PrReworkResult[] = [ + { + number: 1, title: 'a', author: 'alice', mergedAt: undefined, + reworkRate: 0, reviewCycles: 0, hadChangesRequested: false, + reworkCommits: 0, totalCommits: 3, reworkTimeMs: null, + totalReviews: 1, additions: 10, deletions: 5, + }, + { + number: 2, title: 'b', author: 'bob', mergedAt: undefined, + reworkRate: 50, reviewCycles: 1, hadChangesRequested: true, + reworkCommits: 2, totalCommits: 4, reworkTimeMs: 7_200_000, + totalReviews: 2, additions: 20, deletions: 3, + }, + { + number: 3, title: 'c', author: 'carol', mergedAt: undefined, + reworkRate: 25, reviewCycles: 1, hadChangesRequested: true, + reworkCommits: 1, totalCommits: 4, reworkTimeMs: 3_600_000, + totalReviews: 2, additions: 5, deletions: 1, + }, + ]; + + const summary = calculateReworkSummary(results); + + expect(summary.totalPrs).toBe(3); + expect(summary.avgReworkRate).toBe(25); // (0+50+25)/3 = 25 + expect(summary.prsWithRework).toBe(2); + expect(summary.prsWithChangesRequested).toBe(2); + expect(summary.avgReviewCycles).toBe(+(2 / 3).toFixed(1)); + expect(summary.totalReworkCommits).toBe(3); + expect(summary.totalCommits).toBe(11); + expect(summary.rejectionRate).toBe(67); // 2/3 * 100 rounded + expect(summary.avgReworkTimeHours).toBe(1.5); // (7200000+3600000)/2/3600000 + }); + + it('handles all clean PRs', () => { + const results: PrReworkResult[] = [ + { + number: 1, title: 'a', author: 'alice', mergedAt: undefined, + reworkRate: 0, reviewCycles: 0, hadChangesRequested: false, + reworkCommits: 0, totalCommits: 2, reworkTimeMs: null, + totalReviews: 1, additions: 10, deletions: 0, + }, + { + number: 2, title: 'b', author: 'bob', mergedAt: undefined, + reworkRate: 0, reviewCycles: 0, hadChangesRequested: false, + reworkCommits: 0, totalCommits: 1, reworkTimeMs: null, + totalReviews: 1, additions: 5, deletions: 2, + }, + ]; + + const summary = calculateReworkSummary(results); + + expect(summary.avgReworkRate).toBe(0); + expect(summary.prsWithRework).toBe(0); + expect(summary.rejectionRate).toBe(0); + expect(summary.avgReworkTimeHours).toBeNull(); + }); + + it('handles single PR with high rework', () => { + const results: PrReworkResult[] = [ + { + number: 10, title: 'big refactor', author: 'dan', mergedAt: undefined, + reworkRate: 75, reviewCycles: 3, hadChangesRequested: true, + reworkCommits: 6, totalCommits: 8, reworkTimeMs: 86_400_000, + totalReviews: 6, additions: 100, deletions: 50, + }, + ]; + + const summary = calculateReworkSummary(results); + + expect(summary.totalPrs).toBe(1); + expect(summary.avgReworkRate).toBe(75); + expect(summary.prsWithRework).toBe(1); + expect(summary.rejectionRate).toBe(100); + expect(summary.avgReworkTimeHours).toBe(24); + }); +}); + +// --------------------------------------------------------------------------- +// OTEL Rework Metrics — recordReworkMetrics / recordReworkSummary +// --------------------------------------------------------------------------- + +interface SpyInstrument { + add: Mock; + record: Mock; +} + +interface SpyMeter { + createCounter: Mock; + createHistogram: Mock; + createUpDownCounter: Mock; + createGauge: Mock; + _instruments: Map; +} + +function createSpyMeter(): SpyMeter { + const instruments = new Map(); + + function makeInstrument(name: string): SpyInstrument { + const inst: SpyInstrument = { add: vi.fn(), record: vi.fn() }; + instruments.set(name, inst); + return inst; + } + + return { + createCounter: vi.fn((name: string) => makeInstrument(name)), + createHistogram: vi.fn((name: string) => makeInstrument(name)), + createUpDownCounter: vi.fn((name: string) => makeInstrument(name)), + createGauge: vi.fn((name: string) => makeInstrument(name)), + _instruments: instruments, + }; +} + +let spyMeter: SpyMeter; + +vi.mock('@bradygaster/squad-sdk/runtime/otel', () => ({ + getMeter: () => spyMeter, +})); + +function getInstrument(name: string): SpyInstrument { + const inst = spyMeter._instruments.get(name); + if (!inst) throw new Error(`No instrument created for "${name}". Created: ${[...spyMeter._instruments.keys()].join(', ')}`); + return inst; +} + +describe('OTel Rework Metrics (#265)', () => { + beforeEach(() => { + spyMeter = createSpyMeter(); + _resetMetrics(); + }); + + it('recordReworkMetrics records rate gauge and cycles histogram', () => { + const result: PrReworkResult = { + number: 42, title: 'feat: add widget', author: 'alice', mergedAt: undefined, + reworkRate: 33, reviewCycles: 2, hadChangesRequested: true, + reworkCommits: 3, totalCommits: 9, reworkTimeMs: 7_200_000, + totalReviews: 4, additions: 50, deletions: 10, + }; + + recordReworkMetrics(result); + + const rate = getInstrument('squad.rework.rate'); + const cycles = getInstrument('squad.rework.cycles'); + const time = getInstrument('squad.rework.time_ms'); + + expect(rate.record).toHaveBeenCalledWith(33, { 'pr.number': 42, 'pr.author': 'alice' }); + expect(cycles.record).toHaveBeenCalledWith(2, { 'pr.number': 42, 'pr.author': 'alice' }); + expect(time.record).toHaveBeenCalledWith(7_200_000, { 'pr.number': 42, 'pr.author': 'alice' }); + }); + + it('recordReworkMetrics skips time when reworkTimeMs is null', () => { + const result: PrReworkResult = { + number: 1, title: 'clean', author: 'bob', mergedAt: undefined, + reworkRate: 0, reviewCycles: 0, hadChangesRequested: false, + reworkCommits: 0, totalCommits: 2, reworkTimeMs: null, + totalReviews: 1, additions: 5, deletions: 0, + }; + + recordReworkMetrics(result); + + const time = getInstrument('squad.rework.time_ms'); + expect(time.record).not.toHaveBeenCalled(); + }); + + it('recordReworkSummary records aggregate metrics', () => { + const summary = { + totalPrs: 10, + avgReworkRate: 22, + prsWithRework: 5, + prsWithChangesRequested: 4, + avgReviewCycles: 1.3, + totalReworkCommits: 15, + totalCommits: 40, + avgReworkTimeHours: 2.5, + rejectionRate: 40, + }; + + recordReworkSummary(summary); + + const rate = getInstrument('squad.rework.rate'); + const rejection = getInstrument('squad.rework.rejection_rate'); + const cycles = getInstrument('squad.rework.cycles'); + const time = getInstrument('squad.rework.time_ms'); + + expect(rate.record).toHaveBeenCalledWith(22, { scope: 'summary' }); + expect(rejection.record).toHaveBeenCalledWith(40, { scope: 'summary' }); + expect(cycles.record).toHaveBeenCalledWith(1.3, { scope: 'summary' }); + expect(time.record).toHaveBeenCalledWith(2.5 * 3_600_000, { scope: 'summary' }); + }); + + it('recordReworkSummary handles empty summary gracefully', () => { + const summary = { totalPrs: 0 }; + + // Should not throw even with undefined optional fields + expect(() => recordReworkSummary(summary)).not.toThrow(); + }); + + it('creates rework instruments with correct metric names', () => { + recordReworkMetrics({ + number: 1, title: 'x', author: 'a', mergedAt: undefined, + reworkRate: 0, reviewCycles: 0, hadChangesRequested: false, + reworkCommits: 0, totalCommits: 1, reworkTimeMs: null, + totalReviews: 1, additions: 0, deletions: 0, + }); + + expect(spyMeter.createGauge).toHaveBeenCalledWith('squad.rework.rate', expect.objectContaining({ unit: '%' })); + expect(spyMeter.createHistogram).toHaveBeenCalledWith('squad.rework.cycles', expect.any(Object)); + expect(spyMeter.createGauge).toHaveBeenCalledWith('squad.rework.rejection_rate', expect.objectContaining({ unit: '%' })); + expect(spyMeter.createHistogram).toHaveBeenCalledWith('squad.rework.time_ms', expect.objectContaining({ unit: 'ms' })); + }); + + it('_resetMetrics clears rework metric instances', () => { + recordReworkMetrics({ + number: 1, title: 'x', author: 'a', mergedAt: undefined, + reworkRate: 10, reviewCycles: 1, hadChangesRequested: false, + reworkCommits: 1, totalCommits: 5, reworkTimeMs: null, + totalReviews: 1, additions: 0, deletions: 0, + }); + + _resetMetrics(); + spyMeter = createSpyMeter(); + + recordReworkMetrics({ + number: 2, title: 'y', author: 'b', mergedAt: undefined, + reworkRate: 20, reviewCycles: 0, hadChangesRequested: false, + reworkCommits: 0, totalCommits: 3, reworkTimeMs: null, + totalReviews: 1, additions: 0, deletions: 0, + }); + + const rate = getInstrument('squad.rework.rate'); + expect(rate.record).toHaveBeenCalledTimes(1); + expect(rate.record).toHaveBeenCalledWith(20, { 'pr.number': 2, 'pr.author': 'b' }); + }); +});