diff --git a/CLAUDE.md b/CLAUDE.md index 295ce60c..05139c71 100644 --- a/CLAUDE.md +++ b/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). diff --git a/docs/superpowers/plans/2026-06-01-glook-13-report-integrity.md b/docs/superpowers/plans/2026-06-01-glook-13-report-integrity.md index bc1d2b76..cb8e8d72 100644 --- a/docs/superpowers/plans/2026-06-01-glook-13-report-integrity.md +++ b/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." ``` diff --git a/src/components/IntegrityBadge.tsx b/src/components/IntegrityBadge.tsx index c7bc52d5..e6595211 100644 --- a/src/components/IntegrityBadge.tsx +++ b/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)` : ''} {open && (
diff --git a/src/lib/__tests__/unit/github-retry.test.ts b/src/lib/__tests__/unit/github-retry.test.ts index f49e0109..059f289f 100644 --- a/src/lib/__tests__/unit/github-retry.test.ts +++ b/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); + }); }); diff --git a/src/lib/__tests__/unit/github-secondary-rate-limit.test.ts b/src/lib/__tests__/unit/github-secondary-rate-limit.test.ts new file mode 100644 index 00000000..2a70030c --- /dev/null +++ b/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 = {}, + 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); + }); +}); diff --git a/src/lib/__tests__/unit/integrity-guard-regression.test.ts b/src/lib/__tests__/unit/integrity-guard-regression.test.ts new file mode 100644 index 00000000..72a95dd8 --- /dev/null +++ b/src/lib/__tests__/unit/integrity-guard-regression.test.ts @@ -0,0 +1,162 @@ +/** + * GLOOK-13 regression (found 2026-09-02): reports silently under-collected for + * days — 65 -> 64 -> 59 -> 51 developers — while integrity state stayed 'ok'. + * + * Mechanism: evaluateIntegrity() counted only 'unknown' skips, and + * loadSkipClassifier() auto-flags any member skipped in >=4 of the last 5 runs. + * So a member failed "loudly" for ~4 runs, then converted to 'auto-flagged' and + * permanently left the health check's numerator. Each newly-failing member + * followed the same path, which is why the loss was progressive and never + * tripped a threshold. + * + * Verified against GitHub at the time: @junky had 28 commits in the window and + * @ningjiang118 had 4, both absent from the report entirely. Both appear in + * GLOOK-13's own May list of 41 skipped engineers, so both were auto-flagged. + * + * GLOOK-13's stated requirement is the spec these tests encode: + * "Today's 41/102 SKIPs (~40%) should have aborted the run, not silently + * shipped." + * + * Only a human-confirmed allowlist entry ('expected') may suppress the guard. + * 'auto-flagged' is a *suggestion* surfaced in Settings for a human to promote + * to the allowlist (see /api/settings/skip-allowlist autoFlaggedCandidates) — + * it must not silence the alarm on its own. + */ +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 type { SkipClassification } from '@/lib/report-runner/types'; + +function snapshotWith(expectedCount: number, skips: Array<[string, SkipClassification]>) { + const t = new IntegrityTracker({ expectedCount, thresholds: DEFAULT_THRESHOLDS }); + for (const [login, classification] of skips) t.recordSkip(login, 'boom', classification); + return t.snapshot(); +} + +const many = (n: number, c: SkipClassification, prefix = 'u'): Array<[string, SkipClassification]> => + Array.from({ length: n }, (_, i) => [`${prefix}${i}`, c]); + +describe('integrity guard counts auto-flagged skips (GLOOK-13 regression)', () => { + it('aborts on the GLOOK-13 incident shape even when every skip is auto-flagged', () => { + // The literal scenario GLOOK-13 was filed about: 41 of 102 dropped. + expect(evaluateIntegrity(snapshotWith(102, many(41, 'auto-flagged')))).toBe('failed'); + }); + + it('aborts on the 2026-09-02 shape: 13 chronic members lost from 101', () => { + // 13/101 = 12.9% >= 10% and >= 5 absolute, so both halves of the + // deliberate AND gate are satisfied. + expect(evaluateIntegrity(snapshotWith(101, many(13, 'auto-flagged')))).toBe('failed'); + }); + + it('still lets a human-allowlisted skip suppress the guard', () => { + // 'expected' means a person reviewed this member and accepted the skip. + expect(evaluateIntegrity(snapshotWith(101, many(41, 'expected')))).toBe('ok'); + }); + + it('counts unknown and auto-flagged together rather than separately', () => { + // 6 + 6 = 12 of 101 = 11.9%. Neither group alone reaches 10%. + const mixed = [...many(6, 'unknown', 'a'), ...many(6, 'auto-flagged', 'b')]; + expect(evaluateIntegrity(snapshotWith(101, mixed))).toBe('failed'); + }); + + it('excludes allowlisted members from the count but not the others', () => { + // 3 auto-flagged of 100 hits the degraded count threshold (3), and the + // 40 allowlisted skips must not push it to failed. + const mixed = [...many(40, 'expected', 'ok'), ...many(3, 'auto-flagged', 'bad')]; + expect(evaluateIntegrity(snapshotWith(100, mixed))).toBe('degraded'); + }); + + it('preserves the deliberate AND gate for abort', () => { + // These two cases are pinned by the original GLOOK-13 suite as intended: + // significant in absolute AND relative terms. Auto-flagged now counts, but + // the gate itself is unchanged. + expect(evaluateIntegrity(snapshotWith(100, many(6, 'auto-flagged')))).toBe('degraded'); // 6% < 10% + expect(evaluateIntegrity(snapshotWith(10, many(4, 'auto-flagged')))).toBe('degraded'); // count 4 < 5 + }); + + it('still reports ok with no skips', () => { + expect(evaluateIntegrity(snapshotWith(101, []))).toBe('ok'); + }); + + it('keeps auto-flagged skips visible in run_metadata while still counting them', () => { + // Counting them must not remove them from the surfaced list — the badge + // needs to name who was dropped. Asserts against countableSkips (the shared + // predicate the badge and the runner use), not just the fixture, so this + // fails if the predicate ever stops counting auto-flagged. + const snap = snapshotWith(101, many(2, 'auto-flagged')); + expect(snap.skipped).toHaveLength(2); + expect(countableSkips(snap.skipped)).toHaveLength(2); + }); + + it('countableSkips counts unknown and auto-flagged, never allowlisted', () => { + const snap = snapshotWith(100, [ + ...many(2, 'unknown', 'x'), + ...many(3, 'auto-flagged', 'y'), + ...many(4, 'expected', 'z'), + ]); + expect(countableSkips(snap.skipped).map(s => s.login).sort()) + .toEqual(['x0', 'x1', 'y0', 'y1', 'y2']); + }); +}); + +/** + * The abort fired correctly and then misreported itself: report-runner and + * IntegrityBadge each re-filtered for 'unknown', so an abort driven entirely by + * auto-flagged members persisted "0 of 102 engineers couldn't be fetched (0%)" + * into reports.error — telling the on-call that nothing was dropped, which is + * the same class of silent misreporting this whole fix is about. + */ +describe('abort message agrees with the verdict that produced it', () => { + it('names the real count on the GLOOK-13 shape instead of a false zero', () => { + const snap = snapshotWith(102, many(41, 'auto-flagged')); + expect(evaluateIntegrity(snap)).toBe('failed'); + + const reason = formatIntegrityAbortReason(snap); + expect(reason).toContain('41 of 102'); + expect(reason).toContain('(40%)'); + expect(reason).not.toContain('0 of 102'); + expect(reason).not.toContain('(0%)'); + }); + + it('reports counts and denominator consistently when skips are mixed', () => { + const snap = snapshotWith(100, [...many(6, 'unknown', 'u'), ...many(20, 'expected', 'a')]); + // 20 allowlisted leave both numerator and denominator: 6 of 80 = 7.5% -> 8%. + expect(formatIntegrityAbortReason(snap)).toContain('6 of 80'); + }); +}); + +/** + * The denominator has to shed allowlisted members too. Otherwise each allowlist + * addition makes the percentage gate strictly less sensitive — and since the + * thresholds are compile-time constants, promoting candidates into the + * allowlist is the ONLY lever for unblocking a hard-failing run. The guard + * would desensitise precisely as it gets used. + */ +describe('allowlisted members leave the denominator, not just the numerator', () => { + it('does not let allowlist growth dilute the percentage gate', () => { + // 6 genuine failures in a 100-member org with 40 allowlisted. + // Against the full 100 that is 6% — under abortUnknownPct (10%), so it + // would only degrade. Against the 60 members actually expected it is 10%, + // which is the honest reading and aborts. + const snap = snapshotWith(100, [...many(40, 'expected', 'a'), ...many(6, 'unknown', 'u')]); + const { countable, allowlisted, effectiveExpected } = integrityCounts(snap); + expect({ countable, allowlisted, effectiveExpected }).toEqual({ + countable: 6, allowlisted: 40, effectiveExpected: 60, + }); + expect(evaluateIntegrity(snap)).toBe('failed'); + }); + + it('still reports ok when every skip is allowlisted', () => { + const snap = snapshotWith(101, many(41, 'expected')); + expect(integrityCounts(snap).countable).toBe(0); + expect(evaluateIntegrity(snap)).toBe('ok'); + }); + + it('never produces a negative denominator', () => { + // Defensive: expectedCount smaller than the allowlisted skip count. + const snap = snapshotWith(2, many(5, 'expected')); + expect(integrityCounts(snap).effectiveExpected).toBe(0); + expect(integrityCounts(snap).countablePct).toBe(0); + expect(evaluateIntegrity(snap)).toBe('ok'); + }); +}); diff --git a/src/lib/__tests__/unit/skip-classifier.test.ts b/src/lib/__tests__/unit/skip-classifier.test.ts index e0be21a3..bbbcb304 100644 --- a/src/lib/__tests__/unit/skip-classifier.test.ts +++ b/src/lib/__tests__/unit/skip-classifier.test.ts @@ -130,8 +130,13 @@ describe('evaluateIntegrity', () => { }))).toBe('degraded'); }); - it("does NOT count 'auto-flagged' against the threshold", () => { + // Changed by the 2026-09-02 GLOOK-13 regression fix. This previously asserted + // 'ok', which meant the exact incident GLOOK-13 was filed about (41 of 102 + // members dropped) passed the guard as soon as those members had been failing + // long enough to be auto-flagged. Only a human allowlist entry ('expected') + // may suppress the guard now. See integrity-guard-regression.test.ts. + it("DOES count 'auto-flagged' against the threshold", () => { const skips: Array<[string, 'auto-flagged']> = Array.from({ length: 41 }, (_, i) => [`u${i}`, 'auto-flagged']); - expect(evaluateIntegrity(trackerWith({ expectedCount: 102, skips }))).toBe('ok'); + expect(evaluateIntegrity(trackerWith({ expectedCount: 102, skips }))).toBe('failed'); }); }); diff --git a/src/lib/github.ts b/src/lib/github.ts index 81ac676d..f96cb02c 100644 --- a/src/lib/github.ts +++ b/src/lib/github.ts @@ -101,6 +101,148 @@ const TRANSIENT_BACKOFF_MS = [1000, 2000, 4000]; // attempt 1, 2, 3 const TOTAL_MAX_ATTEMPTS = 12; // hard cap across all error types (5xx/429/network mix) +/** Base wait for a secondary (abuse-detection) rate limit, before exponential growth. */ +const SECONDARY_BASE_SEC = 60; +/** Cap on a single secondary wait, so one unrecoverable call cannot stall a run for an hour. */ +const SECONDARY_MAX_SEC = 300; +/** Floor for a primary-limit wait derived from x-ratelimit-reset. */ +const PRIMARY_FLOOR_SEC = 10; +/** Base for the primary schedule when a 429 carries no usable headers at all. */ +const PRIMARY_FALLBACK_BASE_SEC = 30; + +/** + * GitHub renamed "abuse detection mechanism" to "secondary rate limit" but + * still emits the old wording on some endpoints, so both must match. + */ +const SECONDARY_PHRASES = /secondary rate limit|abuse detection mechanism/i; +/** Any 403 that names a rate limit is one, even when the headers are missing. */ +const RATE_LIMIT_PHRASES = /rate limit|abuse detection mechanism/i; + +interface GitHubErrorLike { + status?: number; + message?: string; + response?: { + status?: number; + headers?: Record; + data?: { message?: string }; + }; +} + +function statusOf(err: unknown): number | undefined { + const e = err as GitHubErrorLike | null | undefined; + return e?.status ?? e?.response?.status; +} + +function headersOf(err: unknown): Record { + const e = err as GitHubErrorLike | null | undefined; + return e?.response?.headers ?? {}; +} + +/** + * Both message carriers, concatenated rather than `??`-chained. Octokit's + * RequestError always sets `.message`, so a `??` chain makes + * `response.data.message` unreachable — and that is the carrier a non-Octokit + * caller or a raw fetch surfaces. + */ +function messageOf(err: unknown): string { + const e = err as GitHubErrorLike | null | undefined; + return `${e?.message ?? ''} ${e?.response?.data?.message ?? ''}`; +} + +/** + * Is this 403/429 a rate limit at all? + * + * 429 always is. A 403 is only a rate limit when GitHub says so — via + * `retry-after`, the wording, or an exhausted primary quota. Everything else + * with a 403 is a permission condition (SSO/SAML enforcement, missing scope, + * `Resource not accessible by integration`) that no amount of waiting fixes; + * those propagate immediately the way 404 already does, instead of burning the + * whole retry budget on a deterministic failure. + */ +export function isRateLimitError(err: unknown): boolean { + const status = statusOf(err); + if (status === 429) return true; + if (status !== 403) return false; + + const h = headersOf(err); + if (h['retry-after'] !== undefined) return true; + if (RATE_LIMIT_PHRASES.test(messageOf(err))) return true; + return String(h['x-ratelimit-remaining']) === '0'; +} + +/** + * GitHub has two rate limits and they need different backoffs. + * + * The primary limit is quota-based: `/rate_limit` reports it and responses + * carry `x-ratelimit-reset`, so waiting until that reset is exactly right. + * + * The secondary (abuse-detection) limit is in neither. During the 2026-09-02 + * incident the search quota read 30/30 while search calls were being rejected. + * Treating that as a primary limit meant deriving the wait from + * `x-ratelimit-reset` — the *healthy* primary window — so the wait collapsed to + * the 10s floor and the retry immediately re-tripped the same limit. + * + * Detection is structural first and textual second. GitHub's own documented + * algorithm discriminates on remaining quota, not on prose: a rate-limited + * response with primary quota left cannot be a primary limit, whatever the + * message says. Relying on the wording alone left the original bug one + * rewording away from returning. + */ +export function isSecondaryRateLimit(err: unknown): boolean { + if (!isRateLimitError(err)) return false; + if (SECONDARY_PHRASES.test(messageOf(err))) return true; + + const remaining = headersOf(err)['x-ratelimit-remaining']; + return remaining !== undefined && String(remaining) !== '0'; +} + +/** + * RFC 7231 allows `Retry-After` to be delta-seconds or an HTTP-date; GitHub + * sends delta-seconds. Returns null when absent or unparseable so the caller + * falls through to a real schedule instead of inventing a number — the previous + * `Number(raw) || 60` turned `retry-after: 0` into a 60s wait and an HTTP-date + * into `NaN || 60`. + */ +function parseRetryAfter(raw: unknown): number | null { + if (raw === undefined || raw === null || raw === '') return null; + const sec = Number(raw); + if (Number.isFinite(sec)) return Math.max(sec, 0); + const at = Date.parse(String(raw)); + if (Number.isFinite(at)) return Math.max(Math.ceil((at - Date.now()) / 1000), 0); + return null; +} + +/** How long to wait before retrying a rate-limited GitHub call. */ +export function rateLimitWaitSeconds( + err: unknown, + attempt: number, + nowSec: number = Math.floor(Date.now() / 1000), +): number { + const h = headersOf(err); + const secondary = isSecondaryRateLimit(err); + const secondarySchedule = Math.min(SECONDARY_BASE_SEC * Math.pow(2, attempt), SECONDARY_MAX_SEC); + + const asked = parseRetryAfter(h['retry-after']); + if (asked !== null) { + // Never retry earlier than GitHub asked. On a secondary limit keep the + // escalation as well: GitHub routinely sends retry-after on abuse-detection + // 403s, and returning it flat meant every one of the 5 attempts waited the + // same 60s, re-tripping the limit at the boundary — the same no-growth shape + // as the bug this fixes. + return secondary ? Math.max(asked, secondarySchedule) : asked; + } + + // Deliberately ignore x-ratelimit-reset here: it describes the primary window. + if (secondary) return secondarySchedule; + + const resetEpoch = h['x-ratelimit-reset']; + if (resetEpoch !== undefined) { + const reset = Number(resetEpoch); + if (Number.isFinite(reset)) return Math.max(reset - nowSec, PRIMARY_FLOOR_SEC); + } + return PRIMARY_FALLBACK_BASE_SEC * Math.pow(2, attempt); +} + export async function withRetry( fn: () => Promise, log?: (msg: string) => void, @@ -118,24 +260,18 @@ export async function withRetry( const status = err?.status || err?.response?.status; const networkCode = err?.code as string | undefined; - const isRateLimit = status === 403 || status === 429; + const isRateLimit = isRateLimitError(err); const is5xx = typeof status === 'number' && status >= 500 && status < 600; const isNetwork = !!networkCode && NETWORK_ERROR_CODES.has(networkCode); - // 1. Rate limit — existing behavior preserved (longer backoffs, header-aware) + // 1. Rate limit — primary waits until x-ratelimit-reset, secondary escalates + // 60s→300s. A 403 that is NOT a rate limit falls through to case 3. if (isRateLimit) { if (attempt === maxRetries) throw err; - const retryAfter = err?.response?.headers?.['retry-after']; - const resetEpoch = err?.response?.headers?.['x-ratelimit-reset']; - let waitSec: number; - if (retryAfter) { - waitSec = Number(retryAfter) || 60; - } else if (resetEpoch) { - waitSec = Math.max(Number(resetEpoch) - Math.floor(Date.now() / 1000), 10); - } else { - waitSec = 30 * Math.pow(2, attempt); - } - log?.(`Rate limited (attempt ${attempt + 1}/${maxRetries}). Waiting ${waitSec}s…`); + const secondary = isSecondaryRateLimit(err); + const waitSec = rateLimitWaitSeconds(err, attempt); + const kind = secondary ? 'Secondary rate limit' : 'Rate limited'; + log?.(`${kind} (attempt ${attempt + 1}/${maxRetries}). Waiting ${waitSec}s…`); await sleep(waitSec * 1000); continue; } diff --git a/src/lib/report-runner.ts b/src/lib/report-runner.ts index 0e8f1076..d0016db0 100644 --- a/src/lib/report-runner.ts +++ b/src/lib/report-runner.ts @@ -13,7 +13,7 @@ import { refreshCcSpendForReport } from './cc-spend/service'; import { AnthropicAnalyticsKeyMissingError } from './cc-spend/anthropic-provider'; import { IntegrityTracker } from './report-runner/integrity-tracker'; import { loadSkipClassifier, evaluateIntegrity } from './report-runner/skip-classifier'; -import { DEFAULT_THRESHOLDS, type RunMetadata } from './report-runner/types'; +import { DEFAULT_THRESHOLDS, formatIntegrityAbortReason, type RunMetadata } from './report-runner/types'; const CONCURRENCY = Number(process.env.LLM_CONCURRENCY || 5); @@ -452,10 +452,10 @@ export async function runReport( const integrityState = evaluateIntegrity(integritySnapshot); if (integrityState === 'failed') { - const unknownCount = integritySnapshot.skipped.filter(s => s.classification === 'unknown').length; - const expectedCount = integritySnapshot.expectedCount; - const pct = expectedCount > 0 ? Math.round((unknownCount / expectedCount) * 100) : 0; - const abortReason = `GitHub API degraded: ${unknownCount} of ${expectedCount} engineers couldn't be fetched (${pct}%). Likely upstream auth/permission regression.`; + // Built from the same integrityCounts() the verdict came from. This used + // to re-filter for 'unknown' only, so an abort driven by auto-flagged + // members reported "0 of 102 (0%)" and read like a false alarm. + const abortReason = formatIntegrityAbortReason(integritySnapshot); log(`ABORT (GLOOK-13): ${abortReason}`); const runMetadata: RunMetadata = { diff --git a/src/lib/report-runner/skip-classifier.ts b/src/lib/report-runner/skip-classifier.ts index a77c342b..15e36587 100644 --- a/src/lib/report-runner/skip-classifier.ts +++ b/src/lib/report-runner/skip-classifier.ts @@ -5,6 +5,7 @@ import db from '@/lib/db'; import type { IntegrityState, RunMetadata, SkipClassification } from './types'; +import { integrityCounts } from './types'; export const AUTO_FLAG_RECENT_RUNS = 5; export const AUTO_FLAG_THRESHOLD = 4; @@ -19,9 +20,19 @@ export async function loadRecentSkipCounts(limit = AUTO_FLAG_RECENT_RUNS): Promi // bound LIMIT params. `limit` is sanitized via Number() — callers pass // a hardcoded module constant, never user input. const safeLimit = Number(limit) || AUTO_FLAG_RECENT_RUNS; + // 'failed' runs are included deliberately. A run that aborts on the integrity + // guard is exactly the run whose skips an operator needs to see, and + // promoting a candidate into report_skip_allowlist is the only lever for + // unblocking one (the thresholds are compile-time constants). Filtering to + // 'completed' froze the history the moment runs started failing, so the + // Settings candidate list — the unblock path — stayed empty. + // + // Safe because auto-flagging no longer silences anything: 'auto-flagged' and + // 'unknown' both count toward the thresholds, so this only affects the labels + // and the Settings suggestions. const [recentRows] = await db.execute( `SELECT run_metadata FROM reports - WHERE status = 'completed' AND run_metadata IS NOT NULL + WHERE status IN ('completed', 'failed') AND run_metadata IS NOT NULL ORDER BY completed_at DESC LIMIT ${safeLimit}`, ) as [any[], any]; @@ -74,18 +85,35 @@ export async function loadSkipClassifier(): Promise<(login: string) => SkipClass /** * Pure evaluator — given a tracker snapshot, returns the integrity state. - * Only unknown SKIPs count toward thresholds; expected + auto-flagged are - * surfaced in run_metadata but ignored here. + * + * Which skips count, and against what denominator, lives in `integrityCounts` + * (types.ts) so the runner's abort message and the UI badge cannot drift from + * this verdict. Only 'expected' SKIPs are excluded: those are members a human + * put on the allowlist, i.e. someone looked and accepted the gap. + * + * 'auto-flagged' DOES count. It previously did not, which is how GLOOK-13 + * regressed on 2026-09-02: a member failing persistently is auto-flagged after + * >=4 of the last 5 runs (see loadSkipClassifier), so it dropped out of this + * calculation and the report went back to 'ok'. Reports fell from 65 to 51 + * developers over four runs with integrity green the whole way, because every + * newly-failing member walked the same path out of the numerator. + * + * Auto-flagging is a *suggestion* — /api/settings/skip-allowlist surfaces + * `autoFlaggedCandidates` for a human to promote to the allowlist. Suggesting + * that someone might be a known-inactive member is not the same as confirming + * it, and only the confirmation may silence the alarm. + * + * The AND gate on abort is deliberate and unchanged: a run must be degraded in + * both absolute and relative terms before it aborts, so a small org isn't + * killed by a handful of skips and a large one isn't killed by a rounding error. */ export function evaluateIntegrity( snapshot: Pick, ): IntegrityState { const T = snapshot.thresholds; - const unknownCount = snapshot.skipped.filter(s => s.classification === 'unknown').length; - const expected = snapshot.expectedCount; - const unknownPct = expected > 0 ? unknownCount / expected : 0; + const { countable, countablePct } = integrityCounts(snapshot); - if (unknownCount >= T.abortUnknownCount && unknownPct >= T.abortUnknownPct) return 'failed'; - if (unknownCount >= T.degradedUnknownCount || unknownPct >= T.degradedUnknownPct) return 'degraded'; + if (countable >= T.abortUnknownCount && countablePct >= T.abortUnknownPct) return 'failed'; + if (countable >= T.degradedUnknownCount || countablePct >= T.degradedUnknownPct) return 'degraded'; return 'ok'; } diff --git a/src/lib/report-runner/types.ts b/src/lib/report-runner/types.ts index 2f0144f0..ec340b5a 100644 --- a/src/lib/report-runner/types.ts +++ b/src/lib/report-runner/types.ts @@ -53,3 +53,65 @@ export const DEFAULT_THRESHOLDS: IntegrityThresholds = { degradedUnknownCount: 3, degradedUnknownPct: 0.05, }; + +/** + * Skip classifications that count against the integrity thresholds. + * + * Deliberately an explicit inclusion list rather than `!== 'expected'`: adding + * a new SkipClassification must be a conscious decision about whether it is + * allowed to silence the guard. The 2026-09-02 regression happened precisely + * because 'auto-flagged' drifted out of this set. + */ +export const COUNTABLE_SKIP_CLASSIFICATIONS: readonly SkipClassification[] = [ + 'unknown', + 'auto-flagged', +]; + +/** The skips that count against the thresholds — everything except human-allowlisted. */ +export function countableSkips( + skipped: readonly T[], +): T[] { + return skipped.filter((s) => COUNTABLE_SKIP_CLASSIFICATIONS.includes(s.classification)); +} + +/** + * The single place the integrity numerator and denominator are computed. + * + * `evaluateIntegrity`, the runner's abort message, and the UI badge all read + * from here. Three hand-written copies of "which skips count" is what let the + * guard abort correctly and then tell the operator `0 of 102 (0%)`. + */ +export function integrityCounts( + snapshot: Pick, +): { countable: number; allowlisted: number; effectiveExpected: number; countablePct: number } { + const countable = countableSkips(snapshot.skipped).length; + const allowlisted = snapshot.skipped.filter((s) => s.classification === 'expected').length; + + // Allowlisted members leave the numerator, so they must leave the denominator + // too. Otherwise every allowlist addition makes the percentage gate strictly + // less sensitive — and since the thresholds are compile-time constants, the + // allowlist is the only lever for unblocking a hard-failing run. The guard + // would desensitise exactly as it gets used. + const effectiveExpected = Math.max((snapshot.expectedCount ?? 0) - allowlisted, 0); + const countablePct = effectiveExpected > 0 ? countable / effectiveExpected : 0; + + return { countable, allowlisted, effectiveExpected, countablePct }; +} + +/** + * The operator-facing abort summary, persisted to `reports.error` and + * `run_metadata.abortReason` and rendered verbatim by IntegrityBadge. + * + * Lives here, next to the counts it reports, so the message can never again + * disagree with the verdict that produced it. + */ +export function formatIntegrityAbortReason( + snapshot: Pick, +): string { + const { countable, effectiveExpected, countablePct } = integrityCounts(snapshot); + const pct = Math.round(countablePct * 100); + return ( + `GitHub API degraded: ${countable} of ${effectiveExpected} engineers couldn't be fetched ` + + `(${pct}%). Likely upstream auth/permission regression.` + ); +}