fix(#287): memory correctness cleanups (contradiction fallback, note merge, batch dedup)

[sshekhar563]

Resolves #287

Description

Three related correctness gaps in the memory pipeline (found during 2026-07-03 dev audit, commit 2a08e9):

Bugs Fixed

1. Raw-text contradiction fallback stored a sentinel, not a replacement fact

   • manager.py:1163 set superseded_by = "raw-text-contradiction" (a string literal in an ID field) and never stored the new fact
   • Semantic recall filters on superseded_by is None, so the old fact vanished — but no replacement was ever written → knowledge loss
   • Fix: Store the raw text as a real MemoryEntry, use its actual ID for superseded_by, set supersedes back-edge, and append a supersede-audit record

2. note() MERGE had no provenance trail

   • The MERGE branch at soul.py:1604 set old.superseded_by = new_id but never set new.supersedes = old_id and never appended a supersede-audit record
   • _walk_supersedes_chain() relies on the supersedes field, so note-driven merges were completely invisible to provenance walks
   • Fix: Look up the new entry via _memory_lookup_sync(), set supersedes, and append audit record with reason: "note-merge"

3. observe() batch dedup missed within-batch duplicates

   • existing_facts = self._semantic.facts() was snapshotted once at manager.py:1053 before the dedup loop
   • Two near-identical facts extracted from the same interaction (e.g. "User likes Python" and "User enjoys Python") were never compared — both passed dedup and persisted
   • The dream sweeper only catches overlap ≥ 0.85, so 0.6–0.84 band pairs survived indefinitely
   • Fix: Append each stored fact to existing_facts after self.add() in both MERGE and CREATE branches

Changes

File	Change
src/soul_protocol/runtime/memory/manager.py	Bug 1 + Bug 3 fixes, updated file header
src/soul_protocol/runtime/soul.py	Bug 2 fix (note MERGE provenance), updated file header
tests/test_memory/test_correctness_287.py	[NEW] 4 regression tests

Tests

• test_raw_text_contradiction_uses_real_id — verifies no fact has sentinel superseded_by
• test_note_merge_sets_supersedes_backedge — verifies new MERGE entry has supersedes set
• test_note_merge_records_audit_trail — verifies supersede_audit contains the merge record
• test_observe_batch_dedup_appends_to_existing — verifies near-duplicate notes get MERGE/SKIP, not double CREATE

422 existing tests passing, 4 new regression tests passing.

[prakashUXtech]

Good direction, and two of the three items are genuinely fixed in the code. I verified by running your tests and my own reproductions against both dev and this branch, so the following is measured rather than read.

What's solidly right:

• Item 3 (batch dedup) is fully fixed. Appending each stored fact back into the working set is exactly the right shape, and you got the two subtle parts right: facts() returns a fresh sorted list so the append can't corrupt the store, and the elements stay live references so the superseded_by mutation still persists. Measured: "I like Python. I prefer Python for backend work." in one interaction gives 2 live facts on dev and 1 here.
• Item 2's back-edge and audit trail work. supersedes is set and _walk_supersedes_chain now returns a walkable chain where dev returned [].
• Item 1's sentinel is gone and the knowledge no longer vanishes. new_id now carries a resolvable id, so provenance output in diff.py and the CLI resolves properly.

Two blockers:

1. Internal supersession is now written into the public supersede_audit. That list is documented as a user-intent trail in three places: the supersede_audit docstring (manager.py:1971-1982), docs/api-reference.md:508, and docs/cli-reference.md:964, all saying internal supersession (dream-cycle dedup, contradiction resolution) does not append here. After a single observe() this branch appends a verb-fact-conflict record, so the property no longer means what it advertises. The existing guard test passed only because it simulates internal supersession by mutating the entry directly instead of driving observe(). Could you route these to a separate internal list (or tag them and filter the public property), and while you're there add the documented tier and prediction_error fields and use datetime.now(UTC)? Right now the list mixes naive and aware timestamps, so sorting it raises TypeError.

2. One message contradicting N facts stores N identical replacements. The MemoryEntry is constructed inside the per-contradiction loop (manager.py:1174, loop opens at :1165), so I get two byte-identical live facts with different supersedes values from a single interaction. It also bypasses reconcile_fact, so nothing downstream collapses them. That is the exact duplicate-accumulation class item 3 exists to remove.

On the replacement itself, I'd push back on the approach a little. Storing interaction.user_input verbatim promotes raw first-person chat text into the semantic tier, which by convention holds normalized third-person facts. It is unbounded and unsanitized, arrives with no category or abstract, and is hard-coded to importance=5 while superseding an importance-7 fact, so the replacement is more likely to be evicted than what it replaced. The cheaper and safer fix is probably to stop suppressing the old fact when there is no genuine replacement: report the contradiction in the returned dict and let the next extraction settle it. Nothing is lost, and the duplicate problem disappears. If you'd rather keep a replacement, hoist the construction out of the loop, run it through reconcile_fact, normalize it, and bound the length.

Two more before merge:

3. detect_contradictions is still a no-op. soul.py:1515-1516 is byte-identical to dev, TODO comment and all. So item 2 is only half-addressed and #239 is not closed. Either wire ContradictionDetector in here (the detector is already available on the manager, so it's a small addition) or say so plainly in the PR body so the checklist isn't ticked early.

4. Two of the four tests pass on unpatched dev, so they don't guard anything. test_raw_text_contradiction_uses_real_id never calls observe(), so the 4d contradiction path never runs and it asserts a code path that didn't execute didn't misbehave. test_observe_batch_dedup_appends_to_existing also never calls observe(); its first half asserts len(facts) >= 2 after two remember() calls, which is #251's bug asserted as expected behavior, and its second half exercises note()'s single-fact path, which already worked on dev. The two behaviors you actually fixed are the ones left unprotected. Useful triggers: for the 4d path, a user_input that the verb-fact patterns match but FACT_PATTERNS misses ("I reside in Amsterdam now" works); for intra-batch dedup, the Python example above.

Smaller notes: a SOCIAL note-merge writes the audit record but silently skips the back-edge, because _memory_lookup_sync only covers episodic, semantic, and procedural, so the audit claims a supersession the chain can't reconstruct. The note-merge also doesn't set superseded = True on the loser, which the contradiction detector and the procedural curator both branch on. And soul.py:1616 reaches through the façade into self._memory._supersede_audit, which is the coupling #284 is trying to remove.

Finally: the title should follow the repo pattern fix(memory): ... (#287) rather than putting the issue number in the scope slot, and the CHANGELOG plus the two doc pages above need updating since this changes documented behavior.

No regressions anywhere: I ran the full suite both sides and the only delta is your four new tests. The note() provenance work and the batch-dedup fix are both genuinely good, so this is mostly about the raw-text replacement approach and making the tests prove what they claim.

[prakashUXtech]

The three correctness blockers are genuinely resolved, and this time the tests prove it. I re-ran the new file against a reconstructed dev runtime and 6 of 7 tests fail without your changes, so they're real. Specifically: the internal supersession now goes to a separate _internal_supersede_log with the documented tier field and a UTC timestamp, so the public supersede_audit is no longer polluted; the per-contradiction loop no longer constructs a replacement at all, so the duplicate bug is gone by construction; and no raw user_input is written into the semantic tier anymore. That's the hard part done.

What's left:

1. The detect_contradictions block you added in note() (soul.py:1519-1535) is currently dead code. It calls the detector but the result is never returned or read, sets no contradicted_id, and its one test (test_note_detect_contradictions_wired) only asserts action in (CREATE, MERGE, SKIP), which is a tautology that passes on dev too. So it doesn't meet #239's contract and it's the one vacuous test in the file. It also has a live side effect: any semantic note() that trips the verb heuristic silently flips superseded=True on the matched fact. I'd either finish it per #239 (return the contradicted id, set the supersede pointers and prediction_error, gate it on CREATE, reference #239) or drop it from this PR, since it's outside #287's scope. Right now it's the worst of both.

2. A design point worth a decision: both search() and facts() filter recall on superseded_by is None, not the superseded boolean. The report-and-defer path sets superseded=True but leaves superseded_by=None, so a contradicted fact stays fully recallable. That's correct if the goal was only "stop losing knowledge", which it achieves, but it means contradiction resolution is a no-op for retrieval, so after note("User lives in Tokyo") both Berlin and Tokyo stay live. If that's intended, fine; if not, it's a good tracked follow-up rather than expanding this PR.

3. The SOCIAL note-merge still skips the back-edge: _memory_lookup_sync covers episodic, semantic, and procedural but not social, so a social note-merge writes the old entry's superseded_by and the audit record but not the new entry's supersedes. Either extend the lookup to social or skip the forward-pointer and audit for social too, and add a social test.

4. The PR body no longer matches the code after the force-push: it still describes the rejected "store the raw text as a real MemoryEntry" approach and lists two test methods that don't exist. Please rewrite it to describe report-and-defer with the actual test names, add a CHANGELOG line plus a one-line doc note for the new note-merge audit behavior, and retitle to fix(memory): ... (#287).

The correctness work is basically there, so this is mostly about the #239 block (finish or drop) and the paperwork.

[prakashUXtech]

The hard part is behind you here. All three original blockers hold, the dead code is genuinely gone, and the test file is now honest. Reading this as a third round of "the work is wrong" would be the wrong takeaway. What's left is one test and paperwork.

Verified:

• The dead block is gone, including the part that mattered. I specifically checked the live side effect, not just the return value. A repo-wide grep for the superseded boolean assignment now finds exactly one hit, inside the MERGE branch guarded by if e.id == target_id. The path that silently flipped superseded=True on any verb-matched fact is gone, so the invisible blast radius into diff.py counts and contradiction.py is closed. Deferring the real wiring to #239 is the right call.
• The three blockers all still hold: internal supersession goes to _internal_supersede_log and never the public audit; the contradiction loop constructs no replacement entries at all; no raw user_input reaches the semantic tier.
• The social lookup works end to end. I ran a social note-merge: the new entry gets supersedes, the old gets superseded_by and superseded, the audit record lands with tier: "social", and the chain walk returns the link. Procedural behaves identically.
• Test quality is clean now. All 6 fail against dev with substantive assertion errors, not collection errors. The vacuous one is gone.

Two corrections to your summary: the file has 6 tests, not 3. The header's "Remaining tests:" lists three bug groupings, not test counts, so it reads as an undercount. Your "6/6 passing" is the accurate number, and I reproduced it.

Before merge:

1. Add the social-tier note-merge test. This is the one substantive gap. All 6 tests ride the default semantic path; there's no MemoryType or social reference anywhere in the file. The code is right, but nothing pins it, so a future change to _memory_lookup_sync would silently re-break social back-edges.
2. Rewrite the PR body. It's stale on four counts and this one has teeth, because the body becomes the squash commit message and enters permanent history. It still describes Bug 1 as "store the raw text as a real MemoryEntry and append a supersede-audit record", which is the approach we rejected and which your own code comment now explicitly contradicts ("Do NOT store a replacement entry"). It claims 4 new tests when there are 6, and 2 of the 4 named tests don't exist.

Then the smaller ones: a CHANGELOG line; a one-line note in docs/api-reference.md and docs/cli-reference.md (both currently say the supersede audit is "for explicit user intent only" and attribute it solely to supersede() — note-merge is now a second writer and is undocumented); and retitle to fix(memory): ... (#287) per CONTRIBUTING, since your commit already uses the right shape.

Also worth adjusting: "Resolves #287" now over-claims. That issue's second bullet is compound — it names both the detect_contradictions no-op and the missing back-edge/audit. You fixed the latter and correctly deferred the former to #239, so auto-closing #287 would close it with a sub-item outstanding. Either re-word or leave that checkbox open.

Two follow-ups for separate issues, not this PR:

• The docstring above note() still points at #231 in three places, and that issue is closed. The new TODO correctly says #239.
• The recall-filter gap from last round is unchanged and untracked: facts() and search() filter on superseded_by is None while the report-and-defer path sets only superseded=True, so a contradicted fact stays recallable. Worth a tracked issue — and note the social store has no superseded filtering at all, so a merged-away social duplicate stays fully recallable even after a successful merge.