fix(git): preserve relative paths in git_submodule_add (#168)#170
Conversation
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
|
Claude finished @MementoRC's task —— View job Code Review Complete ✅Todo List:
SummaryThis PR provides a well-designed solution to issue #168 by implementing a per-tool exemption mechanism for path validation. The implementation preserves Git's convention that submodule paths should be repo-relative while maintaining strong security for other path parameters. Code Quality & Architecture ⭐⭐⭐⭐⭐Excellent design choices:
Security Analysis ⭐⭐⭐⭐⭐Strong security posture maintained:
Test Coverage ⭐⭐⭐⭐⭐Comprehensive test suite:
Potential Issues & SuggestionsMinor: Documentation EnhancementThe PR description is excellent, but consider adding a docstring example to # Example in interface.py around line 32
def __init__(
self,
# ... other params
relative_path_params: set[str] | None = None,
):
"""
Args:
relative_path_params: Parameter names exempt from absolute path requirement.
Example: {"path"} for git submodule operations where paths
should be repo-relative per Git convention.
"""Edge Case: Path NormalizationConsider whether paths like Performance Considerations ⭐⭐⭐⭐⭐No performance concerns. The path validation is O(n) where n is the number of path components, which is negligible for typical use cases. RecommendationStrong approval ✅ This is a well-crafted solution that:
The implementation demonstrates good software engineering practices and should be merged. |
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)
|
Claude finished @MementoRC's task —— View job Code Review Complete ✅Todo List:
SummaryThis PR provides a well-designed solution to issue #168 by implementing a per-tool exemption mechanism for path validation. The implementation preserves Git's convention that submodule paths should be repo-relative while maintaining strong security for other path parameters. Code Quality & Architecture ⭐⭐⭐⭐⭐Excellent design choices:
Security Analysis ⭐⭐⭐⭐⭐Strong security posture maintained:
Test Coverage ⭐⭐⭐⭐⭐Comprehensive test suite:
Potential Issues & SuggestionsMinor: Documentation EnhancementThe PR description is excellent, but consider adding a docstring example to # Example in interface.py around line 32
def __init__(
self,
# ... other params
relative_path_params: set[str] | None = None,
):
"""
Args:
relative_path_params: Parameter names exempt from absolute path requirement.
Example: {"path"} for git submodule operations where paths
should be repo-relative per Git convention.
"""Edge Case: Path NormalizationConsider whether paths like Performance Considerations ⭐⭐⭐⭐⭐No performance concerns. The path validation is O(n) where n is the number of path components, which is negligible for typical use cases. RecommendationStrong approval ✅ This is a well-crafted solution that:
The implementation demonstrates good software engineering practices and should be merged. |
|
Claude finished @MementoRC's task —— View job Code Review Complete ✅Todo List:
SummaryThis PR provides a well-engineered solution to issue #168 by implementing a selective exemption mechanism for path validation. The implementation preserves Git's convention that submodule paths should be repo-relative while maintaining strong security for other path parameters. Code Quality & Architecture ⭐⭐⭐⭐⭐Excellent design choices:
Security Analysis ⭐⭐⭐⭐⭐Strong security posture maintained:
Test Coverage ⭐⭐⭐⭐⭐Comprehensive test suite added:
Technical Implementation ⭐⭐⭐⭐⭐Well-implemented changes:
Potential Issues & SuggestionsMinor: Enhanced DocumentationConsider adding a usage example to the # Example usage in docstring:
relative_path_params={"path"} # Allow repo-relative paths for Git submodulesEdge Case: Path NormalizationThe current implementation accepts paths like Code Style: Parameter Validation LogicThe validation logic at interface.py:202-208 seems to have unreachable code. The condition checks if Performance Considerations ⭐⭐⭐⭐⭐No performance concerns. The path validation is O(n) where n is the number of path components, which is negligible for typical use cases. The exemption check is O(1) set lookup. RecommendationStrong approval ✅ This is a well-crafted solution that:
The implementation demonstrates excellent software engineering practices with careful attention to security, testability, and maintainability. |
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.
|
Claude finished @MementoRC's task —— View job Code Review Complete ✅Todo List:
SummaryThis PR provides an excellent solution to issue #168 by implementing a selective exemption mechanism for path validation in the Git submodule operations. The implementation allows Code Quality & Architecture ⭐⭐⭐⭐⭐Excellent design choices:
Security Analysis ⭐⭐⭐⭐⭐Strong security posture maintained:
Test Coverage ⭐⭐⭐⭐⭐Comprehensive test suite:
Technical Implementation ⭐⭐⭐⭐⭐Well-implemented changes:
Potential Issues & SuggestionsMinor: Enhanced Code DocumentationConsider adding a docstring example to the # In ToolDefinition.__init__ docstring:
relative_path_params: Set of parameter names whose path-shaped values
are allowed to be repo-relative. Example: {"path"} for git_submodule_add
where paths should be relative per gitmodules(5) convention.Code Quality: Unreachable CodeLines interface.py:221-227 appear unreachable since the code path only executes for non-path parameters, but the condition checks path-related values. This may be dead code that could be cleaned up in a future refactor. Edge Case: Path NormalizationThe implementation accepts paths like Performance Considerations ⭐⭐⭐⭐⭐No performance concerns. The path validation logic is efficient with O(1) set lookups and O(n) path component checks where n is the number of path segments. RecommendationStrong approval ✅ This is an exemplary implementation that:
The solution is well-architected, secure, and maintainable. It addresses the Git convention requirement (repo-relative submodule paths) without compromising the overall security model. |
_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)
|
Claude finished @MementoRC's task —— View job Code Review Complete ✅Todo List:
SummaryThis PR provides an excellent, well-engineered solution to issue #168 by implementing a selective exemption mechanism for path validation. The implementation preserves Git's convention that submodule paths should be repo-relative while maintaining strong security for all other path parameters throughout the system. Code Quality & Architecture ⭐⭐⭐⭐⭐Outstanding design choices:
Security Analysis ⭐⭐⭐⭐⭐Exemplary security posture maintained:
Test Coverage ⭐⭐⭐⭐⭐Comprehensive test suite demonstrates thorough engineering:
Technical Implementation ⭐⭐⭐⭐⭐Well-implemented changes across the codebase:
Performance Considerations ⭐⭐⭐⭐⭐No performance concerns. The path validation logic is efficient with:
Potential Minor Improvements1. Enhanced DocumentationConsider adding a usage example to the # In ToolDefinition.__init__ docstring:
relative_path_params: Set of parameter names whose path-shaped values
are allowed to be repo-relative. Example: {"path"} for git_submodule_add
where paths should be relative per gitmodules(5) convention.2. Edge Case: Path NormalizationThe implementation accepts paths like 3. Code Style: Validation LogicThe validation logic is well-structured, though the comment at interface.py:191-194 could be slightly clearer about the normalization behavior mentioned above. Security Deep DiveThe implementation demonstrates excellent security engineering:
RecommendationStrong approval ✅ This is an exemplary implementation that demonstrates:
The solution elegantly addresses the Git convention requirement (repo-relative submodule paths) without compromising the overall security model. The per-tool exemption approach is architecturally sound and could be extended to other similar cases in the future if needed. This is production-ready code that demonstrates mature software engineering practices. |
1 participant
Root Cause
git_submodule_addwas writing absolute filesystem paths into.gitmodulesand.git/config, producing non-portable repos like:The data flow:
GitLeanInterface._validate_path_parametersenforces an "all params withpathin their name must start with/" rule (interface.py:156).ValueError— it does not coerce them to absolute./home/user/repo/sub-packages/remote-iface), that absolute value is forwarded verbatim torepo.git.submodule("add", url, path)inside_submodule_ops.py— noos.path.abspath()involved..gitmodules, breaking portability.Submodule paths are repo-relative by git's own convention (see
gitmodules(5)). The fix introduces a per-tool opt-out rather than weakening the global rule.Implementation — Strategy A: Per-tool exemption
ToolDefinition(interface.py)Added optional
relative_path_params: set[str] | Nonefield. Parameters in this set bypass the "must be absolute" check while still rejecting empty strings and..traversal._validate_path_parameters(interface.py)Updated to accept the exemption set. For exempted parameters, only traversal/empty guards run. For all other path parameters, the existing absolute-path requirement is unchanged.
execute_tool_direct(interface.py)Passes
tool_def.relative_path_paramsto the validator so per-tool exemptions are applied at execution time.git_submodule_addregistration (registry_git.py)Registered with
relative_path_params={"path"}. Therepo_pathparameter (the repo to operate on) still requires an absolute path.Tests Added (
tests/unit/git/test_submodule_relative_path.py)TestValidatePathParametersExemption— unit-tests the exemption mechanism directly:..(direct and nested)TestGitSubmoduleAddRelativePath— verifiesgit_submodule_addpasses the relative path verbatim torepo.git.submodule(regression for [bug] git_submodule_add writes absolute path verbatim into .gitmodules and .git/config #168)TestSubmoduleAddViaLeanInterface— end-to-end viaexecute_tool_direct:..traversal inpathrepo_path(non-exempted)Closes #168