From 537979e3c5f37bbe6b1f79e273b8da2b614bf703 Mon Sep 17 00:00:00 2001 From: Connor Moss Date: Sat, 5 Sep 2026 19:55:11 -0400 Subject: [PATCH 1/3] fix(git): refuse to commit when nothing is staged repo.index.commit() writes a tree from the index unconditionally, so git_commit returned "Changes committed successfully with hash ..." even when the index matched HEAD. A caller that edited files and skipped git_add got a hash back, reported the work as committed, and left the working tree dirty with an empty commit on top. The message could not be false, so nothing downstream could tell a real commit from an empty one. Mirrors git commit, which refuses this without --allow-empty: raises when the index matches HEAD, while still allowing the first commit on an unborn branch and an empty merge commit when MERGE_HEAD is present. --- src/git/src/mcp_server_git/server.py | 21 ++++++++ src/git/tests/test_server.py | 75 ++++++++++++++++++++++++++++ 2 files changed, 96 insertions(+) diff --git a/src/git/src/mcp_server_git/server.py b/src/git/src/mcp_server_git/server.py index b94af84661..d94175d310 100644 --- a/src/git/src/mcp_server_git/server.py +++ b/src/git/src/mcp_server_git/server.py @@ -125,7 +125,28 @@ def git_diff(repo: git.Repo, target: str, context_lines: int = DEFAULT_CONTEXT_L repo.rev_parse(target) # Validates target is a real git ref, throws BadName if not return repo.git.diff(f"--unified={context_lines}", target) +def _has_staged_changes(repo: git.Repo) -> bool: + """Whether the index holds anything git would record as a commit. + + Mirrors `git commit`, which refuses to create an empty commit unless + --allow-empty is given, but permits one while a merge is in progress. + """ + if (Path(repo.git_dir) / "MERGE_HEAD").exists(): + return True + if not repo.head.is_valid(): + # Unborn branch: the first commit, so anything in the index counts. + return bool(repo.index.entries) + return bool(repo.index.diff(repo.head.commit)) + def git_commit(repo: git.Repo, message: str) -> str: + # repo.index.commit() writes a tree unconditionally, so without this check + # a caller that forgot to stage gets a hash back for an empty commit and no + # way to tell it apart from a real one. + if not _has_staged_changes(repo): + raise ValueError( + "No changes staged for commit. Use git_add to stage changes first; " + "git_status shows what is currently staged." + ) commit = repo.index.commit(message) return f"Changes committed successfully with hash {commit.hexsha}" diff --git a/src/git/tests/test_server.py b/src/git/tests/test_server.py index 05d5931466..eff5790096 100644 --- a/src/git/tests/test_server.py +++ b/src/git/tests/test_server.py @@ -198,6 +198,81 @@ def test_git_commit(test_repository): latest_commit = test_repository.head.commit assert latest_commit.message.strip() == "test commit message" +def test_git_commit_refuses_when_nothing_is_staged(test_repository): + """repo.index.commit() writes a tree unconditionally, so an unstaged edit + used to come back as a hash for an empty commit while the working tree + stayed dirty and the edit stayed uncommitted.""" + head_before = test_repository.head.commit.hexsha + file_path = Path(test_repository.working_dir) / "test.txt" + file_path.write_text("edited but never staged") + + with pytest.raises(ValueError, match="No changes staged for commit"): + git_commit(test_repository, "should not be created") + + assert test_repository.head.commit.hexsha == head_before + assert test_repository.is_dirty() + +def test_git_commit_refuses_on_a_clean_tree(test_repository): + head_before = test_repository.head.commit.hexsha + + with pytest.raises(ValueError, match="No changes staged for commit"): + git_commit(test_repository, "nothing to record") + + assert test_repository.head.commit.hexsha == head_before + +def test_git_commit_refuses_when_only_untracked_files_exist(test_repository): + head_before = test_repository.head.commit.hexsha + Path(test_repository.working_dir, "untracked.txt").write_text("never added") + + with pytest.raises(ValueError, match="No changes staged for commit"): + git_commit(test_repository, "should not be created") + + assert test_repository.head.commit.hexsha == head_before + +def test_git_commit_records_a_staged_deletion(test_repository): + """A deletion leaves no file behind, so it must not read as an empty index.""" + test_repository.git.rm("test.txt") + + result = git_commit(test_repository, "remove test.txt") + + assert "Changes committed successfully with hash" in result + assert "test.txt" not in test_repository.head.commit.tree + +def test_git_commit_allows_the_first_commit_on_an_unborn_branch(tmp_path: Path): + repo = git.Repo.init(tmp_path / "unborn") + Path(repo.working_dir, "first.txt").write_text("first") + repo.index.add(["first.txt"]) + + result = git_commit(repo, "initial commit") + + assert "Changes committed successfully with hash" in result + assert repo.head.commit.message.strip() == "initial commit" + +def test_git_commit_allows_an_empty_merge_commit(test_repository): + """git itself permits an empty commit while a merge is in progress, so a + conflict resolved back to HEAD's content must still be committable.""" + test_repository.git.checkout("-b", "side") + Path(test_repository.working_dir, "test.txt").write_text("side") + test_repository.git.add("test.txt") + test_repository.index.commit("side change") + + test_repository.git.checkout("-") + Path(test_repository.working_dir, "test.txt").write_text("mainline") + test_repository.git.add("test.txt") + test_repository.index.commit("mainline change") + + with pytest.raises(git.GitCommandError): + test_repository.git.merge("side") + + # Resolve to HEAD's own content, so the index matches HEAD exactly. + Path(test_repository.working_dir, "test.txt").write_text("mainline") + test_repository.git.add("test.txt") + assert not test_repository.index.diff(test_repository.head.commit) + + result = git_commit(test_repository, "merge side") + + assert "Changes committed successfully with hash" in result + def test_git_reset(test_repository): file_path = Path(test_repository.working_dir) / "reset_test.txt" file_path.write_text("content to reset") From 94b1bae7dbb230fb4615b763865525b7262e8211 Mon Sep 17 00:00:00 2001 From: Connor Moss Date: Sat, 5 Sep 2026 19:58:04 -0400 Subject: [PATCH 2/3] test(git): make the empty-merge-commit test deterministic The test provoked a merge conflict and asserted only that some GitCommandError was raised, so a merge that failed for an unrelated reason satisfied it and left no MERGE_HEAD. It passed locally and failed on CI, where the fixture's missing user identity makes `git merge` refuse to run at all. Uses --no-commit --no-ff instead, which sets MERGE_HEAD without depending on how a git version reports conflicts, sets an explicit identity because `git merge` shells out to git, and asserts MERGE_HEAD exists so a failure says what actually went wrong. --- src/git/tests/test_server.py | 34 ++++++++++++++++++++++------------ 1 file changed, 22 insertions(+), 12 deletions(-) diff --git a/src/git/tests/test_server.py b/src/git/tests/test_server.py index eff5790096..f70f4685af 100644 --- a/src/git/tests/test_server.py +++ b/src/git/tests/test_server.py @@ -250,23 +250,33 @@ def test_git_commit_allows_the_first_commit_on_an_unborn_branch(tmp_path: Path): def test_git_commit_allows_an_empty_merge_commit(test_repository): """git itself permits an empty commit while a merge is in progress, so a - conflict resolved back to HEAD's content must still be committable.""" + merge whose result matches HEAD must still be committable. + + Uses --no-commit rather than provoking a conflict: it sets MERGE_HEAD + deterministically, without depending on how a given git version reports + conflicts. The repo needs an explicit identity because `git merge` shells + out to git, which refuses to run without one. + """ + with test_repository.config_writer() as cw: + cw.set_value("user", "name", "Test") + cw.set_value("user", "email", "test@example.com") + + starting_branch = test_repository.active_branch.name test_repository.git.checkout("-b", "side") - Path(test_repository.working_dir, "test.txt").write_text("side") - test_repository.git.add("test.txt") + Path(test_repository.working_dir, "side.txt").write_text("side only") + test_repository.git.add("side.txt") test_repository.index.commit("side change") - test_repository.git.checkout("-") - Path(test_repository.working_dir, "test.txt").write_text("mainline") - test_repository.git.add("test.txt") - test_repository.index.commit("mainline change") + test_repository.git.checkout(starting_branch) + test_repository.git.merge("side", "--no-commit", "--no-ff") - with pytest.raises(git.GitCommandError): - test_repository.git.merge("side") + # A merge is genuinely in progress... + assert (Path(test_repository.git_dir) / "MERGE_HEAD").exists() - # Resolve to HEAD's own content, so the index matches HEAD exactly. - Path(test_repository.working_dir, "test.txt").write_text("mainline") - test_repository.git.add("test.txt") + # ...and the index is rolled back to HEAD's own content, so the pending + # commit records no change at all. git allows exactly this. + test_repository.git.rm("side.txt", "--cached") + Path(test_repository.working_dir, "side.txt").unlink() assert not test_repository.index.diff(test_repository.head.commit) result = git_commit(test_repository, "merge side") From b2c8e10b91164b56919e53a57705df98ee61de4e Mon Sep 17 00:00:00 2001 From: Connor Moss Date: Sun, 6 Sep 2026 00:54:54 -0400 Subject: [PATCH 3/3] test(git): fold the three refusal cases into one test They differed only in starting state, and the repo's python tests use no parametrize, so one test walking clean tree -> untracked -> unstaged edit covers the same ground in a third of the lines. --- src/git/tests/test_server.py | 30 +++++++++++------------------- 1 file changed, 11 insertions(+), 19 deletions(-) diff --git a/src/git/tests/test_server.py b/src/git/tests/test_server.py index f70f4685af..6fbee81b81 100644 --- a/src/git/tests/test_server.py +++ b/src/git/tests/test_server.py @@ -199,35 +199,27 @@ def test_git_commit(test_repository): assert latest_commit.message.strip() == "test commit message" def test_git_commit_refuses_when_nothing_is_staged(test_repository): - """repo.index.commit() writes a tree unconditionally, so an unstaged edit - used to come back as a hash for an empty commit while the working tree - stayed dirty and the edit stayed uncommitted.""" - head_before = test_repository.head.commit.hexsha - file_path = Path(test_repository.working_dir) / "test.txt" - file_path.write_text("edited but never staged") - - with pytest.raises(ValueError, match="No changes staged for commit"): - git_commit(test_repository, "should not be created") - - assert test_repository.head.commit.hexsha == head_before - assert test_repository.is_dirty() - -def test_git_commit_refuses_on_a_clean_tree(test_repository): + """repo.index.commit() writes a tree unconditionally, so any of these used + to come back as a hash for an empty commit with HEAD left where it was.""" head_before = test_repository.head.commit.hexsha + working_file = Path(test_repository.working_dir) / "test.txt" + # A clean tree. with pytest.raises(ValueError, match="No changes staged for commit"): git_commit(test_repository, "nothing to record") - assert test_repository.head.commit.hexsha == head_before - -def test_git_commit_refuses_when_only_untracked_files_exist(test_repository): - head_before = test_repository.head.commit.hexsha + # An untracked file, which git_add was never called for. Path(test_repository.working_dir, "untracked.txt").write_text("never added") + with pytest.raises(ValueError, match="No changes staged for commit"): + git_commit(test_repository, "nothing to record") + # A tracked file edited but not staged: the case an agent actually hits. + working_file.write_text("edited but never staged") with pytest.raises(ValueError, match="No changes staged for commit"): - git_commit(test_repository, "should not be created") + git_commit(test_repository, "nothing to record") assert test_repository.head.commit.hexsha == head_before + assert working_file.read_text() == "edited but never staged" def test_git_commit_records_a_staged_deletion(test_repository): """A deletion leaves no file behind, so it must not read as an empty index."""