From e0db8ac5c0bddc73ee78f3a48cced0d4b5f3a436 Mon Sep 17 00:00:00 2001 From: BSmick6 <14306627+BSmick6@users.noreply.github.com> Date: Mon, 24 Aug 2026 10:37:25 -0400 Subject: [PATCH 1/3] feat(agent-server): add per-key tag endpoints POST/DELETE /conversations/{id}/tags/{key} MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #4577. Adds atomic single-key mutations on the conversation tag map so callers avoid the read-modify-write race on PATCH /conversations/{id}: - POST /api/conversations/{id}/tags/{key} – set or overwrite one tag - DELETE /api/conversations/{id}/tags/{key} – remove one tag (404 if absent) Neither endpoint touches updated_at so tag mutations do not perturb sort order or localStorage-to-server migration checks. Co-authored-by: openhands --- openhands-agent-server/AGENTS.md | 24 +++ .../agent_server/conversation_router.py | 58 ++++++++ .../agent_server/conversation_service.py | 54 +++++++ .../openhands/agent_server/models.py | 9 ++ tests/agent_server/test_conversation_tags.py | 139 ++++++++++++++++++ 5 files changed, 284 insertions(+) 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..af5e76388f 100644 --- a/openhands-agent-server/openhands/agent_server/conversation_service.py +++ b/openhands-agent-server/openhands/agent_server/conversation_service.py @@ -1851,6 +1851,60 @@ async def update_conversation( ) return True + 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 + + loop = asyncio.get_running_loop() + state = await event_service.get_state() + event_service.stored.tags[key] = value + 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() + 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 + + loop = asyncio.get_running_loop() + state = await event_service.get_state() + del event_service.stored.tags[key] + 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() + 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..017fc7fa63 100644 --- a/tests/agent_server/test_conversation_tags.py +++ b/tests/agent_server/test_conversation_tags.py @@ -225,6 +225,145 @@ 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_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. From 5937b543db62b8bf101222c13fc07d08be5c6fc5 Mon Sep 17 00:00:00 2001 From: BSmick6 <14306627+BSmick6@users.noreply.github.com> Date: Mon, 24 Aug 2026 11:16:02 -0400 Subject: [PATCH 2/3] fix(agent-server): add webhook notification and missing DELETE invalid-key test - set_conversation_tag and delete_conversation_tag now call _notify_conversation_webhooks after mutating tags, matching the behavior of update_conversation - add test_delete_conversation_tag_invalid_key to cover 422 on the DELETE handler (mirrors the existing POST invalid-key test) Co-authored-by: openhands --- .../agent_server/conversation_service.py | 8 ++++++++ tests/agent_server/test_conversation_tags.py | 16 ++++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/openhands-agent-server/openhands/agent_server/conversation_service.py b/openhands-agent-server/openhands/agent_server/conversation_service.py index af5e76388f..e467322034 100644 --- a/openhands-agent-server/openhands/agent_server/conversation_service.py +++ b/openhands-agent-server/openhands/agent_server/conversation_service.py @@ -1873,6 +1873,10 @@ async def set_conversation_tag( 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) logger.info("Set tag '%s' on conversation %s", key, conversation_id) return True @@ -1902,6 +1906,10 @@ async def delete_conversation_tag( 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) logger.info("Deleted tag '%s' from conversation %s", key, conversation_id) return True diff --git a/tests/agent_server/test_conversation_tags.py b/tests/agent_server/test_conversation_tags.py index 017fc7fa63..8ea98c736f 100644 --- a/tests/agent_server/test_conversation_tags.py +++ b/tests/agent_server/test_conversation_tags.py @@ -303,6 +303,22 @@ def test_delete_conversation_tag(client, mock_conversation_service): 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 ): From 031b8c0e0ee5943846d13f67867bb4fe091d571b Mon Sep 17 00:00:00 2001 From: BSmick6 <14306627+BSmick6@users.noreply.github.com> Date: Tue, 25 Aug 2026 14:40:58 -0400 Subject: [PATCH 3/3] refactor(conversation-service): extract _flush_tags_mutation to deduplicate tag-write sequence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit set_conversation_tag and delete_conversation_tag shared identical post-mutation blocks (state sync, cache invalidation, save_meta, webhook notification). Extract into _flush_tags_mutation so future changes to the notification or caching sequence only need to happen once. Also adds test_set_delete_tag_persists_to_disk — a service-level test that injects a real EventService (without LocalConversation startup) and asserts both in-memory and on-disk tag state after each mutation. Co-authored-by: openhands --- .../agent_server/conversation_service.py | 45 +++++++--------- tests/agent_server/test_conversation_tags.py | 51 +++++++++++++++++++ 2 files changed, 70 insertions(+), 26 deletions(-) diff --git a/openhands-agent-server/openhands/agent_server/conversation_service.py b/openhands-agent-server/openhands/agent_server/conversation_service.py index e467322034..fa2dbc4c5e 100644 --- a/openhands-agent-server/openhands/agent_server/conversation_service.py +++ b/openhands-agent-server/openhands/agent_server/conversation_service.py @@ -1851,21 +1851,11 @@ async def update_conversation( ) return True - 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 - + async def _flush_tags_mutation( + self, event_service: "EventService", conversation_id: UUID + ) -> None: loop = asyncio.get_running_loop() state = await event_service.get_state() - event_service.stored.tags[key] = value 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) @@ -1877,6 +1867,21 @@ async def set_conversation_tag( 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 @@ -1896,20 +1901,8 @@ async def delete_conversation_tag( if key not in event_service.stored.tags: return None - loop = asyncio.get_running_loop() - state = await event_service.get_state() del event_service.stored.tags[key] - 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) + await self._flush_tags_mutation(event_service, conversation_id) logger.info("Deleted tag '%s' from conversation %s", key, conversation_id) return True diff --git a/tests/agent_server/test_conversation_tags.py b/tests/agent_server/test_conversation_tags.py index 8ea98c736f..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 @@ -465,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", {})