fix: use query parameters for git API endpoints to preserve path slashes (OpenHands#2249)

Cherry-pick from upstream 97ab053

Author: zparnold

openhands-agent-server/openhands/agent_server/git_router.py

@@ -4,33 +4,74 @@
 import logging
 from pathlib import Path
 
-from fastapi import APIRouter
+from fastapi import APIRouter, Query
 
 from openhands.agent_server.server_details_router import update_last_execution_time
 from openhands.sdk.git.git_changes import get_git_changes
 from openhands.sdk.git.git_diff import get_git_diff
 from openhands.sdk.git.models import GitChange, GitDiff
+from openhands.sdk.utils.deprecation import deprecated
 
 
 git_router = APIRouter(prefix="/git", tags=["Git"])
 logger = logging.getLogger(__name__)
 
 
-@git_router.get("/changes/{path:path}")
-async def git_changes(
-    path: Path,
-) -> list[GitChange]:
+async def _get_git_changes(path: str) -> list[GitChange]:
+    """Internal helper to get git changes for a given path."""
     update_last_execution_time()
     loop = asyncio.get_running_loop()
-    changes = await loop.run_in_executor(None, get_git_changes, path)
-    return changes
+    return await loop.run_in_executor(None, get_git_changes, Path(path))
 
 
-@git_router.get("/diff/{path:path}")
-async def git_diff(
-    path: Path,
-) -> GitDiff:
+async def _get_git_diff(path: str) -> GitDiff:
+    """Internal helper to get git diff for a given path."""
     update_last_execution_time()
     loop = asyncio.get_running_loop()
-    changes = await loop.run_in_executor(None, get_git_diff, path)
-    return changes
+    return await loop.run_in_executor(None, get_git_diff, Path(path))
+
+
+@git_router.get("/changes")
+async def git_changes_query(
+    path: str = Query(..., description="The git repository path"),
+) -> list[GitChange]:
+    """Get git changes using query parameter (preferred method)."""
+    return await _get_git_changes(path)
+
+
+@git_router.get("/changes/{path:path}")
+@deprecated(
+    deprecated_in="1.15.0",
+    removed_in="1.20.0",
+    details=(
+        "Use the /git/changes endpoint with a query parameter for the path "
+        "instead of a path parameter. This allows for better handling of "
+        "complex paths and is more consistent with other endpoints."
+    ),
+)
+async def git_changes_path(path: str) -> list[GitChange]:
+    """Get git changes using path parameter (legacy, for backwards compatibility)."""
+    return await _get_git_changes(path)
+
+
+@git_router.get("/diff")
+async def git_diff_query(
+    path: str = Query(..., description="The file path to get diff for"),
+) -> GitDiff:
+    """Get git diff using query parameter (preferred method)."""
+    return await _get_git_diff(path)
+
+
+@git_router.get("/diff/{path:path}")
+@deprecated(
+    deprecated_in="1.15.0",
+    removed_in="1.20.0",
+    details=(
+        "Use the /git/diff endpoint with a query parameter for the path "
+        "instead of a path parameter. This allows for better handling of "
+        "complex paths and is more consistent with other endpoints."
+    ),
+)
+async def git_diff_path(path: str) -> GitDiff:
+    """Get git diff using path parameter (legacy, for backwards compatibility)."""
+    return await _get_git_diff(path)

tests/agent_server/test_git_router.py

@@ -18,10 +18,14 @@ def client():
     return TestClient(create_app(config), raise_server_exceptions=False)
 
 
+# =============================================================================
+# Query Parameter Tests (Preferred Method)
+# =============================================================================
+
+
 @pytest.mark.asyncio
-async def test_git_changes_success(client):
-    """Test successful git changes endpoint."""
-    # Mock the get_git_changes function
+async def test_git_changes_query_param_success(client):
+    """Test successful git changes endpoint with query parameter."""
     expected_changes = [
         GitChange(status=GitChangeStatus.ADDED, path=Path("new_file.py")),
         GitChange(status=GitChangeStatus.UPDATED, path=Path("existing_file.py")),
@@ -32,12 +36,11 @@ async def test_git_changes_success(client):
         mock_git_changes.return_value = expected_changes
 
         test_path = "src/test_repo"
-        response = client.get(f"/api/git/changes/{test_path}")
+        response = client.get("/api/git/changes", params={"path": test_path})
 
         assert response.status_code == 200
         response_data = response.json()
 
-        # Verify the response structure
         assert len(response_data) == 3
         assert response_data[0]["status"] == "ADDED"
         assert response_data[0]["path"] == "new_file.py"
@@ -46,40 +49,64 @@ async def test_git_changes_success(client):
         assert response_data[2]["status"] == "DELETED"
         assert response_data[2]["path"] == "old_file.py"
 
-        # Verify the mock was called correctly
         mock_git_changes.assert_called_once_with(Path(test_path))
 
 
 @pytest.mark.asyncio
-async def test_git_changes_empty_result(client):
-    """Test git changes endpoint with no changes."""
+async def test_git_changes_query_param_empty_result(client):
+    """Test git changes endpoint with query parameter and no changes."""
     with patch("openhands.agent_server.git_router.get_git_changes") as mock_git_changes:
         mock_git_changes.return_value = []
 
         test_path = "src/empty_repo"
-        response = client.get(f"/api/git/changes/{test_path}")
+        response = client.get("/api/git/changes", params={"path": test_path})
 
         assert response.status_code == 200
         assert response.json() == []
 
 
 @pytest.mark.asyncio
-async def test_git_changes_with_exception(client):
-    """Test git changes endpoint when git operation fails."""
+async def test_git_changes_query_param_with_exception(client):
+    """Test git changes endpoint with query parameter when git operation fails."""
     with patch("openhands.agent_server.git_router.get_git_changes") as mock_git_changes:
         mock_git_changes.side_effect = Exception("Git repository not found")
 
         test_path = "nonexistent/repo"
-        response = client.get(f"/api/git/changes/{test_path}")
+        response = client.get("/api/git/changes", params={"path": test_path})
 
-        # Should return 500 due to exception
         assert response.status_code == 500
 
 
 @pytest.mark.asyncio
-async def test_git_diff_success(client):
-    """Test successful git diff endpoint."""
-    # Mock the get_git_diff function
+async def test_git_changes_missing_path_param(client):
+    """Test git changes endpoint returns 422 when path parameter is missing."""
+    response = client.get("/api/git/changes")
+
+    assert response.status_code == 422
+
+
+@pytest.mark.asyncio
+async def test_git_changes_query_param_absolute_path(client):
+    """Test git changes with query parameter and absolute path (main fix use case)."""
+    expected_changes = [
+        GitChange(status=GitChangeStatus.ADDED, path=Path("new_file.py")),
+    ]
+
+    with patch("openhands.agent_server.git_router.get_git_changes") as mock_git_changes:
+        mock_git_changes.return_value = expected_changes
+
+        # This is the main use case - absolute paths with leading slash
+        test_path = "/workspace/project"
+        response = client.get("/api/git/changes", params={"path": test_path})
+
+        assert response.status_code == 200
+        assert len(response.json()) == 1
+        mock_git_changes.assert_called_once_with(Path(test_path))
+
+
+@pytest.mark.asyncio
+async def test_git_diff_query_param_success(client):
+    """Test successful git diff endpoint with query parameter."""
     expected_diff = GitDiff(
         modified="def new_function():\n    return 'updated'",
         original="def old_function():\n    return 'original'",
@@ -89,74 +116,141 @@ async def test_git_diff_success(client):
         mock_git_diff.return_value = expected_diff
 
         test_path = "src/test_file.py"
-        response = client.get(f"/api/git/diff/{test_path}")
+        response = client.get("/api/git/diff", params={"path": test_path})
 
         assert response.status_code == 200
         response_data = response.json()
 
-        # Verify the response structure
         assert response_data["modified"] == expected_diff.modified
         assert response_data["original"] == expected_diff.original
-
-        # Verify the mock was called correctly (now expects Path object)
         mock_git_diff.assert_called_once_with(Path(test_path))
 
 
 @pytest.mark.asyncio
-async def test_git_diff_with_none_values(client):
-    """Test git diff endpoint with None values."""
-    # Mock the get_git_diff function with None values
+async def test_git_diff_query_param_with_none_values(client):
+    """Test git diff endpoint with query parameter and None values."""
     expected_diff = GitDiff(modified=None, original=None)
 
     with patch("openhands.agent_server.git_router.get_git_diff") as mock_git_diff:
         mock_git_diff.return_value = expected_diff
 
         test_path = "nonexistent_file.py"
-        response = client.get(f"/api/git/diff/{test_path}")
+        response = client.get("/api/git/diff", params={"path": test_path})
 
         assert response.status_code == 200
         response_data = response.json()
 
-        # Verify the response structure
         assert response_data["modified"] is None
         assert response_data["original"] is None
 
 
 @pytest.mark.asyncio
-async def test_git_diff_with_exception(client):
-    """Test git diff endpoint when git operation fails."""
+async def test_git_diff_query_param_with_exception(client):
+    """Test git diff endpoint with query parameter when git operation fails."""
     with patch("openhands.agent_server.git_router.get_git_diff") as mock_git_diff:
         mock_git_diff.side_effect = Exception("Git diff failed")
 
         test_path = "nonexistent/file.py"
-        response = client.get(f"/api/git/diff/{test_path}")
+        response = client.get("/api/git/diff", params={"path": test_path})
 
-        # Should return 500 due to exception
         assert response.status_code == 500
 
 
 @pytest.mark.asyncio
-async def test_git_diff_nested_path(client):
-    """Test git diff endpoint with nested file path."""
-    expected_diff = GitDiff(modified="updated content", original="original content")
+async def test_git_diff_missing_path_param(client):
+    """Test git diff endpoint returns 422 when path parameter is missing."""
+    response = client.get("/api/git/diff")
+
+    assert response.status_code == 422
+
+
+# =============================================================================
+# Path Parameter Tests (Legacy/Backwards Compatibility)
+# =============================================================================
+
+
+@pytest.mark.asyncio
+async def test_git_changes_path_param_success(client):
+    """Test git changes endpoint with path parameter (legacy)."""
+    expected_changes = [
+        GitChange(status=GitChangeStatus.ADDED, path=Path("new_file.py")),
+        GitChange(status=GitChangeStatus.UPDATED, path=Path("existing_file.py")),
+    ]
+
+    with patch("openhands.agent_server.git_router.get_git_changes") as mock_git_changes:
+        mock_git_changes.return_value = expected_changes
+
+        test_path = "src/test_repo"
+        response = client.get(f"/api/git/changes/{test_path}")
+
+        assert response.status_code == 200
+        response_data = response.json()
+
+        assert len(response_data) == 2
+        assert response_data[0]["status"] == "ADDED"
+        assert response_data[1]["status"] == "UPDATED"
+        mock_git_changes.assert_called_once_with(Path(test_path))
+
+
+@pytest.mark.asyncio
+async def test_git_changes_path_param_nested(client):
+    """Test git changes endpoint with nested path parameter."""
+    expected_changes = [
+        GitChange(status=GitChangeStatus.ADDED, path=Path("file.py")),
+    ]
+
+    with patch("openhands.agent_server.git_router.get_git_changes") as mock_git_changes:
+        mock_git_changes.return_value = expected_changes
+
+        test_path = "src/deep/nested/repo"
+        response = client.get(f"/api/git/changes/{test_path}")
+
+        assert response.status_code == 200
+        mock_git_changes.assert_called_once_with(Path(test_path))
+
+
+@pytest.mark.asyncio
+async def test_git_diff_path_param_success(client):
+    """Test git diff endpoint with path parameter (legacy)."""
+    expected_diff = GitDiff(modified="new content", original="old content")
 
     with patch("openhands.agent_server.git_router.get_git_diff") as mock_git_diff:
         mock_git_diff.return_value = expected_diff
 
-        # Test with nested path
-        test_path = "src/utils/helper.py"
+        test_path = "src/test_file.py"
         response = client.get(f"/api/git/diff/{test_path}")
 
         assert response.status_code == 200
+        response_data = response.json()
 
-        # Verify the correct path was passed (now expects Path object)
+        assert response_data["modified"] == "new content"
+        assert response_data["original"] == "old content"
         mock_git_diff.assert_called_once_with(Path(test_path))
 
 
+@pytest.mark.asyncio
+async def test_git_diff_path_param_nested(client):
+    """Test git diff endpoint with nested path parameter."""
+    expected_diff = GitDiff(modified="updated", original="original")
+
+    with patch("openhands.agent_server.git_router.get_git_diff") as mock_git_diff:
+        mock_git_diff.return_value = expected_diff
+
+        test_path = "src/utils/helper.py"
+        response = client.get(f"/api/git/diff/{test_path}")
+
+        assert response.status_code == 200
+        mock_git_diff.assert_called_once_with(Path(test_path))
+
+
+# =============================================================================
+# Additional Edge Case Tests
+# =============================================================================
+
+
 @pytest.mark.asyncio
 async def test_git_changes_with_all_status_types(client):
     """Test git changes endpoint with all possible GitChangeStatus values."""
-    # Test all possible status types
     expected_changes = [
         GitChange(status=GitChangeStatus.ADDED, path=Path("added.py")),
         GitChange(status=GitChangeStatus.UPDATED, path=Path("updated.py")),
@@ -168,7 +262,7 @@ async def test_git_changes_with_all_status_types(client):
         mock_git_changes.return_value = expected_changes
 
         test_path = "src/test_repo"
-        response = client.get(f"/api/git/changes/{test_path}")
+        response = client.get("/api/git/changes", params={"path": test_path})
 
         assert response.status_code == 200
         response_data = response.json()
@@ -183,7 +277,6 @@ async def test_git_changes_with_all_status_types(client):
 @pytest.mark.asyncio
 async def test_git_changes_with_complex_paths(client):
     """Test git changes endpoint with complex file paths."""
-    # Test with various path complexities
     expected_changes = [
         GitChange(
             status=GitChangeStatus.ADDED,
@@ -203,7 +296,7 @@ async def test_git_changes_with_complex_paths(client):
         mock_git_changes.return_value = expected_changes
 
         test_path = "src/complex_repo"
-        response = client.get(f"/api/git/changes/{test_path}")
+        response = client.get("/api/git/changes", params={"path": test_path})
 
         assert response.status_code == 200
         response_data = response.json()