From 486e585f39cdb17a2bbf19f6fc536c55f8e69f12 Mon Sep 17 00:00:00 2001 From: can1357 Date: Sun, 8 Mar 2026 16:22:18 +0100 Subject: [PATCH] refactor(coding-agent): migrated fetch mocks to hookFetch utility with resource cleanup - Replaced vi.spyOn fetch mocking pattern with hookFetch utility across 5 test files. - Migrated fetch mocks to use resource management with 'using' keyword for automatic cleanup. - Removed @ts-expect-error comments related to fetch.preconnect type issues. - Updated ts-hook-fetch rule documentation with expanded patterns and clearer lifecycle guidance. --- .omp/rules/ts-hook-fetch.md | 36 +++++++---- packages/coding-agent/test/compaction.test.ts | 61 ++++++++++-------- .../test/tools/fetch-kagi-toggle.test.ts | 14 ++--- .../test/tools/web-search-exa.test.ts | 62 +++++++++---------- .../test/tools/web-search-kagi.test.ts | 49 ++++++++------- 5 files changed, 120 insertions(+), 102 deletions(-) diff --git a/.omp/rules/ts-hook-fetch.md b/.omp/rules/ts-hook-fetch.md index 1630fc9f8..09a91a39d 100644 --- a/.omp/rules/ts-hook-fetch.md +++ b/.omp/rules/ts-hook-fetch.md @@ -1,20 +1,20 @@ --- -description: Use hookFetch instead of assigning globalThis.fetch directly in tests -condition: "globalThis\\.fetch\\s*=" +description: Use hookFetch instead of assigning or spying on globalThis.fetch in tests +condition: "globalThis\\.fetch\\s*=|spyOn\\(globalThis.*fetch" scope: "tool:edit(**/*.test.{ts,tsx,js,jsx}), tool:write(**/*.test.{ts,tsx,js,jsx})" --- -**Do not assign `globalThis.fetch = ...` directly in tests.** +**Do not assign `globalThis.fetch` or use `vi.spyOn(globalThis, "fetch")` in tests.** ## Why it's wrong -- It bypasses the project's standard fetch mocking helper -- It is easier to forget restoration and leak state across tests -- It makes test mocking inconsistent across the codebase +- Forgetting restoration leaks state across tests +- `vi.spyOn` ties fetch mocking to vitest lifecycle instead of explicit scoping +- Makes test mocking inconsistent across the codebase ## What to use instead -Use `hookFetch` from `@oh-my-pi/pi-utils`: +Use `hookFetch` from `@oh-my-pi/pi-utils`. It returns a `Disposable` — use `using` for automatic cleanup: ```ts import { hookFetch } from "@oh-my-pi/pi-utils"; @@ -30,8 +30,22 @@ using _hook = hookFetch((input, init, next) => { // WRONG globalThis.fetch = async () => new Response("ok"); -// RIGHT -using _hook = hookFetch(() => new Response("ok")); -``` +// WRONG +vi.spyOn(globalThis, "fetch").mockResolvedValue(new Response("ok")); -If you need to intercept fetch in tests, use `hookFetch`. +// RIGHT — fixed response +using _hook = hookFetch(() => new Response("ok")); + +// RIGHT — conditional mock with passthrough +using _hook = hookFetch((input, init, next) => { + if (String(input).includes("127.0.0.1")) { + return new Response(JSON.stringify({ data: [] })); + } + return next(input, init); +}); + +// RIGHT — when you need vi.fn() for mock assertions +const fetchSpy = vi.fn(() => new Response("ok")); +using _hook = hookFetch(fetchSpy); +// later: expect(fetchSpy.mock.calls[0]).toEqual(...) +``` \ No newline at end of file diff --git a/packages/coding-agent/test/compaction.test.ts b/packages/coding-agent/test/compaction.test.ts index d1d899984..f4a4652ce 100644 --- a/packages/coding-agent/test/compaction.test.ts +++ b/packages/coding-agent/test/compaction.test.ts @@ -3,6 +3,7 @@ import * as path from "node:path"; import type { AgentMessage } from "@oh-my-pi/pi-agent-core"; import { getBundledModel } from "@oh-my-pi/pi-ai/models"; import type { AssistantMessage, Model, Usage } from "@oh-my-pi/pi-ai/types"; +import { hookFetch } from "@oh-my-pi/pi-utils"; const completeSimpleMock = vi.fn(); @@ -318,12 +319,14 @@ describe("remote compaction setting", () => { throw new Error("Expected compaction preparation"); } - const fetchSpy = vi.spyOn(globalThis, "fetch").mockResolvedValue( - new Response(JSON.stringify({ summary: "remote summary" }), { - status: 200, - headers: { "Content-Type": "application/json" }, - }), + const fetchSpy = vi.fn( + () => + new Response(JSON.stringify({ summary: "remote summary" }), { + status: 200, + headers: { "Content-Type": "application/json" }, + }), ); + using _hook = hookFetch(fetchSpy); const completeSpy = completeSimpleMock .mockResolvedValueOnce(createAssistantMessage("Local history summary")) .mockResolvedValueOnce(createAssistantMessage("Local turn summary")) @@ -394,12 +397,14 @@ describe("remote compaction setting", () => { { type: "message", role: "user", content: [{ type: "input_text", text: "Compacted retained user" }] }, { type: "compaction", encrypted_content: "new_encrypted" }, ]; - const fetchSpy = vi.spyOn(globalThis, "fetch").mockResolvedValue( - new Response(JSON.stringify({ output: remoteOutput }), { - status: 200, - headers: { "Content-Type": "application/json" }, - }), + const fetchSpy = vi.fn( + () => + new Response(JSON.stringify({ output: remoteOutput }), { + status: 200, + headers: { "Content-Type": "application/json" }, + }), ); + using _hook = hookFetch(fetchSpy); completeSimpleMock .mockResolvedValueOnce(createAssistantMessage("History summary")) .mockResolvedValueOnce(createAssistantMessage("Turn prefix summary")) @@ -458,12 +463,14 @@ describe("remote compaction setting", () => { }); if (!preparation) throw new Error("Expected compaction preparation"); - const fetchSpy = vi.spyOn(globalThis, "fetch").mockResolvedValue( - new Response(JSON.stringify({ output: [{ type: "compaction", encrypted_content: "new_encrypted" }] }), { - status: 200, - headers: { "Content-Type": "application/json" }, - }), + const fetchSpy = vi.fn( + () => + new Response(JSON.stringify({ output: [{ type: "compaction", encrypted_content: "new_encrypted" }] }), { + status: 200, + headers: { "Content-Type": "application/json" }, + }), ); + using _hook = hookFetch(fetchSpy); completeSimpleMock.mockResolvedValue(createAssistantMessage("Short summary")); await compact(preparation, model, "test-api-key"); @@ -499,12 +506,14 @@ describe("remote compaction setting", () => { }); if (!preparation) throw new Error("Expected compaction preparation"); - const fetchSpy = vi.spyOn(globalThis, "fetch").mockResolvedValue( - new Response(JSON.stringify({ output: [{ type: "compaction", encrypted_content: "new_encrypted" }] }), { - status: 200, - headers: { "Content-Type": "application/json" }, - }), + const fetchSpy = vi.fn( + () => + new Response(JSON.stringify({ output: [{ type: "compaction", encrypted_content: "new_encrypted" }] }), { + status: 200, + headers: { "Content-Type": "application/json" }, + }), ); + using _hook = hookFetch(fetchSpy); completeSimpleMock.mockResolvedValue(createAssistantMessage("Short summary")); await compact(preparation, model, "test-api-key"); @@ -541,12 +550,14 @@ describe("remote compaction setting", () => { { type: "message", role: "assistant", content: [{ type: "output_text", text: "Kept assistant" }] }, { type: "compaction", encrypted_content: "new_encrypted" }, ]; - const fetchSpy = vi.spyOn(globalThis, "fetch").mockResolvedValue( - new Response(JSON.stringify({ output: remoteOutput }), { - status: 200, - headers: { "Content-Type": "application/json" }, - }), + const fetchSpy = vi.fn( + () => + new Response(JSON.stringify({ output: remoteOutput }), { + status: 200, + headers: { "Content-Type": "application/json" }, + }), ); + using _hook = hookFetch(fetchSpy); completeSimpleMock.mockResolvedValue(createAssistantMessage("Short summary")); const result = await compact(preparation, model, "test-api-key", undefined, undefined, { diff --git a/packages/coding-agent/test/tools/fetch-kagi-toggle.test.ts b/packages/coding-agent/test/tools/fetch-kagi-toggle.test.ts index eefbc8b43..233ed85ec 100644 --- a/packages/coding-agent/test/tools/fetch-kagi-toggle.test.ts +++ b/packages/coding-agent/test/tools/fetch-kagi-toggle.test.ts @@ -12,7 +12,7 @@ import type { LoadPageResult } from "@oh-my-pi/pi-coding-agent/web/scrapers/type import * as scrapers from "@oh-my-pi/pi-coding-agent/web/scrapers/types"; import * as scraperUtils from "@oh-my-pi/pi-coding-agent/web/scrapers/utils"; import * as natives from "@oh-my-pi/pi-natives"; -import { ptree, Snowflake } from "@oh-my-pi/pi-utils"; +import { hookFetch, ptree, Snowflake } from "@oh-my-pi/pi-utils"; describe("fetch tool Kagi summarization toggle", () => { let testDir: string; @@ -73,7 +73,7 @@ describe("fetch tool Kagi summarization toggle", () => { const loadPageSpy = mockLoadPage(); const summarizeSpy = vi.spyOn(kagi, "summarizeUrlWithKagi").mockResolvedValue("x".repeat(150)); vi.spyOn(toolsManager, "ensureTool").mockResolvedValue(undefined); - vi.spyOn(globalThis, "fetch").mockResolvedValue(new Response("blocked", { status: 500, statusText: "Blocked" })); + using _hook = hookFetch(() => new Response("blocked", { status: 500, statusText: "Blocked" })); const result = await tool.execute("fetch-1", { url: "https://example.com" }); @@ -88,7 +88,7 @@ describe("fetch tool Kagi summarization toggle", () => { const loadPageSpy = mockLoadPage(); const summarizeSpy = vi.spyOn(kagi, "summarizeUrlWithKagi").mockResolvedValue("x".repeat(150)); vi.spyOn(toolsManager, "ensureTool").mockResolvedValue(undefined); - vi.spyOn(globalThis, "fetch").mockResolvedValue(new Response("blocked", { status: 500, statusText: "Blocked" })); + using _hook = hookFetch(() => new Response("blocked", { status: 500, statusText: "Blocked" })); const result = await tool.execute("fetch-2", { url: "https://example.com" }); @@ -425,9 +425,7 @@ describe("fetch tool Kagi summarization toggle", () => { content: "", }; }); - vi.spyOn(globalThis, "fetch").mockResolvedValue( - new Response("blocked", { status: 500, statusText: "Blocked" }), - ); + using _hook = hookFetch(() => new Response("blocked", { status: 500, statusText: "Blocked" })); vi.spyOn(toolsManager, "ensureTool").mockResolvedValue(undefined); vi.spyOn(natives, "htmlToMarkdown").mockResolvedValue(renderedMarkdown); @@ -513,9 +511,7 @@ describe("fetch tool Kagi summarization toggle", () => { content: "", }; }); - vi.spyOn(globalThis, "fetch").mockResolvedValue( - new Response("blocked", { status: 500, statusText: "Blocked" }), - ); + using _hook = hookFetch(() => new Response("blocked", { status: 500, statusText: "Blocked" })); vi.spyOn(toolsManager, "ensureTool").mockResolvedValue("/usr/bin/trafilatura"); const result = await tool.execute("fetch-section-llms", { url: pageUrl }); diff --git a/packages/coding-agent/test/tools/web-search-exa.test.ts b/packages/coding-agent/test/tools/web-search-exa.test.ts index 283e239a9..ae68463e2 100644 --- a/packages/coding-agent/test/tools/web-search-exa.test.ts +++ b/packages/coding-agent/test/tools/web-search-exa.test.ts @@ -1,4 +1,5 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "bun:test"; +import { hookFetch } from "@oh-my-pi/pi-utils"; import { runSearchQuery } from "../../src/web/search"; import { buildExaRequestBody, @@ -228,9 +229,8 @@ describe("searchExa", () => { delete process.env.EXA_API_KEY; }); - function mockFetch(responseBody: unknown, status = 200) { - // @ts-expect-error - test mock doesn't need fetch.preconnect - vi.spyOn(globalThis, "fetch").mockImplementation(async (_url: string | URL | Request, init?: RequestInit) => { + function mockFetch(responseBody: unknown, status = 200): Disposable { + return hookFetch((_url, init) => { if (init?.body) { capturedRequestBody = JSON.parse(init.body as string); } @@ -242,7 +242,7 @@ describe("searchExa", () => { } it("populates answer from per-result summaries", async () => { - mockFetch(makeMockExaResponse()); + using _hook = mockFetch(makeMockExaResponse()); const result = await searchExa({ query: "test query" }); expect(result.provider).toBe("exa"); expect(result.answer).toBeDefined(); @@ -253,7 +253,7 @@ describe("searchExa", () => { }); it("returns answer=undefined when no summaries are present", async () => { - mockFetch( + using _hook = mockFetch( makeMockExaResponse({ results: [{ title: "No Summary", url: "https://nosummary.com", text: "some text" }] }), ); const result = await searchExa({ query: "no answer query" }); @@ -263,28 +263,28 @@ describe("searchExa", () => { }); it("returns answer=undefined when results array is empty", async () => { - mockFetch(makeMockExaResponse({ results: [] })); + using _hook = mockFetch(makeMockExaResponse({ results: [] })); const result = await searchExa({ query: "empty" }); expect(result.answer).toBeUndefined(); expect(result.sources).toHaveLength(0); }); it("returns answer=undefined when results is missing from response", async () => { - mockFetch({ requestId: "req-empty" }); + using _hook = mockFetch({ requestId: "req-empty" }); const result = await searchExa({ query: "nothing" }); expect(result.answer).toBeUndefined(); expect(result.sources).toHaveLength(0); }); it("sends contents.summary in request body", async () => { - mockFetch(makeMockExaResponse()); + using _hook = mockFetch(makeMockExaResponse()); await searchExa({ query: "check body" }); expect(capturedRequestBody).toBeDefined(); expect(capturedRequestBody!.contents).toEqual({ summary: { query: "check body" } }); }); it("sends correct full request shape", async () => { - mockFetch(makeMockExaResponse()); + using _hook = mockFetch(makeMockExaResponse()); await searchExa({ query: "shape test", num_results: 5, type: "neural" }); expect(capturedRequestBody).toEqual({ query: "shape test", @@ -295,7 +295,7 @@ describe("searchExa", () => { }); it("prefers summary over text for snippet field", async () => { - mockFetch( + using _hook = mockFetch( makeMockExaResponse({ results: [{ title: "Has Both", url: "https://both.com", text: "full text here", summary: "summary here" }], }), @@ -305,7 +305,7 @@ describe("searchExa", () => { }); it("falls back to text when summary is null", async () => { - mockFetch( + using _hook = mockFetch( makeMockExaResponse({ results: [{ title: "Text Only", url: "https://text.com", text: "fallback text", summary: null }], }), @@ -315,7 +315,7 @@ describe("searchExa", () => { }); it("falls back to highlights when both summary and text are null", async () => { - mockFetch( + using _hook = mockFetch( makeMockExaResponse({ results: [ { @@ -333,7 +333,7 @@ describe("searchExa", () => { }); it("skips results without url", async () => { - mockFetch( + using _hook = mockFetch( makeMockExaResponse({ results: [ { title: "No URL", url: null, summary: "orphan" }, @@ -347,7 +347,7 @@ describe("searchExa", () => { }); it("falls back to text when summary is empty string (not just null)", async () => { - mockFetch( + using _hook = mockFetch( makeMockExaResponse({ results: [{ title: "Empty Summary", url: "https://empty.com", text: "real text", summary: "" }], }), @@ -357,7 +357,7 @@ describe("searchExa", () => { }); it("does not include url-less results in synthesized answer", async () => { - mockFetch( + using _hook = mockFetch( makeMockExaResponse({ results: [ { title: "No URL", url: null, summary: "ghost summary" }, @@ -373,13 +373,13 @@ describe("searchExa", () => { it("uses Exa MCP when API key is missing", async () => { delete process.env.EXA_API_KEY; - // @ts-expect-error - test mock doesn't need fetch.preconnect - const fetchSpy = vi.spyOn(globalThis, "fetch").mockImplementation(async () => { + const fetchSpy = vi.fn(async () => { return new Response(JSON.stringify({ jsonrpc: "2.0", id: "mcp-1", result: makeMockExaResponse() }), { status: 200, headers: { "Content-Type": "application/json" }, }); }); + using _hook = hookFetch(fetchSpy); const result = await searchExa({ query: "no key" }); expect(result.provider).toBe("exa"); @@ -393,8 +393,7 @@ describe("searchExa", () => { it("accepts MCP structuredContent search payloads when API key is missing", async () => { delete process.env.EXA_API_KEY; - // @ts-expect-error - test mock doesn't need fetch.preconnect - vi.spyOn(globalThis, "fetch").mockImplementation(async () => { + using _hook = hookFetch(async () => { return new Response( JSON.stringify({ jsonrpc: "2.0", @@ -414,8 +413,7 @@ describe("searchExa", () => { it("accepts MCP text content JSON payloads when API key is missing", async () => { delete process.env.EXA_API_KEY; const payload = makeMockExaResponse(); - // @ts-expect-error - test mock doesn't need fetch.preconnect - const fetchSpy = vi.spyOn(globalThis, "fetch").mockImplementation(async () => { + const fetchSpy = vi.fn(async () => { return new Response( JSON.stringify({ jsonrpc: "2.0", @@ -427,6 +425,7 @@ describe("searchExa", () => { { status: 200, headers: { "Content-Type": "application/json" } }, ); }); + using _hook = hookFetch(fetchSpy); const result = await searchExa({ query: "content payload" }); expect(result.provider).toBe("exa"); @@ -450,8 +449,7 @@ describe("searchExa", () => { "URL: https://plain-beta.com", "Text: Beta snippet", ].join("\n"); - // @ts-expect-error - test mock doesn't need fetch.preconnect - const fetchSpy = vi.spyOn(globalThis, "fetch").mockImplementation(async () => { + const fetchSpy = vi.fn(async () => { return new Response( JSON.stringify({ jsonrpc: "2.0", @@ -461,6 +459,7 @@ describe("searchExa", () => { { status: 200, headers: { "Content-Type": "application/json" } }, ); }); + using _hook = hookFetch(fetchSpy); const result = await searchExa({ query: "plain text content payload" }); expect(result.provider).toBe("exa"); @@ -494,8 +493,7 @@ describe("searchExa", () => { "URL: https://crlf-beta.com", "Text: Second result", ].join("\r\n"); - // @ts-expect-error - test mock doesn't need fetch.preconnect - vi.spyOn(globalThis, "fetch").mockImplementation(async () => { + using _hook = hookFetch(async () => { return new Response( JSON.stringify({ jsonrpc: "2.0", @@ -526,8 +524,7 @@ describe("searchExa", () => { "URL: https://plain-beta.com", "Text: Beta snippet", ].join("\n"); - // @ts-expect-error - test mock doesn't need fetch.preconnect - vi.spyOn(globalThis, "fetch").mockImplementation(async () => { + using _hook = hookFetch(async () => { return new Response( JSON.stringify({ jsonrpc: "2.0", @@ -556,8 +553,7 @@ describe("searchExa", () => { "URL: https://result-two.com", "Text: Second plain-text result", ].join("\n"); - // @ts-expect-error - test mock doesn't need fetch.preconnect - vi.spyOn(globalThis, "fetch").mockImplementation(async () => { + using _hook = hookFetch(async () => { return new Response( JSON.stringify({ jsonrpc: "2.0", @@ -576,8 +572,7 @@ describe("searchExa", () => { it("runSearchQuery with provider=exa succeeds without EXA_API_KEY for MCP structuredContent", async () => { delete process.env.EXA_API_KEY; - // @ts-expect-error - test mock doesn't need fetch.preconnect - vi.spyOn(globalThis, "fetch").mockImplementation(async () => { + using _hook = hookFetch(async () => { return new Response( JSON.stringify({ jsonrpc: "2.0", @@ -596,8 +591,7 @@ describe("searchExa", () => { it("throws clear error when MCP content payload is not parseable JSON", async () => { delete process.env.EXA_API_KEY; - // @ts-expect-error - test mock doesn't need fetch.preconnect - vi.spyOn(globalThis, "fetch").mockImplementation(async () => { + using _hook = hookFetch(async () => { return new Response( JSON.stringify({ jsonrpc: "2.0", @@ -616,7 +610,7 @@ describe("searchExa", () => { }); it("throws SearchProviderError on non-ok HTTP response", async () => { - mockFetch("Forbidden", 403); + using _hook = mockFetch("Forbidden", 403); await expect(searchExa({ query: "forbidden" })).rejects.toThrow("Exa API error (403)"); }); }); diff --git a/packages/coding-agent/test/tools/web-search-kagi.test.ts b/packages/coding-agent/test/tools/web-search-kagi.test.ts index 08f0358ec..dd75fc917 100644 --- a/packages/coding-agent/test/tools/web-search-kagi.test.ts +++ b/packages/coding-agent/test/tools/web-search-kagi.test.ts @@ -1,4 +1,5 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "bun:test"; +import { hookFetch } from "@oh-my-pi/pi-utils"; import { searchWithKagi } from "../../src/web/kagi"; import { searchKagi } from "../../src/web/search/providers/kagi"; import { SearchProviderError } from "../../src/web/search/types"; @@ -17,11 +18,12 @@ describe("Kagi web search error handling", () => { const providerMessage = "Kagi Search API is in beta. Please contact support@kagi.com to enable API access for your account."; - vi.spyOn(globalThis, "fetch").mockResolvedValue( - new Response(JSON.stringify({ error: [{ code: 401, msg: providerMessage }] }), { - status: 401, - headers: { "Content-Type": "application/json" }, - }), + using _hook = hookFetch( + () => + new Response(JSON.stringify({ error: [{ code: 401, msg: providerMessage }] }), { + status: 401, + headers: { "Content-Type": "application/json" }, + }), ); try { @@ -35,29 +37,30 @@ describe("Kagi web search error handling", () => { }); it("falls back to plain text for non-JSON error bodies", async () => { - vi.spyOn(globalThis, "fetch").mockResolvedValue(new Response("upstream unavailable", { status: 503 })); + using _hook = hookFetch(() => new Response("upstream unavailable", { status: 503 })); await expect(searchWithKagi("plain text error")).rejects.toThrow("Kagi API error (503): upstream unavailable"); }); it("preserves successful search parsing", async () => { - vi.spyOn(globalThis, "fetch").mockResolvedValue( - new Response( - JSON.stringify({ - meta: { id: "req-kagi-success" }, - data: [ - { - t: 0, - url: "https://example.com/article", - title: "Example Article", - snippet: "Example snippet", - published: "2025-01-01T00:00:00Z", - }, - { t: 1, list: ["What is Kagi Search API beta access?"] }, - ], - }), - { status: 200, headers: { "Content-Type": "application/json" } }, - ), + using _hook = hookFetch( + () => + new Response( + JSON.stringify({ + meta: { id: "req-kagi-success" }, + data: [ + { + t: 0, + url: "https://example.com/article", + title: "Example Article", + snippet: "Example snippet", + published: "2025-01-01T00:00:00Z", + }, + { t: 1, list: ["What is Kagi Search API beta access?"] }, + ], + }), + { status: 200, headers: { "Content-Type": "application/json" } }, + ), ); await expect(searchWithKagi("success case")).resolves.toEqual({