fix(git): preserve relative paths in git_submodule_add (#168) (#170)

* fix(git): preserve relative paths in git_submodule_add (closes #168)

git_submodule_add was writing ABSOLUTE filesystem paths into .gitmodules
and .git/config because the lean interface's generic "all *path*
parameters must be absolute" validation forced callers to pass absolute
paths, which then propagated verbatim to `git submodule add`. The
result was non-portable repos with entries like:

  [submodule "/home/user/repo/sub-packages/remote-iface"]
      path = /home/user/repo/sub-packages/remote-iface

Submodule paths are repo-relative by git's own convention (see
gitmodules(5)), so this commit introduces a per-tool exemption:
git_submodule_add's `path` parameter now bypasses the absolute-path
requirement while still rejecting `..` and path-traversal attempts.

After the fix:
- Callers pass `path="sub-packages/remote-iface"` (relative)
- The relative path is passed verbatim to `repo.git.submodule(add, ...)`
- .gitmodules entries are portable, matching native git behavior

Tests added:
- Submodule add accepts relative path (regression test for #168)
- Submodule add still rejects `..` traversal
- Validator exemption mechanism unit test

* test(lean): update dotdot rejection assertion for new validation order

PR #170 reordered _validate_path_parameters so the traversal check
('..' component rejection) runs before the absolute-path check.
As a result, repo_path='..' now raises the traversal error
("Invalid path '..': empty paths and '..' traversal components are
not allowed.") instead of the older "Relative path '..' not supported"
message.

Update test_relative_dotdot_rejected to assert against the new
behavior. The test now confirms both that '..' appears in the message
and that it is rejected for traversal reasons — which is the more
precise, security-aligned diagnosis.

Resolves CI failures in:
- ci / Test Python 3.12 on ubuntu-latest
- ci / Code Quality

Co-Authored-By: MementoRC (https://github.com/MementoRC)

* docs(lean): document relative_path_params and non-normalization note

Addresses minor reviewer feedback on PR #170:
- ToolDefinition class docstring now includes a full Args: section
  documenting relative_path_params with the gitmodules(5) rationale
  and a concrete example (``{"path"}`` for git_submodule_add).
- _validate_path_parameters docstring adds a Note: that paths such as
  ``./lib/submod`` or ``lib//submod`` are NOT normalised here — git
  canonicalises them downstream; we only enforce no-``..`` components
  and non-empty values.

* fix(lean): restrict path validation to string params with 'path' in name

_validate_path_parameters had a dead second-pass block that iterated
through every parameter (not just path ones) and rejected any value not
starting with '/'. This caused four CI failures on PR #170:

- test_multiple_params_with_one_invalid_path: the outer block caught
  ``other_param='value'`` before the path-aware branch reached
  ``another_path='relative/bad'``, so the assertion on 'relative/bad'
  failed.
- test_non_path_params_ignored: ``branch_name='main'`` and similar
  non-path strings were rejected with 'Relative path' errors.
- test_path_param_with_non_string_value_ignored: the fallback called
  ``.startswith('/')`` on an int, raising AttributeError.
- test_submodule_add_rejects_path_traversal: the URL value (not a path
  param) was rejected first, masking the expected traversal error on
  ``path='../escape'``.

Fix: collapse to a single guarded branch. Skip any parameter that is
not a string or whose name does not contain 'path'. For string path
params, always reject empty / '..' traversal, then require absolute
unless the param is in the per-tool exempt set (e.g. submodule_add's
'path').

This preserves the exemption mechanism introduced for issue #168 and
restores correct behavior for non-path params and non-string values.

Co-Authored-By: MementoRC (https://github.com/MementoRC)

Author: MementoRC

src/mcp_server_git/lean/interface.py

@@ -27,7 +27,22 @@
 
 
 class ToolDefinition:
-    """Tool definition with metadata for lean MCP registry."""
+    """Tool definition with metadata for lean MCP registry.
+
+    Args:
+        name: Unique tool identifier.
+        implementation: Callable that executes the tool.
+        description: Human-readable description shown in discovery.
+        schema: JSON Schema dict describing the tool's parameters.
+        domain: Tool domain (``"git"``, ``"github"``, ``"azure"``).
+        complexity: Complexity tier (``"core"``, ``"focused"``, ``"advanced"``).
+        examples: Optional list of example invocations.
+        relative_path_params: Names of ``*path*`` parameters that should be
+            treated as repo-relative instead of filesystem-absolute.  Empty
+            paths and ``..`` traversal components are still rejected.
+            Example: ``{"path"}`` for ``git_submodule_add`` — gitmodules(5)
+            requires relative paths in ``.gitmodules``.
+    """
 
     def __init__(
         self,
@@ -64,7 +79,9 @@ def __init__(
         self.domain = domain
         self.complexity = complexity
         self.examples = examples or []
-        self.relative_path_params = relative_path_params or set()
+        # Parameters listed here bypass the "must be absolute" path check.
+        # They still reject ".." and empty strings to prevent traversal.
+        self.relative_path_params: set[str] = relative_path_params or set()
 
 
 class GitLeanInterface:
@@ -166,50 +183,45 @@ def _validate_path_parameters(
         MCP servers resolve paths relative to their process CWD, not Claude Code's
         working directory. This causes cross-repository pollution when using ".".
 
-        Parameters explicitly marked in ``relative_path_params`` are exempt from
-        the absolute-path requirement (e.g. submodule "path" which is repo-relative
-        by git convention per gitmodules(5); issue #168). Exempted parameters are
-        STILL rejected when they are empty or contain ".." path-traversal segments.
+        Parameters listed in *relative_path_params* are exempted from the
+        "must be absolute" requirement (git submodule paths are repo-relative by
+        git's own convention — see gitmodules(5)).  Traversal via ".." and empty
+        strings are still rejected for all parameters.
+
+        Note: paths such as ``./lib/submod`` or ``lib//submod`` are NOT
+        normalised here — git canonicalises them downstream; we only enforce
+        no-``..`` components and non-empty values.
 
         Args:
-            parameters: Dictionary of tool parameters.
-            relative_path_params: Set of parameter names whose values are
-                semantically repo-relative by git convention and should NOT
-                be rejected for being relative. Defaults to no exemptions.
+            parameters: Dictionary of tool parameters
+            relative_path_params: Parameter names that are allowed to be relative.
 
         Raises:
-            ValueError: If any non-exempt path parameter is relative, or if an
-                exempt parameter is empty or contains a ".." traversal segment.
+            ValueError: If any path parameter fails validation
         """
-        exemptions = relative_path_params or set()
+        exempt = relative_path_params or set()
         for param_name, param_value in parameters.items():
-            # Only inspect string-valued parameters whose name contains "path".
-            if not isinstance(param_value, str):
-                continue
-            if "path" not in param_name.lower():
+            # Only validate string parameters whose name contains "path".
+            # Non-path params (e.g. branch_name, commit_message, url) and
+            # non-string values (e.g. int IDs, bools) are ignored.
+            if "path" not in param_name.lower() or not isinstance(param_value, str):
                 continue
 
-            if param_name in exemptions:
-                # Exempted: relative paths are allowed, but reject empty values
-                # and any ".." segment to prevent path-traversal escapes.
-                if param_value == "":
-                    raise ValueError(
-                        f"Path parameter '{param_name}' must not be empty."
-                    )
-                segments = param_value.replace("\\", "/").split("/")
-                if ".." in segments:
-                    raise ValueError(
-                        f"Path traversal ('..') not allowed in '{param_name}': "
-                        f"'{param_value}'."
-                    )
-                continue
+            # Always reject empty strings and ".." traversal components,
+            # even for exempt parameters.
+            if not param_value or ".." in param_value.split("/"):
+                raise ValueError(
+                    f"Invalid path '{param_value}': empty paths and '..' "
+                    f"traversal components are not allowed."
+                )
 
-            # Non-exempt path params must be absolute.
-            if param_value in (".", "..") or not param_value.startswith("/"):
+            # Exempt parameters may be repo-relative (e.g. submodule paths
+            # per gitmodules(5)); all other path params must be absolute.
+            if param_name not in exempt and not param_value.startswith("/"):
                 raise ValueError(
                     f"Relative path '{param_value}' not supported. MCP servers "
-                    f"resolve paths relative to their process CWD, not Claude Code's "
-                    f"working directory. Use absolute path instead."
+                    f"resolve paths relative to their process CWD, not Claude "
+                    f"Code's working directory. Use absolute path instead."
                 )
 
     def _wrap_tool(self, tool_func: Callable, tool_name: str) -> Callable:
@@ -309,10 +321,9 @@ async def execute_tool_direct(
                 }
 
         try:
-            # Validate path parameters (honoring per-tool exemptions, e.g.
-            # submodule "path" which is intentionally repo-relative).
+            # Validate path parameters (pass per-tool exemption set)
             self._validate_path_parameters(
-                parameters, tool_def.relative_path_params
+                parameters, relative_path_params=tool_def.relative_path_params
             )
 
             # Execute tool

tests/lean/test_interface.py

@@ -430,11 +430,18 @@ def test_relative_dot_rejected(self):
         )
 
     def test_relative_dotdot_rejected(self):
-        """Test that '..' is rejected."""
+        """Test that '..' is rejected as a traversal component.
+
+        After issue #168's fix, '..' is caught by the traversal-rejection
+        branch (which runs before the absolute-path check) so the error
+        message references traversal rather than relativeness.
+        """
         params = {"repo_path": ".."}
         with pytest.raises(ValueError) as exc_info:
             self.interface._validate_path_parameters(params)
-        assert "Relative path '..' not supported" in str(exc_info.value)
+        msg = str(exc_info.value)
+        assert "'..'" in msg
+        assert "traversal" in msg
 
     def test_relative_path_rejected(self):
         """Test that relative paths without leading slash are rejected."""