Skip to content

fix(git): name /dev/null for files a commit added or deleted - #4822

Open
chrikrah wants to merge 1 commit into
modelcontextprotocol:mainfrom
chrikrah:fix/git-show-dev-null
Open

chrikrah wants to merge 1 commit into
modelcontextprotocol:mainfrom
chrikrah:fix/git-show-dev-null

Conversation

@chrikrah

Copy link
Copy Markdown

Description

GitPython leaves a_path or b_path as None for the side of a diff that does not exist. The f-string at src/git/src/mcp_server_git/server.py:226 renders that as the four characters None. A unified diff names /dev/null there.

So git_show output for any commit that adds or removes a file is not a valid diff. No patch tool accepts it.

Server Details

  • Server: git
  • Changes to: tools (git_show)

Motivation and Context

On d73f99e, for a commit that adds added.txt and deletes gone.txt:

mcp_server_git          real git show

--- None                --- /dev/null
+++ added.txt           +++ b/added.txt

--- gone.txt            --- a/gone.txt
+++ None                +++ /dev/null

An agent handed that output cannot apply it. A model reading it sees a file named None.

How Has This Been Tested?

One regression test covers both directions in a single commit. It sits beside the existing git_show tests.

$ uv run pytest
48 passed

$ uv run ruff check .
All checks passed!

$ pyright --pythonpath .venv/bin/python src/mcp_server_git/server.py
0 errors, 0 warnings, 0 informations

Revert the production change, keep the test, and the test fails.

The two existing tests, test_git_show and test_git_show_initial_commit, assert only that Commit: and a filename appear. Both pass with None present today.

Breaking Changes

None. A client that parsed the old output was parsing an invalid diff.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
    Nothing new. The tool already emits this line.
  • Breaking change (fix or feature that would cause existing functionality to change)
    See above.
  • Documentation update
    No document describes the diff header.

Checklist

  • I have read the MCP Protocol Documentation
  • My changes follows MCP security best practices
  • I have updated the server's README accordingly
    The README does not document the diff format of git_show.
  • I have tested this with an LLM client
    The defect lives in the tool's return value. The test asserts on that value directly.
  • 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
    No environment variable or configuration option changed.

Additional context

Open PR #4532 touches lines 229 to 232 of the same loop, for non-UTF-8 bytes. This change sits at line 226 and does not overlap it.

I searched gh issue list --state all and gh pr list --state all for git_show, mcp-server-git show diff, a_path None and git_show dev/null. Nothing covers this.

Rename detection is out of scope. GitPython fills both paths for a rename, so that case was already right.

GitPython leaves a_path or b_path as None for the side that does not exist, so
the f-string at server.py:226 rendered it as the four characters "None". A
unified diff names /dev/null there, and no patch tool accepts "None", so
git_show's output could not be applied or read reliably for any commit that
added or removed a file.

  --- None            real git show:  --- /dev/null
  +++ added.txt                       +++ b/added.txt

  --- gone.txt                        --- a/gone.txt
  +++ None                            +++ /dev/null

One regression test covering both directions in a single commit, which fails
against the unmodified file.

Tests: uv run pytest
Lint: uv run ruff check . and pyright
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant