feat(selfhost): add a per-analyzer circuit breaker to review-enrichment - #2632
Conversation
Every analyzer that depends on a third-party HTTP API (osv.dev, bundlephobia, endoflife.date, npm/pypi registries, deps.dev) re-attempts a currently-unhealthy endpoint from a cold state on every request -- even seconds after an identical call just timed out or errored. Adds a circuit breaker to the shared boundedFetchText/boundedFetchStatus chokepoint (boundedFetchJson inherits it via boundedFetchText), keyed by endpointCategory: after 3 consecutive remote-health failures within a window, further calls are skipped for a 30s cooldown instead of paying the real network cost. Only failures that actually indicate the remote is unhealthy count toward the breaker (timeout, network_error, and http_error with status 403/429/5xx) -- a plain 404 does not, since analyzers like typosquat.ts call boundedFetchStatus for candidate package names where a 404 is a legitimate negative result, not a service-health signal. A circuit-open skip returns the same BoundedFetchFailure shape every other failure path already returns (never throws), so the service's existing fail-safe/non-blocking contract with the main review is unaffected -- it surfaces through the same diagnostics pipeline every other failure reason already uses. Evaluated parallelizing analyzer cost-class execution (currently strictly sequential in brief.ts) per the issue's second ask; deferring it as a scoped follow-up rather than implementing here, since it would change the effective total concurrency profile against third-party rate limits in a way that hasn't been validated against real traffic.
|
Tip 🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩 ✅ Gittensory review result - approve/merge recommendedReview updated: 2026-07-02 20:34:52 UTC
✅ Suggested Action - Approve/Merge
Review summary Nits — 6 non-blocking
Review context
Contributor next steps
Signal definitions
🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed 💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.
|
The AI review flagged a real defect: cachedFetchLivePullRequestMergeState
/ cachedFetchLivePullRequestState / cachedFetchLivePullRequestHeadSha
each wrote through only the ONE field they cared about, but all three
share a single prStateFetchedAt freshness stamp. A live fetch from any
one of them would make the OTHER two fields look "fresh" to a
subsequent reader despite never having been fetched, so that reader
would silently return undefined for a field that was simply never
populated -- indistinguishable from a confirmed-empty GitHub value.
All three narrow live-fetchers already hit the exact same GET
/pulls/{n} endpoint, just extracting one field each, so there's no
extra API cost to fixing this: a new internal fetchAndCachePrStateFields
helper fetches the full payload once and writes mergeable_state, state,
and headSha through together (headSha omitted, not nulled, when absent,
preserving the existing PARTIAL-UPDATE CONTRACT so a prior headSha the
files cache depends on is never cleared). It also only writes when the
fetch actually succeeds, so a transient failure no longer poisons the
cache with a false "confirmed fresh" stamp the way the old per-field
writes did. The three public, uncached narrow fetchers used by the
act-boundary/gate-override callers are untouched.
Also fixes a second, separate flagged issue: `{ token: "installation-token" }`
in six of this PR's own new test fixtures tripped the deterministic
generic_secret_assignment scanner (a keyword-shaped heuristic, not a
real credential format) -- renamed to `"fake-installation-token"`,
which the scanner's own placeholder-value allowlist already recognizes,
without touching the shared scanner itself.
Rebased onto current main (renumbered migration 0093->0094 to resolve
a collision with #2616, and again to catch up with #2632). Full local
gate (test:ci, npm audit) green; no other changes.
The AI review flagged a real defect: cachedFetchLivePullRequestMergeState
/ cachedFetchLivePullRequestState / cachedFetchLivePullRequestHeadSha
each wrote through only the ONE field they cared about, but all three
share a single prStateFetchedAt freshness stamp. A live fetch from any
one of them would make the OTHER two fields look "fresh" to a
subsequent reader despite never having been fetched, so that reader
would silently return undefined for a field that was simply never
populated -- indistinguishable from a confirmed-empty GitHub value.
All three narrow live-fetchers already hit the exact same GET
/pulls/{n} endpoint, just extracting one field each, so there's no
extra API cost to fixing this: a new internal fetchAndCachePrStateFields
helper fetches the full payload once and writes mergeable_state, state,
and headSha through together (headSha omitted, not nulled, when absent,
preserving the existing PARTIAL-UPDATE CONTRACT so a prior headSha the
files cache depends on is never cleared). It also only writes when the
fetch actually succeeds, so a transient failure no longer poisons the
cache with a false "confirmed fresh" stamp the way the old per-field
writes did. The three public, uncached narrow fetchers used by the
act-boundary/gate-override callers are untouched.
Also fixes a second, separate flagged issue: `{ token: "installation-token" }`
in six of this PR's own new test fixtures tripped the deterministic
generic_secret_assignment scanner (a keyword-shaped heuristic, not a
real credential format) -- renamed to `"fake-installation-token"`,
which the scanner's own placeholder-value allowlist already recognizes,
without touching the shared scanner itself.
Rebased onto current main (renumbered migration 0093->0094 to resolve
a collision with #2616, and again to catch up with #2632). Full local
gate (test:ci, npm audit) green; no other changes.
The AI review flagged a real defect: cachedFetchLivePullRequestMergeState
/ cachedFetchLivePullRequestState / cachedFetchLivePullRequestHeadSha
each wrote through only the ONE field they cared about, but all three
share a single prStateFetchedAt freshness stamp. A live fetch from any
one of them would make the OTHER two fields look "fresh" to a
subsequent reader despite never having been fetched, so that reader
would silently return undefined for a field that was simply never
populated -- indistinguishable from a confirmed-empty GitHub value.
All three narrow live-fetchers already hit the exact same GET
/pulls/{n} endpoint, just extracting one field each, so there's no
extra API cost to fixing this: a new internal fetchAndCachePrStateFields
helper fetches the full payload once and writes mergeable_state, state,
and headSha through together (headSha omitted, not nulled, when absent,
preserving the existing PARTIAL-UPDATE CONTRACT so a prior headSha the
files cache depends on is never cleared). It also only writes when the
fetch actually succeeds, so a transient failure no longer poisons the
cache with a false "confirmed fresh" stamp the way the old per-field
writes did. The three public, uncached narrow fetchers used by the
act-boundary/gate-override callers are untouched.
Also fixes a second, separate flagged issue: `{ token: "installation-token" }`
in six of this PR's own new test fixtures tripped the deterministic
generic_secret_assignment scanner (a keyword-shaped heuristic, not a
real credential format) -- renamed to `"fake-installation-token"`,
which the scanner's own placeholder-value allowlist already recognizes,
without touching the shared scanner itself.
Rebased onto current main (renumbered migration 0093->0094 to resolve
a collision with #2616, and again to catch up with #2632). Full local
gate (test:ci, npm audit) green; no other changes.
Advances #2541
Problem
The review-enrichment service's analyzer pipeline is correctly fail-safe with respect to the main review — an enrichment timeout degrades review quality but never blocks the review itself. However, within the service, individual analyzers that depend on a third-party HTTP API (osv.dev, bundlephobia, endoflife.date, npm/pypi registries, deps.dev) have no memory of recent failures: every request re-attempts a currently-unhealthy endpoint from a cold state, even seconds after an identical call just timed out or errored.
Changes
review-enrichment/src/external-fetch.ts, wired into the sharedboundedFetchText/boundedFetchStatuschokepoint (boundedFetchJsoninherits it automatically via its internalboundedFetchTextcall). Keyed byendpointCategory. After 3 consecutive remote-health failures, further calls for that category are skipped for a 30s cooldown — no real HTTP cost — instead of retrying a known-broken endpoint on every request.isRemoteHealthFailure): onlytimeout,network_error, andhttp_errorwith status 403/429/5xx trip the breaker. A plain 404 does not —typosquat.tscallsboundedFetchStatusfor many candidate package names where a 404 just means "this candidate doesn't exist," a legitimate negative result, not a sign the remote service is unhealthy.aborted(always caller-driven) andresponse_too_large/invalid_json(the remote did respond) are excluded too.BoundedFetchFailureshape every other failure path already returns (never throws) and flows through the existingattachDiagnosticspipeline, so the service's fail-safe/non-blocking contract with the main review is unchanged — no new instrumentation needed.Cost-class parallelization (issue's second ask)
Evaluated parallelizing analyzer cost-class execution (
review-enrichment/src/brief.tscurrently drains each of the 5COST_ORDERclasses — local, registry, github-light, github-heavy, tooling — strictly sequentially before starting the next) and am deferring it as a follow-up rather than implementing it here:Recommend filing a scoped follow-up issue to trial cost-class parallelization behind a flag/profile variant rather than as a blanket change.
Test plan
npm run build(review-enrichment) cleannpm run test(review-enrichment: build + sourcemap validation + analyzer-metadata check + full node:test suite) — 402/402 passingnpm run typecheckcleannpm audit --audit-level=moderate— 0 vulnerabilitiesboundedFetchStatus/boundedFetchText/boundedFetchJsonshare one circuit perendpointCategory; two different categories never cross-contaminate; reset helper clears state between tests