feat: expose observability fields on app conversations - #130
Conversation
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||
1860de8 to
7e97c22
Compare
juanmichelini
changed the title
Impact assessment: self-hosted / customer installs vs SaaSVerdict: Additive, opt-in, and inert for existing callers — this adds three optional observability fields ( What changed
Environment × flag matrix (before → after)
Validation of data sources: SDK/agent-server support verified live — installed Behaviors that change, and for whom
Why customer / self-hosted installs are safe
Rollout noteThis PR is OPEN (not merged). It reaches surfaces only through a new enterprise-server image build (the PR advertises This analysis was produced by an AI agent (OpenHands) on behalf of @aivong-openhands. |
Mutation review of the added testsCoverage says a line ran; mutation testing says a line is pinned — break the behaviour on purpose and see if the suite notices. I hand-wrote 7 mutants against the PR's changes and ran them with Controls — the tests workThese revert the PR's actual behaviour; all three died, so the direct-builder tests genuinely assert the merge:
What makes them land: Survivors — the gaps
Both new forwarding tests call the builders directly, so nothing exercises the two hops that actually carry the API request into them. M1 — the API→builder wiring is unasserted (highest consequence)The three fields are added to Fix — drive the real flow and assert the builder receives the fields (mirrors the existing @patch('...live_status_app_conversation_service.AsyncRemoteWorkspace')
@patch('...live_status_app_conversation_service.ConversationInfo')
async def test_start_app_conversation_forwards_observability_to_builder(
self, mock_conversation_info_class, mock_remote_workspace_class
):
conversation_id = uuid4()
self.mock_user_context.get_user_id = AsyncMock(return_value='u1')
self.mock_user_context.get_user_info = AsyncMock(return_value=self.mock_user)
mock_sandbox_spec = Mock(spec=SandboxSpecInfo)
mock_sandbox_spec.working_dir = '/test/workspace'
self.mock_sandbox.sandbox_spec_id = str(uuid4())
self.mock_sandbox.id = str(uuid4())
self.mock_sandbox.session_api_key = 'k'
self.mock_sandbox.exposed_urls = [
ExposedUrl(name=AGENT_SERVER, url='http://agent-server:8000', port=60000)
]
self.mock_sandbox_service.get_sandbox = AsyncMock(return_value=self.mock_sandbox)
self.mock_sandbox_spec_service.get_sandbox_spec = AsyncMock(
return_value=mock_sandbox_spec
)
mock_remote_workspace_class.return_value = Mock()
async def mock_wait_for_sandbox(task):
task.sandbox_id = self.mock_sandbox.id
yield task
async def mock_run_setup_scripts(task, sandbox, workspace, agent_server_url, conversation_id):
yield task
self.service._wait_for_sandbox_start = mock_wait_for_sandbox
self.service.run_setup_scripts = mock_run_setup_scripts
mock_agent = Mock(spec=Agent)
mock_agent.llm = Mock(spec=LLM)
mock_agent.llm.model = 'gpt-4'
mock_start_request = Mock(spec=StartConversationRequest)
mock_start_request.agent = mock_agent
mock_start_request.model_dump.return_value = {'test': 'data'}
self.service._build_start_conversation_request_for_user = AsyncMock(
return_value=mock_start_request
)
mock_conversation_info = Mock()
mock_conversation_info.id = conversation_id
mock_conversation_info_class.model_validate.return_value = mock_conversation_info
mock_response = Mock()
mock_response.json.return_value = {'id': str(conversation_id)}
mock_response.raise_for_status = Mock()
self.mock_httpx_client.post = AsyncMock(return_value=mock_response)
self.mock_event_callback_service.save_event_callback = AsyncMock()
request = AppConversationStartRequest(
observability_metadata={'evaluation': 'wb'},
observability_tags=['wb-rubric'],
observability_span_name='mySpanName',
)
async for _ in self.service._start_app_conversation(request):
pass
self.service._build_start_conversation_request_for_user.assert_called_once()
kwargs = self.service._build_start_conversation_request_for_user.call_args.kwargs
assert kwargs['request_observability_metadata'] == {'evaluation': 'wb'}
assert kwargs['request_observability_tags'] == ['wb-rubric']
assert kwargs['request_observability_span_name'] == 'mySpanName'Verified: this passes on the PR branch unmodified and fails with M1 applied. M2 — the ACP routing hop is unasserted
Fix — route through the OpenHands builder with ACP settings and assert the ACP builder receives them: @patch('...live_status_app_conversation_service.get_default_tools', return_value=[])
@pytest.mark.asyncio
async def test_build_request_forwards_observability_to_acp_builder(self, _mock_tools):
from openhands.sdk.settings import ACPAgentSettings
self.mock_user.agent_settings = ACPAgentSettings(
acp_server='claude-code',
llm=LLM(model='claude-sonnet-4-5', api_key=None),
agent_context=None,
)
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=(LLM(model='gpt-4', api_key=SecretStr('k')), {})
)
self.service._resolve_registered_marketplaces = AsyncMock(return_value=None)
sentinel = Mock(spec=StartConversationRequest)
self.service._build_acp_start_conversation_request = AsyncMock(return_value=sentinel)
result = await self.service._build_start_conversation_request_for_user(
sandbox=self.mock_sandbox,
conversation_id=uuid4(),
initial_message=None,
system_message_suffix=None,
git_provider=ProviderType.GITHUB,
working_dir='/test/dir',
remote_workspace=None,
selected_repository='test/repo',
selected_branch='feature-x',
request_observability_span_name='mySpanName',
request_observability_tags=['wb-rubric'],
request_observability_metadata={'evaluation': 'wb'},
)
assert result is sentinel
self.service._build_acp_start_conversation_request.assert_called_once()
kwargs = self.service._build_acp_start_conversation_request.call_args.kwargs
assert kwargs['request_observability_metadata'] == {'evaluation': 'wb'}
assert kwargs['request_observability_tags'] == ['wb-rubric']
assert kwargs['request_observability_span_name'] == 'mySpanName'Verified: this passes on the PR branch unmodified and fails with M2 applied. Not a test gap
This comment was generated by an AI assistant on behalf of the user. |
Suggestion: pin the observability validation contract (negative cases)The added These validators live in the SDK ( import pytest
from pydantic import ValidationError
@pytest.mark.parametrize(
'kwargs',
[
# metadata: mixed-numeric list is not homogeneous (exactly what the
# frontend `number[]` type would wrongly allow)
{'observability_metadata': {'scores': [1, 1.5]}},
# metadata: non-scalar value
{'observability_metadata': {'nested': {'a': 1}}},
# metadata: empty key
{'observability_metadata': {'': 'x'}},
# tags: empty-string tag
{'observability_tags': ['ok', '']},
# tags: not a list
{'observability_tags': 'not-a-list'},
# span name: illegal characters
{'observability_span_name': 'bad name!'},
# span name: exceeds the 128-char limit
{'observability_span_name': 'x' * 129},
],
)
def test_app_conversation_start_request_rejects_invalid_observability_fields(
self, kwargs
):
with pytest.raises(ValidationError):
AppConversationStartRequest(**kwargs)This is ~15 lines in the file you've already touched, no new fixtures or mocks. It's optional/nice-to-have — the runtime validation itself already works; this just guards it from silent drift, which given the frontend/backend contract being hand-maintained is where I'd expect the next regression to come from. This comment was generated by an AI assistant on behalf of the user. |
|
Addressed the reviewer test-coverage concerns in |
|
🚀 Released in 1.51.0. |
|
Screenshot: 
|
3 participants
Summary
Ports OpenHands/sandbox-server#3 to enterprise.
Test plan
Notes:
Enterprise server image for this PR: