From 0ad6a639a3449e03d00fdcc2a8d1145839d1b8cb Mon Sep 17 00:00:00 2001 From: Tamir Dresher Date: Sat, 14 Mar 2026 11:55:52 +0200 Subject: [PATCH] feat(metrics): add rework rate CLI command (5th DORA metric) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add 'squad rework' subcommand that analyzes PR rework rate by examining merged PRs for post-review commits, review cycles, and changes-requested patterns using the gh CLI. Changes: - lib/rework.js: Pure calculation functions for PR rework analysis - index.js: 'rework' CLI subcommand with --days, --limit, --json flags - templates/skills/rework-rate/SKILL.md: Agent skill for rework analysis - test/rework-rate.test.js: 10 unit tests for calculation logic - package.json: Include lib/ in published files The rework command outputs: - Per-PR rework rate (% of commits after first review) - Review cycle count (changes-requested → approved loops) - Aggregate rejection rate and average rework time - Color-coded health indicators (green/yellow/red) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- index.js | 134 +++++++++++++++++++++ lib/rework.js | 119 +++++++++++++++++++ package.json | 1 + templates/skills/rework-rate/SKILL.md | 82 +++++++++++++ test/rework-rate.test.js | 164 ++++++++++++++++++++++++++ 5 files changed, 500 insertions(+) create mode 100644 lib/rework.js create mode 100644 templates/skills/rework-rate/SKILL.md create mode 100644 test/rework-rate.test.js diff --git a/index.js b/index.js index 6f23d24ff..33d7ab499 100644 --- a/index.js +++ b/index.js @@ -3,6 +3,8 @@ const fs = require('fs'); const path = require('path'); +const { calculatePrRework, calculateReworkSummary } = require('./lib/rework'); + const GREEN = '\x1b[32m'; const RED = '\x1b[31m'; const YELLOW = '\x1b[33m'; @@ -64,6 +66,9 @@ if (cmd === '--help' || cmd === '-h' || cmd === 'help') { console.log(` Flags: --migrate-directory (rename .ai-team/ → .squad/)`); console.log(` ${BOLD}copilot${RESET} Add/remove the Copilot coding agent (@copilot)`); console.log(` Usage: copilot [--off] [--auto-assign]`); + console.log(` ${BOLD}rework${RESET} Analyze PR rework rate (the 5th DORA metric)`); + console.log(` Usage: rework [--days ] [--limit ] [--json]`); + console.log(` Default: last 30 days, up to 20 PRs`); console.log(` ${BOLD}watch${RESET} Run Ralph's work monitor as a local polling process`); console.log(` Usage: watch [--interval ]`); console.log(` Default: checks every 10 minutes (Ctrl+C to stop)`); @@ -100,6 +105,135 @@ function copyRecursive(src, target) { } +// --- Rework Rate subcommand --- +if (cmd === 'rework') { + const { execSync } = require('child_process'); + + // Verify gh CLI is available + try { + execSync('gh --version', { stdio: 'pipe' }); + } catch { + fatal('gh CLI not found — install from https://cli.github.com'); + } + + // Parse flags + const daysIdx = process.argv.indexOf('--days'); + const lookbackDays = (daysIdx !== -1 && process.argv[daysIdx + 1]) + ? parseInt(process.argv[daysIdx + 1], 10) : 30; + const limitIdx = process.argv.indexOf('--limit'); + const prLimit = (limitIdx !== -1 && process.argv[limitIdx + 1]) + ? parseInt(process.argv[limitIdx + 1], 10) : 20; + const jsonOutput = process.argv.includes('--json'); + + if (isNaN(lookbackDays) || lookbackDays < 1) fatal('--days must be a positive number'); + if (isNaN(prLimit) || prLimit < 1) fatal('--limit must be a positive number'); + + const sinceDate = new Date(Date.now() - lookbackDays * 86400000).toISOString().split('T')[0]; + + if (!jsonOutput) { + console.log(`\n${BOLD}📊 Rework Rate Analysis${RESET}`); + console.log(`${DIM}Analyzing merged PRs from the last ${lookbackDays} days (limit: ${prLimit})...${RESET}\n`); + } + + // Fetch merged PRs + let prs; + try { + const prJson = execSync( + `gh pr list --state merged --limit ${prLimit} --search "merged:>=${sinceDate}" --json number,title,author,mergedAt,additions,deletions,changedFiles`, + { encoding: 'utf8', timeout: 30000, stdio: ['pipe', 'pipe', 'pipe'] } + ); + prs = JSON.parse(prJson || '[]'); + } catch (err) { + fatal(`Failed to fetch PRs: ${err.message}`); + } + + if (prs.length === 0) { + if (jsonOutput) { + console.log(JSON.stringify({ prs: [], summary: { totalPrs: 0 } }, null, 2)); + } else { + console.log(`${DIM}No merged PRs found in the last ${lookbackDays} days.${RESET}`); + } + process.exit(0); + } + + // Analyze each PR for rework + const results = []; + for (const pr of prs) { + let reviews, commits; + try { + const prDetail = execSync( + `gh pr view ${pr.number} --json reviews,commits`, + { encoding: 'utf8', timeout: 15000, stdio: ['pipe', 'pipe', 'pipe'] } + ); + const detail = JSON.parse(prDetail); + reviews = detail.reviews || []; + commits = detail.commits || []; + } catch { + // Skip PRs we can't fetch details for + continue; + } + + const rework = calculatePrRework(pr, reviews, commits); + results.push(rework); + } + + // Calculate aggregate metrics + const summary = calculateReworkSummary(results); + + if (jsonOutput) { + console.log(JSON.stringify({ prs: results, summary }, null, 2)); + } else { + printReworkReport(results, summary, lookbackDays); + } + + process.exit(0); +} + +/** + * Print human-readable rework report. + */ +function printReworkReport(results, summary, lookbackDays) { + console.log(`${BOLD}Summary (${summary.totalPrs} PRs, last ${lookbackDays} days)${RESET}`); + console.log(`${'─'.repeat(50)}`); + console.log(` Average rework rate: ${colorReworkRate(summary.avgReworkRate)}%`); + console.log(` PRs with rework: ${summary.prsWithRework}/${summary.totalPrs} (${Math.round(summary.prsWithRework / summary.totalPrs * 100)}%)`); + console.log(` Rejection rate: ${summary.rejectionRate}%`); + console.log(` Avg review cycles: ${summary.avgReviewCycles}`); + console.log(` Total rework commits: ${summary.totalReworkCommits}/${summary.totalCommits}`); + if (summary.avgReworkTimeHours !== null) { + console.log(` Avg rework time: ${summary.avgReworkTimeHours}h`); + } + console.log(); + + // Per-PR breakdown + console.log(`${BOLD}Per-PR Breakdown${RESET}`); + console.log(`${'─'.repeat(80)}`); + + for (const r of results) { + const rateLabel = colorReworkRate(r.reworkRate); + const changeLabel = r.hadChangesRequested ? `${YELLOW}⚑${RESET}` : ' '; + console.log(` #${String(r.number).padEnd(5)} ${rateLabel.padEnd(20)}% rework ${r.reviewCycles} cycles ${changeLabel} ${DIM}${r.title.substring(0, 40)}${RESET}`); + } + + console.log(); + + // Interpretation guide + if (summary.avgReworkRate <= 15) { + console.log(`${GREEN}✓${RESET} Rework rate is healthy. Code quality and review process are strong.`); + } else if (summary.avgReworkRate <= 30) { + console.log(`${YELLOW}⚠${RESET} Moderate rework rate. Consider improving PR descriptions or pre-review checks.`); + } else { + console.log(`${RED}✗${RESET} High rework rate. Review process may need attention — consider smaller PRs, clearer specs, or pair reviews.`); + } + console.log(); +} + +function colorReworkRate(rate) { + if (rate <= 15) return `${GREEN}${rate}${RESET}`; + if (rate <= 30) return `${YELLOW}${rate}${RESET}`; + return `${RED}${rate}${RESET}`; +} + // --- Watch subcommand (Ralph local watchdog) --- if (cmd === 'watch') { const { execSync } = require('child_process'); diff --git a/lib/rework.js b/lib/rework.js new file mode 100644 index 000000000..346b8b35a --- /dev/null +++ b/lib/rework.js @@ -0,0 +1,119 @@ +/** + * Rework Rate — Calculation helpers. + * + * Pure functions for computing PR rework metrics. + * No I/O, no side effects — safe to require() in tests. + */ + +/** + * Calculate rework metrics for a single PR. + * Rework = commits pushed after the first review. + */ +function calculatePrRework(pr, reviews, commits) { + // Sort reviews by timestamp + const sortedReviews = reviews + .filter(r => r.submittedAt) + .sort((a, b) => new Date(a.submittedAt).getTime() - new Date(b.submittedAt).getTime()); + + // Sort commits by timestamp + const sortedCommits = commits + .filter(c => c.committedDate || (c.commit && c.commit.committedDate)) + .sort((a, b) => { + const dateA = a.committedDate || (a.commit && a.commit.committedDate); + const dateB = b.committedDate || (b.commit && b.commit.committedDate); + return new Date(dateA).getTime() - new Date(dateB).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 = null; + let lastApproval = 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 + const postReviewCommits = firstReviewTime + ? sortedCommits.filter(c => { + const d = c.committedDate || (c.commit && c.commit.committedDate); + return new Date(d).getTime() > firstReviewTime; + }) + : []; + + const totalCommits = sortedCommits.length; + const reworkCommits = postReviewCommits.length; + const reworkRate = totalCommits > 0 ? reworkCommits / totalCommits : 0; + + // Rework time (ms) + const reworkTimeMs = (firstChangesRequested && lastApproval) + ? new Date(lastApproval).getTime() - new Date(firstChangesRequested).getTime() + : null; + + return { + number: pr.number, + title: pr.title, + author: pr.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. + */ +function calculateReworkSummary(results) { + 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.reworkTimeMs !== null).map(r => r.reworkTimeMs); + const avgReworkTimeHours = reworkTimes.length > 0 + ? +(reworkTimes.reduce((s, t) => s + t, 0) / reworkTimes.length / 3600000).toFixed(1) + : null; + + return { + totalPrs, + avgReworkRate, + prsWithRework, + prsWithChangesRequested, + avgReviewCycles, + totalReworkCommits, + totalCommits, + avgReworkTimeHours, + rejectionRate: Math.round((prsWithChangesRequested / totalPrs) * 100), + }; +} + +module.exports = { calculatePrRework, calculateReworkSummary }; diff --git a/package.json b/package.json index c90adacf4..7d323d54d 100644 --- a/package.json +++ b/package.json @@ -7,6 +7,7 @@ }, "files": [ "index.js", + "lib/**/*", ".github/agents/squad.agent.md", "templates/**/*", "CHANGELOG.md" diff --git a/templates/skills/rework-rate/SKILL.md b/templates/skills/rework-rate/SKILL.md new file mode 100644 index 000000000..2a80cf46e --- /dev/null +++ b/templates/skills/rework-rate/SKILL.md @@ -0,0 +1,82 @@ +--- +name: "rework-rate" +description: "Measure and interpret PR rework rate — the emerging 5th DORA metric" +domain: "metrics, code-review, quality" +confidence: "high" +source: "manual" +tools: + - name: "squad rework" + description: "Analyze PR rework rate from merged PRs" + when: "When measuring code quality, review efficiency, or team health metrics" +--- + +## Context + +Rework Rate measures the percentage of code changes that require revision after initial review submission. It is considered the emerging 5th DORA metric alongside Deployment Frequency, Lead Time for Changes, Mean Time to Recovery, and Change Failure Rate. + +Use this skill when: +- Measuring team code review efficiency +- Identifying patterns in PR revision cycles +- Coaching on PR quality and review practices +- Tracking AI-generated code retention rates +- Building engineering health dashboards + +## Patterns + +### Calculation + +``` +Rework Rate = (commits after first review) / (total commits) × 100 + +Sub-metrics: + Review Cycles = number of changes-requested → push → approval loops + Rejection Rate = PRs with ≥1 "changes requested" / total PRs × 100 + Rework Time = last approval timestamp - first changes-requested timestamp +``` + +### Healthy Ranges + +| Metric | 🟢 Healthy | 🟡 Moderate | 🔴 Needs Attention | +|--------|-----------|------------|-------------------| +| Rework Rate | ≤15% | 15–30% | >30% | +| Review Cycles | ≤1.0 | 1.0–2.0 | >2.0 | +| Rejection Rate | ≤20% | 20–40% | >40% | + +### Using the CLI + +```bash +# Analyze last 30 days (default) +npx github:bradygaster/squad rework + +# Custom period +npx github:bradygaster/squad rework --days 7 --limit 50 + +# Machine-readable output +npx github:bradygaster/squad rework --json +``` + +### Improvement Strategies + +When rework rate is high: +1. **Smaller PRs** — Break large changes into focused, reviewable units +2. **Clear specs** — Write acceptance criteria before coding +3. **Pre-review checklist** — Self-review before requesting reviews +4. **Pair reviews** — Complex changes benefit from synchronous review +5. **Draft PRs** — Use draft status for early feedback on direction + +## Examples + +```javascript +// Ralph can use rework metrics in watch cycles: +// "📊 Weekly Rework Rate: 12% (healthy) — 3/25 PRs had changes requested" + +// In squad ceremonies, rework trends inform retrospectives: +// "Rework rate dropped from 28% to 14% after adopting smaller PR sizes" +``` + +## Anti-Patterns + +- **Using rework rate to blame individuals** — Use it for team coaching, not performance reviews +- **Optimizing for zero rework** — Some rework is healthy; it means reviews are catching issues +- **Ignoring context** — Large refactors naturally have higher rework; compare like with like +- **Measuring without acting** — Metrics without follow-up actions are waste diff --git a/test/rework-rate.test.js b/test/rework-rate.test.js new file mode 100644 index 000000000..3b6c883fe --- /dev/null +++ b/test/rework-rate.test.js @@ -0,0 +1,164 @@ +const { describe, it } = require('node:test'); +const assert = require('node:assert/strict'); +const path = require('path'); +const { execFileSync } = require('child_process'); + +const CLI = path.join(__dirname, '..', 'index.js'); + +// Import rework calculation functions from the pure module (no CLI side effects) +const { calculatePrRework, calculateReworkSummary } = require(path.join(__dirname, '..', 'lib', 'rework.js')); + +describe('calculatePrRework', () => { + it('should return zero rework for PR with no reviews', () => { + const pr = { number: 1, title: 'test', author: { login: 'alice' }, additions: 10, deletions: 5 }; + const reviews = []; + const commits = [ + { committedDate: '2025-01-01T10:00:00Z', oid: 'abc' }, + { committedDate: '2025-01-01T11:00:00Z', oid: 'def' }, + ]; + + const result = calculatePrRework(pr, reviews, commits); + assert.equal(result.reworkRate, 0); + assert.equal(result.reworkCommits, 0); + assert.equal(result.totalCommits, 2); + assert.equal(result.reviewCycles, 0); + assert.equal(result.hadChangesRequested, false); + assert.equal(result.reworkTimeMs, null); + }); + + it('should detect rework when commits are pushed after first review', () => { + const pr = { number: 2, title: 'feat', author: { login: 'bob' }, additions: 20, deletions: 3 }; + const reviews = [ + { submittedAt: '2025-01-01T12:00:00Z', state: 'CHANGES_REQUESTED' }, + { submittedAt: '2025-01-01T15:00:00Z', state: 'APPROVED' }, + ]; + const commits = [ + { 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); + assert.equal(result.reworkRate, 50); // 2 of 4 commits + assert.equal(result.reworkCommits, 2); + assert.equal(result.totalCommits, 4); + assert.equal(result.reviewCycles, 1); + assert.equal(result.hadChangesRequested, true); + assert.ok(result.reworkTimeMs > 0); + }); + + it('should count multiple review cycles', () => { + const pr = { number: 3, title: 'fix', author: { login: 'carol' }, additions: 5, deletions: 1 }; + const reviews = [ + { 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 = [ + { committedDate: '2025-01-01T09:00:00Z', oid: 'c1' }, + ]; + + const result = calculatePrRework(pr, reviews, commits); + assert.equal(result.reviewCycles, 2); + assert.equal(result.hadChangesRequested, true); + }); + + it('should handle PR approved without changes requested', () => { + const pr = { number: 4, title: 'docs', author: { login: 'dave' }, additions: 2, deletions: 0 }; + const reviews = [ + { submittedAt: '2025-01-01T10:00:00Z', state: 'APPROVED' }, + ]; + const commits = [ + { committedDate: '2025-01-01T09:00:00Z', oid: 'd1' }, + ]; + + const result = calculatePrRework(pr, reviews, commits); + assert.equal(result.reworkRate, 0); + assert.equal(result.reviewCycles, 0); + assert.equal(result.hadChangesRequested, false); + assert.equal(result.reworkTimeMs, null); + }); + + it('should handle commits with nested commit object', () => { + const pr = { number: 5, title: 'test nested', author: { login: 'eve' }, additions: 1, deletions: 1 }; + const reviews = [ + { submittedAt: '2025-01-01T12:00:00Z', state: 'COMMENTED' }, + ]; + const commits = [ + { 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); + assert.equal(result.reworkCommits, 1); + assert.equal(result.totalCommits, 2); + assert.equal(result.reworkRate, 50); + }); + + it('should handle empty commits', () => { + const pr = { number: 6, title: 'empty', author: { login: 'frank' }, additions: 0, deletions: 0 }; + const result = calculatePrRework(pr, [], []); + assert.equal(result.reworkRate, 0); + assert.equal(result.totalCommits, 0); + assert.equal(result.reworkCommits, 0); + }); +}); + +describe('calculateReworkSummary', () => { + it('should return empty summary for no results', () => { + const summary = calculateReworkSummary([]); + assert.equal(summary.totalPrs, 0); + }); + + it('should calculate correct aggregate metrics', () => { + const results = [ + { number: 1, reworkRate: 0, reviewCycles: 0, hadChangesRequested: false, reworkCommits: 0, totalCommits: 3, reworkTimeMs: null }, + { number: 2, reworkRate: 50, reviewCycles: 1, hadChangesRequested: true, reworkCommits: 2, totalCommits: 4, reworkTimeMs: 7200000 }, + { number: 3, reworkRate: 25, reviewCycles: 1, hadChangesRequested: true, reworkCommits: 1, totalCommits: 4, reworkTimeMs: 3600000 }, + ]; + + const summary = calculateReworkSummary(results); + assert.equal(summary.totalPrs, 3); + assert.equal(summary.avgReworkRate, 25); // (0+50+25)/3 = 25 + assert.equal(summary.prsWithRework, 2); + assert.equal(summary.prsWithChangesRequested, 2); + assert.equal(summary.avgReviewCycles, +(2 / 3).toFixed(1)); + assert.equal(summary.totalReworkCommits, 3); + assert.equal(summary.totalCommits, 11); + assert.equal(summary.rejectionRate, 67); // 2/3 * 100 rounded + assert.equal(summary.avgReworkTimeHours, 1.5); // (7200000+3600000)/2/3600000 + }); + + it('should handle all clean PRs', () => { + const results = [ + { number: 1, reworkRate: 0, reviewCycles: 0, hadChangesRequested: false, reworkCommits: 0, totalCommits: 2, reworkTimeMs: null }, + { number: 2, reworkRate: 0, reviewCycles: 0, hadChangesRequested: false, reworkCommits: 0, totalCommits: 1, reworkTimeMs: null }, + ]; + + const summary = calculateReworkSummary(results); + assert.equal(summary.avgReworkRate, 0); + assert.equal(summary.prsWithRework, 0); + assert.equal(summary.rejectionRate, 0); + assert.equal(summary.avgReworkTimeHours, null); + }); +}); + +describe('rework CLI command', () => { + it('should show help for rework in --help output', () => { + try { + const result = execFileSync(process.execPath, [CLI, '--help'], { + encoding: 'utf8', + timeout: 10000, + env: { ...process.env, NO_COLOR: '1' }, + }); + assert.ok(result.includes('rework'), 'Help should mention rework command'); + assert.ok(result.includes('5th DORA'), 'Help should mention 5th DORA metric'); + } catch (err) { + // --help exits with 0, but execFileSync may not throw + const output = (err.stdout || '') + (err.stderr || ''); + assert.ok(output.includes('rework'), 'Help should mention rework command'); + } + }); +});