diff --git a/src/create-prompt/index.ts b/src/create-prompt/index.ts index 833161d8..3f211b28 100644 --- a/src/create-prompt/index.ts +++ b/src/create-prompt/index.ts @@ -824,7 +824,7 @@ ${ ? `- Use mcp__github_file_ops__commit_files for making commits (works for both new and existing files, single or multiple). Use mcp__github_file_ops__delete_files for deleting files (supports deleting single or multiple files atomically), or mcp__github__delete_file for deleting a single file. Edit files locally, and the tool will read the content from the same path on disk. Tool usage examples: - mcp__github_file_ops__commit_files: {"files": ["path/to/file1.js", "path/to/file2.py"], "message": "feat: add new feature"} - - mcp__github_file_ops__delete_files: {"files": ["path/to/old.js"], "message": "chore: remove deprecated file"}` + - mcp__github_file_ops__delete_files: {"paths": ["path/to/old.js"], "message": "chore: remove deprecated file"}` : `- Use git commands via the Bash tool for version control (remember that you have access to these git commands): - Stage files: Bash(git add ) - Commit changes: Bash(git commit -m "") diff --git a/src/mcp/github-file-ops-schemas.ts b/src/mcp/github-file-ops-schemas.ts new file mode 100644 index 00000000..cf6fa92b --- /dev/null +++ b/src/mcp/github-file-ops-schemas.ts @@ -0,0 +1,24 @@ +import { z } from "zod"; + +/** Raw shape passed to `server.tool` for commit_files. */ +export const commitFilesInputSchema = { + files: z + .array(z.string()) + .describe( + 'Array of file paths relative to repository root (e.g. ["src/main.js", "README.md"]). All files must exist locally.', + ), + message: z.string().describe("Commit message"), +}; + +/** Raw shape passed to `server.tool` for delete_files. */ +export const deleteFilesInputSchema = { + paths: z + .array(z.string()) + .describe( + 'Array of file paths to delete relative to repository root (e.g. ["src/old-file.js", "docs/deprecated.md"])', + ), + message: z.string().describe("Commit message"), +}; + +export const commitFilesPayloadSchema = z.object(commitFilesInputSchema); +export const deleteFilesPayloadSchema = z.object(deleteFilesInputSchema); diff --git a/src/mcp/github-file-ops-server.ts b/src/mcp/github-file-ops-server.ts index 0c274d5b..e341e1d4 100644 --- a/src/mcp/github-file-ops-server.ts +++ b/src/mcp/github-file-ops-server.ts @@ -2,7 +2,6 @@ // GitHub File Operations MCP Server import { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; import { StdioServerTransport } from "@modelcontextprotocol/sdk/server/stdio.js"; -import { z } from "zod"; import { readFile, stat } from "fs/promises"; import { resolve } from "path"; import { constants } from "fs"; @@ -11,6 +10,10 @@ import { GITHUB_API_URL } from "../github/api/config"; import { isBinaryContent } from "./binary-detection"; import { validatePathWithinRepo } from "./path-validation"; import { updateGitReference } from "./update-git-reference"; +import { + commitFilesInputSchema, + deleteFilesInputSchema, +} from "./github-file-ops-schemas"; type GitHubRef = { object: { @@ -197,14 +200,7 @@ async function getFileMode(filePath: string): Promise { server.tool( "commit_files", "Commit one or more files to a repository in a single commit (this will commit them atomically in the remote repository)", - { - files: z - .array(z.string()) - .describe( - 'Array of file paths relative to repository root (e.g. ["src/main.js", "README.md"]). All files must exist locally.', - ), - message: z.string().describe("Commit message"), - }, + commitFilesInputSchema, async ({ files, message }) => { const owner = REPO_OWNER; const repo = REPO_NAME; @@ -414,14 +410,7 @@ server.tool( server.tool( "delete_files", "Delete one or more files from a repository in a single commit", - { - paths: z - .array(z.string()) - .describe( - 'Array of file paths to delete relative to repository root (e.g. ["src/old-file.js", "docs/deprecated.md"])', - ), - message: z.string().describe("Commit message"), - }, + deleteFilesInputSchema, async ({ paths, message }) => { const owner = REPO_OWNER; const repo = REPO_NAME; diff --git a/test/create-prompt.test.ts b/test/create-prompt.test.ts index e91f5cfb..aa38585b 100644 --- a/test/create-prompt.test.ts +++ b/test/create-prompt.test.ts @@ -819,6 +819,12 @@ describe("generatePrompt", () => { // Should have commit signing tool instructions expect(prompt).toContain("mcp__github_file_ops__commit_files"); expect(prompt).toContain("mcp__github_file_ops__delete_files"); + expect(prompt).toContain( + 'mcp__github_file_ops__delete_files: {"paths": ["path/to/old.js"]', + ); + expect(prompt).not.toContain( + 'mcp__github_file_ops__delete_files: {"files":', + ); // Comment tool should always be from comment server, not file ops expect(prompt).toContain("mcp__github_comment__update_claude_comment"); diff --git a/test/delete-files-prompt-schema.test.ts b/test/delete-files-prompt-schema.test.ts new file mode 100644 index 00000000..91b8a496 --- /dev/null +++ b/test/delete-files-prompt-schema.test.ts @@ -0,0 +1,169 @@ +#!/usr/bin/env bun + +import { describe, expect, test, beforeAll } from "bun:test"; +import { generatePrompt } from "../src/create-prompt"; +import type { PreparedContext } from "../src/create-prompt"; +import { + commitFilesPayloadSchema, + deleteFilesPayloadSchema, +} from "../src/mcp/github-file-ops-schemas"; + +beforeAll(() => { + process.env.GITHUB_ACTION_PATH = "/test/action/path"; +}); + +const mockGitHubData = { + contextData: { + title: "Test PR", + body: "This is a test PR", + author: { login: "testuser" }, + state: "OPEN", + labels: { nodes: [] }, + createdAt: "2023-01-01T00:00:00Z", + additions: 15, + deletions: 5, + baseRefName: "main", + headRefName: "feature-branch", + headRefOid: "abc123", + isCrossRepository: false, + headRepository: { owner: { login: "testowner" }, name: "testrepo" }, + commits: { totalCount: 0, nodes: [] }, + files: { nodes: [] }, + comments: { nodes: [] }, + reviews: { nodes: [] }, + }, + comments: [], + changedFiles: [], + changedFilesWithSHA: [], + reviewData: null, + imageUrlMap: new Map(), +}; + +const signingContext: PreparedContext = { + repository: "owner/repo", + claudeCommentId: "12345", + triggerPhrase: "@claude", + eventData: { + eventName: "issue_comment", + commentId: "67890", + isPR: true, + prNumber: "123", + commentBody: "@claude delete the old file", + }, +}; + +function extractToolExample( + prompt: string, + tool: string, +): Record { + const match = prompt.match( + new RegExp( + `${tool.replace(/[.*+?^${}()|[\]\\]/g, "\\$&")}:\\s*(\\{[^}]+\\})`, + ), + ); + if (!match) { + throw new Error(`No JSON example for ${tool} in prompt`); + } + return JSON.parse(match[1] as string); +} + +describe("delete_files prompt vs live MCP schema (#1665)", () => { + test("the payload the old prompt taught is rejected by the tool schema", () => { + const taughtByOldPrompt = { + files: ["path/to/old.js"], + message: "chore: remove deprecated file", + }; + const result = deleteFilesPayloadSchema.safeParse(taughtByOldPrompt); + expect(result.success).toBe(false); + if (!result.success) { + const fields = result.error.issues.map((issue) => issue.path.join(".")); + expect(fields).toContain("paths"); + } + }); + + test("the payload the new prompt teaches is accepted by the tool schema", () => { + const taughtByNewPrompt = { + paths: ["path/to/old.js"], + message: "chore: remove deprecated file", + }; + const result = deleteFilesPayloadSchema.safeParse(taughtByNewPrompt); + expect(result.success).toBe(true); + }); + + test("generated tag-mode prompt example parses against the live schema", async () => { + const prompt = await generatePrompt( + signingContext, + mockGitHubData, + true, + "tag", + ); + const example = extractToolExample( + prompt, + "mcp__github_file_ops__delete_files", + ); + expect(example).toHaveProperty("paths"); + expect(example).not.toHaveProperty("files"); + const result = deleteFilesPayloadSchema.safeParse(example); + expect(result.success).toBe(true); + }); + + test("rejects paths when the value is a string instead of an array", () => { + const result = deleteFilesPayloadSchema.safeParse({ + paths: "path/to/old.js", + message: "chore: remove deprecated file", + }); + expect(result.success).toBe(false); + }); + + test("rejects a payload that has paths but omits message", () => { + const result = deleteFilesPayloadSchema.safeParse({ + paths: ["path/to/old.js"], + }); + expect(result.success).toBe(false); + if (!result.success) { + expect( + result.error.issues.map((issue) => issue.path.join(".")), + ).toContain("message"); + } + }); + + test("accepts a payload that still includes the old files key beside paths", () => { + const result = deleteFilesPayloadSchema.safeParse({ + files: ["path/to/old.js"], + paths: ["path/to/old.js"], + message: "chore: remove deprecated file", + }); + expect(result.success).toBe(true); + }); + + test("does not change commit_files — that sibling tool still requires files", async () => { + const prompt = await generatePrompt( + signingContext, + mockGitHubData, + true, + "tag", + ); + const example = extractToolExample( + prompt, + "mcp__github_file_ops__commit_files", + ); + expect(example).toHaveProperty("files"); + expect(example).not.toHaveProperty("paths"); + expect(commitFilesPayloadSchema.safeParse(example).success).toBe(true); + expect(deleteFilesPayloadSchema.safeParse(example).success).toBe(false); + }); + + test("generated delete_files example keys are exactly paths and message", async () => { + const prompt = await generatePrompt( + signingContext, + mockGitHubData, + true, + "tag", + ); + const example = extractToolExample( + prompt, + "mcp__github_file_ops__delete_files", + ); + expect(Object.keys(example).sort()).toEqual(["message", "paths"]); + }); +});