fix: address PR review feedback (runtime mock, HTML detection, retry docs)

This commit is contained in:
Rodrigo Uroz 2026-02-10 13:21:12 -03:00 committed by Tak Hoffman
parent 1e388a503a
commit 3fcfe2c056
4 changed files with 23 additions and 5 deletions

View File

@ -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 <!DOCTYPE html> upstream responded with partial HTML text";
expect(isCloudflareOrHtmlErrorPage(plainTextWithHtmlPrefix)).toBe(false);
});
});

View File

@ -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 {

View File

@ -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.`,
);

View File

@ -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<typeof import("./queue.js")>("./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();
});
});