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

[msogin]

Fixes GLOOK-48. Regression of GLOOK-13.

What broke

Reports progressively lost developers over two days — 65 → 64 → 59 → 51 — while every run reported integrity: ok and status completed. Roughly half the GDN team was missing with no warning anywhere in the UI.

It took two independent defects to produce a silently wrong report, which is why this ships as one PR.

1. The trigger — secondary rate limit misclassified as primary

GitHub has two rate limits, and withRetry only knew about one.

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 appears in neither — during the incident the search quota read 30/30 while search calls were being rejected with You have exceeded a secondary rate limit.

Because the code derived its wait from x-ratelimit-reset — which describes the healthy primary window — the computed wait collapsed to the 10s floor, the retry immediately re-tripped the same limit, and the whole 5-attempt budget burned in ~50s before throwing. Reproduced locally with ~6 successive search calls.

report-runner.ts:429 then caught it, logged SKIP @login, and dropped the member. The report still completed.

Fix: isSecondaryRateLimit() detects it by response message on 403/429. Secondary limits ignore x-ratelimit-reset and wait 60s, doubling to a 300s cap. retry-after still wins when present, and primary-limit behaviour is unchanged.

The 2.5s inter-request pacing is deliberately untouched — it held for months; the retry was the broken lever.

2. Why nothing alarmed — the integrity guard stopped counting

evaluateIntegrity counted only skips classified unknown. But classifySkip auto-flags any login that failed in ≥4 of the last 5 runs (AUTO_FLAG_RECENT_RUNS=5, AUTO_FLAG_THRESHOLD=4).

So a chronically failing member left the numerator after four runs and integrity went back to green. That is a ratchet, and it is exactly the 65 → 51 slide: every newly-failing developer walks the same path out of the calculation.

Auto-flagging is only a suggestion — /api/settings/skip-allowlist surfaces autoFlaggedCandidates for a human to promote into report_skip_allowlist. Suggesting someone might be a known-inactive member is not the same as confirming it, and only the confirmation should be able to silence the guard.

Fix: evaluateIntegrity now excludes only expected (human-allowlisted) skips. The AND gate on abort is deliberately left as-is — 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.

Corroborating evidence

junky (28 real commits in the window) and ningjiang118 (4) were absent from the report; oleksandrkuzmin (0 commits) was correctly absent. All three appear in GLOOK-13's original May list of 41 dropped members — the same population, re-dropped.

Tests

• github-secondary-rate-limit.test.ts (10 tests) — secondary detection, retry-after precedence, ignoring x-ratelimit-reset on secondary, growth-and-cap, primary behaviour preserved.
• integrity-guard-regression.test.ts (8 tests) — encodes GLOOK-13's stated requirement, including its own 41-of-102 incident shape, which the old code passed as ok.
• The skip-classifier test that pinned the bug (does not count 'auto-flagged') is inverted, with a comment recording why it changed.

123 suites / 1184 tests pass; npm run build succeeds.

Verified in dev

Deployed as glooker-dev:88 (image 9ab42d7) and generated a report successfully.

Reviewer note

IntegrityThresholds fields are still named abortUnknownCount / degradedUnknownPct although they no longer count only "unknown". Left as-is to keep this diff to the fix; noted as a follow-up on GLOOK-48.

Also worth knowing: on a deployment that has accumulated chronic skips, the first run after this lands may legitimately hard-fail rather than complete, because those members start counting again. That is the guard working — triage them into report_skip_allowlist if genuinely inactive.

[msogin]

Automated review — 3 reviewers

Ran three independent reviewers over ac6d4ba..9ab42d7: a Sr. Architect (holistic), a Senior Backend Dev (correctness lens), and the standard Smartling fullstack review. Repo custom properties came back empty and there is no .github/claude-code-review.json, so the standard review used the fullstack profile with no production-tier prefix.

Both mechanisms in this PR check out against the code, and the diagnosis is well-evidenced. npx jest (1184 tests) and tsc --noEmit are clean. One critical defect should land before merge, plus two weaknesses that leave the fixed bug reachable.

All three reviewers independently found the same top issue, which is the main reason I'd hold merge on it.

The critical one, in short

evaluateIntegrity now counts !== 'expected', but the two consumers of that verdict were not updated — both still filter === 'unknown':

• src/lib/report-runner.ts:455 (abort reason)
• src/components/IntegrityBadge.tsx:49 (degraded pill)

Feed it the exact case this PR's own new test pins (41 auto-flagged of 102, integrity-guard-regression.test.ts:44): the guard correctly aborts, then writes into reports.error and run_metadata.abortReason:

GitHub API degraded: 0 of 102 engineers couldn't be fetched (0%). Likely upstream auth/permission regression.

An on-call reading "0 of 102 (0%)" dismisses the abort. The guard fires correctly and then misreports itself — the same class of silent misreporting this PR fixes.

Worth noting the shared root cause: "which skips count" now has four copies — evaluateIntegrity, report-runner.ts:455, IntegrityBadge.tsx:49, and the abortUnknownCount/degradedUnknownPct field names. This PR updates one. Exporting a single countableSkips() predicate (or having evaluateIntegrity return { state, countable, countablePct }) would make the drift structurally impossible rather than fixing today's instance of it.

Structural note (not a merge blocker)

The architect reviewer raised one point worth recording on GLOOK-48 rather than fixing here: the ratchet is moved to the allowlist, not closed. Two silencing paths survive.

1. Settings → Skip Allowlist has a one-click Promote that writes auto-promoted: SKIPped in 4+ of the last 5 runs and converts a candidate to expected permanently — no expiry, no re-validation. Since DEFAULT_THRESHOLDS are compile-time literals with no override, that button is now the only lever to unblock a hard-failing run, which is real pressure to click it.
2. expectedCount comes from the same listOrgMembers call that supplies the members (report-runner.ts:91). Anyone who drops out of the org listing leaves both numerator and denominator — so a 65→51 loss caused by a truncated member list still reads integrity: ok after this PR.

Both are instances of the guard only ever measuring "failures we noticed this run". Changing which classifications count is the line-level fix that has now been applied twice. Closing it properly needs one absolute reference — e.g. a run-over-run delta on member/developer count that fails independently of classification.

Smaller items, folded in here rather than inline

• integrity-guard-regression.test.ts:81 — "keeps auto-flagged skips visible in run_metadata" only exercises IntegrityTracker.snapshot(), which this diff doesn't touch; it passes against the old code too. Harmless, but it inflates apparent regression coverage. The other six cases do genuinely fail against the old filter (verified by hand: 41 auto-flagged/102 → old ok; 6 unknown + 6 auto-flagged/101 → old degraded, new failed).
• skip-classifier.ts:101 — !== 'expected' is open-ended: any future SkipClassification silently counts toward abort. An explicit inclusion list (['unknown','auto-flagged'].includes(...)) makes adding one a deliberate decision.
• types.ts:50-55 — on the threshold rename you already noted as follow-up: IntegrityThresholds is serialised verbatim into reports.run_metadata, so the rename has to tolerate historical rows carrying the old keys. Not a pure find-and-replace.
• github.ts:124,132 — err: any on newly-exported public API; unknown with narrowing keeps call sites honest.
• Pre-existing, out of this diff: withRetry classifies retryability by status alone (github.ts:170), so non-rate-limit 403s — SSO/SAML enforcement, Resource not accessible, missing scope — take the rate-limit branch and burn all 5 retries. The x-ratelimit-remaining gate suggested inline is the natural place to also short-circuit these and propagate immediately, the way 404 already does.
• Docs drift: CLAUDE.md:50 still describes the old model ("exponential back-off retry on 403/429") with no primary/secondary split, and docs/superpowers/plans/2026-06-01-glook-13-report-integrity.md documents the superseded "only unknown SKIPs count" rule.

Assessment

Ready to merge? With fixes — the abort-message defect, since it defeats the operator-facing half of the guard. The two github.ts hardening items are close behind: as written, one variation in GitHub's error wording reinstates the 10s-floor collapse.

_{🤖 Generated with Claude Code}

[msogin]

🔴 critical — the abort message this guard produces reports a false zero

evaluateIntegrity now counts !== 'expected', but the two consumers of its verdict still filter === 'unknown':

• src/lib/report-runner.ts:455 — builds abortReason
• src/components/IntegrityBadge.tsx:49 — the degraded pill

In the exact shape this PR's own test pins (41 auto-flagged of 102, integrity-guard-regression.test.ts:44), unknownCount is 0, so the abort persists to reports.error and run_metadata.abortReason:

GitHub API degraded: 0 of 102 engineers couldn't be fetched (0%). Likely upstream auth/permission regression.

That string is rendered verbatim by IntegrityBadge.tsx:29. The guard fires correctly and then tells the operator nothing was dropped — the same misreporting class this PR fixes. The degraded pill has the same divergence (⚠ 41 partial with the (N unknown) suffix suppressed).

Rather than changing the two filters (which leaves four copies of the rule), make it un-driftable:

// skip-classifier.ts
export function countableSkips(skipped: SkippedMember[]): SkippedMember[] {
  return skipped.filter(s => s.classification !== 'expected');
}

export function evaluateIntegrity(snapshot: ...): IntegrityState {
  const countable = countableSkips(snapshot.skipped).length;
  ...
}

Then use countableSkips at both call sites. Found independently by all three reviewers.

[msogin]

Fixed in 06aa5db — and taking the un-driftable route you suggested rather than patching the two filters.

countableSkips() and integrityCounts() now live in report-runner/types.ts. That module rather than skip-classifier.ts because skip-classifier imports db, so a client component can't touch it — which is exactly why IntegrityBadge had its own copy. All three consumers now read from the one helper, and grep for a hand-written classification filter outside types.ts comes back empty.

COUNTABLE_SKIP_CLASSIFICATIONS is an explicit ['unknown','auto-flagged'] inclusion list, per your skip-classifier.ts:101 note, so a new SkipClassification can't silently join the countable set.

The abort string moved into formatIntegrityAbortReason() next to the counts it reports, so the message cannot disagree with the verdict again. Pinned by a test asserting the 41/102 shape contains 41 of 102 / (40%) and explicitly not 0 of 102 / (0%).

[msogin]

🟡 warning — allowlisted members leave the numerator but stay in the denominator

countable excludes expected, but expected = snapshot.expectedCount is still the full org member list (report-runner.ts:91: expectedCount: members.length). So every allowlist addition makes the percentage gate strictly less sensitive.

With 40 allowlisted of 100 members you now need 10 genuine failures to hit abortUnknownPct: 0.10 instead of 6. And your own rollout note ("the first run may legitimately hard-fail on accumulated chronic skips") guarantees the allowlist is about to grow — so the guard desensitises exactly as it is used.

const expected = snapshot.expectedCount - snapshot.skipped.filter(s => s.classification === 'expected').length;

The new test excludes allowlisted members from the count but not the others still passes under that denominator (3/60 = 5% is still degraded).

[msogin]

Fixed in 06aa5db. You're right, and the reasoning about why it matters is the part I'd missed: the allowlist is the only unblock lever, so the guard was set to desensitise precisely as it gets used.

integrityCounts() now returns effectiveExpected = expectedCount - allowlisted, clamped at 0.

Your worked example is now a test: 6 genuine failures in a 100-member org with 40 allowlisted reads 6/60 = 10% and aborts, where before it was 6/100 = 6% and only degraded. And confirming your prediction — excludes allowlisted members from the count but not the others still passes untouched (3 auto-flagged, 40 allowlisted of 100 → 3/60 = 5%, still degraded).

[msogin]

🟣 question — rollout: a hard-failing run produces no report at all, and does not self-heal

With auto-flagged counted, any org carrying ≥5 chronic un-allowlisted skips at ≥10% now hard-fails every run (DEFAULT_THRESHOLDS, types.ts:51-52). On 'failed' the runner returns at report-runner.ts:474 before Jira collection and before any developer_stats are written — so operators go from a partial report to zero data, where the same input previously shipped something.

Your second test case (13 auto-flagged of 101) looks like the current production shape, so the first post-deploy run aborts. I read that as intended. The part I'd want confirmed is that it is escapable:

• loadRecentSkipCounts reads WHERE status = 'completed' (skip-classifier.ts:24), so once runs start failing the auto-flag history freezes — it can't age out.
• IntegrityThresholds uses literal values with no override path, so thresholds can't be relaxed as a temporary escape hatch.

That leaves a human writing report_skip_allowlist rows as the only unblock. Is the allowlist being seeded before this ships, and was an operator override considered? Otherwise the tool goes dark until someone triages by hand.

[msogin]

Good question, and one part of it was a real defect rather than just a rollout concern.

The freeze is fixed (06aa5db). loadRecentSkipCounts filtered status = 'completed', so the moment runs started hard-failing the auto-flag history stopped advancing — and with it the Settings autoFlaggedCandidates list, which is the unblock path. It now reads status IN ('completed','failed'). That's safe specifically because of this PR: now that auto-flagged and unknown both count, the classification no longer gates the verdict at all — it only drives labels and the Settings suggestions. So the failing runs that need triaging are the ones that now populate the triage list.

Hard-fail-over-partial-report is intended, and confirmed with the ticket owner before implementation — a report that silently ships 51 of 102 developers is worse than no report, because people act on it. GLOOK-13's stated requirement is that the 41/102 run "should have aborted, not silently shipped".

No threshold override, deliberately. An env-var escape hatch would be a third silencing path, and adding one to a PR whose entire subject is "a silencing path let reports go wrong for two days" seemed like the wrong trade. Promoting a genuinely-inactive member into report_skip_allowlist is a decision with an audit trail; turning the guard down globally isn't.

On seeding: dev's current population is under threshold — the post-deploy report generated fine on glooker-dev:88. So there's no pre-seeding to do, but I've flagged in the PR description that a deployment carrying more accumulated chronic skips may abort its first run by design.

Your underlying point — that this only ever measures "failures we noticed this run", and expectedCount shares its source with the member list — is the one I can't close here. Recorded on GLOOK-48 as the follow-up, since it needs an absolute reference (run-over-run developer-count delta) rather than another adjustment to which classifications count.

[msogin]

🟡 warning — retry-after short-circuits the secondary escalation, so it never grows

This branch returns before isSecondaryRateLimit is consulted, and GitHub routinely sends retry-after on abuse-detection 403s. On that (common) path the wait is a flat 60s on every one of the 5 attempts — no doubling, no 300s ceiling. Retrying exactly at the boundary can re-trip the same limit and burn the whole budget in ~5 minutes: the same no-growth shape as the bug being fixed, just slower.

It also means SECONDARY_MAX_SEC is not actually an invariant, so the comment at line 106 ("cap on a single secondary wait, so one unrecoverable member cannot stall a run for an hour") overstates what the code guarantees.

Two smaller things on the same line: Number('0') || SECONDARY_BASE_SEC turns retry-after: 0 into a 60s wait, an RFC 7231 HTTP-date value becomes NaN || 60, and on the primary path an unparseable header also falls back to SECONDARY_BASE_SEC — a constant misnamed for that branch.

if (retryAfter) {
  const sec = Number(retryAfter);
  const floor = Number.isFinite(sec) && sec > 0 ? sec : SECONDARY_BASE_SEC;
  // Never retry earlier than GitHub asked, but keep growing on secondary limits.
  return isSecondaryRateLimit(err)
    ? Math.max(floor, Math.min(SECONDARY_BASE_SEC * 2 ** attempt, SECONDARY_MAX_SEC))
    : floor;
}

If strict Retry-After honouring is the deliberate choice, worth saying so in the comment — right now the advertised "60s doubling to a 300s cap" schedule never engages on this path. Found by all three reviewers.

[msogin]

Fixed in 9398d58 — and you're right that the advertised schedule never engaged on the common path, which made the whole escalation decorative.

Now: retry-after sets a floor, the secondary schedule still escalates on top of it — Math.max(asked, min(60 * 2**attempt, 300)). So GitHub's request is never undercut, but repeated trips back off instead of hammering the boundary. Primary limits honour retry-after exactly, with no escalation.

Parsing fixed too, all three cases you named: retry-after: 0 now returns 0 rather than 60; an RFC 7231 HTTP-date is parsed via Date.parse instead of becoming NaN || 60; and an unparseable value returns null so the caller falls through to a real schedule rather than the primary path inheriting a constant named for the secondary one. PRIMARY_FALLBACK_BASE_SEC is now its own named constant.

Tests pin [60,120,240,300,300] with retry-after: 60 present, which is the exact flat-wait case.

You were also right that SECONDARY_MAX_SEC's comment overstated the guarantee — it holds per call, not per member or run. Reworded to say "per call", and the per-run cost is the subject of your github.ts:145 thread.

[msogin]

🟡 warning — detection rests entirely on GitHub's prose, so the original bug stays reachable

/secondary rate limit/i is the only signal, and a miss is silent: execution falls through to line 149, finds x-ratelimit-reset (present on essentially every REST response, including secondary blocks), and with a healthy primary quota returns Math.max(reset - now, 10) — the 10s floor this PR exists to eliminate. The only observable difference is one word in a log line.

Three ways it misses:

1. GitHub still emits the pre-rename wording on some endpoints: "You have triggered an abuse detection mechanism".
2. A non-JSON/HTML 403 body yields a message with neither phrase.
3. The ?? chain means err.response.data.message is consulted only when err.message is null/undefined. Octokit's RequestError always sets .message, so that fallback is effectively unreachable — and it's the one branch the new test file never exercises.

GitHub's own documented algorithm discriminates on x-ratelimit-remaining, not on wording — and that is exactly the signal your incident describes (search quota read 30/30 while calls were rejected). Corroborate structurally:

const msg = `${err?.message ?? ''} ${err?.response?.data?.message ?? ''}`;
return /secondary rate limit|abuse detection mechanism/i.test(msg);

and at line 148, gate the primary path on the quota actually being exhausted:

const resetEpoch = headers['x-ratelimit-reset'];
// Primary iff the quota is genuinely spent. A 403/429 with quota remaining
// is a secondary limit regardless of how the message is worded.
if (resetEpoch && String(headers['x-ratelimit-remaining']) === '0') {
  return Math.max(Number(resetEpoch) - nowSec, PRIMARY_FLOOR_SEC);
}
return Math.min(SECONDARY_BASE_SEC * Math.pow(2, attempt), SECONDARY_MAX_SEC);

That makes the wording a hint rather than the load-bearing signal. Found by all three reviewers.

[msogin]

Fixed in 9398d58, and this was the most valuable finding in the review — the fix as written really was one rewording away from reverting.

Detection is now structural first, textual second, per your x-ratelimit-remaining point:

• Added abuse detection mechanism to the phrase set.
• Both carriers are concatenated rather than ??-chained, so response.data.message is actually reachable — and it now has a test, which you correctly noted the old file never exercised.
• The load-bearing change: a rate-limited response with primary quota remaining is classified secondary regardless of wording. That's GitHub's own discriminator and the literal incident signal (30/30 while rejected), so prose is now only a hint.

One consequence worth flagging, because it's why this commit is bigger than your snippet: gating the primary path on x-ratelimit-remaining === '0' would have made the permission 403s from your last thread worse — no wording, quota healthy, so they'd fall to the secondary schedule and sleep 60s five times instead of 10s five times. So I took your pre-existing-issue note in the same commit: isRateLimitError() now treats a 403 as a rate limit only when GitHub says so (retry-after, rate-limit wording, or exhausted quota), and a plain permission 403 — SSO/SAML, missing scope, Resource not accessible by integration — propagates immediately the way 404 does.

429 is still unconditionally a rate limit. Tests cover both directions, including a withRetry test asserting a permission 403 is called exactly once.

[msogin]

🟡 warning — per-call backoff for a token-wide condition

A secondary limit applies to the token, not the request, but withRetry holds no shared state — so under a sustained block every member rediscovers it from scratch. With maxRetries = 5, attempt 0–4 gives 60+120+240+300+300 = 1020s (~17 min) of sleeping before that member throws and is SKIPped; then the next member starts again at 60s. (Confirmed attempt is 0 on first entry — the attempt-- in the transient branch is cancelled by the loop's ++ — so the first wait really is 60s, but the tail is long.)

Members are fetched sequentially over ~100 logins with several withRetry calls each, and the previous misclassified path cost ~50s per call. So this can turn a ~1.5h run into a many-hour one that then hard-fails under the new guard — and Croner jobs are registered with no overlap guard, so it collides with the next schedule. There's also no jitter.

Two follow-ups rather than blockers:

• Hoist a module- or run-scoped secondaryBlockedUntil timestamp, set on any secondary trip, that later calls await directly instead of each independently re-tripping and re-backing-off.
• shouldStop(reportId) is only checked between members, so a user pressing Stop can hang up to 300s inside sleep(). Worth making the sleep abortable.

Also worth reconciling with the comment at line 106: the cap holds per call, not per member or per run. Found by all three reviewers.

[msogin]

Not fixed here — taking your framing that these are follow-ups, and recording them on GLOOK-48. Agreed on the diagnosis, including the arithmetic: 60+120+240+300+300 = 1020s per member, and the escalation I just added to the retry-after path makes that more reachable, not less.

Deferring rather than fixing because a shared secondaryBlockedUntil is a different kind of change from the rest of this PR — it's cross-call coordination touching run orchestration, and it wants the abortable-sleep work to land with it or a Stop press gets worse, not better. Bundling it into an incident fix that's already verified in dev seemed like the wrong risk.

Recorded as one follow-up covering all three: token-scoped secondaryBlockedUntil so members don't each rediscover the block, jitter, and making sleep() abortable so shouldStop isn't up to 300s late. Your note about Croner having no overlap guard is on it too.

Thanks for confirming attempt is 0 on first entry — I'd relied on that for the 60s first wait without verifying the attempt--/++ interaction, so that's a real check I hadn't done.

[msogin]

🔵 suggestion — these assertions don't pin the schedule they document

toBeGreaterThanOrEqual(60) here, and w1 > w0 && w9 <= 300 at line 69, would all still pass if the base regressed to 30s, if growth went linear, or if the cap moved to 120. Assert the exact sequence instead:

expect([0,1,2,3,4].map(a => rateLimitWaitSeconds(e, a, NOW))).toEqual([60,120,240,300,300]);

Gaps worth adding while you're here:

• retry-after: '0' → Number('0') || 60 = 60, not 0. Defensible, but untested.
• A non-numeric or RFC 7231 HTTP-date retry-after → NaN || 60 = 60.
• retry-after at a non-zero attempt — the flat-wait path flagged at github.ts:141.
• err.response.data.message as the only carrier of the phrase (unreachable today, per the ?? note on github.ts:127).

Bigger gap: only the pure function is covered. Nothing asserts that withRetry actually sleeps ≥60s on a secondary 403 — i.e. the wiring added at github.ts:178, which is the behaviour this PR is about. github-retry.test.ts already has the fake-timer harness (runWithImmediateTimers) to pin that a 403-secondary schedules a ≥60,000ms timer.

[msogin]

Fixed in 9398d58. The bounds assertions were weak in exactly the way you describe — they'd have passed with a 30s base, linear growth, or a moved cap.

Now expect([0,1,2,3,4].map(a => rateLimitWaitSeconds(e, a, NOW))).toEqual([60,120,240,300,300]), plus the same exact-sequence form for the primary fallback ([30,60,120]) and for the retry-after-present secondary path.

All four gaps added: retry-after: '0' (now asserting 0, not 60 — it was a real bug, not just untested), a non-numeric value, an HTTP-date, retry-after at non-zero attempt, and response.data.message as sole carrier.

And the bigger gap — coverage of the wiring rather than just the pure function. Added to github-retry.test.ts to reuse its fake-timer harness as you suggested: a secondary 403 carrying x-ratelimit-reset only 5s out (the exact shape that used to collapse to the 10s floor) is asserted not to retry at 59,999ms and to retry at 60,000ms. That test fails against the code as originally submitted.

25 tests in the rate-limit file, 9 in the retry file.

[msogin]

Review addressed — 3 commits

Thanks, this was a good review: the critical finding was a genuine defect I'd introduced, and the x-ratelimit-remaining point closed a hole that would have made the whole fix one GitHub rewording away from reverting.

Replied on each thread. Summary:

Finding	Outcome
🔴 Abort message reports a false zero	Fixed — 06aa5db
🟡 Allowlisted stay in the denominator	Fixed — 06aa5db
🟣 Rollout: history freezes, no escape	Partly a defect, fixed — 06aa5db; design question answered on-thread
🟡 retry-after kills the escalation	Fixed — 9398d58
🟡 Detection rests on prose	Fixed — 9398d58
🟡 Per-call backoff for a token-wide condition	Deferred to GLOOK-48 follow-up (your framing)
🔵 Assertions don't pin the schedule	Fixed — 9398d58

Smaller items from the review body

• Vacuous test at integrity-guard-regression.test.ts:81 — you were right, it only asserted against its own fixture and passed against the old code. Replaced with one asserting through countableSkips (the shared predicate), plus a new case pinning that it counts unknown + auto-flagged and never expected.
• !== 'expected' is open-ended — now COUNTABLE_SKIP_CLASSIFICATIONS = ['unknown','auto-flagged'], an explicit inclusion list.
• err: any on new public API — now unknown, narrowed through small statusOf/headersOf/messageOf accessors. The remaining catch (err: any) in withRetry is pre-existing and out of scope.
• Threshold rename — still deferred, and thanks for the warning that IntegrityThresholds is serialised verbatim into run_metadata, so it has to tolerate historical rows carrying the old keys. That detail is on the follow-up; it would have been a nasty surprise treated as a find-and-replace.
• Non-rate-limit 403s burning the budget — no longer pre-existing-and-untouched: fixing it became required by the x-ratelimit-remaining gate, so it's in 9398d58. Details on that thread.
• Docs drift — f2871a0. CLAUDE.md now documents the primary/secondary split and the single-source-of-truth integrity rule; the GLOOK-13 plan's "only unknown SKIPs count" is marked superseded rather than silently edited, since it's a record of the original design. Also corrected the @octokit/rest note, which claimed a bare jest.mock('@octokit/rest') suffices — it doesn't, and the resulting error points at dist-src/index.js and reads like a transform-config problem.

Structural note — recorded, not fixed

Your point that the ratchet is moved to the allowlist rather than closed is the one I can't resolve inside this PR, and I don't want to pretend otherwise. expectedCount sharing its source with the member list means a truncated listOrgMembers still reads ok, because the loss leaves numerator and denominator together. Closing that needs an absolute reference — a run-over-run developer-count delta that fails independently of classification — not another adjustment to which classifications count. On GLOOK-48 as a follow-up.

Verification

• 123 suites / 1207 tests pass (up from 1184 — 23 added)
• tsc --noEmit clean on all git-tracked code
• npm run build succeeds (the badge's new import resolves — types.ts has no db import, which is why it hosts the shared helper)
• Previously verified in dev as glooker-dev:88; these commits are not yet deployed