GLOOK-50: never let a GitHub search timeout become a silent zero

[msogin]

Fixes GLOOK-50.

What broke

Developers were vanishing from reports with no skip, no error and no integrity warning — the report read perfectly healthy.

• dev, Sept 8: 66 → 58 developers and 954 → 899 commits overnight. Six engineers recorded 0 commits while GitHub held 21/15/9/8/5/2 for them.
• local, Sept 8: oprokopenko-smartling recorded 0 commits while GitHub held 43 — alongside 42 merged PRs.

Cause

GitHub's commit search succeeds and returns {total_count: 0, items: [], incomplete_results: true}.

Per GitHub's docs, a query exceeding the time limit "returns the matches that were already found prior to the timeout" with that flag set. So an empty page means the query did not finish, not that nothing exists. We read neither field anywhere in the codebase and treated the empty page as authoritative.

This is a third failure mode, and the distinction is what made it dangerous:

ticket	failure	guard sees it?
GLOOK-48	the fetch throws	yes — becomes a counted SKIP
GLOOK-49	genuinely no merged commits	n/a — correct behaviour
this	the fetch succeeds with a false zero	no — structurally blind

No integrity threshold could have caught it. My first analysis blamed the abort gate's AND condition; the logs disproved that — only one unrelated skip occurred.

Instrumentation came first, and changed the design

I shipped logging before behaviour, because two hypotheses were indistinguishable from the evidence available. One local run flagged six pages — only one was a real loss — which produced two corrections to my own plan:

• The total_count > 0 with empty items shape, which I had expected to be the trigger, occurred zero times. It's a real latent defect and is kept as a guard, but it was not the cause and I had over-weighted it.
• GitHub's caveat that a timeout "does not necessarily mean that search results are incomplete" is empirically true — 5 of 6 flagged pages were fine (3 genuine zeros, 2 complete-despite-flag). So the rule requires items.length === 0 too. Acting on the flag alone would retry constantly for one real fault, and would have needlessly retried bkoval-smartling (15 of 15) and kbroadrick-smartling (4 of 4).

The fix

GitHub prescribes no remedy beyond "narrow your query", so the policy is ours:

1. Retry on incomplete_results && items.length === 0 — twice, 5s apart.
2. Narrow, don't hammer. Still untrustworthy → halve the date window (max 2 levels). The second half keeps an open upper bound so no commit can fall off the end; halves are unioned by SHA. The un-split query is byte-identical to before.
3. Raise, never zero. Exhausted → throw, so it becomes a SKIP the GLOOK-13/48 guard counts. A loud skip is the goal, not a workaround.
4. Reconcile against total_count — a short page ending pagination below total_count is under-delivery → throw. Exact, rather than acting on an advisory flag.
5. Log total_count and incomplete_results at all four search sites, so the next occurrence is one query away instead of an excavation.

Also fixed

A separate fault this surfaced: pagination continued while hits < total_count, so an author with >1000 matches was paged past GitHub's hard ceiling and 422'd with "Only the first 1000 search results are available" — observed as an unexplained SKIP for oshpak. Now capped at 1000 results / 10 pages.

Tests

125 suites / 1224 tests pass. New coverage drives real scenarios through the octokit test hook rather than asserting on mocks: genuine zero still trusted with no retry; timeout-then-success recovers; persistent timeout narrows the window and unions the halves; total failure raises instead of returning zero; self-contradiction raises; under-delivery raises with got 2 of 5; the 1000-cap stops at 10 pages rather than 50.

Type checking verified via npm run build inside the image build. Bare tsc is currently unusable in my working copy — two runs were OOM-killed and a third hung while idle, which looks environmental (this tree is under iCloud sync with hundreds of conflict duplicates), not code-related.

Reviewer notes

• The fix has not yet been confirmed by a live run. The mechanism is proven and the unit tests cover the branches, but no report has run against real GitHub since the behaviour change. Worth knowing before merge.
• Deliberately not included: the cheapest discriminator of all. Every false zero had mergedPRs > 0 && commits === 0, and a merged PR implies commits. That check belongs in report-runner (which knows both numbers) rather than github.ts, and would catch this class regardless of which GitHub field misbehaves. Noted as a follow-up on GLOOK-50.
• The SUSPECT marker is intentionally narrow. If you'd prefer it flag every incomplete_results: true for visibility, that's a one-line change — but the run data says it would be ~83% noise.

🤖 Generated with Claude Code

[msogin]

Automated review — 3 reviewers, run blind to each other

Two persona reviewers (Sr. Distributed-Systems Architect / Correctness, SRE / Operability) plus the standard Smartling fullstack review profile. No fixes have been applied — everything below is for you to accept or reject.

Verification the PR description asked about

The suite passes at 125 suites / 1224 tests, and tsc --noEmit is clean. Your description notes bare tsc was unusable in your working copy (OOM/hang under iCloud sync); it ran fine from a clean checkout, so that does look environmental rather than code-related.

Where the reviewers converged

All three independently landed on github.ts:580, and two reproduced it by driving the real collectCommits through __setOctokitForTest rather than reading it. The total <= SEARCH_RESULT_CAP guard doesn't clamp the reconciliation to the cap — it disables it entirely whenever total_count > 1000. A 1500-commit author with a short page returns 140 hits, no throw, and a log line that isn't even marked SUSPECT. That's the silent-loss class this PR exists to close, still open for exactly the high-volume authors the 1000-cap was added for.

A second, independent blind spot sits three lines earlier at github.ts:567: total is overwritten by every page, so a later page reporting {total_count: 0, incomplete_results: false, items: []} — the shape your own test calls "the one trustworthy zero" — resets the baseline and lets 100-of-250 through silently.

These two share a root cause worth naming, because no single reviewer saw both: the reconciliation baseline is taken from the same untrusted response it exists to validate, and re-read at the wrong moment. Both are line-level fixes, but two blind spots in one guard is an argument for extracting it into a single tested helper.

Two tensions to decide, not two bugs to fix

1. github.ts:620 and 848 vs. 560. The merged-PR and review-count paths both log SUSPECT and then return the bad value anyway — the incident developer had 42 merged PRs, so the PR hardens the commits half of its own headline case. But fixing them adds throw sites, and 560 argues the abort gate already converts a correlated brownout into no report at all (66-member org: 6 skips = degraded, 7 = failed; the incident hit 6). Fixing the silent zeros makes the abort cliff more likely, not less. Worth deciding together. Note 848 is the cheap one — report-runner.ts:203 already catches it into a non-fatal recordError, so it carries no abort risk.

2. github.ts:582 challenges this PR's stated philosophy. "A loud skip is the goal" is right for an empty page. For a page delivering 90 of 100 commits, the reconciliation throws all 90 away — less data than main and more abort pressure. Raised as a question, not a defect, because it's a deliberate design position you took and it may still be the right one for now.

Also worth a look

• search-timeout-recovery.test.ts:118 — clock-dependent; green today, fails in CI around early November 2026 with no code change, and will read as a splitDateWindow regression.
• CLAUDE.md is not updated. Every prior incident in this area has a gotcha entry (GLOOK-13, GLOOK-48, the primary/secondary rate-limit split). This PR introduces a policy a future change is likely to break: incomplete_results + empty is untrustworthy, timed-out-with-items deliberately is not, and an untrustworthy answer must raise rather than return zero. A GLOOK-50 bullet alongside the rate-limit entry would fit the established convention, including the 1000-cap behaviour.
• The accounting policy is now applied inconsistently across the four call sites. collectCommits acts on it; 620, 848 and 871 only log it. Either lift the retry/raise ladder into a shared helper, or comment each unfixed site naming the deliberate gap — otherwise the next reader reasonably assumes logging equals protection.

Credit where due

Several things were checked and found genuinely sound: the trust predicate is correctly narrow (requiring items.length === 0 rather than acting on the advisory flag is right, and the run data supports it); splitDateWindow is DST- and UTC-clean with contiguous non-overlapping halves and no gap at the boundary; the recursion terminates on both guards; union-by-SHA cannot drop a legitimate commit; logSearchAccounting is reachable at all four sites in a real run (and the report-runner.ts:200 change fixes a real pre-existing gap — withRetry in countReviewedPRs had no log, so its rate-limit waits were invisible); and the 1000-cap removes a genuine source of unexplained SKIPs.

Assessment

With fixes. The direction is right and the diagnostics are a real improvement. 580 is a one-line fix in a PR whose entire purpose is closing this exact hole; 567 is a second instance of it. Those two plus a decision on the 620/848-vs-560 tension are the merge-blockers — the rest can follow.

---

Generated with Claude Code — 3 independent reviewers, findings triaged and de-duplicated. No fixes applied.

[msogin]

🔴 Critical — reconciliation is skipped entirely above the cap. (found independently by all three reviewers; two reproduced it)

total <= SEARCH_RESULT_CAP && hits.length < total means that when total_count > 1000 the guard does not run at all — it isn't clamped to the cap, it's disabled.

Reproduced against the real code path via __setOctokitForTest: total_count: 1500, page 1 = 100 items, page 2 = 40 items → loop breaks at line 574 (items.length < SEARCH_PER_PAGE), then 1500 <= 1000 is false so line 580 never fires. searchUserCommits returns 140 hits, no throw, and the log line is not even marked SUSPECT. The identical shape with total_count: 900 correctly throws.

This is the exact silent-loss class the PR exists to close, still open — and open specifically for the highest-volume authors, the population the 1000-cap was added for. It also contradicts this PR's own documentation at search-accounting.test.ts:39-42: "Under-delivery of a NON-empty page is not ignored — it is caught exactly, by reconciling collected hits against total_count."

const expected = Math.min(total, SEARCH_RESULT_CAP);
if (hits.length < expected) {
  throw new Error(
    `GitHub commit search under-delivered for @${user} (${clause}): ` +
    `got ${hits.length} of ${expected}` +
    (total > SEARCH_RESULT_CAP ? ` (capped from ${total})` : ''),
  );
}

[msogin]

Fixed in 23541c0. Confirmed by reading it back — you're right, and it's the worst possible place for me to have made this mistake: a PR whose entire premise is "an untrustworthy zero must never pass silently", with the guard switched off for the highest-volume authors.

Now Math.min(counted, SEARCH_RESULT_CAP), clamped rather than gated, with your capped from detail in the message.

Your reproduction is now a test — reconciles ABOVE the 1000 cap instead of switching the check off — asserting the 1500/100+40 shape yields {expected: 1000, collected: 140} and a detail matching capped from 1500. It fails against the code you reviewed.

One deviation from your snippet: it no longer throws in this case. 140 real commits are kept and reported as a shortfall, for the reasons on your github.ts:582 thread.

[msogin]

🟡 Important — the reconciliation baseline is overwritten by every page.

total is reassigned on each iteration, so line 580 compares against whatever the last page reported, not the promise page 1 made.

Reproduced: page 1 = {total_count: 250, items: 100}, page 2 = {total_count: 0, incomplete_results: false, items: []}. That second shape is precisely what search-accounting.test.ts:12 blesses as "the one trustworthy zero", so isSuspectSearchResult correctly returns false. Then total becomes 0, hits.length(100) >= Math.min(0, 1000) breaks the loop, and line 580 evaluates 100 < 0 → false.

100 of 250 commits returned. No throw, no SUSPECT, nothing in the log. Same accounting failure this PR is fixing, one page later.

Together with the finding at line 580, the root cause is shared and worth naming: the reconciliation baseline is derived from the same untrusted response it is meant to validate, and re-read at the wrong time. Two independent blind spots in one guard suggests extracting this into a single tested helper rather than inline arithmetic.

if (page === 1) total = data.total_count ?? 0;
else total = Math.max(total, data.total_count ?? 0);

[msogin]

Fixed in 23541c0, and thank you for naming the shared root cause — that framing is what made me restructure rather than patch.

The baseline is now taken once from page 1 and only ever revised upward:

let expectedTotal: number | null = null;
...
expectedTotal = expectedTotal === null ? pageTotal : Math.max(expectedTotal, pageTotal);

I took your Math.max rather than a strict page-1 read, so a later page reporting a higher count still widens the promise instead of being ignored.

The sting in your reproduction is that the resetting page is the exact shape my own test blesses as "the one trustworthy zero" — the predicate is right and the arithmetic around it was wrong. That's precisely the "baseline derived from the response it is meant to validate" problem you described, and it's why I didn't just swap the comparison.

On extracting a helper: I've left the reconciliation inline but reduced it to a single decision point (clamp → compare → keep-or-raise) with the cap arithmetic in one expression, and the three shapes are now pinned by tests. Happy to extract it if you still think the inline form invites the next blind spot.

[msogin]

🟡 data.items is iterated unguarded, but the type and the tests both say it may be absent.

SearchAccounting.items is declared optional, and search-accounting.test.ts:69-70 deliberately asserts isSuspectSearchResult({}) and isSuspectSearchResult({ items: [] }) are not suspect ("treats missing fields as not-suspect rather than inventing a failure").

So a response missing items passes the trust gate by design and then dies here with TypeError: data.items is not iterable — converting a shape the tests explicitly tolerate into a hard SKIP. Lines 573 and 574 have the same gap.

const items: any[] = data.items ?? [];
for (const item of items) hits.push(toRawHit(item, user));
...
if (hits.length >= Math.min(total, SEARCH_RESULT_CAP)) break;
if (items.length < SEARCH_PER_PAGE) break;

[msogin]

Fixed in 23541c0 — const items = data.items ?? [];, used for the iteration and both break conditions.

You identified a genuine internal contradiction: the predicate deliberately tolerates a missing items (and has a test saying so), and the very next thing the code did was iterate it raw. Tolerating a shape and then crashing on it is worse than rejecting it outright, because the crash converts it into a hard SKIP — the outcome this PR is trying to avoid.

Added does not TypeError on a shape the trust predicate tolerates, which drives {total_count: 0, incomplete_results: false} with no items key through the real collection loop.

[msogin]

🟡 The 1000-cap replaces a loud failure with a silent one.

Before this PR, a 1500-commit author paged to page 11 and got 422 Only the first 1000 search results are available → an unexplained but visible SKIP. Now the walk stops at 1000 and the developer is recorded with 1000 commits, with nothing anywhere saying the number is truncated.

Capping the pagination is right — the 422 was a genuine operational wart, and removing it is a real win. But given this PR's premise that untrustworthy data must always leave a signal, the truncation deserves one:

if (total > SEARCH_RESULT_CAP) {
  log?.(`[search] commits @${user} ${clause}: total_count=${total} exceeds the ${SEARCH_RESULT_CAP} cap — results truncated`);
}

[msogin]

Fixed in 23541c0. Fair hit — I replaced a loud failure with a quiet one and called it a win in the commit message.

total_count above the cap now emits:

[search] commits @user <clause>: total_count=1500 exceeds the 1000-result cap — this developer's commits are truncated

It also feeds the shortfall now, so truncation reaches run_metadata.errors rather than only the log: with the reconciliation clamped to the cap, a 1500-count author who yields fewer than 1000 hits is reported as got N of 1000 (capped from 1500).

Worth noting the truncation itself is unavoidable — 1000 is GitHub's hard ceiling, and the honest options are "truncate with a signal" or "refuse the developer entirely". The signal is the right half of that trade.

[msogin]

🟡 Important — this logs SUSPECT and the next line then executes the bug. (all three reviewers)

Line 621 is if (page === 1 && res.data.total_count === 0) return [];. Feed it {total_count: 0, items: [], incomplete_results: true} — the exact 2026-09-08 payload — and it emits

[search] merged-prs @dev page=1 items=0 total_count=0 incomplete_results=true SUSPECT

…and then returns []. No retry, no narrowing, no throw.

This matters because the incident in the PR description is "43 commits, 42 merged PRs, recorded as 0" — this PR hardens the commits half only. The two searches are independent calls, so commits can succeed while the merged-PR search times out; that developer then lands in the report with correct commits and totalPRs = 0, silently, and prPercentage (an impact-score input) is wrong.

The PR adds the diagnostic that proves the failure and then declines to act on it.

if (page === 1 && isSuspectSearchResult(res.data)) {
  throw new Error(`GitHub merged-PR search gave no trustworthy result for @${user}`);
}
if (page === 1 && res.data.total_count === 0) return [];

Note this function also lacks the 1000-cap added to commits (line 632 will page past page 10 into the same 422), worth fixing in the same pass. See also the abort-pressure caveat noted at line 560 — these two interact.

[msogin]

Fixed in 23541c0 — and this was the most uncomfortable finding, because you're right that the PR added the diagnostic that proves the failure and then declined to act on it. "43 commits, 42 merged PRs, recorded as 0" is the headline case and I hardened one half of it.

Now raises on isSuspectSearchResult at any page, before the total_count === 0 early return can swallow it. Also added the 1000-result cap you flagged at line 632, so this path can't page into the same 422.

On the abort-pressure interaction with 560: I resolved it structurally rather than by trading the two off, which I think dissolves the tension. Partial data no longer raises at all — it's kept and reported through IntegrityError (see the 582 thread). So the new throw sites fire only when a search yields nothing trustworthy, which is the case that genuinely warrants a SKIP. The additional abort pressure is therefore limited to members we'd otherwise have reported as flatly wrong.

I also took your second suggestion from 560: the abort message now names search timeouts when they dominate, instead of always blaming auth.

[msogin]

🟣 Design question — collectCommits has only one channel for degradation, so partial success is discarded wholesale.

This challenges the PR's stated position ("a loud skip is the goal, not a workaround"), so raising it as a question rather than a defect — but it is backed by two probes against the real code path:

• {total_count: 100, incomplete_results: true, items: [90 commits]} → correctly not flagged suspect (isUntrustworthyEmpty requires an empty page). The loop collects 90 real commits, and line 581 then throws all 90 away: under-delivered for @devx: got 90 of 100. The developer disappears entirely and moves the run 1/7th of the way toward the abort at line 560. Old behaviour: 90 commits recorded, 10% low.
• Left half healthy (40 commits), right half dead → the depth-2 throw propagates through both parents, 11 calls burned, 40 known-good commits discarded.

For an empty page, throwing is clearly right — that's the GLOOK-50 fix and it's correct. For 90-of-100 it appears strictly worse than the old behaviour on both axes: less data and more abort pressure.

The channel for this already exists but is unreachable from here: IntegrityError is documented in report-runner/types.ts:19 as "a member-kept partial-data condition, not a SKIP", and the runner already uses it for openPRs and unmerged-commit-detail. Commit search can't reach it because fetchUserActivity returns UserActivity with no completeness field.

Flagging this as structural rather than a line fix deliberately: the obvious patch (if (hits.length === 0 && total > 0) throw) would make 90-of-100 silently succeed with no signal anywhere — GLOOK-50 in miniature, and exactly what this PR set out to prevent. Resolving it properly means UserActivity carrying { complete, expected, collected } and the runner gaining a third outcome between "trusted" and "skipped".

Is the all-or-nothing channel a deliberate call for now, with partial-data reporting as a follow-up? Reasonable either way — but worth being explicit, since the 90-of-100 case is a behaviour regression against main.

[msogin]

Fixed in 23541c0 — you called it correctly, and I'm glad you raised it as a question rather than letting me ship the position.

You were right that 90-of-100 was a regression against main on both axes, and right that the obvious patch would have recreated GLOOK-50 in miniature. So I built the channel you described rather than either.

UserActivity now carries an optional SearchShortfall {expected, collected, detail}. collectCommits raises only when hits.length === 0; partial-but-real data is returned with the shortfall attached, and report-runner keeps the commits and records integrity.recordError({context: 'other', login, message}) — the member-kept partial-data channel already used for openPRs. Split halves merge their shortfalls.

So the three outcomes are now distinct, which is what was missing:

outcome	data	signal
trusted	kept	none
partial	kept	run_metadata.errors, no abort pressure
nothing trustworthy	none	counted SKIP

"A loud skip is the goal" was over-stated as a universal, and I've narrowed it in the code comments and CLAUDE.md to what it was actually about: an empty untrustworthy page. For a page that delivered real commits, the goal is a loud record, not a skip.

Worth flagging one consequence for you to sanity-check: context: 'other' is the closest existing IntegrityError context. A dedicated 'commit-search-shortfall' would read better in run_metadata, but it's a union type change touching the badge, so I left it. Say the word if you'd prefer it in this PR.

[msogin]

🔵 Four unconditional log lines per member will evict SUSPECT from the operator's view.

log is addLog(reportId, …) + console.log, and addLog (src/lib/progress-store.ts:49) truncates to the last 200 lines.

This adds 4 lines per member (commits, merged-prs, reviewed-prs, open-prs) — 264 lines for a 66-member org, 408 for 102. The run log already exceeds 200 lines, so the ring turns over roughly twice as fast: a SUSPECT marker for an early member is guaranteed gone from the progress view by the time the run ends.

console.log still has it, so this isn't a blocker — but it partly defeats the stated diagnostic goal in the surface an operator actually opens first.

Suggestion: keep console.log unconditional, but only push to addLog when suspect is true or items !== total — or emit one compact per-member summary instead of four per-page lines.

[msogin]

Fixed in 23541c0. Good catch on the ring-buffer arithmetic — I'd added the diagnostic and then made it evict itself, which is a neat own goal.

Took your first suggestion: logSearchAccounting now sends a line to log() only when it's interesting (suspect, or items !== total_count), and routine pages go to console.log only. So the progress view keeps SUSPECT and shortfall lines, app.log/stdout keeps everything for the forensic query, and a healthy 66-member run adds roughly zero lines to the 200-line ring instead of 264.

Chose that over the compact per-member summary because the per-page granularity is what made the incident diagnosable — the page number and per-page items/total_count are exactly what distinguished the timeout shape from the contradiction shape.

[msogin]

🔵 The pre-existing docblock above this header is now orphaned.

Lines 364-368 ("Search all commits by a user in an org…") used to sit directly above searchUserCommits. That function is now at line 590, so the docblock describes a section header instead. Worth moving down to the exported function.

[msogin]

Fixed in 23541c0 — the docblock is reattached to searchUserCommits, and extended to describe the new return shape (hits plus optional shortfall).

[msogin]

🟡 Important — this test is clock-dependent and will start failing in CI around early November 2026.

jest.useFakeTimers({ doNotFake: ['nextTick'] }) fakes Date but initialises it to the real wall clock, and collectCommits calls splitDateWindow(from, to) with the default now = new Date(). The window is SINCE = 2026-08-26 → now, so the midpoint these two assertions hard-code moves with the calendar:

now	span	midpoint	line 118	line 119
2026-09-06	11d	2026-08-31	❌	✅
2026-09-08 (today)	13d	2026-09-01	✅	✅
2026-11-04	70d	2026-09-30	✅	❌ (>=2026-10-01)
2027-01-15	—	—	❌	❌

Green today, red later with no code change — and the failure will look like a regression in splitDateWindow rather than a test-clock problem, which is the expensive kind of red.

One-line fix, already supported by the existing setup:

jest.useFakeTimers({ doNotFake: ['nextTick'], now: new Date('2026-09-09T00:00:00Z') });

Better still, thread an optional now through collectCommits → splitDateWindow so production splitting is deterministic and injectable rather than reading the ambient clock mid-recursion.

[msogin]

Fixed in 23541c0 — now: new Date('2026-09-09T00:00:00Z') on the fake timers, with a comment explaining why the clock is pinned.

Your table is the convincing part: green today, red in November, and failing in a way that reads as a splitDateWindow regression rather than a test-clock problem. That's the expensive kind of red, and I'd have been the one debugging it.

On threading now through collectCommits → splitDateWindow: splitDateWindow already takes an injectable now (defaulted), which is what made pinning the fake clock sufficient. I've left the production call reading the ambient clock, since the only consumer is the split midpoint and a wrong midpoint is harmless — both halves are still contiguous and cover the full window. Happy to thread it if you'd rather have no ambient-clock read in the recursion at all.

[msogin]

🔵 This case is byte-identical to the one at line 25.

'flags a timed-out query that returned NOTHING' (47-51) and 'flags a timed-out query that returned nothing' (25-30) use the same input {total_count: 0, incomplete_results: true, items: []} and the same expectation. 2 of the 8 cases here are the same case, so the suite reads as broader than it is — worth deleting one and keeping the better comment.

Separately, and more substantively: the union dedup is never actually exercised. search-timeout-recovery.test.ts:100 generates a distinct sha per call (h${call}), so right.filter((h) => !seen.has(h.sha)) never filters anything, despite that test being titled "unions the halves". Worth a split case where both halves return a commit with the same sha, asserting the result has length 1 — that's the line most likely to break silently under a future refactor.

[msogin]

Both fixed in 23541c0.

Duplicate removed — you were right that they were byte-identical inputs and expectations. Kept the one with the better comment.

The dedup gap was the more valuable half of this. You're right that h${call} minted a unique sha per call, so right.filter(h => !seen.has(h.sha)) never filtered anything and the test named "unions the halves" proved only concatenation. Added dedupes a commit that appears in both halves: both ranged queries return a commit with the same sha (a realistic boundary commit), asserting the result is ['shared'] — length 1, not 2.

That test fails if the filter is removed, which the previous one did not.

[msogin]

Review addressed — 23541c0

This was a good review, and the two critical findings were both mine, both in the same three lines, and both the exact silent-loss class this PR exists to close. Worth stating plainly: I shipped a guard with the check disabled above the 1000-result cap, for precisely the high-volume authors the cap was added for.

finding	outcome
🔴 580 reconciliation skipped above the cap	Fixed — clamped, not gated
🟡 567 baseline overwritten each page	Fixed — page-1 baseline, revised upward only
🟡 568 items iterated unguarded	Fixed — ?? []
🟡 573 cap truncation now silent	Fixed — logged and fed into the shortfall
🟡 620 merged-PR logs SUSPECT then returns it	Fixed — raises, plus the missing 1000-cap
🟡 848 review count logs SUSPECT then returns it	Fixed — raises (no abort risk, as you showed)
🟡 560 abort message misdiagnoses brownouts	Fixed (option 2); in-loop short-circuit deferred
🟣 582 partial success discarded wholesale	Fixed structurally — see below
🔵 552 retry × split fan-out unbounded	Deferred to GLOOK-50, per your consensus
🔵 437 four log lines evict SUSPECT from the ring	Fixed — routine lines to stdout only
🔵 369 orphaned docblock	Fixed
🟡 test 118 clock-dependent	Fixed — fake clock pinned
🔵 test 47 duplicate case + dedup never exercised	Both fixed

The structural change

582 was the finding that changed the design rather than a line. You were right that discarding 90 of 100 real commits was worse than main on both axes, and right that the obvious patch would have recreated GLOOK-50 in miniature.

UserActivity now carries an optional SearchShortfall. There are three outcomes instead of two:

outcome	data	signal
trusted	kept	none
partial	kept	run_metadata.errors via recordError — no abort pressure
nothing trustworthy	none	counted SKIP

This also dissolves the 620/848-vs-560 tension you asked me to decide as a trade-off. I didn't trade them: since partial degradation no longer produces SKIPs, the new throw sites fire only when a search yields nothing, so the added abort pressure is confined to members who'd otherwise have been reported as flatly wrong. Your 66-member cliff is still real, but it's no longer reachable by partial degradation — which was the likely path to it.

I've also narrowed this PR's own slogan. "A loud skip is the goal" was over-stated as a universal; it was only ever about an empty untrustworthy page. For a page that delivered real commits the goal is a loud record. Corrected in the code comments and CLAUDE.md.

Two questions back to you

1. context: 'other' is the closest existing IntegrityError context for a shortfall. A dedicated 'commit-search-shortfall' would read far better in run_metadata, but it's a union-type change that touches IntegrityBadge. Want it in this PR?
2. Extracting the reconciliation into a helper — I reduced it to one decision point (clamp → compare → keep-or-raise) with the three shapes pinned by tests, but left it inline. Your "two blind spots in one guard" argument may still apply.

Verification

125 suites / 1226 tests pass; npm run build clean (type-check included). Thanks for confirming tsc --noEmit is clean from a fresh checkout — that settles the environmental question on my side.

CLAUDE.md now carries a GLOOK-50 gotcha alongside the rate-limit entry, per the convention you pointed at.

Still true and worth repeating before merge: no report has run against real GitHub since the behaviour change. The mechanism is proven and the branches are tested, but the end-to-end confirmation is outstanding.