diff --git a/openhands-agent-server/AGENTS.md b/openhands-agent-server/AGENTS.md index 5b961871bc..1fcfef8cd3 100644 --- a/openhands-agent-server/AGENTS.md +++ b/openhands-agent-server/AGENTS.md @@ -115,6 +115,30 @@ test) captures the expected behaviour. - In async routes/services, move state-lock acquisition into `run_in_executor(...)` (or another worker-thread boundary) before awaiting network I/O. +## Per-key conversation tag endpoints + +Two sub-resource endpoints allow atomic single-key mutations on the conversation +tag map without a read-modify-write round-trip: + +| Method | Path | Description | +|---|---|---| +| `POST` | `/api/conversations/{id}/tags/{key}` | Set or overwrite one tag; body: `{"value": "..."}` | +| `DELETE` | `/api/conversations/{id}/tags/{key}` | Remove one tag | + +Both operations leave all other tags untouched and **do not update `updated_at`** +on the conversation (tag mutations are metadata-only and must not perturb +sort order or localStorage-to-server migration checks). + +Key constraints: lowercase alphanumeric only (matches `TAG_KEY_PATTERN`); value +up to 256 characters. + +Response codes: +- `200` — success +- `404` — conversation not found, or (DELETE only) key not present +- `422` — key fails the pattern validation + +The existing `PATCH /api/conversations/{id}` full-replace behaviour is unchanged. + ## REST API compatibility & deprecation policy The agent-server **REST API** (the FastAPI OpenAPI surface under `/api/**`) is a diff --git a/openhands-agent-server/openhands/agent_server/conversation_router.py b/openhands-agent-server/openhands/agent_server/conversation_router.py index 6e252f667c..c5961df655 100644 --- a/openhands-agent-server/openhands/agent_server/conversation_router.py +++ b/openhands-agent-server/openhands/agent_server/conversation_router.py @@ -37,6 +37,7 @@ SendMessageRequest, SetConfirmationPolicyRequest, SetSecurityAnalyzerRequest, + SetTagRequest, StartConversationRequest, StartGoalRequest, Success, @@ -46,6 +47,7 @@ ) from openhands.sdk import LLM, Agent, TextContent from openhands.sdk.conversation.state import ConversationExecutionStatus +from openhands.sdk.conversation.types import TAG_KEY_PATTERN from openhands.sdk.marketplace.registry import ( MarketplaceNotFoundError, PluginNotFoundError, @@ -611,6 +613,62 @@ async def update_conversation( return Success() +@conversation_router.post( + "/{conversation_id}/tags/{key}", + responses={404: {"description": "Conversation not found"}}, +) +async def set_conversation_tag( + conversation_id: UUID, + key: str, + request: SetTagRequest, + conversation_service: ConversationService = Depends(get_conversation_service), +) -> Success: + """Set or overwrite a single tag on a conversation. + + All other tags on the conversation are left unchanged. + Does not update the conversation's ``updated_at`` timestamp. + """ + if not TAG_KEY_PATTERN.match(key): + raise HTTPException( + status.HTTP_422_UNPROCESSABLE_ENTITY, + detail="Tag key must be lowercase alphanumeric only", + ) + updated = await conversation_service.set_conversation_tag( + conversation_id, key, request.value + ) + if not updated: + raise HTTPException(status.HTTP_404_NOT_FOUND) + return Success() + + +@conversation_router.delete( + "/{conversation_id}/tags/{key}", + responses={404: {"description": "Conversation or tag key not found"}}, +) +async def delete_conversation_tag( + conversation_id: UUID, + key: str, + conversation_service: ConversationService = Depends(get_conversation_service), +) -> Success: + """Remove a single tag from a conversation. + + All other tags on the conversation are left unchanged. + Returns 404 if the conversation does not exist or the key is not present. + Does not update the conversation's ``updated_at`` timestamp. + """ + if not TAG_KEY_PATTERN.match(key): + raise HTTPException( + status.HTTP_422_UNPROCESSABLE_ENTITY, + detail="Tag key must be lowercase alphanumeric only", + ) + result = await conversation_service.delete_conversation_tag(conversation_id, key) + if result is False: + raise HTTPException(status.HTTP_404_NOT_FOUND, detail="Conversation not found") + if result is None: + raise HTTPException(status.HTTP_404_NOT_FOUND, detail="Tag key not found") + return Success() + + @conversation_router.post( "/{conversation_id}/ask_agent", responses={404: {"description": "Item not found"}}, diff --git a/openhands-agent-server/openhands/agent_server/conversation_service.py b/openhands-agent-server/openhands/agent_server/conversation_service.py index 4f9abac659..fa2dbc4c5e 100644 --- a/openhands-agent-server/openhands/agent_server/conversation_service.py +++ b/openhands-agent-server/openhands/agent_server/conversation_service.py @@ -1851,6 +1851,61 @@ async def update_conversation( ) return True + async def _flush_tags_mutation( + self, event_service: "EventService", conversation_id: UUID + ) -> None: + loop = asyncio.get_running_loop() + state = await event_service.get_state() + new_tags = dict(event_service.stored.tags) + await loop.run_in_executor(None, _update_state_tags_sync, state, new_tags) + record = self._conversation_records.get(conversation_id) + if record is not None: + record.stored = event_service.stored + record.cached_info = None + await event_service.save_meta() + conversation_info = await loop.run_in_executor( + None, _compose_webhook_conversation_info_sync, event_service.stored, state + ) + await self._notify_conversation_webhooks(conversation_info) + + async def set_conversation_tag( + self, conversation_id: UUID, key: str, value: str + ) -> bool: + """Set a single tag on a conversation without touching ``updated_at``. + + Returns: + bool: True if the tag was set, False if the conversation was not found. + """ + event_service = await self._get_or_load_event_service(conversation_id) + if event_service is None: + return False + + event_service.stored.tags[key] = value + await self._flush_tags_mutation(event_service, conversation_id) + logger.info("Set tag '%s' on conversation %s", key, conversation_id) + return True + + async def delete_conversation_tag( + self, conversation_id: UUID, key: str + ) -> bool | None: + """Remove a single tag from a conversation without touching ``updated_at``. + + Returns: + True if the tag was removed, None if the key did not exist, + False if the conversation was not found. + """ + event_service = await self._get_or_load_event_service(conversation_id) + if event_service is None: + return False + + if key not in event_service.stored.tags: + return None + + del event_service.stored.tags[key] + await self._flush_tags_mutation(event_service, conversation_id) + logger.info("Deleted tag '%s' from conversation %s", key, conversation_id) + return True + async def get_event_service(self, conversation_id: UUID) -> EventService | None: return await self._get_or_load_event_service(conversation_id) diff --git a/openhands-agent-server/openhands/agent_server/models.py b/openhands-agent-server/openhands/agent_server/models.py index 0182ccf1aa..fc990730ab 100644 --- a/openhands-agent-server/openhands/agent_server/models.py +++ b/openhands-agent-server/openhands/agent_server/models.py @@ -468,6 +468,15 @@ class SetSecurityAnalyzerRequest(BaseModel): ) +class SetTagRequest(BaseModel): + """Payload to set a single conversation tag.""" + + value: str = Field( + description="Tag value (arbitrary string, up to 256 characters)", + max_length=256, + ) + + class UpdateConversationRequest(BaseModel): """Payload to update conversation metadata.""" diff --git a/tests/agent_server/test_conversation_tags.py b/tests/agent_server/test_conversation_tags.py index f76a762f5d..9f144d33fe 100644 --- a/tests/agent_server/test_conversation_tags.py +++ b/tests/agent_server/test_conversation_tags.py @@ -1,5 +1,6 @@ """Tests for conversation tags in the API layer.""" +import json from datetime import UTC, datetime from unittest.mock import AsyncMock, MagicMock, patch from uuid import uuid4 @@ -225,6 +226,161 @@ def test_get_conversation_includes_tags( client.app.dependency_overrides.clear() +def test_set_conversation_tag(client, mock_conversation_service): + """POST /tags/{key} sets a single tag without touching others.""" + mock_conversation_service.set_conversation_tag = AsyncMock(return_value=True) + client.app.dependency_overrides[get_conversation_service] = ( + lambda: mock_conversation_service + ) + + conversation_id = uuid4() + try: + response = client.post( + f"/api/conversations/{conversation_id}/tags/env", + json={"value": "production"}, + ) + assert response.status_code == 200 + assert response.json() == {"success": True} + mock_conversation_service.set_conversation_tag.assert_awaited_once_with( + conversation_id, "env", "production" + ) + finally: + client.app.dependency_overrides.clear() + + +def test_set_conversation_tag_missing_conversation(client, mock_conversation_service): + """POST /tags/{key} returns 404 when the conversation does not exist.""" + mock_conversation_service.set_conversation_tag = AsyncMock(return_value=False) + client.app.dependency_overrides[get_conversation_service] = ( + lambda: mock_conversation_service + ) + + conversation_id = uuid4() + try: + response = client.post( + f"/api/conversations/{conversation_id}/tags/env", + json={"value": "production"}, + ) + assert response.status_code == 404 + finally: + client.app.dependency_overrides.clear() + + +def test_set_conversation_tag_invalid_key(client, mock_conversation_service): + """POST /tags/{key} returns 422 for an invalid tag key.""" + client.app.dependency_overrides[get_conversation_service] = ( + lambda: mock_conversation_service + ) + + conversation_id = uuid4() + try: + response = client.post( + f"/api/conversations/{conversation_id}/tags/INVALID-KEY", + json={"value": "value"}, + ) + assert response.status_code == 422 + finally: + client.app.dependency_overrides.clear() + + +def test_delete_conversation_tag(client, mock_conversation_service): + """DELETE /tags/{key} removes a single tag.""" + mock_conversation_service.delete_conversation_tag = AsyncMock(return_value=True) + client.app.dependency_overrides[get_conversation_service] = ( + lambda: mock_conversation_service + ) + + conversation_id = uuid4() + try: + response = client.delete( + f"/api/conversations/{conversation_id}/tags/env", + ) + assert response.status_code == 200 + assert response.json() == {"success": True} + mock_conversation_service.delete_conversation_tag.assert_awaited_once_with( + conversation_id, "env" + ) + finally: + client.app.dependency_overrides.clear() + + +def test_delete_conversation_tag_invalid_key(client, mock_conversation_service): + """DELETE /tags/{key} returns 422 for an invalid tag key.""" + client.app.dependency_overrides[get_conversation_service] = ( + lambda: mock_conversation_service + ) + + conversation_id = uuid4() + try: + response = client.delete( + f"/api/conversations/{conversation_id}/tags/INVALID-KEY", + ) + assert response.status_code == 422 + finally: + client.app.dependency_overrides.clear() + + +def test_delete_conversation_tag_missing_conversation( + client, mock_conversation_service +): + """DELETE /tags/{key} returns 404 when the conversation does not exist.""" + mock_conversation_service.delete_conversation_tag = AsyncMock(return_value=False) + client.app.dependency_overrides[get_conversation_service] = ( + lambda: mock_conversation_service + ) + + conversation_id = uuid4() + try: + response = client.delete( + f"/api/conversations/{conversation_id}/tags/env", + ) + assert response.status_code == 404 + assert "Conversation not found" in response.json()["detail"] + finally: + client.app.dependency_overrides.clear() + + +def test_delete_conversation_tag_missing_key(client, mock_conversation_service): + """DELETE /tags/{key} returns 404 when the key is not present.""" + mock_conversation_service.delete_conversation_tag = AsyncMock(return_value=None) + client.app.dependency_overrides[get_conversation_service] = ( + lambda: mock_conversation_service + ) + + conversation_id = uuid4() + try: + response = client.delete( + f"/api/conversations/{conversation_id}/tags/nosuchkey", + ) + assert response.status_code == 404 + assert "Tag key not found" in response.json()["detail"] + finally: + client.app.dependency_overrides.clear() + + +def test_set_tag_does_not_affect_other_tags(client, mock_conversation_service): + """POST /tags/{key} overwrites only the specified key.""" + mock_conversation_service.set_conversation_tag = AsyncMock(return_value=True) + client.app.dependency_overrides[get_conversation_service] = ( + lambda: mock_conversation_service + ) + + conversation_id = uuid4() + try: + response = client.post( + f"/api/conversations/{conversation_id}/tags/repo", + json={"value": "myorg/myrepo"}, + ) + assert response.status_code == 200 + mock_conversation_service.set_conversation_tag.assert_awaited_once_with( + conversation_id, "repo", "myorg/myrepo" + ) + # The service is responsible for leaving other keys untouched; + # here we verify only one key was passed (not the full tag map). + finally: + client.app.dependency_overrides.clear() + + @pytest.mark.asyncio async def test_event_service_start_forwards_tags_to_local_conversation(tmp_path): """EventService.start() must pass stored tags to LocalConversation. @@ -310,3 +466,53 @@ async def test_event_service_start_forwards_observability_span_name(tmp_path): MockConversation.assert_called_once() call_kwargs = MockConversation.call_args.kwargs assert call_kwargs["observability_span_name"] == "pr_review_evaluation" + + +@pytest.mark.asyncio +async def test_set_delete_tag_persists_to_disk(tmp_path): + """set/delete_conversation_tag mutate stored.tags and persist to meta.json.""" + stored = StoredConversation( + id=uuid4(), + workspace=LocalWorkspace(working_dir=str(tmp_path / "workspace")), + confirmation_policy=NeverConfirm(), + tags={"env": "test"}, + created_at=datetime(2025, 1, 1, 12, 0, 0, tzinfo=UTC), + updated_at=datetime(2025, 1, 1, 12, 0, 0, tzinfo=UTC), + ) + conversations_dir = tmp_path / "conversations" + conv_dir = conversations_dir / stored.id.hex + conv_dir.mkdir(parents=True) + (conv_dir / "meta.json").write_text(stored.model_dump_json()) + + # Build an EventService that bypasses LocalConversation startup. + event_service = EventService( + stored=stored, + conversations_dir=conversations_dir, + agent=None, + ) + mock_state = MagicMock() + mock_state.__enter__ = MagicMock(return_value=mock_state) + mock_state.__exit__ = MagicMock(return_value=False) + event_service._conversation = MagicMock() + event_service._conversation._state = mock_state + + with patch( + "openhands.agent_server.conversation_service" + "._compose_webhook_conversation_info_sync", + return_value=MagicMock(), + ): + async with ConversationService(conversations_dir=conversations_dir) as service: + assert service._event_services is not None + service._event_services[stored.id] = event_service + + result = await service.set_conversation_tag(stored.id, "team", "backend") + assert result is True + assert event_service.stored.tags == {"env": "test", "team": "backend"} + meta = json.loads((conv_dir / "meta.json").read_text()) + assert meta["tags"] == {"env": "test", "team": "backend"} + + result = await service.delete_conversation_tag(stored.id, "env") + assert result is True + assert "env" not in event_service.stored.tags + meta = json.loads((conv_dir / "meta.json").read_text()) + assert "env" not in meta.get("tags", {})