From 3e368f13c8d53d4c82e2a1cae9b4d0aa30e0ffc0 Mon Sep 17 00:00:00 2001 From: JerrettDavis Date: Fri, 3 Apr 2026 10:28:18 -0500 Subject: [PATCH] refactor(openclaw): DRY up duplicate code across plugin - Extract shared defaultLogger constant, use in both ProxyManager and HeadroomContextEngine (was duplicated inline in both constructors) - Extract parseProxyUrl() helper, reused by normalizeAndValidateProxyUrl and withDefaultPort (eliminates redundant URL parsing) - Add test helpers: stubProbeSuccess/NonHeadroom/Unreachable to replace 6 identical inline fetch mock constructions - Remove duplicate probe tests from engine.test.ts (already covered in proxy-manager.test.ts) - Clean up unused probeHeadroomProxy import from engine.test.ts Net: -26 lines, same coverage (28 tests) --- plugins/openclaw/src/engine.ts | 9 +-- plugins/openclaw/src/index.ts | 2 +- plugins/openclaw/src/proxy-manager.ts | 27 +++++---- plugins/openclaw/test/engine.test.ts | 24 +------- plugins/openclaw/test/proxy-manager.test.ts | 66 ++++++++++----------- 5 files changed, 51 insertions(+), 77 deletions(-) diff --git a/plugins/openclaw/src/engine.ts b/plugins/openclaw/src/engine.ts index 73ed16366..4e0b44eb1 100644 --- a/plugins/openclaw/src/engine.ts +++ b/plugins/openclaw/src/engine.ts @@ -8,7 +8,7 @@ /* eslint-disable @typescript-eslint/no-explicit-any */ import { compress } from "headroom-ai"; -import { ProxyManager, type ProxyManagerConfig, type ProxyManagerLogger } from "./proxy-manager.js"; +import { ProxyManager, defaultLogger, type ProxyManagerConfig, type ProxyManagerLogger } from "./proxy-manager.js"; import { agentToOpenAI, openAIToAgent } from "./convert.js"; export interface HeadroomEngineConfig extends ProxyManagerConfig { @@ -36,12 +36,7 @@ export class HeadroomContextEngine { constructor(config: HeadroomEngineConfig = {}, logger?: ProxyManagerLogger) { this.config = config; - this.logger = logger ?? { - info: (m) => console.log(`[headroom] ${m}`), - warn: (m) => console.warn(`[headroom] ${m}`), - error: (m) => console.error(`[headroom] ${m}`), - debug: () => {}, - }; + this.logger = logger ?? defaultLogger; this.proxyManager = new ProxyManager(config, this.logger); } diff --git a/plugins/openclaw/src/index.ts b/plugins/openclaw/src/index.ts index aef51f099..915853fb4 100644 --- a/plugins/openclaw/src/index.ts +++ b/plugins/openclaw/src/index.ts @@ -1,5 +1,5 @@ export { default } from "./plugin/index.js"; export { HeadroomContextEngine } from "./engine.js"; -export { ProxyManager, normalizeAndValidateProxyUrl, isLocalProxyUrl, probeHeadroomProxy } from "./proxy-manager.js"; +export { ProxyManager, normalizeAndValidateProxyUrl, isLocalProxyUrl, defaultLogger, probeHeadroomProxy } from "./proxy-manager.js"; export { agentToOpenAI, openAIToAgent } from "./convert.js"; export { createHeadroomRetrieveTool } from "./tools/headroom-retrieve.js"; diff --git a/plugins/openclaw/src/proxy-manager.ts b/plugins/openclaw/src/proxy-manager.ts index bfc56eacf..cece87294 100644 --- a/plugins/openclaw/src/proxy-manager.ts +++ b/plugins/openclaw/src/proxy-manager.ts @@ -27,6 +27,14 @@ export interface ProxyManagerLogger { debug(message: string): void; } +/** Default logger that prefixes all messages with `[headroom]`. */ +export const defaultLogger: ProxyManagerLogger = { + info: (m) => console.log(`[headroom] ${m}`), + warn: (m) => console.warn(`[headroom] ${m}`), + error: (m) => console.error(`[headroom] ${m}`), + debug: () => {}, +}; + export interface ProxyProbeResult { reachable: boolean; isHeadroom: boolean; @@ -41,13 +49,6 @@ interface LaunchSpec { checkArgs: string[]; } -const defaultLogger: ProxyManagerLogger = { - info: (m) => console.log(`[headroom] ${m}`), - warn: (m) => console.warn(`[headroom] ${m}`), - error: (m) => console.error(`[headroom] ${m}`), - debug: () => {}, -}; - export class ProxyManager { private config: ProxyManagerConfig; private logger: ProxyManagerLogger; @@ -309,13 +310,17 @@ export class ProxyManager { } } -export function normalizeAndValidateProxyUrl(proxyUrl: string): string { - let parsed: URL; +/** Parse a URL, returning the parsed object or throwing a descriptive error. */ +function parseProxyUrl(proxyUrl: string): URL { try { - parsed = new URL(proxyUrl); + return new URL(proxyUrl); } catch { throw new Error(`Invalid proxyUrl: "${proxyUrl}"`); } +} + +export function normalizeAndValidateProxyUrl(proxyUrl: string): string { + const parsed = parseProxyUrl(proxyUrl); if (parsed.protocol !== "http:" && parsed.protocol !== "https:") { throw new Error("proxyUrl must use http:// or https://"); @@ -339,7 +344,7 @@ export function isLocalProxyUrl(proxyUrl: string): boolean { } function withDefaultPort(proxyUrl: string, defaultPort: number): string { - const parsed = new URL(proxyUrl); + const parsed = parseProxyUrl(proxyUrl); if (!parsed.port) { parsed.port = String(defaultPort); } diff --git a/plugins/openclaw/test/engine.test.ts b/plugins/openclaw/test/engine.test.ts index c118d16b4..ebf470dc2 100644 --- a/plugins/openclaw/test/engine.test.ts +++ b/plugins/openclaw/test/engine.test.ts @@ -10,7 +10,7 @@ import { describe, it, expect, beforeAll, afterAll, vi, afterEach } from "vitest"; import { HeadroomContextEngine } from "../src/engine.js"; import { agentToOpenAI, openAIToAgent } from "../src/convert.js"; -import { ProxyManager, probeHeadroomProxy } from "../src/proxy-manager.js"; +import { ProxyManager } from "../src/proxy-manager.js"; const RUN = process.env.HEADROOM_INTEGRATION === "1"; const PROXY_URL = process.env.HEADROOM_PROXY_URL ?? "http://127.0.0.1:8787"; @@ -19,27 +19,7 @@ afterEach(() => { vi.restoreAllMocks(); }); -describe("Proxy probing", () => { - it("detects running Headroom proxy", async () => { - const fetchMock = vi.fn() - .mockResolvedValueOnce({ ok: true, status: 200 }) - .mockResolvedValueOnce({ ok: true, status: 200 }); - vi.stubGlobal("fetch", fetchMock); - - const result = await probeHeadroomProxy("http://127.0.0.1:8787"); - expect(result).toEqual({ reachable: true, isHeadroom: true }); - }); - - it("flags non-headroom service at configured URL", async () => { - const fetchMock = vi.fn() - .mockResolvedValueOnce({ ok: true, status: 200 }) - .mockResolvedValueOnce({ ok: false, status: 404 }); - vi.stubGlobal("fetch", fetchMock); - - const manager = new ProxyManager({ proxyUrl: "http://127.0.0.1:8787" }); - await expect(manager.start()).rejects.toThrow(/does not appear to be a Headroom proxy/); - }); -}); +// Proxy probing and ProxyManager.start tests live in proxy-manager.test.ts describe("AgentMessage conversion", () => { it("converts user message", () => { diff --git a/plugins/openclaw/test/proxy-manager.test.ts b/plugins/openclaw/test/proxy-manager.test.ts index ac723b191..cf85634af 100644 --- a/plugins/openclaw/test/proxy-manager.test.ts +++ b/plugins/openclaw/test/proxy-manager.test.ts @@ -10,6 +10,29 @@ afterEach(() => { vi.restoreAllMocks(); }); +/** Stub fetch with a sequence of health/retrieve probe outcomes. */ +function stubProbeSuccess() { + const mock = vi.fn() + .mockResolvedValueOnce({ ok: true, status: 200 }) // /health + .mockResolvedValueOnce({ ok: true, status: 200 }); // /v1/retrieve/stats + vi.stubGlobal("fetch", mock); + return mock; +} + +function stubProbeNonHeadroom() { + const mock = vi.fn() + .mockResolvedValueOnce({ ok: true, status: 200 }) // /health OK + .mockResolvedValueOnce({ ok: false, status: 404 }); // /v1/retrieve/stats 404 + vi.stubGlobal("fetch", mock); + return mock; +} + +function stubProbeUnreachable() { + const mock = vi.fn().mockRejectedValue(new Error("ECONNREFUSED")); + vi.stubGlobal("fetch", mock); + return mock; +} + describe("normalizeAndValidateProxyUrl", () => { it("accepts localhost origins", () => { expect(normalizeAndValidateProxyUrl("http://127.0.0.1:8787")).toBe("http://127.0.0.1:8787"); @@ -50,23 +73,13 @@ describe("isLocalProxyUrl", () => { describe("probeHeadroomProxy", () => { it("returns reachable+isHeadroom when both endpoints succeed", async () => { - const fetchMock = vi - .fn() - .mockResolvedValueOnce({ ok: true, status: 200 }) - .mockResolvedValueOnce({ ok: true, status: 200 }); - vi.stubGlobal("fetch", fetchMock); - + stubProbeSuccess(); const result = await probeHeadroomProxy("http://127.0.0.1:8787"); expect(result).toEqual({ reachable: true, isHeadroom: true }); }); it("returns reachable but non-headroom when retrieve endpoint fails", async () => { - const fetchMock = vi - .fn() - .mockResolvedValueOnce({ ok: true, status: 200 }) - .mockResolvedValueOnce({ ok: false, status: 404 }); - vi.stubGlobal("fetch", fetchMock); - + stubProbeNonHeadroom(); const result = await probeHeadroomProxy("http://127.0.0.1:8787"); expect(result.reachable).toBe(true); expect(result.isHeadroom).toBe(false); @@ -74,7 +87,7 @@ describe("probeHeadroomProxy", () => { }); it("returns unreachable when health check fails", async () => { - vi.stubGlobal("fetch", vi.fn().mockRejectedValue(new Error("boom"))); + stubProbeUnreachable(); const result = await probeHeadroomProxy("http://127.0.0.1:8787"); expect(result.reachable).toBe(false); expect(result.isHeadroom).toBe(false); @@ -115,12 +128,7 @@ describe("ProxyManager.start", () => { it("fails when explicit URL is reachable but not a headroom proxy", async () => { const manager = new ProxyManager({ proxyUrl: "http://127.0.0.1:8787" }); - const fetchMock = vi - .fn() - .mockResolvedValueOnce({ ok: true, status: 200 }) - .mockResolvedValueOnce({ ok: false, status: 404 }); - vi.stubGlobal("fetch", fetchMock); - + stubProbeNonHeadroom(); await expect(manager.start()).rejects.toThrow(/does not appear to be a Headroom proxy/); }); @@ -143,13 +151,7 @@ describe("ProxyManager.start", () => { it("connects to remote proxy without auto-start", async () => { const manager = new ProxyManager({ proxyUrl: "http://headroom.remote.example:8787", autoStart: true }); const startSpy = vi.spyOn(manager as any, "startHeadroomProxy").mockResolvedValue(undefined); - - // Remote probe succeeds - const fetchMock = vi - .fn() - .mockResolvedValueOnce({ ok: true, status: 200 }) // /health - .mockResolvedValueOnce({ ok: true, status: 200 }); // /v1/retrieve/stats - vi.stubGlobal("fetch", fetchMock); + stubProbeSuccess(); const url = await manager.start(); expect(url).toBe("http://headroom.remote.example:8787"); @@ -157,14 +159,8 @@ describe("ProxyManager.start", () => { }); it("does not apply proxyPort default to remote URLs", async () => { - // Remote URL without port should use protocol default, not proxyPort const manager = new ProxyManager({ proxyUrl: "https://headroom.remote.example", proxyPort: 9999 }); - - const fetchMock = vi - .fn() - .mockResolvedValueOnce({ ok: true, status: 200 }) - .mockResolvedValueOnce({ ok: true, status: 200 }); - vi.stubGlobal("fetch", fetchMock); + stubProbeSuccess(); const url = await manager.start(); expect(url).toBe("https://headroom.remote.example"); @@ -173,9 +169,7 @@ describe("ProxyManager.start", () => { it("fails fast for unreachable remote proxy without attempting auto-start", async () => { const manager = new ProxyManager({ proxyUrl: "https://headroom.remote.example:8787", autoStart: true }); const startSpy = vi.spyOn(manager as any, "startHeadroomProxy").mockResolvedValue(undefined); - - const fetchMock = vi.fn().mockRejectedValue(new Error("ECONNREFUSED")); - vi.stubGlobal("fetch", fetchMock); + stubProbeUnreachable(); await expect(manager.start()).rejects.toThrow(/Remote Headroom proxy not reachable/); expect(startSpy).not.toHaveBeenCalled();