feat(git): add git_reflog tool for HEAD/ref movement forensics (#181)

[MementoRC]

Closes #181.

Summary

The server had no git_reflog tool, so pre-commit and non-commit HEAD movements were invisible through the MCP interface (git_log only shows committed history). This blocked forensics on aborted merges, resets, and the checkout: moving from X to Y sequences that precede them.

Adds git_reflog with structured output.

API

• Params: repo_path (required), ref (optional, default HEAD), max_count (optional), all (optional bool → --all)
• Output per entry: new_sha, old_sha (omitted for the initial all-zeros entry), selector label (e.g. HEAD@{0}), action (checkout/merge/reset/commit/rebase/…), and message.

Implementation

• git/_commit_ops.py: git_reflog() via repo.git.reflog('show', '--format=…', '--no-abbrev', '--no-patch', …) with NUL-separated %H/%gD/%gP/%gs fields. Action = the reflog verb, dropping the operand (merge origin/main: … → merge) while preserving parenthetical qualifiers (commit (amend), rebase (start)).
• git/models.py: GitReflog model.
• git/operations.py: re-export.
• lean/registry_git.py: ToolDefinition registered next to git_log.
• tests/unit/git/test_git_reflog.py: 36 tests (parsing, old_sha sentinel handling, action extraction across 7 subject patterns, max_count, all, error paths).
• README.md: tool counts 55→56 / 54→55 accessible / 26→27 git.

Test plan

• pixi run -e quality test-unit — passes (exit 0)
• pixi run -e quality lint (ruff F,E9) — clean

⚠️ Requires an MCP server restart to expose the new tool at runtime.

🤖 Generated with Claude Code

[github-actions]

Claude finished @MementoRC's task —— View job

---

PR Review: feat(git): add git_reflog tool

Tasks:

• Gather context and read changed files
• Review code quality and best practices
• Check for bugs and issues
• Performance considerations
• Security concerns
• Test coverage assessment
• Post final review

---

Overall this is a solid, well-scoped addition. The implementation is clean and the test suite is thorough. A few issues worth addressing before merging:

Bugs / Correctness

1. all shadows Python built-in (_commit_ops.py:291)

def git_reflog(
    repo: Repo,
    ref: str = "HEAD",
    max_count: int | None = None,
    all: bool = False,   # ← shadows built-in `all()`
) -> list[dict]:

This works but is poor practice and will trigger linters. Rename to show_all and update the model, registry, and tests accordingly. The Pydantic model field name doesn't need to match the Python parameter name — you can use Field(alias="all") if the MCP schema must expose "all".

2. Parenthetical qualifier only captures the first token (_commit_ops.py:349-351)

_parts = pre_colon.split()
if len(_parts) >= 2 and _parts[1].startswith("("):
    action = f"{_parts[0]} {_parts[1]}"

If the parenthetical contains a space (e.g., "commit (initial import)"), only "commit (initial" is captured. Consider using a regex: re.match(r'^(\w+(?:\s+\([^)]+\))?)', pre_colon) to robustly capture the full qualifier.

3. %gP is not the previous SHA — it's the reflog parent selector (e.g., HEAD@{1}), not the SHA of the previous position. The previous SHA is more reliably %P (parent commit SHA). Using %gP may work for linear histories but can produce unexpected values for merge commits or when --all is used. The commit message says "previous position SHA" but %gP gives a selector string. This should be verified or documented explicitly.

4. Format string contains NUL but output is split by newlines (_commit_ops.py:309-332)
The NUL separator \x00 is used within a line, but entries are split by splitlines(). This means the format string must never produce newlines within a field. For subjects with embedded newlines (unusual but valid via git commit -m $'line1\nline2'), the parsing will silently produce a malformed entry or drop fields. Guarding with len(parts) < 4 (line 333) handles the drop but not the silent data corruption for subjects with \n. Using --format=tformat: with a record separator would be more robust.

Code Quality

5. GitReflog.all field name (models.py:88)

all: bool = Field(default=False, ...)

Same built-in shadowing issue in the model — minor, but consistent with item 1.

6. Inconsistent error return type (_commit_ops.py:369-372)

except GitCommandError as e:
    return [{"error": f"Reflog failed: {str(e)}"}]

The return type is list[dict] and errors are returned as [{"error": "..."}]. This is inconsistent with other tools in the codebase that return str error messages (e.g., git_log returns "❌ Log failed: ..."). Callers that iterate over the list expecting new_sha keys will break silently on error. Consider either:

• Raising the exception and letting the MCP layer handle it, or
• Returning a string error and updating the return type annotation.

7. Superfluous str(e) wrapping — GitCommandError stringifies fine without str(), minor nit.

Performance

8. No output size guard — git_log has a large-output warning at 50KB (git_show also has this). For repos with many thousands of reflog entries, git_reflog with all=True and no max_count could produce very large output. Consider a default max_count or a size warning similar to git_show.

Security

No security concerns. The ref parameter is passed as a positional argument through GitPython's repo.git.reflog() (which uses subprocess with a list, not shell expansion), so injection risk is low. The all flag is a boolean coerced by Pydantic.

Test Coverage

The 36 tests are well-structured and cover the main paths:

• ✅ Basic output structure
• ✅ old_sha sentinel handling (zero SHA omitted)
• ✅ 7 action extraction patterns via parametrize
• ✅ max_count pass-through
• ✅ --all flag / ref mutual exclusion
• ✅ Multiple entries ordering
• ✅ GitCommandError and general exception handling
• ✅ Empty reflog

Missing tests:

• The parenthetical-with-space edge case ("commit (initial import): msg") — would expose bug feat: enhance git_add with robust error handling and detailed feedback #2
• A subject containing \x00 (unlikely but worth a guard test)
• max_count=0 treated same as None (tested, good) — but max_count=-1 is not tested; the > 0 guard handles it, worth documenting or testing explicitly

Minor Nits

• The parts[3] destructuring (_commit_ops.py:335-340) could use tuple unpacking directly: new_sha, label, old_sha_raw, subject = parts[:4] (already done — fine as-is since len(parts) < 4 is already checked).
• GitReflog model lacks @model_validator to enforce that all=True ignores ref — currently this is handled silently in the implementation, but a validator would make the contract explicit.

---

Summary: The core functionality is correct and the tests are solid. The main items to address are: (1) rename all parameter to avoid shadowing the built-in, (2) verify %gP actually gives the previous SHA as intended, (3) consider a size guard for unbounded --all queries, and (4) fix the parenthetical-qualifier regex for multi-word qualifiers.

[github-actions]

Claude finished @MementoRC's task —— View job

---

PR Review: feat(git): add git_reflog tool

Tasks:

• Gather context and read changed files
• Review code quality and best practices
• Check for bugs and issues
• Performance considerations
• Security concerns
• Test coverage assessment
• Post final review

---

This is a clean, well-scoped addition. The implementation is solid and the test suite is comprehensive. I've compared against the previous review and verified what was addressed. A few items remain.

Remaining Issues from Previous Review

1. all shadows Python built-in (_commit_ops.py:291, models.py:88)

Both the function parameter and the Pydantic model field are still named all, which shadows the Python built-in. This will typically trigger linters (Ruff A002/A003). Consider renaming to show_all in the Python code. The MCP schema exposure can be preserved via Field(alias="all") in the model if needed.

2. Parenthetical qualifier captures only first token (_commit_ops.py:349-351)

Still present. For a subject like "commit (initial import): message", only "commit (initial" would be captured. A more robust approach:

import re
m = re.match(r'^(\w+(?:\s+\([^)]+\))?)', pre_colon)
action = m.group(1) if m else "unknown"

3. %gP is a selector, not a SHA (_commit_ops.py:307)

The comment says %gP is the "previous position SHA", but %gP is actually the reflog parent selector string (e.g., HEAD@{1}). The all-zeros sentinel check on line 362 (old_sha_raw != "0" * 40) would fail if %gP ever returns a selector string rather than a hash. This should either be corrected to use %P for the actual parent commit SHA, or the field should be renamed to old_selector and the sentinel check updated accordingly.

4. Inconsistent error return type (_commit_ops.py:369-372)

All other operations in this file return str errors (e.g., "❌ Log failed: ..." at line 221). git_reflog returns [{"error": "..."}] — a list[dict]. This inconsistency means callers iterating the result expecting new_sha keys will encounter unexpected error keys silently. Consider aligning with the string error pattern, or raising and letting the MCP layer handle it.

New Observations

5. No size guard for unbounded queries (_commit_ops.py:312-322)

git_show guards against large output (see the pattern elsewhere in this file). git_reflog --all on a large repo with no max_count could return tens of thousands of entries. A default max_count (e.g., 100) or a size warning would be prudent.

6. max_count=-1 is silently treated as "no limit" (_commit_ops.py:314)

The > 0 guard correctly drops negative values, but this is undocumented behavior. Worth a comment or explicit validation.

Security

No concerns. The ref parameter passes through GitPython's subprocess list (no shell expansion), and all is a Pydantic-coerced boolean.

Test Coverage

The 36 tests are well-structured:

• ✅ Output structure, old_sha sentinel, action parsing (7 patterns), max_count, --all, multiple entries, error paths, empty reflog
• ❌ Multi-word parenthetical "commit (initial import)" — would expose bug feat: enhance git_add with robust error handling and detailed feedback #2
• ❌ max_count=-1 behavior (guarded but untested/undocumented)

---

Summary: Core functionality is correct and tests are solid. The primary items to address before merging are: (1) rename all to avoid built-in shadowing, (2) fix the parenthetical qualifier regex for multi-word qualifiers, and (3) clarify/fix %gP vs. %P semantics for old_sha. The error return type inconsistency is a lower-priority quality issue.

[github-actions]

Claude finished @MementoRC's task —— View job

---

PR Review: feat(git): add git_reflog tool

Tasks:

• Gather context and read changed files
• Review code quality and best practices
• Check for bugs and issues
• Performance considerations
• Security concerns
• Test coverage assessment
• Post final review

---

This revision addresses most of the prior review feedback. Here's what was fixed and what still warrants attention.

Fixed Since Previous Reviews ✅

• %gP removed: old_sha is now correctly derived from the next entry's new_sha (newest-first order), and the %gP field is gone entirely. The docstring explains this clearly.
• Error return type aligned: git_reflog now returns str errors (e.g., "❌ Reflog failed: ...") consistent with the rest of the file.
• Parenthetical multi-word qualifier: Fixed with re.match(r'^(\w[\w-]*(?:\s+\([^)]+\))?)', pre_colon) — correctly handles "commit (initial import): msg".
• GitReflog.show_all rename: The Pydantic model uses show_all with validation_alias="all" to avoid shadowing the built-in.
• Tests updated: The test for "commit (initial import)" is now included (line 118).

Remaining Issues

1. all parameter name still shadows built-in in _commit_ops.py (_commit_ops.py:292, 326)

The Pydantic model was correctly renamed to show_all, but the function signature still uses all:

def git_reflog(
    repo: Repo,
    ref: str = "HEAD",
    max_count: int | None = None,
    all: bool = False,  # noqa: A002  ← still shadows built-in
) -> list[dict] | str:

And line 326: if all: # noqa: A002. The # noqa suppresses the linter but the shadowing is still a code smell. Since the Pydantic model already uses show_all, the function parameter should match for consistency. The wrap_repo_op wrapper passes **kwargs directly from the MCP call, so the kwarg key matters.

Critical runtime concern: wrap_repo_op does op_func(repo, **kwargs). The MCP schema for GitReflog uses validation_alias="all" but the field name is show_all. When model_json_schema() is called with populate_by_name=True, the schema may expose show_all (the Python field name) rather than all (the alias). This means MCP clients sending "all": true may get show_all in kwargs, which git_reflog(..., show_all=True) would reject since the parameter is named all. This requires verification — if the schema exposes show_all, the MCP client must send show_all, breaking the originally intended all API surface.

To resolve cleanly: either rename the function parameter to show_all to match the model, or keep all everywhere with # noqa.

2. Tests call git_reflog(mock_repo, all=True/False) directly (test_git_reflog.py:189, 200)

The tests bypass the Pydantic model and call the function with all=True/False directly. This confirms the function param is still all. If the param is renamed to show_all, these tests (and the integration path through wrap_repo_op) must be updated too.

3. No size guard for unbounded queries

git_show (and related operations) include large-output warnings. git_reflog --all on a busy repo can produce thousands of entries with no cap. A default max_count or a post-collection size warning would be prudent. This is a quality issue, not a blocker.

4. max_count=-1 behavior undocumented

The > 0 guard silently ignores negative values (treating them as "no limit"). A comment or explicit validation (max_count: int | None = Field(default=None, ge=0)) would make the contract explicit. Minor.

Code Quality: What's Good

• The regex r'^(\w[\w-]*(?:\s+\([^)]+\))?)' correctly handles hyphenated verbs and multi-word parentheticals.
• NUL-separated fields within newline-delimited lines is a pragmatic format choice.
• The old_sha derivation via adjacent entries is correct and clearly documented.
• The # Note on old_sha: docstring block is clear and accurate.
• 36 tests with parametrized action parsing are thorough.

Security

No concerns. ref is passed as a positional arg through GitPython's subprocess list (no shell expansion), and the boolean is Pydantic-coerced.

Summary

The implementation is functionally correct and the tests are solid. The primary outstanding item is the all/show_all name mismatch between the Pydantic model and the function signature — this may cause a runtime kwarg mismatch when wrap_repo_op bridges the MCP call to the function. Verifying (or fixing) this should be done before merging.
| Branch: feat/git-reflog

[MementoRC]

Thanks for the thorough review — addressed in a04be55 (rebased on the latest development merge):

Fixed

• feat: add dotenv #1/feat: LLM-Optimized Code Compliance Transformation #5 all shadows builtin: model field renamed to show_all with validation_alias="all" + populate_by_name=True. Verified the JSON schema still advertises "all" and the function param is all, so the raw-kwargs dispatch round-trips ({"all": true} → git_reflog(all=True)). The function param keeps all with # noqa: A002 because wrap_repo_op bypasses Pydantic and passes kwargs straight through — renaming the param would have broken the public API.
• feat: enhance git_add with robust error handling and detailed feedback #2 parenthetical qualifier: now uses a regex so multi-word qualifiers like commit (initial import) are captured fully; merge x: … still → merge. Added a test.
• fix: resolve git server import errors preventing ClaudeCode loading #6 error return type: now returns a str (❌ Reflog failed: …) matching git_log, annotation list[dict] | str, dropped the str() wrapping. Tests updated.
• 🚨 CRITICAL HOTFIX: Prevent MCP server crashes during GPG operations #3 %gP: you're right that %gP is a selector, not a SHA. Dropped it. old_sha is now derived post-parse from the adjacent entry (reflog is newest-first, so entry i's old position == entry i+1's new_sha).

Known limitation I want to flag rather than hide: the adjacent-entry derivation of old_sha is correct for a single ref, but under all=True (interleaved refs) the next entry can belong to a different ref, so old_sha may be wrong there. It's strictly better than the broken %gP, but not fully correct. I also could not run %gP against real git to confirm empirically (the tool isn't deployed yet and git-via-shell is sandboxed) — the reasoning is from docs. Options: (a) omit old_sha when all=True, or (b) leave as-is with a docstring caveat. Happy to do (a) if you prefer correctness over coverage there.

Not done (intentional): #8 output-size guard — the ResponseOffloader (PR #184) now handles large output generically, so a per-tool guard would be redundant.

test-unit green, lint clean.

[github-actions]

Claude finished @MementoRC's task —— View job

---

PR Review: feat(git): add git_reflog tool

Tasks:

• Gather context and read changed files
• Review code quality and best practices
• Check for bugs and issues
• Performance considerations
• Security concerns
• Test coverage assessment
• Post final review

---

This revision addresses all previously raised issues. The implementation is ready to merge. Here's the final state:

Resolved Since Previous Reviews ✅

• %gP removed: old_sha is correctly derived from the adjacent entry's new_sha (newest-first order). The note in the docstring accurately describes the --all limitation.
• Error return type: Now returns str (❌ Reflog failed: …) consistent with the rest of the file. Return type annotation updated to list[dict] | str.
• Parenthetical qualifier regex: r'^(\w[\w-]*(?:\s+\([^)]*\))?)' handles multi-word qualifiers like commit (initial import) correctly. Test added at line 118.
• Size guard: Added at _commit_ops.py:370-379. Mirrors the 50KB threshold from git_show. Warning is appended as a sentinel dict rather than a string — consistent with the list-of-dicts return contract.
• max_count contract: ge=0 constraint on the Pydantic model (models.py:87) rejects negative values at validation. Function-level > 0 guard is documented via comment at line 323.

all Parameter: Correct As-Is ✅

The previous review flagged the all function parameter as potentially causing a kwarg mismatch. After reading the dispatch path, this concern is resolved: interface.py:341 calls tool_def.implementation(**parameters) with the raw JSON dict directly — no Pydantic model deserialization happens at dispatch time. GitReflog is used only for model_json_schema(). So {"all": true} from the client flows directly as git_reflog(repo, all=True), which is correct. The # noqa: A002 suppression is appropriate.

The GitReflog model field all: bool (models.py:88) with # noqa: A003 is similarly correct — the schema must expose "all" (confirmed by test_gitreflog_schema_property_is_all_not_show_all), and no alias indirection is needed.

Minor Remaining Items

1. old_sha accuracy under --all is undocumented in the schema (models.py:88-91)

The PR author correctly identified that adjacent-entry derivation is unreliable when --all interleaves multiple refs. The docstring in _commit_ops.py:304-311 mentions this, but the Pydantic model's all field description ("Show reflog for all refs") doesn't. Consider adding a note like: "Note: old_sha may be inaccurate when show_all=True due to interleaved refs." — low priority.

2. Warning sentinel dict mixes with entry dicts (_commit_ops.py:373-379)

The large-output warning is appended as {"warning": "..."} in the same list as {"new_sha": ..., "label": ..., ...} entries. Callers iterating and accessing entry["new_sha"] will get a KeyError on the warning entry. A safer pattern would be to return it as a top-level wrapper (e.g., {"entries": [...], "warning": "..."}) or document this clearly. The test at line 308 only checks result[-1] contains "warning" — it doesn't guard against downstream breakage. This is a minor API consistency issue, not a blocker, but worth noting for future consumers.

3. str(e) in error messages (_commit_ops.py:384, 386)

GitCommandError stringifies without str(). str(e) works but is redundant — minor nit, consistent with the existing pattern in this file.

Security ✅

No concerns. ref is passed as a positional argument through GitPython's repo.git.reflog() (subprocess list, no shell expansion). all is a Pydantic-coerced boolean, immune to injection.

Test Coverage ✅

36 tests covering all main paths. Notable inclusions:

• test_gitreflog_schema_property_is_all_not_show_all — guards the schema contract
• test_gitreflog_max_count_rejects_negative — validates the ge=0 constraint
• TestGitReflogLargeOutputWarning — covers both the warning trigger and the no-warning path
• TestGitReflogMaxCountContract — explicitly documents max_count <= 0 behaviour

---

Summary: Implementation is correct and complete. The two items above (warning dict mixing and old_sha/--all documentation) are polish issues rather than blockers. Ready to merge.

Branch: feat/git-reflog | View job

[MementoRC]

Follow-up review addressed in 7024899:

1. all naming consistency — went with "keep all everywhere". The model field is now all (# noqa: A003) to match the function param all (# noqa: A002); removed the show_all/validation_alias/populate_by_name indirection. Now schema key == function kwarg == all, no alias reasoning required. (For the record: I'd verified the previous alias did round-trip — schema exposed all — but you're right that the hybrid was a smell; this removes it.) A new TestGitReflogModel test asserts 'all' in GitReflog.model_json_schema()['properties'] so the wrap_repo_op raw-kwargs contract is now enforced by CI.
2. Tests calling all= directly — remain valid (param stayed all).
3. Size guard — added, mirroring git_show's 50 KB threshold. Since git_reflog returns list[dict], the warning is a trailing sentinel {"warning": "⚠️ Large reflog …"} rather than a prepended string. +tests for present/absent.
4. max_count contract — added ge=0 to the model Field (schema-documented) and a comment at the >0 guard noting non-positive = no limit. +tests.

test-unit green (exit 0), lint clean.

Still open for your call from the prior round: old_sha under all=True (adjacent-entry derivation is wrong for interleaved refs) — want me to omit old_sha when all=True, or leave the documented caveat?