From 0c7ce4ad486fbeeb01ac5f734d2e36de71dc12e4 Mon Sep 17 00:00:00 2001 From: chuckbutkus Date: Mon, 2 Mar 2026 22:37:37 -0500 Subject: [PATCH] V1 Changes to Support Path Based Routing (#13120) Co-authored-by: openhands --- frontend/__tests__/api/v1-git-service.test.ts | 179 ++++++++++++++++-- .../src/api/git-service/v1-git-service.api.ts | 26 ++- .../sandbox/remote_sandbox_service.py | 28 ++- .../sandbox/sandbox_spec_service.py | 2 +- .../app_server/test_remote_sandbox_service.py | 46 ++++- 5 files changed, 241 insertions(+), 40 deletions(-) diff --git a/frontend/__tests__/api/v1-git-service.test.ts b/frontend/__tests__/api/v1-git-service.test.ts index 495a395c81..4111ff7576 100644 --- a/frontend/__tests__/api/v1-git-service.test.ts +++ b/frontend/__tests__/api/v1-git-service.test.ts @@ -1,18 +1,175 @@ -import { test, expect, vi } from "vitest"; +import { describe, test, expect, vi, beforeEach } from "vitest"; import axios from "axios"; import V1GitService from "../../src/api/git-service/v1-git-service.api"; vi.mock("axios"); -test("getGitChanges throws when response is not an array (dead runtime returns HTML)", async () => { - const htmlResponse = "..."; - vi.mocked(axios.get).mockResolvedValue({ data: htmlResponse }); +describe("V1GitService", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); - await expect( - V1GitService.getGitChanges( - "http://localhost:3000/api/conversations/123", - "test-api-key", - "/workspace", - ), - ).rejects.toThrow("Invalid response from runtime"); + describe("getGitChanges", () => { + test("throws when response is not an array (dead runtime returns HTML)", async () => { + const htmlResponse = "..."; + vi.mocked(axios.get).mockResolvedValue({ data: htmlResponse }); + + await expect( + V1GitService.getGitChanges( + "http://localhost:3000/api/conversations/123", + "test-api-key", + "/workspace", + ), + ).rejects.toThrow("Invalid response from runtime"); + }); + + test("uses query parameters instead of path segments for the path", async () => { + vi.mocked(axios.get).mockResolvedValue({ data: [] }); + + await V1GitService.getGitChanges( + "http://localhost:3000/api/conversations/123", + "test-api-key", + "/workspace/project", + ); + + expect(axios.get).toHaveBeenCalledTimes(1); + const [url, config] = vi.mocked(axios.get).mock.calls[0]; + + // URL should NOT contain the path - it should end with /api/git/changes + expect(url).toContain("/api/git/changes"); + expect(url).not.toContain("/workspace/project"); + expect(url).not.toContain(encodeURIComponent("/workspace/project")); + + // Path should be passed as a query parameter + expect(config).toHaveProperty("params"); + expect(config?.params).toEqual({ path: "/workspace/project" }); + }); + + test("preserves slashes in path when using query parameters", async () => { + vi.mocked(axios.get).mockResolvedValue({ data: [] }); + + const pathWithSlashes = "/workspace/project/src/components"; + await V1GitService.getGitChanges( + "http://localhost:3000/api/conversations/123", + "test-api-key", + pathWithSlashes, + ); + + const [, config] = vi.mocked(axios.get).mock.calls[0]; + + // Path should be preserved exactly as provided (slashes intact) + expect(config?.params).toEqual({ path: pathWithSlashes }); + }); + + test("includes session API key in headers when provided", async () => { + vi.mocked(axios.get).mockResolvedValue({ data: [] }); + + await V1GitService.getGitChanges( + "http://localhost:3000/api/conversations/123", + "my-session-key", + "/workspace", + ); + + const [, config] = vi.mocked(axios.get).mock.calls[0]; + expect(config?.headers).toEqual({ "X-Session-API-Key": "my-session-key" }); + }); + + test("maps V1 git statuses to V0 format", async () => { + vi.mocked(axios.get).mockResolvedValue({ + data: [ + { status: "ADDED", path: "new-file.ts" }, + { status: "DELETED", path: "removed-file.ts" }, + { status: "UPDATED", path: "changed-file.ts" }, + { status: "MOVED", path: "renamed-file.ts" }, + ], + }); + + const result = await V1GitService.getGitChanges( + "http://localhost:3000/api/conversations/123", + "test-api-key", + "/workspace", + ); + + expect(result).toEqual([ + { status: "A", path: "new-file.ts" }, + { status: "D", path: "removed-file.ts" }, + { status: "M", path: "changed-file.ts" }, + { status: "R", path: "renamed-file.ts" }, + ]); + }); + }); + + describe("getGitChangeDiff", () => { + test("uses query parameters instead of path segments for the path", async () => { + vi.mocked(axios.get).mockResolvedValue({ + data: { diff: "--- a/file.ts\n+++ b/file.ts\n..." }, + }); + + await V1GitService.getGitChangeDiff( + "http://localhost:3000/api/conversations/123", + "test-api-key", + "/workspace/project/file.ts", + ); + + expect(axios.get).toHaveBeenCalledTimes(1); + const [url, config] = vi.mocked(axios.get).mock.calls[0]; + + // URL should NOT contain the path - it should end with /api/git/diff + expect(url).toContain("/api/git/diff"); + expect(url).not.toContain("/workspace/project/file.ts"); + expect(url).not.toContain(encodeURIComponent("/workspace/project/file.ts")); + + // Path should be passed as a query parameter + expect(config).toHaveProperty("params"); + expect(config?.params).toEqual({ path: "/workspace/project/file.ts" }); + }); + + test("preserves slashes in file path when using query parameters", async () => { + vi.mocked(axios.get).mockResolvedValue({ + data: { diff: "diff content" }, + }); + + const filePath = "/workspace/project/src/components/Button.tsx"; + await V1GitService.getGitChangeDiff( + "http://localhost:3000/api/conversations/123", + "test-api-key", + filePath, + ); + + const [, config] = vi.mocked(axios.get).mock.calls[0]; + + // Path should be preserved exactly as provided (slashes intact) + expect(config?.params).toEqual({ path: filePath }); + }); + + test("includes session API key in headers when provided", async () => { + vi.mocked(axios.get).mockResolvedValue({ + data: { diff: "diff content" }, + }); + + await V1GitService.getGitChangeDiff( + "http://localhost:3000/api/conversations/123", + "my-session-key", + "/workspace/file.ts", + ); + + const [, config] = vi.mocked(axios.get).mock.calls[0]; + expect(config?.headers).toEqual({ "X-Session-API-Key": "my-session-key" }); + }); + + test("returns the diff data from the response", async () => { + const expectedDiff = { + diff: "--- a/file.ts\n+++ b/file.ts\n@@ -1,3 +1,4 @@\n+new line", + }; + vi.mocked(axios.get).mockResolvedValue({ data: expectedDiff }); + + const result = await V1GitService.getGitChangeDiff( + "http://localhost:3000/api/conversations/123", + "test-api-key", + "/workspace/file.ts", + ); + + expect(result).toEqual(expectedDiff); + }); + }); }); diff --git a/frontend/src/api/git-service/v1-git-service.api.ts b/frontend/src/api/git-service/v1-git-service.api.ts index 0c874825aa..6909cf23f7 100644 --- a/frontend/src/api/git-service/v1-git-service.api.ts +++ b/frontend/src/api/git-service/v1-git-service.api.ts @@ -30,7 +30,7 @@ class V1GitService { /** * Get git changes for a V1 conversation - * Uses the agent server endpoint: GET /api/git/changes/{path} + * Uses the agent server endpoint: GET /api/git/changes?path={path} * Maps V1 status types (ADDED, DELETED, etc.) to V0 format (A, D, etc.) * * @param conversationUrl The conversation URL (e.g., "http://localhost:54928/api/conversations/...") @@ -43,15 +43,14 @@ class V1GitService { sessionApiKey: string | null | undefined, path: string, ): Promise { - const encodedPath = encodeURIComponent(path); - const url = this.buildRuntimeUrl( - conversationUrl, - `/api/git/changes/${encodedPath}`, - ); + const url = this.buildRuntimeUrl(conversationUrl, `/api/git/changes`); const headers = buildSessionHeaders(sessionApiKey); // V1 API returns V1GitChangeStatus types, we need to map them to V0 format - const { data } = await axios.get(url, { headers }); + const { data } = await axios.get(url, { + headers, + params: { path }, + }); // Validate response is an array (could be HTML error page if runtime is dead) if (!Array.isArray(data)) { @@ -69,7 +68,7 @@ class V1GitService { /** * Get git change diff for a specific file in a V1 conversation - * Uses the agent server endpoint: GET /api/git/diff/{path} + * Uses the agent server endpoint: GET /api/git/diff?path={path} * * @param conversationUrl The conversation URL (e.g., "http://localhost:54928/api/conversations/...") * @param sessionApiKey Session API key for authentication (required for V1) @@ -81,14 +80,13 @@ class V1GitService { sessionApiKey: string | null | undefined, path: string, ): Promise { - const encodedPath = encodeURIComponent(path); - const url = this.buildRuntimeUrl( - conversationUrl, - `/api/git/diff/${encodedPath}`, - ); + const url = this.buildRuntimeUrl(conversationUrl, `/api/git/diff`); const headers = buildSessionHeaders(sessionApiKey); - const { data } = await axios.get(url, { headers }); + const { data } = await axios.get(url, { + headers, + params: { path }, + }); return data; } } diff --git a/openhands/app_server/sandbox/remote_sandbox_service.py b/openhands/app_server/sandbox/remote_sandbox_service.py index 400c044fce..2a95a3776c 100644 --- a/openhands/app_server/sandbox/remote_sandbox_service.py +++ b/openhands/app_server/sandbox/remote_sandbox_service.py @@ -4,6 +4,7 @@ import logging import os from dataclasses import dataclass from typing import Any, AsyncGenerator, Union +from urllib.parse import urlparse from uuid import UUID import base62 @@ -142,12 +143,13 @@ class RemoteSandboxService(SandboxService): exposed_urls = [] url = runtime.get('url', None) if url: + runtime_id = runtime['runtime_id'] exposed_urls.append( ExposedUrl(name=AGENT_SERVER, url=url, port=AGENT_SERVER_PORT) ) vscode_url = ( - _build_service_url(url, 'vscode') - + f'/?tkn={session_api_key}&folder=%2Fworkspace%2Fproject' + _build_service_url(url, 'vscode', runtime_id) + + f'?tkn={session_api_key}&folder=%2Fworkspace%2Fproject' ) exposed_urls.append( ExposedUrl(name=VSCODE, url=vscode_url, port=VSCODE_PORT) @@ -155,14 +157,14 @@ class RemoteSandboxService(SandboxService): exposed_urls.append( ExposedUrl( name=WORKER_1, - url=_build_service_url(url, 'work-1'), + url=_build_service_url(url, 'work-1', runtime_id), port=WORKER_1_PORT, ) ) exposed_urls.append( ExposedUrl( name=WORKER_2, - url=_build_service_url(url, 'work-2'), + url=_build_service_url(url, 'work-2', runtime_id), port=WORKER_2_PORT, ) ) @@ -663,9 +665,21 @@ class RemoteSandboxService(SandboxService): return results -def _build_service_url(url: str, service_name: str): - scheme, host_and_path = url.split('://') - return scheme + '://' + service_name + '-' + host_and_path +def _build_service_url(url: str, service_name: str, runtime_id: str) -> str: + """Build a service URL for the given service name. + + Handles both path-based and subdomain-based routing: + - Path mode (url path starts with /{runtime_id}): returns {scheme}://{netloc}/{runtime_id}/{service_name} + - Subdomain mode: returns {scheme}://{service_name}-{netloc}{path} + """ + parsed = urlparse(url) + scheme, netloc, path = parsed.scheme, parsed.netloc, parsed.path or '/' + # Path mode if runtime_url path starts with /{id} + path_mode = path.startswith(f'/{runtime_id}') + if path_mode: + return f'{scheme}://{netloc}/{runtime_id}/{service_name}' + else: + return f'{scheme}://{service_name}-{netloc}{path}' async def poll_agent_servers(api_url: str, api_key: str, sleep_interval: int): diff --git a/openhands/app_server/sandbox/sandbox_spec_service.py b/openhands/app_server/sandbox/sandbox_spec_service.py index a8dbe07b93..3d692a031f 100644 --- a/openhands/app_server/sandbox/sandbox_spec_service.py +++ b/openhands/app_server/sandbox/sandbox_spec_service.py @@ -13,7 +13,7 @@ from openhands.sdk.utils.models import DiscriminatedUnionMixin # The version of the agent server to use for deployments. # Typically this will be the same as the values from the pyproject.toml -AGENT_SERVER_IMAGE = 'ghcr.io/openhands/agent-server:010e847-python' +AGENT_SERVER_IMAGE = 'ghcr.io/openhands/agent-server:2c1e72a-python' class SandboxSpecService(ABC): diff --git a/tests/unit/app_server/test_remote_sandbox_service.py b/tests/unit/app_server/test_remote_sandbox_service.py index 113c1ab6e5..7e8a2178dd 100644 --- a/tests/unit/app_server/test_remote_sandbox_service.py +++ b/tests/unit/app_server/test_remote_sandbox_service.py @@ -1196,19 +1196,51 @@ class TestGetSandboxBySessionApiKey: class TestUtilityFunctions: """Test cases for utility functions.""" - def test_build_service_url(self): - """Test _build_service_url function.""" + def test_build_service_url_subdomain_mode(self): + """Test _build_service_url function with subdomain-based routing.""" from openhands.app_server.sandbox.remote_sandbox_service import ( _build_service_url, ) - # Test HTTPS URL - result = _build_service_url('https://sandbox.example.com/path', 'vscode') + # Test HTTPS URL with path (subdomain mode) + result = _build_service_url( + 'https://sandbox.example.com/path', 'vscode', 'runtime-123' + ) assert result == 'https://vscode-sandbox.example.com/path' - # Test HTTP URL - result = _build_service_url('http://localhost:8000', 'work-1') - assert result == 'http://work-1-localhost:8000' + # Test HTTP URL without path (subdomain mode) + result = _build_service_url( + 'http://localhost:8000', 'work-1', 'different-runtime' + ) + assert result == 'http://work-1-localhost:8000/' + + # Test URL with empty path (subdomain mode) + result = _build_service_url('https://sandbox.example.com', 'work-2', 'some-id') + assert result == 'https://work-2-sandbox.example.com/' + + def test_build_service_url_path_mode(self): + """Test _build_service_url function with path-based routing.""" + from openhands.app_server.sandbox.remote_sandbox_service import ( + _build_service_url, + ) + + # Test path-based routing where URL path starts with /{runtime_id} + result = _build_service_url( + 'https://sandbox.example.com/runtime-123', 'vscode', 'runtime-123' + ) + assert result == 'https://sandbox.example.com/runtime-123/vscode' + + # Test path-based routing with work-1 + result = _build_service_url( + 'https://sandbox.example.com/my-runtime-id', 'work-1', 'my-runtime-id' + ) + assert result == 'https://sandbox.example.com/my-runtime-id/work-1' + + # Test path-based routing with work-2 + result = _build_service_url( + 'http://localhost:8080/abc-xyz-123', 'work-2', 'abc-xyz-123' + ) + assert result == 'http://localhost:8080/abc-xyz-123/work-2' def test_hash_session_api_key(self): """Test _hash_session_api_key function."""