From 42a6dfe539e3febae38cd2d5b6fd98851b17d784 Mon Sep 17 00:00:00 2001 From: robbond Date: Thu, 11 Jun 2026 18:19:36 +0100 Subject: [PATCH] feat: add review_code prompt builder --- PROJECT_STATE.md | 3 +- TASKS.md | 20 ++ src/prompts/review-code.js | 98 +++++++- test/prompts/review-code.test.js | 397 +++++++++++++++++++++++++++++++ 4 files changed, 516 insertions(+), 2 deletions(-) create mode 100644 test/prompts/review-code.test.js diff --git a/PROJECT_STATE.md b/PROJECT_STATE.md index 0e02c65..fa2488f 100644 --- a/PROJECT_STATE.md +++ b/PROJECT_STATE.md @@ -28,7 +28,8 @@ Phase 3 in progress. - Task 3.2 — Base prompt template (`src/prompts/base.js`, `test/prompts/base.test.js`) ✅ - Task 3.3 — ask_chatgpt prompt builder (`src/prompts/ask-chatgpt.js`, `test/prompts/ask-chatgpt.test.js`) ✅ - Task 3.4 — review_plan prompt builder (`src/prompts/review-plan.js`, `test/prompts/review-plan.test.js`) ✅ +- Task 3.5 — review_code prompt builder (`src/prompts/review-code.js`, `test/prompts/review-code.test.js`) ✅ ## Next Phase -Phase 3 — in progress; next is Task 3.5 (review_code prompt builder). +Phase 3 — in progress; next is Task 3.6 (debug_issue prompt builder). diff --git a/TASKS.md b/TASKS.md index b887edf..0a91379 100644 --- a/TASKS.md +++ b/TASKS.md @@ -228,6 +228,26 @@ Create `test/prompts/review-code.test.js`. Requirements: - Mirror structure of ask-chatgpt and review-plan tests: base composition, review_code-specific instructions, optional fields, expectedOutput, input validation, tool guard, full integration. +Status: ✅ Complete + +### Task 3.6 - debug_issue prompt builder (NEXT) + +Create `src/prompts/debug-issue.js`. + +Requirements: +- Export a `buildDebugIssuePrompt(input)` function that returns a tool-specific prompt for the debug_issue MCP tool. +- Compose with `buildBasePrompt()` — call it internally and append the result with `---` separator. +- Instruct ChatGPT to analyse errors, logs, failed tests, or stack traces. +- Look for: likely root causes, fast checks, minimal safe experiments, what not to change yet. +- Request response in structured format: Likely Causes, Fast Checks, Minimal Safe Experiments, What Not To Do Yet. +- Support `relevantFiles` and `logs` input fields. +- No other tool prompts yet (architecture_review, etc.). + +Create `test/prompts/debug-issue.test.js`. + +Requirements: +- Mirror structure of ask_chatgpt, review-plan, and review-code tests: base composition, debug_issue-specific instructions, optional fields, expectedOutput, input validation, tool guard, full integration. + Status: ⬜ Pending --- diff --git a/src/prompts/review-code.js b/src/prompts/review-code.js index d9a6335..78bb77d 100644 --- a/src/prompts/review-code.js +++ b/src/prompts/review-code.js @@ -1 +1,97 @@ -// review_code prompt template. +// Tool-specific prompt builder for review_code. + +import { buildBasePrompt } from "./base.js"; + +/** + * Build a tool-specific prompt for the review_code MCP tool. + * + * @param {{ question: string, context?: string, constraints?: string[], expectedOutput?: string, projectSummary?: string, taskSummary?: string, relevantFiles?: Array<{path: string, content: string, language?: string}> }} input + * @returns {string} A two-part prompt: base system message + review_code user message. + */ +export function buildReviewCodePrompt(input) { + if (!input || typeof input.question !== "string" || input.question.length === 0) { + const err = new TypeError("review_code requires a non-empty 'question' field."); + err.kind = "ValidationError"; + throw err; + } + + const base = buildBasePrompt(); + + const lines = [ + "", + "---", + "", + "You are acting as a code reviewer for Claude Code. Claude Code will implement your suggestions — you do not implement anything yourself.", + "", + "Your task: review the supplied code only and provide a focused, actionable assessment.", + "", + "Review across these dimensions:", + "- Correctness — bugs, logic errors, off-by-one, type coercion issues.", + "- Edge cases — boundary values, empty/null inputs, unexpected states.", + "- Security — injection, auth bypass, data exposure, unsafe patterns.", + "- Maintainability — readability, naming, cohesion, testability.", + "- Unnecessary complexity — over-engineering, nested conditionals, abstractions that add no value.", + "- Performance — obvious inefficiencies when relevant (N+1, unnecessary allocations, blocking ops).", + "- Missing validation or error handling — unchecked inputs, swallowed errors, missing catch blocks.", + "- Simpler approaches — more concise or idiomatic alternatives.", + "", + "Important rules:", + "- Claude Code remains the implementation agent. You are acting only as a reviewer.", + "- Review only the supplied files and snippets. Do not assume access to the full repository.", + "- Do not make assumptions about files, configuration, or architecture that you have not been given.", + "- Use only the context provided — call out uncertainty clearly — do not guess about unprovided information.", + "", + "Respond in this exact structure:", + "- Summary: Brief overall assessment of the code quality.", + "- Issues by Severity: Grouped into High, Medium, and Low.", + "- Suggested Fixes: Small, reviewable changes with brief rationale.", + "- Test Suggestions: Specific test cases that should be added or updated.", + "", + ]; + + lines.push(`Question: ${input.question}`); + + if (input.context) { + lines.push(""); + lines.push(`Context: ${input.context}`); + } + + if (input.constraints && input.constraints.length > 0) { + lines.push(""); + lines.push("Constraints:"); + for (const c of input.constraints) { + lines.push(`- ${c}`); + } + } + + if (input.expectedOutput) { + lines.push(""); + lines.push(`Expected output: ${input.expectedOutput}`); + } + + if (input.projectSummary) { + lines.push(""); + lines.push(`Project summary: ${input.projectSummary}`); + } + + if (input.taskSummary) { + lines.push(""); + lines.push(`Task summary: ${input.taskSummary}`); + } + + if (input.relevantFiles && input.relevantFiles.length > 0) { + lines.push(""); + lines.push("Relevant files:"); + + for (const file of input.relevantFiles) { + lines.push(""); + lines.push(`File: ${file.path}`); + if (file.language) { + lines.push(`Language: ${file.language}`); + } + lines.push(file.content); + } + } + + return base + "\n" + lines.join("\n"); +} diff --git a/test/prompts/review-code.test.js b/test/prompts/review-code.test.js new file mode 100644 index 0000000..79b13d4 --- /dev/null +++ b/test/prompts/review-code.test.js @@ -0,0 +1,397 @@ +import { describe, it, expect } from "vitest"; +import { buildReviewCodePrompt } from "../../src/prompts/review-code.js"; + +describe("buildReviewCodePrompt", () => { + // --- Basic output contract --- + + it("returns a non-empty string given question-only input", () => { + const result = buildReviewCodePrompt({ question: "Review this code" }); + expect(typeof result).toBe("string"); + expect(result.length).toBeGreaterThan(0); + }); + + it("is deterministic — same input always produces the same output", () => { + const a = buildReviewCodePrompt({ question: "Review this code" }); + const b = buildReviewCodePrompt({ question: "Review this code" }); + expect(a).toBe(b); + }); + + // --- Composition with base prompt --- + + it("includes the base system prompt content", () => { + const result = buildReviewCodePrompt({ question: "Test" }); + expect(result).toContain("second-opinion assistant"); + expect(result).toContain("Claude Code"); + expect(result).toContain("not responsible for editing files"); + }); + + it("contains a '---' separator between base and tool-specific sections", () => { + const result = buildReviewCodePrompt({ question: "Test" }); + const parts = result.split("\n---\n"); + expect(parts.length).toBe(2); + }); + + // --- review_code-specific role instructions --- + + it("contains review_code-specific role instruction (code reviewer)", () => { + const result = buildReviewCodePrompt({ question: "Test" }); + expect(result).toContain("code reviewer"); + }); + + it("instructs Claude Code as the implementation agent", () => { + const result = buildReviewCodePrompt({ question: "Test" }); + expect(result).toContain("Claude Code will implement your suggestions"); + }); + + it("instructs ChatGPT that you do not implement anything yourself", () => { + const result = buildReviewCodePrompt({ question: "Test" }); + expect(result).toContain("you do not implement anything yourself"); + }); + + // --- Claude Code / reviewer guard rails --- + + it("states Claude Code remains the implementation agent", () => { + const result = buildReviewCodePrompt({ question: "Test" }); + expect(result).toContain("Claude Code remains the implementation agent"); + }); + + it("states ChatGPT is acting only as a reviewer", () => { + const result = buildReviewCodePrompt({ question: "Test" }); + expect(result).toContain("acting only as a reviewer"); + }); + + it("instructs to review only the supplied files and snippets", () => { + const result = buildReviewCodePrompt({ question: "Test" }); + expect(result).toContain("Review only the supplied files and snippets"); + }); + + it("instructs not to assume access to the full repository", () => { + const result = buildReviewCodePrompt({ question: "Test" }); + expect(result).toMatch(/do not assume access to the full repository/i); + }); + + it("instructs to call out uncertainty and not guess", () => { + const result = buildReviewCodePrompt({ question: "Test" }); + expect(result.toLowerCase()).toContain("uncertainty"); + expect(result.toLowerCase()).toContain("do not guess"); + }); + + // --- Review dimensions --- + + it("includes correctness dimension", () => { + const result = buildReviewCodePrompt({ question: "Test" }); + expect(result.toLowerCase()).toContain("correctness"); + }); + + it("includes edge cases dimension", () => { + const result = buildReviewCodePrompt({ question: "Test" }); + expect(result.toLowerCase()).toContain("edge case"); + }); + + it("includes security dimension", () => { + const result = buildReviewCodePrompt({ question: "Test" }); + expect(result.toLowerCase()).toContain("security"); + }); + + it("includes maintainability dimension", () => { + const result = buildReviewCodePrompt({ question: "Test" }); + expect(result.toLowerCase()).toContain("maintainability"); + }); + + it("includes unnecessary complexity dimension", () => { + const result = buildReviewCodePrompt({ question: "Test" }); + expect(result.toLowerCase()).toContain("unnecessary complexity"); + }); + + it("includes performance dimension", () => { + const result = buildReviewCodePrompt({ question: "Test" }); + expect(result.toLowerCase()).toContain("performance"); + }); + + it("includes missing validation or error handling dimension", () => { + const result = buildReviewCodePrompt({ question: "Test" }); + expect(result).toContain("Missing validation or error handling"); + }); + + it("includes simpler approaches dimension", () => { + const result = buildReviewCodePrompt({ question: "Test" }); + expect(result.toLowerCase()).toContain("simpler approaches"); + }); + + // --- Response structure (TASKS.md section) --- + + it("requests Summary section in response structure", () => { + const result = buildReviewCodePrompt({ question: "Test" }); + expect(result).toMatch(/summary/i); + }); + + it("requests Issues by Severity section in response structure", () => { + const result = buildReviewCodePrompt({ question: "Test" }); + expect(result).toMatch(/issues\s+by\s+severity/i); + }); + + it("requests Suggested Fixes section in response structure", () => { + const result = buildReviewCodePrompt({ question: "Test" }); + expect(result).toMatch(/suggested\s+fixes/i); + }); + + it("requests Test Suggestions section in response structure", () => { + const result = buildReviewCodePrompt({ question: "Test" }); + expect(result).toMatch(/test\s+suggestions/i); + }); + + // --- Question field --- + + it("appends the input question verbatim in the task section", () => { + const result = buildReviewCodePrompt({ question: "Review pagination implementation" }); + expect(result).toContain("Question: Review pagination implementation"); + }); + + // --- Optional fields appended correctly (with question) --- + + it("appends Context when context is provided", () => { + const result = buildReviewCodePrompt({ + question: "Review this code", + context: "The middleware handles auth.", + }); + expect(result).toContain("Context: The middleware handles auth."); + }); + + it("does NOT append Context section when context is empty", () => { + const result = buildReviewCodePrompt({ question: "Review this code", context: "" }); + expect(result).not.toContain("Context:"); + }); + + it("appends Constraints list when constraints are provided", () => { + const result = buildReviewCodePrompt({ + question: "Review this code", + constraints: ["must be safe", "must not add deps"], + }); + expect(result).toContain("Constraints:"); + expect(result).toContain("- must be safe"); + expect(result).toContain("- must not add deps"); + }); + + it("does NOT append Constraints section when constraints array is empty", () => { + const result = buildReviewCodePrompt({ question: "Review this code", constraints: [] }); + expect(result).not.toContain("Constraints:"); + }); + + it("appends Expected output when expectedOutput is provided", () => { + const result = buildReviewCodePrompt({ + question: "Review auth middleware", + expectedOutput: "A structured code review with severity ratings.", + }); + expect(result).toContain("Expected output: A structured code review with severity ratings."); + }); + + it("does NOT append Expected output section when expectedOutput is omitted", () => { + const result = buildReviewCodePrompt({ question: "Review auth middleware" }); + expect(result).not.toContain("Expected output:"); + }); + + it("does NOT append Expected output section when expectedOutput is empty", () => { + const result = buildReviewCodePrompt({ question: "Test", expectedOutput: "" }); + expect(result).not.toContain("Expected output:"); + }); + + it("appends all optional fields alongside each other", () => { + const result = buildReviewCodePrompt({ + question: "Review auth middleware", + context: "Current system uses session cookies.", + constraints: ["no infra changes", "must support existing IAM"], + expectedOutput: "A checklist-style review.", + projectSummary: "Internal inventory management system.", + taskSummary: "Migrating from sessions to JWT.", + }); + expect(result).toContain("Question: Review auth middleware"); + expect(result).toContain("Context: Current system uses session cookies."); + expect(result).toContain("Constraints:"); + expect(result).toContain("- no infra changes"); + expect(result).toContain("- must support existing IAM"); + expect(result).toContain("Expected output: A checklist-style review."); + expect(result).toContain("Project summary: Internal inventory management system."); + expect(result).toContain("Task summary: Migrating from sessions to JWT."); + }); + + it("does NOT append Project summary when projectSummary is omitted", () => { + const result = buildReviewCodePrompt({ question: "Test" }); + expect(result).not.toContain("Project summary:"); + }); + + it("does NOT append Task summary when taskSummary is omitted", () => { + const result = buildReviewCodePrompt({ question: "Test" }); + expect(result).not.toContain("Task summary:"); + }); + + // --- relevantFiles (present) --- + + it("includes file section header when relevantFiles is provided", () => { + const result = buildReviewCodePrompt({ + question: "Review pagination", + relevantFiles: [{ path: "src/paginate.js", content: "function paginate() {}" }], + }); + expect(result).toContain("Relevant files:"); + }); + + it("includes file path, language, and content when relevantFiles is provided with language", () => { + const result = buildReviewCodePrompt({ + question: "Review pagination", + relevantFiles: [ + { path: "src/paginate.js", language: "javascript", content: "function paginate() {}" }, + ], + }); + expect(result).toContain("File: src/paginate.js"); + expect(result).toContain("Language: javascript"); + expect(result).toContain("function paginate() {}"); + }); + + it("includes file content without language header when language is omitted", () => { + const result = buildReviewCodePrompt({ + question: "Review pagination", + relevantFiles: [{ path: "src/paginate.js", content: "function paginate() {}" }], + }); + expect(result).toContain("File: src/paginate.js"); + expect(result).not.toContain("Language:"); + expect(result).toContain("function paginate() {}"); + }); + + it("includes multiple files when relevantFiles has multiple entries", () => { + const result = buildReviewCodePrompt({ + question: "Review pagination", + relevantFiles: [ + { path: "src/paginate.js", language: "javascript", content: "function paginate() {}" }, + { path: "src/paginate.test.js", language: "javascript", content: "// no tests" }, + ], + }); + expect(result).toContain("File: src/paginate.js"); + expect(result).toContain("File: src/paginate.test.js"); + expect(result).toContain("function paginate() {}"); + expect(result).toContain("// no tests"); + }); + + // --- relevantFiles (absent/empty) --- + + it("does NOT include Relevant files section when relevantFiles is omitted", () => { + const result = buildReviewCodePrompt({ question: "Test" }); + expect(result).not.toContain("Relevant files:"); + }); + + it("does NOT include Relevant files section when relevantFiles is empty array", () => { + const result = buildReviewCodePrompt({ question: "Test", relevantFiles: [] }); + expect(result).not.toContain("Relevant files:"); + }); + + // --- Tool-specific guard --- + + it("does NOT contain ask_chatgpt specific content", () => { + const result = buildReviewCodePrompt({ question: "Test" }); + expect(result.toLowerCase()).not.toContain("answer the question directly"); + expect(result.toLowerCase()).not.toContain("second-opinion advisor"); + }); + + it("does NOT contain review_plan specific content", () => { + const result = buildReviewCodePrompt({ question: "Test" }); + expect(result).not.toContain("review-plan"); + expect(result).not.toContain("missing steps"); + expect(result).not.toContain("scope creep"); + }); + + it("does NOT contain debug_issue specific content", () => { + const result = buildReviewCodePrompt({ question: "Test" }); + expect(result).not.toContain("debug_issue"); + expect(result.toLowerCase()).not.toContain("stack trace"); + }); + + // --- Input validation --- + + it("throws TypeError when input is missing", () => { + expect(() => buildReviewCodePrompt()).toThrow(TypeError); + expect(() => buildReviewCodePrompt(null)).toThrow(TypeError); + }); + + it("throws TypeError when question is missing", () => { + expect(() => buildReviewCodePrompt({ context: "something" })).toThrow(TypeError); + }); + + it("throws TypeError when question is empty", () => { + expect(() => buildReviewCodePrompt({ question: "" })).toThrow(TypeError); + }); + + // --- Full integration test --- + + it("returns a well-formed two-part prompt with all fields and relevantFiles", () => { + const result = buildReviewCodePrompt({ + question: "Review JWT auth middleware for security issues", + context: "Current system uses session cookies on EC2.", + constraints: ["no infra changes", "must support existing IAM"], + expectedOutput: "A structured code review with severity ratings.", + projectSummary: "Internal inventory management system with multi-tenant isolation.", + taskSummary: "Migrating from session-based auth to JWT.", + relevantFiles: [ + { + path: "src/middleware/auth.js", + language: "javascript", + content: "export function auth(req) {\n const token = req.headers['authorization'];\n return token;\n}", + }, + { + path: "src/middleware/auth.test.js", + language: "javascript", + content: "// no tests yet\nimport { auth } from './auth';\n", + }, + ], + }); + + // Base section present + expect(result).toContain("second-opinion assistant"); + expect(result).toContain("Rules:"); + + // Separator + expect(result).toMatch(/\n---\n/); + + // Tool-specific section present + expect(result).toContain("code reviewer"); + expect(result).toContain("Claude Code remains the implementation agent"); + expect(result).toMatch(/do not assume access to the full repository/i); + + // All 8 review dimensions present + expect(result.toLowerCase()).toContain("correctness"); + expect(result.toLowerCase()).toContain("edge case"); + expect(result.toLowerCase()).toContain("security"); + expect(result.toLowerCase()).toContain("maintainability"); + expect(result.toLowerCase()).toContain("unnecessary complexity"); + expect(result.toLowerCase()).toContain("performance"); + expect(result).toContain("Missing validation or error handling"); + expect(result.toLowerCase()).toContain("simpler approaches"); + + // Response structure present + expect(result).toMatch(/summary/i); + expect(result).toMatch(/issues\s+by\s+severity/i); + expect(result).toMatch(/suggested\s+fixes/i); + expect(result).toMatch(/test\s+suggestions/i); + + // All fields present verbatim + expect(result).toContain("Question: Review JWT auth middleware for security issues"); + expect(result).toContain("Context: Current system uses session cookies on EC2."); + expect(result).toContain("Constraints:"); + expect(result).toContain("- no infra changes"); + expect(result).toContain("- must support existing IAM"); + expect(result).toContain("Expected output: A structured code review with severity ratings."); + expect(result).toContain("Project summary: Internal inventory management system with multi-tenant isolation."); + expect(result).toContain("Task summary: Migrating from session-based auth to JWT."); + + // Relevant files present + expect(result).toContain("Relevant files:"); + expect(result).toContain("File: src/middleware/auth.js"); + expect(result).toContain("Language: javascript"); + expect(result).toContain("export function auth(req) {"); + expect(result).toContain("File: src/middleware/auth.test.js"); + expect(result).toContain("// no tests yet"); + + // Does not contain other tool content + expect(result.toLowerCase()).not.toContain("answer the question directly"); + expect(result.toLowerCase()).not.toContain("second-opinion advisor"); + expect(result).not.toContain("missing steps"); + expect(result).not.toContain("scope creep"); + }); +});