diff --git a/src/review/enrichment-wire.ts b/src/review/enrichment-wire.ts index 0af1d5f0bf..103d801b1f 100644 --- a/src/review/enrichment-wire.ts +++ b/src/review/enrichment-wire.ts @@ -53,6 +53,21 @@ function sharedSecretWasNormalized( return (normalized ?? "") !== raw; } +// Set true once the startup probe confirms REES rejects the shared secret (401/403). Once set, +// buildReviewEnrichment skips every /v1/enrich call for the rest of this process's lifetime instead of +// repeating a call that's confirmed to fail on every PR review, each one logging review_context_fetch_failed. +// Cleared only by a process restart -- exactly the action fixing the secret mismatch already requires. +let reesAuthRejected = false; +let reesAuthRejectedSkipLoggedCount = 0; +const MAX_REES_AUTH_REJECTED_SKIP_LOGS = 3; + +/** Test-only: this module-level circuit-breaker state otherwise persists for the life of the process (by + * design), which would leak across unrelated test cases sharing this module instance within one test file. */ +export function resetReesAuthRejectedForTests(): void { + reesAuthRejected = false; + reesAuthRejectedSkipLoggedCount = 0; +} + /** * Fire-and-forget startup probe that POSTs to REES /v1/ping to verify the shared secret matches. * Logs rees_ping_ok on success, or rees_secret_mismatch / rees_secret_missing / rees_ping_error on @@ -109,13 +124,14 @@ export function probeReesSecretAtStartup(env: Env): void { ); } else { const isAuthError = response.status === 401 || response.status === 403; + if (isAuthError) reesAuthRejected = true; console.error( JSON.stringify({ level: "error", event: isAuthError ? "rees_secret_mismatch" : "rees_ping_error", status: response.status, message: isAuthError - ? `REES /v1/ping rejected the bearer token (${response.status}). The REES_SHARED_SECRET on this engine does not match the REES_SHARED_SECRET on the REES service. All /v1/enrich calls will fail until both are set to the same bare string.` + ? `REES /v1/ping rejected the bearer token (${response.status}). The REES_SHARED_SECRET on this engine does not match the REES_SHARED_SECRET on the REES service. All /v1/enrich calls are disabled for this process lifetime -- restart the engine after fixing both secrets.` : `REES /v1/ping returned an unexpected status (${response.status}). Check the REES service logs.`, }), ); @@ -370,6 +386,23 @@ export async function buildReviewEnrichment( const cfg = reesConfig(env); const base = cfg.REES_URL?.trim(); if (!base) return undefined; + if (reesAuthRejected) { + // The startup probe already confirmed REES rejects this secret -- skip the call rather than repeat a + // guaranteed 401/403 on every single PR review. Cap the log volume; the operator already got the loud + // rees_secret_mismatch error at startup, this is just a reminder the skip is still active. + if (reesAuthRejectedSkipLoggedCount < MAX_REES_AUTH_REJECTED_SKIP_LOGS) { + reesAuthRejectedSkipLoggedCount += 1; + console.warn( + JSON.stringify({ + level: "warn", + event: "rees_enrich_skipped_auth_rejected", + message: + "Skipping REES /v1/enrich call: startup probe confirmed the shared secret is rejected. Fix REES_SHARED_SECRET on both the engine and the REES service, then restart the engine.", + }), + ); + } + return undefined; + } const sharedSecret = normalizeSharedSecret(cfg.REES_SHARED_SECRET); const authConfigured = Boolean(sharedSecret); const authSecretNormalized = sharedSecretWasNormalized( diff --git a/test/unit/enrichment-wire.test.ts b/test/unit/enrichment-wire.test.ts index fa0e48c69e..0aad2c1b66 100644 --- a/test/unit/enrichment-wire.test.ts +++ b/test/unit/enrichment-wire.test.ts @@ -4,6 +4,7 @@ import { buildReviewEnrichment, isReesGithubTokenForwardingEnabled, probeReesSecretAtStartup, + resetReesAuthRejectedForTests, resolveReesAnalyzers, resolveReesAnalyzerBudgetMs, resolveReesProfile, @@ -65,6 +66,10 @@ describe("probeReesSecretAtStartup", () => { }); afterEach(() => { globalThis.fetch = realFetch; + // The 401/403 test below sets the module-level reesAuthRejected circuit-breaker flag (#3738) for real -- + // reset it so it can never leak into a LATER test in this file (including buildReviewEnrichment's own + // suite, which shares this same module instance via the static import above). + resetReesAuthRejectedForTests(); }); const flush = () => new Promise((resolve) => setTimeout(resolve, 0)); @@ -620,6 +625,85 @@ describe("buildReviewEnrichment", () => { }); }); +describe("REGRESSION (#3738): REES auth-rejected circuit breaker", () => { + let realFetch: typeof fetch; + beforeEach(() => { + realFetch = globalThis.fetch; + }); + afterEach(() => { + globalThis.fetch = realFetch; + resetReesAuthRejectedForTests(); + }); + + const flush = () => new Promise((resolve) => setTimeout(resolve, 0)); + + it("buildReviewEnrichment proceeds normally by default (circuit breaker starts closed)", async () => { + const fetchSpy = vi.fn(async () => ({ ok: true, json: async () => ({ promptSection: "brief" }) }) as unknown as Response); + globalThis.fetch = fetchSpy as unknown as typeof fetch; + const r = await buildReviewEnrichment(env({ REES_URL: "https://r" }), input); + expect(r?.promptSection).toBe("brief"); + expect(fetchSpy).toHaveBeenCalledTimes(1); + }); + + it("once the startup probe confirms a 401, buildReviewEnrichment skips every /v1/enrich call without fetching, for the rest of the process", async () => { + const fetchSpy = vi.fn(async () => ({ ok: false, status: 401 }) as Response); + globalThis.fetch = fetchSpy as unknown as typeof fetch; + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + const errSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + try { + probeReesSecretAtStartup(env({ REES_URL: "https://rees.example", REES_SHARED_SECRET: "s3cret" })); + await flush(); + fetchSpy.mockClear(); + + const r1 = await buildReviewEnrichment(env({ REES_URL: "https://rees.example", REES_SHARED_SECRET: "s3cret" }), input); + const r2 = await buildReviewEnrichment(env({ REES_URL: "https://rees.example", REES_SHARED_SECRET: "s3cret" }), input); + expect(r1).toBeUndefined(); + expect(r2).toBeUndefined(); + expect(fetchSpy).not.toHaveBeenCalled(); // never even attempted the doomed call + expect( + warnSpy.mock.calls.some((c) => JSON.parse(c[0] as string).event === "rees_enrich_skipped_auth_rejected"), + ).toBe(true); + } finally { + warnSpy.mockRestore(); + errSpy.mockRestore(); + } + }); + + it("caps the skip log at 3 occurrences to avoid spamming logs on every subsequent PR review", async () => { + globalThis.fetch = vi.fn(async () => ({ ok: false, status: 403 }) as Response) as unknown as typeof fetch; + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + const errSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + try { + probeReesSecretAtStartup(env({ REES_URL: "https://rees.example", REES_SHARED_SECRET: "s3cret" })); + await flush(); + for (let i = 0; i < 5; i += 1) { + await buildReviewEnrichment(env({ REES_URL: "https://rees.example", REES_SHARED_SECRET: "s3cret" }), input); + } + const skipLogs = warnSpy.mock.calls.filter((c) => JSON.parse(c[0] as string).event === "rees_enrich_skipped_auth_rejected"); + expect(skipLogs).toHaveLength(3); + } finally { + warnSpy.mockRestore(); + errSpy.mockRestore(); + } + }); + + it("a NON-auth probe failure (e.g. 500) does NOT trip the circuit breaker -- buildReviewEnrichment still attempts the call", async () => { + const fetchSpy = vi.fn(async () => ({ ok: false, status: 500 }) as Response); + globalThis.fetch = fetchSpy as unknown as typeof fetch; + const errSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + try { + probeReesSecretAtStartup(env({ REES_URL: "https://rees.example", REES_SHARED_SECRET: "s3cret" })); + await flush(); + fetchSpy.mockClear(); + + await buildReviewEnrichment(env({ REES_URL: "https://rees.example", REES_SHARED_SECRET: "s3cret" }), input); + expect(fetchSpy).toHaveBeenCalledTimes(1); // still attempted -- 500 isn't a confirmed secret mismatch + } finally { + errSpy.mockRestore(); + } + }); +}); + describe("isReesGithubTokenForwardingEnabled", () => { it("defaults off and only turns on for explicit truthy values", () => { expect(isReesGithubTokenForwardingEnabled(env({}))).toBe(false);