mirror of
https://github.com/All-Hands-AI/OpenHands.git
synced 2026-01-09 14:57:59 -05:00
[Fix]: Broken links from cloud resolver (#8923)
Co-authored-by: openhands <openhands@all-hands.dev>
This commit is contained in:
@@ -505,10 +505,7 @@ class GitHubService(BaseGitService, GitService):
|
||||
)
|
||||
|
||||
# Return the HTML URL of the created PR
|
||||
if 'html_url' in response:
|
||||
return response['html_url']
|
||||
else:
|
||||
return f'PR created but URL not found in response: {response}'
|
||||
return response['html_url']
|
||||
|
||||
|
||||
|
||||
|
||||
@@ -500,12 +500,8 @@ class GitLabService(BaseGitService, GitService):
|
||||
url=url, params=payload, method=RequestMethod.POST
|
||||
)
|
||||
|
||||
# Return the web URL of the created MR
|
||||
if 'web_url' in response:
|
||||
return response['web_url']
|
||||
else:
|
||||
return f'MR created but URL not found in response: {response}'
|
||||
|
||||
return response['web_url']
|
||||
|
||||
|
||||
|
||||
|
||||
@@ -15,4 +15,3 @@ When you're done, make sure to
|
||||
2. Use the `create_pr` tool to open a new PR
|
||||
3. Name the branch using `openhands/` as a prefix (e.g `openhands/update-readme`)
|
||||
4. The PR description should mention that it "fixes" or "closes" the issue number
|
||||
5. Make sure to leave the following sentence at the end of the PR description: `@{{ username }} can click here to [continue refining the PR]({{ conversation_url }})`
|
||||
|
||||
@@ -9,4 +9,3 @@ When you're done, make sure to
|
||||
|
||||
1. Use the `create_pr` tool to open a new PR
|
||||
2. The PR description should mention that it "fixes" or "closes" the issue number
|
||||
3. Make sure to leave the following sentence at the end of the PR description: `@{{ username }} can click here to [continue refining the PR]({{ conversation_url }})`
|
||||
|
||||
@@ -15,4 +15,3 @@ When you're done, make sure to
|
||||
2. Use the `create_mr` tool to open a new MR
|
||||
3. Name the branch using `openhands/` as a prefix (e.g `openhands/update-readme`)
|
||||
4. The MR description should mention that it "fixes" or "closes" the issue number
|
||||
5. Make sure to leave the following sentence at the end of the MR description: `@{{ username }} can click here to [continue refining the MR]({{ conversation_url }})`
|
||||
|
||||
@@ -9,4 +9,3 @@ When you're done, make sure to
|
||||
|
||||
1. Use the `create_mr` tool to open a new MR
|
||||
2. The MR description should mention that it "fixes" or "closes" the issue number
|
||||
3. Make sure to leave the following sentence at the end of the MR description: `@{{ username }} can click here to [continue refining the MR]({{ conversation_url }})`
|
||||
|
||||
@@ -5,7 +5,3 @@ These are a list of text messages attached in order of most recent.
|
||||
{{ message }}
|
||||
{% if not loop.last %}\n\n{% endif %}
|
||||
{% endfor %}
|
||||
|
||||
|
||||
If you opened a pull request, please leave the following comment at the end your summary and pull request description
|
||||
`{{ username }} can click here to [continue refining the PR]({{ conversation_url }})`
|
||||
|
||||
@@ -1,3 +1,4 @@
|
||||
import os
|
||||
import re
|
||||
from typing import Annotated
|
||||
|
||||
@@ -10,9 +11,10 @@ from openhands.core.logger import openhands_logger as logger
|
||||
from openhands.integrations.github.github_service import GithubServiceImpl
|
||||
from openhands.integrations.gitlab.gitlab_service import GitLabServiceImpl
|
||||
from openhands.integrations.provider import ProviderToken
|
||||
from openhands.integrations.service_types import ProviderType
|
||||
from openhands.integrations.service_types import GitService, ProviderType
|
||||
from openhands.server.dependencies import get_dependencies
|
||||
from openhands.server.shared import ConversationStoreImpl, config
|
||||
from openhands.server.shared import ConversationStoreImpl, config, server_config
|
||||
from openhands.server.types import AppMode
|
||||
from openhands.server.user_auth import (
|
||||
get_access_token,
|
||||
get_provider_tokens,
|
||||
@@ -20,7 +22,31 @@ from openhands.server.user_auth import (
|
||||
)
|
||||
from openhands.storage.data_models.conversation_metadata import ConversationMetadata
|
||||
|
||||
mcp_server = FastMCP('mcp', stateless_http=True, dependencies=get_dependencies(), mask_error_details=True)
|
||||
mcp_server = FastMCP(
|
||||
'mcp', stateless_http=True, dependencies=get_dependencies(), mask_error_details=True
|
||||
)
|
||||
|
||||
HOST = f'https://{os.getenv("WEB_HOST", "app.all-hands.dev").strip()}'
|
||||
CONVO_URL = HOST + '/{}'
|
||||
|
||||
|
||||
async def get_convo_link(service: GitService, conversation_id: str, body: str) -> str:
|
||||
"""
|
||||
Appends a followup link, in the PR body, to the OpenHands conversation that opened the PR
|
||||
"""
|
||||
|
||||
if server_config.app_mode != AppMode.SAAS:
|
||||
return body
|
||||
|
||||
user = await service.get_user()
|
||||
username = user.login
|
||||
convo_url = CONVO_URL.format(conversation_id)
|
||||
convo_link = (
|
||||
f'@{username} can click here to [continue refining the PR]({convo_url})'
|
||||
)
|
||||
body += f'\n\n{convo_link}'
|
||||
return body
|
||||
|
||||
|
||||
async def save_pr_metadata(
|
||||
user_id: str, conversation_id: str, tool_result: str
|
||||
@@ -84,6 +110,11 @@ async def create_pr(
|
||||
base_domain=github_token.host,
|
||||
)
|
||||
|
||||
try:
|
||||
body = await get_convo_link(github_service, conversation_id, body or '')
|
||||
except Exception as e:
|
||||
logger.warning(f'Failed to append convo link: {e}')
|
||||
|
||||
try:
|
||||
response = await github_service.create_pr(
|
||||
repo_name=repo_name,
|
||||
@@ -97,7 +128,7 @@ async def create_pr(
|
||||
await save_pr_metadata(user_id, conversation_id, response)
|
||||
|
||||
except Exception as e:
|
||||
error = f"Error creating pull request: {e}"
|
||||
error = f'Error creating pull request: {e}'
|
||||
raise ToolError(str(error))
|
||||
|
||||
return response
|
||||
@@ -132,7 +163,7 @@ async def create_mr(
|
||||
else ProviderToken()
|
||||
)
|
||||
|
||||
github_service = GitLabServiceImpl(
|
||||
gitlab_service = GitLabServiceImpl(
|
||||
user_id=github_token.user_id,
|
||||
external_auth_id=user_id,
|
||||
external_auth_token=access_token,
|
||||
@@ -141,7 +172,14 @@ async def create_mr(
|
||||
)
|
||||
|
||||
try:
|
||||
response = await github_service.create_mr(
|
||||
description = await get_convo_link(
|
||||
gitlab_service, conversation_id, description or ''
|
||||
)
|
||||
except Exception as e:
|
||||
logger.warning(f'Failed to append convo link: {e}')
|
||||
|
||||
try:
|
||||
response = await gitlab_service.create_mr(
|
||||
id=id,
|
||||
source_branch=source_branch,
|
||||
target_branch=target_branch,
|
||||
@@ -153,7 +191,7 @@ async def create_mr(
|
||||
await save_pr_metadata(user_id, conversation_id, response)
|
||||
|
||||
except Exception as e:
|
||||
error = f"Error creating merge request: {e}"
|
||||
error = f'Error creating merge request: {e}'
|
||||
raise ToolError(str(error))
|
||||
|
||||
return response
|
||||
|
||||
@@ -124,8 +124,8 @@ async def create_new_conversation(
|
||||
image_urls=image_urls or [],
|
||||
)
|
||||
|
||||
if attach_convo_id and conversation_instructions:
|
||||
conversation_instructions = conversation_instructions.format(conversation_id)
|
||||
if attach_convo_id:
|
||||
logger.warning('Attaching convo ID is deprecated, skipping process')
|
||||
|
||||
agent_loop_info = await conversation_manager.maybe_start_agent_loop(
|
||||
conversation_id,
|
||||
|
||||
86
tests/unit/test_mcp_routes.py
Normal file
86
tests/unit/test_mcp_routes.py
Normal file
@@ -0,0 +1,86 @@
|
||||
from unittest.mock import AsyncMock, patch
|
||||
|
||||
import pytest
|
||||
|
||||
from openhands.integrations.service_types import GitService
|
||||
from openhands.server.routes.mcp import get_convo_link
|
||||
from openhands.server.types import AppMode
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_get_convo_link_non_saas_mode():
|
||||
"""Test get_convo_link in non-SAAS mode."""
|
||||
# Mock GitService
|
||||
mock_service = AsyncMock(spec=GitService)
|
||||
|
||||
# Test with non-SAAS mode
|
||||
with patch('openhands.server.routes.mcp.server_config') as mock_config:
|
||||
mock_config.app_mode = AppMode.OSS
|
||||
|
||||
# Call the function
|
||||
result = await get_convo_link(
|
||||
service=mock_service, conversation_id='test-convo-id', body='Original body'
|
||||
)
|
||||
|
||||
# Verify the result
|
||||
assert result == 'Original body'
|
||||
# Verify that get_user was not called
|
||||
mock_service.get_user.assert_not_called()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_get_convo_link_saas_mode():
|
||||
"""Test get_convo_link in SAAS mode."""
|
||||
# Mock GitService and user
|
||||
mock_service = AsyncMock(spec=GitService)
|
||||
mock_user = AsyncMock()
|
||||
mock_user.login = 'testuser'
|
||||
mock_service.get_user.return_value = mock_user
|
||||
|
||||
# Test with SAAS mode
|
||||
with (
|
||||
patch('openhands.server.routes.mcp.server_config') as mock_config,
|
||||
patch('openhands.server.routes.mcp.CONVO_URL', 'https://test.example.com/{}'),
|
||||
):
|
||||
mock_config.app_mode = AppMode.SAAS
|
||||
|
||||
# Call the function
|
||||
result = await get_convo_link(
|
||||
service=mock_service, conversation_id='test-convo-id', body='Original body'
|
||||
)
|
||||
|
||||
# Verify the result
|
||||
expected_link = '@testuser can click here to [continue refining the PR](https://test.example.com/test-convo-id)'
|
||||
assert result == f'Original body\n\n{expected_link}'
|
||||
|
||||
# Verify that get_user was called
|
||||
mock_service.get_user.assert_called_once()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_get_convo_link_empty_body():
|
||||
"""Test get_convo_link with an empty body."""
|
||||
# Mock GitService and user
|
||||
mock_service = AsyncMock(spec=GitService)
|
||||
mock_user = AsyncMock()
|
||||
mock_user.login = 'testuser'
|
||||
mock_service.get_user.return_value = mock_user
|
||||
|
||||
# Test with SAAS mode and empty body
|
||||
with (
|
||||
patch('openhands.server.routes.mcp.server_config') as mock_config,
|
||||
patch('openhands.server.routes.mcp.CONVO_URL', 'https://test.example.com/{}'),
|
||||
):
|
||||
mock_config.app_mode = AppMode.SAAS
|
||||
|
||||
# Call the function
|
||||
result = await get_convo_link(
|
||||
service=mock_service, conversation_id='test-convo-id', body=''
|
||||
)
|
||||
|
||||
# Verify the result
|
||||
expected_link = '@testuser can click here to [continue refining the PR](https://test.example.com/test-convo-id)'
|
||||
assert result == f'\n\n{expected_link}'
|
||||
|
||||
# Verify that get_user was called
|
||||
mock_service.get_user.assert_called_once()
|
||||
Reference in New Issue
Block a user