From 8b919847e0e48d4ac9a853913be1209123d04762 Mon Sep 17 00:00:00 2001 From: Jarred Stelfox Date: Fri, 9 Oct 2026 17:05:43 -0700 Subject: [PATCH 01/17] Model the self-review policy in derive() Developers now CR and QA their own PRs unless they ask for review, so "needs CR" no longer means someone else owes a review. derive() needs to know which PRs are the author's own job and which are waiting on people who were asked. A pull is ownReview when its author is a developer (config projects.developerTeams, or everyone when that's unset) and nothing requested a review: no person, no team, no claim. askedOf lists who was asked, with a requested GitHub team counting as everyone on the roster team of that name, and askedAt is when. Only pulls waiting on others can starve. An author's own stale stamp now owes a re-stamp, like anyone else's. Co-Authored-By: Claude Opus 5.5 --- frontend-v2/src/model/stack.test.ts | 3 + frontend-v2/src/model/status.test.ts | 90 +++++++++++++++++++++++++--- frontend-v2/src/store.ts | 12 +++- shared/model/status.ts | 88 ++++++++++++++++++++++++--- shared/types.ts | 15 +++++ 5 files changed, 190 insertions(+), 18 deletions(-) diff --git a/frontend-v2/src/model/stack.test.ts b/frontend-v2/src/model/stack.test.ts index de497c7e..341cfb75 100644 --- a/frontend-v2/src/model/stack.test.ts +++ b/frontend-v2/src/model/stack.test.ts @@ -78,6 +78,9 @@ function dp( cryo: false, changesRequestedBy: [], engagedNoStamp: [], + ownReview: false, + askedOf: [], + askedAt: null, }; } diff --git a/frontend-v2/src/model/status.test.ts b/frontend-v2/src/model/status.test.ts index 71c99478..1d8cddbb 100644 --- a/frontend-v2/src/model/status.test.ts +++ b/frontend-v2/src/model/status.test.ts @@ -3,7 +3,9 @@ import type { PullData, Signature, SignatureType } from '../../../shared/types'; import { ciVerdict, derive, + isDeveloper, isIterating, + reviewPolicy, reviewWeight, STARVE_DAYS, type Weight, @@ -140,9 +142,11 @@ describe('status derivation precedence', () => { expect(d.recrBy).toEqual([]); }); - it("the author's own stale stamp never creates a re-stamp lane entry", () => { + it("the author's own stale stamp is a re-stamp the author owes", () => { const p = withStatus({ allCR: [sig('CR', 'author', false)] }); - expect(derive(p, undefined, NOW).status).toBe('needs_cr'); + const d = derive(p, undefined, NOW); + expect(d.status).toBe('needs_recr'); + expect(d.recrBy).toEqual(['author']); }); it('CR done, QA missing → needs_qa; both → ready', () => { @@ -399,26 +403,98 @@ describe('ciVerdict', () => { }); }); +const OUTSIDER = reviewPolicy({ Devs: ['r', 'q'] }); + +describe('the self-review policy', () => { + const old = new Date((NOW - 10 * 86400) * 1000).toISOString(); + + it("a developer's unrequested pull is their own review, and never starves", () => { + const d = derive(pull({ created_at: old }), undefined, NOW); + expect(d.ownReview).toBe(true); + expect(d.status).toBe('needs_cr'); + expect(d.starved).toBe(false); + expect(d.askedOf).toEqual([]); + }); + + it('a review request hands it to the people asked, and it can starve', () => { + const p = pull({ + created_at: old, + requested_reviewers: ['r', 'claimer'], + review_requests: [ + { login: 'r', at: NOW - 3600, self: false }, + { login: 'claimer', at: NOW - 60, self: true }, + ], + }); + const d = derive(p, undefined, NOW); + expect(d.ownReview).toBe(false); + expect(d.askedOf).toEqual(['r']); + expect(d.askedAt).toBe(NOW - 3600); + expect(d.starved).toBe(true); + }); + + it('a claim alone takes it off the author too', () => { + const p = pull({ + requested_reviewers: ['claimer'], + review_requests: [{ login: 'claimer', at: NOW, self: true }], + }); + const d = derive(p, undefined, NOW); + expect(d.ownReview).toBe(false); + expect(d.askedOf).toEqual([]); + }); + + it('a team request asks everyone on the roster team of that name', () => { + const policy = reviewPolicy({ 'Store Front': ['author', 'a', 'b'], Other: ['c'] }); + const d = derive( + pull({ requested_teams: ['store-front'] }), + undefined, + NOW, + undefined, + undefined, + policy + ); + expect(d.askedOf).toEqual(['a', 'b']); + expect(d.ownReview).toBe(false); + // a team the roster doesn't know still counts as asking for review + const unknown = derive( + pull({ requested_teams: ['infra'] }), + undefined, + NOW, + undefined, + undefined, + policy + ); + expect(unknown.ownReview).toBe(false); + expect(unknown.askedOf).toEqual([]); + }); + + it("someone outside the dev team doesn't review their own", () => { + const d = derive(pull(), undefined, NOW, undefined, undefined, OUTSIDER); + expect(d.ownReview).toBe(false); + expect(isDeveloper(OUTSIDER, 'R')).toBe(true); + }); +}); + describe('starvation and weight', () => { - it('an old CR-incomplete pull is starved, whatever partial progress it has', () => { + it('an old CR-incomplete pull waiting on others is starved, whatever partial progress it has', () => { + // the author is outside the dev team, so the review is someone else's const old = new Date((NOW - 10 * 86400) * 1000).toISOString(); const p = pull({ created_at: old }); - const d = derive(p, undefined, NOW); + const d = derive(p, undefined, NOW, undefined, undefined, OUTSIDER); expect(d.starved).toBe(true); expect(d.starveScore).toBeGreaterThan(0); // half-reviewed rot: 1 of 2 CRs, still starving (the old crHave === 0 // cliff hid these from the aging lane forever) const half = withStatus({ cr_req: 2, allCR: [sig('CR', 'r', true)] }, { created_at: old }); - expect(derive(half, undefined, NOW).starved).toBe(true); + expect(derive(half, undefined, NOW, undefined, undefined, OUTSIDER).starved).toBe(true); // a stale stamp waiting on its re-CR rots too const stale = withStatus({ allCR: [sig('CR', 'r', false)] }, { created_at: old }); - expect(derive(stale, undefined, NOW).starved).toBe(true); + expect(derive(stale, undefined, NOW, undefined, undefined, OUTSIDER).starved).toBe(true); // fully CRed: never starved const done = withStatus({ allCR: [sig('CR', 'r', true)] }, { created_at: old }); - expect(derive(done, undefined, NOW).starved).toBe(false); + expect(derive(done, undefined, NOW, undefined, undefined, OUTSIDER).starved).toBe(false); // parked (Cryogenic Storage) ages on purpose: never starved, so no // starve nags, no starve score, no turn rotation diff --git a/frontend-v2/src/store.ts b/frontend-v2/src/store.ts index a7d9655a..c48d98d6 100644 --- a/frontend-v2/src/store.ts +++ b/frontend-v2/src/store.ts @@ -3,7 +3,9 @@ import { backend, type ConnectionState } from './backend/socket'; import { derive, parseWeightLabels, + reviewPolicy, type DerivedPull, + type ReviewPolicy, type Weight, } from '../../shared/model/status'; import { isBotLogin } from '../../shared/model/visibility'; @@ -57,6 +59,9 @@ let repoSpecs: RepoSpec[] = []; // payload — not a separate config.json fetch. A fresh weightLabels Map // reference invalidates the derive cache so weights re-resolve when it arrives. let weightLabels: ReadonlyMap = new Map(); +// who's a developer (and what a team review request means), from the same +// payload; replaced whole on initialize, so it's part of the cache key too +let policy: ReviewPolicy = reviewPolicy(); let extraBots: ReadonlySet = new Set(); let projectLabelPrefix: string | null = null; let me = ''; @@ -239,6 +244,7 @@ const derived = new WeakMap< spec: RepoSpec | undefined; warnDays: number; weightLabels: ReadonlyMap; + policy: ReviewPolicy; minute: number; value: DerivedPull; } @@ -251,11 +257,12 @@ function deriveCached(pull: PullData, spec: RepoSpec | undefined, warnDays: numb hit.spec === spec && hit.warnDays === warnDays && hit.weightLabels === weightLabels && + hit.policy === policy && hit.minute === minute ) return hit.value; - const value = derive(pull, spec, Date.now() / 1000, warnDays, weightLabels); - derived.set(pull, { spec, warnDays, weightLabels, minute, value }); + const value = derive(pull, spec, Date.now() / 1000, warnDays, weightLabels, policy); + derived.set(pull, { spec, warnDays, weightLabels, policy, minute, value }); return value; } @@ -320,6 +327,7 @@ function start() { repoSpecs = payload.repos; // server-owned config rides with the board (see shared/types) weightLabels = parseWeightLabels(payload.weightLabels); + policy = reviewPolicy(payload.developerTeams); extraBots = new Set(payload.bots ?? []); projectLabelPrefix = payload.projectLabelPrefix || null; firstBuild ??= payload.build; diff --git a/shared/model/status.ts b/shared/model/status.ts index e4e9022f..5c780309 100644 --- a/shared/model/status.ts +++ b/shared/model/status.ts @@ -80,8 +80,21 @@ export interface DerivedPull { ageDays: number; /** epoch secs the last required sign-off landed; null until fully signed off */ signedOffAt: number | null; - /** CR-incomplete past STARVE_DAYS — including half-reviewed and stale-CR rot */ + /** CR-incomplete past STARVE_DAYS on a pull waiting on someone else's + * review (asked for, or from outside the dev team); a developer's own + * unrequested pull never starves, its review is the author's job */ starved: boolean; + /** the self-review policy: a developer's pull with no review request (no + * person, no team, no claim). Its CR and QA are the author's own to do, + * and nobody else owes it anything */ + ownReview: boolean; + /** logins asked to review it by the author's side: requested reviewers + * (claims excluded) plus every member of a requested roster team; never + * the author */ + askedOf: string[]; + /** epoch secs of the earliest open request in askedOf; null if none or + * unknown. Requests are answered in hours, so this clock drives the nudge */ + askedAt: number | null; starveScore: number; weight: Weight; /** mergeable === false: shows as a flag everywhere, gates "ready" */ @@ -109,7 +122,7 @@ export interface DerivedPull { } export const WEIGHT_ORDER = ['XS', 'S', 'M', 'L', 'XL'] as const; -export type Weight = (typeof WEIGHT_ORDER)[number]; +export type Weight = typeof WEIGHT_ORDER[number]; const WEIGHT_RANK: Record = WEIGHT_ORDER.reduce( (acc, w, i) => ({ ...acc, [w]: i }), {} as Record @@ -152,14 +165,15 @@ export const ROT_DAYS = 10; const activeUsers = (sigs: Signature[]) => unique(sigs.filter(s => s.data.active).map(s => s.data.user.login)); -/** users with only invalidated stamps (no active one), excluding the author */ -const staleUsers = (sigs: Signature[], author: string) => { +/** users with only invalidated stamps (no active one), the author included: + * under self-review an author's own stale stamp is a re-stamp they owe */ +const staleUsers = (sigs: Signature[]) => { const active = activeUsers(sigs); return unique( sigs .filter(s => !s.data.active) .map(s => s.data.user.login) - .filter(u => !active.includes(u) && u !== author) + .filter(u => !active.includes(u)) ); }; @@ -244,6 +258,34 @@ export function headPushedAt(pull: PullData): number | null { export const crDone = (p: { crHave: number; data: PullData }) => p.crHave >= p.data.status.cr_req; export const qaDone = (p: { qaHave: number; data: PullData }) => p.qaHave >= p.data.status.qa_req; +/** Who counts as a developer, and the roster teams a GitHub team request + * resolves through. Built once from config projects.developerTeams. */ +export interface ReviewPolicy { + /** developer logins (lowercased); empty means everyone is a developer */ + developers: ReadonlySet; + /** roster team name, lowercased and slugged like GitHub's ("Store + * Front" -> "store-front"), to its logins */ + teams: ReadonlyMap; +} + +const slug = (name: string) => name.trim().toLowerCase().replace(/\s+/g, '-'); + +export function reviewPolicy(developerTeams?: Record | null): ReviewPolicy { + const teams = new Map(); + const developers = new Set(); + for (const [name, logins] of Object.entries(developerTeams ?? {})) { + if (!Array.isArray(logins)) continue; + teams.set(slug(name), logins); + logins.forEach(l => developers.add(l.toLowerCase())); + } + return { developers, teams }; +} + +const EVERYONE_DEVELOPS: ReviewPolicy = { developers: new Set(), teams: new Map() }; + +export const isDeveloper = (policy: ReviewPolicy, login: string) => + !policy.developers.size || policy.developers.has(login.toLowerCase()); + export function derive( pull: PullData, spec: RepoSpec | undefined, @@ -254,7 +296,9 @@ export function derive( /** label title → weight bucket, from config.json's `weightLabels`. A * matching label is the authoritative review-effort signal and overrides * the diff-size heuristic; empty map keeps the heuristic. */ - weightLabels: ReadonlyMap = new Map() + weightLabels: ReadonlyMap = new Map(), + /** who's a developer and what a team request means (reviewPolicy) */ + policy: ReviewPolicy = EVERYONE_DEVELOPS ): DerivedPull { const st = pull.status; const crBy = activeUsers(st.allCR); @@ -263,8 +307,30 @@ export function derive( const qaHave = qaBy.length; const ci = ciVerdict(pull, spec); - const staleCr = staleUsers(st.allCR, pull.user.login); - const staleQa = staleUsers(st.allQA, pull.user.login); + const author = pull.user.login; + // the author's own stamp goes stale on a push like anyone's, and under + // self-review it's the author who owes the re-stamp + const staleCr = staleUsers(st.allCR); + const staleQa = staleUsers(st.allQA); + + // who the author's side asked: people (not claims, which are a reviewer + // volunteering) and every member of a requested roster team + const claims = new Set((pull.review_requests ?? []).filter(r => r.self).map(r => r.login)); + const requestedTeams = pull.requested_teams ?? []; + const askedOf = unique([ + ...(pull.requested_reviewers ?? []).filter(l => !claims.has(l)), + ...requestedTeams.flatMap(t => policy.teams.get(t.toLowerCase()) ?? []), + ]).filter(l => l !== author); + const askedAt = + Math.min( + ...(pull.review_requests ?? []) + .filter(r => !r.self && r.at != null && askedOf.includes(r.login)) + .map(r => r.at as number) + ) || null; + const ownReview = + isDeveloper(policy, author) && + !(pull.requested_reviewers ?? []).length && + !requestedTeams.length; // a lifted block deactivates its signature, same as a stale CR stamp const devBlockedBy = activeUsers(st.dev_block); @@ -332,7 +398,8 @@ export function derive( // Rot is rot whether the pull has zero stamps, one of two, or a stale one // waiting on a re-stamp — the old `crHave === 0` cliff hid half-reviewed // pulls from the aging lane forever. - const starved = !isParked && CR_INCOMPLETE.includes(status) && !crMet && ageDays >= warnDays; + const starved = + !isParked && !ownReview && CR_INCOMPLETE.includes(status) && !crMet && ageDays >= warnDays; const signedOffAt = crMet && qaMet @@ -373,6 +440,9 @@ export function derive( cryo: isParked, changesRequestedBy, engagedNoStamp, + ownReview, + askedOf, + askedAt: Number.isFinite(askedAt) ? askedAt : null, }; } diff --git a/shared/types.ts b/shared/types.ts index ffab015f..716f81b5 100644 --- a/shared/types.ts +++ b/shared/types.ts @@ -96,6 +96,16 @@ export interface PullData { * omit the field entirely, so read as [] when absent — requested_reviewers * stays the authoritative list of who's requested either way. */ review_requests?: Array<{ login: string; at: number | null; self: boolean }>; + /** GitHub team slugs the PR asks a review from (the pull's + * requested_teams). Under the self-review policy a request means everyone + * listed; derive() turns a slug into people through the roster teams + * (config projects.developerTeams), matched by name. Absent on older + * servers: read as []. */ + requested_teams?: string[]; + /** areas the diff touches that usually deserve team input (ci, migrations, + * alerting, agent-docs, deploy, dependencies), computed server-side from + * the changed file paths. Absent until the server has looked: read as []. */ + input_hints?: string[]; // the wire also sends top-level cr_req/qa_req twins, but status.cr_req/ // qa_req are the ones every consumer reads — typing one copy prevents // reading the wrong one @@ -151,6 +161,11 @@ export interface InitializePayload { * only when the server is set up for projects; the Projects tab shows only * then. See shared/model/projects.ts. */ projectLabelPrefix?: string; + /** config projects.developerTeams: team name -> logins. Anyone listed is + * a developer, who reviews their own PRs unless they ask for review; + * everyone else's PRs still need someone else's. Absent: everyone counts + * as a developer. */ + developerTeams?: Record; /** names the frontend build the server is serving (absent without a build); * a different one on a later initialize means the board was redeployed */ build?: string; From 0ec1d61bb8cd40d6fd224c7183614e82f14c161f Mon Sep 17 00:00:00 2001 From: Jarred Stelfox Date: Fri, 9 Oct 2026 17:13:59 -0700 Subject: [PATCH 02/17] Keep stamps through base merges, read team requests The self-review policy needs a few things from the server: 1) A clean merge of master into a PR no longer voids its CR and QA stamps, the way GitHub keeps an approval across one. The commit list is only fetched when a stamp actually predates the head, to spare the API quota. A merge that resolved conflicts reads the same as a clean one; the code says so. 2) Review requests to GitHub teams (requested_teams) are stored and sent to the board, which counts one as a request to everyone on the roster team of that name. 3) "Input hints": which areas a PR's diff touches that usually deserve team input (CI, migrations, alerting, agent docs, deploy config, dependencies), from its changed paths, fetched once per head. 4) The board gets the developer roster, the server-side derive uses it, and /api/v1 adds review.own, asked_of and asked_at. 5) Stats history's time to first CR only counts someone other than the author. Migration 0033 adds the two columns; run it before this code. The dummy board seeds team requests, request times, self-stamped pulls and input hints. Co-Authored-By: Claude Opus 5.5 --- controllers/api.js | 1 + controllers/githubHooks.js | 4 +- controllers/stats.js | 15 +-- frontend-v2/src/backend/dummy.ts | 91 ++++++++++++++++++- lib/base-merge.js | 33 +++++++ lib/git-manager.js | 70 +++++++++++++- lib/input-hints.js | 34 +++++++ lib/pull-manager.js | 3 + lib/review-model.js | 17 +++- lib/schema-check.js | 6 ++ ...pulls--add-requested-teams-input-hints.sql | 3 + migrations/schema.sql | 2 + models/db_pull.js | 9 ++ models/pull.js | 17 ++++ shared/index.ts | 1 + test/api.test.js | 1 + test/base-merge.test.js | 35 +++++++ test/git-manager-refresh.test.js | 64 +++++++++++++ test/input-hints.test.js | 56 ++++++++++++ test/stats.test.js | 13 +++ 20 files changed, 453 insertions(+), 22 deletions(-) create mode 100644 lib/base-merge.js create mode 100644 lib/input-hints.js create mode 100644 migrations/0033-pulls--add-requested-teams-input-hints.sql create mode 100644 test/base-merge.test.js create mode 100644 test/input-hints.test.js diff --git a/controllers/api.js b/controllers/api.js index 8a679df2..3f04a27c 100644 --- a/controllers/api.js +++ b/controllers/api.js @@ -34,6 +34,7 @@ function toRecord(d) { cr: { req: p.status.cr_req, have: d.crHave, by: d.crBy, recr_by: d.recrBy }, qa: { req: p.status.qa_req, have: d.qaHave, by: d.qaBy }, }, + review: { own: d.ownReview, asked_of: d.askedOf, asked_at: d.askedAt }, dev_blocked_by: d.devBlockedBy, deploy_blocked_by: d.deployBlockedBy, head_sha: p.head.sha, diff --git a/controllers/githubHooks.js b/controllers/githubHooks.js index 70f82bf3..37beca0a 100644 --- a/controllers/githubHooks.js +++ b/controllers/githubHooks.js @@ -316,8 +316,8 @@ function handleLabelEvents(body) { * Record one `review_requested` / `review_request_removed` webhook's * metadata onto the in-memory review_requests cache (models/pull.js) -- * memory-only, no DB write. `body.requested_reviewer` is absent for a team - * review request, which Pulldasher doesn't track per-reviewer, so that's a - * no-op. + * review request, which isn't a per-reviewer clock (the team's slug rides on + * the pull payload's requested_teams, saved with the pull), so that's a no-op. */ function recordReviewRequestMetadata(body) { const reviewer = body.requested_reviewer; diff --git a/controllers/stats.js b/controllers/stats.js index bedc03a6..779efa26 100644 --- a/controllers/stats.js +++ b/controllers/stats.js @@ -72,7 +72,9 @@ function queryMergeAgeByDay() { /** * Per-pull hours from open to first CR signature, for merged pulls in the - * last 12 months. One row per (repo, number) — used both to compute the + * last 12 months. Only someone other than the author counts: under + * self-review the author's own CR would read as zero, and a pull with only + * that has no first review and drops out. One row per (repo, number) — used both to compute the * monthly average in SQL and, in JS, the monthly median (MySQL has no * MEDIAN aggregate, and the row count here is small enough — thousands, not * millions — that pulling the raw latencies and sorting in JS is simpler @@ -86,10 +88,11 @@ function queryFirstCrLatencies() { (cr.min_date - p.date) AS latency_seconds \ FROM pulls p \ JOIN ( \ - SELECT repo, number, MIN(date) AS min_date \ - FROM pull_signatures \ - WHERE type = 'CR' \ - GROUP BY repo, number \ + SELECT s.repo, s.number, MIN(s.date) AS min_date \ + FROM pull_signatures s \ + JOIN pulls o ON o.repo = s.repo AND o.number = s.number \ + WHERE s.type = 'CR' AND s.user <> o.owner \ + GROUP BY s.repo, s.number \ ) cr ON cr.repo = p.repo AND cr.number = p.number \ WHERE p.date_merged >= ? \ AND cr.min_date > p.date"; @@ -209,7 +212,7 @@ function loadHistory() { // factored out so they're independently testable without a live DB connection // (mirrors lib/refresh.js exporting processIssueItem/processPullItem for the // same reason). -export { firstCrByMonth, median, formatDate, formatIsoWeek, round1 }; +export { queryFirstCrLatencies, firstCrByMonth, median, formatDate, formatIsoWeek, round1 }; export default { /** diff --git a/frontend-v2/src/backend/dummy.ts b/frontend-v2/src/backend/dummy.ts index d7e91e02..147a8069 100644 --- a/frontend-v2/src/backend/dummy.ts +++ b/frontend-v2/src/backend/dummy.ts @@ -44,7 +44,7 @@ export async function loadDummy(): Promise { const withBody = (p: PullData, i: number): PullData => p.body === 'pull request dummy body' ? { ...p, body: BODIES[i % BODIES.length] } : p; const pulls = withSyntheticStacks(raw).map((p, i) => - withProject(withBody(withSizes(redate(p, i), i), i), i) + withProject(withBody(withSizes(withSelfReviewDemos(redate(p, i), i), i), i), i) ); // ?projects=100 adds that many projects in flight, to design at real size const scaled = new URLSearchParams(location.search).has('projects') @@ -65,6 +65,7 @@ export async function loadDummy(): Promise { 'size: XL': 'XL', }, projectLabelPrefix: DUMMY_PROJECT_PREFIX, + developerTeams: dummyDeveloperTeams(pulls), // The fixture carries almost no closed/merged pulls and no diff sizes, so // the Stats lens (merge-time-by-size, leaderboards over shipped work) has // nothing to show. Synthesize a fortnight of merged PRs across the size @@ -73,6 +74,20 @@ export async function loadDummy(): Promise { }; } +// The roster of developers (config projects.developerTeams): every dummy author +// but two, who stay outside contributors whose pulls still need someone else's +// review. The viewer is always on it. +const OUTSIDE_CONTRIBUTORS = new Set(['notme', 'Michelle5102']); +function dummyDeveloperTeams(pulls: PullData[]): Record { + const logins = [...new Set([dummyUser(), ...pulls.map(p => p.user.login)])].filter( + l => !OUTSIDE_CONTRIBUTORS.has(l) && !l.endsWith('[bot]') + ); + const names = ['Store Front', 'Platform', 'Community']; + const teams: Record = Object.fromEntries(names.map(n => [n, []])); + logins.forEach((l, i) => teams[names[i % names.length]].push(l)); + return teams; +} + export const DUMMY_PROJECT_PREFIX = 'project:'; // The Projects tab reads project labels off the pulls, and the fixture @@ -244,8 +259,8 @@ function participantsFor(pull: PullData, i: number): string[] { i === CHANGES_REQUESTED_INDEX ? ['grumpy-reviewer'] : i === COMMENTED_INDEX - ? ['curious-commenter'] - : ['silent-lurker']; + ? ['curious-commenter'] + : ['silent-lurker']; return [...new Set([...(pull.participants ?? []), ...stampers, ...extra])]; } @@ -269,6 +284,72 @@ const wantsReviewRequest = (pull: PullData, i: number): boolean => pull.user.login !== dummyUser() && i % 3 === 1; const ASSIGNEE_INDEXES = new Set([6, 12]); +// The self-review policy demos, keyed by index like the rest: team requests +// (one for a team nobody is on the roster of), requests of the viewer with a +// clock in hours (one past four), developers' own stamped pulls, an author +// whose own stamp went stale, and input hints from the diff's paths. +const TEAM_REQUESTS: Record = { + 2: ['store-front'], + 9: ['store-front'], + 17: ['platform'], + 20: ['community'], + 30: ['design'], +}; +const OWN_STAMPED = new Set([3, 8, 21]); +const OWN_STALE_STAMP = 24; +const INPUT_HINTS: Record = { + 4: ['migrations'], + 18: ['ci', 'agent-docs'], + 26: ['deploy', 'dependencies'], + 33: ['alerting'], +}; + +function withSelfReviewDemos(pull: PullData, i: number): PullData { + const now = Date.now(); + const author = pull.user; + const stamp = (type: 'CR' | 'QA', active: boolean, hoursAgo: number) => ({ + data: { + repo: pull.repo, + number: pull.number, + user: { id: 1, ...author }, + type, + created_at: new Date(now - hoursAgo * 3_600_000).toISOString(), + active: active ? 1 : 0, + comment_id: 900000000 + i * 10 + (type === 'CR' ? 1 : 2), + }, + }); + const meAsked = (pull.requested_reviewers ?? []).includes(dummyUser()); + let out: PullData = { + ...pull, + requested_teams: TEAM_REQUESTS[i] ?? [], + input_hints: INPUT_HINTS[i] ?? [], + // asked hours ago; the one at 7 is past the four hours reviewers answer in + review_requests: meAsked + ? [ + { + login: dummyUser(), + at: Math.floor(now / 1000) - (i === 7 ? 5 : 1 + (i % 3)) * 3600, + self: false, + }, + ] + : pull.review_requests, + }; + if (OWN_STAMPED.has(i) || i === OWN_STALE_STAMP) { + out = { + ...out, + requested_reviewers: [], + requested_teams: [], + review_requests: [], + status: { + ...out.status, + allCR: [stamp('CR', i !== OWN_STALE_STAMP, 6)], + allQA: i === OWN_STALE_STAMP ? [] : [stamp('QA', true, 5)], + }, + }; + } + return out; +} + function withSyntheticStacks(pulls: PullData[]): PullData[] { const out = [...pulls]; const rebase = (i: number, baseRef: string) => { @@ -329,8 +410,8 @@ function redate(pull: PullData, i: number): PullData { deletions, requested_reviewers: wantsReviewRequest(pull, i) ? [dummyUser()] - : (pull.requested_reviewers ?? []), - assignees: ASSIGNEE_INDEXES.has(i) ? [dummyUser()] : (pull.assignees ?? []), + : pull.requested_reviewers ?? [], + assignees: ASSIGNEE_INDEXES.has(i) ? [dummyUser()] : pull.assignees ?? [], }; } diff --git a/lib/base-merge.js b/lib/base-merge.js new file mode 100644 index 00000000..661401ae --- /dev/null +++ b/lib/base-merge.js @@ -0,0 +1,33 @@ +const escape = s => s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + +/** + * Whether a commit is a merge of the PR's base branch into the PR branch: + * two parents and git's or GitHub's merge message, e.g. "Merge branch + * 'master' into foo", "Merge branch 'master' of github.com:o/r into foo", + * "Merge remote-tracking branch 'origin/master' into foo". + */ +export function isBaseMerge(commit, baseRef) { + if (!commit.parents || commit.parents.length !== 2) return false; + const subject = String(commit.commit?.message ?? '').split('\n')[0]; + return new RegExp( + `^Merge (?:remote-tracking )?branch '(?:[\\w.-]+/)?${escape( + baseRef + )}'(?: of \\S+)?(?: into .+)?$` + ).test(subject); +} + +/** + * Whether a CR/QA stamp left at `stampAt` survives the commits since: true + * when every commit made after it is a merge of the base into the PR, the way + * GitHub keeps an approval across one. `commits` is pulls.listCommits output. + * + * ponytail: the message can't tell a clean merge from one that resolved + * conflicts, so a conflict-resolving merge keeps the stamp too. Upgrade path: + * compare the merge commit's tree with a fresh merge of its parents, or the + * PR's file patches before and after. + */ +export function stampSurvivesBaseMerges(stampAt, commits, baseRef) { + return commits + .filter(c => new Date(c.commit.committer.date) > stampAt) + .every(c => isBaseMerge(c, baseRef)); +} diff --git a/lib/git-manager.js b/lib/git-manager.js index 4313d192..366c8d2a 100644 --- a/lib/git-manager.js +++ b/lib/git-manager.js @@ -15,6 +15,8 @@ import Status from '../models/status.js'; import Signature from '../models/signature.js'; import getLogin from './get-user-login.js'; import { noopPacer } from './pacer.js'; +import { stampSurvivesBaseMerges } from './base-merge.js'; +import { inputHints, rememberHints, recalledHints } from './input-hints.js'; import { DEFAULT_PROJECT_PREFIX } from '../shared/dist/index.js'; const MyOctokit = Octokit.plugin(throttling, retry); @@ -398,7 +400,7 @@ export default { reviews, botLogin, diffStats, - ]).then(function (results) { + ]).then(async function (results) { var reviewComments = results[0], comments = results[1], headCommit = results[2], @@ -441,16 +443,40 @@ export default { // signature, which means the approval is still valid. Re-applying the // commit-date check would drop approvals GitHub deliberately kept. var headCommitDate = new Date(headCommit.commit.committer.date); + var predatesHead = signature => + (signature.data.type === 'CR' || signature.data.type === 'QA') && + !signature.data.fromGithubApproval && + new Date(signature.data.created_at) < headCommitDate; + // A stamp also stays when every commit since is a merge of the base + // branch into the PR (GitHub keeps approvals across one). The commit + // list costs a call, so only ask when a stamp actually predates the head. + var commits = signatures.some(predatesHead) + ? await getPullCommits(repo, githubPull.number) + : []; signatures.forEach(function (signature) { if ( - (signature.data.type === 'CR' || signature.data.type === 'QA') && - !signature.data.fromGithubApproval && - new Date(signature.data.created_at) < headCommitDate + predatesHead(signature) && + !stampSurvivesBaseMerges( + new Date(signature.data.created_at), + commits, + githubPull.base.ref + ) ) { signature.data.active = false; } }); + // Which areas the diff touches, refetched only when the head moved + // since the stored hints were computed + if (githubPull.state === 'open') { + var hints = recalledHints(repo, githubPull.number, githubPull.head.sha); + if (!hints) { + hints = inputHints(await getPullFilePaths(repo, githubPull.number)); + rememberHints(repo, githubPull.number, githubPull.head.sha, hints); + } + githubPull.input_hints = hints; + } + // Array of Comment objects. comments = comments.map(function (commentData) { commentData.number = githubPull.number; @@ -759,7 +785,8 @@ export function deriveReviewRequests(events, requestedReviewerLogins, botLogin) const activeByLogin = new Map(); ordered.forEach(event => { - // Team review requests have no individual reviewer to attribute this to. + // A team request has no individual to attribute this to; the team's + // slug rides on the pull's requested_teams instead. if (!event.requested_reviewer) { return; } @@ -897,6 +924,39 @@ function getPullReviewComments(repo, number) { ); } +// oldest first, with parents and committer dates; GitHub lists at most 250 +function getPullCommits(repo, number) { + return logErrors( + github.paginate( + githubRest.pulls.listCommits, + params({ pull_number: number, per_page: 100 }, repo) + ), + 'Getting commits for pull %s:%s', + repo, + number + ); +} + +function getPullFilePaths(repo, number) { + return logErrors( + (async () => { + const paths = []; + let page = 1; + for (; page <= 3; page++) { + const res = await githubRest.pulls.listFiles( + params({ pull_number: number, per_page: 100, page }, repo) + ); + paths.push(...res.data.map(f => f.filename)); + if (res.data.length < 100) break; + } + return paths; + })(), + 'Getting changed files for pull %s:%s', + repo, + number + ); +} + function getCommit(repo, sha) { return logErrors( githubRest.repos.getCommit(params({ ref: sha }, repo)).then(res => res.data), diff --git a/lib/input-hints.js b/lib/input-hints.js new file mode 100644 index 00000000..de45b9b2 --- /dev/null +++ b/lib/input-hints.js @@ -0,0 +1,34 @@ +const HINTS = [ + [ + 'ci', + /(^|\/)(\.github|\.circleci|\.buildkite)\/|(^|\/)Jenkinsfile[^/]*$|(^|\/)lefthook[^/]*\.ya?ml$/, + ], + ['migrations', /(^|\/)migrations\/|(^|\/)schema\.sql$|\.sql$/i], + ['alerting', /(^|[/._-])(alert\w*|prometheus|grafana|datadog|pagerduty|sentry)([/._-]|$)/i], + ['agent-docs', /(^|\/)(AGENTS|CLAUDE)\.md$|(^|\/)\.(agents|claude|cursor|codex)\//], + [ + 'deploy', + /(^|\/)(Dockerfile[^/]*|docker-compose[^/]*|Capfile)$|(^|\/)(deploy|terraform|helm|k8s)\/|(^|\/)config\/deploy|\.tf$/, + ], + [ + 'dependencies', + /(^|\/)(package\.json|pnpm-lock\.yaml|package-lock\.json|yarn\.lock|composer\.(json|lock)|Gemfile(\.lock)?|go\.(mod|sum)|requirements[^/]*\.txt|pyproject\.toml)$/, + ], +]; + +/** Areas a diff touches that usually deserve team input, from its changed + * paths, in a fixed order and each once. */ +export function inputHints(paths) { + return HINTS.filter(([, re]) => paths.some(p => re.test(p))).map(([key]) => key); +} + +// repo#number -> { sha, hints }: what the stored hints were computed for, so +// a restart (Pull.fromDB seeds it) or a repeat refresh doesn't refetch the files +const memo = new Map(); +const key = (repo, number) => `${repo}#${number}`; +export const rememberHints = (repo, number, sha, hints) => + memo.set(key(repo, number), { sha, hints }); +export const recalledHints = (repo, number, sha) => { + const m = memo.get(key(repo, number)); + return m && m.sha === sha ? m.hints : null; +}; diff --git a/lib/pull-manager.js b/lib/pull-manager.js index c4b63af6..47129983 100644 --- a/lib/pull-manager.js +++ b/lib/pull-manager.js @@ -5,6 +5,7 @@ import _ from 'underscore'; import dbManager from './db-manager.js'; import Pull from '../models/pull.js'; import { projectSettings } from './projects.js'; +import { developerTeams } from './review-model.js'; import buildId from './build-id.js'; const pmDebug = debug('pulldasher:pull-manager'); @@ -79,6 +80,8 @@ function sendInitialData(socket) { // the project label prefix, sent only when this install uses projects // (config.js `projects`); the board shows its Projects tab only then projectLabelPrefix: projectSettings()?.prefix, + // who reviews their own pulls, and what a team request means (shared reviewPolicy) + developerTeams: developerTeams(), // names the served frontend build, so an open tab can tell a deploy happened build: buildId, }); diff --git a/lib/review-model.js b/lib/review-model.js index 2414b735..d5197ce3 100644 --- a/lib/review-model.js +++ b/lib/review-model.js @@ -1,5 +1,6 @@ import config from './config-loader.js'; -import { derive, isBotLogin, parseWeightLabels } from '../shared/dist/index.js'; +import { derive, isBotLogin, parseWeightLabels, reviewPolicy } from '../shared/dist/index.js'; +import { savedSetting } from './settings.js'; // Org config from config.js -- the single source. The board receives the same // values over the socket (see lib/pull-manager sendInitialData), so the API and @@ -35,15 +36,22 @@ export function toWire(pull) { return JSON.parse(JSON.stringify(pull.toObject())); } +/** The roster of developers (teams saved from the board win over config.js's), + * which decides whose pulls they review themselves; unset means everyone. */ +export const developerTeams = () => + savedSetting('developer_teams') ?? config.projects?.developerTeams; + +const currentPolicy = () => reviewPolicy(developerTeams()); + /** * Derive one Pull into its DerivedPull (status bucket, CI verdict, weight, * sign-off counts, staleness, age, ...). Weight uses the org's weightLabels * (an auto/manual label overrides the diff-size guess), falling back to the * size heuristic for a pull carrying none. */ -export function derivePull(pull, now = Date.now() / 1000) { +export function derivePull(pull, now = Date.now() / 1000, policy = currentPolicy()) { const wire = toWire(pull); - return derive(wire, specForRepo(wire.repo), now, undefined, weightLabels); + return derive(wire, specForRepo(wire.repo), now, undefined, weightLabels, policy); } /** Whether a login is a bot -- the `[bot]` suffix GitHub Apps carry, or the @@ -55,5 +63,6 @@ export function isBot(login) { /** Derive a list of Pulls against one shared `now` (a consistent clock for a * single request/snapshot). */ export function deriveAll(pulls, now = Date.now() / 1000) { - return pulls.map(pull => derivePull(pull, now)); + const policy = currentPolicy(); + return pulls.map(pull => derivePull(pull, now, policy)); } diff --git a/lib/schema-check.js b/lib/schema-check.js index d2190b3e..921e8d99 100644 --- a/lib/schema-check.js +++ b/lib/schema-check.js @@ -29,6 +29,12 @@ export const MIGRATIONS = [ table: 'roadmap_items', column: 'end_kind', }, + // one statement adds both columns, so one stands for the pair + { + file: '0033-pulls--add-requested-teams-input-hints.sql', + table: 'pulls', + column: 'input_hints', + }, ]; /** The migration files this database hasn't had yet, oldest first. */ diff --git a/migrations/0033-pulls--add-requested-teams-input-hints.sql b/migrations/0033-pulls--add-requested-teams-input-hints.sql new file mode 100644 index 00000000..2f40882d --- /dev/null +++ b/migrations/0033-pulls--add-requested-teams-input-hints.sql @@ -0,0 +1,3 @@ +ALTER TABLE `pulls` + ADD COLUMN `requested_teams` json DEFAULT NULL AFTER `requested_reviewers`, + ADD COLUMN `input_hints` json DEFAULT NULL AFTER `requested_teams`; diff --git a/migrations/schema.sql b/migrations/schema.sql index dfd5222b..6512beb5 100644 --- a/migrations/schema.sql +++ b/migrations/schema.sql @@ -145,6 +145,8 @@ CREATE TABLE IF NOT EXISTS `pulls` ( `owner` varchar(255) COLLATE utf8mb4_general_ci NOT NULL, `assignees` json DEFAULT NULL, `requested_reviewers` json DEFAULT NULL, + `requested_teams` json DEFAULT NULL, + `input_hints` json DEFAULT NULL, `cr_req` int NOT NULL DEFAULT '2', `qa_req` int NOT NULL DEFAULT '1', `date` int unsigned DEFAULT NULL, diff --git a/models/db_pull.js b/models/db_pull.js index 2d6b4830..374e260b 100644 --- a/models/db_pull.js +++ b/models/db_pull.js @@ -28,6 +28,7 @@ function DBPull(pull) { // Serialize to a JSON string; MySQL parses it into the JSON column. assignees: JSON.stringify(pullData.assignees || []), requested_reviewers: JSON.stringify(pullData.requested_reviewers || []), + requested_teams: JSON.stringify(pullData.requested_teams || []), cr_req: pullData.cr_req, qa_req: pullData.qa_req, closes: pullData.closes, @@ -36,6 +37,14 @@ function DBPull(pull) { deletions: pullData.deletions, changed_files: pullData.changed_files, }; + // only a full refresh computed them; the column holds {sha, hints} so a + // restart knows which head they were for + if (pullData.input_hints) { + this.data.input_hints = JSON.stringify({ + sha: pullData.input_hints_sha, + hints: pullData.input_hints, + }); + } } DBPull.prototype.save = function () { diff --git a/models/pull.js b/models/pull.js index 9d274543..13eea6e2 100644 --- a/models/pull.js +++ b/models/pull.js @@ -5,6 +5,7 @@ import queue from '../lib/pull-queue.js'; import debug from '../lib/debug.js'; import DBPull from './db_pull.js'; import getLogin from '../lib/get-user-login.js'; +import { recalledHints, rememberHints } from '../lib/input-hints.js'; import { isBot } from '../lib/review-model.js'; const log = debug('pulldasher:pull'); @@ -155,6 +156,7 @@ class Pull { var data = _.extend({}, this.data); data.status = this.getStatus(); data.labels = this.labels.map(label => label.data); + delete data.input_hints_sha; data.review_requests = getReviewRequests(data.repo, data.number, data.requested_reviewers); return data; } @@ -318,6 +320,14 @@ class Pull { }, assignees: (data.assignees || []).map(a => getLogin(a)), requested_reviewers: (data.requested_reviewers || []).map(r => getLogin(r)), + requested_teams: (data.requested_teams || []).map(t => t.slug), + // parse() fills input_hints on a full refresh; a webhook's body + // can't, so reuse what was computed for this same head + input_hints: + data.input_hints ?? + recalledHints(data.base.repo.full_name, data.number, data.head.sha) ?? + undefined, + input_hints_sha: data.head.sha, additions: data.additions, deletions: data.deletions, changed_files: data.changed_files, @@ -392,6 +402,10 @@ class Pull { * Pull object. */ static getFromDB(data, signatures, comments, reviews, commitStatuses, labels) { + // seed what the stored hints were computed for (see lib/input-hints.js) + if (data.input_hints?.sha) { + rememberHints(data.repo, data.number, data.input_hints.sha, data.input_hints.hints); + } var pullData = { repo: data.repo, number: data.number, @@ -439,6 +453,9 @@ class Pull { // mysql2 auto-parses the JSON column, so this is already an array (or null). assignees: data.assignees ?? [], requested_reviewers: data.requested_reviewers ?? [], + requested_teams: data.requested_teams ?? [], + input_hints: data.input_hints?.hints ?? [], + input_hints_sha: data.input_hints?.sha, cr_req: data.cr_req, qa_req: data.qa_req, // read back with cr_req, or the constructor skips the body parse diff --git a/shared/index.ts b/shared/index.ts index 70fd57be..6836b71f 100644 --- a/shared/index.ts +++ b/shared/index.ts @@ -12,6 +12,7 @@ */ export { derive, + reviewPolicy, ciVerdict, ciFailing, headStatuses, diff --git a/test/api.test.js b/test/api.test.js index 24a76b68..f2a3f5cc 100644 --- a/test/api.test.js +++ b/test/api.test.js @@ -153,6 +153,7 @@ test('serves the board classified server-side (/pulls)', async () => { assert.equal(rec.signoffs.cr.req, 2); assert.equal(rec.signoffs.cr.have, 0); assert.equal(rec.url, 'https://github.com/test/repo-a/pull/42'); + assert.deepEqual(rec.review, { own: true, asked_of: [], asked_at: null }); assert.equal(rec.is_bot, false); // bob isn't a configured bot const botRec = body.pulls.find(p => p.id === 'test/repo-a#43'); assert.equal(botRec.is_bot, true); // fixture-bot is, via config.js bots diff --git a/test/base-merge.test.js b/test/base-merge.test.js new file mode 100644 index 00000000..b5732eb2 --- /dev/null +++ b/test/base-merge.test.js @@ -0,0 +1,35 @@ +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { isBaseMerge, stampSurvivesBaseMerges } from '../lib/base-merge.js'; + +const commit = (message, parents, date = '2026-10-02T00:00:00Z') => ({ + parents: parents.map(sha => ({ sha })), + commit: { message, committer: { date } }, +}); + +test('recognises git and GitHub merges of the base into the branch', () => { + for (const message of [ + "Merge branch 'master' into foo", + "Merge branch 'master' of github.com:iFixit/ifixit into foo", + "Merge remote-tracking branch 'origin/master' into foo", + "Merge branch 'master' into foo\n\n# Conflicts:\n#\tapp.js", + ]) { + assert.ok(isBaseMerge(commit(message, ['a', 'b']), 'master'), message); + } + assert.ok(isBaseMerge(commit("Merge branch 'main' into foo", ['a', 'b']), 'main')); +}); + +test('other merges and plain commits are not base merges', () => { + assert.ok(!isBaseMerge(commit("Merge branch 'master' into foo", ['a']), 'master')); + assert.ok(!isBaseMerge(commit("Merge branch 'other' into foo", ['a', 'b']), 'master')); + assert.ok(!isBaseMerge(commit("Merge branch 'master2' into foo", ['a', 'b']), 'master')); + assert.ok(!isBaseMerge(commit('Fix the thing', ['a', 'b']), 'master')); +}); + +test('a stamp survives only when every later commit is a base merge', () => { + const stamp = new Date('2026-10-01T00:00:00Z'); + const before = commit('Work', ['a'], '2026-09-30T00:00:00Z'); + const merge = commit("Merge branch 'master' into foo", ['a', 'b']); + assert.ok(stampSurvivesBaseMerges(stamp, [before, merge, merge], 'master')); + assert.ok(!stampSurvivesBaseMerges(stamp, [before, merge, commit('More', ['c'])], 'master')); +}); diff --git a/test/git-manager-refresh.test.js b/test/git-manager-refresh.test.js index 8a81979f..e79fb91d 100644 --- a/test/git-manager-refresh.test.js +++ b/test/git-manager-refresh.test.js @@ -1,6 +1,7 @@ import { test } from "node:test"; import assert from "node:assert/strict"; import gitManager from "../lib/git-manager.js"; +import { rememberHints } from "../lib/input-hints.js"; // Stand in for api.github.com: each route answers by path (and, for GraphQL, // by query), and every call made is recorded as "METHOD /path". @@ -72,6 +73,7 @@ function parseRoutes(checkRuns) { }), ], [/\/pulls\/3\/reviews$/, () => ({ body: [] })], + [/\/pulls\/3\/files$/, () => ({ body: [] })], ]; } @@ -270,3 +272,65 @@ test("getPullsToCompare reads both lists in one call, then only the one with mor }, ]); }); + +// A CR stamp from before the head commit (10:00): kept when only base merges +// came after it, dropped when real work did. +function stampRoutes(commits, files = [], stamped = true) { + const routes = parseRoutes(() => ({ body: { total_count: 0, check_runs: [] } })); + // first match wins, so these shadow parseRoutes' empty ones + routes.unshift( + [/\/pulls\/3\/commits$/, () => ({ body: commits })], + [/\/pulls\/3\/files$/, () => ({ body: files.map((filename) => ({ filename })) })], + [ + /\/issues\/3\/comments$/, + () => ({ + body: stamped + ? [{ id: 1, body: "CR :+1:", user: { login: "rev" }, created_at: "2026-09-20T09:00:00Z" }] + : [], + }), + ] + ); + return routes; +} +const gitCommit = (message, parents, date) => ({ + parents: parents.map((sha) => ({ sha })), + commit: { message, committer: { date } }, +}); + +test("parse keeps a stamp across a base merge and drops it after real work", async (t) => { + const merge = gitCommit("Merge branch 'master' into f", ["a", "b"], "2026-09-20T10:00:00Z"); + const work = gitCommit("Fix", ["a"], "2026-09-20T09:30:00Z"); + fakeGithub(t, stampRoutes([merge])); + let pull = await gitManager.parse(githubPull()); + assert.equal(pull.signatures[0].data.active, true); + + t.mock.restoreAll(); + fakeGithub(t, stampRoutes([merge, work])); + pull = await gitManager.parse(githubPull()); + assert.equal(pull.signatures[0].data.active, false); +}); + +test("parse only lists commits when a stamp predates the head", async (t) => { + const calls = fakeGithub(t, stampRoutes([], [], false)); + await gitManager.parse(githubPull()); + assert.ok(!calls.some((c) => c.endsWith("/pulls/3/commits"))); +}); + +test("parse reads input hints from the changed files once per head", async (t) => { + rememberHints("test/repo-a", 3, "older", []); // earlier tests left hints for abc + const calls = fakeGithub(t, stampRoutes([], ["db/migrations/1.sql"])); + const gh = { ...githubPull(), head: { ...githubPull().head, sha: "abc" } }; + const first = await gitManager.parse(gh); + assert.deepEqual(first.toObject().input_hints, ["migrations"]); + await gitManager.parse(githubPull()); + assert.equal(calls.filter((c) => c.endsWith("/pulls/3/files")).length, 1); +}); + +test("parse saves the slugs of requested teams", async (t) => { + fakeGithub(t, stampRoutes([])); + const pull = await gitManager.parse({ + ...githubPull(), + requested_teams: [{ slug: "store-front", name: "Store Front" }], + }); + assert.deepEqual(pull.toObject().requested_teams, ["store-front"]); +}); diff --git a/test/input-hints.test.js b/test/input-hints.test.js new file mode 100644 index 00000000..28280767 --- /dev/null +++ b/test/input-hints.test.js @@ -0,0 +1,56 @@ +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { inputHints, rememberHints, recalledHints } from '../lib/input-hints.js'; + +test('names each area once, in a fixed order', () => { + assert.deepEqual( + inputHints([ + 'package.json', + 'apps/web/package.json', + '.claude/skills/x/SKILL.md', + 'Migrations/0001.sql', + '.github/workflows/ci.yml', + ]), + ['ci', 'migrations', 'agent-docs', 'dependencies'] + ); +}); + +test('each area matches its paths', () => { + const cases = { + ci: ['.circleci/config.yml', 'Jenkinsfile', 'lefthook-local.yml', '.buildkite/p.yml'], + migrations: ['db/migrations/1.php', 'migrations/schema.sql', 'x/y.sql'], + alerting: ['ops/alerts/db.yml', 'grafana/dash.json', 'lib/sentry.js', 'Prometheus.yml'], + 'agent-docs': ['AGENTS.md', 'a/b/CLAUDE.md', '.agents/x.md', '.cursor/r', '.codex/c'], + deploy: [ + 'Dockerfile', + 'Dockerfile.dev', + 'docker-compose.yml', + 'deploy/x', + 'Capfile', + 'config/deploy.rb', + 'main.tf', + 'helm/c.yaml', + 'k8s/d.yaml', + ], + dependencies: [ + 'pnpm-lock.yaml', + 'composer.lock', + 'go.mod', + 'requirements-dev.txt', + 'pyproject.toml', + ], + }; + for (const [hint, paths] of Object.entries(cases)) { + for (const path of paths) assert.deepEqual(inputHints([path]), [hint], path); + } +}); + +test('ordinary files get no hint', () => { + assert.deepEqual(inputHints(['src/app.js', 'README.md', 'docs/package-notes.md']), []); +}); + +test('remembered hints are recalled only for the same head', () => { + rememberHints('o/r', 1, 'abc', ['ci']); + assert.deepEqual(recalledHints('o/r', 1, 'abc'), ['ci']); + assert.equal(recalledHints('o/r', 1, 'def'), null); +}); diff --git a/test/stats.test.js b/test/stats.test.js index 933d480c..046ca5d2 100644 --- a/test/stats.test.js +++ b/test/stats.test.js @@ -5,7 +5,9 @@ import assert from "node:assert/strict"; // importing lib/db.js (transitively, via controllers/stats.js) doesn't blow // up at module-load time. Nothing here actually queries the DB — only the // pure reducer functions below are exercised. +const db = (await import("../lib/db.js")).default; const { + queryFirstCrLatencies, firstCrByMonth, median, formatDate, @@ -61,3 +63,14 @@ test("firstCrByMonth groups per-pull latencies into sorted monthly avg+median ro test("firstCrByMonth returns an empty array for no rows", () => { assert.deepEqual(firstCrByMonth([]), []); }); + +test("first-review latency ignores the author's own CR stamp", async (t) => { + let sql; + t.mock.method(db, "query", async (q) => { + sql = q; + return []; + }); + await queryFirstCrLatencies(); + assert.match(sql, /s\.user <> o\.owner/); + assert.match(sql, /JOIN pulls o ON o\.repo = s\.repo AND o\.number = s\.number/); +}); From 7ae41c892c9ee7f14699a473158a6b980b0a3e2b Mon Sep 17 00:00:00 2001 From: Jarred Stelfox Date: Fri, 9 Oct 2026 17:13:59 -0700 Subject: [PATCH 03/17] Rework Stats for self-review With authors stamping their own PRs, "time to first CR" drops to about zero and review debt counts work nobody owes. The Stats tab now shows the time to answer a review request (median and slowest 10%, in hours), who's waiting on someone (open requests by hours, outside contributors' PRs by days), and how merged PRs were reviewed: asked, by someone else, self-reviewed, or not stamped, with a rough count of reverts and quick follow-up fixes after self-reviewed merges. Open PRs by author and repo say "in self-review" and "waiting on others"; stamps per day count only stamps on other people's PRs. Co-Authored-By: Claude Opus 5.5 --- frontend-v2/src/model/stats.test.ts | 224 ++++++++++----- frontend-v2/src/model/stats.ts | 264 ++++++++++++------ frontend-v2/src/views/Stats.tsx | 28 +- .../src/views/stats/AuthorLoadCard.tsx | 14 +- frontend-v2/src/views/stats/DebtCard.tsx | 51 ---- frontend-v2/src/views/stats/RepoLoadCard.tsx | 10 +- .../src/views/stats/SelfReviewCard.tsx | 48 ++++ .../src/views/stats/ShippingPulseCard.tsx | 27 +- .../src/views/stats/StarvationCard.tsx | 56 ---- frontend-v2/src/views/stats/WaitingCard.tsx | 75 +++++ 10 files changed, 499 insertions(+), 298 deletions(-) delete mode 100644 frontend-v2/src/views/stats/DebtCard.tsx create mode 100644 frontend-v2/src/views/stats/SelfReviewCard.tsx delete mode 100644 frontend-v2/src/views/stats/StarvationCard.tsx create mode 100644 frontend-v2/src/views/stats/WaitingCard.tsx diff --git a/frontend-v2/src/model/stats.test.ts b/frontend-v2/src/model/stats.test.ts index c0bb980a..c4e1cd64 100644 --- a/frontend-v2/src/model/stats.test.ts +++ b/frontend-v2/src/model/stats.test.ts @@ -4,18 +4,18 @@ import type { DerivedPull, Status, Weight } from '../../../shared/model/status'; import { ageMix, authorLoad, - crStarvation, effortMix, - firstCrLatency, friction, humanHours, mergedPerDay, mergeTimeBySize, reciprocity, repoBreakdown, - reviewDebt, + requestAnswerTimes, + selfReviewMix, signoffLeaders, stampsPerDay, + waitingOnSomeone, } from './stats'; function sig(login: string) { @@ -78,22 +78,6 @@ describe('signoffLeaders', () => { }); }); -describe('crStarvation', () => { - it('sums open-days by author over CR-incomplete PRs, tracking the worst', () => { - const pulls = [ - open({ author: 'alice', status: 'needs_cr', ageDays: 10, n: 1 }), - open({ author: 'alice', status: 'needs_recr', ageDays: 4, n: 2 }), - open({ author: 'bob', status: 'needs_cr', ageDays: 20, n: 3 }), - open({ author: 'carol', status: 'ready', ageDays: 99, n: 4 }), // not starving - ]; - const s = crStarvation(pulls); - expect(s.map(x => x.login)).toEqual(['bob', 'alice']); // bob 20 > alice 14 - const alice = s.find(x => x.login === 'alice')!; - expect(alice).toMatchObject({ count: 2, totalDays: 14, worstDays: 10 }); - expect(s.find(x => x.login === 'carol')).toBeUndefined(); - }); -}); - describe('mergeTimeBySize', () => { it('buckets merged PRs by weight and reports count and medians, skipping unsized', () => { const closed = [ @@ -154,6 +138,12 @@ function full(over: { recrBy?: string[]; reqaBy?: string[]; qaing?: string | null; + ownReview?: boolean; + title?: string; + body?: string; + requested?: string[]; + requests?: { login: string; at: number | null; self: boolean }[]; + reviews?: { login: string; date: number }[]; conflict?: boolean; deployBlockedBy?: string[]; devBlockedBy?: string[]; @@ -171,6 +161,7 @@ function full(over: { recrBy: over.recrBy ?? [], reqaBy: over.reqaBy ?? [], qaingLogin: over.qaing ?? null, + ownReview: over.ownReview ?? true, conflict: over.conflict ?? false, deployBlockedBy: over.deployBlockedBy ?? [], devBlockedBy: over.devBlockedBy ?? [], @@ -179,6 +170,10 @@ function full(over: { data: { repo: over.repo ?? 'iFixit/ifixit', number: over.n ?? 1, + title: over.title ?? 't', + body: over.body ?? '', + requested_reviewers: over.requested ?? [], + review_requests: over.requests ?? [], additions: over.additions === undefined ? 100 : over.additions, deletions: over.deletions === undefined ? 20 : over.deletions, user: { login: over.author ?? 'alice' }, @@ -187,6 +182,7 @@ function full(over: { qa_req: over.qaReq ?? 1, allCR: over.allCR ?? [], allQA: over.allQA ?? [], + unstamped_reviewers: over.reviews ?? [], }, }, } as unknown as DerivedPull; @@ -218,12 +214,20 @@ describe('stampsPerDay', () => { }), ]; const closed = [ - { status: { allCR: [sigAt('carol', NOON - 3 * HOUR)], allQA: [] } }, + { + user: { login: 'zed' }, + status: { allCR: [sigAt('carol', NOON - 3 * HOUR)], allQA: [] }, + }, ] as unknown as PullData[]; const days = stampsPerDay(pulls, closed, 3, NOON); expect(days[2].count).toBe(3); // today: bob, dave, carol expect(days[1].count).toBe(1); // yesterday: bob's earlier stamp }); + + it("skips the author's own stamps, which are normal under self-review", () => { + const pulls = [full({ author: 'alice', allCR: [sigAt('alice', NOON), sigAt('bob', NOON)] })]; + expect(stampsPerDay(pulls, [], 1, NOON)[0].count).toBe(1); + }); }); describe('ageMix', () => { @@ -253,49 +257,119 @@ describe('effortMix', () => { }); }); -describe('reviewDebt', () => { - it('sums missing stamps, restamps owed, and unclaimed QA', () => { - const debt = reviewDebt([ - full({ status: 'needs_cr', crReq: 2, crHave: 0 }), // 2 CR slots - full({ status: 'needs_recr', crReq: 2, crHave: 1, recrBy: ['bob'] }), // 1 slot + 1 restamp - full({ status: 'needs_qa', qaReq: 1, qaHave: 0 }), // 1 QA slot, unclaimed - full({ status: 'needs_qa', qaReq: 1, qaHave: 0, qaing: 'dave' }), // claimed - full({ status: 'needs_qa', qaReq: 1, qaHave: 0, reqaBy: ['eve'] }), // re-QA owed - full({ status: 'ready' }), // no debt - ]); - expect(debt).toEqual({ crSlots: 3, qaSlots: 3, restamps: 2, unclaimedQa: 1 }); +const AT = NOON / 1000; // request time, epoch secs +const ask = (login: string, hoursAgo: number) => ({ login, at: AT - hoursAgo * 3600, self: false }); + +describe('requestAnswerTimes', () => { + it('measures request to the asked person first stamp or review after it', () => { + const pulls = [ + // bob answers with a stamp 2h after the request, carol with a review 6h after + full({ + n: 1, + requests: [ask('bob', 10), ask('carol', 10)], + allCR: [sigAt('bob', NOON - 8 * HOUR)], + reviews: [{ login: 'carol', date: AT - 4 * 3600 }], + }), + // dave's only stamp predates his request, erin never answered, a claim is no request + full({ + n: 2, + requests: [ask('dave', 5), ask('erin', 5), { ...ask('fay', 5), self: true }], + allCR: [sigAt('dave', NOON - 9 * HOUR), sigAt('fay', NOON - HOUR)], + }), + ]; + const t = requestAnswerTimes(pulls, []); + expect(t.sampled).toBe(2); + expect(t.medianHours).toBe(4); + expect(t.p90Hours).toBe(6); + }); + + it('is empty without requests', () => { + expect(requestAnswerTimes([full({})], [])).toEqual({ + medianHours: 0, + p90Hours: 0, + sampled: 0, + }); }); }); -describe('firstCrLatency', () => { - it('measures created→first CR on merged pulls only', () => { - const closed = [ - { - merged_at: new Date(NOON).toISOString(), - created_at: new Date(NOON - 10 * HOUR).toISOString(), - status: { - allCR: [sigAt('bob', NOON - 6 * HOUR), sigAt('carol', NOON - 2 * HOUR)], - allQA: [], - }, - }, // first CR after 4h - { - merged_at: new Date(NOON).toISOString(), - created_at: new Date(NOON - 10 * HOUR).toISOString(), - status: { allCR: [sigAt('bob', NOON - 4 * HOUR)], allQA: [] }, - }, // 6h - { - merged_at: null, - created_at: new Date(NOON - 10 * HOUR).toISOString(), - status: { allCR: [sigAt('bob', NOON)], allQA: [] }, - }, // unmerged, ignored - ] as unknown as PullData[]; - const lat = firstCrLatency(closed); - expect(lat.sampled).toBe(2); - expect(lat.medianHours).toBe(5); - expect(lat.avgHours).toBe(5); +describe('waitingOnSomeone', () => { + it('lists unanswered requests by hours and outside pulls by days, never self-review', () => { + const w = waitingOnSomeone( + [ + full({ n: 1, requests: [ask('bob', 30), ask('carol', 2)], allCR: [] }), + full({ + n: 2, + requests: [ask('dave', 4)], + allCR: [sigAt('dave', NOON - HOUR)], // answered + }), + full({ n: 3, ownReview: false, ageDays: 6, author: 'zed' }), // outside, unasked + full({ n: 4, ownReview: false, ageDays: 9, requested: ['bob'] }), // asked: not outside + full({ n: 5, ageDays: 20 }), // own review: nobody owes it + full({ n: 6, ownReview: false, ageDays: 1, status: 'ready' }), // not short of CR + ], + AT + ); + expect(w.requests.map(r => [r.number, r.login, r.hours])).toEqual([ + [1, 'bob', 30], + [1, 'carol', 2], + ]); + expect(w.outside).toEqual([ + { repo: 'iFixit/ifixit', number: 3, title: 't', author: 'zed', days: 6 }, + ]); }); }); +function mergedPull(over: { + n: number; + author?: string; + title?: string; + body?: string; + at?: number; + cr?: string[]; + reviewers?: string[]; + teams?: string[]; +}): PullData { + return { + repo: 'iFixit/ifixit', + number: over.n, + title: over.title ?? 'Add a thing', + body: over.body ?? '', + merged_at: new Date(over.at ?? NOON).toISOString(), + user: { login: over.author ?? 'alice' }, + requested_teams: over.teams ?? [], + requested_reviewers: over.reviewers ?? [], + review_requests: [], + status: { allCR: (over.cr ?? []).map(l => sigAt(l, NOON)), allQA: [] }, + } as unknown as PullData; +} + +describe('selfReviewMix', () => { + it('sorts merged pulls into asked, reviewed by others, self-reviewed, and unstamped', () => { + const mix = selfReviewMix([ + mergedPull({ n: 1, reviewers: ['bob'], cr: ['alice'] }), // asked wins + mergedPull({ n: 2, teams: ['store'] }), + mergedPull({ n: 3, cr: ['bob'] }), + mergedPull({ n: 4, cr: ['alice'] }), + mergedPull({ n: 5, cr: ['alice'] }), + mergedPull({ n: 6 }), + { ...mergedPull({ n: 7 }), merged_at: null } as unknown as PullData, // closed unmerged + ]); + expect(mix).toMatchObject({ merged: 6, asked: 2, byOthers: 1, self: 2, unstamped: 1 }); + }); + + it('flags reverts and fix-ups of a self-reviewed pull within a week', () => { + const mix = selfReviewMix([ + mergedPull({ n: 10, cr: ['alice'], at: NOON - 2 * 86_400_000 }), // self-reviewed + mergedPull({ n: 11, title: 'Revert "Add a thing"', body: 'Reverts #10', at: NOON }), + mergedPull({ n: 12, body: 'Fixes #10', at: NOON }), + mergedPull({ n: 13, body: 'Fixes #99', at: NOON }), // not on the board + mergedPull({ n: 14, title: 'Revert the thing', at: NOON }), // revert, no known source + mergedPull({ n: 15, cr: ['alice'], at: NOON - 20 * 86_400_000 }), + mergedPull({ n: 16, body: 'Fixes #15', at: NOON }), // 20 days later: not close + ]); + expect(mix.risk).toEqual({ reverts: 2, afterSelfReview: 2 }); + }); +}); describe('reciprocity', () => { it('pairs distinct PRs reviewed against stamps received, ignoring self-stamps', () => { const pulls = [ @@ -316,26 +390,44 @@ describe('reciprocity', () => { }); describe('authorLoad', () => { - it('counts open PRs per author with their oldest and awaiting-CR share', () => { + it('splits pulls short of CR into self-review and waiting on others', () => { const rows = authorLoad([ - full({ author: 'alice', n: 1, ageDays: 3, status: 'needs_cr' }), + full({ author: 'alice', n: 1, ageDays: 3, status: 'needs_cr' }), // own review full({ author: 'alice', n: 2, ageDays: 9, status: 'ready' }), - full({ author: 'bob', n: 3, ageDays: 1, status: 'needs_recr' }), + full({ author: 'bob', n: 3, ageDays: 1, status: 'needs_recr', ownReview: false }), ]); - expect(rows[0]).toEqual({ login: 'alice', count: 2, awaitingCr: 1, oldestDays: 9 }); - expect(rows[1]).toEqual({ login: 'bob', count: 1, awaitingCr: 1, oldestDays: 1 }); + expect(rows[0]).toEqual({ + login: 'alice', + count: 2, + inSelfReview: 1, + waitingOnOthers: 0, + oldestDays: 9, + }); + expect(rows[1]).toEqual({ + login: 'bob', + count: 1, + inSelfReview: 0, + waitingOnOthers: 1, + oldestDays: 1, + }); }); }); describe('repoBreakdown', () => { - it('splits each repo pile into total, awaiting-CR, and oldest', () => { + it('splits each repo pile into total, self-review, waiting on others, and oldest', () => { const rows = repoBreakdown([ full({ repo: 'iFixit/a', status: 'needs_cr', ageDays: 5 }), full({ repo: 'iFixit/a', status: 'ready', ageDays: 2 }), - full({ repo: 'iFixit/b', status: 'needs_recr', ageDays: 1 }), + full({ repo: 'iFixit/b', status: 'needs_recr', ageDays: 1, ownReview: false }), ]); - expect(rows[0]).toEqual({ repo: 'iFixit/a', count: 2, awaitingCr: 1, oldestDays: 5 }); - expect(rows[1]).toEqual({ repo: 'iFixit/b', count: 1, awaitingCr: 1, oldestDays: 1 }); + expect(rows[0]).toEqual({ + repo: 'iFixit/a', + count: 2, + inSelfReview: 1, + waitingOnOthers: 0, + oldestDays: 5, + }); + expect(rows[1]).toMatchObject({ repo: 'iFixit/b', inSelfReview: 0, waitingOnOthers: 1 }); }); }); diff --git a/frontend-v2/src/model/stats.ts b/frontend-v2/src/model/stats.ts index 54fbd2ba..a63e7e85 100644 --- a/frontend-v2/src/model/stats.ts +++ b/frontend-v2/src/model/stats.ts @@ -65,38 +65,6 @@ export function signoffLeaders( .sort((a, b) => b.count - a.count || a.login.localeCompare(b.login)); } -export interface Starved { - login: string; - /** their open PRs still short of full CR */ - count: number; - /** summed open-days across those PRs (the waiting they're carrying) */ - totalDays: number; - /** the single longest-waiting one */ - worstDays: number; -} - -/** - * Whose authored PRs are waiting longest for CR. A pull counts while it's - * CR-incomplete (needs_cr or needs_recr); the author is ranked by the total - * open-days their unreviewed PRs have piled up, with the worst single wait - * alongside so one ancient PR and ten fresh ones read differently. - */ -export function crStarvation(pulls: DerivedPull[]): Starved[] { - const by = new Map(); - for (const p of pulls) { - if (!CR_INCOMPLETE.includes(p.status)) continue; - const login = p.data.user.login; - const cur = by.get(login) ?? { count: 0, totalDays: 0, worstDays: 0 }; - cur.count += 1; - cur.totalDays += p.ageDays; - cur.worstDays = Math.max(cur.worstDays, p.ageDays); - by.set(login, cur); - } - return [...by.entries()] - .map(([login, v]) => ({ login, ...v })) - .sort((a, b) => b.totalDays - a.totalDays || b.worstDays - a.worstDays); -} - export interface MergeBucket { weight: Weight; count: number; @@ -181,8 +149,9 @@ export function mergedPerDay(closed: PullData[], days: number, now: number): Day /** * CR+QA stamps landed per local day — the board's review pulse. Counts every - * signature (a re-stamp is new review work), across open pulls and the closed - * window, so a day reads "how much reviewing happened", not "how much survived". + * signature (a re-stamp is new review work) on someone else's pull, across open + * pulls and the closed window; an author's own stamp is normal under + * self-review and says nothing about reviewing. */ export function stampsPerDay( pulls: DerivedPull[], @@ -193,6 +162,7 @@ export function stampsPerDay( const buckets = dayBuckets(days, now); const add = (d: PullData) => { for (const s of [...d.status.allCR, ...d.status.allQA]) { + if (s.data.user.login === d.user.login) continue; const t = Date.parse(String(s.data.created_at)); if (Number.isFinite(t)) bump(buckets, t); } @@ -238,64 +208,172 @@ export function effortMix(pulls: DerivedPull[]): EffortMix { return { buckets: WEIGHT_ORDER.map(weight => ({ weight, count: by.get(weight)! })), estimated }; } -export interface ReviewDebt { - /** CR stamps still needed across CR-incomplete pulls */ - crSlots: number; - /** QA stamps still needed across pulls at the QA gate */ - qaSlots: number; - /** stamps a push invalidated that haven't been refreshed (re-CR + re-QA) */ - restamps: number; - /** needs-QA pulls nobody has claimed or owes a re-QA on */ - unclaimedQa: number; +const sec = (iso: unknown) => Date.parse(String(iso)) / 1000; + +/** epoch secs of every stamp or GitHub review by `login` on this pull */ +function answersBy(d: PullData, login: string): number[] { + const stamps = [...d.status.allCR, ...d.status.allQA] + .filter(s => s.data.user.login === login) + .map(s => sec(s.data.created_at)); + const reviews = (d.status.unstamped_reviewers ?? []) + .filter(r => r.login === login) + .map(r => r.date); + return [...stamps, ...reviews].filter(Number.isFinite); +} + +/** requests someone else made of a person, with a known time */ +function openedRequests(d: PullData) { + return (d.review_requests ?? []).filter( + r => !r.self && r.at != null && r.login !== d.user.login + ) as { login: string; at: number }[]; +} + +export interface AnswerTime { + medianHours: number; + p90Hours: number; + /** requests with a known time that the asked person answered */ + sampled: number; } /** - * What the board is owed, in stamps: the reviewer-hours of standing debt. The - * restamp count is the cheapest debt to clear — the reviewer already knows - * the code. + * How long a review request takes to get answered: from the request to that + * person's first stamp or GitHub review after it. The policy says hours. + * GitHub drops a request once the person reviews, so closed pulls rarely keep + * their requests; what's measured is every request still on the wire, open + * pulls and the closed window alike. A person's comment that isn't a stamp + * or a review isn't on the board, so it doesn't count as an answer. */ -export function reviewDebt(pulls: DerivedPull[]): ReviewDebt { - const debt = { crSlots: 0, qaSlots: 0, restamps: 0, unclaimedQa: 0 }; - for (const p of pulls) { - if (CR_INCOMPLETE.includes(p.status)) - debt.crSlots += Math.max(p.data.status.cr_req - p.crHave, 0); - if (p.status === 'needs_qa') { - debt.qaSlots += Math.max(p.data.status.qa_req - p.qaHave, 0); - if (!p.qaingLogin && !p.reqaBy.length) debt.unclaimedQa += 1; +export function requestAnswerTimes(pulls: DerivedPull[], closed: PullData[]): AnswerTime { + const hours: number[] = []; + for (const d of [...pulls.map(p => p.data), ...closed]) { + for (const r of openedRequests(d)) { + const first = Math.min(...answersBy(d, r.login).filter(t => t >= r.at)); + if (Number.isFinite(first)) hours.push((first - r.at) / 3600); } - debt.restamps += p.recrBy.length + p.reqaBy.length; } - return debt; + return { medianHours: median(hours), p90Hours: percentile(hours, 0.9), sampled: hours.length }; } -export interface Latency { - medianHours: number; - avgHours: number; - /** merged pulls that had a first CR stamp with a usable timestamp */ - sampled: number; +function percentile(xs: number[], q: number): number { + if (!xs.length) return 0; + const s = [...xs].sort((a, b) => a - b); + return s[Math.min(s.length - 1, Math.ceil(q * s.length) - 1)]; +} + +export interface PullRef { + repo: string; + number: number; + title: string; +} + +export interface OpenRequest extends PullRef { + /** who was asked */ + login: string; + hours: number; +} + +export interface OutsideWait extends PullRef { + author: string; + days: number; +} + +export interface WaitingOnSomeone { + /** open requests nobody has answered yet, longest first */ + requests: OpenRequest[]; + /** pulls from outside the dev team waiting on a first review, oldest first */ + outside: OutsideWait[]; } /** - * How long a merged PR waited for its FIRST CR — the responsiveness number, - * distinct from time-to-merge (which includes the author's own iteration). + * What's waiting on a person who isn't the author: review requests with no + * answer yet (hours since the request) and pulls from outside the dev team + * that nobody was asked to review (days open). An author's own review of their + * own pull is not waiting on anyone, so it never shows here. */ -export function firstCrLatency(closed: PullData[]): Latency { - const hours: number[] = []; - for (const d of closed) { - if (!d.merged_at || !d.status.allCR.length) continue; - const first = Math.min( - ...d.status.allCR.map(s => Date.parse(String(s.data.created_at)) || Infinity) - ); - const h = (first - Date.parse(d.created_at)) / 3_600_000; - if (Number.isFinite(h) && h > 0) hours.push(h); +export function waitingOnSomeone(pulls: DerivedPull[], now: number): WaitingOnSomeone { + const requests: OpenRequest[] = []; + const outside: OutsideWait[] = []; + for (const p of pulls) { + const d = p.data; + const ref = { repo: d.repo, number: d.number, title: d.title }; + for (const r of openedRequests(d)) { + if (answersBy(d, r.login).some(t => t >= r.at)) continue; + requests.push({ ...ref, login: r.login, hours: Math.max(0, (now - r.at) / 3600) }); + } + const asked = (d.requested_reviewers ?? []).length || (d.requested_teams ?? []).length; + if (!p.ownReview && !asked && CR_INCOMPLETE.includes(p.status)) + outside.push({ ...ref, author: d.user.login, days: p.ageDays }); } return { - medianHours: median(hours), - avgHours: hours.length ? hours.reduce((a, b) => a + b, 0) / hours.length : 0, - sampled: hours.length, + requests: requests.sort((a, b) => b.hours - a.hours), + outside: outside.sort((a, b) => b.days - a.days), }; } +export interface SelfReviewMix { + /** merged in the window */ + merged: number; + /** a request was on record (person or team) */ + asked: number; + /** no request on record, but someone other than the author stamped or reviewed */ + byOthers: number; + /** only the author's own stamp */ + self: number; + /** merged with no stamp or review at all */ + unstamped: number; + /** rough risk signal over the self-reviewed merges */ + risk: { reverts: number; afterSelfReview: number }; +} + +const REF = /\b(?:fix(?:e[sd])?|revert(?:s|ed)?)\s+(?:[\w.-]+\/[\w.-]+)?#(\d+)/gi; +const WEEK_SECS = 7 * 86400; + +/** + * Of the pulls merged in the window, who reviewed them: asked for a review, + * someone else stamped it unasked, or the author alone. GitHub clears a + * request once it's answered, so "asked" undercounts: an answered request + * lands in "reviewed by someone else". The risk signal is rough on purpose: + * titles that start with "Revert", and any merge that says "Fixes #n" or + * "Reverts #n" of a self-reviewed pull in the same repo merged up to a week + * before it. It sees only what the title and description say. + */ +export function selfReviewMix(closed: PullData[]): SelfReviewMix { + const mergedAt = (d: PullData) => sec(d.merged_at); + const mix = { merged: 0, asked: 0, byOthers: 0, self: 0, unstamped: 0 }; + const selfMerged = new Map(); + const mergedPulls = closed.filter(d => d.merged_at); + for (const d of mergedPulls) { + mix.merged += 1; + const author = d.user.login; + const asked = + (d.requested_teams ?? []).length || + (d.requested_reviewers ?? []).length || + (d.review_requests ?? []).some(r => !r.self); + const stampers = [...d.status.allCR, ...d.status.allQA].map(s => s.data.user.login); + const reviewers = (d.status.unstamped_reviewers ?? []).map(r => r.login); + const others = [...stampers, ...reviewers].some(l => l !== author); + if (asked) mix.asked += 1; + else if (others) mix.byOthers += 1; + else if (stampers.includes(author)) { + mix.self += 1; + selfMerged.set(`${d.repo}#${d.number}`, mergedAt(d)); + } else mix.unstamped += 1; + } + let reverts = 0; + let afterSelfReview = 0; + for (const d of mergedPulls) { + const isRevert = /^revert\b/i.test(d.title); + if (isRevert) reverts += 1; + const text = `${d.title}\n${d.body ?? ''}`; + const hitsSelf = [...text.matchAll(REF)].some(m => { + const at = selfMerged.get(`${d.repo}#${m[1]}`); + return at != null && mergedAt(d) >= at && mergedAt(d) - at <= WEEK_SECS; + }); + if (hitsSelf) afterSelfReview += 1; + } + return { ...mix, risk: { reverts, afterSelfReview } }; +} + export interface Reciprocity { login: string; /** distinct PRs this person stamped (CR or QA) for someone else */ @@ -336,21 +414,37 @@ export function reciprocity(pulls: DerivedPull[], closed: PullData[]): Reciproci ); } +interface Load { + count: number; + inSelfReview: number; + waitingOnOthers: number; + oldestDays: number; +} +const emptyLoad = (): Load => ({ count: 0, inSelfReview: 0, waitingOnOthers: 0, oldestDays: 0 }); +/** a pull short of CR is the author's own to review, or waits on someone else */ +function countReview(load: Load, p: DerivedPull) { + if (!CR_INCOMPLETE.includes(p.status)) return; + if (p.ownReview) load.inSelfReview += 1; + else load.waitingOnOthers += 1; +} + export interface AuthorLoad { login: string; count: number; - /** how many of those still need CR (the reviewer-facing share) */ - awaitingCr: number; + /** needing review, and the author's own to do under self-review */ + inSelfReview: number; + /** needing review from someone who was asked, or from outside the dev team */ + waitingOnOthers: number; oldestDays: number; } /** who has the most open PRs on the board (work-in-progress load). */ export function authorLoad(pulls: DerivedPull[]): AuthorLoad[] { - const by = new Map(); + const by = new Map(); for (const p of pulls) { - const cur = by.get(p.data.user.login) ?? { count: 0, awaitingCr: 0, oldestDays: 0 }; + const cur = by.get(p.data.user.login) ?? emptyLoad(); cur.count += 1; - if (CR_INCOMPLETE.includes(p.status)) cur.awaitingCr += 1; + countReview(cur, p); cur.oldestDays = Math.max(cur.oldestDays, p.ageDays); by.set(p.data.user.login, cur); } @@ -365,18 +459,18 @@ export function authorLoad(pulls: DerivedPull[]): AuthorLoad[] { export interface RepoLoad { repo: string; count: number; - /** how many of those still need CR (the reviewer-facing share) */ - awaitingCr: number; + inSelfReview: number; + waitingOnOthers: number; oldestDays: number; } /** where the open PRs live, and how much of each repo's pile is review work. */ export function repoBreakdown(pulls: DerivedPull[]): RepoLoad[] { - const by = new Map(); + const by = new Map(); for (const p of pulls) { - const cur = by.get(p.data.repo) ?? { count: 0, awaitingCr: 0, oldestDays: 0 }; + const cur = by.get(p.data.repo) ?? emptyLoad(); cur.count += 1; - if (CR_INCOMPLETE.includes(p.status)) cur.awaitingCr += 1; + countReview(cur, p); cur.oldestDays = Math.max(cur.oldestDays, p.ageDays); by.set(p.data.repo, cur); } diff --git a/frontend-v2/src/views/Stats.tsx b/frontend-v2/src/views/Stats.tsx index dbd8090f..60771497 100644 --- a/frontend-v2/src/views/Stats.tsx +++ b/frontend-v2/src/views/Stats.tsx @@ -3,18 +3,18 @@ import type { DerivedPull } from '../../../shared/model/status'; import { ageMix, authorLoad, - crStarvation, effortMix, - firstCrLatency, friction, mergedPerDay, mergeTimeBySize, reciprocity, + requestAnswerTimes, repoBreakdown, - reviewDebt, + selfReviewMix, signoffLeaders, stampsPerDay, statusBreakdown, + waitingOnSomeone, } from '../model/stats'; import { fillMonthGaps, useStatsHistory, weeklyTimeInReview } from '../model/statsHistory'; import { ageRotDays as deriveAgeRotDays, useSettings } from '../settings'; @@ -22,7 +22,6 @@ import type { PullData } from '../../../shared/types'; import { EmptyState, STATUS_DOT, STATUS_LABEL } from '../components/bits'; import { AgeMixCard } from './stats/AgeMixCard'; import { AuthorLoadCard } from './stats/AuthorLoadCard'; -import { DebtCard } from './stats/DebtCard'; import { EffortMixCard } from './stats/EffortMixCard'; import { FrictionCard } from './stats/FrictionCard'; import { Leaderboard } from './stats/Leaderboard'; @@ -30,10 +29,11 @@ import { MergeSizeCard } from './stats/MergeSizeCard'; import { MonthlyThroughputCard } from './stats/MonthlyThroughputCard'; import { ReciprocityCard } from './stats/ReciprocityCard'; import { RepoLoadCard } from './stats/RepoLoadCard'; +import { SelfReviewCard } from './stats/SelfReviewCard'; import { ShippingPulseCard } from './stats/ShippingPulseCard'; -import { StarvationCard } from './stats/StarvationCard'; import { StatusBar } from './stats/StatusBar'; import { TimeInReviewCard } from './stats/TimeInReviewCard'; +import { WaitingCard } from './stats/WaitingCard'; const WINDOW_DAYS = 14; @@ -85,14 +85,15 @@ export function Stats({ ); // flow — the closed window plus stamp timestamps const perDay = useMemo(() => mergedPerDay(closed, WINDOW_DAYS, Date.now()), [closed]); - const firstCr = useMemo(() => firstCrLatency(closed), [closed]); + const answer = useMemo(() => requestAnswerTimes(pulls, closed), [pulls, closed]); + const selfMix = useMemo(() => selfReviewMix(closed), [closed]); const pulse = useMemo( () => stampsPerDay(pulls, closed, WINDOW_DAYS, Date.now()), [pulls, closed] ); const merge = useMemo(() => mergeTimeBySize(closed), [closed]); // the board right now - const debt = useMemo(() => reviewDebt(pulls), [pulls]); + const waiting = useMemo(() => waitingOnSomeone(pulls, Date.now() / 1000), [pulls]); const ages = useMemo(() => ageMix(pulls), [pulls]); const effort = useMemo(() => effortMix(pulls), [pulls]); const stuck = useMemo(() => friction(pulls), [pulls]); @@ -102,7 +103,6 @@ export function Stats({ const crLeaders = useMemo(() => signoffLeaders(pulls, closed, 'CR'), [pulls, closed]); const qaLeaders = useMemo(() => signoffLeaders(pulls, closed, 'QA'), [pulls, closed]); const giveTake = useMemo(() => reciprocity(pulls, closed), [pulls, closed]); - const starved = useMemo(() => crStarvation(pulls), [pulls]); const settings = useSettings(); // the heaviest text tier follows the warn threshold automatically, same // derivation as app.tsx's rowOpts — see ageWarnDays in settings.ts @@ -138,7 +138,7 @@ export function Stats({
- + @@ -146,17 +146,11 @@ export function Stats({ - + + - r.count), 1); const more = rows.length - shown.length; return ( - +
{shown.map(r => ( 0 ? (r.awaitingCr / r.count) * 100 : 0} + splitPct={r.count > 0 ? (r.waitingOnOthers / r.count) * 100 : 0} color="var(--slate)" splitColor="var(--warn)" - title={`${r.login}: ${r.count} open, ${r.awaitingCr} awaiting CR, oldest ${r.oldestDays}d`} + title={`${r.login}: ${r.count} open, ${r.inSelfReview} in self-review, ${r.waitingOnOthers} waiting on others, oldest ${r.oldestDays}d`} lead={} trail={ {r.count} - · {r.awaitingCr} CR · oldest {r.oldestDays}d + · {r.inSelfReview} self · {r.waitingOnOthers} waiting · oldest{' '} + {r.oldestDays}d } diff --git a/frontend-v2/src/views/stats/DebtCard.tsx b/frontend-v2/src/views/stats/DebtCard.tsx deleted file mode 100644 index 10c131cc..00000000 --- a/frontend-v2/src/views/stats/DebtCard.tsx +++ /dev/null @@ -1,51 +0,0 @@ -import type { ReviewDebt } from '../../model/stats'; -import { MiniMeter, StatsCard } from './parts'; - -/** - * The board's standing review debt, in stamps. Re-stamps get the amber tint - * when nonzero: they're the cheapest debt on the board (the reviewer already - * knows the code) and the most commonly forgotten. Each number carries a thin - * meter scaled to the card's shared max, so relative size is glanceable - * without reading four digits and doing the comparison in your head. - */ -export function DebtCard({ debt }: { debt: ReviewDebt }) { - const max = Math.max(debt.crSlots, debt.qaSlots, debt.restamps, debt.unclaimedQa, 1); - const cell = (value: number, label: string, hint: string, hot?: boolean) => ( -
-
- {value} -
-
{label}
- -
- ); - return ( - -
- {cell( - debt.crSlots, - 'CR stamps needed', - 'CR sign-offs still missing across CR-incomplete PRs' - )} - {cell( - debt.qaSlots, - 'QA stamps needed', - 'QA sign-offs still missing across PRs at the QA gate' - )} - {cell( - debt.restamps, - 're-stamps owed', - 'stamps a push invalidated; the reviewer already knows the code', - true - )} - {cell(debt.unclaimedQa, 'QA unclaimed', 'needs-QA PRs nobody has picked up')} -
-
- ); -} diff --git a/frontend-v2/src/views/stats/RepoLoadCard.tsx b/frontend-v2/src/views/stats/RepoLoadCard.tsx index e47fe638..672c904f 100644 --- a/frontend-v2/src/views/stats/RepoLoadCard.tsx +++ b/frontend-v2/src/views/stats/RepoLoadCard.tsx @@ -4,21 +4,21 @@ import { SplitBarRow, StatsCard } from './parts'; /** * Where the open PRs live. The bar splits into the repo's whole pile and the - * amber share of it still awaiting CR — the reviewer-facing debt, not just - * the total count — plus the oldest age trailing. + * amber share of it waiting on someone else's review (self-reviews aren't + * debt to anyone) — plus the oldest age trailing. */ export function RepoLoadCard({ rows, cap = 10 }: { rows: RepoLoad[]; cap?: number }) { const shown = rows.slice(0, cap); const max = Math.max(...rows.map(r => r.count), 1); const more = rows.length - shown.length; return ( - +
{shown.map(r => ( 0 ? (r.awaitingCr / r.count) * 100 : 0} + splitPct={r.count > 0 ? (r.waitingOnOthers / r.count) * 100 : 0} color="var(--ink-3)" splitColor="var(--warn)" title={r.repo} @@ -31,7 +31,7 @@ export function RepoLoadCard({ rows, cap = 10 }: { rows: RepoLoad[]; cap?: numbe {r.count} - · {r.awaitingCr} CR · {r.oldestDays}d + · {r.inSelfReview} self · {r.waitingOnOthers} waiting · {r.oldestDays}d } diff --git a/frontend-v2/src/views/stats/SelfReviewCard.tsx b/frontend-v2/src/views/stats/SelfReviewCard.tsx new file mode 100644 index 00000000..2810ab1b --- /dev/null +++ b/frontend-v2/src/views/stats/SelfReviewCard.tsx @@ -0,0 +1,48 @@ +import type { SelfReviewMix } from '../../model/stats'; +import { BarRow, StatsCard } from './parts'; + +/** + * How the pulls merged in the window were reviewed. The risk line is a rough + * signal: it only sees what titles and descriptions say. + */ +export function SelfReviewCard({ mix }: { mix: SelfReviewMix }) { + const rows = [ + { label: 'Self-reviewed', n: mix.self, color: 'var(--ink-3)' }, + { label: 'Asked for review', n: mix.asked, color: 'var(--brand)' }, + { label: 'Reviewed by someone else', n: mix.byOthers, color: 'var(--ok)' }, + { label: 'No stamp', n: mix.unstamped, color: 'var(--secondary)' }, + ]; + return ( + + {mix.merged === 0 ? ( +
Nothing merged in this window.
+ ) : ( +
+ {rows.map(r => ( + {r.label}} + trail={ + + {r.n} + + · {Math.round((r.n / mix.merged) * 100)}% + + + } + /> + ))} +
+ Rough signal: {mix.risk.reverts} {mix.risk.reverts === 1 ? 'revert' : 'reverts'},{' '} + {mix.risk.afterSelfReview} fix-up or revert of a self-reviewed pull within a week +
+
+ )} +
+ ); +} diff --git a/frontend-v2/src/views/stats/ShippingPulseCard.tsx b/frontend-v2/src/views/stats/ShippingPulseCard.tsx index 7614d5a1..d7193a9b 100644 --- a/frontend-v2/src/views/stats/ShippingPulseCard.tsx +++ b/frontend-v2/src/views/stats/ShippingPulseCard.tsx @@ -1,22 +1,21 @@ -import { humanHours, type DayCount, type Latency } from '../../model/stats'; +import { humanHours, type DayCount, type AnswerTime } from '../../model/stats'; import { type AxisTick, LineChart, StatsCard } from './parts'; /** * Volume vs. speed, one panel: merges per day (bars, left axis) against * CR+QA stamps per day (line, right axis) — the Shipping and Review-pulse * cards fused, since "how much shipped" only means something next to "how - * much reviewing happened" the same days. First-CR median rides along as the - * responsiveness number time-to-merge (which includes the author's own - * iteration) hides. + * much reviewing happened" the same days. Time to answer a review request rides + * along as the responsiveness number: requests are answered in hours. */ export function ShippingPulseCard({ perDay, pulse, - firstCr, + answer, }: { perDay: DayCount[]; pulse: DayCount[]; - firstCr: Latency; + answer: AnswerTime; }) { const total = perDay.reduce((a, d) => a + d.count, 0); const half = Math.floor(perDay.length / 2); @@ -95,15 +94,17 @@ export function ShippingPulseCard({ )} - {firstCr.sampled > 0 && ( - - first CR in{' '} + {answer.sampled > 0 && ( + + requests answered in{' '} - {humanHours(firstCr.medianHours)} + {humanHours(answer.medianHours)} {' '} - median + median,{' '} + + {humanHours(answer.p90Hours)} + {' '} + p90 )}
diff --git a/frontend-v2/src/views/stats/StarvationCard.tsx b/frontend-v2/src/views/stats/StarvationCard.tsx deleted file mode 100644 index ed41620f..00000000 --- a/frontend-v2/src/views/stats/StarvationCard.tsx +++ /dev/null @@ -1,56 +0,0 @@ -import { BarRow, PersonCell, StatsCard } from './parts'; - -export function StarvationCard({ - rows, - me, - onPerson, - warnDays, - rotDays, - cap = 10, -}: { - rows: { login: string; count: number; totalDays: number; worstDays: number }[]; // sorted desc by totalDays - me?: string; - onPerson?: (login: string) => void; - /** worstDays >= warnDays => amber heat; >= rotDays => red heat */ - warnDays: number; - rotDays: number; - cap?: number; -}) { - const shown = rows.slice(0, cap); - const max = rows[0]?.totalDays ?? 0; - - return ( - - {shown.length === 0 ? ( -
Nothing has been waiting long for CR.
- ) : ( -
- {shown.map(row => { - const heat = - row.worstDays >= rotDays - ? 'var(--bad)' - : row.worstDays >= warnDays - ? 'var(--warn)' - : 'var(--ink-3)'; - return ( - 0 ? (row.totalDays / max) * 100 : 0} - color={heat} - lead={} - trail={ - - {row.count} {row.count === 1 ? 'PR' : 'PRs'} · worst {row.worstDays}d - - } - /> - ); - })} -
- )} -
- ); -} diff --git a/frontend-v2/src/views/stats/WaitingCard.tsx b/frontend-v2/src/views/stats/WaitingCard.tsx new file mode 100644 index 00000000..844dd870 --- /dev/null +++ b/frontend-v2/src/views/stats/WaitingCard.tsx @@ -0,0 +1,75 @@ +import { shortRepo } from '../../../../shared/format'; +import { humanHours, type PullRef, type WaitingOnSomeone } from '../../model/stats'; +import { PersonCell, StatsCard } from './parts'; + +const link = (p: PullRef) => ( + + {shortRepo(p.repo)}#{p.number} {p.title} + +); + +/** + * Review owed to a person other than the author: requests nobody has + * answered (hours, the policy's clock) and pulls from outside the dev team + * (days). An author's own review of their own pull is not waiting on anyone. + */ +export function WaitingCard({ + waiting, + me, + onPerson, + cap = 6, +}: { + waiting: WaitingOnSomeone; + me?: string; + onPerson?: (login: string) => void; + cap?: number; +}) { + const { requests, outside } = waiting; + const empty = !requests.length && !outside.length; + return ( + + {empty ? ( +
Nobody is waiting on a review.
+ ) : ( +
+ {requests.slice(0, cap).map(r => ( +
+ {link(r)} + + + {humanHours(r.hours)} + +
+ ))} + {requests.length > cap && ( +
+{requests.length - cap} more requests
+ )} + {outside.length > 0 && ( +
+ From outside the dev team +
+ )} + {outside.slice(0, cap).map(o => ( +
+ {link(o)} + {o.author} + {o.days}d +
+ ))} + {outside.length > cap && ( +
+{outside.length - cap} more pulls
+ )} +
+ )} +
+ ); +} From c47c2f61a7819ccecc684d0fa9f53ba203035f3c Mon Sep 17 00:00:00 2001 From: Jarred Stelfox Date: Fri, 9 Oct 2026 17:31:11 -0700 Subject: [PATCH 04/17] Never count a bot as a developer A bot's PR still needs someone else's review, but derive() only knew the [bot] suffix, so with no roster configured a config-listed bot like ifixit-systems counted as reviewing its own. The review policy now carries config.bots, on the board and the server. Co-Authored-By: Claude Opus 5.5 --- frontend-v2/src/model/status.test.ts | 16 ++++++++++++++++ frontend-v2/src/store.ts | 2 +- lib/review-model.js | 2 +- shared/model/status.ts | 17 +++++++++++++---- 4 files changed, 31 insertions(+), 6 deletions(-) diff --git a/frontend-v2/src/model/status.test.ts b/frontend-v2/src/model/status.test.ts index 1d8cddbb..6fd57d2f 100644 --- a/frontend-v2/src/model/status.test.ts +++ b/frontend-v2/src/model/status.test.ts @@ -472,6 +472,22 @@ describe('the self-review policy', () => { expect(d.ownReview).toBe(false); expect(isDeveloper(OUTSIDER, 'R')).toBe(true); }); + + it('a bot is never a developer, by suffix or by config', () => { + const policy = reviewPolicy(undefined, ['ifixit-systems']); + expect(isDeveloper(policy, 'dependabot[bot]')).toBe(false); + expect(isDeveloper(policy, 'iFixit-Systems')).toBe(false); + expect(isDeveloper(policy, 'someone')).toBe(true); + const d = derive( + pull({ user: { login: 'ifixit-systems' } }), + undefined, + NOW, + undefined, + undefined, + policy + ); + expect(d.ownReview).toBe(false); + }); }); describe('starvation and weight', () => { diff --git a/frontend-v2/src/store.ts b/frontend-v2/src/store.ts index c48d98d6..fd639fd1 100644 --- a/frontend-v2/src/store.ts +++ b/frontend-v2/src/store.ts @@ -327,7 +327,7 @@ function start() { repoSpecs = payload.repos; // server-owned config rides with the board (see shared/types) weightLabels = parseWeightLabels(payload.weightLabels); - policy = reviewPolicy(payload.developerTeams); + policy = reviewPolicy(payload.developerTeams, payload.bots); extraBots = new Set(payload.bots ?? []); projectLabelPrefix = payload.projectLabelPrefix || null; firstBuild ??= payload.build; diff --git a/lib/review-model.js b/lib/review-model.js index d5197ce3..158ef0e4 100644 --- a/lib/review-model.js +++ b/lib/review-model.js @@ -41,7 +41,7 @@ export function toWire(pull) { export const developerTeams = () => savedSetting('developer_teams') ?? config.projects?.developerTeams; -const currentPolicy = () => reviewPolicy(developerTeams()); +const currentPolicy = () => reviewPolicy(developerTeams(), config.bots || []); /** * Derive one Pull into its DerivedPull (status bucket, CI verdict, weight, diff --git a/shared/model/status.ts b/shared/model/status.ts index 5c780309..157c41c0 100644 --- a/shared/model/status.ts +++ b/shared/model/status.ts @@ -266,11 +266,17 @@ export interface ReviewPolicy { /** roster team name, lowercased and slugged like GitHub's ("Store * Front" -> "store-front"), to its logins */ teams: ReadonlyMap; + /** config `bots` beyond the `[bot]` suffix: never developers, so their + * PRs still need someone else's review */ + bots: ReadonlySet; } const slug = (name: string) => name.trim().toLowerCase().replace(/\s+/g, '-'); -export function reviewPolicy(developerTeams?: Record | null): ReviewPolicy { +export function reviewPolicy( + developerTeams?: Record | null, + bots: readonly string[] = [] +): ReviewPolicy { const teams = new Map(); const developers = new Set(); for (const [name, logins] of Object.entries(developerTeams ?? {})) { @@ -278,13 +284,16 @@ export function reviewPolicy(developerTeams?: Record | null): teams.set(slug(name), logins); logins.forEach(l => developers.add(l.toLowerCase())); } - return { developers, teams }; + return { developers, teams, bots: new Set(bots.map(b => b.toLowerCase())) }; } -const EVERYONE_DEVELOPS: ReviewPolicy = { developers: new Set(), teams: new Map() }; +const EVERYONE_DEVELOPS: ReviewPolicy = reviewPolicy(); +/** a bot never reviews its own PRs, whatever the roster says */ export const isDeveloper = (policy: ReviewPolicy, login: string) => - !policy.developers.size || policy.developers.has(login.toLowerCase()); + !isSuffixBot(login) && + !policy.bots.has(login.toLowerCase()) && + (!policy.developers.size || policy.developers.has(login.toLowerCase())); export function derive( pull: PullData, From b85cd66f1f200a5f5629f0a6d18c5650b605d791 Mon Sep 17 00:00:00 2001 From: Jarred Stelfox Date: Fri, 9 Oct 2026 17:31:12 -0700 Subject: [PATCH 05/17] Rework the board's review flow for self-review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Developers review their own PRs unless they ask, so the board stops treating every unstamped PR as someone else's job. 1) Nothing assigns review: the rotation, "your turn" alerts and cheers, and "Return the favor" are gone. 2) The review queue and Needs QA hold only PRs that asked you (by name or team), ones you said you'd review, and PRs from outside the dev team or bots. Requests lead Waiting on you, oldest first, with "asked 3h ago"; a new request sends a desktop alert. 3) Your own PRs say what you owe: Stamp CR, Stamp QA, Re-stamp, or "waiting on bob · asked 3h ago", turning into "Nudge bob" after 4 hours. "Find a QA-er", "Nudge for a review" and "in the CR queue" are gone, and so is the "Getting QA is a to-do" setting. 4) Claim reads "I'll review" and "Drop it". 5) New: "Could use your input", a closed-by-default fold of other people's self-reviews in your code regions or repos you review, with no counts; and a quiet "worth asking for review?" hint on your PRs that touch CI, migrations, alerting, agent docs, deploy config or dependencies. 6) Words across Review, My work, Team, CI, Classic, Projects stages, cheers and PR jail now match the policy, and "is:asked" finds requests of you. Co-Authored-By: Claude Opus 5.5 --- frontend-v2/src/app.tsx | 47 +- frontend-v2/src/components/JailMode.test.tsx | 42 +- frontend-v2/src/components/JailMode.tsx | 42 +- frontend-v2/src/components/Legend.tsx | 14 +- frontend-v2/src/components/RegionHint.tsx | 4 +- frontend-v2/src/components/Row.tsx | 91 ++-- frontend-v2/src/components/Settings.tsx | 26 +- frontend-v2/src/components/StatePopover.tsx | 71 +-- frontend-v2/src/components/WordGroups.tsx | 28 +- frontend-v2/src/model/actions.test.ts | 283 ++++++++---- frontend-v2/src/model/actions.ts | 410 +++++++++--------- frontend-v2/src/model/cheers.test.ts | 180 ++++---- frontend-v2/src/model/cheers.ts | 212 +++------ frontend-v2/src/model/deal.test.ts | 21 +- frontend-v2/src/model/deal.ts | 35 +- .../src/model/notificationRelevance.ts | 8 +- frontend-v2/src/model/portfolio.ts | 2 +- frontend-v2/src/model/query.test.ts | 9 + frontend-v2/src/model/query.ts | 8 +- frontend-v2/src/model/reviewLanes.test.ts | 199 ++++++--- frontend-v2/src/model/reviewLanes.ts | 221 +++++----- frontend-v2/src/model/reviewers.test.ts | 34 -- frontend-v2/src/model/reviewers.ts | 54 +-- frontend-v2/src/model/rotation.test.ts | 112 ----- frontend-v2/src/model/rotation.ts | 99 ----- frontend-v2/src/model/sort.test.ts | 18 + frontend-v2/src/model/sort.ts | 18 +- frontend-v2/src/model/stage.test.ts | 52 ++- frontend-v2/src/model/stage.ts | 43 +- frontend-v2/src/model/team.test.ts | 13 + frontend-v2/src/model/team.ts | 18 +- frontend-v2/src/model/toast.ts | 6 - frontend-v2/src/model/updateDraft.test.ts | 4 +- frontend-v2/src/notifications.ts | 47 +- frontend-v2/src/settings.ts | 11 +- frontend-v2/src/toasts.tsx | 77 +--- frontend-v2/src/views/Ci.tsx | 17 +- frontend-v2/src/views/MyWork.tsx | 11 +- frontend-v2/src/views/Review.tsx | 196 +++++---- frontend-v2/src/views/Team.tsx | 72 ++- frontend-v2/src/views/projects/Portfolio.tsx | 2 +- frontend-v2/src/views/projects/Work.tsx | 7 +- 42 files changed, 1314 insertions(+), 1550 deletions(-) delete mode 100644 frontend-v2/src/model/reviewers.test.ts delete mode 100644 frontend-v2/src/model/rotation.test.ts delete mode 100644 frontend-v2/src/model/rotation.ts diff --git a/frontend-v2/src/app.tsx b/frontend-v2/src/app.tsx index 0fff1684..338a9eaa 100644 --- a/frontend-v2/src/app.tsx +++ b/frontend-v2/src/app.tsx @@ -11,16 +11,15 @@ import { } from 'react'; import { ago, closedEpoch, n, pullKey, shortRepo } from '../../shared/format'; import type { ActionStateKey } from './model/actions'; -import { actionState } from './model/actions'; +import { actionState, askedOf } from './model/actions'; import { DEFAULT_RANGE, DEFAULT_SORT, LENS_LABELS, ORIGIN_KEYS, ZOOM_KEY, type Lens } from './lens'; import type { DerivedPull } from '../../shared/model/status'; import { matchesWeightFilter } from '../../shared/model/status'; import { buildParentLookup } from './model/stack'; -import { buildReviewerPools, turnFor } from './model/rotation'; import { shipRelevance, shippedToast } from './model/shipped'; import { notificationStale } from './model/notificationRelevance'; import type { Toast } from './model/toast'; -import { claimReview, isSnoozed, refreshAllText, usePulldasher } from './store'; +import { isSnoozed, refreshAllText, usePulldasher } from './store'; import { primeScope, useScope } from './prefs'; import { useBoardHotkeys, useHeaderHeightVar } from './hooks'; import { ageRotDays, getSettings, type Settings as SettingsShape, useSettings } from './settings'; @@ -28,7 +27,6 @@ import { useNotifications } from './notifications'; import { ToastStack, useToasts } from './toasts'; import { requestNames, useNames } from './model/names'; import { CRYO_KEY, isBotLogin, personHidden, repoHidden } from '../../shared/model/visibility'; -import { reviewRequestedFrom } from './model/reviewers'; import { CornerBadge, QuietButton, textInputClass } from './components/bits'; import { foldDomId, openFold } from './components/Lane'; import { Legend } from './components/Legend'; @@ -300,12 +298,6 @@ export function App() { snoozed, projectLabelPrefix, } = usePulldasher(); - // per-repo reviewer pools and whose turn each starved, unclaimed pull is — - // computed ONCE over the whole board and shared by the rows (rowOpts), - // desktop notifications, and the cheers evaluator, so the three surfaces - // can never disagree about the rotation (and never re-derive it in - // parallel; buildReviewerPools walks every pull's signatures) - const pools = useMemo(() => buildReviewerPools(pulls), [pulls]); // Publish the widest CR/QA required counts on the board so every row's pip // slot pads to them (styles.css .pd-pip-slot-*) and the marks line up in a // column. Over ALL open pulls, not the filtered view: simpler, and the @@ -324,14 +316,6 @@ export function App() { root.setProperty('--cr-slots', String(maxCrReq)); root.setProperty('--qa-slots', String(maxQaReq)); }, [maxCrReq, maxQaReq]); - const turns = useMemo(() => { - const m = new Map(); - for (const p of pulls) { - const who = turnFor(p, pools, pulls); - if (who) m.set(pullKey(p.data), who); - } - return m; - }, [pulls, pools]); // resolve every author on the board to a human display name (batched, // cached — model/names.ts): the person hover-cards, the people pickers, // and name-aware search all read from this one map @@ -625,7 +609,7 @@ export function App() { p.data.draft && p.data.user.login !== me && !revealedAuthor(p.data.user.login) && - !reviewRequestedFrom(p, me); + !askedOf(p).includes(me); return hidden || cryoHidden || draftHidden; }, [ @@ -700,7 +684,7 @@ export function App() { ); // desktop notifications watch the whole board (minus the standing hidden // settings above), not the current filter - useNotifications(nudgeablePulls, me, turns, initialized); + useNotifications(nudgeablePulls, me, initialized); // scope + hidden, but deliberately NOT free text: a query drives the global // Search lens (App renders instead of the current lens), so @@ -840,7 +824,7 @@ export function App() { const c = { parked: 0, drafts: 0, hiddenRepos: 0, hiddenPeople: 0, bots: 0, hiddenNow: 0 }; for (const p of pulls) { if (p.cryo) c.parked++; - if (p.data.draft && p.data.user.login !== me && !reviewRequestedFrom(p, me)) c.drafts++; + if (p.data.draft && p.data.user.login !== me && !askedOf(p).includes(me)) c.drafts++; if (repoHidden(p.data.repo, settings.repoPrefs)) c.hiddenRepos++; if ( p.data.user.login !== me && @@ -930,8 +914,6 @@ export function App() { compact: settings.density === 'compact', laneCap: settings.laneCapByLens[lens] ?? settings.laneCap, parentOf, - pools, - turns, isBotAuthor, onProject, }), @@ -947,8 +929,6 @@ export function App() { settings.laneCapByLens, lens, parentOf, - pools, - turns, isBotAuthor, onProject, ] @@ -1047,15 +1027,6 @@ export function App() { setLens('review'); setWeightSel(['XS', 'S']); }, []); - // the "your turn" toast's Claim button: claim the review straight from the - // nudge (which also adds you as a GitHub reviewer, same as any claim) - const onClaimTurn = useCallback( - (repo: string, number: number) => { - const p = pulls.find(x => x.data.repo === repo && x.data.number === number); - if (p) claimReview(p.data); - }, - [pulls] - ); // pull keys the viewer snoozed in Review, so cheers skips them the way the // Review lane does — a snooze is "not today", not "nudge me anyway" const snoozedKeys = useMemo( @@ -1072,11 +1043,9 @@ export function App() { } = useToasts( nudgeablePulls, me, - turns, shippedExtras, closed, onQuickWins, - onClaimTurn, initialized, names, snoozedKeys, @@ -1084,7 +1053,7 @@ export function App() { ); // A fired nudge outlives the PR it points at: once that PR merges or closes - // it drops off the board, so a bell entry like "return the favor on + // it drops off the board, so a bell entry like "review requested on // fixbot#3116" is left pointing at a dead link. Drop any panel record whose // PR has left the open board -- notificationStale is general across every // kind, and exempts the retrospective shipped recap and board-wide rewards. @@ -1328,11 +1297,11 @@ export function App() { sessionActive={sessionActive} inputProps={{ 'aria-label': - 'Search all PRs, open and closed: text, #number, label:x, status:x, older:5, repo:x, author:x, weight:xs, has:action, is:draft, is:mine, is:restamp, is:blocked, is:bot', + 'Search all PRs, open and closed: text, #number, label:x, status:x, older:5, repo:x, author:x, weight:xs, has:action, is:draft, is:mine, is:restamp, is:asked, is:blocked, is:bot', // "/" finds within a view that has its own find box // (hooks.ts), so the hint only shows where "/" comes here placeholder: viewFinds ? 'Search all PRs' : 'Search all PRs (press /)', - title: 'text, #number, label:x, status:x, older:5, repo:x, author:x, weight:xs, has:action, is:draft, is:mine, is:restamp, is:blocked, is:bot', + title: 'text, #number, label:x, status:x, older:5, repo:x, author:x, weight:xs, has:action, is:draft, is:mine, is:restamp, is:asked, is:blocked, is:bot', className: `w-[210px] max-w-full grow pr-2.5 pl-8 sm:grow-0 ${textInputClass}`, }} /> diff --git a/frontend-v2/src/components/JailMode.test.tsx b/frontend-v2/src/components/JailMode.test.tsx index 5e0e5a84..476084a8 100644 --- a/frontend-v2/src/components/JailMode.test.tsx +++ b/frontend-v2/src/components/JailMode.test.tsx @@ -12,8 +12,12 @@ import { JailMode } from './JailMode'; let n = 0; /** One of my open PRs, run through the real derive; `ready` waives CR and - * QA so it can merge now, otherwise it waits on CR. */ -const pull = (ageDays = 1, { ready = false } = {}): DerivedPull => { + * QA so it can merge now, otherwise it needs CR: my own to stamp (self-review), + * or `asked`'s, whom I requested a review from an hour ago. */ +const pull = ( + ageDays = 1, + { ready = false, asked = [] as string[], hints = [] as string[] } = {} +): DerivedPull => { n += 1; const at = new Date(Date.now() - ageDays * 86400_000 - 60_000).toISOString(); const data: PullData = { @@ -47,6 +51,9 @@ const pull = (ageDays = 1, { ready = false } = {}): DerivedPull => { }, labels: [], participants: [], + requested_reviewers: asked, + review_requests: asked.map(login => ({ login, at: Date.now() / 1000 - 3600, self: false })), + input_hints: hints, }; return derive(data, undefined); }; @@ -101,7 +108,8 @@ describe('JailMode while the cell is down', () => { it('lifts ready PRs to the top and counts them in the why line', () => { const ready = pull(2, { ready: true }); render([...many(7), ready]); - expect(headers()).toEqual(['Ready to merge', 'Waiting on others']); + // the rest are mine to stamp under self-review + expect(headers()).toEqual(['Ready to merge', 'Your move']); expect(document.querySelector('[role="dialog"] li a')?.textContent).toContain( ready.data.title ); @@ -164,20 +172,38 @@ describe('JailMode while the cell is down', () => { expect(listed()).toBe(8); }); - it('copies a nudge listing the PRs waiting on others', async () => { + it('copies a nudge listing only the PRs waiting on people I asked, naming them', async () => { const writeText = vi.fn(() => Promise.resolve()); Object.assign(navigator, { clipboard: { writeText } }); - const first = pull(3); - render([first, ...many(7)]); + const first = pull(3, { asked: ['bob'] }); + const second = pull(2, { asked: ['cy', 'dee'] }); + render([first, second, ...many(6)]); + expect(headers()).toEqual(['Your move', 'Waiting on others']); const copy = [...document.querySelectorAll('[role="dialog"] button')].find( b => b.textContent === 'Copy a nudge' ); await act(async () => copy?.click()); const text = (writeText.mock.calls[0] as unknown as [string])[0]; - expect(text.split('\n')).toHaveLength(9); + expect(text.split('\n')).toHaveLength(3); expect(text).toContain( - `• ${first.data.title} https://github.com/iFixit/ifixit/pull/${first.data.number}` + `• ${first.data.title} https://github.com/iFixit/ifixit/pull/${first.data.number} (bob)` ); + expect(text).toContain(`(cy, dee)`); expect(copy?.textContent).toBe('Copied'); }); + + it('offers no nudge when I asked nobody: a self-review waits on no one', () => { + render(many(8)); + const copy = [...document.querySelectorAll('[role="dialog"] button')].find( + b => b.textContent === 'Copy a nudge' + ); + expect(copy).toBeUndefined(); + }); + + it('hints when a self-reviewed PR touches what usually deserves team input', () => { + render([pull(1, { hints: ['ci', 'migrations'] }), ...many(7)]); + expect(dialog()?.textContent).toContain( + 'touches CI and migrations · worth asking for review?' + ); + }); }); diff --git a/frontend-v2/src/components/JailMode.tsx b/frontend-v2/src/components/JailMode.tsx index 7360958d..22062086 100644 --- a/frontend-v2/src/components/JailMode.tsx +++ b/frontend-v2/src/components/JailMode.tsx @@ -3,7 +3,7 @@ import { createPortal } from 'react-dom'; import { Lock } from 'lucide-react'; import type { DerivedPull } from '../../../shared/model/status'; import { githubUrl } from '../../../shared/format'; -import { rowNote, rowWord } from '../model/actions'; +import { askedOf, askForInputHint, rowNote, rowWord } from '../model/actions'; import { STAGE_WORDS } from '../model/stage'; import { jailCase, @@ -477,22 +477,21 @@ function useLeaving(pulls: DerivedPull[]): DerivedPull[] { return leaving.filter(p => !pulls.some(q => pullId(q) === pullId(p))); } -/** Slack-ready text asking for the reviews your waiting PRs need: each - * title, its link, and who it waits on. */ -function nudgeText(rows: { pull: DerivedPull }[], me: string): string { +/** Slack-ready text chasing the reviews you asked for: each title, its + * link, and who was asked. Only PRs with a request: a self-reviewed PR waits + * on nobody, so there's no one to nudge about it. */ +function nudgeText(rows: { pull: DerivedPull }[]): string { return [ - 'A few of my PRs are waiting on a review or a stamp:', - ...rows.map(({ pull: p }) => { - const who = rowNote(p, me).context; - return `• ${p.data.title} ${githubUrl(p.data.repo, p.data.number)}${ - who ? ` (${who})` : '' - }`; - }), + 'A few of my PRs are waiting on a review I asked for:', + ...rows.map( + ({ pull: p }) => + `• ${p.data.title} ${githubUrl(p.data.repo, p.data.number)} (${askedOf(p).join(', ')})` + ), ].join('\n'); } /** Copies nudgeText, and says whether it worked. */ -function CopyNudge({ rows, me }: { rows: { pull: DerivedPull }[]; me: string }) { +function CopyNudge({ rows }: { rows: { pull: DerivedPull }[] }) { const [copied, setCopied] = useState<'copied' | 'failed' | null>(null); const done = (result: 'copied' | 'failed') => { setCopied(result); @@ -502,7 +501,7 @@ function CopyNudge({ rows, me }: { rows: { pull: DerivedPull }[]; me: string }) { if (!navigator.clipboard) return done('failed'); - navigator.clipboard.writeText(nudgeText(rows, me)).then( + navigator.clipboard.writeText(nudgeText(rows)).then( () => done('copied'), () => done('failed') ); @@ -526,16 +525,21 @@ function JailList({ pulls, me, maxDays }: { pulls: DerivedPull[]; me: string; ma ? jailGroups(all, me) : { ready: [], move: [], waiting: all.map(pull => ({ pull })) }; const live = (rows: { pull: DerivedPull }[]) => rows.filter(r => !gone.has(pullId(r.pull))); + // the PRs waiting on people you asked, wherever they sit (a request gone + // quiet past 4 hours is Your move); the copy button rides on the first + // section holding one + const asked = (rows: { pull: DerivedPull }[]) => live(rows).filter(r => askedOf(r.pull).length); + const nudgeRows = asked([...groups.move, ...groups.waiting]); const sections = [ { title: STAGE_WORDS.ready, sub: 'Merge these first', rows: groups.ready, ready: true }, { title: 'Your move', sub: 'Waiting on you', rows: groups.move }, { title: 'Waiting on others', - sub: 'Nudge them, or close what you don’t need', + sub: 'Nudge whoever you asked, or close what you don’t need', rows: groups.waiting, - nudge: true, }, ].filter(g => g.rows.length); + const nudgeOn = sections.find(g => !g.ready && asked(g.rows).length)?.title; return (
{sections.map(g => ( @@ -546,11 +550,7 @@ function JailList({ pulls, me, maxDays }: { pulls: DerivedPull[]; me: string; ma title={g.title} sub={g.sub} count={live(g.rows).length} - headerExtra={ - g.nudge && live(g.rows).length ? ( - - ) : undefined - } + headerExtra={g.title === nudgeOn ? : undefined} /> )}
    @@ -597,6 +597,7 @@ function JailRow({ // stands in when there's no detail const move = word?.kind === 'do' ? word.word : null; const detail = context ?? (word?.kind === 'wait' ? word.word : null); + const hint = mine && !ready ? askForInputHint(p) : null; const old = p.ageDays > maxDays; return (
  • } {detail && {detail}} + {hint && {hint}}
  • ); } diff --git a/frontend-v2/src/components/Legend.tsx b/frontend-v2/src/components/Legend.tsx index ef319df2..2a3b0695 100644 --- a/frontend-v2/src/components/Legend.tsx +++ b/frontend-v2/src/components/Legend.tsx @@ -147,16 +147,16 @@ export function Legend() { Claim} - def="hover a row for its verbs. Claiming adds you as a reviewer on the PR, so GitHub and the board both show you’re on it; the row moves to Waiting on you until you submit, release, or are removed" + term={self-review} + def="a developer reviews and tests their own PR unless they ask for a review. Nobody else owes it anything" /> review requested} - def="someone requested your review on GitHub. Lands in Waiting on you, and the board quiets its own suggestions for that PR" + def="the author asked people (or a team) for a review on GitHub. It leads their Waiting on you; answer within hours" /> your turn} - def="a review that sat too long, pointed at the best-matched reviewer. A nudge, not a lock: anyone can take it" + term={I’ll review} + def="hover a row for its verbs. It adds you as a reviewer on the PR, so GitHub and the board both show you’re on it; the row moves to Waiting on you until you submit, drop it, or are removed" /> @@ -173,6 +173,10 @@ export function Legend() { term={is:restamp} def="a push undid your stamp and the PR is reviewable again" /> + is:asked} + def="someone asked you for a review" + /> is:blocked} def="under a dev or deploy block, whoever holds it" diff --git a/frontend-v2/src/components/RegionHint.tsx b/frontend-v2/src/components/RegionHint.tsx index 0a605fc4..40f3c2a2 100644 --- a/frontend-v2/src/components/RegionHint.tsx +++ b/frontend-v2/src/components/RegionHint.tsx @@ -30,8 +30,8 @@ export function RegionHint() { Review a particular area? Add code regions in Settings. - A region is plain text matched against a PR’s title, labels, branch, and repo; matches - gather in their own section above the queue. + A region is plain text matched against a PR’s title, labels, branch, and repo; other + people’s self-reviews that match gather under Could use your input. )} {showSnooze && ( @@ -660,15 +655,12 @@ function RowImpl({ const d = pull.data; const key = pullKey(d); const fresh = freshKind(pull, opts); - // who's claimed to review this pull, and whose turn the rotation names — - // both optional, so a lens that hasn't threaded them (yet) just sees null - // and rowNote falls back to its base (pre-coordination) note + // who said they'd review this pull const claim = claimFor(d); - const turn = opts.turns?.get(key) ?? null; - const poolSize = opts.pools?.get(d.repo)?.length ?? 0; // the viewer-relative note (model/actions.ts): never both-null for an open // pull, so the one badge below always has something to say - const note = rowNote(pull, opts.me, { claim, turn }); + const note = rowNote(pull, opts.me, { claim }); + const askHint = opts.askHint && d.user.login === opts.me ? askForInputHint(pull) : null; // the wait badge may itself carry a "last commit …" — don't say it twice const showIterating = isIterating(pull) && !(note.context ?? '').includes('last commit'); // the smallest clean marker for a teammate: a tiny heart over their @@ -680,7 +672,7 @@ function RowImpl({ const regions = matchedRegions(pull, settings.codeRegions); // only worth asking the whole-board lookup when this row is stacked but // rendering flat (its parent isn't visible right above it already) - const orphanParent = pull.dependent && depth === 0 ? (opts.parentOf?.(pull) ?? null) : null; + const orphanParent = pull.dependent && depth === 0 ? opts.parentOf?.(pull) ?? null : null; return ( 0 && d.user.login !== opts.me && ( - {regions.length > 0 && !opts.hideRegionMark && ( + {regions.length > 0 && ( // the brand diamond leads and the matched region is NAMED in // quiet ink beside it — which of your regions hit must read // without a hover (a bare glyph hid it; owner call). No @@ -756,7 +750,9 @@ function RowImpl({ - )}