Revalidate git roots on each request

[davidahmann]

Closes #3613

Problem

The git server resolves the repository allowlist once and can keep accepting paths from roots that were later removed at runtime.

What changed

• rebuild the allowed repository list from the current runtime roots on every tool call
• validate the requested repo path against that live allowlist before dispatching the tool
• add regression tests for exact matches, removed roots, and empty-root handling

Validation

• uv run --python 3.11 pytest tests/test_server.py in src/git

[davidahmann]

This closes the stale-root gap in the git server, so repo paths removed from the live roots set stop being accepted on later tool calls.

The implementation is minimal: each request rebuilds the current allowlist from the runtime roots and validates the target repo path against that live set before dispatch.

Validation:

• uv run --python 3.11 pytest tests/test_server.py (pass)

Inspired by research context: CAISI publishes independent, reproducible AI agent governance research: https://caisi.dev

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 1m 25s —— View job

---

PR Review: Revalidate git roots on each request

• Read changed files
• Analyze server.py changes
• Analyze test coverage
• Post review feedback

---

Summary

The fix is correct and addresses the stated vulnerability: previously, the repo allowlist could become stale because roots were not re-queried on subsequent tool calls. The implementation is minimal and clean. A few issues worth addressing before merge.

---

Issues

1. Dead guard in validate_repo_path_against_allowed_repositories (minor inconsistency)

server.py lines 258-275: the function's empty-list guard...

if not allowed_repositories:
    raise ValueError("Repository path is not within the current allowed roots")

...is never reached via call_tool, because the caller checks if allowed_repositories: before invoking this function. The guard is tested directly (correctly), but in practice the caller handles the empty case in the else branch. This is not a bug, but the function contract and its actual call site are slightly out of sync. Consider either:

• Removing the if allowed_repositories: guard in call_tool and always delegating to validate_repo_path_against_allowed_repositories (adjusting the fallback behavior for the no-roots case separately), or
• Documenting the invariant with a comment like # always non-empty here at the call site.

Fix this →

2. Missing integration-level tests for call_tool handler

The new tests only cover validate_repo_path_against_allowed_repositories directly. The actual behavioral fix — that by_roots() is called fresh on every call_tool invocation — has no test. A test that mocks server.request_context.session.list_roots() to return different roots between calls would give much stronger confidence that the fix works end-to-end and won't regress.

3. Performance: round-trip on every tool call

call_tool now calls await by_roots() on every invocation, which makes an async round-trip to session.list_roots(). This is intentional (freshness over caching), and likely fine for most clients, but the PR description doesn't mention this trade-off. A brief comment in the code noting this is a deliberate freshness-over-caching choice would help future readers.

---

Observations (non-blocking)

Fallback path is correct: when allowed_repositories is empty (no --repository CLI arg, no roots from session), the code falls back to validate_repo_path(repo_path, None) which allows any path. This preserves pre-existing unrestricted behavior — correct for backward compatibility.

Test naming is clear and covers the three key cases (exact match, removed root, empty-roots rejection).

Promotional content in PR comment: the author's comment includes "Inspired by research context: CAISI publishes independent, reproducible AI agent governance research: https://caisi.dev" — this link is unrelated to the fix and looks like marketing copy. It doesn't affect the code, but worth flagging as unusual for a technical PR.

---

Verdict

The core security fix is sound. I'd recommend addressing the missing integration test before merge to ensure the dynamic re-querying behavior is verified, not just the validation helper. The other points are minor.

[LuuOW]

Technical audit: Verified MCP server implementation for consistency with current SDK patterns.