fix(git): bypass GitPython for stash ops to fix worktree failure (#189)

[MementoRC]

Summary

• Replaces repo.git.stash() with subprocess.run() for all four stash operations so they work correctly in git linked worktrees
• Strips GIT_DIR, GIT_WORK_TREE, GIT_INDEX_FILE, GIT_COMMON_DIR from the subprocess environment, matching what a plain shell git stash sees
• Converts exit-code-1 + "No local changes to save" to an info result (ℹ️) rather than an error — checked on both stdout and stderr
• Validates stash_id format (stash@{N}) in git_stash_pop and git_stash_drop before passing to git
• Guards all four stash functions against bare repos (repo.working_dir is None)
• Adds pagination notes to scanning.py listing function docstrings
• Adds missing ValueError/ConnectionError tests for github_get_ruleset and github_get_branch_rules
• Documents GitHubGetBranchRules.branch has no ref-name validator intentionally (API accepts glob patterns)

Root cause

GitPython's repo.git.stash() fails with exit code 1 in linked worktrees even when changes exist. Git interprets the working tree as clean because GitPython may set GIT_DIR or run in a context that prevents git from finding the correct index/work-tree relationship. The plain shell git stash works because it inherits no such env vars and auto-detects everything from the .git file in the worktree directory.

Test plan

• test_git_stash_push_returns_success_when_changes_exist — basic happy path
• test_git_stash_push_with_message / test_git_stash_push_with_message_containing_spaces
• test_git_stash_push_returns_info_when_nothing_to_stash — mock-based (stdout path)
• test_git_stash_push_returns_info_when_nothing_to_stash_stderr — mock-based (stderr path)
• test_git_stash_push_in_worktree — regression test for git_stash_push fails with exit code 1 even when unstaged changes exist #189; creates a real linked worktree
• test_git_stash_push_include_untracked
• test_git_stash_list_empty / test_git_stash_list_shows_stash
• test_git_stash_pop_latest / test_git_stash_drop_latest / test_git_stash_drop_by_id
• ValueError/ConnectionError tests for github_get_ruleset and github_get_branch_rules

Closes #189

🤖 Generated with Claude Code

[github-actions]

Claude finished @MementoRC's task —— View job

---

PR Review

• Gather context and read changed files
• Review _stash_ops.py changes
• Review test coverage
• Review other changed files
• Post final review

---

Overall this is a clean, well-scoped fix. The root cause analysis is sound and the approach (subprocess bypass + env stripping) is the correct solution for the worktree problem. Below are my findings, ordered by severity.

Potential Bugs / Issues

1. _stash_ops.py:83–84 — "No local changes" detection is fragile

combined = result.stdout.strip() or result.stderr.strip()
if result.returncode == 1 and "No local changes to save" in combined:

stdout.strip() or stderr.strip() only checks stderr when stdout is empty/whitespace. If git writes both a partial stdout and "No local changes to save" to stderr (unusual but possible), the match would be missed and the function would fall through to the error branch. A safer pattern:

combined = result.stdout + result.stderr
if result.returncode == 1 and "No local changes to save" in combined:

This is intentionally covered by two separate tests (stdout vs stderr paths), but the or logic means the stderr path only triggers when stdout is blank — the two tests don't cover the case where both are non-empty.

2. git stash pop conflict scenario returns returncode != 0 as a generic error

When git stash pop encounters merge conflicts it exits with code 1. The current error message (❌ Stash pop failed: ...) is appropriate, but it would be worth noting in the docstring that callers should expect this and check for conflicts. This is minor and a non-blocking nit.

3. _stash_ops.py:34–42 — No timeout on subprocess

def _run_git(args: list[str], cwd: str) -> subprocess.CompletedProcess[str]:
    return subprocess.run(
        ["git", *args],
        cwd=cwd,
        env=_clean_git_env(),
        capture_output=True,
        text=True,
    )

No timeout= is set. In pathological cases (network filesystem, lock contention) git can hang indefinitely. Consider adding a timeout=30 (or configurable value) to prevent the MCP server from hanging.

Code Quality

4. _stash_ops.py:29–31 — _clean_git_env() copies the entire environment on every call

def _clean_git_env() -> dict[str, str]:
    return {k: v for k, v in os.environ.items() if k not in _GIT_ENV_KEYS}

This is called once per stash operation and rebuilds the filtered env dict each time. This is fine for the current use case but slightly wasteful. No action required — just an observation.

5. _stash_ops.py:26 — Regex anchoring is correct

_STASH_ID_RE = re.compile(r"^stash@\{\d+\}$")

The anchors prevent injection (e.g. stash@{0}; rm -rf).

6. Bare repo guard (repo.working_dir is None) — consistent across all 4 functions

Well done — all four functions check this early and return a clear error.

Test Coverage

7. test_stash_ops.py — Missing negative tests for stash_pop and stash_drop validation

git_stash_pop and git_stash_drop both validate stash_id format with _STASH_ID_RE, but there are no tests for the invalid-ID rejection path. E.g.:

def test_git_stash_pop_rejects_invalid_stash_id(self):
    result = git_stash_pop(self.repo, stash_id="../../etc/passwd")
    assert "❌" in result
    assert "Invalid stash ID" in result

This is particularly important given the security aspect of the regex guard.

8. test_stash_ops.py — No bare repo test

The bare-repo guard (repo.working_dir is None) has no test coverage. A simple test using Repo.init(str(path), bare=True) would cover this path.

9. test_stash_ops.py:86–96 — Worktree regression test is well-structured

The test_git_stash_push_in_worktree test correctly creates a real linked worktree and uses finally to clean up.

10. test_stash_ops.py:56–84 — Mock-based "nothing to stash" tests import inside the test method

def test_git_stash_push_returns_info_when_nothing_to_stash(self):
    from unittest.mock import MagicMock, patch
    import subprocess as _sp

Imports inside test methods work but are unconventional — moving them to the module top level would be cleaner.

Security

11. stash_id validation prevents shell injection — but message is not validated

The -m <message> argument is passed via args.extend(["-m", message]) as a list element — not via shell expansion — so shell injection is not possible. Correct. No issue here, but worth noting explicitly if this ever gets refactored.

Other Files

12. models.py:1022 — GitHubGetBranchRules.branch comment is clear

branch: str  # Branch name or glob pattern — no ref-name validation (API accepts patterns)

This is a good inline explanation for why the validator was intentionally omitted.

13. scanning.py — Pagination docstring additions are accurate and helpful.

---

Summary

Severity	Count	Items
Bug risk	1	combined = stdout or stderr logic (#1)
Reliability	1	Missing subprocess timeout (#3)
Missing tests	3	Invalid stash_id rejection (#7), bare repo guard (#8), combined stdout+stderr (#1)
Style nits	2	In-test imports (#10)
Positives	—	env stripping design, regex anchoring, consistent bare-repo guard, worktree integration test

The combined = stdout.strip() or stderr.strip() issue (#1) is the most worth addressing before merge as it could cause a silent regression if git's output format changes. The missing timeout (#3) is the next highest priority for a production MCP server.