robbond/chatgpt-mcp
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

feat: add provider abstraction for review backends

Browse Source
This commit is contained in:
robbond
2026-06-12 19:23:45 +01:00
parent 1b8edb7eba
commit 732d6edd38
22 changed files with 1432 additions and 882 deletions
Download Patch File Download Diff File Expand all files Collapse all files
test/tools/review-code.test.js
+109 -134
View File
@@ -5,7 +5,7 @@ const mockConfig = {
openaiApiKey: "sk-test-key",
openaiModel: "gpt-5.1",
temperature: 0.2,
maxOutputTokens: 2000,
maxOutputTokens: 4000,
logLevel: "info",
enableFileContext: false,
contextDir: "./context",
@@ -16,8 +16,8 @@ const mockConfig = {
redactSecrets: true,
};
function makeValidInput(question) {
return { question };
function makeValidInput(question, context) {
return { question: question || "Review this code", context: context || "x" };
}
// --- Success path ---
@@ -25,11 +25,11 @@ function makeValidInput(question) {
describe("success path", () => {
it("returns ok:true with answer on full happy flow", async () => {
const loadConfig = vi.fn(() => mockConfig);
const createOpenAIClient = vi.fn(() => ({ responses: { create: vi.fn() } }));
const sendOpenAIResponse = vi.fn(async () => ({ content: "OK" }));
const sendMock = vi.fn(async () => ({ content: "OK" }));
const createProvider = vi.fn(() => ({ send: sendMock }));
const result = await handleReviewCode(makeValidInput("Review my code"), {
loadConfig, createOpenAIClient, sendOpenAIResponse,
const result = await handleReviewCode(makeValidInput("Review this code", "x"), {
loadConfig, createProvider,
});
expect(result.ok).toBe(true);
@@ -40,11 +40,11 @@ describe("success path", () => {
it("propagates budget warnings through to success result", async () => {
const trimmedBudget = { ...mockConfig, maxInputChars: 50 };
const loadConfig = vi.fn(() => trimmedBudget);
const createOpenAIClient = vi.fn(() => ({ responses: { create: vi.fn() } }));
const sendOpenAIResponse = vi.fn(async () => ({ content: "OK" }));
const sendMock = vi.fn(async () => ({ content: "OK" }));
const createProvider = vi.fn(() => ({ send: sendMock }));
const result = await handleReviewCode(makeValidInput("hi"), {
loadConfig, createOpenAIClient, sendOpenAIResponse,
const result = await handleReviewCode(makeValidInput("Review this code", "x"), {
loadConfig, createProvider,
});
expect(result.ok).toBe(true);
@@ -57,10 +57,10 @@ describe("success path", () => {
describe("validation failure", () => {
it("returns structured error when question is missing", async () => {
const loadConfig = vi.fn();
const createOpenAIClient = vi.fn();
const sendOpenAIResponse = vi.fn();
const sendMock = vi.fn();
const createProvider = vi.fn(() => ({ send: sendMock }));
const result = await handleReviewCode({}, { loadConfig, createOpenAIClient, sendOpenAIResponse });
const result = await handleReviewCode({}, { loadConfig, createProvider });
expect(result.ok).toBe(false);
expect(typeof result.error).toBe("string");
expect(result.warnings).toEqual([]);
@@ -68,21 +68,21 @@ describe("validation failure", () => {
it("short-circuits — no other deps called", async () => {
const loadConfig = vi.fn();
const createOpenAIClient = vi.fn();
const sendOpenAIResponse = vi.fn();
const sendMock = vi.fn();
const createProvider = vi.fn(() => ({ send: sendMock }));
await handleReviewCode({ foo: "bar" }, { loadConfig, createOpenAIClient, sendOpenAIResponse });
await handleReviewCode({ foo: "bar" }, { loadConfig, createProvider });
expect(loadConfig).not.toHaveBeenCalled();
expect(createOpenAIClient).not.toHaveBeenCalled();
expect(sendOpenAIResponse).not.toHaveBeenCalled();
expect(createProvider).not.toHaveBeenCalled();
expect(sendMock).not.toHaveBeenCalled();
});
it("returns structured error when question is empty string", async () => {
const loadConfig = vi.fn();
const createOpenAIClient = vi.fn();
const sendOpenAIResponse = vi.fn();
const sendMock = vi.fn();
const createProvider = vi.fn(() => ({ send: sendMock }));
const result = await handleReviewCode({ question: "" }, { loadConfig, createOpenAIClient, sendOpenAIResponse });
const result = await handleReviewCode({ question: "", context: "x" }, { loadConfig, createProvider });
expect(result.ok).toBe(false);
expect(typeof result.error).toBe("string");
expect(result.warnings).toEqual([]);
@@ -90,10 +90,10 @@ describe("validation failure", () => {
it("returns structured error when question is wrong type", async () => {
const loadConfig = vi.fn();
const createOpenAIClient = vi.fn();
const sendOpenAIResponse = vi.fn();
const sendMock = vi.fn();
const createProvider = vi.fn(() => ({ send: sendMock }));
const result = await handleReviewCode({ question: 123 }, { loadConfig, createOpenAIClient, sendOpenAIResponse });
const result = await handleReviewCode({ question: 123, context: "x" }, { loadConfig, createProvider });
expect(result.ok).toBe(false);
expect(typeof result.error).toBe("string");
expect(result.warnings).toEqual([]);
@@ -105,69 +105,69 @@ describe("validation failure", () => {
describe("config failure", () => {
it("returns structured error when loadConfig throws", async () => {
const loadConfig = vi.fn(() => { throw new Error("OPENAI_API_KEY is missing."); });
const createOpenAIClient = vi.fn();
const sendOpenAIResponse = vi.fn();
const sendMock = vi.fn();
const createProvider = vi.fn(() => ({ send: sendMock }));
const result = await handleReviewCode(makeValidInput("hi"), { loadConfig, createOpenAIClient, sendOpenAIResponse });
const result = await handleReviewCode(makeValidInput("hi"), { loadConfig, createProvider });
expect(result.ok).toBe(false);
expect(result.error).toContain("OPENAI_API_KEY is missing");
});
it("short-circuits — no client or response calls after config failure", async () => {
it("short-circuits — no provider or send calls after config failure", async () => {
const loadConfig = vi.fn(() => { throw new Error("No key."); });
const createOpenAIClient = vi.fn();
const sendOpenAIResponse = vi.fn();
const sendMock = vi.fn();
const createProvider = vi.fn(() => ({ send: sendMock }));
await handleReviewCode(makeValidInput("hi"), { loadConfig, createOpenAIClient, sendOpenAIResponse });
expect(createOpenAIClient).not.toHaveBeenCalled();
expect(sendOpenAIResponse).not.toHaveBeenCalled();
await handleReviewCode(makeValidInput("hi"), { loadConfig, createProvider });
expect(createProvider).not.toHaveBeenCalled();
expect(sendMock).not.toHaveBeenCalled();
});
it("passes original error message", async () => {
const loadConfig = vi.fn(() => { throw new Error("OPENAI_API_KEY is missing."); });
const createOpenAIClient = vi.fn();
const sendOpenAIResponse = vi.fn();
const sendMock = vi.fn();
const createProvider = vi.fn(() => ({ send: sendMock }));
const result = await handleReviewCode(makeValidInput("hi"), { loadConfig, createOpenAIClient, sendOpenAIResponse });
const result = await handleReviewCode(makeValidInput("hi"), { loadConfig, createProvider });
expect(result.error).toBe("Error: OPENAI_API_KEY is missing.");
});
});
// --- Budget failure (short-circuit before prompt/client) ---
// --- Budget failure (short-circuit before prompt/provider) ---
describe("budget failure", () => {
it("returns structured error with budget warnings when input exceeds budget", async () => {
const tinyBudget = { ...mockConfig, maxInputChars: 0 };
const loadConfig = vi.fn(() => tinyBudget);
const createOpenAIClient = vi.fn();
const sendOpenAIResponse = vi.fn();
const sendMock = vi.fn();
const createProvider = vi.fn(() => ({ send: sendMock }));
const result = await handleReviewCode(makeValidInput("hi"), { loadConfig, createOpenAIClient, sendOpenAIResponse });
const result = await handleReviewCode(makeValidInput("hi"), { loadConfig, createProvider });
expect(result.ok).toBe(false);
expect(typeof result.error).toBe("string");
expect(Array.isArray(result.warnings)).toBe(true);
});
it("short-circuits — no prompt built, no client created, no response sent", async () => {
it("short-circuits — no prompt built, no provider created, no send called", async () => {
const tinyBudget = { ...mockConfig, maxInputChars: 0 };
const loadConfig = vi.fn(() => tinyBudget);
const createOpenAIClient = vi.fn();
const sendOpenAIResponse = vi.fn();
const sendMock = vi.fn();
const createProvider = vi.fn(() => ({ send: sendMock }));
await handleReviewCode(makeValidInput("hi"), { loadConfig, createOpenAIClient, sendOpenAIResponse });
expect(createOpenAIClient).not.toHaveBeenCalled();
expect(sendOpenAIResponse).not.toHaveBeenCalled();
await handleReviewCode(makeValidInput("hi"), { loadConfig, createProvider });
expect(createProvider).not.toHaveBeenCalled();
expect(sendMock).not.toHaveBeenCalled();
});
it("passes budget warnings through to the error result", async () => {
const tinyBudget = { ...mockConfig, maxInputChars: 0 };
const loadConfig = vi.fn(() => tinyBudget);
const createOpenAIClient = vi.fn();
const sendOpenAIResponse = vi.fn();
const sendMock = vi.fn();
const createProvider = vi.fn(() => ({ send: sendMock }));
const result = await handleReviewCode(
{ question: "x", context: "a".repeat(20) },
{ loadConfig, createOpenAIClient, sendOpenAIResponse },
{ loadConfig, createProvider },
);
expect(result.ok).toBe(false);
@@ -175,69 +175,69 @@ describe("budget failure", () => {
});
});
// --- Client creation failure ---
// --- Provider creation failure ---
describe("client creation failure", () => {
it("returns structured error when createOpenAIClient throws", async () => {
describe("provider creation failure", () => {
it("returns structured error when createProvider throws", async () => {
const loadConfig = vi.fn(() => mockConfig);
const createOpenAIClient = vi.fn(() => { throw new Error("Invalid config."); });
const sendOpenAIResponse = vi.fn();
const sendMock = vi.fn();
const createProvider = vi.fn(() => { throw new Error("Invalid config."); });
const result = await handleReviewCode(makeValidInput("hi"), { loadConfig, createOpenAIClient, sendOpenAIResponse });
const result = await handleReviewCode(makeValidInput("hi"), { loadConfig, createProvider });
expect(result.ok).toBe(false);
expect(typeof result.error).toBe("string");
expect(result.warnings).toEqual([]);
});
it("short-circuits — no response sent after client failure", async () => {
it("short-circuits — no send called after provider creation failure", async () => {
const loadConfig = vi.fn(() => mockConfig);
const createOpenAIClient = vi.fn(() => { throw new Error("Invalid config."); });
const sendOpenAIResponse = vi.fn();
const sendMock = vi.fn();
const createProvider = vi.fn(() => { throw new Error("Invalid config."); });
await handleReviewCode(makeValidInput("hi"), { loadConfig, createOpenAIClient, sendOpenAIResponse });
expect(sendOpenAIResponse).not.toHaveBeenCalled();
await handleReviewCode(makeValidInput("hi"), { loadConfig, createProvider });
expect(sendMock).not.toHaveBeenCalled();
});
it("passes original error message unchanged", async () => {
const loadConfig = vi.fn(() => mockConfig);
const createOpenAIClient = vi.fn(() => { throw new Error("Invalid config."); });
const sendOpenAIResponse = vi.fn();
const sendMock = vi.fn();
const createProvider = vi.fn(() => { throw new Error("Invalid config."); });
const result = await handleReviewCode(makeValidInput("hi"), { loadConfig, createOpenAIClient, sendOpenAIResponse });
const result = await handleReviewCode(makeValidInput("hi"), { loadConfig, createProvider });
expect(result.error).toBe("Error: Invalid config.");
});
});
// --- OpenAI failure (pass-through) ---
// --- Provider send failure (pass-through) ---
describe("OpenAI failure", () => {
describe("provider send failure", () => {
it("passes through err.message unchanged", async () => {
const loadConfig = vi.fn(() => mockConfig);
const createOpenAIClient = vi.fn(() => ({ responses: { create: vi.fn() } }));
const sendOpenAIResponse = vi.fn().mockRejectedValue(new Error("API key invalid."));
const sendMock = vi.fn().mockRejectedValue(new Error("API key invalid."));
const createProvider = vi.fn(() => ({ send: sendMock }));
const result = await handleReviewCode(makeValidInput("hi"), { loadConfig, createOpenAIClient, sendOpenAIResponse });
const result = await handleReviewCode(makeValidInput("hi"), { loadConfig, createProvider });
expect(result.ok).toBe(false);
expect(result.error).toBe("Error: API key invalid.");
expect(result.warnings).toEqual([]);
});
it("short-circuits — no extra processing after API failure", async () => {
it("short-circuits — no extra processing after provider send failure", async () => {
const loadConfig = vi.fn(() => mockConfig);
const createOpenAIClient = vi.fn(() => ({ responses: { create: vi.fn() } }));
const sendOpenAIResponse = vi.fn().mockRejectedValue(new Error("rate limit"));
const sendMock = vi.fn().mockRejectedValue(new Error("rate limit"));
const createProvider = vi.fn(() => ({ send: sendMock }));
const result = await handleReviewCode(makeValidInput("hi"), { loadConfig, createOpenAIClient, sendOpenAIResponse });
const result = await handleReviewCode(makeValidInput("hi"), { loadConfig, createProvider });
expect(result.ok).toBe(false);
expect(result.error).toBe("Error: rate limit");
});
it("does not wrap or reformat the error", async () => {
const loadConfig = vi.fn(() => mockConfig);
const createOpenAIClient = vi.fn(() => ({ responses: { create: vi.fn() } }));
const sendOpenAIResponse = vi.fn().mockRejectedValue(new Error("429 Too Many Requests"));
const sendMock = vi.fn().mockRejectedValue(new Error("429 Too Many Requests"));
const createProvider = vi.fn(() => ({ send: sendMock }));
const result = await handleReviewCode(makeValidInput("hi"), { loadConfig, createOpenAIClient, sendOpenAIResponse });
const result = await handleReviewCode(makeValidInput("hi"), { loadConfig, createProvider });
expect(result.error).toBe("Error: 429 Too Many Requests");
});
});
@@ -245,60 +245,35 @@ describe("OpenAI failure", () => {
// --- Dependency call order ---
describe("dependency call order", () => {
it("calls deps in correct order: config -> client -> response", async () => {
it("calls deps in correct order: config -> provider -> send", async () => {
const callLog = [];
const loadConfig = vi.fn(() => { callLog.push("config"); return mockConfig; });
const createOpenAIClient = vi.fn(() => { callLog.push("client"); return { responses: { create: vi.fn() } }; });
const sendOpenAIResponse = vi.fn(async () => { callLog.push("response"); return { content: "OK" }; });
const createProvider = vi.fn(() => { callLog.push("provider"); return { send: vi.fn().mockImplementation(async () => { callLog.push("send"); return { content: "OK" }; }) }; });
await handleReviewCode(makeValidInput("hi"), { loadConfig, createOpenAIClient, sendOpenAIResponse });
expect(callLog).toEqual(["config", "client", "response"]);
});
});
// --- buildReviewCodePrompt called with correct input ---
describe("buildReviewCodePrompt integration", () => {
it("calls buildReviewCodePrompt with budget.input (trimmed data)", async () => {
const loadConfig = vi.fn(() => mockConfig);
const createOpenAIClient = vi.fn(() => ({ responses: { create: vi.fn() } }));
const sendOpenAIResponse = vi.fn(async () => ({ content: "OK" }));
// Spy on buildReviewCodePrompt import by capturing the prompt arg.
const promptCapture = [];
const patchedSend = vi.fn(async (client, params) => {
promptCapture.push(params.input[0].content);
return { content: "OK" };
});
await handleReviewCode(makeValidInput("test question"), {
loadConfig, createOpenAIClient, sendOpenAIResponse: patchedSend,
});
expect(promptCapture.length).toBe(1);
expect(typeof promptCapture[0]).toBe("string");
await handleReviewCode(makeValidInput("hi"), { loadConfig, createProvider });
expect(callLog).toEqual(["config", "provider", "send"]);
});
});
// --- No throws escaping ---
describe("no throws escaping", () => {
it("returns structured result when sendOpenAIResponse throws null", async () => {
it("returns structured result when send throws null", async () => {
const loadConfig = vi.fn(() => mockConfig);
const createOpenAIClient = vi.fn(() => ({ responses: { create: vi.fn() } }));
const sendOpenAIResponse = vi.fn().mockRejectedValue(null);
const sendMock = vi.fn().mockRejectedValue(null);
const createProvider = vi.fn(() => ({ send: sendMock }));
const result = await handleReviewCode(makeValidInput("hi"), { loadConfig, createOpenAIClient, sendOpenAIResponse });
const result = await handleReviewCode(makeValidInput("hi"), { loadConfig, createProvider });
expect(result.ok).toBe(false);
expect(typeof result.error).toBe("string");
});
it("returns structured result when loadConfig throws non-Error", async () => {
const loadConfig = vi.fn(() => { throw "string error"; });
const createOpenAIClient = vi.fn();
const sendOpenAIResponse = vi.fn();
const sendMock = vi.fn();
const createProvider = vi.fn(() => ({ send: sendMock }));
const result = await handleReviewCode(makeValidInput("hi"), { loadConfig, createOpenAIClient, sendOpenAIResponse });
const result = await handleReviewCode(makeValidInput("hi"), { loadConfig, createProvider });
expect(result.ok).toBe(false);
expect(typeof result.error).toBe("string");
});
@@ -310,12 +285,12 @@ describe("warning propagation", () => {
it("includes budget warnings in success result when budget passes with warnings", async () => {
const trimmedBudget = { ...mockConfig, maxInputChars: 50 };
const loadConfig = vi.fn(() => trimmedBudget);
const createOpenAIClient = vi.fn(() => ({ responses: { create: vi.fn() } }));
const sendOpenAIResponse = vi.fn(async () => ({ content: "OK" }));
const sendMock = vi.fn(async () => ({ content: "OK" }));
const createProvider = vi.fn(() => ({ send: sendMock }));
const result = await handleReviewCode(
{ question: "hi", context: "x".repeat(49) },
{ loadConfig, createOpenAIClient, sendOpenAIResponse },
{ loadConfig, createProvider },
);
expect(result.ok).toBe(true);
@@ -325,10 +300,10 @@ describe("warning propagation", () => {
it("includes budget warnings in failure result when budget fails", async () => {
const emptyBudget = { ...mockConfig, maxInputChars: 0 };
const loadConfig = vi.fn(() => emptyBudget);
const createOpenAIClient = vi.fn();
const sendOpenAIResponse = vi.fn();
const sendMock = vi.fn();
const createProvider = vi.fn(() => ({ send: sendMock }));
const result = await handleReviewCode(makeValidInput("hi"), { loadConfig, createOpenAIClient, sendOpenAIResponse });
const result = await handleReviewCode(makeValidInput("hi"), { loadConfig, createProvider });
expect(result.ok).toBe(false);
expect(Array.isArray(result.warnings)).toBe(true);
});
@@ -339,10 +314,10 @@ describe("warning propagation", () => {
describe("result shape", () => {
it("returns exactly { ok, answer, warnings } on success", async () => {
const loadConfig = vi.fn(() => mockConfig);
const createOpenAIClient = vi.fn(() => ({ responses: { create: vi.fn() } }));
const sendOpenAIResponse = vi.fn(async () => ({ content: "OK" }));
const sendMock = vi.fn(async () => ({ content: "OK" }));
const createProvider = vi.fn(() => ({ send: sendMock }));
const result = await handleReviewCode(makeValidInput("hi"), { loadConfig, createOpenAIClient, sendOpenAIResponse });
const result = await handleReviewCode(makeValidInput("hi"), { loadConfig, createProvider });
expect(Object.keys(result).sort()).toEqual(["answer", "ok", "warnings"]);
expect(result.ok).toBe(true);
expect(typeof result.answer).toBe("string");
@@ -351,10 +326,10 @@ describe("result shape", () => {
it("returns exactly { ok, error, warnings } on failure", async () => {
const loadConfig = vi.fn(() => { throw new Error("fail"); });
const createOpenAIClient = vi.fn();
const sendOpenAIResponse = vi.fn();
const sendMock = vi.fn();
const createProvider = vi.fn(() => ({ send: sendMock }));
const result = await handleReviewCode(makeValidInput("hi"), { loadConfig, createOpenAIClient, sendOpenAIResponse });
const result = await handleReviewCode(makeValidInput("hi"), { loadConfig, createProvider });
expect(Object.keys(result).sort()).toEqual(["error", "ok", "warnings"]);
expect(result.ok).toBe(false);
expect(typeof result.error).toBe("string");
@@ -367,23 +342,23 @@ describe("result shape", () => {
describe("short-circuit behavior", () => {
it("stops at first failure without calling downstream deps", async () => {
const loadConfig = vi.fn(() => mockConfig);
const createOpenAIClient = vi.fn(() => ({ responses: { create: vi.fn() } }));
const sendOpenAIResponse = vi.fn().mockRejectedValue(new Error("boom"));
const sendMock = vi.fn().mockRejectedValue(new Error("boom"));
const createProvider = vi.fn(() => ({ send: sendMock }));
await handleReviewCode(makeValidInput("hi"), { loadConfig, createOpenAIClient, sendOpenAIResponse });
await handleReviewCode(makeValidInput("hi"), { loadConfig, createProvider });
expect(loadConfig).toHaveBeenCalledTimes(1);
expect(createOpenAIClient).toHaveBeenCalledTimes(1);
expect(sendOpenAIResponse).toHaveBeenCalledTimes(1);
expect(createProvider).toHaveBeenCalledTimes(1);
expect(sendMock).toHaveBeenCalledTimes(1);
});
it("stops at config failure without calling downstream deps", async () => {
const loadConfig = vi.fn(() => { throw new Error("fail"); });
const createOpenAIClient = vi.fn();
const sendOpenAIResponse = vi.fn();
const sendMock = vi.fn();
const createProvider = vi.fn(() => ({ send: sendMock }));
await handleReviewCode(makeValidInput("hi"), { loadConfig, createOpenAIClient, sendOpenAIResponse });
await handleReviewCode(makeValidInput("hi"), { loadConfig, createProvider });
expect(loadConfig).toHaveBeenCalledTimes(1);
expect(createOpenAIClient).not.toHaveBeenCalled();
expect(sendOpenAIResponse).not.toHaveBeenCalled();
expect(createProvider).not.toHaveBeenCalled();
expect(sendMock).not.toHaveBeenCalled();
});
});