diff --git a/src/git/README.md b/src/git/README.md index 01ae034782..817bfe93e7 100644 --- a/src/git/README.md +++ b/src/git/README.md @@ -111,6 +111,25 @@ Roots is deprecated in 2026-07-28, and this server never asks a client for Roots - **A tool that fails** returns a result with `isError: true` whose text is `Error executing tool : `, so the model can read it and correct the call. This covers git errors (an unknown revision, a failed command), a `repo_path` that is not a repository or is outside `--repository`, the flag-injection guards, and arguments that do not match the tool's input schema. - **An unknown tool name** is a protocol error, JSON-RPC `-32602` (`Unknown tool: `), as the specification requires in both eras. Before 2.0.0 it was an `isError` result. +### Prompts + +Beside its tools, the server offers two prompts, which a client typically shows the user as slash commands. Each reads the repository when it is requested and returns one user message that embeds the changes, ready to send to the model. They open the repository the way the tools do, so `--repository` confines them too, and the same flag-injection guards apply. They behave the same in both protocol eras. + +1. `git_commit_message` + - Asks the model for a commit message for the staged changes + - Arguments: + - `repo_path` (string, required): Path to the Git repository + - Returns: A message embedding the staged diff and asking for a commit message, or one saying nothing is staged + +2. `git_summarize_changes` + - Asks the model to summarize the working tree's changes, or the diff against a branch or commit + - Arguments: + - `repo_path` (string, required): Path to the Git repository + - `target` (string, optional): Branch or commit to diff against. Without it, the working tree's status and its staged and unstaged diffs are summarized + - Returns: A message embedding those changes and asking for a summary, or one saying there are none + +A prompt request that fails (a `repo_path` that is not a repository or is outside `--repository`, an unknown `target`, a `target` starting with `-`) is a JSON-RPC `-32602` error carrying the message a tool would report, since `prompts/get` has no `isError` result. + ## Installation ### Using uv (recommended) diff --git a/src/git/src/mcp_server_git/server.py b/src/git/src/mcp_server_git/server.py index 8ced4d97cd..a17c66b9cf 100644 --- a/src/git/src/mcp_server_git/server.py +++ b/src/git/src/mcp_server_git/server.py @@ -1,8 +1,9 @@ # The git MCP server: tools that read and change a Git repository through -# GitPython. Repository confinement (`--repository`, `validate_repo_path` on the -# path given, `validate_opened_repo` on the directories git then uses) and the -# flag-injection guards live beside the handlers because every tool call must -# pass them before any git command runs. +# GitPython, and prompts that embed a repository's changes in a message for the +# model (#5195). Repository confinement (`--repository`, `validate_repo_path` on +# the path given, `validate_opened_repo` on the directories git then uses) and +# the flag-injection guards live beside the handlers because every tool call and +# prompt must pass them before any git command runs. # # Built on the MCP Python SDK's high-level `MCPServer` (#5174), the API these # reference servers are meant to teach: each tool is a typed function, and the @@ -596,6 +597,93 @@ def writes(*, destructive: bool, idempotent: bool) -> ToolAnnotations: ) +class GitPrompts(str, Enum): + COMMIT_MESSAGE = "git_commit_message" + SUMMARIZE_CHANGES = "git_summarize_changes" + + +def _fenced(text: str, info: str = "diff") -> str: + """`text` in a Markdown code fence longer than any backtick run inside it. + + A diff can itself hold a ``` line (a Markdown file's code block), which + would close a fixed three-backtick fence early. + """ + # One pass for the longest run: growing the fence and searching again for + # each extra backtick is quadratic in a long run, under the git lock. + longest = run = 0 + for char in text: + run = run + 1 if char == "`" else 0 + longest = max(longest, run) + fence = "`" * max(3, longest + 1) + body = text.rstrip("\n") + return f"{fence}{info}\n{body}\n{fence}" + + +def commit_message_prompt(repo: git.Repo) -> str: + """The git_commit_message prompt's text: the staged diff and the request.""" + diff = git_diff_staged(repo) + if not diff.strip(): + return ( + "There are no staged changes in this repository, so there is nothing " + "to write a commit message for. Tell the user to stage changes first " + "(git_add); git_status shows what is modified or untracked." + ) + return ( + "Write a commit message for the staged changes below. Start with an " + "imperative subject line of at most 72 characters, then a blank line and " + "a short body explaining what changed and why, only if the subject alone " + "does not say enough. Follow the repository's existing convention if " + "its history shows one (git_log).\n\n" + f"Staged changes:\n{_fenced(diff)}" + ) + + +def summarize_changes_prompt(repo: git.Repo, target: str | None = None) -> str: + """The git_summarize_changes prompt's text: the changes and the request.""" + if target: + # git_diff applies the flag-injection guard and resolves the target. + diff = git_diff(repo, target) + scope = f"compared with `{target}`" + changes = f"Diff against {target}:\n{_fenced(diff)}" if diff.strip() else "" + else: + staged = git_diff_staged(repo) + unstaged = git_diff_unstaged(repo) + scope = "in the working tree" + parts = [f"Status:\n{_fenced(git_status(repo), 'text')}"] + if staged.strip(): + parts.append(f"Staged changes:\n{_fenced(staged)}") + if unstaged.strip(): + parts.append(f"Unstaged changes:\n{_fenced(unstaged)}") + # The porcelain status is empty only on a clean tree; an untracked file + # alone leaves both diffs empty but is still a change to summarize. + clean = not repo.git.status("--porcelain").strip() + changes = "" if clean else "\n\n".join(parts) + if not changes: + return f"There are no changes {scope} to summarize. Tell the user so." + return ( + f"Summarize the changes {scope} below for a reviewer, in plain " + "language. Group related changes, explain the intent behind them rather " + "than restating each line, and point out anything that looks unfinished " + "or risky.\n\n" + f"{changes}" + ) + + +@contextmanager +def reported_as_invalid_params() -> Iterator[None]: + """Turn an anticipated failure in a prompt into a JSON-RPC -32602 error. + + `prompts/get` has no `isError` result: the spec's error handling for it is + a JSON-RPC error, and a repository path, target or guard the request named + is an invalid argument. Without this, MCPServer hides the message behind + "Error rendering prompt" (code 0 or -32603, depending on the era). + """ + try: + yield + except ANTICIPATED_ERRORS as e: + raise MCPError(code=INVALID_PARAMS, message=str(e)) from e + + def create_server(repository: Path | None) -> MCPServer: """The git server, confined to `repository` when one is given.""" @@ -636,6 +724,43 @@ def open_repo(repo_path: str) -> git.Repo: raise return repo + # Prompts (#5195): each embeds what the repository holds now in one user + # message, through the same `open_repo` (and so the same confinement) and + # the same guarded helpers as the tools, under the same lock. + @server.prompt( + name=GitPrompts.COMMIT_MESSAGE.value, + title="Draft a commit message", + description="Asks the model for a commit message for the staged changes", + ) + def commit_message( + repo_path: Annotated[str, Field(description="The path to the Git repository.")], + ) -> str: + with _GIT_LOCK, reported_as_invalid_params(), open_repo(repo_path) as repo: + return commit_message_prompt(repo) + + @server.prompt( + name=GitPrompts.SUMMARIZE_CHANGES.value, + title="Summarize changes", + description=( + "Asks the model to summarize the working tree's staged and unstaged " + "changes, or the diff against a branch or commit" + ), + ) + def summarize_changes( + repo_path: Annotated[str, Field(description="The path to the Git repository.")], + target: Annotated[ + Optional[str], + Field( + description=( + "Branch or commit to diff against. Defaults to the working " + "tree's staged and unstaged changes" + ) + ), + ] = None, + ) -> str: + with _GIT_LOCK, reported_as_invalid_params(), open_repo(repo_path) as repo: + return summarize_changes_prompt(repo, target) + @server.tool( name=GitTools.STATUS.value, description="Shows the working tree status", diff --git a/src/git/tests/test_prompts.py b/src/git/tests/test_prompts.py new file mode 100644 index 0000000000..75f6de9fa9 --- /dev/null +++ b/src/git/tests/test_prompts.py @@ -0,0 +1,291 @@ +# The git prompts (#5195), driven through a real `mcp` 2.x `Client` in both +# protocol eras, plus the prompt-text builders called directly. +# +# The prompts exist to demonstrate the Prompts feature beside the tools, so +# what matters is what a client receives from prompts/list and prompts/get: +# the names and arguments, one user message embedding the repository's +# changes, and a JSON-RPC -32602 error (prompts/get has no isError result) when +# the repository path, the target or a guard rejects the request. Each era test +# is parametrized over both eras, since nothing about prompts should differ +# between them. + +from pathlib import Path +from typing import Any + +import git +import pytest +from mcp.shared.exceptions import MCPError +from mcp.types import INVALID_PARAMS, GetPromptResult, TextContent + +from mcp_server_git.server import ( + _fenced, + commit_message_prompt, + summarize_changes_prompt, +) + +from test_eras import ERAS, MODERN, connect + +COMMIT_MESSAGE = "git_commit_message" +SUMMARIZE_CHANGES = "git_summarize_changes" + + +def repo_dir(repo: git.Repo) -> Path: + return Path(repo.working_dir) + + +def message_text(result: GetPromptResult) -> str: + """The text of a prompt result holding one user message of text.""" + assert len(result.messages) == 1 + message = result.messages[0] + assert message.role == "user" + assert isinstance(message.content, TextContent) + return message.content.text + + +def prompt_wire(prompts: Any) -> list[dict[str, Any]]: + return [ + p.model_dump(by_alias=True, mode="json", exclude_none=True) for p in prompts + ] + + +EXPECTED_PROMPTS = [ + { + "name": COMMIT_MESSAGE, + "title": "Draft a commit message", + "description": "Asks the model for a commit message for the staged changes", + "arguments": [ + { + "name": "repo_path", + "description": "The path to the Git repository.", + "required": True, + } + ], + }, + { + "name": SUMMARIZE_CHANGES, + "title": "Summarize changes", + "description": ( + "Asks the model to summarize the working tree's staged and unstaged " + "changes, or the diff against a branch or commit" + ), + "arguments": [ + { + "name": "repo_path", + "description": "The path to the Git repository.", + "required": True, + }, + { + "name": "target", + "description": ( + "Branch or commit to diff against. Defaults to the working " + "tree's staged and unstaged changes" + ), + "required": False, + }, + ], + }, +] + + +# --- Over the protocol, in both eras ---------------------------------------- + + +@pytest.mark.parametrize(("mode", "negotiated"), ERAS) +async def test_prompts_are_listed_in_each_era( + repo: git.Repo, mode: str, negotiated: str +) -> None: + async with connect(mode, repo_dir(repo)) as client: + assert client.session.protocol_version == negotiated + prompts = prompt_wire((await client.list_prompts()).prompts) + assert prompts == EXPECTED_PROMPTS + + +@pytest.mark.parametrize(("mode", "negotiated"), ERAS) +async def test_commit_message_embeds_the_staged_diff( + repo: git.Repo, mode: str, negotiated: str +) -> None: + (repo_dir(repo) / "feature.txt").write_text("hello prompt world\n") + repo.index.add(["feature.txt"]) + (repo_dir(repo) / "test.txt").write_text("unstaged edit\n") + async with connect(mode, repo_dir(repo)) as client: + result = await client.get_prompt( + COMMIT_MESSAGE, {"repo_path": str(repo_dir(repo))} + ) + assert result.description == ( + "Asks the model for a commit message for the staged changes" + ) + text = message_text(result) + assert text.startswith("Write a commit message for the staged changes below.") + assert "Staged changes:\n```diff\ndiff --git a/feature.txt b/feature.txt" in text + assert "+hello prompt world" in text + # Only what is staged: the unstaged edit to test.txt is not in it. + assert "unstaged edit" not in text + + +@pytest.mark.parametrize(("mode", "negotiated"), ERAS) +async def test_summarize_changes_embeds_the_working_tree( + repo: git.Repo, mode: str, negotiated: str +) -> None: + (repo_dir(repo) / "feature.txt").write_text("staged line\n") + repo.index.add(["feature.txt"]) + (repo_dir(repo) / "test.txt").write_text("unstaged edit\n") + (repo_dir(repo) / "untracked.txt").write_text("new\n") + async with connect(mode, repo_dir(repo)) as client: + result = await client.get_prompt( + SUMMARIZE_CHANGES, {"repo_path": str(repo_dir(repo))} + ) + text = message_text(result) + assert text.startswith("Summarize the changes in the working tree below") + assert "Status:\n```text\n" in text + assert "untracked.txt" in text + assert "Staged changes:\n```diff\n" in text + assert "+staged line" in text + assert "Unstaged changes:\n```diff\n" in text + assert "+unstaged edit" in text + + +@pytest.mark.parametrize(("mode", "negotiated"), ERAS) +async def test_summarize_changes_against_a_target( + repo: git.Repo, mode: str, negotiated: str +) -> None: + (repo_dir(repo) / "test.txt").write_text("committed edit\n") + repo.index.add(["test.txt"]) + repo.index.commit("second commit") + async with connect(mode, repo_dir(repo)) as client: + result = await client.get_prompt( + SUMMARIZE_CHANGES, + {"repo_path": str(repo_dir(repo)), "target": "HEAD~1"}, + ) + text = message_text(result) + assert text.startswith("Summarize the changes compared with `HEAD~1` below") + assert "Diff against HEAD~1:\n```diff\n" in text + assert "+committed edit" in text + + +@pytest.mark.parametrize(("mode", "negotiated"), ERAS) +async def test_target_flag_injection_is_invalid_params( + repo: git.Repo, mode: str, negotiated: str +) -> None: + # git_diff's guard applies unchanged, and its message reaches the client. + async with connect(mode, repo_dir(repo)) as client: + with pytest.raises(MCPError) as excinfo: + await client.get_prompt( + SUMMARIZE_CHANGES, + {"repo_path": str(repo_dir(repo)), "target": "--output=/tmp/x"}, + ) + assert excinfo.value.error.code == INVALID_PARAMS + assert excinfo.value.error.message == ( + "Invalid target: '--output=/tmp/x' - cannot start with '-'" + ) + + +@pytest.mark.parametrize(("mode", "negotiated"), ERAS) +async def test_unknown_target_is_invalid_params( + repo: git.Repo, mode: str, negotiated: str +) -> None: + async with connect(mode, repo_dir(repo)) as client: + with pytest.raises(MCPError) as excinfo: + await client.get_prompt( + SUMMARIZE_CHANGES, + {"repo_path": str(repo_dir(repo)), "target": "no-such-ref"}, + ) + assert excinfo.value.error.code == INVALID_PARAMS + assert "no-such-ref" in excinfo.value.error.message + + +@pytest.mark.parametrize("prompt", [COMMIT_MESSAGE, SUMMARIZE_CHANGES]) +@pytest.mark.parametrize(("mode", "negotiated"), ERAS) +async def test_repository_confinement_applies_to_prompts( + tmp_path: Path, mode: str, negotiated: str, prompt: str +) -> None: + # Both prompts open the repository through open_repo, so --repository + # confines them exactly as it confines the tools. + allowed = git.Repo.init(tmp_path / "allowed") + other = git.Repo.init(tmp_path / "other") + try: + async with connect(mode, repo_dir(allowed)) as client: + with pytest.raises(MCPError) as excinfo: + await client.get_prompt(prompt, {"repo_path": str(repo_dir(other))}) + finally: + allowed.close() + other.close() + assert excinfo.value.error.code == INVALID_PARAMS + assert "is outside the allowed repository" in excinfo.value.error.message + + +async def test_a_path_that_is_not_a_repository_is_invalid_params( + tmp_path: Path, +) -> None: + async with connect(MODERN, None) as client: + with pytest.raises(MCPError) as excinfo: + await client.get_prompt(COMMIT_MESSAGE, {"repo_path": str(tmp_path)}) + assert excinfo.value.error.code == INVALID_PARAMS + + +async def test_unknown_prompt_and_missing_argument_are_protocol_errors( + repo: git.Repo, +) -> None: + # Both are rejected by MCPServer before a prompt function runs. The code + # is the SDK's to choose (python-sdk#3669 moves both to -32602), so only + # that each is a JSON-RPC error, not a result, is pinned here. + async with connect(MODERN, repo_dir(repo)) as client: + with pytest.raises(MCPError): + await client.get_prompt("git_no_such_prompt", {}) + with pytest.raises(MCPError): + await client.get_prompt(COMMIT_MESSAGE, {}) + + +# --- The prompt text, called directly ---------------------------------------- + + +def test_commit_message_with_nothing_staged(repo: git.Repo) -> None: + (repo_dir(repo) / "test.txt").write_text("unstaged edit\n") + text = commit_message_prompt(repo) + assert text.startswith("There are no staged changes in this repository") + + +def test_summarize_a_clean_working_tree(repo: git.Repo) -> None: + assert summarize_changes_prompt(repo) == ( + "There are no changes in the working tree to summarize. Tell the user so." + ) + + +def test_summarize_only_unstaged_changes(repo: git.Repo) -> None: + (repo_dir(repo) / "test.txt").write_text("unstaged edit\n") + text = summarize_changes_prompt(repo) + assert "Unstaged changes:" in text + assert "Staged changes:" not in text + + +def test_summarize_only_staged_changes(repo: git.Repo) -> None: + (repo_dir(repo) / "test.txt").write_text("staged edit\n") + repo.index.add(["test.txt"]) + text = summarize_changes_prompt(repo) + assert "Staged changes:" in text + assert "Unstaged changes:" not in text + + +def test_summarize_only_untracked_files(repo: git.Repo) -> None: + # Both diffs are empty, but the status lists the new file. + (repo_dir(repo) / "untracked.txt").write_text("new\n") + text = summarize_changes_prompt(repo) + assert text.startswith("Summarize the changes in the working tree below") + assert "untracked.txt" in text + assert "Staged changes:" not in text + assert "Unstaged changes:" not in text + + +def test_summarize_against_a_target_with_no_difference(repo: git.Repo) -> None: + assert summarize_changes_prompt(repo, "HEAD") == ( + "There are no changes compared with `HEAD` to summarize. Tell the user so." + ) + + +def test_fence_outgrows_backticks_in_the_content() -> None: + # A diff of a Markdown file can hold its own ``` fence; the embedding + # fence must be longer so it is not closed early. + assert _fenced("plain\n") == "```diff\nplain\n```" + assert _fenced("+```python\n+x\n+```\n") == ("````diff\n+```python\n+x\n+```\n````") + assert _fenced("a ```` b", "text") == "`````text\na ```` b\n`````" + # A long run is measured in one pass, not by growing the fence. + assert _fenced("`" * 10_000).startswith("`" * 10_001 + "diff\n") diff --git a/src/git/tests/test_protocol.py b/src/git/tests/test_protocol.py index 370ff2b692..e356946916 100644 --- a/src/git/tests/test_protocol.py +++ b/src/git/tests/test_protocol.py @@ -37,8 +37,9 @@ SNAPSHOTS = Path(__file__).parent / "snapshots" -# What the server advertises in either era. MCPServer serves empty prompt and -# resource lists, so both appear; nothing ever changes, so no listChanged. +# What the server advertises in either era. MCPServer serves prompt and +# resource lists (the git prompts, #5195; no resources), so both appear; +# nothing ever changes, so no listChanged. CAPABILITIES = { "prompts": {"listChanged": False}, "resources": {"listChanged": False, "subscribe": False}, @@ -78,8 +79,8 @@ async def test_initialize_reports_package_version_as_server_version(repo: git.Re "version": version("mcp-server-git"), } assert version("mcp") != version("mcp-server-git") - # MCPServer (#5174) always serves prompts/list and resources/list (empty - # here), so it advertises both capabilities beside tools. + # MCPServer (#5174) always serves prompts/list and resources/list (the + # resource list is empty here), so it advertises both beside tools. assert init["capabilities"] == CAPABILITIES