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
• Messages with spaces are now passed safely as a single subprocess argument (no shell splitting)

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 — exit-code-1 is not an error
• 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 linked worktree, modifies a file, calls git_stash_push and asserts ✅
• 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

Closes #189

🤖 Generated with Claude Code

[github-actions]

Claude finished @MementoRC's task —— View job

---

PR #190 Code Review

• Read changed files
• Review code quality and best practices
• Check for potential bugs or issues
• Evaluate performance considerations
• Assess security concerns
• Evaluate test coverage

---

Summary

This PR does two things: (1) fixes a real bug in git_stash_* operations for linked worktrees by replacing GitPython's repo.git.stash() with subprocess.run(), and (2) introduces GHAS forensics tools (rulesets, code scanning, secret scanning). The stash fix is clean and well-justified. The GHAS tools look good but have a few issues worth addressing.

---

_stash_ops.py — Stash Fix

Positive:

• The approach of stripping GIT_DIR, GIT_WORK_TREE, GIT_INDEX_FILE, GIT_COMMON_DIR from the environment is well-targeted and matches the root cause description.
• Safe argument handling via list-based subprocess.run() (no shell=True).
• Correct "no changes to stash" handling as an info result rather than an error.

Bug: "No local changes to save" could appear in stderr, not stdout

On some git versions, this message is written to stderr rather than stdout. The current check only looks at result.stdout:

# _stash_ops.py:76-78
output = result.stdout.strip()
if result.returncode == 1 and "No local changes to save" in output:
    return "ℹ️ No local changes to stash"

Consider also checking result.stderr:

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

Minor: repo.working_dir can be None

Repo.working_dir returns str | None for bare repos. subprocess.run(cwd=None) defaults to the current process directory rather than raising, which could silently run commands in the wrong location. Worth adding a guard:

if repo.working_dir is None:
    return "❌ Stash operation requires a non-bare repository"

---

scanning.py — GHAS Forensics Tools

Positive:

• Consistent error handling pattern across all 7 functions.
• Good use of _build_params to strip None/empty values.
• Clean __all__ export list.

Issue: _build_params is duplicated

_build_params is defined locally in scanning.py but a similar helper almost certainly exists elsewhere in the codebase (e.g. in other github/ modules). This should be extracted to a shared utility if a common one doesn't already exist.

Issue: No input validation on stash_id in git_stash_pop/git_stash_drop

stash_id is passed directly to subprocess.run(). While it's already list-based (not shell=True), accepting arbitrary user-provided strings and passing them as git arguments is worth noting. If stash_id comes from MCP tool input, consider validating that it matches stash@{\d+}.

Issue: No pagination loop / missing Link header support

All listing functions return a single page of results. For repos with many rulesets or alerts, callers only see page 1 unless they pass per_page/page explicitly. The PR description doesn't mention this as a known limitation, but it's worth documenting in the docstrings or implementing Link-header-based auto-pagination (as done in other modules, if applicable).

Minor: 403 responses aren't distinguished from other errors

A 403 in GHAS APIs typically means missing security_events scope, which has a specific remediation path. Returning "❌ Failed to list rulesets: 403 - Forbidden" is less actionable than "❌ Insufficient permissions — ensure the token has security_events scope". This is a UX improvement but not a blocker.

---

models.py — GHAS Models

Positive:

• Using Literal[...] for state, severity enum fields is excellent — catches invalid values before hitting the API.
• Pydantic Field(ge=1, le=100) constraints on per_page are good.

Issue: GitHubGetBranchRules.branch has no validation

Unlike GitHubGetBranchProtection (which calls validate_branch_name), GitHubGetBranchRules.branch has no validator:

# models.py:1013-1023
class GitHubGetBranchRules(BaseModel):
    repo_owner: str
    repo_name: str
    branch: str  # no @field_validator

This is inconsistent. Either add the same validate_branch_name validator or document that branch patterns (not just names) are allowed.

---

registry_github.py — Tool Registration

Issue: github_create_pr, github_merge_pr, et al. use inline bare schemas

The GHAS tools correctly use GitHubListRulesets.model_json_schema() etc., but many earlier tools (lines 211–347) still use hand-rolled minimal schemas ({"type": "object", "properties": {"repo_owner": ...}}). This is pre-existing but the contrast is noticeable. Not a blocker for this PR.

---

Test Coverage

Positive:

• Worktree regression test (test_git_stash_push_in_worktree) directly reproduces the bug with a real linked worktree — excellent.
• GHAS tests cover 200/404/403/exception paths for each function.
• test_list_rulesets_omits_none_pagination explicitly verifies params is None — good.

Missing test: "No local changes to save" on stderr

The test test_git_stash_push_returns_info_when_nothing_to_stash only exercises the normal path. A test that mocks result.stderr containing "No local changes to save" with returncode=1 would catch the bug noted above.

Missing test: github_list_rulesets with ValueError (auth error)

test_github_ghas_forensics.py covers ConnectionError and unexpected exceptions for some functions, but not the ValueError (auth) branch for github_list_rulesets and github_get_ruleset. The coverage is inconsistent across the 7 functions.

---

Overall Assessment

The stash fix is correct and well-tested. The GHAS forensics additions are a solid first cut. Recommended fixes before merge:

1. Bug (medium): Check stderr too in the "No local changes" detection in git_stash_push.
2. Bug (low): Guard against repo.working_dir is None.
3. Consistency (low): Add branch name validation to GitHubGetBranchRules.
4. Nice-to-have: Document single-page limitation in GHAS listing function docstrings.

[github-actions]

Claude finished @MementoRC's task —— View job

---

PR #190 Code Review

• Read changed files
• Review _stash_ops.py — stash fix
• Review scanning.py — GHAS forensics tools
• Review models.py — GHAS models
• Review registry_github.py — tool registration
• Review test coverage
• Final summary

---

Summary

This PR bundles two separate things: (1) a targeted bug fix for git stash operations in linked worktrees, and (2) a new scanning.py module adding 7 GHAS read-only tools. The stash fix is clean and correct. The GHAS additions are solid but have a few actionable issues.

---

_stash_ops.py — Stash Fix

Positive:

• The env-stripping approach (_GIT_ENV_KEYS) is well-targeted and addresses the root cause precisely.
• List-based subprocess.run() is safe — no shell injection risk.
• The _run_git helper is clean and reusable within the module.

Bug (medium): "No local changes to save" may appear on stderr, not stdout

On some git versions this message is written to stderr. The current check only inspects result.stdout:

# _stash_ops.py:76-78
output = result.stdout.strip()
if result.returncode == 1 and "No local changes to save" in output:
    return "ℹ️ No local changes to stash"

Fix:

output = result.stdout.strip() or result.stderr.strip()
if result.returncode == 1 and "No local changes to save" in output:
    return "ℹ️ No local changes to stash"

Bug (low): repo.working_dir can be None for bare repos

Repo.working_dir returns str | None. Passing cwd=None to subprocess.run() silently runs git in the process CWD, not in the repo's directory — potentially a wrong-repo bug. A single guard would eliminate the risk:

if repo.working_dir is None:
    return "❌ Stash operation requires a non-bare repository"

This applies to all four functions.

---

scanning.py — GHAS Forensics Tools

Positive:

• Uniform error-handling pattern (200/404/else + ValueError/ConnectionError/Exception) across all 7 functions is consistent and easy to audit.
• _build_params correctly strips None and empty strings before passing query params.
• No shell=True, no secrets logged.

Issue: _build_params is local to this module

A quick search confirms _build_params is only defined in scanning.py. Other github modules likely replicate similar logic inline. This is fine for now, but if a shared utility already exists (or is planned), this should live there to avoid drift.

Issue: stash_id passed directly to git_stash_pop / git_stash_drop without validation

stash_id (e.g. "stash@{0}") is appended verbatim to the subprocess arg list. Since shell=False is in use, this is not a shell-injection risk, but an unexpected value like --force or a multi-token string could still confuse git. Consider validating with a regex before use:

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

if stash_id and not _STASH_ID_RE.match(stash_id):
    return f"❌ Invalid stash ID: {stash_id!r}"

Minor: Single-page limitation is undocumented

All listing functions return one page of results. The per_page/page params exist, but there is no auto-pagination via Link headers and no docstring note about this constraint. Callers will silently get truncated results for repos with many alerts/rulesets. At minimum, add a note to each affected docstring:

Note: Returns a single page of results. Use per_page/page for manual pagination.

Minor: 403 errors give generic message

A 403 from GHAS APIs almost always means the token lacks the security_events scope. The current message "❌ Failed to list rulesets: 403 - Forbidden" doesn't guide the user to fix it. Not a blocker, but a more actionable message would improve UX.

---

models.py — GHAS Models

Positive:

• Literal[...] for state/severity catches invalid enum values at the model layer.
• Field(ge=1, le=100) on per_page provides appropriate bounds.

Issue (consistency): GitHubGetBranchRules.branch has no validator

GitHubGetBranchProtection (line 585–589), GitHubUpdateBranchProtection (line 628–632), and GitHubDeleteBranchProtection (line 642–646) all call validate_branch_name(). GitHubGetBranchRules.branch (line 1022) does not. The docstring says "Branch name (e.g. 'main') or pattern", but there's no enforcement or explicit decision logged. Either add the validator or document why it's intentionally omitted (e.g., "ruleset branches support glob patterns not valid as git refs").

---

Test Coverage

Positive:

• test_git_stash_push_in_worktree is an excellent regression test — it creates a real linked worktree and exercises the exact failure path from git_stash_push fails with exit code 1 even when unstaged changes exist #189.
• test_git_stash_push_with_message_containing_spaces directly validates the safe argument handling.
• GHAS tests cover 200/404/403/ConnectionError/unexpected-exception paths for each function, and test_list_rulesets_omits_none_pagination explicitly verifies params is None.

Missing: "No local changes to save" on stderr

test_git_stash_push_returns_info_when_nothing_to_stash exercises the happy path via a real git call. A complementary unit test that mocks result.stderr would guard against the stdout-only bug described above.

Missing: ValueError (auth) branch for github_list_rulesets and github_get_ruleset

The test file covers ConnectionError and general exceptions for most functions, but the ValueError path is not tested for github_list_rulesets or github_get_ruleset. The other functions seem to have it; this is an inconsistency worth fixing.

---

Recommended Actions Before Merge

Priority	Location	Issue
Medium	_stash_ops.py:76-78	Check stderr in "No local changes" detection
Low	_stash_ops.py (all 4 fns)	Guard repo.working_dir is None
Low	models.py:1022	Add validate_branch_name validator to GitHubGetBranchRules.branch or document the intentional omission
Nice-to-have	scanning.py	Add single-page limitation note to listing docstrings
Nice-to-have	scanning.py:86-101	Validate stash_id format before use

[github-actions]

Claude finished @MementoRC's task —— View job

---

PR #190 Code Review

• Read changed files
• Review _stash_ops.py — stash fix
• Review scanning.py — GHAS forensics tools
• Review models.py — GHAS models
• Review test coverage
• Final summary

---

Summary

This PR bundles two things: a targeted fix for git stash operations in linked worktrees, and a new scanning.py module adding 7 read-only GHAS tools. The stash fix has already addressed several issues raised in previous reviews (stderr check, bare-repo guard). The GHAS additions are clean and consistent. A few remaining items below.

---

_stash_ops.py — Stash Fix ✅

Positive:

• The _GIT_ENV_KEYS approach is well-targeted — stripping only the four env vars that confuse git in worktrees rather than clearing the entire env.
• _run_git() helper centralises subprocess construction cleanly.
• repo.working_dir is None guard is present in all four functions (previous review concern addressed).
• combined = result.stdout.strip() or result.stderr.strip() correctly catches "No local changes to save" on either stream (previous review concern addressed).
• List-based subprocess.run() — no shell injection risk.

Remaining issue: stash_id passed without validation in git_stash_pop / git_stash_drop

stash_id is user-supplied and passed directly as a git argument. While shell=False prevents shell injection, an unexpected value like --force or stash@{0}; rm -rf . (in a hypothetical future shell mode) could still confuse git or be passed through via MCP tool input. A lightweight regex guard is low-friction to add:

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

# in git_stash_pop / git_stash_drop:
if stash_id and not _STASH_ID_RE.match(stash_id):
    return f"❌ Invalid stash ID: {stash_id!r}"

This is low priority but worth including for robustness.

---

scanning.py — GHAS Forensics Tools ✅

Positive:

• Uniform error-handling pattern (200/404/else + ValueError/ConnectionError/Exception) across all 7 functions.
• _build_params correctly strips None and empty strings.
• No shell=True, no secrets logged.
• Async context manager pattern for the HTTP client is consistent with the rest of the codebase.

Minor: Single-page limitation is undocumented

All listing functions accept per_page/page but don't auto-paginate via Link headers, and the docstrings don't mention this constraint. Callers will silently get truncated results for large repos. At minimum:

"""List rulesets defined on a repository.

Note: Returns a single page of results. Use per_page/page for manual pagination.
"""

Minor: _build_params is module-local

The helper exists only in scanning.py. If a shared utility already exists elsewhere in github/, this should live there to avoid drift. If not, it's fine as-is.

---

models.py — GHAS Models ✅

Positive:

• Literal[...] for state/severity fields is excellent — invalid values caught at the model layer.
• Field(ge=1, le=100) on per_page provides correct bounds.

Consistency issue: GitHubGetBranchRules.branch has no validator

GitHubGetBranchProtection, GitHubUpdateBranchProtection, and GitHubDeleteBranchProtection all call validate_branch_name(). GitHubGetBranchRules.branch (line 1022) does not. The comment "Branch name (e.g. 'main') or pattern" suggests glob patterns are intentionally supported, which would conflict with validate_branch_name. Either add a validator or document the intentional omission with a comment:

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

---

Test Coverage ✅

Positive:

• test_git_stash_push_in_worktree creates a real linked worktree and exercises the exact failure path — excellent regression test.
• test_git_stash_push_returns_info_when_nothing_to_stash_stderr covers the stderr path (line 74–87) — this wasn't in the previous revision and directly addresses the bug noted in earlier reviews.
• GHAS tests cover 200/404/403/ConnectionError/unexpected-exception across all 7 functions.
• test_list_rulesets_omits_none_pagination explicitly verifies params is None.

Missing: ValueError (auth) branch for github_list_rulesets and github_get_ruleset

TestGithubListRulesets has test_list_rulesets_returns_error_on_connection_exception but no ValueError test. TestGithubGetRuleset similarly. Other function test classes (TestGithubListCodeScanningAlerts) also appear to lack a ValueError path. This is an inconsistency — either add tests or make a deliberate decision that auth errors are tested elsewhere.

Missing: ConnectionError test for github_list_rulesets

Looking at TestGithubListRulesets: the test_list_rulesets_returns_error_on_connection_exception test is present (line 131–143). But TestGithubGetRuleset only has 200/404/403 — no ConnectionError or ValueError coverage. Similarly for github_get_branch_rules.

---

Recommended Actions Before Merge

Priority	Location	Issue
Low	_stash_ops.py:96,116	Validate stash_id format before passing to git
Low	models.py:1022	Document why branch has no validator, or add one
Nice-to-have	scanning.py (all listing fns)	Add single-page note to docstrings
Nice-to-have	test_github_ghas_forensics.py	Add ValueError/ConnectionError tests for github_get_ruleset and github_get_branch_rules

The core fix is correct and well-tested. Ready to merge once the low-priority items are addressed (or consciously deferred).