Skip to content

Commit d761b72

Browse files
authored
Allow completed conversations to enter merging so conversation merge works (#9955)
POST /v1/conversations/merge has two gates that disagree, so every merge request fails. 1. routers/conversations.py:1279 calls validate_merge_compatibility, which rejects the request with a 400 unless EVERY conversation has status == 'completed' (merge_conversations.py:124-128). 2. routers/conversations.py:1285 then calls lifecycle_service.begin_merge on each conversation, which is transition(..., merging) with expected=None. _STATUS_TRANSITIONS has no 'completed' key, so _STATUS_TRANSITIONS.get('completed', set()) is empty and transition raises LifecycleTransitionError at lifecycle.py:164. Validation admits only 'completed'; the table allowed entering 'merging' only from 'in_progress'. Those are mutually exclusive, so a merge that passes validation always raises. LifecycleTransitionError subclasses ValueError and main.py registers no handler for it, so it surfaces as an unhandled 500. begin_merge has exactly one production caller, that endpoint. This regressed in 61c9b04 ("refactor(backend): centralize conversation lifecycle writes"), which introduced _STATUS_TRANSITIONS and replaced the previous unconditional write (conversations_db.update_conversation_status(..., ConversationStatus.merging)) with begin_merge. The new table never got a 'completed' row. That completed -> merging is the intended edge is already documented in the codebase: the failure rollback in merge_conversations.py says "Since source conversations were set to 'merging' status, we need to reset them back to 'completed'", and the reverse edge merging -> completed is already declared. Fix: declare the edge. ConversationStatus.completed.value: {ConversationStatus.merging.value}, Tests (tests/unit/test_conversation_lifecycle_contract.py): - Adds test_merge_admission_and_lifecycle_agree_on_completed, asserting the two gates agree: validate_merge_compatibility accepts completed conversations, and begin_merge then moves one to merging. - Retargets the negative assertion in test_lifecycle_service_allows_only_declared_transitions. It previously asserted that a completed conversation could NOT begin merge, which pinned this bug. It now asserts on processing -> merging, which is genuinely undeclared, so the test keeps its original purpose. Verification (run in backend/): - pytest tests/unit/test_conversation_lifecycle_contract.py: 14 passed - prove-fail: with lifecycle.py reverted to main, the new test fails with "LifecycleTransitionError: invalid lifecycle transition completed->merging" at lifecycle.py:164 - pytest test_merge_validation.py (38 passed), test_merge_conversations_canonical_delete.py (3 passed), test_check_conversation_lifecycle_writes.py (6 passed) - black --line-length 120 --skip-string-normalization --check: clean - pyright utils/conversations/lifecycle.py: 0 errors - scripts/check_module_stub_pollution.py: 0 violations I did not exercise the endpoint against a live backend. The contract test drives begin_merge through the same lifecycle seam the endpoint calls.
1 parent bee280f commit d761b72

2 files changed

Lines changed: 28 additions & 2 deletions

File tree

backend/tests/unit/test_conversation_lifecycle_contract.py

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -115,10 +115,32 @@ def test_lifecycle_service_allows_only_declared_transitions(lifecycle_store):
115115
)
116116

117117
assert lifecycle_service.admit_processing('uid', 'conversation') is True
118-
assert lifecycle_service.complete('uid', 'conversation') is True
119-
assert lifecycle_store.conversation('uid', 'conversation')['status'] == ConversationStatus.completed
118+
# processing -> merging stays undeclared: merge only ever admits completed conversations.
120119
with pytest.raises(lifecycle_service.LifecycleTransitionError, match='invalid lifecycle transition'):
121120
lifecycle_service.begin_merge('uid', 'conversation')
121+
assert lifecycle_service.complete('uid', 'conversation') is True
122+
assert lifecycle_store.conversation('uid', 'conversation')['status'] == ConversationStatus.completed
123+
124+
125+
def test_merge_admission_and_lifecycle_agree_on_completed(lifecycle_store):
126+
"""The two gates in POST /v1/conversations/merge must agree on the admitted status.
127+
128+
validate_merge_compatibility rejects every status except completed, so completed is the only
129+
status that can reach begin_merge. If the transition table omits completed -> merging, every
130+
accepted merge raises LifecycleTransitionError, which is an unhandled 500 and makes the merge
131+
feature unusable. merge_conversations' failure rollback documents the same edge in reverse.
132+
"""
133+
conversations = [
134+
{'id': 'conversation', 'status': ConversationStatus.completed.value},
135+
{'id': 'other', 'status': ConversationStatus.completed.value},
136+
]
137+
is_valid, error_message, _ = validate_merge_compatibility(conversations)
138+
assert (is_valid, error_message) == (True, None)
139+
140+
lifecycle_store.put_conversation('uid', 'conversation', status=ConversationStatus.completed.value, discarded=False)
141+
142+
assert lifecycle_service.begin_merge('uid', 'conversation') is True
143+
assert lifecycle_store.conversation('uid', 'conversation')['status'] == ConversationStatus.merging
122144

123145

124146
def test_generic_lifecycle_field_write_fails_closed(lifecycle_store):

backend/utils/conversations/lifecycle.py

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,10 @@
4444
ConversationStatus.failed.value,
4545
},
4646
ConversationStatus.merging.value: {ConversationStatus.completed.value, ConversationStatus.failed.value},
47+
# Merge admission rejects every status except completed (validate_merge_compatibility), so
48+
# completed is the only status that can reach begin_merge. Without this edge every accepted
49+
# merge raises LifecycleTransitionError. The merging -> completed edge above is its rollback.
50+
ConversationStatus.completed.value: {ConversationStatus.merging.value},
4751
}
4852

4953

0 commit comments

Comments
 (0)