fix(git): accept revision ranges in git_diff

[BlueX888]

Description

git_diff rejects revision ranges. git_diff(repo, "main..feature") raises BadName: Ref 'main..range-feature' did not resolve to an object, even though git diff main..feature is a valid command and the tool passes target straight through to git diff.

Root cause: the CWE-88 flag-injection hardening in 9e5d5b8 added repo.rev_parse(target) at src/git/src/mcp_server_git/server.py:125 to confirm the target resolves to a git ref. rev_parse resolves a single object name, so a range such as main..feature, main...feature, or HEAD~1..HEAD is treated as one ref name and rejected before it ever reaches git diff.

The fix splits the target on .. / ... and validates each endpoint separately, so ranges are accepted while flag-like or unresolvable targets are still rejected.

Server Details

• Server: git
• Changes to: tools

Motivation and Context

A revision range is an ordinary single argument to git diff (git diff main..feature), and the git_diff tool documents target as the branch or commit to compare with. Asking for a branch-to-branch comparison as a range is a natural thing for a client to do, and it worked before 9e5d5b8; the added ref validation silently broke it. Validating each endpoint keeps the CWE-88 protection intact: the - prefix check still runs first on the whole target, and every endpoint must resolve to a real git ref.

How Has This Been Tested?

Added test_git_diff_allows_revision_ranges in src/git/tests/test_server.py, which builds a repository with a divergent branch and asserts working diffs for:

• main..range-feature, range-feature..main (both directions)
• main...range-feature (three-dot)
• HEAD~1..HEAD and <sha>..HEAD (commit ranges)

and that invalid inputs still raise BadName: nonexistent..HEAD and main..--output=/tmp/evil.

The test fails on the unpatched tree (gitdb.exc.BadName: Ref 'main..range-feature' did not resolve to an object) and passes with the fix.

Commands run from src/git:

Command	Result
uv run pytest -q	48 passed
uv run pytest tests/test_server.py -q -k test_git_diff_allows_revision_ranges	1 passed
uv run ruff check .	All checks passed!
uv run --frozen pyright	0 errors, 0 warnings, 0 informations

Breaking Changes

None. Targets that resolved before still resolve; the only change is that revision ranges are no longer rejected. git_diff still raises BadName for flag-like targets and for targets whose endpoints are not real refs.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation update

Checklist

• I have read the MCP Protocol Documentation
• My changes follow MCP security best practices
• I have updated the server's README accordingly
• I have tested this with an LLM client
• My code follows the repository's style guidelines
• New and existing tests pass locally
• I have added appropriate error handling
• I have documented all environment variables and configuration options

Additional context

The - prefix guard and the per-endpoint rev_parse check are the existing CWE-88 defense from 9e5d5b8, unchanged in spirit: a target may not start with -, and every component of the target must resolve to a real git ref. Endpoints are not forwarded to the git CLI individually — the original target string is still passed to git diff as a single argument, so no new flag-parsing surface is introduced. No new environment variables or configuration options are added.

[Copilot]

🟡 Changes recommended

Empty endpoints bypass validation, and the README needs clarification.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Updates git_diff to accept two- and three-dot revision ranges while validating endpoints.

Changes:

• Adds range endpoint validation.
• Adds valid and invalid range tests.
• Documents revision-range support.

File summaries

File	Summary
src/git/src/mcp_server_git/server.py	Adds range parsing and validation. Moderate (2 votes): reject empty endpoints such as ..HEAD, HEAD.., and ....
src/git/tests/test_server.py	Adds regression coverage for valid and invalid ranges.
src/git/README.md	Documents range support. Nit (2 votes): clarify behavior for revision ranges versus single revisions.

Review details

• Files reviewed: 3/3 changed files
• Comments generated: 2
• Review effort level: Lite (auto)

Note

Copilot is running an experiment and ran this review at Lite.

---

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Three moderate validation issues remain unresolved in server.py.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

src/git/src/mcp_server_git/server.py:128

• re.split runs before validating the whole target, so this regresses valid single-revision selectors whose search expression contains .. (for example, Git accepts :/fix..bug as a commit-message revision). The previous repo.rev_parse(target) accepted that target, but this now validates :/fix and bug separately and raises BadName; try the whole target first and only fall back to endpoint validation when that fails.

    # 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)

src/git/src/mcp_server_git/server.py:136

• The loop only rejects empty endpoints before calling rev_parse, so an option-like endpoint is still handed to Git's option parser. For example, rev-parse --all can resolve to a commit when the repository has one ref, allowing git_diff(repo, "HEAD..--all") past validation and then producing a raw ambiguous-argument error from git diff instead of BadName. Apply the --prefix guard to each extracted endpoint as well.

    for revision in revisions:
        if not revision:
            raise BadName(f"Invalid target: '{target}' - empty range endpoint")
        repo.rev_parse(revision)

• Files reviewed: 3/3 changed files
• Comments generated: 1
• Review effort level: Lite (auto)

Note

Copilot is running an experiment and ran this review at Lite.

[BlueX888]

Addressed the three points in ba757944. Two of them do not reproduce as described; the third is real and was a regression, so that is what the commit is mostly about. I checked each against git and GitPython 3.1.62 rather than reasoning from the regex alone.

:/fix..bug — real, and a regression. re.split(r"\.\.\.?", target) ran before the target was resolved, so this selector was split into the endpoints :/fix and bug and validated as a range. git rev-parse --verify ':/fix..bug' resolves to a commit, so a target that worked before this PR started raising BadName. The target is now resolved unchanged first, and the range split is only the fallback when that fails. rev_parse reports an unresolvable target as BadName, but rejects a spec its own parser cannot tokenize (such as HEAD~1..HEAD) with ValueError, so the fallback catches both.

HEAD.... — does not reach git, but reported badly. re.split does produce ['HEAD', '.'] here, but repo.rev_parse('.') raises BadName, so the input was already rejected. The rejection was incidental and named a fragment the caller never typed (Ref '.' did not resolve to an object), so a run of four or more dots is now rejected explicitly.

HEAD..--all — does not reproduce. repo.rev_parse('--all') raises BadName (git rev-parse --verify --all -> fatal: Needed a single revision), so the option-like endpoint was already rejected before reaching git.diff. I applied the --prefix guard to each endpoint anyway: the guard on the whole target exists for the case where a ref with that name is created directly on disk, and an endpoint is not obviously safer than the target.

Test state: 51 passed. The :/fix..bug test fails on 9d9aad1f with BadName: Ref 'bug' did not resolve to an object and passes here.

[Copilot]

🔵 Needs a closer look

Range parsing must handle separators inside nested revision selectors.

Review details

Suppressed comments (1)

src/git/src/mcp_server_git/server.py:145

• The fallback splits on every ../..., including dots inside a nested revision selector. Git accepts ranges such as HEAD^{/fix..bug}..HEAD, where the first endpoint is the valid HEAD^{/fix..bug} selector, but this produces three pieces and raises BadName instead of diffing the range. Split only on a top-level range separator (and add a regression test for a dotted selector endpoint).

        revisions = re.split(r"\.\.\.?", target)
        if len(revisions) > 2:
            raise BadName(
                f"Invalid target: '{target}' - expected a revision or a single range"

• Files reviewed: 3/3 changed files
• Comments generated: 0 new
• Review effort level: Lite (auto)

Note

Copilot is running an experiment and ran this review at Lite.

[BlueX888]

The selector is valid on its own, but the range around it is not one — git rejects it too, so there is nothing to accept here. Checking the claim did turn up a real bug of mine, fixed in 2c1e3c1e.

HEAD^{/fix..bug} does resolve (git rev-parse --verify returns a commit), and the commit before this branch leaked a ValueError on it, which ba757944 fixed. But as a range endpoint it does not work, because git splits on the first .. exactly as the fallback does:

git diff 'HEAD^{/fix..bug}..HEAD'   -> fatal: ambiguous argument 'HEAD^{/fix..bug}..HEAD':
                                       unknown revision or path not in the working tree
git diff 'HEAD^{/fix..bug}...HEAD'  -> fatal: ambiguous argument ...

So BadName here is not a regression against git — it is the same refusal, with a better message.

What the check did find. Probing the target unchanged first assumed rev_parse says no only via BadName or ValueError. It also raises KeyError when a rev:path target's revision resolves but the path is not in the tree:

git_diff(repo, "HEAD:missing/path")  -> KeyError: "Blob or Tree named 'missing' not found"

That leaked before this branch, and trying the whole target first extended it to HEAD:missing..path..HEAD, which previously raised BadName. 2c1e3c1e routes all three exception types through one _resolves probe, so an unusable target is a BadName as documented, and the range fallback reports which endpoint was the problem. 53 passed.

[Copilot]

🟢 Approval recommended

Tests and validation cover the change; the remaining documentation nit is non-blocking.

Review details

Suppressed comments (1)

src/git/README.md:41

• The MCP-facing Tool description in server.py still says only “Shows differences between branches or commits,” while this README now documents revision ranges. Clients receive the runtime tool description rather than this README, so they will not discover the newly supported target form; update that description alongside this documentation change.

     - `target` (string): Target branch, commit, or revision range (e.g. `main..feature`) to compare with

• Files reviewed: 3/3 changed files
• Comments generated: 0 new
• Review effort level: Lite (auto)

Note

Copilot is running an experiment and ran this review at Lite.