feat: add review_code tool handler

This commit is contained in:
2026-06-12 09:20:09 +01:00
parent 4c3776e2ce
commit 8e95ea0e5f
5 changed files with 506 additions and 9 deletions
+7 -5
View File
@@ -41,15 +41,17 @@ Build the MCP tool handlers that:
- Task 4.1 — ask_chatgpt handler (`src/tools/ask-chatgpt.js`, `test/tools/ask-chatgpt.test.js`) ✅
- Task 4.2 — review_plan handler (`src/tools/review-plan.js`, `test/tools/review-plan.test.js`) ✅
- Task 4.3 — review_code handler (`src/tools/review-code.js`, `test/tools/review-code.test.js`) ✅
## Next Pending
### Task 4.3 - review_code tool handler
### Task 4.3 - review_code tool handler
Build the review_code MCP tool handler:
1. Create `src/tools/review-code.js` with dependency-injected `handleReviewCode(input, deps)`
2. Create `test/tools/review-code.test.js` with ~28 orchestration-only tests
3. Follow the same pattern as 4.1 and 4.2
Built `src/tools/review-code.js` with dependency-injected `handleReviewCode(input, deps)` and 28 orchestration-only tests in `test/tools/review-code.test.js`. Followed the same pattern as 4.1 and 4.2. All 28 tests pass.
### Next Pending
- Task 4.4 — debug_issue tool handler (`src/tools/debug-issue.js`, `test/tools/debug-issue.test.js`)
## General Rules
+17 -3
View File
@@ -7,7 +7,7 @@ ChatGPT MCP Server
## Status
Planning complete.
Phase 0 complete. Phase 1 complete. Phase 2 complete. Phase 3 complete. Task 4.1 complete. Task 4.2 complete.
Phase 0 complete. Phase 1 complete. Phase 2 complete. Phase 3 complete. Task 4.1 complete. Task 4.2 complete. Task 4.3 complete.
## Current Phase
@@ -32,6 +32,8 @@ Phase 4 - Tool Handlers
- Task 3.6 — debug_issue prompt builder (`src/prompts/debug-issue.js`, `test/prompts/debug-issue.test.js`) ✅
- Task 3.7 — architecture_review prompt builder (`src/prompts/architecture-review.js`, `test/prompts/architecture-review.test.js`) ✅
- Task 4.1 — ask_chatgpt MCP tool handler (`src/tools/ask-chatgpt.js`, `test/tools/ask-chatgpt.test.js`) ✅
- Task 4.2 — review_plan MCP tool handler (`src/tools/review-plan.js`, `test/tools/review-plan.test.js`) ✅
- Task 4.3 — review_code MCP tool handler (`src/tools/review-code.js`, `test/tools/review-code.test.js`) ✅
## Phase 3 Completion Summary
@@ -81,9 +83,21 @@ Implemented `src/tools/review-plan.js` with dependency-injected handler and 28 o
- Every path returns structured `{ ok, answer|error, warnings }` — never throws to caller
- 28 tests mirroring 4.1 structure with one additional test verifying buildReviewPlanPrompt is called with budget.input
**Task 4.3 — review_code MCP Tool Handler ✅**
Implemented `src/tools/review-code.js` with dependency-injected handler and 28 orchestration-only tests in `test/tools/review-code.test.js`.
**Key design decisions:**
- Execution order: validate → loadConfig → checkContextBudget → buildReviewCodePrompt → createOpenAIClient → sendOpenAIResponse
- All external deps injected (loadConfig, createOpenAIClient, sendOpenAIResponse); internal utilities imported directly
- OpenAI errors pass through `String(err)` unchanged — no wrapping or reformatting
- Budget check short-circuits before prompt building or client creation
- Every path returns structured `{ ok, answer|error, warnings }` — never throws to caller
- 28 tests mirroring 4.1/4.2 structure with one test verifying buildReviewCodePrompt is called with budget.input
**Next pending task:**
- Task 4.3review_code tool handler (`src/tools/review-code.js`, `test/tools/review-code.test.js`)
- Task 4.4debug_issue tool handler (`src/tools/debug-issue.js`, `test/tools/debug-issue.test.js`)
**Not done yet (Phase 4):**
- No MCP tool registration
- Other tool handlers (review_code, debug_issue, architecture_review)
- Other tool handlers (debug_issue, architecture_review)
+15
View File
@@ -342,6 +342,21 @@ Create `src/tools/review-code.js` and `test/tools/review-code.test.js`.
- OpenAI errors pass through `String(err)` unchanged (no wrapping/reformatting).
- Mirror 4.1 and 4.2 structure: ~28 orchestration-only tests covering the same test categories.
Status: ✅ Complete
### Task 4.4 - debug_issue tool handler (NEXT)
Create `src/tools/debug-issue.js` and `test/tools/debug-issue.test.js`.
**Requirements:**
- Export `handleDebugIssue(input, deps)` as standalone dependency-injected function.
- Execution order: validateToolInput → loadConfig → checkContextBudget → buildDebugIssuePrompt → createOpenAIClient → sendOpenAIResponse.
- Inject only 3 external deps: `loadConfig`, `createOpenAIClient`, `sendOpenAIResponse`. Internal utilities imported directly.
- If budget check fails (`ok: false`), immediately return that result — do not call buildDebugIssuePrompt or create client.
- All paths return structured `{ ok, answer|error, warnings }` — never throws to caller.
- OpenAI errors pass through `String(err)` unchanged (no wrapping/reformatting).
- Mirror 4.1/4.2/4.3 structure: ~28 orchestration-only tests covering the same test categories.
Status: ⬜ Pending
---
+78 -1
View File
@@ -1 +1,78 @@
// review_code tool handler.
// Tool handler for the review_code MCP tool.
import { validateToolInput } from "./schemas.js";
import { checkContextBudget } from "../utils/context-budget.js";
import { buildReviewCodePrompt } from "../prompts/review-code.js";
/**
* Handle the review_code MCP tool.
*
* Orchestration order: validate -> config -> budget -> prompt -> client -> response.
* All external dependencies injected via deps. Internal utilities imported directly.
* No throws escape — all paths return structured results.
*
* @param {unknown} input
* Raw tool input per ARCHITECTURE.md §7 schema.
* @param {{
* loadConfig: () => object,
* createOpenAIClient: (config: object) => any,
* sendOpenAIResponse: (client: any, params: object) => Promise<any>
* }} deps
* Injected external dependencies.
* @returns {Promise<{ ok: true, answer: string, warnings: string[] } | { ok: false, error: string, warnings: string[] }>}
*/
export async function handleReviewCode(input, deps) {
// --- 1. Validate input (before anything else) ---
const validation = validateToolInput(input);
if (!validation.ok) {
return { ok: false, error: validation.errors.join(" | "), warnings: [] };
}
// --- 2. Load config ---
let config;
try {
config = deps.loadConfig();
} catch (err) {
return { ok: false, error: String(err), warnings: [] };
}
// --- 3. Context budget check ---
const budget = checkContextBudget(validation.data, config);
if (!budget.ok) {
return { ok: false, error: budget.error, warnings: budget.warnings };
}
// --- 4. Build prompt ---
const promptMessages = buildReviewCodePrompt(budget.input);
// --- 5. Create OpenAI client ---
let client;
try {
client = deps.createOpenAIClient(config);
} catch (err) {
return { ok: false, error: String(err), warnings: [] };
}
// --- 6. Send to OpenAI ---
let aiResult;
try {
aiResult = await deps.sendOpenAIResponse(client, {
input: [{ role: "system", content: promptMessages }],
model: config.openaiModel,
temperature: config.temperature,
maxOutputTokens: config.maxOutputTokens,
});
} catch (err) {
return { ok: false, error: String(err), warnings: [] };
}
// --- 7. Success ---
return { ok: true, answer: aiResult.content, warnings: budget.warnings };
}
+389
View File
@@ -0,0 +1,389 @@
import { describe, it, expect, vi } from "vitest";
import { handleReviewCode } from "../../src/tools/review-code.js";
const mockConfig = {
openaiApiKey: "sk-test-key",
openaiModel: "gpt-5.1",
temperature: 0.2,
maxOutputTokens: 2000,
logLevel: "info",
enableFileContext: false,
contextDir: "./context",
maxInputChars: 30000,
maxFileChars: 12000,
maxFiles: 5,
maxLogChars: 10000,
redactSecrets: true,
};
function makeValidInput(question) {
return { question };
}
// --- Success path ---
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 result = await handleReviewCode(makeValidInput("Review my code"), {
loadConfig, createOpenAIClient, sendOpenAIResponse,
});
expect(result.ok).toBe(true);
expect(result.answer).toBe("OK");
expect(result.warnings).toEqual([]);
});
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 result = await handleReviewCode(makeValidInput("hi"), {
loadConfig, createOpenAIClient, sendOpenAIResponse,
});
expect(result.ok).toBe(true);
expect(Array.isArray(result.warnings)).toBe(true);
});
});
// --- Validation failure (short-circuit before config) ---
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 result = await handleReviewCode({}, { loadConfig, createOpenAIClient, sendOpenAIResponse });
expect(result.ok).toBe(false);
expect(typeof result.error).toBe("string");
expect(result.warnings).toEqual([]);
});
it("short-circuits — no other deps called", async () => {
const loadConfig = vi.fn();
const createOpenAIClient = vi.fn();
const sendOpenAIResponse = vi.fn();
await handleReviewCode({ foo: "bar" }, { loadConfig, createOpenAIClient, sendOpenAIResponse });
expect(loadConfig).not.toHaveBeenCalled();
expect(createOpenAIClient).not.toHaveBeenCalled();
expect(sendOpenAIResponse).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 result = await handleReviewCode({ question: "" }, { loadConfig, createOpenAIClient, sendOpenAIResponse });
expect(result.ok).toBe(false);
expect(typeof result.error).toBe("string");
expect(result.warnings).toEqual([]);
});
it("returns structured error when question is wrong type", async () => {
const loadConfig = vi.fn();
const createOpenAIClient = vi.fn();
const sendOpenAIResponse = vi.fn();
const result = await handleReviewCode({ question: 123 }, { loadConfig, createOpenAIClient, sendOpenAIResponse });
expect(result.ok).toBe(false);
expect(typeof result.error).toBe("string");
expect(result.warnings).toEqual([]);
});
});
// --- Config 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 result = await handleReviewCode(makeValidInput("hi"), { loadConfig, createOpenAIClient, sendOpenAIResponse });
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 () => {
const loadConfig = vi.fn(() => { throw new Error("No key."); });
const createOpenAIClient = vi.fn();
const sendOpenAIResponse = vi.fn();
await handleReviewCode(makeValidInput("hi"), { loadConfig, createOpenAIClient, sendOpenAIResponse });
expect(createOpenAIClient).not.toHaveBeenCalled();
expect(sendOpenAIResponse).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 result = await handleReviewCode(makeValidInput("hi"), { loadConfig, createOpenAIClient, sendOpenAIResponse });
expect(result.error).toBe("Error: OPENAI_API_KEY is missing.");
});
});
// --- Budget failure (short-circuit before prompt/client) ---
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 result = await handleReviewCode(makeValidInput("hi"), { loadConfig, createOpenAIClient, sendOpenAIResponse });
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 () => {
const tinyBudget = { ...mockConfig, maxInputChars: 0 };
const loadConfig = vi.fn(() => tinyBudget);
const createOpenAIClient = vi.fn();
const sendOpenAIResponse = vi.fn();
await handleReviewCode(makeValidInput("hi"), { loadConfig, createOpenAIClient, sendOpenAIResponse });
expect(createOpenAIClient).not.toHaveBeenCalled();
expect(sendOpenAIResponse).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 result = await handleReviewCode(
{ question: "x", context: "a".repeat(20) },
{ loadConfig, createOpenAIClient, sendOpenAIResponse },
);
expect(result.ok).toBe(false);
expect(Array.isArray(result.warnings)).toBe(true);
});
});
// --- Client creation failure ---
describe("client creation failure", () => {
it("returns structured error when createOpenAIClient throws", async () => {
const loadConfig = vi.fn(() => mockConfig);
const createOpenAIClient = vi.fn(() => { throw new Error("Invalid config."); });
const sendOpenAIResponse = vi.fn();
const result = await handleReviewCode(makeValidInput("hi"), { loadConfig, createOpenAIClient, sendOpenAIResponse });
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 () => {
const loadConfig = vi.fn(() => mockConfig);
const createOpenAIClient = vi.fn(() => { throw new Error("Invalid config."); });
const sendOpenAIResponse = vi.fn();
await handleReviewCode(makeValidInput("hi"), { loadConfig, createOpenAIClient, sendOpenAIResponse });
expect(sendOpenAIResponse).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 result = await handleReviewCode(makeValidInput("hi"), { loadConfig, createOpenAIClient, sendOpenAIResponse });
expect(result.error).toBe("Error: Invalid config.");
});
});
// --- OpenAI failure (pass-through) ---
describe("OpenAI 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 result = await handleReviewCode(makeValidInput("hi"), { loadConfig, createOpenAIClient, sendOpenAIResponse });
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 () => {
const loadConfig = vi.fn(() => mockConfig);
const createOpenAIClient = vi.fn(() => ({ responses: { create: vi.fn() } }));
const sendOpenAIResponse = vi.fn().mockRejectedValue(new Error("rate limit"));
const result = await handleReviewCode(makeValidInput("hi"), { loadConfig, createOpenAIClient, sendOpenAIResponse });
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 result = await handleReviewCode(makeValidInput("hi"), { loadConfig, createOpenAIClient, sendOpenAIResponse });
expect(result.error).toBe("Error: 429 Too Many Requests");
});
});
// --- Dependency call order ---
describe("dependency call order", () => {
it("calls deps in correct order: config -> client -> response", 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" }; });
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");
});
});
// --- No throws escaping ---
describe("no throws escaping", () => {
it("returns structured result when sendOpenAIResponse throws null", async () => {
const loadConfig = vi.fn(() => mockConfig);
const createOpenAIClient = vi.fn(() => ({ responses: { create: vi.fn() } }));
const sendOpenAIResponse = vi.fn().mockRejectedValue(null);
const result = await handleReviewCode(makeValidInput("hi"), { loadConfig, createOpenAIClient, sendOpenAIResponse });
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 result = await handleReviewCode(makeValidInput("hi"), { loadConfig, createOpenAIClient, sendOpenAIResponse });
expect(result.ok).toBe(false);
expect(typeof result.error).toBe("string");
});
});
// --- Warning propagation ---
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 result = await handleReviewCode(
{ question: "hi", context: "x".repeat(49) },
{ loadConfig, createOpenAIClient, sendOpenAIResponse },
);
expect(result.ok).toBe(true);
expect(Array.isArray(result.warnings)).toBe(true);
});
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 result = await handleReviewCode(makeValidInput("hi"), { loadConfig, createOpenAIClient, sendOpenAIResponse });
expect(result.ok).toBe(false);
expect(Array.isArray(result.warnings)).toBe(true);
});
});
// --- Result shape ---
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 result = await handleReviewCode(makeValidInput("hi"), { loadConfig, createOpenAIClient, sendOpenAIResponse });
expect(Object.keys(result).sort()).toEqual(["answer", "ok", "warnings"]);
expect(result.ok).toBe(true);
expect(typeof result.answer).toBe("string");
expect(Array.isArray(result.warnings)).toBe(true);
});
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 result = await handleReviewCode(makeValidInput("hi"), { loadConfig, createOpenAIClient, sendOpenAIResponse });
expect(Object.keys(result).sort()).toEqual(["error", "ok", "warnings"]);
expect(result.ok).toBe(false);
expect(typeof result.error).toBe("string");
expect(Array.isArray(result.warnings)).toBe(true);
});
});
// --- Short-circuit behavior ---
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"));
await handleReviewCode(makeValidInput("hi"), { loadConfig, createOpenAIClient, sendOpenAIResponse });
expect(loadConfig).toHaveBeenCalledTimes(1);
expect(createOpenAIClient).toHaveBeenCalledTimes(1);
expect(sendOpenAIResponse).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();
await handleReviewCode(makeValidInput("hi"), { loadConfig, createOpenAIClient, sendOpenAIResponse });
expect(loadConfig).toHaveBeenCalledTimes(1);
expect(createOpenAIClient).not.toHaveBeenCalled();
expect(sendOpenAIResponse).not.toHaveBeenCalled();
});
});