GLOOK-48: reports silently dropped developers — secondary rate limit + integrity guard regression

CLAUDE.md

@@ -47,13 +47,14 @@ Glooker is a Next.js 15 web app that generates developer impact reports for a Gi
 - **Never pin a charset on a MySQL table** (`DEFAULT CHARSET=...`). `reports.id` and every other table inherit the database default, and MySQL refuses a foreign key whose string column differs in charset/collation from its referent (`ER_FK_INCOMPATIBLE_COLUMNS`, errno 3780). Dev's DB is utf8mb3 while a stock local MySQL 8/9 defaults to utf8mb4, so a pinned `utf8mb4` passes locally and fails only in dev. `initSchema()` **catches and logs** DDL failures instead of throwing, so the table just silently doesn't exist until a query hits it — the 2026-08-11 org-report outage. Guarded by `mysql-schema-fk-charset.test.ts`. To verify MySQL schema changes locally: `CREATE DATABASE x CHARACTER SET utf8mb3`, apply `sed '1,2d' schema.sql` (its first two lines are `CREATE DATABASE`/`USE glooker` and will otherwise hit your real local DB), then run the app against it.
 - Base tables (`reports`, `developer_stats`, `commit_analyses`) come from `schema.sql` on MySQL — `db/mysql.ts` only creates the newer tables and runs `ALTER` migrations. SQLite creates everything in `db/sqlite.ts`. A new table therefore needs adding in **both** places. `jira_projects` follows this rule too — it exists in `schema.sql`, `db/mysql.ts`, and `db/sqlite.ts`.
 - GitHub search API returns max 1000 results per query — per-user search avoids this
-- GitHub secondary rate limits trigger on rapid successive search calls — 2.5s sleeps between requests + exponential back-off retry on 403/429
+- **GitHub has two rate limits and they back off differently** (`github.ts`). The **primary** limit is quota-based: `/rate_limit` reports it, responses carry `x-ratelimit-reset`, and we wait until that reset (10s floor). The **secondary** (abuse-detection) limit appears in neither — it triggers on rapid successive search calls, and during the 2026-09-02 incident the search quota read 30/30 while calls were being rejected. Secondary limits therefore **ignore `x-ratelimit-reset`** and wait 60s doubling to a 300s cap; deriving the wait from the primary window collapsed it to the 10s floor, which re-tripped the limit and dropped developers from the report (GLOOK-48). `retry-after` is honoured on both, and still escalates on secondary. Detection is structural first (`x-ratelimit-remaining` non-zero on a rate-limited response cannot be primary) and textual second, so it does not hinge on GitHub's wording. A 403 that is **not** a rate limit — SSO/SAML enforcement, missing scope, `Resource not accessible by integration` — propagates immediately instead of burning the retry budget. Still 2.5s sleeps between requests.
 - Some LLM providers wrap JSON in markdown fences despite `response_format: json_object` — the parser strips ` ```json ``` ` fences
 - Smartling auth token expires in ~24h — `smartling-auth.ts` caches and auto-refreshes 5 min before expiry
 - `next build` artifacts conflict with `next dev` — always `rm -rf .next` when switching
 - 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 `jest.mock('@octokit/rest')` before the import
+- `@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.
+- **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`)
 - On `pull_request`, CI tests the **merge of the PR head into main**, not the branch tip — so a suite that is green locally can fail in CI purely because main moved. When a CI failure won't reproduce, check the suite/test counts first: a mismatch (e.g. local 102 suites/922 tests vs CI 103/925) means you are running a different tree. Reproduce with `git merge origin/main` on a throwaway branch, and mimic the runner's worker count (`--maxWorkers=3`, ubuntu-latest has 4 vCPU).

docs/superpowers/plans/2026-06-01-glook-13-report-integrity.md

@@ -605,6 +605,12 @@ export async function loadSkipClassifier(): Promise<(login: string) => SkipClass
 
 /**
  * Pure evaluator — given a tracker snapshot, returns the integrity state.
+ * SUPERSEDED 2026-09-02 (GLOOK-48): counting only 'unknown' here is the
+ * regression that let reports slide 65 -> 51 with integrity green, because
+ * loadSkipClassifier() auto-flags chronic failures out of the numerator.
+ * 'auto-flagged' now counts too — see countableSkips()/integrityCounts() in
+ * report-runner/types.ts. The text below records the original design.
+ *
  * Only unknown SKIPs count toward thresholds; expected + auto-flagged are
  * surfaced in run_metadata but ignored here.
  */
@@ -645,6 +651,8 @@ git commit -m "feat(report-runner): skip classifier + threshold evaluator (GLOOK
 loadSkipClassifier() runs 2 SELECTs at the top of each report and
 returns a hot closure (allowlist ⊃ auto-flagged ⊃ unknown). 
 evaluateIntegrity() is pure: counts only unknown SKIPs against the
+[SUPERSEDED by GLOOK-48 — auto-flagged counts as well, and allowlisted
+members are removed from the denominator; see report-runner/types.ts]
 abort (AND) and degraded (OR) thresholds defined in DEFAULT_THRESHOLDS."
 ```
 

src/components/IntegrityBadge.tsx

@@ -1,6 +1,7 @@
 'use client';
 import { useState } from 'react';
 import type { RunMetadata, SkippedMember, IntegrityError } from '@/lib/report-runner/types';
+import { countableSkips } from '@/lib/report-runner/types';
 
 export interface IntegrityBadgeProps {
   metadata: RunMetadata | null;
@@ -46,7 +47,10 @@ export default function IntegrityBadge({ metadata }: IntegrityBadgeProps) {
   }
 
   // degraded
-  const unknownCount = metadata.skipped.filter(s => s.classification === 'unknown').length;
+  // countableSkips, not a local 'unknown' filter: an auto-flagged member counts
+  // toward the thresholds, so suppressing it here made the pill under-report
+  // the very skips that triggered the warning.
+  const countedCount = countableSkips(metadata.skipped).length;
   const totalCount = metadata.skipped.length;
   return (
     <>
@@ -56,7 +60,7 @@ export default function IntegrityBadge({ metadata }: IntegrityBadgeProps) {
         className={`${PILL_BASE} bg-amber-500/15 text-amber-300 border border-amber-500/30 hover:bg-amber-500/25 transition-colors`}
         title="Click for details"
       >
-        ⚠ {totalCount} partial{unknownCount > 0 ? ` (${unknownCount} unknown)` : ''}
+        ⚠ {totalCount} partial{countedCount > 0 ? ` (${countedCount} unexplained)` : ''}
       </button>
       {open && (
         <div className="mt-3 bg-amber-500/5 border border-amber-500/20 rounded-lg p-3">

src/lib/__tests__/unit/github-retry.test.ts

@@ -79,4 +79,51 @@ describe('withRetry — transient-error coverage (GLOOK-13)', () => {
     expect(fn).toHaveBeenCalledTimes(2);
     expect(result).toBe('ok');
   });
+
+  // ---- GLOOK-48: the wiring, not just the pure function -------------------
+  // rateLimitWaitSeconds is unit-tested in github-secondary-rate-limit.test.ts.
+  // These two pin that withRetry actually acts on it, which is the behaviour
+  // the incident was about.
+
+  async function settle() {
+    for (let i = 0; i < 5; i++) await Promise.resolve();
+  }
+
+  it('waits at least 60s before retrying a secondary rate limit', async () => {
+    // x-ratelimit-reset only 5s out: this is the exact shape that used to
+    // collapse to the 10s floor and immediately re-trip the same limit.
+    const err = Object.assign(new Error('You have exceeded a secondary rate limit'), {
+      status: 403,
+      response: {
+        status: 403,
+        headers: { 'x-ratelimit-reset': String(Math.floor(Date.now() / 1000) + 5) },
+      },
+    });
+    const fn = jest.fn().mockRejectedValueOnce(err).mockResolvedValue('ok');
+
+    const p = withRetry(fn);
+    await settle();
+    expect(fn).toHaveBeenCalledTimes(1);
+
+    jest.advanceTimersByTime(59_999);
+    await settle();
+    expect(fn).toHaveBeenCalledTimes(1); // would have retried at 10s before the fix
+
+    jest.advanceTimersByTime(1);
+    await settle();
+    expect(fn).toHaveBeenCalledTimes(2);
+    await expect(p).resolves.toBe('ok');
+  });
+
+  it('does NOT retry a permission 403 (SSO / missing scope) — propagates immediately', async () => {
+    // Gating the primary path on x-ratelimit-remaining would otherwise make
+    // these sleep 60s five times over instead of failing fast.
+    const err = Object.assign(new Error('Resource not accessible by integration'), {
+      status: 403,
+      response: { status: 403, headers: { 'x-ratelimit-remaining': '4999' } },
+    });
+    const fn = jest.fn().mockRejectedValue(err);
+    await expect(withRetry(fn)).rejects.toBe(err);
+    expect(fn).toHaveBeenCalledTimes(1);
+  });
 });

src/lib/__tests__/unit/github-secondary-rate-limit.test.ts

@@ -0,0 +1,179 @@
+// @octokit/rest is ESM-only; github.ts imports it, so it must be mocked with a
+// factory before the import or the whole suite fails to load. A bare
+// jest.mock() is NOT enough — that still loads the real module to auto-mock it.
+jest.mock('@octokit/rest', () => ({ Octokit: jest.fn().mockImplementation(() => ({})) }));
+
+import { isRateLimitError, isSecondaryRateLimit, rateLimitWaitSeconds } from '@/lib/github';
+
+const NOW = 1_700_000_000;
+
+/** Build an Octokit-shaped error. `message` goes on `.message` like RequestError. */
+function err(
+  status: number,
+  headers: Record<string, unknown> = {},
+  message = 'boom',
+  dataMessage?: string,
+): unknown {
+  return Object.assign(new Error(message), {
+    status,
+    response: { status, headers, ...(dataMessage ? { data: { message: dataMessage } } : {}) },
+  });
+}
+
+const SECONDARY = 'You have exceeded a secondary rate limit. Please wait a few minutes.';
+const ABUSE = 'You have triggered an abuse detection mechanism. Please wait a few minutes.';
+const PRIMARY = "API rate limit exceeded for user ID 1234.";
+
+describe('isRateLimitError — separating rate limits from permission 403s', () => {
+  it('treats every 429 as a rate limit, headers or not', () => {
+    expect(isRateLimitError(err(429, {}, 'slow down'))).toBe(true);
+  });
+
+  it('treats a 403 as a rate limit when GitHub sends retry-after', () => {
+    expect(isRateLimitError(err(403, { 'retry-after': '60' }, 'no idea'))).toBe(true);
+  });
+
+  it('treats a 403 as a rate limit when the message names one', () => {
+    expect(isRateLimitError(err(403, {}, PRIMARY))).toBe(true);
+    expect(isRateLimitError(err(403, {}, SECONDARY))).toBe(true);
+    expect(isRateLimitError(err(403, {}, ABUSE))).toBe(true);
+  });
+
+  it('treats a 403 as a rate limit when the primary quota is exhausted', () => {
+    expect(isRateLimitError(err(403, { 'x-ratelimit-remaining': '0' }, 'forbidden'))).toBe(true);
+  });
+
+  // The reason 403 retryability had to change alongside the reset-header gate:
+  // without this, a deterministic permission failure would now sleep 60s five
+  // times over instead of the old 10s five times over.
+  it('does NOT treat a permission 403 as a rate limit', () => {
+    expect(isRateLimitError(err(403, { 'x-ratelimit-remaining': '4999' },
+      'Resource not accessible by integration'))).toBe(false);
+    expect(isRateLimitError(err(403, { 'x-ratelimit-remaining': '4999' },
+      'You must be a member of the organization'))).toBe(false);
+    expect(isRateLimitError(err(403, {}, 'SAML enforcement is enabled'))).toBe(false);
+  });
+
+  it('ignores non-403/429 statuses and junk', () => {
+    for (const e of [err(404), err(500), err(401), undefined, null, {}, 'nope']) {
+      expect(isRateLimitError(e)).toBe(false);
+    }
+  });
+});
+
+describe('isSecondaryRateLimit', () => {
+  it('detects the current wording on 403 and 429', () => {
+    expect(isSecondaryRateLimit(err(403, {}, SECONDARY))).toBe(true);
+    expect(isSecondaryRateLimit(err(429, {}, SECONDARY))).toBe(true);
+  });
+
+  // GitHub renamed this but still emits the old phrase on some endpoints.
+  it('detects the legacy "abuse detection mechanism" wording', () => {
+    expect(isSecondaryRateLimit(err(403, {}, ABUSE))).toBe(true);
+  });
+
+  // Octokit always sets .message, so a ?? chain would make this unreachable.
+  it('reads the phrase from response.data.message too', () => {
+    expect(isSecondaryRateLimit(err(403, { 'x-ratelimit-remaining': '0' }, 'Forbidden', SECONDARY)))
+      .toBe(true);
+  });
+
+  // The structural signal — this is the incident shape: quota healthy, calls rejected.
+  it('detects a secondary limit with NO wording when quota remains', () => {
+    expect(isSecondaryRateLimit(err(429, { 'x-ratelimit-remaining': '4999' }, 'Forbidden')))
+      .toBe(true);
+  });
+
+  it('does NOT call an exhausted primary quota secondary', () => {
+    expect(isSecondaryRateLimit(err(403, { 'x-ratelimit-remaining': '0' }, PRIMARY))).toBe(false);
+  });
+
+  it('is false for anything that is not a rate limit at all', () => {
+    expect(isSecondaryRateLimit(err(403, { 'x-ratelimit-remaining': '4999' },
+      'Resource not accessible by integration'))).toBe(false);
+    expect(isSecondaryRateLimit(err(404))).toBe(false);
+  });
+});
+
+describe('rateLimitWaitSeconds — secondary schedule', () => {
+  // Pinned exactly, not as bounds: `>= 60` and `w9 <= 300` would still pass if
+  // the base regressed to 30s, growth went linear, or the cap moved.
+  it('is exactly 60/120/240/300/300 across the retry budget', () => {
+    const e = err(403, {}, SECONDARY);
+    expect([0, 1, 2, 3, 4].map(a => rateLimitWaitSeconds(e, a, NOW))).toEqual([60, 120, 240, 300, 300]);
+  });
+
+  it('never exceeds the 300s cap however high the attempt', () => {
+    expect(rateLimitWaitSeconds(err(403, {}, SECONDARY), 20, NOW)).toBe(300);
+  });
+
+  it('ignores x-ratelimit-reset on a secondary limit — that is the primary window', () => {
+    const e = err(403, { 'x-ratelimit-reset': String(NOW + 8) }, SECONDARY);
+    expect(rateLimitWaitSeconds(e, 0, NOW)).toBe(60);
+  });
+});
+
+describe('rateLimitWaitSeconds — retry-after', () => {
+  it('honours a longer retry-after over the schedule', () => {
+    const e = err(403, { 'retry-after': '90' }, SECONDARY);
+    expect(rateLimitWaitSeconds(e, 0, NOW)).toBe(90);
+  });
+
+  // The flat-wait bug: retry-after used to return early, so a secondary limit
+  // waited the same 60s on all 5 attempts and re-tripped at the boundary.
+  it('keeps escalating on a secondary limit when retry-after is short', () => {
+    const e = err(403, { 'retry-after': '60' }, SECONDARY);
+    expect([0, 1, 2, 3, 4].map(a => rateLimitWaitSeconds(e, a, NOW))).toEqual([60, 120, 240, 300, 300]);
+  });
+
+  it('honours retry-after exactly on a primary limit (no escalation)', () => {
+    const e = err(403, { 'retry-after': '7', 'x-ratelimit-remaining': '0' }, PRIMARY);
+    expect([0, 1, 2].map(a => rateLimitWaitSeconds(e, a, NOW))).toEqual([7, 7, 7]);
+  });
+
+  it('treats retry-after: 0 as zero, not as 60', () => {
+    const e = err(403, { 'retry-after': '0', 'x-ratelimit-remaining': '0' }, PRIMARY);
+    expect(rateLimitWaitSeconds(e, 0, NOW)).toBe(0);
+  });
+
+  it('falls through to the schedule on an unparseable retry-after', () => {
+    // Previously `Number('later') || 60` silently produced 60 on the primary
+    // path, from a constant named for the secondary one.
+    const e = err(403, { 'retry-after': 'later', 'x-ratelimit-reset': String(NOW + 240),
+      'x-ratelimit-remaining': '0' }, PRIMARY);
+    expect(rateLimitWaitSeconds(e, 0, NOW)).toBe(240);
+  });
+
+  it('accepts an RFC 7231 HTTP-date retry-after', () => {
+    const spy = jest.spyOn(Date, 'now').mockReturnValue(NOW * 1000);
+    try {
+      const e = err(403, { 'retry-after': new Date((NOW + 120) * 1000).toUTCString(),
+        'x-ratelimit-remaining': '0' }, PRIMARY);
+      expect(rateLimitWaitSeconds(e, 0, NOW)).toBe(120);
+    } finally {
+      spy.mockRestore();
+    }
+  });
+});
+
+describe('rateLimitWaitSeconds — primary limit behaviour preserved', () => {
+  it('waits until x-ratelimit-reset', () => {
+    const e = err(403, { 'x-ratelimit-reset': String(NOW + 240), 'x-ratelimit-remaining': '0' }, PRIMARY);
+    expect(rateLimitWaitSeconds(e, 0, NOW)).toBe(240);
+  });
+
+  it('never waits less than the 10s floor when reset is in the past', () => {
+    const e = err(403, { 'x-ratelimit-reset': String(NOW - 500), 'x-ratelimit-remaining': '0' }, PRIMARY);
+    expect(rateLimitWaitSeconds(e, 0, NOW)).toBe(10);
+  });
+
+  it('falls back to 30s doubling with no usable headers', () => {
+    const e = err(429, {}, 'slow down');
+    expect([0, 1, 2].map(a => rateLimitWaitSeconds(e, a, NOW))).toEqual([30, 60, 120]);
+  });
+
+  it('falls back to the schedule on a non-numeric reset header', () => {
+    const e = err(429, { 'x-ratelimit-reset': 'soon' }, 'slow down');
+    expect(rateLimitWaitSeconds(e, 1, NOW)).toBe(60);
+  });
+});