feat(agent-server): add per-key tag endpoints POST/DELETE /conversations/{id}/tags/{key} - #4617
feat(agent-server): add per-key tag endpoints POST/DELETE /conversations/{id}/tags/{key}#4617BSmick6 wants to merge 4 commits into
Conversation
…ons/{id}/tags/{key}
Closes OpenHands#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@all-hands.dev>
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟡 Acceptable — Clean, focused addition. The router layer is correct, validation is consistent across both handlers, the three-valued return type from the service is clearly documented, and the overall approach mirrors the existing update_conversation pattern well.
One meaningful behavioral inconsistency and a testing gap at the service layer need to be addressed before merging.
[CRITICAL ISSUES]
See inline comment on set_conversation_tag in conversation_service.py.
[TESTING GAPS]
-
No service-level tests — All new tests are router-layer tests with a mocked
ConversationService. The actual mutation logic inset_conversation_taganddelete_conversation_tag(state sync via_update_state_tags_sync, record cache invalidation,save_metacall) has no integration tests. By contrast,update_conversationhas dedicated service-level tests intest_conversation_service.py(including atest_update_conversation_tags_uses_state_lockthat verifies the state lock is held). The same coverage is needed here. -
Missing
test_delete_conversation_tag_invalid_key—test_set_conversation_tag_invalid_keyexists for POST, but there is no corresponding test for the DELETE handler's 422 path even though both handlers apply identical key validation.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM
New endpoints add surface area on the conversation router. The webhook notification gap (if unintentional) could silently break external integrations that subscribe to conversation changes. The service logic itself is straightforward and follows the established pattern.
VERDICT: ❌ Needs rework — Webhook notification inconsistency must be explicitly resolved (either add the notification call, or document and test the intentional omission), and service-level tests are required.
KEY INSIGHT: The new per-key tag endpoints skip _notify_conversation_webhooks that PATCH /api/conversations/{id} fires on tag changes — this behavioral divergence is undocumented and likely unintentional.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:
- Add a
.agents/skills/custom-codereview-guide.mdfile to your branch (or edit it if one already exists) with the/codereviewtrigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.- Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
- When your PR is merged, the guideline file goes through normal code review by repository maintainers.
Resolve with AI? Install the iterate skill in your agent and run
/iterateto automatically drive this PR through CI, review, and QA until it's merge-ready.Was this review helpful? React with 👍 or 👎 to give feedback.
…d-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 <openhands@all-hands.dev>
|
👋 This PR needs a couple of things fixed before OpenHands can review it:
Push an update once this is addressed and this check re-runs automatically. This is an automated check - no AI was used to generate this comment. |
|
🚦 CI is currently failing on this PR's latest commit. Please fix the failing checks before OpenHands reviews it - this is re-checked automatically once you push a new commit. (A maintainer can also request This is an automated check - no AI was used to generate this comment. |
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Test review with updated line numbers
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟡 Acceptable — feature is correct and well-structured; one real improvement opportunity and a testing gap worth addressing before the pattern solidifies.
[IMPROVEMENT OPPORTUNITIES]
[conversation_service.py, lines 1866–1879 and 1899–1912]Duplication:set_conversation_taganddelete_conversation_tagshare ~10 lines of identical boilerplate — state-sync, cache invalidation,save_meta(), webhook notification. Only the actual mutation differs. Extract a private_flush_tags_mutation(event_service, conversation_id)helper so that future changes (new cache fields, tracing, additional notification targets) only need to happen in one place.
[TESTING GAPS]
[test_conversation_tags.py]Mock-only router coverage: Every route test mocksConversationService, so they test FastAPI wiring and validation logic, but not the service methods themselves. If_update_state_tags_sync,save_meta(), or webhook notification is broken inset_conversation_tag/delete_conversation_tag, no test will catch it. A single integration-style test that calls the real service against a lightweightEventService+ temp dir would give real regression coverage here.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟢 LOW — purely additive new endpoints; no changes to existing paths, data models, or persistence format.updated_atintentionally left untouched (documented in AGENTS.md).
VERDICT:
✅ Worth merging: Core logic is sound, validation and response semantics are correct, and the architectural design is clean.
KEY INSIGHT:
Both service methods are correct but copy-paste the entire persistence-and-notification sequence — that duplication is the single thing worth cleaning up before it drifts.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:
- Add a
.agents/skills/custom-codereview-guide.mdfile to your branch (or edit it if one already exists) with the/codereviewtrigger and the context the reviewer is missing. See the customization docs for the required frontmatter format.- Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
- When your PR is merged, the guideline file goes through normal code review by repository maintainers.
Resolve with AI? Install the iterate skill in your agent and run
/iterateto automatically drive this PR through CI, review, and QA until it's merge-ready.Was this review helpful? React with 👍 or 👎 to give feedback.
…licate tag-write sequence 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 <openhands@all-hands.dev>
2 participants
HUMAN:
My first PR in this repo. Adding new endpoints for
POST/DELETE /api/conversations/{id}/tags/{key}. This is actually a blocker for OpenHands/OpenHands#16809. You can find more details in #4577 (comment) and in the note at the end of OpenHands/OpenHands#16809. Thank you.AGENT:
Why
The existing replaces the entire tag map, so updating a single tag requires a read-modify-write round trip. Two concurrent clients patching different keys will silently drop one write. This adds atomic per-key endpoints that eliminate the race entirely.
Summary
Issue Number
Closes #4577
How to Test
Run the tag-specific test suite:
All 16 tests should pass. The new cases are:
test_set_conversation_tag— happy pathtest_set_conversation_tag_missing_conversation— 404test_set_conversation_tag_invalid_key— 422test_delete_conversation_tag— happy pathtest_delete_conversation_tag_invalid_key— 422test_delete_conversation_tag_missing_conversation— 404 with distinct detailtest_delete_conversation_tag_missing_key— 404 with distinct detailtest_set_tag_does_not_affect_other_tags— verifies single-key callVideo/Screenshots
N/A (no UI changes)
Design Doc
N/A
Type
Notes
The full-replace
PATCH /api/conversations/{id}behavior is unchanged. Downstream work required before agent-canvas (OpenHands/OpenHands) can consume these endpoints:@openhands/typescript-client— addsetConversationTag(conversationId, key, value)anddeleteConversationTag(conversationId, key)methods toConversationClient, mirroring the new endpoints.AgentServerConversationServiceto use the new per-key methods, reinstate the localStorage migration (dropped from #16809 specifically because the old full-replace PATCH bumpsupdated_at), and simplifyupdateConversationRepository(feat: persist workspace/repo metadata as server-side conversation tags OpenHands#16666) to drop its read-modify-write pattern.