GLOOK-50: stop the merged-PR search from manufacturing false skips

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`)

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 (
+      <button
+        type="button"
+        onClick={() => setOpen(o => !o)}
+        className={`${PILL_BASE} bg-sky-500/15 text-sky-300 border border-sky-500/30 hover:bg-sky-500/25 transition-colors`}
+        title={unverified.map(u => `@${u.login} ${u.field}: ${u.reason}`).join('\n')}
+      >
+        ⓘ {unverified.length} unverified
+      </button>
+    );
+  }
 
   const expectedCount = metadata.expectedCount ?? 0;
 

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);
+  });
+});

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<T>(p: Promise<T>): Promise<T> {
+  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/);
+  });
+});

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<T>(p: Promise<T>): Promise<T> {
-  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 };

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) {