From 2a0656b9674a2ee1acfd84d1325e2d934070da15 Mon Sep 17 00:00:00 2001 From: BlueX888 <140241684+BlueX888@users.noreply.github.com> Date: Wed, 16 Sep 2026 14:37:37 +0800 Subject: [PATCH 1/4] fix(git): accept revision ranges in git_diff --- src/git/README.md | 2 +- src/git/src/mcp_server_git/server.py | 7 ++++- src/git/tests/test_server.py | 40 ++++++++++++++++++++++++++++ 3 files changed, 47 insertions(+), 2 deletions(-) diff --git a/src/git/README.md b/src/git/README.md index 70b00974e6..dec0d21333 100644 --- a/src/git/README.md +++ b/src/git/README.md @@ -38,7 +38,7 @@ Please note that mcp-server-git is currently in early development. The functiona - Shows differences between branches or commits - Inputs: - `repo_path` (string): Path to Git repository - - `target` (string): Target branch or commit to compare with + - `target` (string): Target branch, commit, or revision range (e.g. `main..feature`) to compare with - `context_lines` (number, optional): Number of context lines to show (default: 3) - Returns: Diff output comparing current state with target diff --git a/src/git/src/mcp_server_git/server.py b/src/git/src/mcp_server_git/server.py index b94af84661..6367e5c7c5 100644 --- a/src/git/src/mcp_server_git/server.py +++ b/src/git/src/mcp_server_git/server.py @@ -1,4 +1,5 @@ import logging +import re from pathlib import Path from typing import Any, Optional, Sequence from mcp.server import Server @@ -122,7 +123,11 @@ def git_diff(repo: git.Repo, target: str, context_lines: int = DEFAULT_CONTEXT_L # even if a malicious ref with that name exists (e.g. via filesystem manipulation) if target.startswith("-"): raise BadName(f"Invalid target: '{target}' - cannot start with '-'") - repo.rev_parse(target) # Validates target is a real git ref, throws BadName if not + # target may be a revision range (e.g. 'main..feature' or 'main...feature'), + # so validate each endpoint is a real git ref rather than the range as a whole + for revision in re.split(r"\.\.\.?", target): + if revision: + repo.rev_parse(revision) return repo.git.diff(f"--unified={context_lines}", target) def git_commit(repo: git.Repo, message: str) -> str: diff --git a/src/git/tests/test_server.py b/src/git/tests/test_server.py index 05d5931466..cc557ef4f0 100644 --- a/src/git/tests/test_server.py +++ b/src/git/tests/test_server.py @@ -393,6 +393,46 @@ def test_git_diff_allows_valid_refs(test_repository): assert result is not None +def test_git_diff_allows_revision_ranges(test_repository): + """git_diff should accept revision ranges such as 'A..B' and 'A...B'.""" + default_branch = test_repository.active_branch.name + + test_repository.git.checkout("-b", "range-feature") + file_path = Path(test_repository.working_dir) / "test.txt" + file_path.write_text("first range change") + test_repository.index.add(["test.txt"]) + first_commit = test_repository.index.commit("first range commit") + file_path.write_text("second range change") + test_repository.index.add(["test.txt"]) + test_repository.index.commit("second range commit") + + # Branch ranges, in both directions and with both range syntaxes + test_repository.git.checkout(default_branch) + for target in ( + f"{default_branch}..range-feature", + f"range-feature..{default_branch}", + f"{default_branch}...range-feature", + ): + result = git_diff(test_repository, target) + assert "test.txt" in result + assert "range change" in result + + # Commit ranges reachable from HEAD + test_repository.git.checkout("range-feature") + result = git_diff(test_repository, "HEAD~1..HEAD") + assert "second range change" in result + + result = git_diff(test_repository, f"{first_commit.hexsha}..HEAD") + assert "second range change" in result + + # Endpoints that are not real refs are still rejected + with pytest.raises(BadName): + git_diff(test_repository, "nonexistent..HEAD") + + with pytest.raises(BadName): + git_diff(test_repository, f"{default_branch}..--output=/tmp/evil") + + def test_git_checkout_allows_valid_branches(test_repository): """git_checkout should work normally with valid branch names.""" # Get the default branch name From 9d9aad1fa798c7bd5a0440f0a9b8965a263797de Mon Sep 17 00:00:00 2001 From: BlueX888 <140241684+BlueX888@users.noreply.github.com> Date: Thu, 17 Sep 2026 10:06:36 +0800 Subject: [PATCH 2/4] fix(git): reject revision ranges that have no two endpoints Splitting the target on `..`/`...` and skipping empty pieces accepted `..HEAD`, `HEAD..` and `...`, which have no second endpoint, and `....`, which is not a range at all. `git diff` takes the first three as a range against HEAD and returns an empty diff, while `....` reached git and failed with a raw GitCommandError instead of BadName. `rev_parse` rejected all four before this change, so requiring both endpoints keeps the previous contract for everything except the two-endpoint ranges this PR adds. Ranges with more than one separator are rejected too. Also corrects the README: for a revision range, git compares the two endpoints rather than the current state with the target. --- src/git/README.md | 2 +- src/git/src/mcp_server_git/server.py | 12 +++++++++--- src/git/tests/test_server.py | 18 ++++++++++++++++++ 3 files changed, 28 insertions(+), 4 deletions(-) diff --git a/src/git/README.md b/src/git/README.md index dec0d21333..79b32dcc43 100644 --- a/src/git/README.md +++ b/src/git/README.md @@ -40,7 +40,7 @@ Please note that mcp-server-git is currently in early development. The functiona - `repo_path` (string): Path to Git repository - `target` (string): Target branch, commit, or revision range (e.g. `main..feature`) to compare with - `context_lines` (number, optional): Number of context lines to show (default: 3) - - Returns: Diff output comparing current state with target + - Returns: Diff output comparing current state with target, or comparing the two endpoints with each other when target is a revision range 5. `git_commit` - Records changes to the repository diff --git a/src/git/src/mcp_server_git/server.py b/src/git/src/mcp_server_git/server.py index 6367e5c7c5..77bb834345 100644 --- a/src/git/src/mcp_server_git/server.py +++ b/src/git/src/mcp_server_git/server.py @@ -125,9 +125,15 @@ def git_diff(repo: git.Repo, target: str, context_lines: int = DEFAULT_CONTEXT_L raise BadName(f"Invalid target: '{target}' - cannot start with '-'") # target may be a revision range (e.g. 'main..feature' or 'main...feature'), # so validate each endpoint is a real git ref rather than the range as a whole - for revision in re.split(r"\.\.\.?", target): - if revision: - repo.rev_parse(revision) + revisions = re.split(r"\.\.\.?", target) + if len(revisions) > 2: + raise BadName( + f"Invalid target: '{target}' - expected a revision or a single range" + ) + for revision in revisions: + if not revision: + raise BadName(f"Invalid target: '{target}' - empty range endpoint") + repo.rev_parse(revision) return repo.git.diff(f"--unified={context_lines}", target) def git_commit(repo: git.Repo, message: str) -> str: diff --git a/src/git/tests/test_server.py b/src/git/tests/test_server.py index cc557ef4f0..57a05917c9 100644 --- a/src/git/tests/test_server.py +++ b/src/git/tests/test_server.py @@ -433,6 +433,24 @@ def test_git_diff_allows_revision_ranges(test_repository): git_diff(test_repository, f"{default_branch}..--output=/tmp/evil") +def test_git_diff_rejects_ranges_without_two_endpoints(test_repository): + """A range has to name two revisions. + + `..target`, `target..` and `...` are ranges with an empty endpoint, and + `....` is not a range at all. None of them resolve to two real refs, so + they are rejected here rather than reaching `git diff` and failing there + with a raw git error. + """ + for target in ("..HEAD", "HEAD..", "...", "...."): + with pytest.raises(BadName): + git_diff(test_repository, target) + + # A range may not carry more than one separator either + default_branch = test_repository.active_branch.name + with pytest.raises(BadName): + git_diff(test_repository, f"{default_branch}..{default_branch}...HEAD") + + def test_git_checkout_allows_valid_branches(test_repository): """git_checkout should work normally with valid branch names.""" # Get the default branch name From ba7579448133e4ec09f4ac7e978efdbd422d78ef Mon Sep 17 00:00:00 2001 From: BlueX888 <140241684+BlueX888@users.noreply.github.com> Date: Thu, 17 Sep 2026 17:11:35 +0800 Subject: [PATCH 3/4] fix(git): resolve a target as one revision before splitting it Trying the target unchanged first keeps single revisions that contain '..', such as the commit-message selector ':/fix..bug', from being split into the endpoints ':/fix' and 'bug' and rejected. rev_parse reports such a target as BadName, and rejects a spec its own parser cannot tokenize with ValueError, so the range fallback catches both. The range fallback also rejects a run of four or more dots outright: 'HEAD....' is not a range with two endpoints, and relying on the trailing '.' failing to resolve reports the wrong part of the input as the problem. --- src/git/src/mcp_server_git/server.py | 40 +++++++++++++++++++-------- src/git/tests/test_server.py | 41 ++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 11 deletions(-) diff --git a/src/git/src/mcp_server_git/server.py b/src/git/src/mcp_server_git/server.py index 77bb834345..663e19b5f2 100644 --- a/src/git/src/mcp_server_git/server.py +++ b/src/git/src/mcp_server_git/server.py @@ -123,17 +123,35 @@ def git_diff(repo: git.Repo, target: str, context_lines: int = DEFAULT_CONTEXT_L # even if a malicious ref with that name exists (e.g. via filesystem manipulation) if target.startswith("-"): raise BadName(f"Invalid target: '{target}' - cannot start with '-'") - # target may be a revision range (e.g. 'main..feature' or 'main...feature'), - # so validate each endpoint is a real git ref rather than the range as a whole - revisions = re.split(r"\.\.\.?", target) - if len(revisions) > 2: - raise BadName( - f"Invalid target: '{target}' - expected a revision or a single range" - ) - for revision in revisions: - if not revision: - raise BadName(f"Invalid target: '{target}' - empty range endpoint") - repo.rev_parse(revision) + # target may be a revision range, so the endpoints have to be validated + # individually: rev_parse rejects 'main..feature' as a whole. Try the target + # unchanged first, because a single revision is allowed to contain '..' + # itself, as the commit-message selector ':/fix..bug' does. + # rev_parse reports an unresolvable target as BadName, but rejects a spec its + # own parser cannot tokenize (e.g. 'HEAD~1..HEAD') with ValueError. + try: + repo.rev_parse(target) + except (BadName, ValueError): + # Only '..' and '...' separate endpoints, so a run of four or more dots + # is a malformed range rather than a range with an odd endpoint. + if re.search(r"\.\.\.\.", target): + raise BadName( + f"Invalid target: '{target}' - expected a revision or a single " + f"'..' or '...' range" + ) + revisions = re.split(r"\.\.\.?", target) + if len(revisions) > 2: + raise BadName( + f"Invalid target: '{target}' - expected a revision or a single range" + ) + for revision in revisions: + if not revision: + raise BadName(f"Invalid target: '{target}' - empty range endpoint") + # Same flag-injection guard as the whole target above: an endpoint + # reaching git's option parser is no safer than the target doing so. + if revision.startswith("-"): + raise BadName(f"Invalid target: '{target}' - cannot start with '-'") + repo.rev_parse(revision) return repo.git.diff(f"--unified={context_lines}", target) def git_commit(repo: git.Repo, message: str) -> str: diff --git a/src/git/tests/test_server.py b/src/git/tests/test_server.py index 57a05917c9..753e9275eb 100644 --- a/src/git/tests/test_server.py +++ b/src/git/tests/test_server.py @@ -451,6 +451,47 @@ def test_git_diff_rejects_ranges_without_two_endpoints(test_repository): git_diff(test_repository, f"{default_branch}..{default_branch}...HEAD") +def test_git_diff_allows_single_revisions_containing_dots(test_repository): + """A single revision may contain '..' without being a revision range. + + `:/text` matches commit messages, and the text is a regular expression, so + a selector such as ':/fix..bug' resolves to one revision. Splitting every + target on '..' before resolving it would validate ':/fix' and 'bug' + separately and reject a target git accepts, so the whole target is tried + first and the range split is only the fallback. + """ + file_path = Path(test_repository.working_dir) / "test.txt" + file_path.write_text("dotted selector change") + test_repository.index.add(["test.txt"]) + test_repository.index.commit("fix..bug in the subject") + + # Leave a change behind so the diff against that revision is not empty + file_path.write_text("dotted selector change plus worktree edit") + + assert test_repository.rev_parse(":/fix..bug") is not None + + result = git_diff(test_repository, ":/fix..bug") + assert "worktree edit" in result + + # A dotted selector is still rejected when it matches no commit + with pytest.raises(BadName): + git_diff(test_repository, ":/no..such..subject") + + +def test_git_diff_rejects_ranges_with_more_than_two_endpoints(test_repository): + """Only '..' and '...' separate endpoints, so extra dots are malformed. + + `HEAD....` splits into `HEAD` and `.`, which is not a range with two real + endpoints; it has to be rejected explicitly rather than by whichever + endpoint happens to fail to resolve. + """ + default_branch = test_repository.active_branch.name + + for target in ("HEAD....", f"{default_branch}....HEAD", "HEAD.....", "HEAD..--all"): + with pytest.raises(BadName): + git_diff(test_repository, target) + + def test_git_checkout_allows_valid_branches(test_repository): """git_checkout should work normally with valid branch names.""" # Get the default branch name From 2c1e3c1efc8e6b75d41a7428207635c5b3dc84c2 Mon Sep 17 00:00:00 2001 From: BlueX888 <140241684+BlueX888@users.noreply.github.com> Date: Thu, 17 Sep 2026 22:53:03 +0800 Subject: [PATCH 4/4] fix(git): treat every rev_parse refusal as an unresolvable target Probing the target unchanged relies on rev_parse saying no in every way it can. It raises BadName for a revision that does not resolve and ValueError for a spec its own parser cannot tokenize, but a 'rev:path' target whose revision resolves and whose path is not in the tree raises KeyError from the tree lookup, which escaped git_diff and reached the caller as a raw KeyError. 'HEAD:missing/path' leaked that way before this branch, and probing the whole target first extended it to 'HEAD:missing..path..HEAD'. All three now read as one refusal, so an unusable target is a BadName as documented. --- src/git/src/mcp_server_git/server.py | 26 +++++++++++++++----- src/git/tests/test_server.py | 36 ++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 6 deletions(-) diff --git a/src/git/src/mcp_server_git/server.py b/src/git/src/mcp_server_git/server.py index 663e19b5f2..6ad12c6321 100644 --- a/src/git/src/mcp_server_git/server.py +++ b/src/git/src/mcp_server_git/server.py @@ -118,6 +118,21 @@ def git_diff_unstaged(repo: git.Repo, context_lines: int = DEFAULT_CONTEXT_LINES def git_diff_staged(repo: git.Repo, context_lines: int = DEFAULT_CONTEXT_LINES) -> str: return repo.git.diff(f"--unified={context_lines}", "--cached") +def _resolves(repo: git.Repo, revision: str) -> bool: + """Whether ``revision`` names a git object, treating every refusal as one. + + ``rev_parse`` raises ``BadName`` for a revision that does not resolve, + ``ValueError`` for a spec its own parser cannot tokenize (e.g. + ``HEAD~1..HEAD``), and ``KeyError`` when a ``rev:path`` target names a path + the tree does not contain. Only the first is a deliberate "not a revision" + signal, but all three mean the target is unusable as one. + """ + try: + repo.rev_parse(revision) + except (BadName, ValueError, KeyError): + return False + return True + def git_diff(repo: git.Repo, target: str, context_lines: int = DEFAULT_CONTEXT_LINES) -> str: # Defense in depth: reject targets starting with '-' to prevent flag injection, # even if a malicious ref with that name exists (e.g. via filesystem manipulation) @@ -127,11 +142,7 @@ def git_diff(repo: git.Repo, target: str, context_lines: int = DEFAULT_CONTEXT_L # individually: rev_parse rejects 'main..feature' as a whole. Try the target # unchanged first, because a single revision is allowed to contain '..' # itself, as the commit-message selector ':/fix..bug' does. - # rev_parse reports an unresolvable target as BadName, but rejects a spec its - # own parser cannot tokenize (e.g. 'HEAD~1..HEAD') with ValueError. - try: - repo.rev_parse(target) - except (BadName, ValueError): + if not _resolves(repo, target): # Only '..' and '...' separate endpoints, so a run of four or more dots # is a malformed range rather than a range with an odd endpoint. if re.search(r"\.\.\.\.", target): @@ -151,7 +162,10 @@ def git_diff(repo: git.Repo, target: str, context_lines: int = DEFAULT_CONTEXT_L # reaching git's option parser is no safer than the target doing so. if revision.startswith("-"): raise BadName(f"Invalid target: '{target}' - cannot start with '-'") - repo.rev_parse(revision) + if not _resolves(repo, revision): + raise BadName( + f"Invalid target: '{target}' - '{revision}' is not a revision" + ) return repo.git.diff(f"--unified={context_lines}", target) def git_commit(repo: git.Repo, message: str) -> str: diff --git a/src/git/tests/test_server.py b/src/git/tests/test_server.py index 753e9275eb..422f572618 100644 --- a/src/git/tests/test_server.py +++ b/src/git/tests/test_server.py @@ -492,6 +492,42 @@ def test_git_diff_rejects_ranges_with_more_than_two_endpoints(test_repository): git_diff(test_repository, target) +def test_git_diff_reports_an_unresolvable_rev_path_as_bad_name(test_repository): + """A 'rev:path' target naming a missing path is a BadName, not a KeyError. + + `rev_parse` resolves the revision and then raises `KeyError` from the tree + lookup, so probing a target for being a single revision has to treat that + the same way as a revision that does not resolve at all. + """ + for target in ("HEAD:missing/path", "HEAD:missing..path"): + with pytest.raises(BadName): + git_diff(test_repository, target) + + default_branch = test_repository.active_branch.name + with pytest.raises(BadName): + git_diff(test_repository, f"{default_branch}:missing..path..HEAD") + + +def test_git_diff_accepts_a_peeled_message_selector(test_repository): + """'HEAD^{/text}' is one revision; wrapping it in a range is not a range. + + The selector is accepted on its own, and a range naming it is rejected + because git cannot parse one either -- it splits on the first '..' too. + """ + file_path = Path(test_repository.working_dir) / "test.txt" + file_path.write_text("peeled selector change") + test_repository.index.add(["test.txt"]) + test_repository.index.commit("fix..bug in the subject") + + file_path.write_text("peeled selector change plus worktree edit") + + assert test_repository.rev_parse("HEAD^{/fix..bug}") is not None + assert "worktree edit" in git_diff(test_repository, "HEAD^{/fix..bug}") + + with pytest.raises(BadName): + git_diff(test_repository, "HEAD^{/fix..bug}..HEAD") + + def test_git_checkout_allows_valid_branches(test_repository): """git_checkout should work normally with valid branch names.""" # Get the default branch name