mirror of
https://github.com/OpenHands/OpenHands.git
synced 2026-03-22 13:47:19 +08:00
fix: return empty skills/hooks list instead of 404 for stopped sandboxes
When opening a previously closed conversation, the skills and hooks endpoints were returning 404 because the sandbox is not running. This is incorrect - a closed conversation is still valid, it just doesn't have active skills/hooks. Fix by modifying _get_agent_server_context to return None when sandbox is paused/error/missing (closed conversation), and having endpoints return empty lists. Keep 404 for STARTING state so frontend knows to retry. Co-authored-by: openhands <openhands@all-hands.dev>
This commit is contained in:
@@ -115,7 +115,7 @@ async def _get_agent_server_context(
|
||||
app_conversation_service: AppConversationService,
|
||||
sandbox_service: SandboxService,
|
||||
sandbox_spec_service: SandboxSpecService,
|
||||
) -> AgentServerContext | JSONResponse:
|
||||
) -> AgentServerContext | JSONResponse | None:
|
||||
"""Get the agent server context for a conversation.
|
||||
|
||||
This helper retrieves all necessary information to communicate with the
|
||||
@@ -129,7 +129,8 @@ async def _get_agent_server_context(
|
||||
sandbox_spec_service: Service for sandbox spec operations
|
||||
|
||||
Returns:
|
||||
AgentServerContext if successful, or JSONResponse with error details.
|
||||
AgentServerContext if successful, JSONResponse(404) if conversation
|
||||
not found, or None if sandbox is not running (e.g. closed conversation).
|
||||
"""
|
||||
# Get the conversation info
|
||||
conversation = await app_conversation_service.get_app_conversation(conversation_id)
|
||||
@@ -141,12 +142,20 @@ async def _get_agent_server_context(
|
||||
|
||||
# Get the sandbox info
|
||||
sandbox = await sandbox_service.get_sandbox(conversation.sandbox_id)
|
||||
if not sandbox or sandbox.status != SandboxStatus.RUNNING:
|
||||
if not sandbox:
|
||||
return None
|
||||
# Return None for paused/error/missing sandboxes (closed conversation)
|
||||
if sandbox.status in (
|
||||
SandboxStatus.PAUSED,
|
||||
SandboxStatus.ERROR,
|
||||
SandboxStatus.MISSING,
|
||||
):
|
||||
return None
|
||||
# Return 404 for STARTING state (sandbox not ready yet)
|
||||
if sandbox.status != SandboxStatus.RUNNING:
|
||||
return JSONResponse(
|
||||
status_code=status.HTTP_404_NOT_FOUND,
|
||||
content={
|
||||
'error': f'Sandbox not found or not running for conversation {conversation_id}'
|
||||
},
|
||||
content={'error': f'Sandbox not ready for conversation {conversation_id}'},
|
||||
)
|
||||
|
||||
# Get the sandbox spec to find the working directory
|
||||
@@ -587,6 +596,7 @@ async def get_conversation_skills(
|
||||
|
||||
Returns:
|
||||
JSONResponse: A JSON response containing the list of skills.
|
||||
Returns an empty list if the sandbox is not running.
|
||||
"""
|
||||
try:
|
||||
# Get agent server context (conversation, sandbox, sandbox_spec, agent_server_url)
|
||||
@@ -598,6 +608,8 @@ async def get_conversation_skills(
|
||||
)
|
||||
if isinstance(ctx, JSONResponse):
|
||||
return ctx
|
||||
if ctx is None:
|
||||
return JSONResponse(status_code=status.HTTP_200_OK, content={'skills': []})
|
||||
|
||||
# Load skills from all sources
|
||||
logger.info(f'Loading skills for conversation {conversation_id}')
|
||||
@@ -685,6 +697,7 @@ async def get_conversation_hooks(
|
||||
|
||||
Returns:
|
||||
JSONResponse: A JSON response containing the list of hook event types.
|
||||
Returns an empty list if the sandbox is not running.
|
||||
"""
|
||||
try:
|
||||
# Get agent server context (conversation, sandbox, sandbox_spec, agent_server_url)
|
||||
@@ -696,6 +709,8 @@ async def get_conversation_hooks(
|
||||
)
|
||||
if isinstance(ctx, JSONResponse):
|
||||
return ctx
|
||||
if ctx is None:
|
||||
return JSONResponse(status_code=status.HTTP_200_OK, content={'hooks': []})
|
||||
|
||||
from openhands.app_server.app_conversation.hook_loader import (
|
||||
fetch_hooks_from_agent_server,
|
||||
|
||||
@@ -263,7 +263,7 @@ class TestGetConversationHooks:
|
||||
|
||||
assert response.status_code == status.HTTP_404_NOT_FOUND
|
||||
|
||||
async def test_get_hooks_returns_404_when_sandbox_not_running(self):
|
||||
async def test_get_hooks_returns_empty_list_when_sandbox_not_running(self):
|
||||
conversation_id = uuid4()
|
||||
sandbox_id = str(uuid4())
|
||||
|
||||
@@ -290,4 +290,8 @@ class TestGetConversationHooks:
|
||||
httpx_client=AsyncMock(spec=httpx.AsyncClient),
|
||||
)
|
||||
|
||||
assert response.status_code == status.HTTP_404_NOT_FOUND
|
||||
assert response.status_code == status.HTTP_200_OK
|
||||
import json
|
||||
|
||||
data = json.loads(response.body.decode('utf-8'))
|
||||
assert data == {'hooks': []}
|
||||
|
||||
@@ -198,12 +198,12 @@ class TestGetConversationSkills:
|
||||
assert 'error' in data
|
||||
assert str(conversation_id) in data['error']
|
||||
|
||||
async def test_get_skills_returns_404_when_sandbox_not_found(self):
|
||||
"""Test endpoint returns 404 when sandbox doesn't exist.
|
||||
async def test_get_skills_returns_empty_list_when_sandbox_not_found(self):
|
||||
"""Test endpoint returns empty skills when sandbox doesn't exist.
|
||||
|
||||
Arrange: Setup conversation but no sandbox
|
||||
Act: Call get_conversation_skills endpoint
|
||||
Assert: Response is 404 with sandbox error message
|
||||
Assert: Response is 200 with empty skills list
|
||||
"""
|
||||
# Arrange
|
||||
conversation_id = uuid4()
|
||||
@@ -236,20 +236,19 @@ class TestGetConversationSkills:
|
||||
)
|
||||
|
||||
# Assert
|
||||
assert response.status_code == status.HTTP_404_NOT_FOUND
|
||||
assert response.status_code == status.HTTP_200_OK
|
||||
content = response.body.decode('utf-8')
|
||||
import json
|
||||
|
||||
data = json.loads(content)
|
||||
assert 'error' in data
|
||||
assert 'Sandbox not found' in data['error']
|
||||
assert data == {'skills': []}
|
||||
|
||||
async def test_get_skills_returns_404_when_sandbox_not_running(self):
|
||||
"""Test endpoint returns 404 when sandbox is not in RUNNING state.
|
||||
async def test_get_skills_returns_empty_list_when_sandbox_not_running(self):
|
||||
"""Test endpoint returns empty skills when sandbox is not in RUNNING state.
|
||||
|
||||
Arrange: Setup conversation with stopped sandbox
|
||||
Act: Call get_conversation_skills endpoint
|
||||
Assert: Response is 404 with sandbox not running message
|
||||
Assert: Response is 200 with empty skills list
|
||||
"""
|
||||
# Arrange
|
||||
conversation_id = uuid4()
|
||||
@@ -290,13 +289,12 @@ class TestGetConversationSkills:
|
||||
)
|
||||
|
||||
# Assert
|
||||
assert response.status_code == status.HTTP_404_NOT_FOUND
|
||||
assert response.status_code == status.HTTP_200_OK
|
||||
content = response.body.decode('utf-8')
|
||||
import json
|
||||
|
||||
data = json.loads(content)
|
||||
assert 'error' in data
|
||||
assert 'not running' in data['error']
|
||||
assert data == {'skills': []}
|
||||
|
||||
async def test_get_skills_handles_task_trigger_skills(self):
|
||||
"""Test endpoint correctly handles skills with TaskTrigger.
|
||||
|
||||
Reference in New Issue
Block a user