diff --git a/CLAUDE.md b/CLAUDE.md index 1b63116..e99c390 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -54,7 +54,7 @@ Glooker is a Next.js 15 web app that generates developer impact reports for a Gi - SQLite SQL translator handles `INSERT IGNORE`, `ON DUPLICATE KEY UPDATE`, and `NOW()` — if adding new MySQL-specific SQL, update `translateSQL()` in `db/sqlite.ts` - Progress store and stop-signal store use `globalThis` to survive Next.js HMR module reloads - `@octokit/rest` is ESM-only — any test file that imports from `github.ts` (directly or transitively) must mock it before the import, and it must be the **factory** form: `jest.mock('@octokit/rest', () => ({ Octokit: jest.fn().mockImplementation(() => ({})) }))`. A bare `jest.mock('@octokit/rest')` does **not** work — auto-mocking still loads the real module, so the suite dies with "Jest encountered an unexpected token" pointing at `dist-src/index.js`, which reads like a transform-config problem rather than a mocking one. -- **A GitHub search that succeeds can still be lying (GLOOK-50).** `incomplete_results: true` means the query timed out and GitHub returned only what it had found by then — so an **empty** page with that flag is an unreliable zero, not a real one. That is how six developers were recorded with 0 commits while GitHub held 60 between them, with no skip and no integrity warning. The rules, all in `github.ts`: a timeout with **items** is deliberately NOT distrusted (GitHub warns a timeout "does not necessarily mean that search results are incomplete", and on the run that produced this code 5 of 6 flagged pages were fine); an untrustworthy **empty** page is retried twice, then the date window is halved (`splitDateWindow`, max 2 levels, second half keeps an open upper bound so nothing falls off the end), and only then does it **raise** — a raise becomes a counted SKIP, which is the goal, not a workaround. Collected hits are reconciled against `total_count` **clamped to** the 1000-result cap, never gated on it. Partial-but-real data is **kept** and reported as a `SearchShortfall` → `integrity.recordError` (member-kept partial data), never discarded — throwing away 90 of 100 commits would be worse than shipping them slightly low. Pagination stops at 1000 results because paging past it returns `Only the first 1000 search results are available`. +- **A GitHub search that succeeds can still be lying (GLOOK-50).** `incomplete_results: true` means the query timed out and GitHub returned only what it had found by then — so an **empty** page with that flag is an unreliable zero, not a real one. That is how six developers were recorded with 0 commits while GitHub held 60 between them, with no skip and no integrity warning. The rules, all in `github.ts`: a timeout with **items** is deliberately NOT distrusted (GitHub warns a timeout "does not necessarily mean that search results are incomplete", and on the run that produced this code 5 of 6 flagged pages were fine); an untrustworthy **empty** page is retried twice, then the date window is halved (`splitDateWindow`, max 2 levels, second half keeps an open upper bound so nothing falls off the end), and only then does it **raise** — a raise becomes a counted SKIP, which is the goal, not a workaround. Collected hits are reconciled against `total_count` **clamped to** the 1000-result cap, never gated on it. Partial-but-real data is **kept** and reported as a `SearchShortfall` → `integrity.recordError` (member-kept partial data), never discarded — throwing away 90 of 100 commits would be worse than shipping them slightly low. Pagination stops at 1000 results because paging past it returns `Only the first 1000 search results are available`. **The endpoints are deliberately asymmetric, and this is the thing most likely to be "tidied" into a bug:** commit search stays strict (an empty timed-out page really did hide 43 commits), but for **issue/PR search an empty result routinely reports `incomplete_results: true` while being correct** — true for bots and anyone with genuinely zero merged PRs. Raising there produced 19 false skips in one run and aborted the report at 21%. So merged-PR and review-count searches retry, then raise **only** when the loss is provable — a self-contradicting page (`total_count > 0` with empty `items`), or a mid-pagination timeout after page 1 already told us how many exist. A timed-out result with **nothing collected and nothing promised** keeps the zero. **Whenever a figure is kept but unproven, it must land on the typed `unverified` channel** (`RunMetadata.unverified`, via `integrity.recordUnverified`) — never only in `errors`, which already carries hundreds of per-commit entries on a healthy run and so cannot be thresholded. `evaluateIntegrity` counts distinct unverified logins and downgrades to `degraded` at `degradedUnverifiedPct` (15%); it can never abort on them, because those members are present in the report. `IntegrityBadge` renders them even at `state === 'ok'`. Without that chain an org-wide issue-search brownout produces zero skips and ships a green report in which every developer reads 0 merged PRs. Do not unify the two policies. - **Report integrity (GLOOK-13/48): `countableSkips()` and `integrityCounts()` in `report-runner/types.ts` are the only place "which skips count" is defined.** `evaluateIntegrity`, the runner's abort message, and `IntegrityBadge` all read from them — the rule previously existed as four hand-written filters, and when one drifted the guard went green while reports lost half the org. Only `expected` (human-allowlisted via `report_skip_allowlist`) is excluded, and allowlisted members are removed from the **denominator** too, so growing the allowlist can't dilute the percentage gate. `auto-flagged` is only a *suggestion* for a human to promote in Settings — it must never silence the guard. If you add a `SkipClassification`, `COUNTABLE_SKIP_CLASSIFICATIONS` is an explicit inclusion list precisely so that becomes a deliberate decision. - Tests use Jest + ts-jest with `@/` path alias — config in `jest.config.ts` - CI runs on all pull requests and pushes to main (`.github/workflows/test.yml`) diff --git a/src/components/IntegrityBadge.tsx b/src/components/IntegrityBadge.tsx index e659521..ad8aa7e 100644 --- a/src/components/IntegrityBadge.tsx +++ b/src/components/IntegrityBadge.tsx @@ -16,7 +16,25 @@ function classificationLabel(c: SkippedMember['classification']): string { export default function IntegrityBadge({ metadata }: IntegrityBadgeProps) { const [open, setOpen] = useState(false); - if (!metadata || metadata.state === 'ok') return null; + const unverified = metadata?.unverified ?? []; + + // GLOOK-50: a run can be 'ok' and still be hiding unverified figures — an + // org-wide issue-search brownout produces zero skips, so without this the + // report renders completely clean while every developer shows 0 merged PRs. + if (!metadata || (metadata.state === 'ok' && unverified.length === 0)) return null; + + if (metadata.state === 'ok') { + return ( + + ); + } const expectedCount = metadata.expectedCount ?? 0; diff --git a/src/lib/__tests__/unit/integrity-guard-regression.test.ts b/src/lib/__tests__/unit/integrity-guard-regression.test.ts index 72a95dd..32c95a8 100644 --- a/src/lib/__tests__/unit/integrity-guard-regression.test.ts +++ b/src/lib/__tests__/unit/integrity-guard-regression.test.ts @@ -24,7 +24,8 @@ */ import { evaluateIntegrity } from '@/lib/report-runner/skip-classifier'; import { IntegrityTracker } from '@/lib/report-runner/integrity-tracker'; -import { DEFAULT_THRESHOLDS, countableSkips, integrityCounts, formatIntegrityAbortReason } from '@/lib/report-runner/types'; +import { DEFAULT_THRESHOLDS, countableSkips, integrityCounts, formatIntegrityAbortReason, unverifiedCounts } from '@/lib/report-runner/types'; +import type { UnverifiedMember } from '@/lib/report-runner/types'; import type { SkipClassification } from '@/lib/report-runner/types'; function snapshotWith(expectedCount: number, skips: Array<[string, SkipClassification]>) { @@ -160,3 +161,73 @@ describe('allowlisted members leave the denominator, not just the numerator', () expect(evaluateIntegrity(snap)).toBe('ok'); }); }); + +/** + * GLOOK-50: an unverified member is kept in the report, so it must never abort + * a run — but it must also not be invisible. Before this, an org-wide + * issue-search brownout produced ZERO skips, so the state stayed 'ok', the + * badge returned null, and every developer showed 0 merged PRs with their + * impact score silently reshuffled. The doubt was written to run_metadata and + * read by nothing. + */ +function withUnverified(expectedCount: number, n: number, field: UnverifiedMember['field'] = 'merged-prs') { + return { + expectedCount, + thresholds: DEFAULT_THRESHOLDS, + skipped: [] as never[], + unverified: Array.from({ length: n }, (_, i) => ({ + login: `u${i}`, field, kept: 0, reason: 'search timed out', + })), + }; +} + +describe('unverified members downgrade a run without aborting it', () => { + it('an org-wide brownout no longer reports a clean bill of health', () => { + // 100 of 100 members unverified: the exact shape that shipped green. + expect(evaluateIntegrity(withUnverified(100, 100))).toBe('degraded'); + }); + + it('crosses to degraded at the 15% gate', () => { + expect(evaluateIntegrity(withUnverified(100, 14))).toBe('ok'); + expect(evaluateIntegrity(withUnverified(100, 15))).toBe('degraded'); + }); + + it('NEVER aborts, however many members are unverified', () => { + // They are present in the report — losing the whole report over an + // unverified PR count would be worse than the doubt it signals. + expect(evaluateIntegrity(withUnverified(100, 100))).not.toBe('failed'); + expect(evaluateIntegrity(withUnverified(10, 10))).not.toBe('failed'); + }); + + it('a handful of unverified members stays ok', () => { + expect(evaluateIntegrity(withUnverified(100, 3))).toBe('ok'); + }); + + it('counts each login once even when several figures are unverified', () => { + // merged-prs AND reviews for the same person is one affected member. + const snap = { + expectedCount: 10, + thresholds: DEFAULT_THRESHOLDS, + skipped: [] as never[], + unverified: [ + { login: 'a', field: 'merged-prs' as const, kept: 0, reason: 'x' }, + { login: 'a', field: 'reviews' as const, kept: 0, reason: 'x' }, + ], + }; + expect(unverifiedCounts(snap).count).toBe(1); + }); + + it('excludes allowlisted members from the denominator, like the skip gate', () => { + const snap = { + expectedCount: 100, + skipped: many(40, 'expected').map(([login, classification]) => ({ + login, reason: 'ok', classification, + })), + unverified: Array.from({ length: 9 }, (_, i) => ({ + login: `u${i}`, field: 'merged-prs' as const, kept: 0, reason: 'x', + })), + }; + // 9 of 60 real members = 15%, not 9 of 100 = 9%. + expect(unverifiedCounts(snap).pct).toBeCloseTo(0.15, 5); + }); +}); diff --git a/src/lib/__tests__/unit/pr-search-false-skips.test.ts b/src/lib/__tests__/unit/pr-search-false-skips.test.ts new file mode 100644 index 0000000..aa57741 --- /dev/null +++ b/src/lib/__tests__/unit/pr-search-false-skips.test.ts @@ -0,0 +1,173 @@ +// @octokit/rest is ESM-only; FACTORY-form mock required before the import. +jest.mock('@octokit/rest', () => ({ Octokit: jest.fn().mockImplementation(() => ({})) })); + +import { fetchUserActivity, __setOctokitForTest } from '@/lib/github'; +import { formatIntegrityAbortReason, DEFAULT_THRESHOLDS } from '@/lib/report-runner/types'; + +/** + * GLOOK-50 follow-up. The first fix made the merged-PR search raise on any + * untrustworthy result. On 2026-09-09 that aborted the whole report at 21%: + * + * ABORT (GLOOK-13): 21 of 100 engineers couldn't be fetched (21%). + * Most failures are GitHub search timeouts (21 of 21) + * + * ~19 of those 21 were false. Verified against GitHub: sl-chromatic-bot, + * sl-data-team-jenkins, magdalenastaller, keser, flaksie, gkim-smartling and + * viakivchuk-smartling all have GENUINELY zero merged PRs, and an empty issue + * search routinely reports incomplete_results=true while being correct. + * + * Commit search is different and stays strict: there an empty timed-out page + * really did hide 43 commits. The asymmetry is the point. + */ + +jest.useFakeTimers({ doNotFake: ['nextTick'], now: new Date('2026-09-09T12:00:00Z') }); +afterEach(() => { jest.clearAllTimers(); __setOctokitForTest(null); }); + +/** + * Drain the 2.5s inter-page and 5s retry sleeps. + * + * Deliberately does NOT stop at the first moment `getTimerCount()` hits zero: + * between two awaited sleeps there is a tick with no timer pending, and + * breaking there leaves the promise waiting forever on a clock that has + * stopped advancing. + */ +async function drain(p: Promise): Promise { + let settled = false; + const tracked = p.then( + (v) => { settled = true; return v; }, + (e) => { settled = true; throw e; }, + ); + tracked.catch(() => {}); // don't trip unhandled-rejection while we pump + for (let i = 0; i < 200 && !settled; i++) { + jest.advanceTimersByTime(10_000); + for (let j = 0; j < 10; j++) await Promise.resolve(); + } + return tracked; +} + +const SINCE = new Date('2026-08-26T00:00:00Z'); +const HEALTHY_EMPTY_COMMITS = { total_count: 0, items: [], incomplete_results: false }; + +/** Octokit stub: healthy commit search, scripted issue search. */ +function stub(issueData: any) { + const calls = { commits: 0, issues: 0 }; + __setOctokitForTest({ + search: { + commits: async () => { calls.commits++; return { data: HEALTHY_EMPTY_COMMITS }; }, + issuesAndPullRequests: async () => { calls.issues++; return { data: issueData }; }, + }, + }); + return calls; +} + +describe('an empty merged-PR search that timed out', () => { + const TIMED_OUT_EMPTY = { total_count: 0, items: [], incomplete_results: true }; + + it('does NOT skip the member — it keeps the zero and records the doubt', async () => { + stub(TIMED_OUT_EMPTY); + const activity = await drain(fetchUserActivity('Smartling', 'sl-chromatic-bot', SINCE)); + expect(activity.prs).toEqual([]); + expect(activity.prsUnverified).toMatch(/timed out on an empty result/); + }); + + it('retries before giving up on it', async () => { + const calls = stub(TIMED_OUT_EMPTY); + await drain(fetchUserActivity('Smartling', 'keser', SINCE)); + // Exactly 3 attempts on the merged-PR page: 1 + SEARCH_TIMEOUT_RETRIES. + expect(calls.issues).toBe(3); + }); + + it('takes the good answer when a retry recovers', async () => { + let n = 0; + __setOctokitForTest({ + search: { + commits: async () => ({ data: HEALTHY_EMPTY_COMMITS }), + issuesAndPullRequests: async () => { + n++; + if (n === 1) return { data: TIMED_OUT_EMPTY }; + return { + data: { + total_count: 1, incomplete_results: false, + items: [{ + number: 7, title: 'fix', repository_url: 'https://api.github.com/repos/Smartling/pinch', + pull_request: { merged_at: '2026-09-01T00:00:00Z' }, + }], + }, + }; + }, + }, + }); + const activity = await drain(fetchUserActivity('Smartling', 'devx', SINCE)); + expect(activity.prs).toHaveLength(1); + expect(activity.prsUnverified).toBeUndefined(); + }); +}); + +describe('a self-contradicting merged-PR page still raises', () => { + it('raises when GitHub counts PRs but delivers none', async () => { + // The protection worth keeping: this is the shape that would land a + // developer with 42 merged PRs in the report as 0. Observed in the same + // run for ksoloviov-smartling (total_count=3) and kbroadrick (2). + stub({ total_count: 3, items: [], incomplete_results: true }); + await expect(drain(fetchUserActivity('Smartling', 'ksoloviov-smartling', SINCE))) + .rejects.toThrow(/no trustworthy result for @ksoloviov-smartling: counted 3 PRs but delivered none/); + }); +}); + +describe('a healthy empty merged-PR search', () => { + it('is trusted with no retry and no doubt recorded', async () => { + const calls = stub({ total_count: 0, items: [], incomplete_results: false }); + const activity = await drain(fetchUserActivity('Smartling', 'flaksie', SINCE)); + expect(activity.prs).toEqual([]); + expect(activity.prsUnverified).toBeUndefined(); + // One call only: fetchUserActivity does commits + merged PRs; + // countReviewedPRs is invoked separately by report-runner. + expect(calls.issues).toBe(1); + }); +}); + +/** + * The abort banner classifies a skip as a search brownout by pattern-matching + * the skip reason. Nothing coupled the thrown messages to that matcher, so a + * reworded raise silently sent the on-call to rotate the PAT during a GitHub + * outage — which is the exact lead #70 added the attribution to prevent. + */ +describe('thrown search messages stay matched to the brownout attribution', () => { + const attributable = (reason: string) => { + const snap = { + expectedCount: 100, + thresholds: DEFAULT_THRESHOLDS, + skipped: Array.from({ length: 6 }, (_, i) => ({ + login: `u${i}`, reason, classification: 'unknown' as const, + })), + }; + return formatIntegrityAbortReason(snap); + }; + + it('attributes the contradictory merged-PR raise to a search brownout', () => { + const msg = attributable( + 'GitHub merged-PR search gave no trustworthy result for @u: counted 3 PRs but delivered none (page 1)', + ); + expect(msg).toMatch(/GitHub search timeouts/); + expect(msg).not.toMatch(/auth\/permission/); + }); + + it('attributes the mid-pagination merged-PR raise too', () => { + const msg = attributable( + 'GitHub merged-PR search gave no trustworthy result for @u: under-delivered 100 of 250 (page 2)', + ); + expect(msg).toMatch(/GitHub search timeouts/); + }); + + it('attributes the commit-search raises', () => { + expect(attributable('GitHub commit search gave no trustworthy result for @u (…) after 3 attempts')) + .toMatch(/GitHub search timeouts/); + expect(attributable('GitHub commit search under-delivered for @u (…): got 0 of 21')) + .toMatch(/GitHub search timeouts/); + }); + + it('still blames auth for a genuine permission failure', () => { + expect(attributable('Validation Failed: the listed users cannot be searched')) + .toMatch(/auth\/permission/); + }); +}); diff --git a/src/lib/__tests__/unit/search-timeout-recovery.test.ts b/src/lib/__tests__/unit/search-timeout-recovery.test.ts index 3c27089..6298801 100644 --- a/src/lib/__tests__/unit/search-timeout-recovery.test.ts +++ b/src/lib/__tests__/unit/search-timeout-recovery.test.ts @@ -22,13 +22,24 @@ jest.useFakeTimers({ doNotFake: ['nextTick'], now: new Date('2026-09-09T00:00:00 afterEach(() => { jest.clearAllTimers(); __setOctokitForTest(null); }); /** Drain the 2.5s inter-page and 5s retry sleeps. */ +/** + * Drain the 2.5s inter-page and 5s retry sleeps. Does NOT stop at the first + * moment `getTimerCount()` hits zero — between two awaited sleeps there is a + * tick with no timer pending, and breaking there strands the promise on a + * clock that has stopped advancing. + */ async function drain(p: Promise): Promise { - for (let i = 0; i < 40; i++) { - for (let j = 0; j < 5; j++) await Promise.resolve(); - if (jest.getTimerCount() === 0) break; + let settled = false; + const tracked = p.then( + (v) => { settled = true; return v; }, + (e) => { settled = true; throw e; }, + ); + tracked.catch(() => {}); + for (let i = 0; i < 200 && !settled; i++) { jest.advanceTimersByTime(10_000); + for (let j = 0; j < 10; j++) await Promise.resolve(); } - return p; + return tracked; } const TIMED_OUT_EMPTY = { total_count: 0, items: [], incomplete_results: true }; diff --git a/src/lib/github-mock.ts b/src/lib/github-mock.ts index ff9f692..0f09db6 100644 --- a/src/lib/github-mock.ts +++ b/src/lib/github-mock.ts @@ -69,7 +69,7 @@ export function createMockGitHubProvider(): GitHubProvider { }, async countReviewedPRs() { - return Math.floor(Math.random() * 15); + return { reviews: Math.floor(Math.random() * 15) }; }, async fetchOpenPRs(_org, user, _since, log) { diff --git a/src/lib/github.ts b/src/lib/github.ts index 5574366..8b13df9 100644 --- a/src/lib/github.ts +++ b/src/lib/github.ts @@ -80,13 +80,19 @@ export interface UserActivity { prs: PRInfo[]; /** Set when the commit search delivered fewer commits than it counted. */ commitsShortfall?: SearchShortfall; + /** + * Set when the merged-PR count could not be verified — the search timed out + * on an empty result. The zero is kept (it is usually correct) and the + * uncertainty is recorded, rather than skipping the member outright. + */ + prsUnverified?: string; } export interface GitHubProvider { listOrgMembers(org: string, log?: (msg: string) => void): Promise; fetchUserActivity(org: string, user: string, since: Date, log?: (msg: string) => void): Promise; listOrgs(): Promise>; - countReviewedPRs(org: string, user: string, since: Date, log?: (msg: string) => void): Promise; + countReviewedPRs(org: string, user: string, since: Date, log?: (msg: string) => void): Promise<{ reviews: number; unverified?: string }>; fetchOpenPRs(org: string, user: string, since: Date, log?: (msg: string) => void): Promise; isCommitInDefaultBranch(owner: string, repo: string, sha: string): Promise; fetchRepoEvents(owner: string, repo: string, log?: (msg: string) => void): Promise; @@ -517,6 +523,49 @@ function toRawHit(item: any, user: string): RawCommitHit { }; } +/** + * Retry a search page while its result cannot be trusted, and report whether it + * ever became trustworthy. The caller decides what an untrustworthy result + * means, because that differs sharply by endpoint: + * + * - Commit search: an empty timed-out page really can hide commits (the + * 2026-09-08 incident had total_count=0 while GitHub held 43), so the caller + * narrows the window and ultimately raises. + * - Issue/PR search: an EMPTY result set routinely reports + * incomplete_results=true while being perfectly correct — verified against + * GitHub for bots and non-engineers with genuinely zero merged PRs. Raising + * on that produced 19 false skips in one run and aborted the report, so the + * caller only raises on a self-contradicting page. + */ +async function searchPageWithRetry( + label: string, + user: string, + page: number, + call: () => Promise<{ data: T }>, + log?: (msg: string) => void, +): Promise<{ data: T; trustworthy: boolean }> { + // Seeded on the first pass rather than cast from null at the end: the loop + // bound makes the null impossible today, and would make it an undefined the + // moment SEARCH_TIMEOUT_RETRIES changed. + let last: T | undefined; + for (let attempt = 0; attempt <= SEARCH_TIMEOUT_RETRIES; attempt++) { + const res = await call(); + logSearchAccounting(label, user, page, res.data, log); + last = res.data; + if (!isSuspectSearchResult(res.data)) return { data: res.data, trustworthy: true }; + if (attempt < SEARCH_TIMEOUT_RETRIES) { + log?.( + `[search] ${label} @${user} page=${page} untrustworthy ` + + `(attempt ${attempt + 1}/${SEARCH_TIMEOUT_RETRIES + 1}); retrying`, + ); + await sleep(SEARCH_TIMEOUT_RETRY_MS); + } + } + /* istanbul ignore next -- unreachable while SEARCH_TIMEOUT_RETRIES >= 0 */ + if (last === undefined) throw new Error(`[search] ${label} @${user}: no response captured`); + return { data: last, trustworthy: false }; +} + /** * Collect a user's commits for one date window. * @@ -549,28 +598,18 @@ async function collectCommits( while (true) { await sleep(2500); - let data: SearchAccounting | null = null; - for (let attempt = 0; attempt <= SEARCH_TIMEOUT_RETRIES; attempt++) { - const res = await withRetry( + const { data, trustworthy } = await searchPageWithRetry( + 'commits', user, page, + () => withRetry( () => getOctokit().search.commits({ q: query, sort: 'committer-date', order: 'desc', per_page: SEARCH_PER_PAGE, page, }), log, - ); - logSearchAccounting('commits', user, page, res.data, log); - - if (!isSuspectSearchResult(res.data)) { data = res.data; break; } - - if (attempt < SEARCH_TIMEOUT_RETRIES) { - log?.( - `[search] commits @${user} page=${page} untrustworthy ` + - `(attempt ${attempt + 1}/${SEARCH_TIMEOUT_RETRIES + 1}); retrying`, - ); - await sleep(SEARCH_TIMEOUT_RETRY_MS); - } - } + ), + log, + ); - if (!data) { + if (!trustworthy) { const halves = splitDateWindow(from, to); if (halves && depth < MAX_WINDOW_SPLIT_DEPTH) { log?.(`[search] commits @${user} ${clause}: narrowing window after repeated timeouts`); @@ -599,7 +638,7 @@ async function collectCommits( // `items` is optional on the type, and the trust predicate deliberately // tolerates its absence — so it must not be iterated raw here, or a shape // the tests bless becomes a TypeError and a hard SKIP. - const items = data.items ?? []; + const items = (data.items ?? []) as any[]; const pageTotal = data.total_count ?? 0; expectedTotal = expectedTotal === null ? pageTotal : Math.max(expectedTotal, pageTotal); @@ -672,33 +711,64 @@ async function searchUserMergedPRs( user: string, since: Date, log?: (msg: string) => void, -): Promise { +): Promise<{ prs: PRInfo[]; unverified?: string }> { const sinceStr = since.toISOString().split('T')[0]; const query = `org:${org} type:pr is:merged author:${user} merged:>=${sinceStr}`; const prs: PRInfo[] = []; let page = 1; + let expectedTotal: number | null = null; while (true) { await sleep(2500); - const res = await withRetry( - () => getOctokit().search.issuesAndPullRequests({ - q: query, sort: 'updated', order: 'desc', per_page: 100, page, - }), + const { data, trustworthy } = await searchPageWithRetry( + 'merged-prs', user, page, + () => withRetry( + () => getOctokit().search.issuesAndPullRequests({ + q: query, sort: 'updated', order: 'desc', per_page: 100, page, + }), + log, + ), log, ); - logSearchAccounting('merged-prs', user, page, res.data, log); - // The incident developer had 43 commits AND 42 merged PRs. These are two - // independent searches, so the merged-PR half can time out while commits - // succeed — landing the developer in the report with totalPRs = 0 and a - // wrong prPercentage (an impact-score input), silently. Raise instead. - if (isSuspectSearchResult(res.data)) { - throw new Error( - `GitHub merged-PR search gave no trustworthy result for @${user} (page ${page})`, - ); + + // Monotonic, like collectCommits: page 1's promise must survive a later + // page reporting a lower (or zero) total, or a page-2 timeout silently + // rewrites how many PRs we were owed. + expectedTotal = expectedTotal === null + ? (data.total_count ?? 0) + : Math.max(expectedTotal, data.total_count ?? 0); + + if (!trustworthy) { + // A self-contradicting page is wrong no matter how it is read. + if (isContradictoryPage(data)) { + throw new Error( + `GitHub merged-PR search gave no trustworthy result for @${user}: ` + + `counted ${data.total_count} PRs but delivered none (page ${page})`, + ); + } + // Mid-pagination timeout. The leniency below is a PAGE-1 observation — + // by now page 1 has already told us how many PRs exist, so returning + // what we have would be the GLOOK-50 undercount moved to page N. + if (prs.length > 0 || expectedTotal > 0) { + throw new Error( + `GitHub merged-PR search gave no trustworthy result for @${user}: ` + + `under-delivered ${prs.length} of ${expectedTotal} (page ${page})`, + ); + } + // Nothing collected and nothing promised: an EMPTY issue search routinely + // sets incomplete_results while being correct — verified against GitHub + // for bots and non-engineers with genuinely zero merged PRs. Treating it + // as a failure produced 19 false skips in one run and aborted the report + // at 21%. Keep the zero, record the doubt. + const unverified = + `merged-PR search timed out on an empty result (page ${page}); kept 0 as unverified`; + log?.(`[search] merged-prs @${user} UNVERIFIED ${unverified}`); + return { prs, unverified }; } - if (page === 1 && res.data.total_count === 0) return []; - for (const item of res.data.items) { + if (page === 1 && data.total_count === 0) return { prs: [] }; + + for (const item of (data.items ?? []) as any[]) { const repoFullName = item.repository_url.split('/repos/')[1] || ''; const repo = repoFullName.split('/')[1] || ''; prs.push({ @@ -710,11 +780,20 @@ async function searchUserMergedPRs( } // Same 1000-result ceiling as the commit search: paging past it returns // "Only the first 1000 search results are available". - if (prs.length >= Math.min(res.data.total_count, SEARCH_RESULT_CAP)) break; - if (res.data.items.length < SEARCH_PER_PAGE) break; + if (prs.length >= Math.min(expectedTotal ?? 0, SEARCH_RESULT_CAP)) break; + if ((data.items ?? []).length < SEARCH_PER_PAGE) break; page++; } - return prs; + + // Reconcile, as the commit path does: a short page must not quietly end the + // walk below what page 1 promised. + const owed = Math.min(expectedTotal ?? 0, SEARCH_RESULT_CAP); + if (prs.length < owed) { + throw new Error( + `GitHub merged-PR search under-delivered for @${user}: got ${prs.length} of ${owed}`, + ); + } + return { prs }; } // ---------- Commit detail (diff) ---------- @@ -752,7 +831,8 @@ export async function fetchUserActivity( // 1. Get all commits and merged PRs in parallel-ish (with rate limit gaps) const commitResult = await searchUserCommits(org, user, since, log); const rawCommits = commitResult.hits; - const prs = await searchUserMergedPRs(org, user, since, log); + const prResult = await searchUserMergedPRs(org, user, since, log); + const prs = prResult.prs; // 2. Build PR lookup: repo#number → PRInfo // Also parse PR refs from commit messages: "(#123)" @@ -895,7 +975,12 @@ export async function fetchUserActivity( } } - return { commits, prs, commitsShortfall: commitResult.shortfall }; + return { + commits, + prs, + commitsShortfall: commitResult.shortfall, + prsUnverified: prResult.unverified, + }; } // ---------- Org listing ---------- @@ -916,12 +1001,16 @@ async function countReviewedPRs( user: string, since: Date, log?: (msg: string) => void, -): Promise { +): Promise<{ reviews: number; unverified?: string }> { const sinceStr = since.toISOString().split('T')[0]; const q = `org:${org} is:pr is:merged reviewed-by:${user} merged:>${sinceStr}`; await sleep(2500); - const res = await withRetry( - () => getOctokit().search.issuesAndPullRequests({ q, per_page: 1 }), + const { data, trustworthy } = await searchPageWithRetry( + 'reviewed-prs', user, 1, + () => withRetry( + () => getOctokit().search.issuesAndPullRequests({ q, per_page: 1 }), + log, + ), log, ); // per_page:1 means items is capped at 1 while total_count is the real answer, @@ -929,14 +1018,18 @@ async function countReviewedPRs( // timeout arm still can, and a timed-out review count reads 0 — which halves // a scoring factor (weight 0.5, min(reviews/15, 1)) and moves the developer // down the ranking. - logSearchAccounting('reviewed-prs', user, 1, res.data, log); - if (isSuspectSearchResult(res.data)) { - // Safe to raise: report-runner catches this into a non-fatal - // integrity.recordError, so it lands in run_metadata.errors and cannot - // trip the abort gate. - throw new Error(`GitHub review-count search returned an untrustworthy result for @${user}`); + if (!trustworthy) { + // per_page:1 caps items at 1, so isContradictoryPage can effectively never + // fire here — which means without a doubt channel this endpoint would trust + // everything. A timed-out review count reads 0 and halves a scoring factor + // (weight 0.5, min(reviews/15, 1)), moving the developer down the ranking. + // Keep the number, hand the caller the doubt. + const unverified = + `review-count search timed out (total_count=${data.total_count ?? 0}); kept as unverified`; + log?.(`[search] reviewed-prs @${user} UNVERIFIED ${unverified}`); + return { reviews: data.total_count ?? 0, unverified }; } - return res.data.total_count; + return { reviews: data.total_count ?? 0 }; } // ---------- Open PR search ---------- diff --git a/src/lib/report-runner.ts b/src/lib/report-runner.ts index 1210b71..634111f 100644 --- a/src/lib/report-runner.ts +++ b/src/lib/report-runner.ts @@ -198,6 +198,20 @@ export async function runReport( // nothing at all — and record it as a member-kept partial-data // condition so it appears in run_metadata.errors without counting // toward the abort gate the way a SKIP would. + // GLOOK-50: the merged-PR count could not be verified because the + // search timed out on an empty result. Kept, not skipped — verified + // against GitHub that an empty issue search routinely reports + // incomplete_results while being correct. + if (activity.prsUnverified) { + log(`@${member.login}: UNVERIFIED PR count — ${activity.prsUnverified}`); + integrity.recordUnverified({ + login: member.login, + field: 'merged-prs', + kept: activity.prs.length, + reason: activity.prsUnverified, + }); + } + if (activity.commitsShortfall) { const sf = activity.commitsShortfall; log(`@${member.login}: PARTIAL commit data — ${sf.detail}`); @@ -212,9 +226,19 @@ export async function runReport( // Fetch PR review count (overlaps with LLM work from previous members) try { - const reviews = await github.countReviewedPRs(org, member.login, since, log); + const reviewResult = await github.countReviewedPRs(org, member.login, since, log); + const reviews = reviewResult.reviews; reviewCounts.set(member.login, reviews); if (reviews > 0) log(`@${member.login}: ${reviews} PRs reviewed`); + if (reviewResult.unverified) { + log(`@${member.login}: UNVERIFIED review count — ${reviewResult.unverified}`); + integrity.recordUnverified({ + login: member.login, + field: 'reviews', + kept: reviews, + reason: reviewResult.unverified, + }); + } } catch (err) { reviewCounts.set(member.login, 0); const message = err instanceof Error ? err.message : String(err); @@ -477,6 +501,7 @@ export async function runReport( state: 'failed', skipped: integritySnapshot.skipped, errors: integritySnapshot.errors, + unverified: integritySnapshot.unverified, expectedCount: integritySnapshot.expectedCount, thresholds: integritySnapshot.thresholds, abortReason, @@ -688,6 +713,7 @@ export async function runReport( state: integrityState, skipped: integritySnapshot.skipped, errors: integritySnapshot.errors, + unverified: integritySnapshot.unverified, expectedCount: integritySnapshot.expectedCount, thresholds: integritySnapshot.thresholds, }; diff --git a/src/lib/report-runner/integrity-tracker.ts b/src/lib/report-runner/integrity-tracker.ts index 218c76e..04cd43f 100644 --- a/src/lib/report-runner/integrity-tracker.ts +++ b/src/lib/report-runner/integrity-tracker.ts @@ -11,6 +11,7 @@ import type { RunMetadata, SkipClassification, SkippedMember, + UnverifiedMember, } from './types'; const MAX_MESSAGE_LENGTH = 500; @@ -29,6 +30,8 @@ export class IntegrityTracker { private readonly skipsByLogin = new Map(); /** Errors appended in order; not deduped (could be many per member/sha). */ private readonly errors: IntegrityError[] = []; + /** Members kept with an unverified figure, keyed login+field so retries collapse. */ + private readonly unverifiedByKey = new Map(); readonly expectedCount: number; readonly thresholds: IntegrityThresholds; @@ -52,11 +55,21 @@ export class IntegrityTracker { }); } + /** + * A member kept in the report whose figure could not be verified (GLOOK-50). + * Not a skip and not an error: countable on its own so a correlated brownout + * can downgrade the run without aborting it. + */ + recordUnverified(u: UnverifiedMember): void { + this.unverifiedByKey.set(`${u.login}:${u.field}`, { ...u, reason: truncate(u.reason) }); + } + /** Frozen snapshot for evaluator + persistence. Independent of tracker state. */ - snapshot(): Pick { + snapshot(): Pick { return Object.freeze({ skipped: Object.freeze([...this.skipsByLogin.values()]) as SkippedMember[], errors: Object.freeze([...this.errors]) as IntegrityError[], + unverified: Object.freeze([...this.unverifiedByKey.values()]) as UnverifiedMember[], expectedCount: this.expectedCount, thresholds: this.thresholds, }); diff --git a/src/lib/report-runner/skip-classifier.ts b/src/lib/report-runner/skip-classifier.ts index 15e3658..c9c483d 100644 --- a/src/lib/report-runner/skip-classifier.ts +++ b/src/lib/report-runner/skip-classifier.ts @@ -5,7 +5,7 @@ import db from '@/lib/db'; import type { IntegrityState, RunMetadata, SkipClassification } from './types'; -import { integrityCounts } from './types'; +import { integrityCounts, unverifiedCounts } from './types'; export const AUTO_FLAG_RECENT_RUNS = 5; export const AUTO_FLAG_THRESHOLD = 4; @@ -108,12 +108,22 @@ export async function loadSkipClassifier(): Promise<(login: string) => SkipClass * killed by a handful of skips and a large one isn't killed by a rounding error. */ export function evaluateIntegrity( - snapshot: Pick, + snapshot: Pick, ): IntegrityState { const T = snapshot.thresholds; const { countable, countablePct } = integrityCounts(snapshot); if (countable >= T.abortUnknownCount && countablePct >= T.abortUnknownPct) return 'failed'; if (countable >= T.degradedUnknownCount || countablePct >= T.degradedUnknownPct) return 'degraded'; + + // GLOOK-50: members kept with an unverified figure never abort a run — they + // are present in the report — but a correlated brownout that leaves a large + // share of the org unverified must not read as a clean bill of health. Before + // this, an org-wide issue-search brownout produced zero skips, state 'ok', + // no badge, and every developer showing 0 merged PRs with their impact score + // silently reshuffled. + const { pct: unverifiedPct } = unverifiedCounts(snapshot); + if (unverifiedPct >= T.degradedUnverifiedPct) return 'degraded'; + return 'ok'; } diff --git a/src/lib/report-runner/types.ts b/src/lib/report-runner/types.ts index 948c7c1..1569059 100644 --- a/src/lib/report-runner/types.ts +++ b/src/lib/report-runner/types.ts @@ -31,14 +31,41 @@ export interface IntegrityThresholds { abortUnknownPct: 0.10; degradedUnknownCount: 3; degradedUnknownPct: 0.05; + /** + * Members whose data was KEPT but could not be verified (GLOOK-50). Its own + * gate, deliberately separate from the skip thresholds: an unverified member + * still has a row in the report, so this must never abort a run — but a + * correlated brownout that leaves a third of the org unverified has to stop + * reading as a clean bill of health. + */ + degradedUnverifiedPct: 0.15; } export type IntegrityState = 'ok' | 'degraded' | 'failed'; +/** + * A member kept in the report whose data could not be verified. + * + * Deliberately NOT an IntegrityError: `errors` already mixes per-commit + * `sha-merge-check` and `unmerged-commit-detail` entries that run to hundreds + * on a healthy run, so anything thresholded on `errors.length` would cry wolf + * and get switched off. This is member-scoped, deduped, and countable. + */ +export interface UnverifiedMember { + login: string; + /** Which figure is unverified — lets the badge and an operator filter. */ + field: 'merged-prs' | 'reviews' | 'commits'; + /** What was kept in its place (usually 0). */ + kept: number; + reason: string; +} + export interface RunMetadata { state: IntegrityState; skipped: SkippedMember[]; errors: IntegrityError[]; + /** Members kept in the report with an unverified figure (GLOOK-50). */ + unverified?: UnverifiedMember[]; /** Org member count at run start — denominator for percentage calculations */ expectedCount: number; thresholds: IntegrityThresholds; @@ -52,6 +79,7 @@ export const DEFAULT_THRESHOLDS: IntegrityThresholds = { abortUnknownPct: 0.10, degradedUnknownCount: 3, degradedUnknownPct: 0.05, + degradedUnverifiedPct: 0.15, }; /** @@ -98,6 +126,23 @@ export function integrityCounts( return { countable, allowlisted, effectiveExpected, countablePct }; } +/** + * How many members were kept but unverified, as a share of those expected. + * + * Separate from `integrityCounts` because an unverified member is NOT a skip: + * they are present in the report with a figure we could not confirm. The only + * thing this may do is downgrade a run to `degraded` — never abort it. + */ +export function unverifiedCounts( + snapshot: Pick, +): { count: number; pct: number } { + const logins = new Set((snapshot.unverified ?? []).map((u) => u.login)); + const count = logins.size; + const allowlisted = snapshot.skipped.filter((s) => s.classification === 'expected').length; + const expected = Math.max((snapshot.expectedCount ?? 0) - allowlisted, 0); + return { count, pct: expected > 0 ? count / expected : 0 }; +} + /** * The operator-facing abort summary, persisted to `reports.error` and * `run_metadata.abortReason` and rendered verbatim by IntegrityBadge. diff --git a/src/lib/report/org.ts b/src/lib/report/org.ts index 8166a52..b1d4e56 100644 --- a/src/lib/report/org.ts +++ b/src/lib/report/org.ts @@ -20,8 +20,11 @@ export interface OrgModelUsage { export async function getOrgReport(reportId: string) { // 1. Report metadata const [reportRows] = await db.execute( + // run_metadata is selected so IntegrityBadge can render on the org page. + // Without it the badge receives null here, which closed the last surface + // an unverified/degraded signal could reach (GLOOK-50). `SELECT id, org, period_days, status, created_at, completed_at, - cc_period_start, cc_period_end + cc_period_start, cc_period_end, run_metadata FROM reports WHERE id = ?`, [reportId], ) as [any[], any];