fix: address gemini-code-assist feedback on #2133

HIGH — open-sse/services/accountFallback.ts
  ProviderProfile type was missing useUpstream429BreakerHints, and the
  buildProviderProfile helper was not propagating the stored override.
  Result: resolvedProfile.useUpstream429BreakerHints was always undefined,
  so the circuit breaker silently ignored every user override and always
  fell back to the per-provider default — making the new UI toggle a no-op.

  Fix: add the optional field to ProviderProfile, populate it from
  resilience.connectionCooldown[category].useUpstream429BreakerHints in
  buildProviderProfile, and drop the now-unnecessary type cast at the
  configureProviderBreaker call site.

MEDIUM — src/shared/utils/classify429.ts (2 call sites)
  classify429FromError was casting response.headers / err.headers directly
  to Record<string, string>. That breaks when the upstream uses a native
  fetch Headers instance, because Headers does not respond to
  Object.entries (used downstream in getHeader). On those errors the
  classifier would silently see no headers and never produce a kind.

  Fix: add a normalizeHeaders(raw) helper that detects a Headers-like
  .entries() method and converts via Object.fromEntries, falling back to
  the previous plain-object treatment. Use it at both call sites.

All 39 existing tests still pass.
This commit is contained in:
Hernan Inverso
2026-05-10 16:35:45 -03:00
parent f7279b3e19
commit 4aea2c8b30
2 changed files with 29 additions and 4 deletions

View File

@@ -32,6 +32,8 @@ import { resolveUseUpstream429BreakerHints } from "../../src/shared/utils/provid
type ProviderProfile = {
baseCooldownMs: number;
useUpstreamRetryHints: boolean;
/** Issue #2100 follow-up. Stored override; undefined → per-provider default. */
useUpstream429BreakerHints?: boolean;
maxBackoffSteps: number;
failureThreshold: number;
resetTimeoutMs: number;
@@ -188,6 +190,9 @@ function buildProviderProfile(
return {
baseCooldownMs: connectionCooldown.baseCooldownMs,
useUpstreamRetryHints: connectionCooldown.useUpstreamRetryHints,
// Issue #2100 follow-up: propagate stored override (boolean | undefined)
// so the runtime resolver picks user setting first, then per-provider default.
useUpstream429BreakerHints: connectionCooldown.useUpstream429BreakerHints,
maxBackoffSteps: connectionCooldown.maxBackoffSteps,
failureThreshold: providerBreaker.failureThreshold,
resetTimeoutMs: providerBreaker.resetTimeoutMs,
@@ -541,8 +546,7 @@ function configureProviderBreaker(
// Issue #2100 follow-up: resolve useUpstream429BreakerHints from the
// provider profile (stored override) or fall back to per-provider default.
// Stored value type is `boolean | undefined` — never `null` after PATCH.
const userValue = (resolvedProfile as { useUpstream429BreakerHints?: boolean })
.useUpstream429BreakerHints;
const userValue = resolvedProfile.useUpstream429BreakerHints;
const useHints = resolveUseUpstream429BreakerHints(provider, userValue);
return getCircuitBreaker(provider, {
failureThreshold: resolvedProfile.failureThreshold ?? resolvedProfile.circuitBreakerThreshold,

View File

@@ -163,6 +163,27 @@ export function retryAfterFromResponse(response: {
return parseRetryAfter(getHeader(response.headers, "retry-after"));
}
/**
* Normalize an unknown headers-like value into a plain `Record<string, string>`.
* Native `Headers` (from `fetch`) does NOT respond to `Object.entries` — it
* exposes `.entries()` instead. Without this normalization, `getHeader` would
* silently miss every header on a Headers instance.
*/
function normalizeHeaders(raw: unknown): Record<string, string> | undefined {
if (raw === null || typeof raw !== "object") return undefined;
const maybeIter = (raw as { entries?: unknown }).entries;
if (typeof maybeIter === "function") {
try {
return Object.fromEntries(
(raw as { entries: () => Iterable<[string, string]> }).entries(),
);
} catch {
// fall through to plain-object treatment
}
}
return raw as Record<string, string>;
}
/**
* Adapter that takes an error thrown by an HTTP client (fetch wrapper, axios,
* upstream SDK, etc.) and produces a {@link FailureKind} suitable for the
@@ -199,7 +220,7 @@ export function classify429FromError(err: unknown): FailureKind | undefined {
status = resp.status;
}
if (resp.headers && typeof resp.headers === "object") {
headers = resp.headers as Record<string, string>;
headers = normalizeHeaders(resp.headers);
}
if (resp.data !== undefined) {
body = resp.data;
@@ -209,7 +230,7 @@ export function classify429FromError(err: unknown): FailureKind | undefined {
}
if (headers === undefined && e.headers && typeof e.headers === "object") {
headers = e.headers as Record<string, string>;
headers = normalizeHeaders(e.headers);
}
if (body === undefined) {
if (typeof e.body !== "undefined") {