mirror of
https://github.com/stack-auth/stack.git
synced 2026-06-30 21:01:54 +08:00
Address review comments: add TTL cache, fetch timeout, and null/undefined tests
- Add 5-minute TTL module-level cache for skill fetch to prevent redundant HTTP calls (addresses Greptile P1) - Add AbortController with 5s timeout to prevent indefinite hangs (addresses CodeRabbit + Cubic P1) - Add test cases for null and undefined toolName inputs (addresses Greptile P2) - Add test for timeout error handling - Add test for cache hit behavior Co-Authored-By: mantra <mantra@stack-auth.com>
This commit is contained in:
parent
29b6554cb2
commit
1836e101bb
@ -1,18 +1,33 @@
|
||||
import { afterEach, describe, expect, it, vi } from "vitest";
|
||||
import { getMcpSkillContextPrompt } from "./mcp-skill-context";
|
||||
import { _clearSkillCache, getMcpSkillContextPrompt } from "./mcp-skill-context";
|
||||
|
||||
describe("getMcpSkillContextPrompt", () => {
|
||||
afterEach(() => {
|
||||
vi.restoreAllMocks();
|
||||
_clearSkillCache();
|
||||
});
|
||||
|
||||
it("does not fetch skill context for non-ask_hexclave requests", async () => {
|
||||
it("returns empty string for non-ask_hexclave tool names", async () => {
|
||||
const fetchSpy = vi.spyOn(globalThis, "fetch");
|
||||
|
||||
await expect(getMcpSkillContextPrompt("other_tool")).resolves.toMatchInlineSnapshot(`""`);
|
||||
expect(fetchSpy).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("returns empty string for null toolName", async () => {
|
||||
const fetchSpy = vi.spyOn(globalThis, "fetch");
|
||||
|
||||
await expect(getMcpSkillContextPrompt(null)).resolves.toMatchInlineSnapshot(`""`);
|
||||
expect(fetchSpy).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("returns empty string for undefined toolName", async () => {
|
||||
const fetchSpy = vi.spyOn(globalThis, "fetch");
|
||||
|
||||
await expect(getMcpSkillContextPrompt(undefined)).resolves.toMatchInlineSnapshot(`""`);
|
||||
expect(fetchSpy).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("fetches and embeds the canonical skill for ask_hexclave requests", async () => {
|
||||
const fetchSpy = vi.spyOn(globalThis, "fetch").mockResolvedValue(
|
||||
new Response("# Hexclave Skill\n\nUse Hexclave docs."),
|
||||
@ -34,9 +49,10 @@ describe("getMcpSkillContextPrompt", () => {
|
||||
Use Hexclave docs.
|
||||
"
|
||||
`);
|
||||
expect(fetchSpy).toHaveBeenCalledWith("https://skill.hexclave.com", {
|
||||
expect(fetchSpy).toHaveBeenCalledWith("https://skill.hexclave.com", expect.objectContaining({
|
||||
headers: { Accept: "text/markdown" },
|
||||
});
|
||||
signal: expect.any(AbortSignal),
|
||||
}));
|
||||
});
|
||||
|
||||
it("fails loudly when the canonical skill cannot be fetched", async () => {
|
||||
@ -48,4 +64,27 @@ describe("getMcpSkillContextPrompt", () => {
|
||||
`[Error: Failed to fetch skill from https://skill.hexclave.com: 503 Service Unavailable]`,
|
||||
);
|
||||
});
|
||||
|
||||
it("throws a descriptive error when the fetch times out", async () => {
|
||||
vi.spyOn(globalThis, "fetch").mockImplementation(() => {
|
||||
const err = new DOMException("The operation was aborted", "AbortError");
|
||||
return Promise.reject(err);
|
||||
});
|
||||
|
||||
await expect(getMcpSkillContextPrompt("ask_hexclave")).rejects.toThrowErrorMatchingInlineSnapshot(
|
||||
`[Error: Skill fetch from https://skill.hexclave.com timed out after 5000ms]`,
|
||||
);
|
||||
});
|
||||
|
||||
it("returns cached skill on subsequent calls within TTL", async () => {
|
||||
const fetchSpy = vi.spyOn(globalThis, "fetch").mockResolvedValue(
|
||||
new Response("# Cached Skill"),
|
||||
);
|
||||
|
||||
const first = await getMcpSkillContextPrompt("ask_hexclave");
|
||||
const second = await getMcpSkillContextPrompt("ask_hexclave");
|
||||
|
||||
expect(first).toBe(second);
|
||||
expect(fetchSpy).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
});
|
||||
|
||||
@ -1,20 +1,50 @@
|
||||
const hexclaveSkillResourceUri = "https://skill.hexclave.com";
|
||||
const HEXCLAVE_SKILL_URI = "https://skill.hexclave.com";
|
||||
const FETCH_TIMEOUT_MS = 5_000;
|
||||
const CACHE_TTL_MS = 5 * 60 * 1_000; // 5 minutes
|
||||
|
||||
let cachedSkill: { text: string, fetchedAt: number } | null = null;
|
||||
|
||||
async function fetchSkillText(): Promise<string> {
|
||||
const now = Date.now();
|
||||
if (cachedSkill && now - cachedSkill.fetchedAt < CACHE_TTL_MS) {
|
||||
return cachedSkill.text;
|
||||
}
|
||||
|
||||
const controller = new AbortController();
|
||||
const timeoutId = setTimeout(() => controller.abort(), FETCH_TIMEOUT_MS);
|
||||
|
||||
let response: Response;
|
||||
try {
|
||||
response = await fetch(HEXCLAVE_SKILL_URI, {
|
||||
headers: { Accept: "text/markdown" },
|
||||
signal: controller.signal,
|
||||
});
|
||||
} catch (err: unknown) {
|
||||
if (err instanceof DOMException && err.name === "AbortError") {
|
||||
throw new Error(`Skill fetch from ${HEXCLAVE_SKILL_URI} timed out after ${FETCH_TIMEOUT_MS}ms`);
|
||||
}
|
||||
throw err;
|
||||
} finally {
|
||||
clearTimeout(timeoutId);
|
||||
}
|
||||
|
||||
if (!response.ok) {
|
||||
throw new Error(
|
||||
`Failed to fetch skill from ${HEXCLAVE_SKILL_URI}: ${response.status} ${response.statusText}`,
|
||||
);
|
||||
}
|
||||
|
||||
const text = await response.text();
|
||||
cachedSkill = { text, fetchedAt: now };
|
||||
return text;
|
||||
}
|
||||
|
||||
export async function getMcpSkillContextPrompt(toolName: string | null | undefined): Promise<string> {
|
||||
if (toolName !== "ask_hexclave") {
|
||||
return "";
|
||||
}
|
||||
|
||||
const response = await fetch(hexclaveSkillResourceUri, {
|
||||
headers: { Accept: "text/markdown" },
|
||||
});
|
||||
if (!response.ok) {
|
||||
throw new Error(
|
||||
`Failed to fetch skill from ${hexclaveSkillResourceUri}: ${response.status} ${response.statusText}`,
|
||||
);
|
||||
}
|
||||
|
||||
const skillContext = await response.text();
|
||||
const skillContext = await fetchSkillText();
|
||||
return `
|
||||
|
||||
## MCP-Provided Hexclave Skill Context
|
||||
@ -28,3 +58,10 @@ facts and citations:
|
||||
${skillContext}
|
||||
`;
|
||||
}
|
||||
|
||||
/**
|
||||
* Exposed for testing only — clears the module-level skill cache.
|
||||
*/
|
||||
export function _clearSkillCache(): void {
|
||||
cachedSkill = null;
|
||||
}
|
||||
|
||||
Loading…
Reference in New Issue
Block a user