mirror of
https://github.com/OpenHands/OpenHands.git
synced 2026-03-22 13:47:19 +08:00
fix: use project_dir consistently for workspace.working_dir, setup.sh, and git hooks (#13329)
Co-authored-by: openhands <openhands@all-hands.dev>
This commit is contained in:
@@ -1035,7 +1035,7 @@ class TestLoadAndMergeAllSkills:
|
||||
|
||||
# Act
|
||||
result = await service.load_and_merge_all_skills(
|
||||
sandbox, 'owner/repo', '/workspace', 'http://localhost:8000'
|
||||
sandbox, 'owner/repo', '/workspace/repo', 'http://localhost:8000'
|
||||
)
|
||||
|
||||
# Assert
|
||||
@@ -1073,7 +1073,7 @@ class TestLoadAndMergeAllSkills:
|
||||
# Act - pass empty string to simulate no agent server URL
|
||||
# This should still call load_skills_from_agent_server but it will fail
|
||||
result = await service.load_and_merge_all_skills(
|
||||
sandbox, 'owner/repo', '/workspace', ''
|
||||
sandbox, 'owner/repo', '/workspace/repo', ''
|
||||
)
|
||||
|
||||
# Assert - should return empty list when agent_server_url is empty
|
||||
@@ -1089,13 +1089,13 @@ class TestLoadAndMergeAllSkills:
|
||||
@patch(
|
||||
'openhands.app_server.app_conversation.app_conversation_service_base.build_sandbox_config'
|
||||
)
|
||||
async def test_uses_working_dir_when_no_repository(
|
||||
async def test_uses_project_dir_when_no_repository(
|
||||
self,
|
||||
mock_build_sandbox_config,
|
||||
mock_build_org_config,
|
||||
mock_load_skills,
|
||||
):
|
||||
"""Test uses working_dir as project_dir when no repository is selected."""
|
||||
"""Test uses project_dir directly when no repository is selected."""
|
||||
# Arrange
|
||||
mock_user_context = Mock(spec=UserContext)
|
||||
with patch.object(AppConversationServiceBase, '__abstractmethods__', set()):
|
||||
@@ -1164,7 +1164,7 @@ class TestLoadAndMergeAllSkills:
|
||||
|
||||
# Act
|
||||
result = await service.load_and_merge_all_skills(
|
||||
sandbox, 'owner/repo', '/workspace', 'http://localhost:8000'
|
||||
sandbox, 'owner/repo', '/workspace/repo', 'http://localhost:8000'
|
||||
)
|
||||
|
||||
# Assert
|
||||
|
||||
@@ -1199,6 +1199,9 @@ class TestLiveStatusAppConversationService:
|
||||
self.service._configure_llm_and_mcp.assert_called_once_with(
|
||||
self.mock_user, 'gpt-4'
|
||||
)
|
||||
# When selected_repository='test/repo', project_dir is resolved
|
||||
# to '/test/dir/repo' via get_project_dir. All downstream calls
|
||||
# (agent context, workspace, skills) must use this path.
|
||||
self.service._create_agent_with_context.assert_called_once_with(
|
||||
mock_llm,
|
||||
AgentType.DEFAULT,
|
||||
@@ -1207,7 +1210,7 @@ class TestLiveStatusAppConversationService:
|
||||
self.mock_user.condenser_max_size,
|
||||
secrets=mock_secrets,
|
||||
git_provider=ProviderType.GITHUB,
|
||||
working_dir='/test/dir',
|
||||
working_dir='/test/dir/repo',
|
||||
)
|
||||
self.service._finalize_conversation_request.assert_called_once()
|
||||
|
||||
@@ -1989,6 +1992,111 @@ class TestLiveStatusAppConversationService:
|
||||
assert stdio_server['command'] == 'npx'
|
||||
assert stdio_server['env'] == {'TOKEN': 'value'}
|
||||
|
||||
# ------------------------------------------------------------------ #
|
||||
# Regression tests: workspace.working_dir == project_dir #
|
||||
# ------------------------------------------------------------------ #
|
||||
|
||||
def test_get_project_dir_with_repo(self):
|
||||
"""get_project_dir appends repo name to working_dir."""
|
||||
from openhands.app_server.app_conversation.app_conversation_service_base import (
|
||||
get_project_dir,
|
||||
)
|
||||
|
||||
assert (
|
||||
get_project_dir('/workspace/project', 'OpenHands/software-agent-sdk')
|
||||
== '/workspace/project/software-agent-sdk'
|
||||
)
|
||||
assert get_project_dir('/w', 'org/repo-name') == '/w/repo-name'
|
||||
|
||||
def test_get_project_dir_without_repo(self):
|
||||
"""get_project_dir returns working_dir unchanged when no repo selected."""
|
||||
from openhands.app_server.app_conversation.app_conversation_service_base import (
|
||||
get_project_dir,
|
||||
)
|
||||
|
||||
assert get_project_dir('/workspace/project', None) == '/workspace/project'
|
||||
assert get_project_dir('/workspace/project', '') == '/workspace/project'
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_build_request_workspace_uses_project_dir(self):
|
||||
"""workspace.working_dir in StartConversationRequest must equal project_dir.
|
||||
|
||||
This is the root cause of the V1 hook-stop bug: if workspace.working_dir
|
||||
points to the sandbox mount root (/workspace/project) instead of the
|
||||
cloned repo (/workspace/project/<repo>), the agent's CWD is wrong and
|
||||
.openhands/hooks/on_stop.sh is not found.
|
||||
"""
|
||||
self.mock_user_context.get_user_info.return_value = self.mock_user
|
||||
|
||||
mock_secrets = {'GITHUB_TOKEN': Mock()}
|
||||
mock_llm = Mock(spec=LLM)
|
||||
mock_agent = Mock(spec=Agent)
|
||||
|
||||
self.service._setup_secrets_for_git_providers = AsyncMock(
|
||||
return_value=mock_secrets
|
||||
)
|
||||
self.service._configure_llm_and_mcp = AsyncMock(return_value=(mock_llm, {}))
|
||||
self.service._create_agent_with_context = Mock(return_value=mock_agent)
|
||||
|
||||
captured = {}
|
||||
|
||||
async def capture_finalize(
|
||||
agent, conversation_id, user, workspace, *args, **kwargs
|
||||
):
|
||||
captured['workspace_working_dir'] = workspace.working_dir
|
||||
return Mock(spec=StartConversationRequest)
|
||||
|
||||
self.service._finalize_conversation_request = AsyncMock(
|
||||
side_effect=capture_finalize
|
||||
)
|
||||
|
||||
await self.service._build_start_conversation_request_for_user(
|
||||
sandbox=self.mock_sandbox,
|
||||
initial_message=None,
|
||||
system_message_suffix=None,
|
||||
git_provider=None,
|
||||
working_dir='/workspace/project',
|
||||
selected_repository='OpenHands/software-agent-sdk',
|
||||
)
|
||||
|
||||
assert (
|
||||
captured['workspace_working_dir'] == '/workspace/project/software-agent-sdk'
|
||||
), 'workspace.working_dir must point to the repo root, not the sandbox mount'
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_build_request_no_repo_workspace_unchanged(self):
|
||||
"""Without selected_repository, workspace.working_dir == sandbox working_dir."""
|
||||
self.mock_user_context.get_user_info.return_value = self.mock_user
|
||||
|
||||
self.service._setup_secrets_for_git_providers = AsyncMock(return_value={})
|
||||
self.service._configure_llm_and_mcp = AsyncMock(
|
||||
return_value=(Mock(spec=LLM), {})
|
||||
)
|
||||
self.service._create_agent_with_context = Mock(return_value=Mock(spec=Agent))
|
||||
|
||||
captured = {}
|
||||
|
||||
async def capture_finalize(
|
||||
agent, conversation_id, user, workspace, *args, **kwargs
|
||||
):
|
||||
captured['workspace_working_dir'] = workspace.working_dir
|
||||
return Mock(spec=StartConversationRequest)
|
||||
|
||||
self.service._finalize_conversation_request = AsyncMock(
|
||||
side_effect=capture_finalize
|
||||
)
|
||||
|
||||
await self.service._build_start_conversation_request_for_user(
|
||||
sandbox=self.mock_sandbox,
|
||||
initial_message=None,
|
||||
system_message_suffix=None,
|
||||
git_provider=None,
|
||||
working_dir='/workspace/project',
|
||||
selected_repository=None,
|
||||
)
|
||||
|
||||
assert captured['workspace_working_dir'] == '/workspace/project'
|
||||
|
||||
|
||||
class TestPluginHandling:
|
||||
"""Test cases for plugin-related functionality in LiveStatusAppConversationService."""
|
||||
|
||||
Reference in New Issue
Block a user