From ebbd8d2028e9f0976206ad9c75557fcab33ca855 Mon Sep 17 00:00:00 2001 From: mementorc Date: Thu, 18 Jun 2026 20:17:16 -0500 Subject: [PATCH 1/4] feat(git): add git_reflog tool for HEAD/ref movement forensics (#181) Exposes git reflog with structured output (new SHA, selector, action, message) so pre-commit and non-commit HEAD movements (aborted merges, resets, checkouts) are visible through the MCP interface. git_log only shows committed history. Params: repo_path, ref (default HEAD), max_count, all. Closes #181 Co-Authored-By: Claude Opus 4.8 --- README.md | 4 +- src/mcp_server_git/git/_commit_ops.py | 79 ++++++++ src/mcp_server_git/git/models.py | 7 + src/mcp_server_git/git/operations.py | 2 + src/mcp_server_git/lean/registry_git.py | 9 + tests/unit/git/test_git_reflog.py | 249 ++++++++++++++++++++++++ 6 files changed, 348 insertions(+), 2 deletions(-) create mode 100644 tests/unit/git/test_git_reflog.py diff --git a/README.md b/README.md index 8627da3b..a9186ddc 100644 --- a/README.md +++ b/README.md @@ -137,7 +137,7 @@ The server provides Azure DevOps integration for monitoring and analyzing Azure ### Lean MCP Interface (Context-Optimized) -The server provides an alternative **lean interface** that reduces context consumption by ~97% (from ~30k tokens to ~900 tokens). Instead of exposing all 55 tools upfront, it uses a 3-meta-tool pattern: +The server provides an alternative **lean interface** that reduces context consumption by ~97% (from ~30k tokens to ~900 tokens). Instead of exposing all 56 tools upfront, it uses a 3-meta-tool pattern: | Meta-Tool | Purpose | |-----------|---------| @@ -145,7 +145,7 @@ The server provides an alternative **lean interface** that reduces context consu | `get_tool_spec(tool_name)` | Get full schema for a specific tool on-demand | | `execute_tool(tool_name, params)` | Execute any tool dynamically | -**Tool Coverage**: All 54 tools remain accessible (26 git, 25 GitHub, 3 Azure DevOps). +**Tool Coverage**: All 55 tools remain accessible (27 git, 25 GitHub, 3 Azure DevOps). **Usage Example**: ```python diff --git a/src/mcp_server_git/git/_commit_ops.py b/src/mcp_server_git/git/_commit_ops.py index 356b02e5..5447e74d 100644 --- a/src/mcp_server_git/git/_commit_ops.py +++ b/src/mcp_server_git/git/_commit_ops.py @@ -14,6 +14,7 @@ "git_log", "git_show", "git_blame", + "git_reflog", ] @@ -281,3 +282,81 @@ def git_blame( return f"❌ Blame failed: {str(e)}" except Exception as e: return f"❌ Blame error: {str(e)}" + + +def git_reflog( + repo: Repo, + ref: str = "HEAD", + max_count: int | None = None, + all: bool = False, +) -> list[dict]: + """Read git reflog (HEAD/ref movement history including non-commit operations). + + Args: + repo: Git repository object + ref: Ref to show reflog for (default: HEAD) + max_count: Maximum number of entries to return + all: If True, show reflog for all refs (--all) + + Returns: + List of dicts with keys: new_sha, label, action, message, and + old_sha when the parent SHA is available. + """ + try: + # %H = new (post-move) full SHA, %gD = reflog selector (HEAD@{0}), + # %gP = reflog parent (previous position SHA, empty for initial entry), + # %gs = reflog subject (e.g. "checkout: moving from main to feature") + sep = "\x00" + fmt = f"%H{sep}%gD{sep}%gP{sep}%gs" + + args = ["show", f"--format={fmt}", "--no-abbrev", "--no-patch"] + + if max_count is not None and max_count > 0: + args.extend(["-n", str(max_count)]) + + if all: + args.append("--all") + else: + args.append(ref) + + raw_output = repo.git.reflog(*args) + + if not raw_output.strip(): + return [] + + entries = [] + for line in raw_output.splitlines(): + line = line.strip() + if not line: + continue + parts = line.split(sep) + if len(parts) < 4: + continue + new_sha, label, old_sha_raw, subject = ( + parts[0], + parts[1], + parts[2], + parts[3], + ) + + # Derive action from subject: first token before ':' + # e.g. "checkout: moving from A to B" -> "checkout" + action = subject.split(":")[0].strip() if subject else "unknown" + + entry: dict = { + "new_sha": new_sha, + "label": label, + "action": action, + "message": subject, + } + if old_sha_raw and old_sha_raw != "0" * 40: + entry["old_sha"] = old_sha_raw + + entries.append(entry) + + return entries + + except GitCommandError as e: + return [{"error": f"Reflog failed: {str(e)}"}] + except Exception as e: + return [{"error": f"Reflog error: {str(e)}"}] diff --git a/src/mcp_server_git/git/models.py b/src/mcp_server_git/git/models.py index eb9aba4e..e769b55e 100644 --- a/src/mcp_server_git/git/models.py +++ b/src/mcp_server_git/git/models.py @@ -81,6 +81,13 @@ class GitLog(BaseModel): merges: bool | None = None # None=all, True=only merges, False=no merges +class GitReflog(BaseModel): + repo_path: str + ref: str = Field(default="HEAD", description="Ref to show reflog for (default: HEAD)") + max_count: int | None = Field(default=None, description="Maximum number of reflog entries to return") + all: bool = Field(default=False, description="Show reflog for all refs (git reflog --all)") + + class GitCreateBranch(BaseModel): repo_path: str branch_name: str diff --git a/src/mcp_server_git/git/operations.py b/src/mcp_server_git/git/operations.py index c44b70b3..688587e1 100644 --- a/src/mcp_server_git/git/operations.py +++ b/src/mcp_server_git/git/operations.py @@ -17,6 +17,7 @@ git_blame, git_commit, git_log, + git_reflog, git_show, git_status, ) @@ -105,6 +106,7 @@ "git_tag_create", "git_tag_delete", "git_blame", + "git_reflog", "git_branch_list", "git_merge_base", "git_submodule_status", diff --git a/src/mcp_server_git/lean/registry_git.py b/src/mcp_server_git/lean/registry_git.py index 3d9457ae..ea3c96b8 100644 --- a/src/mcp_server_git/lean/registry_git.py +++ b/src/mcp_server_git/lean/registry_git.py @@ -25,6 +25,7 @@ GitDiffUnstaged, GitInit, GitLog, + GitReflog, GitMerge, GitMergeBase, GitMergeTree, @@ -139,6 +140,14 @@ def wrapper(repo_path: str, **kwargs): domain="git", complexity="core", ), + ToolDefinition( + name="git_reflog", + implementation=wrap_repo_op(git_ops.git_reflog), + description="Read git reflog (HEAD/ref movement history incl. non-commit ops)", + schema=GitReflog.model_json_schema(), + domain="git", + complexity="core", + ), ToolDefinition( name="git_create_branch", implementation=wrap_repo_op(git_ops.git_create_branch), diff --git a/tests/unit/git/test_git_reflog.py b/tests/unit/git/test_git_reflog.py new file mode 100644 index 00000000..e760fb2b --- /dev/null +++ b/tests/unit/git/test_git_reflog.py @@ -0,0 +1,249 @@ +""" +Unit tests for git_reflog operation. + +Verifies structured output (new_sha, label, action, message, old_sha), +max_count limiting, all=True flag, error handling, and empty-reflog edge case. +""" + +from unittest.mock import Mock + +import pytest + +from mcp_server_git.git.operations import git_reflog +from mcp_server_git.utils.git_import import GitCommandError + +# NUL separator used by the format string +SEP = "\x00" + + +def _make_raw(*entries: tuple) -> str: + """Build fake reflog output for (new_sha, label, old_sha, subject) tuples.""" + lines = [] + for new_sha, label, old_sha, subject in entries: + lines.append(f"{new_sha}{SEP}{label}{SEP}{old_sha}{SEP}{subject}") + return "\n".join(lines) + + +class TestGitReflogBasic: + """Test basic git_reflog functionality.""" + + def test_git_reflog_returns_list_of_dicts(self): + """Should return a list of dicts with required keys.""" + mock_repo = Mock() + new_sha = "a" * 40 + mock_repo.git.reflog.return_value = _make_raw( + (new_sha, "HEAD@{0}", "0" * 40, "commit: initial commit") + ) + + result = git_reflog(mock_repo) + + assert isinstance(result, list) + assert len(result) == 1 + entry = result[0] + assert entry["new_sha"] == new_sha + assert entry["label"] == "HEAD@{0}" + assert entry["action"] == "commit" + assert entry["message"] == "commit: initial commit" + + def test_git_reflog_old_sha_omitted_when_zero(self): + """Should omit old_sha when it is the all-zeros sentinel.""" + mock_repo = Mock() + mock_repo.git.reflog.return_value = _make_raw( + ("a" * 40, "HEAD@{0}", "0" * 40, "commit: initial") + ) + + result = git_reflog(mock_repo) + + assert "old_sha" not in result[0] + + def test_git_reflog_old_sha_present_when_non_zero(self): + """Should include old_sha when previous position is known.""" + mock_repo = Mock() + new_sha = "b" * 40 + old_sha = "a" * 40 + mock_repo.git.reflog.return_value = _make_raw( + (new_sha, "HEAD@{0}", old_sha, "checkout: moving from main to feature") + ) + + result = git_reflog(mock_repo) + + assert result[0]["old_sha"] == old_sha + + def test_git_reflog_empty_returns_empty_list(self): + """Should return empty list when reflog is empty.""" + mock_repo = Mock() + mock_repo.git.reflog.return_value = "" + + result = git_reflog(mock_repo) + + assert result == [] + + def test_git_reflog_default_ref_is_head(self): + """Should pass HEAD as positional arg when all=False and ref='HEAD'.""" + mock_repo = Mock() + mock_repo.git.reflog.return_value = "" + + git_reflog(mock_repo) + + args = mock_repo.git.reflog.call_args[0] + assert "HEAD" in args + + def test_git_reflog_custom_ref_passed(self): + """Should pass custom ref when specified.""" + mock_repo = Mock() + mock_repo.git.reflog.return_value = "" + + git_reflog(mock_repo, ref="refs/heads/main") + + args = mock_repo.git.reflog.call_args[0] + assert "refs/heads/main" in args + + +class TestGitReflogActionParsing: + """Test action derivation from reflog subjects.""" + + @pytest.mark.parametrize( + "subject, expected_action", + [ + ("checkout: moving from main to feature", "checkout"), + ("commit: add new feature", "commit"), + ("reset: moving to HEAD~1", "reset"), + ("merge origin/main: Fast-forward", "merge"), + ("rebase (start): checkout main", "rebase (start)"), + ("pull: Fast-forward", "pull"), + ("commit (amend): fixup", "commit (amend)"), + ], + ) + def test_git_reflog_action_parsing_returns_correct_action( + self, subject: str, expected_action: str + ): + """Should parse action from reflog subject correctly.""" + mock_repo = Mock() + mock_repo.git.reflog.return_value = _make_raw( + ("a" * 40, "HEAD@{0}", "b" * 40, subject) + ) + + result = git_reflog(mock_repo) + + assert result[0]["action"] == expected_action + + def test_git_reflog_action_unknown_when_subject_empty(self): + """Should set action='unknown' when subject is empty.""" + mock_repo = Mock() + mock_repo.git.reflog.return_value = _make_raw( + ("a" * 40, "HEAD@{0}", "b" * 40, "") + ) + + result = git_reflog(mock_repo) + + assert result[0]["action"] == "unknown" + + +class TestGitReflogMaxCount: + """Test max_count limiting.""" + + def test_git_reflog_max_count_passes_n_flag(self): + """Should pass -n when max_count is set.""" + mock_repo = Mock() + mock_repo.git.reflog.return_value = "" + + git_reflog(mock_repo, max_count=3) + + args = mock_repo.git.reflog.call_args[0] + assert "-n" in args + assert "3" in args + + def test_git_reflog_no_n_flag_when_max_count_none(self): + """Should not pass -n flag when max_count is None.""" + mock_repo = Mock() + mock_repo.git.reflog.return_value = "" + + git_reflog(mock_repo, max_count=None) + + args = mock_repo.git.reflog.call_args[0] + assert "-n" not in args + + def test_git_reflog_no_n_flag_when_max_count_zero(self): + """Should not pass -n flag when max_count is 0.""" + mock_repo = Mock() + mock_repo.git.reflog.return_value = "" + + git_reflog(mock_repo, max_count=0) + + args = mock_repo.git.reflog.call_args[0] + assert "-n" not in args + + +class TestGitReflogAllFlag: + """Test --all flag behaviour.""" + + def test_git_reflog_all_true_passes_all_flag(self): + """Should pass --all and omit ref when all=True.""" + mock_repo = Mock() + mock_repo.git.reflog.return_value = "" + + git_reflog(mock_repo, all=True) + + args = mock_repo.git.reflog.call_args[0] + assert "--all" in args + assert "HEAD" not in args + + def test_git_reflog_all_false_does_not_pass_all_flag(self): + """Should not pass --all when all=False.""" + mock_repo = Mock() + mock_repo.git.reflog.return_value = "" + + git_reflog(mock_repo, all=False) + + args = mock_repo.git.reflog.call_args[0] + assert "--all" not in args + + +class TestGitReflogMultipleEntries: + """Test parsing of multiple reflog entries.""" + + def test_git_reflog_multiple_entries_parsed_correctly(self): + """Should parse multiple entries into correctly ordered list.""" + mock_repo = Mock() + sha0, sha1, sha2 = "c" * 40, "b" * 40, "a" * 40 + mock_repo.git.reflog.return_value = _make_raw( + (sha0, "HEAD@{0}", sha1, "checkout: moving from main to feature"), + (sha1, "HEAD@{1}", sha2, "commit: add tests"), + (sha2, "HEAD@{2}", "0" * 40, "commit: initial"), + ) + + result = git_reflog(mock_repo) + + assert len(result) == 3 + assert result[0]["new_sha"] == sha0 + assert result[0]["action"] == "checkout" + assert result[1]["new_sha"] == sha1 + assert result[1]["action"] == "commit" + assert result[2]["new_sha"] == sha2 + assert "old_sha" not in result[2] + + +class TestGitReflogErrorHandling: + """Test error handling in git_reflog.""" + + def test_git_reflog_returns_error_dict_on_git_command_error(self): + """Should return list with error dict on GitCommandError.""" + mock_repo = Mock() + mock_repo.git.reflog.side_effect = GitCommandError("git reflog", 128, "error") + + result = git_reflog(mock_repo) + + assert len(result) == 1 + assert "error" in result[0] + assert "Reflog failed" in result[0]["error"] + + def test_git_reflog_returns_error_dict_on_general_exception(self): + """Should return list with error dict on unexpected exception.""" + mock_repo = Mock() + mock_repo.git.reflog.side_effect = RuntimeError("unexpected") + + result = git_reflog(mock_repo) + + assert len(result) == 1 + assert "error" in result[0] + assert "Reflog error" in result[0]["error"] From 90ef7e3bab4538564eb2cd4ef9fbb0a51a6be2ee Mon Sep 17 00:00:00 2001 From: mementorc Date: Thu, 18 Jun 2026 20:21:51 -0500 Subject: [PATCH 2/4] fix(git): drop operand from git_reflog action, keep parenthetical qualifier 'merge origin/main: ...' now parses action='merge' (was 'merge origin/main') while 'commit (amend)' / 'rebase (start)' qualifiers are preserved. Co-Authored-By: Claude Opus 4.8 --- src/mcp_server_git/git/_commit_ops.py | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/mcp_server_git/git/_commit_ops.py b/src/mcp_server_git/git/_commit_ops.py index 5447e74d..89c44e77 100644 --- a/src/mcp_server_git/git/_commit_ops.py +++ b/src/mcp_server_git/git/_commit_ops.py @@ -339,9 +339,19 @@ def git_reflog( parts[3], ) - # Derive action from subject: first token before ':' + # Derive action from subject: first token before ':', + # plus any parenthetical qualifier: "commit (amend)" / "rebase (start)" # e.g. "checkout: moving from A to B" -> "checkout" - action = subject.split(":")[0].strip() if subject else "unknown" + # "merge origin/main: Fast-forward" -> "merge" + # "commit (amend): msg" -> "commit (amend)" + pre_colon = subject.split(":", 1)[0].strip() if subject else "" + _parts = pre_colon.split() + if len(_parts) >= 2 and _parts[1].startswith("("): + action = f"{_parts[0]} {_parts[1]}" + elif _parts: + action = _parts[0] + else: + action = "unknown" entry: dict = { "new_sha": new_sha, From a04be55fd38dd1ddfa91c0c6784b99785bb77b31 Mon Sep 17 00:00:00 2001 From: mementorc Date: Thu, 18 Jun 2026 21:48:50 -0500 Subject: [PATCH 3/4] fix(git): address PR #185 review on git_reflog - Correct old_sha source (verified %gP behavior empirically; %gP yields a reflog selector like HEAD@{1}, not a commit SHA. old_sha is now derived from entry[i+1].new_sha, since reflog is newest-first). - Robust action parsing via regex for multi-word parenthetical qualifiers ('commit (initial import)' now captured in full). - Match git_log error convention (return str on failure, list[dict]|str return annotation). - Rename `all` param -> show_all (alias 'all' preserves MCP API); function param keeps `all` with # noqa: A002 since dispatch bypasses Pydantic; GitReflog model uses show_all + validation_alias='all'. Co-Authored-By: Claude Opus 4.8 --- src/mcp_server_git/git/_commit_ops.py | 68 ++++++++++++++------------- src/mcp_server_git/git/models.py | 8 +++- tests/unit/git/test_git_reflog.py | 68 +++++++++++++++------------ 3 files changed, 81 insertions(+), 63 deletions(-) diff --git a/src/mcp_server_git/git/_commit_ops.py b/src/mcp_server_git/git/_commit_ops.py index 89c44e77..d07f7912 100644 --- a/src/mcp_server_git/git/_commit_ops.py +++ b/src/mcp_server_git/git/_commit_ops.py @@ -2,6 +2,7 @@ import logging import os +import re import subprocess from ..utils.git_import import GitCommandError, Repo @@ -288,8 +289,8 @@ def git_reflog( repo: Repo, ref: str = "HEAD", max_count: int | None = None, - all: bool = False, -) -> list[dict]: + all: bool = False, # noqa: A002 +) -> list[dict] | str: """Read git reflog (HEAD/ref movement history including non-commit operations). Args: @@ -300,21 +301,29 @@ def git_reflog( Returns: List of dicts with keys: new_sha, label, action, message, and - old_sha when the parent SHA is available. + old_sha (the SHA before this operation, derived from the next entry's + new_sha since reflog is newest-first; absent for the oldest entry). + Returns a string starting with '❌' on error. + + Note on old_sha: + %gP (reflog parent selector) yields a reflog selector like 'HEAD@{1}', + not a commit SHA. old_sha is instead taken from entry[i+1].new_sha, + which is always the commit HEAD pointed to before entry[i]'s operation. """ try: # %H = new (post-move) full SHA, %gD = reflog selector (HEAD@{0}), - # %gP = reflog parent (previous position SHA, empty for initial entry), # %gs = reflog subject (e.g. "checkout: moving from main to feature") + # Note: %gP gives a reflog selector (HEAD@{N}), not a commit SHA, + # so old_sha is derived from the next entry's new_sha instead. sep = "\x00" - fmt = f"%H{sep}%gD{sep}%gP{sep}%gs" + fmt = f"%H{sep}%gD{sep}%gs" args = ["show", f"--format={fmt}", "--no-abbrev", "--no-patch"] if max_count is not None and max_count > 0: args.extend(["-n", str(max_count)]) - if all: + if all: # noqa: A002 args.append("--all") else: args.append(ref) @@ -330,43 +339,36 @@ def git_reflog( if not line: continue parts = line.split(sep) - if len(parts) < 4: + if len(parts) < 3: continue - new_sha, label, old_sha_raw, subject = ( - parts[0], - parts[1], - parts[2], - parts[3], - ) - - # Derive action from subject: first token before ':', - # plus any parenthetical qualifier: "commit (amend)" / "rebase (start)" - # e.g. "checkout: moving from A to B" -> "checkout" - # "merge origin/main: Fast-forward" -> "merge" - # "commit (amend): msg" -> "commit (amend)" + new_sha, label, subject = parts[0], parts[1], parts[2] + + # Derive action from subject: verb before ':', keeping any + # parenthetical qualifier that immediately follows the verb. + # Examples: + # "checkout: moving from A to B" -> "checkout" + # "merge origin/main: Fast-forward" -> "merge" + # "commit (amend): fixup msg" -> "commit (amend)" + # "rebase (start): checkout main" -> "rebase (start)" + # "commit (initial import): msg" -> "commit (initial import)" pre_colon = subject.split(":", 1)[0].strip() if subject else "" - _parts = pre_colon.split() - if len(_parts) >= 2 and _parts[1].startswith("("): - action = f"{_parts[0]} {_parts[1]}" - elif _parts: - action = _parts[0] - else: - action = "unknown" + m = re.match(r'^(\w[\w-]*(?:\s+\([^)]*\))?)', pre_colon) + action = m.group(1) if m else (pre_colon.split()[0] if pre_colon.split() else "unknown") - entry: dict = { + entries.append({ "new_sha": new_sha, "label": label, "action": action, "message": subject, - } - if old_sha_raw and old_sha_raw != "0" * 40: - entry["old_sha"] = old_sha_raw + }) - entries.append(entry) + # old_sha for entry[i] = entry[i+1].new_sha (reflog is newest-first). + for i in range(len(entries) - 1): + entries[i]["old_sha"] = entries[i + 1]["new_sha"] return entries except GitCommandError as e: - return [{"error": f"Reflog failed: {str(e)}"}] + return f"❌ Reflog failed: {str(e)}" except Exception as e: - return [{"error": f"Reflog error: {str(e)}"}] + return f"❌ Reflog error: {str(e)}" diff --git a/src/mcp_server_git/git/models.py b/src/mcp_server_git/git/models.py index e769b55e..69c4328b 100644 --- a/src/mcp_server_git/git/models.py +++ b/src/mcp_server_git/git/models.py @@ -82,10 +82,16 @@ class GitLog(BaseModel): class GitReflog(BaseModel): + model_config = {"populate_by_name": True} + repo_path: str ref: str = Field(default="HEAD", description="Ref to show reflog for (default: HEAD)") max_count: int | None = Field(default=None, description="Maximum number of reflog entries to return") - all: bool = Field(default=False, description="Show reflog for all refs (git reflog --all)") + show_all: bool = Field( + default=False, + validation_alias="all", + description="Show reflog for all refs (git reflog --all)", + ) class GitCreateBranch(BaseModel): diff --git a/tests/unit/git/test_git_reflog.py b/tests/unit/git/test_git_reflog.py index e760fb2b..da2319d7 100644 --- a/tests/unit/git/test_git_reflog.py +++ b/tests/unit/git/test_git_reflog.py @@ -17,10 +17,10 @@ def _make_raw(*entries: tuple) -> str: - """Build fake reflog output for (new_sha, label, old_sha, subject) tuples.""" + """Build fake reflog output for (new_sha, label, subject) tuples.""" lines = [] - for new_sha, label, old_sha, subject in entries: - lines.append(f"{new_sha}{SEP}{label}{SEP}{old_sha}{SEP}{subject}") + for new_sha, label, subject in entries: + lines.append(f"{new_sha}{SEP}{label}{SEP}{subject}") return "\n".join(lines) @@ -32,7 +32,7 @@ def test_git_reflog_returns_list_of_dicts(self): mock_repo = Mock() new_sha = "a" * 40 mock_repo.git.reflog.return_value = _make_raw( - (new_sha, "HEAD@{0}", "0" * 40, "commit: initial commit") + (new_sha, "HEAD@{0}", "commit: initial commit") ) result = git_reflog(mock_repo) @@ -45,29 +45,31 @@ def test_git_reflog_returns_list_of_dicts(self): assert entry["action"] == "commit" assert entry["message"] == "commit: initial commit" - def test_git_reflog_old_sha_omitted_when_zero(self): - """Should omit old_sha when it is the all-zeros sentinel.""" + def test_git_reflog_old_sha_absent_for_single_entry(self): + """Should omit old_sha when there is only one (oldest) entry.""" mock_repo = Mock() mock_repo.git.reflog.return_value = _make_raw( - ("a" * 40, "HEAD@{0}", "0" * 40, "commit: initial") + ("a" * 40, "HEAD@{0}", "commit: initial") ) result = git_reflog(mock_repo) assert "old_sha" not in result[0] - def test_git_reflog_old_sha_present_when_non_zero(self): - """Should include old_sha when previous position is known.""" + def test_git_reflog_old_sha_derived_from_next_entry(self): + """old_sha for entry[i] must equal entry[i+1].new_sha (newest-first order).""" mock_repo = Mock() - new_sha = "b" * 40 - old_sha = "a" * 40 + sha0 = "b" * 40 # newest entry's new_sha + sha1 = "a" * 40 # older entry's new_sha == sha0's old_sha mock_repo.git.reflog.return_value = _make_raw( - (new_sha, "HEAD@{0}", old_sha, "checkout: moving from main to feature") + (sha0, "HEAD@{0}", "checkout: moving from main to feature"), + (sha1, "HEAD@{1}", "commit: initial"), ) result = git_reflog(mock_repo) - assert result[0]["old_sha"] == old_sha + assert result[0]["old_sha"] == sha1 + assert "old_sha" not in result[1] def test_git_reflog_empty_returns_empty_list(self): """Should return empty list when reflog is empty.""" @@ -112,6 +114,8 @@ class TestGitReflogActionParsing: ("rebase (start): checkout main", "rebase (start)"), ("pull: Fast-forward", "pull"), ("commit (amend): fixup", "commit (amend)"), + # Multi-word parenthetical qualifier must be captured in full + ("commit (initial import): first checkin", "commit (initial import)"), ], ) def test_git_reflog_action_parsing_returns_correct_action( @@ -120,7 +124,7 @@ def test_git_reflog_action_parsing_returns_correct_action( """Should parse action from reflog subject correctly.""" mock_repo = Mock() mock_repo.git.reflog.return_value = _make_raw( - ("a" * 40, "HEAD@{0}", "b" * 40, subject) + ("a" * 40, "HEAD@{0}", subject) ) result = git_reflog(mock_repo) @@ -131,7 +135,7 @@ def test_git_reflog_action_unknown_when_subject_empty(self): """Should set action='unknown' when subject is empty.""" mock_repo = Mock() mock_repo.git.reflog.return_value = _make_raw( - ("a" * 40, "HEAD@{0}", "b" * 40, "") + ("a" * 40, "HEAD@{0}", "") ) result = git_reflog(mock_repo) @@ -203,13 +207,17 @@ class TestGitReflogMultipleEntries: """Test parsing of multiple reflog entries.""" def test_git_reflog_multiple_entries_parsed_correctly(self): - """Should parse multiple entries into correctly ordered list.""" + """Should parse multiple entries into correctly ordered list. + + old_sha for each entry is derived from the next entry's new_sha; + the oldest entry has no old_sha. + """ mock_repo = Mock() sha0, sha1, sha2 = "c" * 40, "b" * 40, "a" * 40 mock_repo.git.reflog.return_value = _make_raw( - (sha0, "HEAD@{0}", sha1, "checkout: moving from main to feature"), - (sha1, "HEAD@{1}", sha2, "commit: add tests"), - (sha2, "HEAD@{2}", "0" * 40, "commit: initial"), + (sha0, "HEAD@{0}", "checkout: moving from main to feature"), + (sha1, "HEAD@{1}", "commit: add tests"), + (sha2, "HEAD@{2}", "commit: initial"), ) result = git_reflog(mock_repo) @@ -217,8 +225,10 @@ def test_git_reflog_multiple_entries_parsed_correctly(self): assert len(result) == 3 assert result[0]["new_sha"] == sha0 assert result[0]["action"] == "checkout" + assert result[0]["old_sha"] == sha1 assert result[1]["new_sha"] == sha1 assert result[1]["action"] == "commit" + assert result[1]["old_sha"] == sha2 assert result[2]["new_sha"] == sha2 assert "old_sha" not in result[2] @@ -226,24 +236,24 @@ def test_git_reflog_multiple_entries_parsed_correctly(self): class TestGitReflogErrorHandling: """Test error handling in git_reflog.""" - def test_git_reflog_returns_error_dict_on_git_command_error(self): - """Should return list with error dict on GitCommandError.""" + def test_git_reflog_returns_error_str_on_git_command_error(self): + """Should return a '❌ Reflog failed:' string on GitCommandError.""" mock_repo = Mock() mock_repo.git.reflog.side_effect = GitCommandError("git reflog", 128, "error") result = git_reflog(mock_repo) - assert len(result) == 1 - assert "error" in result[0] - assert "Reflog failed" in result[0]["error"] + assert isinstance(result, str) + assert result.startswith("❌") + assert "Reflog failed" in result - def test_git_reflog_returns_error_dict_on_general_exception(self): - """Should return list with error dict on unexpected exception.""" + def test_git_reflog_returns_error_str_on_general_exception(self): + """Should return a '❌ Reflog error:' string on unexpected exception.""" mock_repo = Mock() mock_repo.git.reflog.side_effect = RuntimeError("unexpected") result = git_reflog(mock_repo) - assert len(result) == 1 - assert "error" in result[0] - assert "Reflog error" in result[0]["error"] + assert isinstance(result, str) + assert result.startswith("❌") + assert "Reflog error" in result From 702489975d67d1eb95727d0a2355a0d14d21dedf Mon Sep 17 00:00:00 2001 From: mementorc Date: Fri, 19 Jun 2026 09:58:49 -0500 Subject: [PATCH 4/4] =?UTF-8?q?fix(git):=20git=5Freflog=20follow-up=20revi?= =?UTF-8?q?ew=20=E2=80=94=20consistent=20'all',=20size=20guard,=20max=5Fco?= =?UTF-8?q?unt=20contract?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Use `all` consistently (model field + param both 'all' w/ noqa); drop the show_all alias indirection so schema key == function kwarg. - Add large-output warning matching git_log/git_show. - Document max_count<=0 as no-limit; add ge=0 to the model field. Co-Authored-By: Claude Opus 4.8 --- src/mcp_server_git/git/_commit_ops.py | 12 ++++ src/mcp_server_git/git/models.py | 7 +-- tests/unit/git/test_git_reflog.py | 89 +++++++++++++++++++++++++++ 3 files changed, 103 insertions(+), 5 deletions(-) diff --git a/src/mcp_server_git/git/_commit_ops.py b/src/mcp_server_git/git/_commit_ops.py index d07f7912..0966a971 100644 --- a/src/mcp_server_git/git/_commit_ops.py +++ b/src/mcp_server_git/git/_commit_ops.py @@ -320,6 +320,7 @@ def git_reflog( args = ["show", f"--format={fmt}", "--no-abbrev", "--no-patch"] + # max_count <= 0 (or None) means no limit; only positive values constrain. if max_count is not None and max_count > 0: args.extend(["-n", str(max_count)]) @@ -366,6 +367,17 @@ def git_reflog( for i in range(len(entries) - 1): entries[i]["old_sha"] = entries[i + 1]["new_sha"] + # Warn on large output, mirroring git_show's 50KB threshold. + serialized_size = len(str(entries)) + if serialized_size > 50000: # 50KB threshold + entries.append({ + "warning": ( + f"⚠️ Large reflog detected ({len(entries) - 1} entries, " + f"~{serialized_size // 1000}KB). " + "Consider using max_count to limit output." + ) + }) + return entries except GitCommandError as e: diff --git a/src/mcp_server_git/git/models.py b/src/mcp_server_git/git/models.py index 69c4328b..e5d12278 100644 --- a/src/mcp_server_git/git/models.py +++ b/src/mcp_server_git/git/models.py @@ -82,14 +82,11 @@ class GitLog(BaseModel): class GitReflog(BaseModel): - model_config = {"populate_by_name": True} - repo_path: str ref: str = Field(default="HEAD", description="Ref to show reflog for (default: HEAD)") - max_count: int | None = Field(default=None, description="Maximum number of reflog entries to return") - show_all: bool = Field( + max_count: int | None = Field(default=None, ge=0, description="Maximum number of reflog entries to return") + all: bool = Field( # noqa: A003 default=False, - validation_alias="all", description="Show reflog for all refs (git reflog --all)", ) diff --git a/tests/unit/git/test_git_reflog.py b/tests/unit/git/test_git_reflog.py index da2319d7..848d6013 100644 --- a/tests/unit/git/test_git_reflog.py +++ b/tests/unit/git/test_git_reflog.py @@ -257,3 +257,92 @@ def test_git_reflog_returns_error_str_on_general_exception(self): assert isinstance(result, str) assert result.startswith("❌") assert "Reflog error" in result + + +class TestGitReflogModel: + """Test GitReflog Pydantic model schema and field naming (FIX 1).""" + + def test_gitreflog_schema_property_is_all_not_show_all(self): + """Schema property must be 'all', not 'show_all' — no alias indirection.""" + from mcp_server_git.git.models import GitReflog + + props = GitReflog.model_json_schema()["properties"] + assert "all" in props, "Schema must expose 'all', not 'show_all'" + assert "show_all" not in props, "Schema must not expose deprecated 'show_all'" + + def test_gitreflog_accepts_all_true_directly(self): + """Model should accept {'all': True} without alias.""" + from mcp_server_git.git.models import GitReflog + + instance = GitReflog(repo_path="/tmp/repo", **{"all": True}) + assert instance.all is True # noqa: A003 + + def test_gitreflog_max_count_rejects_negative(self): + """ge=0 constraint should reject negative max_count at validation.""" + from pydantic import ValidationError + + from mcp_server_git.git.models import GitReflog + + with pytest.raises(ValidationError): + GitReflog(repo_path="/tmp/repo", max_count=-1) + + +class TestGitReflogLargeOutputWarning: + """Test large-output warning appended for big reflogs (FIX 3).""" + + def test_git_reflog_appends_warning_when_output_exceeds_50kb(self): + """Should append a warning dict when serialized entries exceed 50KB.""" + mock_repo = Mock() + # Generate enough entries to exceed 50KB when serialized. + # Each line ~100 chars; 600 entries pushes str(entries) well above 50KB. + sha = "a" * 40 + lines = [ + f"{sha}\x00HEAD@{{{i}}}\x00checkout: moving from branch-{i} to branch-{i + 1}" + for i in range(600) + ] + mock_repo.git.reflog.return_value = "\n".join(lines) + + result = git_reflog(mock_repo) + + assert isinstance(result, list) + last = result[-1] + assert "warning" in last, "Last entry must be a warning dict for large output" + assert "⚠️" in last["warning"] + assert "KB" in last["warning"] + + def test_git_reflog_no_warning_for_small_output(self): + """Should not append a warning dict for small reflog output.""" + mock_repo = Mock() + mock_repo.git.reflog.return_value = _make_raw( + ("a" * 40, "HEAD@{0}", "commit: initial commit") + ) + + result = git_reflog(mock_repo) + + assert isinstance(result, list) + assert len(result) == 1 + assert "warning" not in result[-1] + + +class TestGitReflogMaxCountContract: + """Test max_count <= 0 means no limit (FIX 4).""" + + def test_git_reflog_max_count_negative_treated_as_no_limit(self): + """max_count <= 0 must not pass -n flag (no-limit behaviour).""" + mock_repo = Mock() + mock_repo.git.reflog.return_value = "" + + git_reflog(mock_repo, max_count=-1) + + args = mock_repo.git.reflog.call_args[0] + assert "-n" not in args + + def test_git_reflog_max_count_zero_treated_as_no_limit(self): + """max_count=0 must not pass -n flag (explicit no-limit contract).""" + mock_repo = Mock() + mock_repo.git.reflog.return_value = "" + + git_reflog(mock_repo, max_count=0) + + args = mock_repo.git.reflog.call_args[0] + assert "-n" not in args