From 663f4b5ee957adc86c39fb650e570463b19cbebf Mon Sep 17 00:00:00 2001 From: openhands Date: Sun, 1 Mar 2026 05:09:18 +0000 Subject: [PATCH 1/3] fix: support both query and path parameters for git API endpoints Add support for both query parameters (preferred) and path parameters (legacy/backwards compatible) for git/changes and git/diff endpoints. Query parameter method (?path=/workspace/project) is preferred as it avoids URL path normalization issues with Traefik/Gateway API where encoded slashes (%2F) in path segments would be decoded and normalized, causing leading slashes to be lost. Path parameter method (/changes/workspace/project) is maintained for backwards compatibility with existing clients. Co-authored-by: openhands --- .../openhands/agent_server/git_router.py | 48 +++-- tests/agent_server/test_git_router.py | 171 ++++++++++++++---- 2 files changed, 167 insertions(+), 52 deletions(-) diff --git a/openhands-agent-server/openhands/agent_server/git_router.py b/openhands-agent-server/openhands/agent_server/git_router.py index 5437580710..9502ae51f3 100644 --- a/openhands-agent-server/openhands/agent_server/git_router.py +++ b/openhands-agent-server/openhands/agent_server/git_router.py @@ -4,7 +4,7 @@ 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 @@ -16,21 +16,43 @@ 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}") +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}") +async def git_diff_path(path: str) -> GitDiff: + """Get git diff using path parameter (legacy, for backwards compatibility).""" + return await _get_git_diff(path) diff --git a/tests/agent_server/test_git_router.py b/tests/agent_server/test_git_router.py index afdca4e925..40537385e4 100644 --- a/tests/agent_server/test_git_router.py +++ b/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() From b7d9edff1dca2d33d680b4b02f9f03157a56ce5f Mon Sep 17 00:00:00 2001 From: Chuck Butkus Date: Mon, 2 Mar 2026 16:31:12 -0500 Subject: [PATCH 2/3] Deprecate old endpoints --- .../openhands/agent_server/git_router.py | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/openhands-agent-server/openhands/agent_server/git_router.py b/openhands-agent-server/openhands/agent_server/git_router.py index 9502ae51f3..cd8a301507 100644 --- a/openhands-agent-server/openhands/agent_server/git_router.py +++ b/openhands-agent-server/openhands/agent_server/git_router.py @@ -10,6 +10,7 @@ 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"]) @@ -39,6 +40,15 @@ async def git_changes_query( @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) @@ -53,6 +63,15 @@ async def git_diff_query( @git_router.get("/diff/{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_diff_path(path: str) -> GitDiff: """Get git diff using path parameter (legacy, for backwards compatibility).""" return await _get_git_diff(path) From 6915f87c570997c33d6eeb40754a6ed52de4cc8d Mon Sep 17 00:00:00 2001 From: Chuck Butkus Date: Mon, 2 Mar 2026 16:32:00 -0500 Subject: [PATCH 3/3] Fix syntax error --- openhands-agent-server/openhands/agent_server/git_router.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openhands-agent-server/openhands/agent_server/git_router.py b/openhands-agent-server/openhands/agent_server/git_router.py index cd8a301507..df31e5737b 100644 --- a/openhands-agent-server/openhands/agent_server/git_router.py +++ b/openhands-agent-server/openhands/agent_server/git_router.py @@ -67,7 +67,7 @@ async def git_diff_query( deprecated_in="1.15.0", removed_in="1.20.0", details=( - "Use the /git/changes endpoint with a query parameter for the path " + "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." ),