From 3fcfe2c056887990bb15a3c9e357d945f95191f6 Mon Sep 17 00:00:00 2001 From: Rodrigo Uroz Date: Tue, 10 Feb 2026 13:21:12 -0300 Subject: [PATCH] fix: address PR review feedback (runtime mock, HTML detection, retry docs) --- ...ed-helpers.iscloudflareorhtmlerrorpage.test.ts | 5 +++++ src/agents/pi-embedded-helpers/errors.ts | 4 +++- src/auto-reply/reply/agent-runner-execution.ts | 4 ++++ .../agent-runner.transient-http-retry.test.ts | 15 +++++++++++---- 4 files changed, 23 insertions(+), 5 deletions(-) diff --git a/src/agents/pi-embedded-helpers.iscloudflareorhtmlerrorpage.test.ts b/src/agents/pi-embedded-helpers.iscloudflareorhtmlerrorpage.test.ts index a5593fc1edf..ebdb22c6c5d 100644 --- a/src/agents/pi-embedded-helpers.iscloudflareorhtmlerrorpage.test.ts +++ b/src/agents/pi-embedded-helpers.iscloudflareorhtmlerrorpage.test.ts @@ -21,4 +21,9 @@ describe("isCloudflareOrHtmlErrorPage", () => { expect(isCloudflareOrHtmlErrorPage("500 Internal Server Error")).toBe(false); expect(isCloudflareOrHtmlErrorPage("429 Too Many Requests")).toBe(false); }); + + it("does not flag quoted HTML without a closing html tag", () => { + const plainTextWithHtmlPrefix = "500 upstream responded with partial HTML text"; + expect(isCloudflareOrHtmlErrorPage(plainTextWithHtmlPrefix)).toBe(false); + }); }); diff --git a/src/agents/pi-embedded-helpers/errors.ts b/src/agents/pi-embedded-helpers/errors.ts index e40ea502021..fe186bd596a 100644 --- a/src/agents/pi-embedded-helpers/errors.ts +++ b/src/agents/pi-embedded-helpers/errors.ts @@ -127,7 +127,9 @@ export function isCloudflareOrHtmlErrorPage(raw: string): boolean { return true; } - return status.code < 600 && HTML_ERROR_PREFIX_RE.test(status.rest); + return ( + status.code < 600 && HTML_ERROR_PREFIX_RE.test(status.rest) && /<\/html>/i.test(status.rest) + ); } export function isTransientHttpError(raw: string): boolean { diff --git a/src/auto-reply/reply/agent-runner-execution.ts b/src/auto-reply/reply/agent-runner-execution.ts index aaa4acb18f7..c1e1b4c66cd 100644 --- a/src/auto-reply/reply/agent-runner-execution.ts +++ b/src/auto-reply/reply/agent-runner-execution.ts @@ -583,6 +583,10 @@ export async function runAgentTurnWithFallback(params: { if (isTransientHttp && !didRetryTransientHttpError) { didRetryTransientHttpError = true; + // Retry the full runWithModelFallback() cycle — transient errors + // (502/521/etc.) typically affect the whole provider, so falling + // back to an alternate model first would not help. Instead we wait + // and retry the complete primary→fallback chain. defaultRuntime.error( `Transient HTTP provider error before reply (${message}). Retrying once in ${TRANSIENT_HTTP_RETRY_DELAY_MS}ms.`, ); diff --git a/src/auto-reply/reply/agent-runner.transient-http-retry.test.ts b/src/auto-reply/reply/agent-runner.transient-http-retry.test.ts index 7c858f2125d..5f21a40a9cc 100644 --- a/src/auto-reply/reply/agent-runner.transient-http-retry.test.ts +++ b/src/auto-reply/reply/agent-runner.transient-http-retry.test.ts @@ -4,6 +4,7 @@ import type { FollowupRun, QueueSettings } from "./queue.js"; import { createMockTypingController } from "./test-helpers.js"; const runEmbeddedPiAgentMock = vi.fn(); +const runtimeErrorMock = vi.fn(); vi.mock("../../agents/model-fallback.js", () => ({ runWithModelFallback: async ({ @@ -26,6 +27,14 @@ vi.mock("../../agents/pi-embedded.js", () => ({ runEmbeddedPiAgent: (params: unknown) => runEmbeddedPiAgentMock(params), })); +vi.mock("../../runtime.js", () => ({ + defaultRuntime: { + log: vi.fn(), + error: (...args: unknown[]) => runtimeErrorMock(...args), + exit: vi.fn(), + }, +})); + vi.mock("./queue.js", async () => { const actual = await vi.importActual("./queue.js"); return { @@ -40,6 +49,7 @@ import { runReplyAgent } from "./agent-runner.js"; describe("runReplyAgent transient HTTP retry", () => { beforeEach(() => { runEmbeddedPiAgentMock.mockReset(); + runtimeErrorMock.mockReset(); vi.useFakeTimers(); }); @@ -48,8 +58,6 @@ describe("runReplyAgent transient HTTP retry", () => { }); it("retries once after transient 521 HTML failure and then succeeds", async () => { - const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); - runEmbeddedPiAgentMock .mockRejectedValueOnce( new Error( @@ -118,12 +126,11 @@ describe("runReplyAgent transient HTTP retry", () => { const result = await runPromise; expect(runEmbeddedPiAgentMock).toHaveBeenCalledTimes(2); - expect(errorSpy).toHaveBeenCalledWith( + expect(runtimeErrorMock).toHaveBeenCalledWith( expect.stringContaining("Transient HTTP provider error before reply"), ); const payload = Array.isArray(result) ? result[0] : result; expect(payload?.text).toContain("Recovered response"); - errorSpy.mockRestore(); }); });