fix(conversations): reject reprocessing a soft-deleted conversation#10270
fix(conversations): reject reprocessing a soft-deleted conversation#10270aryanorastar wants to merge 1 commit into
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Git-on-my-level
left a comment
There was a problem hiding this comment.
Thanks for tightening this up — rejecting soft-deleted conversation tombstones before reprocess is the right direction for protecting deleted user data.
I reviewed the diff statically. The shared is_soft_deleted() predicate keeps discarded conversations revivable while blocking deleted tombstones, and the router guard runs before deserialization/reprocessing so deleted content should not re-enter structured data, memories, or embeddings. The unit coverage also exercises the tombstone case and the discarded regression path.
I’m not formally approving because this touches a user-data/privacy-sensitive reprocessing path and GitHub currently reports the backend unit suite failing (tests/unit/test_smoke_what_matters_now.py). I added security-review / needs-maintainer-review so a human maintainer can validate the tombstone contract and the failing check before merge. No code blocker found in the changed lines from my review.
by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.
kodjima33
left a comment
There was a problem hiding this comment.
fix(conversations): reject reprocessing a soft-deleted conversation — bug fix; held (backend unit suite red)
reprocess_conversation fetches through _get_valid_conversation_by_id, which does not filter soft-deleted tombstones (get_conversation returns them; the helper only 404s on a missing doc), then runs process_conversation with force_process=True — regenerating structured data, action items, memories and embeddings. Reprocessing a tombstone (via a direct API call, or a delete-vs- reprocess race) therefore resurrects content the user deleted back into their memories and derived data. Reject a deleted conversation while still allowing a discarded one, which reprocess intentionally revives. Third instance of the tombstone-eligibility contract (sync BasedHardware#10119, merge BasedHardware#10262); extract the shared is_soft_deleted predicate and converge eligible_merge_target onto it. Verified: pytest test_reprocess_tombstone_guard.py test_sync_merge_target_selection.py test_merge_validation.py -> 49 passed (behaviour-preserving for eligible_merge_target). Failure-Class: none Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
b520d51 to
e1af484
Compare
What
A soft-delete/tombstone data-integrity gap:
POST /v1/conversations/{id}/reprocessregenerates a soft-deleted conversation's derived data — resurrecting content the user deleted.reprocess_conversationfetches through_get_valid_conversation_by_id, which does not filter soft-deleted tombstones (get_conversationreturns them; the helper only 404s on a missing doc). It then runsprocess_conversation(force_process=True, is_reprocess=True), which regenerates structured data, action items, memories and embeddings. So reprocessing a deleted conversation — via a direct API call, or a delete-vs-reprocess race — re-derives the user's memories / action items / embeddings from content they deleted.Fix
Reject a deleted conversation in
reprocess_conversation(404), while still allowing a discarded one — which reprocess intentionally revives (per its docstring, and theeligible_merge_targetcontract). Checked on the raw doc because theConversationmodel does not carrydeleted.Repairing the failure-class boundary
This is the third instance of one tombstone-eligibility contract — a soft-deleted tombstone must not have content operations applied, or deleted data resurfaces:
eligible_merge_target)Per AGENTS.md ("if two or more recent fixes share the cause, add a reusable guard surface"), rather than a third point-fix I extracted the shared
is_soft_deletedpredicate and convergedeligible_merge_targetonto it (behaviour-preserving), so the reprocess guard and the sync guard share one definition. #10262's inline merge check can adopt it too — a small follow-up rather than re-touching that merged path here; formalizing a named failure class is a reasonable registry follow-up.Tests (hermetic)
test_reprocess_tombstone_guard.py— the predicate (deleted → tombstone; discarded / plain / None → not), reprocess rejects a deleted conversation (404,process_conversationnever called), and still allows a discarded one.eligible_merge_targetand merge-validation tests stay green (behaviour-preserving refactor). 49 passed.Product invariants affected
none
Failure-Class: none