[BSmick6]

HUMAN:
My first PR in this repo. Adding new endpoints for PUT/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

• Add PUT /api/conversations/{id}/tags/{key} — set or overwrite one tag without touching others
• Add DELETE /api/conversations/{id}/tags/{key} — remove one tag (404 if key absent)
• Neither endpoint bumps updated_at; both fire webhook notifications matching PATCH behavior

Issue Number

Closes #4577

How to Test

Run the tag-specific test suite:

uv run pytest tests/agent_server/test_conversation_tags.py -v

All 16 tests should pass. The new cases are:

• test_set_conversation_tag — happy path
• test_set_conversation_tag_missing_conversation — 404
• test_set_conversation_tag_invalid_key — 422
• test_delete_conversation_tag — happy path
• test_delete_conversation_tag_invalid_key — 422
• test_delete_conversation_tag_missing_conversation — 404 with distinct detail
• test_delete_conversation_tag_missing_key — 404 with distinct detail
• test_set_tag_does_not_affect_other_tags — verifies single-key call

Video/Screenshots

N/A (no UI changes)

Design Doc

N/A

Type

• Bug fix
• Feature
• Refactor
• Breaking change
• Docs / chore

Notes

The full-replace PATCH /api/conversations/{id} behavior is unchanged. Downstream work required before agent-canvas (OpenHands/OpenHands) can consume these endpoints:

1. @openhands/typescript-client — add setConversationTag(conversationId, key, value) and deleteConversationTag(conversationId, key) methods to ConversationClient, mirroring the new endpoints.
2. agent-canvas (fix: migrate archived conversations from localStorage to server-side tags OpenHands#16809) — update AgentServerConversationService to use the new per-key methods, reinstate the localStorage migration (dropped from #16809 specifically because the old full-replace PATCH bumps updated_at), and simplify updateConversationRepository (feat: persist workspace/repo metadata as server-side conversation tags OpenHands#16666) to drop its read-modify-write pattern.

Jev-Fast-Audit

⚡ Jev fast audit · estimates · 0.39s · commit adcf6c5
Strongest signal: No primary concern selected.
Evidence: No primary concern to locate.
Coverage: complete supplied coverage; 12/12 hunks, 6/6 files.

All estimates and evidence

Estimate	Likelihood / value	Direct evidence
SQL injection	3.0%	No direct hunk selected
Command injection	3.0%	No direct hunk selected
Weakened authentication	8.0%	No direct hunk selected
Weakened authorization	22.0%	No direct hunk selected
Contract regression	20.0%	F005H001 · openhands-sdk/openhands/sdk/conversation/types.py:26–32
Data loss	12.0%	F003H001 · openhands-agent-server/openhands/agent_server/conversation_service.py:1910–1981
Sensitive data disclosure	6.0%	No direct hunk selected
Unexpected data transfer	4.0%	No direct hunk selected
Credential misuse	4.0%	No direct hunk selected
Untrusted instruction authority	2.0%	No direct hunk selected
Package source redirection	3.0%	No direct hunk selected
Unverified remote execution	2.0%	No direct hunk selected
Privileged environment access	7.0%	No direct hunk selected
Security assessment bypass	4.0%	No direct hunk selected
Prohibited workload	2.0%	No direct hunk selected
Primary concern	None selected; confidence 78.0%	No primary concern to locate

[all-hands-bot]

✅ Review complete.

This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here.

[all-hands-bot]

🟡 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 in set_conversation_tag and delete_conversation_tag (state sync via _update_state_tags_sync, record cache invalidation, save_meta call) has no integration tests. By contrast, update_conversation has dedicated service-level tests in test_conversation_service.py (including a test_update_conversation_tags_uses_state_lock that verifies the state lock is held). The same coverage is needed here.

• Missing test_delete_conversation_tag_invalid_key — test_set_conversation_tag_invalid_key exists 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:

1. Add a .agents/skills/custom-codereview-guide.md file to your branch (or edit it if one already exists) with the /codereview trigger 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.
2. Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
3. 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 /iterate to automatically drive this PR through CI, review, and QA until it's merge-ready.

Was this review helpful? React with 👍 or 👎 to give feedback.

[all-hands-bot]

👋 This PR needs a couple of things fixed before OpenHands can review it:

• the PR description's HUMAN: section needs at least 20 characters describing what you tested, not just the template placeholder

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.

[all-hands-bot]

🚦 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 @all-hands-bot as a reviewer to have it reviewed regardless of CI status.)

This is an automated check - no AI was used to generate this comment.

[all-hands-bot]

✅ Review complete.

This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here.

[all-hands-bot]

Test review with updated line numbers

[all-hands-bot]

🟡 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_tag and delete_conversation_tag share ~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 mocks ConversationService, 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 in set_conversation_tag / delete_conversation_tag, no test will catch it. A single integration-style test that calls the real service against a lightweight EventService + 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_at intentionally 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:

1. Add a .agents/skills/custom-codereview-guide.md file to your branch (or edit it if one already exists) with the /codereview trigger and the context the reviewer is missing. See the customization docs for the required frontmatter format.
2. Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
3. 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 /iterate to automatically drive this PR through CI, review, and QA until it's merge-ready.

Was this review helpful? React with 👍 or 👎 to give feedback.

[all-hands-bot]

🤖 OpenHands is reviewing this PR.

Head commit: 8c78369f3a34c4c49096bc20055421f237d45eb7
View the conversation: https://oss-agent-canvas.ngrok.dev/conversations/31f060aa-0c1b-4440-b2e2-8b72c7ad2efc

This comment was posted by an AI agent (OpenHands).

[all-hands-bot]

This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.

Approved — no material findings.

This is a clean, additive change that resolves a real race: the existing full-replace PATCH /api/conversations/{id} requires a read-modify-write round trip, so two concurrent clients mutating different keys can silently drop one write. The new per-key endpoints eliminate that.

What I verified against the workspace code:

• Router (conversation_router.py): both handlers validate key with TAG_KEY_PATTERN via Path(pattern=...) and return the correct 404/422 distinctions (conversation-not-found vs. key-not-found on DELETE). Return-type handling (bool for PUT, bool | None for DELETE) is clean and unambiguous.
• Service (conversation_service.py): _flush_tags_mutation correctly mirrors update_conversation's state-sync + save_meta() + webhook-notification sequence, and deliberately skips the updated_at bump (documented in AGENTS.md). The per-key mutation runs synchronously on the event loop before any await, so the dict(event_service.stored.tags) snapshot captures all prior mutations — no lost update across keys.
• fullmatch fix (types.py): a genuine hardening. re.match(r"^[a-z0-9]+$", "abc\n") previously accepted a trailing-newline key ($ matches before a trailing newline); fullmatch rejects it. The new regression test pins this so a revert to match is caught immediately. Value length is also correctly bounded by the shared TAG_VALUE_MAX_LENGTH constant in SetTagRequest.
• Tests: prior review feedback is fully addressed — webhook notification, DELETE invalid-key coverage, the _flush_tags_mutation extraction, and a real persistence round-trip (test_set_delete_tag_persists_to_disk asserts both in-memory stored.tags and on-disk meta.json). uv run pytest tests/agent_server/test_conversation_tags.py passes (18/18).

No blocking issues, breaking changes, or eval-risk concerns (REST surface only; no agent/tool/prompt behavior change).

[RISK ASSESSMENT]

• [Overall PR] 🟢 LOW — additive sub-resource endpoints with validation consistent with existing tag handling and adequate test coverage. The one behavioral choice worth a maintainer glance — intentionally not bumping updated_at — is explicitly documented and covered by a test assertion.

[all-hands-bot]

This review was posted by an AI agent (OpenHands).

Scope: in the right repository. These are Agent Server REST endpoints on the canonical /api/** surface, which this repo owns, and the linked issue #4577 is open with ready-for-dev. The router/service/test structure mirrors update_conversation, and the AGENTS.md + OpenAPI expectations from the issue are addressed. No product/architecture gate needed.

One blocking finding (inline) plus one backward-compatibility finding worth a maintainer decision.

1. Concurrent per-key writes can silently drop a sibling key from ConversationState. The PR's whole premise is that per-key updates are atomic; _flush_tags_mutation snapshots stored.tags into a plain dict and then overwrites state.tags wholesale, so two concurrent PUTs race on which snapshot lands last in the source of truth. Reproduced with a real ConversationState and the unmodified service code: 6/60 runs left base_state.json and meta.json disagreeing, and because the resume path keeps the persisted state over the passed tags, the dropped key stays dropped. Details and a suggested fix are in the inline comment.

2. The match → fullmatch change in openhands-sdk/openhands/sdk/conversation/types.py is a validation tightening with no migration path. A persisted conversation whose tag key contains a trailing newline (previously accepted) can no longer be resumed — I verified ConversationState.create(...) and StoredConversation.model_validate_json both raise ValidationError. Repo policy requires a deprecation/compat path for persistence-contract changes; see the inline comment.

Verification I ran on this head (adcf6c52):

• uv run pytest tests/agent_server/test_conversation_tags.py -v → 18 passed.
• uv run pytest tests/agent_server/test_openapi_contract.py tests/sdk/conversation/test_tags.py → 25 passed.
• Confirmed the Path(pattern=...) validation on both handlers rejects invalid keys and trailing-newline keys with 422 (so the route-level validation is fine; the concern in finding 2 is the model-level validator over persisted data).
• Reproduced finding 1 with the real ConversationState/ConversationService path, stubbing only _compose_webhook_conversation_info_sync (irrelevant to the race).

CI state for this head: every PR workflow for adcf6c52 is action_required (fork PR awaiting workflow approval), so there is no Run tests result for this head — only Validate PR description and check-pr-artifacts have completed, both successful. Local runs above are therefore the only executed evidence. The earlier all-hands-bot approvals were on 8c78369f/f320ee2a, which predate the current head.

🔄 CHANGES REQUESTED

[all-hands-bot]

This review was posted by an AI agent (OpenHands).

Review of 0c2e0b2c

Prior findings — verified fixed on this head

1. Race / base_state.json vs meta.json divergence (was blocking). _mutate_state_tag_sync (conversation_service.py:380) now does the read-modify-write and the stored.tags projection inside with state:, and _mutate_conversation_tag runs it via run_in_executor. I re-ran an independent two-concurrent-PUT reproduction against this head: base_state.json and meta.json agree in 60/60 runs (the same script diverged 6/60 on adcf6c52). test_concurrent_tag_sets_keep_every_key now exercises a real ConversationState and deterministically interleaves two writes.
2. fullmatch tightening breaking persisted state (was blocking). openhands-sdk/openhands/sdk/conversation/types.py is byte-identical to main (TAG_KEY_PATTERN.match), so previously persisted keys keep loading; the routes reject malformed keys via Path(pattern=...) instead. Verified at the route: env → 200, abc%0A/ABC/a-b → 422, 257-char value → 422.
3. Missing webhook notification. _mutate_conversation_tag now calls _notify_conversation_webhooks (conversation_service.py:1899), matching update_conversation.

Also verified on this head: both mutations publish a ConversationStateUpdateEvent with key tags (matching F04.patch-tags), updated_at is not bumped, and the new lock usage follows the concurrency guidance in openhands-agent-server/AGENTS.md. Focused tests pass: tests/agent_server/test_conversation_tags.py (18), test_conversation_service.py + test_openapi_contract.py (139), tests/sdk/conversation/test_tags.py + tests/cross/test_verify_agent_server_skill.py (87). CI for this head is green except the unresolved-review-threads gate, which failed at the moment the author replied and whose threads are now all resolved — a stale run, not a code defect.

Material finding

The two new routes have no owner in the repository's agent-server feature map, so the map gate documented in openhands-agent-server/AGENTS.md fails on this head. Details inline.

[all-hands-bot]

This review was posted by an AI agent (OpenHands).

Review of e6460347

Fresh review at this head. The prior CHANGES_REQUESTED at e0db8ac5 is superseded; I re-established the current state before deciding.

Verified fixed on this head

• The two routes now have an F04 owner (F04.tag-key, F04-conversation-lifecycle.md), so the feature-map gate the previous review flagged is resolved. control-agent-server map coverage now reports the same two pre-existing unowned routes as origin/main (POST /api/conversations/{id}/children, POST /api/plugins/fetch) — none introduced here — and map check --no-routes passes.
• openhands-sdk/.../conversation/types.py is unchanged from main, so previously persisted tag keys still load; malformed keys are rejected at the route via Path(pattern=TAG_KEY_PATTERN.pattern). Verified at the route: env → 200; ENV, a-b, env%0A → 422.
• updated_at is not bumped, webhooks fire, and the mutation runs under run_in_executor with the state lock held, matching the async-safety guidance in openhands-agent-server/AGENTS.md.
• Tests: tests/agent_server/test_conversation_tags.py (18) and tests/cross/test_verify_agent_server_skill.py (69) pass. Current-head check runs: 26 success, 4 skipped, 0 failing.

Material finding

A refused per-key write leaves the tag visible and later persists it. The new helper mutates in-memory state before the metadata write, which inverts the ordering update_conversation deliberately uses.

conversation_service.py:387 _mutate_state_tag_sync runs inside with state:, so state.tags = tags goes through ConversationState.__setattr__, which assigns the field and then autosaves. When the save is refused (lost lease) the write guard raises, but the in-memory field is already set; stored.tags = dict(tags) on the next line then runs only if the save succeeded, leaving state.tags and stored.tags disagreeing.

PATCH does not have this problem because it persists first: update_conversation calls await event_service.save_meta(...) before _update_state_tags_sync, so a refused write raises before any state is touched. There is an existing test asserting exactly this invariant for PATCH — test_stale_owner_refused_update_leaves_no_trace (tests/agent_server/test_conversation_service.py:512). Its docstring is "A metadata update refused for lost ownership changes nothing visible."

I reproduced this against the real lease mechanism, mirroring that test for the per-key route (expire the lease, let a second ConversationService take ownership, then set_conversation_tag on the stale owner):

• set_conversation_tag raises ConversationOwnershipLostError (so the client correctly gets a 409), and meta.json/base_state.json are unchanged — the guarded writes themselves behave.
• But GET /api/conversations/{id} and search on the stale service return {"env": "old", "team": "stale"}, and state.tags still carries team.
• The divergence is durable, not transient. After the lease is released, any later metadata-only save flushes it to disk. With a transient meta.json write failure injected instead of a lease loss, a subsequent ordinary PATCH (e.g. a title change) writes {"env": "old", "team": "ghost"} to meta.json — a tag the PUT returned an error for.

This matters for the downstream consumer this PR unblocks: a client that sees a failed per-key tag write and retries, or that reads tags back to decide what to send, can be misled by a tag that was never acknowledged. Suggested fix: do the metadata write before mutating state (or roll the in-memory fields back when the write raises), the same way update_conversation orders it, and add a per-key analogue of test_stale_owner_refused_update_leaves_no_trace.

Everything else in the diff looks sound. Nice work on the lock placement and the feature-map coverage.

🔄 CHANGES REQUESTED

[all-hands-bot]

In-memory mutation is not covered by the write guard.

state.tags = tags goes through ConversationState.__setattr__, which assigns the field and then autosaves. When the save is refused (lost lease) the guard raises, but the in-memory field is already set, so state.tags and stored.tags diverge. update_conversation avoids this by calling save_meta(...) before _update_state_tags_sync, and test_stale_owner_refused_update_leaves_no_trace (tests/agent_server/test_conversation_service.py:512) pins that invariant for PATCH.

Reproduced against the real lease: with the lease expired and a second service owning the conversation, set_conversation_tag raises ConversationOwnershipLostError and disk stays correct, but GET/search on the stale service return {"env": "old", "team": "stale"} and state.tags keeps team. It is durable: a later metadata-only save flushes it — injecting a transient meta.json write failure and then issuing an ordinary title PATCH persists the never-acknowledged tag.

Suggest persisting the metadata first (or rolling the in-memory fields back on failure), as update_conversation does.

[github-actions]

📁 PR Artifacts Notice

This PR contains a .pr/ directory with temporary PR-specific documents. The directory will be automatically removed after this PR merges to main through a cleanup PR.