fix(retain): delete the removed chunks when a re-retain only removes content

[nicoloboschi]

The bug

Delta retain classifies a replacement body against the stored chunks as
unchanged / changed / new / removed. Delete the tail of a document and leave the rest
byte-identical, and the diff is changed=0 new=0 removed=N: nothing to extract, but
something to delete.

_try_delta_retain bailed out to the metadata-only path as soon as the extraction list
came back empty:

delta_contents, delta_chunk_map = _build_delta_contents(contents, new_chunks_with_contents, chunks_to_process)

if not delta_contents:          # <- non-empty removed_indices thrown away here
    return await _delta_metadata_only(...)

So the document row advanced to the shrunken body and nothing was deleted. The removed
sections' chunks and facts stayed recallable as part of a document that no longer contained
them — and the state was stable: the stored content_hash now matched the new body, so
re-submitting the same document was a no-op that never revisited the leftovers.

Measured on a 10-section document re-retained with the last 4 sections dropped:

	before	after
within retain_batch_tokens	all 10 sections still stored	6
over retain_batch_tokens (sliced transport)	all 10 sections still stored	6

The same deletion combined with any other edit was always correct — the write path deletes
removed_indices alongside the changed chunks it rewrites — which is what kept this
invisible. It takes a pure deletion to hit. That also makes deleting a section from the
middle correct already: the removal shifts every later chunk, so delta has changed chunks
to extract and takes the write path.

The fix

One condition. Bail out to metadata-only only when there is nothing to write, not merely
nothing to extract:

if not delta_contents and not removed_indices:
    return await _delta_metadata_only(...)

The write path already deletes removed_indices whether or not it also has facts to insert,
and both write paths (Postgres and the store-owned Protocol-B one) handle an empty fact set:
extract_facts_from_contents([]) returns before any LLM call, insert_facts_batch inserts
nothing, and _store_document_bodies re-puts the full new chunk set. A removal-only
re-retain therefore costs zero extraction — asserted below.

Tests

tests/test_delta_removed_chunks.py:

• test_pure_deletion_drops_the_removed_chunks[within_budget] — fails on main
• test_pure_deletion_drops_the_removed_chunks[oversized] — fails on main
• test_pure_deletion_only_deletes — the surviving chunks are byte-identical, so a
  removal-only delta must not send anything back through fact extraction (guards the other
  direction: letting the write path run must not turn this into a full re-ingest)
• test_unchanged_re_retain_still_takes_the_metadata_only_path — a no-op re-retain still
  extracts nothing and keeps every fact
• test_middle_deletion_still_drops_the_removed_section — control for the case that was
  already correct

Verification

• New file: 5 passed here, 2 failed on main (the two repro cases).
• pytest tests/ -k "retain or delta or chunk or document or append or observation or recovery or oversized":
  979 passed. The 2 failures / 29 errors are pre-existing and environmental
  (HINDSIGHT_API_LLM_API_KEY — real-LLM tests); each was confirmed failing identically on
  clean main.
• ruff check / ruff format --check / ty check hindsight_api/ clean; check-unused.sh
  reports nothing new.

Relationship to #3666

#3666 targets the same area but was branched before #3660, which had already fixed the
symptoms it describes (stale-fact accumulation and full re-extraction on an oversized edit
— 4 of its 5 tests pass unmodified on current main). Its remaining test is the shrinking
tail, which is this bug.

#3666's approach — dropping removed_indices for every sliced retain — should not be
merged: on its own head it deletes content that is still in the document. Re-retaining a
10-section oversized document with section 0 edited and section 3 deleted:

BEFORE  00 01 02 03 04 05 06 07 08 09
#3666   00 01 02 03 04 05 06 07 08      deleted section 03 survives; live section 09 destroyed
main    00 01 02    04 05 06 07 08 09   correct
this PR 00 01 02    04 05 06 07 08 09   correct

Suggest closing #3666 in favour of this.

[nicoloboschi]

Reviewed — correct and complete, and the localisation is the part worth stating.

Guard audit. There are four bailouts to _delta_metadata_only, and three already checked removed:

line	guard
3340	not unchanged_indices + strict-append	deliberate — preserves historical chunks; this is the one case where removed must NOT be acted on
3370	not chunks_to_process and not removed_indices	correct
3391	not delta_contents	the bug
3440	not (recheck.changed or recheck.new or recheck.removed)	correct

So the asymmetry was isolated to one guard, and this fixes it without disturbing 3340 — which is the one an over-eager "make them consistent" pass would have broken, since it deliberately drops a non-empty removed set.

Two things I checked and found clean:

• The append path cannot produce removed without changed in the normal case — the prepended stored body classifies as unchanged and the new turn as new — so a pure deletion cannot arise there.
• The store-owned write path needs no counterpart change: chunks_to_delete is built from changed_indices + removed_indices, so once the guard lets execution through, the delete happens on both backends.

On the test. Asserting through list_memory_units on markers is the right level — it catches the symptom (removed sections still recallable under a document that no longer contains them) rather than a chunk-row count, and it runs against either backend. test_pure_deletion_only_deletes is the half I would have omitted: without it, a fix that deletes correctly while re-extracting everything would pass. Parametrising over batch_tokens earns its place too, since the sub-batch splitter changes which code path handles the deletion.

One optional addition: a line where delta_contents is built, saying that it is the EXTRACTION list and not the work list. "Nothing to extract" ≠ "nothing to write" is the conflation that caused this, and the variable name invites it again.

Worth noting for the record: I was in this function earlier fixing a stale comment at the top of it and read straight past this. A paired store-vs-Postgres sweep would not have found it either — it fails identically on both backends, so it is invisible to a differential method. It takes a pure deletion to hit, because any other edit masks it.