fix(conversations): reject soft-deleted conversations from merge

[aryanorastar]

What

The user-initiated conversation merge (POST /v1/conversations/merge) validated that sources are unlocked and completed, but never that they are not soft-deleted.

conversations_db.get_conversation returns tombstones unfiltered, and the endpoint only 404s on a missing (None) doc. So a soft-deleted conversation id — passed directly by an API client, or arriving via a delete-vs-merge race — passed both the endpoint's None check and validate_merge_compatibility, and was merged.

Merging a tombstone resurrects deleted content into a new visible conversation: the merge reads the deleted source's transcript/photos/audio, builds a new visible conversation from them, then re-deletes the sources. That is the inverse of the tombstone contract the sync merge path already enforces (conversations_db.eligible_merge_target, #10119 — "a soft-deleted tombstone must never absorb new segments").

Fix

Reject soft-deleted sources in validate_merge_compatibility, mirroring the existing locked/completed gates. This is the direct sibling of #10119: that guarded the automatic sync merge target; this guards the user-initiated merge path. Narrow and behavior-preserving for every non-deleted source.

Verification

• python -m pytest tests/unit/test_merge_validation.py → 40 passed (2 new: rejects a deleted source; still allows non-deleted sources). The new test_rejects_deleted_conversation fails without the guard (validation would return ok=True).

Product invariants affected

none

Failure-Class: none

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[Git-on-my-level]

Thanks for the narrow fix and for adding regression coverage around the validator path. The new validate_merge_compatibility check is directionally right and the current unit test file passes locally (python -m pytest tests/unit/test_merge_validation.py -q → 40 passed).

I do think one privacy/data-deletion edge still needs to be closed before merge: perform_merge_async re-fetches the source conversations later, after the endpoint validation and after the background task has been queued. If one of those source docs becomes a soft-deleted tombstone between admission and the background worker's fetch, the worker still proceeds to merge its transcript/photos/audio into the new visible conversation. That leaves the delete-vs-merge race described in the PR body partially open.

Please add a final deleted-source guard in the background merge path after the worker fetches the source docs and before it merges raw data/copies artifacts, with a focused regression test for “source becomes deleted=True after initial validation/admission”. The failure path should abort the merge rather than creating a visible merged conversation from deleted content.

Once that background-path guard is in place, this looks like a good, well-scoped privacy fix.

---

by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.

[aryanorastar]

Good catch — fixed. perform_merge_async now re-checks for a soft-deleted source right after its background re-fetch, before reading any content: if any source became a tombstone between admission and the worker's fetch, it aborts into _handle_merge_failure rather than building a visible conversation from deleted content. That closes the delete-vs-merge race the admission guard left open.

Regression in test_merge_deleted_source_race.py: asserts the abort (create_processing_conversation never called) when a source is deleted=True after admission, plus a control that a live pair still passes the guard. 46 tests green (incl. the existing merge suites).

[kodjima33]

fix(conversations): reject soft-deleted conversations from merge — bug fix; held (unrelated desktop-swift check red)

[aryanorastar]

@Git-on-my-level the TOCTOU race you flagged is fixed in 8a9740cfbe: perform_merge_async now re-checks deleted on every re-fetched source after admission and aborts via _handle_merge_failure before any content read, so a source soft-deleted between admission and the background fetch can't be resurrected. Regression test test_merge_aborts_when_a_source_is_deleted_after_admission drives the exact race; the control test proves a clean merge still passes the guard. Rebased on latest main. Could you re-review?

Resolved on current head 8a9740c: background merge now re-checks deleted sources after worker fetch and before content merge; regression coverage added.

[Git-on-my-level]

Thanks for the follow-up. I re-reviewed the current head (8a9740cfbe) and the blocker from my earlier review looks resolved: perform_merge_async now re-checks the re-fetched source conversations for deleted=True before transcript/photo/audio merge work starts, and aborts through _handle_merge_failure instead of creating a visible merged conversation from tombstoned content.

I also ran the relevant backend merge unit coverage locally with secrets stripped:

python -m pytest tests/unit/test_merge_validation.py tests/unit/test_merge_deleted_source_race.py tests/unit/test_merge_audio_chunk_copy_failure.py tests/unit/test_merge_conversations_canonical_delete.py -q

Result: 49 passed, 8 warnings.

I’m leaving this as a positive maintainer signal rather than a formal approval because this touches a privacy/data-deletion invariant and the PR is still labeled for security review, but from my pass the requested TOCTOU guard and regression coverage are now in place.

---

by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.

[aryanorastar]

Status after the rebase: the PR Metadata Preflight red is resolved — it was a stale-base artifact (CI diffed the merge commit against a merge-base 434 commits behind, so it saw 334 files and demanded 8 spurious invariant citations). Rebased onto current main, the diff is just the 3 merge files and pr-preflight --suggest reports invariants: none.

The now-failing Hermetic Backend E2E / Merge Gate is unrelated to this PR: the failing case is testing/e2e/test_canonical_memory_pipeline.py::TestSurfaceDefaultAccessMatrix.test_surface_excludes_archive_and_respects_grants[agent_tools] — a canonical memory surface/access-matrix assertion ('coffee fresh short term' short-term memory not surfaced), which this conversation-merge change doesn't touch. It passed on the pre-rebase head and only appeared after rebasing onto current main, so it's a main-side flake/regression in the memory E2E, not this diff.

This PR itself is green on the merge coverage (test_merge_validation.py, test_merge_deleted_source_race.py) and preflight passes 19/19 locally. Could a maintainer re-run the hermetic job (I don't have admin to) or merge past the unrelated memory-E2E failure? kodjima33's approval already stands.